Replace api_password in Camera.Push by dgomes · Pull Request #16339 · home-assistant/core

dgomes · 2018-08-31T23:14:56Z

Description:

Remove api_password, and changes to using access_token/user provided token accordingly to #15376 (comment)

This is a breaking change for most users pushing camera images through cURL scripts

Related issue (if applicable): #15376

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

Example entry for `configuration.yaml` (if applicable):

camera:
  - platform: push
    name: cam1
    timeout: 10
    cache: 5
    token: 12345678

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:

Documentation added/updated in home-assistant.github.io

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

New dependencies have been added to the REQUIREMENTS variable (example).
New dependencies are only imported inside functions that use them (example).
New or updated 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:

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

awarecan

Should add note in PR description regarding breaking changes

homeassistant/components/camera/push.py

awarecan · 2018-09-01T00:19:13Z

homeassistant/components/camera/push.py

    vol.Optional(CONF_TIMEOUT, default=timedelta(seconds=5)): vol.All(
        cv.time_period, cv.positive_timedelta),
    vol.Optional(CONF_IMAGE_FIELD, default='image'): cv.string,
+    vol.Required(CONF_TOKEN): vol.All(cv.string, vol.Length(min=8)),


Since you accept bearer token, CONF_TOKEN should be optional.

awarecan

Cannot find test case to test return HTTP status 401, i.e. wrong token or missed token scenario

awarecan · 2018-09-01T15:23:27Z

tests/components/camera/test_push.py

-    assert resp.status == 400
+    assert resp.status == 404
+
+    # wrong token but authenticated user


Did I missed something? How this be authenticated user? I didn't find the code to set Authorization header.

client always makes authenticated requests... how can I disable it ?

https://github.com/home-assistant/home-assistant/blob/a5d95dfbdcf0d9ca3da93f44e2cbb5d162ac3272/tests/components/http/test_auth.py#L158-L163

https://github.com/home-assistant/home-assistant/blob/a5d95dfbdcf0d9ca3da93f44e2cbb5d162ac3272/tests/components/conftest.py#L43-L44

dgomes · 2018-09-01T20:59:34Z

tests added 👍

pvizeli · 2018-09-03T10:41:51Z

Maybe we should wait until the long time tokens are implemented and we can switch to this common version. Otherwise it going to a edge case for handling auth.

dgomes · 2018-09-03T10:45:25Z

I'm all in favor of using long time tokens.

But I was under the impression from #15376 (comment) that long time tokens would not become an option and that the current api_password would be dropped soon.

balloob · 2018-09-04T08:52:14Z

homeassistant/components/camera/push.py

    vol.Optional(CONF_TIMEOUT, default=timedelta(seconds=5)): vol.All(
        cv.time_period, cv.positive_timedelta),
    vol.Optional(CONF_IMAGE_FIELD, default='image'): cv.string,
+    vol.Optional(CONF_TOKEN): vol.All(cv.string, vol.Length(min=8)),


This should be required

I made it optional, since the implementation allows the use of the normal authentication.

balloob · 2018-09-04T08:53:00Z

@pvizeli we don't want to use long lived access tokens for this because an access token would allow full access. For these one off access, just allowing an auth token to be hard coded is fine.

balloob · 2018-09-04T08:53:30Z

This is ok to merge once the token has been changed in the schema to be required.

dgomes · 2018-09-04T08:58:42Z

I see this PR as a stop-gap, what we need are long lived access tokens that are limited to given API's

The preferred authentication and authorisation method should be that of HA, and that is why I keep the option of uploading the camera images using HA auth.

balloob · 2018-09-04T09:13:40Z

homeassistant/components/camera/push.py

                          config[CONF_BUFFER_SIZE],
-                          config[CONF_TIMEOUT])]
+                          config[CONF_TIMEOUT],
+                          config[CONF_TOKEN])]


This expects the CONF_TOKEN to be always set, which is not the case as it can be optional.

balloob · 2018-09-04T09:14:19Z

homeassistant/components/camera/push.py

+                                     status)
+
+        authenticated = (request[KEY_AUTHENTICATED] or
+                         request.query.get('token') == _camera.token)


