You are viewing a plain text version of this content. The canonical link for it is here.
Posted to oak-dev@jackrabbit.apache.org by "Shivam-Agg (via GitHub)" <gi...@apache.org> on 2026/06/04 04:35:57 UTC
[PR] Lazy Initialization of ExternalIdentityMonitor [jackrabbit-oak]
Shivam-Agg opened a new pull request, #2928:
URL: https://github.com/apache/jackrabbit-oak/pull/2928
(no comment)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Lazy Initialisation of ExternalIdentityMonitor [jackrabbit-oak]
Posted by "rishabhdaim (via GitHub)" <gi...@apache.org>.
rishabhdaim commented on code in PR #2928:
URL: https://github.com/apache/jackrabbit-oak/pull/2928#discussion_r3355689303
##########
oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/ExternalLoginModule.java:
##########
@@ -165,11 +167,16 @@ public void initialize(Subject subject, CallbackHandler callbackHandler, Map<Str
log.debug("No 'SupportedCredentials' configured. Using default implementation supporting 'SimpleCredentials'.");
}
- monitor = WhiteboardUtils.getService(whiteboard, ExternalIdentityMonitor.class);
- if (monitor == null) {
- log.debug("No ExternalIdentityMonitor registered.");
- monitor = ExternalIdentityMonitor.NOOP;
- }
+ monitorSupplier = () -> {
Review Comment:
oak-auth-external enforces 100% branch coverage (`mvn verify -pl oak-auth-external -Pcoverage` passes on trunk but fails on this PR at 0.9). The supplier's cached `monitor != null` branch is never exercised because each call site invokes `get()` only once—please add a test that hits the supplier twice in one login.
##########
oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/ExternalLoginModuleTest.java:
##########
@@ -803,4 +803,59 @@ public void testGetExternalIdentityFails() throws Exception {
}
}
-}
\ No newline at end of file
+ @Test
+ public void testMonitorNotResolvedWhenLoginFailsBeforeSync() throws Exception {
+ ExternalIdentityProvider idp = mock(ExternalIdentityProvider.class, withSettings().extraInterfaces(CredentialsSupport.class));
+ when(idp.getName()).thenReturn(DEFAULT_IDP_NAME);
+ when(((CredentialsSupport) idp).getUserId(any(Credentials.class))).thenReturn(null);
+ when(((CredentialsSupport) idp).getCredentialClasses()).thenReturn(Set.of(SimpleCredentials.class));
+ when(extIPMgr.getProvider(DEFAULT_IDP_NAME)).thenReturn(idp);
+ when(syncManager.getSyncHandler("syncHandler")).thenReturn(new DefaultSyncHandler(new DefaultSyncConfigImpl().setName("syncHandler")));
+
+ wb.register(ExternalIdentityProviderManager.class, extIPMgr, Collections.emptyMap());
+ wb.register(SyncManager.class, syncManager, Collections.emptyMap());
+
+ CallbackHandler cbh = createCallbackHandler(wb, getContentRepository(), getSecurityProvider(), new SimpleCredentials("testUser", new char[0]));
+ loginModule.initialize(new Subject(), cbh, new HashMap<>(), Map.of(PARAM_IDP_NAME, DEFAULT_IDP_NAME, PARAM_SYNC_HANDLER_NAME, "syncHandler"));
+ assertFalse(loginModule.login());
+
+ verify(wb, never()).track(ExternalIdentityMonitor.class);
+ verifyNoInteractions(externalIdentityMonitor);
+ }
+
+ @Test
+ public void testMonitorResolvedExactlyOnceOnSuccessfulSync() throws Exception {
Review Comment:
This largely duplicates `testSyncUser` (same IDP/sync setup) while only asserting `track` once. Consider folding `verify(wb, times(1)).track(ExternalIdentityMonitor.class)` into `testSyncUser` and removing this test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Lazy Initialisation of ExternalIdentityMonitor [jackrabbit-oak]
Posted by "rishabhdaim (via GitHub)" <gi...@apache.org>.
rishabhdaim commented on code in PR #2928:
URL: https://github.com/apache/jackrabbit-oak/pull/2928#discussion_r3355738346
##########
oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/ExternalLoginModule.java:
##########
@@ -454,7 +461,7 @@ private void validateUser(@NotNull String id, @NotNull UserManager userMgr) thro
root.commit();
timer.mark("commit");
log.debug("validateUser({}) {}", id, timer);
- monitor.doneSyncId(watch.elapsed(NANOSECONDS), syncResult);
+ monitorSupplier.get().doneSyncId(watch.elapsed(NANOSECONDS), syncResult);
Review Comment:
`validateUser` now resolves the monitor lazily via `monitorSupplier.get().doneSyncId(...)`. Consider asserting `never().track` right after `initialize()` and `times(1)` after `login()` in `testValidateUser` so this path documents the same lazy contract as the new sync tests.
##########
oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/ExternalLoginModuleTest.java:
##########
@@ -803,4 +803,59 @@ public void testGetExternalIdentityFails() throws Exception {
}
}
-}
\ No newline at end of file
+ @Test
+ public void testMonitorNotResolvedWhenLoginFailsBeforeSync() throws Exception {
+ ExternalIdentityProvider idp = mock(ExternalIdentityProvider.class, withSettings().extraInterfaces(CredentialsSupport.class));
+ when(idp.getName()).thenReturn(DEFAULT_IDP_NAME);
+ when(((CredentialsSupport) idp).getUserId(any(Credentials.class))).thenReturn(null);
+ when(((CredentialsSupport) idp).getCredentialClasses()).thenReturn(Set.of(SimpleCredentials.class));
+ when(extIPMgr.getProvider(DEFAULT_IDP_NAME)).thenReturn(idp);
+ when(syncManager.getSyncHandler("syncHandler")).thenReturn(new DefaultSyncHandler(new DefaultSyncConfigImpl().setName("syncHandler")));
+
+ wb.register(ExternalIdentityProviderManager.class, extIPMgr, Collections.emptyMap());
+ wb.register(SyncManager.class, syncManager, Collections.emptyMap());
+
+ CallbackHandler cbh = createCallbackHandler(wb, getContentRepository(), getSecurityProvider(), new SimpleCredentials("testUser", new char[0]));
+ loginModule.initialize(new Subject(), cbh, new HashMap<>(), Map.of(PARAM_IDP_NAME, DEFAULT_IDP_NAME, PARAM_SYNC_HANDLER_NAME, "syncHandler"));
+ assertFalse(loginModule.login());
+
+ verify(wb, never()).track(ExternalIdentityMonitor.class);
+ verifyNoInteractions(externalIdentityMonitor);
+ }
+
+ @Test
+ public void testMonitorResolvedExactlyOnceOnSuccessfulSync() throws Exception {
+ when(extIPMgr.getProvider(DEFAULT_IDP_NAME)).thenReturn(new TestIdentityProvider());
+ when(syncManager.getSyncHandler("syncHandler")).thenReturn(new DefaultSyncHandler(new DefaultSyncConfigImpl().setName("syncHandler")));
+
+ wb.register(ExternalIdentityProviderManager.class, extIPMgr, Collections.emptyMap());
+ wb.register(SyncManager.class, syncManager, Collections.emptyMap());
+
+ Credentials crds = new SimpleCredentials(ID_TEST_USER, new char[0]);
+ CallbackHandler cbh = createCallbackHandler(wb, getContentRepository(), getSecurityProvider(), crds);
+ loginModule.initialize(new Subject(), cbh, new HashMap<>(), Map.of(PARAM_IDP_NAME, DEFAULT_IDP_NAME, PARAM_SYNC_HANDLER_NAME, "syncHandler"));
+ assertTrue(loginModule.login());
+
+ verify(wb, times(1)).track(ExternalIdentityMonitor.class);
+ }
+
+ @Test
+ public void testNoopMonitorUsedWhenMonitorNotRegistered() throws Exception {
Review Comment:
Login succeeds and `track` runs once, but the test does not show `ExternalIdentityMonitor.NOOP` was selected. Consider stronger assertions (sync side effects) or a failure-path variant without a registered monitor for `syncFailed`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Lazy Initialisation of ExternalIdentityMonitor [jackrabbit-oak]
Posted by "Shivam-Agg (via GitHub)" <gi...@apache.org>.
Shivam-Agg closed pull request #2928: Lazy Initialisation of ExternalIdentityMonitor
URL: https://github.com/apache/jackrabbit-oak/pull/2928
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org