From 5f06a08daa4ae48a0dc8f197f26c1b289e2b290e Mon Sep 17 00:00:00 2001 From: Govindarajan Nagarajan Date: Wed, 5 Aug 2020 17:04:25 +0200 Subject: [PATCH] Address PR comments: Use `undefined` instead of passing Empty strings Type fn that identifies the env (environment) of a Request Fail early if `env` is not present in the request or has an invalid `env` in the state --- .../src/lib/EnvironmentHandler.ts | 9 ++-- .../src/lib/OAuthProvider.test.ts | 3 +- plugins/auth-backend/src/lib/OAuthProvider.ts | 43 +++++++++++-------- .../auth-backend/src/providers/factories.ts | 9 ++-- .../src/providers/saml/provider.ts | 4 +- plugins/auth-backend/src/providers/types.ts | 6 ++- 6 files changed, 45 insertions(+), 29 deletions(-) diff --git a/plugins/auth-backend/src/lib/EnvironmentHandler.ts b/plugins/auth-backend/src/lib/EnvironmentHandler.ts index d2fe25c947..fb39ed86b8 100644 --- a/plugins/auth-backend/src/lib/EnvironmentHandler.ts +++ b/plugins/auth-backend/src/lib/EnvironmentHandler.ts @@ -15,7 +15,10 @@ */ import express from 'express'; -import { AuthProviderRouteHandlers } from '../providers/types'; +import { + AuthProviderRouteHandlers, + EnvironmentIdentifierFn, +} from '../providers/types'; export type EnvironmentHandlers = { [key: string]: AuthProviderRouteHandlers; @@ -25,14 +28,14 @@ export class EnvironmentHandler implements AuthProviderRouteHandlers { constructor( private readonly providerId: string, private readonly providers: EnvironmentHandlers, - private readonly envIdentifier: (req: express.Request) => string, + private readonly envIdentifier: EnvironmentIdentifierFn, ) {} private getProviderForEnv( req: express.Request, res: express.Response, ): AuthProviderRouteHandlers | undefined { - const env: string = this.envIdentifier(req); + const env: string | undefined = this.envIdentifier(req); if (env && this.providers.hasOwnProperty(env)) { return this.providers[env]; diff --git a/plugins/auth-backend/src/lib/OAuthProvider.test.ts b/plugins/auth-backend/src/lib/OAuthProvider.test.ts index 8c6c902b72..771e991c8e 100644 --- a/plugins/auth-backend/src/lib/OAuthProvider.test.ts +++ b/plugins/auth-backend/src/lib/OAuthProvider.test.ts @@ -65,7 +65,7 @@ describe('OAuthProvider Utils', () => { } as unknown) as express.Request; expect(() => { verifyNonce(mockRequest, 'providera'); - }).toThrowError('Auth response is missing state nonce'); + }).toThrowError('Invalid state passed via request'); }); it('should throw error if nonce mismatch', () => { @@ -221,6 +221,7 @@ describe('OAuthProvider', () => { const mockRequest = ({ query: { scope: 'user', + env: 'development', }, } as unknown) as express.Request; diff --git a/plugins/auth-backend/src/lib/OAuthProvider.ts b/plugins/auth-backend/src/lib/OAuthProvider.ts index 2e4c85a372..ca8fc5bf0a 100644 --- a/plugins/auth-backend/src/lib/OAuthProvider.ts +++ b/plugins/auth-backend/src/lib/OAuthProvider.ts @@ -41,26 +41,27 @@ export type Options = { }; const readState = (stateString: string): OAuthState => { - try { - const state = Object.fromEntries( - new URLSearchParams(decodeURIComponent(stateString)), - ); - return { - nonce: state.nonce ?? '', - env: state.env ?? '', - }; - } catch (e) { - return { - nonce: '', - env: '', - }; + const state = Object.fromEntries( + new URLSearchParams(decodeURIComponent(stateString)), + ); + if ( + !state.nonce || + !state.env || + state.nonce?.length === 0 || + state.env?.length === 0 + ) { + throw Error(`Invalid state passed via request`); } + return { + nonce: state.nonce, + env: state.env, + }; }; export const encodeState = (state: OAuthState): string => { const searchParams = new URLSearchParams(); - searchParams.append('nonce', state.nonce ?? ''); - searchParams.append('env', state.env ?? ''); + searchParams.append('nonce', state.nonce); + searchParams.append('env', state.env); return encodeURIComponent(searchParams.toString()); }; @@ -130,7 +131,11 @@ export class OAuthProvider implements AuthProviderRouteHandlers { async start(req: express.Request, res: express.Response): Promise { // retrieve scopes from request const scope = req.query.scope?.toString() ?? ''; - const env = req.query.env?.toString() ?? ''; + const env = req.query.env?.toString(); + + if (!env) { + throw new InputError('No env provided in request query parameters'); + } if (this.options.persistScopes) { this.setScopesCookie(res, scope); @@ -255,17 +260,17 @@ export class OAuthProvider implements AuthProviderRouteHandlers { } } - identifyEnv(req: express.Request): string { + identifyEnv(req: express.Request): string | undefined { const reqEnv = req.query.env?.toString(); if (reqEnv) { return reqEnv; } const stateParams = req.query.state?.toString(); if (!stateParams) { - return ''; + return undefined; } const env = readState(stateParams).env; - return env ?? ''; + return env; } /** diff --git a/plugins/auth-backend/src/providers/factories.ts b/plugins/auth-backend/src/providers/factories.ts index 403b39312c..701eef81af 100644 --- a/plugins/auth-backend/src/providers/factories.ts +++ b/plugins/auth-backend/src/providers/factories.ts @@ -15,7 +15,6 @@ */ import Router from 'express-promise-router'; -import express from 'express'; import { Logger } from 'winston'; import { TokenIssuer } from '../identity'; import { createGithubProvider } from './github'; @@ -24,7 +23,11 @@ import { createGoogleProvider } from './google'; import { createOAuth2Provider } from './oauth2'; import { createOktaProvider } from './okta'; import { createSamlProvider } from './saml'; -import { AuthProviderConfig, AuthProviderFactory } from './types'; +import { + AuthProviderConfig, + AuthProviderFactory, + EnvironmentIdentifierFn, +} from './types'; import { Config } from '@backstage/config'; import { EnvironmentHandlers, @@ -55,7 +58,7 @@ export const createAuthProviderRouter = ( const router = Router(); const envs = providerConfig.keys(); const envProviders: EnvironmentHandlers = {}; - let envIdentifier: ((req: express.Request) => string) | undefined; + let envIdentifier: EnvironmentIdentifierFn | undefined; for (const env of envs) { const envConfig = providerConfig.getConfig(env); diff --git a/plugins/auth-backend/src/providers/saml/provider.ts b/plugins/auth-backend/src/providers/saml/provider.ts index 9db88c8fa9..1ccee1a9cb 100644 --- a/plugins/auth-backend/src/providers/saml/provider.ts +++ b/plugins/auth-backend/src/providers/saml/provider.ts @@ -107,8 +107,8 @@ export class SamlAuthProvider implements AuthProviderRouteHandlers { res.send('noop'); } - identifyEnv(): string { - return ''; + identifyEnv(): string | undefined { + return undefined; } } diff --git a/plugins/auth-backend/src/providers/types.ts b/plugins/auth-backend/src/providers/types.ts index 74ed75d57a..d372fd1b94 100644 --- a/plugins/auth-backend/src/providers/types.ts +++ b/plugins/auth-backend/src/providers/types.ts @@ -211,7 +211,7 @@ export interface AuthProviderRouteHandlers { *- contains the environment context information encoded in the request * @param {express.Request} req */ - identifyEnv?(req: express.Request): string; + identifyEnv?(req: express.Request): string | undefined; } export type AuthProviderFactory = ( @@ -348,3 +348,7 @@ export type OAuthState = { nonce: string; env: string; }; + +export type EnvironmentIdentifierFn = ( + req: express.Request, +) => string | undefined;