From 341c1f76a4c41158a41bbe6e7302741fc15b6c44 Mon Sep 17 00:00:00 2001 From: Dean Del Ponte Date: Wed, 4 Oct 2017 12:16:08 -0500 Subject: [PATCH 1/3] Issue #79: Refactor UserController as demonstrated in PR 13 This is step one of the refactoring which: 1. pulls the existing code used to build the `roleMap` out into its own, testable method 2. implements a test to verify existing functionality --- .../springsecurity/ui/UserController.groovy | 19 +++++++--- .../ui/UserControllerSpec.groovy | 38 +++++++++++++++++++ 2 files changed, 51 insertions(+), 6 deletions(-) create mode 100644 plugin/src/test/groovy/grails/plugin/springsecurity/ui/UserControllerSpec.groovy diff --git a/plugin/grails-app/controllers/grails/plugin/springsecurity/ui/UserController.groovy b/plugin/grails-app/controllers/grails/plugin/springsecurity/ui/UserController.groovy index fc3a3526..033aadf5 100644 --- a/plugin/grails-app/controllers/grails/plugin/springsecurity/ui/UserController.groovy +++ b/plugin/grails-app/controllers/grails/plugin/springsecurity/ui/UserController.groovy @@ -77,19 +77,26 @@ class UserController extends AbstractS2UiDomainController { protected Map buildUserModel(user) { Set userRoleNames = user[authoritiesPropertyName].collect { it[authorityNameField] } - def granted = [:] - def notGranted = [:] + Map roleMap = buildRoleMap(userRoleNames) + + [roleMap: roleMap, tabData: tabData, user: user] + } + + protected Map buildRoleMap(Set userRoleNames) { + if (!userRoleNames) { + return [:] + } + Map granted = [:] + Map notGranted = [:] for (role in sortedRoles()) { String authority = role[authorityNameField] if (userRoleNames.contains(authority)) { granted[(role)] = userRoleNames.contains(authority) - } - else { + } else { notGranted[(role)] = userRoleNames.contains(authority) } } - - [roleMap: granted + notGranted, tabData: tabData, user: user] + return granted + notGranted } protected List sortedRoles() { diff --git a/plugin/src/test/groovy/grails/plugin/springsecurity/ui/UserControllerSpec.groovy b/plugin/src/test/groovy/grails/plugin/springsecurity/ui/UserControllerSpec.groovy new file mode 100644 index 00000000..4cb84fbd --- /dev/null +++ b/plugin/src/test/groovy/grails/plugin/springsecurity/ui/UserControllerSpec.groovy @@ -0,0 +1,38 @@ +package grails.plugin.springsecurity.ui + +import grails.testing.web.controllers.ControllerUnitTest +import spock.lang.Specification +import spock.lang.Unroll + +@Unroll +class UserControllerSpec extends Specification implements ControllerUnitTest { + static final Map ADMIN_ROLE = [authority: "ROLE_ADMIN"] + static final Map SUPER_ADMIN_ROLE = [authority: "ROLE_SUPER_ADMIN"] + static final Map USER_ROLE = [authority: "ROLE_USER"] + + void "verify proper construction of roleMap for user with roles #rolesAssignedToUser"() { + given: "the authority name field has been set to the default name of 'authority'" + controller.authorityNameField = "authority" + + and: "we mock the returning of all Role instances within the database" + controller.metaClass.sortedRoles = { + [ + ADMIN_ROLE, + SUPER_ADMIN_ROLE, + USER_ROLE + ] + } + + when: "we call buildRoleMap with the role names associated to the user" + Map results = controller.buildRoleMap(rolesAssignedToUser) + + then: "the user is only granted access to roles with which they are associated" + results == expectedResults + + where: + rolesAssignedToUser | expectedResults + [ADMIN_ROLE.authority, USER_ROLE.authority] as Set | [(ADMIN_ROLE): true, (SUPER_ADMIN_ROLE): false, (USER_ROLE): true] + [] as Set | [:] + null | [:] + } +} From fccf2626d8643361cbcc87a9b3e95cee68c6b570 Mon Sep 17 00:00:00 2001 From: Dean Del Ponte Date: Wed, 4 Oct 2017 13:40:06 -0500 Subject: [PATCH 2/3] Issue #79: Refactor UserController as demonstrated in PR 13 Applied refactoring from PR #13 --- .../plugin/springsecurity/ui/UserController.groovy | 11 +++-------- .../springsecurity/ui/UserControllerSpec.groovy | 1 + 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/plugin/grails-app/controllers/grails/plugin/springsecurity/ui/UserController.groovy b/plugin/grails-app/controllers/grails/plugin/springsecurity/ui/UserController.groovy index 033aadf5..a7e81cb4 100644 --- a/plugin/grails-app/controllers/grails/plugin/springsecurity/ui/UserController.groovy +++ b/plugin/grails-app/controllers/grails/plugin/springsecurity/ui/UserController.groovy @@ -86,17 +86,12 @@ class UserController extends AbstractS2UiDomainController { if (!userRoleNames) { return [:] } - Map granted = [:] - Map notGranted = [:] + Map roleMap = [:] for (role in sortedRoles()) { String authority = role[authorityNameField] - if (userRoleNames.contains(authority)) { - granted[(role)] = userRoleNames.contains(authority) - } else { - notGranted[(role)] = userRoleNames.contains(authority) - } + roleMap[(role)] = userRoleNames.contains(authority) } - return granted + notGranted + return roleMap } protected List sortedRoles() { diff --git a/plugin/src/test/groovy/grails/plugin/springsecurity/ui/UserControllerSpec.groovy b/plugin/src/test/groovy/grails/plugin/springsecurity/ui/UserControllerSpec.groovy index 4cb84fbd..d65251e8 100644 --- a/plugin/src/test/groovy/grails/plugin/springsecurity/ui/UserControllerSpec.groovy +++ b/plugin/src/test/groovy/grails/plugin/springsecurity/ui/UserControllerSpec.groovy @@ -28,6 +28,7 @@ class UserControllerSpec extends Specification implements ControllerUnitTest Date: Thu, 19 Oct 2017 17:25:20 +0200 Subject: [PATCH 3/3] Back to original two map implementation --- .../springsecurity/ui/UserController.groovy | 17 ++++++++++++----- .../springsecurity/ui/UserControllerSpec.groovy | 10 ++-------- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/plugin/grails-app/controllers/grails/plugin/springsecurity/ui/UserController.groovy b/plugin/grails-app/controllers/grails/plugin/springsecurity/ui/UserController.groovy index a7e81cb4..17d99f87 100644 --- a/plugin/grails-app/controllers/grails/plugin/springsecurity/ui/UserController.groovy +++ b/plugin/grails-app/controllers/grails/plugin/springsecurity/ui/UserController.groovy @@ -15,6 +15,7 @@ package grails.plugin.springsecurity.ui import grails.plugin.springsecurity.ui.strategy.UserStrategy +import groovy.transform.CompileStatic /** * @author Burt Beckwith @@ -82,16 +83,22 @@ class UserController extends AbstractS2UiDomainController { [roleMap: roleMap, tabData: tabData, user: user] } - protected Map buildRoleMap(Set userRoleNames) { + @CompileStatic + protected Map buildRoleMap(Set userRoleNames, List sortedRoles) { if (!userRoleNames) { return [:] } - Map roleMap = [:] - for (role in sortedRoles()) { + Map granted = [:] + Map notGranted = [:] + for (role in sortedRoles) { String authority = role[authorityNameField] - roleMap[(role)] = userRoleNames.contains(authority) + if (userRoleNames.contains(authority)) { + granted[(role)] = true + } else { + notGranted[(role)] = false + } } - return roleMap + granted + notGranted } protected List sortedRoles() { diff --git a/plugin/src/test/groovy/grails/plugin/springsecurity/ui/UserControllerSpec.groovy b/plugin/src/test/groovy/grails/plugin/springsecurity/ui/UserControllerSpec.groovy index d65251e8..b3f64c48 100644 --- a/plugin/src/test/groovy/grails/plugin/springsecurity/ui/UserControllerSpec.groovy +++ b/plugin/src/test/groovy/grails/plugin/springsecurity/ui/UserControllerSpec.groovy @@ -15,16 +15,10 @@ class UserControllerSpec extends Specification implements ControllerUnitTest