Fix assignment policies and authorize assignment views #18791
Conversation
krauselukas
force-pushed
the
fix/show_assignment_option_authorization
branch
from
f92bd5e to
e6026eb
Compare
hennevogel
left a comment
hennevogel
left a comment
There was a problem hiding this comment.
I don't know why you invent a new term "collaborator" for this. What is this supposed to be and why don't you take the "role" part of relationship into account?
krauselukas
force-pushed
the
fix/show_assignment_option_authorization
branch
from
e6026eb to
646803e
Compare
I changed the wording "collaborator" everywhere now, I use the words "roles" and "relationship" now. |
krauselukas
force-pushed
the
fix/show_assignment_option_authorization
branch
from
646803e to
18e11fc
Compare
| return false unless PackagePolicy.new(user, record.package).assign? | ||
|
|
||
| record.assignee_is_a_collaborator? | ||
| record.assignee_has_required_role_to_be_assigned? |
There was a problem hiding this comment.
Why do you run this validation here?
There was a problem hiding this comment.
It was already like that, I just adapted then naming and logic inside to be correct. Didn't wanted to go further into refactors.
There was a problem hiding this comment.
we have to check on the authorization level for this as well, so in this term its correct. If it is about reusing the validation code here I would prefer to create a follow up for this. I already changed more in this PR as I originally planned :)
There was a problem hiding this comment.
we have to check on the authorization level for this as well
Why?
There was a problem hiding this comment.
okay, the validation is no longer used in the policy and only run on model level
krauselukas
force-pushed
the
fix/show_assignment_option_authorization
branch
from
18e11fc to
7c179e3
Compare
krauselukas
force-pushed
the
fix/show_assignment_option_authorization
branch
4 times, most recently
from
9757b4a to
1dcae87
Compare
| def assigneer_has_role_to_assign? | ||
| roles = Role.where(title: %w[maintainer bugowner reviewer]) | ||
|
|
||
| (record.package.relationships.where(role_id: roles.ids, user_id: user) + record.package.project.relationships.where(role_id: roles.ids, user_id: user)).any? | ||
| end |
There was a problem hiding this comment.
No need to put those two lines in a method...
The policy doesn't check if the assigner is authorized to add an assignee to a package. As of now everyone can add an assigne. We have to check if the assigner has the required role on the package or project. Also, we can drop checking for the assignee here, this is done through a validation on the model level. No need to do it here as well.
Right now we show the assignment option on a package to every user. We should only show it if the user is actually authorized to assign someone.
Only users with the role maintainer, bugowner and reviewer should be able to be assigned to a package. Right now every role is valid since we just checking, if some sort of relationship exists.
We don't use the word "collaborator" anywhere else in the codebase, this term is newly invented and can be confusing. Let's stick to the term 'roles' and 'relationships'.
…e method The validation is not longer used outside of the model.
krauselukas
force-pushed
the
fix/show_assignment_option_authorization
branch
from
1dcae87 to
286d95d
Compare
4 participants
Uh oh!
There was an error while loading. Please reload this page.