Skip to content

Conversation

@awoie
Copy link
Contributor

@awoie awoie commented Jun 25, 2024

This PR adds an optional error code wallet_unavailable; fixes #191.

@awoie awoie requested review from Sakurann, c2bo, jogu and tplooker June 25, 2024 07:58
Copy link
Collaborator

@jogu jogu left a comment

Choose a reason for hiding this comment

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

I think we probably need some more explanation in the specification text; it's not intuitive how this code is used and I think we need to mention the privacy/security concerns (as discussed on the issue) that the error code mustn't be returned without consent from the user.

Co-authored-by: Joseph Heenan <joseph@authlete.com>
@awoie
Copy link
Contributor Author

awoie commented Jun 27, 2024

I think we probably need some more explanation in the specification text; it's not intuitive how this code is used and I think we need to mention the privacy/security concerns (as discussed on the issue) that the error code mustn't be returned without consent from the user.

That sounds good to me. I'll update the PR.

@awoie awoie requested review from jogu and peppelinux June 27, 2024 12:40
@awoie
Copy link
Contributor Author

awoie commented Jun 27, 2024

@peppelinux @jogu I updated the PR to incorporate your requested changes. Please review again.


`wallet_unavailable`:

- The Wallet appears to be unavailable and therefore unable to respond to the request. This can be useful in situations where the user agent cannot invoke the Wallet and another component receives the request while the user still controls the user experience and wishes to continue the journey on the Verifier website. For example, this applies when using claimed HTTPS URIs handled by the Wallet provider in case the platform cannot or does not translate the URI into a platform intent to invoke the Wallet.
Copy link
Member

Choose a reason for hiding this comment

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

I would use the term user-agent to clarify that's a specialized entity

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I'm following, could you expand on your suggestion please Guiseppe? Maybe suggest alternative text?


## Authorization Error Response with Wallet unavailable error code

In the event that another component is invoked instead of the Wallet while the user still controls the user experience, the user MUST be informed and give consent before returning the `wallet_unavailable` Authorization Error Response to the Verifier.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I follow the 'while the user still controls the user experience' part. What is an example of where the user isn't in control?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I can remove that by-sentence because we are already saying the user MUST be informed. I updated the PR.

@awoie awoie requested a review from jogu July 5, 2024 11:48
@jogu jogu requested a review from peppelinux July 5, 2024 16:57

## Authorization Error Response with Wallet unavailable error code

In the event that another component is invoked instead of the Wallet, the user MUST be informed and give consent before returning the `wallet_unavailable` Authorization Error Response to the Verifier.
Copy link
Collaborator

Choose a reason for hiding this comment

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

so we are relying on this another component that is invoked to return this error, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Sakurann I updated the PR.


`wallet_unavailable`:

- The Wallet appears to be unavailable and therefore unable to respond to the request. This can be useful in situations where the user agent cannot invoke the Wallet and another component receives the request while and the user wishes to continue the journey on the Verifier website. For example, this applies when using claimed HTTPS URIs handled by the Wallet provider in case the platform cannot or does not translate the URI into a platform intent to invoke the Wallet.
Copy link
Collaborator

Choose a reason for hiding this comment

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

If my understanding is correct, I would add something to clarify that it is not the wallet, but the wrong component invoked instead of a correct wallet that return this error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that is correct. I can add this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Sakurann I updated the PR.

Copy link
Collaborator

@Sakurann Sakurann left a comment

Choose a reason for hiding this comment

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

please add document history entry :)

awoie and others added 4 commits July 15, 2024 18:40
Co-authored-by: Kristina <52878547+Sakurann@users.noreply.github.com>
Co-authored-by: Kristina <52878547+Sakurann@users.noreply.github.com>
@awoie awoie requested a review from Sakurann July 16, 2024 14:12
@jogu
Copy link
Collaborator

jogu commented Jul 23, 2024

Now has 3 reviews (many thanks @ssanchocanela!), all outstanding comments resolved and has been open for over a week - merging!

@jogu jogu merged commit 5cbccf1 into main Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Error code if the wallet is not installed

6 participants