-
-
Notifications
You must be signed in to change notification settings - Fork 9
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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.
Thanks for your comment ! 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. |
e08f99f
to
723e7d6
Compare
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.