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

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];

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;
 

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<void> {
     // 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;
   }
 
   /**

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);

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;
   }
 }
 

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;