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 5295- Role create endpoint #62

Merged
merged 13 commits into from
May 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions api/app/src/main/kotlin/packit/controllers/LoginController.kt
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ import org.springframework.validation.annotation.Validated
import org.springframework.web.bind.annotation.*
import packit.AppConfig
import packit.exceptions.PackitException
import packit.model.LoginWithPassword
import packit.model.LoginWithToken
import packit.model.dto.LoginWithPassword
import packit.model.dto.LoginWithToken
import packit.service.BasicLoginService
import packit.service.GithubAPILoginService

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@ import org.springframework.core.io.ByteArrayResource
import org.springframework.data.domain.Page
import org.springframework.http.ResponseEntity
import org.springframework.web.bind.annotation.*
import packit.model.PacketGroupSummary
import packit.model.PacketMetadata
import packit.model.PageablePayload
import packit.model.dto.PacketDto
import packit.model.dto.PacketGroupSummary
import packit.model.toDto
import packit.service.PacketService

Expand Down
25 changes: 25 additions & 0 deletions api/app/src/main/kotlin/packit/controllers/RoleController.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
package packit.controllers

import org.springframework.http.ResponseEntity
import org.springframework.security.access.prepost.PreAuthorize
import org.springframework.stereotype.Controller
import org.springframework.validation.annotation.Validated
import org.springframework.web.bind.annotation.PostMapping
import org.springframework.web.bind.annotation.RequestBody
import org.springframework.web.bind.annotation.RequestMapping
import packit.model.dto.CreateRole
import packit.service.RoleService

