Skip to content

Make single team auth Bolt JS compatible#99

Merged
seratch merged 1 commit into
slackapi:mainfrom
seratch:remove-auth-test-enabled-option
Sep 23, 2020
Merged

Make single team auth Bolt JS compatible#99
seratch merged 1 commit into
slackapi:mainfrom
seratch:remove-auth-test-enabled-option

Conversation

@seratch

@seratch seratch commented Sep 22, 2020

Copy link
Copy Markdown
Contributor

This pull request modifies SingleTeamAuthorization middleware to be compatible with Bolt for JS. Now the App calls auth.test API method when booting a Bolt app and raises an exception if the call fails.

Also, this pull request removes the authorization_test_enabled option and we will be rethinking a better way to provide the option for multiple workspace apps. Probably, one of the options developers can take advantage of would be to utilize authorize function described at #98 .

(Describe the goal of this PR. Mention any related Issue numbers)

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

  • slack_bolt.App and/or its core components
  • slack_bolt.async_app.AsyncApp and/or its core components
  • Adapters in slack_bolt.adapter
  • Document pages under /docs
  • Others

Requirements (place an x in each [ ])

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

  • 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.
  • I've run ./scripts/install_all_and_run_tests.sh after making the changes.

Also, this change removes authorization_test_enabled option
from all authorization middleare.
@seratch seratch added this to the 0.9.0b0 milestone Sep 22, 2020

@seratch seratch left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Comments for reviewers

verification_enabled=self._authorization_test_enabled
)
)
self._async_middleware_list.append(AsyncSingleTeamAuthorization())

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

To avoid depending on a specific event loop here, we don't do a blocking operation here (NOTE: __init__ is not an async function. We cannot have await here).

response = app.dispatch(request)
assert response.status == 404
assert self.mock_received_requests["/auth.test"] == 2
assert self.mock_received_requests["/auth.test"] == 1

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Now that the auth.test API call result is cached, the call count must be always 1 for the same token.

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

Merging #99 into main will increase coverage by 0.29%.
The diff coverage is 58.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #99      +/-   ##
==========================================
+ Coverage   89.58%   89.88%   +0.29%     
==========================================
  Files         129      129              
  Lines        3621     3597      -24     
==========================================
- Hits         3244     3233      -11     
+ Misses        377      364      -13     
Impacted Files Coverage Δ
...e/authorization/async_multi_teams_authorization.py 65.71% <20.00%> (+7.17%) ⬆️
...dleware/authorization/multi_teams_authorization.py 64.70% <20.00%> (+7.20%) ⬆️
slack_bolt/app/app.py 84.01% <60.00%> (+0.60%) ⬆️
slack_bolt/app/async_app.py 91.24% <100.00%> (-0.03%) ⬇️
...e/authorization/async_single_team_authorization.py 88.88% <100.00%> (-1.74%) ⬇️
...dleware/authorization/single_team_authorization.py 88.46% <100.00%> (-1.20%) ⬇️

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 f96d9b7...a712bd5. Read the comment docs.

@seratch

seratch commented Sep 23, 2020

Copy link
Copy Markdown
Contributor Author

To: reviewers
Let me merge this change now. If you have comments / feedback, please let me know by writing comments at any time. I will quickly fix them by the first beta release.

@seratch seratch merged commit 9968804 into slackapi:main Sep 23, 2020
@seratch seratch deleted the remove-auth-test-enabled-option branch September 23, 2020 02:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants