Error when config file is not writable by Morikko · Pull Request #7205 · nextcloud/server

Morikko · 2017-11-17T11:00:46Z

Fix:

HTTP status returned is 500 Internal Server Error
Add a hint message that the server can allow config file to be read only with "config_is_read_only" set to true.

Notes:

My new hint message use l10n but is not translated in any other language than English
A first check is done in lib/base.php with function checkConfig(), this one is handle completely.
A second one in lib/private/legacy/util.php with function checkServer. This one only check the folder. Thus, it can throw an error if config folder is not writable but config file is writable. Moreover, the http status returned was already 503.

Questions:

Should I change the http status returned with the second check to 500 ?
Should we open a new issue to correct the behavior of the second check to focus on the file instead of the folder ?

The http status code is now 500 Internal Server Error when config file can't be written. #6893 Signed-off-by: Eric Masseran <[email protected]>

If the config.php is not writable, print an error message: #6893 - with HTTP status 500 - Set config writable - Or set option to keep it read only Signed-off-by: Eric Masseran <[email protected]>

codecov · 2017-11-17T11:27:24Z

Codecov Report

Merging #7205 into master will decrease coverage by 1.05%.
The diff coverage is 22.22%.

@@             Coverage Diff              @@
##             master    #7205      +/-   ##
============================================
- Coverage     51.91%   50.85%   -1.06%     
+ Complexity    25361    24541     -820     
============================================
  Files          1606     1584      -22     
  Lines         95311    93809    -1502     
  Branches       1394     1358      -36     
============================================
- Hits          49478    47704    -1774     
- Misses        45833    46105     +272

Impacted Files	Coverage Δ	Complexity Δ
lib/base.php	`3.06% <0%> (+0.91%)`	`167 <0> (-1)`	⬇️
lib/private/legacy/util.php	`58.48% <100%> (+0.18%)`	`224 <0> (-16)`	⬇️
apps/admin_audit/lib/Actions/GroupManagement.php	`0% <0%> (-100%)`	`4% <0%> (ø)`
...are/Security/Exceptions/AppNotEnabledException.php	`0% <0%> (-100%)`	`1% <0%> (ø)`
...ware/Security/Exceptions/NotConfirmedException.php	`0% <0%> (-100%)`	`1% <0%> (ø)`
apps/workflowengine/lib/Check/RequestUserAgent.php	`0% <0%> (-96.43%)`	`10% <0%> (-4%)`
apps/dav/lib/AppInfo/PluginManager.php	`0% <0%> (-71.93%)`	`31% <0%> (ø)`
apps/admin_audit/lib/Actions/AppManagement.php	`0% <0%> (-66.67%)`	`3% <0%> (ø)`
apps/files_sharing/lib/Helper.php	`20.35% <0%> (-58.96%)`	`38% <0%> (+27%)`
apps/comments/lib/Activity/Listener.php	`23.52% <0%> (-56.87%)`	`10% <0%> (ø)`
... and 668 more

MorrisJobke · 2017-11-17T12:43:53Z

lib/base.php

-					 [ $urlGenerator->linkToDocs('admin-dir_permissions') ])
+                    [ $urlGenerator->linkToDocs('admin-dir_permissions') ]) . '. '
+                    . $l->t('Or, if you prefer to keep config.php file read only, set the option "config_is_read_only" to true in it. See %s',
+                    [ $urlGenerator->linkToDocs('admin-config') ] )


Could you fix the indentation of that and one below. We use tabs

Signed-off-by: Eric Masseran <[email protected]>

LukasReschke · 2017-11-27T14:36:07Z

lib/base.php

 			} else {
+
+				header('HTTP/1.1 500 Internal Server Error');
+				header('Status: 500 Internal Server Error');


Go with http_response_code(500); instead.

Signed-off-by: Eric Masseran <[email protected]>

juliusknorr · 2018-04-18T10:06:54Z

@Morikko There is another check in lib/private/Config.php Could i ask you to adjust that as well? Otherwise the changes look good to me. 👍

Fixed

juliusknorr · 2018-04-18T10:10:46Z

My new hint message use l10n but is not translated in any other language than English

That is normal. Translations will be added though transifex after the PR is merged.

Should I change the http status returned with the second check to 500 ?

Yes, I'd say 500 makes more sense here.

Should we open a new issue to correct the behavior of the second check to focus on the file instead of the folder ?

Yes, we should probably consolidate all those config checks into one method that can then be used in the different places.

MorrisJobke · 2018-05-23T17:43:50Z

lib/private/legacy/util.php

+                    [ $urlGenerator->linkToDocs('admin-dir_permissions') ]) . '. '
+                    . $l->t('Or, if you prefer to keep config.php file read only, set the option "config_is_read_only" to true in it. See %s',
+                    [ $urlGenerator->linkToDocs('admin-config') ] )
+                );


Could you check the indentation of this, because it looks off. We usually use tabs for indenting lines. Beside that the changes look good 👍

MorrisJobke

Looks good except the one little nitpick.

skjnldsv · 2018-05-26T10:17:39Z

@juliushaertl you broke the sign-off ^^

skjnldsv · 2018-06-26T07:06:10Z

