Conversation
|
Congratulations on your first Pull Request and welcome to the Apache CloudStack community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md)
|
|
@blueorangutan package |
|
@sureshanaparti a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #13033 +/- ##
============================================
- Coverage 18.01% 17.96% -0.05%
- Complexity 16474 16540 +66
============================================
Files 5976 6023 +47
Lines 537858 541527 +3669
Branches 66049 66357 +308
============================================
+ Hits 96882 97290 +408
- Misses 430052 433264 +3212
- Partials 10924 10973 +49
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds Keycloak as an additional OAuth2/OIDC provider by extending the OAuth provider model/API/schema and exposing new provider URL fields in the UI, along with a new Keycloak authenticator implementation and tests.
Changes:
- Add
authorizeurlandtokenurlfields end-to-end (DB schema, VO/response objects, API commands, and UI config/i18n). - Register a new
KeycloakOAuth2Providerand include it in the default provider order. - Update GitHub token exchange to use a configurable token URL instead of a hardcoded endpoint.
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/src/views/auth/Login.vue | Adds Keycloak social login button and consumes authorizeurl from listOauthProvider. |
| ui/src/config/section/config.js | Extends OAuth provider UI config to edit/display authorizeurl/tokenurl and adds keycloak to provider options. |
| ui/public/locales/en.json | Adds UI labels for Authorize URL and Token URL. |
| ui/public/assets/keycloak.svg | Adds Keycloak icon asset for the login UI. |
| plugins/user-authenticators/oauth2/src/test/java/org/apache/cloudstack/oauth2/keycloak/KeycloakOAuth2ProviderTest.java | Introduces unit tests for the new Keycloak provider. |
| plugins/user-authenticators/oauth2/src/main/resources/META-INF/cloudstack/oauth2/spring-oauth2-context.xml | Registers Keycloak provider bean and adds it to default ordering. |
| plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/vo/OauthProviderVO.java | Adds authorizeUrl and tokenUrl columns/fields to the OAuth provider entity. |
| plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/keycloak/KeycloakOAuth2Provider.java | Implements Keycloak OAuth2/OIDC flow using token endpoint + ID token parsing. |
| plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/google/GoogleOAuth2Provider.java | Minor refactor/cleanup and fixes a format string. |
| plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/github/GithubOAuth2Provider.java | Switches GitHub token endpoint to DB-configured tokenUrl. |
| plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/response/OauthProviderResponse.java | Extends API response to include authorizeurl and tokenurl. |
| plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/UpdateOAuthProviderCmd.java | Adds new parameters and returns them in response. |
| plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/RegisterOAuthProviderCmd.java | Adds new parameters and validates Keycloak requires the URLs. |
| plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/ListOAuthProvidersCmd.java | Includes new URL fields in list responses. |
| plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/OAuth2AuthManagerImpl.java | Plumbs new URL fields through register/update persistence. |
| plugins/user-authenticators/oauth2/pom.xml | Adds CXF JOSE dependency used for JWT parsing. |
| engine/schema/src/main/resources/META-INF/db/schema-42210to42300.sql | Adds authorize_url and token_url columns to cloud.oauth_provider. |
| api/src/main/java/org/apache/cloudstack/api/ApiConstants.java | Adds API constants for authorizeurl and tokenurl. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (StringUtils.equals("keycloak", getProvider())) { | ||
| if (getAuthorizeUrl() == null || "".equals(getAuthorizeUrl())) { | ||
| throw new ServerApiException(ApiErrorCode.BAD_REQUEST, "Parameter authorizationurl is mandatory for keycloak OAuth Provider"); |
There was a problem hiding this 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.
| 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"); |
There was a problem hiding this 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.
| throw new ServerApiException(ApiErrorCode.BAD_REQUEST, "Parameter authorizationurl is mandatory for keycloak OAuth Provider"); | |
| throw new ServerApiException(ApiErrorCode.BAD_REQUEST, "Parameter authorizeurl is mandatory for keycloak OAuth Provider"); |
| protected String idToken = null; | ||
|
|
There was a problem hiding this 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.
| try { | ||
| post.setEntity(new UrlEncodedFormEntity(params)); | ||
| } catch (UnsupportedEncodingException e) { | ||
| throw new CloudRuntimeException("Unable to generating URL parameters: " + e.getMessage()); |
There was a problem hiding this comment.
Minor grammar issue in exception message: "Unable to generating URL parameters" should be "Unable to generate URL parameters".
| throw new CloudRuntimeException("Unable to generating URL parameters: " + e.getMessage()); | |
| throw new CloudRuntimeException("Unable to generate URL parameters: " + e.getMessage()); |
| boolean result = provider.verifyUser("user@example.com", secretCode); | ||
|
|
||
| assertTrue("L'utilisateur devrait être vérifié avec succès", result); | ||
| } |
There was a problem hiding this 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).
| 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(); |
There was a problem hiding this 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.
| @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)") |
There was a problem hiding this comment.
Typo in parameter description: "keyloack" should be "keycloak".
| @Parameter(name = ApiConstants.AUTHORIZE_URL, type = CommandType.STRING, description = "Authorize URL for OAuth initialization (only required for keyloack provider)") | |
| @Parameter(name = ApiConstants.AUTHORIZE_URL, type = CommandType.STRING, description = "Authorize URL for OAuth initialization (only required for keycloak provider)") |
| 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)); |
There was a problem hiding this 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.
| 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"); |
There was a problem hiding this 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.
| :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" /> |
There was a problem hiding this comment.
The new Keycloak login button adds an 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).
| <img src="/assets/keycloak.svg" style="width: 32px; padding: 5px" /> | |
| <img src="/assets/keycloak.svg" alt="" style="width: 32px; padding: 5px" /> |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17513 |
|
thanks for the contribution @tazouxme , looks useful. Please have a look at co-pilot’s comments and the pre-commit and license check failures. |
|
Looks very useful feature. Thanks for Contributing @tazouxme Will test it out, I had previously tested saml authentication with keycloak https://kiranchavala.in/blog/cloudstack-integration-with-keycloak/ |
Description
Add Keycloak as a new OAuth provider. This PR adds two new fields in the UI and in the DB:
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Screenshots (if appropriate):
How Has This Been Tested?
Manual testing using local Keycloak instance with basic Realm