Remote repository: Keep in sync when repos are moved to another org/users #9178
Conversation
There was a problem hiding this comment.
This approach looks good. I'm a bit worried, tho. I'm not sure that I'm understanding 100% what it conveys and what problems this change can bring.
The main thing I'm thinking about is if we may need to trigger a re-sync for all users that has access to a repository when its organization is changed? I'm trying to think if when a project is moved from one organization to the other users won't lose permissions on it because being out-of-sync.
stsewd
force-pushed
the
keep-in-sync-remote-repos
branch
from
ad68360
to
4c7f257
Compare
I think this is a problem with the current implementation as well, like when a project is transferred to another user or a user is removed from an org/project, it looks like we are refreshing the permissions every 4 hours. Anyway, those problems should be solved by subscribing to a webhook if we aren't already. |
readthedocs/oauth/services/github.py
Outdated
| if ( | ||
| organization | ||
| and owner_type == "Organization" | ||
| and organization.remote_id == fields["owner"]["id"] |
There was a problem hiding this comment.
This shouldn't be needed since we are passing the org from where the projects were fetched from, but just in case.
…ser. We were doing some checks to avoid removing the remote organization from a remote repo when this is listed from the /users/repos/ endpoint or changing the organization when the project was moved. But, since we are hitting the GitHub API directly, we can always trust the result from there, which includes the type of owner of the repository, we can use that to decide if the repository belongs to an organization or not. Fixes #8715
stsewd
force-pushed
the
keep-in-sync-remote-repos
branch
from
4c7f257
to
8c7c362
Compare
stsewd
force-pushed
the
keep-in-sync-remote-repos
branch
from
1248df5
to
eb8d042
Compare
Yeah, we tried to do this, but we can't do it properly being a OAuth application. It will be possible when using a GitHub Application instead. See #7336 (comment) |
There was a problem hiding this comment.
I think this is fine. I've made more suggestions after a deeper review. Note this is changing a core part of our auth permissions and we should be careful here. I'd like another pair of eyes reviewing this as well just in case I'm missing something -- cc @ericholscher @agjohnson
| def create_repository(self, fields, privacy=None, organization=None): | ||
| """ | ||
| Update or create a repository from API response. | ||
|
|
||
| :param fields: dictionary of response data from API | ||
| :param privacy: privacy level to support | ||
| :param organization: remote organization to associate with | ||
| :type organization: RemoteOrganization | ||
| :rtype: RemoteRepository | ||
| """ | ||
| raise NotImplementedError | ||
|
|
||
| def create_organization(self, fields): | ||
| """ | ||
| Update or create remote organization from API response. | ||
|
|
||
| :param fields: dictionary response of data from API | ||
| :rtype: RemoteOrganization | ||
| """ | ||
| raise NotImplementedError | ||
|
|
There was a problem hiding this comment.
the signature is different for the github provider now, we don't make any reference to these methods outside the class (we use the sync method) so we are fine having a different signature.
There was a problem hiding this comment.
We should implement the same logic for the other providers as well if that's technically possible. Otherwise, we will have different behavior between GH SSO and the others.
| def create_repository(self, fields, privacy=None, organization=None): | ||
| """ | ||
| Update or create a repository from API response. | ||
|
|
||
| :param fields: dictionary of response data from API | ||
| :param privacy: privacy level to support | ||
| :param organization: remote organization to associate with | ||
| :type organization: RemoteOrganization | ||
| :rtype: RemoteRepository | ||
| """ | ||
| raise NotImplementedError | ||
|
|
||
| def create_organization(self, fields): | ||
| """ | ||
| Update or create remote organization from API response. | ||
|
|
||
| :param fields: dictionary response of data from API | ||
| :rtype: RemoteOrganization | ||
| """ | ||
| raise NotImplementedError | ||
|
|
There was a problem hiding this comment.
We should implement the same logic for the other providers as well if that's technically possible. Otherwise, we will have different behavior between GH SSO and the others.
|
Ops, I didn't add the comment to the approval. I'm fine merging these changes since they are good and they will solve some issues we have. However, we should add a new issue to track the work required for the other VCS and implement the same logic for them. |
We were doing some checks to avoid removing the remote organization
from a remote repo when this is listed from the /users/repos/ endpoint
or changing the organization when the project was moved.
But, since we are hitting the GitHub API directly,
we can always trust the result from there,
which includes the type of owner of the repository,
we can use that to decide if the repository belongs to an organization or not.
Fixes #8715