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

Make ACL filter configurable for method and path #4776

Merged
merged 2 commits into from
Jun 11, 2024

Conversation

fmarco76
Copy link
Member

If the servlet has different ACLs for different HTTP methods or paths the new implementation allows to define a map to identify the correct ACL.

The MAP will associate a key with the ACL and the key is made as <method>:<path>. The method is one of the HTTP defined method (e.g. GET, POST, ...) and the special value of "*" indicate all methods. The path is internal of the servlet path and in case of path parameter these can be identified with "{}".
An example of entry could be:

         key= PUT:/token/{} value= token.read

@fmarco76 fmarco76 requested a review from edewata June 10, 2024 19:07
Copy link
Contributor

@edewata edewata left a comment

Choose a reason for hiding this comment

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

I have some questions, but feel free to update/merge assuming it's sufficiently tested.

for (String a: aclKeys) {
String[] keyParts = a.split(":");
if (keyParts[0].equals("*")){
acl = aclMap.get(a);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need a break here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually it is not needed. In the list we could have an element with "*" and a matching path, followed by an entry with matching method and matching path. If we break here the more specific entry will not be selected.

Comment on lines 93 to 96
String[] aclKeys = aclMap.keySet().stream().filter( key -> {
String keyRegex = key.replaceFirst("\\*", ".*").replace("\\{\\}", "([A-Za-z0-9_\\-]*)");
return aclSearch.matches(keyRegex);
} ).toArray(String[]::new);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not an expert on streams, is it possible to get the map values directly? Something like:

String[] acls = aclMap.entrySet().stream()
      .filter(e -> { ... })
      .map(Map.Entry::getValue)
      .toArray(String[]::new);

if (acls.length > 0) {
    acl = acls[0];
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I have modified the stream sorting the keys in reverse order. In this case, the "*" will always be last and it is possible to collect the first element. In this way the loop among the element is not needed.

If the servlet has different ACLs for different HTTP methods or paths
the new implementation allows to define a map to identify the correct
ACL.

The MAP will associate a key with the ACL and the key is made as

     <method>:<path>

The method is one of the HTTP defined method (e.g. GET, POST, ...) and
the special value of "*" indicate all methods.

The path is internal of the servlet path and in case of path parameter
these can be identified with "{}". An example of entry could be:

     key= PUT:/token/{} value= token.read
Copy link

sonarcloud bot commented Jun 11, 2024

@fmarco76
Copy link
Member Author

@edewata Thanks!

I have done several tests of this code outside of pki. It is not used for the moment. I will do additional tests with the updated TokenService I am working on, which will use the ACL map.

@fmarco76 fmarco76 merged commit 90c4a48 into dogtagpki:master Jun 11, 2024
148 of 152 checks passed
@fmarco76 fmarco76 deleted the ACLMap branch June 11, 2024 13:07
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