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

Switch OwnTracks HTTP to use webhook component #17034

Merged
merged 3 commits into from Nov 6, 2018

Conversation

@kirichkov
Contributor

kirichkov commented Oct 1, 2018

Description:

The purpose is to update owntracks to work with the new auth system and to remove the dependency on legacy_api_password.

Following the discussion in #15376 my understanding is that components that do not support OAuth2 should implement their own authentication solution, so here I see two possibilities:

  1. Use token in the URL setting of the OwnTracks app, similar to the Camera Push, or
  2. Implement a Basic authentication, which the OwnTracks app supports.

My personal preference would be option 2. but I haven't tried to implement it yet, so I need some feedback on whether it's OK to implement Basic Auth in the Owntracks HTTP component, or should it be made to work with a simple token setting, as the proposed change?

I haven't written any tests yet, since this approach might not be desirable

Related issue (if applicable): Partial fix for #15376

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

Example entry for configuration.yaml (if applicable):

device_tracker:
     - platform: owntracks_http
       webhook_id: long-randomly-generated-string

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox.

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

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.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.
if not authenticated:
return self.json_message(
'Invalid authorization credentials for {}'.format(entity_id),
HTTP_UNAUTHORIZED)

This comment has been minimized.

@houndci-bot

houndci-bot Oct 1, 2018

undefined name 'HTTP_UNAUTHORIZED'

