Skip to content

(NII-Proposal) feat(files): add dynamic file provider registry for foreign addon support - #1066

Draft
chiku-samugari wants to merge 1 commit into
CenterForOpenScience:developfrom
chiku-samugari:feat/file-provider-dynamic-registry
Draft

chiku-samugari wants to merge 1 commit into
CenterForOpenScience:developfrom
chiku-samugari:feat/file-provider-dynamic-registry

Conversation

@chiku-samugari

Copy link
Copy Markdown
  • Ticket: []
  • Feature flag: n/a

Purpose

The Files route /{guid}/files/{provider} is guarded by isFileProvider, which admits only the provider
names listed in the build-time constant FileProvider. A storage service that GravyValet serves under any
other 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

  • Add FileProviderRegistryService, the registry of valid provider names.
    • The built-in FileProvider values and the external_service_name of every service returned by
      GravyValet's GET /v1/external-storage-services are registered as valid provider names.
    • Initialize the registry in initializeApplication(), which Angular runs during bootstrap and waits
      for, so that the registry is filled before the router evaluates canMatch.
    • Because the request runs during bootstrap on every page, it bypasses the global error interceptor
      (no toast, no redirect when GravyValet is unavailable), asks only for external_service_name, and
      gives up after 5 seconds.
    • If the request fails, the registry falls back to the built-in names.
    • Names are compared case-insensitively.
  • Make isFileProvider ask the registry instead of the build-time list.
  • Introduce a new type FileProviderType for the provider field 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.
  • Fall back to the name served by GravyValet for a service that the AddonServiceNames enum does not list.
    This avoids an empty name in the success toasts and in the header of the disconnect dialog.
  • Tests: new spec for the registry, rewritten spec for the guard, and the initializer spec now provides the
    registry and covers the ordering (config first, bootstrap completes after the registry). New spec for
    AddonDialogService. The specs of ConnectConfiguredAddonComponent and ConnectAddonComponent were
    skipped (describe.skip); they run now, with a router mock that supplies the navigation state both
    components 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 only
    read 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 the
    first 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.
  • Bootstrap now waits for that request, for 5 seconds at most. A failed request is reported to Sentry by
    the interceptor's bypass path; a timeout request is not reported. Neither is visible to the user.
  • The provider of the files state can now be a name that FileProvider does not list. The type
    allowed this before (it resolved to string), so this is a change of the values at run time, not of the
    types. Code that compares the provider with a built-in name must expect other values. The existing
    comparisons are equality checks with FileProvider.OsfStorage or FileProvider.GoogleDrive; a dynamic
    name takes the "is not that provider" branch, which is the intended behaviour (no revisions, no Google
    picker).

QA Notes

  • Pages: Files page of a project and of a registration; project overview files widget; move/copy dialog.
  • Cases:
    • a built-in provider (/{guid}/files/osfstorage, /{guid}/files/googledrive) opens as before;
    • with a foreign storage add-on configured on the project, /{guid}/files/<name> opens, including on a
      direct page load and reload;
    • an unknown name (/{guid}/files/nope) is still rejected;
    • logged-out visitor on a public project: Files page works;
    • GravyValet unreachable: the application boots.
  • Add-on names (project Add-ons page and user settings Add-ons page):
    • foreign service: the disconnect dialog header and the success toast after connecting show the
      service's name instead of an empty one;
    • built-in service (e.g. Dropbox): the texts are not changed.
  • Bootstrap and GravyValet availability (because the initializer now performs network access):
    • GravyValet answering slowly: the application starts after 5 seconds at the latest;
    • GravyValet answering with an error (401, 403, 5xx): no toast, no redirect to sign-in or to the
      forbidden page, the application starts with the built-in providers;
    • test environments that stub or block backend requests need an answer (or a block) for
      GET {addonsApiUrl}/external-storage-services.
  • Risk: low. The guard only becomes more permissive, and only for names GravyValet lists.
  • Cross-browser testing: not required.
  • Requires a GravyValet that includes PR #318, with at least one foreign storage add-on registered.

…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant