Skip to content
This repository has been archived by the owner on May 7, 2020. It is now read-only.

[Sonos] Please add support for CurrentAlbumArt, Shuffle and Repeat #1617

Closed
joergkling opened this issue Jun 5, 2016 · 38 comments
Closed

[Sonos] Please add support for CurrentAlbumArt, Shuffle and Repeat #1617

joergkling opened this issue Jun 5, 2016 · 38 comments

Comments

@joergkling
Copy link
Contributor

joergkling commented Jun 5, 2016

Would be great if these three feature could also be provided by the Sonos bindind.

  • CurrentAlbumArt: is avaible in the OH1 binding, String provided by the Sonos player with the URI to the AlbumArt
  • Shuffle: boolean triggering the shuffle function in the player
  • Repeat: boolean triggering the repeat function in the player
@lolodomo
Copy link
Contributor

lolodomo commented Aug 2, 2016

I could take a look for repeat and shuffle features but if I correctly remember they are mixed in the same setting.

@watou
Copy link
Contributor

watou commented Nov 5, 2016

If a String item can contain a URL to the cover art, #2412 addresses it.

@lolodomo
Copy link
Contributor

lolodomo commented Nov 5, 2016

@watou: thank you for your PR, I will add the required channel in the Sonos binding to provide album art.

@kaikreuzer
Copy link
Contributor

I still think the proper way to have cover art support for the Sonos binding is to add an Image channel, not a String channel. Supporting urls in String items is rather a workaround for OH1 bindings that do not have an Image item type available.

@watou
Copy link
Contributor

watou commented Nov 5, 2016

Supporting urls in String items is rather a workaround for OH1 bindings that do not have an Image item type available.

I would expect to see the opposite: APIs that provide URLs to local or remote images over HTTP, where the URLs just need to be conveyed and possibly proxied. It would also be helpful to know an item's state has changed at the client without also marshalling the entire raw image data, which URLs are fine for. It seems like so much more work to have a binding capture the raw image data and marshall it in a different manner. Surveillance camera snapshots, for example, would seem much better suited to HTTP IMHO -- the plumbing is mainly in place already. No harm in properly supporting both, though, is there?

@kaikreuzer
Copy link
Contributor

You should note that the url-string trick only works for UIs that are using sitemaps - and the ProxyServlet is also tied to sitemap use. So this mechanism does NOT work for UIs like Paper UI and HABPanel.

No harm in properly supporting both, though, is there?

The more I think about it, the less I like it. I am back to the situation where I think that it really only makes sense if the urls are pointing to some Internet resource. After all, we are sending item state updates out to the world (the UIs) and these urls should make sense to them. The miraculous conversion from "any url" to one that points to the proxy servlet cannot be done on the item level.

@watou
Copy link
Contributor

watou commented Nov 5, 2016

The only real point of the proxy servlet is to solve the problem with the server and client existing on different networks. Some thought about solving that specific problem, so a client like HABPanel or Paper UI is always given a URL it can dependably use to retrieve an image, and less proxying would happen, and clients can deal in all image and video URLs in a natural way. A new "media" servlet could handle all image requests, but either proxy if necessary, or return a 301 redirect to the already addressable URL. No reference to sitemaps or widgets needed. (Rough idea anyway.)

@watou
Copy link
Contributor

watou commented Nov 5, 2016

Could the non-sitemap-based UIs simply use new, additional usages of the proxy servlet like:

http://openhab:8080/proxy?url=http%3A%2F%2Fwww.placecage.com%2Fc%2F640%2F480
http://openhab:8080/proxy?item=MyImageOrStringItem

with the possible enhancement of returning a 301 redirect in the case that the client is on the same network?

@kaikreuzer
Copy link
Contributor

As I mentioned, the proxy servlet is really tied to the sitemaps. You are not asking a url from it, but the content of a sitemap widget. This is designed like that for security reasons (as otherwise, you can very easily spy on the local network, just by having access to the runtime through HTTP).

Furthermore there are ESH solutions that do not use sitemaps and thus do not even have the smarthome.ui bundle included (which holds the servlet), so this is nothing that is always available.

@watou
Copy link
Contributor

watou commented Nov 5, 2016

the proxy servlet is really tied to the sitemaps

Yes, so why not extend it to also take item=X instead of sitemap=X&widget=Y?

ESH solutions that do not use sitemaps and thus do not even have the smarthome.ui bundle included

In that case, instead make a new servlet that has no dependencies on the smarthome.ui bundle, but only takes an item name and returns media over HTTP that is from either an ImageItem's RawType state, or a StringItem's StringType state that holds a URL?

