Closed notandyvee closed 3 months ago
3 Warnings | |
---|---|
:warning: | strings.xml files should only be updated on release branches, when the translations are downloaded by our automation. |
:warning: | This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews. |
:warning: | Class OkHttpPasskeyRepository is missing tests, but unit-tests-exemption label was set to ignore this. |
Generated by :no_entry_sign: Danger
📲 You can test the changes from this Pull Request in Simplenote Android by scanning the QR code below to install the corresponding build.
App Name | Simplenote Android | |
Build Type | Debug | |
Commit | 2c0748b4530c79bfab698ee169dec61b1f56c2d6 | |
Direct Download | simplenote-android-prototype-build-pr1665-2c0748b-0190ccf9-2d08-4ea6-a99d-d397c0dee14a.apk |
Hey @khaykov if you can fill in for Danilo 🙏🏻
@notandyvee It works! I was able to get to "Successfully added passkey".
I have a couple of usability comments:
I have no idea what the "Passkey" does and why should I use it. Can we have some sort of explanation on what it is? Maybe combined with password dialog?
If you do not have a screen lock enabled, you will have to setup it up first. After you are done, I would assume that passkey will be enabled, but it is not - I have to go back to the app and restart the process. Wonder is it possible to resume process automatically? Or let user know that passkey is not set up?
After I add a passkey, the menu item still says "Add Passkey Authentication".
Error message when entering wrong password is not very clear for user. I believe it should be something simple like "Wrong credentials", etc. Part about "challenge" will not make much sense to regular person.
I see an error when dismissing passkey bottom sheet, the message is very generic and does not represent what actually happens.
@khaykov I think all your points are valid. But I think we should address all of them in a separate PR. Most of them require discussions. For example, there is no way to know when a passkey was added on the backend. We could check the local passkeys. But what if the user added the passkey on web? Should mobile not show that? For now I think it's out of scope for the PR.
The exception is
I see an error when dismissing passkey bottom sheet, the message is very generic and does not represent what actually happens.
That's sort of a legitimate issue. For some reason the library throws an exception when that happens 😅 . I can update the event to say user cancelled so there is no error.
Done @khaykov . Any other improvements we can add in a future PR. You shouldn't see an error anymore. For the record, CredentialManager is not properly documented. Apparently they use these errors. Not very descriptive for our specific issue. But for now this helps.
Fix
This PR is the 2nd step in this 3-4 part series in adding passkeys to Simplenote.
This step allows you to add a passkey for your account in order to be able to authenticate with something.
Test
CredentialManager
will pop up a bottomsheet to associate a passkey with your account. Select the device you are using. Using other methods has not been tested.Review
Only thing I'd like to note on this PR is
PasskeyManager
was created in order extract the logic out of the Fragment and into kotlin land. I didn't put it in the viewmodel because the credential manager needs a context, which could expose a memory leak. Your thoughts on a better place is welcome.Release
This PR requires updates to release notes.