Merge pull request #25601 from backstage/rugvip/perm

backend-plugin-api: remove deprecated token option from PermissionsService
This commit is contained in:
Patrik Oldsberg
2024-07-15 20:38:57 +02:00
committed by GitHub
14 changed files with 165 additions and 254 deletions
+5
View File
@@ -0,0 +1,5 @@
---
'@backstage/plugin-permission-common': minor
---
**BREAKING**: Removed the deprecated and unused `token` option from `EvaluatorRequestOptions`. The `PermissionsClient` now has its own `PermissionClientRequestOptions` type that declares the `token` option instead.
+5
View File
@@ -0,0 +1,5 @@
---
'@backstage/plugin-permission-node': minor
---
**BREAKING**: Updated the `ServerPermissionClient` to match the new `PermissionsService` interface, where the deprecated `token` option has been removed and the options are now required.
+5
View File
@@ -0,0 +1,5 @@
---
'@backstage/backend-plugin-api': minor
---
**BREAKING**: The `PermissionsService` no longer supports passing the deprecated `token` option, and the request options are now required.
+5
View File
@@ -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.
+8 -9
View File
@@ -9,6 +9,7 @@ import { AuthorizePermissionRequest } from '@backstage/plugin-permission-common'
import { AuthorizePermissionResponse } from '@backstage/plugin-permission-common';
import { Config } from '@backstage/config';
import { Duration } from 'luxon';
import { EvaluatorRequestOptions } from '@backstage/plugin-permission-common';
import { Handler } from 'express';
import { HumanDuration } from '@backstage/types';
import { IdentityApi } from '@backstage/plugin-auth-node';
@@ -423,22 +424,20 @@ export interface LoggerService {
export interface PermissionsService extends PermissionEvaluator {
authorize(
requests: AuthorizePermissionRequest[],
options?: PermissionsServiceRequestOptions,
options: PermissionsServiceRequestOptions,
): Promise<AuthorizePermissionResponse[]>;
authorizeConditional(
requests: QueryPermissionRequest[],
options?: PermissionsServiceRequestOptions,
options: PermissionsServiceRequestOptions,
): Promise<QueryPermissionResponse[]>;
}
// @public
export type PermissionsServiceRequestOptions =
| {
token?: string;
}
| {
credentials: BackstageCredentials;
};
export interface PermissionsServiceRequestOptions
extends EvaluatorRequestOptions {
// (undocumented)
credentials: BackstageCredentials;
}
// @public
export interface PluginMetadataService {
@@ -17,6 +17,7 @@
import {
AuthorizePermissionRequest,
AuthorizePermissionResponse,
EvaluatorRequestOptions,
PermissionEvaluator,
QueryPermissionRequest,
QueryPermissionResponse,
@@ -24,18 +25,14 @@ import {
import { BackstageCredentials } from './AuthService';
/**
* Options for {@link @backstage/plugin-permission-common#PermissionEvaluator} requests.
* Options for {@link PermissionsService} requests.
*
* @public
*/
export type PermissionsServiceRequestOptions =
| {
/** @deprecated use the `credentials` option instead. */
token?: string;
}
| {
credentials: BackstageCredentials;
};
export interface PermissionsServiceRequestOptions
extends EvaluatorRequestOptions {
credentials: BackstageCredentials;
}
/**
* Permission system integration for authorization of user/service actions.
@@ -59,7 +56,7 @@ export interface PermissionsService extends PermissionEvaluator {
*/
authorize(
requests: AuthorizePermissionRequest[],
options?: PermissionsServiceRequestOptions,
options: PermissionsServiceRequestOptions,
): Promise<AuthorizePermissionResponse[]>;
/**
@@ -79,6 +76,6 @@ export interface PermissionsService extends PermissionEvaluator {
*/
authorizeConditional(
requests: QueryPermissionRequest[],
options?: PermissionsServiceRequestOptions,
options: PermissionsServiceRequestOptions,
): Promise<QueryPermissionResponse[]>;
}
+14 -7
View File
@@ -94,9 +94,7 @@ export type EvaluatePermissionResponseBatch =
PermissionMessageBatch<EvaluatePermissionResponse>;
// @public
export type EvaluatorRequestOptions = {
token?: string;
};
export interface EvaluatorRequestOptions {}
// @public
export type IdentifiedPermissionMessage<T> = T & {
@@ -162,14 +160,19 @@ export class PermissionClient implements PermissionEvaluator {
constructor(options: { discovery: DiscoveryApi; config: Config });
authorize(
requests: AuthorizePermissionRequest[],
options?: EvaluatorRequestOptions,
options?: PermissionClientRequestOptions,
): Promise<AuthorizePermissionResponse[]>;
authorizeConditional(
queries: QueryPermissionRequest[],
options?: EvaluatorRequestOptions,
options?: PermissionClientRequestOptions,
): Promise<QueryPermissionResponse[]>;
}
// @public
export type PermissionClientRequestOptions = {
token?: string;
};
// @public
export type PermissionCondition<
TResourceType extends string = string,
@@ -191,11 +194,15 @@ export type PermissionCriteria<TQuery> =
export interface PermissionEvaluator {
authorize(
requests: AuthorizePermissionRequest[],
options?: EvaluatorRequestOptions,
options?: EvaluatorRequestOptions & {
_ignored?: never;
},
): Promise<AuthorizePermissionResponse[]>;
authorizeConditional(
requests: QueryPermissionRequest[],
options?: EvaluatorRequestOptions,
options?: EvaluatorRequestOptions & {
_ignored?: never;
},
): Promise<QueryPermissionResponse[]>;
}
@@ -27,7 +27,6 @@ import {
PermissionEvaluator,
QueryPermissionRequest,
AuthorizePermissionRequest,
EvaluatorRequestOptions,
AuthorizePermissionResponse,
QueryPermissionResponse,
} from './types/api';
@@ -93,6 +92,15 @@ const responseSchema = <T>(
),
});
/**
* Options for {@link PermissionClient} requests.
*
* @public
*/
export type PermissionClientRequestOptions = {
token?: string;
};
/**
* An isomorphic client for requesting authorization for Backstage permissions.
* @public
@@ -112,7 +120,7 @@ export class PermissionClient implements PermissionEvaluator {
*/
async authorize(
requests: AuthorizePermissionRequest[],
options?: EvaluatorRequestOptions,
options?: PermissionClientRequestOptions,
): Promise<AuthorizePermissionResponse[]> {
return this.makeRequest(
requests,
@@ -126,7 +134,7 @@ export class PermissionClient implements PermissionEvaluator {
*/
async authorizeConditional(
queries: QueryPermissionRequest[],
options?: EvaluatorRequestOptions,
options?: PermissionClientRequestOptions,
): Promise<QueryPermissionResponse[]> {
return this.makeRequest(queries, queryPermissionResponseSchema, options);
}
+10 -12
View File
@@ -244,7 +244,7 @@ export interface PermissionEvaluator {
*/
authorize(
requests: AuthorizePermissionRequest[],
options?: EvaluatorRequestOptions,
options?: EvaluatorRequestOptions & { _ignored?: never }, // Since the options are empty we add this placeholder to reject all options
): Promise<AuthorizePermissionResponse[]>;
/**
@@ -257,22 +257,20 @@ export interface PermissionEvaluator {
*/
authorizeConditional(
requests: QueryPermissionRequest[],
options?: EvaluatorRequestOptions,
options?: EvaluatorRequestOptions & { _ignored?: never }, // Since the options are empty we add this placeholder to reject all options
): Promise<QueryPermissionResponse[]>;
}
// Note(Rugvip): I kept the below type around in case we want to add new options
// in the future, for example a signal. It also helps out enabling API
// constraints, as without this we can't have the permissions service implement
// the evaluator interface due to the mismatch in parameter count.
/**
* Options for {@link PermissionEvaluator} requests.
*
* This is currently empty, as there are no longer any common options for the permission evaluator.
*
* @public
*/
export type EvaluatorRequestOptions = {
/**
* @deprecated Backend plugins should no longer depend on the
* `PermissionEvaluator`, but instead use the `PermissionService` from
* `@backstage/backend-plugin-api`. Frontend plugins should not need to inject
* this token at all, but instead implicitly rely on underlying fetchApi to do
* it for them.
*/
token?: string;
};
export interface EvaluatorRequestOptions {}
@@ -83,18 +83,22 @@ describe('ServerPermissionClient', () => {
const client = ServerPermissionClient.fromConfig(new ConfigReader({}), {
discovery,
tokenManager: mockServices.tokenManager.mock(),
auth: mockServices.auth(),
});
await client.authorize([
{
permission: testBasicPermission,
},
]);
await client.authorize(
[
{
permission: testBasicPermission,
},
],
{ credentials: mockCredentials.none() },
);
expect(mockAuthorizeHandler).not.toHaveBeenCalled();
});
it('should bypass the permission backend if permissions are enabled and request has valid server token', async () => {
it('should bypass the permission backend if permissions are enabled and request has valid server credentials', async () => {
const client = ServerPermissionClient.fromConfig(config, {
discovery,
tokenManager: mockServices.tokenManager.mock(),
@@ -102,13 +106,13 @@ describe('ServerPermissionClient', () => {
});
await client.authorize([{ permission: testBasicPermission }], {
token: mockCredentials.service.token(),
credentials: mockCredentials.service(),
});
expect(mockAuthorizeHandler).not.toHaveBeenCalled();
});
it('should call the permission backend if permissions are enabled and request does not have valid server token', async () => {
it('should call the permission backend if permissions are enabled and request does not have valid server credentials', async () => {
const client = ServerPermissionClient.fromConfig(config, {
discovery,
tokenManager: mockServices.tokenManager.mock(),
@@ -116,10 +120,18 @@ describe('ServerPermissionClient', () => {
});
await client.authorize([{ permission: testBasicPermission }], {
token: mockCredentials.user.token(),
credentials: mockCredentials.user(),
});
expect(mockAuthorizeHandler).toHaveBeenCalled();
expect(
mockAuthorizeHandler.mock.calls[0][0].headers.get('authorization'),
).toBe(
mockCredentials.service.header({
onBehalfOf: mockCredentials.user(),
targetPluginId: 'permission',
}),
);
});
});
@@ -145,201 +157,59 @@ describe('ServerPermissionClient', () => {
const client = ServerPermissionClient.fromConfig(new ConfigReader({}), {
discovery,
tokenManager: mockServices.tokenManager.mock(),
});
await client.authorizeConditional([
{ permission: testResourcePermission },
]);
expect(mockAuthorizeHandler).not.toHaveBeenCalled();
});
it('should bypass the permission backend if permissions are enabled and request has valid server token', async () => {
const client = ServerPermissionClient.fromConfig(config, {
discovery,
tokenManager: mockServices.tokenManager.mock(),
auth: mockServices.auth(),
});
await client.authorizeConditional(
[{ permission: testResourcePermission }],
{
token: mockCredentials.service.token(),
credentials: mockCredentials.none(),
},
);
expect(mockAuthorizeHandler).not.toHaveBeenCalled();
});
it('should call the permission backend if permissions are enabled and request does not have valid server token', async () => {
const auth = mockServices.auth();
it('should bypass the permission backend if permissions are enabled and request has valid server credentials', async () => {
const client = ServerPermissionClient.fromConfig(config, {
discovery,
tokenManager: mockServices.tokenManager.mock(),
auth,
auth: mockServices.auth(),
});
await client.authorizeConditional(
[{ permission: testResourcePermission }],
{
token: mockCredentials.user.token(),
credentials: mockCredentials.service(),
},
);
expect(mockAuthorizeHandler).not.toHaveBeenCalled();
});
it('should call the permission backend if permissions are enabled and request does not have valid server credentials', async () => {
const client = ServerPermissionClient.fromConfig(config, {
discovery,
tokenManager: mockServices.tokenManager.mock(),
auth: mockServices.auth(),
});
await client.authorizeConditional(
[{ permission: testResourcePermission }],
{
credentials: mockCredentials.user(),
},
);
expect(mockAuthorizeHandler).toHaveBeenCalled();
});
});
describe('with credentials', () => {
describe('authorize', () => {
let mockAuthorizeHandler: jest.Mock;
beforeEach(() => {
mockAuthorizeHandler = jest.fn((req, res, { json }: RestContext) => {
const responses = req.body.items.map(
(r: IdentifiedPermissionMessage<DefinitivePolicyDecision>) => ({
id: r.id,
result: AuthorizeResult.ALLOW,
}),
);
return res(json({ items: responses }));
});
server.use(rest.post(`${mockBaseUrl}/authorize`, mockAuthorizeHandler));
});
it('should bypass the permission backend if permissions are disabled', async () => {
const client = ServerPermissionClient.fromConfig(new ConfigReader({}), {
discovery,
tokenManager: mockServices.tokenManager.mock(),
auth: mockServices.auth(),
});
await client.authorize(
[
{
permission: testBasicPermission,
},
],
{ credentials: mockCredentials.none() },
);
expect(mockAuthorizeHandler).not.toHaveBeenCalled();
});
it('should bypass the permission backend if permissions are enabled and request has valid server token', async () => {
const client = ServerPermissionClient.fromConfig(config, {
discovery,
tokenManager: mockServices.tokenManager.mock(),
auth: mockServices.auth(),
});
await client.authorize([{ permission: testBasicPermission }], {
credentials: mockCredentials.service(),
});
expect(mockAuthorizeHandler).not.toHaveBeenCalled();
});
it('should call the permission backend if permissions are enabled and request does not have valid server token', async () => {
const client = ServerPermissionClient.fromConfig(config, {
discovery,
tokenManager: mockServices.tokenManager.mock(),
auth: mockServices.auth(),
});
await client.authorize([{ permission: testBasicPermission }], {
credentials: mockCredentials.user(),
});
expect(mockAuthorizeHandler).toHaveBeenCalled();
expect(
mockAuthorizeHandler.mock.calls[0][0].headers.get('authorization'),
).toBe(
mockCredentials.service.header({
onBehalfOf: mockCredentials.user(),
targetPluginId: 'permission',
}),
);
});
});
describe('authorizeConditional', () => {
let mockAuthorizeHandler: jest.Mock;
beforeEach(() => {
mockAuthorizeHandler = jest.fn((req, res, { json }: RestContext) => {
const responses = req.body.items.map(
(r: IdentifiedPermissionMessage<ConditionalPolicyDecision>) => ({
id: r.id,
result: AuthorizeResult.ALLOW,
}),
);
return res(json({ items: responses }));
});
server.use(rest.post(`${mockBaseUrl}/authorize`, mockAuthorizeHandler));
});
it('should bypass the permission backend if permissions are disabled', async () => {
const client = ServerPermissionClient.fromConfig(new ConfigReader({}), {
discovery,
tokenManager: mockServices.tokenManager.mock(),
auth: mockServices.auth(),
});
await client.authorizeConditional(
[{ permission: testResourcePermission }],
{
credentials: mockCredentials.none(),
},
);
expect(mockAuthorizeHandler).not.toHaveBeenCalled();
});
it('should bypass the permission backend if permissions are enabled and request has valid server token', async () => {
const client = ServerPermissionClient.fromConfig(config, {
discovery,
tokenManager: mockServices.tokenManager.mock(),
auth: mockServices.auth(),
});
await client.authorizeConditional(
[{ permission: testResourcePermission }],
{
credentials: mockCredentials.service(),
},
);
expect(mockAuthorizeHandler).not.toHaveBeenCalled();
});
it('should call the permission backend if permissions are enabled and request does not have valid server token', async () => {
const client = ServerPermissionClient.fromConfig(config, {
discovery,
tokenManager: mockServices.tokenManager.mock(),
auth: mockServices.auth(),
});
await client.authorizeConditional(
[{ permission: testResourcePermission }],
{
credentials: mockCredentials.user(),
},
);
expect(mockAuthorizeHandler).toHaveBeenCalled();
expect(
mockAuthorizeHandler.mock.calls[0][0].headers.get('authorization'),
).toBe(
mockCredentials.service.header({
onBehalfOf: mockCredentials.user(),
targetPluginId: 'permission',
}),
);
});
expect(
mockAuthorizeHandler.mock.calls[0][0].headers.get('authorization'),
).toBe(
mockCredentials.service.header({
onBehalfOf: mockCredentials.user(),
targetPluginId: 'permission',
}),
);
});
});
@@ -146,14 +146,6 @@ export class ServerPermissionClient implements PermissionsService {
return options.credentials;
}
if (options?.token) {
try {
return await this.#auth.authenticate(options.token);
} catch {
// ignore
}
}
return undefined;
}
@@ -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
@@ -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<string, DocumentTypeInfo>,
private readonly permissions: PermissionsService,
private readonly auth: AuthService,
config: Config,
) {
this.queryLatencyBudgetMs =
@@ -92,9 +93,14 @@ export class AuthorizedSearchEngine implements SearchEngine {
): Promise<IndexableResultSet> {
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(
@@ -145,6 +145,7 @@ export async function createRouter(
inputEngine,
types,
permissionEvaluator,
auth,
config,
)
: inputEngine;