Add option to disable sharing in user-mounted external storages #28646

PVince81 · 2017-08-10T15:29:57Z

Description

Adds a new checkbox to disable sharing in user-mount external storages.

Since ConfigAdapterTest was missing I had to write all of it from scratch.
It turned out that some interfaces had missing methods for mocking (IStorageFactory::wrap), so I added it.
And also added Temporary::getAvailability() for the temporary test storage to avoid a DB round trip which would fail in this test.

Related Issue

Fixes #28636

Motivation and Context

See ticket

How Has This Been Tested?

TEST: manual testing in web UI
- TEST: when allowed, user mount has "Enable sharing" option visible. Share action visible in files UI.
- TEST: when disallowed, user mount has no "Enable sharing" option visible. Share action not visible in files UI
- TEST: when disallowed, system mounts can still have sharing enabled
TEST: unit tests

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

DeepDiver1975 · 2017-08-11T08:30:04Z

tests/lib/Files/External/ConfigAdapterTest.php

+
+	protected function setUp() {
+		$this->config = $this->createMock(IConfig::class);
+		// FIXME: cannot mock the interface because the UserTrait functions are not on them


but I'd say it's okay to mock the implementation

jvillafanez · 2017-08-11T10:05:10Z

apps/files_external/templates/settings.php

 			<?php endforeach; ?>
+			<br/>
+			<input type="checkbox" name="allowUserMountSharing" id="allowUserMountSharing" class="checkbox"
+				value="1" <?php if ($_['allowUserMountSharing'] == 'yes') print_unescaped(' checked="checked"'); ?> />


I think it's better to use '===' unless it's explicitly told otherwise (with the corresponding explanation)

yeah... this was just copy-pasted from the other block

jvillafanez · 2017-08-11T10:21:49Z

lib/public/Files/Storage/IStorageFactory.php

+	 *
+	 * @since 10.0.3
+	 */
+	public function wrap(IMountPoint $mountPoint, $storage);


Is this being used? It doesn't appear in the changes of this PR.

Is it the best place for the method? I see 2 different responsabilities for the storage factory: create the objects via getInstance and wrap the storages. From my point of view, the factory should focus on creating objects, if that object is wrapped somehow, that should be an implementation detail.

Unfortunately it is being used in the StorageFactory implementation, and it seems the calling code is assuming that the method always exists: https://github.com/owncloud/core/blob/master/lib/private/Files/Mount/MountPoint.php#L97

I only added it here to be able to mock it...

I don't know what to do with this. Bad solutions everywhere. I guess this is the "less bad" option we have.

Moving the method to the interface -> bad

Typecasting the interface in the MountPoint class -> bad

Checking if the implementation has the method -> bad

Removing the interface and use the class directly -> unexpected consecuences

Create a new interface and make the implementation implement also the new one -> too much work + adding a non-planned interface

The only reason I added it is because @DeepDiver1975 said to use this opportunity to add it. I discovered it while testing and do not wish to hold this PR much longer due to this.

My solution would be to resort to mocking StorageFactory instead and raise a tech debt ticket about "review all these weird FS classes where the method wasn't on the interface"

PVince81 · 2017-08-11T14:07:58Z

I decided to remote IStorageFactory::wrap for now and mocking the StorageFactory class.
Other comments addressed as well: removed "FIXME" and used "===".

Please review

PVince81 · 2017-08-11T14:10:28Z

Raised StorageFactory tech debt issue here #28656 as it seems to be more complex to fix than just adding the method on the interface

jvillafanez · 2017-08-14T06:51:49Z

lib/private/Files/Storage/Temporary.php

 	}
+
+	public function getAvailability() {
+		return ['available' => true];


I'm not sure if the 'last_checked' key is optional. The PHPDoc doesn't say anything, so it should be required.
If it isn't being actively used, we could adjust the PHPDoc to say the 'last_checked' key is optional.

This class is supposed to be used only by unit tests. Maybe we should move it to the "tests" folder.

PVince81 · 2017-08-15T14:20:31Z

@jvillafanez I've added "last_checked => 0" and squashed. Please rereview.

PVince81 · 2017-08-16T11:07:33Z

stable10: #28706

lock · 2019-08-02T19:02:23Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

PVince81 added 3 - To Review backport-request labels Aug 10, 2017

PVince81 added this to the development milestone Aug 10, 2017

PVince81 self-assigned this Aug 10, 2017

PVince81 requested review from DeepDiver1975 and jvillafanez August 10, 2017 15:29

PVince81 mentioned this pull request Aug 10, 2017

Ext storage checkbox to disallow sharing on user-created mounts #28636

Closed

DeepDiver1975 reviewed Aug 11, 2017

View reviewed changes

jvillafanez reviewed Aug 11, 2017

View reviewed changes

PVince81 mentioned this pull request Aug 11, 2017

Review and adjust FS layer classes that have mandatory methods missing on interfaces #28656

Closed

1 task

jvillafanez reviewed Aug 14, 2017

View reviewed changes

PVince81 added the p1-urgent Critical issue, need to consider hotfix with just that issue label Aug 15, 2017

Add option to disable sharing in user-mounted external storages

fc910be

PVince81 force-pushed the extstorage-noshare-checkbox branch from 702dd03 to fc910be Compare August 15, 2017 14:20

jvillafanez approved these changes Aug 16, 2017

View reviewed changes

PVince81 merged commit c15c0c3 into master Aug 16, 2017

PVince81 deleted the extstorage-noshare-checkbox branch August 16, 2017 11:06

PVince81 mentioned this pull request Aug 16, 2017

[stable10] Add option to disable sharing in user-mounted external storages #28706

Merged

PVince81 removed the backport-request label Aug 16, 2017

ownclouders added the status/STALE label Dec 20, 2017

lock bot locked as resolved and limited conversation to collaborators Aug 2, 2019

Add option to disable sharing in user-mounted external storages #28646

Add option to disable sharing in user-mounted external storages #28646

Uh oh!

Conversation

PVince81 commented Aug 10, 2017

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PVince81 commented Aug 11, 2017

Uh oh!

PVince81 commented Aug 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PVince81 commented Aug 15, 2017

Uh oh!

PVince81 commented Aug 16, 2017

Uh oh!

lock bot commented Aug 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants