Ambiguous association between permissions and authz provider
Created by: unknwon
In the current design of permissions tables (i.e. user_permissions
, repo_permissions
), it has the assumption that only one authz provider per code host is configured for mirroring permissions. This assumption IMO is inherited from the assumption of a customer would only want to configure at most one code host connection per type, i.e. only one GitLab code host connection, and maybe another GitHub code host connection.
Some evidences are in cache implementation, we only store the project/repo ID but not the actual service ID (e.g. https://github.com, https://gitlab.mycorp.com) these project/repo IDs belong to, which could potentially have collisions when there are multiple authz providers configured for the same type of code host, e.g. two GitLab authz providers.
- https://sourcegraph.com/github.com/sourcegraph/sourcegraph/-/blob/enterprise/cmd/frontend/internal/authz/github/cache.go
- https://sourcegraph.com/github.com/sourcegraph/sourcegraph/-/blob/enterprise/cmd/frontend/internal/authz/gitlab/cache.go
It is generally unlikely thus not a problem so far, but could cause ambiguity with the transition of moving permissions syncing to the background (RFC 113) which unifies the store layer to Postgres.
There are two ways we could think about this problem: a. We make it clear in our docs: Repository permissions that we currently only support one code host authz provider per type until a large customer wants such support. b. We fix it now.
I personally prefer option a because:
- We could do the migration in the future (find the code host config has the authz provider and update the column in perms table with the URL of that code host).
- It is still possible to configure multiple code host connections of same type, but only have single authz provider.
- Tackle option b requires non-trivial work with not so much in return (for the foreseeable future) given the assumption of only having one authz provider configured has lasted in fairly long time.
cc @beyang in case I missed some parts from the whole picture.