From 9312572360b69d65bcc2165a1f3c157a60c1eb54 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Mon, 22 Nov 2021 13:17:26 +0100 Subject: [PATCH] auth-backend: use more standardized error responses Co-authored-by: Johan Haals Signed-off-by: Patrik Oldsberg --- .changeset/forty-teachers-argue.md | 5 +++ .../src/lib/oauth/OAuthAdapter.test.ts | 20 ++++------- .../src/lib/oauth/OAuthAdapter.ts | 28 +++++++-------- .../src/lib/oauth/OAuthEnvironmentHandler.ts | 36 ++++++++----------- 4 files changed, 41 insertions(+), 48 deletions(-) create mode 100644 .changeset/forty-teachers-argue.md diff --git a/.changeset/forty-teachers-argue.md b/.changeset/forty-teachers-argue.md new file mode 100644 index 0000000000..2e32719037 --- /dev/null +++ b/.changeset/forty-teachers-argue.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-auth-backend': patch +--- + +Switched to using the standardized JSON error responses for all provider endpoints. diff --git a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts index dfb3a0a79a..fa61d69d80 100644 --- a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts +++ b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts @@ -176,7 +176,7 @@ describe('OAuthAdapter', () => { const mockResponse = { cookie: jest.fn().mockReturnThis(), - send: jest.fn().mockReturnThis(), + end: jest.fn().mockReturnThis(), status: jest.fn().mockReturnThis(), } as unknown as express.Response; @@ -187,6 +187,7 @@ describe('OAuthAdapter', () => { '', expect.objectContaining({ path: '/auth/test-provider' }), ); + expect(mockResponse.end).toHaveBeenCalledTimes(1); }); it('gets new access-token when refreshing', async () => { @@ -230,21 +231,14 @@ describe('OAuthAdapter', () => { const mockRequest = { header: () => 'XMLHttpRequest', - cookies: { - 'test-provider-refresh-token': 'token', - }, - query: {}, } as unknown as express.Request; - const mockResponse = { - send: jest.fn().mockReturnThis(), - status: jest.fn().mockReturnThis(), - } as unknown as express.Response; + const mockResponse = {} as unknown as express.Response; - await oauthProvider.refresh(mockRequest, mockResponse); - expect(mockResponse.send).toHaveBeenCalledTimes(1); - expect(mockResponse.send).toHaveBeenCalledWith( - 'Refresh token not supported for provider: test-provider', + await expect( + oauthProvider.refresh(mockRequest, mockResponse), + ).rejects.toThrow( + 'Refresh token is not supported for provider test-provider', ); }); }); diff --git a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts index 24b9a0f210..e1a6fdf2f9 100644 --- a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts +++ b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts @@ -22,7 +22,12 @@ import { BackstageIdentity, AuthProviderConfig, } from '../../providers/types'; -import { InputError, isError, NotAllowedError } from '@backstage/errors'; +import { + AuthenticationError, + InputError, + isError, + NotAllowedError, +} from '@backstage/errors'; import { TokenIssuer } from '../../identity/types'; import { readState, verifyNonce } from './helpers'; import { postMessageResponse, ensuresXRequestedWith } from '../flow'; @@ -166,29 +171,24 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { async logout(req: express.Request, res: express.Response): Promise { if (!ensuresXRequestedWith(req)) { - res.status(401).send('Invalid X-Requested-With header'); - return; + throw new AuthenticationError('Invalid X-Requested-With header'); } // remove refresh token cookie if it is set this.removeRefreshTokenCookie(res); - res.status(200).send('logout!'); + res.status(200).end(); } async refresh(req: express.Request, res: express.Response): Promise { if (!ensuresXRequestedWith(req)) { - res.status(401).send('Invalid X-Requested-With header'); - return; + throw new AuthenticationError('Invalid X-Requested-With header'); } if (!this.handlers.refresh || this.options.disableRefresh) { - res - .status(400) - .send( - `Refresh token not supported for provider: ${this.options.providerId}`, - ); - return; + throw new InputError( + `Refresh token is not supported for provider ${this.options.providerId}`, + ); } try { @@ -197,7 +197,7 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { // throw error if refresh token is missing in the request if (!refreshToken) { - throw new Error('Missing session cookie'); + throw new InputError('Missing session cookie'); } const scope = req.query.scope?.toString() ?? ''; @@ -220,7 +220,7 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { res.status(200).json(response); } catch (error) { - res.status(401).send(String(error)); + throw new AuthenticationError('Refresh failed', error); } } diff --git a/plugins/auth-backend/src/lib/oauth/OAuthEnvironmentHandler.ts b/plugins/auth-backend/src/lib/oauth/OAuthEnvironmentHandler.ts index a2f427e741..95a0547a5e 100644 --- a/plugins/auth-backend/src/lib/oauth/OAuthEnvironmentHandler.ts +++ b/plugins/auth-backend/src/lib/oauth/OAuthEnvironmentHandler.ts @@ -16,7 +16,7 @@ import express from 'express'; import { Config } from '@backstage/config'; -import { InputError } from '@backstage/errors'; +import { InputError, NotFoundError } from '@backstage/errors'; import { readState } from './helpers'; import { AuthProviderRouteHandlers } from '../../providers/types'; @@ -42,26 +42,26 @@ export class OAuthEnvironmentHandler implements AuthProviderRouteHandlers { ) {} async start(req: express.Request, res: express.Response): Promise { - const provider = this.getProviderForEnv(req, res); - await provider?.start(req, res); + const provider = this.getProviderForEnv(req); + await provider.start(req, res); } async frameHandler( req: express.Request, res: express.Response, ): Promise { - const provider = this.getProviderForEnv(req, res); - await provider?.frameHandler(req, res); + const provider = this.getProviderForEnv(req); + await provider.frameHandler(req, res); } async refresh(req: express.Request, res: express.Response): Promise { - const provider = this.getProviderForEnv(req, res); - await provider?.refresh?.(req, res); + const provider = this.getProviderForEnv(req); + await provider.refresh?.(req, res); } async logout(req: express.Request, res: express.Response): Promise { - const provider = this.getProviderForEnv(req, res); - await provider?.logout?.(req, res); + const provider = this.getProviderForEnv(req); + await provider.logout?.(req, res); } private getRequestFromEnv(req: express.Request): string | undefined { @@ -77,26 +77,20 @@ export class OAuthEnvironmentHandler implements AuthProviderRouteHandlers { return env; } - private getProviderForEnv( - req: express.Request, - res: express.Response, - ): AuthProviderRouteHandlers | undefined { + private getProviderForEnv(req: express.Request): AuthProviderRouteHandlers { const env: string | undefined = this.getRequestFromEnv(req); if (!env) { throw new InputError(`Must specify 'env' query to select environment`); } - if (!this.handlers.has(env)) { - res.status(404).send( - `Missing configuration. -
-
- For this flow to work you need to supply a valid configuration for the "${env}" environment of provider.`, + const handler = this.handlers.get(env); + if (!handler) { + throw new NotFoundError( + `No configuration available for the '${env}' environment of this provider.`, ); - return undefined; } - return this.handlers.get(env); + return handler; } }