From e4f935aab3f1b9621758131520cacc74158ab99d Mon Sep 17 00:00:00 2001 From: Nimisha Asthagiri Date: Mon, 11 Mar 2019 20:59:30 -0400 Subject: [PATCH] JWT Cookie updates: remove refresh cookie, cookie expires with JWT ARCH-418, ARCH-548 --- openedx/core/djangoapps/oauth_dispatch/api.py | 44 ---------- .../decisions/0009-jwt-in-session-cookie.rst | 6 -- .../oauth_dispatch/tests/test_api.py | 37 -------- openedx/core/djangoapps/user_authn/cookies.py | 88 +++++++++---------- .../user_authn/tests/test_cookies.py | 34 +++---- .../core/djangoapps/user_authn/views/login.py | 3 +- .../user_authn/views/tests/test_login.py | 2 +- 7 files changed, 60 insertions(+), 154 deletions(-) diff --git a/openedx/core/djangoapps/oauth_dispatch/api.py b/openedx/core/djangoapps/oauth_dispatch/api.py index edde0bd407..d2b61a1c52 100644 --- a/openedx/core/djangoapps/oauth_dispatch/api.py +++ b/openedx/core/djangoapps/oauth_dispatch/api.py @@ -1,7 +1,4 @@ """ OAuth related Python apis. """ -import json - -from oauthlib.oauth2.rfc6749.errors import OAuth2Error from oauthlib.oauth2.rfc6749.tokens import BearerToken from oauth2_provider.models import AccessToken as dot_access_token from oauth2_provider.models import RefreshToken as dot_refresh_token @@ -41,26 +38,6 @@ def create_dot_access_token(request, user, client, expires_in=None, scopes=None) return token_generator.create_token(request, refresh_token=True) -def refresh_dot_access_token(request, client_id, refresh_token, expires_in=None): - """ - Create and return a new (persisted) access token, given a previously created - refresh_token, possibly returned from create_dot_access_token above. - """ - expires_in = _get_expires_in_value(expires_in) - auth_core = _get_oauthlib_core(expires_in) - _populate_refresh_token_request(request, client_id, refresh_token) - - # Note: Unlike create_dot_access_token, we use the top-level auth library - # code for creating the token since we want to enforce registered validations - # (valid refresh token, valid client, etc), rather than create the token - # ourselves directly. - _, _, body, status = auth_core.create_token_response(request) # returns uri, headers, body, status - - if status != 200: - raise OAuth2Error(body) - return json.loads(body) - - def _get_expires_in_value(expires_in): """ Returns the expires_in value to use for the token. @@ -81,24 +58,3 @@ def _populate_create_access_token_request(request, user, client, scopes): request.refresh_token = None request.extra_credentials = None request.grant_type = client.authorization_grant_type - - -def _populate_refresh_token_request(request, client_id, refresh_token): - """ - django-oauth-toolkit expects parameters passed through the request's POST. - """ - request.POST = dict( - client_id=client_id, - refresh_token=refresh_token, - grant_type='refresh_token', - ) - - -def _get_oauthlib_core(expires_in): - """ - Based on oauth2_provider.oauth2_backends.get_oauthlib_core, but allows - passing in a value for token_expires_in. - """ - validator = dot_settings.OAUTH2_VALIDATOR_CLASS() - server = dot_settings.OAUTH2_SERVER_CLASS(validator, token_expires_in=expires_in) - return dot_settings.OAUTH2_BACKEND_CLASS(server) diff --git a/openedx/core/djangoapps/oauth_dispatch/docs/decisions/0009-jwt-in-session-cookie.rst b/openedx/core/djangoapps/oauth_dispatch/docs/decisions/0009-jwt-in-session-cookie.rst index 2eb5608927..d48b974cf8 100644 --- a/openedx/core/djangoapps/oauth_dispatch/docs/decisions/0009-jwt-in-session-cookie.rst +++ b/openedx/core/djangoapps/oauth_dispatch/docs/decisions/0009-jwt-in-session-cookie.rst @@ -113,12 +113,6 @@ JWT Cookie Lifetime endpoint, or preemptively recognize an imminent expiration. To automatically refresh the JWT cookie, the microfrontend will call a new endpoint ("refresh") that returns a new JWT Cookie to keep the user's session alive. - * To support this, the login endpoint will include 3 related cookies in its response: - - * **Two JWT Cookies** (as described above), with a *domain* setting so that it is forwarded to any microservice - in the system. - * **JWT Refresh Cookie**, with a *domain* setting so that it is sent to the login service only. - #. **Remove JWT Cookie on Logout.** When the user logs out, we will remove all JWT-related cookies in the response, which will remove them from the user's browser cookie jar. Thus, the user will be logged out of all the microfrontends. diff --git a/openedx/core/djangoapps/oauth_dispatch/tests/test_api.py b/openedx/core/djangoapps/oauth_dispatch/tests/test_api.py index 81150a2b2c..5d99069bec 100644 --- a/openedx/core/djangoapps/oauth_dispatch/tests/test_api.py +++ b/openedx/core/djangoapps/oauth_dispatch/tests/test_api.py @@ -63,40 +63,3 @@ class TestOAuthDispatchAPI(TestCase): ) self.assertDictContainsSubset({u'scope': u'profile'}, token) self.assertDictContainsSubset({u'expires_in': expires_in}, token) - - def test_refresh_token_success(self): - old_token = api.create_dot_access_token(HttpRequest(), self.user, self.client) - new_token = api.refresh_dot_access_token(HttpRequest(), self.client.client_id, old_token['refresh_token']) - self.assertDictContainsSubset( - { - u'token_type': u'Bearer', - u'expires_in': EXPECTED_DEFAULT_EXPIRES_IN, - u'scope': u'', - }, - new_token, - ) - - # verify new tokens are generated - self.assertNotEqual(old_token['access_token'], new_token['access_token']) - self.assertNotEqual(old_token['refresh_token'], new_token['refresh_token']) - - # verify old token is replaced by the new token - with self.assertRaises(AccessToken.DoesNotExist): - self._assert_stored_token(old_token['access_token'], self.user, self.client) - self._assert_stored_token(new_token['access_token'], self.user, self.client) - - def test_refresh_token_invalid_client(self): - token = api.create_dot_access_token(HttpRequest(), self.user, self.client) - with self.assertRaises(api.OAuth2Error) as error: - api.refresh_dot_access_token( - HttpRequest(), 'invalid_client_id', token['refresh_token'], - ) - self.assertIn('invalid_client', error.exception.description) - - def test_refresh_token_invalid_token(self): - api.create_dot_access_token(HttpRequest(), self.user, self.client) - with self.assertRaises(api.OAuth2Error) as error: - api.refresh_dot_access_token( - HttpRequest(), self.client.client_id, 'invalid_refresh_token', - ) - self.assertIn('invalid_grant', error.exception.description) diff --git a/openedx/core/djangoapps/user_authn/cookies.py b/openedx/core/djangoapps/user_authn/cookies.py index bf45c8a749..d21cf6cf6c 100644 --- a/openedx/core/djangoapps/user_authn/cookies.py +++ b/openedx/core/djangoapps/user_authn/cookies.py @@ -18,7 +18,7 @@ from edx_rest_framework_extensions.auth.jwt import cookies as jwt_cookies from edx_rest_framework_extensions.auth.jwt.constants import JWT_DELIMITER from oauth2_provider.models import Application from openedx.core.djangoapps.oauth_dispatch.adapters import DOTAdapter -from openedx.core.djangoapps.oauth_dispatch.api import create_dot_access_token, refresh_dot_access_token +from openedx.core.djangoapps.oauth_dispatch.api import create_dot_access_token from openedx.core.djangoapps.oauth_dispatch.jwt import create_jwt_from_token from openedx.core.djangoapps.user_api.accounts.utils import retrieve_last_sitewide_block_completed from openedx.core.djangoapps.user_authn.exceptions import AuthFailedError @@ -38,9 +38,6 @@ JWT_COOKIE_NAMES = ( # Signature section of a JSON Web Token. jwt_cookies.jwt_cookie_signature_name(), - - # Refresh token, which can be used to get a new JSON Web Token. - jwt_cookies.jwt_refresh_cookie_name(), ) # TODO (ARCH-245): Remove the following deprecated cookies. @@ -89,22 +86,14 @@ def delete_logged_in_cookies(response): def standard_cookie_settings(request): """ Returns the common cookie settings (e.g. expiration time). """ - if request.session.get_expire_at_browser_close(): - max_age = None - expires = None - else: - max_age = request.session.get_expiry_age() - _expires_time = time.time() + max_age - expires = cookie_date(_expires_time) - cookie_settings = { - 'max_age': max_age, - 'expires': expires, 'domain': settings.SESSION_COOKIE_DOMAIN, 'path': '/', 'httponly': None, } + _set_expires_in_cookie_settings(cookie_settings, request.session.get_expiry_age()) + # In production, TLS should be enabled so that this cookie is encrypted # when we send it. We also need to set "secure" to True so that the browser # will transmit it only over secure connections. @@ -119,6 +108,20 @@ def standard_cookie_settings(request): return cookie_settings +def _set_expires_in_cookie_settings(cookie_settings, expires_in): + """ + Updates the max_age and expires fields of the given cookie_settings, + based on the value of expires_in. + """ + expires_time = time.time() + expires_in + expires = cookie_date(expires_time) + + cookie_settings.update({ + 'max_age': expires_in, + 'expires': expires, + }) + + def set_logged_in_cookies(request, response, user): """ Set cookies at the time of user login. See ALL_LOGGED_IN_COOKIE_NAMES to see @@ -151,23 +154,17 @@ def set_logged_in_cookies(request, response, user): return response -def refresh_jwt_cookies(request, response): +def refresh_jwt_cookies(request, response, user): """ - Resets the JWT related cookies in the response, while expecting a refresh - cookie in the request. + Resets the JWT related cookies in the response for the given user. """ - try: - refresh_token = request.COOKIES[jwt_cookies.jwt_refresh_cookie_name()] - except KeyError: - raise AuthFailedError(u"JWT Refresh Cookie not found in request.") - - # TODO don't extend the cookie expiration - reuse value from existing cookie - cookie_settings = standard_cookie_settings(request) - _create_and_set_jwt_cookies(response, request, cookie_settings, refresh_token=refresh_token) + if user.is_authenticated and not user.is_anonymous: + cookie_settings = standard_cookie_settings(request) + _create_and_set_jwt_cookies(response, request, cookie_settings, user=user) return response -def _set_deprecated_user_info_cookie(response, request, user, cookie_settings=None): +def _set_deprecated_user_info_cookie(response, request, user, cookie_settings): """ Sets the user info cookie on the response. @@ -184,7 +181,6 @@ def _set_deprecated_user_info_cookie(response, request, user, cookie_settings=No } } """ - cookie_settings = cookie_settings or standard_cookie_settings(request) user_info = _get_user_info_cookie_data(request, user) response.set_cookie( settings.EDXMKTG_USER_INFO_COOKIE_NAME.encode('utf-8'), @@ -249,7 +245,7 @@ def _get_user_info_cookie_data(request, user): return user_info -def _create_and_set_jwt_cookies(response, request, cookie_settings, user=None, refresh_token=None): +def _create_and_set_jwt_cookies(response, request, cookie_settings, user=None): """ Sets a cookie containing a JWT on the response. """ # Skip setting JWT cookies for most unit tests, since it raises errors when @@ -259,31 +255,32 @@ def _create_and_set_jwt_cookies(response, request, cookie_settings, user=None, r if settings.FEATURES.get('DISABLE_SET_JWT_COOKIES_FOR_TESTS', False): return - # For security reasons, the JWT that is embedded inside the cookie expires - # much sooner than the cookie itself, per the following setting. expires_in = settings.JWT_AUTH['JWT_IN_COOKIE_EXPIRATION'] + _set_expires_in_cookie_settings(cookie_settings, expires_in) - oauth_application = _get_login_oauth_client() - if refresh_token: - access_token = refresh_dot_access_token( - request, oauth_application.client_id, refresh_token, expires_in=expires_in, - ) - else: - access_token = create_dot_access_token( - # Note: Scopes for JWT cookies do not require additional permissions - request, user, oauth_application, expires_in=expires_in, scopes=['user_id', 'email', 'profile'], - ) - jwt = create_jwt_from_token(access_token, DOTAdapter(), use_asymmetric_key=True) + jwt = _create_jwt(request, user, expires_in) jwt_header_and_payload, jwt_signature = _parse_jwt(jwt) + _set_jwt_cookies( response, cookie_settings, jwt_header_and_payload, jwt_signature, - access_token['refresh_token'], ) +def _create_jwt(request, user, expires_in): + """ + Creates and returns a jwt for the given user with the given expires_in value. + """ + oauth_application = _get_login_oauth_client() + access_token = create_dot_access_token( + # Note: Scopes for JWT cookies do not require additional permissions + request, user, oauth_application, expires_in=expires_in, scopes=['user_id', 'email', 'profile'], + ) + return create_jwt_from_token(access_token, DOTAdapter(), use_asymmetric_key=True) + + def _parse_jwt(jwt): """ Parses and returns the following parts of the jwt: header_and_payload, signature @@ -294,7 +291,7 @@ def _parse_jwt(jwt): return header_and_payload, signature -def _set_jwt_cookies(response, cookie_settings, jwt_header_and_payload, jwt_signature, refresh_token): +def _set_jwt_cookies(response, cookie_settings, jwt_header_and_payload, jwt_signature): """ Sets the given jwt_header_and_payload, jwt_signature, and refresh token in 3 different cookies. The latter 2 cookies are set as httponly. @@ -312,11 +309,6 @@ def _set_jwt_cookies(response, cookie_settings, jwt_header_and_payload, jwt_sign jwt_signature, **cookie_settings ) - response.set_cookie( - jwt_cookies.jwt_refresh_cookie_name(), - refresh_token, - **cookie_settings - ) def _get_login_oauth_client(): diff --git a/openedx/core/djangoapps/user_authn/tests/test_cookies.py b/openedx/core/djangoapps/user_authn/tests/test_cookies.py index 7e852d9194..f6a18fc39e 100644 --- a/openedx/core/djangoapps/user_authn/tests/test_cookies.py +++ b/openedx/core/djangoapps/user_authn/tests/test_cookies.py @@ -25,9 +25,8 @@ class CookieTests(TestCase): self.request.user = self.user self.request.session = self._get_stub_session() - def _get_stub_session(self, expire_at_browser_close=False, max_age=604800): + def _get_stub_session(self, max_age=604800): return MagicMock( - get_expire_at_browser_close=lambda: expire_at_browser_close, get_expiry_age=lambda: max_age, ) @@ -81,9 +80,12 @@ class CookieTests(TestCase): """ Verify all expected_cookies are present in the response. """ self.assertSetEqual(set(response.cookies.keys()), set(expected_cookies)) - def _assert_consistent_expires(self, response): - """ Verify all cookies in the response have the same expiration. """ - self.assertEqual(1, len(set([response.cookies[c]['expires'] for c in response.cookies]))) + def _assert_consistent_expires(self, response, num_of_unique_expires=1): + """ Verify cookies in the response have the same expiration, as expected. """ + self.assertEqual( + num_of_unique_expires, + len(set([response.cookies[c]['expires'] for c in response.cookies])), + ) def test_get_user_info_cookie_data(self): actual = cookies_api._get_user_info_cookie_data(self.request, self.user) # pylint: disable=protected-access @@ -114,7 +116,7 @@ class CookieTests(TestCase): self._set_use_jwt_cookie_header(self.request) response = cookies_api.set_logged_in_cookies(self.request, HttpResponse(), self.user) self._assert_cookies_present(response, cookies_api.ALL_LOGGED_IN_COOKIE_NAMES) - self._assert_consistent_expires(response) + self._assert_consistent_expires(response, num_of_unique_expires=2) self._assert_recreate_jwt_from_cookies(response, can_recreate=True) @patch.dict("django.conf.settings.FEATURES", {"DISABLE_SET_JWT_COOKIES_FOR_TESTS": False}) @@ -130,17 +132,15 @@ class CookieTests(TestCase): @patch.dict("django.conf.settings.FEATURES", {"DISABLE_SET_JWT_COOKIES_FOR_TESTS": False}) def test_refresh_jwt_cookies(self): - def _get_refresh_token_value(response): - return response.cookies[cookies_api.jwt_cookies.jwt_refresh_cookie_name()].value - setup_login_oauth_client() self._set_use_jwt_cookie_header(self.request) - response = cookies_api.set_logged_in_cookies(self.request, HttpResponse(), self.user) - self._copy_cookies_to_request(response, self.request) + response = cookies_api.refresh_jwt_cookies(self.request, HttpResponse(), self.user) + self._assert_cookies_present(response, cookies_api.JWT_COOKIE_NAMES) + self._assert_consistent_expires(response, num_of_unique_expires=1) + self._assert_recreate_jwt_from_cookies(response, can_recreate=True) - new_response = cookies_api.refresh_jwt_cookies(self.request, HttpResponse()) - self._assert_recreate_jwt_from_cookies(new_response, can_recreate=True) - self.assertNotEqual( - _get_refresh_token_value(response), - _get_refresh_token_value(new_response), - ) + @patch.dict("django.conf.settings.FEATURES", {"DISABLE_SET_JWT_COOKIES_FOR_TESTS": False}) + def test_refresh_jwt_cookies_anonymous_user(self): + anonymous_user = AnonymousUserFactory() + response = cookies_api.refresh_jwt_cookies(self.request, HttpResponse(), anonymous_user) + self._assert_cookies_present(response, []) diff --git a/openedx/core/djangoapps/user_authn/views/login.py b/openedx/core/djangoapps/user_authn/views/login.py index 058b207e3e..c5f61a1dd3 100644 --- a/openedx/core/djangoapps/user_authn/views/login.py +++ b/openedx/core/djangoapps/user_authn/views/login.py @@ -367,11 +367,12 @@ def login_user(request): # to get a CSRF token before we need to refresh adds too much # complexity. @csrf_exempt +@login_required @require_http_methods(['POST']) def login_refresh(request): try: response = JsonResponse({'success': True}) - return refresh_jwt_cookies(request, response) + return refresh_jwt_cookies(request, response, request.user) except AuthFailedError as error: log.exception(error.get_response()) return JsonResponse(error.get_response(), status=400) diff --git a/openedx/core/djangoapps/user_authn/views/tests/test_login.py b/openedx/core/djangoapps/user_authn/views/tests/test_login.py index 4902c803b8..29e10cc941 100644 --- a/openedx/core/djangoapps/user_authn/views/tests/test_login.py +++ b/openedx/core/djangoapps/user_authn/views/tests/test_login.py @@ -293,7 +293,7 @@ class LoginTest(CacheIsolationTestCase): def test_login_refresh(self): def _assert_jwt_cookie_present(response): self.assertEqual(response.status_code, 200) - self.assertIn(jwt_cookies.jwt_refresh_cookie_name(), self.client.cookies) + self.assertIn(jwt_cookies.jwt_cookie_header_payload_name(), self.client.cookies) setup_login_oauth_client() response, _ = self._login_response('test@edx.org', 'test_password')