[SAP IAS] Fix thread-safety race condition in PKCE handling The previous implementation used a shared AuthorizationUrlBuilder field in the SAPIasOAuthService singleton. Calling initPKCE() on this shared builder meant that concurrent login requests from different users would overwrite the code_verifier, leading to intermittent authentication failures. Use the new OAuthAuthorizationInfo extension point to generate and return a per-request code_verifier. This allows Gerrit to store the verifier in the user's session and pass it back to the getAccessToken(OAuthVerifier, String) method, ensuring the verifier matches the original challenge for each specific login flow. The single-argument getAccessToken(OAuthVerifier) and getAuthorizationUrl() methods are removed as they are now provided by default implementations in the OAuthServiceProvider interface. Add SAPIasOAuthServiceTest to verify the PKCE flow and ensure that the code verifier is correctly extracted from the session and passed to the underlying ScribeJava service. Depends-On: https://gerrit-review.googlesource.com/c/gerrit/+/564343 Change-Id: I718ec9d72da1c13ca4ac405563a8d4c4a5636b8e
diff --git a/BUILD b/BUILD index 9c1e3ca..72d61d8 100644 --- a/BUILD +++ b/BUILD
@@ -47,6 +47,8 @@ ":oauth__plugin", "@oauth_plugin_deps//:com_github_scribejava_scribejava_apis", "@oauth_plugin_deps//:com_github_scribejava_scribejava_core", + "@oauth_plugin_deps//:com_sap_cloud_security_java_api", + "@oauth_plugin_deps//:com_sap_cloud_security_java_security", ], )
diff --git a/src/main/java/com/googlesource/gerrit/plugins/oauth/sap/SAPIasOAuthService.java b/src/main/java/com/googlesource/gerrit/plugins/oauth/sap/SAPIasOAuthService.java index 7925e6b..1f7f0e1 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/oauth/sap/SAPIasOAuthService.java +++ b/src/main/java/com/googlesource/gerrit/plugins/oauth/sap/SAPIasOAuthService.java
@@ -21,6 +21,8 @@ import com.github.scribejava.core.oauth.AuthorizationUrlBuilder; import com.github.scribejava.core.oauth.OAuth20Service; import com.google.common.base.Strings; +import com.google.gerrit.common.Nullable; +import com.google.gerrit.extensions.auth.oauth.OAuthAuthorizationInfo; import com.google.gerrit.extensions.auth.oauth.OAuthServiceProvider; import com.google.gerrit.extensions.auth.oauth.OAuthToken; import com.google.gerrit.extensions.auth.oauth.OAuthUserInfo; @@ -56,7 +58,6 @@ private final String rootUrl; private final boolean linkExistingGerrit; private final boolean enablePKCE; - private final AuthorizationUrlBuilder authorizationUrlBuilder; private final String extIdScheme; private final CombiningValidator<Token> tokenValidator; @@ -76,7 +77,6 @@ service = oauth20ServiceFactory.create(PROVIDER_NAME, new SAPIasApi(rootUrl), "openid profile email"); - authorizationUrlBuilder = service.createAuthorizationUrlBuilder(); extIdScheme = OAuthServiceProviderExternalIdScheme.create(PROVIDER_NAME); this.tokenValidator = tokenValidator; } @@ -110,11 +110,11 @@ } @Override - public OAuthToken getAccessToken(OAuthVerifier rv) { + public OAuthToken getAccessToken(OAuthVerifier rv, @Nullable String codeVerifier) { try { AccessTokenRequestParams reqParams = AccessTokenRequestParams.create(rv.getValue()); - if (enablePKCE) { - reqParams.pkceCodeVerifier(authorizationUrlBuilder.getPkce().getCodeVerifier()); + if (enablePKCE && codeVerifier != null) { + reqParams.pkceCodeVerifier(codeVerifier); } OAuth2AccessToken accessToken = service.getAccessToken(reqParams); return new OAuthToken( @@ -127,11 +127,14 @@ } @Override - public String getAuthorizationUrl() { + public OAuthAuthorizationInfo getAuthorizationInfo() { + AuthorizationUrlBuilder builder = service.createAuthorizationUrlBuilder(); + String pkceVerifier = null; if (enablePKCE) { - authorizationUrlBuilder.initPKCE(); + builder.initPKCE(); + pkceVerifier = builder.getPkce().getCodeVerifier(); } - return authorizationUrlBuilder.build(); + return new OAuthAuthorizationInfo(builder.build(), pkceVerifier); } public OAuth2AccessToken getAccessToken(String externalUsername, String password) {
diff --git a/src/test/java/com/googlesource/gerrit/plugins/oauth/sap/SAPIasOAuthServiceTest.java b/src/test/java/com/googlesource/gerrit/plugins/oauth/sap/SAPIasOAuthServiceTest.java new file mode 100644 index 0000000..a929d54 --- /dev/null +++ b/src/test/java/com/googlesource/gerrit/plugins/oauth/sap/SAPIasOAuthServiceTest.java
@@ -0,0 +1,115 @@ +// Copyright (C) 2026 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.googlesource.gerrit.plugins.oauth.sap; + +import static com.google.common.truth.Truth.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import com.github.scribejava.core.oauth.AccessTokenRequestParams; +import com.github.scribejava.core.oauth.AuthorizationUrlBuilder; +import com.github.scribejava.core.oauth.OAuth20Service; +import com.github.scribejava.core.pkce.PKCE; +import com.google.gerrit.extensions.auth.oauth.OAuthAuthorizationInfo; +import com.google.gerrit.extensions.auth.oauth.OAuthVerifier; +import com.google.gerrit.server.config.PluginConfig; +import com.googlesource.gerrit.plugins.oauth.InitOAuth; +import com.googlesource.gerrit.plugins.oauth.OAuth20ServiceFactory; +import com.googlesource.gerrit.plugins.oauth.OAuthPluginConfigFactory; +import com.sap.cloud.security.token.Token; +import com.sap.cloud.security.token.validation.CombiningValidator; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.ArgumentCaptor; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnitRunner; + +@RunWith(MockitoJUnitRunner.class) +public class SAPIasOAuthServiceTest { + + @Mock private OAuthPluginConfigFactory mockConfigFactory; + @Mock private PluginConfig mockPluginConfig; + @Mock private OAuth20ServiceFactory mockServiceFactory; + @Mock private OAuth20Service mockScribeOAuthService; + @Mock private CombiningValidator<Token> mockTokenValidator; + @Mock private AuthorizationUrlBuilder mockUrlBuilder; + + private static final String TEST_SAP_ROOT_URL = "https://accounts.sap.com"; + + @Before + public void setUp() { + when(mockConfigFactory.create(SAPIasOAuthService.PROVIDER_NAME)).thenReturn(mockPluginConfig); + when(mockPluginConfig.getString(InitOAuth.ROOT_URL)).thenReturn(TEST_SAP_ROOT_URL); + + when(mockServiceFactory.create( + anyString(), + any(com.github.scribejava.core.builder.api.DefaultApi20.class), + anyString())) + .thenReturn(mockScribeOAuthService); + } + + @Test + public void getAuthorizationInfo_withPkce_shouldReturnVerifierFromLocalBuilder() { + when(mockPluginConfig.getBoolean(InitOAuth.ENABLE_PKCE, false)).thenReturn(true); + + AuthorizationUrlBuilder mockUrlBuilder = mock(AuthorizationUrlBuilder.class); + when(mockScribeOAuthService.createAuthorizationUrlBuilder()).thenReturn(mockUrlBuilder); + + PKCE pkce = new PKCE(); + pkce.setCodeVerifier("sap-secret-verifier"); + + when(mockUrlBuilder.getPkce()).thenReturn(pkce); + when(mockUrlBuilder.build()).thenReturn("https://sap.com/auth?code_challenge=xyz"); + + SAPIasOAuthService service = + new SAPIasOAuthService(mockConfigFactory, mockServiceFactory, mockTokenValidator); + OAuthAuthorizationInfo info = service.getAuthorizationInfo(); + + assertThat(info.getPkceVerifier()).isEqualTo("sap-secret-verifier"); + } + + @Test + public void getAccessToken_withPkce_shouldUsePassedVerifierIgnoringInternalState() + throws Exception { + when(mockPluginConfig.getBoolean(InitOAuth.ENABLE_PKCE, false)).thenReturn(true); + SAPIasOAuthService service = + new SAPIasOAuthService(mockConfigFactory, mockServiceFactory, mockTokenValidator); + + OAuthVerifier verifier = new OAuthVerifier("auth-code"); + String verifierFromSession = "session-stored-verifier"; + + com.github.scribejava.core.model.OAuth2AccessToken mockToken = + mock(com.github.scribejava.core.model.OAuth2AccessToken.class); + + when(mockToken.getAccessToken()).thenReturn("token"); + when(mockToken.getTokenType()).thenReturn("Bearer"); + when(mockToken.getRawResponse()).thenReturn("dummy-raw"); + + when(mockScribeOAuthService.getAccessToken(any(AccessTokenRequestParams.class))) + .thenReturn(mockToken); + + service.getAccessToken(verifier, verifierFromSession); + + ArgumentCaptor<AccessTokenRequestParams> captor = + ArgumentCaptor.forClass(AccessTokenRequestParams.class); + verify(mockScribeOAuthService).getAccessToken(captor.capture()); + + assertThat(captor.getValue().getPkceCodeVerifier()).isEqualTo(verifierFromSession); + } +}