Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix] Allow undefined url in permission check #1298

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mguihal
Copy link
Contributor

@mguihal mguihal commented Aug 19, 2024

Hello,

Little change here to allow undefined urls to be passed to useCheckPermissions hook and getPermissions method.
React-admin can first render view component without context url fully provided, and so an error was thrown.

Copy link
Contributor

@srosset81 srosset81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It means that the user could potentially see the page (potentially in a "flash") even if he's not connected. That doesn't seem like a good practice. useCheckPermissions is supposed to act like a "blocker".

There are many ways to ensure the URL is present before calling the hook. Where exactly do you have the problem ? React-Admin notably has a useGetRecordId that always returns the URI, even if the resource is not loaded yet.

@mguihal
Copy link
Contributor Author

mguihal commented Aug 20, 2024

Thanks for your comment !
I made further investigations.

I had the problem on Archipelago when I wanted to convert to TS these files:

In that case, the resourceId exists in context, but it is loaded only after resource fetch. So at the first render of the page, it is undefined. If the resource is not authorized, it returns a 403 which made the page to redirect. There is no content flash has react-admin seems to handle that internally until the resource is loaded.

So finally, this call to useCheckPermissions is useless, as it is handled directly by react-admin when fetching the resource.

In that case, resourceId doesn't exist as it is a "container" page. We need to ask dataProvider to recreate the correct middleware url to check permissions. That line is here useful. In the same logic, at the first render, the url is not provided, so it is also undefined (until the dataProvider has fetched the void endpoint to retrieve middleware containers urls). I didn't detect any content flash here, but we can eventually wait for container url defined to display the page.

I also found that, contrary to what I thought, even if the url is undefined when calling useCheckPermissions, react-admin transforms it to a empty object in usePermissions hook (https://github.com/marmelab/react-admin/blob/v4.16.20/packages/ra-core/src/auth/usePermissions.ts#L39) !! So my updates in the authProvider.js file here is useless too 😅 Sorry to have missed that point!

I amend my commit to just update the useCheckPermissions types to allow undefined urls.

@mguihal mguihal force-pushed the fix-UseCheckPermissionsUndefinedUrl branch from e08f99f to 723e7d6 Compare August 20, 2024 21:16
@mguihal mguihal changed the title [fix] Allow undefined url in permission check and return if any [fix] Allow undefined url in permission check Aug 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants