tiled icon indicating copy to clipboard operation
tiled copied to clipboard

Add Authenticator for when serving OIDC as a proxy

Open DiamondJoseph opened this issue 11 months ago • 10 comments

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

DiamondJoseph avatar Feb 03 '25 17:02 DiamondJoseph

why setattr do not call public keys with every request- find reasonable TTL

DiamondJoseph avatar Feb 11 '25 14:02 DiamondJoseph

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 Depends of the app as a whole

DiamondJoseph avatar Feb 20 '25 13:02 DiamondJoseph

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) =

danielballan avatar Mar 06 '25 23:03 danielballan

Rebased on main and force-pushed.

danielballan avatar Mar 07 '25 00:03 danielballan

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 ================

danielballan avatar Mar 07 '25 01:03 danielballan

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().

danielballan avatar Mar 07 '25 02:03 danielballan

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) ===

danielballan avatar Mar 07 '25 02:03 danielballan

We might stash the merged_settings there too, rather than similarly relying on the global get_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.

DiamondJoseph avatar Mar 07 '25 09:03 DiamondJoseph

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

DiamondJoseph avatar Mar 07 '25 11:03 DiamondJoseph

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.

danielballan avatar Mar 07 '25 11:03 danielballan