diff --git a/common b/common index c4c9f09de1e..9bc493b4064 160000 --- a/common +++ b/common @@ -1 +1 @@ -Subproject commit c4c9f09de1e04cc4e75b00b8e58d851396abfe11 +Subproject commit 9bc493b40647f89450f1e913067f199ff2a86861 diff --git a/docs/dev/settings.rst b/docs/dev/settings.rst index 79da27e29ad..2db4844c16c 100644 --- a/docs/dev/settings.rst +++ b/docs/dev/settings.rst @@ -158,6 +158,8 @@ providers using the following environment variables: .. envvar:: RTD_SOCIALACCOUNT_PROVIDERS_GITHUB_CLIENT_ID .. envvar:: RTD_SOCIALACCOUNT_PROVIDERS_GITHUB_SECRET +.. envvar:: RTD_SOCIALACCOUNT_PROVIDERS_GITHUBAPP_CLIENT_ID +.. envvar:: RTD_SOCIALACCOUNT_PROVIDERS_GITHUBAPP_SECRET .. envvar:: RTD_SOCIALACCOUNT_PROVIDERS_GITLAB_CLIENT_ID .. envvar:: RTD_SOCIALACCOUNT_PROVIDERS_GITLAB_SECRET .. envvar:: RTD_SOCIALACCOUNT_PROVIDERS_BITBUCKET_OAUTH2_CLIENT_ID @@ -179,4 +181,4 @@ Ethical Ads variables The following variables are required to use ``ethicalads`` in dev: -.. envvar:: RTD_USE_PROMOS \ No newline at end of file +.. envvar:: RTD_USE_PROMOS diff --git a/readthedocs/allauth/providers/githubapp/__init__.py b/readthedocs/allauth/providers/githubapp/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/readthedocs/allauth/providers/githubapp/provider.py b/readthedocs/allauth/providers/githubapp/provider.py new file mode 100644 index 00000000000..442126c27e1 --- /dev/null +++ b/readthedocs/allauth/providers/githubapp/provider.py @@ -0,0 +1,17 @@ +from allauth.socialaccount.providers.github.provider import GitHubProvider + +from readthedocs.allauth.providers.githubapp.views import GitHubAppOAuth2Adapter + + +class GitHubAppProvider(GitHubProvider): + """ + Provider for GitHub App. + + We subclass the GitHubProvider to have two separate providers for the GitHub OAuth App and the GitHub App. + """ + + id = "githubapp" + oauth2_adapter_class = GitHubAppOAuth2Adapter + + +provider_classes = [GitHubAppProvider] diff --git a/readthedocs/allauth/providers/githubapp/urls.py b/readthedocs/allauth/providers/githubapp/urls.py new file mode 100644 index 00000000000..525e82eacc7 --- /dev/null +++ b/readthedocs/allauth/providers/githubapp/urls.py @@ -0,0 +1,7 @@ +"""Copied from allauth.socialaccount.providers.github.urls.""" + +from allauth.socialaccount.providers.oauth2.urls import default_urlpatterns + +from readthedocs.allauth.providers.githubapp.provider import GitHubAppProvider + +urlpatterns = default_urlpatterns(GitHubAppProvider) diff --git a/readthedocs/allauth/providers/githubapp/views.py b/readthedocs/allauth/providers/githubapp/views.py new file mode 100644 index 00000000000..d40eede0ef4 --- /dev/null +++ b/readthedocs/allauth/providers/githubapp/views.py @@ -0,0 +1,15 @@ +"""Copied from allauth.socialaccount.providers.github.views.""" + +from allauth.socialaccount.providers.github.views import GitHubOAuth2Adapter +from allauth.socialaccount.providers.oauth2.views import ( + OAuth2CallbackView, + OAuth2LoginView, +) + + +class GitHubAppOAuth2Adapter(GitHubOAuth2Adapter): + provider_id = "githubapp" + + +oauth2_login = OAuth2LoginView.adapter_view(GitHubAppOAuth2Adapter) +oauth2_callback = OAuth2CallbackView.adapter_view(GitHubAppOAuth2Adapter) diff --git a/readthedocs/core/adapters.py b/readthedocs/core/adapters.py index f36a6fe761a..394516637ff 100644 --- a/readthedocs/core/adapters.py +++ b/readthedocs/core/adapters.py @@ -2,9 +2,17 @@ import structlog from allauth.account.adapter import DefaultAccountAdapter +from allauth.account.adapter import get_adapter as get_account_adapter +from allauth.exceptions import ImmediateHttpResponse from allauth.socialaccount.adapter import DefaultSocialAccountAdapter +from allauth.socialaccount.models import SocialAccount +from allauth.socialaccount.providers.github.provider import GitHubProvider +from django.contrib import messages +from django.http import HttpResponseRedirect +from django.urls import reverse from django.utils.encoding import force_str +from readthedocs.allauth.providers.githubapp.provider import GitHubAppProvider from readthedocs.core.utils import send_email_from_object from readthedocs.invitations.models import Invitation @@ -54,6 +62,10 @@ def save_user(self, request, user, form, commit=True): class SocialAccountAdapter(DefaultSocialAccountAdapter): def pre_social_login(self, request, sociallogin): + self._filter_email_addresses(sociallogin) + self._connect_github_app_to_existing_github_account(request, sociallogin) + + def _filter_email_addresses(self, sociallogin): """ Remove all email addresses except the primary one. @@ -65,3 +77,59 @@ def pre_social_login(self, request, sociallogin): sociallogin.email_addresses = [ email for email in sociallogin.email_addresses if email.primary ] + + def _connect_github_app_to_existing_github_account(self, request, sociallogin): + """ + Connect a GitHub App (new integration) account to an existing GitHub account (old integration). + + When a user signs up with the GitHub App we check if there is an existing GitHub account, + and if it belongs to the same user, we connect the accounts instead of creating a new one. + """ + provider = sociallogin.account.get_provider() + + # If the provider is not GitHub App, nothing to do. + if provider.id != GitHubAppProvider.id: + return + + # If the user already signed up with the GitHub App, nothing to do. + if sociallogin.is_existing: + return + + social_account = SocialAccount.objects.filter( + provider=GitHubProvider.id, + uid=sociallogin.account.uid, + ).first() + + # If there is an existing GH account, we check if that user can use the GH App, + # otherwise we check for the current user. + user_to_check = social_account.user if social_account else request.user + if not self._can_use_github_app(user_to_check): + raise ImmediateHttpResponse(HttpResponseRedirect(reverse("account_login"))) + + # If there isn't an existing GH account, nothing to do, + # just let allauth create the new account. + if not social_account: + return + + # If the user is logged in, and the GH OAuth account belongs to + # a different user, we should not connect the accounts, + # this is the same as trying to connect an existing GH account to another user. + if request.user.is_authenticated and request.user != social_account.user: + message_template = "socialaccount/messages/account_connected_other.txt" + get_account_adapter(request).add_message( + request=request, + level=messages.ERROR, + message_template=message_template, + ) + url = reverse("socialaccount_connections") + raise ImmediateHttpResponse(HttpResponseRedirect(url)) + + sociallogin.connect(request, social_account.user) + + def _can_use_github_app(self, user): + """ + Check if the user can use the GitHub App. + + Only staff users can use the GitHub App for now. + """ + return user.is_staff diff --git a/readthedocs/core/tests/test_adapters.py b/readthedocs/core/tests/test_adapters.py new file mode 100644 index 00000000000..bef74c85745 --- /dev/null +++ b/readthedocs/core/tests/test_adapters.py @@ -0,0 +1,174 @@ +from unittest import mock + +from allauth.exceptions import ImmediateHttpResponse +from allauth.socialaccount.adapter import get_adapter as get_social_account_adapter +from allauth.socialaccount.models import SocialAccount, SocialLogin +from allauth.socialaccount.providers.github.provider import GitHubProvider +from django.contrib.auth.models import AnonymousUser, User +from django.test import TestCase +from django_dynamic_fixture import get + +from readthedocs.allauth.providers.githubapp.provider import GitHubAppProvider + + +class SocialAdapterTest(TestCase): + def setUp(self): + self.user = get(User, username="test") + self.adapter = get_social_account_adapter() + + def test_dont_allow_using_githubapp_for_non_staff_users(self): + assert not SocialAccount.objects.filter(provider=GitHubAppProvider.id).exists() + + # Anonymous user + request = mock.MagicMock(user=AnonymousUser()) + sociallogin = SocialLogin( + user=User(email="me@example.com"), + account=SocialAccount(provider=GitHubAppProvider.id), + ) + with self.assertRaises(ImmediateHttpResponse) as exc: + self.adapter.pre_social_login(request, sociallogin) + self.assertEqual(exc.exception.response.status_code, 302) + + assert not SocialAccount.objects.filter(provider=GitHubAppProvider.id).exists() + + # Existing non-staff user + assert not self.user.is_staff + request = mock.MagicMock(user=self.user) + sociallogin = SocialLogin( + user=User(email="me@example.com"), + account=SocialAccount(provider=GitHubAppProvider.id), + ) + with self.assertRaises(ImmediateHttpResponse) as exc: + self.adapter.pre_social_login(request, sociallogin) + self.assertEqual(exc.exception.response.status_code, 302) + assert not self.user.socialaccount_set.filter( + provider=GitHubAppProvider.id + ).exists() + + def test_allow_using_githubapp_for_staff_users(self): + self.user.is_staff = True + self.user.save() + assert self.user.is_staff + + request = mock.MagicMock(user=self.user) + sociallogin = SocialLogin( + user=User(email="me@example.com"), + account=SocialAccount(provider=GitHubAppProvider.id), + ) + self.adapter.pre_social_login(request, sociallogin) + # No exception raised, but the account is not created, as that is done in another step by allauth. + assert not self.user.socialaccount_set.filter( + provider=GitHubAppProvider.id + ).exists() + + def test_connect_to_existing_github_account_from_staff_user(self): + self.user.is_staff = True + self.user.save() + assert self.user.is_staff + assert not self.user.socialaccount_set.filter( + provider=GitHubAppProvider.id + ).exists() + + github_account = get( + SocialAccount, + provider=GitHubProvider.id, + uid="1234", + user=self.user, + ) + + request = mock.MagicMock(user=AnonymousUser()) + sociallogin = SocialLogin( + user=User(email="me@example.com"), + account=SocialAccount( + provider=GitHubAppProvider.id, uid=github_account.uid + ), + ) + self.adapter.pre_social_login(request, sociallogin) + # A new user is not created, but the existing user is connected to the GitHub App. + assert self.user.socialaccount_set.filter( + provider=GitHubAppProvider.id + ).exists() + + def test_connect_to_existing_github_account_from_staff_user_logged_in(self): + self.user.is_staff = True + self.user.save() + assert self.user.is_staff + assert not self.user.socialaccount_set.filter( + provider=GitHubAppProvider.id + ).exists() + + github_account = get( + SocialAccount, + provider=GitHubProvider.id, + uid="1234", + user=self.user, + ) + + request = mock.MagicMock(user=self.user) + sociallogin = SocialLogin( + user=User(email="me@example.com"), + account=SocialAccount( + provider=GitHubAppProvider.id, uid=github_account.uid + ), + ) + self.adapter.pre_social_login(request, sociallogin) + # A new user is not created, but the existing user is connected to the GitHub App. + assert self.user.socialaccount_set.filter( + provider=GitHubAppProvider.id + ).exists() + + def test_dont_connect_to_existing_github_account_if_user_is_logged_in_with_different_account( + self, + ): + self.user.is_staff = True + self.user.save() + assert self.user.is_staff + assert not self.user.socialaccount_set.filter( + provider=GitHubAppProvider.id + ).exists() + + github_account = get( + SocialAccount, + provider=GitHubProvider.id, + uid="1234", + user=self.user, + ) + + another_user = get(User, username="another") + request = mock.MagicMock(user=another_user) + sociallogin = SocialLogin( + user=User(email="me@example.com"), + account=SocialAccount( + provider=GitHubAppProvider.id, uid=github_account.uid + ), + ) + with self.assertRaises(ImmediateHttpResponse) as exc: + self.adapter.pre_social_login(request, sociallogin) + self.assertEqual(exc.exception.response.status_code, 302) + assert not self.user.socialaccount_set.filter( + provider=GitHubAppProvider.id + ).exists() + assert not another_user.socialaccount_set.filter( + provider=GitHubAppProvider.id + ).exists() + + def test_allow_existing_githubapp_accounts_to_login(self): + assert not self.user.is_staff + githubapp_account = get( + SocialAccount, + provider=GitHubAppProvider.id, + uid="1234", + user=self.user, + ) + + request = mock.MagicMock(user=AnonymousUser()) + sociallogin = SocialLogin( + user=self.user, + account=githubapp_account, + ) + self.adapter.pre_social_login(request, sociallogin) + + self.user.is_staff = True + self.user.save() + assert self.user.is_staff + self.adapter.pre_social_login(request, sociallogin) diff --git a/readthedocs/settings/base.py b/readthedocs/settings/base.py index 1431137f11c..eb42105200e 100644 --- a/readthedocs/settings/base.py +++ b/readthedocs/settings/base.py @@ -297,6 +297,7 @@ def INSTALLED_APPS(self): # noqa "allauth.account", "allauth.socialaccount", "allauth.socialaccount.providers.github", + "readthedocs.allauth.providers.githubapp", "allauth.socialaccount.providers.gitlab", "allauth.socialaccount.providers.bitbucket_oauth2", "allauth.mfa", @@ -713,6 +714,13 @@ def DOCKER_LIMITS(self): "repo:status", ], }, + "githubapp": { + "APPS": [ + {"client_id": "123", "secret": "456", "key": ""}, + ], + # Scope is determined by the GitHub App permissions. + "SCOPE": [], + }, "gitlab": { "APPS": [ {"client_id": "123", "secret": "456", "key": ""}, diff --git a/readthedocs/templates/socialaccount/snippets/provider_list.html b/readthedocs/templates/socialaccount/snippets/provider_list.html index 5175810679c..7aa1a03d2c3 100644 --- a/readthedocs/templates/socialaccount/snippets/provider_list.html +++ b/readthedocs/templates/socialaccount/snippets/provider_list.html @@ -7,8 +7,9 @@ {% comment %} - OpenID is not implemented. - SAML is handled in another view, we don't want to list all SAML integrations here. + - GitHub App is not exposed to users yet. {% endcomment %} - {% if provider.id != 'saml' %} + {% if provider.id != 'saml' and provider.id != 'githubapp' %} {% if allowed_providers and provider.id in allowed_providers or not allowed_providers %}
  • {# Bitbucket doesn't allow more than one callback URL for their OAuth app, so we are redirecting users to the new dashboard for now. #}