Skip to content

Commit

Permalink
Add a service account.
Browse files Browse the repository at this point in the history
As part of adding support for authentication from OIDC tokens for GitHub
actions, we need a user that we can assign to the tokens we issue. This
adds a new user source, called "service", and populates the database
with one of these accounts.

The service user is hidden from the API, and therefore is not visible in
the control panel. Moreover, it cannot be given additional roles and
permissions.

The service user has the ADMIN role, but in practice when issuing the
access tokens we will restrict the actual scopes given to each token. As
a precaution, I added some checks to make sure the different user
sources cannot use users created by another.
  • Loading branch information
plietar committed Sep 20, 2024
1 parent 3af2f5a commit 4652b88
Show file tree
Hide file tree
Showing 14 changed files with 221 additions and 82 deletions.
1 change: 1 addition & 0 deletions api/app/src/main/kotlin/packit/model/Role.kt
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ class Role(
return result
}

fun isAdminRole(): Boolean = name == "ADMIN"
fun isHidden(): Boolean = isUsername
}

Expand Down
9 changes: 6 additions & 3 deletions api/app/src/main/kotlin/packit/model/User.kt
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,10 @@ class User(
)
@Filter(name = "isUsername", condition = "isUsername = TRUE")
var userRole: Role? = null,
)
){
fun isServiceUser(): Boolean = userSource == "service"
fun isHidden(): Boolean = isServiceUser()
}

fun User.toBasicDto() = BasicUserDto(username, id!!)

Expand All @@ -55,5 +58,5 @@ fun User.toDto() =
userRole?.rolePermissions?.map { it.toDto() } ?: listOf()
)

fun List<User>.toDto() = sortedBy { it.username }.map { it.toDto() }
fun List<User>.toBasicDto() = sortedBy { it.username }.map { it.toBasicDto() }
fun List<User>.toDto() = filter { !it.isHidden() }.sortedBy { it.username }.map { it.toDto() }
fun List<User>.toBasicDto() = filter { !it.isHidden() }.sortedBy { it.username }.map { it.toBasicDto() }
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import java.util.*
interface UserRepository : JpaRepository<User, UUID>
{
fun findByUsername(username: String): User?
fun findByUsernameAndUserSource(username: String, userSource: String): User?
fun findByUsernameIn(usernames: List<String>): List<User>
fun existsByUsername(username: String): Boolean
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ class BasicUserDetailsService(
{
override fun loadUserByUsername(username: String): UserDetails
{
val user = userService.getUserForLogin(username)
val user = userService.getUserForBasicLogin(username)
return BasicUserDetails(
UserPrincipal(
user.username,
Expand Down
25 changes: 12 additions & 13 deletions api/app/src/main/kotlin/packit/service/RoleService.kt
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,10 @@ import packit.repository.RoleRepository

interface RoleService
{
fun getUsernameRole(username: String): Role
fun getAdminRole(): Role
fun getGrantedAuthorities(roles: List<Role>): MutableSet<GrantedAuthority>
fun createRole(createRole: CreateRole): Role
fun createUsernameRole(username: String): Role
fun deleteRole(roleName: String)
fun deleteUsernameRole(username: String)
fun getRolesByRoleNames(roleNames: List<String>): List<Role>
Expand All @@ -38,17 +38,6 @@ class BaseRoleService(
private val rolePermissionService: RolePermissionService,
) : RoleService
{
override fun getUsernameRole(username: String): Role
{
val userRole = roleRepository.findByName(username)

if (userRole != null)
{
return userRole
}
return roleRepository.save(Role(name = username, isUsername = true))
}

override fun getAdminRole(): Role
{
return roleRepository.findByName("ADMIN")
Expand All @@ -62,12 +51,22 @@ class BaseRoleService(
return saveRole(createRole.name, permissions)
}

override fun createUsernameRole(username: String): Role
{
val userRole = roleRepository.findByName(username)
if (userRole != null)
{
throw PackitException("roleAlreadyExists", HttpStatus.BAD_REQUEST)
}
return roleRepository.save(Role(name = username, isUsername = true))
}

override fun deleteRole(roleName: String)
{
val roleToDelete = roleRepository.findByName(roleName)
?: throw PackitException("roleNotFound", HttpStatus.BAD_REQUEST)

if (roleToDelete.name == "ADMIN" || roleToDelete.isUsername)
if (roleToDelete.isAdminRole() || roleToDelete.isUsername)
{
throw PackitException("cannotDeleteAdminOrUsernameRole", HttpStatus.BAD_REQUEST)
}
Expand Down
11 changes: 10 additions & 1 deletion api/app/src/main/kotlin/packit/service/UserRoleService.kt
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ class BaseUserRoleService(
val user = userService.getByUsername(username)
?: throw PackitException("userNotFound", HttpStatus.NOT_FOUND)

if (user.isServiceUser()) {
throw PackitException("cannotModifyServiceUser", HttpStatus.BAD_REQUEST)
}

val rolesToUpdate = getRolesForUpdate(updateUserRoles.roleNamesToAdd + updateUserRoles.roleNamesToRemove)
addRolesToUser(user, rolesToUpdate.filter { it.name in updateUserRoles.roleNamesToAdd })
removeRolesFromUser(user, rolesToUpdate.filter { it.name in updateUserRoles.roleNamesToRemove })
Expand All @@ -40,13 +44,18 @@ class BaseUserRoleService(
{
throw PackitException("cannotUpdateUsernameRoles", HttpStatus.BAD_REQUEST)
}

val updateUsers =
userService.getUsersByUsernames(usersToUpdate.usernamesToAdd + usersToUpdate.usernamesToRemove)

if (updateUsers.any { it.isServiceUser() }) {
throw PackitException("cannotModifyServiceUser", HttpStatus.BAD_REQUEST)
}

addUsersToRole(role, updateUsers.filter { it.username in usersToUpdate.usernamesToAdd })
removeUsersFromRole(role, updateUsers.filter { it.username in usersToUpdate.usernamesToRemove })

// have to update users to save change as it is owner of many-to-many relationship
// have to update users to save change as it is owner of many-to-many relationship
userService.saveUsers(updateUsers)
return role
}
Expand Down
27 changes: 20 additions & 7 deletions api/app/src/main/kotlin/packit/service/UserService.kt
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ interface UserService
{
fun saveUserFromGithub(username: String, displayName: String?, email: String?): User
fun createBasicUser(createBasicUser: CreateBasicUser): User
fun getUserForLogin(username: String): User
fun getUserForBasicLogin(username: String): User
fun deleteUser(username: String)
fun getUsersByUsernames(usernames: List<String>): List<User>
fun getByUsername(username: String): User?
Expand All @@ -25,6 +25,7 @@ interface UserService
fun updatePassword(username: String, updatePassword: UpdatePassword)
fun checkAndUpdateLastLoggedIn(username: String)
fun getAllUsers(): List<User>
fun getServiceUser(): User
}

@Service
Expand All @@ -37,13 +38,15 @@ class BaseUserService(
override fun saveUserFromGithub(username: String, displayName: String?, email: String?): User
{
val user = userRepository.findByUsername(username)
if (user != null)
{
if (user != null) {
if (user.userSource != "github") {
throw PackitException("userAlreadyExists", HttpStatus.BAD_REQUEST)
}
return updateUserLastLoggedIn(user, Instant.now())
}

val roles = roleService.getDefaultRoles().toMutableList()
roles.add(roleService.getUsernameRole(username))
roles.add(roleService.createUsernameRole(username))

val newUser = User(
username = username,
Expand All @@ -68,7 +71,7 @@ class BaseUserService(

val roles = roleService.getDefaultRoles().toMutableList()
roles.addAll(roleService.getRolesByRoleNames(createBasicUser.userRoles).toMutableList())
roles.add(roleService.getUsernameRole(createBasicUser.email))
roles.add(roleService.createUsernameRole(createBasicUser.email))

val newUser = User(
username = createBasicUser.email,
Expand All @@ -88,9 +91,9 @@ class BaseUserService(
return userRepository.save(user)
}

override fun getUserForLogin(username: String): User
override fun getUserForBasicLogin(username: String): User
{
return userRepository.findByUsername(username) ?: throw AuthenticationException()
return userRepository.findByUsernameAndUserSource(username, "basic") ?: throw AuthenticationException()
}

override fun getUsersByUsernames(usernames: List<String>): List<User>
Expand Down Expand Up @@ -150,6 +153,10 @@ class BaseUserService(
val user = userRepository.findByUsername(username)
?: throw PackitException("userNotFound", HttpStatus.NOT_FOUND)

if (user.userSource == "service") {
throw PackitException("cannotUpdateServiceUser", HttpStatus.BAD_REQUEST)
}

userRepository.delete(user)
roleService.deleteUsernameRole(username)
}
Expand All @@ -158,4 +165,10 @@ class BaseUserService(
{
return userRepository.findAll()
}

override fun getServiceUser(): User
{
return userRepository.findByUsernameAndUserSource("service", "service")
?: throw PackitException("serviceUserNotFound", HttpStatus.INTERNAL_SERVER_ERROR)
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
WITH inserted_user AS (
INSERT INTO "user"("username", "display_name", "user_source")
VALUES ('service', 'Service Account', 'service')
RETURNING id
)
INSERT INTO "user_role"("user_id", "role_id")
SELECT inserted_user.id, role.id
FROM inserted_user
CROSS JOIN "role"
WHERE role.name = 'ADMIN';
2 changes: 2 additions & 0 deletions api/app/src/main/resources/errorBundle.properties
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
adminRoleNotFound=Admin role not found, contact your administrator
serviceUserNotFound=Service user not found, contact your administrator
basicLoginDisabled=Basic login is disabled
cannotUpdateUsernameRoles=Cannot add or remove roles from the username roles
cannotDeleteAdminOrUsernameRole=Cannot delete admin or username role
cannotUpdateServiceUser=Cannot modify or delete service user
updatePassword=You must change your password before logging in
couldNotGetFile=Error occurred while retrieving file
doesNotExist=Resource does not exist
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
package packit.integration.controllers

import com.fasterxml.jackson.core.type.TypeReference
import com.fasterxml.jackson.module.kotlin.jacksonObjectMapper
import org.springframework.beans.factory.annotation.Autowired
import org.springframework.core.ParameterizedTypeReference
import org.springframework.http.HttpMethod
import org.springframework.http.HttpStatus
import org.springframework.test.context.TestPropertySource
Expand Down Expand Up @@ -158,6 +158,28 @@ class UserControllerTest : IntegrationTest()
assertEquals(userRepository.findByUsername(testUser.username), null)
assertEquals(roleRepository.findByName(username), null)
}

@Test
@WithAuthenticatedUser(authorities = ["user.manage"])
fun `getAllUsers excludes service accounts`()
{
userRepository.save(
User(username = "[email protected]", userSource = "service")
)
userRepository.save(
User(username = "[email protected]", userSource = "basic")
)

val result = restTemplate.exchange(
"/user",
HttpMethod.GET,
getTokenizedHttpEntity(),
object : ParameterizedTypeReference<List<UserDto>>() {}
)

val body = assertSuccess(result)
assertEquals(body.map { it.username }, listOf("[email protected]"))
}
}

@Sql("/delete-test-users.sql", executionPhase = Sql.ExecutionPhase.AFTER_TEST_METHOD)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ class BasicUserDetailsServiceTest
@Test
fun `loadUserByUsername returns correct UserDetails for valid username`()
{
whenever(mockUserService.getUserForLogin(mockUser.username)).thenReturn(
whenever(mockUserService.getUserForBasicLogin(mockUser.username)).thenReturn(
mockUser
)
val userDetails: UserDetails = service.loadUserByUsername(mockUser.username)
Expand Down
14 changes: 7 additions & 7 deletions api/app/src/test/kotlin/packit/unit/service/RoleServiceTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -40,23 +40,23 @@ class RoleServiceTest
}

@Test
fun `getUsernameRole returns existing role`()
fun `createUsernameRole throws exception if role exists`()
{
val role = Role(name = "username")
val role = Role(name = "username", isUsername = true)
whenever(roleRepository.findByName("username")).thenReturn(role)

val result = roleService.getUsernameRole("username")

assertEquals(role, result)
assertThrows(PackitException::class.java) {
roleService.createUsernameRole("username")
}
}

@Test
fun `getUsernameRole creates new role with is_username flag if not exists`()
fun `createUsernameRole creates new role with is_username flag`()
{
whenever(roleRepository.findByName("username")).thenReturn(null)
whenever(roleRepository.save(any<Role>())).thenAnswer { it.getArgument(0) }

val result = roleService.getUsernameRole("username")
val result = roleService.createUsernameRole("username")

assertEquals("username", result.name)
assertEquals(true, result.isUsername)
Expand Down
44 changes: 44 additions & 0 deletions api/app/src/test/kotlin/packit/unit/service/UserRoleServiceTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,16 @@ class UserRoleServiceTest
roles = testRoles.toMutableList(),
id = UUID.randomUUID()
)
private val mockServiceUser = User(
username = "service",
displayName = "Service Account",
disabled = false,
email = "email",
userSource = "service",
lastLoggedIn = Instant.parse("2018-12-12T00:00:00Z"),
roles = testRoles.toMutableList(),
id = UUID.randomUUID()
)
private val mockRole = Role("role1", users = mutableListOf(mockUser))
private val mockRoleService = mock<RoleService>()

Expand Down Expand Up @@ -224,4 +234,38 @@ class UserRoleServiceTest
assertEquals(ex.key, "userRoleNotExists")
assertEquals(ex.httpStatus, HttpStatus.BAD_REQUEST)
}

@Test
fun `updateRoleUsers throws exception when adding service account to roles`()
{
whenever(mockRoleService.getByRoleName(mockRole.name)).doReturn(mockRole)
whenever(mockUserService.getUsersByUsernames(listOf(mockServiceUser.username)))
.doReturn(listOf(mockServiceUser))

val service = BaseUserRoleService(mockRoleService, mockUserService)
val ex = assertThrows<PackitException> {
service.updateRoleUsers(mockRole.name, UpdateRoleUsers(usernamesToAdd = listOf(mockServiceUser.username)))
}

assertEquals(ex.key, "cannotModifyServiceUser")
assertEquals(ex.httpStatus, HttpStatus.BAD_REQUEST)
}

@Test
fun `updateRoleUsers throws exception when adding roles to service account`()
{
whenever(mockUserService.getByUsername(mockServiceUser.username)).doReturn(mockServiceUser)
whenever(mockRoleService.getRolesByRoleNames(listOf(mockRole.name))).doReturn(listOf(mockRole))

val service = BaseUserRoleService(mockRoleService, mockUserService)
val ex = assertThrows<PackitException> {
service.updateUserRoles(
mockServiceUser.username,
UpdateUserRoles(roleNamesToAdd = listOf(mockRole.name))
)
}

assertEquals(ex.key, "cannotModifyServiceUser")
assertEquals(ex.httpStatus, HttpStatus.BAD_REQUEST)
}
}
Loading

0 comments on commit 4652b88

Please sign in to comment.