Skip to content

Commit

Permalink
remove jwt authentication from csrf endpoint
Browse files Browse the repository at this point in the history
The main purpose of this commit is to remove the IsAuthenticated
permission from the /csrf/api/v1/token endpoint. The permission was
never needed, and doesn't allow the endpoint to be used to retrieve a
csrf token for use with an anonymous POST.

A temporary Django Setting was added to help with the roll-out:
- EDX_DRF_EXTENSIONS[ENABLE_ANONYMOUS_ACCESS_ROLLOUT]
This setting will help manage the rollout of CSRF protection.

Other minor fixes included:
- Updated README to clarify the purpose of the repo.
- Added testing of the existing csrf app to tox.

ARCH-1269
  • Loading branch information
robrap committed Nov 7, 2019
1 parent 59fb194 commit 491d7e3
Show file tree
Hide file tree
Showing 12 changed files with 129 additions and 43 deletions.
13 changes: 5 additions & 8 deletions README.rst
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,14 @@ edX Django REST Framework Extensions |Travis|_ |Codecov|_
.. |Codecov| image:: http://codecov.io/github/edx/edx-drf-extensions/coverage.svg?branch=master
.. _Codecov: http://codecov.io/github/edx/edx-drf-extensions?branch=master

This library includes extensions of `Django REST Framework <http://www.django-rest-framework.org/>`_
useful for edX applications.
This library is also used for api extensions that do not use DRF.
This library includes various cross-cutting concerns related to APIs. API functionality added to this library must be required for multiple Open edX applications or multiple repositories.

Some of these concerns include extensions of `Django REST Framework <http://www.django-rest-framework.org/>`_ (DRF), which is how the repository initially got its name.

CSRF API
--------

This library also includes a ``csrf`` app containing an API endpoint for retrieving CSRF tokens from
the Django service in which it is installed. This is useful for frontend apps attempting to make POST,
PUT, and DELETE requests to a Django service with Django's CSRF middleware enabled.
One feature of this library is a ``csrf`` app containing an API endpoint for retrieving CSRF tokens from the Django service in which it is installed. This is useful for frontend apps attempting to make POST, PUT, and DELETE requests to a Django service with Django's CSRF middleware enabled.

To make use of this API endpoint:

Expand All @@ -41,8 +39,7 @@ Contributions are very welcome.

Please read `How To Contribute <https://github.com/edx/edx-platform/blob/master/CONTRIBUTING.rst>`_ for details.

Even though they were written with ``edx-platform`` in mind, the guidelines
should be followed for Open edX code in general.
Even though they were written with ``edx-platform`` in mind, the guidelines should be followed for Open edX code in general.

Reporting Security Issues
-------------------------
Expand Down
6 changes: 0 additions & 6 deletions csrf/api/v1/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,9 @@
"""

from django.middleware.csrf import get_token
from rest_framework.permissions import IsAuthenticated
from rest_framework.response import Response
from rest_framework.views import APIView

from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication


class CsrfTokenView(APIView):
"""
Expand All @@ -26,9 +23,6 @@ class CsrfTokenView(APIView):
>>> }
"""

authentication_classes = (JwtAuthentication,)
permission_classes = (IsAuthenticated,)

def get(self, request):
"""
GET /csrf/api/v1/token
Expand Down
7 changes: 1 addition & 6 deletions csrf/tests/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,6 @@
from rest_framework import status
from rest_framework.test import APITestCase

from edx_rest_framework_extensions.auth.jwt.tests.utils import generate_jwt
from edx_rest_framework_extensions.tests.factories import UserFactory


class CsrfTokenTests(APITestCase):
""" Tests for the CSRF token endpoint. """
Expand All @@ -16,9 +13,7 @@ def test_get_token(self):
Ensure we can get a CSRF token.
"""
url = reverse('csrf_token')
user = UserFactory()
jwt = generate_jwt(user)
self.client.credentials(HTTP_AUTHORIZATION='JWT {}'.format(jwt)) # pylint: disable=no-member
response = self.client.get(url, format='json')
self.assertEqual(response.status_code, status.HTTP_200_OK)
self.assertIn('csrfToken', response.data)
self.assertIsNotNone(response.data['csrfToken'])
2 changes: 1 addition & 1 deletion edx_rest_framework_extensions/__init__.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
""" edx Django REST Framework extensions. """

__version__ = '2.4.2' # pragma: no cover
__version__ = '2.4.3' # pragma: no cover
47 changes: 46 additions & 1 deletion edx_rest_framework_extensions/auth/jwt/authentication.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,19 +3,28 @@
import logging

from django.contrib.auth import get_user_model
from django.middleware.csrf import CsrfViewMiddleware
from rest_framework import exceptions
from rest_framework_jwt.authentication import (
BaseJSONWebTokenAuthentication,
JSONWebTokenAuthentication,
)

from edx_rest_framework_extensions.auth.jwt.constants import USE_JWT_COOKIE_HEADER
from edx_rest_framework_extensions.auth.jwt.decoder import jwt_decode_handler
from edx_rest_framework_extensions.config import ENABLE_ANONYMOUS_ACCESS_ROLLOUT
from edx_rest_framework_extensions.settings import get_setting


logger = logging.getLogger(__name__)


class CSRFCheck(CsrfViewMiddleware):
def _reject(self, request, reason):
# Return the failure reason instead of an HttpResponse
return reason


class JwtAuthentication(JSONWebTokenAuthentication):
"""
JSON Web Token based authentication.
Expand Down Expand Up @@ -54,7 +63,28 @@ def get_jwt_claim_mergeable_attributes(self):

def authenticate(self, request):
try:
return super(JwtAuthentication, self).authenticate(request)
user_and_auth = super(JwtAuthentication, self).authenticate(request)

is_anonymous_access_rollout_enabled = get_setting(ENABLE_ANONYMOUS_ACCESS_ROLLOUT)
# Use Django Setting for rollout to coordinate with frontend-auth changes for
# anonymous access being available across MFEs.
if not is_anonymous_access_rollout_enabled:
return user_and_auth

# Unauthenticated, CSRF validation not required
if not user_and_auth:
return user_and_auth

# Not using JWT cookies, CSRF validation not required
use_jwt_cookie_requested = request.META.get(USE_JWT_COOKIE_HEADER)
if not use_jwt_cookie_requested:
return user_and_auth

self.enforce_csrf(request)

# CSRF passed validation with authenticated user
return user_and_auth

except Exception as ex:
# Errors in production do not need to be logged (as they may be noisy),
# but debug logging can help quickly resolve issues during development.
Expand Down Expand Up @@ -125,6 +155,21 @@ def authenticate_credentials(self, payload):

return user

def enforce_csrf(self, request):
"""
Enforce CSRF validation for Jwt cookie authentication.
Copied from SessionAuthentication.
See https://github.com/encode/django-rest-framework/blob/3f19e66d9f2569895af6e91455e5cf53b8ce5640/rest_framework/authentication.py#L131-L141 # noqa E501 line too long
"""
check = CSRFCheck()
# populates request.META['CSRF_COOKIE'], which is used in process_view()
check.process_request(request)
reason = check.process_view(request, None, (), {})
if reason:
# CSRF failed, bail with explicit error message
raise exceptions.PermissionDenied('CSRF Failed: %s' % reason)


