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 all 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 37 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,9 @@ 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.RoleDto
import packit.model.dto.UpdateRolePermissions
import packit.model.toDto
import packit.service.RoleService

@Controller
Expand All @@ -21,13 +24,44 @@ 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()
}

@GetMapping("/names")
fun getRoleNames(): ResponseEntity<List<String>>
{
return ResponseEntity.ok(roleService.getRoleNames())
}

@GetMapping
fun getRolesWithRelationships(@RequestParam isUsername: Boolean?): ResponseEntity<List<RoleDto>>
{
val roles = roleService.getRoles(isUsername)
return ResponseEntity.ok(roles.map { it.toDto() })
}

@GetMapping("/{roleName}")
fun getRole(@PathVariable roleName: String): ResponseEntity<RoleDto>
{
val role = roleService.getRole(roleName)
return ResponseEntity.ok(role.toDto())
}
}
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
3 changes: 3 additions & 0 deletions api/app/src/main/kotlin/packit/model/Packet.kt
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package packit.model
import jakarta.persistence.*
import org.hibernate.annotations.JdbcTypeCode
import org.hibernate.type.SqlTypes
import packit.model.dto.BasicPacketDto
import packit.model.dto.PacketDto

@Entity
Expand Down Expand Up @@ -32,3 +33,5 @@ class Packet(
fun Packet.toDto() = PacketDto(
id, name, displayName, parameters, published, importTime, startTime, endTime
)

fun Packet.toBasicDto() = BasicPacketDto(name, id)
5 changes: 4 additions & 1 deletion api/app/src/main/kotlin/packit/model/PacketGroup.kt
Original file line number Diff line number Diff line change
@@ -1,14 +1,17 @@
package packit.model

import jakarta.persistence.*
import packit.model.dto.PacketGroupDto

@Entity
@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)
var id: Int? = null
)

fun PacketGroup.toDto() = PacketGroupDto(name, id!!)
8 changes: 7 additions & 1 deletion api/app/src/main/kotlin/packit/model/Role.kt
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package packit.model

import jakarta.persistence.*
import packit.model.dto.RoleDto

@Entity
@Table(name = "`role`")
Expand All @@ -9,9 +10,14 @@ class Role(
var isUsername: Boolean = false,
@OneToMany(mappedBy = "role", fetch = FetchType.EAGER, cascade = [CascadeType.ALL])
var rolePermissions: MutableList<RolePermission> = mutableListOf(),
@ManyToMany(mappedBy = "roles", fetch = FetchType.LAZY)
@ManyToMany(mappedBy = "roles", fetch = FetchType.EAGER)
var users: MutableList<User> = mutableListOf(),
@Id
@GeneratedValue(strategy = GenerationType.IDENTITY)
var id: Int? = null,
)

fun Role.toDto() =
RoleDto(
name, rolePermissions.map { it.toDto() }, users.map { it.toDto() }, id!!
)
44 changes: 44 additions & 0 deletions api/app/src/main/kotlin/packit/model/RolePermission.kt
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package packit.model

import jakarta.persistence.*
import packit.model.dto.RolePermissionDto

@Entity
@Table(name = "role_permission")
Expand Down Expand Up @@ -29,3 +30,46 @@ 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

}

fun RolePermission.toDto() = RolePermissionDto(
permission.name,
packet?.toBasicDto(),
tag?.toDto(),
packetGroup?.toDto(),
id!!
)
3 changes: 3 additions & 0 deletions api/app/src/main/kotlin/packit/model/Tag.kt
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package packit.model

import jakarta.persistence.*
import packit.model.dto.TagDto

@Entity
@Table(name = "tag")
Expand All @@ -14,3 +15,5 @@ class Tag(
@GeneratedValue(strategy = GenerationType.IDENTITY)
val id: Int? = null,
)

fun Tag.toDto() = TagDto(name, id!!)
3 changes: 3 additions & 0 deletions api/app/src/main/kotlin/packit/model/User.kt
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package packit.model

import jakarta.persistence.*
import packit.model.dto.UserDto
import java.time.Instant
import java.util.*

Expand All @@ -25,3 +26,5 @@ class User(
@GeneratedValue(strategy = GenerationType.UUID)
val id: UUID? = null
)

fun User.toDto() = UserDto(username, id!!)
6 changes: 6 additions & 0 deletions api/app/src/main/kotlin/packit/model/dto/BasicPacketDto.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package packit.model.dto

data class BasicPacketDto(
val name: String,
val id: String,
)
3 changes: 3 additions & 0 deletions api/app/src/main/kotlin/packit/model/dto/PacketGroupDto.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
package packit.model.dto

data class PacketGroupDto(val name: String, val id: Int)
8 changes: 8 additions & 0 deletions api/app/src/main/kotlin/packit/model/dto/RoleDto.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package packit.model.dto

data class RoleDto(
var name: String,
var rolePermissions: List<RolePermissionDto> = listOf(),
var users: List<UserDto> = listOf(),
var id: Int
)
9 changes: 9 additions & 0 deletions api/app/src/main/kotlin/packit/model/dto/RolePermissionDto.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
package packit.model.dto

data class RolePermissionDto(
val permission: String,
val packet: BasicPacketDto? = null,
val tag: TagDto? = null,
val packetGroup: PacketGroupDto? = null,
val id: Int,
)
3 changes: 3 additions & 0 deletions api/app/src/main/kotlin/packit/model/dto/TagDto.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
package packit.model.dto

data class TagDto(val name: String, val id: Int)
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

@absternator absternator May 7, 2024

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.

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

import java.util.*

data class UserDto(val username: String, val id: UUID)
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>)
}
4 changes: 4 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,8 @@ interface RoleRepository : JpaRepository<Role, Int>
fun findByName(name: String): Role?
fun existsByName(name: String): Boolean
fun findByNameIn(names: List<String>): List<Role>
fun findAllByIsUsername(isUsername: Boolean): List<Role>

@Transactional
fun deleteByName(name: String)
}
80 changes: 80 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,80 @@
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 removeRolePermissionsFromRole(role: Role, removeRolePermissions: List<UpdateRolePermission>)
fun getRolePermissionsToAdd(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
{
internal fun getRolePermissionsToUpdate(
role: Role,
updateRolePermissions: List<UpdateRolePermission>
): List<RolePermission>
{
return updateRolePermissions.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 getRolePermissionsToAdd(
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
}
}
Loading
Loading