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

Make it possible to disable local storage mount type #26653

Closed
PVince81 opened this Issue Nov 18, 2016 · 11 comments

Comments

Projects
None yet
5 participants
@PVince81
Member

PVince81 commented Nov 18, 2016

In some scenarios, the ownCloud admin is not the same person as the sysadmin.
In such scenarios, the ownCloud admin only has access to the web interface and cannot run CLI commands and is not supposed to do anything outside of the OC folder.
Allowing such admin to mount arbitrary folders with the "Local" external storage could be considered a security issue in such specific scenarios.

So the idea would be to introduce a config.php option (which is edited by the sysadmin) to prevent admins to mount external storages of the type "Local".

@hodyroff @jvillafanez @DeepDiver1975

@hodyroff

This comment has been minimized.

Show comment
Hide comment
@hodyroff

hodyroff Nov 18, 2016

Contributor

Yes.
@pmaier1

Contributor

hodyroff commented Nov 18, 2016

Yes.
@pmaier1

@pmaier1

This comment has been minimized.

Show comment
Hide comment
@pmaier1

pmaier1 Nov 18, 2016

Contributor

Sounds good, effort probably low (?!), adding to 9.2 milestone

Contributor

pmaier1 commented Nov 18, 2016

Sounds good, effort probably low (?!), adding to 9.2 milestone

@pmaier1 pmaier1 added this to the 9.2 milestone Nov 18, 2016

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Nov 18, 2016

Member

Yeah, I think doable in 1-2 md.

Member

PVince81 commented Nov 18, 2016

Yeah, I think doable in 1-2 md.

@PVince81 PVince81 self-assigned this Nov 18, 2016

@PVince81 PVince81 added the junior job label Nov 18, 2016

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Nov 18, 2016

Member

I'd say the technical approach is as follows: if the config.php switch is false (defaults to true), then remove the registration of the "Local" storage backend completely. This will make it disappear from the web UI and any storages mounted already with this backend will disappear too.

Member

PVince81 commented Nov 18, 2016

I'd say the technical approach is as follows: if the config.php switch is false (defaults to true), then remove the registration of the "Local" storage backend completely. This will make it disappear from the web UI and any storages mounted already with this backend will disappear too.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jan 27, 2017

Member

Thanks a lot to @grischdian for implementing this in #26990

Member

PVince81 commented Jan 27, 2017

Thanks a lot to @grischdian for implementing this in #26990

@PVince81 PVince81 closed this Jan 27, 2017

@pmaier1

This comment has been minimized.

Show comment
Hide comment
@pmaier1

pmaier1 Mar 13, 2017

Contributor

@PVince81 Please change the default of 'files_external_deny_local' to 'true' for 10.0 (mounting local storage should have to be enabled explicitly)

Contributor

pmaier1 commented Mar 13, 2017

@PVince81 Please change the default of 'files_external_deny_local' to 'true' for 10.0 (mounting local storage should have to be enabled explicitly)

@pmaier1 pmaier1 reopened this Mar 13, 2017

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 13, 2017

Member

@pmaier1 it is possible to change the default, yes.

This will be a bit more work because we need to make sure that if "Local" storages were mounted in past versions, then the default will still set itself to no. Without this, an upgrade to OC 10 with legitimate local storages would see them suddenly unmounted which could cause sync clients to start deleting local files when they see the folder is gone. (not good)

So: write a migration that checks all existing storage configs. If any of the storage configs contain at least one with the "Local" external storage, then set it explicitly to "yes".
If nothing was found, don't set anything and let it default to "no".

Makes sense ?

Member

PVince81 commented Mar 13, 2017

@pmaier1 it is possible to change the default, yes.

This will be a bit more work because we need to make sure that if "Local" storages were mounted in past versions, then the default will still set itself to no. Without this, an upgrade to OC 10 with legitimate local storages would see them suddenly unmounted which could cause sync clients to start deleting local files when they see the folder is gone. (not good)

So: write a migration that checks all existing storage configs. If any of the storage configs contain at least one with the "Local" external storage, then set it explicitly to "yes".
If nothing was found, don't set anything and let it default to "no".

Makes sense ?

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 13, 2017

Member

Further clarification:

The current implementation is an absolute setting. This means that if existing local storages existed, flipping the switch would make them disappear.

However actual the requirement is different:

  • the setting must only prevent creating new "Local" storage mounts from the web UI when disabled. So if "Local" storages existed before, they will continue to exist.
  • the setting must be disabled by default (no "Local" storages can be created)
  • occ commands for external storage are not affected by the setting.

Let's add a second config.php setting called "files_external_allow_create_new_local" that fits this purpose.

The permission check needs to be done in the UI controllers:

@grischdian another challenge ? or @noveens ?

Member

PVince81 commented Mar 13, 2017

Further clarification:

The current implementation is an absolute setting. This means that if existing local storages existed, flipping the switch would make them disappear.

However actual the requirement is different:

  • the setting must only prevent creating new "Local" storage mounts from the web UI when disabled. So if "Local" storages existed before, they will continue to exist.
  • the setting must be disabled by default (no "Local" storages can be created)
  • occ commands for external storage are not affected by the setting.

Let's add a second config.php setting called "files_external_allow_create_new_local" that fits this purpose.

The permission check needs to be done in the UI controllers:

@grischdian another challenge ? or @noveens ?

@noveens

This comment has been minimized.

Show comment
Hide comment
@noveens

noveens Mar 13, 2017

Contributor

Would love to take this after #27359 .

Contributor

noveens commented Mar 13, 2017

Would love to take this after #27359 .

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Apr 4, 2017

Member

@noveens do you still intend to work on this or should someone else take over ?

Member

PVince81 commented Apr 4, 2017

@noveens do you still intend to work on this or should someone else take over ?

@noveens

This comment has been minimized.

Show comment
Hide comment
@noveens

noveens Apr 4, 2017

Contributor

@PVince81 ,
Will take this in 3-4 days.

Thanks for reminding 😃

Contributor

noveens commented Apr 4, 2017

@PVince81 ,
Will take this in 3-4 days.

Thanks for reminding 😃

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