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.

Sign up for GitHub

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

1943: Fix permission checks and reuse user checks #1944

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

1943: Fix permission checks and reuse user checks #1944

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 11 additions & 0 deletions backend/src/main/kotlin/app/ehrenamtskarte/backend/auth/database/Schema.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package app.ehrenamtskarte.backend.auth.database

import app.ehrenamtskarte.backend.auth.webservice.schema.types.Role
import app.ehrenamtskarte.backend.projects.database.ProjectEntity
import app.ehrenamtskarte.backend.projects.database.Projects
import app.ehrenamtskarte.backend.regions.database.Regions
import org.jetbrains.exposed.dao.IntEntity
Expand Down Expand Up @@ -54,6 +55,16 @@ class AdministratorEntity(id: EntityID<Int>) : IntEntity(id) {
var notificationOnApplication by Administrators.notificationOnApplication
var notificationOnVerification by Administrators.notificationOnVerification
var deleted by Administrators.deleted

fun isProject(project: String): Boolean =
this.projectId == ProjectEntity.find { Projects.project eq project }.single().id

fun isProject(projectId: Int): Boolean = this.projectId.value == projectId

fun isRole(role: Role, vararg other: Role): Boolean =
this.role == role.db_value || other.any { this.role == it.db_value }
Comment on lines +64 to +65
Copy link
Member

@michael-markl michael-markl Feb 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

wouldn't hasRole make more sense?
Similarly, I'd prefer isInProject and isInRegion?

Or maybe roleIs, projectIs and regionIs?


fun isRegion(regionId: Int): Boolean = this.regionId?.value == regionId
}

