(NII-Proposal) feat(files): add dynamic file provider registry for foreign addon support - #1066
Draft
chiku-samugari wants to merge 1 commit into
Conversation
…port
The Files route was guarded by a build-time list of provider names, so a
storage service that GravyValet serves under any other name (a foreign
addon imp) could not be opened.
Changes:
- add `FileProviderRegistryService`, which registers the built-in names
and the names GravyValet lists as valid file providers
- initialize the registry during application bootstrap, so that it is
filled before the router evaluates the guard
- the request bypasses the error interceptor, asks only for the
service name and gives up after 5 seconds
- on failure or timeout the registry keeps the built-in names
- make `isFileProvider` ask the registry instead of the build-time list
- introduce `FileProviderType` for the provider of the files state: a
built-in name or a dynamic one (no change for the compiler, the
previous type already resolved to `string`)
- show the name of a service that `AddonServiceNames` does not list in
the disconnect dialog and in the success toasts after connecting
Tests:
- add specs for `FileProviderRegistryService` and `AddonDialogService`
- rewrite the `isFileProvider` guard spec against the registry
- cover the bootstrap order in the initializer spec: the config is
loaded first, and bootstrap completes only after the registry
- revive the skipped specs of `ConnectConfiguredAddonComponent` and
`ConnectAddonComponent`: the router mock supplies the navigation
state that both components read in their constructor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
The Files route
/{guid}/files/{provider}is guarded byisFileProvider, which admits only the providernames listed in the build-time constant
FileProvider. A storage service that GravyValet serves under anyother name cannot be opened: the route does not match, the router falls through to the file-detail route,
and the page shows an empty file view for a file that does not exist.
GravyValet PR #318 (Foreign Addon Imps)
lets a deployment add storage add-on implementations as external packages. Such a service is registered in
GravyValet, served by osf.io and WaterButler, and listed on the Add-ons page, but the Files page refuses
its name. This PR removes that last build-time allow-list.
Summary of Changes
FileProviderRegistryService, the registry of valid provider names.FileProvidervalues and theexternal_service_nameof every service returned byGravyValet's
GET /v1/external-storage-servicesare registered as valid provider names.initializeApplication(), which Angular runs during bootstrap and waitsfor, so that the registry is filled before the router evaluates
canMatch.(no toast, no redirect when GravyValet is unavailable), asks only for
external_service_name, andgives up after 5 seconds.
isFileProviderask the registry instead of the build-time list.FileProviderTypefor theproviderfield of the files state (FilesStateModel).The type states what the field holds with this PR: a built-in name, or a name that GravyValet lists.
This avoids an empty name in the success toasts and in the header of the disconnect dialog.
registry and covers the ordering (config first, bootstrap completes after the registry). New spec for
AddonDialogService. The specs ofConnectConfiguredAddonComponentandConnectAddonComponentwereskipped (
describe.skip); they run now, with a router mock that supplies the navigation state bothcomponents read in their constructor.
For deployments without foreign storage add-ons, the only change is the bootstrap request described under
Side Effects: the built-in names are always valid and every other provider-specific behaviour
(osfstorage revisions, Google Drive picker) is untouched.
Screenshot(s)
(preparing)
Side Effects
initializeApplication()now performs network access. Until now the application initializer onlyread the local
config.json. With this change it also sends one request to GravyValet(
GET /v1/external-storage-services) on every application start, in the browser and in SSR. This is thefirst bootstrap-time dependency on a backend service, so it is relevant for QA environments, E2E test
setups and monitoring: GravyValet is contacted before any page is rendered, including pages that have
nothing to do with files.
Under SSR the application starts once per rendered request, so each server-rendered page sends this
request and waits for it, for 5 seconds at most.
the interceptor's bypass path; a timeout request is not reported. Neither is visible to the user.
providerof the files state can now be a name thatFileProviderdoes not list. The typeallowed this before (it resolved to
string), so this is a change of the values at run time, not of thetypes. Code that compares the provider with a built-in name must expect other values. The existing
comparisons are equality checks with
FileProvider.OsfStorageorFileProvider.GoogleDrive; a dynamicname takes the "is not that provider" branch, which is the intended behaviour (no revisions, no Google
picker).
QA Notes
/{guid}/files/osfstorage,/{guid}/files/googledrive) opens as before;/{guid}/files/<name>opens, including on adirect page load and reload;
/{guid}/files/nope) is still rejected;service's name instead of an empty one;
forbidden page, the application starts with the built-in providers;
GET {addonsApiUrl}/external-storage-services.