Permalink
Browse files

WL-3191 Only count READ roles

When determining which roles are defined for role-based access, only
consider those that grant READ access.

This exists because it is possible to define a role which doesn't grant
read access but will still consider this as role-based access.

This currently blocks adding role-based access and removing it.
  • Loading branch information...
Ben Holmes
Ben Holmes committed Feb 3, 2014
1 parent e351a72 commit 4e5b24b54a945bf8ad55b7ac27908132577835b3
@@ -9138,7 +9138,9 @@ public boolean isInheritingRoleView(final String id, final String roleId) {
Set<Role> roles = realm.getRoles();
for (Role role : roles) {
roleIds.add(role.getId());
if(role.isAllowed(AUTH_RESOURCE_READ)) {
roleIds.add(role.getId());
}
}
return roleIds;

3 comments on commit 4e5b24b

@buckett

This comment has been minimized.

Show comment
Hide comment
@buckett

buckett Feb 4, 2014

Member

Despite your full commit message, why should we only return roles that can read resources?

Member

buckett replied Feb 4, 2014

Despite your full commit message, why should we only return roles that can read resources?

@binhums

This comment has been minimized.

Show comment
Hide comment
@binhums

binhums Feb 4, 2014

Contributor

I think because we are talking specifically about the RoleView methods (which looking back is a terrible name).

When we disable RoleView or test with isRoleView it sets / tests permission on AUTH_RESOURCE_READ. Why that decision was made I'm not absolutely sure.

If we return roles that can't read resources then we shouldn't consider them to have 'RoleView', i.e. role-based access.

Contributor

binhums replied Feb 4, 2014

I think because we are talking specifically about the RoleView methods (which looking back is a terrible name).

When we disable RoleView or test with isRoleView it sets / tests permission on AUTH_RESOURCE_READ. Why that decision was made I'm not absolutely sure.

If we return roles that can't read resources then we shouldn't consider them to have 'RoleView', i.e. role-based access.

@buckett

This comment has been minimized.

Show comment
Hide comment
@buckett

buckett Feb 4, 2014

Member

From the discussion in the room there is already code in BaseContentService that does these permission checks and this is just bringing this method "into line".

Member

buckett replied Feb 4, 2014

From the discussion in the room there is already code in BaseContentService that does these permission checks and this is just bringing this method "into line".

Please sign in to comment.