From 6407178b455e1ee0c535f720d9d843978f4ea2f8 Mon Sep 17 00:00:00 2001 From: jensp Date: Wed, 5 Aug 2015 11:46:01 +0000 Subject: [PATCH] CCM NG: PermissionManager finished git-svn-id: https://svn.libreccm.org/ccm/ccm_ng@3548 8810af33-2d31-482b-a856-94f89814c4df --- .../org/libreccm/core/PermissionManager.java | 137 ++++++++++++++---- .../java/org/libreccm/core/Privilege.java | 3 + .../libreccm/core/PermissionManagerTest.java | 73 ++++++++-- .../after-grant-wildcard.json | 4 +- .../PermissionManagerTest/after-grant.json | 36 ++++- .../PermissionManagerTest/after-revoke.json | 26 +--- .../core/PermissionManagerTest/data.json | 4 +- 7 files changed, 202 insertions(+), 81 deletions(-) diff --git a/ccm-core/src/main/java/org/libreccm/core/PermissionManager.java b/ccm-core/src/main/java/org/libreccm/core/PermissionManager.java index 176895adc..64f311cde 100644 --- a/ccm-core/src/main/java/org/libreccm/core/PermissionManager.java +++ b/ccm-core/src/main/java/org/libreccm/core/PermissionManager.java @@ -31,17 +31,23 @@ import javax.inject.Inject; public class PermissionManager { @Inject + @SuppressWarnings("PMD.LongVariable") //Nothing wrong with this name private transient PermissionRepository permissionRepository; @Inject + @SuppressWarnings("PMD.LongVariable") //Nothing wrong with this name private transient PrivilegeRepository privilegeRepository; @Inject + @SuppressWarnings("PMD.LongVariable") //Nothing wrong with this name private transient CcmObjectRepository ccmObjectRepository; @Inject private transient SubjectRepository subjectRepository; + @Inject + private transient UserRepository userRepository; + /** * Creates a new permission granting the provided {@code privilege} on the * provided {@code object} to the provided {@code subject}. If the @@ -57,6 +63,16 @@ public class PermissionManager { public void grantPermission(final Privilege privilege, final CcmObject object, final Subject subject) { + if (privilege == null) { + throw new IllegalArgumentException( + "Illegal argument 'null' provided for parameter privilege."); + } + + if (subject == null) { + throw new IllegalArgumentException( + "Illegal argument 'null' provided for parameter subject"); + } + if (!isPermitted(privilege, object, subject)) { final Permission permission = new Permission(); permission.setGrantedPrivilege(privilege); @@ -110,6 +126,11 @@ public class PermissionManager { * Checks if the the provided {@code subject} has a permission granting the * provided {@code privilege} on the provided {@code object}. * + * The method will also check if the subject has a permission granting the + * {@code admin} privilege on the provided object or on all objects. The + * {@code admin} privilege implies that the user is granted all other + * privileges on the object. + * * If the provided subject is {@code null} the method will try to retrieve * the public user from the database. If there is no public user the method * will return {@code false}. @@ -128,52 +149,104 @@ public class PermissionManager { public boolean isPermitted(final Privilege privilege, final CcmObject object, final Subject subject) { - if (subject instanceof User) { - return isPermitted(privilege, object, (User) subject); - } else if (subject instanceof Group) { - return isPermitted(privilege, object, (Group) subject); + if (privilege == null) { + throw new IllegalArgumentException( + "Illegal value 'null' provided for parameter privilege"); + } + + //Check if the current subject is null. If yes try to retrieve the + //public user + Subject subjectObj = null; + if (subject == null) { + final User publicUser = userRepository.retrievePublicUser(); + + if (publicUser == null) { + + //If the public user is not available an null value for the + //subject parameter is an illegal argument. + throw new IllegalArgumentException( + "Illegal value 'null' provided for parameter privilege"); + } else { + subjectObj = publicUser; + } } else { + //Subject is not null. Use provided subject + subjectObj = subject; + } + + //Depending if the subject is a user or a group delegate to the correct + //method. For users we have to check the permissions of the groups the + //user is member of also, for group we don't have. + if (subjectObj instanceof User) { + return isPermitted(privilege, object, (User) subjectObj); + } else if (subjectObj instanceof Group) { + return isPermitted(privilege, object, (Group) subjectObj); + } else { + //For unknown subclasses of subject return false. return false; } } - public boolean isPermitted(final Privilege privilege, - final CcmObject object, - final User user) { - boolean result; + /** + * Checks if a user has a permission granting a privilege on an object. If + * the provided {@code object} is {@code null} the method will only check + * for wildcard permission (permissions for all objects). + * + * @param privilege The privilege. Can't be null. + * @param object The object. Can be null. + * @param user The user. Can't be null. + * + * @return {@code true} if the provided {@code user} has a permission + * granting the provided privilege for the provided object, + * {@code false} if not. + */ + @SuppressWarnings("PMD.CyclomaticComplexity") + private boolean isPermitted(final Privilege privilege, + final CcmObject object, + final User user) { + boolean result = false; + final Privilege admin = privilegeRepository.retrievePrivilege( + Privilege.ADMIN); - final List directPermissions = permissionRepository - .findPermissionsForUserPrivilegeAndObject(user, privilege, object); - result = !directPermissions.isEmpty(); + //Check if the there is direct permission granting the provided + //privilege on the provided object. If the object is null or the + //privilege is the admin privilege this check is not performed. + if (object != null && !privilege.equals(admin)) { + final List permissions = permissionRepository + .findPermissionsForUserPrivilegeAndObject(user, privilege, + object); + result = !permissions.isEmpty(); + } - if (!result) { + //Check for an wildcard permission (a permission for all objects) for + //the provided privilege. This check is only performed when result is + //still false the provided privilege is not the admin privilege. + if (!result && !privilege.equals(admin)) { final List permissions = permissionRepository .findPermissionsForUserPrivilegeAndObject(user, privilege, null); result = !permissions.isEmpty(); } - if (!result) { - final Privilege admin = privilegeRepository.retrievePrivilege( - "admin"); - if (admin != null) { - final List permissions = permissionRepository - .findPermissionsForUserPrivilegeAndObject(user, - privilege, - object); - result = !permissions.isEmpty(); - } + //Check for a permission granting the admin privilege on the provided + //object. This check is only performed if result is still false, + //the admin variable is not null (null means that there is no admin + //privilege) and the provided object is not null. + if (!result && admin != null && object != null) { + final List permissions = permissionRepository + .findPermissionsForUserPrivilegeAndObject(user, + admin, + object); + result = !permissions.isEmpty(); } - if (!result) { - final Privilege admin = privilegeRepository.retrievePrivilege( - "admin"); - if (admin != null) { - final List permissions = permissionRepository - .findPermissionsForUserPrivilegeAndObject(user, - privilege, - null); - result = !permissions.isEmpty(); - } + //Check for a permission granting the admin privilege systemwide. This + //check in only performed if result is still false and admin is not null. + if (!result && admin != null) { + final List permissions = permissionRepository + .findPermissionsForUserPrivilegeAndObject(user, + admin, + null); + result = !permissions.isEmpty(); } return result; diff --git a/ccm-core/src/main/java/org/libreccm/core/Privilege.java b/ccm-core/src/main/java/org/libreccm/core/Privilege.java index f02e22953..8919bc5e7 100644 --- a/ccm-core/src/main/java/org/libreccm/core/Privilege.java +++ b/ccm-core/src/main/java/org/libreccm/core/Privilege.java @@ -62,6 +62,9 @@ import javax.xml.bind.annotation.XmlRootElement; public class Privilege implements Serializable { private static final long serialVersionUID = -3986038536996049440L; + + //Constant for the admin privilege. + public static final String ADMIN = "admin"; @Id @Column(name = "privilege_id") diff --git a/ccm-core/src/test/java/org/libreccm/core/PermissionManagerTest.java b/ccm-core/src/test/java/org/libreccm/core/PermissionManagerTest.java index 05e541b5f..88af98c14 100644 --- a/ccm-core/src/test/java/org/libreccm/core/PermissionManagerTest.java +++ b/ccm-core/src/test/java/org/libreccm/core/PermissionManagerTest.java @@ -208,7 +208,7 @@ public class PermissionManagerTest { @UsingDataSet("datasets/org/libreccm/core/PermissionManagerTest/" + "data.json") @InSequence(10) - public void isPermittedWebmaster() { + public void isPermittedWebmasterAdmin() { final User webmaster = userRepository.findByScreenName("webmaster"); final Map testObjects = retrieveTestObjects(); final Map privileges = retrievePrivileges(); @@ -224,10 +224,55 @@ public class PermissionManagerTest { expected.put(testObjects.get(TEST_OBJECT_8), true); verifyIsPermitted(webmaster, privileges.get(ADMIN), expected); + } + + @Test + @UsingDataSet("datasets/org/libreccm/core/PermissionManagerTest/" + + "data.json") + @InSequence(11) + public void isPermittedWebmasterRead() { + final User webmaster = userRepository.findByScreenName("webmaster"); + final Map testObjects = retrieveTestObjects(); + final Map privileges = retrievePrivileges(); + + final Map expected = new LinkedHashMap<>(); + expected.put(testObjects.get(TEST_OBJECT_1), true); + expected.put(testObjects.get(TEST_OBJECT_2), true); + expected.put(testObjects.get(TEST_OBJECT_3), true); + expected.put(testObjects.get(TEST_OBJECT_4), true); + expected.put(testObjects.get(TEST_OBJECT_5), true); + expected.put(testObjects.get(TEST_OBJECT_6), true); + expected.put(testObjects.get(TEST_OBJECT_7), true); + expected.put(testObjects.get(TEST_OBJECT_8), true); + verifyIsPermitted(webmaster, privileges.get(READ), expected); + } + + @Test + @UsingDataSet("datasets/org/libreccm/core/PermissionManagerTest/" + + "data.json") + @InSequence(12) + public void isPermittedWebmasterWrite() { + final User webmaster = userRepository.findByScreenName("webmaster"); + final Map testObjects = retrieveTestObjects(); + final Map privileges = retrievePrivileges(); + + final Map expected = new LinkedHashMap<>(); + expected.put(testObjects.get(TEST_OBJECT_1), true); + expected.put(testObjects.get(TEST_OBJECT_2), true); + expected.put(testObjects.get(TEST_OBJECT_3), true); + expected.put(testObjects.get(TEST_OBJECT_4), true); + expected.put(testObjects.get(TEST_OBJECT_5), true); + expected.put(testObjects.get(TEST_OBJECT_6), true); + expected.put(testObjects.get(TEST_OBJECT_7), true); + expected.put(testObjects.get(TEST_OBJECT_8), true); + verifyIsPermitted(webmaster, privileges.get(WRITE), expected); } + + + @Test @UsingDataSet("datasets/org/libreccm/core/PermissionManagerTest/" + "data.json") @@ -405,17 +450,16 @@ public class PermissionManagerTest { permissionManager.isPermitted(null, object, user); } - @Test(expected = IllegalArgumentException.class) + @Test @UsingDataSet("datasets/org/libreccm/core/PermissionManagerTest/" + "data.json") - @ShouldThrowException(IllegalArgumentException.class) @InSequence(80) public void isPermittedNullObject() { final Privilege privilege = privilegeRepository .retrievePrivilege(READ); final User user = userRepository.findByScreenName("webmaster"); - permissionManager.isPermitted(privilege, null, user); + assertThat(permissionManager.isPermitted(privilege, null, user), is(true)); } @Test @@ -458,10 +502,9 @@ public class PermissionManagerTest { permissionManager.checkPermission(null, object, user); } - @Test(expected = IllegalArgumentException.class) + @Test @UsingDataSet("datasets/org/libreccm/core/PermissionManagerTest/" + "data.json") - @ShouldThrowException(IllegalArgumentException.class) @InSequence(130) public void checkPermissionNullObject() throws UnauthorizedAcccessException { final Privilege privilege = privilegeRepository @@ -471,10 +514,9 @@ public class PermissionManagerTest { permissionManager.checkPermission(privilege, null, user); } - @Test(expected = IllegalArgumentException.class) + @Test @UsingDataSet("datasets/org/libreccm/core/PermissionManagerTest/" + "data.json") - @ShouldThrowException(IllegalArgumentException.class) @InSequence(140) public void checkPermissionNullSubject() throws UnauthorizedAcccessException { final Privilege privilege = privilegeRepository @@ -487,8 +529,9 @@ public class PermissionManagerTest { @Test @UsingDataSet("datasets/org/libreccm/core/PermissionManagerTest/" + "data.json") - @ShouldMatchDataSet("datasets/org/libreccm/core/PermissionManagerTest/" - + "after-grant.json") + @ShouldMatchDataSet(value = "datasets/org/libreccm/core/" + + "PermissionManagerTest/after-grant.json", + excludeColumns = {"permission_id"}) @InSequence(150) public void grantPermission() { final Privilege read = privilegeRepository.retrievePrivilege(READ); @@ -509,8 +552,9 @@ public class PermissionManagerTest { @Test @UsingDataSet("datasets/org/libreccm/core/PermissionManagerTest/" + "data.json") - @ShouldMatchDataSet("datasets/org/libreccm/core/PermissionManagerTest/" - + "after-grant-wildcard.json") + @ShouldMatchDataSet(value = "datasets/org/libreccm/core/" + + "PermissionManagerTest/after-grant-wildcard.json", + excludeColumns = {"permission_id"}) @InSequence(160) public void grantWildcardPermission() { final Privilege read = privilegeRepository.retrievePrivilege(READ); @@ -546,8 +590,9 @@ public class PermissionManagerTest { @Test @UsingDataSet("datasets/org/libreccm/core/PermissionManagerTest/" + "data.json") - @ShouldMatchDataSet("datasets/org/libreccm/core/PermissionManagerTest/" - + "after-revoke.json") + @ShouldMatchDataSet(value = "datasets/org/libreccm/core/" + + "PermissionManagerTest/after-revoke.json", + excludeColumns = {"permission_id"}) @InSequence(190) public void revokePermission() { final Privilege read = privilegeRepository.retrievePrivilege(READ); diff --git a/ccm-core/src/test/resources/datasets/org/libreccm/core/PermissionManagerTest/after-grant-wildcard.json b/ccm-core/src/test/resources/datasets/org/libreccm/core/PermissionManagerTest/after-grant-wildcard.json index b2066762f..e60f483b6 100644 --- a/ccm-core/src/test/resources/datasets/org/libreccm/core/PermissionManagerTest/after-grant-wildcard.json +++ b/ccm-core/src/test/resources/datasets/org/libreccm/core/PermissionManagerTest/after-grant-wildcard.json @@ -301,13 +301,13 @@ { "permission_id": -250, "grantee_id": -50, - "object_id": -70, + "object_id": -60, "granted_privilege_id": -20 }, { "permission_id": -260, "grantee_id": -50, - "object_id": -70, + "object_id": -60, "granted_privilege_id": -30 }, { diff --git a/ccm-core/src/test/resources/datasets/org/libreccm/core/PermissionManagerTest/after-grant.json b/ccm-core/src/test/resources/datasets/org/libreccm/core/PermissionManagerTest/after-grant.json index fc22ec0e8..12796ee11 100644 --- a/ccm-core/src/test/resources/datasets/org/libreccm/core/PermissionManagerTest/after-grant.json +++ b/ccm-core/src/test/resources/datasets/org/libreccm/core/PermissionManagerTest/after-grant.json @@ -208,12 +208,6 @@ "object_id": -60, "granted_privilege_id": -20 }, - { - "permission_id": -70, - "grantee_id": -50, - "object_id": -60, - "granted_privilege_id": -30 - }, { "permission_id": -80, "grantee_id": -30, @@ -303,6 +297,36 @@ "grantee_id": -40, "object_id": -80, "granted_privilege_id": -30 + }, + { + "permission_id": -250, + "grantee_id": -50, + "object_id": -60, + "granted_privilege_id": -20 + }, + { + "permission_id": -260, + "grantee_id": -50, + "object_id": -60, + "granted_privilege_id": -30 + }, + { + "permission_id": -270, + "grantee_id": -10, + "object_id": -60, + "granted_privilege_id": -20 + }, + { + "permission_id": -280, + "grantee_id": -50, + "object_id": -70, + "granted_privilege_id": -20 + }, + { + "permission_id": -290, + "grantee_id": -50, + "object_id": -70, + "granted_privilege_id": -30 } ] } \ No newline at end of file diff --git a/ccm-core/src/test/resources/datasets/org/libreccm/core/PermissionManagerTest/after-revoke.json b/ccm-core/src/test/resources/datasets/org/libreccm/core/PermissionManagerTest/after-revoke.json index 1a81e26df..a940fd187 100644 --- a/ccm-core/src/test/resources/datasets/org/libreccm/core/PermissionManagerTest/after-revoke.json +++ b/ccm-core/src/test/resources/datasets/org/libreccm/core/PermissionManagerTest/after-revoke.json @@ -184,12 +184,6 @@ "object_id": -50, "granted_privilege_id": -20 }, - { - "permission_id": -30, - "grantee_id": -10, - "object_id": -50, - "granted_privilege_id": -20 - }, { "permission_id": -40, "grantee_id": -50, @@ -208,12 +202,6 @@ "object_id": -60, "granted_privilege_id": -20 }, - { - "permission_id": -70, - "grantee_id": -50, - "object_id": -60, - "granted_privilege_id": -30 - }, { "permission_id": -80, "grantee_id": -30, @@ -304,23 +292,11 @@ "object_id": -80, "granted_privilege_id": -30 }, - { - "permission_id": -240, - "grantee_id": -10, - "object_id": -60, - "granted_privilege_id": -20 - }, { "permission_id": -250, "grantee_id": -50, - "object_id": -70, + "object_id": -60, "granted_privilege_id": -20 - }, - { - "permission_id": -260, - "grantee_id": -50, - "object_id": -70, - "granted_privilege_id": -30 } ] } \ No newline at end of file diff --git a/ccm-core/src/test/resources/datasets/org/libreccm/core/PermissionManagerTest/data.json b/ccm-core/src/test/resources/datasets/org/libreccm/core/PermissionManagerTest/data.json index 76d9afe57..89b5b9edb 100644 --- a/ccm-core/src/test/resources/datasets/org/libreccm/core/PermissionManagerTest/data.json +++ b/ccm-core/src/test/resources/datasets/org/libreccm/core/PermissionManagerTest/data.json @@ -301,13 +301,13 @@ { "permission_id": -250, "grantee_id": -50, - "object_id": -70, + "object_id": -60, "granted_privilege_id": -20 }, { "permission_id": -260, "grantee_id": -50, - "object_id": -70, + "object_id": -60, "granted_privilege_id": -30 } ]