From fa53c1c2ec904396c549dfc74b935ba766f063c1 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 6 Feb 2025 19:00:44 -0500 Subject: [PATCH 01/11] 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 02/11] 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 03/11] 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 04/11] 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 05/11] 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 06/11] 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 07/11] 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 08/11] 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( From 12892987dbc01abe9366c785086fad379307df0a Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 13 Feb 2025 13:34:08 -0500 Subject: [PATCH 09/11] Git service: attach each service to a allauth provider --- readthedocs/api/v2/views/model_views.py | 4 +- readthedocs/builds/tasks.py | 2 +- readthedocs/oauth/services/base.py | 111 ++------------ readthedocs/oauth/services/bitbucket.py | 29 ++-- readthedocs/oauth/services/github.py | 34 ++--- readthedocs/oauth/services/gitlab.py | 40 +++-- readthedocs/oauth/tasks.py | 6 +- readthedocs/projects/models.py | 2 +- readthedocs/projects/views/private.py | 2 +- readthedocs/rtd_tests/tests/test_oauth.py | 174 ++++++++++------------ 10 files changed, 148 insertions(+), 256 deletions(-) diff --git a/readthedocs/api/v2/views/model_views.py b/readthedocs/api/v2/views/model_views.py index b730a42bac8..7317a630999 100644 --- a/readthedocs/api/v2/views/model_views.py +++ b/readthedocs/api/v2/views/model_views.py @@ -420,7 +420,7 @@ def get_queryset(self): self.model.objects.api_v2(self.request.user) .filter( remote_organization_relations__account__provider__in=[ - service.adapter.provider_id for service in registry + service.allauth_provider.id for service in registry ] ) .distinct() @@ -466,7 +466,7 @@ def get_queryset(self): query = query.filter( remote_repository_relations__account__provider__in=[ - service.adapter.provider_id for service in registry + service.allauth_provider.id for service in registry ], ).distinct() diff --git a/readthedocs/builds/tasks.py b/readthedocs/builds/tasks.py index 211bcdf436c..464df1da544 100644 --- a/readthedocs/builds/tasks.py +++ b/readthedocs/builds/tasks.py @@ -428,7 +428,7 @@ def send_build_status(build_pk, commit, status): message_id=MESSAGE_OAUTH_BUILD_STATUS_FAILURE, attached_to=build.project, format_values={ - "provider_name": service_class.provider_name, + "provider_name": service_class.allauth_provider.name, "url_connect_account": reverse("socialaccount_connections"), }, dismissable=True, diff --git a/readthedocs/oauth/services/base.py b/readthedocs/oauth/services/base.py index b2c30a78f62..b8a4a42a27e 100644 --- a/readthedocs/oauth/services/base.py +++ b/readthedocs/oauth/services/base.py @@ -1,19 +1,18 @@ """OAuth utility functions.""" import re -from datetime import datetime +from functools import cached_property import structlog from allauth.socialaccount.models import SocialAccount -from allauth.socialaccount.providers.oauth2.views import OAuth2Adapter +from allauth.socialaccount.providers.oauth2.provider import OAuth2Provider from django.conf import settings from django.urls import reverse -from django.utils import timezone from django.utils.translation import gettext_lazy as _ from oauthlib.oauth2.rfc6749.errors import InvalidClientIdError from requests.exceptions import RequestException -from requests_oauthlib import OAuth2Session from readthedocs.core.permissions import AdminPermission +from readthedocs.oauth.clients import get_oauth2_client log = structlog.get_logger(__name__) @@ -33,8 +32,9 @@ class Service: """Base class for service that interacts with a VCS provider and a project.""" vcs_provider_slug: str + allauth_provider = type[OAuth2Provider] + 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 supports_build_status = False @@ -127,15 +127,12 @@ class UserService(Service): :param account: :py:class:`SocialAccount` instance for user """ - adapter = None - def __init__(self, user, account): - self.session = None self.user = user self.account = account log.bind( user_username=self.user.username, - social_provider=self.provider_id, + social_provider=self.allauth_provider.id, social_account_id=self.account.pk, ) @@ -149,96 +146,14 @@ def for_project(cls, project): def for_user(cls, user): accounts = SocialAccount.objects.filter( user=user, - provider=cls.adapter.provider_id, + provider=cls.allauth_provider.id, ) for account in accounts: yield cls(user=user, account=account) - def get_adapter(self) -> type[OAuth2Adapter]: - return self.adapter - - @property - def provider_id(self): - return self.get_adapter().provider_id - - def get_session(self): - if self.session is None: - self.create_session() - return self.session - - def get_access_token_url(self): - # ``access_token_url`` is a property in some adapters, - # so we need to instantiate it to get the actual value. - # pylint doesn't recognize that get_adapter returns a class. - # pylint: disable=not-callable - adapter = self.get_adapter()(request=None) - return adapter.access_token_url - - def create_session(self): - """ - Create OAuth session for user. - - This configures the OAuth session based on the :py:class:`SocialToken` - attributes. If there is an ``expires_at``, treat the session as an auto - renewing token. Some providers expire tokens after as little as 2 hours. - """ - token = self.account.socialtoken_set.first() - if token is None: - return None - - token_config = { - "access_token": token.token, - "token_type": "bearer", - } - if token.expires_at is not None: - token_expires = (token.expires_at - timezone.now()).total_seconds() - token_config.update( - { - "refresh_token": token.token_secret, - "expires_in": token_expires, - } - ) - - social_app = self.account.get_provider().app - self.session = OAuth2Session( - client_id=social_app.client_id, - token=token_config, - auto_refresh_kwargs={ - "client_id": social_app.client_id, - "client_secret": social_app.secret, - }, - auto_refresh_url=self.get_access_token_url(), - token_updater=self.token_updater(token), - ) - - return self.session or None - - def token_updater(self, token): - """ - Update token given data from OAuth response. - - Expect the following response into the closure:: - - { - u'token_type': u'bearer', - u'scopes': u'webhook repository team account', - u'refresh_token': u'...', - u'access_token': u'...', - u'expires_in': 3600, - u'expires_at': 1449218652.558185 - } - """ - - def _updater(data): - token.token = data["access_token"] - token.token_secret = data.get("refresh_token", "") - token.expires_at = timezone.make_aware( - datetime.fromtimestamp(data["expires_at"]), - ) - token.save() - log.info("Updated token.", token_id=token.pk) - - return _updater + @cached_property + def session(self): + return get_oauth2_client(self.account) def paginate(self, url, **kwargs): """ @@ -251,7 +166,7 @@ def paginate(self, url, **kwargs): """ resp = None try: - resp = self.get_session().get(url, params=kwargs) + resp = self.session.get(url, params=kwargs) # TODO: this check of the status_code would be better in the # ``create_session`` method since it could be used from outside, but @@ -263,7 +178,7 @@ def paginate(self, url, **kwargs): # needs to reconnect his account raise SyncServiceError( SyncServiceError.INVALID_OR_REVOKED_ACCESS_TOKEN.format( - provider=self.provider_name + provider=self.allauth_provider.name ) ) @@ -277,7 +192,7 @@ def paginate(self, url, **kwargs): log.warning("access_token or refresh_token failed.", url=url) raise SyncServiceError( SyncServiceError.INVALID_OR_REVOKED_ACCESS_TOKEN.format( - provider=self.provider_name + provider=self.allauth_provider.name ) ) # Catch exceptions with request or deserializing JSON diff --git a/readthedocs/oauth/services/bitbucket.py b/readthedocs/oauth/services/bitbucket.py index 193c11ba03c..60f36ae4ac8 100644 --- a/readthedocs/oauth/services/bitbucket.py +++ b/readthedocs/oauth/services/bitbucket.py @@ -4,8 +4,8 @@ import re import structlog -from allauth.socialaccount.providers.bitbucket_oauth2.views import ( - BitbucketOAuth2Adapter, +from allauth.socialaccount.providers.bitbucket_oauth2.provider import ( + BitbucketOAuth2Provider, ) from django.conf import settings from requests.exceptions import RequestException @@ -24,12 +24,12 @@ class BitbucketService(UserService): """Provider service for Bitbucket.""" - adapter = BitbucketOAuth2Adapter + vcs_provider_slug = BITBUCKET + allauth_provider = BitbucketOAuth2Provider + base_api_url = "https://api.bitbucket.org/2.0" # TODO replace this with a less naive check 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.""" @@ -49,7 +49,7 @@ def sync_repositories(self): log.warning("Error syncing Bitbucket repositories") raise SyncServiceError( SyncServiceError.INVALID_OR_REVOKED_ACCESS_TOKEN.format( - provider=self.vcs_provider_slug + provider=self.allauth_provider.name ) ) @@ -82,7 +82,7 @@ def sync_organizations(self): try: workspaces = self.paginate( - "https://api.bitbucket.org/2.0/workspaces/", + f"{self.base_api_url}/workspaces/", role="member", ) for workspace in workspaces: @@ -102,7 +102,7 @@ def sync_organizations(self): log.warning("Error syncing Bitbucket organizations") raise SyncServiceError( SyncServiceError.INVALID_OR_REVOKED_ACCESS_TOKEN.format( - provider=self.vcs_provider_slug + provider=self.allauth_provider.name ) ) @@ -235,9 +235,8 @@ def get_provider_data(self, project, integration): if integration.provider_data: return integration.provider_data - session = self.get_session() owner, repo = build_utils.get_bitbucket_username_repo(url=project.repo) - url = f"https://api.bitbucket.org/2.0/repositories/{owner}/{repo}/hooks" + url = f"{self.base_api_url}/repositories/{owner}/{repo}/hooks" rtd_webhook_url = self.get_webhook_url(project, integration) @@ -247,7 +246,7 @@ def get_provider_data(self, project, integration): url=url, ) try: - resp = session.get(url) + resp = self.session.get(url) if resp.status_code == 200: recv_data = resp.json() @@ -284,9 +283,8 @@ def setup_webhook(self, project, integration=None): :returns: boolean based on webhook set up success, and requests Response object :rtype: (Bool, Response) """ - session = self.get_session() owner, repo = build_utils.get_bitbucket_username_repo(url=project.repo) - url = f"https://api.bitbucket.org/2.0/repositories/{owner}/{repo}/hooks" + url = f"{self.base_api_url}/repositories/{owner}/{repo}/hooks" if not integration: integration, _ = Integration.objects.get_or_create( project=project, @@ -302,7 +300,7 @@ def setup_webhook(self, project, integration=None): ) try: - resp = session.post( + resp = self.session.post( url, data=data, headers={"content-type": "application/json"}, @@ -355,13 +353,12 @@ def update_webhook(self, project, integration): if not provider_data: return self.setup_webhook(project, integration) - session = self.get_session() data = self.get_webhook_data(project, integration) resp = None try: # Expect to throw KeyError here if provider_data is invalid url = provider_data["links"]["self"]["href"] - resp = session.put( + resp = self.session.put( url, data=data, headers={"content-type": "application/json"}, diff --git a/readthedocs/oauth/services/github.py b/readthedocs/oauth/services/github.py index e0f22457bd7..e60c305c7f2 100644 --- a/readthedocs/oauth/services/github.py +++ b/readthedocs/oauth/services/github.py @@ -4,7 +4,7 @@ import re import structlog -from allauth.socialaccount.providers.github.views import GitHubOAuth2Adapter +from allauth.socialaccount.providers.github.provider import GitHubProvider from django.conf import settings from oauthlib.oauth2.rfc6749.errors import InvalidGrantError, TokenExpiredError from requests.exceptions import RequestException @@ -24,11 +24,11 @@ class GitHubService(UserService): """Provider service for GitHub.""" - adapter = GitHubOAuth2Adapter + vcs_provider_slug = GITHUB + allauth_provider = GitHubProvider + base_api_url = "https://api.github.com" # TODO replace this with a less naive check url_pattern = re.compile(r"github\.com") - vcs_provider_slug = GITHUB - provider_name = "GitHub" supports_build_status = True def sync_repositories(self): @@ -36,7 +36,7 @@ def sync_repositories(self): remote_repositories = [] try: - repos = self.paginate("https://api.github.com/user/repos", per_page=100) + repos = self.paginate(f"{self.base_api_url}/user/repos", per_page=100) for repo in repos: remote_repository = self.create_repository(repo) remote_repositories.append(remote_repository) @@ -55,9 +55,9 @@ def sync_organizations(self): remote_repositories = [] try: - orgs = self.paginate("https://api.github.com/user/orgs", per_page=100) + orgs = self.paginate(f"{self.base_api_url}/user/orgs", per_page=100) for org in orgs: - org_details = self.get_session().get(org["url"]).json() + org_details = self.session.get(org["url"]).json() remote_organization = self.create_organization( org_details, create_user_relationship=True, @@ -77,7 +77,7 @@ def sync_organizations(self): log.warning("Error syncing GitHub organizations") raise SyncServiceError( SyncServiceError.INVALID_OR_REVOKED_ACCESS_TOKEN.format( - provider=self.vcs_provider_slug + provider=self.allauth_provider.name ) ) @@ -241,9 +241,8 @@ def get_provider_data(self, project, integration): if integration.provider_data: return integration.provider_data - session = self.get_session() owner, repo = build_utils.get_github_username_repo(url=project.repo) - url = f"https://api.github.com/repos/{owner}/{repo}/hooks" + url = f"{self.base_api_url}/repos/{owner}/{repo}/hooks" log.bind( url=url, project_slug=project.slug, @@ -253,7 +252,7 @@ def get_provider_data(self, project, integration): rtd_webhook_url = self.get_webhook_url(project, integration) try: - resp = session.get(url) + resp = self.session.get(url) if resp.status_code == 200: recv_data = resp.json() @@ -288,7 +287,6 @@ def setup_webhook(self, project, integration=None): :returns: boolean based on webhook set up success, and requests Response object :rtype: (Bool, Response) """ - session = self.get_session() owner, repo = build_utils.get_github_username_repo(url=project.repo) if not integration: @@ -298,7 +296,7 @@ def setup_webhook(self, project, integration=None): ) data = self.get_webhook_data(project, integration) - url = f"https://api.github.com/repos/{owner}/{repo}/hooks" + url = f"{self.base_api_url}/repos/{owner}/{repo}/hooks" log.bind( url=url, project_slug=project.slug, @@ -306,7 +304,7 @@ def setup_webhook(self, project, integration=None): ) resp = None try: - resp = session.post( + resp = self.session.post( url, data=data, headers={"content-type": "application/json"}, @@ -353,7 +351,6 @@ def update_webhook(self, project, integration): :returns: boolean based on webhook update success, and requests Response object :rtype: (Bool, Response) """ - session = self.get_session() data = self.get_webhook_data(project, integration) resp = None @@ -370,7 +367,7 @@ def update_webhook(self, project, integration): try: url = provider_data.get("url") - resp = session.patch( + resp = self.session.patch( url, data=data, headers={"content-type": "application/json"}, @@ -422,14 +419,13 @@ def send_build_status(self, build, commit, status): :returns: boolean based on commit status creation was successful or not. :rtype: Bool """ - session = self.get_session() project = build.project owner, repo = build_utils.get_github_username_repo(url=project.repo) # select the correct status and description. github_build_status = SELECT_BUILD_STATUS[status]["github"] description = SELECT_BUILD_STATUS[status]["description"] - statuses_url = f"https://api.github.com/repos/{owner}/{repo}/statuses/{commit}" + statuses_url = f"{self.base_api_url}/repos/{owner}/{repo}/statuses/{commit}" if status == BUILD_STATUS_SUCCESS: # Link to the documentation for this version @@ -457,7 +453,7 @@ def send_build_status(self, build, commit, status): ) resp = None try: - resp = session.post( + resp = self.session.post( statuses_url, data=json.dumps(data), headers={"content-type": "application/json"}, diff --git a/readthedocs/oauth/services/gitlab.py b/readthedocs/oauth/services/gitlab.py index 9050bcfabbb..bb2dc21813d 100644 --- a/readthedocs/oauth/services/gitlab.py +++ b/readthedocs/oauth/services/gitlab.py @@ -5,7 +5,7 @@ from urllib.parse import quote_plus, urlparse import structlog -from allauth.socialaccount.providers.gitlab.views import GitLabOAuth2Adapter +from allauth.socialaccount.providers.gitlab.provider import GitLabProvider from django.conf import settings from oauthlib.oauth2.rfc6749.errors import InvalidGrantError, TokenExpiredError from requests.exceptions import RequestException @@ -31,13 +31,13 @@ class GitLabService(UserService): - https://docs.gitlab.com/ce/api/oauth2.html """ - provider_name = "GitLab" - adapter = GitLabOAuth2Adapter + allauth_provider = GitLabProvider + base_api_url = "https://gitlab.com" 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( - re.escape(urlparse(adapter.provider_default_url).netloc), + re.escape(urlparse(base_api_url).netloc), ) PERMISSION_NO_ACCESS = 0 @@ -75,7 +75,7 @@ def sync_repositories(self): remote_repositories = [] try: repos = self.paginate( - "{url}/api/v4/projects".format(url=self.adapter.provider_default_url), + f"{self.base_api_url}/api/v4/projects", per_page=100, archived=False, order_by="path", @@ -102,7 +102,7 @@ def sync_organizations(self): try: orgs = self.paginate( - "{url}/api/v4/groups".format(url=self.adapter.provider_default_url), + f"{self.base_api_url}/api/v4/groups", per_page=100, all_available=False, order_by="path", @@ -112,7 +112,7 @@ def sync_organizations(self): remote_organization = self.create_organization(org) org_repos = self.paginate( "{url}/api/v4/groups/{id}/projects".format( - url=self.adapter.provider_default_url, + url=self.base_api_url, id=org["id"], ), per_page=100, @@ -131,9 +131,9 @@ def sync_organizations(self): # admin permission fields for GitLab projects. # So, fetch every single project data from the API # which contains the admin permission fields. - resp = self.get_session().get( + resp = self.session.get( "{url}/api/v4/projects/{id}".format( - url=self.adapter.provider_default_url, id=repo["id"] + url=self.base_api_url, id=repo["id"] ) ) @@ -272,7 +272,7 @@ def create_organization(self, fields): organization.name = fields.get("name") organization.slug = fields.get("path") organization.url = "{url}/{path}".format( - url=self.adapter.provider_default_url, + url=self.base_api_url, path=fields.get("path"), ) organization.avatar_url = fields.get("avatar_url") @@ -327,7 +327,6 @@ def get_provider_data(self, project, integration): if repo_id is None: return None - session = self.get_session() log.bind( project_slug=project.slug, integration_id=integration.pk, @@ -336,9 +335,9 @@ def get_provider_data(self, project, integration): rtd_webhook_url = self.get_webhook_url(project, integration) try: - resp = session.get( + resp = self.session.get( "{url}/api/v4/projects/{repo_id}/hooks".format( - url=self.adapter.provider_default_url, + url=self.base_api_url, repo_id=repo_id, ), ) @@ -385,7 +384,7 @@ def setup_webhook(self, project, integration=None): ) repo_id = self._get_repo_id(project) - url = f"{self.adapter.provider_default_url}/api/v4/projects/{repo_id}/hooks" + url = f"{self.base_api_url}/api/v4/projects/{repo_id}/hooks" if repo_id is None: return (False, resp) @@ -396,9 +395,8 @@ def setup_webhook(self, project, integration=None): url=url, ) data = self.get_webhook_data(repo_id, project, integration) - session = self.get_session() try: - resp = session.post( + resp = self.session.post( url, data=data, headers={"content-type": "application/json"}, @@ -448,7 +446,6 @@ def update_webhook(self, project, integration): return self.setup_webhook(project, integration) resp = None - session = self.get_session() repo_id = self._get_repo_id(project) if repo_id is None: @@ -462,9 +459,9 @@ def update_webhook(self, project, integration): ) try: hook_id = provider_data.get("id") - resp = session.put( + resp = self.session.put( "{url}/api/v4/projects/{repo_id}/hooks/{hook_id}".format( - url=self.adapter.provider_default_url, + url=self.base_api_url, repo_id=repo_id, hook_id=hook_id, ), @@ -512,7 +509,6 @@ def send_build_status(self, build, commit, status): :rtype: Bool """ resp = None - session = self.get_session() project = build.project repo_id = self._get_repo_id(project) @@ -539,7 +535,7 @@ def send_build_status(self, build, commit, status): "description": description, "context": context, } - url = f"{self.adapter.provider_default_url}/api/v4/projects/{repo_id}/statuses/{commit}" + url = f"{self.base_api_url}/api/v4/projects/{repo_id}/statuses/{commit}" log.bind( project_slug=project.slug, @@ -548,7 +544,7 @@ def send_build_status(self, build, commit, status): url=url, ) try: - resp = session.post( + resp = self.session.post( url, data=json.dumps(data), headers={"content-type": "application/json"}, diff --git a/readthedocs/oauth/tasks.py b/readthedocs/oauth/tasks.py index 13a6185c1f1..cff086804d4 100644 --- a/readthedocs/oauth/tasks.py +++ b/readthedocs/oauth/tasks.py @@ -49,7 +49,7 @@ def sync_remote_repositories(user_id): try: service.sync() except SyncServiceError: - failed_services.add(service.provider_name) + failed_services.add(service_cls.allauth_provider.name) if failed_services: raise SyncServiceError( SyncServiceError.INVALID_OR_REVOKED_ACCESS_TOKEN.format( @@ -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.provider_name, + "provider_name": service_class.allauth_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.provider_name, + "provider_name": service_class.allauth_provider.name, "url_docs_webhook": "https://docs.readthedocs.io/page/webhooks.html", }, ) diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index 7b4a33d0cbd..091c72bcea3 100644 --- a/readthedocs/projects/models.py +++ b/readthedocs/projects/models.py @@ -1053,7 +1053,7 @@ def get_git_service_class(self, fallback_to_clone_url=False): def git_provider_name(self): """Get the provider name for project. e.g: GitHub, GitLab, Bitbucket.""" service_class = self.get_git_service_class(fallback_to_clone_url=True) - return service_class.provider_name if service_class else None + return service_class.allauth_provider.name if service_class else None def find(self, filename, version): """ diff --git a/readthedocs/projects/views/private.py b/readthedocs/projects/views/private.py index aeacb38c81c..ecbbbf1863e 100644 --- a/readthedocs/projects/views/private.py +++ b/readthedocs/projects/views/private.py @@ -489,7 +489,7 @@ def get(self, request, *args, **kwargs): deprecated_accounts = SocialAccount.objects.filter( user=self.request.user ).exclude( - provider__in=[service.adapter.provider_id for service in registry], + provider__in=[service.allauth_provider.id for service in registry], ) # yapf: disable for account in deprecated_accounts: provider_account = account.get_provider_account() diff --git a/readthedocs/rtd_tests/tests/test_oauth.py b/readthedocs/rtd_tests/tests/test_oauth.py index ba206f12a7a..02ee702535c 100644 --- a/readthedocs/rtd_tests/tests/test_oauth.py +++ b/readthedocs/rtd_tests/tests/test_oauth.py @@ -301,9 +301,9 @@ def test_multiple_users_same_repo(self): self.assertEqual(github_project_2, github_project_6) @mock.patch("readthedocs.oauth.services.github.log") - @mock.patch("readthedocs.oauth.services.github.GitHubService.get_session") + @mock.patch("readthedocs.oauth.services.github.GitHubService.session") def test_send_build_status_successful(self, session, mock_logger): - session().post.return_value.status_code = 201 + session.post.return_value.status_code = 201 success = self.service.send_build_status( self.external_build, self.external_build.commit, BUILD_STATUS_SUCCESS ) @@ -315,9 +315,9 @@ def test_send_build_status_successful(self, session, mock_logger): ) @mock.patch("readthedocs.oauth.services.github.log") - @mock.patch("readthedocs.oauth.services.github.GitHubService.get_session") + @mock.patch("readthedocs.oauth.services.github.GitHubService.session") def test_send_build_status_404_error(self, session, mock_logger): - session().post.return_value.status_code = 404 + session.post.return_value.status_code = 404 success = self.service.send_build_status( self.external_build, self.external_build.commit, BUILD_STATUS_SUCCESS ) @@ -329,9 +329,9 @@ def test_send_build_status_404_error(self, session, mock_logger): ) @mock.patch("readthedocs.oauth.services.github.log") - @mock.patch("readthedocs.oauth.services.github.GitHubService.get_session") + @mock.patch("readthedocs.oauth.services.github.GitHubService.session") def test_send_build_status_value_error(self, session, mock_logger): - session().post.side_effect = ValueError + session.post.side_effect = ValueError success = self.service.send_build_status( self.external_build, self.external_build.commit, BUILD_STATUS_SUCCESS ) @@ -357,10 +357,10 @@ def test_create_public_repo_when_private_projects_are_enabled(self): self.assertEqual(repo.remote_id, str(self.repo_with_org_response_data["id"])) @mock.patch("readthedocs.oauth.services.github.log") - @mock.patch("readthedocs.oauth.services.github.GitHubService.get_session") + @mock.patch("readthedocs.oauth.services.github.GitHubService.session") def test_setup_webhook_successful(self, session, mock_logger): - session().post.return_value.status_code = 201 - session().post.return_value.json.return_value = {} + session.post.return_value.status_code = 201 + session.post.return_value.json.return_value = {} success, _ = self.service.setup_webhook(self.project, self.integration) self.integration.refresh_from_db() @@ -373,9 +373,9 @@ def test_setup_webhook_successful(self, session, mock_logger): ) @mock.patch("readthedocs.oauth.services.github.log") - @mock.patch("readthedocs.oauth.services.github.GitHubService.get_session") + @mock.patch("readthedocs.oauth.services.github.GitHubService.session") def test_setup_webhook_404_error(self, session, mock_logger): - session().post.return_value.status_code = 404 + session.post.return_value.status_code = 404 success, _ = self.service.setup_webhook(self.project, self.integration) self.integration.refresh_from_db() @@ -387,9 +387,9 @@ def test_setup_webhook_404_error(self, session, mock_logger): ) @mock.patch("readthedocs.oauth.services.github.log") - @mock.patch("readthedocs.oauth.services.github.GitHubService.get_session") + @mock.patch("readthedocs.oauth.services.github.GitHubService.session") def test_setup_webhook_value_error(self, session, mock_logger): - session().post.side_effect = ValueError + session.post.side_effect = ValueError self.service.setup_webhook(self.project, self.integration) self.integration.refresh_from_db() @@ -405,10 +405,10 @@ def test_setup_webhook_value_error(self, session, mock_logger): ) @mock.patch("readthedocs.oauth.services.github.log") - @mock.patch("readthedocs.oauth.services.github.GitHubService.get_session") + @mock.patch("readthedocs.oauth.services.github.GitHubService.session") def test_update_webhook_successful(self, session, mock_logger): - session().patch.return_value.status_code = 201 - session().patch.return_value.json.return_value = {} + session.patch.return_value.status_code = 201 + session.patch.return_value.json.return_value = {} success, _ = self.service.update_webhook(self.project, self.integration) self.integration.refresh_from_db() @@ -423,29 +423,29 @@ def test_update_webhook_successful(self, session, mock_logger): "GitHub webhook update successful for project.", ) - @mock.patch("readthedocs.oauth.services.github.GitHubService.get_session") + @mock.patch("readthedocs.oauth.services.github.GitHubService.session") @mock.patch("readthedocs.oauth.services.github.GitHubService.setup_webhook") def test_update_webhook_404_error(self, setup_webhook, session): - session().patch.return_value.status_code = 404 + session.patch.return_value.status_code = 404 self.service.update_webhook(self.project, self.integration) setup_webhook.assert_called_once_with(self.project, self.integration) - @mock.patch("readthedocs.oauth.services.github.GitHubService.get_session") + @mock.patch("readthedocs.oauth.services.github.GitHubService.session") @mock.patch("readthedocs.oauth.services.github.GitHubService.setup_webhook") def test_update_webhook_no_provider_data(self, setup_webhook, session): self.integration.provider_data = {} self.integration.save() - session().patch.side_effect = AttributeError + session.patch.side_effect = AttributeError self.service.update_webhook(self.project, self.integration) setup_webhook.assert_called_once_with(self.project, self.integration) @mock.patch("readthedocs.oauth.services.github.log") - @mock.patch("readthedocs.oauth.services.github.GitHubService.get_session") + @mock.patch("readthedocs.oauth.services.github.GitHubService.session") def test_update_webhook_value_error(self, session, mock_logger): - session().patch.side_effect = ValueError + session.patch.side_effect = ValueError self.service.update_webhook(self.project, self.integration) self.integration.refresh_from_db() @@ -456,7 +456,7 @@ def test_update_webhook_value_error(self, session, mock_logger): ) @mock.patch("readthedocs.oauth.services.github.log") - @mock.patch("readthedocs.oauth.services.github.GitHubService.get_session") + @mock.patch("readthedocs.oauth.services.github.GitHubService.session") def test_get_provider_data_successful(self, session, mock_logger): self.integration.provider_data = {} self.integration.save() @@ -474,8 +474,8 @@ def test_get_provider_data_successful(self, session, mock_logger): ) webhook_data[0]["config"]["url"] = rtd_webhook_url - session().get.return_value.status_code = 200 - session().get.return_value.json.return_value = webhook_data + session.get.return_value.status_code = 200 + session.get.return_value.json.return_value = webhook_data self.service.get_provider_data(self.project, self.integration) @@ -492,12 +492,12 @@ def test_get_provider_data_successful(self, session, mock_logger): ) @mock.patch("readthedocs.oauth.services.github.log") - @mock.patch("readthedocs.oauth.services.github.GitHubService.get_session") + @mock.patch("readthedocs.oauth.services.github.GitHubService.session") def test_get_provider_data_404_error(self, session, mock_logger): self.integration.provider_data = {} self.integration.save() - session().get.return_value.status_code = 404 + session.get.return_value.status_code = 404 self.service.get_provider_data(self.project, self.integration) @@ -510,12 +510,12 @@ def test_get_provider_data_404_error(self, session, mock_logger): ) @mock.patch("readthedocs.oauth.services.github.log") - @mock.patch("readthedocs.oauth.services.github.GitHubService.get_session") + @mock.patch("readthedocs.oauth.services.github.GitHubService.session") def test_get_provider_data_attribute_error(self, session, mock_logger): self.integration.provider_data = {} self.integration.save() - session().get.side_effect = AttributeError + session.get.side_effect = AttributeError self.service.get_provider_data(self.project, self.integration) @@ -531,10 +531,6 @@ def test_get_provider_data_attribute_error(self, session, mock_logger): "GitHub webhook Listing failed for project.", ) - def test_get_access_token_url(self): - url = self.service.get_access_token_url() - self.assertEqual(url, "https://github.com/login/oauth/access_token") - class BitbucketOAuthTests(TestCase): fixtures = ["eric", "test_data"] @@ -780,10 +776,10 @@ def test_import_with_no_token(self): self.assertEqual(services, []) @mock.patch("readthedocs.oauth.services.bitbucket.log") - @mock.patch("readthedocs.oauth.services.bitbucket.BitbucketService.get_session") + @mock.patch("readthedocs.oauth.services.bitbucket.BitbucketService.session") def test_setup_webhook_successful(self, session, mock_logger): - session().post.return_value.status_code = 201 - session().post.return_value.json.return_value = {} + session.post.return_value.status_code = 201 + session.post.return_value.json.return_value = {} success, _ = self.service.setup_webhook(self.project, self.integration) self.assertTrue(success) @@ -797,9 +793,9 @@ def test_setup_webhook_successful(self, session, mock_logger): ) @mock.patch("readthedocs.oauth.services.bitbucket.log") - @mock.patch("readthedocs.oauth.services.bitbucket.BitbucketService.get_session") + @mock.patch("readthedocs.oauth.services.bitbucket.BitbucketService.session") def test_setup_webhook_404_error(self, session, mock_logger): - session().post.return_value.status_code = 404 + session.post.return_value.status_code = 404 success, _ = self.service.setup_webhook(self.project, self.integration) self.assertFalse(success) @@ -813,9 +809,9 @@ def test_setup_webhook_404_error(self, session, mock_logger): ) @mock.patch("readthedocs.oauth.services.bitbucket.log") - @mock.patch("readthedocs.oauth.services.bitbucket.BitbucketService.get_session") + @mock.patch("readthedocs.oauth.services.bitbucket.BitbucketService.session") def test_setup_webhook_value_error(self, session, mock_logger): - session().post.side_effect = ValueError + session.post.side_effect = ValueError self.service.setup_webhook(self.project, self.integration) mock_logger.bind.assert_called_with( @@ -828,10 +824,10 @@ def test_setup_webhook_value_error(self, session, mock_logger): ) @mock.patch("readthedocs.oauth.services.bitbucket.log") - @mock.patch("readthedocs.oauth.services.bitbucket.BitbucketService.get_session") + @mock.patch("readthedocs.oauth.services.bitbucket.BitbucketService.session") def test_update_webhook_successful(self, session, mock_logger): - session().put.return_value.status_code = 200 - session().put.return_value.json.return_value = {} + session.put.return_value.status_code = 200 + session.put.return_value.json.return_value = {} success, _ = self.service.update_webhook(self.project, self.integration) self.assertTrue(success) @@ -841,29 +837,29 @@ def test_update_webhook_successful(self, session, mock_logger): "Bitbucket webhook update successful for project.", ) - @mock.patch("readthedocs.oauth.services.bitbucket.BitbucketService.get_session") + @mock.patch("readthedocs.oauth.services.bitbucket.BitbucketService.session") @mock.patch("readthedocs.oauth.services.bitbucket.BitbucketService.setup_webhook") def test_update_webhook_404_error(self, setup_webhook, session): - session().put.return_value.status_code = 404 + session.put.return_value.status_code = 404 self.service.update_webhook(self.project, self.integration) setup_webhook.assert_called_once_with(self.project, self.integration) - @mock.patch("readthedocs.oauth.services.bitbucket.BitbucketService.get_session") + @mock.patch("readthedocs.oauth.services.bitbucket.BitbucketService.session") @mock.patch("readthedocs.oauth.services.bitbucket.BitbucketService.setup_webhook") def test_update_webhook_no_provider_data(self, setup_webhook, session): self.integration.provider_data = {} self.integration.save() - session().put.side_effect = AttributeError + session.put.side_effect = AttributeError self.service.update_webhook(self.project, self.integration) setup_webhook.assert_called_once_with(self.project, self.integration) @mock.patch("readthedocs.oauth.services.bitbucket.log") - @mock.patch("readthedocs.oauth.services.bitbucket.BitbucketService.get_session") + @mock.patch("readthedocs.oauth.services.bitbucket.BitbucketService.session") def test_update_webhook_value_error(self, session, mock_logger): - session().put.side_effect = ValueError + session.put.side_effect = ValueError self.service.update_webhook(self.project, self.integration) mock_logger.bind.assert_called_with(project_slug=self.project.slug) @@ -872,7 +868,7 @@ def test_update_webhook_value_error(self, session, mock_logger): ) @mock.patch("readthedocs.oauth.services.bitbucket.log") - @mock.patch("readthedocs.oauth.services.bitbucket.BitbucketService.get_session") + @mock.patch("readthedocs.oauth.services.bitbucket.BitbucketService.session") def test_get_provider_data_successful(self, session, mock_logger): self.integration.provider_data = {} self.integration.save() @@ -890,8 +886,8 @@ def test_get_provider_data_successful(self, session, mock_logger): ) webhook_data["values"][0]["url"] = rtd_webhook_url - session().get.return_value.status_code = 200 - session().get.return_value.json.return_value = webhook_data + session.get.return_value.status_code = 200 + session.get.return_value.json.return_value = webhook_data self.service.get_provider_data(self.project, self.integration) @@ -908,12 +904,12 @@ def test_get_provider_data_successful(self, session, mock_logger): ) @mock.patch("readthedocs.oauth.services.bitbucket.log") - @mock.patch("readthedocs.oauth.services.bitbucket.BitbucketService.get_session") + @mock.patch("readthedocs.oauth.services.bitbucket.BitbucketService.session") def test_get_provider_data_404_error(self, session, mock_logger): self.integration.provider_data = {} self.integration.save() - session().get.return_value.status_code = 404 + session.get.return_value.status_code = 404 self.service.get_provider_data(self.project, self.integration) @@ -930,12 +926,12 @@ def test_get_provider_data_404_error(self, session, mock_logger): ) @mock.patch("readthedocs.oauth.services.bitbucket.log") - @mock.patch("readthedocs.oauth.services.bitbucket.BitbucketService.get_session") + @mock.patch("readthedocs.oauth.services.bitbucket.BitbucketService.session") def test_get_provider_data_attribute_error(self, session, mock_logger): self.integration.provider_data = {} self.integration.save() - session().get.side_effect = AttributeError + session.get.side_effect = AttributeError self.service.get_provider_data(self.project, self.integration) @@ -951,10 +947,6 @@ def test_get_provider_data_attribute_error(self, session, mock_logger): "Bitbucket webhook Listing failed for project.", ) - def test_get_access_token_url(self): - url = self.service.get_access_token_url() - self.assertEqual(url, "https://bitbucket.org/site/oauth2/access_token") - class GitLabOAuthTests(TestCase): fixtures = ["eric", "test_data"] @@ -1164,10 +1156,10 @@ def test_make_private_project(self): self.assertIsNotNone(repo) @mock.patch("readthedocs.oauth.services.gitlab.log") - @mock.patch("readthedocs.oauth.services.gitlab.GitLabService.get_session") + @mock.patch("readthedocs.oauth.services.gitlab.GitLabService.session") @mock.patch("readthedocs.oauth.services.gitlab.GitLabService._get_repo_id") def test_send_build_status_successful(self, repo_id, session, mock_logger): - session().post.return_value.status_code = 201 + session.post.return_value.status_code = 201 repo_id().return_value = "9999" success = self.service.send_build_status( @@ -1181,10 +1173,10 @@ def test_send_build_status_successful(self, repo_id, session, mock_logger): ) @mock.patch("readthedocs.oauth.services.gitlab.log") - @mock.patch("readthedocs.oauth.services.gitlab.GitLabService.get_session") + @mock.patch("readthedocs.oauth.services.gitlab.GitLabService.session") @mock.patch("readthedocs.oauth.services.gitlab.GitLabService._get_repo_id") def test_send_build_status_404_error(self, repo_id, session, mock_logger): - session().post.return_value.status_code = 404 + session.post.return_value.status_code = 404 repo_id.return_value = "9999" success = self.service.send_build_status( @@ -1198,10 +1190,10 @@ def test_send_build_status_404_error(self, repo_id, session, mock_logger): ) @mock.patch("readthedocs.oauth.services.gitlab.log") - @mock.patch("readthedocs.oauth.services.gitlab.GitLabService.get_session") + @mock.patch("readthedocs.oauth.services.gitlab.GitLabService.session") @mock.patch("readthedocs.oauth.services.gitlab.GitLabService._get_repo_id") def test_send_build_status_value_error(self, repo_id, session, mock_logger): - session().post.side_effect = ValueError + session.post.side_effect = ValueError repo_id().return_value = "9999" success = self.service.send_build_status( @@ -1221,10 +1213,10 @@ def test_send_build_status_value_error(self, repo_id, session, mock_logger): ) @mock.patch("readthedocs.oauth.services.gitlab.log") - @mock.patch("readthedocs.oauth.services.gitlab.GitLabService.get_session") + @mock.patch("readthedocs.oauth.services.gitlab.GitLabService.session") def test_setup_webhook_successful(self, session, mock_logger): - session().post.return_value.status_code = 201 - session().post.return_value.json.return_value = {} + session.post.return_value.status_code = 201 + session.post.return_value.json.return_value = {} success, _ = self.service.setup_webhook(self.project, self.integration) self.integration.refresh_from_db() @@ -1239,9 +1231,9 @@ def test_setup_webhook_successful(self, session, mock_logger): ) @mock.patch("readthedocs.oauth.services.gitlab.log") - @mock.patch("readthedocs.oauth.services.gitlab.GitLabService.get_session") + @mock.patch("readthedocs.oauth.services.gitlab.GitLabService.session") def test_setup_webhook_404_error(self, session, mock_logger): - session().post.return_value.status_code = 404 + session.post.return_value.status_code = 404 success, _ = self.service.setup_webhook(self.project, self.integration) self.integration.refresh_from_db() @@ -1254,9 +1246,9 @@ def test_setup_webhook_404_error(self, session, mock_logger): ) @mock.patch("readthedocs.oauth.services.gitlab.log") - @mock.patch("readthedocs.oauth.services.gitlab.GitLabService.get_session") + @mock.patch("readthedocs.oauth.services.gitlab.GitLabService.session") def test_setup_webhook_value_error(self, session, mock_logger): - session().post.side_effect = ValueError + session.post.side_effect = ValueError self.service.setup_webhook(self.project, self.integration) self.integration.refresh_from_db() @@ -1272,12 +1264,12 @@ def test_setup_webhook_value_error(self, session, mock_logger): ) @mock.patch("readthedocs.oauth.services.gitlab.log") - @mock.patch("readthedocs.oauth.services.gitlab.GitLabService.get_session") + @mock.patch("readthedocs.oauth.services.gitlab.GitLabService.session") @mock.patch("readthedocs.oauth.services.gitlab.GitLabService._get_repo_id") def test_update_webhook_successful(self, repo_id, session, mock_logger): repo_id.return_value = "9999" - session().put.return_value.status_code = 200 - session().put.return_value.json.return_value = {} + session.put.return_value.status_code = 200 + session.put.return_value.json.return_value = {} success, _ = self.service.update_webhook(self.project, self.integration) self.integration.refresh_from_db() @@ -1292,17 +1284,17 @@ def test_update_webhook_successful(self, repo_id, session, mock_logger): "GitLab webhook update successful for project.", ) - @mock.patch("readthedocs.oauth.services.gitlab.GitLabService.get_session") + @mock.patch("readthedocs.oauth.services.gitlab.GitLabService.session") @mock.patch("readthedocs.oauth.services.gitlab.GitLabService.setup_webhook") @mock.patch("readthedocs.oauth.services.gitlab.GitLabService._get_repo_id") def test_update_webhook_404_error(self, repo_id, setup_webhook, session): repo_id.return_value = "9999" - session().put.return_value.status_code = 404 + session.put.return_value.status_code = 404 self.service.update_webhook(self.project, self.integration) setup_webhook.assert_called_once_with(self.project, self.integration) - @mock.patch("readthedocs.oauth.services.gitlab.GitLabService.get_session") + @mock.patch("readthedocs.oauth.services.gitlab.GitLabService.session") @mock.patch("readthedocs.oauth.services.gitlab.GitLabService.setup_webhook") @mock.patch("readthedocs.oauth.services.gitlab.GitLabService._get_repo_id") def test_update_webhook_no_provider_data(self, repo_id, setup_webhook, session): @@ -1310,17 +1302,17 @@ def test_update_webhook_no_provider_data(self, repo_id, setup_webhook, session): self.integration.save() repo_id.return_value = "9999" - session().put.side_effect = AttributeError + session.put.side_effect = AttributeError self.service.update_webhook(self.project, self.integration) setup_webhook.assert_called_once_with(self.project, self.integration) @mock.patch("readthedocs.oauth.services.gitlab.log") - @mock.patch("readthedocs.oauth.services.gitlab.GitLabService.get_session") + @mock.patch("readthedocs.oauth.services.gitlab.GitLabService.session") @mock.patch("readthedocs.oauth.services.gitlab.GitLabService._get_repo_id") def test_update_webhook_value_error(self, repo_id, session, mock_logger): repo_id.return_value = "9999" - session().put.side_effect = ValueError + session.put.side_effect = ValueError self.service.update_webhook(self.project, self.integration) self.integration.refresh_from_db() @@ -1336,7 +1328,7 @@ def test_update_webhook_value_error(self, repo_id, session, mock_logger): ) @mock.patch("readthedocs.oauth.services.gitlab.log") - @mock.patch("readthedocs.oauth.services.gitlab.GitLabService.get_session") + @mock.patch("readthedocs.oauth.services.gitlab.GitLabService.session") def test_get_provider_data_successful(self, session, mock_logger): self.integration.provider_data = {} self.integration.save() @@ -1354,8 +1346,8 @@ def test_get_provider_data_successful(self, session, mock_logger): ) webhook_data[0]["url"] = rtd_webhook_url - session().get.return_value.status_code = 200 - session().get.return_value.json.return_value = webhook_data + session.get.return_value.status_code = 200 + session.get.return_value.json.return_value = webhook_data self.service.get_provider_data(self.project, self.integration) @@ -1371,12 +1363,12 @@ def test_get_provider_data_successful(self, session, mock_logger): ) @mock.patch("readthedocs.oauth.services.gitlab.log") - @mock.patch("readthedocs.oauth.services.gitlab.GitLabService.get_session") + @mock.patch("readthedocs.oauth.services.gitlab.GitLabService.session") def test_get_provider_data_404_error(self, session, mock_logger): self.integration.provider_data = {} self.integration.save() - session().get.return_value.status_code = 404 + session.get.return_value.status_code = 404 self.service.get_provider_data(self.project, self.integration) @@ -1392,12 +1384,12 @@ def test_get_provider_data_404_error(self, session, mock_logger): ) @mock.patch("readthedocs.oauth.services.gitlab.log") - @mock.patch("readthedocs.oauth.services.gitlab.GitLabService.get_session") + @mock.patch("readthedocs.oauth.services.gitlab.GitLabService.session") def test_get_provider_data_attribute_error(self, session, mock_logger): self.integration.provider_data = {} self.integration.save() - session().get.side_effect = AttributeError + session.get.side_effect = AttributeError self.service.get_provider_data(self.project, self.integration) @@ -1411,7 +1403,3 @@ def test_get_provider_data_attribute_error(self, session, mock_logger): mock_logger.exception.assert_called_with( "GitLab webhook Listing failed for project.", ) - - def test_get_access_token_url(self): - url = self.service.get_access_token_url() - self.assertEqual(url, "https://gitlab.com/oauth/token") From 531960894011f36b7c1323b1bf2c4583848fddeb Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 13 Feb 2025 13:39:50 -0500 Subject: [PATCH 10/11] Missed this file --- readthedocs/oauth/clients.py | 71 ++++++++++++++++++++++++++++++++++++ 1 file changed, 71 insertions(+) create mode 100644 readthedocs/oauth/clients.py diff --git a/readthedocs/oauth/clients.py b/readthedocs/oauth/clients.py new file mode 100644 index 00000000000..9cc91ed9c59 --- /dev/null +++ b/readthedocs/oauth/clients.py @@ -0,0 +1,71 @@ +from datetime import datetime + +import structlog +from django.utils import timezone +from requests_oauthlib import OAuth2Session + +log = structlog.get_logger(__name__) + + +def _get_token_updater(token): + """ + Update token given data from OAuth response. + + Expect the following response into the closure:: + + { + u'token_type': u'bearer', + u'scopes': u'webhook repository team account', + u'refresh_token': u'...', + u'access_token': u'...', + u'expires_in': 3600, + u'expires_at': 1449218652.558185 + } + """ + + def _updater(data): + token.token = data["access_token"] + token.token_secret = data.get("refresh_token", "") + token.expires_at = timezone.make_aware( + datetime.fromtimestamp(data["expires_at"]), + ) + token.save() + log.info("Updated token.", token_id=token.pk) + + return _updater + + +def get_oauth2_client(account): + """Get an OAuth2 client for the given social account.""" + token = account.socialtoken_set.first() + if token is None: + return None + + token_config = { + "access_token": token.token, + "token_type": "bearer", + } + if token.expires_at is not None: + token_expires = (token.expires_at - timezone.now()).total_seconds() + token_config.update( + { + "refresh_token": token.token_secret, + "expires_in": token_expires, + } + ) + + provider = account.get_provider() + social_app = provider.app + oauth2_adapter = provider.get_oauth2_adapter(request=provider.request) + + session = OAuth2Session( + client_id=social_app.client_id, + token=token_config, + auto_refresh_kwargs={ + "client_id": social_app.client_id, + "client_secret": social_app.secret, + }, + auto_refresh_url=oauth2_adapter.access_token_url, + token_updater=_get_token_updater(token), + ) + return session From c9eb355711d5297ec81363e58d94c13af5c1e2e0 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 13 Feb 2025 13:46:08 -0500 Subject: [PATCH 11/11] Use just the hostname for the base_api_url --- readthedocs/oauth/services/bitbucket.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/readthedocs/oauth/services/bitbucket.py b/readthedocs/oauth/services/bitbucket.py index 60f36ae4ac8..bf8fc90daa6 100644 --- a/readthedocs/oauth/services/bitbucket.py +++ b/readthedocs/oauth/services/bitbucket.py @@ -26,7 +26,7 @@ class BitbucketService(UserService): vcs_provider_slug = BITBUCKET allauth_provider = BitbucketOAuth2Provider - base_api_url = "https://api.bitbucket.org/2.0" + base_api_url = "https://api.bitbucket.org" # TODO replace this with a less naive check url_pattern = re.compile(r"bitbucket.org") https_url_pattern = re.compile(r"^https:\/\/[^@]+@bitbucket.org/") @@ -82,7 +82,7 @@ def sync_organizations(self): try: workspaces = self.paginate( - f"{self.base_api_url}/workspaces/", + f"{self.base_api_url}/2.0/workspaces/", role="member", ) for workspace in workspaces: @@ -236,7 +236,7 @@ def get_provider_data(self, project, integration): return integration.provider_data owner, repo = build_utils.get_bitbucket_username_repo(url=project.repo) - url = f"{self.base_api_url}/repositories/{owner}/{repo}/hooks" + url = f"{self.base_api_url}/2.0/repositories/{owner}/{repo}/hooks" rtd_webhook_url = self.get_webhook_url(project, integration) @@ -284,7 +284,7 @@ def setup_webhook(self, project, integration=None): :rtype: (Bool, Response) """ owner, repo = build_utils.get_bitbucket_username_repo(url=project.repo) - url = f"{self.base_api_url}/repositories/{owner}/{repo}/hooks" + url = f"{self.base_api_url}/2.0/repositories/{owner}/{repo}/hooks" if not integration: integration, _ = Integration.objects.get_or_create( project=project,