Handle invalid ext storage backend to keep mount point visible #26539

PVince81 · 2016-11-03T11:21:15Z

Description

Keep mount point visible and also ext storage config visible when
dealing with configs relating to storage backends or auth mechanisms
that were provided by an app that is currently disabled.

Introduces InvalidBackend and InvalidAuth placeholders to be able to properly pass around the StorageConfig. This makes it possible to keep the mount point visible but have it be a FailedStorage that gracefully throws StorageNotAvailableException which results in 503 Storage not available for clients.

Related Issue

Fixes #26538

Motivation and Context

See original ticket.
Basically an admin might forget to reenable storage apps for which there are existing configs.
The current code would remove the mount point completely which would cause sync clients to delete local files.

How Has This Been Tested?

TEST: invalid backend keeps mount point
TEST: sync client behavior with invalid backend
TEST: invalid auth keeps mount point
TEST: broken config is read-only in web UI (admin + personal page)
TEST: broken config can be deleted in web UI
TEST: broken config cannot be modified with CLI commands
TEST: broken config can be deleted with CLI commands
TEST: broken config cannot be exported with CLI commands, shows warning (because storage class is missing)
TEST: reenable storage app (ex: WND), mount point available again, sync client functions normally (no redownload)

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.

@owncloud/filesystem @butonic

mention-bot · 2016-11-03T11:21:17Z

@PVince81, thanks for your PR! By analyzing the history of the files in this pull request, we identified @icewind1991, @Xenopathic and @butonic to be potential reviewers.

PVince81 · 2016-12-06T17:41:18Z

Rebased

PVince81 · 2017-10-17T15:02:52Z

Bringing to "development" as discussed with @pmaier1.

This will ease the migration to external storage apps. It will preserve the mount points and configs present but not editable (not editable for technical reasons), so that when the user installs the external storage app the mounts will work again.

PVince81 · 2017-10-17T15:04:47Z

Rebased.

I have tested this with the app "files_external_gdrive":

Install "files_external_gdrive"
Configure a GDrive mount point
Access it and see that downloading work
Disable the "files_external_gdrive" app
Refresh web UI
Check mount point
Reenable app

Before the fix: web UI would crash with some errors about missing storage classes
After the fix: web UI shows the "/gdrive" mount point in red. Accessing it through Webdav returns 503 Storage not available. After reenabling the app, the mount point works again.

@jvillafanez please review

PVince81 · 2017-11-13T09:58:47Z

rebased, fixed tests

PVince81 · 2017-11-13T10:21:27Z

I've retested all cases from the first ticket.
The only missing thing is:

BUG: prevent exporting incomplete config from CLI when backend is not known.

PVince81 · 2017-11-13T10:31:26Z

I decided to only display a warning when listing configs on the CLI. The warning tells about how many invalid storages there are and that the admin must enable apps, else the config is incomplete.
The warning doesn't appear when exporting to JSON because we can't output to stderr directly from OutputInterface.

PVince81 · 2017-11-13T10:31:46Z

@jvillafanez second review ?

PVince81 · 2017-11-13T10:48:28Z

Squashed a few unit test additions to increase coverage.

codecov · 2017-11-13T10:48:31Z

Codecov Report

Merging #26539 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master   #26539      +/-   ##
============================================
+ Coverage     61.63%   61.65%   +0.02%     
- Complexity    17477    17482       +5     
============================================
  Files          1044     1047       +3     
  Lines         57691    57706      +15     
============================================
+ Hits          35555    35577      +22     
+ Misses        22136    22129       -7

Impacted Files	Coverage Δ	Complexity Δ
lib/private/Files/External/InvalidStorage.php	`100% <100%> (ø)`	`1 <1> (?)`
...private/Files/External/Service/StoragesService.php	`93.06% <100%> (+0.06%)`	`60 <0> (+1)`	⬆️
lib/public/Files/External/Auth/InvalidAuth.php	`100% <100%> (ø)`	`1 <1> (?)`
...b/public/Files/External/Backend/InvalidBackend.php	`100% <100%> (ø)`	`2 <2> (?)`
lib/public/Files/External/Backend/Backend.php	`85.71% <0%> (+14.28%)`	`7% <0%> (ø)`	⬇️
lib/private/Files/External/IdentifierTrait.php	`40.9% <0%> (+18.18%)`	`8% <0%> (ø)`	⬇️

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 36fbfc6...cdda2f0. Read the comment docs.

PVince81 · 2017-11-13T11:03:21Z

BUG: when only auth plugin is not available, the config that had it selected looks broken:

This is with SFTP and keypair, but I commented out registration of the keypair plugin after creation:

The right approach might be to do display something selected in the list but remove any other controls and add a text "Missing plugin for backend auth".

add admin-friendly message whenever a storage backend is not found, something like "please verify that the apps providing the storage backend are installed and enabled". We can't tell which app it is though.

jvillafanez · 2017-11-13T11:00:19Z

apps/files_external/lib/Command/ListCommand.php

I'd rather use 2 different messages or use a different message such as "number of invalid storages: $countInvalid"

jvillafanez · 2017-11-13T11:07:44Z

lib/public/Files/External/Auth/InvalidAuth.php

I think the following style is the most used, so we should keep it for consistency. The same for the InvalidBackend

$this->setIdentifier($invalidId) ->setScheme(self::SCHEME_NULL) ->setText('Unknown auth mechanism backend ' . $invalidId);

PVince81 · 2017-11-13T11:33:41Z

UI fixed:

invalid storage backend:
invalid auth backend:

PVince81 · 2017-11-13T14:13:29Z

@jvillafanez adjusted styles and fixed the failiing test

PVince81 · 2017-11-14T08:27:06Z

