safe-react icon indicating copy to clipboard operation
safe-react copied to clipboard

feat: add permissions management

Open yagopv opened this issue 1 year ago • 5 comments

What it solves

Resolves https://github.com/safe-global/safe-apps-sdk/issues/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

yagopv avatar Jul 12 '22 16:07 yagopv

CLA Assistant Lite All Contributors have signed the CLA.

github-actions[bot] avatar Jul 12 '22 16:07 github-actions[bot]

Designs! => https://www.figma.com/file/RqEZSC19sJRH6rbOFm6kxj/Assistance-pop-ups?node-id=222%3A2298

yagopv avatar Jul 27 '22 13:07 yagopv

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 1 0
Ignored 6 N/A
  • Result: :white_check_mark: success

  • Annotations: 1 total


[warning] @typescript-eslint/explicit-module-boundary-types

Require explicit return and argument types on exported functions' and classes' public class methods


Report generated by eslint-plus-action

github-actions[bot] avatar Jul 28 '22 11:07 github-actions[bot]

Deployment links

:white_circle: Mainnet     :purple_circle: Polygon     :orange_circle: Rinkeby

github-actions[bot] avatar Jul 28 '22 12:07 github-actions[bot]

Pull Request Test Coverage Report for Build 2918354175

  • 86 of 229 (37.55%) changed or added relevant lines in 19 files are covered.
  • 41 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.5%) to 42.074%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/routes/safe/components/Apps/components/SecurityFeedbackModal/SecurityFeedbackAllowedFeatures.tsx 3 4 75.0%
src/routes/safe/components/Apps/hooks/appList/useAppList.ts 4 5 80.0%
src/routes/safe/components/Apps/hooks/useSafeAppFromManifest.ts 13 15 86.67%
src/routes/safe/components/Apps/components/PermissionsPrompt.tsx 2 6 33.33%
src/routes/safe/components/Settings/index.tsx 0 4 0.0%
src/routes/safe/components/Apps/components/SecurityFeedbackModal/index.tsx 6 11 54.55%
src/routes/safe/components/Apps/utils.ts 5 10 50.0%
src/routes/safe/components/Apps/index.tsx 0 6 0.0%
src/routes/safe/components/Apps/hooks/permissions/useBrowserPermissions.ts 11 21 52.38%
src/routes/safe/components/Apps/components/AppFrame.tsx 9 20 45.0%
<!-- Total: 86 229
Files with Coverage Reduction New Missed Lines %
src/routes/safe/components/Apps/utils.ts 1 51.11%
src/routes/safe/components/Apps/hooks/appList/useAppList.ts 6 72.55%
src/routes/safe/components/Apps/hooks/useSecurityFeedbackModal.ts 34 0%
<!-- Total: 41
Totals Coverage Status
Change from base Build 2838498098: 0.5%
Covered Lines: 4765
Relevant Lines: 10334

💛 - Coveralls

coveralls avatar Jul 28 '22 12:07 coveralls

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:

https://user-images.githubusercontent.com/92332710/185937468-e58ca0d7-222a-4cde-bfcd-293c32055dd5.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 avatar Aug 22 '22 15:08 JagoFigueroa

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

JagoFigueroa avatar Aug 23 '22 08:08 JagoFigueroa

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

yagopv avatar Aug 23 '22 08:08 yagopv

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?

JagoFigueroa avatar Aug 23 '22 10:08 JagoFigueroa

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

@liliiaorlenko, @dasanra ?

yagopv avatar Aug 23 '22 14:08 yagopv

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

dasanra avatar Aug 24 '22 07:08 dasanra

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

image

yagopv avatar Aug 24 '22 07:08 yagopv

All bueno after the latest changes, thank you guys!

JagoFigueroa avatar Aug 24 '22 11:08 JagoFigueroa