-
Notifications
You must be signed in to change notification settings - Fork 381
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
Offer to store Account Number in password manager #7737
Offer to store Account Number in password manager #7737
Conversation
0dd1e93
to
bbce20b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found three things while testing:
- The account number is saved as a password and not a username. I assume this intentional?
- When trying to log in to the app I do not get prompted to use my stored account number. I guess this is because the input field is not a password. IIRC we do not want it to be a password for reasons...
- When trying to log in on the homepage I also do not get prompted. I guess this is becase the account is saved under
mullvadvpn.mullvad.net
is there a way to change this?
Reviewed 3 of 3 files at r1, 5 of 5 files at r2, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved
bbce20b
to
43663bd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Yes, this is intentional, since the account number in closer to a password than a username and a bit of a secret that you don't share. This also means that password managers won't show it w/o user explicitly revealing it like on our account page.
- That would part of another feature, with Compose 1.8.0 we can offer autofill from keyboard, then if we want to offer to store on login that would be yet another feature.
- Yes, there is nothing linking the password entry to the website at this moment, I'm a bit uncertain about the integration to make it work with the website (if it is supported at all). But there is something called digital asset links (https://developer.android.com/identity/sign-in/credential-manager#add-support-dal) that allows us to add a link to our webiste as well, if this would be forwarded to the website with the store password request I'm not sure of though.
Reviewable status: 8 of 10 files reviewed, all discussions resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 👍
- Right, let's wait until then again.
- I think it is fine with this feature then, just maybe create a new issue if we want to explore that in the future.
Reviewable status: 3 of 10 files reviewed, all discussions resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 7 of 7 files at r4, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Overview
This PR adds a feature to prompt the password manager to store a new account number during account creation by updating dependencies and documentation.
- Added the androidx-credentials dependency in the Gradle versions file.
- Updated CHANGELOG.md to reflect the new functionality.
Reviewed Changes
File | Description |
---|---|
android/gradle/libs.versions.toml | Added the androidx-credentials dependency for password manager usage |
android/CHANGELOG.md | Added changelog entry describing the new account number storage prompt |
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
43663bd
to
0b0d60e
Compare
6f04b15
to
f65ac5c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved
These are only required if we use passkeys or uses methods that make use DAL (Digital Asset Links).
f65ac5c
to
1907ced
Compare
When users create new account prompt their password manager, if available, to store the password.
This change is