semapps icon indicating copy to clipboard operation
semapps copied to clipboard

Fix: Handle list permissions correctly

Open mguihal opened this issue 2 years ago • 4 comments

Hello,

I made this PR to fix some oversights concerning permissions for List pages.

This PR is a dependency for this Archipelago PR https://github.com/assemblee-virtuelle/archipelago/pull/154. I exposed the initial needs and a bit more context why I did that there.

Context

In Archipelago (and Semapps), it is possible to remove rights to a container, including "acl:read" right. Doing this, list page can still be accessible. Work here is to prevent that.

Auth-provider

Two modifications have been made in this package.

The first is to hide "List" button displayed in ShowActionsWithPermissions and EditActionsWithPermissions components in case the container doesn't have Read permissions for the current user.

The second is to update the hook useCheckPermissions to remove the call to getIdentity. This call returns undefined if the user is not logged, ignoring permissions. (The bug was not seen before because useCheckPermissions was only used on logged pages).

All updated files were converted to Typescript to continue packages typings. Permissions response are now fully typed (I hope I didn't make a mistake undestanding webacl mecanism).

Semantic-data-provider

One modification have been made in this package.

The hook useCreateContainer is taking a single resource parameter, returning the corresponding URI. This hook is not adapted when we have to retrieve dynamically multiple containers URI in a single component. Instead, I recreated this hook changing its response with a function taking the resource. Doing so (and it's a good thing for all hooks), we can call it multiple time in a component. I didn't want to break things, that's why I duplicated the useCreateContainer to useCreateContainerUri (naming is also more relevant as it returns an URI), and marking the useCreateContainer as deprecated. Other possibility would have to update directly useCreateContainer, but apps using Semapps would have to update to continue working.

Other updated files are Typescript conversion to make this new hook have all its data typed correctly.

Feel free to tell me I did something wrong! 🙏

mguihal avatar Nov 30 '23 00:11 mguihal

Thank you @mguihal ! It's a precious evolution allowing new use cases !

GuillaumeAV avatar Nov 30 '23 07:11 GuillaumeAV

@mguihal I don't understand why changes to Semantic-data-provider are linked to permissions

simonLouvet avatar Dec 04 '23 10:12 simonLouvet

I understand changes to Semantic-data-provider with https://github.com/assemblee-virtuelle/archipelago/pull/154

simonLouvet avatar Dec 04 '23 10:12 simonLouvet

It's difficult to review because of TS conversion, since Git considers the JS file has been deleted and the TS file is new. Maybe it would be a good idea to do all the TS conversions in one PR, so that it doesn't clutter the review of other PRs ? Or otherwise do the TS conversion in a single commit, and the PR-related changes in other commits.

I'm sorry I rushed into committing without splitting correctly the updates! I have rewrited the commit history to split them into 4 commits:

  • 47b504c9c3249474e24d0f39d7ed98b26d383ec7 Add useCreateContainerUri in semantic-data-provider + docs

  • 2152b9e26197e8864bd865a6cec1e5293691ef2d Convert semantic-data-provider updated files to Typescript

  • f1aac2e9f5550d4c7506c115b21821888bea981c Handle correctly list permissions in auth-provider

  • 0351c125f510024148127f43dc4e5baefc10c056 Convert auth-provider updated files to Typescript

I hope updates I made will be clearer like that.

Anyway it looks good, but you should document the new useCreateContainerUri hook on the website (see how it is done for useCreateContainer).

Indeed! I have added hook documentation in the first commit

mguihal avatar Dec 07 '23 23:12 mguihal

Thanks @srosset81 for your last commit on semantic-data-provider (https://github.com/assemblee-virtuelle/semapps/commit/23688d0b2a11ff0cbf6d15749a40cf8905fa4300), I rebased the converted file to match your fix, and I updated typings to make containers attributes in dataServer config optional.

I think I will merge this PR tomorrow to avoid further rebases (and because I forgot this one was still not merged ^^) if no obstructions, it will improve typing a lot in data-provider :) And I'll also publish a new version of frontend packages with my two merged PR. (Do you confirm doing yarn version and then yarn publish will make all the necessaries or do I miss a point? Maybe we can document how to release a new version somewhere)

mguihal avatar Jul 22 '24 21:07 mguihal