Copilot commented on code in PR #13889:
URL: https://github.com/apache/cloudstack/pull/13889#discussion_r3969395658
##########
plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/RegisterOAuthProviderCmd.java:
##########
@@ -151,6 +161,22 @@ public void execute() throws ServerApiException,
ConcurrentOperationException, E
provider.getAuthorizeUrl(), provider.getTokenUrl(), domain);
response.setResponseName(getCommandName());
response.setObjectName(ApiConstants.OAUTH_PROVIDER);
+
+ 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());
Review Comment:
Calling a static `OAuth2AuthManager.isPluginEnabledForDomain(...)` from the
API command makes this behavior harder to test/mocks and can bypass
instance-level configuration logic in the injected `_oauth2mgr`. Prefer using
an instance method on `_oauth2mgr` (the same enablement check used in the
manager implementation) so behavior is consistent and testable.
##########
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 == null || enabled);
Review Comment:
New defaulting behavior is introduced here (`null` -> enabled). There’s a
test for `enabled=false`, but no coverage asserting the default behavior when
`cmd.getEnabled()` is `null` (and optionally `true`). Adding tests for both
cases will prevent regressions in the default-enable contract.
##########
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",
since = "24.0.0")
Review Comment:
The parameter description doesn’t document the default behavior when
`enabled` is omitted (it defaults to enabled per `enabled == null || enabled`).
Consider updating the description to explicitly state the default (e.g.,
'Defaults to true when not provided') to avoid API consumer confusion.
##########
plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/RegisterOAuthProviderCmd.java:
##########
@@ -151,6 +161,22 @@ public void execute() throws ServerApiException,
ConcurrentOperationException, E
provider.getAuthorizeUrl(), provider.getTokenUrl(), domain);
response.setResponseName(getCommandName());
response.setObjectName(ApiConstants.OAUTH_PROVIDER);
+
+ 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);
+ }
Review Comment:
The response `enabled` value is being computed as a composite of (domain
plugin enabled) AND (provider exists in authenticator list) AND
(provider.isEnabled). This can misrepresent the newly introduced `enabled`
registration flag: an explicitly enabled provider may be returned as disabled
if a plugin is temporarily unavailable or disabled for the domain. If the
intent of the new parameter is to control the provider’s persisted enabled
state, the response should reflect `provider.isEnabled()`; if you also need a
computed runtime state, consider exposing a separate field (e.g.,
`active`/`available`) rather than overloading `enabled`.
##########
plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/RegisterOAuthProviderCmd.java:
##########
@@ -151,6 +161,22 @@ public void execute() throws ServerApiException,
ConcurrentOperationException, E
provider.getAuthorizeUrl(), provider.getTokenUrl(), domain);
response.setResponseName(getCommandName());
response.setObjectName(ApiConstants.OAUTH_PROVIDER);
+
+ List<UserOAuth2Authenticator> userOAuth2AuthenticatorPlugins =
_oauth2mgr.listUserOAuth2AuthenticationProviders();
+ List<String> authenticatorPluginNames = new ArrayList<>();
+
+ for (UserOAuth2Authenticator authenticator :
userOAuth2AuthenticatorPlugins) {
+ String name = authenticator.getName();
+ authenticatorPluginNames.add(name);
+ }
Review Comment:
This builds an intermediate `List<String>` just to do a containment check
later. You can simplify by directly checking the provider name against the
authenticators (e.g., iterate once and break on match, or use a stream
anyMatch), which reduces moving parts and makes the intent clearer.
--
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]
