OAuth: update the affected member directly on organization member events

Bulk membership changes send one organization event per member, and
each event triggered a full sync of the installation, costing at least
one API request per repository. Large installations exceed GitHub's
per-installation rate limit this way.

Since the event includes the affected member, we now update that
member only. Removed members have their access revoked directly from
the database, without querying the API. Added members with an account
connected have their access checked against repositories linked to a
project (one request each); the rest of their access is synced when
they sign in.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LVjkPjH3EDEQmtywJvDVau

Author: claude

readthedocs/oauth/services/githubapp.py

@@ -7,6 +7,7 @@
 from django.conf import settings
 from github import Github
 from github import GithubException
+from github import RateLimitExceededException
 from github.Installation import Installation as GHInstallation
 from github.Organization import Organization as GHOrganization
 from github.Repository import Repository as GHRepository
@@ -394,6 +395,49 @@ def _get_social_accounts(self, ids):
             provider=self.allauth_provider.id,
         ).select_related("user")
 
+    def update_member_access(self, account: SocialAccount, login: str):
+        """
+        Update the relations of a user with the repositories linked to a project.
+
+        Instead of re-syncing all collaborators of all repositories,
+        we check the permission of the user on each repository linked to a project
+        (one API request per repository). Their access to the rest of the repositories
+        is synced when they sign in or manually re-sync their repositories.
+
+        :param account: The social account connected to the user.
+        :param login: The GitHub username of the user.
+        """
+        remote_repositories = self.installation.repositories.filter(
+            projects__isnull=False
+        ).distinct()
+        for remote_repo in remote_repositories:
+            try:
+                # NOTE: we use the lazy option to avoid fetching the repository object,
+                # since we only need the object to interact with the permissions API.
+                gh_repo = self.installation_client.get_repo(int(remote_repo.remote_id), lazy=True)
+                permission = gh_repo.get_collaborator_permission(login)
+            except RateLimitExceededException:
+                # All the remaining requests will fail as well.
+                raise
+            except GithubException:
+                log.info(
+                    "Failed to fetch the repository permissions of the user",
+                    repository_id=remote_repo.remote_id,
+                    exc_info=True,
+                )
+                continue
+
+            if permission == "none":
+                RemoteRepositoryRelation.objects.filter(
+                    remote_repository=remote_repo,
+                    account=account,
+                ).delete()
+                continue
+
+            remote_repo_relation = remote_repo.get_remote_repository_relation(account.user, account)
+            remote_repo_relation.admin = permission == "admin"
+            remote_repo_relation.save()
+
     def send_build_status(self, *, build, commit, status):
         """
         Create a commit status on GitHub for the given build.

readthedocs/oauth/tasks.py

@@ -4,11 +4,13 @@
 from functools import cached_property
 
 import structlog
+from allauth.socialaccount.models import SocialAccount
 from django.contrib.auth.models import User
 from django.db.models.functions import ExtractIsoWeekDay
 from django.urls import reverse
 from django.utils import timezone
 
+from readthedocs.allauth.providers.githubapp.provider import GitHubAppProvider
 from readthedocs.api.v2.views.integrations import ExternalVersionData
 from readthedocs.builds.constants import EXTERNAL
 from readthedocs.builds.utils import memcache_lock
@@ -25,6 +27,7 @@
 from readthedocs.oauth.constants import GITHUB_APP
 from readthedocs.oauth.models import GitHubAppInstallation
 from readthedocs.oauth.models import RemoteRepository
+from readthedocs.oauth.models import RemoteRepositoryRelation
 from readthedocs.oauth.notifications import MESSAGE_OAUTH_SYNCING_REMOTE_REPOSITORIES
 from readthedocs.oauth.notifications import MESSAGE_OAUTH_UNSUPPORTED_GIT_PROVIDER
 from readthedocs.oauth.notifications import MESSAGE_OAUTH_WEBHOOK_INTEGRATION_MISMATCH
@@ -753,11 +756,12 @@ def _handle_organization_event(self):
         if created:
             return
 
-        # We need to do a full sync of the repositories if members were added or removed,
-        # this is since we don't know to which repositories the members have access.
-        # GH doesn't send a member event for this.
-        if action in ("member_added", "member_removed"):
-            installation.service.sync()
+        if action == "member_added":
+            self._add_member_repository_relations(installation)
+            return
+
+        if action == "member_removed":
+            self._remove_member_repository_relations(installation)
             return
 
         # NOTE: installation_target should handle this instead?
@@ -781,6 +785,49 @@ def _handle_organization_event(self):
         # - member_invited: We don't do anything with invited members.
         return
 
+    def _add_member_repository_relations(self, installation):
+        """
+        Grant the member the event was triggered for access to relevant repositories.
+
+        If the member has an account connected, we check their access to each
+        repository linked to a project. Their access to the rest of the repositories
+        is synced when they sign in or manually re-sync their repositories.
+        """
+        member = self.data.get("membership", {}).get("user", {})
+        member_id = member.get("id")
+        if not member_id:
+            log.info("Organization event doesn't include the affected member.")
+            return
+        account = SocialAccount.objects.filter(
+            provider=GitHubAppProvider.id,
+            uid=str(member_id),
+        ).first()
+        if not account:
+            return
+        installation.service.update_member_access(account, member["login"])
+
+    def _remove_member_repository_relations(self, installation):
+        """
+        Remove all repository relations from the member the event was triggered for.
+
+        A member removed from the organization lost access to all its repositories,
+        so we can revoke their access without querying the GH API.
+        """
+        member_id = self.data.get("membership", {}).get("user", {}).get("id")
+        if not member_id:
+            log.info("Organization event doesn't include the affected member.")
+            return
+        count, _ = RemoteRepositoryRelation.objects.filter(
+            account__provider=GitHubAppProvider.id,
+            account__uid=str(member_id),
+            remote_repository__github_app_installation=installation,
+        ).delete()
+        log.info(
+            "Removed repository relations from member removed from the organization.",
+            member_id=member_id,
+            count=count,
+        )
+
     def _handle_member_event(self):
         """
         Handle the member event.

