Skip to content
This repository has been archived by the owner on Nov 10, 2023. It is now read-only.

feat: add permissions management #4019

Merged
merged 62 commits into from
Aug 25, 2022
Merged

feat: add permissions management #4019

merged 62 commits into from
Aug 25, 2022

Conversation

yagopv
Copy link
Member

@yagopv yagopv commented Jul 12, 2022

What it solves

Resolves safe-global/safe-apps-sdk#318

How this PR fixes it

  • Adding permissions management support for the safe apps sdk EIP-2255 implementation released on 7.6.0
  • Adding permission support for allowing Browser features in Safe Apps

Designs
RFC

How to test it

For testing purposes we can use these 3 custom applications allowing you to test both Safe Permissions and Browser Permissions

https://safe-test-gmj5jjjzh-yagopv.vercel.app
https://safe-test-abc8dg6ee-yagopv.vercel.app
https://safe-test-3gnbptoms-yagopv.vercel.app

This applications will ...

  1. ... ask for camera and microphone permissions on startup
  2. ... let you test wallet_getPermissions, wallet_requestPermissions and requestAddressBook using the RPC tab

This Wallet Connect version test application:

https://safe-react-apps-yagopv.vercel.app

... is a wallet-connect version asking for camera permissions because we added the safe_apps_permissions property in manifest.json

🧑‍💻 Start testing with empty localStorage:

Safe Permissions

Test in this order:

  1. Calling wallet_getPermissions should return an empty array as you don't have permissions

  2. Calling wallet_requestPermissions entering an unknown method string (any different than requestAddressBook), should raise a Permissions error in the console

image

  1. Calling wallet_requestPermissions entering a known method string (requestAddressBook) should show a popup asking for approve or reject the permissions.
    3.1) when approve the permission will be saved to localStorage under SAFE__BROWSER_PERMISSIONS
    3.1) when reject the permission will be saved to localStorage under SAFE__BROWSER_PERMISSIONS but with a caveats array with the userRestricted type in place (true value). This means the user explicitly decide to not allow the access to the permission and is important for not asking again when using sdk.requestAddressBook() method. An exception with Permissions rejected is raised in the console

image

  1. Calling wallet_getPermissions after accepting or rejecting should return an array with the new permission configuration for the corresponding origin

  2. Calling requestAddressBook
    5.1) when you accepted then should return the addressBook
    5.2) when you rejected then should return an empty array

🧑‍💻 Remove (only) SAFE__BROWSER_PERMISSIONS entry from localStorage

  1. Calling requestAddressBook (This is what the safe apps developers are going to use as internally call wallet_getPermissions and wallet_requestPermissions) should show the prompt asking for permissions.
    6.1) when you accepted then should return the addressBook. Subsequents requests will return the address book without prompting again
    6.2) when you rejected then and a exception is raised in the console. Subsequents will return an empty address book

  2. Go to settings and select/unselect the options per domain. When you return to the app the selection should be reflected. The application is going to match the selection but not prompting again unless you call explicitly wallet_requestPermission. So calls to requestAddressBook will return the address book or an empty array

  3. Calling wallet_requestPermission should always show the prompt and update the stored user decision

  4. Calling window.parent.postMessage with the correct data
    9.1) when the user doesn't have permissions the application cannot get the addressBook
    9.2) when the user has permissions then should return the address book

window.parent.postMessage(
  {
    env: { sdkVersion: '7.4.1' },
    id: '5b63a0e43b',
    method: 'requestAddressBook',
    params: undefined,
  },
  '*'
);

You can use the "Regular postMessage" button in the Test apps

  1. When custom app is removed the corresponding permissions should be removed

Browser Permissions

  1. When the app initially loads a new permissions slide in Security Feedback Modal should be added for selecting the browser features the user wants to allow. You can keep all selected or make a choice. When you end with the Modal the choice should be reflected in localStorage under SAFE__BROWSER_PERMISSIONS.

  2. After the initial selection the features should be added to the allow iframe attribute

image

  1. Once selected the modal shouldn't show again unless the permissions in the manifest change

  2. When the user choose to unselect some of the features then an error is raised in the console when the application try to access them as some of the features will be missing in the allow attribute on the iframe

image

You can use the Wallet Connect test app included previously

  1. When you select the features required (camera in wallet connect for example) then a prompt should be showed when trying to access the camera:

image

You can allow or block. This is showed once per top level domain so if several apps are requesting for camera access only once should be showed

  1. Go to the Settings management section and change features in each app. Next time you open the app it should reflect the selection

  2. When the app change the permissions in the manifest then the initial modal should be showed again

  3. When a custom app is removed the corresponding permissions should be removed

Screenshots

Safe Permissions prompt

image

Browser permissions slide in Security Feedback modal

image

Safe Permissions management in Settings

image

@github-actions
Copy link

CLA Assistant Lite All Contributors have signed the CLA.

@yagopv yagopv changed the title feat: add permissions management for eip-2255 feat: add permissions management Jul 20, 2022
@yagopv yagopv requested a review from mmv08 August 16, 2022 12:19
@yagopv yagopv requested a review from mmv08 August 17, 2022 08:10
Copy link
Member

@mmv08 mmv08 left a comment

Choose a reason for hiding this comment

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

Good job senor 🎉 I suggest to also ask someone else for a review, it's a big PR and I could easily miss something

safeAppsUtils.canLoadAppImage = jest.fn().mockResolvedValue(true)
// @ts-ignore
axios.get.mockImplementation((url: string) => {
console.log(url)
Copy link
Member

Choose a reason for hiding this comment

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

console.log sneaked in

Copy link
Member Author

Choose a reason for hiding this comment

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

done!. Yep, i will wait to Dani's to review it

@JagoFigueroa
Copy link

Hola señores. I come with an issue and an improvement that can be tackled in another ticket :)

  • Permissions are not being removed for me when removing a custom app:
preferences.mp4
  • About the improvement, I think it would be neat to show a custom pop up for the cases were an action fails due to non browser permissions like in this case:
    Screenshot 2022-08-22 at 10 38 20

Un abrazo ;)

@JagoFigueroa
Copy link

Providing a bit more info for the issue mentioned above, If I provide address book safe permissions for one of the testing apps and then choose to remove the app from custom apps it will remain in the safe apps permissions section as well (let me know if this magic is the expected behaviour):
Screenshot 2022-08-23 at 10 14 50

@yagopv
Copy link
Member Author

yagopv commented Aug 23, 2022

Providing a bit more info for the issue mentioned above, If I provide address book safe permissions for one of the testing apps and then choose to remove the app from custom apps it will remain in the safe apps permissions section as well (let me know if this magic is the expected behaviour): Screenshot 2022-08-23 at 10 14 50

Nah the issue is about not removing the latest application never for browser or safe permissions. If we have more than one is fine but the latest is not removed. On it

@JagoFigueroa
Copy link

Hola! I wonder if we should do some magic for custom apps that are not added to the list and opened, for example, via the share app link.

If I open this app directly I am always going to be asked for permissions (which makes sense as the app wouldn't work otherwise) but as the app is not added as a custom app, permissions will be created in the local storage but I will not be able to remove them.

As a workaround I think we could add a remove option per app in this screen:
Screenshot 2022-08-23 at 12 19 58

What do you think?

@yagopv
Copy link
Member Author

yagopv commented Aug 23, 2022

A button for remove the entry in the settings is fine for me

@liliiaorlenko, @dasanra ?

@dasanra
Copy link
Collaborator

dasanra commented Aug 24, 2022

I suppose that the typical bin icon could help there. @yagopv check the styling in case there is a long list of permissions there

@yagopv
Copy link
Member Author

yagopv commented Aug 24, 2022

Yep I would add the bin icon (or Remove Permission link) just in the links row

image

@JagoFigueroa
Copy link

All bueno after the latest changes, thank you guys!

@yagopv yagopv merged commit 43a658e into dev Aug 25, 2022
@yagopv yagopv deleted the feat/add-permissions-management branch August 25, 2022 13:40
@github-actions github-actions bot locked and limited conversation to collaborators Aug 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[EPIC] Create permission system to access SDK methods
5 participants