actinium15 commented on a change in pull request #40:
URL:
https://github.com/apache/sling-org-apache-sling-distribution-core/pull/40#discussion_r413718474
##########
File path:
src/main/java/org/apache/sling/distribution/agent/impl/PrivilegeDistributionRequestAuthorizationStrategy.java
##########
@@ -37,12 +39,18 @@
private final String jcrPrivilege;
- public PrivilegeDistributionRequestAuthorizationStrategy(String
jcrPrivilege) {
+ private String[] additionalJcrPrivilegesForAdd;
+
+ private String[] additionalJcrPrivilegesForDelete;
Review comment:
> I change the
additionalJcrPrivilegesForDelete/additionalJcrPrivilegesForAdd arrays based on
their nullability so can't use final here.
Um, but I don't see any reason why you'd need to do that. You can, in fact,
you should move the initializations in the CTOR itself. Something like this:
```
---
a/src/main/java/org/apache/sling/distribution/agent/impl/PrivilegeDistributionRequestAuthorizationStrategy.java
+++
b/src/main/java/org/apache/sling/distribution/agent/impl/PrivilegeDistributionRequestAuthorizationStrategy.java
@@ -39,9 +39,9 @@ public class
PrivilegeDistributionRequestAuthorizationStrategy implements Distri
private final String jcrPrivilege;
- private String[] additionalJcrPrivilegesForAdd;
+ private final String[] additionalJcrPrivilegesForAdd;
- private String[] additionalJcrPrivilegesForDelete;
+ private final String[] additionalJcrPrivilegesForDelete;
public PrivilegeDistributionRequestAuthorizationStrategy(String
jcrPrivilege, String[] additionalJcrPrivilegesForAdd, String[]
additionalJcrPrivilegesForDelete) {
if (jcrPrivilege == null) {
@@ -49,8 +49,12 @@ public class
PrivilegeDistributionRequestAuthorizationStrategy implements Distri
}
this.jcrPrivilege = jcrPrivilege;
- this.additionalJcrPrivilegesForAdd = additionalJcrPrivilegesForAdd;
- this.additionalJcrPrivilegesForDelete =
additionalJcrPrivilegesForDelete;
+ this.additionalJcrPrivilegesForAdd =
+ additionalJcrPrivilegesForAdd != null ?
+ additionalJcrPrivilegesForAdd : new String[]
{Privilege.JCR_READ};;
+ this.additionalJcrPrivilegesForDelete =
+ additionalJcrPrivilegesForDelete != null ?
+ additionalJcrPrivilegesForDelete : new String[]
{Privilege.JCR_REMOVE_NODE};;
}
public void checkPermission(@NotNull ResourceResolver resourceResolver,
@NotNull DistributionRequest distributionRequest) throws DistributionException {
@@ -76,7 +80,6 @@ public class
PrivilegeDistributionRequestAuthorizationStrategy implements Distri
private void checkPermissionForAdd(Session session, String[] paths)
throws RepositoryException, DistributionException {
AccessControlManager acMgr = session.getAccessControlManager();
- additionalJcrPrivilegesForAdd = additionalJcrPrivilegesForAdd !=
null ? additionalJcrPrivilegesForAdd : new String[] {Privilege.JCR_READ};
Privilege[] privileges = computePrivileges(acMgr, jcrPrivilege,
additionalJcrPrivilegesForAdd);
for (String path : paths) {
if (!acMgr.hasPrivileges(path, privileges)) {
@@ -89,7 +92,6 @@ public class
PrivilegeDistributionRequestAuthorizationStrategy implements Distri
private void checkPermissionForDelete(Session session, String[] paths)
throws RepositoryException, DistributionException {
AccessControlManager acMgr = session.getAccessControlManager();
- additionalJcrPrivilegesForDelete = additionalJcrPrivilegesForDelete
!= null ? additionalJcrPrivilegesForDelete : new String[]
{Privilege.JCR_REMOVE_NODE};
Privilege[] privileges = computePrivileges(acMgr, jcrPrivilege,
additionalJcrPrivilegesForDelete);
for (String path : paths) {
```
Would you think above is a bad idea? if yes, why? What am I missing here?
----------------------------------------------------------------
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:
[email protected]
