Demo #1

loicgasser · 2020-07-13T03:31:46Z

just to be sure we're on the same page.

Here is a test a application that uses this fork (a enhanced version of the original pull request):
jazzband/djangorestframework-simplejwt@master...AtuzSolution:jwt_cookie_bis

So it doesn't store any sensitive information in local storage and relies on an initial refresh token call to determine if the user is authenticated.

Additionally, when we store the jwt token in a http only cookie, I made the obtain token view return 3 new properties access_expiry, refresh_expiry and csrf_token. I kept access and refresh even though we don't need or use them since they are in the cookie, I would remove them if it was up to me but I am not sure what's your take on this.

The same happens to the response in the refresh_view.

I kept it as simple as possible, there 2 are endpoints to test the authentication, one GET and one POST so that you can test unsafe requests.

You can try opening multiple tabs, signing out, closing a tab and re-opening and it works pretty good. Let me know if you find something fishy.

Just wanted to make sure we are on the same page before I go further with this.

Additional unrelated note:
I think you may want to create only one repository for all the web integrations and add one folder per techno like you did for the mobile part.

Andrew-Chen-Wang · 2020-07-13T03:56:03Z

Hi @loicgasser,

Thanks for the addition! I'll do an in-depth review of the PR within this week; I will also take a look at your fork for the PR in SimpleJWT more closely. (Quick note: is there a reason to use get_user_model() multiple times in tests? I assume the user model will stay the same through tests...; additionally, module level naming of User = get_user_model() will run the function once whereas constantly using get_user_model() would consume unnecessary resources often.

What you've described for the action sounds great, especially as a model for the other repos!

Regarding combining the repositories, I considered it, but, if you didn't notice, I made these repositories template repositories so that people can easily use these as templates for their next Django project. I want these repositories to be standard for ANY Django project using SimpleJWT so that security vulnerabilities are kept at a minimum.

Currently multitasking (i.e. watching movie and coding), so expect a quick review within the next 15 minutes I suppose.

Thanks again for the contribution!

Andrew-Chen-Wang

Again, thanks for this huge contribution! I'm not a JS framework person, so you'll have to bear with me on this PR. Hopefully someone else with more expertise can chime in.

One of my biggest concerns was the scheduleRefresh. If, for some reason, your user presses a button but the scheduled refresh was still in progress, wouldn't you be signed out because you got a 401 error? That's the point of an authenticator interceptor. If you get a 401 error, you refresh. A second 401 error would be a sign out.

Edit: I went from the bottom up. The crucial review is at the bottom.

Last edit before I head off: I didn't test any of these suggestions or your PR in general. I don't have Angular installed, so you'll just have to bear with me for now until I get the change to manually test this. Thanks again!

Andrew-Chen-Wang · 2020-07-13T04:10:43Z

server/README.md

@@ -8,7 +8,7 @@ If the refresh token expires (after 1 day for security reasons), the client need

 ### Running the server

-1. Create a virtual environment and install the packages: `virtualenv venv && source venv/bin/activate && pip install  -r requirements.txt`.
+1. Create a virtual environment and install the packages: `python3 -m venv venv && source venv/bin/activate && pip install -U pip && pip install  -r requirements.txt`.


Not very typical for people to use python3. Virtualenv Python2 isn't typically used with Windows and OS X users. Even for Linux, I can't recall having a virtualenvwrapper for Python 2. I think it's pretty standard to just say virtualenv, especially with Python's new addition for making a faster virtual environment with CPython.

tl;dr Don't change.

I would argue that python 2 is not maintained anymore since January 1 2020. Django 3 doesn't support python 2 anymore and it seems to me that it is now more typical for people to use python 3 with Django, but I can be wrong.
I'll leave it if you prefer.

Sorry, I need to work on my communication skills and generally not confusing people:

Not very typical for people to use python3

We're totally on the same page when you say:

I would argue that python 2

I meant not very typical for people to create a virtualenv with python3 -m venv venv.

Just keep it simple. I'd much rather you regressed this change.

Andrew-Chen-Wang · 2020-07-13T04:11:28Z

server/public/views.py

@@ -19,3 +20,6 @@ def list(self, request, *args, **kwargs):
            data={"id": request.GET.get("id")},
            status=HTTP_200_OK
        )
+
+    def create(self, request, *args, **kwargs):
+        return Response(status=HTTP_200_OK)


Typically status code 201.

yes typically, but since we don't create anything..

Haha true. I think just for "best practices" sake, write a comment saying "Create your obj" above the return and then still change the status code to 201.

It might also be worth it, for best practices sake, to simply import all status:

from rest_framework import status

And then everything else would be status.HTTP_200_OK and status.HTTP_201_CREATED. Thanks!

Andrew-Chen-Wang · 2020-07-13T04:14:12Z

server/server/settings.py

+# CSRF settings
+# SIMPLE_JWT settings counterparts take CSRF settings per default
+CSRF_COOKIE_NAME = 'CSRF'
+
+CSRF_COOKIE_HTTPONLY = True
+
+CSRF_COOKIE_SECURE = False
+
+CSRF_COOKIE_SAMESITE = None
+
+CSRF_COOKIE_DOMAIN = 'localhost'
+
+# We use a very small timedelta for demonstration purpose
+CSRF_COOKIE_AGE = timedelta(seconds=30).total_seconds()  # same as jwt refresh


We're any of these changed (besides CSRF_COOKIE_AGE) from their defaults? If not. do you mind clumping together the names and then writing a new comment saying something along the lines of the following are the defaults?

Yes because they take the same defaults as their JWT counter part. This was part of your review in the original pull request.

Mm, I meant writing a single comment between L56-57 saying "The following are the default CSRF settings, too" so people don't need to go to the Django settings doc page to figure out what changed. But you are correct; my review of that PR stated that the defaults for the SimpleJWT settings should be the same as the CSRF settings.

Andrew-Chen-Wang · 2020-07-13T04:17:33Z

jwt-angular/src/app/ping.service.ts

