-
Notifications
You must be signed in to change notification settings - Fork 41
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
Add authorization relations in proto #2218
Conversation
FWIW, on first thought, I'm more keen on the approached proposed by this PR than the alternative documented in the description |
Giving this some thought, I do like the proposed approach more than the described alternative. Explicit is better than implicit and it seems to me like it's less error-prone. This will also help us handle some edge-cases such as the profile status (which needs a dedicated relation IMO). |
0490628
to
766ad95
Compare
cfc49e3
to
63549f1
Compare
63549f1
to
3bb279b
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.
awesome! great work!
ObjectOwner auth_scope = 5; | ||
TargetResource target_resource = 6; | ||
Relation relation = 7; | ||
reserved 1, 5; // deprecated anonymous and auth_scope |
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.
Not sure if we also want to reserve the names -- since these options don't show up much on the wire proto and don't mean anything to clients, I'm okay with being faster & looser here.
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.
Yeah I was wondering if it's even worth reserving the field numbers here, since it's not something the clients use.
bool owner_only = 3; | ||
bool root_admin_only = 4; |
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.
These are temporary until we complete the OpenFGA switchover, right?
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.
Yes, that's right
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'm fairly sure they don't serve any purpose now either, since currently users are always owners of their single org. They can likely be removed even before the openfga switchover.
In this approach we enumerate all of the roles a user can have with an entity (in this case only project).
If our API evolves to have finer grained authorization, then we can add values to
ObjectOwner
to describe the entity type we are requesting authorization for.For example, if we add a relation where a user can "create" a repo, then we can add an
OBJECT_OWNER_REPO
enum value toObjectOwner
and reuse theRelation.CREATE
value.Alternatives considered
Only specifying the resource type we are requesting access to in the rpc definition and deriving the relation, either by:
List
then it's aget
relation)Both of these seem fragile, because they rely on us following certain conventions. In the case of something as important as authorization, I would rather be explicit, even if it's verbose.