implementing authorization code flow using zitadel library#92
Open
aaishwarya6694 wants to merge 2 commits intomainfrom
Open
implementing authorization code flow using zitadel library#92aaishwarya6694 wants to merge 2 commits intomainfrom
aaishwarya6694 wants to merge 2 commits intomainfrom
Conversation
aaishwarya6694
force-pushed
the
task/implement-zitadel-lib
branch
from
e138ac6 to
70ae615
Compare
aaishwarya6694
force-pushed
the
task/implement-zitadel-lib
branch
from
37b50a2 to
c41d89e
Compare
aaishwarya6694
requested review from
alienvspredator,
cb80 and
pabloszel11
and removed request for
alienvspredator and
pabloszel11
pabloszel11
requested changes
Nov 13, 2025
|
Contributor
|
Could you rebase the branch on the latest main? |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaced previous OIDC config usage with Zitadel OIDC v3 discovery types; getOpenIDConfig now performs direct discovery with issuer URL parsing/validation and explicit HTTP client selection; go.mod updated to add Zitadel OIDC and related indirect dependencies. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Manager
participant Cache
participant HTTPClient as HTTP Client
participant Discover as Zitadel Discover
Caller->>Manager: request OpenID config (issuer)
Manager->>Cache: lookup discovery config (issuer)
alt cache miss
Manager->>HTTPClient: choose http.Client (m.newCreds("").Transport())
Manager->>Discover: client.Discover(ctx, issuerURL, HTTPClient)
Discover-->>Manager: DiscoveryConfiguration / error
Manager->>Cache: store DiscoveryConfiguration
end
Manager-->>Caller: return DiscoveryConfiguration / error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Poem
🚥 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. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/session/manager.go (2)
186-189:⚠️ Potential issue | 🟠 MajorMissing
resp.Body.Close()call.The response body is decoded but never closed, which will leak connections over time. Compare with
exchangeCode(line 618) which properly defers closing the body.🔧 Proposed fix to close response body
resp, err := http.DefaultClient.Do(req) if err != nil { return nil, fmt.Errorf("executing an http request: %w", err) } + defer resp.Body.Close() err = json.NewDecoder(resp.Body).Decode(&keySet)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/session/manager.go` around lines 186 - 189, The JSON decode of resp.Body in manager.go uses json.NewDecoder(resp.Body).Decode(&keySet) but never closes resp.Body; add a defer resp.Body.Close() immediately after obtaining and validating resp (before decoding) so the HTTP connection is released (mirror the pattern used in exchangeCode). Ensure resp.Body.Close() is deferred even when Decode returns an error.
173-192:⚠️ Potential issue | 🟠 MajorInconsistent HTTP client usage: uses
http.DefaultClientinstead ofm.secureClient.This function uses
http.DefaultClient(line 181) to fetch the JWKS, but theexchangeCodefunction correctly usesm.secureClientfor token exchange, andopenid.goalso properly usesm.secureClientfor discovery. This inconsistency could bypass any custom TLS configurations, timeouts, or security policies configured onsecureClient.🔒 Proposed fix to use secureClient consistently
func (m *Manager) getProviderKeySet(ctx context.Context, oidcConf *zitadeloidc.DiscoveryConfiguration) (*jose.JSONWebKeySet, error) { var keySet jose.JSONWebKeySet uri := oidcConf.JwksURI req, err := http.NewRequestWithContext(ctx, http.MethodGet, uri, nil) if err != nil { return nil, fmt.Errorf("creating a new HTTP request: %w", err) } - resp, err := http.DefaultClient.Do(req) + httpClient := m.secureClient + if httpClient == nil { + httpClient = http.DefaultClient + } + resp, err := httpClient.Do(req) if err != nil { return nil, fmt.Errorf("executing an http request: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/session/manager.go` around lines 173 - 192, The getProviderKeySet function is using http.DefaultClient which bypasses the Manager's configured m.secureClient; update getProviderKeySet to use m.secureClient for the JWKS HTTP request and response handling (mirror how exchangeCode and discovery in openid.go use m.secureClient), ensure the request is created with http.NewRequestWithContext(ctx, ...) and executed via m.secureClient.Do(req), and handle/close resp.Body and propagate errors unchanged.
🧹 Nitpick comments (1)
internal/session/openid.go (1)
15-24: Good security enhancement with HTTP scheme validation.The URL parsing and explicit rejection of insecure
httpschemes (unless explicitly allowed) is a solid security improvement.One minor refinement per static analysis: line 23 can use
errors.Newsince there's no format argument.✨ Optional: Use errors.New for static error message
+import "errors" + func (m *Manager) getOpenIDConfig(ctx context.Context, issuerURL string) (*oidc.DiscoveryConfiguration, error) { ... if issuer.Scheme == "http" && !m.allowHttpScheme { - return nil, fmt.Errorf("insecure http issuer url is not allowed") + return nil, errors.New("insecure http issuer url is not allowed") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/session/openid.go` around lines 15 - 24, In getOpenIDConfig, replace the fmt.Errorf call that returns the static message "insecure http issuer url is not allowed" with errors.New and ensure the errors package is imported; specifically update the logic around Manager.allowHttpScheme / issuerURL parsing in getOpenIDConfig to return errors.New("insecure http issuer url is not allowed") instead of fmt.Errorf(...) since there are no formatting directives.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/session/manager.go`:
- Around line 186-189: The JSON decode of resp.Body in manager.go uses
json.NewDecoder(resp.Body).Decode(&keySet) but never closes resp.Body; add a
defer resp.Body.Close() immediately after obtaining and validating resp (before
decoding) so the HTTP connection is released (mirror the pattern used in
exchangeCode). Ensure resp.Body.Close() is deferred even when Decode returns an
error.
- Around line 173-192: The getProviderKeySet function is using
http.DefaultClient which bypasses the Manager's configured m.secureClient;
update getProviderKeySet to use m.secureClient for the JWKS HTTP request and
response handling (mirror how exchangeCode and discovery in openid.go use
m.secureClient), ensure the request is created with
http.NewRequestWithContext(ctx, ...) and executed via m.secureClient.Do(req),
and handle/close resp.Body and propagate errors unchanged.
---
Nitpick comments:
In `@internal/session/openid.go`:
- Around line 15-24: In getOpenIDConfig, replace the fmt.Errorf call that
returns the static message "insecure http issuer url is not allowed" with
errors.New and ensure the errors package is imported; specifically update the
logic around Manager.allowHttpScheme / issuerURL parsing in getOpenIDConfig to
return errors.New("insecure http issuer url is not allowed") instead of
fmt.Errorf(...) since there are no formatting directives.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ea75819c-e649-4684-9ba2-bb21e7213a5b
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.modinternal/session/manager.gointernal/session/openid.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/session/manager.go (2)
615-623:⚠️ Potential issue | 🟠 MajorAdd an explicit timeout for token exchange requests.
This is an auth-critical external call; without a deadline it can hang request handling under provider/network issues.
⏱️ Proposed fix
- req, err := http.NewRequestWithContext(ctx, http.MethodPost, openidConf.TokenEndpoint, strings.NewReader(data.Encode())) + reqCtx, cancel := context.WithTimeout(ctx, 10*time.Second) + defer cancel() + req, err := http.NewRequestWithContext(reqCtx, http.MethodPost, openidConf.TokenEndpoint, strings.NewReader(data.Encode())) if err != nil { return tokenResponse{}, fmt.Errorf("creating request: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/session/manager.go` around lines 615 - 623, The token exchange request can hang because there's no deadline; wrap the request in a cancellable timeout: create a child context with context.WithTimeout(ctx, <reasonableDuration>) and defer cancel, use that timed context when building the request (replace http.NewRequestWithContext(ctx, ...) with the new context) and then call client.Do(req) as before; alternatively ensure the HTTP client returned by m.httpClient(mapping) has a non-zero Timeout, but prefer adding context.WithTimeout around the token exchange to guarantee per-request deadlines for the code paths around NewRequestWithContext / m.httpClient(mapping) / client.Do.
165-181:⚠️ Potential issue | 🟠 MajorHarden JWKS fetch: avoid default client, check status, and close response body.
Current JWKS retrieval can leak connections and may fail in environments needing custom transport settings.
🔧 Proposed fix
-func (m *Manager) getProviderKeySet(ctx context.Context, oidcConf *zitadeloidc.DiscoveryConfiguration) (*jose.JSONWebKeySet, error) { +func (m *Manager) getProviderKeySet(ctx context.Context, oidcConf *zitadeloidc.DiscoveryConfiguration, mapping trust.OIDCMapping) (*jose.JSONWebKeySet, error) { var keySet jose.JSONWebKeySet uri := oidcConf.JwksURI req, err := http.NewRequestWithContext(ctx, http.MethodGet, uri, nil) if err != nil { return nil, fmt.Errorf("creating a new HTTP request: %w", err) } - resp, err := http.DefaultClient.Do(req) + resp, err := m.httpClient(mapping).Do(req) if err != nil { return nil, fmt.Errorf("executing an http request: %w", err) } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + return nil, fmt.Errorf("fetching jwks failed with status: %d", resp.StatusCode) + } err = json.NewDecoder(resp.Body).Decode(&keySet) if err != nil { return nil, fmt.Errorf("decoding keyset response: %w", err) }Also update call sites in this file:
m.getProviderKeySet(ctx, openidConf, mapping)inFinaliseOIDCLoginm.getProviderKeySet(ctx, oidcConf, mapping)inBCLogout🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/session/manager.go` around lines 165 - 181, Replace use of http.DefaultClient and the unclosed response with a safe fetch: create or use a configurable http.Client (e.g., a client with a timeout or m.httpClient if available), perform the request with req.Context(), defer resp.Body.Close() immediately after checking resp != nil, verify resp.StatusCode is 200 (or in 2xx) and return a descriptive error including status and response body when not OK, and keep decoding into keySet only after those checks; update call sites FinaliseOIDCLogin and BCLogout to call the revised getProviderKeySet signature/behavior if you introduce a client dependency or signature change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/session/manager.go`:
- Around line 615-623: The token exchange request can hang because there's no
deadline; wrap the request in a cancellable timeout: create a child context with
context.WithTimeout(ctx, <reasonableDuration>) and defer cancel, use that timed
context when building the request (replace http.NewRequestWithContext(ctx, ...)
with the new context) and then call client.Do(req) as before; alternatively
ensure the HTTP client returned by m.httpClient(mapping) has a non-zero Timeout,
but prefer adding context.WithTimeout around the token exchange to guarantee
per-request deadlines for the code paths around NewRequestWithContext /
m.httpClient(mapping) / client.Do.
- Around line 165-181: Replace use of http.DefaultClient and the unclosed
response with a safe fetch: create or use a configurable http.Client (e.g., a
client with a timeout or m.httpClient if available), perform the request with
req.Context(), defer resp.Body.Close() immediately after checking resp != nil,
verify resp.StatusCode is 200 (or in 2xx) and return a descriptive error
including status and response body when not OK, and keep decoding into keySet
only after those checks; update call sites FinaliseOIDCLogin and BCLogout to
call the revised getProviderKeySet signature/behavior if you introduce a client
dependency or signature change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 23f7be2c-8d3d-4343-91af-72fc789c1be2
📒 Files selected for processing (1)
internal/session/manager.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/session/openid.go`:
- Around line 22-24: The check that returns a static error when an insecure HTTP
issuer is used (in the block referencing issuer.Scheme and m.allowHttpScheme)
should use errors.New instead of fmt.Errorf; add the errors import to the file
and replace the fmt.Errorf("insecure http issuer url is not allowed") call with
errors.New("insecure http issuer url is not allowed") in the function that
performs the issuer.Scheme check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cbe4f0b0-c1f8-4e80-a9b9-9c3d25747fde
📒 Files selected for processing (1)
internal/session/openid.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/session/manager.go (2)
3-31:⚠️ Potential issue | 🟠 MajorFix import grouping/formatting to unblock CI.
The quality pipeline is failing on this file (
gci) at Line 23. Please run the project’s import formatter (gci/format target) on this file before merge.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/session/manager.go` around lines 3 - 31, The import block is misformatted and failing the gci check; run the project's import formatter (gci/format target) to reorder and group imports correctly in the import block that contains packages like "context", "github.com/go-jose/go-jose/v4", "github.com/openkcm/session-manager/internal/config", "github.com/openkcm/session-manager/internal/credentials" and others so the file passes CI; ensure standard library, third-party, and internal imports are grouped per repo style and then commit the changes.
177-195:⚠️ Potential issue | 🔴 CriticalHandle JWKS HTTP response safely before decoding.
At Line 190,
resp.Bodyis decoded withoutClose(), and non-200 statuses are not checked. This can leak HTTP connections and mask upstream JWKS errors.Suggested fix
func (m *Manager) getProviderKeySet(ctx context.Context, oidcConf *zitadeloidc.DiscoveryConfiguration) (*jose.JSONWebKeySet, error) { var keySet jose.JSONWebKeySet uri := oidcConf.JwksURI req, err := http.NewRequestWithContext(ctx, http.MethodGet, uri, nil) if err != nil { return nil, fmt.Errorf("creating a new HTTP request: %w", err) } resp, err := http.DefaultClient.Do(req) if err != nil { return nil, fmt.Errorf("executing an http request: %w", err) } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + return nil, fmt.Errorf("fetching jwks: unexpected status %d", resp.StatusCode) + } err = json.NewDecoder(resp.Body).Decode(&keySet) if err != nil { return nil, fmt.Errorf("decoding keyset response: %w", err) } return &keySet, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/session/manager.go` around lines 177 - 195, In getProviderKeySet, ensure the HTTP response is handled safely by deferring resp.Body.Close() immediately after a successful http.DefaultClient.Do(req) and by checking resp.StatusCode for non-2xx responses before decoding; for non-OK statuses read (or io.ReadAll) the body into a string and return a descriptive error (including status and body) instead of attempting json.Decode on a bad response, then only json.NewDecoder(resp.Body).Decode(&keySet) for 2xx responses so connections aren’t leaked and upstream JWKS errors aren’t masked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/session/manager.go`:
- Around line 3-31: The import block is misformatted and failing the gci check;
run the project's import formatter (gci/format target) to reorder and group
imports correctly in the import block that contains packages like "context",
"github.com/go-jose/go-jose/v4",
"github.com/openkcm/session-manager/internal/config",
"github.com/openkcm/session-manager/internal/credentials" and others so the file
passes CI; ensure standard library, third-party, and internal imports are
grouped per repo style and then commit the changes.
- Around line 177-195: In getProviderKeySet, ensure the HTTP response is handled
safely by deferring resp.Body.Close() immediately after a successful
http.DefaultClient.Do(req) and by checking resp.StatusCode for non-2xx responses
before decoding; for non-OK statuses read (or io.ReadAll) the body into a string
and return a descriptive error (including status and body) instead of attempting
json.Decode on a bad response, then only
json.NewDecoder(resp.Body).Decode(&keySet) for 2xx responses so connections
aren’t leaked and upstream JWKS errors aren’t masked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 506173ba-b5cd-43f4-b467-790d99f60948
📒 Files selected for processing (1)
internal/session/manager.go
aaishwarya6694
force-pushed
the
task/implement-zitadel-lib
branch
from
b9d0f71 to
c49d727
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Chores
Bug Fixes