diff --git a/CHANGELOG.md b/CHANGELOG.md index f5229a0..b977db2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ All notable changes to this project will be documented in this file. - `grant favorites add` now fails immediately without a terminal instead of authenticating first - `grant favorites add`'s non-interactive error now mentions the required favorite name, not only the flags +- Interactive selectors now elevate the row you picked, not another target or Entra ID group that happens to render the same way ## [0.9.0] - 2026-08-14 diff --git a/cmd/pty_linux_test.go b/cmd/pty_linux_test.go new file mode 100644 index 0000000..4862427 --- /dev/null +++ b/cmd/pty_linux_test.go @@ -0,0 +1,147 @@ +package cmd + +import ( + "bytes" + "errors" + "fmt" + "io" + "os" + "strings" + "sync" + "syscall" + "testing" + "time" + "unsafe" + + "github.com/aaearon/grant-cli/internal/ui" +) + +// ptySession drives a survey prompt over a real pseudo-terminal. +// +// survey refuses to render unless its input is a tty and it can put that tty into +// raw mode, so the only way to exercise the real SelectTarget/SelectGroup wiring — +// as opposed to the pure resolve* helpers — is to give it one. This uses the raw +// /dev/ptmx + TIOCSPTLCK/TIOCGPTN ioctls rather than a pty module so the repo keeps +// its zero-new-dependency goal. Linux-only by file name; the ioctl constants do not +// exist on other platforms and CI's Windows leg simply never compiles this file. +type ptySession struct { + master *os.File + slave *os.File + + mu sync.Mutex + out bytes.Buffer +} + +// newPTYSession allocates a pty, points os.Stdin and os.Stderr at the slave and +// starts draining the master. Everything is restored via t.Cleanup. +// +// Not parallel-safe: it mutates os.Stdin/os.Stderr. Callers must not call t.Parallel(). +func newPTYSession(t *testing.T) *ptySession { + t.Helper() + + master, err := os.OpenFile("/dev/ptmx", os.O_RDWR, 0) + if err != nil { + t.Skipf("cannot open /dev/ptmx: %v", err) + } + + // unlockpt(3): TIOCSPTLCK with a zero value. + var unlock int32 + if _, _, errno := syscall.Syscall(syscall.SYS_IOCTL, master.Fd(), + syscall.TIOCSPTLCK, uintptr(unsafe.Pointer(&unlock))); errno != 0 { + _ = master.Close() + t.Skipf("TIOCSPTLCK failed: %v", errno) + } + + // ptsname(3): TIOCGPTN yields the slave index. + var ptn uint32 + if _, _, errno := syscall.Syscall(syscall.SYS_IOCTL, master.Fd(), + syscall.TIOCGPTN, uintptr(unsafe.Pointer(&ptn))); errno != 0 { + _ = master.Close() + t.Skipf("TIOCGPTN failed: %v", errno) + } + + slave, err := os.OpenFile(fmt.Sprintf("/dev/pts/%d", ptn), os.O_RDWR|syscall.O_NOCTTY, 0) + if err != nil { + _ = master.Close() + t.Skipf("cannot open pty slave: %v", err) + } + + p := &ptySession{master: master, slave: slave} + + origStdin, origStderr := os.Stdin, os.Stderr + os.Stdin, os.Stderr = slave, slave + + drained := make(chan struct{}) + go func() { + defer close(drained) + buf := make([]byte, 4096) + for { + n, err := master.Read(buf) + if n > 0 { + p.mu.Lock() + p.out.Write(buf[:n]) + p.mu.Unlock() + } + if err != nil { + return + } + } + }() + + t.Cleanup(func() { + os.Stdin, os.Stderr = origStdin, origStderr + _ = slave.Close() + _ = master.Close() + select { + case <-drained: + case <-time.After(2 * time.Second): + } + }) + + return p +} + +// screen returns everything the prompt has written so far. +func (p *ptySession) screen() string { + p.mu.Lock() + defer p.mu.Unlock() + return p.out.String() +} + +// waitFor blocks until the prompt has rendered want. Waiting for the prompt text is +// what guarantees survey has already switched the tty into raw mode, so the keys sent +// afterwards are not mangled by the canonical line discipline. +func (p *ptySession) waitFor(t *testing.T, want string) { + t.Helper() + deadline := time.Now().Add(10 * time.Second) + for time.Now().Before(deadline) { + if strings.Contains(p.screen(), want) { + return + } + time.Sleep(5 * time.Millisecond) + } + t.Fatalf("timed out waiting for %q; screen so far:\n%s", want, p.screen()) +} + +// send writes keys in a single Write so multi-byte escape sequences cannot be split. +func (p *ptySession) send(t *testing.T, keys string) { + t.Helper() + if _, err := p.master.WriteString(keys); err != nil && !errors.Is(err, io.EOF) { + t.Fatalf("writing %q to pty: %v", keys, err) + } +} + +const ( + keyDown = "\x1b[B" + keyEnter = "\r" +) + +// forceInteractive makes ui.IsInteractive() report true for the duration of the test. +// +// Not parallel: mutates the package-global ui.IsTerminalFunc. +func forceInteractive(t *testing.T) { + t.Helper() + orig := ui.IsTerminalFunc + ui.IsTerminalFunc = func(uintptr) bool { return true } + t.Cleanup(func() { ui.IsTerminalFunc = orig }) +} diff --git a/cmd/request_submit.go b/cmd/request_submit.go index f01e95f..4360485 100644 --- a/cmd/request_submit.go +++ b/cmd/request_submit.go @@ -446,6 +446,12 @@ func selectSubmitWorkspace(workspaces []submitWorkspace) (*submitWorkspace, erro if err != nil { return nil, err } + // Bounds-checked rather than trusted: survey reports an index into Options, which + // is built one-for-one from workspaces, so this should always be in range — but a + // panic here would be a poor way to find out otherwise. + if selected < 0 || selected >= len(workspaces) { + return nil, fmt.Errorf("invalid workspace selection index %d", selected) + } return &workspaces[selected], nil } diff --git a/cmd/root.go b/cmd/root.go index 729903f..1abd49f 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -971,7 +971,12 @@ func (s *uiSelector) SelectTarget(targets []models.EligibleTarget) (*models.Elig return ui.SelectTarget(targets) } -// uiUnifiedSelector implements unifiedSelector using survey.Select +// uiUnifiedSelector implements unifiedSelector using survey.Select. This is the +// selector plain `grant`, `grant --groups` and `grant favorites add` all reach. +// It resolves the answer by the selected index rather than by display text, and +// renders from the same sorted slice it resolves against, so duplicate display +// strings — two Entra ID groups with the same name in different directories, say — +// cannot elevate into the wrong one. type uiUnifiedSelector struct{} func (s *uiUnifiedSelector) SelectItem(items []selectionItem) (*selectionItem, error) { @@ -985,16 +990,16 @@ func (s *uiUnifiedSelector) SelectItem(items []selectionItem) (*selectionItem, e options, sorted := buildUnifiedOptions(items) - var selected string + var selectedIdx int prompt := &survey.Select{ Message: "Select a target:", Options: options, Filter: nil, } - if err := survey.AskOne(prompt, &selected, survey.WithStdio(os.Stdin, os.Stderr, os.Stderr)); err != nil { + if err := survey.AskOne(prompt, &selectedIdx, survey.WithStdio(os.Stdin, os.Stderr, os.Stderr)); err != nil { return nil, fmt.Errorf("selection failed: %w", err) } - return findItemByDisplay(sorted, selected) + return resolveSelectionItem(sorted, selectedIdx) } diff --git a/cmd/selection.go b/cmd/selection.go index 2ad7d81..b0533db 100644 --- a/cmd/selection.go +++ b/cmd/selection.go @@ -57,7 +57,9 @@ func buildUnifiedOptions(items []selectionItem) ([]string, []selectionItem) { pairs[i] = indexed{display: formatSelectionItem(item), item: item} } - sort.Slice(pairs, func(i, j int) bool { + // Stable so that items which render identically keep their input order, and the + // options slice and the sorted items slice stay index-for-index aligned. + sort.SliceStable(pairs, func(i, j int) bool { return pairs[i].display < pairs[j].display }) @@ -71,12 +73,15 @@ func buildUnifiedOptions(items []selectionItem) ([]string, []selectionItem) { return options, sorted } -// findItemByDisplay finds a selectionItem by its formatted display string. -func findItemByDisplay(items []selectionItem, display string) (*selectionItem, error) { - for i := range items { - if formatSelectionItem(items[i]) == display { - return &items[i], nil - } +// resolveSelectionItem recovers the item at the index survey returned. Resolving by +// index rather than by display text is what makes duplicate display strings safe: +// formatSelectionItem carries no ID, so the same group name in two directories — or +// the same workspace name and role in two subscriptions — renders identically, and a +// text lookup would return the first match no matter which row the user highlighted. +// Out-of-range indexes are an error, never clamped: guessing a row is the bug. +func resolveSelectionItem(sorted []selectionItem, idx int) (*selectionItem, error) { + if idx < 0 || idx >= len(sorted) { + return nil, fmt.Errorf("invalid selection index %d", idx) } - return nil, fmt.Errorf("item not found: %s", display) + return &sorted[idx], nil } diff --git a/cmd/selection_pty_linux_test.go b/cmd/selection_pty_linux_test.go new file mode 100644 index 0000000..b2f176e --- /dev/null +++ b/cmd/selection_pty_linux_test.go @@ -0,0 +1,109 @@ +package cmd + +import ( + "testing" + + scamodels "github.com/aaearon/grant-cli/internal/sca/models" +) + +// duplicateGroupItems returns three selection items, the first two of which are +// distinct Entra ID groups that render identically: FormatGroupOption falls back to +// "Group: " when no directory name has been cross-referenced, so the same group +// name in two different directories is indistinguishable on screen. +func duplicateGroupItems() []selectionItem { + first := &scamodels.GroupsEligibleTarget{ + DirectoryID: "dir-first", + GroupID: "group-first", + GroupName: "Cloud Admins", + } + second := &scamodels.GroupsEligibleTarget{ + DirectoryID: "dir-second", + GroupID: "group-second", + GroupName: "Cloud Admins", + } + zebra := &scamodels.GroupsEligibleTarget{ + DirectoryID: "dir-zebra", + GroupID: "group-zebra", + GroupName: "Zebra Admins", + } + return []selectionItem{ + {kind: selectionGroup, group: first}, + {kind: selectionGroup, group: second}, + {kind: selectionGroup, group: zebra}, + } +} + +// TestUIUnifiedSelector_PTY_DuplicateGroupDisplay pins the wiring of the selector that +// plain `grant`, `grant --groups` and `grant favorites add` actually reach. It drives +// the live survey prompt over a pty and asserts that highlighting the second of two +// identically rendered groups elevates into that group. Resolving by display string +// instead of by index must fail this test. +// +// Not parallel: newPTYSession swaps os.Stdin/os.Stderr and forceInteractive swaps +// the package-global ui.IsTerminalFunc. +func TestUIUnifiedSelector_PTY_DuplicateGroupDisplay(t *testing.T) { + tests := []struct { + name string + keys string + wantGroupID string + wantDirID string + }{ + { + name: "arrow to second duplicate", + keys: keyDown + keyEnter, + wantGroupID: "group-second", + wantDirID: "dir-second", + }, + { + name: "filter first, then arrow to second duplicate", + // Typing a filter is the dangerous path: the visible rows are a subset, + // yet survey still reports an index into the original option slice. + keys: "Cloud" + keyDown + keyEnter, + wantGroupID: "group-second", + wantDirID: "dir-second", + }, + { + name: "accept the highlighted first duplicate", + keys: keyEnter, + wantGroupID: "group-first", + wantDirID: "dir-first", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + forceInteractive(t) + p := newPTYSession(t) + + type result struct { + item *selectionItem + err error + } + done := make(chan result, 1) + go func() { + s := &uiUnifiedSelector{} + item, err := s.SelectItem(duplicateGroupItems()) + done <- result{item, err} + }() + + p.waitFor(t, "Select a target:") + p.send(t, tt.keys) + + got := <-done + if got.err != nil { + t.Fatalf("SelectItem() error = %v; screen:\n%s", got.err, p.screen()) + } + if got.item.kind != selectionGroup { + t.Fatalf("SelectItem() kind = %v, want selectionGroup", got.item.kind) + } + if got.item.group.GroupID != tt.wantGroupID { + t.Errorf("SelectItem() GroupID = %q, want %q (wrong group selected)", + got.item.group.GroupID, tt.wantGroupID) + } + if got.item.group.DirectoryID != tt.wantDirID { + t.Errorf("SelectItem() DirectoryID = %q, want %q (wrong directory)", + got.item.group.DirectoryID, tt.wantDirID) + } + }) + } +} diff --git a/cmd/selection_test.go b/cmd/selection_test.go index 554c79b..0ed0cf2 100644 --- a/cmd/selection_test.go +++ b/cmd/selection_test.go @@ -1,6 +1,8 @@ package cmd import ( + "fmt" + "reflect" "testing" scamodels "github.com/aaearon/grant-cli/internal/sca/models" @@ -145,59 +147,182 @@ func TestBuildUnifiedOptions(t *testing.T) { } } -func TestFindItemByDisplay(t *testing.T) { - cloudTarget := &scamodels.EligibleTarget{ +func TestResolveSelectionItem(t *testing.T) { + first := &scamodels.GroupsEligibleTarget{ + DirectoryID: "dir-first", + GroupID: "group-first", + GroupName: "Cloud Admins", + } + second := &scamodels.GroupsEligibleTarget{ + DirectoryID: "dir-second", + GroupID: "group-second", + GroupName: "Cloud Admins", + } + cloud := &scamodels.EligibleTarget{ + WorkspaceID: "sub-1", WorkspaceName: "Prod-EastUS", WorkspaceType: scamodels.WorkspaceTypeSubscription, RoleInfo: scamodels.RoleInfo{Name: "Contributor"}, } - groupTarget := &scamodels.GroupsEligibleTarget{ - DirectoryName: "Contoso", - GroupName: "Engineering", - } + // The two groups render identically, which is exactly the case a display-string + // lookup gets wrong. items := []selectionItem{ - {kind: selectionCloud, cloud: cloudTarget}, - {kind: selectionGroup, group: groupTarget}, + {kind: selectionGroup, group: first}, + {kind: selectionGroup, group: second}, + {kind: selectionCloud, cloud: cloud}, } tests := []struct { name string - display string + idx int wantErr bool + wantID string }{ - { - name: "finds cloud by display", - display: "Subscription: Prod-EastUS / Role: Contributor", - wantErr: false, - }, - { - name: "finds group by display", - display: "Directory: Contoso / Group: Engineering (azure)", - wantErr: false, - }, - { - name: "returns error on mismatch", - display: "NonExistent Display String", - wantErr: true, - }, + {name: "first of two identical displays", idx: 0, wantID: "group-first"}, + {name: "second of two identical displays", idx: 1, wantID: "group-second"}, + {name: "cloud item", idx: 2, wantID: "sub-1"}, + {name: "negative index errors", idx: -1, wantErr: true}, + {name: "index past end errors", idx: 3, wantErr: true}, + {name: "far out of range errors", idx: 99, wantErr: true}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - item, err := findItemByDisplay(items, tt.display) + item, err := resolveSelectionItem(items, tt.idx) if tt.wantErr { if err == nil { - t.Error("expected error but got none") + t.Fatalf("resolveSelectionItem(%d) = %v, want error", tt.idx, item) + } + if item != nil { + t.Errorf("resolveSelectionItem(%d) item = %v, want nil", tt.idx, item) } return } if err != nil { - t.Errorf("unexpected error: %v", err) + t.Fatalf("unexpected error: %v", err) + } + var gotID string + switch item.kind { + case selectionGroup: + gotID = item.group.GroupID + case selectionCloud: + gotID = item.cloud.WorkspaceID } - if item == nil { - t.Fatal("expected non-nil item") + if gotID != tt.wantID { + t.Errorf("resolveSelectionItem(%d) id = %q, want %q", tt.idx, gotID, tt.wantID) } }) } } + +// TestResolveSelectionItem_EmptySlice guards the degenerate case: with nothing to +// select from, every index must be rejected rather than panicking. +func TestResolveSelectionItem_EmptySlice(t *testing.T) { + if item, err := resolveSelectionItem(nil, 0); err == nil { + t.Errorf("resolveSelectionItem(nil, 0) = %v, want error", item) + } +} + +// collidingSelectionItems returns 15 items in 5 groups of 3 that render identically +// within each group, mixing cloud targets and groups. The size matters: Go's pdqsort +// delegates to insertion sort below n=12, which happens to be stable, so a small +// fixture cannot tell sort.Slice and sort.SliceStable apart. The ID encodes the input +// position within the group. +func collidingSelectionItems() []selectionItem { + names := []string{"Alpha", "Bravo", "Charlie", "Delta", "Echo"} + var items []selectionItem + // Interleaved and reversed by name so an unstable sort has plenty to scramble. + for copyNum := 1; copyNum <= 3; copyNum++ { + for n := len(names) - 1; n >= 0; n-- { + id := fmt.Sprintf("%s-%d", names[n], copyNum) + if n%2 == 0 { + items = append(items, selectionItem{ + kind: selectionGroup, + group: &scamodels.GroupsEligibleTarget{ + DirectoryID: "dir-" + id, + GroupID: id, + GroupName: names[n], + }, + }) + continue + } + items = append(items, selectionItem{ + kind: selectionCloud, + cloud: &scamodels.EligibleTarget{ + WorkspaceID: id, + WorkspaceName: names[n], + WorkspaceType: scamodels.WorkspaceTypeSubscription, + RoleInfo: scamodels.RoleInfo{Name: "Owner"}, + }, + }) + } + } + return items +} + +// selectionItemID returns the identity that formatSelectionItem does not render. +func selectionItemID(item selectionItem) string { + switch item.kind { + case selectionGroup: + return item.group.GroupID + case selectionCloud: + return item.cloud.WorkspaceID + default: + return "" + } +} + +// TestBuildUnifiedOptions_StableAmongCollisions pins the sort as *stable*, not merely +// ordered. The options slice and the returned items slice must stay index-for-index +// aligned, and two items that render identically must keep their input order — +// otherwise the same keystrokes elevate into a different target run to run. +func TestBuildUnifiedOptions_StableAmongCollisions(t *testing.T) { + items := collidingSelectionItems() + if len(items) < 13 { + t.Fatalf("fixture has %d items; needs >= 13 to distinguish sort.Slice from sort.SliceStable", len(items)) + } + + options, sorted := buildUnifiedOptions(items) + if len(options) != len(items) || len(sorted) != len(items) { + t.Fatalf("buildUnifiedOptions() = %d options / %d items, want %d each", len(options), len(sorted), len(items)) + } + + // options[i] must be the rendering of sorted[i]; index resolution depends on it. + for i := range sorted { + if options[i] != formatSelectionItem(sorted[i]) { + t.Fatalf("options[%d] = %q, but sorted[%d] renders as %q", i, options[i], i, formatSelectionItem(sorted[i])) + } + } + + // Displays must be non-decreasing (it is still a sort). + for i := 1; i < len(options); i++ { + if options[i-1] > options[i] { + t.Fatalf("not sorted at %d: %q > %q", i, options[i-1], options[i]) + } + } + + // Within each colliding display, input order must be preserved. + wantOrder := map[string][]string{} + for _, item := range items { + d := formatSelectionItem(item) + wantOrder[d] = append(wantOrder[d], selectionItemID(item)) + } + gotOrder := map[string][]string{} + for _, item := range sorted { + d := formatSelectionItem(item) + gotOrder[d] = append(gotOrder[d], selectionItemID(item)) + } + if len(wantOrder) < 2 { + t.Fatalf("fixture no longer collides: %d distinct displays", len(wantOrder)) + } + for display, want := range wantOrder { + if len(want) < 2 { + t.Fatalf("display %q does not collide; fixture is broken", display) + } + if !reflect.DeepEqual(gotOrder[display], want) { + t.Errorf("display %q: colliding items reordered\n got %v\nwant %v (input order)", + display, gotOrder[display], want) + } + } +} diff --git a/docs/mutation-ledger.md b/docs/mutation-ledger.md index fd3e343..46bb994 100644 --- a/docs/mutation-ledger.md +++ b/docs/mutation-ledger.md @@ -190,16 +190,16 @@ premise does not hold). | CFG-07 | internal/config | `internal/config/config.go:103` | `filepath.Join(home, ".grant")` → `".grantx"` | CONFIRMED | test | `TestConfigDir_EndsInDotGrant` | PR6 | todo | | CFG-08 | internal/config | `internal/config/config.go:84` | `os.WriteFile(path, data, 0o600)` → `0o644` | CONFIRMED | test | `TestSave_FileMode` with the `runtime.GOOS == "windows"` skip | PR6 | todo | | CFG-09 | internal/config | `internal/config/config.go:75` | `if err := os.MkdirAll(dir, 0o700); err != nil { ... }` → ignore the error | CONFIRMED | test | `TestSave_MkdirAllFailure` — force it portably by pointing at a path whose parent component is an existing **regular file** (ENOTDIR / ERROR_DIRECTORY), never a hardcoded `/dev/null/...` | PR6 | todo | -| UI-01 | internal/ui | `internal/ui/tty.go:18` | `return IsTerminalFunc(os.Stdin.Fd())` → `IsTerminalFunc(os.Stdout.Fd())`. This swap is what makes `grant revoke < /dev/null` hang in a terminal. All twelve prompt-level guards are well covered (8 spot-checked, all killed with `errors.Is` + flag hints); `IsInteractive()` itself is not, because every stub ignores `fd` | CONFIRMED | test | `TestIsInteractive_ChecksStdinFd` — a stub that records the fd and asserts `os.Stdin.Fd()` | PR7 | todo | -| UI-02 | internal/ui | `internal/ui/group_selector.go:60` | Delete the `sort.Slice(sorted, ...)` call in the group selector | CONFIRMED | test + prod-fix | Extract `sortGroupsForDisplay` so ordering is testable without a TTY; `TestSortGroupsForDisplay_CollisionOrdering` | PR7 | todo | -| UI-03 | internal/ui | `internal/ui/session_selector.go:57` | `if remaining <= 0 {` → `if remaining < 0 {`. The fixture only supplies `-5m` (`session_selector_test.go:154`), so exactly-zero is unpinned | CONFIRMED | test | `TestFormatSessionOption_ExactlyZeroRemaining` | PR7 | todo | -| UI-04 | internal/ui | `internal/ui/request_selector.go:18` | Delete the `time.Parse(time.RFC3339Nano, ts)` branch in the timestamp formatter | CONFIRMED | test | `TestFormatRequestOption_RFC3339Nano` | PR7 | todo | -| UI-05 | internal/ui | `internal/ui/role_selector.go:36` | Make the role sort case-**sensitive** (drop the `strings.ToLower` normalization in the `sort.SliceStable` less-func). Note: the raw mutation orphans the `strings` import — remove it too | CONFIRMED | test | `TestSortRolesForDisplay_MixedCase` | PR7 | todo | -| UI-06 | internal/ui | `internal/ui/selector.go:49` | Delete the `if len(targets) == 0` guard in `SelectTarget`. (`SelectRole`/`SelectRequest` equivalents are **killed**; these three are not) | CONFIRMED | test | `TestSelectTarget_EmptyList` | PR7 | todo | -| UI-07 | internal/ui | `internal/ui/session_selector.go:112` | Delete the `if len(sessions) == 0` guard in `SelectSessions` | CONFIRMED | test | `TestSelectSessions_EmptyList` | PR7 | todo | -| UI-08 | internal/ui | `internal/ui/group_selector.go:54` | Delete the `if len(groups) == 0` guard in `SelectGroup`. Note: the raw mutation orphans the `errors` import — remove it too | CONFIRMED | test | `TestSelectGroup_EmptyList` | PR7 | todo | -| UI-09 | internal/ui | `internal/ui/role_selector.go` (post-`survey` index bounds check) | Disable the returned-index bounds check in `SelectRole` | CONFIRMED | wont-fix | none — defensive-only and unreachable through `survey`, which can only return a string it was given | PR7 | todo | -| UI-10 | internal/ui | `internal/ui/request_selector.go` (post-`survey` index bounds check) | Disable the returned-index bounds check in `SelectRequest` | CONFIRMED | wont-fix | none — same rationale as UI-09 | PR7 | todo | +| UI-01 | internal/ui | `internal/ui/tty.go:18` | `return IsTerminalFunc(os.Stdin.Fd())` → `IsTerminalFunc(os.Stdout.Fd())`. This swap is what makes `grant revoke < /dev/null` hang in a terminal. All twelve prompt-level guards are well covered (8 spot-checked, all killed with `errors.Is` + flag hints); `IsInteractive()` itself is not, because every stub ignores `fd` | CONFIRMED | test | `TestIsInteractive_ChecksStdinFd` — a stub that records the fd and asserts `os.Stdin.Fd()` | PR7 | done | +| UI-02 | internal/ui | `internal/ui/group_selector.go:55` | Delete the `sort.SliceStable(sorted, ...)` call in `sortGroupsForDisplay` (orphans nothing — `sort` stays live via `BuildGroupOptions`) | CONFIRMED | test + prod-fix | Extract `sortGroupsForDisplay` so **list ordering** is testable without a TTY; `TestSortGroupsForDisplay_Ordering`. This row guards the order options are rendered in, and nothing else: sorting a copy never prevented wrong-group selection, it only made a display collision resolve consistently to the *first* matching row. Which group a selection denotes is `resolveGroupSelection`'s job (see the `SelectGroup` index fix in the production-changes table below) | PR7 | done | +| UI-03 | internal/ui | `internal/ui/session_selector.go:57` | `if remaining <= 0 {` → `if remaining < 0 {`. The fixture only supplies `-5m` (`session_selector_test.go:154`), so exactly-zero is unpinned | CONFIRMED | test | `TestFormatSessionOption_ExactlyZeroRemaining` | PR7 | done | +| UI-04 | internal/ui | `internal/ui/request_selector.go:18` | Delete the `time.Parse(time.RFC3339Nano, ts)` branch in the timestamp formatter | CONFIRMED | test | `TestFormatRequestOption_RFC3339Nano` | PR7 | done | +| UI-05 | internal/ui | `internal/ui/role_selector.go:36` | Make the role sort case-**sensitive** (drop the `strings.ToLower` normalization in the `sort.SliceStable` less-func). Note: the raw mutation orphans the `strings` import — remove it too | CONFIRMED | test | `TestBuildRoleOptions_MixedCaseSort` — the sort lives in `BuildRoleOptions`; no `sortRolesForDisplay` helper exists or was needed | PR7 | done | +| UI-06 | internal/ui | `internal/ui/selector.go:107` | Delete the `if len(targets) == 0` guard in `SelectTarget` (`:49` is the identically-worded guard inside `BuildOptions`, and mutating *that* one is an **equivalent mutant** — `make([]string, 0)` + `sort.Strings` yields the same empty non-nil slice — so it is not an escape). (`SelectRole`/`SelectRequest` equivalents are **killed**; these three are not) | CONFIRMED | test | `TestSelectTarget_EmptyList`; guard **order** (non-interactive first) is pinned separately by `TestSelectTarget_NonTTYEmptyList` (mutation also orphans the `errors` import — removed so the package compiles; survey then fails with "please provide options to select from") | PR7 | done | +| UI-07 | internal/ui | `internal/ui/session_selector.go:112` | Delete the `if len(sessions) == 0` guard in `SelectSessions` | CONFIRMED | test | `TestSelectSessions_EmptyList`; guard **order** pinned by `TestSelectSessions_NonTTYEmptyList` (the `errors` import stays live via "no sessions selected"; survey fails with "please provide options to select from") | PR7 | done | +| UI-08 | internal/ui | `internal/ui/group_selector.go:79` | Delete the `if len(groups) == 0` guard in `SelectGroup`. Note: the raw mutation orphans the `errors` import — remove it too | CONFIRMED | test | `TestSelectGroup_EmptyList`; guard **order** pinned by `TestSelectGroup_NonTTYEmptyList` | PR7 | done | +| UI-09 | internal/ui | `internal/ui/role_selector.go` (post-`survey` index bounds check) | Disable the returned-index bounds check in `SelectRole` | CONFIRMED | wont-fix | none — defensive-only and unreachable through `survey`, which can only return a string it was given. Closed as **wont-fix in PR7**: the guard stays in place, deliberately uncovered; do not chase it | PR7 | wont-fix (closed) | +| UI-10 | internal/ui | `internal/ui/request_selector.go` (post-`survey` index bounds check) | Disable the returned-index bounds check in `SelectRequest` | CONFIRMED | wont-fix | none — same rationale as UI-09. Closed as **wont-fix in PR7**: the guard stays in place, deliberately uncovered; do not chase it | PR7 | wont-fix (closed) | --- @@ -239,7 +239,7 @@ premise does not hold). | `refuted` | 5 | | **Total** | **165** | -The seven production changes, matching the plan's table: +The seven production changes from the plan's table, plus two added by the PR7 review: | Row | Change | PR | CHANGELOG | |---|---|---|---| @@ -248,8 +248,20 @@ The seven production changes, matching the plan's table: | OUT-26 | `favorites add` early non-interactive guard, favorites-specific message | PR1 | `### Fixed` | | CFG-02 | `ParseCacheTTL` errors on any explicitly-invalid value | PR6 | `### Changed` | | CACHE-07 | `maxSessionAge` → `sessionTimestampRetention` | PR6 | no (internal) | -| UI-02 | `sortGroupsForDisplay` extraction | PR7 | no (refactor) | +| UI-02 | `sortGroupsForDisplay` extraction | PR7 | no (refactor; since removed with `SelectGroup`) | | SFU-10 | `syncStagedFileFn` seam | PR3 | no (test seam; **not** a production gap) | +| — | `SelectTarget` resolves the answer by **index** (`resolveTargetSelection`) against the same sorted copy it renders (`sortTargetsForDisplay`), instead of looking the display text up in the caller's **unsorted** slice. `FormatTargetOption` carries no ID, so two targets with the same workspace name and role in different subscriptions/accounts render identically, and the rendered and searched slices were in different orders besides. Not an audit row — found by the PR7 review. Pinned by `TestSelectTarget_PTY_DuplicateDisplay`, which drives the live prompt over a pty and covers the filtered path; `TestResolveTargetSelection_DuplicateDisplayStrings` and `TestSortTargetsForDisplay_Ordering` cover the extracted helpers only and do **not** pin the wiring | PR7 | `### Fixed` | +| — | `uiUnifiedSelector.SelectItem` resolves the answer by **index** (`resolveSelectionItem`) instead of display text, and `buildUnifiedOptions` sorts stably so options and items stay index-aligned. This is the selector plain `grant`, `grant --groups` and `grant favorites add` actually reach, and it was still bug-for-bug identical to the pre-fix `SelectGroup`: `formatSelectionItem` carries no ID, so two Entra ID groups with the same name in different directories render identically and the text lookup elevated into the first. Not an audit row — found by the PR7 adversarial re-review, which showed the original PR7 fix missed the live caller. Pinned by `TestUIUnifiedSelector_PTY_DuplicateGroupDisplay` (wiring, incl. the filtered path), `TestResolveSelectionItem` (bounds) and `TestBuildUnifiedOptions_StableAmongCollisions` (stability) | PR7 | `### Fixed` | +| — | `SelectGroup`, `FindGroupByDisplay` and `FindTargetByDisplay` deleted — zero production callers. `sortGroupsForDisplay` and `resolveGroupSelection` existed only to serve `SelectGroup` and went with it; the group path in production is the unified selector above. Closes the deletion-candidate note that used to sit under this table | PR7 | no (dead code) | +| — | Sort fixtures for `sortTargetsForDisplay` and `buildUnifiedOptions` widened to 15 colliding entries. At n=4 Go's pdqsort delegates to insertion sort, which is incidentally stable, so `sort.SliceStable` → `sort.Slice` survived the suite in both places. Both mutants are now killed | PR7 | no (test-only) | +| — | `SelectSessions` still resolves by display text, deliberately. Its option strings embed `session.SessionID` and the SCA API is **assumed** to return distinct IDs, so collisions should be unreachable; `FindSessionByDisplay` fails *open* if that assumption breaks. Assumption now stated at the call site rather than asserted as fact | PR7 | no (comment only) | + +The two index fixes above are pinned by pty-driven tests that drive the real +`survey.Select` prompt (`internal/ui/pty_linux_test.go`, `cmd/pty_linux_test.go`; +raw `/dev/ptmx` + `TIOCSPTLCK`/`TIOCGPTN`, no new module dependency, Linux-only by +file name). This matters: tests over the extracted `resolve*` helpers alone left the +whole suite green when the selectors were reverted to display-string resolution, so +they pinned the helpers and not the behaviour. ### Total CONFIRMED diff --git a/internal/ui/group_selector.go b/internal/ui/group_selector.go index 8087dba..16f91b3 100644 --- a/internal/ui/group_selector.go +++ b/internal/ui/group_selector.go @@ -1,12 +1,9 @@ package ui import ( - "errors" "fmt" - "os" "sort" - "github.com/Iilun/survey/v2" "github.com/aaearon/grant-cli/internal/sca/models" ) @@ -32,50 +29,3 @@ func BuildGroupOptions(groups []models.GroupsEligibleTarget) []string { sort.Strings(options) return options } - -// FindGroupByDisplay finds a group by its formatted display string. -func FindGroupByDisplay(groups []models.GroupsEligibleTarget, display string) (*models.GroupsEligibleTarget, error) { - for i := range groups { - if FormatGroupOption(groups[i]) == display { - return &groups[i], nil - } - } - return nil, fmt.Errorf("group not found: %s", display) -} - -// SelectGroup presents an interactive selector for choosing a group. -// It sorts a copy of the groups so that FindGroupByDisplay searches the same -// ordered slice the user saw, avoiding wrong-group selection on display collisions. -func SelectGroup(groups []models.GroupsEligibleTarget) (*models.GroupsEligibleTarget, error) { - if !IsInteractive() { - return nil, fmt.Errorf("%w; use --group flag for non-interactive mode", ErrNotInteractive) - } - - if len(groups) == 0 { - return nil, errors.New("no eligible groups available") - } - - sorted := make([]models.GroupsEligibleTarget, len(groups)) - copy(sorted, groups) - sort.Slice(sorted, func(i, j int) bool { - return FormatGroupOption(sorted[i]) < FormatGroupOption(sorted[j]) - }) - - options := make([]string, len(sorted)) - for i := range sorted { - options[i] = FormatGroupOption(sorted[i]) - } - - var selected string - prompt := &survey.Select{ - Message: "Select a group:", - Options: options, - Filter: nil, - } - - if err := survey.AskOne(prompt, &selected, survey.WithStdio(os.Stdin, os.Stderr, os.Stderr)); err != nil { - return nil, fmt.Errorf("group selection failed: %w", err) - } - - return FindGroupByDisplay(sorted, selected) -} diff --git a/internal/ui/group_selector_test.go b/internal/ui/group_selector_test.go index 578f73a..32f7d89 100644 --- a/internal/ui/group_selector_test.go +++ b/internal/ui/group_selector_test.go @@ -1,8 +1,6 @@ package ui import ( - "errors" - "strings" "testing" "github.com/aaearon/grant-cli/internal/sca/models" @@ -122,101 +120,3 @@ func TestBuildGroupOptions_DuplicateDisplayStrings(t *testing.T) { } } } - -func TestFindGroupByDisplay_DuplicateDisplayStrings(t *testing.T) { - t.Parallel() - // When display strings collide, FindGroupByDisplay returns the first match - // in the slice it's given. SelectGroup sorts a copy, so the caller controls order. - groups := []models.GroupsEligibleTarget{ - {DirectoryID: "dir2", GroupID: "grp2", GroupName: "Engineering"}, - {DirectoryID: "dir1", GroupID: "grp1", GroupName: "Engineering"}, - } - got, err := FindGroupByDisplay(groups, "Group: Engineering") - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - // Should return first in slice (grp2 since dir2 is first) - if got.GroupID != "grp2" { - t.Errorf("expected grp2 (first in slice), got %q", got.GroupID) - } -} - -func TestFindGroupByDisplay(t *testing.T) { - t.Parallel() - groups := []models.GroupsEligibleTarget{ - {DirectoryID: "dir1", DirectoryName: "Contoso", GroupID: "grp1", GroupName: "Engineering"}, - {DirectoryID: "dir1", DirectoryName: "Contoso", GroupID: "grp2", GroupName: "DevOps"}, - } - - tests := []struct { - name string - groups []models.GroupsEligibleTarget - display string - wantID string - wantErr bool - }{ - { - name: "found engineering", - groups: groups, - display: "Directory: Contoso / Group: Engineering", - wantID: "grp1", - }, - { - name: "found devops", - groups: groups, - display: "Directory: Contoso / Group: DevOps", - wantID: "grp2", - }, - { - name: "not found", - groups: groups, - display: "Directory: Contoso / Group: NonExistent", - wantErr: true, - }, - { - name: "empty groups", - groups: []models.GroupsEligibleTarget{}, - display: "Group: Test", - wantErr: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - got, err := FindGroupByDisplay(tt.groups, tt.display) - if (err != nil) != tt.wantErr { - t.Errorf("FindGroupByDisplay() error = %v, wantErr %v", err, tt.wantErr) - return - } - if tt.wantErr { - return - } - if got.GroupID != tt.wantID { - t.Errorf("FindGroupByDisplay().GroupID = %q, want %q", got.GroupID, tt.wantID) - } - }) - } -} - -// Not parallel: mutates the package-global IsTerminalFunc. -func TestSelectGroup_NonTTY(t *testing.T) { - original := IsTerminalFunc - defer func() { IsTerminalFunc = original }() - IsTerminalFunc = func(fd uintptr) bool { return false } - - groups := []models.GroupsEligibleTarget{ - {DirectoryID: "dir1", GroupID: "grp1", GroupName: "Engineering"}, - } - - _, err := SelectGroup(groups) - if err == nil { - t.Fatal("expected error for non-TTY") - } - if !errors.Is(err, ErrNotInteractive) { - t.Errorf("expected ErrNotInteractive, got: %v", err) - } - if !strings.Contains(err.Error(), "--group") { - t.Errorf("error should mention --group, got: %v", err) - } -} diff --git a/internal/ui/pty_linux_test.go b/internal/ui/pty_linux_test.go new file mode 100644 index 0000000..cc2d3d1 --- /dev/null +++ b/internal/ui/pty_linux_test.go @@ -0,0 +1,145 @@ +package ui + +import ( + "bytes" + "errors" + "fmt" + "io" + "os" + "strings" + "sync" + "syscall" + "testing" + "time" + "unsafe" +) + +// ptySession drives a survey prompt over a real pseudo-terminal. +// +// survey refuses to render unless its input is a tty and it can put that tty into +// raw mode, so the only way to exercise the real SelectTarget/SelectGroup wiring — +// as opposed to the pure resolve* helpers — is to give it one. This uses the raw +// /dev/ptmx + TIOCSPTLCK/TIOCGPTN ioctls rather than a pty module so the repo keeps +// its zero-new-dependency goal. Linux-only by file name; the ioctl constants do not +// exist on other platforms and CI's Windows leg simply never compiles this file. +type ptySession struct { + master *os.File + slave *os.File + + mu sync.Mutex + out bytes.Buffer +} + +// newPTYSession allocates a pty, points os.Stdin and os.Stderr at the slave and +// starts draining the master. Everything is restored via t.Cleanup. +// +// Not parallel-safe: it mutates os.Stdin/os.Stderr. Callers must not call t.Parallel(). +func newPTYSession(t *testing.T) *ptySession { + t.Helper() + + master, err := os.OpenFile("/dev/ptmx", os.O_RDWR, 0) + if err != nil { + t.Skipf("cannot open /dev/ptmx: %v", err) + } + + // unlockpt(3): TIOCSPTLCK with a zero value. + var unlock int32 + if _, _, errno := syscall.Syscall(syscall.SYS_IOCTL, master.Fd(), + syscall.TIOCSPTLCK, uintptr(unsafe.Pointer(&unlock))); errno != 0 { + _ = master.Close() + t.Skipf("TIOCSPTLCK failed: %v", errno) + } + + // ptsname(3): TIOCGPTN yields the slave index. + var ptn uint32 + if _, _, errno := syscall.Syscall(syscall.SYS_IOCTL, master.Fd(), + syscall.TIOCGPTN, uintptr(unsafe.Pointer(&ptn))); errno != 0 { + _ = master.Close() + t.Skipf("TIOCGPTN failed: %v", errno) + } + + slave, err := os.OpenFile(fmt.Sprintf("/dev/pts/%d", ptn), os.O_RDWR|syscall.O_NOCTTY, 0) + if err != nil { + _ = master.Close() + t.Skipf("cannot open pty slave: %v", err) + } + + p := &ptySession{master: master, slave: slave} + + origStdin, origStderr := os.Stdin, os.Stderr + os.Stdin, os.Stderr = slave, slave + + drained := make(chan struct{}) + go func() { + defer close(drained) + buf := make([]byte, 4096) + for { + n, err := master.Read(buf) + if n > 0 { + p.mu.Lock() + p.out.Write(buf[:n]) + p.mu.Unlock() + } + if err != nil { + return + } + } + }() + + t.Cleanup(func() { + os.Stdin, os.Stderr = origStdin, origStderr + _ = slave.Close() + _ = master.Close() + select { + case <-drained: + case <-time.After(2 * time.Second): + } + }) + + return p +} + +// screen returns everything the prompt has written so far. +func (p *ptySession) screen() string { + p.mu.Lock() + defer p.mu.Unlock() + return p.out.String() +} + +// waitFor blocks until the prompt has rendered want. Waiting for the prompt text is +// what guarantees survey has already switched the tty into raw mode, so the keys sent +// afterwards are not mangled by the canonical line discipline. +func (p *ptySession) waitFor(t *testing.T, want string) { + t.Helper() + deadline := time.Now().Add(10 * time.Second) + for time.Now().Before(deadline) { + if strings.Contains(p.screen(), want) { + return + } + time.Sleep(5 * time.Millisecond) + } + t.Fatalf("timed out waiting for %q; screen so far:\n%s", want, p.screen()) +} + +// send writes keys in a single Write so multi-byte escape sequences cannot be split. +func (p *ptySession) send(t *testing.T, keys string) { + t.Helper() + if _, err := p.master.WriteString(keys); err != nil && !errors.Is(err, io.EOF) { + t.Fatalf("writing %q to pty: %v", keys, err) + } +} + +const ( + keyDown = "\x1b[B" + keyEnter = "\r" +) + +// forceInteractive makes IsInteractive() report true for the duration of the test. +// +// Not parallel: mutates the package-global ui.IsTerminalFunc. +func forceInteractive(t *testing.T) { + t.Helper() + orig := IsTerminalFunc + IsTerminalFunc = func(uintptr) bool { return true } + t.Cleanup(func() { IsTerminalFunc = orig }) +} diff --git a/internal/ui/request_selector_test.go b/internal/ui/request_selector_test.go index a4dbc6f..80183cb 100644 --- a/internal/ui/request_selector_test.go +++ b/internal/ui/request_selector_test.go @@ -78,6 +78,23 @@ func TestSelectRequest_NonInteractive(t *testing.T) { } } +// TestSelectRequest_NonInteractiveEmptyList pins the order of the two guards: the +// non-interactive check must win when both conditions hold at once. +// Not parallel: mutates the package-global IsTerminalFunc. +func TestSelectRequest_NonInteractiveEmptyList(t *testing.T) { + orig := IsTerminalFunc + defer func() { IsTerminalFunc = orig }() + IsTerminalFunc = func(fd uintptr) bool { return false } + + _, err := SelectRequest(nil) + if err == nil { + t.Fatal("expected error for non-TTY with an empty list") + } + if !errors.Is(err, ErrNotInteractive) { + t.Errorf("expected ErrNotInteractive to win over the empty-list guard, got: %v", err) + } +} + func TestBuildRequestOptions_SortedCorrectlyWithOffsets(t *testing.T) { // "2026-04-20T10:00:00+02:00" == 08:00 UTC — earlier than 09:30Z // String sort would put +02:00 after Z; time sort must put 09:30Z first. @@ -94,6 +111,45 @@ func TestBuildRequestOptions_SortedCorrectlyWithOffsets(t *testing.T) { } } +// TestFormatRequestOption_RFC3339Nano pins the RFC3339Nano parse branch of +// formatSelectorTimestamp. The textual fallback below it truncates at the '.' +// and therefore drops the timezone offset, so the two paths genuinely differ +// for any timestamp carrying fractional seconds — which the API does emit. +func TestFormatRequestOption_RFC3339Nano(t *testing.T) { + tests := []struct { + name string + createdAt string + want string + }{ + { + name: "fractional seconds with offset keeps the offset", + createdAt: "2026-04-20T10:00:00.123456789+02:00", + want: "2026-04-20T10:00:00+02:00", + }, + { + name: "fractional seconds in UTC", + createdAt: "2026-04-20T10:00:00.5Z", + want: "2026-04-20T10:00:00Z", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r := wfmodels.AccessRequest{ + RequestID: "req-nano", + RequestState: wfmodels.RequestStatePending, + CreatedBy: "user@test", + CreatedAt: tt.createdAt, + } + got := FormatRequestOption(r) + want := "PENDING - / - (by user@test, " + tt.want + ") [req-nano]" + if got != want { + t.Errorf("FormatRequestOption() = %q, want %q", got, want) + } + }) + } +} + func TestSelectRequest_EmptyList(t *testing.T) { orig := IsTerminalFunc defer func() { IsTerminalFunc = orig }() diff --git a/internal/ui/role_selector_test.go b/internal/ui/role_selector_test.go index 821d5c7..6eeb85f 100644 --- a/internal/ui/role_selector_test.go +++ b/internal/ui/role_selector_test.go @@ -78,6 +78,53 @@ func TestSelectRole_NonInteractive(t *testing.T) { } } +// TestSelectRole_NonInteractiveEmptyList pins the order of the two guards: the +// non-interactive check must win when both conditions hold at once. +// Not parallel: mutates the package-global IsTerminalFunc. +func TestSelectRole_NonInteractiveEmptyList(t *testing.T) { + orig := IsTerminalFunc + defer func() { IsTerminalFunc = orig }() + IsTerminalFunc = func(fd uintptr) bool { return false } + + _, err := SelectRole(nil) + if err == nil { + t.Fatal("expected error for non-TTY with an empty list") + } + if !errors.Is(err, ErrNotInteractive) { + t.Errorf("expected ErrNotInteractive to win over the empty-list guard, got: %v", err) + } +} + +// TestBuildRoleOptions_MixedCaseSort pins the strings.ToLower normalisation in the +// role sort. A case-sensitive comparison puts every capitalised name ahead of every +// lower-case one ("Zone" < "admin" in ASCII), scattering the list the user scans. +// The custom-first fixture above is uniformly capitalised and cannot see that. +func TestBuildRoleOptions_MixedCaseSort(t *testing.T) { + roles := []models.OnDemandResource{ + {ResourceName: "cost-reader"}, + {ResourceName: "Backup Operator"}, + {ResourceName: "admin-lite"}, + {ResourceName: "Zone Editor"}, + } + opts, sorted := BuildRoleOptions(roles) + + want := []string{"admin-lite", "Backup Operator", "cost-reader", "Zone Editor"} + // Length first: without it a regression that shortens the slice panics on the + // index below instead of failing with a readable message. + if len(sorted) != len(want) || len(opts) != len(want) { + t.Fatalf("BuildRoleOptions() returned %d options / %d roles, want %d of each", len(opts), len(sorted), len(want)) + } + for i, name := range want { + if sorted[i].ResourceName != name { + t.Errorf("position %d: got %q, want %q", i, sorted[i].ResourceName, name) + } + // options and roles must stay index-parallel — SelectRole resolves by index. + if opts[i] != FormatRoleOption(sorted[i]) { + t.Errorf("position %d: option %q does not render roles[%d] (%q)", i, opts[i], i, FormatRoleOption(sorted[i])) + } + } +} + func TestSelectRole_EmptyList(t *testing.T) { orig := IsTerminalFunc defer func() { IsTerminalFunc = orig }() diff --git a/internal/ui/selector.go b/internal/ui/selector.go index b124b96..6f9ee3e 100644 --- a/internal/ui/selector.go +++ b/internal/ui/selector.go @@ -59,17 +59,34 @@ func BuildOptions(targets []models.EligibleTarget) []string { return options } -// FindTargetByDisplay finds a target by its formatted display string. -func FindTargetByDisplay(targets []models.EligibleTarget, display string) (*models.EligibleTarget, error) { - for i := range targets { - if FormatTargetOption(targets[i]) == display { - return &targets[i], nil - } +// sortTargetsForDisplay returns a copy of targets ordered by display string, leaving +// the caller's slice untouched. It only fixes the order the options are rendered in; +// which target a selection denotes is decided by index in resolveTargetSelection. +// The sort is stable, so targets that render identically keep their input order. +func sortTargetsForDisplay(targets []models.EligibleTarget) []models.EligibleTarget { + sorted := make([]models.EligibleTarget, len(targets)) + copy(sorted, targets) + sort.SliceStable(sorted, func(i, j int) bool { + return FormatTargetOption(sorted[i]) < FormatTargetOption(sorted[j]) + }) + return sorted +} + +// resolveTargetSelection recovers the target at the index survey returned. Resolving +// by index rather than by display text is what makes duplicate display strings safe: +// FormatTargetOption carries no ID, so the same workspace name and role in two +// subscriptions or accounts renders identically, and a text lookup would return the +// first match no matter which row the user highlighted. +func resolveTargetSelection(sorted []models.EligibleTarget, idx int) (*models.EligibleTarget, error) { + if idx < 0 || idx >= len(sorted) { + return nil, fmt.Errorf("invalid target selection index %d", idx) } - return nil, fmt.Errorf("target not found: %s", display) + return &sorted[idx], nil } -// SelectTarget presents an interactive selector for choosing a target. +// SelectTarget presents an interactive selector for choosing a target. Uses the +// selected index (not display text) to recover the target, so duplicate display +// strings are safe, and renders from the same sorted copy it resolves against. func SelectTarget(targets []models.EligibleTarget) (*models.EligibleTarget, error) { if !IsInteractive() { return nil, fmt.Errorf("%w; use --target and --role flags for non-interactive mode", ErrNotInteractive) @@ -79,18 +96,22 @@ func SelectTarget(targets []models.EligibleTarget) (*models.EligibleTarget, erro return nil, errors.New("no eligible targets available") } - options := BuildOptions(targets) + sorted := sortTargetsForDisplay(targets) + options := make([]string, len(sorted)) + for i := range sorted { + options[i] = FormatTargetOption(sorted[i]) + } - var selected string + var selectedIdx int prompt := &survey.Select{ Message: "Select a target:", Options: options, Filter: nil, // Enable default fuzzy filter } - if err := survey.AskOne(prompt, &selected, survey.WithStdio(os.Stdin, os.Stderr, os.Stderr)); err != nil { + if err := survey.AskOne(prompt, &selectedIdx, survey.WithStdio(os.Stdin, os.Stderr, os.Stderr)); err != nil { return nil, fmt.Errorf("target selection failed: %w", err) } - return FindTargetByDisplay(targets, selected) + return resolveTargetSelection(sorted, selectedIdx) } diff --git a/internal/ui/selector_pty_linux_test.go b/internal/ui/selector_pty_linux_test.go new file mode 100644 index 0000000..9623f4c --- /dev/null +++ b/internal/ui/selector_pty_linux_test.go @@ -0,0 +1,98 @@ +package ui + +import ( + "testing" + + "github.com/aaearon/grant-cli/internal/sca/models" +) + +// duplicateTargets returns three targets, the first two of which render identically: +// FormatTargetOption carries no workspace ID, so the same workspace name and role in +// two different subscriptions is indistinguishable on screen. +func duplicateTargets() []models.EligibleTarget { + return []models.EligibleTarget{ + { + CSP: models.CSPAzure, + WorkspaceID: "sub-first", + WorkspaceName: "Production", + WorkspaceType: "SUBSCRIPTION", + RoleInfo: models.RoleInfo{Name: "Owner"}, + }, + { + CSP: models.CSPAzure, + WorkspaceID: "sub-second", + WorkspaceName: "Production", + WorkspaceType: "SUBSCRIPTION", + RoleInfo: models.RoleInfo{Name: "Owner"}, + }, + { + CSP: models.CSPAzure, + WorkspaceID: "sub-zebra", + WorkspaceName: "Zebra", + WorkspaceType: "SUBSCRIPTION", + RoleInfo: models.RoleInfo{Name: "Reader"}, + }, + } +} + +// TestSelectTarget_PTY_DuplicateDisplay pins the real SelectTarget wiring, not just +// resolveTargetSelection: it drives the live survey prompt over a pty and asserts that +// highlighting the second of two identically rendered targets returns that second +// target. Reverting SelectTarget to display-string resolution must fail this test. +// +// Not parallel: newPTYSession swaps os.Stdin/os.Stderr and forceInteractive swaps +// the package-global IsTerminalFunc. +func TestSelectTarget_PTY_DuplicateDisplay(t *testing.T) { + tests := []struct { + name string + keys string + wants string + }{ + { + name: "arrow to second duplicate", + keys: keyDown + keyEnter, + wants: "sub-second", + }, + { + name: "filter first, then arrow to second duplicate", + // Typing a filter is the dangerous path: the visible rows are a subset, + // yet survey still reports an index into the original option slice. + keys: "Production" + keyDown + keyEnter, + wants: "sub-second", + }, + { + name: "accept the highlighted first duplicate", + keys: keyEnter, + wants: "sub-first", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + forceInteractive(t) + p := newPTYSession(t) + + type result struct { + target *models.EligibleTarget + err error + } + done := make(chan result, 1) + go func() { + target, err := SelectTarget(duplicateTargets()) + done <- result{target, err} + }() + + p.waitFor(t, "Select a target:") + p.send(t, tt.keys) + + got := <-done + if got.err != nil { + t.Fatalf("SelectTarget() error = %v; screen:\n%s", got.err, p.screen()) + } + if got.target.WorkspaceID != tt.wants { + t.Errorf("SelectTarget() WorkspaceID = %q, want %q (wrong row selected)", + got.target.WorkspaceID, tt.wants) + } + }) + } +} diff --git a/internal/ui/selector_test.go b/internal/ui/selector_test.go index b73b7df..d9b8aaa 100644 --- a/internal/ui/selector_test.go +++ b/internal/ui/selector_test.go @@ -2,6 +2,9 @@ package ui import ( "errors" + "fmt" + "reflect" + "sort" "strings" "testing" @@ -237,83 +240,203 @@ func TestSelectTarget_NonTTY(t *testing.T) { } } -func TestFindTargetByDisplay(t *testing.T) { +// TestSelectTarget_NonTTYEmptyList pins the order of the two guards. _NonTTY passes a +// non-empty list and _EmptyList forces a TTY, so their inputs never intersect and +// swapping the guards survives both. This case satisfies both conditions at once. +// Not parallel: mutates the package-global IsTerminalFunc. +func TestSelectTarget_NonTTYEmptyList(t *testing.T) { + original := IsTerminalFunc + defer func() { IsTerminalFunc = original }() + IsTerminalFunc = func(fd uintptr) bool { return false } + + _, err := SelectTarget(nil) + if err == nil { + t.Fatal("expected error for non-TTY with an empty list") + } + if !errors.Is(err, ErrNotInteractive) { + t.Errorf("expected ErrNotInteractive to win over the empty-list guard, got: %v", err) + } +} + +// TestSortTargetsForDisplay_Ordering pins the order options are rendered in, and +// nothing else. Which target a selection denotes is resolveTargetSelection's job. +func TestSortTargetsForDisplay_Ordering(t *testing.T) { t.Parallel() + // Deliberately unsorted input containing a display collision: the two "Shared" + // subscriptions carry the same workspace name and role, so both render identically. targets := []models.EligibleTarget{ - { - OrganizationID: "org1", - WorkspaceID: "sub1", - WorkspaceName: "Production", - WorkspaceType: models.WorkspaceTypeSubscription, - RoleInfo: models.RoleInfo{ID: "role1", Name: "Owner"}, - }, - { - OrganizationID: "org1", - WorkspaceID: "rg1", - WorkspaceName: "rg-web", - WorkspaceType: models.WorkspaceTypeResourceGroup, - RoleInfo: models.RoleInfo{ID: "role2", Name: "Contributor"}, - }, + {OrganizationID: "org-z", WorkspaceID: "sub-zebra", WorkspaceName: "Zebra Sub", WorkspaceType: models.WorkspaceTypeSubscription, RoleInfo: models.RoleInfo{ID: "r0", Name: "Reader"}}, + {OrganizationID: "org-1", WorkspaceID: "sub-shared-1", WorkspaceName: "Shared", WorkspaceType: models.WorkspaceTypeSubscription, RoleInfo: models.RoleInfo{ID: "r1", Name: "Owner"}}, + {OrganizationID: "org-2", WorkspaceID: "sub-shared-2", WorkspaceName: "Shared", WorkspaceType: models.WorkspaceTypeSubscription, RoleInfo: models.RoleInfo{ID: "r1", Name: "Owner"}}, + {OrganizationID: "org-a", WorkspaceID: "sub-alpha", WorkspaceName: "Alpha Sub", WorkspaceType: models.WorkspaceTypeSubscription, RoleInfo: models.RoleInfo{ID: "r2", Name: "Contributor"}}, } + before := append([]models.EligibleTarget(nil), targets...) + + sorted := sortTargetsForDisplay(targets) + if len(sorted) != len(targets) { + t.Fatalf("sortTargetsForDisplay() length = %d, want %d", len(sorted), len(targets)) + } + + options := make([]string, len(sorted)) + for i := range sorted { + options[i] = FormatTargetOption(sorted[i]) + } + if !sort.StringsAreSorted(options) { + t.Errorf("rendered options are not in display order: %q", options) + } + + // The sort is stable, so the two colliding rows keep their input order. + if sorted[1].WorkspaceID != "sub-shared-1" || sorted[2].WorkspaceID != "sub-shared-2" { + t.Errorf("colliding rows lost input order: got %q, %q", sorted[1].WorkspaceID, sorted[2].WorkspaceID) + } + + if !reflect.DeepEqual(targets, before) { + t.Errorf("sortTargetsForDisplay() mutated the caller's slice: got %+v, want %+v", targets, before) + } +} + +// TestResolveTargetSelection_DuplicateDisplayStrings pins the wrong-target fix. +// FormatTargetOption carries no ID, so two eligible targets with the same workspace +// name and role in different subscriptions/accounts render to the same string. +// Recovering the answer by text returns the first match regardless of which row the +// user highlighted — and SelectTarget searched the caller's *unsorted* slice while +// rendering a sorted one, so the two could disagree even without a collision. +// This covers the extracted resolver in isolation. The real SelectTarget wiring is +// pinned separately by TestSelectTarget_PTY_DuplicateDisplay, which drives the live +// survey prompt over a pseudo-terminal. +func TestResolveTargetSelection_DuplicateDisplayStrings(t *testing.T) { + t.Parallel() + targets := []models.EligibleTarget{ + {OrganizationID: "org-1", WorkspaceID: "sub-1", WorkspaceName: "Shared", WorkspaceType: models.WorkspaceTypeSubscription, RoleInfo: models.RoleInfo{ID: "role-1", Name: "Owner"}}, + {OrganizationID: "org-2", WorkspaceID: "sub-2", WorkspaceName: "Shared", WorkspaceType: models.WorkspaceTypeSubscription, RoleInfo: models.RoleInfo{ID: "role-1", Name: "Owner"}}, + } + sorted := sortTargetsForDisplay(targets) + if len(sorted) != 2 { + t.Fatalf("sortTargetsForDisplay() length = %d, want 2", len(sorted)) + } + if FormatTargetOption(sorted[0]) != FormatTargetOption(sorted[1]) { + t.Fatalf("fixture no longer collides: %q vs %q", FormatTargetOption(sorted[0]), FormatTargetOption(sorted[1])) + } + if sorted[0].WorkspaceID == sorted[1].WorkspaceID { + t.Fatalf("fixture targets are indistinguishable: %+v", sorted) + } + + // The sort is stable and the two entries compare equal, so the rendered order is + // the input order: row 0 is sub-1, row 1 is sub-2. tests := []struct { - name string - targets []models.EligibleTarget - display string - want *models.EligibleTarget - wantErr bool + idx int + wantWorkspaceID string + wantOrgID string }{ - { - name: "found subscription", - targets: targets, - display: "Subscription: Production / Role: Owner", - want: &targets[0], - wantErr: false, - }, - { - name: "found resource group", - targets: targets, - display: "Resource Group: rg-web / Role: Contributor", - want: &targets[1], - wantErr: false, - }, - { - name: "not found", - targets: targets, - display: "Subscription: NonExistent / Role: Reader", - want: nil, - wantErr: true, - }, - { - name: "empty targets", - targets: []models.EligibleTarget{}, - display: "Subscription: Test / Role: Owner", - want: nil, - wantErr: true, - }, + {idx: 0, wantWorkspaceID: "sub-1", wantOrgID: "org-1"}, + {idx: 1, wantWorkspaceID: "sub-2", wantOrgID: "org-2"}, } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - got, err := FindTargetByDisplay(tt.targets, tt.display) - if (err != nil) != tt.wantErr { - t.Errorf("FindTargetByDisplay() error = %v, wantErr %v", err, tt.wantErr) - return - } - if tt.want == nil && got != nil { - t.Errorf("FindTargetByDisplay() = %v, want nil", got) - return - } - if tt.want != nil { - if got == nil { - t.Errorf("FindTargetByDisplay() = nil, want %v", tt.want) - return - } - if got.WorkspaceID != tt.want.WorkspaceID || got.RoleInfo.ID != tt.want.RoleInfo.ID { - t.Errorf("FindTargetByDisplay() = %v, want %v", got, tt.want) - } - } - }) + got, err := resolveTargetSelection(sorted, tt.idx) + if err != nil { + t.Fatalf("resolveTargetSelection(_, %d) error = %v", tt.idx, err) + } + if got.WorkspaceID != tt.wantWorkspaceID || got.OrganizationID != tt.wantOrgID { + t.Errorf("selecting row %d returned WorkspaceID=%q OrganizationID=%q, want %q/%q", + tt.idx, got.WorkspaceID, got.OrganizationID, tt.wantWorkspaceID, tt.wantOrgID) + } + } +} + +func TestResolveTargetSelection_OutOfRange(t *testing.T) { + t.Parallel() + targets := []models.EligibleTarget{ + {WorkspaceID: "sub-1", WorkspaceName: "Shared", WorkspaceType: models.WorkspaceTypeSubscription, RoleInfo: models.RoleInfo{Name: "Owner"}}, + } + for _, idx := range []int{-1, 1} { + if _, err := resolveTargetSelection(targets, idx); err == nil { + t.Errorf("resolveTargetSelection(_, %d) = nil error, want out-of-range error", idx) + } + } +} + +// Not parallel: mutates the package-global IsTerminalFunc. +func TestSelectTarget_EmptyList(t *testing.T) { + original := IsTerminalFunc + defer func() { IsTerminalFunc = original }() + IsTerminalFunc = func(fd uintptr) bool { return true } + + _, err := SelectTarget(nil) + if err == nil { + t.Fatal("expected error for empty list") + } + if !strings.Contains(err.Error(), "no eligible targets available") { + t.Errorf("unexpected error: %v", err) + } +} + +// collidingTargets returns 15 targets in 5 groups of 3 that render identically within +// each group. The size matters: Go's pdqsort delegates to insertion sort below n=12, +// which happens to be stable, so a 4-element fixture cannot tell sort.Slice and +// sort.SliceStable apart. WorkspaceID encodes the input position within its group. +func collidingTargets() []models.EligibleTarget { + names := []string{"Alpha", "Bravo", "Charlie", "Delta", "Echo"} + var targets []models.EligibleTarget + // Interleaved and reversed by name so an unstable sort has plenty to scramble. + for copyNum := 1; copyNum <= 3; copyNum++ { + for n := len(names) - 1; n >= 0; n-- { + targets = append(targets, models.EligibleTarget{ + WorkspaceID: fmt.Sprintf("%s-%d", names[n], copyNum), + WorkspaceName: names[n], + WorkspaceType: models.WorkspaceTypeSubscription, + RoleInfo: models.RoleInfo{Name: "Owner"}, + }) + } + } + return targets +} + +// TestSortTargetsForDisplay_StableAmongCollisions pins the sort as *stable*, not merely +// ordered. Rendering order is what the user sees and index resolution is resolved +// against the same slice, so two targets that render identically must keep their input +// order — otherwise the same keystrokes pick a different target run to run. +func TestSortTargetsForDisplay_StableAmongCollisions(t *testing.T) { + t.Parallel() + + targets := collidingTargets() + if len(targets) < 13 { + t.Fatalf("fixture has %d targets; needs >= 13 to distinguish sort.Slice from sort.SliceStable", len(targets)) + } + + sorted := sortTargetsForDisplay(targets) + if len(sorted) != len(targets) { + t.Fatalf("sortTargetsForDisplay() length = %d, want %d", len(sorted), len(targets)) + } + + // Displays must be non-decreasing (it is still a sort). + for i := 1; i < len(sorted); i++ { + if FormatTargetOption(sorted[i-1]) > FormatTargetOption(sorted[i]) { + t.Fatalf("not sorted at %d: %q > %q", i, FormatTargetOption(sorted[i-1]), FormatTargetOption(sorted[i])) + } + } + + // Within each colliding display, input order must be preserved. + wantOrder := map[string][]string{} + for _, target := range targets { + d := FormatTargetOption(target) + wantOrder[d] = append(wantOrder[d], target.WorkspaceID) + } + gotOrder := map[string][]string{} + for _, target := range sorted { + d := FormatTargetOption(target) + gotOrder[d] = append(gotOrder[d], target.WorkspaceID) + } + if len(wantOrder) < 2 { + t.Fatalf("fixture no longer collides: %d distinct displays", len(wantOrder)) + } + for display, want := range wantOrder { + if len(want) < 2 { + t.Fatalf("display %q does not collide; fixture is broken", display) + } + if !reflect.DeepEqual(gotOrder[display], want) { + t.Errorf("display %q: colliding targets reordered\n got %v\nwant %v (input order)", + display, gotOrder[display], want) + } } } diff --git a/internal/ui/session_selector.go b/internal/ui/session_selector.go index 89f16c6..a7d4846 100644 --- a/internal/ui/session_selector.go +++ b/internal/ui/session_selector.go @@ -104,6 +104,16 @@ func FindSessionByDisplay( } // SelectSessions presents a multi-select prompt for choosing sessions to revoke. +// It resolves the answer by display text rather than by index, unlike the target, +// role and request selectors. +// +// That rests on an assumption about the SCA API, stated here rather than left +// implicit: every option string embeds session.SessionID, and the API is assumed to +// return distinct session IDs, so no two options can render identically. If that ever +// stops holding, FindSessionByDisplay fails *open* — it returns the first match — and +// the wrong session would be revoked. Revoking is far less dangerous than elevating +// into the wrong role, which is why this is documented rather than converted to index +// binding, but the assumption is load-bearing. func SelectSessions(sessions []models.SessionInfo, nameMap map[string]string) ([]models.SessionInfo, error) { if !IsInteractive() { return nil, fmt.Errorf("%w; use --all or provide session IDs as arguments", ErrNotInteractive) diff --git a/internal/ui/session_selector_test.go b/internal/ui/session_selector_test.go index 86f8bbb..43faf2b 100644 --- a/internal/ui/session_selector_test.go +++ b/internal/ui/session_selector_test.go @@ -272,6 +272,24 @@ func TestSelectSessions_NonTTY(t *testing.T) { } } +// TestSelectSessions_NonTTYEmptyList pins the order of the two guards. _NonTTY passes +// a non-empty list and _EmptyList forces a TTY, so their inputs never intersect and +// swapping the guards survives both. This case satisfies both conditions at once. +// Not parallel: mutates the package-global IsTerminalFunc. +func TestSelectSessions_NonTTYEmptyList(t *testing.T) { + original := IsTerminalFunc + defer func() { IsTerminalFunc = original }() + IsTerminalFunc = func(fd uintptr) bool { return false } + + _, err := SelectSessions(nil, nil) + if err == nil { + t.Fatal("expected error for non-TTY with an empty list") + } + if !errors.Is(err, ErrNotInteractive) { + t.Errorf("expected ErrNotInteractive to win over the empty-list guard, got: %v", err) + } +} + // Not parallel: mutates the package-global IsTerminalFunc. func TestConfirmRevocation_NonTTY(t *testing.T) { original := IsTerminalFunc @@ -289,3 +307,39 @@ func TestConfirmRevocation_NonTTY(t *testing.T) { t.Errorf("error should mention --yes, got: %v", err) } } + +// TestFormatSessionOption_ExactlyZeroRemaining exercises the `remaining <= 0` +// boundary itself. The table above only supplies a negative duration, so a +// `<= 0` → `< 0` regression would leave a just-expired session rendering as +// "remaining: 0m" instead of "expired". +func TestFormatSessionOption_ExactlyZeroRemaining(t *testing.T) { + t.Parallel() + session := models.SessionInfo{ + SessionID: "session-rem-zero", + CSP: models.CSPAzure, + WorkspaceID: "/subscriptions/sub-1", + RoleID: "Reader", + SessionDuration: 3600, + } + remainingMap := map[string]time.Duration{"session-rem-zero": 0} + + want := "Reader on /subscriptions/sub-1 - expired (session: session-rem-zero)" + if got := FormatSessionOption(session, nil, nil, remainingMap); got != want { + t.Errorf("FormatSessionOption() = %q, want %q", got, want) + } +} + +// Not parallel: mutates the package-global IsTerminalFunc. +func TestSelectSessions_EmptyList(t *testing.T) { + original := IsTerminalFunc + defer func() { IsTerminalFunc = original }() + IsTerminalFunc = func(fd uintptr) bool { return true } + + _, err := SelectSessions(nil, nil) + if err == nil { + t.Fatal("expected error for empty list") + } + if !strings.Contains(err.Error(), "no sessions available") { + t.Errorf("unexpected error: %v", err) + } +} diff --git a/internal/ui/tty_test.go b/internal/ui/tty_test.go index c0c94b5..1e10808 100644 --- a/internal/ui/tty_test.go +++ b/internal/ui/tty_test.go @@ -1,6 +1,36 @@ package ui -import "testing" +import ( + "os" + "testing" +) + +// TestIsInteractive_ChecksStdinFd pins WHICH descriptor is probed, not just the +// boolean result. Every other stub in this package ignores its fd argument, so +// swapping os.Stdin.Fd() for os.Stdout.Fd() survives them all — and that swap is +// exactly what would make `grant revoke < /dev/null` in a terminal report an +// interactive session and then hang on a prompt reading closed stdin. +// +// Not parallel: mutates the package-global IsTerminalFunc. +func TestIsInteractive_ChecksStdinFd(t *testing.T) { + original := IsTerminalFunc + defer func() { IsTerminalFunc = original }() + + var gotFDs []uintptr + IsTerminalFunc = func(fd uintptr) bool { + gotFDs = append(gotFDs, fd) + return true + } + + IsInteractive() + + if len(gotFDs) != 1 { + t.Fatalf("IsTerminalFunc called %d times, want exactly 1 (fds: %v)", len(gotFDs), gotFDs) + } + if want := os.Stdin.Fd(); gotFDs[0] != want { + t.Errorf("IsInteractive() probed fd %d, want stdin fd %d (stdout is %d)", gotFDs[0], want, os.Stdout.Fd()) + } +} // Not parallel: mutates the package-global IsTerminalFunc. func TestIsInteractive_WhenTerminal(t *testing.T) {