Author: angela Date: Wed Sep 18 09:02:56 2019 New Revision: 1867106 URL: http://svn.apache.org/viewvc?rev=1867106&view=rev Log: OAK-8632 : rep:CugPolicy nodes containing duplicate principal names
Modified: jackrabbit/oak/trunk/oak-authorization-cug/src/main/java/org/apache/jackrabbit/oak/spi/security/authorization/cug/impl/CugAccessControlManager.java jackrabbit/oak/trunk/oak-authorization-cug/src/main/java/org/apache/jackrabbit/oak/spi/security/authorization/cug/impl/CugPolicyImpl.java jackrabbit/oak/trunk/oak-authorization-cug/src/test/java/org/apache/jackrabbit/oak/spi/security/authorization/cug/impl/CugPolicyImplTest.java Modified: jackrabbit/oak/trunk/oak-authorization-cug/src/main/java/org/apache/jackrabbit/oak/spi/security/authorization/cug/impl/CugAccessControlManager.java URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-authorization-cug/src/main/java/org/apache/jackrabbit/oak/spi/security/authorization/cug/impl/CugAccessControlManager.java?rev=1867106&r1=1867105&r2=1867106&view=diff ============================================================================== --- jackrabbit/oak/trunk/oak-authorization-cug/src/main/java/org/apache/jackrabbit/oak/spi/security/authorization/cug/impl/CugAccessControlManager.java (original) +++ jackrabbit/oak/trunk/oak-authorization-cug/src/main/java/org/apache/jackrabbit/oak/spi/security/authorization/cug/impl/CugAccessControlManager.java Wed Sep 18 09:02:56 2019 @@ -27,6 +27,7 @@ import javax.jcr.security.AccessControlP import javax.jcr.security.AccessControlPolicyIterator; import javax.jcr.security.Privilege; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.common.collect.Sets; @@ -259,12 +260,12 @@ class CugAccessControlManager extends Ab } } - private Set<Principal> getPrincipals(@NotNull Tree cugTree) { + private Iterable<Principal> getPrincipals(@NotNull Tree cugTree) { PropertyState property = cugTree.getProperty(REP_PRINCIPAL_NAMES); if (property == null) { return Collections.emptySet(); } else { - return ImmutableSet.copyOf(Iterables.transform(property.getValue(Type.STRINGS), principalName -> { + return ImmutableList.copyOf(Iterables.transform(property.getValue(Type.STRINGS), principalName -> { Principal principal = principalManager.getPrincipal(principalName); if (principal == null) { log.debug("Unknown principal {}", principalName); Modified: jackrabbit/oak/trunk/oak-authorization-cug/src/main/java/org/apache/jackrabbit/oak/spi/security/authorization/cug/impl/CugPolicyImpl.java URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-authorization-cug/src/main/java/org/apache/jackrabbit/oak/spi/security/authorization/cug/impl/CugPolicyImpl.java?rev=1867106&r1=1867105&r2=1867106&view=diff ============================================================================== --- jackrabbit/oak/trunk/oak-authorization-cug/src/main/java/org/apache/jackrabbit/oak/spi/security/authorization/cug/impl/CugPolicyImpl.java (original) +++ jackrabbit/oak/trunk/oak-authorization-cug/src/main/java/org/apache/jackrabbit/oak/spi/security/authorization/cug/impl/CugPolicyImpl.java Wed Sep 18 09:02:56 2019 @@ -16,14 +16,7 @@ */ package org.apache.jackrabbit.oak.spi.security.authorization.cug.impl; -import java.security.Principal; -import java.util.Collections; -import java.util.HashSet; -import java.util.Set; -import javax.jcr.security.AccessControlException; - import com.google.common.base.Strings; -import com.google.common.collect.Iterables; import com.google.common.collect.Sets; import org.apache.jackrabbit.api.security.principal.PrincipalManager; import org.apache.jackrabbit.oak.namepath.NamePathMapper; @@ -35,6 +28,13 @@ import org.jetbrains.annotations.Nullabl import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import javax.jcr.security.AccessControlException; +import java.security.Principal; +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.Map; +import java.util.Set; + /** * Implementation of the {@link org.apache.jackrabbit.oak.spi.security.authorization.cug.CugPolicy} * interface that respects the configured {@link org.apache.jackrabbit.oak.spi.xml.ImportBehavior}. @@ -49,7 +49,7 @@ class CugPolicyImpl implements CugPolicy private final int importBehavior; private final CugExclude cugExclude; - private final Set<Principal> principals = new HashSet<>(); + private final Map<String,Principal> principals = new LinkedHashMap<>(); CugPolicyImpl(@NotNull String oakPath, @NotNull NamePathMapper namePathMapper, @NotNull PrincipalManager principalManager, int importBehavior, @NotNull CugExclude cugExclude) { @@ -58,28 +58,31 @@ class CugPolicyImpl implements CugPolicy CugPolicyImpl(@NotNull String oakPath, @NotNull NamePathMapper namePathMapper, @NotNull PrincipalManager principalManager, int importBehavior, - @NotNull CugExclude cugExclude, @NotNull Set<Principal> principals) { + @NotNull CugExclude cugExclude, @NotNull Iterable<Principal> principals) { ImportBehavior.nameFromValue(importBehavior); this.oakPath = oakPath; this.namePathMapper = namePathMapper; this.principalManager = principalManager; this.importBehavior = importBehavior; this.cugExclude = cugExclude; - this.principals.addAll(principals); + for (Principal principal : principals) { + this.principals.put(principal.getName(), principal); + } } @NotNull @Override public Set<Principal> getPrincipals() { - return Sets.newHashSet(principals); + return Sets.newHashSet(principals.values()); } @Override public boolean addPrincipals(@NotNull Principal... principals) throws AccessControlException { boolean modified = false; for (Principal principal : principals) { - if (isValidPrincipal(principal)) { - modified |= this.principals.add(principal); + if (isValidPrincipal(principal) && !this.principals.containsKey(principal.getName())) { + this.principals.put(principal.getName(), principal); + modified = true; } } return modified; @@ -89,8 +92,9 @@ class CugPolicyImpl implements CugPolicy public boolean removePrincipals(@NotNull Principal... principals) { boolean modified = false; for (Principal principal : principals) { - if (principal != null) { - modified |= this.principals.remove(principal); + if (principal != null && this.principals.containsKey(principal.getName())) { + this.principals.remove(principal.getName()); + modified = true; } } return modified; @@ -104,7 +108,7 @@ class CugPolicyImpl implements CugPolicy //-------------------------------------------------------------------------- Iterable<String> getPrincipalNames() { - return Iterables.transform(principals, Principal::getName); + return Sets.newHashSet(principals.keySet()); } //-------------------------------------------------------------------------- Modified: jackrabbit/oak/trunk/oak-authorization-cug/src/test/java/org/apache/jackrabbit/oak/spi/security/authorization/cug/impl/CugPolicyImplTest.java URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-authorization-cug/src/test/java/org/apache/jackrabbit/oak/spi/security/authorization/cug/impl/CugPolicyImplTest.java?rev=1867106&r1=1867105&r2=1867106&view=diff ============================================================================== --- jackrabbit/oak/trunk/oak-authorization-cug/src/test/java/org/apache/jackrabbit/oak/spi/security/authorization/cug/impl/CugPolicyImplTest.java (original) +++ jackrabbit/oak/trunk/oak-authorization-cug/src/test/java/org/apache/jackrabbit/oak/spi/security/authorization/cug/impl/CugPolicyImplTest.java Wed Sep 18 09:02:56 2019 @@ -69,11 +69,11 @@ public class CugPolicyImplTest extends A return new CugPolicyImpl(path, NamePathMapper.DEFAULT, principalManager, importBehavior, exclude); } - private CugPolicyImpl createCugPolicy(@NotNull Set<Principal> principals) { + private CugPolicyImpl createCugPolicy(@NotNull Iterable<Principal> principals) { return createCugPolicy(ImportBehavior.ABORT, principals); } - private CugPolicyImpl createCugPolicy(int importBehavior, @NotNull Set<Principal> principals) { + private CugPolicyImpl createCugPolicy(int importBehavior, @NotNull Iterable<Principal> principals) { return new CugPolicyImpl(path, NamePathMapper.DEFAULT, principalManager, importBehavior, exclude, principals); } @@ -104,6 +104,21 @@ public class CugPolicyImplTest extends A } @Test + public void testCreateWithDuplicateName() { + Set<Principal> duplication = ImmutableSet.of(testPrincipal, new Principal() { + @Override + public String getName() { + return testPrincipal.getName(); + } + }); + assertEquals(2, duplication.size()); + + CugPolicyImpl cugPolicy = createCugPolicy(duplication); + assertEquals(1, cugPolicy.getPrincipals().size()); + assertEquals(1, Iterables.size(cugPolicy.getPrincipalNames())); + } + + @Test public void testGetPrincipalNames() { CugPolicyImpl cug = createCugPolicy(principals); @@ -173,6 +188,13 @@ public class CugPolicyImplTest extends A } @Test + public void testAddContainedPrincipalNonEqualImpl() throws Exception { + CugPolicy cug = createCugPolicy(ImportBehavior.BESTEFFORT, principals); + assertFalse(cug.addPrincipals((Principal) () -> testPrincipal.getName())); + assertEquals(principals, cug.getPrincipals()); + } + + @Test public void testAddNullPrincipal() throws Exception { CugPolicy cug = createCugPolicy(ImportBehavior.ABORT, principals); assertTrue(cug.addPrincipals(EveryonePrincipal.getInstance(), null)); @@ -206,7 +228,13 @@ public class CugPolicyImplTest extends A public void testRemoveNullPrincipal() throws Exception { CugPolicy cug = createCugPolicy(ImportBehavior.ABORT, principals); assertTrue(cug.removePrincipals(testPrincipal, null)); + assertTrue(cug.getPrincipals().isEmpty()); + } + @Test + public void testRemoveContainedPrincipalNotEqual() throws Exception { + CugPolicy cug = createCugPolicy(ImportBehavior.BESTEFFORT, principals); + assertTrue(cug.removePrincipals((Principal) () -> testPrincipal.getName())); assertTrue(cug.getPrincipals().isEmpty()); }