safe-react
safe-react copied to clipboard
feat: add permissions management
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
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 ...
- ... ask for camera and microphone permissions on startup
- ... let you test
wallet_getPermissions
,wallet_requestPermissions
andrequestAddressBook
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:
-
Calling
wallet_getPermissions
should return an empty array as you don't have permissions -
Calling
wallet_requestPermissions
entering an unknown method string (any different thanrequestAddressBook
), should raise a Permissions error in the console
![image](https://user-images.githubusercontent.com/1576776/180243111-a03cb859-b0fc-4214-9230-82a243209751.png)
- 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 theuserRestricted
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 usingsdk.requestAddressBook()
method. An exception with Permissions rejected is raised in the console
![image](https://user-images.githubusercontent.com/1576776/180244283-cd80307f-5d8e-43b2-932a-843408d8a57f.png)
-
Calling
wallet_getPermissions
after accepting or rejecting should return an array with the new permission configuration for the corresponding origin -
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
-
Calling
requestAddressBook
(This is what the safe apps developers are going to use as internally callwallet_getPermissions
andwallet_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 -
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 torequestAddressBook
will return the address book or an empty array -
Calling
wallet_requestPermission
should always show the prompt and update the stored user decision -
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
- When custom app is removed the corresponding permissions should be removed
Browser Permissions
-
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.
-
After the initial selection the features should be added to the allow iframe attribute
![image](https://user-images.githubusercontent.com/1576776/180237953-b96b4d49-1fc3-49e1-a65e-a728a8e47346.png)
-
Once selected the modal shouldn't show again unless the permissions in the manifest change
-
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](https://user-images.githubusercontent.com/1576776/180237216-58fad956-8b9d-4c34-84f8-031b04a12704.png)
You can use the Wallet Connect test app included previously
- 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](https://user-images.githubusercontent.com/1576776/180237022-65919da5-7c3c-47af-9b6a-114f6123f1c1.png)
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
-
Go to the Settings management section and change features in each app. Next time you open the app it should reflect the selection
-
When the app change the permissions in the manifest then the initial modal should be showed again
-
When a custom app is removed the corresponding permissions should be removed
Screenshots
Safe Permissions prompt
![image](https://user-images.githubusercontent.com/1576776/181510665-f4e30dd9-5da5-4910-acee-2706db6179fd.png)
Browser permissions slide in Security Feedback modal
![image](https://user-images.githubusercontent.com/1576776/181510734-b2188eac-c554-4f8e-99c7-760858475df7.png)
Safe Permissions management in Settings
![image](https://user-images.githubusercontent.com/1576776/181510805-9f02b9bd-986b-4a1b-ae2a-9d170f977c1c.png)
CLA Assistant Lite All Contributors have signed the CLA.
Designs! => https://www.figma.com/file/RqEZSC19sJRH6rbOFm6kxj/Assistance-pop-ups?node-id=222%3A2298
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
- src/routes/safe/components/Apps/utils.ts Line 200 - Missing return type on function.
Report generated by eslint-plus-action
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 | |
---|---|
Change from base Build 2838498098: | 0.5% |
Covered Lines: | 4765 |
Relevant Lines: | 10334 |
💛 - 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:
Un abrazo ;)
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):
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):
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
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:
What do you think?
A button for remove the entry in the settings is fine for me
@liliiaorlenko, @dasanra ?
I suppose that the typical bin icon could help there. @yagopv check the styling in case there is a long list of permissions there
Yep I would add the bin icon (or Remove Permission link) just in the links row
![image](https://user-images.githubusercontent.com/1576776/186362442-6374c83b-72ad-44f8-a7d4-00e4208eadf9.png)
All bueno after the latest changes, thank you guys!