Skip to content

Commit d541e90

Browse files
nvazqueznvazquez
committed
Merge branch '4.18' into 4.19
2 parents 9033ab7 + 2e0024e commit d541e90

File tree

6 files changed

+515
-21
lines changed

6 files changed

+515
-21
lines changed

‎.github/workflows/ci.yml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ jobs:
3535
fail-fast: false
3636
matrix:
3737
tests: [ "smoke/test_accounts
38+
smoke/test_account_access
3839
smoke/test_affinity_groups
3940
smoke/test_affinity_groups_projects
4041
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
@@ -2744,7 +2744,9 @@ public Map<String, String> getKeys(Long userId) {
27442744
throw new InvalidParameterValueException("Unable to find user by id");
27452745
}
27462746
final ControlledEntity account = getAccount(getUserAccountById(userId).getAccountId()); //Extracting the Account from the userID of the requested user.
2747-
checkAccess(CallContext.current().getCallingUser(), account);
2747+
User caller = CallContext.current().getCallingUser();
2748+
preventRootDomainAdminAccessToRootAdminKeys(caller, account);
2749+
checkAccess(caller, account);
27482750

27492751
Map<String, String> keys = new HashMap<String, String>();
27502752
keys.put("apikey", user.getApiKey());
@@ -2753,6 +2755,19 @@ public Map<String, String> getKeys(Long userId) {
27532755
return keys;
27542756
}
27552757

2758+
protected void preventRootDomainAdminAccessToRootAdminKeys(User caller, ControlledEntity account) {
2759+
if (isDomainAdminForRootDomain(caller) && isRootAdmin(account.getAccountId())) {
2760+
String msg = String.format("Caller Username %s does not have access to root admin keys", caller.getUsername());
2761+
s_logger.error(msg);
2762+
throw new PermissionDeniedException(msg);
2763+
}
2764+
}
2765+
2766+
protected boolean isDomainAdminForRootDomain(User callingUser) {
2767+
AccountVO caller = _accountDao.findById(callingUser.getAccountId());
2768+
return caller.getType() == Account.Type.DOMAIN_ADMIN && caller.getDomainId() == Domain.ROOT_DOMAIN;
2769+
}
2770+
27562771
@Override
27572772
public List<UserTwoFactorAuthenticator> listUserTwoFactorAuthenticationProviders() {
27582773
return userTwoFactorAuthenticationProviders;
@@ -2787,6 +2802,7 @@ public String[] createApiKeyAndSecretKey(RegisterCmd cmd) {
27872802
}
27882803

27892804
Account account = _accountDao.findById(user.getAccountId());
2805+
preventRootDomainAdminAccessToRootAdminKeys(user, account);
27902806
checkAccess(caller, null, true, account);
27912807

27922808
// 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: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,34 @@
1616
// under the License.
1717
package com.cloud.user;
1818

19+
import java.net.InetAddress;
20+
import java.net.UnknownHostException;
21+
import java.util.ArrayList;
22+
import java.util.Arrays;
23+
import java.util.List;
24+
import java.util.Map;
25+
import java.util.HashMap;
26+
27+
import org.apache.cloudstack.acl.ControlledEntity;
28+
import org.apache.cloudstack.acl.SecurityChecker.AccessType;
29+
import org.apache.cloudstack.api.command.admin.user.GetUserKeysCmd;
30+
import org.apache.cloudstack.api.command.admin.user.UpdateUserCmd;
31+
import org.apache.cloudstack.api.response.UserTwoFactorAuthenticationSetupResponse;
32+
import org.apache.cloudstack.auth.UserAuthenticator;
33+
import org.apache.cloudstack.auth.UserAuthenticator.ActionOnFailedAuthentication;
34+
import org.apache.cloudstack.auth.UserTwoFactorAuthenticator;
35+
import org.apache.cloudstack.context.CallContext;
36+
import org.apache.cloudstack.framework.config.ConfigKey;
37+
import org.junit.Assert;
38+
import org.junit.Before;
39+
import org.junit.Test;
40+
import org.junit.runner.RunWith;
41+
import org.mockito.InOrder;
42+
import org.mockito.Mock;
43+
import org.mockito.Mockito;
44+
import org.mockito.junit.MockitoJUnitRunner;
45+
import static org.mockito.ArgumentMatchers.nullable;
46+
1947
import com.cloud.acl.DomainChecker;
2048
import com.cloud.api.auth.SetupUserTwoFactorAuthenticationCmd;
2149
import com.cloud.domain.Domain;
@@ -241,6 +269,63 @@ public void testgetUserCmd() {
241269
accountManagerImpl.getKeys(_listkeyscmd);
242270
}
243271

272+
@Test(expected = PermissionDeniedException.class)
273+
public void testGetUserKeysCmdDomainAdminRootAdminUser() {
274+
CallContext.register(callingUser, callingAccount);
275+
Mockito.when(_listkeyscmd.getID()).thenReturn(2L);
276+
Mockito.when(accountManagerImpl.getActiveUser(2L)).thenReturn(userVoMock);
277+
Mockito.when(userAccountDaoMock.findById(2L)).thenReturn(userAccountVO);
278+
Mockito.when(userAccountVO.getAccountId()).thenReturn(2L);
279+
Mockito.when(userDetailsDaoMock.listDetailsKeyPairs(Mockito.anyLong())).thenReturn(null);
280+
281+
// Queried account - admin account
282+
AccountVO adminAccountMock = Mockito.mock(AccountVO.class);
283+
Mockito.when(adminAccountMock.getAccountId()).thenReturn(2L);
284+
Mockito.when(_accountDao.findByIdIncludingRemoved(2L)).thenReturn(adminAccountMock);
285+
Mockito.lenient().when(accountService.isRootAdmin(2L)).thenReturn(true);
286+
Mockito.lenient().when(securityChecker.checkAccess(Mockito.any(Account.class),
287+
Mockito.nullable(ControlledEntity.class), Mockito.nullable(AccessType.class), Mockito.anyString())).thenReturn(true);
288+
289+
// Calling account is domain admin of the ROOT domain
290+
Mockito.lenient().when(callingAccount.getType()).thenReturn(Account.Type.DOMAIN_ADMIN);
291+
Mockito.lenient().when(callingAccount.getDomainId()).thenReturn(Domain.ROOT_DOMAIN);
292+
293+
Mockito.lenient().when(callingUser.getAccountId()).thenReturn(2L);
294+
Mockito.lenient().when(_accountDao.findById(2L)).thenReturn(callingAccount);
295+
296+
Mockito.lenient().when(accountService.isDomainAdmin(Mockito.anyLong())).thenReturn(Boolean.TRUE);
297+
Mockito.lenient().when(accountMock.getAccountId()).thenReturn(2L);
298+
299+
accountManagerImpl.getKeys(_listkeyscmd);
300+
}
301+
302+
@Test
303+
public void testPreventRootDomainAdminAccessToRootAdminKeysNormalUser() {
304+
User user = Mockito.mock(User.class);
305+
ControlledEntity entity = Mockito.mock(ControlledEntity.class);
306+
Mockito.when(user.getAccountId()).thenReturn(1L);
307+
AccountVO account = Mockito.mock(AccountVO.class);
308+
Mockito.when(account.getType()).thenReturn(Account.Type.NORMAL);
309+
Mockito.when(_accountDao.findById(1L)).thenReturn(account);
310+
accountManagerImpl.preventRootDomainAdminAccessToRootAdminKeys(user, entity);
311+
Mockito.verify(accountManagerImpl, Mockito.never()).isRootAdmin(Mockito.anyLong());
312+
}
313+
314+
@Test(expected = PermissionDeniedException.class)
315+
public void testPreventRootDomainAdminAccessToRootAdminKeysRootDomainAdminUser() {
316+
User user = Mockito.mock(User.class);
317+
ControlledEntity entity = Mockito.mock(ControlledEntity.class);
318+
Mockito.when(user.getAccountId()).thenReturn(1L);
319+
AccountVO account = Mockito.mock(AccountVO.class);
320+
Mockito.when(account.getType()).thenReturn(Account.Type.DOMAIN_ADMIN);
321+
Mockito.when(account.getDomainId()).thenReturn(Domain.ROOT_DOMAIN);
322+
Mockito.when(_accountDao.findById(1L)).thenReturn(account);
323+
Mockito.when(entity.getAccountId()).thenReturn(1L);
324+
Mockito.lenient().when(securityChecker.checkAccess(Mockito.any(Account.class),
325+
Mockito.nullable(ControlledEntity.class), Mockito.nullable(AccessType.class), Mockito.anyString())).thenReturn(true);
326+
accountManagerImpl.preventRootDomainAdminAccessToRootAdminKeys(user, entity);
327+
}
328+
244329
@Test
245330
public void updateUserTestTimeZoneAndEmailNull() {
246331
prepareMockAndExecuteUpdateUserTest(0);

0 commit comments

Comments
 (0)