From 0c257b763d5f32fa8009aa65092e48a382829e86 Mon Sep 17 00:00:00 2001 From: Benjamin Gamard Date: Thu, 18 Oct 2018 18:57:06 +0200 Subject: [PATCH] Closes #245: admin group undeletable + admin can see all --- .../com/sismics/docs/core/dao/AclDao.java | 5 +++++ .../com/sismics/docs/core/dao/TagDao.java | 3 ++- .../sismics/docs/core/util/SecurityUtil.java | 12 ++++++++++ .../util/indexing/LuceneIndexingHandler.java | 13 ++++++----- .../docs/rest/resource/DocumentResource.java | 2 +- .../docs/rest/resource/GroupResource.java | 12 ++++++++++ .../docs/rest/resource/UserResource.java | 6 ++--- .../sismics/docs/rest/TestGroupResource.java | 22 +++++++++++++------ .../sismics/docs/rest/TestRouteResource.java | 18 +++++++-------- pom.xml | 2 +- 10 files changed, 67 insertions(+), 28 deletions(-) diff --git a/docs-core/src/main/java/com/sismics/docs/core/dao/AclDao.java b/docs-core/src/main/java/com/sismics/docs/core/dao/AclDao.java index 2b32396b..328bf1ac 100644 --- a/docs-core/src/main/java/com/sismics/docs/core/dao/AclDao.java +++ b/docs-core/src/main/java/com/sismics/docs/core/dao/AclDao.java @@ -7,6 +7,7 @@ import com.sismics.docs.core.constant.PermType; import com.sismics.docs.core.dao.dto.AclDto; import com.sismics.docs.core.model.jpa.Acl; import com.sismics.docs.core.util.AuditLogUtil; +import com.sismics.docs.core.util.SecurityUtil; import com.sismics.util.context.ThreadLocalContext; import javax.persistence.EntityManager; @@ -124,6 +125,10 @@ public class AclDao { * @return True if the document is accessible */ public boolean checkPermission(String sourceId, PermType perm, List targetIdList) { + if (SecurityUtil.skipAclCheck(targetIdList)) { + return true; + } + EntityManager em = ThreadLocalContext.get().getEntityManager(); StringBuilder sb = new StringBuilder("select a.ACL_ID_C from T_ACL a "); sb.append(" where a.ACL_TARGETID_C in (:targetIdList) and a.ACL_SOURCEID_C = :sourceId and a.ACL_PERM_C = :perm and a.ACL_DELETEDATE_D is null "); diff --git a/docs-core/src/main/java/com/sismics/docs/core/dao/TagDao.java b/docs-core/src/main/java/com/sismics/docs/core/dao/TagDao.java index 52ac570a..7a51071e 100644 --- a/docs-core/src/main/java/com/sismics/docs/core/dao/TagDao.java +++ b/docs-core/src/main/java/com/sismics/docs/core/dao/TagDao.java @@ -7,6 +7,7 @@ import com.sismics.docs.core.dao.dto.TagDto; import com.sismics.docs.core.model.jpa.DocumentTag; import com.sismics.docs.core.model.jpa.Tag; import com.sismics.docs.core.util.AuditLogUtil; +import com.sismics.docs.core.util.SecurityUtil; import com.sismics.docs.core.util.jpa.QueryParam; import com.sismics.docs.core.util.jpa.QueryUtil; import com.sismics.docs.core.util.jpa.SortCriteria; @@ -185,7 +186,7 @@ public class TagDao { criteriaList.add("t.TAG_ID_C = :id"); parameterMap.put("id", criteria.getId()); } - if (criteria.getTargetIdList() != null) { + if (criteria.getTargetIdList() != null && !SecurityUtil.skipAclCheck(criteria.getTargetIdList())) { sb.append(" left join T_ACL a on a.ACL_TARGETID_C in (:targetIdList) and a.ACL_SOURCEID_C = t.TAG_ID_C and a.ACL_PERM_C = 'READ' and a.ACL_DELETEDATE_D is null "); criteriaList.add("a.ACL_ID_C is not null"); parameterMap.put("targetIdList", criteria.getTargetIdList()); diff --git a/docs-core/src/main/java/com/sismics/docs/core/util/SecurityUtil.java b/docs-core/src/main/java/com/sismics/docs/core/util/SecurityUtil.java index 8142e930..8ab08151 100644 --- a/docs-core/src/main/java/com/sismics/docs/core/util/SecurityUtil.java +++ b/docs-core/src/main/java/com/sismics/docs/core/util/SecurityUtil.java @@ -6,6 +6,8 @@ import com.sismics.docs.core.dao.UserDao; import com.sismics.docs.core.model.jpa.Group; import com.sismics.docs.core.model.jpa.User; +import java.util.List; + /** * Security utilities. * @@ -37,4 +39,14 @@ public class SecurityUtil { return null; } + + /** + * Return true if the ACL targets provided don't need security checks (administrator users). + * + * @param targetIdList Target ID list + * @return True if skip ACL checks + */ + public static boolean skipAclCheck(List targetIdList) { + return targetIdList.contains("admin") || targetIdList.contains("administrators"); + } } diff --git a/docs-core/src/main/java/com/sismics/docs/core/util/indexing/LuceneIndexingHandler.java b/docs-core/src/main/java/com/sismics/docs/core/util/indexing/LuceneIndexingHandler.java index 3e53c17e..792ee54b 100644 --- a/docs-core/src/main/java/com/sismics/docs/core/util/indexing/LuceneIndexingHandler.java +++ b/docs-core/src/main/java/com/sismics/docs/core/util/indexing/LuceneIndexingHandler.java @@ -12,6 +12,7 @@ import com.sismics.docs.core.model.jpa.Config; import com.sismics.docs.core.model.jpa.Document; import com.sismics.docs.core.model.jpa.File; import com.sismics.docs.core.util.DirectoryUtil; +import com.sismics.docs.core.util.SecurityUtil; import com.sismics.docs.core.util.jpa.PaginatedList; import com.sismics.docs.core.util.jpa.PaginatedLists; import com.sismics.docs.core.util.jpa.QueryParam; @@ -229,11 +230,13 @@ public class LuceneIndexingHandler implements IndexingHandler { // Add search criterias if (criteria.getTargetIdList() != null) { - // Read permission is enough for searching - sb.append(" left join T_ACL a on a.ACL_TARGETID_C in (:targetIdList) and a.ACL_SOURCEID_C = d.DOC_ID_C and a.ACL_PERM_C = 'READ' and a.ACL_DELETEDATE_D is null "); - sb.append(" left join T_DOCUMENT_TAG dta on dta.DOT_IDDOCUMENT_C = d.DOC_ID_C and dta.DOT_DELETEDATE_D is null "); - sb.append(" left join T_ACL a2 on a2.ACL_TARGETID_C in (:targetIdList) and a2.ACL_SOURCEID_C = dta.DOT_IDTAG_C and a2.ACL_PERM_C = 'READ' and a2.ACL_DELETEDATE_D is null "); - criteriaList.add("(a.ACL_ID_C is not null or a2.ACL_ID_C is not null)"); + if (!SecurityUtil.skipAclCheck(criteria.getTargetIdList())) { + // Read permission is enough for searching + sb.append(" left join T_ACL a on a.ACL_TARGETID_C in (:targetIdList) and a.ACL_SOURCEID_C = d.DOC_ID_C and a.ACL_PERM_C = 'READ' and a.ACL_DELETEDATE_D is null "); + sb.append(" left join T_DOCUMENT_TAG dta on dta.DOT_IDDOCUMENT_C = d.DOC_ID_C and dta.DOT_DELETEDATE_D is null "); + sb.append(" left join T_ACL a2 on a2.ACL_TARGETID_C in (:targetIdList) and a2.ACL_SOURCEID_C = dta.DOT_IDTAG_C and a2.ACL_PERM_C = 'READ' and a2.ACL_DELETEDATE_D is null "); + criteriaList.add("(a.ACL_ID_C is not null or a2.ACL_ID_C is not null)"); + } parameterMap.put("targetIdList", criteria.getTargetIdList()); } if (!Strings.isNullOrEmpty(criteria.getSearch()) || !Strings.isNullOrEmpty(criteria.getFullSearch())) { diff --git a/docs-web/src/main/java/com/sismics/docs/rest/resource/DocumentResource.java b/docs-web/src/main/java/com/sismics/docs/rest/resource/DocumentResource.java index aa3da7b3..78798adc 100644 --- a/docs-web/src/main/java/com/sismics/docs/rest/resource/DocumentResource.java +++ b/docs-web/src/main/java/com/sismics/docs/rest/resource/DocumentResource.java @@ -377,7 +377,7 @@ public class DocumentResource extends BaseResource { } for (DocumentDto documentDto : paginatedList.getResultList()) { - // Get tags added by the current user on this document + // Get tags accessible by the current user on this document List tagDtoList = tagDao.findByCriteria(new TagCriteria() .setTargetIdList(getTargetIdList(null)) .setDocumentId(documentDto.getId()), new SortCriteria(1, true)); diff --git a/docs-web/src/main/java/com/sismics/docs/rest/resource/GroupResource.java b/docs-web/src/main/java/com/sismics/docs/rest/resource/GroupResource.java index 4fcb473e..6e9e7b58 100644 --- a/docs-web/src/main/java/com/sismics/docs/rest/resource/GroupResource.java +++ b/docs-web/src/main/java/com/sismics/docs/rest/resource/GroupResource.java @@ -1,7 +1,9 @@ package com.sismics.docs.rest.resource; import com.google.common.base.Strings; +import com.google.common.collect.Sets; import com.sismics.docs.core.dao.GroupDao; +import com.sismics.docs.core.dao.RoleBaseFunctionDao; import com.sismics.docs.core.dao.UserDao; import com.sismics.docs.core.dao.criteria.GroupCriteria; import com.sismics.docs.core.dao.criteria.UserCriteria; @@ -24,6 +26,7 @@ import javax.ws.rs.*; import javax.ws.rs.core.Response; import java.text.MessageFormat; import java.util.List; +import java.util.Set; /** * Group REST resources. @@ -185,6 +188,15 @@ public class GroupResource extends BaseResource { if (group == null) { throw new NotFoundException(); } + + // Ensure that the admin group is not deleted + if (group.getRoleId() != null) { + RoleBaseFunctionDao roleBaseFunctionDao = new RoleBaseFunctionDao(); + Set baseFunctionSet = roleBaseFunctionDao.findByRoleId(Sets.newHashSet(group.getRoleId())); + if (baseFunctionSet.contains(BaseFunction.ADMIN.name())) { + throw new ClientException("ForbiddenError", "The administrators group cannot be deleted"); + } + } // Delete the group groupDao.delete(group.getId(), principal.getId()); diff --git a/docs-web/src/main/java/com/sismics/docs/rest/resource/UserResource.java b/docs-web/src/main/java/com/sismics/docs/rest/resource/UserResource.java index 785f107b..ed0df960 100644 --- a/docs-web/src/main/java/com/sismics/docs/rest/resource/UserResource.java +++ b/docs-web/src/main/java/com/sismics/docs/rest/resource/UserResource.java @@ -445,7 +445,7 @@ public class UserResource extends BaseResource { throw new ForbiddenClientException(); } - // Ensure that the admin user is not deleted + // Ensure that the admin or guest users are not deleted if (hasBaseFunction(BaseFunction.ADMIN) || principal.isGuest()) { throw new ClientException("ForbiddenError", "This user cannot be deleted"); } @@ -519,8 +519,8 @@ public class UserResource extends BaseResource { } // Ensure that the admin user is not deleted - RoleBaseFunctionDao userBaseFuction = new RoleBaseFunctionDao(); - Set baseFunctionSet = userBaseFuction.findByRoleId(Sets.newHashSet(user.getRoleId())); + RoleBaseFunctionDao roleBaseFunctionDao = new RoleBaseFunctionDao(); + Set baseFunctionSet = roleBaseFunctionDao.findByRoleId(Sets.newHashSet(user.getRoleId())); if (baseFunctionSet.contains(BaseFunction.ADMIN.name())) { throw new ClientException("ForbiddenError", "The admin user cannot be deleted"); } diff --git a/docs-web/src/test/java/com/sismics/docs/rest/TestGroupResource.java b/docs-web/src/test/java/com/sismics/docs/rest/TestGroupResource.java index 1d5d37a9..785ea611 100644 --- a/docs-web/src/test/java/com/sismics/docs/rest/TestGroupResource.java +++ b/docs-web/src/test/java/com/sismics/docs/rest/TestGroupResource.java @@ -1,17 +1,16 @@ package com.sismics.docs.rest; -import java.util.ArrayList; -import java.util.List; +import com.sismics.util.filter.TokenBasedSecurityFilter; +import org.junit.Assert; +import org.junit.Test; import javax.json.JsonArray; import javax.json.JsonObject; import javax.ws.rs.client.Entity; import javax.ws.rs.core.Form; - -import org.junit.Assert; -import org.junit.Test; - -import com.sismics.util.filter.TokenBasedSecurityFilter; +import javax.ws.rs.core.Response; +import java.util.ArrayList; +import java.util.List; /** @@ -167,6 +166,15 @@ public class TestGroupResource extends BaseJerseyTest { target().path("/group/g1").request() .cookie(TokenBasedSecurityFilter.COOKIE_NAME, adminToken) .delete(JsonObject.class); + + // Delete group administrators + Response response = target().path("/group/administrators").request() + .cookie(TokenBasedSecurityFilter.COOKIE_NAME, adminToken) + .delete(); + Assert.assertEquals(Response.Status.BAD_REQUEST, Response.Status.fromStatusCode(response.getStatus())); + json = response.readEntity(JsonObject.class); + Assert.assertEquals("ForbiddenError", json.getString("type")); + Assert.assertEquals("The administrators group cannot be deleted", json.getString("message")); // Check group1 groups (all computed groups) json = target().path("/user").request() diff --git a/docs-web/src/test/java/com/sismics/docs/rest/TestRouteResource.java b/docs-web/src/test/java/com/sismics/docs/rest/TestRouteResource.java index a9bf279a..5eac656b 100644 --- a/docs-web/src/test/java/com/sismics/docs/rest/TestRouteResource.java +++ b/docs-web/src/test/java/com/sismics/docs/rest/TestRouteResource.java @@ -216,7 +216,7 @@ public class TestRouteResource extends BaseJerseyTest { .param("documentId", document1Id) .param("transition", "APPROVED")), JsonObject.class); Assert.assertFalse(json.containsKey("route_step")); - Assert.assertFalse(json.getBoolean("readable")); + Assert.assertTrue(json.getBoolean("readable")); // Admin can read everything Assert.assertTrue(popEmail().contains("workflow step")); // Get the route on document 1 @@ -239,10 +239,9 @@ public class TestRouteResource extends BaseJerseyTest { Assert.assertEquals("APPROVED", step.getString("transition")); // Get document 1 as admin - Response response = target().path("/document/" + document1Id).request() + target().path("/document/" + document1Id).request() .cookie(TokenBasedSecurityFilter.COOKIE_NAME, adminToken) - .get(); - Assert.assertEquals(Response.Status.NOT_FOUND, Response.Status.fromStatusCode(response.getStatus())); + .get(JsonObject.class); // Get document 1 as route1 json = target().path("/document/" + document1Id).request() @@ -265,7 +264,7 @@ public class TestRouteResource extends BaseJerseyTest { .cookie(TokenBasedSecurityFilter.COOKIE_NAME, adminToken) .get(JsonObject.class); documents = json.getJsonArray("documents"); - Assert.assertEquals(0, documents.size()); + Assert.assertEquals(1, documents.size()); // Admin can read all documents // Start the default route on document 1 target().path("/route/start").request() @@ -282,7 +281,7 @@ public class TestRouteResource extends BaseJerseyTest { Assert.assertTrue(json.containsKey("route_step")); // Get document 1 as admin - response = target().path("/document/" + document1Id).request() + Response response = target().path("/document/" + document1Id).request() .cookie(TokenBasedSecurityFilter.COOKIE_NAME, adminToken) .get(); Assert.assertEquals(Response.Status.OK, Response.Status.fromStatusCode(response.getStatus())); @@ -328,10 +327,9 @@ public class TestRouteResource extends BaseJerseyTest { Assert.assertFalse(json.containsKey("route_step")); // Get document 1 as admin - response = target().path("/document/" + document1Id).request() + target().path("/document/" + document1Id).request() .cookie(TokenBasedSecurityFilter.COOKIE_NAME, adminToken) - .get(); - Assert.assertEquals(Response.Status.NOT_FOUND, Response.Status.fromStatusCode(response.getStatus())); + .get(JsonObject.class); // Admin can see all documents // List all documents with route1 json = target().path("/document/list") @@ -348,7 +346,7 @@ public class TestRouteResource extends BaseJerseyTest { .cookie(TokenBasedSecurityFilter.COOKIE_NAME, adminToken) .get(JsonObject.class); documents = json.getJsonArray("documents"); - Assert.assertEquals(0, documents.size()); + Assert.assertEquals(1, documents.size()); } /** diff --git a/pom.xml b/pom.xml index 3525f792..daf7068d 100644 --- a/pom.xml +++ b/pom.xml @@ -33,7 +33,7 @@ 0.3m 5.5.0 4.2 - 2.0.8 + 2.0.12 1.54 2.9.2 5.1.0.Final