Copilot commented on code in PR #12689:
URL: https://github.com/apache/cloudstack/pull/12689#discussion_r2840802280
##########
plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/OauthLoginAPIAuthenticatorCmd.java:
##########
@@ -177,6 +177,10 @@ 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);
+ if (domainIdArr == null) {
+ // Fallback to support clients using the camelCase parameter name
"domainId"
+ domainIdArr = (String[])params.get(ApiConstants.DOMAIN__ID);
+ }
Review Comment:
The new fallback to accept both "domainid" and "domainId" is duplicated here
and in DefaultLoginAPIAuthenticatorCmd. Consider extracting a small shared
helper (e.g., in a common auth/util class) to reduce the chance of future auth
paths missing the same compatibility handling.
##########
server/src/main/java/com/cloud/api/auth/DefaultLoginAPIAuthenticatorCmd.java:
##########
@@ -110,7 +110,11 @@ public String authenticate(String command, Map<String,
Object[]> params, HttpSes
// FIXME: ported from ApiServlet, refactor and cleanup
final String[] username = (String[])params.get(ApiConstants.USERNAME);
final String[] password = (String[])params.get(ApiConstants.PASSWORD);
- final String[] domainIdArr =
(String[])params.get(ApiConstants.DOMAIN_ID);
+ 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);
+ }
Review Comment:
This same dual-parameter lookup logic is now required in multiple
authenticators (e.g., OAuth2 and default login). Consider centralizing the
lookup (e.g., a helper that returns the first non-null entry for DOMAIN_ID /
DOMAIN__ID) to avoid future inconsistencies.
##########
plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/OauthLoginAPIAuthenticatorCmd.java:
##########
@@ -177,6 +177,10 @@ 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);
+ if (domainIdArr == null) {
+ // Fallback to support clients using the camelCase parameter name
"domainId"
+ domainIdArr = (String[])params.get(ApiConstants.DOMAIN__ID);
+ }
Review Comment:
This adds behavior to accept the camelCase "domainId" parameter, but there
doesn't appear to be a unit test covering this path (the existing tests use
ApiConstants.DOMAIN_ID). Please add/extend a test to verify that passing
ApiConstants.DOMAIN__ID is accepted and parsed correctly.
--
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]
