From 45a74fc16e96394dd731d1a8521d0924bd5d5ca6 Mon Sep 17 00:00:00 2001 From: Espen Hovlandsdal Date: Tue, 15 Oct 2024 19:00:34 +0200 Subject: [PATCH] fix: prevent excessive requests to access endpoint (#7597) --- .../_legacy/grants/documentPairPermissions.ts | 10 +- .../grants/documentValuePermissions.ts | 107 +++++++++++++----- .../src/core/store/_legacy/grants/types.ts | 2 +- .../studio/screens/RequestAccessScreen.tsx | 35 +++--- .../useRoleRequestsStatus.tsx | 103 +++++++++-------- .../document/documentPanel/DocumentPanel.tsx | 11 +- ...r.tsx => InsufficientPermissionBanner.tsx} | 9 +- .../document/documentPanel/banners/index.ts | 2 +- 8 files changed, 165 insertions(+), 114 deletions(-) rename packages/sanity/src/structure/panes/document/documentPanel/banners/{PermissionCheckBanner.tsx => InsufficientPermissionBanner.tsx} (94%) diff --git a/packages/sanity/src/core/store/_legacy/grants/documentPairPermissions.ts b/packages/sanity/src/core/store/_legacy/grants/documentPairPermissions.ts index 7f632c5d489..429c420632e 100644 --- a/packages/sanity/src/core/store/_legacy/grants/documentPairPermissions.ts +++ b/packages/sanity/src/core/store/_legacy/grants/documentPairPermissions.ts @@ -42,7 +42,7 @@ function getPairPermissions({ draft, published, liveEdit, -}: PairPermissionsOptions): Array<[string, Observable] | null> { +}: PairPermissionsOptions): Array<[string, Observable]> { // this was introduced because we ran into a bug where a user with publish // access was marked as not allowed to duplicate a document unless it had a // draft variant. this would happen in non-live edit cases where the document @@ -220,9 +220,9 @@ export function getDocumentPairPermissions({ draft, published, liveEdit, - }).map(([label, observable]: any) => - observable.pipe( - map(({granted, reason}: any) => ({ + }).map(([label, permissionCheck$]) => + permissionCheck$.pipe( + map(({granted, reason}) => ({ granted, reason: granted ? '' : `not allowed to ${label}: ${reason}`, label, @@ -234,7 +234,7 @@ export function getDocumentPairPermissions({ if (!pairPermissions.length) return of({granted: true, reason: ''}) return combineLatest(pairPermissions).pipe( - map((permissionResults: any[]) => { + map((permissionResults) => { const granted = permissionResults.every((permissionResult) => permissionResult.granted) const reason = granted ? '' diff --git a/packages/sanity/src/core/store/_legacy/grants/documentValuePermissions.ts b/packages/sanity/src/core/store/_legacy/grants/documentValuePermissions.ts index 8c285db1fa7..a0635af5cf8 100644 --- a/packages/sanity/src/core/store/_legacy/grants/documentValuePermissions.ts +++ b/packages/sanity/src/core/store/_legacy/grants/documentValuePermissions.ts @@ -1,6 +1,8 @@ -import {type Observable} from 'rxjs' +import {isEqual} from 'lodash' +import {useEffect, useReducer, useRef} from 'react' +import {distinctUntilChanged, type Observable, type Subscription} from 'rxjs' -import {createHookFromObservableFactory, type PartialExcept} from '../../../util' +import {type LoadingTuple, type PartialExcept} from '../../../util' import {useGrantsStore} from '../datastores' import {type DocumentValuePermission, type GrantsStore, type PermissionCheckResult} from './types' @@ -11,23 +13,6 @@ export interface DocumentValuePermissionsOptions { permission: DocumentValuePermission } -/** - * The observable version of `useDocumentValuePermissions` - * - * @see useDocumentValuePermissions - * - * @internal - */ -export function getDocumentValuePermissions({ - grantsStore, - document, - permission, -}: DocumentValuePermissionsOptions): Observable { - const {checkDocumentPermission} = grantsStore - - return checkDocumentPermission(permission, document) -} - /** * Gets permissions based on the value of the document passed into the hook * (stateless). @@ -49,23 +34,83 @@ export function getDocumentValuePermissions({ * * @internal */ -export const useDocumentValuePermissionsFromHookFactory = createHookFromObservableFactory( - getDocumentValuePermissions, -) +export function getDocumentValuePermissions({ + grantsStore, + document, + permission, +}: DocumentValuePermissionsOptions): Observable { + const {checkDocumentPermission} = grantsStore + + return checkDocumentPermission(permission, document) +} + +const INITIAL_STATE: LoadingTuple = [undefined, true] + +function stateReducer( + prev: LoadingTuple, + action: + | {type: 'loading'} + | {type: 'value'; value: PermissionCheckResult} + | {type: 'error'; error: unknown}, +): LoadingTuple { + const [prevResult, prevIsLoading] = prev + switch (action.type) { + // Keep the old value around while loading a new one. Prevents "jittering" UIs, and permissions + // usually don't change _that_ rapidly (this hook runs on every single document change). + case 'loading': + return [prevResult, true] + case 'value': + // Signal "no update" to React if we've got the same value as before by returning old state + return !prevIsLoading && isEqual(action.value, prevResult) ? prev : [action.value, false] + case 'error': + throw action.error + default: + throw new Error(`Invalid action type: ${action}`) + } +} /** @internal */ export function useDocumentValuePermissions({ document, permission, - ...rest -}: PartialExcept): ReturnType< - typeof useDocumentValuePermissionsFromHookFactory + grantsStore: specifiedGrantsStore, +}: PartialExcept): LoadingTuple< + PermissionCheckResult | undefined > { - const grantsStore = useGrantsStore() + const defaultGrantsStore = useGrantsStore() + const grantsStore = specifiedGrantsStore || defaultGrantsStore + + const [state, dispatch] = useReducer(stateReducer, INITIAL_STATE) + const subscriptionRef = useRef(null) + + useEffect(() => { + dispatch({type: 'loading'}) + + // Unsubscribe from any previous subscription + if (subscriptionRef.current) { + subscriptionRef.current.unsubscribe() + } + + const permissions$ = getDocumentValuePermissions({ + grantsStore, + document, + permission, + }) + + subscriptionRef.current = permissions$ + .pipe(distinctUntilChanged((prev, next) => isEqual(prev, next))) + .subscribe({ + next: (value) => dispatch({type: 'value', value}), + error: (error) => dispatch({type: 'error', error}), + }) + + return () => { + if (subscriptionRef.current) { + subscriptionRef.current.unsubscribe() + subscriptionRef.current = null + } + } + }, [grantsStore, document, permission]) - return useDocumentValuePermissionsFromHookFactory({ - grantsStore: rest.grantsStore || grantsStore, - document, - permission, - }) + return state } diff --git a/packages/sanity/src/core/store/_legacy/grants/types.ts b/packages/sanity/src/core/store/_legacy/grants/types.ts index 098a29579ae..4e80efba79a 100644 --- a/packages/sanity/src/core/store/_legacy/grants/types.ts +++ b/packages/sanity/src/core/store/_legacy/grants/types.ts @@ -27,7 +27,7 @@ export interface GrantsStore { /** * Returns an observable of `PermissionCheckResult` * - * This API is returns an observable (vs a promise) so the consumer can reac + * This API is returns an observable (vs a promise) so the consumer can react * to incoming changes to the user permissions (e.g. for changing _debug_ * roles). * diff --git a/packages/sanity/src/core/studio/screens/RequestAccessScreen.tsx b/packages/sanity/src/core/studio/screens/RequestAccessScreen.tsx index 24d08692a88..b78a49f6eae 100644 --- a/packages/sanity/src/core/studio/screens/RequestAccessScreen.tsx +++ b/packages/sanity/src/core/studio/screens/RequestAccessScreen.tsx @@ -2,6 +2,7 @@ import {Box, Card, Flex, Stack, Text, TextInput, useToast} from '@sanity/ui' import {addWeeks, isAfter, isBefore} from 'date-fns' import {useCallback, useEffect, useState} from 'react' +import {finalize} from 'rxjs' import { type CurrentUser, getProviderTitle, @@ -73,16 +74,19 @@ export function RequestAccessScreen() { } }, [activeWorkspace]) - // Check if user has a pending - // access request for this project + // Check if user has a pending access request for this project useEffect(() => { - if (!client || !projectId) return - client + if (!client || !projectId) return () => {} + const request$ = client.observable .request({ - url: `/access/requests/me`, + url: '/access/requests/me', + tag: 'request-access', }) - .then((requests) => { - if (requests && requests?.length) { + .pipe(finalize(() => setLoading(false))) + .subscribe({ + next: (requests) => { + if (!requests || !requests.length) return + const projectRequests = requests.filter((request) => request.resourceId === projectId) const declinedRequest = projectRequests.find((request) => request.status === 'declined') if ( @@ -111,15 +115,16 @@ export function RequestAccessScreen() { if (oldPendingRequest) { setExpiredHasPendingRequest(true) } - } - }) - .catch((err) => { - console.error(err) - setError(true) - }) - .finally(() => { - setLoading(false) + }, + error: (err) => { + console.error(err) + setError(true) + }, }) + + return () => { + request$.unsubscribe() + } }, [client, projectId]) const handleSubmitRequest = useCallback(() => { diff --git a/packages/sanity/src/structure/components/requestPermissionDialog/useRoleRequestsStatus.tsx b/packages/sanity/src/structure/components/requestPermissionDialog/useRoleRequestsStatus.tsx index 6c43d6d887b..27a6bb6a5a0 100644 --- a/packages/sanity/src/structure/components/requestPermissionDialog/useRoleRequestsStatus.tsx +++ b/packages/sanity/src/structure/components/requestPermissionDialog/useRoleRequestsStatus.tsx @@ -1,9 +1,9 @@ import {addWeeks, isAfter, isBefore} from 'date-fns' import {useMemo} from 'react' import {useObservable} from 'react-rx' -import {from, of} from 'rxjs' +import {of} from 'rxjs' import {catchError, map, startWith} from 'rxjs/operators' -import {useClient, useProjectId} from 'sanity' +import {type SanityClient, useClient, useProjectId} from 'sanity' /** @internal */ export interface AccessRequest { @@ -20,70 +20,75 @@ export interface AccessRequest { note: string } +const LOADING_STATE = {loading: true, error: false, status: undefined} +const EMPTY_STATE = {loading: false, error: false, status: 'none'} +const DECLINED_STATE = {loading: false, error: false, status: 'declined'} +const PENDING_STATE = {loading: false, error: false, status: 'pending'} +const EXPIRED_STATE = {loading: false, error: false, status: 'expired'} + /** @internal */ export const useRoleRequestsStatus = () => { const client = useClient({apiVersion: '2024-07-01'}) const projectId = useProjectId() - const checkRoleRequests = useMemo(() => { + const checkRoleRequests$ = useMemo(() => { if (!client || !projectId) { - return of({loading: false, error: false, status: 'none'}) + return of(EMPTY_STATE) } - return from( - client.request({ - url: `/access/requests/me`, - }), - ).pipe( + return checkRoleRequests(client, projectId) + }, [client, projectId]) + + const {loading, error, status} = useObservable(checkRoleRequests$, LOADING_STATE) + return {data: status, loading, error} +} + +function checkRoleRequests(client: SanityClient, projectId: string) { + return client.observable + .request({ + url: '/access/requests/me', + tag: 'use-role-requests-status', + }) + .pipe( map((requests) => { - if (requests && requests.length) { - // Filter requests for the specific project and where type is 'role' - const projectRequests = requests.filter( - (request) => request.resourceId === projectId && request.type === 'role', - ) + if (!requests || requests.length === 0) { + return EMPTY_STATE + } - const declinedRequest = projectRequests.find((request) => request.status === 'declined') - if ( - declinedRequest && - isAfter(addWeeks(new Date(declinedRequest.createdAt), 2), new Date()) - ) { - return {loading: false, error: false, status: 'declined'} - } + // Filter requests for the specific project and where type is 'role' + const projectRequests = requests.filter( + (request) => request.resourceId === projectId && request.type === 'role', + ) - const pendingRequest = projectRequests.find( - (request) => - request.status === 'pending' && - isAfter(addWeeks(new Date(request.createdAt), 2), new Date()), - ) - if (pendingRequest) { - return {loading: false, error: false, status: 'pending'} - } + const declinedRequest = projectRequests.find((request) => request.status === 'declined') + if ( + declinedRequest && + isAfter(addWeeks(new Date(declinedRequest.createdAt), 2), new Date()) + ) { + return DECLINED_STATE + } - const oldPendingRequest = projectRequests.find( - (request) => - request.status === 'pending' && - isBefore(addWeeks(new Date(request.createdAt), 2), new Date()), - ) - if (oldPendingRequest) { - return {loading: false, error: false, status: 'expired'} - } + const pendingRequest = projectRequests.find( + (request) => + request.status === 'pending' && + isAfter(addWeeks(new Date(request.createdAt), 2), new Date()), + ) + if (pendingRequest) { + return PENDING_STATE } - return {loading: false, error: false, status: 'none'} + + const oldPendingRequest = projectRequests.find( + (request) => + request.status === 'pending' && + isBefore(addWeeks(new Date(request.createdAt), 2), new Date()), + ) + + return oldPendingRequest ? EXPIRED_STATE : EMPTY_STATE }), catchError((err) => { console.error('Failed to fetch access requests', err) return of({loading: false, error: true, status: undefined}) }), - startWith({loading: true, error: false, status: undefined}), // Start with loading state + startWith(LOADING_STATE), // Start with loading state ) - }, [client, projectId]) - - // Use useObservable to subscribe to the checkRoleRequests observable - const {loading, error, status} = useObservable(checkRoleRequests, { - loading: true, - error: false, - status: undefined, - }) - - return {data: status, loading, error} } diff --git a/packages/sanity/src/structure/panes/document/documentPanel/DocumentPanel.tsx b/packages/sanity/src/structure/panes/document/documentPanel/DocumentPanel.tsx index 51f0f2b77bc..fca8f6bc25d 100644 --- a/packages/sanity/src/structure/panes/document/documentPanel/DocumentPanel.tsx +++ b/packages/sanity/src/structure/panes/document/documentPanel/DocumentPanel.tsx @@ -12,7 +12,7 @@ import {useDocumentPane} from '../useDocumentPane' import { DeletedDocumentBanner, DeprecatedDocumentTypeBanner, - PermissionCheckBanner, + InsufficientPermissionBanner, ReferenceChangedBanner, } from './banners' import {DraftLiveEditBanner} from './banners/DraftLiveEditBanner' @@ -51,7 +51,6 @@ export const DocumentPanel = function DocumentPanel(props: DocumentPanelProps) { activeViewId, displayed, documentId, - documentType, editState, inspector, value, @@ -63,7 +62,6 @@ export const DocumentPanel = function DocumentPanel(props: DocumentPanelProps) { isDeleting, isDeleted, timelineStore, - onChange, } = useDocumentPane() const {collapsed: layoutCollapsed} = usePaneLayout() const {collapsed} = usePane() @@ -164,10 +162,9 @@ export const DocumentPanel = function DocumentPanel(props: DocumentPanelProps) { {activeView.type === 'form' && !isPermissionsLoading && ready && ( <> - + {!permissions?.granted && ( + + )} {!isDeleting && isDeleted && ( )} diff --git a/packages/sanity/src/structure/panes/document/documentPanel/banners/PermissionCheckBanner.tsx b/packages/sanity/src/structure/panes/document/documentPanel/banners/InsufficientPermissionBanner.tsx similarity index 94% rename from packages/sanity/src/structure/panes/document/documentPanel/banners/PermissionCheckBanner.tsx rename to packages/sanity/src/structure/panes/document/documentPanel/banners/InsufficientPermissionBanner.tsx index cb46edf7555..6fa00fc737c 100644 --- a/packages/sanity/src/structure/panes/document/documentPanel/banners/PermissionCheckBanner.tsx +++ b/packages/sanity/src/structure/panes/document/documentPanel/banners/InsufficientPermissionBanner.tsx @@ -12,12 +12,13 @@ import {AskToEditDialogOpened} from '../../../../components/requestPermissionDia import {structureLocaleNamespace} from '../../../../i18n' import {Banner} from './Banner' -interface PermissionCheckBannerProps { - granted: boolean +interface InsufficientPermissionBannerProps { requiredPermission: 'update' | 'create' } -export function PermissionCheckBanner({granted, requiredPermission}: PermissionCheckBannerProps) { +export function InsufficientPermissionBanner({ + requiredPermission, +}: InsufficientPermissionBannerProps) { const currentUser = useCurrentUser() const { @@ -38,8 +39,6 @@ export function PermissionCheckBanner({granted, requiredPermission}: PermissionC const {t} = useTranslation(structureLocaleNamespace) const telemetry = useTelemetry() - if (granted) return null - const roleTitles = currentUserRoles.map((role) => role.title) const roles = listFormat .formatToParts(roleTitles) diff --git a/packages/sanity/src/structure/panes/document/documentPanel/banners/index.ts b/packages/sanity/src/structure/panes/document/documentPanel/banners/index.ts index 10f706b9937..0716965d7de 100644 --- a/packages/sanity/src/structure/panes/document/documentPanel/banners/index.ts +++ b/packages/sanity/src/structure/panes/document/documentPanel/banners/index.ts @@ -1,4 +1,4 @@ export * from './DeletedDocumentBanner' export * from './DeprecatedDocumentTypeBanner' -export * from './PermissionCheckBanner' +export * from './InsufficientPermissionBanner' export * from './ReferenceChangedBanner'