Support username, password and port specification in RIA URLs#5902
Support username, password and port specification in RIA URLs#5902mih merged 3 commits intodatalad:maintfrom
Conversation
Previously only the hostname was retained for performing, e.g. SSH-based logins, making it required to use an appropriate SSH config. While not using such properties in RIA URLs is advisable, silently stripping such information is a bug. Moreover, other mechanisms such as HTTP (or a future FTP support) may not have equivalent configuration analogs. Using such a core helper as `verify_ria_url()` to silently remove information seems inappropriate. A more comprehensive test is included. Fixes datalad#5475
Codecov Report
@@ Coverage Diff @@
## maint #5902 +/- ##
===========================================
- Coverage 90.32% 36.09% -54.23%
===========================================
Files 300 297 -3
Lines 42396 42366 -30
===========================================
- Hits 38294 15292 -23002
- Misses 4102 27074 +22972
Continue to review full report at Codecov.
|
| hostname=url_ri.hostname or '', | ||
| portdelim=':' if url_ri.port else '', | ||
| port=url_ri.port or '', | ||
| ) |
There was a problem hiding this comment.
not familiar with RIA enough and can't grasp from the docstring -- what is the purpose/use of this returned host entity?
it seems no longer a URL (no protocol:// prefix) and also might contain sensitive information (password) thus should probably not be logged anywhere (although I just now realized that we might be logging such URLs left and right) and should not be served as some kind of a "annex remote description".
is that a host to ssh into? (I guess not since IIRC you can't even add :port not to say password like that)
There was a problem hiding this comment.
is that a host to ssh into?
If it's SSH, then yes. Will be passed to our SSH machinery in that case.
Overall it depends: It's the thing the base_path of the RIA store is referring to.
There was a problem hiding this comment.
I guess not since IIRC you can't even add :port not to say password like that
Thx @yarikoptic for making this point. I think it is better to switch to URL style destination description right away to actually make this possible. I will push an update.
bpoldrack
left a comment
bpoldrack
left a comment
There was a problem hiding this comment.
Actually, I was planning to use the parsed URL within ORA instead, but I'm fine with that change - it's the easier quick fix and the added test helps. Also doesn't stand in the way of overall RF'ing plans - so: Thanks much!
| hostname=url_ri.hostname or '', | ||
| portdelim=':' if url_ri.port else '', | ||
| port=url_ri.port or '', | ||
| ) |
There was a problem hiding this comment.
is that a host to ssh into?
If it's SSH, then yes. Will be passed to our SSH machinery in that case.
Overall it depends: It's the thing the base_path of the RIA store is referring to.
This is used to log into machines (SSH only right now). Only this form actually allows for the specification of a port within the RIA URL. Given that a URL is the main input, it makes sense to maintain such a form also within subsequent internal handling.
|
Thx for the review @bpoldrack |
3 participants
Previously only the hostname was retained for performing, e.g. SSH-based
logins, making it required to use an appropriate SSH config.
While not using such properties in RIA URLs is advisable, silently
stripping such information is a bug. Moreover, other mechanisms such
as HTTP (or a future FTP support) may not have equivalent configuration
analogs. Using such a core helper as
verify_ria_url()to silentlyremove information seems inappropriate.
A more comprehensive test is included.
Fixes #5475