D27804: smb: add hack to support spaces in workgroup names

[Harald Sitter]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R320:f40191a147c9: smb: add hack to support spaces in 
workgroup names (authored by sitter).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D27804?vs=76934=79458#toc

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27804?vs=76934=79458

REVISION DETAIL
  https://phabricator.kde.org/D27804

AFFECTED FILES
  smb/autotests/smburltest.cpp
  smb/kio_smb_browse.cpp
  smb/smburl.cpp

To: sitter, ngraham
Cc: kde-frameworks-devel, kfm-devel, thiago, azyx, nikolaik, pberestov, 
iasensio, fprice, LeGast00n, cblack, fbampaloukas, alexde, GB_2, Codezela, 
feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, rdieter, mikesomov

D27804: smb: add hack to support spaces in workgroup names

[Harald Sitter]

sitter added a comment.


  Yep. I'm 100% certain of this. The library in fact has no API that returns a 
complete URL or anything near a complete URL. It's using dirent-inspired API to 
let us iterate/stat paths and only ever returns paths relative to whatever 
input it got, from those paths we then compose the actual output URLs again.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D27804

To: sitter, ngraham
Cc: kde-frameworks-devel, kfm-devel, thiago, pberestov, iasensio, fprice, 
LeGast00n, cblack, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, 
meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

D27804: smb: add hack to support spaces in workgroup names

[Thiago Macieira]

thiago added a comment.


  In D27804#621988 , @sitter wrote:
  
  > In D27804#621970 , @thiago wrote:
  >
  > > Still want to see that round-trip.
  >
  >
  > But why? Converting an smbcUrl to a QUrl would literally be useless code.
  
  
  Are you saying you never convert the URL coming from the library so it can be 
displayed in KIO/Dolphin? Are you sure you always decompose its parts and 
recompose a QUrl from it?

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D27804

To: sitter, ngraham
Cc: kde-frameworks-devel, kfm-devel, thiago, pberestov, iasensio, fprice, 
LeGast00n, cblack, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, 
meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

D27804: smb: add hack to support spaces in workgroup names

[Harald Sitter]

sitter added a comment.


  In D27804#621988 , @sitter wrote:
  
  > In D27804#621970 , @thiago wrote:
  >
  > > Still want to see that round-trip.
  >
  >
  > But why? Converting an smbcUrl to a QUrl would literally be useless code.
  
  
  @thiago ^

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D27804

To: sitter, ngraham
Cc: kde-frameworks-devel, kfm-devel, thiago, pberestov, iasensio, fprice, 
LeGast00n, cblack, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, 
meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

D27804: smb: add hack to support spaces in workgroup names

[Harald Sitter]

sitter added a comment.


  In D27804#621970 , @thiago wrote:
  
  > Still want to see that round-trip.
  
  
  But why? Converting an smbcUrl to a QUrl would literally be useless code.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D27804

To: sitter, ngraham
Cc: kde-frameworks-devel, kfm-devel, thiago, pberestov, iasensio, fprice, 
LeGast00n, cblack, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, 
meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

D27804: smb: add hack to support spaces in workgroup names

[Thiago Macieira]

thiago added a comment.


  Still want to see that round-trip.

INLINE COMMENTS

> smburltest.cpp:112
> +// % character - run through .url() to simulate behavior of our 
> listDir()
> +
> QCOMPARE(SMBUrl(QUrl(QUrl("smb://?kio-workgroup=HAX%MAX").url())).toSmbcUrl(),
> + "smb://HAX%25MAX/");

Please don't use QUrl's URL-correction in the constructor. This is not a valid 
URL. Use %25 here.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D27804

To: sitter, ngraham
Cc: kde-frameworks-devel, kfm-devel, thiago, pberestov, iasensio, fprice, 
LeGast00n, cblack, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, 
meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

D27804: smb: add hack to support spaces in workgroup names

[Harald Sitter]

sitter updated this revision to Diff 76934.
sitter added a comment.


  extend test coverage to % character in wg and umlaut in wg.
  I've also changed the construction in browse.cpp to use QUrlQuery so it does 
not trip over potential hash or question marks in the workgroup
  
  libsmbc is actually incredibly lenient in parsing the input urls, so you can 
more or less throw anything unencoded at it and it'll work.
  that is why the original `sambaUrl.toString(PrettyDecoded)` call works 
