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

WIP - add support for multiple passwords #9980

Closed

Conversation

ttroy50
Copy link
Contributor

@ttroy50 ttroy50 commented Oct 19, 2017

I've adding basic support for multiple users and am looking for feedback / thoughts on the general idea.

In part it comes from the discussion from https://community.home-assistant.io/t/multiple-users-accounts/396. The basic structure of the user dict is based on some of the work from k-laus@fc18173. I only have a plaintext password but the dict could be expanded to later allow permissions or different password types.

I've no plan to add read / write / execute permissions as it would involve touching too much, but wanted a way to specify multiple passwords so that I can easily revoke one in the case the service using it is compromised. For example, If I'm using IFTTT and Stringify, each service can have it's own api_password and if one has a breach I can just revoke that password instead of having to change both. It could also be used to add a level of accountability for who triggered a service.

Description:

Related issue (if applicable): fixes #

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.github.io#<home-assistant.github.io PR number goes here>

Example entry for configuration.yaml (if applicable):

http:
  api_users: 
    ifttt:
      password: ifttt_password
    stringify:
      password: stringify_password

Checklist:

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

If the code communicates with devices, web services, or third-party tools:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.

@@ -162,6 +199,16 @@ def test_basic_auth_works(mock_api_client, caplog):
assert req.status == 200
assert const.URL_API in caplog.text

@asyncio.coroutine

Choose a reason for hiding this comment

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

expected 2 blank lines, found 1

@@ -162,6 +199,16 @@ def test_basic_auth_works(mock_api_client, caplog):
assert req.status == 200
assert const.URL_API in caplog.text

@asyncio.coroutine

Choose a reason for hiding this comment

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

expected 2 blank lines, found 1

@emlove
Copy link
Contributor

emlove commented Oct 19, 2017

High level impressions:

We should only validate user passwords when combined with a username. IMO allowing any of them to validate is violating the expectation of how the security will work. That of course means these won't be usable from the frontend UI. That being said, this will still give us the ability to create separate credentials for API integrations authenticated by querystring / headers, which is definitely a big first step and decent MVP.

That being said, I think this is a pretty reasonable approach, and it feels like a good base that is future-proof.

@balloob
Copy link
Member

balloob commented Oct 20, 2017

Although the general idea of this approach is ok, I think that we should make it even more future proof: let's add support for Auth providers and make the first provider just take in a dictionary of users.

homeassistant:
  auth:
    - provider: dict
      users:
        ifttt: ifttt_password

Notes:

  • Under the homeassistant key in the config because this is bigger than just http (potentially)
  • Auth providers should operate on username+password combo
  • Having auth as list so that an incoming user/pass could be matched against multiple providers
  • All auth providers should have async entry point only.

@balloob
Copy link
Member

balloob commented Oct 20, 2017

Django is good inspiration for this (docs). Maybe we can just adopt their API? (async style)

@balloob
Copy link
Member

balloob commented Nov 11, 2017

This PR seems to have gone stale. Closing it. You can reopen it when you're ready to finish it.

@balloob balloob closed this Nov 11, 2017
@home-assistant home-assistant locked and limited conversation to collaborators Mar 2, 2018
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

7 participants