Add option to disable state verification, fix redirect uri bug #1116

srajiang · 2021-09-16T00:36:59Z

Summary

See both for more context.

It adds the option to disable state verification during OAuth flow via a stateVerification flag. By default this will be true. When state verification is disabled, stateSecret is no longer required, so this updates error messages associated with that previous hard requirement.

This PR also fixes an issue where the user supplied redirect_uri is not properly being passed as part of oauth/v2/authorize request params. It adds a separate top-level redirectUri parameter that contains the full url and includes some input validation.

Todo

Add tests for redirectUri configuration error states
Add test for stateVerification flow (needs new types pulled in)
Prep documentation PR to follow here
node-slack-sdk/pull/1329 should be released in node-slack-sdk.

Requirements (place an `x` in each `[ ]`)

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.

docs/_basic/authenticating_oauth.md

src/receivers/HTTPReceiver.ts

codecov · 2021-09-24T00:36:14Z

Codecov Report

Merging #1116 (daf5cee) into main (2f3f499) will increase coverage by 0.79%.
The diff coverage is 86.04%.

@@            Coverage Diff             @@
##             main    #1116      +/-   ##
==========================================
+ Coverage   70.92%   71.71%   +0.79%     
==========================================
  Files          14       15       +1     
  Lines        1324     1354      +30     
  Branches      392      402      +10     
==========================================
+ Hits          939      971      +32     
- Misses        311      312       +1     
+ Partials       74       71       -3

