From c8edfde5dced73cca12e4008ee7d45ae9de386e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Zieli=C5=84ski?= Date: Mon, 22 Aug 2022 14:22:19 +0200 Subject: [PATCH 1/6] Don't run duplicate requests in canUser resolver --- packages/core-data/src/resolvers.js | 101 +++++++++++++++-------- packages/core-data/src/test/resolvers.js | 54 +++++++++++- 2 files changed, 118 insertions(+), 37 deletions(-) diff --git a/packages/core-data/src/resolvers.js b/packages/core-data/src/resolvers.js index 5b7c42d661e77e..9680a0f3ced164 100644 --- a/packages/core-data/src/resolvers.js +++ b/packages/core-data/src/resolvers.js @@ -273,52 +273,87 @@ export const getEmbedPreview = * Checks whether the current user can perform the given action on the given * REST resource. * - * @param {string} action Action to check. One of: 'create', 'read', 'update', - * 'delete'. - * @param {string} resource REST resource to check, e.g. 'media' or 'posts'. - * @param {?string} id ID of the rest resource to check. + * @param {string} requestedAction Action to check. One of: 'create', 'read', 'update', + * 'delete'. + * @param {string} resource REST resource to check, e.g. 'media' or 'posts'. + * @param {?string} id ID of the rest resource to check. */ export const canUser = - ( action, resource, id ) => - async ( { dispatch } ) => { - const methods = { - create: 'POST', - read: 'GET', - update: 'PUT', - delete: 'DELETE', - }; - - const method = methods[ action ]; - if ( ! method ) { - throw new Error( `'${ action }' is not a valid action.` ); + ( requestedAction, resource, id ) => + async ( { dispatch, registry } ) => { + const { hasStartedResolution } = registry.select( STORE_NAME ); + + let resourcePath; + let retrievedActions; + if ( id ) { + retrievedActions = [ 'create', 'read', 'update', 'delete' ]; + resourcePath = `${ resource }/${ id }`; + + // Prevent resolving the same resource twice. + for ( const relatedAction of retrievedActions ) { + if ( relatedAction === requestedAction ) { + continue; + } + const isAlreadyResolving = hasStartedResolution( 'canUser', [ + relatedAction, + resource, + id, + ] ); + if ( isAlreadyResolving ) { + return; + } + } + } else { + retrievedActions = [ 'create' ]; + resourcePath = resource; } - const path = id - ? `/wp/v2/${ resource }/${ id }` - : `/wp/v2/${ resource }`; + if ( ! retrievedActions.includes( requestedAction ) ) { + throw new Error( `'${ requestedAction }' is not a valid action.` ); + } - let response; + let permissions; try { - response = await apiFetch( { - path, - method: 'OPTIONS', - parse: false, - } ); + permissions = await checkAllowedActions( resourcePath ); } 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 key = [ action, resource, id ].filter( Boolean ).join( '/' ); - const isAllowed = - allowHeader?.includes?.( method ) || allowHeader?.allow === method; - dispatch.receiveUserPermission( key, isAllowed ); + for ( const action of retrievedActions ) { + dispatch.receiveUserPermission( + `${ action }/${ resourcePath }`, + permissions[ action ] + ); + } + }; + +const checkAllowedActions = async ( path ) => { + const methods = { + create: 'POST', + read: 'GET', + update: 'PUT', + delete: 'DELETE', }; + const response = await apiFetch( { + path: `/wp/v2/${ path }`, + method: 'OPTIONS', + parse: false, + } ); + + // 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 = {}; + for ( const [ actionName, methodName ] of Object.entries( methods ) ) { + permissions[ actionName ] = allowedMethods.includes( methodName ); + } + return permissions; +}; /** * Checks whether the current user can perform the given action on the given diff --git a/packages/core-data/src/test/resolvers.js b/packages/core-data/src/test/resolvers.js index 7909ac83dfa628..cc6ac335d40e54 100644 --- a/packages/core-data/src/test/resolvers.js +++ b/packages/core-data/src/test/resolvers.js @@ -291,7 +291,13 @@ describe( 'getEmbedPreview', () => { } ); describe( 'canUser', () => { + let registry; beforeEach( async () => { + registry = { + select: jest.fn( () => ( { + hasStartedResolution: () => false, + } ) ), + }; triggerFetch.mockReset(); } ); @@ -304,7 +310,7 @@ describe( 'canUser', () => { Promise.reject( { status: 404 } ) ); - await canUser( 'create', 'media' )( { dispatch } ); + await canUser( 'create', 'media' )( { dispatch, registry } ); expect( triggerFetch ).toHaveBeenCalledWith( { path: '/wp/v2/media', @@ -324,7 +330,7 @@ describe( 'canUser', () => { headers: new Map( [ [ 'allow', 'GET' ] ] ), } ) ); - await canUser( 'create', 'media' )( { dispatch } ); + await canUser( 'create', 'media' )( { dispatch, registry } ); expect( triggerFetch ).toHaveBeenCalledWith( { path: '/wp/v2/media', @@ -347,7 +353,7 @@ describe( 'canUser', () => { headers: new Map( [ [ 'allow', 'POST, GET, PUT, DELETE' ] ] ), } ) ); - await canUser( 'create', 'media' )( { dispatch } ); + await canUser( 'create', 'media' )( { dispatch, registry } ); expect( triggerFetch ).toHaveBeenCalledWith( { path: '/wp/v2/media', @@ -370,7 +376,7 @@ describe( 'canUser', () => { headers: new Map( [ [ 'allow', 'POST, GET, PUT, DELETE' ] ] ), } ) ); - await canUser( 'create', 'blocks', 123 )( { dispatch } ); + await canUser( 'create', 'blocks', 123 )( { dispatch, registry } ); expect( triggerFetch ).toHaveBeenCalledWith( { path: '/wp/v2/blocks/123', @@ -383,6 +389,46 @@ describe( 'canUser', () => { true ); } ); + + it( 'runs apiFetch only once per resource', async () => { + const dispatch = Object.assign( jest.fn(), { + receiveUserPermission: jest.fn(), + } ); + + registry = { + select: () => ( { + hasStartedResolution: ( _, [ action ] ) => action === 'create', + } ), + }; + + triggerFetch.mockImplementation( () => ( { + headers: new Map( [ [ 'allow', 'POST, GET, PUT, DELETE' ] ] ), + } ) ); + + await canUser( 'create', 'blocks', 123 )( { dispatch, registry } ); + await canUser( 'read', 'blocks', 123 )( { dispatch, registry } ); + await canUser( 'update', 'blocks', 123 )( { dispatch, registry } ); + await canUser( 'delete', 'blocks', 123 )( { dispatch, registry } ); + + expect( triggerFetch ).toHaveBeenCalledTimes( 1 ); + + expect( dispatch.receiveUserPermission ).toHaveBeenCalledWith( + 'create/blocks/123', + true + ); + expect( dispatch.receiveUserPermission ).toHaveBeenCalledWith( + 'read/blocks/123', + true + ); + expect( dispatch.receiveUserPermission ).toHaveBeenCalledWith( + 'update/blocks/123', + true + ); + expect( dispatch.receiveUserPermission ).toHaveBeenCalledWith( + 'delete/blocks/123', + true + ); + } ); } ); describe( 'getAutosaves', () => { From 8bc814770d7dceac8f58008af6f6e0812521840e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Zieli=C5=84ski?= Date: Mon, 22 Aug 2022 14:30:54 +0200 Subject: [PATCH 2/6] Remove the checkAllowedActions function --- packages/core-data/src/resolvers.js | 51 +++++++++++++---------------- 1 file changed, 23 insertions(+), 28 deletions(-) diff --git a/packages/core-data/src/resolvers.js b/packages/core-data/src/resolvers.js index 9680a0f3ced164..11825206f3040e 100644 --- a/packages/core-data/src/resolvers.js +++ b/packages/core-data/src/resolvers.js @@ -312,15 +312,36 @@ export const canUser = throw new Error( `'${ requestedAction }' is not a valid action.` ); } - let permissions; + let response; try { - permissions = await checkAllowedActions( resourcePath ); + response = await apiFetch( { + path: `/wp/v2/${ resourcePath }`, + 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 ); + } + for ( const action of retrievedActions ) { dispatch.receiveUserPermission( `${ action }/${ resourcePath }`, @@ -329,32 +350,6 @@ export const canUser = } }; -const checkAllowedActions = async ( path ) => { - const methods = { - create: 'POST', - read: 'GET', - update: 'PUT', - delete: 'DELETE', - }; - const response = await apiFetch( { - path: `/wp/v2/${ path }`, - method: 'OPTIONS', - parse: false, - } ); - - // 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 = {}; - for ( const [ actionName, methodName ] of Object.entries( methods ) ) { - permissions[ actionName ] = allowedMethods.includes( methodName ); - } - return permissions; -}; - /** * Checks whether the current user can perform the given action on the given * REST resource. From cedf8696c976f9d6254d8e5c4284ab8b5adc260d Mon Sep 17 00:00:00 2001 From: Adam Zielinski Date: Mon, 22 Aug 2022 15:14:45 +0200 Subject: [PATCH 3/6] Update packages/core-data/src/resolvers.js Co-authored-by: George Mamadashvili --- packages/core-data/src/resolvers.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core-data/src/resolvers.js b/packages/core-data/src/resolvers.js index 11825206f3040e..89bd08fd848dd7 100644 --- a/packages/core-data/src/resolvers.js +++ b/packages/core-data/src/resolvers.js @@ -304,7 +304,7 @@ export const canUser = } } } else { - retrievedActions = [ 'create' ]; + retrievedActions = [ 'create', 'read' ]; resourcePath = resource; } From ad5ad899edbbe5e8309c2f9c4a5dba412110ee1d Mon Sep 17 00:00:00 2001 From: George Mamadashvili Date: Wed, 24 Aug 2022 11:30:14 +0400 Subject: [PATCH 4/6] Check resolution status for resources without an ID --- packages/core-data/src/resolvers.js | 39 +++++++++++------------- packages/core-data/src/test/resolvers.js | 30 ++++++++++++++++++ 2 files changed, 47 insertions(+), 22 deletions(-) diff --git a/packages/core-data/src/resolvers.js b/packages/core-data/src/resolvers.js index 89bd08fd848dd7..08b8e62c45467d 100644 --- a/packages/core-data/src/resolvers.js +++ b/packages/core-data/src/resolvers.js @@ -283,29 +283,24 @@ export const canUser = async ( { dispatch, registry } ) => { const { hasStartedResolution } = registry.select( STORE_NAME ); - let resourcePath; - let retrievedActions; - if ( id ) { - retrievedActions = [ 'create', 'read', 'update', 'delete' ]; - resourcePath = `${ resource }/${ id }`; - - // Prevent resolving the same resource twice. - for ( const relatedAction of retrievedActions ) { - if ( relatedAction === requestedAction ) { - continue; - } - const isAlreadyResolving = hasStartedResolution( 'canUser', [ - relatedAction, - resource, - id, - ] ); - if ( isAlreadyResolving ) { - return; - } + const resourcePath = id ? `${ resource }/${ id }` : resource; + const retrievedActions = id + ? [ 'create', 'read', 'update', 'delete' ] + : [ 'create', 'read' ]; + + // Prevent resolving the same resource twice. + for ( const relatedAction of retrievedActions ) { + if ( relatedAction === requestedAction ) { + continue; + } + const isAlreadyResolving = hasStartedResolution( 'canUser', [ + relatedAction, + resource, + id, + ] ); + if ( isAlreadyResolving ) { + return; } - } else { - retrievedActions = [ 'create', 'read' ]; - resourcePath = resource; } if ( ! retrievedActions.includes( requestedAction ) ) { diff --git a/packages/core-data/src/test/resolvers.js b/packages/core-data/src/test/resolvers.js index cc6ac335d40e54..0db7d84ac58c22 100644 --- a/packages/core-data/src/test/resolvers.js +++ b/packages/core-data/src/test/resolvers.js @@ -395,6 +395,36 @@ describe( 'canUser', () => { receiveUserPermission: jest.fn(), } ); + registry = { + select: () => ( { + hasStartedResolution: ( _, [ action ] ) => action === 'read', + } ), + }; + + triggerFetch.mockImplementation( () => ( { + headers: new Map( [ [ 'allow', 'POST, GET' ] ] ), + } ) ); + + await canUser( 'create', 'blocks' )( { dispatch, registry } ); + await canUser( 'read', 'blocks' )( { dispatch, registry } ); + + expect( triggerFetch ).toHaveBeenCalledTimes( 1 ); + + expect( dispatch.receiveUserPermission ).toHaveBeenCalledWith( + 'create/blocks', + true + ); + expect( dispatch.receiveUserPermission ).toHaveBeenCalledWith( + 'read/blocks', + true + ); + } ); + + it( 'runs apiFetch only once per resource ID', async () => { + const dispatch = Object.assign( jest.fn(), { + receiveUserPermission: jest.fn(), + } ); + registry = { select: () => ( { hasStartedResolution: ( _, [ action ] ) => action === 'create', From 80f376cccf91718982fc1fee5b69a855643f754e Mon Sep 17 00:00:00 2001 From: George Mamadashvili Date: Wed, 24 Aug 2022 12:22:47 +0400 Subject: [PATCH 5/6] Throw an error early --- packages/core-data/src/resolvers.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/core-data/src/resolvers.js b/packages/core-data/src/resolvers.js index 08b8e62c45467d..aebd2daee7712a 100644 --- a/packages/core-data/src/resolvers.js +++ b/packages/core-data/src/resolvers.js @@ -288,6 +288,10 @@ export const canUser = ? [ 'create', 'read', 'update', 'delete' ] : [ 'create', 'read' ]; + if ( ! retrievedActions.includes( requestedAction ) ) { + throw new Error( `'${ requestedAction }' is not a valid action.` ); + } + // Prevent resolving the same resource twice. for ( const relatedAction of retrievedActions ) { if ( relatedAction === requestedAction ) { @@ -303,10 +307,6 @@ export const canUser = } } - if ( ! retrievedActions.includes( requestedAction ) ) { - throw new Error( `'${ requestedAction }' is not a valid action.` ); - } - let response; try { response = await apiFetch( { From f54ba14f95ed86749fa92d3cd1be5852c2caf315 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Zieli=C5=84ski?= Date: Wed, 24 Aug 2022 14:08:02 +0200 Subject: [PATCH 6/6] Receive update and delete permissions even if no ID was passed --- packages/core-data/src/resolvers.js | 4 +-- packages/core-data/src/test/resolvers.js | 38 ++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 3 deletions(-) diff --git a/packages/core-data/src/resolvers.js b/packages/core-data/src/resolvers.js index aebd2daee7712a..b33bb42e653379 100644 --- a/packages/core-data/src/resolvers.js +++ b/packages/core-data/src/resolvers.js @@ -284,9 +284,7 @@ export const canUser = const { hasStartedResolution } = registry.select( STORE_NAME ); const resourcePath = id ? `${ resource }/${ id }` : resource; - const retrievedActions = id - ? [ 'create', 'read', 'update', 'delete' ] - : [ 'create', 'read' ]; + const retrievedActions = [ 'create', 'read', 'update', 'delete' ]; if ( ! retrievedActions.includes( requestedAction ) ) { throw new Error( `'${ requestedAction }' is not a valid action.` ); diff --git a/packages/core-data/src/test/resolvers.js b/packages/core-data/src/test/resolvers.js index 0db7d84ac58c22..24269b3c1cb9b7 100644 --- a/packages/core-data/src/test/resolvers.js +++ b/packages/core-data/src/test/resolvers.js @@ -420,6 +420,44 @@ describe( 'canUser', () => { ); } ); + it( 'retrieves all permissions even when ID is not given', async () => { + const dispatch = Object.assign( jest.fn(), { + receiveUserPermission: jest.fn(), + } ); + + registry = { + select: () => ( { + hasStartedResolution: ( _, [ action ] ) => action === 'read', + } ), + }; + + triggerFetch.mockImplementation( () => ( { + headers: new Map( [ [ 'allow', 'POST, GET' ] ] ), + } ) ); + + await canUser( 'create', 'blocks' )( { dispatch, registry } ); + await canUser( 'read', 'blocks' )( { dispatch, registry } ); + await canUser( 'update', 'blocks' )( { dispatch, registry } ); + await canUser( 'delete', 'blocks' )( { dispatch, registry } ); + + expect( dispatch.receiveUserPermission ).toHaveBeenCalledWith( + 'create/blocks', + true + ); + expect( dispatch.receiveUserPermission ).toHaveBeenCalledWith( + 'read/blocks', + true + ); + expect( dispatch.receiveUserPermission ).toHaveBeenCalledWith( + 'update/blocks', + false + ); + expect( dispatch.receiveUserPermission ).toHaveBeenCalledWith( + 'delete/blocks', + false + ); + } ); + it( 'runs apiFetch only once per resource ID', async () => { const dispatch = Object.assign( jest.fn(), { receiveUserPermission: jest.fn(),