feat(admin): implement basic admin user roles #3522
Conversation
snuba/admin/auth_scopes.py
Outdated
| MEMBER_READ = "member_read" | ||
|
|
||
|
|
||
| ADMIN_CATEGORY_RESOURCES = { |
There was a problem hiding this comment.
Would it be possible/worth it to generate this list automatically from the migration groups in code?
There was a problem hiding this comment.
+1, we should not have to add a new scope in the admin tool code each time we add a new migration group. That is something product teams do.
|
Lgtm though I think @dbanda may want to take a look as I saw some offline conversations about this. Where will the user -> group -> role -> category -> resource mappings be kept? Can you give a rough idea of how you imagine this configuration would look/be managed? |
snuba/admin/auth_scopes.py
Outdated
| MEMBER_READ = "member_read" | ||
|
|
||
|
|
||
| ADMIN_CATEGORY_RESOURCES = { |
There was a problem hiding this comment.
+1, we should not have to add a new scope in the admin tool code each time we add a new migration group. That is something product teams do.
snuba/admin/auth_scopes.py
Outdated
| class AuthScope: | ||
| category: str | ||
| resource: str | ||
| role: AdminRole | ||
|
|
||
| def to_str(self) -> str: | ||
| return f"{self.category}.{self.resource}.{self.role.value}" |
There was a problem hiding this comment.
I am a bit confused about this model.
As I mentioned on the notion doc, most role based access system have at least three entities:
- resources
- actions
- principals / roles
There generally is a relationship of ownership between a role, and action and a resource so that a role is granted the permission to execute the action on a resource.
Then you assign roles to a person.
Here I think the resource is composed by the pair category + resource. Is this correct ?
I cannot see the action in this model.
Also does the scope represent the relationship between the three ?
Since these concepts in role based access control are generally familiar to developers I would suggest we stick to them without introducing new ones. That makes it much easier for somebody who does not know the snuba admin to pick them up without having to understand the custom model.
snuba/admin/auth_scopes.py
Outdated
| return scopes | ||
|
|
||
|
|
||
| ADMIN_SCOPES = scopes() |
There was a problem hiding this comment.
This looks like it will generate all permutations of scopes for each resource and admin role. Is there a reason why this is done? It looks like the only place ADMIN_SCOPES is used is in _set_scopes() which will assign all these scopes to the user. Was that intentional?
MeredithAnya
force-pushed
the
meredith/admin-roles
branch
from
d8cbe6d
to
b11de42
Compare
Codecov ReportBase: 92.19% // Head: 92.46% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #3522 +/- ##
==========================================
+ Coverage 92.19% 92.46% +0.27%
==========================================
Files 726 734 +8
Lines 33790 34019 +229
==========================================
+ Hits 31152 31456 +304
+ Misses 2638 2563 -75
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
snuba/admin/auth_roles.py
Outdated
|
|
||
| class Action(Enum): | ||
| EXECUTE = "execute" | ||
| LIMITED_EXECUTE = "limited_execute" |
There was a problem hiding this comment.
Have you considered calling the actions in the same way as the policy ?
MeredithAnya
changed the title
| pass | ||
|
|
||
|
|
||
| # todo, shoudln't need .keys() once ADMIN_ALLOWED_MIGRATION_GROUPS is a set not dict |
There was a problem hiding this comment.
| # todo, shoudln't need .keys() once ADMIN_ALLOWED_MIGRATION_GROUPS is a set not dict | |
| # todo, `shouldn't` need .keys() once ADMIN_ALLOWED_MIGRATION_GROUPS is a set not dict |
context:
The access to the snuba admin tool right now for SaaS is determined via google IAP which is great for all or nothing access but doesn't really allow for more granular permissions. The reason that we want more granular permissions is so that we can open up parts admin tooling to more folks (outside the SNS team) without giving access to everything
Roles
I've added the concept of roles to the admin user. A role has two properties:
name- the name of the roleactions- the actions a user is allowed to take on specified resources, for migrations this would be the actions they could take on a migration(s) within a migration groupActions
Actions correspond to specific resources, instead of generic actions that apply to all resources. Custom actions can be implemented for each
category(e.g. "migrations" resource has theMigrationsAction) and those can be further split up in specific actions within a category[EDIT]: Instead of scopes, the actions within a role map to a policy. e.g.
ExecuteNonBlockingActionwould correspond to theNonBlockingMigrationsPolicy.Scopes will be used to map to migration polices - this PR https://github.com/getsentry/snuba/pull/3543/files builds off of this one to do just that.This PR adds the roles to the user but doesn't do any checking of the roles. This will come later when implementing this for migrations.