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

By default to use access_token if hass.auth.active #15212

Merged
merged 6 commits into from
Jul 1, 2018

Conversation

awarecan
Copy link
Contributor

@awarecan awarecan commented Jun 29, 2018

Description:

Force to use access_token authorization if hass.auth.active (e.g. has auth_provider configured)
User can choose to still use api_password, if load legacy_api_password auth provider.

Breaking Changes: api_password still allowed in configuration.xml, but as long as hass.auth.active==True, it would not be used for authorization. All existing web-hook and web-socket based integration will break.
Workaround: load legacy_api_password auth provider to continue use api_password

Related issue (if applicable): fixes #15191
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):

# configuraiton.yaml
homeassistant:
  auth_providers:
    - type: homeassistant
    - type: legacy_api_password  # enable legacy api_password support

auth:

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

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.

auth=BasicAuth('homeassistant', API_PASSWORD))
assert req.status == 401


Choose a reason for hiding this comment

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

blank line at end of file

# A valid auth header has been set
authenticated = True

elif (DATA_API_PASSWORD in request.query and
elif (legacy_auth and api_password and DATA_API_PASSWORD in request.query and

Choose a reason for hiding this comment

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

line too long (85 > 79 characters)

@awarecan awarecan force-pushed the disable-api-password branch from 7d7fe28 to c3a473e Compare June 29, 2018 07:29
@awarecan awarecan force-pushed the disable-api-password branch from 9e6f59f to 6cb0659 Compare June 29, 2018 07:42
@awarecan awarecan changed the title Force to use access_token if hass.auth.active WIP: Force to use access_token if hass.auth.active Jun 29, 2018
@awarecan awarecan changed the title WIP: Force to use access_token if hass.auth.active Force to use access_token if hass.auth.active Jun 29, 2018
@pvizeli
Copy link
Member

pvizeli commented Jun 29, 2018

This break all Hass.io devices. We can this merge after we have a exchange of API keys for Hass.io integrated otherwise we hit a lot of device.

If we want go this way, we need register an API key on hassio component and exchange this to supervisor on startup.

EDIT:
We can also bypass the communication between supervisor and homeassistant. All Add-ons need to be a token from supervisor to speak with home-assistant over a internal proxy.

@balloob
Copy link
Member

balloob commented Jun 29, 2018

@pvizeli the user system is still an opt-in feature. But okay, let's merge this one after the RC has been cut.

Writing up an issue #15220

@pvizeli
Copy link
Member

pvizeli commented Jun 30, 2018

I'm fine to merge it, if this PR bypass the supervisor communication in case of disable the api_password or if they is a hassio envoerement, you can't enable auth.

Otherwise we need wait until the auth system is on that level that he work with supervisor together.

@pvizeli
Copy link
Member

pvizeli commented Jun 30, 2018

We can also make a patch they strip that out for docker images and I apply it on build time to it. But that mean we run not the same Home-Assistant base on docker.

Other question is, we have a lot ecosystems around Home Assistant and a new, not ready to use, auth system. After this PR, people need decide in which part of splitted ecosystem want stay. The new cool user system with auth provider and all others don't work anymore or on old login system with working ecosystem. I speak not about Add-ons and Hass.io / They are not affected while the supervisor handle all the auth handling. That is "simple" to change if the auth system can handle it.

Normally is the flow to finish a system and if they is production ready, it breaks to old implementation. The code of @awarecan is good. But I think we run the wrong order and set the wrong priority. I think the correct flow is: finish auth - anouncement that all ecosystem will break soon and need implement the new one - stop the compatibility to old. Actual we have no real solutions for any scripting interface.

@pvizeli
Copy link
Member

pvizeli commented Jun 30, 2018

I approve it. Code is okay. @balloob it's your decision if you want break any script and ecosystem integration with new auth system at this point. I think the auth system needs also token system like GitHub for scripts and there more important work as to breaking it to old integration.

I love the auth system but it is not at the point that's sysadmin can work with it. For a developer it's okay.

@awarecan
Copy link
Contributor Author

How about we add a config allow user continue to use api_password while auth.active. Default set it to false, so that user have to "open a hole" by himself. Then we phase out this option when the auth system is production ready.

@awarecan awarecan force-pushed the disable-api-password branch from 4ce0b5f to 983f79b Compare June 30, 2018 22:27
@awarecan awarecan changed the title Force to use access_token if hass.auth.active By default to use access_token if hass.auth.active Jun 30, 2018
@awarecan
Copy link
Contributor Author

Add a legacy_api_password auth provider to allow user live in old world.

@balloob
Copy link
Member

balloob commented Jul 1, 2018

We should not add a legacy auth provider, as it will still force scripts to use a refresh token to get an access token every 30 minutes.

Instead, I had written up #15195 earlier. It is to create a refresh token with a long lived access token, which will allow users to make old ecosystem work with minor changes (sent authorization: bearer <access token> instead of x-ha-auth)

@balloob
Copy link
Member

balloob commented Jul 1, 2018

I've upgraded issue #15195 to p1, making sure it is part of our initial release of auth.

@balloob
Copy link
Member

balloob commented Jul 1, 2018

Oh I see your implementation now, that's actually pretty nice.

@balloob balloob merged commit f874efb into home-assistant:dev Jul 1, 2018
@ghost ghost removed the in progress label Jul 1, 2018
@pvizeli
Copy link
Member

pvizeli commented Jul 1, 2018

@awarecan that is only for a time frame until all ecosystem support the new one.

@balloob balloob added this to the 0.73 milestone Jul 2, 2018
balloob pushed a commit that referenced this pull request Jul 2, 2018
* Force to use access_token if hass.auth.active

* Not allow Basic auth with api_password if hass.auth.active

* Block websocket api_password auth when hass.auth.active

* Add legacy_api_password auth provider

* lint

* lint
@balloob balloob mentioned this pull request Jul 6, 2018
@awarecan awarecan deleted the disable-api-password branch July 17, 2018 18:01
girlpunk pushed a commit to girlpunk/home-assistant that referenced this pull request Sep 4, 2018
)

* Force to use access_token if hass.auth.active

* Not allow Basic auth with api_password if hass.auth.active

* Block websocket api_password auth when hass.auth.active

* Add legacy_api_password auth provider

* lint

* lint
@home-assistant home-assistant locked and limited conversation to collaborators Dec 10, 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.

Do not allow authentication with api_password when using auth providers
5 participants