feat(access): Add identity export - #3399
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.
Make authentication diagnostics validate refreshed OIDC credentials by default. This changes whoami to consume single-use refresh tokens unless disabled explicitly. BREAKING CHANGE: whoami now refreshes OIDC tokens by default
…tics Provide governance consumers with deterministic identity-register exports containing external users, memberships, and access grants. Bound each whoami endpoint attempt independently and report all failed candidates, making authentication failures actionable.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
WalkthroughThe pull request adds credential-store support and structured OIDC refresh handling, introduces SDK and Faro access reporting commands, expands query filtering, and embeds build metadata in Faro binaries. ChangesCredential storage and authentication
Access SDK and Faro commands
Query, CLI, and build updates
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: 9
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (13)
Makefile-5-6 (1)
5-6: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEvaluate build metadata once per Make invocation.
BUILD_DATE =executesdateeach time Make expands the variable. The platform builds can therefore contain different timestamps from the same build invocation.Use immediate expansion for both metadata values.
Proposed fix
-GIT_COMMIT = $(shell git rev-parse HEAD 2>/dev/null || echo none) -BUILD_DATE = $(shell date -u "+%Y-%m-%dT%H:%M:%SZ") +GIT_COMMIT := $(shell git rev-parse HEAD 2>/dev/null || echo none) +BUILD_DATE := $(shell date -u "+%Y-%m-%dT%H:%M:%SZ")🤖 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 `@Makefile` around lines 5 - 6, Update the GIT_COMMIT and BUILD_DATE assignments in the Makefile to use immediate expansion so each shell command runs once when Make starts, ensuring all targets in one invocation share identical build metadata.Source: Linters/SAST tools
sdk/access_history_test.go-15-20 (1)
15-20: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSynchronize
seenmap access inpgRoutes.
pgRouteswritesseen[table] = r.URL.Query()inside thehttptesthandler goroutine, whileaccess_history_test.goreadsseen["config_access_logs"]afterListAccessLogsreturns.seenmust be guarded with a mutex, or replaced with a synchronization-safe channel/result value, before it is accessed from both goroutines.🤖 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_history_test.go` around lines 15 - 20, Guard concurrent access to the seen map used by pgRoutes in the access-history test. Add synchronization around the handler’s seen[table] write and the subsequent reads of seen["config_access_logs"], ensuring ListAccessLogs completes before retrieving the recorded query.faro/access_export_test.go-83-95 (1)
83-95: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe assertion does not verify the behavior the spec name states.
The spec name says the tenant key is omitted entirely, and the comment cites a register schema that rejects an empty tenant. The assertion only checks that the in-memory
Tenantfield is empty. Omission depends on theomitemptytag onRegisterIdentity.Tenant, which this spec never exercises. Marshal the entry and assert the key is absent.💚 Proposed fix
Expect(entries[0].Tenant).To(BeEmpty()) + encoded, err := json.Marshal(entries[0]) + Expect(err).ToNot(HaveOccurred()) + Expect(string(encoded)).ToNot(ContainSubstring(`"tenant"`))🤖 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_export_test.go` around lines 83 - 95, Update the test around projectRegisterIdentities to marshal the returned entry and assert the serialized object omits the tenant key entirely, rather than only checking entries[0].Tenant is empty. Preserve the existing empty-tenant setup and error assertion, and validate absence of the key in the marshaled output.sdk/access_batch_test.go-77-83 (1)
77-83: 🩺 Stability & Availability | 🟡 MinorAdd
defer ginkgo.GinkgoRecover()to the handler goroutine.Line 79 runs a Gomega assertion inside the
httptesthandler. That handler runs on a goroutine the spec does not own. If the assertion fails, Ginkgo panics on the foreign goroutine, and the test failure can be reported incorrectly or crash the suite. Placedefer ginkgo.GinkgoRecover()at the top of the handler before any assertions.🤖 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() as the first statement in the httptest handler passed to http.HandlerFunc, before the request counter and Gomega assertion, so failures from that handler goroutine are correctly reported by Ginkgo.sdk/access_identity.go-393-416 (1)
393-416: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
pgGetIndrops the requested ordering across batches.Callers set
order=nameinparams. The server applies that order inside each batch of 100 ids.pgGetInthen concatenates the batches, so the merged slice is ordered only within each batch. Any caller with more than 100 ids returns rows in an order the caller did not request.This affects
ListExternalUsers(Line 79),ListExternalGroups(Line 96),ListExternalRoles(Line 112),GetGroupsForUser(Line 251), andGetRoleHolders(Lines 296 and 299).Sort the merged slice, or document that
pgGetIndoes not preserve order and sort at each call site.🤖 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 393 - 416, Update pgGetIn to preserve the ordering requested through params across all batches by sorting the merged out slice after accumulation, using the existing order semantics or comparator. Ensure callers including ListExternalUsers, ListExternalGroups, ListExternalRoles, GetGroupsForUser, and GetRoleHolders receive globally ordered results for requests such as order=name.faro/access_filter_test.go-39-56 (1)
39-56: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
DeferCleanuprestores only part of the mutated global state.The spec sets
permissionsByUser,permissionsByConfig,permissionsExpandGroups,permissionsLimit,accessLogsLimit, andaccessReviewsLimit, but the cleanup does not reset them. Those values leak into every later spec in the package and make results depend on spec order.♻️ Proposed change
ginkgo.DeferCleanup(func() { permissionsUser = "" + permissionsByUser = false + permissionsByConfig = false + permissionsExpandGroups = false + permissionsLimit = 0 accessLogsUser = "" accessLogsSince = "90d" + accessLogsLimit = 0 accessReviewsUser = "" accessReviewsSince = "90d" + accessReviewsLimit = 0 })Confirm the correct zero values against the
init()flag defaults forAccessLogsandAccessReviews.🤖 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_filter_test.go` around lines 39 - 56, Update the DeferCleanup callback in the affected access-filter spec setup to restore every mutated global: reset permissionsByUser, permissionsByConfig, and permissionsExpandGroups to their init() defaults, permissionsLimit to its default limit, and accessLogsLimit and accessReviewsLimit to the corresponding init() flag defaults. Keep the existing resets for the user and since values.faro/access_test.go-35-48 (1)
35-48: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCall
ginkgo.Failonly from the spec goroutine.
accessServerrunsginkgo.Failinside thehttptesthandler goroutine. If an unexpected table reaches this handler, adddefer ginkgo.GinkgoRecover()inside the handler so Ginkgo can report the panic as the current spec failure, then return instead of continuing to readbodyor write a response.🤖 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, Update the HTTP handler in accessServer to defer ginkgo.GinkgoRecover(), and when the table lookup fails, call ginkgo.Fail and return immediately. Prevent the handler from reading the missing body or writing a response after the failure.clientcmd/context.go-193-208 (1)
193-208: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
ReauthErrorpanics on a nil receiver.
HasAuthis nil-safe and returns false for a nil*MCContext.ReauthErrordereferencesc.Name,c.NeedsReauth, andc.Serverwith no nil guard. A caller that pairs the two, for exampleif !ctx.HasAuth() { return ctx.ReauthError() }, panics whenctxis nil.🛡️ Proposed guard
func (c *MCContext) ReauthError() error { + if c == nil { + return fmt.Errorf("no context is selected\n run: %s auth login", filepath.Base(os.Args[0])) + } return fmt.Errorf("context %q needs re-authentication (%s)\n run: %s auth login --server %s", c.Name, c.NeedsReauth, filepath.Base(os.Args[0]), c.Server) }🤖 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/context.go` around lines 193 - 208, Make ReauthError nil-safe like HasAuth by guarding a nil *MCContext before accessing Name, NeedsReauth, or Server. Return an appropriate error for a nil context while preserving the existing re-authentication message for non-nil contexts.clientcmd/api_client.go-114-120 (1)
114-120: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRaise the log level when the re-authentication marker cannot be persisted.
The refresh token was rejected. If
store.Setfails, the dead refresh token stays on disk. The next command reads it and spends it again against the token endpoint. The user sees the sameinvalid_grantfailure repeatedly.logger.Debugfhides that state from the user.Log at warn level so the condition is visible without debug logging.
🔧 Proposed change
if setErr := store.Set(mcCtx.Name, cred); setErr != nil { - logger.Debugf("failed to record that context %q needs re-authentication: %v", mcCtx.Name, setErr) + logger.Warnf("failed to record that context %q needs re-authentication in the %s store; the rejected refresh token remains stored: %v", + mcCtx.Name, store.Name(), setErr) }🤖 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 114 - 120, In the re-authentication handling around mcCtx.applyCredential and store.Set, change the persistence-failure log from debug level to warn level so users see when the re-authentication marker could not be saved. Keep the existing message and control flow unchanged.clientcmd/refresh_test.go-28-39 (1)
28-39: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd
defer ginkgo.GinkgoRecover()in the HTTP handler goroutine.
httptest.NewServerruns this handler on a goroutine that Ginkgo did not start. The handler callsExpectat lines 29-31, and the injectedrespondfunctions callExpecttoo. If any assertion fails, the panic is not recovered and the whole suite aborts instead of failing the spec.💚 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")) Expect(r.ParseForm()).To(Succeed()) Expect(r.Form.Get("grant_type")).To(Equal("refresh_token"))🤖 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 28 - 39, Add defer ginkgo.GinkgoRecover() at the start of the httptest.NewServer HTTP handler before any Expect calls or e.respond invocation, ensuring assertion panics from the handler goroutine are recovered and reported as spec failures.auth/oidcclient/oidcclient.go-142-147 (1)
142-147: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRaise the body read limit so large error documents still parse.
The reader caps the body at 512 bytes. If a provider returns a larger JSON error document,
json.Unmarshalfails on the truncated bytes andCodestays empty.Terminal()then returns false for a realinvalid_grant, andrefreshContextTokeninclientcmd/api_client.gokeeps the dead refresh token instead of marking the context for re-authentication.Read a larger limit for parsing and truncate only the message body.
🐛 Proposed fix
if resp.StatusCode != http.StatusOK { - body, _ := io.ReadAll(io.LimitReader(resp.Body, 512)) - tokenErr := &TokenError{StatusCode: resp.StatusCode, Body: strings.TrimSpace(string(body))} + body, _ := io.ReadAll(io.LimitReader(resp.Body, 8192)) + message := strings.TrimSpace(string(body)) + if len(message) > 512 { + message = message[:512] + } + tokenErr := &TokenError{StatusCode: resp.StatusCode, Body: message} _ = 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 - 147, Increase the response read limit in the non-OK branch of the OIDC client request so the full provider error JSON can be unmarshaled and fields such as TokenError.Code are preserved. Keep TokenError.Body limited to a safe message length by truncating only that stored body value after parsing, while passing the larger read buffer to json.Unmarshal.clientcmd/refresh_test.go-175-184 (1)
175-184: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRestore the directory mode with
ginkgo.DeferCleanup.The responder sets the config directory to 0500 at line 176. The restore at line 184 is a plain statement. If
refreshContextTokenpanics, or if an assertion inside the responder fails, line 184 never runs. The temporary directory then stays read-only and Ginkgo'sTempDircleanup can fail.Register the restore before the mode changes.
💚 Proposed fix
// The store goes read-only between the pre-flight check and the save — // the mount-went-away race the loud error exists for. + ginkgo.DeferCleanup(func() { _ = os.Chmod(configDir(), 0700) }) endpoint := newTokenEndpoint(func(w http.ResponseWriter, presented string) { Expect(os.Chmod(configDir(), 0500)).To(Succeed()) writeRotatedTokens(w, presented) }) defer endpoint.server.Close() mcCtx := seedExpiringContext("beta", endpoint.server.URL+"/token", "refresh-1") err := refreshContextToken(mcCtx) - Expect(os.Chmod(configDir(), 0700)).To(Succeed()) + Expect(os.Chmod(configDir(), 0700)).To(Succeed())🤖 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 175 - 184, In the test setup around endpoint and refreshContextToken, register the config directory mode restoration with ginkgo.DeferCleanup before the responder can change it to 0500. Remove the plain post-refresh os.Chmod restoration while preserving the existing 0700 mode.clientcmd/whoami_token.go-58-70 (1)
58-70: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReject discovery metadata that has no token endpoint.
oidcclient.Discoverdecodes the document without checking required fields. If a provider omitstoken_endpoint, this function caches an empty value inconfig.jsonand returns"", nil.refreshContextTokenthen posts to an empty URL and reports an opaque transport error. The empty value is also cached, so every later refresh repeats the failure.Validate the field before caching.
🐛 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🤖 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 58 - 70, Validate that discoverOIDCEndpoints returns a non-empty TokenEndpoint before assigning endpoints to mcCtx.Endpoints or caching it via saveConfigLocked; return an explicit error when the token endpoint is missing, while preserving the existing discovery error handling.
🧹 Nitpick comments (16)
db/middleware_test.go (1)
97-117: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDo not add this independent spec to
ginkgo.Ordered.This test has no ordered dependency. Move it to a regular
ginkgo.Describeblock. Keepginkgo.Orderedonly for sequential test steps.As per coding guidelines, “Use
ginkgo.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 `@db/middleware_test.go` around lines 97 - 117, Move the “returns malformed filters as JSON bad requests” spec out of any ginkgo.Ordered block and into a regular ginkgo.Describe block. Keep ginkgo.Ordered only around tests with sequential dependencies, preserving this spec’s assertions and behavior unchanged.Source: Coding guidelines
sdk/access_batch_test.go (1)
14-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive the test limit from the production constant.
testAccessQueryLimitduplicates the URL budget that the batching code insdk/access_batch.goenforces. If the production budget changes, this test keeps asserting against a stale number. Reference the production constant, or add a comment that states which constant this value must stay below.🤖 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 14, Update testAccessQueryLimit to derive its value from the production URL-budget constant enforced by the batching code in access_batch.go, rather than duplicating 6000. If direct reference is not possible, add a comment identifying the production constant this value must remain below.faro/access.go (1)
21-29: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
accessClientreturns an uncancellable context.
context.Background()carries no deadline and no cancellation. If the remote server stalls, every access subcommand blocks with no way to interrupt it, and Ctrl-C kills the process before in-flight work unwinds. Return a context bound toSIGINT/SIGTERM, and return its cancel function so callers can release it.♻️ Proposed change
-func accessClient() (*sdk.Client, context.Context, error) { +func accessClient() (*sdk.Client, context.Context, error) { client, err := clientcmd.RemoteClient() if err != nil { return nil, nil, err } - return client, context.Background(), nil + ctx, _ := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) + return client, ctx, 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 `@faro/access.go` around lines 21 - 29, Update accessClient to create a signal-aware context canceled by SIGINT and SIGTERM, and return the context’s cancel function alongside the client and context. Adjust every access subcommand caller to invoke the cancel function when finished, including error paths, while preserving the existing remote-client error propagation.sdk/access_filter.go (1)
51-59: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffThe candidate fetch has no server-side bound.
matchingIdentityIDsdeleteslimitand keeps onlydeleted_atand the optional type filter. On a tenant with a largeexternal_userstable, one--userfilter pulls every active row into memory before matching. The matching itself must stay client-side, but a hard safety cap or a server-side prefix narrowing for the common literal-prefix case would bound the response.🤖 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, The candidate query in matchingIdentityIDs currently removes limit without replacing it, allowing unbounded rows to be loaded into memory. Add a server-side safety cap or narrow the query for literal-prefix matches while preserving client-side matching, and ensure the existing deleted_at and optional type filtering remains intact.sdk/access_test.go (1)
30-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
defer ginkgo.GinkgoRecover()in the handler goroutine.
Expectandginkgo.Failpanic on failure. The handler runs on a goroutine owned byhttptest, andnet/httprecovers that panic. Ginkgo then never records the failure, so an unexpected request path produces a confusing 500 response instead of a clear test failure. The same applies toidentityRoutesinsdk/access_filter_test.go.♻️ Proposed change
return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + defer ginkgo.GinkgoRecover() Expect(r.Method).To(Equal(http.MethodGet))🤖 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, Update the HTTP handler in pgRoutes to call ginkgo.GinkgoRecover() at the start of its goroutine before any Expect or ginkgo.Fail calls. Apply the same recovery setup to the handler used by identityRoutes so assertion failures from both httptest servers are recorded by Ginkgo.faro/access_users.go (1)
106-122: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider one generic name/type filter pair instead of three copies. The three access entities declare identical
nameandtypefilters that differ only in the options type. A generic filter type parameterised over the options type, with a field for the key, the label, and an accessor func, would replace all six declarations and keep the labels in one place.
faro/access_users.go#L106-L122: replaceaccessUserNameFilterandaccessUserTypeFilterwith the shared generic filter, passing accessors foropts.Nameandopts.Type.faro/access_groups.go#L106-L122: replaceaccessGroupNameFilterandaccessGroupTypeFilterwith the same shared generic filter.faro/access_roles.go#L107-L123: replaceaccessRoleNameFilterandaccessRoleTypeFilterwith the same shared generic filter.🤖 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 106 - 122, Replace the duplicated name/type filter declarations with one generic filter type parameterized by the options type, storing the key, label, and accessor function while preserving the existing Lookup and Options behavior. Apply this in faro/access_users.go:106-122, faro/access_groups.go:106-122, and faro/access_roles.go:107-123, passing the appropriate Name and Type accessors for each entity and updating their filter registrations to use the shared type.clientcmd/credentials/keychain_test.go (1)
13-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
ginkgo.Label("ignore_local")to this suite.These specs require the OS keychain, which is an external service. The coding guidelines require the
ignore_locallabel for such tests. TheMC_TEST_KEYCHAINenvironment gate is useful, but it does not let the runner filter the specs by label.Also consider dropping
ginkgo.Ordered. Each spec sets its own credential and cleans up throughDeferCleanup, so no spec depends on the previous one.♻️ Proposed change
-var _ = ginkgo.Describe("KeychainStore", ginkgo.Ordered, func() { +var _ = ginkgo.Describe("KeychainStore", ginkgo.Label("ignore_local"), func() {As per 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, Add the ginkgo.Label("ignore_local") argument to the KeychainStore Describe suite so OS-keychain tests can be filtered, and remove ginkgo.Ordered because each spec independently manages credentials and cleanup.Source: Coding guidelines
clientcmd/credentials/file.go (1)
123-151: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueApply
ChmodbeforeSyncso the final mode is flushed with the data.The current order writes, syncs, then changes the mode. The mode change after
Syncis not covered by that flush. TheRenameplusfsyncDirnormally persists the metadata, so this is a durability nit rather than a defect. MovingChmodbeforeSyncremoves the gap at no cost.♻️ Proposed reorder
if _, err := f.Write(data); err != nil { _ = f.Close() return fmt.Errorf("write %s: %w", tmp, err) } - if err := f.Sync(); err != nil { - _ = f.Close() - return fmt.Errorf("sync %s: %w", tmp, err) - } if err := f.Chmod(0600); err != nil { _ = f.Close() return fmt.Errorf("chmod %s: %w", tmp, err) } + if err := f.Sync(); err != nil { + _ = f.Close() + return fmt.Errorf("sync %s: %w", tmp, err) + }🤖 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/file.go` around lines 123 - 151, In WriteAtomic, move the f.Chmod(0600) operation before f.Sync so the file mode is included in the same durability flush as the written data. Preserve the existing error handling and close, rename, and directory-sync flow.clientcmd/credentials/file_test.go (2)
222-280: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd coverage for the
WithLocktimeout path.
LockTimeoutis a package variable, so a spec can lower it and assert that a second acquisition fails with the timeout message. The current specs cover serialization, error propagation, and release, but never the bounded-wait behavior that the timeout exists for.🤖 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/file_test.go` around lines 222 - 280, Add a WithLock spec covering the timeout path: temporarily reduce the package-level LockTimeout, hold the lock in one critical section, then attempt a second acquisition and assert it fails with the expected timeout message. Restore LockTimeout afterward and preserve the existing WithLock coverage.
194-219: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueThe reader loop spins without a pause.
The loop reads
credentials.jsoncontinuously untildonecloses. It saturates one core for the duration of 50 locked writes. A short sleep keeps the same detection power at much lower cost.♻️ Proposed pause
data, err := os.ReadFile(filepath.Join(dir, credentialsFile)) if os.IsNotExist(err) { continue } Expect(err).To(Succeed()) Expect(json.Unmarshal(data, &fileContents{})).To(Succeed(), string(data)) + time.Sleep(time.Millisecond) }🤖 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/file_test.go` around lines 194 - 219, Add a short pause inside the reader loop in the “never writes a partial file that a concurrent reader can see” test, after each read/validation attempt and before the next iteration. Preserve the existing done-channel exit behavior and partial-file validation while preventing the loop from busy-spinning.clientcmd/context_credentials_test.go (2)
106-120: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
ginkgo.DeferCleanupto restore the directory mode.Line 113 uses a plain
defer. The equivalent spec inclientcmd/credentials/file_test.goLine 145 usesginkgo.DeferCleanup.DeferCleanupruns in Ginkgo's cleanup phase, so it also restores the mode when the node is interrupted or when a laterAfterEachneeds the directory writable.♻️ Proposed change
Expect(os.Chmod(configDir(), 0500)).To(Succeed()) - defer func() { _ = os.Chmod(configDir(), 0700) }() + ginkgo.DeferCleanup(func() { _ = os.Chmod(configDir(), 0700) })🤖 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/context_credentials_test.go` around lines 106 - 120, Update the cleanup in the “keeps working when the inline secrets cannot be migrated” spec to use ginkgo.DeferCleanup instead of a plain defer, restoring configDir() permissions to 0700 through Ginkgo’s cleanup phase.
122-218: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a spec for a failed
config.jsonwrite during a store change.The specs cover the successful move at Lines 174-198. They do not cover the case where
syncCredentialssucceeds and theconfig.jsonwrite then fails. That is the window described in theclientcmd/context.goSaveConfigcomment. A spec that makesconfigPath()unwritable after the credential sync would pin the intended recovery behavior.🤖 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/context_credentials_test.go` around lines 122 - 218, Add a Ginkgo spec alongside “moves credentials across when the store kind changes” that changes the credential store successfully, then makes configPath() unwritable before SaveConfig writes config.json. Assert SaveConfig returns an error and verify the intended recovery behavior: credentials remain available in the new store and the previous store is not incorrectly restored or left as the active store, matching the SaveConfig and syncCredentials contract.clientcmd/context.go (1)
87-98: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winA corrupt
credentials.jsonmakes every command fail, including the one that repairs it.
hydrateCredentialspropagates the parse error fromFileStore.load.LoadConfigreturns it unchanged. Every CLI command callsLoadConfig, so a truncated or hand-editedcredentials.jsonblocksauth loginas well. The error names the file path, so recovery is discoverable, but the user must delete the file by hand.Consider naming the recovery step in the returned error, for example instructing the user to remove the file and run
auth loginagain.🤖 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/context.go` around lines 87 - 98, Update LoadConfig’s hydrateCredentials error handling so a credentials.json parse failure returns a descriptive recovery message that instructs the user to remove the corrupt file and rerun auth login, while preserving the underlying error details and existing behavior for successful loads.clientcmd/whoami_test.go (1)
80-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDo not read
whoamiEndpointTimeoutinside the handler goroutine.Line 87 reads the package variable from the server goroutine. Line 83 writes it from the spec goroutine. The ordering works today only because
defer server.Close()runs before the restore and blocks until the slow handler returns. That coupling is not obvious, and the race detector may still flag the pair.Capture a fixed duration instead. This also removes the dependency on defer ordering.
♻️ Proposed change
restore := whoamiEndpointTimeout whoamiEndpointTimeout = 200 * time.Millisecond defer func() { whoamiEndpointTimeout = restore }() + slow := whoamiEndpointTimeout + 2*time.Second server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.URL.Path == "/api/auth/whoami" { - time.Sleep(whoamiEndpointTimeout + 2*time.Second) + time.Sleep(slow) return }🤖 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 80 - 98, In the test `does not let a slow candidate consume the next candidate's timeout`, capture the configured timeout in a local fixed-duration variable before starting the HTTP handler, and use that variable inside the handler instead of reading the package-level `whoamiEndpointTimeout`. Preserve the existing timeout setup and restore behavior while removing the cross-goroutine access.clientcmd/credentials/keychain.go (1)
45-70: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
Getcan return a credential whose refresh token is silently missing.If the keychain item is absent but
credentials.jsonstill holds the ephemeral half,secret.isZero()is true andGetreturns the file half unchanged. That credential carries a stale access token and no refresh token.MCContext.HasAuththen reports true while the credential cannot be refreshed. This happens when a user resets the login keychain or deletes the item.The failure recovers on the next refresh attempt, so this is a user-experience gap rather than data loss. Consider treating a missing keychain item with a present OIDC half as a re-auth condition.
🤖 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 45 - 70, Update KeychainStore.Get so a zero secret does not return an ephemeral credential with stale authentication data when its OIDC refresh-token half is present; instead, treat the missing keychain item as a re-authentication condition. Preserve returning cred unchanged only when no OIDC credential requires the keychain secret, and keep normal secret-merging behavior intact.clientcmd/whoami_token.go (1)
49-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument that the caller must hold the credential-store lock.
Line 66 calls
saveConfigLocked. That name states a precondition, butcontextTokenEndpointdoes not. The only current caller,refreshContextTokeninclientcmd/api_client.go, runs insidecredentials.WithLock. A future caller outside the lock would writeconfig.jsonwithout it.Add the precondition to the doc comment.
♻️ Proposed change
// contextTokenEndpoint returns the token endpoint for a context, discovering // and caching it in config.json on first use so later commands skip the two // discovery round-trips. Failing to write that cache is not fatal — it holds no // credential, and the refresh it guards is already in flight. +// +// The caller must hold the credential-store lock, because this writes config.json +// through saveConfigLocked.🤖 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 49 - 53, Update the documentation comment for contextTokenEndpoint to explicitly state that callers must hold the credential-store lock before invoking it, matching the saveConfigLocked precondition and the existing refreshContextToken usage.
🤖 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 `@clientcmd/context_credentials.go`:
- Around line 90-141: Prevent nested credential-lock acquisition across
refreshContextToken and LoadConfig: ensure the migration/save flow invoked by
LoadConfig does not call SaveConfig or credentials.WithLock while the caller
already holds the lock. Update the relevant LoadConfig,
migrateInlineCredentials, SaveConfig, and syncCredentials paths to share the
existing critical section or defer persistence until the outer lock is released,
while preserving credential migration and synchronization behavior.
In `@clientcmd/context.go`:
- Around line 103-129: Change clientcmd/context.go lines 103-129 so
syncCredentials performs the credential copy and returns a deferred purge
function, saveConfigLocked(cfg) runs before that purge, and the purge executes
only after config.json is successfully written; preserve both stores when saving
fails. Add the requested failure-path spec in
clientcmd/context_credentials_test.go lines 122-218 by making configPath()
unwritable after syncing during a store change and asserting credentials remain
readable from the store recorded in config.json.
In `@clientcmd/whoami.go`:
- Around line 227-239: Update the refresh failure branch in the whoami flow to
return the refresh error directly instead of setting out.RefreshStatus and
continuing with the expired token. Preserve the successful refresh branch, and
ensure the early return occurs before any access to refreshed OIDC fields so
re-authentication guidance and the original refresh error reach the caller.
In `@faro/access_export_test.go`:
- Around line 138-155: Replace the DescribeTable and ginkgo.Entry cases in the
“maps every user type the tenant actually reports” test with a table of case
data and a for loop containing one ginkgo.It per case. Preserve each user type,
expected identity type, setup, and assertions, and rely on Go 1.22+
loop-variable capture without copying variables.
In `@faro/access_permissions.go`:
- Around line 131-140: Update sdk.AccessGrantOptions.CanUseUserRollup to return
false whenever UserIDs or ConfigIDs are non-nil or ConfigType is non-empty,
preventing printAccessByUser from calling ListAccessSummaryByUser for filtered
requests. Add rollup tests covering each filter combination, including
--by-config with resolved users, positional configs, and config type.
In `@sdk/access_batch.go`:
- Around line 25-44: The batch helpers must apply query modifiers after merging
all pages. In sdk/access_batch.go lines 25-44, update the pgGet batch flow to
remove limit from requests when multiple batches are used, then apply the limit
after the existing merge and sort. In sdk/access_identity.go lines 393-416,
update pgGetIn to sort the merged results by the requested order column,
preserving the caller’s ordering contract.
In `@sdk/access_identity.go`:
- Around line 375-381: Update nameOrEmailFilter and nameOrAliasFilter to escape
user-provided values according to PostgREST filter grammar before embedding them
in or expressions. Quote each value so commas, dots, parentheses, braces, and
other structural characters remain literal, and escape embedded quotes and
backslashes using the SDK’s supported URL double-quote encoding, including the
aliases.cs value.
In `@sdk/access.go`:
- Around line 306-308: Update Client.pgGet in sdk/access.go (lines 306-308) to
request an exact count for access queries regardless of whether limit is set. In
faro/access_export.go (lines 239-242), retain the total returned by
ListExternalGroups and pass it to warnTruncated("groups", len(groups), total).
In faro/access_export.go (lines 325-329), ensure warnTruncated treats -1 as an
unknown total rather than comparing against it; with the exact count, report
genuine truncation.
- Around line 67-69: The user filter construction in the options query-building
logic must quote and escape o.User before inserting it into the PostgREST or
expression. Escape embedded backslashes and double quotes, then use the escaped
value inside double quotes for both ilike patterns so reserved characters remain
literal.
---
Minor comments:
In `@auth/oidcclient/oidcclient.go`:
- Around line 142-147: Increase the response read limit in the non-OK branch of
the OIDC client request so the full provider error JSON can be unmarshaled and
fields such as TokenError.Code are preserved. Keep TokenError.Body limited to a
safe message length by truncating only that stored body value after parsing,
while passing the larger read buffer to json.Unmarshal.
In `@clientcmd/api_client.go`:
- Around line 114-120: In the re-authentication handling around
mcCtx.applyCredential and store.Set, change the persistence-failure log from
debug level to warn level so users see when the re-authentication marker could
not be saved. Keep the existing message and control flow unchanged.
In `@clientcmd/context.go`:
- Around line 193-208: Make ReauthError nil-safe like HasAuth by guarding a nil
*MCContext before accessing Name, NeedsReauth, or Server. Return an appropriate
error for a nil context while preserving the existing re-authentication message
for non-nil contexts.
In `@clientcmd/refresh_test.go`:
- Around line 28-39: Add defer ginkgo.GinkgoRecover() at the start of the
httptest.NewServer HTTP handler before any Expect calls or e.respond invocation,
ensuring assertion panics from the handler goroutine are recovered and reported
as spec failures.
- Around line 175-184: In the test setup around endpoint and
refreshContextToken, register the config directory mode restoration with
ginkgo.DeferCleanup before the responder can change it to 0500. Remove the plain
post-refresh os.Chmod restoration while preserving the existing 0700 mode.
In `@clientcmd/whoami_token.go`:
- Around line 58-70: Validate that discoverOIDCEndpoints returns a non-empty
TokenEndpoint before assigning endpoints to mcCtx.Endpoints or caching it via
saveConfigLocked; return an explicit error when the token endpoint is missing,
while preserving the existing discovery error handling.
In `@faro/access_export_test.go`:
- Around line 83-95: Update the test around projectRegisterIdentities to marshal
the returned entry and assert the serialized object omits the tenant key
entirely, rather than only checking entries[0].Tenant is empty. Preserve the
existing empty-tenant setup and error assertion, and validate absence of the key
in the marshaled output.
In `@faro/access_filter_test.go`:
- Around line 39-56: Update the DeferCleanup callback in the affected
access-filter spec setup to restore every mutated global: reset
permissionsByUser, permissionsByConfig, and permissionsExpandGroups to their
init() defaults, permissionsLimit to its default limit, and accessLogsLimit and
accessReviewsLimit to the corresponding init() flag defaults. Keep the existing
resets for the user and since values.
In `@faro/access_test.go`:
- Around line 35-48: Update the HTTP handler in accessServer to defer
ginkgo.GinkgoRecover(), and when the table lookup fails, call ginkgo.Fail and
return immediately. Prevent the handler from reading the missing body or writing
a response after the failure.
In `@Makefile`:
- Around line 5-6: Update the GIT_COMMIT and BUILD_DATE assignments in the
Makefile to use immediate expansion so each shell command runs once when Make
starts, ensuring all targets in one invocation share identical build metadata.
In `@sdk/access_batch_test.go`:
- Around line 77-83: Add defer ginkgo.GinkgoRecover() as the first statement in
the httptest handler passed to http.HandlerFunc, before the request counter and
Gomega assertion, so failures from that handler goroutine are correctly reported
by Ginkgo.
In `@sdk/access_history_test.go`:
- Around line 15-20: Guard concurrent access to the seen map used by pgRoutes in
the access-history test. Add synchronization around the handler’s seen[table]
write and the subsequent reads of seen["config_access_logs"], ensuring
ListAccessLogs completes before retrieving the recorded query.
In `@sdk/access_identity.go`:
- Around line 393-416: Update pgGetIn to preserve the ordering requested through
params across all batches by sorting the merged out slice after accumulation,
using the existing order semantics or comparator. Ensure callers including
ListExternalUsers, ListExternalGroups, ListExternalRoles, GetGroupsForUser, and
GetRoleHolders receive globally ordered results for requests such as order=name.
---
Nitpick comments:
In `@clientcmd/context_credentials_test.go`:
- Around line 106-120: Update the cleanup in the “keeps working when the inline
secrets cannot be migrated” spec to use ginkgo.DeferCleanup instead of a plain
defer, restoring configDir() permissions to 0700 through Ginkgo’s cleanup phase.
- Around line 122-218: Add a Ginkgo spec alongside “moves credentials across
when the store kind changes” that changes the credential store successfully,
then makes configPath() unwritable before SaveConfig writes config.json. Assert
SaveConfig returns an error and verify the intended recovery behavior:
credentials remain available in the new store and the previous store is not
incorrectly restored or left as the active store, matching the SaveConfig and
syncCredentials contract.
In `@clientcmd/context.go`:
- Around line 87-98: Update LoadConfig’s hydrateCredentials error handling so a
credentials.json parse failure returns a descriptive recovery message that
instructs the user to remove the corrupt file and rerun auth login, while
preserving the underlying error details and existing behavior for successful
loads.
In `@clientcmd/credentials/file_test.go`:
- Around line 222-280: Add a WithLock spec covering the timeout path:
temporarily reduce the package-level LockTimeout, hold the lock in one critical
section, then attempt a second acquisition and assert it fails with the expected
timeout message. Restore LockTimeout afterward and preserve the existing
WithLock coverage.
- Around line 194-219: Add a short pause inside the reader loop in the “never
writes a partial file that a concurrent reader can see” test, after each
read/validation attempt and before the next iteration. Preserve the existing
done-channel exit behavior and partial-file validation while preventing the loop
from busy-spinning.
In `@clientcmd/credentials/file.go`:
- Around line 123-151: In WriteAtomic, move the f.Chmod(0600) operation before
f.Sync so the file mode is included in the same durability flush as the written
data. Preserve the existing error handling and close, rename, and directory-sync
flow.
In `@clientcmd/credentials/keychain_test.go`:
- Around line 13-26: Add the ginkgo.Label("ignore_local") argument to the
KeychainStore Describe suite so OS-keychain tests can be filtered, and remove
ginkgo.Ordered because each spec independently manages credentials and cleanup.
In `@clientcmd/credentials/keychain.go`:
- Around line 45-70: Update KeychainStore.Get so a zero secret does not return
an ephemeral credential with stale authentication data when its OIDC
refresh-token half is present; instead, treat the missing keychain item as a
re-authentication condition. Preserve returning cred unchanged only when no OIDC
credential requires the keychain secret, and keep normal secret-merging behavior
intact.
In `@clientcmd/whoami_test.go`:
- Around line 80-98: In the test `does not let a slow candidate consume the next
candidate's timeout`, capture the configured timeout in a local fixed-duration
variable before starting the HTTP handler, and use that variable inside the
handler instead of reading the package-level `whoamiEndpointTimeout`. Preserve
the existing timeout setup and restore behavior while removing the
cross-goroutine access.
In `@clientcmd/whoami_token.go`:
- Around line 49-53: Update the documentation comment for contextTokenEndpoint
to explicitly state that callers must hold the credential-store lock before
invoking it, matching the saveConfigLocked precondition and the existing
refreshContextToken usage.
In `@db/middleware_test.go`:
- Around line 97-117: Move the “returns malformed filters as JSON bad requests”
spec out of any ginkgo.Ordered block and into a regular ginkgo.Describe block.
Keep ginkgo.Ordered only around tests with sequential dependencies, preserving
this spec’s assertions and behavior unchanged.
In `@faro/access_users.go`:
- Around line 106-122: Replace the duplicated name/type filter declarations with
one generic filter type parameterized by the options type, storing the key,
label, and accessor function while preserving the existing Lookup and Options
behavior. Apply this in faro/access_users.go:106-122,
faro/access_groups.go:106-122, and faro/access_roles.go:107-123, passing the
appropriate Name and Type accessors for each entity and updating their filter
registrations to use the shared type.
In `@faro/access.go`:
- Around line 21-29: Update accessClient to create a signal-aware context
canceled by SIGINT and SIGTERM, and return the context’s cancel function
alongside the client and context. Adjust every access subcommand caller to
invoke the cancel function when finished, including error paths, while
preserving the existing remote-client error propagation.
In `@sdk/access_batch_test.go`:
- Line 14: Update testAccessQueryLimit to derive its value from the production
URL-budget constant enforced by the batching code in access_batch.go, rather
than duplicating 6000. If direct reference is not possible, add a comment
identifying the production constant this value must remain below.
In `@sdk/access_filter.go`:
- Around line 51-59: The candidate query in matchingIdentityIDs currently
removes limit without replacing it, allowing unbounded rows to be loaded into
memory. Add a server-side safety cap or narrow the query for literal-prefix
matches while preserving client-side matching, and ensure the existing
deleted_at and optional type filtering remains intact.
In `@sdk/access_test.go`:
- Around line 30-44: Update the HTTP handler in pgRoutes to call
ginkgo.GinkgoRecover() at the start of its goroutine before any Expect or
ginkgo.Fail calls. Apply the same recovery setup to the handler used by
identityRoutes so assertion failures from both httptest servers are recorded by
Ginkgo.
🪄 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: 3de14e8a-40a8-4b5d-8f90-c186387f4f53
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (59)
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_export.gofaro/access_export_test.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
| func (c *MCConfig) syncCredentials(store, previous credentials.Store) error { | ||
| if c.hydrated == nil { | ||
| c.hydrated = map[string]*credentials.Credential{} | ||
| } | ||
|
|
||
| // Leaving credentials behind in the old backend would strand the secrets in | ||
| // a store the config no longer points at, and every context would silently | ||
| // look logged out. | ||
| moved := map[string]*credentials.Credential{} | ||
| if previous != nil { | ||
| logger.Debugf("moving %d credential(s) from the %s store to the %s store", len(c.hydrated), previous.Name(), store.Name()) | ||
| moved, c.hydrated = c.hydrated, map[string]*credentials.Credential{} | ||
| } | ||
|
|
||
| live := map[string]bool{} | ||
| for i := range c.Contexts { | ||
| ctx := &c.Contexts[i] | ||
| live[ctx.Name] = true | ||
|
|
||
| cred := ctx.credential() | ||
| same, err := sameCredential(c.hydrated[ctx.Name], cred) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if same { | ||
| continue | ||
| } | ||
| if err := storeCredential(store, ctx.Name, cred); err != nil { | ||
| return err | ||
| } | ||
| c.hydrated[ctx.Name] = cred | ||
| } | ||
|
|
||
| for name := range c.hydrated { | ||
| if live[name] { | ||
| continue | ||
| } | ||
| if err := store.Delete(name); err != nil { | ||
| return fmt.Errorf("remove credential for context %q from the %s store: %w", name, store.Name(), err) | ||
| } | ||
| delete(c.hydrated, name) | ||
| } | ||
|
|
||
| // Only once every credential is safely in the new store. | ||
| for name := range moved { | ||
| if err := previous.Delete(name); err != nil { | ||
| return fmt.Errorf("remove credential for context %q from the %s store: %w", name, previous.Name(), err) | ||
| } | ||
| } | ||
| c.loadedStore = c.CredentialStore | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find WithLock call sites and every LoadConfig/SaveConfig reference to check for nesting.
rg -nP --type=go -C6 '\bcredentials\.WithLock\s*\(|\bWithLock\s*\('
echo '--- LoadConfig / SaveConfig call sites ---'
rg -nP --type=go -C3 '\b(LoadConfig|SaveConfig)\s*\('Repository: flanksource/mission-control
Length of output: 165
🏁 Script executed:
#!/bin/bash
set -u
echo '--- repo files matching clientcmd/context_credentials.go ---'
fd -a 'context_credentials\.go$' . || true
echo '--- Go files mentioning migrateInlineCredentials/syncCredentials/WithLock/LoadConfig/SaveConfig ---'
rg -n -C 3 'migrateInlineCredentials|syncCredentials|WithLock|LoadConfig|SaveConfig|CredentialStore|credentials\.' . || true
echo '--- go module dependencies mentioning github.com/flanksource/duty ---'
rg -n 'github\.com/flanksource/duty' go.mod go.sum || trueRepository: flanksource/mission-control
Length of output: 50383
🏁 Script executed:
#!/bin/bash
set -u
echo '--- locate lock file ---'
fd -a 'lock\.go$' . || true
echo '--- credentials/lock.go ---'
cat -n clientcmd/credentials/lock.go
echo '--- MCConfig store/storeCredentials-related implementation ---'
sed -n '73,230p' clientcmd/context.go | cat -n -v
sed -n '1,180p' clientcmd/context_credentials.go | cat-
python3 - <<'PY'
from pathlib import Path
p=Path('clientcmd/context_credentials.go'); txt=p.read_text()
for name in ['store', 'credential', 'syncCredentials', 'migrateInlineCredentials', 'storeCredential']:
idx=txt.find(f'func {name}')
print(f'\n{name}: lines {idx}')
PY
echo '--- exact LoadConfig/SaveConfig/WithLock reference list ---'
rg -n 'credentials\.WithLock|func (LoadConfig|SaveConfig|migrateInlineCredentials|store)*' clientcmd -g '*.go'
echo '--- call sites inside WithLock callback bodies ---'
python3 - <<'PY'
from pathlib import Path
for path in Path('clientcmd').glob('*.go'):
text = path.read_text()
i = 0
while True:
pos = text.find('credentials.WithLock(configDir(), func()', i)
if pos == -1:
break
# find function closure roughly by matching braces from this block
# scan until next "go func" outside current or close function end? simple: print snippet around callback fn
start = text.rfind('\nfunc', 0, pos)+1
if start < 0:
start = 0
end = text.find('\n}', pos)+2
if end < 0:
end = min(len(text), pos+800)
print(f'== {path} ==')
print(text[start:end])
i = pos + 1
PYRepository: flanksource/mission-control
Length of output: 37083
Prevent nested credential lock acquisitions.
refreshContextToken calls LoadConfig before acquiring credentials.WithLock(configDir()), so LoadConfig can call migrateInlineCredentials and then SaveConfig must acquire the same lock. SaveConfig also calls syncCredentials inside WithLock. Treat LoadConfig as a critical-section entry that the migration/save path must not enter while the same lock is held.
🤖 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/context_credentials.go` around lines 90 - 141, Prevent nested
credential-lock acquisition across refreshContextToken and LoadConfig: ensure
the migration/save flow invoked by LoadConfig does not call SaveConfig or
credentials.WithLock while the caller already holds the lock. Update the
relevant LoadConfig, migrateInlineCredentials, SaveConfig, and syncCredentials
paths to share the existing critical section or defer persistence until the
outer lock is released, while preserving credential migration and
synchronization behavior.
| func SaveConfig(cfg *MCConfig) error { | ||
| if err := os.MkdirAll(filepath.Dir(configPath()), 0700); err != nil { | ||
| store, err := cfg.store() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| previous, err := cfg.previousStore() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return credentials.WithLock(configDir(), func() error { | ||
| if err := cfg.syncCredentials(store, previous); err != nil { | ||
| return err | ||
| } | ||
| return saveConfigLocked(cfg) | ||
| }) | ||
| } | ||
|
|
||
| func saveConfigLocked(cfg *MCConfig) error { | ||
| if err := os.MkdirAll(configDir(), 0700); err != nil { | ||
| return err | ||
| } | ||
| data, err := json.MarshalIndent(cfg, "", " ") | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return os.WriteFile(configPath(), data, 0600) | ||
| return credentials.WriteAtomic(configPath(), data) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
SaveConfig commits the credential move before config.json is durable. syncCredentials writes every credential to the new store, deletes each one from the previous store, and updates cfg.loadedStore, all before saveConfigLocked persists config.json. If the config write fails, config.json still names the old store and that store is now empty, so every context reads as logged out while the secrets sit in a store the config does not point at.
clientcmd/context.go#L103-L129: runsaveConfigLocked(cfg)before the removal from the previous store. HavesyncCredentialsreturn the purge step and invoke it only afterconfig.jsonis written, so a failed config write leaves both stores populated.clientcmd/context_credentials_test.go#L122-L218: add a spec that makesconfigPath()unwritable after the credential sync during a store change, and assert that the credentials remain readable through the store recorded inconfig.json.
📍 Affects 2 files
clientcmd/context.go#L103-L129(this comment)clientcmd/context_credentials_test.go#L122-L218
🤖 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/context.go` around lines 103 - 129, Change clientcmd/context.go
lines 103-129 so syncCredentials performs the credential copy and returns a
deferred purge function, saveConfigLocked(cfg) runs before that purge, and the
purge executes only after config.json is successfully written; preserve both
stores when saving fails. Add the requested failure-path spec in
clientcmd/context_credentials_test.go lines 122-218 by making configPath()
unwritable after syncing during a store change and asserting credentials remain
readable from the store recorded in config.json.
| default: | ||
| // --refresh spends the single-use refresh token, so it is opt-in: | ||
| // a diagnostic must not be able to consume the credential it is | ||
| // diagnosing. | ||
| if err := refreshContextToken(mcCtx); err != nil { | ||
| out.RefreshStatus = "failed: " + err.Error() | ||
| } else { | ||
| mcCtx.SetOIDCTokens(refreshed) | ||
| token = mcCtx.AccessToken() | ||
| out.RefreshStatus = "refreshed" | ||
| out.TokenExpires = formatTime(mcCtx.OIDC.ExpiresAt) | ||
| out.TokenTTL = formatTTL(mcCtx.OIDC.ExpiresAt) | ||
| updateContextOIDCTokens(cfg, mcCtx.Name, refreshed) | ||
| } | ||
| } else if oidcTokenExpiring(mcCtx.OIDC) && mcCtx.OIDC.RefreshToken == "" { | ||
| out.RefreshStatus = "unavailable: no refresh token" | ||
| } else if oidcTokenExpiring(mcCtx.OIDC) { | ||
| out.RefreshStatus = "needed" | ||
| } else { | ||
| out.RefreshStatus = "not needed" | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Report the refresh failure as the authentication error.
When refreshContextToken fails, the code records the reason in out.RefreshStatus and continues. token still holds the expired access token, so line 258 calls whoami and receives a 401. Line 261-265 then overwrites out.Status with "invalid" and out.Error with the 401 text. whoamiStatusError at line 136 prints out.Error, so the operator sees the 401 instead of the refresh failure.
The failure is also terminal for this run. If the refresh token was rejected, refreshContextToken sets mcCtx.NeedsReauth and returns the re-authentication error, but that guidance never reaches the output.
Return the refresh failure directly.
🐛 Proposed fix
default:
// --refresh spends the single-use refresh token, so it is opt-in:
// a diagnostic must not be able to consume the credential it is
// diagnosing.
if err := refreshContextToken(mcCtx); err != nil {
out.RefreshStatus = "failed: " + err.Error()
- } else {
- token = mcCtx.AccessToken()
- out.RefreshStatus = "refreshed"
- out.TokenExpires = formatTime(mcCtx.OIDC.ExpiresAt)
- out.TokenTTL = formatTTL(mcCtx.OIDC.ExpiresAt)
+ out.Status = "invalid"
+ out.Error = err.Error()
+ return out
}
+ token = mcCtx.AccessToken()
+ out.RefreshStatus = "refreshed"
+ out.TokenExpires = formatTime(mcCtx.OIDC.ExpiresAt)
+ out.TokenTTL = formatTTL(mcCtx.OIDC.ExpiresAt)
}Note: refreshContextToken sets mcCtx.OIDC = nil on a rejected grant. The early return also avoids the nil dereference that lines 236-237 would otherwise reach if the else branch were restructured differently.
📝 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.
| default: | |
| // --refresh spends the single-use refresh token, so it is opt-in: | |
| // a diagnostic must not be able to consume the credential it is | |
| // diagnosing. | |
| if err := refreshContextToken(mcCtx); err != nil { | |
| out.RefreshStatus = "failed: " + err.Error() | |
| } else { | |
| mcCtx.SetOIDCTokens(refreshed) | |
| token = mcCtx.AccessToken() | |
| out.RefreshStatus = "refreshed" | |
| out.TokenExpires = formatTime(mcCtx.OIDC.ExpiresAt) | |
| out.TokenTTL = formatTTL(mcCtx.OIDC.ExpiresAt) | |
| updateContextOIDCTokens(cfg, mcCtx.Name, refreshed) | |
| } | |
| } else if oidcTokenExpiring(mcCtx.OIDC) && mcCtx.OIDC.RefreshToken == "" { | |
| out.RefreshStatus = "unavailable: no refresh token" | |
| } else if oidcTokenExpiring(mcCtx.OIDC) { | |
| out.RefreshStatus = "needed" | |
| } else { | |
| out.RefreshStatus = "not needed" | |
| } | |
| default: | |
| // --refresh spends the single-use refresh token, so it is opt-in: | |
| // a diagnostic must not be able to consume the credential it is | |
| // diagnosing. | |
| if err := refreshContextToken(mcCtx); err != nil { | |
| out.RefreshStatus = "failed: " + err.Error() | |
| out.Status = "invalid" | |
| out.Error = err.Error() | |
| return out | |
| } | |
| token = mcCtx.AccessToken() | |
| out.RefreshStatus = "refreshed" | |
| out.TokenExpires = formatTime(mcCtx.OIDC.ExpiresAt) | |
| out.TokenTTL = formatTTL(mcCtx.OIDC.ExpiresAt) | |
| } |
🤖 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.go` around lines 227 - 239, Update the refresh failure
branch in the whoami flow to return the refresh error directly instead of
setting out.RefreshStatus and continuing with the expired token. Preserve the
successful refresh branch, and ensure the early return occurs before any access
to refreshed OIDC fields so re-authentication guidance and the original refresh
error reach the caller.
| ginkgo.DescribeTable("maps every user type the tenant actually reports", | ||
| func(userType, expected string) { | ||
| user := models.ExternalUser{ID: serviceID, Name: "principal", UserType: userType, CreatedAt: grantedAt} | ||
| entries, err := projectRegisterIdentities( | ||
| []models.ExternalUser{user}, | ||
| []sdk.AccessGrant{grant(serviceID, "AWS::Account", "")}, | ||
| nil, | ||
| ) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| Expect(entries).To(HaveLen(1)) | ||
| Expect(entries[0].IdentityType).To(Equal(expected)) | ||
| }, | ||
| ginkgo.Entry("Azure/Google directory user", "User", "person"), | ||
| ginkgo.Entry("GitHub account", "GitHub::User", "person"), | ||
| ginkgo.Entry("local Mission Control account", "local", "person"), | ||
| ginkgo.Entry("cloud service account", "ServiceAccount", "workload_identity"), | ||
| ginkgo.Entry("AWS service principal", "AWSService", "workload_identity"), | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use a for loop with ginkgo.It per case.
The coding guidelines require table-driven tests to use a for loop with one ginkgo.It per case. This block uses ginkgo.DescribeTable and ginkgo.Entry instead.
♻️ Proposed refactor
- ginkgo.DescribeTable("maps every user type the tenant actually reports",
- func(userType, expected string) {
- user := models.ExternalUser{ID: serviceID, Name: "principal", UserType: userType, CreatedAt: grantedAt}
- entries, err := projectRegisterIdentities(
- []models.ExternalUser{user},
- []sdk.AccessGrant{grant(serviceID, "AWS::Account", "")},
- nil,
- )
- Expect(err).ToNot(HaveOccurred())
- Expect(entries).To(HaveLen(1))
- Expect(entries[0].IdentityType).To(Equal(expected))
- },
- ginkgo.Entry("Azure/Google directory user", "User", "person"),
- ginkgo.Entry("GitHub account", "GitHub::User", "person"),
- ginkgo.Entry("local Mission Control account", "local", "person"),
- ginkgo.Entry("cloud service account", "ServiceAccount", "workload_identity"),
- ginkgo.Entry("AWS service principal", "AWSService", "workload_identity"),
- )
+ for _, tt := range []struct {
+ name string
+ userType string
+ expected string
+ }{
+ {"Azure/Google directory user", "User", "person"},
+ {"GitHub account", "GitHub::User", "person"},
+ {"local Mission Control account", "local", "person"},
+ {"cloud service account", "ServiceAccount", "workload_identity"},
+ {"AWS service principal", "AWSService", "workload_identity"},
+ } {
+ ginkgo.It("maps the user type of a "+tt.name, func() {
+ user := models.ExternalUser{ID: serviceID, Name: "principal", UserType: tt.userType, CreatedAt: grantedAt}
+ entries, err := projectRegisterIdentities(
+ []models.ExternalUser{user},
+ []sdk.AccessGrant{grant(serviceID, "AWS::Account", "")},
+ nil,
+ )
+ Expect(err).ToNot(HaveOccurred())
+ Expect(entries).To(HaveLen(1))
+ Expect(entries[0].IdentityType).To(Equal(tt.expected))
+ })
+ }Based on coding guidelines: "For table-driven tests, use a for loop with ginkgo.It per case; do not copy loop variables (Go 1.22+ captures them correctly)".
📝 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.
| ginkgo.DescribeTable("maps every user type the tenant actually reports", | |
| func(userType, expected string) { | |
| user := models.ExternalUser{ID: serviceID, Name: "principal", UserType: userType, CreatedAt: grantedAt} | |
| entries, err := projectRegisterIdentities( | |
| []models.ExternalUser{user}, | |
| []sdk.AccessGrant{grant(serviceID, "AWS::Account", "")}, | |
| nil, | |
| ) | |
| Expect(err).ToNot(HaveOccurred()) | |
| Expect(entries).To(HaveLen(1)) | |
| Expect(entries[0].IdentityType).To(Equal(expected)) | |
| }, | |
| ginkgo.Entry("Azure/Google directory user", "User", "person"), | |
| ginkgo.Entry("GitHub account", "GitHub::User", "person"), | |
| ginkgo.Entry("local Mission Control account", "local", "person"), | |
| ginkgo.Entry("cloud service account", "ServiceAccount", "workload_identity"), | |
| ginkgo.Entry("AWS service principal", "AWSService", "workload_identity"), | |
| ) | |
| for _, tt := range []struct { | |
| name string | |
| userType string | |
| expected string | |
| }{ | |
| {"Azure/Google directory user", "User", "person"}, | |
| {"GitHub account", "GitHub::User", "person"}, | |
| {"local Mission Control account", "local", "person"}, | |
| {"cloud service account", "ServiceAccount", "workload_identity"}, | |
| {"AWS service principal", "AWSService", "workload_identity"}, | |
| } { | |
| ginkgo.It("maps the user type of a "+tt.name, func() { | |
| user := models.ExternalUser{ID: serviceID, Name: "principal", UserType: tt.userType, CreatedAt: grantedAt} | |
| entries, err := projectRegisterIdentities( | |
| []models.ExternalUser{user}, | |
| []sdk.AccessGrant{grant(serviceID, "AWS::Account", "")}, | |
| nil, | |
| ) | |
| Expect(err).ToNot(HaveOccurred()) | |
| Expect(entries).To(HaveLen(1)) | |
| Expect(entries[0].IdentityType).To(Equal(tt.expected)) | |
| }) | |
| } |
🤖 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_export_test.go` around lines 138 - 155, Replace the DescribeTable
and ginkgo.Entry cases in the “maps every user type the tenant actually reports”
test with a table of case data and a for loop containing one ginkgo.It per case.
Preserve each user type, expected identity type, setup, and assertions, and rely
on Go 1.22+ loop-variable capture without copying variables.
Source: Coding guidelines
| func printAccessByUser(ctx context.Context, client *sdk.Client, opts sdk.AccessGrantOptions) error { | ||
| if opts.CanUseUserRollup() { | ||
| rows, total, err := client.ListAccessSummaryByUser(ctx, opts.Limit) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| warnTruncated("users", len(rows), total) | ||
| printAccessResult(AccessSummaryByUserResult{Rows: rows}, userSummaryRows(rows)) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the rollup eligibility predicates and the by-user list signature.
rg -nP --type=go -A 30 'func \(o AccessGrantOptions\) CanUseUserRollup\('
rg -nP --type=go -A 30 'func \(o AccessGrantOptions\) CanUseConfigRollup\('
rg -nP --type=go -A 20 'func \(c \*Client\) ListAccessSummaryByUser\('Repository: flanksource/mission-control
Length of output: 165
🏁 Script executed:
#!/bin/bash
set -u
printf 'Tracked files matching access_permissions.go:\n'
git ls-files | grep -F 'access_permissions.go' || true
printf '\nLocate SDK options and client methods:\n'
git ls-files | grep -E '\.go$' | xargs rg -n 'CanUseUserRollup|CanUseConfigRollup|ListAccessSummaryByUser|AccessGrantOptions|UserIDs|ConfigIDs' || true
printf '\nShow relevant file context if present:\n'
for f in $(git ls-files | grep -F 'faro/access_permissions.go' || true); do
echo "=== $f ==="
wc -l "$f"
ast-grep outline "$f" --view compact || true
sed -n '1,240p' "$f" | cat -n
doneRepository: flanksource/mission-control
Length of output: 19421
🏁 Script executed:
#!/bin/bash
set -u
printf 'SDK access options and rollup implementation:\n'
sed -n '30,225p' sdk/access.go | cat -n
printf '\nFaro access permissions implementation and tests:\n'
sed -n '1,240p' faro/access_test.go | cat -n
printf '\nFarro access_filter_test relevant user resolver tests:\n'
sed -n '1,140p' faro/access_filter_test.go | cat -n
printf '\nRead-only semantic probe of current rollup predicates:\n'
python3 - <<'PY'
import pathlib, re
sdk = pathlib.Path('sdk/access.go').read_text()
matches = re.search(r'func \(o AccessGrantOptions\) CanUseConfigRollup\(\) bool \{(?P<body>.*?)\n\}', sdk, re.S)
body = matches.group('body') if matches else ''
print(body)
def can_use_user_rollup(User, Role, UserType, GroupIDs, ConfigIDs, ConfigType):
can_config = User == "" and Role == "" and UserType == "" and len(GroupIDs) == 0
return can_config and len(ConfigIDs) == 0 and ConfigType == ""
for case in [
('populated user', {}, {'User': 'jane'}),
('populated config ids', {}, {'ConfigIDs': ['id-1']}),
('config type', {}, {'ConfigType': 'Azure::EnterpriseApplication'}),
('role', {}, {'Role': 'Owner'}),
('combined by-user filters', {}, {'User': 'jane', 'ConfigIDs': ['id-1']}),
]:
args={}
for k,v in case[1].items():
args[k] = v
print(f"{case[0]}: CanUseUserRollup={can_use_user_rollup(args.get('User',''), args.get('Role',''), args.get('UserType',''), args.get('GroupIDs',[]), args.get('ConfigIDs',[]), args.get('ConfigType',''))}")
PYRepository: flanksource/mission-control
Length of output: 22978
Reject user and config filters before using the per-user rollup.
CanUseUserRollup returns true when --by-config is selected together with a resolved --user (UserIDs), positional config query (ConfigIDs), or --config-type. In that path, printAccessByUser calls client.ListAccessSummaryByUser(ctx, opts.Limit), which only sends limit and reports every user/config instead of the narrowed grant set. Make CanUseUserRollup reject non-nil UserIDs, non-nil ConfigIDs, and non-empty ConfigType, and add rollup test coverage for those combinations.
🤖 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 131 - 140, Update
sdk.AccessGrantOptions.CanUseUserRollup to return false whenever UserIDs or
ConfigIDs are non-nil or ConfigType is non-empty, preventing printAccessByUser
from calling ListAccessSummaryByUser for filtered requests. Add rollup tests
covering each filter combination, including --by-config with resolved users,
positional configs, and config type.
| for _, batch := range batches { | ||
| var page []T | ||
| batchTotal, err := client.pgGet(ctx, table, batch, &page) | ||
| if err != nil { | ||
| return nil, 0, err | ||
| } | ||
| rows = append(rows, page...) | ||
| if batchTotal < 0 { | ||
| totalKnown = false | ||
| } else { | ||
| total += batchTotal | ||
| } | ||
| } | ||
|
|
||
| if compare != nil { | ||
| sort.SliceStable(rows, func(i, j int) bool { return compare(rows[i], rows[j]) < 0 }) | ||
| } | ||
| if limit, _ := strconv.Atoi(params.Get("limit")); limit > 0 && len(rows) > limit { | ||
| rows = rows[:limit] | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Both batch helpers merge pages without re-applying the server-side query modifiers. pgGetAccess and pgGetIn split one logical query into several requests, then concatenate the responses. Each request still carries limit and order, which the server applies per batch, so the merged slice does not satisfy the caller's original limit or order.
sdk/access_batch.go#L25-L44: striplimitfrom each batch when more than one batch exists, and apply the limit only after the merge and sort.sdk/access_identity.go#L393-L416: sort the merged slice by the requestedordercolumn, or document thatpgGetIndoes not preserve order and sort at each call site.
📍 Affects 2 files
sdk/access_batch.go#L25-L44(this comment)sdk/access_identity.go#L393-L416
🤖 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 - 44, The batch helpers must apply query
modifiers after merging all pages. In sdk/access_batch.go lines 25-44, update
the pgGet batch flow to remove limit from requests when multiple batches are
used, then apply the limit after the existing merge and sort. In
sdk/access_identity.go lines 393-416, update pgGetIn to sort the merged results
by the requested order column, preserving the caller’s ordering contract.
| 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.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
PostgREST or filter double quoted value escaping reserved characters commas
💡 Result:
To handle PostgREST reserved characters (commas, dots, colons, parentheses) within filter values, you must wrap the value in double quotes [1][2]. Because these must be passed as part of a URL, these double quotes should typically be percent-encoded as %22 [1][2]. For example, to filter a column for values that contain commas, such as Hebdon,John: curl "http://localhost:3000/employees?name=in.(%22Hebdon,John%22)" [1][2] If your HTTP client or library automatically encodes URLs (like axios), you may be able to use raw double quotes "" directly instead of %22 [1][2]. Escaping within quoted values: If the value itself needs to contain a double quote or a backslash, you can use backslash escaping [1][2]: 1. To include a double quote, use ": for example, "Quote:"" [1][2]. 2. To include a backslash, use \: for example, "Backslash:\" [1][2]. In a complete URL, these would look like this (noting that the backslash itself is %5C): curl "http://localhost:3000/marks?name=in.(%22Quote:%5C%22%22,%22Backslash:%5C%5C%22)" [1][2] Summary of reserved characters: The characters that trigger the need for double-quoting are commas (,), dots (.), colons (:), and parentheses ((,)) [1][2][3]. If a column name itself contains these characters, it must also be wrapped in double quotes [1][2].
Citations:
- 1: https://docs.postgrest.org/en/stable/references/api/url_grammar.html
- 2: https://docs.postgrest.org/en/v12/references/api/url_grammar.html
- 3: question about reserved characters PostgREST/postgrest#4254
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate sdk/access_identity.go =="
fd -a 'access_identity\.go$' . || true
echo
echo "== surrounding lines =="
if [ -f sdk/access_identity.go ]; then
nl -ba sdk/access_identity.go | sed -n '330,410p'
fi
echo
echo "== search filter usage =="
rg -n "nameOrEmailFilter|nameOrAliasFilter|or|name\.ilike|aliases\.cs" sdk/access_identity.go sdk || true
echo
echo "== PostgREST references from files =="
rg -n "postgrest|PostgREST|DB|url" -S . | head -200 || trueRepository: flanksource/mission-control
Length of output: 309
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== sdk/access_identity.go lines 340-405 =="
awk 'NR>=340 && NR<=410 {printf "%6d %s\n", NR, $0}' sdk/access_identity.go
echo
echo "== filter usages in sdk/access_identity.go =="
grep -nE "nameOrEmailFilter|nameOrAliasFilter|\|q=|filter=\|or" sdk/access_identity.go || true
echo
echo "== likely PostgREST dependency/version manifests =="
for f in go.mod go.sum sdk/go.mod sdk/go.sum; do
[ -f "$f" ] && { echo "--- $f"; grep -E "postgrest|v1[0-9]+\.v[0-9]+|PostgREST" "$f" | head -50 || true; }
doneRepository: flanksource/mission-control
Length of output: 3458
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== lines 270-330 =="
awk 'NR>=270 && NR<=330 {printf "%6d %s\n", NR, $0}' sdk/access_identity.go
echo
echo "== go.sum postgrest entries =="
if [ -f go.sum ]; then
grep -n "postgrest\|postgrest-js\|postgrest-go" go.sum || true
fiRepository: flanksource/mission-control
Length of output: 2800
Escape arguments embedded in the PostgREST or filters.
nameOrEmailFilter and nameOrAliasFilter pass raw user input into or=.... PostgREST treats commas, dots, and parentheses as filter structure, so values like a,b change the filter shape and a) can return a 400 response. aliases.cs.{%s} also needs quoted brace/string escaping for values containing {, }, or ".
Quote each value in the PostgREST URL grammar (and use the SDK/HTTP semantics the client supports for any required URL double-quote encoding), and handle embedded quotes/backslashes as PostgREST expects.
🤖 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 user-provided values according to PostgREST filter
grammar before embedding them in or expressions. Quote each value so commas,
dots, parentheses, braces, and other structural characters remain literal, and
escape embedded quotes and backslashes using the SDK’s supported URL
double-quote encoding, including the aliases.cs value.
| 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.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
PostgREST filter value double quote escaping reserved characters comma
💡 Result:
To use reserved characters (such as commas, dots, colons, parentheses, or asterisks) within filter values in PostgREST, you must wrap the value in double quotes [1][2]. Because these are URL parameters, these double quotes should be percent-encoded as %22 [1][3]. For example, to filter using the in operator with values that contain commas, use:?name=in.(%22Value,With,Comma1%22,%22Value,With,Comma2%22) [1][3] If your value itself contains a double quote character, you can escape it using a backslash (\") [1][4]. Similarly, a literal backslash can be escaped using a double backslash (\\) [1][4]. These should also be percent-encoded within the URL:?name=in.(%22Quote:%5C%22%22,%22Backslash:%5C%5C%22) [1][3] Note that if you are using an HTTP client library that automatically performs URL encoding (such as axios), you may be able to use literal double quotes (") instead of percent-encoded versions (%22) [1][5]. Always ensure that reserved characters within filter values are quoted to prevent them from being interpreted as API syntax markers [1][6].
Citations:
- 1: https://docs.postgrest.org/en/stable/references/api/url_grammar.html
- 2: https://postgrest.org/en/latest/references/api/url_grammar.html
- 3: https://postgrest.org/en/v11/references/api/url_grammar.html
- 4: Values with double quotes on the "in" operator PostgREST/postgrest#1940
- 5: https://docs.postgrest.org/en/latest/references/api/url_grammar.html
- 6: https://postgrest.org/en/stable/references/api/tables_views.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files matching access.go:"
fd -a 'access\.go$' . | sed 's#^\./##'
echo
echo "sdk/access.go context around target:"
if [ -f sdk/access.go ]; then
nl -ba sdk/access.go | sed -n '1,130p'
fi
echo
echo "Search for pgQuote and PostgREST filter handling:"
rg -n "pgQuote|params\\.Set\\(\"or\"|ilike|PostgREST|postgrest|url_grammar" . --glob '!vendor/**' --glob '!node_modules/**' | head -200
echo
echo "Git status/diff stat:"
git diff --stat || true
git diff -- sdk/access.go | sed -n '1,180p' || trueRepository: flanksource/mission-control
Length of output: 458
🏁 Script executed:
#!/bin/sh
set -euo pipefail
echo "sdk/access.go context around target:"
awk '{printf "%6d\t%s\n", NR, $0}' sdk/access.go | sed -n '1,130p'
echo
echo "Search for pgQuote and PostgREST filter handling:"
grep -RIn "pgQuote|params\.Set\\(\"or\"|or\\(\"|PostgREST|postgrest|url_grammar|double-quote|percent-encode|encoding\\.URLEncode" . --exclude-dir=vendor --exclude-dir=node_modules --exclude-dir=.git 2>/dev/null | head -200 || true
echo
echo "SDK related user filtering patterns:"
grep -RIn "o\\.User|User.*ilike|AccessGrant|AccessGrantOptions|ilike\\." sdk --exclude-dir=vendor 2>/dev/null | head -200 || true
echo
echo "Current diff for sdk/access.go:"
git diff -- sdk/access.go | sed -n '1,220p' || true
echo
echo "Behavioral probe for unescaped PostgREST or filter characters:"
python3 - <<'PY'
samples = ["alice", "a,b", "a.b", "a(b", "a)b", 'a"b', r"a\b"]
for s in samples:
raw = f"(user.ilike.*{s}*,email.ilike.*{s}*)"
print("raw:", raw)
PYRepository: flanksource/mission-control
Length of output: 5339
Quote the interpolated value in the or filter.
o.User is inserted into the PostgREST or expression without escaping. Values containing reserved characters such as ,, ., (, or ) change the predicate structure instead of matching literally. Double-quote the value and escape embedded quotes/backslashes so PostgREST treats the characters as part of the ilike 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, The user filter construction in the
options query-building logic must quote and escape o.User before inserting it
into the PostgREST or expression. Escape embedded backslashes and double quotes,
then use the escaped value inside double quotes for both ilike patterns so
reserved characters remain literal.
| if params.Get("limit") != "" { | ||
| req = req.Header("Prefer", "count=exact") | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Row totals are unreliable on the export path, so truncation can pass unreported. Client.pgGet requests Prefer: count=exact only when the caller sets a limit. Every unlimited access query therefore returns a total of -1, and access users export issues unlimited queries. A server-side db-max-rows cap then produces an incomplete identity register with no warning. This contradicts the PR objective of reliable access data.
sdk/access.go#L306-L308: requestcount=exactfor the access surfaces regardless oflimit, or paginate withRangeuntil the reported total is reached, so an unlimited query still reports the true row count.faro/access_export.go#L239-L242: capture the total fromListExternalGroupsinstead of discarding it, and pass it towarnTruncated("groups", len(groups), total).faro/access_export.go#L325-L329: confirm thatwarnTruncatedtreats a total of -1 as unknown rather than comparinglen(grants)against it; oncepgGetreturns an exact total, this call reports real truncation.
📍 Affects 2 files
sdk/access.go#L306-L308(this comment)faro/access_export.go#L239-L242faro/access_export.go#L325-L329
🤖 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 306 - 308, Update Client.pgGet in sdk/access.go
(lines 306-308) to request an exact count for access queries regardless of
whether limit is set. In faro/access_export.go (lines 239-242), retain the total
returned by ListExternalGroups and pass it to warnTruncated("groups",
len(groups), total). In faro/access_export.go (lines 325-329), ensure
warnTruncated treats -1 as an unknown total rather than comparing against it;
with the exact count, report genuine truncation.
1 participant
What
whoamicandidate independently and report all failures.Why
Summary by CodeRabbit
New Features
versioncommand showing build, commit, date, and platform information.Bug Fixes
Documentation