+  constructor(private apiService: ApiService) { }
+
+  get(): Observable<any> {
+    return this.apiService.get('/ping/');


Isn't the Ping views trying to return some serialized data by inputting a query parameter in the url?

Edit: you should specify the query parameter ?id=123 so that users can understand data transfers.

Thanks!

Sorry for asking for too much! I'm pretty much assuming a lot of people want some kind of template repository meaning they want to see the methods of network handling and etc.

Sooo if you don't mind: once you get id back from the server, try showing it in your Angular HTML code. This isn't needed for the POST request, just GET.

Andrew-Chen-Wang · 2020-07-13T04:18:07Z

jwt-angular/src/app/ping.service.ts

+  }
+
+  post(): Observable<any> {
+    return this.apiService.post('/ping/', {});


I feel like if you're demonstrating a method of uploading JSON, you might as well upload some JSON, even if it is random / hard coded.

I believe this is a valid JSON, but I can add something more fancy.

Yes, that's what I meant. A form that shows how to collect user input. Thanks!

sure this is easy

Andrew-Chen-Wang · 2020-07-13T04:24:16Z

jwt-angular/src/app/auth.service.ts

+  isAuthenticated(): boolean {
+    if (!this.hasAuthData()) {
+      return false;
+    }
+    const now = moment();
+    const accessExpiry = moment(this.accessExpiry);
+    return now.isBefore(accessExpiry);
+  }
+
+  canRefresh(): boolean {
+    if (!this.hasAuthData()) {
+      return false;
+    }
+    const now = moment();
+    const refreshExpiry = moment(this.refreshExpiry);
+    return now.isBefore(refreshExpiry);
+  }


Are you trying to save the time so that you know when to refresh? Typically for iOS and Android, if you get a 401, then you just refresh the token and try again. Is that atypical for JS frameworks?

It's probably worth noting that your refresh token should last maybe... a day or 12 hours on HS384 in settings.py.

Again, not very good at reading any of this since I don't work with JS... like at all besides one PR that was mostly copy pasta test configs.

Yes, I am trying to refresh before we hit the 401. I thought this could be a nice addition.

I think the main difference with mobile is that you can have several sessions (or tabs) for the same user.
As I mentioned we already always perform a refresh token when the application is loaded.
This means that for every page load I would need to hit 2 401 responses before I can be sure that my user isn't authenticated. I guess I was trying to avoid this.

https://hasura.io/blog/best-practices-of-using-jwt-with-graphql/ (similar approach)

I think the main difference with mobile is that you can have several sessions (or tabs) for the same user.

Not really a difference but more of a similarity. It's the same as a web browser in that you're saving the tokens once. I guess the only difference is that we also save their username + password in a keychain so we can always log the user in whenever he/she opens the app (persistence).

If you get a 401, you refresh. You might be thinking "several sessions" with multiple devices; you must remember JWT is state less authentication.

This means that for every page load I would need to hit 2 401 responses before I can be sure that my user isn't authenticated.

Sorry I didn't catch that from the beginning, but that's good that it's a 2 hit 401 response mechanism :)

Don't you think that refreshing the token every new page is a little inefficient? It seems much better to have that scheduled refresh expand to be 10 seconds (accounting for poor network conn) before expiration rather than one second so you can avoid two HTTP calls, which starts to amount costs quite a bit for people hosting on EC2 (data transfer out) or Heroku.

The two things that an application would have would be:

the first 401 response would require a token refresh. If that returns 401, then logout

scheduled refreshing of the token.

It's a lot of wasted network resources (and honestly not needed) to continuously call refresh-token every new page, imo.

Don't you think that refreshing the token every new page is a little inefficient?

The problem is that there is no way to tell if the user is authenticated when you are in a new page and bear in mind that we have to add the CSRF token in the header of every HTTP request. CSRF token we don't have access to because it is in a httpOnly cookie. So unless you want me to store the CSRF token in localStorage, there is really no way I would be able to do this differently with the current setup (without adding any further endpoints).

Now I think that I could try to implement the 2 hits mechanism in the interceptor. It isn't the case at the moment. The code will be a little tricky to understand for Angular and rxjs beginners but I could totally implement such an approach.
The question is then do we use

scheduled refresh

2 hits 401

or a combination of both

It seems much better to have that scheduled refresh expand to be 10 second.

yes but then we need to have access tokens that are valid for more than 10 seconds. 😆 The default one was even shorter, 5 seconds! I thought you wanted to keep it small for demonstration purposes.

I'd still prefer the 2 hits 401 method. That way, you won't need to store the CSRF token in the local storage, right?

yes but then we need to have access tokens that are valid for more than 10 seconds. 😆 The default one was even shorter, 5 seconds! I thought you wanted to keep it small for demonstration purposes.

Haha true. The reason I kept it at 5 seconds was because I used the "2 hits 401" method for my mobile application. Using scheduled refreshes would've consumed too much computational resources for a simple background timer on both Android and iOS.

Extending it to 10 seconds would be practical for those in poor network connections for scheduled refresh method. I'd recommend changing it to 10 seconds, BUUUUT:

I believe the 2 hits 401 is the best method and should be the only method. Take a look at my implementation for iOS:

https://github.com/Andrew-Chen-Wang/mobile-auth-example/blob/c51df854d8fa8bf9e15e5f162507aeed32bde998/jwt-ios/jwt-ios/Networking/Service/Router.swift#L27-L48

The rundown is as follows: if a random endpoint, for example "Ping" returned a 401 error, since we already know the user must be logged in to have even gotten "visually seen" the Ping endpoint, refresh the token. You can see the refreshing process in L29 in which I immediately refresh the token. If it's a success, then I retry the initial request; if it failed, I would have to use the saved username + password to get both tokens again.

For this repository, however, you won't be getting to that second portion (i.e. the "getting both tokens again" part). Instead, that would be a simple log out of the current user in the browser. This is because the developer shouldn't be saving the user's password in cookies. Thus I imparted the MITM attack in a different comment. If a thief took the refresh token, due to its stateless nature, it should expire, log out the thief, and the least amount damage could've been done.

In other words, I suggest the 2 hits 401. It's not bad to have the scheduled refresh thing, and you could possibly still include it in perhaps a second directory demonstrating both ways (i.e. scheduled refresh and 2 hits 401), but keep in mind the costs of constantly having to refresh a token, especially if a user, especially someone like my dad, just leaves the browser open forever. Unnecessary traffic; unnecessary costs, computationally and economically.

The code will be a little tricky to understand for Angular and rxjs beginners

When I was looking back at my Android code, I couldn't understand half of it; too complex. That's why I showed the Swift one lol. That's also why I liked the idea of setting up template repositories (even for myself with the mobile-auth-example; you don't need to understand it, just use it and you'll be mostly fine.

I'd still prefer the 2 hits 401 method. That way, you won't need to store the CSRF token in the local storage, right?

No. 2 reasons:

all unsecure requests will fail because you cannot insert the CSRF token in the HTTP header, you don't have access to it with javascript because it's in a httpOnly cookie.

the ping would work because of the cookies, and I guess you could set a boolean to tell the user is authenticated, but it wouldn't return the CSRF token. So you would still need to get it in some way. The only way you can share data between pages in a browser is via a Cookie not http only or localStorage.

keep in mind the costs of constantly having to refresh a token, especially if a user, especially someone like my dad, just leaves the browser open forever.

Yes but there is a very simple way to tell if page/tab is active (visibility API) or not. So we could easily block the referesh when the page is not active and resschedule when the user comes back to it.

I think that you are right in general, we need the 2 hits, just because it would mitigate poor connectivity issues, but it's not a golden ticket either.

In short scheduled, in good conditions would reduce round trips to the server and 2 hits would increase robustness of the system.

Note that in you have a webapp where users have multiple tabs open all the time, scheduling becomes inefficient.

Andrew-Chen-Wang · 2020-07-13T04:25:53Z

jwt-angular/src/app/http-error.interceptor.ts

+
+  intercept(request: HttpRequest<any>, next: HttpHandler): Observable<HttpEvent<any>> {
+    return next.handle(request).pipe(catchError((err: HttpErrorResponse) => {
+      if (err.status === 401) {


This err.status == 401 is pretty crucial for people to know. If you don't mind, in the README, mention to never code a 401 status code, especially for a case like this: 401 unauthenticated vs. 403 forbidden are different use cases.

Thanks!

Yes I have not added any doc yet, but I will mention it for sure.

jwt-angular/src/app/auth.service.ts

Andrew-Chen-Wang · 2020-07-13T04:30:44Z

jwt-angular/src/app/auth.service.ts

+    return this.apiService.post(`/token/both/`, data)
+      .pipe(tap(response => {
+        this.setAuth(response);
+        this.scheduleRefresh();


I mentioned this when reviewing, but shouldn't this scheduling thing be done asynchronously? IIRC when programming for aws-actions, I believe the typical GitHub action will run async something method. I'm not sure how threading works on browsers, but I feel like this is a bit of a concern.

I don't have Angular installed, so question: does this block the user from doing anything? What happens if the user presses a button, but the token was expired and the scheduled refresh hasn't completed? Wouldn't the user be signed out then?

If, for some reason, your user presses a button but the scheduled refresh was still in progress, wouldn't you be signed out because you got a 401 error? That's the point of an authenticator interceptor. If you get a 401 error, you refresh. A second 401 error would be a sign out.

So I have implemented a subscription cancellation via cancelRefresh which is called in purgeAuth and just before I create a new subscription (in case the previous one was already running).

You mention signing the user out at the second 401 request you would get in a row. I think this makes sense and I can bake in something like this.

Yes, that'd be helpful.

Just to clarify, this is because JWT intends to deter those who capture a token in an insecure network (i.e. server using HTTP instead of HTTPS).

Yes 2 reasons, avoid generating unnecessary traffic to your back-end and the one you mention above (which I didn't think about to be honest 😸 )

Andrew-Chen-Wang · 2020-07-13T04:38:10Z

@davesque Do you mind enabling Travis-CI.com or pro version of Travis or just Travis in general? It'd be helpful to run these tests. Thanks!

loicgasser · 2020-07-13T16:26:59Z

I'll make all these changes over the next weekend as I do this on my free time, like you probably do.
Thank you for your valuable inputs. It's going to take a while to get all the tests and doc right but I think it's worth the effort.

loicgasser · 2020-07-13T17:09:25Z

Oh forgot to ask you again, but are you OK removing the access and refresh token from the response when we use them in a cookie?
The reasoning behind this is that if your application is vulnerabe to XSS attacks and the attacker knows your refresh endpoint he could easily get the tokens from response body. In this case I think he'd be safer to simply not return them since we don't use them.

Andrew-Chen-Wang · 2020-07-15T15:29:33Z

Sure @loicgasser, you can comment them out, but I wouldn't remove them entirely. Just explain why they're commented out.

Andrew-Chen-Wang · 2020-10-16T18:55:56Z

It's been such a long time since I viewed this, but is this PR ready? Just a reminder I don't develop with JS frontend frameworks, so I'm unable to test this. I'll commit my two CI files on GitHub action tomorrow (so that we can make sure everything is ready CI wise; if you don't mind updating this branch with master soon), but I'd like to make sure I'm not missing anything @loicgasser

loicgasser · 2020-10-17T00:25:36Z

Hey 👋! Yes I need to get to it. Let me update it. I have had a lot of work lately 😩. I couldn't do as much as I wanted to. But I do have a PR for the backend and I still need to update this pull request. I promise I will finish it within the next 5 days

Andrew-Chen-Wang · 2020-10-17T00:43:08Z

No worries; there's really no rush, I was simply curious. I've updated the master branch with the Node CI in this commit: e7477fb Take your time!

loicgasser · 2020-10-18T20:06:44Z

Ok I updated it. Now, I need to rebase my work on httpOnly cookie and open a proper pull request for the python part.

loicgasser force-pushed the demo branch 2 times, most recently from 42f4388 to eee685a Compare July 13, 2020 04:32

Andrew-Chen-Wang requested changes Jul 13, 2020

View reviewed changes

Andrew-Chen-Wang added enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed labels Jul 13, 2020

loicgasser force-pushed the demo branch from eee685a to 5da2cdc Compare October 18, 2020 18:36

loicgasser changed the title ~~[WP] Demo~~ Demo Oct 18, 2020

loicgasser force-pushed the demo branch from 5da2cdc to 189fd95 Compare October 18, 2020 18:37

Add JWT demo for angular

8e18425

loicgasser force-pushed the demo branch from 189fd95 to 8e18425 Compare October 19, 2020 15:32

loicgasser mentioned this pull request Oct 19, 2020

Jwt cookie bis jazzband/djangorestframework-simplejwt#321

Open

Demo #1

Are you sure you want to change the base?

Demo #1

Conversation

loicgasser commented Jul 13, 2020 • edited Loading

Andrew-Chen-Wang commented Jul 13, 2020

Andrew-Chen-Wang left a comment • edited Loading

Choose a reason for hiding this comment

Andrew-Chen-Wang Jul 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andrew-Chen-Wang Jul 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andrew-Chen-Wang Jul 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loicgasser Jul 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loicgasser Jul 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andrew-Chen-Wang commented Jul 13, 2020

loicgasser commented Jul 13, 2020

loicgasser commented Jul 13, 2020

Andrew-Chen-Wang commented Jul 15, 2020

Andrew-Chen-Wang commented Oct 16, 2020 • edited Loading

loicgasser commented Oct 17, 2020

Andrew-Chen-Wang commented Oct 17, 2020

loicgasser commented Oct 18, 2020

loicgasser commented Jul 13, 2020 •

edited

Loading

Andrew-Chen-Wang left a comment •

edited

Loading

Andrew-Chen-Wang Jul 13, 2020 •

edited

Loading

Andrew-Chen-Wang Jul 13, 2020 •

edited

Loading

Andrew-Chen-Wang Jul 13, 2020 •

edited

Loading

loicgasser Jul 13, 2020 •

edited

Loading

loicgasser Jul 13, 2020 •

edited

Loading

Andrew-Chen-Wang commented Oct 16, 2020 •

edited

Loading