Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add acl resource literal and prefixed matching #215

Merged
merged 1 commit into from
Nov 12, 2024

Conversation

biggusdonzus
Copy link
Contributor

@biggusdonzus biggusdonzus commented Nov 7, 2024

For this feature instead of adding a field resourcePatternType that distinguishes between literal, prefixed and regex (the one used in Aiven ACLs) matching, I added the fields resourceLiteral and resourcePrefixed. I did this way because we already had resource matching splito into resourceRe and resourceRePattern (the later used for backreference regex), and it felt natural (and more backward compatible) adding the new matching in the same way.

In the authorization method, to select between the proper resource matcher, it is used the one who is not null (as was done before my PR). This is a bit fragile by itself, for this reason I added the method validate() to verify that exactly one of the four resource matchers is not null. The validation is called by the constructor and by the decode class AclJsonReader.java.

The literal prefix needed a bit of work for the wildcard match because the wildcard rule is defined like Topic:* or Group:*, etc and the prefix must match the one of the action that we are authorizing, so for example:

Rule Action Result
Topic:* Topic:some-topic true
Topic:* Group:abcde false
Group:* Group:abcde true

I didn't want to introduce some big refactor so I added the class ResourceLiteralWildcardMatcher.java to perform this operation, trying to keep it fast as possible.

@biggusdonzus biggusdonzus force-pushed the fdorlandi-add-literal-and-prefixed-matching branch 5 times, most recently from 17fdce3 to 6826a5d Compare November 11, 2024 13:50
@biggusdonzus biggusdonzus marked this pull request as ready for review November 11, 2024 13:57
@biggusdonzus biggusdonzus requested review from a team as code owners November 11, 2024 13:57
Copy link
Contributor

@tvainika tvainika left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems there are conflicts with hidden flag. I'll look a bit more. Now spotted also some minor issues

@biggusdonzus biggusdonzus force-pushed the fdorlandi-add-literal-and-prefixed-matching branch 2 times, most recently from 3f2deb1 to 0924b2d Compare November 11, 2024 16:42
Copy link
Contributor

@tvainika tvainika left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor things found, but this new exception being thrown with in some cases from validate: I'm not sure what will happen with it. What do you think, is it safe and why?

README.md Outdated Show resolved Hide resolved
src/main/java/io/aiven/kafka/auth/json/AivenAcl.java Outdated Show resolved Hide resolved
@biggusdonzus biggusdonzus force-pushed the fdorlandi-add-literal-and-prefixed-matching branch from 0924b2d to 75360ed Compare November 12, 2024 09:31
@biggusdonzus biggusdonzus force-pushed the fdorlandi-add-literal-and-prefixed-matching branch from 75360ed to d2938ed Compare November 12, 2024 09:56
@tvainika tvainika merged commit 2b891fa into main Nov 12, 2024
4 checks passed
@tvainika tvainika deleted the fdorlandi-add-literal-and-prefixed-matching branch November 12, 2024 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants