From 6e9c0277a3c228f15f71986a54e6fbb0e0a23ef2 Mon Sep 17 00:00:00 2001 From: John Date: Fri, 19 Apr 2024 17:22:19 +0100 Subject: [PATCH] Move vnext changes into v18 (#709) Extracted AppId check into its own file and added tests to validate behaviour. --- .../workspace-platform-starter/CHANGELOG.md | 1 + .../broker/app-id-helper.ts | 93 +++++++++ .../broker/wps-interop-override.ts | 59 +----- .../app-id-helper.spec.ts | 190 ++++++++++++++++++ 4 files changed, 295 insertions(+), 48 deletions(-) create mode 100644 how-to/workspace-platform-starter/client/src/modules/interop-override/wps-interop-override/broker/app-id-helper.ts create mode 100644 how-to/workspace-platform-starter/test/modules/interop-override/wps-interop-override/app-id-helper.spec.ts diff --git a/how-to/workspace-platform-starter/CHANGELOG.md b/how-to/workspace-platform-starter/CHANGELOG.md index 9004ab816f..fab7edaa99 100644 --- a/how-to/workspace-platform-starter/CHANGELOG.md +++ b/how-to/workspace-platform-starter/CHANGELOG.md @@ -2,6 +2,7 @@ ## v18.0.0 +- More efficient validation of appIds associated with views/windows for faster interop based actions. The appId validation in our wps module (modules directory) has extracted into its own file and tests to ensure it continues to behave as expected across future updates have been added to the test directory. - Module Helpers now provide a getAnalyticsClient (it is marked as optional and the result could be undefined so it leaves the option for it to be denied to a module or removed). This client supports a ModuleAnalytic event which will have a source of Module assigned to it (you can still specify type and use the data property to provide additional module specific information). This data will be passed to the analyticProviders that receive the Workspace Analytic events. See [How to Configure Analytics](./docs/how-to-configure-analytics.md). - Added a cloud interop override module so that you can easily test out OpenFin's cloud interop offering. See [How To Add Cloud Interop To Your Interop Broker](./docs/how-to-add-cloud-interop-to-your-interop-broker.md). - Broke up the build process to make it easier to just build your modules. `npm run build` still builds everything and `npm run build-client` still builds all client related code but now if you change framework files you can use `npm run build-framework`, or if you modify our starter modules you can use `npm run build-starter-modules` or if you just want to build your modules (that are listed in webpack.config.js) then you can use `npm run build-client-modules`. This will let you have a much faster build. diff --git a/how-to/workspace-platform-starter/client/src/modules/interop-override/wps-interop-override/broker/app-id-helper.ts b/how-to/workspace-platform-starter/client/src/modules/interop-override/wps-interop-override/broker/app-id-helper.ts new file mode 100644 index 0000000000..67a47d5f1a --- /dev/null +++ b/how-to/workspace-platform-starter/client/src/modules/interop-override/wps-interop-override/broker/app-id-helper.ts @@ -0,0 +1,93 @@ +import type OpenFin from "@openfin/core"; +import type { PlatformApp } from "../../../../framework/shapes/app-shapes"; +import type { Logger } from "../../../../framework/shapes/logger-shapes"; +/** + * The AppIdHelper class provides helpful functions related to app ids. + */ +export class AppIdHelper { + private readonly _validatedAppIds: string[] = []; + + private readonly _invalidAppIds: string[] = []; + + private readonly _unregisteredApp: PlatformApp | undefined; + + private readonly _logger: Logger; + + private readonly _getApp: (appId: string) => Promise; + + private readonly _platformId: string; + + /** + * Provides helpful functions related to app ids. + * @param getApp The function to use to get an app for validation. + * @param platformId The platform id that represents the current platform. + * @param logger The logger to use + * @param unregisteredApp The app to use as a placeholder for unregistered apps. + */ + constructor( + getApp: (appId: string) => Promise, + platformId: string, + logger: Logger, + unregisteredApp?: PlatformApp + ) { + this._unregisteredApp = unregisteredApp; + this._logger = logger; + this._getApp = getApp; + this._platformId = platformId; + } + + /** + * Lookup an application identity. + * @param clientIdentity The client identity to use. + * @returns The application identity. + */ + public async lookupAppId(clientIdentity: OpenFin.ClientIdentity): Promise { + if (clientIdentity.name.startsWith("internal-generated-")) { + if (clientIdentity.uuid === this._platformId) { + if (this._unregisteredApp) { + this._logger.debug( + `A window or view that is not an app but runs within the platform is running and a placeholder app has been specified ${this._unregisteredApp?.appId}}.`, + clientIdentity + ); + return this._unregisteredApp.appId; + } + this._logger.debug( + "A window or view that is not an app but runs within the platform is running and no unregistered placeholder app is specified so no appId will be returned.", + clientIdentity + ); + return; + } + this._logger.debug( + "A window or view that follows the runtime generated naming convention is running from another platform. It will not be assigned an appId.", + clientIdentity + ); + return; + } + const nameParts = clientIdentity.name.split("/"); + let appId: string | undefined; + if (nameParts.length === 1 || nameParts.length === 2) { + appId = nameParts[0]; + } else { + appId = `${nameParts[0]}/${nameParts[1]}`; + } + + if (this._validatedAppIds.includes(appId)) { + return appId; + } + if (this._invalidAppIds.includes(appId)) { + return; + } + + // perform a lookup to validate the appId + const app = await this._getApp(appId); + + if (app) { + this._validatedAppIds.push(appId); + return appId; + } + this._invalidAppIds.push(appId); + this._logger.warn( + `AppId ${appId} does not exist in the directory and it isn't a generated view/window that falls under this platform. No app id will be returned as it is unconfirmed.` + ); + } +} diff --git a/how-to/workspace-platform-starter/client/src/modules/interop-override/wps-interop-override/broker/wps-interop-override.ts b/how-to/workspace-platform-starter/client/src/modules/interop-override/wps-interop-override/broker/wps-interop-override.ts index 4f777493c2..afffb82eae 100644 --- a/how-to/workspace-platform-starter/client/src/modules/interop-override/wps-interop-override/broker/wps-interop-override.ts +++ b/how-to/workspace-platform-starter/client/src/modules/interop-override/wps-interop-override/broker/wps-interop-override.ts @@ -43,6 +43,7 @@ import { sanitizeString } from "workspace-platform-starter/utils"; import { getWindowPositionUsingStrategy } from "workspace-platform-starter/utils-position"; +import { AppIdHelper } from "./app-id-helper"; import { AppIntentHelper } from "./app-intent-helper"; import { ClientRegistrationHelper } from "./client-registration-helper"; import { IntentResolverHelper } from "./intent-resolver-helper"; @@ -70,8 +71,8 @@ export async function getConstructorOverride( if (helpers?.getEndpointClient) { endpointClient = await helpers?.getEndpointClient(); } - const launch = helpers.launchApp; + return (Base: OpenFin.Constructor) => /** * Extend the InteropBroker to handle intents. @@ -89,6 +90,8 @@ export async function getConstructorOverride( private readonly _metadataKey: Readonly; + private readonly _appIdHelper: AppIdHelper; + /** * Create a new instance of InteropBroker. */ @@ -96,10 +99,6 @@ export async function getConstructorOverride( super(); logger.info("Interop Broker Constructor applying settings."); this._appIntentHelper = new AppIntentHelper(getApps, logger); - this._clientRegistrationHelper = new ClientRegistrationHelper( - async (clientIdentity: OpenFin.ClientIdentity) => this.lookupAppId(clientIdentity), - logger - ); this._metadataKey = `_metadata_${randomUUID()}`; if (options.intentResolver) { this._intentResolverHelper = new IntentResolverHelper( @@ -114,6 +113,11 @@ export async function getConstructorOverride( if (!isEmpty(this._unregisteredApp)) { this._unregisteredApp.manifestType = MANIFEST_TYPES.UnregisteredApp.id; } + this._appIdHelper = new AppIdHelper(getApp, fin.me.identity.uuid, logger, this._unregisteredApp); + this._clientRegistrationHelper = new ClientRegistrationHelper( + async (clientIdentity: OpenFin.ClientIdentity) => this._appIdHelper.lookupAppId(clientIdentity), + logger + ); } /** @@ -845,7 +849,7 @@ export async function getConstructorOverride( payload, clientIdentity )) as ImplementationMetadata; - const appId = await this.lookupAppId(clientIdentity); + const appId = await this._appIdHelper.lookupAppId(clientIdentity); if (!isEmpty(appId)) { const updatedResponse = { ...response, @@ -1356,47 +1360,6 @@ export async function getConstructorOverride( return apiMetadata.type === "fdc3" && apiMetadata.version === "2.0"; } - /** - * Lookup an application identity. - * @param clientIdentity The client identity to use. - * @returns The application identity. - */ - private async lookupAppId(clientIdentity: OpenFin.ClientIdentity): Promise { - const nameParts = clientIdentity.name.split("/"); - let app: PlatformApp | undefined; - - if (nameParts.length === 1 || nameParts.length === 2) { - app = await getApp(nameParts[0]); - } - if (nameParts.length > 2) { - app = await getApp(`${nameParts[0]}/${nameParts[1]}`); - } - - const appNotFound = isEmpty(app); - - if (appNotFound && clientIdentity.uuid !== fin.me.identity.uuid) { - logger.warn( - "Lookup made by a non-registered app that is outside of this platform.", - clientIdentity - ); - return; - } - - if (appNotFound && isEmpty(this._unregisteredApp)) { - logger.warn( - "Lookup made by a non-registered app that falls under this platform. No unregistered placeholder app is specified.", - clientIdentity - ); - return; - } - - if (appNotFound) { - app = this._unregisteredApp; - logger.info("Assigned the following unregistered app to represent the app.", app); - } - return app?.appId; - } - /** * Process a context. * @param context The context to process. @@ -1428,7 +1391,7 @@ export async function getConstructorOverride( * @returns The context metadata. */ private async getContextMetadata(clientIdentity: OpenFin.ClientIdentity): Promise { - const appId = (await this.lookupAppId(clientIdentity)) ?? clientIdentity.name; + const appId = (await this._appIdHelper.lookupAppId(clientIdentity)) ?? "unknown"; return { source: { appId, diff --git a/how-to/workspace-platform-starter/test/modules/interop-override/wps-interop-override/app-id-helper.spec.ts b/how-to/workspace-platform-starter/test/modules/interop-override/wps-interop-override/app-id-helper.spec.ts new file mode 100644 index 0000000000..56703c2168 --- /dev/null +++ b/how-to/workspace-platform-starter/test/modules/interop-override/wps-interop-override/app-id-helper.spec.ts @@ -0,0 +1,190 @@ +import type { PlatformApp } from "../../../../client/src/framework/shapes/app-shapes"; +import type { Logger } from "../../../../client/src/framework/shapes/logger-shapes"; +import { AppIdHelper } from "../../../../client/src/modules/interop-override/wps-interop-override/broker/app-id-helper"; + +describe("app-id-helper", () => { + describe("isAppIdValid", () => { + const standardApp = "test-app-1"; + const directoryBasedApp = "directory1/test-app-2"; + const internalGeneratedViewWindow = "internal-generated-test"; + const platformId = "platform-id"; + const otherPlatformId = "other-platform-id"; + const unknownAppId = "unknown-app-id"; + const logger: Logger = { + debug: console.log, + info: console.log, + warn: console.warn, + error: console.error, + trace: console.trace + }; + + const apps: string[] = [standardApp, directoryBasedApp]; + /** + * Get an app by id. + * @param appId The app id to get. + * @returns The app or undefined if not found. + */ + async function getApp(appId: string): Promise { + if (apps.includes(appId)) { + return { appId } as PlatformApp; + } + return undefined; + } + + const unregisteredApp: PlatformApp = { appId: "unregistered-app" } as PlatformApp; + + it("should return undefined for internal generated views and windows from other platforms", async () => { + const appIdHelper = new AppIdHelper(getApp, platformId, logger); + const validatedAppId = await appIdHelper.lookupAppId({ + name: internalGeneratedViewWindow, + uuid: platformId, + endpointId: "x", + isLocalEndpointId: true + }); + expect(validatedAppId).toBeUndefined(); + }); + + it("should return undefined for internal generated views and windows from the same platform if no unregistered app is passed.", async () => { + const appIdHelper = new AppIdHelper(getApp, platformId, logger); + const validatedAppId = await appIdHelper.lookupAppId({ + name: internalGeneratedViewWindow, + uuid: platformId, + endpointId: "x", + isLocalEndpointId: true + }); + expect(validatedAppId).toBeUndefined(); + }); + + it("should return a defined unregistered appId for internal generated views and windows from the same platform if an unregistered app is passed.", async () => { + const appIdHelper = new AppIdHelper(getApp, platformId, logger, unregisteredApp); + const validatedAppId = await appIdHelper.lookupAppId({ + name: internalGeneratedViewWindow, + uuid: platformId, + endpointId: "x", + isLocalEndpointId: true + }); + expect(validatedAppId).toBe(unregisteredApp.appId); + }); + + it("should return an undefined appId for internal generated views and windows from a different platform even if an unregistered app is passed.", async () => { + const appIdHelper = new AppIdHelper(getApp, platformId, logger, unregisteredApp); + const validatedAppId = await appIdHelper.lookupAppId({ + name: internalGeneratedViewWindow, + uuid: otherPlatformId, + endpointId: "x", + isLocalEndpointId: false + }); + expect(validatedAppId).toBeUndefined(); + }); + + it("should return a standard appId if it exists in the directory.", async () => { + const appIdHelper = new AppIdHelper(getApp, platformId, logger, unregisteredApp); + const validatedAppId = await appIdHelper.lookupAppId({ + name: standardApp, + uuid: platformId, + endpointId: "x", + isLocalEndpointId: true + }); + expect(validatedAppId).toBe(standardApp); + }); + + it("should return a directory structured appId if it exists in the directory.", async () => { + const appIdHelper = new AppIdHelper(getApp, platformId, logger, unregisteredApp); + const validatedAppId = await appIdHelper.lookupAppId({ + name: `${directoryBasedApp}/guid`, + uuid: platformId, + endpointId: "x", + isLocalEndpointId: true + }); + expect(validatedAppId).toBe(directoryBasedApp); + }); + + it("should return a standard appId if it exists in the directory and comes from a different platform.", async () => { + const appIdHelper = new AppIdHelper(getApp, platformId, logger, unregisteredApp); + const validatedAppId = await appIdHelper.lookupAppId({ + name: standardApp, + uuid: otherPlatformId, + endpointId: "x", + isLocalEndpointId: false + }); + expect(validatedAppId).toBe(standardApp); + }); + + it("should return an undefined appId if an unlisted appId is provided.", async () => { + const appIdHelper = new AppIdHelper(getApp, platformId, logger, unregisteredApp); + const validatedAppId = await appIdHelper.lookupAppId({ + name: unknownAppId, + uuid: platformId, + endpointId: "x", + isLocalEndpointId: true + }); + expect(validatedAppId).toBeUndefined(); + }); + + it("A valid appId should not query the app directory more than once if queried more than once.", async () => { + const getAppSpy = jest.fn(getApp); + const appIdHelper = new AppIdHelper(getAppSpy, platformId, logger, unregisteredApp); + const clientIdentity = { + name: standardApp, + uuid: platformId, + endpointId: "x", + isLocalEndpointId: true + }; + let validatedAppId = await appIdHelper.lookupAppId(clientIdentity); + expect(validatedAppId).toBe(standardApp); + expect(getAppSpy).toHaveBeenCalledTimes(1); + validatedAppId = await appIdHelper.lookupAppId(clientIdentity); + expect(validatedAppId).toBe(standardApp); + expect(getAppSpy).toHaveBeenCalledTimes(1); + }); + + it("An invalid appId should not query the app directory more than once if queried more than once.", async () => { + const getAppSpy = jest.fn(getApp); + const appIdHelper = new AppIdHelper(getAppSpy, platformId, logger, unregisteredApp); + const clientIdentity = { + name: unknownAppId, + uuid: platformId, + endpointId: "x", + isLocalEndpointId: true + }; + let validatedAppId = await appIdHelper.lookupAppId(clientIdentity); + expect(validatedAppId).toBeUndefined(); + expect(getAppSpy).toHaveBeenCalledTimes(1); + validatedAppId = await appIdHelper.lookupAppId(clientIdentity); + expect(validatedAppId).toBeUndefined(); + expect(getAppSpy).toHaveBeenCalledTimes(1); + }); + + it("A warning should be logged if an invalid appId is passed.", async () => { + const loggerSpy = jest.spyOn(logger, "warn"); + const appIdHelper = new AppIdHelper(getApp, platformId, logger, unregisteredApp); + const clientIdentity = { + name: unknownAppId, + uuid: platformId, + endpointId: "x", + isLocalEndpointId: true + }; + const validatedAppId = await appIdHelper.lookupAppId(clientIdentity); + expect(validatedAppId).toBeUndefined(); + expect(loggerSpy).toHaveBeenCalled(); + }); + + it("An invalid appId will only log one warning regardless of how many times it is checked.", async () => { + const loggerSpy = jest.spyOn(logger, "warn"); + loggerSpy.mockClear(); + const appIdHelper = new AppIdHelper(getApp, platformId, logger, unregisteredApp); + const clientIdentity = { + name: unknownAppId, + uuid: platformId, + endpointId: "x", + isLocalEndpointId: true + }; + let validatedAppId = await appIdHelper.lookupAppId(clientIdentity); + expect(validatedAppId).toBeUndefined(); + expect(loggerSpy).toHaveBeenCalled(); + validatedAppId = await appIdHelper.lookupAppId(clientIdentity); + expect(validatedAppId).toBeUndefined(); + expect(loggerSpy).toHaveBeenCalledTimes(1); + }); + }); +});