From 40611bb915c92a4913cc9e58b4ddb157e17c4d9c Mon Sep 17 00:00:00 2001 From: Robert Raposa Date: Tue, 23 Feb 2021 10:57:11 -0500 Subject: [PATCH] ARCHBOM-1667: fix: remove authentication from auth exchange (#26618) * Remove authentication, including SessionAuthentication, to fix CSRF exemption by dropping CSRF check of SessionAuthentication. * Several changes to make it more clear that only POST is supported and clean up GET method testing. * Removed the temporary 403 error logging that wasn't working. * Removed test_single_access_token which was written for DOP, but doesn't work with DOT. See [MA-2122](https://openedx.atlassian.net/browse/MA-2122) for a ticket about implementing this for DOT, although it doesn't seem to be a priority. NOTE: A comment was added to the ticket explaining that this test was removed. * GET now returns default error for methods not allowed. ARCHBOM-1667 --- .../third_party_auth/tests/utils.py | 2 +- .../djangoapps/auth_exchange/tests/mixins.py | 22 ------- .../auth_exchange/tests/test_forms.py | 2 +- .../auth_exchange/tests/test_views.py | 62 +++++++------------ .../djangoapps/auth_exchange/tests/utils.py | 2 +- .../core/djangoapps/auth_exchange/views.py | 39 +++++------- 6 files changed, 40 insertions(+), 89 deletions(-) diff --git a/common/djangoapps/third_party_auth/tests/utils.py b/common/djangoapps/third_party_auth/tests/utils.py index c0f4232d3e..ec3811e9f3 100644 --- a/common/djangoapps/third_party_auth/tests/utils.py +++ b/common/djangoapps/third_party_auth/tests/utils.py @@ -36,7 +36,7 @@ class ThirdPartyOAuthTestMixin(ThirdPartyAuthTestMixin): def setUp(self): # lint-amnesty, pylint: disable=arguments-differ super(ThirdPartyOAuthTestMixin, self).setUp() # lint-amnesty, pylint: disable=super-with-arguments if self.CREATE_USER: - self.user = UserFactory() + self.user = UserFactory.create(password='secret') UserSocialAuth.objects.create(user=self.user, provider=self.BACKEND, uid=self.social_uid) self.oauth_client = self._create_client() if self.BACKEND == 'google-oauth2': diff --git a/openedx/core/djangoapps/auth_exchange/tests/mixins.py b/openedx/core/djangoapps/auth_exchange/tests/mixins.py index d5751ab4eb..d8ec2d9612 100644 --- a/openedx/core/djangoapps/auth_exchange/tests/mixins.py +++ b/openedx/core/djangoapps/auth_exchange/tests/mixins.py @@ -2,17 +2,9 @@ Mixins to facilitate testing OAuth connections to Django-OAuth-Toolkit or Django-OAuth2-Provider. """ - - -from unittest import expectedFailure - -from django.test.client import RequestFactory - from openedx.core.djangoapps.oauth_dispatch import adapters from openedx.core.djangoapps.oauth_dispatch.tests.constants import DUMMY_REDIRECT_URL -from ..views import DOTAccessTokenExchangeView - class DOTAdapterMixin(object): """ @@ -52,17 +44,3 @@ class DOTAdapterMixin(object): Return the set of keys provided when requesting an access token """ return {'access_token', 'refresh_token', 'token_type', 'expires_in', 'scope'} - - def test_get_method(self): - # Dispatch routes all get methods to DOP, so we test this on the view - request_factory = RequestFactory() - request = request_factory.get('/oauth2/exchange_access_token/') - request.session = {} - view = DOTAccessTokenExchangeView.as_view() - response = view(request, backend='facebook') - assert response.status_code == 400 - - @expectedFailure - def test_single_access_token(self): - # TODO: Single access tokens not supported yet for DOT (See MA-2122) - super(DOTAdapterMixin, self).test_single_access_token() # lint-amnesty, pylint: disable=super-with-arguments diff --git a/openedx/core/djangoapps/auth_exchange/tests/test_forms.py b/openedx/core/djangoapps/auth_exchange/tests/test_forms.py index 21d9d094ed..5887a3e95b 100644 --- a/openedx/core/djangoapps/auth_exchange/tests/test_forms.py +++ b/openedx/core/djangoapps/auth_exchange/tests/test_forms.py @@ -41,7 +41,7 @@ class AccessTokenExchangeFormTest(AccessTokenExchangeTestMixin): form = AccessTokenExchangeForm(request=self.request, oauth2_adapter=self.oauth2_adapter, data=data) assert form.errors == {'error': expected_error, 'error_description': expected_error_description} - def _assert_success(self, data, expected_scopes): + def _assert_success(self, data, expected_scopes, expected_logged_in_user=None): form = AccessTokenExchangeForm(request=self.request, oauth2_adapter=self.oauth2_adapter, data=data) assert form.is_valid() assert form.cleaned_data['user'] == self.user diff --git a/openedx/core/djangoapps/auth_exchange/tests/test_views.py b/openedx/core/djangoapps/auth_exchange/tests/test_views.py index 212b8183d8..e1a50b2e97 100644 --- a/openedx/core/djangoapps/auth_exchange/tests/test_views.py +++ b/openedx/core/djangoapps/auth_exchange/tests/test_views.py @@ -4,15 +4,12 @@ Tests for OAuth token exchange views # pylint: disable=no-member - import json import unittest from datetime import timedelta -import pytest import ddt import httpretty -import mock from django.conf import settings from django.test import TestCase from django.urls import reverse @@ -22,7 +19,10 @@ from social_django.models import Partial from openedx.core.djangoapps.oauth_dispatch.tests import factories as dot_factories from common.djangoapps.student.tests.factories import UserFactory -from common.djangoapps.third_party_auth.tests.utils import ThirdPartyOAuthTestMixinFacebook, ThirdPartyOAuthTestMixinGoogle # lint-amnesty, pylint: disable=line-too-long +from common.djangoapps.third_party_auth.tests.utils import ( + ThirdPartyOAuthTestMixinFacebook, + ThirdPartyOAuthTestMixinGoogle, +) from .mixins import DOTAdapterMixin from .utils import TPA_FEATURE_ENABLED, TPA_FEATURES_KEY, AccessTokenExchangeTestMixin @@ -32,6 +32,12 @@ from .utils import TPA_FEATURE_ENABLED, TPA_FEATURES_KEY, AccessTokenExchangeTes class AccessTokenExchangeViewTest(AccessTokenExchangeTestMixin): """ Mixin that defines test cases for AccessTokenExchangeView + + Warning: This class was originally created to support multiple libraries, + but we currently only support django-oauth-toolkit (DOT). At this point, + the variety of mixins can be quite confusing and are no longer providing + any benefit, other than the potential for reintroducing another library + in the future. """ def setUp(self): super(AccessTokenExchangeViewTest, self).setUp() # lint-amnesty, pylint: disable=super-with-arguments @@ -51,8 +57,11 @@ class AccessTokenExchangeViewTest(AccessTokenExchangeTestMixin): expected_data['error_code'] = error_code assert json.loads(response.content.decode('utf-8')) == expected_data - def _assert_success(self, data, expected_scopes): + def _assert_success(self, data, expected_scopes, expected_logged_in_user=None): response = self.csrf_client.post(self.url, data) + if expected_logged_in_user: + # Ensure that safe sessions isn't preventing an expected login + assert expected_logged_in_user == response.wsgi_request.user assert response.status_code == 200 assert response['Content-Type'] == 'application/json' content = json.loads(response.content.decode('utf-8')) @@ -70,51 +79,26 @@ class AccessTokenExchangeViewTest(AccessTokenExchangeTestMixin): assert self.oauth2_adapter.get_client_for_token(token) == self.oauth_client assert set(self.oauth2_adapter.get_token_scope_names(token)) == set(expected_scopes) - def test_single_access_token(self): - def extract_token(response): - """ - Returns the access token from the response payload. - """ - return json.loads(response.content.decode('utf-8'))["access_token"] - - self._setup_provider_response(success=True) - for single_access_token in [True, False]: - with mock.patch( - "openedx.core.djangoapps.auth_exchange.views.constants.SINGLE_ACCESS_TOKEN", - single_access_token, - ): - first_response = self.client.post(self.url, self.data) - second_response = self.client.post(self.url, self.data) - assert first_response.status_code == 200 - assert second_response.status_code == 200 - assert (extract_token(first_response) == extract_token(second_response)) == single_access_token - def test_get_method(self): response = self.client.get(self.url, self.data) - assert response.status_code == 400 - assert json.loads(response.content.decode('utf-8')) ==\ - {'error': 'invalid_request', 'error_description': 'Only POST requests allowed.'} + assert response.status_code == 405 + assert json.loads(response.content.decode('utf-8')) == {'detail': 'Method "GET" not allowed.'} def test_invalid_provider(self): url = reverse("exchange_access_token", kwargs={"backend": "invalid"}) response = self.client.post(url, self.data) assert response.status_code == 404 - @pytest.mark.skip(reason="this is very entangled with dop use in third_party_auth") - def test_invalid_client(self): - """TODO(jinder): this test overwrites function of same name in mixin - Remove when dop has been removed from third party auth - (currently underlying code used dop adapter, which is no longer supported by auth_exchange) + def test_logged_in_user_without_csrf_error(self): """ - pass # lint-amnesty, pylint: disable=unnecessary-pass + Test that a logged in user succeeds without a CSRF permission denied. - @pytest.mark.skip(reason="this is very entangled with dop use in third_party_auth") - def test_missing_fields(self): - """TODO(jinder): this test overwrites function of same name in mixin - Remove when dop has been removed from third party auth - (currently underlying code used dop adapter, which is no longer supported by auth_exchange) + Note: The logged in user does not match the user of the token, but that is not + being treated as an error. """ - pass # lint-amnesty, pylint: disable=unnecessary-pass + self.csrf_client.login(username='test', password='secret') + self._setup_provider_response(success=True) + self._assert_success(self.data, expected_scopes=[], expected_logged_in_user=self.user) def test_disabled_user(self): """ diff --git a/openedx/core/djangoapps/auth_exchange/tests/utils.py b/openedx/core/djangoapps/auth_exchange/tests/utils.py index 09d13950f7..22c1be97be 100644 --- a/openedx/core/djangoapps/auth_exchange/tests/utils.py +++ b/openedx/core/djangoapps/auth_exchange/tests/utils.py @@ -35,7 +35,7 @@ class AccessTokenExchangeTestMixin(ThirdPartyOAuthTestMixin): """ raise NotImplementedError() - def _assert_success(self, data, expected_scopes): + def _assert_success(self, data, expected_scopes, expected_logged_in_user=None): """ Given request data, execute a test and check that the expected scopes were returned (along with any other appropriate assertions). diff --git a/openedx/core/djangoapps/auth_exchange/views.py b/openedx/core/djangoapps/auth_exchange/views.py index b1fa31f943..941f6fa97d 100644 --- a/openedx/core/djangoapps/auth_exchange/views.py +++ b/openedx/core/djangoapps/auth_exchange/views.py @@ -6,8 +6,6 @@ The following are currently implemented: 2. LoginWithAccessTokenView: 1st party (open-edx) OAuth 2.0 access token -> session cookie """ -import logging - import django.contrib.auth as auth import social_django.utils as social_utils from django.conf import settings @@ -27,25 +25,24 @@ from openedx.core.djangoapps.oauth_dispatch import adapters from openedx.core.djangoapps.oauth_dispatch.api import create_dot_access_token from openedx.core.lib.api.authentication import BearerAuthenticationAllowInactiveUser -log = logging.getLogger(__name__) - class AccessTokenExchangeBase(APIView): """ View for token exchange from 3rd party OAuth access token to 1st party OAuth access token. + + Note: This base class was originally created to support multiple libraries, + but we currently only support django-oauth-toolkit (DOT). """ - @method_decorator(csrf_exempt) + # No CSRF protection is required because the provided 3rd party OAuth access + # token is sufficient + authentication_classes = [] + allowed_methods = ['POST'] + @method_decorator(social_utils.psa("social:complete")) def dispatch(self, *args, **kwargs): # pylint: disable=arguments-differ return super(AccessTokenExchangeBase, self).dispatch(*args, **kwargs) # lint-amnesty, pylint: disable=super-with-arguments - def get(self, request, _backend): - """ - Pass through GET requests without the _backend - """ - return super(AccessTokenExchangeBase, self).get(request) # lint-amnesty, pylint: disable=no-member, super-with-arguments - def post(self, request, _backend): """ Handle POST requests to get a first-party access token. @@ -53,13 +50,6 @@ class AccessTokenExchangeBase(APIView): form = AccessTokenExchangeForm(request=request, oauth2_adapter=self.oauth2_adapter, data=request.POST) # lint-amnesty, pylint: disable=no-member if not form.is_valid(): error_response = self.error_response(form.errors) # pylint: disable=no-member - if error_response.status_code == 403: - log.info('message=login_filed_1, status="%d", user="%s" ,agent="%s", error_response"%s"', - error_response.status_code, - request.user, - request.META.get('HTTP_USER_AGENT', ''), - error_response.data - ) return error_response user = form.cleaned_data["user"] @@ -72,10 +62,15 @@ class AccessTokenExchangeBase(APIView): Exchange third party credentials for an edx access token, and return a serialized access token response. """ - edx_access_token = self.create_access_token(request, user, scope, client) return self.access_token_response(edx_access_token) # lint-amnesty, pylint: disable=no-member + def _get_invalid_request_response(self, description): + return Response(status=400, data={ + 'error': 'invalid_request', + 'error_description': description, + }) + class DOTAccessTokenExchangeView(AccessTokenExchangeBase, DOTAccessTokenView): """ @@ -86,12 +81,6 @@ class DOTAccessTokenExchangeView(AccessTokenExchangeBase, DOTAccessTokenView): oauth2_adapter = adapters.DOTAdapter() - def get(self, request, _backend): - return Response(status=400, data={ - 'error': 'invalid_request', - 'error_description': 'Only POST requests allowed.', - }) - def create_access_token(self, request, user, scopes, client): """ Create and return a new access token.