readthedocs/oauth/tests/test_githubapp_webhook.py

@@ -32,6 +32,7 @@
     GitHubAppInstallation,
     RemoteOrganization,
     RemoteRepository,
+    RemoteRepositoryRelation,
 )
 from readthedocs.oauth.services import GitHubAppService
 from readthedocs.projects.models import Project
@@ -901,33 +902,101 @@ def test_repository_created(self):
         r = self.post_webhook("repository", payload)
         assert r.status_code == 200
 
+    @mock.patch.object(GitHubAppService, "update_member_access")
     @mock.patch.object(GitHubAppService, "sync")
-    def test_organization_member_added(self, sync):
+    def test_organization_member_added(self, sync, update_member_access):
+        member = get(User)
+        member_account = get(
+            SocialAccount,
+            user=member,
+            provider=GitHubAppProvider.id,
+            uid="9999",
+        )
         payload = {
             "installation": {
                 "id": self.installation.installation_id,
                 "target_id": self.installation.target_id,
                 "target_type": self.installation.target_type,
             },
             "action": "member_added",
+            "membership": {
+                "user": {
+                    "login": "member",
+                    "id": 9999,
+                },
+            },
         }
         r = self.post_webhook("organization", payload)
         assert r.status_code == 200
-        sync.assert_called_once()
+        update_member_access.assert_called_once_with(member_account, "member")
+        sync.assert_not_called()
+
+    @mock.patch.object(GitHubAppService, "update_member_access")
+    @mock.patch.object(GitHubAppService, "sync")
+    def test_organization_member_added_without_account(self, sync, update_member_access):
+        payload = {
+            "installation": {
+                "id": self.installation.installation_id,
+                "target_id": self.installation.target_id,
+                "target_type": self.installation.target_type,
+            },
+            "action": "member_added",
+            "membership": {
+                "user": {
+                    "login": "member",
+                    "id": 9999,
+                },
+            },
+        }
+        r = self.post_webhook("organization", payload)
+        assert r.status_code == 200
+        # The member doesn't have an account connected,
+        # their access is synced when they sign in.
+        update_member_access.assert_not_called()
+        sync.assert_not_called()
 
     @mock.patch.object(GitHubAppService, "sync")
     def test_organization_member_removed(self, sync):
+        member = get(User)
+        member_account = get(
+            SocialAccount,
+            user=member,
+            provider=GitHubAppProvider.id,
+            uid="9999",
+        )
+        get(
+            RemoteRepositoryRelation,
+            remote_repository=self.remote_repository,
+            user=member,
+            account=member_account,
+        )
+        relation = get(
+            RemoteRepositoryRelation,
+            remote_repository=self.remote_repository,
+            user=self.user,
+            account=self.socialaccount,
+        )
         payload = {
             "installation": {
                 "id": self.installation.installation_id,
                 "target_id": self.installation.target_id,
                 "target_type": self.installation.target_type,
             },
             "action": "member_removed",
+            "membership": {
+                "user": {
+                    "login": "member",
+                    "id": 9999,
+                },
+            },
         }
         r = self.post_webhook("organization", payload)
         assert r.status_code == 200
-        sync.assert_called_once()
+        sync.assert_not_called()
+        # The relations from the removed member are deleted without querying the API,
+        # relations from other users are kept.
+        assert not member.remote_repository_relations.exists()
+        assert list(self.user.remote_repository_relations.all()) == [relation]
 
     @mock.patch.object(GitHubAppService, "sync")
     def test_organization_renamed(self, sync):

readthedocs/rtd_tests/tests/test_oauth.py

@@ -596,6 +596,72 @@ def test_update_invalid_repository(self, request):
             id=self.remote_repository.id
         ).exists()
 
+    @requests_mock.Mocker(kw="request")
+    def test_update_member_access(self, request):
+        member = get(User)
+        member_account = get(
+            SocialAccount,
+            uid="9999",
+            user=member,
+            provider=GitHubAppProvider.id,
+        )
+        # ``self.remote_repository`` is linked to a project, ``repo2`` is not.
+        repo2 = get(
+            RemoteRepository,
+            remote_id="7777",
+            name="repo2",
+            full_name="user/repo2",
+            vcs_provider=GITHUB_APP,
+            github_app_installation=self.installation,
+        )
+        request.post(
+            f"{self.api_url}/app/installations/1111/access_tokens",
+            json=self._get_access_token_json(),
+        )
+        # Only permissions on repositories linked to a project are checked,
+        # requesting the permissions on user/repo2 would fail the test (not mocked).
+        request.get(
+            f"{self.api_url}/repositories/{self.remote_repository.remote_id}/collaborators/member/permission",
+            json={"permission": "admin"},
+        )
+
+        service = self.installation.service
+        service.update_member_access(member_account, "member")
+
+        relation = member.remote_repository_relations.get()
+        assert relation.remote_repository == self.remote_repository
+        assert relation.account == member_account
+        assert relation.admin is True
+
+    @requests_mock.Mocker(kw="request")
+    def test_update_member_access_without_access(self, request):
+        member = get(User)
+        member_account = get(
+            SocialAccount,
+            uid="9999",
+            user=member,
+            provider=GitHubAppProvider.id,
+        )
+        get(
+            RemoteRepositoryRelation,
+            remote_repository=self.remote_repository,
+            user=member,
+            account=member_account,
+        )
+        request.post(
+            f"{self.api_url}/app/installations/1111/access_tokens",
+            json=self._get_access_token_json(),
+        )
+        request.get(
+            f"{self.api_url}/repositories/{self.remote_repository.remote_id}/collaborators/member/permission",
+            json={"permission": "none"},
+        )
+
+        service = self.installation.service
+        service.update_member_access(member_account, "member")
+
+        assert not member.remote_repository_relations.exists()
+
     @requests_mock.Mocker(kw="request")
     def test_sync(self, request):
         assert self.installation.repositories.count() == 1