Bug/perm check cache (#1184)

* Update PermissionService.tsx

Semi-naive cache to prevent redundant perm calls.

* Permission Cache (and reading/updating/wiping)

Simple cache Map to avoid making piles of redundant permission checks.

* Update PermissionCacheService.ts

Coderabbit comment.

* Update PermissionCacheService.ts

A little more elegant.

* Update PermissionCacheService.ts

Rolled validation back in.

* Update PermissionCacheService.ts

A little more elegant.

* Update permissionsService to be a little more robust, and test spec

* Update to always run callback

Now creates a response from the cache to feed the callback.

Still seeing the cypress error though (and remember you have to adjust the PermssionsServiceCache tests).

* Permission cache enhancements

PermissionCache now stores all returned permission for a path (without duplicates). Incoming permission requests now find and (if found) return the PermissionVerbResults they were looking for (based on the verbs in the PermissionsToCheck) from the overall collection for a given path.

* Update PermissionCacheService.ts

* Adjust Cache Tests

* fix lint

* Update PermissionService.tsx

Lint error ugh.

* kills consoles, add aider ignore

---------

Co-authored-by: Kevin Burnett <18027+burnettk@users.noreply.github.com>
Co-authored-by: burnettk <burnettk@users.noreply.github.com>

.gitignore

@@ -11,3 +11,4 @@ process_models/
 .env*
 .cache
 .mypy_cache
+.aider*

spiffworkflow-frontend/src/hooks/PermissionService.tsx

@@ -5,6 +5,10 @@ import { useContext, useEffect, useState } from 'react';
 import { AbilityContext } from '../contexts/Can';
 import { PermissionCheckResponseBody, PermissionsToCheck } from '../interfaces';
 import HttpService from '../services/HttpService';
+import {
+  findPermissionsInCache,
+  updatePermissionsCache,
+} from '../services/PermissionCacheService';
 
 export const checkPermissions = (
   permissionsToCheck: PermissionsToCheck,
@@ -49,9 +53,23 @@ export const usePermissionFetcher = (
         }
       });
       ability.update(rules);
+
+      // Update the cache with the new permissions
+      updatePermissionsCache(result);
       setPermissionsLoaded(true);
     };
+
+    /**
+     * Are the incoming permission requests all in the cache?
+     * If not, make the backend call, update the cache, and process the results.
+     * Otherwise, use the cached results.
+     */
+    const foundResults = findPermissionsInCache(permissionsToCheck);
+    if (foundResults) {
+      processPermissionResult(foundResults);
+    } else {
       checkPermissions(permissionsToCheck, processPermissionResult);
+    }
   });
 
   return { ability, permissionsLoaded };

spiffworkflow-frontend/src/services/PermissionCacheService.test.ts

@@ -0,0 +1,56 @@
+import {
+  updatePermissionsCache,
+  inspectPermissionsCache,
+  clearPermissionsCache,
+} from './PermissionCacheService';
+
+describe('updatePermissionsCache', () => {
+  it('should update the permission cache with the provided permissions', () => {
+    const permissionsResponse = {
+      results: {
+        '/path1': { GET: true },
+        '/path2': { POST: true },
+      },
+    };
+    updatePermissionsCache(permissionsResponse);
+    // Note the cache stores the perms in an array
+    expect(inspectPermissionsCache().get('/path1')).toEqual([{ GET: true }]);
+    expect(inspectPermissionsCache().get('/path2')).toEqual([{ POST: true }]);
+  });
+  it('should update the permissions cache with the new permissions', () => {
+    // Add more permissions to a given path
+    const permissionsResponse = {
+      results: {
+        '/path1': { POST: true },
+        '/path2': { GET: true },
+      },
+    };
+    updatePermissionsCache(permissionsResponse);
+    // Each path should now have the new permissions added to the existing ones
+    expect(inspectPermissionsCache().get('/path1')).toEqual([
+      { GET: true },
+      { POST: true },
+    ]);
+    expect(inspectPermissionsCache().get('/path2')).toEqual([
+      { POST: true },
+      { GET: true },
+    ]);
+  });
+  it('cache should be unchanged if results are empty', () => {
+    const permissionsResponse = { results: {} };
+    updatePermissionsCache(permissionsResponse);
+    expect(inspectPermissionsCache().get('/path1')).toEqual([
+      { GET: true },
+      { POST: true },
+    ]);
+    expect(inspectPermissionsCache().get('/path2')).toEqual([
+      { POST: true },
+      { GET: true },
+    ]);
+    expect(inspectPermissionsCache().size).toEqual(2);
+  });
+  it('should clear the permissions cache when told to', () => {
+    clearPermissionsCache();
+    expect(inspectPermissionsCache().size).toEqual(0);
+  });
+});

spiffworkflow-frontend/src/services/PermissionCacheService.ts

@@ -0,0 +1,93 @@
+/**
+ * There can be a lot of redundant requests for permissions (probably deps/contexts firing etc.)
+ * This service provides a cache to check for already-processed perms.
+ */
+import {
+  PermissionCheckResponseBody,
+  PermissionVerbResults,
+  PermissionsToCheck,
+} from '../interfaces';
+
+/** Map makes sense: no prototype to hack, high perf, easily wiped. */
+const permissionsCache = new Map<string, Array<PermissionVerbResults>>();
+
+const updatePermissionsCache = (
+  permissionsResponse: PermissionCheckResponseBody
+) => {
+  if (Object.entries(permissionsResponse.results).length > 0) {
+    Object.entries(permissionsResponse.results).forEach(
+      ([path, permissions]) => {
+        if (permissionsCache.has(path)) {
+          const cachedPermissions = permissionsCache.get(path);
+          // Make sure we don't add duplicate PermissionVerb objects
+          if (
+            cachedPermissions &&
+            !cachedPermissions.some(
+              (p) => JSON.stringify(p) === JSON.stringify(permissions)
+            )
+          ) {
+            cachedPermissions.push(permissions);
+          }
+        } else {
+          permissionsCache.set(path, [permissions]);
+        }
+      }
+    );
+  }
+};
+
+/**
+ * Look for the permissions in the cache. If satisfied, we don't need to make a backend call.
+ * Then, create a response object and return it so we can complete any callbacks.
+ */
+const findPermissionsInCache = (
+  permissionsToCheck: PermissionsToCheck
+): PermissionCheckResponseBody | null => {
+  const results: Record<string, PermissionVerbResults> = {};
+  if (permissionsToCheck) {
+    Object.entries(permissionsToCheck).forEach(([path, verbs]) => {
+      const cachedPermissions = permissionsCache.get(path);
+      if (cachedPermissions) {
+        const found = cachedPermissions.find((p) => {
+          // Note that the project config doesn't seem to support "hasOwn"
+          return Object.prototype.hasOwnProperty.call(p, verbs[0]);
+        });
+
+        if (found) {
+          results[path] = found;
+        }
+      }
+    });
+
+    console.log(results);
+  }
+
+  /**
+   * Results must have content, and must be the same number of permissions as we're checking
+   * TODO: This implementation can lead to redundant permissions requests if
+   * the same individual permission is requested by different permission sets
+   * (any perm in a set not found will trigger a backend call for the whole set).
+   * This is erring on the side of caution for now, but a more robust individual
+   * checker might be useful.
+   */
+  return Object.keys(results).length > 0 &&
+    Object.keys(results).length === Object.keys(permissionsToCheck).length
+    ? { results: results as any }
+    : null;
+};
+
+// Don't allow retrieval or manipulation of the cache directly
+const inspectPermissionsCache = () => {
+  return new Map(permissionsCache);
+};
+
+const clearPermissionsCache = () => {
+  permissionsCache.clear();
+};
+
+export {
+  updatePermissionsCache,
+  findPermissionsInCache,
+  clearPermissionsCache,
+  inspectPermissionsCache,
+};

spiffworkflow-frontend/src/services/UserService.ts

@@ -3,6 +3,7 @@ import cookie from 'cookie';
 import { BACKEND_BASE_URL } from '../config';
 import { AuthenticationOption } from '../interfaces';
 import { parseTaskShowUrl } from '../helpers';
+import { clearPermissionsCache } from './PermissionCacheService';
 
 // NOTE: this currently stores the jwt token in local storage
 // which is considered insecure. Server set cookies seem to be considered
@@ -101,6 +102,10 @@ const doLogout = () => {
     logoutRedirectUrl += '&backend_only=true';
   }
 
+  // Wipe all cached permissions so if user logs back in
+  // (either as themselves or a different user), they get the correct permissions
+  clearPermissionsCache();
+
   window.location.href = logoutRedirectUrl;
 };