Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mrc-5298-Role Update Endpoints #64

Merged
merged 42 commits into from
May 14, 2024
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
58c6150
feat: get updates working
absternator May 1, 2024
9580f70
Remove unused code in RoleService
absternator May 1, 2024
49dd4aa
test: test logic
absternator May 1, 2024
8baee2c
Fix formatting and remove unnecessary code
absternator May 1, 2024
12a3d3c
Merge branch 'mrc-5297-role-delete' of https://github.com/mrc-ide/pac…
absternator May 1, 2024
aa7b95e
Merge branch 'mrc-5297-role-delete' of https://github.com/mrc-ide/pac…
absternator May 1, 2024
ce58d17
Update RoleController endpoints
absternator May 1, 2024
bd0145b
Refactor variable name in RolePermissionService.kt
absternator May 2, 2024
1095f2c
Add exception handler for IllegalArgumentException and add validation…
absternator May 2, 2024
5cf4187
feat: get reads working
absternator May 2, 2024
98e3062
test: unit + integration tests
absternator May 2, 2024
5ed4a0d
Merge branch 'mrc-5297-role-delete' of https://github.com/mrc-ide/pac…
absternator May 2, 2024
5f1f1a3
Merge branch 'mrc-5298-role-update' of https://github.com/mrc-ide/pac…
absternator May 2, 2024
e223d24
Merge branch 'mrc-5297-role-delete' of https://github.com/mrc-ide/pac…
absternator May 2, 2024
a83cacf
Merge branch 'mrc-5298-role-update' of https://github.com/mrc-ide/pac…
absternator May 2, 2024
252975a
Merge branch 'mrc-5297-role-delete' of https://github.com/mrc-ide/pac…
absternator May 3, 2024
8658e23
Merge branch 'mrc-5298-role-update' of https://github.com/mrc-ide/pac…
absternator May 3, 2024
27d5b0e
Refactor model classes
absternator May 3, 2024
bf920c5
Add getUsernameRoles and getNonUsernameRoles endpoints
absternator May 3, 2024
ecb871e
Fix roleDto saving in RoleControllerTest
absternator May 3, 2024
1ce3d7d
Refactor test case for getting non-username roles with relationships
absternator May 3, 2024
169ea1d
Refactor RoleControllerTest to improve readability and add assertions
absternator May 3, 2024
5765f22
Refactor RoleControllerTest to use usernameRole instead of roleDto
absternator May 3, 2024
a413033
Refactor RoleControllerTest to improve username role retrieval
absternator May 3, 2024
f25fcf0
Merge branch 'mrc-5297-role-delete' of https://github.com/mrc-ide/pac…
absternator May 3, 2024
54eabf2
Merge branch 'mrc-5298-role-update' of https://github.com/mrc-ide/pac…
absternator May 3, 2024
513285e
Merge branch 'mrc-5297-role-delete' of https://github.com/mrc-ide/pac…
absternator May 7, 2024
eb95c4c
Merge branch 'mrc-5297-role-delete' of https://github.com/mrc-ide/pac…
absternator May 7, 2024
fa37abd
Merge branch 'mrc-5297-role-delete' of https://github.com/mrc-ide/pac…
absternator May 7, 2024
c6d2bc5
update as per pr coments
absternator May 7, 2024
dab6a1b
Merge branch 'mrc-5298-role-update' of https://github.com/mrc-ide/pac…
absternator May 7, 2024
27d0103
Refactor RoleControllerTest***
absternator May 7, 2024
163e5c0
Refactor RoleController and RoleService
absternator May 7, 2024
30d168b
Remove empty lines in RoleControllerTest
absternator May 7, 2024
bc05370
feat: make add/remove into update
absternator May 8, 2024
5e07232
Merge branch 'mrc-5298-role-update' of https://github.com/mrc-ide/pac…
absternator May 8, 2024
1ea9487
Refactor imports in RoleControllerTest
absternator May 8, 2024
771eead
Update assertSuccess to assertEquals for result status code
absternator May 8, 2024
a98ea9e
Refactor imports in RoleController.kt
absternator May 8, 2024
8130999
Refactor role permission service and role service
absternator May 13, 2024
d6d9e5f
Merge branch 'mrc-5298-role-update' of https://github.com/mrc-ide/pac…
absternator May 13, 2024
6be3222
Merge pull request #65 from mrc-ide/mrc-5296-role-read
absternator May 13, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 15 additions & 3 deletions api/app/src/main/kotlin/packit/controllers/RoleController.kt
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import org.springframework.stereotype.Controller
import org.springframework.validation.annotation.Validated
import org.springframework.web.bind.annotation.*
import packit.model.dto.CreateRole
import packit.model.dto.UpdateRolePermissions
import packit.service.RoleService

@Controller
Expand All @@ -21,12 +22,23 @@ class RoleController(private val roleService: RoleService)
return ResponseEntity.ok(mapOf("message" to "Role created"))
}

@DeleteMapping("/{name}")
@DeleteMapping("/{roleName}")
fun deleteRole(
@PathVariable name: String
@PathVariable roleName: String
): ResponseEntity<Map<String, String?>>
{
roleService.deleteRole(roleName)

return ResponseEntity.noContent().build()
}

@PutMapping("/update-permissions/{roleName}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, this overrides my previous comment!
I think this should just be PUT /role/{roleName}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have done for all PUT as suggested as above in PR #68.. /role/{rolenmame}/permissions & /role/{rolenmame}/users

fun updatePermissionsToRole(
@RequestBody @Validated updateRolePermissions: UpdateRolePermissions,
@PathVariable roleName: String
): ResponseEntity<Unit>
{
roleService.deleteRole(name)
roleService.updatePermissionsToRole(roleName, updateRolePermissions)

return ResponseEntity.noContent().build()
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,13 @@ class PackitExceptionHandler
.toResponseEntity()
}

@ExceptionHandler(IllegalArgumentException::class)
fun handleIllegalArgumentException(e: Exception): ResponseEntity<String>
{
return ErrorDetail(HttpStatus.BAD_REQUEST, e.message ?: "Invalid argument")
.toResponseEntity()
}

@ExceptionHandler(AccessDeniedException::class, AuthenticationException::class)
fun handleAccessDenied(e: Exception): ResponseEntity<String>
{
Expand Down
2 changes: 1 addition & 1 deletion api/app/src/main/kotlin/packit/model/PacketGroup.kt
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import jakarta.persistence.*
@Table(name = "packet_group")
class PacketGroup(
var name: String,
@OneToMany(mappedBy = "packetGroup")
@OneToMany(mappedBy = "packetGroup", cascade = [CascadeType.ALL])
var rolePermissions: MutableList<RolePermission> = mutableListOf(),
@Id
@GeneratedValue(strategy = GenerationType.IDENTITY)
Expand Down
35 changes: 35 additions & 0 deletions api/app/src/main/kotlin/packit/model/RolePermission.kt
Original file line number Diff line number Diff line change
Expand Up @@ -29,3 +29,38 @@ class RolePermission(
@GeneratedValue(strategy = GenerationType.IDENTITY)
var id: Int? = null,
)
{
init
{
val nonNullFields = listOf(packet, tag, packetGroup).count { it != null }
require(nonNullFields <= 1) {
"Either all of packet, tag, packetGroup should be null or only one of them should be not null"
}
}
Comment on lines +33 to +40
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Repeated logic... Given that you've got this constraint on the db, and also in the dto, maybe you don't need it here as well. But if so, should be able to implement once and use from both types.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah i thought that too but couldnt find a way to implement once and use both places.. Also they a bit different as one is ids and other is whole objects... And probs don't need but put here to for safety and mirror this constraint on DB and make more visible (as many people don't check db constraints haha)


override fun equals(other: Any?): Boolean
{
return when
{
this === other -> true
other !is RolePermission -> false
role.name != other.role.name -> false
permission.name != other.permission.name -> false
packet?.id != other.packet?.id -> false
packetGroup?.id != other.packetGroup?.id -> false
tag?.id != other.tag?.id -> false
else -> true
}
}
Comment on lines +42 to +55
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be less verbose just to check equality through the dto data class..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need on entity so can filter out data returned from entities... also mostly services using entites

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, pros and cons of both approaches, but as discussed it does seem easier to have the services talking to each other with entities.


override fun hashCode(): Int
{
val prime = 31
var result = role.name.hashCode()
result = prime * result + permission.name.hashCode()
result = prime * result + packet?.id.hashCode()
result = prime * result + packetGroup?.id.hashCode()
result = prime * result + tag?.id.hashCode()
return result
}
Comment on lines +57 to +66
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need the hash code? And could you just use that to check for equality?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah i did that initially but lint complained and said I need equals and haschode... and read and found its best to have both

}
25 changes: 25 additions & 0 deletions api/app/src/main/kotlin/packit/model/dto/UpdateRolePermission.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
package packit.model.dto

import org.jetbrains.annotations.NotNull

data class UpdateRolePermissions(
val addPermissions: List<UpdateRolePermission> = listOf(),
val removePermissions: List<UpdateRolePermission> = listOf()
)

data class UpdateRolePermission(
@field:NotNull
val permission: String,
val packetId: String? = null,
val tagId: Int? = null,
val packetGroupId: Int? = null
)
Comment on lines +10 to +16
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we're only allowing global permissions when we create a role, but can update with scoped permissions? That seems inconsistent. Could accept a list of these on create role, rather than just permission names.. and then wouldn't this type also be the scoped permission type returned from the GET request too? ...oh, minus the id of course

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for the create the FE most likely wont pass any permissions.. And here update is where it can pass in scoped if needed...The idea was it would be weird adding scoped permissions when creating a role as well... But adding a scoped permission when updating makes sense also.... This logic will be mirrored on the FE.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, but you still want to allow global permissions on create to make life easier for the admin cli/setup scripts?
I'm not really convinced it's a useful distinction. Setup might want to make roles with scoped permissions in some cases for example. I think it would be fine to make role create take no perms and require that to be done in a second request.

{
init
{
val nonNullFields = listOf(packetId, tagId, packetGroupId).count { it != null }
require(nonNullFields <= 1) {
"Either all of packetId, tagId, packetGroupId should be null or only one of them should be not null"
}
}
Comment on lines +18 to +24
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is nice, and I think we could still keep this anyway, even this type was also re-used as the RolePermission dto.

}
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,5 @@ import packit.model.Permission
interface PermissionRepository : JpaRepository<Permission, Int>
{
fun findByNameIn(names: List<String>): List<Permission>
fun findByName(name: String): Permission?
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
package packit.repository

import org.springframework.data.jpa.repository.JpaRepository
import org.springframework.data.jpa.repository.Modifying
import org.springframework.data.jpa.repository.Query
import org.springframework.stereotype.Repository
import org.springframework.transaction.annotation.Transactional
import packit.model.RolePermission

@Repository
interface RolePermissionRepository : JpaRepository<RolePermission, Int>
{
@Modifying
@Transactional
@Query("DELETE FROM RolePermission rp WHERE rp.id IN :ids")
fun deleteAllByIdIn(ids: List<Int>)
}
3 changes: 3 additions & 0 deletions api/app/src/main/kotlin/packit/repository/RoleRepository.kt
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package packit.repository

import org.springframework.data.jpa.repository.JpaRepository
import org.springframework.stereotype.Repository
import org.springframework.transaction.annotation.Transactional
import packit.model.Role

@Repository
Expand All @@ -10,5 +11,7 @@ interface RoleRepository : JpaRepository<Role, Int>
fun findByName(name: String): Role?
fun existsByName(name: String): Boolean
fun findByNameIn(names: List<String>): List<Role>

@Transactional
fun deleteByName(name: String)
}
81 changes: 81 additions & 0 deletions api/app/src/main/kotlin/packit/service/RolePermissionService.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
package packit.service

import org.springframework.http.HttpStatus
import org.springframework.stereotype.Service
import packit.exceptions.PackitException
import packit.model.Role
import packit.model.RolePermission
import packit.model.dto.UpdateRolePermission
import packit.repository.*

interface RolePermissionService
{
fun getRolePermissionsToUpdate(role: Role, addRolePermissions: List<UpdateRolePermission>): List<RolePermission>
fun removeRolePermissionsFromRole(role: Role, removeRolePermissions: List<UpdateRolePermission>)
fun getAddRolePermissionsFromRole(role: Role, addRolePermissions: List<UpdateRolePermission>): List<RolePermission>
}

@Service
class BaseRolePermissionService(
private val permissionRepository: PermissionRepository,
private val packetRepository: PacketRepository,
private val packetGroupRepository: PacketGroupRepository,
private val tagRepository: TagRepository,
private val rolePermissionRepository: RolePermissionRepository
) : RolePermissionService
{
override fun getRolePermissionsToUpdate(
role: Role,
addRolePermissions: List<UpdateRolePermission>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
addRolePermissions: List<UpdateRolePermission>
updateRolePermissions: List<UpdateRolePermission>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i named like that to differentiate the adding and deleting of role permissions

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh yup was mistaken have updated

): List<RolePermission>
{
return addRolePermissions.map { addRolePermission ->
val permission = permissionRepository.findByName(addRolePermission.permission)
?: throw PackitException("invalidPermissionsProvided", HttpStatus.BAD_REQUEST)

RolePermission(
role = role,
permission = permission,
packet = addRolePermission.packetId?.let {
packetRepository.findById(it)
.orElseThrow { PackitException("packetNotFound", HttpStatus.BAD_REQUEST) }
},
packetGroup = addRolePermission.packetGroupId?.let {
packetGroupRepository.findById(it)
.orElseThrow { PackitException("packetGroupNotFound", HttpStatus.BAD_REQUEST) }
},
tag = addRolePermission.tagId?.let {
tagRepository.findById(it).orElseThrow { PackitException("tagNotFound", HttpStatus.BAD_REQUEST) }
}
)
}
}

override fun removeRolePermissionsFromRole(role: Role, removeRolePermissions: List<UpdateRolePermission>)
{
val rolePermissionsToRemove = getRolePermissionsToUpdate(role, removeRolePermissions)

val matchedRolePermissionsToRemove = rolePermissionsToRemove.map { rolePermissionToRemove ->
val matchedPermission = role.rolePermissions.find { rolePermissionToRemove == it }
?: throw PackitException("rolePermissionDoesNotExist", HttpStatus.BAD_REQUEST)

matchedPermission
}

rolePermissionRepository.deleteAllByIdIn(matchedRolePermissionsToRemove.map { it.id!! })
}

override fun getAddRolePermissionsFromRole(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's quite confusing having getRolePermissionsToUpdate and getAddRolePermissionsFromRole (that sounds like it's getting the permissions from the role, but if the permissions are already in the role, you're going to throw an exception!). I wouldn't mind just making the method into a checkPermissionsToAddAreNotOnRole or something, and have the main update method call `getRolePermissionsToUpdate directly, and pass the perms into here to just do the check...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup agree ended up changing to getRolePermissionsToAdd and making the getRolePermissionsToUpdate internal so not visible to public

role: Role,
addRolePermissions: List<UpdateRolePermission>
): List<RolePermission>
{
val rolePermissionsToAdd = getRolePermissionsToUpdate(role, addRolePermissions)
if (rolePermissionsToAdd.any { role.rolePermissions.contains(it) })
{
throw PackitException("rolePermissionAlreadyExists", HttpStatus.BAD_REQUEST)
}

return rolePermissionsToAdd
}
}
30 changes: 26 additions & 4 deletions api/app/src/main/kotlin/packit/service/RoleService.kt
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,13 @@ import org.springframework.http.HttpStatus
import org.springframework.security.core.GrantedAuthority
import org.springframework.security.core.authority.SimpleGrantedAuthority
import org.springframework.stereotype.Service
import org.springframework.transaction.annotation.Transactional
import packit.exceptions.PackitException
import packit.model.Permission
import packit.model.Role
import packit.model.RolePermission
import packit.model.dto.CreateRole
import packit.model.dto.UpdateRolePermission
import packit.model.dto.UpdateRolePermissions
import packit.repository.RoleRepository

interface RoleService
Expand All @@ -20,12 +21,14 @@ interface RoleService
fun getGrantedAuthorities(roles: List<Role>): MutableList<GrantedAuthority>
fun createRole(createRole: CreateRole)
fun deleteRole(roleName: String)
fun updatePermissionsToRole(roleName: String, updateRolePermissions: UpdateRolePermissions)
}

@Service
class BaseRoleService(
private val roleRepository: RoleRepository,
private val permissionService: PermissionService
private val permissionService: PermissionService,
private val rolePermissionService: RolePermissionService
) : RoleService
{
override fun getUsernameRole(username: String): Role
Expand All @@ -52,17 +55,36 @@ class BaseRoleService(
saveRole(createRole.name, permissions)
}

@Transactional
override fun deleteRole(roleName: String)
{

if (!roleRepository.existsByName(roleName))
{
throw PackitException("roleNotFound", HttpStatus.BAD_REQUEST)
}
roleRepository.deleteByName(roleName)
}

override fun updatePermissionsToRole(roleName: String, updateRolePermissions: UpdateRolePermissions)
{
val role = roleRepository.findByName(roleName)
?: throw PackitException("roleNotFound", HttpStatus.BAD_REQUEST)

val roleAfterPermissionAdd = addRolePermissionsToRole(role, updateRolePermissions.addPermissions)

rolePermissionService.removeRolePermissionsFromRole(
roleAfterPermissionAdd,
updateRolePermissions.removePermissions
)
}

internal fun addRolePermissionsToRole(role: Role, addRolePermissions: List<UpdateRolePermission>): Role
{
val rolePermissionsToAdd =
rolePermissionService.getAddRolePermissionsFromRole(role, addRolePermissions)
role.rolePermissions.addAll(rolePermissionsToAdd)
return roleRepository.save(role)
}

internal fun saveRole(roleName: String, permissions: List<Permission> = listOf())
{
if (roleRepository.existsByName(roleName))
Expand Down
5 changes: 5 additions & 0 deletions api/app/src/main/resources/errorBundle.properties
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,9 @@ userAlreadyExists=User already exists
userNotFound=User not found
roleAlreadyExists=Role already exists
roleNotFound=Role not found
rolePermissionAlreadyExists=Role permission already exists
packetNotFound=Packet not found
packetGroupNotFound=Packet group not found
tagNotFound=Tag not found
rolePermissionDoesNotExist=Permission does not exist on Role
adminRoleNotFound=Admin role not found, contact your administrator