Skip to content

Bugfix: 10509 - http is hard coded in plex sensor #11072

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

Merged
emlove merged 14 commits into from
Dec 21, 2017
Merged

Bugfix: 10509 - http is hard coded in plex sensor #11072

Changes from 3 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
fcd92f6
Fix for sensor no ssl
ryanm101 Nov 13, 2017
00f468d
Line length Fixes
ryanm101 Dec 10, 2017
032c527
Removed unneeded schema extensions
ryanm101 Dec 10, 2017
19f1d85
Removed unrequired variable
ryanm101 Dec 11, 2017
17bbe0e
Added defaults for SSL & SSL Verify
ryanm101 Dec 14, 2017
1905579
Moved Defaults to Variables
ryanm101 Dec 14, 2017
3c1ffb6
Fixed Typo
ryanm101 Dec 14, 2017
2cfa728
Removed option to disable verify ssl
ryanm101 Dec 18, 2017
0a4e08e
Merge branch 'clean_plex_uri_fix' of github.com:ryanm101/home-assista…
ryanm101 Dec 18, 2017
92bfd05
Removed unused import
ryanm101 Dec 18, 2017
f6bbf2c
Removed unused CONST
ryanm101 Dec 18, 2017
c28ac78
Fixed error handling
ryanm101 Dec 18, 2017
bf87edb
Cleanup of unneeded vars & logging
ryanm101 Dec 18, 2017
11079a2
Fix for linting
ryanm101 Dec 18, 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
45 changes: 33 additions & 12 deletions homeassistant/components/sensor/plex.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,13 @@
"""
from datetime import timedelta
import logging
import requests

@houndci-bot houndci-bot Dec 18, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

'requests' imported but unused

import voluptuous as vol

from homeassistant.components.switch import PLATFORM_SCHEMA
from homeassistant.const import (
CONF_NAME, CONF_USERNAME, CONF_PASSWORD, CONF_HOST, CONF_PORT, CONF_TOKEN)
CONF_NAME, CONF_USERNAME, CONF_PASSWORD, CONF_HOST, CONF_PORT, CONF_TOKEN,
CONF_SSL, CONF_VERIFY_SSL)
from homeassistant.helpers.entity import Entity
from homeassistant.util import Throttle
import homeassistant.helpers.config_validation as cv
Expand Down Expand Up @@ -48,34 +50,53 @@ def setup_platform(hass, config, add_devices, discovery_info=None):
plex_host = config.get(CONF_HOST)
plex_port = config.get(CONF_PORT)
plex_token = config.get(CONF_TOKEN)
plex_url = 'http://{}:{}'.format(plex_host, plex_port)
plex_ssl = config.get(CONF_SSL)
plex_verify_ssl = config.get(CONF_VERIFY_SSL)

@balloob balloob Dec 10, 2017

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.

Why is this needed?

@ryanm101 ryanm101 Dec 10, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Why is what needed?

@ryanm101 ryanm101 Dec 10, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If you mean why am I assigning them to variables it's because the variables are used in the main Class as well as before it and having them as variables looks cleaner, in truth we could pass the whole of config into the main class and use the config options directly but I'm not sure it would be very clean looking.

@balloob balloob Dec 11, 2017

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.

Why do we need to be able to disable SSL?

@ryanm101 ryanm101 Dec 11, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not everyone has it enabled for their local plex server
The issue this is to fix is that the uri was hardcoded as "http://" so no-one with ssl could use the sensor, this patch corrects that oversight.
In the case of the verify, it is also possible people are using self-signed certs (I think).

To be honest I dont use ssl for my local plex, I'm just doing this as it was raised as a bug.

@ryanm101 ryanm101 Dec 12, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

See issue: #10509

@balloob balloob Dec 14, 2017

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.

Okay, so that's SSL in the cloud. So I understand the SSL config var. I don't understand why we would ever allow disabling SSL verification?

@ryanm101 ryanm101 Dec 14, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Basically the 3 use cases I can think of are:

  1. Self-Signed Certs
  2. proxy issues.
  3. Troubleshooting

It is NOT recommended and actively discouraged but better to have the option and not need it than need it and not be able to do it.

Side Note: Thanks for forcing me to clean up last PR, forced me to actually take some time to learn a bit more about GIT. :)

@emlove emlove Dec 16, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We generally take a lot of care to limit the number of configuration options exposed to the user just to avoid confusion. Let's not preemptively add an option for something that's actively discouraged and breaks security. If they are using self-signed certs they need to properly configure their system to accept them.

@ryanm101 ryanm101 Dec 16, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@armills

This option already is exposed in the plex media_player component, would it not be preferable to keep parity with the existing implementation within the same product?

Additionally this option is defaulted to True so most people will take the default option here, you would have to explicitly say "No don't check", which to me means you know what you are doing and why.

I dont use the SSL option for this anyway so i'm not particularly fussed, though I do think that where there is a chance someone will need an option we should expose it with a sensible default set. This way people can ignore it if its not applicable but equally if they need it it is there.


http_prefix = 'https' if plex_ssl else 'http'
plex_url = '{}://{}:{}'.format(http_prefix, plex_host, plex_port)

add_devices([PlexSensor(
name, plex_url, plex_user, plex_password, plex_server,
plex_token)], True)
plex_token, plex_ssl, plex_verify_ssl)], True)


class PlexSensor(Entity):
"""Representation of a Plex now playing sensor."""

def __init__(self, name, plex_url, plex_user, plex_password,
plex_server, plex_token):
plex_server, plex_token, plex_ssl, plex_verify_ssl):
"""Initialize the sensor."""
from plexapi.myplex import MyPlexAccount
from plexapi.server import PlexServer
import plexapi.exceptions

cert_session = None

@emlove emlove Dec 18, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It looks like this is always None. Can this be dropped?

if plex_ssl and (plex_verify_ssl is False):
_LOGGER.info("Ignoring SSL verification")
cert_session = requests.Session()
cert_session.verify = False

self._name = name
self._state = 0
self._now_playing = []

if plex_token:
self._server = PlexServer(plex_url, plex_token)
elif plex_user and plex_password:
user = MyPlexAccount(plex_user, plex_password)
server = plex_server if plex_server else user.resources()[0].name
self._server = user.resource(server).connect()
else:
self._server = PlexServer(plex_url)
try:
if plex_token:
self._server = PlexServer(plex_url, plex_token, cert_session)
elif plex_user and plex_password:
user = MyPlexAccount(plex_user, plex_password)
server = plex_server if plex_server \
else user.resources()[0].name
self._server = user.resource(server).connect()
else:
self._server = PlexServer(plex_url)

_LOGGER.info("Plex Sensor Configuration done")
except (plexapi.exceptions.BadRequest, plexapi.exceptions.Unauthorized,

@emlove emlove Dec 18, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Move this try/except to surround the add_devices line in setup platform. Ignoring the exception here means we're creating the entity without a valid self._server, which the rest of the code doesn't handle.

@ryanm101 ryanm101 Dec 18, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

plexapi.exceptions.NotFound) as error:
_LOGGER.info(error)

@emlove emlove Dec 18, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we can't recover from this exception this should be an error level log.

@ryanm101 ryanm101 Dec 18, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

return

@property
def name(self):
Expand Down