Bug 418499

Summary: API clarification SlaveBase::Copy permissions & umask for remote slaves
Product: [Frameworks and Libraries] frameworks-kio Reporter: Harald Sitter <sitter>
Component: generalAssignee: KIO Bugs <kio-bugs-null>
Status: REPORTED ---    
Severity: normal CC: kdelibs-bugs-null
Priority: NOR    
Version First Reported In: git master   
Target Milestone: ---   
Platform: Other   
OS: Linux   
Latest Commit: Version Fixed/Implemented In:
Sentry Crash Report:

Description Harald Sitter 2020-03-05 14:46:02 UTC 
See https://bugs.kde.org/show_bug.cgi?id=345687

TLDR: we ignore server-side umasks because we get explicit permission bits in Slave::copy and explicitly set them via chmod (thus ignoring the umask). The documentation doesn't explicitly say the permissions are explicit but it is certainly implied and it is how (e.g.) the FTP slave works too.

https://api.kde.org/frameworks/kio/html/classKIO_1_1CopyJob.html#a3bd4dda22a3bd50adf195542d9d009ad
https://api.kde.org/frameworks/kio/html/classKIO_1_1SlaveBase.html#a1f382438285d8f1112e9340f3c0e8bbf

I would make an argument that KIO::CopyJob has this somewhat backwards. I would rather expect copies across protocol and/or host boundary to only have implicit default permissions (e.g. 644) but be subject to umasking (i.e. stay away from chmod).

Simply put:
- copy within same protocol and host: use input permissions
- copy across protocol or host: use default permissions potentially subject to umask (e.g. avoid chmodding) and ignore the input permissions

To accomplish that we'd kind of only need to send -1 when src.scheme!=dst.scheme || src.host!=dst.host.

Mind you I'm equally content with keeping everything as-is even though on paper I find the behavior underwhelming.
Comment 1 David Faure 2020-06-25 22:13:57 UTC 
Sounds good, but executables should remain executables, no?
Comment 2 Harald Sitter 2020-06-26 09:51:50 UTC 
If the umask strips +x we really shouldn't put it back in, beyond that I'm fairly unopinionated on the matter.
That being said, if we want to have +x preserved we kind of need a better separation of desired permission outcomes:

- implicit permissions (the new default for boundary crossing copies): copyjob passes in the existing permissions and the slave must "create" with these AND must honor umask
- explicit permissions (the default for non-boundary crossing copies): copyjob passes in permission and the slave must call chmod to ensure those permissions are applied regardless of umask
- default permissions: same as implicit but the permissions are some default value

implicit permission would then preserve the entire mode in so far as the umask doesn't strip bits.

API changes I am thinking of:

```
Mode {
 Auto, /* boundary-crossing: Implicit; !boundary-crossing: Explicit */
 Implicit, /* honor umask */
 Explicit, /* override umask */
 Default /* 644 */ }
CopyJob::setPermissionMode(Mode)
deprecated CopyJob::setDefaultPermissions(bool b) { setPermissionMode(b ? Default : Auto) }
```

On the protocol side I suppose we can simply add another value that carries the mode and deprecate SlaveBase::copy(QUrl,int,JobFlags) in favor of copy(QUrl,int,Mode,JobFlags) with the dispatch function taking care of compatibility.