fix: support federation between systems in subdirectories - #41599
Conversation
testRemoteUrl was only checking for a responding ownCloud at the provided host. But if the server is installed in a subdirectory then the requests were not sent there. For example: - the cloud server is at http://mydomain.com/cloud - testRemoteUrl checked for a repsonse from http://mydomain.com/status.php Now it checks for a response from http://mydomain.com/cloud/status.php
|
I found this problem because in the acceptance test CI with GitHub workflows I have 2 ownCloud core servers running. One is at I had to make this fix/enhancement so that the two core servers would find each other. I imagine that this would also be a problem for a production site that had cloud storage servers at locations like: somedomain.com.au/sydneycloud otherdomain.com/chicagocloud and tried to make federated connections between them. |
DeepDiver1975
left a comment
There was a problem hiding this comment.
Code Review — fix: support federation between systems in subdirectories
Overview: Fixes testRemoteUrl to include the URL path component when constructing the remote URL to test, so servers installed in subdirectories (e.g. mydomain.com/cloud) are correctly probed at mydomain.com/cloud/status.php instead of mydomain.com/status.php.
Correctness
The fix is correct for the stated case. Both the with-scheme and without-scheme branches (if/else) are updated consistently, and the if ($parsed_path !== null) guard correctly skips appending when there is no path.
Edge case: trailing slash on path
If $remote is mydomain.com/cloud/ (trailing slash), parse_url returns $parsed_path = '/cloud/'. After appending, $remote becomes mydomain.com/cloud/. The downstream testUrl call then constructs https://mydomain.com/cloud//status.php (double slash), which may fail depending on the remote server's handling of double slashes.
Suggested guard in both branches:
if ($parsed_path !== null) {
$remote .= \rtrim($parsed_path, '/');
}Tests
Good use of @dataProvider — reusing providesRemoteAddress across both testRemoteWithValidHttps and testRemoteWithWorkingHttp is clean. Consider adding a trailing-slash case to the provider ('owncloud.com/cloud/') to cover the edge case above.
Changelog
Present and correctly formatted. ✅
Summary
| Aspect | Assessment |
|---|---|
| Correctness | ✅ Fixes the stated bug |
| Edge cases | Minor: trailing slash on path produces double slash in URL |
| Tests | Good coverage; one additional case recommended |
| Changelog | ✅ Present |
Verdict: Nearly ready to merge. The trailing-slash edge case is minor but easy to fix with rtrim.
|
@phil-davis one minor review item - thank you |
|
I edited the PR title to start "bugfix" because I saw that #41603 seems to require "Bugfix" in the changelog. Are there different rules for what goes in the changelog, and what goes in commit messages? |
|
Don't get confused with calens and semantic commit messages. But "fix:" is correct for the PR title |
DeepDiver1975
left a comment
There was a problem hiding this comment.
Code Review — fix: support federation between systems in subdirectories
Overview: testRemoteUrl() in External\Manager was discarding the URL path when constructing the probe URL, so a server at mydomain.com/cloud was probed at mydomain.com/status.php instead of mydomain.com/cloud/status.php. The fix preserves the path component.
Correctness
The fix adds $parsed_path extraction and appends it (with trailing slash stripped) to the reconstructed host string. The same pattern is applied in both the if (\is_string($parsed_host)) branch (scheme-bearing URLs) and the else branch (bare host/path strings prepended with http://). Symmetric fix. ✅
\rtrim($parsed_path, '/'): Prevents double slashes when calens appends /status.php downstream. ✅
$parsed_path !== null guard: parse_url returns null for the path component when there is no path. The guard prevents concatenating a null. ✅
Edge case — path /: A URL like http://mydomain.com/ yields $parsed_path = '/', which after rtrim(..., '/') becomes ''. This is correct — the root path contributes nothing to the base. ✅
Tests
providesRemoteAddress() adds three cases:
'owncloud.com'— bare host (existing behaviour) ✅'owncloud.com/cloud'— host + path without trailing slash ✅'owncloud.com/cloud/'— host + path with trailing slash (tests the rtrim) ✅
Both testRemoteWithValidHttps and testRemoteWithWorkingHttp are converted to data-provider tests covering all three cases. ✅
Summary
| Aspect | Assessment |
|---|---|
| Path preserved in probe URL | ✅ Both URL branches fixed symmetrically |
| Trailing slash stripped | ✅ rtrim prevents double slash |
| Null path guard | ✅ Correct |
| Tests | ✅ Three cases including trailing-slash variant |
| Changelog | ✅ Present |
Verdict: Ready to merge.
testRemoteUrl was only checking for a responding ownCloud at the provided host. But if the server is installed in a subdirectory then the requests were not sent there. For example:
Now it checks for a response from http://mydomain.com/cloud/status.php