Remote repository: Keep in sync when repos are moved to another org/user.

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

Author: stsewd

readthedocs/oauth/services/github.py

@@ -103,34 +103,23 @@ def create_repository(self, fields, privacy=None, organization=None):
                 remote_id=fields['id'],
                 vcs_provider=self.vcs_provider_slug
             )
-            remote_repository_relation = repo.get_remote_repository_relation(
-                self.user, self.account
-            )
 
-            # It's possible that a user has access to a repository from within
-            # an organization without being member of that organization
-            # (external contributor). In this case, the repository will be
-            # listed under the ``/repos`` endpoint but not under ``/orgs``
-            # endpoint. Then, when calling this method (``create_repository``)
-            # we will have ``organization=None`` but we don't have to skip the
-            # creation of the ``RemoteRepositoryRelation``.
-            if repo.organization and organization and repo.organization != organization:
-                log.debug(
-                    'Not importing repository because mismatched orgs.',
-                    repository=fields['name'],
-                )
-                return None
-
-            if any([
-                # There is an organization associated with this repository:
-                # attach the organization to the repository
-                organization is not None,
-                # There is no organization and the repository belongs to a
-                # user: removes the organization linked to the repository
-                not organization and fields['owner']['type'] == 'User',
-            ]):
+            owner_type = fields["owner"]["type"]
+
+            # If there is an organization associated with this repository,
+            # attach the organization to the repository.
+            if (
+                organization
+                and owner_type == "Organization"
+                and organization.remote_id == fields["owner"]["id"]
+            ):
                 repo.organization = organization
 
+            # If there is no organization and the repository belongs to a user,
+            # remove the organization linked to the repository.
+            if not organization and owner_type == "User":
+                repo.organization = None
+
             repo.name = fields['name']
             repo.full_name = fields['full_name']
             repo.description = fields['description']
@@ -151,7 +140,12 @@ def create_repository(self, fields, privacy=None, organization=None):
 
             repo.save()
 
-            remote_repository_relation.admin = fields.get('permissions', {}).get('admin', False)
+            remote_repository_relation = repo.get_remote_repository_relation(
+                self.user, self.account
+            )
+            remote_repository_relation.admin = fields.get("permissions", {}).get(
+                "admin", False
+            )
             remote_repository_relation.save()
 
             return repo

readthedocs/rtd_tests/tests/test_oauth.py

@@ -13,11 +13,7 @@
 from readthedocs.integrations.models import GitHubWebhook, GitLabWebhook
 from readthedocs.oauth.constants import BITBUCKET, GITHUB, GITLAB
 from readthedocs.oauth.models import RemoteOrganization, RemoteRepository
-from readthedocs.oauth.services import (
-    BitbucketService,
-    GitHubService,
-    GitLabService,
-)
+from readthedocs.oauth.services import BitbucketService, GitHubService, GitLabService
 from readthedocs.projects import constants
 from readthedocs.projects.models import Project
 
@@ -29,8 +25,8 @@ class GitHubOAuthTests(TestCase):
     def setUp(self):
         self.client.login(username='eric', password='test')
         self.user = User.objects.get(pk=1)
-        self.project = Project.objects.get(slug='pip')
-        self.org = RemoteOrganization.objects.create(slug='rtfd')
+        self.project = Project.objects.get(slug="pip")
+        self.org = RemoteOrganization.objects.create(slug="rtfd", remote_id=1234)
         self.privacy = settings.DEFAULT_PRIVACY_LEVEL
         self.service = GitHubService(
             user=self.user,
@@ -55,9 +51,7 @@ def setUp(self):
                 "url": "https://api.github.com/repos/test/Hello-World/hooks/12345678",
             }
         ]
-
-    def test_make_project_pass(self):
-        repo_json = {
+        self.repo_response_data = {
             'name': 'testrepo',
             'full_name': 'testuser/testrepo',
             'id': '12345678',
@@ -67,9 +61,45 @@ def test_make_project_pass(self):
             'ssh_url': 'ssh://[email protected]:testuser/testrepo.git',
             'html_url': 'https://github.com/testuser/testrepo',
             'clone_url': 'https://github.com/testuser/testrepo.git',
+            "owner": {
+                "type": "User",
+                "id": 1234,
+            },
         }
+
+    def test_create_remote_repository(self):
         repo = self.service.create_repository(
-            repo_json, organization=self.org, privacy=self.privacy,
+            self.repo_response_data,
+            privacy=self.privacy,
+        )
+        self.assertIsInstance(repo, RemoteRepository)
+        self.assertEqual(repo.name, "testrepo")
+        self.assertEqual(repo.full_name, "testuser/testrepo")
+        self.assertEqual(repo.remote_id, "12345678")
+        self.assertEqual(repo.vcs_provider, GITHUB)
+        self.assertEqual(repo.description, "Test Repo")
+        self.assertEqual(
+            repo.avatar_url,
+            settings.OAUTH_AVATAR_USER_DEFAULT_URL,
+        )
+        self.assertIn(self.user, repo.users.all())
+        self.assertEqual(repo.organization, None)
+        self.assertEqual(
+            repo.clone_url,
+            "https://github.com/testuser/testrepo.git",
+        )
+        self.assertEqual(
+            repo.ssh_url,
+            "ssh://[email protected]:testuser/testrepo.git",
+        )
+        self.assertEqual(repo.html_url, "https://github.com/testuser/testrepo")
+
+    def test_create_remote_repository_with_organization(self):
+        self.repo_response_data["owner"]["type"] = "Organization"
+        repo = self.service.create_repository(
+            self.repo_response_data,
+            organization=self.org,
+            privacy=self.privacy,
         )
         self.assertIsInstance(repo, RemoteRepository)
         self.assertEqual(repo.name, 'testrepo')
@@ -91,23 +121,103 @@ def test_make_project_pass(self):
         )
         self.assertEqual(repo.html_url, 'https://github.com/testuser/testrepo')
 
