fix(sdk): Batch access queries - #3398
Conversation
…h tokens Treat skipped database probes as optional and surface detailed auth or database failures for actionable diagnostics. BREAKING CHANGE: OIDC refresh is now opt-in; pass --refresh to spend a single-use refresh token. Claude-Session-Id: 4ee06f93-f1d6-414c-8cbd-3199fd82c1f3
Separate credentials from config.json and protect rotating refresh-token exchanges with atomic writes and cross-process locking. Persist the selected file or OS keychain backend, migrate legacy inline secrets, cache OIDC endpoints, and mark terminal invalid_grant failures for re-authentication.
Extract boolean flag and server-side filter parsing into clientcmd so catalog and access commands share one implementation. This removes duplicated parsing logic and keeps defaults and filter labels consistent.
Add remote access auditing for permissions, users, groups, roles, sign-in logs, and reviews with MatchItem filtering, rollups, group expansion, and machine-readable exports. Normalize filter middleware to preserve case-insensitive alternatives, exclusions, repeated values, and malformed-filter errors.
Make faro version output identify the source revision, build time, toolchain, platform, and dirty state across stamped and direct Go builds. Add a machine-local build and install workflow with an overridable destination.
Split oversized access-filter requests into bounded batches while preserving merged ordering, limits, and totals. Return malformed search filters as structured JSON bad requests for consistent client handling.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
WalkthroughThe pull request adds credential-store-backed OIDC authentication, access-control SDK and Faro commands, deterministic database filters, shared CLI helpers, and Faro build metadata with local installation targets. ChangesCredential storage and OIDC lifecycle
Access SDK and Faro commands
Query filters and shared helpers
Faro build metadata
Sequence Diagram(s)sequenceDiagram
participant FaroCommand
participant AccessSDK
participant PostgREST
participant Renderer
FaroCommand->>AccessSDK: Resolve filters and request access data
AccessSDK->>PostgREST: Query identities, grants, logs, or reviews
PostgREST-->>AccessSDK: Return rows and totals
AccessSDK-->>FaroCommand: Return hydrated results
FaroCommand->>Renderer: Render grouped or flat output
sequenceDiagram
participant Login
participant OIDCProvider
participant CredentialStore
Login->>OIDCProvider: Discover endpoints and exchange authorization code
OIDCProvider-->>Login: Return tokens and discovery metadata
Login->>CredentialStore: Save credentials and context metadata
CredentialStore-->>Login: Confirm persistence
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
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.
Actionable comments posted: 15
🧹 Nitpick comments (14)
clientcmd/credentials/keychain.go (1)
27-34: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winKeychain items are not scoped to the config directory.
FileStoreis rooted atdir, butKeychainStorekeys items by context name under the fixed servicemission-control. Two config directories (for example, a test run or a CI job that setsXDG_CONFIG_HOME) share one keychain namespace. A context namedbetain one config directory then reads and overwrites the secret of a context namedbetain another config directory. Consider scoping the keychain account or service bydirso the two stores have the same isolation contract.♻️ Example scoping by config directory
func NewKeychainStore(dir string) *KeychainStore { - return &KeychainStore{service: keychainService, ephemeral: NewFileStore(dir)} + sum := sha256.Sum256([]byte(dir)) + return &KeychainStore{ + service: keychainService + ":" + hex.EncodeToString(sum[:8]), + ephemeral: NewFileStore(dir), + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@clientcmd/credentials/keychain.go` around lines 27 - 34, Update NewKeychainStore and KeychainStore so keychain entries are scoped by the config directory dir, rather than always using the fixed keychainService. Derive a stable service or account namespace from dir while preserving the existing context-name keying and ephemeral FileStore behavior.clientcmd/credentials/keychain_test.go (1)
13-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
ginkgo.Label("ignore_local")and dropginkgo.Ordered.These specs need the OS keychain, which is an external service. The coding guidelines require the
ignore_locallabel for such tests. The environment-variable gate does not replace the label, because label-based filtering is what excludes the specs from local runs.Each spec also sets up its own state in
BeforeEachand cleans up withDeferCleanup, so no spec depends on the order of a previous one. Removeginkgo.Orderedunless a sequential dependency exists.♻️ Proposed change
-var _ = ginkgo.Describe("KeychainStore", ginkgo.Ordered, func() { +var _ = ginkgo.Describe("KeychainStore", ginkgo.Label("ignore_local"), func() {Based on coding guidelines: "Use
ginkgo.Label(\"ignore_local\")for tests requiring external services" and "Useginkgo.Orderedonly when test steps must run sequentially".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@clientcmd/credentials/keychain_test.go` around lines 13 - 26, Update the KeychainStore Describe declaration to add the ginkgo.Label("ignore_local") label and remove ginkgo.Ordered. Keep the existing BeforeEach environment gate and DeferCleanup behavior unchanged.Source: Coding guidelines
clientcmd/auth_login_test.go (1)
25-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReset
loginCredentialStoreinAfterEach.
AfterEachresetsloginServerandloginToken, but the new package-level flag variableloginCredentialStorekeeps its value between specs. A spec that sets it leaks the store selection into later specs in the same package.♻️ Proposed change
ginkgo.AfterEach(func() { oidcLogin = oldOIDCLogin loginServer = "" loginToken = "" + loginCredentialStore = "" })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@clientcmd/auth_login_test.go` around lines 25 - 29, Update the ginkgo.AfterEach cleanup to reset the package-level loginCredentialStore variable to its original/default value, alongside loginServer and loginToken, so each spec starts with an isolated credential-store selection.clientcmd/api_client.go (1)
59-68: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winResolve the config and store inside the lock.
LoadConfigandcfg.store()run beforecredentials.WithLock. If another process changescredential_storein that window, this process reloads the credential inside the lock but writes it back through the store selected from the stale config. The rotated refresh token then lands in the store that is no longer active.Move both calls into the locked function so the store choice matches the state observed under the lock.
♻️ Proposed refactor
func refreshContextToken(mcCtx *MCContext) error { - cfg, err := LoadConfig() - if err != nil { - return err - } - store, err := cfg.store() - if err != nil { - return err - } - return credentials.WithLock(configDir(), func() error { + cfg, err := LoadConfig() + if err != nil { + return err + } + store, err := cfg.store() + if err != nil { + return err + } + cred, err := store.Get(mcCtx.Name)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@clientcmd/api_client.go` around lines 59 - 68, Move the LoadConfig and cfg.store calls into the credentials.WithLock callback so configuration and store selection occur while the lock is held. Preserve their existing error returns and use the lock-scoped store for all subsequent credential operations, ensuring the selected store reflects the current configuration.clientcmd/whoami_test.go (1)
51-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a spec for the
NeedsReauthbranch ofprobeAuth.
probeAuthgained a new first branch inclientcmd/whoami.golines 210-215. It setsStatustoinvalid, setsRefreshStatustounavailable: <reason>, and returnsReauthError(). No spec covers it. A spec here needs no network server, because the branch returns beforecallWhoami.💚 Proposed spec
+ ginkgo.It("reports contexts that need re-authentication as invalid", func() { + report := probeAuth(context.TODO(), &MCContext{ + Name: "test", + Server: "http://mission-control.local", + NeedsReauth: "refresh token rejected", + }, "", false) + + Expect(report.Status).To(Equal("invalid")) + Expect(report.TokenSource).To(Equal("oidc")) + Expect(report.RefreshStatus).To(Equal("unavailable: refresh token rejected")) + Expect(report.Error).To(ContainSubstring("needs re-authentication")) + }) +🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@clientcmd/whoami_test.go` around lines 51 - 57, Add a test in clientcmd/whoami_test.go covering probeAuth’s NeedsReauth branch: provide a reauthentication-needed result, assert the auth status is invalid, RefreshStatus is “unavailable: <reason>”, and verify ReauthError is returned without requiring a network server. Anchor the setup and assertions to probeAuth and the existing whoami auth test patterns.faro/access_permissions.go (1)
99-105: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument that
--expand-groupscan produce more rows than--limit.
--limitis applied server-side to the grant rows.expandGroupsthen adds one synthetic row per active group member. The printed row count can therefore exceedpermissionsLimit. The behaviour looks intentional, but the flag help at line 181 states "Maximum number of rows", which contradicts it.Update the
--limithelp text or the--expand-groupshelp text to state that the limit applies to grants before expansion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@faro/access_permissions.go` around lines 99 - 105, Update the CLI help text for permissionsLimit or permissionsExpandGroups to clarify that --limit applies to grant rows before group expansion and that --expand-groups may produce additional rows. Keep the existing expansion behavior in expandGroups unchanged.sdk/access_batch.go (2)
39-44: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueDocument the ordering precondition, and guard the
compare == nilcase.The merged top-k is only correct when the client
comparematches the serverorderparam. Each batch is limited server-side, so a row that is globally in the toplimitmust also be in its own batch's toplimit. That holds only under a matching order.If
compareis nil, line 42 still truncates. The retained rows are then the firstlimitrows in batch-concatenation order, which is not a global top-k. All current callers pass acompare, so this is latent.Add a short doc comment on
pgGetAccessthat states the precondition, and skip truncation whencompareis nil and more than one batch ran.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/access_batch.go` around lines 39 - 44, Update pgGetAccess to document that global top-limit correctness requires compare to match the server order parameter. Guard the limit truncation so it only runs when compare is non-nil, or when no more than one batch was executed; preserve existing sorting and truncation behavior otherwise.
25-37: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider running batches concurrently with a bounded worker count.
The batches are independent and are issued sequentially. For a large ID set the wall-clock time scales linearly with the batch count. A bounded
errgroupwith a small concurrency limit would reduce latency without overloading the proxy.This is optional. Keep the sequential path if predictable server load matters more than latency.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/access_batch.go` around lines 25 - 37, Optionally update the batch-processing loop in the surrounding access-batch function to execute independent client.pgGet calls through an errgroup with a small bounded concurrency limit, while preserving rows aggregation, totalKnown/total handling, and immediate error propagation; retain the current sequential loop if predictable server load is preferred.sdk/access_batch_test.go (1)
82-82: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the ordering assumption in the fixture explicit.
Line 82 encodes the expected winner through
100-requestand%02d. The assertion at line 95 then depends on three facts: the batch count stays below 100,%02dkeeps the values lexicographically comparable, andListAccessGrantsissues no extra requests beyond the grant batches. If a future change makesListAccessGrantsissue a hydration request,requests.Load()no longer equals the batch count, and lines 93 and 95 fail for an unrelated reason.Consider deriving the expected user from the response bodies the handler recorded, rather than from the request counter.
Also applies to: 93-95
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/access_batch_test.go` at line 82, Update the access-grant fixture assertions around ListAccessGrants to derive the expected user from the response bodies recorded by the handler, rather than calculating it from requests.Load() and the 100-request ordering formula. Keep the ordering expectation explicit while removing dependence on batch-count limits, %02d formatting, or additional hydration requests.sdk/access_filter.go (1)
51-59: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider a safety cap on the unbounded candidate fetch.
params.Del("limit")removes the server-side bound. When a name filter is present, the client downloads every non-deleted row of the identity table before it matches locally. On a large tenant this can be a very large response. Add a hard ceiling and warn when it is reached, so the command degrades predictably instead of transferring the whole table.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/access_filter.go` around lines 51 - 59, Update matchingIdentityIDs to retain a hard maximum on the candidate fetch instead of removing the limit entirely. Apply the cap through params and emit a warning when the fetched candidates reach that ceiling, while preserving the existing local matching and error behavior.faro/access.go (1)
23-29: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBind the shared context to interrupt signals.
accessClientreturnscontext.Background(). Every access subcommand uses it, and the SDK fan-out issues one request per id batch. A slow or hung server leaves the CLI with no cancellation path. Usesignal.NotifyContextso Ctrl-C cancels the in-flight requests, and return the cancel function to the caller.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@faro/access.go` around lines 23 - 29, Update accessClient to create a signal.NotifyContext for interrupt signals instead of context.Background(), and extend its return values with the associated cancel function. Ensure the cancel function is returned on successful client initialization and also invoked before returning when RemoteClient fails; update all callers to receive and defer the cancel function while preserving the shared context for SDK requests.faro/access_users.go (1)
57-63: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReport truncation for the grants section.
ListAccessGrantsreturns a total, but this call discards it. The list paths callwarnTruncated. Without it,access users getcan silently show a truncated grant list.♻️ Proposed change
- grants, _, err := client.ListAccessGrants(ctx, sdk.AccessGrantOptions{UserIDs: []string{user.ID.String()}}) + grants, total, err := client.ListAccessGrants(ctx, sdk.AccessGrantOptions{UserIDs: []string{user.ID.String()}}) if err != nil { return nil, err } + warnTruncated("access grants", len(grants), total) result.Access = grants🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@faro/access_users.go` around lines 57 - 63, Update the access-grants handling in the user retrieval flow to retain the total returned by ListAccessGrants and pass it with the fetched grants to warnTruncated. Preserve the existing error handling and result.Access assignment while ensuring access users get reports when the grant list is truncated.faro/access_logs.go (1)
34-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffDuplicated history-command body in
faro/access_logs.goandfaro/access_reviews.go. BothRunEbodies perform the same sequence: build the client, resolve config IDs, resolve user IDs, parse--since, short-circuit on an empty user match, call one SDK list method, warn on truncation, then print. Only the SDK call and the result type differ. A shared generic helper that accepts a fetch function and a row mapper would remove the duplication and keep the two commands in sync.
faro/access_logs.go#L34-L70: extract the shared sequence into a helper and call it withclient.ListAccessLogsandaccessLogRows.faro/access_reviews.go#L33-L69: call the same helper withclient.ListAccessReviewsandaccessReviewRows.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@faro/access_logs.go` around lines 34 - 70, Extract the duplicated history-command sequence from faro/access_logs.go:34-70 into a shared generic helper that handles client creation, ID resolution, since parsing, empty-user short-circuiting, truncation warnings, and result printing; parameterize it with the SDK fetch function and row mapper. Update faro/access_logs.go:34-70 to call the helper with client.ListAccessLogs and accessLogRows, and update faro/access_reviews.go:33-69 to call it with client.ListAccessReviews and accessReviewRows.faro/access_test.go (1)
35-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
ginkgo.GinkgoRecoverinside theaccessServerhandler.The handler is invoked by
httptestin a non-spec goroutine. Callginkgo.Failfrom the spec goroutine, or adddefer ginkgo.GinkgoRecover()at the top of this handler so Ginkgo reports the failure correctly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@faro/access_test.go` around lines 35 - 48, Add defer ginkgo.GinkgoRecover() at the start of the HTTP handler in accessServer, before any request processing or potential ginkgo.Fail call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@auth/oidcclient/oidcclient.go`:
- Around line 142-146: Increase the response-body read limit in the non-OK
branch before constructing and unmarshalling TokenError, using a size sufficient
for normal OAuth error documents so the error code is preserved and Terminal()
can classify invalid_grant correctly. Keep the existing TokenError construction
and return flow unchanged.
In `@clientcmd/api_client.go`:
- Around line 59-68: The refreshContextToken flow in clientcmd/api_client.go
must load the configuration and select cfg.store() inside the callback passed to
credentials.WithLock, so all subsequent decisions and writes use state observed
under the lock. In clientcmd/whoami_token.go, retain saveConfigLocked but ensure
its caller passes the configuration loaded within that locked callback,
preventing endpoint-cache persistence from overwriting concurrent context
changes.
- Around line 68-84: Ensure the refresh flow around refreshContextToken uses a
non-nil MCContext before calling mcCtx.credential(), applying credentials, or
accessing credential fields inside credentials.WithLock. Add the guard at the
entry point while preserving the existing locked read-modify-write behavior and
reauthentication checks.
In `@clientcmd/refresh_test.go`:
- Around line 26-41: Add defer ginkgo.GinkgoRecover() at the start of the
httptest server HTTP handler in newTokenEndpoint, and in the handler-based
assertion paths such as writeRotatedTokens and the os.Chmod assertion, so Gomega
failures from server goroutines are recovered and reported to the spec.
In `@clientcmd/whoami_token.go`:
- Around line 53-70: Update contextTokenEndpoint to validate
endpoints.TokenEndpoint immediately after discoverOIDCEndpoints returns,
returning an error when it is empty before assigning or caching the discovery
result. Preserve the existing endpoint caching and successful return behavior
for valid token endpoints.
In `@faro/access_filter_test.go`:
- Around line 39-56: Update the test setup around the DeferCleanup callback to
capture the prior values of all eleven mutated package-level variables,
including the permissions, access-logs, and access-reviews limits and flags.
Restore each captured value in cleanup rather than hard-coding defaults,
ensuring later AccessPermissions, AccessLogs, and AccessReviews specs see the
original state.
In `@faro/access_groups.go`:
- Around line 64-70: Update the access-grant retrieval in the access-group flow
to retain the total returned by ListAccessGrants instead of discarding it. After
the successful call, invoke warnTruncated with "access grants", len(grants), and
the returned total before assigning result.Access, while preserving the existing
error handling.
- Around line 94-104: Update completeAccessGroupIDs to return each group's Name
instead of ID.String(), so Cobra can prefix-filter completions by the partially
typed group name. Also update the related ValidArgs filtering to match group
names rather than IDs, preserving the existing completion behavior and limits.
In `@faro/access_permissions.go`:
- Around line 74-84: Update the empty-result branch in the permissionsUser flow
to set Derived: true on both AccessSummaryByUserResult and
AccessSummaryByConfigResult, matching the non-empty rollup paths; leave the
AccessPermissionsResult path unchanged.
In `@Makefile`:
- Around line 5-6: Change the GIT_COMMIT and BUILD_DATE definitions in the
Makefile to simply expanded variables so git and date are evaluated once when
Make reads the file, ensuring all Faro platform builds in one invocation use
identical metadata.
In `@sdk/access_batch_test.go`:
- Around line 77-83: Add defer ginkgo.GinkgoRecover() at the start of the
httptest HTTP handler passed to http.HandlerFunc, before the request counter and
Gomega assertion, so assertion failures from the handler goroutine are recovered
by Ginkgo.
In `@sdk/access_identity.go`:
- Around line 399-415: Update the generic pgGetIn batching flow to restore
global ordering after concatenating batches: accept an optional comparison
function and sort the merged out slice after the loop when provided. Pass the
name comparison from ListExternalUsers, ListExternalGroups, and
ListExternalRoles, while preserving existing behavior for callers without
ordering requirements.
- Around line 375-381: Update nameOrEmailFilter and nameOrAliasFilter to escape
each interpolated value for PostgREST filter syntax: escape embedded backslashes
and double quotes, then wrap the value in PostgREST double quotes before
inserting it into the ilike and aliases expressions. Reuse the escaped value
consistently across every occurrence.
In `@sdk/access_test.go`:
- Around line 30-44: Add defer ginkgo.GinkgoRecover() as the first statement in
the pgRoutes HTTP handler and the inline HTTP handlers at sdk/access_test.go
lines 89-94, 105-111, and 415-421. Also add it as the first statement in the
identityRoutes handler at sdk/access_filter_test.go lines 26-41, so assertions
from server goroutines report through Ginkgo correctly.
In `@sdk/access.go`:
- Around line 67-69: Update ListAccessGrants to pass the o.User wildcard value
through a quotePostgRESTValue helper before constructing the PostgREST or
filter. Add the helper using strings.NewReplacer to escape backslashes and
double quotes, then wrap the value in double quotes so reserved characters
remain literal.
---
Nitpick comments:
In `@clientcmd/api_client.go`:
- Around line 59-68: Move the LoadConfig and cfg.store calls into the
credentials.WithLock callback so configuration and store selection occur while
the lock is held. Preserve their existing error returns and use the lock-scoped
store for all subsequent credential operations, ensuring the selected store
reflects the current configuration.
In `@clientcmd/auth_login_test.go`:
- Around line 25-29: Update the ginkgo.AfterEach cleanup to reset the
package-level loginCredentialStore variable to its original/default value,
alongside loginServer and loginToken, so each spec starts with an isolated
credential-store selection.
In `@clientcmd/credentials/keychain_test.go`:
- Around line 13-26: Update the KeychainStore Describe declaration to add the
ginkgo.Label("ignore_local") label and remove ginkgo.Ordered. Keep the existing
BeforeEach environment gate and DeferCleanup behavior unchanged.
In `@clientcmd/credentials/keychain.go`:
- Around line 27-34: Update NewKeychainStore and KeychainStore so keychain
entries are scoped by the config directory dir, rather than always using the
fixed keychainService. Derive a stable service or account namespace from dir
while preserving the existing context-name keying and ephemeral FileStore
behavior.
In `@clientcmd/whoami_test.go`:
- Around line 51-57: Add a test in clientcmd/whoami_test.go covering probeAuth’s
NeedsReauth branch: provide a reauthentication-needed result, assert the auth
status is invalid, RefreshStatus is “unavailable: <reason>”, and verify
ReauthError is returned without requiring a network server. Anchor the setup and
assertions to probeAuth and the existing whoami auth test patterns.
In `@faro/access_logs.go`:
- Around line 34-70: Extract the duplicated history-command sequence from
faro/access_logs.go:34-70 into a shared generic helper that handles client
creation, ID resolution, since parsing, empty-user short-circuiting, truncation
warnings, and result printing; parameterize it with the SDK fetch function and
row mapper. Update faro/access_logs.go:34-70 to call the helper with
client.ListAccessLogs and accessLogRows, and update faro/access_reviews.go:33-69
to call it with client.ListAccessReviews and accessReviewRows.
In `@faro/access_permissions.go`:
- Around line 99-105: Update the CLI help text for permissionsLimit or
permissionsExpandGroups to clarify that --limit applies to grant rows before
group expansion and that --expand-groups may produce additional rows. Keep the
existing expansion behavior in expandGroups unchanged.
In `@faro/access_test.go`:
- Around line 35-48: Add defer ginkgo.GinkgoRecover() at the start of the HTTP
handler in accessServer, before any request processing or potential ginkgo.Fail
call.
In `@faro/access_users.go`:
- Around line 57-63: Update the access-grants handling in the user retrieval
flow to retain the total returned by ListAccessGrants and pass it with the
fetched grants to warnTruncated. Preserve the existing error handling and
result.Access assignment while ensuring access users get reports when the grant
list is truncated.
In `@faro/access.go`:
- Around line 23-29: Update accessClient to create a signal.NotifyContext for
interrupt signals instead of context.Background(), and extend its return values
with the associated cancel function. Ensure the cancel function is returned on
successful client initialization and also invoked before returning when
RemoteClient fails; update all callers to receive and defer the cancel function
while preserving the shared context for SDK requests.
In `@sdk/access_batch_test.go`:
- Line 82: Update the access-grant fixture assertions around ListAccessGrants to
derive the expected user from the response bodies recorded by the handler,
rather than calculating it from requests.Load() and the 100-request ordering
formula. Keep the ordering expectation explicit while removing dependence on
batch-count limits, %02d formatting, or additional hydration requests.
In `@sdk/access_batch.go`:
- Around line 39-44: Update pgGetAccess to document that global top-limit
correctness requires compare to match the server order parameter. Guard the
limit truncation so it only runs when compare is non-nil, or when no more than
one batch was executed; preserve existing sorting and truncation behavior
otherwise.
- Around line 25-37: Optionally update the batch-processing loop in the
surrounding access-batch function to execute independent client.pgGet calls
through an errgroup with a small bounded concurrency limit, while preserving
rows aggregation, totalKnown/total handling, and immediate error propagation;
retain the current sequential loop if predictable server load is preferred.
In `@sdk/access_filter.go`:
- Around line 51-59: Update matchingIdentityIDs to retain a hard maximum on the
candidate fetch instead of removing the limit entirely. Apply the cap through
params and emit a warning when the fetched candidates reach that ceiling, while
preserving the existing local matching and error behavior.
🪄 Autofix
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 Plus
Run ID: 2cc08a62-8130-4b90-8810-6ece33e63c4e
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (57)
Makefileauth/oidcclient/oidcclient.goclientcmd/api_client.goclientcmd/auth_login.goclientcmd/auth_login_test.goclientcmd/context.goclientcmd/context_credentials.goclientcmd/context_credentials_test.goclientcmd/context_test.goclientcmd/credentials/file.goclientcmd/credentials/file_test.goclientcmd/credentials/keychain.goclientcmd/credentials/keychain_test.goclientcmd/credentials/lock.goclientcmd/credentials/store.goclientcmd/credentials/suite_test.goclientcmd/flags.goclientcmd/refresh_test.goclientcmd/whoami.goclientcmd/whoami_test.goclientcmd/whoami_token.gocmd/access_groups.gocmd/access_roles.gocmd/access_users.gocmd/catalog_entity.gocmd/catalog_get.godb/middleware.godb/middleware_test.gofaro/access.gofaro/access_filter_test.gofaro/access_groups.gofaro/access_help.gofaro/access_help_test.gofaro/access_logs.gofaro/access_permissions.gofaro/access_print.gofaro/access_reviews.gofaro/access_roles.gofaro/access_rollup.gofaro/access_rows.gofaro/access_test.gofaro/access_users.gofaro/catalog_help.gofaro/main.gofaro/version.gofaro/version_test.gogo.modsdk/access.gosdk/access_batch.gosdk/access_batch_test.gosdk/access_filter.gosdk/access_filter_test.gosdk/access_history_test.gosdk/access_identity.gosdk/access_logs.gosdk/access_test.gosdk/client.go
| if resp.StatusCode != http.StatusOK { | ||
| body, _ := io.ReadAll(io.LimitReader(resp.Body, 512)) | ||
| msg := strings.TrimSpace(string(body)) | ||
| if msg == "" { | ||
| return nil, fmt.Errorf("token endpoint returned %d", resp.StatusCode) | ||
| } | ||
| return nil, fmt.Errorf("token endpoint returned %d: %s", resp.StatusCode, msg) | ||
| tokenErr := &TokenError{StatusCode: resp.StatusCode, Body: strings.TrimSpace(string(body))} | ||
| _ = json.Unmarshal(body, tokenErr) | ||
| return nil, tokenErr |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The 512-byte body limit can hide the error code and defeat Terminal().
The reader truncates the body at 512 bytes. If a provider returns a longer error body, json.Unmarshal fails, the error is discarded, and Code stays empty. Terminal() then returns false, so refreshContextToken in clientcmd/api_client.go classifies a real invalid_grant as an unknown failure. The dead refresh token stays on disk and every later command fails with the same error instead of prompting a new login.
Raise the limit so normal OAuth error documents fit.
🐛 Proposed fix
if resp.StatusCode != http.StatusOK {
- body, _ := io.ReadAll(io.LimitReader(resp.Body, 512))
+ body, _ := io.ReadAll(io.LimitReader(resp.Body, 8<<10))
tokenErr := &TokenError{StatusCode: resp.StatusCode, Body: strings.TrimSpace(string(body))}
_ = json.Unmarshal(body, tokenErr)
return nil, tokenErr
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if resp.StatusCode != http.StatusOK { | |
| body, _ := io.ReadAll(io.LimitReader(resp.Body, 512)) | |
| msg := strings.TrimSpace(string(body)) | |
| if msg == "" { | |
| return nil, fmt.Errorf("token endpoint returned %d", resp.StatusCode) | |
| } | |
| return nil, fmt.Errorf("token endpoint returned %d: %s", resp.StatusCode, msg) | |
| tokenErr := &TokenError{StatusCode: resp.StatusCode, Body: strings.TrimSpace(string(body))} | |
| _ = json.Unmarshal(body, tokenErr) | |
| return nil, tokenErr | |
| if resp.StatusCode != http.StatusOK { | |
| body, _ := io.ReadAll(io.LimitReader(resp.Body, 8<<10)) | |
| tokenErr := &TokenError{StatusCode: resp.StatusCode, Body: strings.TrimSpace(string(body))} | |
| _ = json.Unmarshal(body, tokenErr) | |
| return nil, tokenErr |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@auth/oidcclient/oidcclient.go` around lines 142 - 146, Increase the
response-body read limit in the non-OK branch before constructing and
unmarshalling TokenError, using a size sufficient for normal OAuth error
documents so the error code is preserved and Terminal() can classify
invalid_grant correctly. Keep the existing TokenError construction and return
flow unchanged.
| cfg, err := LoadConfig() | ||
| if err != nil { | ||
| logger.Debugf("failed to refresh OIDC token for context %q server %s: %v", mcCtx.Name, mcCtx.Server, err) | ||
| if mcCtx.Token != "" && mcCtx.Token != previousAccessToken { | ||
| return mcCtx.Token, nil | ||
| } | ||
| return "", fmt.Errorf("refresh OIDC token for %s: %w", mcCtx.Server, err) | ||
| return err | ||
| } | ||
| store, err := cfg.store() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| logger.Debugf("refreshed OIDC token for context %q server %s", mcCtx.Name, mcCtx.Server) | ||
| mcCtx.SetOIDCTokens(refreshed) | ||
| if cfg, err := LoadConfig(); err == nil { | ||
| updateContextOIDCTokens(cfg, mcCtx.Name, refreshed) | ||
| } else { | ||
| return "", fmt.Errorf("failed to update context OIDC tokens: %w", err) | ||
| return credentials.WithLock(configDir(), func() error { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
The configuration is read outside credentials.WithLock and then used for decisions and writes inside it. refreshContextToken loads the config and selects the credential store before it acquires the cross-process lock. Everything the locked block does afterwards, including the endpoint-cache write, is based on that pre-lock snapshot. A concurrent process that changes credential_store or any context in the window causes a write to the wrong store or an overwrite of the other process' config changes.
clientcmd/api_client.go#L59-L68: move theLoadConfigandcfg.store()calls into the function passed tocredentials.WithLock, so the store selection matches the state observed under the lock.clientcmd/whoami_token.go#L64-L69: keepsaveConfigLockedbut require the caller to pass a config that was loaded under the lock; the endpoint-cache write then no longer discards concurrent context changes.
📍 Affects 2 files
clientcmd/api_client.go#L59-L68(this comment)clientcmd/whoami_token.go#L64-L69
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@clientcmd/api_client.go` around lines 59 - 68, The refreshContextToken flow
in clientcmd/api_client.go must load the configuration and select cfg.store()
inside the callback passed to credentials.WithLock, so all subsequent decisions
and writes use state observed under the lock. In clientcmd/whoami_token.go,
retain saveConfigLocked but ensure its caller passes the configuration loaded
within that locked callback, preventing endpoint-cache persistence from
overwriting concurrent context changes.
| return credentials.WithLock(configDir(), func() error { | ||
| cred, err := store.Get(mcCtx.Name) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if cred.IsZero() { | ||
| cred = mcCtx.credential() | ||
| } | ||
| mcCtx.applyCredential(cred) | ||
|
|
||
| if cred.NeedsReauth != "" { | ||
| return mcCtx.ReauthError() | ||
| } | ||
| if cred.OIDC == nil || cred.OIDC.RefreshToken == "" { | ||
| mcCtx.NeedsReauth = "no refresh token" | ||
| return mcCtx.ReauthError() | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -e go . clientcmd | head -50
echo "== credential() and applyCredential"
ast-grep run --lang go --pattern 'func (c *MCContext) credential() $$$' clientcmd || true
rg -nP -C6 'func \(c \*MCContext\) (credential|applyCredential)\s*\(' clientcmd
echo "== WithLock implementation"
rg -nP -C15 'func WithLock\s*\(' clientcmd/credentials
echo "== callers of WithLock"
rg -nP -C3 'WithLock\s*\(' --type=go
echo "== saveConfigLocked / store.Set / Writable lock usage"
rg -nP -C10 'func saveConfigLocked\s*\(' clientcmd
rg -nP -C6 'func \(s \*(FileStore|KeychainStore)\) (Set|Get|Writable)\s*\(' clientcmd/credentialsRepository: flanksource/mission-control
Length of output: 5582
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== clientcmd/api_client.go relevant lines"
cat -n clientcmd/api_client.go | sed -n '50,110p'
echo "== clientcmd/credentials/file.go lock/store methods"
cat -n clientcmd/credentials/file.go | sed -n '1,260p'
echo "== clientcmd/credentials/lock.go full"
cat -n clientcmd/credentials/lock.go | sed -n '1,140p'
echo "== contextTokenEndpoint definition/usages"
rg -n -C8 'contextTokenEndpoint' --type=go
echo "== lock helper exports and WithLock caller sites"
rg -n -C4 'WithLock\(|Unlock\(|F*Lock\(' clientcmd --type=goRepository: flanksource/mission-control
Length of output: 10221
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== clientcmd/credentials/store.go"
cat -n clientcmd/credentials/store.go | sed -n '1,220p'
echo "== all token endpoint names/usages"
rg -n -C8 'TokenEndpoint|tokenEndpoint|contextToken|RefreshToken' --type=go
echo "== all WithLock call sites"
rg -n -C4 '\bWithLock\s*\(' --type=go
echo "== all lock Unlock usage"
rg -n -C4 '\.Unlock\(' clientcmd --type=goRepository: flanksource/mission-control
Length of output: 3225
Ensure refreshContextToken is called through a non-nil MCContext.
mcCtx.credential() returns nil for nil receivers, and cred.OIDC is dereferenced inside WithLock. The nil guard at line 73 does not cover this path because IsZero() accepts nil credentials. The store methods called here do not acquire another WithLock, so the refreshed read-modify-write under the existing lock is fine.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@clientcmd/api_client.go` around lines 68 - 84, Ensure the refresh flow around
refreshContextToken uses a non-nil MCContext before calling mcCtx.credential(),
applying credentials, or accessing credential fields inside
credentials.WithLock. Add the guard at the entry point while preserving the
existing locked read-modify-write behavior and reauthentication checks.
| func newTokenEndpoint(respond func(w http.ResponseWriter, presented string)) *tokenEndpoint { | ||
| e := &tokenEndpoint{respond: respond} | ||
| e.server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| Expect(r.URL.Path).To(Equal("/token")) | ||
| Expect(r.ParseForm()).To(Succeed()) | ||
| Expect(r.Form.Get("grant_type")).To(Equal("refresh_token")) | ||
|
|
||
| presented := r.Form.Get("refresh_token") | ||
| e.mu.Lock() | ||
| e.requests = append(e.requests, presented) | ||
| e.mu.Unlock() | ||
|
|
||
| e.respond(w, presented) | ||
| })) | ||
| return e | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Add defer ginkgo.GinkgoRecover() to the HTTP handler.
The handler runs in a goroutine owned by httptest.Server, not in the spec goroutine. If any Expect in the handler fails, Gomega calls ginkgo.Fail, which panics. Without GinkgoRecover in that goroutine, the panic aborts the whole suite instead of failing the spec. The same applies to the assertions that respond runs, for example writeRotatedTokens at Line 57 and the os.Chmod assertion at Line 176.
💚 Proposed fix
e.server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ defer ginkgo.GinkgoRecover()
Expect(r.URL.Path).To(Equal("/token"))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func newTokenEndpoint(respond func(w http.ResponseWriter, presented string)) *tokenEndpoint { | |
| e := &tokenEndpoint{respond: respond} | |
| e.server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| Expect(r.URL.Path).To(Equal("/token")) | |
| Expect(r.ParseForm()).To(Succeed()) | |
| Expect(r.Form.Get("grant_type")).To(Equal("refresh_token")) | |
| presented := r.Form.Get("refresh_token") | |
| e.mu.Lock() | |
| e.requests = append(e.requests, presented) | |
| e.mu.Unlock() | |
| e.respond(w, presented) | |
| })) | |
| return e | |
| } | |
| func newTokenEndpoint(respond func(w http.ResponseWriter, presented string)) *tokenEndpoint { | |
| e := &tokenEndpoint{respond: respond} | |
| e.server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| defer ginkgo.GinkgoRecover() | |
| Expect(r.URL.Path).To(Equal("/token")) | |
| Expect(r.ParseForm()).To(Succeed()) | |
| Expect(r.Form.Get("grant_type")).To(Equal("refresh_token")) | |
| presented := r.Form.Get("refresh_token") | |
| e.mu.Lock() | |
| e.requests = append(e.requests, presented) | |
| e.mu.Unlock() | |
| e.respond(w, presented) | |
| })) | |
| return e | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@clientcmd/refresh_test.go` around lines 26 - 41, Add defer
ginkgo.GinkgoRecover() at the start of the httptest server HTTP handler in
newTokenEndpoint, and in the handler-based assertion paths such as
writeRotatedTokens and the os.Chmod assertion, so Gomega failures from server
goroutines are recovered and reported to the spec.
| func contextTokenEndpoint(cfg *MCConfig, mcCtx *MCContext) (string, error) { | ||
| if mcCtx.Endpoints != nil && mcCtx.Endpoints.TokenEndpoint != "" { | ||
| return mcCtx.Endpoints.TokenEndpoint, nil | ||
| } | ||
| ctx := cfg.GetContext(name) | ||
| if ctx == nil { | ||
| return | ||
|
|
||
| endpoints, err := discoverOIDCEndpoints(mcCtx.Server) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| ctx.SetOIDCTokens(tokens) | ||
| if err := SaveConfig(cfg); err != nil { | ||
| fmt.Fprintf(os.Stderr, "failed to update context OIDC tokens: %v\n", err) | ||
| mcCtx.Endpoints = endpoints | ||
|
|
||
| if stored := cfg.GetContext(mcCtx.Name); stored != nil { | ||
| stored.Endpoints = endpoints | ||
| if err := saveConfigLocked(cfg); err != nil { | ||
| logger.Debugf("failed to cache OIDC endpoints for context %q: %v", mcCtx.Name, err) | ||
| } | ||
| } | ||
| return endpoints.TokenEndpoint, nil |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reject discovery results that carry no token endpoint.
discoverOIDCEndpoints returns the first candidate that answers with valid JSON. oidcclient.Discover does not require token_endpoint to be present, so a partial or unrelated JSON document produces a Discovery with an empty TokenEndpoint. Line 70 then returns "", nil, and refreshContextToken posts the refresh token to an empty URL. The empty endpoint is also cached in config.json, so every later refresh repeats the failure.
Fail early when the discovered token endpoint is empty.
🐛 Proposed fix
endpoints, err := discoverOIDCEndpoints(mcCtx.Server)
if err != nil {
return "", err
}
+ if endpoints.TokenEndpoint == "" {
+ return "", fmt.Errorf("OIDC discovery for %s returned no token_endpoint", mcCtx.Server)
+ }
mcCtx.Endpoints = endpoints📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func contextTokenEndpoint(cfg *MCConfig, mcCtx *MCContext) (string, error) { | |
| if mcCtx.Endpoints != nil && mcCtx.Endpoints.TokenEndpoint != "" { | |
| return mcCtx.Endpoints.TokenEndpoint, nil | |
| } | |
| ctx := cfg.GetContext(name) | |
| if ctx == nil { | |
| return | |
| endpoints, err := discoverOIDCEndpoints(mcCtx.Server) | |
| if err != nil { | |
| return "", err | |
| } | |
| ctx.SetOIDCTokens(tokens) | |
| if err := SaveConfig(cfg); err != nil { | |
| fmt.Fprintf(os.Stderr, "failed to update context OIDC tokens: %v\n", err) | |
| mcCtx.Endpoints = endpoints | |
| if stored := cfg.GetContext(mcCtx.Name); stored != nil { | |
| stored.Endpoints = endpoints | |
| if err := saveConfigLocked(cfg); err != nil { | |
| logger.Debugf("failed to cache OIDC endpoints for context %q: %v", mcCtx.Name, err) | |
| } | |
| } | |
| return endpoints.TokenEndpoint, nil | |
| func contextTokenEndpoint(cfg *MCConfig, mcCtx *MCContext) (string, error) { | |
| if mcCtx.Endpoints != nil && mcCtx.Endpoints.TokenEndpoint != "" { | |
| return mcCtx.Endpoints.TokenEndpoint, nil | |
| } | |
| endpoints, err := discoverOIDCEndpoints(mcCtx.Server) | |
| if err != nil { | |
| return "", err | |
| } | |
| if endpoints.TokenEndpoint == "" { | |
| return "", fmt.Errorf("OIDC discovery for %s returned no token_endpoint", mcCtx.Server) | |
| } | |
| mcCtx.Endpoints = endpoints | |
| if stored := cfg.GetContext(mcCtx.Name); stored != nil { | |
| stored.Endpoints = endpoints | |
| if err := saveConfigLocked(cfg); err != nil { | |
| logger.Debugf("failed to cache OIDC endpoints for context %q: %v", mcCtx.Name, err) | |
| } | |
| } | |
| return endpoints.TokenEndpoint, nil |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@clientcmd/whoami_token.go` around lines 53 - 70, Update contextTokenEndpoint
to validate endpoints.TokenEndpoint immediately after discoverOIDCEndpoints
returns, returning an error when it is empty before assigning or caching the
discovery result. Preserve the existing endpoint caching and successful return
behavior for valid token endpoints.
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| request := requests.Add(1) | ||
| Expect(len(r.URL.RawQuery)).To(BeNumerically("<=", testAccessQueryLimit)) | ||
| w.Header().Set("Content-Type", "application/json") | ||
| w.Header().Set("Content-Range", "0-0/2") | ||
| _, _ = fmt.Fprintf(w, `[{"config_id":%q,"config_name":"config","external_user_id":%q,"user":"user-%02d","role":"Reader"}]`, config1, userOne, 100-request) | ||
| })) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Ginkgo v2 GinkgoRecover required assertions in goroutines
💡 Result:
In Ginkgo v2, using defer GinkgoRecover() is required at the top of any goroutine that executes Ginkgo-related assertions or calls Fail() [1][2]. The reason for this requirement is that Ginkgo handles test failures by calling Fail(), which internally triggers a panic to immediately stop the execution of the current spec [1][2]. While Ginkgo can normally catch this panic to report the failure and safely transition to the next test, it cannot intercept panics originating from a different goroutine [3][4]. Without defer GinkgoRecover(), a failure inside a goroutine will cause the entire test suite to panic and crash [4][2]. When you place defer GinkgoRecover() at the start of a spawned goroutine, it provides a hook for Ginkgo to catch the panic, associate the failure with the correct spec, and prevent the entire suite from crashing [3][1]. Key best practices include: - Always use defer GinkgoRecover() in any goroutine where Gomega assertions or Fail() might be called [1][2]. - When waiting for a goroutine to complete in a test, ensure you use proper synchronization methods (like channels or Eventually matchers) to avoid blocking the main test thread indefinitely or leaking resources [2][5]. - If you find yourself frequently needing to add this to production code, it may be an indication to refactor the code to return errors instead of relying on panics or inline assertions [3][4]. For specialized cases, such as custom test helpers, Ginkgo provides tools like GinkgoHelper() to ensure that stack traces and failure locations are reported correctly [2].
Citations:
- 1: https://github.com/containers/podman/blob/f0f740a4/vendor/github.com/onsi/ginkgo/v2/core_dsl.go
- 2: https://onsi.github.io/ginkgo/
- 3: when using concurrency, a test case failure in a background go routine does not show which test case failed onsi/ginkgo#1392
- 4: Panic in a goroutine crashes test suite onsi/ginkgo#480
- 5: GinkgoRecover not aborting the current It test node when Expect in goroutine fails onsi/ginkgo#1114
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the reviewed file context and relevant imports.
if [ -f sdk/access_batch_test.go ]; then
echo "== file exists =="
wc -l sdk/access_batch_test.go
echo
echo "== imports and target section =="
sed -n '1,110p' sdk/access_batch_test.go | cat -n
else
echo "sdk/access_batch_test.go not found"
echo "matching files:"
fd -i 'access_batch_test.go' .
fi
echo
echo "== imports from sdk/access_batch_test.go =="
rg -n '^import|^[[:space:]]*(ginkgo|gomega|httptest|http|net/http)/[^ (]|"github.com/(onsi/ginkgo/v2|onsi/gomega|flasksource/[^"]+)"' sdk/access_batch_test.go || true
echo
echo "== testAccessQueryLimit definition =="
rg -n 'testAccessQueryLimit|AccessQueryLimit|RAW_QUERY_LIMIT' sdk/access_batch_test.go sdk || true
echo
echo "== server HTTP handler context in reviewed file =="
rg -n -C 4 'httptest.NewServer|http.HandlerFunc|testAccessQueryLimit|GinkgoRecover|Expect\(len\(r\.URL\.RawQuery\)\)' sdk/access_batch_test.go || trueRepository: flanksource/mission-control
Length of output: 5928
Add defer ginkgo.GinkgoRecover() to the Gomega assertion handler.
The HTTP handler runs in a separate goroutine, and a Gomega assertion failing there still panics outside Ginkgo’s main goroutine. Add defer ginkgo.GinkgoRecover() at the top of this handler.
🐛 Proposed fix
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ defer ginkgo.GinkgoRecover()
request := requests.Add(1)
Expect(len(r.URL.RawQuery)).To(BeNumerically("<=", testAccessQueryLimit))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| request := requests.Add(1) | |
| Expect(len(r.URL.RawQuery)).To(BeNumerically("<=", testAccessQueryLimit)) | |
| w.Header().Set("Content-Type", "application/json") | |
| w.Header().Set("Content-Range", "0-0/2") | |
| _, _ = fmt.Fprintf(w, `[{"config_id":%q,"config_name":"config","external_user_id":%q,"user":"user-%02d","role":"Reader"}]`, config1, userOne, 100-request) | |
| })) | |
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| defer ginkgo.GinkgoRecover() | |
| request := requests.Add(1) | |
| Expect(len(r.URL.RawQuery)).To(BeNumerically("<=", testAccessQueryLimit)) | |
| w.Header().Set("Content-Type", "application/json") | |
| w.Header().Set("Content-Range", "0-0/2") | |
| _, _ = fmt.Fprintf(w, `[{"config_id":%q,"config_name":"config","external_user_id":%q,"user":"user-%02d","role":"Reader"}]`, config1, userOne, 100-request) | |
| })) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sdk/access_batch_test.go` around lines 77 - 83, Add defer
ginkgo.GinkgoRecover() at the start of the httptest HTTP handler passed to
http.HandlerFunc, before the request counter and Gomega assertion, so assertion
failures from the handler goroutine are recovered by Ginkgo.
| func nameOrEmailFilter(value string) string { | ||
| return fmt.Sprintf("(name.ilike.*%s*,email.ilike.*%s*,aliases.cs.{%s})", value, value, value) | ||
| } | ||
|
|
||
| func nameOrAliasFilter(value string) string { | ||
| return fmt.Sprintf("(name.ilike.*%s*,aliases.cs.{%s})", value, value) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Escape the interpolated value in the PostgREST or filter.
value goes straight into the or=(...) expression. A ,, (, ) or . in the argument changes the structure of the filter and adds or breaks disjuncts. Aliases and display names can contain these characters. The deleted_at filter is a separate AND parameter, so this does not widen access, but it produces wrong matches or opaque PostgREST errors.
Wrap each value in PostgREST double quotes and escape embedded quotes and backslashes.
🛠️ Proposed fix
+// pgQuote quotes a value for use inside a PostgREST logical filter, where
+// commas and parentheses are structural.
+func pgQuote(value string) string {
+ return `"` + strings.NewReplacer(`\`, `\\`, `"`, `\"`).Replace(value) + `"`
+}
+
func nameOrEmailFilter(value string) string {
- return fmt.Sprintf("(name.ilike.*%s*,email.ilike.*%s*,aliases.cs.{%s})", value, value, value)
+ quoted := pgQuote("*" + value + "*")
+ return fmt.Sprintf("(name.ilike.%s,email.ilike.%s,aliases.cs.{%s})", quoted, quoted, pgQuote(value))
}
func nameOrAliasFilter(value string) string {
- return fmt.Sprintf("(name.ilike.*%s*,aliases.cs.{%s})", value, value)
+ return fmt.Sprintf("(name.ilike.%s,aliases.cs.{%s})", pgQuote("*"+value+"*"), pgQuote(value))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func nameOrEmailFilter(value string) string { | |
| return fmt.Sprintf("(name.ilike.*%s*,email.ilike.*%s*,aliases.cs.{%s})", value, value, value) | |
| } | |
| func nameOrAliasFilter(value string) string { | |
| return fmt.Sprintf("(name.ilike.*%s*,aliases.cs.{%s})", value, value) | |
| } | |
| // pgQuote quotes a value for use inside a PostgREST logical filter, where | |
| // commas and parentheses are structural. | |
| func pgQuote(value string) string { | |
| return `"` + strings.NewReplacer(`\`, `\\`, `"`, `\"`).Replace(value) + `"` | |
| } | |
| func nameOrEmailFilter(value string) string { | |
| quoted := pgQuote("*" + value + "*") | |
| return fmt.Sprintf("(name.ilike.%s,email.ilike.%s,aliases.cs.{%s})", quoted, quoted, pgQuote(value)) | |
| } | |
| func nameOrAliasFilter(value string) string { | |
| return fmt.Sprintf("(name.ilike.%s,aliases.cs.{%s})", pgQuote("*"+value+"*"), pgQuote(value)) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sdk/access_identity.go` around lines 375 - 381, Update nameOrEmailFilter and
nameOrAliasFilter to escape each interpolated value for PostgREST filter syntax:
escape embedded backslashes and double quotes, then wrap the value in PostgREST
double quotes before inserting it into the ilike and aliases expressions. Reuse
the escaped value consistently across every occurrence.
| var out []T | ||
| for start := 0; start < len(ids); start += accessIDBatchSize { | ||
| end := min(start+accessIDBatchSize, len(ids)) | ||
|
|
||
| batchParams := url.Values{} | ||
| for key, values := range params { | ||
| batchParams[key] = values | ||
| } | ||
| batchParams.Set(column, inList(ids[start:end])) | ||
|
|
||
| var batch []T | ||
| if _, err := c.pgGet(ctx, table, batchParams, &batch); err != nil { | ||
| return nil, err | ||
| } | ||
| out = append(out, batch...) | ||
| } | ||
| return out, nil |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Merged results lose the global ordering across batches.
Each batch request carries order=name, so PostgREST sorts inside a batch only. out is the concatenation of the batches, so the final slice is not sorted globally. ListExternalUsers, ListExternalGroups and ListExternalRoles all pass order=name and the Faro commands default to --limit 500, so any name filter that matches more than accessIDBatchSize ids returns a visibly unsorted list.
Sort the merged slice after the loop. pgGetIn is generic, so accept an optional comparison function, or sort in each caller.
🛠️ Sketch of a fix
-func pgGetIn[T any](ctx context.Context, c *Client, table, column string, ids []string, params url.Values) ([]T, error) {
+func pgGetIn[T any](ctx context.Context, c *Client, table, column string, ids []string, params url.Values, less ...func(a, b T) int) ([]T, error) {
ids = uniqueIDs(ids)
@@
out = append(out, batch...)
}
+ if len(less) > 0 && len(ids) > accessIDBatchSize {
+ slices.SortStableFunc(out, less[0])
+ }
return out, nil
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sdk/access_identity.go` around lines 399 - 415, Update the generic pgGetIn
batching flow to restore global ordering after concatenating batches: accept an
optional comparison function and sort the merged out slice after the loop when
provided. Pass the name comparison from ListExternalUsers, ListExternalGroups,
and ListExternalRoles, while preserving existing behavior for callers without
ordering requirements.
| func pgRoutes(bodies map[string]string, seen map[string]url.Values) *httptest.Server { | ||
| return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| Expect(r.Method).To(Equal(http.MethodGet)) | ||
| table := strings.TrimPrefix(r.URL.Path, "/db/") | ||
| body, ok := bodies[table] | ||
| if !ok { | ||
| ginkgo.Fail("unexpected request: " + r.URL.Path) | ||
| } | ||
| if seen != nil { | ||
| seen[table] = r.URL.Query() | ||
| } | ||
| w.Header().Set("Content-Type", "application/json") | ||
| _, _ = w.Write([]byte(body)) | ||
| })) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assertions run on httptest handler goroutines without a Ginkgo recovery point. Both helpers call Expect or ginkgo.Fail inside the HTTP handler. The httptest server runs that handler on its own goroutine. Ginkgo reports a failure by panicking, so a failed assertion there crashes the suite instead of failing the spec, and the reported location is wrong.
sdk/access_test.go#L30-L44: adddefer ginkgo.GinkgoRecover()as the first statement of thepgRouteshandler, and apply the same change to the inline handlers at lines 89-94, 105-111 and 415-421.sdk/access_filter_test.go#L26-L41: adddefer ginkgo.GinkgoRecover()as the first statement of theidentityRouteshandler.
📍 Affects 2 files
sdk/access_test.go#L30-L44(this comment)sdk/access_filter_test.go#L26-L41
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sdk/access_test.go` around lines 30 - 44, Add defer ginkgo.GinkgoRecover() as
the first statement in the pgRoutes HTTP handler and the inline HTTP handlers at
sdk/access_test.go lines 89-94, 105-111, and 415-421. Also add it as the first
statement in the identityRoutes handler at sdk/access_filter_test.go lines
26-41, so assertions from server goroutines report through Ginkgo correctly.
| if o.User != "" { | ||
| params.Set("or", fmt.Sprintf(`(user.ilike.*%s*,email.ilike.*%s*)`, o.User, o.User)) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
PostgREST filter value double quotes escaping reserved characters comma parenthesis
💡 Result:
To handle PostgREST reserved characters in filter values—specifically commas (,), periods (.), colons (:), asterisks (*), and parentheses (, )—you must enclose the value in double quotes [1][2]. For example, to filter a value containing a comma, use:?name=in.(%22Hebdon,John%22,%22Williams,Mary%22) [1][3] If your HTTP client or library automatically encodes URLs (such as axios), you can often use double quotes directly instead of their percent-encoded form (%22):?name=in.("Hebdon,John","Williams,Mary") [1][2] Escaping Double Quotes and Backslashes: If a value itself contains a double quote character, you can escape it using a backslash ("). A literal backslash can be represented by a double backslash (\). Note that when used in a URL, these should be percent-encoded (e.g., %5C for backslash) [1][2]. Example for escaping special characters within a value:?name=in.(%22Quote:%5C%22%22,%22Backslash:%5C%5C%22) [1][2] This results in the values: Quote:" and Backslash:\ [1][2].
Citations:
- 1: https://docs.postgrest.org/en/stable/references/api/url_grammar.html
- 2: https://docs.postgrest.org/en/latest/references/api/url_grammar.html
- 3: https://docs.postgrest.org/en/v12/references/api/url_grammar.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map and inspect the relevant Go file and nearby call sites.
git ls-files | rg '(^|/)access\.go$|sdk/access\.go' || true
printf '\n--- sdk/access.go outline ---\n'
ast-grep outline sdk/access.go --view compact || true
printf '\n--- sdk/access.go lines 1-140 ---\n'
sed -n '1,140p' sdk/access.go | nl -ba
printf '\n--- references to User / ilike / PostgREST in sdk/access.go ---\n'
rg -n 'User|ilike|PostgREST|or|params\.Set|fmt\.Sprintf' sdk/access.go || trueRepository: flanksource/mission-control
Length of output: 479
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- sdk/access.go first 180 lines ---\n'
sed -n '1,180p' sdk/access.go
printf '\n--- references to User / ilike / PostgREST in sdk/access.go ---\n'
rg -n 'User|ilike|PostgREST|or|params\.Set|fmt\.Sprintf' sdk/access.go || true
printf '\n--- call sites for sdk User filter ---\n'
rg -n 'access|User:\s*|\.User' . --glob '*.go' --glob '!vendor/**' --glob '!node_modules/**' | head -200Repository: flanksource/mission-control
Length of output: 256
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- sdk/access.go first 180 lines ---'
sed -n '1,180p' sdk/access.go
printf '%s\n' ''
printf '%s\n' '--- references to User / ilike / PostgREST in sdk/access.go ---'
rg -n 'User|ilike|PostgREST|or|params\.Set|fmt\.Sprintf' sdk/access.go || true
printf '%s\n' ''
printf '%s\n' '--- call sites for sdk User filter ---'
rg -n 'access|User:\s*|\.User' . --glob '*.go' --glob '!vendor/**' --glob '!node_modules/**' | head -200Repository: flanksource/mission-control
Length of output: 28849
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- sdk/access.go pgGet implementation ---'
sed -n '294,370p' sdk/access.go
printf '%s\n' ''
printf '%s\n' '--- ListAccessGrants call locations ---'
rg -n 'ListAccessGrants|AccessGrantOptions|User:' . --glob '*.go' --glob '!vendor/**' --glob '!node_modules/**' | head -200
printf '%s\n' ''
python3 - <<'PY'
import urllib.parse
base = 'http://demo.postgrest.org/config_access_summary?deleted_at=is.null&order=config_name,user'
for user in ['alice', 'a,role.eq.Owner', 'a)eq.Owner', 'a.b']:
params = urllib.parse.urlencode({
'or': f'(user.ilike.*{user}*,email.ilike.*{user}*)',
}, delimiter=';&')
print(user, '=>', base + '&' + params)
PYRepository: flanksource/mission-control
Length of output: 7614
Escape o.User in the PostgREST or filter.
ListAccessGrants sends o.User directly into user.ilike.*...*; values containing reserved characters such as ,, ), :, or . change the filter structure. Double-quote the wildcatted value and escape embedded quotes/backslashes.
🛠️ Proposed escaping helper
if o.User != "" {
- params.Set("or", fmt.Sprintf(`(user.ilike.*%s*,email.ilike.*%s*)`, o.User, o.User))
+ pattern := quotePostgRESTValue("*" + o.User + "*")
+ params.Set("or", fmt.Sprintf(`(user.ilike.%s,email.ilike.%s)`, pattern, pattern))
}Add the helper:
// quotePostgRESTValue double-quotes a filter value so reserved characters
// (",", ")", ".") are matched literally.
func quotePostgRESTValue(v string) string {
return `"` + strings.NewReplacer(`\`, `\\`, `"`, `\"`).Replace(v) + `"`
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if o.User != "" { | |
| params.Set("or", fmt.Sprintf(`(user.ilike.*%s*,email.ilike.*%s*)`, o.User, o.User)) | |
| } | |
| if o.User != "" { | |
| pattern := quotePostgRESTValue("*" + o.User + "*") | |
| params.Set("or", fmt.Sprintf(`(user.ilike.%s,email.ilike.%s)`, pattern, pattern)) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sdk/access.go` around lines 67 - 69, Update ListAccessGrants to pass the
o.User wildcard value through a quotePostgRESTValue helper before constructing
the PostgREST or filter. Add the helper using strings.NewReplacer to escape
backslashes and double quotes, then wrap the value in double quotes so reserved
characters remain literal.
1 participant
What
Why
Summary by CodeRabbit