TheoLauw opened a new pull request, #73134:
URL: https://github.com/apache/airflow/pull/73134
Fixes #73133 (opened alongside this PR)
## What
`FabAuthManager.get_url_login()` accepted `**kwargs` but never used them, so
callers passing `next_url=request.url` (to redirect the user back to their
originally requested page after login) had that value silently discarded. Every
unauthenticated/expired-session redirect to `/auth/login/` therefore lost the
deep-link target, and the user landed on the homepage after logging in.
This is the same class of bug fixed for `SimpleAuthManager` in #67476 /
#67483 / #67965, but that fix was explicitly scoped to `SimpleAuthManager`
only. `FabAuthManager` (used for any FAB-backed auth: DB, LDAP, OAuth/SSO,
etc.) had the identical bug and was left unpatched.
## Why only one change is needed here (unlike the Simple auth manager fix)
`SimpleAuthManager` needed two PRs: one to propagate `next_url` into the
login URL (#67965) and one to make the login route actually consume `next` on
completion (#67483), because its login route is a bespoke FastAPI
implementation with no built-in notion of "return to this page."
`FabAuthManager` doesn't need the second half: it delegates to
`flask_appbuilder`'s own `AuthOAuthView`/`AuthDBView`, whose `login()` view
already reads `next` from the query string and encodes it into the OAuth
`state` (or session, for non-OAuth backends), and whose
`redirect`/`get_safe_redirect` are already monkey-patched by
`providers/fab/src/airflow/providers/fab/www/extensions/init_appbuilder.py` to
Airflow's own versions
(`providers/fab/src/airflow/providers/fab/www/views.py`), which set the
`_token` JWT cookie and forward to that URL. I traced this end-to-end and
confirmed it already works correctly — the only missing link was `next_url`
never making it into the initial `/auth/login/` URL in the first place.
So this PR only needs to touch `get_url_login()`, mirroring the exact
propagation fix from #67965:
```python
def get_url_login(self, **kwargs) -> str:
"""Return the login page url."""
login_url = urljoin(self.apiserver_endpoint,
f"{AUTH_MANAGER_FASTAPI_APP_PREFIX}/login/")
next_url = kwargs.get("next_url")
if next_url:
return f"{login_url}?{urlencode({'next': next_url})}"
return login_url
```
## Tests
Added `test_get_url_login_with_next_url` and
`test_get_url_login_without_next_url_kwarg` to `TestFabAuthManager`, mirroring
the tests added for `SimpleAuthManager` in #67965.
Verified manually against the released `apache-airflow-providers-fab==3.2.0`
package (not just against `main`) by patching the installed `get_url_login` and
exercising it directly:
- no `next_url` kwarg → unchanged bare login URL (backward compatible)
- `next_url` provided → correctly appended as a url-encoded `next` query
param
- `next_url=None` → falls back to the bare URL
- unrelated/unknown kwargs → ignored, no crash
- `next_url` containing spaces/`&`/`?` → properly percent-encoded, not
passed through raw
## How I found this
Root-caused while investigating a real-world report from our Airflow 3
deployment (`FabAuthManager` + OAuth SSO): a data engineer reported that a deep
link opened after session expiry always landed on the homepage, requiring a
second click on the same link to actually reach the target. Filed as issue
#73133 with full reproduction details and the trace of the surrounding call
chain.
## Checklist
- [x] I have performed a self-review of my own code
- [x] Unit tests added for the change
- [x] No breaking changes: when `next_url` is not supplied,
`get_url_login()` returns exactly the same value as before
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
