From 27bf0c939b0076c9fc4d2292dc91f55a7e2052c6 Mon Sep 17 00:00:00 2001
From: Sonia Zorba <sonia.zorba@inaf.it>
Date: Fri, 26 Mar 2021 15:50:07 +0100
Subject: [PATCH] isMemberOf bugfix
---
.../controller/JWTWebServiceController.java | 40 ++++++++---------
.../ia2/gms/persistence/MembershipsDAO.java | 14 +++---
.../ia2/gms/service/GroupNameService.java | 6 +++
.../JWTWebServiceControllerTest.java | 10 +++--
.../gms/persistence/MembershipsDAOTest.java | 45 +++++++++++++++++--
5 files changed, 83 insertions(+), 32 deletions(-)
diff --git a/gms/src/main/java/it/inaf/ia2/gms/controller/JWTWebServiceController.java b/gms/src/main/java/it/inaf/ia2/gms/controller/JWTWebServiceController.java
index a3d57a0..95fe2b1 100644
--- a/gms/src/main/java/it/inaf/ia2/gms/controller/JWTWebServiceController.java
+++ b/gms/src/main/java/it/inaf/ia2/gms/controller/JWTWebServiceController.java
@@ -110,31 +110,31 @@ public class JWTWebServiceController {
@GetMapping(value = {"/ws/jwt/search/**", "/vo/search/**"}, produces = MediaType.TEXT_PLAIN_VALUE)
public void isMemberOf(HttpServletRequest request, HttpServletResponse response) throws IOException {
- String group = getGroupFromRequest(request, "/ws/jwt/search/", "/vo/search/");
-
- List<String> groupNames = groupNameService.extractGroupNames(group);
-
- boolean isMember = membershipManager.isCurrentUserMemberOf("ROOT");
- if (!isMember) {
- String parentPath = ""; // starting from ROOT
- for (String groupName : groupNames) {
- Optional<GroupEntity> optionalGroup = groupsDAO.findGroupByParentAndName(parentPath, groupName);
- if (optionalGroup.isPresent()) {
- GroupEntity groupEntity = optionalGroup.get();
- parentPath = groupEntity.getPath();
- isMember = membershipManager.isCurrentUserMemberOf(groupEntity.getId());
- if (isMember) {
- break;
- }
- } else {
- break;
+ String groupNamesString = getGroupFromRequest(request, "/ws/jwt/search/", "/vo/search/");
+
+ List<String> groupNames = groupNameService.extractGroupNames(groupNamesString);
+
+ GroupEntity group = null;
+
+ String parentPath = ""; // starting from ROOT
+ for (String groupName : groupNames) {
+ Optional<GroupEntity> optionalGroup = groupsDAO.findGroupByParentAndName(parentPath, groupName);
+ if (optionalGroup.isPresent()) {
+ GroupEntity groupEntity = optionalGroup.get();
+ parentPath = groupEntity.getPath();
+ boolean isMember = membershipManager.isCurrentUserMemberOf(groupEntity.getId());
+ if (isMember) {
+ group = groupEntity;
}
+ } else {
+ group = null;
+ break;
}
}
- if (isMember) {
+ if (group != null) {
try ( PrintWriter pw = new PrintWriter(response.getOutputStream())) {
- pw.println(group);
+ pw.println(groupNameService.getCompleteName(groupNames));
}
}
// else: empty response (as defined by GMS standard)
diff --git a/gms/src/main/java/it/inaf/ia2/gms/persistence/MembershipsDAO.java b/gms/src/main/java/it/inaf/ia2/gms/persistence/MembershipsDAO.java
index 2a4309d..06476e9 100644
--- a/gms/src/main/java/it/inaf/ia2/gms/persistence/MembershipsDAO.java
+++ b/gms/src/main/java/it/inaf/ia2/gms/persistence/MembershipsDAO.java
@@ -120,18 +120,22 @@ public class MembershipsDAO {
public boolean isMemberOf(String userId, String groupId) {
- String sql = "SELECT COUNT(*) FROM gms_membership "
- + " WHERE user_id = ? AND group_id = ?";
+ String sql = "SELECT COUNT(*)\n"
+ + "FROM gms_membership m\n"
+ + "JOIN gms_group g ON g.id = m.group_id\n"
+ + "JOIN gms_group gs ON g.path @> gs.path\n"
+ + "WHERE gs.id = ?\n"
+ + "AND m.user_id = ?";
return jdbcTemplate.query(conn -> {
PreparedStatement ps = conn.prepareStatement(sql);
- ps.setString(1, userId);
- ps.setString(2, groupId);
+ ps.setString(1, groupId);
+ ps.setString(2, userId);
return ps;
}, resultSet -> {
resultSet.next();
int count = resultSet.getInt(1);
- return count == 1;
+ return count > 0;
});
}
diff --git a/gms/src/main/java/it/inaf/ia2/gms/service/GroupNameService.java b/gms/src/main/java/it/inaf/ia2/gms/service/GroupNameService.java
index f6651e0..8b58646 100644
--- a/gms/src/main/java/it/inaf/ia2/gms/service/GroupNameService.java
+++ b/gms/src/main/java/it/inaf/ia2/gms/service/GroupNameService.java
@@ -157,6 +157,12 @@ public class GroupNameService {
return names;
}
+ public String getCompleteName(List<String> names) {
+ return String.join(".", names.stream()
+ .map(n -> n.replace(".", "\\."))
+ .collect(Collectors.toList()));
+ }
+
private GroupEntity getRoot() {
return groupsDAO.findGroupById("ROOT")
.orElseThrow(() -> new IllegalStateException("Missing root group"));
diff --git a/gms/src/test/java/it/inaf/ia2/gms/controller/JWTWebServiceControllerTest.java b/gms/src/test/java/it/inaf/ia2/gms/controller/JWTWebServiceControllerTest.java
index 952f33d..2f4b2a1 100644
--- a/gms/src/test/java/it/inaf/ia2/gms/controller/JWTWebServiceControllerTest.java
+++ b/gms/src/test/java/it/inaf/ia2/gms/controller/JWTWebServiceControllerTest.java
@@ -115,11 +115,15 @@ public class JWTWebServiceControllerTest {
when(membershipManager.isCurrentUserMemberOf(eq(group3.getId()))).thenReturn(true);
- String group = "group\\.1.subgroup.subsubgroup";
+ String group = "group%5C.1.subgroup.subsubgroup";
- mockMvc.perform(get("/ws/jwt/search/" + group).principal(principal))
+ mockMvc.perform(get("/vo/search/" + group).principal(principal))
.andExpect(status().isOk())
- .andExpect(content().string(group + "\n"));
+ .andExpect(content().string("group\\.1.subgroup.subsubgroup\n"));
+
+ mockMvc.perform(get("/vo/search/" + group + "/inexistent").principal(principal))
+ .andExpect(status().isOk())
+ .andExpect(content().string(""));
}
@Test
diff --git a/gms/src/test/java/it/inaf/ia2/gms/persistence/MembershipsDAOTest.java b/gms/src/test/java/it/inaf/ia2/gms/persistence/MembershipsDAOTest.java
index cb32b19..4529f49 100644
--- a/gms/src/test/java/it/inaf/ia2/gms/persistence/MembershipsDAOTest.java
+++ b/gms/src/test/java/it/inaf/ia2/gms/persistence/MembershipsDAOTest.java
@@ -37,8 +37,8 @@ public class MembershipsDAOTest {
@Test
public void testAddAndRemoveMembers() {
- groupsDAO.createGroup(groupEntity("A"));
- groupsDAO.createGroup(groupEntity("B"));
+ groupsDAO.createGroup(groupEntity("A", "A"));
+ groupsDAO.createGroup(groupEntity("B", "B"));
assertTrue(membershipsDAO.findByGroup("A").isEmpty());
@@ -72,11 +72,48 @@ public class MembershipsDAOTest {
membershipsDAO.deleteAllGroupsMembership(new ArrayList<>());
}
- private GroupEntity groupEntity(String groupId) {
+ @Test
+ public void testIsMemberOfRecursive() {
+
+ groupsDAO.createGroup(groupEntity("ROOT", ""));
+
+ groupsDAO.createGroup(groupEntity("A", "A"));
+ groupsDAO.createGroup(groupEntity("E", "A.E"));
+ groupsDAO.createGroup(groupEntity("F", "A.E.F"));
+
+ groupsDAO.createGroup(groupEntity("B", "B"));
+ groupsDAO.createGroup(groupEntity("C", "B.C"));
+ groupsDAO.createGroup(groupEntity("D", "B.C.D"));
+
+ membershipsDAO.addMember(membershipEntity(USER_1, "F"));
+ membershipsDAO.addMember(membershipEntity(USER_1, "C"));
+
+ assertFalse(membershipsDAO.isMemberOf(USER_1, "ROOT"));
+ assertFalse(membershipsDAO.isMemberOf(USER_1, "A"));
+ assertFalse(membershipsDAO.isMemberOf(USER_1, "B"));
+ assertTrue(membershipsDAO.isMemberOf(USER_1, "C"));
+ assertTrue(membershipsDAO.isMemberOf(USER_1, "D"));
+ assertFalse(membershipsDAO.isMemberOf(USER_1, "E"));
+ assertTrue(membershipsDAO.isMemberOf(USER_1, "F"));
+
+ membershipsDAO.addMember(membershipEntity(USER_1, "ROOT"));
+
+ assertTrue(membershipsDAO.isMemberOf(USER_1, "ROOT"));
+ assertTrue(membershipsDAO.isMemberOf(USER_1, "A"));
+ assertTrue(membershipsDAO.isMemberOf(USER_1, "B"));
+ assertTrue(membershipsDAO.isMemberOf(USER_1, "C"));
+ assertTrue(membershipsDAO.isMemberOf(USER_1, "D"));
+ assertTrue(membershipsDAO.isMemberOf(USER_1, "E"));
+ assertTrue(membershipsDAO.isMemberOf(USER_1, "F"));
+
+ assertFalse(membershipsDAO.isMemberOf(USER_1, "inexistent-id"));
+ }
+
+ private GroupEntity groupEntity(String groupId, String groupPath) {
GroupEntity groupEntity = new GroupEntity();
groupEntity.setId(groupId);
groupEntity.setName(groupId);
- groupEntity.setPath(groupId);
+ groupEntity.setPath(groupPath);
groupEntity.setLeaf(false);
return groupEntity;
}
--
GitLab