OAuthProvider: add param enabled in register API - #13889
Conversation
this allows to created a provider disabled.
There was a problem hiding this comment.
Pull request overview
This PR extends the OAuth2 provider registration flow to optionally create providers in a disabled state, instead of always enabling them immediately. It wires the new API parameter through the register command into persistence and adds/updates unit tests around the new behavior and response payload.
Changes:
- Add optional
enabledparameter toregisterOauthProviderand pass it through to persistence (defaulting to enabled when omitted). - Update register API response to explicitly set the
enabledflag based on runtime/provider state (consistent with list/update behavior). - Add unit tests covering registering a disabled provider and ensuring the response reflects the disabled state.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/OAuth2AuthManagerImpl.java | Propagates enabled from the register command into provider persistence (defaults to enabled when null). |
| plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/RegisterOAuthProviderCmd.java | Adds the enabled API parameter and sets the response enabled field based on plugin/provider state. |
| plugins/user-authenticators/oauth2/src/test/java/org/apache/cloudstack/oauth2/OAuth2AuthManagerImplTest.java | Adds a unit test verifying a provider can be registered as disabled. |
| plugins/user-authenticators/oauth2/src/test/java/org/apache/cloudstack/oauth2/api/command/RegisterOAuthProviderCmdTest.java | Updates tests to account for enabled handling and adds coverage for disabled response. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #13889 +/- ##
============================================
+ Coverage 19.65% 19.80% +0.15%
- Complexity 19794 20024 +230
============================================
Files 6368 6371 +3
Lines 574889 575968 +1079
Branches 70353 70524 +171
============================================
+ Hits 112985 114064 +1079
+ Misses 449634 449468 -166
- Partials 12270 12436 +166
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:
|
resmo
changed the title
DaanHoogland
left a comment
DaanHoogland
left a comment
There was a problem hiding this comment.
clgtm, are yu doing this for automation purposes, @resmo ?
yes, it is an improvment I noticed during the development of ansible module oauth_provider. |
|
@resmo , is this still draft on purpose? |
DaanHoogland
moved this from Backlog
to conflict/waiting for author
in CloudStack Testing
|
@DaanHoogland ready for merge |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| oauthProviderVO.setAuthorizeUrl(authorizeUrl); | ||
| oauthProviderVO.setTokenUrl(tokenUrl); | ||
| oauthProviderVO.setEnabled(true); | ||
| oauthProviderVO.setEnabled(enabled == null || enabled); |
| authenticatorPluginNames.add(name); | ||
| } | ||
|
|
||
| boolean oauthEnabled = OAuth2AuthManager.isPluginEnabledForDomain(provider.getDomainId()); |
| List<UserOAuth2Authenticator> userOAuth2AuthenticatorPlugins = _oauth2mgr.listUserOAuth2AuthenticationProviders(); | ||
| List<String> authenticatorPluginNames = new ArrayList<>(); | ||
|
|
||
| for (UserOAuth2Authenticator authenticator : userOAuth2AuthenticatorPlugins) { | ||
| String name = authenticator.getName(); | ||
| authenticatorPluginNames.add(name); | ||
| } | ||
|
|
||
| boolean oauthEnabled = OAuth2AuthManager.isPluginEnabledForDomain(provider.getDomainId()); | ||
| if (oauthEnabled && authenticatorPluginNames.contains(provider.getProvider()) && provider.isEnabled()) { | ||
| response.setEnabled(true); | ||
| } else { | ||
| response.setEnabled(false); | ||
| } |
| @Parameter(name = ApiConstants.TOKEN_URL, type = CommandType.STRING, description = "Token URL for OAuth finalization (only required for keycloak provider)") | ||
| private String tokenUrl; | ||
|
|
||
| @Parameter(name = ApiConstants.ENABLED, type = CommandType.BOOLEAN, description = "OAuth provider will be enabled or disabled based on this value", since = "24.0.0") |
| List<UserOAuth2Authenticator> userOAuth2AuthenticatorPlugins = _oauth2mgr.listUserOAuth2AuthenticationProviders(); | ||
| List<String> authenticatorPluginNames = new ArrayList<>(); | ||
|
|
||
| for (UserOAuth2Authenticator authenticator : userOAuth2AuthenticatorPlugins) { | ||
| String name = authenticator.getName(); | ||
| authenticatorPluginNames.add(name); | ||
| } |
Description
Before this change, when registering a new provider, it will always be enabled from the start.
This change allows to pass
enabledto the register process to control whether the new provider should already be enabled after registering.Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?