...or extend the REST API for retrieving an item's State with a followLinks=true parameter or end in .../state/raw etc., which will return the ImageItem's raw RawType, setting correct headers, or proxy the link in the StringItem's StringType, copying the headers?

@kaikreuzer
Copy link
Contributor

Yes, so why not extend it to also take item=X instead of sitemap=X&widget=Y?

You are not resolving the security issue: You can post any url as state to the item and then have it resolved - all through http.

@watou
Copy link
Contributor

watou commented Nov 5, 2016

Just so it's clear, what is the specific security issue in the above scenario? Having write access to items' states or issuing commands to items is in the same class of security issue, isn't it? Having unprotected APIs, as a general issue, be it servlets or the REST API, is in the same bucket of potential exposures.

@lolodomo
Copy link
Contributor

lolodomo commented Nov 6, 2016

Sorry, just to be clear with a less technical speaking, kai you would expect that the binding downloads the image from the URL and stores the data in the image object ?
Do we have somewhere in OH a common util to dowmload data from a URL ? This could be used by any binding.
We could have 2 channels, one string channel with the URL and one image channel with the downloaded image. Then the user will choose and at least the string channel will be usable immediately without waiting for implementation in all UI.

@kgoderis
Copy link
Contributor

kgoderis commented Nov 6, 2016

This is in fact quite an interesting discussion, even a bit related to openhab/openhab-distro#312 and #2359. Reading the comments and concerns it looks as if we have to ask ourselves to what extend ESH/OH(2) should be considered a trusted and secure gateway to control a home. In that sense moving away from proxying towards a transfer, even if CPU/resource intensive, of raw data does make sense as we can then better control what is transferred, and to whom. Also, it would reduce the dependency on the security framework of the underlying home devices. E,g, some home devices, like security cameras, have their own security infrastructure, but some have none at all. ESH/OH could function as a uniform security gateway to all these devices. Couple this with a clean cut interface/boundary with the UI's and you have the best of all worlds. As much as it has to to with security, it also has to do with privacy.

@watou
Copy link
Contributor

watou commented Nov 6, 2016

Good points, @kgoderis. When all API access to ESH/OH is properly secured and access to private resources must flow through secured paths, then complicated reverse proxy setups will be called for less often. But until the paths are secured system wide, we should still be building those paths!

@lolodomo
Copy link
Contributor

lolodomo commented Nov 6, 2016

For Sonos, we are talking about album cover art. There is no problem of privacy, they are public data.
If I correctly remember, the URL is a HTTP request to the Sonos device and so will only work in the local network if not proxied by the OH server.

@lolodomo
Copy link
Contributor

lolodomo commented Nov 6, 2016

If the channel is declared as read only, is a state update by the user possible ?
Maybe we should forbid all state updates not done by the binding for any read only channel. It would avoid security problem mentionned by Kai.

@watou
Copy link
Contributor

watou commented Nov 6, 2016

Maybe we should forbid all state updates not done by the binding for any read only channel.

My thinking is that having full access and authorization logic over the REST API would be the right place to implement this kind of control. Adding ad hoc security limits in absence of that would only cripple ESH in very specific ways, which would then need workarounds. REST APIs that implement permissions for various read/write access would be a superior approach IMHO.

@kaikreuzer
Copy link
Contributor

Just so it's clear, what is the specific security issue in the above scenario?