if not authenticated:
return self.json_message(
'Invalid authorization credentials for {}'.format(entity_id),

This comment has been minimized.

@houndci-bot

houndci-bot Oct 1, 2018

undefined name 'entity_id'

@kirichkov kirichkov force-pushed the kirichkov:owntracks-new-auth branch from d2bca47 to 6220440 Oct 1, 2018

@balloob

This comment has been minimized.

Member

balloob commented Oct 1, 2018

We should migrate it to use the new webhook component. Example IFTTT component using webhook component: https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/components/ifttt/__init__.py#L77-L88

@kirichkov

This comment has been minimized.

Contributor

kirichkov commented Oct 1, 2018

OK. I'll change it in that direction. Thanks for the advice!

@bachya bachya added the Hacktoberfest label Oct 1, 2018

@kirichkov kirichkov force-pushed the kirichkov:owntracks-new-auth branch from 6220440 to ecb9944 Oct 4, 2018

@kirichkov kirichkov requested a review from home-assistant/core as a code owner Oct 4, 2018

description_placeholders={
'webhook_url': webhook_url,
'docs_url':
'https://www.home-assistant.io/components/device_tracker.owntracks_http/'

This comment has been minimized.

@houndci-bot

houndci-bot Oct 4, 2018

line too long (89 > 79 characters)

hass.bus.fire(EVENT_RECEIVED, message)
async def async_setup_entry(hass, entry):

This comment has been minimized.

@houndci-bot

houndci-bot Oct 4, 2018

expected 2 blank lines, found 1

elif 'd' in request.query:
message['device'] = request.query['d']
else:
return self.json({'error': 'You need to supply device name. Use ?d=DEVICENAME after the WebHook URL'})

This comment has been minimized.

@houndci-bot

houndci-bot Oct 4, 2018

undefined name 'self'
line too long (110 > 79 characters)

message['user'] = request.query['u']
else:
return self.json({ 'error': 'You need to supply username. Use ?u=USERNAME after the WebHook URL' })

This comment has been minimized.

@houndci-bot

houndci-bot Oct 4, 2018

blank line contains whitespace

elif 'u' in request.query:
message['user'] = request.query['u']
else:
return self.json({ 'error': 'You need to supply username. Use ?u=USERNAME after the WebHook URL' })

This comment has been minimized.

@houndci-bot

houndci-bot Oct 4, 2018

undefined name 'self'
whitespace after '{'
line too long (107 > 79 characters)
whitespace before '}'

"""Set up the OwnTracks HTTP service component."""
return True
async def async_setup_scanner(hass, config, async_see, discovery_info=None):

This comment has been minimized.

@houndci-bot

houndci-bot Oct 4, 2018

expected 2 blank lines, found 1

from homeassistant.components.device_tracker.owntracks import ( # NOQA
PLATFORM_SCHEMA, REQUIREMENTS, async_handle_message, context_from_config)
import homeassistant.helpers.config_validation as cv
from homeassistant.util.network import is_local

This comment has been minimized.

@houndci-bot

houndci-bot Oct 4, 2018

'homeassistant.util.network.is_local' imported but unused

from homeassistant import config_entries
from homeassistant.components.device_tracker.owntracks import ( # NOQA
PLATFORM_SCHEMA, REQUIREMENTS, async_handle_message, context_from_config)
import homeassistant.helpers.config_validation as cv

This comment has been minimized.

@houndci-bot

houndci-bot Oct 4, 2018

'homeassistant.helpers.config_validation as cv' imported but unused

import pdb
import re
import voluptuous as vol

This comment has been minimized.

@houndci-bot

houndci-bot Oct 4, 2018

'voluptuous as vol' imported but unused

"""
import json
import logging
import pdb

This comment has been minimized.

@houndci-bot

houndci-bot Oct 4, 2018

'pdb' imported but unused

@@ -145,6 +145,7 @@ class ExampleConfigFlow(config_entries.ConfigFlow):
'mqtt',
'nest',
'openuv',
'device_tracker.owntracks_http',

This comment has been minimized.

@balloob

balloob Oct 5, 2018

Member

This is not supported.

This comment has been minimized.

@kirichkov

kirichkov Oct 5, 2018

Contributor

In the sense that it’s not working currently, or that I can’t use a ConfigFlow for the platform (assuming because of the dot)?

This comment has been minimized.

@balloob

balloob Oct 8, 2018

Member

We don't support config flows for platforms. It might work but that would be by accident.

@kirichkov kirichkov force-pushed the kirichkov:owntracks-new-auth branch from 419eda1 to de0e440 Oct 13, 2018

@kirichkov

This comment has been minimized.

Contributor

kirichkov commented Oct 13, 2018

I've replaced the config flow with a static webhook_id set in config, however in this new implementation, I think the communication between the webhook and the code is unidirectional, and it will return a 200 status even if there was an error in the owntracks code and that won't be returned to the OwnTracks app.

I tried to make a communication between handle_webhook and handle_event using events, but that got pretty messy, so I scrapped it. Maybe I missed something in the docs, but I don't see any other way for communication between the components other than the event bus?

@balloob

This comment has been minimized.

Member

balloob commented Oct 15, 2018

use helpers/dispatcher.py

@balloob

This comment has been minimized.

Member

balloob commented Oct 15, 2018

Can you rename it back from __init__.py to owntracks_http.py ?

@kirichkov kirichkov force-pushed the kirichkov:owntracks-new-auth branch from 866c3ce to ccec3d6 Oct 16, 2018

@kirichkov kirichkov referenced this pull request Oct 16, 2018

Merged

Update documentation for device_tracker.owntracks_http #6863

2 of 2 tasks complete

@kirichkov kirichkov changed the title from [WIP] Remove dependency of OwnTracks HTTP to legacy_api_password to Switch OwnTracks HTTP to use webhook component Oct 16, 2018

response = {'body': json.dumps({'error': 'Error parsing JSON'}),
'status': 500}
async_dispatcher_send(hass, EVENT_RESPONSE + str(message['tst']),

This comment has been minimized.

@balloob

balloob Oct 16, 2018

Member

I don't see any reason why this needs to use a dispatcher. It should all be done inside the webhook handler.

This comment has been minimized.

@kirichkov

kirichkov Oct 16, 2018

Contributor

I can't find a way to access async_see and config from the webhook handler to initialize the context in https://github.com/home-assistant/home-assistant/pull/17034/files/ccec3d6a60ac14f4af9e9854f1a378cfed103c1b#diff-8cd102f735f57c74c7f9ebe438dce2e7R46

Without the owntracks context I can't get the MQTT topic. Maybe I'm missing something and I can extract that data from the hass object directly, as I dislike this back-and-forth between the callbacks too.

This comment has been minimized.

@balloob

balloob Oct 16, 2018

Member

Use partial: from functools import partial

@kirichkov

This comment has been minimized.

Contributor

kirichkov commented Oct 17, 2018

I completely refactored the code. I don't know why I thought I had to keep the webhook handler outside of the setup scanner and can't just access the variables directly. I don't think using partials will be necessary.

'homematicip_cloud',
'hue',
'ifttt',
'ios',
'lifx',

This comment has been minimized.

@balloob

balloob Oct 18, 2018

Member

Please revert.

@@ -1,60 +0,0 @@
"""Test the owntracks_http platform."""

This comment has been minimized.

@balloob

balloob Oct 18, 2018

Member

Don't delete the tests 😱

You will need to fix them

async def async_setup(hass, config):

This comment has been minimized.

@balloob

balloob Oct 18, 2018

Member

Why did you add this?

except ValueError:
raise HTTPInternalServerError
return Response(body=json.dumps({'error': 'Error parsing JSON'}),

This comment has been minimized.

@balloob

balloob Oct 18, 2018

Member

We should log an error but just return None. Webhooks are unauthenticated and we don't want to give away that they hit a valid webhook.

This comment has been minimized.

@kirichkov

kirichkov Oct 19, 2018

Contributor

As a matter of fact, if noone specifies u=USERNAME&d=DEVICE as part of the webhook URL this line would never be hit, instead the "Provide username" error will be returned. I've adjusted it nevertheless.

The provide username error is cryptic enough and doesn't reveal that the username should be provided as part of the webhook query string.

This comment has been minimized.

@balloob

balloob Nov 6, 2018

Member

good point.

@kirichkov kirichkov force-pushed the kirichkov:owntracks-new-auth branch from 67e904c to d90bc28 Oct 19, 2018

@balloob

balloob approved these changes Nov 6, 2018

@balloob

This comment has been minimized.

Member

balloob commented Nov 6, 2018

Looks great ! Missing docs.

@kirichkov

This comment has been minimized.

Contributor

kirichkov commented Nov 6, 2018

Looks great ! Missing docs.

If you mean updating the ones for the website they're ready, awaiting the approval of the code changes -
home-assistant/home-assistant.io#6863

@balloob

This comment has been minimized.

Member

balloob commented Nov 6, 2018

oh my bad ! perfect. Merge when Travis is 👍

@balloob

This comment has been minimized.

Member

balloob commented Nov 6, 2018

I might look into converting owntracks into a component before the next release and use our new webhook helper.

@kirichkov

This comment has been minimized.

Contributor

kirichkov commented Nov 6, 2018

That sounds good. I have the idea to implement the "friends" feature for the HTTP component, but wanted to get this through before moving forward with it. I found out HTTP is much milder on the battery than MQTT is.

P.S. I do not think I can merge this :-)

@balloob

This comment has been minimized.

Member

balloob commented Nov 6, 2018

Yeah OwnTracks HTTP is the bomb 👍

For friend feature, it would be nice if we had Persons

@kirichkov

This comment has been minimized.

Contributor

kirichkov commented Nov 6, 2018

For friend feature, it would be nice if we had Persons

I'll look into this! I'm currently implementing "Persons" using Groups :-)

@balloob balloob merged commit eb38551 into home-assistant:dev Nov 6, 2018

4 checks passed

Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wafflebot wafflebot bot removed the in progress label Nov 6, 2018

@kirichkov kirichkov deleted the kirichkov:owntracks-new-auth branch Nov 6, 2018

zxdavb added a commit to zxdavb/home-assistant that referenced this pull request Nov 13, 2018

Switch OwnTracks HTTP to use webhook component (home-assistant#17034)
* Update OwnTracks_HTTP to use the webhook component

* Update owntracks_http.py

* Update owntracks_http.py

@balloob balloob referenced this pull request Nov 29, 2018

Merged

0.83 #18776

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment