Store and manage lifecycle of access tokens beyond authentication #362

tnware · 2025-03-09T20:28:26Z

I was putting together a solution for my organization in attempt to streamline providing delegated user access to MS Graph APIs to our users who are already authenticated through django-auth-adfs. After some research into how I was going to do this, I realized that I was not alone in looking for this functionality from this package for one reason or another (#267, #270, #278, #343)

While reviewing the materials, I noticed a few reasons why this type of implementation was not added to the package yet, even as there was some interest from at least one of the maintainers.

I was still going to need this solution, so with all of that in mind I landed on a design that I would feel comfortable contributing to this project for consideration. While it's originally designed to solve the needs of my organization, I realize those needs are not unique, so I propose this Middleware as a solution in fulfillment of these needs while also addressing concerns of @tim-schilling in #343

✅ Move as much of the new changes to the middleware and out of the backend
✅ Only store the token information in the session if the refresh token middleware is being used (not sure the best way here)
✅ Prevent the library from storing tokens in the session if cookie based sessions are being used
✅ There needs to be some documentation

In spirit of keeping things contained to the middleware, all the related functionality has been implemented in a manager class singleton and the backend.py was only adjusted to call this during the authentication flow, providing the request in case we want to store tokens on their session, and safely determine if the middleware is enabled before trying to perform any operations surrounding this functionality.

Otherwise, if you don't enable the middleware in your application, nothing should be any different, and you won't be forced to have the tokens managed or stored on the sessions if you don't want that.

Sphinx documentation has been provided
Unit tests have been generated (in attempt) to maintain integrity, but maybe some more work could be done there

Open to discussion/feedback/critiques etc.
I understand if it's not in the interest of the maintainers to have the package operate in this scope.
Thank you for your time and consideration

* make middleware check more rigid * token integrity validation * don't be so strict on refresh tokens

* remove useless validation method * use actual expiration time from obo token

tim-schilling

This is a really big PR. I'm not entirely sure we want to accrue this much in the repository. However, I've read through the code and provided some initial thoughts. While most of my suggestions are to make things more extensible, I wonder if we'd be better off reducing the scope of this to something closer to #343 where it requires the end user to manage the tokens.

I didn't look at the tests or docs.

tim-schilling · 2025-03-11T01:09:34Z

django_auth_adfs/__init__.py

@@ -4,4 +4,4 @@
 Adding imports here will break setup.py
 """

-__version__ = '1.15.0'
+__version__ = "1.16.0"


This doesn't need to be incremented. We'll do that when we cut a release.

tim-schilling · 2025-03-11T01:13:49Z

django_auth_adfs/backend.py

+        # Store tokens in session if middleware is enabled
+        if request and adfs_response and hasattr(request, "token_storage"):
+            request.token_storage.store_tokens(request, access_token, adfs_response)


Would it be better to pull this out into a separate method that's called after process_access_token? It would make it easier for someone to customize what they want to do with the access token at that moment.

tim-schilling · 2025-03-11T01:21:16Z

docs/settings_ref.rst

+        "TOKEN_REFRESH_THRESHOLD": 600
+    }
+
+STORE_OBO_TOKEN


I don't understand the purpose of this setting. Seems like if a person is using TokenLifecycleMiddleware, this has to be True.

tim-schilling · 2025-03-11T01:21:39Z

docs/settings_ref.rst

+
+LOGOUT_ON_TOKEN_REFRESH_FAILURE
+-------------------------------
+* **Default**: ``False``


Given the security note below, this probably should be True.

tim-schilling · 2025-03-11T01:22:43Z

docs/settings_ref.rst

+   If you change this setting after tokens have been stored in sessions, those tokens will no longer be decryptable.
+   This effectively invalidates all existing tokens, requiring users to re-authenticate.
+   Consider this when deploying changes to the salt in production environments.


This is a good note. I think we may want to consider shipping with a fallback strategy or making it extensible so someone would write that themselves if needed.

tim-schilling · 2025-03-11T01:26:44Z

django_auth_adfs/token_manager.py

+                                logger.debug(
+                                    "Stored OBO token with expiration from token claims"
+                                )
+                except Exception as e:


There are several blank Exception catches. That's typically an anti-pattern. Is there a good reason to employ it in this file?

tim-schilling · 2025-03-11T01:27:34Z

django_auth_adfs/token_manager.py

+                    from django_auth_adfs.backend import AdfsBaseBackend
+
+                    backend = AdfsBaseBackend()
+                    obo_token = backend.get_obo_access_token(access_token)


This implies there's a circular dependency in the project. Can we refactor that out? Or did you have other thoughts on it?

tim-schilling · 2025-03-11T01:29:18Z

django_auth_adfs/token_manager.py

+                "reasons and cookie size limitations. Token storage will be disabled."
+            )
+
+    def store_tokens(self, request, access_token, adfs_response=None):


Please consider reworking this to allow for someone to override the significant aspects of this method. If you look at house the backend classes are implemented, you can get a sense of logic being broken up to allow extensibility.

tim-schilling · 2025-03-11T01:30:14Z

django_auth_adfs/token_manager.py

+            key = self._get_encryption_key()
+            f = Fernet(key)
+            encrypted_token = f.encrypt(token.encode())
+            return encrypted_token.decode()


Would we save ourselves some code (and tests) if we rely on Django's signing API?

tim-schilling · 2025-03-11T01:40:23Z

I forgot to say, thank you for the PR! I appreciate the time you've put into this and the thoroughness. The doc strings are well written and there's clearly focus given to documentation and tests. That's amazing to see!

tnware · 2025-03-11T02:00:17Z

I forgot to say, thank you for the PR! I appreciate the time you've put into this and the thoroughness. The doc strings are well written and there's clearly focus given to documentation and tests. That's amazing to see!

Thanks for all the feedback Tim. What started as a focused attempt to solve an immediate need quickly grew into something rather larger and more complex for sure.

I'm not eager to submit something incomplete, and with time and your feedback I see much room for improvement. I'm pleased that you have entertained the PR in such detail, thanks again for taking the time to review and comment.

I will think it over, right now it's covering a bit much for no reason other than to try and be flexible, which is probably its biggest downfall as the focus is not where it needs to be.

tim-schilling · 2025-03-11T12:06:46Z

I will think it over, right now it's covering a bit much for no reason other than to try and be flexible, which is probably its biggest downfall as the focus is not where it needs to be.

Would it make more sense to leave the renewal of the token to any libraries the developers may be using to interface with the GraphQL API? Or do it explicitly themselves?

tnware and others added 9 commits March 8, 2025 16:26

Token lifecycle management

eed1f4f

Optimize token middleware

d9b3018

TokenManager class (#3)

2a9f4e7

Cleanup implementation

63009a0

* make middleware check more rigid * token integrity validation * don't be so strict on refresh tokens

remove unused imports

d9214e3

fix flake8 feedback

285717e

cleanup (#5)

0d821b7

* remove useless validation method * use actual expiration time from obo token

actually middleware

7cdeb9b

docs update

a43b087

tim-schilling reviewed Mar 11, 2025

View reviewed changes

tnware closed this Mar 13, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Store and manage lifecycle of access tokens beyond authentication #362

Store and manage lifecycle of access tokens beyond authentication #362

Uh oh!

tnware commented Mar 9, 2025

Uh oh!

tim-schilling left a comment

Uh oh!

tim-schilling Mar 11, 2025

Uh oh!

tim-schilling Mar 11, 2025

Uh oh!

tim-schilling Mar 11, 2025

Uh oh!

tim-schilling Mar 11, 2025

Uh oh!

tim-schilling Mar 11, 2025

Uh oh!

tim-schilling Mar 11, 2025

Uh oh!

tim-schilling Mar 11, 2025

Uh oh!

tim-schilling Mar 11, 2025

Uh oh!

tim-schilling Mar 11, 2025

Uh oh!

tim-schilling commented Mar 11, 2025

Uh oh!

tnware commented Mar 11, 2025

Uh oh!

tim-schilling commented Mar 11, 2025

Uh oh!

Uh oh!

Uh oh!

Store and manage lifecycle of access tokens beyond authentication #362

Store and manage lifecycle of access tokens beyond authentication #362

Uh oh!

Conversation

tnware commented Mar 9, 2025

Uh oh!

tim-schilling left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tim-schilling commented Mar 11, 2025

Uh oh!

tnware commented Mar 11, 2025

Uh oh!

tim-schilling commented Mar 11, 2025

Uh oh!

Uh oh!