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.

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

jsonrpc: expose profile directory in Profiles.GetCurrentProfile/GetProfiles. #15509 #8196

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
10 participants
@ghost
Copy link
Contributor

ghost commented Oct 10, 2015

Bumps version to 7.13.0.

> {"jsonrpc":"2.0","id":1,"method":"Profiles.GetProfiles","params":{"properties":["thumbnail","lockmode","directory"]}}

< {"id":1,"jsonrpc":"2.0","result":{"limits":{"end":2,"start":0,"total":2},"profiles":[{"directory":"special://masterprofile/","label":"Master user","lockmode":0,"thumbnail":""},{"directory":"profiles/prova/","label":"prova","lockmode":0,"thumbnail":""}]}}

@Montellese

cc @MilhouseVH @un1versal

Edit: diff might be confusing - I've inverted the 2 for loops.

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

MilhouseVH commented Oct 10, 2015

Many thanks for this.

However, assuming this is being accessed by a remote script (as it's JSON), I suspect it would be more useful if the returned directory were the real OS directory (ie. /storage/.kodi/userdata/profiles/master) rather than the internal "special://" internal Kodi path - is it possible to translate from Kodi to OS and return the latter?

Edit: The relative directory returned by the prova profile is useful, however the special://masterprofile/ format is not so useful - assuming this is also relative to userdata then it should be translated into profiles/master similar to the prova profile.

@anaconda anaconda force-pushed the menakite:json-profile-directory branch 2 times, most recently from a3316a0 to 784b9c4 Oct 10, 2015

@ghost

This comment has been minimized.

Copy link
Contributor Author

ghost commented Oct 10, 2015

Updated as discussed on Slack.

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

MilhouseVH commented Oct 11, 2015

Works perfectly - many thanks!

@MilhouseVH MilhouseVH referenced this pull request Oct 11, 2015

Closed

json profiles #30

}
else if (propertyIter->asString() == "directory")
{
std::string path(CSpecialProtocol::TranslatePath("special://masterprofile"));

This comment has been minimized.

@Montellese

Montellese Oct 11, 2015

Member

Please use CProfilesManager::GetInstance().GetUserDataFolder() instead of hardcoded special://masterprofile.

@Montellese

This comment has been minimized.

Copy link
Member

Montellese commented Oct 11, 2015

Looks good apart from my comment.

@anaconda anaconda force-pushed the menakite:json-profile-directory branch from 784b9c4 to 2c035fa Oct 11, 2015

@ghost

This comment has been minimized.

Copy link
Contributor Author

ghost commented Oct 11, 2015

Removed the 2 hardcoded special://s.

@Tolriq

This comment has been minimized.

Copy link
Contributor

Tolriq commented Oct 14, 2015

Since those folders will not be accessible from remotes after but only from a local script the utility in current form is quite limited :(

Should not the API return the special paths with added things in https://github.com/Xbmc/xbmc/blob/master/xbmc/utils/FileUtils.cpp#L110 to allow remote access (Would allow remotes to finally get the advancedsettings and things like that if they are in profiles).

And eventually, to fit some special needs, a function to translate path added to JSON API?

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

MilhouseVH commented Oct 14, 2015

Since those folders will not be accessible from remotes after but only from a local script the utility in current form is quite limited :(

Yes they will - you can access /storage/.kodi/userdata/advancedsettings.xml (or whatever the actual OS path is) remotely via JSON:

JSON WEB REQUEST: [{"jsonrpc": "2.0", "params": {"path": "/storage/.kodi/userdata/advancedsettings.xml"}, "method": "Files.PrepareDownload", "id": "preparedl"}]
RECEIVED DATA: {"id":"preparedl","jsonrpc":"2.0","result":{"details":{"path":"vfs/%2fstorage%2f.kodi%2fuserdata%2fadvancedsettings.xml"},"mode":"redirect","protocol":"http"}}

then download the path to access the contents of the file, so this should work for you.

Oddly I'm not able to obtain a directory listing using JSON Files.GetDirectory with special://masterprofile/:

JSON SOCKET REQUEST: [{"jsonrpc": "2.0", "params": {"directory": "special://masterprofile/", "media": "files", "properties": ["file", "lastmodified"]}, "method": "Files.GetDirectory", "id": "libDirectory"}]
BUFFER RECEIVED (len 89)
PARSING JSON DATA: {"error":{"code":-32602,"message":"Invalid params."},"id":"libDirectory","jsonrpc":"2.0"}

but /storage/.kodi/userdata is working:

JSON SOCKET REQUEST: [{"jsonrpc": "2.0", "params": {"directory": "/storage/.kodi/userdata", "media": "files", "properties": ["file", "lastmodified"]}, "method": "Files.GetDirectory", "id": "libDirectory"}]
BUFFER RECEIVED (len 2433)
PARSING JSON DATA: {"id":"libDirectory","jsonrpc":"2.0","result":{"files":[{"file":"/storage/.kodi/userdata/peripheral_data/","filetype":"directory","label":"peripheral_data","lastmodified":"2015-07-09 22:41:48","type":"unknown"},{"file":"/storage/.kodi/userdata/playlists/","filetype":"directory","label":"playlists","lastmodified":"2015-07-09 22:30:41","type":"unknown"},{"file":"/storage/.kodi/userdata/sources.xml","filetype":"file","label":"sources.xml","lastmodified":"2015-09-25 22:26:25","type":"unknown"},{"file":"/storage/.kodi/userdata/favourites.xml","filetype":"file","label":"favourites.xml","lastmodified":"2015-09-22 10:33:42","type":"unknown"},{"file":"/storage/.kodi/userdata/guisettings.xml","filetype":"file","label":"guisettings.xml","lastmodified":"2015-10-14 03:41:31","type":"unknown"},{"file":"/storage/.kodi/userdata/library/","filetype":"directory","label":"library","lastmodified":"2015-07-09 22:30:41","type":"unknown"},{"file":"/storage/.kodi/userdata/profiles/","filetype":"directory","label":"profiles","lastmodified":"2015-10-11 00:56:06","type":"unknown"},{"file":"/storage/.kodi/userdata/RssFeeds.xml","filetype":"file","label":"RssFeeds.xml","lastmodified":"2015-10-13 21:35:16","type":"unknown"},{"file":"/storage/.kodi/userdata/passwords.xml","filetype":"file","label":"passwords.xml","lastmodified":"2015-07-09 22:41:18","type":"unknown"},{"file":"/storage/.kodi/userdata/advancedsettings.xml","filetype":"file","label":"advancedsettings.xml","lastmodified":"2015-08-19 02:25:19","type":"unknown"},{"file":"/storage/.kodi/userdata/keymaps/","filetype":"directory","label":"keymaps","lastmodified":"2015-08-20 20:44:42","type":"unknown"},{"file":"/storage/.kodi/userdata/profiles.xml","filetype":"file","label":"profiles.xml","lastmodified":"2015-10-14 03:41:31","type":"unknown"},{"file":"/storage/.kodi/userdata/Thumbnails/","filetype":"directory","label":"Thumbnails","lastmodified":"2015-07-10 03:53:26","type":"unknown"},{"file":"/storage/.kodi/userdata/guisettings.xml.keep","filetype":"file","label":"guisettings.xml.keep","lastmodified":"2015-07-10 18:30:46","type":"unknown"},{"file":"/storage/.kodi/userdata/Database/","filetype":"directory","label":"Database","lastmodified":"2015-10-14 07:14:19","type":"unknown"},{"file":"/storage/.kodi/userdata/addon_data/","filetype":"directory","label":"addon_data","lastmodified":"2015-10-03 16:42:14","type":"unknown"}],"limits":{"end":16,"start":0,"total":16}}}

so in some cases there is more utility from the OS directories than there is from the special:// directories, and this should work for you as it will for me.

That said, I'd have no objection if this method returned special:// notation provided there is another field with the translated "OS" equivalent, or there is a method to translate from special:// to OS native.

@Tolriq

This comment has been minimized.

Copy link
Contributor

Tolriq commented Oct 14, 2015

This is more complicated as explained in the link I gave earlier ;)

The fact that PrepareDownload returns something does not mean you have access to it as security are not checked in it.

By default from JSON you have no access to files except those explicit declared at : https://github.com/Xbmc/xbmc/blob/master/xbmc/utils/FileUtils.cpp#L110

Or sources with the allowSharing at true and not locked.

This means this is normal you can not browse special://masterprofile or any profile. And should not be able to browse real profile directory too.

So either you have a source pointing to that or

 {
    std::string strPlaylistsPath = CSettings::GetInstance().GetString(CSettings::SETTING_SYSTEM_PLAYLISTSPATH);
    URIUtils::RemoveSlashAtEnd(strPlaylistsPath);
    if (StringUtils::StartsWithNoCase(realPath, strPlaylistsPath)) 
      return true;
  }

Is triggered when accessing those folders, and IMO it should not as security risk.

I added the allowSharing to sources and fixed the FileUtils too, so have a little background on how this part works.

Maybe @Montellese can comment on the CSettings::SETTING_SYSTEM_PLAYLISTSPATH part that maybe opens full access to profiles. (But what profiles : connected one, all, none ?)

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

MilhouseVH commented Oct 14, 2015

And should not be able to browse real profile directory too.

Yeah, true - I had previously added /storage to my sources.xml and forgotten all about it... with this source disabled, /storage/.kodi/userdata is no longer accessible, so now behaves the same as special://masterprofile/.

If the security is relaxed to allow special:// access via JSON then I'm happy for this method to return directory paths using the special:// notation, but I'd also require a method to translate these special:// paths into OS native paths.

However if there are no plans to relax the security around special:// access then it's a moot point and this method and PR may as well stay as it currently is, returning OS native paths.

@Tolriq

This comment has been minimized.

Copy link
Contributor

Tolriq commented Oct 14, 2015

Well seeing how many things where not included because they are not perfect, adding something that can't be used by vast majority seems a little premature, but well I'm not in charge (But decision coherence should prevail) ;)

I have no problem about special or real path (even if real path can leads to some security concerns, but there's so many others in Kodi).

But whatever path is returned, IMO if there's no access from remotes then it should not be present.

Your use case is very very specific and potentially dangerous, accessing data from a non loaded profile that could have a settings to not enable web server for security reason should not be possible.

And think about problems accessing files like passwords.xml.

Anyway as I said not my call, just bringing some knowledge here.

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

MilhouseVH commented Oct 14, 2015

All my niche case requires is the profile directory name relative to userdata, which I can determine from even the special:// notation, so if this PR can return paths using special:// notation (and not OS native paths - sorry for messing you around, @anaconda) then it should work just fine for both of us, for example special://masterprofile/ => "" and special://masterprofile/profiles/Test_123/ => profiles/Test_123/ will still work for me.

Of course the fact the special:// directories remain inaccessible via JSON is a completely different issue.

@anaconda anaconda force-pushed the menakite:json-profile-directory branch from 2c035fa to c84abcd Oct 22, 2015

@un1versal

This comment has been minimized.

Copy link
Contributor

un1versal commented Oct 27, 2015

@MilhouseVH this works here. @anaconda thx.

@anaconda anaconda force-pushed the menakite:json-profile-directory branch from c84abcd to 1dc7948 Oct 31, 2015

@anaconda anaconda force-pushed the menakite:json-profile-directory branch from 1dc7948 to 05748df Nov 8, 2015

@Razzeee Razzeee changed the title jsonrpc: expose profile directory in Profiles.GetCurrentProfile/GetProfiles. jsonrpc: expose profile directory in Profiles.GetCurrentProfile/GetProfiles. #15509 Nov 23, 2015

@Razzeee

This comment has been minimized.

Copy link
Member

Razzeee commented Nov 23, 2015

Linked this to the corresponding trac ticket
http://trac.kodi.tv/ticket/15509

@anaconda anaconda force-pushed the menakite:json-profile-directory branch 2 times, most recently from c677989 to b257dac Nov 24, 2015

@ghost

This comment has been minimized.

Copy link
Contributor Author

ghost commented Nov 24, 2015

Added fixup commit to return the special:// path as Milhouse seems happy with it too.
This would return something like:

> {"jsonrpc":"2.0","id":1,"method":"Profiles.GetProfiles","params":{"properties":["thumbnail","lockmode","directory"]}}
< {"id":1,"jsonrpc":"2.0","result":{"limits":{"end":2,"start":0,"total":2},"profiles":[{"directory":"special://masterprofile/","label":"Master user","lockmode":0,"thumbnail":""},{"directory":"special://masterprofile/profiles/prova/","label":"prova","lockmode":0,"thumbnail":""}]}}

@anaconda anaconda force-pushed the menakite:json-profile-directory branch from b257dac to 957cf43 Dec 3, 2015

@ghost

This comment has been minimized.

Copy link
Contributor Author

ghost commented Dec 4, 2015

@Montellese any chance to get your final ACK before entering RC?

@anaconda anaconda force-pushed the menakite:json-profile-directory branch from 957cf43 to ede5f6f Dec 6, 2015

@MartijnKaijser

This comment has been minimized.

Copy link
Member

MartijnKaijser commented Dec 23, 2015

@Montellese for merge approval

@anaconda anaconda force-pushed the menakite:json-profile-directory branch 4 times, most recently from 7ce49e0 to 5c4b211 Dec 23, 2015

@anaconda anaconda force-pushed the menakite:json-profile-directory branch 3 times, most recently from bfba81b to 162921a Jan 6, 2016

@anaconda anaconda force-pushed the menakite:json-profile-directory branch 2 times, most recently from 06d1602 to 42a8318 Jan 16, 2016

@anaconda anaconda force-pushed the menakite:json-profile-directory branch from 42a8318 to cc26720 Jan 24, 2016

@anaconda anaconda force-pushed the menakite:json-profile-directory branch from cc26720 to 568b99b Feb 3, 2016

@anaconda anaconda force-pushed the menakite:json-profile-directory branch from 568b99b to ea96573 Feb 15, 2016

@anaconda anaconda force-pushed the menakite:json-profile-directory branch from ea96573 to 19b2d63 Apr 5, 2016

@anaconda anaconda force-pushed the menakite:json-profile-directory branch from 19b2d63 to 3e534c9 May 7, 2016

@Rechi

This comment has been minimized.

Copy link
Member

Rechi commented Sep 27, 2017

@menakite any plans to revisit this?

@jenkins4kodi

This comment has been minimized.

Copy link
Contributor

jenkins4kodi commented Nov 19, 2017

@anaconda this needs a rebase

@MartijnKaijser

This comment has been minimized.

Copy link
Member

MartijnKaijser commented Jun 24, 2018

I will close this PR. If there's interest please open a new one (with required changes if needed).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.