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.

Sign up for GitHub

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

Adding a new condition to identify when a everyone deny is found and … #677

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Adding a new condition to identify when a everyone deny is found and … #677

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion ...rc/main/java/biz/netcentric/cq/tools/actool/aceinstaller/AceBeanInstallerIncremental.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,8 @@ protected void installAcl(Set<AceBean> aceBeanSetFromConfig, String path, Set<St
int currentPositionConfig = 0;

boolean changeHasBeenFound = false;

boolean everyoneDenyFound = false;

AccessControlManager acMgr = session.getAccessControlManager();

JackrabbitAccessControlList acl = getAccessControlList(acMgr, path);
Expand All @@ -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())
Copy link
Member

Choose a reason for hiding this comment

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

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?

&& (StringUtils.startsWith(actualAceBeanCompareStr, "everyone") && StringUtils.contains(actualAceBeanCompareStr, "deny"))) {
Copy link
Member

Choose a reason for hiding this comment

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

please act on actualAceBean instead of the string serialization for those checks

everyoneDenyFound = true;
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a test case for that condition?

}
continue;
}

Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

wouldn't it make more sense that instead of reapplying everything just reorder the deny?

if (everyoneDenyFound && !changeHasBeenFound) {
Iterator<AccessControlEntry> items = Arrays.asList(acl.getAccessControlEntries()).iterator();
while (items.hasNext()) {
AccessControlEntry currentEntry = items.next();
if (principalsInConfiguration.contains(currentEntry.getPrincipal().getName())) {
acl.removeAccessControlEntry(currentEntry);
}
}
currentPositionConfig = 0;
}

// install missing - this can be either because not all configured ACEs were found (append) or because a change was detected and old
// aces have been deleted

Expand Down
Loading