1943: Fix permission checks and reuse user checks #1944
Conversation
steffenkleinle
requested review from
f1sh1918,
ztefanie and
seluianova
as code owners
| return newAdminRole != Role.EXTERNAL_VERIFIED_API_USER || transaction { | ||
| actingAdmin.isProject( | ||
| EAK_BAYERN_PROJECT | ||
| ) | ||
| } |
There was a problem hiding this comment.
This check is new, aligns behavior with create user
| actingAdmin: AdministratorEntity, | ||
| existingAdmin: AdministratorEntity | ||
| ): Boolean { | ||
| if (actingAdmin.projectId.value != existingAdmin.projectId.value && existingAdmin.role != Role.NO_RIGHTS.db_value) { |
There was a problem hiding this comment.
Why should we not delete a user if it has no rights?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks for clarifying 👍
circleci-checks
bot
requested review from
andrew8er and
bahaaTuffaha
as code owners
| fun isRole(role: Role, vararg other: Role): Boolean = | ||
| this.role == role.db_value || other.any { this.role == it.db_value } |
There was a problem hiding this comment.
wouldn't hasRole make more sense?
Similarly, I'd prefer isInProject and isInRegion?
Or maybe roleIs, projectIs and regionIs?
| return newAdminRole != Role.EXTERNAL_VERIFIED_API_USER || transaction { | ||
| actingAdmin.isProject( | ||
| EAK_BAYERN_PROJECT | ||
| ) | ||
| } |
There was a problem hiding this comment.
I feel like negating statements, using disjunctions, and packing everything into a return statement makes it harder to read (at least for me).
| 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 |
There was a problem hiding this comment.
Agreed, negating should be avoided if possible. Will adjust accordingly.
| actingAdmin: AdministratorEntity, | ||
| existingAdmin: AdministratorEntity | ||
| ): Boolean { | ||
| if (actingAdmin.projectId.value != existingAdmin.projectId.value && existingAdmin.role != Role.NO_RIGHTS.db_value) { |
There was a problem hiding this comment.
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.
| 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)) | ||
| } | ||
| } |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Agreed. @f1sh1918 do you have any more insights whether we actually have to distinguish between projects here?
There was a problem hiding this comment.
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
Short Description
Create reusable functions for user checks for project, region and role.
Proposed Changes
Side Effects
NO_RIGHTSHopefully no other changes. I ran the tests both on the code before and after and all tests succeed in both versions (exception: changing a role to EXTERNAL_VERIFIED_API_USER in koblenz.
Testing
Test that the user permissions still work.
Resolved Issues
Fixes: #1943