From 0dbf5f51381647b630b8422c5c19d8ca0183366d Mon Sep 17 00:00:00 2001 From: Sanjay Dalal Date: Fri, 14 May 2010 17:27:22 +0000 Subject: [PATCH] NOJIRA incorporate some code review changes. started refactoring seed test in order to convert it to generator and import for all services M services/authorization-mgt/client/src/test/java/org/collectionspace/services/authorization/test/AuthorizationSeedTest.java M services/authorization-mgt/client/src/test/java/org/collectionspace/services/authorization/test/AuthorizationGen.java M services/authorization-mgt/client/src/test/resources/test-data/test-permissions-roles.xml M services/authorization/service/src/main/java/org/collectionspace/services/authorization/spring/SpringPermissionManager.java M services/authorization/service/src/main/java/org/collectionspace/services/authorization/spring/SpringAuthorizationProvider.java M services/authorization/service/src/main/java/org/collectionspace/services/authorization/spring/SpringPermissionEvaluator.java --- .../authorization/test/AuthorizationGen.java | 82 ++++++++----------- .../test/AuthorizationSeedTest.java | 27 +++--- .../test-data/test-permissions-roles.xml | 8 -- .../spring/SpringAuthorizationProvider.java | 2 +- .../spring/SpringPermissionEvaluator.java | 2 +- .../spring/SpringPermissionManager.java | 41 +++++----- 6 files changed, 70 insertions(+), 92 deletions(-) diff --git a/services/authorization-mgt/client/src/test/java/org/collectionspace/services/authorization/test/AuthorizationGen.java b/services/authorization-mgt/client/src/test/java/org/collectionspace/services/authorization/test/AuthorizationGen.java index 6cf2bd0fc..d1b2acfed 100644 --- a/services/authorization-mgt/client/src/test/java/org/collectionspace/services/authorization/test/AuthorizationGen.java +++ b/services/authorization-mgt/client/src/test/java/org/collectionspace/services/authorization/test/AuthorizationGen.java @@ -15,28 +15,6 @@ * https://source.collectionspace.org/collection-space/LICENSE.txt - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - *//** - * This document is a part of the source code and related artifacts - * for CollectionSpace, an open source collections management system - * for museums and related institutions: - - * http://www.collectionspace.org - * http://wiki.collectionspace.org - - * Copyright 2009 University of California at Berkeley - - * Licensed under the Educational Community License (ECL), Version 2.0. - * You may not use this file except in compliance with this License. - - * You may obtain a copy of the ECL 2.0 License at - - * https://source.collectionspace.org/collection-space/LICENSE.txt - * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. @@ -49,6 +27,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import java.util.ArrayList; import java.util.List; +import java.util.UUID; import org.collectionspace.services.authorization.ActionType; import org.collectionspace.services.authorization.Permission; import org.collectionspace.services.authorization.EffectType; @@ -67,29 +46,35 @@ import org.collectionspace.services.authorization.SubjectType; public class AuthorizationGen { final Logger logger = LoggerFactory.getLogger(AuthorizationGen.class); + private PermissionsList pcList = new PermissionsList(); + PermissionsRolesList psrsl = new PermissionsRolesList(); - public void genPermissions() { - PermissionsList pcList = new PermissionsList(); + public PermissionsList genPermissions() { ArrayList apcList = new ArrayList(); pcList.setPermissions(apcList); - Permission accPerm = buildCommonPermission("1", "accounts"); + Permission accPerm = buildCommonPermission("1", "1", "accounts"); apcList.add(accPerm); - Permission dimPerm = buildCommonPermission("2", "dimensions"); + Permission dimPerm = buildCommonPermission("1", "2", "dimensions"); apcList.add(dimPerm); + return pcList; + + } + + public void writePermissions(PermissionsList pcList, String fileName) { AbstractAuthorizationTestImpl.toFile(pcList, PermissionsList.class, - AbstractAuthorizationTestImpl.testDataDir + "test-permissions.xml"); + AbstractAuthorizationTestImpl.testDataDir + fileName); logger.info("generated permissions to " - + AbstractAuthorizationTestImpl.testDataDir + "test-permissions.xml"); - + + AbstractAuthorizationTestImpl.testDataDir + fileName); } - private Permission buildCommonPermission(String id, String resourceName) { + private Permission buildCommonPermission(String tenantId, String permId, String resourceName) { + //String id = UUID.randomUUID().toString(); Permission perm = new Permission(); - perm.setCsid(id); + perm.setCsid(permId); perm.setResourceName(resourceName); perm.setEffect(EffectType.PERMIT); - perm.setTenantId("1"); + perm.setTenantId(tenantId); ArrayList pas = new ArrayList(); perm.setActions(pas); @@ -108,39 +93,38 @@ public class AuthorizationGen { return perm; } - public void genPermissionsRoles() { - PermissionsRolesList psrsl = new PermissionsRolesList(); + public PermissionsRolesList genPermissionsRoles(PermissionsList pcList) { ArrayList prl = new ArrayList(); - prl.add(buildCommonPermissionRoles("1", "accounts")); - prl.add(buildCommonPermissionRoles("2", "dimensions")); + prl.add(buildCommonPermissionRoles("1", "1", "accounts")); + prl.add(buildCommonPermissionRoles("1", "2", "dimensions")); psrsl.setPermissionRoles(prl); + return psrsl; + } + + public void writePermissionRoles(PermissionsRolesList psrsl, String fileName) { AbstractAuthorizationTestImpl.toFile(psrsl, PermissionsRolesList.class, - AbstractAuthorizationTestImpl.testDataDir + "test-permissions-roles.xml"); + AbstractAuthorizationTestImpl.testDataDir + fileName); logger.info("generated permissions-roles to " - + AbstractAuthorizationTestImpl.testDataDir + "test-permissions-roles.xml"); + + AbstractAuthorizationTestImpl.testDataDir + fileName); } - private PermissionRole buildCommonPermissionRoles(String id, String resName) { + private PermissionRole buildCommonPermissionRoles(String tenantId, String permissionId, + String resName) { PermissionRole pr = new PermissionRole(); pr.setSubject(SubjectType.ROLE); - List permValues = new ArrayList(); pr.setPermissions(permValues); PermissionValue permValue = new PermissionValue(); - permValue.setPermissionId(id); + permValue.setPermissionId(permissionId); permValue.setResourceName(resName); permValues.add(permValue); List roleValues = new ArrayList(); - RoleValue rv1 = new RoleValue(); - rv1.setRoleName("ROLE_USERS"); - rv1.setRoleId("1"); - roleValues.add(rv1); - RoleValue rv2 = new RoleValue(); - rv2.setRoleName("ROLE_ADMINISTRATOR"); - rv2.setRoleId("2"); - roleValues.add(rv2); + RoleValue radmin = new RoleValue(); + radmin.setRoleName("ROLE_ADMINISTRATOR"); + radmin.setRoleId(tenantId); + roleValues.add(radmin); pr.setRoles(roleValues); return pr; diff --git a/services/authorization-mgt/client/src/test/java/org/collectionspace/services/authorization/test/AuthorizationSeedTest.java b/services/authorization-mgt/client/src/test/java/org/collectionspace/services/authorization/test/AuthorizationSeedTest.java index ea650f014..c22344991 100644 --- a/services/authorization-mgt/client/src/test/java/org/collectionspace/services/authorization/test/AuthorizationSeedTest.java +++ b/services/authorization-mgt/client/src/test/java/org/collectionspace/services/authorization/test/AuthorizationSeedTest.java @@ -51,6 +51,8 @@ import org.testng.annotations.BeforeClass; public class AuthorizationSeedTest extends AbstractAuthorizationTestImpl { final Logger logger = LoggerFactory.getLogger(AuthorizationSeedTest.class); + final static String PERMISSION_FILE = "test-permissions.xml"; + final static String PERMISSION_ROLE_FILE = "test-permissions-roles.xml"; @BeforeClass(alwaysRun = true) public void seedData() { @@ -58,8 +60,10 @@ public class AuthorizationSeedTest extends AbstractAuthorizationTestImpl { TransactionStatus status = beginTransaction("seedData"); try { AuthorizationGen authzGen = new AuthorizationGen(); - authzGen.genPermissions(); - authzGen.genPermissionsRoles(); + PermissionsList pl = authzGen.genPermissions(); + authzGen.writePermissions(pl, PERMISSION_FILE); + PermissionsRolesList prl = authzGen.genPermissionsRoles(pl); + authzGen.writePermissionRoles(prl, PERMISSION_ROLE_FILE); seedRoles(); seedPermissions(); } catch (Exception ex) { @@ -71,20 +75,20 @@ public class AuthorizationSeedTest extends AbstractAuthorizationTestImpl { } public void seedRoles() throws Exception { - //Should this test really be empty? + //Should this test really be empty? } - public void seedPermissions() throws Exception { + public void seedPermissions() throws Exception { PermissionsList pcList = - (PermissionsList) fromFile(PermissionsList.class, baseDir + - AbstractAuthorizationTestImpl.testDataDir + "test-permissions.xml"); + (PermissionsList) fromFile(PermissionsList.class, baseDir + + AbstractAuthorizationTestImpl.testDataDir + PERMISSION_FILE); logger.info("read permissions from " - + baseDir + AbstractAuthorizationTestImpl.testDataDir + "test-permissions.xml"); + + baseDir + AbstractAuthorizationTestImpl.testDataDir + PERMISSION_FILE); PermissionsRolesList pcrList = - (PermissionsRolesList) fromFile(PermissionsRolesList.class, baseDir + - AbstractAuthorizationTestImpl.testDataDir + "test-permissions-roles.xml"); + (PermissionsRolesList) fromFile(PermissionsRolesList.class, baseDir + + AbstractAuthorizationTestImpl.testDataDir + PERMISSION_ROLE_FILE); logger.info("read permissions-roles from " - + baseDir + AbstractAuthorizationTestImpl.testDataDir + "test-permissions.xml"); + + baseDir + AbstractAuthorizationTestImpl.testDataDir + PERMISSION_ROLE_FILE); AuthZ authZ = AuthZ.get(); for (Permission p : pcList.getPermissions()) { if (logger.isDebugEnabled()) { @@ -98,7 +102,7 @@ public class AuthorizationSeedTest extends AbstractAuthorizationTestImpl { } } - /** + /** * addPermissionsForUri add permissions from given permission configuration * with assumption that resource is of type URI * @param permission configuration @@ -125,7 +129,6 @@ public class AuthorizationSeedTest extends AbstractAuthorizationTestImpl { } } - /** * getAction is a convenience method to get corresponding action for * given ActionType diff --git a/services/authorization-mgt/client/src/test/resources/test-data/test-permissions-roles.xml b/services/authorization-mgt/client/src/test/resources/test-data/test-permissions-roles.xml index ea59f1115..ef35f5a0f 100644 --- a/services/authorization-mgt/client/src/test/resources/test-data/test-permissions-roles.xml +++ b/services/authorization-mgt/client/src/test/resources/test-data/test-permissions-roles.xml @@ -8,10 +8,6 @@ 1 - ROLE_USERS - - - 2 ROLE_ADMINISTRATOR @@ -23,10 +19,6 @@ 1 - ROLE_USERS - - - 2 ROLE_ADMINISTRATOR diff --git a/services/authorization/service/src/main/java/org/collectionspace/services/authorization/spring/SpringAuthorizationProvider.java b/services/authorization/service/src/main/java/org/collectionspace/services/authorization/spring/SpringAuthorizationProvider.java index 3bf90ebb3..be2c8afac 100644 --- a/services/authorization/service/src/main/java/org/collectionspace/services/authorization/spring/SpringAuthorizationProvider.java +++ b/services/authorization/service/src/main/java/org/collectionspace/services/authorization/spring/SpringAuthorizationProvider.java @@ -122,7 +122,7 @@ public class SpringAuthorizationProvider implements CSpaceAuthorizationProvider return sids.toArray(new Sid[0]); } - static Permission mapPermission(CSpaceAction perm) { + static Permission mapAction(CSpaceAction perm) { switch (perm) { case ADMIN: return BasePermission.ADMINISTRATION; diff --git a/services/authorization/service/src/main/java/org/collectionspace/services/authorization/spring/SpringPermissionEvaluator.java b/services/authorization/service/src/main/java/org/collectionspace/services/authorization/spring/SpringPermissionEvaluator.java index 18b6f40d8..3f238693d 100644 --- a/services/authorization/service/src/main/java/org/collectionspace/services/authorization/spring/SpringPermissionEvaluator.java +++ b/services/authorization/service/src/main/java/org/collectionspace/services/authorization/spring/SpringPermissionEvaluator.java @@ -50,7 +50,7 @@ public class SpringPermissionEvaluator implements CSpacePermissionEvaluator { @Override public boolean hasPermission(CSpaceResource res, CSpaceAction perm) { PermissionEvaluator eval = provider.getProviderPermissionEvaluator(); - Permission p = SpringAuthorizationProvider.mapPermission(perm); + Permission p = SpringAuthorizationProvider.mapAction(perm); Authentication authToken = SecurityContextHolder.getContext().getAuthentication(); return eval.hasPermission(authToken, Long.valueOf(res.getId().hashCode()), diff --git a/services/authorization/service/src/main/java/org/collectionspace/services/authorization/spring/SpringPermissionManager.java b/services/authorization/service/src/main/java/org/collectionspace/services/authorization/spring/SpringPermissionManager.java index 8b942352b..aa1462a82 100644 --- a/services/authorization/service/src/main/java/org/collectionspace/services/authorization/spring/SpringPermissionManager.java +++ b/services/authorization/service/src/main/java/org/collectionspace/services/authorization/spring/SpringPermissionManager.java @@ -61,7 +61,7 @@ public class SpringPermissionManager implements CSpacePermissionManager { throws PermissionException { ObjectIdentity oid = SpringAuthorizationProvider.mapResource(res); Sid[] sids = SpringAuthorizationProvider.mapPrincipal(principals); - Permission p = SpringAuthorizationProvider.mapPermission(action); + Permission p = SpringAuthorizationProvider.mapAction(action); TransactionStatus status = provider.beginTransaction("addPermssions"); //add permission for each sid @@ -121,7 +121,7 @@ public class SpringPermissionManager implements CSpacePermissionManager { throws PermissionNotFoundException, PermissionException { ObjectIdentity oid = SpringAuthorizationProvider.mapResource(res); Sid[] sids = SpringAuthorizationProvider.mapPrincipal(principals); - Permission p = SpringAuthorizationProvider.mapPermission(action); + Permission p = SpringAuthorizationProvider.mapAction(action); TransactionStatus status = provider.beginTransaction("deletePermssions"); //delete permission for each sid for (Sid sid : sids) { @@ -178,10 +178,18 @@ public class SpringPermissionManager implements CSpacePermissionManager { public void deletePermissions(CSpaceResource res, CSpaceAction action) throws PermissionNotFoundException, PermissionException { ObjectIdentity oid = SpringAuthorizationProvider.mapResource(res); - Permission p = SpringAuthorizationProvider.mapPermission(action); + Permission p = SpringAuthorizationProvider.mapAction(action); TransactionStatus status = provider.beginTransaction("deletePermssions"); try { deletePermissions(oid, p, null); + provider.commitTransaction(status); + if (log.isDebugEnabled()) { + log.debug("deletepermissions(res,action) success, " + + " res=" + res.toString() + + " action=" + action.toString() + + " oid=" + oid.toString() + + " perm=" + p.toString()); + } } catch (AclDataAccessException aex) { provider.rollbackTransaction(status); log.debug("deletepermissions(res,action) failed," @@ -207,14 +215,6 @@ public class SpringPermissionManager implements CSpacePermissionManager { } throw new PermissionException(msg, ex); } - provider.commitTransaction(status); - if (log.isDebugEnabled()) { - log.debug("deletepermissions(res,action) success, " - + " res=" + res.toString() - + " action=" + action.toString() - + " oid=" + oid.toString() - + " perm=" + p.toString()); - } } @@ -222,9 +222,15 @@ public class SpringPermissionManager implements CSpacePermissionManager { public void deletePermissions(CSpaceResource res) throws PermissionNotFoundException, PermissionException { ObjectIdentity oid = SpringAuthorizationProvider.mapResource(res); - TransactionStatus status = provider.beginTransaction("addPermssion"); + TransactionStatus status = provider.beginTransaction("deletePermssion"); try { provider.getProviderAclService().deleteAcl(oid, true); + provider.commitTransaction(status); + if (log.isDebugEnabled()) { + log.debug("deletepermissions(res) success, " + + " res=" + res.toString() + + " oid=" + oid.toString()); + } } catch (AclDataAccessException aex) { provider.rollbackTransaction(status); log.debug("deletepermissions(res) failed," @@ -246,13 +252,6 @@ public class SpringPermissionManager implements CSpacePermissionManager { } throw new PermissionException(msg, ex); } - provider.commitTransaction(status); - - if (log.isDebugEnabled()) { - log.debug("deletepermissions(res) success, " - + " res=" + res.toString() - + " oid=" + oid.toString()); - } } private void addPermission(ObjectIdentity oid, Permission permission, @@ -293,8 +292,8 @@ public class SpringPermissionManager implements CSpacePermissionManager { ArrayList foundAces = new ArrayList(); Iterator iter = acel.listIterator(); //not possible to delete while iterating - while(iter.hasNext()) { - AccessControlEntry ace = (AccessControlEntry)iter.next(); + while (iter.hasNext()) { + AccessControlEntry ace = (AccessControlEntry) iter.next(); if (sid != null) { if (ace.getSid().equals(sid) && ace.getPermission().equals(permission)) { -- 2.47.3