Skip to content

Commit

Permalink
#222 Improve filtering by role
Browse files Browse the repository at this point in the history 
#202 Remove SecurityContextHolder stuff
  • Loading branch information
@etj
etj committed Aug 26, 2022
1 parent 805cac4 commit 56af683
Show file tree
Hide file tree
Showing 4 changed files with 198 additions and 59 deletions.
18 changes: 17 additions & 1 deletion ...vices/core/services-api/src/main/java/org/geoserver/geofence/services/dto/RuleFilter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@
import org.geoserver.geofence.core.model.Rule;

import java.io.Serializable;
import java.util.Arrays;
import java.util.HashSet;
import java.util.Set;
import java.util.stream.Collectors;


/**
Expand All @@ -25,7 +29,7 @@
*/
public class RuleFilter implements Serializable, Cloneable {

private static final long serialVersionUID = 5629211135629700042L;
private static final long serialVersionUID = 5629211135629700043L;

public enum FilterType {

Expand Down Expand Up @@ -159,6 +163,7 @@ public RuleFilter setUser(SpecialFilterType type) {
public RuleFilter setRole(String name) {
if(name == null)
throw new NullPointerException();
name = sortNames(name); // force ordering to preserve hashing
role.setText(name);
return this;
}
Expand Down Expand Up @@ -276,6 +281,17 @@ public TextFilter getWorkspace() {
// return this;
// }


private String sortNames(String s) {
if(s.contains(",")) {
s = Arrays.asList(s.split(",")).stream()
.map(n -> n.trim())
.sorted()
.collect(Collectors.joining(","));
}
return s;
}

@Override
public boolean equals(Object obj) {
if (obj == null) {
Expand Down
30 changes: 30 additions & 0 deletions .../core/services-api/src/test/java/org/geoserver/geofence/services/util/RuleFilterTest.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
package org.geoserver.geofence.services.util;

import org.geoserver.geofence.services.dto.RuleFilter;
import static org.junit.Assert.*;
import org.junit.Test;

/**
*
*/
public class RuleFilterTest {
@Test
public void testRole() {
RuleFilter f = new RuleFilter();
f.setRole("pippo");
assertEquals("pippo", f.getRole().getText());

f.setRole("a,b");
assertEquals("a,b", f.getRole().getText());

f.setRole("b,a");
assertEquals("a,b", f.getRole().getText());

f.setRole("a, b");
assertEquals("a,b", f.getRole().getText());

f.setRole(" b , a ");
assertEquals("a,b", f.getRole().getText());
}

}
98 changes: 40 additions & 58 deletions ...re/services-impl/src/main/java/org/geoserver/geofence/services/RuleReaderServiceImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import org.geoserver.geofence.services.dto.AccessInfo;
import org.geoserver.geofence.services.dto.AuthUser;
import org.geoserver.geofence.services.dto.RuleFilter;
import org.geoserver.geofence.services.dto.RuleFilter.FilterType;
import org.geoserver.geofence.services.dto.RuleFilter.IdNameFilter;
import org.geoserver.geofence.services.dto.RuleFilter.SpecialFilterType;
import org.geoserver.geofence.services.dto.RuleFilter.TextFilter;
Expand All @@ -33,9 +32,6 @@
import org.opengis.referencing.crs.CoordinateReferenceSystem;
import org.opengis.referencing.operation.MathTransform;
import org.opengis.referencing.operation.TransformException;
import org.springframework.security.core.Authentication;
import org.springframework.security.core.GrantedAuthority;
import org.springframework.security.core.context.SecurityContextHolder;

import java.util.*;
import java.util.Map.Entry;
Expand Down Expand Up @@ -364,14 +360,19 @@ private String validateUsername(TextFilter filter) {
}
}

private String validateRolename(TextFilter filter) {
private List<String> validateRolenames(TextFilter filter) {

switch(filter.getType()) {
case NAMEVALUE:
String name = filter.getText();
if(StringUtils.isBlank(name) )
throw new BadRequestServiceEx("Blank role name");
return name.trim();
String names = filter.getText();
List<String> roles = new ArrayList<>();
for(String name : names.split(",")) {
if(StringUtils.isBlank(name) )
throw new BadRequestServiceEx("Blank role name");
roles.add(name.trim());
}
return roles;

case DEFAULT:
case ANY:
return null;
Expand Down Expand Up @@ -533,7 +534,7 @@ protected static CatalogMode getLarger(CatalogMode m1, CatalogMode m2) {
* username assigned and rolename:ANY -> should consider all the roles the user belongs to
* username:ANY and rolename assigned -> should consider all the users belonging to the given role
*
*
* @param filter a RuleFilter for rule selection. <B>side effect</B> May be changed by the method
* @return a Map having role names as keys, and the list of matching Rules as values. The NULL key holds the rules for the DEFAULT group.
*/
protected Map<String, List<Rule>> getRules(RuleFilter filter) throws BadRequestServiceEx {
Expand All @@ -547,7 +548,8 @@ protected Map<String, List<Rule>> getRules(RuleFilter filter) throws BadRequestS
Map<String, List<Rule>> ret = new HashMap<>();

if(finalRoleFilter.isEmpty()) {
List<Rule> found = getRuleAux(filter, filter.getRole());
TextFilter roleFilter = new TextFilter(filter.getRole().getType(), filter.getRole().isIncludeDefault());
List<Rule> found = getRuleAux(filter, roleFilter);
ret.put(null, found);
} else {
for (String role : finalRoleFilter) {
Expand Down Expand Up @@ -593,65 +595,45 @@ protected Set<String> validateUserRoles(RuleFilter filter) throws BadRequestServ
String username = validateUsername(filter.getUser());

// rolename can be null if the group filter asks for ANY or DEFAULT
String rolename = validateRolename(filter.getRole());
List<String> requestedRoles = validateRolenames(filter.getRole()); // CSV rolenames

// filtering by both user and role is pointless
if(username != null && rolename != null) {
throw new BadRequestServiceEx("You can filter either by user or role");
}

Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
Collection<? extends GrantedAuthority> authorities;
if (authentication == null) {
authorities = new HashSet<>();
} else {
authorities = authentication.getAuthorities();
}
Set<String> finalRoleFilter = new HashSet<>();
// If both user and group are defined in filter
// if user doensn't belong to group, no rule is returned
// if user doesn't belong to group, no rule is returned
// otherwise assigned or default rules are searched for
if(username != null) {
Set<String> assignedRoles = userResolver.getRoles(username);
if (authorities != null) {
for (GrantedAuthority authority : authorities) {
assignedRoles.add(authority.getAuthority());
}
}
if(rolename != null) {
if( assignedRoles.contains(rolename)) {
finalRoleFilter = Collections.singleton(rolename);

switch(filter.getRole().getType()) {
case NAMEVALUE:
if(username != null) {
Set<String> resolvedRoles = userResolver.getRoles(username);
for(String role: requestedRoles) {
if(resolvedRoles.contains(role)) {
finalRoleFilter.add(role);
} else {
LOGGER.warn("User does not belong to role [User:"+filter.getUser()+"] [Role:"+role+"] [ResolvedRoles:"+resolvedRoles+"]");
}
}
} else {
LOGGER.warn("User does not belong to role [User:"+filter.getUser()+"] [Role:"+filter.getRole()+"] [Roles:"+assignedRoles+"]");
return null;
finalRoleFilter.addAll(requestedRoles);
}
} else {
// User set and found, role (ANY, DEFAULT or notfound):
break;

if(filter.getRole().getType() == FilterType.ANY) {
if( ! assignedRoles.isEmpty()) {
finalRoleFilter = assignedRoles;
case ANY:
if(username != null) {
Set<String> resolvedRoles = userResolver.getRoles(username);
if( ! resolvedRoles.isEmpty()) {
finalRoleFilter = resolvedRoles;
} else {
filter.setRole(SpecialFilterType.DEFAULT);
}
} else {
// role is DEFAULT or not found:
// if role filter request DEFAULT -> ok, apply the filter
// if role does not exists, just apply the filter, even if probably no rule will match
// no changes, use requested filtering
}
}
} else {
// user is null: then either:
// 1) no filter on user was requested (ANY or DEFAULT)
// 2) user has not been found
if(rolename != null) {
finalRoleFilter.add(rolename);
} else if(filter.getUser().getType() != FilterType.ANY) {
filter.setRole(SpecialFilterType.DEFAULT);
} else {
// group is ANY, DEFAULT or not found:
// no grouping, use requested filtering
}
break;

case DEFAULT:
// no changes
break;
}

return finalRoleFilter;
Expand Down
111 changes: 111 additions & 0 deletions ...ervices-impl/src/test/java/org/geoserver/geofence/services/RuleReaderServiceImplTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
import java.util.Arrays;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import java.util.stream.Collectors;

import org.junit.AfterClass;
import org.junit.BeforeClass;
Expand Down Expand Up @@ -719,4 +721,113 @@ public void testAdminRules() {
assertTrue(accessInfo.getAdminRights());
}

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


private RuleFilter createFilter(String userName, String groupName) {
return new RuleFilter(userName, groupName, "*", "*", "*", "*", "*", "*");
}

private void assertRules(RuleFilter filter, Integer[] expectedPriorities) {
RuleFilter origFilter = filter.clone();
List<ShortRule> rules = ruleReaderService.getMatchingRules(filter);

Set<Long> pri = rules.stream()
.map(r -> r.getPriority())
.collect(Collectors.toSet());
Set<Long> exp = Arrays.asList(expectedPriorities).stream()
.map(i -> i.longValue())
.collect(Collectors.toSet());
assertEquals("Bad rule set selected for filter " + origFilter, exp, pri);
}

}

0 comments on commit 56af683

Please sign in to comment.