diff --git a/src/Access/DiskAccessStorage.cpp b/src/Access/DiskAccessStorage.cpp index eae63976071..fff872af290 100644 --- a/src/Access/DiskAccessStorage.cpp +++ b/src/Access/DiskAccessStorage.cpp @@ -658,13 +658,6 @@ void DiskAccessStorage::deleteAccessEntityOnDisk(const UUID & id) const } -std::vector> DiskAccessStorage::readAllForBackup(AccessEntityType type, const BackupSettings &) const -{ - if (!isBackupAllowed()) - throwBackupNotAllowed(); - return readAllWithIDs(type); -} - void DiskAccessStorage::insertFromBackup( const std::vector> & entities_from_backup, const RestoreSettings & restore_settings, diff --git a/src/Access/DiskAccessStorage.h b/src/Access/DiskAccessStorage.h index 79ae5a86c7d..a011403f434 100644 --- a/src/Access/DiskAccessStorage.h +++ b/src/Access/DiskAccessStorage.h @@ -30,7 +30,6 @@ public: bool exists(const UUID & id) const override; bool isBackupAllowed() const override { return backup_allowed; } - std::vector> readAllForBackup(AccessEntityType type, const BackupSettings & backup_settings) const override; void insertFromBackup(const std::vector> & entities_from_backup, const RestoreSettings & restore_settings, std::shared_ptr restore_coordination) override; private: diff --git a/src/Access/IAccessEntity.h b/src/Access/IAccessEntity.h index c4c7e0e00e1..5614a172f6f 100644 --- a/src/Access/IAccessEntity.h +++ b/src/Access/IAccessEntity.h @@ -52,6 +52,9 @@ struct IAccessEntity /// Replaces dependencies according to a specified map. virtual void replaceDependencies(const std::unordered_map & /* old_to_new_ids */) {} + /// Whether this access entity should be written to a backup. + virtual bool isBackupAllowed() const { return false; } + protected: String name; diff --git a/src/Access/IAccessStorage.cpp b/src/Access/IAccessStorage.cpp index 82ef935dda0..d2bb1ad1bac 100644 --- a/src/Access/IAccessStorage.cpp +++ b/src/Access/IAccessStorage.cpp @@ -10,6 +10,7 @@ #include #include #include +#include namespace DB @@ -524,11 +525,14 @@ bool IAccessStorage::isRestoreAllowed() const return isBackupAllowed() && !isReadOnly(); } -std::vector> IAccessStorage::readAllForBackup(AccessEntityType, const BackupSettings &) const +std::vector> IAccessStorage::readAllForBackup(AccessEntityType type, const BackupSettings &) const { if (!isBackupAllowed()) throwBackupNotAllowed(); - throw Exception(ErrorCodes::NOT_IMPLEMENTED, "readAllForBackup() is not implemented in {}", getStorageType()); + + auto res = readAllWithIDs(type); + boost::range::remove_erase_if(res, [](const std::pair & x) { return !x.second->isBackupAllowed(); }); + return res; } void IAccessStorage::insertFromBackup(const std::vector> &, const RestoreSettings &, std::shared_ptr) diff --git a/src/Access/MemoryAccessStorage.cpp b/src/Access/MemoryAccessStorage.cpp index a82776cc0e3..82fc5e1bdd0 100644 --- a/src/Access/MemoryAccessStorage.cpp +++ b/src/Access/MemoryAccessStorage.cpp @@ -272,14 +272,6 @@ void MemoryAccessStorage::setAll(const std::vector> MemoryAccessStorage::readAllForBackup(AccessEntityType type, const BackupSettings &) const -{ - if (!isBackupAllowed()) - throwBackupNotAllowed(); - - return readAllWithIDs(type); -} - void MemoryAccessStorage::insertFromBackup( const std::vector> & entities_from_backup, const RestoreSettings & restore_settings, diff --git a/src/Access/MemoryAccessStorage.h b/src/Access/MemoryAccessStorage.h index f35244a2668..a26776a0563 100644 --- a/src/Access/MemoryAccessStorage.h +++ b/src/Access/MemoryAccessStorage.h @@ -28,7 +28,6 @@ public: bool exists(const UUID & id) const override; bool isBackupAllowed() const override { return backup_allowed; } - std::vector> readAllForBackup(AccessEntityType type, const BackupSettings & backup_settings) const override; void insertFromBackup(const std::vector> & entities_from_backup, const RestoreSettings & restore_settings, std::shared_ptr restore_coordination) override; private: diff --git a/src/Access/Quota.h b/src/Access/Quota.h index 37f7237dd07..eb9edb14fb0 100644 --- a/src/Access/Quota.h +++ b/src/Access/Quota.h @@ -48,6 +48,7 @@ struct Quota : public IAccessEntity std::vector findDependencies() const override; void replaceDependencies(const std::unordered_map & old_to_new_ids) override; + bool isBackupAllowed() const override { return true; } }; using QuotaPtr = std::shared_ptr; diff --git a/src/Access/ReplicatedAccessStorage.cpp b/src/Access/ReplicatedAccessStorage.cpp index 86c7b6599ea..64aa911f650 100644 --- a/src/Access/ReplicatedAccessStorage.cpp +++ b/src/Access/ReplicatedAccessStorage.cpp @@ -613,13 +613,6 @@ AccessEntityPtr ReplicatedAccessStorage::readImpl(const UUID & id, bool throw_if return entry.entity; } -std::vector> ReplicatedAccessStorage::readAllForBackup(AccessEntityType type, const BackupSettings &) const -{ - if (!isBackupAllowed()) - throwBackupNotAllowed(); - return readAllWithIDs(type); -} - void ReplicatedAccessStorage::insertFromBackup(const std::vector> & entities_from_backup, const RestoreSettings & restore_settings, std::shared_ptr restore_coordination) { if (!isRestoreAllowed()) diff --git a/src/Access/ReplicatedAccessStorage.h b/src/Access/ReplicatedAccessStorage.h index 5653d0f44c3..08cfcc207a5 100644 --- a/src/Access/ReplicatedAccessStorage.h +++ b/src/Access/ReplicatedAccessStorage.h @@ -37,7 +37,6 @@ public: bool exists(const UUID & id) const override; bool isBackupAllowed() const override { return backup_allowed; } - std::vector> readAllForBackup(AccessEntityType type, const BackupSettings & backup_settings) const override; void insertFromBackup(const std::vector> & entities_from_backup, const RestoreSettings & restore_settings, std::shared_ptr restore_coordination) override; private: diff --git a/src/Access/Role.h b/src/Access/Role.h index 19b6662f937..9ddf1b49c78 100644 --- a/src/Access/Role.h +++ b/src/Access/Role.h @@ -22,6 +22,7 @@ struct Role : public IAccessEntity std::vector findDependencies() const override; void replaceDependencies(const std::unordered_map & old_to_new_ids) override; + bool isBackupAllowed() const override { return settings.isBackupAllowed(); } }; using RolePtr = std::shared_ptr; diff --git a/src/Access/RowPolicy.h b/src/Access/RowPolicy.h index f36efbf9f09..99e6f1992f5 100644 --- a/src/Access/RowPolicy.h +++ b/src/Access/RowPolicy.h @@ -48,6 +48,7 @@ struct RowPolicy : public IAccessEntity std::vector findDependencies() const override; void replaceDependencies(const std::unordered_map & old_to_new_ids) override; + bool isBackupAllowed() const override { return true; } /// Which roles or users should use this row policy. RolesOrUsersSet to_roles; diff --git a/src/Access/SettingsProfile.h b/src/Access/SettingsProfile.h index 0e1e763b40f..f85630d324d 100644 --- a/src/Access/SettingsProfile.h +++ b/src/Access/SettingsProfile.h @@ -23,6 +23,7 @@ struct SettingsProfile : public IAccessEntity std::vector findDependencies() const override; void replaceDependencies(const std::unordered_map & old_to_new_ids) override; + bool isBackupAllowed() const override { return elements.isBackupAllowed(); } }; using SettingsProfilePtr = std::shared_ptr; diff --git a/src/Access/SettingsProfileElement.cpp b/src/Access/SettingsProfileElement.cpp index 842ccb03b3a..465f26f37d9 100644 --- a/src/Access/SettingsProfileElement.cpp +++ b/src/Access/SettingsProfileElement.cpp @@ -12,6 +12,13 @@ namespace DB { + +namespace +{ + constexpr const char ALLOW_BACKUP_SETTING_NAME[] = "allow_backup"; +} + + SettingsProfileElement::SettingsProfileElement(const ASTSettingsProfileElement & ast) { init(ast, nullptr); @@ -41,7 +48,10 @@ void SettingsProfileElement::init(const ASTSettingsProfileElement & ast, const A /// Optionally check if a setting with that name is allowed. if (access_control) - access_control->checkSettingNameIsAllowed(setting_name); + { + if (setting_name != ALLOW_BACKUP_SETTING_NAME) + access_control->checkSettingNameIsAllowed(setting_name); + } value = ast.value; min_value = ast.min_value; @@ -168,8 +178,11 @@ Settings SettingsProfileElements::toSettings() const Settings res; for (const auto & elem : *this) { - if (!elem.setting_name.empty() && !elem.value.isNull()) - res.set(elem.setting_name, elem.value); + if (!elem.setting_name.empty() && (elem.setting_name != ALLOW_BACKUP_SETTING_NAME)) + { + if (!elem.value.isNull()) + res.set(elem.setting_name, elem.value); + } } return res; } @@ -179,8 +192,11 @@ SettingsChanges SettingsProfileElements::toSettingsChanges() const SettingsChanges res; for (const auto & elem : *this) { - if (!elem.setting_name.empty() && !elem.value.isNull()) - res.push_back({elem.setting_name, elem.value}); + if (!elem.setting_name.empty() && (elem.setting_name != ALLOW_BACKUP_SETTING_NAME)) + { + if (!elem.value.isNull()) + res.push_back({elem.setting_name, elem.value}); + } } return res; } @@ -190,7 +206,7 @@ SettingsConstraints SettingsProfileElements::toSettingsConstraints(const AccessC SettingsConstraints res{access_control}; for (const auto & elem : *this) { - if (!elem.setting_name.empty()) + if (!elem.setting_name.empty() && (elem.setting_name != ALLOW_BACKUP_SETTING_NAME)) { if (!elem.min_value.isNull()) res.setMinValue(elem.setting_name, elem.min_value); @@ -219,5 +235,14 @@ std::vector SettingsProfileElements::toProfileIDs() const return res; } +bool SettingsProfileElements::isBackupAllowed() const +{ + for (const auto & setting : *this) + { + if (setting.setting_name == ALLOW_BACKUP_SETTING_NAME) + return static_cast(SettingFieldBool{setting.value}); + } + return true; +} } diff --git a/src/Access/SettingsProfileElement.h b/src/Access/SettingsProfileElement.h index e171119e46c..818e7804a76 100644 --- a/src/Access/SettingsProfileElement.h +++ b/src/Access/SettingsProfileElement.h @@ -67,6 +67,8 @@ public: SettingsChanges toSettingsChanges() const; SettingsConstraints toSettingsConstraints(const AccessControl & access_control) const; std::vector toProfileIDs() const; + + bool isBackupAllowed() const; }; } diff --git a/src/Access/User.h b/src/Access/User.h index ce1296bf047..958d8bb486f 100644 --- a/src/Access/User.h +++ b/src/Access/User.h @@ -32,6 +32,7 @@ struct User : public IAccessEntity std::vector findDependencies() const override; void replaceDependencies(const std::unordered_map & old_to_new_ids) override; + bool isBackupAllowed() const override { return settings.isBackupAllowed(); } }; using UserPtr = std::shared_ptr; diff --git a/tests/integration/test_backup_restore_new/test.py b/tests/integration/test_backup_restore_new/test.py index a8f9e2cece1..0fff1683426 100644 --- a/tests/integration/test_backup_restore_new/test.py +++ b/tests/integration/test_backup_restore_new/test.py @@ -453,7 +453,7 @@ def test_temporary_table(): ) == TSV([["e"], ["q"], ["w"]]) -# "BACKUP DATABASE _temporary_and_external_tables" is allowed but the backup must not contain any tables. +# "BACKUP DATABASE _temporary_and_external_tables" is allowed but the backup must not contain these tables. def test_temporary_tables_database(): session_id = new_session_id() instance.http_query( @@ -607,8 +607,10 @@ def test_system_users_required_privileges(): instance.query("CREATE USER u1 DEFAULT ROLE r1") instance.query("GRANT SELECT ON test.* TO u1") + # SETTINGS allow_backup=false means the following user won't be included in backups. + instance.query("CREATE USER u2 SETTINGS allow_backup=false") + backup_name = new_backup_name() - instance.query("CREATE USER u2") expected_error = "necessary to have grant BACKUP ON system.users" assert expected_error in instance.query_and_get_error( @@ -630,18 +632,27 @@ def test_system_users_required_privileges(): instance.query("DROP USER u1") instance.query("DROP ROLE r1") - expected_error = "necessary to have grant CREATE USER ON *.*" - assert expected_error in instance.query_and_get_error(f"RESTORE ALL FROM {backup_name}", user="u2") + expected_error = ( + "necessary to have grant CREATE USER, CREATE ROLE, ROLE ADMIN ON *.*" + ) + assert expected_error in instance.query_and_get_error( + f"RESTORE ALL FROM {backup_name}", user="u2" + ) - instance.query("GRANT CREATE USER ON *.* TO u2") + instance.query("GRANT CREATE USER, CREATE ROLE, ROLE ADMIN ON *.* TO u2") + + expected_error = "necessary to have grant SELECT ON test.* WITH GRANT OPTION" + assert expected_error in instance.query_and_get_error( + f"RESTORE ALL FROM {backup_name}", user="u2" + ) + + instance.query("GRANT SELECT ON test.* TO u2 WITH GRANT OPTION") instance.query(f"RESTORE ALL FROM {backup_name}", user="u2") - assert ( - instance.query("SHOW CREATE USER u1") - == "CREATE USER u1 IDENTIFIED WITH sha256_password SETTINGS PROFILE default, custom_a = 1\n" - ) + assert instance.query("SHOW CREATE USER u1") == "CREATE USER u1 DEFAULT ROLE r1\n" assert instance.query("SHOW GRANTS FOR u1") == TSV( - ["GRANT SELECT ON test.* TO u1", "GRANT r2 TO u1"] + ["GRANT SELECT ON test.* TO u1", "GRANT r1 TO u1"] ) + assert instance.query("SHOW CREATE ROLE r1") == "CREATE ROLE r1\n" assert instance.query("SHOW GRANTS FOR r1") == ""