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 #859 Improve the way to handle authorize fn errors in built-in receivers #891

Merged
merged 1 commit into from
Aug 28, 2021

Conversation

seratch
Copy link
Member

@seratch seratch commented Apr 22, 2021

Summary

This pull request closes #859 by improving the error handling of authorize function failures. As described here, the current implementation returns 500 Internal Server Error in the case and displays the following warn/error logs.

[WARN]  bolt-app Authorization of incoming event did not succeed. No listeners will be called.
[ERROR]  bolt-app Error: Failed fetching data from the Installation Store
    at new AuthorizationError (/path-to-app/node_modules/@slack/oauth/dist/errors.js:71:28)
    at InstallProvider.<anonymous> (/path-to-app/node_modules/@slack/oauth/dist/index.js:158:31)
    at step (/path-to-app/node_modules/@slack/oauth/dist/index.js:44:23)
    at Object.next (/path-to-app/node_modules/@slack/oauth/dist/index.js:25:53)
    at fulfilled (/path-to-app/node_modules/@slack/oauth/dist/index.js:16:58)
    at processTicksAndRejections (internal/process/task_queues.js:97:5) {
  code: 'slack_bolt_authorization_error'
}
[ERROR]   An unhandled error occurred while Bolt processed an event
[DEBUG]   Error details: Error: Failed fetching data from the Installation Store, storedResponse: undefined
[ERROR]   An incoming event was not acknowledged within 3 seconds. Ensure that the ack() argument is called in a listener.

As we recommend throwing an exception if there is no valid installation data in fetchInstallation method, this pattern should be handled as a normal case, not an internal server error.

This pull request fixes #364 too.

Requirements (place an x in each [ ])

@seratch seratch added bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented enhancement M-T: A feature request for new functionality semver:minor tests M-T: Testing work only labels Apr 22, 2021
@seratch seratch added this to the 3.4.0 milestone Apr 22, 2021
@seratch seratch requested review from mwbrooks and stevengill April 22, 2021 05:09
@seratch seratch self-assigned this Apr 22, 2021
@codecov
Copy link

codecov bot commented Apr 22, 2021

Codecov Report

Merging #891 (8c73ca6) into main (926b669) will decrease coverage by 5.52%.
The diff coverage is 23.52%.

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

@@            Coverage Diff             @@
##             main     #891      +/-   ##
==========================================
- Coverage   71.01%   65.48%   -5.53%     
==========================================
  Files          13       13              
  Lines        1235     1217      -18     
  Branches      362      359       -3     
==========================================
- Hits          877      797      -80     
- Misses        288      351      +63     
+ Partials       70       69       -1     
Impacted Files Coverage Δ
src/receivers/HTTPReceiver.ts 16.47% <0.00%> (-15.55%) ⬇️
src/receivers/ExpressReceiver.ts 64.70% <14.28%> (-0.96%) ⬇️
src/App.ts 82.91% <100.00%> (-0.52%) ⬇️
src/receivers/SocketModeReceiver.ts 11.53% <0.00%> (-56.89%) ⬇️
src/receivers/render-html-for-install-path.ts 50.00% <0.00%> (-50.00%) ⬇️
src/errors.ts 91.48% <0.00%> (-8.52%) ⬇️
src/conversation-store.ts 92.30% <0.00%> (-7.70%) ⬇️
src/receivers/AwsLambdaReceiver.ts 59.49% <0.00%> (-0.51%) ⬇️
... and 4 more

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 926b669...d641204. Read the comment docs.

@seratch
Copy link
Member Author

seratch commented Jun 2, 2021

As we are aiming to release v3.4.0 within a few days and this pull request needs more time, let me move this one to v3.5 milestone.

@seratch seratch modified the milestones: 3.4.0, 3.5.0 Jun 2, 2021
@seratch seratch modified the milestones: 3.5.0, 3.6.0 Jul 2, 2021
@srajiang srajiang modified the milestones: 3.6.0, 3.7.0 Aug 19, 2021
@seratch seratch force-pushed the issue-859-auth-error branch from 6d8e685 to 11f7c9e Compare August 26, 2021 03:49
@seratch seratch marked this pull request as ready for review August 26, 2021 03:57
@seratch
Copy link
Member Author

seratch commented Aug 26, 2021

I wanted to write some unit tests for this change but I found that it's not that simple and it may take longer than I thought. For this reason, I did some manual tests with example apps using npm link to the latest revision of the branch.

Example apps that I used for testing:

import { App } from '@slack/bolt';
const app = new App({
  signingSecret: process.env.SLACK_SIGNING_SECRET,
  authorize: async (_) => { throw new Error("Failed!"); }
});
(async () => {
  await app.start(Number(process.env.PORT) || 3000);
  console.log('⚡️ Bolt app is running!');
})();
import { App, ExpressReceiver } from '@slack/bolt';
const app = new App({
  receiver: new ExpressReceiver({ signingSecret: process.env.SLACK_SIGNING_SECRET!! }),
  authorize: async (_) => { throw new Error("Failed!"); }
});

@seratch
Copy link
Member Author

seratch commented Aug 27, 2021

This one is waiting for reviews

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.

LGTM!

@seratch seratch force-pushed the issue-859-auth-error branch from 11f7c9e to d641204 Compare August 27, 2021 23:57
@seratch
Copy link
Member Author

seratch commented Aug 27, 2021

resolved conflicts

@seratch seratch merged commit b3ba64b into slackapi:main Aug 28, 2021
@seratch seratch deleted the issue-859-auth-error branch August 28, 2021 00:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented enhancement M-T: A feature request for new functionality semver:minor tests M-T: Testing work only
Projects
None yet
3 participants