From b2e918fa0bf6e8536f2bd216641426e4527dd9ba Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Mon, 17 Jan 2022 17:46:07 +0000 Subject: [PATCH 01/22] search-common: add optional resourceRef field to IndexableDocument type Signed-off-by: MT Lewis --- .changeset/nasty-moose-trade.md | 5 +++++ packages/search-common/api-report.md | 3 +++ packages/search-common/src/types.ts | 11 +++++++++++ 3 files changed, 19 insertions(+) create mode 100644 .changeset/nasty-moose-trade.md diff --git a/.changeset/nasty-moose-trade.md b/.changeset/nasty-moose-trade.md new file mode 100644 index 0000000000..bf101f447c --- /dev/null +++ b/.changeset/nasty-moose-trade.md @@ -0,0 +1,5 @@ +--- +'@backstage/search-common': patch +--- + +Add optional resourceRef field to the IndexableDocument type for use when authorizing access to documents. diff --git a/packages/search-common/api-report.md b/packages/search-common/api-report.md index c4950e3556..dcbbbee1bf 100644 --- a/packages/search-common/api-report.md +++ b/packages/search-common/api-report.md @@ -27,6 +27,9 @@ export interface DocumentDecorator { // // @public export interface IndexableDocument { + authorization?: { + resourceRef: string; + }; location: string; text: string; title: string; diff --git a/packages/search-common/src/types.ts b/packages/search-common/src/types.ts index f73665de5b..bac6967d19 100644 --- a/packages/search-common/src/types.ts +++ b/packages/search-common/src/types.ts @@ -53,6 +53,17 @@ export interface IndexableDocument { * is clicked). */ location: string; + + /** + * Optional authorization information to be used when determining whether this + * search result should be visible to a given user. + */ + authorization?: { + /** + * Identifier for the resource. + */ + resourceRef: string; + }; } /** From 96cbebc62956bf341eb809aff977642a749fd4c5 Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Mon, 17 Jan 2022 17:48:07 +0000 Subject: [PATCH 02/22] search-common: add optional request options with token to SearchEngine#query Signed-off-by: MT Lewis --- .changeset/nervous-walls-exercise.md | 5 +++++ packages/search-common/api-report.md | 12 +++++++++++- packages/search-common/src/types.ts | 9 ++++++++- 3 files changed, 24 insertions(+), 2 deletions(-) create mode 100644 .changeset/nervous-walls-exercise.md diff --git a/.changeset/nervous-walls-exercise.md b/.changeset/nervous-walls-exercise.md new file mode 100644 index 0000000000..58c19d53ca --- /dev/null +++ b/.changeset/nervous-walls-exercise.md @@ -0,0 +1,5 @@ +--- +'@backstage/search-common': patch +--- + +Add optional query request options containing authorization token to SearchEngine#query. diff --git a/packages/search-common/api-report.md b/packages/search-common/api-report.md index dcbbbee1bf..e63caf016f 100644 --- a/packages/search-common/api-report.md +++ b/packages/search-common/api-report.md @@ -35,6 +35,13 @@ export interface IndexableDocument { title: string; } +// Warning: (ae-missing-release-tag) "QueryRequestOptions" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) +// +// @public (undocumented) +export type QueryRequestOptions = { + token?: string; +}; + // Warning: (ae-missing-release-tag) "QueryTranslator" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) // // @public @@ -45,7 +52,10 @@ export type QueryTranslator = (query: SearchQuery) => unknown; // @public export interface SearchEngine { index(type: string, documents: IndexableDocument[]): Promise; - query(query: SearchQuery): Promise; + query( + query: SearchQuery, + options?: QueryRequestOptions, + ): Promise; setTranslator(translator: QueryTranslator): void; } diff --git a/packages/search-common/src/types.ts b/packages/search-common/src/types.ts index bac6967d19..e8d21535bb 100644 --- a/packages/search-common/src/types.ts +++ b/packages/search-common/src/types.ts @@ -99,6 +99,10 @@ export interface DocumentDecorator { */ export type QueryTranslator = (query: SearchQuery) => unknown; +export type QueryRequestOptions = { + token?: string; +}; + /** * Interface that must be implemented by specific search engines, responsible * for performing indexing and querying and translating abstract queries into @@ -118,5 +122,8 @@ export interface SearchEngine { /** * Perform a search query against the SearchEngine. */ - query(query: SearchQuery): Promise; + query( + query: SearchQuery, + options?: QueryRequestOptions, + ): Promise; } From 9a511968b1521fae919e2e05a2495ddfe7818be0 Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Mon, 17 Jan 2022 17:57:09 +0000 Subject: [PATCH 03/22] search-common: add optional visibilityPermission property to DocumentCollator type Signed-off-by: MT Lewis --- .changeset/nasty-dodos-complain.md | 5 +++++ packages/search-common/api-report.md | 2 ++ packages/search-common/package.json | 3 ++- packages/search-common/src/types.ts | 9 +++++++++ 4 files changed, 18 insertions(+), 1 deletion(-) create mode 100644 .changeset/nasty-dodos-complain.md diff --git a/.changeset/nasty-dodos-complain.md b/.changeset/nasty-dodos-complain.md new file mode 100644 index 0000000000..24e53f08d8 --- /dev/null +++ b/.changeset/nasty-dodos-complain.md @@ -0,0 +1,5 @@ +--- +'@backstage/search-common': patch +--- + +Add optional visibilityPermission property to DocumentCollator type diff --git a/packages/search-common/api-report.md b/packages/search-common/api-report.md index e63caf016f..ec5a671d33 100644 --- a/packages/search-common/api-report.md +++ b/packages/search-common/api-report.md @@ -4,6 +4,7 @@ ```ts import { JsonObject } from '@backstage/types'; +import { Permission } from '@backstage/plugin-permission-common'; // Warning: (ae-missing-release-tag) "DocumentCollator" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) // @@ -12,6 +13,7 @@ export interface DocumentCollator { // (undocumented) execute(): Promise; readonly type: string; + readonly visibilityPermission?: Permission; } // Warning: (ae-missing-release-tag) "DocumentDecorator" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) diff --git a/packages/search-common/package.json b/packages/search-common/package.json index 9dabf6dd1a..ba587d9904 100644 --- a/packages/search-common/package.json +++ b/packages/search-common/package.json @@ -36,7 +36,8 @@ "url": "https://github.com/backstage/backstage/issues" }, "dependencies": { - "@backstage/types": "^0.1.1" + "@backstage/types": "^0.1.1", + "@backstage/plugin-permission-common": "^0.4.0-next.0" }, "devDependencies": { "@backstage/cli": "^0.12.0-next.0" diff --git a/packages/search-common/src/types.ts b/packages/search-common/src/types.ts index e8d21535bb..8dd4c58b0e 100644 --- a/packages/search-common/src/types.ts +++ b/packages/search-common/src/types.ts @@ -13,6 +13,8 @@ * See the License for the specific language governing permissions and * limitations under the License. */ + +import { Permission } from '@backstage/plugin-permission-common'; import { JsonObject } from '@backstage/types'; export interface SearchQuery { @@ -76,6 +78,13 @@ export interface DocumentCollator { * index name by Search Engines. */ readonly type: string; + + /** + * The {@link @backstage/plugin-permission-common#Permission} that controls + * visibility of resources associated with this collator's documents. + */ + readonly visibilityPermission?: Permission; + execute(): Promise; } From f6389e9e5d75dc30b5802d21828606aecf600fff Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Mon, 17 Jan 2022 17:59:33 +0000 Subject: [PATCH 04/22] search-backend-node: track document visibility permission by type in IndexBuilder Tracking permissions by collator ensures that all the documents of a given type are authorized using the same permission. This is not critical for result-by-result authorization, but will likely be a useful constraint when adding in-engine permission filtering to specific search engines in the future. Signed-off-by: MT Lewis --- .changeset/angry-carrots-brush.md | 5 +++++ plugins/search-backend-node/api-report.md | 10 ++++++++++ plugins/search-backend-node/package.json | 1 + .../search-backend-node/src/IndexBuilder.ts | 18 ++++++++++++++++++ plugins/search-backend-node/src/index.ts | 1 + 5 files changed, 35 insertions(+) create mode 100644 .changeset/angry-carrots-brush.md diff --git a/.changeset/angry-carrots-brush.md b/.changeset/angry-carrots-brush.md new file mode 100644 index 0000000000..bb92b0214f --- /dev/null +++ b/.changeset/angry-carrots-brush.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-search-backend-node': patch +--- + +Track visibility permissions by document type in IndexBuilder diff --git a/plugins/search-backend-node/api-report.md b/plugins/search-backend-node/api-report.md index 17887b3fd1..41ef31645e 100644 --- a/plugins/search-backend-node/api-report.md +++ b/plugins/search-backend-node/api-report.md @@ -8,11 +8,19 @@ import { DocumentDecorator } from '@backstage/search-common'; import { IndexableDocument } from '@backstage/search-common'; import { Logger as Logger_2 } from 'winston'; import { default as lunr_2 } from 'lunr'; +import { Permission } from '@backstage/plugin-permission-common'; import { QueryTranslator } from '@backstage/search-common'; import { SearchEngine } from '@backstage/search-common'; import { SearchQuery } from '@backstage/search-common'; import { SearchResultSet } from '@backstage/search-common'; +// Warning: (ae-missing-release-tag) "DocumentTypeInfo" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) +// +// @public (undocumented) +export type DocumentTypeInfo = { + visibilityPermission?: Permission; +}; + // Warning: (ae-missing-release-tag) "IndexBuilder" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) // // @public (undocumented) @@ -30,6 +38,8 @@ export class IndexBuilder { scheduler: Scheduler; }>; // (undocumented) + getDocumentTypes(): Record; + // (undocumented) getSearchEngine(): SearchEngine; } diff --git a/plugins/search-backend-node/package.json b/plugins/search-backend-node/package.json index 09de7f16ee..d6ff59773e 100644 --- a/plugins/search-backend-node/package.json +++ b/plugins/search-backend-node/package.json @@ -20,6 +20,7 @@ "clean": "backstage-cli clean" }, "dependencies": { + "@backstage/plugin-permission-common": "^0.4.0-next.0", "@backstage/search-common": "^0.2.1", "winston": "^3.2.1", "lunr": "^2.3.9", diff --git a/plugins/search-backend-node/src/IndexBuilder.ts b/plugins/search-backend-node/src/IndexBuilder.ts index 5268caf58b..5029e644f5 100644 --- a/plugins/search-backend-node/src/IndexBuilder.ts +++ b/plugins/search-backend-node/src/IndexBuilder.ts @@ -14,6 +14,7 @@ * limitations under the License. */ +import { Permission } from '@backstage/plugin-permission-common'; import { DocumentCollator, DocumentDecorator, @@ -37,15 +38,25 @@ type IndexBuilderOptions = { logger: Logger; }; +export type DocumentTypeInfo = { + /** + * The {@link @backstage/plugin-permission-common#Permission} that controls + * visibility of resources associated with this collator's documents. + */ + visibilityPermission?: Permission; +}; + export class IndexBuilder { private collators: Record; private decorators: Record; + private documentTypes: Record; private searchEngine: SearchEngine; private logger: Logger; constructor({ logger, searchEngine }: IndexBuilderOptions) { this.collators = {}; this.decorators = {}; + this.documentTypes = {}; this.logger = logger; this.searchEngine = searchEngine; } @@ -54,6 +65,10 @@ export class IndexBuilder { return this.searchEngine; } + getDocumentTypes(): Record { + return this.documentTypes; + } + /** * Makes the index builder aware of a collator that should be executed at the * given refresh interval. @@ -69,6 +84,9 @@ export class IndexBuilder { refreshInterval: defaultRefreshIntervalSeconds, collate: collator, }; + this.documentTypes[collator.type] = { + visibilityPermission: collator.visibilityPermission, + }; } /** diff --git a/plugins/search-backend-node/src/index.ts b/plugins/search-backend-node/src/index.ts index d49ef195f2..006d9c6edc 100644 --- a/plugins/search-backend-node/src/index.ts +++ b/plugins/search-backend-node/src/index.ts @@ -21,6 +21,7 @@ */ export { IndexBuilder } from './IndexBuilder'; +export type { DocumentTypeInfo } from './IndexBuilder'; export { Scheduler } from './Scheduler'; export { LunrSearchEngine } from './engines'; From 4f5bde47e9a6b011b8b73bf701cd0f19a927782c Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Mon, 17 Jan 2022 18:03:42 +0000 Subject: [PATCH 05/22] catalog-backend: support authorization in DefaultCatalogCollator Signed-off-by: MT Lewis --- .changeset/angry-jeans-bow.md | 5 +++++ plugins/catalog-backend/api-report.md | 3 +++ .../src/search/DefaultCatalogCollator.test.ts | 6 ++++++ .../src/search/DefaultCatalogCollator.ts | 11 ++++++++++- 4 files changed, 24 insertions(+), 1 deletion(-) create mode 100644 .changeset/angry-jeans-bow.md diff --git a/.changeset/angry-jeans-bow.md b/.changeset/angry-jeans-bow.md new file mode 100644 index 0000000000..6f28797c42 --- /dev/null +++ b/.changeset/angry-jeans-bow.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-catalog-backend': patch +--- + +Add support for permissions to the DefaultCatalogCollator by adding the visibilityPermission prop and including authorization.resourceRef on indexed documents. diff --git a/plugins/catalog-backend/api-report.md b/plugins/catalog-backend/api-report.md index 6cd386c122..deed3e2d8b 100644 --- a/plugins/catalog-backend/api-report.md +++ b/plugins/catalog-backend/api-report.md @@ -26,6 +26,7 @@ import { Location as Location_2 } from '@backstage/catalog-model'; import { LocationSpec } from '@backstage/catalog-model'; import { Logger as Logger_2 } from 'winston'; import { Organizations } from 'aws-sdk'; +import { Permission } from '@backstage/plugin-permission-common'; import { PermissionAuthorizer } from '@backstage/plugin-permission-common'; import { PermissionCondition } from '@backstage/plugin-permission-common'; import { PermissionCriteria } from '@backstage/plugin-permission-common'; @@ -539,6 +540,8 @@ export class DefaultCatalogCollator implements DocumentCollator { protected tokenManager: TokenManager; // (undocumented) readonly type: string; + // (undocumented) + readonly visibilityPermission: Permission; } // Warning: (ae-missing-release-tag) "DefaultCatalogProcessingOrchestrator" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) diff --git a/plugins/catalog-backend/src/search/DefaultCatalogCollator.test.ts b/plugins/catalog-backend/src/search/DefaultCatalogCollator.test.ts index 1360ca2647..81abb91328 100644 --- a/plugins/catalog-backend/src/search/DefaultCatalogCollator.test.ts +++ b/plugins/catalog-backend/src/search/DefaultCatalogCollator.test.ts @@ -113,6 +113,9 @@ describe('DefaultCatalogCollator', () => { componentType: expectedEntities[0]!.spec!.type, lifecycle: expectedEntities[0]!.spec!.lifecycle, owner: expectedEntities[0]!.spec!.owner, + authorization: { + resourceRef: 'component:default/test-entity', + }, }); expect(documents[1]).toMatchObject({ title: expectedEntities[1].metadata.title, @@ -122,6 +125,9 @@ describe('DefaultCatalogCollator', () => { componentType: expectedEntities[1]!.spec!.type, lifecycle: expectedEntities[1]!.spec!.lifecycle, owner: expectedEntities[1]!.spec!.owner, + authorization: { + resourceRef: 'component:default/test-entity-2', + }, }); }); diff --git a/plugins/catalog-backend/src/search/DefaultCatalogCollator.ts b/plugins/catalog-backend/src/search/DefaultCatalogCollator.ts index ebad95e72f..93ef6968d1 100644 --- a/plugins/catalog-backend/src/search/DefaultCatalogCollator.ts +++ b/plugins/catalog-backend/src/search/DefaultCatalogCollator.ts @@ -18,7 +18,11 @@ import { PluginEndpointDiscovery, TokenManager, } from '@backstage/backend-common'; -import { Entity, UserEntity } from '@backstage/catalog-model'; +import { + Entity, + stringifyEntityRef, + UserEntity, +} from '@backstage/catalog-model'; import { IndexableDocument, DocumentCollator } from '@backstage/search-common'; import { Config } from '@backstage/config'; import { @@ -26,6 +30,7 @@ import { CatalogClient, CatalogEntitiesRequest, } from '@backstage/catalog-client'; +import { catalogEntityReadPermission } from '@backstage/plugin-catalog-common'; export interface CatalogEntityDocument extends IndexableDocument { componentType: string; @@ -41,6 +46,7 @@ export class DefaultCatalogCollator implements DocumentCollator { protected filter?: CatalogEntitiesRequest['filter']; protected readonly catalogClient: CatalogApi; public readonly type: string = 'software-catalog'; + public readonly visibilityPermission = catalogEntityReadPermission; protected tokenManager: TokenManager; static fromConfig( @@ -126,6 +132,9 @@ export class DefaultCatalogCollator implements DocumentCollator { kind: entity.kind, lifecycle: (entity.spec?.lifecycle as string) || '', owner: (entity.spec?.owner as string) || '', + authorization: { + resourceRef: stringifyEntityRef(entity), + }, }; }); } From 46823404570d5924cb878c6f06269f22e377a1ee Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Mon, 17 Jan 2022 18:10:56 +0000 Subject: [PATCH 06/22] techdocs-backend: support authorization in DefaultTechDocsCollator Signed-off-by: MT Lewis --- .changeset/small-avocados-decide.md | 5 +++++ plugins/techdocs-backend/api-report.md | 3 +++ plugins/techdocs-backend/package.json | 1 + .../src/search/DefaultTechDocsCollator.test.ts | 3 +++ .../src/search/DefaultTechDocsCollator.ts | 11 ++++++++++- 5 files changed, 22 insertions(+), 1 deletion(-) create mode 100644 .changeset/small-avocados-decide.md diff --git a/.changeset/small-avocados-decide.md b/.changeset/small-avocados-decide.md new file mode 100644 index 0000000000..dc5cea1184 --- /dev/null +++ b/.changeset/small-avocados-decide.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-techdocs-backend': patch +--- + +Add support for permissions to the DefaultTechDocsCollator by adding the visibilityPermission prop and including authorization.resourceRef on indexed documents. diff --git a/plugins/techdocs-backend/api-report.md b/plugins/techdocs-backend/api-report.md index 76183d4c6c..cefb7d937b 100644 --- a/plugins/techdocs-backend/api-report.md +++ b/plugins/techdocs-backend/api-report.md @@ -10,6 +10,7 @@ import express from 'express'; import { GeneratorBuilder } from '@backstage/techdocs-common'; import { Knex } from 'knex'; import { Logger as Logger_2 } from 'winston'; +import { Permission } from '@backstage/plugin-permission-common'; import { PluginCacheManager } from '@backstage/backend-common'; import { PluginEndpointDiscovery } from '@backstage/backend-common'; import { PreparerBuilder } from '@backstage/techdocs-common'; @@ -46,6 +47,8 @@ export class DefaultTechDocsCollator implements DocumentCollator { protected locationTemplate: string; // (undocumented) readonly type: string; + // (undocumented) + readonly visibilityPermission: Permission; } // Warning: (ae-missing-release-tag) "OutOfTheBoxDeploymentOptions" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) diff --git a/plugins/techdocs-backend/package.json b/plugins/techdocs-backend/package.json index 882c8092de..7692eb6825 100644 --- a/plugins/techdocs-backend/package.json +++ b/plugins/techdocs-backend/package.json @@ -37,6 +37,7 @@ "@backstage/config": "^0.1.13", "@backstage/errors": "^0.2.0", "@backstage/integration": "^0.7.2", + "@backstage/plugin-catalog-common": "^0.1.1-next.0", "@backstage/search-common": "^0.2.1", "@backstage/techdocs-common": "^0.11.4", "@types/express": "^4.17.6", diff --git a/plugins/techdocs-backend/src/search/DefaultTechDocsCollator.test.ts b/plugins/techdocs-backend/src/search/DefaultTechDocsCollator.test.ts index 5f0bed55dd..7e7545ea1f 100644 --- a/plugins/techdocs-backend/src/search/DefaultTechDocsCollator.test.ts +++ b/plugins/techdocs-backend/src/search/DefaultTechDocsCollator.test.ts @@ -200,6 +200,9 @@ describe('DefaultTechDocsCollator', () => { owner: '', kind: entity.kind, name: entity.metadata.name, + authorization: { + resourceRef: `component:default/${entity.metadata.name}`, + }, }); }); }); diff --git a/plugins/techdocs-backend/src/search/DefaultTechDocsCollator.ts b/plugins/techdocs-backend/src/search/DefaultTechDocsCollator.ts index 1d3a3dbc3a..5149fe6c09 100644 --- a/plugins/techdocs-backend/src/search/DefaultTechDocsCollator.ts +++ b/plugins/techdocs-backend/src/search/DefaultTechDocsCollator.ts @@ -18,13 +18,18 @@ import { PluginEndpointDiscovery, TokenManager, } from '@backstage/backend-common'; -import { Entity, RELATION_OWNED_BY } from '@backstage/catalog-model'; +import { + Entity, + RELATION_OWNED_BY, + stringifyEntityRef, +} from '@backstage/catalog-model'; import { DocumentCollator } from '@backstage/search-common'; import fetch from 'node-fetch'; import unescape from 'lodash/unescape'; import { Logger } from 'winston'; import pLimit from 'p-limit'; import { Config } from '@backstage/config'; +import { catalogEntityReadPermission } from '@backstage/plugin-catalog-common'; import { CatalogApi, CatalogClient } from '@backstage/catalog-client'; import { TechDocsDocument } from '@backstage/techdocs-common'; @@ -59,6 +64,7 @@ export class DefaultTechDocsCollator implements DocumentCollator { private readonly parallelismLimit: number; private readonly legacyPathCasing: boolean; public readonly type: string = 'techdocs'; + public readonly visibilityPermission = catalogEntityReadPermission; /** * @deprecated use static fromConfig method instead. @@ -148,6 +154,9 @@ export class DefaultTechDocsCollator implements DocumentCollator { owner: entity.relations?.find(r => r.type === RELATION_OWNED_BY) ?.target?.name || '', + authorization: { + resourceRef: stringifyEntityRef(entity), + }, })); } catch (e) { this.logger.debug( From 7da1b7c42af75757ebe7d9c3d9096c6b70284700 Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Mon, 17 Jan 2022 18:22:34 +0000 Subject: [PATCH 07/22] search-backend: add optional latency budget for authorized search backend Signed-off-by: MT Lewis --- plugins/search-backend/config.d.ts | 39 ++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) create mode 100644 plugins/search-backend/config.d.ts diff --git a/plugins/search-backend/config.d.ts b/plugins/search-backend/config.d.ts new file mode 100644 index 0000000000..99f4d0ab2f --- /dev/null +++ b/plugins/search-backend/config.d.ts @@ -0,0 +1,39 @@ +/* + * Copyright 2021 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +export interface Config { + /** Configuration options for the search plugin */ + search?: { + /** + * Options related to the search integration with the Backstage permissions system + */ + permissions?: { + /** + * Limits the amount of time the search backend will spend retrieving and + * authorizing results from the search engine when permissions are + * enabled. When the latency of the query endpoint reaches this threshold, + * the results obtained up until that point will be returned, even if the + * page is incomplete. + * + * This limit is only expected to be hit for broad queries from users with + * extremely restrictive visibility, or for very high page offsets. + * + * @default 1000 + */ + queryLatencyBudgetMs?: number; + }; + }; +} From bbfbc755aa7dc3305ad816decf082547e968cf53 Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Mon, 17 Jan 2022 18:26:25 +0000 Subject: [PATCH 08/22] [wip] search-backend: result-by-result authorization To do: - add tests for AuthorizedSearchEngine - update router tests - add diff to changeset Signed-off-by: MT Lewis --- .changeset/pink-spoons-hope.md | 5 + packages/backend/src/plugins/search.ts | 4 + .../packages/backend/src/plugins/search.ts | 4 + plugins/search-backend/api-report.md | 6 + plugins/search-backend/package.json | 5 + .../src/service/AuthorizedSearchEngine.ts | 171 ++++++++++++++++++ .../search-backend/src/service/router.test.ts | 18 ++ plugins/search-backend/src/service/router.ts | 26 ++- .../src/service/standaloneServer.ts | 20 +- 9 files changed, 254 insertions(+), 5 deletions(-) create mode 100644 .changeset/pink-spoons-hope.md create mode 100644 plugins/search-backend/src/service/AuthorizedSearchEngine.ts diff --git a/.changeset/pink-spoons-hope.md b/.changeset/pink-spoons-hope.md new file mode 100644 index 0000000000..cc8fba42de --- /dev/null +++ b/.changeset/pink-spoons-hope.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-search-backend': patch +--- + +Add support for filtering search results according to visibility. diff --git a/packages/backend/src/plugins/search.ts b/packages/backend/src/plugins/search.ts index 9a8db0f0f9..4b5363e73d 100644 --- a/packages/backend/src/plugins/search.ts +++ b/packages/backend/src/plugins/search.ts @@ -56,6 +56,7 @@ async function createSearchEngine({ export default async function createPlugin({ logger, + permissions, discovery, config, database, @@ -95,6 +96,9 @@ export default async function createPlugin({ return await createRouter({ engine: indexBuilder.getSearchEngine(), + types: indexBuilder.getDocumentTypes(), + permissions, + config, logger, }); } diff --git a/packages/create-app/templates/default-app/packages/backend/src/plugins/search.ts b/packages/create-app/templates/default-app/packages/backend/src/plugins/search.ts index f23b0c7bcf..a0a1cc3701 100644 --- a/packages/create-app/templates/default-app/packages/backend/src/plugins/search.ts +++ b/packages/create-app/templates/default-app/packages/backend/src/plugins/search.ts @@ -10,6 +10,7 @@ import { DefaultTechDocsCollator } from '@backstage/plugin-techdocs-backend'; export default async function createPlugin({ logger, + permissions, discovery, config, tokenManager, @@ -49,6 +50,9 @@ export default async function createPlugin({ return await createRouter({ engine: indexBuilder.getSearchEngine(), + types: indexBuilder.getDocumentTypes(), + permissions, + config, logger, }); } diff --git a/plugins/search-backend/api-report.md b/plugins/search-backend/api-report.md index cac97bd725..125bd6cc1d 100644 --- a/plugins/search-backend/api-report.md +++ b/plugins/search-backend/api-report.md @@ -3,8 +3,11 @@ > Do not edit this file. It is a report generated by [API Extractor](https://api-extractor.com/). ```ts +import { Config } from '@backstage/config'; +import { DocumentTypeInfo } from '@backstage/plugin-search-backend-node'; import express from 'express'; import { Logger as Logger_2 } from 'winston'; +import { PermissionAuthorizer } from '@backstage/plugin-permission-common'; import { SearchEngine } from '@backstage/plugin-search-backend-node'; // Warning: (ae-missing-release-tag) "createRouter" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) @@ -17,6 +20,9 @@ export function createRouter(options: RouterOptions): Promise; // @public (undocumented) export type RouterOptions = { engine: SearchEngine; + types: Record; + permissions: PermissionAuthorizer; + config: Config; logger: Logger_2; }; ``` diff --git a/plugins/search-backend/package.json b/plugins/search-backend/package.json index 19b42e62af..4ff4e98e52 100644 --- a/plugins/search-backend/package.json +++ b/plugins/search-backend/package.json @@ -24,11 +24,16 @@ "@backstage/config": "^0.1.13", "@backstage/errors": "^0.2.0", "@backstage/search-common": "^0.2.0", + "@backstage/plugin-auth-backend": "^0.7.0-next.0", + "@backstage/plugin-permission-common": "^0.4.0-next.0", + "@backstage/plugin-permission-node": "^0.4.0-next.0", "@backstage/plugin-search-backend-node": "^0.4.4", "@backstage/types": "^0.1.1", "@types/express": "^4.17.6", + "dataloader": "^2.0.0", "express": "^4.17.1", "express-promise-router": "^4.1.0", + "lodash": "^4.17.21", "winston": "^3.2.1", "yn": "^4.0.0", "zod": "^3.11.6" diff --git a/plugins/search-backend/src/service/AuthorizedSearchEngine.ts b/plugins/search-backend/src/service/AuthorizedSearchEngine.ts new file mode 100644 index 0000000000..6f33307f6f --- /dev/null +++ b/plugins/search-backend/src/service/AuthorizedSearchEngine.ts @@ -0,0 +1,171 @@ +/* + * Copyright 2022 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { compact, zipObject } from 'lodash'; +import DataLoader from 'dataloader'; +import { + AuthorizeDecision, + AuthorizeQuery, + AuthorizeResult, + PermissionAuthorizer, +} from '@backstage/plugin-permission-common'; +import { + IndexableDocument, + QueryRequestOptions, + QueryTranslator, + SearchEngine, + SearchQuery, + SearchResult, + SearchResultSet, +} from '@backstage/search-common'; +import { DocumentTypeInfo } from '@backstage/plugin-search-backend-node'; +import { Config } from '@backstage/config'; + +export function decodePageCursor(pageCursor?: string): { page: number } { + if (!pageCursor) { + return { page: 0 }; + } + + return { + page: Number(Buffer.from(pageCursor, 'base64').toString('utf-8')), + }; +} + +export function encodePageCursor({ page }: { page: number }): string { + return Buffer.from(`${page}`, 'utf-8').toString('base64'); +} + +export class AuthorizedSearchEngine implements SearchEngine { + private readonly pageSize: number = 25; + private readonly queryLatencyBudgetMs: number; + + constructor( + private readonly searchEngine: SearchEngine, + private readonly types: Record, + private readonly permissions: PermissionAuthorizer, + config: Config, + ) { + this.queryLatencyBudgetMs = + config.getOptionalNumber('search.permissions.queryLatencyBudgetMs') ?? + 1000; + } + + setTranslator(translator: QueryTranslator): void { + this.searchEngine.setTranslator(translator); + } + + async index(type: string, documents: IndexableDocument[]): Promise { + this.searchEngine.index(type, documents); + } + + async query( + query: SearchQuery, + options: QueryRequestOptions, + ): Promise { + const queryStartTime = Date.now(); + + const authorizer = new DataLoader((requests: readonly AuthorizeQuery[]) => + this.permissions.authorize(requests.slice(), options), + ); + + const requestedTypes = query.types || Object.keys(this.types); + const typeDecisions = zipObject( + requestedTypes, + await Promise.all( + requestedTypes.map(type => { + const permission = this.types[type].visibilityPermission; + + return permission + ? authorizer.load({ permission }) + : { result: AuthorizeResult.ALLOW as const }; + }), + ), + ); + + const authorizedTypes = requestedTypes.filter( + type => typeDecisions[type]?.result !== AuthorizeResult.DENY, + ); + + const { page } = decodePageCursor(query.pageCursor); + const targetResults = (page + 1) * this.pageSize; + + let filteredResults: SearchResult[] = []; + let nextPageCursor: string | undefined; + let latencyBudgetExhausted: boolean; + + do { + const nextPage = await this.searchEngine.query( + { ...query, types: authorizedTypes, pageCursor: nextPageCursor }, + options, + ); + + filteredResults = filteredResults.concat( + await this.filterResults(nextPage.results, typeDecisions, authorizer), + ); + + nextPageCursor = nextPage.nextPageCursor; + latencyBudgetExhausted = + Date.now() - queryStartTime > this.queryLatencyBudgetMs; + } while ( + nextPageCursor && + filteredResults.length < targetResults && + !latencyBudgetExhausted + ); + + return { + results: filteredResults.slice( + page * this.pageSize, + (page + 1) * this.pageSize, + ), + previousPageCursor: + page === 0 ? undefined : encodePageCursor({ page: page - 1 }), + nextPageCursor: + !latencyBudgetExhausted && + (nextPageCursor || filteredResults.length > targetResults) + ? encodePageCursor({ page: page + 1 }) + : undefined, + }; + } + + private async filterResults( + results: SearchResult[], + typeDecisions: Record, + authorizer: DataLoader, + ) { + return compact( + await Promise.all( + results.map(result => { + if (typeDecisions[result.type]?.result === AuthorizeResult.ALLOW) { + return result; + } + + const permission = this.types[result.type].visibilityPermission; + const resourceRef = result.document.authorization?.resourceRef; + + if (!permission || !resourceRef) { + return result; + } + + return authorizer + .load({ permission, resourceRef }) + .then(decision => + decision.result === AuthorizeResult.ALLOW ? result : undefined, + ); + }), + ), + ); + } +} diff --git a/plugins/search-backend/src/service/router.test.ts b/plugins/search-backend/src/service/router.test.ts index 407e118a8c..0a94250aeb 100644 --- a/plugins/search-backend/src/service/router.test.ts +++ b/plugins/search-backend/src/service/router.test.ts @@ -15,6 +15,8 @@ */ import { getVoidLogger } from '@backstage/backend-common'; +import { ConfigReader } from '@backstage/config'; +import { PermissionAuthorizer } from '@backstage/plugin-permission-common'; import { IndexBuilder, LunrSearchEngine, @@ -25,6 +27,12 @@ import request from 'supertest'; import { createRouter } from './router'; +const mockPermissionAuthorizer: PermissionAuthorizer = { + authorize: () => { + throw new Error('Not implemented'); + }, +}; + describe('createRouter', () => { let app: express.Express; let mockSearchEngine: jest.Mocked; @@ -36,6 +44,12 @@ describe('createRouter', () => { const router = await createRouter({ engine: indexBuilder.getSearchEngine(), + types: { + 'first-type': {}, + 'second-type': {}, + }, + config: new ConfigReader({ permissions: { enabled: false } }), + permissions: mockPermissionAuthorizer, logger, }); app = express().use(router); @@ -74,6 +88,7 @@ describe('createRouter', () => { 'term[0]=foo', 'term[prop]=value', 'types=foo', + 'types[0]=unknown-type', 'types[length]=10000&types[0]=first-type', 'filters=stringValue', 'pageCursor[0]=1', @@ -101,6 +116,9 @@ describe('createRouter', () => { const router = await createRouter({ engine: indexBuilder.getSearchEngine(), + types: indexBuilder.getDocumentTypes(), + config: new ConfigReader({ permissions: { enabled: false } }), + permissions: mockPermissionAuthorizer, logger, }); app = express().use(router); diff --git a/plugins/search-backend/src/service/router.ts b/plugins/search-backend/src/service/router.ts index 34289d6343..f8cf2ae95b 100644 --- a/plugins/search-backend/src/service/router.ts +++ b/plugins/search-backend/src/service/router.ts @@ -20,9 +20,16 @@ import { Logger } from 'winston'; import { z } from 'zod'; import { errorHandler } from '@backstage/backend-common'; import { InputError } from '@backstage/errors'; +import { Config } from '@backstage/config'; import { JsonObject, JsonValue } from '@backstage/types'; +import { IdentityClient } from '@backstage/plugin-auth-backend'; +import { PermissionAuthorizer } from '@backstage/plugin-permission-common'; import { SearchResultSet } from '@backstage/search-common'; -import { SearchEngine } from '@backstage/plugin-search-backend-node'; +import { + DocumentTypeInfo, + SearchEngine, +} from '@backstage/plugin-search-backend-node'; +import { AuthorizedSearchEngine } from './AuthorizedSearchEngine'; const jsonObjectSchema: z.ZodSchema = z.lazy(() => { const jsonValueSchema: z.ZodSchema = z.lazy(() => @@ -41,6 +48,9 @@ const jsonObjectSchema: z.ZodSchema = z.lazy(() => { export type RouterOptions = { engine: SearchEngine; + types: Record; + permissions: PermissionAuthorizer; + config: Config; logger: Logger; }; @@ -49,15 +59,21 @@ const allowedLocationProtocols = ['http:', 'https:']; export async function createRouter( options: RouterOptions, ): Promise { - const { engine, logger } = options; + const { engine: inputEngine, types, permissions, config, logger } = options; const requestSchema = z.object({ term: z.string().default(''), filters: jsonObjectSchema.optional(), - types: z.array(z.string()).optional(), + types: z + .array(z.string().refine(type => Object.keys(types).includes(type))) + .optional(), pageCursor: z.string().optional(), }); + const engine = config.getOptionalBoolean('permission.enabled') + ? new AuthorizedSearchEngine(inputEngine, types, permissions, config) + : inputEngine; + const filterResultSet = ({ results, ...resultSet }: SearchResultSet) => ({ ...resultSet, results: results.filter(result => { @@ -93,8 +109,10 @@ export async function createRouter( }, pageCursor=${query.pageCursor ?? ''}`, ); + const token = IdentityClient.getBearerToken(req.header('authorization')); + try { - const resultSet = await engine?.query(query); + const resultSet = await engine?.query(query, { token }); res.send(filterResultSet(resultSet)); } catch (err) { diff --git a/plugins/search-backend/src/service/standaloneServer.ts b/plugins/search-backend/src/service/standaloneServer.ts index 9ba9dbd5f7..3f1fdcb34e 100644 --- a/plugins/search-backend/src/service/standaloneServer.ts +++ b/plugins/search-backend/src/service/standaloneServer.ts @@ -14,7 +14,12 @@ * limitations under the License. */ -import { createServiceBuilder } from '@backstage/backend-common'; +import { + createServiceBuilder, + loadBackendConfig, + ServerTokenManager, + SingleHostDiscovery, +} from '@backstage/backend-common'; import { Server } from 'http'; import { Logger } from 'winston'; import { createRouter } from './router'; @@ -22,6 +27,7 @@ import { LunrSearchEngine, IndexBuilder, } from '@backstage/plugin-search-backend-node'; +import { ServerPermissionClient } from '@backstage/plugin-permission-node'; export interface ServerOptions { port: number; @@ -33,14 +39,26 @@ export async function startStandaloneServer( options: ServerOptions, ): Promise { const logger = options.logger.child({ service: 'search-backend' }); + const config = await loadBackendConfig({ logger, argv: process.argv }); const searchEngine = new LunrSearchEngine({ logger }); const indexBuilder = new IndexBuilder({ logger, searchEngine }); + const discovery = SingleHostDiscovery.fromConfig(config); + const tokenManager = ServerTokenManager.fromConfig(config, { + logger, + }); + const permissions = ServerPermissionClient.fromConfig(config, { + discovery, + tokenManager, + }); logger.debug('Starting application server...'); // TODO: stub out some documents/indices? const router = await createRouter({ engine: indexBuilder.getSearchEngine(), + types: indexBuilder.getDocumentTypes(), + permissions, + config, logger, }); From 1887a857c41f1d83296d9112ce3f72b5ff2225df Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Wed, 19 Jan 2022 13:33:53 +0000 Subject: [PATCH 09/22] reduce detail in changesets Co-authored-by: Eric Peterson Signed-off-by: MT Lewis --- .changeset/angry-jeans-bow.md | 2 +- .changeset/small-avocados-decide.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.changeset/angry-jeans-bow.md b/.changeset/angry-jeans-bow.md index 6f28797c42..346cd1c557 100644 --- a/.changeset/angry-jeans-bow.md +++ b/.changeset/angry-jeans-bow.md @@ -2,4 +2,4 @@ '@backstage/plugin-catalog-backend': patch --- -Add support for permissions to the DefaultCatalogCollator by adding the visibilityPermission prop and including authorization.resourceRef on indexed documents. +Add support for permissions to the DefaultCatalogCollator. diff --git a/.changeset/small-avocados-decide.md b/.changeset/small-avocados-decide.md index dc5cea1184..02097aa601 100644 --- a/.changeset/small-avocados-decide.md +++ b/.changeset/small-avocados-decide.md @@ -2,4 +2,4 @@ '@backstage/plugin-techdocs-backend': patch --- -Add support for permissions to the DefaultTechDocsCollator by adding the visibilityPermission prop and including authorization.resourceRef on indexed documents. +Add support for permissions to the DefaultTechDocsCollator. From 634abc1a41a678155ee1a7039bf3a1012348b362 Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Mon, 24 Jan 2022 14:46:11 +0000 Subject: [PATCH 10/22] search: move DocumentTypeInfo to search-common Signed-off-by: MT Lewis --- .changeset/nasty-dodos-complain.md | 3 ++- packages/search-common/api-report.md | 7 +++++++ packages/search-common/src/types.ts | 13 +++++++++++++ plugins/search-backend-node/api-report.md | 9 +-------- plugins/search-backend-node/package.json | 1 - plugins/search-backend-node/src/IndexBuilder.ts | 10 +--------- plugins/search-backend-node/src/index.ts | 1 - plugins/search-backend/api-report.md | 2 +- plugins/search-backend/package.json | 2 +- .../src/service/AuthorizedSearchEngine.ts | 2 +- plugins/search-backend/src/service/router.ts | 7 ++----- 11 files changed, 29 insertions(+), 28 deletions(-) diff --git a/.changeset/nasty-dodos-complain.md b/.changeset/nasty-dodos-complain.md index 24e53f08d8..f81ddfd97a 100644 --- a/.changeset/nasty-dodos-complain.md +++ b/.changeset/nasty-dodos-complain.md @@ -2,4 +2,5 @@ '@backstage/search-common': patch --- -Add optional visibilityPermission property to DocumentCollator type +- Add optional visibilityPermission property to DocumentCollator type +- Add new DocumentTypeInfo type for housing information about the document types stored in a search engine. diff --git a/packages/search-common/api-report.md b/packages/search-common/api-report.md index ec5a671d33..65f9f25180 100644 --- a/packages/search-common/api-report.md +++ b/packages/search-common/api-report.md @@ -25,6 +25,13 @@ export interface DocumentDecorator { readonly types?: string[]; } +// Warning: (ae-missing-release-tag) "DocumentTypeInfo" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) +// +// @public +export type DocumentTypeInfo = { + visibilityPermission?: Permission; +}; + // Warning: (ae-missing-release-tag) "IndexableDocument" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) // // @public diff --git a/packages/search-common/src/types.ts b/packages/search-common/src/types.ts index 8dd4c58b0e..4e61767ea0 100644 --- a/packages/search-common/src/types.ts +++ b/packages/search-common/src/types.ts @@ -68,6 +68,19 @@ export interface IndexableDocument { }; } +/** + * Information about a specific document type. Intended to be used in the + * {@link @backstage/search-backend-node#IndexBuilder} to collect information + * about the types stored in the index. + */ +export type DocumentTypeInfo = { + /** + * The {@link @backstage/plugin-permission-common#Permission} that controls + * visibility of resources associated with this collator's documents. + */ + visibilityPermission?: Permission; +}; + /** * Interface that must be implemented in order to expose new documents to * search. diff --git a/plugins/search-backend-node/api-report.md b/plugins/search-backend-node/api-report.md index 41ef31645e..1929009760 100644 --- a/plugins/search-backend-node/api-report.md +++ b/plugins/search-backend-node/api-report.md @@ -5,22 +5,15 @@ ```ts import { DocumentCollator } from '@backstage/search-common'; import { DocumentDecorator } from '@backstage/search-common'; +import { DocumentTypeInfo } from '@backstage/search-common'; import { IndexableDocument } from '@backstage/search-common'; import { Logger as Logger_2 } from 'winston'; import { default as lunr_2 } from 'lunr'; -import { Permission } from '@backstage/plugin-permission-common'; import { QueryTranslator } from '@backstage/search-common'; import { SearchEngine } from '@backstage/search-common'; import { SearchQuery } from '@backstage/search-common'; import { SearchResultSet } from '@backstage/search-common'; -// Warning: (ae-missing-release-tag) "DocumentTypeInfo" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) -// -// @public (undocumented) -export type DocumentTypeInfo = { - visibilityPermission?: Permission; -}; - // Warning: (ae-missing-release-tag) "IndexBuilder" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) // // @public (undocumented) diff --git a/plugins/search-backend-node/package.json b/plugins/search-backend-node/package.json index d6ff59773e..09de7f16ee 100644 --- a/plugins/search-backend-node/package.json +++ b/plugins/search-backend-node/package.json @@ -20,7 +20,6 @@ "clean": "backstage-cli clean" }, "dependencies": { - "@backstage/plugin-permission-common": "^0.4.0-next.0", "@backstage/search-common": "^0.2.1", "winston": "^3.2.1", "lunr": "^2.3.9", diff --git a/plugins/search-backend-node/src/IndexBuilder.ts b/plugins/search-backend-node/src/IndexBuilder.ts index 5029e644f5..92adb03d7d 100644 --- a/plugins/search-backend-node/src/IndexBuilder.ts +++ b/plugins/search-backend-node/src/IndexBuilder.ts @@ -14,10 +14,10 @@ * limitations under the License. */ -import { Permission } from '@backstage/plugin-permission-common'; import { DocumentCollator, DocumentDecorator, + DocumentTypeInfo, IndexableDocument, SearchEngine, } from '@backstage/search-common'; @@ -38,14 +38,6 @@ type IndexBuilderOptions = { logger: Logger; }; -export type DocumentTypeInfo = { - /** - * The {@link @backstage/plugin-permission-common#Permission} that controls - * visibility of resources associated with this collator's documents. - */ - visibilityPermission?: Permission; -}; - export class IndexBuilder { private collators: Record; private decorators: Record; diff --git a/plugins/search-backend-node/src/index.ts b/plugins/search-backend-node/src/index.ts index 006d9c6edc..d49ef195f2 100644 --- a/plugins/search-backend-node/src/index.ts +++ b/plugins/search-backend-node/src/index.ts @@ -21,7 +21,6 @@ */ export { IndexBuilder } from './IndexBuilder'; -export type { DocumentTypeInfo } from './IndexBuilder'; export { Scheduler } from './Scheduler'; export { LunrSearchEngine } from './engines'; diff --git a/plugins/search-backend/api-report.md b/plugins/search-backend/api-report.md index 125bd6cc1d..796f54c09d 100644 --- a/plugins/search-backend/api-report.md +++ b/plugins/search-backend/api-report.md @@ -4,7 +4,7 @@ ```ts import { Config } from '@backstage/config'; -import { DocumentTypeInfo } from '@backstage/plugin-search-backend-node'; +import { DocumentTypeInfo } from '@backstage/search-common'; import express from 'express'; import { Logger as Logger_2 } from 'winston'; import { PermissionAuthorizer } from '@backstage/plugin-permission-common'; diff --git a/plugins/search-backend/package.json b/plugins/search-backend/package.json index 4ff4e98e52..1db6a0a910 100644 --- a/plugins/search-backend/package.json +++ b/plugins/search-backend/package.json @@ -23,7 +23,7 @@ "@backstage/backend-common": "^0.10.4", "@backstage/config": "^0.1.13", "@backstage/errors": "^0.2.0", - "@backstage/search-common": "^0.2.0", + "@backstage/search-common": "^0.2.1", "@backstage/plugin-auth-backend": "^0.7.0-next.0", "@backstage/plugin-permission-common": "^0.4.0-next.0", "@backstage/plugin-permission-node": "^0.4.0-next.0", diff --git a/plugins/search-backend/src/service/AuthorizedSearchEngine.ts b/plugins/search-backend/src/service/AuthorizedSearchEngine.ts index 6f33307f6f..7d39230bf5 100644 --- a/plugins/search-backend/src/service/AuthorizedSearchEngine.ts +++ b/plugins/search-backend/src/service/AuthorizedSearchEngine.ts @@ -23,6 +23,7 @@ import { PermissionAuthorizer, } from '@backstage/plugin-permission-common'; import { + DocumentTypeInfo, IndexableDocument, QueryRequestOptions, QueryTranslator, @@ -31,7 +32,6 @@ import { SearchResult, SearchResultSet, } from '@backstage/search-common'; -import { DocumentTypeInfo } from '@backstage/plugin-search-backend-node'; import { Config } from '@backstage/config'; export function decodePageCursor(pageCursor?: string): { page: number } { diff --git a/plugins/search-backend/src/service/router.ts b/plugins/search-backend/src/service/router.ts index f8cf2ae95b..28a3247919 100644 --- a/plugins/search-backend/src/service/router.ts +++ b/plugins/search-backend/src/service/router.ts @@ -24,11 +24,8 @@ import { Config } from '@backstage/config'; import { JsonObject, JsonValue } from '@backstage/types'; import { IdentityClient } from '@backstage/plugin-auth-backend'; import { PermissionAuthorizer } from '@backstage/plugin-permission-common'; -import { SearchResultSet } from '@backstage/search-common'; -import { - DocumentTypeInfo, - SearchEngine, -} from '@backstage/plugin-search-backend-node'; +import { DocumentTypeInfo, SearchResultSet } from '@backstage/search-common'; +import { SearchEngine } from '@backstage/plugin-search-backend-node'; import { AuthorizedSearchEngine } from './AuthorizedSearchEngine'; const jsonObjectSchema: z.ZodSchema = z.lazy(() => { From 32e46459308db9f42c84b981f1e4434a157932b9 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Tue, 25 Jan 2022 11:44:56 +0000 Subject: [PATCH 11/22] search-backend: validate page cursor when decoding Signed-off-by: MT Lewis --- .../src/service/AuthorizedSearchEngine.ts | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/plugins/search-backend/src/service/AuthorizedSearchEngine.ts b/plugins/search-backend/src/service/AuthorizedSearchEngine.ts index 7d39230bf5..c39d66a22b 100644 --- a/plugins/search-backend/src/service/AuthorizedSearchEngine.ts +++ b/plugins/search-backend/src/service/AuthorizedSearchEngine.ts @@ -33,14 +33,24 @@ import { SearchResultSet, } from '@backstage/search-common'; import { Config } from '@backstage/config'; +import { InputError } from '@backstage/errors'; export function decodePageCursor(pageCursor?: string): { page: number } { if (!pageCursor) { return { page: 0 }; } + const page = Number(Buffer.from(pageCursor, 'base64').toString('utf-8')); + if (isNaN(page)) { + throw new InputError('Invalid page cursor'); + } + + if (page < 0) { + throw new InputError('Invalid page cursor'); + } + return { - page: Number(Buffer.from(pageCursor, 'base64').toString('utf-8')), + page, }; } From bdbacb3b9c531b14d8526d76866221555e4ef5b8 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Tue, 25 Jan 2022 11:46:58 +0000 Subject: [PATCH 12/22] search-backend: handle missing type configuration in AuthorizedSearchEngine The search-backend router ensures that all requested types are configured, but since we don't know how this search engine will be used, it's good to properly handle the case where an unconfigured type is supplied. Signed-off-by: MT Lewis --- .../search-backend/src/service/AuthorizedSearchEngine.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/plugins/search-backend/src/service/AuthorizedSearchEngine.ts b/plugins/search-backend/src/service/AuthorizedSearchEngine.ts index c39d66a22b..cfc2f9b34d 100644 --- a/plugins/search-backend/src/service/AuthorizedSearchEngine.ts +++ b/plugins/search-backend/src/service/AuthorizedSearchEngine.ts @@ -90,13 +90,13 @@ export class AuthorizedSearchEngine implements SearchEngine { const authorizer = new DataLoader((requests: readonly AuthorizeQuery[]) => this.permissions.authorize(requests.slice(), options), ); - const requestedTypes = query.types || Object.keys(this.types); + const typeDecisions = zipObject( requestedTypes, await Promise.all( requestedTypes.map(type => { - const permission = this.types[type].visibilityPermission; + const permission = this.types[type]?.visibilityPermission; return permission ? authorizer.load({ permission }) @@ -162,7 +162,7 @@ export class AuthorizedSearchEngine implements SearchEngine { return result; } - const permission = this.types[result.type].visibilityPermission; + const permission = this.types[result.type]?.visibilityPermission; const resourceRef = result.document.authorization?.resourceRef; if (!permission || !resourceRef) { From 12b9fb8b34e6e688168ca81aa320c67ace056b96 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Tue, 25 Jan 2022 11:48:24 +0000 Subject: [PATCH 13/22] search-backend: initialize variable rather than declaring type Signed-off-by: MT Lewis --- plugins/search-backend/src/service/AuthorizedSearchEngine.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/search-backend/src/service/AuthorizedSearchEngine.ts b/plugins/search-backend/src/service/AuthorizedSearchEngine.ts index cfc2f9b34d..13fa86904a 100644 --- a/plugins/search-backend/src/service/AuthorizedSearchEngine.ts +++ b/plugins/search-backend/src/service/AuthorizedSearchEngine.ts @@ -114,7 +114,7 @@ export class AuthorizedSearchEngine implements SearchEngine { let filteredResults: SearchResult[] = []; let nextPageCursor: string | undefined; - let latencyBudgetExhausted: boolean; + let latencyBudgetExhausted = false; do { const nextPage = await this.searchEngine.query( From c7ce3076d9f71f8fa36a23a8623c7e5768df0610 Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Tue, 25 Jan 2022 13:01:22 +0000 Subject: [PATCH 14/22] search-backend: skip result-by-result filtering when possible Signed-off-by: MT Lewis --- .../src/service/AuthorizedSearchEngine.ts | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/plugins/search-backend/src/service/AuthorizedSearchEngine.ts b/plugins/search-backend/src/service/AuthorizedSearchEngine.ts index 13fa86904a..7b5f211d71 100644 --- a/plugins/search-backend/src/service/AuthorizedSearchEngine.ts +++ b/plugins/search-backend/src/service/AuthorizedSearchEngine.ts @@ -109,6 +109,29 @@ export class AuthorizedSearchEngine implements SearchEngine { type => typeDecisions[type]?.result !== AuthorizeResult.DENY, ); + const resultByResultFilteringRequired = authorizedTypes.some( + type => typeDecisions[type]?.result === AuthorizeResult.CONDITIONAL, + ); + + // When there are no CONDITIONAL decisions for any of the requested + // result types, we can skip filtering result by result by simply + // skipping the types the user is not permitted to see, which will + // be much more efficient. + // + // Since it's not currently possible to configure the page size used + // by search engines, this detail means that a single user might see + // a different page size depending on whether their search required + // result-by-result filtering or not. We can fix this minor + // inconsistency by introducing a configurable page size. + // + // cf. https://github.com/backstage/backstage/issues/9162 + if (!resultByResultFilteringRequired) { + return this.searchEngine.query( + { ...query, types: authorizedTypes }, + options, + ); + } + const { page } = decodePageCursor(query.pageCursor); const targetResults = (page + 1) * this.pageSize; From bd895e5812fce50b1f682bd4915601b691c18b67 Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Tue, 25 Jan 2022 13:17:08 +0000 Subject: [PATCH 15/22] search-backend: update changeset to minor and add migration steps Signed-off-by: MT Lewis --- .changeset/pink-spoons-hope.md | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/.changeset/pink-spoons-hope.md b/.changeset/pink-spoons-hope.md index cc8fba42de..c99219561e 100644 --- a/.changeset/pink-spoons-hope.md +++ b/.changeset/pink-spoons-hope.md @@ -1,5 +1,25 @@ --- -'@backstage/plugin-search-backend': patch +'@backstage/plugin-search-backend': minor --- -Add support for filtering search results according to visibility. +**BREAKING** Added three additional required properties to `createRouter` to support filtering search results based on permissions. To make this change to an existing app, add the required parameters to the `createRouter` call in `packages/backend/src/plugins/search.ts`: + +```diff +export default async function createPlugin({ + logger, ++ permissions, + discovery, + config, + tokenManager, +}: PluginEnvironment) { + /* ... */ + + return await createRouter({ + engine: indexBuilder.getSearchEngine(), ++ types: indexBuilder.getDocumentTypes(), ++ permissions, ++ config, + logger, + }); +} +``` From 01b27d547c74ee0f3a739bce4589ddbe004f5475 Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Tue, 25 Jan 2022 13:23:40 +0000 Subject: [PATCH 16/22] create-app: add changeset for new search-backend createRouter params Signed-off-by: MT Lewis --- .changeset/heavy-jars-repair.md | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 .changeset/heavy-jars-repair.md diff --git a/.changeset/heavy-jars-repair.md b/.changeset/heavy-jars-repair.md new file mode 100644 index 0000000000..b4fba9f00c --- /dev/null +++ b/.changeset/heavy-jars-repair.md @@ -0,0 +1,25 @@ +--- +'@backstage/create-app': patch +--- + +Added three additional required properties to the search-backend `createRouter` function to support filtering search results based on permissions. To make this change to an existing app, add the required parameters to the `createRouter` call in `packages/backend/src/plugins/search.ts`: + +```diff +export default async function createPlugin({ + logger, ++ permissions, + discovery, + config, + tokenManager, +}: PluginEnvironment) { + /* ... */ + + return await createRouter({ + engine: indexBuilder.getSearchEngine(), ++ types: indexBuilder.getDocumentTypes(), ++ permissions, ++ config, + logger, + }); +} +``` From e73ad9c89a91a9eab4dd516b7129ee7763fe89fd Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Fri, 21 Jan 2022 22:49:31 +0100 Subject: [PATCH 17/22] Add AuthorizedSearchEngine tests Signed-off-by: Vincenzo Scamporlino --- .../service/AuthorizedSearchEngine.test.ts | 382 ++++++++++++++++++ .../src/service/AuthorizedSearchEngine.ts | 23 +- plugins/search-backend/src/service/router.ts | 6 +- 3 files changed, 399 insertions(+), 12 deletions(-) create mode 100644 plugins/search-backend/src/service/AuthorizedSearchEngine.test.ts diff --git a/plugins/search-backend/src/service/AuthorizedSearchEngine.test.ts b/plugins/search-backend/src/service/AuthorizedSearchEngine.test.ts new file mode 100644 index 0000000000..2080e36331 --- /dev/null +++ b/plugins/search-backend/src/service/AuthorizedSearchEngine.test.ts @@ -0,0 +1,382 @@ +/* + * Copyright 2022 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +import { + AuthorizeDecision, + AuthorizeResult, + PermissionAuthorizer, +} from '@backstage/plugin-permission-common'; +import { DocumentTypeInfo } from '@backstage/plugin-search-backend-node'; +import { IndexableDocument, SearchEngine } from '@backstage/search-common'; +import { + encodePageCursor, + decodePageCursor, + AuthorizedSearchEngine, +} from './AuthorizedSearchEngine'; + +describe('AuthorizedSearchEngine', () => { + const typeUsers = 'users'; + const typeTemplates = 'templates'; + const typeServices = 'services'; + + function generateSampleResults(type: string, withAuthorization?: boolean) { + return Array(10) + .fill(0) + .map((_, index) => ({ + type, + document: { + title: `${type}_doc_${index}`, + authorization: withAuthorization + ? { resourceRef: `${type}_doc_${index}` } + : undefined, + } as IndexableDocument, + })); + } + + const allUsers = generateSampleResults(typeUsers); + const allTemplates = generateSampleResults(typeTemplates); + + const results = allUsers.concat(allTemplates); + + const mockedQuery: jest.MockedFunction = jest + .fn() + .mockImplementation(async () => ({ results })); + + const searchEngine: SearchEngine = { + setTranslator: () => { + throw new Error('Function not implemented. 1'); + }, + index: () => { + throw new Error('Function not implemented.2'); + }, + query: mockedQuery, + }; + + const mockedAuthorize: jest.MockedFunction< + PermissionAuthorizer['authorize'] + > = jest.fn(); + + const permissionAuthorizer: PermissionAuthorizer = { + authorize: mockedAuthorize, + }; + + const defaultTypes: Record = { + [typeUsers]: { + visibilityPermission: { + name: 'search.users.read', + attributes: { action: 'read' }, + }, + }, + [typeTemplates]: { + visibilityPermission: { + name: 'search.templates.read', + attributes: { action: 'read' }, + }, + }, + }; + + const authorizedSearchEngine = new AuthorizedSearchEngine( + searchEngine, + defaultTypes, + permissionAuthorizer, + ); + + const options = { token: 'token' }; + + const allowAll: PermissionAuthorizer['authorize'] = async queries => { + return queries.map(() => ({ + result: AuthorizeResult.ALLOW, + })); + }; + + beforeEach(() => { + mockedQuery.mockClear(); + mockedAuthorize.mockClear(); + }); + + it('should forward the parameters correctly', async () => { + mockedAuthorize.mockImplementation(allowAll); + const filters = { just: 1, a: 2, filter: 3 }; + await authorizedSearchEngine.query( + { term: 'term', filters, types: ['one', 'two'] }, + options, + ); + expect(mockedQuery).toHaveBeenCalledWith( + { + term: 'term', + types: ['one', 'two'], + filters, + }, + { token: 'token' }, + ); + }); + + it('should forward the default types if none are passed', async () => { + mockedAuthorize.mockImplementation(allowAll); + await authorizedSearchEngine.query({ term: '' }, options); + expect(mockedQuery).toHaveBeenCalledWith( + { term: '', types: ['users', 'templates'] }, + { token: 'token' }, + ); + }); + + it('should return all the results if all queries are allowed', async () => { + mockedAuthorize.mockImplementation(allowAll); + + await expect( + authorizedSearchEngine.query({ term: '' }, options), + ).resolves.toEqual({ results }); + expect(mockedAuthorize).toHaveBeenCalledTimes(1); + }); + + it('should batch authorized requests', async () => { + mockedAuthorize.mockImplementation(allowAll); + + await authorizedSearchEngine.query( + { term: '', types: [typeUsers, typeTemplates] }, + options, + ); + expect(mockedQuery).toHaveBeenCalledWith( + { term: '', types: ['users', 'templates'] }, + { token: 'token' }, + ); + expect(mockedAuthorize).toHaveBeenCalledTimes(1); + expect(mockedAuthorize).toHaveBeenLastCalledWith( + [ + { permission: defaultTypes[typeUsers].visibilityPermission }, + { permission: defaultTypes[typeTemplates].visibilityPermission }, + ], + { token: 'token' }, + ); + }); + + it('should skip sending request for types that are not allowed', async () => { + mockedAuthorize.mockImplementation(async queries => { + return queries.map(query => { + if ( + query.permission.name === + defaultTypes.users.visibilityPermission?.name + ) { + return { + result: AuthorizeResult.DENY, + }; + } + return { + result: AuthorizeResult.ALLOW, + }; + }); + }); + + await authorizedSearchEngine.query({ term: '' }, options); + + expect(mockedQuery).toHaveBeenCalledWith( + { term: '', types: ['templates'] }, + { token: 'token' }, + ); + + expect(mockedAuthorize).toHaveBeenCalledTimes(1); + }); + + it('should perform result-by-result filtering', async () => { + const usersWithAuth = generateSampleResults(typeUsers, true); + const templatesWithAuth = generateSampleResults(typeTemplates, true); + + const resultsWithAuth = usersWithAuth.concat(templatesWithAuth); + + mockedQuery.mockImplementation(async () => ({ + results: resultsWithAuth, + })); + + const userToBeReturned = 8; + + // TODO(vinzscam): I am not sure if such authorizer makes sense + mockedAuthorize.mockImplementation(async queries => + queries.map(query => { + if ( + query.permission.name === + defaultTypes.users.visibilityPermission?.name + ) { + if (query.resourceRef) { + return { + result: query.resourceRef.endsWith(userToBeReturned.toString()) + ? AuthorizeResult.ALLOW + : AuthorizeResult.DENY, + }; + } + return { + result: AuthorizeResult.CONDITIONAL, + // TODO(vinzscam): is something like this allowed? + // I guess NO, but I am not sure. + // if yes, who is responsible for evaluating this? + conditions: { not: { rule: 'ok', params: [] } }, + }; + } + + return { + result: AuthorizeResult.DENY, + }; + }), + ); + + await expect( + authorizedSearchEngine.query({ term: '' }, options), + ).resolves.toEqual({ results: [usersWithAuth[userToBeReturned]] }); + + expect(mockedQuery).toHaveBeenCalledWith( + { term: '', types: ['users'] }, + { token: 'token' }, + ); + }); + + it('should perform search until the target number of results is reached', async () => { + mockedAuthorize.mockImplementation(async queries => + queries.map(query => { + if (query.resourceRef) { + return { result: AuthorizeResult.ALLOW }; + } + // TODO(vinzscam): again, not sure about this + return { result: AuthorizeResult.CONDITIONAL } as AuthorizeDecision; + }), + ); + + const usersWithAuth = generateSampleResults(typeUsers, true); + const templatesWithAuth = generateSampleResults(typeTemplates, true); + const servicesWithAuth = generateSampleResults(typeServices, true); + + mockedQuery + .mockImplementationOnce(async () => ({ + results: usersWithAuth, + nextPageCursor: encodePageCursor({ page: 1 }), + })) + .mockImplementationOnce(async () => ({ + results: templatesWithAuth, + nextPageCursor: encodePageCursor({ page: 2 }), + })) + .mockImplementationOnce(async () => ({ + results: servicesWithAuth, + })); + + const result = await authorizedSearchEngine.query({ term: '' }, options); + + expect(mockedQuery).toHaveBeenCalledTimes(3); + expect(mockedQuery).toHaveBeenNthCalledWith( + 1, + { term: '', types: ['users', 'templates'] }, + { token: 'token' }, + ); + expect(mockedQuery).toHaveBeenNthCalledWith( + 2, + { term: '', types: ['users', 'templates'], pageCursor: 'MQ==' }, + { token: 'token' }, + ); + expect(mockedQuery).toHaveBeenNthCalledWith( + 3, + { term: '', types: ['users', 'templates'], pageCursor: 'Mg==' }, + { token: 'token' }, + ); + + const expectedResult = [ + ...usersWithAuth, + ...templatesWithAuth, + ...servicesWithAuth, + ].slice(0, 25); + + const expectedFirstRequestCursor = 'MQ=='; + expect(result).toEqual({ + results: expectedResult, + nextPageCursor: expectedFirstRequestCursor, + }); + }); + + it('should discard results until the target cursor is reached', async () => { + mockedAuthorize.mockImplementation(async queries => + queries.map(query => { + if (query.resourceRef) { + return { result: AuthorizeResult.ALLOW }; + } + // TODO(vinzscam): again, not sure about this + return { result: AuthorizeResult.CONDITIONAL } as AuthorizeDecision; + }), + ); + + const usersWithAuth = generateSampleResults(typeUsers, true); + const templatesWithAuth = generateSampleResults(typeTemplates, true); + const servicesWithAuth = generateSampleResults(typeServices, true); + + mockedQuery + .mockImplementationOnce(async () => ({ + results: usersWithAuth, + nextPageCursor: encodePageCursor({ page: 1 }), + })) + .mockImplementationOnce(async () => ({ + results: templatesWithAuth, + nextPageCursor: encodePageCursor({ page: 2 }), + })) + .mockImplementationOnce(async () => ({ + results: servicesWithAuth, + })); + + const startingFromCursor = encodePageCursor({ page: 1 }); + + const result = await authorizedSearchEngine.query( + { term: '', pageCursor: startingFromCursor }, + options, + ); + expect(mockedQuery).toHaveBeenCalledTimes(3); + expect(mockedQuery).toHaveBeenNthCalledWith( + 1, + { term: '', types: ['users', 'templates'] }, + { token: 'token' }, + ); + expect(mockedQuery).toHaveBeenNthCalledWith( + 2, + { term: '', types: ['users', 'templates'], pageCursor: 'MQ==' }, + { token: 'token' }, + ); + expect(mockedQuery).toHaveBeenNthCalledWith( + 3, + { term: '', types: ['users', 'templates'], pageCursor: 'Mg==' }, + { token: 'token' }, + ); + + expect(result).toEqual({ + results: servicesWithAuth.slice(5), + previousPageCursor: encodePageCursor({ page: 0 }), + }); + }); +}); + +describe('decodePageCursor', () => { + it('should correctly decode the cursor', () => { + expect(decodePageCursor()).toEqual({ page: 0 }); + expect(decodePageCursor(encodePageCursor({ page: 1 }))).toEqual({ + page: 1, + }); + expect(decodePageCursor('Mg==')).toEqual({ + page: 2, + }); + expect(decodePageCursor(encodePageCursor({ page: 0 }))).toEqual({ + page: 0, + }); + expect(decodePageCursor(encodePageCursor({ page: 100 }))).toEqual({ + page: 100, + }); + }); + + it('should throw an error if the cursor is not valid', () => { + expect(() => decodePageCursor(encodePageCursor({ page: -100 }))).toThrow(); + expect(() => decodePageCursor('something')).toThrow(); + }); +}); diff --git a/plugins/search-backend/src/service/AuthorizedSearchEngine.ts b/plugins/search-backend/src/service/AuthorizedSearchEngine.ts index 7b5f211d71..14214523b0 100644 --- a/plugins/search-backend/src/service/AuthorizedSearchEngine.ts +++ b/plugins/search-backend/src/service/AuthorizedSearchEngine.ts @@ -32,7 +32,6 @@ import { SearchResult, SearchResultSet, } from '@backstage/search-common'; -import { Config } from '@backstage/config'; import { InputError } from '@backstage/errors'; export function decodePageCursor(pageCursor?: string): { page: number } { @@ -58,19 +57,21 @@ export function encodePageCursor({ page }: { page: number }): string { return Buffer.from(`${page}`, 'utf-8').toString('base64'); } +export type AuthorizedSearchEngineConfig = { + queryLatencyBudgetMs: number; + pageSize: number; +}; + export class AuthorizedSearchEngine implements SearchEngine { - private readonly pageSize: number = 25; - private readonly queryLatencyBudgetMs: number; + private readonly config: AuthorizedSearchEngineConfig; constructor( private readonly searchEngine: SearchEngine, private readonly types: Record, private readonly permissions: PermissionAuthorizer, - config: Config, + config?: Partial, ) { - this.queryLatencyBudgetMs = - config.getOptionalNumber('search.permissions.queryLatencyBudgetMs') ?? - 1000; + this.config = { queryLatencyBudgetMs: 1000, pageSize: 25, ...config }; } setTranslator(translator: QueryTranslator): void { @@ -133,7 +134,7 @@ export class AuthorizedSearchEngine implements SearchEngine { } const { page } = decodePageCursor(query.pageCursor); - const targetResults = (page + 1) * this.pageSize; + const targetResults = (page + 1) * this.config.pageSize; let filteredResults: SearchResult[] = []; let nextPageCursor: string | undefined; @@ -151,7 +152,7 @@ export class AuthorizedSearchEngine implements SearchEngine { nextPageCursor = nextPage.nextPageCursor; latencyBudgetExhausted = - Date.now() - queryStartTime > this.queryLatencyBudgetMs; + Date.now() - queryStartTime > this.config.queryLatencyBudgetMs; } while ( nextPageCursor && filteredResults.length < targetResults && @@ -160,8 +161,8 @@ export class AuthorizedSearchEngine implements SearchEngine { return { results: filteredResults.slice( - page * this.pageSize, - (page + 1) * this.pageSize, + page * this.config.pageSize, + (page + 1) * this.config.pageSize, ), previousPageCursor: page === 0 ? undefined : encodePageCursor({ page: page - 1 }), diff --git a/plugins/search-backend/src/service/router.ts b/plugins/search-backend/src/service/router.ts index 28a3247919..51d3f8d795 100644 --- a/plugins/search-backend/src/service/router.ts +++ b/plugins/search-backend/src/service/router.ts @@ -68,7 +68,11 @@ export async function createRouter( }); const engine = config.getOptionalBoolean('permission.enabled') - ? new AuthorizedSearchEngine(inputEngine, types, permissions, config) + ? new AuthorizedSearchEngine(inputEngine, types, permissions, { + queryLatencyBudgetMs: config.getOptionalNumber( + 'search.permissions.queryLatencyBudgetMs', + ), + }) : inputEngine; const filterResultSet = ({ results, ...resultSet }: SearchResultSet) => ({ From 77cbb318834d272a0aaed6d303fb98ae645c03d9 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Mon, 24 Jan 2022 23:34:03 +0100 Subject: [PATCH 18/22] Fix comments Signed-off-by: Vincenzo Scamporlino --- .../service/AuthorizedSearchEngine.test.ts | 30 +++++++++---------- .../src/service/AuthorizedSearchEngine.ts | 18 ++++++----- plugins/search-backend/src/service/router.ts | 6 +--- 3 files changed, 26 insertions(+), 28 deletions(-) diff --git a/plugins/search-backend/src/service/AuthorizedSearchEngine.test.ts b/plugins/search-backend/src/service/AuthorizedSearchEngine.test.ts index 2080e36331..201f9b44f3 100644 --- a/plugins/search-backend/src/service/AuthorizedSearchEngine.test.ts +++ b/plugins/search-backend/src/service/AuthorizedSearchEngine.test.ts @@ -13,6 +13,8 @@ * See the License for the specific language governing permissions and * limitations under the License. */ + +import { ConfigReader } from '@backstage/config'; import { AuthorizeDecision, AuthorizeResult, @@ -91,6 +93,7 @@ describe('AuthorizedSearchEngine', () => { searchEngine, defaultTypes, permissionAuthorizer, + new ConfigReader({}), ); const options = { token: 'token' }; @@ -201,7 +204,6 @@ describe('AuthorizedSearchEngine', () => { const userToBeReturned = 8; - // TODO(vinzscam): I am not sure if such authorizer makes sense mockedAuthorize.mockImplementation(async queries => queries.map(query => { if ( @@ -217,11 +219,7 @@ describe('AuthorizedSearchEngine', () => { } return { result: AuthorizeResult.CONDITIONAL, - // TODO(vinzscam): is something like this allowed? - // I guess NO, but I am not sure. - // if yes, who is responsible for evaluating this? - conditions: { not: { rule: 'ok', params: [] } }, - }; + } as AuthorizeDecision; } return { @@ -246,7 +244,6 @@ describe('AuthorizedSearchEngine', () => { if (query.resourceRef) { return { result: AuthorizeResult.ALLOW }; } - // TODO(vinzscam): again, not sure about this return { result: AuthorizeResult.CONDITIONAL } as AuthorizeDecision; }), ); @@ -255,17 +252,23 @@ describe('AuthorizedSearchEngine', () => { const templatesWithAuth = generateSampleResults(typeTemplates, true); const servicesWithAuth = generateSampleResults(typeServices, true); + const allDocuments = [ + ...usersWithAuth, + ...templatesWithAuth, + ...servicesWithAuth, + ].sort(() => Math.floor(Math.random() * 3 - 1)); + mockedQuery .mockImplementationOnce(async () => ({ - results: usersWithAuth, + results: allDocuments.slice(0, 10), nextPageCursor: encodePageCursor({ page: 1 }), })) .mockImplementationOnce(async () => ({ - results: templatesWithAuth, + results: allDocuments.slice(10, 20), nextPageCursor: encodePageCursor({ page: 2 }), })) .mockImplementationOnce(async () => ({ - results: servicesWithAuth, + results: allDocuments.slice(20, 30), })); const result = await authorizedSearchEngine.query({ term: '' }, options); @@ -287,11 +290,7 @@ describe('AuthorizedSearchEngine', () => { { token: 'token' }, ); - const expectedResult = [ - ...usersWithAuth, - ...templatesWithAuth, - ...servicesWithAuth, - ].slice(0, 25); + const expectedResult = allDocuments.slice(0, 25); const expectedFirstRequestCursor = 'MQ=='; expect(result).toEqual({ @@ -306,7 +305,6 @@ describe('AuthorizedSearchEngine', () => { if (query.resourceRef) { return { result: AuthorizeResult.ALLOW }; } - // TODO(vinzscam): again, not sure about this return { result: AuthorizeResult.CONDITIONAL } as AuthorizeDecision; }), ); diff --git a/plugins/search-backend/src/service/AuthorizedSearchEngine.ts b/plugins/search-backend/src/service/AuthorizedSearchEngine.ts index 14214523b0..8d76f36071 100644 --- a/plugins/search-backend/src/service/AuthorizedSearchEngine.ts +++ b/plugins/search-backend/src/service/AuthorizedSearchEngine.ts @@ -32,6 +32,7 @@ import { SearchResult, SearchResultSet, } from '@backstage/search-common'; +import { Config } from '@backstage/config'; import { InputError } from '@backstage/errors'; export function decodePageCursor(pageCursor?: string): { page: number } { @@ -63,15 +64,18 @@ export type AuthorizedSearchEngineConfig = { }; export class AuthorizedSearchEngine implements SearchEngine { - private readonly config: AuthorizedSearchEngineConfig; + private readonly pageSize = 25; + private readonly queryLatencyBudgetMs: number; constructor( private readonly searchEngine: SearchEngine, private readonly types: Record, private readonly permissions: PermissionAuthorizer, - config?: Partial, + config: Config, ) { - this.config = { queryLatencyBudgetMs: 1000, pageSize: 25, ...config }; + this.queryLatencyBudgetMs = + config.getOptionalNumber('search.permissions.queryLatencyBudgetMs') ?? + 1000; } setTranslator(translator: QueryTranslator): void { @@ -134,7 +138,7 @@ export class AuthorizedSearchEngine implements SearchEngine { } const { page } = decodePageCursor(query.pageCursor); - const targetResults = (page + 1) * this.config.pageSize; + const targetResults = (page + 1) * this.pageSize; let filteredResults: SearchResult[] = []; let nextPageCursor: string | undefined; @@ -152,7 +156,7 @@ export class AuthorizedSearchEngine implements SearchEngine { nextPageCursor = nextPage.nextPageCursor; latencyBudgetExhausted = - Date.now() - queryStartTime > this.config.queryLatencyBudgetMs; + Date.now() - queryStartTime > this.queryLatencyBudgetMs; } while ( nextPageCursor && filteredResults.length < targetResults && @@ -161,8 +165,8 @@ export class AuthorizedSearchEngine implements SearchEngine { return { results: filteredResults.slice( - page * this.config.pageSize, - (page + 1) * this.config.pageSize, + page * this.pageSize, + (page + 1) * this.pageSize, ), previousPageCursor: page === 0 ? undefined : encodePageCursor({ page: page - 1 }), diff --git a/plugins/search-backend/src/service/router.ts b/plugins/search-backend/src/service/router.ts index 51d3f8d795..28a3247919 100644 --- a/plugins/search-backend/src/service/router.ts +++ b/plugins/search-backend/src/service/router.ts @@ -68,11 +68,7 @@ export async function createRouter( }); const engine = config.getOptionalBoolean('permission.enabled') - ? new AuthorizedSearchEngine(inputEngine, types, permissions, { - queryLatencyBudgetMs: config.getOptionalNumber( - 'search.permissions.queryLatencyBudgetMs', - ), - }) + ? new AuthorizedSearchEngine(inputEngine, types, permissions, config) : inputEngine; const filterResultSet = ({ results, ...resultSet }: SearchResultSet) => ({ From 43f9d00c1efa6b5d8a1e0168d48970dafeb06b3c Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Tue, 25 Jan 2022 00:25:10 +0100 Subject: [PATCH 19/22] Add result-by-result filtering pagination test Signed-off-by: Vincenzo Scamporlino --- .../service/AuthorizedSearchEngine.test.ts | 160 ++++++++++++++++-- 1 file changed, 144 insertions(+), 16 deletions(-) diff --git a/plugins/search-backend/src/service/AuthorizedSearchEngine.test.ts b/plugins/search-backend/src/service/AuthorizedSearchEngine.test.ts index 201f9b44f3..0a9f0718db 100644 --- a/plugins/search-backend/src/service/AuthorizedSearchEngine.test.ts +++ b/plugins/search-backend/src/service/AuthorizedSearchEngine.test.ts @@ -32,6 +32,7 @@ describe('AuthorizedSearchEngine', () => { const typeUsers = 'users'; const typeTemplates = 'templates'; const typeServices = 'services'; + const typeGroups = 'groups'; function generateSampleResults(type: string, withAuthorization?: boolean) { return Array(10) @@ -52,9 +53,7 @@ describe('AuthorizedSearchEngine', () => { const results = allUsers.concat(allTemplates); - const mockedQuery: jest.MockedFunction = jest - .fn() - .mockImplementation(async () => ({ results })); + const mockedQuery: jest.MockedFunction = jest.fn(); const searchEngine: SearchEngine = { setTranslator: () => { @@ -87,6 +86,18 @@ describe('AuthorizedSearchEngine', () => { attributes: { action: 'read' }, }, }, + [typeServices]: { + visibilityPermission: { + name: 'search.services.read', + attributes: { action: 'read' }, + }, + }, + [typeGroups]: { + visibilityPermission: { + name: 'search.groups.read', + attributes: { action: 'read' }, + }, + }, }; const authorizedSearchEngine = new AuthorizedSearchEngine( @@ -105,11 +116,12 @@ describe('AuthorizedSearchEngine', () => { }; beforeEach(() => { - mockedQuery.mockClear(); + mockedQuery.mockReset(); mockedAuthorize.mockClear(); }); it('should forward the parameters correctly', async () => { + mockedQuery.mockImplementation(async () => ({ results })); mockedAuthorize.mockImplementation(allowAll); const filters = { just: 1, a: 2, filter: 3 }; await authorizedSearchEngine.query( @@ -127,15 +139,17 @@ describe('AuthorizedSearchEngine', () => { }); it('should forward the default types if none are passed', async () => { + mockedQuery.mockImplementation(async () => ({ results })); mockedAuthorize.mockImplementation(allowAll); await authorizedSearchEngine.query({ term: '' }, options); expect(mockedQuery).toHaveBeenCalledWith( - { term: '', types: ['users', 'templates'] }, + { term: '', types: ['users', 'templates', 'services', 'groups'] }, { token: 'token' }, ); }); it('should return all the results if all queries are allowed', async () => { + mockedQuery.mockImplementation(async () => ({ results })); mockedAuthorize.mockImplementation(allowAll); await expect( @@ -145,6 +159,7 @@ describe('AuthorizedSearchEngine', () => { }); it('should batch authorized requests', async () => { + mockedQuery.mockImplementation(async () => ({ results })); mockedAuthorize.mockImplementation(allowAll); await authorizedSearchEngine.query( @@ -166,6 +181,7 @@ describe('AuthorizedSearchEngine', () => { }); it('should skip sending request for types that are not allowed', async () => { + mockedQuery.mockImplementation(async () => ({ results })); mockedAuthorize.mockImplementation(async queries => { return queries.map(query => { if ( @@ -185,7 +201,7 @@ describe('AuthorizedSearchEngine', () => { await authorizedSearchEngine.query({ term: '' }, options); expect(mockedQuery).toHaveBeenCalledWith( - { term: '', types: ['templates'] }, + { term: '', types: ['templates', 'services', 'groups'] }, { token: 'token' }, ); @@ -242,7 +258,9 @@ describe('AuthorizedSearchEngine', () => { mockedAuthorize.mockImplementation(async queries => queries.map(query => { if (query.resourceRef) { - return { result: AuthorizeResult.ALLOW }; + return { + result: AuthorizeResult.ALLOW, + }; } return { result: AuthorizeResult.CONDITIONAL } as AuthorizeDecision; }), @@ -256,7 +274,7 @@ describe('AuthorizedSearchEngine', () => { ...usersWithAuth, ...templatesWithAuth, ...servicesWithAuth, - ].sort(() => Math.floor(Math.random() * 3 - 1)); + ]; mockedQuery .mockImplementationOnce(async () => ({ @@ -271,22 +289,33 @@ describe('AuthorizedSearchEngine', () => { results: allDocuments.slice(20, 30), })); - const result = await authorizedSearchEngine.query({ term: '' }, options); + const result = await authorizedSearchEngine.query( + { term: '', types: ['users', 'templates', 'services'] }, + options, + ); expect(mockedQuery).toHaveBeenCalledTimes(3); expect(mockedQuery).toHaveBeenNthCalledWith( 1, - { term: '', types: ['users', 'templates'] }, + { term: '', types: ['users', 'templates', 'services'] }, { token: 'token' }, ); expect(mockedQuery).toHaveBeenNthCalledWith( 2, - { term: '', types: ['users', 'templates'], pageCursor: 'MQ==' }, + { + term: '', + types: ['users', 'templates', 'services'], + pageCursor: 'MQ==', + }, { token: 'token' }, ); expect(mockedQuery).toHaveBeenNthCalledWith( 3, - { term: '', types: ['users', 'templates'], pageCursor: 'Mg==' }, + { + term: '', + types: ['users', 'templates', 'services'], + pageCursor: 'Mg==', + }, { token: 'token' }, ); @@ -299,6 +328,93 @@ describe('AuthorizedSearchEngine', () => { }); }); + it('should perform search until the target number of results is reached, excluding unauthorized results', async () => { + mockedAuthorize.mockImplementation(async queries => + queries.map(query => { + if (query.resourceRef) { + return { + result: + query.permission.name === 'search.services.read' + ? AuthorizeResult.DENY + : AuthorizeResult.ALLOW, + }; + } + return { result: AuthorizeResult.CONDITIONAL } as AuthorizeDecision; + }), + ); + + const usersWithAuth = generateSampleResults(typeUsers, true); + const templatesWithAuth = generateSampleResults(typeTemplates, true); + const servicesWithAuth = generateSampleResults(typeServices, true); + const groupsWithAuth = generateSampleResults(typeGroups, true); + + const allDocuments = [ + ...usersWithAuth, + ...templatesWithAuth, + ...servicesWithAuth, + ...groupsWithAuth, + ].sort(() => Math.floor(Math.random() * 3 - 1)); + + mockedQuery + .mockImplementationOnce(async () => ({ + results: allDocuments.slice(0, 10), + nextPageCursor: encodePageCursor({ page: 1 }), + })) + .mockImplementationOnce(async () => ({ + results: allDocuments.slice(10, 20), + nextPageCursor: encodePageCursor({ page: 2 }), + })) + .mockImplementationOnce(async () => ({ + results: allDocuments.slice(20, 30), + nextPageCursor: encodePageCursor({ page: 3 }), + })) + .mockImplementationOnce(async () => ({ + results: allDocuments.slice(30, 40), + })); + + const result = await authorizedSearchEngine.query({ term: '' }, options); + + // check if a fourth request is needed for retrieving all results + const fourthRequestNeeded = + allDocuments.slice(0, 30).filter(d => d.type !== typeServices).length < + 25; + + expect(mockedQuery).toHaveBeenCalledTimes(fourthRequestNeeded ? 4 : 3); + expect(mockedQuery).toHaveBeenNthCalledWith( + 1, + { term: '', types: ['users', 'templates', 'services', 'groups'] }, + { token: 'token' }, + ); + expect(mockedQuery).toHaveBeenNthCalledWith( + 2, + { + term: '', + types: ['users', 'templates', 'services', 'groups'], + pageCursor: 'MQ==', + }, + { token: 'token' }, + ); + expect(mockedQuery).toHaveBeenNthCalledWith( + 3, + { + term: '', + types: ['users', 'templates', 'services', 'groups'], + pageCursor: 'Mg==', + }, + { token: 'token' }, + ); + + const expectedResult = allDocuments + .filter(d => d.type !== typeServices) + .slice(0, 25); + + const expectedFirstRequestCursor = 'MQ=='; + expect(result).toEqual({ + results: expectedResult, + nextPageCursor: expectedFirstRequestCursor, + }); + }); + it('should discard results until the target cursor is reached', async () => { mockedAuthorize.mockImplementation(async queries => queries.map(query => { @@ -329,23 +445,35 @@ describe('AuthorizedSearchEngine', () => { const startingFromCursor = encodePageCursor({ page: 1 }); const result = await authorizedSearchEngine.query( - { term: '', pageCursor: startingFromCursor }, + { + term: '', + pageCursor: startingFromCursor, + types: ['users', 'templates', 'services'], + }, options, ); expect(mockedQuery).toHaveBeenCalledTimes(3); expect(mockedQuery).toHaveBeenNthCalledWith( 1, - { term: '', types: ['users', 'templates'] }, + { term: '', types: ['users', 'templates', 'services'] }, { token: 'token' }, ); expect(mockedQuery).toHaveBeenNthCalledWith( 2, - { term: '', types: ['users', 'templates'], pageCursor: 'MQ==' }, + { + term: '', + types: ['users', 'templates', 'services'], + pageCursor: 'MQ==', + }, { token: 'token' }, ); expect(mockedQuery).toHaveBeenNthCalledWith( 3, - { term: '', types: ['users', 'templates'], pageCursor: 'Mg==' }, + { + term: '', + types: ['users', 'templates', 'services'], + pageCursor: 'Mg==', + }, { token: 'token' }, ); From 074f36cc43fa8a3f6808ab5b931aaa496058895c Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Tue, 25 Jan 2022 15:31:04 +0100 Subject: [PATCH 20/22] Import DocumentTypeInfo from common Signed-off-by: Vincenzo Scamporlino --- .../src/service/AuthorizedSearchEngine.test.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/plugins/search-backend/src/service/AuthorizedSearchEngine.test.ts b/plugins/search-backend/src/service/AuthorizedSearchEngine.test.ts index 0a9f0718db..878e24bb07 100644 --- a/plugins/search-backend/src/service/AuthorizedSearchEngine.test.ts +++ b/plugins/search-backend/src/service/AuthorizedSearchEngine.test.ts @@ -20,8 +20,11 @@ import { AuthorizeResult, PermissionAuthorizer, } from '@backstage/plugin-permission-common'; -import { DocumentTypeInfo } from '@backstage/plugin-search-backend-node'; -import { IndexableDocument, SearchEngine } from '@backstage/search-common'; +import { + DocumentTypeInfo, + IndexableDocument, + SearchEngine, +} from '@backstage/search-common'; import { encodePageCursor, decodePageCursor, From 9f394b231c1f270aca49a7d86cb3d52756dd8918 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Tue, 25 Jan 2022 16:31:29 +0100 Subject: [PATCH 21/22] Remove unused type Signed-off-by: Vincenzo Scamporlino --- plugins/search-backend/src/service/AuthorizedSearchEngine.ts | 5 ----- 1 file changed, 5 deletions(-) diff --git a/plugins/search-backend/src/service/AuthorizedSearchEngine.ts b/plugins/search-backend/src/service/AuthorizedSearchEngine.ts index 8d76f36071..d13fb2c02a 100644 --- a/plugins/search-backend/src/service/AuthorizedSearchEngine.ts +++ b/plugins/search-backend/src/service/AuthorizedSearchEngine.ts @@ -58,11 +58,6 @@ export function encodePageCursor({ page }: { page: number }): string { return Buffer.from(`${page}`, 'utf-8').toString('base64'); } -export type AuthorizedSearchEngineConfig = { - queryLatencyBudgetMs: number; - pageSize: number; -}; - export class AuthorizedSearchEngine implements SearchEngine { private readonly pageSize = 25; private readonly queryLatencyBudgetMs: number; From af4864b46234114f21aef03f5c4f3caf8151f93a Mon Sep 17 00:00:00 2001 From: MT Lewis Date: Wed, 26 Jan 2022 19:38:01 +0000 Subject: [PATCH 22/22] search-backend: deduplicate authorize calls Signed-off-by: MT Lewis --- plugins/search-backend/package.json | 1 + .../service/AuthorizedSearchEngine.test.ts | 66 +++++++++++++++++++ .../src/service/AuthorizedSearchEngine.ts | 13 +++- 3 files changed, 78 insertions(+), 2 deletions(-) diff --git a/plugins/search-backend/package.json b/plugins/search-backend/package.json index 1db6a0a910..049c199164 100644 --- a/plugins/search-backend/package.json +++ b/plugins/search-backend/package.json @@ -34,6 +34,7 @@ "express": "^4.17.1", "express-promise-router": "^4.1.0", "lodash": "^4.17.21", + "qs": "^6.10.1", "winston": "^3.2.1", "yn": "^4.0.0", "zod": "^3.11.6" diff --git a/plugins/search-backend/src/service/AuthorizedSearchEngine.test.ts b/plugins/search-backend/src/service/AuthorizedSearchEngine.test.ts index 878e24bb07..0a7b45e164 100644 --- a/plugins/search-backend/src/service/AuthorizedSearchEngine.test.ts +++ b/plugins/search-backend/src/service/AuthorizedSearchEngine.test.ts @@ -257,6 +257,72 @@ describe('AuthorizedSearchEngine', () => { ); }); + it('should deduplicate authorization queries when resourceRefs match', async () => { + const searchResults = [ + { + type: 'templates', + document: { + title: `doc_0_a`, + authorization: { resourceRef: `template_doc_0` }, + } as IndexableDocument, + }, + { + type: 'templates', + document: { + title: `doc_0_b`, + authorization: { resourceRef: `template_doc_0` }, + } as IndexableDocument, + }, + ]; + + mockedQuery.mockImplementation(async () => ({ + results: searchResults, + })); + + mockedAuthorize.mockImplementation(async queries => + queries.map(query => { + if (query.resourceRef) { + return { + result: AuthorizeResult.ALLOW, + }; + } + + return { + result: AuthorizeResult.CONDITIONAL, + } as AuthorizeDecision; + }), + ); + + await expect( + authorizedSearchEngine.query({ term: '', types: ['templates'] }, options), + ).resolves.toEqual({ results: searchResults }); + + expect(mockedAuthorize).toHaveBeenCalledTimes(2); + expect(mockedAuthorize).toHaveBeenNthCalledWith( + 1, + [ + { + permission: expect.objectContaining({ + name: 'search.templates.read', + }), + }, + ], + { token: 'token' }, + ); + expect(mockedAuthorize).toHaveBeenNthCalledWith( + 2, + [ + { + permission: expect.objectContaining({ + name: 'search.templates.read', + }), + resourceRef: 'template_doc_0', + }, + ], + { token: 'token' }, + ); + }); + it('should perform search until the target number of results is reached', async () => { mockedAuthorize.mockImplementation(async queries => queries.map(query => { diff --git a/plugins/search-backend/src/service/AuthorizedSearchEngine.ts b/plugins/search-backend/src/service/AuthorizedSearchEngine.ts index d13fb2c02a..d0488af00d 100644 --- a/plugins/search-backend/src/service/AuthorizedSearchEngine.ts +++ b/plugins/search-backend/src/service/AuthorizedSearchEngine.ts @@ -15,6 +15,7 @@ */ import { compact, zipObject } from 'lodash'; +import qs from 'qs'; import DataLoader from 'dataloader'; import { AuthorizeDecision, @@ -87,8 +88,16 @@ export class AuthorizedSearchEngine implements SearchEngine { ): Promise { const queryStartTime = Date.now(); - const authorizer = new DataLoader((requests: readonly AuthorizeQuery[]) => - this.permissions.authorize(requests.slice(), options), + const authorizer = new DataLoader( + (requests: readonly AuthorizeQuery[]) => + this.permissions.authorize(requests.slice(), options), + { + // Serialize the permission name and resourceRef as + // a query string to avoid collisions from overlapping + // permission names and resourceRefs. + cacheKeyFn: ({ permission: { name }, resourceRef }) => + qs.stringify({ name, resourceRef }), + }, ); const requestedTypes = query.types || Object.keys(this.types);