despite also not carying much for encoding.
  
  @thiago I am not sure I understood points 3 and 4 but I think they aren't 
really applicable:
  
  3. libsmbc doesn't give out urls so there is no need to convert the other way 
around
  4. slash, backslash, colon may not be part of netbios names and by extension 
the url parsing always works regardless of encoding as the host field cannot be 
ambigious with a trailing slash (which we always have)
  
  All that said, perhaps it'd make sense to change everything to 
QUrl::FullyEncoded to be on the safe side for the future? smbc does know how to 
deal with complete percent encoding from what I can tell

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27804?vs=76847=76934

BRANCH
  smb-smburl-static-autotest-ipv6-spaces

REVISION DETAIL
  https://phabricator.kde.org/D27804

AFFECTED FILES
  smb/autotests/smburltest.cpp
  smb/kio_smb_browse.cpp
  smb/smburl.cpp

To: sitter, ngraham
Cc: kde-frameworks-devel, kfm-devel, thiago, pberestov, iasensio, fprice, 
LeGast00n, cblack, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, 
meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

D27804: smb: add hack to support spaces in workgroup names

[Thiago Macieira]

thiago added a comment.


  Looks good, but testing should be extended. I suggest:
  
  1. A workgroup with non-US-ASCII characters in the name, if that's permitted
  2. A workgroup with % in the name: the URL should have "%25"
  3. Roundtrip checking. The test only does toSmbcUrl(), so please test the 
reverse.
  4. Error-checking the conversion from query to hostname: things like %00 (a 
NUL byte), URL delimiters (':', '/', '?' and '#'), byte sequences that do not 
encode UTF-8 (like %FF).
  
  I think that you'll find that % works in one direction only. I have no idea 
what you'll find for #4.

INLINE COMMENTS

> smburl.cpp:134
> +QUrlQuery query(sambaUrl);
> +const QString workgroup = query.queryItemValue("kio-workgroup");
> +if (workgroup.isEmpty()) {

For the % issue, you may want to pass `QUrl::FullyDecoded` as the second 
argument to queryItemValue.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D27804

To: sitter, ngraham
Cc: kde-frameworks-devel, kfm-devel, thiago, pberestov, iasensio, fprice, 
LeGast00n, cblack, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, 
meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

D27804: smb: add hack to support spaces in workgroup names

[Harald Sitter]

sitter created this revision.
sitter added a reviewer: ngraham.
Herald added projects: Dolphin, Frameworks.
Herald added subscribers: kfm-devel, kde-frameworks-devel.
sitter requested review of this revision.

REVISION SUMMARY
  workgroup names are as best I can tell always still netbios names which
  means they can contain a bunch of characters ordinarily not found in valid
  host names. e.g. spaces
  this causes trouble with the IANA SMB URI draft, as used by libsmbc,
  since the workgroup would be the host field of the RI when browsing
  a workgroup (i.e. filtering hosts that are member of a given workgroup)
  because QUrl does not allow invalid hostnames in the host field.
  
  to bypass this problem we now put the workgroup name into the query of the
  url as `kio-workgroup`, should it cause trouble in the host field. SMBUrl
  takes this query into account when constructing the url for smbc.
  since the latter has uniquely exciting potential for breakage this entire
  dance is only done when absolutely necessary and otherwise we continue with
  all the same code and behavior as without this commit.
  
  on a side note: the awkward name flexibility seems to not extend to
  computer names anymore (supposedly because of LLMNR) so this entire
  use case is already very niche as we (and libsmbclient) currently only
  support workgroup browsing for NT1 networks, and NT1 is by default not
  supported on windows10 or samba.
  
  FIXED-IN: 20.04
  BUG: 204423

TEST PLAN
  builds, test passes, can browse workgroup with space in name

REPOSITORY
  R320 KIO Extras

BRANCH
  smb-smburl-static-autotest-ipv6-spaces

REVISION DETAIL
  https://phabricator.kde.org/D27804

AFFECTED FILES
  smb/autotests/smburltest.cpp
  smb/kio_smb_browse.cpp
  smb/smburl.cpp

To: sitter, ngraham
Cc: kde-frameworks-devel, kfm-devel, thiago, pberestov, iasensio, fprice, 
LeGast00n, cblack, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, 
meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov