From 18918db1d42b769efc4f6c4cd0ebea567fce8da2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Karl-Einar=20Bergstr=C3=B8m?= Date: Sun, 11 Oct 2026 14:23:14 +0200 Subject: [PATCH] refactor: no group list in an account's details, and no Microsoft Graph call for it Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/azure.md | 10 ++-- internal/provider/azure/account.go | 70 ------------------------- internal/provider/azure/account_test.go | 47 ----------------- internal/provider/azure/azure.go | 12 ++--- internal/provider/provider.go | 7 --- internal/ui/app/app.go | 10 +--- internal/ui/app/app_test.go | 33 +++--------- internal/ui/app/fields.go | 69 +++--------------------- 8 files changed, 26 insertions(+), 232 deletions(-) diff --git a/docs/azure.md b/docs/azure.md index 2686d85..6a20e0f 100644 --- a/docs/azure.md +++ b/docs/azure.md @@ -16,12 +16,10 @@ A service principal is used instead when `AZURE_CLIENT_ID`, On an account's group row, the fourth pane shows who it signs in as: name, user, tenant, object ID, whether the session used MFA, directory roles (built-in ones by name) and its subscriptions, read from az's account list -and the token lazysecrets already has. Below them are the groups the -account is in, nested ones too: those come from Microsoft Graph, once a -session, the first time the pane shows them. `y` on the row copies the -object ID, the one access policies and role assignments ask for; `tab` -goes into the pane, where `j`/`k` pick any field or group, `/` filters -them (`/tenant`, `/platform`) and `y` copies the selected one. +and the token lazysecrets already has: no requests of their own. `y` on +the row copies the object ID, the one access policies and role +assignments ask for; `l` goes into the pane, where `j`/`k` pick any +field, `/` filters them (`/tenant`) and `y` copies the selected one. If an account's az login has expired, the status line says so once, and the Details pane on that account's rows says it until you sign in: press `L` diff --git a/internal/provider/azure/account.go b/internal/provider/azure/account.go index 690234e..8b85691 100644 --- a/internal/provider/azure/account.go +++ b/internal/provider/azure/account.go @@ -1,15 +1,10 @@ package azure import ( - "context" - "encoding/json" "fmt" - "net/http" "slices" "strings" - "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" - "git.kebcloud.no/keb/lazysecrets/internal/provider" ) @@ -160,68 +155,3 @@ func directoryRoles(wids any) string { slices.Sort(names) return strings.Join(names, ", ") } - -// graphGroups lists the signed-in user's groups, nested ones too, from -// Microsoft Graph with az's token for it. -const graphGroups = "https://graph.microsoft.com/v1.0/me/transitiveMemberOf?$select=displayName&$top=999" - -var _ provider.AccountGroups = (*Provider)(nil) - -// Groups lists the groups the account is a member of, by name. -func (p *Provider) Groups(ctx context.Context) ([]string, error) { - if p.account == nil || (p.account.cloud != "" && p.account.cloud != "AzureCloud") { - return nil, provider.ErrUnsupported - } - tok, err := p.cred.GetToken(ctx, policy.TokenRequestOptions{Scopes: []string{"https://graph.microsoft.com/.default"}}) - if err != nil { - return nil, err - } - var names []string - for next := graphGroups; next != ""; { - req, err := http.NewRequestWithContext(ctx, http.MethodGet, next, nil) - if err != nil { - return nil, err - } - req.Header.Set("Authorization", "Bearer "+tok.Token) - resp, err := p.http().Do(req) - if err != nil { - return nil, err - } - var page struct { - Value []struct { - Type string `json:"@odata.type"` - Name string `json:"displayName"` - } `json:"value"` - Next string `json:"@odata.nextLink"` - Error *struct { - Message string `json:"message"` - } `json:"error"` - } - err = json.NewDecoder(resp.Body).Decode(&page) - resp.Body.Close() - switch { - case err != nil: - return nil, fmt.Errorf("listing groups from Microsoft Graph: %w", err) - case page.Error != nil: - return nil, fmt.Errorf("listing groups from Microsoft Graph: %s", page.Error.Message) - case resp.StatusCode != http.StatusOK: - return nil, fmt.Errorf("listing groups from Microsoft Graph: %s", resp.Status) - } - for _, v := range page.Value { - if v.Type == "#microsoft.graph.group" && v.Name != "" { - names = append(names, v.Name) - } - } - next = page.Next - } - slices.SortFunc(names, func(a, b string) int { return strings.Compare(strings.ToLower(a), strings.ToLower(b)) }) - return names, nil -} - -// http is the client for Microsoft Graph; tests replace it. -func (p *Provider) http() *http.Client { - if p.graphClient != nil { - return p.graphClient - } - return http.DefaultClient -} diff --git a/internal/provider/azure/account_test.go b/internal/provider/azure/account_test.go index a951ce8..0093ea9 100644 --- a/internal/provider/azure/account_test.go +++ b/internal/provider/azure/account_test.go @@ -1,17 +1,7 @@ package azure import ( - "context" - "errors" - "io" - "net/http" - "slices" - "strings" "testing" - "time" - - "github.com/Azure/azure-sdk-for-go/sdk/azcore" - "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" "git.kebcloud.no/keb/lazysecrets/internal/provider" ) @@ -75,40 +65,3 @@ func TestSignInMethods(t *testing.T) { } } } - -type staticToken struct{} - -func (staticToken) GetToken(context.Context, policy.TokenRequestOptions) (azcore.AccessToken, error) { - return azcore.AccessToken{Token: "tok", ExpiresOn: time.Now().Add(time.Hour)}, nil -} - -type graphPages map[string]string // by URL - -func (g graphPages) RoundTrip(r *http.Request) (*http.Response, error) { - body, ok := g[r.URL.String()] - if !ok || r.Header.Get("Authorization") != "Bearer tok" { - return &http.Response{StatusCode: 404, Status: "404 Not Found", Body: io.NopCloser(strings.NewReader(`{}`))}, nil - } - return &http.Response{StatusCode: 200, Status: "200 OK", Body: io.NopCloser(strings.NewReader(body))}, nil -} - -func TestGroups(t *testing.T) { - next := "https://graph.microsoft.com/v1.0/next" - p := &Provider{ - cred: staticToken{}, - account: &accountDetails{account: account{user: "alice@contoso.com"}}, - graphClient: &http.Client{Transport: graphPages{ - graphGroups: `{"value":[{"@odata.type":"#microsoft.graph.group","displayName":"vault readers"}, - {"@odata.type":"#microsoft.graph.directoryRole","displayName":"Global Reader"}],"@odata.nextLink":"` + next + `"}`, - next: `{"value":[{"@odata.type":"#microsoft.graph.group","displayName":"Platform team"}]}`, - }}, - } - got, err := p.Groups(context.Background()) - if err != nil || !slices.Equal(got, []string{"Platform team", "vault readers"}) { - t.Errorf("groups %q, %v", got, err) - } - p.account.cloud = "AzureChinaCloud" - if _, err := p.Groups(context.Background()); !errors.Is(err, provider.ErrUnsupported) { - t.Errorf("other cloud: %v", err) - } -} diff --git a/internal/provider/azure/azure.go b/internal/provider/azure/azure.go index f3fda6f..4a7bbc1 100644 --- a/internal/provider/azure/azure.go +++ b/internal/provider/azure/azure.go @@ -9,7 +9,6 @@ import ( "crypto/x509" "errors" "fmt" - "net/http" "regexp" "slices" "strings" @@ -37,12 +36,11 @@ var objectTypes = []provider.ObjectType{provider.TypeSecret, provider.TypeKey, p type Provider struct { id, label string // "" for the single-account defaults - cred azcore.TokenCredential - opts options - account *accountDetails // nil for a service principal from AZURE_* - graphClient *http.Client // for Groups; nil for the default - subs *armsubscriptions.Client - graph *armresourcegraph.Client + cred azcore.TokenCredential + opts options + account *accountDetails // nil for a service principal from AZURE_* + subs *armsubscriptions.Client + graph *armresourcegraph.Client mu sync.Mutex vaults map[string]*vaultClients // by vault URL diff --git a/internal/provider/provider.go b/internal/provider/provider.go index c47dccc..2b3ae9f 100644 --- a/internal/provider/provider.go +++ b/internal/provider/provider.go @@ -227,10 +227,3 @@ func (d *Denied) Unwrap() error { return d.Err } type ContainerDetails interface { Details(ctx context.Context, c Container) ([]Attr, error) } - -// AccountGroups is implemented by providers that can list the groups -// their account is a member of, directly or through other groups. It -// goes over the network. -type AccountGroups interface { - Groups(ctx context.Context) ([]string, error) -} diff --git a/internal/ui/app/app.go b/internal/ui/app/app.go index 9fd1c8e..4604886 100644 --- a/internal/ui/app/app.go +++ b/internal/ui/app/app.go @@ -172,12 +172,10 @@ type Model struct { clip Clipboard clipClear time.Duration openURL func(string) error - // fields is the fourth pane for anything but a secret, with each - // account's groups as they load; lastLeft is the left pane it - // describes while the fourth has focus (fields.go). + // fields is the fourth pane for anything but a secret; lastLeft is + // the left pane it describes while the fourth has focus (fields.go). fields panes.Fields lastLeft pane - groups map[string]groupsState details map[string]detailsState // by containerKey getenv func(string) string nvimOpen func(server, path string, line int) error // edit.go @@ -377,7 +375,6 @@ func New(providers []provider.Provider, opts Options) Model { getenv: os.Getenv, nvimOpen: nvimOpen, cache: newCache(), - groups: map[string]groupsState{}, details: map[string]detailsState{}, fields: panes.NewFields(), } @@ -590,9 +587,6 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { case persistedMsg: m.handlePersisted(msg) - case groupsMsg: - m.groups[msg.provider] = groupsState{names: msg.names, err: msg.err} - case detailsMsg: if msg.req == m.reqID[slotDetails] { m.done(slotDetails) diff --git a/internal/ui/app/app_test.go b/internal/ui/app/app_test.go index 948907b..deac928 100644 --- a/internal/ui/app/app_test.go +++ b/internal/ui/app/app_test.go @@ -1106,25 +1106,16 @@ func TestAccountInDetails(t *testing.T) { } } -// grouped is an account in two groups. -type grouped struct{ described } - -func (grouped) Groups(context.Context) ([]string, error) { - return []string{"Platform team", "Vault readers"}, nil -} - func TestAccountPane(t *testing.T) { t.Parallel() - d := newDriver(t, grouped{described{fake.Azure(0)}}) - d.keys("g", "tab") // the account's row, then straight to its pane + d := newDriver(t, described{fake.Azure(0)}) + d.keys("g", "l") // the account's row, then its pane if d.m.focus != paneDetail { t.Fatalf("focus %v, want the fourth pane", d.m.focus) } scr := d.screen() - for _, want := range []string{"Alice Example", "groups 2", "Platform team", "Vault readers"} { - if !strings.Contains(scr, want) { - t.Errorf("missing %q:\n%s", want, scr) - } + if !strings.Contains(scr, "Alice Example") { + t.Errorf("account not shown:\n%s", scr) } bar := lastLine(scr) if !strings.Contains(bar, "copy") || strings.Contains(bar, "reveal") || strings.Contains(bar, "versions") { @@ -1134,28 +1125,20 @@ func TestAccountPane(t *testing.T) { if got := d.clip().content; got != "oid-123" { t.Errorf("y on the object ID copied %q", got) } - d.keys("G", "y") - if got := d.clip().content; got != "Vault readers" { - t.Errorf("y on a group copied %q", got) - } // / filters it, labels too. d.keys("/") - d.typeText("vault") + d.typeText("object") d.keys("enter") - if scr := d.screen(); !strings.Contains(scr, "Vault readers") || strings.Contains(scr, "Platform team") { + if scr := d.screen(); !strings.Contains(scr, "oid-123") || strings.Contains(scr, "Alice Example") { t.Errorf("filtered:\n%s", scr) } - d.keys("y") - if got := d.clip().content; got != "Vault readers" { - t.Errorf("y in the filtered pane copied %q", got) - } d.keys("esc") - if !strings.Contains(d.screen(), "Platform team") { + if !strings.Contains(d.screen(), "Alice Example") { t.Errorf("filter not cleared:\n%s", d.screen()) } - d.keys("shift+tab") + d.keys("h") if d.m.focus != paneSources { t.Errorf("back to %v, want Sources", d.m.focus) } diff --git a/internal/ui/app/fields.go b/internal/ui/app/fields.go index cba5c27..bb6fa6f 100644 --- a/internal/ui/app/fields.go +++ b/internal/ui/app/fields.go @@ -1,7 +1,6 @@ package app import ( - "context" "errors" "fmt" "strings" @@ -14,20 +13,7 @@ import ( ) // The fourth pane shows Fields for anything but a secret. On a provider's -// row: its account, from what the provider knows locally, then the -// groups the account is in, loaded once a session when first shown. - -type groupsState struct { - names []string - err error - loading bool -} - -type groupsMsg struct { - provider string - names []string - err error -} +// row: its account, from what the provider knows locally. // A vault's, scope's or file's details beyond its listing load once a // session, when the cursor rests on it, and again with refresh. They're @@ -57,7 +43,6 @@ func (m Model) detailOf() pane { // syncFields shows the Fields pane for what the focused pane has selected, // unless that's a secret (or a vault with the Secrets pane in focus), which // the secret pane shows, as it does a revealed value whatever the focus. -// An account's groups load the first time. func (m *Model) syncFields() tea.Cmd { if m.detail.State() != panes.ValueHidden { // A value revealed (or being fetched) from any pane stays in @@ -76,7 +61,7 @@ func (m *Model) syncFields() tea.Cmd { case m.selectedProblem() != "": m.fields.Show(cmpOr(r.label, r.source.Name), m.problemFields(m.selectedProblem())) case r.kind == rowGroup: - return m.showAccount(r) + m.showAccount(r) case r.kind == rowSource: m.fields.Show(r.source.Name, m.sourceFields(r.source)) default: @@ -98,9 +83,8 @@ func (m *Model) syncFields() tea.Cmd { return nil } -// showAccount shows a provider's account, loading its groups the first -// time. -func (m *Model) showAccount(r sourceRow) tea.Cmd { +// showAccount shows a provider's account. +func (m *Model) showAccount(r sourceRow) { p := m.byID[r.provider] var fields []panes.Field if a, ok := p.(provider.Accounts); ok { @@ -110,28 +94,11 @@ func (m *Model) showAccount(r sourceRow) tea.Cmd { } } } - var cmd tea.Cmd - if g, ok := p.(provider.AccountGroups); ok { - st, seen := m.groups[r.provider] - if !seen { - st = groupsState{loading: true} - m.groups[r.provider] = st - id, timeout := r.provider, m.timeout - cmd = func() tea.Msg { - ctx, cancel := context.WithTimeout(context.Background(), timeout) - defer cancel() - names, err := g.Groups(ctx) - return groupsMsg{provider: id, names: names, err: err} - } - } - fields = append(fields, groupFields(st)...) - } if res, done := m.sourceResults[r.provider]; done && res.err == nil && len(res.sources) == 0 { hint := emptyHint(p.Kind()) fields = append(fields, panes.Field{Label: "nothing", Value: "found: try", Info: true}, panes.Field{Value: hint.Value, URL: hint.URL}) } m.fields.Show(p.Label(), fields) - return cmd } // sourceFields describe a subscription, workspace or repo. @@ -248,30 +215,8 @@ func (m Model) problemFields(problem string) []panes.Field { return out } -// fieldName names a copied field: its label, or for a group, the group. +// fieldName names a copied field: its label, or a row of several under +// one label (a tag), its value. func fieldName(f panes.Field) string { - if f.Label == "" || strings.HasPrefix(f.Label, "groups") { - return "group " + f.Value - } - return f.Label -} - -// groupFields are the groups' rows: labelled on the first. -func groupFields(st groupsState) []panes.Field { - switch { - case st.loading: - return []panes.Field{{Label: "groups", Value: "loading…", Info: true}} - case errors.Is(st.err, provider.ErrUnsupported): - return nil - case st.err != nil: - return []panes.Field{{Label: "groups", Value: "couldn't list them: " + st.err.Error(), Info: true}} - case len(st.names) == 0: - return []panes.Field{{Label: "groups", Value: "none", Info: true}} - } - out := make([]panes.Field, len(st.names)) - for i, n := range st.names { - out[i] = panes.Field{Value: n} - } - out[0].Label = fmt.Sprintf("groups %d", len(st.names)) - return out + return cmpOr(f.Label, f.Value) }