feat(modules/dex): add Dex OIDC provider module#3659
Conversation
Upstream-merge-grade module with: Run() + endpoint getters, functional options (WithClient, WithUser, WithConnector, WithIssuer, WithSkipApprovalScreen, WithStorage, WithLogger, WithLogLevel, WithEnableClientCredentials), gRPC admin mutation (AddClient, RemoveClient, AddUser, RemoveUser), slog-backed log consumer, password DB + mockCallback connectors, and feature-flag support for the client_credentials grant. 36 tests cover YAML rendering, container lifecycle, gRPC mutation, auth_code + refresh + multi-redirect, ROPC, multi-client, mock connector, cross-container reachability via network alias, and consumer JWT verification via coreos/go-oidc. Monorepo origin: github.com/guilycst/totvs-work PR testcontainers#19 (merge commit 0764128de).
… distribution While the module incubates on the fork before upstream merge, consumers import it at github.com/guilycst/testcontainers-go/modules/dex so Go can resolve the package from the fork repo without a replace directive. Reverts to github.com/testcontainers/testcontainers-go/modules/dex once upstreamed.
…factor, constructors, validation, and logger support
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
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:
WalkthroughAdds a new Testcontainers Go module for Dex (OIDC provider): exported types, option API, config renderer, container runner, gRPC admin client, logging consumer, docs, Make/VSCode/Dependabot entries, examples, and extensive unit/integration tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant Run as dex.Run()
participant Config as config.render()
participant Container as DexContainer
participant Dex as Dex Server
participant Wait as Readiness Check
Test->>Run: Run(ctx, image, opts...)
Run->>Config: render(options)
Config->>Config: validate issuer & auth sources
Config->>Config: bcrypt-hash users & marshal YAML
Config-->>Run: dex.yml
Run->>Container: create container, copy dex.yml, set env (client_credentials flag if enabled)
Container->>Dex: start
Run->>Wait: poll discovery endpoint & dial gRPC
Wait->>Dex: GET /.well-known/openid-configuration
Wait->>Dex: Dial gRPC admin port
Dex-->>Wait: ready
Wait-->>Run: healthy
Run-->>Test: return *DexContainer
sequenceDiagram
participant Test
participant DexC as DexContainer
participant OAuth as oauth2.Config
participant Dex as Dex Provider
participant Token as Token Endpoint
Test->>DexC: IssuerURL() / AuthEndpoint() / TokenEndpoint()
DexC-->>Test: URLs
Test->>OAuth: build auth URL
OAuth-->>Test: redirect to Dex /auth
Test->>Dex: GET /auth (login)
Dex-->>Test: login form (password or mock)
Test->>Dex: POST credentials
Dex-->>Test: redirect with code to configured redirect URI
Test->>OAuth: Exchange(ctx, code)
OAuth->>Token: POST /token
Token-->>OAuth: id_token + access_token
OAuth-->>Test: tokens
Test->>Test: verify ID token claims (using OIDC verifier)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 review |
|
Only users with a collaborator, contributor, member, or owner role can interact with CodeRabbit. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (16)
modules/dex/dextest_helpers_test.go (1)
45-65: Nit: preferhttp.MethodGet/http.MethodPostover string literals.Idiomatic Go uses the
http.Method*constants innet/httpfor request methods.Proposed diff
- req, err := http.NewRequestWithContext(ctx, "GET", authURL, nil) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, authURL, nil) @@ - postReq, err := http.NewRequestWithContext(ctx, "POST", actionURL.String(), strings.NewReader(form.Encode())) + postReq, err := http.NewRequestWithContext(ctx, http.MethodPost, actionURL.String(), strings.NewReader(form.Encode()))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/dex/dextest_helpers_test.go` around lines 45 - 65, Replace the string literals "GET" and "POST" passed to http.NewRequestWithContext with the net/http constants http.MethodGet and http.MethodPost respectively; locate the calls to http.NewRequestWithContext that create req and postReq in dextest_helpers_test.go and update their method arguments to use http.MethodGet and http.MethodPost to follow idiomatic Go usage.modules/dex/options.go (4)
147-152:WithLogger(nil)clobbers a previously-set logger.Because options are applied sequentially from the slice, calling
WithLogger(logger)followed byWithLogger(nil)will unset the logger silently (anddex.gothen skipsLogConsumersbecausesettings.logger != nilis false). That matches the godoc ("When nil or not called, Dex container logs are discarded"), but it makes the nil argument a destructive action rather than a no-op. If you prefer the no-op reading, guard withif logger != nil { o.logger = logger }. Otherwise leave as-is and call out in the doc that order-dependent overrides apply.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/dex/options.go` around lines 147 - 152, The WithLogger option currently unconditionally assigns o.logger, so calling WithLogger(nil) will unset a previously-set logger; update the WithLogger function (the Option returned by WithLogger) to only assign o.logger when the provided logger argument is non-nil (guard with if logger != nil { ... }) so passing nil becomes a no-op; reference the WithLogger function and the options struct (o.logger) when making this change, or alternatively document that WithLogger(nil) intentionally overrides earlier values if you decide to keep the current behavior.
86-100:WithConnector(ConnectorPassword, …)swallows blank id/name silently.The short-circuit on Line 88–89 returns before the blank-id/name checks, so
WithConnector(ConnectorPassword, "", "")succeeds (as the test onoptions_test.go:61confirms). That's probably intentional — the password DB is auto-configured elsewhere — but a caller who expects the blank-field guard to apply uniformly may be surprised. If the behavior is deliberate, consider a one-line godoc addition: "ForConnectorPassword, id/name are ignored and validation is skipped." Otherwise, move the blank checks above the early return.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/dex/options.go` around lines 86 - 100, Update the behavior documentation for WithConnector to clarify that when t == ConnectorPassword the id/name parameters are ignored and validation is intentionally skipped; add a one-line Godoc comment above the WithConnector function stating "For ConnectorPassword, id and name are ignored and blank-field validation is skipped." This makes the early return (if t == ConnectorPassword) explicit to callers without changing the existing logic used in options_test.go.
40-54: Customize no-op meansOptioncan't be forwarded through generic tc-go helpers.Any wrapper that only calls
Customizeon customizers (rather than type-asserting toOption) will silently drop your settings —Runis the only place that actually appliesOptionfunctions. This is documented ("Real state mutation happens in Run"), but a user who reasonably passesdex.WithClient(...)to, say, a network-level customization pipeline that re-emits customizers viaCustomizewill get zero error and zero effect. Consider at minimum adding a short godocNOTE:that Options must be passed directly todex.Run, not to arbitrary tc-go helpers. Low priority, more of a trap than a bug.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/dex/options.go` around lines 40 - 54, The current Option type implements Customize as a no-op (Option.Customize) so any generic testcontainers helper that only calls Customize will silently drop dex Options; update the documentation and API surface to prevent this trap by adding a clear godoc NOTE on the Option type (and/or Customize) that states Options (e.g., dex.WithClient) must be passed directly to dex.Run and not forwarded through generic tc-go customizer pipelines, and include a short example or sentence indicating that real mutation happens only in Run; reference Option, Customize, Run and a typical Option helper like WithClient so callers can find and apply the guidance.
164-177: Module doesn't validate image tag — fragile for users.Per PR objectives and README,
WithEnableClientCredentials()silently yields a runtimeunsupported_grant_typeerror on Dex < v2.46.0. The option sets an env var unconditionally, so users on v2.45.x get no compile/runtime hint that they picked an incompatible image. A best-effort warning when Dex's discovery doc doesn't advertiseclient_credentialsingrant_types_supportedwould catch this early — but that crosses into Run/lifecycle territory and is reasonably deferred given the known-limitation caveat in the docs.modules/dex/config.go (1)
187-197:newUUIDv4panics on randomness failure in a container-startup path.
renderis invoked from thePostStarthook (permodules/dex/dex.go), andAddUseruses the same helper on the request goroutine (modules/dex/grpc.go:96). A panic oncrypto/rand.Readfailure propagates as an unrecoverable goroutine crash rather than a wrapped error the caller can retry. Returning(string, error)and surfacing viafmt.Errorf("dex: read randomness: %w", err)would keep error handling uniform with the rest of the module (both call sites already return errors). Failure is vanishingly rare on modern kernels, so treat this as cleanup rather than a bug.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/dex/config.go` around lines 187 - 197, The function newUUIDv4 currently panics on crypto/rand.Read failure; change its signature to newUUIDv4() (string, error) and return an empty string plus fmt.Errorf("dex: read randomness: %w", err) on read failure instead of panicking, and on success return the UUID string with nil error; update callers (render and AddUser) to handle the error return (propagate it up or wrap it where appropriate) so failures become recoverable errors rather than panics.modules/dex/dex_test.go (4)
538-547: Ignored error fromcookiejar.New.
cookiejar.New(nil)never returns a non-nil error today, but ignoring it here is inconsistent with the rest of the test file, which usesrequire.NoErroron every call. Swapping inrequire.NoError(t, err)(ort.Fatal) keeps the pattern uniform and guards against future signature changes.🧹 Proposed fix
- jar, _ := cookiejar.New(nil) + jar, err := cookiejar.New(nil) + require.NoError(t, err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/dex/dex_test.go` around lines 538 - 547, The call to cookiejar.New ignores its error; change jar, _ := cookiejar.New(nil) to capture the error (jar, err := cookiejar.New(nil)) and assert no error using require.NoError(t, err) before using jar so the test follows the file's pattern and will fail if the constructor signature changes; update any downstream usage in the test that references jar if needed.
198-212: Log-capture poll window may flake under load.The 5 s
deadlinestarts running the instantRunreturns, butPostStart's readiness probe can satisfy HTTP/gRPC probes before Dex's "listening on" line has propagated all the way through theLogConsumergoroutine (especially on slow/loaded CI runners). Consider bumping this to 30 s (aligned with the in-flight startup timeouts index.go) or plumbing a channel-based signal from the consumer instead of polling a string — otherwise any slowdown surfaces as a flake rather than a real bug.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/dex/dex_test.go` around lines 198 - 212, The test's 5s polling deadline (deadline := time.Now().Add(5 * time.Second)) can flake under load because Run returns before the LogConsumer goroutine has propagated the "listening on" line; change the deadline to 30s (e.g., time.Now().Add(30 * time.Second)) or, preferably, wire a deterministic signal from the LogConsumer to the test (a done/ready channel the consumer closes when it has emitted the "listening on" line) and make the test wait on that channel instead of polling buf.String() — locate the polling loop around buf.String(), Run, PostStart and LogConsumer and implement one of these two fixes.
556-573:bodyread-and-discard looks like dead code.Lines 552–553 drain the GET response and Lines 559 does
_ = body. Either use the body to confirm the form renders as expected (asserting state machine progression) or drop the read entirely — the current form is confusing and will prompt future "is this needed?" questions. The manual login dance here is already documented as "do it manually to avoidrequire.NoErroraborting on the expected failure"; pruning the dead read would sharpen that intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/dex/dex_test.go` around lines 556 - 573, The GET response body is read into the variable named "body" and then discarded ("_ = body"), which is dead code; remove the initial read-and-discard (the io.ReadAll call that sets "body" and the "_ = body" line) or instead assert on that body to validate the login form; specifically either delete the code that reads the GET response into "body" (keep only the POST using client.Post to loginURL and the existing checks on "body2"/r2) or replace the discard with an assertion that the GET response contains expected form fields before proceeding to client.Post, referencing the variables "body", "body2", "client.Post", and "loginURL" to locate the code to change.
31-31:dexidp/dex:mastertag introduces non-deterministic CI behavior.Using the floating
:mastertag means the image contents change without any version pin, so this single test is vulnerable to upstream regressions, breaking image layouts, or Docker Hub rate limits unrelated to this PR. The PR notes call this out as a deliberate trade-off until v2.46.0 ships. Two hardening options:
- Gate the test behind an env var (e.g., skip unless
DEX_TEST_MASTER=1is set) so the default CI run is deterministic and the master-image test is opt-in.- Once v2.46.0 is released, replace
dexImageWithCCwith the pinned tag and delete the feature-flag caveat in the README.Also applies to: 620-644
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/dex/dex_test.go` at line 31, The test is using an unstable floating image tag via the dexImageWithCC constant; make running that master-image test opt-in by gating it behind an env var: in dex_test.go detect os.Getenv("DEX_TEST_MASTER") != "1" and call t.Skipf(...) at the start of the test(s) that reference dexImageWithCC (or in TestMain to skip those cases), keeping the existing dexImageWithCC name but ensuring CI doesn't run it by default; when v2.46.0 is released replace dexImageWithCC with the pinned release tag and remove the env-var gating and README note.modules/dex/README.md (2)
52-58: Documentation will age quickly — "not yet released at time of writing."As of April 2026, the latest tagged Dex release is still v2.45.1 (cut March 3, 2026), so this wording remains accurate for now. Once v2.46.0 lands, please revisit this paragraph (and the parallel
Known limitationsentry on Lines 160–162) so new readers don't have to reason about stale tense. Consider rewording to something like "requires Dex ≥ v2.46.0 (ordexidp/dex:masteruntil that release ships)" which stays correct either way.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/dex/README.md` around lines 52 - 58, Update the README wording to avoid time-sensitive phrasing: change the sentence that currently reads "requires Dex ≥ v2.46.0 (not yet released at time of writing)..." to a timeless form such as "requires Dex ≥ v2.46.0 (or use dexidp/dex:master until that release ships)" and apply the same rewording to the corresponding Known limitations entry; keep the note that callers must pin a compatible image and preserve the reference to WithEnableClientCredentials() so readers can find the option.
142-145: Signature style inconsistency in endpoint getters list.
GRPCEndpoint(ctx) (string, error)is shown with its full signature while the others are bare names. Consider either dropping the signature (it's documented in godoc) or showing all signatures for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/dex/README.md` around lines 142 - 145, The README endpoint getters list is inconsistent: all functions are listed by bare name (IssuerURL, ConfigEndpoint, JWKSEndpoint, TokenEndpoint, AuthEndpoint) but GRPCEndpoint is shown with a full signature (GRPCEndpoint(ctx) (string, error)); update the list to be consistent by either removing the signature from GRPCEndpoint (make it just GRPCEndpoint) or by adding full signatures for all entries (e.g., IssuerURL() string, ConfigEndpoint() string, JWKSEndpoint() string, TokenEndpoint() string, AuthEndpoint() string, GRPCEndpoint(ctx) (string, error)), and ensure the chosen style matches godoc conventions used elsewhere in the repo.modules/dex/examples_test.go (1)
71-78: Defer ordering inconsistent with the first example.
ExampleRun_authorizationCodeplacesdefer testcontainers.TerminateContainer(c)immediately afterdex.Run(Line 35) — the canonical testcontainers-go pattern, which works becauseTerminateContainerguards against a nil container. Here the defer is registered only after the error check (Line 78), so a partially-initialized container leaked by a failedRunwould never be cleaned up. Move the defer above theif err != nilto match the first example.🧹 Proposed fix
c, err := dex.Run(ctx, "dexidp/dex:v2.45.1", dex.WithClient(svc), dex.WithUser(user), ) + defer testcontainers.TerminateContainer(c) if err != nil { log.Fatalf("run: %v", err) } - defer testcontainers.TerminateContainer(c)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/dex/examples_test.go` around lines 71 - 78, The defer ordering can leak a partially-initialized container if dex.Run returns an error; move the defer testcontainers.TerminateContainer(c) to immediately follow the call to dex.Run (right after obtaining c and err) and before the if err != nil check so the container is always registered for cleanup even when dex.Run fails; locate the callsite using the dex.Run invocation and the variable c to update the defer placement.modules/dex/log.go (1)
97-109: Quoted-value parser retains backslash escapes.The tokenizer skips
\xinside quoted values (so the terminating"is handled correctly), but the returned sliceline[vStart:i]still contains the raw backslashes — the comment on Line 50 says values are "unquoted," which implies unescaping. For Dex's typicallevel=info msg="..."lines this is harmless, but amsgcontaining\"or\\will round-trip with the escapes intact in the resultingslog.Attr. Acceptable if intentional; otherwise a shortstrings.NewReplacer("\\\"", "\"", "\\\\", "\\").Replace(...)on quoted values would fix it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/dex/log.go` around lines 97 - 109, The quoted-value parser currently appends the raw substring line[vStart:i] into the kv entry, retaining backslash escapes; change this so the extracted quoted value is unescaped before appending. Locate the code that builds kv{k, line[vStart:i]} (uses variables k, vStart, i and the kv type) and replace the raw slice with an unescaped string (for example run it through strings.NewReplacer("\\\"", "\"", "\\\\", "\\").Replace(...) or use strconv.Unquote on `"`+line[vStart:i]+`"`), then append kv{k, unescapedValue} to out.modules/dex/types.go (1)
86-96: Validate grant-type values at construction time.The docstring enumerates four valid values (
authorization_code,refresh_token,client_credentials,password) but the option only rejects blanks. Typos slip throughNewClientand surface much later — either atRun(when Dex rejects the rendered YAML) or, worse, asunsupported_grant_typeat token exchange, well after the test has already set up state. Since one of the stated goals of the constructor is to "surface invalid configuration at call-site rather than at Run", enforcing the accepted set here aligns with that intent.♻️ Proposed fix
+var validGrantTypes = map[string]struct{}{ + "authorization_code": {}, + "refresh_token": {}, + "client_credentials": {}, + "password": {}, +} + func WithClientGrantTypes(grants ...string) ClientOption { return func(c *Client) error { for _, g := range grants { if g == "" { return errors.New("dex: client grant type must not be blank") } + if _, ok := validGrantTypes[g]; !ok { + return fmt.Errorf("dex: unsupported grant type %q", g) + } } c.grantTypes = append(c.grantTypes, grants...) return nil } }(Adds
fmtto the import set.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/dex/types.go` around lines 86 - 96, In WithClientGrantTypes (the ClientOption constructor), validate each provided grant string against the allowed set {"authorization_code","refresh_token","client_credentials","password"} (in addition to rejecting blank values) and return an error immediately for any invalid value so typos are surfaced at construction; update the error message to include the offending grant value(s) and ensure you append to c.grantTypes only after validation (use the function name WithClientGrantTypes and the Client.grantTypes field as references).modules/dex/grpc.go (1)
67-69: AddErrClientNotFound/ErrUserNotFoundsentinels for symmetry with the "exists" sentinels.
AddClient/AddUserreturn theErrClientExists/ErrUserExistssentinels so callers canerrors.Is(...), but the matchingRemoveClient/RemoveUser"not found" paths return ad-hoc formatted errors. Callers that want to tolerate "already gone" on cleanup have to string-match. Promoting these to sentinels (wrapped with the id/email for context) keeps the runtime admin API consistent.♻️ Proposed change (types.go + grpc.go)
In
modules/dex/types.go:var ( // ErrClientExists is returned by AddClient when a client with the given // ID is already registered. ErrClientExists = errors.New("dex: client already exists") // ErrUserExists is returned by AddUser when a user with the given email // is already registered. ErrUserExists = errors.New("dex: user already exists") + // ErrClientNotFound is returned by RemoveClient when no client matches + // the given ID. + ErrClientNotFound = errors.New("dex: client not found") + // ErrUserNotFound is returned by RemoveUser when no user matches the + // given email. + ErrUserNotFound = errors.New("dex: user not found")In
modules/dex/grpc.go:- if resp.NotFound { - return fmt.Errorf("dex: client %q not found", id) - } + if resp.NotFound { + return fmt.Errorf("%w: %q", ErrClientNotFound, id) + }- if resp.NotFound { - return fmt.Errorf("dex: user %q not found", email) - } + if resp.NotFound { + return fmt.Errorf("%w: %q", ErrUserNotFound, email) + }Also applies to: 131-133
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/dex/grpc.go` around lines 67 - 69, Create exported sentinel errors ErrClientNotFound and ErrUserNotFound in modules/dex/types.go (e.g. var ErrClientNotFound = errors.New("client not found"), var ErrUserNotFound = errors.New("user not found")), then update the "not found" return paths in modules/dex/grpc.go (the branches that currently do `return fmt.Errorf("dex: client %q not found", id)` and the analogous user case around lines referenced) to wrap the sentinel so callers can use errors.Is — e.g. return fmt.Errorf("dex: client %q not found: %w", id, ErrClientNotFound) and similarly for users (and update the second occurrence at the other referenced lines to use ErrUserNotFound/ErrClientNotFound as appropriate).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/modules/dex.md`:
- Around line 15-17: Update the fenced code block that contains the line "go get
github.com/testcontainers/testcontainers-go/modules/dex" to include a language
identifier (use "bash") so the fence becomes ```bash; locate the fenced block in
docs/modules/dex.md and change the opening backticks to include bash for
consistent syntax highlighting with other module docs.
In `@modules/dex/config_test.go`:
- Around line 248-261: The doc comment for dexLogLevel in config.go is
incorrect; update the comment near the dexLogLevel description to state that
values between slog's fixed levels round up (not down) — e.g. change the example
from "slog.LevelInfo+1 → 'info'" to "slog.LevelInfo+1 → 'warn'" or alternatively
use "slog.LevelInfo-1 → 'debug'" so the comment matches the dexLogLevel
implementation and the TestDexLogLevel expectations.
In `@modules/dex/config.go`:
- Around line 77-82: defaultGrantTypes currently always includes
"client_credentials" which breaks Dex v2.45.1 unless the env flag is set; update
the logic so "client_credentials" is omitted from defaultGrantTypes and only
added when the config rendering uses o.enableClientCredentials=true. Concretely,
remove "client_credentials" from the static var defaultGrantTypes and, in the
config build/render path that references defaultGrantTypes (the code using
o.enableClientCredentials), append "client_credentials" into the grantTypes
slice only when o.enableClientCredentials is true so the emitted
oauth2.grantTypes matches runtime capability.
In `@modules/dex/dex.go`:
- Around line 87-93: The readiness probe currently hard-codes
"/dex/.well-known/openid-configuration"; change it to compute the discovery path
from the configured issuer: parse the issuer URL (using net/url) and build
discoveryPath := strings.TrimRight(parsed.Path, "/") +
"/.well-known/openid-configuration" (ensure it starts with "/" if parsed.Path is
empty). Replace the literal in wait.ForHTTP(...) with that discoveryPath (keep
WithPort(httpPort) and WithStartupTimeout), and add imports for "net/url" and
"strings"; reference the code that constructs the ready wait (the wait.ForAll /
wait.ForHTTP call) and the variable that holds the configured issuer (the
function/variable supplying the issuer value used by WithIssuer).
In `@modules/dex/README.md`:
- Around line 11-13: The fenced code block containing the line "go get
github.com/testcontainers/testcontainers-go/modules/dex" lacks a language hint;
update that block to use a language specifier (e.g., ```bash) so markdownlint
MD040 is satisfied and syntax highlighting matches other code blocks in the
README.
---
Nitpick comments:
In `@modules/dex/config.go`:
- Around line 187-197: The function newUUIDv4 currently panics on
crypto/rand.Read failure; change its signature to newUUIDv4() (string, error)
and return an empty string plus fmt.Errorf("dex: read randomness: %w", err) on
read failure instead of panicking, and on success return the UUID string with
nil error; update callers (render and AddUser) to handle the error return
(propagate it up or wrap it where appropriate) so failures become recoverable
errors rather than panics.
In `@modules/dex/dex_test.go`:
- Around line 538-547: The call to cookiejar.New ignores its error; change jar,
_ := cookiejar.New(nil) to capture the error (jar, err := cookiejar.New(nil))
and assert no error using require.NoError(t, err) before using jar so the test
follows the file's pattern and will fail if the constructor signature changes;
update any downstream usage in the test that references jar if needed.
- Around line 198-212: The test's 5s polling deadline (deadline :=
time.Now().Add(5 * time.Second)) can flake under load because Run returns before
the LogConsumer goroutine has propagated the "listening on" line; change the
deadline to 30s (e.g., time.Now().Add(30 * time.Second)) or, preferably, wire a
deterministic signal from the LogConsumer to the test (a done/ready channel the
consumer closes when it has emitted the "listening on" line) and make the test
wait on that channel instead of polling buf.String() — locate the polling loop
around buf.String(), Run, PostStart and LogConsumer and implement one of these
two fixes.
- Around line 556-573: The GET response body is read into the variable named
"body" and then discarded ("_ = body"), which is dead code; remove the initial
read-and-discard (the io.ReadAll call that sets "body" and the "_ = body" line)
or instead assert on that body to validate the login form; specifically either
delete the code that reads the GET response into "body" (keep only the POST
using client.Post to loginURL and the existing checks on "body2"/r2) or replace
the discard with an assertion that the GET response contains expected form
fields before proceeding to client.Post, referencing the variables "body",
"body2", "client.Post", and "loginURL" to locate the code to change.
- Line 31: The test is using an unstable floating image tag via the
dexImageWithCC constant; make running that master-image test opt-in by gating it
behind an env var: in dex_test.go detect os.Getenv("DEX_TEST_MASTER") != "1" and
call t.Skipf(...) at the start of the test(s) that reference dexImageWithCC (or
in TestMain to skip those cases), keeping the existing dexImageWithCC name but
ensuring CI doesn't run it by default; when v2.46.0 is released replace
dexImageWithCC with the pinned release tag and remove the env-var gating and
README note.
In `@modules/dex/dextest_helpers_test.go`:
- Around line 45-65: Replace the string literals "GET" and "POST" passed to
http.NewRequestWithContext with the net/http constants http.MethodGet and
http.MethodPost respectively; locate the calls to http.NewRequestWithContext
that create req and postReq in dextest_helpers_test.go and update their method
arguments to use http.MethodGet and http.MethodPost to follow idiomatic Go
usage.
In `@modules/dex/examples_test.go`:
- Around line 71-78: The defer ordering can leak a partially-initialized
container if dex.Run returns an error; move the defer
testcontainers.TerminateContainer(c) to immediately follow the call to dex.Run
(right after obtaining c and err) and before the if err != nil check so the
container is always registered for cleanup even when dex.Run fails; locate the
callsite using the dex.Run invocation and the variable c to update the defer
placement.
In `@modules/dex/grpc.go`:
- Around line 67-69: Create exported sentinel errors ErrClientNotFound and
ErrUserNotFound in modules/dex/types.go (e.g. var ErrClientNotFound =
errors.New("client not found"), var ErrUserNotFound = errors.New("user not
found")), then update the "not found" return paths in modules/dex/grpc.go (the
branches that currently do `return fmt.Errorf("dex: client %q not found", id)`
and the analogous user case around lines referenced) to wrap the sentinel so
callers can use errors.Is — e.g. return fmt.Errorf("dex: client %q not found:
%w", id, ErrClientNotFound) and similarly for users (and update the second
occurrence at the other referenced lines to use
ErrUserNotFound/ErrClientNotFound as appropriate).
In `@modules/dex/log.go`:
- Around line 97-109: The quoted-value parser currently appends the raw
substring line[vStart:i] into the kv entry, retaining backslash escapes; change
this so the extracted quoted value is unescaped before appending. Locate the
code that builds kv{k, line[vStart:i]} (uses variables k, vStart, i and the kv
type) and replace the raw slice with an unescaped string (for example run it
through strings.NewReplacer("\\\"", "\"", "\\\\", "\\").Replace(...) or use
strconv.Unquote on `"`+line[vStart:i]+`"`), then append kv{k, unescapedValue} to
out.
In `@modules/dex/options.go`:
- Around line 147-152: The WithLogger option currently unconditionally assigns
o.logger, so calling WithLogger(nil) will unset a previously-set logger; update
the WithLogger function (the Option returned by WithLogger) to only assign
o.logger when the provided logger argument is non-nil (guard with if logger !=
nil { ... }) so passing nil becomes a no-op; reference the WithLogger function
and the options struct (o.logger) when making this change, or alternatively
document that WithLogger(nil) intentionally overrides earlier values if you
decide to keep the current behavior.
- Around line 86-100: Update the behavior documentation for WithConnector to
clarify that when t == ConnectorPassword the id/name parameters are ignored and
validation is intentionally skipped; add a one-line Godoc comment above the
WithConnector function stating "For ConnectorPassword, id and name are ignored
and blank-field validation is skipped." This makes the early return (if t ==
ConnectorPassword) explicit to callers without changing the existing logic used
in options_test.go.
- Around line 40-54: The current Option type implements Customize as a no-op
(Option.Customize) so any generic testcontainers helper that only calls
Customize will silently drop dex Options; update the documentation and API
surface to prevent this trap by adding a clear godoc NOTE on the Option type
(and/or Customize) that states Options (e.g., dex.WithClient) must be passed
directly to dex.Run and not forwarded through generic tc-go customizer
pipelines, and include a short example or sentence indicating that real mutation
happens only in Run; reference Option, Customize, Run and a typical Option
helper like WithClient so callers can find and apply the guidance.
In `@modules/dex/README.md`:
- Around line 52-58: Update the README wording to avoid time-sensitive phrasing:
change the sentence that currently reads "requires Dex ≥ v2.46.0 (not yet
released at time of writing)..." to a timeless form such as "requires Dex ≥
v2.46.0 (or use dexidp/dex:master until that release ships)" and apply the same
rewording to the corresponding Known limitations entry; keep the note that
callers must pin a compatible image and preserve the reference to
WithEnableClientCredentials() so readers can find the option.
- Around line 142-145: The README endpoint getters list is inconsistent: all
functions are listed by bare name (IssuerURL, ConfigEndpoint, JWKSEndpoint,
TokenEndpoint, AuthEndpoint) but GRPCEndpoint is shown with a full signature
(GRPCEndpoint(ctx) (string, error)); update the list to be consistent by either
removing the signature from GRPCEndpoint (make it just GRPCEndpoint) or by
adding full signatures for all entries (e.g., IssuerURL() string,
ConfigEndpoint() string, JWKSEndpoint() string, TokenEndpoint() string,
AuthEndpoint() string, GRPCEndpoint(ctx) (string, error)), and ensure the chosen
style matches godoc conventions used elsewhere in the repo.
In `@modules/dex/types.go`:
- Around line 86-96: In WithClientGrantTypes (the ClientOption constructor),
validate each provided grant string against the allowed set
{"authorization_code","refresh_token","client_credentials","password"} (in
addition to rejecting blank values) and return an error immediately for any
invalid value so typos are surfaced at construction; update the error message to
include the offending grant value(s) and ensure you append to c.grantTypes only
after validation (use the function name WithClientGrantTypes and the
Client.grantTypes field as references).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 58368487-5191-4973-8304-f4e73cc60d20
⛔ Files ignored due to path filters (1)
modules/dex/go.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
.github/dependabot.yml.vscode/.testcontainers-go.code-workspacedocs/modules/dex.mdmkdocs.ymlmodules/dex/Makefilemodules/dex/README.mdmodules/dex/config.gomodules/dex/config_test.gomodules/dex/dex.gomodules/dex/dex_test.gomodules/dex/dextest_helpers_test.gomodules/dex/examples_test.gomodules/dex/go.modmodules/dex/grpc.gomodules/dex/log.gomodules/dex/log_test.gomodules/dex/options.gomodules/dex/options_test.gomodules/dex/types.go
Actionable: - config.go: omit client_credentials from server-level grantTypes unless WithEnableClientCredentials() is set - dex.go: derive readiness probe path from configured issuer - config.go: fix dexLogLevel doc comment (rounds UP) - docs/modules/dex.md, README.md: add bash hint to go-get fenced block Nits accepted: Option doc note, WithLogger(nil) no-op, WithConnector ConnectorPassword doc, newUUIDv4 returns error, ErrClientNotFound / ErrUserNotFound sentinels, cookiejar error captured, 5s->30s log poll, dead `_ = body` removed, DEX_TEST_MASTER gate, http.Method* constants, examples_test.go defer ordering, README timeless wording + endpoint style. Deferred: WithClientGrantTypes value whitelist (too restrictive), log.go quoted-value unescape (low value).
There was a problem hiding this comment.
🧹 Nitpick comments (2)
modules/dex/grpc.go (1)
87-98: UsetestBcryptCostfor consistency with the YAML path (and faster tests).
config.gohashes static users at cost 10 (testBcryptCost) with the rationale that cost 10 is Dex's minimum and ~10× faster than the default. This runtimeAddUserpath usesbcrypt.DefaultCost(14), which:
- makes gRPC-added users hash ~20× slower than YAML-added users (tangible in suites that add multiple users),
- produces structurally different hashes for the same input across the two paths — a surprising inconsistency in a test-only module.
♻️ Proposed fix
- hash, err := bcrypt.GenerateFromPassword([]byte(u.password), bcrypt.DefaultCost) + hash, err := bcrypt.GenerateFromPassword([]byte(u.password), testBcryptCost) if err != nil { return fmt.Errorf("dex: bcrypt: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/dex/grpc.go` around lines 87 - 98, The bcrypt cost used when hashing runtime-added users is bcrypt.DefaultCost, causing slower and inconsistent hashes vs the YAML path; update the AddUser hashing call (bcrypt.GenerateFromPassword with u.password) to use the testBcryptCost constant instead of bcrypt.DefaultCost so runtime gRPC-added users use the same cost as config.go/test users and speed up tests; leave the surrounding error handling and userID generation (userID, newUUIDv4) unchanged.modules/dex/dex_test.go (1)
148-173: Tighten not-found assertions withErrorIsagainst the sentinel errors.
grpc.gowrapsErrClientNotFound/ErrUserNotFoundwith%w, so the assertions can be specific and would catch a regression where the sentinel is replaced with a generic gRPC error. The existingErrorIschecks for theExistssentinels on Add already do this — worth matching.♻️ Proposed tightening
// Second remove errors (not-found). err = c.RemoveClient(ctx, cl.ID()) - assert.Error(t, err) + assert.ErrorIs(t, err, dex.ErrClientNotFound) }// Second removal errors. err = c.RemoveUser(ctx, u.Email()) - assert.Error(t, err) + assert.ErrorIs(t, err, dex.ErrUserNotFound) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/dex/dex_test.go` around lines 148 - 173, The second removal assertion in TestGRPC_AddRemoveUser is too loose; replace the generic assert.Error(t, err) after the second call to c.RemoveUser(ctx, u.Email()) with assert.ErrorIs(t, err, dex.ErrUserNotFound) so the test checks the wrapped sentinel error (ErrUserNotFound) returned by the gRPC wrapper; locate the failing assertion in TestGRPC_AddRemoveUser and change it accordingly to use ErrorIs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@modules/dex/dex_test.go`:
- Around line 148-173: The second removal assertion in TestGRPC_AddRemoveUser is
too loose; replace the generic assert.Error(t, err) after the second call to
c.RemoveUser(ctx, u.Email()) with assert.ErrorIs(t, err, dex.ErrUserNotFound) so
the test checks the wrapped sentinel error (ErrUserNotFound) returned by the
gRPC wrapper; locate the failing assertion in TestGRPC_AddRemoveUser and change
it accordingly to use ErrorIs.
In `@modules/dex/grpc.go`:
- Around line 87-98: The bcrypt cost used when hashing runtime-added users is
bcrypt.DefaultCost, causing slower and inconsistent hashes vs the YAML path;
update the AddUser hashing call (bcrypt.GenerateFromPassword with u.password) to
use the testBcryptCost constant instead of bcrypt.DefaultCost so runtime
gRPC-added users use the same cost as config.go/test users and speed up tests;
leave the surrounding error handling and userID generation (userID, newUUIDv4)
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4d11e486-ed32-4d42-9eae-6c5e0fae84c3
📒 Files selected for processing (10)
docs/modules/dex.mdmodules/dex/README.mdmodules/dex/config.gomodules/dex/dex.gomodules/dex/dex_test.gomodules/dex/dextest_helpers_test.gomodules/dex/examples_test.gomodules/dex/grpc.gomodules/dex/options.gomodules/dex/types.go
✅ Files skipped from review due to trivial changes (2)
- docs/modules/dex.md
- modules/dex/README.md
- AddUser hashes with testBcryptCost (consistency + speed) - RemoveClient/RemoveUser tests use ErrorIs vs sentinels
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
modules/dex/grpc.go (1)
43-45: Optional: wrapErrClientExists/ErrUserExistswith the identifier for symmetry.
RemoveClient/RemoveUserwrap their sentinels with the id/email ("%w: %q") so callers get useful error text, whileAddClient/AddUserreturn the bare sentinel (ErrClientExistsat line 44,ErrUserExistsat line 112). Mirroring the same pattern here would make error messages more actionable while still beingerrors.Is-compatible.♻️ Proposed consistency tweak
if resp.AlreadyExists { - return ErrClientExists + return fmt.Errorf("%w: %q", ErrClientExists, cl.id) }if resp.AlreadyExists { - return ErrUserExists + return fmt.Errorf("%w: %q", ErrUserExists, u.email) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/dex/grpc.go` around lines 43 - 45, The AddClient/AddUser handlers currently return bare sentinels (ErrClientExists/ErrUserExists); make them mirror RemoveClient/RemoveUser by wrapping the sentinel with the identifier (e.g. fmt.Errorf("%w: %q", ErrClientExists, id) or the email) before returning so callers get actionable error text while preserving errors.Is compatibility; update the return in the AddClient path (resp.AlreadyExists) and the AddUser path where ErrUserExists is returned to use this wrapped format and reference the corresponding id/email variable used in those functions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modules/dex/dex_test.go`:
- Around line 611-614: The test calls sidecar.Logs(ctx) which returns an
io.ReadCloser (assigned to logs) but never closes it, leaking the Docker log
stream; fix by closing the stream after reading—e.g., after obtaining logs from
sidecar.Logs(ctx) ensure you call logs.Close() (or defer logs.Close()
immediately after the call) so the ReadCloser is properly released; update the
block around the logs variable in the test that reads with io.ReadAll to close
the stream when done.
---
Nitpick comments:
In `@modules/dex/grpc.go`:
- Around line 43-45: The AddClient/AddUser handlers currently return bare
sentinels (ErrClientExists/ErrUserExists); make them mirror
RemoveClient/RemoveUser by wrapping the sentinel with the identifier (e.g.
fmt.Errorf("%w: %q", ErrClientExists, id) or the email) before returning so
callers get actionable error text while preserving errors.Is compatibility;
update the return in the AddClient path (resp.AlreadyExists) and the AddUser
path where ErrUserExists is returned to use this wrapped format and reference
the corresponding id/email variable used in those functions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3a13c6a8-f127-403f-9730-f5c291500951
📒 Files selected for processing (2)
modules/dex/dex_test.gomodules/dex/grpc.go
There was a problem hiding this comment.
🧹 Nitpick comments (2)
modules/dex/dex_test.go (2)
480-480: Nit: usehttp.MethodGetinstead of the string literal"GET".Consistent with the rest of the file (which uses
http.MethodPostvia helpers) and idiomatic Go.✏️ Proposed tweak
- req, err := http.NewRequestWithContext(ctx, "GET", authURL, nil) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, authURL, nil)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/dex/dex_test.go` at line 480, Replace the string literal "GET" with the constant http.MethodGet in the http.NewRequestWithContext call (the req, err := http.NewRequestWithContext(ctx, "GET", authURL, nil) invocation) to match idiomatic Go and the rest of the test file; update that call to use http.MethodGet so it is consistent and avoids raw verb strings.
72-75: HTTP calls ignore the test's context deadline.
http.Getandhttp.Client.Get/Postusehttp.DefaultClientsemantics that do not honorctx. If Dex becomes ready but later wedges mid-response, these calls will hang past the 3-minute context timeout (untilgo test’s global timeout fires). The same pattern appears at lines 112, 552, and 562.Prefer
http.NewRequestWithContext(ctx, ...)+client.Do(req)so the ctx cancellation actually aborts in-flight requests.♻️ Example refactor for lines 72-75
- resp, err := http.Get(c.ConfigEndpoint()) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, c.ConfigEndpoint(), nil) + require.NoError(t, err) + resp, err := http.DefaultClient.Do(req) require.NoError(t, err) defer resp.Body.Close() require.Equal(t, 200, resp.StatusCode, "discovery endpoint must return 200")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/dex/dex_test.go` around lines 72 - 75, Replace plain http.Get calls that ignore the test context with context-aware requests: create a request via http.NewRequestWithContext(ctx, "GET", c.ConfigEndpoint(), nil) and execute it with an HTTP client (e.g., http.DefaultClient.Do(req)) so the test's ctx cancels in-flight calls; ensure you still handle resp and defer resp.Body.Close() and assert resp.StatusCode. Apply the same refactor for the other occurrences referenced (lines using http.Get at the same test: the calls around c.ConfigEndpoint() and the instances at the locations flagged).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@modules/dex/dex_test.go`:
- Line 480: Replace the string literal "GET" with the constant http.MethodGet in
the http.NewRequestWithContext call (the req, err :=
http.NewRequestWithContext(ctx, "GET", authURL, nil) invocation) to match
idiomatic Go and the rest of the test file; update that call to use
http.MethodGet so it is consistent and avoids raw verb strings.
- Around line 72-75: Replace plain http.Get calls that ignore the test context
with context-aware requests: create a request via
http.NewRequestWithContext(ctx, "GET", c.ConfigEndpoint(), nil) and execute it
with an HTTP client (e.g., http.DefaultClient.Do(req)) so the test's ctx cancels
in-flight calls; ensure you still handle resp and defer resp.Body.Close() and
assert resp.StatusCode. Apply the same refactor for the other occurrences
referenced (lines using http.Get at the same test: the calls around
c.ConfigEndpoint() and the instances at the locations flagged).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b4dcc950-e0d0-461c-9a33-c70719a5197b
📒 Files selected for processing (1)
modules/dex/dex_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (2)
modules/dex/dex_test.go (2)
315-344: Considert.Runsubtests per redirect URI.Minor readability / reporting nit: the
for _, uri := range urisloop runs both URIs as a single test, so a failure on the second URI surfaces without isolation and short-circuits the first failure’s reporting. Wrapping each iteration int.Run(uri, ...)gives per-URI pass/fail lines and lets-runtarget a specific one.♻️ Proposed refactor
for _, uri := range uris { - cfg := oauth2.Config{ - ClientID: "e2e", - ClientSecret: "s", - RedirectURL: uri, - Endpoint: oauth2.Endpoint{AuthURL: c.AuthEndpoint(), TokenURL: c.TokenEndpoint()}, - Scopes: []string{"openid"}, - } - tok := drivePasswordAuthCode(t, ctx, cfg, "a@e.com", "p") - assert.NotEmpty(t, tok.AccessToken, "uri=%s", uri) + uri := uri + t.Run(uri, func(t *testing.T) { + cfg := oauth2.Config{ + ClientID: "e2e", + ClientSecret: "s", + RedirectURL: uri, + Endpoint: oauth2.Endpoint{AuthURL: c.AuthEndpoint(), TokenURL: c.TokenEndpoint()}, + Scopes: []string{"openid"}, + } + tok := drivePasswordAuthCode(t, ctx, cfg, "a@e.com", "p") + assert.NotEmpty(t, tok.AccessToken) + }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/dex/dex_test.go` around lines 315 - 344, In TestAuthCode_MultipleRedirectURIs, wrap the existing for _, uri := range uris loop body in a t.Run subtest to isolate each redirect URI; inside the loop do uri := uri then t.Run(uri, func(t *testing.T) { /* move the oauth2.Config creation, drivePasswordAuthCode call and assertions here */ }), so each iteration (referencing uri and the loop that currently contains oauth2.Config, drivePasswordAuthCode and assert.NotEmpty) produces a separate subtest result and avoids variable capture issues.
499-506: Minor:loc := resp.Request.URLdefault is effectively dead on the success path.With
CheckRedirectreturninghttp.ErrUseLastResponseon the redirect tocfg.RedirectURL,respwill always be a 3xx whoseLocationheader carries the?code=.... The initial assignment only survives on the failure path (whererequire.NotEmpty(t, code, ...)fails anyway with a reasonable message). Not a bug — but extracting the code directly from theLocationheader (and asserting the 3xx up front) would make the control flow obvious and eliminate the misleading default:♻️ Proposed refactor
- loc := resp.Request.URL - if resp.StatusCode >= 300 && resp.StatusCode < 400 { - parsed, perr := url.Parse(resp.Header.Get("Location")) - require.NoError(t, perr) - loc = parsed - } - code := loc.Query().Get("code") - require.NotEmpty(t, code, "mockCallback should redirect with ?code=...; got %q", loc.String()) + require.GreaterOrEqual(t, resp.StatusCode, 300, "expected redirect to %s", cfg.RedirectURL) + require.Less(t, resp.StatusCode, 400, "expected redirect to %s", cfg.RedirectURL) + loc, err := url.Parse(resp.Header.Get("Location")) + require.NoError(t, err) + code := loc.Query().Get("code") + require.NotEmpty(t, code, "mockCallback should redirect with ?code=...; got %q", loc.String())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/dex/dex_test.go` around lines 499 - 506, The test currently assigns loc := resp.Request.URL which never applies on the success path because CheckRedirect returns http.ErrUseLastResponse causing resp to be a 3xx; instead, assert that resp.StatusCode is in the 300-399 range, parse the redirect target directly from resp.Header.Get("Location") (using url.Parse) and extract the code from the parsed URL query; update the require checks to require.NoError for the parse and require.NotEmpty for the code so the control flow is clear (references: resp, resp.Header.Get("Location"), CheckRedirect behavior, cfg.RedirectURL).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@modules/dex/dex_test.go`:
- Around line 315-344: In TestAuthCode_MultipleRedirectURIs, wrap the existing
for _, uri := range uris loop body in a t.Run subtest to isolate each redirect
URI; inside the loop do uri := uri then t.Run(uri, func(t *testing.T) { /* move
the oauth2.Config creation, drivePasswordAuthCode call and assertions here */
}), so each iteration (referencing uri and the loop that currently contains
oauth2.Config, drivePasswordAuthCode and assert.NotEmpty) produces a separate
subtest result and avoids variable capture issues.
- Around line 499-506: The test currently assigns loc := resp.Request.URL which
never applies on the success path because CheckRedirect returns
http.ErrUseLastResponse causing resp to be a 3xx; instead, assert that
resp.StatusCode is in the 300-399 range, parse the redirect target directly from
resp.Header.Get("Location") (using url.Parse) and extract the code from the
parsed URL query; update the require checks to require.NoError for the parse and
require.NotEmpty for the code so the control flow is clear (references: resp,
resp.Header.Get("Location"), CheckRedirect behavior, cfg.RedirectURL).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 756fecc3-037b-4a10-a2d3-afa127d1112a
📒 Files selected for processing (1)
modules/dex/dex_test.go
…ule convention (PR testcontainers#3659) Module guide (https://golang.testcontainers.org/modules/) states: "Use the name Container, not a module-specific name like PostgresContainer or RedisContainer". Reviewer @mdelapenya requested this explicitly on modules/dex/dex.go:46. Pure rename — type declaration, constructor return, and all method receivers in dex.go and grpc.go. Test files use type inference (c, err := dex.Run(...)) and need no edits. Doc comment in dex.md Run signature updated to match.
…efs (PR testcontainers#3659) Reviewer @mdelapenya flagged hardcoded v0.42.0 ("applies to more places below"): the contributing guide requires the "main" placeholder so release automation rewrites it at release time (https://golang.testcontainers.org/contributing/#adding-functional-options). Replaces 20 refs in docs/modules/dex.md and 1 in README.md, dropping the /releases/tag/v0.42.0 href suffix and swapping ":material-tag: v0.42.0" for ":material-tag: main".
… per module convention (PR testcontainers#3659) Module guide: "Testable examples use blocks marked with inside_block:runContainer for automatic documentation inclusion via codeinclude" (https://golang.testcontainers.org/modules/). The label in examples_test.go and the corresponding inside_block reference in dex.md must match — renaming both.
…tters in README (PR testcontainers#3659 nit) Round-1 review (CodeRabbit nit, README.md:142-145): the prior list named five getters bare and showed GRPCEndpoint with prose about its signature, which the reviewer flagged as inconsistent. Switching to a uniform bullet list with full signatures keeps godoc and README aligned and is clearer than the mixed-style note.
…oted values (PR testcontainers#3659 nit) Round-1 review (CodeRabbit nit, log.go:97-109): the quoted-value branch of tokenizeLogfmt advanced past escape sequences but appended the raw substring, so a Dex `msg=` field containing \" or \\ surfaced with the literal backslash in the resulting slog attr. Added a package-level strings.NewReplacer that expands the two escapes logfmt allows and a regression test covering both.
…owlist (PR testcontainers#3659 nit) Round-1 review (CodeRabbit nit, types.go:86-96): WithClientGrantTypes only rejected blanks, so typos like "authorisation_code" passed construction and surfaced much later — either when Dex parsed the rendered YAML or, worse, as unsupported_grant_type at the token endpoint. The constructor's stated goal is to surface invalid configuration at call-site, so add an allowlist matching the docstring. The allowlist mirrors Dex's four supported grants (authorization_code, refresh_token, client_credentials, password) and is kept as a package-level set so the docstring and check stay in sync. Table-driven test covers the supported set plus typo / unknown / mixed cases.
…d/email for symmetry (PR testcontainers#3659 nit) Round-3 review (CodeRabbit nit, grpc.go:43-45/111-113): RemoveClient and RemoveUser already return fmt.Errorf("%w: %q", ErrClientNotFound, id) (and the user variant), but AddClient and AddUser returned the bare sentinel — callers had no identifier in the error text. Mirrors the wrapping pattern. errors.Is checks against the bare sentinels (assert.ErrorIs in dex_test.go) keep working because %w preserves the chain.
….Run subtests (PR testcontainers#3659 nit) Round-5 review (CodeRabbit nit, dex_test.go:315-344): TestAuthCode_MultipleRedirectURIs ran two URIs in a bare for loop, so a failure on the second short-circuited the first's reporting and -run couldn't target a single URI. Wrap each iteration in t.Run(uri, …). Drop the "uri=%s" annotation on assert.NotEmpty — the subtest name now identifies the iteration. Go ≥ 1.22 fixed loop-variable capture, so no local-shadow needed.
…ivePasswordAuthCode (PR testcontainers#3659 nit) Round-5 review (CodeRabbit nit, dex_test.go:499-506): the helper assigned loc := resp.Request.URL as a fallback, but CheckRedirect returns http.ErrUseLastResponse so resp is always a 3xx on the success path — the assignment never applied. The flow read like it handled both redirect and non-redirect outcomes when in practice the non-redirect branch always failed at require.NotEmpty(code, ...). Replace with explicit 3xx require + direct Location parse so the control flow matches the actual contract.
|
@mdelapenya round-3 push (f58a75d..dc57f25) addresses both your review comments plus six remaining CodeRabbit nits. Ready for re-review. Your feedback:
|
|
@guilycst could you run |
@mdelapenya FIxed the linting issues that appeared locally |
|
Hey @mdelapenya, hope you are doing well. Any update on this PR? |
mdelapenya
left a comment
mdelapenya
left a comment
There was a problem hiding this comment.
@guilycst just an update in the docs, about the next-release placeholder
Co-authored-by: Manuel de la Peña <social.mdelapenya@gmail.com>
Co-authored-by: Manuel de la Peña <social.mdelapenya@gmail.com>
|
|
||
| #### Users | ||
|
|
||
| - Since <a href="https://github.com/testcontainers/testcontainers-go"><span class="tc-version">:material-tag: main</span></a> |
There was a problem hiding this comment.
Sorry @guilycst I should have been more explicit, my fault 🙏
Could you apply this to all the occurrences?
There was a problem hiding this comment.
All good @mdelapenya, I just pushed the docs changes
|
@guilycst to avoid more ping-pong from my side, I'm adding a commit on top to fix this:
Once this is merged, I can merge |
2 participants
What does this PR do?
Adds a new
dexmodule undermodules/dex/, providing a testcontainers wrapper for Dex, a CNCF-sandbox OIDC provider. Tests can spin up a real Dex instance and exercise OAuth2/OIDC flows end-to-end instead of mocking the token endpoint.Shape
DexContainerembedstestcontainers.Container.Run(ctx, image, opts...)entry point (tc-go v0.34+ convention).Client/Userare opaque value types (private fields); construct withNewClient(id, opts...) (Client, error)andNewUser(email, username, password, opts...) (User, error). Invalid input is rejected at call-site rather than atRun.WithClientRedirectURIs(...string),WithClientGrantTypes(...string)).error(type Option func(*options) error);Runpropagates option errors so blankid/name/URLinputs surface immediately.Storage(StorageSQLite/StorageMemory) — no stringly-typed storage kind.WithLogLeveltakesslog.Level; internally mapped to Dex's level vocabulary.GRPCEndpoint(ctx) (string, error)— errors propagate from the Docker API; no private shadow method that swallows them.AddClient,RemoveClient,AddUser,RemoveUserwithErrClientExists/ErrUserExistssentinels.WithClientGrantTypes:authorization_code,refresh_token,password.client_credentialsbehindWithEnableClientCredentials()(Dex ≥ v2.46.0 ordexidp/dex:master).Chicken-and-egg handling
Dex refuses to start without a YAML config, but the default issuer depends on the host-mapped port, only known after the container starts. The module follows the same pattern as the Kafka module: override the image entrypoint with a
sh -c "while [ ! -f /etc/dex/dex.yml ]; do sleep 0.1; done; exec dex serve /etc/dex/dex.yml"loop, then write the rendered YAML in aPostStartslifecycle hook viaCopyToContainer. Readiness probes (/dex/.well-known/openid-configuration+ gRPC listener) run inside the hook after the write.YAML rendering
Config is marshaled with
gopkg.in/yaml.v3from a typed struct (nottext/template) so every string field is escaped by the marshaler — no injection risk from client names / user emails / connector IDs. Bcrypt hashing uses cost 10 (Dex enforces ≥ 10).Why is it important?
navikt/mock-oauth2-server(minimal config surface, non-real grant semantics), or (c) hand-roll a Dex testcontainer with copy-pasted boilerplate.dexmodule closes the prod/test parity gap for every Go project that ships Dex or stands up a Dex-compatible OIDC provider for its tests.Related issues
Client/Userwith private fields + error return (avoids zero-value struct surprises),func(*options) errorfor all module options (validation at option-site),slog.Levelfor logging, typedStorage, singleGRPCEndpoint(ctx) (string, error), variadic list setters, no hand-rolled image-tag semver parsing in the module.How to test this PR
30 tests (unit + integration) including YAML rendering with injection safety, container lifecycle (default +
WithIssuer), discovery doc sanity, gRPC add/remove for clients and users (with sentinel idempotency), auth_code + password connector, refresh_token viaoffline_access, multiple redirect URIs on one client, ROPC, multi-client on one instance,mockCallbackconnector, runtime gRPC add end-to-end,WithIssuercross-container reachability vianetwork.Newalias, ID token verification viacoreos/go-oidc/v3,client_credentialsrejected by local connectors without flag,client_credentialsissued when flag is set on:master,WithLoggercaptures container output.Integration tests pull
dexidp/dex:v2.45.1; one test (TestClientCredentials_WithFeatureFlag) pullsdexidp/dex:masterto exercise the CC grant feature flag. Full suite runs in ~85s against a warm Docker cache.go vet ./...clean,gofmt -l .clean, two runnable godoc examples (ExampleRun_authorizationCode,ExampleRun_passwordGrant).Follow-ups (intentionally deferred)
WithGRPCTLS(...)option. Dex gRPC plaintext is fine inside a test network; not day-one scope.WithRawConnectors([]map[string]any)for LDAP / SAML / upstream OIDC. Can land when a real consumer asks.device_codeandtoken-exchangegrants — declarable in YAML today, but no integration test; flip to first-class support once a consumer appears.