Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle Declined proof (abandoned state callback) #406

Merged
merged 1 commit into from
Jan 26, 2024

Conversation

loneil
Copy link
Contributor

@loneil loneil commented Jan 16, 2024

NOTE: For this to function a subsequent upcoming change to ACA-Py will need to be released. ACA-Py currently does not send a problem report for abandoned presentations on connectionless presentation requests. The change to remove this restriction will need to be accepted and released.

However, on the VCAuthn side, this is just adding a new state handler, so this could conceivably be merged in without any adverse affects, the state just won't be triggered unless the ACA-Py change is in place.


Add a state handler in the controller acapy handler for a present_proof topic that contains a state abandoned (which is triggered when the user declines a presentation request).

The topic handler in the existing code already checks the incoming webhook presentation_exchange_id to get the appropriate auth session. So add a new check for abandoned state and emit that to the handler to display the HTML page with the new state.
The state is just "abandoned", the error message says abandoned: Declined so possibly could parse out more if there were multiple abandoned cases to handle... but that would be relying on a text error message display rather than state.

See object contents from debug session below:
image

Alter the frontend page to handle this state. It looks like below and the user can hit try again or refresh to do a proof again if they accidentally decline. Can adjust wording/icon/color if needed.

image

Additionally refactor the JS in the HTML file to generalize the UI element setup and reduce duplication.

@@ -273,93 +289,27 @@ <h1>Scan with a Digital Wallet</h1>
socket.connect();

const toggleState = (state) => {
const intro = document.querySelector(".intro");
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 refactored all this away to the setUiElements function below to generalize.

qrcode.classList.remove("pending");
setUiElements(".expired", true, false, false);
break;
case "abandoned":
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New state

@loneil loneil self-assigned this Jan 16, 2024
@loneil
Copy link
Contributor Author

loneil commented Jan 16, 2024

Draft ACA-Py changes can be seen at openwallet-foundation/acapy#2723

@loneil loneil marked this pull request as ready for review January 17, 2024 23:17
@loneil loneil requested review from esune, Jsyro and Gavinok January 17, 2024 23:17
Copy link
Contributor

@Jsyro Jsyro left a comment

Choose a reason for hiding this comment

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

Looks Great!

Copy link
Member

@esune esune left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏻

@esune
Copy link
Member

esune commented Jan 23, 2024

@loneil just confirming we can merge and the current code will not break - nothing will actually change until the related fix in ACA-Py is pulled in by a new image.

@loneil
Copy link
Contributor Author

loneil commented Jan 24, 2024

@esune Yes it can go in, the only change depends on a state that won't show up yet so it won't have any affect on anything yet (I did refactor a bit on the UI page but tested out the other cases and there weren't any logic changes)

@loneil
Copy link
Contributor Author

loneil commented Jan 25, 2024

Will try this out with the 0.12.0 RC0 acapy image soon on this PR.

@esune esune merged commit bee6dff into main Jan 26, 2024
5 checks passed
@esune esune deleted the feature/addAbandonedState branch January 26, 2024 17:35
@loneil
Copy link
Contributor Author

loneil commented Jan 26, 2024

This was merged so put the 12.0 (tested and working locally) here #407

@esune
Copy link
Member

esune commented Jan 26, 2024

This was merged so put the 12.0 (tested and working locally) here #407

Sorry, I got merge happy today 🙂

@loneil
Copy link
Contributor Author

loneil commented Jan 26, 2024

Probably best to keep it separate since it's an RC anyways

@sgroh
Copy link

sgroh commented Jan 26, 2024

Sorry I got lost in messages, what is the ACA-Py version that could interoperate with this PR? I mean Once ACA-Py add the new "state", Are we be able to use an external wallet like eSatus/Lissi ?

@esune
Copy link
Member

esune commented Jan 29, 2024

Sorry I got lost in messages, what is the ACA-Py version that could interoperate with this PR? I mean Once ACA-Py add the new "state", Are we be able to use an external wallet like eSatus/Lissi ?

The new ACA-Py version is just to handle the abandoned state (i.e.: when the user declines to share their credentials). eSatus/Lissi should work regardless of what ACA-Py version is used.

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.

4 participants