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.

Author: chrisknoll

pom.xml

@@ -761,18 +761,6 @@
       <artifactId>xml-security-impl</artifactId>
       <version>1.0</version>
     </dependency>
-    <dependency>
-      <groupId>org.springframework.boot</groupId>
-      <artifactId>spring-boot-starter-test</artifactId>
-      <version>${spring.boot.version}</version>
-      <scope>test</scope>
-        <exclusions>
-            <exclusion>
-                <groupId>com.vaadin.external.google</groupId>
-                <artifactId>android-json</artifactId>
-            </exclusion>
-        </exclusions>
-    </dependency>
     <!-- It is overriding a transitive dependency from the dependency above -->
     <!-- After migrating to Spring Boot 2.x we should get rid of it -->
     <!-- It is currently inefficient as com.nimbusds:nimbus-jose-jwt has hard bounds [1.3.1,2.3] -->
@@ -1029,24 +1017,6 @@
       <artifactId>jasypt-hibernate4</artifactId>
       <version>1.9.2</version>
     </dependency>
-    <dependency>
-      <groupId>org.dbunit</groupId>
-      <artifactId>dbunit</artifactId>
-      <version>2.7.0</version>
-      <scope>test</scope>
-    </dependency>
-    <dependency>
-      <groupId>com.github.springtestdbunit</groupId>
-      <artifactId>spring-test-dbunit</artifactId>
-      <version>1.3.0</version>
-      <scope>test</scope>
-    </dependency>
-    <dependency>
-      <groupId>pl.pragmatists</groupId>
-      <artifactId>JUnitParams</artifactId>
-      <version>1.1.0</version>
-      <scope>test</scope>
-    </dependency>
     <dependency>
       <groupId>org.bouncycastle</groupId>
       <artifactId>bcprov-jdk15on</artifactId>
@@ -1208,31 +1178,60 @@
       <artifactId>commonmark-ext-gfm-tables</artifactId>
       <version>0.15.2</version>
     </dependency>
+    <dependency>
+      <groupId>org.dom4j</groupId>
+      <artifactId>dom4j</artifactId>
+      <version>${dom4j.version}</version>
+    </dependency>
+    <dependency>
+      <groupId>com.cloudbees</groupId>
+      <artifactId>syslog-java-client</artifactId>
+      <version>1.1.7</version>
+    </dependency>
     <dependency>
       <groupId>com.opentable.components</groupId>
       <artifactId>otj-pg-embedded</artifactId>
       <version>0.13.1</version>
       <scope>test</scope>
     </dependency>
     <dependency>
-        <groupId>com.github.mjeanroy</groupId>
-        <artifactId>dbunit-plus</artifactId>
-        <version>2.0.1</version>
-        <scope>test</scope>
+      <groupId>org.springframework.boot</groupId>
+      <artifactId>spring-boot-starter-test</artifactId>
+      <version>${spring.boot.version}</version>
+      <scope>test</scope>
+        <exclusions>
+            <exclusion>
+                <groupId>com.vaadin.external.google</groupId>
+                <artifactId>android-json</artifactId>
+            </exclusion>
+        </exclusions>
     </dependency>
     <dependency>
-      <groupId>org.dom4j</groupId>
-      <artifactId>dom4j</artifactId>
-      <version>${dom4j.version}</version>
+      <groupId>org.dbunit</groupId>
+      <artifactId>dbunit</artifactId>
+      <version>2.7.0</version>
+      <scope>test</scope>
     </dependency>
     <dependency>
-      <groupId>com.cloudbees</groupId>
-      <artifactId>syslog-java-client</artifactId>
-      <version>1.1.7</version>
+      <groupId>com.github.springtestdbunit</groupId>
+      <artifactId>spring-test-dbunit</artifactId>
+      <version>1.3.0</version>
+      <scope>test</scope>
+    </dependency>
+    <dependency>
+      <groupId>pl.pragmatists</groupId>
+      <artifactId>JUnitParams</artifactId>
+      <version>1.1.0</version>
+      <scope>test</scope>
+    </dependency>
+    <dependency>
+        <groupId>com.github.mjeanroy</groupId>
+        <artifactId>dbunit-plus</artifactId>
+        <version>2.0.1</version>
+        <scope>test</scope>
     </dependency>
   </dependencies>
   <profiles>
