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

New feature: passkey authentication for backend #97

Merged
merged 47 commits into from
Jan 25, 2025
Merged

New feature: passkey authentication for backend #97

merged 47 commits into from
Jan 25, 2025

Conversation

fballiano
Copy link
Contributor

@fballiano fballiano commented Jan 15, 2025

This PR adds basic passkey support to the backend.

Screenshot 2025-01-15 alle 22 35 53

and in the login page:

Screenshot 2025-01-15 alle 22 36 34

Passkeys only work if the website is under SSL, that's also necessary in order to be able to test this PR.

@justinbeaty
Copy link
Contributor

Cool! Checking it out now.

@justinbeaty
Copy link
Contributor

Do you mind if I push a few changes? I'd like to remove the inline JS from Mage_Adminhtml_Block_System_Account_Edit_Form and put into a separate JS file and a few other small improvements.

@fballiano
Copy link
Contributor Author

Do you mind if I push a few changes? I'd like to remove the inline JS from Mage_Adminhtml_Block_System_Account_Edit_Form and put into a separate JS file and a few other small improvements.

@justinbeaty feel free! absolutely :-)

@justinbeaty
Copy link
Contributor

Couple of changes, as always all are just suggestions and we can modify / revert any of them.

1. I moved the base64 methods to the already existing Base64 JS object.
2. Moved the main code to a new class at public/js/mage/adminhtml/system/account.js, it takes a config object with URLs.
3. Used the $this->getButtonHtml() method for generating the buttons.

I'm actually not sure yet how to test this, it seems I'll need a hardware key as I don't have one built into my computer.

@fballiano
Copy link
Contributor Author

I'm actually not sure yet how to test this, it seems I'll need a hardware key as I don't have one built into my computer.

I can use it with my password manager without any problem

@fballiano
Copy link
Contributor Author

@justinbeaty what do you think about the questions at the end of the PR description?

@justinbeaty
Copy link
Contributor

@justinbeaty what do you think about the questions at the end of the PR description?

Let me look closer, also see if I can figure out how to make it work with bitwarden.

@fballiano
Copy link
Contributor Author

Let me look closer, also see if I can figure out how to make it work with bitwarden.

I use another one but bitwarden should support passkey, when the QR code appears, you should scan it on your phone and it should trigger bitwarden etc...

@justinbeaty
Copy link
Contributor

justinbeaty commented Jan 16, 2025

I figured out the problem with bitwarden, I had my local install URL on the ignore list because I was tired of it asking to update the password when testing the 2fa code.

Also just fixed an error where I had used the same field name in my previous commit.

…ler.php

Co-authored-by: Justin Beaty <51970393+justinbeaty@users.noreply.github.com>
@justinbeaty
Copy link
Contributor

One thing regarding setBodyJson:

            $this->getResponse()
                ->setHttpResponseCode(400)
                ->setBodyJson(['error' => $e->getMessage()]);

The setHttpResponseCode(400) will actually make it so the error message isn't read by mahoFetch, because it fails the if (!response.ok) check.

But I feel not sending a 200 code is better, so probably better to fix mahoFetch, right?

@fballiano
Copy link
Contributor Author

mmm some things are not clear to me

  • the "remove passkey" button doesn't work (it doesn't do any ajax call nor form submit), but it was working when I left the PR
  • when I register a passkey I get this
Screenshot 2025-01-25 alle 13 28 04 but it doesn't show the "enable password authentication" because I have to refresh

sorry man, I kinda liked it more as it was before, with the complete submit of the form instead of the ajax call.

now I don't know what to do

@fballiano
Copy link
Contributor Author

What I've committed now:

  • removed password_enabled field
  • different layout of the form
  • adding a check on user->save to avoid wrong disabling of password login if there's no passkey (I preferred when it was simpler -> passkey = no password and that's it, this passkey_enabled I think it's a bit convoluted and possible to errors and people unable to login)

but still doesn't work, the register passkey experience is not good enough because it doesn't refresh the page and the remove passkey doesn't work.

@justinbeaty
Copy link
Contributor

@fballiano The added or deleted passkeys aren't completed until the save button on the whole form is pressed. Before you could register a passkey without the current password, but with the logic consolidated in the saveAction it seemed cleaner to me. It also keeps it consistent with how 2fa is enabled, password changes, etc.

I think there's other organizational stuff in the commits I pushed that are worth keeping, but let's get this form layout nailed down. If you push your work so far I can probably fix the few issues.

@justinbeaty
Copy link
Contributor

The added or deleted passkeys aren't completed until the save button on the whole form is pressed.

And I'm not saying it has to be this way, but we should require the current password, which I know you handled on the delete action, but it might be a bit trickier on the create passkey option. Example:

  • User leaves current password field empty
  • User clicks register passkey and creates it through their password manager
  • Our JS can't yet make the POST request, need to prompt for the current password
  • How do we allow the user to send the POST request once they enter their password?

@fballiano
Copy link
Contributor Author

fballiano commented Jan 25, 2025 via email

@justinbeaty
Copy link
Contributor

I'll try adding the auto submit and other bug fixes.

@justinbeaty
Copy link
Contributor

justinbeaty commented Jan 25, 2025

Okay I think it is better now. 😃

I pressed the delete button, but hadn't entered my password, so it shows this message:

image

However if the current password was filled, the form would auto submit and reload.

@fballiano
Copy link
Contributor Author

niceeee, I've changed this sentence to make it even clearer

Screenshot 2025-01-25 alle 16 00 39

if you're ok with everything, I'd merge and start spamming 😂

@justinbeaty
Copy link
Contributor

Yeah, you're much better wording those messages than I am, haha.

One quick change I'd do is add a getter Mage_Admin_Model_User::getPasskeyEnabled() so that we don't have to do this everywhere:

$passkeyEnabled = $this->getPasskeyCredentialIdHash() !== null;

@fballiano
Copy link
Contributor Author

will do right away

@fballiano
Copy link
Contributor Author

Added, but I've called it isPasskeyEnabled() since it's not a real "get" of a property/variable

@justinbeaty
Copy link
Contributor

Added, but I've called it isPasskeyEnabled() since it's not a real "get" of a property/variable

Great, I'll test one more time then merge button. 😉

@fballiano
Copy link
Contributor Author

I've broke something cause I can't login anymore 🤦‍♂️😂

@fballiano
Copy link
Contributor Author

mmm seems that when the form is re-saved (let's say changing the username) after the passkey way already enabled -> can't login anymore

@fballiano
Copy link
Contributor Author

nothing, I'm allucinating, cause I've re-did everything and it's working perfectly

@justinbeaty
Copy link
Contributor

nothing, I'm allucinating, cause I've re-did everything and it's working perfectly

I've tested saving, and every combination of passkey +/- password, and 2FA on both types of logins. Checked the DB values at each step and all seemed to work perfect.

If you can't reproduce again I'd say it's ready!

@fballiano
Copy link
Contributor Author

retested again and no problem, let's go!

@fballiano fballiano merged commit 7699760 into main Jan 25, 2025
23 of 24 checks passed
@fballiano fballiano deleted the passkey branch January 25, 2025 16:28
@fballiano
Copy link
Contributor Author

and Maho is the first magento based platform to have passkey support

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.

2 participants