From deda29b46b597d87c584521e32f96537b4241712 Mon Sep 17 00:00:00 2001 From: Robert Schulze Date: Fri, 15 Jul 2022 11:15:46 +0000 Subject: [PATCH 01/10] Pass const StringRef by value, not by reference See #39224 --- src/AggregateFunctions/ThetaSketchData.h | 2 +- src/Columns/ColumnUnique.h | 2 +- src/Columns/ReverseIndex.h | 8 ++--- src/Common/HashTable/StringHashMap.h | 6 ++-- src/Common/SpaceSaving.h | 4 +-- src/Common/TLDListsHolder.cpp | 4 +-- src/Common/TLDListsHolder.h | 4 +-- src/Common/quoteString.cpp | 6 ++-- src/Common/quoteString.h | 6 ++-- src/Dictionaries/FlatDictionary.cpp | 4 +-- src/Dictionaries/HashedArrayDictionary.cpp | 4 +-- src/Dictionaries/HashedDictionary.cpp | 4 +-- src/Dictionaries/IPAddressDictionary.cpp | 2 +- src/Dictionaries/RangeHashedDictionary.cpp | 8 ++--- src/Functions/FunctionsAES.cpp | 4 +-- src/Functions/FunctionsAES.h | 10 +++--- src/Functions/isIPAddressContainedIn.cpp | 4 +-- src/IO/ReadHelpers.cpp | 2 +- src/IO/ReadHelpers.h | 2 +- src/IO/WriteHelpers.cpp | 14 ++++---- src/IO/WriteHelpers.h | 34 +++++++++---------- src/Interpreters/Aggregator.h | 10 +++--- src/Interpreters/Context.cpp | 4 +-- src/Interpreters/Context.h | 4 +-- .../Impl/JSONEachRowRowInputFormat.cpp | 4 +-- .../Formats/Impl/JSONEachRowRowInputFormat.h | 4 +-- src/Processors/Merges/Algorithms/Graphite.cpp | 2 +- src/Server/HTTPHandlerRequestFilter.h | 4 +-- 28 files changed, 83 insertions(+), 83 deletions(-) diff --git a/src/AggregateFunctions/ThetaSketchData.h b/src/AggregateFunctions/ThetaSketchData.h index cc35597ba56..f46836ad189 100644 --- a/src/AggregateFunctions/ThetaSketchData.h +++ b/src/AggregateFunctions/ThetaSketchData.h @@ -43,7 +43,7 @@ public: ~ThetaSketchData() = default; /// Insert original value without hash, as `datasketches::update_theta_sketch.update` will do the hash internal. - void insertOriginal(const StringRef & value) + void insertOriginal(StringRef value) { getSkUpdate()->update(value.data, value.size); } diff --git a/src/Columns/ColumnUnique.h b/src/Columns/ColumnUnique.h index 33135224e11..3c21a65e404 100644 --- a/src/Columns/ColumnUnique.h +++ b/src/Columns/ColumnUnique.h @@ -509,7 +509,7 @@ MutableColumnPtr ColumnUnique::uniqueInsertRangeImpl( if (secondary_index) next_position += secondary_index->size(); - auto insert_key = [&](const StringRef & ref, ReverseIndex & cur_index) -> MutableColumnPtr + auto insert_key = [&](StringRef ref, ReverseIndex & cur_index) -> MutableColumnPtr { auto inserted_pos = cur_index.insert(ref); positions[num_added_rows] = inserted_pos; diff --git a/src/Columns/ReverseIndex.h b/src/Columns/ReverseIndex.h index 3f4427e17ad..ba6a014b49d 100644 --- a/src/Columns/ReverseIndex.h +++ b/src/Columns/ReverseIndex.h @@ -92,7 +92,7 @@ struct ReverseIndexHashTableCell /// Special case when we want to compare with something not in index_column. /// When we compare something inside column default keyEquals checks only that row numbers are equal. - bool keyEquals(const StringRef & object, size_t hash_ [[maybe_unused]], const State & state) const + bool keyEquals(StringRef object, size_t hash_ [[maybe_unused]], const State & state) const { auto index = key; if constexpr (has_base_index) @@ -322,7 +322,7 @@ public: static constexpr bool is_numeric_column = isNumericColumn(static_cast(nullptr)); static constexpr bool use_saved_hash = !is_numeric_column; - UInt64 insert(const StringRef & data); + UInt64 insert(StringRef data); /// Returns the found data's index in the dictionary. If index is not built, builds it. UInt64 getInsertionPoint(StringRef data) @@ -383,7 +383,7 @@ private: void buildIndex(); - UInt64 getHash(const StringRef & ref) const + UInt64 getHash(StringRef ref) const { if constexpr (is_numeric_column) { @@ -478,7 +478,7 @@ ColumnUInt64::MutablePtr ReverseIndex::calcHashes() const } template -UInt64 ReverseIndex::insert(const StringRef & data) +UInt64 ReverseIndex::insert(StringRef data) { if (!index) buildIndex(); diff --git a/src/Common/HashTable/StringHashMap.h b/src/Common/HashTable/StringHashMap.h index a3b5c3e9ed0..ada10180786 100644 --- a/src/Common/HashTable/StringHashMap.h +++ b/src/Common/HashTable/StringHashMap.h @@ -12,7 +12,7 @@ struct StringHashMapCell : public HashMapCellvalue.first); } /// NOLINT + StringRef getKey() const { return toStringRef(this->value.first); } /// NOLINT // internal static const Key & getKey(const value_type & value_) { return value_.first; } }; @@ -32,7 +32,7 @@ struct StringHashMapCell : public HashMapCellvalue.first.items[1] = 0; } // external - const StringRef getKey() const { return toStringRef(this->value.first); } /// NOLINT + StringRef getKey() const { return toStringRef(this->value.first); } /// NOLINT // internal static const StringKey16 & getKey(const value_type & value_) { return value_.first; } }; @@ -53,7 +53,7 @@ struct StringHashMapCell : public HashMapCellvalue.first.c = 0; } // external - const StringRef getKey() const { return toStringRef(this->value.first); } /// NOLINT + StringRef getKey() const { return toStringRef(this->value.first); } /// NOLINT // internal static const StringKey24 & getKey(const value_type & value_) { return value_.first; } }; diff --git a/src/Common/SpaceSaving.h b/src/Common/SpaceSaving.h index 48817d8c926..0f577349722 100644 --- a/src/Common/SpaceSaving.h +++ b/src/Common/SpaceSaving.h @@ -49,12 +49,12 @@ struct SpaceSavingArena template <> struct SpaceSavingArena { - StringRef emplace(const StringRef & key) + StringRef emplace(StringRef key) { return copyStringInArena(arena, key); } - void free(const StringRef & key) + void free(StringRef key) { if (key.data) arena.free(const_cast(key.data), key.size); diff --git a/src/Common/TLDListsHolder.cpp b/src/Common/TLDListsHolder.cpp index 3e5649a5ac6..a3019ac1c49 100644 --- a/src/Common/TLDListsHolder.cpp +++ b/src/Common/TLDListsHolder.cpp @@ -20,13 +20,13 @@ TLDList::TLDList(size_t size) : tld_container(size) , pool(std::make_unique(10 << 20)) {} -bool TLDList::insert(const StringRef & host) +bool TLDList::insert(StringRef host) { bool inserted; tld_container.emplace(DB::ArenaKeyHolder{host, *pool}, inserted); return inserted; } -bool TLDList::has(const StringRef & host) const +bool TLDList::has(StringRef host) const { return tld_container.has(host); } diff --git a/src/Common/TLDListsHolder.h b/src/Common/TLDListsHolder.h index 708d049d5a6..e8acefb1b5e 100644 --- a/src/Common/TLDListsHolder.h +++ b/src/Common/TLDListsHolder.h @@ -23,9 +23,9 @@ public: explicit TLDList(size_t size); /// Return true if the tld_container does not contains such element. - bool insert(const StringRef & host); + bool insert(StringRef host); /// Check is there such TLD - bool has(const StringRef & host) const; + bool has(StringRef host) const; size_t size() const { return tld_container.size(); } private: diff --git a/src/Common/quoteString.cpp b/src/Common/quoteString.cpp index e3e6e0b3249..b464f4837a1 100644 --- a/src/Common/quoteString.cpp +++ b/src/Common/quoteString.cpp @@ -14,7 +14,7 @@ String quoteString(std::string_view x) } -String doubleQuoteString(const StringRef & x) +String doubleQuoteString(StringRef x) { String res(x.size, '\0'); WriteBufferFromString wb(res); @@ -23,7 +23,7 @@ String doubleQuoteString(const StringRef & x) } -String backQuote(const StringRef & x) +String backQuote(StringRef x) { String res(x.size, '\0'); { @@ -34,7 +34,7 @@ String backQuote(const StringRef & x) } -String backQuoteIfNeed(const StringRef & x) +String backQuoteIfNeed(StringRef x) { String res(x.size, '\0'); { diff --git a/src/Common/quoteString.h b/src/Common/quoteString.h index 73c0de03d45..b83988258e2 100644 --- a/src/Common/quoteString.h +++ b/src/Common/quoteString.h @@ -16,12 +16,12 @@ namespace DB } /// Double quote the string. -String doubleQuoteString(const StringRef & x); +String doubleQuoteString(StringRef x); /// Quote the identifier with backquotes. -String backQuote(const StringRef & x); +String backQuote(StringRef x); /// Quote the identifier with backquotes, if required. -String backQuoteIfNeed(const StringRef & x); +String backQuoteIfNeed(StringRef x); } diff --git a/src/Dictionaries/FlatDictionary.cpp b/src/Dictionaries/FlatDictionary.cpp index d77f0bf825c..c858618c5ff 100644 --- a/src/Dictionaries/FlatDictionary.cpp +++ b/src/Dictionaries/FlatDictionary.cpp @@ -105,7 +105,7 @@ ColumnPtr FlatDictionary::getColumn( getItemsImpl( attribute, ids, - [&](size_t row, const StringRef value, bool is_null) + [&](size_t row, StringRef value, bool is_null) { (*vec_null_map_to)[row] = is_null; out->insertData(value.data, value.size); @@ -115,7 +115,7 @@ ColumnPtr FlatDictionary::getColumn( getItemsImpl( attribute, ids, - [&](size_t, const StringRef value, bool) { out->insertData(value.data, value.size); }, + [&](size_t, StringRef value, bool) { out->insertData(value.data, value.size); }, default_value_extractor); } else diff --git a/src/Dictionaries/HashedArrayDictionary.cpp b/src/Dictionaries/HashedArrayDictionary.cpp index d702a02bc2e..b8ed664e91a 100644 --- a/src/Dictionaries/HashedArrayDictionary.cpp +++ b/src/Dictionaries/HashedArrayDictionary.cpp @@ -585,7 +585,7 @@ ColumnPtr HashedArrayDictionary::getAttributeColumn( getItemsImpl( attribute, keys_object, - [&](size_t row, const StringRef value, bool is_null) + [&](size_t row, StringRef value, bool is_null) { (*vec_null_map_to)[row] = is_null; out->insertData(value.data, value.size); @@ -595,7 +595,7 @@ ColumnPtr HashedArrayDictionary::getAttributeColumn( getItemsImpl( attribute, keys_object, - [&](size_t, const StringRef value, bool) { out->insertData(value.data, value.size); }, + [&](size_t, StringRef value, bool) { out->insertData(value.data, value.size); }, default_value_extractor); } else diff --git a/src/Dictionaries/HashedDictionary.cpp b/src/Dictionaries/HashedDictionary.cpp index c5160c0dfa8..9beac59f274 100644 --- a/src/Dictionaries/HashedDictionary.cpp +++ b/src/Dictionaries/HashedDictionary.cpp @@ -117,7 +117,7 @@ ColumnPtr HashedDictionary::getColumn( getItemsImpl( attribute, extractor, - [&](size_t row, const StringRef value, bool is_null) + [&](size_t row, StringRef value, bool is_null) { (*vec_null_map_to)[row] = is_null; out->insertData(value.data, value.size); @@ -127,7 +127,7 @@ ColumnPtr HashedDictionary::getColumn( getItemsImpl( attribute, extractor, - [&](size_t, const StringRef value, bool) { out->insertData(value.data, value.size); }, + [&](size_t, StringRef value, bool) { out->insertData(value.data, value.size); }, default_value_extractor); } else diff --git a/src/Dictionaries/IPAddressDictionary.cpp b/src/Dictionaries/IPAddressDictionary.cpp index 46cba702b5d..2a367323205 100644 --- a/src/Dictionaries/IPAddressDictionary.cpp +++ b/src/Dictionaries/IPAddressDictionary.cpp @@ -261,7 +261,7 @@ ColumnPtr IPAddressDictionary::getColumn( getItemsImpl( attribute, key_columns, - [&](const size_t, const StringRef value) { out->insertData(value.data, value.size); }, + [&](const size_t, StringRef value) { out->insertData(value.data, value.size); }, default_value_extractor); } else diff --git a/src/Dictionaries/RangeHashedDictionary.cpp b/src/Dictionaries/RangeHashedDictionary.cpp index 261e9166ec8..ad962ca4acc 100644 --- a/src/Dictionaries/RangeHashedDictionary.cpp +++ b/src/Dictionaries/RangeHashedDictionary.cpp @@ -151,7 +151,7 @@ ColumnPtr RangeHashedDictionary::getColumn( getItemsImpl( attribute, modified_key_columns, - [&](size_t row, const StringRef value, bool is_null) + [&](size_t row, StringRef value, bool is_null) { (*vec_null_map_to)[row] = is_null; out->insertData(value.data, value.size); @@ -161,7 +161,7 @@ ColumnPtr RangeHashedDictionary::getColumn( getItemsImpl( attribute, modified_key_columns, - [&](size_t, const StringRef value, bool) + [&](size_t, StringRef value, bool) { out->insertData(value.data, value.size); }, @@ -255,7 +255,7 @@ ColumnPtr RangeHashedDictionary::getColumnInternal( getItemsInternalImpl( attribute, key_to_index, - [&](size_t row, const StringRef value, bool is_null) + [&](size_t row, StringRef value, bool is_null) { (*vec_null_map_to)[row] = is_null; out->insertData(value.data, value.size); @@ -264,7 +264,7 @@ ColumnPtr RangeHashedDictionary::getColumnInternal( getItemsInternalImpl( attribute, key_to_index, - [&](size_t, const StringRef value, bool) + [&](size_t, StringRef value, bool) { out->insertData(value.data, value.size); }); diff --git a/src/Functions/FunctionsAES.cpp b/src/Functions/FunctionsAES.cpp index a2dc7e40489..9ef07e2747d 100644 --- a/src/Functions/FunctionsAES.cpp +++ b/src/Functions/FunctionsAES.cpp @@ -25,7 +25,7 @@ void onError(std::string error_message) throw DB::Exception(error_message, DB::ErrorCodes::OPENSSL_ERROR); } -StringRef foldEncryptionKeyInMySQLCompatitableMode(size_t cipher_key_size, const StringRef & key, std::array & folded_key) +StringRef foldEncryptionKeyInMySQLCompatitableMode(size_t cipher_key_size, StringRef key, std::array & folded_key) { assert(cipher_key_size <= EVP_MAX_KEY_LENGTH); memcpy(folded_key.data(), key.data, cipher_key_size); @@ -38,7 +38,7 @@ StringRef foldEncryptionKeyInMySQLCompatitableMode(size_t cipher_key_size, const return StringRef(folded_key.data(), cipher_key_size); } -const EVP_CIPHER * getCipherByName(const StringRef & cipher_name) +const EVP_CIPHER * getCipherByName(StringRef cipher_name) { // NOTE: cipher obtained not via EVP_CIPHER_fetch() would cause extra work on each context reset // with EVP_CIPHER_CTX_reset() or EVP_EncryptInit_ex(), but using EVP_CIPHER_fetch() diff --git a/src/Functions/FunctionsAES.h b/src/Functions/FunctionsAES.h index d3796081f18..d3c533c804b 100644 --- a/src/Functions/FunctionsAES.h +++ b/src/Functions/FunctionsAES.h @@ -32,9 +32,9 @@ namespace ErrorCodes namespace OpenSSLDetails { [[noreturn]] void onError(std::string error_message); -StringRef foldEncryptionKeyInMySQLCompatitableMode(size_t cipher_key_size, const StringRef & key, std::array & folded_key); +StringRef foldEncryptionKeyInMySQLCompatitableMode(size_t cipher_key_size, StringRef key, std::array & folded_key); -const EVP_CIPHER * getCipherByName(const StringRef & name); +const EVP_CIPHER * getCipherByName(StringRef name); enum class CompatibilityMode { @@ -53,7 +53,7 @@ enum class CipherMode template struct KeyHolder { - inline StringRef setKey(size_t cipher_key_size, const StringRef & key) const + inline StringRef setKey(size_t cipher_key_size, StringRef key) const { if (key.size != cipher_key_size) throw DB::Exception(fmt::format("Invalid key size: {} expected {}", key.size, cipher_key_size), @@ -66,7 +66,7 @@ struct KeyHolder template <> struct KeyHolder { - inline StringRef setKey(size_t cipher_key_size, const StringRef & key) + inline StringRef setKey(size_t cipher_key_size, StringRef key) { if (key.size < cipher_key_size) throw DB::Exception(fmt::format("Invalid key size: {} expected {}", key.size, cipher_key_size), @@ -120,7 +120,7 @@ inline void validateCipherMode(const EVP_CIPHER * evp_cipher) } template -inline void validateIV(const StringRef & iv_value, const size_t cipher_iv_size) +inline void validateIV(StringRef iv_value, const size_t cipher_iv_size) { // In MySQL mode we don't care if IV is longer than expected, only if shorter. if ((mode == CipherMode::MySQLCompatibility && iv_value.size != 0 && iv_value.size < cipher_iv_size) diff --git a/src/Functions/isIPAddressContainedIn.cpp b/src/Functions/isIPAddressContainedIn.cpp index 5ef247f7346..6fdc0dfbee8 100644 --- a/src/Functions/isIPAddressContainedIn.cpp +++ b/src/Functions/isIPAddressContainedIn.cpp @@ -27,7 +27,7 @@ class IPAddressVariant { public: - explicit IPAddressVariant(const StringRef & address_str) + explicit IPAddressVariant(StringRef address_str) { /// IP address parser functions require that the input is /// NULL-terminated so we need to copy it. @@ -75,7 +75,7 @@ struct IPAddressCIDR UInt8 prefix; }; -IPAddressCIDR parseIPWithCIDR(const StringRef cidr_str) +IPAddressCIDR parseIPWithCIDR(StringRef cidr_str) { std::string_view cidr_str_view(cidr_str); size_t pos_slash = cidr_str_view.find('/'); diff --git a/src/IO/ReadHelpers.cpp b/src/IO/ReadHelpers.cpp index f09292cd349..c2b0a0f65d7 100644 --- a/src/IO/ReadHelpers.cpp +++ b/src/IO/ReadHelpers.cpp @@ -1053,7 +1053,7 @@ template void readDateTimeTextFallback(time_t &, ReadBuffer &, const DateL template bool readDateTimeTextFallback(time_t &, ReadBuffer &, const DateLUTImpl &); -void skipJSONField(ReadBuffer & buf, const StringRef & name_of_field) +void skipJSONField(ReadBuffer & buf, StringRef name_of_field) { if (buf.eof()) throw Exception("Unexpected EOF for key '" + name_of_field.toString() + "'", ErrorCodes::INCORRECT_DATA); diff --git a/src/IO/ReadHelpers.h b/src/IO/ReadHelpers.h index 57283a396d9..7a5df1ed5ac 100644 --- a/src/IO/ReadHelpers.h +++ b/src/IO/ReadHelpers.h @@ -1238,7 +1238,7 @@ inline void skipWhitespaceIfAny(ReadBuffer & buf, bool one_line = false) } /// Skips json value. -void skipJSONField(ReadBuffer & buf, const StringRef & name_of_field); +void skipJSONField(ReadBuffer & buf, StringRef name_of_field); /** Read serialized exception. diff --git a/src/IO/WriteHelpers.cpp b/src/IO/WriteHelpers.cpp index a6d492b85b0..fae3d21513e 100644 --- a/src/IO/WriteHelpers.cpp +++ b/src/IO/WriteHelpers.cpp @@ -66,7 +66,7 @@ void writeException(const Exception & e, WriteBuffer & buf, bool with_stack_trac /// The same, but quotes apply only if there are characters that do not match the identifier without quotes template -static inline void writeProbablyQuotedStringImpl(const StringRef & s, WriteBuffer & buf, F && write_quoted_string) +static inline void writeProbablyQuotedStringImpl(StringRef s, WriteBuffer & buf, F && write_quoted_string) { if (isValidIdentifier(std::string_view{s}) /// This are valid identifiers but are problematic if present unquoted in SQL query. @@ -79,19 +79,19 @@ static inline void writeProbablyQuotedStringImpl(const StringRef & s, WriteBuffe write_quoted_string(s, buf); } -void writeProbablyBackQuotedString(const StringRef & s, WriteBuffer & buf) +void writeProbablyBackQuotedString(StringRef s, WriteBuffer & buf) { - writeProbablyQuotedStringImpl(s, buf, [](const StringRef & s_, WriteBuffer & buf_) { return writeBackQuotedString(s_, buf_); }); + writeProbablyQuotedStringImpl(s, buf, [](StringRef s_, WriteBuffer & buf_) { return writeBackQuotedString(s_, buf_); }); } -void writeProbablyDoubleQuotedString(const StringRef & s, WriteBuffer & buf) +void writeProbablyDoubleQuotedString(StringRef s, WriteBuffer & buf) { - writeProbablyQuotedStringImpl(s, buf, [](const StringRef & s_, WriteBuffer & buf_) { return writeDoubleQuotedString(s_, buf_); }); + writeProbablyQuotedStringImpl(s, buf, [](StringRef s_, WriteBuffer & buf_) { return writeDoubleQuotedString(s_, buf_); }); } -void writeProbablyBackQuotedStringMySQL(const StringRef & s, WriteBuffer & buf) +void writeProbablyBackQuotedStringMySQL(StringRef s, WriteBuffer & buf) { - writeProbablyQuotedStringImpl(s, buf, [](const StringRef & s_, WriteBuffer & buf_) { return writeBackQuotedStringMySQL(s_, buf_); }); + writeProbablyQuotedStringImpl(s, buf, [](StringRef s_, WriteBuffer & buf_) { return writeBackQuotedStringMySQL(s_, buf_); }); } void writePointerHex(const void * ptr, WriteBuffer & buf) diff --git a/src/IO/WriteHelpers.h b/src/IO/WriteHelpers.h index c3bbaac097d..6f35dae8300 100644 --- a/src/IO/WriteHelpers.h +++ b/src/IO/WriteHelpers.h @@ -102,7 +102,7 @@ inline void writeStringBinary(const std::string & s, WriteBuffer & buf) buf.write(s.data(), s.size()); } -inline void writeStringBinary(const StringRef & s, WriteBuffer & buf) +inline void writeStringBinary(StringRef s, WriteBuffer & buf) { writeVarUInt(s.size, buf); buf.write(s.data, s.size); @@ -360,7 +360,7 @@ void writeAnyEscapedString(const char * begin, const char * end, WriteBuffer & b } -inline void writeJSONString(const StringRef & s, WriteBuffer & buf, const FormatSettings & settings) +inline void writeJSONString(StringRef s, WriteBuffer & buf, const FormatSettings & settings) { writeJSONString(s.data, s.data + s.size, buf, settings); } @@ -435,7 +435,7 @@ inline void writeEscapedString(const String & s, WriteBuffer & buf) } -inline void writeEscapedString(const StringRef & ref, WriteBuffer & buf) +inline void writeEscapedString(StringRef ref, WriteBuffer & buf) { writeEscapedString(ref.data, ref.size, buf); } @@ -462,7 +462,7 @@ void writeAnyQuotedString(const String & s, WriteBuffer & buf) template -void writeAnyQuotedString(const StringRef & ref, WriteBuffer & buf) +void writeAnyQuotedString(StringRef ref, WriteBuffer & buf) { writeAnyQuotedString(ref.data, ref.data + ref.size, buf); } @@ -473,7 +473,7 @@ inline void writeQuotedString(const String & s, WriteBuffer & buf) writeAnyQuotedString<'\''>(s, buf); } -inline void writeQuotedString(const StringRef & ref, WriteBuffer & buf) +inline void writeQuotedString(StringRef ref, WriteBuffer & buf) { writeAnyQuotedString<'\''>(ref, buf); } @@ -488,7 +488,7 @@ inline void writeDoubleQuotedString(const String & s, WriteBuffer & buf) writeAnyQuotedString<'"'>(s, buf); } -inline void writeDoubleQuotedString(const StringRef & s, WriteBuffer & buf) +inline void writeDoubleQuotedString(StringRef s, WriteBuffer & buf) { writeAnyQuotedString<'"'>(s, buf); } @@ -499,13 +499,13 @@ inline void writeDoubleQuotedString(std::string_view s, WriteBuffer & buf) } /// Outputs a string in backquotes. -inline void writeBackQuotedString(const StringRef & s, WriteBuffer & buf) +inline void writeBackQuotedString(StringRef s, WriteBuffer & buf) { writeAnyQuotedString<'`'>(s, buf); } /// Outputs a string in backquotes for MySQL. -inline void writeBackQuotedStringMySQL(const StringRef & s, WriteBuffer & buf) +inline void writeBackQuotedStringMySQL(StringRef s, WriteBuffer & buf) { writeChar('`', buf); writeAnyEscapedString<'`', true>(s.data, s.data + s.size, buf); @@ -514,9 +514,9 @@ inline void writeBackQuotedStringMySQL(const StringRef & s, WriteBuffer & buf) /// Write quoted if the string doesn't look like and identifier. -void writeProbablyBackQuotedString(const StringRef & s, WriteBuffer & buf); -void writeProbablyDoubleQuotedString(const StringRef & s, WriteBuffer & buf); -void writeProbablyBackQuotedStringMySQL(const StringRef & s, WriteBuffer & buf); +void writeProbablyBackQuotedString(StringRef s, WriteBuffer & buf); +void writeProbablyDoubleQuotedString(StringRef s, WriteBuffer & buf); +void writeProbablyBackQuotedStringMySQL(StringRef s, WriteBuffer & buf); /** Outputs the string in for the CSV format. @@ -559,7 +559,7 @@ void writeCSVString(const String & s, WriteBuffer & buf) } template -void writeCSVString(const StringRef & s, WriteBuffer & buf) +void writeCSVString(StringRef s, WriteBuffer & buf) { writeCSVString(s.data, s.data + s.size, buf); } @@ -616,7 +616,7 @@ inline void writeXMLStringForTextElementOrAttributeValue(const String & s, Write writeXMLStringForTextElementOrAttributeValue(s.data(), s.data() + s.size(), buf); } -inline void writeXMLStringForTextElementOrAttributeValue(const StringRef & s, WriteBuffer & buf) +inline void writeXMLStringForTextElementOrAttributeValue(StringRef s, WriteBuffer & buf) { writeXMLStringForTextElementOrAttributeValue(s.data, s.data + s.size, buf); } @@ -657,7 +657,7 @@ inline void writeXMLStringForTextElement(const String & s, WriteBuffer & buf) writeXMLStringForTextElement(s.data(), s.data() + s.size(), buf); } -inline void writeXMLStringForTextElement(const StringRef & s, WriteBuffer & buf) +inline void writeXMLStringForTextElement(StringRef s, WriteBuffer & buf) { writeXMLStringForTextElement(s.data, s.data + s.size, buf); } @@ -890,7 +890,7 @@ requires is_arithmetic_v inline void writeBinary(const T & x, WriteBuffer & buf) { writePODBinary(x, buf); } inline void writeBinary(const String & x, WriteBuffer & buf) { writeStringBinary(x, buf); } -inline void writeBinary(const StringRef & x, WriteBuffer & buf) { writeStringBinary(x, buf); } +inline void writeBinary(StringRef x, WriteBuffer & buf) { writeStringBinary(x, buf); } inline void writeBinary(std::string_view x, WriteBuffer & buf) { writeStringBinary(x, buf); } inline void writeBinary(const Decimal32 & x, WriteBuffer & buf) { writePODBinary(x, buf); } inline void writeBinary(const Decimal64 & x, WriteBuffer & buf) { writePODBinary(x, buf); } @@ -1017,7 +1017,7 @@ inline void writeQuoted(const String & x, WriteBuffer & buf) { writeQuotedString inline void writeQuoted(std::string_view x, WriteBuffer & buf) { writeQuotedString(x, buf); } -inline void writeQuoted(const StringRef & x, WriteBuffer & buf) { writeQuotedString(x, buf); } +inline void writeQuoted(StringRef x, WriteBuffer & buf) { writeQuotedString(x, buf); } inline void writeQuoted(const LocalDate & x, WriteBuffer & buf) { @@ -1050,7 +1050,7 @@ inline void writeDoubleQuoted(const String & x, WriteBuffer & buf) { writeDouble inline void writeDoubleQuoted(std::string_view x, WriteBuffer & buf) { writeDoubleQuotedString(x, buf); } -inline void writeDoubleQuoted(const StringRef & x, WriteBuffer & buf) { writeDoubleQuotedString(x, buf); } +inline void writeDoubleQuoted(StringRef x, WriteBuffer & buf) { writeDoubleQuotedString(x, buf); } inline void writeDoubleQuoted(const LocalDate & x, WriteBuffer & buf) { diff --git a/src/Interpreters/Aggregator.h b/src/Interpreters/Aggregator.h index feb07727725..716849465de 100644 --- a/src/Interpreters/Aggregator.h +++ b/src/Interpreters/Aggregator.h @@ -238,7 +238,7 @@ struct AggregationMethodString std::optional shuffleKeyColumns(std::vector &, const Sizes &) { return {}; } - static void insertKeyIntoColumns(const StringRef & key, std::vector & key_columns, const Sizes &) + static void insertKeyIntoColumns(StringRef key, std::vector & key_columns, const Sizes &) { static_cast(key_columns[0])->insertData(key.data, key.size); } @@ -270,7 +270,7 @@ struct AggregationMethodStringNoCache std::optional shuffleKeyColumns(std::vector &, const Sizes &) { return {}; } - static void insertKeyIntoColumns(const StringRef & key, std::vector & key_columns, const Sizes &) + static void insertKeyIntoColumns(StringRef key, std::vector & key_columns, const Sizes &) { static_cast(key_columns[0])->insertData(key.data, key.size); } @@ -302,7 +302,7 @@ struct AggregationMethodFixedString std::optional shuffleKeyColumns(std::vector &, const Sizes &) { return {}; } - static void insertKeyIntoColumns(const StringRef & key, std::vector & key_columns, const Sizes &) + static void insertKeyIntoColumns(StringRef key, std::vector & key_columns, const Sizes &) { static_cast(key_columns[0])->insertData(key.data, key.size); } @@ -333,7 +333,7 @@ struct AggregationMethodFixedStringNoCache std::optional shuffleKeyColumns(std::vector &, const Sizes &) { return {}; } - static void insertKeyIntoColumns(const StringRef & key, std::vector & key_columns, const Sizes &) + static void insertKeyIntoColumns(StringRef key, std::vector & key_columns, const Sizes &) { static_cast(key_columns[0])->insertData(key.data, key.size); } @@ -501,7 +501,7 @@ struct AggregationMethodSerialized std::optional shuffleKeyColumns(std::vector &, const Sizes &) { return {}; } - static void insertKeyIntoColumns(const StringRef & key, std::vector & key_columns, const Sizes &) + static void insertKeyIntoColumns(StringRef key, std::vector & key_columns, const Sizes &) { const auto * pos = key.data; for (auto & column : key_columns) diff --git a/src/Interpreters/Context.cpp b/src/Interpreters/Context.cpp index cbbcc58df2e..fe59215f7d5 100644 --- a/src/Interpreters/Context.cpp +++ b/src/Interpreters/Context.cpp @@ -1191,7 +1191,7 @@ void Context::setSettings(const Settings & settings_) } -void Context::setSetting(const StringRef & name, const String & value) +void Context::setSetting(StringRef name, const String & value) { auto lock = getLock(); if (name == "profile") @@ -1206,7 +1206,7 @@ void Context::setSetting(const StringRef & name, const String & value) } -void Context::setSetting(const StringRef & name, const Field & value) +void Context::setSetting(StringRef name, const Field & value) { auto lock = getLock(); if (name == "profile") diff --git a/src/Interpreters/Context.h b/src/Interpreters/Context.h index ca0c218a4c0..e7aba31a1d9 100644 --- a/src/Interpreters/Context.h +++ b/src/Interpreters/Context.h @@ -607,8 +607,8 @@ public: void setSettings(const Settings & settings_); /// Set settings by name. - void setSetting(const StringRef & name, const String & value); - void setSetting(const StringRef & name, const Field & value); + void setSetting(StringRef name, const String & value); + void setSetting(StringRef name, const Field & value); void applySettingChange(const SettingChange & change); void applySettingsChanges(const SettingsChanges & changes); diff --git a/src/Processors/Formats/Impl/JSONEachRowRowInputFormat.cpp b/src/Processors/Formats/Impl/JSONEachRowRowInputFormat.cpp index 9eef72f95da..3bcea8a8843 100644 --- a/src/Processors/Formats/Impl/JSONEachRowRowInputFormat.cpp +++ b/src/Processors/Formats/Impl/JSONEachRowRowInputFormat.cpp @@ -62,7 +62,7 @@ const String & JSONEachRowRowInputFormat::columnName(size_t i) const return getPort().getHeader().getByPosition(i).name; } -inline size_t JSONEachRowRowInputFormat::columnIndex(const StringRef & name, size_t key_index) +inline size_t JSONEachRowRowInputFormat::columnIndex(StringRef name, size_t key_index) { /// Optimization by caching the order of fields (which is almost always the same) /// and a quick check to match the next expected field, instead of searching the hash table. @@ -124,7 +124,7 @@ static inline void skipColonDelimeter(ReadBuffer & istr) skipWhitespaceIfAny(istr); } -void JSONEachRowRowInputFormat::skipUnknownField(const StringRef & name_ref) +void JSONEachRowRowInputFormat::skipUnknownField(StringRef name_ref) { if (!format_settings.skip_unknown_fields) throw Exception("Unknown field found while parsing JSONEachRow format: " + name_ref.toString(), ErrorCodes::INCORRECT_DATA); diff --git a/src/Processors/Formats/Impl/JSONEachRowRowInputFormat.h b/src/Processors/Formats/Impl/JSONEachRowRowInputFormat.h index 1da14a532de..1673d55b9fd 100644 --- a/src/Processors/Formats/Impl/JSONEachRowRowInputFormat.h +++ b/src/Processors/Formats/Impl/JSONEachRowRowInputFormat.h @@ -40,9 +40,9 @@ private: void syncAfterError() override; const String & columnName(size_t i) const; - size_t columnIndex(const StringRef & name, size_t key_index); + size_t columnIndex(StringRef name, size_t key_index); bool advanceToNextKey(size_t key_index); - void skipUnknownField(const StringRef & name_ref); + void skipUnknownField(StringRef name_ref); StringRef readColumnName(ReadBuffer & buf); void readField(size_t index, MutableColumns & columns); void readJSONObject(MutableColumns & columns); diff --git a/src/Processors/Merges/Algorithms/Graphite.cpp b/src/Processors/Merges/Algorithms/Graphite.cpp index f77bb790332..6c4ca5ef85b 100644 --- a/src/Processors/Merges/Algorithms/Graphite.cpp +++ b/src/Processors/Merges/Algorithms/Graphite.cpp @@ -71,7 +71,7 @@ static const Graphite::Pattern undef_pattern = .type = undef_pattern.TypeUndef, }; -inline static const Patterns & selectPatternsForMetricType(const Graphite::Params & params, const StringRef path) +inline static const Patterns & selectPatternsForMetricType(const Graphite::Params & params, StringRef path) { if (params.patterns_typed) { diff --git a/src/Server/HTTPHandlerRequestFilter.h b/src/Server/HTTPHandlerRequestFilter.h index 3236b35d5ae..d0156266fe5 100644 --- a/src/Server/HTTPHandlerRequestFilter.h +++ b/src/Server/HTTPHandlerRequestFilter.h @@ -23,7 +23,7 @@ namespace ErrorCodes using CompiledRegexPtr = std::shared_ptr; -static inline bool checkRegexExpression(const StringRef & match_str, const CompiledRegexPtr & compiled_regex) +static inline bool checkRegexExpression(StringRef match_str, const CompiledRegexPtr & compiled_regex) { int num_captures = compiled_regex->NumberOfCapturingGroups() + 1; @@ -32,7 +32,7 @@ static inline bool checkRegexExpression(const StringRef & match_str, const Compi return compiled_regex->Match(match_input, 0, match_str.size, re2::RE2::Anchor::ANCHOR_BOTH, matches, num_captures); } -static inline bool checkExpression(const StringRef & match_str, const std::pair & expression) +static inline bool checkExpression(StringRef match_str, const std::pair & expression) { if (expression.second) return checkRegexExpression(match_str, expression.second); From c85b2b5732dfa6bfa07f3c4d1ff77f342c75c9a3 Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Thu, 7 Jul 2022 19:17:07 +0200 Subject: [PATCH 02/10] Add option enabling that SELECT from the system database requires grant. --- programs/server/config.xml | 13 ++++ src/Access/AccessControl.cpp | 13 ++-- src/Access/AccessControl.h | 8 +++ src/Access/AccessRights.cpp | 37 +++++------ src/Access/AccessRights.h | 4 +- src/Access/ContextAccess.cpp | 64 ++++++++++++++++--- .../enable_access_control_improvements.xml | 2 + .../helpers/0_common_instance_config.xml | 2 + 8 files changed, 105 insertions(+), 38 deletions(-) diff --git a/programs/server/config.xml b/programs/server/config.xml index 203684a9e00..343e8dc7093 100644 --- a/programs/server/config.xml +++ b/programs/server/config.xml @@ -604,9 +604,22 @@ if this setting is true the user B will see all rows, and if this setting is false the user B will see no rows. By default this setting is false for compatibility with earlier access configurations. --> false + false + + + false + + + false diff --git a/src/Access/AccessControl.cpp b/src/Access/AccessControl.cpp index abd481f0bb6..5d3fc558130 100644 --- a/src/Access/AccessControl.cpp +++ b/src/Access/AccessControl.cpp @@ -165,13 +165,12 @@ void AccessControl::setUpFromMainConfig(const Poco::Util::AbstractConfiguration setNoPasswordAllowed(config_.getBool("allow_no_password", true)); setPlaintextPasswordAllowed(config_.getBool("allow_plaintext_password", true)); - setEnabledUsersWithoutRowPoliciesCanReadRows(config_.getBool( - "access_control_improvements.users_without_row_policies_can_read_rows", - false /* false because we need to be compatible with earlier access configurations */)); - - setOnClusterQueriesRequireClusterGrant(config_.getBool( - "access_control_improvements.on_cluster_queries_require_cluster_grant", - false /* false because we need to be compatible with earlier access configurations */)); + /// Optional improvements in access control system. + /// The default values are false because we need to be compatible with earlier access configurations + setEnabledUsersWithoutRowPoliciesCanReadRows(config_.getBool("access_control_improvements.users_without_row_policies_can_read_rows", false)); + setOnClusterQueriesRequireClusterGrant(config_.getBool("access_control_improvements.on_cluster_queries_require_cluster_grant", false)); + setSelectFromSystemDatabaseRequiresGrant(config_.getBool("access_control_improvements.select_from_system_db_requires_grant", false)); + setSelectFromInformationSchemaDatabaseRequiresGrant(config_.getBool("access_control_improvements.select_from_information_schema_db_requires_grant", false)); addStoragesFromMainConfig(config_, config_path_, get_zookeeper_function_); } diff --git a/src/Access/AccessControl.h b/src/Access/AccessControl.h index 22ff0a488f7..e0571ad370e 100644 --- a/src/Access/AccessControl.h +++ b/src/Access/AccessControl.h @@ -152,6 +152,12 @@ public: void setOnClusterQueriesRequireClusterGrant(bool enable) { on_cluster_queries_require_cluster_grant = enable; } bool doesOnClusterQueriesRequireClusterGrant() const { return on_cluster_queries_require_cluster_grant; } + void setSelectFromSystemDatabaseRequiresGrant(bool enable) { select_from_system_db_requires_grant = enable; } + bool doesSelectFromSystemDatabaseRequireGrant() const { return select_from_system_db_requires_grant; } + + void setSelectFromInformationSchemaDatabaseRequiresGrant(bool enable) { select_from_information_schema_db_requires_grant = enable; } + bool doesSelectFromInformationSchemaDatabaseRequireGrant() const { return select_from_information_schema_db_requires_grant; } + std::shared_ptr getContextAccess( const UUID & user_id, const std::vector & current_roles, @@ -215,6 +221,8 @@ private: std::atomic_bool allow_no_password = true; std::atomic_bool users_without_row_policies_can_read_rows = false; std::atomic_bool on_cluster_queries_require_cluster_grant = false; + std::atomic_bool select_from_system_db_requires_grant = false; + std::atomic_bool select_from_information_schema_db_requires_grant = false; }; } diff --git a/src/Access/AccessRights.cpp b/src/Access/AccessRights.cpp index b6fed3ac912..747e7a91b2c 100644 --- a/src/Access/AccessRights.cpp +++ b/src/Access/AccessRights.cpp @@ -388,11 +388,11 @@ public: return res; } - void modifyFlags(const ModifyFlagsFunction & function, bool & flags_added, bool & flags_removed) + void modifyFlags(const ModifyFlagsFunction & function, bool grant_option, bool & flags_added, bool & flags_removed) { flags_added = false; flags_removed = false; - modifyFlagsRec(function, flags_added, flags_removed); + modifyFlagsRec(function, grant_option, flags_added, flags_removed); if (flags_added || flags_removed) optimizeTree(); } @@ -669,11 +669,11 @@ private: } template - void modifyFlagsRec(const ModifyFlagsFunction & function, bool & flags_added, bool & flags_removed, const ParentNames & ... parent_names) + void modifyFlagsRec(const ModifyFlagsFunction & function, bool grant_option, bool & flags_added, bool & flags_removed, const ParentNames & ... parent_names) { - auto invoke = [&function](const AccessFlags & flags_, const AccessFlags & min_flags_with_children_, const AccessFlags & max_flags_with_children_, std::string_view database_ = {}, std::string_view table_ = {}, std::string_view column_ = {}) -> AccessFlags + auto invoke = [function, grant_option](const AccessFlags & flags_, const AccessFlags & min_flags_with_children_, const AccessFlags & max_flags_with_children_, std::string_view database_ = {}, std::string_view table_ = {}, std::string_view column_ = {}) -> AccessFlags { - return function(flags_, min_flags_with_children_, max_flags_with_children_, database_, table_, column_); + return function(flags_, min_flags_with_children_, max_flags_with_children_, database_, table_, column_, grant_option); }; if constexpr (sizeof...(ParentNames) < 3) @@ -683,7 +683,7 @@ private: for (auto & child : *children | boost::adaptors::map_values) { const String & child_name = *child.node_name; - child.modifyFlagsRec(function, flags_added, flags_removed, parent_names..., child_name); + child.modifyFlagsRec(function, grant_option, flags_added, flags_removed, parent_names..., child_name); } } } @@ -1062,24 +1062,21 @@ void AccessRights::modifyFlags(const ModifyFlagsFunction & function) { if (!root) return; + bool flags_added, flags_removed; - root->modifyFlags(function, flags_added, flags_removed); + root->modifyFlags(function, false, flags_added, flags_removed); if (flags_removed && root_with_grant_option) root_with_grant_option->makeIntersection(*root); -} - - -void AccessRights::modifyFlagsWithGrantOption(const ModifyFlagsFunction & function) -{ - if (!root_with_grant_option) - return; - bool flags_added, flags_removed; - root_with_grant_option->modifyFlags(function, flags_added, flags_removed); - if (flags_added) + + if (root_with_grant_option) { - if (!root) - root = std::make_unique(); - root->makeUnion(*root_with_grant_option); + root_with_grant_option->modifyFlags(function, true, flags_added, flags_removed); + if (flags_added) + { + if (!root) + root = std::make_unique(); + root->makeUnion(*root_with_grant_option); + } } } diff --git a/src/Access/AccessRights.h b/src/Access/AccessRights.h index 80e37561c2b..5efffc0037a 100644 --- a/src/Access/AccessRights.h +++ b/src/Access/AccessRights.h @@ -109,9 +109,9 @@ public: const AccessFlags & max_flags_with_children, std::string_view database, std::string_view table, - std::string_view column)>; + std::string_view column, + bool grant_option)>; void modifyFlags(const ModifyFlagsFunction & function); - void modifyFlagsWithGrantOption(const ModifyFlagsFunction & function); friend bool operator ==(const AccessRights & left, const AccessRights & right); friend bool operator !=(const AccessRights & left, const AccessRights & right) { return !(left == right); } diff --git a/src/Access/ContextAccess.cpp b/src/Access/ContextAccess.cpp index 92a5179d861..47f6c35ae32 100644 --- a/src/Access/ContextAccess.cpp +++ b/src/Access/ContextAccess.cpp @@ -44,9 +44,17 @@ namespace } - AccessRights addImplicitAccessRights(const AccessRights & access) + AccessRights addImplicitAccessRights(const AccessRights & access, const AccessControl & access_control) { - auto modifier = [&](const AccessFlags & flags, const AccessFlags & min_flags_with_children, const AccessFlags & max_flags_with_children, std::string_view database, std::string_view table, std::string_view column) -> AccessFlags + AccessFlags max_flags; + + auto modifier = [&](const AccessFlags & flags, + const AccessFlags & min_flags_with_children, + const AccessFlags & max_flags_with_children, + std::string_view database, + std::string_view table, + std::string_view column, + bool /* grant_option */) -> AccessFlags { size_t level = !database.empty() + !table.empty() + !column.empty(); AccessFlags res = flags; @@ -115,17 +123,55 @@ namespace res |= show_databases; } + max_flags |= max_flags_with_children; + return res; }; AccessRights res = access; res.modifyFlags(modifier); - res.modifyFlagsWithGrantOption(modifier); - /// Anyone has access to the "system" and "information_schema" database. - res.grant(AccessType::SELECT, DatabaseCatalog::SYSTEM_DATABASE); - res.grant(AccessType::SELECT, DatabaseCatalog::INFORMATION_SCHEMA); - res.grant(AccessType::SELECT, DatabaseCatalog::INFORMATION_SCHEMA_UPPERCASE); + if (access_control.doesSelectFromSystemDatabaseRequireGrant()) + { + res.grant(AccessType::SELECT, DatabaseCatalog::SYSTEM_DATABASE, "one"); + + if (max_flags.contains(AccessType::SHOW_USERS)) + res.grant(AccessType::SELECT, DatabaseCatalog::SYSTEM_DATABASE, "users"); + + if (max_flags.contains(AccessType::SHOW_ROLES)) + res.grant(AccessType::SELECT, DatabaseCatalog::SYSTEM_DATABASE, "roles"); + + if (max_flags.contains(AccessType::SHOW_ROW_POLICIES)) + res.grant(AccessType::SELECT, DatabaseCatalog::SYSTEM_DATABASE, "row_policies"); + + if (max_flags.contains(AccessType::SHOW_SETTINGS_PROFILES)) + res.grant(AccessType::SELECT, DatabaseCatalog::SYSTEM_DATABASE, "settings_profiles"); + + if (max_flags.contains(AccessType::SHOW_QUOTAS)) + res.grant(AccessType::SELECT, DatabaseCatalog::SYSTEM_DATABASE, "quotas"); + + if (max_flags.contains(AccessType::SHOW_COLUMNS)) + res.grant(AccessType::SELECT, DatabaseCatalog::SYSTEM_DATABASE, "columns"); + + if (max_flags.contains(AccessType::SHOW_TABLES)) + res.grant(AccessType::SELECT, DatabaseCatalog::SYSTEM_DATABASE, "tables"); + + if (max_flags.contains(AccessType::SHOW_DATABASES)) + res.grant(AccessType::SELECT, DatabaseCatalog::SYSTEM_DATABASE, "databases"); + } + else + { + /// Anyone has access to the "system" database. + res.grant(AccessType::SELECT, DatabaseCatalog::SYSTEM_DATABASE); + } + + if (!access_control.doesSelectFromInformationSchemaDatabaseRequireGrant()) + { + /// Anyone has access to the "information_schema" database. + res.grant(AccessType::SELECT, DatabaseCatalog::INFORMATION_SCHEMA); + res.grant(AccessType::SELECT, DatabaseCatalog::INFORMATION_SCHEMA_UPPERCASE); + } + return res; } @@ -247,7 +293,7 @@ void ContextAccess::setRolesInfo(const std::shared_ptr & void ContextAccess::calculateAccessRights() const { access = std::make_shared(mixAccessRightsFromUserAndRoles(*user, *roles_info)); - access_with_implicit = std::make_shared(addImplicitAccessRights(*access)); + access_with_implicit = std::make_shared(addImplicitAccessRights(*access, *access_control)); if (trace_log) { @@ -342,7 +388,7 @@ std::shared_ptr ContextAccess::getFullAccess() auto full_access = std::shared_ptr(new ContextAccess); full_access->is_full_access = true; full_access->access = std::make_shared(AccessRights::getFullAccess()); - full_access->access_with_implicit = std::make_shared(addImplicitAccessRights(*full_access->access)); + full_access->access_with_implicit = full_access->access; return full_access; }(); return res; diff --git a/tests/config/config.d/enable_access_control_improvements.xml b/tests/config/config.d/enable_access_control_improvements.xml index 052858a9519..3bab0d95144 100644 --- a/tests/config/config.d/enable_access_control_improvements.xml +++ b/tests/config/config.d/enable_access_control_improvements.xml @@ -2,5 +2,7 @@ true true + true + true diff --git a/tests/integration/helpers/0_common_instance_config.xml b/tests/integration/helpers/0_common_instance_config.xml index b6ea21648bb..4bede7767c5 100644 --- a/tests/integration/helpers/0_common_instance_config.xml +++ b/tests/integration/helpers/0_common_instance_config.xml @@ -21,5 +21,7 @@ true + true + true From fbb2e14d543f4871aa2d133c6cc13250277c5a6b Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Sat, 9 Jul 2022 17:51:59 +0200 Subject: [PATCH 03/10] Add new table function viewIfPermitted(). --- programs/client/Client.cpp | 4 +- src/Client/Suggest.cpp | 76 ++++++------ src/Parsers/ASTFunction.cpp | 19 +++ src/Parsers/ExpressionElementParsers.cpp | 31 ++++- src/Parsers/ExpressionElementParsers.h | 2 +- src/Storages/StorageView.cpp | 10 +- .../TableFunctionViewIfPermitted.cpp | 113 ++++++++++++++++++ .../TableFunctionViewIfPermitted.h | 35 ++++++ src/TableFunctions/registerTableFunctions.cpp | 1 + src/TableFunctions/registerTableFunctions.h | 1 + .../test.py | 35 ++++++ 11 files changed, 281 insertions(+), 46 deletions(-) create mode 100644 src/TableFunctions/TableFunctionViewIfPermitted.cpp create mode 100644 src/TableFunctions/TableFunctionViewIfPermitted.h diff --git a/programs/client/Client.cpp b/programs/client/Client.cpp index 4e3aa701d95..cf9b7cbafea 100644 --- a/programs/client/Client.cpp +++ b/programs/client/Client.cpp @@ -106,7 +106,9 @@ void Client::processError(const String & query) const std::vector Client::loadWarningMessages() { std::vector messages; - connection->sendQuery(connection_parameters.timeouts, "SELECT message FROM system.warnings", "" /* query_id */, + connection->sendQuery(connection_parameters.timeouts, + "SELECT * FROM viewIfPermitted(SELECT message FROM system.warnings ELSE null('message String'))", + "" /* query_id */, QueryProcessingStage::Complete, &global_context->getSettingsRef(), &global_context->getClientInfo(), false, {}); diff --git a/src/Client/Suggest.cpp b/src/Client/Suggest.cpp index de09c07f4c1..44e9b1bb735 100644 --- a/src/Client/Suggest.cpp +++ b/src/Client/Suggest.cpp @@ -50,52 +50,58 @@ static String getLoadSuggestionQuery(Int32 suggestion_limit, bool basic_suggesti { /// NOTE: Once you will update the completion list, /// do not forget to update 01676_clickhouse_client_autocomplete.sh - WriteBufferFromOwnString query; - query << "SELECT DISTINCT arrayJoin(extractAll(name, '[\\\\w_]{2,}')) AS res FROM (" - "SELECT name FROM system.functions" - " UNION ALL " - "SELECT name FROM system.table_engines" - " UNION ALL " - "SELECT name FROM system.formats" - " UNION ALL " - "SELECT name FROM system.table_functions" - " UNION ALL " - "SELECT name FROM system.data_type_families" - " UNION ALL " - "SELECT name FROM system.merge_tree_settings" - " UNION ALL " - "SELECT name FROM system.settings" - " UNION ALL "; + String query; + + auto add_subquery = [&](std::string_view select, std::string_view result_column_name) + { + if (!query.empty()) + query += " UNION ALL "; + query += fmt::format("SELECT * FROM viewIfPermitted({} ELSE null('{} String'))", select, result_column_name); + }; + + auto add_column = [&](std::string_view column_name, std::string_view table_name, bool distinct, std::optional limit) + { + add_subquery( + fmt::format( + "SELECT {}{} FROM system.{}{}", + (distinct ? "DISTINCT " : ""), + column_name, + table_name, + (limit ? (" LIMIT " + std::to_string(*limit)) : "")), + column_name); + }; + + add_column("name", "functions", false, {}); + add_column("name", "table_engines", false, {}); + add_column("name", "formats", false, {}); + add_column("name", "table_functions", false, {}); + add_column("name", "data_type_families", false, {}); + add_column("name", "merge_tree_settings", false, {}); + add_column("name", "settings", false, {}); + if (!basic_suggestion) { - query << "SELECT cluster FROM system.clusters" - " UNION ALL " - "SELECT macro FROM system.macros" - " UNION ALL " - "SELECT policy_name FROM system.storage_policies" - " UNION ALL "; + add_column("cluster", "clusters", false, {}); + add_column("macro", "macros", false, {}); + add_column("policy_name", "storage_policies", false, {}); } - query << "SELECT concat(func.name, comb.name) FROM system.functions AS func CROSS JOIN system.aggregate_function_combinators AS comb WHERE is_aggregate"; + + add_subquery("SELECT concat(func.name, comb.name) AS x FROM system.functions AS func CROSS JOIN system.aggregate_function_combinators AS comb WHERE is_aggregate", "x"); + /// The user may disable loading of databases, tables, columns by setting suggestion_limit to zero. if (suggestion_limit > 0) { - String limit_str = toString(suggestion_limit); - query << " UNION ALL " - "SELECT name FROM system.databases LIMIT " << limit_str - << " UNION ALL " - "SELECT DISTINCT name FROM system.tables LIMIT " << limit_str - << " UNION ALL "; - + add_column("name", "databases", false, suggestion_limit); + add_column("name", "tables", true, suggestion_limit); if (!basic_suggestion) { - query << "SELECT DISTINCT name FROM system.dictionaries LIMIT " << limit_str - << " UNION ALL "; + add_column("name", "dictionaries", true, suggestion_limit); } - query << "SELECT DISTINCT name FROM system.columns LIMIT " << limit_str; + add_column("name", "columns", true, suggestion_limit); } - query << ") WHERE notEmpty(res)"; - return query.str(); + query = "SELECT DISTINCT arrayJoin(extractAll(name, '[\\\\w_]{2,}')) AS res FROM (" + query + ") WHERE notEmpty(res)"; + return query; } template diff --git a/src/Parsers/ASTFunction.cpp b/src/Parsers/ASTFunction.cpp index 69927c430dc..39d89f56e91 100644 --- a/src/Parsers/ASTFunction.cpp +++ b/src/Parsers/ASTFunction.cpp @@ -509,6 +509,25 @@ void ASTFunction::formatImplWithoutAlias(const FormatSettings & settings, Format settings.ostr << ')'; written = true; } + + if (!written && name == "viewIfPermitted"sv) + { + /// viewIfPermitted() needs special formatting: ELSE instead of comma between arguments, and better indents too. + const auto * nl_or_nothing = settings.one_line ? "" : "\n"; + auto indent0 = settings.one_line ? "" : String(4u * frame.indent, ' '); + auto indent1 = settings.one_line ? "" : String(4u * (frame.indent + 1), ' '); + auto indent2 = settings.one_line ? "" : String(4u * (frame.indent + 2), ' '); + settings.ostr << (settings.hilite ? hilite_function : "") << name << "(" << (settings.hilite ? hilite_none : "") << nl_or_nothing; + FormatStateStacked frame_nested = frame; + frame_nested.need_parens = false; + frame_nested.indent += 2; + arguments->children[0]->formatImpl(settings, state, frame_nested); + settings.ostr << nl_or_nothing << indent1 << (settings.hilite ? hilite_keyword : "") << (settings.one_line ? " " : "") + << "ELSE " << (settings.hilite ? hilite_none : "") << nl_or_nothing << indent2; + arguments->children[1]->formatImpl(settings, state, frame_nested); + settings.ostr << nl_or_nothing << indent0 << ")"; + return; + } } if (!written && arguments->children.size() >= 2) diff --git a/src/Parsers/ExpressionElementParsers.cpp b/src/Parsers/ExpressionElementParsers.cpp index 045f6aad2b5..bd65305cc52 100644 --- a/src/Parsers/ExpressionElementParsers.cpp +++ b/src/Parsers/ExpressionElementParsers.cpp @@ -1068,13 +1068,16 @@ bool ParserFunction::parseImpl(Pos & pos, ASTPtr & node, Expected & expected) bool ParserTableFunctionView::parseImpl(Pos & pos, ASTPtr & node, Expected & expected) { ParserIdentifier id_parser; - ParserKeyword view("VIEW"); ParserSelectWithUnionQuery select; ASTPtr identifier; ASTPtr query; - if (!view.ignore(pos, expected)) + bool if_permitted = false; + + if (ParserKeyword{"VIEWIFPERMITTED"}.ignore(pos, expected)) + if_permitted = true; + else if (!ParserKeyword{"VIEW"}.ignore(pos, expected)) return false; if (pos->type != TokenType::OpeningRoundBracket) @@ -1094,15 +1097,30 @@ bool ParserTableFunctionView::parseImpl(Pos & pos, ASTPtr & node, Expected & exp return false; } + ASTPtr else_ast; + if (if_permitted) + { + if (!ParserKeyword{"ELSE"}.ignore(pos, expected)) + return false; + + if (!ParserWithOptionalAlias{std::make_unique(true, true), true}.parse(pos, else_ast, expected)) + return false; + } + if (pos->type != TokenType::ClosingRoundBracket) return false; + ++pos; + + auto expr_list = std::make_shared(); + expr_list->children.push_back(query); + if (if_permitted) + expr_list->children.push_back(else_ast); + auto function_node = std::make_shared(); tryGetIdentifierNameInto(identifier, function_node->name); - auto expr_list_with_single_query = std::make_shared(); - expr_list_with_single_query->children.push_back(query); - function_node->name = "view"; - function_node->arguments = expr_list_with_single_query; + function_node->name = if_permitted ? "viewIfPermitted" : "view"; + function_node->arguments = expr_list; function_node->children.push_back(function_node->arguments); node = function_node; return true; @@ -1971,6 +1989,7 @@ const char * ParserAlias::restricted_keywords[] = "WITH", "INTERSECT", "EXCEPT", + "ELSE", nullptr }; diff --git a/src/Parsers/ExpressionElementParsers.h b/src/Parsers/ExpressionElementParsers.h index f4dfe80f43e..3883631b61c 100644 --- a/src/Parsers/ExpressionElementParsers.h +++ b/src/Parsers/ExpressionElementParsers.h @@ -162,7 +162,7 @@ protected: bool is_table_function; }; -// A special function parser for view table function. +// A special function parser for view and viewIfPermitted table functions. // It parses an SELECT query as its argument and doesn't support getColumnName(). class ParserTableFunctionView : public IParserBase { diff --git a/src/Storages/StorageView.cpp b/src/Storages/StorageView.cpp index bbbad012547..3377af685f0 100644 --- a/src/Storages/StorageView.cpp +++ b/src/Storages/StorageView.cpp @@ -180,9 +180,13 @@ void StorageView::replaceWithSubquery(ASTSelectQuery & outer_query, ASTPtr view_ if (!table_expression->database_and_table_name) { // If it's a view table function, add a fake db.table name. - if (table_expression->table_function && table_expression->table_function->as()->name == "view") - table_expression->database_and_table_name = std::make_shared("__view"); - else + if (table_expression->table_function) + { + auto table_function_name = table_expression->table_function->as()->name; + if ((table_function_name == "view") || (table_function_name == "viewIfPermitted")) + table_expression->database_and_table_name = std::make_shared("__view"); + } + if (!table_expression->database_and_table_name) throw Exception("Logical error: incorrect table expression", ErrorCodes::LOGICAL_ERROR); } diff --git a/src/TableFunctions/TableFunctionViewIfPermitted.cpp b/src/TableFunctions/TableFunctionViewIfPermitted.cpp new file mode 100644 index 00000000000..72469cf918c --- /dev/null +++ b/src/TableFunctions/TableFunctionViewIfPermitted.cpp @@ -0,0 +1,113 @@ +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include "registerTableFunctions.h" + + +namespace DB +{ +namespace ErrorCodes +{ + extern const int NUMBER_OF_ARGUMENTS_DOESNT_MATCH; + extern const int BAD_ARGUMENTS; + extern const int ACCESS_DENIED; +} + + +const ASTSelectWithUnionQuery & TableFunctionViewIfPermitted::getSelectQuery() const +{ + return *create.select; +} + +void TableFunctionViewIfPermitted::parseArguments(const ASTPtr & ast_function, ContextPtr context) +{ + const auto * function = ast_function->as(); + if (!function || !function->arguments || (function->arguments->children.size() != 2)) + throw Exception( + ErrorCodes::NUMBER_OF_ARGUMENTS_DOESNT_MATCH, + "Table function '{}' requires two arguments: a SELECT query and a table function", + getName()); + + const auto & arguments = function->arguments->children; + auto * select = arguments[0]->as(); + if (!select) + throw Exception(ErrorCodes::BAD_ARGUMENTS, "Table function '{}' requires a SELECT query as its first argument", getName()); + create.set(create.select, select->clone()); + + else_ast = arguments[1]; + if (!else_ast->as()) + throw Exception(ErrorCodes::BAD_ARGUMENTS, "Table function '{}' requires a table function as its second argument", getName()); + else_table_function = TableFunctionFactory::instance().get(else_ast, context); +} + +ColumnsDescription TableFunctionViewIfPermitted::getActualTableStructure(ContextPtr context) const +{ + return else_table_function->getActualTableStructure(context); +} + +StoragePtr TableFunctionViewIfPermitted::executeImpl( + const ASTPtr & /* ast_function */, ContextPtr context, const std::string & table_name, ColumnsDescription /* cached_columns */) const +{ + StoragePtr storage; + auto columns = getActualTableStructure(context); + + if (isPermitted(context, columns)) + { + storage = std::make_shared(StorageID(getDatabaseName(), table_name), create, columns, ""); + } + else + { + storage = else_table_function->execute(else_ast, context, table_name); + } + + storage->startup(); + return storage; +} + +bool TableFunctionViewIfPermitted::isPermitted(const ContextPtr & context, const ColumnsDescription & else_columns) const +{ + Block sample_block; + + try + { + /// Will throw ACCESS_DENIED if the current user is not allowed to execute the SELECT query. + sample_block = InterpreterSelectWithUnionQuery::getSampleBlock(create.children[0], context); + } + catch (Exception & e) + { + if (e.code() == ErrorCodes::ACCESS_DENIED) + return {}; + throw; + } + + /// We check that columns match only if permitted (otherwise we could reveal the structure to an user who must not know it). + ColumnsDescription columns{sample_block.getNamesAndTypesList()}; + if (columns != else_columns) + { + throw Exception( + ErrorCodes::BAD_ARGUMENTS, + "Table function '{}' requires a SELECT query with the result columns matching a table function after 'ELSE'. " + "Currently the result columns of the SELECT query are {}, and the table function after 'ELSE' gives {}", + getName(), + columns.toString(), + else_columns.toString()); + } + + return true; +} + +void registerTableFunctionViewIfPermitted(TableFunctionFactory & factory) +{ + factory.registerFunction(); +} + +} diff --git a/src/TableFunctions/TableFunctionViewIfPermitted.h b/src/TableFunctions/TableFunctionViewIfPermitted.h new file mode 100644 index 00000000000..0fd0b050eaf --- /dev/null +++ b/src/TableFunctions/TableFunctionViewIfPermitted.h @@ -0,0 +1,35 @@ +#pragma once + +#include +#include +#include + +namespace DB +{ + +/* viewIfPermitted(query ELSE null('structure')) + * Works as "view(query)" if the current user has the permissions required to execute "query"; works as "null('structure')" otherwise. + */ +class TableFunctionViewIfPermitted : public ITableFunction +{ +public: + static constexpr auto name = "viewIfPermitted"; + std::string getName() const override { return name; } + + const ASTSelectWithUnionQuery & getSelectQuery() const; + +private: + StoragePtr executeImpl(const ASTPtr & ast_function, ContextPtr context, const String & table_name, ColumnsDescription cached_columns) const override; + const char * getStorageTypeName() const override { return "ViewIfPermitted"; } + + void parseArguments(const ASTPtr & ast_function, ContextPtr context) override; + ColumnsDescription getActualTableStructure(ContextPtr context) const override; + + bool isPermitted(const ContextPtr & context, const ColumnsDescription & else_columns) const; + + ASTCreateQuery create; + ASTPtr else_ast; + TableFunctionPtr else_table_function; +}; + +} diff --git a/src/TableFunctions/registerTableFunctions.cpp b/src/TableFunctions/registerTableFunctions.cpp index 12ca4abe113..3ef93c9b69d 100644 --- a/src/TableFunctions/registerTableFunctions.cpp +++ b/src/TableFunctions/registerTableFunctions.cpp @@ -42,6 +42,7 @@ void registerTableFunctions() registerTableFunctionJDBC(factory); registerTableFunctionView(factory); + registerTableFunctionViewIfPermitted(factory); #if USE_MYSQL registerTableFunctionMySQL(factory); diff --git a/src/TableFunctions/registerTableFunctions.h b/src/TableFunctions/registerTableFunctions.h index 49a1ef60a6b..d7e38403cae 100644 --- a/src/TableFunctions/registerTableFunctions.h +++ b/src/TableFunctions/registerTableFunctions.h @@ -40,6 +40,7 @@ void registerTableFunctionODBC(TableFunctionFactory & factory); void registerTableFunctionJDBC(TableFunctionFactory & factory); void registerTableFunctionView(TableFunctionFactory & factory); +void registerTableFunctionViewIfPermitted(TableFunctionFactory & factory); #if USE_MYSQL void registerTableFunctionMySQL(TableFunctionFactory & factory); diff --git a/tests/integration/test_table_functions_access_rights/test.py b/tests/integration/test_table_functions_access_rights/test.py index 705150c8bdd..09a05122c07 100644 --- a/tests/integration/test_table_functions_access_rights/test.py +++ b/tests/integration/test_table_functions_access_rights/test.py @@ -65,3 +65,38 @@ def test_merge(): "it's necessary to have grant SELECT ON default.table2" in instance.query_and_get_error(select_query, user="A") ) + + +def test_view_if_permitted(): + assert ( + instance.query( + "SELECT * FROM viewIfPermitted(SELECT * FROM table1 ELSE null('x UInt32'))" + ) + == "1\n" + ) + + expected_error = "requires a SELECT query with the result columns matching a table function after 'ELSE'" + assert expected_error in instance.query_and_get_error( + "SELECT * FROM viewIfPermitted(SELECT * FROM table1 ELSE null('x Int32'))" + ) + assert expected_error in instance.query_and_get_error( + "SELECT * FROM viewIfPermitted(SELECT * FROM table1 ELSE null('y UInt32'))" + ) + + instance.query("CREATE USER A") + assert ( + instance.query( + "SELECT * FROM viewIfPermitted(SELECT * FROM table1 ELSE null('x UInt32'))", + user="A", + ) + == "" + ) + + instance.query("GRANT SELECT ON table1 TO A") + assert ( + instance.query( + "SELECT * FROM viewIfPermitted(SELECT * FROM table1 ELSE null('x UInt32'))", + user="A", + ) + == "1\n" + ) From 9e70e025890c43862c115b30b97d5b9c441423c8 Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Sun, 10 Jul 2022 02:49:45 +0200 Subject: [PATCH 04/10] Table function null() & view() don't require CREATE TEMPORARY TABLE privilege anymore. --- src/TableFunctions/ITableFunction.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/TableFunctions/ITableFunction.cpp b/src/TableFunctions/ITableFunction.cpp index 639240fd105..82b6230dc30 100644 --- a/src/TableFunctions/ITableFunction.cpp +++ b/src/TableFunctions/ITableFunction.cpp @@ -23,7 +23,12 @@ StoragePtr ITableFunction::execute(const ASTPtr & ast_function, ContextPtr conte ColumnsDescription cached_columns, bool use_global_context) const { ProfileEvents::increment(ProfileEvents::TableFunctionExecute); - context->checkAccess(AccessType::CREATE_TEMPORARY_TABLE | getSourceAccessType()); + + AccessFlags required_access = getSourceAccessType(); + String function_name = getName(); + if ((function_name != "null") && (function_name != "view") && (function_name != "viewIfPermitted")) + required_access |= AccessType::CREATE_TEMPORARY_TABLE; + context->checkAccess(required_access); auto context_to_use = use_global_context ? context->getGlobalContext() : context; From de34d173ba23d39f165e7c34cf7cf47f74d83a40 Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Sun, 10 Jul 2022 02:51:31 +0200 Subject: [PATCH 05/10] Remove excessive log messages. --- src/Access/ContextAccess.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Access/ContextAccess.cpp b/src/Access/ContextAccess.cpp index 47f6c35ae32..2fc42cf5d0a 100644 --- a/src/Access/ContextAccess.cpp +++ b/src/Access/ContextAccess.cpp @@ -459,7 +459,7 @@ bool ContextAccess::checkAccessImplHelper(AccessFlags flags, const Args &... arg }; if (is_full_access) - return access_granted(); + return true; if (user_was_dropped) return access_denied("User has been dropped", ErrorCodes::UNKNOWN_USER); @@ -468,7 +468,7 @@ bool ContextAccess::checkAccessImplHelper(AccessFlags flags, const Args &... arg flags &= ~AccessType::CLUSTER; if (!flags) - return access_granted(); + return true; /// Access to temporary tables is controlled in an unusual way, not like normal tables. /// Creating of temporary tables is controlled by AccessType::CREATE_TEMPORARY_TABLES grant, From 6bf7bffbebe6302e8731ccf0d25a454501fa9c5a Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Sun, 10 Jul 2022 15:15:51 +0200 Subject: [PATCH 06/10] Correct the list of always accessible system tables. --- programs/server/config.xml | 11 ++-- src/Access/AccessControl.cpp | 2 +- src/Access/AccessControl.h | 6 +- src/Access/AccessRights.cpp | 4 +- src/Access/ContextAccess.cpp | 55 ++++++++++++++----- src/Client/Suggest.cpp | 2 +- .../enable_access_control_improvements.xml | 2 +- .../helpers/0_common_instance_config.xml | 3 +- 8 files changed, 56 insertions(+), 29 deletions(-) diff --git a/programs/server/config.xml b/programs/server/config.xml index 343e8dc7093..40e561c1880 100644 --- a/programs/server/config.xml +++ b/programs/server/config.xml @@ -611,15 +611,16 @@ + If it's set to true then this query requires "GRANT SELECT ON system." just like as for non-system tables. + Exceptions: a few system tables ("tables", "columns", "databases", and some constant tables like "one", "contributors") + are still accessible for everyone; and if there is a SHOW privilege (e.g. "SHOW USERS") granted the corresponding system + table (i.e. "system.users") will be accessible. --> false - false + If it's set to true then this query requires "GRANT SELECT ON information_schema.
" just like as for ordinary tables. --> + false diff --git a/src/Access/AccessControl.cpp b/src/Access/AccessControl.cpp index 5d3fc558130..c6729459988 100644 --- a/src/Access/AccessControl.cpp +++ b/src/Access/AccessControl.cpp @@ -170,7 +170,7 @@ void AccessControl::setUpFromMainConfig(const Poco::Util::AbstractConfiguration setEnabledUsersWithoutRowPoliciesCanReadRows(config_.getBool("access_control_improvements.users_without_row_policies_can_read_rows", false)); setOnClusterQueriesRequireClusterGrant(config_.getBool("access_control_improvements.on_cluster_queries_require_cluster_grant", false)); setSelectFromSystemDatabaseRequiresGrant(config_.getBool("access_control_improvements.select_from_system_db_requires_grant", false)); - setSelectFromInformationSchemaDatabaseRequiresGrant(config_.getBool("access_control_improvements.select_from_information_schema_db_requires_grant", false)); + setSelectFromInformationSchemaRequiresGrant(config_.getBool("access_control_improvements.select_from_information_schema_requires_grant", false)); addStoragesFromMainConfig(config_, config_path_, get_zookeeper_function_); } diff --git a/src/Access/AccessControl.h b/src/Access/AccessControl.h index e0571ad370e..ab9cdba9ad1 100644 --- a/src/Access/AccessControl.h +++ b/src/Access/AccessControl.h @@ -155,8 +155,8 @@ public: void setSelectFromSystemDatabaseRequiresGrant(bool enable) { select_from_system_db_requires_grant = enable; } bool doesSelectFromSystemDatabaseRequireGrant() const { return select_from_system_db_requires_grant; } - void setSelectFromInformationSchemaDatabaseRequiresGrant(bool enable) { select_from_information_schema_db_requires_grant = enable; } - bool doesSelectFromInformationSchemaDatabaseRequireGrant() const { return select_from_information_schema_db_requires_grant; } + void setSelectFromInformationSchemaRequiresGrant(bool enable) { select_from_information_schema_requires_grant = enable; } + bool doesSelectFromInformationSchemaRequireGrant() const { return select_from_information_schema_requires_grant; } std::shared_ptr getContextAccess( const UUID & user_id, @@ -222,7 +222,7 @@ private: std::atomic_bool users_without_row_policies_can_read_rows = false; std::atomic_bool on_cluster_queries_require_cluster_grant = false; std::atomic_bool select_from_system_db_requires_grant = false; - std::atomic_bool select_from_information_schema_db_requires_grant = false; + std::atomic_bool select_from_information_schema_requires_grant = false; }; } diff --git a/src/Access/AccessRights.cpp b/src/Access/AccessRights.cpp index 747e7a91b2c..20afc916901 100644 --- a/src/Access/AccessRights.cpp +++ b/src/Access/AccessRights.cpp @@ -1062,12 +1062,12 @@ void AccessRights::modifyFlags(const ModifyFlagsFunction & function) { if (!root) return; - + bool flags_added, flags_removed; root->modifyFlags(function, false, flags_added, flags_removed); if (flags_removed && root_with_grant_option) root_with_grant_option->makeIntersection(*root); - + if (root_with_grant_option) { root_with_grant_option->modifyFlags(function, true, flags_added, flags_removed); diff --git a/src/Access/ContextAccess.cpp b/src/Access/ContextAccess.cpp index 2fc42cf5d0a..49736c76994 100644 --- a/src/Access/ContextAccess.cpp +++ b/src/Access/ContextAccess.cpp @@ -123,7 +123,7 @@ namespace res |= show_databases; } - max_flags |= max_flags_with_children; + max_flags |= res; return res; }; @@ -131,13 +131,48 @@ namespace AccessRights res = access; res.modifyFlags(modifier); + /// If "select_from_system_db_requires_grant" is enabled we provide implicit grants only for a few tables in the system database. if (access_control.doesSelectFromSystemDatabaseRequireGrant()) { - res.grant(AccessType::SELECT, DatabaseCatalog::SYSTEM_DATABASE, "one"); + const char * always_accessible_tables[] = { + /// Constant tables + "one", + + /// "numbers", "numbers_mt", "zeros", "zeros_mt" were excluded because they can generate lots of values and + /// that can decrease performance in some cases. + + "contributors", + "licenses", + "time_zones", + "collations", + + "formats", + "privileges", + "data_type_families", + "table_engines", + "table_functions", + "aggregate_function_combinators", + + "functions", /// Can contain user-defined functions + + /// The following tables hide some rows if the current user doesn't have corresponding SHOW privileges. + "databases", + "tables", + "columns", + + /// Specific to the current session + "settings", + "current_roles", + "enabled_roles", + "quota_usage" + }; + + for (const auto * table_name : always_accessible_tables) + res.grant(AccessType::SELECT, DatabaseCatalog::SYSTEM_DATABASE, table_name); if (max_flags.contains(AccessType::SHOW_USERS)) res.grant(AccessType::SELECT, DatabaseCatalog::SYSTEM_DATABASE, "users"); - + if (max_flags.contains(AccessType::SHOW_ROLES)) res.grant(AccessType::SELECT, DatabaseCatalog::SYSTEM_DATABASE, "roles"); @@ -149,25 +184,15 @@ namespace if (max_flags.contains(AccessType::SHOW_QUOTAS)) res.grant(AccessType::SELECT, DatabaseCatalog::SYSTEM_DATABASE, "quotas"); - - if (max_flags.contains(AccessType::SHOW_COLUMNS)) - res.grant(AccessType::SELECT, DatabaseCatalog::SYSTEM_DATABASE, "columns"); - - if (max_flags.contains(AccessType::SHOW_TABLES)) - res.grant(AccessType::SELECT, DatabaseCatalog::SYSTEM_DATABASE, "tables"); - - if (max_flags.contains(AccessType::SHOW_DATABASES)) - res.grant(AccessType::SELECT, DatabaseCatalog::SYSTEM_DATABASE, "databases"); } else { - /// Anyone has access to the "system" database. res.grant(AccessType::SELECT, DatabaseCatalog::SYSTEM_DATABASE); } - if (!access_control.doesSelectFromInformationSchemaDatabaseRequireGrant()) + /// If "select_from_information_schema_requires_grant" is enabled we don't provide implicit grants for the information_schema database. + if (!access_control.doesSelectFromInformationSchemaRequireGrant()) { - /// Anyone has access to the "information_schema" database. res.grant(AccessType::SELECT, DatabaseCatalog::INFORMATION_SCHEMA); res.grant(AccessType::SELECT, DatabaseCatalog::INFORMATION_SCHEMA_UPPERCASE); } diff --git a/src/Client/Suggest.cpp b/src/Client/Suggest.cpp index 44e9b1bb735..1074adb2bd4 100644 --- a/src/Client/Suggest.cpp +++ b/src/Client/Suggest.cpp @@ -87,7 +87,7 @@ static String getLoadSuggestionQuery(Int32 suggestion_limit, bool basic_suggesti } add_subquery("SELECT concat(func.name, comb.name) AS x FROM system.functions AS func CROSS JOIN system.aggregate_function_combinators AS comb WHERE is_aggregate", "x"); - + /// The user may disable loading of databases, tables, columns by setting suggestion_limit to zero. if (suggestion_limit > 0) { diff --git a/tests/config/config.d/enable_access_control_improvements.xml b/tests/config/config.d/enable_access_control_improvements.xml index 3bab0d95144..5a186548098 100644 --- a/tests/config/config.d/enable_access_control_improvements.xml +++ b/tests/config/config.d/enable_access_control_improvements.xml @@ -3,6 +3,6 @@ true true true - true + true diff --git a/tests/integration/helpers/0_common_instance_config.xml b/tests/integration/helpers/0_common_instance_config.xml index 4bede7767c5..64f0ce9e361 100644 --- a/tests/integration/helpers/0_common_instance_config.xml +++ b/tests/integration/helpers/0_common_instance_config.xml @@ -21,7 +21,8 @@ true + true true - true + true From ef3a24d20fe6851b9e4e557b0af8a2b26c89e6b2 Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Sun, 10 Jul 2022 18:15:32 +0200 Subject: [PATCH 07/10] Fix tests. --- tests/integration/test_backup_restore_on_cluster/test.py | 2 ++ .../configs/config_allow_databases.xml | 1 + tests/queries/0_stateless/00600_replace_running_query.sh | 1 + tests/queries/0_stateless/01317_no_password_in_command_line.sh | 1 + 4 files changed, 5 insertions(+) diff --git a/tests/integration/test_backup_restore_on_cluster/test.py b/tests/integration/test_backup_restore_on_cluster/test.py index 8ba06d9a88c..a0e8758ddc5 100644 --- a/tests/integration/test_backup_restore_on_cluster/test.py +++ b/tests/integration/test_backup_restore_on_cluster/test.py @@ -434,6 +434,7 @@ def test_required_privileges(): node1.query("INSERT INTO tbl VALUES (100)") node1.query("CREATE USER u1") + node1.query("GRANT CLUSTER ON *.* TO u1") backup_name = new_backup_name() expected_error = "necessary to have grant BACKUP ON default.tbl" @@ -478,6 +479,7 @@ def test_system_users(): backup_name = new_backup_name() node1.query("CREATE USER u2 SETTINGS allow_backup=false") + node1.query("GRANT CLUSTER ON *.* TO u2") expected_error = "necessary to have grant BACKUP ON system.users" assert expected_error in node1.query_and_get_error( diff --git a/tests/integration/test_config_substitutions/configs/config_allow_databases.xml b/tests/integration/test_config_substitutions/configs/config_allow_databases.xml index be727360dcf..ba38a4f250a 100644 --- a/tests/integration/test_config_substitutions/configs/config_allow_databases.xml +++ b/tests/integration/test_config_substitutions/configs/config_allow_databases.xml @@ -19,6 +19,7 @@ default + system diff --git a/tests/queries/0_stateless/00600_replace_running_query.sh b/tests/queries/0_stateless/00600_replace_running_query.sh index 89c9d1c4279..6a682210489 100755 --- a/tests/queries/0_stateless/00600_replace_running_query.sh +++ b/tests/queries/0_stateless/00600_replace_running_query.sh @@ -9,6 +9,7 @@ CURDIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) ${CLICKHOUSE_CLIENT} -q "drop user if exists u_00600" ${CLICKHOUSE_CLIENT} -q "create user u_00600 settings max_execution_time=60, readonly=1" +${CLICKHOUSE_CLIENT} -q "grant select on system.numbers to u_00600" function wait_for_query_to_start() { diff --git a/tests/queries/0_stateless/01317_no_password_in_command_line.sh b/tests/queries/0_stateless/01317_no_password_in_command_line.sh index 5b95f077ea2..7f2e91201a3 100755 --- a/tests/queries/0_stateless/01317_no_password_in_command_line.sh +++ b/tests/queries/0_stateless/01317_no_password_in_command_line.sh @@ -10,6 +10,7 @@ set -e user=user_$CLICKHOUSE_TEST_UNIQUE_NAME $CLICKHOUSE_CLIENT --query "DROP USER IF EXISTS $user" $CLICKHOUSE_CLIENT --query "CREATE USER $user IDENTIFIED WITH PLAINTEXT_PASSWORD BY 'hello'" +$CLICKHOUSE_CLIENT --query "GRANT SELECT ON system.numbers TO $user" trap '$CLICKHOUSE_CLIENT --query "DROP USER $user"' EXIT # Wait for query to start executing. At that time, the password should be cleared. From c7cef91d4d48e24f1a082ab096caba0818f8eba9 Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Mon, 11 Jul 2022 18:21:44 +0200 Subject: [PATCH 08/10] Two ways to provide access to system.users: "GRANT SHOW USERS ON *.*" and "GRANT SELECT ON system.users" --- src/Storages/System/StorageSystemGrants.cpp | 5 ++++- src/Storages/System/StorageSystemQuotaLimits.cpp | 5 ++++- src/Storages/System/StorageSystemQuotaUsage.cpp | 6 +++++- src/Storages/System/StorageSystemQuotas.cpp | 5 ++++- src/Storages/System/StorageSystemQuotasUsage.cpp | 6 +++++- src/Storages/System/StorageSystemRoleGrants.cpp | 5 ++++- src/Storages/System/StorageSystemRoles.cpp | 5 ++++- src/Storages/System/StorageSystemRowPolicies.cpp | 5 ++++- .../System/StorageSystemSettingsProfileElements.cpp | 5 ++++- src/Storages/System/StorageSystemSettingsProfiles.cpp | 5 ++++- src/Storages/System/StorageSystemUsers.cpp | 5 ++++- 11 files changed, 46 insertions(+), 11 deletions(-) diff --git a/src/Storages/System/StorageSystemGrants.cpp b/src/Storages/System/StorageSystemGrants.cpp index 26bd241023a..461efd7f640 100644 --- a/src/Storages/System/StorageSystemGrants.cpp +++ b/src/Storages/System/StorageSystemGrants.cpp @@ -36,8 +36,11 @@ NamesAndTypesList StorageSystemGrants::getNamesAndTypes() void StorageSystemGrants::fillData(MutableColumns & res_columns, ContextPtr context, const SelectQueryInfo &) const { - context->checkAccess(AccessType::SHOW_USERS | AccessType::SHOW_ROLES); + /// If "select_from_system_db_requires_grant" is enabled the access rights were already checked in InterpreterSelectQuery. const auto & access_control = context->getAccessControl(); + if (!access_control.doesSelectFromSystemDatabaseRequireGrant()) + context->checkAccess(AccessType::SHOW_USERS | AccessType::SHOW_ROLES); + std::vector ids = access_control.findAll(); boost::range::push_back(ids, access_control.findAll()); diff --git a/src/Storages/System/StorageSystemQuotaLimits.cpp b/src/Storages/System/StorageSystemQuotaLimits.cpp index c98e060a62f..0261d3d2cd9 100644 --- a/src/Storages/System/StorageSystemQuotaLimits.cpp +++ b/src/Storages/System/StorageSystemQuotaLimits.cpp @@ -66,8 +66,11 @@ NamesAndTypesList StorageSystemQuotaLimits::getNamesAndTypes() void StorageSystemQuotaLimits::fillData(MutableColumns & res_columns, ContextPtr context, const SelectQueryInfo &) const { - context->checkAccess(AccessType::SHOW_QUOTAS); + /// If "select_from_system_db_requires_grant" is enabled the access rights were already checked in InterpreterSelectQuery. const auto & access_control = context->getAccessControl(); + if (!access_control.doesSelectFromSystemDatabaseRequireGrant()) + context->checkAccess(AccessType::SHOW_QUOTAS); + std::vector ids = access_control.findAll(); size_t column_index = 0; diff --git a/src/Storages/System/StorageSystemQuotaUsage.cpp b/src/Storages/System/StorageSystemQuotaUsage.cpp index 54f403803d6..6ba47a86dbf 100644 --- a/src/Storages/System/StorageSystemQuotaUsage.cpp +++ b/src/Storages/System/StorageSystemQuotaUsage.cpp @@ -78,7 +78,11 @@ NamesAndTypesList StorageSystemQuotaUsage::getNamesAndTypesImpl(bool add_column_ void StorageSystemQuotaUsage::fillData(MutableColumns & res_columns, ContextPtr context, const SelectQueryInfo &) const { - context->checkAccess(AccessType::SHOW_QUOTAS); + /// If "select_from_system_db_requires_grant" is enabled the access rights were already checked in InterpreterSelectQuery. + const auto & access_control = context->getAccessControl(); + if (!access_control.doesSelectFromSystemDatabaseRequireGrant()) + context->checkAccess(AccessType::SHOW_QUOTAS); + auto usage = context->getQuotaUsage(); if (!usage) return; diff --git a/src/Storages/System/StorageSystemQuotas.cpp b/src/Storages/System/StorageSystemQuotas.cpp index 046db151684..17863fa7326 100644 --- a/src/Storages/System/StorageSystemQuotas.cpp +++ b/src/Storages/System/StorageSystemQuotas.cpp @@ -53,8 +53,11 @@ NamesAndTypesList StorageSystemQuotas::getNamesAndTypes() void StorageSystemQuotas::fillData(MutableColumns & res_columns, ContextPtr context, const SelectQueryInfo &) const { - context->checkAccess(AccessType::SHOW_QUOTAS); + /// If "select_from_system_db_requires_grant" is enabled the access rights were already checked in InterpreterSelectQuery. const auto & access_control = context->getAccessControl(); + if (!access_control.doesSelectFromSystemDatabaseRequireGrant()) + context->checkAccess(AccessType::SHOW_QUOTAS); + std::vector ids = access_control.findAll(); size_t column_index = 0; diff --git a/src/Storages/System/StorageSystemQuotasUsage.cpp b/src/Storages/System/StorageSystemQuotasUsage.cpp index fae0629a209..a3c97247111 100644 --- a/src/Storages/System/StorageSystemQuotasUsage.cpp +++ b/src/Storages/System/StorageSystemQuotasUsage.cpp @@ -15,7 +15,11 @@ NamesAndTypesList StorageSystemQuotasUsage::getNamesAndTypes() void StorageSystemQuotasUsage::fillData(MutableColumns & res_columns, ContextPtr context, const SelectQueryInfo &) const { - context->checkAccess(AccessType::SHOW_QUOTAS); + /// If "select_from_system_db_requires_grant" is enabled the access rights were already checked in InterpreterSelectQuery. + const auto & access_control = context->getAccessControl(); + if (!access_control.doesSelectFromSystemDatabaseRequireGrant()) + context->checkAccess(AccessType::SHOW_QUOTAS); + auto all_quotas_usage = context->getAccessControl().getAllQuotasUsage(); StorageSystemQuotaUsage::fillDataImpl(res_columns, context, /* add_column_is_current = */ true, all_quotas_usage); } diff --git a/src/Storages/System/StorageSystemRoleGrants.cpp b/src/Storages/System/StorageSystemRoleGrants.cpp index 94ee28cfe83..cf5a24f88cd 100644 --- a/src/Storages/System/StorageSystemRoleGrants.cpp +++ b/src/Storages/System/StorageSystemRoleGrants.cpp @@ -31,8 +31,11 @@ NamesAndTypesList StorageSystemRoleGrants::getNamesAndTypes() void StorageSystemRoleGrants::fillData(MutableColumns & res_columns, ContextPtr context, const SelectQueryInfo &) const { - context->checkAccess(AccessType::SHOW_USERS | AccessType::SHOW_ROLES); + /// If "select_from_system_db_requires_grant" is enabled the access rights were already checked in InterpreterSelectQuery. const auto & access_control = context->getAccessControl(); + if (!access_control.doesSelectFromSystemDatabaseRequireGrant()) + context->checkAccess(AccessType::SHOW_USERS | AccessType::SHOW_ROLES); + std::vector ids = access_control.findAll(); boost::range::push_back(ids, access_control.findAll()); diff --git a/src/Storages/System/StorageSystemRoles.cpp b/src/Storages/System/StorageSystemRoles.cpp index e5b8d53ce7e..5fda021428a 100644 --- a/src/Storages/System/StorageSystemRoles.cpp +++ b/src/Storages/System/StorageSystemRoles.cpp @@ -27,8 +27,11 @@ NamesAndTypesList StorageSystemRoles::getNamesAndTypes() void StorageSystemRoles::fillData(MutableColumns & res_columns, ContextPtr context, const SelectQueryInfo &) const { - context->checkAccess(AccessType::SHOW_ROLES); + /// If "select_from_system_db_requires_grant" is enabled the access rights were already checked in InterpreterSelectQuery. const auto & access_control = context->getAccessControl(); + if (!access_control.doesSelectFromSystemDatabaseRequireGrant()) + context->checkAccess(AccessType::SHOW_ROLES); + std::vector ids = access_control.findAll(); size_t column_index = 0; diff --git a/src/Storages/System/StorageSystemRowPolicies.cpp b/src/Storages/System/StorageSystemRowPolicies.cpp index 064f610730d..c0bc38edc21 100644 --- a/src/Storages/System/StorageSystemRowPolicies.cpp +++ b/src/Storages/System/StorageSystemRowPolicies.cpp @@ -53,8 +53,11 @@ NamesAndTypesList StorageSystemRowPolicies::getNamesAndTypes() void StorageSystemRowPolicies::fillData(MutableColumns & res_columns, ContextPtr context, const SelectQueryInfo &) const { - context->checkAccess(AccessType::SHOW_ROW_POLICIES); + /// If "select_from_system_db_requires_grant" is enabled the access rights were already checked in InterpreterSelectQuery. const auto & access_control = context->getAccessControl(); + if (!access_control.doesSelectFromSystemDatabaseRequireGrant()) + context->checkAccess(AccessType::SHOW_ROW_POLICIES); + std::vector ids = access_control.findAll(); size_t column_index = 0; diff --git a/src/Storages/System/StorageSystemSettingsProfileElements.cpp b/src/Storages/System/StorageSystemSettingsProfileElements.cpp index 8013a3f2e9e..565ff5e471e 100644 --- a/src/Storages/System/StorageSystemSettingsProfileElements.cpp +++ b/src/Storages/System/StorageSystemSettingsProfileElements.cpp @@ -37,8 +37,11 @@ NamesAndTypesList StorageSystemSettingsProfileElements::getNamesAndTypes() void StorageSystemSettingsProfileElements::fillData(MutableColumns & res_columns, ContextPtr context, const SelectQueryInfo &) const { - context->checkAccess(AccessType::SHOW_SETTINGS_PROFILES); + /// If "select_from_system_db_requires_grant" is enabled the access rights were already checked in InterpreterSelectQuery. const auto & access_control = context->getAccessControl(); + if (!access_control.doesSelectFromSystemDatabaseRequireGrant()) + context->checkAccess(AccessType::SHOW_SETTINGS_PROFILES); + std::vector ids = access_control.findAll(); boost::range::push_back(ids, access_control.findAll()); boost::range::push_back(ids, access_control.findAll()); diff --git a/src/Storages/System/StorageSystemSettingsProfiles.cpp b/src/Storages/System/StorageSystemSettingsProfiles.cpp index d03848ba68b..069c8762154 100644 --- a/src/Storages/System/StorageSystemSettingsProfiles.cpp +++ b/src/Storages/System/StorageSystemSettingsProfiles.cpp @@ -34,8 +34,11 @@ NamesAndTypesList StorageSystemSettingsProfiles::getNamesAndTypes() void StorageSystemSettingsProfiles::fillData(MutableColumns & res_columns, ContextPtr context, const SelectQueryInfo &) const { - context->checkAccess(AccessType::SHOW_SETTINGS_PROFILES); + /// If "select_from_system_db_requires_grant" is enabled the access rights were already checked in InterpreterSelectQuery. const auto & access_control = context->getAccessControl(); + if (!access_control.doesSelectFromSystemDatabaseRequireGrant()) + context->checkAccess(AccessType::SHOW_SETTINGS_PROFILES); + std::vector ids = access_control.findAll(); size_t column_index = 0; diff --git a/src/Storages/System/StorageSystemUsers.cpp b/src/Storages/System/StorageSystemUsers.cpp index be56abfa3e8..d7cdf280d14 100644 --- a/src/Storages/System/StorageSystemUsers.cpp +++ b/src/Storages/System/StorageSystemUsers.cpp @@ -60,8 +60,11 @@ NamesAndTypesList StorageSystemUsers::getNamesAndTypes() void StorageSystemUsers::fillData(MutableColumns & res_columns, ContextPtr context, const SelectQueryInfo &) const { - context->checkAccess(AccessType::SHOW_USERS); + /// If "select_from_system_db_requires_grant" is enabled the access rights were already checked in InterpreterSelectQuery. const auto & access_control = context->getAccessControl(); + if (!access_control.doesSelectFromSystemDatabaseRequireGrant()) + context->checkAccess(AccessType::SHOW_USERS); + std::vector ids = access_control.findAll(); size_t column_index = 0; From 5691a859d6228100537cdd424cbbb857c00f064f Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Mon, 11 Jul 2022 18:22:21 +0200 Subject: [PATCH 09/10] Add tests. --- .../disable_access_control_improvements.xml | 2 + .../configs/users.d/another_user.xml | 3 + .../test_select_from_system_tables.py | 162 +++++++++++++++ .../configs/another_user.xml | 16 ++ .../test_select_from_system_tables.py | 192 ++++++++++++++++++ 5 files changed, 375 insertions(+) create mode 100644 tests/integration/test_disabled_access_control_improvements/test_select_from_system_tables.py create mode 100644 tests/integration/test_select_access_rights/configs/another_user.xml create mode 100644 tests/integration/test_select_access_rights/test_select_from_system_tables.py diff --git a/tests/integration/test_disabled_access_control_improvements/configs/config.d/disable_access_control_improvements.xml b/tests/integration/test_disabled_access_control_improvements/configs/config.d/disable_access_control_improvements.xml index 0192e211b68..7969c638fd7 100644 --- a/tests/integration/test_disabled_access_control_improvements/configs/config.d/disable_access_control_improvements.xml +++ b/tests/integration/test_disabled_access_control_improvements/configs/config.d/disable_access_control_improvements.xml @@ -1,5 +1,7 @@ + + diff --git a/tests/integration/test_disabled_access_control_improvements/configs/users.d/another_user.xml b/tests/integration/test_disabled_access_control_improvements/configs/users.d/another_user.xml index 19249011968..476072bd138 100644 --- a/tests/integration/test_disabled_access_control_improvements/configs/users.d/another_user.xml +++ b/tests/integration/test_disabled_access_control_improvements/configs/users.d/another_user.xml @@ -13,6 +13,9 @@ default default + + mydb + diff --git a/tests/integration/test_disabled_access_control_improvements/test_select_from_system_tables.py b/tests/integration/test_disabled_access_control_improvements/test_select_from_system_tables.py new file mode 100644 index 00000000000..5d760c9fc2c --- /dev/null +++ b/tests/integration/test_disabled_access_control_improvements/test_select_from_system_tables.py @@ -0,0 +1,162 @@ +import os +import pytest +from helpers.cluster import ClickHouseCluster +from helpers.test_tools import TSV + +cluster = ClickHouseCluster(__file__) +node = cluster.add_instance( + "node", + main_configs=["configs/config.d/disable_access_control_improvements.xml"], + user_configs=[ + "configs/users.d/another_user.xml", + ], +) + + +@pytest.fixture(scope="module", autouse=True) +def started_cluster(): + try: + cluster.start() + node.query("CREATE DATABASE mydb") + node.query("CREATE TABLE mydb.table1(x UInt32) ENGINE=Log") + node.query("CREATE TABLE table2(x UInt32) ENGINE=Log") + yield cluster + + finally: + cluster.shutdown() + + +@pytest.fixture(autouse=True) +def reset_after_test(): + try: + node.query("CREATE USER OR REPLACE sqluser") + yield + finally: + pass + + +def test_system_db(): + assert node.query("SELECT count()>0 FROM system.settings") == "1\n" + assert node.query("SELECT count()>0 FROM system.users") == "1\n" + assert node.query("SELECT count()>0 FROM system.clusters") == "1\n" + assert node.query("SELECT count() FROM system.tables WHERE name='table1'") == "1\n" + assert node.query("SELECT count() FROM system.tables WHERE name='table2'") == "1\n" + + assert node.query("SELECT count()>0 FROM system.settings", user="another") == "1\n" + expected_error = "necessary to have grant SHOW USERS ON *.*" + assert expected_error in node.query_and_get_error( + "SELECT count()>0 FROM system.users", user="another" + ) + assert node.query("SELECT count()>0 FROM system.clusters", user="another") == "1\n" + assert ( + node.query( + "SELECT count() FROM system.tables WHERE name='table1'", user="another" + ) + == "1\n" + ) + assert ( + node.query( + "SELECT count() FROM system.tables WHERE name='table2'", user="another" + ) + == "0\n" + ) + + assert node.query("SELECT count()>0 FROM system.settings", user="sqluser") == "1\n" + expected_error = "necessary to have grant SHOW USERS ON *.*" + assert expected_error in node.query_and_get_error( + "SELECT count()>0 FROM system.users", user="sqluser" + ) + assert node.query("SELECT count()>0 FROM system.clusters", user="sqluser") == "1\n" + assert ( + node.query( + "SELECT count() FROM system.tables WHERE name='table1'", user="sqluser" + ) + == "0\n" + ) + assert ( + node.query( + "SELECT count() FROM system.tables WHERE name='table2'", user="sqluser" + ) + == "0\n" + ) + + node.query("GRANT SHOW USERS ON *.* TO sqluser") + node.query("GRANT SHOW ON mydb.table1 TO sqluser") + node.query("GRANT SHOW ON table2 TO sqluser") + assert node.query("SELECT count()>0 FROM system.settings", user="sqluser") == "1\n" + assert node.query("SELECT count()>0 FROM system.users", user="sqluser") == "1\n" + assert node.query("SELECT count()>0 FROM system.clusters", user="sqluser") == "1\n" + assert ( + node.query( + "SELECT count() FROM system.tables WHERE name='table1'", user="sqluser" + ) + == "1\n" + ) + assert ( + node.query( + "SELECT count() FROM system.tables WHERE name='table2'", user="sqluser" + ) + == "1\n" + ) + + +def test_information_schema(): + assert ( + node.query( + "SELECT count() FROM information_schema.tables WHERE table_name='table1'" + ) + == "1\n" + ) + assert ( + node.query( + "SELECT count() FROM information_schema.tables WHERE table_name='table2'" + ) + == "1\n" + ) + + assert ( + node.query( + "SELECT count() FROM information_schema.tables WHERE table_name='table1'", + user="another", + ) + == "1\n" + ) + assert ( + node.query( + "SELECT count() FROM information_schema.tables WHERE table_name='table2'", + user="another", + ) + == "0\n" + ) + + assert ( + node.query( + "SELECT count() FROM information_schema.tables WHERE table_name='table1'", + user="sqluser", + ) + == "0\n" + ) + assert ( + node.query( + "SELECT count() FROM information_schema.tables WHERE table_name='table2'", + user="sqluser", + ) + == "0\n" + ) + + node.query("GRANT SHOW ON mydb.table1 TO sqluser") + node.query("GRANT SHOW ON table2 TO sqluser") + assert ( + node.query( + "SELECT count() FROM information_schema.tables WHERE table_name='table1'", + user="sqluser", + ) + == "1\n" + ) + assert ( + node.query( + "SELECT count() FROM information_schema.tables WHERE table_name='table2'", + user="sqluser", + ) + == "1\n" + ) diff --git a/tests/integration/test_select_access_rights/configs/another_user.xml b/tests/integration/test_select_access_rights/configs/another_user.xml new file mode 100644 index 00000000000..627ebccdada --- /dev/null +++ b/tests/integration/test_select_access_rights/configs/another_user.xml @@ -0,0 +1,16 @@ + + + + + + + ::/0 + + default + default + + mydb + + + + diff --git a/tests/integration/test_select_access_rights/test_select_from_system_tables.py b/tests/integration/test_select_access_rights/test_select_from_system_tables.py new file mode 100644 index 00000000000..ac938a9694a --- /dev/null +++ b/tests/integration/test_select_access_rights/test_select_from_system_tables.py @@ -0,0 +1,192 @@ +import os +import pytest +from helpers.cluster import ClickHouseCluster +from helpers.test_tools import TSV + +cluster = ClickHouseCluster(__file__) +node = cluster.add_instance( + "node", + user_configs=[ + "configs/another_user.xml", + ], +) + + +@pytest.fixture(scope="module", autouse=True) +def started_cluster(): + try: + cluster.start() + node.query("CREATE DATABASE mydb") + node.query("CREATE TABLE mydb.table1(x UInt32) ENGINE=Log") + node.query("CREATE TABLE table2(x UInt32) ENGINE=Log") + yield cluster + + finally: + cluster.shutdown() + + +@pytest.fixture(autouse=True) +def reset_after_test(): + try: + node.query("CREATE USER OR REPLACE sqluser") + yield + finally: + pass + + +def test_system_db(): + assert node.query("SELECT count()>0 FROM system.settings") == "1\n" + assert node.query("SELECT count()>0 FROM system.users") == "1\n" + assert node.query("SELECT count()>0 FROM system.clusters") == "1\n" + assert node.query("SELECT count() FROM system.tables WHERE name='table1'") == "1\n" + assert node.query("SELECT count() FROM system.tables WHERE name='table2'") == "1\n" + + assert node.query("SELECT count()>0 FROM system.settings", user="another") == "1\n" + + expected_error = ( + "necessary to have grant SELECT for at least one column on system.users" + ) + assert expected_error in node.query_and_get_error( + "SELECT count()>0 FROM system.users", user="another" + ) + + expected_error = ( + "necessary to have grant SELECT for at least one column on system.clusters" + ) + assert expected_error in node.query_and_get_error( + "SELECT count()>0 FROM system.clusters", user="another" + ) + assert ( + node.query( + "SELECT count() FROM system.tables WHERE name='table1'", user="another" + ) + == "1\n" + ) + assert ( + node.query( + "SELECT count() FROM system.tables WHERE name='table2'", user="another" + ) + == "0\n" + ) + + assert node.query("SELECT count()>0 FROM system.settings", user="sqluser") == "1\n" + + expected_error = ( + "necessary to have grant SELECT for at least one column on system.users" + ) + assert expected_error in node.query_and_get_error( + "SELECT count()>0 FROM system.users", user="sqluser" + ) + + expected_error = ( + "necessary to have grant SELECT for at least one column on system.clusters" + ) + assert node.query_and_get_error( + "SELECT count()>0 FROM system.clusters", user="sqluser" + ) + + assert ( + node.query( + "SELECT count() FROM system.tables WHERE name='table1'", user="sqluser" + ) + == "0\n" + ) + assert ( + node.query( + "SELECT count() FROM system.tables WHERE name='table2'", user="sqluser" + ) + == "0\n" + ) + + node.query("GRANT SELECT ON system.users TO sqluser") + node.query("GRANT SELECT ON system.clusters TO sqluser") + node.query("GRANT SHOW ON mydb.table1 TO sqluser") + node.query("GRANT SHOW ON table2 TO sqluser") + assert node.query("SELECT count()>0 FROM system.settings", user="sqluser") == "1\n" + assert node.query("SELECT count()>0 FROM system.users", user="sqluser") == "1\n" + assert node.query("SELECT count()>0 FROM system.clusters", user="sqluser") == "1\n" + assert ( + node.query( + "SELECT count() FROM system.tables WHERE name='table1'", user="sqluser" + ) + == "1\n" + ) + assert ( + node.query( + "SELECT count() FROM system.tables WHERE name='table2'", user="sqluser" + ) + == "1\n" + ) + + node.query("REVOKE ALL ON *.* FROM sqluser") + node.query("GRANT SHOW USERS ON *.* TO sqluser") + assert node.query("SELECT count()>0 FROM system.users", user="sqluser") == "1\n" + + +def test_information_schema(): + assert ( + node.query( + "SELECT count() FROM information_schema.tables WHERE table_name='table1'" + ) + == "1\n" + ) + assert ( + node.query( + "SELECT count() FROM information_schema.tables WHERE table_name='table2'" + ) + == "1\n" + ) + + expected_error = ( + "necessary to have grant SELECT(table_name) ON information_schema.tables" + ) + assert expected_error in node.query_and_get_error( + "SELECT count() FROM information_schema.tables WHERE table_name='table1'", + user="another", + ) + assert expected_error in node.query_and_get_error( + "SELECT count() FROM information_schema.tables WHERE table_name='table2'", + user="another", + ) + + assert expected_error in node.query_and_get_error( + "SELECT count() FROM information_schema.tables WHERE table_name='table1'", + user="sqluser", + ) + assert expected_error in node.query_and_get_error( + "SELECT count() FROM information_schema.tables WHERE table_name='table2'", + user="sqluser", + ) + + node.query("GRANT SELECT ON information_schema.* TO sqluser") + assert ( + node.query( + "SELECT count() FROM information_schema.tables WHERE table_name='table1'", + user="sqluser", + ) + == "0\n" + ) + assert ( + node.query( + "SELECT count() FROM information_schema.tables WHERE table_name='table2'", + user="sqluser", + ) + == "0\n" + ) + + node.query("GRANT SHOW ON mydb.table1 TO sqluser") + node.query("GRANT SHOW ON table2 TO sqluser") + assert ( + node.query( + "SELECT count() FROM information_schema.tables WHERE table_name='table1'", + user="sqluser", + ) + == "1\n" + ) + assert ( + node.query( + "SELECT count() FROM information_schema.tables WHERE table_name='table2'", + user="sqluser", + ) + == "1\n" + ) From 3eb847f449dc1e55f883a24fe7083362246bf0f1 Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Fri, 15 Jul 2022 14:43:15 +0200 Subject: [PATCH 10/10] Small correction. --- src/TableFunctions/TableFunctionViewIfPermitted.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/TableFunctions/TableFunctionViewIfPermitted.cpp b/src/TableFunctions/TableFunctionViewIfPermitted.cpp index 72469cf918c..dbc4d40d079 100644 --- a/src/TableFunctions/TableFunctionViewIfPermitted.cpp +++ b/src/TableFunctions/TableFunctionViewIfPermitted.cpp @@ -85,7 +85,7 @@ bool TableFunctionViewIfPermitted::isPermitted(const ContextPtr & context, const catch (Exception & e) { if (e.code() == ErrorCodes::ACCESS_DENIED) - return {}; + return false; throw; }