[plex] Use https for local connections

[mlobstein]

Addresses #15304

Test build: https://github.com/mlobstein/openhab-addons/releases/download/4.0/org.openhab.binding.plex-4.1.0-SNAPSHOT.jar

@morph166955
@aronbeurskens

[morph166955]

I'll try to throw it in this afternoon and validate.

[morph166955]

Had a free moment, LGTM

2023-07-26 08:07:34.228 [DEBUG] [ex.internal.handler.PlexApiConnector] - Getting session data 'https://REMOVED:32400/status/sessions?X-Plex-Token=REMOVED'
2023-07-26 08:07:34.230 [DEBUG] [ex.internal.handler.PlexApiConnector] - HTTP response: <?xml version="1.0" encoding="UTF-8"?>
<MediaContainer size="0">
</MediaContainer>

[jlaur]

Thanks for quickly implementing this! I have added some minor comments.

[jlaur]

This seems a little overcomplicated. Can you remove the User-Agent header from getClientHeaders() and instead apply it directly in this method (here and when verify is true)?

Since this is a local connection, is it even needed/used for anything?

[mlobstein]

I am not a plex user and thus can't test the code directly so it is difficult to assess which headers are actually needed. I took the approach of trying to maintain what was already implemented. I actually borrowed this header adding code from the core HttpUtil.

[jlaur]

The core HttpUtil code is partly like that because of a bugfix because of the way Jetty works - see openhab/openhab-core#3411. The other part of the story is that the method was bound by contract to have agent delivered inside Properties. We are not bound by this contract, but can freely write the code.

I think it would be better to avoid for each call to put 9-10 headers into a HashMap, with User-Agent being one of them and then iterate through the map in order to apply it for building a Request. while taking special care of User-Agent by string comparison. At least the User-Agent handling part seems like a work-around as it could have been avoided in the first place.

But I can appreciate that you don't want to mess too much with this code since you are not using the binding yourself, so let's leave it as is for now.

[jlaur]

LGTM

[Vendivar]

Hello I’m on Build #3567 and I am having an issue with the plex binding

I am using PLEX docker on an Unraid server with a host mode with Openhab docker on the same server but with a custom host IP

In openhab I get the error: PLEX is not returning valid session data

In the debug log it says no route to host

2023-07-28 14:27:58.567 [DEBUG] [rg.eclipse.jetty.client.HttpExchange] - Failed HttpExchange@399299a7{req=HttpRequest[GET /status/sessions HTTP/1.1]@3be02390[COMPLETED/java.net.NoRouteToHostException: No route to host] res=HttpResponse[null 0 null]@2a5551c0[COMPLETED/java.net.NoRouteToHostException: No route to host]}: req=true/rsp=true null
2023-07-28 14:27:58.567 [DEBUG] [rg.eclipse.jetty.client.HttpExchange] - Failed HttpExchange@5b447797{req=WebSocketUpgradeRequest[GET /:/websockets/notifications HTTP/1.1]@4fd87412[COMPLETED/java.net.NoRouteToHostException: No route to host] res=HttpResponse[null 0 null]@58e130c9[COMPLETED/java.net.NoRouteToHostException: No route to host]}: req=true/rsp=true null

[jlaur]

Hello I’m on Build #3567 and I am having an issue with the plex binding

That seems unrelated to this merged pull request. I would advise to ask for help in the community forum, and then create a new issue if you are able to conclude there's a bug (or room for improvement) in the binding.

[morph166955]

I worked with him on the forum. It's almost definitely related to this PR. Using a snapshot from immediately before has no issues. That said I would suggest creating an issue and tie it to this as a regression

[jlaur]

@morph166955 - thanks, I missed this correlation.

[mlobstein]

@morph166955 Can you take over fixing this issue?

[morph166955]

I certainly don't mind giving it a shot. My plex works on the same code rev and OH version so I believe this is something to do with the dockers. I'm confused as to why moving to https changes a routing decision.

[mlobstein]

@morph166955 I was working on this change to catch the failure and re-try with http

        try {
                // Requests sent to the local server need to bypass certificate checking via the custom httpClient
                final Request request = httpClient.newRequest(url).method(HttpUtil.createHttpMethod(method));
                for (String httpHeaderKey : headers.stringPropertyNames()) {
                    if (httpHeaderKey.equalsIgnoreCase(HttpHeader.USER_AGENT.toString())) {
                        request.agent(headers.getProperty(httpHeaderKey));
                    } else {
                        request.header(httpHeaderKey, headers.getProperty(httpHeaderKey));
                    }
                }
                final ContentResponse res = request.send();
                response = res.getContentAsString();
            } catch (Exception e) {
                // if https conection to the local server fails, re-try with http
                response = HttpUtil.executeUrl(method, url.replace("https:", "http:"), headers, null, null, REQUEST_TIMEOUT_MS);
            }

[aronbeurskens]

I really think this is a bad idea. We shouldn't fall back to insecure urls if connection fails. Instead we have to find out why it fails and fix the https connection

[mlobstein]

Ok, I'll let you sort it out. It was just rather alarming that a problem report came in right after the first nightly build of this PR.

[jlaur]

Perhaps an advanced configuration option could allow users to explicitly use http rather than https? I agree it shouldn't silently fallback to http for security reasons. See also useTLS here (which is defaulted to true): https://www.openhab.org/addons/bindings/lgwebos/#thing-configuration