From f2ffa219c364c04059a722ea1b442b7bc2e668d9 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Mon, 20 Apr 2026 20:34:29 +0200 Subject: [PATCH 1/4] refactor: decouple --output json from non-interactive mode Remove the PersistentPreRunE override that forced IsTerminalFunc to return false whenever --output json was set. Interactive prompts now run normally in a TTY regardless of output format; JSON serialisation and TTY detection are fully orthogonal. Also collapse the JSON-specific error branch in resolveRequestIDInteractive to the standard ErrNotInteractive path, and delete the now-obsolete TestResolveRequestIDInteractive_JSONMode test. --- CHANGELOG.md | 4 ++++ CLAUDE.md | 2 +- cmd/request_picker.go | 3 --- cmd/request_picker_test.go | 22 ---------------------- cmd/root.go | 3 --- 5 files changed, 5 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0b76491..6a9c9ce 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ All notable changes to this project will be documented in this file. ## [Unreleased] +### Changed + +- `--output json` is now a pure serialisation flag; it no longer forces non-interactive mode. Interactive pickers and prompts (e.g. `grant request get -o json` with no ID, `grant request submit -o json` without `--target`/`--role`) work in a TTY, writing prompts to stderr and JSON to stdout. + ### Added - `grant request` command group for managing access requests through the approval workflow diff --git a/CLAUDE.md b/CLAUDE.md index 3363e78..d3143fe 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -98,7 +98,7 @@ Custom `SCAAccessService` follows SDK conventions: ## JSON Output - `--output` / `-o` persistent flag on root command: `text` (default) or `json` -- Validated in `PersistentPreRunE`; JSON mode forces `IsTerminalFunc` to return false (non-interactive) +- Validated in `PersistentPreRunE`. `--output json` is a pure serialisation flag; it does not affect interactivity — interactive prompts still run in a TTY and write to stderr while JSON goes to stdout - `cmd/output.go` — `outputFormat` var, `isJSONOutput()`, `writeJSON(w, data)` - `cmd/output_types.go` — JSON structs: `cloudElevationOutput`, `groupElevationJSON`, `sessionOutput`, `statusOutput`, `revocationOutput`, `favoriteOutput`, `awsCredentialOutput`, `accessRequestOutput`, `accessRequestListOutput` - All commands support JSON: root elevation, `env`, `status`, `revoke`, `favorites list`, `request list`, `request get`, `request submit`, `request cancel`, `request approve`, `request reject` diff --git a/cmd/request_picker.go b/cmd/request_picker.go index f87ad85..3307f0b 100644 --- a/cmd/request_picker.go +++ b/cmd/request_picker.go @@ -32,9 +32,6 @@ var resolveRequestIDFn = resolveRequestIDInteractive // shows the interactive picker, returning the chosen request ID. func resolveRequestIDInteractive(ctx context.Context, svc accessRequestService, scope pickerScope) (string, error) { if !ui.IsInteractive() { - if isJSONOutput() { - return "", errors.New("request ID is required with --output json; run `grant request list --output json` to find it") - } return "", fmt.Errorf("%w; pass the request ID as a positional argument (run `grant request list` to find it)", ui.ErrNotInteractive) } diff --git a/cmd/request_picker_test.go b/cmd/request_picker_test.go index d17bf05..c04ebdb 100644 --- a/cmd/request_picker_test.go +++ b/cmd/request_picker_test.go @@ -45,28 +45,6 @@ func TestResolveRequestIDInteractive_NonInteractive(t *testing.T) { } } -func TestResolveRequestIDInteractive_JSONMode(t *testing.T) { - withInteractiveTTY(t, false) - orig := outputFormat - outputFormat = "json" - t.Cleanup(func() { outputFormat = orig }) - - svc := &capturingMockAccessRequestService{} - _, err := resolveRequestIDInteractive(t.Context(), svc, pickerScope{emptyMsg: "access requests"}) - if err == nil { - t.Fatal("expected error") - } - if errors.Is(err, ui.ErrNotInteractive) { - t.Errorf("JSON mode error should not wrap ErrNotInteractive") - } - if !strings.Contains(err.Error(), "--output json") { - t.Errorf("expected --output json hint, got %v", err) - } - if strings.Contains(err.Error(), "requires a terminal") { - t.Errorf("JSON mode error should not mention terminal: %v", err) - } -} - func TestResolveRequestIDInteractive_EmptyList(t *testing.T) { withInteractiveTTY(t, true) diff --git a/cmd/root.go b/cmd/root.go index a063e60..1a80e7c 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -101,9 +101,6 @@ Examples: if outputFormat != "text" && outputFormat != "json" { return fmt.Errorf("invalid output format %q: must be one of: text, json", outputFormat) } - if isJSONOutput() { - ui.IsTerminalFunc = func(fd uintptr) bool { return false } - } return nil }, RunE: runFn, From dbd2c54ecb8dd9511d448a3210f9e56596240f7b Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Mon, 20 Apr 2026 21:04:07 +0200 Subject: [PATCH 2/4] feat: --refresh flag and expanded azure_resource scope support for grant request submit - Add --refresh flag to grant request submit to bypass the on-demand role and eligibility caches (mirrors grant/grant env --refresh behavior) - Expand interactive role selection to SUBSCRIPTION, RESOURCE_GROUP, and RESOURCE workspace types using the naive 2-level ancestors pattern validated in the PoC (subscription: 876 roles, resource_group: 873 roles); custom roles scoped to intermediate management groups may be absent - Add refresh bool to NewCachedRolesLister signature (mirrors CachedEligibilityLister) - Delete untracked poc_subscription.go and poc_ondemand_roles.go --- CHANGELOG.md | 3 +- CLAUDE.md | 6 ++-- cmd/request_submit.go | 32 ++++++++++++----- cmd/request_test.go | 54 +++++++++++------------------ internal/cache/cached_roles.go | 26 ++++++++------ internal/cache/cached_roles_test.go | 43 ++++++++++++++++++++--- 6 files changed, 104 insertions(+), 60 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6a9c9ce..f5aba00 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,8 +21,9 @@ All notable changes to this project will be documented in this file. - New `internal/workflows/` package implementing the CyberArk Access Requests API client (`/api/workflows/requests`) - Interactive role selector for `grant request submit`: after workspace selection, fuzzy-filterable list of requestable roles is fetched from the SCA on-demand role discovery endpoints (`/api/cloud/resources/ondemand`, `/api/cloud/cloud-roles/ondemand`) - Supported workspace types: `DIRECTORY` (azure_ad), `ACCOUNT` (aws), `MANAGEMENT_GROUP` (azure_resource) - - Other Azure-resource scopes (subscription, resource group, resource) still require `--role-id` until validated + - Interactive role selection now also supports `SUBSCRIPTION`, `RESOURCE_GROUP`, and `RESOURCE` workspaces (uses naive 2-level ancestors; custom roles scoped to intermediate management groups may not appear — use `--role-id` for those) - Roles cached in `~/.grant/cache/ondemand_roles__.json` (4h TTL) +- `grant request submit --refresh` bypasses the on-demand role and eligibility caches (mirrors `grant --refresh`) - Interactive request picker for `grant request cancel`, `approve`, `reject`, and `get` — omit the `` positional argument in a terminal to pick from a scoped, fuzzy-filterable list (cancel: open requests you created; approve/reject: pending requests assigned to you; get: any request). Non-TTY invocation still requires the positional argument. ## [0.6.1] - 2026-04-08 diff --git a/CLAUDE.md b/CLAUDE.md index d3143fe..c0d02d8 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -74,9 +74,9 @@ Custom `SCAAccessService` follows SDK conventions: - `grant list` — list eligible targets and groups without triggering elevation; supports `--provider`, `--groups`, `--refresh`, `--output json`; used by LLMs to discover available targets programmatically - `grant revoke` — revoke sessions: direct (`grant revoke `), `--all`, or interactive multi-select; `--yes` skips confirmation - `grant request` — manage access requests through approval workflow; subcommands: `submit`, `list`, `get`, `cancel`, `approve`, `reject` -- `grant request submit` — submit on-demand access request; workspace selector uses SCA eligibility (deduplicated to unique workspaces); after workspace selection, interactive role selector fetches roles via SCA on-demand endpoints (GET `/api/cloud/resources/ondemand` for `azure_ad`/`aws`, POST `/api/cloud/cloud-roles/ondemand` for `azure_resource`); shows summary + confirmation before submitting; flags: `--target`, `--role-id`, `--role`, `--provider`, `--reason`, `--priority`, `--date`, `--timezone`, `--from`, `--to`, `--yes` - - Interactive role selection supports `DIRECTORY`, `ACCOUNT` (AWS), and `MANAGEMENT_GROUP` workspaces; other Azure-resource scopes (subscription, resource group, resource) require `--role-id` - - On-demand role cache: `~/.grant/cache/ondemand_roles__.json` (4h TTL); no `--refresh` flag — delete manually to invalidate +- `grant request submit` — submit on-demand access request; workspace selector uses SCA eligibility (deduplicated to unique workspaces); after workspace selection, interactive role selector fetches roles via SCA on-demand endpoints (GET `/api/cloud/resources/ondemand` for `azure_ad`/`aws`, POST `/api/cloud/cloud-roles/ondemand` for `azure_resource`); shows summary + confirmation before submitting; flags: `--target`, `--role-id`, `--role`, `--provider`, `--reason`, `--priority`, `--date`, `--timezone`, `--from`, `--to`, `--yes`, `--refresh` + - Interactive role selection supports `DIRECTORY`, `ACCOUNT` (AWS), `MANAGEMENT_GROUP`, `SUBSCRIPTION`, `RESOURCE_GROUP`, and `RESOURCE` workspaces (azure_resource scopes use naive 2-level ancestors; custom roles scoped to intermediate management groups may be missing — fall back to `--role-id`) + - On-demand role cache: `~/.grant/cache/ondemand_roles__.json` (4h TTL); `--refresh` on `grant request submit` bypasses the cache - `grant request list` — list access requests; flags: `--state`, `--result`, `--priority`, `--role` (CREATOR/APPROVER), `--search`, `--sort`, `--desc` - `grant request get [id]` — get full request details; omitting `` in a TTY opens a fuzzy-filterable picker of all your requests - `grant request cancel [id]` — cancel an open request; optional `--reason`. Omitting `` in a TTY opens a picker scoped to STARTING/RUNNING/PENDING requests you created (role=CREATOR) diff --git a/cmd/request_submit.go b/cmd/request_submit.go index f4fb771..0ec9387 100644 --- a/cmd/request_submit.go +++ b/cmd/request_submit.go @@ -46,6 +46,7 @@ func newRequestSubmitCommand(svc accessRequestService) *cobra.Command { cmd.Flags().String("from", "", "Start time (HH:MM)") cmd.Flags().String("to", "", "End time (HH:MM)") cmd.Flags().BoolP("yes", "y", false, "Skip confirmation prompt") + cmd.Flags().Bool("refresh", false, "Bypass on-demand role and eligibility caches") return cmd } @@ -223,12 +224,13 @@ func runRequestSubmit(cmd *cobra.Command, svc accessRequestService) error { targetName, _ := cmd.Flags().GetString("target") roleID, _ := cmd.Flags().GetString("role-id") roleName, _ := cmd.Flags().GetString("role") + refresh, _ := cmd.Flags().GetBool("refresh") ctx, cancel := context.WithTimeout(cmd.Context(), apiTimeout) defer cancel() // 1. Workspace - workspace, err := resolveSubmitTargetFn(ctx, provider, targetName) + workspace, err := resolveSubmitTargetFn(ctx, provider, targetName, refresh) if err != nil { return err } @@ -238,7 +240,7 @@ func runRequestSubmit(cmd *cobra.Command, svc accessRequestService) error { if !ui.IsInteractive() { return errors.New("non-interactive mode requires --role-id") } - resolvedID, resolvedName, err := resolveRoleFn(ctx, workspace) + resolvedID, resolvedName, err := resolveRoleFn(ctx, workspace, refresh) if err != nil { return fmt.Errorf("%w; retry with --role-id to bypass interactive role selection", err) } @@ -352,7 +354,7 @@ func resolveSubmitFields(cmd *cobra.Command) (*submitFields, error) { return f, nil } -func resolveSubmitTarget(ctx context.Context, provider, targetName string) (*submitWorkspace, error) { +func resolveSubmitTarget(ctx context.Context, provider, targetName string, refresh bool) (*submitWorkspace, error) { _, scaSvc, _, err := bootstrapSCAService() if err != nil { return nil, fmt.Errorf("failed to bootstrap SCA service: %w", err) @@ -362,7 +364,7 @@ func resolveSubmitTarget(ctx context.Context, provider, targetName string) (*sub if cfg == nil { cfg = config.DefaultConfig() } - cachedLister := buildCachedLister(cfg, false, scaSvc, nil) + cachedLister := buildCachedLister(cfg, refresh, scaSvc, nil) fetchCtx, fetchCancel := context.WithTimeout(ctx, apiTimeout) defer fetchCancel() @@ -487,7 +489,7 @@ func buildRequestDetails(ws *submitWorkspace, roleID, roleName string, f *submit // resolveSubmitRole fetches on-demand roles for the selected workspace and // prompts the user to choose one. Returns the role's resource_id and resource_name. -func resolveSubmitRole(ctx context.Context, ws *submitWorkspace) (roleID, roleName string, _ error) { +func resolveSubmitRole(ctx context.Context, ws *submitWorkspace, refresh bool) (roleID, roleName string, _ error) { req, err := buildOnDemandRequest(ws) if err != nil { return "", "", err @@ -508,7 +510,7 @@ func resolveSubmitRole(ctx context.Context, ws *submitWorkspace) (roleID, roleNa if cacheErr == nil { ttl := config.ParseCacheTTL(cfg) store := cache.NewStore(cacheDir, ttl) - lister = cache.NewCachedRolesLister(scaSvc, store, common.GetLogger("grant", -1)) + lister = cache.NewCachedRolesLister(scaSvc, store, refresh, common.GetLogger("grant", -1)) } fetchCtx, cancel := context.WithTimeout(ctx, apiTimeout) @@ -555,9 +557,21 @@ func buildOnDemandRequest(ws *submitWorkspace) (models.OnDemandRequest, error) { }, }, nil case "SUBSCRIPTION", "RESOURCE_GROUP", "RESOURCE": - return models.OnDemandRequest{}, fmt.Errorf( - "interactive role selection not supported for %s workspaces yet; use --role-id (see docs/handoff-ondemand-roles-poc.md)", - strings.ToLower(wt)) + resourceType := map[string]string{ + "SUBSCRIPTION": "subscription", + "RESOURCE_GROUP": "resource_group", + "RESOURCE": "resource", + }[wt] + return models.OnDemandRequest{ + WorkspaceID: ws.WorkspaceID, + PlatformName: "azure_resource", + OrgID: ws.OrganizationID, + ResourceType: resourceType, + Ancestors: []string{ + "/" + ws.OrganizationID, + "/" + ws.WorkspaceID, + }, + }, nil default: return models.OnDemandRequest{}, fmt.Errorf( "interactive role selection not supported for workspace type %q; use --role-id", diff --git a/cmd/request_test.go b/cmd/request_test.go index 79875ed..40bf88f 100644 --- a/cmd/request_test.go +++ b/cmd/request_test.go @@ -501,7 +501,7 @@ func TestRunRequestSubmit_NonInteractive(t *testing.T) { original := resolveSubmitTargetFn defer func() { resolveSubmitTargetFn = original }() - resolveSubmitTargetFn = func(_ context.Context, _, _ string) (*submitWorkspace, error) { + resolveSubmitTargetFn = func(_ context.Context, _, _ string, _ bool) (*submitWorkspace, error) { return &submitWorkspace{ WorkspaceName: "Test Sub", WorkspaceID: "ws-1", @@ -539,7 +539,7 @@ func TestRunRequestSubmit_JSONOutput(t *testing.T) { original := resolveSubmitTargetFn defer func() { resolveSubmitTargetFn = original }() - resolveSubmitTargetFn = func(_ context.Context, _, _ string) (*submitWorkspace, error) { + resolveSubmitTargetFn = func(_ context.Context, _, _ string, _ bool) (*submitWorkspace, error) { return &submitWorkspace{ WorkspaceName: "Test Sub", WorkspaceID: "ws-1", @@ -586,7 +586,7 @@ func TestRunRequestSubmit_ServiceError(t *testing.T) { original := resolveSubmitTargetFn defer func() { resolveSubmitTargetFn = original }() - resolveSubmitTargetFn = func(_ context.Context, _, _ string) (*submitWorkspace, error) { + resolveSubmitTargetFn = func(_ context.Context, _, _ string, _ bool) (*submitWorkspace, error) { return &submitWorkspace{ WorkspaceName: "Sub", WorkspaceID: "ws-1", @@ -620,7 +620,7 @@ func TestRunRequestSubmit_MissingFlags_NonInteractive(t *testing.T) { original := resolveSubmitTargetFn defer func() { resolveSubmitTargetFn = original }() - resolveSubmitTargetFn = func(_ context.Context, _, _ string) (*submitWorkspace, error) { + resolveSubmitTargetFn = func(_ context.Context, _, _ string, _ bool) (*submitWorkspace, error) { return &submitWorkspace{ WorkspaceName: "Sub", WorkspaceID: "ws-1", @@ -646,31 +646,16 @@ func TestRunRequestSubmit_MissingFlags_NonInteractive(t *testing.T) { } func TestBuildOnDemandRequest_UnsupportedType(t *testing.T) { - tests := []struct { - name string - wt models.WorkspaceType - }{ - {"subscription", models.WorkspaceTypeSubscription}, - {"resource_group", models.WorkspaceType("RESOURCE_GROUP")}, - {"resource", models.WorkspaceType("RESOURCE")}, + _, err := buildOnDemandRequest(&submitWorkspace{ + WorkspaceID: "ws-1", + WorkspaceType: models.WorkspaceType("SOMETHING_NEW"), + OrganizationID: "org-1", + }) + if err == nil { + t.Fatal("expected error for unknown workspace type") } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - _, err := buildOnDemandRequest(&submitWorkspace{ - WorkspaceID: "ws-1", - WorkspaceType: tt.wt, - OrganizationID: "org-1", - }) - if err == nil { - t.Fatalf("expected error for %s", tt.name) - } - if !strings.Contains(err.Error(), "not supported") { - t.Errorf("error should mention not supported: %v", err) - } - if !strings.Contains(err.Error(), "--role-id") { - t.Errorf("error should point to --role-id: %v", err) - } - }) + if !strings.Contains(err.Error(), "--role-id") { + t.Errorf("error should point to --role-id: %v", err) } } @@ -686,6 +671,9 @@ func TestBuildOnDemandRequest_SupportedTypes(t *testing.T) { {"directory", models.WorkspaceType("DIRECTORY"), "dir-1", "dir-1", "azure_ad", 0}, {"account", models.WorkspaceType("ACCOUNT"), "123", "123", "aws", 0}, {"management_group", models.WorkspaceType("MANAGEMENT_GROUP"), "providers/Microsoft.Management/managementGroups/root", "dir-456", "azure_resource", 2}, + {"subscription", models.WorkspaceType("SUBSCRIPTION"), "sub-1", "dir-456", "azure_resource", 2}, + {"resource_group", models.WorkspaceType("RESOURCE_GROUP"), "rg-1", "dir-456", "azure_resource", 2}, + {"resource", models.WorkspaceType("RESOURCE"), "res-1", "dir-456", "azure_resource", 2}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -718,7 +706,7 @@ func TestRunRequestSubmit_InteractiveRoleSelection(t *testing.T) { }() ui.IsTerminalFunc = func(fd uintptr) bool { return true } - resolveSubmitTargetFn = func(_ context.Context, _, _ string) (*submitWorkspace, error) { + resolveSubmitTargetFn = func(_ context.Context, _, _ string, _ bool) (*submitWorkspace, error) { return &submitWorkspace{ WorkspaceName: "Dir", WorkspaceID: "dir-1", @@ -727,7 +715,7 @@ func TestRunRequestSubmit_InteractiveRoleSelection(t *testing.T) { OrganizationID: "dir-1", }, nil } - resolveRoleFn = func(_ context.Context, ws *submitWorkspace) (string, string, error) { + resolveRoleFn = func(_ context.Context, ws *submitWorkspace, _ bool) (string, string, error) { if ws.WorkspaceID != "dir-1" { t.Errorf("expected ws dir-1, got %s", ws.WorkspaceID) } @@ -878,7 +866,7 @@ func TestRunRequestSubmit_InvalidProvider(t *testing.T) { original := resolveSubmitTargetFn defer func() { resolveSubmitTargetFn = original }() - resolveSubmitTargetFn = func(_ context.Context, _, _ string) (*submitWorkspace, error) { + resolveSubmitTargetFn = func(_ context.Context, _, _ string, _ bool) (*submitWorkspace, error) { t.Fatal("resolveSubmitTarget should not be called with invalid provider") return nil, nil } @@ -910,7 +898,7 @@ func TestRunRequestSubmit_ConfirmationDenied(t *testing.T) { confirmSubmitFn = originalConfirm }() - resolveSubmitTargetFn = func(_ context.Context, _, _ string) (*submitWorkspace, error) { + resolveSubmitTargetFn = func(_ context.Context, _, _ string, _ bool) (*submitWorkspace, error) { return &submitWorkspace{ WorkspaceName: "Test Sub", WorkspaceID: "ws-1", @@ -954,7 +942,7 @@ func TestRunRequestSubmit_YesFlagSkipsConfirmation(t *testing.T) { confirmSubmitFn = originalConfirm }() - resolveSubmitTargetFn = func(_ context.Context, _, _ string) (*submitWorkspace, error) { + resolveSubmitTargetFn = func(_ context.Context, _, _ string, _ bool) (*submitWorkspace, error) { return &submitWorkspace{ WorkspaceName: "Test Sub", WorkspaceID: "ws-1", diff --git a/internal/cache/cached_roles.go b/internal/cache/cached_roles.go index a10dc7e..0655680 100644 --- a/internal/cache/cached_roles.go +++ b/internal/cache/cached_roles.go @@ -15,29 +15,35 @@ type OnDemandRolesLister interface { // CachedRolesLister decorates an OnDemandRolesLister with file-based caching. type CachedRolesLister struct { - inner OnDemandRolesLister - store *Store - log Logger + inner OnDemandRolesLister + store *Store + refresh bool + log Logger } // NewCachedRolesLister creates a caching decorator for on-demand role discovery. -func NewCachedRolesLister(inner OnDemandRolesLister, store *Store, log Logger) *CachedRolesLister { +// When refresh is true, the cache read is bypassed but the API response is still cached. +func NewCachedRolesLister(inner OnDemandRolesLister, store *Store, refresh bool, log Logger) *CachedRolesLister { if log == nil { log = nopLogger{} } - return &CachedRolesLister{inner: inner, store: store, log: log} + return &CachedRolesLister{inner: inner, store: store, refresh: refresh, log: log} } // ListOnDemandResources checks the cache first, then falls through to the inner lister. func (c *CachedRolesLister) ListOnDemandResources(ctx context.Context, req scamodels.OnDemandRequest) ([]scamodels.OnDemandResource, error) { key := onDemandRolesCacheKey(req.PlatformName, req.WorkspaceID) - var cached []scamodels.OnDemandResource - if Get(c.store, key, &cached) { - c.log.Info("Cache hit for on-demand roles (%s, %d roles)", req.PlatformName, len(cached)) - return cached, nil + if c.refresh { + c.log.Info("Cache refresh requested for on-demand roles (%s), bypassing cache", req.PlatformName) + } else { + var cached []scamodels.OnDemandResource + if Get(c.store, key, &cached) { + c.log.Info("Cache hit for on-demand roles (%s, %d roles)", req.PlatformName, len(cached)) + return cached, nil + } + c.log.Info("Cache miss for on-demand roles (%s), fetching from API", req.PlatformName) } - c.log.Info("Cache miss for on-demand roles (%s), fetching from API", req.PlatformName) roles, err := c.inner.ListOnDemandResources(ctx, req) if err != nil { diff --git a/internal/cache/cached_roles_test.go b/internal/cache/cached_roles_test.go index 8623121..c017450 100644 --- a/internal/cache/cached_roles_test.go +++ b/internal/cache/cached_roles_test.go @@ -30,7 +30,7 @@ func newTestStore(t *testing.T) *Store { func TestCachedRolesLister_MissThenHit(t *testing.T) { fake := &fakeRolesLister{roles: []scamodels.OnDemandResource{{ResourceID: "r1", ResourceName: "Role 1"}}} - cached := NewCachedRolesLister(fake, newTestStore(t), nil) + cached := NewCachedRolesLister(fake, newTestStore(t), false, nil) req := scamodels.OnDemandRequest{WorkspaceID: "ws-1", PlatformName: "azure_ad", OrgID: "ws-1"} @@ -56,7 +56,7 @@ func TestCachedRolesLister_MissThenHit(t *testing.T) { func TestCachedRolesLister_DifferentWorkspacesDistinct(t *testing.T) { fake := &fakeRolesLister{roles: []scamodels.OnDemandResource{{ResourceID: "r1"}}} - cached := NewCachedRolesLister(fake, newTestStore(t), nil) + cached := NewCachedRolesLister(fake, newTestStore(t), false, nil) reqA := scamodels.OnDemandRequest{WorkspaceID: "ws-A", PlatformName: "azure_ad", OrgID: "ws-A"} reqB := scamodels.OnDemandRequest{WorkspaceID: "ws-B", PlatformName: "azure_ad", OrgID: "ws-B"} @@ -74,7 +74,7 @@ func TestCachedRolesLister_DifferentWorkspacesDistinct(t *testing.T) { func TestCachedRolesLister_DifferentPlatformsDistinct(t *testing.T) { fake := &fakeRolesLister{roles: []scamodels.OnDemandResource{{ResourceID: "r1"}}} - cached := NewCachedRolesLister(fake, newTestStore(t), nil) + cached := NewCachedRolesLister(fake, newTestStore(t), false, nil) reqAD := scamodels.OnDemandRequest{WorkspaceID: "same-id", PlatformName: "azure_ad", OrgID: "same-id"} reqAWS := scamodels.OnDemandRequest{WorkspaceID: "same-id", PlatformName: "aws", OrgID: "same-id"} @@ -92,7 +92,7 @@ func TestCachedRolesLister_DifferentPlatformsDistinct(t *testing.T) { func TestCachedRolesLister_InnerError(t *testing.T) { fake := &fakeRolesLister{err: errors.New("api failure")} - cached := NewCachedRolesLister(fake, newTestStore(t), nil) + cached := NewCachedRolesLister(fake, newTestStore(t), false, nil) req := scamodels.OnDemandRequest{WorkspaceID: "ws-1", PlatformName: "aws", OrgID: "ws-1"} _, err := cached.ListOnDemandResources(t.Context(), req) @@ -101,6 +101,41 @@ func TestCachedRolesLister_InnerError(t *testing.T) { } } +func TestCachedRolesLister_Refresh(t *testing.T) { + fake := &fakeRolesLister{roles: []scamodels.OnDemandResource{{ResourceID: "r1", ResourceName: "Role 1"}}} + store := newTestStore(t) + req := scamodels.OnDemandRequest{WorkspaceID: "ws-1", PlatformName: "azure_ad", OrgID: "ws-1"} + + warm := NewCachedRolesLister(fake, store, false, nil) + if _, err := warm.ListOnDemandResources(t.Context(), req); err != nil { + t.Fatalf("prewarm: %v", err) + } + if fake.callCount != 1 { + t.Fatalf("prewarm: expected 1 inner call, got %d", fake.callCount) + } + + refreshed := NewCachedRolesLister(fake, store, true, nil) + roles, err := refreshed.ListOnDemandResources(t.Context(), req) + if err != nil { + t.Fatalf("refresh: %v", err) + } + if len(roles) != 1 { + t.Errorf("refresh: expected 1 role, got %d", len(roles)) + } + if fake.callCount != 2 { + t.Errorf("refresh: expected inner called again, got callCount=%d", fake.callCount) + } + + // Cache should have been rewritten — a non-refresh lister reads it without re-calling inner. + hit := NewCachedRolesLister(fake, store, false, nil) + if _, err := hit.ListOnDemandResources(t.Context(), req); err != nil { + t.Fatalf("hit: %v", err) + } + if fake.callCount != 2 { + t.Errorf("hit after refresh: expected cache hit, got callCount=%d", fake.callCount) + } +} + func TestOnDemandRolesCacheKey_HandlesSlashes(t *testing.T) { key := onDemandRolesCacheKey("azure_resource", "/providers/Microsoft.Management/managementGroups/abc") for _, c := range key { From bfceab33726a83afdb1945b752673d7226155c6d Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Tue, 21 Apr 2026 07:38:17 +0200 Subject: [PATCH 3/4] fix: normalize leading slash in azure_resource ancestors to prevent double-slash Workspace IDs for SUBSCRIPTION/RESOURCE_GROUP/RESOURCE (and MANAGEMENT_GROUP orgID) can arrive with a leading slash from the SCA eligibility API (e.g. /subscriptions/abc-123). Prepending "/" naively produced double-slash ancestors. Use ensureLeadingSlash() to strip any existing leading slashes before re-adding one. Addresses Copilot review comment on PR #42. --- cmd/request_submit.go | 15 ++++++++++----- cmd/request_test.go | 36 ++++++++++++++++++++++++------------ 2 files changed, 34 insertions(+), 17 deletions(-) diff --git a/cmd/request_submit.go b/cmd/request_submit.go index 0ec9387..095807b 100644 --- a/cmd/request_submit.go +++ b/cmd/request_submit.go @@ -529,7 +529,12 @@ func resolveSubmitRole(ctx context.Context, ws *submitWorkspace, refresh bool) ( } // buildOnDemandRequest maps a workspace into the on-demand discovery request. -// Only directory / AWS account / management-group workspaces are supported in v1. +// ensureLeadingSlash returns s with exactly one leading slash. +func ensureLeadingSlash(s string) string { + return "/" + strings.TrimLeft(s, "/") +} + +// buildOnDemandRequest maps a workspace into the on-demand discovery request. func buildOnDemandRequest(ws *submitWorkspace) (models.OnDemandRequest, error) { wt := strings.ToUpper(string(ws.WorkspaceType)) switch wt { @@ -552,8 +557,8 @@ func buildOnDemandRequest(ws *submitWorkspace) (models.OnDemandRequest, error) { OrgID: ws.OrganizationID, ResourceType: "management_group", Ancestors: []string{ - "/" + ws.OrganizationID, - "/" + ws.WorkspaceID, + ensureLeadingSlash(ws.OrganizationID), + ensureLeadingSlash(ws.WorkspaceID), }, }, nil case "SUBSCRIPTION", "RESOURCE_GROUP", "RESOURCE": @@ -568,8 +573,8 @@ func buildOnDemandRequest(ws *submitWorkspace) (models.OnDemandRequest, error) { OrgID: ws.OrganizationID, ResourceType: resourceType, Ancestors: []string{ - "/" + ws.OrganizationID, - "/" + ws.WorkspaceID, + ensureLeadingSlash(ws.OrganizationID), + ensureLeadingSlash(ws.WorkspaceID), }, }, nil default: diff --git a/cmd/request_test.go b/cmd/request_test.go index 40bf88f..34d5338 100644 --- a/cmd/request_test.go +++ b/cmd/request_test.go @@ -661,19 +661,23 @@ func TestBuildOnDemandRequest_UnsupportedType(t *testing.T) { func TestBuildOnDemandRequest_SupportedTypes(t *testing.T) { tests := []struct { - name string - wt models.WorkspaceType - wsID string - orgID string - wantPlatform string - wantAnces int + name string + wt models.WorkspaceType + wsID string + orgID string + wantPlatform string + wantResourceType string + wantAnces int }{ - {"directory", models.WorkspaceType("DIRECTORY"), "dir-1", "dir-1", "azure_ad", 0}, - {"account", models.WorkspaceType("ACCOUNT"), "123", "123", "aws", 0}, - {"management_group", models.WorkspaceType("MANAGEMENT_GROUP"), "providers/Microsoft.Management/managementGroups/root", "dir-456", "azure_resource", 2}, - {"subscription", models.WorkspaceType("SUBSCRIPTION"), "sub-1", "dir-456", "azure_resource", 2}, - {"resource_group", models.WorkspaceType("RESOURCE_GROUP"), "rg-1", "dir-456", "azure_resource", 2}, - {"resource", models.WorkspaceType("RESOURCE"), "res-1", "dir-456", "azure_resource", 2}, + {"directory", models.WorkspaceType("DIRECTORY"), "dir-1", "dir-1", "azure_ad", "", 0}, + {"account", models.WorkspaceType("ACCOUNT"), "123", "123", "aws", "", 0}, + // Real IDs have no leading slash for management groups. + {"management_group", models.WorkspaceType("MANAGEMENT_GROUP"), "providers/Microsoft.Management/managementGroups/root", "dir-456", "azure_resource", "management_group", 2}, + // Real subscription/RG/resource IDs include a leading slash (e.g. /subscriptions/...). + {"subscription_noslash", models.WorkspaceType("SUBSCRIPTION"), "subscriptions/abc-123", "dir-456", "azure_resource", "subscription", 2}, + {"subscription_slash", models.WorkspaceType("SUBSCRIPTION"), "/subscriptions/abc-123", "dir-456", "azure_resource", "subscription", 2}, + {"resource_group", models.WorkspaceType("RESOURCE_GROUP"), "/subscriptions/abc/resourceGroups/rg1", "dir-456", "azure_resource", "resource_group", 2}, + {"resource", models.WorkspaceType("RESOURCE"), "/subscriptions/abc/resourceGroups/rg1/providers/Microsoft.Compute/virtualMachines/vm1", "dir-456", "azure_resource", "resource", 2}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -688,9 +692,17 @@ func TestBuildOnDemandRequest_SupportedTypes(t *testing.T) { if req.PlatformName != tt.wantPlatform { t.Errorf("platform: got %q want %q", req.PlatformName, tt.wantPlatform) } + if tt.wantResourceType != "" && req.ResourceType != tt.wantResourceType { + t.Errorf("resourceType: got %q want %q", req.ResourceType, tt.wantResourceType) + } if len(req.Ancestors) != tt.wantAnces { t.Errorf("ancestors: got %d want %d", len(req.Ancestors), tt.wantAnces) } + for _, a := range req.Ancestors { + if strings.HasPrefix(a, "//") { + t.Errorf("ancestor has double slash: %q", a) + } + } }) } } From 2243f4034263e0a8277f49d2d1adf59c6887bb9e Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Tue, 21 Apr 2026 07:43:25 +0200 Subject: [PATCH 4/4] fix: set CSP on selector item in interactive_mode_success test to eliminate race fetchEligibility with no --provider fetches all CSPs concurrently. The mock returned the same response for both AZURE and AWS queries, so allTargets contained the same workspace ID tagged with each CSP in non-deterministic order. resolveTargetCSP would pick whichever appeared first, causing the "az CLI session" Azure post-elevation message to appear or not depending on goroutine scheduling. Fix: set CSP: models.CSPAzure on the selector item so resolveTargetCSP short-circuits at the target.CSP != "" early return. --- cmd/root_elevate_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/cmd/root_elevate_test.go b/cmd/root_elevate_test.go index aae264d..1ae1ea3 100644 --- a/cmd/root_elevate_test.go +++ b/cmd/root_elevate_test.go @@ -75,6 +75,7 @@ func TestRootElevate_InteractiveMode(t *testing.T) { WorkspaceID: "sub-456", WorkspaceName: "Prod-EastUS", WorkspaceType: models.WorkspaceTypeSubscription, + CSP: models.CSPAzure, RoleInfo: models.RoleInfo{ ID: "role-789", Name: "Contributor",