-    def test_make_project_fail(self):
-        repo_json = {
-            'name': '',
-            'full_name': '',
-            'id': '',
-            'description': '',
-            'git_url': '',
-            'private': True,
-            'ssh_url': '',
-            'html_url': '',
-            'clone_url': '',
-        }
+    def test_skip_creation_remote_repository_on_private_repos(self):
+        self.repo_response_data["private"] = True
         github_project = self.service.create_repository(
-            repo_json, organization=self.org, privacy=self.privacy,
+            self.repo_response_data,
+            organization=self.org,
+            privacy=self.privacy,
         )
         self.assertIsNone(github_project)
 
+    def test_project_was_moved_from_a_personal_account_to_an_organization(self):
+        github_project = self.service.create_repository(
+            self.repo_response_data,
+            privacy=self.privacy,
+        )
+        self.assertEqual(github_project.organization, None)
+
+        # Project belongs has been moved to an organization.
+        self.repo_response_data["owner"]["type"] = "Organization"
+
+        self.service.create_repository(
+            self.repo_response_data,
+            privacy=self.privacy,
+        )
+        github_project.refresh_from_db()
+        self.assertEqual(github_project.organization, None)
+
+        self.service.create_repository(
+            self.repo_response_data,
+            organization=self.org,
+            privacy=self.privacy,
+        )
+        github_project.refresh_from_db()
+        self.assertEqual(github_project.organization, self.org)
+
+    def test_project_was_moved_from_an_organization_to_a_personal_account(self):
+        # Project belongs to an organization.
+        self.repo_response_data["owner"]["type"] = "Organization"
+        github_project = self.service.create_repository(
+            self.repo_response_data,
+            organization=self.org,
+            privacy=self.privacy,
+        )
+        self.assertEqual(github_project.organization, self.org)
+
+        # Project belongs has been moved to a personal account.
+        self.repo_response_data["owner"]["type"] = "User"
+
+        # This operation doesn't have any effect.
+        self.service.create_repository(
+            self.repo_response_data,
+            organization=self.org,
+            privacy=self.privacy,
+        )
+        github_project.refresh_from_db()
+        self.assertEqual(github_project.organization, self.org)
+
+        self.service.create_repository(
+            self.repo_response_data,
+            privacy=self.privacy,
+        )
+        github_project.refresh_from_db()
+        self.assertEqual(github_project.organization, None)
+
+    def test_project_was_moved_to_another_organization(self):
+        another_remote_organization = RemoteOrganization.objects.create(
+            slug="another", remote_id=4321
+        )
+
+        # Project belongs to an organization.
+        self.repo_response_data["owner"]["type"] = "Organization"
+        github_project = self.service.create_repository(
+            self.repo_response_data,
+            organization=self.org,
+            privacy=self.privacy,
+        )
+        self.assertEqual(github_project.organization, self.org)
+
+        # Project was moved to another organization.
+        self.repo_response_data["owner"]["id"] = 4321
+
+        self.service.create_repository(
+            self.repo_response_data,
+            organization=another_remote_organization,
+            privacy=self.privacy,
+        )
+        github_project.refresh_from_db()
+        self.assertEqual(github_project.organization, another_remote_organization)
+
+        # The remote ID doesn't match, the project isn't moved to the organization.
+        self.service.create_repository(
+            self.repo_response_data,
+            organization=self.org,
+            privacy=self.privacy,
+        )
+        github_project.refresh_from_db()
+        self.assertEqual(github_project.organization, another_remote_organization)
+
     def test_make_organization(self):
         org_json = {
             'id': 12345,
@@ -131,20 +241,11 @@ def test_import_with_no_token(self):
         self.assertEqual(services, [])
 
     def test_multiple_users_same_repo(self):
-        repo_json = {
-            'name': '',
-            'full_name': 'testrepo/multiple',
-            'id': '12345678',
-            'description': '',
-            'git_url': '',
-            'private': False,
-            'ssh_url': '',
-            'html_url': '',
-            'clone_url': '',
-        }
-
+        self.repo_response_data["owner"]["type"] = "Organization"
         github_project = self.service.create_repository(
-            repo_json, organization=self.org, privacy=self.privacy,
+            self.repo_response_data,
+            organization=self.org,
+            privacy=self.privacy,
         )
 
         user2 = User.objects.get(pk=2)
@@ -153,31 +254,43 @@ def test_multiple_users_same_repo(self):
             account=get(SocialAccount, user=self.user)
         )
         github_project_2 = service.create_repository(
-            repo_json, organization=self.org, privacy=self.privacy,
+            self.repo_response_data,
+            organization=self.org,
+            privacy=self.privacy,
         )
         self.assertIsInstance(github_project, RemoteRepository)
         self.assertIsInstance(github_project_2, RemoteRepository)
         self.assertEqual(github_project_2, github_project)
 
         github_project_3 = self.service.create_repository(
-            repo_json, organization=self.org, privacy=self.privacy,
+            self.repo_response_data,
+            organization=self.org,
+            privacy=self.privacy,
         )
         github_project_4 = service.create_repository(
-            repo_json, organization=self.org, privacy=self.privacy,
+            self.repo_response_data,
+            organization=self.org,
+            privacy=self.privacy,
         )
         self.assertIsInstance(github_project_3, RemoteRepository)
         self.assertIsInstance(github_project_4, RemoteRepository)
         self.assertEqual(github_project, github_project_3)
         self.assertEqual(github_project_2, github_project_4)
 
         github_project_5 = self.service.create_repository(
-            repo_json, organization=self.org, privacy=self.privacy,
+            self.repo_response_data,
+            organization=self.org,
+            privacy=self.privacy,
         )
         github_project_6 = service.create_repository(
-            repo_json, organization=self.org, privacy=self.privacy,
+            self.repo_response_data,
+            organization=self.org,
+            privacy=self.privacy,
         )
 
