Enable embedder frame to override feature flags

Allow the embedder frame to specify a list of feature flags that should be
overridden in addition to those enabled in the H user profile. This will be used
in the Hypothesis LMS app to enable feature flags to be turned on at the level
of an LMS app installation, rather than for individual users.

The ability to override flags has been limited to embedders (in practice, our
LMS app) to try and avoid a situation where arbitrary web pages enable, and
become dependent upon, feature flags that we later remove.

The flags coming from the embedder add to, rather than replacing, the profile
flags. This has been done to avoid a confusing situation where a feature flag
that is enabled for "everyone" in H actually ends up not being enabled for all
users due to eg. the LMS app disabling it for certain installations.

Part of https://github.com/hypothesis/client/issues/5803

 - Change `ConfigFromHost` type to only include settings that can come from
   either the host frame or embedder frame. Settings that can only come from
   a specific context have been moved into the `ConfigFromAnnotator` or
   `ConfigFromEmbedder` types respectively.

 - Add `features` config to `ConfigFromEmbedder`

 - Add `features` store method which combines feature flags from profile and
   global settings

 - Replace some references to `Config*` types in `build-settings-test.js`. The
   type references were wrong because RPC calls return the `ConfigFromEmbedder`
   type, not `ConfigFromHost`.

src/sidebar/config/test/build-settings-test.js

@@ -123,7 +123,7 @@ describe('sidebar/config/build-settings', () => {
         );
       });
 
