Make JWTRefreshMiddleware extensible via BaseAuthManager.get_jwt_refresh_middleware() - #70783
Make JWTRefreshMiddleware extensible via BaseAuthManager.get_jwt_refresh_middleware()#70783stephen-bracken wants to merge 1 commit into
JWTRefreshMiddleware extensible via BaseAuthManager.get_jwt_refresh_middleware()#70783Conversation
0fb0bdd to
9502fe9
Compare
JWTRefreshMiddleware extensible via BaseAuthManager.get_jwt_refresh_middleware()`
JWTRefreshMiddleware extensible via BaseAuthManager.get_jwt_refresh_middleware()`JWTRefreshMiddleware extensible via BaseAuthManager.get_jwt_refresh_middleware()
474ce7b to
7655032
Compare
16198fd to
3a89840
Compare
|
CI is failing |
a8c2ae1 to
3dc4d12
Compare
|
I've updated edit: updated |
87b198d to
908b21e
Compare
908b21e to
bfa77e7
Compare
e94f3ba to
a9bffa6
Compare
pierrejeambrun
left a comment
There was a problem hiding this comment.
I would keep the PR focused around the target issue.
That's also updating/refactoring a whole bunch, making the PR harder to review and surface to test bigger.
- Moves generate_jwt from before call_next to inside _set_new_token after call_next — introduces the fail-hard-vs-fail-soft semantic
- Reorganizes the outer try scope so the "if current_token" gate lives inside _refresh_user — makes the flow harder to read for anyone tracing "when is _refresh_user called".
- Changes the outer condition from if new_token is not None to if new_user or new_token is not None. Under a minimal refactor, the original condition
still works. - Introduces the if new_user: else new_token = "" dead branch inside _set_new_token — dead because dispatch never calls it with new_user=None, but present because the extraction was over-scoped.
- Swaps delete_cookie for set_cookie(max_age=0) — drops the expires=0 attribute. Small, but again: not needed for extensibility.
- @classmethod async def on _set_new_token despite using neither cls nor await.
| previous = flask_app.config.get("AUTH_ROLE_PUBLIC") | ||
| flask_app.config["AUTH_ROLE_PUBLIC"] = None | ||
| base_middleware = [] | ||
| if AIRFLOW_V_3_4_PLUS: |
There was a problem hiding this comment.
Why AIRFLOW_V_3_4_PLUS here?
There was a problem hiding this comment.
The FabAuthManager change is for compatibility. I am assuming that this change will be released in 3.4.0 (as that is what airflow.__version__ is set to on main), which will add the changes to BaseAuthManager.get_fastapi_middlewares() and the _get_jwt_refresh_middleware() method - can correct the guard as necessary.
670f2da to
28c2f77
Compare
When
Restored the
In the new flow,
Removed dead branch
Restored
Swapped to |
6e99a99 to
d39ba4f
Compare
d39ba4f to
5d184f0
Compare
closes: #70720
Make JWTRefreshMiddleware extensible via BaseAuthManager.get_jwt_refresh_middleware()
By moving the
JWTRefreshMiddlewareinitialisation toBaseAuthManager.get_jwt_refresh_middleware(), this allows auth managers to override the JWTRefreshMiddleware behaviour by inheriting from it and using the inherited class in get_fastapi_middlewares instead.By factoring out the
_set_new_token()method fromdispatch(), this gives an interface for the inherited token refresh middleware to alter the behaviour of the middleware when setting the tokens. The interface has access to the response object to set any cookies as needed.Changed the
_refresh_user()method to accept arequestobject to allow accessing any cookies or state information from the request.Also adds the
airflow.api_fastapi.app.request_cookie_is_secure()helper to standardise setting HTTP secure cookiesWas generative AI tooling used to co-author this PR?