Skip to content

Lazy Initialisation of ExternalIdentityMonitor #2928

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
Shivam-Agg wants to merge 3 commits into
base: trunk
Choose a base branch
from
Draft

Lazy Initialisation of ExternalIdentityMonitor #2928

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
79d94cd
resolve externalIdentityMonitor lazily
Shivam-Agg Jun 4, 2026
1f428fe
Merge pull request #1 from Shivam-Agg/OAK-10578
Shivam-Agg Jun 4, 2026
b2f9ca8
Add tests for ExternalLoginModule behavior (#2)
Shivam-Agg Jun 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 16 additions & 9 deletions .../apache/jackrabbit/oak/spi/security/authentication/external/impl/ExternalLoginModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import java.util.function.Supplier;
import java.util.stream.Collectors;
import java.util.stream.Stream;

Expand Down Expand Up @@ -123,7 +124,8 @@ public class ExternalLoginModule extends AbstractLoginModule {
private Set<? extends Principal> principals;
private AuthInfo authInfo;

private ExternalIdentityMonitor monitor;
private ExternalIdentityMonitor monitor; // do not access directly, use monitorSupplier
private Supplier<ExternalIdentityMonitor> monitorSupplier;

/**
* Default constructor for the OSGIi LoginModuleFactory case and the default non-OSGi JAAS case.
Expand Down Expand Up @@ -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 = () -> {

@rishabhdaim rishabhdaim Jun 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (monitor == null) {
monitor = WhiteboardUtils.getService(whiteboard, ExternalIdentityMonitor.class);
if (monitor == null) {
log.debug("No ExternalIdentityMonitor registered.");
monitor = ExternalIdentityMonitor.NOOP;
}
}
return monitor;
};
}

private void initializeIdpManager(@NotNull String idpName, @NotNull Whiteboard whiteboard) {
Expand Down Expand Up @@ -294,7 +301,7 @@ public boolean login() throws LoginException {
} catch (SyncException e) {
log.error("SyncHandler {} throws sync exception for '{}'", syncHandler.getName(), logId, e);
onError();
monitor.syncFailed(e);
monitorSupplier.get().syncFailed(e);
throw createLoginException(e, "Error while syncing user.");
}
}
Expand Down Expand Up @@ -423,7 +430,7 @@ private void syncUser(@NotNull ExternalUser user, @Nullable UserManager userMgr)
timer.mark("commit");
}
log.debug("syncUser({}) {}, status: {}", user.getId(), timer, syncResult.getStatus());
monitor.doneSyncExternalIdentity(watch.elapsed(NANOSECONDS), syncResult, numAttempt-1);
monitorSupplier.get().doneSyncExternalIdentity(watch.elapsed(NANOSECONDS), syncResult, numAttempt-1);
success = true;
} catch (CommitFailedException e) {
log.warn("User synchronization failed during commit: {}. (attempt {}/{})", e, numAttempt, MAX_SYNC_ATTEMPTS);
Expand Down Expand Up @@ -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);

@rishabhdaim rishabhdaim Jun 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} catch (CommitFailedException e) {
throw new SyncException("User synchronization failed during commit.", e);
} finally {
Expand Down
57 changes: 56 additions & 1 deletion ...che/jackrabbit/oak/spi/security/authentication/external/impl/ExternalLoginModuleTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -803,4 +803,59 @@ public void testGetExternalIdentityFails() throws Exception {
}
}

}
@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 {

@rishabhdaim rishabhdaim Jun 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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 {

@rishabhdaim rishabhdaim Jun 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Whiteboard freshWb = spy(new DefaultWhiteboard());
when(extIPMgr.getProvider(DEFAULT_IDP_NAME)).thenReturn(new TestIdentityProvider());
when(syncManager.getSyncHandler("syncHandler")).thenReturn(new DefaultSyncHandler(new DefaultSyncConfigImpl().setName("syncHandler")));

freshWb.register(ExternalIdentityProviderManager.class, extIPMgr, Collections.emptyMap());
freshWb.register(SyncManager.class, syncManager, Collections.emptyMap());

Credentials crds = new SimpleCredentials(ID_TEST_USER, new char[0]);
CallbackHandler cbh = createCallbackHandler(freshWb, getContentRepository(), getSecurityProvider(), crds);

ExternalLoginModule lm = new ExternalLoginModule();
lm.initialize(new Subject(), cbh, new HashMap<>(), Map.of(PARAM_IDP_NAME, DEFAULT_IDP_NAME, PARAM_SYNC_HANDLER_NAME, "syncHandler"));
assertTrue(lm.login());

verify(freshWb, times(1)).track(ExternalIdentityMonitor.class);
}

}
Loading