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

Built-in InstallationStores fail to resolve a valid bot token when both bot and user-only installations co-exist in database tables #1441

Closed
kulmatitskiy opened this issue Dec 2, 2023 · 2 comments · Fixed by #1442
Assignees
Labels
bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented enhancement M-T: A feature request for new functionality oauth Version: 3x
Milestone

Comments

@kulmatitskiy
Copy link

kulmatitskiy commented Dec 2, 2023

This happens in apps that implement both org-wide bot-scope installs together with individual user-scope installs.

Examples where an app would want to support this: (a) handling "sign in with Slack" using Bolt and /oauth_redirect post-auth redirect url; or (b) offering extra user-level functionality to individuals on top of the org-wide more restricted bot-scope functionality, e.g. see this older discussion: link

Steps to reproduce:

  1. Have the app installed as a bot in slack workspace -> creates a row with bot token and bot scopes in the installation table, but no user tokens in the same row.
  2. Get some user go through a user-token based oauth route (not the install to workspace route) -> it creates a row with user token only.
  3. Now, let's say the same user mentions the bot (from step 1) in Slack –> event is triggered and our Bolt program eventually gets into the method SQLAlchemyInstallationStore.find_installation

Expected: we are supposed to find the bot token to respond to the mention.

What actually happens. Error will be logged and the mention event will be ignored, here is why. This query:

query = self.installations.select().where(where_clause).order_by(desc(c.installed_at)).limit(1)

gets us the user token row from Step 2, not the one from Step 1. Why? B/c (a) it's the most recent installed_at, and (b) the argument user_id passed to the function is None (as happens when using Bolt - e.g. called from here). So at this point we are missing the bot scopes we actually wanted (in order to get the bot respond to the reply).

You would think that the following part of the method is directly aimed at this scenario, i.e. it should go the extra step to retrieve the bot token, but since user_id is None, this block is never executed:

if user_id is not None and installation is not None:
# Retrieve the latest bot token, just in case
# See also: https://github.com/slackapi/bolt-python/issues/664
where_clause = and_(
c.client_id == self.client_id,
c.enterprise_id == enterprise_id,
c.team_id == team_id,
c.bot_token.is_not(None), # the latest one that has a bot token
)
query = self.installations.select().where(where_clause).order_by(desc(c.installed_at)).limit(1)
with self.engine.connect() as conn:
result: object = conn.execute(query)
for row in result.mappings(): # type: ignore
installation.bot_token = row["bot_token"]
installation.bot_id = row["bot_id"]
installation.bot_user_id = row["bot_user_id"]
installation.bot_scopes = row["bot_scopes"]
installation.bot_refresh_token = row["bot_refresh_token"]
installation.bot_token_expires_at = row["bot_token_expires_at"]

As a result, the error is logged (if using the Bolt App):

"Although the app should be installed into this workspace, the AuthorizeResult (returned value from authorize) for it was not found."

And thus the user does not get to see any response from the bot.

Moreover, this outcome depends on the order of the installation rows. If someone reinstalls the bot to the workspace, then the mention will now start working because row with the bot token will become the one with the most recent installed_at again. If the user then does user-token oauth again, the mention will stop working again. And so on – leading to unpredictable bot behaviour from the users' point of view.

@kulmatitskiy kulmatitskiy changed the title SQLAlchemyInstallationStore.find_installation bug in apps with both user/oauth tokens and bot tokens SQLAlchemyInstallationStore.find_installation bug in apps that have both user-scope and bot-scope installs Dec 2, 2023
@seratch seratch changed the title SQLAlchemyInstallationStore.find_installation bug in apps that have both user-scope and bot-scope installs Built-in InstallationStores fail to resolve a valid bot token when both bot and user-only installations co-exist in database tables Dec 4, 2023
@seratch seratch self-assigned this Dec 4, 2023
@seratch seratch added bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented enhancement M-T: A feature request for new functionality Version: 3x oauth labels Dec 4, 2023
@seratch seratch added this to the 3.26.1 milestone Dec 4, 2023
seratch added a commit to seratch/python-slack-sdk that referenced this issue Dec 4, 2023
… bot token when both bot and user-only installations co-exist in database tables
@seratch
Copy link
Contributor

seratch commented Dec 4, 2023

Hi @kulmatitskiy, thanks for submitting the detailed issue report! You're right that currently the built-in InstallationStore does not work properly when an installation without bot scopes exists.

Regarding the OIDC user cases, I would advise having a separate table (at the coding level, this might mean a different InstallationStore that points to a different table for OIDC) for reasons of data efficiency (= OIDC can generate a larger number of records than app installations) and operational simplicity.

However, our current code does not support user-scope-only installations post bot installation and this must be resolved. We aim to fix this issue in the following patch release. Once again, I appreciate your report there.

seratch added a commit that referenced this issue Dec 4, 2023
…en when both bot and user-only installations co-exist in database tables (#1442)
@kulmatitskiy
Copy link
Author

Amazing, thank you @seratch! This fix is unblocking my use cases, and separately thanks for the advice re: separate tables idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented enhancement M-T: A feature request for new functionality oauth Version: 3x
Projects
None yet
2 participants