From 81c5f7d6cb067eac157b43fca1625ba64855c254 Mon Sep 17 00:00:00 2001 From: Steve Hetzel Date: Thu, 7 Mar 2024 12:35:03 -0700 Subject: [PATCH] fix: re-enable code challenge support --- src/org/authInfo.ts | 6 +++-- src/webOAuthServer.ts | 3 +++ test/unit/webOauthServerTest.ts | 46 ++++++++++++++++++++++++++++++++- 3 files changed, 52 insertions(+), 3 deletions(-) diff --git a/src/org/authInfo.ts b/src/org/authInfo.ts index d0a1fc6e1..2f7e3b841 100644 --- a/src/org/authInfo.ts +++ b/src/org/authInfo.ts @@ -333,8 +333,10 @@ export class AuthInfo extends AsyncOptionalCreatable { * @param options The options to generate the URL. */ public static getAuthorizationUrl(options: JwtOAuth2Config & { scope?: string }, oauth2?: OAuth2): string { - // Always use a verifier for enhanced security - options.useVerifier = true; + // Unless explicitly turned off, use a code verifier for enhanced security + if (options.useVerifier !== false) { + options.useVerifier = true; + } const oauth2Verifier = oauth2 ?? new OAuth2(options); // The state parameter allows the redirectUri callback listener to ignore request diff --git a/src/webOAuthServer.ts b/src/webOAuthServer.ts index 7695bb82a..88e8f4f91 100644 --- a/src/webOAuthServer.ts +++ b/src/webOAuthServer.ts @@ -141,6 +141,8 @@ export class WebOAuthServer extends AsyncCreatable { if (!this.oauthConfig.clientId) this.oauthConfig.clientId = DEFAULT_CONNECTED_APP_INFO.clientId; if (!this.oauthConfig.loginUrl) this.oauthConfig.loginUrl = AuthInfo.getDefaultInstanceUrl(); if (!this.oauthConfig.redirectUri) this.oauthConfig.redirectUri = `http://localhost:${port}/OauthRedirect`; + // Unless explicitly turned off, use a code verifier as a best practice + if (this.oauthConfig.useVerifier !== false) this.oauthConfig.useVerifier = true; this.webServer = await WebServer.create({ port }); this.oauth2 = new OAuth2(this.oauthConfig); @@ -237,6 +239,7 @@ export class WebOAuthServer extends AsyncCreatable { this.logger.debug(`oauthConfig.loginUrl: ${this.oauthConfig.loginUrl}`); this.logger.debug(`oauthConfig.clientId: ${this.oauthConfig.clientId}`); this.logger.debug(`oauthConfig.redirectUri: ${this.oauthConfig.redirectUri}`); + this.logger.debug(`oauthConfig.useVerifier: ${this.oauthConfig.useVerifier}`); return authCode; } return null; diff --git a/test/unit/webOauthServerTest.ts b/test/unit/webOauthServerTest.ts index 720c40942..1ba9008d9 100644 --- a/test/unit/webOauthServerTest.ts +++ b/test/unit/webOauthServerTest.ts @@ -35,11 +35,24 @@ describe('WebOauthServer', () => { }); describe('getAuthorizationUrl', () => { - it('should return authorization url', async () => { + it('should return authorization url with expected (default) params', async () => { const oauthServer = await WebOAuthServer.create({ oauthConfig: {} }); const authUrl = oauthServer.getAuthorizationUrl(); expect(authUrl).to.not.be.undefined; expect(authUrl).to.include('client_id=PlatformCLI'); + expect(authUrl).to.include('code_challenge='); + expect(authUrl).to.include('state='); + expect(authUrl).to.include('response_type=code'); + }); + + it('should return authorization url with no code_challenge', async () => { + const oauthServer = await WebOAuthServer.create({ oauthConfig: { useVerifier: false } }); + const authUrl = oauthServer.getAuthorizationUrl(); + expect(authUrl).to.not.be.undefined; + expect(authUrl).to.include('client_id=PlatformCLI'); + expect(authUrl).to.not.include('code_challenge='); + expect(authUrl).to.include('state='); + expect(authUrl).to.include('response_type=code'); }); }); @@ -70,6 +83,37 @@ describe('WebOauthServer', () => { const handleSuccessStub = stubMethod($$.SANDBOX, oauthServer.webServer, 'handleSuccess').resolves(); const authInfo = await oauthServer.authorizeAndSave(); expect(authInfoStub.save.callCount).to.equal(1); + expect(authStub.callCount).to.equal(1); + const authCreateOptions = authStub.firstCall.args[0] as AuthInfo.Options; + expect(authCreateOptions).have.property('oauth2Options'); + expect(authCreateOptions).have.property('oauth2'); + expect(authCreateOptions.oauth2Options).to.have.property('useVerifier', true); + expect(authCreateOptions.oauth2Options).to.have.property('clientId', 'PlatformCLI'); + expect(authCreateOptions.oauth2).to.have.property('codeVerifier'); + expect(authCreateOptions.oauth2?.codeVerifier).to.have.length.greaterThan(1); + expect(authCreateOptions.oauth2).to.have.property('clientId', 'PlatformCLI'); + expect(authInfo.getFields()).to.deep.equal(authFields); + expect(handleSuccessStub.calledOnce).to.be.true; + }); + + it('should save new AuthInfo without codeVerifier', async () => { + redirectStub = stubMethod($$.SANDBOX, WebServer.prototype, 'doRedirect').callsFake(async () => {}); + stubMethod($$.SANDBOX, WebOAuthServer.prototype, 'executeOauthRequest').callsFake(async () => serverResponseStub); + const oauthServer = await WebOAuthServer.create({ oauthConfig: { useVerifier: false } }); + await oauthServer.start(); + // @ts-expect-error because private member + const handleSuccessStub = stubMethod($$.SANDBOX, oauthServer.webServer, 'handleSuccess').resolves(); + const authInfo = await oauthServer.authorizeAndSave(); + expect(authInfoStub.save.callCount).to.equal(1); + expect(authStub.callCount).to.equal(1); + const authCreateOptions = authStub.firstCall.args[0] as AuthInfo.Options; + expect(authCreateOptions).have.property('oauth2Options'); + expect(authCreateOptions).have.property('oauth2'); + expect(authCreateOptions.oauth2Options).to.have.property('useVerifier', false); + expect(authCreateOptions.oauth2Options).to.have.property('clientId', 'PlatformCLI'); + expect(authCreateOptions.oauth2).to.have.property('codeVerifier'); + expect(authCreateOptions.oauth2?.codeVerifier).to.be.undefined; + expect(authCreateOptions.oauth2).to.have.property('clientId', 'PlatformCLI'); expect(authInfo.getFields()).to.deep.equal(authFields); expect(handleSuccessStub.calledOnce).to.be.true; });