fix: Better output in case dataDirectory is invalid

[d7oc]

Description

If the dataDirectory is not correct there is currently only the message Your Data directory is invalid without specifying which directory is currently set. This small PR aims to add more output in this case.

Motivation and Context

It solved the missing output in case something is wrong with the data directory.

How Has This Been Tested?

• Local test

Screenshots (if appropriate):

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Database schema changes (next release will require increase of minor version instead of patch)
• Breaking change (fix or feature that would cause existing functionality to change)
• Technical debt
• Tests only (no source changes)

Checklist:

• Code changes
• Unit tests added
• Acceptance tests added
• Documentation ticket raised:
• Changelog item, see TEMPLATE

[update-docs]

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

[jvillafanez]

Could this be considered a security risk since we're leaking internal information?. Assuming the directory is invalid, anyone (maybe only registered users) could know where the data directory for ownCloud is located.

[DeepDiver1975]

Could this be considered a security risk since we're leaking internal information?. Assuming the directory is invalid, anyone (maybe only registered users) could know where the data directory for ownCloud is located.

Absolutly - this error is exposed via the browser and we shall never ever expose internal paths.

[DeepDiver1975]

writing to log as an alternative is a no-go as we cannot write to data dir.
error_log() might be an alternative - error will go into the webserver error log then

[d7oc]

Will error_log() also be visible when executing occ? I mainly need it in case that occ is executed, wasn't even aware that this shows up in the browser as well.

[DeepDiver1975]

Will error_log() also be visible when executing occ?

no

[DeepDiver1975]

I mainly need it in case that occ is executed, wasn't even aware that this shows up in the browser as well.

you can add a condition and use your error message if executed via occ

if (\OC::$CLI) {
 # TODO: detailed message
}

[DeepDiver1975]

Or is this when executing occ check ?

[d7oc]

No it happened directly when calling occ. Will go with the if condition in this case.

[d7oc]

Updated the commit by force push to the branch. The output now looks like the following for my specific case:

root@owncloud-675d799b87-8mb4v:/var/www/owncloud# occ
Value of Data directory was: ""
Your Data directory must be an absolute path
Check the value of "datadirectory" in your configuration

Your Data directory is invalid
Please check that the data directory contains a file ".ocdata" in its root.

Cannot create "data" directory
This can usually be fixed by <a href="https://doc.owncloud.com/server/10.11/go.php?to=admin-dir_permissions" target="_blank" rel="noreferrer">giving the webserver write access to the root directory</a>.

An unhandled exception has been thrown:
Exception: Environment not properly prepared. in /var/www/owncloud/lib/private/Console/Application.php:139
Stack trace:
#0 /var/www/owncloud/console.php(115): OC\Console\Application->loadCommands()
#1 /var/www/owncloud/occ(11): require_once('/var/www/ownclo...')
#2 {main}root@owncloud-675d799b87-8mb4v:/var/www/owncloud#

[ownclouders]

💥 Acceptance tests pipeline apiShareReshareToShares1-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/37810/63

[mmattel]

@phil-davis can you have a look why the build gets constantly killed by CI?

test-javascript
yarn-install
Pulling from owncloudci/nodejs
Step is killed (canceled)

[phil-davis]

#40482 (comment)
https://drone.owncloud.com/owncloud/core/37803/38/14

Running apiCapabilities tests tagged ~@skipWhenTestingRemoteSystems&&~@skipOnOcVValue of Data directory was: /drone/src/data 10&&~@skipOnOcVValue of Data directory was: /drone/src/data 10.12&&~@skipOnOcVValue of Data directory was: /drone/src/data 10.12.0&&~@local_storage&&~@files_external-app-required&&@api&&~@skip 

For some reason there is output "Value of Data directory was: /drone/src/data" mixed in with the output about test tags.
"Something" is happening with occ commands at some point in the system setup that is emitting these messages. That needs investigation, because we should not be triggering the new message, but we are somehow.

I restarted CI to confirm the problem.

[phil-davis]

The problem is;

$ php occ config:system:get version
Value of Data directory was: '/home/phil/git/owncloud/core/data'
10.12.0.1

That command is giving "bonus" new output.

[phil-davis]

I adjusted the code so that the Data directory message is only emitted if it has a problem.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication

[phil-davis]

@DeepDiver1975 @jvillafanez please review and comment. If this is OK to do, then I can add a changelog...

[jvillafanez]

I'd vote for writing the message in the log instead of printing it. Printing messages should be the responsibility of a class as close to the user as possible (one of the Command for example).

The problem is;

$ php occ config:system:get version
Value of Data directory was: '/home/phil/git/owncloud/core/data'
10.12.0.1

That command is giving "bonus" new output.

That's the side effect of printing anywhere. Not only it shows irrelevant information to the command, but also it could break 3rd party things (I could use that command to get the version and send an email about the update).

If logging isn't possible, I don't know if it's possible to send a 3rd key in the errors. I mean, I assume the "hint" is text that will be displayed in the web, and maybe "error" is a generic error text, so we could add a "cli" key for the error that should be displayed in the command line.

[phil-davis]

@pako81 should anyone look at this for 10.13?
I see that @jvillafanez commented back in Feb 2023. But who would make decision, and who codes it?

[pako81]

We can try to get this into 10.13. No high priority but still nice to have. @d7oc would you be able to implement the requested change (logging it instead of printing it) ?

[d7oc]

If \OCP\Util::writeLog can be used for that similar to

core/lib/private/legacy/util.php

Line 398 in 8e3d093

\OCP\Util::writeLog(

I could adjust the PR.