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

[HOLD for payment 2021-12-15] Unable to use 1Password option for autofill on login #3855

Closed
isagoico opened this issue Jul 1, 2021 · 52 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@isagoico
Copy link

isagoico commented Jul 1, 2021

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Navigate to staging.expensify.cash
  2. Right click on the email field
  3. Click on the 1Password option

Expected Result:

Option to select existing accounts should be displayed

Actual Result:

Nothing happens when clicking on the 1Password option

Workaround:

No, unable to use the 1Password extension.

Platform:

Where is this issue occurring?

Web ✔️
iOS
Android
Desktop App
Mobile Web

Version Number: 1.0.75-5

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

image

Expensify/Expensify Issue URL:

View all open jobs on Upwork


From @mallenexpensify https://expensify.slack.com/archives/C01GTK53T8Q/p1625156109326100?thread_ts=1625155719.325000&cid=C01GTK53T8Q

Be able to right click on the Phone or Email box and have a menu option that allows for selecting 1Password. OR, be able to see and click a 1Password icon on the far right of the box.
Ideally, once 1P is used for entering the email address, the Password and Two Factor Code on the following page will be autofilled as well.

@MelvinBot
Copy link

Triggered auto assignment to @flodnv (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@anthony-hull
Copy link
Contributor

anthony-hull commented Jul 2, 2021

This is because the password forms on this page don't meet best practices for user password forms:
https://web.dev/sign-in-form-best-practices/
I believe that it won't detect the form because of this.
When I add the password field after the username lastpass detects the inputs and adds the autofill features. So I think it's a detection issue.
It would be good to know is this happens on Android and firefox as they use heuristics for detection.
I'll test 1Password later to see if it behaves the same.

Can I ask if you've experienced 1Password working with split forms before (slip forms where username and password input appear on different pages or stages? Once we follow best practices there is still a chance they don't implement support for them. An example of split forms is amazon.com etc.

Secondly there is another issue that will appear after this one is fix: I don't think it would be able to chose the right password for you when it does trigger as the login is split across two pages. The password manager doesn't have the context it needs to match a user:pass combo.

I saw another symptom of the same issue before when setting a password, it tries to save the password without a username because it doesn't know what the username is in that context.

If you've implemented an "email first" sign-in flow

The saving issue is with chrome password manager and last pass that I've experienced.

For the initial issue I propose further investigating the limitations of detection in the major password managers. Hopefully once the best practices are implemented the split form will work. Otherwise we might need to change the form depending on the limitation of which one we decide to support. which brings me to the next question.
Which managers do you want to support?

Firefox, Chrome, Edge, iOS, Android, Safari, 1Password, LastPass.... the list goes on. I don't see a browserlist query in the project.

For my second issue:
I propose adding a hidden username form field in the form to let the manager know what password to fill.
I propose also to create another issue to further audit all the password forms in the application to meet best practices e.g. not using placeholders, don't make the user replete themselves (once password managers work correctly).

@flodnv flodnv removed their assignment Jul 2, 2021
@flodnv flodnv added the External Added to denote the issue can be worked on by a contributor label Jul 2, 2021
@MelvinBot
Copy link

Triggered auto assignment to @kevinksullivan (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@parasharrajat
Copy link
Member

cc: @iwiznia I think this is an important issue and @anthony-hull has a good proposal.

@iwiznia
Copy link
Contributor

iwiznia commented Aug 13, 2021

Thanks for bumping the issue @parasharrajat!
Seems Kevin missed this issue and should've been exported a while ago. Asking someone else to export it and then an engineer will get assigned to review the proposals.

@parasharrajat
Copy link
Member

It just needs a hidden username input and all password managers should work. If chrome can detect the form then all others will. They follow what chrome does.

@laurenreidexpensify
Copy link
Contributor

https://www.upwork.com/jobs/~01b7f83dea2310bcf3 posted to Upwork

@MelvinBot MelvinBot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 13, 2021
@MelvinBot
Copy link

Triggered auto assignment to @AndrewGable (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@laurenreidexpensify
Copy link
Contributor

@anthony-hull did you have any proposals to fix this issue?

@MelvinBot MelvinBot removed the Overdue label Aug 16, 2021
@kevinksullivan
Copy link
Contributor

Sorry I completely missed the export on this, and thanks for stepping in @laurenreidexpensify . Updating my email filters/notifications to make sure this doesn't happen again.

@laurenreidexpensify
Copy link
Contributor

No stress @kevinksullivan - pro tip, download the K2 extension and use it daily and I guarantee you you'll never need a GH notification again to manage your issues :)

@anthony-hull
Copy link
Contributor

It just needs a hidden username input and all password managers should work. If chrome can detect the form then all others will. They follow what chrome does.

as per chromium's docs I did try adding a hidden username field when I first investigated. It didn't like it! Then suddenly worked once I removed display: none I'll have a look at it again 👍

@AndrewGable AndrewGable added Weekly KSv2 and removed Daily KSv2 labels Aug 18, 2021
@MelvinBot MelvinBot removed the Overdue label Aug 18, 2021
@laurenreidexpensify laurenreidexpensify removed their assignment Aug 20, 2021
@mallenexpensify
Copy link
Contributor

@parasharrajat should I reopen this issue then? Or... is the PR you mentioned above associated with a separate issue?

@parasharrajat
Copy link
Member

parasharrajat commented Oct 14, 2021

Yes please reopen. PR is linked to this issue so I would like to finish that. and you can remove the help wanted and assign me. Thanks

@mallenexpensify mallenexpensify removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 14, 2021
@rushatgabhane
Copy link
Member

Hey @parasharrajat what's the status on this one?
Asking because it's frustrating when autofill doesn't work.

@parasharrajat
Copy link
Member

It's in review. I will update the code with new changes this weekend. So hopefully you will have it next week.

@parasharrajat
Copy link
Member

Awaiting review...

@kevinksullivan
Copy link
Contributor

Looks like this is in requested changes now.

@botify botify added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Monthly KSv2 labels Dec 8, 2021
@botify
Copy link

botify commented Dec 8, 2021

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.18-3 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2021-12-15. 🎊

@botify botify changed the title Unable to use 1Password option for autofill on login [HOLD for payment 2021-12-15] Unable to use 1Password option for autofill on login Dec 8, 2021
@AndrewGable
Copy link
Contributor

AndrewGable commented Dec 10, 2021

Used this today on staging, worked great!! 😍

@parasharrajat
Copy link
Member

Thanks, I had to do a lot of refactor to make it work.

@mallenexpensify
Copy link
Contributor

@parasharrajat I just tested on Web, Chrome, Staging and - after I used 1Password to add my email address, I wasn't able to click the return key to move to the next step (activate the continue button). I had to click tab then enter to get to the next step.

Can this be fixed, updated? If so... can/should it be done via the current PR or should a new issue be created?

@parasharrajat
Copy link
Member

Let me check @mallenexpensify

@mallenexpensify
Copy link
Contributor

One other thing, which I don't think is directly related to this. Can you test to see, if a user has a 2FA one-time password in 1Password, should that auto-fill on all platforms? I've had issues with 1P and NewDot on iOS for many months, wondering if others are also having similar issues. (I think I just got 1P and NewDot iOS working better except that I'm not getting the one-time password added after the password is added)

@parasharrajat
Copy link
Member

parasharrajat commented Dec 10, 2021

Ok. I got you there @mallenexpensify. I will add that as well but just for the record not part of this issue.

About this #3855 (comment),
It seems to be configured but If you could share a video that would be helpful.

@mallenexpensify
Copy link
Contributor

I will add that as well but just for the record not part of this issue.
If the 2FA/one-time-code isn't clearly called out in this issue, does $250 for increased scope seem fair? Feel free to include in this PR or we can create a new issue.

For focus not advancing to the Continue button after an email address has been entered, he's a vid

2021-12-10_15-27-11.mp4

and here's what's showing in logs
image

@parasharrajat
Copy link
Member

@mallenexpensify pressing enter after email is autofill should take you to the password screen. It's working for me and was working before this issue.

@parasharrajat
Copy link
Member

parasharrajat commented Dec 13, 2021

Do you want me to create a PR for #3855 (comment)?

@mallenexpensify
Copy link
Contributor

mallenexpensify commented Dec 14, 2021

pressing enter after email is autofill should take you to the password screen. It's working for me and was working before this issue.

It's not working for me and I think we should fix it. I'm on staging.new.expensify.com and am using 1Password to autofill. After I select my email address and hit enter, nothing happens, I'm unable to advance to the password screen without clicking the Continue box with my cursor.

Do you want me to create a PR for #3855 (comment)?

@AndrewGable are you alright with this or would you prefer a brand new issue to address including the one-time 2FA pass?

@parasharrajat
Copy link
Member

@mallenexpensify We can process the payment for this at it is pay day before proceeding to follow up issue.

@mallenexpensify
Copy link
Contributor

@parasharrajat This issue is a bit confusing, I paid $500 for the initial fix, is more compensation due? If so, can you provide details?

@parasharrajat
Copy link
Member

Yeah, Looks like this went through both holds (n6 and company offsite).

@mallenexpensify
Copy link
Contributor

Thanks @parasharrajat It's def clear in this issue that n6-hold is applicable
image

For Company offsite hold (where I didn't properly assign the label to issues that were currently in-progress, it appears that the timeline for that is also applicable, as your PR wasn't reviewed until we lifted it.

image

$500 Bonus added to the original job that I already paid $500 on.

Closing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests