Copilot commented on code in PR #13818:
URL: https://github.com/apache/cloudstack/pull/13818#discussion_r3969422918
##########
server/src/main/java/org/apache/cloudstack/network/ssl/CertServiceImpl.java:
##########
@@ -199,16 +200,44 @@ public List<SslCertResponse> listSslCerts(final
ListSslCertsCmd listSslCertCmd)
final Account caller = ctx.getCallingAccount();
final Long certId = listSslCertCmd.getCertId();
- final Long accountId = listSslCertCmd.getAccountId();
final Long lbRuleId = listSslCertCmd.getLbId();
final Long projectId = listSslCertCmd.getProjectId();
+ final Long accountId = listSslCertCmd.getAccountId();
+ final String accountName = listSslCertCmd.getAccountName();
+ final Long domainId = listSslCertCmd.getDomainId();
- final List<SslCertResponse> certResponseList = new
ArrayList<SslCertResponse>();
+ if (accountId != null && (StringUtils.isNotBlank(accountName) ||
domainId != null)) {
+ throw new InvalidParameterValueException("The accountid and
account/domainid are mutually exclusive");
Review Comment:
This error message is ambiguous/inconsistent with the actual API parameter
names (`accountid`, `account`, `domainid`). Consider clarifying it to
explicitly state the supported combinations, e.g. that `accountid` cannot be
used together with `account` and/or `domainid` (and ideally that `account`
requires `domainid`).
##########
server/src/main/java/org/apache/cloudstack/network/ssl/CertServiceImpl.java:
##########
@@ -199,16 +200,44 @@ public List<SslCertResponse> listSslCerts(final
ListSslCertsCmd listSslCertCmd)
final Account caller = ctx.getCallingAccount();
final Long certId = listSslCertCmd.getCertId();
- final Long accountId = listSslCertCmd.getAccountId();
final Long lbRuleId = listSslCertCmd.getLbId();
final Long projectId = listSslCertCmd.getProjectId();
+ final Long accountId = listSslCertCmd.getAccountId();
+ final String accountName = listSslCertCmd.getAccountName();
+ final Long domainId = listSslCertCmd.getDomainId();
- final List<SslCertResponse> certResponseList = new
ArrayList<SslCertResponse>();
+ if (accountId != null && (StringUtils.isNotBlank(accountName) ||
domainId != null)) {
+ throw new InvalidParameterValueException("The accountid and
account/domainid are mutually exclusive");
+ }
- if (certId == null && accountId == null && lbRuleId == null &&
projectId == null) {
- throw new InvalidParameterValueException("Invalid parameters
either certificate ID or Account ID or Loadbalancer ID or Project ID required");
+ // Validate that only one of certid, lbid, projectid, or
accountid/account can be specified
+ ArrayList<Object> params = new ArrayList<>();
+ params.add(certId);
+ params.add(accountId != null ? accountId : accountName);
+ params.add(lbRuleId);
+ params.add(projectId);
+
+ int nonNullIds = 0;
+ for (Object param : params) {
+ if (param != null) {
+ nonNullIds++;
+ }
+ }
+ if (nonNullIds > 1) {
+ throw new InvalidParameterValueException("Only one of certid,
lbid, projectid, or accountid/account can be specified");
}
Review Comment:
The ‘only one filter’ validation currently treats an empty `accountName`
(e.g. `account=`) as ‘specified’ because it only checks `param != null`. This
can incorrectly reject otherwise valid requests. Also, `domainId` is not part
of the `params` list, so requests that pass only `domainId` are not
counted/validated and will silently fall back to the caller. Consider (1)
counting `accountName` only when `StringUtils.isNotBlank(accountName)`, (2)
explicitly validating `accountName` and `domainId` must be provided together
(reject `domainId` without `accountName`, and vice versa), and (3) applying the
‘only one filter’ rule to the effective account filter (`accountId` or
(`accountName`+`domainId`)).
##########
server/src/test/java/org/apache/cloudstack/network/ssl/CertServiceTest.java:
##########
@@ -819,6 +820,25 @@ public void runDeleteSslCertInvalidId() throws
NoSuchFieldException, IllegalAcce
}
+ @Test
+ public void runListSslCertsUsesCallerAccountWhenNoFilters() {
+ final long callerAccountId = 42L;
+ final CertServiceImpl certService = new CertServiceImpl();
+
+ certService._sslCertDao = Mockito.mock(SslCertDao.class);
+
when(certService._sslCertDao.listByAccountId(anyLong())).thenReturn(new
ArrayList<>());
+
+ final AccountVO callerAccount = new AccountVO("testaccount", 1,
"networkdomain", Account.Type.NORMAL, UUID.randomUUID().toString());
+ callerAccount.setId(callerAccountId);
+ final UserVO user = new UserVO(1, "testuser", "password", "firstname",
"lastName", "email", "timezone", UUID.randomUUID().toString(),
User.Source.UNKNOWN);
+ CallContext.unregister();
+ CallContext.register(user, callerAccount);
+
+ certService.listSslCerts(new ListSslCertsCmdExtn());
+
+
Mockito.verify(certService._sslCertDao).listByAccountId(callerAccountId);
Review Comment:
The test registers a `CallContext` but doesn’t unregister it at the end.
Since `CallContext` is a thread-local/static context, this can leak into
subsequent tests when run in the same thread. Wrap the registration in a
try/finally (or add teardown) to ensure `CallContext.unregister()` is always
called after the assertions.
--
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]
