feat(sdk) remove pkce option becuase all OAuth flows must use PKCE

moneytree · Oct 6, 2023 · 2234e0e · 2234e0e
1 parent 3099aee
commit 2234e0e
Show file tree

Hide file tree

Showing 11 changed files with 92 additions and 75 deletions.
diff --git a/jest.config.js b/jest.config.js
@@ -8,6 +8,7 @@ module.exports = {
     // https://github.com/facebook/jest/issues/2070#issuecomment-431706685
     '<rootDir>/.*/__mocks__'
   ],
+  testPathIgnorePatterns: ['/__tests__/helper/'],
   globals: {
     __VERSION__: packageJSON.version
   }

diff --git a/sample/src/index.ts b/sample/src/index.ts
@@ -55,8 +55,6 @@ elements.authorizeBtn.onclick = () => {
     authorizeOptions.scopes = scopesSelectedOptions.map((x) => x.value);
   }
 
-  authorizeOptions.pkce = true;
-
   mtLinkSdk.authorize(authorizeOptions);
 };
 
@@ -75,7 +73,6 @@ elements.doOnboardBtn.onclick = async () => {
     }
 
     onBoardOptions.email = onboardOptionsElms.email.value;
-    onBoardOptions.pkce = true;
 
     mtLinkSdk.onboard(onBoardOptions);
   } catch (error) {

diff --git a/src/__tests__/helper/expect-url-to-match.ts b/src/__tests__/helper/expect-url-to-match.ts
@@ -0,0 +1,21 @@
+import qs from 'qs';
+
+export interface UrlExpectation {
+  baseUrl: string;
+  path: string;
+  query?: Record<string, string>;
+}
+
+export default function expectUrlToMatchWithPKCE(actual: URL | string, expectation: UrlExpectation) {
+  const url = typeof actual === 'string' ? new URL(actual) : actual;
+  const actualQuery = qs.parse(new URLSearchParams(url.search).toString());
+
+  expect(actualQuery.code_challenge).toBeDefined();
+  delete actualQuery.code_challenge; // ignore PKCE code challenge because it's randomly generated
+  expect(actualQuery.code_challenge_method).toBe('S256');
+  delete actualQuery.code_challenge_method;
+
+  expect(url.pathname).toBe(expectation.path);
+  expect(`${url.protocol}//${url.hostname}`).toBe(expectation.baseUrl);
+  expect(actualQuery).toEqual(expectation.query || {});
+}
diff --git a/src/__tests__/index.test.ts b/src/__tests__/index.test.ts
@@ -10,6 +10,7 @@ import tokenInfo from '../api/token-info';
 import mtLinkSdk, { Mode, MtLinkSdk } from '..';
 
 import packageJson from '../../package.json';
+import expectUrlToMatchWithPKCE from './helper/expect-url-to-match';
 
 jest.mock('../api/authorize');
 jest.mock('../api/onboard');
@@ -80,19 +81,30 @@ describe('index', () => {
 
     const sdkVersion = packageJson.version;
 
+    const authQuery = {
+      client_id: 'clientId',
+      response_type: 'code',
+      scope: 'scopes',
+      redirect_uri: 'redirectUri',
+      country: 'JP',
+      configs: `authn_method=sso&sdk_platform=js&sdk_version=${sdkVersion}`
+    }
     const result8 = instance.authorizeUrl({ scopes: 'scopes' });
-    expect(result8).toBe(
-      'https://myaccount.getmoneytree.com/oauth/authorize?client_id=clientId&response_type=code&' +
-        'scope=scopes&redirect_uri=redirectUri&country=JP&saml_subject_id=samlSubjectId&' +
-        `configs=authn_method%3Dsso%26sdk_platform%3Djs%26sdk_version%3D${sdkVersion}`
-    );
+    expectUrlToMatchWithPKCE(result8, {
+      baseUrl: 'https://myaccount.getmoneytree.com',
+      path: '/oauth/authorize',
+      query: {
+        ...authQuery,
+        saml_subject_id: 'samlSubjectId',
+      }
+    })
 
     const result9 = instance.onboardUrl({ scopes: 'scopes' });
-    expect(result9).toBe(
-      'https://myaccount.getmoneytree.com/onboard?client_id=clientId&response_type=code&' +
-        'scope=scopes&redirect_uri=redirectUri&country=JP&' +
-        `configs=authn_method%3Dsso%26sdk_platform%3Djs%26sdk_version%3D${sdkVersion}`
-    );
+    expectUrlToMatchWithPKCE(result9, {
+      baseUrl: 'https://myaccount.getmoneytree.com',
+      path: '/onboard',
+      query: authQuery
+    })
 
     const result10 = instance.logoutUrl({ backTo: 'backTo' });
     expect(result10).toBe(

diff --git a/src/api/__tests__/authorize-url.test.ts b/src/api/__tests__/authorize-url.test.ts
@@ -1,11 +1,11 @@
-import qs from 'qs';
 import { mocked } from 'ts-jest/utils';
 
 import { MY_ACCOUNT_DOMAINS } from '../../server-paths';
 import { MtLinkSdk } from '../..';
 import authorizeUrl from '../authorize-url';
 import { generateConfigs } from '../../helper';
 import storage from '../../storage';
+import expectUrlToMatchWithPKCE from '../../__tests__/helper/expect-url-to-match';
 
 jest.mock('../../storage');
 
@@ -53,7 +53,7 @@ describe('api', () => {
 
       const url = authorizeUrl(mtLinkSdk.storedOptions);
 
-      const query = qs.stringify({
+      const query = {
         client_id: clientId,
         cobrand_client_id: cobrandClientId,
         response_type: 'code',
@@ -63,9 +63,8 @@ describe('api', () => {
         locale,
         saml_subject_id: samlSubjectId,
         configs: generateConfigs()
-      });
-
-      expect(url).toBe(`${MY_ACCOUNT_DOMAINS.production}/oauth/authorize?${query}`);
+      };
+      expectUrlToMatchWithPKCE(url, {baseUrl: MY_ACCOUNT_DOMAINS.production, path: '/oauth/authorize', query})
     });
 
     test('with options', () => {
@@ -85,7 +84,7 @@ describe('api', () => {
         scopes
       });
 
-      const query = qs.stringify({
+      const query = {
         client_id: clientId,
         response_type: 'code',
         scope: scopes,
@@ -94,8 +93,8 @@ describe('api', () => {
         country,
         saml_subject_id: samlSubjectId,
         configs: generateConfigs()
-      });
-      expect(url).toBe(`${MY_ACCOUNT_DOMAINS.production}/oauth/authorize?${query}`);
+      }
+      expectUrlToMatchWithPKCE(url, {baseUrl: MY_ACCOUNT_DOMAINS.production, path: '/oauth/authorize', query})
     });
   });
 });
diff --git a/src/api/__tests__/authorize.test.ts b/src/api/__tests__/authorize.test.ts
@@ -1,11 +1,11 @@
-import qs from 'qs';
 import { mocked } from 'ts-jest/utils';
 
 import { MY_ACCOUNT_DOMAINS } from '../../server-paths';
 import { MtLinkSdk } from '../..';
 import authorize from '../authorize';
 import { generateConfigs } from '../../helper';
 import storage from '../../storage';
+import expectUrlToMatchWithPKCE from '../../__tests__/helper/expect-url-to-match';
 
 jest.mock('../../storage');
 
@@ -57,8 +57,9 @@ describe('api', () => {
       authorize(mtLinkSdk.storedOptions);
 
       expect(open).toBeCalledTimes(1);
-
-      const query = qs.stringify({
+      expect(open).toBeCalledWith(expect.any(String), '_self', 'noreferrer');
+      const url = open.mock.calls[0][0]
+      const query = {
         client_id: clientId,
         cobrand_client_id: cobrandClientId,
         response_type: 'code',
@@ -68,9 +69,8 @@ describe('api', () => {
         locale,
         saml_subject_id: samlSubjectId,
         configs: generateConfigs()
-      });
-      const url = `${MY_ACCOUNT_DOMAINS.production}/oauth/authorize?${query}`;
-      expect(open).toBeCalledWith(url, '_self', 'noreferrer');
+      };
+      expectUrlToMatchWithPKCE(url, {baseUrl: MY_ACCOUNT_DOMAINS.production, path: '/oauth/authorize', query })
     });
 
     test('with options', () => {
@@ -92,8 +92,9 @@ describe('api', () => {
       });
 
       expect(open).toBeCalledTimes(1);
-
-      const query = qs.stringify({
+      expect(open).toBeCalledWith(expect.any(String), '_self', 'noreferrer');
+      const url = open.mock.calls[0][0]
+      const query = {
         client_id: clientId,
         response_type: 'code',
         scope: scopes,
@@ -102,9 +103,8 @@ describe('api', () => {
         country,
         saml_subject_id: samlSubjectId,
         configs: generateConfigs()
-      });
-      const url = `${MY_ACCOUNT_DOMAINS.production}/oauth/authorize?${query}`;
-      expect(open).toBeCalledWith(url, '_self', 'noreferrer');
+      };
+      expectUrlToMatchWithPKCE(url, {baseUrl: MY_ACCOUNT_DOMAINS.production, path: '/oauth/authorize', query })
     });
 
     test('without window', () => {

diff --git a/src/api/__tests__/onboard-url.test.ts b/src/api/__tests__/onboard-url.test.ts
@@ -6,6 +6,7 @@ import { MtLinkSdk } from '../..';
 import onboardUrl from '../onboard-url';
 import { generateConfigs } from '../../helper';
 import storage from '../../storage';
+import expectUrlToMatchWithPKCE from '../../__tests__/helper/expect-url-to-match';
 
 jest.mock('../../storage');
 
@@ -53,7 +54,7 @@ describe('api', () => {
 
       const url = onboardUrl(mtLinkSdk.storedOptions);
 
-      const query = qs.stringify({
+      const query = {
         client_id: clientId,
         cobrand_client_id: cobrandClientId,
         response_type: 'code',
@@ -62,9 +63,9 @@ describe('api', () => {
         country,
         locale,
         configs: generateConfigs({ email })
-      });
+      };
 
-      expect(url).toBe(`${MY_ACCOUNT_DOMAINS.production}/onboard?${query}`);
+      expectUrlToMatchWithPKCE(url, {baseUrl: MY_ACCOUNT_DOMAINS.production, path: '/onboard', query: query })
     });
 
     test('with options', () => {
@@ -78,23 +79,24 @@ describe('api', () => {
       mtLinkSdk.init(clientId);
 
       const url = onboardUrl(mtLinkSdk.storedOptions, {
-        state,
-        redirectUri,
-        scopes,
-        email
-      });
+          state,
+          redirectUri,
+          scopes,
+          email
+        }
+      );
 
-      const query = qs.stringify({
+      const query = {
         client_id: clientId,
         response_type: 'code',
         scope: scopes,
         redirect_uri: redirectUri,
         state,
         country,
         configs: generateConfigs({ email })
-      });
+      };
 
-      expect(url).toBe(`${MY_ACCOUNT_DOMAINS.production}/onboard?${query}`);
+      expectUrlToMatchWithPKCE(url, {baseUrl: MY_ACCOUNT_DOMAINS.production, path: '/onboard', query})
     });
   });
 });
diff --git a/src/api/__tests__/onboard.test.ts b/src/api/__tests__/onboard.test.ts
@@ -1,11 +1,11 @@
-import qs from 'qs';
 import { mocked } from 'ts-jest/utils';
 
 import { MY_ACCOUNT_DOMAINS } from '../../server-paths';
 import { MtLinkSdk } from '../..';
 import onboard from '../onboard';
 import { generateConfigs } from '../../helper';
 import storage from '../../storage';
+import expectUrlToMatchWithPKCE from '../../__tests__/helper/expect-url-to-match';
 
 jest.mock('../../storage');
 
@@ -57,8 +57,9 @@ describe('api', () => {
       onboard(mtLinkSdk.storedOptions);
 
       expect(open).toBeCalledTimes(1);
-
-      const query = qs.stringify({
+      expect(open).toBeCalledWith(expect.any(String), '_self', 'noreferrer');
+      const url = open.mock.calls[0][0]
+      const query = {
         client_id: clientId,
         cobrand_client_id: cobrandClientId,
         response_type: 'code',
@@ -67,9 +68,8 @@ describe('api', () => {
         country,
         locale,
         configs: generateConfigs({ email })
-      });
-      const url = `${MY_ACCOUNT_DOMAINS.production}/onboard?${query}`;
-      expect(open).toBeCalledWith(url, '_self', 'noreferrer');
+      };
+      expectUrlToMatchWithPKCE(url, {baseUrl: MY_ACCOUNT_DOMAINS.production, path: '/onboard', query})
     });
 
     test('with options', () => {
@@ -91,18 +91,19 @@ describe('api', () => {
       });
 
       expect(open).toBeCalledTimes(1);
-
-      const query = qs.stringify({
+      expect(open).toBeCalledWith(expect.any(String), '_self', 'noreferrer');
+      const url = open.mock.calls[0][0]
+      const query = {
         client_id: clientId,
         response_type: 'code',
         scope: scopes,
         redirect_uri: redirectUri,
         state,
         country,
         configs: generateConfigs({ email })
-      });
-      const url = `${MY_ACCOUNT_DOMAINS.production}/onboard?${query}`;
-      expect(open).toBeCalledWith(url, '_self', 'noreferrer');
+      };
+
+      expectUrlToMatchWithPKCE(url, {baseUrl: MY_ACCOUNT_DOMAINS.production, path: '/onboard', query})
     });
 
     test('without window', () => {

diff --git a/src/api/authorize-url.ts b/src/api/authorize-url.ts
@@ -20,14 +20,7 @@ export default function authorize(storedOptions: StoredOptions, options: Authori
     throw new Error('[mt-link-sdk] Make sure to call `init` before calling `authorizeUrl/authorize`.');
   }
 
-  const {
-    scopes = defaultScopes,
-    redirectUri = defaultRedirectUri,
-    pkce = false,
-    codeChallenge,
-    state,
-    ...rest
-  } = options;
+  const { scopes = defaultScopes, redirectUri = defaultRedirectUri, codeChallenge, state, ...rest } = options;
 
   if (!redirectUri) {
     throw new Error(
@@ -37,7 +30,7 @@ export default function authorize(storedOptions: StoredOptions, options: Authori
 
   storage.del('cv');
 
-  const cc = codeChallenge || (pkce && generateCodeChallenge());
+  const cc = codeChallenge || generateCodeChallenge();
 
   const queryString = stringify({
     client_id: clientId,

diff --git a/src/api/onboard-url.ts b/src/api/onboard-url.ts
@@ -19,14 +19,7 @@ export default function onboardUrl(storedOptions: StoredOptions, options: Onboar
     throw new Error('[mt-link-sdk] Make sure to call `init` before calling `onboardUrl/onboard`.');
   }
 
-  const {
-    scopes = defaultScopes,
-    redirectUri = defaultRedirectUri,
-    pkce = false,
-    codeChallenge,
-    state,
-    ...rest
-  } = options;
+  const { scopes = defaultScopes, redirectUri = defaultRedirectUri, codeChallenge, state, ...rest } = options;
 
   const configs = mergeConfigs(storedOptions, rest, ['authAction', 'showAuthToggle', 'showRememberMe', 'forceLogout']);
 
@@ -38,7 +31,7 @@ export default function onboardUrl(storedOptions: StoredOptions, options: Onboar
 
   storage.del('cv');
 
-  const cc = codeChallenge || (pkce && generateCodeChallenge());
+  const cc = codeChallenge || generateCodeChallenge();
 
   const queryString = stringify({
     client_id: clientId,