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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

XMPP HTTP upload #17426

Merged
merged 16 commits into from Nov 4, 2018

Conversation

@flowolf
Contributor

flowolf commented Oct 14, 2018

Description:

Add support for sending files through Jabber and HTTP upload (XEP_0363)
can load a file from a local file or a remote URL. e.g. images. 馃柤

Related issue (if applicable): none

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

Example entry for configuration.yaml (if applicable):

# Example configuration.yaml entry
notify:
  - name: NOTIFIER_NAME  # e.g. jabber
    platform: xmpp
    sender: YOUR_JID
    password: YOUR_JABBER_ACCOUNT_PASSWORD
    recipient: YOUR_RECIPIENT
    tls: true

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

flowolf added some commits Oct 13, 2018

@flowolf flowolf requested a review from fabaff as a code owner Oct 14, 2018

@wafflebot wafflebot bot added the in progress label Oct 14, 2018

@flowolf flowolf referenced this pull request Oct 14, 2018

Merged

Xmpp http upload #6788

2 of 2 tasks complete

flowolf added some commits Oct 15, 2018

catch more errors, allow unverified SSL request
allow user to specify unverified SSL request to URL
cleaner code
catch more exceptions
@fabaff

This comment has been minimized.

Member

fabaff commented Oct 15, 2018

It seems that all servers that I'm using do not support working with files/images.

Service call run without error.

{
  "message": "{{ now() }}",
  "title": "The sun is {% if is_state('sun.sun', 'above_horizon') %}up{% else %}down{% endif %}!",
  "data": {
        "url": "https://github.com/home-assistant/home-assistant.io/raw/next/source/images/favicon-192x192.png"
  }
}

According to the code the sending of the message and the file are two tasks. In my case the text message alone works but not if there is a file attached (as in the service call above). Shouldn't at least the message get through?

@fabaff

This comment has been minimized.

Member

fabaff commented Oct 15, 2018

Uff, after 10 min there is the error:

2018-10-15 23:26:37 INFO (MainThread) [homeassistant.components.notify.xmpp] getting file from https://github.com/home-assistant/home-assistant.io/raw/next/source/images/favicon-192x192.png
2018-10-15 23:37:32 ERROR (MainThread) [homeassistant.core] Error doing job: Task was destroyed but it is pending!
2018-10-15 23:37:32 ERROR (MainThread) [homeassistant.core] Error doing job: Future exception was never retrieved
slixmpp.exceptions.IqError: <iq xml:lang="en" to="notifications@jabber.me/home-assistant" from="dict.jabber.me" type="error" id="89220499-2768-4abd-b5dd-7f3aa7c74d49-23"><query xmlns="http://jabber.org/protocol/disco#info" /><error code="501" type="cancel"><feature-not-implemented xmlns="urn:ietf:params:xml:ns:xmpp-stanzas" /><text xmlns="urn:ietf:params:xml:ns:xmpp-stanzas">The feature requested is not implemented by the recipient or server and therefore cannot be processed.</text></error></iq>
@flowolf

This comment has been minimized.

Contributor

flowolf commented Oct 16, 2018

I'll have a look. Maybe I've to do extra service discovery earlier to see if the server supports it. Or maybe we can set a shorter timeout. I'm on it!

@thundergreen

This comment has been minimized.

thundergreen commented Oct 16, 2018

Woooow...that's awesome dude! I was praying for soooo long to get this feature.please be aware that the pic has to be wrapped into oob to make it compatible with conversations and other XMPP clients

We also need to know which server is used but normally if the component works according to the xep there shouldn't be any problem

catching XMPP exceptions, timeout for requests call
removed calls for roster and presence
added timeout for upload request call
cleared up debug, info, warning messages
cleared up requests call for secure and insecure retrieval of image
catching IqError, IqTimeout, XMPPError from slixmpp
docstring updated
@flowolf

This comment has been minimized.

Contributor

flowolf commented Oct 16, 2018

@fabaff I overlooked the slixmpp.exceptions.IqError. I'm catching that and others now.
Also I've added a timeout to the requests call (getting the external resource), defaults to 30 seconds.
Further the code now catches errors closer to the send call, so that the text part of a message is more likely to get trough, even if the image upload fails.
The code now catches:

  • wrong credentials
  • missing XEP_0363 on the server side
  • non-existing URL - via timeout
  • upload errors
  • SSL Exceptions
  • file access issues
    and others.

the XMPP lib checks on its own if the server supports XEP_0363, if not, an exception is caught and an error is logged.
there is a lot that can go wrong. I hope most of it is contained.

@thundergreen I know about OOB. The docs will document that the server needs to have XEP_0363 enabled: future-docs. Also I'm testing with Conversations. It would be great if you could test the code as well, with different clients when the beta is out; whenever this will be merged.

@thundergreen

This comment has been minimized.

thundergreen commented Oct 16, 2018

@flowolf I'll test it as soon as it is out. Not clear how to attach the picture ....with data? Afaik will the lib itself take care of oob and the rest ..I just wouldn't know how to implement this.

