From 3cb30c32e16da9fc17d11a50ad23a637f151f35a Mon Sep 17 00:00:00 2001 From: George Mamadashvili Date: Tue, 9 Jul 2024 14:37:36 +0400 Subject: [PATCH 1/5] Core Data: Introduce entity-aware permission selector --- docs/reference-guides/data/data-core.md | 20 ++++++ packages/core-data/README.md | 20 ++++++ packages/core-data/src/resolvers.js | 84 +++++++++++++++++++++++++ packages/core-data/src/selectors.ts | 28 +++++++++ 4 files changed, 152 insertions(+) diff --git a/docs/reference-guides/data/data-core.md b/docs/reference-guides/data/data-core.md index ba77f065584cfe..bd35fb87beb9ce 100644 --- a/docs/reference-guides/data/data-core.md +++ b/docs/reference-guides/data/data-core.md @@ -550,6 +550,26 @@ _Returns_ - `boolean`: True if the REST request was completed. False otherwise. +### hasPermission + +Returns whether the current user can perform the given action on the entity record. + +Calling this may trigger an OPTIONS request to the REST API via the `hasPermission()` resolver. + + + +_Parameters_ + +- _state_ `State`: Data state. +- _action_ `string`: Action to check. One of: 'create', 'read', 'update', 'delete'. +- _kind_ `string`: Entity kind. +- _name_ `string`: Entity name +- _key_ `EntityRecordKey`: Optional record's key. + +_Returns_ + +- `boolean | undefined`: Whether or not the user can perform the action, or `undefined` if the OPTIONS request is still being made. + ### hasRedo Returns true if there is a next edit from the current undo offset for the entity records edits history, and false otherwise. diff --git a/packages/core-data/README.md b/packages/core-data/README.md index 694f780dafb99d..50ce5957eb2633 100644 --- a/packages/core-data/README.md +++ b/packages/core-data/README.md @@ -871,6 +871,26 @@ _Returns_ - `boolean`: True if the REST request was completed. False otherwise. +### hasPermission + +Returns whether the current user can perform the given action on the entity record. + +Calling this may trigger an OPTIONS request to the REST API via the `hasPermission()` resolver. + + + +_Parameters_ + +- _state_ `State`: Data state. +- _action_ `string`: Action to check. One of: 'create', 'read', 'update', 'delete'. +- _kind_ `string`: Entity kind. +- _name_ `string`: Entity name +- _key_ `EntityRecordKey`: Optional record's key. + +_Returns_ + +- `boolean | undefined`: Whether or not the user can perform the action, or `undefined` if the OPTIONS request is still being made. + ### hasRedo Returns true if there is a next edit from the current undo offset for the entity records edits history, and false otherwise. diff --git a/packages/core-data/src/resolvers.js b/packages/core-data/src/resolvers.js index 5eb6fd8642c75e..d5b4a5db764ee2 100644 --- a/packages/core-data/src/resolvers.js +++ b/packages/core-data/src/resolvers.js @@ -424,6 +424,90 @@ export const canUser = } ); }; +/** + * Checks whether the current user can perform the given action on the entity record. + * + * @param {string} action Action to check. One of: 'create', 'read', 'update', 'delete'. + * @param {string} kind Entity kind. + * @param {string} name Entity name + * @param {number|string} key Optional record's key. + */ +export const hasPermission = + ( action, kind, name, key ) => + async ( { dispatch, registry } ) => { + const configs = await dispatch( getOrLoadEntitiesConfig( kind, name ) ); + const entityConfig = configs.find( + ( config ) => config.name === name && config.kind === kind + ); + if ( ! entityConfig ) { + return; + } + + const { hasStartedResolution } = registry.select( STORE_NAME ); + const supportedActions = [ 'create', 'read', 'update', 'delete' ]; + + if ( ! supportedActions.includes( action ) ) { + throw new Error( `'${ action }' is not a valid action.` ); + } + + // Prevent resolving the same resource twice. + for ( const relatedAction of supportedActions ) { + if ( relatedAction === action ) { + continue; + } + const isAlreadyResolving = hasStartedResolution( 'hasPermission', [ + relatedAction, + kind, + name, + key, + ] ); + if ( isAlreadyResolving ) { + return; + } + } + + let response; + try { + response = await apiFetch( { + path: entityConfig.baseURL + ( key ? '/' + key : '' ), + method: 'OPTIONS', + parse: false, + } ); + } catch ( error ) { + // Do nothing if our OPTIONS request comes back with an API error (4xx or + // 5xx). The previously determined isAllowed value will remain in the store. + return; + } + + // Optional chaining operator is used here because the API requests don't + // return the expected result in the native version. Instead, API requests + // only return the result, without including response properties like the headers. + const allowHeader = response.headers?.get( 'allow' ); + const allowedMethods = allowHeader?.allow || allowHeader || ''; + + const permissions = {}; + const methods = { + create: 'POST', + read: 'GET', + update: 'PUT', + delete: 'DELETE', + }; + for ( const [ actionName, methodName ] of Object.entries( methods ) ) { + permissions[ actionName ] = allowedMethods.includes( methodName ); + } + + registry.batch( () => { + for ( const supportedAction of supportedActions ) { + dispatch.receiveUserPermission( + [ supportedAction, kind, name, key ] + .filter( Boolean ) + .join( '/' ), + permissions[ supportedAction ] + ); + } + } ); + }; + /** * Checks whether the current user can perform the given action on the given * REST resource. diff --git a/packages/core-data/src/selectors.ts b/packages/core-data/src/selectors.ts index 425a537cad7363..e110ed4e63bcd2 100644 --- a/packages/core-data/src/selectors.ts +++ b/packages/core-data/src/selectors.ts @@ -1182,6 +1182,34 @@ export function canUserEditEntityRecord( return canUser( state, 'update', resource, recordId ); } +/** + * Returns whether the current user can perform the given action on the entity record. + * + * Calling this may trigger an OPTIONS request to the REST API via the + * `hasPermission()` resolver. + * + * https://developer.wordpress.org/rest-api/reference/ + * + * @param state Data state. + * @param action Action to check. One of: 'create', 'read', 'update', 'delete'. + * @param kind Entity kind. + * @param name Entity name + * @param key Optional record's key. + * + * @return Whether or not the user can perform the action, + * or `undefined` if the OPTIONS request is still being made. + */ +export function hasPermission( + state: State, + action: string, + kind: string, + name: string, + key?: EntityRecordKey +): boolean | undefined { + const cacheKey = [ action, kind, name, key ].filter( Boolean ).join( '/' ); + return state.userPermissions[ cacheKey ]; +} + /** * Returns the latest autosaves for the post. * From a83454c8a567daa5ea893c001dec5998d36d671b Mon Sep 17 00:00:00 2001 From: George Mamadashvili Date: Tue, 9 Jul 2024 15:36:14 +0400 Subject: [PATCH 2/5] Add unit tests --- packages/core-data/src/test/resolvers.js | 273 +++++++++++++++++++++++ packages/core-data/src/test/selectors.js | 34 +++ 2 files changed, 307 insertions(+) diff --git a/packages/core-data/src/test/resolvers.js b/packages/core-data/src/test/resolvers.js index 4e900615df3868..e0fea0983938a3 100644 --- a/packages/core-data/src/test/resolvers.js +++ b/packages/core-data/src/test/resolvers.js @@ -15,6 +15,7 @@ import { canUser, getAutosaves, getCurrentUser, + hasPermission, } from '../resolvers'; describe( 'getEntityRecord', () => { @@ -495,6 +496,278 @@ describe( 'canUser', () => { } ); } ); +describe( 'hasPermission', () => { + const ENTITIES = [ + { + name: 'media', + kind: 'root', + baseURL: '/wp/v2/media', + baseURLParams: { context: 'edit' }, + }, + { + name: 'wp_block', + kind: 'postType', + baseURL: '/wp/v2/blocks', + baseURLParams: { context: 'edit' }, + }, + ]; + + let dispatch, registry; + beforeEach( async () => { + registry = { + select: jest.fn( () => ( { + hasStartedResolution: () => false, + } ) ), + batch: ( callback ) => callback(), + }; + dispatch = Object.assign( jest.fn(), { + receiveUserPermission: jest.fn(), + } ); + + // Provide entities + dispatch.mockReturnValue( ENTITIES ); + triggerFetch.mockReset(); + } ); + + it( 'does nothing when there is an API error', async () => { + triggerFetch.mockImplementation( () => + Promise.reject( { status: 404 } ) + ); + + await hasPermission( + 'create', + 'root', + 'media' + )( { dispatch, registry } ); + + expect( triggerFetch ).toHaveBeenCalledWith( { + path: '/wp/v2/media', + method: 'OPTIONS', + parse: false, + } ); + + expect( dispatch.receiveUserPermission ).not.toHaveBeenCalled(); + } ); + + it( 'receives false when the user is not allowed to perform an action', async () => { + triggerFetch.mockImplementation( () => ( { + headers: new Map( [ [ 'allow', 'GET' ] ] ), + } ) ); + + await hasPermission( + 'create', + 'root', + 'media' + )( { + dispatch, + registry, + } ); + + expect( triggerFetch ).toHaveBeenCalledWith( { + path: '/wp/v2/media', + method: 'OPTIONS', + parse: false, + } ); + + expect( dispatch.receiveUserPermission ).toHaveBeenCalledWith( + 'create/root/media', + false + ); + } ); + + it( 'receives true when the user is allowed to perform an action', async () => { + triggerFetch.mockImplementation( () => ( { + headers: new Map( [ [ 'allow', 'POST, GET, PUT, DELETE' ] ] ), + } ) ); + + await hasPermission( + 'create', + 'root', + 'media' + )( { dispatch, registry } ); + + expect( triggerFetch ).toHaveBeenCalledWith( { + path: '/wp/v2/media', + method: 'OPTIONS', + parse: false, + } ); + + expect( dispatch.receiveUserPermission ).toHaveBeenCalledWith( + 'create/root/media', + true + ); + } ); + + it( 'receives true when the user is allowed to perform an action on a specific resource', async () => { + triggerFetch.mockImplementation( () => ( { + headers: new Map( [ [ 'allow', 'POST, GET, PUT, DELETE' ] ] ), + } ) ); + + await hasPermission( + 'create', + 'postType', + 'wp_block', + 123 + )( { dispatch, registry } ); + + expect( triggerFetch ).toHaveBeenCalledWith( { + path: '/wp/v2/blocks/123', + method: 'OPTIONS', + parse: false, + } ); + + expect( dispatch.receiveUserPermission ).toHaveBeenCalledWith( + 'create/postType/wp_block/123', + true + ); + } ); + + it( 'runs apiFetch only once per resource', async () => { + registry = { + ...registry, + select: () => ( { + hasStartedResolution: ( _, [ action ] ) => action === 'read', + } ), + }; + + triggerFetch.mockImplementation( () => ( { + headers: new Map( [ [ 'allow', 'POST, GET' ] ] ), + } ) ); + + await hasPermission( + 'create', + 'postType', + 'wp_block' + )( { dispatch, registry } ); + await hasPermission( + 'read', + 'postType', + 'wp_block' + )( { dispatch, registry } ); + + expect( triggerFetch ).toHaveBeenCalledTimes( 1 ); + + expect( dispatch.receiveUserPermission ).toHaveBeenCalledWith( + 'create/postType/wp_block', + true + ); + expect( dispatch.receiveUserPermission ).toHaveBeenCalledWith( + 'read/postType/wp_block', + true + ); + } ); + + it( 'retrieves all permissions even when ID is not given', async () => { + registry = { + ...registry, + select: () => ( { + hasStartedResolution: ( _, [ action ] ) => action === 'read', + } ), + }; + + triggerFetch.mockImplementation( () => ( { + headers: new Map( [ [ 'allow', 'POST, GET' ] ] ), + } ) ); + + await hasPermission( + 'create', + 'postType', + 'wp_block' + )( { dispatch, registry } ); + await hasPermission( + 'read', + 'postType', + 'wp_block' + )( { dispatch, registry } ); + await hasPermission( + 'update', + 'postType', + 'wp_block' + )( { dispatch, registry } ); + await hasPermission( + 'delete', + 'postType', + 'wp_block' + )( { dispatch, registry } ); + + expect( triggerFetch ).toHaveBeenCalledTimes( 1 ); + + expect( dispatch.receiveUserPermission ).toHaveBeenCalledWith( + 'create/postType/wp_block', + true + ); + expect( dispatch.receiveUserPermission ).toHaveBeenCalledWith( + 'read/postType/wp_block', + true + ); + expect( dispatch.receiveUserPermission ).toHaveBeenCalledWith( + 'update/postType/wp_block', + false + ); + expect( dispatch.receiveUserPermission ).toHaveBeenCalledWith( + 'delete/postType/wp_block', + false + ); + } ); + + it( 'runs apiFetch only once per resource ID', async () => { + registry = { + ...registry, + select: () => ( { + hasStartedResolution: ( _, [ action ] ) => action === 'create', + } ), + }; + + triggerFetch.mockImplementation( () => ( { + headers: new Map( [ [ 'allow', 'POST, GET, PUT, DELETE' ] ] ), + } ) ); + + await hasPermission( + 'create', + 'postType', + 'wp_block', + 123 + )( { dispatch, registry } ); + await hasPermission( + 'read', + 'postType', + 'blocks', + 123 + )( { dispatch, registry } ); + await hasPermission( + 'update', + 'postType', + 'wp_block', + 123 + )( { dispatch, registry } ); + await hasPermission( + 'delete', + 'postType', + 'wp_block', + 123 + )( { dispatch, registry } ); + + expect( triggerFetch ).toHaveBeenCalledTimes( 1 ); + + expect( dispatch.receiveUserPermission ).toHaveBeenCalledWith( + 'create/postType/wp_block/123', + true + ); + expect( dispatch.receiveUserPermission ).toHaveBeenCalledWith( + 'read/postType/wp_block/123', + true + ); + expect( dispatch.receiveUserPermission ).toHaveBeenCalledWith( + 'update/postType/wp_block/123', + true + ); + expect( dispatch.receiveUserPermission ).toHaveBeenCalledWith( + 'delete/postType/wp_block/123', + true + ); + } ); +} ); + describe( 'getAutosaves', () => { const SUCCESSFUL_RESPONSE = [ { diff --git a/packages/core-data/src/test/selectors.js b/packages/core-data/src/test/selectors.js index 43c84a3e978917..fcfc80e65594d3 100644 --- a/packages/core-data/src/test/selectors.js +++ b/packages/core-data/src/test/selectors.js @@ -24,6 +24,7 @@ import { getCurrentUser, getRevisions, getRevision, + hasPermission, } from '../selectors'; // getEntityRecord and __experimentalGetEntityRecordNoResolver selectors share the same tests. describe.each( [ @@ -766,6 +767,39 @@ describe( 'canUserEditEntityRecord', () => { } ); } ); +describe( 'hasPermission', () => { + it( 'returns undefined by default', () => { + const state = deepFreeze( { + userPermissions: {}, + } ); + expect( hasPermission( state, 'create', 'root', 'media' ) ).toBe( + undefined + ); + } ); + + it( 'returns whether an action can be performed', () => { + const state = deepFreeze( { + userPermissions: { + 'create/root/media': false, + }, + } ); + expect( hasPermission( state, 'create', 'root', 'media' ) ).toBe( + false + ); + } ); + + it( 'returns whether an action can be performed for a given resource', () => { + const state = deepFreeze( { + userPermissions: { + 'update/root/media/123': true, + }, + } ); + expect( hasPermission( state, 'update', 'root', 'media', 123 ) ).toBe( + true + ); + } ); +} ); + describe( 'getAutosave', () => { const testAutosave = { author: 1, From a876add405f0b4a0720869fae89b2308fd3e130f Mon Sep 17 00:00:00 2001 From: George Mamadashvili Date: Tue, 9 Jul 2024 15:38:23 +0400 Subject: [PATCH 3/5] Remove repeated mocks from 'canUser' unit tests --- packages/core-data/src/test/resolvers.js | 33 +++--------------------- 1 file changed, 4 insertions(+), 29 deletions(-) diff --git a/packages/core-data/src/test/resolvers.js b/packages/core-data/src/test/resolvers.js index e0fea0983938a3..63d5375be4aecf 100644 --- a/packages/core-data/src/test/resolvers.js +++ b/packages/core-data/src/test/resolvers.js @@ -284,7 +284,7 @@ describe( 'getEmbedPreview', () => { } ); describe( 'canUser', () => { - let registry; + let dispatch, registry; beforeEach( async () => { registry = { select: jest.fn( () => ( { @@ -292,14 +292,13 @@ describe( 'canUser', () => { } ) ), batch: ( callback ) => callback(), }; + dispatch = Object.assign( jest.fn(), { + receiveUserPermission: jest.fn(), + } ); triggerFetch.mockReset(); } ); it( 'does nothing when there is an API error', async () => { - const dispatch = Object.assign( jest.fn(), { - receiveUserPermission: jest.fn(), - } ); - triggerFetch.mockImplementation( () => Promise.reject( { status: 404 } ) ); @@ -316,10 +315,6 @@ describe( 'canUser', () => { } ); it( 'receives false when the user is not allowed to perform an action', async () => { - const dispatch = Object.assign( jest.fn(), { - receiveUserPermission: jest.fn(), - } ); - triggerFetch.mockImplementation( () => ( { headers: new Map( [ [ 'allow', 'GET' ] ] ), } ) ); @@ -339,10 +334,6 @@ describe( 'canUser', () => { } ); it( 'receives true when the user is allowed to perform an action', async () => { - const dispatch = Object.assign( jest.fn(), { - receiveUserPermission: jest.fn(), - } ); - triggerFetch.mockImplementation( () => ( { headers: new Map( [ [ 'allow', 'POST, GET, PUT, DELETE' ] ] ), } ) ); @@ -362,10 +353,6 @@ describe( 'canUser', () => { } ); it( 'receives true when the user is allowed to perform an action on a specific resource', async () => { - const dispatch = Object.assign( jest.fn(), { - receiveUserPermission: jest.fn(), - } ); - triggerFetch.mockImplementation( () => ( { headers: new Map( [ [ 'allow', 'POST, GET, PUT, DELETE' ] ] ), } ) ); @@ -385,10 +372,6 @@ describe( 'canUser', () => { } ); it( 'runs apiFetch only once per resource', async () => { - const dispatch = Object.assign( jest.fn(), { - receiveUserPermission: jest.fn(), - } ); - registry = { ...registry, select: () => ( { @@ -416,10 +399,6 @@ describe( 'canUser', () => { } ); it( 'retrieves all permissions even when ID is not given', async () => { - const dispatch = Object.assign( jest.fn(), { - receiveUserPermission: jest.fn(), - } ); - registry = { ...registry, select: () => ( { @@ -455,10 +434,6 @@ describe( 'canUser', () => { } ); it( 'runs apiFetch only once per resource ID', async () => { - const dispatch = Object.assign( jest.fn(), { - receiveUserPermission: jest.fn(), - } ); - registry = { ...registry, select: () => ( { From 1227cc5ea3294e799e72a25ceed670978ed28a60 Mon Sep 17 00:00:00 2001 From: George Mamadashvili Date: Tue, 9 Jul 2024 15:51:29 +0400 Subject: [PATCH 4/5] Replace 'canUserEditEntityRecord' selector usages --- packages/block-library/src/post-title/edit.js | 3 ++- packages/block-library/src/utils/hooks.js | 2 +- packages/editor/src/bindings/post-meta.js | 3 ++- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/block-library/src/post-title/edit.js b/packages/block-library/src/post-title/edit.js index 5b11ec3a9452ae..9bf9ce7abd334b 100644 --- a/packages/block-library/src/post-title/edit.js +++ b/packages/block-library/src/post-title/edit.js @@ -42,7 +42,8 @@ export default function PostTitleEdit( { if ( isDescendentOfQueryLoop ) { return false; } - return select( coreStore ).canUserEditEntityRecord( + return select( coreStore ).hasPermission( + 'update', 'postType', postType, postId diff --git a/packages/block-library/src/utils/hooks.js b/packages/block-library/src/utils/hooks.js index 43733d7f49a046..8023db52e57c0e 100644 --- a/packages/block-library/src/utils/hooks.js +++ b/packages/block-library/src/utils/hooks.js @@ -18,7 +18,7 @@ import { useViewportMatch } from '@wordpress/compose'; export function useCanEditEntity( kind, name, recordId ) { return useSelect( ( select ) => - select( coreStore ).canUserEditEntityRecord( kind, name, recordId ), + select( coreStore ).hasPermission( 'update', kind, name, recordId ), [ kind, name, recordId ] ); } diff --git a/packages/editor/src/bindings/post-meta.js b/packages/editor/src/bindings/post-meta.js index 0b46e58542a359..40a34ff8a8bd4f 100644 --- a/packages/editor/src/bindings/post-meta.js +++ b/packages/editor/src/bindings/post-meta.js @@ -59,7 +59,8 @@ export default { } // Check that the user has the capability to edit post meta. - const canUserEdit = select( coreDataStore ).canUserEditEntityRecord( + const canUserEdit = select( coreDataStore ).hasPermission( + 'update', 'postType', context?.postType, context?.postId From 8a418a0bb0cc15869390bc4f0bfd8d1c9a4d0570 Mon Sep 17 00:00:00 2001 From: George Mamadashvili Date: Tue, 9 Jul 2024 15:55:45 +0400 Subject: [PATCH 5/5] Make 'canUserEditEntityRecord' forwarded selector/resolver --- packages/core-data/src/resolvers.js | 11 +---- packages/core-data/src/selectors.ts | 8 +--- packages/core-data/src/test/selectors.js | 56 ------------------------ 3 files changed, 2 insertions(+), 73 deletions(-) diff --git a/packages/core-data/src/resolvers.js b/packages/core-data/src/resolvers.js index d5b4a5db764ee2..6d4f9aae5b9dfa 100644 --- a/packages/core-data/src/resolvers.js +++ b/packages/core-data/src/resolvers.js @@ -519,16 +519,7 @@ export const hasPermission = export const canUserEditEntityRecord = ( kind, name, recordId ) => async ( { dispatch } ) => { - const configs = await dispatch( getOrLoadEntitiesConfig( kind, name ) ); - const entityConfig = configs.find( - ( config ) => config.name === name && config.kind === kind - ); - if ( ! entityConfig ) { - return; - } - - const resource = entityConfig.__unstable_rest_base; - await dispatch( canUser( 'update', resource, recordId ) ); + await dispatch( hasPermission( 'update', kind, name, recordId ) ); }; /** diff --git a/packages/core-data/src/selectors.ts b/packages/core-data/src/selectors.ts index e110ed4e63bcd2..8c20323fea63df 100644 --- a/packages/core-data/src/selectors.ts +++ b/packages/core-data/src/selectors.ts @@ -1173,13 +1173,7 @@ export function canUserEditEntityRecord( name: string, recordId: EntityRecordKey ): boolean | undefined { - const entityConfig = getEntityConfig( state, kind, name ); - if ( ! entityConfig ) { - return false; - } - const resource = entityConfig.__unstable_rest_base; - - return canUser( state, 'update', resource, recordId ); + return hasPermission( state, 'update', kind, name, recordId ); } /** diff --git a/packages/core-data/src/test/selectors.js b/packages/core-data/src/test/selectors.js index fcfc80e65594d3..86bebead042847 100644 --- a/packages/core-data/src/test/selectors.js +++ b/packages/core-data/src/test/selectors.js @@ -18,7 +18,6 @@ import { getEmbedPreview, isPreviewEmbedFallback, canUser, - canUserEditEntityRecord, getAutosave, getAutosaves, getCurrentUser, @@ -712,61 +711,6 @@ describe( 'canUser', () => { } ); } ); -describe( 'canUserEditEntityRecord', () => { - it( 'returns false by default', () => { - const state = deepFreeze( { - userPermissions: {}, - entities: { records: {} }, - } ); - expect( canUserEditEntityRecord( state, 'postType', 'post' ) ).toBe( - false - ); - } ); - - it( 'returns whether the user can edit', () => { - const state = deepFreeze( { - userPermissions: { - 'create/posts': false, - 'update/posts/1': true, - }, - entities: { - config: [ - { - kind: 'postType', - name: 'post', - __unstable_rest_base: 'posts', - }, - ], - records: { - root: { - postType: { - queriedData: { - items: { - default: { - post: { - slug: 'post', - __unstable: 'posts', - }, - }, - }, - itemIsComplete: { - default: { - post: true, - }, - }, - queries: {}, - }, - }, - }, - }, - }, - } ); - expect( - canUserEditEntityRecord( state, 'postType', 'post', '1' ) - ).toBe( true ); - } ); -} ); - describe( 'hasPermission', () => { it( 'returns undefined by default', () => { const state = deepFreeze( {