From 55dddd1d531ac8787f2938b66775b57484ab50ed Mon Sep 17 00:00:00 2001 From: John Flavin Date: Mon, 8 Jun 2026 12:18:21 -0500 Subject: [PATCH 01/12] Add unit tests for PKCE provider, user details, and login extension Adds 18 isolated unit tests (Mockito only, no live OIDC/DB/Spring context): - PkceAuthorizationCodeAccessTokenProviderTest (7): authorization-redirect parameter building, S256 code_challenge = BASE64URL(SHA-256(verifier)), PKCE-disabled path, CSRF rejection on missing state, code_verifier replay at the token endpoint, and configurable state-key length. - OpenIdConnectUserDetailsTest (6): username-pattern resolution (default, custom multi-claim, missing-claim error) and email/name claim mapping. - OpenIdLoginExtensionTest (5): one-shot error-message display/clear and the blank/absent/no-data/exception short-circuits. Raises coverage from 46%/26% to 57%/37% (instruction/branch); the three targeted classes reach 95%/94%/100% instruction coverage. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../openid/OpenIdConnectUserDetailsTest.java | 119 +++++++++++ ...horizationCodeAccessTokenProviderTest.java | 189 ++++++++++++++++++ .../Login/OpenIdLoginExtensionTest.java | 95 +++++++++ 3 files changed, 403 insertions(+) create mode 100644 src/test/java/au/edu/qcif/xnat/auth/openid/OpenIdConnectUserDetailsTest.java create mode 100644 src/test/java/au/edu/qcif/xnat/auth/openid/pkce/PkceAuthorizationCodeAccessTokenProviderTest.java create mode 100644 src/test/java/org/nrg/xnat/extensions/screens/Login/OpenIdLoginExtensionTest.java diff --git a/src/test/java/au/edu/qcif/xnat/auth/openid/OpenIdConnectUserDetailsTest.java b/src/test/java/au/edu/qcif/xnat/auth/openid/OpenIdConnectUserDetailsTest.java new file mode 100644 index 0000000..fefd9d5 --- /dev/null +++ b/src/test/java/au/edu/qcif/xnat/auth/openid/OpenIdConnectUserDetailsTest.java @@ -0,0 +1,119 @@ +package au.edu.qcif.xnat.auth.openid; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnitRunner; + +import java.util.HashMap; +import java.util.Map; + +import static au.edu.qcif.xnat.auth.openid.etc.OpenIdAuthConstant.*; +import static org.junit.Assert.*; +import static org.mockito.Mockito.lenient; +import static org.mockito.Mockito.when; + +/** + * Unit tests for {@link OpenIdConnectUserDetails}, focused on username-pattern resolution and the + * mapping of OIDC claims onto user fields. No live identity provider or XNAT context is needed: the + * only collaborator, {@link OpenIdAuthPlugin}, is mocked and the claim set is supplied directly as a map. + */ +@RunWith(MockitoJUnitRunner.class) +public class OpenIdConnectUserDetailsTest { + + private static final String PROVIDER = "keycloak"; + + @Mock + private OpenIdAuthPlugin plugin; + + private Map userInfo; + + @Before + public void setUp() { + userInfo = new HashMap<>(); + userInfo.put("sub", "12345"); + } + + /** Tells the mock plugin which claim key backs a given configurable property. */ + private void mapProperty(final String propName, final String value) { + lenient().when(plugin.getProperty(PROVIDER, propName)).thenReturn(value); + } + + private OpenIdConnectUserDetails build() { + return new OpenIdConnectUserDetails(PROVIDER, userInfo, null, plugin); + } + + @Test + public void defaultUsernamePatternCombinesProviderIdAndSub() { + // No usernamePattern configured -> falls back to "[providerId]_[sub]". + final OpenIdConnectUserDetails details = build(); + + assertEquals("keycloak_12345", details.getUsername()); + } + + @Test + public void customUsernamePatternResolvesMultipleClaims() { + mapProperty(USERNAME_PATTERN, "[preferred_username]@[domain]"); + userInfo.put("preferred_username", "jdoe"); + userInfo.put("domain", "example.org"); + + assertEquals("jdoe@example.org", build().getUsername()); + } + + @Test + public void missingClaimInUsernamePatternThrows() { + mapProperty(USERNAME_PATTERN, "[providerId]_[employeeId]"); + // "employeeId" is not present in userInfo. + + try { + build(); + fail("Expected IllegalArgumentException when a username-pattern claim is missing"); + } catch (IllegalArgumentException expected) { + assertTrue("Message should name the missing claim", + expected.getMessage().contains("employeeId")); + } + } + + @Test + public void mapsConfiguredEmailAndNameClaims() { + mapProperty(EMAIL, "email"); + mapProperty(GIVEN_NAME, "given_name"); + mapProperty(FAMILY_NAME, "family_name"); + userInfo.put("email", "jane.doe@example.org"); + userInfo.put("given_name", "Jane"); + userInfo.put("family_name", "Doe"); + + final OpenIdConnectUserDetails details = build(); + + assertEquals("jane.doe@example.org", details.getEmail()); + assertEquals("Jane", details.getFirstname()); + assertEquals("Doe", details.getLastname()); + } + + @Test + public void absentNameClaimsResolveToEmptyString() { + mapProperty(EMAIL, "email"); + mapProperty(GIVEN_NAME, "given_name"); + // family_name property is configured but the claim is absent from the response. + mapProperty(FAMILY_NAME, "family_name"); + userInfo.put("email", "jane.doe@example.org"); + userInfo.put("given_name", "Jane"); + + final OpenIdConnectUserDetails details = build(); + + assertEquals("", details.getLastname()); + } + + @Test + public void getFieldValueReadsDeclaredFieldThenFallsBackToClaims() { + final OpenIdConnectUserDetails details = build(); + + // Declared field on the class, resolved via reflection. + assertEquals("keycloak", details.getFieldValue("providerId")); + // Not a declared field -> falls back to the claims map. + assertEquals("12345", details.getFieldValue("sub")); + // Neither a field nor a claim -> null. + assertNull(details.getFieldValue("does_not_exist")); + } +} diff --git a/src/test/java/au/edu/qcif/xnat/auth/openid/pkce/PkceAuthorizationCodeAccessTokenProviderTest.java b/src/test/java/au/edu/qcif/xnat/auth/openid/pkce/PkceAuthorizationCodeAccessTokenProviderTest.java new file mode 100644 index 0000000..375559a --- /dev/null +++ b/src/test/java/au/edu/qcif/xnat/auth/openid/pkce/PkceAuthorizationCodeAccessTokenProviderTest.java @@ -0,0 +1,189 @@ +package au.edu.qcif.xnat.auth.openid.pkce; + +import org.junit.Test; +import org.springframework.security.oauth2.client.resource.UserRedirectRequiredException; +import org.springframework.security.oauth2.client.token.AccessTokenRequest; +import org.springframework.security.oauth2.client.token.DefaultAccessTokenRequest; +import org.springframework.security.oauth2.client.token.grant.code.AuthorizationCodeResourceDetails; +import org.springframework.security.oauth2.common.exceptions.InvalidRequestException; +import org.springframework.util.MultiValueMap; + +import java.lang.reflect.Method; +import java.nio.charset.StandardCharsets; +import java.security.MessageDigest; +import java.util.Arrays; +import java.util.Base64; +import java.util.Map; + +import static org.junit.Assert.*; + +/** + * Unit tests for {@link PkceAuthorizationCodeAccessTokenProvider}. + * + *

These exercise the PKCE-specific logic in isolation, with no live OAuth2 / OIDC provider: + * the authorization-redirect path and the token-request parameter building are both reachable + * without performing the HTTP token exchange. Lives in the same package as the provider so it can + * read the package-private {@code PreservedState} the provider stashes on the request.

+ */ +public class PkceAuthorizationCodeAccessTokenProviderTest { + + private static final String CLIENT_ID = "xnat-client"; + private static final String AUTHORIZE_URI = "https://idp.example.org/authorize"; + private static final String TOKEN_URI = "https://idp.example.org/token"; + private static final String REDIRECT_URI = "https://xnat.example.org/openid-login"; + + private static PkceAuthorizationCodeResourceDetails resource(final boolean pkceEnabled) { + final PkceAuthorizationCodeResourceDetails resource = new PkceAuthorizationCodeResourceDetails(); + resource.setClientId(CLIENT_ID); + resource.setUserAuthorizationUri(AUTHORIZE_URI); + resource.setAccessTokenUri(TOKEN_URI); + resource.setPreEstablishedRedirectUri(REDIRECT_URI); + resource.setScope(Arrays.asList("openid", "email")); + resource.setPkceEnabled(pkceEnabled); + return resource; + } + + private static PkceAuthorizationCodeAccessTokenProvider provider() { + return new PkceAuthorizationCodeAccessTokenProvider(32); + } + + /** + * Drives {@code obtainAccessToken} with no code and no state, which makes the provider build and + * throw the authorization redirect. Returns that exception; the passed-in request is mutated with + * the generated state key and preserved state. + */ + private static UserRedirectRequiredException triggerAuthorizationRedirect( + final PkceAuthorizationCodeAccessTokenProvider provider, + final PkceAuthorizationCodeResourceDetails resource, + final AccessTokenRequest request) { + try { + provider.obtainAccessToken(resource, request); + fail("Expected a UserRedirectRequiredException when no authorization code is present"); + return null; // unreachable + } catch (UserRedirectRequiredException expected) { + return expected; + } + } + + @SuppressWarnings("unchecked") + private static MultiValueMap invokeGetParametersForTokenRequest( + final PkceAuthorizationCodeAccessTokenProvider provider, + final PkceAuthorizationCodeResourceDetails resource, + final AccessTokenRequest request) throws Exception { + final Method method = PkceAuthorizationCodeAccessTokenProvider.class.getDeclaredMethod( + "getParametersForTokenRequest", AuthorizationCodeResourceDetails.class, AccessTokenRequest.class); + method.setAccessible(true); + return (MultiValueMap) method.invoke(provider, resource, request); + } + + @Test + public void redirectIncludesPkceChallengeAndStandardParameters() { + final DefaultAccessTokenRequest request = new DefaultAccessTokenRequest(); + + final UserRedirectRequiredException redirect = triggerAuthorizationRedirect(provider(), resource(true), request); + + assertEquals(AUTHORIZE_URI, redirect.getRedirectUri()); + final Map params = redirect.getRequestParams(); + assertEquals("code", params.get("response_type")); + assertEquals(CLIENT_ID, params.get("client_id")); + assertEquals("openid email", params.get("scope")); + assertEquals(REDIRECT_URI, params.get("redirect_uri")); + assertEquals("S256", params.get("code_challenge_method")); + assertNotNull("code_challenge must be present when PKCE is enabled", params.get("code_challenge")); + + // The state key is generated, returned on the exception, and stashed on the request for later CSRF checks. + assertNotNull(redirect.getStateKey()); + assertEquals(redirect.getStateKey(), request.getStateKey()); + + // The code verifier is preserved client-side so it can be replayed at the token endpoint. + final Object preserved = request.getPreservedState(); + assertTrue(preserved instanceof PkceAuthorizationCodeAccessTokenProvider.PreservedState); + assertNotNull(((PkceAuthorizationCodeAccessTokenProvider.PreservedState) preserved).getCodeVerifier()); + } + + /** + * The security-critical PKCE invariant: code_challenge == BASE64URL(SHA-256(code_verifier)). + */ + @Test + public void codeChallengeIsSha256OfCodeVerifier() throws Exception { + final DefaultAccessTokenRequest request = new DefaultAccessTokenRequest(); + + final UserRedirectRequiredException redirect = triggerAuthorizationRedirect(provider(), resource(true), request); + + final String challenge = redirect.getRequestParams().get("code_challenge"); + final String verifier = ((PkceAuthorizationCodeAccessTokenProvider.PreservedState) request.getPreservedState()) + .getCodeVerifier(); + + final byte[] digest = MessageDigest.getInstance("SHA-256").digest(verifier.getBytes(StandardCharsets.US_ASCII)); + final String expectedChallenge = Base64.getUrlEncoder().withoutPadding().encodeToString(digest); + + assertEquals(expectedChallenge, challenge); + } + + @Test + public void redirectOmitsPkceParametersWhenPkceDisabled() { + final DefaultAccessTokenRequest request = new DefaultAccessTokenRequest(); + + final UserRedirectRequiredException redirect = triggerAuthorizationRedirect(provider(), resource(false), request); + + final Map params = redirect.getRequestParams(); + assertNull("code_challenge must not be sent when PKCE is disabled", params.get("code_challenge")); + assertNull("code_challenge_method must not be sent when PKCE is disabled", params.get("code_challenge_method")); + assertNull(((PkceAuthorizationCodeAccessTokenProvider.PreservedState) request.getPreservedState()).getCodeVerifier()); + } + + @Test + public void stateKeyHonoursConfiguredLength() { + final PkceAuthorizationCodeAccessTokenProvider provider = new PkceAuthorizationCodeAccessTokenProvider(40); + final DefaultAccessTokenRequest request = new DefaultAccessTokenRequest(); + + final UserRedirectRequiredException redirect = triggerAuthorizationRedirect(provider, resource(true), request); + + assertEquals(40, redirect.getStateKey().length()); + } + + @Test + public void tokenRequestReplaysCodeVerifierWhenPkceEnabled() throws Exception { + final DefaultAccessTokenRequest request = new DefaultAccessTokenRequest(); + request.setAuthorizationCode("auth-code-123"); + request.setPreservedState(new PkceAuthorizationCodeAccessTokenProvider.PreservedState(REDIRECT_URI, "the-code-verifier")); + + final MultiValueMap form = invokeGetParametersForTokenRequest(provider(), resource(true), request); + + assertEquals("authorization_code", form.getFirst("grant_type")); + assertEquals("auth-code-123", form.getFirst("code")); + assertEquals("the-code-verifier", form.getFirst("code_verifier")); + assertEquals(REDIRECT_URI, form.getFirst("redirect_uri")); + } + + @Test + public void tokenRequestOmitsCodeVerifierWhenPkceDisabled() throws Exception { + final DefaultAccessTokenRequest request = new DefaultAccessTokenRequest(); + request.setAuthorizationCode("auth-code-123"); + request.setPreservedState(new PkceAuthorizationCodeAccessTokenProvider.PreservedState(REDIRECT_URI, "the-code-verifier")); + + final MultiValueMap form = invokeGetParametersForTokenRequest(provider(), resource(false), request); + + assertNull("code_verifier must not be sent when PKCE is disabled", form.getFirst("code_verifier")); + assertEquals("authorization_code", form.getFirst("grant_type")); + assertEquals("auth-code-123", form.getFirst("code")); + } + + /** + * State is mandatory by default, so a token request that arrives with an authorization code but no + * preserved state is treated as a possible CSRF attack and rejected before any HTTP call is made. + */ + @Test + public void tokenRequestRejectsMissingStateAsPossibleCsrf() { + final DefaultAccessTokenRequest request = new DefaultAccessTokenRequest(); + request.setAuthorizationCode("auth-code-123"); + // No preserved state set. + + try { + provider().obtainAccessToken(resource(true), request); + fail("Expected an InvalidRequestException for missing state (possible CSRF)"); + } catch (InvalidRequestException expected) { + assertTrue(expected.getMessage().toLowerCase().contains("csrf")); + } + } +} diff --git a/src/test/java/org/nrg/xnat/extensions/screens/Login/OpenIdLoginExtensionTest.java b/src/test/java/org/nrg/xnat/extensions/screens/Login/OpenIdLoginExtensionTest.java new file mode 100644 index 0000000..c6cffd1 --- /dev/null +++ b/src/test/java/org/nrg/xnat/extensions/screens/Login/OpenIdLoginExtensionTest.java @@ -0,0 +1,95 @@ +package org.nrg.xnat.extensions.screens.Login; + +import org.apache.turbine.util.RunData; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnitRunner; + +import javax.servlet.http.HttpSession; +import java.util.HashMap; +import java.util.Map; + +import static au.edu.qcif.xnat.auth.openid.OpenIdConnectFilter.OPENID_ERROR_MESSAGE; +import static org.mockito.Mockito.*; + +/** + * Unit tests for {@link OpenIdLoginExtension}. The extension only manipulates the Turbine + * {@link RunData} / {@link HttpSession}, both of which are mocked here, so no servlet container or + * XNAT runtime is required. + */ +@RunWith(MockitoJUnitRunner.class) +public class OpenIdLoginExtensionTest { + + @Mock + private RunData data; + + @Mock + private HttpSession session; + + private OpenIdLoginExtension extension; + + @Before + public void setUp() { + extension = new OpenIdLoginExtension(); + } + + private Map paramsWithData() { + final Map params = new HashMap<>(); + params.put("data", data); + return params; + } + + @Test + public void displaysAndClearsErrorMessageWhenPresent() { + when(data.getSession()).thenReturn(session); + when(session.getAttribute(OPENID_ERROR_MESSAGE)).thenReturn("Your email domain is not permitted"); + + extension.execute(paramsWithData()); + + verify(data).setMessage("Your email domain is not permitted"); + // The one-shot message is removed so it does not reappear on the next page load. + verify(session).removeAttribute(OPENID_ERROR_MESSAGE); + } + + @Test + public void doesNothingWhenNoErrorMessage() { + when(data.getSession()).thenReturn(session); + when(session.getAttribute(OPENID_ERROR_MESSAGE)).thenReturn(null); + + extension.execute(paramsWithData()); + + verify(data, never()).setMessage(anyString()); + verify(session, never()).removeAttribute(anyString()); + } + + @Test + public void doesNothingWhenErrorMessageIsBlank() { + when(data.getSession()).thenReturn(session); + when(session.getAttribute(OPENID_ERROR_MESSAGE)).thenReturn(" "); + + extension.execute(paramsWithData()); + + verify(data, never()).setMessage(anyString()); + verify(session, never()).removeAttribute(anyString()); + } + + @Test + public void returnsQuietlyWhenRunDataAbsent() { + // No "data" entry -> the extension must short-circuit without touching anything or throwing. + extension.execute(new HashMap<>()); + + verifyNoInteractions(data, session); + } + + @Test + public void swallowsExceptionsRaisedWhileReadingSession() { + when(data.getSession()).thenThrow(new IllegalStateException("session unavailable")); + + // Must not propagate; the login page should still render. + extension.execute(paramsWithData()); + + verify(data, never()).setMessage(anyString()); + } +} From 45e415b2de9366ed1e1480d67fa8ba2b96791843 Mon Sep 17 00:00:00 2001 From: John Flavin Date: Mon, 8 Jun 2026 12:27:48 -0500 Subject: [PATCH 02/12] Add unit tests for OpenIdConnectFilter decision logic Covers the filter's pure logic that previously had no direct tests, all without an OAuth2 exchange, database, or servlet container (filter built with mocked collaborators; private methods exercised via reflection): - Email-domain whitelisting (isAllowedEmailDomain / shouldFilterEmailDomains): filtering disabled, on/off whitelist, case-insensitive match, "*" wildcard, malformed address with no domain, unknown provider, and default-off config. - Login-page error mapping (getUserMessage): usernamePattern config error vs. generic retry message for other failures. - ID-token encryption detection (isIdTokenEncrypted): 5-part JWE vs 3-part JWS. Raises OpenIdConnectFilter coverage from 38%/24% to 56%/54% (instruction/branch) and overall coverage to 63%/46%. The static-XNAT paths (Users.createUser, TurbineUtils redirects) remain for integration testing. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../openid/OpenIdConnectFilterLogicTest.java | 197 ++++++++++++++++++ 1 file changed, 197 insertions(+) create mode 100644 src/test/java/au/edu/qcif/xnat/auth/openid/OpenIdConnectFilterLogicTest.java diff --git a/src/test/java/au/edu/qcif/xnat/auth/openid/OpenIdConnectFilterLogicTest.java b/src/test/java/au/edu/qcif/xnat/auth/openid/OpenIdConnectFilterLogicTest.java new file mode 100644 index 0000000..f973770 --- /dev/null +++ b/src/test/java/au/edu/qcif/xnat/auth/openid/OpenIdConnectFilterLogicTest.java @@ -0,0 +1,197 @@ +package au.edu.qcif.xnat.auth.openid; + +import au.edu.qcif.xnat.auth.openid.service.KeystoreService; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnitRunner; +import org.nrg.xdat.preferences.SiteConfigPreferences; +import org.nrg.xdat.services.XdatUserAuthService; +import org.springframework.security.authentication.AuthenticationEventPublisher; +import org.springframework.security.authentication.BadCredentialsException; +import org.springframework.security.authentication.CredentialsExpiredException; +import org.springframework.security.core.AuthenticationException; + +import java.lang.reflect.Method; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; + +import static org.junit.Assert.*; +import static org.mockito.Mockito.lenient; +import static org.mockito.Mockito.when; + +/** + * Unit tests for the pure decision logic in {@link OpenIdConnectFilter}: email-domain whitelisting, + * the login-page error-message mapping, and ID-token encryption detection. + * + *

These paths need no OAuth2 exchange, database, or servlet container. The filter is constructed + * directly with mocked collaborators (only {@link OpenIdAuthPlugin} drives these branches), and the + * package-private/private methods are exercised via reflection — the same approach used for the PKCE + * provider's parameter builder. The static-XNAT-dependent paths (user creation, redirects) are + * deliberately left to integration testing.

+ */ +@RunWith(MockitoJUnitRunner.class) +public class OpenIdConnectFilterLogicTest { + + private static final String PROVIDER = "keycloak"; + + @Mock private OpenIdAuthPlugin plugin; + @Mock private AuthenticationEventPublisher eventPublisher; + @Mock private XdatUserAuthService userAuthService; + @Mock private SiteConfigPreferences siteConfigPreferences; + @Mock private KeystoreService keystoreService; + + /** + * Builds a filter with {@code PROVIDER} enabled and the given provider properties stubbed. The + * filter's constructor reads these to build its allowed-domain map, so all stubbing must happen + * before construction. + */ + private OpenIdConnectFilter filterWith(final Map providerProps) { + lenient().when(plugin.getRedirectUri()).thenReturn("/openid/callback"); + lenient().when(plugin.getEnabledProviders()).thenReturn(Collections.singletonList(PROVIDER)); + providerProps.forEach((key, value) -> lenient().when(plugin.getProperty(PROVIDER, key)).thenReturn(value)); + return new OpenIdConnectFilter(plugin, eventPublisher, userAuthService, siteConfigPreferences, keystoreService); + } + + private static Map filteringOn(final String allowedEmailDomains) { + final Map props = new HashMap<>(); + props.put("shouldFilterEmailDomains", "true"); + props.put("allowedEmailDomains", allowedEmailDomains); + return props; + } + + private static boolean isAllowedEmailDomain(final OpenIdConnectFilter filter, final String email, final String providerId) throws Exception { + final Method method = OpenIdConnectFilter.class.getDeclaredMethod("isAllowedEmailDomain", String.class, String.class); + method.setAccessible(true); + return (boolean) method.invoke(filter, email, providerId); + } + + private static boolean shouldFilterEmailDomains(final OpenIdConnectFilter filter, final String providerId) throws Exception { + final Method method = OpenIdConnectFilter.class.getDeclaredMethod("shouldFilterEmailDomains", String.class); + method.setAccessible(true); + return (boolean) method.invoke(filter, providerId); + } + + private static boolean isIdTokenEncrypted(final OpenIdConnectFilter filter, final String idToken) throws Exception { + final Method method = OpenIdConnectFilter.class.getDeclaredMethod("isIdTokenEncrypted", String.class); + method.setAccessible(true); + return (boolean) method.invoke(filter, idToken); + } + + private static String getUserMessage(final AuthenticationException failed) throws Exception { + final Method method = OpenIdConnectFilter.class.getDeclaredMethod("getUserMessage", AuthenticationException.class); + method.setAccessible(true); + return (String) method.invoke(null, failed); + } + + // ---- email-domain whitelisting ------------------------------------------------------------- + + @Test + public void anyDomainAllowedWhenFilteringDisabled() throws Exception { + final OpenIdConnectFilter filter = filterWith(Collections.singletonMap("shouldFilterEmailDomains", "false")); + + assertTrue(isAllowedEmailDomain(filter, "anyone@whatever.example", PROVIDER)); + } + + @Test + public void domainOnWhitelistIsAllowed() throws Exception { + final OpenIdConnectFilter filter = filterWith(filteringOn("example.org, wustl.edu")); + + assertTrue(isAllowedEmailDomain(filter, "jane@wustl.edu", PROVIDER)); + } + + @Test + public void domainNotOnWhitelistIsRejected() throws Exception { + final OpenIdConnectFilter filter = filterWith(filteringOn("example.org, wustl.edu")); + + assertFalse(isAllowedEmailDomain(filter, "jane@gmail.com", PROVIDER)); + } + + @Test + public void whitelistMatchIsCaseInsensitive() throws Exception { + final OpenIdConnectFilter filter = filterWith(filteringOn("wustl.edu")); + + assertTrue(isAllowedEmailDomain(filter, "Jane@WUSTL.EDU", PROVIDER)); + } + + @Test + public void wildcardWhitelistAllowsAnyDomain() throws Exception { + final OpenIdConnectFilter filter = filterWith(filteringOn("*")); + + assertTrue(isAllowedEmailDomain(filter, "jane@anything.example", PROVIDER)); + } + + @Test + public void malformedEmailWithoutDomainIsRejected() throws Exception { + final OpenIdConnectFilter filter = filterWith(filteringOn("wustl.edu")); + + assertFalse("an address with no @ has no parseable domain", isAllowedEmailDomain(filter, "not-an-email", PROVIDER)); + } + + @Test + public void unknownProviderIsRejected() throws Exception { + // Only PROVIDER is enabled; a different provider id is absent from the allowed-domains map. + final OpenIdConnectFilter filter = filterWith(filteringOn("wustl.edu")); + + assertFalse(isAllowedEmailDomain(filter, "jane@wustl.edu", "some-other-provider")); + } + + @Test + public void shouldFilterEmailDomainsDefaultsToFalseWhenUnset() throws Exception { + // Property not stubbed -> null -> defaults to "false". + final OpenIdConnectFilter filter = filterWith(Collections.emptyMap()); + + assertFalse(shouldFilterEmailDomains(filter, PROVIDER)); + } + + @Test + public void shouldFilterEmailDomainsReflectsConfiguredValue() throws Exception { + final OpenIdConnectFilter filter = filterWith(filteringOn("wustl.edu")); + + assertTrue(shouldFilterEmailDomains(filter, PROVIDER)); + } + + // ---- login-page error messages ------------------------------------------------------------- + + @Test + public void usernamePatternFailureYieldsConfigurationErrorMessage() throws Exception { + final String message = getUserMessage(new BadCredentialsException( + "Cannot resolve username pattern: please check your usernamePattern configuration")); + + assertTrue(message.toLowerCase().contains("configuration error")); + } + + @Test + public void genericBadCredentialsYieldsRetryMessage() throws Exception { + final String message = getUserMessage(new BadCredentialsException("Could not obtain access token")); + + assertTrue(message.contains("Please try again")); + assertFalse("generic failures must not leak the config-error wording", message.toLowerCase().contains("configuration error")); + } + + @Test + public void nonBadCredentialsExceptionYieldsRetryMessage() throws Exception { + final String message = getUserMessage(new CredentialsExpiredException("Attempted login to locked account: jdoe")); + + assertTrue(message.contains("Please try again")); + } + + // ---- ID token encryption detection --------------------------------------------------------- + + @Test + public void fivePartTokenIsDetectedAsEncrypted() throws Exception { + final OpenIdConnectFilter filter = filterWith(Collections.emptyMap()); + + // A JWE compact serialization has five dot-separated parts. + assertTrue(isIdTokenEncrypted(filter, "header.encKey.iv.ciphertext.tag")); + } + + @Test + public void threePartTokenIsNotEncrypted() throws Exception { + final OpenIdConnectFilter filter = filterWith(Collections.emptyMap()); + + // A signed JWT (JWS) has three dot-separated parts. + assertFalse(isIdTokenEncrypted(filter, "header.payload.signature")); + } +} From 8a9c7fc70608ff13ea90612016fc87efcedd4b81 Mon Sep 17 00:00:00 2001 From: John Flavin Date: Mon, 8 Jun 2026 16:15:15 -0500 Subject: [PATCH 03/12] Add GitHub Actions CI workflow to run tests on JDK 21 Runs `./gradlew test jacocoTestReport` on a Temurin JDK 21 runner for pushes and PRs to main, with Gradle dependency caching. Uploads JUnit results and the JaCoCo HTML report as build artifacts. JDK 21 matches the build's sourceCompatibility/targetCompatibility (the Java 21 / XNAT 1.10.0 migration). The legacy .circleci/config.yml is pinned to JDK 8 and cannot compile the current codebase. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/workflows/ci.yml | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 .github/workflows/ci.yml diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 0000000..63e9a8c --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,38 @@ +name: CI + +on: + push: + branches: ["**"] + pull_request: + branches: [main] + +jobs: + test: + runs-on: ubuntu-latest + steps: + - name: Check out repository + uses: actions/checkout@v4 + + - name: Set up JDK 21 + uses: actions/setup-java@v4 + with: + distribution: temurin + java-version: "21" + cache: gradle + + - name: Run tests and coverage report + run: ./gradlew test jacocoTestReport --no-daemon + + - name: Upload test results + if: always() + uses: actions/upload-artifact@v4 + with: + name: test-results + path: build/test-results/test + + - name: Upload coverage report + if: always() + uses: actions/upload-artifact@v4 + with: + name: jacoco-coverage + path: build/reports/jacoco/test/html From a0c8b9f33ca9581194f542636a1d0b61778725ee Mon Sep 17 00:00:00 2001 From: John Flavin Date: Mon, 8 Jun 2026 16:20:22 -0500 Subject: [PATCH 04/12] Remove legacy CircleCI configuration The CircleCI pipeline was pinned to JDK 8 (circleci/openjdk:8-jdk-buster), which cannot compile the Java 21 codebase, and the circleci/* image namespace is deprecated. It is superseded by the GitHub Actions workflow added in the previous commit. Also removes the CircleCI-only Maven settings.xml (env-var placeholders, no secrets) that was used solely by the removed publish step. Co-Authored-By: Claude Opus 4.8 (1M context) --- .circleci/config.yml | 80 ------------------------------------------ .circleci/settings.xml | 12 ------- 2 files changed, 92 deletions(-) delete mode 100644 .circleci/config.yml delete mode 100644 .circleci/settings.xml diff --git a/.circleci/config.yml b/.circleci/config.yml deleted file mode 100644 index 1faeef6..0000000 --- a/.circleci/config.yml +++ /dev/null @@ -1,80 +0,0 @@ -build_config: &build_config - BUILD_IMAGE_NAME: circleci/openjdk - BUILD_IMAGE_TAG: 8-jdk-buster - CACHE_VERSION: v1 - _JAVA_OPTIONS: "-Xmx3g" - GRADLE_OPTS: "-Dorg.gradle.daemon=false -Dorg.gradle.workers.max=2" - -version: 2 -jobs: - build: - environment: - <<: *build_config - docker: - - image: ${BUILD_IMAGE_NAME}:${BUILD_IMAGE_TAG} - steps: - - checkout - - restore_cache: - key: ${CACHE_VERSION}-gradle-wrapper-{{ checksum "gradle/wrapper/gradle-wrapper.properties" }} - - restore_cache: - key: ${CACHE_VERSION}-gradle-cache-{{ checksum "build.gradle" }} - - run: - name: Install dependencies - command: ./gradlew build --exclude-task test - - save_cache: - paths: - - ~/.gradle/wrapper - key: ${CACHE_VERSION}-gradle-wrapper-{{ checksum "gradle/wrapper/gradle-wrapper.properties" }} - - save_cache: - paths: - - ~/.gradle/caches - key: ${CACHE_VERSION}-gradle-cache-{{ checksum "build.gradle" }} - - persist_to_workspace: - root: . - paths: - - build - - test: - environment: - <<: *build_config - docker: - - image: ${BUILD_IMAGE_NAME}:${BUILD_IMAGE_TAG} - steps: - - checkout - - attach_workspace: - at: . - - run: - name: Run tests - command: | - ./gradlew test - - run: - name: Generate code coverage report - command: - ./gradlew jacocoTestReport - - store_test_results: - path: build/test-results/test - - store_artifacts: - path: build/test-results/test - when: always - - store_artifacts: - path: build/reports/jacoco/test/html - when: always - - run: - name: Create XNAT plugin jar - command: | - ./gradlew xnatPluginJar --exclude-task test - - store_artifacts: - path: build/libs - - run: - name: Publish XNAT plugin jar - command: | - ./gradlew publish -PmavenSettings=./.circleci/settings.xml - -workflows: - version: 2 - workflow: - jobs: - - build - - test: - requires: - - build diff --git a/.circleci/settings.xml b/.circleci/settings.xml deleted file mode 100644 index a06c932..0000000 --- a/.circleci/settings.xml +++ /dev/null @@ -1,12 +0,0 @@ - - - - - XNAT_Artifactory - ${REPO_USER} - ${REPO_TOKEN} - - - From 861ac346b6e5fc0397c9fb7f692c32187bbf10ec Mon Sep 17 00:00:00 2001 From: John Flavin Date: Tue, 9 Jun 2026 16:42:13 -0500 Subject: [PATCH 05/12] Remove generated Maven settings before running tests in CI actions/setup-java writes a default ~/.m2/settings.xml whose server credentials the net.linguica.maven-settings plugin fails to decrypt at configuration time, breaking the test-only CI job. Remove the file before invoking Gradle. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/workflows/ci.yml | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 63e9a8c..de5fe68 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -20,6 +20,16 @@ jobs: java-version: "21" cache: gradle + # actions/setup-java writes a default ~/.m2/settings.xml with server + # credentials. The net.linguica.maven-settings plugin (applied in + # build.gradle) tries to decrypt those at configuration time and fails + # ("Unable to decrypt local Maven settings credentials") because the + # runner has no Maven master password. This job only runs tests and + # never publishes, so we remove the file. Drop this step once the build + # no longer applies the maven-settings plugin in test-only contexts. + - name: Remove generated Maven settings + run: rm -f ~/.m2/settings.xml + - name: Run tests and coverage report run: ./gradlew test jacocoTestReport --no-daemon From ef180067b1f495ec6ea3f1b79fabcf0df70a3dff Mon Sep 17 00:00:00 2001 From: John Flavin Date: Tue, 9 Jun 2026 16:45:47 -0500 Subject: [PATCH 06/12] Bump GitHub Actions to latest major versions Updates checkout v4->v6, setup-java v4->v5, and upload-artifact v4->v7 to clear the Node.js 20 deprecation warning and run on Node.js 24. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/workflows/ci.yml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index de5fe68..26ab2d5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -11,10 +11,10 @@ jobs: runs-on: ubuntu-latest steps: - name: Check out repository - uses: actions/checkout@v4 + uses: actions/checkout@v6 - name: Set up JDK 21 - uses: actions/setup-java@v4 + uses: actions/setup-java@v5 with: distribution: temurin java-version: "21" @@ -35,14 +35,14 @@ jobs: - name: Upload test results if: always() - uses: actions/upload-artifact@v4 + uses: actions/upload-artifact@v7 with: name: test-results path: build/test-results/test - name: Upload coverage report if: always() - uses: actions/upload-artifact@v4 + uses: actions/upload-artifact@v7 with: name: jacoco-coverage path: build/reports/jacoco/test/html From ede98e48aa38e1aff129b3385f14c2e463e25fbb Mon Sep 17 00:00:00 2001 From: John Flavin Date: Tue, 9 Jun 2026 16:55:57 -0500 Subject: [PATCH 07/12] Run CI on pull requests to develop Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 26ab2d5..7572ae5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -4,7 +4,7 @@ on: push: branches: ["**"] pull_request: - branches: [main] + branches: [main, develop] jobs: test: From 8be3183f24fdb74bfd8beaebf76a26cd5c15f779 Mon Sep 17 00:00:00 2001 From: John Flavin Date: Tue, 9 Jun 2026 16:59:22 -0500 Subject: [PATCH 08/12] Run CI only on PRs to main and develop Drop the push trigger on all branches so opening a PR no longer runs CI twice against the same commit. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/workflows/ci.yml | 2 -- 1 file changed, 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7572ae5..5d3c18e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1,8 +1,6 @@ name: CI on: - push: - branches: ["**"] pull_request: branches: [main, develop] From 9088e5cc58b6634cf88f578db20523268dc9be69 Mon Sep 17 00:00:00 2001 From: John Flavin Date: Tue, 9 Jun 2026 17:00:55 -0500 Subject: [PATCH 09/12] Also run CI on pushes/merges to main and develop Scoped to main/develop so merges get a post-merge run without double-running on PR branches. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/workflows/ci.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5d3c18e..d6a37f3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1,6 +1,8 @@ name: CI on: + push: + branches: [main, develop] pull_request: branches: [main, develop] From 5f1632418cf8b3243bd9d25eb82f6c3bff4fe820 Mon Sep 17 00:00:00 2001 From: John Flavin Date: Wed, 10 Jun 2026 08:53:33 -0500 Subject: [PATCH 10/12] Render test results in the CI run summary Add test-summary/action to read Gradle's JUnit XML and display a results table on the workflow run summary page. Matches the setup in the scout repo (same action, same pinned v2.6 SHA). Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/workflows/ci.yml | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d6a37f3..6b55763 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -33,6 +33,15 @@ jobs: - name: Run tests and coverage report run: ./gradlew test jacocoTestReport --no-daemon + # Render the JUnit XML that Gradle writes to build/test-results/test + # into the workflow run's summary page. Runs on always() so results + # are reported even when the test step above fails. + - name: Test summary + uses: test-summary/action@37b508cfee6d4d080eedd00b5bb240a6a784a6a5 # v2.6 + if: always() + with: + paths: build/test-results/test/TEST-*.xml + - name: Upload test results if: always() uses: actions/upload-artifact@v7 From 035fb8e8c6c2b3fe95f08b2cec725806eb5c90e5 Mon Sep 17 00:00:00 2001 From: John Flavin Date: Wed, 10 Jun 2026 08:57:20 -0500 Subject: [PATCH 11/12] Log per-test results to the console during the test task Gradle's test task is silent by default. Add a testLogging block so CI shows live pass/skip/fail progress and full stack traces on failure, and forwards test stdout/stderr to the build output. Co-Authored-By: Claude Opus 4.8 (1M context) --- build.gradle | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/build.gradle b/build.gradle index 3b33871..2ef054d 100644 --- a/build.gradle +++ b/build.gradle @@ -198,6 +198,19 @@ sourceSets { test { useJUnit() maxHeapSize = "1G" + + testLogging { + // Log each test as it passes/skips/fails so CI shows live progress + // instead of going silent until the build finishes. + events "passed", "skipped", "failed" + // Forward the tests' own System.out/System.err to the build output. + showStandardStreams = true + // Print full stack traces (with causes) for failures. + exceptionFormat = "full" + showExceptions = true + showCauses = true + showStackTraces = true + } } publishing { From bb9ebeb69ed6d24d7dc06cfe8b4bd6270e93cca1 Mon Sep 17 00:00:00 2001 From: John Flavin Date: Wed, 10 Jun 2026 09:07:31 -0500 Subject: [PATCH 12/12] Float test-summary action to v2 instead of a pinned SHA This repo has no automation to bump pinned action SHAs, so track the v2 major tag to pick up fixes automatically. Drop the Scout reference from the rationale comment. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6b55763..ea3560e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -37,7 +37,7 @@ jobs: # into the workflow run's summary page. Runs on always() so results # are reported even when the test step above fails. - name: Test summary - uses: test-summary/action@37b508cfee6d4d080eedd00b5bb240a6a784a6a5 # v2.6 + uses: test-summary/action@v2 if: always() with: paths: build/test-results/test/TEST-*.xml