has[Write|Read]Access does not use wildcard permissions

[chrisknoll]

Expected behavior

Wildcard (*) permissions should be honored when checking read/write access.

Actual behavior

When checking entity permissions via hasWriteAccess or hasReadAccess it is checking the entitySchema (which is a string template of permissions for the given entity) for an exact string match for the specific entity ID. This means that permission granted to the user via wildcard permissions (such as a role that grants write permission to everything via *) is not honored.

Ie: checking cohortdefinition:1:get against a grant of cohortdefinitiion:*:get will fail, when the wildcard should allow any ID to pass.

Steps to reproduce behavior

This is a symptom of #2323 , in so far as the wildcard permission to cohortdefinition:*:get should be seen as read permission. and in that case only the specific cohortdefinition:{id}:get is recognized as the read permission. But this is a general symptom that we're not using WildCard permissions when checking perms in some cases but we are in other (such as URL checks).

Technical Considerations

As I was researching how permissions were applied, I was greatly confused by the approach taken in the code, and will try to describe it here so that the original devs (or people closer to the code) can comment and clarify.

The hasWriteAccess() is doing something particularly strange: I would have expected that in order to determine if you have write permissions, you get the set of permissions from getTemplatesForType() and just check that the user has all necessary permissions in order to determine if the user has WRITE access. However, the code builds a permission cache in preparePermissionCache() which does the following:

    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);
                    });
                }
            });
        }
    }

in short: this will find all the roles that contain the specified permissions (from the template), and back in getRolesHavingPermissions it performs this work to return RoleDTOs that contain all the specified permissions in the template:

    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))
                .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;
    }

Note: using WildCard semantics, it's possible a person may match 2 permissions for the same check (the * check and the literal {id} check), throwing off the 'fitCount' check which says we want the roles that have exactly the correct permissions.

Back in hasWriteAccess:

                } 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())));
                }

This block appears to be saying : ' if the user has any role that matches any role that has the required write permissions matches any role that is assigned to the user.

This seems like a wildly round-about way to answer the question 'do you have write access'. If write access is determined by if they have the required permission, a permission grant of * should match all 5 of the required permissions and thus grant the user TRUE for write access.

However, I may be missing some underlying concept of this logic that is not clear. For example, maybe associating back to the role-level permissions handles some corner case...or maybe it's faster to check a set of roles a user has compared to a set of permissions (although we still need to check all the permissions to find the roles these permissions belong to). It also seems to force that all permissions must be found in a single role in order to 'count', but I'm not sure why that needs to be the case here: If RoleA grants some the required WRITE perms, and RoleB grants the rest, wouldn't a person assigned to RoleA and RoleB then have write perms to the entity?

I would appreciate if @alex-odysseus and members of @OHDSI/odysseus could speak on these considerations because they were responsible for the initial implementation.

[chrisknoll]

In lieu of feedback on this issue, I've crated PR #2355 to address these concerns.

@rkboyce : can you please grab this branch and see if your global read (and other expected permission assignments) work correctly under this branch? Note; this will require the Atlas version that is currently on master.