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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