Impacted Files	Coverage Δ
src/receivers/ExpressReceiver.ts	`60.39% <60.00%> (+1.93%)`	⬆️
src/receivers/verify-redirect-opts.ts	`83.33% <83.33%> (ø)`
src/receivers/SocketModeReceiver.ts	`89.02% <93.33%> (+4.09%)`	⬆️
src/App.ts	`83.64% <100.00%> (+0.04%)`	⬆️
src/receivers/HTTPReceiver.ts	`37.12% <100.00%> (+2.25%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f3f499...daf5cee. Read the comment docs.

srajiang

Updated changes include:

Moved docs changes to separate PR.
Utility verification function that throws an error when custom redirect options are incomplete or invalid + tests for those scenarios at App and Receiver levels
Tests to check that install options are being directly handed over to handleCallback when stateVerification is disabled.
Did some reorganization of SocketModeReceiver and HTTPReceiver tests sections for readability.

Thanks in advance for your 👀 - I will squash and merge when the PR is ready!

srajiang · 2021-09-23T23:38:15Z

src/receivers/HTTPReceiver.spec.ts

@@ -117,217 +117,344 @@ describe('HTTPReceiver', function () {
      });


I have added additional describe sections and reorganized some of the tests here and also in SocketModeReceiver.spec.ts to make the test files more consistent with one another and to make navigating test files / reading test output a little more clear.

Note: I attempted to incorporate state validation related tests for this, but ran into an issue with the requestHandler being a private function - wasn't sure how to really set up a fake request. I see that we don't really have the same test coverage for ExpressReceiver that we do for the other receivers and I'm wondering if that's a known limitation.

seratch

I left one comment. As long as your Slack app has only one Redirect URI in its settings, skipping to have redirect_uri parameter should be allowed. Also, if we add the validation about the existence of redirectUri as a requirement, it'll be a breaking change to many of the existing OAuth ebaled apps. They will need updates when upgrading bolt-js version.

seratch · 2021-09-24T08:25:43Z

src/receivers/ExpressReceiver.spec.ts

+        scopes,
+        redirectUri,
+      }), AppInitializationError);
+      // missing redirectUri


Not setting redirectUri should be allowed. You can initiate the OAuth flow without redirect_uri parameter when your Slack app has only one redirect_uri setting.

I won't repeat the same comment for HTTPReceiver and SocketModeReceiver.

Good catch. I have modified the check so now it makes sure that if redirectUri is supplied, redirectUriPath is required only.

seratch · 2021-09-24T08:27:39Z

src/receivers/verify-redirect-opts.ts

+  redirectUriPath?: HTTPReceiverInstallerOptions['redirectUriPath'],
+}
+
+export function verifyRedirectOpts({ redirectUri, redirectUriPath }: RedirectOptions): void {


I added a comment on the corresponding test code side but missing only redirectUri should be okay.

seratch

LGTM 👍

seratch · 2021-09-24T23:06:53Z

Hey, other reviewers! This one is the last pull request waiting for merging in the v3.7.0 release milestone: https://github.com/slackapi/bolt-js/milestone/11 If you have any comments on this PR, submit your reviews by next ~~Monday~~ your Tuesday!

@srajiang Can I ask you to work on the release early next week?

filmaj

LGTM! Left a couple minor comments.

src/receivers/HTTPReceiver.ts

src/receivers/HTTPReceiver.spec.ts

Co-authored-by: Fil Maj <maj.fil@gmail.com>

srajiang added 3 commits September 7, 2021 14:52

Add

4506c4d

Add stateValidation opt

c938755

Correct documentation

0ab1439

srajiang mentioned this pull request Sep 16, 2021

Add optional InstallProvider#stateVerification flag slackapi/node-slack-sdk#1329

Merged

6 tasks

srajiang requested review from misscoded, seratch, stevengill and filmaj September 16, 2021 00:47

Merge branch 'main' into 1101_add_state_validation_opt

d95dbf9

seratch requested changes Sep 16, 2021

View reviewed changes

docs/_basic/authenticating_oauth.md Outdated Show resolved Hide resolved

stevengill reviewed Sep 16, 2021

View reviewed changes

src/receivers/HTTPReceiver.ts Outdated Show resolved Hide resolved

srajiang added 2 commits September 16, 2021 11:23

Correct docs + add top-levelfield

5ceced2

Correct comments

85bd440

seratch added the enhancement M-T: A feature request for new functionality label Sep 17, 2021

seratch added this to the 3.7.0 milestone Sep 17, 2021

srajiang added 3 commits September 17, 2021 11:58

Add receiver support

7083d6b

Tidy

355f85b

Tidy

2aa5498

srajiang self-assigned this Sep 17, 2021

srajiang added 4 commits September 20, 2021 11:23

Add state verification opt to installurl gen

d95677a

Update installProvider instantiation logic

26a9ed8

Add verify redirect helper

2f154ad

Pass state verification to generateInstallUrl

78a2f60

srajiang mentioned this pull request Sep 21, 2021

Clarify oauth setup requirements and customization options #1122

Merged

2 tasks

srajiang added 2 commits September 22, 2021 11:29

Merge branch 'main' into 1101_add_state_validation_opt

3355e6d

Fix additional merge conflicts

dc8fe2e

srajiang force-pushed the 1101_add_state_validation_opt branch from f5e4c69 to beb3d57 Compare September 23, 2021 23:46

srajiang changed the title ~~[WIP] Add option to disable state verification~~ Add option to disable state verification, fix redirect uri bug Sep 23, 2021

srajiang added the semver:minor label Sep 23, 2021

srajiang force-pushed the 1101_add_state_validation_opt branch from beb3d57 to 79a77cd Compare September 24, 2021 00:34

srajiang commented Sep 24, 2021

View reviewed changes

srajiang force-pushed the 1101_add_state_validation_opt branch 2 times, most recently from cf22ddc to 283f6b6 Compare September 24, 2021 00:49

srajiang marked this pull request as ready for review September 24, 2021 00:56

srajiang requested review from seratch and stevengill September 24, 2021 00:56

seratch reviewed Sep 24, 2021

View reviewed changes

srajiang force-pushed the 1101_add_state_validation_opt branch 2 times, most recently from 08b9f87 to efc5e17 Compare September 24, 2021 18:10

Add redirect uri opts validation + stateVerification tests

f053a34

srajiang force-pushed the 1101_add_state_validation_opt branch from efc5e17 to f053a34 Compare September 24, 2021 18:12

srajiang requested a review from seratch September 24, 2021 18:14

seratch approved these changes Sep 24, 2021

View reviewed changes

Merge branch 'main' into 1101_add_state_validation_opt

d6ffd59

filmaj approved these changes Sep 27, 2021

View reviewed changes

src/receivers/HTTPReceiver.ts Outdated Show resolved Hide resolved

src/receivers/HTTPReceiver.spec.ts Outdated Show resolved Hide resolved

srajiang and others added 2 commits September 27, 2021 11:14

Update src/receivers/HTTPReceiver.ts

cf9ff66

Co-authored-by: Fil Maj <maj.fil@gmail.com>

Update src/receivers/HTTPReceiver.spec.ts

daf5cee

Co-authored-by: Fil Maj <maj.fil@gmail.com>

srajiang merged commit 1c1eb57 into slackapi:main Sep 27, 2021

srajiang deleted the 1101_add_state_validation_opt branch September 27, 2021 18:18

hello-ashleyintech mentioned this pull request Nov 3, 2022

redirectUri not being passed into installUrlOptions #1115

Closed

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to disable state verification, fix redirect uri bug #1116

Add option to disable state verification, fix redirect uri bug #1116

srajiang commented Sep 16, 2021 •

edited by seratch

Loading

codecov bot commented Sep 24, 2021 •

edited

Loading

srajiang left a comment •

edited

Loading

srajiang Sep 23, 2021

srajiang Sep 24, 2021

seratch left a comment

seratch Sep 24, 2021

srajiang Sep 24, 2021

seratch Sep 24, 2021

seratch left a comment

seratch commented Sep 24, 2021 •

edited

Loading

filmaj left a comment

		@@ -117,217 +117,344 @@ describe('HTTPReceiver', function () {
		});

Add option to disable state verification, fix redirect uri bug #1116

Add option to disable state verification, fix redirect uri bug #1116

Conversation

srajiang commented Sep 16, 2021 • edited by seratch Loading

Summary

Requirements (place an x in each [ ])

codecov bot commented Sep 24, 2021 • edited Loading

Codecov Report

srajiang left a comment • edited Loading

Choose a reason for hiding this comment

srajiang Sep 23, 2021

Choose a reason for hiding this comment

srajiang Sep 24, 2021

Choose a reason for hiding this comment

seratch left a comment

Choose a reason for hiding this comment

seratch Sep 24, 2021

Choose a reason for hiding this comment

srajiang Sep 24, 2021

Choose a reason for hiding this comment

seratch Sep 24, 2021

Choose a reason for hiding this comment

seratch left a comment

Choose a reason for hiding this comment

seratch commented Sep 24, 2021 • edited Loading

filmaj left a comment

Choose a reason for hiding this comment

srajiang commented Sep 16, 2021 •

edited by seratch

Loading

Requirements (place an `x` in each `[ ]`)

codecov bot commented Sep 24, 2021 •

edited

Loading

srajiang left a comment •

edited

Loading

seratch commented Sep 24, 2021 •

edited

Loading