-
     <profile>
       <id>webapi-oracle</id>
       <properties>

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

@@ -4,7 +4,6 @@
 import com.cosium.spring.data.jpa.entity.graph.domain.EntityGraphUtils;
 import org.apache.shiro.authz.UnauthorizedException;
 import org.ohdsi.webapi.model.CommonEntity;
-import org.ohdsi.webapi.security.dto.RoleDTO;
 import org.ohdsi.webapi.security.model.EntityPermissionSchema;
 import org.ohdsi.webapi.security.model.EntityPermissionSchemaResolver;
 import org.ohdsi.webapi.security.model.EntityType;
@@ -34,16 +33,15 @@
 import javax.annotation.PostConstruct;
 import java.io.Serializable;
 import java.util.Arrays;
-import java.util.Collection;
-import java.util.HashMap;
-import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.function.Function;
 import java.util.stream.Collectors;
+import org.apache.shiro.SecurityUtils;
+import org.apache.shiro.authz.Permission;
+import org.apache.shiro.authz.permission.WildcardPermission;
+import org.apache.shiro.subject.Subject;
 
 @Service
 public class PermissionService {
@@ -64,9 +62,6 @@ public class PermissionService {
     @Value("#{!'${security.provider}'.equals('DisabledSecurity')}")
     private boolean securityEnabled;
 
-    private ThreadLocal<ConcurrentHashMap<EntityType, ConcurrentHashMap<String, Set<RoleDTO>>>> permissionCache =
-            ThreadLocal.withInitial(ConcurrentHashMap::new);
-
     private final EntityGraph PERMISSION_ENTITY_GRAPH = EntityGraphUtils.fromAttributePaths("rolePermissions", "rolePermissions.role");
 
     public PermissionService(
@@ -133,14 +128,14 @@ public void checkCommonEntityOwnership(EntityType entityType, Integer entityId)
 
     public Map<String, String> getPermissionTemplates(EntityPermissionSchema permissionSchema, AccessType accessType) {
 
-        switch (accessType) {
-            case WRITE:
-                return permissionSchema.getWritePermissions();
- 	    case READ:
-	        return permissionSchema.getReadPermissions();
-            default:
-                throw new UnsupportedOperationException();
-        }
+      switch (accessType) {
+        case WRITE:
+          return permissionSchema.getWritePermissions();
+        case READ:
+          return permissionSchema.getReadPermissions();
+        default:
+          throw new UnsupportedOperationException();
+      }
     }
 
     public List<RoleEntity> finaAllRolesHavingPermissions(List<String> permissions) {
@@ -178,97 +173,28 @@ private boolean isCurrentUserOwnerOf(CommonEntity entity) {
         return Objects.equals(owner.getLogin(), loggedInUsername);
     }
 
+    public List<Permission> getEntityPermissions(EntityType entityType, Number id, AccessType accessType) {
+        Set<String> permissionTemplates = getTemplatesForType(entityType, accessType).keySet();
 
-    public void preparePermissionCache(EntityType entityType, Set<String> permissionTemplates) {
-        if (permissionCache.get().get(entityType) == null) {
-            final ConcurrentHashMap<String, Set<RoleDTO>> rolesForEntity = new ConcurrentHashMap<>();
-            permissionCache.get().put(entityType, rolesForEntity);
-
-            List<String> permissionsSQLTemplates = permissionTemplates.stream()
-                    .map(pt -> getPermissionSqlTemplate(pt))
-                    .collect(Collectors.toList());
-
-            Map<Long, RoleDTO> roleDTOMap = new HashMap<>();
-            permissionsSQLTemplates.forEach(p -> {
-                Iterable<PermissionEntity> permissionEntities = permissionRepository.findByValueLike(p, PERMISSION_ENTITY_GRAPH);
-                for (PermissionEntity permissionEntity : permissionEntities) {
-                    Set<RoleDTO> roles = rolesForEntity.get(permissionEntity.getValue());
-                    if (roles == null) {
-                        rolesForEntity.put(permissionEntity.getValue(), new HashSet<>());
-                    }
-                    Set<RoleDTO> cachedRoles = rolesForEntity.get(permissionEntity.getValue());
-                    permissionEntity.getRolePermissions().forEach(rp -> {
-                        RoleDTO roleDTO = roleDTOMap.get(rp.getRole().getId());
-                        if (roleDTO == null) {
-                            roleDTO = conversionService.convert(rp.getRole(), RoleDTO.class);
-                            roleDTOMap.put(roleDTO.getId(), roleDTO);
-                        }
-                        cachedRoles.add(roleDTO);
-                    });
-                }
-            });
-        }
-    }
-
-    public List<RoleDTO> getRolesHavingPermissions(EntityType entityType, Number id) {
-        Set<String> permissionTemplates = getTemplatesForType(entityType, AccessType.WRITE).keySet();
-        preparePermissionCache(entityType, permissionTemplates);
-
-        List<String> permissions = permissionTemplates.stream()
-                .map(pt -> getPermission(pt, id))
+        List<Permission> permissions = permissionTemplates.stream()
+                .map(pt -> new WildcardPermission(getPermission(pt, id)))
                 .collect(Collectors.toList());
-        int fitCount = permissions.size();
-        Map<RoleDTO, Long> roleMap = permissions.stream()
-                .filter(p -> permissionCache.get().get(entityType).get(p) != null)
-                .flatMap(p -> permissionCache.get().get(entityType).get(p).stream())
-                .collect(Collectors.groupingBy(Function.identity(), Collectors.counting()));
-        List<RoleDTO> roles = roleMap.entrySet().stream()
-                .filter(es -> es.getValue() == fitCount)
-                .map(es -> es.getKey())
-                .collect(Collectors.toList());
-        return roles;
-    }
-
-    public List<RoleDTO> getRolesHavingReadPermissions(EntityType entityType, Number id) {
-        Set<String> permissionTemplates = getTemplatesForType(entityType, AccessType.READ).keySet();
-        preparePermissionCache(entityType, permissionTemplates);
-
-        List<String> permissions = permissionTemplates.stream()
-                .map(pt -> getPermission(pt, id))
-                .collect(Collectors.toList());
-        int fitCount = permissions.size();
-        Map<RoleDTO, Long> roleMap = permissions.stream()
-                .filter(p -> permissionCache.get().get(entityType).get(p) != null)
-                .flatMap(p -> permissionCache.get().get(entityType).get(p).stream())
-                .collect(Collectors.groupingBy(Function.identity(), Collectors.counting()));
-        List<RoleDTO> roles = roleMap.entrySet().stream()
-                .filter(es -> es.getValue() == fitCount)
-                .map(es -> es.getKey())
-                .collect(Collectors.toList());
-        return roles;
-    }
-
-    public void clearPermissionCache() {
-        this.permissionCache.set(new ConcurrentHashMap<>());
+        return permissions;
     }
 
-    public boolean hasWriteAccess(CommonEntity entity) {
+    public boolean hasAccess(CommonEntity entity, AccessType accessType) {
         boolean hasAccess = false;
         if (securityEnabled && entity.getCreatedBy() != null) {
             try {
+                Subject subject = SecurityUtils.getSubject();
                 String login = this.permissionManager.getSubjectName();
                 UserSimpleAuthorizationInfo authorizationInfo = this.permissionManager.getAuthorizationInfo(login);
                 if (Objects.equals(authorizationInfo.getUserId(), entity.getCreatedBy().getId())) {
                     hasAccess = true; // the role is the one that created the artifact
                 } else {
                     EntityType entityType = entityPermissionSchemaResolver.getEntityType(entity.getClass());
-
-                    List<RoleDTO> roles = getRolesHavingPermissions(entityType, entity.getId());
-
-                    Collection<String> userRoles = authorizationInfo.getRoles();
-                    hasAccess = roles.stream()
-                            .anyMatch(r -> userRoles.stream()
-                            .anyMatch(re -> re.equals(r.getName())));
+                    List<Permission> permsToCheck = getEntityPermissions(entityType, entity.getId(), accessType);
+                    hasAccess = permsToCheck.stream().allMatch(p -> subject.isPermitted(p));
                 }
             } catch (Exception e) {
                 logger.error("Error getting user roles and permissions", e);
@@ -277,43 +203,24 @@ public boolean hasWriteAccess(CommonEntity entity) {
         }
         return hasAccess;
     }
-
+    
+    public boolean hasWriteAccess(CommonEntity entity) {
+      return hasAccess(entity, AccessType.WRITE);
+    }
 
     public boolean hasReadAccess(CommonEntity entity) {
-        boolean hasAccess = false;
-        if (securityEnabled && entity.getCreatedBy() != null) {
-            try {
-                String login = this.permissionManager.getSubjectName();
-                UserSimpleAuthorizationInfo authorizationInfo = this.permissionManager.getAuthorizationInfo(login);
-                if (Objects.equals(authorizationInfo.getUserId(), entity.getCreatedBy().getId())){
-		    hasAccess = true; // the role is the one that created the artifact
-		} else {
-                    EntityType entityType = entityPermissionSchemaResolver.getEntityType(entity.getClass());
-
-                    List<RoleDTO> roles = getRolesHavingReadPermissions(entityType, entity.getId());
-
-                    Collection<String> userRoles = authorizationInfo.getRoles();
-                    hasAccess = roles.stream()
-                            .anyMatch(r -> userRoles.stream()
-                                    .anyMatch(re -> re.equals(r.getName())));
-                }
-            } catch (Exception e) {
-                logger.error("Error getting user roles and permissions", e);
-                throw new RuntimeException(e);
-            }
-        }
-        return hasAccess;
+      return hasAccess(entity, AccessType.READ);
     }
 
     public void fillWriteAccess(CommonEntity entity, CommonEntityDTO entityDTO) {
         if (securityEnabled && entity.getCreatedBy() != null) {
-            entityDTO.setHasWriteAccess(hasWriteAccess(entity));
+            entityDTO.setHasWriteAccess(hasAccess(entity, AccessType.WRITE));
         }
     }
 
     public void fillReadAccess(CommonEntity entity, CommonEntityDTO entityDTO) {
         if (securityEnabled && entity.getCreatedBy() != null) {
-            entityDTO.setHasReadAccess(hasReadAccess(entity));
+            entityDTO.setHasReadAccess(hasAccess(entity, AccessType.READ));
         }
     }
 

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

@@ -1,25 +1,39 @@
 package org.ohdsi.webapi.security.model;
 
+import java.util.List;
+import java.util.Map;
+import org.apache.shiro.authz.Permission;
 import org.apache.shiro.authz.SimpleAuthorizationInfo;
 
 public class UserSimpleAuthorizationInfo extends SimpleAuthorizationInfo {
-    private Long userId;
 
-    private String login;
+  private Long userId;
+  private String login;
+  private Map<String,List<Permission>> permissionIdx;
 
-    public Long getUserId() {
-        return userId;
-    }
+  
+  public Long getUserId() {
+    return userId;
+  }
 
-    public void setUserId(Long userId) {
-        this.userId = userId;
-    }
+  public void setUserId(Long userId) {
+    this.userId = userId;
+  }
 
-    public String getLogin() {
-        return login;
-    }
+  public String getLogin() {
+    return login;
+  }
+
+  public void setLogin(String login) {
+    this.login = login;
+  }
+  
+  public Map<String, List<Permission>> getPermissionIdx() {
+    return permissionIdx;
+  }
+
+  public void setPermissionIdx(Map<String, List<Permission>> permissionIdx) {
+    this.permissionIdx = permissionIdx;
+  }
 
-    public void setLogin(String login) {
-        this.login = login;
-    }
 }

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

@@ -51,7 +51,7 @@ public static class User implements Comparable<User> {
     public String login;
     public String name;
     public List<Permission> permissions;
-    public JsonNode permissionIdx;
+    public Map<String, List<String>> permissionIdx;
 
     public User() {}