diff --git a/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/google/GoogleOAuth2Provider.java b/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/google/GoogleOAuth2Provider.java index e383b41999e2..cc07848075c1 100644 --- a/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/google/GoogleOAuth2Provider.java +++ b/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/google/GoogleOAuth2Provider.java @@ -30,6 +30,7 @@ import org.apache.commons.lang3.StringUtils; import com.cloud.exception.CloudAuthenticationException; +import com.cloud.utils.Pair; import com.cloud.utils.component.AdapterBase; import com.cloud.utils.exception.CloudRuntimeException; import com.github.benmanes.caffeine.cache.Cache; @@ -45,9 +46,28 @@ import com.google.api.client.json.jackson2.JacksonFactory; import com.google.api.services.oauth2.Oauth2; import com.google.api.services.oauth2.model.Userinfo; +import com.google.common.cache.Cache; +import com.google.common.cache.CacheBuilder; +import com.google.common.util.concurrent.UncheckedExecutionException; +import org.apache.cloudstack.auth.UserOAuth2Authenticator; +import org.apache.cloudstack.oauth2.dao.OauthProviderDao; +import org.apache.cloudstack.oauth2.vo.OauthProviderVO; +import org.apache.commons.lang3.StringUtils; + +import javax.inject.Inject; +import java.io.IOException; +import java.util.Arrays; +import java.util.List; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.TimeUnit; public class GoogleOAuth2Provider extends AdapterBase implements UserOAuth2Authenticator { + protected final Cache> tokensByCode = CacheBuilder.newBuilder() + .maximumSize(1000) + .expireAfterWrite(10, TimeUnit.MINUTES) + .build(); + @Inject OauthProviderDao _oauthProviderDao; @@ -108,10 +128,9 @@ public boolean verifyUser(String email, String secretCode) { return true; } - protected String verifyCodeAndFetchEmailInternal(String secretCode, OauthProviderVO googleProvider) { - if (googleProvider == null) { - googleProvider = _oauthProviderDao.findByProvider(getName()); - } + @Override + public String verifyCodeAndFetchEmail(String secretCode) { + OauthProviderVO googleProvider = _oauthProviderDao.findByProvider(getName()); String clientId = googleProvider.getClientId(); String secret = googleProvider.getSecretKey(); String redirectURI = googleProvider.getRedirectUri(); @@ -129,14 +148,20 @@ protected String verifyCodeAndFetchEmailInternal(String secretCode, OauthProvide httpTransport, jsonFactory, clientSecrets, scopes) .build(); - GoogleTokenResponse tokenResponse; + Pair tokens; try { - tokenResponse = flow.newTokenRequest(secretCode) - .setRedirectUri(redirectURI) - .execute(); - } catch (IOException e) { - throw new CloudRuntimeException("Failed to verify secret code", e); + tokens = tokensByCode.get(secretCode, () -> { + GoogleTokenResponse tokenResponse = flow.newTokenRequest(secretCode) + .setRedirectUri(redirectURI) + .execute(); + return new Pair<>(tokenResponse.getAccessToken(), tokenResponse.getRefreshToken()); + }); + } catch (ExecutionException | UncheckedExecutionException e) { + Throwable cause = e.getCause() != null ? e.getCause() : e; + throw new CloudRuntimeException(String.format("Failed to exchange the OAuth2 authorization code for tokens: %s", cause.getMessage()), cause); } + String accessToken = tokens.first(); + String refreshToken = tokens.second(); String accessToken = tokenResponse.getAccessToken(); String refreshToken = tokenResponse.getRefreshToken(); @@ -155,18 +180,13 @@ protected String verifyCodeAndFetchEmailInternal(String secretCode, OauthProvide try { userinfo = oauth2.userinfo().get().execute(); } catch (IOException e) { - throw new CloudRuntimeException(String.format("Failed to fetch the email address with the provided secret: %s", e.getMessage())); + throw new CloudRuntimeException(String.format("Failed to fetch the email address with the provided secret: %s", e.getMessage()), e); } String verifiedEmail = userinfo.getEmail(); addValidatedEmailToCache(secretCode, verifiedEmail); return verifiedEmail; } - @Override - public String verifyCodeAndFetchEmail(String secretCode) { - return verifyCodeAndFetchEmailInternal(secretCode, null); - } - @Override public String getUserEmailAddress() throws CloudRuntimeException { return null; diff --git a/plugins/user-authenticators/oauth2/src/test/java/org/apache/cloudstack/oauth2/google/GoogleOAuth2ProviderTest.java b/plugins/user-authenticators/oauth2/src/test/java/org/apache/cloudstack/oauth2/google/GoogleOAuth2ProviderTest.java index a6c628182b4b..dc5f8693780e 100644 --- a/plugins/user-authenticators/oauth2/src/test/java/org/apache/cloudstack/oauth2/google/GoogleOAuth2ProviderTest.java +++ b/plugins/user-authenticators/oauth2/src/test/java/org/apache/cloudstack/oauth2/google/GoogleOAuth2ProviderTest.java @@ -17,15 +17,13 @@ package org.apache.cloudstack.oauth2.google; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertTrue; -import static org.junit.Assert.fail; -import static org.mockito.ArgumentMatchers.anyString; -import static org.mockito.Mockito.doReturn; -import static org.mockito.Mockito.when; - +import com.cloud.exception.CloudAuthenticationException; +import com.cloud.utils.exception.CloudRuntimeException; +import com.google.api.client.googleapis.auth.oauth2.GoogleAuthorizationCodeFlow; +import com.google.api.client.googleapis.auth.oauth2.GoogleAuthorizationCodeTokenRequest; +import com.google.api.client.googleapis.auth.oauth2.GoogleTokenResponse; +import com.google.api.services.oauth2.Oauth2; +import com.google.api.services.oauth2.model.Userinfo; import org.apache.cloudstack.oauth2.dao.OauthProviderDao; import org.apache.cloudstack.oauth2.vo.OauthProviderVO; import org.junit.After; @@ -36,8 +34,16 @@ import org.mockito.MockitoAnnotations; import org.mockito.Spy; -import com.cloud.exception.CloudAuthenticationException; -import com.cloud.utils.exception.CloudRuntimeException; +import java.io.IOException; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; public class GoogleOAuth2ProviderTest { @@ -66,6 +72,26 @@ public void tearDown() throws Exception { closeable.close(); } + private OauthProviderVO mockRegisteredProvider() { + OauthProviderVO providerVO = mock(OauthProviderVO.class); + when(_oauthProviderDao.findByProvider(anyString())).thenReturn(providerVO); + when(providerVO.getProvider()).thenReturn("testProvider"); + when(providerVO.getSecretKey()).thenReturn("testSecret"); + when(providerVO.getClientId()).thenReturn("testClientid"); + return providerVO; + } + + private GoogleAuthorizationCodeFlow mockTokenExchangeFlow(GoogleAuthorizationCodeTokenRequest tokenRequest) throws IOException { + GoogleAuthorizationCodeFlow flow = mock(GoogleAuthorizationCodeFlow.class); + GoogleTokenResponse tokenResponse = mock(GoogleTokenResponse.class); + when(flow.newTokenRequest(anyString())).thenReturn(tokenRequest); + when(tokenRequest.setRedirectUri(any())).thenReturn(tokenRequest); + when(tokenRequest.execute()).thenReturn(tokenResponse); + when(tokenResponse.getAccessToken()).thenReturn("testAccessToken"); + when(tokenResponse.getRefreshToken()).thenReturn("testRefreshToken"); + return flow; + } + @Test(expected = CloudAuthenticationException.class) public void testVerifyUserWithNullEmail() { _googleOAuth2Provider.verifyUser(null, "secretCode"); @@ -83,95 +109,138 @@ public void testVerifyUserWithUnregisteredProvider() { } @Test(expected = CloudRuntimeException.class) - public void testVerifyUserWithInvalidSecretCode() { - when(_oauthProviderDao.findByProvider(anyString())).thenReturn(mockProvider); - doReturn(null).when(_googleOAuth2Provider).verifyCodeAndFetchEmailInternal( - "secretCode", mockProvider); - - _googleOAuth2Provider.verifyUser("email@example.com", "secretCode"); + public void testVerifyUserWithInvalidSecretCode() throws IOException { + mockRegisteredProvider(); + GoogleAuthorizationCodeTokenRequest tokenRequest = mock(GoogleAuthorizationCodeTokenRequest.class); + GoogleAuthorizationCodeFlow flow = mockTokenExchangeFlow(tokenRequest); + Oauth2 oauth2 = mock(Oauth2.class); + try (MockedConstruction ignoredFlow = Mockito.mockConstruction(GoogleAuthorizationCodeFlow.Builder.class, + (mock, context) -> when(mock.build()).thenReturn(flow)); + MockedConstruction ignored = Mockito.mockConstruction(Oauth2.Builder.class, + (mock, context) -> when(mock.build()).thenReturn(oauth2))) { + Userinfo userinfo = mock(Userinfo.class); + Oauth2.Userinfo userinfo1 = mock(Oauth2.Userinfo.class); + when(oauth2.userinfo()).thenReturn(userinfo1); + Oauth2.Userinfo.Get userinfoGet = mock(Oauth2.Userinfo.Get.class); + when(userinfo1.get()).thenReturn(userinfoGet); + when(userinfoGet.execute()).thenReturn(userinfo); + when(userinfo.getEmail()).thenReturn(null); + + _googleOAuth2Provider.verifyUser("email@example.com", "secretCode"); + } } @Test(expected = CloudRuntimeException.class) - public void testVerifyUserWithMismatchedEmail() { - when(_oauthProviderDao.findByProvider(anyString())).thenReturn(mockProvider); - doReturn("otheremail@example.com").when(_googleOAuth2Provider).verifyCodeAndFetchEmailInternal( - "secretCode", mockProvider); - - _googleOAuth2Provider.verifyUser("email@example.com", "secretCode"); - } - - @Test - public void testVerifyUserEmail() { - when(_oauthProviderDao.findByProvider(anyString())).thenReturn(mockProvider); - doReturn("email@example.com").when(_googleOAuth2Provider).verifyCodeAndFetchEmailInternal( - "secretCode", mockProvider); - - boolean result = _googleOAuth2Provider.verifyUser("email@example.com", "secretCode"); - - assertTrue(result); + public void testVerifyUserWithMismatchedEmail() throws IOException { + mockRegisteredProvider(); + GoogleAuthorizationCodeTokenRequest tokenRequest = mock(GoogleAuthorizationCodeTokenRequest.class); + GoogleAuthorizationCodeFlow flow = mockTokenExchangeFlow(tokenRequest); + Oauth2 oauth2 = mock(Oauth2.class); + try (MockedConstruction ignoredFlow = Mockito.mockConstruction(GoogleAuthorizationCodeFlow.Builder.class, + (mock, context) -> when(mock.build()).thenReturn(flow)); + MockedConstruction ignored = Mockito.mockConstruction(Oauth2.Builder.class, + (mock, context) -> when(mock.build()).thenReturn(oauth2))) { + Userinfo userinfo = mock(Userinfo.class); + Oauth2.Userinfo userinfo1 = mock(Oauth2.Userinfo.class); + when(oauth2.userinfo()).thenReturn(userinfo1); + Oauth2.Userinfo.Get userinfoGet = mock(Oauth2.Userinfo.Get.class); + when(userinfo1.get()).thenReturn(userinfoGet); + when(userinfoGet.execute()).thenReturn(userinfo); + when(userinfo.getEmail()).thenReturn("otheremail@example.com"); + + _googleOAuth2Provider.verifyUser("email@example.com", "secretCode"); + } } - @Test - public void testCacheInitializationAndCleanupResources() { - // Verifies that cache cleanup executor is properly initialized - GoogleOAuth2Provider provider = new GoogleOAuth2Provider(); - assertNotNull("GoogleOAuth2Provider should initialize", provider); + @Test(expected = CloudRuntimeException.class) + public void testVerifyUserWithFailedTokenExchange() throws IOException { + mockRegisteredProvider(); + GoogleAuthorizationCodeFlow flow = mock(GoogleAuthorizationCodeFlow.class); + GoogleAuthorizationCodeTokenRequest tokenRequest = mock(GoogleAuthorizationCodeTokenRequest.class); + when(flow.newTokenRequest(anyString())).thenReturn(tokenRequest); + when(tokenRequest.setRedirectUri(any())).thenReturn(tokenRequest); + when(tokenRequest.execute()).thenThrow(new IOException("invalid_grant")); + try (MockedConstruction ignoredFlow = Mockito.mockConstruction(GoogleAuthorizationCodeFlow.Builder.class, + (mock, context) -> when(mock.build()).thenReturn(flow))) { + _googleOAuth2Provider.verifyUser("email@example.com", "secretCode"); + } } @Test - public void testNoSensitiveDataInErrorMessages() { - // Verifies that error messages don't expose sensitive information - when(_oauthProviderDao.findByProvider(anyString())).thenReturn(mockProvider); - String testSecret = "secret_key"; - String testEmail = "email@example.com"; - - try { - _googleOAuth2Provider.verifyUser(testEmail, testSecret); - fail("Expected exception"); - } catch (Exception e) { - String errorMsg = e.getMessage(); - // Verify sensitive terms are not in error messages - assertFalse("Error should not contain secret", errorMsg.toLowerCase().contains(testSecret)); - assertFalse("Error should not contain email", errorMsg.toLowerCase().contains(testEmail)); - assertFalse("Error should not contain token", errorMsg.toLowerCase().contains("access_token")); + public void testVerifyUserEmail() throws IOException { + mockRegisteredProvider(); + GoogleAuthorizationCodeTokenRequest tokenRequest = mock(GoogleAuthorizationCodeTokenRequest.class); + GoogleAuthorizationCodeFlow flow = mockTokenExchangeFlow(tokenRequest); + Oauth2 oauth2 = mock(Oauth2.class); + try (MockedConstruction ignoredFlow = Mockito.mockConstruction(GoogleAuthorizationCodeFlow.Builder.class, + (mock, context) -> when(mock.build()).thenReturn(flow)); + MockedConstruction ignored = Mockito.mockConstruction(Oauth2.Builder.class, + (mock, context) -> when(mock.build()).thenReturn(oauth2))) { + Userinfo userinfo = mock(Userinfo.class); + Oauth2.Userinfo userinfo1 = mock(Oauth2.Userinfo.class); + when(oauth2.userinfo()).thenReturn(userinfo1); + Oauth2.Userinfo.Get userinfoGet = mock(Oauth2.Userinfo.Get.class); + when(userinfo1.get()).thenReturn(userinfoGet); + when(userinfoGet.execute()).thenReturn(userinfo); + when(userinfo.getEmail()).thenReturn("email@example.com"); + + boolean result = _googleOAuth2Provider.verifyUser("email@example.com", "secretCode"); + + assertTrue(result); + verify(tokenRequest, times(1)).execute(); } } @Test - public void testVerifyUserErrorHandlingAndCleanup() { - // Tests that any authentication error properly cleans up - when(_oauthProviderDao.findByProvider(anyString())).thenReturn(mockProvider); - - doReturn("error@example.com").when(_googleOAuth2Provider).verifyCodeAndFetchEmailInternal( - "bad_code", mockProvider); - - try { - _googleOAuth2Provider.verifyUser("expected@example.com", "bad_code"); - fail("Should throw exception for email mismatch"); - } catch (CloudRuntimeException e) { - assertEquals("Should have proper error message", - "Unable to verify the email address with the provided secret", - e.getMessage()); + public void testVerifyCodeAndFetchEmailExchangesEachCodeIndependently() throws IOException { + mockRegisteredProvider(); + GoogleAuthorizationCodeTokenRequest tokenRequest = mock(GoogleAuthorizationCodeTokenRequest.class); + GoogleAuthorizationCodeFlow flow = mockTokenExchangeFlow(tokenRequest); + Oauth2 oauth2 = mock(Oauth2.class); + try (MockedConstruction ignoredFlow = Mockito.mockConstruction(GoogleAuthorizationCodeFlow.Builder.class, + (mock, context) -> when(mock.build()).thenReturn(flow)); + MockedConstruction ignored = Mockito.mockConstruction(Oauth2.Builder.class, + (mock, context) -> when(mock.build()).thenReturn(oauth2))) { + Userinfo userinfo = mock(Userinfo.class); + Oauth2.Userinfo userinfo1 = mock(Oauth2.Userinfo.class); + when(oauth2.userinfo()).thenReturn(userinfo1); + Oauth2.Userinfo.Get userinfoGet = mock(Oauth2.Userinfo.Get.class); + when(userinfo1.get()).thenReturn(userinfoGet); + when(userinfoGet.execute()).thenReturn(userinfo); + when(userinfo.getEmail()).thenReturn("email@example.com"); + + assertEquals("email@example.com", _googleOAuth2Provider.verifyCodeAndFetchEmail("secretCode1")); + assertEquals("email@example.com", _googleOAuth2Provider.verifyCodeAndFetchEmail("secretCode2")); + + verify(flow, times(1)).newTokenRequest("secretCode1"); + verify(flow, times(1)).newTokenRequest("secretCode2"); + verify(tokenRequest, times(2)).execute(); } } @Test - public void testMultipleFailedVerificationAttempts() { - // Tests that multiple failures are handled gracefully without cache pollution - when(_oauthProviderDao.findByProvider(anyString())).thenReturn(mockProvider); - - // Multiple failed attempts - for (int i = 0; i < 5; i++) { - try { - doReturn("wrong@example.com").when(_googleOAuth2Provider) - .verifyCodeAndFetchEmailInternal("code_" + i, mockProvider); - _googleOAuth2Provider.verifyUser("correct@example.com", "code_" + i); - fail("Should fail on attempt " + i); - } catch (CloudRuntimeException e) { - // Expected - cache should be cleaned for each failure - assertTrue("Should report email verification failure", - e.getMessage().contains("email")); - } + public void testVerifyCodeAndFetchEmailReusesTokensForSameCode() throws IOException { + mockRegisteredProvider(); + GoogleAuthorizationCodeTokenRequest tokenRequest = mock(GoogleAuthorizationCodeTokenRequest.class); + GoogleAuthorizationCodeFlow flow = mockTokenExchangeFlow(tokenRequest); + Oauth2 oauth2 = mock(Oauth2.class); + try (MockedConstruction ignoredFlow = Mockito.mockConstruction(GoogleAuthorizationCodeFlow.Builder.class, + (mock, context) -> when(mock.build()).thenReturn(flow)); + MockedConstruction ignored = Mockito.mockConstruction(Oauth2.Builder.class, + (mock, context) -> when(mock.build()).thenReturn(oauth2))) { + Userinfo userinfo = mock(Userinfo.class); + Oauth2.Userinfo userinfo1 = mock(Oauth2.Userinfo.class); + when(oauth2.userinfo()).thenReturn(userinfo1); + Oauth2.Userinfo.Get userinfoGet = mock(Oauth2.Userinfo.Get.class); + when(userinfo1.get()).thenReturn(userinfoGet); + when(userinfoGet.execute()).thenReturn(userinfo); + when(userinfo.getEmail()).thenReturn("email@example.com"); + + // the login flow uses the same one-time code twice: verifyOauthCodeAndGetUser then oauthlogin + assertEquals("email@example.com", _googleOAuth2Provider.verifyCodeAndFetchEmail("secretCode")); + assertTrue(_googleOAuth2Provider.verifyUser("email@example.com", "secretCode")); + + verify(tokenRequest, times(1)).execute(); } } }