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

Disable move function on shared folder, if target is out of scope of shared root folder #40881

Open
GoWeasel opened this issue Oct 11, 2023 · 10 comments
Labels
0. Needs triage Pending check for reproducibility or if it fits our roadmap enhancement feature: sharing

Comments

@GoWeasel
Copy link

GoWeasel commented Oct 11, 2023

How to use GitHub

  • Please use the 👍 reaction to show that you are interested into the same feature.
  • Please don't comment if you have no relevant information to add. It's just extra noise for everyone subscribed to this issue.
  • Subscribe to receive notifications on status change and new comments.

Is your feature request related to a problem? Please describe.

Example usecase:

  • You have a shared folder [Team 1] for 5 people,
    and everybody should be able to edit/rename/update/delete on all files in this share.
    The share further is not syncable (protected by hidden-Tag and "file access control")

  • Then u have a further shared folder [Team 2] for 3 people,
    and also everybody should be able to edit/rename/update/delete on all files in this share.
    The share further is not syncable (protected by hidden-Tag and "file access control")

  • Further all 8 people also have their own folders in user space (unlimted)

Then u got a problem with the "moveFileFunction", as everbybody is able to move files from shared folder to his own root folder (in user space) or to a subfolder (in user space). Then further from his user space, he can do everything, also sync!, what is in upper usecase not allowed.

Actually i found no way to protect it, also tried things like "File access control" and "Group folders".

Describe the solution you'd like

  • A user and shared permission based solution, which allow either move operation out of shared-root scope or forbid it.

Describe alternatives you've considered

  • For the moment i build a simple, alpha like, solution which just take care that the targetPath of the moveFileFunction call, is valid to the RootPath of the share. (it works with just one file to be edited)

Additional context
Here some examples:

@martin-77
Copy link

any news on this? Will it be added to the roadmap? I think this is an essential feature we need to have!

@GoWeasel
Copy link
Author

GoWeasel commented Dec 12, 2023

Short Update:

just updated my test server from 27.1.4 to latest 28.0.0 and the result is:

Vue.js is faster = nice, but (as expected) also bringing back old bugs....same problem as with 27.1.4...i'll fix it again by my self.

Further thanks for a not working upgrade progress (neither by webui or occ upgrade) from 27.1.4 to 28.0.0, never see such a way to upgrade, what a nightmare, sorry but that rly bad.

@GoWeasel
Copy link
Author

Intresting behavior by testing my old way to fix,

i change the filelist.js with my fixed edition, and spooki things happening

  • as normal user:
    file move to other root path is not allowed as expected, but a different error message i got (not mine!!!)

  • as normal user + admin rights:
    file move to other root path is not allowed as expected, but file is moved, and get the right error message (mine)

  • as nc_admin (default admin) -> get NONE error message and file is just moved

@rohit1dhiman
Copy link

@GoWeasel can you share GIT link of your solution, i am also facing similar problem

@GoWeasel
Copy link
Author

GoWeasel commented Apr 9, 2024

@GoWeasel can you share GIT link of your solution, i am also facing similar problem

i'll prepare it after testing on latest update, as the problem is old, i need to re-check it first, in new version.

@rohit1dhiman
Copy link

@GoWeasel can you share GIT link of your solution, i am also facing similar problem

i'll prepare it after testing on latest update, as the problem is old, i need to re-check it first, in new version.

I need that for non productive usage, doing some POC, so need it for a quick turnaround, share me even no tested version, will help me to draw some inspiration , sorry for pushing 😁😁😁😁😁

@GoWeasel
Copy link
Author

GoWeasel commented Apr 9, 2024

Modified file: /var/www/nextcloud/apps/files/js/filelist.js

you find the needed part at lines:
image

That was the rudimentary way to fix in NC 27.1.2 from start of issue, as mentioned in NC 28 upper, there was something not as expected, but hopefully u can work out a nice way for the future.

I marked the changed areas for you in MODIFICATION start/end, hope you can find inspiration for something useful with this rudimentary and actually untested way

Basically it is just a root-path comparism between source and target,
if not matching = forbidden, if matching = allowed.

Very simple, but effective.

I wish some one implement such feature in a prof. way, as without, NC is in my case not applicable in any mid business environment, concerning new directives coming up in europe.

