From 2d20cf389cccd51953d1b07f565ef140d9915738 Mon Sep 17 00:00:00 2001
From: Govindarajan Nagarajan <govindarajan.nagarajan@zalando.de>
Date: Tue, 4 Aug 2020 14:45:03 +0200
Subject: [PATCH] Refactor: Address PR Comments

Create a new `OAuthState` type for storing the fields we want in the
`state` parameter.

Update the `encodeState` and `readState` fns to use the new `OAuthState`
type.

Update tests to reflect the new type
---
 .../src/lib/EnvironmentHandler.ts             | 14 -------
 plugins/auth-backend/src/lib/OAuthProvider.ts | 41 ++++++++++---------
 .../auth-backend/src/providers/factories.ts   | 10 +++--
 .../src/providers/saml/provider.ts            |  2 +-
 plugins/auth-backend/src/providers/types.ts   |  7 ++++
 5 files changed, 35 insertions(+), 39 deletions(-)

diff --git a/plugins/auth-backend/src/lib/EnvironmentHandler.ts b/plugins/auth-backend/src/lib/EnvironmentHandler.ts
index c9a182fc05..d2fe25c947 100644
--- a/plugins/auth-backend/src/lib/EnvironmentHandler.ts
+++ b/plugins/auth-backend/src/lib/EnvironmentHandler.ts
@@ -28,20 +28,6 @@ export class EnvironmentHandler implements AuthProviderRouteHandlers {
     private readonly envIdentifier: (req: express.Request) => string,
   ) {}
 
-  /* Return the value of `env` key, if it is exists, encoded within
-     the `state` parameter in the request
-  */
-  private getEnv(stateParams: Array<string>): string {
-    const envParams = stateParams.filter(
-      param => param.split('=')[0] === 'env',
-    );
-
-    if (envParams.length > 0) {
-      return envParams[0].split('=')[1];
-    }
-    return '';
-  }
-
   private getProviderForEnv(
     req: express.Request,
     res: express.Response,
diff --git a/plugins/auth-backend/src/lib/OAuthProvider.ts b/plugins/auth-backend/src/lib/OAuthProvider.ts
index 4dfed10016..608e7f2075 100644
--- a/plugins/auth-backend/src/lib/OAuthProvider.ts
+++ b/plugins/auth-backend/src/lib/OAuthProvider.ts
@@ -22,6 +22,7 @@ import {
   OAuthProviderHandlers,
   WebMessageResponse,
   BackstageIdentity,
+  OAuthState,
 } from '../providers/types';
 import { InputError } from '@backstage/backend-common';
 import { TokenIssuer } from '../identity';
@@ -39,27 +40,26 @@ export type Options = {
   tokenIssuer: TokenIssuer;
 };
 
-/* Return the value of `env` key, if it is exists, encoded within
-     the `state` parameter in the request
-  */
-const readStateParameter = (
-  stateParams: Array<string>,
-  parameter: string,
-): string => {
-  const envParams = stateParams.filter(
-    param => param.split('=')[0] === parameter,
-  );
-
-  if (envParams.length > 0) {
-    return envParams[0].split('=')[1];
+const readState = (stateString: string): OAuthState => {
+  try {
+    const state = JSON.parse(decodeURIComponent(stateString));
+    return {
+      nonce: state.nonce,
+      env: state.env,
+    };
+  } catch (e) {
+    return {
+      nonce: undefined,
+      env: undefined,
+    };
   }
-  return '';
 };
 
-const readState = (stateString: string): Array<string> => {
-  return decodeURIComponent(stateString).split('&');
+export const encodeState = (state: OAuthState): string => {
+  return encodeURIComponent(JSON.stringify(state));
 };
 
+/*
 export const encodeState = (stateObject: object): string => {
   const vals = Object.keys(stateObject).map(
     key =>
@@ -70,10 +70,11 @@ export const encodeState = (stateObject: object): string => {
 
   return vals.join('&');
 };
+*/
 export const verifyNonce = (req: express.Request, providerId: string) => {
   const cookieNonce = req.cookies[`${providerId}-nonce`];
-  const state: Array<string> = readState(req.query.state?.toString() ?? '');
-  const stateNonce = readStateParameter(state, 'nonce');
+  const state: OAuthState = readState(req.query.state?.toString() ?? '');
+  const stateNonce = state.nonce;
 
   if (!cookieNonce) {
     throw new Error('Auth response is missing cookie nonce');
@@ -269,8 +270,8 @@ export class OAuthProvider implements AuthProviderRouteHandlers {
     if (!stateParams) {
       return '';
     }
-    const env = readStateParameter(readState(stateParams), 'env');
-    return env;
+    const env = readState(stateParams).env;
+    return env ?? '';
   }
 
   /**
diff --git a/plugins/auth-backend/src/providers/factories.ts b/plugins/auth-backend/src/providers/factories.ts
index f4144ddc6c..403b39312c 100644
--- a/plugins/auth-backend/src/providers/factories.ts
+++ b/plugins/auth-backend/src/providers/factories.ts
@@ -55,19 +55,21 @@ export const createAuthProviderRouter = (
   const router = Router();
   const envs = providerConfig.keys();
   const envProviders: EnvironmentHandlers = {};
-  let envIdentifier: (req: express.Request) => string;
+  let envIdentifier: ((req: express.Request) => string) | undefined;
 
   for (const env of envs) {
     const envConfig = providerConfig.getConfig(env);
     const provider = factory(globalConfig, env, envConfig, logger, issuer);
     if (provider) {
       envProviders[env] = provider;
-      if (envIdentifier === undefined) {
-        envIdentifier = provider.identifyEnv;
-      }
+      envIdentifier = provider.identifyEnv;
     }
   }
 
+  if (typeof envIdentifier === 'undefined') {
+    throw Error(`No envIdentifier provided for '${providerId}'`);
+  }
+
   const handler = new EnvironmentHandler(
     providerId,
     envProviders,
diff --git a/plugins/auth-backend/src/providers/saml/provider.ts b/plugins/auth-backend/src/providers/saml/provider.ts
index 605a46ece4..9db88c8fa9 100644
--- a/plugins/auth-backend/src/providers/saml/provider.ts
+++ b/plugins/auth-backend/src/providers/saml/provider.ts
@@ -107,7 +107,7 @@ export class SamlAuthProvider implements AuthProviderRouteHandlers {
     res.send('noop');
   }
 
-  identifyEnv(_req: express.Request): string {
+  identifyEnv(): string {
     return '';
   }
 }
diff --git a/plugins/auth-backend/src/providers/types.ts b/plugins/auth-backend/src/providers/types.ts
index c2570b8768..afb136fd0f 100644
--- a/plugins/auth-backend/src/providers/types.ts
+++ b/plugins/auth-backend/src/providers/types.ts
@@ -341,3 +341,10 @@ export type SAMLProviderConfig = {
 export type SAMLEnvironmentProviderConfig = {
   [key: string]: SAMLProviderConfig;
 };
+
+export type OAuthState = {
+  /* A type for the serialized value in the `state` parameter of the OAuth authorization flow
+   */
+  nonce?: string;
+  env?: string;
+};