def is_jwt_authenticated(request):
successful_authenticator = getattr(request, 'successful_authenticator', None)
Expand Down
1 change: 1 addition & 0 deletions edx_rest_framework_extensions/auth/jwt/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,4 @@
"""

JWT_DELIMITER = '.'
USE_JWT_COOKIE_HEADER = 'HTTP_USE_JWT_COOKIE'
6 changes: 4 additions & 2 deletions edx_rest_framework_extensions/auth/jwt/middleware.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,10 @@
from rest_framework.request import Request
from rest_framework_jwt.authentication import BaseJSONWebTokenAuthentication

from edx_rest_framework_extensions.auth.jwt.constants import JWT_DELIMITER
from edx_rest_framework_extensions.auth.jwt.constants import (
JWT_DELIMITER,
USE_JWT_COOKIE_HEADER,
)
from edx_rest_framework_extensions.auth.jwt.cookies import (
jwt_cookie_header_payload_name,
jwt_cookie_name,
Expand All @@ -27,7 +30,6 @@


log = logging.getLogger(__name__)
USE_JWT_COOKIE_HEADER = 'HTTP_USE_JWT_COOKIE'


class EnsureJWTAuthSettingsMiddleware(MiddlewareMixin):
Expand Down
61 changes: 48 additions & 13 deletions edx_rest_framework_extensions/auth/jwt/tests/test_authentication.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,18 @@
import mock
from django.contrib.auth import get_user_model
from django.test import RequestFactory, TestCase, override_settings
from rest_framework.exceptions import AuthenticationFailed
from rest_framework.exceptions import AuthenticationFailed, PermissionDenied
from rest_framework_jwt.authentication import JSONWebTokenAuthentication

from edx_rest_framework_extensions.auth.jwt import authentication
from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication
from edx_rest_framework_extensions.auth.jwt.constants import USE_JWT_COOKIE_HEADER
from edx_rest_framework_extensions.auth.jwt.decoder import jwt_decode_handler
from edx_rest_framework_extensions.auth.jwt.tests.utils import (
generate_jwt_token,
generate_latest_version_payload,
)
from edx_rest_framework_extensions.config import ENABLE_ANONYMOUS_ACCESS_ROLLOUT
from edx_rest_framework_extensions.tests import factories


Expand Down Expand Up @@ -174,18 +176,51 @@ def test_authenticate_credentials_no_usernames(self):
with self.assertRaises(AuthenticationFailed):
JwtAuthentication().authenticate_credentials({'email': 'test@example.com'})

def test_authenticate(self):
""" Verify exceptions raised during authentication are properly logged. """
request = RequestFactory().get('/')

with mock.patch.object(JSONWebTokenAuthentication, 'authenticate', side_effect=Exception):
with mock.patch.object(Logger, 'debug') as logger:
self.assertRaises(
Exception,
JwtAuthentication().authenticate,
request
)
self.assertTrue(logger.called)
_MOCK_ANONYMOUS_RETURN = None
_MOCK_USER_AUTH_RETURN = ('mock-user', "mock-auth")

@ddt.data(
# CSRF exempt because roll-out is not enabled
(False, False, _MOCK_USER_AUTH_RETURN),
# CSRF exempt because roll-out is not enabled (even though JWT cookies are used)
(False, True, _MOCK_USER_AUTH_RETURN),
# CSRF exempt because of anonymous access (similar to SessionAuthentication)
(True, True, _MOCK_ANONYMOUS_RETURN),
# CSRF exempt because request uses JWT authentication without JWT cookies
(True, False, _MOCK_USER_AUTH_RETURN),
)
@ddt.unpack
def test_authenticate_csrf_exempt(self, enable_rollout, use_jwt_cookies, mocked_return_value_user_and_auth):
""" Verify authenticate success for cases that are CSRF exempt. """
request = RequestFactory().post('/')
if use_jwt_cookies:
request.META[USE_JWT_COOKIE_HEADER] = 'true'

with mock.patch.object(JSONWebTokenAuthentication, 'authenticate', return_value=mocked_return_value_user_and_auth): # noqa E501 line too long
with override_settings(EDX_DRF_EXTENSIONS={ENABLE_ANONYMOUS_ACCESS_ROLLOUT: enable_rollout}):
actual_user_and_auth = JwtAuthentication().authenticate(request)

self.assertEqual(mocked_return_value_user_and_auth, actual_user_and_auth)

@ddt.data(
# CSRF protected because using JWT cookies to successfully authenticate (similar to SessionAuthentication)
(True, True, _MOCK_USER_AUTH_RETURN),
)
@ddt.unpack
def test_authenticate_csrf_protected(self, enable_rollout, use_jwt_cookies, mocked_return_value_user_and_auth):
""" Verify authenticate exception for CSRF protected cases. """
request = RequestFactory().post('/')
if use_jwt_cookies:
request.META[USE_JWT_COOKIE_HEADER] = 'true'

with mock.patch.object(JSONWebTokenAuthentication, 'authenticate', return_value=mocked_return_value_user_and_auth): # noqa E501 line too long
with override_settings(EDX_DRF_EXTENSIONS={ENABLE_ANONYMOUS_ACCESS_ROLLOUT: enable_rollout}):
with mock.patch.object(Logger, 'debug') as debug_logger:
with self.assertRaises(PermissionDenied) as context_manager:
JwtAuthentication().authenticate(request)

self.assertEqual(context_manager.exception.detail, 'CSRF Failed: CSRF cookie not set.')
self.assertTrue(debug_logger.called)

@ddt.data(True, False)
def test_get_decoded_jwt_from_auth(self, is_jwt_authentication):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,13 @@
from rest_framework.viewsets import ViewSet
from rest_framework_jwt.authentication import BaseJSONWebTokenAuthentication

from edx_rest_framework_extensions.auth.jwt.constants import USE_JWT_COOKIE_HEADER
from edx_rest_framework_extensions.auth.jwt.cookies import (
jwt_cookie_header_payload_name,
jwt_cookie_name,
jwt_cookie_signature_name,
)
from edx_rest_framework_extensions.auth.jwt.middleware import (
USE_JWT_COOKIE_HEADER,
EnsureJWTAuthSettingsMiddleware,
JwtAuthCookieMiddleware,
JwtRedirectToLoginIfUnauthenticatedMiddleware,
Expand Down
15 changes: 14 additions & 1 deletion edx_rest_framework_extensions/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,24 @@
# .. toggle_implementation: DjangoSetting
# .. toggle_default: False
# .. toggle_description: Toggle for setting request.user with jwt cookie authentication
# .. toggle_category: experiments
# .. toggle_category: micro-frontend
# .. toggle_use_cases: incremental_release
# .. toggle_creation_date: 2019-10-15
# .. toggle_expiration_date: 2019-12-31
# .. toggle_warnings: This feature fixed ecommerce, but broke edx-platform. The toggle enables us to fix over time.
# .. toggle_tickets: ARCH-1210, ARCH-1199, ARCH-1197
# .. toggle_status: supported
ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE = 'ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE'

# .. toggle_name: EDX_DRF_EXTENSIONS[ENABLE_ANONYMOUS_ACCESS_ROLLOUT]
# .. toggle_implementation: DjangoSetting
# .. toggle_default: False
# .. toggle_description: Toggle for enabling some functionality related to anonymous access
# .. toggle_category: micro-frontend
# .. toggle_use_cases: incremental_release
# .. toggle_creation_date: 2019-11-06
# .. toggle_expiration_date: 2019-12-31
# .. toggle_warnings: Requires coordination with MFE updates of frontend-auth refactor.
# .. toggle_tickets: ARCH-1229
# .. toggle_status: supported
ENABLE_ANONYMOUS_ACCESS_ROLLOUT = 'ENABLE_ANONYMOUS_ACCESS_ROLLOUT'
6 changes: 5 additions & 1 deletion edx_rest_framework_extensions/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,10 @@
from django.conf import settings
from rest_framework_jwt.settings import api_settings

from edx_rest_framework_extensions.config import ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE
from edx_rest_framework_extensions.config import (
ENABLE_ANONYMOUS_ACCESS_ROLLOUT,
ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE,
)


logger = logging.getLogger(__name__)
Expand All @@ -31,6 +34,7 @@
},
'JWT_PAYLOAD_MERGEABLE_USER_ATTRIBUTES': (),
ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE: False,
ENABLE_ANONYMOUS_ACCESS_ROLLOUT: False,
}


Expand Down
6 changes: 3 additions & 3 deletions tox.ini
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,14 @@ deps =
-rtest_requirements.txt

commands =
django-admin.py test {posargs:edx_rest_framework_extensions} --settings=test_settings --with-coverage --cover-package=edx_rest_framework_extensions
django-admin.py test {posargs:csrf edx_rest_framework_extensions} --settings=test_settings --with-coverage --cover-package=csrf,edx_rest_framework_extensions
coverage report

[testenv:quality]
commands =
pycodestyle
isort --recursive edx_rest_framework_extensions csrf --check-only --diff
pylint --disable=R,C --rcfile=pylintrc edx_rest_framework_extensions csrf
isort --recursive csrf edx_rest_framework_extensions --check-only --diff
pylint --disable=R,C --rcfile=pylintrc csrf edx_rest_framework_extensions

[testenv:docs]
changedir=docs
Expand Down

0 comments on commit 491d7e3

Please sign in to comment.