Jwt cookie bis #321
Jwt cookie bis #321
Conversation
… work at the same time when AUTH_COOKIE is enabled
…esh view name if required
Set `token_refresh_view_name` argument in `as_view` method of TokenObtainPairView and TokenCookieDeleteView in urlpatterns to change refresh token view name
Example:
```python
urlpatterns = [
path('api/token/', TokenObtainPairView.as_view(token_refresh_view_name='jwt_refresh'), name='token_obtain_pair'),
path('api/token/delete/', TokenCookieDeleteView.as_view(token_refresh_view_name='jwt_refresh'), name='token_delete'),
]
```
# where TokenRefreshView url name is `jwt_refresh`
|
@loicgasser Sorry I didn't look at this earlier. Didn't notice it was changed back from [WIP]. I'll take a look at this and also ask David to also look. Not sure if it'll be settled to not use httpOnly cookies based on the referenced PR/issue. Also updating master branch might be helpful (plus CI checks look fine besides the workflow one). |
| 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: |
There was a problem hiding this comment.
I think this is now handled by the new auth rules (we definitely need to update our changelog and documentation)...
|
Yeah, I did rebase it when I first opened it. I can rebase again. It seems that the workflow is broken in all pull requests. |
|
There still seems to be some merge conflicts @loicgasser And yeah, we fixed the initial problem, but when I was trying to fix it, I managed to duck something up so we're back at square one. But they should be working fine soon. Edit: you may just need to rebase again since we fixed the problem after your last commit. I wouldn't take too much time to continuously rebase this until we review it again (after not reviewing this thing for awhile). I think we might as well merge this PR considering it's been too long. If anything, if a CVE comes up, we'll resolve it. We can label this feature as experimental as well, but it might as well be published at this point. |
|
This is not a suggestion veiled as a question; I'm genuinely curious. Is there a good reason not to make authentication via cookie its own authentication class? So, just as I can choose one or more authentication schemes in DRF through |
|
Good question. To me, it's just to avoid duplicate code, so it's easier to maintain (especially with a couple of new features that I haven't had time to document).
I think the code is taking away the need for you to check, so you just don't need to worry about context switching (between mobile Auth header and browser cookies). Specifically: the |
|
Any reason why this still isn't merged? Happy to help and contribute as necessary, but I think this is a critical functionality to be merged knowing the fact that most DRF APIs are used with SPAs like React, Vue, and storing tokens in cookies is considered to be one of the safest methods. |
|
@AfrazHussain This is my preferred PR for JWT cookies. The issue at the moment is 1) resolving merge conflicts and 2) it's just huge. I've never taken the time to look thoroughly through everything. I'm skeptical of tests (I'm always skeptical of tests, so it has nothing specific to do with this PR). And finally, this just needs to be marked as super experimental. Even though people have been doing cookie stuff with SimpleJWT for awhile, I don't want to encourage people to use cookies with JWTs for their SPAs. But I digress. I just want a very very safe PR for the users, not devs, of Django SPA projects. |
|
Hi! Is there any way how we could support you in speeding up the reviewing process? I took me some while to find out what is the current state of the "Store JWT tokens in Cookies" discussion and finally I've landed here :) |
|
hi this needs to be updated to master and pass tests with updates docs. another swipe through and we can mark it as experimental with some dangers in its use. then it can be merged |
|
@Andrew-Chen-Wang SimpleJWT is currently implemented in a way which reflects the original concept of JWTs and goes against the best practice consistently recommended around the world. It pushes people to manage their tokens in the JS, which in turn requires them to store the tokens in long-term local storage. I understand your skepticism, but there's a consideration of Opportunity Cost at play here, too. You have 100+ reactions collectively in tickets and PRs about this specific functional gap. If you can point out specific tasks to complete in order for you to merge this PR, I am available to contribute some or all of them. |
This is the work I have done based on #157
I addressed all the comments in the pull requests, added some tests, rebased it to master, fixed the built, refactored the code and changed the approach to a "more secure" approach.
In my view, leaving the access and refresh token in the response body defies the purpose of putting the token in a
httpOnlycookie.The only way local storage can be compromised is if your application is already compromised with XSS, in which case it would be easy to call the server and retrieve both the access and refresh token.
Now, if an attacker was able to inject a script in your site, he would also be able to send a request to one of his server and could extract the access and refresh token. This is why if you don't use
SameSitewith valuesStrictorLax, this approach has no real impact on security, since it would only raise the difficulty for attackers to steal the credentials.Note that the
SameSitesetting is not available in older browsers like Internet Explorer which still account for around 6% of the global market.So I would argue that storing the tokens in a
httpOnlycookie is better, but only marginally better. In my case I decided to not support IE.Here is a demo that uses this branch with Angular:
SimpleJWT/drf-SimpleJWT-Angular#1
This pull request still needs doc. But has it a chance to be merged?