Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify oauth setup requirements and customization options #1122

Merged
merged 3 commits into from
Sep 27, 2021

Conversation

srajiang
Copy link
Member

Summary

Updates Oauth documentation to be consistent with changes in bolt-js/pull//#1116. Adjusts formatting for better readability and adds clarifying language to concepts (Redirect URI vs. Redirect URI path) etc.

Requirements (place an x in each [ ])

@@ -27,7 +68,7 @@ const app = new App({
clientId: process.env.SLACK_CLIENT_ID,
clientSecret: process.env.SLACK_CLIENT_SECRET,
stateSecret: 'my-state-secret',
scopes: ['channels:read', 'groups:read', 'channels:manage', 'chat:write', 'incoming-webhook'],
scopes: ['chat:write', 'incoming-webhook'],
Copy link
Member Author

Choose a reason for hiding this comment

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

Doesn't need so many scopes. It's already a very cluttered code sample.

Copy link
Member

Choose a reason for hiding this comment

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

I agree. If we revisit this part, we may want to have commands and chat:write instead because that's a common minimum set for Bolt apps (you use say(), shortcuts, slash commands etc. but don't use incoming webhooks with Bolt, right?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure thing - I've modified the scopes and also adjusted the comments in the storeInstallation bit.

@srajiang srajiang requested review from seratch, stevengill and filmaj and removed request for seratch and stevengill September 21, 2021 00:38
@srajiang srajiang added the docs M-T: Documentation work only label Sep 21, 2021
@srajiang srajiang added this to the 3.7.0 milestone Sep 21, 2021
@srajiang srajiang self-assigned this Sep 21, 2021
@srajiang srajiang requested a review from misscoded September 21, 2021 00:39
@srajiang srajiang changed the title Update oauth docs Clarify oauth setup requirements and customization options Sep 21, 2021
@srajiang srajiang removed this from the 3.7.0 milestone Sep 21, 2021
clientId: process.env.SLACK_CLIENT_ID,
clientSecret: process.env.SLACK_CLIENT_SECRET,
scopes: ['chat:write'],
stateVerification: false,
Copy link
Member

Choose a reason for hiding this comment

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

We don't want to highlight this way (=as the first example code in the section). Can we we move this to the bottom of the section?

Copy link
Member Author

@srajiang srajiang Sep 21, 2021

Choose a reason for hiding this comment

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

Yes, I agree that having it appear as the first code sample in the section may not be great. More than that it's an org-wide app installation related section so it probably makes more sense in it's own section under a new heading Org-wide installation. I've added that to the latest changes.

@@ -27,7 +68,7 @@ const app = new App({
clientId: process.env.SLACK_CLIENT_ID,
clientSecret: process.env.SLACK_CLIENT_SECRET,
stateSecret: 'my-state-secret',
scopes: ['channels:read', 'groups:read', 'channels:manage', 'chat:write', 'incoming-webhook'],
scopes: ['chat:write', 'incoming-webhook'],
Copy link
Member

Choose a reason for hiding this comment

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

I agree. If we revisit this part, we may want to have commands and chat:write instead because that's a common minimum set for Bolt apps (you use say(), shortcuts, slash commands etc. but don't use incoming webhooks with Bolt, right?)

@codecov
Copy link

codecov bot commented Sep 21, 2021

Codecov Report

Merging #1122 (a51c65f) into main (9303ab0) will decrease coverage by 0.79%.
The diff coverage is n/a.

❗ Current head a51c65f differs from pull request most recent head 7506067. Consider uploading reports for the commit 7506067 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1122      +/-   ##
==========================================
- Coverage   71.71%   70.92%   -0.80%     
==========================================
  Files          15       14       -1     
  Lines        1354     1324      -30     
  Branches      402      392      -10     
==========================================
- Hits          971      939      -32     
+ Misses        312      311       -1     
- Partials       71       74       +3     
Impacted Files Coverage Δ
src/receivers/SocketModeReceiver.ts 84.93% <0.00%> (-4.10%) ⬇️
src/receivers/HTTPReceiver.ts 34.87% <0.00%> (-2.26%) ⬇️
src/receivers/ExpressReceiver.ts 58.46% <0.00%> (-1.94%) ⬇️
src/App.ts 83.60% <0.00%> (-0.05%) ⬇️
src/receivers/verify-redirect-opts.ts

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 9303ab0...7506067. Read the comment docs.

@srajiang srajiang force-pushed the 1101_add_state_validation_docs branch from f1d7e95 to eae1b3f Compare September 21, 2021 16:56
Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

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

I found two code snippet errors. Apart from them, looks good to me. We can merge this PR after v3.7 release.

docs/_basic/authenticating_oauth.md Outdated Show resolved Hide resolved
docs/_basic/authenticating_oauth.md Outdated Show resolved Hide resolved
@seratch seratch added this to the 3.7.0 milestone Sep 22, 2021
@srajiang srajiang force-pushed the 1101_add_state_validation_docs branch from a51c65f to 7506067 Compare September 27, 2021 20:18
@srajiang srajiang merged commit 98ae2f2 into slackapi:main Sep 27, 2021
@srajiang srajiang deleted the 1101_add_state_validation_docs branch September 27, 2021 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs M-T: Documentation work only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants