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 23, 2024
1 parent 1aa26e9 commit ca88e5c
Show file tree
Hide file tree
Showing 15 changed files with 211 additions and 79 deletions.
5 changes: 5 additions & 0 deletions api/app/src/main/kotlin/packit/model/Role.kt
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,11 @@ class Role(
result = 31 * result + isUsername.hashCode()
return result
}

fun isServiceRole(): Boolean
{
return isUsername && users.any { it.isServiceUser() }
}
}

fun Role.toDto() =
Expand Down
4 changes: 3 additions & 1 deletion api/app/src/main/kotlin/packit/model/User.kt
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,9 @@ class User(
val id: UUID? = null,
@OneToMany(mappedBy = "user")
var runInfos: MutableList<RunInfo> = mutableListOf()
)
){
fun isServiceUser(): Boolean = userSource == "service"
}

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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ interface UserRepository : JpaRepository<User, UUID>
fun findByUsername(username: String): User?
fun deleteByEmail(email: String)
fun findByEmail(email: String): User?
fun findByUsernameAndUserSource(username: String, userSource: String): User?
fun existsByUsername(username: String): Boolean
fun existsByEmail(email: String): Boolean
fun findByUsernameIn(usernames: List<String>): List<User>
Expand Down
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
24 changes: 12 additions & 12 deletions api/app/src/main/kotlin/packit/service/RoleService.kt
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,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 getRoleNames(): List<String>
Expand All @@ -43,17 +43,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 @@ -67,6 +56,16 @@ 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)
Expand All @@ -88,6 +87,7 @@ class BaseRoleService(
{
throw PackitException("roleIsNotUsername", HttpStatus.INTERNAL_SERVER_ERROR)
}

roleRepository.deleteByName(username)
}

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,14 +16,15 @@ 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?
fun saveUser(user: User): User
fun saveUsers(users: List<User>): List<User>
fun updatePassword(username: String, updatePassword: UpdatePassword)
fun checkAndUpdateLastLoggedIn(username: String)
fun getServiceUser(): User
}

@Service
Expand All @@ -36,13 +37,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 @@ -67,7 +70,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 @@ -87,9 +90,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 @@ -149,7 +152,17 @@ 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)
}

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,12 @@
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,5 +1,4 @@
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
Expand Down
10 changes: 10 additions & 0 deletions api/app/src/test/kotlin/packit/unit/model/UserTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -38,4 +38,14 @@ class UserTest
assertEquals(user.email, userDto.email)
assertEquals(user.id, userDto.id)
}

@Test
fun `isServiceUser works`()
{
val serviceUser = User("service", disabled = false, userSource = "service", displayName = null)
val basicUser = User("user", disabled = false, userSource = "basic", displayName = null)

assertTrue(serviceUser.isServiceUser())
assertFalse(basicUser.isServiceUser())
}
}
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 @@ -41,23 +41,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 `updateUserRoles 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 ca88e5c

Please sign in to comment.