Skip to content

Commit 6a43da5

Browse files
chrisknollchrisknoll
authored
Permission Checks now use Wildcard semantics. (#2355)
Permission checks are using Subject.isPermitted() which honors wildcard semantics. Changed read/write permission checks to use permissions instead of roles. Removed role cache from Permission Service. Altered JwtAuthRealm to filter user permissions to either * or first element of permission to check for speed. Changed permission index from JsonNode to Map<>. Serializes same way, but map semantics are simpler to navigate. Altered AuthrizationInfo to contain index of Permissions and store Wildcard perms. General cleanup of unused imports and removed unused dependencies (ie: Autowired fields were removed if no longer needed). Fixes #2353. * Added test cases. Reordered test scoped dependencies in pom.xml. Refactored shared methods to AbstractDatabaseTest.
1 parent f347d2d commit 6a43da5

File tree

12 files changed

+439
-268
lines changed

12 files changed

+439
-268
lines changed

‎pom.xml

+40-41
Original file line numberDiff line numberDiff line change
@@ -761,18 +761,6 @@
761761
<artifactId>xml-security-impl</artifactId>
762762
<version>1.0</version>
763763
</dependency>
764-
<dependency>
765-
<groupId>org.springframework.boot</groupId>
766-
<artifactId>spring-boot-starter-test</artifactId>
767-
<version>${spring.boot.version}</version>
768-
<scope>test</scope>
769-
<exclusions>
770-
<exclusion>
771-
<groupId>com.vaadin.external.google</groupId>
772-
<artifactId>android-json</artifactId>
773-
</exclusion>
774-
</exclusions>
775-
</dependency>
776764
<!-- It is overriding a transitive dependency from the dependency above -->
777765
<!-- After migrating to Spring Boot 2.x we should get rid of it -->
778766
<!-- It is currently inefficient as com.nimbusds:nimbus-jose-jwt has hard bounds [1.3.1,2.3] -->
@@ -1029,24 +1017,6 @@
10291017
<artifactId>jasypt-hibernate4</artifactId>
10301018
<version>1.9.2</version>
10311019
</dependency>
1032-
<dependency>
1033-
<groupId>org.dbunit</groupId>
1034-
<artifactId>dbunit</artifactId>
1035-
<version>2.7.0</version>
1036-
<scope>test</scope>
1037-
</dependency>
1038-
<dependency>
1039-
<groupId>com.github.springtestdbunit</groupId>
1040-
<artifactId>spring-test-dbunit</artifactId>
1041-
<version>1.3.0</version>
1042-
<scope>test</scope>
1043-
</dependency>
1044-
<dependency>
1045-
<groupId>pl.pragmatists</groupId>
1046-
<artifactId>JUnitParams</artifactId>
1047-
<version>1.1.0</version>
1048-
<scope>test</scope>
1049-
</dependency>
10501020
<dependency>
10511021
<groupId>org.bouncycastle</groupId>
10521022
<artifactId>bcprov-jdk15on</artifactId>
@@ -1208,31 +1178,60 @@
12081178
<artifactId>commonmark-ext-gfm-tables</artifactId>
12091179
<version>0.15.2</version>
12101180
</dependency>
1181+
<dependency>
1182+
<groupId>org.dom4j</groupId>
1183+
<artifactId>dom4j</artifactId>
1184+
<version>${dom4j.version}</version>
1185+
</dependency>
1186+
<dependency>
1187+
<groupId>com.cloudbees</groupId>
1188+
<artifactId>syslog-java-client</artifactId>
1189+
<version>1.1.7</version>
1190+
</dependency>
12111191
<dependency>
12121192
<groupId>com.opentable.components</groupId>
12131193
<artifactId>otj-pg-embedded</artifactId>
12141194
<version>0.13.1</version>
12151195
<scope>test</scope>
12161196
</dependency>
12171197
<dependency>
1218-
<groupId>com.github.mjeanroy</groupId>
1219-
<artifactId>dbunit-plus</artifactId>
1220-
<version>2.0.1</version>
1221-
<scope>test</scope>
1198+
<groupId>org.springframework.boot</groupId>
1199+
<artifactId>spring-boot-starter-test</artifactId>
1200+
<version>${spring.boot.version}</version>
1201+
<scope>test</scope>
1202+
<exclusions>
1203+
<exclusion>
1204+
<groupId>com.vaadin.external.google</groupId>
1205+
<artifactId>android-json</artifactId>
1206+
</exclusion>
1207+
</exclusions>
12221208
</dependency>
12231209
<dependency>
1224-
<groupId>org.dom4j</groupId>
1225-
<artifactId>dom4j</artifactId>
1226-
<version>${dom4j.version}</version>
1210+
<groupId>org.dbunit</groupId>
1211+
<artifactId>dbunit</artifactId>
1212+
<version>2.7.0</version>
1213+
<scope>test</scope>
12271214
</dependency>
12281215
<dependency>
1229-
<groupId>com.cloudbees</groupId>
1230-
<artifactId>syslog-java-client</artifactId>
1231-
<version>1.1.7</version>
1216+
<groupId>com.github.springtestdbunit</groupId>
1217+
<artifactId>spring-test-dbunit</artifactId>
1218+
<version>1.3.0</version>
1219+
<scope>test</scope>
1220+
</dependency>
1221+
<dependency>
1222+
<groupId>pl.pragmatists</groupId>
1223+
<artifactId>JUnitParams</artifactId>
1224+
<version>1.1.0</version>
1225+
<scope>test</scope>
1226+
</dependency>
1227+
<dependency>
1228+
<groupId>com.github.mjeanroy</groupId>
1229+
<artifactId>dbunit-plus</artifactId>
1230+
<version>2.0.1</version>
1231+
<scope>test</scope>
12321232
</dependency>
12331233
</dependencies>
12341234
<profiles>
1235-
12361235
<profile>
12371236
<id>webapi-oracle</id>
12381237
<properties>

‎src/main/java/org/ohdsi/webapi/security/PermissionService.java

+28-121
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@
44
import com.cosium.spring.data.jpa.entity.graph.domain.EntityGraphUtils;
55
import org.apache.shiro.authz.UnauthorizedException;
66
import org.ohdsi.webapi.model.CommonEntity;
7-
import org.ohdsi.webapi.security.dto.RoleDTO;
87
import org.ohdsi.webapi.security.model.EntityPermissionSchema;
98
import org.ohdsi.webapi.security.model.EntityPermissionSchemaResolver;
109
import org.ohdsi.webapi.security.model.EntityType;
@@ -34,16 +33,15 @@
3433
import javax.annotation.PostConstruct;
3534
import java.io.Serializable;
3635
import java.util.Arrays;
37-
import java.util.Collection;
38-
import java.util.HashMap;
39-
import java.util.HashSet;
4036
import java.util.List;
4137
import java.util.Map;
4238
import java.util.Objects;
4339
import java.util.Set;
44-
import java.util.concurrent.ConcurrentHashMap;
45-
import java.util.function.Function;
4640
import java.util.stream.Collectors;
41+
import org.apache.shiro.SecurityUtils;
42+
import org.apache.shiro.authz.Permission;
43+
import org.apache.shiro.authz.permission.WildcardPermission;
44+
import org.apache.shiro.subject.Subject;
4745

4846
@Service
4947
public class PermissionService {
@@ -64,9 +62,6 @@ public class PermissionService {
6462
@Value("#{!'${security.provider}'.equals('DisabledSecurity')}")
6563
private boolean securityEnabled;
6664

67-
private ThreadLocal<ConcurrentHashMap<EntityType, ConcurrentHashMap<String, Set<RoleDTO>>>> permissionCache =
68-
ThreadLocal.withInitial(ConcurrentHashMap::new);
69-
7065
private final EntityGraph PERMISSION_ENTITY_GRAPH = EntityGraphUtils.fromAttributePaths("rolePermissions", "rolePermissions.role");
7166

7267
public PermissionService(
@@ -133,14 +128,14 @@ public void checkCommonEntityOwnership(EntityType entityType, Integer entityId)
133128

134129
public Map<String, String> getPermissionTemplates(EntityPermissionSchema permissionSchema, AccessType accessType) {
135130

136-
switch (accessType) {
137-
case WRITE:
138-
return permissionSchema.getWritePermissions();
139-
case READ:
140-
return permissionSchema.getReadPermissions();
141-
default:
142-
throw new UnsupportedOperationException();
143-
}
131+
switch (accessType) {
132+
case WRITE:
133+
return permissionSchema.getWritePermissions();
134+
case READ:
135+
return permissionSchema.getReadPermissions();
136+
default:
137+
throw new UnsupportedOperationException();
138+
}
144139
}
145140

146141
public List<RoleEntity> finaAllRolesHavingPermissions(List<String> permissions) {
@@ -178,97 +173,28 @@ private boolean isCurrentUserOwnerOf(CommonEntity entity) {
178173
return Objects.equals(owner.getLogin(), loggedInUsername);
179174
}
180175

176+
public List<Permission> getEntityPermissions(EntityType entityType, Number id, AccessType accessType) {
177+
Set<String> permissionTemplates = getTemplatesForType(entityType, accessType).keySet();
181178

182-
public void preparePermissionCache(EntityType entityType, Set<String> permissionTemplates) {
183-
if (permissionCache.get().get(entityType) == null) {
184-
final ConcurrentHashMap<String, Set<RoleDTO>> rolesForEntity = new ConcurrentHashMap<>();
185-
permissionCache.get().put(entityType, rolesForEntity);
186-
187-
List<String> permissionsSQLTemplates = permissionTemplates.stream()
188-
.map(pt -> getPermissionSqlTemplate(pt))
189-
.collect(Collectors.toList());
190-
191-
Map<Long, RoleDTO> roleDTOMap = new HashMap<>();
192-
permissionsSQLTemplates.forEach(p -> {
193-
Iterable<PermissionEntity> permissionEntities = permissionRepository.findByValueLike(p, PERMISSION_ENTITY_GRAPH);
194-
for (PermissionEntity permissionEntity : permissionEntities) {
195-
Set<RoleDTO> roles = rolesForEntity.get(permissionEntity.getValue());
196-
if (roles == null) {
197-
rolesForEntity.put(permissionEntity.getValue(), new HashSet<>());
198-
}
199-
Set<RoleDTO> cachedRoles = rolesForEntity.get(permissionEntity.getValue());
200-
permissionEntity.getRolePermissions().forEach(rp -> {
201-
RoleDTO roleDTO = roleDTOMap.get(rp.getRole().getId());
202-
if (roleDTO == null) {
203-
roleDTO = conversionService.convert(rp.getRole(), RoleDTO.class);
204-
roleDTOMap.put(roleDTO.getId(), roleDTO);
205-
}
206-
cachedRoles.add(roleDTO);
207-
});
208-
}
209-
});
210-
}
211-
}
212-
213-
public List<RoleDTO> getRolesHavingPermissions(EntityType entityType, Number id) {
214-
Set<String> permissionTemplates = getTemplatesForType(entityType, AccessType.WRITE).keySet();
215-
preparePermissionCache(entityType, permissionTemplates);
216-
217-
List<String> permissions = permissionTemplates.stream()
218-
.map(pt -> getPermission(pt, id))
179+
List<Permission> permissions = permissionTemplates.stream()
180+
.map(pt -> new WildcardPermission(getPermission(pt, id)))
219181
.collect(Collectors.toList());
220-
int fitCount = permissions.size();
221-
Map<RoleDTO, Long> roleMap = permissions.stream()
222-
.filter(p -> permissionCache.get().get(entityType).get(p) != null)
223-
.flatMap(p -> permissionCache.get().get(entityType).get(p).stream())
224-
.collect(Collectors.groupingBy(Function.identity(), Collectors.counting()));
225-
List<RoleDTO> roles = roleMap.entrySet().stream()
226-
.filter(es -> es.getValue() == fitCount)
227-
.map(es -> es.getKey())
228-
.collect(Collectors.toList());
229-
return roles;
230-
}
231-
232-
public List<RoleDTO> getRolesHavingReadPermissions(EntityType entityType, Number id) {
233-
Set<String> permissionTemplates = getTemplatesForType(entityType, AccessType.READ).keySet();
234-
preparePermissionCache(entityType, permissionTemplates);
235-
236-
List<String> permissions = permissionTemplates.stream()
237-
.map(pt -> getPermission(pt, id))
238-
.collect(Collectors.toList());
239-
int fitCount = permissions.size();
240-
Map<RoleDTO, Long> roleMap = permissions.stream()
241-
.filter(p -> permissionCache.get().get(entityType).get(p) != null)
242-
.flatMap(p -> permissionCache.get().get(entityType).get(p).stream())
243-
.collect(Collectors.groupingBy(Function.identity(), Collectors.counting()));
244-
List<RoleDTO> roles = roleMap.entrySet().stream()
245-
.filter(es -> es.getValue() == fitCount)
246-
.map(es -> es.getKey())
247-
.collect(Collectors.toList());
248-
return roles;
249-
}
250-
251-
public void clearPermissionCache() {
252-
this.permissionCache.set(new ConcurrentHashMap<>());
182+
return permissions;
253183
}
254184

255-
public boolean hasWriteAccess(CommonEntity entity) {
185+
public boolean hasAccess(CommonEntity entity, AccessType accessType) {
256186
boolean hasAccess = false;
257187
if (securityEnabled && entity.getCreatedBy() != null) {
258188
try {
189+
Subject subject = SecurityUtils.getSubject();
259190
String login = this.permissionManager.getSubjectName();
260191
UserSimpleAuthorizationInfo authorizationInfo = this.permissionManager.getAuthorizationInfo(login);
261192
if (Objects.equals(authorizationInfo.getUserId(), entity.getCreatedBy().getId())) {
262193
hasAccess = true; // the role is the one that created the artifact
263194
} else {
264195
EntityType entityType = entityPermissionSchemaResolver.getEntityType(entity.getClass());
265-
266-
List<RoleDTO> roles = getRolesHavingPermissions(entityType, entity.getId());
267-
268-
Collection<String> userRoles = authorizationInfo.getRoles();
269-
hasAccess = roles.stream()
270-
.anyMatch(r -> userRoles.stream()
271-
.anyMatch(re -> re.equals(r.getName())));
196+
List<Permission> permsToCheck = getEntityPermissions(entityType, entity.getId(), accessType);
197+
hasAccess = permsToCheck.stream().allMatch(p -> subject.isPermitted(p));
272198
}
273199
} catch (Exception e) {
274200
logger.error("Error getting user roles and permissions", e);
@@ -277,43 +203,24 @@ public boolean hasWriteAccess(CommonEntity entity) {
277203
}
278204
return hasAccess;
279205
}
280-
206+
207+
public boolean hasWriteAccess(CommonEntity entity) {
208+
return hasAccess(entity, AccessType.WRITE);
209+
}
281210

282211
public boolean hasReadAccess(CommonEntity entity) {
283-
boolean hasAccess = false;
284-
if (securityEnabled && entity.getCreatedBy() != null) {
285-
try {
286-
String login = this.permissionManager.getSubjectName();
287-
UserSimpleAuthorizationInfo authorizationInfo = this.permissionManager.getAuthorizationInfo(login);
288-
if (Objects.equals(authorizationInfo.getUserId(), entity.getCreatedBy().getId())){
289-
hasAccess = true; // the role is the one that created the artifact
290-
} else {
291-
EntityType entityType = entityPermissionSchemaResolver.getEntityType(entity.getClass());
292-
293-
List<RoleDTO> roles = getRolesHavingReadPermissions(entityType, entity.getId());
294-
295-
Collection<String> userRoles = authorizationInfo.getRoles();
296-
hasAccess = roles.stream()
297-
.anyMatch(r -> userRoles.stream()
298-
.anyMatch(re -> re.equals(r.getName())));
299-
}
300-
} catch (Exception e) {
301-
logger.error("Error getting user roles and permissions", e);
302-
throw new RuntimeException(e);
303-
}
304-
}
305-
return hasAccess;
212+
return hasAccess(entity, AccessType.READ);
306213
}
307214

308215
public void fillWriteAccess(CommonEntity entity, CommonEntityDTO entityDTO) {
309216
if (securityEnabled && entity.getCreatedBy() != null) {
310-
entityDTO.setHasWriteAccess(hasWriteAccess(entity));
217+
entityDTO.setHasWriteAccess(hasAccess(entity, AccessType.WRITE));
311218
}
312219
}
313220

314221
public void fillReadAccess(CommonEntity entity, CommonEntityDTO entityDTO) {
315222
if (securityEnabled && entity.getCreatedBy() != null) {
316-
entityDTO.setHasReadAccess(hasReadAccess(entity));
223+
entityDTO.setHasReadAccess(hasAccess(entity, AccessType.READ));
317224
}
318225
}
319226

‎src/main/java/org/ohdsi/webapi/security/model/UserSimpleAuthorizationInfo.java

+28-14
Original file line numberDiff line numberDiff line change
@@ -1,25 +1,39 @@
11
package org.ohdsi.webapi.security.model;
22

3+
import java.util.List;
4+
import java.util.Map;
5+
import org.apache.shiro.authz.Permission;
36
import org.apache.shiro.authz.SimpleAuthorizationInfo;
47

58
public class UserSimpleAuthorizationInfo extends SimpleAuthorizationInfo {
6-
private Long userId;
79

8-
private String login;
10+
private Long userId;
11+
private String login;
12+
private Map<String,List<Permission>> permissionIdx;
913

10-
public Long getUserId() {
11-
return userId;
12-
}
14+
15+
public Long getUserId() {
16+
return userId;
17+
}
1318

14-
public void setUserId(Long userId) {
15-
this.userId = userId;
16-
}
19+
public void setUserId(Long userId) {
20+
this.userId = userId;
21+
}
1722

18-
public String getLogin() {
19-
return login;
20-
}
23+
public String getLogin() {
24+
return login;
25+
}
26+
27+
public void setLogin(String login) {
28+
this.login = login;
29+
}
30+
31+
public Map<String, List<Permission>> getPermissionIdx() {
32+
return permissionIdx;
33+
}
34+
35+
public void setPermissionIdx(Map<String, List<Permission>> permissionIdx) {
36+
this.permissionIdx = permissionIdx;
37+
}
2138

22-
public void setLogin(String login) {
23-
this.login = login;
24-
}
2539
}

‎src/main/java/org/ohdsi/webapi/service/UserService.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ public static class User implements Comparable<User> {
5151
public String login;
5252
public String name;
5353
public List<Permission> permissions;
54-
public JsonNode permissionIdx;
54+
public Map<String, List<String>> permissionIdx;
5555

5656
public User() {}
5757

0 commit comments

Comments
 (0)