Index: oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/basic/DefaultSyncContext.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/basic/DefaultSyncContext.java (revision 1709519) +++ oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/basic/DefaultSyncContext.java (revision ) @@ -297,43 +297,21 @@ return new DefaultSyncResultImpl(new DefaultSyncedIdentity(id, null, false, -1), SyncResult.Status.FOREIGN); } - if (auth instanceof Group) { - Group group = (Group) auth; + if (auth.isGroup()) { ExternalGroup external = idp.getGroup(id); timer.mark("retrieve"); if (external == null) { - DefaultSyncedIdentity syncId = DefaultSyncContext.createSyncedIdentity(auth); - if (group.getDeclaredMembers().hasNext()) { - log.info("won't remove local group with members: {}", id); - ret = new DefaultSyncResultImpl(syncId, SyncResult.Status.NOP); - } else if (!keepMissing) { - auth.remove(); - log.debug("removing authorizable '{}' that no longer exists on IDP {}", id, idp.getName()); - timer.mark("remove"); - ret = new DefaultSyncResultImpl(syncId, SyncResult.Status.DELETE); + ret = handleMissingIdentity(id, auth, timer); - } else { + } else { - ret = new DefaultSyncResultImpl(syncId, SyncResult.Status.MISSING); - log.info("external identity missing for {}, but purge == false.", id); - } - } else { - ret = syncGroup(external, group); + ret = syncGroup(external, (Group) auth); timer.mark("sync"); } } else { ExternalUser external = idp.getUser(id); timer.mark("retrieve"); if (external == null) { - DefaultSyncedIdentity syncId = DefaultSyncContext.createSyncedIdentity(auth); - if (!keepMissing) { - auth.remove(); - log.debug("removing authorizable '{}' that no longer exists on IDP {}", id, idp.getName()); - timer.mark("remove"); - ret = new DefaultSyncResultImpl(syncId, SyncResult.Status.DELETE); + ret = handleMissingIdentity(id, auth, timer); - } else { + } else { - ret = new DefaultSyncResultImpl(syncId, SyncResult.Status.MISSING); - log.info("external identity missing for {}, but purge == false.", id); - } - } else { ret = syncUser(external, (User) auth); timer.mark("sync"); } @@ -349,6 +327,26 @@ } } + private DefaultSyncResultImpl handleMissingIdentity(@Nonnull String id, + @Nonnull Authorizable authorizable, + @Nonnull DebugTimer timer) throws RepositoryException { + DefaultSyncedIdentity syncId = DefaultSyncContext.createSyncedIdentity(authorizable); + SyncResult.Status status; + if (authorizable.isGroup() && ((Group) authorizable).getDeclaredMembers().hasNext()) { + log.info("won't remove local group with members: {}", id); + status = SyncResult.Status.NOP; + } else if (!keepMissing) { + authorizable.remove(); + log.debug("removing authorizable '{}' that no longer exists on IDP {}", id, idp.getName()); + timer.mark("remove"); + status = SyncResult.Status.DELETE; + } else { + status = SyncResult.Status.MISSING; + log.info("external identity missing for {}, but purge == false.", id); + } + return new DefaultSyncResultImpl(syncId, status); + } + /** * Retrieves the repository authorizable that corresponds to the given external identity * @param external the external identity @@ -418,50 +416,60 @@ @Nonnull protected DefaultSyncResultImpl syncUser(@Nonnull ExternalUser external, @Nonnull User user) throws RepositoryException { - // first check if user is expired - if (!forceUserSync && !isExpired(user, config.user().getExpirationTime(), "Properties")) { - DefaultSyncedIdentity syncId = DefaultSyncContext.createSyncedIdentity(user); - return new DefaultSyncResultImpl(syncId, SyncResult.Status.NOP); - } + SyncResult.Status status; - // synchronize the properties - syncProperties(external, user, config.user().getPropertyMapping()); - - // synchronize auto-group membership - applyMembership(user, config.user().getAutoMembership()); - + // first check if user is expired + if (!forceUserSync && !isExpired(user)) { + status = SyncResult.Status.NOP; + } else { + syncExternalIdentity(external, user, config.user()); - if (isExpired(user, config.user().getMembershipExpirationTime(), "Membership")) { - // synchronize external memberships - syncMembership(external, user, config.user().getMembershipNestingDepth()); - } + if (isExpired(user, config.user().getMembershipExpirationTime(), "Membership")) { + // synchronize external memberships + syncMembership(external, user, config.user().getMembershipNestingDepth()); + } - - // finally "touch" the sync property - user.setProperty(REP_LAST_SYNCED, nowValue); + // finally "touch" the sync property + user.setProperty(REP_LAST_SYNCED, nowValue); - DefaultSyncedIdentity syncId = DefaultSyncContext.createSyncedIdentity(user); - return new DefaultSyncResultImpl(syncId, SyncResult.Status.UPDATE); + status = SyncResult.Status.UPDATE; - } + } + return new DefaultSyncResultImpl(createSyncedIdentity(user), status); + } + @Nonnull protected DefaultSyncResultImpl syncGroup(@Nonnull ExternalGroup external, @Nonnull Group group) throws RepositoryException { + SyncResult.Status status; // first check if user is expired - if (!forceGroupSync && !isExpired(group, config.group().getExpirationTime(), "Properties")) { - DefaultSyncedIdentity syncId = DefaultSyncContext.createSyncedIdentity(group); - return new DefaultSyncResultImpl(syncId, SyncResult.Status.NOP); - } + if (!forceGroupSync && !isExpired(group)) { + status = SyncResult.Status.NOP; + } else { + syncExternalIdentity(external, group, config.group()); - // synchronize the properties - syncProperties(external, group, config.group().getPropertyMapping()); - - // synchronize auto-group membership - applyMembership(group, config.group().getAutoMembership()); - - // finally "touch" the sync property - group.setProperty(REP_LAST_SYNCED, nowValue); + // finally "touch" the sync property + group.setProperty(REP_LAST_SYNCED, nowValue); - DefaultSyncedIdentity syncId = DefaultSyncContext.createSyncedIdentity(group); - return new DefaultSyncResultImpl(syncId, SyncResult.Status.UPDATE); + status = SyncResult.Status.UPDATE; - } + } + return new DefaultSyncResultImpl(createSyncedIdentity(group), status); + } + /** + * Synchronize content common to both external users and external groups: + * - properties + * - auto-group membership + * + * @param external The external identity + * @param authorizable The corresponding repository user/group + * @param config The sync configuration + * @throws RepositoryException If an error occurs. + */ + private void syncExternalIdentity(@Nonnull ExternalIdentity external, + @Nonnull Authorizable authorizable, + @Nonnull DefaultSyncConfig.Authorizable config) throws RepositoryException { + syncProperties(external, authorizable, config.getPropertyMapping()); + applyMembership(authorizable, config.getAutoMembership()); + } + + /** * Recursively sync the memberships of an authorizable up-to the specified depth. If the given depth * is equal or less than 0, no syncing is performed. * @@ -489,13 +497,13 @@ } timer.mark("fetching"); - // first get the set of the existing groups that are synced ones - Map declaredExternalGroups = new HashMap(); + // first get the set of the existing groups that belong to this IDP + Map lostMembership = new HashMap(); Iterator grpIter = auth.declaredMemberOf(); while (grpIter.hasNext()) { Group grp = grpIter.next(); if (isSameIDP(grp)) { - declaredExternalGroups.put(grp.getID(), grp); + lostMembership.put(grp.getID(), grp); } } timer.mark("reading"); @@ -503,45 +511,43 @@ for (ExternalIdentityRef ref : externalGroups) { log.debug("- processing membership {}", ref.getId()); // get group + ExternalGroup extGroup; try { - extGroup = (ExternalGroup) idp.getIdentity(ref); - } catch (ClassCastException e) { - // this should really not be the case, so catching the CCE is ok here. - log.warn("External identity '{}' is not a group, but should be one.", ref.getString()); + ExternalIdentity extId = idp.getIdentity(ref); + if (extId instanceof ExternalGroup) { + extGroup = (ExternalGroup) extId; + } else { + log.warn("No external group found for ref '{}'.", ref.getString()); - continue; + continue; + } } catch (ExternalIdentityException e) { log.warn("Unable to retrieve external group '{}' from provider.", ref.getString(), e); continue; } - if (extGroup == null) { - log.warn("External group for ref '{}' could not be retrieved from provider.", ref); - continue; - } log.debug("- idp returned '{}'", extGroup.getId()); Group grp; - try { - grp = (Group) userManager.getAuthorizable(extGroup.getId()); - } catch (ClassCastException e) { - // this should really not be the case, so catching the CCE is ok here. + Authorizable a = userManager.getAuthorizable(extGroup.getId()); + if (a == null) { + grp = createGroup(extGroup); + log.debug("- created new group"); + } else if (a.isGroup()) { + grp = (Group) a; + } else { log.warn("Authorizable '{}' is not a group, but should be one.", extGroup.getId()); continue; } log.debug("- user manager returned '{}'", grp); - if (grp == null) { - grp = createGroup(extGroup); - log.debug("- created new group"); - } syncGroup(extGroup, grp); // ensure membership grp.addMember(auth); log.debug("- added '{}' as member to '{}'", auth, grp); - // remember the declared group - declaredExternalGroups.remove(grp.getID()); + // update the collection of lost memberships + lostMembership.remove(grp.getID()); // recursively apply further membership if (depth > 1) { @@ -553,7 +559,7 @@ } timer.mark("adding"); // remove us from the lost membership groups - for (Group grp: declaredExternalGroups.values()) { + for (Group grp : lostMembership.values()) { grp.removeMember(auth); log.debug("- removing member '{}' for group '{}'", auth.getID(), grp.getID()); } @@ -617,6 +623,17 @@ } } } + } + + /** + * Checks if the given authorizable needs syncing based on the {@link #REP_LAST_SYNCED} property. + * + * @param authorizable the authorizable to check + * @return {@code true} if the authorizable needs sync + */ + private boolean isExpired(@Nonnull Authorizable authorizable) throws RepositoryException { + long expTime = (authorizable.isGroup()) ? config.group().getExpirationTime() : config.user().getExpirationTime(); + return isExpired(authorizable, expTime, "Properties"); } /** \ No newline at end of file Index: oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/basic/DefaultSyncContextTest.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/basic/DefaultSyncContextTest.java (revision ) +++ oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/basic/DefaultSyncContextTest.java (revision ) @@ -0,0 +1,385 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.jackrabbit.oak.spi.security.authentication.external.basic; + +import java.util.ArrayList; +import java.util.Iterator; +import java.util.List; +import java.util.Map; +import java.util.UUID; +import javax.annotation.CheckForNull; +import javax.annotation.Nonnull; +import javax.annotation.Nullable; +import javax.jcr.RepositoryException; + +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Iterators; +import org.apache.jackrabbit.api.security.user.Authorizable; +import org.apache.jackrabbit.api.security.user.Group; +import org.apache.jackrabbit.api.security.user.User; +import org.apache.jackrabbit.api.security.user.UserManager; +import org.apache.jackrabbit.oak.AbstractSecurityTest; +import org.apache.jackrabbit.oak.namepath.NamePathMapper; +import org.apache.jackrabbit.oak.plugins.value.ValueFactoryImpl; +import org.apache.jackrabbit.oak.spi.security.authentication.external.ExternalGroup; +import org.apache.jackrabbit.oak.spi.security.authentication.external.ExternalIdentity; +import org.apache.jackrabbit.oak.spi.security.authentication.external.ExternalIdentityRef; +import org.apache.jackrabbit.oak.spi.security.authentication.external.ExternalUser; +import org.apache.jackrabbit.oak.spi.security.authentication.external.SyncResult; +import org.apache.jackrabbit.oak.spi.security.authentication.external.TestIdentityProvider; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; + +public class DefaultSyncContextTest extends AbstractSecurityTest { + + private TestIdentityProvider idp = new TestIdentityProvider(); + private DefaultSyncConfig config = new DefaultSyncConfig(); + + private DefaultSyncContext syncCtx; + + private List authorizableIds = new ArrayList(); + + @Before + public void before() throws Exception { + super.before(); + syncCtx = new DefaultSyncContext(config, idp, getUserManager(root), new ValueFactoryImpl(root, NamePathMapper.DEFAULT)); + } + + @After + public void after() throws Exception { + try { + syncCtx.close(); + UserManager umgr = getUserManager(root); + Iterator ids = Iterators.concat(idp.listGroups(), idp.listUsers()); + while (ids.hasNext()) { + Authorizable a = umgr.getAuthorizable(ids.next().getId()); + if (a != null) { + a.remove(); + } + } + for (String id : authorizableIds) { + Authorizable a = umgr.getAuthorizable(id); + if (a != null) { + a.remove(); + } + } + root.commit(); + } finally { + super.after(); + } + } + + private Group createTestGroup() throws Exception { + Group gr = getUserManager(root).createGroup("group" + UUID.randomUUID()); + authorizableIds.add(gr.getID()); + return gr; + } + + private void setExternalID(@Nonnull Authorizable authorizable, @Nullable String idpName) throws RepositoryException { + authorizable.setProperty(DefaultSyncContext.REP_EXTERNAL_ID, getValueFactory().createValue(authorizable.getID() + ';' + idpName)); + } + + @Test(expected = IllegalArgumentException.class) + public void testSyncInvalidExternalIdentity() throws Exception { + syncCtx.sync(new TestExternalIdentity()); + } + + @Test + public void testSyncExternalUser() throws Exception { + ExternalUser user = idp.listUsers().next(); + assertNotNull(user); + + SyncResult result = syncCtx.sync(user); + assertEquals(SyncResult.Status.ADD, result.getStatus()); + + result = syncCtx.sync(user); + assertEquals(SyncResult.Status.NOP, result.getStatus()); + + syncCtx.setForceUserSync(true); + result = syncCtx.sync(user); + assertEquals(SyncResult.Status.UPDATE, result.getStatus()); + + } + + @Test + public void testSyncExternalGroup() throws Exception { + ExternalGroup gr = idp.listGroups().next(); + assertNotNull(gr); + + SyncResult result = syncCtx.sync(gr); + assertEquals(SyncResult.Status.ADD, result.getStatus()); + + result = syncCtx.sync(gr); + assertEquals(SyncResult.Status.NOP, result.getStatus()); + + syncCtx.setForceGroupSync(true); + result = syncCtx.sync(gr); + assertEquals(SyncResult.Status.UPDATE, result.getStatus()); + } + + @Test + public void testSyncUserById() throws Exception { + ExternalIdentity externalId = idp.listUsers().next(); + + // no initial sync -> sync-by-id doesn't succeed + SyncResult result = syncCtx.sync(externalId.getId()); + assertEquals(SyncResult.Status.NO_SUCH_AUTHORIZABLE, result.getStatus()); + + // force sync + syncCtx.sync(externalId); + + // try again + syncCtx.setForceUserSync(true); + result = syncCtx.sync(externalId.getId()); + assertEquals(SyncResult.Status.UPDATE, result.getStatus()); + } + + @Test + public void testSyncRemovedUserById() throws Exception { + // mark a regular repo user as external user from the test IDP + User u = getUserManager(root).createUser("test" + UUID.randomUUID(), null); + String userId = u.getID(); + authorizableIds.add(userId); + + setExternalID(u, idp.getName()); + + // test sync with 'keepmissing' = true + syncCtx.setKeepMissing(true); + SyncResult result = syncCtx.sync(userId); + assertEquals(SyncResult.Status.MISSING, result.getStatus()); + assertNotNull(getUserManager(root).getAuthorizable(userId)); + + // test sync with 'keepmissing' = false + syncCtx.setKeepMissing(false); + result = syncCtx.sync(userId); + assertEquals(SyncResult.Status.DELETE, result.getStatus()); + + assertNull(getUserManager(root).getAuthorizable(userId)); + } + + @Test + public void testSyncGroupById() throws Exception { + ExternalIdentity externalId = idp.listGroups().next(); + + // no initial sync -> sync-by-id doesn't succeed + SyncResult result = syncCtx.sync(externalId.getId()); + assertEquals(SyncResult.Status.NO_SUCH_AUTHORIZABLE, result.getStatus()); + + // force sync + syncCtx.sync(externalId); + + // try again + syncCtx.setForceGroupSync(true); + result = syncCtx.sync(externalId.getId()); + assertEquals(SyncResult.Status.UPDATE, result.getStatus()); + } + + @Test + public void testSyncRemovedGroupById() throws Exception { + // mark a regular repo user as external user from the test IDP + Group gr = createTestGroup(); + String groupId = gr.getID(); + + setExternalID(gr, idp.getName()); + + // test sync with 'keepmissing' = true + syncCtx.setKeepMissing(true); + SyncResult result = syncCtx.sync(groupId); + assertEquals(SyncResult.Status.MISSING, result.getStatus()); + assertNotNull(getUserManager(root).getAuthorizable(groupId)); + + // test sync with 'keepmissing' = false + syncCtx.setKeepMissing(false); + result = syncCtx.sync(groupId); + assertEquals(SyncResult.Status.DELETE, result.getStatus()); + + assertNull(getUserManager(root).getAuthorizable(groupId)); + } + + @Test + public void testSyncRemovedGroupWithMembers() throws Exception { + // mark a regular repo user as external user from the test IDP + Group gr = createTestGroup(); + gr.addMember(getTestUser()); + + String groupId = gr.getID(); + setExternalID(gr, idp.getName()); + + // test sync with 'keepmissing' = true + syncCtx.setKeepMissing(true); + SyncResult result = syncCtx.sync(groupId); + assertEquals(SyncResult.Status.NOP, result.getStatus()); + assertNotNull(getUserManager(root).getAuthorizable(groupId)); + + // test sync with 'keepmissing' = false + syncCtx.setKeepMissing(false); + result = syncCtx.sync(groupId); + assertEquals(SyncResult.Status.NOP, result.getStatus()); + + assertNotNull(getUserManager(root).getAuthorizable(groupId)); + } + + @Test + public void testSyncByForeignId() throws Exception { + SyncResult result = syncCtx.sync(getTestUser().getID()); + assertEquals(SyncResult.Status.FOREIGN, result.getStatus()); + } + + @Test + public void testSyncByForeignId2() throws Exception { + User u = getTestUser(); + setExternalID(u, "differentIDP"); + + SyncResult result = syncCtx.sync(u.getID()); + assertEquals(SyncResult.Status.FOREIGN, result.getStatus()); + } + + @Test + public void testSyncAutoMembership() throws Exception { + Group gr = createTestGroup(); + + config.user().setAutoMembership(gr.getID()); + + SyncResult result = syncCtx.sync(idp.listUsers().next()); + assertEquals(SyncResult.Status.ADD, result.getStatus()); + + Authorizable a = getUserManager(root).getAuthorizable(result.getIdentity().getId()); + assertTrue(gr.isDeclaredMember(a)); + } + + @Test + public void testSyncAutoMembershipListsNonExistingGroup() throws Exception { + config.user().setAutoMembership("nonExistingGroup"); + + SyncResult result = syncCtx.sync(idp.listUsers().next()); + assertEquals(SyncResult.Status.ADD, result.getStatus()); + } + + @Test + public void testSyncAutoMembershipListsUser() throws Exception { + // set auto-membership config to point to a user instead a group + config.user().setAutoMembership(getTestUser().getID()); + syncCtx.sync(idp.listUsers().next()); + } + + @Test + public void testLostMembership() throws Exception { + // create a group in the repository which is marked as being external + // and associated with the test-IDP to setup the situation that a + // repository group is no longer listed in the IDP. + Group gr = createTestGroup(); + setExternalID(gr, idp.getName()); + + // sync an external user from the IDP into the repo and make it member + // of the test group + SyncResult result = syncCtx.sync(idp.listUsers().next()); + User user = getUserManager(root).getAuthorizable(result.getIdentity().getId(), User.class); + gr.addMember(user); + root.commit(); + + // enforce synchronization of the user and it's group membership + syncCtx.setForceUserSync(true); + config.user().setMembershipExpirationTime(-1); + + // 1. membership nesting is < 0 => membership not synchronized + config.user().setMembershipNestingDepth(-1); + syncCtx.sync(user.getID()).getStatus(); + assertTrue(gr.isDeclaredMember(user)); + + // 2. membership nesting is > 0 => membership gets synchronized + config.user().setMembershipNestingDepth(1); + assertEquals(SyncResult.Status.UPDATE, syncCtx.sync(user.getID()).getStatus()); + + assertFalse(gr.isDeclaredMember(user)); + } + + @Test + public void testLostMembershipDifferentIDP() throws Exception { + // create a group in the repository which is marked as being external + // and associated with another IPD. + Group gr = createTestGroup(); + setExternalID(gr, "differentIDP"); + + // sync an external user from the IDP into the repo and make it member + // of the test group + SyncResult result = syncCtx.sync(idp.listUsers().next()); + User user = getUserManager(root).getAuthorizable(result.getIdentity().getId(), User.class); + gr.addMember(user); + root.commit(); + + // enforce synchronization of the user and it's group membership + syncCtx.setForceUserSync(true); + config.user().setMembershipExpirationTime(-1); + config.user().setMembershipNestingDepth(1); + + assertEquals(SyncResult.Status.UPDATE, syncCtx.sync(user.getID()).getStatus()); + + // since the group is not associated with the test-IDP the group-membership + // must NOT be modified during the sync. + assertTrue(gr.isDeclaredMember(user)); + } + + /** + * ExternalIdentity implementation that is neither user nor group. + */ + private final class TestExternalIdentity implements ExternalIdentity { + + @Nonnull + @Override + public ExternalIdentityRef getExternalId() { + return new ExternalIdentityRef(getId(), idp.getName()); + } + + @Nonnull + @Override + public String getId() { + return "externalId"; + } + + @Nonnull + @Override + public String getPrincipalName() { + return "principalName"; + } + + @CheckForNull + @Override + public String getIntermediatePath() { + return null; + } + + @Nonnull + @Override + public Iterable getDeclaredGroups() { + return ImmutableSet.of(); + } + + @Nonnull + @Override + public Map getProperties() { + return ImmutableMap.of(); + } + } +} \ No newline at end of file