From 779ca4e3cbe958b7715c22e84bf547306a5134f3 Mon Sep 17 00:00:00 2001 From: David Lougheed Date: Wed, 17 Apr 2024 14:11:50 -0400 Subject: [PATCH 1/5] feat: multi-resource hook for fetching permissions BREAKING CHANGE: fetch resource permissions action requires array --- src/hooks.ts | 48 ++++++++++++++--------- src/index.ts | 2 +- src/redux/authSlice.ts | 87 ++++++++++++++++++++++++------------------ 3 files changed, 82 insertions(+), 55 deletions(-) diff --git a/src/hooks.ts b/src/hooks.ts index b178e82..35492df 100644 --- a/src/hooks.ts +++ b/src/hooks.ts @@ -5,7 +5,7 @@ import { ThunkAction } from "redux-thunk"; import { useBentoAuthContext } from "./contexts"; import { Resource, makeResourceKey } from "./resources"; -import { fetchResourcePermissions, refreshTokens, tokenHandoff } from "./redux/authSlice"; +import { fetchResourcesPermissions, refreshTokens, tokenHandoff } from "./redux/authSlice"; import { LS_SIGN_IN_POPUP, createAuthURL } from "./performAuth"; import { fetchOpenIdConfigurationIfNecessary } from "./redux/openIdConfigSlice"; import { getIsAuthenticated, logMissingAuthContext, makeAuthorizationHeader } from "./utils"; @@ -28,35 +28,49 @@ export const useAuthorizationHeader = () => { return useMemo(() => makeAuthorizationHeader(accessToken), [accessToken]); }; -export const useResourcePermissions = (resource: Resource, authzUrl: string) => { +export const useResourcesPermissions = (resources: Resource[], authzUrl: string) => { const dispatch: AppDispatch = useDispatch(); const haveAuthorizationService = !!authzUrl; - const key = useMemo(() => makeResourceKey(resource), [resource]); + const keys = useMemo(() => resources.map((resource) => makeResourceKey(resource)), [resources]); - const { permissions, isFetching, hasAttempted, error } = - useSelector((state: RootState) => state.auth.resourcePermissions?.[key]) ?? {}; + const { resourcePermissions } = useSelector((state: RootState) => state.auth); useEffect(() => { - if (!haveAuthorizationService || isFetching || permissions || hasAttempted) return; - dispatch(fetchResourcePermissions({ resource, authzUrl })); + const allFetching = keys.reduce((acc, key) => acc && !!resourcePermissions[key]?.isFetching, true); + const allHavePermissions = + keys.reduce((acc, key) => acc && !!resourcePermissions[key]?.permissions?.length, true); + const allAttempted = keys.reduce((acc, key) => acc && !!resourcePermissions[key]?.hasAttempted, true); + + if (!haveAuthorizationService || allFetching || allHavePermissions || allAttempted) return; + dispatch(fetchResourcesPermissions({ resources, authzUrl })); }, [ dispatch, haveAuthorizationService, - isFetching, - permissions, - hasAttempted, - resource, + keys, + resourcePermissions, authzUrl, ]); - return { - permissions: permissions ?? [], - isFetching: isFetching ?? false, - hasAttempted: hasAttempted ?? false, - error: error ?? "", - }; + return useMemo(() => Object.fromEntries(keys.map((key) => { + const { permissions, isFetching, hasAttempted, error } = resourcePermissions[key] ?? {}; + return [ + key, + { + permissions: permissions ?? [], + isFetching: isFetching ?? false, + hasAttempted: hasAttempted ?? false, + error: error ?? "", + } + ]; + })), [keys, resourcePermissions]); +}; + +export const useResourcePermissions = (resource: Resource, authzUrl: string) => { + const key = makeResourceKey(resource); + const resourcesPermissions = useResourcesPermissions([resource], authzUrl); + return resourcesPermissions[key]; }; export const useHasResourcePermission = (resource: Resource, authzUrl: string, permission: string) => { diff --git a/src/index.ts b/src/index.ts index 8f0bb79..6f652e1 100644 --- a/src/index.ts +++ b/src/index.ts @@ -10,7 +10,7 @@ export * from "./utils"; export { default as AuthReducer, - fetchResourcePermissions, + fetchResourcesPermissions, refreshTokens, signOut, tokenHandoff diff --git a/src/redux/authSlice.ts b/src/redux/authSlice.ts index fb1eff8..f7e296d 100644 --- a/src/redux/authSlice.ts +++ b/src/redux/authSlice.ts @@ -105,28 +105,33 @@ export const refreshTokens = createAsyncThunk( type FetchPermissionPayload = { result: string[][]; }; -type FetchPermissionParams = { - resource: Resource; +type FetchPermissionsParams = { + resources: Resource[]; authzUrl: string; } -export const fetchResourcePermissions = createAsyncThunk( - "auth/FETCH_RESOURCE_PERMISSIONS", - async ({ resource, authzUrl }: FetchPermissionParams, { getState }) => { +export const fetchResourcesPermissions = createAsyncThunk( + "auth/FETCH_RESOURCES_PERMISSIONS", + async ({ resources, authzUrl }: FetchPermissionsParams, { getState }) => { const url = `${authzUrl}/policy/permissions`; const { auth } = getState() as RootState; const response = await fetch(url, { method: "POST", headers: { "Content-Type": "application/json", Authorization: `Bearer ${auth.accessToken}` }, - body: JSON.stringify({resources: [resource]}), + body: JSON.stringify({ resources }), }); return await response.json(); }, { - condition: ({ resource }, { getState }) => { + condition: ({ resources }, { getState }) => { + // allow action to fire if at least one permission is not being fetched right now (and we need it): + const { auth } = getState() as RootState; - const key = makeResourceKey(resource); - const rp = auth.resourcePermissions?.[key]; - return !rp?.isFetching; + + return resources.reduce((acc, resource) => { + const key = makeResourceKey(resource); + const rp = auth.resourcePermissions?.[key]; + return acc || !rp?.isFetching; + }, false); }, } ); @@ -258,37 +263,45 @@ export const authSlice = createSlice({ setLSNotSignedIn(); }) - .addCase(fetchResourcePermissions.pending, (state, { meta }) => { - const key = makeResourceKey(meta.arg.resource); - state.resourcePermissions[key] = { - ...state.resourcePermissions[key], - isFetching: true, - hasAttempted: false, - permissions: [], - error: "", - }; + .addCase(fetchResourcesPermissions.pending, (state, { meta }) => { + for (const resource of meta.arg.resources) { + const key = makeResourceKey(resource); + state.resourcePermissions[key] = { + ...state.resourcePermissions[key], + isFetching: true, + hasAttempted: false, + permissions: [], + error: "", + }; + } }) - .addCase(fetchResourcePermissions.fulfilled, (state, { meta, payload }) => { - const key = makeResourceKey(meta.arg.resource); - state.resourcePermissions[key] = { - ...state.resourcePermissions[key], - isFetching: false, - hasAttempted: true, - permissions: payload?.result?.[0] ?? [], - }; + .addCase(fetchResourcesPermissions.fulfilled, (state, { meta, payload }) => { + const resources = meta.arg.resources; + for (const r in resources) { + const key = makeResourceKey(resources[r]); + state.resourcePermissions[key] = { + ...state.resourcePermissions[key], + isFetching: false, + hasAttempted: true, + permissions: payload?.result?.[r] ?? [], + }; + } }) - .addCase(fetchResourcePermissions.rejected, (state, { meta, error }) => { - const key = makeResourceKey(meta.arg.resource); + .addCase(fetchResourcesPermissions.rejected, (state, { meta, error }) => { if (error) console.error(error); - const permissionsError = error.message ?? - "An error occurred while fetching permissions for a resource"; - state.resourcePermissions[key] = { - ...state.resourcePermissions[key], - isFetching: false, - hasAttempted: true, - error: permissionsError, - }; + for (const resource of meta.arg.resources) { + const key = makeResourceKey(resource); + + const permissionsError = error.message ?? + "An error occurred while fetching permissions for a resource"; + state.resourcePermissions[key] = { + ...state.resourcePermissions[key], + isFetching: false, + hasAttempted: true, + error: permissionsError, + }; + } }); }, }); From 7022260a06ec92a304cb093079ea3661a683030c Mon Sep 17 00:00:00 2001 From: David Lougheed Date: Fri, 19 Apr 2024 10:20:25 -0400 Subject: [PATCH 2/5] refact: factor out selector into a useAuthState hook --- src/hooks.ts | 18 ++++++++++++------ src/performSignOut.ts | 8 +++----- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/src/hooks.ts b/src/hooks.ts index 35492df..219ba8c 100644 --- a/src/hooks.ts +++ b/src/hooks.ts @@ -5,7 +5,7 @@ import { ThunkAction } from "redux-thunk"; import { useBentoAuthContext } from "./contexts"; import { Resource, makeResourceKey } from "./resources"; -import { fetchResourcesPermissions, refreshTokens, tokenHandoff } from "./redux/authSlice"; +import { AuthSliceState, fetchResourcesPermissions, refreshTokens, tokenHandoff } from "./redux/authSlice"; import { LS_SIGN_IN_POPUP, createAuthURL } from "./performAuth"; import { fetchOpenIdConfigurationIfNecessary } from "./redux/openIdConfigSlice"; import { getIsAuthenticated, logMissingAuthContext, makeAuthorizationHeader } from "./utils"; @@ -16,12 +16,14 @@ const AUTH_RESULT_TYPE = "authResult"; type MessageHandlerFunc = (e: MessageEvent) => void; +export const useAuthState = (): AuthSliceState => useSelector((state: RootState) => state.auth); + export const useIsAuthenticated = () => { - const { idTokenContents } = useSelector((state: RootState) => state.auth); + const { idTokenContents } = useAuthState(); return getIsAuthenticated(idTokenContents); }; -export const useAccessToken = () => useSelector((state: RootState) => state.auth.accessToken); +export const useAccessToken = () => useAuthState().accessToken; export const useAuthorizationHeader = () => { const accessToken = useAccessToken(); @@ -35,15 +37,18 @@ export const useResourcesPermissions = (resources: Resource[], authzUrl: string) const keys = useMemo(() => resources.map((resource) => makeResourceKey(resource)), [resources]); - const { resourcePermissions } = useSelector((state: RootState) => state.auth); + const { resourcePermissions } = useAuthState(); useEffect(() => { - const allFetching = keys.reduce((acc, key) => acc && !!resourcePermissions[key]?.isFetching, true); + const allFetching = keys.reduce((acc, key) => acc || !!resourcePermissions[key]?.isFetching, false); const allHavePermissions = keys.reduce((acc, key) => acc && !!resourcePermissions[key]?.permissions?.length, true); const allAttempted = keys.reduce((acc, key) => acc && !!resourcePermissions[key]?.hasAttempted, true); + // If any permissions are currently fetching, or all requested permissions have already been tried/returned, we + // don't need to dispatch the fetch action: if (!haveAuthorizationService || allFetching || allHavePermissions || allAttempted) return; + dispatch(fetchResourcesPermissions({ resources, authzUrl })); }, [ dispatch, @@ -53,6 +58,7 @@ export const useResourcesPermissions = (resources: Resource[], authzUrl: string) authzUrl, ]); + // Construct an object with resource keys yielding an object containing the permissions on the object return useMemo(() => Object.fromEntries(keys.map((key) => { const { permissions, isFetching, hasAttempted, error } = resourcePermissions[key] ?? {}; return [ @@ -62,7 +68,7 @@ export const useResourcesPermissions = (resources: Resource[], authzUrl: string) isFetching: isFetching ?? false, hasAttempted: hasAttempted ?? false, error: error ?? "", - } + }, ]; })), [keys, resourcePermissions]); }; diff --git a/src/performSignOut.ts b/src/performSignOut.ts index 64a4ebc..360e788 100644 --- a/src/performSignOut.ts +++ b/src/performSignOut.ts @@ -1,18 +1,16 @@ import { useCallback } from "react"; -import { useDispatch, useSelector } from "react-redux"; +import { useDispatch } from "react-redux"; import { useBentoAuthContext } from "./contexts"; -import { useOpenIdConfig } from "./hooks"; +import { useAuthState, useOpenIdConfig } from "./hooks"; import { setLSNotSignedIn } from "./performAuth"; import { signOut } from "./redux/authSlice"; import { logMissingAuthContext } from "./utils"; -import type { RootState } from "./redux/store"; - export const usePerformSignOut = () => { const dispatch = useDispatch(); const { clientId, postSignOutUrl } = useBentoAuthContext(); - const { idToken } = useSelector((state: RootState) => state.auth); + const { idToken } = useAuthState(); const openIdConfig = useOpenIdConfig(); const endSessionEndpoint = openIdConfig?.end_session_endpoint; From 197ca4bcca15a32ae2d60b91d3754f62e67a10e5 Mon Sep 17 00:00:00 2001 From: David Lougheed Date: Fri, 19 Apr 2024 10:21:13 -0400 Subject: [PATCH 3/5] chore: rename FetchPermissionsPayload type BREAKING CHANGE: type name change (pluralization) --- src/redux/authSlice.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/redux/authSlice.ts b/src/redux/authSlice.ts index 491be0c..ba03cae 100644 --- a/src/redux/authSlice.ts +++ b/src/redux/authSlice.ts @@ -102,14 +102,14 @@ export const refreshTokens = createAsyncThunk( } ); -type FetchPermissionPayload = { +type FetchPermissionsPayload = { result: string[][]; }; type FetchPermissionsParams = { resources: Resource[]; authzUrl: string; } -export const fetchResourcesPermissions = createAsyncThunk( +export const fetchResourcesPermissions = createAsyncThunk( "auth/FETCH_RESOURCES_PERMISSIONS", async ({ resources, authzUrl }: FetchPermissionsParams, { getState }) => { const url = `${authzUrl}/policy/permissions`; From 75554b81ef7f0e247cc2e3b794db3a061d1dd302 Mon Sep 17 00:00:00 2001 From: David Lougheed Date: Fri, 19 Apr 2024 10:23:03 -0400 Subject: [PATCH 4/5] chore: fix type hint for recursiveOrderedObject --- src/utils.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/utils.ts b/src/utils.ts index d1cde9d..342c638 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -1,5 +1,4 @@ -import { JWTPayload } from "jose"; -import { Resource } from "./resources"; +import type { JWTPayload } from "jose"; export const LS_OPENID_CONFIG_KEY = "BENTO_OPENID_CONFIG"; @@ -17,14 +16,14 @@ export const getIsAuthenticated = (idTokenContents: JWTPayload | null | undefine export const makeAuthorizationHeader = (token: string | null | undefined): HeadersInit | Record => (token ? { Authorization: `Bearer ${token}` } : {}); -export const recursiveOrderedObject = (x: Resource): unknown => { +export const recursiveOrderedObject = (x: Record): unknown => { if (Array.isArray(x)) { // Don't sort array, but DO make sure each nested object has sorted keys return x.map((y) => recursiveOrderedObject(y)); } else if (typeof x === "object" && x !== null) { return Object.keys(x) .sort() - .reduce((acc, y: string) => { + .reduce>((acc, y: string) => { acc[y] = x[y]; return acc; }, {}); From a86f79d533110af269d123d9d41db3640cc709a4 Mon Sep 17 00:00:00 2001 From: David Lougheed Date: Fri, 19 Apr 2024 14:08:53 -0400 Subject: [PATCH 5/5] perf: use .some/.every for boolean consolidation also swaps conditions for fetching permissions to make sure nothing is fetching that would lead to a race condition. --- src/hooks.ts | 9 ++++----- src/redux/authSlice.ts | 8 ++++---- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/src/hooks.ts b/src/hooks.ts index 219ba8c..4f3239c 100644 --- a/src/hooks.ts +++ b/src/hooks.ts @@ -40,14 +40,13 @@ export const useResourcesPermissions = (resources: Resource[], authzUrl: string) const { resourcePermissions } = useAuthState(); useEffect(() => { - const allFetching = keys.reduce((acc, key) => acc || !!resourcePermissions[key]?.isFetching, false); - const allHavePermissions = - keys.reduce((acc, key) => acc && !!resourcePermissions[key]?.permissions?.length, true); - const allAttempted = keys.reduce((acc, key) => acc && !!resourcePermissions[key]?.hasAttempted, true); + const anyFetching = keys.some((key) => !!resourcePermissions[key]?.isFetching); + const allHavePermissions = keys.every((key) => !!resourcePermissions[key]?.permissions?.length); + const allAttempted = keys.every((key) => !!resourcePermissions[key]?.hasAttempted); // If any permissions are currently fetching, or all requested permissions have already been tried/returned, we // don't need to dispatch the fetch action: - if (!haveAuthorizationService || allFetching || allHavePermissions || allAttempted) return; + if (!haveAuthorizationService || anyFetching || allHavePermissions || allAttempted) return; dispatch(fetchResourcesPermissions({ resources, authzUrl })); }, [ diff --git a/src/redux/authSlice.ts b/src/redux/authSlice.ts index ba03cae..0f607e7 100644 --- a/src/redux/authSlice.ts +++ b/src/redux/authSlice.ts @@ -123,15 +123,15 @@ export const fetchResourcesPermissions = createAsyncThunk { - // allow action to fire if at least one permission is not being fetched right now (and we need it): + // allow action to fire if all requested resource permission-sets are not being fetched right now: const { auth } = getState() as RootState; - return resources.reduce((acc, resource) => { + return resources.every((resource) => { const key = makeResourceKey(resource); const rp = auth.resourcePermissions?.[key]; - return acc || !rp?.isFetching; - }, false); + return !rp?.isFetching; + }); }, } );