From e73ad9c89a91a9eab4dd516b7129ee7763fe89fd Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Fri, 21 Jan 2022 22:49:31 +0100 Subject: [PATCH 1/5] 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 2/5] 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 3/5] 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 4/5] 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 5/5] 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;