Allow configuring the location of the `appdata_[instanceid]` folder

[summersab]

trafficstars

Allow configuring the location of the appdata_[instanceid] folder

• Resolves: #12873

Summary

This allows administrators to change the location of the appdata_[instanceid] folder by specifying an appdataroot value in config.php. This is especially useful when using shared storage, object storage as primary storage (storing appdata_[instanceid] in an object store is extremely slow), etc.

TODO

Resolved with commit 783ad06

• [x] Regardless of appdataroot being set, the function should really return $rootFolder and not $rootFolder->get($folderName). The current code will store files in /appdata_[instanceid]/appdata_[instanceid] when an appdataroot is provided and /appdata_[instanceid] in normal operation (i.e. it will create a /appdata_[instanceid] folder on the mount point instead of writing appdata folders to the mount point itself). Some apps such as OnlyOffice expect the appdata_[instanceid] folder to be a direct child of system root: https://github.com/ONLYOFFICE/onlyoffice-nextcloud/blob/504884f16ce2b66e45a66579aa3cdf0fd0197330/lib/templatemanager.php#L53

~~However, I am having permission issues. The system is able to create files and folders at the mount point, but when it tries to read the folders, they don't appear to exist, and errors are thrown. I think it has something to do with the OC\FilesView attached to the mount point/folder, but I'm not sure. It seems to occur at this function:~~ ~~https://github.com/nextcloud/server/blob/75d7203f579f5761fc921d9506579bec48ebab74/lib/private/Files/Node/Folder.php#L161~~

If that doesn't make sense, please feel free to ask me to explain in further detail.

Checklist

• Code is properly formatted
• Sign-off message is added to all commits
• [ ] Tests (unit, integration, api and/or acceptance) are included
• [ ] Screenshots before/after for front-end changes
• [ ] Documentation (manuals or wiki) has been updated or is not required
• [ ] Backports requested where applicable (ex: critical bugfixes)

[kesselb]

Thanks :+1:

Can you have a look at tests/lib/Files/AppData/AppDataTest.php?

[summersab]

@kesselb - I hope I did that right? I've never made a commit to a project as big as this.

I see some of the other tests are failing. What else do I need to do?

[summersab]

Does anyone know how to fix the issue of being able to write directly to the mount point instead of having to create a directory inside of the mount? It's some sort of permission issue. Without fixing it, anyone who uses this feature will have apps like OnlyOffice break.

[summersab]

I hate to tag everyone, but I need a little help to get this working. @ArtificialOwl @icewind1991 @blizzz @kesselb @tcitworld

I noticed that by default, the appdata folder has permissions of: OC::$server->getRootFolder()->get('/appdata_[instanceid]')->getPermissions() == 31

After I add my mount point, I checked the same location (which is now the mount point of my storage instead of just a folder inside /), and I get permissions of 23. So, I tried adding a storage wrapper to the mount point like this. However, I STILL get permissions of 23. What am I doing wrong?

$this->rootFolder = new LazyRoot(function () use ($rootFolder, $systemConfig) {
	return $rootFolder;
	if ($appdataroot = $systemConfig->getValue('appdataroot', null)) {
		$instanceId = $systemConfig->getValue('instanceid', null);
		if ($instanceId === null) {
			throw new \RuntimeException('no instance id!');
		}

		$folderName = 'appdata_' . $instanceId;
		$storageFactory = new \OC\Files\Storage\StorageFactory();
		$storageFactory->addStorageWrapper('permissions_all', function ($mountPoint, IStorage $storage, IMountPoint $mount) {
			return new PermissionsMask([
				'storage' => $storage,
				'mask' => Constants::PERMISSION_ALL,
			]);
			return $storage;
		});

		$arguments = [
			'datadir' => $appdataroot,
		];
		$storage = new LocalRootStorage($arguments);
		$mount = new MountPoint($storage, $folderName, $arguments, $storageFactory);

		\OC::$server->getMountManager()->addMount($mount);

		// This is what ought to be returned - the root for AppData should be the actual root, not a folder inside the root
		return $rootFolder;//->get($folderName);
	}
	return $rootFolder;
});

[summersab]

I am still having trouble getting the mount point to have the correct permissions. I have tried a number of different methods, and this is one of them. Take note of the perm values at the end. How do I get the /appdata_[instanceid]/ mount point to have permissions of 31 and operate as if it were a folder?

$this->rootFolder = new LazyRoot(function () use ($rootFolder, $systemConfig) {
	if ($appdataroot = $systemConfig->getValue('appdataroot', null)) {
		$instanceId = $systemConfig->getValue('instanceid', null);
		if ($instanceId === null) {
			throw new \RuntimeException('no instance id!');
		}

		$folderName = 'appdata_' . $instanceId;
		\OC\Files\Filesystem::initInternal($folderName);
		$arguments = [
			'datadir' => $appdataroot,
		];

		$mount = new MountPoint(LocalRootStorage::class, '/' . $folderName, $arguments, \OC::$server->getStorageFactory());
		\OC::$server->getMountManager()->addMount($mount);

		// Evaluates to 31
		$perm1 = $mount->getStorage()->getPermissions('');
		
		// Evaluates to 31; functionally the same as above
		$perm2 = \OC::$server->getMountManager()->getAll()["/$folderName/"]->getStorage()->getPermissions('');

		// Evaluates to 23. As such, `newFolder` will be able to create folders at the mount point, but `getFolder` cannot access them
		$perm3 = $rootFolder->get("/$folderName/")->getPermissions();
	}

	return $rootFolder;
}

[nickvergessen]

Something went wrong with your rebase.

Try:

git checkout master
# Replace nextcloud with the name of the remote, if you don't know, post the result of `git remote -v`
git pull nextcloud master
git checkout appdata_location
git rebase -i nextcloud/master
# Remove all commits which are not yours, then save and exit
git push --force origin appdata_location

[summersab]

Something went wrong with your rebase.

Yeah, I'm not the best with git, yet. I think I fixed it.

My latest commit also fixes the issue I was having trouble with as described in my initial comment (being unable to use the root of the new mount as appdata_[instanceid]. It was a issue with the caching and LocalRootScanner. In short, the PR now works the way it should have from the start.

[summersab]

I'm not sure that the errors in CI are related to me, and I'm not sure about the performance-8,0 check. Happy to fix them, though!

[jonas-w]

@summersab i don't know much about the nextcloud CI/CD nor php, but i'm watching this issue as i want this feature too. I see that you are 334 commits behind, maybe the CI issues were already fixed in master, you would need to rebase your branch to master. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

[summersab]

Thanks for the comment, @jonas-w, and I'm glad to be helping someone else who wants this feature.

I just merged and rebased, but now, I have two commits. I'm pretty sure one of the tests requires that there be only one commit. I don't think I want to squash my commit and the merged commit from upstream, though. I'm a novice with git, so I don't want to do much more and mess things up.

Edit: I think I might have made things worse. I hate to ask @nickvergessen for help, but . . . help? The CI and performance-8.0 checks were failing, so I rebased on master hoping that might make it pass the checks.

[szaimen]

@summersab I can take care of rebasing if you want... (a merge commit is not a rebase)

edit: done

[szaimen]

I had a look at the tests and https://github.com/nextcloud/server/actions/runs/4226966799/jobs/7340960268 and https://github.com/nextcloud/server/actions/runs/4226966804/jobs/7340960157 and https://github.com/nextcloud/server/actions/runs/4226966790/jobs/7340960261 definitely seem to be caused by this PR. But https://github.com/nextcloud/server/actions/runs/4226966796/jobs/7340959942 is not.

[summersab]

@szaimen - looks like I got all of the tests to pass except performance-8.0. It's giving the error:

Fetching submodule 3rdparty
fatal: remote error: upload-pack: not our ref

I didn't change anything in 3rdparty. Also, I usually avoid submodules in my projects, so I'm not sure what is required to fix this.

[szaimen]

Fetching submodule 3rdparty fatal: remote error: upload-pack: not our ref

this one seems to be unrealted. Thanks for fixing the tests!

[susnux]

Your branch is broken, something with the rebase gone wrong.

[summersab]

Got it fixed. I forgot to update my fork from upstream before rebasing. I'm such a git amateur.

@susnux, you think you could put this in a queue for review?

[szaimen]

Why is this still wip? Seems like tests are passing?

[summersab]

Why is this still wip? Seems like tests are passing?

@szaimen, I made it a draft because I suck at rebasing, and I didn't want to send notifications to the world. This should be good to merge, though - it's been ready for approval for awhile (in my opinion).

[summersab]

Rebased on the latest master. Should be good to go.

Any chance of this getting merged in time for v27?

[kesselb]

Any chance of this getting merged in time for v27?

Sorry, the time frame for 27 is already over.

[summersab]

Sorry, the time frame for 27 is already over.

Okay, what is the process of getting this merged regardless of the v27 release?

[kesselb]

Okay, what is the process of getting this merged regardless of the v27 release?

If time allows, someone will look into your pull request.

[summersab]

Hmm - not sure why CI is angry with me. It looks like there might have been a network blip when trying to build?

cypress-summary doesn't provide anything useful, and the two runners are throwing errors on an EventDispatcher instance (which my PR doesn't touch): Error: Call to undefined method OC\Server::getEventDispatcher() in /var/www/html/console.php:94

[imchasingshadows]

Is there still work that needs to be done here? Seems like this has been idle for quite some time.

[susnux]

Is there still work that needs to be done here? Seems like this has been idle for quite some time.

See the unresolved comments above, so yes there is work to be done. But feel free to fork and contribute on the feature :)

[skjnldsv]

Closing this pull request due to lack of recent activity and updates. We appreciate your contribution and encourage you to reopen or provide further updates if necessary. :relaxed: Our aim is to keep the project moving forward with active collaboration. Thank you for your understanding and continued support! :pray:

[luxzg]

Wait, you closed an issue due to inactivity, issue that had already started development and people are actively waiting for it? If you want activity I will repost comment here every week so issue is "active". I don't think that's what is expected, and is usually considered spam on GitHub, but after this closing it seems this project has different code of conduct - "spam us if you want the feature".

Please be reasonable and keep the issue open until it is finished, reviewed and accepted.

It was planned for NC 26, then 27, 28, 29... I have no words...

[ChristophWurst]

It was planned for NC 26, then 27, 28, 29...

For your information: assigning to a milestone is not a sign of planning. All PRs are moved to the next milestone after the branch-off. This PR is waiting for the author or someone else to finish the work, then it can be merged. The requested changes had been pending for half a year.

[luxzg]

Ok, sorry for misinterpreting the closing comment, it was just for this commit which indeed got stale, not the original issue. Hopefully someone will step up to finish the needed work.