Copilot commented on code in PR #13033: URL: https://github.com/apache/cloudstack/pull/13033#discussion_r3166869490
########## plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/keycloak/KeycloakOAuth2Provider.java: ########## @@ -0,0 +1,177 @@ +// +// 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 (provider == null) { + throw new CloudAuthenticationException("Keycloak provider is not registered, so user cannot be verified"); + } + + 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 generate 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); + Review Comment: The token endpoint response is assumed to always contain an `id_token`. If Keycloak is configured to not return an ID token (or an error response body is returned with HTTP 200), `json.get("id_token")` will be null and this will throw a NullPointerException. Please validate the JSON response (presence/type of `id_token`) and raise a clear authentication error when it’s missing. ########## plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/keycloak/KeycloakOAuth2Provider.java: ########## @@ -0,0 +1,177 @@ +// +// 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; Review Comment: `idToken` is stored as an instance field on the provider bean. Since these authenticators are created as singleton Spring beans by default, this cached token can be shared across concurrent login requests and leak/mix authentication state between users. Avoid storing per-request auth state in fields; keep the token in local variables and pass it through methods instead. ########## plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/keycloak/KeycloakOAuth2Provider.java: ########## @@ -0,0 +1,177 @@ +// +// 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 (provider == null) { + throw new CloudAuthenticationException("Keycloak provider is not registered, so user cannot be verified"); + } + + 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 generate 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; Review Comment: `getUserEmailAddress()` is currently unimplemented and always returns null. Since it is part of the `UserOAuth2Authenticator` contract, either implement it (e.g., derive the email from the cached token) or throw a clear exception indicating it’s unsupported to avoid confusing null propagation. ```suggestion if (StringUtils.isBlank(idToken)) { throw new CloudRuntimeException("User email address is unavailable because no ID token is cached"); } OauthProviderVO provider = oauthProviderDao.findByProvider(getName()); if (provider == null) { throw new CloudAuthenticationException("Keycloak provider is not registered, so user email address cannot be retrieved"); } return obtainEmail(idToken, provider); ``` ########## plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/keycloak/KeycloakOAuth2Provider.java: ########## @@ -0,0 +1,177 @@ +// +// 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 (provider == null) { + throw new CloudAuthenticationException("Keycloak provider is not registered, so user cannot be verified"); + } + + 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 generate 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; Review Comment: The local variable `String idToken = ...` shadows the `idToken` field, which is easy to misread and can lead to subtle bugs when modifying this code. Use a different local name (e.g., `fetchedIdToken`) or eliminate the field entirely (see concurrency concern). ```suggestion String fetchedIdToken = json.get("id_token").getAsString(); validateIdToken(fetchedIdToken, provider); this.idToken = fetchedIdToken; ``` ########## ui/src/views/auth/Login.vue: ########## @@ -198,10 +198,22 @@ :href="getGoogleUrl(from)" class="auth-btn google-auth" style="height: 38px; width: 185px; padding: 0" > - <img src="/assets/google.svg" style="width: 32px; padding: 5px" /> + <img src="/assets/google.svg" alt="Github" style="width: 32px; padding: 5px" /> <a-typography-text>Sign in with Google</a-typography-text> Review Comment: The Google button icon has an incorrect alt text (currently "Github"), which harms accessibility/screen readers. Update the alt text to describe the Google icon. ########## plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/keycloak/KeycloakOAuth2Provider.java: ########## @@ -0,0 +1,177 @@ +// +// 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 (provider == null) { + throw new CloudAuthenticationException("Keycloak provider is not registered, so user cannot be verified"); + } + + 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 generate 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: `validateIdToken`/`obtainEmail` parse the JWT but never verify its JWS signature or validate standard OIDC claims (e.g., `exp`, `iss`, `nonce`). This means a forged/unsigned token could be accepted as long as it contains the expected `aud`/`email` fields. Please implement proper ID token verification (signature validation via JWKS and required claim checks) before trusting the `email` claim. ########## ui/src/views/auth/Login.vue: ########## @@ -186,7 +186,7 @@ :href="getGitHubUrl(from)" class="auth-btn github-auth" style="height: 38px; width: 185px; padding: 0; margin-bottom: 5px;" > - <img src="/assets/github.svg" style="width: 32px; padding: 5px" /> + <img src="/assets/github.svg" alt="Google" style="width: 32px; padding: 5px" /> Review Comment: The GitHub button icon has an incorrect alt text (currently "Google"), which harms accessibility/screen readers. Update the alt text to describe the GitHub icon. ```suggestion <img src="/assets/github.svg" alt="GitHub" style="width: 32px; padding: 5px" /> ``` -- 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]
