[issue-2953] Fixed access to resources for non-admin Tag Managers (having a 'tag:management' permission)

[oleg-odysseus]

Fixes OHDSI/Atlas#2953

[chrisknoll]

I noticed there's a few places where this was changed:

checkOwnerOrAdminOrGranted(entity);

to:

checkPermissions(entity, ImmutableSet.of(PermissionCheckType.IS_OWNER, PermissionCheckType.IS_ADMIN, PermissionCheckType.HAS_WRITE_ACCESS));

On the surface, it seems it makes sense to have a shared function that does a permission check that is shared across different code contexts. The naming of it seems little odd since the name of the function describes something literal (checking owner, or admin, or granted) vs. something like checkModifyPermissions, where 'modify permissions' can change over time (like adding a new permission role).

Did you think it makes sense to use a shared function in those places where you removed 'checkOwnerOrAdminOrGranted' instead of duplicating the set? I think having a shared function can encapsulate the logic about what the collection of roles implies a certain permission vs. deriving the meaning from an arbitrary collection of permissions/roles.

[oleg-odysseus]

Hi Chris. I`ve removed the refactoring part from this and issue-2953-2.14 branch to retain only the bug fix.