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
This commit is contained in:
Robert Raposa
@@ -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':
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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):
|
||||
"""
|
||||
|
||||
@@ -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).
|
||||
|
||||
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user