@Morikko could you rebase and drop the unsigned merge pr?

…instead of the actual resource * found while reviewing #7205 Signed-off-by: Morris Jobke <[email protected]>

MorrisJobke · 2018-06-26T07:22:20Z

lib/base.php

+				echo $l->t('See %s', [ $urlGenerator->linkToDocs('admin-config') ])."\n";
 				exit;
 			} else {
+				http_response_code(500);


I added this to the printErrorPage one in #9995

MorrisJobke · 2018-06-26T07:22:35Z

Let me take this one and create a new PR out of this.

MorrisJobke · 2018-06-26T07:38:57Z

@Morikko Sorry that we missed this for so long. I rebased your PR and squashed the commits into one (with you as Author). Thanks for this contribution and you are welcome to help out with other issues :)

See #9996 for the rebased version of this.

See https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22 for other good first issues ;)

Thanks a lot

…instead of the actual resource * found while reviewing #7205 Signed-off-by: Morris Jobke <[email protected]>

…instead of the actual resource * found while reviewing #7205 * allow to specify a special status code Signed-off-by: Morris Jobke <[email protected]>

Morikko · 2018-06-26T09:50:17Z

Thank you @MorrisJobke for merging this. I had started when I had more time few months ago. Now, I am still busy on some other projects to keep continuing.

…instead of the actual resource * found while reviewing #7205 * allow to specify a special status code Signed-off-by: Morris Jobke <[email protected]>

Morikko added 2 commits November 16, 2017 18:35

Change status code when config is write protected

2cee6ab

The http status code is now 500 Internal Server Error when config file can't be written. #6893 Signed-off-by: Eric Masseran <[email protected]>

Add hint msg, you can make config file read only

b243177

If the config.php is not writable, print an error message: #6893 - with HTTP status 500 - Set config writable - Or set option to keep it read only Signed-off-by: Eric Masseran <[email protected]>

MorrisJobke reviewed Nov 17, 2017

View reviewed changes

Correct indentation

fae80ce

Signed-off-by: Eric Masseran <[email protected]>

LukasReschke previously requested changes Nov 27, 2017

View reviewed changes

LukasReschke added 2. developing Work in progress enhancement labels Nov 27, 2017

Morikko and others added 2 commits November 29, 2017 19:59

Use http_response_code instead of header

3fa843a

Signed-off-by: Eric Masseran <[email protected]>

Merge branch 'master' into config-write-code-error

0bfd621

juliusknorr added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Apr 18, 2018

juliusknorr added this to the Nextcloud 14 milestone Apr 18, 2018

juliusknorr requested a review from MorrisJobke April 18, 2018 10:07

MorrisJobke reviewed May 23, 2018

View reviewed changes

MorrisJobke approved these changes May 23, 2018

View reviewed changes

skjnldsv approved these changes May 26, 2018

View reviewed changes

MorrisJobke added the stale Ticket or PR with no recent activity label Jun 19, 2018

nextcloud-bot removed the stale Ticket or PR with no recent activity label Jun 26, 2018

MorrisJobke added a commit that referenced this pull request Jun 26, 2018

Server error/hint pages with a 500 error code to avoid it being seen …

6186e83

…instead of the actual resource * found while reviewing #7205 Signed-off-by: Morris Jobke <[email protected]>

MorrisJobke mentioned this pull request Jun 26, 2018

Server error/hint pages with a 500 error code to avoid it being seen … #9995

Merged

MorrisJobke reviewed Jun 26, 2018

View reviewed changes

MorrisJobke mentioned this pull request Jun 26, 2018

Change status code when config is write protected #9996

Merged

MorrisJobke closed this Jun 26, 2018

MorrisJobke removed this from the Nextcloud 14 milestone Jun 26, 2018

MorrisJobke mentioned this pull request Jun 26, 2018

Status code should be 500 on error #6893

Closed

MorrisJobke added a commit that referenced this pull request Jun 26, 2018

Server error/hint pages with a 500 error code to avoid it being seen …

574aa04

…instead of the actual resource * found while reviewing #7205 Signed-off-by: Morris Jobke <[email protected]>

Uh oh!

Comments

Conversation

Morikko commented Nov 17, 2017

Uh oh!

codecov bot commented Nov 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

MorrisJobke Nov 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LukasReschke Nov 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juliusknorr commented Apr 18, 2018

Uh oh!

juliusknorr commented Apr 18, 2018

Uh oh!

MorrisJobke May 23, 2018

Choose a reason for hiding this comment

Uh oh!

MorrisJobke left a comment

Choose a reason for hiding this comment

Uh oh!

skjnldsv commented May 26, 2018

Uh oh!

skjnldsv commented Jun 26, 2018

Uh oh!

MorrisJobke Jun 26, 2018

Choose a reason for hiding this comment

Uh oh!

MorrisJobke commented Jun 26, 2018

Uh oh!

MorrisJobke commented Jun 26, 2018

Uh oh!

Morikko commented Jun 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov bot commented Nov 17, 2017 •

edited

Loading

MorrisJobke Nov 17, 2017 •

edited

Loading

LukasReschke Nov 27, 2017 •

edited

Loading