From ed2af768a1ef091f464e4797773a30a8edce49b0 Mon Sep 17 00:00:00 2001
From: Nikita Mikhaylov <nikitamikhaylov@clickhouse.com>
Date: Mon, 18 Nov 2024 22:37:28 +0000
Subject: [PATCH 1/3] Correct permissions for dictionaries

---
 src/Storages/StorageDictionary.cpp            |  2 +-
 .../03273_dictionary_rbac.reference           |  6 ++
 .../0_stateless/03273_dictionary_rbac.sh      | 56 +++++++++++++++++++
 3 files changed, 63 insertions(+), 1 deletion(-)
 create mode 100644 tests/queries/0_stateless/03273_dictionary_rbac.reference
 create mode 100755 tests/queries/0_stateless/03273_dictionary_rbac.sh

diff --git a/src/Storages/StorageDictionary.cpp b/src/Storages/StorageDictionary.cpp
index e9f732466ff..754f61f00eb 100644
--- a/src/Storages/StorageDictionary.cpp
+++ b/src/Storages/StorageDictionary.cpp
@@ -179,7 +179,7 @@ Pipe StorageDictionary::read(
 {
     auto registered_dictionary_name = location == Location::SameDatabaseAndNameAsDictionary ? getStorageID().getInternalDictionaryName() : dictionary_name;
     auto dictionary = getContext()->getExternalDictionariesLoader().getDictionary(registered_dictionary_name, local_context);
-    local_context->checkAccess(AccessType::dictGet, dictionary->getDatabaseOrNoDatabaseTag(), dictionary->getDictionaryID().getTableName());
+    local_context->checkAccess(AccessType::dictGet | AccessType::SELECT, dictionary->getDatabaseOrNoDatabaseTag(), dictionary->getDictionaryID().getTableName());
     return dictionary->read(column_names, max_block_size, threads);
 }
 
diff --git a/tests/queries/0_stateless/03273_dictionary_rbac.reference b/tests/queries/0_stateless/03273_dictionary_rbac.reference
new file mode 100644
index 00000000000..989c60a878a
--- /dev/null
+++ b/tests/queries/0_stateless/03273_dictionary_rbac.reference
@@ -0,0 +1,6 @@
+Ok.
+Ok.
+Ok.
+ACCESS_DENIED
+
+
diff --git a/tests/queries/0_stateless/03273_dictionary_rbac.sh b/tests/queries/0_stateless/03273_dictionary_rbac.sh
new file mode 100755
index 00000000000..2b66f55fa07
--- /dev/null
+++ b/tests/queries/0_stateless/03273_dictionary_rbac.sh
@@ -0,0 +1,56 @@
+#!/usr/bin/env bash
+
+CUR_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)
+# shellcheck source=../shell_config.sh
+. "$CUR_DIR"/../shell_config.sh
+
+username="user_${CLICKHOUSE_TEST_UNIQUE_NAME}"
+dictname="dict_${CLICKHOUSE_TEST_UNIQUE_NAME}"
+dicttablename="dict_table_${CLICKHOUSE_TEST_UNIQUE_NAME}"
+
+# Create a dictionary and a table that points to the dictionary.
+${CLICKHOUSE_CLIENT} -nm --query "
+    CREATE DICTIONARY IF NOT EXISTS ${dictname}
+    (
+        id UInt64,
+        value UInt64
+    )
+    PRIMARY KEY id
+    SOURCE(NULL())
+    LAYOUT(FLAT())
+    LIFETIME(MIN 0 MAX 1000);
+    CREATE TABLE ${dicttablename} (id UInt64, value UInt64)
+    ENGINE = Dictionary(${CLICKHOUSE_DATABASE}.${dictname});
+"
+
+# Create a user, assign only a few permissions.
+${CLICKHOUSE_CLIENT} -nm --query "
+    CREATE USER IF NOT EXISTS ${username} NOT IDENTIFIED;
+    GRANT SELECT, CREATE TEMPORARY TABLE ON *.* to ${username};
+"
+
+# Reading from dictionary via direct SELECT is Ok.
+$CLICKHOUSE_CLIENT -nm --user="${username}" --query "
+    SELECT * FROM ${dictname};
+" 2>&1 > /dev/null && echo "Ok."
+
+# Reading from dictionary via dictionary storage is Ok.
+$CLICKHOUSE_CLIENT -nm --user="${username}" --query "
+    SELECT * FROM ${dicttablename};
+" 2>&1 > /dev/null && echo "Ok."
+
+# Reading from dictionary via dictionary table function is Ok.
+$CLICKHOUSE_CLIENT -nm --user="${username}" --query "
+    SELECT * FROM dictionary(${dictname});
+" 2>&1 > /dev/null && echo "Ok."
+
+# Function dictGet requires a permission dictGet to use.
+$CLICKHOUSE_CLIENT -nm --user="${username}" --query "
+    SELECT dictGet(${dictname}, 'value', 1);
+" 2>&1 | grep -o ACCESS_DENIED | uniq
+
+${CLICKHOUSE_CLIENT} -nm --query "
+    DROP TABLE IF EXISTS ${dicttablename} SYNC;
+    DROP DICTIONARY IF EXISTS ${dictname};
+    DROP USER IF EXISTS ${username};
+"

