fix: Unwrap SecretStr tokens when building Trino auth objects - #6771
fix: Unwrap SecretStr tokens when building Trino auth objects#6771AdityaPatil22 wants to merge 2 commits into
Conversation
Sorry, something went wrong.
|
⚠️ Please install the Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## master #6771 +/- ##
==========================================
+ Coverage 47.09% 47.11% +0.02%
==========================================
Files 419 419
Lines 51878 51879 +1
Branches 7525 7525
==========================================
+ Hits 24430 24441 +11
+ Misses 25700 25689 -11
- Partials 1748 1749 +1
Continue to review full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Sorry, something went wrong.
aniketpalu
left a comment
There was a problem hiding this comment.
It would be valuable to add a test case that verifies the behavior when a config contains both SecretStr and regular fields to ensure the conditional unwrapping works correctly for mixed scenarios.
Sorry, something went wrong.
| from feast.infra.offline_stores.contrib.trino_offline_store.trino import AuthConfig | ||
|
|
||
|
|
||
| def test_jwt_auth_produces_plain_str_token(): |
There was a problem hiding this comment.
Not a blocker: While this test ensures OAuth2 auth still works, it could be enhanced to test a config with actual OAuth2 parameters to ensure non-SecretStr fields are handled correctly in the new code path.
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks for the suggestion - OAuth2 doesn't actually have any config parameters in CLASSES_BY_AUTH_TYPE (its auth_model is None, and to_trino_auth() short-circuits to trino_auth_cls() with no fields at all), so there's no non-SecretStr field handling to exercise on that path. The new test_to_trino_auth_unwraps_only_secret_fields_in_mixed_model and test_basic_auth_with_plain_fields_unaffected tests cover the non-SecretStr field handling instead.
Sorry, something went wrong.
|
Please rebase the branch with master. @ntkathole: Needs ok-to-test |
Sorry, something went wrong.
Signed-off-by: Aditya Patil <adityapatil7649@gmail.com>
Signed-off-by: Aditya Patil <adityapatil7649@gmail.com>
|
@aniketpalu - Made the changes as suggested, added a test covering the mixed SecretStr + plain field scenario. |
Sorry, something went wrong.
What this PR does / why we need it:
The Trino offline store fails when using JWT authentication: TypeError: can only concatenate str (not "SecretStr") to str
This happens because AuthConfig.to_trino_auth() passed model.model_dump() directly into Trino's auth classes. For JWT, the token field is a Pydantic SecretStr (to avoid leaking it in logs/reprs), but model_dump() leaves it as a SecretStr object instead of a plain string, and Trino's JWTAuthentication does "Bearer " + self.token internally, which fails on a non-str.
The fix unwraps any SecretStr fields via .get_secret_value() before constructing the Trino auth object, in to_trino_auth(). This is scoped to the auth boundary only — it doesn't touch _get_trino_client(), doesn't monkey-patch the Trino library, and doesn't change the user-facing config format. Other auth types (basic, kerberos, oauth2, certificate) are unaffected since none of their models use SecretStr.
Which issue(s) this PR fixes:
Fixes #6760
Checks
Testing Strategy
Added sdk/python/tests/unit/infra/offline_stores/contrib/trino_offline_store/test_trino_auth.py:
Ran the full trino_offline_store unit test suite (46 passed) plus ruff lint/format on modified files.