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

Users continue to be authenticated past access token expiry #270

Open
daggaz opened this issue Jan 20, 2023 · 13 comments
Open

Users continue to be authenticated past access token expiry #270

daggaz opened this issue Jan 20, 2023 · 13 comments

Comments

@daggaz
Copy link

daggaz commented Jan 20, 2023

I don't know if this is expected behaviour, but it seems wrong.

If you authenticate a user using this library, the user will retain a logged in session indefinitely (depending on your django session settings).

Should the user not be automatically logged out when the access token expires?

Fund with Polar
@JonasKs
Copy link
Member

JonasKs commented Jan 20, 2023

How long the token is valid for and how you decide to configure your sessions is in my opinion two different things in an MVC app. In restful (DRF) APIs, the token is validated on each request, but this is not true for sessions.

@daggaz
Copy link
Author

daggaz commented Jan 20, 2023

I can see an argument for that position, and you are right, in the DRF each request is authenticated.

However, if you're coming from the point of view of knowing that access tokens expire, it is somewhat surprising that authenticating with a JWT doesn't expire.

I wonder if there shouldn't be a middleware that's checking expiry and using refresh tokens to update the access token?

@daggaz
Copy link
Author

daggaz commented Jan 20, 2023

Alternatively, a middleware that redirects you back through login at/near the expiry.

Presumably either would be optional addons so that we preserve the current behaviour for existing installations.

@JonasKs
Copy link
Member

JonasKs commented Jan 25, 2023

I don't see why this has to be implemented? Set session time out to the same time as expiration of the token and everything is solved?

@daggaz
Copy link
Author

daggaz commented Jan 26, 2023

Typical access token expiry is very short. I think the default is 1 hour for ADFS.
This is what the refresh token is for, so you can transparently refresh your access token.
This also means that any claims/permissions/roles/whatever in the access token get automatically refreshed.

@daggaz
Copy link
Author

daggaz commented Feb 22, 2023

Any further thoughts on this issue?

@tim-schilling
Copy link
Member

Typical access token expiry is very short. I think the default is 1 hour for ADFS.
This is what the refresh token is for, so you can transparently refresh your access token.
This also means that any claims/permissions/roles/whatever in the access token get automatically refreshed.

@daggaz I'm still a bit confused here. Why does any of this relate to a session? I could see us benefiting from a docs change to let a user know to change their session timeout to slightly exceed their refresh token period, but I don't see the need for us to change something as I currently understand it.

@JonasKs
Copy link
Member

JonasKs commented Feb 24, 2023

So, there's these benefits:

  • tokens are renewed, and groups therefor renewed, without the user having to log out and in again
  • the user won't be redirected to ADFS once an hour (depending on company settings) if session is set to match token expiry
  • sessions can match the token.

And on the other side:

  • If you're signed in to ADFS, the redirect don't require you to log in again.
  • You can just set the session to be as long as you'd like, but this would obviously mean the user has to log out and in to refresh groups, and won't match the expiry of the token.

How ever, normally, these things are handled by the frontend asynchronous in the background. I'm not sure how to implement this beautifully. You're suggesting if a request goes through a session, check the token, if expiring, refresh it?

@daggaz
Copy link
Author

daggaz commented Feb 28, 2023

I've added a PR with something I think works.

Lemme know what you think.

Tests will follow.

@daggaz
Copy link
Author

daggaz commented Mar 1, 2023

Updated PR with complete set of tests, and fixed up a few other things I found along the way.

@daggaz
Copy link
Author

daggaz commented Mar 1, 2023

Not updated docs. I'm not sure if you want to make this an optional addon, or add it to the standard setup instructions.

One thing to point out, is that if you are using the LoginRequiredMiddleware, the new adfs_refresh_middleware should go first. This is so that when the refresh token eventually expires (not the access token), and you are logged out, your will be correctly redirected to login.

@Dejiah
Copy link

Dejiah commented Jul 2, 2024

Hi,

thanks for creating the library, looks really good!
However, the missing refresh token is a dealbreaker for using it right now because we require up-to-date access tokens to query a downstream service.

I saw that the PR is stale since a year. Any changes that this will get merged soon?

I would also be happy to contribute and fix remaining issues if any are open.

@tim-schilling
Copy link
Member

I imagine you could take over the PR and implement the changes suggested by @JonasKs and myself.

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

No branches or pull requests

4 participants