Skip to content

Commit

Permalink
fix: re-enable code challenge support
Browse files Browse the repository at this point in the history
  • Loading branch information
shetzel committed Mar 7, 2024
1 parent 5ae6122 commit 81c5f7d
Show file tree
Hide file tree
Showing 3 changed files with 52 additions and 3 deletions.
6 changes: 4 additions & 2 deletions src/org/authInfo.ts
Original file line number Diff line number Diff line change
Expand Up @@ -333,8 +333,10 @@ export class AuthInfo extends AsyncOptionalCreatable<AuthInfo.Options> {
* @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
Expand Down
3 changes: 3 additions & 0 deletions src/webOAuthServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,8 @@ export class WebOAuthServer extends AsyncCreatable<WebOAuthServer.Options> {
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);
Expand Down Expand Up @@ -237,6 +239,7 @@ export class WebOAuthServer extends AsyncCreatable<WebOAuthServer.Options> {
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;
Expand Down
46 changes: 45 additions & 1 deletion test/unit/webOauthServerTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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');
});
});

Expand Down Expand Up @@ -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;
});
Expand Down

3 comments on commit 81c5f7d

@svc-cli-bot
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logger Benchmarks - ubuntu-latest

Benchmark suite Current: 81c5f7d Previous: 932153e Ratio
Child logger creation 479584 ops/sec (±1.54%) 468660 ops/sec (±1.74%) 0.98
Logging a string on root logger 795800 ops/sec (±11.74%) 798951 ops/sec (±8.59%) 1.00
Logging an object on root logger 630387 ops/sec (±6.03%) 596105 ops/sec (±8.11%) 0.95
Logging an object with a message on root logger 5098 ops/sec (±212.84%) 6272 ops/sec (±213.16%) 1.23
Logging an object with a redacted prop on root logger 411871 ops/sec (±13.95%) 399307 ops/sec (±19.83%) 0.97
Logging a nested 3-level object on root logger 391081 ops/sec (±6.65%) 380136 ops/sec (±6.53%) 0.97

This comment was automatically generated by workflow using github-action-benchmark.

@svc-cli-bot
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logger Benchmarks - windows-latest

Benchmark suite Current: 81c5f7d Previous: 932153e Ratio
Child logger creation 351826 ops/sec (±0.52%) 336593 ops/sec (±3.03%) 0.96
Logging a string on root logger 872805 ops/sec (±6.67%) 810185 ops/sec (±5.74%) 0.93
Logging an object on root logger 644136 ops/sec (±7.78%) 640509 ops/sec (±6.72%) 0.99
Logging an object with a message on root logger 2329 ops/sec (±255.67%) 4939 ops/sec (±211.64%) 2.12
Logging an object with a redacted prop on root logger 475122 ops/sec (±6.35%) 459611 ops/sec (±14.15%) 0.97
Logging a nested 3-level object on root logger 344838 ops/sec (±4.69%) 318490 ops/sec (±7.66%) 0.92

This comment was automatically generated by workflow using github-action-benchmark.

@svc-cli-bot
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Logger Benchmarks - windows-latest'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.

Benchmark suite Current: 81c5f7d Previous: 932153e Ratio
Logging an object with a message on root logger 2329 ops/sec (±255.67%) 4939 ops/sec (±211.64%) 2.12

This comment was automatically generated by workflow using github-action-benchmark.

Please sign in to comment.