fix: process credentials when custom HTTPClient is provided#276
fix: process credentials when custom HTTPClient is provided#276SoulPancake wants to merge 4 commits intomainfrom
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughDecouples HTTP client defaulting from credentials processing, ensures credential-provided header overrides are always applied, allows OAuth2 to wrap a provided base HTTP client, adds tests for credential scenarios, and updates OpenTelemetry versions in example modules. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related issues
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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. Comment |
Codecov Report❌ Patch coverage is
❌ Your project status has failed because the head coverage (33.61%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #276 +/- ##
==========================================
+ Coverage 33.59% 33.61% +0.01%
==========================================
Files 115 115
Lines 9810 9812 +2
==========================================
+ Hits 3296 3298 +2
Misses 6252 6252
Partials 262 262 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Related Documentation 1 document(s) may need updating based on files changed in this PR: OpenFGA's Space Custom HTTP Headers SupportView Suggested Changes@@ -160,13 +160,11 @@
[Source](https://github.com/openfga/go-sdk/blob/c52ca06c03934b954d50ca34845b73076b6fa9eb/api_client.go#L0-L500)
#### Validation Rules and Reserved Headers
-The Go SDK does not explicitly document reserved headers or validation rules for headers, nor explicit injection prevention mechanisms.
-
----
+The Go SDK does not explicitly document a reserved header list or detailed validation rules for headers. However, authentication headers such as `Authorization` are always set internally by the SDK, regardless of whether a custom HTTPClient is provided. If you provide both a custom HTTPClient and use ClientCredentials authentication, the SDK will override your HTTPClient with an OAuth2-enabled client to ensure proper authentication. Attempts to set reserved headers like `Authorization` in your custom headers will be ignored in favor of the SDK-managed value. There are no explicit header injection prevention mechanisms documented, so avoid including newline characters in header values.
## Go SDK
+
#### Calling Other Endpoints with APIExecutor
-
The Go SDK provides an `APIExecutor` interface for making raw HTTP calls to any OpenFGA endpoint, including endpoints not yet wrapped by the SDK. This is useful if you need to:
- Call a new or experimental endpoint not yet supported by the SDK
@@ -203,11 +201,10 @@
### Best Practices
- Use global default headers for values that should be sent with every request (e.g., application identifiers, telemetry).
- Use per-request headers for values that are specific to a single operation (e.g., request IDs, tracing).
-- Do not attempt to set reserved headers such as `Authorization` or `Content-Type` unless explicitly supported by the SDK.
+- Do not attempt to set reserved headers such as `Authorization` or `Content-Type`; these are always set by the SDK as needed for authentication and protocol compliance.
+- If you provide both a custom HTTPClient and use ClientCredentials authentication, be aware that the SDK will override your HTTPClient with an OAuth2-enabled client to ensure proper authentication. For custom transport settings with OAuth2, configure the transport through the OAuth2 client.
- Avoid including newline characters in header values to prevent header injection vulnerabilities.
- Review SDK documentation and source for updates on reserved headers and validation rules, especially if you need to set headers that may conflict with protocol or SDK-managed headers.
-
----
### Comparison Table
| SDK | Global Headers Config | Per-Request Headers | Header Merging | Reserved Headers | Injection Prevention |Note: You must be authenticated to accept/decline updates. How did I do? Any feedback? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
api_client_test.go (2)
32-89: Consider extracting shared mock setup into a test helper.The httpmock registration and response construction are nearly identical between "ApiToken credentials should be applied with custom HTTPClient" and "Credentials should work when HTTPClient is nil". A small helper (e.g.,
registerAuthModelResponder) would reduce duplication.Also applies to: 120-172
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api_client_test.go` around lines 32 - 89, The two tests ("ApiToken credentials should be applied with custom HTTPClient" and "Credentials should work when HTTPClient is nil") duplicate the httpmock.RegisterResponder and JSON response construction; extract that into a helper function (e.g., registerAuthModelResponder) that accepts the expected URL or storeID and returns/sets the responder for apiClient.OpenFgaApi.ReadAuthorizationModels calls, builds the ReadAuthorizationModelsResponse payload, and exposes a way to assert the Authorization header was observed; replace the duplicated registration blocks in both tests with calls to this helper to reduce duplication and centralize responder logic.
91-118: Test doesn't verify the OAuth2 client is functional — only that the pointer changed.The assertion
configuration.HTTPClient == customHTTPClientconfirms replacement but doesn't validate that the new client has the OAuth2 transport configured. Consider adding an assertion on the transport type (e.g., checking that the client's transport is anoauth2.Transport).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api_client_test.go` around lines 91 - 118, The test currently only checks that configuration.HTTPClient != customHTTPClient; update the test "ClientCredentials should be processed with custom HTTPClient" to also verify the new HTTP client uses an OAuth2 transport by asserting the client's Transport (configuration.HTTPClient.Transport) is of type *oauth2.Transport (or wraps it), and ensure the oauth2 package is imported; use NewAPIClient or the resulting configuration to obtain the HTTPClient and fail the test if the transport is not an oauth2.Transport.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@example/opentelemetry/go.mod`:
- Around line 13-16: Dependency versions for OpenTelemetry modules are
inconsistent: update
go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc,
go.opentelemetry.io/otel/sdk, and go.opentelemetry.io/otel/sdk/metric to v1.40.0
to match go.opentelemetry.io/otel v1.40.0; locate those module lines in go.mod
and change their versions from v1.39.0 to v1.40.0 so all otel modules are
aligned.
---
Nitpick comments:
In `@api_client_test.go`:
- Around line 32-89: The two tests ("ApiToken credentials should be applied with
custom HTTPClient" and "Credentials should work when HTTPClient is nil")
duplicate the httpmock.RegisterResponder and JSON response construction; extract
that into a helper function (e.g., registerAuthModelResponder) that accepts the
expected URL or storeID and returns/sets the responder for
apiClient.OpenFgaApi.ReadAuthorizationModels calls, builds the
ReadAuthorizationModelsResponse payload, and exposes a way to assert the
Authorization header was observed; replace the duplicated registration blocks in
both tests with calls to this helper to reduce duplication and centralize
responder logic.
- Around line 91-118: The test currently only checks that
configuration.HTTPClient != customHTTPClient; update the test "ClientCredentials
should be processed with custom HTTPClient" to also verify the new HTTP client
uses an OAuth2 transport by asserting the client's Transport
(configuration.HTTPClient.Transport) is of type *oauth2.Transport (or wraps it),
and ensure the oauth2 package is imported; use NewAPIClient or the resulting
configuration to obtain the HTTPClient and fail the test if the transport is not
an oauth2.Transport.
There was a problem hiding this comment.
Pull request overview
This PR attempts to fix issue #234 where credentials were being ignored when a custom HTTPClient was provided. The original bug was that credentials processing was only done when cfg.HTTPClient == nil, preventing users from combining custom HTTP client settings (like timeouts, transports) with authentication.
Changes:
- Refactored credential processing to always execute regardless of whether a custom HTTPClient is provided
- Added logic to handle conflicts between custom HTTPClient and OAuth2-enabled clients for ClientCredentials
- Added comprehensive test coverage for ApiToken and ClientCredentials authentication with custom HTTPClient
- Updated OpenTelemetry dependencies from v1.39.0 to v1.40.0 in example modules
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| api_client.go | Restructured NewAPIClient to always process credentials and handle HTTPClient conflicts |
| api_client_test.go | Added tests for ApiToken and ClientCredentials with custom HTTPClient scenarios |
| CHANGELOG.md | Added entry for the fix |
| example/*/go.mod | Updated OpenTelemetry dependencies to v1.40.0 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Actually, Instead of replacing the custom HTTPClient with an OAuth2 client, we can inject the custom client into the OAuth2 context essentially honoring both |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (3)
credentials/credentials.go (1)
104-148: Sentinel-value coupling betweenGetHttpClientAndHeaderOverridesand its caller is fragile.Returning
http.DefaultClientas a sentinel to signal "no OAuth2 client was created" (line 106) works because the caller inapi_client.go(line 82) checkshttpClient != http.DefaultClient. However, this creates an implicit contract: if anyone changes the default on either side, the logic silently breaks.Consider returning
nilwhen no OAuth2 client is produced (forApiToken/Nonebranches) and having the caller check fornilinstead. This makes the "no client override" semantic explicit.Proposed diff
func (c *Credentials) GetHttpClientAndHeaderOverrides(retryParams retryutils.RetryParams, debug bool, baseClient *http.Client) (*http.Client, []*HeaderParams) { var headers []*HeaderParams - var client = http.DefaultClient + var client *http.Client switch c.Method { case CredentialsMethodClientCredentials: ...And in
api_client.go:- if httpClient != nil && httpClient != http.DefaultClient { + if httpClient != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@credentials/credentials.go` around lines 104 - 148, The function GetHttpClientAndHeaderOverrides currently uses http.DefaultClient as a sentinel to indicate "no OAuth2 client"; change this to return nil when no OAuth2 client is created (i.e., for CredentialsMethodApiToken and CredentialsMethodNone) and only set the client variable for the CredentialsMethodClientCredentials branch (where you call ccConfig.Client(ctx)); then update the caller in api_client.go to check for a nil httpClient instead of comparing to http.DefaultClient so the "no client override" semantic is explicit and not tied to the default client value.api_client_test.go (2)
227-280: Same observation: good scenario but could assert transport chain integrity.This test sets an explicit
customTransport(line 230–234) and is the ideal place to verify the OAuth2 client actually wraps it, since the custom transport is non-nil here. Without that assertion, the test only proves the pointer changed, not that the fix is functioning as intended.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api_client_test.go` around lines 227 - 280, Add an assertion to verify the OAuth2 client actually wraps the provided customTransport (not just that the HTTPClient pointer changed). After creating configuration via NewConfiguration and apiClient via NewAPIClient, unwrap configuration.HTTPClient/ apiClient.GetConfig().HTTPClient.Transport (e.g., type-assert to *oauth2.Transport and check its Base field, or iterate RoundTripper wrappers) and assert that the final/base transport equals customTransport; fail the test if the wrapped/base transport is not customTransport. Use symbols NewConfiguration, NewAPIClient, apiClient.GetConfig().HTTPClient, and customTransport to locate where to add this check.
96-134: ClientCredentials wrapping test only verifies pointer inequality, not that the custom transport is actually in the chain.The test confirms the HTTP client was replaced but doesn't verify the custom client's transport is preserved as the base of the OAuth2 transport chain. Consider asserting that the resulting client's transport's
Baseis the custom transport — this would confirm the wrapping behavior that the PR aims to guarantee.Example assertion (after existing checks)
// Verify the custom transport is wrapped inside the OAuth2 transport if oauthTransport, ok := configuration.HTTPClient.Transport.(*oauth2.Transport); ok { if oauthTransport.Base != customHTTPClient.Transport { // For nil transport, Base may also be nil (defaults to http.DefaultTransport) if customHTTPClient.Transport != nil { t.Error("OAuth2 transport should wrap the custom client's transport") } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api_client_test.go` around lines 96 - 134, Update the test that constructs Configuration via NewConfiguration (using the customHTTPClient) to assert that the OAuth2 transport actually wraps the custom client's transport: after creating apiClient and verifying configuration.HTTPClient changed, cast configuration.HTTPClient.Transport to *oauth2.Transport (reference oauthTransport) and check that oauthTransport.Base equals customHTTPClient.Transport, accounting for the case where customHTTPClient.Transport is nil (which implies Base may be nil or http.DefaultTransport); if the wrapped Base does not match the custom transport (and the custom transport was non-nil) fail the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@api_client_test.go`:
- Around line 227-280: Add an assertion to verify the OAuth2 client actually
wraps the provided customTransport (not just that the HTTPClient pointer
changed). After creating configuration via NewConfiguration and apiClient via
NewAPIClient, unwrap configuration.HTTPClient/
apiClient.GetConfig().HTTPClient.Transport (e.g., type-assert to
*oauth2.Transport and check its Base field, or iterate RoundTripper wrappers)
and assert that the final/base transport equals customTransport; fail the test
if the wrapped/base transport is not customTransport. Use symbols
NewConfiguration, NewAPIClient, apiClient.GetConfig().HTTPClient, and
customTransport to locate where to add this check.
- Around line 96-134: Update the test that constructs Configuration via
NewConfiguration (using the customHTTPClient) to assert that the OAuth2
transport actually wraps the custom client's transport: after creating apiClient
and verifying configuration.HTTPClient changed, cast
configuration.HTTPClient.Transport to *oauth2.Transport (reference
oauthTransport) and check that oauthTransport.Base equals
customHTTPClient.Transport, accounting for the case where
customHTTPClient.Transport is nil (which implies Base may be nil or
http.DefaultTransport); if the wrapped Base does not match the custom transport
(and the custom transport was non-nil) fail the test.
In `@credentials/credentials.go`:
- Around line 104-148: The function GetHttpClientAndHeaderOverrides currently
uses http.DefaultClient as a sentinel to indicate "no OAuth2 client"; change
this to return nil when no OAuth2 client is created (i.e., for
CredentialsMethodApiToken and CredentialsMethodNone) and only set the client
variable for the CredentialsMethodClientCredentials branch (where you call
ccConfig.Client(ctx)); then update the caller in api_client.go to check for a
nil httpClient instead of comparing to http.DefaultClient so the "no client
override" semantic is explicit and not tied to the default client value.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
2 participants
Description
For issue #234
Honors both the custom client and credentials
What problem is being solved?
How is it being solved?
What changes are made to solve it?
References
Review Checklist
mainSummary by CodeRabbit
Bug Fixes
Tests
Chores
Documentation