tmaret commented on a change in pull request #40:
URL:
https://github.com/apache/sling-org-apache-sling-distribution-core/pull/40#discussion_r413630674
##########
File path:
src/main/java/org/apache/sling/distribution/agent/impl/PrivilegeDistributionRequestAuthorizationStrategy.java
##########
@@ -68,8 +76,8 @@ public void checkPermission(@NotNull ResourceResolver
resourceResolver, @NotNull
private void checkPermissionForAdd(Session session, String[] paths)
throws RepositoryException, DistributionException {
AccessControlManager acMgr = session.getAccessControlManager();
-
- Privilege[] privileges = new
Privilege[]{acMgr.privilegeFromName(jcrPrivilege),
acMgr.privilegeFromName(Privilege.JCR_READ)};
+ additionalJcrPrivilegesForAdd = additionalJcrPrivilegesForAdd != null
? additionalJcrPrivilegesForAdd : new String[] {Privilege.JCR_READ};
Review comment:
The backward compatible JRC_READ default should be a default value of
the `JCR_ADD_PRIVILEGES` property.
```
value = {"jcr:read"}
```
##########
File path:
src/main/java/org/apache/sling/distribution/agent/impl/PrivilegeDistributionRequestAuthorizationStrategy.java
##########
@@ -105,5 +113,14 @@ private String getClosestParent(Session session, String
path) throws RepositoryE
return null;
}
+ private static Privilege[] computePrivileges(AccessControlManager acMgr,
String[] jcrPrivileges, String jcrPrivilege)
Review comment:
Privileges are currently re-computed for every permission check. How
about computing them once in the constructor ? You may also keep track of
privileges in a `Set` to avoid potential duplicates.
##########
File path:
src/main/java/org/apache/sling/distribution/agent/impl/PrivilegeDistributionRequestAuthorizationStrategy.java
##########
@@ -81,8 +89,8 @@ private void checkPermissionForAdd(Session session, String[]
paths)
private void checkPermissionForDelete(Session session, String[] paths)
throws RepositoryException, DistributionException {
AccessControlManager acMgr = session.getAccessControlManager();
-
- Privilege[] privileges = new
Privilege[]{acMgr.privilegeFromName(jcrPrivilege),
acMgr.privilegeFromName(Privilege.JCR_REMOVE_NODE)};
+ additionalJcrPrivilegesForDelete = additionalJcrPrivilegesForDelete !=
null ? additionalJcrPrivilegesForDelete : new String[]
{Privilege.JCR_REMOVE_NODE};
Review comment:
The backward compatible JRC_REMOVE_NODE default should be a default
value of the `JCR_DELETE_PRIVILEGES` property.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
