feat(secret): add --shared-with-me list mode for recipient discovery - #153
feat(secret): add --shared-with-me list mode for recipient discovery#153c1-squire-dev[bot] wants to merge 7 commits into
Conversation
Add cone secret list --shared-with-me, calling the caller-bound POST /api/v1/search/secrets/shared_with_me operation through the generated SDK (PaperSecret.SearchSecretsSharedWithMe). The default creator list (SearchMySecrets), its flags, help, and output stay unchanged; the new mode preserves pagination, query/status/type filters, enforces the endpoint's page_size<=100 and query<=256 limits, defaults include_own=false (opt-in via --include-own), and rejects an explicit --sharing-mode filter instead of silently dropping it, since the endpoint accepts no user_id, sort_by, or sharing_mode. Generalize the SDK-error-to-HTTPError mapping (mapPaperSecretCreateError -> mapPaperSecretError) so the shared search reports HTTP failures with the same shape as create. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
…redWithMe build Pin github.com/conductorone/conductorone-sdk-go to v1.29.1-0.20260905002051-ef0d92d9c5f2 (the speakeasy-sdk-regen branch head carrying the generated PaperSecret.SearchSecretsSharedWithMe operation and its request/response models) and re-vendor. This is the established SDK generation output from the refreshed canonical OpenAPI input (insulator now serves the C1 main canonical spec including the shared_with_me route); no generated file is hand-edited. Re-pin to the v1.29.1 tag once conductorone-sdk-go PR #117 merges and publishes. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| filippo.io/age v1.3.1 | ||
| github.com/conductorone/baton-sdk v0.3.17 | ||
| github.com/conductorone/conductorone-sdk-go v1.29.0 | ||
| github.com/conductorone/conductorone-sdk-go v1.29.1-0.20260905002051-ef0d92d9c5f2 |
There was a problem hiding this comment.
🟡 Suggestion: This pins conductorone-sdk-go to a pseudo-version on the unmerged speakeasy-sdk-regen-1784593459 branch rather than a published tag. Because that branch head is not reachable from any released ref, a force-push or branch deletion breaks go mod download/go mod verify and any non-vendored build (GOFLAGS=-mod=mod), and a release cut from this commit would ship an unreviewed pre-release SDK. Worth gating merge on the v1.29.1 tag landing so the re-pin is done here rather than as a follow-up. (confidence: high)
| // or sharing-mode filter, so those incompatibilities are rejected here rather | ||
| // than silently dropped. | ||
| func buildSearchSecretsSharedWithMeRequest(v *viper.Viper, cmd *cobra.Command) (*shared.PaperSecretServiceSearchSecretsSharedWithMeRequest, error) { | ||
| if cmd.Flags().Changed(secretSharingFlag) { |
There was a problem hiding this comment.
🟡 Suggestion: The incompatibility guards use cmd.Flags().Changed(...), but every value in this file is read through viper, which also resolves CONE_SHARING_MODE/CONE_INCLUDE_OWN env vars and profiles.<name>.sharing-mode config keys (see getSubViperForProfile in config.go). A user who sets sharing-mode via env or profile config gets it silently dropped in --shared-with-me mode instead of the clear error this is meant to produce, and include-own set the same way is silently ignored at line 646 while still being honored by v.GetBool(includeOwnFlag) at line 751. Consider gating on v.GetString(secretSharingFlag) != allFilter / v.GetBool(includeOwnFlag) so the guard matches how the values are actually read. (confidence: high)
| PageSize: &pageSize, | ||
| } | ||
| if query := strings.TrimSpace(v.GetString(queryFlag)); query != "" { | ||
| if len(query) > 256 { |
There was a problem hiding this comment.
🟡 Suggestion: len(query) counts bytes, not characters. If the endpoint's limit is 256 characters, a valid non-ASCII query (e.g. 100 CJK characters = 300 bytes) is rejected client-side before it ever reaches the API. Use utf8.RuneCountInString(query) > 256 if the contract is character-based. (confidence: medium — depends on whether the server counts bytes or runes)
| for { | ||
| resp, err := c.sdk.PaperSecret.SearchSecretsSharedWithMe(ctx, req) | ||
| if err != nil { | ||
| return nil, mapPaperSecretError(err) | ||
| } | ||
| if err := NewHTTPError(resp.RawResponse); err != nil { | ||
| return nil, err | ||
| } | ||
| if resp.PaperSecretServiceSearchResponse != nil { | ||
| out = append(out, resp.PaperSecretServiceSearchResponse.List...) | ||
| token := StringFromPtr(resp.PaperSecretServiceSearchResponse.NextPageToken) | ||
| if token != "" { | ||
| req.PageToken = &token | ||
| continue | ||
| } | ||
| } | ||
| return out, nil |
There was a problem hiding this comment.
🟡 Suggestion: Two things about this loop, both inherited from SearchMySecrets but now on a newly exported C1Client method. First, it mutates the caller's request in place (req.PageToken = &token), so a caller who reuses a request value gets a stale page token on the next call — copying req locally before looping would keep the method side-effect free. Second, the loop is unbounded: a server that keeps returning the same non-empty nextPageToken spins forever while out grows without limit. A page cap or a check that the token actually changed would bound it. (confidence: medium)
| // secretListRunForTest executes the same flag-routing core secretListRun uses, | ||
| // against the harness, without the authenticated cmdContext. | ||
| func secretListRunForTest(ctx context.Context, h *sharedListHarness, v *viper.Viper, cmd *cobra.Command) error { | ||
| if v.GetBool(sharedWithMeFlag) { | ||
| return secretListSharedWithMeRun(ctx, h, v, cmd) | ||
| } | ||
| req, err := buildSearchMySecretsRequest(v) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| _, err = h.SearchMySecrets(ctx, req) | ||
| return err |
There was a problem hiding this comment.
🟡 Suggestion: secretListRunForTest re-implements secretListRun's routing rather than exercising it, so the tests validate a copy that can drift from production. Concretely, the --include-own requires --shared-with-me guard (secret.go:646-648) has no coverage at all because this copy omits it. Extracting the post-cmdContext body of secretListRun into a helper that takes (ctx, creator, sharer, v, cmd) and calling that from both places would close the gap. (confidence: high)
General PR Review: feat(secret): add --shared-with-me list mode for recipient discoveryBlocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0 Review SummaryThe new commit adds a 1,000-page cap to the generic Security IssuesNone found. The shared-with-me request model carries no Correctness IssuesNone found. Suggestions
Prompt for AI agents |
…n routing Address review feedback on PR #153: - The shared-with-me query limit now counts Unicode code points (utf8.RuneCountInString) matching the server's protoc-gen-validate max_len:256 semantics, not UTF-8 bytes. A 256-character multibyte query (512 bytes) passes; 257 characters fails. Regression tests cover the multibyte boundary both sides. - Routing tests now drive the production runSecretList core (extracted from secretListRun so the branch selection, include-own-without- shared-with-me rejection, and creator-path contract are the exact code the CLI executes) instead of a duplicated dispatch. New tests pin the include-own guard and the creator path's page-size-1000 / created-desc sort / sharing-mode-allowed contract. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
…nation The --include-own and --sharing-mode incompatibility guards read cmd.Flags().Changed(), but every value in this file is consumed through viper (CONE_INCLUDE_OWN / CONE_SHARING_MODE env vars and profile config keys), so those sources bypassed the guards. Both guards now read the viper-resolved value; runSecretList and buildSearchSecretsSharedWithMeRequest no longer need the cobra command at all. New tests drive the guards via v.Set to cover exactly that path. The paper-secret listings (SearchMySecrets, SearchSecretsSharedWithMe, SearchSecretAuditEvents) shared one pagination loop shape: it mutated the caller-supplied request in place and followed next_page_token without any bound, so a server echoing a constant token spun forever while results grew. paginate() now copies the request per page, threads the token locally, and stops with an error when the server repeats a non-empty token. Regression tests cover the repeated-token stop and the no-mutation guarantee on both caller-facing methods. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
v1.29.0-...-ef0d92d9c5f2 pointed at the head of the unmerged speakeasy-sdk-regen-1784593459 branch; the nightly has since rebuilt that branch from newer main, leaving the pinned commit unreachable from any ref. The v1.29.1 release now publishes the same regenerated SDK (SearchSecretsSharedWithMe included), so pin the tag instead and re-run go mod tidy + go mod vendor. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
…uard The page-token guard compared only against the immediately preceding token, so a server cycling tokens (A → B → A → B …) still looped forever while results grew. paginate() now records every token the server has handed out and stops at the first repeat; a cycling-token regression test covers exactly that shape. The --sharing-mode guard compared the raw viper value, but secretListSharingMode normalizes with strings.ToLower(strings.TrimSpace). --sharing-mode ALL, " all", or a profile-config sharing-mode: All was rejected on --shared-with-me even though it means the no-filter default. The guard now normalizes the same way; a test drives it via v.Set with " ALL " to pin the two paths together. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
…error The pagination guard still terminated only on token repetition: a server minting a fresh unique token every page kept the listing and the seen-token set growing forever. paginate() now also refuses to follow more than paginationPageCap pages (1000), so the no-unbounded-listing guarantee in its doc comment holds for any server behaviour; the cap bounds the seen set as well. A fresh-token regression test asserts the cap stops the loop. The --sharing-mode guard on --shared-with-me reported the incompatibility message for genuinely invalid values, hiding the validation error the creator path gives. The guard now routes the value through secretListSharingMode first -- invalid values surface "must be internal, external, or all"; any concrete mode then reports the incompatibility. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
| for page := 1; ; page++ { | ||
| if page > paginationPageCap { | ||
| return nil, fmt.Errorf("listing exceeded %d pages; stopping to avoid an unbounded listing", paginationPageCap) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: The cap counts pages, not items, so the effective item ceiling scales with --page-size — 100k for --shared-with-me (max 100/page) but only 1,000 events for cone secret audit --page-size 1, which the flag validation still allows. A legitimate secret with >1,000 audit events at that page size now fails outright and discards everything already collected, rather than terminating a runaway server. Capping on accumulated len(out) (or on pages × page size) would keep the runaway protection while matching the "real listings end long before this" intent regardless of page size. (Confidence: medium)
| if err == nil { | ||
| t.Fatal("an endless stream of fresh tokens must abort at the page cap instead of looping") | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: This test drives a real httptest server through exactly paginationPageCap round trips, so its runtime is pinned to that constant — raising the cap later (which is plausible, given the item-ceiling concern on paginate) silently makes this test proportionally slower. Consider testing paginate directly with a pure in-memory fetch closure so the cap behaviour is asserted without 1,000 HTTP exchanges. (Confidence: medium)
|
Squire (Sonnet 5): Ran build/test/lint verification plus a live end-to-end pass against a dev C1 backend. Summary: Build — Note for reviewers: Test — CI's exact command ( Vet — Lint — Live end-to-end — started the backend service chain needed (postgres, dynamodb, temporal, valkey, innkeeper, ratelimit, session, vault, tenant seed, frontend, auth/api/accounts, envoy, temporal-worker, db-stream, conductor) against a
Not exercised live (already covered by the PR's own unit/httptest suite): multi-page pagination, and Side note unrelated to this PR: resolving an internal recipient by email ( No blockers found. |
1 participant
Summary
Adds
cone secret list --shared-with-me, the recipient-discovery path Phase 1 needs: a recipient discovers secrets shared with them and can then view one by its returned vault ID using the existingsecret view.POST /api/v1/search/secrets/shared_with_meoperation (PaperSecret.SearchSecretsSharedWithMe) through the generated SDK — continuing the architecture from Replace PaperSecret bridge with generated SDK #148; no handwritten bridge.secret listbehavior, flags, help, and output are unchanged (creator list viaSearchMySecrets).next_page_tokento exhaustion) and--query/--status/--typefilters across pages.--page-sizemax 100,--querymax 256 chars, nouser_id/sort_by/sharing_modeever sent; an explicit--sharing-modeis rejected with a clear error instead of silently dropped.include_owndefaults to false (the endpoint default), opt-in via--include-own(rejected without--shared-with-me).mapPaperSecretCreateError→mapPaperSecretError) so HTTP failures surface as coneHTTPErroruniformly.Dependency note
conductorone-sdk-gois pinned to the publishedv1.29.1release, which carries the regeneratedPaperSecret.SearchSecretsSharedWithMeoperation (nightly Speakeasy regen from the refreshed canonical OpenAPI input, including theshared_with_meroute). The earlier interim pin (v1.29.1-0.20260905002051-ef0d92d9c5f2) pointed at the unmergedspeakeasy-sdk-regen-1784593459branch head; when the nightly rebuilt that branch from newer main, the pinned commit became unreachable — so the re-pin to the tag landed in this PR rather than as a follow-up.Tests
--shared-with-meselects the shared endpoint and never sendsuserId/sortBy/sharingMode(asserted on the wire via httptest)pageTokenthreaded--sharing-moderejected;--include-owndefault/opt-ingo test ./...,go vet ./..., golangci-lint (0 issues) all pass locallyReview fixups (2026-09-09)
go.modre-pinned to the publishedv1.29.1tag and vendor re-synced, resolving the unmerged-branch pseudo-version fragility.The
--include-own/--sharing-modeguards now read the viper-resolved values (CONE_INCLUDE_OWN/CONE_SHARING_MODE, profile config) instead ofcmd.Flags().Changed(); tests drive the guards viav.Setto cover exactly that path.SearchSecretsSharedWithMe— and the identicalSearchMySecrets/SearchSecretAuditEventsloops — no longer mutate the caller's request and abort with an error when the server repeats a non-emptynext_page_token; regression tests cover the repeated-token stop and the no-mutation guarantee.The pagination guard tracks every token the server has handed out, so a cycling
next_page_token(A → B → A → …) stops the listing, not just a constant one.The
--sharing-modeguard normalizes the viper value exactly likesecretListSharingMode(strings.ToLower(strings.TrimSpace(...))), so--sharing-mode ALL," all", or a profile-configsharing-mode: Allstill means the no-filter default on--shared-with-me.paginatealso caps any listing at 1000 pages, so a server minting a fresh unique token every page terminates with an error too — the no-unbounded-listing guarantee now holds for any server behaviour, and the cap bounds the seen-token set as well.The
--sharing-modeguard validates throughsecretListSharingModefirst: invalid values surface the creator path'smust be internal, external, or allerror, and only a valid concrete mode reports the incompatibility.