Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve filtering by role #222

Closed
etj opened this issue Aug 22, 2022 · 2 comments
Closed

Improve filtering by role #222

etj opened this issue Aug 22, 2022 · 2 comments

Comments

@etj
Copy link
Member

etj commented Aug 22, 2022

A GeoServer instance may have multiple chained authentication services, providing different role semantics.

As a use case, there is an instance having

  • an auth service:
    • Access via authkey
    • Accessing with two different authkey, the same user may be belonging to different groups (with different privs)
    • The geoserver auth class selects the proper role and set it into the request object
    • In this case GeoFence should use that role in the rule filtering --> we need the "Use GEOSERVER roles to get authorizations" and pass to GeoFence the roles selected by the auth plugin.
  • a geonode auth service:
    • Access via credentials/authkey
    • The credentials uniquely identify the user, which may belong to one or more roles
    • GeoFence should use all the roles in the rule filtering and merge all the privs
    • Unsetting "Use GEOSERVER roles to get authorizations" will make the geofence client create a filter without any role set, so GeoFence will retrieve all the roles associated to the user, get the privs associate to each role and merge them.

Role filtering

In order to make the first auth service work, we must enable "Use GEOSERVER roles to get authorizations".
Please note that such an option requires a list of role names (a text field labeled "comma delimited list of mutually exclusive roles for authorization"): the GeoFenceAccessManager will only consider a role passed in the request instance only if it's in the role name list.
This behaviour was too limiting in some cases where the role names are not static so, as reported in a previous comment and documented in GEOS-10420, an improvement was implemented in order to use the first of the roles passed in the request object (so the role name is dynamic and not pre-determined).
Please also note that the previous implementation would only use the last role matching in the intersection role list provided in the "comma delimited list of mutually exclusive roles for authorization" and the role list in the request.

There are several limitations and misleading behaviours in this implementation:

  • Using "*", only the first role found is passed in the GeoFence filter
    • it's not said the ordering of the roles in the request is fixed and that we are consistently taking the right role
    • usually "*" is meant to take "all elements found", but in this case it means "any one element"
  • at the moment GeoFence accepts filtering with a single role

Proposed solution

If "Use GEOSERVER roles to get authorizations" is selected and we have "*" as authorization groups, we will pass all the provided roles to GeoFence.

  • in djam case, we'll be passing the djam role plus any other general user role (such as "ROLE_AUTHENTICATED" or similar)
  • in geonode case, we'll be passing all the roles which GeoFence would otherwise retrieve from the GroupRoleProvider (which should be the very same list).
    This would solve the misleading behaviour described above.

Code changes

geofence:RuleFilter

The RuleFilter should allow for multiple roles.
Changing the bean could be a risk for back compatibility: consider implementing this change by packing all the role names in a single comma separated textin order to not change the DTO. In this case we should enforce some ordering of the role names, in order to preserve the instance hash.

geoserver:GeofenceAccessManager

The setRuleFilterUserOrRole method should be able to set multiple roles into RuleFilter

geofence:RuleReaderServiceImpl

The logic should be changed in order to use multiple roles passed in the filter.

Pls note the the changes in RuleReaderServiceImpl may require extensive changes: at the moment GeoServer can request authorization either by user or by role, in a mutually exlusive way.

  • If auth by user is requested, GeoFence will retrieve all the rules for the user and for the assigned roles found in the DB.
  • If auth by role is requested, GeoFence will only apply rules related to the group.
    This behaviour should be improved (yet being backcompatible)

Other changes

"Use GEOSERVER roles to get authorizations" should read "User authentication roles to get authorizations"

@etj
Copy link
Member Author

etj commented Aug 22, 2022

Created related GeoServer ticket at GEOS-10625

@etj etj self-assigned this Aug 23, 2022
etj added a commit that referenced this issue Aug 26, 2022
#202 Remove SecurityContextHolder stuff
@etj
Copy link
Member Author

etj commented Aug 26, 2022

This test covers all possible user/role combinations:

