Skip to content

Commit 2e0024e

Browse files
shwstpprwinterhazelnvazquez
committed
server, api: account and api entity access improvements
Fixes domain-admin access check to prevent unauthorized access. Introduces a new non-dynamic global setting - api.allow.internal.db.ids to control whether to allow using internal DB IDs as API parameters or not. Default value for the global setting is false. Co-authored-by: Fabricio Duarte <[email protected]> Co-authored-by: nvazquez <[email protected]> Co-authored-by: Abhishek Kumar <[email protected]>
1 parent cf0e44d commit 2e0024e

File tree

6 files changed

+488
-21
lines changed

6 files changed

+488
-21
lines changed

‎.github/workflows/ci.yml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ jobs:
3232
fail-fast: false
3333
matrix:
3434
tests: [ "smoke/test_accounts
35+
smoke/test_account_access
3536
smoke/test_affinity_groups
3637
smoke/test_affinity_groups_projects
3738
smoke/test_annotations

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

Lines changed: 48 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -208,38 +208,66 @@ public boolean checkAccess(Account caller, ControlledEntity entity, AccessType a
208208

209209
return true;
210210
} else if (entity instanceof Network && accessType != null && accessType == AccessType.UseEntry) {
211-
_networkMgr.checkNetworkPermissions(caller, (Network)entity);
211+
_networkMgr.checkNetworkPermissions(caller, (Network) entity);
212212
} else if (entity instanceof Network && accessType != null && accessType == AccessType.OperateEntry) {
213213
_networkMgr.checkNetworkOperatePermissions(caller, (Network)entity);
214214
} else if (entity instanceof VirtualRouter) {
215215
_networkMgr.checkRouterPermissions(caller, (VirtualRouter)entity);
216216
} else if (entity instanceof AffinityGroup) {
217217
return false;
218218
} else {
219-
if (_accountService.isNormalUser(caller.getId())) {
220-
Account account = _accountDao.findById(entity.getAccountId());
221-
String errorMessage = String.format("%s does not have permission to operate with resource", caller);
222-
if (account != null && account.getType() == Account.Type.PROJECT) {
223-
//only project owner can delete/modify the project
224-
if (accessType != null && accessType == AccessType.ModifyProject) {
225-
if (!_projectMgr.canModifyProjectAccount(caller, account.getId())) {
226-
throw new PermissionDeniedException(errorMessage);
227-
}
228-
} else if (!_projectMgr.canAccessProjectAccount(caller, account.getId())) {
229-
throw new PermissionDeniedException(errorMessage);
230-
}
231-
checkOperationPermitted(caller, entity);
232-
} else {
233-
if (caller.getId() != entity.getAccountId()) {
234-
throw new PermissionDeniedException(errorMessage);
235-
}
219+
validateCallerHasAccessToEntityOwner(caller, entity, accessType);
220+
}
221+
return true;
222+
}
223+
224+
protected void validateCallerHasAccessToEntityOwner(Account caller, ControlledEntity entity, AccessType accessType) {
225+
PermissionDeniedException exception = new PermissionDeniedException("Caller does not have permission to operate with provided resource.");
226+
String entityLog = String.format("entity [owner ID: %d, type: %s]", entity.getAccountId(),
227+
entity.getEntityType().getSimpleName());
228+
229+
if (_accountService.isRootAdmin(caller.getId())) {
230+
return;
231+
}
232+
233+
if (caller.getId() == entity.getAccountId()) {
234+
return;
235+
}
236+
237+
Account owner = _accountDao.findById(entity.getAccountId());
238+
if (owner == null) {
239+
s_logger.error(String.format("Owner not found for %s", entityLog));
240+
throw exception;
241+
}
242+
243+
Account.Type callerAccountType = caller.getType();
244+
if ((callerAccountType == Account.Type.DOMAIN_ADMIN || callerAccountType == Account.Type.RESOURCE_DOMAIN_ADMIN) &&
245+
_domainDao.isChildDomain(caller.getDomainId(), owner.getDomainId())) {
246+
return;
247+
}
248+
249+
if (owner.getType() == Account.Type.PROJECT) {
250+
// only project owner can delete/modify the project
251+
if (accessType == AccessType.ModifyProject) {
252+
if (!_projectMgr.canModifyProjectAccount(caller, owner.getId())) {
253+
s_logger.error(String.format("Caller ID: %d does not have permission to modify project with " +
254+
"owner ID: %d", caller.getId(), owner.getId()));
255+
throw exception;
236256
}
257+
} else if (!_projectMgr.canAccessProjectAccount(caller, owner.getId())) {
258+
s_logger.error(String.format("Caller ID: %d does not have permission to access project with " +
259+
"owner ID: %d", caller.getId(), owner.getId()));
260+
throw exception;
237261
}
262+
checkOperationPermitted(caller, entity);
263+
return;
238264
}
239-
return true;
265+
266+
s_logger.error(String.format("Caller ID: %d does not have permission to access %s", caller.getId(), entityLog));
267+
throw exception;
240268
}
241269

242-
private boolean checkOperationPermitted(Account caller, ControlledEntity entity) {
270+
protected boolean checkOperationPermitted(Account caller, ControlledEntity entity) {
243271
User user = CallContext.current().getCallingUser();
244272
Project project = projectDao.findByProjectAccountId(entity.getAccountId());
245273
if (project == null) {

‎server/src/main/java/com/cloud/user/AccountManagerImpl.java‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2717,7 +2717,9 @@ public Map<String, String> getKeys(Long userId) {
27172717
throw new InvalidParameterValueException("Unable to find user by id");
27182718
}
27192719
final ControlledEntity account = getAccount(getUserAccountById(userId).getAccountId()); //Extracting the Account from the userID of the requested user.
2720-
checkAccess(CallContext.current().getCallingUser(), account);
2720+
User caller = CallContext.current().getCallingUser();
2721+
preventRootDomainAdminAccessToRootAdminKeys(caller, account);
2722+
checkAccess(caller, account);
27212723

27222724
Map<String, String> keys = new HashMap<String, String>();
27232725
keys.put("apikey", user.getApiKey());
@@ -2726,6 +2728,19 @@ public Map<String, String> getKeys(Long userId) {
27262728
return keys;
27272729
}
27282730

2731+
protected void preventRootDomainAdminAccessToRootAdminKeys(User caller, ControlledEntity account) {
2732+
if (isDomainAdminForRootDomain(caller) && isRootAdmin(account.getAccountId())) {
2733+
String msg = String.format("Caller Username %s does not have access to root admin keys", caller.getUsername());
2734+
s_logger.error(msg);
2735+
throw new PermissionDeniedException(msg);
2736+
}
2737+
}
2738+
2739+
protected boolean isDomainAdminForRootDomain(User callingUser) {
2740+
AccountVO caller = _accountDao.findById(callingUser.getAccountId());
2741+
return caller.getType() == Account.Type.DOMAIN_ADMIN && caller.getDomainId() == Domain.ROOT_DOMAIN;
2742+
}
2743+
27292744
@Override
27302745
public List<UserTwoFactorAuthenticator> listUserTwoFactorAuthenticationProviders() {
27312746
return userTwoFactorAuthenticationProviders;
@@ -2760,6 +2775,7 @@ public String[] createApiKeyAndSecretKey(RegisterCmd cmd) {
27602775
}
27612776

27622777
Account account = _accountDao.findById(user.getAccountId());
2778+
preventRootDomainAdminAccessToRootAdminKeys(user, account);
27632779
checkAccess(caller, null, true, account);
27642780

27652781
// don't allow updating system user

‎server/src/test/java/com/cloud/acl/DomainCheckerTest.java‎

Lines changed: 166 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,166 @@
1+
// Licensed to the Apache Software Foundation (ASF) under one
2+
// or more contributor license agreements. See the NOTICE file
3+
// distributed with this work for additional information
4+
// regarding copyright ownership. The ASF licenses this file
5+
// to you under the Apache License, Version 2.0 (the
6+
// "License"); you may not use this file except in compliance
7+
// with the License. You may obtain a copy of the License at
8+
//
9+
// http://www.apache.org/licenses/LICENSE-2.0
10+
//
11+
// Unless required by applicable law or agreed to in writing,
12+
// software distributed under the License is distributed on an
13+
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
14+
// KIND, either express or implied. See the License for the
15+
// specific language governing permissions and limitations
16+
// under the License.
17+
package com.cloud.acl;
18+
19+
import org.apache.cloudstack.acl.ControlledEntity;
20+
import org.apache.cloudstack.acl.SecurityChecker;
21+
import org.junit.Test;
22+
import org.junit.runner.RunWith;
23+
import org.mockito.InjectMocks;
24+
import org.mockito.Mock;
25+
import org.mockito.Mockito;
26+
import org.mockito.Spy;
27+
import org.mockito.junit.MockitoJUnitRunner;
28+
29+
import com.cloud.domain.dao.DomainDao;
30+
import com.cloud.exception.PermissionDeniedException;
31+
import com.cloud.projects.ProjectManager;
32+
import com.cloud.user.Account;
33+
import com.cloud.user.AccountService;
34+
import com.cloud.user.AccountVO;
35+
import com.cloud.user.dao.AccountDao;
36+
import com.cloud.utils.Ternary;
37+
38+
@RunWith(MockitoJUnitRunner.class)
39+
public class DomainCheckerTest {
40+
41+
@Mock
42+
AccountService _accountService;
43+
@Mock
44+
AccountDao _accountDao;
45+
@Mock
46+
DomainDao _domainDao;
47+
@Mock
48+
ProjectManager _projectMgr;
49+
50+
@Spy
51+
@InjectMocks
52+
DomainChecker domainChecker;
53+
54+
private ControlledEntity getMockedEntity(long accountId) {
55+
ControlledEntity entity = Mockito.mock(Account.class);
56+
Mockito.when(entity.getAccountId()).thenReturn(accountId);
57+
Mockito.when(entity.getEntityType()).thenReturn((Class)Account.class);
58+
return entity;
59+
}
60+
61+
@Test
62+
public void testRootAdminHasAccess() {
63+
Account rootAdmin = Mockito.mock(Account.class);
64+
Mockito.when(rootAdmin.getId()).thenReturn(1L);
65+
ControlledEntity entity = getMockedEntity(2L);
66+
Mockito.when(_accountService.isRootAdmin(rootAdmin.getId())).thenReturn(true);
67+
68+
domainChecker.validateCallerHasAccessToEntityOwner(rootAdmin, entity, SecurityChecker.AccessType.ModifyProject);
69+
}
70+
71+
@Test
72+
public void testCallerIsOwner() {
73+
Account caller = Mockito.mock(Account.class);
74+
Mockito.when(caller.getId()).thenReturn(1L);
75+
ControlledEntity entity = getMockedEntity(1L);
76+
77+
domainChecker.validateCallerHasAccessToEntityOwner(caller, entity, SecurityChecker.AccessType.ModifyProject);
78+
}
79+
80+
@Test(expected = PermissionDeniedException.class)
81+
public void testOwnerNotFound() {
82+
Account caller = Mockito.mock(Account.class);
83+
Mockito.when(caller.getId()).thenReturn(1L);
84+
ControlledEntity entity = getMockedEntity(2L);
85+
Mockito.when(_accountDao.findById(entity.getAccountId())).thenReturn(null);
86+
87+
domainChecker.validateCallerHasAccessToEntityOwner(caller, entity, SecurityChecker.AccessType.ModifyProject);
88+
}
89+
90+
@Test
91+
public void testDomainAdminHasAccess() {
92+
Account caller = Mockito.mock(Account.class);
93+
Mockito.when(caller.getId()).thenReturn(1L);
94+
Mockito.when(caller.getDomainId()).thenReturn(100L);
95+
Mockito.when(caller.getType()).thenReturn(Account.Type.DOMAIN_ADMIN);
96+
ControlledEntity entity = getMockedEntity(2L);
97+
AccountVO owner = Mockito.mock(AccountVO.class);
98+
Mockito.when(owner.getDomainId()).thenReturn(101L);
99+
Mockito.when(_accountDao.findById(entity.getAccountId())).thenReturn(owner);
100+
Mockito.when(_domainDao.isChildDomain(100L, 101L)).thenReturn(true);
101+
102+
domainChecker.validateCallerHasAccessToEntityOwner(caller, entity, SecurityChecker.AccessType.ModifyProject);
103+
}
104+
105+
private Ternary<Account, ControlledEntity, AccountVO> getProjectAccessCheckResources() {
106+
Account caller = Mockito.mock(Account.class);
107+
Mockito.when(caller.getId()).thenReturn(100L);
108+
Mockito.when(caller.getType()).thenReturn(Account.Type.PROJECT);
109+
ControlledEntity entity = getMockedEntity(2L);
110+
AccountVO projectAccount = Mockito.mock(AccountVO.class);
111+
Mockito.when(projectAccount.getId()).thenReturn(2L);
112+
Mockito.when(projectAccount.getType()).thenReturn(Account.Type.PROJECT);
113+
return new Ternary<>(caller, entity, projectAccount);
114+
}
115+
116+
@Test
117+
public void testProjectOwnerCanModify() {
118+
Ternary<Account, ControlledEntity, AccountVO> resources = getProjectAccessCheckResources();
119+
Account caller = resources.first();
120+
ControlledEntity entity = resources.second();
121+
AccountVO projectAccount = resources.third();
122+
Mockito.when(_accountDao.findById(entity.getAccountId())).thenReturn(projectAccount);
123+
Mockito.when(_projectMgr.canModifyProjectAccount(caller, projectAccount.getId())).thenReturn(true);
124+
Mockito.doReturn(true).when(domainChecker).checkOperationPermitted(caller, entity);
125+
126+
domainChecker.validateCallerHasAccessToEntityOwner(caller, entity, SecurityChecker.AccessType.ModifyProject);
127+
}
128+
129+
@Test(expected = PermissionDeniedException.class)
130+
public void testProjectOwnerCannotModify() {
131+
Ternary<Account, ControlledEntity, AccountVO> resources = getProjectAccessCheckResources();
132+
Account caller = resources.first();
133+
ControlledEntity entity = resources.second();
134+
AccountVO projectAccount = resources.third();
135+
Mockito.when(_accountDao.findById(entity.getAccountId())).thenReturn(projectAccount);
136+
Mockito.when(_projectMgr.canModifyProjectAccount(caller, projectAccount.getId())).thenReturn(false);
137+
138+
domainChecker.validateCallerHasAccessToEntityOwner(caller, entity, SecurityChecker.AccessType.ModifyProject);
139+
}
140+
141+
@Test
142+
public void testProjectOwnerCanAccess() {
143+
Ternary<Account, ControlledEntity, AccountVO> resources = getProjectAccessCheckResources();
144+
Account caller = resources.first();
145+
ControlledEntity entity = resources.second();
146+
AccountVO projectAccount = resources.third();
147+
Mockito.when(_accountDao.findById(entity.getAccountId())).thenReturn(projectAccount);
148+
Mockito.when(_projectMgr.canAccessProjectAccount(caller, projectAccount.getId())).thenReturn(true);
149+
Mockito.doReturn(true).when(domainChecker).checkOperationPermitted(caller, entity);
150+
151+
domainChecker.validateCallerHasAccessToEntityOwner(caller, entity, SecurityChecker.AccessType.ListEntry);
152+
}
153+
154+
@Test(expected = PermissionDeniedException.class)
155+
public void testProjectOwnerCannotAccess() {
156+
Ternary<Account, ControlledEntity, AccountVO> resources = getProjectAccessCheckResources();
157+
Account caller = resources.first();
158+
ControlledEntity entity = resources.second();
159+
AccountVO projectAccount = resources.third();
160+
Mockito.when(_accountDao.findById(entity.getAccountId())).thenReturn(projectAccount);
161+
Mockito.when(_projectMgr.canAccessProjectAccount(caller, projectAccount.getId())).thenReturn(false);
162+
163+
domainChecker.validateCallerHasAccessToEntityOwner(caller, entity, SecurityChecker.AccessType.ListEntry);
164+
}
165+
166+
}

‎server/src/test/java/com/cloud/user/AccountManagerImplTest.java‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
import java.util.Map;
2525
import java.util.HashMap;
2626

27+
import org.apache.cloudstack.acl.ControlledEntity;
2728
import org.apache.cloudstack.acl.SecurityChecker.AccessType;
2829
import org.apache.cloudstack.api.command.admin.user.GetUserKeysCmd;
2930
import org.apache.cloudstack.api.command.admin.user.UpdateUserCmd;
@@ -240,6 +241,63 @@ public void testgetUserCmd() {
240241
accountManagerImpl.getKeys(_listkeyscmd);
241242
}
242243

244+
@Test(expected = PermissionDeniedException.class)
245+
public void testGetUserKeysCmdDomainAdminRootAdminUser() {
246+
CallContext.register(callingUser, callingAccount);
247+
Mockito.when(_listkeyscmd.getID()).thenReturn(2L);
248+
Mockito.when(accountManagerImpl.getActiveUser(2L)).thenReturn(userVoMock);
249+
Mockito.when(userAccountDaoMock.findById(2L)).thenReturn(userAccountVO);
250+
Mockito.when(userAccountVO.getAccountId()).thenReturn(2L);
251+
Mockito.when(userDetailsDaoMock.listDetailsKeyPairs(Mockito.anyLong())).thenReturn(null);
252+
253+
// Queried account - admin account
254+
AccountVO adminAccountMock = Mockito.mock(AccountVO.class);
255+
Mockito.when(adminAccountMock.getAccountId()).thenReturn(2L);
256+
Mockito.when(_accountDao.findByIdIncludingRemoved(2L)).thenReturn(adminAccountMock);
257+
Mockito.lenient().when(accountService.isRootAdmin(2L)).thenReturn(true);
258+
Mockito.lenient().when(securityChecker.checkAccess(Mockito.any(Account.class),
259+
Mockito.nullable(ControlledEntity.class), Mockito.nullable(AccessType.class), Mockito.anyString())).thenReturn(true);
260+
261+
// Calling account is domain admin of the ROOT domain
262+
Mockito.lenient().when(callingAccount.getType()).thenReturn(Account.Type.DOMAIN_ADMIN);
263+
Mockito.lenient().when(callingAccount.getDomainId()).thenReturn(Domain.ROOT_DOMAIN);
264+
265+
Mockito.lenient().when(callingUser.getAccountId()).thenReturn(2L);
266+
Mockito.lenient().when(_accountDao.findById(2L)).thenReturn(callingAccount);
267+
268+
Mockito.lenient().when(accountService.isDomainAdmin(Mockito.anyLong())).thenReturn(Boolean.TRUE);
269+
Mockito.lenient().when(accountMock.getAccountId()).thenReturn(2L);
270+
271+
accountManagerImpl.getKeys(_listkeyscmd);
272+
}
273+
274+
@Test
275+
public void testPreventRootDomainAdminAccessToRootAdminKeysNormalUser() {
276+
User user = Mockito.mock(User.class);
277+
ControlledEntity entity = Mockito.mock(ControlledEntity.class);
278+
Mockito.when(user.getAccountId()).thenReturn(1L);
279+
AccountVO account = Mockito.mock(AccountVO.class);
280+
Mockito.when(account.getType()).thenReturn(Account.Type.NORMAL);
281+
Mockito.when(_accountDao.findById(1L)).thenReturn(account);
282+
accountManagerImpl.preventRootDomainAdminAccessToRootAdminKeys(user, entity);
283+
Mockito.verify(accountManagerImpl, Mockito.never()).isRootAdmin(Mockito.anyLong());
284+
}
285+
286+
@Test(expected = PermissionDeniedException.class)
287+
public void testPreventRootDomainAdminAccessToRootAdminKeysRootDomainAdminUser() {
288+
User user = Mockito.mock(User.class);
289+
ControlledEntity entity = Mockito.mock(ControlledEntity.class);
290+
Mockito.when(user.getAccountId()).thenReturn(1L);
291+
AccountVO account = Mockito.mock(AccountVO.class);
292+
Mockito.when(account.getType()).thenReturn(Account.Type.DOMAIN_ADMIN);
293+
Mockito.when(account.getDomainId()).thenReturn(Domain.ROOT_DOMAIN);
294+
Mockito.when(_accountDao.findById(1L)).thenReturn(account);
295+
Mockito.when(entity.getAccountId()).thenReturn(1L);
296+
Mockito.lenient().when(securityChecker.checkAccess(Mockito.any(Account.class),
297+
Mockito.nullable(ControlledEntity.class), Mockito.nullable(AccessType.class), Mockito.anyString())).thenReturn(true);
298+
accountManagerImpl.preventRootDomainAdminAccessToRootAdminKeys(user, entity);
299+
}
300+
243301
@Test
244302
public void updateUserTestTimeZoneAndEmailNull() {
245303
prepareMockAndExecuteUpdateUserTest(0);

0 commit comments

Comments
 (0)