Skip to content

Allow webhooks that support GET#17782

Closed
rohankapoorcom wants to merge 1 commit intohome-assistant:devfrom
rohankapoorcom:get-webhooks
Closed

Allow webhooks that support GET#17782
rohankapoorcom wants to merge 1 commit intohome-assistant:devfrom
rohankapoorcom:get-webhooks

Conversation

@rohankapoorcom
Copy link
Copy Markdown
Member

@rohankapoorcom rohankapoorcom commented Oct 25, 2018

Description:

Copying the description from my comment #15376 (comment):

I'm just starting migrating over the Prometheus component to an auto-generated webhook url and noticed that it (and several others) are using GET requests to scrape data from Home Assistant instead of POSTing data to Home Assistant. I'm going to extend the webhook component to support GET requests as well first.

Related issue (if applicable): relates to #15376

Checklist:

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

If the code does not interact with devices:

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

@rohankapoorcom
Copy link
Copy Markdown
Member Author

rohankapoorcom commented Oct 25, 2018

I'm not sure if the best approach is to allow each webhook to be used as GET/POST endpoint. I was considering another approach: changing the webhook schema to include an HTTP method in addition to the webhook_id but I think that unnecessarily over-complicates things and this is sufficient.

Thoughts?

@balloob
Copy link
Copy Markdown
Member

balloob commented Oct 25, 2018

Webhooks are not for returning data. If a component wants to expose an endpoint that returns data, they need to define a webview.

@balloob balloob closed this Oct 25, 2018
@ghost ghost removed the in progress label Oct 25, 2018
@rohankapoorcom rohankapoorcom deleted the get-webhooks branch October 26, 2018 04:10
@rohankapoorcom rohankapoorcom restored the get-webhooks branch October 27, 2018 09:41
@rohankapoorcom
Copy link
Copy Markdown
Member Author

rohankapoorcom commented Oct 27, 2018

@balloob in light of your latest comments on #15376 and #17804, I wanted to bring this one up again.

Here's my reasoning for wanting GET based webhooks:

  • There are some services example that allow GET requests to be used to send data. I would hazard a guess that there are likely some services that only allow GET requests to be used for webhooks (but I have not looked for them).
  • Similar to the discussion we had in Migrate dialogflow over to the new webhook component #17804, we have several components that provide data out to external services when their endpoints are hit (example). That behavior (sending a response) is not quite that of a traditional webhook, but is a scenario where it's still helpful to have a common url prefix (for whitelisting) like /api/webhook/* and avoiding the security implications of trusting third parties with a global long lived access token (when all they need is access to one specific url).

Again, I think we could solve this use case without using the webhook component (if we want to keep that component only for POST requests, by creating a new generalized component (similar to the webhook component) to be used for all these things that need an authenticated webview (but only for GET requests).

Personally, I think it's a better solution to allow this case to be handled by the webhook component itself, rather than having two similar components (with slightly similar url schemes) and possible confusion for other developers down the line.

Let me know what you think and how you want to move forward.

@balloob
Copy link
Copy Markdown
Member

balloob commented Oct 28, 2018

customer.io uses form encoded, so is POST ?

@balloob
Copy link
Copy Markdown
Member

balloob commented Oct 28, 2018

Webhooks should focus on getting data in, I don't want to increase the scope about getting data out just now.

@rohankapoorcom
Copy link
Copy Markdown
Member Author

Okay, sounds good. Let's work on getting all of the inbound webhooks migrated over to the new component and then we can circle back on the things that are using HomeAssistantViews to get data out.

@home-assistant home-assistant locked and limited conversation to collaborators Feb 5, 2019
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.

3 participants