I'm really excited to test. Unfortunately I only use conversations...well more like a custom fork but in the core the same with all xmop standards included ..so testing would have same base.
'''

Example script.yaml entry

5_send_jabber_message_with_image_and_text:
alias: "Send Image and Text via Jabber"
sequence:
- service: notify.jabber
data:
title: "The Time is now"
message: " {{ now() }} , templating works as well..."
data:
url: "https://github.com/home-assistant/home-assistant.io/raw/next/source/images/favicon-192x192.png"
'''
This seems odd to me as u don't use data_template to catch states ...is that normal in scripts?

@flowolf

This comment has been minimized.

Contributor

flowolf commented Oct 16, 2018

@thundergreen the second data block is optional and refers to the image or file you want to send. maybe we should rename this to file?
the script shall just illustrate the structure of the data that is passed to the platform. @fabaff mentioned his JSON version above. You can pass the JSON into the services developer tools for the XMPP service as service data.

@thundergreen

This comment has been minimized.

thundergreen commented Oct 17, 2018

@flowolf not sure if it speaks against Hass architecture to rename it into file.if not I'd recommend that . Are u also looking into possibility to upload into Muc as I c eater a special Muc to receive messages ... Instead of setting up Multiple notifier .

Just out of curiosity..is it also possible to let the component look for latest file or having something like a regex or time range? I also was thinking about having a generated videofile of pics taken the whole day..but I guess that's another component

Edit: found folder watchdog

https://www.home-assistant.io/components/folder_watcher/

So let it to data..like this it is compliant to the other notification platforms

@flowolf

This comment has been minimized.

Contributor

flowolf commented Oct 21, 2018

I reproduced the problem. Currently when trying to send a file through jabber.sk, there are issues. It seems to be a library issue with slixmpp. I opened up an issue on their gitlab page, and am waiting for a response.

I checked with @thundergreen 's ejabberd, and it worked. My prosody server works fine as well.

The behavior with jabber.sk is that slixmpp does not handle the discovery correctly, does not see or react to the upload feature, and/or waits around for responses from servers. Not sure what goes on.

I opened another bug report, because working around the problem by specifying a timeout for the upload is fruitless.

timeout, mimetypes, random filenames
guessing filetypes and mimetypes with stdlib mimetypes
setting a random filename for privacy
working around slixmpp timeout issues with asyncio.wait_for
@thundergreen

This comment has been minimized.

thundergreen commented Oct 25, 2018

Is it possible to push this pr toi test ?

flowolf added some commits Oct 26, 2018

@flowolf

This comment has been minimized.

Contributor

flowolf commented Oct 26, 2018

I cleaned up the code, and added support for sending file/images to rooms.

the code is as feature complete as I can get it for HTTP-upload.
The issues that are present with jabber.sk, are handled with timeouts and logged as errors. Users should be able to see that their Jabber server does not work as intended.

@fabaff please have another look.

@thundergreen

This comment has been minimized.

thundergreen commented Oct 27, 2018

Sounds great .thanks for that.hope it will be merged into Dev branch

@flowolf

This comment has been minimized.

Contributor

flowolf commented Oct 27, 2018

there is a fix of some issues on the way in slixmpp. I'll integrate those with a new version.

version bump for slixmpp to 1.4.1
added NotConnectedError, removed double catches of IqErrors
removed asyncio import
@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'
@flowolf

This comment has been minimized.

Contributor

flowolf commented Oct 28, 2018

version 1.4.1 fixes the issues with jabber.sk. The server still is has issues of it's own (which are reported), but sending images will work.
Thanks to @mathieui and @linkmauve for their help and swift release of slixmpp 1.4.1.

@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'

@flowolf flowolf changed the title from Xmpp http upload to XMPP HTTP upload Oct 29, 2018

flowolf and others added some commits Oct 30, 2018

@fabaff fabaff closed this Nov 3, 2018

@fabaff fabaff reopened this Nov 3, 2018

@wafflebot wafflebot bot added in progress and removed in progress labels Nov 3, 2018

@amelchio

Looks good! I have added a few comments that you may want to address but nothing big.

Show resolved Hide resolved homeassistant/components/notify/xmpp.py Outdated
Show resolved Hide resolved homeassistant/components/notify/xmpp.py
Show resolved Hide resolved homeassistant/components/notify/xmpp.py Outdated
Show resolved Hide resolved homeassistant/components/notify/xmpp.py Outdated
Show resolved Hide resolved homeassistant/components/notify/xmpp.py Outdated
Show resolved Hide resolved homeassistant/components/notify/xmpp.py
Show resolved Hide resolved homeassistant/components/notify/xmpp.py
Show resolved Hide resolved homeassistant/components/notify/xmpp.py Outdated
fixed review requests
fixed possible path issue for foo/../bar/ paths
fixed possible access for non-whitelisted files
fixed None or X
fixed try-else block, moved else block into try
fixed raising error in upload_file if url is None
fixed using data.get after it's already been checked
fixed added docstring for tiny get_url function
@flowolf

This comment has been minimized.

Contributor

flowolf commented Nov 3, 2018

@amelchio @fabaff thanks for the reviews!

@fabaff fabaff merged commit 5418e05 into home-assistant:dev Nov 4, 2018

4 of 5 checks passed

coverage/coveralls Coverage decreased (-0.5%) to 93.062%
Details
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 4, 2018

@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