29873 read only mount by TJHeeringa · Pull Request #33035 · owncloud/core

TJHeeringa · 2018-10-05T20:05:36Z

Description

TL;DR: Made a read only option for external storages

I added a extra option in the advanced settings menu when mounting an external storage. This causes an extra row to be added in the database, when saving the mount, with whether an the mount is read only or not. A storagewrapper is added that checks the option and if the mount has the read only option the storagewrapper adds an permissionmask that makes the mount read only.

Related Issue

Fixes [Feature Request] Mark mount as read only in the mount settings #29873

Motivation and Context

Sometimes you want to have a group only to be able to read a certain map. Currently no option exists to do this. Extra info can be found in:
owncloud-archive/user_management#23
#29873

How Has This Been Tested?

Currently tested it by making a external share called 'Local' and setting the option read_only that I made. Haven't figured out how to write proper 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)
Technical debt
Tests

Checklist:

Code changes
Unit tests added
Acceptance tests added
Documentation ticket raised:

Open tasks:

Write tests
Figure out why !$storage->instanceOfStorage('\OC\Files\Storage\Home')) is required in the line if ($mount->getOption('read_only', true) && !$storage->instanceOfStorage('\OC\Files\Storage\Home')) on lib/private/legacy/util.php line 178

CLAassistant · 2018-10-05T20:05:43Z

All committers have signed the CLA.

DeepDiver1975 · 2018-10-05T21:09:28Z

@mmattel Hi Martin - we have been chatting about this feature at the conference - right? ;-)

Can you test this? Thanks a lot!

mmattel · 2018-10-05T21:22:08Z

That´s pre-Christmas 😄 will do !

mmattel · 2018-10-05T21:51:40Z

@DeepDiver1975
hmmm, just pulled but I can´t find 29873-Read_only_mount in origin. did I miss something?

DeepDiver1975 · 2018-10-05T22:04:06Z

@DeepDiver1975
hmmm, just pulled but I can´t find 29873-Read_only_mount in origin. did I miss something?

Click on the link 'vie command line options'

Step 1 is what you need:

mmattel · 2018-10-05T22:32:23Z

The first thing I found is the following:

when you create a new mount point, the read-only checkmark is not set
ok
when you look at existing mount points, the check mark ist set to on
you need to manually disable them wrong
this must be fixed so that existing mouts do net get set read only automatically
depending if the read-only checkmark is set or not, you get the correct message in the files screen You don´t have permissions... or not. ok

@TJHeeringa

can you check signing the CLA
can you go for a fix for 2, I will retest it afterwards

TJHeeringa · 2018-10-06T09:10:11Z

Sure, I can.

Usually a default for existing options would be added by migrations. Since this code does not have migrations, I am unsure how to fix this. I have been looking around and could do either of the following:

change the storagewrapper to look for the read only option. If it has not been set, then it should be considered false
force the read only button to be unchecked somehow
--- OR ---
make or call a function that reloads the options for all the external mounts

The second option is easier to make, but gives unwanted behavior, since it has to be triggered somehow. The first option on the other hand seems a bit hackish and isn't something that one should do, since then you have the same problem again, when adding another options in the future.

Do you guys now a better solution?

PVince81 · 2018-10-08T13:30:42Z

 	'		<label for="mountOptionsEncrypt">{{t "files_external" "Enable encryption"}}</label>' +
 	'	</div>' +
 	'	<div class="optionRow">' +
+	'		<input id="mountOptionsPreviews" name="read_only" type="checkbox" value="false" checked="checked"/>' +


here you need value="true" because checking the checkbox will make the value "true"

with a bit of luck it might solve the problem that it's checked by default

Sadly this doesn't work with me locally. It still automatically checks it.

PVince81 · 2018-10-08T13:31:02Z

 		});

+		\OC\Files\Filesystem::addStorageWrapper('read_only', function ($mountPoint, IStorage $storage, \OCP\Files\Mount\IMountPoint $mount) {
+			if ($mount->getOption('read_only', true) && !$storage->instanceOfStorage('\OC\Files\Storage\Home')) {


should this be "false" to make it default to false ?

When I wrote it, I thought it meant: get the options read_only and return true if it is true and false otherwise, but return the value and set true otherwise, makes way more sense.

codecov · 2018-10-09T07:50:46Z

Codecov Report

Merging #33035 into master will decrease coverage by 0.01%.
The diff coverage is 21.73%.

@@             Coverage Diff              @@
##             master   #33035      +/-   ##
============================================
- Coverage     64.76%   64.75%   -0.02%     
- Complexity    18345    18348       +3     
============================================
  Files          1198     1199       +1     
  Lines         69435    69458      +23     
  Branches       1278     1278              
============================================
+ Hits          44971    44976       +5     
- Misses        24093    24111      +18     
  Partials        371      371

Flag	Coverage Δ	Complexity Δ
#javascript	`53.04% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`66.1% <21.73%> (-0.02%)`	`18348 <2> (+3)`

Impacted Files	Coverage Δ	Complexity Δ
apps/files_external/appinfo/app.php	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
apps/files_external/js/settings.js	`51.84% <ø> (ø)`	`0 <0> (ø)`	⬇️
...ernal/appinfo/Migrations/Version20190109104110.php	`0% <0%> (ø)`	`0 <0> (?)`
apps/files_external/lib/Command/ListCommand.php	`71.13% <0%> (-0.37%)`	`0 <0> (ø)`
apps/files_external/templates/settings.php	`33.69% <100%> (+0.72%)`	`0 <0> (ø)`	⬇️
lib/private/legacy/util.php	`68.18% <66.66%> (-0.02%)`	`243 <2> (+3)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1c1bf85...0b88c62. Read the comment docs.

TJHeeringa · 2018-10-09T08:04:19Z

Weird issues still persisting:

Changing 'mountOptionsPreviews' to 'mountOptionsReadOnly' on lines 22 and 23 in js/settings.js gives the error "PHP Fatal error: Uncaught Error: Call to a member function getLogger() on null in /var/www/owncloud/core/index.php:86\nStack trace:\n#0 {main}\n thrown in /var/www/owncloud/core/index.php on line 86"
For existing mounts, when clicking on the advanced menu in settings for the first time after the read_only options is added, the checkbox is set, eventhough 'checked="checked"' is not set on line 22 in js/settings.js

Solutions:

The fact that 'id="mountOptionsPreviews"' now appears twice in js/settings.js (22/23 and 26/27) doesn't seem to cause any issue at all. It is not neat programming, but could be ignored.
Some logic seems to mishave regarding the checkboxes. The easy fix is to add a migration(contrary to what I said before, there are migrations) in appinfo/migrations, which sets the option read_only on all the existing mounts.

mmattel · 2018-11-04T11:40:07Z

Hi @TJHeeringa , any update on this PR ?

TJHeeringa · 2018-11-04T11:47:06Z

I've had no spare time to work on this PR. In about a week or two I will have some time to continue working on the PR.

TJHeeringa · 2019-01-09T12:42:28Z

The failing tests have to do with php-lint. I don't know how to fix this.

phil-davis · 2019-01-09T13:55:18Z

@TJHeeringa please rebase the PR.
The drone jobs matrix was modified today, and drone for "old" PRs will now have the wrong idea about what jobs to run.

TJHeeringa · 2019-01-09T15:07:14Z

Codecov.io keeps giving secure connection failed errors. Do I need to fix more things?

phil-davis · 2019-01-09T15:13:15Z

@TJHeeringa codecov can be a pain. It might just be having a bad day.

@PVince81 drone is passing. Please review again.

mmattel · 2019-01-21T15:36:04Z

@PVince81 any update ?

PVince81 · 2019-01-21T15:38:28Z

@jvillafanez please test and review

jvillafanez · 2019-01-22T08:42:54Z

Is the migration needed?

For new installations, the "set read-only" checkbox should be unchecked / false
For upgrades from a recent version (no legacy storages), the checkbox should also be set as unchecked / false to keep the previous behaviour
For upgrades from older versions (with legacy storages), I expect another migration to take care of the conversion (which should be already in place, so no need to touch anything here), and then consider the checkbox as false.

I expect that, if the option is missing from the storage information, it's considered as false. The checkbox is expected to be shown unchecked in any case, and modifying its state should store the proper information in the DB as expected.

Unless I'm missing something, the migration should be removed. The rest of the code looks good. I still need to test it though.

TJHeeringa · 2019-01-22T08:56:32Z

Is the migration needed?
* For new installations, the "set read-only" checkbox should be unchecked / false

* For upgrades from a recent version (no legacy storages), the checkbox should also be set as unchecked / false to keep the previous behaviour

* For upgrades from older versions (with legacy storages), I expect another migration to take care of the conversion (which should be already in place, so no need to touch anything here), and then consider the checkbox as false.
I expect that, if the option is missing from the storage information, it's considered as false. The checkbox is expected to be shown unchecked in any case, and modifying its state should store the proper information in the DB as expected.

Unless I'm missing something, the migration should be removed. The rest of the code looks good. I still need to test it though.

I also thought that the migration wasn't needed, but for some weird reason the code, regardless of what I set as default value, always made the existing mounts behave like read_only = true. This is unwanted behavior and the migration seems to fix this issue.

A test from your end without the migration would be helpful. If it works without, the migration can be removed.

jvillafanez · 2019-01-22T17:42:27Z

I also thought that the migration wasn't needed, but for some weird reason the code, regardless of what I set as default value, always made the existing mounts behave like read_only = true. This is unwanted behavior and the migration seems to fix this issue.

Works for me without the migration. Maybe another person can confirm.

Install fresh master code
Apply the patch of this branch (removed the migration file)
Set up a mount point (SFTP). Do not touch the default settings, so read-only is unchecked

With those steps, I can upload to the external storage

Install OC 10.0.10
Set up a a mount point (SFTP)
Upgrade to master code
Apply the patch of this branch (removed the migration file)

I also can upload files to the SFTP storage with those steps.

After that, the "set read-only" works fine, so I don't see any issue.

@TJHeeringa if you had issues without the migration, could you provide some steps as detailed as possible, so we can try to reproduce the issue?

TJHeeringa · 2019-01-29T14:42:13Z

What I did was

Write the code -- equivalent for someone else to just pulling the code
Make a new mount by inserting directly into the database -- equivalent to having a entry in your database prior to pulling
Access the mount point
Conclude that it is read_only
Apply migration
Access mount point again
Conclude that it is not read_only anymore

I inserted it directly into the database, since I thought of that before thinking of checking out to a different branch that does not have the read_only code and making a mount point there.

TJHeeringa · 2019-01-29T14:47:24Z

I just tried doing the checkout 'trick', but my OwnCloud won't sign in anymore after displaying a message about upgrading to 11.0 pre-alpha. Will probably take a while to figuring out what is broken. Probably some permission with the session cookie location. Error is probably occuring due to that I'm running WSL, but that's off topic for this PR.

@mmattel Could you perhaps try and see if it works without the migration?

mmattel · 2019-01-29T15:12:44Z

@TJHeeringa to "fix" you login problem (I ran into the same ...), just run make clean and then make in the root of core. That should fix it. For details run make help which gives you quite useful infos.
If not done already, run make test-php-style-fix to make the tests happy 😄

I am currently very short in time, so testing is an issue for me...

jvillafanez · 2019-01-29T17:52:23Z

I suggest you to use either docker or a VM in order to revert to a previous state easily.

Write the code -- equivalent for someone else to just pulling the code

Make a new mount by inserting directly into the database -- equivalent to having a entry in your database prior to pulling

Access the mount point

Conclude that it is read_only

Apply migration

Access mount point again

Conclude that it is not read_only anymore

I don't think the steps are good.

Assuming you wrote the same code (which let me distrust this because people always make stupid mistakes), then adding a mount point directly in the DB is bad: there is no need for this when you can easily do the same in the UI; you can also add wrong or incompleted information in the DB.

You also apply the migration in step 5. Under normal circumstances, migrations should be executed just after apply the code patch, without any other interaction. As far as I know, apps increases the version which should cause core to automatically check and apply new migrations; I'm not sure how this process is done for core migrations, but in any case I'd expect the migration to be run manually just after the patch.

Note that what we want (if possible) is to remove the migration file, that "apply migration" step shouldn't be there in order to test what we want.

As said, since downgrading isn't supported, I'd recommend you to use either docker or a VM to restore a previous state. It might also happen that you had some additional information somewhere that you forgot (or maybe you didn't know it was there) to cleanup before trying to restore the initial information.

mmattel · 2019-06-19T08:08:54Z

Hi, any update on this? Would be very useful !

DeepDiver1975 · 2019-07-30T06:20:02Z

As discussed in #35777 the master branch will from now on hold the ownCloud 10 codebase.

This PR targetted ownCloud 11 which is postponed to a far distant future.

Because of that I'm closing this PR and kindly ask you to re-submit this PR in a few days.

Thanks a lot for your patience

TJHeeringa force-pushed the 29873-Read_only_mount branch from 4c3c642 to 84973d9 Compare October 5, 2018 20:11

DeepDiver1975 added the 3 - To Review label Oct 5, 2018

DeepDiver1975 added this to the development milestone Oct 5, 2018

mmattel requested a review from DeepDiver1975 October 5, 2018 21:22

patrickjahns requested review from hodyroff and pmaier1 October 6, 2018 13:58

PVince81 reviewed Oct 8, 2018

View reviewed changes

PVince81 reviewed Oct 12, 2018

View reviewed changes

Comment thread apps/files_external/js/settings.js

mmattel reviewed Jan 9, 2019

View reviewed changes

Comment thread apps/files_external/appinfo/Migrations/Version20190109104110.php Outdated

TJHeeringa and others added 4 commits January 9, 2019 15:03

Added read-only option for mounts

ece8d71

Fixed default behavior mount w/o read_only option

516c20d

Made read_only migration

d7a6785

Fix for tests

0b88c62

TJHeeringa force-pushed the 29873-Read_only_mount branch from 226498c to 0b88c62 Compare January 9, 2019 14:06

mmattel mentioned this pull request Jan 10, 2019

Browser upload to a mount where the account is read only fails late #29999

Open

PVince81 requested a review from jvillafanez January 21, 2019 15:38

DeepDiver1975 closed this Jul 30, 2019

mmattel mentioned this pull request Nov 10, 2019

New mount setting: read only mount #36397

Merged

11 tasks

Conversation

TJHeeringa commented Oct 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Open tasks:

Uh oh!

CLAassistant commented Oct 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DeepDiver1975 commented Oct 5, 2018

Uh oh!

mmattel commented Oct 5, 2018

Uh oh!

mmattel commented Oct 5, 2018

Uh oh!

DeepDiver1975 commented Oct 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mmattel commented Oct 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TJHeeringa commented Oct 6, 2018

Uh oh!

PVince81 Oct 8, 2018

Choose a reason for hiding this comment

Uh oh!

TJHeeringa Oct 9, 2018

Choose a reason for hiding this comment

Uh oh!

PVince81 Oct 8, 2018

Choose a reason for hiding this comment

Uh oh!

TJHeeringa Oct 9, 2018

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Oct 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

TJHeeringa commented Oct 9, 2018

Uh oh!

Uh oh!

mmattel commented Nov 4, 2018

Uh oh!

TJHeeringa commented Nov 4, 2018

Uh oh!

Uh oh!

TJHeeringa commented Jan 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phil-davis commented Jan 9, 2019

Uh oh!

TJHeeringa commented Jan 9, 2019

Uh oh!

phil-davis commented Jan 9, 2019

Uh oh!

mmattel commented Jan 21, 2019

Uh oh!

PVince81 commented Jan 21, 2019

Uh oh!

jvillafanez commented Jan 22, 2019

Uh oh!

TJHeeringa commented Jan 22, 2019

Uh oh!

jvillafanez commented Jan 22, 2019

Uh oh!

TJHeeringa commented Jan 29, 2019

Uh oh!

TJHeeringa commented Jan 29, 2019

Uh oh!

mmattel commented Jan 29, 2019

Uh oh!

jvillafanez commented Jan 29, 2019

Uh oh!

TJHeeringa commented Oct 5, 2018 •

edited

Loading

CLAassistant commented Oct 5, 2018 •

edited

Loading

DeepDiver1975 commented Oct 5, 2018 •

edited

Loading

mmattel commented Oct 5, 2018 •

edited

Loading

codecov Bot commented Oct 9, 2018 •

edited

Loading

TJHeeringa commented Jan 9, 2019 •

edited

Loading