DevHeads.net

Review Request: Copy files instead of moving if parent dir is not writable

Review request for KDE Runtime and Plasma.

Description
When adding an application resource to a private activity kactivitymanager tries to move the resource's .desktop file to the activity's private folder. The new .desktop file is created successfully but the source file is not deleted if the user does not have write permission on the file's directory. This patch detects such situation and uses copy instead of move to prevent "permission denied" messages for every resource being added.

This addresses bug 296808.
<a href="http://bugs.kde.org/show_bug.cgi?id=296808" title="http://bugs.kde.org/show_bug.cgi?id=296808">http://bugs.kde.org/show_bug.cgi?id=296808</a>

Diffs
service/jobs/nepomuk/Move.h 8a8afd1
service/jobs/nepomuk/Move.cpp 08a3cc2

Diff: <a href="http://git.reviewboard.kde.org/r/104417/diff/" title="http://git.reviewboard.kde.org/r/104417/diff/">http://git.reviewboard.kde.org/r/104417/diff/</a>

Testing
Works on Meego devel image. The file is copied and no error message is shown.

Thanks,

Lamarque Vieira Souza

Comments

Re: Review Request: Copy files instead of moving if parent dir i

By Lamarque V. Souza at 03/27/2012 - 09:22

(Updated March 27, 2012, 2:22 p.m.)

Review request for KDE Runtime and Plasma.

Changes
Changing patch to do not move files/folders if their parent folders are not writable. At least in Linux/Unix if the parent folder is not writable in means the user does not have permission to move the file (he/she may have permission to copy it though).

Description
When adding an application resource to a private activity kactivitymanager tries to move the resource's .desktop file to the activity's private folder. The new .desktop file is created successfully but the source file is not deleted if the user does not have write permission on the file's directory. This patch detects such situation and uses copy instead of move to prevent "permission denied" messages for every resource being added.

This addresses bug 296808.
<a href="http://bugs.kde.org/show_bug.cgi?id=296808" title="http://bugs.kde.org/show_bug.cgi?id=296808">http://bugs.kde.org/show_bug.cgi?id=296808</a>

Diffs (updated)
service/jobs/nepomuk/Move.h 8a8afd1
service/jobs/nepomuk/Move.cpp 08a3cc2

Diff: <a href="http://git.reviewboard.kde.org/r/104417/diff/" title="http://git.reviewboard.kde.org/r/104417/diff/">http://git.reviewboard.kde.org/r/104417/diff/</a>

Testing
Works on Meego devel image. The file is copied and no error message is shown.

Thanks,

Lamarque Vieira Souza

Re: Review Request: Copy files instead of moving if parent dir i

By Lamarque V. Souza at 03/28/2012 - 20:58

(Updated March 29, 2012, 1:58 a.m.)

Review request for KDE Runtime and Plasma.

Changes
QFileInfo does not work with urls like "file:///path/to/file", we must use "/path/to/file".

Only urls with scheme "file" can be moved now.

Owning a folder and not having permission to write to it is a bit suspecious, so let's allow
kio show the "permission denied dialog" when trying to move a file/folder in such situation.

Description
When adding an application resource to a private activity kactivitymanager tries to move the resource's .desktop file to the activity's private folder. The new .desktop file is created successfully but the source file is not deleted if the user does not have write permission on the file's directory. This patch detects such situation and uses copy instead of move to prevent "permission denied" messages for every resource being added.

This addresses bug 296808.
<a href="http://bugs.kde.org/show_bug.cgi?id=296808" title="http://bugs.kde.org/show_bug.cgi?id=296808">http://bugs.kde.org/show_bug.cgi?id=296808</a>

Diffs (updated)
service/jobs/nepomuk/Move.h 8a8afd1
service/jobs/nepomuk/Move.cpp 08a3cc2

Diff: <a href="http://git.reviewboard.kde.org/r/104417/diff/" title="http://git.reviewboard.kde.org/r/104417/diff/">http://git.reviewboard.kde.org/r/104417/diff/</a>

Testing
Works on Meego devel image. The file is copied and no error message is shown.

Thanks,

Lamarque Vieira Souza

Re: Review Request: Copy files instead of moving if parent dir i

By Ivan Cukic at 03/28/2012 - 02:47

I don't mind this approach - some things need to be related to the private activity that can't be encapsulated in it.

The things that should be added here are (something I forgot) is first to check whether url is a local file or not. If not, it should act like it is unmovable. (web pages...)

- Ivan Čukić

On March 27, 2012, 2:22 p.m., Lamarque Vieira Souza wrote:

Re: Review Request: Copy files instead of moving if parent dir i

By =?utf-8?q?Thoma... at 03/27/2012 - 09:30

Does the new patch actually *silently* skip move impossible attempts??
Excuse my ignorance, but why are system resources actually needed to be *moved* anywhere by a random user - what means they're now gone in their original location (and for everyone else)
This does not sound as if the current move has a problem, but the design of those private activities has (single user approach -> fix that by logging him in as root and watch the project fail ;-)

- Thomas Lübking

On March 27, 2012, 2:22 p.m., Lamarque Vieira Souza wrote:

Re: Review Request: Copy files instead of moving if parent dir i

By Lamarque V. Souza at 03/27/2012 - 16:15

kactivitymanager tries to move all resources related to the private activity to the private folder. "Related" here is in nepomuk's definition of related. Although the app is related to the activity (it is on the homescreen of the private activity) it is not an user resource, so it should not be moved. This patch tries to handle that case where the resource is not an user resource then it should not be moved.

- Lamarque Vieira

On March 27, 2012, 2:22 p.m., Lamarque Vieira Souza wrote:

Re: Review Request: Copy files instead of moving if parent dir i

By =?utf-8?q?Thoma... at 03/27/2012 - 15:09

Is this part of kactivities limited to plasma active? (even if, "hard" is not "impossible")

The point is that *in general* if a file operation fails i'd like to be informed about that (because it means that either sth. is broken or that i'm stupid) - scratching that for a particular client usage is wrong.
(let's assume some filemanager *cough* would have used to crash on file deletes - you would not remove file deletion features from the filesystem to avoid crashes in that filemanager, would you ;-)

So either it should happen in the user code or there should be some weakForcedMove() or whatever function to explicitly work around permissions and copy in case move is not possible at all and can be used in such special usercode.

Just silently skipping filesystem job instructions to prevent error messages is the wrong solution (because you lie to the user, faking success)

- Thomas

On March 27, 2012, 2:22 p.m., Lamarque Vieira Souza wrote:

Re: Review Request: Copy files instead of moving if parent dir i

By Lamarque V. Souza at 03/27/2012 - 14:13

Well, Plasma Active makes heavy use of nepomuk to hide filesystem structure, which means there is no easy way for the user to change file permissions. Hidding filesystem structure is a design decision (made before I joined the team by the way). We still ship Dolphin with the images but since it is reduntant compared to active-filebrowser (PA's file manager) I guess it will be removed in the future.

I could change the patch to move the file if either the user owns the parent directory or he/she has write permissions to the parent folder.

- Lamarque Vieira

On March 27, 2012, 2:22 p.m., Lamarque Vieira Souza wrote:

Re: Review Request: Copy files instead of moving if parent dir i

By =?utf-8?q?Thoma... at 03/27/2012 - 11:08

Anyway, i guess the proper layer to decide which data to copy/move/symlink is not the general data management but the private activity creation, where you will likely also want to break/restore file permissions (eg. if a user dir has been tagged read-only, it's files should still be moved to the private activity and the former status restored there)

In general, just silently skipping impossible file operations is imo no option, because even if you just copied the data instead, it remains in a pot. public location what may be explicitly not wanted by explicitly attempting to move the file.
This could (for eg. chmod 500 directories) end up in exposing company secrets as well as just your kids suddenly stumbling across your FapFolder(tm)

- Thomas

On March 27, 2012, 2:22 p.m., Lamarque Vieira Souza wrote:

Re: Review Request: Copy files instead of moving if parent dir i

By Lamarque V. Souza at 03/27/2012 - 10:22

Yes, the new patch silently skips moving impossible attempts. I tested it here and we do not need to move the .desktop file to add the app to the homescreen. Skipping the move seems to fix the other problem described in #296808, now the containments are not empty after a reboot, I still need to figure out why this change fixes that problem (debugging nepomuk is not easy :-/).

Private activities are intended to protect data from different persons, not user accounts (like it usually is the case in Linux/Unix). Everybody logs in using the same non-root account and to access a private activity the person must authenticate yourself first. The itention is to treat a private activity as if it is different user account but the kde daemons (kactivitymanager, nepomuk, kded, contourd, etc) were not designed to authenticate users so we are resorting to encfs for that. With encfs the person using the device must supply a password to mount the encrypted folder and access the private data. One use case for that is a parent that creates a "Work" activity with data from his/her work, the parent also lends the device to his/her kids to play and do not want them to mess with his data, so the parent can mark the activity as private and the kids will not access to the data.

We are working on how to decide which data to move to the private folder and also when move them back to the original place. In this case we do not need to move .desktop files but moving files is still necessary if the file is a document created by the user.

- Lamarque Vieira

On March 27, 2012, 2:22 p.m., Lamarque Vieira Souza wrote:

Re: Review Request: Copy files instead of moving if parent dir i

By Sebastian =?utf... at 03/27/2012 - 06:27

Hm, moving a .desktop file seems wrong to me in any case, that shouldn't be necessary in the first place.

- Sebastian Kügler

On March 26, 2012, 6:04 p.m., Lamarque Vieira Souza wrote:

Re: Review Request: Copy files instead of moving if parent dir i

By Sebastian =?utf... at 03/27/2012 - 04:56

Excuse my naivity here, but how does this make sense? If a user asks to make certain data private, he expects them to be not available in unencrypted fashion anymore. Copying to the encrypted folder doesn't solve this, as the file is still available unencrypted, at its previous location.

The user would think his data is protected, while it isn't -> not good.

Or am I missing something here?

- Sebastian Kügler

On March 26, 2012, 6:04 p.m., Lamarque Vieira Souza wrote:

Re: Review Request: Copy files instead of moving if parent dir i

By Lamarque V. Souza at 03/27/2012 - 06:22

In my test I did not ask to make the data private, I just asked to add an app to the homescreen of a private activity. Apps are system resources (owned by root, so it's not user data), there is no sense in making them private or even moving them to the private folder since they will be restored on the next upgrade. Anyway, kamd only tries to move the .desktop, the executable still remains in /usr/bin. I do not know why moving/copying the .desktop is necessary for adding the app to the homescreen. Maybe there is a better solution, that's why I opened this review request instead of commiting the patch. The patch just ry to do not bother the user with an error message that does not make sense to him/her (in despite of the error message the app is added to homescreen).

- Lamarque Vieira

On March 26, 2012, 6:04 p.m., Lamarque Vieira Souza wrote: