You are viewing a plain text version of this content. The canonical link for it is here.
Posted to oak-commits@jackrabbit.apache.org by ju...@apache.org on 2013/12/02 21:19:58 UTC
svn commit: r1547174 - in
/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol:
AccessControlValidator.java AccessControlValidatorProvider.java
Author: jukka
Date: Mon Dec 2 20:19:58 2013
New Revision: 1547174
URL: http://svn.apache.org/r1547174
Log:
OAK-1252: Lazy privilege access in AccessControlValidator
Defer privilege access to when they're really needed
Modified:
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlValidator.java
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlValidatorProvider.java
Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlValidator.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlValidator.java?rev=1547174&r1=1547173&r2=1547174&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlValidator.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlValidator.java Mon Dec 2 20:19:58 2013
@@ -18,7 +18,6 @@ package org.apache.jackrabbit.oak.securi
import java.util.Collection;
import java.util.Collections;
-import java.util.Map;
import java.util.Set;
import javax.jcr.RepositoryException;
@@ -30,6 +29,7 @@ import com.google.common.collect.Iterabl
import com.google.common.collect.Sets;
import org.apache.jackrabbit.JcrConstants;
+import org.apache.jackrabbit.api.security.authorization.PrivilegeManager;
import org.apache.jackrabbit.oak.api.CommitFailedException;
import org.apache.jackrabbit.oak.api.PropertyState;
import org.apache.jackrabbit.oak.api.Tree;
@@ -61,19 +61,19 @@ class AccessControlValidator extends Def
private final Tree parentAfter;
private final PrivilegeBitsProvider privilegeBitsProvider;
- private final Map<String, Privilege> privileges;
+ private final PrivilegeManager privilegeManager;
private final RestrictionProvider restrictionProvider;
private final ReadOnlyNodeTypeManager ntMgr;
AccessControlValidator(Tree parentBefore, Tree parentAfter,
- Map<String, Privilege> privileges,
+ PrivilegeManager privilegeManager,
PrivilegeBitsProvider privilegeBitsProvider,
RestrictionProvider restrictionProvider,
ReadOnlyNodeTypeManager ntMgr) {
this.parentBefore = parentBefore;
this.parentAfter = parentAfter;
this.privilegeBitsProvider = privilegeBitsProvider;
- this.privileges = privileges;
+ this.privilegeManager = privilegeManager;
this.restrictionProvider = restrictionProvider;
this.ntMgr = ntMgr;
}
@@ -109,7 +109,7 @@ class AccessControlValidator extends Def
Tree treeAfter = checkNotNull(parentAfter.getChild(name));
checkValidTree(parentAfter, treeAfter, after);
- return new AccessControlValidator(null, treeAfter, privileges, privilegeBitsProvider, restrictionProvider, ntMgr);
+ return new AccessControlValidator(null, treeAfter, privilegeManager, privilegeBitsProvider, restrictionProvider, ntMgr);
}
@Override
@@ -118,7 +118,7 @@ class AccessControlValidator extends Def
Tree treeAfter = checkNotNull(parentAfter.getChild(name));
checkValidTree(parentAfter, treeAfter, after);
- return new AccessControlValidator(treeBefore, treeAfter, privileges, privilegeBitsProvider, restrictionProvider, ntMgr);
+ return new AccessControlValidator(treeBefore, treeAfter, privilegeManager, privilegeBitsProvider, restrictionProvider, ntMgr);
}
@Override
@@ -220,13 +220,15 @@ class AccessControlValidator extends Def
throw accessViolation(9, "Missing privileges.");
}
for (String privilegeName : privilegeNames) {
- if (privilegeName == null || !privileges.containsKey(privilegeName)) {
+ try {
+ Privilege privilege = privilegeManager.getPrivilege(privilegeName);
+ if (privilege.isAbstract()) {
+ throw accessViolation(11, "Abstract privilege " + privilegeName);
+ }
+ } catch (AccessControlException e) {
throw accessViolation(10, "Invalid privilege " + privilegeName);
- }
-
- Privilege privilege = privileges.get(privilegeName);
- if (privilege.isAbstract()) {
- throw accessViolation(11, "Abstract privilege " + privilegeName);
+ } catch (RepositoryException e) {
+ throw new RuntimeException(e);
}
}
}
Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlValidatorProvider.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlValidatorProvider.java?rev=1547174&r1=1547173&r2=1547174&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlValidatorProvider.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlValidatorProvider.java Mon Dec 2 20:19:58 2013
@@ -16,12 +16,8 @@
*/
package org.apache.jackrabbit.oak.security.authorization.accesscontrol;
-import java.util.Map;
import javax.annotation.Nonnull;
-import javax.jcr.RepositoryException;
-import javax.jcr.security.Privilege;
-import com.google.common.collect.ImmutableMap;
import org.apache.jackrabbit.api.security.authorization.PrivilegeManager;
import org.apache.jackrabbit.oak.api.Root;
import org.apache.jackrabbit.oak.api.Tree;
@@ -37,8 +33,6 @@ import org.apache.jackrabbit.oak.spi.sec
import org.apache.jackrabbit.oak.spi.security.privilege.PrivilegeBitsProvider;
import org.apache.jackrabbit.oak.spi.security.privilege.PrivilegeConfiguration;
import org.apache.jackrabbit.oak.spi.state.NodeState;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
/**
* {@code AccessControlValidatorProvider} aimed to provide a root validator
@@ -48,8 +42,6 @@ import org.slf4j.LoggerFactory;
*/
public class AccessControlValidatorProvider extends ValidatorProvider {
- private static final Logger log = LoggerFactory.getLogger(AccessControlValidatorProvider.class);
-
private final SecurityProvider securityProvider;
public AccessControlValidatorProvider(@Nonnull SecurityProvider securityProvider) {
@@ -66,24 +58,11 @@ public class AccessControlValidatorProvi
RestrictionProvider restrictionProvider = getConfig(AuthorizationConfiguration.class).getRestrictionProvider();
Root root = new ImmutableRoot(before);
- Map<String, Privilege> privileges = getPrivileges(root);
+ PrivilegeManager privilegeManager = getConfig(PrivilegeConfiguration.class).getPrivilegeManager(root, NamePathMapper.DEFAULT);
PrivilegeBitsProvider privilegeBitsProvider = new PrivilegeBitsProvider(root);
ReadOnlyNodeTypeManager ntMgr = ReadOnlyNodeTypeManager.getInstance(before);
- return new AccessControlValidator(rootBefore, rootAfter, privileges, privilegeBitsProvider, restrictionProvider, ntMgr);
- }
-
- private Map<String, Privilege> getPrivileges(Root root) {
- PrivilegeManager pMgr = getConfig(PrivilegeConfiguration.class).getPrivilegeManager(root, NamePathMapper.DEFAULT);
- ImmutableMap.Builder privileges = ImmutableMap.builder();
- try {
- for (Privilege privilege : pMgr.getRegisteredPrivileges()) {
- privileges.put(privilege.getName(), privilege);
- }
- } catch (RepositoryException e) {
- log.error("Unexpected error: failed to read privileges.", e);
- }
- return privileges.build();
+ return new AccessControlValidator(rootBefore, rootAfter, privilegeManager, privilegeBitsProvider, restrictionProvider, ntMgr);
}
private <T> T getConfig(Class<T> configClass) {
Re: svn commit: r1547174 - in /jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol:
AccessControlValidator.java AccessControlValidatorProvider.java
Posted by Jukka Zitting <ju...@gmail.com>.
Hi,
On Tue, Dec 3, 2013 at 3:08 AM, Michael Dürig <md...@apache.org> wrote:
> Could we include some form of an explanation why a RuntimeException is
> thrown here. E.g. throw a tailored descendant of RuntimeException including
> some extra message?
Good point, I kind of thought about that but then dropped the ball.
The replaced code used to log an "Unexpected error: failed to read
privileges." error message in that case, and would follow up to fail
the commit because matching privileges were not found. That kind of
confuses the purpose of the commit failure (was it caused by an
unexpected repository error or a validation issue) so I figured it
would be better to throw a separate exception in that case. Given our
practice of using checked exceptions for recoverable issues (adjust
the content being committed, etc.) and unchecked ones for unexpected
issues (network problems, etc.) I went for RuntimeException but left
it at that. Fixed in revision 1547319.
BR,
Jukka Zitting
Re: svn commit: r1547174 - in /jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol:
AccessControlValidator.java AccessControlValidatorProvider.java
Posted by Michael Dürig <md...@apache.org>.
On 2.12.13 9:19 , jukka@apache.org wrote:
> Author: jukka
> Date: Mon Dec 2 20:19:58 2013
> New Revision: 1547174
>
> URL: http://svn.apache.org/r1547174
> Log:
> OAK-1252: Lazy privilege access in AccessControlValidator
>
> Defer privilege access to when they're really needed
[...]
> for (String privilegeName : privilegeNames) {
> - if (privilegeName == null || !privileges.containsKey(privilegeName)) {
> + try {
> + Privilege privilege = privilegeManager.getPrivilege(privilegeName);
> + if (privilege.isAbstract()) {
> + throw accessViolation(11, "Abstract privilege " + privilegeName);
> + }
> + } catch (AccessControlException e) {
> throw accessViolation(10, "Invalid privilege " + privilegeName);
> - }
> -
> - Privilege privilege = privileges.get(privilegeName);
> - if (privilege.isAbstract()) {
> - throw accessViolation(11, "Abstract privilege " + privilegeName);
> + } catch (RepositoryException e) {
> + throw new RuntimeException(e);
> }
> }
> }
Could we include some form of an explanation why a RuntimeException is
thrown here. E.g. throw a tailored descendant of RuntimeException
including some extra message?
Michael