I have not enough time for it - at moment, too much focused on rust, as security matters.

		/**
		 * Moves a file to a given target folder.
		 *
		 * @param fileNames array of file names to move
		 * @param targetPath absolute target path
		 * @param callback function to call when movement is finished
		 * @param dir the dir path where fileNames are located (optional, will take current folder if undefined)
		 */
		move: function(fileNames, targetPath, callback, dir) {
			var self = this;

			dir = typeof dir === 'string' ? dir : this.getCurrentDirectory();
			if (dir.charAt(dir.length - 1) !== '/') {
				dir += '/';
			}

			var target = OC.basename(targetPath);
			if (!_.isArray(fileNames)) {
				fileNames = [fileNames];
			}

// MODIFICATION START

			// Source -> Target ? Check RootPath
			var rootPath = dir.substring(0,dir.indexOf("/",1))

			console.log("SourcePath: " + dir.toString());
			console.log("TargetPath: " + targetPath.toString());
			console.log("Allowed RootPath: " + rootPath);

			// If targetPath starts with RootPath == true, then move is allowed, else forbidden
			if (targetPath.toString().startsWith(rootPath) === true ){
				console.log("Allowed - Move on same RootPath structure");

				var moveFileFunction = function(fileName) {
					var $tr = self.findFileEl(fileName);
					self.showFileBusyState($tr, true);
					if (targetPath.charAt(targetPath.length - 1) !== '/') {
						// make sure we move the files into the target dir,
						// not overwrite it
						targetPath = targetPath + '/';
					}

					return self.filesClient.move(dir + fileName, targetPath + fileName)
						.done(function() {
							// if still viewing the same directory
							if (OC.joinPaths(self.getCurrentDirectory(), '/') === OC.joinPaths(dir, '/')) {
								// recalculate folder size
								var oldFile = self.findFileEl(target);
								var newFile = self.findFileEl(fileName);
								var oldSize = oldFile.data('size');
								var newSize = oldSize + newFile.data('size');
								oldFile.data('size', newSize);
								oldFile.find('td.filesize').text(OC.Util.humanFileSize(newSize));
								self.remove(fileName);
							}
						})
						.fail(function(status) {
							if (status === 412) {
								// TODO: some day here we should invoke the conflict dialog
								OC.Notification.show(t('files', 'Could not move "{file}", target exists',
									{file: fileName}), {type: 'error'}
								);
							} else {
								OC.Notification.show(t('files', 'Could not move "{file}"',
									{file: fileName}), {type: 'error'}
								);
							}
						})
						.always(function() {
							self.showFileBusyState($tr, false);
						});
				};

			} else{
				console.log("Forbidden - Move in other RootPath is not allowed");
				OC.Notification.show(t('files', 'Forbidden - Move in other RootPath is not allowed'));

			}

			return this.reportOperationProgress(fileNames, moveFileFunction, callback).then(function() {
				self.updateStorageStatistics();
				self.updateStorageQuotas();
			});
		},
// MODIFICATION END
		_reflect: function (promise){
			return promise.then(function(v){ return {};}, function(e){ return {};});
		},

		reportOperationProgress: function (fileNames, operationFunction, callback){
			var self = this;
			self._operationProgressBar.showProgressBar(false);
			var mcSemaphore = new OCA.Files.Semaphore(5);
			var counter = 0;
			var promises = _.map(fileNames, function(arg) {
				return mcSemaphore.acquire().then(function(){
					return operationFunction(arg).always(function(){
						mcSemaphore.release();
						counter++;
						self._operationProgressBar.setProgressBarValue(100.0*counter/fileNames.length);
					});
				});
			});

			return Promise.all(_.map(promises, self._reflect)).then(function(){
				if (callback) {
					callback();
				}
				self._operationProgressBar.hideProgressBar();
			});
		},

@GoWeasel
Copy link
Author

GoWeasel commented Apr 9, 2024

As expected, just tested on NC 28.04, and something changed, does not work any more 👎

@rohit1dhiman
Copy link

thanks @GoWeasel for your support, let me try this, at least i have something to start with. thanks a ton for your quick help and response

@martin-77
Copy link

this function is as basic as essential - still do not understand, why this is not part of nextcloud.
Let´s try upvoting in the related help.nextcloud-thread...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0. Needs triage Pending check for reproducibility or if it fits our roadmap enhancement feature: sharing
Projects
None yet
Development

No branches or pull requests

4 participants