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

RFC 113: Implement PermsFetcher interface for GitHub authz provider #8890

Closed
unknwon opened this issue Mar 9, 2020 · 0 comments · Fixed by #9386
Closed

RFC 113: Implement PermsFetcher interface for GitHub authz provider #8890

unknwon opened this issue Mar 9, 2020 · 0 comments · Fixed by #9386
Assignees
Labels
auth estimate/1d planned/3.15 Issues that were planned for the given milestone. Used by cmd/tracking-issue.
Milestone

Comments

@unknwon
Copy link
Member

unknwon commented Mar 9, 2020

Essentially, add two methods to GitHub authz provider: enterprise/cmd/frontend/internal/authz/github

// FetchUserPerms returns a list of repository/project IDs (on code host) that the
// given account has read access on the code host. The repository ID should be the
// same value as it would be used as api.ExternalRepoSpec.ID. The returned list
// should only include private repositories/project IDs.
//
// Because permissions fetching APIs are often expensive, the implementation should
// try to return partial but valid results in case of error, and it is up to callers
// to decide whether to discard.
FetchUserPerms(ctx context.Context, account *extsvc.ExternalAccount) ([]extsvc.ExternalRepoID, error)
// FetchRepoPerms returns a list of user IDs (on code host) who have read ccess to
// the given repository/project on the code host. The user ID should be the same value
// as it would be used as extsvc.ExternalAccount.AccountID. The returned list should
// include both direct access and inherited from the group/organization/team membership.
//
// Because permissions fetching APIs are often expensive, the implementation should
// try to return partial but valid results in case of error, and it is up to callers
// to decide whether to discard.
FetchRepoPerms(ctx context.Context, repo *api.ExternalRepoSpec) ([]extsvc.ExternalAccountID, error)

An example implementation for GitLab can be found at https://github.com/sourcegraph/sourcegraph/blob/master/enterprise/cmd/frontend/internal/authz/gitlab/sudo.go

Things to note

  1. Return partial but valid results whenever possible.
  2. When fetch in user-centric:
    1. Use token extracted from github.GetExternalAccountData(&userAccount.ExternalAccountData).
    2. The GraphQL node ID as the extsvc.ExternalAccountID for results.
    3. The API is same as what we use for RepoPerms today.
  3. When fetch in repo-centric:
    1. Use the admin token configured in external service.
    2. The GraphQL node ID as the extsvc.ExternalRepoID for results.
    3. API example (login is not required, just for debugging):
    {
      repository(name: "sourcegraph", owner: "sourcegraph") {
        id
        collaborators(affiliation: ALL) {
          edges {
            node {
              id
              login
            }
          }
          pageInfo {
            endCursor
            hasNextPage
          }
        }
      }
    }

Let me know when you have any questions!

@unknwon unknwon added this to the 3.14 milestone Mar 9, 2020
@tsenart tsenart assigned unknwon and unassigned keegancsmith Mar 17, 2020
@tsenart tsenart added the planned/3.14 Issues that were planned for the given milestone. Used by cmd/tracking-issue. label Mar 17, 2020
@tsenart tsenart modified the milestones: 3.14, 3.15 Mar 17, 2020
@unknwon unknwon added planned/3.15 Issues that were planned for the given milestone. Used by cmd/tracking-issue. estimate/1d and removed planned/3.14 Issues that were planned for the given milestone. Used by cmd/tracking-issue. labels Mar 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auth estimate/1d planned/3.15 Issues that were planned for the given milestone. Used by cmd/tracking-issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants