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

#270 - Add middleware to refresh access token #343

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Dejiah
Copy link

@Dejiah Dejiah commented Jul 2, 2024

Hi,

as mentioned in #270 , I think this would be a great addition to the library and the other PR ( #278 ) is stale, so I am trying to take this over the finish line.

I tried to address your comments as far as possible.

You mentioned in the review for the stale PR that you would not add the access token to the session "just for the sake" of it.
I kept this part of the code intentionally, because I would argue that having access to the access token within the request(-session) is an important feature.
I think the most important use case is to be able to use the access token for subsequent requests on behalf of the user to other services.

In our specific situation, we would like to use django-auth-adfs in a situation, where we have a Django Frontend that needs to perform the user sign-in (via Azure AD) and then request multiple downstream services on behalf of the user to acquire data to show. As all of these downstream services are in-turn OAuth2 protected, we need to send a valid access token with each request, which is only possible if we can access it outside of the AuthBackend e.g. via the session.

I also checked the solution proposed in #267 but I believe that extending the User model and thus saving the access token in the database leads to a security vulnerability as in this case anyone having access to the database could perform requests on behalf of every logged-in user.

Let's discuss and find a solution :-)

@tim-schilling
Copy link
Member

I kept this part of the code intentionally, because I would argue that having access to the access token within the request(-session) is an important feature. I think the most important use case is to be able to use the access token for subsequent requests on behalf of the user to other services.

If you didn't have the access token from here, how would you implement it otherwise in the application (not this library)? I'm not convinced that this package should handle anything other than authentication (not authorization or integration with Azure web services).

@Dejiah
Copy link
Author

Dejiah commented Jul 2, 2024

Thanks for you swift reply!

I kept this part of the code intentionally, because I would argue that having access to the access token within the request(-session) is an important feature. I think the most important use case is to be able to use the access token for subsequent requests on behalf of the user to other services.

If you didn't have the access token from here, how would you implement it otherwise in the application (not this library)? I'm not convinced that this package should handle anything other than authentication (not authorization or integration with Azure web services).

You would also have to carry out one of the OAuth2 authentication flows to retrieve an access token and thus authenticate the users, e.g. using the MSAL library (https://github.com/AzureAD/microsoft-authentication-library-for-python) in case of Azure AD. But this is exactly what you already do within the library and would mean duplicating the whole token exchange logic etc.

I think the point here is not yet about authorisation or integration yet as exposing the access token is simply the "source of truth" for authenticating the user. This is also what the library uses internally to derive the exposed user information.
However, in some cases (e.g. when one wants to forward the authentication to some other service) the exposed information, which you extract from the token, is not sufficient (e.g. because it is not signed and not verifiable by an external service).
Authorisation (i.e. if you are actually allowed to do something) and integrating with other services would still be out of scope for the library but would require to have this "pure" authentication available in some cases.

Merge in latest changes
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

Successfully merging this pull request may close these issues.

None yet

3 participants