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.90% +0.24%
- Complexity 19794 20148 +354
============================================
Files 6368 6371 +3
Lines 574889 576845 +1956
Branches 70353 70630 +277
============================================
+ Hits 112985 114795 +1810
+ Misses 449634 449504 -130
- Partials 12270 12546 +276
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); |
|
@resmo some minor code changes. otherwise the PR looks good. |
|
@vishesh92 ignore my previous comment, you're right. we can default early and we should. I change the code accordently. |
There was a problem hiding this comment.
🟡 Changes recommended
Fix the nullable enabled handling and address the remaining API-version and default-path test findings.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Lite
0ebad77 to
db9e8b3
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
One or more issues must be addressed before approval.
Review details
Suppressed comments (3)
plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/OAuth2AuthManagerImpl.java:286
enabledis a nullableBoolean, butOauthProviderVO#setEnabledaccepts primitiveboolean, so this call auto-unboxes null and throws aNullPointerExceptionwhenever the command returns null (the existing Mockito-based registration tests do this, and it represents an omitted value). Normalize null to the documented default oftruebefore persisting, while preserving an explicitfalse.
oauthProviderVO.setEnabled(enabled);
plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/RegisterOAuthProviderCmd.java:134
- The new null-default branch is not covered by the added tests: the command tests pass a mocked manager, while the manager tests mock
RegisterOAuthProviderCmdand never verify that omittingenabledpersiststrue. Add a regression test for an unset parameter so the backward-compatible default cannot silently change.
if (enabled == null) {
enabled = true; // default to enabled if not specified
}
return enabled;
plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/RegisterOAuthProviderCmd.java:82
- The new API metadata uses
since = "24.0.0", but this repository is on4.23.0.0-SNAPSHOTand the adjacent OAuth API parameters use the4.23.0format (see lines 69 and 73). This records an invalid CloudStack release for the parameter in generated API documentation; use the intended4.xrelease value instead.
@Parameter(name = ApiConstants.ENABLED, type = CommandType.BOOLEAN, description = "OAuth provider will be enabled or disabled based on this value, defaults to true if not specified", since = "24.0.0")
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
A critical null-list registration failure and an incorrect API introduction version remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/RegisterOAuthProviderCmd.java:82
- The
sincevalue is using24.0.0, while this repository is on the4.23.0release line and the surrounding OAuth API metadata uses4.23.0. API discovery publishes this annotation value verbatim, so the new parameter will be documented as introduced in the wrong/nonexistent release; please use the actual4.xtarget version.
@Parameter(name = ApiConstants.ENABLED, type = CommandType.BOOLEAN, description = "OAuth provider will be enabled or disabled based on this value, defaults to true if not specified", since = "24.0.0")
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
| 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?