Fix #1059 by adding the token rotation feature support #1060

seratch · 2021-07-12T13:26:50Z

Summary

This pull request fixes #1059 by adding token rotation feature support in this SDK.

TODOs:

Implement the token rotator in the OAuth module
Add example apps (sync/async) in integration tests
Update oauth_v2_access API method to have new arguments
Add oauth_v2_exchange API method support
Update database schema (SQLAlchemy, SQLite)
Add unit tests to verify if the verification logic works as expected
~~[ ] Update the OAuth module docs~~ (in a different PR)

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

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.
I've run python3 -m venv .venv && source .venv/bin/activate && ./scripts/run_validation.sh after making the changes.

codecov · 2021-07-12T13:29:50Z

Codecov Report

Merging #1060 (2784350) into main (df9d71b) will decrease coverage by 0.19%.
The diff coverage is 69.91%.

@@            Coverage Diff             @@
##             main    #1060      +/-   ##
==========================================
- Coverage   84.33%   84.14%   -0.20%     
==========================================
  Files          95       99       +4     
  Lines        8938     9239     +301     
==========================================
+ Hits         7538     7774     +236     
- Misses       1400     1465      +65

Impacted Files	Coverage Δ
...sdk/oauth/installation_store/amazon_s3/__init__.py	`0.00% <0.00%> (ø)`
...lation_store/async_cacheable_installation_store.py	`52.23% <35.29%> (-1.47%)`	⬇️
...k_sdk/oauth/installation_store/sqlite3/__init__.py	`83.33% <42.10%> (-5.06%)`	⬇️
...uth/installation_store/async_installation_store.py	`60.86% <50.00%> (-1.04%)`	⬇️
...sdk/oauth/installation_store/installation_store.py	`69.56% <50.00%> (-1.87%)`	⬇️
slack_sdk/web/legacy_client.py	`94.78% <55.55%> (-0.48%)`	⬇️
...installation_store/cacheable_installation_store.py	`79.41% <70.00%> (+25.70%)`	⬆️
slack_sdk/oauth/token_rotation/async_rotator.py	`74.54% <74.54%> (ø)`
slack_sdk/oauth/token_rotation/rotator.py	`74.54% <74.54%> (ø)`
...dk/oauth/installation_store/models/installation.py	`90.72% <75.67%> (-9.28%)`	⬇️
... and 17 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 df9d71b...2784350. Read the comment docs.

seratch

comments for reviews or future reference

seratch · 2021-07-14T09:19:10Z

integration_tests/samples/oauth/oauth_v2_async.py

@@ -116,7 +116,7 @@ async def oauth_callback(req: Request):
                body=html,
            )

-    error = req.args["error"] if "error" in req.args else ""
+    error = req.args.get("error") if "error" in req.args else ""


req.args["error"] returns an array in Sanic's latest version.

seratch · 2021-07-14T09:20:48Z

integration_tests/samples/token_rotation/oauth.py

+        is_enterprise_install=is_enterprise_install,
+    )
+    if installation is not None:
+        updated_installation = token_rotator.perform_token_rotation(


Bolt for Python internally uses this TokenRotator in authorize middleware. Bolt users do not need to directly use this.

seratch · 2021-07-14T09:21:29Z

integration_tests/samples/token_rotation/oauth.py

+
+    raw_body = request.data.decode("utf-8")
+    body = parse_body(body=raw_body, content_type=extract_content_type(request.headers))
+    rotate_tokens(


Run the rotation for all incoming requests for easy testing

seratch · 2021-07-14T09:21:54Z

integration_tests/samples/token_rotation/oauth.py

+                bot_refresh_token=oauth_response.get("refresh_token"),
+                bot_token_expires_in=oauth_response.get("expires_in"),


newly added

seratch · 2021-07-14T09:22:05Z

integration_tests/samples/token_rotation/oauth.py

+                user_refresh_token=installer.get("refresh_token"),
+                user_token_expires_in=installer.get("expires_in"),


newly added

seratch · 2021-07-14T09:31:26Z

slack_sdk/oauth/token_rotation/rotator.py

+        self,
+        *,
+        installation: Installation,
+        minutes_before_expiration: int = 120,  # 2 hours by default


aligned with bolt-js

seratch · 2021-07-14T10:03:35Z

slack_sdk/web/async_client.py

-        code: str,
+        # This field is required when processing the OAuth redirect URL requests
+        # while it's absent for token rotation
+        code: Optional[str] = None,


Now code is optional

seratch · 2021-07-14T10:04:46Z

tests/slack_sdk/oauth/installation_store/test_simple_cache.py

+        # find bots
+        bot = store.find_bot(enterprise_id="E111", team_id="T111")
+        self.assertIsNotNone(bot)
+        self.assertEqual(bot.bot_refresh_token, "xoxe-1-refreshed")


This test verifies if the find_bot call returns the refreshed token

seratch · 2021-07-14T10:06:33Z

tests/slack_sdk/oauth/token_rotation/test_token_rotator.py

+            bot_refresh_token="xoxe-1-initial",
+            bot_token_expires_in=43200,
+        )
+        refreshed = self.token_rotator.perform_token_rotation(


If the perform_token_rotation method returns a new installation data, your app needs to call InstallationStore#save(installation) to save the new values.

seratch · 2021-07-14T10:07:31Z

tests/slack_sdk/web/mock_web_api_server.py

@@ -70,6 +92,12 @@ def _handle(self):
                if self.headers["authorization"] == "Basic MTExLjIyMjpzZWNyZXQ=":
                    self.wfile.write("""{"ok":true}""".encode("utf-8"))
                    return
+                elif (
+                    self.headers["authorization"]
+                    == "Basic MTExLjIyMjp0b2tlbl9yb3RhdGlvbl9zZWNyZXQ="


This means the values (client_id="111.222", client_secret="token_rotation_secret") that are used in the tests

seratch added enhancement M-T: A feature request for new functionality web-client Version: 3x oauth labels Jul 12, 2021

seratch added this to the 3.8.0 milestone Jul 12, 2021

seratch force-pushed the issue-1059-token-rotation branch from e9703c9 to 383bab4 Compare July 14, 2021 09:11

seratch changed the title ~~WIP: Fix #1059 by adding the token rotation feature support~~ Fix #1059 by adding the token rotation feature support Jul 14, 2021

seratch marked this pull request as ready for review July 14, 2021 09:17

seratch commented Jul 14, 2021

View reviewed changes

seratch added 2 commits July 14, 2021 21:17

Fix issue slackapi#1059 token rotation support

8dd42be

version 3.8.0rc1

6ae2d3b

seratch force-pushed the issue-1059-token-rotation branch from 5d480c2 to 6ae2d3b Compare July 14, 2021 12:18

seratch added 3 commits July 14, 2021 23:00

Add save_bot methods

91b459a

version 3.8.0rc2

5a75c17

Tweak comments and code format

2784350

seratch merged commit dcf1c7c into slackapi:main Jul 15, 2021

seratch deleted the issue-1059-token-rotation branch July 15, 2021 07:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #1059 by adding the token rotation feature support #1060

Fix #1059 by adding the token rotation feature support #1060

seratch commented Jul 12, 2021 •

edited

codecov bot commented Jul 12, 2021 •

edited

seratch left a comment

seratch Jul 14, 2021

seratch Jul 14, 2021

seratch Jul 14, 2021

seratch Jul 14, 2021

seratch Jul 14, 2021

seratch Jul 14, 2021

seratch Jul 14, 2021

seratch Jul 14, 2021

seratch Jul 14, 2021

seratch Jul 14, 2021

		bot_refresh_token=oauth_response.get("refresh_token"),
		bot_token_expires_in=oauth_response.get("expires_in"),

		user_refresh_token=installer.get("refresh_token"),
		user_token_expires_in=installer.get("expires_in"),

Fix #1059 by adding the token rotation feature support #1060

Fix #1059 by adding the token rotation feature support #1060

Conversation

seratch commented Jul 12, 2021 • edited

Summary

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

Requirements (place an x in each [ ])

codecov bot commented Jul 12, 2021 • edited

Codecov Report

seratch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seratch commented Jul 12, 2021 •

edited

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

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

codecov bot commented Jul 12, 2021 •

edited