This is an automated email from the ASF dual-hosted git repository.
angela pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/jackrabbit-oak.git
The following commit(s) were added to refs/heads/trunk by this push:
new 84bf4f2d78 OAK-10517 : Consistently clean membership when switch
between default and dynamic sync
84bf4f2d78 is described below
commit 84bf4f2d78344e41cd0e5455c5aed7eeaec68af4
Author: anchela <[email protected]>
AuthorDate: Thu Nov 2 11:08:54 2023 +0100
OAK-10517 : Consistently clean membership when switch between default and
dynamic sync
---
.../external/basic/DefaultSyncContext.java | 9 ++
.../external/impl/DynamicSyncContext.java | 19 ++-
.../external/impl/ExternalIdentityConstants.java | 9 ++
.../external/impl/DynamicSyncContextTest.java | 5 +-
.../external/impl/SwitchSyncModeTest.java | 161 +++++++++++++++++++++
5 files changed, 199 insertions(+), 4 deletions(-)
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..92236533fa 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
@@ -91,7 +91,7 @@ public class DynamicSyncContext extends DefaultSyncContext {
}
Collection<String> principalNames = clearGroupMembership(authorizable);
-
authorizable.setProperty(ExternalIdentityConstants.REP_EXTERNAL_PRINCIPAL_NAMES,
createValues(principalNames));
+ setExternalPrincipalNames(authorizable, createValues(principalNames));
return true;
}
@@ -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) {
@@ -200,7 +204,12 @@ public class DynamicSyncContext extends DefaultSyncContext
{
Set<String> principalsNames = syncEntries.stream().map(syncEntry
-> syncEntry.principalName).collect(Collectors.toSet());
vs = createValues(principalsNames);
}
-
authorizable.setProperty(ExternalIdentityConstants.REP_EXTERNAL_PRINCIPAL_NAMES,
vs);
+ setExternalPrincipalNames(authorizable, vs);
+ }
+
+ private void setExternalPrincipalNames(@NotNull Authorizable authorizable,
@NotNull Value[] principalNames) throws RepositoryException {
+
authorizable.setProperty(ExternalIdentityConstants.REP_EXTERNAL_PRINCIPAL_NAMES,
principalNames);
+
authorizable.setProperty(ExternalIdentityConstants.REP_LAST_DYNAMIC_SYNC,
nowValue);
}
@NotNull
@@ -378,6 +387,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/DynamicSyncContextTest.java
b/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/DynamicSyncContextTest.java
index 0e0a7a09ed..07029a3a20 100644
---
a/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/DynamicSyncContextTest.java
+++
b/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/DynamicSyncContextTest.java
@@ -60,6 +60,7 @@ import static
org.apache.jackrabbit.oak.spi.security.authentication.external.Tes
import static
org.apache.jackrabbit.oak.spi.security.authentication.external.TestIdentityProvider.ID_TEST_USER;
import static
org.apache.jackrabbit.oak.spi.security.authentication.external.impl.ExternalIdentityConstants.REP_EXTERNAL_ID;
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.user.UserConstants.REP_MEMBERS;
import static
org.apache.jackrabbit.oak.spi.security.user.UserConstants.REP_MEMBERS_LIST;
import static org.junit.Assert.assertEquals;
@@ -759,10 +760,12 @@ public class DynamicSyncContextTest extends
AbstractDynamicTest {
User user = userManager.getAuthorizable(PREVIOUS_SYNCED_ID,
User.class);
assertNotNull(user);
assertFalse(user.hasProperty(REP_EXTERNAL_PRINCIPAL_NAMES));
+ assertFalse(user.hasProperty(REP_LAST_DYNAMIC_SYNC));
assertTrue(syncContext.convertToDynamicMembership(user));
assertTrue(user.hasProperty(REP_EXTERNAL_PRINCIPAL_NAMES));
-
+ assertTrue(user.hasProperty(REP_LAST_DYNAMIC_SYNC));
+
assertDeclaredGroups(previouslySyncedUser);
}
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..596894485d
--- /dev/null
+++
b/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/SwitchSyncModeTest.java
@@ -0,0 +1,161 @@
+/*
+ * 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();
+
+ // verify that synchronizing with the default-context again clears all
properties that would have been set
+ // if dynamic-sync had been enabled temporarily in between.
+ syncWithDefaultContext(SyncResult.Status.UPDATE);
+ assertIsDynamic(externalUser, getUserManager(r), r, false);
+ }
+}
\ No newline at end of file
