From 725432d8b6425e02c7f2af34c8eeeabfcd7f9828 Mon Sep 17 00:00:00 2001 From: Manoj Kumar Date: Thu, 9 Nov 2023 20:16:05 +0530 Subject: [PATCH 1/3] Introduce domainid and account parameter in createTemplate API --- .../user/template/CreateTemplateCmd.java | 63 +++++++++++++------ .../user/template/CreateTemplateCmdTest.java | 8 +++ 2 files changed, 51 insertions(+), 20 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/template/CreateTemplateCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/template/CreateTemplateCmd.java index 73a6155c8c58..095a2c34e91c 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/template/CreateTemplateCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/template/CreateTemplateCmd.java @@ -21,18 +21,6 @@ import java.util.Map; import org.apache.cloudstack.acl.SecurityChecker; -import org.apache.cloudstack.api.command.user.UserCmd; -import org.apache.cloudstack.api.response.GuestOSResponse; -import org.apache.cloudstack.api.response.SnapshotResponse; -import org.apache.cloudstack.api.response.TemplateResponse; -import org.apache.cloudstack.api.response.UserVmResponse; -import org.apache.cloudstack.api.response.VolumeResponse; -import org.apache.cloudstack.api.response.ProjectResponse; - -import org.apache.cloudstack.api.response.ZoneResponse; -import org.apache.commons.lang3.StringUtils; -import org.apache.log4j.Logger; - import org.apache.cloudstack.api.APICommand; import org.apache.cloudstack.api.ApiCommandResourceType; import org.apache.cloudstack.api.ApiConstants; @@ -41,7 +29,18 @@ import org.apache.cloudstack.api.Parameter; import org.apache.cloudstack.api.ResponseObject.ResponseView; import org.apache.cloudstack.api.ServerApiException; +import org.apache.cloudstack.api.command.user.UserCmd; +import org.apache.cloudstack.api.response.DomainResponse; +import org.apache.cloudstack.api.response.GuestOSResponse; +import org.apache.cloudstack.api.response.ProjectResponse; +import org.apache.cloudstack.api.response.SnapshotResponse; +import org.apache.cloudstack.api.response.TemplateResponse; +import org.apache.cloudstack.api.response.UserVmResponse; +import org.apache.cloudstack.api.response.VolumeResponse; +import org.apache.cloudstack.api.response.ZoneResponse; import org.apache.cloudstack.context.CallContext; +import org.apache.commons.lang3.StringUtils; +import org.apache.log4j.Logger; import com.cloud.event.EventTypes; import com.cloud.exception.InvalidParameterValueException; @@ -139,6 +138,15 @@ public class CreateTemplateCmd extends BaseAsyncCreateCmd implements UserCmd { @Parameter(name = ApiConstants.ZONE_ID, type = CommandType.UUID, entityType = ZoneResponse.class, description = "the zone for the template. Can be specified with snapshot only", since = "4.19.0") private Long zoneId; + @Parameter(name = ApiConstants.DOMAIN_ID, + type = CommandType.UUID, + entityType = DomainResponse.class, + description = "an optional domainId. If the account parameter is used, domainId must also be used.") + private Long domainId; + + @Parameter(name = ApiConstants.ACCOUNT, type = CommandType.STRING, description = "an optional accountName. Must be used with domainId.") + private String accountName; + // /////////////////////////////////////////////////// // ///////////////// Accessors /////////////////////// // /////////////////////////////////////////////////// @@ -217,6 +225,14 @@ public Long getZoneId() { return zoneId; } + public Long getDomainId() { + return domainId; + } + + public String getAccountName() { + return accountName; + } + // /////////////////////////////////////////////////// // ///////////// API Implementation/////////////////// // /////////////////////////////////////////////////// @@ -251,17 +267,25 @@ public long getEntityOwnerId() { } } - if(projectId != null){ + Account accountToUse = callingAccount; + if (accountName != null && domainId != null) { + try { + accountToUse = _accountService.getAccount(_accountService.finalyzeAccountId(accountName, domainId, projectId, true)); + + } catch (Exception exception) { + s_logger.info("Unable to find accountId associated with accountName=" + accountName + " and domainId=" + + domainId + " using account=" + accountToUse); + } + } else if (projectId != null) { final Project project = _projectService.getProject(projectId); if (project != null) { if (project.getState() == Project.State.Active) { - Account projectAccount= _accountService.getAccount(project.getProjectAccountId()); - _accountService.checkAccess(callingAccount, SecurityChecker.AccessType.UseEntry, false, projectAccount); - return project.getProjectAccountId(); + accountToUse = _accountService.getAccount(project.getProjectAccountId()); + _accountService.checkAccess(callingAccount, SecurityChecker.AccessType.UseEntry, false, accountToUse); } else { final PermissionDeniedException ex = - new PermissionDeniedException("Can't add resources to the project with specified projectId in state=" + project.getState() + - " as it's no longer active"); + new PermissionDeniedException("Can't add resources to the project with specified projectId in state=" + project.getState() + + " as it's no longer active"); ex.addProxyObject(project.getUuid(), "projectId"); throw ex; } @@ -269,8 +293,7 @@ public long getEntityOwnerId() { throw new InvalidParameterValueException("Unable to find project by id"); } } - - return callingAccount.getId(); + return accountToUse.getId(); } @Override diff --git a/api/src/test/java/org/apache/cloudstack/api/command/user/template/CreateTemplateCmdTest.java b/api/src/test/java/org/apache/cloudstack/api/command/user/template/CreateTemplateCmdTest.java index d8af670a7b21..3bd92bae25ed 100644 --- a/api/src/test/java/org/apache/cloudstack/api/command/user/template/CreateTemplateCmdTest.java +++ b/api/src/test/java/org/apache/cloudstack/api/command/user/template/CreateTemplateCmdTest.java @@ -29,4 +29,12 @@ public void testGetZoneId() { ReflectionTestUtils.setField(cmd, "zoneId", id); Assert.assertEquals(id, cmd.getZoneId()); } + + @Test + public void testDomainId() { + final CreateTemplateCmd cmd = new CreateTemplateCmd(); + Long id = 2L; + ReflectionTestUtils.setField(cmd, "domainId", id); + Assert.assertEquals(id, cmd.getDomainId()); + } } From 3c437fff023c3d59c07a1fd2a04a50a51b5e0249 Mon Sep 17 00:00:00 2001 From: Manoj Kumar Date: Fri, 10 Nov 2023 14:33:26 +0530 Subject: [PATCH 2/3] Modified PR based on review comments, following changes are made: 1. Created separate methods for access check and obtaining accountId 2. Included since property in params 3. Catching specific exceptions instead of all 4. Modified exception log to warn, included debug log 5. Removed projectId block as it's taken care in _accountService.finalyzeAccountId method --- .../user/template/CreateTemplateCmd.java | 102 +++++++++--------- .../user/template/CreateTemplateCmdTest.java | 8 ++ 2 files changed, 62 insertions(+), 48 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/template/CreateTemplateCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/template/CreateTemplateCmd.java index 095a2c34e91c..78a2ee3c8cff 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/template/CreateTemplateCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/template/CreateTemplateCmd.java @@ -46,7 +46,6 @@ import com.cloud.exception.InvalidParameterValueException; import com.cloud.exception.PermissionDeniedException; import com.cloud.exception.ResourceAllocationException; -import com.cloud.projects.Project; import com.cloud.storage.Snapshot; import com.cloud.storage.Volume; import com.cloud.template.VirtualMachineTemplate; @@ -141,10 +140,14 @@ public class CreateTemplateCmd extends BaseAsyncCreateCmd implements UserCmd { @Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.UUID, entityType = DomainResponse.class, - description = "an optional domainId. If the account parameter is used, domainId must also be used.") + description = "an optional domainId. If the account parameter is used, domainId must also be used.", + since = "4.19.0") private Long domainId; - @Parameter(name = ApiConstants.ACCOUNT, type = CommandType.STRING, description = "an optional accountName. Must be used with domainId.") + @Parameter(name = ApiConstants.ACCOUNT, + type = CommandType.STRING, + description = "an optional accountName. Must be used with domainId.", + since = "4.19.0") private String accountName; // /////////////////////////////////////////////////// @@ -248,54 +251,14 @@ public static String getResultObjectName() { @Override public long getEntityOwnerId() { - Long volumeId = getVolumeId(); - Long snapshotId = getSnapshotId(); Account callingAccount = CallContext.current().getCallingAccount(); - if (volumeId != null) { - Volume volume = _entityMgr.findById(Volume.class, volumeId); - if (volume != null) { - _accountService.checkAccess(callingAccount, SecurityChecker.AccessType.UseEntry, false, volume); - } else { - throw new InvalidParameterValueException("Unable to find volume by id=" + volumeId); - } - } else { - Snapshot snapshot = _entityMgr.findById(Snapshot.class, snapshotId); - if (snapshot != null) { - _accountService.checkAccess(callingAccount, SecurityChecker.AccessType.UseEntry, false, snapshot); - } else { - throw new InvalidParameterValueException("Unable to find snapshot by id=" + snapshotId); - } - } - - Account accountToUse = callingAccount; - if (accountName != null && domainId != null) { - try { - accountToUse = _accountService.getAccount(_accountService.finalyzeAccountId(accountName, domainId, projectId, true)); - - } catch (Exception exception) { - s_logger.info("Unable to find accountId associated with accountName=" + accountName + " and domainId=" - + domainId + " using account=" + accountToUse); - } - } else if (projectId != null) { - final Project project = _projectService.getProject(projectId); - if (project != null) { - if (project.getState() == Project.State.Active) { - accountToUse = _accountService.getAccount(project.getProjectAccountId()); - _accountService.checkAccess(callingAccount, SecurityChecker.AccessType.UseEntry, false, accountToUse); - } else { - final PermissionDeniedException ex = - new PermissionDeniedException("Can't add resources to the project with specified projectId in state=" + project.getState() + - " as it's no longer active"); - ex.addProxyObject(project.getUuid(), "projectId"); - throw ex; - } - } else { - throw new InvalidParameterValueException("Unable to find project by id"); - } - } - return accountToUse.getId(); + // Perform access check on volume/snapshot for callingAccount + ensureAccessCheck(callingAccount); + // Obtain accountIdToUse using (account and domainId) | projectId + return findAccountIdToUse(callingAccount); } + @Override public String getEventType() { return EventTypes.EVENT_TEMPLATE_CREATE; @@ -353,4 +316,47 @@ public void execute() { } } + + /*** + * Performs access check on volume and snapshot for given account + * @param account + */ + private void ensureAccessCheck(Account account) { + if (volumeId != null) { + Volume volume = _entityMgr.findById(Volume.class, volumeId); + if (volume != null) { + _accountService.checkAccess(account, SecurityChecker.AccessType.UseEntry, false, volume); + } else { + throw new InvalidParameterValueException("Unable to find volume by id=" + volumeId); + } + } else { + Snapshot snapshot = _entityMgr.findById(Snapshot.class, snapshotId); + if (snapshot != null) { + _accountService.checkAccess(account, SecurityChecker.AccessType.UseEntry, false, snapshot); + } else { + throw new InvalidParameterValueException("Unable to find snapshot by id=" + snapshotId); + } + } + } + + /*** + * Find accountId based on accountName and domainId or projectId + * if not found, return callingAccountId for further use + * @param callingAccount + * @return accountId + */ + private Long findAccountIdToUse(Account callingAccount) { + Long accountIdToUse = null; + try { + accountIdToUse = _accountService.finalyzeAccountId(accountName, domainId, projectId, true); + } catch (InvalidParameterValueException | PermissionDeniedException ex) { + if (s_logger.isDebugEnabled()) { + s_logger.debug(String.format("An exception occurred while finalizing account id with accountName, domainId and projectId" + + "using callingAccountId=%s", callingAccount.getUuid()), ex); + } + s_logger.warn("Unable to find accountId associated with accountName=" + accountName + " and domainId=" + + domainId + " or projectId=" + projectId + ", using callingAccountId=" + callingAccount.getUuid()); + } + return accountIdToUse != null ? accountIdToUse : callingAccount.getAccountId(); + } } diff --git a/api/src/test/java/org/apache/cloudstack/api/command/user/template/CreateTemplateCmdTest.java b/api/src/test/java/org/apache/cloudstack/api/command/user/template/CreateTemplateCmdTest.java index 3bd92bae25ed..55e9c8dba1ef 100644 --- a/api/src/test/java/org/apache/cloudstack/api/command/user/template/CreateTemplateCmdTest.java +++ b/api/src/test/java/org/apache/cloudstack/api/command/user/template/CreateTemplateCmdTest.java @@ -37,4 +37,12 @@ public void testDomainId() { ReflectionTestUtils.setField(cmd, "domainId", id); Assert.assertEquals(id, cmd.getDomainId()); } + + @Test + public void testGetAccountName() { + final CreateTemplateCmd cmd = new CreateTemplateCmd(); + String accountName = "user1"; + ReflectionTestUtils.setField(cmd, "accountName", accountName); + Assert.assertEquals(accountName, cmd.getAccountName()); + } } From 4732ed7a4e91ef41a64499815d81497b5cdfc048 Mon Sep 17 00:00:00 2001 From: Manoj Kumar Date: Sat, 11 Nov 2023 09:39:44 +0530 Subject: [PATCH 3/3] Remove redundant comment --- .../cloudstack/api/command/user/template/CreateTemplateCmd.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/template/CreateTemplateCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/template/CreateTemplateCmd.java index 78a2ee3c8cff..6c39ab6d3c7e 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/template/CreateTemplateCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/template/CreateTemplateCmd.java @@ -252,9 +252,7 @@ public static String getResultObjectName() { @Override public long getEntityOwnerId() { Account callingAccount = CallContext.current().getCallingAccount(); - // Perform access check on volume/snapshot for callingAccount ensureAccessCheck(callingAccount); - // Obtain accountIdToUse using (account and domainId) | projectId return findAccountIdToUse(callingAccount); }