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

Migrate dialogflow over to the new webhook component #17804

Merged
merged 5 commits into from Oct 28, 2018

Conversation

Projects
None yet
5 participants
@rohankapoorcom
Contributor

rohankapoorcom commented Oct 26, 2018

Description:

Migrating Dialogflow over to the webhook API so that it doesn't need an api password.

Breaking Change: Each instance of Home Assistant will now generate it's own unique webhook url for Dialogflow to use. One will need to be generated and provided to Dialogflow via the webhook integration to replace the existing url: https://myhome.duckdns.org/api/dialogflow?api_password=HA_PASSWORD

I followed the approach used in #16817 to implement a new config flow to register the webhook. Existing configuration (account_sid, auth_token) still occurs via the configuration.yaml file.

I do not actually use dialogflow myself so I cannot test the behavior with Dialogflow , but playing with the request JSON looks okay and all unit tests are passing.

Related issue (if applicable): Related to #15376

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

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 communicates with devices, web services, or third-party tools:

  • 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.
@@ -40,7 +40,8 @@ def context(self, request):
return Context(user_id=user.id)
def json(self, result, status_code=200, headers=None):
@staticmethod
def json(result, status_code=200, headers=None):

This comment has been minimized.

@rohankapoorcom

rohankapoorcom Oct 26, 2018

Contributor

I had to expose this and the json_message as static methods so that they could be accessed by things that don't implement HomeAssistantView. Not sure if that is acceptable or if it would be preferred to move them out of the class (within the same module)?

This comment has been minimized.

@balloob

balloob Oct 26, 2018

Member

We should not do this. This is conflation of concerns.

Webhooks should work without any http component related things.

@rohankapoorcom rohankapoorcom referenced this pull request Oct 26, 2018

Merged

Update Dialogflow with the webhook changes #7109

2 of 2 tasks complete
@rohankapoorcom

This comment has been minimized.

Contributor

rohankapoorcom commented Oct 26, 2018

This is a Breaking Change

try:
response = await async_handle_message(hass, message)
return b'' if response is None else HomeAssistantView.json(response)

This comment has been minimized.

@balloob

balloob Oct 26, 2018

Member

I don't think that it's a webhook when the caller expects a response.

This comment has been minimized.

@balloob

balloob Oct 26, 2018

Member

Forget about the comment above.

However, we should not use HomeAssistantView but instead use aiohttp directly.

from aiohttp import web

return web.json_response(…)

Docs

@balloob

This comment has been minimized.

Member

balloob commented Oct 26, 2018

https://dialogflow.com/docs/fulfillment/configure

DialogFlow supports setting headers so I suggest DialogFlow just uses an authenticated webview with a long lived access token.

@rohankapoorcom

This comment has been minimized.

Contributor

rohankapoorcom commented Oct 26, 2018

Makes sense. I'm going to convert this pull request over to support that then (and keep the async test changes)

@balloob

This comment has been minimized.

Member

balloob commented Oct 26, 2018

We can still use a config entry to tell people what url to use. We might even want to generate a new config flow helper for that?

@rohankapoorcom

This comment has been minimized.

Contributor

rohankapoorcom commented Oct 26, 2018

Sorry, I'm a little lost. Wouldn't they just be using the existing url? /api/dialogflow?

Or are you suggesting that we use the config entry to show them that and generate them a long lived access token? In that case I agree, a new config flow helper is probably a good idea.

I have one concern with using long lived tokens for things like this. They allow access to everything right? There's no scoping like the webhook component provides. Not a huge fan of using that here for security reasons if that's the correct understanding. If Dialog Flow is comprised, then a lot more damage can be done.

@balloob

This comment has been minimized.

Member

balloob commented Oct 26, 2018

that is a good point. We could use a simliar strategy for generating a url but still have that be registered as a webview, ie /api/dialogflow/zxcljkdsjlkdsljkcxljkq12312

@rohankapoorcom

This comment has been minimized.

Contributor

rohankapoorcom commented Oct 26, 2018

Yeah that makes sense. Couple of other thoughts here:

I'm not a huge fan of exposing the purpose of the webview and would prefer if the component name wasn't inside the url.

For people whitelisting specific urls to be publicly accessible (for example, I require client certificates for auth on the reverse proxy and whitelist /api/webhook/*), this makes a bunch of additional paths (1 per component with a webview). Are you okay with creating a generalized component (similar in construction to the webhook) to be used for all these things that need authenticated webviews.

We could call the component authenticated_webview and then build the urls similarly to the webhook component webview/zxcljkdsjlkdsljkcxljkq12312

@balloob

This comment has been minimized.

Member

balloob commented Oct 26, 2018

@rohankapoorcom I've been researching webhooks, and I think that you're right, we should allow responses. So let's stick with a webhook for dialog flow.

@rohankapoorcom rohankapoorcom referenced this pull request Oct 27, 2018

Closed

Allow webhooks that support GET #17782

3 of 3 tasks complete
@houndci-bot

Some files could not be reviewed due to errors:

Traceback (most recent call last):
Traceback (most recent call last):
  File "/home/linters/.local/bin/flake8", line 7, in 
    from flake8.main.cli import main
ModuleNotFoundError: No module named 'flake8'
@rohankapoorcom

This comment has been minimized.

Contributor

rohankapoorcom commented Oct 27, 2018

@balloob thanks for the feedback. Changes are committed and pushed up.

@balloob balloob merged commit 60080a5 into home-assistant:dev Oct 28, 2018

5 checks passed

Hound No violations found. Woof!
WIP Legacy commit status override — see details
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.05%) to 93.221%
Details
@balloob

This comment has been minimized.

Member

balloob commented Oct 28, 2018

Nice!

@wafflebot wafflebot bot removed the in progress label Oct 28, 2018

@rohankapoorcom rohankapoorcom deleted the rohankapoorcom:dialogflow-webhooks branch Oct 28, 2018

@balloob balloob referenced this pull request Nov 9, 2018

Merged

0.82 #18335

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