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.

Sign up for GitHub

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

Jump to bottom

fix share roots always being marked as writable #38179

Merged
merged 2 commits into from
Jul 28, 2023
Merged

fix share roots always being marked as writable #38179

merged 2 commits into from
Jul 28, 2023

Conversation

icewind1991
Copy link
Member

Since movable mounts are always marked as "updatable" some extra logic is needed to split "updatable" into "renamable" and "writable"

Fixes nextcloud/viewer#1606

@icewind1991 icewind1991 added the 3. to review Waiting for reviews label May 10, 2023
@icewind1991 icewind1991 added this to the Nextcloud 27 milestone May 10, 2023
@icewind1991 icewind1991 requested review from marcelklehr, a team, ArtificialOwl, nfebe and come-nc and removed request for a team May 10, 2023 13:23
@icewind1991 icewind1991 mentioned this pull request May 10, 2023
Closed
skjnldsv
skjnldsv reviewed May 11, 2023
View reviewed changes
lib/public/Files/DavUtil.php Outdated
// since we always add update permissions for the root of movable mounts (and thus shares)
// we need to check the shared cache item directly to determine if it's writable
$storage = $info->getStorage();
if ($info->getInternalPath() === '' && $storage->instanceOfStorage(SharedStorage::class)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird psalm error 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OCP cannot use stuff from OC or OCA

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@icewind1991 Can this be put into isUpdatable instead?
Or in new methods isWritable/…?

Copy link
Member Author

@icewind1991 icewind1991 May 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't be put into isUpdatable because that is also used to check for rename permissions.

Adding a new method to the interface is wrought with compatibility pitfalls

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is wrought with compatibility pitfalls

what would those be?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It means that any (possibly 3rdparty) implementation of the interface needs to be updated as well

This was referenced May 12, 2023
Merged
Merged
@blizzz blizzz modified the milestones: Nextcloud 27, Nextcloud 28 May 23, 2023
@marcelklehr
Copy link
Member

Any update in this? Would be cool if we could solve #1606 soon

@icewind1991 icewind1991 force-pushed the dav-permissions-share-root-write branch from 18c9f40 to 910e79f Compare June 6, 2023 14:59
@icewind1991 icewind1991 marked this pull request as ready for review June 6, 2023 16:45
@AndyScherzinger AndyScherzinger requested a review from a team June 6, 2023 17:47
@icewind1991 icewind1991 force-pushed the dav-permissions-share-root-write branch from 910e79f to ad7ad62 Compare June 6, 2023 18:02
@come-nc
Copy link
Contributor

come-nc commented Jun 8, 2023

Any update in this? Would be cool if we could solve #1606 soon

Wrong ticket number I suppose?

@come-nc
Copy link
Contributor

come-nc commented Jun 8, 2023

Any update in this? Would be cool if we could solve #1606 soon

Wrong ticket number I suppose?

Oh okay wrong repository, it’s nextcloud/viewer#1606

come-nc
come-nc reviewed Jun 8, 2023
View reviewed changes
lib/public/Files/DavUtil.php Outdated
// since we always add update permissions for the root of movable mounts (and thus shares)
// we need to check the shared cache item directly to determine if it's writable
$storage = $info->getStorage();
if ($info->getInternalPath() === '' && $storage->instanceOfStorage(SharedStorage::class)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if ($info->getInternalPath() === '' && $storage->instanceOfStorage(SharedStorage::class)) {
/** @psalm-suppress UndefinedClass files_sharing may be disabled but this will still work as expected */
if ($info->getInternalPath() === '' && $storage->instanceOfStorage(SharedStorage::class)) {

Add a suppress like this then, I do not see a better way. And ::class does not actually complains on non-existing classes so it should be fine. Maybe still test this with files_sharing disabled to be sure.

A clean fix would be to add a new interface for this, like IWritableAwareStorage, and change the code for:

Suggested change
if ($info->getInternalPath() === '' && $storage->instanceOfStorage(SharedStorage::class)) {
if ($storage->instanceOfStorage(IWritableAwareStorage::class)) {
$isWritable = $storage->isWritable($info);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping @icewind1991

@icewind1991 icewind1991 force-pushed the dav-permissions-share-root-write branch from ad7ad62 to 533456d Compare June 8, 2023 12:29
@marcelklehr marcelklehr requested review from come-nc and removed request for marcelklehr July 3, 2023 12:41
@marcelklehr
Copy link
Member

Hello there!

What is the current status on this? Assume another review round is needed? Or is there still a disagreement that we need to resolve on how to solve this issue? Would be cool if we could cross this issue off soon 💙

Cheers

@icewind1991 icewind1991 force-pushed the dav-permissions-share-root-write branch from 533456d to d82f21e Compare July 12, 2023 14:13
@icewind1991
Copy link
Member Author

Switched to checking for MoveableMount instead and exposed that interface in OCP

@marcelklehr
Copy link
Member

Thanks for looking into this @icewind1991! Am I right in thinking that we can't backport this?

@marcelklehr
Copy link
Member

Some CI runs are failling.

@marcelklehr
Copy link
Member

marcelklehr commented Jul 27, 2023 •
edited
Loading

image

Autoloaders are not updated it seems and a test broke:

1) OCA\DAV\Tests\unit\Connector\Sabre\NodeTest::testDavPermissions with data set #5 (31, 'file', true, 29, true, '', 'SRMGDNV')
--


+'SRMGDNVW'

nfebe
nfebe approved these changes Jul 28, 2023
View reviewed changes
Copy link
Contributor

@nfebe nfebe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@marcelklehr
Copy link
Member

More approvals will not help, I'm afraid. the tests and CI failures need to be fixed cc @icewind1991

@GretaD
Copy link
Contributor

GretaD commented Jul 28, 2023

I need this to be merged as soon as possible for a customer

@icewind1991
expose MovableMount in public interface
fda820a 
Signed-off-by: Robin Appelman <robin@icewind.nl>
@icewind1991 icewind1991 force-pushed the dav-permissions-share-root-write branch from d82f21e to 0186a94 Compare July 28, 2023 12:49
@icewind1991 icewind1991 mentioned this pull request Jul 28, 2023
Merged
@icewind1991 icewind1991 enabled auto-merge July 28, 2023 13:18
@icewind1991 icewind1991 disabled auto-merge July 28, 2023 13:35
@icewind1991
fix share roots always being marked as writable
8af60b9 
Signed-off-by: Robin Appelman <robin@icewind.nl>
@icewind1991 icewind1991 force-pushed the dav-permissions-share-root-write branch from 0186a94 to 8af60b9 Compare July 28, 2023 15:35
@icewind1991 icewind1991 merged commit 6a467ec into master Jul 28, 2023
@icewind1991 icewind1991 deleted the dav-permissions-share-root-write branch July 28, 2023 18:24
@icewind1991
Copy link
Member Author

#39614 for the backport

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@come-nc come-nc come-nc approved these changes

@nfebe nfebe nfebe approved these changes

@ArtificialOwl ArtificialOwl ArtificialOwl approved these changes

@marcelklehr marcelklehr marcelklehr approved these changes

@skjnldsv skjnldsv Awaiting requested review from skjnldsv

Assignees
No one assigned
Labels
3. to review Waiting for reviews
Projects
None yet
Milestone
Nextcloud 28
Development

Successfully merging this pull request may close these issues.

Shared image file appears to have "SRGDNVW" permissions (although PROPFIND ground truth has "SRGD")
8 participants
@icewind1991 @marcelklehr @come-nc @GretaD @ArtificialOwl @nfebe @skjnldsv @blizzz