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} - - - diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 0000000..ea3560e --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,57 @@ +name: CI + +on: + push: + branches: [main, develop] + pull_request: + branches: [main, develop] + +jobs: + test: + runs-on: ubuntu-latest + steps: + - name: Check out repository + uses: actions/checkout@v6 + + - name: Set up JDK 21 + uses: actions/setup-java@v5 + with: + distribution: temurin + 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 + + # 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@v2 + if: always() + with: + paths: build/test-results/test/TEST-*.xml + + - name: Upload test results + if: always() + uses: actions/upload-artifact@v7 + with: + name: test-results + path: build/test-results/test + + - name: Upload coverage report + if: always() + uses: actions/upload-artifact@v7 + with: + name: jacoco-coverage + path: build/reports/jacoco/test/html 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 { 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")); + } +} 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()); + } +}