Add Authenticator for when serving OIDC as a proxy
WIP: adds an Authenticator implementation for use when serving Tiled behind a proxy.
This is intended be used to define an alternative decode_access_token
In order to inject the desired behaviour for decode_access_token has required a fairly weighty refactor, inverting the creation order of a lot of router objects.
- Removes injection of password into security obj
- Removes use of dependency_override which is intended for use in tests
Checklist
- [ ] Add a Changelog entry
- [ ] Add the ticket number which this PR closes to the comment section
why setattr
do not call public keys with every request- find reasonable TTL
Test Summary as I hand off this PR for a week of leave:
173 failed, 432 passed, 4 skipped, 4 xfailed, 13 warnings in 510.08s (0:08:30) Of which >=156 from search/distinct signature or function being broken (see minor change to how patch_route_signature is called, but I don't see where I've gone wrong...) 8 instances where the settings object does not have database settings when gotten from get_settings, (theory: because injected in via server settings and those changes not reflected on global object (?)) 4 where did not raise expected exception 2 where the token is expired when decrypted for some reason now
Summary of changes' intents:
- Pass registries into the construction of base router
- Pass Authenticators into the construction of the authentication router
- Do not mutate FastAPI objects after creation
- Pass OAuth2 (Callable[[Request], str | None]) object around to get encoded token from incoming request
- Pass
token_decoder: Callable[[str], dict[str, Any]]object around to get decoded token contents - Make moving the API key from query params to headers its own function and have as
Dependsof the app as a whole
We're down to a tractable number of failures with a hilariously tiny change, pushed above.
FAILED tiled/_tests/test_access_control.py::test_entry_based_scopes - AttributeError: 'NoneType' object has no attribute 'execute'
FAILED tiled/_tests/test_access_control.py::test_top_level_access_control - AttributeError: 'NoneType' object has no attribute 'execute'
FAILED tiled/_tests/test_access_control.py::test_access_control_with_api_key_auth - AttributeError: 'NoneType' object has no attribute 'execute'
FAILED tiled/_tests/test_access_control.py::test_node_export - AttributeError: 'NoneType' object has no attribute 'execute'
FAILED tiled/_tests/test_access_control.py::test_create_and_update_allowed - AttributeError: 'NoneType' object has no attribute 'execute'
FAILED tiled/_tests/test_access_control.py::test_writing_blocked_by_access_policy - AttributeError: 'NoneType' object has no attribute 'execute'
FAILED tiled/_tests/test_access_control.py::test_create_blocked_by_access_policy - AttributeError: 'NoneType' object has no attribute 'execute'
FAILED tiled/_tests/test_access_control.py::test_public_access - AttributeError: 'NoneType' object has no attribute 'execute'
FAILED tiled/_tests/test_authentication.py::test_refresh_transparent - jose.exceptions.ExpiredSignatureError: Signature has expired.
FAILED tiled/_tests/test_authentication.py::test_admin - Failed: DID NOT RAISE <class 'httpx.HTTPStatusError'>
FAILED tiled/_tests/test_cli.py::test_serve_config[] - _queue.Empty
FAILED tiled/_tests/test_size_limit.py::test_array - Failed: DID NOT RAISE <class 'httpx.HTTPStatusError'>
FAILED tiled/_tests/test_size_limit.py::test_dataframe - Failed: DID NOT RAISE <class 'httpx.HTTPStatusError'>
FAILED tiled/_tests/adapters/test_sql.py::test_write_read_one_batch_one_part[adapter_psql_one_partition] - ValueError: The database uri must start with `duckdb:`, `sqlite:`, or `postgresql:`
FAILED tiled/_tests/adapters/test_sql.py::test_write_read_list_batch_one_part[adapter_psql_one_partition] - ValueError: The database uri must start with `duckdb:`, `sqlite:`, or `postgresql:`
FAILED tiled/_tests/adapters/test_sql.py::test_write_read_one_batch_many_part[adapter_psql_many_partition] - ValueError: The database uri must start with `duckdb:`, `sqlite:`, or `postgresql:`
ERROR tiled/_tests/adapters/test_sql.py::test_psql - ValueError: The database uri must start with `duckdb:`, `sqlite:`, or `postgresql:`
= 16 failed, 554 passed, 13 skipped, 4 xfailed, 301 warnings, 1 error in 470.27s (0:07:50) =
Rebased on main and force-pushed.
This is where we are post-rebase:
FAILED tiled/_tests/test_authentication.py::test_refresh_transparent[sqlite_database_uri] - jose.exceptions.ExpiredSignatureError: Signature has expired.
FAILED tiled/_tests/test_authentication.py::test_refresh_transparent[postgresql_database_uri] - jose.exceptions.ExpiredSignatureError: Signature has expired.
FAILED tiled/_tests/test_authentication.py::test_admin[sqlite_database_uri] - Failed: DID NOT RAISE <class 'httpx.HTTPStatusError'>
FAILED tiled/_tests/test_authentication.py::test_admin[postgresql_database_uri] - Failed: DID NOT RAISE <class 'httpx.HTTPStatusError'>
ERROR tiled/_tests/test_access_control.py::test_entry_based_scopes - jose.exceptions.ExpiredSignatureError: Signature has expired.
ERROR tiled/_tests/test_access_control.py::test_top_level_access_control - jose.exceptions.ExpiredSignatureError: Signature has expired.
ERROR tiled/_tests/test_access_control.py::test_access_control_with_api_key_auth - jose.exceptions.ExpiredSignatureError: Signature has expired.
ERROR tiled/_tests/test_access_control.py::test_node_export - jose.exceptions.ExpiredSignatureError: Signature has expired.
ERROR tiled/_tests/test_access_control.py::test_create_and_update_allowed - jose.exceptions.ExpiredSignatureError: Signature has expired.
ERROR tiled/_tests/test_access_control.py::test_writing_blocked_by_access_policy - jose.exceptions.ExpiredSignatureError: Signature has expired.
ERROR tiled/_tests/test_access_control.py::test_create_blocked_by_access_policy - jose.exceptions.ExpiredSignatureError: Signature has expired.
ERROR tiled/_tests/test_access_control.py::test_public_access - jose.exceptions.ExpiredSignatureError: Signature has expired.
ERROR tiled/_tests/test_size_limit.py::test_array - asyncpg.exceptions.InvalidCatalogNameError: database "tiled_test_disposable_845349...
ERROR tiled/_tests/test_size_limit.py::test_dataframe - asyncpg.exceptions.InvalidCatalogNameError: database "tiled_test_disposable_845349...
================ 4 failed, 21 passed, 19 warnings, 10 errors in 26.59s ================
I think the next issue is that tiled.authn_database.connection_pool makes reference to the global-ish get_settings(), a FastAPI-recommended pattern that I wish I had ignored, and that this result does not align with merged_settings.
With fresh eyes, I think that it would make sense to put the database connection pool in app.state, rather than using global get_* functions. It is in fact "application state", and this seems like a better place for it to live than in a global registry.
We might stash the merged_settings there too, rather than similarly relying on the global get_settings().
This is where things stand as I set this down. The None issue is related to get_settings() and merged_settings being out of sync, per comment above.
FAILED tiled/_tests/test_access_control.py::test_entry_based_scopes - AttributeError: 'NoneType' object has no attribute 'execute'
FAILED tiled/_tests/test_access_control.py::test_top_level_access_control - AttributeError: 'NoneType' object has no attribute 'execute'
FAILED tiled/_tests/test_access_control.py::test_access_control_with_api_key_auth - AttributeError: 'NoneType' object has no attribute 'execute'
FAILED tiled/_tests/test_access_control.py::test_node_export - AttributeError: 'NoneType' object has no attribute 'execute'
FAILED tiled/_tests/test_access_control.py::test_create_and_update_allowed - AttributeError: 'NoneType' object has no attribute 'execute'
FAILED tiled/_tests/test_access_control.py::test_writing_blocked_by_access_policy - AttributeError: 'NoneType' object has no attribute 'execute'
FAILED tiled/_tests/test_access_control.py::test_create_blocked_by_access_policy - AttributeError: 'NoneType' object has no attribute 'execute'
FAILED tiled/_tests/test_access_control.py::test_public_access - AttributeError: 'NoneType' object has no attribute 'execute'
FAILED tiled/_tests/test_authentication.py::test_admin[sqlite_database_uri] - Failed: DID NOT RAISE <class 'httpx.HTTPStatusError'>
FAILED tiled/_tests/test_authentication.py::test_admin[postgresql_database_uri] - Failed: DID NOT RAISE <class 'httpx.HTTPStatusError'>
FAILED tiled/_tests/test_size_limit.py::test_array - tiled.client.utils.ClientError: 401: Invalid API key http://local-tiled-app/api/v1...
FAILED tiled/_tests/test_size_limit.py::test_dataframe - tiled.client.utils.ClientError: 401: Invalid API key http://local-tiled-app/api/v1...
=== 12 failed, 647 passed, 13 skipped, 4 xfailed, 453 warnings in 547.45s (0:09:07) ===
We might stash the
merged_settingsthere too, rather than similarly relying on the globalget_settings().
So what exactly is the difference between what is loaded by Settings, which extends Pydantic BaseSettings and so picks up any environment (PR to make it slightly clearer :sunglasses: ) and what goes into the merged_settings object?
I think one of the easiest ways to get to decoupling the fastapi creation from the server startup is making use of BaseSettings as the CLI entrypoint- we looked at using BaseSettings for blueapi previously but because we made heavy use of click for our entrypoint and it hijacked it we found it unsuitable- but if refactoring how the app is starting anyway it may be an option.
I think one of the easiest ways to get to decoupling the fastapi creation from the server startup is making use of BaseSettings as the CLI entrypoint [...]
I've made a mockup of how this would look for some of the simple commands, but it would require another turning the sock inside out
https://github.com/bluesky/tiled/pull/916
Using a Settings object as the way to initialize an app seems right.
I am less sure about building a CLI application around it, but open to exploring. We have quite a lot of CLI code built on Typer (which is in turn built on Click) that would need to be changed.
For this PR, given the scope, the small change we can devise that moves in the generally right direction and passes the tests is the thing.