From d82de19a8a093dc6656c00628458a0c372b4e7d0 Mon Sep 17 00:00:00 2001 From: Kazuhiro Sera Date: Mon, 4 Dec 2023 17:20:46 +0900 Subject: [PATCH] Fix #1441 Built-in InstallationStores fail to resolve a valid bot token when both bot and user-only installations co-exist in database tables (#1442) --- .../installation_store/amazon_s3/__init__.py | 11 +++- .../oauth/installation_store/file/__init__.py | 11 +++- .../installation_store/sqlalchemy/__init__.py | 32 +++++------ .../installation_store/sqlite3/__init__.py | 10 +++- .../installation_store/test_amazon_s3.py | 55 ++++++++++++++++++ .../oauth/installation_store/test_file.py | 55 ++++++++++++++++++ .../installation_store/test_sqlalchemy.py | 57 ++++++++++++++++++- .../oauth/installation_store/test_sqlite3.py | 55 ++++++++++++++++++ 8 files changed, 262 insertions(+), 24 deletions(-) diff --git a/slack_sdk/oauth/installation_store/amazon_s3/__init__.py b/slack_sdk/oauth/installation_store/amazon_s3/__init__.py index 49da3d940..0c2681459 100644 --- a/slack_sdk/oauth/installation_store/amazon_s3/__init__.py +++ b/slack_sdk/oauth/installation_store/amazon_s3/__init__.py @@ -105,6 +105,10 @@ def save(self, installation: Installation): self.logger.debug(f"S3 put_object response: {response}") def save_bot(self, bot: Bot): + if bot.bot_token is None: + self.logger.debug("Skipped saving a new row because of the absense of bot token in it") + return + none = "none" e_id = bot.enterprise_id or none t_id = bot.team_id or none @@ -215,10 +219,13 @@ def find_installation( data = json.loads(body) installation = Installation(**data) - if installation is not None and user_id is not None: + has_user_installation = user_id is not None and installation is not None + no_bot_token_installation = installation is not None and installation.bot_token is None + should_find_bot_installation = has_user_installation or no_bot_token_installation + if should_find_bot_installation: # Retrieve the latest bot token, just in case # See also: https://github.com/slackapi/bolt-python/issues/664 - latest_bot_installation = self.find_installation( + latest_bot_installation = self.find_bot( enterprise_id=enterprise_id, team_id=team_id, is_enterprise_install=is_enterprise_install, diff --git a/slack_sdk/oauth/installation_store/file/__init__.py b/slack_sdk/oauth/installation_store/file/__init__.py index beb18c3df..2c6c3b1d7 100644 --- a/slack_sdk/oauth/installation_store/file/__init__.py +++ b/slack_sdk/oauth/installation_store/file/__init__.py @@ -77,6 +77,10 @@ def save(self, installation: Installation): f.write(entity) def save_bot(self, bot: Bot): + if bot.bot_token is None: + self.logger.debug("Skipped saving a new row because of the absense of bot token in it") + return + none = "none" e_id = bot.enterprise_id or none t_id = bot.team_id or none @@ -169,10 +173,13 @@ def find_installation( data = json.loads(f.read()) installation = Installation(**data) - if installation is not None and user_id is not None: + has_user_installation = user_id is not None and installation is not None + no_bot_token_installation = installation is not None and installation.bot_token is None + should_find_bot_installation = has_user_installation or no_bot_token_installation + if should_find_bot_installation: # Retrieve the latest bot token, just in case # See also: https://github.com/slackapi/bolt-python/issues/664 - latest_bot_installation = self.find_installation( + latest_bot_installation = self.find_bot( enterprise_id=enterprise_id, team_id=team_id, is_enterprise_install=is_enterprise_install, diff --git a/slack_sdk/oauth/installation_store/sqlalchemy/__init__.py b/slack_sdk/oauth/installation_store/sqlalchemy/__init__.py index 3aa783360..3a416f618 100644 --- a/slack_sdk/oauth/installation_store/sqlalchemy/__init__.py +++ b/slack_sdk/oauth/installation_store/sqlalchemy/__init__.py @@ -209,6 +209,7 @@ def find_bot( 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 ) ) .order_by(desc(c.installed_at)) @@ -294,25 +295,24 @@ def find_installation( installed_at=row["installed_at"], ) - if user_id is not None and installation is not None: + has_user_installation = user_id is not None and installation is not None + no_bot_token_installation = installation is not None and installation.bot_token is None + should_find_bot_installation = has_user_installation or no_bot_token_installation + if should_find_bot_installation: # 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 + latest_bot_installation = self.find_bot( + enterprise_id=enterprise_id, + team_id=team_id, + is_enterprise_install=is_enterprise_install, ) - 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"] + if latest_bot_installation is not None and installation.bot_token != latest_bot_installation.bot_token: + installation.bot_id = latest_bot_installation.bot_id + installation.bot_user_id = latest_bot_installation.bot_user_id + installation.bot_token = latest_bot_installation.bot_token + installation.bot_scopes = latest_bot_installation.bot_scopes + installation.bot_refresh_token = latest_bot_installation.bot_refresh_token + installation.bot_token_expires_at = latest_bot_installation.bot_token_expires_at return installation diff --git a/slack_sdk/oauth/installation_store/sqlite3/__init__.py b/slack_sdk/oauth/installation_store/sqlite3/__init__.py index 75fcd305d..c2c6a1771 100644 --- a/slack_sdk/oauth/installation_store/sqlite3/__init__.py +++ b/slack_sdk/oauth/installation_store/sqlite3/__init__.py @@ -59,9 +59,9 @@ def create_tables(self): enterprise_url text, team_id text not null default '', team_name text, - bot_token text not null, - bot_id text not null, - bot_user_id text not null, + bot_token text, + bot_id text, + bot_user_id text, bot_scopes text, bot_refresh_token text, -- since v3.8 bot_token_expires_at datetime, -- since v3.8 @@ -224,6 +224,10 @@ def save(self, installation: Installation): self.save_bot(installation.to_bot()) def save_bot(self, bot: Bot): + if bot.bot_token is None: + self.logger.debug("Skipped saving a new row because of the absense of bot token in it") + return + with self.connect() as conn: conn.execute( """ diff --git a/tests/slack_sdk/oauth/installation_store/test_amazon_s3.py b/tests/slack_sdk/oauth/installation_store/test_amazon_s3.py index fa0d33cc4..ab4b25fb4 100644 --- a/tests/slack_sdk/oauth/installation_store/test_amazon_s3.py +++ b/tests/slack_sdk/oauth/installation_store/test_amazon_s3.py @@ -242,3 +242,58 @@ def test_save_and_find_token_rotation(self): self.assertIsNone(i) bot = store.find_bot(enterprise_id="E111", team_id="T222") self.assertIsNone(bot) + + def test_issue_1441_mixing_user_and_bot_installations(self): + store = self.build_store() + + bot_installation = Installation( + app_id="A111", + enterprise_id="E111", + team_id="T111", + user_id="U111", + bot_id="B111", + bot_token="xoxb-111", + bot_scopes=["chat:write"], + bot_user_id="U222", + ) + store.save(bot_installation) + + # find bots + bot = store.find_bot(enterprise_id="E111", team_id="T111") + self.assertIsNotNone(bot) + bot = store.find_bot(enterprise_id="E111", team_id="T222") + self.assertIsNone(bot) + bot = store.find_bot(enterprise_id=None, team_id="T111") + self.assertIsNone(bot) + + installation = store.find_installation(enterprise_id="E111", team_id="T111") + self.assertIsNotNone(installation.bot_token) + installation = store.find_installation(enterprise_id="E111", team_id="T222") + self.assertIsNone(installation) + installation = store.find_installation(enterprise_id=None, team_id="T111") + self.assertIsNone(installation) + + user_installation = Installation( + app_id="A111", + enterprise_id="E111", + team_id="T111", + user_id="U111", + bot_scopes=["openid"], + user_token="xoxp-111", + ) + store.save(user_installation) + + # find bots + bot = store.find_bot(enterprise_id="E111", team_id="T111") + self.assertIsNotNone(bot.bot_token) + bot = store.find_bot(enterprise_id="E111", team_id="T222") + self.assertIsNone(bot) + bot = store.find_bot(enterprise_id=None, team_id="T111") + self.assertIsNone(bot) + + installation = store.find_installation(enterprise_id="E111", team_id="T111") + self.assertIsNotNone(installation.bot_token) + installation = store.find_installation(enterprise_id="E111", team_id="T222") + self.assertIsNone(installation) + installation = store.find_installation(enterprise_id=None, team_id="T111") + self.assertIsNone(installation) diff --git a/tests/slack_sdk/oauth/installation_store/test_file.py b/tests/slack_sdk/oauth/installation_store/test_file.py index 0cb44dc17..74675452a 100644 --- a/tests/slack_sdk/oauth/installation_store/test_file.py +++ b/tests/slack_sdk/oauth/installation_store/test_file.py @@ -148,3 +148,58 @@ def test_org_installation(self): self.assertIsNone(i) bot = store.find_bot(enterprise_id=None, team_id="T222") self.assertIsNone(bot) + + def test_issue_1441_mixing_user_and_bot_installations(self): + store = FileInstallationStore(client_id="111.222") + + bot_installation = Installation( + app_id="A111", + enterprise_id="E111", + team_id="T111", + user_id="U111", + bot_id="B111", + bot_token="xoxb-111", + bot_scopes=["chat:write"], + bot_user_id="U222", + ) + store.save(bot_installation) + + # find bots + bot = store.find_bot(enterprise_id="E111", team_id="T111") + self.assertIsNotNone(bot) + bot = store.find_bot(enterprise_id="E111", team_id="T222") + self.assertIsNone(bot) + bot = store.find_bot(enterprise_id=None, team_id="T111") + self.assertIsNone(bot) + + installation = store.find_installation(enterprise_id="E111", team_id="T111") + self.assertIsNotNone(installation.bot_token) + installation = store.find_installation(enterprise_id="E111", team_id="T222") + self.assertIsNone(installation) + installation = store.find_installation(enterprise_id=None, team_id="T111") + self.assertIsNone(installation) + + user_installation = Installation( + app_id="A111", + enterprise_id="E111", + team_id="T111", + user_id="U111", + bot_scopes=["openid"], + user_token="xoxp-111", + ) + store.save(user_installation) + + # find bots + bot = store.find_bot(enterprise_id="E111", team_id="T111") + self.assertIsNotNone(bot.bot_token) + bot = store.find_bot(enterprise_id="E111", team_id="T222") + self.assertIsNone(bot) + bot = store.find_bot(enterprise_id=None, team_id="T111") + self.assertIsNone(bot) + + installation = store.find_installation(enterprise_id="E111", team_id="T111") + self.assertIsNotNone(installation.bot_token) + installation = store.find_installation(enterprise_id="E111", team_id="T222") + self.assertIsNone(installation) + installation = store.find_installation(enterprise_id=None, team_id="T111") + self.assertIsNone(installation) diff --git a/tests/slack_sdk/oauth/installation_store/test_sqlalchemy.py b/tests/slack_sdk/oauth/installation_store/test_sqlalchemy.py index 5e8abea88..4d827f70b 100644 --- a/tests/slack_sdk/oauth/installation_store/test_sqlalchemy.py +++ b/tests/slack_sdk/oauth/installation_store/test_sqlalchemy.py @@ -7,7 +7,7 @@ from slack_sdk.oauth.installation_store.sqlalchemy import SQLAlchemyInstallationStore -class TestSQLite3(unittest.TestCase): +class TestSQLAlchemy(unittest.TestCase): engine: Engine def setUp(self): @@ -234,3 +234,58 @@ def test_save_and_find_token_rotation(self): self.assertIsNone(i) bot = store.find_bot(enterprise_id="E111", team_id="T222") self.assertIsNone(bot) + + def test_issue_1441_mixing_user_and_bot_installations(self): + store = self.store + + bot_installation = Installation( + app_id="A111", + enterprise_id="E111", + team_id="T111", + user_id="U111", + bot_id="B111", + bot_token="xoxb-111", + bot_scopes=["chat:write"], + bot_user_id="U222", + ) + store.save(bot_installation) + + # find bots + bot = store.find_bot(enterprise_id="E111", team_id="T111") + self.assertIsNotNone(bot) + bot = store.find_bot(enterprise_id="E111", team_id="T222") + self.assertIsNone(bot) + bot = store.find_bot(enterprise_id=None, team_id="T111") + self.assertIsNone(bot) + + installation = store.find_installation(enterprise_id="E111", team_id="T111") + self.assertIsNotNone(installation.bot_token) + installation = store.find_installation(enterprise_id="E111", team_id="T222") + self.assertIsNone(installation) + installation = store.find_installation(enterprise_id=None, team_id="T111") + self.assertIsNone(installation) + + user_installation = Installation( + app_id="A111", + enterprise_id="E111", + team_id="T111", + user_id="U111", + bot_scopes=["openid"], + user_token="xoxp-111", + ) + store.save(user_installation) + + # find bots + bot = store.find_bot(enterprise_id="E111", team_id="T111") + self.assertIsNotNone(bot.bot_token) + bot = store.find_bot(enterprise_id="E111", team_id="T222") + self.assertIsNone(bot) + bot = store.find_bot(enterprise_id=None, team_id="T111") + self.assertIsNone(bot) + + installation = store.find_installation(enterprise_id="E111", team_id="T111") + self.assertIsNotNone(installation.bot_token) + installation = store.find_installation(enterprise_id="E111", team_id="T222") + self.assertIsNone(installation) + installation = store.find_installation(enterprise_id=None, team_id="T111") + self.assertIsNone(installation) diff --git a/tests/slack_sdk/oauth/installation_store/test_sqlite3.py b/tests/slack_sdk/oauth/installation_store/test_sqlite3.py index 24e106be9..e616c20bc 100644 --- a/tests/slack_sdk/oauth/installation_store/test_sqlite3.py +++ b/tests/slack_sdk/oauth/installation_store/test_sqlite3.py @@ -235,3 +235,58 @@ def test_save_and_find_token_rotation(self): self.assertIsNone(i) bot = store.find_bot(enterprise_id="E111", team_id="T222") self.assertIsNone(bot) + + def test_issue_1441_mixing_user_and_bot_installations(self): + store = SQLite3InstallationStore(database="logs/test.db", client_id="111.222") + + bot_installation = Installation( + app_id="A111", + enterprise_id="E111", + team_id="T111", + user_id="U111", + bot_id="B111", + bot_token="xoxb-111", + bot_scopes=["chat:write"], + bot_user_id="U222", + ) + store.save(bot_installation) + + # find bots + bot = store.find_bot(enterprise_id="E111", team_id="T111") + self.assertIsNotNone(bot) + bot = store.find_bot(enterprise_id="E111", team_id="T222") + self.assertIsNone(bot) + bot = store.find_bot(enterprise_id=None, team_id="T111") + self.assertIsNone(bot) + + installation = store.find_installation(enterprise_id="E111", team_id="T111") + self.assertIsNotNone(installation.bot_token) + installation = store.find_installation(enterprise_id="E111", team_id="T222") + self.assertIsNone(installation) + installation = store.find_installation(enterprise_id=None, team_id="T111") + self.assertIsNone(installation) + + user_installation = Installation( + app_id="A111", + enterprise_id="E111", + team_id="T111", + user_id="U111", + bot_scopes=["openid"], + user_token="xoxp-111", + ) + store.save(user_installation) + + # find bots + bot = store.find_bot(enterprise_id="E111", team_id="T111") + self.assertIsNotNone(bot.bot_token) + bot = store.find_bot(enterprise_id="E111", team_id="T222") + self.assertIsNone(bot) + bot = store.find_bot(enterprise_id=None, team_id="T111") + self.assertIsNone(bot) + + installation = store.find_installation(enterprise_id="E111", team_id="T111") + self.assertIsNotNone(installation.bot_token) + installation = store.find_installation(enterprise_id="E111", team_id="T222") + self.assertIsNone(installation) + installation = store.find_installation(enterprise_id=None, team_id="T111") + self.assertIsNone(installation)