From 343f656eb15315164a2722e8b32553494b4b5d69 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Fri, 12 Jul 2024 12:21:41 +0200 Subject: [PATCH] search-backend: update to treat requests without credentials as unauthenticated Signed-off-by: Patrik Oldsberg --- .changeset/tiny-pianos-work.md | 5 ++ .../service/AuthorizedSearchEngine.test.ts | 49 ++++++++++++------- .../src/service/AuthorizedSearchEngine.ts | 16 ++++-- plugins/search-backend/src/service/router.ts | 1 + 4 files changed, 48 insertions(+), 23 deletions(-) create mode 100644 .changeset/tiny-pianos-work.md diff --git a/.changeset/tiny-pianos-work.md b/.changeset/tiny-pianos-work.md new file mode 100644 index 0000000000..033e8272e5 --- /dev/null +++ b/.changeset/tiny-pianos-work.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-search-backend': patch +--- + +The `AuthorizedSearchEngine` will now ignore the deprecated `token` option, and treat it as an unauthorized request. This will not have any effect in practice, since credentials are always provided by the router. diff --git a/plugins/search-backend/src/service/AuthorizedSearchEngine.test.ts b/plugins/search-backend/src/service/AuthorizedSearchEngine.test.ts index 420af5744b..e41f3f888b 100644 --- a/plugins/search-backend/src/service/AuthorizedSearchEngine.test.ts +++ b/plugins/search-backend/src/service/AuthorizedSearchEngine.test.ts @@ -32,6 +32,7 @@ import { decodePageCursor, AuthorizedSearchEngine, } from './AuthorizedSearchEngine'; +import { mockCredentials, mockServices } from '@backstage/backend-test-utils'; describe('AuthorizedSearchEngine', () => { const typeUsers = 'users'; @@ -117,10 +118,11 @@ describe('AuthorizedSearchEngine', () => { searchEngine, defaultTypes, permissionEvaluator, + mockServices.auth(), new ConfigReader({}), ); - const options = { token: 'token' }; + const options = { credentials: mockCredentials.user() }; const allowAll: PermissionEvaluator['authorize'] & PermissionEvaluator['authorizeConditional'] = async queries => { @@ -149,7 +151,7 @@ describe('AuthorizedSearchEngine', () => { types: ['one', 'two'], filters, }, - { token: 'token' }, + options, ); }); @@ -160,7 +162,18 @@ describe('AuthorizedSearchEngine', () => { await authorizedSearchEngine.query({ term: '' }, options); expect(mockedQuery).toHaveBeenCalledWith( { term: '', types: ['users', 'templates', 'services', 'groups'] }, - { token: 'token' }, + options, + ); + }); + + it('should fall back to none credentials if a token is provided', async () => { + mockedQuery.mockImplementation(async () => ({ results })); + mockedPermissionQuery.mockImplementation(allowAll); + + await authorizedSearchEngine.query({ term: '' }, { token: 'token' }); + expect(mockedQuery).toHaveBeenCalledWith( + { term: '', types: ['users', 'templates', 'services', 'groups'] }, + { credentials: mockCredentials.none() }, ); }); @@ -184,7 +197,7 @@ describe('AuthorizedSearchEngine', () => { ); expect(mockedQuery).toHaveBeenCalledWith( { term: '', types: ['users', 'templates'] }, - { token: 'token' }, + options, ); expect(mockedPermissionQuery).toHaveBeenCalledTimes(1); expect(mockedPermissionQuery).toHaveBeenLastCalledWith( @@ -192,7 +205,7 @@ describe('AuthorizedSearchEngine', () => { { permission: defaultTypes[typeUsers].visibilityPermission }, { permission: defaultTypes[typeTemplates].visibilityPermission }, ], - { token: 'token' }, + options, ); }); @@ -218,7 +231,7 @@ describe('AuthorizedSearchEngine', () => { expect(mockedQuery).toHaveBeenCalledWith( { term: '', types: ['templates', 'services', 'groups'] }, - { token: 'token' }, + options, ); expect(mockedPermissionQuery).toHaveBeenCalledTimes(1); @@ -270,7 +283,7 @@ describe('AuthorizedSearchEngine', () => { expect(mockedQuery).toHaveBeenCalledWith( { term: '', types: ['users'] }, - { token: 'token' }, + options, ); }); @@ -326,7 +339,7 @@ describe('AuthorizedSearchEngine', () => { }), }, ], - { token: 'token' }, + options, ); expect(mockedAuthorize).toHaveBeenCalledTimes(1); expect(mockedAuthorize).toHaveBeenCalledWith( @@ -338,7 +351,7 @@ describe('AuthorizedSearchEngine', () => { resourceRef: 'template_doc_0', }, ], - { token: 'token' }, + options, ); }); @@ -403,7 +416,7 @@ describe('AuthorizedSearchEngine', () => { expect(mockedQuery).toHaveBeenNthCalledWith( 1, { term: '', types: ['users', 'templates', 'services'] }, - { token: 'token' }, + options, ); expect(mockedQuery).toHaveBeenNthCalledWith( 2, @@ -412,7 +425,7 @@ describe('AuthorizedSearchEngine', () => { types: ['users', 'templates', 'services'], pageCursor: 'MQ==', }, - { token: 'token' }, + options, ); expect(mockedQuery).toHaveBeenNthCalledWith( 3, @@ -421,7 +434,7 @@ describe('AuthorizedSearchEngine', () => { types: ['users', 'templates', 'services'], pageCursor: 'Mg==', }, - { token: 'token' }, + options, ); const expectedResult = allDocuments @@ -494,7 +507,7 @@ describe('AuthorizedSearchEngine', () => { expect(mockedQuery).toHaveBeenNthCalledWith( 1, { term: '', types: ['users', 'templates', 'services', 'groups'] }, - { token: 'token' }, + options, ); expect(mockedQuery).toHaveBeenNthCalledWith( 2, @@ -503,7 +516,7 @@ describe('AuthorizedSearchEngine', () => { types: ['users', 'templates', 'services', 'groups'], pageCursor: 'MQ==', }, - { token: 'token' }, + options, ); expect(mockedQuery).toHaveBeenNthCalledWith( 3, @@ -512,7 +525,7 @@ describe('AuthorizedSearchEngine', () => { types: ['users', 'templates', 'services', 'groups'], pageCursor: 'Mg==', }, - { token: 'token' }, + options, ); const expectedResult = allDocuments @@ -574,7 +587,7 @@ describe('AuthorizedSearchEngine', () => { expect(mockedQuery).toHaveBeenNthCalledWith( 1, { term: '', types: ['users', 'templates', 'services'] }, - { token: 'token' }, + options, ); expect(mockedQuery).toHaveBeenNthCalledWith( 2, @@ -583,7 +596,7 @@ describe('AuthorizedSearchEngine', () => { types: ['users', 'templates', 'services'], pageCursor: 'MQ==', }, - { token: 'token' }, + options, ); expect(mockedQuery).toHaveBeenNthCalledWith( 3, @@ -592,7 +605,7 @@ describe('AuthorizedSearchEngine', () => { types: ['users', 'templates', 'services'], pageCursor: 'Mg==', }, - { token: 'token' }, + options, ); const expectedResults = servicesWithAuth diff --git a/plugins/search-backend/src/service/AuthorizedSearchEngine.ts b/plugins/search-backend/src/service/AuthorizedSearchEngine.ts index defb535f42..d9d3feeacf 100644 --- a/plugins/search-backend/src/service/AuthorizedSearchEngine.ts +++ b/plugins/search-backend/src/service/AuthorizedSearchEngine.ts @@ -39,7 +39,7 @@ import { import { Config } from '@backstage/config'; import { InputError } from '@backstage/errors'; import { Writable } from 'stream'; -import { PermissionsService } from '@backstage/backend-plugin-api'; +import { AuthService, PermissionsService } from '@backstage/backend-plugin-api'; export function decodePageCursor(pageCursor?: string): { page: number } { if (!pageCursor) { @@ -71,6 +71,7 @@ export class AuthorizedSearchEngine implements SearchEngine { private readonly searchEngine: SearchEngine, private readonly types: Record, private readonly permissions: PermissionsService, + private readonly auth: AuthService, config: Config, ) { this.queryLatencyBudgetMs = @@ -92,9 +93,14 @@ export class AuthorizedSearchEngine implements SearchEngine { ): Promise { const queryStartTime = Date.now(); + const compatOptions = + 'credentials' in options + ? options + : { credentials: await this.auth.getNoneCredentials() }; + const conditionFetcher = new DataLoader( (requests: readonly QueryPermissionRequest[]) => - this.permissions.authorizeConditional(requests.slice(), options), + this.permissions.authorizeConditional(requests.slice(), compatOptions), { cacheKeyFn: ({ permission: { name } }) => name, }, @@ -102,7 +108,7 @@ export class AuthorizedSearchEngine implements SearchEngine { const authorizer = new DataLoader( (requests: readonly AuthorizePermissionRequest[]) => - this.permissions.authorize(requests.slice(), options), + this.permissions.authorize(requests.slice(), compatOptions), { // Serialize the permission name and resourceRef as // a query string to avoid collisions from overlapping @@ -159,7 +165,7 @@ export class AuthorizedSearchEngine implements SearchEngine { if (!resultByResultFilteringRequired) { return this.searchEngine.query( { ...query, types: authorizedTypes }, - options, + compatOptions, ); } @@ -174,7 +180,7 @@ export class AuthorizedSearchEngine implements SearchEngine { do { const nextPage = await this.searchEngine.query( { ...query, types: authorizedTypes, pageCursor: nextPageCursor }, - options, + compatOptions, ); filteredResults = filteredResults.concat( diff --git a/plugins/search-backend/src/service/router.ts b/plugins/search-backend/src/service/router.ts index 250be2853c..d226805baf 100644 --- a/plugins/search-backend/src/service/router.ts +++ b/plugins/search-backend/src/service/router.ts @@ -145,6 +145,7 @@ export async function createRouter( inputEngine, types, permissionEvaluator, + auth, config, ) : inputEngine;