feat(jwt authenticator): Allow configuring the signed JWT token to be injected as a request_option for JwtAuthenticator #776
Conversation
…tion for JwtAuthenticator
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. Testing This CDK VersionYou can test this version of the CDK using the following: # Run the CLI from this branch:
uvx 'git+https://github.com/airbytehq/airbyte-python-cdk.git@brian/jwt_authenticator_configurable_request_option#egg=airbyte-python-cdk[dev]' --help
# Update a connector to use the CDK from this branch ref:
cd airbyte-integrations/connectors/source-example
poe use-cdk-branch brian/jwt_authenticator_configurable_request_optionHelpful ResourcesPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
PyTest Results (Fast)3 777 tests +5 3 765 ✅ +5 6m 20s ⏱️ +5s Results for commit 0ce9630. ± Comparison against base commit 25132b6. This pull request removes 1 and adds 6 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
PyTest Results (Full)3 780 tests +5 3 768 ✅ +5 11m 2s ⏱️ ±0s Results for commit 0ce9630. ± Comparison against base commit 25132b6. This pull request removes 1 and adds 6 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
airbyte_cdk/sources/declarative/models/declarative_component_schema.py
Outdated
Show resolved
Hide resolved
📝 WalkthroughWalkthroughAdds RequestOption-driven JWT injection to declarative components. JwtAuthenticator now accepts an optional Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Factory as ModelToComponentFactory
participant Auth as JwtAuthenticator
participant ReqOpt as RequestOption
participant HTTP as HTTP Request
Client->>Factory: Build components from manifest
Factory->>Auth: new JwtAuthenticator(..., request_option)
Note over Auth: Ensure default RequestOption if absent
Client->>Auth: Prepare request
Auth->>Auth: Generate JWT (payload, headers)
Auth->>ReqOpt: Resolve injection target & key
alt inject_into == header
Auth->>HTTP: get_auth_header() -> {key: "Authorization" or custom, value: "Bearer <jwt>"}
else inject_into == request_parameter
Auth->>HTTP: get_request_params() -> {field_name: "<jwt>"}
else inject_into == body_data
Auth->>HTTP: get_request_body_data() -> {field_name: "<jwt>"} or string
else inject_into == body_json
Auth->>HTTP: get_request_body_json() -> {field_name: "<jwt>"}
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2024-12-11T16:34:46.319ZApplied to files:
🧬 Code graph analysis (1)airbyte_cdk/sources/declarative/models/declarative_component_schema.py (4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
unit_tests/sources/declarative/parsers/test_model_to_component_factory.py (1)
3037-3040: Consider validating thefield_nameas well?The test adds a
request_optionwithfield_name: authorization, but lines 3148-3151 only checkinject_into. Should we also assert thatauthenticator._request_option.field_name.eval(config) == "authorization"to ensure the field name is correctly propagated? This would make the test more thorough, wdyt?Apply this diff to add field_name validation:
if authenticator_manifest.get("request_option"): assert authenticator._request_option.inject_into.value == authenticator_manifest.get( "request_option", {} ).get("inject_into") + assert authenticator._request_option.field_name.eval(config) == authenticator_manifest.get( + "request_option", {} + ).get("field_name")unit_tests/sources/declarative/auth/test_jwt.py (2)
293-354: Fix the duplicated test IDs for clarity.The test IDs at lines 303, 310, and 317 are all set to
"test_get_request_headers", but this test is actuallytest_get_request_options. This makes it harder to identify which parametrized case failed. Could you update them to something like"inject_into_request_parameter","inject_into_body_data", and"inject_into_body_json"to better reflect what's being tested? Wdyt?Apply this diff:
pytest.param( RequestOption( inject_into=RequestOptionType.request_parameter, field_name="custom_parameter", parameters={}, ), "custom_parameter", - id="test_get_request_headers", + id="inject_into_request_parameter", ), pytest.param( RequestOption( inject_into=RequestOptionType.body_data, field_name="custom_body", parameters={} ), "custom_body", - id="test_get_request_headers", + id="inject_into_body_data", ), pytest.param( RequestOption( inject_into=RequestOptionType.body_json, field_name="custom_json", parameters={} ), "custom_json", - id="test_get_request_headers", + id="inject_into_body_json", ),
356-394: Consider a more descriptive test ID for the custom header case.The test ID at line 366 is
"test_get_request_headers", which just repeats the test name. For consistency with the second case ("test_with_default_authorization_header"), maybe something like"test_with_custom_authorization_header"would be clearer? Just a thought!Apply this diff:
pytest.param( RequestOption( inject_into=RequestOptionType.header, field_name="custom_authorization", parameters={}, ), "custom_authorization", - id="test_get_request_headers", + id="test_with_custom_authorization_header", ),airbyte_cdk/sources/declarative/auth/jwt.py (1)
227-228: The auth_header implementation is clever but could use a clarifying comment.As noted in the past review comment, returning an empty string when not injecting into headers is a bit implicit—it relies on the caller (
get_auth_header()) treating empty string as falsy. While this reuses the existing abstract method, it might be worth adding a brief comment explaining why an empty string is returned in the non-header case. This would help future readers understand the intention. Wdyt?For example:
@property def auth_header(self) -> str: # Returns the header key if injecting into headers, otherwise returns "" # which causes get_auth_header() to return an empty dict options = self._get_request_options(RequestOptionType.header) return next(iter(options.keys()), "")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
airbyte_cdk/sources/declarative/auth/jwt.py(5 hunks)airbyte_cdk/sources/declarative/declarative_component_schema.yaml(1 hunks)airbyte_cdk/sources/declarative/models/declarative_component_schema.py(2 hunks)airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py(2 hunks)unit_tests/sources/declarative/auth/test_jwt.py(2 hunks)unit_tests/sources/declarative/parsers/test_model_to_component_factory.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
unit_tests/sources/declarative/auth/test_jwt.py (3)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (2)
RequestOption(360-379)JwtAuthenticator(429-491)airbyte_cdk/sources/declarative/requesters/request_option.py (1)
RequestOptionType(13-21)airbyte_cdk/sources/declarative/auth/jwt.py (7)
JwtAuthenticator(55-251)_get_jwt_payload(154-178)_get_secret_key(180-199)_get_jwt_headers(135-152)get_request_params(238-239)get_request_body_data(241-242)get_request_body_json(244-245)
airbyte_cdk/sources/declarative/auth/jwt.py (2)
airbyte_cdk/sources/declarative/auth/declarative_authenticator.py (1)
DeclarativeAuthenticator(14-29)airbyte_cdk/sources/declarative/requesters/request_option.py (2)
RequestOptionType(13-21)inject_into_request(71-117)
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (2)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
JwtAuthenticator(429-491)airbyte_cdk/sources/declarative/auth/jwt.py (1)
JwtAuthenticator(55-251)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
airbyte_cdk/sources/declarative/requesters/request_option.py (1)
RequestOption(25-117)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Check: source-shopify
🔇 Additional comments (10)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)
1279-1282: LGTM! Clean addition of therequest_optionfield.The new
request_optionproperty is well-defined and appropriately optional, ensuring backward compatibility. The description clearly communicates its purpose, and the reference toRequestOptionis correct. I can see thatRequestOption(defined at lines 2530-2573) supports all the necessary injection targets includingbody_json, which addresses the Box connector use case mentioned in the PR objectives. Nice work!unit_tests/sources/declarative/parsers/test_model_to_component_factory.py (1)
3148-3151: Incorrect review comment: default-case already tested The default Authorization header injection is covered by thepytest.param(None, "Authorization", id="test_with_default_authorization_header")intest_get_request_headersofunit_tests/sources/declarative/auth/test_jwt.py.Likely an incorrect or invalid review comment.
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (2)
2686-2714: LGTM! The request_option propagation looks good.The method now correctly instantiates a
RequestOptioncomponent from the model when provided and passes it through toJwtAuthenticator. The change fromstaticmethodto instance method is necessary and appropriate for accessing_create_component_from_model. This enables the new per-request injection feature while maintaining backward compatibility. Nice work!
2716-2730: Consistent request_option handling for ListPartitionRouter.The method follows the same pattern as
create_jwt_authenticator, conditionally creating and passing arequest_optiontoListPartitionRouter. This consistency makes the codebase easier to understand and maintain.airbyte_cdk/sources/declarative/auth/jwt.py (3)
92-92: Great backward-compatible default for request_option!The new
request_optionfield with its default initialization ensures that existing connectors continue to work without any changes (injecting into theAuthorizationheader). The explanatory comment is helpful for future maintainers. Nice design choice!Also applies to: 128-133
238-245: Clean implementation of the authenticator interface methods.The new
get_request_params,get_request_body_data, andget_request_body_jsonmethods properly implement theDeclarativeAuthenticatorinterface by delegating to the shared_get_request_optionshelper. This keeps the code DRY and makes the logic easy to follow.
247-251: Excellent separation of concerns in the injection logic.The
_get_request_optionshelper cleanly encapsulates the conditional injection logic. It only populates the options mapping when the target matches the configuredinject_intotype, which keeps the interface methods simple and the behavior predictable. Well done!airbyte_cdk/sources/declarative/models/declarative_component_schema.py (3)
353-357: LGTM! The InjectInto enum values align with the runtime RequestOptionType.The four injection targets (request_parameter, header, body_data, body_json) match the runtime implementation expectations.
360-379: Consider adding schema-level validation for field_name/field_path mutual exclusivity?The runtime
RequestOptionclass (inrequest_option.py) enforces that exactly one offield_nameorfield_pathmust be provided, and thatfield_pathonly works withbody_jsoninjection. Currently, the schema model allows both to beNoneor both to be set, which would fail at runtime.Would it make sense to add a Pydantic validator here to catch these issues earlier during manifest parsing, or is the intent to keep the schema simple and rely on runtime validation? The current approach works, but early validation might provide a better developer experience. wdyt?
486-490: LGTM! The request_option field addition enables flexible JWT injection.The optional
request_optionfield is well-documented and maintains backward compatibility. The description clearly explains that it controls where the JWT token is injected into outbound requests.
brianjlai
deleted the
brian/jwt_authenticator_configurable_request_option
branch
What
When first developing the JWT authenticator, based on various specs and usage by connectors, we did not see a use case where the signed JWT token needed to be injected anywhere but under the
Authorizationheader of requests.While working on a Box connector, this is the first usage where we need to load it into the
request_json_bodyunder theassertionkey.How
This PR adds functionality so that the
JwtAuthenticatorcan now specify an optionalrequest_optionto inject the signed token into different request options. Optional so that we retain backwards compatibilityValidation steps
source-google-search-consolewhich use the defaultAuthorizationheader behavior to ensure regressionsSummary by CodeRabbit
New Features
Schema
Parser
Tests
Chores