Not setting a token will result in token being None, so then not passing a token means we're checking None == None, allowing user to always access it.

balloob · 2018-09-11T09:30:35Z

Putting this in the 0.78 release as example how to do auth both ways.

As far as I can see, this is not a breaking change as people can still authenticate with API password via legacy API password provider, or even use new auth.

dgomes · 2018-09-11T09:41:18Z

It you don't remove legacy_api that is true :)

I was afraid that it was going out in 0.78 already...

balloob · 2018-09-11T15:41:41Z

Oh no, that beast will probably be around for a looooooooong time. Eventually it will become opt-in instead of opt-out.

balloob · 2018-09-11T15:41:53Z

But first we need long lived access tokens, which will come this release.

@awarecan

* Use access_token and user provided token instead of api_password * address comments by @awarecan * new tests * add extra checks and test * lint * add comment

Use access_token and user provided token instead of api_password

351fb72

homeassistant added cla-signed platform: camera.push labels Aug 31, 2018

ghost assigned dgomes Aug 31, 2018

ghost added the in progress label Aug 31, 2018

dgomes changed the title ~~Use access_token and user provided token instead of api_password~~ Replace api_password Aug 31, 2018

dgomes changed the title ~~Replace api_password~~ Replace api_password in Camera.Push Aug 31, 2018

dgomes mentioned this pull request Aug 31, 2018

Adds token parameter to Camera.Push home-assistant/home-assistant.io#6130

Merged

2 tasks

awarecan reviewed Sep 1, 2018

View reviewed changes

awarecan mentioned this pull request Sep 1, 2018

Use access_token instead api_password #15376

Closed

19 tasks

address comments by @awarecan

c57fab9

homeassistant added the cla-signed label Sep 1, 2018

dgomes added the breaking-change label Sep 1, 2018

awarecan requested changes Sep 1, 2018

View reviewed changes

dgomes removed the in progress label Sep 1, 2018

new tests

8eda035

homeassistant added the cla-signed label Sep 1, 2018

ghost added the in progress label Sep 1, 2018

awarecan approved these changes Sep 1, 2018

View reviewed changes

balloob reviewed Sep 4, 2018

View reviewed changes

oblogic7 mentioned this pull request Sep 9, 2018

Doorbird auth and event data #16504

Closed

8 tasks

add extra checks and test

42f370a

lint

ef507fc

homeassistant added the cla-signed label Sep 10, 2018

dgomes removed the in progress label Sep 10, 2018

dgomes closed this Sep 10, 2018

dgomes reopened this Sep 10, 2018

ghost added the in progress label Sep 10, 2018

add comment

413a93a

homeassistant added the cla-signed label Sep 10, 2018

balloob approved these changes Sep 11, 2018

View reviewed changes

balloob added this to the 0.78.0 milestone Sep 11, 2018

balloob merged commit 20f6cb7 into home-assistant:dev Sep 11, 2018

ghost removed the in progress label Sep 11, 2018

balloob removed the breaking-change label Sep 11, 2018

balloob added the cherry-picked label Sep 11, 2018

balloob pushed a commit that referenced this pull request Sep 11, 2018

Replace api_password in Camera.Push (#16339)

968809c

* Use access_token and user provided token instead of api_password * address comments by @awarecan * new tests * add extra checks and test * lint * add comment

balloob mentioned this pull request Sep 17, 2018

0.78.0 #16666

Merged

dgomes deleted the camera.push-token branch November 11, 2018 23:49

ghost added integration: push and removed platform: camera.push labels Mar 21, 2019

Uh oh!

Conversation

dgomes commented Aug 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Uh oh!

awarecan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

awarecan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgomes commented Sep 1, 2018

Uh oh!

pvizeli commented Sep 3, 2018

Uh oh!

dgomes commented Sep 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

balloob commented Sep 4, 2018

Uh oh!

balloob commented Sep 4, 2018

Uh oh!

dgomes commented Sep 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

balloob commented Sep 11, 2018

Uh oh!

dgomes commented Sep 11, 2018

Uh oh!

balloob commented Sep 11, 2018

Uh oh!

balloob commented Sep 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dgomes commented Aug 31, 2018 •

edited

Loading

Example entry for `configuration.yaml` (if applicable):