Add tests and UI support and update response params

api/src/main/java/org/apache/cloudstack/api/command/admin/backup/UpdateBackupOfferingCmd.java

@@ -37,6 +37,7 @@ import com.cloud.user.Account;
 import com.cloud.utils.exception.CloudRuntimeException;
 
 import java.util.List;
+import java.util.function.LongFunction;
 
 @APICommand(name = "updateBackupOffering", description = "Updates a backup offering.", responseObject = BackupOfferingResponse.class,
 requestHasSensitiveInfo = false, responseHasSensitiveInfo = false, since = "4.16.0")
@@ -114,7 +115,15 @@ public class UpdateBackupOfferingCmd extends BaseCmd implements DomainAndZoneIdR
     }
 
     public List<Long> getDomainIds() {
-        return resolveDomainIds(domainIds, id, backupManager::getBackupOfferingDomains, "backup offering");
+        // backupManager may be null in unit tests where the command is spied without injection.
+        // Avoid creating a method reference to a null receiver which causes NPE. When backupManager
+        // is null, pass null as the defaultDomainsProvider so resolveDomainIds will simply return
+        // an empty list or parse the explicit domainIds string.
+        LongFunction<List<Long>> defaultDomainsProvider = null;
+        if (backupManager != null) {
+            defaultDomainsProvider = backupManager::getBackupOfferingDomains;
+        }
+        return resolveDomainIds(domainIds, id, defaultDomainsProvider, "backup offering");
     }
 
     @Override

api/src/main/java/org/apache/cloudstack/api/response/BackupOfferingResponse.java

@@ -61,6 +61,14 @@ public class BackupOfferingResponse extends BaseResponse {
     @Param(description = "zone name")
     private String zoneName;
 
+    @SerializedName(ApiConstants.DOMAIN_ID)
+    @Param(description = "the domain ID(s) this disk offering belongs to. Ignore this information as it is not currently applicable.")
+    private String domainId;
+
+    @SerializedName(ApiConstants.DOMAIN)
+    @Param(description = "the domain name(s) this disk offering belongs to. Ignore this information as it is not currently applicable.")
+    private String domain;
+
     @SerializedName(ApiConstants.CROSS_ZONE_INSTANCE_CREATION)
     @Param(description = "the backups with this offering can be used to create Instances on all Zones", since = "4.22.0")
     private Boolean crossZoneInstanceCreation;
@@ -108,4 +116,13 @@ public class BackupOfferingResponse extends BaseResponse {
     public void setCreated(Date created) {
         this.created = created;
     }
+
+    public void setDomainId(String domainId) {
+        this.domainId = domainId;
+    }
+
+    public void setDomain(String domain) {
+        this.domain = domain;
+    }
+
 }

engine/schema/src/main/java/org/apache/cloudstack/backup/dao/BackupOfferingDaoImpl.java

@@ -20,6 +20,8 @@ package org.apache.cloudstack.backup.dao;
 import javax.annotation.PostConstruct;
 import javax.inject.Inject;
 
+import com.cloud.domain.DomainVO;
+import com.cloud.domain.dao.DomainDao;
 import org.apache.cloudstack.api.response.BackupOfferingResponse;
 import org.apache.cloudstack.backup.BackupOffering;
 import org.apache.cloudstack.backup.BackupOfferingVO;
@@ -30,10 +32,16 @@ import com.cloud.utils.db.GenericDaoBase;
 import com.cloud.utils.db.SearchBuilder;
 import com.cloud.utils.db.SearchCriteria;
 
+import java.util.List;
+
 public class BackupOfferingDaoImpl extends GenericDaoBase<BackupOfferingVO, Long> implements BackupOfferingDao {
 
     @Inject
     DataCenterDao dataCenterDao;
+    @Inject
+    BackupOfferingDetailsDao backupOfferingDetailsDao;
+    @Inject
+    DomainDao domainDao;
 
     private SearchBuilder<BackupOfferingVO> backupPoliciesSearch;
 
@@ -51,8 +59,9 @@ public class BackupOfferingDaoImpl extends GenericDaoBase<BackupOfferingVO, Long
 
     @Override
     public BackupOfferingResponse newBackupOfferingResponse(BackupOffering offering, Boolean crossZoneInstanceCreation) {
-        DataCenterVO zone = dataCenterDao.findById(offering.getZoneId());
 
+        DataCenterVO zone = dataCenterDao.findById(offering.getZoneId());
+        List<Long> domainIds = backupOfferingDetailsDao.findDomainIds(offering.getId());
         BackupOfferingResponse response = new BackupOfferingResponse();
         response.setId(offering.getUuid());
         response.setName(offering.getName());
@@ -64,6 +73,18 @@ public class BackupOfferingDaoImpl extends GenericDaoBase<BackupOfferingVO, Long
             response.setZoneId(zone.getUuid());
             response.setZoneName(zone.getName());
         }
+        if (domainIds != null && !domainIds.isEmpty()) {
+            String domainUUIDs = domainIds.stream().map(Long::valueOf).map(domainId -> {
+                DomainVO domain = domainDao.findById(domainId);
+                return domain != null ? domain.getUuid() : "";
+            }).filter(name -> !name.isEmpty()).reduce((a, b) -> a + "," + b).orElse("");
+            String domainNames = domainIds.stream().map(Long::valueOf).map(domainId -> {
+                DomainVO domain = domainDao.findById(domainId);
+                return domain != null ? domain.getName() : "";
+            }).filter(name -> !name.isEmpty()).reduce((a, b) -> a + "," + b).orElse("");
+            response.setDomain(domainNames);
+            response.setDomainId(domainUUIDs);
+        }
         if (crossZoneInstanceCreation) {
             response.setCrossZoneInstanceCreation(true);
         }

server/src/main/java/com/cloud/acl/DomainChecker.java

@@ -481,15 +481,12 @@ public class DomainChecker extends AdapterBase implements SecurityChecker {
     @Override
     public boolean checkAccess(Account account, BackupOffering backupOffering) throws PermissionDeniedException {
         boolean hasAccess = false;
-        // Check for domains
         if (account == null || backupOffering == null) {
             hasAccess = true;
         } else {
-            // admin has all permissions
             if (_accountService.isRootAdmin(account.getId())) {
                 hasAccess = true;
             }
-            // if account is normal user or domain admin or project
             else if (_accountService.isNormalUser(account.getId())
                     || account.getType() == Account.Type.RESOURCE_DOMAIN_ADMIN
                     || _accountService.isDomainAdmin(account.getId())

server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java

@@ -291,7 +291,6 @@ public class BackupManagerImpl extends ManagerBase implements BackupManager {
             }
         }
 
-        // enforce account permissions: domain-admins cannot create public offerings
         final Account caller = CallContext.current().getCallingAccount();
         List<Long> filteredDomainIds = cmd.getDomainIds() == null ? new ArrayList<>() : new ArrayList<>(cmd.getDomainIds());
         if (filteredDomainIds.size() > 1) {
@@ -310,11 +309,10 @@ public class BackupManagerImpl extends ManagerBase implements BackupManager {
         if (savedOffering == null) {
             throw new CloudRuntimeException("Unable to create backup offering: " + cmd.getExternalId() + ", name: " + cmd.getName());
         }
-        // Persist domain dedication details (if any)
         if (org.apache.commons.collections.CollectionUtils.isNotEmpty(filteredDomainIds)) {
             List<BackupOfferingDetailsVO> detailsVOList = new ArrayList<>();
             for (Long domainId : filteredDomainIds) {
-                detailsVOList.add(new BackupOfferingDetailsVO(savedOffering.getId(), org.apache.cloudstack.api.ApiConstants.DOMAIN_ID, String.valueOf(domainId), false));
+                detailsVOList.add(new BackupOfferingDetailsVO(savedOffering.getId(), ApiConstants.DOMAIN_ID, String.valueOf(domainId), false));
             }
             if (!detailsVOList.isEmpty()) {
                 backupOfferingDetailsDao.saveDetails(detailsVOList);
@@ -400,7 +398,6 @@ public class BackupManagerImpl extends ManagerBase implements BackupManager {
             throw new CloudRuntimeException("Could not find a backup offering with id: " + offeringId);
         }
 
-        // Ensure caller has permission to delete this offering
         accountManager.checkAccess(CallContext.current().getCallingAccount(), offering);
 
         if (backupDao.listByOfferingId(offering.getId()).size() > 0) {
@@ -2179,6 +2176,7 @@ public class BackupManagerImpl extends ManagerBase implements BackupManager {
         String name = updateBackupOfferingCmd.getName();
         String description = updateBackupOfferingCmd.getDescription();
         Boolean allowUserDrivenBackups = updateBackupOfferingCmd.getAllowUserDrivenBackups();
+        List<Long> domainIds = updateBackupOfferingCmd.getDomainIds();
 
         BackupOfferingVO backupOfferingVO = backupOfferingDao.findById(id);
         if (backupOfferingVO == null) {
@@ -2209,16 +2207,58 @@ public class BackupManagerImpl extends ManagerBase implements BackupManager {
             fields.add("allowUserDrivenBackups: " + allowUserDrivenBackups);
         }
 
-        if (!backupOfferingDao.update(id, offering)) {
+        if (org.apache.commons.collections.CollectionUtils.isNotEmpty(domainIds)) {
+            for (final Long domainId: domainIds) {
+                if (domainDao.findById(domainId) == null) {
+                    throw new InvalidParameterValueException("Please specify a valid domain id");
+                }
+            }
+        }
+        List<Long> filteredDomainIds = filterChildSubDomains(domainIds);
+        Collections.sort(filteredDomainIds);
+
+        boolean success =backupOfferingDao.update(id, offering);
+        if (!success) {
             logger.warn(String.format("Couldn't update Backup offering (%s) with [%s].", backupOfferingVO, String.join(", ", fields)));
         }
 
+        if (success) {
+            List<Long> existingDomainIds = backupOfferingDetailsDao.findDomainIds(id);
+            Collections.sort(existingDomainIds);
+            updateBackupOfferingDomainDetails(id, filteredDomainIds, existingDomainIds);
+        }
+
         BackupOfferingVO response = backupOfferingDao.findById(id);
         CallContext.current().setEventDetails(String.format("Backup Offering updated [%s].",
                 ReflectionToStringBuilderUtils.reflectOnlySelectedFields(response, "id", "name", "description", "userDrivenBackupAllowed", "externalId")));
         return response;
     }
 
+    private void updateBackupOfferingDomainDetails(Long id, List<Long> filteredDomainIds, List<Long> existingDomainIds) {
+            if (existingDomainIds == null) {
+                existingDomainIds = new ArrayList<>();
+            }
+        List<BackupOfferingDetailsVO> detailsVO = new ArrayList<>();
+        if(!filteredDomainIds.equals(existingDomainIds)) {
+            SearchBuilder<BackupOfferingDetailsVO> sb = backupOfferingDetailsDao.createSearchBuilder();
+            sb.and("offeringId", sb.entity().getResourceId(), SearchCriteria.Op.EQ);
+            sb.and("detailName", sb.entity().getName(), SearchCriteria.Op.EQ);
+            sb.done();
+            SearchCriteria<BackupOfferingDetailsVO> sc = sb.create();
+            sc.setParameters("offeringId", String.valueOf(id));
+            sc.setParameters("detailName", ApiConstants.DOMAIN_ID);
+            backupOfferingDetailsDao.remove(sc);
+            for (Long domainId : filteredDomainIds) {
+                detailsVO.add(new BackupOfferingDetailsVO(id, ApiConstants.DOMAIN_ID, String.valueOf(domainId), false));
+            }
+        }
+        if (!detailsVO.isEmpty()) {
+            for (BackupOfferingDetailsVO detailVO : detailsVO) {
+                backupOfferingDetailsDao.persist(detailVO);
+            }
+        }
+    }
+
     Map<String, String> getDetailsFromBackupDetails(Long backupId) {
         Map<String, String> details = backupDetailsDao.listDetailsKeyPairs(backupId, true);
         if (details == null) {

server/src/test/java/org/apache/cloudstack/backup/BackupManagerTest.java

@@ -85,6 +85,7 @@ import org.apache.cloudstack.api.response.BackupResponse;
 import org.apache.cloudstack.backup.dao.BackupDao;
 import org.apache.cloudstack.backup.dao.BackupDetailsDao;
 import org.apache.cloudstack.backup.dao.BackupOfferingDao;
+import org.apache.cloudstack.backup.dao.BackupOfferingDetailsDao;
 import org.apache.cloudstack.backup.dao.BackupScheduleDao;
 import org.apache.cloudstack.context.CallContext;
 import org.apache.cloudstack.framework.config.ConfigKey;
@@ -241,6 +242,9 @@ public class BackupManagerTest {
     @Mock
     private GuestOSDao _guestOSDao;
 
+    @Mock
+    private BackupOfferingDetailsDao backupOfferingDetailsDao;
+
     private Gson gson;
 
     private String[] hostPossibleValues = {"127.0.0.1", "hostname"};
@@ -352,6 +356,7 @@ public class BackupManagerTest {
         when(cmd.getName()).thenReturn("New name");
         when(cmd.getDescription()).thenReturn("New description");
         when(cmd.getAllowUserDrivenBackups()).thenReturn(true);
+        when(backupOfferingDetailsDao.findDomainIds(id)).thenReturn(Collections.emptyList());
 
         BackupOffering updated = backupManager.updateBackupOffering(cmd);
         assertEquals("New name", updated.getName());
@@ -1081,7 +1086,7 @@ public class BackupManagerTest {
 
         assertEquals("root-disk-offering-uuid", VmDiskInfo.getDiskOffering().getUuid());
         assertEquals(Long.valueOf(5), VmDiskInfo.getSize());
-//        assertNull(com.cloud.vm.VmDiskInfo.getDeviceId());
+        assertNull(com.cloud.vm.VmDiskInfo.getDeviceId());
     }
 
     @Test
@@ -2156,4 +2161,75 @@ public class BackupManagerTest {
         verify(vmInstanceDao, times(1)).findByIdIncludingRemoved(vmId);
         verify(volumeDao, times(1)).findByInstance(vmId);
     }
+
+    @Test
+    public void getBackupOfferingDomainsTestOfferingNotFound() {
+        Long offeringId = 1L;
+        when(backupOfferingDao.findById(offeringId)).thenReturn(null);
+
+        InvalidParameterValueException exception = Assert.assertThrows(InvalidParameterValueException.class,
+                () -> backupManager.getBackupOfferingDomains(offeringId));
+        assertEquals("Unable to find backup offering null", exception.getMessage());
+    }
+
+    @Test
+    public void getBackupOfferingDomainsTestReturnsDomains() {
+        Long offeringId = 1L;
+        BackupOfferingVO offering = Mockito.mock(BackupOfferingVO.class);
+        when(backupOfferingDao.findById(offeringId)).thenReturn(offering);
+        when(backupOfferingDetailsDao.findDomainIds(offeringId)).thenReturn(List.of(10L, 20L));
+
+        List<Long> result = backupManager.getBackupOfferingDomains(offeringId);
+
+        assertEquals(2, result.size());
+        assertTrue(result.contains(10L));
+        assertTrue(result.contains(20L));
+    }
+
+    @Test
+    public void testUpdateBackupOfferingThrowsWhenDomainIdInvalid() {
+        Long id = 1234L;
+        UpdateBackupOfferingCmd cmd = Mockito.spy(UpdateBackupOfferingCmd.class);
+        when(cmd.getId()).thenReturn(id);
+        when(cmd.getDomainIds()).thenReturn(List.of(99L));
+
+        when(domainDao.findById(99L)).thenReturn(null);
+
+        InvalidParameterValueException exception = Assert.assertThrows(InvalidParameterValueException.class,
+                () -> backupManager.updateBackupOffering(cmd));
+        assertEquals("Please specify a valid domain id", exception.getMessage());
+    }
+
+    @Test
+    public void testUpdateBackupOfferingPersistsDomainDetailsWhenProvided() {
+        Long id = 1234L;
+        Long domainId = 11L;
+        UpdateBackupOfferingCmd cmd = Mockito.spy(UpdateBackupOfferingCmd.class);
+        when(cmd.getId()).thenReturn(id);
+        when(cmd.getDomainIds()).thenReturn(List.of(domainId));
+
+        DomainVO domain = Mockito.mock(DomainVO.class);
+        when(domainDao.findById(domainId)).thenReturn(domain);
+
+        when(backupOfferingDetailsDao.findDomainIds(id)).thenReturn(Collections.emptyList());
+
+        SearchBuilder<BackupOfferingDetailsVO> sb = Mockito.mock(SearchBuilder.class);
+        SearchCriteria<BackupOfferingDetailsVO> sc = Mockito.mock(SearchCriteria.class);
+        BackupOfferingDetailsVO entity = Mockito.mock(BackupOfferingDetailsVO.class);
+        when(backupOfferingDetailsDao.createSearchBuilder()).thenReturn(sb);
+        when(sb.entity()).thenReturn(entity);
+        when(sb.and(Mockito.anyString(), (Object) any(), Mockito.any())).thenReturn(sb);
+        when(sb.create()).thenReturn(sc);
+
+        BackupOfferingVO offering = Mockito.mock(BackupOfferingVO.class);
+        BackupOfferingVO offeringUpdate = Mockito.mock(BackupOfferingVO.class);
+        when(backupOfferingDao.findById(id)).thenReturn(offering);
+        when(backupOfferingDao.createForUpdate(id)).thenReturn(offeringUpdate);
+        when(backupOfferingDao.update(id, offeringUpdate)).thenReturn(true);
+
+        BackupOffering updated = backupManager.updateBackupOffering(cmd);
+
+        verify(backupOfferingDetailsDao, times(1)).persist(Mockito.any(BackupOfferingDetailsVO.class));
+    }
+
 }

ui/src/config/section/offering.js

@@ -340,9 +340,9 @@ export default {
       icon: 'cloud-upload-outlined',
       docHelp: 'adminguide/virtual_machines.html#backup-offerings',
       permission: ['listBackupOfferings'],
-      searchFilters: ['zoneid'],
+      searchFilters: ['zoneid', 'domainid'],
-      columns: ['name', 'description', 'zonename'],
+      columns: ['name', 'description', 'domain', 'zonename'],
-      details: ['name', 'id', 'description', 'externalid', 'zone', 'allowuserdrivenbackups', 'created'],
+      details: ['name', 'id', 'description', 'externalid', 'domain', 'zone', 'allowuserdrivenbackups', 'created'],
       related: [{
         name: 'vm',
         title: 'label.instances',

ui/src/views/offering/ImportBackupOffering.vue

@@ -85,6 +85,33 @@
         </template>
         <a-switch v-model:checked="form.allowuserdrivenbackups"/>
       </a-form-item>
+      <a-form-item name="ispublic" ref="ispublic" :label="$t('label.ispublic')" v-if="isAdmin()">
+          <a-switch v-model:checked="form.ispublic" />
+        </a-form-item>
+        <a-form-item name="domainid" ref="domainid" v-if="!form.ispublic">
+          <template #label>
+            <tooltip-label :title="$t('label.domainid')" :tooltip="apiParams.domainid.description"/>
+          </template>
+          <a-select
+            mode="multiple"
+            :getPopupContainer="(trigger) => trigger.parentNode"
+            v-model:value="form.domainid"
+            showSearch
+            optionFilterProp="label"
+            :filterOption="(input, option) => {
+              return option.label.toLowerCase().indexOf(input.toLowerCase()) >= 0
+            }"
+            :loading="domains.loading"
+            :placeholder="apiParams.domainid.description">
+            <a-select-option v-for="(opt, optIndex) in domains.opts" :key="optIndex" :label="opt.path || opt.name || opt.description">
+              <span>
+                <resource-icon v-if="opt && opt.icon" :image="opt.icon.base64image" size="1x" style="margin-right: 5px"/>
+                <block-outlined v-else style="margin-right: 5px" />
+                {{ opt.path || opt.name || opt.description }}
+              </span>
+            </a-select-option>
+          </a-select>
+        </a-form-item>
       <div :span="24" class="action-button">
         <a-button :loading="loading" @click="closeAction">{{ this.$t('label.cancel') }}</a-button>
         <a-button :loading="loading" ref="submit" type="primary" @click="handleSubmit">{{ this.$t('label.ok') }}</a-button>
@@ -96,6 +123,7 @@
 <script>
 import { ref, reactive, toRaw } from 'vue'
 import { getAPI, postAPI } from '@/api'
+import { isAdmin } from '@/role'
 import ResourceIcon from '@/components/view/ResourceIcon'
 import TooltipLabel from '@/components/widgets/TooltipLabel'
 
@@ -108,6 +136,10 @@ export default {
   data () {
     return {
       loading: false,
+      domains: {
+        loading: false,
+        opts: []
+      },
       zones: {
         loading: false,
         opts: []
@@ -129,17 +161,23 @@ export default {
     initForm () {
       this.formRef = ref()
       this.form = reactive({
-        allowuserdrivenbackups: true
+        allowuserdrivenbackups: true,
+        ispublic: true
       })
       this.rules = reactive({
         name: [{ required: true, message: this.$t('message.error.required.input') }],
         description: [{ required: true, message: this.$t('message.error.required.input') }],
         zoneid: [{ required: true, message: this.$t('message.error.select') }],
-        externalid: [{ required: true, message: this.$t('message.error.select') }]
+        externalid: [{ required: true, message: this.$t('message.error.select') }],
+        domainid: [{ type: 'array', message: this.$t('message.error.select') }]
       })
     },
+    isAdmin () {
+      return isAdmin()
+    },
     fetchData () {
       this.fetchZone()
+      this.fetchDomainData()
     },
     fetchZone () {
       this.zones.loading = true
@@ -151,6 +189,19 @@ export default {
         this.zones.loading = false
       })
     },
+    fetchDomainData () {
+      const params = {}
+      params.listAll = true
+      params.details = 'min'
+      this.domains.loading = true
+      getAPI('listDomains', params).then(json => {
+        this.domains.opts = json.listdomainsresponse.domain || []
+      }).catch(error => {
+        this.$notifyError(error)
+      }).finally(() => {
+        this.domains.loading = false
+      })
+    },
     fetchExternal (zoneId) {
       if (!zoneId) {
         this.externals.opts = []
@@ -179,6 +230,20 @@ export default {
             params[key] = input
           }
         }
+        if (values.ispublic !== true) {
+          var domainIndexes = values.domainid
+          var domainId = null
+          if (domainIndexes && domainIndexes.length > 0) {
+            var domainIds = []
+            for (var i = 0; i < domainIndexes.length; i++) {
+              domainIds = domainIds.concat(this.domains.opts[domainIndexes[i]].id)
+            }
+            domainId = domainIds.join(',')
+          }
+          if (domainId) {
+            params.domainid = domainId
+          }
+        }
         params.allowuserdrivenbackups = values.allowuserdrivenbackups
         this.loading = true
         const title = this.$t('label.import.offering')