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 an...@apache.org on 2023/10/26 16:36:29 UTC
[jackrabbit-oak] 01/01: OAK-10517 : Consistently clean membership when switch between default and dynamic sync
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 <an...@adobe.com>
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