Fix #1078 Add Sign in with Slack (OpenID Connect) support by seratch · Pull Request #1079 · slackapi/python-slack-sdk

seratch · 2021-07-30T09:40:55Z

Summary

This pull request fixes #1078 by adding the following changes to this project.

Add new Web API support
- openid.connect.token
- openid.connect.userInfo
Add "Sign in with Slack" example apps
- Flask
- Sanic (for asyncio users)

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-30T09:44:02Z

Codecov Report

Merging #1079 (10a095e) into main (4591de7) will decrease coverage by 0.27%.
The diff coverage is 20.83%.

@@            Coverage Diff             @@
##             main    #1079      +/-   ##
==========================================
- Coverage   84.15%   83.88%   -0.28%     
==========================================
  Files          99       99              
  Lines        9255     9304      +49     
==========================================
+ Hits         7789     7805      +16     
- Misses       1466     1499      +33

Impacted Files	Coverage Δ
slack_sdk/web/async_client.py	`92.81% <16.66%> (-0.94%)`	⬇️
slack_sdk/web/client.py	`94.21% <16.66%> (-0.96%)`	⬇️
slack_sdk/web/legacy_client.py	`93.83% <16.66%> (-0.95%)`	⬇️
...lack_sdk/oauth/authorize_url_generator/__init__.py	`62.96% <27.27%> (-30.38%)`	⬇️
slack_sdk/oauth/__init__.py	`100.00% <100.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 4591de7...10a095e. Read the comment docs.

seratch

comments for reviewers

seratch · 2021-07-30T09:41:31Z

integration_tests/samples/openid_connect/app_manifest.yml

@@ -0,0 +1,22 @@
+_metadata:


The minimum App Manifest of an OpenID Connect app

seratch · 2021-07-30T09:42:27Z

integration_tests/samples/openid_connect/flask_example.py

+authorization_url_generator = OpenIDConnectAuthorizeUrlGenerator(
+    client_id=client_id,
+    scopes=scopes,
+    redirect_uri=redirect_uri,


Unlike the existing authorize URL, redirect_uri is mandatory for building OpenID Connect authorize URL.

seratch · 2021-07-30T09:43:18Z

integration_tests/samples/openid_connect/requirements.txt

+slack-sdk
+Flask>=2,<3
+pyjwt>=2.1,<3
+cryptography>=3.4,<4


In addition to pyjwt, cryptography is required for RS256 algorithm

seratch · 2021-07-30T09:44:59Z

slack_sdk/oauth/authorize_url_generator/__init__.py

+            "response_type=code&"
+            f"state={state}&"
+            f"client_id={self.client_id}&"
+            f"scope={scopes}&"


The user scopes like oauth in the Slack app configuration page are set as "scope" (not "user_scope")

seratch · 2021-07-30T09:45:58Z

slack_sdk/oauth/authorize_url_generator/__init__.py

+            f"scope={scopes}&"
+            f"redirect_uri={self.redirect_uri}"
+        )
+        if nonce is not None:


nonce parameter is an optional one in the OpenID Connect 1.0 Specification https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest

seratch · 2021-07-30T09:46:34Z

tests/slack_sdk_async/web/test_web_client_coverage.py

                self.api_methods_to_call.remove(method(users="U123,U234")["method"])
                method(users="U123,U234")
                await async_method(users="U123,U234")
-            elif method_name == "oauth_access":


Just removed unused ones

filmaj

Nice, looks good, thanks for sharing. It seems like the pattern to implement new API methods is straightforward and clear. As someone new to the project, the integration tests / sample applications are very helpful, too!

filmaj · 2021-07-30T12:27:18Z

tests/slack_sdk_async/web/test_web_client_coverage.py

-    # Can be fetched by running `var methodNames = [].slice.call(document.getElementsByClassName('bold')).map(e => e.text);console.log(methodNames.toString());console.log(methodNames.length);` on https://api.slack.com/methods
-    all_api_methods = "admin.analytics.getFile,admin.apps.approve,admin.apps.clearResolution,admin.apps.restrict,admin.apps.uninstall,admin.apps.approved.list,admin.apps.requests.list,admin.apps.restricted.list,admin.auth.policy.assignEntities,admin.auth.policy.getEntities,admin.auth.policy.removeEntities,admin.barriers.create,admin.barriers.delete,admin.barriers.list,admin.barriers.update,admin.conversations.archive,admin.conversations.convertToPrivate,admin.conversations.create,admin.conversations.delete,admin.conversations.getConversationPrefs,admin.conversations.getCustomRetention,admin.conversations.getTeams,admin.conversations.invite,admin.conversations.removeCustomRetention,admin.conversations.rename,admin.conversations.search,admin.conversations.setConversationPrefs,admin.conversations.setCustomRetention,admin.conversations.setTeams,admin.conversations.unarchive,admin.conversations.ekm.listOriginalConnectedChannelInfo,admin.conversations.restrictAccess.addGroup,admin.conversations.restrictAccess.listGroups,admin.conversations.restrictAccess.removeGroup,admin.emoji.add,admin.emoji.addAlias,admin.emoji.list,admin.emoji.remove,admin.emoji.rename,admin.inviteRequests.approve,admin.inviteRequests.deny,admin.inviteRequests.list,admin.inviteRequests.approved.list,admin.inviteRequests.denied.list,admin.teams.admins.list,admin.teams.create,admin.teams.list,admin.teams.owners.list,admin.teams.settings.info,admin.teams.settings.setDefaultChannels,admin.teams.settings.setDescription,admin.teams.settings.setDiscoverability,admin.teams.settings.setIcon,admin.teams.settings.setName,admin.usergroups.addChannels,admin.usergroups.addTeams,admin.usergroups.listChannels,admin.usergroups.removeChannels,admin.users.assign,admin.users.invite,admin.users.list,admin.users.remove,admin.users.setAdmin,admin.users.setExpiration,admin.users.setOwner,admin.users.setRegular,admin.users.session.clearSettings,admin.users.session.getSettings,admin.users.session.invalidate,admin.users.session.list,admin.users.session.reset,admin.users.session.setSettings,api.test,apps.connections.open,apps.uninstall,apps.event.authorizations.list,auth.revoke,auth.test,auth.teams.list,bots.info,calls.add,calls.end,calls.info,calls.update,calls.participants.add,calls.participants.remove,chat.delete,chat.deleteScheduledMessage,chat.getPermalink,chat.meMessage,chat.postEphemeral,chat.postMessage,chat.scheduleMessage,chat.unfurl,chat.update,chat.scheduledMessages.list,conversations.archive,conversations.close,conversations.create,conversations.history,conversations.info,conversations.invite,conversations.join,conversations.kick,conversations.leave,conversations.list,conversations.mark,conversations.members,conversations.open,conversations.rename,conversations.replies,conversations.setPurpose,conversations.setTopic,conversations.unarchive,dialog.open,dnd.endDnd,dnd.endSnooze,dnd.info,dnd.setSnooze,dnd.teamInfo,emoji.list,files.delete,files.info,files.list,files.revokePublicURL,files.sharedPublicURL,files.upload,files.comments.delete,files.remote.add,files.remote.info,files.remote.list,files.remote.remove,files.remote.share,files.remote.update,migration.exchange,oauth.access,oauth.token,oauth.v2.access,pins.add,pins.list,pins.remove,reactions.add,reactions.get,reactions.list,reactions.remove,reminders.add,reminders.complete,reminders.delete,reminders.info,reminders.list,rtm.connect,rtm.start,search.all,search.files,search.messages,stars.add,stars.list,stars.remove,team.accessLogs,team.billableInfo,team.info,team.integrationLogs,team.profile.get,usergroups.create,usergroups.disable,usergroups.enable,usergroups.list,usergroups.update,usergroups.users.list,usergroups.users.update,users.conversations,users.deletePhoto,users.getPresence,users.identity,users.info,users.list,users.lookupByEmail,users.setActive,users.setPhoto,users.setPresence,users.profile.get,users.profile.set,views.open,views.publish,views.push,views.update,workflows.stepCompleted,workflows.stepFailed,workflows.updateStep,apps.permissions.info,apps.permissions.request,apps.permissions.resources.list,apps.permissions.scopes.list,apps.permissions.users.list,apps.permissions.users.request,channels.archive,channels.create,channels.history,channels.info,channels.invite,channels.join,channels.kick,channels.leave,channels.list,channels.mark,channels.rename,channels.replies,channels.setPurpose,channels.setTopic,channels.unarchive,groups.archive,groups.create,groups.createChild,groups.history,groups.info,groups.invite,groups.kick,groups.leave,groups.list,groups.mark,groups.open,groups.rename,groups.replies,groups.setPurpose,groups.setTopic,groups.unarchive,im.close,im.history,im.list,im.mark,im.open,im.replies,mpim.close,mpim.history,mpim.list,mpim.mark,mpim.open,mpim.replies".split(
+    # 247 endpoints as of July 29, 2021
+    # Can be fetched by running `var methodNames = [].slice.call(document.getElementsByClassName('apiReferenceFilterableList__listItemLink')).map(e => e.href.replace("https://api.slack.com/methods/", ""));console.log(methodNames.toString());console.log(methodNames.length);` on https://api.slack.com/methods


stevengill

Looks great as usual Kaz! Love having examples with the code changes too.

stevengill · 2021-07-31T00:08:14Z

tests/slack_sdk_async/web/test_web_client_coverage.py

                "admin.conversations.getCustomRetention",
                "admin.conversations.removeCustomRetention",
                "admin.conversations.setCustomRetention",
+                # TODO: Slack Connect APIs


I believe @srajiang will look at adding these when she gets back next week

Great! Happy to be a reviewer for it

Just for reference: #1058

stevengill · 2021-07-31T00:24:59Z

integration_tests/samples/openid_connect/flask_example.py

+    )
+
+
+@app.route("/slack/oauth_redirect", methods=["GET"])


Interesting that you are using the same routes we use for normal oauth

Not strong opinion here 😸 If you prefer other paths for tutorials, we can update this example for consistency

stevengill · 2021-07-31T00:26:39Z

integration_tests/samples/openid_connect/flask_example.py

+
+@app.route("/slack/install", methods=["GET"])
+def oauth_start():
+    state = state_store.issue()


What is this line doing?

The line generates a new unique state parameter value. The state_store (FileOAuthStateStore in this example) stores the generated value in the local file system. And then, state_store.consume(state) in the redirect URL handler checks and deletes it.

This SDK provides local files, Amazon S3, and RDB as the backends of OAuthStateStore: https://github.com/slackapi/python-slack-sdk/tree/main/slack_sdk/oauth/state_store

If you switch to other implementation, this code works with the chosen backend without any changes.

Fix slackapi#1078 Add Sign in with Slack (OpenID Connect) support

d97c41e

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

seratch added this to the 3.9.0 milestone Jul 30, 2021

seratch requested review from filmaj, mwbrooks and stevengill July 30, 2021 09:40

Remove unused files

10a095e

seratch commented Jul 30, 2021

View reviewed changes

filmaj approved these changes Jul 30, 2021

View reviewed changes

stevengill mentioned this pull request Jul 30, 2021

Add "Sign in with Slack (OpenID Connect)" support slackapi/node-slack-sdk#1296

Closed

9 tasks

stevengill approved these changes Jul 31, 2021

View reviewed changes

seratch mentioned this pull request Jul 31, 2021

Add "Sign in with Slack (OpenID Connect)" support slackapi/java-slack-sdk#795

Closed

6 tasks

seratch merged commit 1453e1d into slackapi:main Aug 1, 2021

seratch deleted the issue-1078-openid-connect branch August 1, 2021 21:15

		)


		@app.route("/slack/oauth_redirect", methods=["GET"])

Conversation

seratch commented Jul 30, 2021

Summary

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

Requirements (place an x in each [ ])

Uh oh!

codecov bot commented Jul 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

seratch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seratch Jul 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seratch Jul 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filmaj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevengill left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

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

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

codecov bot commented Jul 30, 2021 •

edited

Loading

seratch Jul 30, 2021 •

edited

Loading

seratch Jul 30, 2021 •

edited

Loading