public void testMultiRoles() {
RuleFilter filter;
filter = new RuleFilter(RuleFilter.SpecialFilterType.ANY);
assertEquals(0, ruleAdminService.count(filter));
UserGroup p1 = createRole("p1");
UserGroup p2 = createRole("p2");
UserGroup p3 = createRole("p3");
String u1 = "TestUser1";
String u2 = "TestUser2";
String u3 = "TestUser3";
GSUser user1 = new GSUser();
user1.setName(u1);
user1.getGroups().add(p1);
GSUser user2 = new GSUser();
user2.setName(u2);
user2.getGroups().add(p2);
GSUser user12 = new GSUser();
user12.setName(u3);
user12.getGroups().add(p1);
user12.getGroups().add(p2);
userAdminService.insert(user1);
userAdminService.insert(user2);
userAdminService.insert(user12);
ruleAdminService.insert(new Rule(10, u1, "p1", null, null, "s1", "r1", "w1", "l1", GrantType.ALLOW));
ruleAdminService.insert(new Rule(20, u2, "p2", null, null, "s1", "r2", "w2", "l2", GrantType.ALLOW));
ruleAdminService.insert(new Rule(30, u1, null, null, null, null, null, null, null, GrantType.ALLOW));
ruleAdminService.insert(new Rule(40, u2, null, null, null, null, null, null, null, GrantType.ALLOW));
ruleAdminService.insert(new Rule(50, u3, null, null, null, null, null, null, null, GrantType.ALLOW));
ruleAdminService.insert(new Rule(51, u3, "p1", null, null, null, null, null, null, GrantType.ALLOW));
ruleAdminService.insert(new Rule(52, u3, "p2", null, null, null, null, null, null, GrantType.ALLOW));
ruleAdminService.insert(new Rule(60, null,"p1", null, null, null, null, null, null, GrantType.ALLOW));
ruleAdminService.insert(new Rule(70, null,"p2", null, null, null, null, null, null, GrantType.ALLOW));
ruleAdminService.insert(new Rule(80, null,"p3", null, null, null, null, null, null, GrantType.ALLOW));
ruleAdminService.insert(new Rule(901, u1, "p2", null, null, null, null, null, null, GrantType.ALLOW));
ruleAdminService.insert(new Rule(902, u2, "p1", null, null, null, null, null, null, GrantType.ALLOW));
ruleAdminService.insert(new Rule(999, null, null, null, null, null, null, null, null, GrantType.ALLOW));
assertRules(createFilter("*", "*"), new Integer[]{10,20,30,40,50,51,52,60,70,80,901,902,999});
assertRules(createFilter("*", null), new Integer[]{30,40,50,999});
assertRules(createFilter("*", "NO"), new Integer[]{30,40,50,999});
assertRules(createFilter("*", "p1"), new Integer[]{10,30,40,50,51,60,902,999});
assertRules(createFilter("*", "p1,NO"), new Integer[]{10,30,40,50,51,60,902,999});
assertRules(createFilter("*", "p1,p2"), new Integer[]{10,20,30,40,50,51,52,60,70,901,902,999});
assertRules(createFilter("*", "p1,p2,NO"), new Integer[]{10,20,30,40,50,51,52,60,70,901,902,999});
assertRules(createFilter(null, "*"), new Integer[]{60,70,80,999});
assertRules(createFilter(null, null), new Integer[]{999});
assertRules(createFilter(null, "NO"), new Integer[]{999});
assertRules(createFilter(null, "p1"), new Integer[]{60,999});
assertRules(createFilter(null, "p1,NO"), new Integer[]{60,999});
assertRules(createFilter(null, "p1,p2"), new Integer[]{60,70,999});
assertRules(createFilter(null, "p1,p2,NO"), new Integer[]{60,70,999});
assertRules(createFilter("NO", "*"), new Integer[]{999});
assertRules(createFilter("NO", null), new Integer[]{999});
assertRules(createFilter("NO", "NO"), new Integer[]{999});
assertRules(createFilter("NO","p1"), new Integer[]{999});
assertRules(createFilter("NO","p1,NO"), new Integer[]{999});
assertRules(createFilter("NO","p1,p2"), new Integer[]{999});
assertRules(createFilter("NO","p1,p2,NO"), new Integer[]{999});
assertRules(createFilter(u1, "*"), new Integer[]{10,30,60,999});
assertRules(createFilter(u1, null), new Integer[]{30,999});
assertRules(createFilter(u1, "NO"), new Integer[]{30,999});
assertRules(createFilter(u1, "p1"), new Integer[]{10,30,60,999});
assertRules(createFilter(u1, "p1,NO"), new Integer[]{10,30,60,999});
assertRules(createFilter(u1, "p1,p2"), new Integer[]{10,30,60,999});
assertRules(createFilter(u1, "p1,p2,NO"), new Integer[]{10,30,60,999});
assertRules(createFilter(u3, "*"), new Integer[]{50,51,52,60,70,999});
assertRules(createFilter(u3, null), new Integer[]{50,999});
assertRules(createFilter(u3, "NO"), new Integer[]{50,999});
assertRules(createFilter(u3, "p1"), new Integer[]{50,51,60,999});
assertRules(createFilter(u3, "p2"), new Integer[]{50,52,70,999});
assertRules(createFilter(u3, "p1,NO"), new Integer[]{50,51,60,999});
assertRules(createFilter(u3, "p1,p2"), new Integer[]{50,51,52,60,70,999});
assertRules(createFilter(u3, "p1,p2,p3"), new Integer[]{50,51,52,60,70,999});
assertRules(createFilter(u3, "p1,p2,NO"), new Integer[]{50,51,52,60,70,999});
}

etj added a commit that referenced this issue Aug 29, 2022
#202 Remove SecurityContextHolder stuff
@etj etj closed this as completed Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant