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

Delete WebAuthn key #195

Merged
merged 3 commits into from
May 31, 2023
Merged

Delete WebAuthn key #195

merged 3 commits into from
May 31, 2023

Conversation

iandunn
Copy link
Member

@iandunn iandunn commented May 27, 2023

See #193

This adds the functionality for deleting a key. Design will be refined in #194, so this PR only focuses on the code. The Registration flow will be completed in #200, so this PR only focuses on the deletion flow.

Screenshot 2023-05-29 at 4 22 22 PM
Screenshot 2023-05-29 at 4 22 42 PM
Screenshot 2023-05-29 at 4 23 30 PM
Screenshot 2023-05-29 at 4 23 57 PM

@iandunn iandunn added this to the Iteration 1 milestone May 27, 2023
@iandunn iandunn self-assigned this May 27, 2023
@iandunn iandunn changed the title Add dynamic functionality to WebAuthn UI stub Delete WebAuthn key May 29, 2023
@iandunn iandunn force-pushed the webauthn-functionality branch 4 times, most recently from cecd50f to cdcdd80 Compare May 29, 2023 23:20
@iandunn iandunn marked this pull request as ready for review May 29, 2023 23:25
@renintw
Copy link
Contributor

renintw commented May 30, 2023

Saw error messages when inputting. Has it been noticed and expected to be fixed in #194?

t

--

Also, I saw two keys always with the name New Key added when registering a new key while sandboxed or non-sandboxed, is it expected behavior at the moment?
image

--

And I couldn't Remove the key, it would get 400 error also on both sandbox and local env, is it expected at the moment as well or I might've missed some settings?
image

@iandunn iandunn mentioned this pull request May 30, 2023
@iandunn
Copy link
Member Author

iandunn commented May 30, 2023

Saw error messages when inputting

I noticed that too, but the Registration flow is still just a stub from #95. The only reason I made a change to it in this PR was because I needed to decouple it from the Deletion flow (see 5973b13)

The Deletion flow is what this PR is really about.

two keys always with the name New Key added when registering a new key while sandboxed or non-sandboxed, is it expected behavior at the moment?

Yeah, that's just throwaway code from #95, it's only purpose is to demonstrate the flow to Design, and give us a code skeleton to iterate on. The deletion process should be working fully in this PR though.

I couldn't Remove the key, it would get 400 error

That's definitely a problem, but I haven't been able to reproduce it. Can you check that the handle and _ajax_nonce fields in the POST request match their values in wp-admin/profile.php? Look for them in the data- attributes in .webauthn-keys span.delete a.

Also make sure that wp-admin isn't asking you to revalidate the 2fa session. Thanks!

@renintw
Copy link
Contributor

renintw commented May 30, 2023

Saw error messages when inputting

I noticed that too, but the Registration flow is still just a stub from #95. The only reason I made a change to it in this PR was because I needed to decouple it from the Deletion flow (see 5973b13)
The Deletion flow is what this PR is really about.

two keys always with the name New Key added when registering a new key while sandboxed or non-sandboxed, is it expected behavior at the moment?

Yeah, that's just throwaway code from #95, it's only purpose is to demonstrate the flow to Design, and give us a code skeleton to iterate on. The deletion process should be working fully in this PR though.

👍

I couldn't Remove the key, it would get 400 error

That's definitely a problem, but I haven't been able to reproduce it. Can you check that the handle and _ajax_nonce fields in the POST request match their values in wp-admin/profile.php? Look for them in the data- attributes in .webauthn-keys span.delete a.

Also make sure that wp-admin isn't asking you to revalidate the 2fa session. Thanks!

I see the problem. I thought I could directly register a new key using the button provided and then delete it, but then I realized that the Register Key functionality has not been implemented yet, as there's an open PR for it. I've added webauthn key manually and successfully deleted it on sandbox 👍 Sorry for the confusion.

@iandunn
Copy link
Member Author

iandunn commented May 31, 2023

Ah, that makes sense, my bad for not giving clearer testing instructions!

@iandunn iandunn force-pushed the webauthn-functionality branch from a2d081d to 5561e30 Compare May 31, 2023 16:02
@iandunn iandunn merged commit e426735 into trunk May 31, 2023
@iandunn iandunn deleted the webauthn-functionality branch May 31, 2023 16:03
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.

3 participants