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

Added command_line auth provider that validates credentials by calling a command #19985

Merged
merged 15 commits into from Feb 7, 2019

Conversation

bob1de
Copy link
Contributor

@bob1de bob1de commented Jan 11, 2019

Description:

A new authentication provider that checks username/password by calling an external program, passing the values as environment variables. If the program exits with exit code 0, authentication succeeds.

Additionally, the program can print out lines of the form

KEY=VALUE

to give HA some information about the authenticating user. Currently, only name is supported, but groups are thinkable as well when there will be an official API for interfacing with HA's group system in the future.

As an example, I wrote a script to do authentication against LDAP, that I'll link here soon.

Related issue (if applicable): Closes #19975

Pull request in home-assistant-polymer: home-assistant/frontend#2561

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#8321

Example entry for configuration.yaml (if applicable):

homeassistant:
  auth_providers:
  - type: command_line
    command: /config/auth.sh
    # Optionally, a list of arguments.
    #args: []
    # Optionally, accept meta variables.
    #meta: true

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

If user exposed functionality or configuration variables are added/changed:

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

Copy link
Contributor

@awarecan awarecan left a comment

Choose a reason for hiding this comment

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

Please add unit test.

You can include a script to mock external program in your unit test to return valid or invalid result

homeassistant/auth/providers/command_line.py Outdated Show resolved Hide resolved
@bob1de
Copy link
Contributor Author

bob1de commented Jan 12, 2019

I'll create an unit test when the API is final.

@balloob
Copy link
Member

balloob commented Jan 12, 2019

This looks good. Will need some tests written.

@balloob
Copy link
Member

balloob commented Jan 12, 2019

I suggest that for MVP we remove the metadata and just look at the return code. Keep it simple.

@bob1de
Copy link
Contributor Author

bob1de commented Jan 12, 2019

Ah I see, MVP... Removing the metadata sounds not desirable to me. I don't want to see "You're logged in as ." in the UI because HA doesn't know my name. Could become more problematic when HA maybe logs user actions some day.

Having to turn it on explicitly sounds more appropriate to me.

@bob1de bob1de changed the title Added external auth provider that validates credentials by calling an external program Added command_line auth provider that validates credentials by calling a command Jan 12, 2019
@bob1de
Copy link
Contributor Author

bob1de commented Jan 12, 2019

And here's a working example for authentication against LDAP:

https://github.com/efficiosoft/ldap-auth-sh

@frenck
Copy link
Member

frenck commented Jan 14, 2019

Could not find a matching documentation PR on our documentation repository, adding docs-missing label.

Copy link
Contributor

@awarecan awarecan left a comment

Choose a reason for hiding this comment

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

There are two stuffs are missing besides the documentation pointed by Frenck

homeassistant/auth/providers/command_line.py Outdated Show resolved Hide resolved
homeassistant/auth/providers/command_line.py Show resolved Hide resolved
homeassistant/auth/providers/command_line.py Show resolved Hide resolved
homeassistant/auth/providers/command_line.py Outdated Show resolved Hide resolved
@bob1de
Copy link
Contributor Author

bob1de commented Jan 14, 2019

@frenck and @awarecan

I won't create any docs until we mutually agreed on the API, won't do it twice when it needs to be changed. Are you happy with meta being optional and False by default?

I'll look into testing the login flow as well... Just took the existing tests from insecure_example, which didn't include the login flow.

Strings for the frontend... alright.

@awarecan
Copy link
Contributor

I am fine with meta being optional and False by default

@awarecan
Copy link
Contributor

bob1de pushed a commit to bob1de/home-assistant.io that referenced this pull request Jan 27, 2019
@klaasnicolaas
Copy link
Member

@bob1de
Copy link
Contributor Author

bob1de commented Jan 29, 2019

I think this PR is ready for final review and merge now.

Copy link
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

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

It looks good 🎉 Just one question about the username stripping. Probably easiest to implement the username stripping in the config flow btw.

@bob1de
Copy link
Contributor Author

bob1de commented Jan 29, 2019

@balloob Hmm, I'm not sure I understand correctly. Usernames aren't stripped by this auth provider, as with any other existing provider I think.

@bob1de
Copy link
Contributor Author

bob1de commented Jan 29, 2019

The only things that are stripped are the meta variables(currently only the user's real name).

@awarecan
Copy link
Contributor

Balloob is referencing a recent merged PR, #20150

@balloob
Copy link
Member

balloob commented Jan 30, 2019

If a user enters "hello ", and the next time it enters "hello", the command line will probably see it as the same (or not?), the credentials are matched on user input, not on what script understood of it being.

@bob1de
Copy link
Contributor Author

bob1de commented Jan 30, 2019

No, it won't. Environment variables preserve all characters. However, it depends on the script the user configures to run to not strip them itself. I can add a note about that to the docs.

@bob1de
Copy link
Contributor Author

bob1de commented Jan 30, 2019

Did it.

@bob1de
Copy link
Contributor Author

bob1de commented Jan 30, 2019

On the other hand, I see nothing that stands against stripping usernames from the beginning on. So do you think it should simply be done before it get's released to prevent having a breaking change later?

@bob1de
Copy link
Contributor Author

bob1de commented Feb 4, 2019

@balloob Considering the modernized homeassistant auth provider from #20150, I'd probably start stripping usernames with the command_line auth provider from the beginning. Will change it in code & docs.

@balloob
Copy link
Member

balloob commented Feb 7, 2019

Niiiiiice 🎉

@balloob balloob merged commit 06f3e81 into home-assistant:dev Feb 7, 2019
@ghost ghost removed the in progress label Feb 7, 2019
balloob pushed a commit to home-assistant/frontend that referenced this pull request Feb 7, 2019
* Added strings for command line auth provider

Regards home-assistant/core#19985

* Reuse existing translation keys for new command_line auth provider
balloob pushed a commit to home-assistant/home-assistant.io that referenced this pull request Feb 7, 2019
* Added docs for command line auth provider

Regards home-assistant/core#19985

* Added reference to a compatible script for LDAP authentication

* Added note about stderr with command_line auth provider

* Added note about stripping usernames

* Note that usernames are stripped with command_line auth provider
@bob1de bob1de deleted the external-auth-provider branch February 7, 2019 21:14
bachya pushed a commit to bachya/home-assistant that referenced this pull request Feb 7, 2019
…g a command (home-assistant#19985)

* Added external auth provider that calls a configurable program

Closes home-assistant#19975

* Raise proper InvalidAuth exception on OSError during program execution

* Changed name of external auth provider to command_line

* Renamed program config option to command in command_line auth provider

* Made meta variable parsing in command_line auth provider optional

* Added tests for command_line auth provider

* Fixed indentation

* Suppressed wrong pylint warning

* Fixed linting

* Added test for command line auth provider login flow

* Log error when user fails authentication

* Use %r formatter instead of explicit repr()

* Mix all used names of typing module into module namespace

I consider this nasty and bad coding style, but was requested by
@awarecan for consistency with the remaining codebase.

* Small code style change

* Strip usernames with command_line auth provider
@balloob balloob mentioned this pull request Feb 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants