From 86568da1153b165d47a79d8a14f1addec5a41289 Mon Sep 17 00:00:00 2001 From: Christian Rocha Date: Tue, 19 May 2026 22:39:24 -0400 Subject: [PATCH] fix(permissions): make permission resolution idempotent across clients When multiple clients are viewing the same session, both can answer the same permission prompt. Without idempotency, the second answer could publish a duplicate notification, block a goroutine, or corrupt session-wide approval state. Permission grants and denials now resolve a request at most once and report back whether the call actually resolved it. A losing persistent grant no longer leaves behind a stale session-wide approval. Also fixes a long standing mix-up where one-time and persistent grants were swapped on the wire when running with a remote server. Co-Authored-By: Charm Crush --- internal/agent/tools/bash_test.go | 16 +- internal/agent/tools/multiedit_test.go | 8 +- internal/agent/tools/view_test.go | 8 +- internal/backend/permission.go | 17 +- internal/client/proto.go | 18 +- internal/permission/permission.go | 105 ++++---- internal/permission/permission_test.go | 271 ++++++++++++++++++++ internal/proto/proto.go | 9 + internal/server/proto.go | 7 +- internal/workspace/app_workspace.go | 12 +- internal/workspace/client_workspace.go | 19 +- internal/workspace/client_workspace_test.go | 88 +++++++ internal/workspace/workspace.go | 14 +- 13 files changed, 504 insertions(+), 88 deletions(-) diff --git a/internal/agent/tools/bash_test.go b/internal/agent/tools/bash_test.go index b9c4a13adbb1f948c9fb85f5cb762bd79906bd68..40169e84e6c691e0ee8272cfcab71dde8ac86762 100644 --- a/internal/agent/tools/bash_test.go +++ b/internal/agent/tools/bash_test.go @@ -21,11 +21,13 @@ func (m *mockBashPermissionService) Request(ctx context.Context, req permission. return true, nil } -func (m *mockBashPermissionService) Grant(req permission.PermissionRequest) {} +func (m *mockBashPermissionService) Grant(req permission.PermissionRequest) bool { return true } -func (m *mockBashPermissionService) Deny(req permission.PermissionRequest) {} +func (m *mockBashPermissionService) Deny(req permission.PermissionRequest) bool { return true } -func (m *mockBashPermissionService) GrantPersistent(req permission.PermissionRequest) {} +func (m *mockBashPermissionService) GrantPersistent(req permission.PermissionRequest) bool { + return true +} func (m *mockBashPermissionService) AutoApproveSession(sessionID string) {} @@ -90,11 +92,13 @@ func (m *recordingPermissionService) Request(ctx context.Context, req permission return m.allow, nil } -func (m *recordingPermissionService) Grant(req permission.PermissionRequest) {} +func (m *recordingPermissionService) Grant(req permission.PermissionRequest) bool { return true } -func (m *recordingPermissionService) Deny(req permission.PermissionRequest) {} +func (m *recordingPermissionService) Deny(req permission.PermissionRequest) bool { return true } -func (m *recordingPermissionService) GrantPersistent(req permission.PermissionRequest) {} +func (m *recordingPermissionService) GrantPersistent(req permission.PermissionRequest) bool { + return true +} func (m *recordingPermissionService) AutoApproveSession(sessionID string) {} diff --git a/internal/agent/tools/multiedit_test.go b/internal/agent/tools/multiedit_test.go index 1ca2a6f7689e345ac944889f1f92284de0652f90..fe56ad6859e896c7a39cd487f7b55e8f59dcbd2f 100644 --- a/internal/agent/tools/multiedit_test.go +++ b/internal/agent/tools/multiedit_test.go @@ -20,11 +20,13 @@ func (m *mockPermissionService) Request(ctx context.Context, req permission.Crea return true, nil } -func (m *mockPermissionService) Grant(req permission.PermissionRequest) {} +func (m *mockPermissionService) Grant(req permission.PermissionRequest) bool { return true } -func (m *mockPermissionService) Deny(req permission.PermissionRequest) {} +func (m *mockPermissionService) Deny(req permission.PermissionRequest) bool { return true } -func (m *mockPermissionService) GrantPersistent(req permission.PermissionRequest) {} +func (m *mockPermissionService) GrantPersistent(req permission.PermissionRequest) bool { + return true +} func (m *mockPermissionService) AutoApproveSession(sessionID string) {} diff --git a/internal/agent/tools/view_test.go b/internal/agent/tools/view_test.go index de853f6cc3f1a0a5b72808983f0fe628f5145f59..43c793a39e94064a43dd27a954a5ed9cbfb572f8 100644 --- a/internal/agent/tools/view_test.go +++ b/internal/agent/tools/view_test.go @@ -216,11 +216,13 @@ func (m *mockViewPermissionService) Request(ctx context.Context, req permission. return true, nil } -func (m *mockViewPermissionService) Grant(req permission.PermissionRequest) {} +func (m *mockViewPermissionService) Grant(req permission.PermissionRequest) bool { return true } -func (m *mockViewPermissionService) Deny(req permission.PermissionRequest) {} +func (m *mockViewPermissionService) Deny(req permission.PermissionRequest) bool { return true } -func (m *mockViewPermissionService) GrantPersistent(req permission.PermissionRequest) {} +func (m *mockViewPermissionService) GrantPersistent(req permission.PermissionRequest) bool { + return true +} func (m *mockViewPermissionService) AutoApproveSession(sessionID string) {} diff --git a/internal/backend/permission.go b/internal/backend/permission.go index bb7876d6989ec8bee6a99362cb5f5ef914fc5c49..d6db237989ac3a85244c8f9ab4c14df1a7afa1d0 100644 --- a/internal/backend/permission.go +++ b/internal/backend/permission.go @@ -6,11 +6,13 @@ import ( ) // GrantPermission grants, denies, or persistently grants a permission -// request. -func (b *Backend) GrantPermission(workspaceID string, req proto.PermissionGrant) error { +// request. The returned bool reports whether this call resolved the +// pending request (true) or found it already resolved by a previous +// caller (false). A false return is not an error. +func (b *Backend) GrantPermission(workspaceID string, req proto.PermissionGrant) (bool, error) { ws, err := b.GetWorkspace(workspaceID) if err != nil { - return err + return false, err } perm := permission.PermissionRequest{ @@ -26,15 +28,14 @@ func (b *Backend) GrantPermission(workspaceID string, req proto.PermissionGrant) switch req.Action { case proto.PermissionAllow: - ws.Permissions.Grant(perm) + return ws.Permissions.Grant(perm), nil case proto.PermissionAllowForSession: - ws.Permissions.GrantPersistent(perm) + return ws.Permissions.GrantPersistent(perm), nil case proto.PermissionDeny: - ws.Permissions.Deny(perm) + return ws.Permissions.Deny(perm), nil default: - return ErrInvalidPermissionAction + return false, ErrInvalidPermissionAction } - return nil } // SetPermissionsSkip sets whether permission prompts are skipped. diff --git a/internal/client/proto.go b/internal/client/proto.go index e17f08dc8b836e7066476ad354c9ea3229e0bfb1..080a8de73f134479e5a9d1c6fd2a34cff5240fa1 100644 --- a/internal/client/proto.go +++ b/internal/client/proto.go @@ -490,17 +490,25 @@ func (c *Client) ListSessions(ctx context.Context, id string) ([]proto.Session, return sessions, nil } -// GrantPermission grants a permission on a workspace. -func (c *Client) GrantPermission(ctx context.Context, id string, req proto.PermissionGrant) error { +// GrantPermission grants a permission on a workspace. The returned +// bool reports whether this call resolved the pending request (true) +// or found it already resolved by a previous caller (false). A false +// value is not an error — it just means another subscriber resolved +// the same request first. +func (c *Client) GrantPermission(ctx context.Context, id string, req proto.PermissionGrant) (bool, error) { rsp, err := c.post(ctx, fmt.Sprintf("/workspaces/%s/permissions/grant", id), nil, jsonBody(req), http.Header{"Content-Type": []string{"application/json"}}) if err != nil { - return fmt.Errorf("failed to grant permission: %w", err) + return false, fmt.Errorf("failed to grant permission: %w", err) } defer rsp.Body.Close() if rsp.StatusCode != http.StatusOK { - return fmt.Errorf("failed to grant permission: status code %d", rsp.StatusCode) + return false, fmt.Errorf("failed to grant permission: status code %d", rsp.StatusCode) } - return nil + var resp proto.PermissionGrantResponse + if err := json.NewDecoder(rsp.Body).Decode(&resp); err != nil { + return false, fmt.Errorf("failed to decode grant permission response: %w", err) + } + return resp.Resolved, nil } // SetPermissionsSkipRequests sets the skip-requests flag for a workspace. diff --git a/internal/permission/permission.go b/internal/permission/permission.go index 6619ce37b05576d049c6ae402d9d946c6affca1f..ea7c21e8a4114ec4c6ace76f020ce2d0e25d4385 100644 --- a/internal/permission/permission.go +++ b/internal/permission/permission.go @@ -63,9 +63,19 @@ type PermissionRequest struct { type Service interface { pubsub.Subscriber[PermissionRequest] - GrantPersistent(permission PermissionRequest) - Grant(permission PermissionRequest) - Deny(permission PermissionRequest) + // GrantPersistent grants a permission request and remembers the grant + // for the session. It returns true if this call actually resolved the + // pending request; false if the request had already been resolved + // (e.g., by another concurrent caller) or is unknown. + GrantPersistent(permission PermissionRequest) bool + // Grant grants a permission request. It returns true if this call + // actually resolved the pending request; false if the request had + // already been resolved or is unknown. + Grant(permission PermissionRequest) bool + // Deny denies a permission request. It returns true if this call + // actually resolved the pending request; false if the request had + // already been resolved or is unknown. + Deny(permission PermissionRequest) bool Request(ctx context.Context, opts CreatePermissionRequest) (bool, error) AutoApproveSession(sessionID string) SetSkipRequests(skip bool) @@ -99,63 +109,72 @@ type permissionService struct { activeRequestMu sync.Mutex } -func (s *permissionService) GrantPersistent(permission PermissionRequest) { - s.notificationBroker.Publish(pubsub.CreatedEvent, PermissionNotification{ - ToolCallID: permission.ToolCallID, - Granted: true, - }) - respCh, ok := s.pendingRequests.Get(permission.ID) - if ok { - respCh <- true +// resolve atomically removes the pending request entry for the given +// permission and, if it was still pending, publishes exactly one +// PermissionNotification and forwards the outcome to the waiter on +// respCh. It returns true if this call resolved the request, false if +// it had already been resolved (e.g., by another concurrent caller) or +// the request ID is unknown. +// +// If onResolve is non-nil it runs after the pending entry has been +// taken but before the notification is published or the waiter is +// unblocked. This lets GrantPersistent record the session permission +// only when it actually wins the race, so a losing GrantPersistent +// that lost to a Deny does not leak an auto-approve entry. +// +// All three public resolution methods (Grant, GrantPersistent, Deny) +// route through this helper so multi-subscriber UIs can race safely: +// the first caller wins, the rest become no-ops. +func (s *permissionService) resolve(permission PermissionRequest, granted, denied bool, onResolve func()) bool { + respCh, ok := s.pendingRequests.Take(permission.ID) + if !ok { + return false } - s.sessionPermissions.Set(PermissionKey{ - SessionID: permission.SessionID, - ToolName: permission.ToolName, - Action: permission.Action, - Path: permission.Path, - }, true) - - s.activeRequestMu.Lock() - if s.activeRequest != nil && s.activeRequest.ID == permission.ID { - s.activeRequest = nil + if onResolve != nil { + onResolve() } - s.activeRequestMu.Unlock() -} -func (s *permissionService) Grant(permission PermissionRequest) { s.notificationBroker.Publish(pubsub.CreatedEvent, PermissionNotification{ ToolCallID: permission.ToolCallID, - Granted: true, + Granted: granted, + Denied: denied, }) - respCh, ok := s.pendingRequests.Get(permission.ID) - if ok { - respCh <- true - } + + // respCh is buffered (cap 1) and only ever has at most one sender + // per request because Take removes the entry under the map lock, + // so this send never blocks. + respCh <- granted s.activeRequestMu.Lock() if s.activeRequest != nil && s.activeRequest.ID == permission.ID { s.activeRequest = nil } s.activeRequestMu.Unlock() + return true } -func (s *permissionService) Deny(permission PermissionRequest) { - s.notificationBroker.Publish(pubsub.CreatedEvent, PermissionNotification{ - ToolCallID: permission.ToolCallID, - Granted: false, - Denied: true, +func (s *permissionService) GrantPersistent(permission PermissionRequest) bool { + // Record the persistent grant only if this call wins the + // pending-request race. Otherwise a losing GrantPersistent that + // lost to a Deny would still leave an auto-approve entry behind, + // silently flipping later denied calls to allowed. + return s.resolve(permission, true, false, func() { + s.sessionPermissions.Set(PermissionKey{ + SessionID: permission.SessionID, + ToolName: permission.ToolName, + Action: permission.Action, + Path: permission.Path, + }, true) }) - respCh, ok := s.pendingRequests.Get(permission.ID) - if ok { - respCh <- false - } +} - s.activeRequestMu.Lock() - if s.activeRequest != nil && s.activeRequest.ID == permission.ID { - s.activeRequest = nil - } - s.activeRequestMu.Unlock() +func (s *permissionService) Grant(permission PermissionRequest) bool { + return s.resolve(permission, true, false, nil) +} + +func (s *permissionService) Deny(permission PermissionRequest) bool { + return s.resolve(permission, false, true, nil) } func (s *permissionService) Request(ctx context.Context, opts CreatePermissionRequest) (bool, error) { diff --git a/internal/permission/permission_test.go b/internal/permission/permission_test.go index 34b06cfe58c4f0e86d23780aa7b9a4b14e51be1a..42f3b40378185c4da5f90f654589ba988ddffa7d 100644 --- a/internal/permission/permission_test.go +++ b/internal/permission/permission_test.go @@ -2,7 +2,9 @@ package permission import ( "sync" + "sync/atomic" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -327,3 +329,272 @@ func TestPermissionService_SequentialProperties(t *testing.T) { assert.True(t, result, "Repeated request should be auto-approved due to persistent permission") }) } + +// TestPermissionService_ResolveIdempotency covers the multi-subscriber +// resolve guarantees added for client/server mode: exactly one +// notification per resolution, racing callers see "already resolved", +// and stray Grant/Deny calls for unknown IDs are safe no-ops. +func TestPermissionService_ResolveIdempotency(t *testing.T) { + t.Parallel() + + t.Run("concurrent grants resolve exactly once", func(t *testing.T) { + t.Parallel() + service := NewPermissionService("/tmp", false, nil) + + events := service.Subscribe(t.Context()) + notifications := service.SubscribeNotifications(t.Context()) + + req := CreatePermissionRequest{ + SessionID: "race-session", + ToolCallID: "race-call", + ToolName: "tool", + Action: "act", + Path: "/tmp/race", + } + + var ( + wg sync.WaitGroup + granted bool + requestErr error + ) + wg.Go(func() { + granted, requestErr = service.Request(t.Context(), req) + }) + + // Wait for the request to be published so we have a real + // PermissionRequest (with its server-side ID) to race on. + var pending PermissionRequest + select { + case ev := <-events: + pending = ev.Payload + case <-time.After(2 * time.Second): + t.Fatal("permission request was never published") + } + + // Drain the initial "request opened" notification (Granted == + // false && Denied == false) so the next read is the resolution + // itself. + select { + case ev := <-notifications: + require.False(t, ev.Payload.Granted, "initial notification must not be granted") + require.False(t, ev.Payload.Denied, "initial notification must not be denied") + case <-time.After(2 * time.Second): + t.Fatal("initial notification was never published") + } + + // Race two grants from two goroutines. + var ( + resolvedCount atomic.Int32 + start = make(chan struct{}) + racers sync.WaitGroup + ) + for range 2 { + racers.Go(func() { + <-start + if service.Grant(pending) { + resolvedCount.Add(1) + } + }) + } + close(start) + racers.Wait() + + // Original Request must return granted exactly once. + wg.Wait() + require.NoError(t, requestErr) + assert.True(t, granted, "request should observe its grant") + + // Exactly one of the two grants resolved the request. + assert.Equal(t, int32(1), resolvedCount.Load(), + "exactly one Grant should report it resolved the request") + + // Exactly one resolution notification, and no further ones. + select { + case ev := <-notifications: + assert.True(t, ev.Payload.Granted, "resolution notification should be granted") + assert.Equal(t, "race-call", ev.Payload.ToolCallID) + case <-time.After(2 * time.Second): + t.Fatal("resolution notification was never published") + } + select { + case ev := <-notifications: + t.Fatalf("unexpected duplicate notification: %+v", ev.Payload) + case <-time.After(50 * time.Millisecond): + // good: no duplicate. + } + + // pendingRequests must be empty: no goroutine is left blocked + // on a send, and a future Grant for the same ID is a no-op. + ps := service.(*permissionService) + assert.Equal(t, 0, ps.pendingRequests.Len(), + "pendingRequests must be empty after resolution") + + assert.False(t, service.Grant(pending), + "a third Grant should report already-resolved") + }) + + t.Run("grant after deny is a no-op", func(t *testing.T) { + t.Parallel() + service := NewPermissionService("/tmp", false, nil) + + events := service.Subscribe(t.Context()) + notifications := service.SubscribeNotifications(t.Context()) + + req := CreatePermissionRequest{ + SessionID: "deny-first", + ToolCallID: "df-call", + ToolName: "tool", + Action: "act", + Path: "/tmp/df", + } + + var ( + wg sync.WaitGroup + granted bool + requestErr error + ) + wg.Go(func() { + granted, requestErr = service.Request(t.Context(), req) + }) + + var pending PermissionRequest + select { + case ev := <-events: + pending = ev.Payload + case <-time.After(2 * time.Second): + t.Fatal("permission request was never published") + } + + // Drain the initial neither-granted-nor-denied notification. + <-notifications + + assert.True(t, service.Deny(pending), "Deny should resolve the request") + wg.Wait() + require.NoError(t, requestErr) + assert.False(t, granted, "request should observe denial") + + // A follow-up Grant must be a no-op and must not flip the + // outcome or publish anything new. + assert.False(t, service.Grant(pending), + "Grant after Deny should report already-resolved") + + select { + case ev := <-notifications: + // The first resolution notification (denial) is expected; + // anything after that is a bug. + require.True(t, ev.Payload.Denied, + "the only post-initial notification must be the denial") + case <-time.After(2 * time.Second): + t.Fatal("denial notification was never published") + } + select { + case ev := <-notifications: + t.Fatalf("Grant after Deny must not publish: %+v", ev.Payload) + case <-time.After(50 * time.Millisecond): + // good. + } + }) + + t.Run("losing GrantPersistent does not record session permission", func(t *testing.T) { + t.Parallel() + service := NewPermissionService("/tmp", false, nil) + + events := service.Subscribe(t.Context()) + notifications := service.SubscribeNotifications(t.Context()) + + req := CreatePermissionRequest{ + SessionID: "race-persist", + ToolCallID: "rp-call", + ToolName: "tool", + Action: "act", + Path: "/tmp/rp", + } + + var ( + wg sync.WaitGroup + granted bool + requestErr error + ) + wg.Go(func() { + granted, requestErr = service.Request(t.Context(), req) + }) + + // Wait for the request to be published so we have the real + // pending PermissionRequest to race on. + var pending PermissionRequest + select { + case ev := <-events: + pending = ev.Payload + case <-time.After(2 * time.Second): + t.Fatal("permission request was never published") + } + + // Drain the initial neither-granted-nor-denied notification. + <-notifications + + // Deny wins, then a competing GrantPersistent loses. + assert.True(t, service.Deny(pending), "Deny should resolve the request") + assert.False(t, service.GrantPersistent(pending), + "GrantPersistent after Deny should report already-resolved") + + wg.Wait() + require.NoError(t, requestErr) + assert.False(t, granted, "request should observe denial") + + // The losing GrantPersistent must not have inserted an + // auto-approve entry. Issue a matching follow-up request and + // confirm the service still publishes a pending request (i.e. + // not auto-approved). We then Deny it to drain the goroutine. + var ( + wg2 sync.WaitGroup + granted2 bool + requestErr2 error + ) + wg2.Go(func() { + granted2, requestErr2 = service.Request(t.Context(), req) + }) + + select { + case ev := <-events: + assert.Equal(t, pending.SessionID, ev.Payload.SessionID) + service.Deny(ev.Payload) + case <-time.After(2 * time.Second): + t.Fatal("follow-up request was auto-approved; persistent grant leaked") + } + + wg2.Wait() + require.NoError(t, requestErr2) + assert.False(t, granted2, "follow-up request should be denied, not auto-approved") + }) + + t.Run("grant for unknown id is a safe no-op", func(t *testing.T) { + t.Parallel() + service := NewPermissionService("/tmp", false, nil) + + notifications := service.SubscribeNotifications(t.Context()) + + bogus := PermissionRequest{ + ID: "does-not-exist", + ToolCallID: "ghost", + ToolName: "tool", + Action: "act", + Path: "/tmp/ghost", + } + + assert.NotPanics(t, func() { + assert.False(t, service.Grant(bogus), + "Grant for unknown ID should report already-resolved") + assert.False(t, service.GrantPersistent(bogus), + "GrantPersistent for unknown ID should report already-resolved") + assert.False(t, service.Deny(bogus), + "Deny for unknown ID should report already-resolved") + }) + + select { + case ev := <-notifications: + t.Fatalf("unknown-ID resolution must not publish: %+v", ev.Payload) + case <-time.After(50 * time.Millisecond): + // good: no notification. + } + }) +} diff --git a/internal/proto/proto.go b/internal/proto/proto.go index fbd71f33da3a330cfe7c14112ead7763d4b4d948..35a2abf8d84c9b7ed28e07b173cfeba4c72d56ef 100644 --- a/internal/proto/proto.go +++ b/internal/proto/proto.go @@ -86,6 +86,15 @@ type PermissionGrant struct { Action PermissionAction `json:"action"` } +// PermissionGrantResponse is the server's response to a permission +// grant call. Resolved is true when this call resolved the pending +// request, and false when the request had already been resolved by a +// previous caller (e.g., another client in a multi-subscriber UI). A +// false value is not an error. +type PermissionGrantResponse struct { + Resolved bool `json:"resolved"` +} + // PermissionSkipRequest represents a request to skip permission prompts. type PermissionSkipRequest struct { Skip bool `json:"skip"` diff --git a/internal/server/proto.go b/internal/server/proto.go index 0523904a3d2d4317da9a4afcea40985b675b41ba..51d1d58eec905834992cde9a434608e2028bfc13 100644 --- a/internal/server/proto.go +++ b/internal/server/proto.go @@ -898,7 +898,7 @@ func (c *controllerV1) handleGetWorkspaceAgentDefaultSmallModel(w http.ResponseW // @Accept json // @Param id path string true "Workspace ID" // @Param request body proto.PermissionGrant true "Permission grant" -// @Success 200 +// @Success 200 {object} proto.PermissionGrantResponse // @Failure 400 {object} proto.Error // @Failure 404 {object} proto.Error // @Failure 500 {object} proto.Error @@ -913,11 +913,12 @@ func (c *controllerV1) handlePostWorkspacePermissionsGrant(w http.ResponseWriter return } - if err := c.backend.GrantPermission(id, req); err != nil { + resolved, err := c.backend.GrantPermission(id, req) + if err != nil { c.handleError(w, r, err) return } - w.WriteHeader(http.StatusOK) + jsonEncode(w, proto.PermissionGrantResponse{Resolved: resolved}) } // handlePostWorkspacePermissionsSkip sets whether to skip permission prompts. diff --git a/internal/workspace/app_workspace.go b/internal/workspace/app_workspace.go index d4e5ed790e3a1cf7a4bcc299e4e4e63bedfbacd5..0e9460854dc59c63ffc0bcd411aa838df2b68ca1 100644 --- a/internal/workspace/app_workspace.go +++ b/internal/workspace/app_workspace.go @@ -173,16 +173,16 @@ func (w *AppWorkspace) GetDefaultSmallModel(providerID string) config.SelectedMo // -- Permissions -- -func (w *AppWorkspace) PermissionGrant(perm permission.PermissionRequest) { - w.app.Permissions.Grant(perm) +func (w *AppWorkspace) PermissionGrant(perm permission.PermissionRequest) bool { + return w.app.Permissions.Grant(perm) } -func (w *AppWorkspace) PermissionGrantPersistent(perm permission.PermissionRequest) { - w.app.Permissions.GrantPersistent(perm) +func (w *AppWorkspace) PermissionGrantPersistent(perm permission.PermissionRequest) bool { + return w.app.Permissions.GrantPersistent(perm) } -func (w *AppWorkspace) PermissionDeny(perm permission.PermissionRequest) { - w.app.Permissions.Deny(perm) +func (w *AppWorkspace) PermissionDeny(perm permission.PermissionRequest) bool { + return w.app.Permissions.Deny(perm) } func (w *AppWorkspace) PermissionSkipRequests() bool { diff --git a/internal/workspace/client_workspace.go b/internal/workspace/client_workspace.go index 82fde1c5bbcf8393d854f98ecff6aa2a64fe0de9..ad292ddcb5e380152f46d8d7bd5eece0e1c384c6 100644 --- a/internal/workspace/client_workspace.go +++ b/internal/workspace/client_workspace.go @@ -244,8 +244,8 @@ func (w *ClientWorkspace) GetDefaultSmallModel(providerID string) config.Selecte // -- Permissions -- -func (w *ClientWorkspace) PermissionGrant(perm permission.PermissionRequest) { - _ = w.client.GrantPermission(context.Background(), w.workspaceID(), proto.PermissionGrant{ +func (w *ClientWorkspace) PermissionGrant(perm permission.PermissionRequest) bool { + resolved, _ := w.client.GrantPermission(context.Background(), w.workspaceID(), proto.PermissionGrant{ Permission: proto.PermissionRequest{ ID: perm.ID, SessionID: perm.SessionID, @@ -256,12 +256,13 @@ func (w *ClientWorkspace) PermissionGrant(perm permission.PermissionRequest) { Path: perm.Path, Params: perm.Params, }, - Action: proto.PermissionAllowForSession, + Action: proto.PermissionAllow, }) + return resolved } -func (w *ClientWorkspace) PermissionGrantPersistent(perm permission.PermissionRequest) { - _ = w.client.GrantPermission(context.Background(), w.workspaceID(), proto.PermissionGrant{ +func (w *ClientWorkspace) PermissionGrantPersistent(perm permission.PermissionRequest) bool { + resolved, _ := w.client.GrantPermission(context.Background(), w.workspaceID(), proto.PermissionGrant{ Permission: proto.PermissionRequest{ ID: perm.ID, SessionID: perm.SessionID, @@ -272,12 +273,13 @@ func (w *ClientWorkspace) PermissionGrantPersistent(perm permission.PermissionRe Path: perm.Path, Params: perm.Params, }, - Action: proto.PermissionAllow, + Action: proto.PermissionAllowForSession, }) + return resolved } -func (w *ClientWorkspace) PermissionDeny(perm permission.PermissionRequest) { - _ = w.client.GrantPermission(context.Background(), w.workspaceID(), proto.PermissionGrant{ +func (w *ClientWorkspace) PermissionDeny(perm permission.PermissionRequest) bool { + resolved, _ := w.client.GrantPermission(context.Background(), w.workspaceID(), proto.PermissionGrant{ Permission: proto.PermissionRequest{ ID: perm.ID, SessionID: perm.SessionID, @@ -290,6 +292,7 @@ func (w *ClientWorkspace) PermissionDeny(perm permission.PermissionRequest) { }, Action: proto.PermissionDeny, }) + return resolved } func (w *ClientWorkspace) PermissionSkipRequests() bool { diff --git a/internal/workspace/client_workspace_test.go b/internal/workspace/client_workspace_test.go index 43d7e3a0b0554d8028541e91f952797338c3038f..6b0adcdd37215b6b65ff3a88a9d57cb86a764e8f 100644 --- a/internal/workspace/client_workspace_test.go +++ b/internal/workspace/client_workspace_test.go @@ -1,9 +1,16 @@ package workspace import ( + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "net/url" "testing" + "github.com/charmbracelet/crush/internal/client" "github.com/charmbracelet/crush/internal/message" + "github.com/charmbracelet/crush/internal/permission" "github.com/charmbracelet/crush/internal/proto" "github.com/stretchr/testify/require" ) @@ -44,3 +51,84 @@ func TestProtoToMessageToolResult(t *testing.T) { require.Equal(t, `{"file_path":"/tmp/x","content":"hi"}`, tr.Metadata) require.False(t, tr.IsError) } + +// TestClientWorkspace_PermissionGrantMapping verifies that +// PermissionGrant on the ClientWorkspace serializes a one-time grant +// (proto.PermissionAllow) and PermissionGrantPersistent serializes a +// persistent grant (proto.PermissionAllowForSession). A swap between +// these two would silently flip "allow once" into "remember for the +// session", and vice versa, so we pin the wire mapping here. +func TestClientWorkspace_PermissionGrantMapping(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + call func(*ClientWorkspace, permission.PermissionRequest) + want proto.PermissionAction + }{ + { + name: "Grant -> PermissionAllow", + call: func(w *ClientWorkspace, p permission.PermissionRequest) { + w.PermissionGrant(p) + }, + want: proto.PermissionAllow, + }, + { + name: "GrantPersistent -> PermissionAllowForSession", + call: func(w *ClientWorkspace, p permission.PermissionRequest) { + w.PermissionGrantPersistent(p) + }, + want: proto.PermissionAllowForSession, + }, + { + name: "Deny -> PermissionDeny", + call: func(w *ClientWorkspace, p permission.PermissionRequest) { + w.PermissionDeny(p) + }, + want: proto.PermissionDeny, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + var got proto.PermissionGrant + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, http.MethodPost, r.Method) + require.Equal(t, "/v1/workspaces/ws-1/permissions/grant", r.URL.Path) + body, err := io.ReadAll(r.Body) + require.NoError(t, err) + require.NoError(t, json.Unmarshal(body, &got)) + require.NoError(t, json.NewEncoder(w).Encode(proto.PermissionGrantResponse{Resolved: true})) + })) + defer srv.Close() + + u, err := url.Parse(srv.URL) + require.NoError(t, err) + c, err := client.NewClient(t.TempDir(), "tcp", u.Host) + require.NoError(t, err) + + ws := NewClientWorkspace(c, proto.Workspace{ID: "ws-1"}) + + perm := permission.PermissionRequest{ + ID: "req-1", + SessionID: "sess-1", + ToolCallID: "tc-1", + ToolName: "tool", + Description: "do thing", + Action: "act", + Path: "/tmp/p", + } + tc.call(ws, perm) + + require.Equal(t, tc.want, got.Action) + require.Equal(t, "req-1", got.Permission.ID) + require.Equal(t, "sess-1", got.Permission.SessionID) + require.Equal(t, "tc-1", got.Permission.ToolCallID) + require.Equal(t, "tool", got.Permission.ToolName) + require.Equal(t, "act", got.Permission.Action) + require.Equal(t, "/tmp/p", got.Permission.Path) + }) + } +} diff --git a/internal/workspace/workspace.go b/internal/workspace/workspace.go index 02c54c616f3251140bbee441451c3a4cb14845bd..3f317f2eb25986ef2bee084f0a11b471a05e7f50 100644 --- a/internal/workspace/workspace.go +++ b/internal/workspace/workspace.go @@ -89,9 +89,17 @@ type Workspace interface { GetDefaultSmallModel(providerID string) config.SelectedModel // Permissions - PermissionGrant(perm permission.PermissionRequest) - PermissionGrantPersistent(perm permission.PermissionRequest) - PermissionDeny(perm permission.PermissionRequest) + // + // PermissionGrant, PermissionGrantPersistent, and PermissionDeny + // return true if the call resolved the pending request and false if + // it had already been resolved by another subscriber (or is no + // longer pending). A false return is not an error; the modal can + // still close locally because the resolution will arrive via the + // PermissionNotification event stream regardless of which client + // won the race. + PermissionGrant(perm permission.PermissionRequest) bool + PermissionGrantPersistent(perm permission.PermissionRequest) bool + PermissionDeny(perm permission.PermissionRequest) bool PermissionSkipRequests() bool PermissionSetSkipRequests(skip bool)