dfinity / wg-identity-authentication

Repository of the Identity and Wallet Standards Working Group
https://wiki.internetcomputer.org/wiki/Identity_%26_Authentication
Apache License 2.0
26 stars 8 forks source link

ICRC-25: Specifying user interactions #131

Closed frederikrothenberger closed 2 months ago

frederikrothenberger commented 5 months ago

So far, ICRC-25 and it's extensions have in some cases left open when to interact with the user. It was brought up by @sea-snake in the last working group session that this makes it confusing for relying parties to provide a consistent UX for the signer interactions.

This issue should be used to discuss that point.


My personal opinion on the topic:

I think it would be good to specify for each signer method how they interact with the user:

Method Requires approval Notes
icrc25_request_permissions yes It's up to the signer how to describe the requested permissions, but they need to show a screen that the user has to confirm.

Note: There might exist a use-case for institutional grade signers, where a user that has access to the signer might not have the authority to grant certain permissions. If the set of approvable permissions is empty, the approval screen can be skipped and a response can be sent immediately.

TLDR: Requires approval if the set of possible new permissions is larger than the currently approved one.
icrc25_granted_permissions No This is simply querying session state.
icrc25_revoke_permissions No Reducing permission does not require approval. It is up to the signer to notify the user.
icrc25_supported_standards No This is simply querying signer capabilities.
icrc27_get_icrc1_accounts Yes Prompts the user to select / reconfirm the accounts.
icrc31_get_principals Yes Prompts the user to select / reconfirm the connected signer identities.
icrc32_sign_challenge Yes Prompts the user to confirm the challenge signature
icrc34_get_delegation No The user has already approved that the dapp may retrieve a scoped delegation when granting the permission. The actual exchange / signing of the delegation is a technical detail.
icrc49_call_canister Yes Prompts the user to approve a transaction

I think for an MVP framework of standards, this should be good enough.

More advanced use-cases, such as

should IMHO be solved within their own standards and at a later stage. I think it is important to first validate basic MVP use-cases with the tools given by the methods above (which arguably already go beyond an MVP) and then later extend ICRC-25 further where there is a clear need for improved UX / not supported use-cases.


@sea-snake, @plitzenberger, @kristoferlund, @dostro: Please share your opinion on this.

plitzenberger commented 5 months ago

@frederikrothenberger I like the proposal! My two cents on:

getting notified upon changes (i.e. account name change)

Ethereum defines provider events. Maybe we should standardize something like that as well? https://docs.metamask.io/wallet/reference/provider-api/#events

frederikrothenberger commented 5 months ago

@frederikrothenberger I like the proposal! My two cents on:

getting notified upon changes (i.e. account name change)

Ethereum defines provider events. Maybe we should standardize something like that as well? https://docs.metamask.io/wallet/reference/provider-api/#events

Yes, I agree. πŸ‘ Let's do that in a future extension though. πŸ˜‰

kristoferlund commented 5 months ago

Would we expect the signer to be able to handle "grouped" requests and show them to the user in one prompt? If not, making a canister call will require two prompts and approvals:

  1. Approve permissions request, icrc25_request_permissions
  2. Approve canister call, icrc49_call_canister

This is not an ideal UX. Showing both approvals in one prompt might be doable, basically "Do you approve this permission request and this call?"

 β”Œβ”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”
 β”‚  Approve permission request?    β”‚
 β”‚  β”Œβ”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”  β”‚
 β”‚  β”‚          <scope>          β”‚  β”‚
 β”‚  β””β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”˜  β”‚
 β”‚  Approve the following action?  β”‚
 β”‚  β”Œβ”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”  β”‚
 β”‚  β”‚    <consent_message>      β”‚  β”‚
 β”‚  β””β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”˜  β”‚
 β”‚  β”Œβ”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”   β”Œβ”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”  β”‚
 β”‚  β”‚  Reject   β”‚   β”‚  Approve  β”‚  β”‚
 β”‚  β””β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”˜   β””β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”˜  β”‚
 β””β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”˜
frederikrothenberger commented 5 months ago

@kristoferlund: Imho there should be connection / permission screen, where multiple permissions can be grouped.

So there would be n+1 approval screens for n canister calls, where the first one is the prompt for icrc25_request_permissions when you initially connect the signer which could group several permissions. This example would group the permissions for icrc31_get_principals and icrc49_call_canister:

Do you wish to connect to https://awesomedapp.com and share the following identities:

  • [x] Social Media (njsxs-anfxg-se6ti-anmmo-qdc7u-wtcd5-aqn6g-i6hna-uelej-nypov-6qe)
    • [x] Allow the dapp to initiate transactions with this identity
  • [x] Read Only Identity (hcg7g-lsf6s-ghhip-rjn6u-jkbdw-wsdzw-rfdwn-kd2ii-nicnq-dzsdp-aae)
    • [ ] Allow the dapp to initiate transactions with this identity
  • [ ] Top Secret (444v7-kuyfk-v4b5q-sbxol-2tjqu-jpohg-bpdog-pr53h-6jppm-uk2lw-jae)

In that way, the initial connection screen could already collect the permissions that the dapp will need anyway.

If you want to build an app that expands the permission as you go along feature by feature, then the UX should prompt for approval of that permission when the feature is enabled. That way it is also a distinct moment from when the transaction is submitted for approval (and motivated to the user, why they have to approve additional permissions in that moment).

I.e. compare that to say Android apps, where fundamental permissions are asked up-front and then other capabilities (such as access to the camera) is usually tied to a specific functionality of the app and nicely explained in well designed apps.

sea-snake commented 5 months ago

Just updated https://github.com/dfinity/wg-identity-authentication/pull/132 regarding interactivity before seeing this issue πŸ˜…

Prompts the user to select / reconfirm the (sub)accounts.

Main issue I see here would be stale data, there's no way to get the latest data in case the user e.g. changed the list of accounts to be shared to the dapp within the wallet itself.

Not sure how events would resolve this, since those would only be received when the change happens and the dapp is open at the same time. Unless events would be acknowledged by the dapp, so the wallet can make sure all dapps are aware of the user list change.

sea-snake commented 5 months ago

On the other hand, by not assuming data to be dynamic by default e.g. change user list within wallet after being shared with dapp. It would greatly simplify the amount of method calls to the wallet and implementation.

So yeah I think considering these facts I would opt to make methods either always interactive or non interactive but not both.

I'll update ICRC-27 to be always interactive (prompt user). Handling dynamic data changes should imho be handled through events with possibly acknowledge messages to make sure the dapp has received and processed the events

frederikrothenberger commented 5 months ago

Great feedback, thanks everyone! It seems that we agree on making user interaction explicit and solving refreshing data later. πŸ‘

We will discuss this in the next working group session on February 6 to make the final decision on this matter. If we agree on that path forward then, I'll update the drafts accordingly.

sea-snake commented 5 months ago

icrc34_get_delegation | Yes | Prompts the user to approve sharing a delegation

Not sure if it makes sense to prompt the user again for a delegation, the user already gets prompted for selecting principals or icrc1 accounts.

To additionally prompt the user for a delegation when you just want to create a delegation for one of those principals/accounts to communicate with your dapp canisters directly without any user prompts seems weird.

Assuming the intention behind icrc34 is something as described in https://github.com/dfinity/wg-identity-authentication/issues/33

I would assume icrc34 expects a principal as input and returns a delegation chain as output if the canister target(s) are valid for the given frontend.

frederikrothenberger commented 5 months ago

@sea-snake: I see your point. Currently undecided whether session authentication is something the user should be explicitly aware off... Let's discuss tomorrow. I'm currently leaning to non-interactive, as you have pointed out.

I'm also not sure on the arguments to get_delegation. If we implement this in II, then we will for sure go with a privacy preserving implementation similar to what we do now. I.e. it would just be a replacement for the current window post message interface built on a standardized interface.

But we should absolutely integrate the targets based approach for session isolation as well.

frederikrothenberger commented 5 months ago

Update working group meeting February 6, 2024:

Agreement was formed that the above table (initial post) is sensible and that user interaction should be explicit (rather than a signer choice).

For ICRC-34 it was clear, that additional discussions need to take place in order to find it's exact scope (i.e. how it relates to ICRC-28 and ICRC-31 and on the requirements for session isolation). However, ICRC-34 should be finalized independently of this discussion.

I will now update all the drafts to reflect the user interactions decided here and close this issue once these changes have been merged.