-
Notifications
You must be signed in to change notification settings - Fork 672
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add JWT httpOnly cookie storage. #157
base: master
Are you sure you want to change the base?
Changes from all commits
8dd263d
8661f5b
34431cf
c1e15c7
078224e
56222b8
2b74318
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
from django.utils.translation import gettext_lazy as _ | ||
from rest_framework import HTTP_HEADER_ENCODING, authentication | ||
from rest_framework import HTTP_HEADER_ENCODING, authentication, exceptions | ||
from rest_framework.authentication import CSRFCheck | ||
|
||
from .exceptions import AuthenticationFailed, InvalidToken, TokenError | ||
from .settings import api_settings | ||
|
@@ -16,6 +17,19 @@ | |
) | ||
|
||
|
||
def enforce_csrf(request): | ||
""" | ||
Enforce CSRF validation. | ||
""" | ||
check = CSRFCheck() | ||
# populates request.META['CSRF_COOKIE'], which is used in process_view() | ||
check.process_request(request) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. BehaviourOn this line the Later on line Later After we have these csrf tokens we compare Problem?My Is this supposed to happen? Or should my backend set NoteWhen my frontend and backend are running from different servers my frontend doesn't have access to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, try setting: |
||
reason = check.process_view(request, None, (), {}) | ||
if reason: | ||
# CSRF failed, bail with explicit error message | ||
raise exceptions.PermissionDenied('CSRF Failed: %s' % reason) | ||
NiyazNz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
|
||
class JWTAuthentication(authentication.BaseAuthentication): | ||
""" | ||
An authentication plugin that authenticates requests through a JSON web | ||
|
@@ -26,15 +40,25 @@ class JWTAuthentication(authentication.BaseAuthentication): | |
def authenticate(self, request): | ||
header = self.get_header(request) | ||
if header is None: | ||
return None | ||
|
||
raw_token = self.get_raw_token(header) | ||
if not api_settings.AUTH_COOKIE: | ||
return None | ||
else: | ||
raw_token = request.COOKIES.get(api_settings.AUTH_COOKIE) or None | ||
else: | ||
raw_token = self.get_raw_token(header) | ||
if raw_token is None: | ||
return None | ||
|
||
validated_token = self.get_validated_token(raw_token) | ||
|
||
return self.get_user(validated_token), validated_token | ||
user = self.get_user(validated_token) | ||
if not user or not user.is_active: | ||
return None | ||
|
||
if api_settings.AUTH_COOKIE: | ||
enforce_csrf(request) | ||
|
||
return user, validated_token | ||
|
||
def authenticate_header(self, request): | ||
return '{0} realm="{1}"'.format( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,6 +34,18 @@ | |
'SLIDING_TOKEN_REFRESH_EXP_CLAIM': 'refresh_exp', | ||
'SLIDING_TOKEN_LIFETIME': timedelta(minutes=5), | ||
'SLIDING_TOKEN_REFRESH_LIFETIME': timedelta(days=1), | ||
|
||
# Cookie name. Enables cookies if value is set. | ||
'AUTH_COOKIE': None, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel like this should have a better name, more verbose like |
||
# A string like "example.com", or None for standard domain cookie. | ||
'AUTH_COOKIE_DOMAIN': None, | ||
# Whether the auth cookies should be secure (https:// only). | ||
'AUTH_COOKIE_SECURE': False, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since you're working with CSRF, you should instead use this Django setting instead of |
||
# The path of the auth cookie. | ||
'AUTH_COOKIE_PATH': '/', | ||
# Whether to set the flag restricting cookie leaks on cross-site requests. | ||
# This can be 'Lax', 'Strict', or None to disable the flag. | ||
'AUTH_COOKIE_SAMESITE': 'Lax', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similarly to the AUTH_COOKIE_SECURE: CSRF_COOKIE_SAMESITE |
||
} | ||
|
||
IMPORT_STRINGS = ( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,15 @@ | ||
from datetime import datetime | ||
|
||
from django.middleware import csrf | ||
from django.utils.translation import ugettext_lazy as _ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
from rest_framework import generics, status | ||
from rest_framework.exceptions import NotAuthenticated | ||
from rest_framework.response import Response | ||
from rest_framework.reverse import reverse | ||
from rest_framework.views import APIView | ||
|
||
from rest_framework_simplejwt.settings import api_settings | ||
from rest_framework_simplejwt.tokens import RefreshToken | ||
from . import serializers | ||
from .authentication import AUTH_HEADER_TYPES | ||
from .exceptions import InvalidToken, TokenError | ||
|
@@ -28,10 +37,69 @@ def post(self, request, *args, **kwargs): | |
except TokenError as e: | ||
raise InvalidToken(e.args[0]) | ||
|
||
return Response(serializer.validated_data, status=status.HTTP_200_OK) | ||
response = Response(serializer.validated_data, status=status.HTTP_200_OK) | ||
|
||
if api_settings.AUTH_COOKIE: | ||
csrf.get_token(self.request) | ||
response = self.set_auth_cookies(response, serializer.validated_data) | ||
|
||
return response | ||
NiyazNz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
def set_auth_cookies(self, response, data): | ||
return response | ||
|
||
|
||
class TokenObtainPairView(TokenViewBase): | ||
class TokenRefreshViewBase(TokenViewBase): | ||
def extract_token_from_cookie(self, request): | ||
return request | ||
|
||
def post(self, request, *args, **kwargs): | ||
if api_settings.AUTH_COOKIE: | ||
request = self.extract_token_from_cookie(request) | ||
return super().post(request, *args, **kwargs) | ||
|
||
|
||
class TokenCookieViewMixin: | ||
token_refresh_view_name = 'token_refresh' | ||
|
||
def extract_token_from_cookie(self, request): | ||
"""Extracts token from cookie and sets it in request.data as it would be sent by the user""" | ||
if not request.data: | ||
token = request.COOKIES.get('{}_refresh'.format(api_settings.AUTH_COOKIE)) | ||
if not token: | ||
raise NotAuthenticated(detail=_('Refresh cookie not set. Try to authenticate first.')) | ||
else: | ||
request.data['refresh'] = token | ||
return request | ||
|
||
def set_auth_cookies(self, response, data): | ||
expires = self.get_refresh_token_expiration() | ||
response.set_cookie( | ||
api_settings.AUTH_COOKIE, data['access'], | ||
expires=expires, | ||
domain=api_settings.AUTH_COOKIE_DOMAIN, | ||
path=api_settings.AUTH_COOKIE_PATH, | ||
secure=api_settings.AUTH_COOKIE_SECURE or None, | ||
httponly=True, | ||
samesite=api_settings.AUTH_COOKIE_SAMESITE, | ||
) | ||
if 'refresh' in data: | ||
response.set_cookie( | ||
'{}_refresh'.format(api_settings.AUTH_COOKIE), data['refresh'], | ||
expires=expires, | ||
domain=None, | ||
path=reverse(self.token_refresh_view_name), | ||
secure=api_settings.AUTH_COOKIE_SECURE or None, | ||
httponly=True, | ||
samesite='Strict', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you set the samesite atribute to 'strict'? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what about the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here use domain.com for the front-end and api.domain.com for the backend. i rather have the cookie in domain.com There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here, I'd prefer this to not be hardcoded. Current implementation means backend I would prefer this: or at least a separate setting for the refresh token so it's configurable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
) | ||
return response | ||
|
||
def get_refresh_token_expiration(self): | ||
return datetime.now() + api_settings.REFRESH_TOKEN_LIFETIME | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Idk if L124 would be a problem then. |
||
|
||
|
||
class TokenObtainPairView(TokenCookieViewMixin, TokenViewBase): | ||
""" | ||
Takes a set of user credentials and returns an access and refresh JSON web | ||
token pair to prove the authentication of those credentials. | ||
|
@@ -42,18 +110,48 @@ class TokenObtainPairView(TokenViewBase): | |
token_obtain_pair = TokenObtainPairView.as_view() | ||
|
||
|
||
class TokenRefreshView(TokenViewBase): | ||
class TokenRefreshView(TokenCookieViewMixin, TokenRefreshViewBase): | ||
""" | ||
Takes a refresh type JSON web token and returns an access type JSON web | ||
token if the refresh token is valid. | ||
""" | ||
serializer_class = serializers.TokenRefreshSerializer | ||
|
||
def get_refresh_token_expiration(self): | ||
if api_settings.ROTATE_REFRESH_TOKENS: | ||
return super().get_refresh_token_expiration() | ||
token = RefreshToken(self.request.data['refresh']) | ||
return datetime.fromtimestamp(token.payload['exp']) | ||
|
||
|
||
token_refresh = TokenRefreshView.as_view() | ||
|
||
|
||
class TokenObtainSlidingView(TokenViewBase): | ||
class SlidingTokenCookieViewMixin: | ||
def extract_token_from_cookie(self, request): | ||
"""Extracts token from cookie and sets it in request.data as it would be sent by the user""" | ||
if not request.data: | ||
token = request.COOKIES.get(api_settings.AUTH_COOKIE) | ||
if not token: | ||
raise NotAuthenticated(detail=_('Refresh cookie not set. Try to authenticate first.')) | ||
else: | ||
request.data['token'] = token | ||
return request | ||
|
||
def set_auth_cookies(self, response, data): | ||
response.set_cookie( | ||
api_settings.AUTH_COOKIE, data['token'], | ||
expires=datetime.now() + api_settings.REFRESH_TOKEN_LIFETIME, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, should probably use |
||
domain=api_settings.AUTH_COOKIE_DOMAIN, | ||
path=api_settings.AUTH_COOKIE_PATH, | ||
secure=api_settings.AUTH_COOKIE_SECURE or None, | ||
httponly=True, | ||
samesite=api_settings.AUTH_COOKIE_SAMESITE, | ||
) | ||
return response | ||
|
||
|
||
class TokenObtainSlidingView(SlidingTokenCookieViewMixin, TokenViewBase): | ||
""" | ||
Takes a set of user credentials and returns a sliding JSON web token to | ||
prove the authentication of those credentials. | ||
|
@@ -64,7 +162,7 @@ class TokenObtainSlidingView(TokenViewBase): | |
token_obtain_sliding = TokenObtainSlidingView.as_view() | ||
|
||
|
||
class TokenRefreshSlidingView(TokenViewBase): | ||
class TokenRefreshSlidingView(SlidingTokenCookieViewMixin, TokenRefreshViewBase): | ||
""" | ||
Takes a sliding JSON web token and returns a new, refreshed version if the | ||
token's refresh period has not expired. | ||
|
@@ -84,3 +182,36 @@ class TokenVerifyView(TokenViewBase): | |
|
||
|
||
token_verify = TokenVerifyView.as_view() | ||
|
||
|
||
class TokenCookieDeleteView(APIView): | ||
""" | ||
Deletes httpOnly auth cookies. | ||
Used as logout view while using AUTH_COOKIE | ||
""" | ||
token_refresh_view_name = 'token_refresh' | ||
authentication_classes = () | ||
permission_classes = () | ||
|
||
def post(self, request): | ||
response = Response({}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it necessary to have an empty dict inside the response? Shouldn't it just be |
||
|
||
if api_settings.AUTH_COOKIE: | ||
self.delete_auth_cookies(response) | ||
|
||
return response | ||
|
||
def delete_auth_cookies(self, response): | ||
response.delete_cookie( | ||
api_settings.AUTH_COOKIE, | ||
domain=api_settings.AUTH_COOKIE_DOMAIN, | ||
path=api_settings.AUTH_COOKIE_PATH | ||
) | ||
response.delete_cookie( | ||
'{}_refresh'.format(api_settings.AUTH_COOKIE), | ||
domain=None, | ||
path=reverse(self.token_refresh_view_name), | ||
) | ||
|
||
NiyazNz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
token_delete = TokenCookieDeleteView.as_view() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll need to merge with master and move this documentation to where the rest of the documentation is now