@Controller
@PreAuthorize("hasAuthority('user.manage')")
@RequestMapping("/role")
class RoleController(private val roleService: RoleService)
{
@PostMapping()
fun createRole(@RequestBody @Validated createRole: CreateRole): ResponseEntity<Map<String, String?>>
{
roleService.createRole(createRole)

return ResponseEntity.ok(mapOf("message" to "Role created"))
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure how useful {"message": "Role created"} is really. It might be nice to actually just return the new Role, since the front end is probably going to want to display it immediately. Or else just return an empty 200 and let the front end decide what message it wants to show.

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.. have created dtos needed for this in user endpoints so for now have created ticket https://mrc-ide.myjetbrains.com/youtrack/issue/mrc-5324/Create-endpoints-to-return-Dtos... this will get done after endpoints tickets

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import org.springframework.web.bind.annotation.RequestBody
import org.springframework.web.bind.annotation.RequestMapping
import packit.AppConfig
import packit.exceptions.PackitException
import packit.model.CreateBasicUser
import packit.model.dto.CreateBasicUser
import packit.service.UserService

@Controller
Expand Down
3 changes: 1 addition & 2 deletions api/app/src/main/kotlin/packit/model/Packet.kt
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,7 @@ class Packet(
inverseJoinColumns = [JoinColumn(name = "tag_id")]
)
var tags: MutableList<Tag> = mutableListOf(),

@OneToMany(mappedBy = "packet")
@OneToMany(mappedBy = "packet", cascade = [CascadeType.ALL])
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure rolePermissions actually belongs on Packet anyway - it's not really of interest relating to the packet itself, just something we need to take account of when determing user access to packet, and for admin users managing access -but they'll be coming from the user/role side and won't be interested in the full packet data at that point.

But even so, I guess it's useful to include it here so that we get the cascade functionality on delete?

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 it needs to be there because there a FK from the role_permissions table to the packet table...

var rolePermissions: MutableList<RolePermission> = mutableListOf()
)

Expand Down
2 changes: 1 addition & 1 deletion api/app/src/main/kotlin/packit/model/Permission.kt
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import jakarta.persistence.*
class Permission(
var name: String,
var description: String,
@OneToMany(mappedBy = "permission")
@OneToMany(mappedBy = "permission", cascade = [CascadeType.ALL])
var rolePermissions: MutableList<RolePermission> = mutableListOf(),
@Id
@GeneratedValue(strategy = GenerationType.IDENTITY)
Expand Down
2 changes: 1 addition & 1 deletion api/app/src/main/kotlin/packit/model/Role.kt
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import jakarta.persistence.*
class Role(
var name: String,
var isUsername: Boolean = false,
@OneToMany(mappedBy = "role", fetch = FetchType.EAGER)
@OneToMany(mappedBy = "role", fetch = FetchType.EAGER, cascade = [CascadeType.ALL])
var rolePermissions: MutableList<RolePermission> = mutableListOf(),
@ManyToMany(mappedBy = "roles", fetch = FetchType.LAZY)
var users: MutableList<User> = mutableListOf(),
Expand Down
2 changes: 1 addition & 1 deletion api/app/src/main/kotlin/packit/model/Tag.kt
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ class Tag(
val name: String,
@ManyToMany(mappedBy = "tags")
var packets: MutableList<Packet> = mutableListOf(),
@OneToMany(mappedBy = "tag")
@OneToMany(mappedBy = "tag", cascade = [CascadeType.ALL])
var rolePermissions: MutableList<RolePermission> = mutableListOf(),
@Id
@GeneratedValue(strategy = GenerationType.IDENTITY)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package packit.model
package packit.model.dto

import jakarta.validation.constraints.Email
import jakarta.validation.constraints.Size
Expand Down
9 changes: 9 additions & 0 deletions api/app/src/main/kotlin/packit/model/dto/CreateRole.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
package packit.model.dto

import org.jetbrains.annotations.NotNull

data class CreateRole(
@field:NotNull
val name: String,
val permissionNames: List<String> = listOf()
)
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package packit.model
package packit.model.dto

import org.jetbrains.annotations.NotNull

Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package packit.model
package packit.model.dto

import org.jetbrains.annotations.NotNull

Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
package packit.model
package packit.model.dto

import packit.model.TimeMetadata

data class OutpackMetadata(
val id: String,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package packit.model
package packit.model.dto

// Projection class for PacketRepository.findPacketGroupSummaryByName
interface PacketGroupSummary
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import org.springframework.data.jpa.repository.JpaRepository
import org.springframework.data.jpa.repository.Query
import org.springframework.stereotype.Repository
import packit.model.Packet
import packit.model.PacketGroupSummary
import packit.model.dto.PacketGroupSummary

@Repository
interface PacketRepository : JpaRepository<Packet, String>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,6 @@ import packit.model.Permission

@Repository
interface PermissionRepository : JpaRepository<Permission, Int>
{
fun findByNameIn(names: List<String>): List<Permission>
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,6 @@ import packit.model.Role
interface RoleRepository : JpaRepository<Role, Int>
{
fun findByName(name: String): Role?

fun existsByName(name: String): Boolean
fun findByNameIn(names: List<String>): List<Role>
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import org.springframework.security.authentication.AuthenticationManager
import org.springframework.security.authentication.UsernamePasswordAuthenticationToken
import org.springframework.stereotype.Component
import packit.exceptions.PackitException
import packit.model.LoginWithPassword
import packit.model.dto.LoginWithPassword
import packit.security.profile.BasicUserDetails
import packit.security.provider.JwtIssuer

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import org.springframework.stereotype.Component
import packit.AppConfig
import packit.clients.GithubUserClient
import packit.exceptions.PackitException
import packit.model.LoginWithToken
import packit.model.dto.LoginWithToken
import packit.security.profile.UserPrincipal
import packit.security.provider.JwtIssuer

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@ import org.springframework.web.client.HttpStatusCodeException
import org.springframework.web.client.RestTemplate
import packit.AppConfig
import packit.exceptions.PackitException
import packit.model.OutpackMetadata
import packit.model.OutpackResponse
import packit.model.PacketMetadata
import packit.model.dto.OutpackMetadata
import java.net.URI

interface OutpackServer
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,11 @@ import org.springframework.http.MediaType
import org.springframework.stereotype.Service
import packit.contentTypes
import packit.exceptions.PackitException
import packit.model.*
import packit.model.Packet
import packit.model.PacketGroup
import packit.model.PacketMetadata
import packit.model.PageablePayload
import packit.model.dto.PacketGroupSummary
import packit.repository.PacketGroupRepository
import packit.repository.PacketRepository
import java.security.MessageDigest
Expand Down
29 changes: 29 additions & 0 deletions api/app/src/main/kotlin/packit/service/PermissionService.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package packit.service
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there's an inconsistency in module naming creeping in here, as you've got PacketService interface and BasePacketService class declared in BasePacketService module, but here you've named the module after the interface, PermissionsService. I actually prefer this way, so would suggest renaming BasePacketService module to PacketService.

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 yeah didnt even notice that for packet one must've been old... but yup il update this


import org.springframework.http.HttpStatus
import org.springframework.stereotype.Service
import packit.exceptions.PackitException
import packit.model.Permission
import packit.repository.PermissionRepository

interface PermissionService
{
fun checkMatchingPermissions(permissionsToCheck: List<String>): List<Permission>
}

@Service
class BasePermissionService(
private val permissionRepository: PermissionRepository
) : PermissionService
{
override fun checkMatchingPermissions(permissionsToCheck: List<String>): List<Permission>
Copy link
Contributor

Choose a reason for hiding this comment

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

So this method is literally just checking that the basic permissions themselves exist in the database? We're not checking that a user has sufficient permissions to do anything? So we can rely on the result count since we know that permission names are enforced as unique?

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 yup exactly

{
val matchedPermissions = permissionRepository.findByNameIn(permissionsToCheck)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this case sensitive?

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 will be


if (matchedPermissions.size != permissionsToCheck.size)
{
throw PackitException("invalidPermissionsProvided", HttpStatus.BAD_REQUEST)
}
return matchedPermissions
}
}
25 changes: 18 additions & 7 deletions api/app/src/main/kotlin/packit/service/RoleService.kt
Original file line number Diff line number Diff line change
Expand Up @@ -5,22 +5,25 @@ import org.springframework.security.core.GrantedAuthority
import org.springframework.security.core.authority.SimpleGrantedAuthority
import org.springframework.stereotype.Service
import packit.exceptions.PackitException
import packit.model.Permission
import packit.model.Role
import packit.model.RolePermission
import packit.model.dto.CreateRole
import packit.repository.RoleRepository

interface RoleService
{
fun getUsernameRole(username: String): Role
fun getAdminRole(): Role
fun saveRole(roleName: String)
fun checkMatchingRoles(rolesToCheck: List<String>): List<Role>
fun getGrantedAuthorities(roles: List<Role>): MutableList<GrantedAuthority>
fun createRole(createRole: CreateRole)
}

@Service
class BaseRoleService(
private val roleRepository: RoleRepository
private val roleRepository: RoleRepository,
private val permissionService: PermissionService
) : RoleService
{
override fun getUsernameRole(username: String): Role
Expand All @@ -40,26 +43,34 @@ class BaseRoleService(
?: throw PackitException("adminRoleNotFound", HttpStatus.INTERNAL_SERVER_ERROR)
}

override fun saveRole(roleName: String)
override fun createRole(createRole: CreateRole)
{
val permissions = permissionService.checkMatchingPermissions(createRole.permissionNames)

saveRole(createRole.name, permissions)
}

internal fun saveRole(roleName: String, permissions: List<Permission> = listOf())
{
if (roleRepository.existsByName(roleName))
{
throw PackitException("roleAlreadyExists")
}
val role = Role(name = roleName)
role.rolePermissions = permissions.map { RolePermission(permission = it, role = role) }
.toMutableList()
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be toList(), this doesn't need to be mutable. Oh, except it's defined as mutable on the entity type.. maybe it's doesn't need to be mutable there either, seems a bit odd..? Depends on how you're going to do updates I guess.

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 wish but cant because its a mutable list.. needs to be mutable because we will be updating the list (adding/ removing)...

roleRepository.save(role)
}

override fun checkMatchingRoles(rolesToCheck: List<String>): List<Role>
{
val allRoles = roleRepository.findAll()
val foundRoles = rolesToCheck.mapNotNull { name -> allRoles.find { it.name == name } }
val matchedRoles = roleRepository.findByNameIn(rolesToCheck)

if (foundRoles.size != rolesToCheck.size)
if (matchedRoles.size != rolesToCheck.size)
{
throw PackitException("invalidRolesProvided", HttpStatus.BAD_REQUEST)
}
return foundRoles
return matchedRoles
}

/**
Expand Down
2 changes: 1 addition & 1 deletion api/app/src/main/kotlin/packit/service/UserService.kt
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ import org.springframework.http.HttpStatus
import org.springframework.security.crypto.password.PasswordEncoder
import org.springframework.stereotype.Service
import packit.exceptions.PackitException
import packit.model.CreateBasicUser
import packit.model.User
import packit.model.dto.CreateBasicUser
import packit.repository.UserRepository
import java.time.Instant

Expand Down
1 change: 1 addition & 0 deletions api/app/src/main/resources/errorBundle.properties
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ githubLoginDisabled=GitHub login is disabled
basicLoginDisabled=Basic login is disabled
insufficientPrivileges=You do not have sufficient privileges for attempted action
invalidRolesProvided=Invalid roles provided
invalidPermissionsProvided=Invalid permissions provided
userAlreadyExists=User already exists
userNotFound=User not found
roleAlreadyExists=Role already exists
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@ import org.springframework.http.*
import org.springframework.security.crypto.password.PasswordEncoder
import org.springframework.test.context.TestPropertySource
import packit.integration.IntegrationTest
import packit.model.LoginWithPassword
import packit.model.LoginWithToken
import packit.model.User
import packit.model.dto.LoginWithPassword
import packit.model.dto.LoginWithToken
import packit.repository.UserRepository
import kotlin.test.assertEquals

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
package packit.integration.controllers

import com.fasterxml.jackson.databind.ObjectMapper
import org.junit.jupiter.api.Test
import org.springframework.beans.factory.annotation.Autowired
import org.springframework.http.HttpStatus
import org.springframework.test.context.jdbc.Sql
import packit.integration.IntegrationTest
import packit.integration.WithAuthenticatedUser
import packit.model.dto.CreateRole
import packit.repository.RoleRepository
import kotlin.test.assertEquals
import kotlin.test.assertNotNull

@Sql("/delete-test-users.sql", executionPhase = Sql.ExecutionPhase.AFTER_TEST_METHOD)
class RoleControllerTest : IntegrationTest()
{
@Autowired
private lateinit var roleRepository: RoleRepository
private val createTestRoleBody = ObjectMapper().writeValueAsString(
CreateRole(
name = "testRole",
permissionNames = listOf("packet.run", "packet.read")
)
)

@Test
@WithAuthenticatedUser(authorities = ["user.manage"])
fun `users with manage authority can create roles`()
{
val result = restTemplate.postForEntity(
"/role",
getTokenizedHttpEntity(data = createTestRoleBody),
String::class.java
)

assertSuccess(result)
assertNotNull(roleRepository.findByName("testRole"))
}

@Test
@WithAuthenticatedUser(authorities = ["none"])
fun `user without user manage permission cannot create roles`()
{
val result = restTemplate.postForEntity(
"/role",
getTokenizedHttpEntity(data = createTestRoleBody),
String::class.java
)

assertEquals(result.statusCode, HttpStatus.UNAUTHORIZED)
}

@Test
@WithAuthenticatedUser(authorities = ["user.manage"])
fun `reject request if createRole body is invalid`()
{
val result = restTemplate.postForEntity(
"/role",
getTokenizedHttpEntity(data = "{}"),
String::class.java
)

assertEquals(result.statusCode, HttpStatus.BAD_REQUEST)
}
}