From aa0bd53162bd3f4450e5e7915e3b1f9e898fe55d Mon Sep 17 00:00:00 2001 From: Kshitij Kansal Date: Thu, 23 Jul 2015 21:25:17 +0530 Subject: [PATCH 1/5] Dereference NULL return value --- .../apache/cloudstack/saml/SAML2AuthManagerImpl.java | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/plugins/user-authenticators/saml2/src/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java b/plugins/user-authenticators/saml2/src/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java index 7232ac9e074f..9b5ad99c6127 100644 --- a/plugins/user-authenticators/saml2/src/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java +++ b/plugins/user-authenticators/saml2/src/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java @@ -368,8 +368,14 @@ private boolean setup() { _idpMetaDataProvider = new HTTPMetadataProvider(_timer, client, idpMetaDataUrl); } else { File metadataFile = PropertiesUtil.findConfigFile(idpMetaDataUrl); - s_logger.debug("Provided Metadata is not a URL, trying to read metadata file from local path: " + metadataFile.getAbsolutePath()); - _idpMetaDataProvider = new FilesystemMetadataProvider(_timer, metadataFile); + if (metadataFile == null) { + s_logger.error("Metadata file returned null"); + return false; + } + else{ + s_logger.debug("Provided Metadata is not a URL, trying to read metadata file from local path: " + metadataFile.getAbsolutePath()); + _idpMetaDataProvider = new FilesystemMetadataProvider(_timer, metadataFile); + } } _idpMetaDataProvider.setRequireValidMetadata(true); _idpMetaDataProvider.setParserPool(new BasicParserPool()); From 27b966265001d0b79fd2b1b6538a25a9c4452b1d Mon Sep 17 00:00:00 2001 From: Kshitij Kansal Date: Fri, 24 Jul 2015 02:23:44 +0530 Subject: [PATCH 2/5] Dereference null return value: Suggested changes made --- .../src/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/plugins/user-authenticators/saml2/src/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java b/plugins/user-authenticators/saml2/src/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java index 9b5ad99c6127..c0eaca074e1b 100644 --- a/plugins/user-authenticators/saml2/src/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java +++ b/plugins/user-authenticators/saml2/src/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java @@ -338,6 +338,7 @@ public void run() { return; } s_logger.debug("Starting SAML IDP Metadata Refresh Task"); + Map metadataMap = new HashMap(); try { discoverAndAddIdp(_idpMetaDataProvider.getMetadata(), metadataMap); @@ -369,7 +370,7 @@ private boolean setup() { } else { File metadataFile = PropertiesUtil.findConfigFile(idpMetaDataUrl); if (metadataFile == null) { - s_logger.error("Metadata file returned null"); + s_logger.error("Provided Metadata is not a URL, Unable to locate metadata file from local path: " + idpMetaDataUrl); return false; } else{ From 434a617e5fd029599d53e90085c4da4dbdf7c1a5 Mon Sep 17 00:00:00 2001 From: Kshitij Kansal Date: Fri, 24 Jul 2015 02:30:18 +0530 Subject: [PATCH 3/5] Dereference null return value --- .../src/com/cloud/api/ApiResponseHelper.java | 32 +++++++++++++------ 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/server/src/com/cloud/api/ApiResponseHelper.java b/server/src/com/cloud/api/ApiResponseHelper.java index 99a80f8a44fb..249be24009d6 100644 --- a/server/src/com/cloud/api/ApiResponseHelper.java +++ b/server/src/com/cloud/api/ApiResponseHelper.java @@ -505,7 +505,8 @@ public SnapshotResponse createSnapshotResponse(Snapshot snapshot) { List tagResponses = new ArrayList(); for (ResourceTag tag : tags) { ResourceTagResponse tagResponse = createResourceTagResponse(tag, true); - tagResponses.add(tagResponse); + if(tagResponse != null) + tagResponses.add(tagResponse); } snapshotResponse.setTags(tagResponses); @@ -790,7 +791,8 @@ public IPAddressResponse createIPAddressResponse(ResponseView view, IpAddress ip List tagResponses = new ArrayList(); for (ResourceTag tag : tags) { ResourceTagResponse tagResponse = createResourceTagResponse(tag, true); - tagResponses.add(tagResponse); + if(tagResponse != null) + tagResponses.add(tagResponse); } ipResponse.setTags(tagResponses); @@ -832,7 +834,8 @@ public LoadBalancerResponse createLoadBalancerResponse(LoadBalancer loadBalancer List tagResponses = new ArrayList(); for (ResourceTag tag : tags) { ResourceTagResponse tagResponse = createResourceTagResponse(tag, true); - tagResponses.add(tagResponse); + if(tagResponse != null) + tagResponses.add(tagResponse); } lbResponse.setTags(tagResponses); @@ -1119,7 +1122,8 @@ public FirewallRuleResponse createPortForwardingRuleResponse(PortForwardingRule List tagResponses = new ArrayList(); for (ResourceTag tag : tags) { ResourceTagResponse tagResponse = createResourceTagResponse(tag, true); - tagResponses.add(tagResponse); + if(tagResponse != null) + tagResponses.add(tagResponse); } response.setTags(tagResponses); @@ -2072,7 +2076,8 @@ public NetworkResponse createNetworkResponse(ResponseView view, Network network) List tagResponses = new ArrayList(); for (ResourceTag tag : tags) { ResourceTagResponse tagResponse = createResourceTagResponse(tag, true); - tagResponses.add(tagResponse); + if(tagResponse != null) + tagResponses.add(tagResponse); } response.setTags(tagResponses); @@ -2158,7 +2163,8 @@ public FirewallResponse createFirewallResponse(FirewallRule fwRule) { List tagResponses = new ArrayList(); for (ResourceTag tag : tags) { ResourceTagResponse tagResponse = createResourceTagResponse(tag, true); - tagResponses.add(tagResponse); + if(tagResponse != null) + tagResponses.add(tagResponse); } response.setTags(tagResponses); @@ -2209,7 +2215,8 @@ public NetworkACLItemResponse createNetworkACLItemResponse(NetworkACLItem aclIte List tagResponses = new ArrayList(); for (ResourceTag tag : tags) { ResourceTagResponse tagResponse = createResourceTagResponse(tag, true); - tagResponses.add(tagResponse); + if(tagResponse != null) + tagResponses.add(tagResponse); } response.setTags(tagResponses); @@ -2645,6 +2652,8 @@ public RegionResponse createRegionResponse(Region region) { @Override public ResourceTagResponse createResourceTagResponse(ResourceTag resourceTag, boolean keyValueOnly) { ResourceTagJoinVO rto = ApiDBUtils.newResourceTagView(resourceTag); + if(rto == null) + return null; return ApiDBUtils.newResourceTagResponse(rto, keyValueOnly); } @@ -2753,7 +2762,8 @@ public VpcResponse createVpcResponse(ResponseView view, Vpc vpc) { List tagResponses = new ArrayList(); for (ResourceTag tag : tags) { ResourceTagResponse tagResponse = createResourceTagResponse(tag, true); - tagResponses.add(tagResponse); + if(tagResponse != null) + tagResponses.add(tagResponse); } response.setTags(tagResponses); response.setObjectName("vpc"); @@ -2944,7 +2954,8 @@ public StaticRouteResponse createStaticRouteResponse(StaticRoute result) { List tagResponses = new ArrayList(); for (ResourceTag tag : tags) { ResourceTagResponse tagResponse = createResourceTagResponse(tag, true); - tagResponses.add(tagResponse); + if(tagResponse != null) + tagResponses.add(tagResponse); } response.setTags(tagResponses); response.setObjectName("staticroute"); @@ -3512,7 +3523,8 @@ public ApplicationLoadBalancerResponse createLoadBalancerContainerReponse(Applic List tagResponses = new ArrayList(); for (ResourceTag tag : tags) { ResourceTagResponse tagResponse = createResourceTagResponse(tag, true); - tagResponses.add(tagResponse); + if(tagResponse != null) + tagResponses.add(tagResponse); } lbResponse.setTags(tagResponses); From ad5c33cceb48f390f6f737cba1b912e9f55e1f98 Mon Sep 17 00:00:00 2001 From: Kshitij Kansal Date: Mon, 27 Jul 2015 12:25:22 +0530 Subject: [PATCH 4/5] Tests added, used CollectionUtils.addIgnoreNull() api --- .../saml/SAML2AuthManagerImplTest.java | 60 +++++++++++++++++++ 1 file changed, 60 insertions(+) create mode 100755 plugins/user-authenticators/saml2/test/org/apache/cloudstack/saml/SAML2AuthManagerImplTest.java diff --git a/plugins/user-authenticators/saml2/test/org/apache/cloudstack/saml/SAML2AuthManagerImplTest.java b/plugins/user-authenticators/saml2/test/org/apache/cloudstack/saml/SAML2AuthManagerImplTest.java new file mode 100755 index 000000000000..59198a540284 --- /dev/null +++ b/plugins/user-authenticators/saml2/test/org/apache/cloudstack/saml/SAML2AuthManagerImplTest.java @@ -0,0 +1,60 @@ + +package org.apache.cloudstack.saml; + +import com.cloud.user.DomainManager; +import com.cloud.user.dao.UserDao; +import org.apache.cloudstack.framework.security.keystore.KeystoreDao; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.Spy; +import org.mockito.runners.MockitoJUnitRunner; + +import static org.junit.Assert.assertFalse; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.when; + + +@RunWith(MockitoJUnitRunner.class) +public class SAML2AuthManagerImplTest { + + @Mock + private KeystoreDao _ksDao; + + @Mock + private SAMLTokenDao _samlTokenDao; + + @Mock + private UserDao _userDao; + + @Mock + DomainManager _domainMgr; + + @InjectMocks + @Spy + SAML2AuthManagerImpl saml2AuthManager = new SAML2AuthManagerImpl(); + + @Before + public void setUp() { + doReturn(true).when(saml2AuthManager).isSAMLPluginEnabled(); + doReturn(true).when(saml2AuthManager).initSP(); + } + + @Test + public void testStart() { + when(saml2AuthManager.getSAMLIdentityProviderMetadataURL()).thenReturn("file://does/not/exist"); + boolean started = saml2AuthManager.start(); + assertFalse("saml2authmanager should not start as the file doesnt exist", started); + + when(saml2AuthManager.getSAMLIdentityProviderMetadataURL()).thenReturn(" "); + started = saml2AuthManager.start(); + assertFalse("saml2authmanager should not start as the file doesnt exist", started); + + when(saml2AuthManager.getSAMLIdentityProviderMetadataURL()).thenReturn(""); + started = saml2AuthManager.start(); + assertFalse("saml2authmanager should not start as the file doesnt exist", started); + + } +} \ No newline at end of file From db8de3221797047ac070a61b167b5430a3653b7e Mon Sep 17 00:00:00 2001 From: Kshitij Kansal Date: Mon, 27 Jul 2015 13:51:14 +0530 Subject: [PATCH 5/5] Updates made --- .../cloudstack/saml/SAML2AuthManagerImpl.java | 17 +++++++--- .../saml/SAML2AuthManagerImplTest.java | 19 ++++++++++-- .../src/com/cloud/api/ApiResponseHelper.java | 31 +++++++------------ 3 files changed, 41 insertions(+), 26 deletions(-) diff --git a/plugins/user-authenticators/saml2/src/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java b/plugins/user-authenticators/saml2/src/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java index c0eaca074e1b..c7828889b9dc 100644 --- a/plugins/user-authenticators/saml2/src/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java +++ b/plugins/user-authenticators/saml2/src/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java @@ -104,6 +104,10 @@ public class SAML2AuthManagerImpl extends AdapterBase implements SAML2AuthManage private int _refreshInterval = SAMLPluginConstants.SAML_REFRESH_INTERVAL; private AbstractReloadingMetadataProvider _idpMetaDataProvider; + public String getSAMLIdentityProviderMetadataURL(){ + return SAMLIdentityProviderMetadataURL.value(); + } + @Inject private KeystoreDao _ksDao; @@ -119,12 +123,12 @@ public class SAML2AuthManagerImpl extends AdapterBase implements SAML2AuthManage @Override public boolean start() { if (isSAMLPluginEnabled()) { - setup(); s_logger.info("SAML auth plugin loaded"); + return setup(); } else { s_logger.info("SAML auth plugin not enabled so not loading"); + return super.start(); } - return super.start(); } @Override @@ -135,7 +139,7 @@ public boolean stop() { return super.stop(); } - private boolean initSP() { + protected boolean initSP() { KeystoreVO keyStoreVO = _ksDao.findByName(SAMLPluginConstants.SAMLSP_KEYPAIR); if (keyStoreVO == null) { try { @@ -359,7 +363,8 @@ private boolean setup() { } _timer = new Timer(); final HttpClient client = new HttpClient(); - final String idpMetaDataUrl = SAMLIdentityProviderMetadataURL.value(); + //final String idpMetaDataUrl = SAMLIdentityProviderMetadataURL.value(); + final String idpMetaDataUrl = getSAMLIdentityProviderMetadataURL(); if (SAMLTimeout.value() != null && SAMLTimeout.value() > SAMLPluginConstants.SAML_REFRESH_INTERVAL) { _refreshInterval = SAMLTimeout.value(); } @@ -382,14 +387,18 @@ private boolean setup() { _idpMetaDataProvider.setParserPool(new BasicParserPool()); _idpMetaDataProvider.initialize(); _timer.scheduleAtFixedRate(new MetadataRefreshTask(), 0, _refreshInterval * 1000); + } catch (MetadataProviderException e) { s_logger.error("Unable to read SAML2 IDP MetaData URL, error:" + e.getMessage()); s_logger.error("SAML2 Authentication may be unavailable"); + return false; } catch (ConfigurationException | FactoryConfigurationError e) { s_logger.error("OpenSAML bootstrapping failed: error: " + e.getMessage()); + return false; } catch (NullPointerException e) { s_logger.error("Unable to setup SAML Auth Plugin due to NullPointerException" + " please check the SAML global settings: " + e.getMessage()); + return false; } return true; } diff --git a/plugins/user-authenticators/saml2/test/org/apache/cloudstack/saml/SAML2AuthManagerImplTest.java b/plugins/user-authenticators/saml2/test/org/apache/cloudstack/saml/SAML2AuthManagerImplTest.java index 59198a540284..8e811d0c5b6a 100755 --- a/plugins/user-authenticators/saml2/test/org/apache/cloudstack/saml/SAML2AuthManagerImplTest.java +++ b/plugins/user-authenticators/saml2/test/org/apache/cloudstack/saml/SAML2AuthManagerImplTest.java @@ -1,4 +1,19 @@ - +// 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.saml; import com.cloud.user.DomainManager; @@ -57,4 +72,4 @@ public void testStart() { assertFalse("saml2authmanager should not start as the file doesnt exist", started); } -} \ No newline at end of file +} diff --git a/server/src/com/cloud/api/ApiResponseHelper.java b/server/src/com/cloud/api/ApiResponseHelper.java index 249be24009d6..8116343b9d4b 100644 --- a/server/src/com/cloud/api/ApiResponseHelper.java +++ b/server/src/com/cloud/api/ApiResponseHelper.java @@ -30,6 +30,7 @@ import javax.inject.Inject; +import org.apache.commons.collections.CollectionUtils; import org.apache.log4j.Logger; import org.apache.cloudstack.acl.ControlledEntity; @@ -505,8 +506,7 @@ public SnapshotResponse createSnapshotResponse(Snapshot snapshot) { List tagResponses = new ArrayList(); for (ResourceTag tag : tags) { ResourceTagResponse tagResponse = createResourceTagResponse(tag, true); - if(tagResponse != null) - tagResponses.add(tagResponse); + CollectionUtils.addIgnoreNull(tagResponses,tagResponse); } snapshotResponse.setTags(tagResponses); @@ -791,8 +791,7 @@ public IPAddressResponse createIPAddressResponse(ResponseView view, IpAddress ip List tagResponses = new ArrayList(); for (ResourceTag tag : tags) { ResourceTagResponse tagResponse = createResourceTagResponse(tag, true); - if(tagResponse != null) - tagResponses.add(tagResponse); + CollectionUtils.addIgnoreNull(tagResponses,tagResponse); } ipResponse.setTags(tagResponses); @@ -834,8 +833,7 @@ public LoadBalancerResponse createLoadBalancerResponse(LoadBalancer loadBalancer List tagResponses = new ArrayList(); for (ResourceTag tag : tags) { ResourceTagResponse tagResponse = createResourceTagResponse(tag, true); - if(tagResponse != null) - tagResponses.add(tagResponse); + CollectionUtils.addIgnoreNull(tagResponses,tagResponse); } lbResponse.setTags(tagResponses); @@ -1122,8 +1120,7 @@ public FirewallRuleResponse createPortForwardingRuleResponse(PortForwardingRule List tagResponses = new ArrayList(); for (ResourceTag tag : tags) { ResourceTagResponse tagResponse = createResourceTagResponse(tag, true); - if(tagResponse != null) - tagResponses.add(tagResponse); + CollectionUtils.addIgnoreNull(tagResponses,tagResponse); } response.setTags(tagResponses); @@ -2076,8 +2073,7 @@ public NetworkResponse createNetworkResponse(ResponseView view, Network network) List tagResponses = new ArrayList(); for (ResourceTag tag : tags) { ResourceTagResponse tagResponse = createResourceTagResponse(tag, true); - if(tagResponse != null) - tagResponses.add(tagResponse); + CollectionUtils.addIgnoreNull(tagResponses,tagResponse); } response.setTags(tagResponses); @@ -2163,8 +2159,7 @@ public FirewallResponse createFirewallResponse(FirewallRule fwRule) { List tagResponses = new ArrayList(); for (ResourceTag tag : tags) { ResourceTagResponse tagResponse = createResourceTagResponse(tag, true); - if(tagResponse != null) - tagResponses.add(tagResponse); + CollectionUtils.addIgnoreNull(tagResponses,tagResponse); } response.setTags(tagResponses); @@ -2215,8 +2210,7 @@ public NetworkACLItemResponse createNetworkACLItemResponse(NetworkACLItem aclIte List tagResponses = new ArrayList(); for (ResourceTag tag : tags) { ResourceTagResponse tagResponse = createResourceTagResponse(tag, true); - if(tagResponse != null) - tagResponses.add(tagResponse); + CollectionUtils.addIgnoreNull(tagResponses,tagResponse); } response.setTags(tagResponses); @@ -2762,8 +2756,7 @@ public VpcResponse createVpcResponse(ResponseView view, Vpc vpc) { List tagResponses = new ArrayList(); for (ResourceTag tag : tags) { ResourceTagResponse tagResponse = createResourceTagResponse(tag, true); - if(tagResponse != null) - tagResponses.add(tagResponse); + CollectionUtils.addIgnoreNull(tagResponses,tagResponse); } response.setTags(tagResponses); response.setObjectName("vpc"); @@ -2954,8 +2947,7 @@ public StaticRouteResponse createStaticRouteResponse(StaticRoute result) { List tagResponses = new ArrayList(); for (ResourceTag tag : tags) { ResourceTagResponse tagResponse = createResourceTagResponse(tag, true); - if(tagResponse != null) - tagResponses.add(tagResponse); + CollectionUtils.addIgnoreNull(tagResponses,tagResponse); } response.setTags(tagResponses); response.setObjectName("staticroute"); @@ -3523,8 +3515,7 @@ public ApplicationLoadBalancerResponse createLoadBalancerContainerReponse(Applic List tagResponses = new ArrayList(); for (ResourceTag tag : tags) { ResourceTagResponse tagResponse = createResourceTagResponse(tag, true); - if(tagResponse != null) - tagResponses.add(tagResponse); + CollectionUtils.addIgnoreNull(tagResponses,tagResponse); } lbResponse.setTags(tagResponses);