+        self.assertIsNotNone(github_project)
         self.assertEqual(github_project, github_project_5)
+        self.assertIsNotNone(github_project_2)
         self.assertEqual(github_project_2, github_project_6)
 
     @mock.patch('readthedocs.oauth.services.github.log')
@@ -237,15 +350,18 @@ def test_make_private_project(self):
         Test ability to import ``public`` repositories under ``private`` level.
         """
         repo_json = {
-            'name': 'testrepo',
-            'full_name': 'testuser/testrepo',
-            'id': '12345678',
-            'description': 'Test Repo',
-            'git_url': 'git://github.com/testuser/testrepo.git',
-            'private': False,
-            'ssh_url': 'ssh://[email protected]:testuser/testrepo.git',
-            'html_url': 'https://github.com/testuser/testrepo',
-            'clone_url': 'https://github.com/testuser/testrepo.git',
+            "name": "testrepo",
+            "full_name": "testuser/testrepo",
+            "id": "12345678",
+            "description": "Test Repo",
+            "git_url": "git://github.com/testuser/testrepo.git",
+            "private": False,
+            "ssh_url": "ssh://[email protected]:testuser/testrepo.git",
+            "html_url": "https://github.com/testuser/testrepo",
+            "clone_url": "https://github.com/testuser/testrepo.git",
+            "owner": {
+                "type": "User",
+            },
         }
         repo = self.service.create_repository(repo_json, organization=self.org)
         self.assertIsNotNone(repo)

readthedocs/rtd_tests/tests/test_oauth_sync.py

@@ -1,8 +1,7 @@
-from allauth.socialaccount.models import SocialAccount, SocialToken
-from allauth.socialaccount.providers.github.views import GitHubOAuth2Adapter
 import django_dynamic_fixture as fixture
 import requests_mock
-
+from allauth.socialaccount.models import SocialAccount, SocialToken
+from allauth.socialaccount.providers.github.views import GitHubOAuth2Adapter
 from django.contrib.auth.models import User
 from django.test import TestCase
 
@@ -13,9 +12,7 @@
     RemoteRepository,
     RemoteRepositoryRelation,
 )
-from readthedocs.oauth.services import (
-    GitHubService,
-)
+from readthedocs.oauth.services import GitHubService
 from readthedocs.projects.models import Project
 
 
@@ -173,7 +170,7 @@ def test_sync_repositories_relation_with_organization(self, mock_request):
         """
         Sync repositories relations for a user where the RemoteRepository and RemoteOrganization already exist.
 
-        Note that ``repository.owner.type == 'Organzation'`` in the GitHub response.
+        Note that ``repository.owner.type == 'Organization'`` in the GitHub response.
         """
         self.payload_user_repos[0]['owner']['type'] = 'Organization'
         mock_request.get('https://api.github.com/user/repos', json=self.payload_user_repos)