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

Fix #689 by adding flags in AppConfig #690

Merged
merged 4 commits into from
Feb 16, 2021

Conversation

seratch
Copy link
Member

@seratch seratch commented Feb 12, 2021

This pull request fixes #689 by adding flags in AppConfig. Refer to the issue for the context.

We do not usually recommend turning the built-in middleware off. Especially, disabling request verification just for easiness is not great from security perspective. These flags are available only for the use case like #689 or debugging purposes.

Category (place an x in each of the [ ])

  • bolt (Bolt for Java)
  • bolt-{sub modules} (Bolt for Java - optional modules)
  • slack-api-client (Slack API Clients)
  • slack-api-model (Slack API Data Models)
  • slack-api-*-kotlin-extension (Kotlin Extensions for Slack API Clients)
  • slack-app-backend (The primitive layer of Bolt for Java)

Requirements

Please read the Contributing guidelines and Code of Conduct before creating this issue or pull request. By submitting, you are agreeing to the those rules.

@seratch seratch added enhancement M-T: A feature request for new functionality project:bolt labels Feb 12, 2021
@seratch seratch added this to the 1.6.1 milestone Feb 12, 2021
@seratch seratch self-assigned this Feb 12, 2021
@codecov
Copy link

codecov bot commented Feb 12, 2021

Codecov Report

Merging #690 (6eac7a9) into main (22f36e7) will increase coverage by 0.03%.
The diff coverage is 81.25%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #690      +/-   ##
============================================
+ Coverage     77.80%   77.83%   +0.03%     
- Complexity     3216     3222       +6     
============================================
  Files           359      359              
  Lines          9464     9467       +3     
  Branches        892      895       +3     
============================================
+ Hits           7363     7369       +6     
+ Misses         1561     1560       -1     
+ Partials        540      538       -2     
Impacted Files Coverage Δ Complexity Δ
...lt/src/main/java/com/slack/api/bolt/AppConfig.java 72.34% <ø> (ø) 25.00 <0.00> (ø)
bolt/src/main/java/com/slack/api/bolt/App.java 65.33% <81.25%> (+0.20%) 120.00 <1.00> (+4.00)
...lt/middleware/builtin/SingleTeamAuthorization.java 92.30% <0.00%> (+3.84%) 15.00% <0.00%> (+1.00%)
...om/slack/api/bolt/middleware/builtin/SSLCheck.java 88.23% <0.00%> (+5.88%) 6.00% <0.00%> (+1.00%)

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 0d9be43...a7ddde3. Read the comment docs.

@luispollo
Copy link

@seratch Thank you for picking this up so quickly! ❤️

The PR looks good to me! My only suggestion would be to update the middleware documentation to add a note about the ability to disable default middleware, which would be a good place to add a warning about the fact that disabling request verification is not recommended for security reasons, as you mentioned in the description.

@seratch seratch force-pushed the issue-689-default-middleware-config branch from 5e817f2 to 9aedef2 Compare February 13, 2021 00:45
@seratch
Copy link
Member Author

seratch commented Feb 13, 2021

@luispollo Thanks for the suggestion! I've updated the documents in this pull request ✅

Copy link
Member

@mwbrooks mwbrooks left a comment

Choose a reason for hiding this comment

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

This is a great addition. Is it something we want to bring to JavaScript and Python?

docs/guides/bolt-basics.md Outdated Show resolved Hide resolved
docs/guides/bolt-basics.md Outdated Show resolved Hide resolved
@seratch
Copy link
Member Author

seratch commented Feb 16, 2021

Is it something we want to bring to JavaScript and Python?

It may be worth considering to add these options to Python as its App has a quite similar structure. As for JavaScript, request verification is HTTPReceiver / ExpressReceiver's responsibility. Thus, if we add this, the options should be available in these receivers' constructors.

@seratch
Copy link
Member Author

seratch commented Feb 16, 2021

Thanks for the suggestions on the document 🙇

@seratch seratch merged commit e4f2309 into slackapi:main Feb 16, 2021
@seratch seratch deleted the issue-689-default-middleware-config branch February 16, 2021 22:19
@gal-yardeni
Copy link

Hi @seratch, thank you for fixing the issue for us! When it will be available for use at the SDK level?

@seratch
Copy link
Member Author

seratch commented Feb 16, 2021

@gal-yardeni We'll release a new version within a few hours!

@gal-yardeni
Copy link

that's awesome! thanks for the update!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement M-T: A feature request for new functionality project:bolt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow removing default middleware in Bolt
4 participants