diff --git a/api/src/main/java/com/cloud/storage/VMTemplateStorageResourceAssoc.java b/api/src/main/java/com/cloud/storage/VMTemplateStorageResourceAssoc.java index f43d5331222..db702a61f2b 100644 --- a/api/src/main/java/com/cloud/storage/VMTemplateStorageResourceAssoc.java +++ b/api/src/main/java/com/cloud/storage/VMTemplateStorageResourceAssoc.java @@ -17,6 +17,7 @@ package com.cloud.storage; import java.util.Date; +import java.util.List; import org.apache.cloudstack.api.InternalIdentity; @@ -25,6 +26,8 @@ public interface VMTemplateStorageResourceAssoc extends InternalIdentity { UNKNOWN, DOWNLOAD_ERROR, NOT_DOWNLOADED, DOWNLOAD_IN_PROGRESS, DOWNLOADED, ABANDONED, UPLOADED, NOT_UPLOADED, UPLOAD_ERROR, UPLOAD_IN_PROGRESS, CREATING, CREATED, BYPASSED } + List PENDING_DOWNLOAD_STATES = List.of(Status.NOT_DOWNLOADED, Status.DOWNLOAD_IN_PROGRESS); + String getInstallPath(); long getTemplateId(); diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/iso/ExtractIsoCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/iso/ExtractIsoCmd.java index 7861c1e5d41..4d11f0dc3d2 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/iso/ExtractIsoCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/iso/ExtractIsoCmd.java @@ -17,6 +17,7 @@ package org.apache.cloudstack.api.command.user.iso; +import com.cloud.dc.DataCenter; import org.apache.cloudstack.api.APICommand; import org.apache.cloudstack.api.ApiCommandResourceType; import org.apache.cloudstack.api.ApiConstants; @@ -101,7 +102,15 @@ public class ExtractIsoCmd extends BaseAsyncCmd { @Override public String getEventDescription() { - return "extracting ISO: " + getId() + " from zone: " + getZoneId(); + String isoId = this._uuidMgr.getUuid(VirtualMachineTemplate.class, getId()); + String baseDescription = String.format("Extracting ISO: %s", isoId); + + Long zoneId = getZoneId(); + if (zoneId == null) { + return baseDescription; + } + + return String.format("%s from zone: %s", baseDescription, this._uuidMgr.getUuid(DataCenter.class, zoneId)); } @Override diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/template/ExtractTemplateCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/template/ExtractTemplateCmd.java index 0fa0679bfd9..22f59351e9a 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/template/ExtractTemplateCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/template/ExtractTemplateCmd.java @@ -101,7 +101,15 @@ public class ExtractTemplateCmd extends BaseAsyncCmd { @Override public String getEventDescription() { - return "extracting template: " + this._uuidMgr.getUuid(VirtualMachineTemplate.class, getId()) + ((getZoneId() != null) ? " from zone: " + this._uuidMgr.getUuid(DataCenter.class, getZoneId()) : ""); + String templateId = this._uuidMgr.getUuid(VirtualMachineTemplate.class, getId()); + String baseDescription = String.format("Extracting template: %s", templateId); + + Long zoneId = getZoneId(); + if (zoneId == null) { + return baseDescription; + } + + return String.format("%s from zone: %s", baseDescription, this._uuidMgr.getUuid(DataCenter.class, zoneId)); } @Override diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/volume/ExtractVolumeCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/volume/ExtractVolumeCmd.java index 9445aba23c0..6ddb5f1e567 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/volume/ExtractVolumeCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/volume/ExtractVolumeCmd.java @@ -17,6 +17,7 @@ package org.apache.cloudstack.api.command.user.volume; +import com.cloud.dc.DataCenter; import org.apache.cloudstack.acl.SecurityChecker.AccessType; import org.apache.cloudstack.api.ACL; import org.apache.cloudstack.api.APICommand; @@ -114,12 +115,15 @@ public class ExtractVolumeCmd extends BaseAsyncCmd { @Override public String getEventDescription() { - return "Extraction job"; + String volumeId = this._uuidMgr.getUuid(Volume.class, getId()); + String zoneId = this._uuidMgr.getUuid(DataCenter.class, getZoneId()); + + return String.format("Extracting volume: %s from zone: %s", volumeId, zoneId); } @Override public void execute() { - CallContext.current().setEventDetails("Volume Id: " + this._uuidMgr.getUuid(Volume.class, getId())); + CallContext.current().setEventDetails(getEventDescription()); String uploadUrl = _volumeService.extractVolume(this); if (uploadUrl != null) { ExtractResponse response = _responseGenerator.createVolumeExtractResponse(id, zoneId, getEntityOwnerId(), mode, uploadUrl); diff --git a/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java b/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java index 472fe148a5d..f3b3b31bd71 100755 --- a/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java +++ b/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java @@ -5240,10 +5240,9 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac workJob = newVmWorkJobAndInfo.first(); VmWorkMigrateAway workInfo = new VmWorkMigrateAway(newVmWorkJobAndInfo.second(), srcHostId); - workJob.setCmdInfo(VmWorkSerializer.serialize(workInfo)); + setCmdInfoAndSubmitAsyncJob(workJob, workInfo, vmId); } - _jobMgr.submitAsyncJob(workJob, VmWorkConstants.VM_WORK_QUEUE, vmId); AsyncJobExecutionContext.getCurrentExecutionContext().joinJob(workJob.getId()); diff --git a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/DataMigrationUtility.java b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/DataMigrationUtility.java index c260f48dcf8..9609ba7751d 100644 --- a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/DataMigrationUtility.java +++ b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/DataMigrationUtility.java @@ -208,9 +208,7 @@ public class DataMigrationUtility { List files = new LinkedList<>(); for (TemplateDataStoreVO template : templates) { VMTemplateVO templateVO = templateDao.findById(template.getTemplateId()); - if (template.getState() == ObjectInDataStoreStateMachine.State.Ready && templateVO != null && - (!templateVO.isPublicTemplate() || (templateVO.isPublicTemplate() && templateVO.getUrl() == null)) && - templateVO.getHypervisorType() != Hypervisor.HypervisorType.Simulator && templateVO.getParentTemplateId() == null) { + if (shouldMigrateTemplate(template, templateVO)) { files.add(templateFactory.getTemplate(template.getTemplateId(), srcDataStore)); } } @@ -231,6 +229,34 @@ public class DataMigrationUtility { return getAllReadyTemplates(srcDataStore, childTemplates, templates); } + /** + * Returns whether a template should be migrated. A template should be migrated if: + *
    + *
  1. its state is ready, and
    2. + *
  2. its hypervisor type is not simulator, and
    3. + *
  3. it is not a child template.
    4. + *
+ */ + protected boolean shouldMigrateTemplate(TemplateDataStoreVO template, VMTemplateVO templateVO) { + if (template.getState() != State.Ready) { + logger.debug("Template [{}] should not be migrated as it is not ready.", template); + return false; + } + + if (templateVO.getHypervisorType() == Hypervisor.HypervisorType.Simulator) { + logger.debug("Template [{}] should not be migrated as its hypervisor type is simulator.", template); + return false; + } + + if (templateVO.getParentTemplateId() != null) { + logger.debug("Template [{}] should not be migrated as it has a parent template.", template); + return false; + } + + logger.debug("Template [{}] should be migrated.", template); + return true; + } + /** Returns parent snapshots and snapshots that do not have any children; snapshotChains comprises of the snapshot chain info * for each parent snapshot and the cumulative size of the chain - this is done to ensure that all the snapshots in a chain * are migrated to the same datastore diff --git a/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/DataMigrationUtilityTest.java b/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/DataMigrationUtilityTest.java new file mode 100644 index 00000000000..acd98e1cbff --- /dev/null +++ b/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/DataMigrationUtilityTest.java @@ -0,0 +1,88 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. +package org.apache.cloudstack.engine.orchestration; + +import com.cloud.hypervisor.Hypervisor; +import com.cloud.storage.VMTemplateVO; +import junit.framework.TestCase; +import org.apache.cloudstack.engine.subsystem.api.storage.ObjectInDataStoreStateMachine; +import org.apache.cloudstack.storage.datastore.db.TemplateDataStoreVO; +import org.junit.Assert; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.Spy; +import org.mockito.junit.MockitoJUnitRunner; + +@RunWith(MockitoJUnitRunner.class) +public class DataMigrationUtilityTest extends TestCase { + + @Spy + private DataMigrationUtility dataMigrationUtility; + + @Mock + private VMTemplateVO templateVoMock; + + @Mock + private TemplateDataStoreVO templateDataStoreVoMock; + + private void prepareForShouldMigrateTemplateTests() { + Mockito.when(templateDataStoreVoMock.getState()).thenReturn(ObjectInDataStoreStateMachine.State.Ready); + Mockito.when(templateVoMock.getHypervisorType()).thenReturn(Hypervisor.HypervisorType.KVM); + Mockito.when(templateVoMock.getParentTemplateId()).thenReturn(null); + } + + @Test + public void shouldMigrateTemplateTestReturnsFalseWhenTemplateIsNotReady() { + prepareForShouldMigrateTemplateTests(); + Mockito.when(templateDataStoreVoMock.getState()).thenReturn(ObjectInDataStoreStateMachine.State.Migrating); + + boolean result = dataMigrationUtility.shouldMigrateTemplate(templateDataStoreVoMock, templateVoMock); + + Assert.assertFalse(result); + } + + @Test + public void shouldMigrateTemplateTestReturnsFalseWhenHypervisorTypeIsSimulator() { + prepareForShouldMigrateTemplateTests(); + Mockito.when(templateVoMock.getHypervisorType()).thenReturn(Hypervisor.HypervisorType.Simulator); + + boolean result = dataMigrationUtility.shouldMigrateTemplate(templateDataStoreVoMock, templateVoMock); + + Assert.assertFalse(result); + } + + @Test + public void shouldMigrateTemplateTestReturnsFalseWhenTemplateHasParentTemplate() { + prepareForShouldMigrateTemplateTests(); + Mockito.when(templateVoMock.getParentTemplateId()).thenReturn(1L); + + boolean result = dataMigrationUtility.shouldMigrateTemplate(templateDataStoreVoMock, templateVoMock); + + Assert.assertFalse(result); + } + + @Test + public void shouldMigrateTemplateTestReturnsTrueWhenTemplateIsReadyAndDoesNotHaveParentTemplateAndHypervisorTypeIsNotSimulator() { + prepareForShouldMigrateTemplateTests(); + + boolean result = dataMigrationUtility.shouldMigrateTemplate(templateDataStoreVoMock, templateVoMock); + + Assert.assertTrue(result); + } +} diff --git a/engine/schema/src/main/java/com/cloud/vm/VMInstanceVO.java b/engine/schema/src/main/java/com/cloud/vm/VMInstanceVO.java index a1d9f4a8089..9d5e1b0ff50 100644 --- a/engine/schema/src/main/java/com/cloud/vm/VMInstanceVO.java +++ b/engine/schema/src/main/java/com/cloud/vm/VMInstanceVO.java @@ -503,7 +503,7 @@ public class VMInstanceVO implements VirtualMachine, FiniteStateObject future, DataObject srcDataObject, DataObject destDataObject, DataStore destDatastore) throws ExecutionException, InterruptedException { MigrateDataContext context = new MigrateDataContext(null, future, srcDataObject, destDataObject, destDatastore); AsyncCallbackDispatcher caller = AsyncCallbackDispatcher.create(this); diff --git a/engine/storage/image/src/test/java/org/apache/cloudstack/storage/image/SecondaryStorageServiceImplTest.java b/engine/storage/image/src/test/java/org/apache/cloudstack/storage/image/SecondaryStorageServiceImplTest.java new file mode 100644 index 00000000000..740194ea253 --- /dev/null +++ b/engine/storage/image/src/test/java/org/apache/cloudstack/storage/image/SecondaryStorageServiceImplTest.java @@ -0,0 +1,138 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. +package org.apache.cloudstack.storage.image; + +import com.cloud.agent.api.to.DataStoreTO; +import com.cloud.storage.VMTemplateStorageResourceAssoc; +import com.cloud.utils.exception.CloudRuntimeException; +import junit.framework.TestCase; +import org.apache.cloudstack.engine.subsystem.api.storage.DataStore; +import org.apache.cloudstack.engine.subsystem.api.storage.ObjectInDataStoreStateMachine; +import org.apache.cloudstack.engine.subsystem.api.storage.TemplateInfo; +import org.apache.cloudstack.storage.datastore.db.TemplateDataStoreDao; +import org.apache.cloudstack.storage.datastore.db.TemplateDataStoreVO; +import org.apache.cloudstack.storage.to.TemplateObjectTO; +import org.junit.Assert; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.Spy; +import org.mockito.junit.MockitoJUnitRunner; + +@RunWith(MockitoJUnitRunner.class) +public class SecondaryStorageServiceImplTest extends TestCase { + + @Spy + @InjectMocks + private SecondaryStorageServiceImpl secondaryStorageService; + + @Mock + TemplateDataStoreDao templateDataStoreDaoMock; + + @Mock + TemplateDataStoreVO templateDataStoreVoMock; + + @Mock + TemplateInfo templateInfoMock; + + @Mock + TemplateObjectTO templateObjectToMock; + + @Mock + DataStore dataStoreMock; + + @Mock + DataStoreTO dataStoreToMock; + + private void prepareForTemplateIsOnDestinationTests() { + long dataStoreId = 1; + long templateId = 2; + + Mockito.when(dataStoreMock.getId()).thenReturn(dataStoreId); + Mockito.when(dataStoreMock.getTO()).thenReturn(dataStoreToMock); + Mockito.when(templateInfoMock.getId()).thenReturn(templateId); + Mockito.when(templateInfoMock.getTO()).thenReturn(templateObjectToMock); + Mockito.doReturn(templateDataStoreVoMock).when(templateDataStoreDaoMock).findByStoreTemplate(dataStoreId, templateId); + Mockito.when(templateDataStoreVoMock.getState()).thenReturn(ObjectInDataStoreStateMachine.State.Ready); + } + + @Test + public void templateIsOnDestinationTestReturnsFalseWhenTemplateStoreRefDoesNotExist() { + prepareForTemplateIsOnDestinationTests(); + Mockito.doReturn(null).when(templateDataStoreDaoMock).findByStoreTemplate(Mockito.anyLong(), Mockito.anyLong()); + + boolean result = secondaryStorageService.templateIsOnDestination(templateInfoMock, dataStoreMock); + + Assert.assertFalse(result); + } + + @Test + public void templateIsOnDestinationTestReturnsTrueWhenTemplateIsReady() { + prepareForTemplateIsOnDestinationTests(); + + boolean result = secondaryStorageService.templateIsOnDestination(templateInfoMock, dataStoreMock); + + Assert.assertTrue(result); + } + + @Test + public void templateIsOnDestinationTestReturnsFalseWhenTemplateIsNotReady() { + prepareForTemplateIsOnDestinationTests(); + Mockito.when(templateDataStoreVoMock.getState()).thenReturn(ObjectInDataStoreStateMachine.State.Creating); + + boolean result = secondaryStorageService.templateIsOnDestination(templateInfoMock, dataStoreMock); + + Assert.assertFalse(result); + } + + @Test + public void templateIsOnDestinationTestReturnsTrueIfTemplateIsDownloadedSuccessfully() { + prepareForTemplateIsOnDestinationTests(); + Mockito.when(templateDataStoreVoMock.getDownloadState()).thenReturn(VMTemplateStorageResourceAssoc.Status.DOWNLOAD_IN_PROGRESS); + Mockito.doAnswer(I -> Mockito.when(templateDataStoreVoMock.getDownloadState()).thenReturn(VMTemplateStorageResourceAssoc.Status.DOWNLOADED)).when(secondaryStorageService).waitForTemplateDownload(Mockito.anyLong(), Mockito.anyString(), Mockito.anyString()); + + boolean result = secondaryStorageService.templateIsOnDestination(templateInfoMock, dataStoreMock); + + Assert.assertTrue(result); + } + + @Test + public void templateIsOnDestinationTestReturnsFalseIfTemplateIsNotDownloadedSuccessfully() { + prepareForTemplateIsOnDestinationTests(); + Mockito.when(templateDataStoreVoMock.getDownloadState()).thenReturn(VMTemplateStorageResourceAssoc.Status.DOWNLOAD_IN_PROGRESS); + Mockito.doAnswer(I -> { + Mockito.when(templateDataStoreVoMock.getDownloadState()).thenReturn(VMTemplateStorageResourceAssoc.Status.DOWNLOAD_ERROR); + Mockito.when(templateDataStoreVoMock.getState()).thenReturn(ObjectInDataStoreStateMachine.State.Failed); + return "mocked download fail"; + }).when(secondaryStorageService).waitForTemplateDownload(Mockito.anyLong(), Mockito.anyString(), Mockito.anyString()); + + boolean result = secondaryStorageService.templateIsOnDestination(templateInfoMock, dataStoreMock); + + Assert.assertFalse(result); + } + + @Test(expected = CloudRuntimeException.class) + public void templateIsOnDestinationTestThrowsExceptionIfDownloadTimesOut() { + prepareForTemplateIsOnDestinationTests(); + Mockito.when(templateDataStoreVoMock.getDownloadState()).thenReturn(VMTemplateStorageResourceAssoc.Status.DOWNLOAD_IN_PROGRESS); + Mockito.doReturn(0L).when(secondaryStorageService).getTemplateDownloadTimeout(); + + secondaryStorageService.templateIsOnDestination(templateInfoMock, dataStoreMock); + } +} diff --git a/framework/cluster/src/main/java/com/cloud/cluster/ClusterManagerImpl.java b/framework/cluster/src/main/java/com/cloud/cluster/ClusterManagerImpl.java index d2c85b498f2..309d66e9d30 100644 --- a/framework/cluster/src/main/java/com/cloud/cluster/ClusterManagerImpl.java +++ b/framework/cluster/src/main/java/com/cloud/cluster/ClusterManagerImpl.java @@ -73,8 +73,8 @@ public class ClusterManagerImpl extends ManagerBase implements ClusterManager, C private static final int EXECUTOR_SHUTDOWN_TIMEOUT = 1000; // 1 second private static final int DEFAULT_OUTGOING_WORKERS = 5; - private final List _listeners = new ArrayList(); - private final Map _activePeers = new HashMap(); + private final List _listeners = new ArrayList<>(); + private final Map _activePeers = new HashMap<>(); private final Map _clusterPeers; @@ -83,7 +83,7 @@ public class ClusterManagerImpl extends ManagerBase implements ClusterManager, C private final ScheduledExecutorService _heartbeatScheduler = Executors.newScheduledThreadPool(1, new NamedThreadFactory("Cluster-Heartbeat")); private final ExecutorService _notificationExecutor = Executors.newFixedThreadPool(1, new NamedThreadFactory("Cluster-Notification")); - private final List _notificationMsgs = new ArrayList(); + private final List _notificationMsgs = new ArrayList<>(); private ConnectionConcierge _heartbeatConnection = null; private final ExecutorService _executor; @@ -118,12 +118,12 @@ public class ClusterManagerImpl extends ManagerBase implements ClusterManager, C private String _clusterNodeIP = "127.0.0.1"; - private final List _clusterPduOutgoingQueue = new ArrayList(); - private final List _clusterPduIncomingQueue = new ArrayList(); - private final Map _outgoingPdusWaitingForAck = new HashMap(); + private final List _clusterPduOutgoingQueue = new ArrayList<>(); + private final List _clusterPduIncomingQueue = new ArrayList<>(); + private final Map _outgoingPdusWaitingForAck = new HashMap<>(); public ClusterManagerImpl() { - _clusterPeers = new HashMap(); + _clusterPeers = new HashMap<>(); // executor to perform remote-calls in another thread context, to avoid potential // recursive remote calls between nodes @@ -161,7 +161,7 @@ public class ClusterManagerImpl extends ManagerBase implements ClusterManager, C } private void cancelClusterRequestToPeer(final String strPeer) { - final List candidates = new ArrayList(); + final List candidates = new ArrayList<>(); synchronized (_outgoingPdusWaitingForAck) { for (final Map.Entry entry : _outgoingPdusWaitingForAck.entrySet()) { if (entry.getValue().getDestPeer().equalsIgnoreCase(strPeer)) { @@ -193,7 +193,7 @@ public class ClusterManagerImpl extends ManagerBase implements ClusterManager, C synchronized (_clusterPduOutgoingQueue) { try { _clusterPduOutgoingQueue.wait(timeoutMs); - } catch (final InterruptedException e) { + } catch (final InterruptedException ignored) { } if (_clusterPduOutgoingQueue.size() > 0) { @@ -216,7 +216,7 @@ public class ClusterManagerImpl extends ManagerBase implements ClusterManager, C synchronized (_clusterPduIncomingQueue) { try { _clusterPduIncomingQueue.wait(timeoutMs); - } catch (final InterruptedException e) { + } catch (final InterruptedException ignored) { } if (_clusterPduIncomingQueue.size() > 0) { @@ -449,7 +449,7 @@ public class ClusterManagerImpl extends ManagerBase implements ClusterManager, C synchronized (pdu) { try { pdu.wait(); - } catch (final InterruptedException e) { + } catch (final InterruptedException ignored) { } } @@ -620,8 +620,8 @@ public class ClusterManagerImpl extends ManagerBase implements ClusterManager, C if (profiler.getDurationInMillis() >= HeartbeatInterval.value()) { if (logger.isDebugEnabled()) { - logger.debug("Management server heartbeat takes too long to finish. profiler: " + profiler.toString() + ", profilerHeartbeatUpdate: " + - profilerHeartbeatUpdate.toString() + ", profilerPeerScan: " + profilerPeerScan.toString()); + logger.debug("Management server heartbeat takes too long to finish. profiler: " + profiler + ", profilerHeartbeatUpdate: " + + profilerHeartbeatUpdate + ", profilerPeerScan: " + profilerPeerScan); } } } @@ -685,7 +685,7 @@ public class ClusterManagerImpl extends ManagerBase implements ClusterManager, C synchronized (_notificationMsgs) { try { _notificationMsgs.wait(1000); - } catch (final InterruptedException e) { + } catch (final InterruptedException ignored) { } } @@ -745,7 +745,7 @@ public class ClusterManagerImpl extends ManagerBase implements ClusterManager, C try { Thread.sleep(1000); - } catch (final InterruptedException e) { + } catch (final InterruptedException ignored) { } } } @@ -816,7 +816,7 @@ public class ClusterManagerImpl extends ManagerBase implements ClusterManager, C } } - final List downHostList = new ArrayList(); + final List downHostList = new ArrayList<>(); for (final ManagementServerHostVO host : inactiveList) { // Check if peer state is Up in the period if (!_mshostPeerDao.isPeerUpState(_mshostId, host.getId(), new Date(cutTime.getTime() - HeartbeatThreshold.value()))) { @@ -846,8 +846,8 @@ public class ClusterManagerImpl extends ManagerBase implements ClusterManager, C final Profiler profilerSyncClusterInfo = new Profiler(); profilerSyncClusterInfo.start(); - final List removedNodeList = new ArrayList(); - final List invalidatedNodeList = new ArrayList(); + final List removedNodeList = new ArrayList<>(); + final List invalidatedNodeList = new ArrayList<>(); if (_mshostId != null) { @@ -941,7 +941,7 @@ public class ClusterManagerImpl extends ManagerBase implements ClusterManager, C try { JmxUtil.unregisterMBean("ClusterManager", "Node " + mshost.getId()); } catch (final Exception e) { - logger.warn("Unable to deregister cluster node from JMX monitoring due to exception " + e.toString()); + logger.warn("Unable to deregister cluster node from JMX monitoring due to exception " + e); } } @@ -961,7 +961,7 @@ public class ClusterManagerImpl extends ManagerBase implements ClusterManager, C try { JmxUtil.unregisterMBean("ClusterManager", "Node " + mshost.getId()); } catch (final Exception e) { - logger.warn("Unable to deregiester cluster node from JMX monitoring due to exception " + e.toString()); + logger.warn("Unable to deregiester cluster node from JMX monitoring due to exception " + e); } } else { logger.info("Management node {} is detected inactive by timestamp but sent node status to this node", mshost); @@ -975,7 +975,7 @@ public class ClusterManagerImpl extends ManagerBase implements ClusterManager, C } private void processNewNodes(Date cutTime, List currentList) { - final List newNodeList = new ArrayList(); + final List newNodeList = new ArrayList<>(); for (final ManagementServerHostVO mshost : currentList) { if (!_activePeers.containsKey(mshost.getId())) { _activePeers.put(mshost.getId(), mshost); @@ -1037,7 +1037,7 @@ public class ClusterManagerImpl extends ManagerBase implements ClusterManager, C logger.info("Starting Cluster manager, msid: {}, mshost: {}", _msId, _mshost); } - final ManagementServerHostVO mshost = Transaction.execute(new TransactionCallback() { + final ManagementServerHostVO mshost = Transaction.execute(new TransactionCallback<>() { @Override public ManagementServerHostVO doInTransaction(final TransactionStatus status) { @@ -1105,11 +1105,13 @@ public class ClusterManagerImpl extends ManagerBase implements ClusterManager, C } if (_mshostId != null) { - final ManagementServerHostVO mshost = _mshostDao.findByMsid(_msId); + ManagementServerHostVO mshost = _mshostDao.findByMsid(_msId); if (mshost != null) { ManagementServerStatusVO mshostStatus = mshostStatusDao.findByMsId(mshost.getUuid()); if (mshostStatus != null) { + mshost.setState(ManagementServerHost.State.Down); mshostStatus.setLastJvmStop(new Date()); + _mshostDao.update(_mshostId, mshost); mshostStatusDao.update(mshostStatus.getId(), mshostStatus); } else { logger.warn("Found a management server host [{}] without a status. This should never happen!", mshost); @@ -1135,7 +1137,7 @@ public class ClusterManagerImpl extends ManagerBase implements ClusterManager, C try { _heartbeatScheduler.awaitTermination(EXECUTOR_SHUTDOWN_TIMEOUT, TimeUnit.MILLISECONDS); _executor.awaitTermination(EXECUTOR_SHUTDOWN_TIMEOUT, TimeUnit.MILLISECONDS); - } catch (final InterruptedException e) { + } catch (final InterruptedException ignored) { } if (logger.isInfoEnabled()) { @@ -1271,14 +1273,14 @@ public class ClusterManagerImpl extends ManagerBase implements ClusterManager, C if (sch != null) { try { sch.close(); - } catch (final IOException e) { + } catch (final IOException ignored) { } } } try { Thread.sleep(1000); - } catch (final InterruptedException ex) { + } catch (final InterruptedException ignored) { } } diff --git a/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java b/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java index a8ba033f3cc..6d002360b9b 100644 --- a/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java +++ b/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java @@ -5104,9 +5104,9 @@ public class VmwareResource extends ServerResourceBase implements StoragePoolRes answer.setVolumeChainInfo(chainInfo); return answer; } catch (Exception e) { - String msg = "Catch Exception " + e.getClass().getName() + " due to " + e.toString(); + String msg = "Catch Exception " + e.getClass().getName() + " due to " + e.getMessage(); logger.error(msg, e); - return new MigrateVolumeAnswer(cmd, false, msg, null); + return new MigrateVolumeAnswer(cmd, false, e.getMessage(), null); } } diff --git a/plugins/hypervisors/vmware/src/main/java/org/apache/cloudstack/storage/motion/VmwareStorageMotionStrategy.java b/plugins/hypervisors/vmware/src/main/java/org/apache/cloudstack/storage/motion/VmwareStorageMotionStrategy.java index 5b389e0d9df..b0cacf60a17 100644 --- a/plugins/hypervisors/vmware/src/main/java/org/apache/cloudstack/storage/motion/VmwareStorageMotionStrategy.java +++ b/plugins/hypervisors/vmware/src/main/java/org/apache/cloudstack/storage/motion/VmwareStorageMotionStrategy.java @@ -260,7 +260,7 @@ public class VmwareStorageMotionStrategy implements DataMotionStrategy { } else { answer = agentMgr.sendTo(sourcePool.getDataCenterId(), HypervisorType.VMware, cmd); } - updateVolumeAfterMigration(answer, srcData, destData); + handleAnswerAndUpdateVolumeAfterMigration(answer, srcData, destData); CopyCommandResult result = new CopyCommandResult(null, answer); callback.complete(result); } @@ -286,21 +286,19 @@ public class VmwareStorageMotionStrategy implements DataMotionStrategy { return hostId; } - private void updateVolumeAfterMigration(Answer answer, DataObject srcData, DataObject destData) { + private void handleAnswerAndUpdateVolumeAfterMigration(Answer answer, DataObject srcData, DataObject destData) { VolumeVO destinationVO = volDao.findById(destData.getId()); if (!(answer instanceof MigrateVolumeAnswer)) { // OfflineVmwareMigration: reset states and such - VolumeVO sourceVO = volDao.findById(srcData.getId()); - sourceVO.setState(Volume.State.Ready); - volDao.update(sourceVO.getId(), sourceVO); - if (destinationVO.getId() != sourceVO.getId()) { - destinationVO.setState(Volume.State.Expunged); - destinationVO.setRemoved(new Date()); - volDao.update(destinationVO.getId(), destinationVO); - } + resetVolumeState(srcData, destinationVO); throw new CloudRuntimeException("unexpected answer from hypervisor agent: " + answer.getDetails()); } MigrateVolumeAnswer ans = (MigrateVolumeAnswer) answer; + if (!answer.getResult()) { + String msg = "Unable to migrate volume: " + srcData.getName() + " due to " + answer.getDetails(); + resetVolumeState(srcData, destinationVO); + throw new CloudRuntimeException(msg); + } if (logger.isDebugEnabled()) { String format = "retrieved '%s' as new path for volume(%d)"; logger.debug(String.format(format, ans.getVolumePath(), destData.getId())); @@ -311,6 +309,17 @@ public class VmwareStorageMotionStrategy implements DataMotionStrategy { volDao.update(destinationVO.getId(), destinationVO); } + private void resetVolumeState(DataObject srcData, VolumeVO destinationVO) { + VolumeVO sourceVO = volDao.findById(srcData.getId()); + sourceVO.setState(Volume.State.Ready); + volDao.update(sourceVO.getId(), sourceVO); + if (destinationVO.getId() != sourceVO.getId()) { + destinationVO.setState(Volume.State.Expunged); + destinationVO.setRemoved(new Date()); + volDao.update(destinationVO.getId(), destinationVO); + } + } + @Override public void copyAsync(Map volumeMap, VirtualMachineTO vmTo, Host srcHost, Host destHost, AsyncCompletionCallback callback) { Answer answer = null; diff --git a/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/api/command/ListAndSwitchSAMLAccountCmd.java b/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/api/command/ListAndSwitchSAMLAccountCmd.java index 4851973c2ad..5263ade6b37 100644 --- a/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/api/command/ListAndSwitchSAMLAccountCmd.java +++ b/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/api/command/ListAndSwitchSAMLAccountCmd.java @@ -81,10 +81,10 @@ public class ListAndSwitchSAMLAccountCmd extends BaseCmd implements APIAuthentic //////////////// API parameters ///////////////////// ///////////////////////////////////////////////////// - @Parameter(name = ApiConstants.USER_ID, type = CommandType.UUID, entityType = UserResponse.class, required = false, description = "User uuid") + @Parameter(name = ApiConstants.USER_ID, type = CommandType.UUID, entityType = UserResponse.class, description = "User uuid") private Long userId; - @Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.UUID, entityType = DomainResponse.class, required = false, description = "Domain uuid") + @Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.UUID, entityType = DomainResponse.class, description = "Domain uuid") private Long domainId; @Override @@ -131,10 +131,12 @@ public class ListAndSwitchSAMLAccountCmd extends BaseCmd implements APIAuthentic } if (userUuid != null && domainUuid != null) { + logger.debug("User [" + currentUserAccount.getUsername() + "] is requesting to switch from user profile [" + currentUserAccount.getId() + "] to useraccount [" + userUuid + "] in domain [" + domainUuid + "]"); final User user = _userDao.findByUuid(userUuid); final Domain domain = _domainDao.findByUuid(domainUuid); final UserAccount nextUserAccount = _accountService.getUserAccountById(user.getId()); if (nextUserAccount != null && !nextUserAccount.getAccountState().equals(Account.State.ENABLED.toString())) { + logger.warn("User [" + currentUserAccount.getUsername() + "] is requesting to switch from user profile [" + currentUserId + "] to user profile [" + userUuid + "] in domain [" + domainUuid + "] but the associated target account [" + nextUserAccount.getAccountName() + "] is not enabled"); throw new ServerApiException(ApiErrorCode.ACCOUNT_ERROR, _apiServer.getSerializedApiError(ApiErrorCode.PARAM_ERROR.getHttpCode(), "The requested user account is locked and cannot be switched to, please contact your administrator.", params, responseType)); @@ -145,25 +147,31 @@ public class ListAndSwitchSAMLAccountCmd extends BaseCmd implements APIAuthentic || !nextUserAccount.getExternalEntity().equals(currentUserAccount.getExternalEntity()) || (nextUserAccount.getDomainId() != domain.getId()) || (nextUserAccount.getSource() != User.Source.SAML2)) { + logger.warn("User [" + currentUserAccount.getUsername() + "] is requesting to switch from user profile [" + currentUserId + "] to user profile [" + userUuid + "] in domain [" + domainUuid + "] but the associated target account is not found or invalid"); throw new ServerApiException(ApiErrorCode.PARAM_ERROR, _apiServer.getSerializedApiError(ApiErrorCode.PARAM_ERROR.getHttpCode(), "User account is not allowed to switch to the requested account", params, responseType)); } try { if (_apiServer.verifyUser(nextUserAccount.getId())) { + logger.info("User [" + currentUserAccount.getUsername() + "] user profile switch is accepted: from [" + currentUserId + "] to user profile [" + userUuid + "] in domain [" + domainUuid + "] with account [" + nextUserAccount.getAccountName() + "]"); + // need to set a sessoin variable to inform the login function of the specific user to login as, rather than using email only (which could have multiple matches) + session.setAttribute("nextUserId", user.getId()); final LoginCmdResponse loginResponse = (LoginCmdResponse) _apiServer.loginUser(session, nextUserAccount.getUsername(), nextUserAccount.getUsername() + nextUserAccount.getSource().toString(), nextUserAccount.getDomainId(), null, remoteAddress, params); SAMLUtils.setupSamlUserCookies(loginResponse, resp); - resp.sendRedirect(SAML2AuthManager.SAMLCloudStackRedirectionUrl.value()); + session.removeAttribute("nextUserId"); + logger.debug("User [" + currentUserAccount.getUsername() + "] user profile switch cookies set: from [" + currentUserId + "] to user profile [" + userUuid + "] in domain [" + domainUuid + "] with account [" + nextUserAccount.getAccountName() + "]"); + //resp.sendRedirect(SAML2AuthManager.SAMLCloudStackRedirectionUrl.value()); return ApiResponseSerializer.toSerializedString(loginResponse, responseType); } } catch (CloudAuthenticationException | IOException exception) { - logger.debug("Failed to switch to request SAML user account due to: " + exception.getMessage()); + logger.debug("User [{}] user profile switch cookies set FAILED: from [{}] to user profile [{}] in domain [{}] with account [{}]", currentUserAccount.getUsername(), currentUserId, userUuid, domainUuid, nextUserAccount.getAccountName(), exception); } } else { List switchableAccounts = _userAccountDao.getAllUsersByNameAndEntity(currentUserAccount.getUsername(), currentUserAccount.getExternalEntity()); - if (switchableAccounts != null && switchableAccounts.size() > 0 && currentUserId != User.UID_SYSTEM) { - List accountResponses = new ArrayList(); + if (switchableAccounts != null && !switchableAccounts.isEmpty() && currentUserId != User.UID_SYSTEM) { + List accountResponses = new ArrayList<>(); for (UserAccountVO userAccount: switchableAccounts) { User user = _userDao.getUser(userAccount.getId()); Domain domain = _domainService.getDomain(userAccount.getDomainId()); @@ -176,8 +184,9 @@ public class ListAndSwitchSAMLAccountCmd extends BaseCmd implements APIAuthentic accountResponse.setAccountName(userAccount.getAccountName()); accountResponse.setIdpId(user.getExternalEntity()); accountResponses.add(accountResponse); + logger.debug("Returning available useraccount for [{}]: UserUUID: [{}], DomainUUID: [{}], Account: [{}]", currentUserAccount.getUsername(), user.getUuid(), domain.getUuid(), userAccount.getAccountName()); } - ListResponse response = new ListResponse(); + ListResponse response = new ListResponse<>(); response.setResponses(accountResponses); response.setResponseName(getCommandName()); return ApiResponseSerializer.toSerializedString(response, responseType); @@ -196,7 +205,7 @@ public class ListAndSwitchSAMLAccountCmd extends BaseCmd implements APIAuthentic @Override public void setAuthenticators(List authenticators) { for (PluggableAPIAuthenticator authManager: authenticators) { - if (authManager != null && authManager instanceof SAML2AuthManager) { + if (authManager instanceof SAML2AuthManager) { _samlAuthManager = (SAML2AuthManager) authManager; } } diff --git a/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/api/command/SAML2LoginAPIAuthenticatorCmd.java b/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/api/command/SAML2LoginAPIAuthenticatorCmd.java index 742680fdcc5..bfd47922142 100644 --- a/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/api/command/SAML2LoginAPIAuthenticatorCmd.java +++ b/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/api/command/SAML2LoginAPIAuthenticatorCmd.java @@ -78,7 +78,7 @@ import com.cloud.user.UserAccountVO; import com.cloud.user.dao.UserAccountDao; import com.cloud.utils.db.EntityManager; -@APICommand(name = "samlSso", description = "SP initiated SAML Single Sign On", requestHasSensitiveInfo = true, responseObject = LoginCmdResponse.class, entityType = {}) +@APICommand(name = "samlSso", description = "SP initiated SAML Single Sign On", responseObject = LoginCmdResponse.class) public class SAML2LoginAPIAuthenticatorCmd extends BaseCmd implements APIAuthenticator, Configurable { private static final String s_name = "loginresponse"; @@ -97,7 +97,7 @@ public class SAML2LoginAPIAuthenticatorCmd extends BaseCmd implements APIAuthent @Inject private UserAccountDao userAccountDao; - protected static ConfigKey saml2FailedLoginRedirectUrl = new ConfigKey("Advanced", String.class, "saml2.failed.login.redirect.url", "", + protected static ConfigKey saml2FailedLoginRedirectUrl = new ConfigKey<>("Advanced", String.class, "saml2.failed.login.redirect.url", "", "The URL to redirect the SAML2 login failed message (the default vaulue is empty).", true); SAML2AuthManager samlAuthManager; @@ -190,7 +190,7 @@ public class SAML2LoginAPIAuthenticatorCmd extends BaseCmd implements APIAuthent String authnId = SAMLUtils.generateSecureRandomId(); samlAuthManager.saveToken(authnId, domainPath, idpMetadata.getEntityId()); logger.debug("Sending SAMLRequest id=" + authnId); - String redirectUrl = SAMLUtils.buildAuthnRequestUrl(authnId, spMetadata, idpMetadata, SAML2AuthManager.SAMLSignatureAlgorithm.value()); + String redirectUrl = SAMLUtils.buildAuthnRequestUrl(authnId, spMetadata, idpMetadata, SAML2AuthManager.SAMLSignatureAlgorithm.value(), SAML2AuthManager.SAMLRequirePasswordLogin.value()); resp.sendRedirect(redirectUrl); return ""; } if (params.containsKey("SAMLart")) { @@ -207,7 +207,7 @@ public class SAML2LoginAPIAuthenticatorCmd extends BaseCmd implements APIAuthent params, responseType)); } - String username = null; + String username; Issuer issuer = processedSAMLResponse.getIssuer(); SAMLProviderMetadata spMetadata = samlAuthManager.getSPMetadata(); SAMLProviderMetadata idpMetadata = samlAuthManager.getIdPMetadata(issuer.getValue()); @@ -273,7 +273,7 @@ public class SAML2LoginAPIAuthenticatorCmd extends BaseCmd implements APIAuthent try { assertion = decrypter.decrypt(encryptedAssertion); } catch (DecryptionException e) { - logger.warn("SAML EncryptedAssertion error: " + e.toString()); + logger.warn("SAML EncryptedAssertion error: " + e); } if (assertion == null) { continue; @@ -310,7 +310,7 @@ public class SAML2LoginAPIAuthenticatorCmd extends BaseCmd implements APIAuthent UserAccount userAccount = null; List possibleUserAccounts = userAccountDao.getAllUsersByNameAndEntity(username, issuer.getValue()); - if (possibleUserAccounts != null && possibleUserAccounts.size() > 0) { + if (possibleUserAccounts != null && !possibleUserAccounts.isEmpty()) { // Log into the first enabled user account // Users can switch to other allowed accounts later for (UserAccountVO possibleUserAccount : possibleUserAccounts) { @@ -370,7 +370,7 @@ public class SAML2LoginAPIAuthenticatorCmd extends BaseCmd implements APIAuthent @Override public void setAuthenticators(List authenticators) { for (PluggableAPIAuthenticator authManager: authenticators) { - if (authManager != null && authManager instanceof SAML2AuthManager) { + if (authManager instanceof SAML2AuthManager) { samlAuthManager = (SAML2AuthManager) authManager; } } diff --git a/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAML2AuthManager.java b/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAML2AuthManager.java index 4e8ba16c739..523f694d80b 100644 --- a/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAML2AuthManager.java +++ b/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAML2AuthManager.java @@ -79,6 +79,10 @@ public interface SAML2AuthManager extends PluggableAPIAuthenticator, PluggableSe ConfigKey SAMLUserSessionKeyPathAttribute = new ConfigKey("Advanced", String.class, "saml2.user.sessionkey.path", "", "The Path attribute of sessionkey cookie when SAML users have logged in. If not set, it will be set to the path of SAML redirection URL (saml2.redirect.url).", true); + ConfigKey SAMLRequirePasswordLogin = new ConfigKey("Advanced", Boolean.class, "saml2.require.password", "true", + "When enabled SAML2 will validate that the SAML login was performed with a password. If disabled, other forms of authentication are allowed (two-factor, certificate, etc) on the SAML Authentication Provider", true); + + SAMLProviderMetadata getSPMetadata(); SAMLProviderMetadata getIdPMetadata(String entityId); Collection getAllIdPMetadata(); diff --git a/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java b/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java index 545b01a4a31..93b7bc5be93 100644 --- a/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java +++ b/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java @@ -541,6 +541,6 @@ public class SAML2AuthManagerImpl extends AdapterBase implements SAML2AuthManage SAMLCloudStackRedirectionUrl, SAMLUserAttributeName, SAMLIdentityProviderMetadataURL, SAMLDefaultIdentityProviderId, SAMLSignatureAlgorithm, SAMLAppendDomainSuffix, SAMLTimeout, SAMLCheckSignature, - SAMLForceAuthn, SAMLUserSessionKeyPathAttribute}; + SAMLForceAuthn, SAMLUserSessionKeyPathAttribute, SAMLRequirePasswordLogin}; } } diff --git a/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAMLUtils.java b/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAMLUtils.java index 364a43e93c0..1a9d677d43a 100644 --- a/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAMLUtils.java +++ b/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAMLUtils.java @@ -152,11 +152,11 @@ public class SAMLUtils { return null; } - public static String buildAuthnRequestUrl(final String authnId, final SAMLProviderMetadata spMetadata, final SAMLProviderMetadata idpMetadata, final String signatureAlgorithm) { + public static String buildAuthnRequestUrl(final String authnId, final SAMLProviderMetadata spMetadata, final SAMLProviderMetadata idpMetadata, final String signatureAlgorithm, boolean requirePasswordAuthentication) { String redirectUrl = ""; try { DefaultBootstrap.bootstrap(); - AuthnRequest authnRequest = SAMLUtils.buildAuthnRequestObject(authnId, spMetadata.getEntityId(), idpMetadata.getSsoUrl(), spMetadata.getSsoUrl()); + AuthnRequest authnRequest = SAMLUtils.buildAuthnRequestObject(authnId, spMetadata.getEntityId(), idpMetadata.getSsoUrl(), spMetadata.getSsoUrl(), requirePasswordAuthentication); PrivateKey privateKey = null; if (spMetadata.getKeyPair() != null) { privateKey = spMetadata.getKeyPair().getPrivate(); @@ -169,13 +169,36 @@ public class SAMLUtils { return redirectUrl; } - public static AuthnRequest buildAuthnRequestObject(final String authnId, final String spId, final String idpUrl, final String consumerUrl) { + public static AuthnRequest buildAuthnRequestObject(final String authnId, final String spId, final String idpUrl, final String consumerUrl, boolean requirePasswordAuthentication) { // Issuer object IssuerBuilder issuerBuilder = new IssuerBuilder(); Issuer issuer = issuerBuilder.buildObject(); issuer.setValue(spId); - // AuthnContextClass + // Creation of AuthRequestObject + AuthnRequestBuilder authRequestBuilder = new AuthnRequestBuilder(); + AuthnRequest authnRequest = authRequestBuilder.buildObject(); + + // AuthnContextClass. When this is false, the authentication requirements are defered to the SAML IDP and its default or configured workflow + if (requirePasswordAuthentication) { + setRequestedAuthnContext(authnRequest, requirePasswordAuthentication); + } + + authnRequest.setID(authnId); + authnRequest.setDestination(idpUrl); + authnRequest.setVersion(SAMLVersion.VERSION_20); + authnRequest.setForceAuthn(SAML2AuthManager.SAMLForceAuthn.value()); + authnRequest.setIsPassive(false); + authnRequest.setIssueInstant(new DateTime()); + authnRequest.setProtocolBinding(SAMLConstants.SAML2_POST_BINDING_URI); + authnRequest.setAssertionConsumerServiceURL(consumerUrl); + authnRequest.setProviderName(spId); + authnRequest.setIssuer(issuer); + + return authnRequest; + } + + public static void setRequestedAuthnContext(AuthnRequest authnRequest, boolean requirePasswordAuthentication) { AuthnContextClassRefBuilder authnContextClassRefBuilder = new AuthnContextClassRefBuilder(); AuthnContextClassRef authnContextClassRef = authnContextClassRefBuilder.buildObject( SAMLConstants.SAML20_NS, @@ -187,23 +210,7 @@ public class SAMLUtils { RequestedAuthnContext requestedAuthnContext = requestedAuthnContextBuilder.buildObject(); requestedAuthnContext.setComparison(AuthnContextComparisonTypeEnumeration.EXACT); requestedAuthnContext.getAuthnContextClassRefs().add(authnContextClassRef); - - // Creation of AuthRequestObject - AuthnRequestBuilder authRequestBuilder = new AuthnRequestBuilder(); - AuthnRequest authnRequest = authRequestBuilder.buildObject(); - authnRequest.setID(authnId); - authnRequest.setDestination(idpUrl); - authnRequest.setVersion(SAMLVersion.VERSION_20); - authnRequest.setForceAuthn(SAML2AuthManager.SAMLForceAuthn.value()); - authnRequest.setIsPassive(false); - authnRequest.setIssueInstant(new DateTime()); - authnRequest.setProtocolBinding(SAMLConstants.SAML2_POST_BINDING_URI); - authnRequest.setAssertionConsumerServiceURL(consumerUrl); - authnRequest.setProviderName(spId); - authnRequest.setIssuer(issuer); authnRequest.setRequestedAuthnContext(requestedAuthnContext); - - return authnRequest; } public static LogoutRequest buildLogoutRequest(String logoutUrl, String spId, String nameIdString) { @@ -285,23 +292,6 @@ public class SAMLUtils { } public static void setupSamlUserCookies(final LoginCmdResponse loginResponse, final HttpServletResponse resp) throws IOException { - resp.addCookie(new Cookie("userid", URLEncoder.encode(loginResponse.getUserId(), HttpUtils.UTF_8))); - resp.addCookie(new Cookie("domainid", URLEncoder.encode(loginResponse.getDomainId(), HttpUtils.UTF_8))); - resp.addCookie(new Cookie("role", URLEncoder.encode(loginResponse.getType(), HttpUtils.UTF_8))); - resp.addCookie(new Cookie("username", URLEncoder.encode(loginResponse.getUsername(), HttpUtils.UTF_8))); - resp.addCookie(new Cookie("account", URLEncoder.encode(loginResponse.getAccount(), HttpUtils.UTF_8))); - resp.addCookie(new Cookie("isSAML", URLEncoder.encode("true", HttpUtils.UTF_8))); - resp.addCookie(new Cookie("twoFaEnabled", URLEncoder.encode(loginResponse.is2FAenabled(), HttpUtils.UTF_8))); - String providerFor2FA = loginResponse.getProviderFor2FA(); - if (StringUtils.isNotEmpty(providerFor2FA)) { - resp.addCookie(new Cookie("twoFaProvider", URLEncoder.encode(loginResponse.getProviderFor2FA(), HttpUtils.UTF_8))); - } - String timezone = loginResponse.getTimeZone(); - if (timezone != null) { - resp.addCookie(new Cookie("timezone", URLEncoder.encode(timezone, HttpUtils.UTF_8))); - } - resp.addCookie(new Cookie("userfullname", URLEncoder.encode(loginResponse.getFirstName() + " " + loginResponse.getLastName(), HttpUtils.UTF_8).replace("+", "%20"))); - String redirectUrl = SAML2AuthManager.SAMLCloudStackRedirectionUrl.value(); String path = SAML2AuthManager.SAMLUserSessionKeyPathAttribute.value(); String domain = null; @@ -317,6 +307,18 @@ public class SAMLUtils { } catch (URISyntaxException ex) { throw new CloudRuntimeException("Invalid URI: " + redirectUrl); } + + addBaseCookies(loginResponse, resp, domain, path); + + String providerFor2FA = loginResponse.getProviderFor2FA(); + if (StringUtils.isNotEmpty(providerFor2FA)) { + resp.addCookie(newCookie(domain, path,"twoFaProvider", URLEncoder.encode(loginResponse.getProviderFor2FA(), HttpUtils.UTF_8))); + } + String timezone = loginResponse.getTimeZone(); + if (timezone != null) { + resp.addCookie(newCookie(domain, path,"timezone", URLEncoder.encode(timezone, HttpUtils.UTF_8))); + } + String sameSite = ApiServlet.getApiSessionKeySameSite(); String sessionKeyCookie = String.format("%s=%s;Domain=%s;Path=%s;%s", ApiConstants.SESSIONKEY, loginResponse.getSessionKey(), domain, path, sameSite); LOGGER.debug("Adding sessionkey cookie to response: " + sessionKeyCookie); @@ -324,6 +326,24 @@ public class SAMLUtils { resp.addHeader("SET-COOKIE", String.format("%s=%s;HttpOnly;Path=/client/api;%s", ApiConstants.SESSIONKEY, loginResponse.getSessionKey(), sameSite)); } + private static void addBaseCookies(final LoginCmdResponse loginResponse, final HttpServletResponse resp, String domain, String path) throws IOException { + resp.addCookie(newCookie(domain, path, "userid", URLEncoder.encode(loginResponse.getUserId(), HttpUtils.UTF_8))); + resp.addCookie(newCookie(domain, path,"domainid", URLEncoder.encode(loginResponse.getDomainId(), HttpUtils.UTF_8))); + resp.addCookie(newCookie(domain, path,"role", URLEncoder.encode(loginResponse.getType(), HttpUtils.UTF_8))); + resp.addCookie(newCookie(domain, path,"username", URLEncoder.encode(loginResponse.getUsername(), HttpUtils.UTF_8))); + resp.addCookie(newCookie(domain, path,"account", URLEncoder.encode(loginResponse.getAccount(), HttpUtils.UTF_8))); + resp.addCookie(newCookie(domain, path,"isSAML", URLEncoder.encode("true", HttpUtils.UTF_8))); + resp.addCookie(newCookie(domain, path,"twoFaEnabled", URLEncoder.encode(loginResponse.is2FAenabled(), HttpUtils.UTF_8))); + resp.addCookie(newCookie(domain, path,"userfullname", URLEncoder.encode(loginResponse.getFirstName() + " " + loginResponse.getLastName(), HttpUtils.UTF_8).replace("+", "%20"))); + } + + private static Cookie newCookie(final String domain, final String path, final String name, final String value) { + Cookie cookie = new Cookie(name, value); + cookie.setDomain(domain); + cookie.setPath(path); + return cookie; + } + /** * Returns base64 encoded PublicKey * @param key PublicKey diff --git a/plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/SAMLUtilsTest.java b/plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/SAMLUtilsTest.java index 752845edb64..891d028aebf 100644 --- a/plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/SAMLUtilsTest.java +++ b/plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/SAMLUtilsTest.java @@ -58,7 +58,7 @@ public class SAMLUtilsTest extends TestCase { String idpUrl = "http://idp.domain.example"; String spId = "cloudstack"; String authnId = SAMLUtils.generateSecureRandomId(); - AuthnRequest req = SAMLUtils.buildAuthnRequestObject(authnId, spId, idpUrl, consumerUrl); + AuthnRequest req = SAMLUtils.buildAuthnRequestObject(authnId, spId, idpUrl, consumerUrl, true); assertEquals(req.getAssertionConsumerServiceURL(), consumerUrl); assertEquals(req.getDestination(), idpUrl); assertEquals(req.getIssuer().getValue(), spId); @@ -86,7 +86,7 @@ public class SAMLUtilsTest extends TestCase { idpMetadata.setSsoUrl(idpUrl); idpMetadata.setEntityId(idpId); - URI redirectUrl = new URI(SAMLUtils.buildAuthnRequestUrl(authnId, spMetadata, idpMetadata, SAML2AuthManager.SAMLSignatureAlgorithm.value())); + URI redirectUrl = new URI(SAMLUtils.buildAuthnRequestUrl(authnId, spMetadata, idpMetadata, SAML2AuthManager.SAMLSignatureAlgorithm.value(), true)); assertThat(redirectUrl).hasScheme(urlScheme).hasHost(idpDomain).hasParameter("SAMLRequest"); assertEquals(urlScheme, redirectUrl.getScheme()); assertEquals(idpDomain, redirectUrl.getHost()); @@ -115,7 +115,7 @@ public class SAMLUtilsTest extends TestCase { idpMetadata.setSsoUrl(idpUrl); idpMetadata.setEntityId(idpId); - URI redirectUrl = new URI(SAMLUtils.buildAuthnRequestUrl(authnId, spMetadata, idpMetadata, SAML2AuthManager.SAMLSignatureAlgorithm.value())); + URI redirectUrl = new URI(SAMLUtils.buildAuthnRequestUrl(authnId, spMetadata, idpMetadata, SAML2AuthManager.SAMLSignatureAlgorithm.value(), true)); assertThat(redirectUrl).hasScheme(urlScheme).hasHost(idpDomain).hasParameter("idpid").hasParameter("SAMLRequest"); assertEquals(urlScheme, redirectUrl.getScheme()); assertEquals(idpDomain, redirectUrl.getHost()); diff --git a/plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/api/command/ListAndSwitchSAMLAccountCmdTest.java b/plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/api/command/ListAndSwitchSAMLAccountCmdTest.java index 9225574ff60..afc2f82b206 100644 --- a/plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/api/command/ListAndSwitchSAMLAccountCmdTest.java +++ b/plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/api/command/ListAndSwitchSAMLAccountCmdTest.java @@ -213,7 +213,6 @@ public class ListAndSwitchSAMLAccountCmdTest extends TestCase { loginCmdResponse.set2FAenabled("false"); Mockito.when(apiServer.loginUser(nullable(HttpSession.class), nullable(String.class), nullable(String.class), nullable(Long.class), nullable(String.class), nullable(InetAddress.class), nullable(Map.class))).thenReturn(loginCmdResponse); - Mockito.doNothing().when(resp).sendRedirect(nullable(String.class)); try { cmd.authenticate("command", params, session, null, HttpUtils.RESPONSE_TYPE_JSON, new StringBuilder(), req, resp); } catch (ServerApiException exception) { @@ -221,7 +220,6 @@ public class ListAndSwitchSAMLAccountCmdTest extends TestCase { } finally { // accountService should have been called 4 times by now, for this case twice and 2 for cases above Mockito.verify(accountService, Mockito.times(4)).getUserAccountById(Mockito.anyLong()); - Mockito.verify(resp, Mockito.times(1)).sendRedirect(anyString()); } } diff --git a/server/src/main/java/com/cloud/api/ApiServer.java b/server/src/main/java/com/cloud/api/ApiServer.java index df72a2719c2..b8227ef9d58 100644 --- a/server/src/main/java/com/cloud/api/ApiServer.java +++ b/server/src/main/java/com/cloud/api/ApiServer.java @@ -196,6 +196,7 @@ import static org.apache.cloudstack.user.UserPasswordResetManager.UserPasswordRe @Component public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiServerService, Configurable { + private static final Logger ACCESSLOGGER = LogManager.getLogger("apiserver." + ApiServer.class.getName()); private static final String SANITIZATION_REGEX = "[\n\r]"; @@ -240,9 +241,9 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer private EventDistributor eventDistributor = null; private static int s_workerCount = 0; - private static Map>> s_apiNameCmdClassMap = new HashMap>>(); + private static Map>> s_apiNameCmdClassMap = new HashMap<>(); - private static ExecutorService s_executor = new ThreadPoolExecutor(10, 150, 60, TimeUnit.SECONDS, new LinkedBlockingQueue(), new NamedThreadFactory( + private static ExecutorService s_executor = new ThreadPoolExecutor(10, 150, 60, TimeUnit.SECONDS, new LinkedBlockingQueue<>(), new NamedThreadFactory( "ApiServer")); @Inject @@ -358,7 +359,7 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer String jobEvent = eventInfo.second(); if (logger.isTraceEnabled()) - logger.trace("Handle asyjob publish event " + jobEvent); + logger.trace("Handle asyjob publish event {}", jobEvent); if (eventDistributor == null) { setEventDistributor(ComponentContext.getComponent(EventDistributor.class)); } @@ -383,7 +384,7 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer cmdEventType = eventTypeObj; if (logger.isDebugEnabled()) - logger.debug("Retrieved cmdEventType from job info: " + cmdEventType); + logger.debug("Retrieved cmdEventType from job info: {}", cmdEventType); } else { if (logger.isDebugEnabled()) logger.debug("Unable to locate cmdEventType marker in job info. publish as unknown event"); @@ -427,11 +428,11 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer protected void setupIntegrationPortListener(Integer apiPort) { if (apiPort == null || apiPort <= 0) { - logger.trace(String.format("Skipping setting up listener for integration port as %s is set to %d", - IntegrationAPIPort.key(), apiPort)); + logger.trace("Skipping setting up listener for integration port as {} is set to {}", + IntegrationAPIPort.key(), apiPort); return; } - logger.debug(String.format("Setting up integration API service listener on port: %d", apiPort)); + logger.debug("Setting up integration API service listener on port: {}", apiPort); final ListenerThread listenerThread = new ListenerThread(this, apiPort); listenerThread.start(); } @@ -442,24 +443,24 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer Integer apiPort = IntegrationAPIPort.value(); // api port, null by default final Long snapshotLimit = ConcurrentSnapshotsThresholdPerHost.value(); - if (snapshotLimit == null || snapshotLimit.longValue() <= 0) { + if (snapshotLimit == null || snapshotLimit <= 0) { logger.debug("Global concurrent snapshot config parameter " + ConcurrentSnapshotsThresholdPerHost.value() + " is less or equal 0; defaulting to unlimited"); } else { dispatcher.setCreateSnapshotQueueSizeLimit(snapshotLimit); } final Long migrationLimit = VolumeApiService.ConcurrentMigrationsThresholdPerDatastore.value(); - if (migrationLimit == null || migrationLimit.longValue() <= 0) { + if (migrationLimit == null || migrationLimit <= 0) { logger.debug("Global concurrent migration config parameter " + VolumeApiService.ConcurrentMigrationsThresholdPerDatastore.value() + " is less or equal 0; defaulting to unlimited"); } else { dispatcher.setMigrateQueueSizeLimit(migrationLimit); } - final Set> cmdClasses = new HashSet>(); + final Set> cmdClasses = new HashSet<>(); for (final PluggableService pluggableService : pluggableServices) { cmdClasses.addAll(pluggableService.getCommands()); if (logger.isDebugEnabled()) { - logger.debug("Discovered plugin " + pluggableService.getClass().getSimpleName()); + logger.debug("Discovered plugin {}", pluggableService.getClass().getSimpleName()); } } @@ -472,7 +473,7 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer String apiName = at.name(); List> apiCmdList = s_apiNameCmdClassMap.get(apiName); if (apiCmdList == null) { - apiCmdList = new ArrayList>(); + apiCmdList = new ArrayList<>(); s_apiNameCmdClassMap.put(apiName, apiCmdList); } apiCmdList.add(cmdClass); @@ -574,14 +575,14 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer throw e; } } finally { - logger.info(sb.toString()); + ACCESSLOGGER.info(sb.toString()); CallContext.unregister(); } } @SuppressWarnings({"unchecked", "rawtypes"}) public void checkCharacterInkParams(final Map params) { - final Map stringMap = new HashMap(); + final Map stringMap = new HashMap<>(); final Set keys = params.keySet(); final Iterator keysIter = keys.iterator(); while (keysIter.hasNext()) { @@ -604,7 +605,7 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer public String handleRequest(final Map params, final String responseType, final StringBuilder auditTrailSb) throws ServerApiException { checkCharacterInkParams(params); - String response = null; + String response; String[] command = null; try { @@ -625,7 +626,7 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer if (authManager.getAPIAuthenticator(command[0]) != null) { return null; } - final Map paramMap = new HashMap(); + final Map paramMap = new HashMap<>(); final Set keys = params.keySet(); final Iterator keysIter = keys.iterator(); while (keysIter.hasNext()) { @@ -641,16 +642,16 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer if (cmdClass != null) { APICommand annotation = cmdClass.getAnnotation(APICommand.class); if (annotation == null) { - logger.error("No APICommand annotation found for class " + cmdClass.getCanonicalName()); + logger.error("No APICommand annotation found for class {}", cmdClass.getCanonicalName()); throw new CloudRuntimeException("No APICommand annotation found for class " + cmdClass.getCanonicalName()); } - BaseCmd cmdObj = (BaseCmd)cmdClass.newInstance(); + BaseCmd cmdObj = (BaseCmd)cmdClass.getDeclaredConstructor().newInstance(); cmdObj = ComponentContext.inject(cmdObj); cmdObj.configure(); cmdObj.setFullUrlParams(paramMap); cmdObj.setResponseType(responseType); - cmdObj.setHttpMethod(paramMap.get(ApiConstants.HTTPMETHOD).toString()); + cmdObj.setHttpMethod(paramMap.get(ApiConstants.HTTPMETHOD)); // This is where the command is either serialized, or directly dispatched StringBuilder log = new StringBuilder(); @@ -659,14 +660,11 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer } else { final String errorString = "Unknown API command: " + command[0]; logger.warn(errorString); - auditTrailSb.append(" " + errorString); + auditTrailSb.append(" ").append(errorString); throw new ServerApiException(ApiErrorCode.UNSUPPORTED_ACTION_ERROR, errorString); } } - } catch (final InvalidParameterValueException ex) { - logger.info(ex.getMessage()); - throw new ServerApiException(ApiErrorCode.PARAM_ERROR, ex.getMessage(), ex); - } catch (final IllegalArgumentException ex) { + } catch (final InvalidParameterValueException | IllegalArgumentException ex) { logger.info(ex.getMessage()); throw new ServerApiException(ApiErrorCode.PARAM_ERROR, ex.getMessage(), ex); } catch (final PermissionDeniedException ex) { @@ -679,9 +677,9 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer buf.append(obj.getUuid()); buf.append(" "); } - logger.info("PermissionDenied: " + ex.getMessage() + " on objs: [" + buf.toString() + "]"); + logger.info("PermissionDenied: " + ex.getMessage() + " on objs: [" + buf + "]"); } else { - logger.info("PermissionDenied: " + ex.getMessage()); + logger.info("PermissionDenied: {}", ex.getMessage()); } throw new ServerApiException(ApiErrorCode.ACCOUNT_ERROR, ex.getMessage(), ex); } catch (final AccountLimitException ex) { @@ -756,7 +754,7 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer throw new ServerApiException(ApiErrorCode.SERVICE_UNAVAILABLE, msg); } Long objectId = null; - String objectUuid = null; + String objectUuid; if (cmdObj instanceof BaseAsyncCreateCmd) { final BaseAsyncCreateCmd createCmd = (BaseAsyncCreateCmd)cmdObj; dispatcher.dispatchCreateCmd(createCmd, params); @@ -797,7 +795,7 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer } params.put("ctxStartEventId", String.valueOf(startEventId)); - params.put("cmdEventType", asyncCmd.getEventType().toString()); + params.put("cmdEventType", asyncCmd.getEventType()); params.put("ctxDetails", ApiGsonHelper.getBuilder().create().toJson(ctx.getContextParameters())); if (asyncCmd.getHttpMethod() != null) { params.put(ApiConstants.HTTPMETHOD, asyncCmd.getHttpMethod().toString()); @@ -860,9 +858,9 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer @SuppressWarnings("unchecked") private void buildAsyncListResponse(final BaseListCmd command, final Account account) { - final List responses = ((ListResponse)command.getResponseObject()).getResponses(); - if (responses != null && responses.size() > 0) { - List jobs = null; + final List responses = ((ListResponse)command.getResponseObject()).getResponses(); + if (responses != null && !responses.isEmpty()) { + List jobs; // list all jobs for ROOT admin if (accountMgr.isRootAdmin(account.getId())) { @@ -871,11 +869,11 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer jobs = asyncMgr.findInstancePendingAsyncJobs(command.getApiResourceType().toString(), account.getId()); } - if (jobs.size() == 0) { + if (jobs.isEmpty()) { return; } - final Map objectJobMap = new HashMap(); + final Map objectJobMap = new HashMap<>(); for (final AsyncJob job : jobs) { if (job.getInstanceId() == null) { continue; @@ -912,7 +910,7 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer if (Boolean.TRUE.equals(apiKeyAccessEnabled)) { return true; } else { - logger.info("Api-Key access is disabled for the User " + user.toString()); + logger.info("Api-Key access is disabled for the User {}", user); return false; } } @@ -921,7 +919,7 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer if (Boolean.TRUE.equals(apiKeyAccessEnabled)) { return true; } else { - logger.info("Api-Key access is disabled for the Account " + account.toString()); + logger.info("Api-Key access is disabled for the Account {}", account); return false; } } @@ -938,7 +936,7 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer public boolean verifyRequest(final Map requestParameters, final Long userId, InetAddress remoteAddress) throws ServerApiException { try { String apiKey = null; - String secretKey = null; + String secretKey; String signature = null; String unsignedRequest = null; @@ -966,11 +964,9 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer // - build a request string with sorted params, make sure it's all lowercase // - sign the request, verify the signature is the same - final List parameterNames = new ArrayList(); - for (final Object paramNameObj : requestParameters.keySet()) { - parameterNames.add((String)paramNameObj); // put the name in a list that we'll sort later - } + // put the name in a list that we'll sort later + final List parameterNames = new ArrayList<>(requestParameters.keySet()); Collections.sort(parameterNames); @@ -1006,7 +1002,7 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer return false; // no signature, bad request } - Date expiresTS = null; + Date expiresTS; // FIXME: Hard coded signature, why not have an enum if ("3".equals(signatureVersion)) { // New signature authentication. Check for expire parameter and its validity @@ -1026,18 +1022,18 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer if (expiresTS.before(now)) { signature = signature.replaceAll(SANITIZATION_REGEX, "_"); apiKey = apiKey.replaceAll(SANITIZATION_REGEX, "_"); - logger.debug(String.format("Request expired -- ignoring ...sig [%s], apiKey [%s].", signature, apiKey)); + logger.debug("Request expired -- ignoring ...sig [{}], apiKey [{}].", signature, apiKey); return false; } } final TransactionLegacy txn = TransactionLegacy.open(TransactionLegacy.CLOUD_DB); txn.close(); - User user = null; + User user; // verify there is a user with this api key final Pair userAcctPair = accountMgr.findUserByApiKey(apiKey); if (userAcctPair == null) { - logger.debug("apiKey does not map to a valid user -- ignoring request, apiKey: " + apiKey); + logger.debug("apiKey does not map to a valid user -- ignoring request, apiKey: {}", apiKey); return false; } @@ -1078,7 +1074,7 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer if (!equalSig) { signature = signature.replaceAll(SANITIZATION_REGEX, "_"); - logger.info(String.format("User signature [%s] is not equaled to computed signature [%s].", signature, computedSignature)); + logger.info("User signature [{}] is not equaled to computed signature [{}].", signature, computedSignature); } else { CallContext.register(user, account); } @@ -1137,10 +1133,10 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer session.removeAttribute("domain_UUID"); } - final Enumeration attrNames = session.getAttributeNames(); + final Enumeration attrNames = session.getAttributeNames(); if (attrNames != null) { while (attrNames.hasMoreElements()) { - final String attrName = (String) attrNames.nextElement(); + final String attrName = attrNames.nextElement(); final Object attrObj = session.getAttribute(attrName); if (ApiConstants.USERNAME.equalsIgnoreCase(attrName)) { response.setUsername(attrObj.toString()); @@ -1202,7 +1198,14 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer domainId = userDomain.getId(); } - UserAccount userAcct = accountMgr.authenticateUser(username, password, domainId, loginIpAddress, requestParameters); + Long userId = (Long)session.getAttribute("nextUserId"); + UserAccount userAcct = null; + if (userId != null) { + userAcct = accountMgr.getUserAccountById(userId); + } else { + userAcct = accountMgr.authenticateUser(username, password, domainId, loginIpAddress, requestParameters); + } + if (userAcct != null) { final String timezone = userAcct.getTimezone(); float offsetInHrs = 0f; @@ -1214,7 +1217,7 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer final long longDate = date.getTime(); final float offsetInMs = (t.getOffset(longDate)); offsetInHrs = offsetInMs / (1000 * 60 * 60); - logger.info("Timezone offset from UTC is: " + offsetInHrs); + logger.info("Timezone offset from UTC is: {}", offsetInHrs); } final Account account = accountMgr.getAccount(userAcct.getAccountId()); @@ -1272,7 +1275,7 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer // (bug 5483) generate a session key that the user must submit on every request to prevent CSRF, add that // to the login response so that session-based authenticators know to send the key back final SecureRandom sesssionKeyRandom = new SecureRandom(); - final byte sessionKeyBytes[] = new byte[20]; + final byte[] sessionKeyBytes = new byte[20]; sesssionKeyRandom.nextBytes(sessionKeyBytes); final String sessionKey = Base64.encodeBase64URLSafeString(sessionKeyBytes); session.setAttribute(ApiConstants.SESSIONKEY, sessionKey); @@ -1285,7 +1288,6 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer @Override public void logoutUser(final long userId) { accountMgr.logoutUser(userId); - return; } @Override @@ -1313,30 +1315,26 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer throw new CloudRuntimeException(errorMessage); } if (StringUtils.isBlank(userAccount.getEmail())) { - logger.error(String.format( - "Email is not set. username: %s account id: %d domain id: %d", - userAccount.getUsername(), userAccount.getAccountId(), userAccount.getDomainId())); + logger.error("Email is not set. username: {} account id: {} domain id: {}", + userAccount.getUsername(), userAccount.getAccountId(), userAccount.getDomainId()); throw new CloudRuntimeException("Email is not set for the user."); } if (!EnumUtils.getEnumIgnoreCase(Account.State.class, userAccount.getState()).equals(Account.State.ENABLED)) { - logger.error(String.format( - "User is not enabled. username: %s account id: %d domain id: %s", - userAccount.getUsername(), userAccount.getAccountId(), domain.getUuid())); + logger.error("User is not enabled. username: {} account id: {} domain id: {}", + userAccount.getUsername(), userAccount.getAccountId(), domain.getUuid()); throw new CloudRuntimeException("User is not enabled."); } if (!EnumUtils.getEnumIgnoreCase(Account.State.class, userAccount.getAccountState()).equals(Account.State.ENABLED)) { - logger.error(String.format( - "Account is not enabled. username: %s account id: %d domain id: %s", - userAccount.getUsername(), userAccount.getAccountId(), domain.getUuid())); + logger.error("Account is not enabled. username: {} account id: {} domain id: {}", + userAccount.getUsername(), userAccount.getAccountId(), domain.getUuid()); throw new CloudRuntimeException("Account is not enabled."); } if (!domain.getState().equals(Domain.State.Active)) { - logger.error(String.format( - "Domain is not active. username: %s account id: %d domain id: %s", - userAccount.getUsername(), userAccount.getAccountId(), domain.getUuid())); + logger.error("Domain is not active. username: {} account id: {} domain id: {}", + userAccount.getUsername(), userAccount.getAccountId(), domain.getUuid()); throw new CloudRuntimeException("Domain is not active."); } @@ -1444,7 +1442,7 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer // code to be very specific to our needs static class ListenerThread extends Thread { - private static Logger LOGGER = LogManager.getLogger(ListenerThread.class); + private static final Logger LOGGER = LogManager.getLogger(ListenerThread.class); private HttpService _httpService = null; private ServerSocket _serverSocket = null; private HttpParams _params = null; @@ -1483,7 +1481,7 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer @Override public void run() { - LOGGER.info("ApiServer listening on port " + _serverSocket.getLocalPort()); + LOGGER.info("ApiServer listening on port {}", _serverSocket.getLocalPort()); while (!Thread.interrupted()) { try { // Set up HTTP connection @@ -1526,10 +1524,10 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer } } catch (final IOException ex) { if (logger.isTraceEnabled()) { - logger.trace("ApiServer: IOException - " + ex); + logger.trace("ApiServer: IOException - {}", ex.toString()); } } catch (final HttpException ex) { - logger.warn("ApiServer: Unrecoverable HTTP protocol violation" + ex); + logger.warn("ApiServer: Unrecoverable HTTP protocol violation {}", ex.toString()); } finally { try { _conn.shutdown(); @@ -1542,7 +1540,7 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer @Override public String getSerializedApiError(final int errorCode, final String errorText, final Map apiCommandParams, final String responseType) { String responseName = null; - Class cmdClass = null; + Class cmdClass; String responseText = null; try { @@ -1555,7 +1553,7 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer final String cmdName = ((String[])cmdObj)[0]; cmdClass = getCmdClass(cmdName); if (cmdClass != null) { - responseName = ((BaseCmd)cmdClass.newInstance()).getCommandName(); + responseName = ((BaseCmd)cmdClass.getDeclaredConstructor().newInstance()).getCommandName(); } else { responseName = "errorresponse"; } @@ -1577,7 +1575,7 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer @Override public String getSerializedApiError(final ServerApiException ex, final Map apiCommandParams, final String responseType) { String responseName = null; - Class cmdClass = null; + Class cmdClass; String responseText = null; if (ex == null) { @@ -1595,7 +1593,7 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer final String cmdName = ((String[])cmdObj)[0]; cmdClass = getCmdClass(cmdName); if (cmdClass != null) { - responseName = ((BaseCmd)cmdClass.newInstance()).getCommandName(); + responseName = ((BaseCmd)cmdClass.getDeclaredConstructor().newInstance()).getCommandName(); } else { responseName = "errorresponse"; } @@ -1607,8 +1605,8 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer apiResponse.setResponseName(responseName); final ArrayList idList = ex.getIdProxyList(); if (idList != null) { - for (int i = 0; i < idList.size(); i++) { - apiResponse.addProxyObject(idList.get(i)); + for (ExceptionProxyObject exceptionProxyObject : idList) { + apiResponse.addProxyObject(exceptionProxyObject); } } // Also copy over the cserror code and the function/layer in which diff --git a/server/src/main/java/com/cloud/api/ApiServlet.java b/server/src/main/java/com/cloud/api/ApiServlet.java index e2ff411f8f4..4994c42bb4d 100644 --- a/server/src/main/java/com/cloud/api/ApiServlet.java +++ b/server/src/main/java/com/cloud/api/ApiServlet.java @@ -21,7 +21,6 @@ import java.net.InetAddress; import java.net.URLDecoder; import java.net.UnknownHostException; import java.util.Arrays; -import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -76,9 +75,7 @@ import com.cloud.utils.net.NetUtils; @Component("apiServlet") public class ApiServlet extends HttpServlet { protected static Logger LOGGER = LogManager.getLogger(ApiServlet.class); - private final static List s_clientAddressHeaders = Collections - .unmodifiableList(Arrays.asList("X-Forwarded-For", - "HTTP_CLIENT_IP", "HTTP_X_FORWARDED_FOR", "Remote_Addr")); + private static final Logger ACCESSLOGGER = LogManager.getLogger("apiserver." + ApiServlet.class.getName()); private static final String REPLACEMENT = "_"; private static final String LOGGER_REPLACEMENTS = "[\n\r\t]"; @@ -374,7 +371,7 @@ public class ApiServlet extends HttpServlet { LOGGER.error("unknown exception writing api response", ex); auditTrailSb.append(" unknown exception writing api response"); } finally { - LOGGER.info(auditTrailSb.toString()); + ACCESSLOGGER.info(auditTrailSb.toString()); if (LOGGER.isDebugEnabled()) { LOGGER.debug("===END=== " + reqStr); } diff --git a/server/src/main/java/com/cloud/api/query/dao/TemplateJoinDaoImpl.java b/server/src/main/java/com/cloud/api/query/dao/TemplateJoinDaoImpl.java index 0bdf5040c82..ac24f14b781 100644 --- a/server/src/main/java/com/cloud/api/query/dao/TemplateJoinDaoImpl.java +++ b/server/src/main/java/com/cloud/api/query/dao/TemplateJoinDaoImpl.java @@ -151,11 +151,6 @@ public class TemplateJoinDaoImpl extends GenericDaoBaseWithTagInformation interfaceMTUs = validateMtuConfig(publicMtu, privateMtu, zone.getId()); mtuCheckForVpcNetwork(vpcId, interfaceMTUs, publicMtu); diff --git a/server/src/main/java/com/cloud/network/element/ConfigDriveNetworkElement.java b/server/src/main/java/com/cloud/network/element/ConfigDriveNetworkElement.java index 47c9979c2f6..5f1c1e58d93 100644 --- a/server/src/main/java/com/cloud/network/element/ConfigDriveNetworkElement.java +++ b/server/src/main/java/com/cloud/network/element/ConfigDriveNetworkElement.java @@ -203,8 +203,8 @@ public class ConfigDriveNetworkElement extends AdapterBase implements NetworkEle private static Map> setCapabilities() { Map> capabilities = new HashMap<>(); capabilities.put(Service.UserData, null); - capabilities.put(Service.Dhcp, new HashMap<>()); - capabilities.put(Service.Dns, new HashMap<>()); + capabilities.put(Service.Dhcp, Map.of(Network.Capability.DhcpAccrossMultipleSubnets, "true")); + capabilities.put(Service.Dns, Map.of(Capability.AllowDnsSuffixModification, "true")); return capabilities; } @@ -841,7 +841,7 @@ public class ConfigDriveNetworkElement extends AdapterBase implements NetworkEle public boolean configDhcpSupportForSubnet(Network network, NicProfile nic, VirtualMachineProfile vm, DeployDestination dest, ReservationContext context) throws ConcurrentOperationException, InsufficientCapacityException, ResourceUnavailableException { - return false; + return true; } @Override diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index 1a25f36efe2..622f73c1e37 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -3740,7 +3740,7 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic VolumeApiResult result = future.get(); if (result.isFailed()) { logger.debug("migrate volume failed:" + result.getResult()); - throw new StorageUnavailableException("Migrate volume failed: " + result.getResult(), destPool.getId()); + throw new CloudRuntimeException("Migrate volume failed: " + result.getResult()); } return result.getVolume(); } catch (InterruptedException e) { @@ -4117,7 +4117,9 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic Optional extractUrl = setExtractVolumeSearchCriteria(sc, volume); if (extractUrl.isPresent()) { - return extractUrl.get(); + String url = extractUrl.get(); + CallContext.current().setEventDetails(String.format("Download URL: %s, volume ID: %s", url, volume.getUuid())); + return url; } VMInstanceVO vm = null; @@ -4134,7 +4136,9 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic VmWorkJobVO placeHolder = null; placeHolder = createPlaceHolderWork(vm.getId()); try { - return orchestrateExtractVolume(volume.getId(), zoneId); + String url = orchestrateExtractVolume(volume.getId(), zoneId); + CallContext.current().setEventDetails(String.format("Download URL: %s, volume ID: %s", url, volume.getUuid())); + return url; } finally { _workJobDao.expunge(placeHolder.getId()); } @@ -4163,13 +4167,17 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic // retrieve the entity url from job result if (jobResult != null && jobResult instanceof String) { - return (String)jobResult; + String url = (String) jobResult; + CallContext.current().setEventDetails(String.format("Download URL: %s, volume ID: %s", url, volume.getUuid())); + return url; } return null; } } - return orchestrateExtractVolume(volume.getId(), zoneId); + String url = orchestrateExtractVolume(volume.getId(), zoneId); + CallContext.current().setEventDetails(String.format("Download URL: %s, volume ID: %s", url, volume.getUuid())); + return url; } @Override diff --git a/server/src/main/java/com/cloud/storage/download/DownloadActiveState.java b/server/src/main/java/com/cloud/storage/download/DownloadActiveState.java index 989e0b55f94..889ffbc0b1c 100644 --- a/server/src/main/java/com/cloud/storage/download/DownloadActiveState.java +++ b/server/src/main/java/com/cloud/storage/download/DownloadActiveState.java @@ -76,7 +76,7 @@ public abstract class DownloadActiveState extends DownloadState { getDownloadListener().log("handleTimeout, updateMs=" + updateMs + ", curr state= " + getName(), Level.TRACE); } String newState = getName(); - if (updateMs > 5 * DownloadListener.STATUS_POLL_INTERVAL) { + if (updateMs > DownloadListener.DOWNLOAD_TIMEOUT) { newState = Status.DOWNLOAD_ERROR.toString(); getDownloadListener().log("timeout: transitioning to download error state, currstate=" + getName(), Level.DEBUG); } else if (updateMs > 3 * DownloadListener.STATUS_POLL_INTERVAL) { diff --git a/server/src/main/java/com/cloud/storage/download/DownloadListener.java b/server/src/main/java/com/cloud/storage/download/DownloadListener.java index e06a31d96b5..42b0e394db4 100644 --- a/server/src/main/java/com/cloud/storage/download/DownloadListener.java +++ b/server/src/main/java/com/cloud/storage/download/DownloadListener.java @@ -100,6 +100,7 @@ public class DownloadListener implements Listener { protected Logger logger = LogManager.getLogger(getClass()); public static final int SMALL_DELAY = 100; public static final long STATUS_POLL_INTERVAL = 10000L; + public static final long DOWNLOAD_TIMEOUT = 5 * STATUS_POLL_INTERVAL; public static final String DOWNLOADED = Status.DOWNLOADED.toString(); public static final String NOT_DOWNLOADED = Status.NOT_DOWNLOADED.toString(); diff --git a/server/src/main/java/com/cloud/template/HypervisorTemplateAdapter.java b/server/src/main/java/com/cloud/template/HypervisorTemplateAdapter.java index fbf70a8eaad..f7eb654141d 100644 --- a/server/src/main/java/com/cloud/template/HypervisorTemplateAdapter.java +++ b/server/src/main/java/com/cloud/template/HypervisorTemplateAdapter.java @@ -293,7 +293,7 @@ public class HypervisorTemplateAdapter extends TemplateAdapterBase { } /** - * For each zone ID in {@link TemplateProfile#zoneIdList}, verifies if there is active heuristic rules for allocating template and returns the + * For each zone ID in {@link TemplateProfile#getZoneIdList()}, verifies if there is active heuristic rules for allocating template and returns the * {@link DataStore} returned by the heuristic rule. If there is not an active heuristic rule, then allocate it to a random {@link DataStore}, if the ISO/template is private * or allocate it to all {@link DataStore} in the zone, if it is public. * @param profile @@ -453,10 +453,10 @@ public class HypervisorTemplateAdapter extends TemplateAdapterBase { /** * If the template/ISO is marked as private, then it is allocated to a random secondary storage; otherwise, allocates to every storage pool in every zone given by the - * {@link TemplateProfile#zoneIdList}. + * {@link TemplateProfile#getZoneIdList()}. */ private void postUploadAllocation(List imageStores, VMTemplateVO template, List payloads) { - Set zoneSet = new HashSet(); + Set zoneSet = new HashSet<>(); Collections.shuffle(imageStores); for (DataStore imageStore : imageStores) { Long zoneId_is = imageStore.getScope().getScopeId(); @@ -697,8 +697,8 @@ public class HypervisorTemplateAdapter extends TemplateAdapterBase { } // delete all cache entries for this template - List cacheTmpls = imageFactory.listTemplateOnCache(template.getId()); - for (TemplateInfo tmplOnCache : cacheTmpls) { + List cachedTemplates = imageFactory.listTemplateOnCache(template.getId()); + for (TemplateInfo tmplOnCache : cachedTemplates) { logger.info("Delete template: {} from image cache store: {}", tmplOnCache, tmplOnCache.getDataStore()); tmplOnCache.delete(); } @@ -727,27 +727,32 @@ public class HypervisorTemplateAdapter extends TemplateAdapterBase { } // remove its related ACL permission - Pair, Long> tmplt = new Pair, Long>(VirtualMachineTemplate.class, template.getId()); - _messageBus.publish(_name, EntityManager.MESSAGE_REMOVE_ENTITY_EVENT, PublishScope.LOCAL, tmplt); - - checkAndRemoveTemplateDetails(template); - - // Remove comments (if any) - AnnotationService.EntityType entityType = template.getFormat().equals(ImageFormat.ISO) ? - AnnotationService.EntityType.ISO : AnnotationService.EntityType.TEMPLATE; - annotationDao.removeByEntityType(entityType.name(), template.getUuid()); + Pair, Long> templateClassForId = new Pair<>(VirtualMachineTemplate.class, template.getId()); + _messageBus.publish(_name, EntityManager.MESSAGE_REMOVE_ENTITY_EVENT, PublishScope.LOCAL, templateClassForId); + List zoneRegistrations = templateZoneDao.listByTemplateId(template.getId()); + if (zoneRegistrations.isEmpty()) { + removeTemplateDetails(template); + removeTemplateAnnotations(template); + } } return success; } + private void removeTemplateAnnotations(VMTemplateVO template) { + // Remove comments (if any) + AnnotationService.EntityType entityType = template.getFormat().equals(ImageFormat.ISO) ? + AnnotationService.EntityType.ISO : AnnotationService.EntityType.TEMPLATE; + annotationDao.removeByEntityType(entityType.name(), template.getUuid()); + } + /** * removes details of the template and * if the template is registered as deploy as is, * then it also deletes the details related to deploy as is only if there are no VMs using the template * @param template */ - void checkAndRemoveTemplateDetails(VMTemplateVO template) { + private void removeTemplateDetails(VMTemplateVO template) { templateDetailsDao.removeDetails(template.getId()); if (template.isDeployAsIs()) { diff --git a/server/src/main/java/com/cloud/template/TemplateManagerImpl.java b/server/src/main/java/com/cloud/template/TemplateManagerImpl.java index 6073b4f0bb7..9f23bdef142 100755 --- a/server/src/main/java/com/cloud/template/TemplateManagerImpl.java +++ b/server/src/main/java/com/cloud/template/TemplateManagerImpl.java @@ -488,7 +488,9 @@ public class TemplateManagerImpl extends ManagerBase implements TemplateManager, String mode = cmd.getMode(); Long eventId = cmd.getStartEventId(); - return extract(account, templateId, url, zoneId, mode, eventId, true); + String extractUrl = extract(account, templateId, url, zoneId, mode, eventId, true); + CallContext.current().setEventDetails(String.format("Download URL: %s, ISO ID: %s", extractUrl, _tmpltDao.findById(templateId).getUuid())); + return extractUrl; } @Override @@ -506,7 +508,9 @@ public class TemplateManagerImpl extends ManagerBase implements TemplateManager, throw new InvalidParameterValueException("unable to find template with id " + templateId); } - return extract(caller, templateId, url, zoneId, mode, eventId, false); + String extractUrl = extract(caller, templateId, url, zoneId, mode, eventId, false); + CallContext.current().setEventDetails(String.format("Download URL: %s, template ID: %s", extractUrl, template.getUuid())); + return extractUrl; } @Override diff --git a/server/src/main/java/com/cloud/user/AccountManagerImpl.java b/server/src/main/java/com/cloud/user/AccountManagerImpl.java index a3ae3fff01f..4913b9a56b5 100644 --- a/server/src/main/java/com/cloud/user/AccountManagerImpl.java +++ b/server/src/main/java/com/cloud/user/AccountManagerImpl.java @@ -386,6 +386,14 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M true, ConfigKey.Scope.Domain); + static ConfigKey userAllowMultipleAccounts = new ConfigKey<>("Advanced", + Boolean.class, + "user.allow.multiple.accounts", + "false", + "Determines if the same username can be added to more than one account in the same domain (SAML-only).", + true, + ConfigKey.Scope.Domain); + protected AccountManagerImpl() { super(); } @@ -1289,7 +1297,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M // Check permissions checkAccess(getCurrentCallingAccount(), domain); - if (!_userAccountDao.validateUsernameInDomain(userName, domainId)) { + if (!userAllowMultipleAccounts.valueInDomain(domainId) && !_userAccountDao.validateUsernameInDomain(userName, domainId)) { throw new InvalidParameterValueException(String.format("The user %s already exists in domain %s", userName, domain)); } @@ -1477,9 +1485,15 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M throw new PermissionDeniedException(String.format("Account: %s is a system account, can't add a user to it", account)); } - if (!_userAccountDao.validateUsernameInDomain(userName, domainId)) { - throw new CloudRuntimeException(String.format("The user %s already exists in domain %s", userName, domain)); + if (!userAllowMultipleAccounts.valueInDomain(domainId) && !_userAccountDao.validateUsernameInDomain(userName, domainId)) { + throw new CloudRuntimeException("The user " + userName + " already exists in domain " + domainId); } + List duplicatedUsers = _userDao.findUsersByName(userName); + for (UserVO duplicatedUser : duplicatedUsers) { + // users can't exist in same account + assertUserNotAlreadyInAccount(duplicatedUser, account); + } + UserVO user; user = createUser(account.getId(), userName, password, firstName, lastName, email, timeZone, userUUID, source); return user; @@ -1607,7 +1621,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M *
  • The username must be unique in each domain. Therefore, if there is already another user with the same username, an {@link InvalidParameterValueException} is thrown. * */ - protected void validateAndUpdateUsernameIfNeeded(UpdateUserCmd updateUserCmd, UserVO user, Account account) { + protected void validateAndUpdateUsernameIfNeeded(UpdateUserCmd updateUserCmd, UserVO newUser, Account newAccount) { String userName = updateUserCmd.getUsername(); if (userName == null) { return; @@ -1615,18 +1629,21 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M if (StringUtils.isBlank(userName)) { throw new InvalidParameterValueException("Username cannot be empty."); } - List duplicatedUsers = _userDao.findUsersByName(userName); - for (UserVO duplicatedUser : duplicatedUsers) { - if (duplicatedUser.getId() == user.getId()) { + List existingUsers = _userDao.findUsersByName(userName); + for (UserVO existingUser : existingUsers) { + if (existingUser.getId() == newUser.getId()) { continue; } - Account duplicatedUserAccountWithUserThatHasTheSameUserName = _accountDao.findById(duplicatedUser.getAccountId()); - if (duplicatedUserAccountWithUserThatHasTheSameUserName.getDomainId() == account.getDomainId()) { - DomainVO domain = _domainDao.findById(duplicatedUserAccountWithUserThatHasTheSameUserName.getDomainId()); - throw new InvalidParameterValueException(String.format("Username (%s) already exists in domain (%s)", duplicatedUser, domain)); + + // duplicate usernames cannot exist in same domain unless explicitly configured + if (!userAllowMultipleAccounts.valueInDomain(newAccount.getDomainId())) { + assertUserNotAlreadyInDomain(existingUser, newAccount); } + + // can't rename a username to an existing one in the same account + assertUserNotAlreadyInAccount(existingUser, newAccount); } - user.setUsername(userName); + newUser.setUsername(userName); } /** @@ -1895,7 +1912,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M // make sure the account is enabled too // if the user is either locked already or disabled already, don't change state...only lock currently enabled -// users + // users boolean success; if (user.getState().equals(State.LOCKED)) { // already locked...no-op @@ -3408,7 +3425,8 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M @Override public ConfigKey[] getConfigKeys() { return new ConfigKey[] {UseSecretKeyInResponse, enableUserTwoFactorAuthentication, - userTwoFactorAuthenticationDefaultProvider, mandateUserTwoFactorAuthentication, userTwoFactorAuthenticationIssuer, apiKeyAccess}; + userTwoFactorAuthenticationDefaultProvider, mandateUserTwoFactorAuthentication, userTwoFactorAuthenticationIssuer, apiKeyAccess, + userAllowMultipleAccounts}; } public List getUserTwoFactorAuthenticationProviders() { @@ -3593,4 +3611,21 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M return userAccountVO; }); } + + void assertUserNotAlreadyInAccount(User existingUser, Account newAccount) { + System.out.println(existingUser.getAccountId()); + System.out.println(newAccount.getId()); + if (existingUser.getAccountId() == newAccount.getId()) { + AccountVO existingAccount = _accountDao.findById(newAccount.getId()); + throw new InvalidParameterValueException(String.format("Username [%s] already exists in account [id=%s,name=%s]", existingUser.getUsername(), existingAccount.getUuid(), existingAccount.getAccountName())); + } + } + + void assertUserNotAlreadyInDomain(User existingUser, Account originalAccount) { + Account existingAccount = _accountDao.findById(existingUser.getAccountId()); + if (existingAccount.getDomainId() == originalAccount.getDomainId()) { + DomainVO existingDomain = _domainDao.findById(existingAccount.getDomainId()); + throw new InvalidParameterValueException(String.format("Username [%s] already exists in domain [id=%s,name=%s] user account [id=%s,name=%s]", existingUser.getUsername(), existingDomain.getUuid(), existingDomain.getName(), existingAccount.getUuid(), existingAccount.getAccountName())); + } + } } diff --git a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java index 2a5f1b15dcf..a1310a8bb1e 100644 --- a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java +++ b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java @@ -1408,4 +1408,75 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase { Assert.assertNull(updatedUser.getUser2faProvider()); Assert.assertNull(updatedUser.getKeyFor2fa()); } + + @Test(expected = InvalidParameterValueException.class) + public void testAssertUserNotAlreadyInAccount_UserExistsInAccount() { + User existingUser = new UserVO(); + existingUser.setUsername("testuser"); + existingUser.setAccountId(1L); + + Account newAccount = Mockito.mock(Account.class); + Mockito.when(newAccount.getId()).thenReturn(1L); + + AccountVO existingAccount = Mockito.mock(AccountVO.class); + Mockito.when(existingAccount.getUuid()).thenReturn("existing-account-uuid"); + Mockito.when(existingAccount.getAccountName()).thenReturn("existing-account"); + + Mockito.when(_accountDao.findById(1L)).thenReturn(existingAccount); + + accountManagerImpl.assertUserNotAlreadyInAccount(existingUser, newAccount); + } + + @Test + public void testAssertUserNotAlreadyInAccount_UserExistsInDiffAccount() { + User existingUser = new UserVO(); + existingUser.setUsername("testuser"); + existingUser.setAccountId(2L); + + Account newAccount = Mockito.mock(Account.class); + Mockito.when(newAccount.getId()).thenReturn(1L); + + accountManagerImpl.assertUserNotAlreadyInAccount(existingUser, newAccount); + } + + @Test(expected = InvalidParameterValueException.class) + public void testAssertUserNotAlreadyInDomain_UserExistsInDomain() { + User existingUser = new UserVO(); + existingUser.setUsername("testuser"); + existingUser.setAccountId(1L); + + Account originalAccount = Mockito.mock(Account.class); + Mockito.when(originalAccount.getDomainId()).thenReturn(1L); + + AccountVO existingAccount = Mockito.mock(AccountVO.class); + Mockito.when(existingAccount.getDomainId()).thenReturn(1L); + Mockito.when(existingAccount.getUuid()).thenReturn("existing-account-uuid"); + Mockito.when(existingAccount.getAccountName()).thenReturn("existing-account"); + + DomainVO existingDomain = Mockito.mock(DomainVO.class); + Mockito.when(existingDomain.getUuid()).thenReturn("existing-domain-uuid"); + Mockito.when(existingDomain.getName()).thenReturn("existing-domain"); + + Mockito.when(_accountDao.findById(1L)).thenReturn(existingAccount); + Mockito.when(_domainDao.findById(1L)).thenReturn(existingDomain); + + accountManagerImpl.assertUserNotAlreadyInDomain(existingUser, originalAccount); + } + + @Test + public void testAssertUserNotAlreadyInDomain_UserExistsInDiffDomain() { + User existingUser = new UserVO(); + existingUser.setUsername("testuser"); + existingUser.setAccountId(1L); + + Account originalAccount = Mockito.mock(Account.class); + Mockito.when(originalAccount.getDomainId()).thenReturn(1L); + + AccountVO existingAccount = Mockito.mock(AccountVO.class); + Mockito.when(existingAccount.getDomainId()).thenReturn(2L); + + Mockito.when(_accountDao.findById(1L)).thenReturn(existingAccount); + + accountManagerImpl.assertUserNotAlreadyInDomain(existingUser, originalAccount); + } } diff --git a/ui/src/components/header/SamlDomainSwitcher.vue b/ui/src/components/header/SamlDomainSwitcher.vue index 1d820dcbcff..082bab7bf13 100644 --- a/ui/src/components/header/SamlDomainSwitcher.vue +++ b/ui/src/components/header/SamlDomainSwitcher.vue @@ -88,6 +88,7 @@ export default { this.showSwitcher = false return } + this.samlAccounts = samlAccounts this.samlAccounts = _.orderBy(samlAccounts, ['domainPath'], ['asc']) const currentAccount = this.samlAccounts.filter(x => { return x.userId === store.getters.userInfo.id @@ -109,6 +110,8 @@ export default { this.$message.success(`Switched to "${account.accountName} (${account.domainPath})"`) this.$router.go() }) + }).else(error => { + console.log('error refreshing with new user context: ' + error) }) } } diff --git a/ui/src/components/view/InfoCard.vue b/ui/src/components/view/InfoCard.vue index 6ecf4885ce5..23fb5164404 100644 --- a/ui/src/components/view/InfoCard.vue +++ b/ui/src/components/view/InfoCard.vue @@ -646,7 +646,7 @@ {{ resource.podname || resource.pod || resource.podid }} -
    +
    {{ $t('label.zone') }}
    @@ -733,7 +733,7 @@ {{ resource.managementserver || resource.managementserverid }}
    -
    +
    {{ $t('label.created') }}
    {{ $toLocaleDate(resource.created) }} diff --git a/ui/src/store/modules/user.js b/ui/src/store/modules/user.js index 1dacc09a54e..1dbb4136de1 100644 --- a/ui/src/store/modules/user.js +++ b/ui/src/store/modules/user.js @@ -331,7 +331,7 @@ const user = { commit('SET_MS_ID', msId) // Ensuring we get the user info so that store.getters.user is never empty when the page is freshly loaded - api('listUsers', { username: Cookies.get('username'), listall: true }).then(response => { + api('listUsers', { id: Cookies.get('userid'), listall: true }).then(response => { const result = response.listusersresponse.user[0] commit('SET_INFO', result) commit('SET_NAME', result.firstname + ' ' + result.lastname) @@ -404,7 +404,7 @@ const user = { }).catch(ignored => {}) } - api('listUsers', { username: Cookies.get('username') }).then(response => { + api('listUsers', { id: Cookies.get('userid') }).then(response => { const result = response.listusersresponse.user[0] commit('SET_INFO', result) commit('SET_NAME', result.firstname + ' ' + result.lastname) diff --git a/ui/src/views/image/IsoZones.vue b/ui/src/views/image/IsoZones.vue index daf1e7e21e0..75eac8fd97f 100644 --- a/ui/src/views/image/IsoZones.vue +++ b/ui/src/views/image/IsoZones.vue @@ -48,6 +48,9 @@ {{ $t('label.yes') }} {{ $t('label.no') }} + +