Skip to content

Allow webhooks that support GET#19648

Closed
rohankapoorcom wants to merge 4 commits intohome-assistant:devfrom
rohankapoorcom:get-webhooks
Closed

Allow webhooks that support GET#19648
rohankapoorcom wants to merge 4 commits intohome-assistant:devfrom
rohankapoorcom:get-webhooks

Conversation

@rohankapoorcom
Copy link
Copy Markdown
Member

@rohankapoorcom rohankapoorcom commented Dec 29, 2018

Description:

We talked about this once before in #17782 and at the time it didn't make sense. Since then, we've run into at least one device (#19523) where the device only can make GET calls. Rather than making it configurable on a per webhook basis, I think it makes sense for all webhooks to respond with the same behavior whether it's a GET or POST call.

Related issue (if applicable): fixes #19523

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#<home-assistant.io PR number goes here>

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

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.
  • 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.



async def test_getting_webhook_nonexisting(hass, mock_client):
"""Test posting to a nonexisting webhook."""
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Stale docstring.



async def test_getting_webhook(hass, mock_client):
"""Test posting a webhook with no data."""
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Stale docstring.

@pvizeli
Copy link
Copy Markdown
Member

pvizeli commented Dec 31, 2018

Per definition is a webhook POST only. A Rest API support also GET but is not a webhook.

@andre2007
Copy link
Copy Markdown

andre2007 commented Dec 31, 2018

While that is technically true, you miss two use cases:

  • some devices manufacturer sends webhooks only using GET requests. I would like to switch on some lights by a movement sensor, but I can't.

  • if you have a NFC capable mobile phone, you can program rfid cards. If you just hold your mobile phone near the rfid cards an url saved on the rfid card will be opened using a get request.

The workaround would be to install a proxy server on the raspberry device which transforms get requests to post requests. But that seems pretty unnecessary...

@pvizeli
Copy link
Copy Markdown
Member

pvizeli commented Jan 1, 2019

@andre2007 yeah that are RESTful calls and not webhooks calls. The question is now, do we want at a hack to webhook API to support RESTful API calls? We have already a RESTful interface on Home Assistant, but the webhook part feels a bit more integrated because you can easily use it in automation or with config flow.

@rohankapoorcom
Copy link
Copy Markdown
Member Author

@pvizeli another option could be that we make another component (like webhook) that supports RESTful API calls (realistically only GET) to make it as integrated with automations and the config flow but that seems like a lot of duplicated code to maintain two separate components and confusion for users when to use which one.

My opinion is that hacking it on top of the webhook component seems better integrated and cleaner in the longer term.

@pvizeli
Copy link
Copy Markdown
Member

pvizeli commented Jan 1, 2019

If we create a RESTful version, we need to adopt the standards, and they allow more as only GET with becomes the URL also an important role.

@andre2007
Copy link
Copy Markdown

It seems a quite heavy artificial restriction that the official definition of webhooks only allows POST requests.
From a user perspective who just know how to enter an URL into a web browser this can't be understood.

Maybe we do not call it a hack but "Smart webhooks".

Even a third GET scenario: from the web browser of my mobile phone I open a webhook url to trigger some action.
I do not want to install a postman app on my mobile phone because web hooks only accept POST requests.

@andre2007
Copy link
Copy Markdown

Another advantage of enhancing the webhooks with GET, they do not need authentication.

In the portal of my device I can only set an Action URL for events, but no additional data, like authentication. The device manufacturer provides here a possibility to set a webhook URL but he calls the URL using GET.
I contacted the device manufacturer a week ago, but no answer so far.

@andre2007
Copy link
Copy Markdown

@rohankapoorcom what is missing in this pull request, I noticed there is a tag "In progress"?

@rohankapoorcom
Copy link
Copy Markdown
Member Author

@andre2007 I'm not sure about the tag. I believe it's set automatically and I cannot change it.

This PR is waiting on nothing from me, it needs approval to be merged by the maintainers.

@balloob
Copy link
Copy Markdown
Member

balloob commented Jan 7, 2019

We should not do GET. From my earlier comment to one of the linked PRs/issues:

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

We're not going to change the way we do things to solve 1 use case.

@balloob balloob closed this Jan 7, 2019
@ghost ghost removed the in progress label Jan 7, 2019
@andre2007
Copy link
Copy Markdown

@balloob

Could you please at least provide me a workaround for my problem? The device manufacturer just let me enter an webhook URL which will be called using get. How can I use this as event similar to webhooks?

@escoand escoand mentioned this pull request Jul 16, 2019
9 tasks
@MartinHjelmare MartinHjelmare mentioned this pull request Sep 22, 2021
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

webhook: make http verb configurable

6 participants