City-of-Helsinki / kerrokantasi

Kerro kantasi participatory democracy backend
MIT License
7 stars 16 forks source link

Treat expired JWT token as an unregistered user #286

Open Rikuoja opened 6 years ago

Rikuoja commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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.

jussih commented 6 years ago

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