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

Treat expired JWT token as an unregistered user #286

Open
Rikuoja opened this issue Dec 15, 2017 · 3 comments
Open

Treat expired JWT token as an unregistered user #286

Rikuoja opened this issue Dec 15, 2017 · 3 comments

Comments

@Rikuoja
Copy link
Contributor

Rikuoja commented Dec 15, 2017

Currently, if a user tries to use an endpoint with an expired token, the API always returns 401 even if the data would be available without a token.

While this is sensible given the assumption that the user really meant to use the token, it is problematic from the UI point of view, since all users with expired tokens will suddenly encounter API errors on all public pages, and they have no idea what is going on.

As long as we don't have silent autorenewal of the JWT token, the API should perhaps return the same response as to a request without a token, if the token is invalid?

@sanesane
Copy link

This might not be a good idea. What I can tell, all API endpoints don't seem to return 401 even if the token is expired. For example the URL /hearings/list makes API calls listed below, only first and last fails when using expired token and those are endpoints that will require authentication with permission class.

[18/Dec/2017 14:16:59] "GET /v1/users/211bedfe-c924-11e7-8d57-c2a5d78378ac/? HTTP/1.1" 401 35
[18/Dec/2017 14:16:59] "GET /v1/label/? HTTP/1.1" 200 2059
[18/Dec/2017 14:17:00] "GET /v1/label/?limit=50&offset=50 HTTP/1.1" 200 2144
[18/Dec/2017 14:17:00] "GET /v1/label/?limit=50&offset=100 HTTP/1.1" 200 893
[18/Dec/2017 14:17:00] "GET /v1/hearing/?limit=10&include=geojson&ordering=-created_at HTTP/1.1" 401 35

I would argue that the API works like it should, and at least the above endpoints shouldn't even return data to anonymous users so the UI would be more confused that before. Especially if the UI still thinks user is logged in (because it has the JWT token and no errors are returned from API?)

Could it be better that the UI purges the JWT token from API request if 401 with this error string is returned:

{detail: "Signature has expired."}
detail: "Signature has expired."

Or UI could automatically prompt user to login again.

However, if there is need to tinker with the authentication, here is the relevant place to do so:
Authentication class lives in helusers app, which in turn uses django_rest_framework_jwt. The relevant line is this: https://github.com/GetBlimp/django-rest-framework-jwt/blob/master/rest_framework_jwt/authentication.py#L36
Basically what we want is not to raise exception but to return None, like at L30, where the jwt_value is not available at all.

Question is should we implement this to helusers app instead kerrokantasi project or override the helusers authentication class in kerrokantasi project?

@Rikuoja
Copy link
Contributor Author

Rikuoja commented Jan 9, 2018

Hm. Here the problem is not the user endpoint, but the /v1/hearing endpoint. It does return most hearings to unauthenticated users, so it definitely does not require authentication.

Of course, if somebody passes an expired token, it would be much cleaner to let the UI know that is the case, but in this case even the user does not realize they still have the token; they probably have forgotten already that they were logged in.

So this is in a way a special case, because we want the same endpoint to return things to both authenticated and unauthenticated users. And that is the origin of the problem. I would fix this for the time being by overriding this in kerrokantasi specifically; that is because this is very much a service specific feature where the endpoint may return different data depending on the presence of authentication. I'm not sure if we want this to be the general behavior in all Helsinki services.

@Rikuoja Rikuoja added the ready label Jan 9, 2018
@jussih
Copy link

jussih commented Feb 23, 2018

Should the overridden authentication class be used specifically for the /v1/hearing endpoint or all endpoints?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants