-
Notifications
You must be signed in to change notification settings - Fork 817
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
Windows: Additional write permissions check #5510
Conversation
Signed-off-by: Alkl58 <[email protected]>
Also closes/fixes: #3201 |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #5510 +/- ##
==========================================
- Coverage 60.56% 58.94% -1.63%
==========================================
Files 145 143 -2
Lines 18846 18313 -533
==========================================
- Hits 11414 10794 -620
- Misses 7432 7519 +87 |
Will this be merged at some point? Or should I close it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good at first glance, but need to find time to test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of small comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Alkl58 are you able to make the requested changes? If not we are happy to take over the PR and include your changes with our own fixes :)
Co-authored-by: Claudio Cambra <[email protected]> Signed-off-by: Alkl58 <[email protected]>
Co-authored-by: Claudio Cambra <[email protected]> Signed-off-by: Alkl58 <[email protected]>
Co-authored-by: Claudio Cambra <[email protected]> Signed-off-by: Alkl58 <[email protected]>
Co-authored-by: Claudio Cambra <[email protected]> Signed-off-by: Alkl58 <[email protected]>
Co-authored-by: Claudio Cambra <[email protected]> Signed-off-by: Alkl58 <[email protected]>
Sorry for the delay. Should be all good now :) |
Signed-off-by: Alkl58 <[email protected]>
AppImage file: nextcloud-PR-5510-67205a2cec923c441ec1e962388e287219073afa-x86_64.AppImage |
Just a FYI: Apparently I completely broke my local dev environment after trying to install all qt dependencies. I'll reinstall my system / use a VM, will take some time so I can properly debug / fix things. |
sure, no worries, I've noticed you're using |
@allexzander sure, thats probably faster, sorry for the hassle |
Moved to #6209 |
The Problem: The isWritable() function checks only(?) for write permissions for the user, owner, group or anyone(?).
Its technically correct - when opening the folder permissions of an SMB share in Explorer, it shows that the user doesn't have those permissions. The actual permission for being able to read/write/delete is not covered by the "base" permissions of Explorer, it's covered by the "special permissions".
It looks similar to this (just imageine it being an SMB share):
![](https://camo.githubusercontent.com/605197d3682b8c65c20010593df92bbeb3efc01d62b4426e6ffd03fa55d2580b/68747470733a2f2f692e737461636b2e696d6775722e636f6d2f64753844352e706e67)
For more info check my comment: #3910 (comment)
This PR adds an additional actual write test for Windows hosts.
This PR fixes #3910
First time using C++, comments/improvements welcome!