Adding a new condition to identify when a everyone deny is found and … #677
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…if that is the case, all configured acls on the node are removed and installed again
kwin
requested changes
Oct 26, 2023
| // usually happens when SP installs some rep:policy nodes | ||
| if ((acl.size() - countOutsideConfig <= configuredAceEntries.size()) | ||
| && (StringUtils.startsWith(actualAceBeanCompareStr, "everyone") && StringUtils.contains(actualAceBeanCompareStr, "deny"))) { | ||
| everyoneDenyFound = true; |
There was a problem hiding this comment.
Can you add a test case for that condition?
| @@ -97,6 +98,12 @@ protected void installAcl(Set<AceBean> aceBeanSetFromConfig, String path, Set<St | |||
| if (!principalsInConfiguration.contains(acePrincipalName)) { | |||
| countOutsideConfig++; | |||
| diffLog.append(" OUTSIDE (not in Config) " + actualAceBeanCompareStr + "\n"); | |||
| // Condition will check if everyone deny was added at the bottom, blocking all permissions created by actool | |||
| // usually happens when SP installs some rep:policy nodes | |||
| if ((acl.size() - countOutsideConfig <= configuredAceEntries.size()) | |||
There was a problem hiding this comment.
I don't understand how this condition checks the position of the everyone deny (what is stated in the comment), or is everyoneDenyFound always true irrespective of the position of the deny for everyone?
| // Condition will check if everyone deny was added at the bottom, blocking all permissions created by actool | ||
| // usually happens when SP installs some rep:policy nodes | ||
| if ((acl.size() - countOutsideConfig <= configuredAceEntries.size()) | ||
| && (StringUtils.startsWith(actualAceBeanCompareStr, "everyone") && StringUtils.contains(actualAceBeanCompareStr, "deny"))) { |
There was a problem hiding this comment.
please act on actualAceBean instead of the string serialization for those checks
| @@ -134,6 +141,18 @@ protected void installAcl(Set<AceBean> aceBeanSetFromConfig, String path, Set<St | |||
|
|
|||
| } | |||
|
|
|||
| // will override the logic to apply all ACLs on the path, and removing all elements present in config files | |||
There was a problem hiding this comment.
wouldn't it make more sense that instead of reapplying everything just reorder the deny?
|
Please also add a reference to #676 in the commit message to autolink it. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The change is going to find an everyone deny at the bottom of the list and if that is the case, loops again the entire permissions on the node removing the permission in the configuration files to apply them again on top.