ChickenBenny opened a new pull request, #76746:
URL: https://github.com/apache/airflow/pull/76746
## Why
Since #61001 (provider 3.2.0), `add_user` in the FAB security manager
override raises
`FabException` when the insert fails. Before that, it returned a falsy
value, as upstream
Flask-AppBuilder still does. Every caller in the provider and in
Flask-AppBuilder handles a failed
registration with `if not user:`. None of them catch `FabException`, which
is the provider's own
class and does not subclass Flask-AppBuilder's `FABException` or
`ValueError`. A failed insert
(for example a duplicate username/email race or a lost DB connection)
therefore skips the intended
failure branch.
For example, this is the call chain when OAuth auto-registration fails:
```
FastAPI (/auth/...)
└ WSGIMiddleware fastapi.middleware.wsgi, forwards
the request to Flask
└ Flask.wsgi_app() flask/app.py:1479 outer try
└ Flask.full_dispatch_request() flask/app.py:904 inner try
└ Flask.dispatch_request() resolves the view for the URL
└ CustomAuthOAuthView.oauth_authorized()
└ AuthOAuthView.oauth_authorized()
(Flask-AppBuilder)
└ sm.auth_user_oauth()
└ sm.add_user() raises FabException
```
`auth_user_oauth` has an `if not user:` branch that returns `None`, and the
FAB view then flashes
"invalid login" and redirects. The exception skips both and ends in Flask's
500 handler. The other
callers fail the same way:
| Path | Intended behaviour on failure | Before this PR |
|---|---|---|
| LDAP auto-registration via FAB `AuthLDAPView.login` | "Invalid login"
flash, redirect | HTTP 500 |
| LDAP via the basic-auth API backend | 401, fall back to `auth_user_db` |
HTTP 500 |
| LDAP via `FabAuthManager.create_token` (`POST /auth/token`) | 401 | HTTP
500 |
| OAuth auto-registration via `oauth_authorized` | "Invalid login" flash,
redirect | HTTP 500 |
| REMOTE_USER auto-registration | "Invalid login" flash | HTTP 500 |
| Self-registration activation (FAB `RegisterUserDBView.activation`) | Error
flash, redirect | HTTP 500, registration request left in the DB |
| `POST /fab/v1/users` | 500 with `Failed to add user ...` detail | Generic
unhandled 500 |
| `airflow users create` | `SystemExit("Failed to create user")` | Traceback
|
## What
- `add_user` returns `None` on failure again (`-> User | None`), so each
caller's existing
`if not user:` branch handles it.
- `auth_user_remote_user` returns `None` when registration fails. It never
checked the result of
`add_user`, so a failed registration reached `update_user_auth_stat(None)`
and still ended in a
500. This gap predates #61001 and also exists upstream.
`add_user` returns `None` rather than upstream's `False`:
- Every caller only tests truthiness, so `None` and `False` behave the same
for them.
- The `auth_user_*` methods assign `find_user(...)` (`User | None`) and then
`add_user(...)` to
the same variable. With `User | None`, mypy accepts this without extra
annotations at each
call site.
- Other create methods in the override (`add_register_user`, `update_role`)
already return `None`
on failure.
The tests make `session.commit()` fail and run the real `add_user`, without
mocking its return
value. This way, a caller's test fails if the failure contract changes again.
I also checked the other methods #61001 changed. `add_role` and
`create_resource` now raise where
they used to return an unsaved, truthy object. No caller checks their result
for falsiness, so they
are left as they are.
## Testing
- New tests in `test_override.py` fail without the fix (3 failed) and pass
with it (3 passed).
- `providers/fab/tests/unit/fab/auth_manager/security_manager/`,
`api_fastapi/services/test_users.py`,
`cli_commands/test_user_command.py`: 204 passed, 1 skipped.
- `breeze run mypy
providers/fab/src/airflow/providers/fab/auth_manager/security_manager/override.py`:
no issues found.
- `prek run --from-ref main --stage pre-commit`: passed.
## Related
- #61001 introduced the `raise FabException` in `add_user`.
- Upstream Flask-AppBuilder 5.2.2 `SecurityManager.add_user`
(`flask_appbuilder/security/sqla/manager.py`)
returns `False` on failure. The FAB callers this PR relies on
(`AuthLDAPView`, `AuthOAuthView`,
`RegisterUserDBView.activation`) are written against that contract.
--
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]
