-
Notifications
You must be signed in to change notification settings - Fork 2
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.
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
Support organizational roles #1284
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1284 +/- ##
==========================================
+ Coverage 87.33% 87.51% +0.17%
==========================================
Files 172 182 +10
Lines 2258 2314 +56
Branches 298 305 +7
==========================================
+ Hits 1972 2025 +53
- Misses 261 264 +3
Partials 25 25 ☔ View full report in Codecov by Sentry. |
ad05885
to
8917d62
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was able to lint
, build
, and test:ci
this PR locally, great!
It is not clear to me if this change implements role-based access control or access control lists. In other words, I cannot tell if Changemaker
in ChangemakerRole
is the subject or the object. The word Role
to me connotes subject. If Changemaker
there is truly a subject, then I think the object is missing in the entity. If Changemaker
there is the object, then I think the word Role
is inappropriate. Perhaps ChangeMakerAccessControl
or something like that would be more appropriate.
I think at least one application of the access control and an associated test would help clarify.
8917d62
to
61e46d7
Compare
@bickelj you asked to show an example of using this in code but I think that's premature since there's no way to grant access via API in this PR (I can add it all to one PR but the PR will get much bigger!) That said, one example of an application of this would be that a user with edit access to a foundation would be allowed to create proposal versions for any proposal associated with that foundation's opportunities, or add new proposals to an opportunity. |
@jasonaowen we talked about taking out |
This helps. I thought of another clarifying question. Does a row in the |
Maybe I actually want the group membership thing to be modeled somewhere. That would be one more level of indirection. Right now it looks like User to Changemaker relationships where the relation indicates access to that Changemaker's objects. But nowhere can I declare "this User is a member of that Changemaker" and then bind that Changemaker to have access to objects. What I just mentioned gives up simpler modeling in the software to gain simpler management for customers in the long run. If Groups or Keycloak Roles are granted permissions to objects instead of Users, adding a User to a Group or Role gives that User all the permission needed straightaway. To support my suggestion above, I see this text in the OP for #1250:
|
Agreed this could be (is!) interesting. What do you think of sticking with this simpler model (direct permissions on users) for this PR, but exploring the potential of moving to a more advanced (and Keycloak) driven one as a future iteration? |
Yes, use this as a first increment. I think it can be adapted pretty easily to fit whatever we do in Keycloak. |
61e46d7
to
e136838
Compare
e136838
to
c8c35e3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OpenAPI spec needs an update to use "Permission" language but otherwise skimming over this it looks great!
1c2c7b6
to
8dc8351
Compare
Users in our system can be given permission to three kinds of organization in the PDC (changemakers, funders, and data providers). These associations will allow them access to perform various actions in the context of those organizations. For instance, reading data, writing data, or managing other user associations. This list of abilities may change in future. We explored the concept of `user_roles` with foreign keys to different organization types (similar to the sources table) but decided to have three separate tables because they are slightly distinct concepts. For instance, there will probably be certain access types that only apply to certain types of organization in future. Another design decision was to have the permissions in terms of granted access type rather than higher level role. For instance, instead of roles like "administrator" and "editor" we have action oriented roles like "read" and "manage". This provides more granularity and is also more explicit about what a given role / access type actually allows. Issue #1250 Support associations between users and organizational entities
8dc8351
to
07da57a
Compare
This PR adds internal support for the creation of role / access grants to users for our three organizational entity types.
It does not yet provide endpoints for creation and deletion of those roles, nor does it apply the roles when determining whether or not a user is allowed to perform a given action. These will come in a future PR.
Related to #1250