-      it('adds RPC settings to resulting SidebarSettings', async () => {
+      it('adds settings received via RPC to merged settings', async () => {
         fakeJsonRpc.call.resolves({ foo: 'baz' }); // host config
 
         const result = await buildSettings({}, fakeWindow);
@@ -134,7 +134,7 @@ describe('sidebar/config/build-settings', () => {
         });
       });
 
-      it('merges ConfigFromHost returned from RPC with ConfigFromSidebar', async () => {
+      it('merges settings received via RPC with sidebar settings', async () => {
         const configFromSidebar = { foo: 'bar', appType: 'via' };
         fakeJsonRpc.call.resolves({ foo: 'baz' }); // host config
         const result = await buildSettings(configFromSidebar, fakeWindow);
@@ -149,7 +149,7 @@ describe('sidebar/config/build-settings', () => {
         });
       });
 
-      it('rejects if RPC request for ConfigFromHost fails', async () => {
+      it('rejects if RPC request for config fails', async () => {
         fakeJsonRpc.call.rejects(new Error('Nope'));
         const configFromSidebar = { foo: 'bar', appType: 'via' };
         await assert.rejects(

src/sidebar/services/frame-sync.ts

@@ -476,7 +476,7 @@ export class FrameSyncService {
 
     // Synchronize highlight visibility in this guest with the sidebar's controls.
     guestRPC.call('setHighlightsVisible', this._highlightsVisible);
-    guestRPC.call('featureFlagsUpdated', this._store.profile().features);
+    guestRPC.call('featureFlagsUpdated', this._store.features());
 
     // If we have content banner data, send it to the guest. If there are
     // multiple guests the banner is likely only appropriate for the main one.
@@ -514,7 +514,7 @@ export class FrameSyncService {
    * Set up synchronization of feature flags to host and guest frames.
    */
   private _setupFeatureFlagSync() {
-    const getFlags = () => this._store.profile().features;
+    const getFlags = () => this._store.features();
 
     const sendFlags = (flags: Record<string, boolean>) => {
       this._hostRPC.call('featureFlagsUpdated', flags);

src/sidebar/services/test/frame-sync-test.js

@@ -104,8 +104,8 @@ describe('FrameSyncService', () => {
     fakeStore = fakeReduxStore(
       {
         annotations: [],
+        features: {},
         frames: [],
-        profile: { features: {} },
         contentInfo: null,
       },
       {
@@ -132,12 +132,12 @@ describe('FrameSyncService', () => {
           this.setState({ frames: frames.filter(f => f.id !== frame.id) });
         },
 
-        frames() {
-          return this.getState().frames;
+        features() {
+          return this.getState().features;
         },
 
-        profile() {
-          return this.getState().profile;
+        frames() {
+          return this.getState().frames;
         },
 
         getContentInfo() {
@@ -1081,8 +1081,8 @@ describe('FrameSyncService', () => {
   });
 
   describe('sending feature flags to frames', () => {
-    const currentFlags = () => fakeStore.getState().profile.features;
-    const setFlags = features => fakeStore.setState({ profile: { features } });
+    const currentFlags = () => fakeStore.getState().features;
+    const setFlags = features => fakeStore.setState({ features });
 
     beforeEach(async () => {
       // Set some initial flags before the host frame is even connected.

src/sidebar/store/modules/session.ts

+import { createSelector } from 'reselect';
+
 import type { Profile } from '../../../types/api';
 import type { SidebarSettings } from '../../../types/config';
 import { createStoreModule, makeAction } from '../create-store';
 
 export type State = {
+  /**
+   * The app's default authority (user identity provider), from settings,
+   * e.g. `hypothes.is` or `localhost`
+   *
+   * FIXME: This is an empty string when `authDomain` is missing
+   * because other app logic has long assumed its string-y presence:
+   * behavior when it's missing is undefined. This setting should be
+   * enforced similarly to how `apiUrl` is enforced.
+   */
   defaultAuthority: string;
+
+  /**
+   * Feature flags to enable, in addition to those that are enabled in the
+   * user's profile.
+   *
+   * This is used in the LMS app for example to enable features based on the
+   * LMS app installation in use, rather than the active H user account.
+   */
+  features: string[];
+
+  /**
+   * Profile object fetched from the `/api/profile` endpoint.
+   */
   profile: Profile;
 };
 
@@ -22,20 +46,10 @@ const initialProfile: Profile = {
   userid: null,
 };
 
-function initialState(settings: SidebarSettings) {
+function initialState(settings: SidebarSettings): State {
   return {
-    /**
-     * The app's default authority (user identity provider), from settings,
-     * e.g. `hypothes.is` or `localhost`
-     * FIXME: This returns an empty string when `authDomain` is missing
-     * because other app logic has long assumed its string-y presence:
-     * behavior when it's missing is undefined. This setting should be
-     * enforced similarly to how `apiUrl` is enforced.
-     */
     defaultAuthority: settings?.authDomain ?? '',
-    /**
-     * Profile object fetched from the `/api/profile` endpoint.
-     */
+    features: settings.features ?? [],
     profile: initialProfile,
   };
 }
@@ -66,6 +80,24 @@ function isLoggedIn(state: State) {
   return state.profile.userid !== null;
 }
 
+/**
+ * Return the effective set of feature flags. This combines feature flags from
+ * the profile with those from other sources.
+ */
+const features = createSelector(
+  (state: State) => state.profile,
+  (state: State) => state.features,
+  (profile: Profile, features: string[]): Record<string, boolean> => {
+    const combinedFeatures = { ...profile.features };
+    if (features) {
+      for (const feat of features) {
+        combinedFeatures[feat] = true;
+      }
+    }
+    return combinedFeatures;
+  },
+);
+
 /**
  * Return true if a given feature flag is enabled for the current user.
  *
@@ -73,7 +105,7 @@ function isLoggedIn(state: State) {
  *   feature flag as declared in the Hypothesis service.
  */
 function isFeatureEnabled(state: State, feature: string) {
-  return !!state.profile.features[feature];
+  return Boolean(features(state)[feature]);
 }
 
 /**
@@ -92,6 +124,9 @@ function hasFetchedProfile(state: State) {
  *
  * If the profile has not yet been fetched yet, a dummy logged-out profile is
  * returned. This allows code to skip a null check.
+ *
+ * NOTE: To check the set of enabled features, use the {@link features} selector
+ * instead, since it combines features from the profile and other sources.
  */
 function profile(state: State) {
   return state.profile;
@@ -107,6 +142,7 @@ export const sessionModule = createStoreModule(initialState, {
 
   selectors: {
     defaultAuthority,
+    features,
     hasFetchedProfile,
     isFeatureEnabled,
     isLoggedIn,

src/sidebar/store/modules/test/session-test.js

@@ -58,6 +58,43 @@ describe('sidebar/store/modules/session', () => {
         assert.equal(store.isFeatureEnabled('some_feature'), enabled);
       });
     });
+
+    it('returns true if feature is enabled in global `features` setting', () => {
+      const settings = {
+        ...fakeSettings,
+        features: ['some_feature'],
+      };
+      store = createStore([sessionModule], [settings]);
+      assert.isTrue(store.isFeatureEnabled('some_feature'));
+    });
+  });
+
+  describe('#features', () => {
+    beforeEach(() => {
+      const settings = {
+        ...fakeSettings,
+        features: ['global_feature'],
+      };
+      store = createStore([sessionModule], [settings]);
+    });
+
+    it('returns only global features before profile is loaded', () => {
+      assert.deepEqual(store.features(), {
+        global_feature: true,
+      });
+    });
+
+    it('returns combined features after profile is loaded', () => {
+      store.updateProfile({
+        userid: null,
+        features: { user_feature: true, other_feature: false },
+      });
+      assert.deepEqual(store.features(), {
+        global_feature: true,
+        user_feature: true,
+        other_feature: false,
+      });
+    });
   });
 
   describe('#hasFetchedProfile', () => {

src/types/config.ts

@@ -104,7 +104,7 @@ export type ThemeProperty =
  * `ConfigFromAnnotator` OR by an ancestor ("embedder frame") as
  * `ConfigFromEmbedder`.
  *
- * This is the subset of keys from
+ * This is mostly a subset of keys from
  * https://h.readthedocs.io/projects/client/en/latest/publishers/config/ which
  * excludes any keys used only by the "annotator" part of the application.
  */
@@ -135,20 +135,6 @@ export type ConfigFromHost = {
   /** Whether to show the "New note" button on the "Page notes" tab. */
   enableExperimentalNewNoteButton?: boolean;
 
-  /**
-   * Instructs the client to fetch configuration from an ancestor of the host
-   * frame.
-   *
-   * This is primarily used in Hypothesis's LMS integration.
-   */
-  requestConfigFromFrame?: EmbedderFrameConfig;
-
-  /**
-   * Request notifications be delivered to the frame specified by
-   * `requestConfigFromFrame` when certain annotation activity happens.
-   */
-  reportActivity?: ReportAnnotationActivityConfig;
-
   /** Configuration for the annotation services that the client connects to. */
   services?: Service[];
 
@@ -219,11 +205,41 @@ export type RPCSettings = {
  * |  +------------------------------------------+                          |
  * +------------------------------------------------------------------------+
  */
-export type SidebarSettings = ConfigFromHost &
+export type SidebarSettings = ConfigFromAnnotator &
+  ConfigFromEmbedder &
   ConfigFromSidebar & { rpc?: RPCSettings };
 
-/** See {@link SidebarSettings} */
-export type ConfigFromAnnotator = Omit<ConfigFromHost, 'reportActivity'>;
+/**
+ * Configuration passed to Hypothesis client from the host frame.
+ */
+export type ConfigFromAnnotator = ConfigFromHost & {
+  /**
+   * Instructs the client to fetch configuration from an ancestor of the host
+   * frame.
+   *
+   * This is primarily used in Hypothesis's LMS integration.
+   */
+  requestConfigFromFrame?: EmbedderFrameConfig;
+};
+
+/**
+ * Configuration passed to Hypothesis client from the embedder frame. This is
+ * primarily used in Hypothesis's LMS integration.
+ *
+ * This is a superset of the configuration which can be passed from the host
+ * frame which enables some additional configuration that we don't want to
+ * allow arbitrary web pages to set.
+ */
+export type ConfigFromEmbedder = ConfigFromHost & {
+  /**
+   * Feature flags to enable. When a flag is listed here, it will be turned
+   * on even if disabled in the H user profile.
+   */
+  features?: string[];
 
-/** See {@link SidebarSettings} */
-export type ConfigFromEmbedder = Omit<ConfigFromHost, 'requestConfigFromFrame'>;
+  /**
+   * Request notifications be delivered to the frame specified by
+   * `requestConfigFromFrame` when certain annotation activity happens.
+   */
+  reportActivity?: ReportAnnotationActivityConfig;
+};