Copilot commented on code in PR #13033:
URL: https://github.com/apache/cloudstack/pull/13033#discussion_r3090955745
##########
plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/RegisterOAuthProviderCmd.java:
##########
@@ -98,10 +114,20 @@ public Map getDetails() {
@Override
public void execute() throws ServerApiException,
ConcurrentOperationException, EntityExistsException {
+ if (StringUtils.equals("keycloak", getProvider())) {
+ if (getAuthorizeUrl() == null || "".equals(getAuthorizeUrl())) {
+ throw new ServerApiException(ApiErrorCode.BAD_REQUEST,
"Parameter authorizationurl is mandatory for keycloak OAuth Provider");
Review Comment:
The validation error message references "authorizationurl", but the API
parameter added is `authorizeurl` (ApiConstants.AUTHORIZE_URL). Please align
the message with the actual parameter name.
```suggestion
throw new ServerApiException(ApiErrorCode.BAD_REQUEST,
"Parameter authorizeurl is mandatory for keycloak OAuth Provider");
```
##########
plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/RegisterOAuthProviderCmd.java:
##########
@@ -98,10 +114,20 @@ public Map getDetails() {
@Override
public void execute() throws ServerApiException,
ConcurrentOperationException, EntityExistsException {
+ if (StringUtils.equals("keycloak", getProvider())) {
+ if (getAuthorizeUrl() == null || "".equals(getAuthorizeUrl())) {
+ throw new ServerApiException(ApiErrorCode.BAD_REQUEST,
"Parameter authorizationurl is mandatory for keycloak OAuth Provider");
Review Comment:
The Keycloak-only parameter validation uses
`org.apache.commons.codec.binary.StringUtils` for string comparison and then
manual null/empty checks. This is error-prone and inconsistent with the rest of
this module (which uses `org.apache.commons.lang3.StringUtils`); please switch
to lang3 and use `isBlank`/`equalsIgnoreCase` as appropriate.
##########
plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/github/GithubOAuth2Provider.java:
##########
@@ -85,10 +86,9 @@ public String verifyCodeAndFetchEmail(String secretCode) {
protected String getAccessToken(String secretCode) throws
CloudRuntimeException {
OauthProviderVO githubProvider =
_oauthProviderDao.findByProvider(getName());
- String tokenUrl = "https://github.com/login/oauth/access_token";;
String generatedAccessToken = null;
try {
- URL url = new URL(tokenUrl);
+ URL url = new URL(githubProvider.getTokenUrl());
HttpURLConnection connection = (HttpURLConnection)
url.openConnection();
Review Comment:
`githubProvider.getTokenUrl()` can be null/blank for existing deployments
(the new DB columns default to NULL and older registered GitHub providers won't
have a token URL). `new URL(null)` will throw and break GitHub OAuth after
upgrade. Consider falling back to the standard GitHub token URL when `tokenUrl`
is blank and/or backfilling/enforcing it at registration/migration time.
##########
plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/RegisterOAuthProviderCmd.java:
##########
@@ -56,6 +58,12 @@ public class RegisterOAuthProviderCmd extends BaseCmd {
@Parameter(name = ApiConstants.REDIRECT_URI, type = CommandType.STRING,
description = "Redirect URI pre-registered in the specific OAuth provider",
required = true)
private String redirectUri;
+ @Parameter(name = ApiConstants.AUTHORIZE_URL, type = CommandType.STRING,
description = "Authorize URL for OAuth initialization (only required for
keyloack provider)")
Review Comment:
Typo in parameter description: "keyloack" should be "keycloak".
```suggestion
@Parameter(name = ApiConstants.AUTHORIZE_URL, type = CommandType.STRING,
description = "Authorize URL for OAuth initialization (only required for
keycloak provider)")
```
##########
plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/keycloak/KeycloakOAuth2Provider.java:
##########
@@ -0,0 +1,174 @@
+//
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you 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 org.apache.cloudstack.oauth2.keycloak;
+
+import java.io.IOException;
+import java.io.UnsupportedEncodingException;
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.Base64;
+import java.util.List;
+
+import javax.inject.Inject;
+import javax.ws.rs.core.HttpHeaders;
+
+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 org.apache.cxf.rs.security.jose.jws.JwsJwtCompactConsumer;
+import org.apache.cxf.rs.security.jose.jwt.JwtClaims;
+import org.apache.http.NameValuePair;
+import org.apache.http.client.entity.UrlEncodedFormEntity;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpPost;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClientBuilder;
+import org.apache.http.message.BasicNameValuePair;
+import org.apache.http.util.EntityUtils;
+
+import com.cloud.exception.CloudAuthenticationException;
+import com.cloud.utils.component.AdapterBase;
+import com.cloud.utils.exception.CloudRuntimeException;
+import com.google.gson.JsonObject;
+import com.google.gson.JsonParser;
+
+public class KeycloakOAuth2Provider extends AdapterBase implements
UserOAuth2Authenticator {
+
+ protected String idToken = null;
+
+ @Inject
+ OauthProviderDao oauthProviderDao;
+
+ private CloseableHttpClient httpClient;
+
+ public KeycloakOAuth2Provider() {
+ this(HttpClientBuilder.create().build());
+ }
+
+ public KeycloakOAuth2Provider(CloseableHttpClient httpClient) {
+ this.httpClient = httpClient;
+ }
+
+ @Override
+ public String getName() {
+ return "keycloak";
+ }
+
+ @Override
+ public String getDescription() {
+ return "Keycloak OAuth2 Provider Plugin";
+ }
+
+ @Override
+ public boolean verifyUser(String email, String secretCode) {
+ if (StringUtils.isAnyEmpty(email, secretCode)) {
+ throw new CloudAuthenticationException("Either email or secret
code should not be null/empty");
+ }
+
+ OauthProviderVO providerVO =
oauthProviderDao.findByProvider(getName());
+ if (providerVO == null) {
+ throw new CloudAuthenticationException("Keycloak provider is not
registered, so user cannot be verified");
+ }
+
+ String verifiedEmail = verifyCodeAndFetchEmail(secretCode);
+ if (StringUtils.isBlank(verifiedEmail) ||
!email.equals(verifiedEmail)) {
+ throw new CloudRuntimeException("Unable to verify the email
address with the provided secret");
+ }
+ clearIdToken();
+
+ return true;
+ }
+
+ @Override
+ public String verifyCodeAndFetchEmail(String secretCode) {
+ OauthProviderVO provider = oauthProviderDao.findByProvider(getName());
+
+ if (StringUtils.isBlank(idToken)) {
+ String auth = provider.getClientId() + ":" +
provider.getSecretKey();
+ String encodedAuth =
Base64.getEncoder().encodeToString(auth.getBytes(StandardCharsets.UTF_8));
+
+ List<NameValuePair> params = new ArrayList<>();
+ params.add(new BasicNameValuePair("grant_type",
"authorization_code"));
+ params.add(new BasicNameValuePair("code", secretCode));
+ params.add(new BasicNameValuePair("redirect_uri",
provider.getRedirectUri()));
+
+ HttpPost post = new HttpPost(provider.getTokenUrl());
+ post.setHeader(HttpHeaders.AUTHORIZATION, "Basic " + encodedAuth);
+
+ try {
+ post.setEntity(new UrlEncodedFormEntity(params));
+ } catch (UnsupportedEncodingException e) {
+ throw new CloudRuntimeException("Unable to generating URL
parameters: " + e.getMessage());
Review Comment:
Minor grammar issue in exception message: "Unable to generating URL
parameters" should be "Unable to generate URL parameters".
```suggestion
throw new CloudRuntimeException("Unable to generate URL
parameters: " + e.getMessage());
```
##########
plugins/user-authenticators/oauth2/src/test/java/org/apache/cloudstack/oauth2/keycloak/KeycloakOAuth2ProviderTest.java:
##########
@@ -0,0 +1,213 @@
+package org.apache.cloudstack.oauth2.keycloak;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.io.ByteArrayInputStream;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.util.Base64;
+
+import org.apache.cloudstack.oauth2.dao.OauthProviderDao;
+import org.apache.cloudstack.oauth2.vo.OauthProviderVO;
+import org.apache.http.HttpEntity;
+import org.apache.http.StatusLine;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpPost;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
+
+import com.cloud.exception.CloudAuthenticationException;
+import com.cloud.utils.exception.CloudRuntimeException;
+
+public class KeycloakOAuth2ProviderTest {
+
+ @Mock
+ private OauthProviderDao oauthProviderDao;
+
+ @Mock
+ private CloseableHttpClient httpClient;
+
+ private KeycloakOAuth2Provider provider;
+
+ private OauthProviderVO mockProviderVO;
+
+ @Before
+ public void setUp() {
+ MockitoAnnotations.initMocks(this);
+
+ provider = new KeycloakOAuth2Provider(httpClient);
+ provider.oauthProviderDao = oauthProviderDao;
+
+ mockProviderVO = new OauthProviderVO();
+ mockProviderVO.setClientId("test-client");
+ mockProviderVO.setSecretKey("test-secret");
+ mockProviderVO.setTokenUrl("http://localhost/token";);
+ mockProviderVO.setRedirectUri("http://localhost/redirect";);
+ }
+
+ @Test
+ public void testGetName() {
+ assertEquals("keycloak", provider.getName());
+ }
+
+ @Test(expected = CloudAuthenticationException.class)
+ public void testVerifyUserEmptyParams() {
+ provider.verifyUser("", "");
+ }
+
+ @Test(expected = CloudAuthenticationException.class)
+ public void testVerifyUserProviderNotFound() {
+ when(oauthProviderDao.findByProvider("keycloak")).thenReturn(null);
+ provider.verifyUser("[email protected]", "code123");
+ }
+
+ @Test(expected = CloudRuntimeException.class)
+ public void testVerifyCodeAndFetchEmailHttpError() throws IOException {
+
when(oauthProviderDao.findByProvider("keycloak")).thenReturn(mockProviderVO);
+
+ CloseableHttpResponse response = mock(CloseableHttpResponse.class);
+ StatusLine statusLine = mock(StatusLine.class);
+
+ when(statusLine.getStatusCode()).thenReturn(400);
+ when(response.getStatusLine()).thenReturn(statusLine);
+
+ HttpEntity entity = mock(HttpEntity.class);
+ when(entity.getContent()).thenReturn(new
ByteArrayInputStream("error".getBytes()));
+ when(response.getEntity()).thenReturn(entity);
+
+ when(httpClient.execute(any(HttpPost.class))).thenReturn(response);
+
+ provider.verifyCodeAndFetchEmail("invalid-code");
+ }
+
+ @Test(expected = CloudRuntimeException.class)
+ public void testVerifyCodeAndFetchEmailNetworkFailure() throws IOException
{
+
when(oauthProviderDao.findByProvider("keycloak")).thenReturn(mockProviderVO);
+ when(httpClient.execute(any(HttpPost.class))).thenThrow(new
IOException("Connexion refusée"));
+
+ provider.verifyCodeAndFetchEmail("code");
+ }
+
+ @Test(expected = CloudRuntimeException.class)
+ public void testVerifyUserWithMismatchedEmail() throws IOException {
+
when(oauthProviderDao.findByProvider("keycloak")).thenReturn(mockProviderVO);
+
+ String testEmail = "[email protected]";
+ String secretCode = "valid-auth-code";
+
+ String header = "{\"alg\":\"none\"}";
+ String payload = "{" +
+ "\"aud\":[\"test-client\"]," +
+ "\"email\":\"" + testEmail + "\"," +
+ "\"iss\":\"http://keycloak\","; +
+ "\"sub\":\"12345\"" +
+ "}";
+
+ String encodedHeader =
Base64.getUrlEncoder().withoutPadding().encodeToString(header.getBytes());
+ String encodedPayload =
Base64.getUrlEncoder().withoutPadding().encodeToString(payload.getBytes());
+ String fakeJwt = encodedHeader + "." + encodedPayload +
".not-checked-signature";
+
+ CloseableHttpResponse response = mock(CloseableHttpResponse.class);
+ StatusLine statusLine = mock(StatusLine.class);
+ HttpEntity entity = mock(HttpEntity.class);
+
+ when(statusLine.getStatusCode()).thenReturn(200);
+ when(response.getStatusLine()).thenReturn(statusLine);
+
+ String jsonResponseBody = "{\"id_token\":\"" + fakeJwt + "\",
\"access_token\":\"acc-123\"}";
+ when(entity.getContent()).thenReturn(new
ByteArrayInputStream(jsonResponseBody.getBytes(StandardCharsets.UTF_8)));
+ when(response.getEntity()).thenReturn(entity);
+
+ when(httpClient.execute(any(HttpPost.class))).thenReturn(response);
+
+ boolean result = provider.verifyUser("[email protected]", secretCode);
+
+ assertTrue("L'utilisateur devrait être vérifié avec succès", result);
+ }
+
+ @Test(expected = CloudRuntimeException.class)
+ public void testVerifyUserWithMismatchedClient() throws IOException {
+
when(oauthProviderDao.findByProvider("keycloak")).thenReturn(mockProviderVO);
+
+ String testEmail = "[email protected]";
+ String secretCode = "valid-auth-code";
+
+ String header = "{\"alg\":\"none\"}";
+ String payload = "{" +
+ "\"aud\":[\"anothertest-client\"]," +
+ "\"email\":\"" + testEmail + "\"," +
+ "\"iss\":\"http://keycloak\","; +
+ "\"sub\":\"12345\"" +
+ "}";
+
+ String encodedHeader =
Base64.getUrlEncoder().withoutPadding().encodeToString(header.getBytes());
+ String encodedPayload =
Base64.getUrlEncoder().withoutPadding().encodeToString(payload.getBytes());
+ String fakeJwt = encodedHeader + "." + encodedPayload +
".not-checked-signature";
+
+ CloseableHttpResponse response = mock(CloseableHttpResponse.class);
+ StatusLine statusLine = mock(StatusLine.class);
+ HttpEntity entity = mock(HttpEntity.class);
+
+ when(statusLine.getStatusCode()).thenReturn(200);
+ when(response.getStatusLine()).thenReturn(statusLine);
+
+ String jsonResponseBody = "{\"id_token\":\"" + fakeJwt + "\",
\"access_token\":\"acc-123\"}";
+ when(entity.getContent()).thenReturn(new
ByteArrayInputStream(jsonResponseBody.getBytes(StandardCharsets.UTF_8)));
+ when(response.getEntity()).thenReturn(entity);
+
+ when(httpClient.execute(any(HttpPost.class))).thenReturn(response);
+
+ boolean result = provider.verifyUser(testEmail, secretCode);
+
+ assertTrue("L'utilisateur devrait être vérifié avec succès", result);
+ }
Review Comment:
Same issue here: the test declares an expected exception but then asserts
success. Also the assertion message is in French; please keep test messages
consistent (English) across the suite.
##########
plugins/user-authenticators/oauth2/src/test/java/org/apache/cloudstack/oauth2/keycloak/KeycloakOAuth2ProviderTest.java:
##########
@@ -0,0 +1,213 @@
+package org.apache.cloudstack.oauth2.keycloak;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.io.ByteArrayInputStream;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.util.Base64;
+
+import org.apache.cloudstack.oauth2.dao.OauthProviderDao;
+import org.apache.cloudstack.oauth2.vo.OauthProviderVO;
+import org.apache.http.HttpEntity;
+import org.apache.http.StatusLine;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpPost;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
+
+import com.cloud.exception.CloudAuthenticationException;
+import com.cloud.utils.exception.CloudRuntimeException;
+
+public class KeycloakOAuth2ProviderTest {
+
+ @Mock
+ private OauthProviderDao oauthProviderDao;
+
+ @Mock
+ private CloseableHttpClient httpClient;
+
+ private KeycloakOAuth2Provider provider;
+
+ private OauthProviderVO mockProviderVO;
+
+ @Before
+ public void setUp() {
+ MockitoAnnotations.initMocks(this);
+
+ provider = new KeycloakOAuth2Provider(httpClient);
+ provider.oauthProviderDao = oauthProviderDao;
+
+ mockProviderVO = new OauthProviderVO();
+ mockProviderVO.setClientId("test-client");
+ mockProviderVO.setSecretKey("test-secret");
+ mockProviderVO.setTokenUrl("http://localhost/token";);
+ mockProviderVO.setRedirectUri("http://localhost/redirect";);
+ }
+
+ @Test
+ public void testGetName() {
+ assertEquals("keycloak", provider.getName());
+ }
+
+ @Test(expected = CloudAuthenticationException.class)
+ public void testVerifyUserEmptyParams() {
+ provider.verifyUser("", "");
+ }
+
+ @Test(expected = CloudAuthenticationException.class)
+ public void testVerifyUserProviderNotFound() {
+ when(oauthProviderDao.findByProvider("keycloak")).thenReturn(null);
+ provider.verifyUser("[email protected]", "code123");
+ }
+
+ @Test(expected = CloudRuntimeException.class)
+ public void testVerifyCodeAndFetchEmailHttpError() throws IOException {
+
when(oauthProviderDao.findByProvider("keycloak")).thenReturn(mockProviderVO);
+
+ CloseableHttpResponse response = mock(CloseableHttpResponse.class);
+ StatusLine statusLine = mock(StatusLine.class);
+
+ when(statusLine.getStatusCode()).thenReturn(400);
+ when(response.getStatusLine()).thenReturn(statusLine);
+
+ HttpEntity entity = mock(HttpEntity.class);
+ when(entity.getContent()).thenReturn(new
ByteArrayInputStream("error".getBytes()));
+ when(response.getEntity()).thenReturn(entity);
+
+ when(httpClient.execute(any(HttpPost.class))).thenReturn(response);
+
+ provider.verifyCodeAndFetchEmail("invalid-code");
+ }
+
+ @Test(expected = CloudRuntimeException.class)
+ public void testVerifyCodeAndFetchEmailNetworkFailure() throws IOException
{
+
when(oauthProviderDao.findByProvider("keycloak")).thenReturn(mockProviderVO);
+ when(httpClient.execute(any(HttpPost.class))).thenThrow(new
IOException("Connexion refusée"));
+
+ provider.verifyCodeAndFetchEmail("code");
+ }
+
+ @Test(expected = CloudRuntimeException.class)
+ public void testVerifyUserWithMismatchedEmail() throws IOException {
+
when(oauthProviderDao.findByProvider("keycloak")).thenReturn(mockProviderVO);
+
+ String testEmail = "[email protected]";
+ String secretCode = "valid-auth-code";
+
+ String header = "{\"alg\":\"none\"}";
+ String payload = "{" +
+ "\"aud\":[\"test-client\"]," +
+ "\"email\":\"" + testEmail + "\"," +
+ "\"iss\":\"http://keycloak\","; +
+ "\"sub\":\"12345\"" +
+ "}";
+
+ String encodedHeader =
Base64.getUrlEncoder().withoutPadding().encodeToString(header.getBytes());
+ String encodedPayload =
Base64.getUrlEncoder().withoutPadding().encodeToString(payload.getBytes());
+ String fakeJwt = encodedHeader + "." + encodedPayload +
".not-checked-signature";
+
+ CloseableHttpResponse response = mock(CloseableHttpResponse.class);
+ StatusLine statusLine = mock(StatusLine.class);
+ HttpEntity entity = mock(HttpEntity.class);
+
+ when(statusLine.getStatusCode()).thenReturn(200);
+ when(response.getStatusLine()).thenReturn(statusLine);
+
+ String jsonResponseBody = "{\"id_token\":\"" + fakeJwt + "\",
\"access_token\":\"acc-123\"}";
+ when(entity.getContent()).thenReturn(new
ByteArrayInputStream(jsonResponseBody.getBytes(StandardCharsets.UTF_8)));
+ when(response.getEntity()).thenReturn(entity);
+
+ when(httpClient.execute(any(HttpPost.class))).thenReturn(response);
+
+ boolean result = provider.verifyUser("[email protected]", secretCode);
+
+ assertTrue("L'utilisateur devrait être vérifié avec succès", result);
+ }
Review Comment:
This test is annotated with `@Test(expected = CloudRuntimeException.class)`
but then asserts success (`assertTrue(...)`). The assertion is unreachable when
the exception is thrown and the message is misleading; please rewrite the test
to explicitly assert the exception (and keep messages consistent with the
expected outcome).
##########
plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/keycloak/KeycloakOAuth2Provider.java:
##########
@@ -0,0 +1,174 @@
+//
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you 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 org.apache.cloudstack.oauth2.keycloak;
+
+import java.io.IOException;
+import java.io.UnsupportedEncodingException;
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.Base64;
+import java.util.List;
+
+import javax.inject.Inject;
+import javax.ws.rs.core.HttpHeaders;
+
+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 org.apache.cxf.rs.security.jose.jws.JwsJwtCompactConsumer;
+import org.apache.cxf.rs.security.jose.jwt.JwtClaims;
+import org.apache.http.NameValuePair;
+import org.apache.http.client.entity.UrlEncodedFormEntity;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpPost;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClientBuilder;
+import org.apache.http.message.BasicNameValuePair;
+import org.apache.http.util.EntityUtils;
+
+import com.cloud.exception.CloudAuthenticationException;
+import com.cloud.utils.component.AdapterBase;
+import com.cloud.utils.exception.CloudRuntimeException;
+import com.google.gson.JsonObject;
+import com.google.gson.JsonParser;
+
+public class KeycloakOAuth2Provider extends AdapterBase implements
UserOAuth2Authenticator {
+
+ protected String idToken = null;
+
Review Comment:
`idToken` is stored as an instance field. Since provider beans are typically
singletons, this shared mutable state can leak between concurrent login
attempts. Please avoid storing per-request tokens on the provider instance;
keep them local to the method or in a request/session-scoped context.
##########
ui/src/views/auth/Login.vue:
##########
@@ -231,10 +243,14 @@ export default {
socialLogin: false,
googleprovider: false,
githubprovider: false,
+ keycloakprovider: false,
googleredirecturi: '',
githubredirecturi: '',
+ keycloakredirecturi: '',
googleclientid: '',
githubclientid: '',
+ keycloakclientid: '',
+ keycloakauthorizeurl: '',
Review Comment:
Variable name `keycloakauthorizeurl` appears misspelled (extra 'a' after
keycloac-). This makes the code harder to search/maintain; consider renaming to
`keycloakAuthorizeUrl` (and adjusting references) to avoid propagating the typo.
```suggestion
keycloakAuthorizeUrl: '',
```
##########
plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/keycloak/KeycloakOAuth2Provider.java:
##########
@@ -0,0 +1,174 @@
+//
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you 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 org.apache.cloudstack.oauth2.keycloak;
+
+import java.io.IOException;
+import java.io.UnsupportedEncodingException;
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.Base64;
+import java.util.List;
+
+import javax.inject.Inject;
+import javax.ws.rs.core.HttpHeaders;
+
+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 org.apache.cxf.rs.security.jose.jws.JwsJwtCompactConsumer;
+import org.apache.cxf.rs.security.jose.jwt.JwtClaims;
+import org.apache.http.NameValuePair;
+import org.apache.http.client.entity.UrlEncodedFormEntity;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpPost;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClientBuilder;
+import org.apache.http.message.BasicNameValuePair;
+import org.apache.http.util.EntityUtils;
+
+import com.cloud.exception.CloudAuthenticationException;
+import com.cloud.utils.component.AdapterBase;
+import com.cloud.utils.exception.CloudRuntimeException;
+import com.google.gson.JsonObject;
+import com.google.gson.JsonParser;
+
+public class KeycloakOAuth2Provider extends AdapterBase implements
UserOAuth2Authenticator {
+
+ protected String idToken = null;
+
+ @Inject
+ OauthProviderDao oauthProviderDao;
+
+ private CloseableHttpClient httpClient;
+
+ public KeycloakOAuth2Provider() {
+ this(HttpClientBuilder.create().build());
+ }
+
+ public KeycloakOAuth2Provider(CloseableHttpClient httpClient) {
+ this.httpClient = httpClient;
+ }
+
+ @Override
+ public String getName() {
+ return "keycloak";
+ }
+
+ @Override
+ public String getDescription() {
+ return "Keycloak OAuth2 Provider Plugin";
+ }
+
+ @Override
+ public boolean verifyUser(String email, String secretCode) {
+ if (StringUtils.isAnyEmpty(email, secretCode)) {
+ throw new CloudAuthenticationException("Either email or secret
code should not be null/empty");
+ }
+
+ OauthProviderVO providerVO =
oauthProviderDao.findByProvider(getName());
+ if (providerVO == null) {
+ throw new CloudAuthenticationException("Keycloak provider is not
registered, so user cannot be verified");
+ }
+
+ String verifiedEmail = verifyCodeAndFetchEmail(secretCode);
+ if (StringUtils.isBlank(verifiedEmail) ||
!email.equals(verifiedEmail)) {
+ throw new CloudRuntimeException("Unable to verify the email
address with the provided secret");
+ }
+ clearIdToken();
+
+ return true;
+ }
+
+ @Override
+ public String verifyCodeAndFetchEmail(String secretCode) {
+ OauthProviderVO provider = oauthProviderDao.findByProvider(getName());
+
+ if (StringUtils.isBlank(idToken)) {
+ String auth = provider.getClientId() + ":" +
provider.getSecretKey();
+ String encodedAuth =
Base64.getEncoder().encodeToString(auth.getBytes(StandardCharsets.UTF_8));
Review Comment:
`verifyCodeAndFetchEmail` dereferences `provider` without checking for null.
This method can be called directly via
`OAuth2AuthManagerImpl.verifyCodeAndFetchEmail(...)`, so an unregistered
Keycloak provider would trigger a NullPointerException. Please add an explicit
null check and throw a clear authentication/validation exception.
##########
ui/src/views/auth/Login.vue:
##########
@@ -202,6 +202,18 @@
<a-typography-text>Sign in with Google</a-typography-text>
</a-button>
</div>
+ <div class="social-auth" v-if="keycloakprovider">
+ <a-button
+ @click="handleKeycloakProviderAndDomain"
+ tag="a"
+ color="primary"
+ :href="getKeycloakUrl(from)"
+ class="auth-btn keycloak-auth"
+ style="height: 38px; width: 185px; padding: 0" >
+ <img src="/assets/keycloak.svg" style="width: 32px; padding: 5px" />
Review Comment:
The new Keycloak login button adds an <img> without an alt attribute. This
is an accessibility issue for screen readers; please add a meaningful alt text
(or mark it as decorative with empty alt if appropriate).
```suggestion
<img src="/assets/keycloak.svg" alt="" style="width: 32px;
padding: 5px" />
```
##########
plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/keycloak/KeycloakOAuth2Provider.java:
##########
@@ -0,0 +1,174 @@
+//
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you 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 org.apache.cloudstack.oauth2.keycloak;
+
+import java.io.IOException;
+import java.io.UnsupportedEncodingException;
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.Base64;
+import java.util.List;
+
+import javax.inject.Inject;
+import javax.ws.rs.core.HttpHeaders;
+
+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 org.apache.cxf.rs.security.jose.jws.JwsJwtCompactConsumer;
+import org.apache.cxf.rs.security.jose.jwt.JwtClaims;
+import org.apache.http.NameValuePair;
+import org.apache.http.client.entity.UrlEncodedFormEntity;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpPost;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClientBuilder;
+import org.apache.http.message.BasicNameValuePair;
+import org.apache.http.util.EntityUtils;
+
+import com.cloud.exception.CloudAuthenticationException;
+import com.cloud.utils.component.AdapterBase;
+import com.cloud.utils.exception.CloudRuntimeException;
+import com.google.gson.JsonObject;
+import com.google.gson.JsonParser;
+
+public class KeycloakOAuth2Provider extends AdapterBase implements
UserOAuth2Authenticator {
+
+ protected String idToken = null;
+
+ @Inject
+ OauthProviderDao oauthProviderDao;
+
+ private CloseableHttpClient httpClient;
+
+ public KeycloakOAuth2Provider() {
+ this(HttpClientBuilder.create().build());
+ }
+
+ public KeycloakOAuth2Provider(CloseableHttpClient httpClient) {
+ this.httpClient = httpClient;
+ }
+
+ @Override
+ public String getName() {
+ return "keycloak";
+ }
+
+ @Override
+ public String getDescription() {
+ return "Keycloak OAuth2 Provider Plugin";
+ }
+
+ @Override
+ public boolean verifyUser(String email, String secretCode) {
+ if (StringUtils.isAnyEmpty(email, secretCode)) {
+ throw new CloudAuthenticationException("Either email or secret
code should not be null/empty");
+ }
+
+ OauthProviderVO providerVO =
oauthProviderDao.findByProvider(getName());
+ if (providerVO == null) {
+ throw new CloudAuthenticationException("Keycloak provider is not
registered, so user cannot be verified");
+ }
+
+ String verifiedEmail = verifyCodeAndFetchEmail(secretCode);
+ if (StringUtils.isBlank(verifiedEmail) ||
!email.equals(verifiedEmail)) {
+ throw new CloudRuntimeException("Unable to verify the email
address with the provided secret");
+ }
+ clearIdToken();
+
+ return true;
+ }
+
+ @Override
+ public String verifyCodeAndFetchEmail(String secretCode) {
+ OauthProviderVO provider = oauthProviderDao.findByProvider(getName());
+
+ if (StringUtils.isBlank(idToken)) {
+ String auth = provider.getClientId() + ":" +
provider.getSecretKey();
+ String encodedAuth =
Base64.getEncoder().encodeToString(auth.getBytes(StandardCharsets.UTF_8));
+
+ List<NameValuePair> params = new ArrayList<>();
+ params.add(new BasicNameValuePair("grant_type",
"authorization_code"));
+ params.add(new BasicNameValuePair("code", secretCode));
+ params.add(new BasicNameValuePair("redirect_uri",
provider.getRedirectUri()));
+
+ HttpPost post = new HttpPost(provider.getTokenUrl());
+ post.setHeader(HttpHeaders.AUTHORIZATION, "Basic " + encodedAuth);
+
+ try {
+ post.setEntity(new UrlEncodedFormEntity(params));
+ } catch (UnsupportedEncodingException e) {
+ throw new CloudRuntimeException("Unable to generating URL
parameters: " + e.getMessage());
+ }
+
+ try (CloseableHttpResponse response = httpClient.execute(post)) {
+ String body = EntityUtils.toString(response.getEntity());
+
+ if (response.getStatusLine().getStatusCode() != 200) {
+ throw new CloudRuntimeException("Keycloak error during
token generation: " + body);
+ }
+
+ JsonObject json =
JsonParser.parseString(body).getAsJsonObject();
+ String idToken = json.get("id_token").getAsString();
+ validateIdToken(idToken, provider);
+
+ this.idToken = idToken;
+ } catch (IOException e) {
+ throw new CloudRuntimeException("Unable to connect to Keycloak
server", e);
+ }
+ }
+
+ return obtainEmail(idToken, provider);
+ }
+
+ @Override
+ public String getUserEmailAddress() throws CloudRuntimeException {
+ return null;
+ }
+
+ private void validateIdToken(String idTokenStr, OauthProviderVO provider) {
+ JwsJwtCompactConsumer jwtConsumer = new
JwsJwtCompactConsumer(idTokenStr);
+ JwtClaims claims = jwtConsumer.getJwtToken().getClaims();
+
+ if (!claims.getAudiences().contains(provider.getClientId())) {
+ throw new CloudAuthenticationException("Audience mismatch");
+ }
+ }
+
+ private String obtainEmail(String idTokenStr, OauthProviderVO provider) {
+ JwsJwtCompactConsumer jwtConsumer = new
JwsJwtCompactConsumer(idTokenStr);
+ JwtClaims claims = jwtConsumer.getJwtToken().getClaims();
+
+ if (!claims.getAudiences().contains(provider.getClientId())) {
+ throw new CloudAuthenticationException("Audience mismatch");
+ }
+
+ return (String) claims.getClaim("email");
Review Comment:
The ID token is parsed with `JwsJwtCompactConsumer` but its signature is
never verified (and the tests even use `{"alg":"none"}` tokens). This allows
token forgery. Please perform proper OIDC validation: verify JWS signature
against the provider's JWKS, and validate standard claims (exp/iat, iss, aud,
nonce) before trusting the `email` claim.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
