From 8021506d288b150aaca56fdf4bbcc3f3cc3af11e Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Tue, 21 Apr 2026 08:29:17 +0200 Subject: [PATCH 1/2] fix: memoize ISP auth to eliminate duplicate auth cycles in grant request submit bootstrapISPAuth() now uses sync.Once so profile load + Authenticate runs exactly once per process, shared by bootstrapSCAService and bootstrapWorkflowsService. Also passes refreshAuth=false so cached, unexpired keyring tokens skip the network round-trip entirely. Reduces StartAuthentication calls from 3 to 1 for `grant request submit` and cuts keyring ops from ~18 to ~6 per invocation. --- CHANGELOG.md | 4 ++++ cmd/request.go | 17 ++++------------- cmd/root.go | 47 +++++++++++++++++++++++++++++++++++++++++------ cmd/root_test.go | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 96 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f5aba00..0b5fdec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,10 @@ All notable changes to this project will be documented in this file. - `--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. +### Fixed + +- `grant request submit` no longer performs 2–3 back-to-back ISP authentication cycles. Profile load + `Authenticate` is now memoized per-invocation and shared by `bootstrapSCAService` / `bootstrapWorkflowsService`. `Authenticate` is also called with `refreshAuth=false` so cached, unexpired keyring tokens short-circuit the network round-trip. Keyring ops drop from ~18× to ~6× per invocation. + ### Added - `grant request` command group for managing access requests through the approval workflow diff --git a/cmd/request.go b/cmd/request.go index 59068ac..79645c2 100644 --- a/cmd/request.go +++ b/cmd/request.go @@ -8,9 +8,6 @@ import ( "github.com/aaearon/grant-cli/internal/workflows" "github.com/aaearon/grant-cli/internal/workflows/models" - "github.com/cyberark/idsec-sdk-golang/pkg/auth" - authmodels "github.com/cyberark/idsec-sdk-golang/pkg/models/auth" - "github.com/cyberark/idsec-sdk-golang/pkg/profiles" "github.com/spf13/cobra" ) @@ -54,18 +51,12 @@ func NewRequestCommandWithDeps(reqSvc accessRequestService) *cobra.Command { } // bootstrapWorkflowsService creates an authenticated AccessRequestService. +// Reuses the memoized ISP auth from bootstrapISPAuth so repeated bootstraps in +// a single invocation (e.g. `grant request submit`) share one auth cycle. func bootstrapWorkflowsService() (*workflows.AccessRequestService, error) { - loader := profiles.DefaultProfilesLoader() - profile, err := (*loader).LoadProfile("grant") + ispAuth, _, err := bootstrapISPAuth() if err != nil { - return nil, fmt.Errorf("failed to load profile: %w", err) - } - - ispAuth := auth.NewIdsecISPAuth(true) - - _, err = ispAuth.Authenticate(profile, nil, &authmodels.IdsecSecret{Secret: ""}, false, true) - if err != nil { - return nil, fmt.Errorf("authentication failed: %w", err) + return nil, err } svc, err := workflows.NewAccessRequestService(ispAuth) diff --git a/cmd/root.go b/cmd/root.go index 1a80e7c..b2d8dd5 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -129,19 +129,54 @@ Examples: var rootCmd = newRootCommand(runElevateProduction) -// bootstrapSCAService loads the profile, authenticates, and creates the SCA service. -func bootstrapSCAService() (auth.IdsecAuth, *sca.SCAAccessService, *sdkmodels.IdsecProfile, error) { +// bootstrap memoization state — shared auth/profile across all service +// bootstraps within a single process invocation so we authenticate exactly once. +var ( + bootstrapOnce sync.Once + bootstrapISPAuthVal auth.IdsecAuth + bootstrapProfileVal *sdkmodels.IdsecProfile + errBootstrap error +) + +// bootstrapImpl is the function that performs the profile load + authentication. +// Overridable for tests. refreshAuth=false lets the SDK reuse cached keyring +// tokens while still valid; a full re-auth only happens on token expiry. +var bootstrapImpl = func() (auth.IdsecAuth, *sdkmodels.IdsecProfile, error) { loader := profiles.DefaultProfilesLoader() profile, err := (*loader).LoadProfile("grant") if err != nil { - return nil, nil, nil, fmt.Errorf("failed to load profile: %w", err) + return nil, nil, fmt.Errorf("failed to load profile: %w", err) } - ispAuth := auth.NewIdsecISPAuth(true) + if _, err := ispAuth.Authenticate(profile, nil, &authmodels.IdsecSecret{Secret: ""}, false, false); err != nil { + return nil, nil, fmt.Errorf("authentication failed: %w", err) + } + return ispAuth, profile, nil +} - _, err = ispAuth.Authenticate(profile, nil, &authmodels.IdsecSecret{Secret: ""}, false, true) +// bootstrapISPAuth returns a process-wide memoized auth.IdsecAuth and profile so +// repeated service bootstraps in one invocation share a single auth cycle. +func bootstrapISPAuth() (auth.IdsecAuth, *sdkmodels.IdsecProfile, error) { + bootstrapOnce.Do(func() { + bootstrapISPAuthVal, bootstrapProfileVal, errBootstrap = bootstrapImpl() + }) + return bootstrapISPAuthVal, bootstrapProfileVal, errBootstrap +} + +// resetBootstrapCache clears the memoized auth state. Intended for tests. +func resetBootstrapCache() { + bootstrapOnce = sync.Once{} + bootstrapISPAuthVal = nil + bootstrapProfileVal = nil + errBootstrap = nil +} + +// bootstrapSCAService loads the profile, authenticates, and creates the SCA service. +// The underlying auth is memoized across calls within a single invocation. +func bootstrapSCAService() (auth.IdsecAuth, *sca.SCAAccessService, *sdkmodels.IdsecProfile, error) { + ispAuth, profile, err := bootstrapISPAuth() if err != nil { - return nil, nil, nil, fmt.Errorf("authentication failed: %w", err) + return nil, nil, nil, err } svc, err := sca.NewSCAAccessService(ispAuth) diff --git a/cmd/root_test.go b/cmd/root_test.go index 41c3c04..f4526ce 100644 --- a/cmd/root_test.go +++ b/cmd/root_test.go @@ -8,10 +8,57 @@ import ( "github.com/aaearon/grant-cli/internal/sca/models" "github.com/aaearon/grant-cli/internal/ui" + sdkauth "github.com/cyberark/idsec-sdk-golang/pkg/auth" "github.com/cyberark/idsec-sdk-golang/pkg/config" + sdkmodels "github.com/cyberark/idsec-sdk-golang/pkg/models" "github.com/spf13/cobra" ) +func TestBootstrapISPAuth_MemoizesAcrossServiceBootstraps(t *testing.T) { + resetBootstrapCache() + t.Cleanup(resetBootstrapCache) + + origImpl := bootstrapImpl + t.Cleanup(func() { bootstrapImpl = origImpl }) + + calls := 0 + stubAuth := sdkauth.NewIdsecISPAuth(true) // never Authenticate'd — used only for identity + stubProfile := &sdkmodels.IdsecProfile{} + bootstrapImpl = func() (sdkauth.IdsecAuth, *sdkmodels.IdsecProfile, error) { + calls++ + return stubAuth, stubProfile, nil + } + + ispAuth1, profile1, err := bootstrapISPAuth() + if err != nil { + t.Fatalf("bootstrapISPAuth #1: %v", err) + } + ispAuth2, profile2, err := bootstrapISPAuth() + if err != nil { + t.Fatalf("bootstrapISPAuth #2: %v", err) + } + ispAuth3, profile3, err := bootstrapISPAuth() + if err != nil { + t.Fatalf("bootstrapISPAuth #3: %v", err) + } + + if calls != 1 { + t.Errorf("expected bootstrapImpl to be invoked exactly once across 3 calls, got %d", calls) + } + if ispAuth1 != ispAuth2 || ispAuth2 != ispAuth3 { + t.Error("expected ispAuth to be the same instance across all bootstrap calls") + } + if profile1 != profile2 || profile2 != profile3 { + t.Error("expected profile to be the same instance across all bootstrap calls") + } + if ispAuth1 != stubAuth { + t.Error("expected returned auth to match stub") + } + if profile1 != stubProfile { + t.Error("expected returned profile to match stub") + } +} + func TestNewRootCommand_SilenceFlags(t *testing.T) { cmd := newRootCommand(nil) From cc3aade15e9067c82b453b91752eb5c5d03f3032 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Tue, 21 Apr 2026 08:47:20 +0200 Subject: [PATCH 2/2] fix: address code review feedback on bootstrap memoization - Use *sync.Once (pointer) instead of sync.Once value to avoid copylocks concern when resetting in tests - Use NewIdsecISPAuth(false) in test stub (non-caching; Authenticate never called so no keyring ops in CI) - Rename test to TestBootstrapISPAuth_MemoizesRepeatCalls to accurately describe what is tested --- cmd/root.go | 5 +++-- cmd/root_test.go | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/cmd/root.go b/cmd/root.go index b2d8dd5..a448fc3 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -132,7 +132,7 @@ var rootCmd = newRootCommand(runElevateProduction) // bootstrap memoization state — shared auth/profile across all service // bootstraps within a single process invocation so we authenticate exactly once. var ( - bootstrapOnce sync.Once + bootstrapOnce = new(sync.Once) bootstrapISPAuthVal auth.IdsecAuth bootstrapProfileVal *sdkmodels.IdsecProfile errBootstrap error @@ -164,8 +164,9 @@ func bootstrapISPAuth() (auth.IdsecAuth, *sdkmodels.IdsecProfile, error) { } // resetBootstrapCache clears the memoized auth state. Intended for tests. +// Uses pointer reassignment to avoid copying the sync.Once value. func resetBootstrapCache() { - bootstrapOnce = sync.Once{} + bootstrapOnce = new(sync.Once) bootstrapISPAuthVal = nil bootstrapProfileVal = nil errBootstrap = nil diff --git a/cmd/root_test.go b/cmd/root_test.go index f4526ce..2618e93 100644 --- a/cmd/root_test.go +++ b/cmd/root_test.go @@ -14,7 +14,7 @@ import ( "github.com/spf13/cobra" ) -func TestBootstrapISPAuth_MemoizesAcrossServiceBootstraps(t *testing.T) { +func TestBootstrapISPAuth_MemoizesRepeatCalls(t *testing.T) { resetBootstrapCache() t.Cleanup(resetBootstrapCache) @@ -22,7 +22,7 @@ func TestBootstrapISPAuth_MemoizesAcrossServiceBootstraps(t *testing.T) { t.Cleanup(func() { bootstrapImpl = origImpl }) calls := 0 - stubAuth := sdkauth.NewIdsecISPAuth(true) // never Authenticate'd — used only for identity + stubAuth := sdkauth.NewIdsecISPAuth(false) // non-caching stub; Authenticate never called stubProfile := &sdkmodels.IdsecProfile{} bootstrapImpl = func() (sdkauth.IdsecAuth, *sdkmodels.IdsecProfile, error) { calls++