bah, of course I should have adjusted the tested message:
OCA\Files_External\Tests\Command\ListCommandTest.testDisplayWarningForIncomplete

Keep mount point visible and also ext storage config visible when dealing with configs relating to storage backends or auth mechanisms that were provided by an app that is currently disabled.

PVince81 · 2017-11-14T08:29:14Z

Fixed last test.

Also, I checked the codecov https://codecov.io/gh/owncloud/core/pull/26539/diff and all is green 😄

PVince81 · 2017-11-14T16:19:55Z

stable10: #29562

lock · 2019-08-01T23:02:03Z

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 2 - Developing app:files_external labels Nov 3, 2016

PVince81 added this to the 9.2 milestone Nov 3, 2016

PVince81 mentioned this pull request Nov 8, 2016

Move files external to core #25422

Merged

PVince81 force-pushed the extstorage-invalidbackendhandling branch from c17101d to 267263b Compare December 6, 2016 17:41

PVince81 mentioned this pull request Jan 13, 2017

Client deletes all data from external storage on mount-error #13238

Closed

PVince81 force-pushed the extstorage-invalidbackendhandling branch from 267263b to 0ea7b46 Compare February 3, 2017 10:25

PVince81 modified the milestones: 10.0.1, 10.0 Apr 26, 2017

DeepDiver1975 modified the milestones: 10.0.1, 10.1 May 17, 2017

PVince81 mentioned this pull request Jun 2, 2017

Bye bye Dropbox v1 #28073

Merged

9 tasks

PVince81 added the backport-request label Jun 2, 2017

PVince81 force-pushed the extstorage-invalidbackendhandling branch from 0ea7b46 to d055015 Compare June 2, 2017 15:18

This was referenced Aug 16, 2017

Downstream - removal of outdated API nextcloud/server#6146

Merged

Downstream 26539 & 28074 - proper handling of invalid config nextcloud/server#6149

Merged

Handle invalid ext storage backend to keep mount point visible nextcloud/server#6196

Merged

PVince81 force-pushed the extstorage-invalidbackendhandling branch from d055015 to f4ed76a Compare October 17, 2017 14:23

PVince81 modified the milestones: 10.1, development Oct 17, 2017

PVince81 self-assigned this Oct 17, 2017

PVince81 added 3 - To Review and removed 2 - Developing labels Oct 17, 2017

PVince81 force-pushed the extstorage-invalidbackendhandling branch from f4ed76a to 52fe70b Compare October 17, 2017 15:34

PVince81 mentioned this pull request Nov 6, 2017

[stable10] Bye bye Dropbox v1 #29135

Merged

PVince81 force-pushed the extstorage-invalidbackendhandling branch from 16803d2 to 88b847f Compare November 13, 2017 09:58

PVince81 force-pushed the extstorage-invalidbackendhandling branch from 88b847f to 925885b Compare November 13, 2017 10:48

PVince81 force-pushed the extstorage-invalidbackendhandling branch from 925885b to 8230e45 Compare November 13, 2017 10:57

jvillafanez approved these changes Nov 13, 2017

View reviewed changes

PVince81 force-pushed the extstorage-invalidbackendhandling branch from 8230e45 to edeaf6a Compare November 13, 2017 11:32

PVince81 force-pushed the extstorage-invalidbackendhandling branch 2 times, most recently from 5b31f3b to 104434e Compare November 13, 2017 14:13

Handle invalid ext storage backend to keep mount point visible

cdda2f0

Keep mount point visible and also ext storage config visible when dealing with configs relating to storage backends or auth mechanisms that were provided by an app that is currently disabled.

PVince81 force-pushed the extstorage-invalidbackendhandling branch from 104434e to cdda2f0 Compare November 14, 2017 08:29

PVince81 added 4 - To release and removed 3 - To Review labels Nov 14, 2017

PVince81 merged commit 4a13603 into master Nov 14, 2017

PVince81 deleted the extstorage-invalidbackendhandling branch November 14, 2017 16:17

PVince81 mentioned this pull request Nov 14, 2017

[stable10] Handle invalid ext storage backend to keep mount point visible #29562

Merged

DeepDiver1975 removed the backport-request label Nov 24, 2017

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

Handle invalid ext storage backend to keep mount point visible #26539

Handle invalid ext storage backend to keep mount point visible #26539

Uh oh!

Conversation

PVince81 commented Nov 3, 2016 • 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:

Uh oh!

mention-bot commented Nov 3, 2016

Uh oh!

PVince81 commented Dec 6, 2016

Uh oh!

PVince81 commented Oct 17, 2017

Uh oh!

PVince81 commented Oct 17, 2017

Uh oh!

PVince81 commented Nov 13, 2017

Uh oh!

PVince81 commented Nov 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PVince81 commented Nov 13, 2017

Uh oh!

PVince81 commented Nov 13, 2017

Uh oh!

PVince81 commented Nov 13, 2017

Uh oh!

codecov bot commented Nov 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

PVince81 commented Nov 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jvillafanez Nov 13, 2017

Choose a reason for hiding this comment

Uh oh!

jvillafanez Nov 13, 2017

Choose a reason for hiding this comment

Uh oh!

PVince81 commented Nov 13, 2017

Uh oh!

PVince81 commented Nov 13, 2017

Uh oh!

PVince81 commented Nov 14, 2017

Uh oh!

PVince81 commented Nov 14, 2017

Uh oh!

PVince81 commented Nov 14, 2017

Uh oh!

lock bot commented Aug 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

PVince81 commented Nov 3, 2016 •

edited

Loading

PVince81 commented Nov 13, 2017 •

edited

Loading

codecov bot commented Nov 13, 2017 •

edited

Loading

PVince81 commented Nov 13, 2017 •

edited

Loading