Fix read-only editor mutations and repeated permission queries

CHANGELOG.md

@@ -34,10 +34,15 @@ Planned version: **1.5.4**.
   `list_paused_steps` return one snapshot per paused step (step id, label, pause time, reason and
   redacted variables) instead of a list of step ids. The MCP result field is now `pausedSteps`
   (#606).
+- Folder and workflow lists reuse one set of permission data per user and request, avoiding an
+  additional database query for each folder while preserving access rules (#576).
 
 ### Fixed
 
 - **Designer**
+  - Read-only workflows keep note and group editing controls disabled. Collapsing or expanding a
+    group changes only the local view, without autosave or an unsaved-changes warning. Save and
+    publish also stop queuing writes when the edit lock is lost (#584).
   - Renaming an output variable updates every reference to it: templates in any field, a dynamic
     target machine or credential, breakpoint conditions, decision cases and edge conditions,
     including templates in their fixed comparison values (#689, #789).
@@ -128,6 +133,9 @@ Planned version: **1.5.4**.
   - The dashboard, Live-Ops and the execution pages update live again instead of waiting for their
     polling interval, and catch up after a reconnect (#706).
 - **Examples, test suite and tooling**
+  - CLI tests now verify write requests and their payloads. Telemetry
+    tests verify actual log delivery over HTTP and gRPC, including collector headers and disabled
+    export settings.
   - The Windows Update health example applies `freshnessDays` (#734). The temp-file cleanup
     example writes a new archive per run and keeps the earlier ones (#723). The README showcase
     reports a failed probe instead of success (#724). The tech demo uses one name for its child

src/NodePilot.Api/Security/ResourceAuthorizationService.cs

@@ -15,7 +15,7 @@ namespace NodePilot.Api.Security;
 /// list/detail endpoint without leaking permissions across requests.
 /// <para>
 /// Permission resolution: walks the folder ancestry from the requested folder up to
-/// Root, collects every grant that targets the user (V1: only PrincipalType=User),
+/// Root, collects every grant that targets the user or an authority-scoped group,
 /// and takes the highest <see cref="SharedFolderRole"/>. Higher-role-implies-lower
 /// is encoded in the private <c>OpRequiresRole</c> helper. Global-Admin bypasses the whole chain.
 /// </para>
@@ -32,9 +32,11 @@ public sealed class ResourceAuthorizationService : IResourceAuthorizationService
     // any caller switching principals mid-scope would otherwise read stale grants. The
     // cost of keying on a Guid pair is one extra hash bucket per lookup.
     private readonly Dictionary<(Guid userId, Guid folderId), List<SharedFolderPermission>> _ancestryGrantsCache = [];
+    private readonly Dictionary<Guid, ILookup<Guid, SharedFolderPermission>> _principalGrantsCache = [];
     private readonly Dictionary<Guid, AccessibleFolderSet> _accessibleFolderSetCache = [];
     private readonly Dictionary<Guid, IReadOnlyCollection<DirectoryGroupPrincipal>> _directoryGroupCache = [];
     private List<SharedWorkflowFolder>? _allFoldersCache;
+    private Dictionary<Guid, SharedWorkflowFolder>? _foldersByIdCache;
 
     public ResourceAuthorizationService(NodePilotDbContext db, IOptions<AuthenticationPolicyOptions>? authenticationPolicy = null)
     {
@@ -100,22 +102,9 @@ public async Task<AccessibleFolderSet> GetAccessibleFolderIdsAsync(ClaimsPrincip
         if (userId is null) return AccessibleFolderSet.None;
         if (_accessibleFolderSetCache.TryGetValue(userId.Value, out var cached)) return cached;
 
-        // Pull every folder + every grant for this user in two queries, then resolve
-        // accessibility entirely in memory. Folder count in any realistic deployment is
-        // bounded (depth 5 means thousands max even at flat-org-of-thousands scale); the
-        // cost of the in-memory walk is negligible compared to per-row DB hits.
+        // Visibility and capabilities use the same per-principal grant snapshot.
         var allFolders = await GetAllFoldersAsync(ct);
-        var directoryGroups = await GetDirectoryGroupsAsync(userId.Value, ct);
-        // Group-aware grant lookup: include any (PrincipalType=Group, PrincipalKey IN
-        // userGroupSids) grant alongside the per-user grants. Local users have an empty
-        // groupSids list so this branch is a no-op for them.
-        var candidateGrants = await _db.SharedFolderPermissions
-            .AsNoTracking()
-            .Where(DirectoryGroupPrincipal.GrantPredicate(userId.Value, directoryGroups))
-            .ToListAsync(ct);
-        var grants = DirectoryGroupPrincipal.ExactMatches(candidateGrants, directoryGroups)
-            .Select(permission => new { permission.FolderId, permission.Role })
-            .ToList();
+        var grants = await GetPrincipalGrantsAsync(userId.Value, ct);
 
         if (grants.Count == 0)
         {
@@ -125,7 +114,7 @@ public async Task<AccessibleFolderSet> GetAccessibleFolderIdsAsync(ClaimsPrincip
 
         // Each grant on a folder F implicitly grants access to F + all descendants of F.
         // Collect the descendant set per granted folder, union them all.
-        var grantedFolderIds = grants.Select(g => g.FolderId).ToHashSet();
+        var grantedFolderIds = grants.Select(g => g.Key).ToHashSet();
         var byParent = allFolders.GroupBy(f => f.ParentFolderId)
             .ToDictionary(g => g.Key ?? Guid.Empty, g => g.ToList());
 
@@ -157,9 +146,11 @@ public async Task<AccessibleFolderSet> GetAccessibleFolderIdsAsync(ClaimsPrincip
     public void InvalidateAll()
     {
         _ancestryGrantsCache.Clear();
+        _principalGrantsCache.Clear();
         _accessibleFolderSetCache.Clear();
         _directoryGroupCache.Clear();
         _allFoldersCache = null;
+        _foldersByIdCache = null;
     }
 
     public async Task<ResourceCapabilities> GetWorkflowCapabilitiesAsync(ClaimsPrincipal user, Guid folderId, CancellationToken ct = default)
@@ -210,8 +201,7 @@ public Task<ResourceCapabilities> GetFolderCapabilitiesAsync(ClaimsPrincipal use
         var userId = TryGetUserId(user);
         if (userId is null) return null;
 
-        var directoryGroups = await GetDirectoryGroupsAsync(userId.Value, ct);
-        var grants = await GetAncestryGrantsAsync(folderId, userId.Value, directoryGroups, ct);
+        var grants = await GetAncestryGrantsAsync(folderId, userId.Value, ct);
         if (grants.Count == 0) return null;
 
         // Highest role wins. Enum is ordered FolderViewer < FolderOperator < FolderEditor <
@@ -222,21 +212,18 @@ public Task<ResourceCapabilities> GetFolderCapabilitiesAsync(ClaimsPrincipal use
     /// <summary>
     /// Returns every <see cref="SharedFolderPermission"/> that grants access to the
     /// folder at <paramref name="folderId"/> for <paramref name="userId"/> — including
-    /// grants on any ancestor folder, and any authority-scoped group grant represented by
-    /// <paramref name="directoryGroups"/>. Cached per (folderId, userId) tuple within the
-    /// request scope (group-membership doesn't change mid-request).
+    /// grants on any ancestor folder, and any authority-scoped group grant.
+    /// Cached per (folderId, userId) tuple within the request scope.
     /// </summary>
     private async Task<List<SharedFolderPermission>> GetAncestryGrantsAsync(
         Guid folderId, Guid userId,
-        IReadOnlyCollection<DirectoryGroupPrincipal> directoryGroups,
         CancellationToken ct)
     {
         var cacheKey = (userId, folderId);
         if (_ancestryGrantsCache.TryGetValue(cacheKey, out var cached)) return cached;
 
         // Build the ancestor chain folderId -> ... -> Root.
-        var allFolders = await GetAllFoldersAsync(ct);
-        var byId = allFolders.ToDictionary(f => f.Id);
+        var byId = _foldersByIdCache ??= (await GetAllFoldersAsync(ct)).ToDictionary(f => f.Id);
         var chain = new List<Guid>();
         var current = folderId;
         var depth = 0;
@@ -254,13 +241,22 @@ private async Task<List<SharedFolderPermission>> GetAncestryGrantsAsync(
             return _ancestryGrantsCache[cacheKey];
         }
 
-        var candidateGrants = await _db.SharedFolderPermissions
-            .AsNoTracking()
-            .Where(p => chain.Contains(p.FolderId))
+        var principalGrants = await GetPrincipalGrantsAsync(userId, ct);
+        var grants = chain.SelectMany(id => principalGrants[id]).ToList();
+        _ancestryGrantsCache[cacheKey] = grants;
+        return grants;
+    }
+
+    private async Task<ILookup<Guid, SharedFolderPermission>> GetPrincipalGrantsAsync(Guid userId, CancellationToken ct)
+    {
+        if (_principalGrantsCache.TryGetValue(userId, out var cached)) return cached;
+        var directoryGroups = await GetDirectoryGroupsAsync(userId, ct);
+        var candidates = await _db.SharedFolderPermissions.AsNoTracking()
             .Where(DirectoryGroupPrincipal.GrantPredicate(userId, directoryGroups))
             .ToListAsync(ct);
-        var grants = DirectoryGroupPrincipal.ExactMatches(candidateGrants, directoryGroups);
-        _ancestryGrantsCache[cacheKey] = grants;
+        var grants = DirectoryGroupPrincipal.ExactMatches(candidates, directoryGroups)
+            .ToLookup(permission => permission.FolderId);
+        _principalGrantsCache[userId] = grants;
         return grants;
     }
 

src/nodepilot-ui/src/__tests__/hooks/useDisplayedGraph.test.tsx

@@ -42,6 +42,29 @@ function edgeData(edges: Edge[], id: string) {
   return (edges.find((e) => e.id === id)!.data ?? {}) as Record<string, unknown>;
 }
 
+it('expands a saved collapsed group for viewing without changing the authored graph', () => {
+  const nodes: Node[] = [
+    { id: 'group', type: 'group', position: { x: 0, y: 0 }, style: { width: 240, height: 74 },
+      data: { collapsed: true, expandedSize: { width: 500, height: 300 } } },
+    { ...NODES[0], parentId: 'group' },
+    NODES[1],
+  ];
+  const edges: Edge[] = [{ id: 'child-edge', source: 'a', target: 'b' }];
+  const authored = structuredClone({ nodes, edges });
+  const { result, rerender } = renderHook(({ overrides }) => useDisplayedGraph(baseArgs({
+    nodes, edges, groupCollapseOverrides: overrides,
+  })), { initialProps: { overrides: new Map([['group', false]]) } });
+  expect(result.current.displayedNodes.map((node) => node.id)).toEqual(['group', 'a', 'b']);
+  expect(result.current.displayedNodes[0].style).toMatchObject({ width: 500, height: 300 });
+  expect(result.current.displayedEdges[0]).toMatchObject({ source: 'a', target: 'b' });
+  expect({ nodes, edges }).toEqual(authored);
+
+  rerender({ overrides: new Map() });
+  expect(result.current.displayedNodes.map((node) => node.id)).toEqual(['group', 'b']);
+  expect(result.current.displayedEdges[0]).toMatchObject({ source: 'group', target: 'b' });
+  expect({ nodes, edges }).toEqual(authored);
+});
+
 describe('useDisplayedGraph — edge detach marker', () => {
   const cleanEdge: Edge = { id: 'e1', source: 'a', target: 'b', type: 'labeled', data: { label: 'On Success' } };
   /** An edge carrying a marker from an earlier session, e.g. a definition saved before the

src/nodepilot-ui/src/__tests__/hooks/useWorkflowPersistence.test.tsx

@@ -44,7 +44,7 @@ function makeWrapper() {
 
 function renderPersistence() {
   return renderHook(
-    () => useWorkflowPersistence({ workflowId: 'wf-1', workflow: undefined, nodes: [], edges: [] }),
+    () => useWorkflowPersistence({ workflowId: 'wf-1', workflow: undefined, canWrite: true, nodes: [], edges: [] }),
     { wrapper: makeWrapper() },
   );
 }
@@ -64,7 +64,7 @@ describe('useWorkflowPersistence — revision-safe requests', () => {
   it('keeps the new workflow dirty when an old unlock completes after navigation', async () => {
     const unlock = deferred<void>();
     const { result, rerender } = renderHook(({ id }) => useWorkflowPersistence({
-      workflowId: id, workflow: undefined, nodes: [], edges: [],
+      workflowId: id, workflow: undefined, canWrite: true, nodes: [], edges: [],
     }), { initialProps: { id: 'a' }, wrapper: makeWrapper() });
     act(() => { result.current.syncFromServer('A', 1); result.current.markDirty(); });
     let pending!: Promise<void>;
@@ -80,13 +80,54 @@ describe('useWorkflowPersistence — revision-safe requests', () => {
     vi.mocked(api.post).mockReset();
   });
 
+  it('stops autosave and explicit writes after write access is lost, and resumes after reacquiring it', async () => {
+    vi.useFakeTimers();
+    try {
+      vi.mocked(api.put).mockResolvedValue(undefined);
+      const { result, rerender } = renderHook(({ canWrite }) => useWorkflowPersistence({
+        workflowId: 'wf-1', workflow: undefined, nodes: [], edges: [], canWrite,
+      }), { initialProps: { canWrite: true }, wrapper: makeWrapper() });
+      act(() => { result.current.syncFromServer('Draft', 1); result.current.markDirty(); });
+      rerender({ canWrite: false });
+
+      await act(async () => { await vi.advanceTimersByTimeAsync(6000); });
+      expect(api.put).not.toHaveBeenCalled();
+      await act(async () => { await result.current.saveAsync(); result.current.publish(); });
+      expect(api.put).not.toHaveBeenCalled();
+      expect(api.post).not.toHaveBeenCalled();
+      expect(result.current.isDirty).toBe(true);
+
+      rerender({ canWrite: true });
+      await act(async () => { await vi.advanceTimersByTimeAsync(5000); });
+      expect(api.put).toHaveBeenCalledTimes(1);
+      expect(result.current.isDirty).toBe(false);
+    } finally { vi.useRealTimers(); }
+  });
+
+  it.each([false, true])('does not follow up a running save after write access is lost (publish queued: %s)', async (queuePublish) => {
+    const saving = deferred<unknown>();
+    vi.mocked(api.put).mockImplementationOnce(() => saving.promise);
+    const { result, rerender } = renderHook(({ canWrite }) => useWorkflowPersistence({
+      workflowId: 'wf-1', workflow: undefined, nodes: [], edges: [], canWrite,
+    }), { initialProps: { canWrite: true }, wrapper: makeWrapper() });
+    act(() => { result.current.syncFromServer('Draft', 1); result.current.markDirty(); result.current.save(); });
+    await waitFor(() => expect(api.put).toHaveBeenCalledTimes(1));
+    act(() => { result.current.markDirty(); if (queuePublish) result.current.publish(); });
+    rerender({ canWrite: false });
+    await act(async () => saving.resolve(undefined));
+    expect(api.put).toHaveBeenCalledTimes(1);
+    expect(api.post).not.toHaveBeenCalled();
+    expect(result.current.isPublishing).toBe(false);
+    expect(result.current.isDirty).toBe(true);
+  });
+
   it.each(['quick-connect', 'edge-insert'])('autosaves a clean draft after %s adds a node', async (operation) => {
     vi.useFakeTimers();
     try {
       const { result } = renderHook(() => {
         const [nodes, setNodes] = React.useState<Node[]>([editableNode('Source')]);
         const [edges, setEdges] = React.useState<Edge[]>([{ id: 'edge-1', source: 'step-1', target: 'step-2' }]);
-        const persistence = useWorkflowPersistence({ workflowId: 'wf-1', workflow: undefined, nodes, edges });
+        const persistence = useWorkflowPersistence({ workflowId: 'wf-1', workflow: undefined, canWrite: true, nodes, edges });
         const { commitHistory } = useWorkflowHistory('wf-1', { nodes, edges, setNodes, setEdges }, persistence.markDirty);
         const connect = useCanvasConnect({ edges, setNodes, setEdges, setSelected: () => {}, commitHistory,
           markDirty: persistence.markDirty,
@@ -121,7 +162,7 @@ describe('useWorkflowPersistence — revision-safe requests', () => {
     const wrapper = makeWrapper();
     const { result, rerender } = renderHook(
       ({ label }) => useWorkflowPersistence({
-        workflowId: 'wf-1', workflow: undefined, nodes: [editableNode(label)], edges: [],
+        workflowId: 'wf-1', workflow: undefined, canWrite: true, nodes: [editableNode(label)], edges: [],
       }),
       { wrapper, initialProps: { label: 'revision-a' } },
     );
@@ -155,7 +196,7 @@ describe('useWorkflowPersistence — revision-safe requests', () => {
     const wrapper = makeWrapper();
     const { result, rerender } = renderHook(
       ({ label }) => useWorkflowPersistence({
-        workflowId: 'wf-1', workflow: undefined, nodes: [editableNode(label)], edges: [],
+        workflowId: 'wf-1', workflow: undefined, canWrite: true, nodes: [editableNode(label)], edges: [],
       }),
       { wrapper, initialProps: { label: 'published-snapshot' } },
     );
@@ -247,7 +288,7 @@ describe('useWorkflowPersistence — revision-safe requests', () => {
     const wrapper = makeWrapper();
     const { result, rerender } = renderHook(
       ({ workflowId }) => useWorkflowPersistence({
-        workflowId, workflow: undefined, nodes: [editableNode('initial')], edges: [],
+        workflowId, workflow: undefined, canWrite: true, nodes: [editableNode('initial')], edges: [],
       }),
       { wrapper, initialProps: { workflowId: 'wf-1' } },
     );
@@ -284,7 +325,7 @@ describe('useWorkflowPersistence — revision-safe requests', () => {
     vi.mocked(api.post).mockResolvedValue({});
     const { result, rerender } = renderHook(
       ({ workflowId }) => useWorkflowPersistence({
-        workflowId, workflow: undefined, nodes: [editableNode(workflowId)], edges: [],
+        workflowId, workflow: undefined, canWrite: true, nodes: [editableNode(workflowId)], edges: [],
       }),
       { wrapper: makeWrapper(), initialProps: { workflowId: 'wf-1' } },
     );