Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add appconfig to disable fixed userfolder permissions optimization #51145

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

icewind1991
Copy link
Member

@icewind1991 icewind1991 commented Feb 28, 2025

Currently, to save having to fetch the metadata of the home folder in many cases, we always assume the same value for the permissions of the home folder.

This does however break if the admin has configured an external storage mounted at / with different permissions.

To fix those cases without breaking the optimization for other instances, this adds a flag in the appconfig to disable the optimization and actually fetch the correct permissions.

This flag is automatically set by the files_external app when such an external storage is detected

To test:

  • setup an external storage with mountpoint / and set as readonly
  • check if the webui suggests that the home folder is writable

@icewind1991 icewind1991 added the 3. to review Waiting for reviews label Feb 28, 2025
@icewind1991 icewind1991 added this to the Nextcloud 32 milestone Feb 28, 2025
@icewind1991 icewind1991 requested review from a team, skjnldsv and yemkareems and removed request for a team February 28, 2025 16:49
@icewind1991 icewind1991 requested a review from artonge as a code owner February 28, 2025 16:49
@icewind1991 icewind1991 requested a review from sorbaugh February 28, 2025 16:49
@icewind1991 icewind1991 force-pushed the home-folder-readonly branch 2 times, most recently from 02aa7cf to 0b62c0e Compare February 28, 2025 17:08
Copy link
Contributor

@artonge artonge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels like a hack, but I guess there is no better way to do it.
Is that optimization really worth it?

/**
* Check if any mountpoint is configured that overwrite the home folder
*
* @return bool
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessary?

@@ -86,6 +87,10 @@ abstract class StoragesServiceTest extends \Test\TestCase {
* @var \PHPUnit\Framework\MockObject\MockObject|IEventDispatcher
*/
protected IEventDispatcher $eventDispatcher;
/**
* @var \PHPUnit\Framework\MockObject\MockObject|IAppConfig
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @var \PHPUnit\Framework\MockObject\MockObject|IAppConfig
* @var \PHPUnit\Framework\MockObject\MockObject&IAppConfig

@@ -46,6 +47,8 @@ abstract class NodeTest extends \Test\TestCase {
protected $eventDispatcher;
/** @var ICacheFactory|\PHPUnit\Framework\MockObject\MockObject */
protected $cacheFactory;
/** @var IAppConfig|\PHPUnit\Framework\MockObject\MockObject */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/** @var IAppConfig|\PHPUnit\Framework\MockObject\MockObject */
/** @var IAppConfig&\PHPUnit\Framework\MockObject\MockObject */

@@ -39,6 +40,8 @@ class RootTest extends \Test\TestCase {
private $eventDispatcher;
/** @var ICacheFactory|\PHPUnit\Framework\MockObject\MockObject */
protected $cacheFactory;
/** @var IAppConfig|\PHPUnit\Framework\MockObject\MockObject */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/** @var IAppConfig|\PHPUnit\Framework\MockObject\MockObject */
/** @var IAppConfig&\PHPUnit\Framework\MockObject\MockObject */

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants