This is an automated email from the ASF dual-hosted git repository. angela pushed a commit to branch OAK-10517 in repository https://gitbox.apache.org/repos/asf/jackrabbit-oak.git
commit 413ea5908c3a35dcb8e45c2b589dd290c9c7eee6 Author: angela <[email protected]> AuthorDate: Thu Oct 26 18:36:07 2023 +0200 OAK-10517 : Consistently clean membership when switch between default and dynamic sync --- .../external/basic/DefaultSyncContext.java | 9 ++ .../external/impl/DynamicSyncContext.java | 11 +- .../external/impl/ExternalIdentityConstants.java | 9 ++ .../external/impl/SwitchSyncModeTest.java | 160 +++++++++++++++++++++ 4 files changed, 188 insertions(+), 1 deletion(-) diff --git a/oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/basic/DefaultSyncContext.java b/oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/basic/DefaultSyncContext.java index bf4ba40c04..1786fe39f1 100644 --- a/oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/basic/DefaultSyncContext.java +++ b/oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/basic/DefaultSyncContext.java @@ -60,6 +60,8 @@ import org.slf4j.LoggerFactory; import static java.text.Normalizer.Form.NFKC; import static java.text.Normalizer.normalize; +import static org.apache.jackrabbit.oak.spi.security.authentication.external.impl.ExternalIdentityConstants.REP_EXTERNAL_PRINCIPAL_NAMES; +import static org.apache.jackrabbit.oak.spi.security.authentication.external.impl.ExternalIdentityConstants.REP_LAST_DYNAMIC_SYNC; /** * Internal implementation of the sync context @@ -588,6 +590,13 @@ public class DefaultSyncContext implements SyncContext { log.debug("- removing member '{}' for group '{}'", auth.getID(), grp.getID()); } timer.mark("removing"); + + // make sure properties added by 'dynamic sync' are cleared + if (!auth.isGroup()) { + auth.removeProperty(REP_EXTERNAL_PRINCIPAL_NAMES); + auth.removeProperty(REP_LAST_DYNAMIC_SYNC); + timer.mark("cleanup"); + } log.debug("syncMembership({}) {}", external.getId(), timer); } diff --git a/oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/DynamicSyncContext.java b/oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/DynamicSyncContext.java index fb331ff0b6..18a24ea334 100644 --- a/oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/DynamicSyncContext.java +++ b/oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/DynamicSyncContext.java @@ -156,6 +156,10 @@ public class DynamicSyncContext extends DefaultSyncContext { super.syncMembership(external, auth, depth); } else { try { + // determine if clean up of groups (i.e. getting rid of previously synchronized membership information) + // is required or not. due to OAK-10517 just checking 'groupsSyncedBefore' is not sufficient. + boolean cleanupGroups = groupsSyncedBefore || requiresCleanup(auth); + Iterable<ExternalIdentityRef> declaredGroupRefs = external.getDeclaredGroups(); // resolve group-refs respecting depth to avoid iterating twice Map<ExternalIdentityRef, SyncEntry> map = collectSyncEntries(declaredGroupRefs, depth); @@ -170,7 +174,7 @@ public class DynamicSyncContext extends DefaultSyncContext { } // clean up any other membership - if (groupsSyncedBefore) { + if (cleanupGroups) { clearGroupMembership(auth); } } catch (ExternalIdentityException e) { @@ -201,6 +205,7 @@ public class DynamicSyncContext extends DefaultSyncContext { vs = createValues(principalsNames); } authorizable.setProperty(ExternalIdentityConstants.REP_EXTERNAL_PRINCIPAL_NAMES, vs); + authorizable.setProperty(ExternalIdentityConstants.REP_LAST_DYNAMIC_SYNC, nowValue); } @NotNull @@ -378,6 +383,10 @@ public class DynamicSyncContext extends DefaultSyncContext { private static boolean groupsSyncedBefore(@NotNull Authorizable authorizable) throws RepositoryException { return authorizable.hasProperty(REP_LAST_SYNCED) && !authorizable.hasProperty(ExternalIdentityConstants.REP_EXTERNAL_PRINCIPAL_NAMES); } + + private static boolean requiresCleanup(@NotNull Authorizable authorizable) throws RepositoryException { + return authorizable.hasProperty(REP_LAST_SYNCED) && !authorizable.hasProperty(ExternalIdentityConstants.REP_LAST_DYNAMIC_SYNC); + } private static boolean isEveryone(@NotNull Group group) { try { diff --git a/oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/ExternalIdentityConstants.java b/oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/ExternalIdentityConstants.java index d4fcf4007f..b55e73293a 100644 --- a/oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/ExternalIdentityConstants.java +++ b/oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/ExternalIdentityConstants.java @@ -62,6 +62,15 @@ public interface ExternalIdentityConstants { */ String REP_EXTERNAL_PRINCIPAL_NAMES = "rep:externalPrincipalNames"; + /** + * Name of the property storing the date of the last synchronization of the dynamic membership of an + * external user together with {@link #REP_EXTERNAL_PRINCIPAL_NAMES}. + * This property is of type {@link org.apache.jackrabbit.oak.api.Type#DATE}. + * + * @see <a href="https://issues.apache.org/jira/browse/OAK-10517";>OAK-10517</a> + */ + String REP_LAST_DYNAMIC_SYNC = "rep:lastDynamicSync"; + /** * The set of served property names defined by this interface. */ diff --git a/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/SwitchSyncModeTest.java b/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/SwitchSyncModeTest.java new file mode 100644 index 0000000000..4ad3c123e5 --- /dev/null +++ b/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/SwitchSyncModeTest.java @@ -0,0 +1,160 @@ +/* + * 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.impl; + +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.api.Root; +import org.apache.jackrabbit.oak.api.Tree; +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.basic.DefaultSyncConfig; +import org.apache.jackrabbit.oak.spi.security.authentication.external.basic.DefaultSyncContext; +import org.jetbrains.annotations.NotNull; +import org.junit.Test; + +import javax.jcr.Value; +import javax.jcr.ValueFactory; + +import java.util.Calendar; + +import static org.apache.jackrabbit.oak.spi.security.authentication.external.impl.ExternalIdentityConstants.REP_EXTERNAL_PRINCIPAL_NAMES; +import static org.apache.jackrabbit.oak.spi.security.authentication.external.impl.ExternalIdentityConstants.REP_LAST_DYNAMIC_SYNC; +import static org.apache.jackrabbit.oak.spi.security.authentication.external.impl.ExternalIdentityConstants.REP_LAST_SYNCED; +import static org.apache.jackrabbit.oak.spi.security.user.UserConstants.REP_MEMBERS; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertTrue; + +public class SwitchSyncModeTest extends AbstractDynamicTest { + + @NotNull ExternalUser syncPriorToDynamicMembership() throws Exception { + return syncWithDefaultContext(SyncResult.Status.ADD); + } + + private @NotNull ExternalUser syncWithDefaultContext(@NotNull SyncResult.Status expectedStatus) throws Exception { + DefaultSyncContext defaultSyncContext = createDefaultSyncContext(r); + ExternalUser previouslySyncedUser = idp.getUser(USER_ID); + assertNotNull(previouslySyncedUser); + + SyncResult result = defaultSyncContext.sync(previouslySyncedUser); + assertSame(expectedStatus, result.getStatus()); + defaultSyncContext.close(); + r.commit(); + + return previouslySyncedUser; + } + + @Override + protected @NotNull DefaultSyncConfig createSyncConfig() { + DefaultSyncConfig config = super.createSyncConfig(); + config.user().setDynamicMembership(true) + .setEnforceDynamicMembership(true) + .setMembershipExpirationTime(1) + .setExpirationTime(1); + config.group().setDynamicGroups(true) + .setExpirationTime(1); + return config; + } + + private @NotNull DefaultSyncContext createDefaultSyncContext(@NotNull Root r) { + UserManager um = getUserManager(r); + return new DefaultSyncContext(createNonDynamicConfig(), idp, um, getValueFactory(r)); + } + + private @NotNull DefaultSyncConfig createNonDynamicConfig() { + DefaultSyncConfig config = createSyncConfig(); + config.user().setDynamicMembership(false) + .setEnforceDynamicMembership(false) + .setMembershipNestingDepth(Long.MAX_VALUE) + .setMembershipExpirationTime(1); + config.group().setDynamicGroups(false); + return config; + } + + private static void assertIsDynamic(@NotNull ExternalUser externalUser, @NotNull UserManager um, @NotNull Root root, boolean expectedDynamic) throws Exception { + User user = um.getAuthorizable(externalUser.getId(), User.class); + assertNotNull(user); + assertEquals(expectedDynamic, user.hasProperty(REP_EXTERNAL_PRINCIPAL_NAMES)); + assertEquals(expectedDynamic, user.hasProperty(REP_LAST_DYNAMIC_SYNC)); + assertTrue(user.hasProperty(REP_LAST_SYNCED)); + + assertGroups(externalUser, um, root, !expectedDynamic); + } + + private static void assertGroups(@NotNull ExternalUser externalUser, @NotNull UserManager um, @NotNull Root r, boolean expectedMemberReference) throws Exception { + for (ExternalIdentityRef groupRef : externalUser.getDeclaredGroups()) { + Group gr = um.getAuthorizable(groupRef.getId(), Group.class); + assertNotNull(gr); + + Tree t = r.getTree(gr.getPath()); + assertEquals(expectedMemberReference, t.hasProperty(REP_MEMBERS)); + } + } + + @Test + public void testDefaultDynamic() throws Exception { + ExternalUser externalUser = idp.getUser(USER_ID); + assertNotNull(externalUser); + + // assert previously synched user is not dynamic + assertIsDynamic(externalUser, getUserManager(r), r, false); + + // assert previously synched user is now dynamic + sync(externalUser, SyncResult.Status.UPDATE); + assertIsDynamic(externalUser, getUserManager(r), r, true); + } + + @Test + public void testDefaultMissingCleanupDynamic() throws Exception { + ExternalUser externalUser = idp.getUser(USER_ID); + assertNotNull(externalUser); + + // modified the default-synchronized user to mock the issue reported in OAK-10517 + UserManager um = getUserManager(r); + User user = um.getAuthorizable(externalUser.getId(), User.class); + user.setProperty(REP_EXTERNAL_PRINCIPAL_NAMES, new Value[] {getValueFactory(r).createValue("test-values")}); + r.commit(); + + // assert previously synched user is now dynamic and groups have been cleaned + sync(externalUser, SyncResult.Status.UPDATE); + assertIsDynamic(externalUser, getUserManager(r), r, true); + } + + @Test + public void testDefaultSyncClearsDynamicProperties() throws Exception { + ExternalUser externalUser = idp.getUser(USER_ID); + assertNotNull(externalUser); + + assertIsDynamic(externalUser, getUserManager(r), r, false); + + // modified the default-synchronized user to mock the issue reported in OAK-10517 + UserManager um = getUserManager(r); + User user = um.getAuthorizable(externalUser.getId(), User.class); + ValueFactory vf = getValueFactory(r); + user.setProperty(REP_EXTERNAL_PRINCIPAL_NAMES, new Value[] {vf.createValue("test-values")}); + user.setProperty(REP_LAST_DYNAMIC_SYNC, vf.createValue(Calendar.getInstance())); + r.commit(); + + // assert previously synched user is now dynamic and groups have been cleaned + syncWithDefaultContext(SyncResult.Status.UPDATE); + assertIsDynamic(externalUser, getUserManager(r), r, false); + } +} \ No newline at end of file
