Skip to content
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

Merged
merged 1 commit into from
Jan 30, 2024

Conversation

eleftherias
Copy link
Contributor

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 to ObjectOwner and reuse the Relation.CREATE value.

Alternatives considered

Only specifying the resource type we are requesting access to in the rpc definition and deriving the relation, either by:

  • Getting the HTTP verb and mapping it to the relation (e.g get -> get, post -> create)
  • Using our naming convention to derive the relation (e.g if the method starts with List then it's a get relation)
rpc ListArtifacts (ListArtifactsRequest) returns (ListArtifactsResponse) {
    option (google.api.http) = {
        get: "/api/v1/artifacts/{provider}"
        additional_bindings {
            get: "/api/v1/artifacts"
        }
    };

    option (rpc_options) = {
        auth_scope: OBJECT_OWNER_PROJECT
        resource: ARTIFACT
    };
}

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.

@JAORMX
Copy link
Contributor

JAORMX commented Jan 29, 2024

FWIW, on first thought, I'm more keen on the approached proposed by this PR than the alternative documented in the description

@JAORMX
Copy link
Contributor

JAORMX commented Jan 29, 2024

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).

@eleftherias eleftherias changed the title WIP: authorization relations in proto Add authorization relations in proto Jan 29, 2024
@eleftherias eleftherias marked this pull request as ready for review January 29, 2024 13:57
@eleftherias eleftherias requested a review from a team as a code owner January 29, 2024 13:57
@eleftherias eleftherias force-pushed the relation-proto branch 3 times, most recently from cfc49e3 to 63549f1 Compare January 29, 2024 15:43
Copy link
Contributor

@JAORMX JAORMX left a 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
Copy link
Member

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.

Copy link
Contributor Author

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.

Comment on lines 85 to 86
bool owner_only = 3;
bool root_admin_only = 4;
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's right

Copy link
Contributor Author

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.

@eleftherias eleftherias merged commit 5ffa060 into mindersec:main Jan 30, 2024
19 checks passed
@eleftherias eleftherias deleted the relation-proto branch January 30, 2024 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants