diff --git a/TODO.md b/TODO.md index 7fb2ba3..d5bd714 100644 --- a/TODO.md +++ b/TODO.md @@ -8,9 +8,6 @@ The product is not complete until the global exit criteria at the end of this fi ## Priority Bugs -- Vault root view bug: - add API-token path translation helpers so token editing, approval prompts, and - audit/resource display stop leaking the physical `keepass` root. - Vault root view bug: ensure vault create/open maintains the physical `keepass` storage root, lazily creates recycle-bin structure when needed, and warns when opening a diff --git a/internal/api/server.go b/internal/api/server.go index 5aba80b..469ee4b 100644 --- a/internal/api/server.go +++ b/internal/api/server.go @@ -1221,6 +1221,7 @@ func (s *Server) authorizeTemplateRequest(ctx context.Context, op apitokens.Oper func (s *Server) authorizeResourceRequest(ctx context.Context, token apitokens.Token, op apitokens.Operation, resource apitokens.Resource) (apitokens.Token, error) { model, _ := s.snapshotModel() + displayResource := displayAuthorizationResource(resource) switch evaluateAuthorization(model, token, op, resource) { case apitokens.DecisionAllow: return token, nil @@ -1233,9 +1234,9 @@ func (s *Server) authorizeResourceRequest(ctx context.Context, token apitokens.T TokenName: token.Name, ClientName: token.ClientName, Operation: op, - Resource: resource, + Resource: displayResource, }) - result, err := s.approvals.Request(ctx, token, op, resource) + result, err := s.approvals.Request(ctx, token, op, displayResource) if result.Rule != nil { if persistErr := s.persistApprovalRule(token.ID, *result.Rule); persistErr != nil { return apitokens.Token{}, status.Errorf(codes.Internal, "persist approval decision: %v", persistErr) @@ -1249,7 +1250,7 @@ func (s *Server) authorizeResourceRequest(ctx context.Context, token apitokens.T TokenName: token.Name, ClientName: token.ClientName, Operation: op, - Resource: resource, + Resource: displayResource, }) return token, nil case errors.Is(err, apiapproval.ErrRequestDenied): @@ -1259,7 +1260,7 @@ func (s *Server) authorizeResourceRequest(ctx context.Context, token apitokens.T TokenName: token.Name, ClientName: token.ClientName, Operation: op, - Resource: resource, + Resource: displayResource, }) return apitokens.Token{}, status.Error(codes.PermissionDenied, "access denied by user approval") case errors.Is(err, apiapproval.ErrRequestCanceled): @@ -1269,7 +1270,7 @@ func (s *Server) authorizeResourceRequest(ctx context.Context, token apitokens.T TokenName: token.Name, ClientName: token.ClientName, Operation: op, - Resource: resource, + Resource: displayResource, }) return apitokens.Token{}, status.Error(codes.Unauthenticated, "authorization request canceled") case errors.Is(err, apiapproval.ErrRequestTimedOut): @@ -1279,7 +1280,7 @@ func (s *Server) authorizeResourceRequest(ctx context.Context, token apitokens.T TokenName: token.Name, ClientName: token.ClientName, Operation: op, - Resource: resource, + Resource: displayResource, }) return apitokens.Token{}, status.Error(codes.DeadlineExceeded, "authorization request timed out") case errors.Is(err, context.Canceled): @@ -1355,6 +1356,11 @@ func canonicalizeAuthorizationResource(model vault.Model, resource apitokens.Res return resource } +func displayAuthorizationResource(resource apitokens.Resource) apitokens.Resource { + resource.Path = displayAuthorizationPath(resource.Path) + return resource +} + func canonicalAuthorizationPath(model vault.Model, path []string) []string { if len(path) == 0 { return nil @@ -1374,6 +1380,19 @@ func canonicalAuthorizationPath(model vault.Model, path []string) []string { return vaultview.VaultRoot(model).ToPhysicalPath(path) } +func displayAuthorizationPath(path []string) []string { + if len(path) == 0 { + return nil + } + if path[0] == vaultview.KeepassRoot { + return append([]string{"Root"}, append([]string(nil), path[1:]...)...) + } + if path[0] == "Root" { + return append([]string(nil), path...) + } + return append([]string(nil), path...) +} + func copyOperation(target string) apitokens.Operation { switch clipboard.Target(target) { case clipboard.TargetUsername: diff --git a/internal/api/server_test.go b/internal/api/server_test.go index 08b9cfb..85fb51b 100644 --- a/internal/api/server_test.go +++ b/internal/api/server_test.go @@ -396,6 +396,58 @@ func TestVaultServiceFindsBrowserLoginsRechecksChildPoliciesAfterPrompt(t *testi } } +func TestVaultServiceApprovalRequestsUseLogicalRootPathForPhysicalVault(t *testing.T) { + t.Parallel() + + model := vault.Model{ + Entries: []vault.Entry{ + { + ID: "codex-nextcloud", + Title: "Nextcloud (codex)", + Username: "jjulian", + Password: "secret-1", + URL: "https://nextcloud.example.invalid", + Path: []string{"keepass", "Joe", "codex"}, + }, + testAPITokenEntry(t), + }, + Groups: [][]string{ + {"keepass"}, + {"keepass", "Joe"}, + {"keepass", "Joe", "codex"}, + }, + } + client, _, service, cleanup := newTestHarnessForModel(t, model) + defer cleanup() + service.approvals = apiapproval.NewBroker(time.Minute) + + respCh := make(chan *keepassgov1.ListEntriesResponse, 1) + errCh := make(chan error, 1) + go func() { + resp, err := client.ListEntries(tokenContext(defaultTestTokenSecret), &keepassgov1.ListEntriesRequest{ + Path: []string{"Joe", "codex"}, + }) + respCh <- resp + errCh <- err + }() + + pending := waitForServerPendingApproval(t, service, 1)[0] + if got := pending.Resource.Path; !slices.Equal(got, []string{"Root", "Joe", "codex"}) { + t.Fatalf("pending.Resource.Path = %v, want [Root Joe codex]", got) + } + if _, _, err := service.ResolveApproval(pending.ID, apiapproval.OutcomeAllowOnce); err != nil { + t.Fatalf("ResolveApproval(allow once) error = %v", err) + } + + if err := <-errCh; err != nil { + t.Fatalf("ListEntries() error = %v", err) + } + resp := <-respCh + if len(resp.GetEntries()) != 1 || resp.GetEntries()[0].GetId() != "codex-nextcloud" { + t.Fatalf("ListEntries().Entries = %#v, want codex-nextcloud after approval", resp.GetEntries()) + } +} + func TestVaultServiceDoesNotMatchSpecificBrowserEntryToParentDomain(t *testing.T) { t.Parallel() diff --git a/internal/appui/api/model.go b/internal/appui/api/model.go index 1ec92bc..74dc47f 100644 --- a/internal/appui/api/model.go +++ b/internal/appui/api/model.go @@ -71,7 +71,7 @@ func AuditEventSearchTerms(event apiaudit.Event) string { event.ClientName, string(event.Operation), AuditOperationLabel(event.Operation), - strings.Join(event.Resource.Path, " / "), + FormatResourcePath(event.Resource.Path), event.Resource.EntryID, event.Message, } @@ -91,3 +91,17 @@ func AuditEventSearchTerms(event apiaudit.Event) string { } return strings.ToLower(strings.Join(parts, " ")) } + +func DisplayResourcePath(path []string) []string { + if len(path) == 0 { + return nil + } + if path[0] == "keepass" { + return append([]string{"Root"}, append([]string(nil), path[1:]...)...) + } + return append([]string(nil), path...) +} + +func FormatResourcePath(path []string) string { + return strings.Join(DisplayResourcePath(path), " / ") +} diff --git a/internal/appui/api_views.go b/internal/appui/api_views.go index 4a53aa7..2a9d391 100644 --- a/internal/appui/api_views.go +++ b/internal/appui/api_views.go @@ -336,7 +336,7 @@ func (u *ui) editAPIPolicyRuleAction(index int) error { } u.apiPolicyGroupScope = true u.apiPolicyGroupScopeW.Value = true - u.apiPolicyPath.SetText(strings.Join(rule.Resource.Path, " / ")) + u.apiPolicyPath.SetText(apiui.FormatResourcePath(rule.Resource.Path)) u.apiPolicyEntryID.SetText("") return nil } @@ -476,7 +476,7 @@ func policyRuleParts(rule apitokens.PolicyRule) (string, string, string) { if rule.Resource.Kind == apitokens.ResourceEntry { resource = "Entry: " + rule.Resource.EntryID } else if len(rule.Resource.Path) > 0 { - resource = strings.Join(rule.Resource.Path, " / ") + resource = apiui.FormatResourcePath(rule.Resource.Path) } return effect, operation, resource } @@ -1211,5 +1211,5 @@ func formatAuditResource(resource apitokens.Resource) string { if len(resource.Path) == 0 { return "/" } - return strings.Join(resource.Path, " / ") + return apiui.FormatResourcePath(resource.Path) } diff --git a/internal/appui/app.go b/internal/appui/app.go index f6fd130..170ace8 100644 --- a/internal/appui/app.go +++ b/internal/appui/app.go @@ -27,6 +27,7 @@ import ( "git.julianfamily.org/keepassgo/internal/apiaudit" "git.julianfamily.org/keepassgo/internal/apitokens" "git.julianfamily.org/keepassgo/internal/appstate" + apiui "git.julianfamily.org/keepassgo/internal/appui/api" detailmodel "git.julianfamily.org/keepassgo/internal/appui/detail" detaillayout "git.julianfamily.org/keepassgo/internal/appui/detail/layout" lifecyclemodel "git.julianfamily.org/keepassgo/internal/appui/lifecycle" @@ -1511,7 +1512,7 @@ func approvalResourceText(request apiapproval.Request) string { } case apitokens.ResourceGroup: if len(request.Resource.Path) > 0 { - return strings.Join(request.Resource.Path, " / ") + return apiui.FormatResourcePath(request.Resource.Path) } } return "Vault root" diff --git a/internal/appui/main_test.go b/internal/appui/main_test.go index 62bada1..59e5212 100644 --- a/internal/appui/main_test.go +++ b/internal/appui/main_test.go @@ -9355,6 +9355,49 @@ func TestUIAPIPolicyTargetActionsUseCurrentContext(t *testing.T) { } } +func TestUIEditAPIPolicyRuleHidesPhysicalKeepassRoot(t *testing.T) { + t.Parallel() + + token := apitokens.Token{ + ID: "token-1", + Name: "Crew Browser", + Policies: []apitokens.PolicyRule{{ + Effect: apitokens.EffectAllow, + Operation: apitokens.OperationListEntries, + Resource: apitokens.Resource{ + Kind: apitokens.ResourceGroup, + Path: []string{"keepass", "Crew", "bashertarr"}, + }, + }}, + } + u := newUIWithModel("desktop", vault.Model{ + Entries: []vault.Entry{ + token.Entry(apitokens.EntryPath), + }, + }) + u.showAPITokensSection() + u.state.SelectedEntryID = "token-1" + + if err := u.editAPIPolicyRuleAction(0); err != nil { + t.Fatalf("editAPIPolicyRuleAction() error = %v", err) + } + if got := u.apiPolicyPath.Text(); got != "Root / Crew / bashertarr" { + t.Fatalf("apiPolicyPath.Text() = %q, want %q", got, "Root / Crew / bashertarr") + } +} + +func TestUIAuditAndApprovalFormattingHidePhysicalKeepassRoot(t *testing.T) { + t.Parallel() + + resource := apitokens.Resource{Kind: apitokens.ResourceGroup, Path: []string{"keepass", "Crew", "bashertarr"}} + if got := formatAuditResource(resource); got != "Root / Crew / bashertarr" { + t.Fatalf("formatAuditResource() = %q, want %q", got, "Root / Crew / bashertarr") + } + if got := approvalResourceText(apiapproval.Request{Resource: resource}); got != "Root / Crew / bashertarr" { + t.Fatalf("approvalResourceText() = %q, want %q", got, "Root / Crew / bashertarr") + } +} + func TestUIVisibleBreadcrumbsCompressesAggressivelyOnPhone(t *testing.T) { t.Parallel()