impl(oauth2): add methods for returning multiple auth related http headers#16064
impl(oauth2): add methods for returning multiple auth related http headers#16064scotthart wants to merge 1 commit intogoogleapis:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the credential authentication system to support multiple HTTP headers, replacing the single AuthenticationHeader with a vector-based AuthenticationHeaders method and adding specific logic for Authorization and AllowedLocations. A review comment suggests that errors from AllowedLocations should be handled more explicitly—either by logging or propagation—to avoid sending requests with missing headers that might cause ambiguous service failures.
| auto allowed_locations = AllowedLocations(tp, endpoint); | ||
| // Not all credential types support the x-allowed-locations header. For those | ||
| // that do, if there is a problem retrieving the header, omit the header. | ||
| if (allowed_locations.ok() && !allowed_locations->empty()) { |
There was a problem hiding this comment.
The current implementation silently ignores any errors returned by AllowedLocations. While the comment suggests this is intentional for credential types that don't support it, an actual Status error (other than OK) from an implementation that does attempt to provide it might indicate a transient failure (e.g., metadata server timeout). Swallowing such errors could lead to requests being sent without required headers, potentially causing confusing 403 or 400 errors from the service. Consider if specific error codes should be propagated or at least logged.
References
- Prefer defensive code, such as explicit ok() checks, even if they seem redundant based on the current implementation of a framework, as the framework's contract may change in the future.
186e65c to
ee8c220
Compare
ee8c220 to
e64f424
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #16064 +/- ##
==========================================
- Coverage 92.70% 92.69% -0.01%
==========================================
Files 2343 2343
Lines 216644 216664 +20
==========================================
+ Hits 200838 200846 +8
- Misses 15806 15818 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This PR adds methods for retrieving the
authorizationandx-allowed-locationsheaders individually, as well as, theAuthenticationHeadersmethod as a single place to get all the auth related headers for RPC calls.