Copilot commented on code in PR #12585:
URL: https://github.com/apache/cloudstack/pull/12585#discussion_r2821717881
##########
server/src/main/java/com/cloud/api/auth/DefaultLoginAPIAuthenticatorCmd.java:
##########
@@ -107,17 +106,13 @@ public String authenticate(String command, Map<String,
Object[]> params, HttpSes
if (HTTPMethod.valueOf(req.getMethod()) != HTTPMethod.POST) {
throw new ServerApiException(ApiErrorCode.METHOD_NOT_ALLOWED,
"Please use HTTP POST to authenticate using this API");
}
+
// FIXME: ported from ApiServlet, refactor and cleanup
final String[] username = (String[])params.get(ApiConstants.USERNAME);
final String[] password = (String[])params.get(ApiConstants.PASSWORD);
- String[] domainIdArr = (String[])params.get(ApiConstants.DOMAIN_ID);
-
- if (domainIdArr == null) {
- domainIdArr = (String[])params.get(ApiConstants.DOMAIN__ID);
- }
- final String[] domainName = (String[])params.get(ApiConstants.DOMAIN);
+ final String[] domainIdArr =
(String[])params.get(ApiConstants.DOMAIN_ID);
Review Comment:
`domainId` (ApiConstants.DOMAIN__ID) is still an accepted/annotated
parameter name for login, but this code now only reads `domainid`
(ApiConstants.DOMAIN_ID). This is a backward-incompatible change: requests
sending `domainId` will no longer resolve the domain and may fail
authentication. Please restore the fallback to also read
ApiConstants.DOMAIN__ID (or normalize parameter keys in one place).
```suggestion
String[] domainIdArr = (String[])params.get(ApiConstants.DOMAIN_ID);
if (domainIdArr == null || domainIdArr.length == 0) {
domainIdArr = (String[])params.get(ApiConstants.DOMAIN__ID);
}
```
##########
server/src/main/java/com/cloud/api/auth/DefaultForgotPasswordAPIAuthenticatorCmd.java:
##########
@@ -108,10 +108,12 @@ public String authenticate(String command, Map<String,
Object[]> params, HttpSes
if (userDomain != null) {
domainId = userDomain.getId();
} else {
+ logger.debug("Unable to find the domain from the path {}",
domain);
throw new ServerApiException(ApiErrorCode.PARAM_ERROR,
String.format("Unable to find the domain from the path %s", domain));
}
Review Comment:
The parameter description says that if no domain is passed, ROOT (/) is
assumed, but this implementation calls
`_domainService.findDomainByPath(domain)` and throws when `domain` is
null/blank. Please default to ROOT when domain is missing (e.g., use
`findDomainByIdOrPath(null, domain)` or explicitly set domainId to
`Domain.ROOT_DOMAIN`).
##########
plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/OauthLoginAPIAuthenticatorCmd.java:
##########
@@ -177,12 +177,8 @@ private String doOauthAuthentication(HttpSession session,
Long domainId, String
protected Long getDomainIdFromParams(Map<String, Object[]> params,
StringBuilder auditTrailSb, String responseType) {
String[] domainIdArr = (String[])params.get(ApiConstants.DOMAIN_ID);
Review Comment:
`getDomainIdFromParams` now only checks ApiConstants.DOMAIN_ID (`domainid`)
but this API command also declares/accepts ApiConstants.DOMAIN__ID
(`domainId`). Removing the fallback breaks clients using `domainId`. Please
handle both keys (or centralize key normalization) to keep compatibility.
```suggestion
String[] domainIdArr = (String[])params.get(ApiConstants.DOMAIN_ID);
if (domainIdArr == null) {
// Fallback to support clients using the camelCase parameter
name "domainId"
domainIdArr = (String[])params.get(ApiConstants.DOMAIN__ID);
}
```
--
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]
