SLING-11243 handle conflict between aggregate and leaf (#15)
Modifying an ACE should not include a allow/deny aggregate privilege
when there is a deny/allow child privilege with the same restrictions as
the parent
diff --git a/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/LocalRestriction.java b/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/LocalRestriction.java
index 58e5a61..645ee11 100644
--- a/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/LocalRestriction.java
+++ b/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/LocalRestriction.java
@@ -69,7 +69,7 @@
builder.append("LocalRestriction [name=");
builder.append(rd == null ? null : rd.getName());
builder.append(", value=");
- builder.append(getValues());
+ builder.append(Arrays.toString(getValues()));
builder.append("]");
return builder.toString();
}
diff --git a/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/impl/PrivilegesHelper.java b/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/impl/PrivilegesHelper.java
index 6404271..7fd2459 100644
--- a/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/impl/PrivilegesHelper.java
+++ b/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/impl/PrivilegesHelper.java
@@ -80,6 +80,46 @@
}
/**
+ * Populates a local allow and/or deny privilege in the privilegeToLocalPrivilegesMap
+ *
+ * @param privilegeToLocalPrivilegesMap the map containing the declared LocalPrivilege items
+ * @param privilege the privilege to allow
+ * @param allow true or false to set the allow value of the LocalPrivilege
+ * @param allowRestrictions if allow is true, the set of restrictions
+ * @param deny true or false to set the allow value of the LocalPrivilege
+ * @param denyRestrictions if deny is true, the set of restrictions
+ * @return the LocalPrivileges that was populated
+ */
+ public static LocalPrivilege localAllowAndDenyPriv(Map<Privilege, LocalPrivilege> privilegeToLocalPrivilegesMap,
+ Privilege privilege,
+ boolean allow, @NotNull Set<LocalRestriction> allowRestrictions,
+ boolean deny, @NotNull Set<LocalRestriction> denyRestrictions
+ ) {
+ LocalPrivilege localPrivilege = privilegeToLocalPrivilegesMap.computeIfAbsent(privilege, LocalPrivilege::new);
+ if (allow) {
+ localPrivilege.setAllow(true);
+ localPrivilege.setAllowRestrictions(allowRestrictions);
+ } else {
+ localPrivilege.setAllow(false);
+ localPrivilege.setAllowRestrictions(Collections.emptySet());
+ }
+ if (deny) {
+ localPrivilege.setDeny(true);
+ localPrivilege.setDenyRestrictions(denyRestrictions);
+ } else {
+ localPrivilege.setDeny(false);
+ localPrivilege.setDenyRestrictions(Collections.emptySet());
+ }
+ if (localPrivilege.isAllow() && localPrivilege.isDeny() && localPrivilege.sameAllowAndDenyRestrictions()) {
+ // same restrictions to we prefer the allow
+ localPrivilege.setDeny(false);
+ localPrivilege.setDenyRestrictions(Collections.emptySet());
+ }
+
+ return localPrivilege;
+ }
+
+ /**
* Populates a local allow privilege in the privilegeToLocalPrivilegesMap
*
* @param privilegeToLocalPrivilegesMap the map containing the declared LocalPrivilege items
@@ -132,6 +172,43 @@
}
/**
+ * Populates each of the local allow and/or deny privilege in the privilegeToLocalPrivilegesMap. If the supplied
+ * privilege is an aggregate then the data is populated for each of non-aggregate privileges contained in
+ * the aggregate privilege. Otherwise, the data is populated for the privilege itself.
+ *
+ * @param privilegeToLocalPrivilegesMap the map containing the declared LocalPrivilege items
+ * @param p the privilege to update
+ * @param allow true or false to set the allow value of the LocalPrivilege
+ * @param allowRestrictions if allow is true, the set of restrictions
+ * @param deny true or false to set the allow value of the LocalPrivilege
+ * @param denyRestrictions if deny is true, the set of restrictions
+ */
+ private static void expandAllowAndDenyPrivWithoutAggregates(Map<Privilege, LocalPrivilege> privilegeToLocalPrivilegesMap,
+ Privilege p,
+ boolean allow, @NotNull Set<LocalRestriction> allowRestrictions,
+ boolean deny, @NotNull Set<LocalRestriction> denyRestrictions
+ ) throws RepositoryException {
+ if (p.isAggregate()) {
+ Privilege[] aggregatePrivileges = p.getDeclaredAggregatePrivileges();
+ for (Privilege aggregatePrivilege : aggregatePrivileges) {
+ if (aggregatePrivilege.isAggregate()) {
+ expandAllowAndDenyPrivWithoutAggregates(privilegeToLocalPrivilegesMap, aggregatePrivilege,
+ allow, allowRestrictions,
+ deny, denyRestrictions);
+ } else {
+ localAllowAndDenyPriv(privilegeToLocalPrivilegesMap, aggregatePrivilege,
+ allow, allowRestrictions,
+ deny, denyRestrictions);
+ }
+ }
+ } else {
+ localAllowAndDenyPriv(privilegeToLocalPrivilegesMap, p,
+ allow, allowRestrictions,
+ deny, denyRestrictions);
+ }
+ }
+
+ /**
* Populates each of the local allow privilege in the privilegeToLocalPrivilegesMap. If the supplied
* privilege is an aggregate then the data is populated for each of non-aggregate privileges contained in
* the aggregate privilege. Otherwise, the data is populated for the privilege itself.
@@ -184,6 +261,28 @@
}
/**
+ * Populates each of the local allow and/or deny privilege in the privilegeToLocalPrivilegesMap. If the supplied
+ * privilege is an aggregate then the data is populated for each of non-aggregate privileges contained in
+ * the aggregate privilege. Otherwise, the data is populated for the privilege itself.
+ *
+ * @param privilegeToLocalPrivilegesMap the map containing the declared LocalPrivilege items
+ * @param allow true or false to set the allow value of the LocalPrivilege
+ * @param allowRestrictions if allow is true, the set of restrictions
+ * @param deny true or false to set the allow value of the LocalPrivilege
+ * @param denyRestrictions if deny is true, the set of restrictions
+ * @param privileges the privilege to update
+ */
+ public static void allowAndDeny(Map<Privilege, LocalPrivilege> privilegeToLocalPrivilegesMap,
+ boolean allow, @NotNull Set<LocalRestriction> allowRestrictions,
+ boolean deny, @NotNull Set<LocalRestriction> denyRestrictions,
+ @NotNull Collection<Privilege> privileges) throws RepositoryException {
+ for (Privilege privilege : privileges) {
+ expandAllowAndDenyPrivWithoutAggregates(privilegeToLocalPrivilegesMap, privilege,
+ allow, allowRestrictions, deny, denyRestrictions);
+ }
+ }
+
+ /**
* Populates each of the allow privilege in the privilegeToLocalPrivilegesMap.
*
* @param privilegeToLocalPrivilegesMap the map containing the declared LocalPrivilege items
@@ -577,43 +676,63 @@
if (childPrivileges.length == childLocalPrivileges.size()) {
boolean allAllow = childLocalPrivileges.stream().allMatch(LocalPrivilege::isAllow);
if (allAllow) {
- // all the child privileges are allow so we can mark the parent as allow
- LocalPrivilege alp = privilegeToLocalPrivilegesMap.computeIfAbsent(aggregatePrivilege, LocalPrivilege::new);
- alp.setAllow(true);
-
// if the restrictions of all the items is the same then we should copy it up
// and unset the data from each child
Set<LocalRestriction> firstAllowRestrictions = childLocalPrivileges.get(0).getAllowRestrictions();
boolean allRestrictionsSame = childLocalPrivileges.stream().allMatch(lp -> firstAllowRestrictions.equals(lp.getAllowRestrictions()));
+ Set<LocalRestriction> restrictions;
if (allRestrictionsSame) {
- alp.setAllowRestrictions(firstAllowRestrictions);
+ restrictions = firstAllowRestrictions;
+ } else {
+ restrictions = Collections.emptySet();
}
- // each child with the same restrictions can be unset
- for (LocalPrivilege lp : childLocalPrivileges) {
- if (lp.sameAllowRestrictions(alp.getAllowRestrictions())) {
- lp.setAllow(false);
- lp.setAllowRestrictions(Collections.emptySet());
+
+ // if any deny child has the same restrictions, then the parent should not be set
+ // as that would cause the deny privilege to get excluded when persisted
+ boolean anyDenyWithSameRestrictions = childLocalPrivileges.stream().anyMatch(p -> p.isDeny() && p.sameDenyRestrictions(restrictions));
+ if (!anyDenyWithSameRestrictions) {
+ // all the child privileges are allow so we can mark the parent as allow
+ LocalPrivilege alp = privilegeToLocalPrivilegesMap.computeIfAbsent(aggregatePrivilege, LocalPrivilege::new);
+ alp.setAllow(true);
+ alp.setAllowRestrictions(restrictions);
+
+ // each child with the same restrictions can be unset
+ for (LocalPrivilege lp : childLocalPrivileges) {
+ if (lp.sameAllowRestrictions(alp.getAllowRestrictions())) {
+ lp.setAllow(false);
+ lp.setAllowRestrictions(Collections.emptySet());
+ }
}
}
}
boolean allDeny = childLocalPrivileges.stream().allMatch(LocalPrivilege::isDeny);
if (allDeny) {
- // all the child privileges are deny so we can mark the parent as deny
- LocalPrivilege alp = privilegeToLocalPrivilegesMap.computeIfAbsent(aggregatePrivilege, LocalPrivilege::new);
- alp.setDeny(true);
-
// if the restrictions of all the items is the same then we should copy it up
// and unset the data from each child
Set<LocalRestriction> firstDenyRestrictions = childLocalPrivileges.get(0).getDenyRestrictions();
boolean allRestrictionsSame = childLocalPrivileges.stream().allMatch(lp -> firstDenyRestrictions.equals(lp.getDenyRestrictions()));
+ Set<LocalRestriction> restrictions;
if (allRestrictionsSame) {
- alp.setDenyRestrictions(firstDenyRestrictions);
+ restrictions = firstDenyRestrictions;
+ } else {
+ restrictions = Collections.emptySet();
}
- // each child with the same restrictions can be unset
- for (LocalPrivilege lp : childLocalPrivileges) {
- if (lp.sameDenyRestrictions(alp.getDenyRestrictions())) {
- lp.setDeny(false);
- lp.setDenyRestrictions(Collections.emptySet());
+
+ // if any allow child has the same restrictions, then the parent should not be set
+ // as that would cause the allow privilege to get excluded when persisted
+ boolean anyAllowWithSameRestrictions = childLocalPrivileges.stream().anyMatch(p -> p.isAllow() && p.sameAllowRestrictions(restrictions));
+ if (!anyAllowWithSameRestrictions) {
+ // all the child privileges are deny so we can mark the parent as deny
+ LocalPrivilege alp = privilegeToLocalPrivilegesMap.computeIfAbsent(aggregatePrivilege, LocalPrivilege::new);
+ alp.setDeny(true);
+ alp.setDenyRestrictions(restrictions);
+
+ // each child with the same restrictions can be unset
+ for (LocalPrivilege lp : childLocalPrivileges) {
+ if (lp.sameDenyRestrictions(alp.getDenyRestrictions())) {
+ lp.setDeny(false);
+ lp.setDenyRestrictions(Collections.emptySet());
+ }
}
}
}
diff --git a/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/ModifyAceServlet.java b/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/ModifyAceServlet.java
index 9669aa4..d46ccee 100644
--- a/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/ModifyAceServlet.java
+++ b/src/main/java/org/apache/sling/jcr/jackrabbit/accessmanager/post/ModifyAceServlet.java
@@ -21,7 +21,6 @@
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
-import java.util.Comparator;
import java.util.EnumMap;
import java.util.Enumeration;
import java.util.HashMap;
@@ -169,6 +168,10 @@
this.paramValue = paramValue;
}
+ public String getParamValue() {
+ return paramValue;
+ }
+
public static PrivilegeValues valueOfParam(String value) {
return Stream.of(PrivilegeValues.values())
.filter(item -> item.paramValue.equalsIgnoreCase(value))
@@ -234,8 +237,7 @@
// and now merge the changes from the request parameters
processPostedPrivilegeDeleteParams(acm, request, privilegeToLocalPrivilegesMap);
processPostedRestrictionDeleteParams(acm, request, srMap, privilegeToLocalPrivilegesMap);
- processPostedPrivilegeParams(acm, request, privilegeToLocalPrivilegesMap, privilegeLongestDepthMap);
- processPostedRestrictionParams(acm, request, srMap, privilegeToLocalPrivilegesMap, privilegeLongestDepthMap);
+ processPostedPrivilegeAndRestrictionParams(acm, request, srMap, privilegeToLocalPrivilegesMap, privilegeLongestDepthMap);
// consolidate any aggregates that are still valid
PrivilegesHelper.consolidateAggregates(session, resourcePath, privilegeToLocalPrivilegesMap, privilegeLongestDepthMap);
@@ -480,25 +482,23 @@
}
/**
- * Merge into the privilegeToLocalPrivilegesMap the changes requested in restriction
- * request parameters.
+ * Populate the restrictions that that were posted and applicable
+ * to the requested privilege
*
- * @param acm the access control manager
* @param request the current request
* @param srMap map of restriction names to the restriction definition
- * @param privilegeToLocalPrivilegesMap the map containing the declared LocalPrivilege items
+ * @param forPrivilege the privilege to load the restrictions for
+ * @param forAllowOrDeny either {@link PrivilegeValues#ALLOW} or {@link PrivilegeValues#DENY}
+ * @param generalRestrictions the general restrictions that are not for a specific privilege
*/
- protected void processPostedRestrictionParams(@NotNull AccessControlManager acm,
+ protected Set<LocalRestriction> postedRestrictionsForPrivilege(
@NotNull SlingHttpServletRequest request,
@NotNull Map<String, RestrictionDefinition> srMap,
- @NotNull Map<Privilege, LocalPrivilege> privilegeToLocalPrivilegesMap,
- @NotNull Map<Privilege, Integer> privilegeLongestDepthMap) throws RepositoryException {
- Session session = request.getResourceResolver().adaptTo(Session.class);
- ValueFactory vf = session.getValueFactory();
+ @NotNull Privilege forPrivilege,
+ @NotNull PrivilegeValues forAllowOrDeny,
+ @NotNull Set<LocalRestriction> generalRestrictions) throws RepositoryException {
+ Set<LocalRestriction> restrictions = new HashSet<>(generalRestrictions);
- // first pass to collect all the restrictions so we can
- // process them in the right order
- Map<Privilege, Map<LocalRestriction, Set<String>>> privilegeToLocalRestrictionMap = new HashMap<>();
@NotNull
Map<String, Matcher> postedRestrictionParams = getMatchedRequestParameterNames(request, RESTRICTION_PATTERN);
for (Entry<String, Matcher> entry : postedRestrictionParams.entrySet()) {
@@ -506,146 +506,214 @@
Matcher matcher = entry.getValue();
String privilegeName;
String restrictionName;
- String allowOrDeny;
+ PrivilegeValues allowOrDeny;
if (matcher.group(2) != null) {
privilegeName = matcher.group(1);
restrictionName = matcher.group(3);
- allowOrDeny = matcher.group(4);
+ allowOrDeny = PrivilegeValues.valueOfParam(matcher.group(4));
} else {
privilegeName = null;
restrictionName = matcher.group(1);
allowOrDeny = null;
}
- Privilege privilege = privilegeName == null ? null : acm.privilegeFromName(privilegeName);
-
- RestrictionDefinition rd = srMap.get(restrictionName);
- if (rd == null) {
- //illegal restriction name?
- throw new AccessControlException(INVALID_OR_NOT_SUPPORTED_RESTRICTION_NAME_WAS_SUPPLIED);
- }
- LocalRestriction localRestriction;
- int restrictionType = rd.getRequiredType().tag();
- if (rd.getRequiredType().isArray()) {
- // multi-value
- String[] parameterValues = request.getParameterValues(paramName);
- Value[] restrictionValue = new Value[parameterValues.length];
- for (int i = 0; i < parameterValues.length; i++) {
- restrictionValue[i] = vf.createValue(parameterValues[i], restrictionType);
- }
- localRestriction = new LocalRestriction(rd, restrictionValue);
- } else {
- // single value
- Value restrictionValue = vf.createValue(request.getParameter(paramName), restrictionType);
- localRestriction = new LocalRestriction(rd, restrictionValue);
- }
-
- Map<LocalRestriction, Set<String>> lrMap = privilegeToLocalRestrictionMap.computeIfAbsent(privilege, k -> new HashMap<>());
- Set<String> valuesSet = lrMap.computeIfAbsent(localRestriction, k -> new HashSet<>());
- valuesSet.add(allowOrDeny);
- }
-
- List<Entry<Privilege, Map<LocalRestriction, Set<String>>>> sortedEntries = new ArrayList<>(privilegeToLocalRestrictionMap.entrySet());
- // sort the entries to process the most shallow last
- Collections.sort(sortedEntries, Comparator.nullsFirst(Comparator.comparing(entry -> privilegeLongestDepthMap.get(entry.getKey()))));
- for (Entry<Privilege, Map<LocalRestriction, Set<String>>> entry : sortedEntries) {
- Privilege privilege = entry.getKey();
-
- Collection<Privilege> privileges;
- if (privilege == null) {
- // process for every privilege
- privileges = privilegeToLocalPrivilegesMap.keySet();
- } else {
- // process for the specific privilege only
- privileges = Collections.singletonList(privilege);
- }
-
- Map<LocalRestriction, Set<String>> lrMap = entry.getValue();
- for (Entry<LocalRestriction, Set<String>> lrEntry : lrMap.entrySet()) {
- LocalRestriction localRestriction = lrEntry.getKey();
- // sort the values to ensure it processes the Allow entry last when
- // there is a conflict
- List<PrivilegeValues> privilegeValues = lrEntry.getValue().stream()
- .map(item -> item == null ? null : PrivilegeValues.valueOfParam(item))
- .sorted(Comparator.nullsLast(Comparator.comparing(PrivilegeValues::ordinal).reversed()))
- .collect(Collectors.toList());
- for (PrivilegeValues allowOrDeny : privilegeValues) {
- if (allowOrDeny == null) {
- // not specified try both the deny and allow sets
- PrivilegesHelper.allowOrDenyRestriction(privilegeToLocalPrivilegesMap, localRestriction, privileges);
- } else {
- switch (allowOrDeny) {
- case DENY:
- PrivilegesHelper.denyRestriction(privilegeToLocalPrivilegesMap, localRestriction, privileges);
- break;
- case ALLOW:
- PrivilegesHelper.allowRestriction(privilegeToLocalPrivilegesMap, localRestriction, privileges);
- break;
- default:
- break;
- }
- }
- }
+ if ((privilegeName == null || forPrivilege.getName().equals(privilegeName)) &&
+ (allowOrDeny == null || forAllowOrDeny.equals(allowOrDeny))) {
+ LocalRestriction localRestriction = toLocalRestriction(request, srMap, restrictionName, paramName);
+ restrictions.removeIf(r -> r.getName().equals(localRestriction.getName()));
+ restrictions.add(localRestriction);
}
}
+
+ return restrictions;
}
/**
- * Merge into the privilegeToLocalPrivilegesMap the changes requested in privilege
- * request parameters.
+ * Construct a LocalRestriction using data a request parameter
+ *
+ * @param request the current request
+ * @param srMap map of restriction names to the restriction definition
+ * @param restrictionName the name of the restriction
+ * @param paramName the request parameter name that contains the restriction values
+ */
+ protected LocalRestriction toLocalRestriction(
+ @NotNull SlingHttpServletRequest request,
+ @NotNull Map<String, RestrictionDefinition> srMap,
+ @NotNull String restrictionName,
+ @NotNull String paramName) throws RepositoryException {
+ RestrictionDefinition rd = srMap.get(restrictionName);
+ if (rd == null) {
+ //illegal restriction name?
+ throw new AccessControlException(INVALID_OR_NOT_SUPPORTED_RESTRICTION_NAME_WAS_SUPPLIED);
+ }
+ Session session = request.getResourceResolver().adaptTo(Session.class);
+ ValueFactory vf = session.getValueFactory();
+ LocalRestriction localRestriction;
+ int restrictionType = rd.getRequiredType().tag();
+ if (rd.getRequiredType().isArray()) {
+ // multi-value
+ String[] parameterValues = request.getParameterValues(paramName);
+ Value[] restrictionValue = new Value[parameterValues.length];
+ for (int i = 0; i < parameterValues.length; i++) {
+ restrictionValue[i] = vf.createValue(parameterValues[i], restrictionType);
+ }
+ localRestriction = new LocalRestriction(rd, restrictionValue);
+ } else {
+ // single value
+ Value restrictionValue = vf.createValue(request.getParameter(paramName), restrictionType);
+ localRestriction = new LocalRestriction(rd, restrictionValue);
+ }
+ return localRestriction;
+ }
+
+ /**
+ * Merge into the privilegeToLocalPrivilegesMap the changes requested in privilege and
+ * restriction request parameters.
*
* @param acm the access control manager
* @param request the current request
+ * @param srMap map of restriction names to the restriction definition
* @param privilegeToLocalPrivilegesMap the map containing the declared LocalPrivilege items
+ * @param privilegeLongestDepthMap the map of privileges to their longest depth
*/
- protected void processPostedPrivilegeParams(@NotNull AccessControlManager acm,
+ protected void processPostedPrivilegeAndRestrictionParams(@NotNull AccessControlManager acm,
@NotNull SlingHttpServletRequest request,
+ @NotNull Map<String, RestrictionDefinition> srMap,
@NotNull Map<Privilege, LocalPrivilege> privilegeToLocalPrivilegesMap,
@NotNull Map<Privilege, Integer> privilegeLongestDepthMap) throws RepositoryException {
@NotNull
Map<String, Matcher> postedPrivilegeNameKeys = getMatchedRequestParameterNames(request, PRIVILEGE_PATTERN);
- // first pass to collect all the privileges so we can
- // process them in the right order
- Map<Privilege, String> privilegeToParamNameMap = new HashMap<>();
+ // collect all the privileges so we can process them in the right order
+ Map<Privilege, Set<String>> privilegeToParamValuesMap = new HashMap<>();
for (Entry<String, Matcher> entry : postedPrivilegeNameKeys.entrySet()) {
String paramName = entry.getKey();
Matcher matcher = entry.getValue();
String privilegeName = matcher.group(1);
Privilege privilege = acm.privilegeFromName(privilegeName);
- privilegeToParamNameMap.put(privilege, paramName);
+ Set<String> paramValues = privilegeToParamValuesMap.computeIfAbsent(privilege, p -> new HashSet<>());
+ paramValues.addAll(Arrays.asList(request.getParameterValues(paramName)));
}
- List<Entry<Privilege, String>> sortedEntries = new ArrayList<>(privilegeToParamNameMap.entrySet());
+
+ //also check for any restriction params
+ Set<LocalRestriction> generalRestrictions = new HashSet<>();
+ Map<String, Matcher> postedRestrictionParams = getMatchedRequestParameterNames(request, RESTRICTION_PATTERN);
+ for (Entry<String, Matcher> entry : postedRestrictionParams.entrySet()) {
+ Matcher matcher = entry.getValue();
+ if (matcher.group(2) != null) {
+ PrivilegeValues allowOrDeny = PrivilegeValues.valueOfParam(matcher.group(4));
+ if (PrivilegeValues.ALLOW.equals(allowOrDeny) ||
+ PrivilegeValues.DENY.equals(allowOrDeny)) {
+ String privilegeName = matcher.group(1);
+ Privilege privilege = acm.privilegeFromName(privilegeName);
+ Set<String> paramValues = privilegeToParamValuesMap.computeIfAbsent(privilege, p -> new HashSet<>());
+ paramValues.add(allowOrDeny.getParamValue());
+ }
+ } else {
+ // restriction but not for a specific privilege
+ String restrictionName = matcher.group(1);
+ String paramName = entry.getKey();
+ LocalRestriction localRestriction = toLocalRestriction(request, srMap, restrictionName, paramName);
+ generalRestrictions.removeIf(r -> r.getName().equals(localRestriction.getName()));
+ generalRestrictions.add(localRestriction);
+ }
+ }
+
+ // apply the general restrictions to any already existing privilege that was not posted
+ // with new state
+ if (!generalRestrictions.isEmpty()) {
+ for (Entry<Privilege, LocalPrivilege> entry : privilegeToLocalPrivilegesMap.entrySet()) {
+ Privilege p = entry.getKey();
+ if (!privilegeToParamValuesMap.containsKey(p)) {
+ LocalPrivilege lp = entry.getValue();
+ applyPrivilegeAndRestrictions(privilegeToLocalPrivilegesMap, p,
+ lp.isAllow(), generalRestrictions,
+ lp.isDeny(), generalRestrictions);
+ }
+ }
+ }
+
+ List<Entry<Privilege, Set<String>>> sortedEntries = new ArrayList<>(privilegeToParamValuesMap.entrySet());
// sort the entries to process the most shallow last
Collections.sort(sortedEntries, (e1, e2) -> privilegeLongestDepthMap.get(e1.getKey()).compareTo(privilegeLongestDepthMap.get(e2.getKey())));
- for (Entry<Privilege, String> entry : sortedEntries) {
- String paramName = entry.getValue();
+ for (Entry<Privilege, Set<String>> entry : sortedEntries) {
+ Set<String> paramValues = entry.getValue();
Privilege privilege = entry.getKey();
- String [] paramValues = request.getParameterValues(paramName);
// convert and sort the values to ensure that allow goes after
// deny or none when there is a conflict
- List<PrivilegeValues> privilegeValues = Stream.of(paramValues)
+ List<PrivilegeValues> privilegeValues = paramValues.stream()
.map(PrivilegeValues::valueOfParam)
.sorted((v1, v2) -> Integer.compare(v2.ordinal(), v1.ordinal()))
.collect(Collectors.toList());
+ boolean none = false;
+ boolean allow = false;
+ Set<LocalRestriction> allowRestrictions = Collections.emptySet();
+ boolean deny = false;
+ Set<LocalRestriction> denyRestrictions = Collections.emptySet();
for (PrivilegeValues value : privilegeValues) {
switch (value) {
case DENY:
case DENIED:
- PrivilegesHelper.deny(privilegeToLocalPrivilegesMap, Collections.emptySet(), Collections.singleton(privilege));
+ deny = true;
+ denyRestrictions = postedRestrictionsForPrivilege(request, srMap, privilege, value, generalRestrictions);
break;
case ALLOW:
case GRANTED:
- PrivilegesHelper.allow(privilegeToLocalPrivilegesMap, Collections.emptySet(), Collections.singleton(privilege));
+ allow = true;
+ allowRestrictions = postedRestrictionsForPrivilege(request, srMap, privilege, value, generalRestrictions);
break;
case NONE:
- PrivilegesHelper.none(privilegeToLocalPrivilegesMap, Collections.singleton(privilege));
+ none = true;
break;
default:
break;
}
}
+ if (none) {
+ PrivilegesHelper.none(privilegeToLocalPrivilegesMap, Collections.singleton(privilege));
+ }
+ applyPrivilegeAndRestrictions(privilegeToLocalPrivilegesMap, privilege,
+ allow, allowRestrictions,
+ deny, denyRestrictions);
+ }
+ }
+
+ /**
+ * Apply the privilege and restrictions to the local privileges
+ *
+ * @param privilegeToLocalPrivilegesMap the map containing the declared LocalPrivilege items
+ * @param p the privilege
+ * @param allow true if the privilege is to be allowed
+ * @param allowRestrictions restrictions (if any) for the allow
+ * @param deny true if the privilege is to be denied
+ * @param denyRestrictions restrictions (if any) for the deny
+ */
+ protected void applyPrivilegeAndRestrictions(
+ @NotNull Map<Privilege, LocalPrivilege> privilegeToLocalPrivilegesMap,
+ @NotNull Privilege p,
+ boolean allow, @NotNull Set<LocalRestriction> allowRestrictions,
+ boolean deny, @NotNull Set<LocalRestriction> denyRestrictions) throws RepositoryException {
+ if (allow) {
+ // clear out the state that we are replacing
+ PrivilegesHelper.unallowRestrictions(privilegeToLocalPrivilegesMap,
+ allowRestrictions.stream().map(LocalRestriction::getName).collect(Collectors.toSet()),
+ Collections.singleton(p));
+ }
+ if (deny) {
+ // clear out the state that we are replacing
+ PrivilegesHelper.undenyRestrictions(privilegeToLocalPrivilegesMap,
+ denyRestrictions.stream().map(LocalRestriction::getName).collect(Collectors.toSet()),
+ Collections.singleton(p));
+ }
+ if (allow && deny) {
+ PrivilegesHelper.allowAndDeny(privilegeToLocalPrivilegesMap,
+ allow, allowRestrictions, deny, denyRestrictions,
+ Collections.singleton(p));
+ } else if (allow) {
+ PrivilegesHelper.allow(privilegeToLocalPrivilegesMap, allowRestrictions, Collections.singleton(p));
+ } else if (deny) {
+ PrivilegesHelper.deny(privilegeToLocalPrivilegesMap, denyRestrictions, Collections.singleton(p));
}
}
diff --git a/src/test/java/org/apache/sling/jcr/jackrabbit/accessmanager/impl/PrivilegesHelperTest.java b/src/test/java/org/apache/sling/jcr/jackrabbit/accessmanager/impl/PrivilegesHelperTest.java
index 666082a..e5635ea 100644
--- a/src/test/java/org/apache/sling/jcr/jackrabbit/accessmanager/impl/PrivilegesHelperTest.java
+++ b/src/test/java/org/apache/sling/jcr/jackrabbit/accessmanager/impl/PrivilegesHelperTest.java
@@ -262,6 +262,117 @@
}
@Test
+ public void testAllowAndDenyBoth() throws RepositoryException {
+ Map<Privilege, LocalPrivilege> merged = new HashMap<>();
+
+ // allow jcr:modifyProperties
+ PrivilegesHelper.allowAndDeny(merged,
+ true, Collections.emptySet(),
+ true, Collections.emptySet(),
+ Collections.singleton(priv(PrivilegeConstants.JCR_MODIFY_PROPERTIES)));
+
+ assertAllowSetHasModifyPropertiesLeafs(merged);
+ assertDenySetIsEmpty(merged);
+ }
+
+ @Test
+ public void testAllowAndDenyBothWithSameRestrictions() throws RepositoryException {
+ Map<Privilege, LocalPrivilege> merged = new HashMap<>();
+
+ // allow jcr:modifyProperties
+ PrivilegesHelper.allowAndDeny(merged,
+ true, Collections.singleton(new LocalRestriction(rd(AccessControlConstants.REP_GLOB), val("/hello"))),
+ true, Collections.singleton(new LocalRestriction(rd(AccessControlConstants.REP_GLOB), val("/hello"))),
+ Collections.singleton(priv(PrivilegeConstants.JCR_MODIFY_PROPERTIES)));
+
+ assertAllowSetHasModifyPropertiesLeafs(merged);
+ assertDenySetIsEmpty(merged);
+ }
+
+ protected void assertDenySetIsEmpty(Map<Privilege, LocalPrivilege> merged) {
+ Set<Privilege> denySet = merged.values().stream()
+ .filter(lp -> lp.isDeny())
+ .map(lp -> lp.getPrivilege())
+ .collect(Collectors.toSet());
+ assertTrue(denySet.isEmpty());
+ }
+
+ protected void assertAllowSetHasModifyPropertiesLeafs(Map<Privilege, LocalPrivilege> merged)
+ throws RepositoryException {
+ Set<Privilege> allowSet = merged.values().stream()
+ .filter(lp -> lp.isAllow())
+ .map(lp -> lp.getPrivilege())
+ .collect(Collectors.toSet());
+ assertThat(allowSet.size(), equalTo(3));
+ assertThat(allowSet, not(hasItems(priv(PrivilegeConstants.JCR_MODIFY_PROPERTIES))));
+ assertThat(allowSet, hasItems(
+ priv(PrivilegeConstants.REP_ADD_PROPERTIES),
+ priv(PrivilegeConstants.REP_ALTER_PROPERTIES),
+ priv(PrivilegeConstants.REP_REMOVE_PROPERTIES)));
+ }
+
+ protected void assertDenySetHasModifyPropertiesLeafs(Map<Privilege, LocalPrivilege> merged)
+ throws RepositoryException {
+ Set<Privilege> denySet = merged.values().stream()
+ .filter(lp -> lp.isDeny())
+ .map(lp -> lp.getPrivilege())
+ .collect(Collectors.toSet());
+ assertThat(denySet.size(), equalTo(3));
+ assertThat(denySet, not(hasItems(priv(PrivilegeConstants.JCR_MODIFY_PROPERTIES))));
+ assertThat(denySet, hasItems(
+ priv(PrivilegeConstants.REP_ADD_PROPERTIES),
+ priv(PrivilegeConstants.REP_ALTER_PROPERTIES),
+ priv(PrivilegeConstants.REP_REMOVE_PROPERTIES)));
+ }
+
+
+ @Test
+ public void testAllowAndDenyBothWithDifferentRestrictions() throws RepositoryException {
+ Map<Privilege, LocalPrivilege> merged = new HashMap<>();
+
+ // allow jcr:modifyProperties
+ PrivilegesHelper.allowAndDeny(merged,
+ true, Collections.singleton(new LocalRestriction(rd(AccessControlConstants.REP_GLOB), val("/hello"))),
+ true, Collections.singleton(new LocalRestriction(rd(AccessControlConstants.REP_GLOB), val("/hello2"))),
+ Collections.singleton(priv(PrivilegeConstants.JCR_MODIFY_PROPERTIES)));
+
+ assertAllowSetHasModifyPropertiesLeafs(merged);
+ assertDenySetHasModifyPropertiesLeafs(merged);
+ }
+
+ @Test
+ public void testAllowAndDenyOnlyAllow() throws RepositoryException {
+ Map<Privilege, LocalPrivilege> merged = new HashMap<>();
+
+ // allow jcr:modifyProperties
+ PrivilegesHelper.allowAndDeny(merged,
+ true, Collections.emptySet(),
+ false, Collections.emptySet(),
+ Collections.singleton(priv(PrivilegeConstants.JCR_MODIFY_PROPERTIES)));
+
+ assertAllowSetHasModifyPropertiesLeafs(merged);
+ assertDenySetIsEmpty(merged);
+ }
+
+ @Test
+ public void testAllowAndDenyOnlyDeny() throws RepositoryException {
+ Map<Privilege, LocalPrivilege> merged = new HashMap<>();
+
+ // allow jcr:modifyProperties
+ PrivilegesHelper.allowAndDeny(merged,
+ false, Collections.emptySet(),
+ true, Collections.emptySet(),
+ Collections.singleton(priv(PrivilegeConstants.JCR_MODIFY_PROPERTIES)));
+
+ Set<Privilege> allowSet = merged.values().stream()
+ .filter(lp -> lp.isAllow())
+ .map(lp -> lp.getPrivilege())
+ .collect(Collectors.toSet());
+ assertTrue(allowSet.isEmpty());
+ assertDenySetHasModifyPropertiesLeafs(merged);
+ }
+
+ @Test
public void testAllow() throws RepositoryException {
Map<Privilege, LocalPrivilege> merged = new HashMap<>();
@@ -269,17 +380,7 @@
PrivilegesHelper.allow(merged, Collections.emptySet(),
Collections.singleton(priv(PrivilegeConstants.JCR_MODIFY_PROPERTIES)));
- Set<Privilege> allowSet = merged.values().stream()
- .filter(lp -> lp.isAllow())
- .map(lp -> lp.getPrivilege())
- .collect(Collectors.toSet());
-
- assertThat(allowSet.size(), equalTo(3));
- assertThat(allowSet, not(hasItems(priv(PrivilegeConstants.JCR_MODIFY_PROPERTIES))));
- assertThat(allowSet, hasItems(
- priv(PrivilegeConstants.REP_ADD_PROPERTIES),
- priv(PrivilegeConstants.REP_ALTER_PROPERTIES),
- priv(PrivilegeConstants.REP_REMOVE_PROPERTIES)));
+ assertAllowSetHasModifyPropertiesLeafs(merged);
}
@Test
@@ -290,17 +391,7 @@
PrivilegesHelper.allow(merged, Collections.emptySet(),
Collections.singleton(priv(PrivilegeConstants.JCR_MODIFY_PROPERTIES)));
- Set<Privilege> allowSet = merged.values().stream()
- .filter(lp -> lp.isAllow())
- .map(lp -> lp.getPrivilege())
- .collect(Collectors.toSet());
-
- assertThat(allowSet.size(), equalTo(3));
- assertThat(allowSet, not(hasItems(priv(PrivilegeConstants.JCR_MODIFY_PROPERTIES))));
- assertThat(allowSet, hasItems(
- priv(PrivilegeConstants.REP_ADD_PROPERTIES),
- priv(PrivilegeConstants.REP_ALTER_PROPERTIES),
- priv(PrivilegeConstants.REP_REMOVE_PROPERTIES)));
+ assertAllowSetHasModifyPropertiesLeafs(merged);
// unallow jcr:modifyProperties
PrivilegesHelper.unallow(merged,
@@ -322,17 +413,7 @@
PrivilegesHelper.deny(merged, Collections.emptySet(),
Collections.singleton(priv(PrivilegeConstants.JCR_MODIFY_PROPERTIES)));
- Set<Privilege> denySet = merged.values().stream()
- .filter(lp -> lp.isDeny())
- .map(lp -> lp.getPrivilege())
- .collect(Collectors.toSet());
-
- assertThat(denySet.size(), equalTo(3));
- assertThat(denySet, not(hasItems(priv(PrivilegeConstants.JCR_MODIFY_PROPERTIES))));
- assertThat(denySet, hasItems(
- priv(PrivilegeConstants.REP_ADD_PROPERTIES),
- priv(PrivilegeConstants.REP_ALTER_PROPERTIES),
- priv(PrivilegeConstants.REP_REMOVE_PROPERTIES)));
+ assertDenySetHasModifyPropertiesLeafs(merged);
}
@Test
@@ -343,17 +424,7 @@
PrivilegesHelper.deny(merged, Collections.emptySet(),
Collections.singleton(priv(PrivilegeConstants.JCR_MODIFY_PROPERTIES)));
- Set<Privilege> denySet = merged.values().stream()
- .filter(lp -> lp.isDeny())
- .map(lp -> lp.getPrivilege())
- .collect(Collectors.toSet());
-
- assertThat(denySet.size(), equalTo(3));
- assertThat(denySet, not(hasItems(priv(PrivilegeConstants.JCR_MODIFY_PROPERTIES))));
- assertThat(denySet, hasItems(
- priv(PrivilegeConstants.REP_ADD_PROPERTIES),
- priv(PrivilegeConstants.REP_ALTER_PROPERTIES),
- priv(PrivilegeConstants.REP_REMOVE_PROPERTIES)));
+ assertDenySetHasModifyPropertiesLeafs(merged);
// undeny jcr:modifyProperties
PrivilegesHelper.undeny(merged,
diff --git a/src/test/java/org/apache/sling/jcr/jackrabbit/accessmanager/it/AccessManagerClientTestSupport.java b/src/test/java/org/apache/sling/jcr/jackrabbit/accessmanager/it/AccessManagerClientTestSupport.java
index 68c19d6..5737aa1 100644
--- a/src/test/java/org/apache/sling/jcr/jackrabbit/accessmanager/it/AccessManagerClientTestSupport.java
+++ b/src/test/java/org/apache/sling/jcr/jackrabbit/accessmanager/it/AccessManagerClientTestSupport.java
@@ -291,14 +291,14 @@
assertNotNull(privilegeObj);
String key = privilegeState.toString();;
if (expectedForAllow) {
- assertTrue("Expected privilege " + privilegeName + " to have key '" + key,
+ assertTrue("Expected privilege " + privilegeName + " to have key: " + key,
privilegeObj.containsKey(key));
JsonValue jsonValue = privilegeObj.get(key);
if (verifyAce != null) {
verifyAce.verify(jsonValue);
}
} else {
- assertFalse("Did not expect privilege " + privilegeName + " to have key '" + key,
+ assertFalse("Did not expect privilege " + privilegeName + " to have key: " + key,
privilegeObj.containsKey(key));
}
}
diff --git a/src/test/java/org/apache/sling/jcr/jackrabbit/accessmanager/it/GetEaceIT.java b/src/test/java/org/apache/sling/jcr/jackrabbit/accessmanager/it/GetEaceIT.java
index 0e71730..42eca5e 100644
--- a/src/test/java/org/apache/sling/jcr/jackrabbit/accessmanager/it/GetEaceIT.java
+++ b/src/test/java/org/apache/sling/jcr/jackrabbit/accessmanager/it/GetEaceIT.java
@@ -290,7 +290,7 @@
assertNotNull(jsonContent);
JsonObject jsonObject = parseJson(jsonContent);
assertTrue("Expected childPropOne property", jsonObject.containsKey("childPropOne"));
- assertFalse("Did not expected childPropTwo property", jsonObject.containsKey("childPropTwo"));
+ assertFalse("Did not expect childPropTwo property", jsonObject.containsKey("childPropTwo"));
// add ACE to the child to make the other property readable
List<NameValuePair> postParams2 = new AcePostParamsBuilder(testUserId)
diff --git a/src/test/java/org/apache/sling/jcr/jackrabbit/accessmanager/it/ModifyAceIT.java b/src/test/java/org/apache/sling/jcr/jackrabbit/accessmanager/it/ModifyAceIT.java
index 63266ec..801b159 100644
--- a/src/test/java/org/apache/sling/jcr/jackrabbit/accessmanager/it/ModifyAceIT.java
+++ b/src/test/java/org/apache/sling/jcr/jackrabbit/accessmanager/it/ModifyAceIT.java
@@ -40,6 +40,7 @@
import javax.servlet.http.HttpServletResponse;
import org.apache.http.NameValuePair;
+import org.apache.jackrabbit.JcrConstants;
import org.apache.jackrabbit.oak.spi.security.authorization.accesscontrol.AccessControlConstants;
import org.apache.jackrabbit.oak.spi.security.authorization.restriction.RestrictionProvider;
import org.apache.jackrabbit.oak.spi.security.privilege.PrivilegeConstants;
@@ -2365,4 +2366,100 @@
assertPrivilege(privilegesObject, true, PrivilegeValues.DENY, PrivilegeConstants.JCR_WRITE, verifyRestrictions);
}
+ /**
+ * Test to verify adding an ACE does not include an allow aggregate privilege
+ * when there is a deny child privilege with the same restrictions as the parent
+ */
+ @Test
+ public void testNoAggregateAllowWhenDenyChildHasSameRestriction() throws IOException, JsonException {
+ testUserId = createTestUser();
+ testFolderUrl = createTestFolder();
+
+ // update the ACE
+ List<NameValuePair> postParams = new AcePostParamsBuilder(testUserId)
+ //allow the child privileges with varying restrictions
+ .withPrivilege(PrivilegeConstants.REP_READ_NODES, PrivilegeValues.ALLOW)
+ .withPrivilege(PrivilegeConstants.REP_READ_PROPERTIES, PrivilegeValues.ALLOW)
+ .withPrivilegeRestriction(PrivilegeValues.ALLOW, PrivilegeConstants.REP_READ_PROPERTIES,
+ AccessControlConstants.REP_ITEM_NAMES, new String[] {JcrConstants.JCR_CREATED, JcrConstants.JCR_PRIMARYTYPE})
+ //deny a child privilege with different restrictions than the same allow privilege
+ .withPrivilege(PrivilegeConstants.REP_READ_PROPERTIES, PrivilegeValues.DENY)
+ .build();
+ addOrUpdateAce(testFolderUrl, postParams);
+
+ JsonObject user = getAce(testFolderUrl, testUserId);
+ assertNotNull(user);
+ assertEquals(testUserId, user.getString("principal"));
+
+ JsonObject privilegesObject = user.getJsonObject("privileges");
+ assertNotNull(privilegesObject);
+ assertEquals(2, privilegesObject.size());
+
+ VerifyAce verifyRestrictions = jsonValue -> {
+ assertNotNull(jsonValue);
+ assertTrue(jsonValue instanceof JsonObject);
+ JsonObject restrictionsObj = (JsonObject)jsonValue;
+
+ JsonValue customValue = restrictionsObj.get(AccessControlConstants.REP_ITEM_NAMES);
+ assertNotNull(customValue);
+ assertTrue(customValue instanceof JsonArray);
+ assertEquals(2, ((JsonArray)customValue).size());
+ assertEquals(JcrConstants.JCR_CREATED, ((JsonArray)customValue).getString(0));
+ assertEquals(JcrConstants.JCR_PRIMARYTYPE, ((JsonArray)customValue).getString(1));
+ };
+ //allow privilege
+ assertPrivilege(privilegesObject, true, PrivilegeValues.ALLOW, PrivilegeConstants.REP_READ_NODES, verifyTrue);
+ assertPrivilege(privilegesObject, true, PrivilegeValues.ALLOW, PrivilegeConstants.REP_READ_PROPERTIES, verifyRestrictions);
+ //deny privilege
+ assertPrivilege(privilegesObject, true, PrivilegeValues.DENY, PrivilegeConstants.REP_READ_PROPERTIES, verifyTrue);
+ }
+
+ /**
+ * Test to verify adding an ACE does not include a deny aggregate privilege
+ * when there is an allow child privilege with the same restrictions as the parent
+ */
+ @Test
+ public void testNoAggregateDenyWhenAllowChildHasSameRestriction() throws IOException, JsonException {
+ testUserId = createTestUser();
+ testFolderUrl = createTestFolder();
+
+ // update the ACE
+ List<NameValuePair> postParams = new AcePostParamsBuilder(testUserId)
+ //deny the child privileges with varying restrictions
+ .withPrivilege(PrivilegeConstants.REP_READ_NODES, PrivilegeValues.DENY)
+ .withPrivilege(PrivilegeConstants.REP_READ_PROPERTIES, PrivilegeValues.DENY)
+ .withPrivilegeRestriction(PrivilegeValues.DENY, PrivilegeConstants.REP_READ_PROPERTIES,
+ AccessControlConstants.REP_ITEM_NAMES, new String[] {JcrConstants.JCR_CREATED, JcrConstants.JCR_PRIMARYTYPE})
+ //allow a child privilege with different restrictions than the same deny privilege
+ .withPrivilege(PrivilegeConstants.REP_READ_PROPERTIES, PrivilegeValues.ALLOW)
+ .build();
+ addOrUpdateAce(testFolderUrl, postParams);
+
+ JsonObject user = getAce(testFolderUrl, testUserId);
+ assertNotNull(user);
+ assertEquals(testUserId, user.getString("principal"));
+
+ JsonObject privilegesObject = user.getJsonObject("privileges");
+ assertNotNull(privilegesObject);
+ assertEquals(2, privilegesObject.size());
+
+ VerifyAce verifyRestrictions = jsonValue -> {
+ assertNotNull(jsonValue);
+ assertTrue(jsonValue instanceof JsonObject);
+ JsonObject restrictionsObj = (JsonObject)jsonValue;
+
+ JsonValue customValue = restrictionsObj.get(AccessControlConstants.REP_ITEM_NAMES);
+ assertNotNull(customValue);
+ assertTrue(customValue instanceof JsonArray);
+ assertEquals(2, ((JsonArray)customValue).size());
+ assertEquals(JcrConstants.JCR_CREATED, ((JsonArray)customValue).getString(0));
+ assertEquals(JcrConstants.JCR_PRIMARYTYPE, ((JsonArray)customValue).getString(1));
+ };
+ //allow privilege
+ assertPrivilege(privilegesObject, true, PrivilegeValues.ALLOW, PrivilegeConstants.REP_READ_PROPERTIES, verifyTrue);
+ //deny privilege
+ assertPrivilege(privilegesObject, true, PrivilegeValues.DENY, PrivilegeConstants.REP_READ_NODES, verifyTrue);
+ assertPrivilege(privilegesObject, true, PrivilegeValues.DENY, PrivilegeConstants.REP_READ_PROPERTIES, verifyRestrictions);
+ }
+
}
