Syk/refactor the config file - #96
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #96 +/- ##
==========================================
+ Coverage 76.35% 80.28% +3.92%
==========================================
Files 9 14 +5
Lines 791 832 +41
Branches 146 149 +3
==========================================
+ Hits 604 668 +64
+ Misses 131 115 -16
+ Partials 56 49 -7
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
beda4cf to
c65fab7
Compare
jason-famedly
left a comment
jason-famedly
left a comment
There was a problem hiding this comment.
That was a lot 😅
Here's a first pass. Mostly, it really looks good and it helped me get it all into my headspace in a more organized way. I certainly know more now than I did before. Let's address all these and see where we sit after. I still want to make those attributes on the main TokenAuthenticatorConfig not be depending on a walrus operator so they could accidentally become unreference-able(this is not an acceptable use of getattr() in my book!), but lets get this first pass done before hand.
| class BaseConfigModel(BaseModel): | ||
| model_config = ConfigDict(arbitrary_types_allowed=True, extra="forbid") |
There was a problem hiding this comment.
I'm curious what will happen if any existing configs have extra fields that were previously ignored and now will be rejected? 🤔 Just noting this as a change in behavior that may have unexpected side-effects.
There was a problem hiding this comment.
hmm I think extra fields should be prohibited and admins should keep there configs clean, but this could be breaking change, so I will switch to extra="ignore"
There was a problem hiding this comment.
Normally, I would absolutely agree. But yes, breaking changes and all that. We can always tighten this up after giving fair warning(and best case scenario there would be nothing for admins to do)
| class TestJwtValidationConfig: | ||
| def test_jwt_validation_config(self): | ||
| jwk = get_jwk().export(private_key=True) | ||
| config = JwtValidationConfig(jwk_set=jwk) | ||
| assert config.validator == Exist() | ||
| assert config.require_expiry is False | ||
| assert config.localpart_path is None | ||
| assert config.user_id_path is None | ||
| assert config.fq_uid_path is None | ||
| assert config.displayname_path is None | ||
| assert config.admin_path is None | ||
| assert config.email_path is None | ||
| assert config.required_scopes is None | ||
| assert config.jwk_set is not None and isinstance(config.jwk_set, JWK) | ||
| assert config.jwk_file is None | ||
| assert config.jwks_endpoint is None | ||
|
|
||
| def test_jwt_validation_config_missing_jwk_source(self): | ||
| with pytest.raises(ValidationError): | ||
| JwtValidationConfig() |
There was a problem hiding this comment.
So from here down in the review, all these Test cases are essentially testing defaults(unless something was required, of course). I would like to see a few more tests where we exercise for things that are not defaults or required. Maybe one test for each that has everything(like a full test), then selectively for odd Typed attributes(like that allowed_client_ids being a list[str]: make sure it won't accept a str since it has a lot of the same attributes of a list[str]).
510c446 to
8ca3565
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit fc97c90. Configure here.
2 participants
SYN-95
Note
Medium Risk
Config parsing at Synapse startup now depends on Pydantic validation and stricter types (e.g. OIDC allowed_client_ids); misconfiguration may fail earlier with different errors. OAuth/EPA metadata registration still treats expose_metadata_resource like a dict in token_authenticator while config now stores a Pydantic model—worth verifying those paths at runtime.
Overview
Replaces the monolithic
config.pywith asynapse_token_authenticator/config/package built on Pydantic, and addspydanticas a runtime dependency.TokenAuthenticatorConfignow instantiates typed models (JwtConfig,OIDCConfig,OAuthConfig,EPaConfig) instead of nested manual parsing. Shared behavior lives inbase.py: claim-validator coercion (parallel to the claims DSL), JWK/JWKSet resolution from dict, JSON string, PEM file, or JWKS URL, and reusable claim-mapping fields.OAuth / EPA / JWT validation rules are largely preserved (required JWK source, encryption JWK for EPA, at least one of jwt vs introspection for OAuth, JWT secret/keyfile checks, allowed algorithms via
Literal).OIDCConfig.allowed_client_idsis nowlist[str](a bare string is rejected).parse_validatorinclaims_validator.pyraisesValueErrorinstead of genericException.http_authmoves underconfig/http_auth.pywithHttpAuthFieldcoercion; config-class prefix was removed from auth error logs. Imports intoken_authenticatorandlogin_metadatafollow the new module layout.Coverage is expanded with
tests/test_config/for validators, JWK sources, and each config type.Reviewed by Cursor Bugbot for commit d2351c1. Bugbot is set up for automated code reviews on this repo. Configure here.