CLOUDSTACK-9369: Restrict default login to ldap/native users - Restricts default login auth handler to ldap and native-cloudstack users - Refactors and create re-usable method to find domain by id/path - Adds unit test for refactored method in DomainManagerImpl - Adds smoke test for login handler
Signed-off-by: Rohit Yadav <rohit.ya...@shapeblue.com> Project: http://git-wip-us.apache.org/repos/asf/cloudstack/repo Commit: http://git-wip-us.apache.org/repos/asf/cloudstack/commit/566e7d9f Tree: http://git-wip-us.apache.org/repos/asf/cloudstack/tree/566e7d9f Diff: http://git-wip-us.apache.org/repos/asf/cloudstack/diff/566e7d9f Branch: refs/heads/4.7 Commit: 566e7d9fac59bae1122c1718f23e8fd3fc1f18ab Parents: d9429f6 Author: Rohit Yadav <rohit.ya...@shapeblue.com> Authored: Wed Apr 27 17:34:14 2016 +0530 Committer: Will Stevens <williamstev...@gmail.com> Committed: Fri May 27 15:00:05 2016 -0400 ---------------------------------------------------------------------- api/src/com/cloud/user/DomainService.java | 10 ++ server/src/com/cloud/api/ApiServer.java | 16 +- .../auth/DefaultLoginAPIAuthenticatorCmd.java | 13 ++ .../src/com/cloud/user/DomainManagerImpl.java | 20 +++ .../com/cloud/user/DomainManagerImplTest.java | 137 ++++++++++++++++++ .../com/cloud/user/MockDomainManagerImpl.java | 5 + test/integration/smoke/test_login.py | 145 +++++++++++++++++++ 7 files changed, 335 insertions(+), 11 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/cloudstack/blob/566e7d9f/api/src/com/cloud/user/DomainService.java ---------------------------------------------------------------------- diff --git a/api/src/com/cloud/user/DomainService.java b/api/src/com/cloud/user/DomainService.java index 4c1f93d..3ccfcbc 100644 --- a/api/src/com/cloud/user/DomainService.java +++ b/api/src/com/cloud/user/DomainService.java @@ -56,4 +56,14 @@ public interface DomainService { */ Domain findDomainByPath(String domainPath); + /** + * finds the domain by either id or provided path + * + * @param id the domain id + * @param domainPath the domain path use to lookup a domain + * + * @return domainId the long value of the domain ID, or null if no domain id exists with provided id/path + */ + Domain findDomainByIdOrPath(Long id, String domainPath); + } http://git-wip-us.apache.org/repos/asf/cloudstack/blob/566e7d9f/server/src/com/cloud/api/ApiServer.java ---------------------------------------------------------------------- diff --git a/server/src/com/cloud/api/ApiServer.java b/server/src/com/cloud/api/ApiServer.java index e5ae097..689ae9c 100644 --- a/server/src/com/cloud/api/ApiServer.java +++ b/server/src/com/cloud/api/ApiServer.java @@ -993,17 +993,11 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer final Map<String, Object[]> requestParameters) throws CloudAuthenticationException { // We will always use domainId first. If that does not exist, we will use domain name. If THAT doesn't exist // we will default to ROOT - if (domainId == null) { - if (domainPath == null || domainPath.trim().length() == 0) { - domainId = Domain.ROOT_DOMAIN; - } else { - final Domain domainObj = _domainMgr.findDomainByPath(domainPath); - if (domainObj != null) { - domainId = domainObj.getId(); - } else { // if an unknown path is passed in, fail the login call - throw new CloudAuthenticationException("Unable to find the domain from the path " + domainPath); - } - } + final Domain userDomain = _domainMgr.findDomainByIdOrPath(domainId, domainPath); + if (userDomain == null || userDomain.getId() < 1L) { + throw new CloudAuthenticationException("Unable to find the domain from the path " + domainPath); + } else { + domainId = userDomain.getId(); } final UserAccount userAcct = _accountMgr.authenticateUser(username, password, domainId, loginIpAddress, requestParameters); http://git-wip-us.apache.org/repos/asf/cloudstack/blob/566e7d9f/server/src/com/cloud/api/auth/DefaultLoginAPIAuthenticatorCmd.java ---------------------------------------------------------------------- diff --git a/server/src/com/cloud/api/auth/DefaultLoginAPIAuthenticatorCmd.java b/server/src/com/cloud/api/auth/DefaultLoginAPIAuthenticatorCmd.java index d6eac78..c83e708 100644 --- a/server/src/com/cloud/api/auth/DefaultLoginAPIAuthenticatorCmd.java +++ b/server/src/com/cloud/api/auth/DefaultLoginAPIAuthenticatorCmd.java @@ -16,6 +16,9 @@ // under the License. package com.cloud.api.auth; +import com.cloud.domain.Domain; +import com.cloud.user.User; +import com.cloud.user.UserAccount; import org.apache.cloudstack.api.ApiServerService; import com.cloud.api.response.ApiResponseSerializer; import com.cloud.exception.CloudAuthenticationException; @@ -156,6 +159,16 @@ public class DefaultLoginAPIAuthenticatorCmd extends BaseCmd implements APIAuthe if (username != null) { final String pwd = ((password == null) ? null : password[0]); try { + final Domain userDomain = _domainService.findDomainByIdOrPath(domainId, domain); + if (userDomain != null) { + domainId = userDomain.getId(); + } else { + throw new CloudAuthenticationException("Unable to find the domain from the path " + domain); + } + final UserAccount userAccount = _accountService.getActiveUserAccount(username[0], domainId); + if (userAccount == null || !(User.Source.UNKNOWN.equals(userAccount.getSource()) || User.Source.LDAP.equals(userAccount.getSource()))) { + throw new CloudAuthenticationException("User is not allowed CloudStack login"); + } return ApiResponseSerializer.toSerializedString(_apiServer.loginUser(session, username[0], pwd, domainId, domain, remoteAddress, params), responseType); } catch (final CloudAuthenticationException ex) { http://git-wip-us.apache.org/repos/asf/cloudstack/blob/566e7d9f/server/src/com/cloud/user/DomainManagerImpl.java ---------------------------------------------------------------------- diff --git a/server/src/com/cloud/user/DomainManagerImpl.java b/server/src/com/cloud/user/DomainManagerImpl.java index 912aa8b..71f9b67 100644 --- a/server/src/com/cloud/user/DomainManagerImpl.java +++ b/server/src/com/cloud/user/DomainManagerImpl.java @@ -73,6 +73,7 @@ import com.cloud.utils.exception.CloudRuntimeException; import com.cloud.utils.net.NetUtils; import com.cloud.vm.ReservationContext; import com.cloud.vm.ReservationContextImpl; +import com.google.common.base.Strings; @Component public class DomainManagerImpl extends ManagerBase implements DomainManager, DomainService { @@ -219,6 +220,25 @@ public class DomainManagerImpl extends ManagerBase implements DomainManager, Dom } @Override + public Domain findDomainByIdOrPath(final Long id, final String domainPath) { + Long domainId = id; + if (domainId == null || domainId < 1L) { + if (Strings.isNullOrEmpty(domainPath) || domainPath.trim().isEmpty()) { + domainId = Domain.ROOT_DOMAIN; + } else { + final Domain domainVO = findDomainByPath(domainPath.trim()); + if (domainVO != null) { + return domainVO; + } + } + } + if (domainId != null && domainId > 0L) { + return _domainDao.findById(domainId); + } + return null; + } + + @Override public Set<Long> getDomainParentIds(long domainId) { return _domainDao.getDomainParentIds(domainId); } http://git-wip-us.apache.org/repos/asf/cloudstack/blob/566e7d9f/server/test/com/cloud/user/DomainManagerImplTest.java ---------------------------------------------------------------------- diff --git a/server/test/com/cloud/user/DomainManagerImplTest.java b/server/test/com/cloud/user/DomainManagerImplTest.java new file mode 100644 index 0000000..82d5491 --- /dev/null +++ b/server/test/com/cloud/user/DomainManagerImplTest.java @@ -0,0 +1,137 @@ +// 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 com.cloud.user; + +import com.cloud.configuration.dao.ResourceCountDao; +import com.cloud.configuration.dao.ResourceLimitDao; +import com.cloud.dc.dao.DedicatedResourceDao; +import com.cloud.domain.DomainVO; +import com.cloud.domain.dao.DomainDao; +import com.cloud.network.dao.NetworkDomainDao; +import com.cloud.projects.ProjectManager; +import com.cloud.projects.dao.ProjectDao; +import com.cloud.service.dao.ServiceOfferingDao; +import com.cloud.storage.dao.DiskOfferingDao; +import com.cloud.user.dao.AccountDao; +import org.apache.cloudstack.engine.orchestration.service.NetworkOrchestrationService; +import org.apache.cloudstack.framework.messagebus.MessageBus; +import org.apache.cloudstack.region.RegionManager; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.runners.MockitoJUnitRunner; + +import javax.inject.Inject; +import java.lang.reflect.Field; + +@RunWith(MockitoJUnitRunner.class) +public class DomainManagerImplTest { + @Mock + DomainDao _domainDao; + @Mock + AccountManager _accountMgr; + @Mock + ResourceCountDao _resourceCountDao; + @Mock + AccountDao _accountDao; + @Mock + DiskOfferingDao _diskOfferingDao; + @Mock + ServiceOfferingDao _offeringsDao; + @Mock + ProjectDao _projectDao; + @Mock + ProjectManager _projectMgr; + @Mock + RegionManager _regionMgr; + @Mock + ResourceLimitDao _resourceLimitDao; + @Mock + DedicatedResourceDao _dedicatedDao; + @Mock + NetworkOrchestrationService _networkMgr; + @Mock + NetworkDomainDao _networkDomainDao; + @Mock + MessageBus _messageBus; + + DomainManagerImpl domainManager; + + @Before + public void setup() throws NoSuchFieldException, SecurityException, + IllegalArgumentException, IllegalAccessException { + domainManager = new DomainManagerImpl(); + for (Field field : DomainManagerImpl.class.getDeclaredFields()) { + if (field.getAnnotation(Inject.class) != null) { + field.setAccessible(true); + try { + Field mockField = this.getClass().getDeclaredField( + field.getName()); + field.set(domainManager, mockField.get(this)); + } catch (Exception ignored) { + } + } + } + } + + @Test + public void testFindDomainByIdOrPathNullOrEmpty() { + final DomainVO domain = new DomainVO("someDomain", 123, 1L, "network.domain"); + Mockito.when(_domainDao.findById(1L)).thenReturn(domain); + Assert.assertEquals(domain, domainManager.findDomainByIdOrPath(null, null)); + Assert.assertEquals(domain, domainManager.findDomainByIdOrPath(0L, "")); + Assert.assertEquals(domain, domainManager.findDomainByIdOrPath(-1L, " ")); + Assert.assertEquals(domain, domainManager.findDomainByIdOrPath(null, " ")); + } + + @Test + public void testFindDomainByIdOrPathValidPathAndInvalidId() { + final DomainVO domain = new DomainVO("someDomain", 123, 1L, "network.domain"); + Mockito.when(_domainDao.findDomainByPath(Mockito.eq("/someDomain/"))).thenReturn(domain); + Assert.assertEquals(domain, domainManager.findDomainByIdOrPath(null, "/someDomain/")); + Assert.assertEquals(domain, domainManager.findDomainByIdOrPath(0L, " /someDomain/")); + Assert.assertEquals(domain, domainManager.findDomainByIdOrPath(-1L, "/someDomain/ ")); + Assert.assertEquals(domain, domainManager.findDomainByIdOrPath(null, " /someDomain/ ")); + } + + @Test + public void testFindDomainByIdOrPathInvalidPathAndInvalidId() { + Mockito.when(_domainDao.findDomainByPath(Mockito.anyString())).thenReturn(null); + Assert.assertNull(domainManager.findDomainByIdOrPath(null, "/nonExistingDomain/")); + Assert.assertNull(domainManager.findDomainByIdOrPath(0L, " /nonExistingDomain/")); + Assert.assertNull(domainManager.findDomainByIdOrPath(-1L, "/nonExistingDomain/ ")); + Assert.assertNull(domainManager.findDomainByIdOrPath(null, " /nonExistingDomain/ ")); + } + + + @Test + public void testFindDomainByIdOrPathValidId() { + final DomainVO domain = new DomainVO("someDomain", 123, 1L, "network.domain"); + Mockito.when(_domainDao.findById(1L)).thenReturn(domain); + Mockito.when(_domainDao.findDomainByPath(Mockito.eq("/validDomain/"))).thenReturn(new DomainVO()); + Assert.assertEquals(domain, domainManager.findDomainByIdOrPath(1L, null)); + Assert.assertEquals(domain, domainManager.findDomainByIdOrPath(1L, "")); + Assert.assertEquals(domain, domainManager.findDomainByIdOrPath(1L, " ")); + Assert.assertEquals(domain, domainManager.findDomainByIdOrPath(1L, " ")); + Assert.assertEquals(domain, domainManager.findDomainByIdOrPath(1L, "/validDomain/")); + } + +} http://git-wip-us.apache.org/repos/asf/cloudstack/blob/566e7d9f/server/test/com/cloud/user/MockDomainManagerImpl.java ---------------------------------------------------------------------- diff --git a/server/test/com/cloud/user/MockDomainManagerImpl.java b/server/test/com/cloud/user/MockDomainManagerImpl.java index cdae887..394c3e2 100644 --- a/server/test/com/cloud/user/MockDomainManagerImpl.java +++ b/server/test/com/cloud/user/MockDomainManagerImpl.java @@ -92,6 +92,11 @@ public class MockDomainManagerImpl extends ManagerBase implements DomainManager, } @Override + public DomainVO findDomainByIdOrPath(Long id, String domainPath) { + return null; + } + + @Override public Set<Long> getDomainParentIds(long domainId) { // TODO Auto-generated method stub return null; http://git-wip-us.apache.org/repos/asf/cloudstack/blob/566e7d9f/test/integration/smoke/test_login.py ---------------------------------------------------------------------- diff --git a/test/integration/smoke/test_login.py b/test/integration/smoke/test_login.py new file mode 100644 index 0000000..5855fea --- /dev/null +++ b/test/integration/smoke/test_login.py @@ -0,0 +1,145 @@ +# 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. + +from nose.plugins.attrib import attr +from marvin.cloudstackTestCase import * +from marvin.cloudstackAPI import * +from marvin.lib.utils import * +from marvin.lib.base import * +from marvin.lib.common import * + +import requests + + +class TestLogin(cloudstackTestCase): + """ + Tests default login API handler + """ + + def setUp(self): + self.apiclient = self.testClient.getApiClient() + self.dbclient = self.testClient.getDbConnection() + self.server_details = self.config.__dict__["mgtSvr"][0].__dict__ + self.server_url = "http://%s:8080/client/api"; % self.server_details['mgtSvrIp'] + self.testdata = { + "account": { + "email": "login-user@test.cloud", + "firstname": "TestLoginFirstName", + "lastname": "TestLoginLastName", + "username": "testloginuser-", + "password": "password123", + } + } + self.cleanup = [] + + + def tearDown(self): + try: + cleanup_resources(self.apiclient, self.cleanup) + except Exception as e: + raise Exception("Warning: Exception during cleanup : %s" % e) + + + def login(self, username, password, domain="/"): + """ + Logs in and returns a session to be used for subsequent API calls + """ + args = {} + args["command"] = 'login' + args["username"] = username + args["password"] = password + args["domain"] = domain + args["response"] = "json" + + session = requests.Session() + + try: + resp = session.post(self.server_url, params=args, verify=False) + except requests.exceptions.ConnectionError, e: + self.fail("Failed to attempt login request to mgmt server") + return None, None + + return resp, session + + + @attr(tags = ["devcloud", "advanced", "advancedns", "advancedsg", "smoke", + "basic", "sg"], required_hardware="false") + def login_test_saml_user(self): + """ + Tests that SAML users are not allowed CloudStack local log in + + Creates account across various account types and converts them to + a SAML user and tests that they are not able to log in; then + converts them back as a CloudStack user account and verifies that + they are allowed to log in and make API requests + """ + # Tests across various account types: 0=User, 1=Root Admin, 2=Domain Admin + for account_type in range(0, 3): + account = Account.create( + self.apiclient, + self.testdata['account'], + admin=account_type + ) + self.cleanup.append(account) + + username = account.user[0].username + password = self.testdata['account']['password'] + + # Convert newly created account user to SAML user + user_id = self.dbclient.execute("select id from user where uuid='%s'" % account.user[0].id)[0][0] + self.dbclient.execute("update user set source='SAML2' where id=%d" % user_id) + + response, session = self.login(username, password) + self.assertEqual( + response.json()['loginresponse']['errorcode'], + 531, + "SAML user should not be allowed to log in, error code 531 not returned" + ) + self.assertEqual( + response.json()['loginresponse']['errortext'], + "User is not allowed CloudStack login", + "Invalid error message returned, SAML user should not be allowed to log in" + ) + + # Convert newly created account user back to normal source + self.dbclient.execute("update user set source='UNKNOWN' where id=%d" % user_id) + + response, session = self.login(username, password) + self.assertEqual( + response.status_code, + 200, + "Login response code was not 200" + ) + self.assertTrue( + len(response.json()['loginresponse']['sessionkey']) > 0, + "Invalid session key received" + ) + + args = {} + args["command"] = 'listUsers' + args["listall"] = 'true' + args["response"] = "json" + response = session.get(self.server_url, params=args) + self.assertEqual( + response.status_code, + 200, + "listUsers response code was not 200" + ) + self.assertTrue( + len(response.json()['listusersresponse']['user']) > 0, + "listUsers list is empty or zero" + )
