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