From dc88b973e9cb5518fbbb917caf704ceb18eab8da Mon Sep 17 00:00:00 2001
From: Nikita Mikhaylov <nikitamikhaylov@clickhouse.com>
Date: Mon, 18 Nov 2024 22:38:16 +0000
Subject: [PATCH 2/3] Better

---
 tests/queries/0_stateless/03273_dictionary_rbac.reference | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tests/queries/0_stateless/03273_dictionary_rbac.reference b/tests/queries/0_stateless/03273_dictionary_rbac.reference
index 989c60a878a..970842ef197 100644
--- a/tests/queries/0_stateless/03273_dictionary_rbac.reference
+++ b/tests/queries/0_stateless/03273_dictionary_rbac.reference
@@ -3,4 +3,3 @@ Ok.
 Ok.
 ACCESS_DENIED
 
-

From 36902c66a0eb8cbc73352152431634667b6a5a57 Mon Sep 17 00:00:00 2001
From: Nikita Mikhaylov <nikitamikhaylov@clickhouse.com>
Date: Tue, 19 Nov 2024 00:20:51 +0000
Subject: [PATCH 3/3] Corrections

---
 src/Storages/StorageDictionary.cpp            | 14 ++++++-
 .../03199_dictionary_table_access.reference   |  2 -
 .../03199_dictionary_table_access.sh          | 41 -------------------
 .../03273_dictionary_rbac.reference           |  1 -
 .../0_stateless/03273_dictionary_rbac.sh      |  6 +--
 5 files changed, 16 insertions(+), 48 deletions(-)
 delete mode 100644 tests/queries/0_stateless/03199_dictionary_table_access.reference
 delete mode 100755 tests/queries/0_stateless/03199_dictionary_table_access.sh

diff --git a/src/Storages/StorageDictionary.cpp b/src/Storages/StorageDictionary.cpp
index 754f61f00eb..80cb1cc285d 100644
--- a/src/Storages/StorageDictionary.cpp
+++ b/src/Storages/StorageDictionary.cpp
@@ -1,4 +1,5 @@
 #include <Access/Common/AccessFlags.h>
+#include <Access/ContextAccess.h>
 #include <Storages/StorageDictionary.h>
 #include <Storages/StorageFactory.h>
 #include <DataTypes/DataTypesNumber.h>
