Bug 404777 - KIO::CopyJob::setDefaultPermissions does not work
Summary: KIO::CopyJob::setDefaultPermissions does not work
Status: RESOLVED FIXED
Alias: None
Product: frameworks-kio
Classification: Frameworks and Libraries
Component: general (show other bugs)
Version: 5.53.0
Platform: Arch Linux Linux
: NOR minor
Target Milestone: ---
Assignee: KIO Bugs
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-02-24 15:49 UTC by Alex Bikadorov
Modified: 2021-11-24 21:39 UTC (History)
6 users (show)

See Also:
Latest Commit:
Version Fixed In: 5.87
Sentry Crash Report:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Bikadorov 2019-02-24 15:49:52 UTC
The following code creates a file at URL `destPath` with file permissions 600 (-rw-------).

>    QTemporaryFile *tempFile = new QTemporaryFile;
>    tempFile->setAutoRemove(false); // done below
>    tempFile->open(); // create file
>    
>    KIO::CopyJob *job = KIO::copy(QUrl::fromLocalFile(tempFile->fileName()), destPath);
>    job->setUiDelegate(nullptr);
>    job->setDefaultPermissions(true);

This is because the temporary file is created with permissions 600 and setDefaultPermission() seems to be ignored. According the documentation for it, umask should be applied and the final file should have permissions 644 (-rw-r--r--) (when umask is set to 022).

Related to bug 395609.

Qt Version: 5.12
Comment 1 Nate Graham 2019-02-26 19:04:02 UTC
Would you like to submit patch? :)
Comment 2 Alex Bikadorov 2019-03-03 19:14:20 UTC
@Nikita let's continue part of the discussion here.

Nikita's comment:
> I checked KIO::CopyJob::setDefaultPermissions and follow up code and this piece looks suspicious:
> 
> filecopyjob.cpp:
>   501     if (d->m_mustChmod) {
>   502         // If d->m_permissions == -1, keep the default permissions
>   503         if (d->m_permissions != -1) {
>   504             d->m_chmodJob = chmod(d->m_dest, d->m_permissions);
>   505             addSubjob(d->m_chmodJob);
>   506         }
>   507         d->m_mustChmod = false;
>   508     }
> 
> Haven't debugged though since KIO likely requires special debugging environment but according to the code (d->m_permissions == -1) is exactly happens after we call setDefaultPermissions and then chmod is never called.
> 
...
> 
> Probably, we should look more into the Dolphin code. They get around it somehow.

Dolphin uses KIO for creating new files (-> filewidgets/knewfilemenu.cpp) and there is the same problem here. Umask is ignored, the default file permission for new text files is alwas -rw-r--r-- and nobody noticed yet.

@Nate
Happy to try it - as soon as I've figured out how to build and install KIO and dependencies into a custom directory without affecting my system. I'm not that good with the cmake/make.
Comment 3 Nikita Melnichenko 2019-03-04 07:38:11 UTC
@KIO devs / @Nate, do you have a wiki / docs describing how to setup a debug environment for KIO as Alex asked? Please point there. We are happy to help since this issue is critical and introduces a security threat. Thanks!
Comment 4 Nate Graham 2019-03-04 14:46:08 UTC
Yes indeed! Thanks very much for volunteering to work on this.

Our tool kdesrc-build makes it easy. Here are the instructions: https://community.kde.org/Get_Involved/development#One-time_setup:_your_development_environment

Alternatively you can set up a VM and install KDE Neon Dev Unstable in it.

Feel free to contact me over email or in the #kde-devel Matrix/Freenode IRC channel if you need any assistance.
Comment 5 Ahmad Samir 2021-04-20 13:35:28 UTC
I think the issue here is the interpretation of the docs[1]:
“By default the permissions of the copied files will be those of the source files.

But when copying "template" files to "new" files, people prefer the umask to apply, rather than the template's permissions. For that case, call setDefaultPermissions(true)”

The code ends up going through FileProtocol::copy() which has:
    // set final permissions
    // if no special mode given, preserve the mode from the sourcefile
    qDebug() << "_mode" << _mode;
    if (_mode == -1) {
        _mode = buff_src.st_mode;
    }


What that means, is that if permissions are -1, which is what setDefaultPermissions(true) does, no special permissions will be set, and the permissions of the source are used.

I'll see if I can make the documentation more clear.

[1] https://api.kde.org/frameworks/kio/html/classKIO_1_1CopyJob.html#a3bd4dda22a3bd50adf195542d9d009ad
Comment 6 Ahmad Samir 2021-09-19 10:09:35 UTC
Git commit de7aff60e81b05239fd3d6c17895c48a985dd27a by Ahmad Samir.
Committed on 19/09/2021 at 10:06.
Pushed by ahmadsamir into branch 'master'.

Fix permissions when copying files

This reverts commit 7863f595991c5, I was wrong :)

Reading the code some more (in copyjob, file_copy and file_unix), the
permissions arg is '-1' when we shouldn't touch the dest permissions at all,
i.e. leave them as the system default permissions for newly created files
(umask ...etc).

The scenarios are:
- calling KIO::copy() A to B, permissions are preserved by default, i.e. the
  job copies A's permissions to the permissions arg when it calls file_copy
- calling KIO::copy() A to B, and calling setDefaultPermissions(true),
  the job sets the permissions arg to '-1' when it calls file_copy
- calling KIO::file_copy() directly, the you have full control, if you want
  to preserve the permissions use A's permissions as the permissions arg,
  otherwise use '-1' to use the system default permissions

ACL attributes are handled implicitly at the moment, which isn't ideal but
can be improved later on.
Related: bug 103331

FIXED-IN: 5.87

M  +4    -6    src/core/filecopyjob.h
M  +15   -22   src/ioslaves/file/file_unix.cpp

https://invent.kde.org/frameworks/kio/commit/de7aff60e81b05239fd3d6c17895c48a985dd27a
Comment 7 Nate Graham 2021-10-08 15:40:31 UTC
*** Bug 443477 has been marked as a duplicate of this bug. ***
Comment 8 Nate Graham 2021-10-08 17:37:35 UTC
*** Bug 443477 has been marked as a duplicate of this bug. ***