This repository was archived by the owner on Dec 14, 2021. It is now read-only.
Authenticate via fingerprint or PIN to autofill when the app is locked#413
Merged
Authenticate via fingerprint or PIN to autofill when the app is locked#413
Conversation
ghost
assigned
ioana-farcas
changed the title
jhugman
force-pushed
the
216-authenticate-to-autofill
branch
from
37f5eeb to
055fd3c
Compare
sashei
reviewed
Feb 7, 2019
Contributor
sashei
left a comment
sashei
left a comment
There was a problem hiding this comment.
drive-by!
I gather you've found a way around the AutofillService requiring both the password list and the authentication callback in the same fill response, but I assume you're looking for feedback on the changes to how the AccountStore / AutolockStore interact with the DataStore -- I like this approach much better than what we had before, but would like to see it live in the DataStore rather than having the Account- and Autolock- stores dispatch their own actions. Happy to do some pairing / discussion on this if you're keen!
| } | ||
|
|
||
| override fun onConnected() { | ||
| // stupidly unlock every time :D |
jhugman
force-pushed
the
216-authenticate-to-autofill
branch
2 times, most recently
from
ab516cc to
f3265da
Compare
…nd if the credentials exist.
This may be better done in the application.
a) making ParsedStructure contain definitive webDomain and packageName (defaulting to the calling activity) b) making PSL a singleton.
…dStructureBuilder tests.
jhugman
force-pushed
the
216-authenticate-to-autofill
branch
from
f3265da to
72a58d8
Compare
jhugman
changed the title
linuxwolf
previously requested changes
Feb 12, 2019
Contributor
linuxwolf
left a comment
linuxwolf
left a comment
There was a problem hiding this comment.
Engineering wise, this looks good to me.
- The PSL changes make sense
- Stores reading state from other stores is sensible
A nit on the localizable strings, to better align with designs.
| <!-- This is the error message when autofill is triggered but no domain is detected. This is unlikely. --> | ||
| <string name="autofill_error_no_hostname">Unexpected hostname format</string> | ||
| <!-- This is the prompt when autofill is triggered, Lockbox is unlocked, and no passwords are detected. --> | ||
| <string name="autofill_search_cta">Tap to search</string> |
Contributor
There was a problem hiding this comment.
UX designs call for this to be "Search Lockbox"
| <string name="convenience_description">Unlock the app with ease using your fingerprint</string> | ||
|
|
||
| <!-- This is the prompt when autofill is triggered, and Lockbox is locked. --> | ||
| <string name="autofill_authenticate_cta">Tap to unlock</string> |
Contributor
There was a problem hiding this comment.
I suspect @mozilla-lockbox/ux would prefer "Unlock Lockbox"
Contributor
There was a problem hiding this comment.
UNLOCK YOUR FIRELOCK FOXBOX
10 tasks
8 tasks
This was referenced Feb 15, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #216
Testing and Review Notes
In the current state, this works for authenticating using PIN (when app does not enter in a weird state, when there are no entries in the list). For authentication using fingerprint, the only reason it is not working (as far as I can see), is the fact that DataStore.State == null. Don't know exactly why, I will try tomorrow to take a look at this.
@jhugman I think you will take over, the code needs a little refactoring..
Screenshots or Videos
(Optional: to clearly demonstrate the feature or fix to help with testing and reviews)
To Do