@@ -179,7 +180,18 @@ Pipe StorageDictionary::read(
 {
     auto registered_dictionary_name = location == Location::SameDatabaseAndNameAsDictionary ? getStorageID().getInternalDictionaryName() : dictionary_name;
     auto dictionary = getContext()->getExternalDictionariesLoader().getDictionary(registered_dictionary_name, local_context);
-    local_context->checkAccess(AccessType::dictGet | AccessType::SELECT, dictionary->getDatabaseOrNoDatabaseTag(), dictionary->getDictionaryID().getTableName());
+
+    /**
+     * For backward compatibility reasons we require either SELECT or dictGet permission to read directly from the dictionary.
+     * If none of these conditions are met - we ask to grant a dictGet.
+     */
+    bool has_dict_get = local_context->getAccess()->isGranted(
+        AccessType::dictGet, dictionary->getDatabaseOrNoDatabaseTag(), dictionary->getDictionaryID().getTableName());
+    bool has_select = local_context->getAccess()->isGranted(
+        AccessType::SELECT, dictionary->getDatabaseOrNoDatabaseTag(), dictionary->getDictionaryID().getTableName());
+    if (!has_dict_get && !has_select)
+        local_context->checkAccess(AccessType::dictGet, dictionary->getDatabaseOrNoDatabaseTag(), dictionary->getDictionaryID().getTableName());
+
     return dictionary->read(column_names, max_block_size, threads);
 }
 
diff --git a/tests/queries/0_stateless/03199_dictionary_table_access.reference b/tests/queries/0_stateless/03199_dictionary_table_access.reference
deleted file mode 100644
index 4a703b3be84..00000000000
--- a/tests/queries/0_stateless/03199_dictionary_table_access.reference
+++ /dev/null
@@ -1,2 +0,0 @@
-ACCESS_DENIED
-ACCESS_DENIED
diff --git a/tests/queries/0_stateless/03199_dictionary_table_access.sh b/tests/queries/0_stateless/03199_dictionary_table_access.sh
deleted file mode 100755
index 14f017c7fbc..00000000000
--- a/tests/queries/0_stateless/03199_dictionary_table_access.sh
+++ /dev/null
@@ -1,41 +0,0 @@
-#!/usr/bin/env bash
-
-CUR_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)
-# shellcheck source=../shell_config.sh
-. "$CUR_DIR"/../shell_config.sh
-
-username="user_${CLICKHOUSE_TEST_UNIQUE_NAME}"
-dictname="dict_${CLICKHOUSE_TEST_UNIQUE_NAME}"
-dicttablename="dict_table_${CLICKHOUSE_TEST_UNIQUE_NAME}"
-
-${CLICKHOUSE_CLIENT} -m --query "
-    CREATE DICTIONARY IF NOT EXISTS ${dictname}
-    (
-        id UInt64,
-        value UInt64
-    )
-    PRIMARY KEY id
-    SOURCE(NULL())
-    LAYOUT(FLAT())
-    LIFETIME(MIN 0 MAX 1000);
-    CREATE USER IF NOT EXISTS ${username} NOT IDENTIFIED;
-    GRANT SELECT, CREATE TEMPORARY TABLE ON *.* to ${username};
-    SELECT * FROM ${dictname};
-    CREATE TABLE ${dicttablename} (id UInt64, value UInt64)
-    ENGINE = Dictionary(${CLICKHOUSE_DATABASE}.${dictname});
-    SELECT * FROM ${dicttablename};
-"
-
-$CLICKHOUSE_CLIENT -m --user="${username}" --query "
-    SELECT * FROM ${dictname};
-" 2>&1 | grep -o ACCESS_DENIED | uniq
-
-$CLICKHOUSE_CLIENT -m --user="${username}" --query "
-    SELECT * FROM ${dicttablename};
-" 2>&1 | grep -o ACCESS_DENIED | uniq
-
-${CLICKHOUSE_CLIENT} -m --query "
-    DROP TABLE IF EXISTS ${dicttablename} SYNC;
-    DROP DICTIONARY IF EXISTS ${dictname};
-    DROP USER IF EXISTS ${username};
-"
diff --git a/tests/queries/0_stateless/03273_dictionary_rbac.reference b/tests/queries/0_stateless/03273_dictionary_rbac.reference
index 970842ef197..f7ff98213ba 100644
--- a/tests/queries/0_stateless/03273_dictionary_rbac.reference
+++ b/tests/queries/0_stateless/03273_dictionary_rbac.reference
@@ -2,4 +2,3 @@ Ok.
 Ok.
 Ok.
 ACCESS_DENIED
-
diff --git a/tests/queries/0_stateless/03273_dictionary_rbac.sh b/tests/queries/0_stateless/03273_dictionary_rbac.sh
index 2b66f55fa07..d74039b4d61 100755
--- a/tests/queries/0_stateless/03273_dictionary_rbac.sh
+++ b/tests/queries/0_stateless/03273_dictionary_rbac.sh
@@ -32,17 +32,17 @@ ${CLICKHOUSE_CLIENT} -nm --query "
 # Reading from dictionary via direct SELECT is Ok.
 $CLICKHOUSE_CLIENT -nm --user="${username}" --query "
     SELECT * FROM ${dictname};
-" 2>&1 > /dev/null && echo "Ok."
+" >/dev/null 2>&1 && echo "Ok."
 
 # Reading from dictionary via dictionary storage is Ok.
 $CLICKHOUSE_CLIENT -nm --user="${username}" --query "
     SELECT * FROM ${dicttablename};
-" 2>&1 > /dev/null && echo "Ok."
+" >/dev/null 2>&1 && echo "Ok."
 
 # Reading from dictionary via dictionary table function is Ok.
 $CLICKHOUSE_CLIENT -nm --user="${username}" --query "
     SELECT * FROM dictionary(${dictname});
-" 2>&1 > /dev/null && echo "Ok."
+" >/dev/null 2>&1 && echo "Ok."
 
 # Function dictGet requires a permission dictGet to use.
 $CLICKHOUSE_CLIENT -nm --user="${username}" --query "