diff --git a/.gitignore b/.gitignore index 22612d066..65cbb6e2c 100644 --- a/.gitignore +++ b/.gitignore @@ -11,3 +11,4 @@ process_models/ .env* .cache .mypy_cache +.aider* diff --git a/spiffworkflow-frontend/src/hooks/PermissionService.tsx b/spiffworkflow-frontend/src/hooks/PermissionService.tsx index 017ea4cb4..debfbe2e5 100644 --- a/spiffworkflow-frontend/src/hooks/PermissionService.tsx +++ b/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); }; - checkPermissions(permissionsToCheck, processPermissionResult); + + /** + * 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 }; diff --git a/spiffworkflow-frontend/src/services/PermissionCacheService.test.ts b/spiffworkflow-frontend/src/services/PermissionCacheService.test.ts new file mode 100644 index 000000000..e6b994232 --- /dev/null +++ b/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); + }); +}); diff --git a/spiffworkflow-frontend/src/services/PermissionCacheService.ts b/spiffworkflow-frontend/src/services/PermissionCacheService.ts new file mode 100644 index 000000000..ef8c34a0c --- /dev/null +++ b/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>(); + +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 = {}; + 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, +}; diff --git a/spiffworkflow-frontend/src/services/UserService.ts b/spiffworkflow-frontend/src/services/UserService.ts index ac27a518c..4efe40182 100644 --- a/spiffworkflow-frontend/src/services/UserService.ts +++ b/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; };