Forgerock OAuth provider - #13499
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)
|
6cc6acd to
3284edd
Compare
|
@bddvlpr |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #13499 +/- ##
============================================
- Coverage 19.47% 19.47% -0.01%
+ Complexity 19353 19351 -2
============================================
Files 6294 6298 +4
Lines 568214 568407 +193
Branches 69609 69627 +18
============================================
+ Hits 110681 110699 +18
- Misses 445452 445627 +175
Partials 12081 12081
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@blueorangutan package |
|
@DaanHoogland 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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18391 |
DaanHoogland
left a comment
DaanHoogland
left a comment
There was a problem hiding this comment.
clgtm. I don’t think the current smoke test suite makes sense for this PR. manual testing needed.
|
Tested and verified successfully with an actual ForgeRock AM setup. |
There was a problem hiding this comment.
Pull request overview
This PR adds ForgeRock as a supported OAuth2/OIDC identity provider and refactors the existing Keycloak implementation by introducing a shared AbstractOIDCOAuth2Provider base class to avoid duplicated OIDC token-exchange logic.
Changes:
- Add ForgeRock as an OAuth provider across UI, config, and Spring plugin registration.
- Introduce
AbstractOIDCOAuth2Providerand refactorKeycloakOAuth2Providerinto a thin subclass. - Consolidate/replace Keycloak-specific tests with base-class OIDC tests.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/src/views/auth/Login.vue | Adds ForgeRock login button and builds ForgeRock authorize URL from configured provider settings. |
| ui/src/config/section/config.js | Extends provider selection options to include forgerock. |
| ui/public/assets/forgerock.svg | Adds ForgeRock logo asset used by the login button. |
| plugins/user-authenticators/oauth2/src/test/java/org/apache/cloudstack/oauth2/oidc/AbstractOIDCOAuth2ProviderTest.java | Adds unit tests for the shared OIDC provider behavior. |
| plugins/user-authenticators/oauth2/src/test/java/org/apache/cloudstack/oauth2/keycloak/KeycloakOAuth2ProviderTest.java | Removes redundant Keycloak-specific tests in favor of shared OIDC tests. |
| plugins/user-authenticators/oauth2/src/main/resources/META-INF/cloudstack/oauth2/spring-oauth2-context.xml | Registers the ForgeRock provider bean and updates default provider ordering. |
| plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/oidc/AbstractOIDCOAuth2Provider.java | Introduces shared OIDC token exchange / ID token parsing logic. |
| plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/keycloak/KeycloakOAuth2Provider.java | Refactors Keycloak provider to extend the shared OIDC base. |
| plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/forgerock/ForgeRockOAuth2Provider.java | Adds ForgeRock provider implementation (thin OIDC subclass). |
| plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/RegisterOAuthProviderCmd.java | Updates registration validation/messages for OIDC providers (Keycloak + ForgeRock). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| public abstract class AbstractOIDCOAuth2Provider extends AdapterBase implements UserOAuth2Authenticator { | ||
|
|
||
| protected String idToken = null; |
| throw new CloudAuthenticationException(String.format("%s provider is not registered, so user cannot be verified", getName())); | ||
| } | ||
|
|
||
| if (StringUtils.isBlank(idToken)) { |
| String idTokenAsString = fetchedIdToken.getAsString(); | ||
| validateIdToken(idTokenAsString, provider); | ||
|
|
||
| this.idToken = idTokenAsString; |
| protected void clearIdToken() { | ||
| idToken = null; | ||
| } |
| const options = { | ||
| redirect_uri: this.forgerockredirecturi, | ||
| client_id: this.forgerockclientid, | ||
| response_type: 'code', | ||
| scope: 'openid email', | ||
| state: 'cloudstack' | ||
| } |
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
Abstracted the shared OIDC token exchange into a new AbstractOIDCAuth2PRovider base class.
3284edd to
4408f2c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
plugins/user-authenticators/oauth2/src/test/java/org/apache/cloudstack/oauth2/oidc/AbstractOIDCOAuth2ProviderTest.java:120
- idTokenWith() uses String.getBytes() without an explicit charset, which is platform-dependent. Use UTF-8 explicitly to keep the test deterministic across environments.
| String verifiedEmail = verifySecretCodeAndFetchEmail(secretCode, domainId); | ||
| if (StringUtils.isBlank(verifiedEmail) || !email.equals(verifiedEmail)) { | ||
| throw new CloudRuntimeException("Unable to verify the email address with the provided secret"); | ||
| } | ||
| clearIdToken(); | ||
|
|
||
| return true; | ||
| } |
| private void validateIdToken(String idTokenStr, OauthProviderVO provider) { | ||
| JwsJwtCompactConsumer jwtConsumer = new JwsJwtCompactConsumer(idTokenStr); | ||
| JwtClaims claims = jwtConsumer.getJwtToken().getClaims(); | ||
|
|
||
| if (!claims.getAudiences().contains(provider.getClientId())) { |
| redirect_uri: redirectUri, | ||
| client_id: clientId, | ||
| response_type: 'code', | ||
| scope: 'openid email', | ||
| state: 'cloudstack' | ||
| } |
|
@blueorangutan package |
|
@Damans227 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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18830 |
Damans227
left a comment
Damans227
left a comment
There was a problem hiding this comment.
@bddvlpr Thanks for this, and also for calling out the duplication with Keycloak instead of just copying it silently. That instinct is right, appreciate it.
The refactor itself looks sound. AbstractOIDCOAuth2Provider is a faithful extraction of the existing Keycloak logic.
But as you already flagged the real issue in the description:
Perhaps in the future this should be handled as an unbound provider (just a generic OIDC provider, pluggable with any OIDC-compliant server), but for now, this'll do.
I've opened #13854 to track that.
@DaanHoogland @weizhouapache @harikrishna-patnala Happy to see this land if the project wants ForgeRock support today, but I'd treat it as the last vendor-specific OIDC PR we accept, not a template for the next one.
If ForgeRock isn't urgently needed right now though, might be worth a maintainer discussion on redirecting this PR toward the generic design directly instead, since the abstraction is already half built.
There was a problem hiding this comment.
@bddvlpr Also, your screenshots need updating. This PR was opened on June 26, before #12702 (per-domain OAuth support) merged on July 9. That PR changed the login UI, the OAuth configuration UI, and some of the underlying functionality (domain scoped providers, global fallback, etc). Please rebase this PR against main so it picks up those changes.
While testing on a rebased build, I noticed the login page gets a bit clumsy once multiple providers are enabled. See attached screenshots, there's a spacing issue around the buttons.
Screen 1:
Screen 2:
|
Went ahead and tested this against a Keycloak instance and drove the actual API flow (
|
Description
This PR adds ForgeRock as a supported OAuth2 identity provider. While writing it up I noticed that the new Keycloak provider (committed 4 days ago) would have been a line-for-line copy of this OIDC pipeline. Rather than duplicating it I've abstracted the process into a shared OIDC provider and reduced both the new Keycloak and ForgeRock providers to thin subclasses. \
Perhaps in the future this should be handled as an unbound provider (just a generic OIDC provider, pluggable with any OIDC-compliant server), but for now, this'll do.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
Currently only tested against a mock setup, hence why this is a draft. Moved (and merged) the keycloak test into the abstraction layer test.
How did you try to break this feature and the system with this change?