Bugfix: 10509 - http is hard coded in plex sensor by ryanm101 · Pull Request #11072 · home-assistant/core

ryanm101 · 2017-12-10T22:50:33Z

ryanm101 · 2017-12-10T23:18:56Z

@balloob Cleaned up PR

balloob · 2017-12-10T23:40:50Z

    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)


Why is this needed?

Why is what needed?

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.

Why do we need to be able to disable SSL?

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.

See issue: #10509

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?

Basically the 3 use cases I can think of are:

Self-Signed Certs

proxy issues.

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

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.

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

Corrected SSL Defaults to match the other Defaults style

cnf · 2017-12-16T15:16:48Z

This seems to work for me, except it doesn't read plex.conf like the media_player component does.

FYI, the url of a plex server is <ip with dots replaces with dashes>.<serverhash>.plex.direct

and you can find the token using curl "https://plex.tv/api/resources?X-Plex-Token=$PLEXTOKEN&includeHttps=1"

you can get the PLEXTOKEN doing https://forums.plex.tv/discussion/129922/how-to-request-a-x-plex-token-token-for-your-app

ryanm101 · 2017-12-16T16:41:17Z

No, it doesn't and it probably should, I'll add that as a feature update once this PR is approved.

…nt into clean_plex_uri_fix

houndci-bot · 2017-12-18T00:30:38Z

 """
 from datetime import timedelta
 import logging
+import requests


'requests' imported but unused

ryanm101 · 2017-12-18T00:35:45Z

@cnf can you please verify this still works?

@armills @balloob Ok While I disagree with removing the option as we have it available in other components and I'd prefer to be consistent by making it easy to do the right thing (via sensible defaults) while still allowing those that wish to knowingly do the wrong thing to continue to work. I've removed the option to disable verification of the SSL Cert.

emlove

Looks mostly good, just a small request on the new try. We'll also need a docs PR for the new config option.

emlove · 2017-12-18T01:32:59Z

+                self._server = PlexServer(plex_url)
+
+            _LOGGER.info("Plex Sensor Configuration done")
+        except (plexapi.exceptions.BadRequest, plexapi.exceptions.Unauthorized,


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.

emlove · 2017-12-18T01:33:23Z

+            _LOGGER.info("Plex Sensor Configuration done")
+        except (plexapi.exceptions.BadRequest, plexapi.exceptions.Unauthorized,
+                plexapi.exceptions.NotFound) as error:
+            _LOGGER.info(error)


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

emlove · 2017-12-18T02:06:30Z

        from plexapi.myplex import MyPlexAccount
        from plexapi.server import PlexServer

+        cert_session = None


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

emlove · 2017-12-18T02:07:02Z

        else:
            self._server = PlexServer(plex_url)

+        _LOGGER.info("Plex Sensor Configuration done")


This log isn't necessary. Home assistant core will already log when each platform is setup.

emlove · 2017-12-18T02:07:45Z

        elif plex_user and plex_password:
            user = MyPlexAccount(plex_user, plex_password)
-            server = plex_server if plex_server else user.resources()[0].name
+            server = plex_server if plex_server \


This line can go back as well now to clean up the diff.

cnf · 2017-12-18T14:43:39Z

@ryanm101 injecting https://github.com/ryanm101/home-assistant/blob/11079a2cc5d60271a091232104db6b80b04fa31c/homeassistant/components/sensor/plex.py into the docker container still works for me.

ryanm101 · 2017-12-18T22:31:12Z

@armills @balloob Guys why is "TestTrendBinarySensor" breaking this?

Also is the py3.4 test needed since iirc the plan it to not support it from the start of next year?

emlove · 2017-12-18T23:26:40Z

Looks like it was just a flapping test. I restarted the job.

emlove

Code is good, but marking this requested changes so we don't forget the docs update.

ryanm101 · 2017-12-21T15:18:41Z

@armills That's already been done:
home-assistant/home-assistant.io#4238

emlove · 2017-12-21T15:24:00Z

Ah, thanks. Missed that. Next time if you can link the docs PR in the description it helps keep track of what's been done.

ryanm101 · 2017-12-21T15:30:15Z

Will do

This reverts commit 901d4b5.

* Revert "Adding MotionIP to BinarySensors for HMIP-SMI (#11268)" This reverts commit c94cc34. * Revert "Bugfix: 10509 - http is hard coded in plex sensor (#11072)" This reverts commit 901d4b5. * Revert "Fix handling zero values for state_on/state_off (#11264)" This reverts commit 2e4e3a4. * Revert "Fix inverted sensors on the concord232 binary sensor component (#11261)" This reverts commit b866687. * Revert "Proper Steam game names and small fixes (#11182)" This reverts commit 7faa940. * Revert "Bugfix homematic available modus (#11256)" This reverts commit 1d57958. * Revert "Fix detection of if a negative node is in use (#11255)" This reverts commit b28bfad. * Revert "added myself to become code owner for miflora and plant (#11251)" This reverts commit e068204. * Revert "Add workaround for running tox on Windows platforms (#11188)" This reverts commit 81f1a65. * Revert "Upgrade to new miflora version 0.2.0 (#11250)" This reverts commit 8efc4b5. * Revert "homematic: add username and password to interface config schema (#11214)" This reverts commit b4e2537. * Revert "Backup configuration files before overwriting (#11216)" This reverts commit 90e25a6.

ryanm101 added 2 commits December 10, 2017 22:48

Fix for sensor no ssl

fcd92f6

Line length Fixes

00f468d

homeassistant added cla-signed platform: sensor.plex labels Dec 10, 2017

balloobbot added the platform label Dec 10, 2017

Removed unneeded schema extensions

032c527

homeassistant added the cla-signed label Dec 10, 2017

balloob reviewed Dec 10, 2017

View reviewed changes

Removed unrequired variable

19f1d85

homeassistant added the cla-signed label Dec 11, 2017

ryanm101 and others added 3 commits December 14, 2017 10:30

Added defaults for SSL & SSL Verify

17bbe0e

Moved Defaults to Variables

1905579

Corrected SSL Defaults to match the other Defaults style

Fixed Typo

3c1ffb6

ryanm101 changed the title ~~Fix for http being hard coded in plex sensor~~ Bugfix: 10509 - http is hard coded in plex sensor Dec 16, 2017

cnf mentioned this pull request Dec 16, 2017

Plex Sensor does not support https only plex server. http is hard coded. #10509

Closed

ryanm101 added 2 commits December 18, 2017 00:27

Removed option to disable verify ssl

2cfa728

Merge branch 'clean_plex_uri_fix' of github.com:ryanm101/home-assista…

0a4e08e

…nt into clean_plex_uri_fix

homeassistant added the cla-signed label Dec 18, 2017

houndci-bot reviewed Dec 18, 2017

View reviewed changes

Removed unused import

92bfd05

Removed unused CONST

f6bbf2c

homeassistant added the cla-signed label Dec 18, 2017

emlove requested changes Dec 18, 2017

View reviewed changes

ryanm101 mentioned this pull request Dec 18, 2017

Remove Option to disable VERIFY_SSL #11195

Closed

Fixed error handling

c28ac78

emlove requested changes Dec 18, 2017

View reviewed changes

ryanm101 added 2 commits December 18, 2017 10:28

Cleanup of unneeded vars & logging

bf87edb

Fix for linting

11079a2

ryanm101 mentioned this pull request Dec 18, 2017

updated docs to add SSL option home-assistant/home-assistant.io#4238

Merged

emlove approved these changes Dec 18, 2017

View reviewed changes

ryanm101 closed this Dec 18, 2017

ryanm101 reopened this Dec 18, 2017

emlove requested changes Dec 20, 2017

View reviewed changes

emlove approved these changes Dec 21, 2017

View reviewed changes

emlove merged commit 901d4b5 into home-assistant:dev Dec 21, 2017

ryanm101 deleted the clean_plex_uri_fix branch December 21, 2017 15:30

pvizeli added a commit that referenced this pull request Dec 21, 2017

Revert "Bugfix: 10509 - http is hard coded in plex sensor (#11072)"

5f8bedb

This reverts commit 901d4b5.

balloob mentioned this pull request Jan 11, 2018

0.61 #11589

Merged

home-assistant locked and limited conversation to collaborators Mar 30, 2018

ghost added integration: plex and removed platform: sensor.plex labels Mar 21, 2019

Uh oh!

Conversation

ryanm101 commented Dec 10, 2017

Uh oh!

ryanm101 commented Dec 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanm101 Dec 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanm101 Dec 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanm101 Dec 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cnf commented Dec 16, 2017

Uh oh!

ryanm101 commented Dec 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanm101 commented Dec 18, 2017

Uh oh!

emlove left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emlove Dec 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cnf commented Dec 18, 2017

Uh oh!

ryanm101 commented Dec 18, 2017

Uh oh!

emlove commented Dec 18, 2017

Uh oh!

emlove left a comment

Choose a reason for hiding this comment

Uh oh!

ryanm101 commented Dec 21, 2017

Uh oh!

emlove commented Dec 21, 2017

Uh oh!

ryanm101 commented Dec 21, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

ryanm101 Dec 11, 2017 •

edited

Loading

ryanm101 Dec 14, 2017 •

edited

Loading

ryanm101 Dec 16, 2017 •

edited

Loading

emlove Dec 18, 2017 •

edited

Loading