All committers have signed the CLA.

@phil-davis do we have integration tests for public share link "Add to your owncloud" button? Mainly on the receiving side that mounts the public link share.

This is sending side, and in the box one need to provide receiving side:

https://github.com/owncloud/core/blob/master/tests/acceptance/features/webUISharingPublic2/shareByPublicLink.feature has scenarios that check the presence of the "add to your owncloud button". Steps like:

Then add to your owncloud button should be displayed on the webUI

But the scenarios do not go any further.

I will get someone to look at adding scenarios that go further an actually click the button!

I'm a bit unsure about this solution... there is a checkStorageAvailability below the code that could be used.
There could be 2 additional cases to check there, both exceptions coming from guzzle library:

• Invalid certificate
• Too many redirects

try {
  $storage->checkStorageAvailability();
} catch (TooManyRedirectsException $e) {
  // clean up and log
  \OCP\JSON::error(['data' => ['message' => .....]]);
} catch (RequestException $e) {
  // clean up and log
  \OCP\JSON::error(.....);
}
......

This way we make sure we're checking whatever we're going to use to access to the data without touching other endpoints.

Assuming it works properly (the checkStorageAvailability method has some exceptions being caught, so we need to ensure both exceptions are being thrown upwards), I think the intentions are clearer.
The drawback is that we might store broken information we might need to cleanup, but it shouldn't happen too often to be a real problem.

I think it's worthy to have a look because I still unsure about the reasons if having two checks instead of one.

BTW, make sure you link this PR to the issue in the top post (there is a "related issue" section in the template) so we can quickly access to the issue this PR fixes.
You also need to assign yourself to the PR, and apply at least the "2 - developing" or "3 - to review" labels (I think the rest are mostly for bugs)
Finally, make sure this PR is assigned to the right projects (mostly "ownCloud 10 server development") so it appears on the right board to keep it tracked.

jan seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.

Please, double-check this. You look suspicious otherwise.

From my point of view, you're expected to hit the target share in https://github.com/owncloud/core/pull/38107/files#diff-938f35dab4a346380cb32819e416003b9b6e19f1b8c6d4661ea6b9ccb15b89cdR98 , which should also be the same endpoint that will be used later on.

I think we can check the SSL certificate (if any) there. I expect the call to throw an "InvalidCertificateException" or "TooManyRedirectsException", both exceptions coming from the guzzle library, if there are problems with the SSL certificate, so we can catch those exceptions.
With this change, https://github.com/owncloud/core/pull/38107/files#diff-938f35dab4a346380cb32819e416003b9b6e19f1b8c6d4661ea6b9ccb15b89cdR75-R87 becomes obsolete and can be removed

Summing up the steps to reproduce the problem, it's something like:

1. user1 in serverA shares a file by link.
2. user2 (from serverB) access the link and add the shared file to his ownCloud (in serverB). Screenshot in This solves the issue, while generating a public link ssl check conne… #38107 (comment)
3. user2 in serverB access and accepts the share.

At step 3, there is a verification of the SSL certificate of the serverA. Such certificate could be invalid (expired, self-signed, not recognized), or we could end up in a redirection loop.

@pmaier1 is it fine to show a generic "Storage is not available" in that scenario instead of the current "Invalid or untrusted SSL certificate" (which isn't really accurate)?. We could clean some code and step 3 should be slightly faster because it's one request less to do.

@pmaier1 is it fine to show a generic "Storage is not available" in that scenario instead of the current "Invalid or untrusted SSL certificate" (which isn't really accurate)?. We could clean some code and step 3 should be slightly faster because it's one request less to do.

Well, a regular user can't do anything about it and in most cases will not understand SSL errors anyway. Best case you add some advice like "Please try again later or ask an administrator for help".

@jvillafanez
In this case, we just throw out the ssl pre-check and everything else is fine ?

@jvillafanez
In this case, we just throw out the ssl pre-check and everything else is fine ?

Yes, that's the idea. We need to verify it works though. In addition, it might be a good idea to rise the log messages to the error level so the admin knows what's happening

@jvillafanez user errors (e.g. wrong url) are not server errors and should be at most debug.

Following the expected flow, the user will need to enter the "serverB" url inside the "serverA" share link, but the remote url we'll ping is the one for "serverA". Basically, a user error shouldn't be possible.
If the user enters a wrong url in the "serverA" share link, the code will never reach this ajax/external.php file.

Taking this into account, if something breaks the admin will want to know. Even if it's a wrong url, the admin will want to know because a wrong url shouldn't be possible.

i think we are overengineering this simple issue, but at least would give good learning to new members :)

i think we are overengineering this simple issue, but at least would give good learning to new members :)

Yeah I think so, too :D

So I just removed the SSL pre-check and tested with an invalid SSL certificate. As @jvillafanez suggested the code still throws an error, thus we should be good to go with removing the SSL pre-check. It will throw a generic "Storage is not available" error.

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
No Duplication information

Note: #38109 (comment)
I found that there were already some automated test scenarios that do the whole "add to your ownCloud" UI workflow.
I added an extra scenario in PR #38117
All is good - this works.

Is log displayed with proper message why „storage is unavailable”?