Copilot commented on code in PR #12702:
URL: https://github.com/apache/cloudstack/pull/12702#discussion_r2893693167
##########
plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/dao/OauthProviderDaoImpl.java:
##########
@@ -22,23 +22,47 @@
import com.cloud.utils.db.SearchCriteria;
import org.apache.cloudstack.oauth2.vo.OauthProviderVO;
+import java.util.List;
+import java.util.Objects;
+
public class OauthProviderDaoImpl extends GenericDaoBase<OauthProviderVO,
Long> implements OauthProviderDao {
- private final SearchBuilder<OauthProviderVO> oauthProviderSearchByName;
+ private final SearchBuilder<OauthProviderVO>
oauthProviderSearchByProviderAndDomain;
public OauthProviderDaoImpl() {
super();
- oauthProviderSearchByName = createSearchBuilder();
- oauthProviderSearchByName.and("provider",
oauthProviderSearchByName.entity().getProvider(), SearchCriteria.Op.EQ);
- oauthProviderSearchByName.done();
+ oauthProviderSearchByProviderAndDomain = createSearchBuilder();
+ oauthProviderSearchByProviderAndDomain.and("provider",
oauthProviderSearchByProviderAndDomain.entity().getProvider(),
SearchCriteria.Op.EQ);
+ oauthProviderSearchByProviderAndDomain.and("domainId",
oauthProviderSearchByProviderAndDomain.entity().getDomainId(),
SearchCriteria.Op.EQ);
+ oauthProviderSearchByProviderAndDomain.done();
}
@Override
- public OauthProviderVO findByProvider(String provider) {
- SearchCriteria<OauthProviderVO> sc =
oauthProviderSearchByName.create();
+ public OauthProviderVO findByProviderAndDomain(String provider, Long
domainId) {
+ SearchCriteria<OauthProviderVO> sc =
oauthProviderSearchByProviderAndDomain.create();
sc.setParameters("provider", provider);
-
+ sc.setParameters("domainId", domainId);
return findOneBy(sc);
}
Review Comment:
Using an EQ filter with a null `domainId` typically generates `domain_id =
NULL`, which won’t match global rows (NULL) in SQL. This can break global
provider lookup and allow duplicate global registrations. Consider handling
`domainId == null` with an `IS NULL` criterion (or a separate
SearchBuilder/query path) instead of setting an EQ parameter to null.
##########
engine/schema/src/main/resources/META-INF/db/schema-42210to42300.sql:
##########
@@ -19,6 +19,12 @@
-- Schema upgrade from 4.22.1.0 to 4.23.0.0
--;
+ALTER TABLE `cloud`.`oauth_provider` ADD COLUMN `domain_id` bigint unsigned
DEFAULT NULL COMMENT 'NULL for global provider, domain ID for domain-specific'
AFTER `redirect_uri`;
+ALTER TABLE `cloud`.`oauth_provider` ADD CONSTRAINT
`fk_oauth_provider__domain_id` FOREIGN KEY (`domain_id`) REFERENCES
`domain`(`id`);
+ALTER TABLE `cloud`.`oauth_provider` ADD INDEX
`i_oauth_provider__domain_id`(`domain_id`);
+
+ALTER TABLE `cloud`.`oauth_provider` ADD UNIQUE KEY
`uk_oauth_provider__provider_domain` (`provider`, `domain_id`);
Review Comment:
In MySQL, UNIQUE(provider, domain_id) does not prevent multiple rows where
`domain_id` is NULL (NULLs are treated as distinct), so multiple 'global'
providers with the same `provider` can still be inserted. If global uniqueness
is required, consider a schema approach that normalizes NULL (e.g., a generated
column like `coalesce(domain_id, 0)` with a unique index) or store a non-NULL
sentinel for global and index that.
```suggestion
ALTER TABLE `cloud`.`oauth_provider` ADD COLUMN `domain_id_norm` bigint
unsigned AS (COALESCE(`domain_id`, 0)) STORED, ADD UNIQUE KEY
`uk_oauth_provider__provider_domain` (`provider`, `domain_id_norm`);
```
##########
plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/ListOAuthProvidersCmd.java:
##########
@@ -59,6 +63,14 @@ public class ListOAuthProvidersCmd extends BaseListCmd
implements APIAuthenticat
@Parameter(name = ApiConstants.PROVIDER, type = CommandType.STRING,
description = "Name of the provider")
private String provider;
+ @Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.UUID,
entityType = DomainResponse.class,
+ description = "List OAuth providers for a specific domain. Use -1
for global providers only.")
Review Comment:
The parameter is declared as `CommandType.UUID` but the description
instructs using `-1`, which is not a UUID and may be rejected by request
validation/parsing. Consider modeling the global-only filter via a separate
boolean/string parameter (or making this parameter a STRING) rather than
overloading a UUID-typed field with `-1`.
```suggestion
description = "List OAuth providers for a specific domain.")
```
##########
ui/src/views/auth/Login.vue:
##########
@@ -150,9 +150,65 @@
</a-select>
</a-form-item>
</a-tab-pane>
+ <a-tab-pane key="oauth" :disabled="!socialLogin">
+ <template #tab>
+ <span>
+ <img src="/assets/github.svg" style="width: 16px; vertical-align:
middle" />
+ <img src="/assets/google.svg" style="width: 16px; vertical-align:
middle" />
+ External
+ </span>
+ </template>
+ <a-form-item name="oauthDomain">
+ <a-input
+ size="large"
+ type="text"
+ :placeholder="$t('label.domain')"
+ v-model:value="form.oauthDomain"
+ @pressEnter="handleOauthDomainSubmit"
+ @blur="handleOauthDomainSubmit"
+ >
+ <template #prefix>
+ <project-outlined />
+ </template>
+ </a-input>
+ </a-form-item>
+ <div class="center" v-if="oauthGithubProvider || oauthGoogleProvider">
+ <div class="social-auth" v-if="oauthGithubProvider">
+ <a-button
+ @click="handleGithubProviderAndDomain"
+ tag="a"
+ color="primary"
+ :href="getGitHubUrl(from)"
+ class="auth-btn github-auth"
+ style="height: 38px; width: 185px; padding: 0; margin-bottom:
5px;" >
+ <img src="/assets/github.svg" style="width: 32px; padding: 5px"
/>
Review Comment:
The `<img>` elements are missing `alt` text (or `aria-hidden=\"true\"` if
decorative). This can reduce screen-reader usability. Add appropriate `alt`
attributes (e.g., 'GitHub'/'Google') or mark them decorative if the adjacent
text fully conveys the meaning.
##########
ui/src/views/auth/Login.vue:
##########
@@ -339,6 +357,59 @@ export default {
}
})
},
+ fetchOauthProviders (domain) {
+ const params = {}
+ if (domain) {
+ params.domain = domain
+ }
+ if (domain) {
Review Comment:
The `if (domain)` check is duplicated. Consider consolidating into a single
block so the domain-handling logic is defined once (e.g., set params and
loading together).
```suggestion
```
##########
plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/OAuth2UserAuthenticator.java:
##########
@@ -89,7 +91,7 @@ public String encode(String password) {
return null;
}
- protected boolean isOAuthPluginEnabled() {
- return OAuth2IsPluginEnabled.value();
+ protected boolean isOAuthPluginEnabled(Long domainId) {
+ return
Boolean.TRUE.equals(OAuth2IsPluginEnabled.valueInScope(ConfigKey.Scope.Domain,
domainId, true));
Review Comment:
Passing `strictScope = true` disables fallback to parent/global scope (it
returns null when no domain-scoped value exists). That contradicts the PR goal
of 'global fallback' and can unexpectedly disable OAuth in domains that rely on
global config. Consider using the default `valueInScope(scope, id)` (or
`strictScope = false`) for enablement checks where fallback is desired.
```suggestion
return
Boolean.TRUE.equals(OAuth2IsPluginEnabled.valueInScope(ConfigKey.Scope.Domain,
domainId));
```
##########
plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/ListOAuthProvidersCmd.java:
##########
@@ -118,8 +154,20 @@ public String authenticate(String command, Map<String,
Object[]> params, HttpSes
responses.add(r);
}
+ int totalEnabledCount = responses.size();
+ if (!domainRequested && !isAuthenticated) {
+ List<OauthProviderVO> allProviders =
_oauth2mgr.listOauthProviders(null, null, null);
+ for (OauthProviderVO domainProvider : allProviders) {
+ if (domainProvider.getDomainId() != null &&
domainProvider.isEnabled()
+ &&
Boolean.TRUE.equals(OAuth2AuthManager.OAuth2IsPluginEnabled.valueInScope(ConfigKey.Scope.Domain,
domainProvider.getDomainId(), true))
+ &&
authenticatorPluginNames.contains(domainProvider.getProvider())) {
+ totalEnabledCount++;
+ }
+ }
+ }
+
ListResponse<OauthProviderResponse> response = new ListResponse<>();
- response.setResponses(responses, resultList.size());
+ response.setResponses(responses, totalEnabledCount);
Review Comment:
`ListResponse#setResponses(items, count)` typically expects `count` to
reflect the number of returned items, not the number of enabled providers
across other (non-returned) domains. Returning a `count` larger than
`responses.size()` can break pagination/clients and is semantically confusing.
If the UI needs 'total enabled providers across all domains', consider
returning that as a separate field/metric rather than overloading `count`.
##########
ui/src/views/auth/Login.vue:
##########
@@ -150,9 +150,65 @@
</a-select>
</a-form-item>
</a-tab-pane>
+ <a-tab-pane key="oauth" :disabled="!socialLogin">
+ <template #tab>
+ <span>
+ <img src="/assets/github.svg" style="width: 16px; vertical-align:
middle" />
+ <img src="/assets/google.svg" style="width: 16px; vertical-align:
middle" />
Review Comment:
The `<img>` elements are missing `alt` text (or `aria-hidden=\"true\"` if
decorative). This can reduce screen-reader usability. Add appropriate `alt`
attributes (e.g., 'GitHub'/'Google') or mark them decorative if the adjacent
text fully conveys the meaning.
##########
plugins/user-authenticators/oauth2/src/test/java/org/apache/cloudstack/oauth2/api/command/VerifyOAuthCodeAndGetUserCmdTest.java:
##########
@@ -102,7 +106,11 @@ public void testSetAuthenticators() {
authenticators.add(mock(PluggableAPIAuthenticator.class));
authenticators.add(oauth2mgr);
authenticators.add(null);
- cmd.setAuthenticators(authenticators);
+ try {
+ cmd.setAuthenticators(authenticators);
+ } catch (AssertionError e) {
+ // ComponentContext is not available in unit test environment
+ }
Review Comment:
Catching and ignoring `AssertionError` can mask genuine failures and make
tests flaky/less trustworthy. Prefer refactoring `setAuthenticators` (or the
test setup) so it doesn’t depend on `ComponentContext`/assertions in unit
tests, or explicitly skip the test with an assumption when the required context
isn’t available.
##########
ui/src/views/auth/Login.vue:
##########
@@ -150,9 +150,65 @@
</a-select>
</a-form-item>
</a-tab-pane>
+ <a-tab-pane key="oauth" :disabled="!socialLogin">
+ <template #tab>
+ <span>
+ <img src="/assets/github.svg" style="width: 16px; vertical-align:
middle" />
+ <img src="/assets/google.svg" style="width: 16px; vertical-align:
middle" />
+ External
+ </span>
+ </template>
+ <a-form-item name="oauthDomain">
+ <a-input
+ size="large"
+ type="text"
+ :placeholder="$t('label.domain')"
+ v-model:value="form.oauthDomain"
+ @pressEnter="handleOauthDomainSubmit"
+ @blur="handleOauthDomainSubmit"
+ >
+ <template #prefix>
+ <project-outlined />
+ </template>
+ </a-input>
+ </a-form-item>
+ <div class="center" v-if="oauthGithubProvider || oauthGoogleProvider">
+ <div class="social-auth" v-if="oauthGithubProvider">
+ <a-button
+ @click="handleGithubProviderAndDomain"
+ tag="a"
+ color="primary"
+ :href="getGitHubUrl(from)"
+ class="auth-btn github-auth"
+ style="height: 38px; width: 185px; padding: 0; margin-bottom:
5px;" >
+ <img src="/assets/github.svg" style="width: 32px; padding: 5px"
/>
+ <a-typography-text>Sign in with Github</a-typography-text>
Review Comment:
Use the correct brand capitalization: 'GitHub' (not 'Github').
```suggestion
<a-typography-text>Sign in with GitHub</a-typography-text>
```
##########
ui/src/views/auth/Login.vue:
##########
@@ -150,9 +150,65 @@
</a-select>
</a-form-item>
</a-tab-pane>
+ <a-tab-pane key="oauth" :disabled="!socialLogin">
+ <template #tab>
+ <span>
+ <img src="/assets/github.svg" style="width: 16px; vertical-align:
middle" />
+ <img src="/assets/google.svg" style="width: 16px; vertical-align:
middle" />
+ External
+ </span>
+ </template>
+ <a-form-item name="oauthDomain">
+ <a-input
+ size="large"
+ type="text"
+ :placeholder="$t('label.domain')"
+ v-model:value="form.oauthDomain"
+ @pressEnter="handleOauthDomainSubmit"
+ @blur="handleOauthDomainSubmit"
+ >
+ <template #prefix>
+ <project-outlined />
+ </template>
+ </a-input>
+ </a-form-item>
+ <div class="center" v-if="oauthGithubProvider || oauthGoogleProvider">
+ <div class="social-auth" v-if="oauthGithubProvider">
+ <a-button
+ @click="handleGithubProviderAndDomain"
+ tag="a"
+ color="primary"
+ :href="getGitHubUrl(from)"
+ class="auth-btn github-auth"
+ style="height: 38px; width: 185px; padding: 0; margin-bottom:
5px;" >
+ <img src="/assets/github.svg" style="width: 32px; padding: 5px"
/>
+ <a-typography-text>Sign in with Github</a-typography-text>
+ </a-button>
+ </div>
+ <div class="social-auth" v-if="oauthGoogleProvider">
+ <a-button
+ @click="handleGoogleProviderAndDomain"
+ tag="a"
+ color="primary"
+ :href="getGoogleUrl(from)"
+ class="auth-btn google-auth"
+ style="height: 38px; width: 185px; padding: 0" >
+ <img src="/assets/google.svg" style="width: 32px; padding: 5px"
/>
Review Comment:
The `<img>` elements are missing `alt` text (or `aria-hidden=\"true\"` if
decorative). This can reduce screen-reader usability. Add appropriate `alt`
attributes (e.g., 'GitHub'/'Google') or mark them decorative if the adjacent
text fully conveys the meaning.
##########
plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/OAuth2AuthManagerImpl.java:
##########
@@ -219,6 +236,38 @@ public boolean deleteOauthProvider(Long id) {
return _oauthProviderDao.remove(id);
}
+ @Override
+ public Long resolveDomainId(Map<String, Object[]> params) {
+ final String[] domainIdArray =
(String[])params.get(ApiConstants.DOMAIN_ID);
+ if (ArrayUtils.isNotEmpty(domainIdArray)) {
+ String domainUuid = domainIdArray[0];
+ if (GLOBAL_DOMAIN_FILTER.equals(domainUuid)) {
+ return GLOBAL_DOMAIN_ID;
+ }
+ DomainVO domain = _domainDao.findByUuid(domainUuid);
+ if (Objects.nonNull(domain)) {
+ return domain.getId();
+ }
+ }
+ final String[] domainArray = (String[])params.get(ApiConstants.DOMAIN);
+ if (ArrayUtils.isNotEmpty(domainArray)) {
+ String path = domainArray[0];
+ if (StringUtils.isNotEmpty(path)) {
+ if (!path.startsWith("/")) {
+ path = "/" + path;
+ }
+ if (!path.endsWith("/")) {
+ path += "/";
+ }
+ DomainVO domain = _domainDao.findDomainByPath(path);
+ if (Objects.nonNull(domain)) {
+ return domain.getId();
+ }
+ }
+ }
+ return null;
+ }
Review Comment:
The new `resolveDomainId` logic adds non-trivial behavior (UUID lookup,
special `-1` handling, and domain-path normalization). Adding unit tests for
these branches would help prevent regressions (e.g., path formatting and the
`-1` global filter behavior).
--
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]
