Copilot commented on code in PR #13889:
URL: https://github.com/apache/cloudstack/pull/13889#discussion_r3986185713
##########
plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/OAuth2AuthManagerImpl.java:
##########
@@ -282,7 +283,7 @@ private OauthProviderVO saveOauthProvider(String provider,
String description, S
oauthProviderVO.setDomainId(domainId);
oauthProviderVO.setAuthorizeUrl(authorizeUrl);
oauthProviderVO.setTokenUrl(tokenUrl);
- oauthProviderVO.setEnabled(true);
+ oauthProviderVO.setEnabled(enabled);
Review Comment:
`enabled` is nullable here, but `OauthProviderVO.setEnabled` takes a
primitive boolean. The existing `registerOauthProvider` tests leave
`cmd.getEnabled()` unstubbed, so Mockito returns null and this call unboxes it
to a `NullPointerException`; the manager should also preserve the legacy
enabled-by-default behavior independently of the command implementation.
Normalize null to true before setting the VO.
##########
plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/RegisterOAuthProviderCmd.java:
##########
@@ -76,6 +79,9 @@ public class RegisterOAuthProviderCmd extends BaseCmd {
@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,
defaults to true if not specified", since = "24.0.0")
Review Comment:
The API annotation uses `24.0.0`, but the surrounding CloudStack API
annotations use the `4.x.0` format (for example, the domain parameters above
use `4.23.0`). This publishes an invalid/misleading introduction version in the
generated API documentation.
##########
plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/RegisterOAuthProviderCmd.java:
##########
@@ -121,6 +127,13 @@ public String getTokenUrl() {
return tokenUrl;
}
+ public Boolean getEnabled() {
+ if (enabled == null) {
+ return true; // default to enabled if not specified
+ }
+ return enabled;
Review Comment:
The new defaulting branch is not covered: the command tests mock the manager
and the manager tests mock `RegisterOAuthProviderCmd`, so none exercises an
actual command with `enabled` omitted and verifies that `getEnabled()` returns
true. Add a focused test for the omitted-parameter path so a future change
cannot regress the non-breaking default.
--
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]
