Skip to content
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_AUTH_COOKIE_* settings paralleling django SESSION_COOKIE_* #29

Merged
merged 2 commits into from Jan 29, 2020

Conversation

nigoroll
Copy link

We add settings analogous to SESSION_COOKIE_* for the JWT cookie:

'JWT_AUTH_COOKIE_DOMAIN': None
'JWT_AUTH_COOKIE_PATH': None
'JWT_AUTH_COOKIE_SECURE': True
'JWT_AUTH_COOKIE_SAMESITE': 'Lax'

with the following differences to django:

  • The HttpOnly attribute remains hardcoded as True in order to avoid unintended access from client code with addition of the Domain attribute.

BREAKING CHANGES with this patch:

This changes the default Secure attribute from False (actually None as in not present in Set-Cookie) to True. Users wishing to use JWT cookies over http (as in no TLS) need to set JWT_AUTH_COOKIE_SECURE to False.

This change is intentional to follow common best common practice.

CHANGES:

Adds the default Samesite attribute Lax

@nigoroll
Copy link
Author

d-oh, the samesite argument to .HttpResponse.set_cookie was added with django 2.1
Do we need to be compatible?

@nigoroll
Copy link
Author

I have added support for Django < 2.1
Feel free to omit this commit if you are dropping support for earlier versions anyway

@codecov-io
Copy link

codecov-io commented Jan 23, 2020

Codecov Report

Merging #29 into master will decrease coverage by 0.69%.
The diff coverage is 84.61%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master     #29     +/-   ##
========================================
- Coverage     100%   99.3%   -0.7%     
========================================
  Files           8       8             
  Lines         281     289      +8     
  Branches       28      29      +1     
========================================
+ Hits          281     287      +6     
- Misses          0       1      +1     
- Partials        0       1      +1
Flag Coverage Δ
#codecov 99.3% <84.61%> (-0.7%) ⬇️
#dj111 99.3% <84.61%> (-0.7%) ⬇️
#dj20 99.3% <84.61%> (-0.7%) ⬇️
#dj21 100% <ø> (ø) ⬆️
#dj22 99.3% <84.61%> (-0.7%) ⬇️
#dj30 99.3% <84.61%> (-0.7%) ⬇️
#drf310 99.3% <84.61%> (-0.7%) ⬇️
#drf311 99.3% <84.61%> (-0.7%) ⬇️
#drf37 99.3% <84.61%> (-0.7%) ⬇️
#drf38 99.3% <84.61%> (-0.7%) ⬇️
#drf39 99.3% <84.61%> (-0.7%) ⬇️
#py27 99.3% <84.61%> (-0.7%) ⬇️
#py34 99.3% <84.61%> (-0.7%) ⬇️
#py35 99.3% <84.61%> (-0.7%) ⬇️
#py36 99.3% <84.61%> (-0.7%) ⬇️
#py37 99.3% <84.61%> (-0.7%) ⬇️
#py38 100% <ø> (ø) ⬆️
Impacted Files Coverage Δ
src/rest_framework_jwt/settings.py 100% <ø> (ø) ⬆️
src/rest_framework_jwt/views.py 100% <100%> (ø) ⬆️
src/rest_framework_jwt/compat.py 86.66% <80%> (-13.34%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ddbf51c...543108c. Read the comment docs.

Copy link
Collaborator

@fitodic fitodic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the pull request 👍

Everything is in order. I've left a few comments for us to discuss for the sake of maintainability.

And please add a changelog.

docs/index.md Outdated Show resolved Hide resolved
src/rest_framework_jwt/views.py Outdated Show resolved Hide resolved
tests/views/test_authentication.py Outdated Show resolved Hide resolved
@nigoroll
Copy link
Author

@fitodic thank you very much for your comprehensive and helpful review.
I have taken in most of your suggestions, commented on them in detail, force-pushed the PR branch and hope to have not missed anything else (sorry for the changelog oversight).
Other than that, please feel free to make any changes to these suggestions as you like.

Thank you again

Copy link
Collaborator

@fitodic fitodic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes. Everything is in order, apart from the two new questions regarding minor adjustments.

Once that's all cleared up, I'd be happy to merge it.

changelog.d/29.feature.md Show resolved Hide resolved
src/rest_framework_jwt/compat.py Outdated Show resolved Hide resolved
We add settings analogous to SESSION_COOKIE_* for the JWT cookie:

    'JWT_AUTH_COOKIE_DOMAIN': None
    'JWT_AUTH_COOKIE_PATH': '/'
    'JWT_AUTH_COOKIE_SECURE': True
    'JWT_AUTH_COOKIE_SAMESITE': 'Lax'

with the following differences to django:

* The HttpOnly attribute remains hardcoded as True in order to avoid
  unintended access from client code with addition of the Domain
  attribute.

These settings also apply to the recently added impersonation cookie.

BREAKING CHANGES with this patch:

This changes the default Secure attribute from False to True. Users
wishing to use JWT cookies over http (as in no TLS) need to set
JWT_AUTH_COOKIE_SECURE to False.

This change is intentional to follow common best common practice.

CHANGES:

Adds the default Samesite attribute 'Lax'
@nigoroll
Copy link
Author

force pushed

@fitodic fitodic merged commit 785bb62 into Styria-Digital:master Jan 29, 2020
@fitodic
Copy link
Collaborator

fitodic commented Jan 29, 2020

Thanks for the pull request and the changes. I'll create the new release shortly so you can start using these changes right away.

@nigoroll nigoroll deleted the cookie-settings branch January 29, 2020 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants