Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix Notifications for Android TV #10798

Merged
merged 9 commits into from
Dec 3, 2017
Merged

Fix Notifications for Android TV #10798

Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
98bec40
Fixed icon path, added dynamic icon
danielperna84 Nov 25, 2017
a92374b
Addressing requested changes
danielperna84 Nov 25, 2017
7ef6019
Using DEFAULT_ICON
danielperna84 Nov 25, 2017
f87560a
Using CONF_ICON from const
danielperna84 Nov 26, 2017
56a3de9
Getting hass_frontend path via import
danielperna84 Nov 27, 2017
5e5ba26
Lint
danielperna84 Nov 27, 2017
a459203
Using embedded 1px transparent icon
danielperna84 Dec 3, 2017
4df8316
woof -.-
danielperna84 Dec 3, 2017
eceb4c8
Lint
danielperna84 Dec 3, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 37 additions & 11 deletions homeassistant/components/notify/nfandroidtv.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
from homeassistant.components.notify import (
ATTR_TITLE, ATTR_TITLE_DEFAULT, ATTR_DATA, BaseNotificationService,
PLATFORM_SCHEMA)
from homeassistant.const import CONF_TIMEOUT
from homeassistant.const import CONF_TIMEOUT, CONF_ICON
Copy link

Choose a reason for hiding this comment

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

'homeassistant.const.CONF_ICON' imported but unused

import homeassistant.helpers.config_validation as cv

_LOGGER = logging.getLogger(__name__)
Expand All @@ -31,13 +31,16 @@
DEFAULT_COLOR = 'grey'
DEFAULT_INTERRUPT = False
DEFAULT_TIMEOUT = 5
DEFAULT_ICON = None
Copy link
Member

Choose a reason for hiding this comment

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

This isn't used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching this. Now it is being used. :)


ATTR_DURATION = 'duration'
ATTR_POSITION = 'position'
ATTR_TRANSPARENCY = 'transparency'
ATTR_COLOR = 'color'
ATTR_BKGCOLOR = 'bkgcolor'
ATTR_INTERRUPT = 'interrupt'
ATTR_ICON = 'icon'
ATTR_FILENAME = 'filename'

POSITIONS = {
'bottom-right': 0,
Expand Down Expand Up @@ -79,6 +82,7 @@
vol.In(COLORS.keys()),
vol.Optional(CONF_TIMEOUT, default=DEFAULT_TIMEOUT): vol.Coerce(int),
vol.Optional(CONF_INTERRUPT, default=DEFAULT_INTERRUPT): cv.boolean,
vol.Optional(CONF_ICON, default=DEFAULT_ICON): cv.string,
})


Expand All @@ -92,16 +96,18 @@ def get_service(hass, config, discovery_info=None):
color = config.get(CONF_COLOR)
interrupt = config.get(CONF_INTERRUPT)
timeout = config.get(CONF_TIMEOUT)
icon = config.get(CONF_ICON)

return NFAndroidTVNotificationService(
remoteip, duration, position, transparency, color, interrupt, timeout)
remoteip, duration, position, transparency, color, interrupt, timeout,
icon)


class NFAndroidTVNotificationService(BaseNotificationService):
"""Notification service for Notifications for Android TV."""

def __init__(self, remoteip, duration, position, transparency, color,
interrupt, timeout):
interrupt, timeout, icon):
"""Initialize the service."""
self._target = 'http://{}:7676'.format(remoteip)
self._default_duration = duration
Expand All @@ -110,19 +116,26 @@ def __init__(self, remoteip, duration, position, transparency, color,
self._default_color = color
self._default_interrupt = interrupt
self._timeout = timeout
self._icon_file = os.path.join(
os.path.dirname(__file__), '..', 'frontend', 'www_static', 'icons',
'favicon-192x192.png')
self._icon_file = None
if icon:
self._icon_file = icon
else:
try:
import hass_frontend
default_icon = os.path.join(
hass_frontend.__path__[0], 'icons', 'favicon-192x192.png')
Copy link
Member

Choose a reason for hiding this comment

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

Use hass_frontend.where() to get path to the frontend

if os.path.exists(default_icon):
self._icon_file = default_icon
except ImportError:
Copy link
Member

Choose a reason for hiding this comment

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

Actually, it's weird that this component even depends on the frontend at all. I wonder if we should include a Home Assistant logo in this repo for such circumstances.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In most situations an icon probably is not required. It's rather a problematic design choice at the TV app. But since I can't influence that there's no other way. Before the changes to the frontend the icons where here, so it wasn't an issue. Having an icon here would be fine for me. Any suggestions where it should go?

Copy link
Member

Choose a reason for hiding this comment

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

Why don't we start with just the bytes of an empty gif, you could embed that in the code.

_LOGGER.warning(
"hass_frontend icon not found. " +
"Provide an icon when calling the service.")

def send_message(self, message="", **kwargs):
"""Send a message to a Android TV device."""
_LOGGER.debug("Sending notification to: %s", self._target)

payload = dict(filename=('icon.png',
open(self._icon_file, 'rb'),
'application/octet-stream',
{'Expires': '0'}), type='0',
title=kwargs.get(ATTR_TITLE, ATTR_TITLE_DEFAULT),
payload = dict(title=kwargs.get(ATTR_TITLE, ATTR_TITLE_DEFAULT),
msg=message, duration="%i" % self._default_duration,
position='%i' % POSITIONS.get(self._default_position),
bkgcolor='%s' % COLORS.get(self._default_color),
Expand All @@ -131,8 +144,11 @@ def send_message(self, message="", **kwargs):
offset='0', app=ATTR_TITLE_DEFAULT, force='true',
interrupt='%i' % self._default_interrupt)

icon_file = None
data = kwargs.get(ATTR_DATA)
if data:
if ATTR_ICON in data:
icon_file = data.get(ATTR_ICON)
if ATTR_DURATION in data:
duration = data.get(ATTR_DURATION)
try:
Expand Down Expand Up @@ -169,6 +185,16 @@ def send_message(self, message="", **kwargs):
_LOGGER.warning("Invalid interrupt-value: %s",
str(interrupt))

if self._icon_file is None and icon_file is None:
_LOGGER.error("No icon available. Not sending notification.")
return
if icon_file is None:
icon_file = self._icon_file
payload[ATTR_FILENAME] = ('icon.png',
open(icon_file, 'rb'),
'application/octet-stream',
{'Expires': '0'})
Copy link
Member

@fabaff fabaff Nov 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

We should check the path with is_allowed_path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I do that this fix doesn't work anymore. Using resources from hass_frontend seems to not be allowed.

Copy link
Member

@fabaff fabaff Nov 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I'm wondering how that's possible because is_allowed_path has no restrictions on the location of files. At least if you are not using a venv I would say it's a permission issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm developing this on a Debian VM as root within a venv. And root shouldn't have any issues with permissions. I haven't looked more deeply into it though. I just noticed, that with a default-configuration (no custom icon set) it won't work anymore.

Copy link
Member

Choose a reason for hiding this comment

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

Since the user is allowed to specify any path to an icon, we need to use is_allowed_path to make sure that we don't have a malicious user send our passwords to a target. Enforce the check only when a custom path is specified.

I think the easier approach here is to remove the option to set custom icons.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see now that setting a custom icon is added in this PR, please revert that change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dynamic icons actually were a feature request a while back. And even though there are some restrcitions with the supported formats it makes sense. Just like other notifiers (like telegram) support sending images.
But I can take it out again if that's preferred.


try:
_LOGGER.debug("Payload: %s", str(payload))
response = requests.post(
Expand Down