Adding a new condition to identify when a everyone deny is found and …#677
Open
pcastelog wants to merge 1 commit intoNetcentric:developfrom
Open
Adding a new condition to identify when a everyone deny is found and …#677pcastelog wants to merge 1 commit intoNetcentric:developfrom
pcastelog wants to merge 1 commit intoNetcentric:developfrom
Conversation
…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; |
Member
There was a problem hiding this comment.
Can you add a test case for that condition?
| 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()) |
Member
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"))) { |
Member
There was a problem hiding this comment.
please act on actualAceBean instead of the string serialization for those checks
|
|
||
| } | ||
|
|
||
| // will override the logic to apply all ACLs on the path, and removing all elements present in config files |
Member
There was a problem hiding this comment.
wouldn't it make more sense that instead of reapplying everything just reorder the deny?
Member
|
Please also add a reference to #676 in the commit message to autolink it. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.