fix(client): apply all schemes in multi-scheme security requirement (AND semantics) - #1140
fix(client): apply all schemes in multi-scheme security requirement (AND semantics)#1140arunmm8335 wants to merge 1 commit into
Conversation
…AND semantics) AuthInterceptor.before() previously returned after applying the first credential, even when a SecurityRequirement listed multiple schemes that must all be satisfied together (AND semantics per OpenAPI spec). This change introduces a two-pass approach per requirement: - Pass 1: collect credentials for every scheme; if any is unavailable or unsupported, mark the requirement as unsatisfiable and skip it. - Pass 2: apply all collected credentials at once, then return. The outer loop over security_requirements retains OR semantics: the first fully satisfiable requirement is used. A _resolve_header() static helper was extracted to map each scheme type to its (header_name, value) pair, improving readability and keeping the collection loop clean. Added 5 new test cases covering: - Multi-scheme AND (the exact issue reproduction) - Multi-requirement OR fallback - Partial requirement prevention - Mixed Bearer + API key in a single requirement - Unsupported scheme type (query API key) fallback Fixes a2aproject#1138
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the AuthInterceptor to correctly implement OpenAPI Security Requirement semantics, utilizing a two-pass approach to handle AND semantics within a single requirement and OR semantics across multiple requirements. It also extracts header resolution into a helper method and adds comprehensive unit tests to verify these behaviors. There are no review comments, so I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
🧪 Code Coverage (vs
|
| Base | PR | Delta | |
|---|---|---|---|
| src/a2a/client/auth/interceptor.py | 85.42% | 98.51% | 🟢 +13.09% |
| Total | 92.95% | 93.04% | 🟢 +0.08% |
Generated by coverage-comment.yml
| Follows OpenAPI Security Requirement semantics: | ||
| - The outer ``security_requirements`` list uses **OR** semantics: | ||
| satisfying any single requirement is sufficient. | ||
| - Multiple schemes **within** a single requirement use **AND** | ||
| semantics: all of them must be satisfied together. | ||
|
|
||
| A two-pass approach is used per requirement to avoid partial | ||
| application when one scheme's credential is unavailable. |
There was a problem hiding this comment.
Let's remove implementation details and just keep high level description to not clutter the code with comments.
There was a problem hiding this comment.
Yes sure will do that could you please review it and assign this issue to me
There was a problem hiding this comment.
Thank you! I submitted the review.
| # Pass 1: collect credentials for every scheme in this | ||
| # requirement. If any scheme cannot be satisfied the whole | ||
| # requirement is skipped (AND semantics). |
There was a problem hiding this comment.
This detailed description is not needed, it adds unnecessary clutter, the behaviour from the description in visible in the next few lines of code.
| if not satisfiable: | ||
| continue # OR: try the next requirement | ||
|
|
||
| # Pass 2: apply all collected credentials at once. |
| async def test_multi_scheme_and_semantics() -> None: | ||
| """A single requirement with two API-key-in-header schemes must apply both headers. | ||
|
|
||
| This is the exact reproduction from issue #1138. |
| # --------------------------------------------------------------------------- | ||
| # Tests for multi-scheme AND / multi-requirement OR semantics (#1138) | ||
| # --------------------------------------------------------------------------- |
| class DictCredentialService: | ||
| """A simple credential service backed by a plain dict.""" | ||
|
|
||
| def __init__(self, creds: dict[str, str]) -> None: | ||
| self._creds = creds | ||
|
|
||
| async def get_credentials( | ||
| self, | ||
| security_scheme_name: str, | ||
| context: ClientCallContext | None, | ||
| ) -> str | None: | ||
| return self._creds.get(security_scheme_name) | ||
|
|
||
|
|
||
| def _make_agent_card( | ||
| *, | ||
| security_schemes: dict[str, SecurityScheme], | ||
| security_requirements: list[SecurityRequirement], | ||
| ) -> AgentCard: | ||
| """Helper to build a minimal AgentCard with security configuration.""" | ||
| return AgentCard( | ||
| name='testbot', | ||
| description='test', | ||
| version='1.0', | ||
| default_input_modes=[], | ||
| default_output_modes=[], | ||
| skills=[], | ||
| capabilities=AgentCapabilities(), | ||
| security_schemes=security_schemes, | ||
| security_requirements=security_requirements, | ||
| ) |
There was a problem hiding this comment.
Let's follow the test design and put helper functions and classes at the top of the file.
|
Verified the security-correctness of this against current main. On main, AuthInterceptor.before iterates That is the part worth stating precisely: for an AND requirement such as [bearer, apiKeyHeader], the old single-return path emits Authorization and drops the API key, producing a request that is authenticated for one factor and silently missing the other. This PR's collect-then-apply design fixes exactly that: pass one gathers a header for every scheme in the requirement and marks the requirement unsatisfiable on any miss, pass two applies them together, and it returns after the first fully satisfiable requirement. That gives AND within a requirement and OR across requirements, and it guarantees a requirement is applied whole or skipped whole. No partial application. That is the right property. One behavior change to call out in the notes so integrators are not surprised: unsupported API-key placements (query and cookie) now make the whole requirement unsatisfiable and cause a fallthrough to the next requirement, where main silently skipped only that one scheme and kept the rest. That is more correct for AND (no half-authenticated request), but it is a real change from prior behavior and worth a changelog line. CI is green across 3.10 to 3.14, the new tests cover multi-scheme AND, OR fallback and partial-credential skip, and the only outstanding review comments look stylistic. The security semantics are sound. |
3 participants
Fixes #1138