Merge "[SAP IAS] Fix thread-safety race condition in PKCE handling"
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);
+ }
+}