const val TOKEN_LENGTH = 60
Expand Down
2 changes: 1 addition & 1 deletion ...rc/main/kotlin/app/ehrenamtskarte/backend/auth/database/repos/AdministratorsRepository.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ object AdministratorsRepository {
fun deleteAdministrator(administrator: AdministratorEntity) {
administrator.deleted = true
administrator.email = UUID.randomUUID().toString() + "@entitlementcard.app"
administrator.role = Role.NO_RIGHTS.db_value
administrator.isRole(Role.NO_RIGHTS)
}

fun setNewPasswordResetKey(administrator: AdministratorEntity): String {
Expand Down
139 changes: 44 additions & 95 deletions backend/src/main/kotlin/app/ehrenamtskarte/backend/auth/service/Authorizer.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,74 +4,54 @@
import app.ehrenamtskarte.backend.auth.webservice.schema.types.Role
import app.ehrenamtskarte.backend.common.webservice.EAK_BAYERN_PROJECT
import app.ehrenamtskarte.backend.common.webservice.KOBLENZ_PASS_PROJECT
import app.ehrenamtskarte.backend.projects.database.ProjectEntity
import app.ehrenamtskarte.backend.projects.database.Projects
import app.ehrenamtskarte.backend.regions.database.RegionEntity
import org.jetbrains.exposed.sql.transactions.transaction

object Authorizer {

fun mayCreateCardInRegion(user: AdministratorEntity, regionId: Int): Boolean {
return user.regionId?.value == regionId && user.role in setOf(
Role.REGION_MANAGER.db_value,
Role.REGION_ADMIN.db_value
)
return user.isRegion(regionId) && user.isRole(Role.REGION_MANAGER, Role.REGION_ADMIN)
}

fun mayDeleteCardInRegion(user: AdministratorEntity, regionId: Int): Boolean {
return user.regionId?.value == regionId && user.role in setOf(
Role.REGION_MANAGER.db_value,
Role.REGION_ADMIN.db_value
)
return user.isRegion(regionId) && user.isRole(Role.REGION_MANAGER, Role.REGION_ADMIN)
}

fun mayViewApplicationsInRegion(user: AdministratorEntity, regionId: Int): Boolean {
return user.regionId?.value == regionId && user.role in setOf(
Role.REGION_MANAGER.db_value,
Role.REGION_ADMIN.db_value
)
return user.isRegion(regionId) && user.isRole(Role.REGION_MANAGER, Role.REGION_ADMIN)
}

fun mayUpdateApplicationsInRegion(user: AdministratorEntity, regionId: Int): Boolean {
return user.regionId?.value == regionId && user.role in setOf(
Role.REGION_MANAGER.db_value,
Role.REGION_ADMIN.db_value
)
return user.isRegion(regionId) && user.isRole(Role.REGION_MANAGER, Role.REGION_ADMIN)
}

fun mayDeleteApplicationsInRegion(user: AdministratorEntity, regionId: Int): Boolean {
return user.regionId?.value == regionId && user.role in setOf(
Role.REGION_MANAGER.db_value,
Role.REGION_ADMIN.db_value
)
return user.isRegion(regionId) && user.isRole(Role.REGION_MANAGER, Role.REGION_ADMIN)
}

fun mayUpdateSettingsInRegion(user: AdministratorEntity, regionId: Int): Boolean {
return user.regionId?.value == regionId && user.role == Role.REGION_ADMIN.db_value
return user.isRegion(regionId) && user.isRole(Role.REGION_ADMIN)
}

fun mayViewUsersInProject(user: AdministratorEntity?, projectId: Int): Boolean {
return user?.projectId?.value == projectId && user.role == Role.PROJECT_ADMIN.db_value
fun mayViewUsersInProject(user: AdministratorEntity, projectId: Int): Boolean {
return user.isProject(projectId) && user.isRole(Role.PROJECT_ADMIN)
}

fun mayViewUsersInRegion(user: AdministratorEntity?, region: RegionEntity): Boolean {
return (user?.role == Role.REGION_ADMIN.db_value && user.regionId == region.id) ||
mayViewUsersInProject(user, region.projectId.value)
fun mayViewUsersInRegion(user: AdministratorEntity, region: RegionEntity): Boolean {
return mayViewUsersInProject(user, region.projectId.value) ||
(user.isRole(Role.REGION_ADMIN) && user.isRegion(region.id.value))
}

fun maySendMailsInRegion(user: AdministratorEntity, regionId: Int): Boolean {
return user.regionId?.value == regionId && user.role in setOf(
Role.REGION_MANAGER.db_value,
Role.REGION_ADMIN.db_value
)
return user.isRegion(regionId) && user.isRole(Role.REGION_MANAGER, Role.REGION_ADMIN)
}

fun mayViewCardStatisticsInProject(user: AdministratorEntity?, projectId: Int): Boolean {
return user?.projectId?.value == projectId && user.role == Role.PROJECT_ADMIN.db_value
fun mayViewCardStatisticsInProject(user: AdministratorEntity, projectId: Int): Boolean {
return user.isProject(projectId) && user.isRole(Role.PROJECT_ADMIN)
}

fun mayViewCardStatisticsInRegion(user: AdministratorEntity?, region: RegionEntity): Boolean {
return user?.role == Role.REGION_ADMIN.db_value && user.regionId == region.id
fun mayViewCardStatisticsInRegion(user: AdministratorEntity, regionId: Int): Boolean {
return user.isRole(Role.REGION_ADMIN) && user.isRegion(regionId)
}

fun mayCreateUser(
Expand All @@ -80,30 +60,19 @@
newAdminRole: Role,
newAdminRegion: RegionEntity?
): Boolean {
if (actingAdmin.projectId.value != newAdminProjectId) {
return false
} else if (newAdminRole == Role.NO_RIGHTS) {
if (actingAdmin.projectId.value != newAdminProjectId || newAdminRole == Role.NO_RIGHTS) {
return false
}

if (actingAdmin.role == Role.PROJECT_ADMIN.db_value && newAdminRole == Role.EXTERNAL_VERIFIED_API_USER) {
return transaction {
ProjectEntity.find { Projects.project eq EAK_BAYERN_PROJECT }
.single().id.value == actingAdmin.projectId.value
if (actingAdmin.isRole(Role.PROJECT_ADMIN)) {
return newAdminRole != Role.EXTERNAL_VERIFIED_API_USER || transaction {
actingAdmin.isProject(
EAK_BAYERN_PROJECT
)
}
Comment on lines +67 to 71
Copy link
Member

@michael-markl michael-markl Feb 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I feel like negating statements, using disjunctions, and packing everything into a return statement makes it harder to read (at least for me).

Suggested change
return newAdminRole != Role.EXTERNAL_VERIFIED_API_USER || transaction {
actingAdmin.isProject(
EAK_BAYERN_PROJECT
)
}
if (newAdminRole == Role.EXTERNAL_VERIFIED_API_USER && !transaction { actingAdmin.isProject(EAK_BAYERN_PROJECT) }) {
// Currently, we only allow external API users for the EAK Bayern project.
return false
}
return true

Copy link
Member Author

@steffenkleinle steffenkleinle Mar 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Agreed, negating should be avoided if possible. Will adjust accordingly.

}

if (actingAdmin.role == Role.PROJECT_ADMIN.db_value) {
return true
} else if (
actingAdmin.role == Role.REGION_ADMIN.db_value &&
return actingAdmin.isRole(Role.REGION_ADMIN) &&
newAdminRegion != null && actingAdmin.regionId == newAdminRegion.id &&
newAdminRole in setOf(Role.REGION_ADMIN, Role.REGION_MANAGER)

Check warning on line 75 in backend/src/main/kotlin/app/ehrenamtskarte/backend/auth/service/Authorizer.kt

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (1762-add-agency-id-to-region)

❌ New issue: Code Duplication 

The module contains 2 functions with similar structure: Authorizer.mayCreateUser,Authorizer.mayEditUser. Avoid duplicated, aka copy-pasted, code inside the module. More duplication lowers the code health.

Check notice on line 75 in backend/src/main/kotlin/app/ehrenamtskarte/backend/auth/service/Authorizer.kt

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (1762-add-agency-id-to-region)

✅ No longer an issue: Complex Method 

Authorizer.mayCreateUser is no longer above the threshold for cyclomatic complexity. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
) {
return true
}

return false
}

fun mayEditUser(
Expand All @@ -113,74 +82,54 @@
newAdminRole: Role,
newAdminRegion: RegionEntity?
): Boolean {
if (actingAdmin.projectId.value != newAdminProjectId || existingAdmin.projectId.value != newAdminProjectId) {
return false
} else if (newAdminRole == Role.NO_RIGHTS) {
if (!actingAdmin.isProject(newAdminProjectId) || !existingAdmin.isProject(newAdminProjectId) || newAdminRole == Role.NO_RIGHTS) {

Check warning on line 85 in backend/src/main/kotlin/app/ehrenamtskarte/backend/auth/service/Authorizer.kt

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (1762-add-agency-id-to-region)

❌ New issue: Complex Conditional 

Authorizer.mayEditUser has 1 complex conditionals with 2 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
return false
}

if (actingAdmin.role == Role.PROJECT_ADMIN.db_value) {
return true
} else if (
actingAdmin.role == Role.REGION_ADMIN.db_value &&
if (actingAdmin.isRole(Role.PROJECT_ADMIN)) {
return newAdminRole != Role.EXTERNAL_VERIFIED_API_USER || transaction {
actingAdmin.isProject(
EAK_BAYERN_PROJECT
)
}
Comment on lines +89 to +93
Copy link
Member Author

Choose a reason for hiding this comment

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

This check is new, aligns behavior with create user

}
return actingAdmin.isRole(Role.REGION_ADMIN) &&
existingAdmin.regionId == actingAdmin.regionId &&
newAdminRegion != null && actingAdmin.regionId == newAdminRegion.id &&
newAdminRole in setOf(Role.REGION_ADMIN, Role.REGION_MANAGER)
) {
return true
}
return false
}

fun mayDeleteUser(
actingAdmin: AdministratorEntity,
existingAdmin: AdministratorEntity
): Boolean {
if (actingAdmin.projectId.value != existingAdmin.projectId.value && existingAdmin.role != Role.NO_RIGHTS.db_value) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Why should we not delete a user if it has no rights?

Copy link
Member

Choose a reason for hiding this comment

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

As far as I know, a user has the NO_RIGHTS if and only if the user is deleted. (This might have changed though).
I think, it does not matter too much, whether we allow a deleted user to be deleted again.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for clarifying 👍

fun mayDeleteUser(actingAdmin: AdministratorEntity, existingAdmin: AdministratorEntity): Boolean {
if (actingAdmin.projectId != existingAdmin.projectId) {
return false
}

if (actingAdmin.role == Role.PROJECT_ADMIN.db_value) {
return true
} else if (actingAdmin.role == Role.REGION_ADMIN.db_value && existingAdmin.regionId == actingAdmin.regionId) {
if (actingAdmin.isRole(Role.PROJECT_ADMIN)) {
return true
}
return false
return actingAdmin.isRole(Role.REGION_ADMIN) && existingAdmin.regionId == actingAdmin.regionId
}

fun mayUpdateStoresInProject(user: AdministratorEntity, projectId: Int): Boolean {
return user.projectId.value == projectId && user.role == Role.PROJECT_STORE_MANAGER.db_value
return user.projectId.value == projectId && user.isRole(Role.PROJECT_STORE_MANAGER)
}

fun mayViewFreinetAgencyInformationInRegion(user: AdministratorEntity, regionId: Int): Boolean {
return user.role == Role.REGION_ADMIN.db_value && ProjectEntity.find { Projects.project eq EAK_BAYERN_PROJECT }
.single().id.value == user.projectId.value && user.regionId?.value == regionId
return user.isRole(Role.REGION_ADMIN) && user.isProject(EAK_BAYERN_PROJECT) && user.isRegion(regionId)
}

fun mayAddApiTokensInProject(user: AdministratorEntity): Boolean {
return transaction {
(
user.role == Role.PROJECT_ADMIN.db_value && ProjectEntity.find { Projects.project eq KOBLENZ_PASS_PROJECT }
.single().id.value == user.projectId.value
) ||
(
user.role == Role.EXTERNAL_VERIFIED_API_USER.db_value && ProjectEntity.find { Projects.project eq EAK_BAYERN_PROJECT }
.single().id.value == user.projectId.value
)
(user.isRole(Role.PROJECT_ADMIN) && user.isProject(KOBLENZ_PASS_PROJECT)) ||
(user.isRole(Role.EXTERNAL_VERIFIED_API_USER) && user.isProject(EAK_BAYERN_PROJECT))
}
}
Comment on lines 119 to 124
Copy link
Member

Choose a reason for hiding this comment

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

This is wild :D (not your changes, but the fact that we actually make such distinctions between projects for the roles. I feel like this is going to get confusing very quickly, when we have more than 3 projects)

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. @f1sh1918 do you have any more insights whether we actually have to distinguish between projects here?

Copy link
Contributor

@f1sh1918 f1sh1918 Mar 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes i'm also not super happy with this implementation. I think the main aspect was not to create a separate role for that because it would be more effort. Which we in the end did for verein360 since we had to ensure that they don't get project admin rights as a third party provider, but that wasn't planned so far when we implemented that for koblenz..

I think it would be easier and cleaner to also use this separate role for koblenz (like for eak)
The problem is that we soon go live with koblenz and we should not break the current implementation.

My suggestion would be to add the ability for koblenz to create EXTERNAL_VERIFIED_API_USER and allow them to use the particular endpoint (entitleduserdata). Then give them the possibility to create a user and a token with the role and adjust their import so everything still works.
If that was done i would remove the token creation from the koblenz project admin role.

As a third step it would probably make sense to add RBAC and also be able to have different roles as a user so someone could be Project_Admin and External_API_User


fun mayViewApiMetadataInProject(user: AdministratorEntity): Boolean =
user.role == Role.PROJECT_ADMIN.db_value || user.role == Role.EXTERNAL_VERIFIED_API_USER.db_value
user.isRole(Role.PROJECT_ADMIN) || user.isRole(Role.EXTERNAL_VERIFIED_API_USER)

fun mayDeleteApiTokensInProject(user: AdministratorEntity): Boolean =
user.role == Role.PROJECT_ADMIN.db_value || user.role == Role.EXTERNAL_VERIFIED_API_USER.db_value
user.isRole(Role.PROJECT_ADMIN) || user.isRole(Role.EXTERNAL_VERIFIED_API_USER)

fun maySeeHashingPepper(user: AdministratorEntity): Boolean {
return transaction {
ProjectEntity.find { Projects.project eq KOBLENZ_PASS_PROJECT }
.single().id.value == user.projectId.value &&
user.role == Role.PROJECT_ADMIN.db_value
}
fun mayViewHashingPepper(user: AdministratorEntity): Boolean {
return transaction { user.isProject(KOBLENZ_PASS_PROJECT) && user.isRole(Role.PROJECT_ADMIN) }
}
}
4 changes: 2 additions & 2 deletions ...otlin/app/ehrenamtskarte/backend/auth/webservice/schema/ViewAdministratorsQueryService.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ class ViewAdministratorsQueryService {
val projectEntity = ProjectEntity.find { Projects.project eq project }.firstOrNull()
?: throw ProjectNotFoundException(project)
val projectId = projectEntity.id.value
if (!Authorizer.mayViewUsersInProject(admin, projectId)) {
if (admin == null || !Authorizer.mayViewUsersInProject(admin, projectId)) {
throw ForbiddenException()
}
val administrators = (Administrators leftJoin Regions)
Expand All @@ -76,7 +76,7 @@ class ViewAdministratorsQueryService {
return transaction {
val admin = AdministratorEntity.findById(jwtPayload.adminId)
val region = RegionEntity.findById(regionId) ?: throw RegionNotFoundException()
if (!Authorizer.mayViewUsersInRegion(admin, region)) {
if (admin == null || !Authorizer.mayViewUsersInRegion(admin, region)) {
throw ForbiddenException()
}
val administrators = AdministratorEntity
Expand Down
2 changes: 1 addition & 1 deletion ...c/main/kotlin/app/ehrenamtskarte/backend/auth/webservice/schema/ViewPepperQueryService.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ class ViewPepperQueryService {

return transaction {
val admin = AdministratorEntity.findById(jwtPayload.adminId) ?: throw UnauthorizedException()
if (!Authorizer.maySeeHashingPepper(admin)) {
if (!Authorizer.mayViewHashingPepper(admin)) {
throw ForbiddenException()
}
Environment.getVariable(KOBLENZ_PEPPER_SYS_ENV) ?: throw NotImplementedException("Koblenz pepper is not set properly in this environment")
Expand Down
4 changes: 2 additions & 2 deletions ...n/kotlin/app/ehrenamtskarte/backend/cards/webservice/schema/CardStatisticsQueryService.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ class CardStatisticsQueryService {
?: throw ProjectNotFoundException(project)
val projectConfig = context.backendConfiguration.getProjectConfig(project)
val projectId = projectEntity.id.value
if (!Authorizer.mayViewCardStatisticsInProject(admin, projectId)) {
if (admin == null || !Authorizer.mayViewCardStatisticsInProject(admin, projectId)) {
throw ForbiddenException()
}

Expand Down Expand Up @@ -65,7 +65,7 @@ class CardStatisticsQueryService {
val projectConfig = context.backendConfiguration.getProjectConfig(project)
val region = RegionEntity.findById(regionId) ?: throw RegionNotFoundException()

if (!Authorizer.mayViewCardStatisticsInRegion(admin, region)) {
if (admin == null || !Authorizer.mayViewCardStatisticsInRegion(admin, region.id.value)) {
throw ForbiddenException()
}

Expand Down
Loading