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

Add input elements to login page for password managers #9369

Merged
merged 7 commits into from
Jun 24, 2021

Conversation

rianadon
Copy link
Contributor

@rianadon rianadon commented Jun 4, 2021

I gave up fighting the quirks that appeared in #9235 so I'm trying plan B: create input fields outside the shadow dom that password managers can see, and keep these new fields in sync with the existing ones.

Proposed change

The new ha-password-manager-polyfill script adds this HTML to the page:

<form id="password-manager-polyfill" aria-hidden="true">
  <input id="username" />
  <input id="password" type="password" />
  <input type="submit"/>
  <style>
    #password-manager-polyfill {
      position: absolute;
      width: 0;
      height: 0;
      overflow: hidden;
    }
  </style>
</form>

With the embedded <style> element, the form is rendered so that password mangers still think it exists, but is invisible to humans.

Input events are captured on the form, and the username and password fields on the page are updated to reflect whatever new content was inserted.
The form submit is also intercepted, and the "next" button is clicked when the form is submitted by the password manager.

⚠️ future problem warning ⚠️

When Chrome, browsers, and password managers fix this bug on their end, having two password fields might confuse password managers. I can't test because nobody has seemed to have fixed it yet, but I'd like to make this change future-proof.

I don't think this would cause issues, because if the password manager decides to fill out the existing HA fields and not the invisible ones, all the better. But I'm open to advice here.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally (in Firefox with gopass-bridge; I haven't had the chance to test any other combination of browser/password manager yet).
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.
    ^ I should probably do this after most changes are approved

@rianadon rianadon changed the title Add input elements to page for password managers Add input elements to login page for password managers Jun 4, 2021
@bramkragten
Copy link
Member

I think this should be part of ha-auth-flow, we should check if the handler of the step is homeassistant and the step_id is init, and only add the input elements for that step.

We can then directly fill _stepData when the hidden form changes, and directly bind to _handleSubmit.

We should also sync the other way btw, so if you empty the rendered input, the hidden one should also be emptied.

@rianadon
Copy link
Contributor Author

rianadon commented Jun 5, 2021

Thanks for the suggestion and fast reply! It's definitely much cleaner integrating with ha-auth-flow. I'm still keeping the bulk of the code in it's own file because ha-auth-flow is so large.

I also figured this change can support the legacy_api_password and command_line handlers as well, so check for these in addition to homeassistant.

@rianadon
Copy link
Contributor Author

rianadon commented Jun 5, 2021

Two screenshots from testing with KeepassXC on Firefox:

The icon to fill in passwords doesn't change positions because I'm positioning the fields relative to the entire page. It would be nice to keep it aligned to the username field, but I'm not sure it's worth the amount of code to do that.

Hopefully everyone stores the correct password in their password manager.

@rianadon
Copy link
Contributor Author

rianadon commented Jun 9, 2021

@bramkragten I rewrote as a Lit component, but I have one linting error when firing a submit event so I can tell the auth-flow component to submit the credentials. Is there any better way of sending this event than the fireEvent call?

@bramkragten
Copy link
Member

@bramkragten I rewrote as a Lit component, but I have one linting error when firing a submit event so I can tell the auth-flow component to submit the credentials. Is there any better way of sending this event than the fireEvent call?

You need to add that event to HASSDomEvents:

  interface HASSDomEvents {
    "submit":undefined;
  }

Copy link
Member

@bramkragten bramkragten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good!

@rianadon
Copy link
Contributor Author

Thanks for the review and changes @bramkragten! I really appreciate all the time you've spent on this PR.

One small issue: when ha-auth-form receives a value-changed event, it expects the full form data to be sent (i.e both the username and password). So this change:
image
broke things, as now the password manager cannot fill out both the username and password simultaneously.

The _valueChanged method of ha-form sends the full form data but might be cleaner than what I had written.

@bramkragten
Copy link
Member

bramkragten commented Jun 15, 2021

My code should do the same as your code? 🤔 Am I missing something?

Oh I see, a timing issue... Because the 2nd change happens before this._stepData is updated...

@rianadon
Copy link
Contributor Author

@bramkragten is there anything left to revise here? It's working great after your changes.

@bramkragten bramkragten merged commit a4aba93 into home-assistant:dev Jun 24, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Jun 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants