Index: grails-app/taglib/org/weceem/tags/EditorFieldTagLib.groovy =================================================================== --- grails-app/taglib/org/weceem/tags/EditorFieldTagLib.groovy (revision 5) +++ grails-app/taglib/org/weceem/tags/EditorFieldTagLib.groovy (revision 6) @@ -11,6 +11,7 @@ static namespace = "wcm" TagLibraryLookup gspTagLibraryLookup // Tag lookup bean from Grails injection + def weceemSecurityService // Security service injected by grails def editorFieldString = { attrs -> out << bean.input(beanName:'content', property:attrs.property, noLabel:true) @@ -47,6 +48,9 @@ def editorFieldStatus = { attrs -> def statuses = Status.listOrderByCode() + statuses = statuses.findAll{ + weceemSecurityService.isUserAllowedContentStatus(attrs.bean.space, attrs.bean.status, it) + } out << bean.select(beanName:'content', property:attrs.property, noLabel:true, from: statuses, optionValue: { v -> g.message(code:'content.status.'+v.description) }, optionKey:'id') } Index: grails-app/services/org/weceem/services/ContentRepositoryService.groovy =================================================================== --- grails-app/services/org/weceem/services/ContentRepositoryService.groovy (revision 5) +++ grails-app/services/org/weceem/services/ContentRepositoryService.groovy (revision 6) @@ -199,7 +199,13 @@ if (!weceemSecurityService.hasPermissions(content, permissionList, type)) { throw new AccessDeniedException("User [${weceemSecurityService.userName}] with roles [${weceemSecurityService.userRoles}] does not have the permissions [$permissionList] to access content at [${content.absoluteURI}] in space [${content.space.name}]") } - } + } + + void requireStatusChangePermissions(Space space, Status oldStatus, Status newStatus) throws AccessDeniedException { + if (!weceemSecurityService.isUserAllowedContentStatus(space, oldStatus, newStatus)) { + throw new AccessDeniedException("User [${weceemSecurityService.userName}] with roles [${weceemSecurityService.userRoles}] does not have permission to change status from [${oldStatus}] to [${newStatus}] in space [${space.name}]") + } + } void deleteSpaceContent(Space space) { requirePermissions(space, [WeceemSecurityPolicy.PERMISSION_ADMIN]) @@ -743,7 +749,7 @@ */ def updateNode(String id, def params) { Content content = Content.get(id) - requirePermissions(content, [WeceemSecurityPolicy.PERMISSION_EDIT]) + requirePermissions(content, [WeceemSecurityPolicy.PERMISSION_EDIT]) if (content) { updateNode(content, params) @@ -779,8 +785,12 @@ def oldAbsURI = content.absoluteURI content.saveRevision(params.title ?: content.title, params.space ? Space.get(params.'space.id')?.name : content.space.name) def oldTitle = content.title + def oldStatus = content.status // map in new values hackedBindData(content, params) + // Make sure the user is allowed to make the status transition + requireStatusChangePermissions(content.space, oldStatus, content.status) + if (content instanceof ContentFile){ content.rename(oldTitle) } Index: grails-app/services/org/weceem/services/WeceemSecurityService.groovy =================================================================== --- grails-app/services/org/weceem/services/WeceemSecurityService.groovy (revision 5) +++ grails-app/services/org/weceem/services/WeceemSecurityService.groovy (revision 6) @@ -96,9 +96,8 @@ * Called to find out if the current user is allowed to transition content in to the specified status * Allows applications to control workflow */ - boolean isUserAllowedContentStatus(Status status) { - // Temporary lame impl, need to add this to policy - return true + boolean isUserAllowedContentStatus(Space space, Status fromStatus, Status toStatus) { + return policy.hasPermissionToChangeStatus(space, getUserRoles(), fromStatus, toStatus) } /** @@ -161,4 +160,4 @@ } -} \ No newline at end of file +} Index: test/unit/SecurityPermissionsBuilderTests.groovy =================================================================== --- test/unit/SecurityPermissionsBuilderTests.groovy (revision 5) +++ test/unit/SecurityPermissionsBuilderTests.groovy (revision 6) @@ -78,4 +78,60 @@ policyMockControl.verify() } -} \ No newline at end of file + + void testWithStatusChangePermissions() { + def policyMockControl = mockFor(WeceemSecurityPolicy) + + def expectedPermArgs = [ + [perm:'admin', granted:false, alias:'main', role:'ROLE_TEST'], + [perm:'admin', granted:false, alias:'extranet', role:'ROLE_TEST'], + [perm:'view', granted:true, alias:'main', role:'ROLE_TEST'], + [perm:'view', granted:true, alias:'extranet', role:'ROLE_TEST'] + ] + def invocation = 0 + policyMockControl.demand.setDefaultPermissionForSpaceAndRole(4..4) { perm, granted, alias, role -> + println "Args for invocation $invocation $perm, $granted, $alias, $role" + + assertEquals expectedPermArgs[invocation].perm, perm + assertEquals expectedPermArgs[invocation].granted, granted + assertEquals expectedPermArgs[invocation].alias, alias + assertEquals expectedPermArgs[invocation].role, role + invocation++ + } + + def invocationForStatus = 0 + def expectedArgs = [ + [from:100, to:[200, 300], alias:'main', role:'ROLE_TEST'], + [from:100, to:[200, 300], alias:'extranet', role:'ROLE_TEST'], + [from:200, to:[100, 300, 500], alias:'main', role:'ROLE_TEST'], + [from:200, to:[100, 300, 500], alias:'extranet', role:'ROLE_TEST'], + [from:300, to:[200], alias:'main', role:'ROLE_TEST'], + [from:300, to:[200], alias:'extranet', role:'ROLE_TEST'] + ] + policyMockControl.demand.setStatusChangePermissionsForSpaceAndRole(6..6) { fromStatus, toStatuses, alias, role -> + println "Args for invocation $invocation $fromStatus, $toStatuses, $alias, $role" + + assertEquals expectedArgs[invocationForStatus].from, fromStatus + assertEquals expectedArgs[invocationForStatus].to, toStatuses + assertEquals expectedArgs[invocationForStatus].alias, alias + assertEquals expectedArgs[invocationForStatus].role, role + invocationForStatus++ + } + + def policy = policyMockControl.createMock() + + def builder = new SecurityPermissionsBuilder("ROLE_TEST", policy) + builder.with { + space 'main', 'extranet' + admin false + view true + allowedStatusChanges{ + '100' (200, 300) + '200' (100, 300, 500) + '300' (200) + } + } + + policyMockControl.verify() + } +} Index: test/unit/StatusChangePermissionsBuilderTests.groovy =================================================================== --- test/unit/StatusChangePermissionsBuilderTests.groovy (revision 0) +++ test/unit/StatusChangePermissionsBuilderTests.groovy (revision 6) @@ -0,0 +1,40 @@ +import grails.test.* + +import org.weceem.security.* + +class StatusChangePermissionsBuilderTests extends grails.test.GrailsUnitTestCase { + void testStatusChangePermissionsBuilder() { + def policyMockControl = mockFor(WeceemSecurityPolicy) + + def expectedArgs = [ + [from:100, to:[200, 300], alias:'space1', role:'ROLE_TEST'], + [from:100, to:[200, 300], alias:'space2', role:'ROLE_TEST'], + [from:200, to:[100, 300, 500], alias:'space1', role:'ROLE_TEST'], + [from:200, to:[100, 300, 500], alias:'space2', role:'ROLE_TEST'], + [from:300, to:[200], alias:'space1', role:'ROLE_TEST'], + [from:300, to:[200], alias:'space2', role:'ROLE_TEST'] + ] + def invocation = 0 + policyMockControl.demand.setStatusChangePermissionsForSpaceAndRole(6..6) { fromStatus, toStatuses, alias, role -> + println "Args for invocation $invocation $fromStatus, $toStatuses, $alias, $role" + + assertEquals expectedArgs[invocation].from, fromStatus + assertEquals expectedArgs[invocation].to, toStatuses + assertEquals expectedArgs[invocation].alias, alias + assertEquals expectedArgs[invocation].role, role + invocation++ + } + + def policy = policyMockControl.createMock() + + def builder = new StatusChangePermissionsBuilder("ROLE_TEST", policy, ['space1', 'space2']) + builder.with { + '100' (200, 300) + '200' (100, 300, 500) + '300' (200) + } + + policyMockControl.verify() + } + +} Index: test/unit/SecurityPolicyTests.groovy =================================================================== --- test/unit/SecurityPolicyTests.groovy (revision 5) +++ test/unit/SecurityPolicyTests.groovy (revision 6) @@ -32,6 +32,17 @@ assertTrue policy.hasPermission('weceem', '/anything', [WeceemSecurityPolicy.ROLE_GUEST], [WeceemSecurityPolicy.PERMISSION_VIEW]) assertFalse policy.hasPermission('weceem', '/anything', [WeceemSecurityPolicy.ROLE_GUEST], [WeceemSecurityPolicy.PERMISSION_ADMIN]) assertFalse policy.hasPermission('weceem', '/anything', [WeceemSecurityPolicy.ROLE_GUEST], [WeceemSecurityPolicy.PERMISSION_DELETE]) + + + // Testing some samples (not exhaustive) + assertTrue policy.hasPermissionToChangeStatus('weceem', [WeceemSecurityPolicy.ROLE_ADMIN], 100, 200) + assertTrue policy.hasPermissionToChangeStatus('weceem', [WeceemSecurityPolicy.ROLE_ADMIN, WeceemSecurityPolicy.ROLE_USER], 100, 300) + assertTrue policy.hasPermissionToChangeStatus('weceem', [WeceemSecurityPolicy.ROLE_ADMIN], 100, 400) + assertTrue policy.hasPermissionToChangeStatus('weceem', [WeceemSecurityPolicy.ROLE_USER], 400, 300) + assertTrue policy.hasPermissionToChangeStatus('weceem', [WeceemSecurityPolicy.ROLE_USER], 400, 400) + assertTrue policy.hasPermissionToChangeStatus('weceem', [WeceemSecurityPolicy.ROLE_GUEST], 200, 200) + assertFalse policy.hasPermissionToChangeStatus('weceem', [WeceemSecurityPolicy.ROLE_GUEST], 200, 300) + assertTrue policy.hasPermissionToChangeStatus('weceem', [WeceemSecurityPolicy.ROLE_GUEST, WeceemSecurityPolicy.ROLE_USER], 200, 300) } void testSpaceRoleDefaults() { @@ -122,6 +133,25 @@ ['spaceA_guest'], [WeceemSecurityPolicy.PERMISSION_VIEW]) } + void testStatusChangePermissions() { + policy.setStatusChangePermissionsForSpaceAndRole(100, [200, 300, 400], WeceemSecurityPolicy.ANY_SPACE_ALIAS, 'editor') + policy.setStatusChangePermissionsForSpaceAndRole(100, [], 'spaceA', 'editor') + policy.setStatusChangePermissionsForSpaceAndRole(100, [300], 'spaceA', 'publisher') + + // Permissions inherit from ANY_SPACE + assertTrue policy.hasPermissionToChangeStatus('spaceB', ['editor'], 100, 300) + // Permissions must be explicitly granted + assertFalse policy.hasPermissionToChangeStatus('spaceB', ['editor'], 200, 300) + // If no change is being made, it should never fail + assertTrue policy.hasPermissionToChangeStatus('spaceB', ['editor'], 300, 300) + // Make sure permissions can be removed for certain spaces + assertFalse policy.hasPermissionToChangeStatus('spaceA', ['editor'], 100, 300) + // Any user role can grant permissions + assertTrue policy.hasPermissionToChangeStatus('spaceA', ['editor', 'publisher'], 100, 300) + // ...and the order of roles shouldn't matter + assertTrue policy.hasPermissionToChangeStatus('spaceA', ['publisher', 'editor'], 100, 300) + } + void testMissingPolicyCausesException() { shouldFail { policy.load('this_path_does_not_exist.groovy') @@ -134,7 +164,7 @@ /* "ROLE_ADMIN" { space '', 'test' - + admin true view true edit true @@ -144,19 +174,24 @@ "ROLE_USER" { space '', 'test' - + view true - + "/blog" { edit true create true } + + allowedStatusChanges{ + '100' (200, 300) + '300' (200) + } } "ROLE_GUEST" { space '' - + view true } */ @@ -177,6 +212,20 @@ assertTrue policy.hasPermission(spc, '/blog/anything', ['ROLE_USER'], [WeceemSecurityPolicy.PERMISSION_CREATE]) assertFalse policy.hasPermission(spc, '/blog/anything', ['ROLE_USER'], [WeceemSecurityPolicy.PERMISSION_DELETE]) } + ['', 'test'].each { spc -> + assertTrue policy.hasPermissionToChangeStatus(spc, ['ROLE_USER'], 100, 200) + assertTrue policy.hasPermissionToChangeStatus(spc, ['ROLE_USER'], 100, 300) + assertTrue policy.hasPermissionToChangeStatus(spc, ['ROLE_USER'], 300, 200) + assertTrue policy.hasPermissionToChangeStatus(spc, ['ROLE_USER'], 200, 200) + assertFalse policy.hasPermissionToChangeStatus(spc, ['ROLE_GUEST'], 100, 200) + assertFalse policy.hasPermissionToChangeStatus(spc, ['ROLE_GUEST'], 100, 300) + assertFalse policy.hasPermissionToChangeStatus(spc, ['ROLE_GUEST'], 300, 200) + assertTrue policy.hasPermissionToChangeStatus(spc, ['ROLE_GUEST'], 200, 200) + assertFalse policy.hasPermissionToChangeStatus(spc, ['ROLE_ADMIN'], 100, 200) + assertFalse policy.hasPermissionToChangeStatus(spc, ['ROLE_ADMIN'], 100, 300) + assertFalse policy.hasPermissionToChangeStatus(spc, ['ROLE_ADMIN'], 300, 200) + assertTrue policy.hasPermissionToChangeStatus(spc, ['ROLE_ADMIN'], 200, 200) + } def spc = '' assertTrue policy.hasPermission(spc, '/anything', ['ROLE_GUEST'], [WeceemSecurityPolicy.PERMISSION_VIEW]) @@ -191,4 +240,4 @@ assertFalse policy.hasPermission(spc, '/anything', ['ROLE_GUEST'], [WeceemSecurityPolicy.PERMISSION_EDIT]) assertFalse policy.hasPermission(spc, '/anything', ['ROLE_GUEST'], [WeceemSecurityPolicy.PERMISSION_DELETE]) } -} \ No newline at end of file +} Index: test/files/testpolicy.groovy =================================================================== --- test/files/testpolicy.groovy (revision 5) +++ test/files/testpolicy.groovy (revision 6) @@ -19,6 +19,11 @@ edit true create true } + + allowedStatusChanges{ + '100' (200, 300) + '300' (200) + } } @@ -27,4 +32,4 @@ view true } -} \ No newline at end of file +} Index: src/groovy/org/weceem/security/StatusChangePermissionsBuilder.groovy =================================================================== --- src/groovy/org/weceem/security/StatusChangePermissionsBuilder.groovy (revision 0) +++ src/groovy/org/weceem/security/StatusChangePermissionsBuilder.groovy (revision 6) @@ -0,0 +1,44 @@ +package org.weceem.security + +/** + * Builder that interperets status change permissions of the form: 'statusIdX' (statusIdY, statusIdZ) + * In other words, a status id as a string, followed by a list of comma separated status ids as integers, within parenthesis. + * The interpretation is that the first status may be changed to any of the statuses in the list. + */ +class StatusChangePermissionsBuilder { + + private policy + private role + private spaceAliases + + /** + * This class should be created within a context that already has a role, SecurityPolicy, and space aliases. + */ + StatusChangePermissionsBuilder(role, policy, aliases) { + this.role = role + this.policy = policy + this.spaceAliases = aliases + } + + /** + * Missing methods will be interpereted as instructions to the builder, with the name being a status id, and the args + * being a list of status ids. + */ + def methodMissing(String name, args) { + assert args.size() >= 1 + def fromStatus = name + assert fromStatus.isInteger() + List toStatuses = args + setPermission(fromStatus.toInteger(), toStatuses) + } + + /** + * Allows one status to be changed to certain other statuses, within the spaces and role defined in this class + */ + private void setPermission(fromStatus, toStatuses) { + spaceAliases.each { alias -> + policy.setStatusChangePermissionsForSpaceAndRole(fromStatus, toStatuses, alias, role) + } + } + +} Index: src/groovy/org/weceem/security/WeceemSecurityPolicy.groovy =================================================================== --- src/groovy/org/weceem/security/WeceemSecurityPolicy.groovy (revision 5) +++ src/groovy/org/weceem/security/WeceemSecurityPolicy.groovy (revision 6) @@ -3,11 +3,15 @@ import org.apache.commons.logging.LogFactory import org.apache.commons.logging.Log +import org.weceem.content.Space +import org.weceem.content.Status + class WeceemSecurityPolicy { Log log = LogFactory.getLog(WeceemSecurityPolicy) private entriesBySpace = [:] + private statusChangeMapBySpace = [:] // key: space alias, value: map(key: role, value: map(key: fromStatus, value: list(toStatus))) static DEFAULT_POLICY_URI = "*/" static ANY_SPACE_ALIAS = "*" @@ -64,6 +68,16 @@ setDefaultPermissionForSpaceAndRole(PERMISSION_EDIT, false, ANY_SPACE_ALIAS, ROLE_GUEST) setDefaultPermissionForSpaceAndRole(PERMISSION_VIEW, true, ANY_SPACE_ALIAS, ROLE_GUEST) setDefaultPermissionForSpaceAndRole(PERMISSION_CREATE, false, ANY_SPACE_ALIAS, ROLE_GUEST) + + setStatusChangePermissionsForSpaceAndRole(100, [200, 300, 400], ANY_SPACE_ALIAS, ROLE_ADMIN) + setStatusChangePermissionsForSpaceAndRole(200, [100, 300, 400], ANY_SPACE_ALIAS, ROLE_ADMIN) + setStatusChangePermissionsForSpaceAndRole(300, [100, 200, 400], ANY_SPACE_ALIAS, ROLE_ADMIN) + setStatusChangePermissionsForSpaceAndRole(400, [100, 200, 300], ANY_SPACE_ALIAS, ROLE_ADMIN) + + setStatusChangePermissionsForSpaceAndRole(100, [200, 300, 400], ANY_SPACE_ALIAS, ROLE_USER) + setStatusChangePermissionsForSpaceAndRole(200, [100, 300, 400], ANY_SPACE_ALIAS, ROLE_USER) + setStatusChangePermissionsForSpaceAndRole(300, [100, 200, 400], ANY_SPACE_ALIAS, ROLE_USER) + setStatusChangePermissionsForSpaceAndRole(400, [100, 200, 300], ANY_SPACE_ALIAS, ROLE_USER) } void setDefaultPermissionForSpaceAndRole(String perm, boolean permGranted, String alias, String role) { @@ -90,6 +104,25 @@ } /** + * Set permissions that control which Status can be changed to which other Statuses by role and space. For example, + * define that in space "intranet", users with the role "editor" may change the status of some content from 300 (approved) + * to 100 (draft) or 400 (published). + * + * @param fromStatus The original status of the content + * @param toStatus A list of the statuses to which the content can be changed + * @param alias The space that the permission applies to + * @param role The role that is allowed to make the defined status change + */ + void setStatusChangePermissionsForSpaceAndRole(Integer fromStatus, List toStatuses, String alias, String role) { + if (log.debugEnabled) { + log.debug("Adding status change permission for space [${alias}] and role [${role}]: ${fromStatus} to ${toStatuses}") + } + def statusMapByRole = statusChangeMapBySpace.get(alias, [:]) + def statusMap = statusMapByRole.get(role, [:]) + statusMap[fromStatus] = toStatuses + } + + /** * Find out if the permission name supplied is granted for the role, spaceAlias and uri */ boolean hasPermission(String spaceAlias, String uri, List roleList, List permissionList ) { @@ -161,4 +194,62 @@ } return explicitMatch } -} \ No newline at end of file + + /** + * Determines if any of the given roles has permission to change content from fromStatus to toStatus, in the context of + * the given space. + * @param spaceAlias The space the permission applies to + * @param roleList A list of the roles of the user who is attempting to change the status + * @param fromStatus The original status + * @param toStatus The new status + */ + boolean hasPermissionToChangeStatus(Space space, List roleList, Status fromStatus, Status toStatus) { + return hasPermissionToChangeStatus(space.aliasURI, roleList, fromStatus.code, toStatus.code) + } + + /** + * Determines if any of the given roles has permission to change content from fromStatus to toStatus, in the context of + * the given space. + * @param spaceAlias Alias of the space the permission applies to + * @param roleList A list of the roles of the user who is attempting to change the status + * @param fromStatus The id of the original status + * @param toStatus The id of the new status + */ + boolean hasPermissionToChangeStatus(String spaceAlias, List roleList, Integer fromStatus, Integer toStatus) { + if (fromStatus == toStatus){ + return true // No change is being made, so this is always allowed + } + + if (!roleList) { + log.debug "No roles provided to status change permission check" + return false // User has no roles, and thus no permissions + } + + def statusMapByRole = statusChangeMapBySpace[spaceAlias] + + if (!statusMapByRole) { + statusMapByRole = statusChangeMapBySpace[ANY_SPACE_ALIAS] + if (log.debugEnabled) { + log.debug "Using status change permission map for 'any' space as [$spaceAlias] has none defined" + } + if (!statusMapByRole) { + log.warn "No status change permissions set for space with alias [$spaceAlias], and no default space permissions set" + return false // No permissions set for the "any" space + } + } + + if (log.debugEnabled) { + log.debug "Found status change permission map for space [$spaceAlias]: $statusMapByRole" + } + + def hasPermission = roleList.collect{statusMapByRole[it]?.get(fromStatus)}.flatten().contains(toStatus) + + if (log.debugEnabled) { + log.debug "Permission ${hasPermission?'granted':'denied'} for roles [$roleList] in space [$spaceAlias] to change status from ${fromStatus} to ${toStatus}" + } + + return hasPermission + } + + +} Index: src/groovy/org/weceem/security/SecurityPermissionsBuilder.groovy =================================================================== --- src/groovy/org/weceem/security/SecurityPermissionsBuilder.groovy (revision 5) +++ src/groovy/org/weceem/security/SecurityPermissionsBuilder.groovy (revision 6) @@ -2,6 +2,7 @@ class SecurityPermissionsBuilder { static PERM_CLAUSE_SPACE = "space" + static STATUS_CHANGE_MAP = "allowedStatusChanges" boolean topLevel def policy @@ -47,6 +48,17 @@ assert args.size() == 1 setPermission(name, args[0]) break; + case STATUS_CHANGE_MAP: + if (!topLevel) { + throw new IllegalArgumentException("Cannot set status change permissions on a nested declaration") + } + assert args.size() == 1 + Closure c = args[0] + def statusMapBuilder = new StatusChangePermissionsBuilder(role, policy, spaceAliases) + c.delegate = statusMapBuilder + c.resolveStrategy = Closure.DELEGATE_FIRST + c.call() + break; default: assert args.size() == 1 Closure c = args[0] @@ -69,4 +81,4 @@ } } -} \ No newline at end of file +}