Users continue to be authenticated past access token expiry

snok / django-auth-adfs

A Django authentication backend for Microsoft ADFS and AzureAD

http://django-auth-adfs.readthedocs.io/

BSD 2-Clause "Simplified" License

272 stars 99 forks source link

Users continue to be authenticated past access token expiry #270

Open daggaz opened 1 year ago

daggaz commented 1 year ago

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?

JonasKs commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Any further thoughts on this issue?

tim-schilling commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

Lemme know what you think.

Tests will follow.

daggaz commented 1 year ago

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

daggaz commented 1 year ago

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 commented 3 months ago

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 commented 3 months ago

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