Add Authenticator for when serving OIDC as a proxy by DiamondJoseph · Pull Request #877 · bluesky/tiled

DiamondJoseph · 2025-02-03T17:50:23Z

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 · 2025-02-05T16:38:59Z

tiled/server/utils.py

+        description: Optional[str] = None,
+    ):
+        self.model: APIKey = APIKey(
+            **{"in": APIKeyIn.header}, name=name, description=description


I hate this but I see why it is required (in is a reserved keyword but APIKey uses it as an init arg... using _in does not work, as it is a serialisation alias only).

DiamondJoseph · 2025-02-11T14:47:09Z

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

DiamondJoseph · 2025-02-20T11:03:28Z

tiled/authenticators.py

        )

+    async def decode_access_token(self, access_token: str) -> dict[str, Any]:
+        keys = httpx.get(self.jwks_uri).raise_for_status().json().get("keys", [])


As discussed, use a TTL cache to ensure we do not hammer the AuthN provider

(for the result of httpx.get(self.jwks_uri))

DiamondJoseph · 2025-02-20T11:04:21Z

tiled/authenticators.py

+        )
+
+    @property
+    def oauth2_scheme(self) -> Callable[[Request], str]:


Suggested change

def oauth2_scheme(self) -> Callable[[Request], str]:

def oauth2_scheme(self) -> OAuth2:

DiamondJoseph · 2025-02-20T11:05:34Z

example_configs/external_service/custom.py

Just some type hinting for my benefit

DiamondJoseph · 2025-02-20T11:05:50Z

tiled/authn_database/connection_pool.py

Just some type hinting for my benefit

DiamondJoseph · 2025-02-20T11:07:12Z

tiled/client/container.py

For consistency with the serialization/deserialization registries.

DiamondJoseph · 2025-02-20T12:06:42Z

tiled/server/router.py

+                    annotation=Optional[List[field_type]],
+                )
+                parameters.append(injected_parameter)
+        route_with_sig.__signature__ = signature.replace(parameters=parameters)


I wonder if much of this could be replaced by replacing search/distinct **kwargs with Unpack[QueryParams] where QueryParams is a TypedDict generated by the QueryRegistry?

tiled/server/authentication.py

DiamondJoseph · 2025-02-20T13:15:05Z

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 · 2025-02-20T13:25:38Z

example_configs/external_service/custom.py

        self.metadata = metadata

-    def with_session_state(self, state):
+    def with_session_state(self, state: dict[str, Any]) -> Self:


From a design perspective, it's just expected to still be an Adapter (?)

danielballan · 2025-03-06T23:31:59Z

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

- Prevents having to inject the password late into the oauth2_schema, and allows supporting Proxied credentials

- Such that decode_access_token can be overriden when serving behind proxied OIDC - Removes injection of password into security obj - Removes use of dependency_override which is intended for use in tests

….ParamSpec

danielballan · 2025-03-07T00:29:21Z

Rebased on main and force-pushed.

danielballan · 2025-03-07T01:09:55Z

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 · 2025-03-07T01:35:36Z

tiled/server/authentication.py

+    ):
+        "Mark a Session as revoked so it cannot be refreshed again."
+        request.state.endpoint = "auth"
+        payload = await decode_access_token(refresh_token.refresh_token)


Something's crossed here, where we're decoding the refresh token with decode_access_token.

Yeah, I think it should be named decode_token but otherwise it's solid: there's a "get access token from request" method and a "decode token" that can take an access or request token.

danielballan · 2025-03-07T02:13:14Z

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 · 2025-03-07T02:17:10Z

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

DiamondJoseph · 2025-03-07T09:31:17Z

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 😎 ) 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 · 2025-03-07T11:16:00Z

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

#916

danielballan · 2025-03-07T11:25:29Z

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.

DiamondJoseph force-pushed the decode-token branch 2 times, most recently from fd664f0 to 02232db Compare February 5, 2025 16:25

DiamondJoseph commented Feb 5, 2025

View reviewed changes

DiamondJoseph mentioned this pull request Feb 19, 2025

Refactor auth #891

Merged

2 tasks

DiamondJoseph force-pushed the decode-token branch from e1a752c to 2ee2a42 Compare February 20, 2025 11:02

DiamondJoseph commented Feb 20, 2025

View reviewed changes

Ware, Joseph (DLSLtd,RAL,LSCI) and others added 18 commits March 6, 2025 19:28

Add Authenticator for when serving OIDC as a proxy

e80b535

Refactor Authentication router construction

0589387

- Prevents having to inject the password late into the oauth2_schema, and allows supporting Proxied credentials

Invert the creation of API routes

939cdb3

- Such that decode_access_token can be overriden when serving behind proxied OIDC - Removes injection of password into security obj - Removes use of dependency_override which is intended for use in tests

Remove incorrect caches

fd7be50

Linting, minimising code changes

3743679

Fix linting for Python 3.9

018d72f

Rename default registries

b50d216

Name consistent and move scope handling

4b88a7b

Pass first authenticator to make decode route

ed75f3e

Remove unused method params

471b797

types

c279181

Split decode and fetch token

a1669a0

Refactor to pass OAuth and token decode methods consistently

7b20ba8

Await awaitables

a0bea99

Restore route signature patching with Query registry

f511991

nit: reorder args

2cb5268

Move definition of routes internally and remove use of non 3.9 Typing…

f6f9a10

….ParamSpec

Fix misplaced prefix.

3ca1344

danielballan force-pushed the decode-token branch from 79f8497 to 3ca1344 Compare March 7, 2025 00:29

Update suggested usage in comment.

765bdae

danielballan reviewed Mar 7, 2025

View reviewed changes

danielballan added 2 commits March 6, 2025 20:52

Fix annotations

f3cc565

Expired token should send 401 to prompt refresh

08b153c

danielballan and others added 4 commits March 8, 2025 11:36

Stash authn database engine on app.state, not global dict.

58cf84f

More type hints

e972d24

Oops

d5cd87b

Rename decode_token

8378221

	def oauth2_scheme(self) -> Callable[[Request], str]:
	def oauth2_scheme(self) -> OAuth2:

Conversation

DiamondJoseph commented Feb 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DiamondJoseph commented Feb 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DiamondJoseph commented Feb 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielballan commented Mar 6, 2025

Uh oh!

danielballan commented Mar 7, 2025

Uh oh!

danielballan commented Mar 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielballan commented Mar 7, 2025

Uh oh!

danielballan commented Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DiamondJoseph commented Mar 7, 2025

Uh oh!

DiamondJoseph commented Mar 7, 2025

Uh oh!

danielballan commented Mar 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

DiamondJoseph commented Feb 3, 2025 •

edited

Loading

danielballan commented Mar 7, 2025 •

edited

Loading