The issue is that a user that has access to the REST API of the runtime server, automatically has access TO ALL HTTP RESOURCES that are in the same network as this server. This is imho a major security issue and the reason, why I never introduced this feature (what I unfortunately didn't remember when giving @watou the ok on #1800 (comment) late at night...).

Apart from security considerations, putting urls in String items is pretty much against all recent efforts in ESH to have more semantics and meta-information on items.

So this feature is for me still only valid as a workaround for OH1, where no Image item exists. I would not want to promote this at all for ESH and definitely do not want to see it used within ESH bindings - these should definitely go for the Image item as this has been specifically introduced for those use cases.

@watou
Copy link
Contributor

watou commented Nov 6, 2016

The issue is that a user that has access to the REST API of the runtime server, automatically has access TO ALL HTTP RESOURCES that are in the same network as this server

OK, but I see this as an argument to secure the APIs, not to restrict the features. The exploit you describe still requires knowledge of which items hold URLs, and anyone with that knowledge would also have knowledge of how to turn off my furnace in winter, unlock my doors, disable the alarm system, and so on.

If the argument is to never deal in URLs at all (even with a new UrlType State type), then I see this is as closing off a wide range of needed functionality. If anything, this is an argument to prioritize the objectives of #2359 so that ESH can more easily address real-world integration goals.

@kaikreuzer
Copy link
Contributor

anyone with that knowledge would also have knowledge of how to turn off my furnace in winter, unlock my doors, disable the alarm system, and so on.

Yes, but if you grant someone access to your smarthome runtime, this is expected. You would not expect to expose all (not smarthome related) servers that are in your LAN, though - e.g. your router, on which you might use a default password as you think it is not exposed anyhow. It is a bit similar to Cross-Site-Scripting attacks.

this is an argument to prioritize the objectives of #2359 so that ESH can more easily address real-world integration goals.

Fully with you and as you can see, I am trying to push #2359 forward as I think this is really important - definitely a priority for me right now!

@watou
Copy link
Contributor

watou commented Nov 6, 2016

It is a bit similar to Cross-Site-Scripting attacks.

I understand the distinction you are drawing, but without an audit of every possible configuration and the backend systems they are tied to, there is already a theoretically infinitely long list of other vulnerabilities of the same sort, in absence of a secured API that stops all malicious actors at the front gate. These vulnerabilities might be less obvious to you and me, but in plain sight to others. A commercial grade security infrastructure around any ESH solution's APIs is the only real alternative to completely isolating it on a truly private network (i.e., don't let anyone else even reach the server).

If there is any way I can contribute to #2359 or those goals, please let me know. I think this has been a fruitful discussion so far. 👍

@lolodomo
Copy link
Contributor

So what are we doing finally ?
We have to wait for implementation of ImageItem in all UIs first ?
Is there already a binding using the ImageItem ?

@kaikreuzer
Copy link
Contributor

@watou: I think there is a substantial difference in the vulnerability severity nonetheless. While it is true that any binding that reaches out to some external system means some risk (which should be carefully assessed and mitigated when developing that binding), the proxy servlet here means that the core runtime of ESH (without ANY bindings installed) could be used as a proxy to any HTTP resource on the current network. I don't we have anything like that and I would want to make sure that the ESH core itself IS secure and that it does not allow any nasty things.

@lolodomo No, there is no binding yet using the ImageItem - the idea for this issue would be to make Sonos the first one to use it, so that this can be used as a future reference for others. And yes, with it, we should push #1800 forward at the same time.

@watou
Copy link
Contributor

watou commented Nov 11, 2016

@kaikreuzer: As I put in the code comments, if you don't use the item= feature in your sitemaps then the behavior is the same as before. The passive act of allowing unsecured access to the REST API on an unsecure network presents a range of vulnerabilities that are likely worse than this feature, because this feature has to be explicitly enabled in a server-based text file which as yet cannot be done via the API. Until the ESH solution is shipped with the API secured by default (which I recommend), any malicious actor can perform any number of actions remotely that adversely affect the owner (installing bindings, configuring them with malicious intent, etc.). But altering the sitemap file to enable item= in sitemaps isn't one of them.

I think it's unwise to rank this useful but entirely optional feature as a more or less severe vulnerability than others that aren't yet catalogued, and which will always exist in a setup that doesn't have a secure API.

If there is a path via the API to install the demo package, and therefore introduce a sitemap with Image item= into the configuration, then perhaps it makes sense to not include it in the demo package for that reason. But we should of course avoid any "security through obscurity" thinking in our decisions.

Lastly, I think it would be disappointing to disallow access to this feature to those users who proceed with the belief that their networks are secure, and therefore left default passwords on their routers, IP cameras, and so on. They would have to explicitly edit their sitemaps to enable this feature, which is a deliberate decision like the other decisions about their network security. ESH is not a firewall, it's an enabler of useful functionality.

@kaikreuzer
Copy link
Contributor

@watou, you are right in all points. My concerns stated above were actually related to changing the ProxyServlet to accept itemNames as parameters (and not only widgetIds) and in such a case, it would not have been a deliberate decision of the user to open this hole. But your implementation indeed stays with the widgetId and so I agree that it is up to the user whether he wants to use it or not.

Regarding this issue here: I would still stick to the statement that bindings should not force users to use this feature because they only provide images through String items - as suggested above, I would love to see Sonos being the first one to make use of the Image item as a reference for others.

@marcelrv
Copy link
Contributor

while I was trying to add album art to the onkyo binding I bumped into this tread.
In case of Onkyo I also get from the receiver an url to the image.

Is now the recommendation to somehow proxy the image, or can the binding simply pass the url and the client is retrieving the image? In the latter is the binding expected to use a ImageItem or StringItem in that case?

@lolodomo
Copy link
Contributor

@marcelrv : @kaikreuzer does not like the URL in string item and would like an ImageItem (even if ImageItem is not yet supported by any UI except Paper UI !). If you want to be the first to implement that in the Onkyo binding, that would be excellent and we will then copy in the Sonos binding.

@kaikreuzer
Copy link
Contributor

@kaikreuzer does not like the URL in string item and would like an ImageItem

I hope it is not just me, but that others agree that this approach makes sense :-)

@lolodomo
Copy link
Contributor

lolodomo commented Dec 19, 2016

The approach makes sense but as this item type is not yet handled by UIs, it will probably discourage developers to implement it.
By the way, as soon as I have a little time, I will prepare the Sonos binding for it by storing the URL in a variable.

What I would appreciate is that we could use a String item (with URL) at least until the ImageItem is supported by Basic UI, HABdroid and the UI for iPhone. Then we will remove it. Is it acceptable ?

@watou
Copy link
Contributor

watou commented Dec 19, 2016

I hope it is not just me, but that others agree that this approach makes sense :-)

My concerns are that there is no support for streaming potentially large RawType states (unless I've missed it), there is no indication of MIME type, the binding will update the ImageItem's state even when no clients are interested in it, and it puts the binding in the job of taking an HTTP resource, re-marshalling it over a proprietary protocol and delivering it to a client that could have handled the HTTP resource in the first place. If the concern is secure proxying, perhaps there is some other approach using tokens that restricts which HTTP resources can be proxied (not that I've worked out such a thing in my head yet 🙂 ). If such a restriction mechanism could be implemented, it could be added to the ProxyServlet and new parameters could be added there.

Just my two cents. 😄

@lolodomo
Copy link
Contributor

You're right, it would add load to the server even when no UI is requiring the image.

PS: at least, we could disable the transfer of data when no item is linked to the channel.

@maggu2810
Copy link
Contributor

@kaikreuzer does not like the URL in string item and would like an ImageItem
I hope it is not just me, but that others agree that this approach makes sense :-)

Me, too. But @watou has a good point. Also if no one is interested in the image it will always be downloaded by the bindings etc. I don't know if this will lead to heavy load but it is something that should be considered.

I don't know much about the current ESH proxy servlet, but I played around the Jetty Proxy Servlet for some other stuff and I think it could be used to provide dynamic proxied stuff. Just a example what a binding could do:

proxy.removeProxyUrl(this.lastProxiedUrl);
this.lastProxiedUrl = proxy.getProxyUrl("http://some-path-to-an/image.png");
updateState(IMAGE_URL, new StringType(this.lastProxiedUrl));

But I am still think the ImageItem should be used if possible.

@marcelrv
Copy link
Contributor

I'm missing the point wrt to the security. If the binding would provide an not proxied url, what is the security concern? The worse thing that could happen is that it points to a location on a local network, which is unreachable from the internet location. It would also ensure the user has access & proper mime-types, no unnessesary burden on restrained seervers etc.

If there is from the binding side a need to provide a proxied version of the image, than that would still be possible by providing a relative link to the proxied resource

@maggu2810
Copy link
Contributor

I'm missing the point wrt to the security. If the binding would provide an not proxied url, what is the security concern?

IMHO there are no security concerns if a string item provides an URL.
The security concerns has been if the proxy servlet is able to be used to proxy every URL and an user could use it to load whatever he wants.
I tried to address that topic above in my code example.
If we ever would use the proxy servlet to provide a proxied URL we need to filter the requests.

But as already written by @kaikreuzer the proxy stuff is not part of the core (correct?) and so only an extension that a binding should not rely on.

@lolodomo
Copy link
Contributor

lolodomo commented Jan 10, 2017

I will implement the album art in the Sonos binding, it should not be so difficult.

I will add:

  • a channel associated to an item of type Image that will contain the raw data of the downloaded image
  • an advanced channel associated to an item of type String that will contain the HTTP URL of the image

We could decide later to suppress the second channel the day ImageItem is supported by most of UIs.

@kaikreuzer or any who knows: just 3 querstions relative to ImageItem:

  • Can you confirm that I just have to use a State of type RawType to update the channel ?
  • I see nothing to specify the format of the image. The raw data as a byte[] will be sufficient for any UI to render properly the image whatever its format ?
  • shall we consider the download of the picture in a dedicated thread ?

@lolodomo
Copy link
Contributor

I have now implemented the album cover art: PR #2838

@lolodomo
Copy link
Contributor

@kaikreuzer : this issue can now be closed because all requested enhancements have been implemented in the binding.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants