From b631da2542e3cdc373f4accbd612e11c6c6b89cc Mon Sep 17 00:00:00 2001 From: Kshitij Kansal Date: Tue, 28 Jul 2015 17:38:47 +0530 Subject: [PATCH] Coverity Issue: Null Pointer Dereferencing fixed and Test cases added Signed-off-by: wilderrodrigues This closes #628 --- .../cloudstack/saml/SAML2AuthManagerImpl.java | 27 +++++-- .../saml/SAML2AuthManagerImplTest.java | 75 +++++++++++++++++++ 2 files changed, 96 insertions(+), 6 deletions(-) create mode 100755 plugins/user-authenticators/saml2/test/org/apache/cloudstack/saml/SAML2AuthManagerImplTest.java 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 7232ac9e074..b1449c1a644 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 class SAML2AuthManagerImpl extends AdapterBase implements SAML2AuthManage return super.stop(); } - private boolean initSP() { + protected boolean initSP() { KeystoreVO keyStoreVO = _ksDao.findByName(SAMLPluginConstants.SAMLSP_KEYPAIR); if (keyStoreVO == null) { try { @@ -338,6 +342,7 @@ public class SAML2AuthManagerImpl extends AdapterBase implements SAML2AuthManage return; } s_logger.debug("Starting SAML IDP Metadata Refresh Task"); + Map metadataMap = new HashMap(); try { discoverAndAddIdp(_idpMetaDataProvider.getMetadata(), metadataMap); @@ -358,7 +363,7 @@ public class SAML2AuthManagerImpl extends AdapterBase implements SAML2AuthManage } _timer = new Timer(); final HttpClient client = new HttpClient(); - final String idpMetaDataUrl = SAMLIdentityProviderMetadataURL.value(); + final String idpMetaDataUrl = getSAMLIdentityProviderMetadataURL(); if (SAMLTimeout.value() != null && SAMLTimeout.value() > SAMLPluginConstants.SAML_REFRESH_INTERVAL) { _refreshInterval = SAMLTimeout.value(); } @@ -368,21 +373,31 @@ public class SAML2AuthManagerImpl extends AdapterBase implements SAML2AuthManage _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("Provided Metadata is not a URL, Unable to locate metadata file from local path: " + idpMetaDataUrl); + 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()); _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 new file mode 100755 index 00000000000..8e811d0c5b6 --- /dev/null +++ b/plugins/user-authenticators/saml2/test/org/apache/cloudstack/saml/SAML2AuthManagerImplTest.java @@ -0,0 +1,75 @@ +// 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; +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); + + } +}