slackapi/python-slack-sdk

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

kulmatitskiy opened this issue · 2 comments

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.

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.

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