From 747a6a5cd3083022769332def61b71ec46eb2d2b Mon Sep 17 00:00:00 2001
From: robot-clickhouse <robot-clickhouse@users.noreply.github.com>
Date: Tue, 19 Nov 2024 07:07:45 +0000
Subject: [PATCH] Backport #72051 to 24.9: Correct permissions for dictionaries

---
 src/Storages/StorageDictionary.cpp            | 14 +++++++-
 ...erence => 03273_dictionary_rbac.reference} |  4 ++-
 ...ble_access.sh => 03273_dictionary_rbac.sh} | 33 ++++++++++++++-----
 3 files changed, 40 insertions(+), 11 deletions(-)
 rename tests/queries/0_stateless/{03199_dictionary_table_access.reference => 03273_dictionary_rbac.reference} (50%)
 rename tests/queries/0_stateless/{03199_dictionary_table_access.sh => 03273_dictionary_rbac.sh} (53%)

diff --git a/src/Storages/StorageDictionary.cpp b/src/Storages/StorageDictionary.cpp
index 4a290ca3a94..af31cf8a512 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, 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/03273_dictionary_rbac.reference
similarity index 50%
rename from tests/queries/0_stateless/03199_dictionary_table_access.reference
rename to tests/queries/0_stateless/03273_dictionary_rbac.reference
index 4a703b3be84..f7ff98213ba 100644
--- a/tests/queries/0_stateless/03199_dictionary_table_access.reference
+++ b/tests/queries/0_stateless/03273_dictionary_rbac.reference
@@ -1,2 +1,4 @@
-ACCESS_DENIED
+Ok.
+Ok.
+Ok.
 ACCESS_DENIED
diff --git a/tests/queries/0_stateless/03199_dictionary_table_access.sh b/tests/queries/0_stateless/03273_dictionary_rbac.sh
similarity index 53%
rename from tests/queries/0_stateless/03199_dictionary_table_access.sh
rename to tests/queries/0_stateless/03273_dictionary_rbac.sh
index 14f017c7fbc..d74039b4d61 100755
--- a/tests/queries/0_stateless/03199_dictionary_table_access.sh
+++ b/tests/queries/0_stateless/03273_dictionary_rbac.sh
@@ -8,7 +8,8 @@ username="user_${CLICKHOUSE_TEST_UNIQUE_NAME}"
 dictname="dict_${CLICKHOUSE_TEST_UNIQUE_NAME}"
 dicttablename="dict_table_${CLICKHOUSE_TEST_UNIQUE_NAME}"
 
-${CLICKHOUSE_CLIENT} -m --query "
+# Create a dictionary and a table that points to the dictionary.
+${CLICKHOUSE_CLIENT} -nm --query "
     CREATE DICTIONARY IF NOT EXISTS ${dictname}
     (
         id UInt64,
@@ -18,23 +19,37 @@ ${CLICKHOUSE_CLIENT} -m --query "
     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 "
+# 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 | grep -o ACCESS_DENIED | uniq
+" >/dev/null 2>&1 && echo "Ok."
 
-$CLICKHOUSE_CLIENT -m --user="${username}" --query "
+# Reading from dictionary via dictionary storage is Ok.
+$CLICKHOUSE_CLIENT -nm --user="${username}" --query "
     SELECT * FROM ${dicttablename};
+" >/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});
+" >/dev/null 2>&1 && 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} -m --query "
+${CLICKHOUSE_CLIENT} -nm --query "
     DROP TABLE IF EXISTS ${dicttablename} SYNC;
     DROP DICTIONARY IF EXISTS ${dictname};
     DROP USER IF EXISTS ${username};