From fa53c1c2ec904396c549dfc74b935ba766f063c1 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 6 Feb 2025 19:00:44 -0500 Subject: [PATCH 1/8] Git service: depend on the project instead of users With https://github.com/readthedocs/readthedocs.org/pull/11942, a GH app is used to interact with the GH API, a GH app is not directly tied to a user, so this assumption is not valid anymore. For most of the operations (except for syncing repos), we don't need to know who is the user that is performing the action, we just need to know the project that is being affected. --- readthedocs/builds/tasks.py | 106 ++++---------- readthedocs/oauth/models.py | 17 ++- readthedocs/oauth/services/base.py | 181 ++++++++++++++---------- readthedocs/oauth/services/bitbucket.py | 9 +- readthedocs/oauth/services/github.py | 5 +- readthedocs/oauth/services/gitlab.py | 5 +- readthedocs/oauth/tasks.py | 101 ++++++------- readthedocs/oauth/utils.py | 45 ++---- readthedocs/projects/models.py | 35 +++-- readthedocs/projects/views/mixins.py | 2 + readthedocs/projects/views/private.py | 8 +- 11 files changed, 247 insertions(+), 267 deletions(-) diff --git a/readthedocs/builds/tasks.py b/readthedocs/builds/tasks.py index fc076af9118..7e13ca62d19 100644 --- a/readthedocs/builds/tasks.py +++ b/readthedocs/builds/tasks.py @@ -30,12 +30,10 @@ ) from readthedocs.builds.models import Build, Version from readthedocs.builds.utils import memcache_lock -from readthedocs.core.permissions import AdminPermission from readthedocs.core.utils import send_email, trigger_build from readthedocs.integrations.models import HttpExchange from readthedocs.notifications.models import Notification from readthedocs.oauth.notifications import MESSAGE_OAUTH_BUILD_STATUS_FAILURE -from readthedocs.projects.constants import GITHUB_BRAND, GITLAB_BRAND from readthedocs.projects.models import Project, WebHookEvent from readthedocs.storage import build_commands_storage from readthedocs.worker import app @@ -385,24 +383,16 @@ def sync_versions_task(project_pk, tags_data, branches_data, **kwargs): @app.task(max_retries=3, default_retry_delay=60, queue="web") def send_build_status(build_pk, commit, status): """ - Send Build Status to Git Status API for project external versions. - - It tries using these services' account in order: - - 1. user's account that imported the project - 2. each user's account from the project's maintainers + Send build status to GitHub/GitLab for a given build/commit. :param build_pk: Build primary key :param commit: commit sha of the pull/merge request :param status: build status failed, pending, or success to be sent. """ - # TODO: Send build status for Bitbucket. build = Build.objects.filter(pk=build_pk).first() if not build: return - provider_name = build.project.git_provider_name - log.bind( build_id=build.pk, project_slug=build.project.slug, @@ -412,76 +402,36 @@ def send_build_status(build_pk, commit, status): log.debug("Sending build status.") - if provider_name in [GITHUB_BRAND, GITLAB_BRAND]: - # get the service class for the project e.g: GitHubService. - service_class = build.project.git_service_class() - users = AdminPermission.admins(build.project) - - if build.project.remote_repository: - remote_repository = build.project.remote_repository - remote_repository_relations = ( - remote_repository.remote_repository_relations.filter( - account__isnull=False, - # Use ``user_in=`` instead of ``user__projects=`` here - # because User's are not related to Project's directly in - # Read the Docs for Business - user__in=AdminPermission.members(build.project), - ) - .select_related("account", "user") - .only("user", "account") - ) - - # Try using any of the users' maintainer accounts - # Try to loop through all remote repository relations for the projects users - for relation in remote_repository_relations: - service = service_class(relation.user, relation.account) - # Send status report using the API. - success = service.send_build_status( - build, - commit, - status, - ) + # Get the service class for the project e.g: GitHubService. + # We fallback to guess the service from the repo, + # in the future we should only consider projects that have a remote repository. + service_class = build.project.get_git_service_class(fallback_to_clone_url=True) + if not service_class: + log.info("Project isn't connected to a Git service.") + return False - if success: - log.debug( - "Build status report sent correctly.", - user_username=relation.user.username, - ) - return True - else: - log.warning("Project does not have a RemoteRepository.") - # Try to send build status for projects with no RemoteRepository - for user in users: - services = service_class.for_user(user) - # Try to loop through services for users all social accounts - # to send successful build status - for service in services: - success = service.send_build_status( - build, - commit, - status, - ) - if success: - log.debug( - "Build status report sent correctly using an user account.", - user_username=user.username, - ) - return True - - # NOTE: this notifications was attached to every user. - # Now, I'm attaching it to the project itself since it's a problem at project level. - Notification.objects.add( - message_id=MESSAGE_OAUTH_BUILD_STATUS_FAILURE, - attached_to=build.project, - format_values={ - "provider_name": provider_name, - "url_connect_account": reverse("socialaccount_connections"), - }, - dismissable=True, + for service in service_class.for_project(build.project): + success = service.send_build_status( + build, + commit, + status, ) + if success: + log.debug("Build status report sent correctly.") + return True + + Notification.objects.add( + message_id=MESSAGE_OAUTH_BUILD_STATUS_FAILURE, + attached_to=build.project, + format_values={ + "provider_name": service_class.provider_name, + "url_connect_account": reverse("socialaccount_connections"), + }, + dismissable=True, + ) - log.info("No social account or repository permission available.") - return False + log.info("No social account or repository permission available.") + return False @app.task(queue="web") diff --git a/readthedocs/oauth/models.py b/readthedocs/oauth/models.py index 4810943d046..b236fb5737f 100644 --- a/readthedocs/oauth/models.py +++ b/readthedocs/oauth/models.py @@ -1,5 +1,5 @@ """OAuth service models.""" - +import structlog from allauth.socialaccount.models import SocialAccount from django.contrib.auth.models import User from django.core.validators import URLValidator @@ -14,6 +14,8 @@ from .constants import VCS_PROVIDER_CHOICES from .querysets import RemoteOrganizationQuerySet, RemoteRepositoryQuerySet +log = structlog.get_logger(__name__) + class RemoteOrganization(TimeStampedModel): @@ -224,6 +226,19 @@ def get_remote_repository_relation(self, user, social_account): ) return remote_repository_relation + def get_service_class(self): + from readthedocs.oauth.services import registry + + for service_cls in registry: + if service_cls.vcs_provider_slug == self.vcs_provider: + return service_cls + + # NOTE: this should never happen, but we log it just in case + log.exception( + "Service not found for the VCS provider", vcs_provider=self.vcs_provider + ) + return None + class RemoteRepositoryRelation(TimeStampedModel): remote_repository = models.ForeignKey( diff --git a/readthedocs/oauth/services/base.py b/readthedocs/oauth/services/base.py index 63c5007859b..7f570acd3ab 100644 --- a/readthedocs/oauth/services/base.py +++ b/readthedocs/oauth/services/base.py @@ -1,10 +1,8 @@ """OAuth utility functions.""" - from datetime import datetime import structlog from allauth.socialaccount.models import SocialAccount -from allauth.socialaccount.providers import registry from allauth.socialaccount.providers.oauth2.views import OAuth2Adapter from django.conf import settings from django.urls import reverse @@ -14,6 +12,8 @@ from requests.exceptions import RequestException from requests_oauthlib import OAuth2Session +from readthedocs.core.permissions import AdminPermission + log = structlog.get_logger(__name__) @@ -29,19 +29,103 @@ class SyncServiceError(Exception): class Service: + """Base class for service that interacts with a VCS provider and a project.""" + + vcs_provider_slug: str + url_pattern: str + provider_name: str + default_user_avatar_url = settings.OAUTH_AVATAR_USER_DEFAULT_URL + default_org_avatar_url = settings.OAUTH_AVATAR_ORG_DEFAULT_URL + + @classmethod + def for_project(self, project): + """Return an iterator of services that can be used for the project.""" + raise NotImplementedError + + @classmethod + def for_user(self, user): + """Return an iterator of services that belong to the user.""" + raise NotImplementedError + + def sync(self): + """ + Sync remote repositories and organizations. + + - Creates a new RemoteRepository/Organization per new repository + - Updates fields for existing RemoteRepository/Organization + - Deletes old RemoteRepository/Organization that are not present + for this user in the current provider + """ + raise NotImplementedError + + def setup_webhook(self, project, integration=None): + """ + Setup webhook for project. + + :param project: project to set up webhook for + :type project: Project + :param integration: Integration for the project + :type integration: Integration + :returns: boolean based on webhook set up success, and requests Response object + :rtype: (Bool, Response) + """ + raise NotImplementedError + + def update_webhook(self, project, integration): + """ + Update webhook integration. + + :param project: project to set up webhook for + :type project: Project + :param integration: Webhook integration to update + :type integration: Integration + :returns: boolean based on webhook update success, and requests Response object + :rtype: (Bool, Response) + """ + raise NotImplementedError + + def send_build_status(self, build, commit, status): + """ + Create commit status for project. + + :param build: Build to set up commit status for + :type build: Build + :param commit: commit sha of the pull/merge request + :type commit: str + :param status: build state failure, pending, or success. + :type status: str + :returns: boolean based on commit status creation was successful or not. + :rtype: Bool + """ + raise NotImplementedError + + @classmethod + def is_project_service(cls, project): + """ + Determine if this is the service the project is using. + + .. note:: + + This should be deprecated in favor of attaching the + :py:class:`RemoteRepository` to the project instance. This is a + slight improvement on the legacy check for webhooks + """ + return ( + cls.url_pattern is not None + and cls.url_pattern.search(project.repo) is not None + ) + + +class UserService(Service): + """ - Service mapping for local accounts. + Subclass of Service that interacts with a VCS provider using the user's OAuth token. :param user: User to use in token lookup and session creation :param account: :py:class:`SocialAccount` instance for user """ adapter = None - url_pattern = None - vcs_provider_slug = None - - default_user_avatar_url = settings.OAUTH_AVATAR_USER_DEFAULT_URL - default_org_avatar_url = settings.OAUTH_AVATAR_ORG_DEFAULT_URL def __init__(self, user, account): self.session = None @@ -53,17 +137,20 @@ def __init__(self, user, account): social_account_id=self.account.pk, ) + @classmethod + def for_project(cls, project): + users = AdminPermission.admins(project) + for user in users: + yield from cls.for_user(user) + @classmethod def for_user(cls, user): - """Return list of instances if user has an account for the provider.""" - try: - accounts = SocialAccount.objects.filter( - user=user, - provider=cls.adapter.provider_id, - ) - return [cls(user=user, account=account) for account in accounts] - except SocialAccount.DoesNotExist: - return [] + accounts = SocialAccount.objects.filter( + user=user, + provider=cls.adapter.provider_id, + ) + for account in accounts: + yield cls(user=user, account=account) def get_adapter(self) -> type[OAuth2Adapter]: return self.adapter @@ -72,10 +159,6 @@ def get_adapter(self) -> type[OAuth2Adapter]: def provider_id(self): return self.get_adapter().provider_id - @property - def provider_name(self): - return registry.get_class(self.provider_id).name - def get_session(self): if self.session is None: self.create_session() @@ -300,60 +383,8 @@ def get_provider_data(self, project, integration): """ raise NotImplementedError - def setup_webhook(self, project, integration=None): - """ - Setup webhook for project. - - :param project: project to set up webhook for - :type project: Project - :param integration: Integration for the project - :type integration: Integration - :returns: boolean based on webhook set up success, and requests Response object - :rtype: (Bool, Response) - """ - raise NotImplementedError - - def update_webhook(self, project, integration): - """ - Update webhook integration. - - :param project: project to set up webhook for - :type project: Project - :param integration: Webhook integration to update - :type integration: Integration - :returns: boolean based on webhook update success, and requests Response object - :rtype: (Bool, Response) - """ + def sync_repositories(self): raise NotImplementedError - def send_build_status(self, build, commit, status): - """ - Create commit status for project. - - :param build: Build to set up commit status for - :type build: Build - :param commit: commit sha of the pull/merge request - :type commit: str - :param status: build state failure, pending, or success. - :type status: str - :returns: boolean based on commit status creation was successful or not. - :rtype: Bool - """ + def sync_organizations(self): raise NotImplementedError - - @classmethod - def is_project_service(cls, project): - """ - Determine if this is the service the project is using. - - .. note:: - - This should be deprecated in favor of attaching the - :py:class:`RemoteRepository` to the project instance. This is a - slight improvement on the legacy check for webhooks - """ - # TODO Replace this check by keying project to remote repos - return ( - cls.url_pattern is not None - and cls.url_pattern.search(project.repo) is not None - ) diff --git a/readthedocs/oauth/services/bitbucket.py b/readthedocs/oauth/services/bitbucket.py index 7ffab66f3e8..992a7db3996 100644 --- a/readthedocs/oauth/services/bitbucket.py +++ b/readthedocs/oauth/services/bitbucket.py @@ -15,12 +15,12 @@ from ..constants import BITBUCKET from ..models import RemoteOrganization, RemoteRepository, RemoteRepositoryRelation -from .base import Service, SyncServiceError +from .base import SyncServiceError, UserService log = structlog.get_logger(__name__) -class BitbucketService(Service): +class BitbucketService(UserService): """Provider service for Bitbucket.""" @@ -29,6 +29,7 @@ class BitbucketService(Service): url_pattern = re.compile(r"bitbucket.org") https_url_pattern = re.compile(r"^https:\/\/[^@]+@bitbucket.org/") vcs_provider_slug = BITBUCKET + provider_name = "Bitbucket" def sync_repositories(self): """Sync repositories from Bitbucket API.""" @@ -393,3 +394,7 @@ def update_webhook(self, project, integration): log.exception("Bitbucket webhook update failed for project.") return (False, resp) + + def send_build_status(self, build, commit, status): + """Send build status is not supported/implemented for Bitbucket.""" + return True diff --git a/readthedocs/oauth/services/github.py b/readthedocs/oauth/services/github.py index fb95492a81d..4dacfb5a2ce 100644 --- a/readthedocs/oauth/services/github.py +++ b/readthedocs/oauth/services/github.py @@ -15,12 +15,12 @@ from ..constants import GITHUB from ..models import RemoteOrganization, RemoteRepository -from .base import Service, SyncServiceError +from .base import SyncServiceError, UserService log = structlog.get_logger(__name__) -class GitHubService(Service): +class GitHubService(UserService): """Provider service for GitHub.""" @@ -28,6 +28,7 @@ class GitHubService(Service): # TODO replace this with a less naive check url_pattern = re.compile(r"github\.com") vcs_provider_slug = GITHUB + provider_name = "GitHub" def sync_repositories(self): """Sync repositories from GitHub API.""" diff --git a/readthedocs/oauth/services/gitlab.py b/readthedocs/oauth/services/gitlab.py index ee809f92efb..4e65012c566 100644 --- a/readthedocs/oauth/services/gitlab.py +++ b/readthedocs/oauth/services/gitlab.py @@ -16,12 +16,12 @@ from ..constants import GITLAB from ..models import RemoteOrganization, RemoteRepository -from .base import Service, SyncServiceError +from .base import SyncServiceError, UserService log = structlog.get_logger(__name__) -class GitLabService(Service): +class GitLabService(UserService): """ Provider service for GitLab. @@ -31,6 +31,7 @@ class GitLabService(Service): - https://docs.gitlab.com/ce/api/oauth2.html """ + provider_name = "GitLab" adapter = GitLabOAuth2Adapter # Just use the network location to determine if it's a GitLab project # because private repos have another base url, eg. git@gitlab.example.com diff --git a/readthedocs/oauth/tasks.py b/readthedocs/oauth/tasks.py index c98ba45ce80..59125bbdfa9 100644 --- a/readthedocs/oauth/tasks.py +++ b/readthedocs/oauth/tasks.py @@ -3,7 +3,6 @@ import datetime import structlog -from allauth.socialaccount.providers import registry as allauth_registry from django.contrib.auth.models import User from django.db.models.functions import ExtractIsoWeekDay from django.urls import reverse @@ -153,8 +152,9 @@ def sync_active_users_remote_repositories(): log.exception("There was a problem re-syncing RemoteRepository.") +# TODO: remove user_pk from the signature on the next release. @app.task(queue="web") -def attach_webhook(project_pk, user_pk, integration=None): +def attach_webhook(project_pk, user_pk=None, integration=None, **kwargs): """ Add post-commit hook on project import. @@ -162,86 +162,67 @@ def attach_webhook(project_pk, user_pk, integration=None): all accounts until we set up a webhook. This should remain around for legacy connections -- that is, projects that do not have a remote repository them and were not set up with a VCS provider. + + :param project_pk: Project primary key + :param integration: Integration instance. If used, this function should + be called directly, not as a task. """ project = Project.objects.filter(pk=project_pk).first() - user = User.objects.filter(pk=user_pk).first() - - if not project or not user: + if not project: return False if integration: - service = SERVICE_MAP.get(integration.integration_type) - - if not service: - log.warning("There are no registered services in the application.") - Notification.objects.add( - message_id=MESSAGE_OAUTH_WEBHOOK_INVALID, - attached_to=project, - dismissable=True, - format_values={ - "url_integrations": reverse( - "projects_integrations", - args=[project.slug], - ), - }, - ) - return None + service_class = SERVICE_MAP.get(integration.integration_type) else: - for service_cls in registry: - if service_cls.is_project_service(project): - service = service_cls - break - else: - log.warning("There are no registered services in the application.") - Notification.objects.add( - message_id=MESSAGE_OAUTH_WEBHOOK_INVALID, - attached_to=project, - dismissable=True, - format_values={ - "url_integrations": reverse( - "projects_integrations", - args=[project.slug], - ), - }, - ) - return None - - provider_class = allauth_registry.get_class(service.adapter.provider_id) - - user_accounts = service.for_user(user) - for account in user_accounts: - success, __ = account.setup_webhook(project, integration=integration) - if success: - # NOTE: do we want to communicate that we connect the webhook here? - # messages.add_message(request, "Webhook successfully added.") + # Get the service class for the project e.g: GitHubService. + # We fallback to guess the service from the repo, + # in the future we should only consider projects that have a remote repository. + service_class = project.get_git_service_class(fallback_to_clone_url=True) - project.has_valid_webhook = True - project.save() - return True - - # No valid account found - if user_accounts: + if not service_class: Notification.objects.add( - message_id=MESSAGE_OAUTH_WEBHOOK_NO_PERMISSIONS, - dismissable=True, + message_id=MESSAGE_OAUTH_WEBHOOK_INVALID, attached_to=project, + dismissable=True, format_values={ - "provider_name": provider_class.name, - "url_docs_webhook": "https://docs.readthedocs.io/page/webhooks.html", + "url_integrations": reverse( + "projects_integrations", + args=[project.slug], + ), }, ) - else: + return False + + services = list(service_class.for_project(project)) + if not services: Notification.objects.add( message_id=MESSAGE_OAUTH_WEBHOOK_NO_ACCOUNT, dismissable=True, attached_to=project, format_values={ - "provider_name": provider_class.name, + "provider_name": service_class.vcs_provider_name, "url_connect_account": reverse( "projects_integrations", args=[project.slug], ), }, ) + return False + for service in service_class.for_project(project): + success, _ = service.setup_webhook(project, integration=integration) + if success: + project.has_valid_webhook = True + project.save() + return True + + Notification.objects.add( + message_id=MESSAGE_OAUTH_WEBHOOK_NO_PERMISSIONS, + dismissable=True, + attached_to=project, + format_values={ + "provider_name": service_class.vcs_provider_name, + "url_docs_webhook": "https://docs.readthedocs.io/page/webhooks.html", + }, + ) return False diff --git a/readthedocs/oauth/utils.py b/readthedocs/oauth/utils.py index 7d527e1a066..e71ca87b056 100644 --- a/readthedocs/oauth/utils.py +++ b/readthedocs/oauth/utils.py @@ -21,43 +21,26 @@ def update_webhook(project, integration, request=None): # FIXME: this method supports ``request=None`` on its definition. # However, it does not work when passing ``request=None`` as # it uses that object without checking if it's ``None`` or not. - service_cls = SERVICE_MAP.get(integration.integration_type) - if service_cls is None: + service_class = SERVICE_MAP.get(integration.integration_type) + if service_class is None: return None # TODO: remove after integrations without a secret are removed. if not integration.secret: integration.save() - updated = False - if project.remote_repository: - remote_repository_relations = ( - project.remote_repository.remote_repository_relations.filter( - account__isnull=False, user=request.user - ).select_related("account") - ) - - for relation in remote_repository_relations: - service = service_cls(request.user, relation.account) - updated, __ = service.update_webhook(project, integration) - - if updated: - break - else: - # The project was imported manually and doesn't have a RemoteRepository - # attached. We do brute force over all the accounts registered for this - # service - service_accounts = service_cls.for_user(request.user) - for service in service_accounts: - updated, __ = service.update_webhook(project, integration) - if updated: - break - - if updated: - messages.success(request, _("Webhook activated")) - project.has_valid_webhook = True - project.save() - return True + # The project was imported manually and doesn't have a RemoteRepository + # attached. We do brute force over all the accounts registered for this + # service + service_class = project.get_git_service_class() or service_class + + for service in service_class.for_project(project): + updated, __ = service.update_webhook(project, integration) + if updated: + messages.success(request, _("Webhook activated")) + project.has_valid_webhook = True + project.save() + return True messages.error( request, diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index a2ea85c862d..d16b4872ced 100644 --- a/readthedocs/projects/models.py +++ b/readthedocs/projects/models.py @@ -7,7 +7,6 @@ from urllib.parse import urlparse import structlog -from allauth.socialaccount.providers import registry as allauth_registry from django.conf import settings from django.contrib.auth.models import User from django.contrib.contenttypes.fields import GenericRelation @@ -1025,28 +1024,36 @@ def vcs_class(self): """ return backend_cls.get(self.repo_type) - def git_service_class(self): - """Get the service class for project. e.g: GitHubService, GitLabService.""" + def _guess_service_class(self): from readthedocs.oauth.services import registry for service_cls in registry: if service_cls.is_project_service(self): - service = service_cls - break - else: - log.warning("There are no registered services in the application.") - service = None + return service_cls + return None - return service + def get_git_service_class(self, fallback_to_clone_url=False): + """ + Get the service class for project. e.g: GitHubService, GitLabService. + + :param fallback_to_clone_url: If the project doesn't have a remote repository, + we try to guess the service class based on the clone URL. + """ + service_cls = None + if self.has_feature(Feature.DONT_SYNC_WITH_REMOTE_REPO): + return self._guess_service_class() + service_cls = ( + self.remote_repository and self.remote_repository.get_service_class() + ) + if not service_cls and fallback_to_clone_url: + return self._guess_service_class() + return service_cls @property def git_provider_name(self): """Get the provider name for project. e.g: GitHub, GitLab, Bitbucket.""" - service = self.git_service_class() - if service: - provider_class = allauth_registry.get_class(service.adapter.provider_id) - return provider_class.name - return None + service_class = self.get_git_service_class() + return service_class.provider_name if service_class else None def find(self, filename, version): """ diff --git a/readthedocs/projects/views/mixins.py b/readthedocs/projects/views/mixins.py index 66c0fa92623..1a112299fab 100644 --- a/readthedocs/projects/views/mixins.py +++ b/readthedocs/projects/views/mixins.py @@ -140,6 +140,8 @@ def trigger_initial_build(self, project, user): from readthedocs.oauth.tasks import attach_webhook task_promise = chain( + # TODO: Remove user_pk on the next release, + # it's used just to keep backward compatibility with the old task signature. attach_webhook.si(project.pk, user.pk), update_docs, ) diff --git a/readthedocs/projects/views/private.py b/readthedocs/projects/views/private.py index 56812ec4d3a..aeacb38c81c 100644 --- a/readthedocs/projects/views/private.py +++ b/readthedocs/projects/views/private.py @@ -999,8 +999,10 @@ def form_valid(self, form): if self.object.has_sync: attach_webhook( project_pk=self.get_project().pk, - user_pk=self.request.user.pk, integration=self.object, + # TODO: Remove user_pk on the next release, + # it's used just to keep backward compatibility with the old task signature. + user_pk=None, ) return HttpResponseRedirect(self.get_success_url()) @@ -1056,7 +1058,9 @@ def post(self, request, *args, **kwargs): # the per-integration sync instead. attach_webhook( project_pk=self.get_project().pk, - user_pk=request.user.pk, + # TODO: Remove user_pk on the next release, + # it's used just to keep backward compatibility with the old task signature. + user_pk=None, ) return HttpResponseRedirect(self.get_success_url()) From f031b4b3b453d41b5959260220d8c650c8cfe38d Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 6 Feb 2025 19:10:47 -0500 Subject: [PATCH 2/8] Fix name --- readthedocs/oauth/tasks.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/readthedocs/oauth/tasks.py b/readthedocs/oauth/tasks.py index 59125bbdfa9..c157da99bf6 100644 --- a/readthedocs/oauth/tasks.py +++ b/readthedocs/oauth/tasks.py @@ -200,7 +200,7 @@ def attach_webhook(project_pk, user_pk=None, integration=None, **kwargs): dismissable=True, attached_to=project, format_values={ - "provider_name": service_class.vcs_provider_name, + "provider_name": service_class.provider_name, "url_connect_account": reverse( "projects_integrations", args=[project.slug], @@ -221,7 +221,7 @@ def attach_webhook(project_pk, user_pk=None, integration=None, **kwargs): dismissable=True, attached_to=project, format_values={ - "provider_name": service_class.vcs_provider_name, + "provider_name": service_class.provider_name, "url_docs_webhook": "https://docs.readthedocs.io/page/webhooks.html", }, ) From f7a3917b3d447af17bc322860dadcabce4415350 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 10 Feb 2025 11:24:29 -0500 Subject: [PATCH 3/8] Fix tests --- readthedocs/projects/models.py | 2 +- readthedocs/rtd_tests/tests/test_celery.py | 7 ++++--- readthedocs/rtd_tests/tests/test_oauth.py | 4 ++-- readthedocs/rtd_tests/tests/test_oauth_sync.py | 2 +- readthedocs/rtd_tests/tests/test_project.py | 10 ++++++++-- readthedocs/rtd_tests/tests/test_project_views.py | 2 +- 6 files changed, 17 insertions(+), 10 deletions(-) diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index d16b4872ced..7b4a33d0cbd 100644 --- a/readthedocs/projects/models.py +++ b/readthedocs/projects/models.py @@ -1052,7 +1052,7 @@ def get_git_service_class(self, fallback_to_clone_url=False): @property def git_provider_name(self): """Get the provider name for project. e.g: GitHub, GitLab, Bitbucket.""" - service_class = self.get_git_service_class() + service_class = self.get_git_service_class(fallback_to_clone_url=True) return service_class.provider_name if service_class else None def find(self, filename, version): diff --git a/readthedocs/rtd_tests/tests/test_celery.py b/readthedocs/rtd_tests/tests/test_celery.py index b64e91233fe..cd0bcf7f366 100644 --- a/readthedocs/rtd_tests/tests/test_celery.py +++ b/readthedocs/rtd_tests/tests/test_celery.py @@ -10,6 +10,7 @@ from readthedocs.builds.constants import BUILD_STATUS_SUCCESS, EXTERNAL, LATEST from readthedocs.builds.models import Build, Version from readthedocs.notifications.models import Notification +from readthedocs.oauth.constants import GITHUB, GITLAB from readthedocs.oauth.models import RemoteRepository, RemoteRepositoryRelation from readthedocs.oauth.notifications import MESSAGE_OAUTH_BUILD_STATUS_FAILURE from readthedocs.projects.models import Project @@ -85,8 +86,8 @@ def test_send_build_status_with_remote_repo_github(self, send_build_status): self.project.repo = "https://github.com/test/test/" self.project.save() - social_account = get(SocialAccount, user=self.eric, provider="gitlab") - remote_repo = get(RemoteRepository) + social_account = get(SocialAccount, user=self.eric, provider="github") + remote_repo = get(RemoteRepository, vcs_provider=GITHUB) remote_repo.projects.add(self.project) get( RemoteRepositoryRelation, @@ -159,7 +160,7 @@ def test_send_build_status_with_remote_repo_gitlab(self, send_build_status): self.project.save() social_account = get(SocialAccount, user=self.eric, provider="gitlab") - remote_repo = get(RemoteRepository) + remote_repo = get(RemoteRepository, vcs_provider=GITLAB) remote_repo.projects.add(self.project) get( RemoteRepositoryRelation, diff --git a/readthedocs/rtd_tests/tests/test_oauth.py b/readthedocs/rtd_tests/tests/test_oauth.py index 9e84886e0b5..ba206f12a7a 100644 --- a/readthedocs/rtd_tests/tests/test_oauth.py +++ b/readthedocs/rtd_tests/tests/test_oauth.py @@ -254,7 +254,7 @@ def test_make_organization(self): def test_import_with_no_token(self): """User without a GitHub SocialToken does not return a service.""" - services = GitHubService.for_user(get(User)) + services = list(GitHubService.for_user(get(User))) self.assertEqual(services, []) def test_multiple_users_same_repo(self): @@ -776,7 +776,7 @@ def test_make_organization(self): def test_import_with_no_token(self): """User without a Bitbucket SocialToken does not return a service.""" - services = BitbucketService.for_user(get(User)) + services = list(BitbucketService.for_user(get(User))) self.assertEqual(services, []) @mock.patch("readthedocs.oauth.services.bitbucket.log") diff --git a/readthedocs/rtd_tests/tests/test_oauth_sync.py b/readthedocs/rtd_tests/tests/test_oauth_sync.py index 43bc308de5c..5773f6e6357 100644 --- a/readthedocs/rtd_tests/tests/test_oauth_sync.py +++ b/readthedocs/rtd_tests/tests/test_oauth_sync.py @@ -70,7 +70,7 @@ def setUp(self): SocialToken, account=self.socialaccount, ) - self.service = GitHubService.for_user(self.user)[0] + self.service = list(GitHubService.for_user(self.user))[0] @requests_mock.Mocker(kw="mock_request") def test_sync_delete_stale(self, mock_request): diff --git a/readthedocs/rtd_tests/tests/test_project.py b/readthedocs/rtd_tests/tests/test_project.py index 7a1e238f302..befa60bccb1 100644 --- a/readthedocs/rtd_tests/tests/test_project.py +++ b/readthedocs/rtd_tests/tests/test_project.py @@ -239,7 +239,10 @@ def test_git_provider_name_github(self): def test_git_service_class_github(self): self.pip.repo = "https://github.com/pypa/pip" self.pip.save() - self.assertEqual(self.pip.git_service_class(), GitHubService) + self.assertEqual(self.pip.get_git_service_class(), None) + self.assertEqual( + self.pip.get_git_service_class(fallback_to_clone_url=True), GitHubService + ) def test_git_provider_name_gitlab(self): self.pip.repo = "https://gitlab.com/pypa/pip" @@ -249,7 +252,10 @@ def test_git_provider_name_gitlab(self): def test_git_service_class_gitlab(self): self.pip.repo = "https://gitlab.com/pypa/pip" self.pip.save() - self.assertEqual(self.pip.git_service_class(), GitLabService) + self.assertEqual(self.pip.get_git_service_class(), None) + self.assertEqual( + self.pip.get_git_service_class(fallback_to_clone_url=True), GitLabService + ) @mock.patch("readthedocs.projects.forms.trigger_build", mock.MagicMock()) diff --git a/readthedocs/rtd_tests/tests/test_project_views.py b/readthedocs/rtd_tests/tests/test_project_views.py index e760c64a690..2f78690af71 100644 --- a/readthedocs/rtd_tests/tests/test_project_views.py +++ b/readthedocs/rtd_tests/tests/test_project_views.py @@ -455,8 +455,8 @@ def test_integration_create(self, attach_webhook): self.assertEqual(response.status_code, 302) attach_webhook.assert_called_once_with( project_pk=self.project.pk, - user_pk=self.user.pk, integration=integration.first(), + user_pk=None, ) @mock.patch("readthedocs.projects.views.private.attach_webhook") From 083cfa012a4e3e0218a0f7eca80ff2c33e0c582e Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 10 Feb 2025 12:08:44 -0500 Subject: [PATCH 4/8] Small updates --- readthedocs/oauth/services/base.py | 7 ++++--- readthedocs/oauth/tasks.py | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/readthedocs/oauth/services/base.py b/readthedocs/oauth/services/base.py index 7f570acd3ab..8596303e3f0 100644 --- a/readthedocs/oauth/services/base.py +++ b/readthedocs/oauth/services/base.py @@ -2,6 +2,7 @@ from datetime import datetime import structlog +import re from allauth.socialaccount.models import SocialAccount from allauth.socialaccount.providers.oauth2.views import OAuth2Adapter from django.conf import settings @@ -32,7 +33,7 @@ class Service: """Base class for service that interacts with a VCS provider and a project.""" vcs_provider_slug: str - url_pattern: str + url_pattern: re.Pattern | None provider_name: str default_user_avatar_url = settings.OAUTH_AVATAR_USER_DEFAULT_URL default_org_avatar_url = settings.OAUTH_AVATAR_ORG_DEFAULT_URL @@ -53,8 +54,8 @@ def sync(self): - Creates a new RemoteRepository/Organization per new repository - Updates fields for existing RemoteRepository/Organization - - Deletes old RemoteRepository/Organization that are not present - for this user in the current provider + - Deletes old RemoteRepository/Organization that are no longer present + in this provider. """ raise NotImplementedError diff --git a/readthedocs/oauth/tasks.py b/readthedocs/oauth/tasks.py index c157da99bf6..13a6185c1f1 100644 --- a/readthedocs/oauth/tasks.py +++ b/readthedocs/oauth/tasks.py @@ -209,7 +209,7 @@ def attach_webhook(project_pk, user_pk=None, integration=None, **kwargs): ) return False - for service in service_class.for_project(project): + for service in services: success, _ = service.setup_webhook(project, integration=integration) if success: project.has_valid_webhook = True From 8e6caa3e6157a0230c0989784e8a421cacd7f7bd Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 10 Feb 2025 12:18:49 -0500 Subject: [PATCH 5/8] Format --- readthedocs/oauth/services/base.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/oauth/services/base.py b/readthedocs/oauth/services/base.py index 8596303e3f0..941a7f277b5 100644 --- a/readthedocs/oauth/services/base.py +++ b/readthedocs/oauth/services/base.py @@ -1,8 +1,8 @@ """OAuth utility functions.""" +import re from datetime import datetime import structlog -import re from allauth.socialaccount.models import SocialAccount from allauth.socialaccount.providers.oauth2.views import OAuth2Adapter from django.conf import settings From 7fc1466a2b09c6b7d0dd97ec21cc7569a5c97250 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 12 Feb 2025 17:19:24 -0500 Subject: [PATCH 6/8] Updates from review --- readthedocs/builds/tasks.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/readthedocs/builds/tasks.py b/readthedocs/builds/tasks.py index 7e13ca62d19..1a085b823e1 100644 --- a/readthedocs/builds/tasks.py +++ b/readthedocs/builds/tasks.py @@ -407,7 +407,7 @@ def send_build_status(build_pk, commit, status): # in the future we should only consider projects that have a remote repository. service_class = build.project.get_git_service_class(fallback_to_clone_url=True) if not service_class: - log.info("Project isn't connected to a Git service.") + log.info("Project isn't connected to a Git service, not sending build status.") return False for service in service_class.for_project(build.project): @@ -430,7 +430,7 @@ def send_build_status(build_pk, commit, status): dismissable=True, ) - log.info("No social account or repository permission available.") + log.info("No social account or repository permission available, no build status sent.") return False From 27ad8c6a21dba388e6c7feb61206ea329f9a65de Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 12 Feb 2025 17:28:59 -0500 Subject: [PATCH 7/8] Skip incompatible integrations --- readthedocs/oauth/utils.py | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/readthedocs/oauth/utils.py b/readthedocs/oauth/utils.py index e71ca87b056..544376ac7af 100644 --- a/readthedocs/oauth/utils.py +++ b/readthedocs/oauth/utils.py @@ -29,10 +29,19 @@ def update_webhook(project, integration, request=None): if not integration.secret: integration.save() - # The project was imported manually and doesn't have a RemoteRepository - # attached. We do brute force over all the accounts registered for this - # service - service_class = project.get_git_service_class() or service_class + # If the integration's service class is different from the project's + # git service class, we skip the update, as the webhook is not valid + # (we can't create a GitHub webhook for a GitLab project, for example). + if service_class != project.get_git_service_class(fallback_to_clone_url=True): + messages.error( + request, + _( + "This integration type is not compatible with the project's Git provider." + ), + ) + project.has_valid_webhook = False + project.save() + return False for service in service_class.for_project(project): updated, __ = service.update_webhook(project, integration) From 5daf4da3eac8e7219c23e1348695e19b0163f7e8 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 12 Feb 2025 23:05:26 -0500 Subject: [PATCH 8/8] Check for attribute instead --- readthedocs/builds/tasks.py | 8 +++++++- readthedocs/oauth/services/base.py | 1 + readthedocs/oauth/services/bitbucket.py | 4 ---- readthedocs/oauth/services/github.py | 1 + readthedocs/oauth/services/gitlab.py | 1 + 5 files changed, 10 insertions(+), 5 deletions(-) diff --git a/readthedocs/builds/tasks.py b/readthedocs/builds/tasks.py index 1a085b823e1..211bcdf436c 100644 --- a/readthedocs/builds/tasks.py +++ b/readthedocs/builds/tasks.py @@ -410,6 +410,10 @@ def send_build_status(build_pk, commit, status): log.info("Project isn't connected to a Git service, not sending build status.") return False + if not service_class.supports_build_status: + log.info("Git service doesn't support build status.") + return False + for service in service_class.for_project(build.project): success = service.send_build_status( build, @@ -430,7 +434,9 @@ def send_build_status(build_pk, commit, status): dismissable=True, ) - log.info("No social account or repository permission available, no build status sent.") + log.info( + "No social account or repository permission available, no build status sent." + ) return False diff --git a/readthedocs/oauth/services/base.py b/readthedocs/oauth/services/base.py index 941a7f277b5..b2c30a78f62 100644 --- a/readthedocs/oauth/services/base.py +++ b/readthedocs/oauth/services/base.py @@ -37,6 +37,7 @@ class Service: provider_name: str default_user_avatar_url = settings.OAUTH_AVATAR_USER_DEFAULT_URL default_org_avatar_url = settings.OAUTH_AVATAR_ORG_DEFAULT_URL + supports_build_status = False @classmethod def for_project(self, project): diff --git a/readthedocs/oauth/services/bitbucket.py b/readthedocs/oauth/services/bitbucket.py index 992a7db3996..193c11ba03c 100644 --- a/readthedocs/oauth/services/bitbucket.py +++ b/readthedocs/oauth/services/bitbucket.py @@ -394,7 +394,3 @@ def update_webhook(self, project, integration): log.exception("Bitbucket webhook update failed for project.") return (False, resp) - - def send_build_status(self, build, commit, status): - """Send build status is not supported/implemented for Bitbucket.""" - return True diff --git a/readthedocs/oauth/services/github.py b/readthedocs/oauth/services/github.py index 4dacfb5a2ce..e0f22457bd7 100644 --- a/readthedocs/oauth/services/github.py +++ b/readthedocs/oauth/services/github.py @@ -29,6 +29,7 @@ class GitHubService(UserService): url_pattern = re.compile(r"github\.com") vcs_provider_slug = GITHUB provider_name = "GitHub" + supports_build_status = True def sync_repositories(self): """Sync repositories from GitHub API.""" diff --git a/readthedocs/oauth/services/gitlab.py b/readthedocs/oauth/services/gitlab.py index 4e65012c566..9050bcfabbb 100644 --- a/readthedocs/oauth/services/gitlab.py +++ b/readthedocs/oauth/services/gitlab.py @@ -33,6 +33,7 @@ class GitLabService(UserService): provider_name = "GitLab" adapter = GitLabOAuth2Adapter + supports_build_status = True # Just use the network location to determine if it's a GitLab project # because private repos have another base url, eg. git@gitlab.example.com url_pattern = re.compile(