Forbid to remove user with access (ydb-platform#13144)

the-ancient-1 · Jan 21, 2025 · 84fc20d · 84fc20d
1 parent 653f1b3
commit 84fc20d
Show file tree

Hide file tree

Showing 12 changed files with 262 additions and 161 deletions.
diff --git a/ydb/core/kqp/ut/scheme/kqp_scheme_ut.cpp b/ydb/core/kqp/ut/scheme/kqp_scheme_ut.cpp
@@ -4066,6 +4066,32 @@ Y_UNIT_TEST_SUITE(KqpScheme) {
             auto result = session.ExecuteSchemeQuery(query).GetValueSync();
             UNIT_ASSERT_VALUES_EQUAL_C(result.GetStatus(), EStatus::SUCCESS, result.GetIssues().ToString());
         }
+        {
+            // Drop user with ACL
+            auto session = db.CreateSession().GetValueSync().GetSession();
+
+            TString query = TStringBuilder() << R"(
+            --!syntax_v1
+            CREATE USER user2 PASSWORD NULL;
+            )";
+            auto result = session.ExecuteSchemeQuery(query).GetValueSync();
+            UNIT_ASSERT_VALUES_EQUAL_C(result.GetStatus(), EStatus::SUCCESS, result.GetIssues().ToString());
+
+            query = TStringBuilder() << R"(
+            --!syntax_v1
+            GRANT ALL ON `/Root` TO user2;
+            )";
+            result = session.ExecuteSchemeQuery(query).GetValueSync();
+            UNIT_ASSERT_VALUES_EQUAL_C(result.GetStatus(), EStatus::SUCCESS, result.GetIssues().ToString());
+
+            query = TStringBuilder() << R"(
+            --!syntax_v1
+            DROP USER user2;
+            )";
+            result = session.ExecuteSchemeQuery(query).GetValueSync();
+            UNIT_ASSERT_VALUES_EQUAL(result.GetStatus(), EStatus::PRECONDITION_FAILED);
+            UNIT_ASSERT_STRING_CONTAINS(result.GetIssues().ToString(), "Error: User user2 has an ACL record on /Root and can't be removed");
+        }
     }
 
     Y_UNIT_TEST(AlterUser) {

diff --git a/ydb/core/tablet_flat/util_fmt_basic.h b/ydb/core/tablet_flat/util_fmt_basic.h
@@ -1,6 +1,7 @@
 #pragma once
 
 #include "util_fmt_desc.h"
+#include <util/datetime/base.h>
 
 namespace NKikimr {
 namespace NFmt {

diff --git a/ydb/core/tx/schemeshard/schemeshard__operation_alter_login.cpp b/ydb/core/tx/schemeshard/schemeshard__operation_alter_login.cpp
@@ -235,38 +235,19 @@ class TAlterLogin: public TSubOperationBase {
                 return {.Error = TStringBuilder() <<
                     "User " << user << " owns " << pathStr << " and can't be removed"};
             }
+            NACLib::TACL acl(path->ACL);
+            if (acl.HasAccess(user)) {
+                auto pathStr = TPath::Init(pathId, context.SS).PathString();
+                return {.Error = TStringBuilder() << 
+                    "User " << user << " has an ACL record on " << pathStr << " and can't be removed"};
+            }
         }
 
         auto removeUserResponse = context.SS->LoginProvider.RemoveUser(user);
         if (removeUserResponse.Error) {
             return removeUserResponse;
         }
 
-        for (auto pathId : subTree) {
-            TPathElement::TPtr path = context.SS->PathsById.at(pathId);
-            NACLib::TACL acl(path->ACL);
-            if (acl.TryRemoveAccess(user)) {
-                ++path->ACLVersion;
-                path->ACL = acl.SerializeAsString();
-                context.SS->PersistACL(db, path);
-                if (!path->IsPQGroup()) {
-                    const auto parent = context.SS->PathsById.at(path->ParentPathId);
-                    ++parent->DirAlterVersion;
-                    context.SS->PersistPathDirAlterVersion(db, parent);
-                    context.SS->ClearDescribePathCaches(parent);
-                    context.OnComplete.PublishToSchemeBoard(OperationId, parent->PathId);
-                }
-            }
-            NACLib::TACL effectiveACL(path->CachedEffectiveACL.GetForSelf());
-            if (effectiveACL.HasAccess(user)) {
-                // publish paths from which the user's access is being removed
-                // user access could have been granted directly (ACL, handled by `acl.TryRemoveAccess(user)` above)
-                // or it might have been inherited from a parent (effective ACL)
-                context.OnComplete.PublishToSchemeBoard(OperationId, pathId);
-            }
-        }
-
-        context.OnComplete.UpdateTenants(std::move(subTree));
         db.Table<Schema::LoginSids>().Key(user).Delete();
         for (const TString& group : removeUserResponse.TouchedGroups) {
             db.Table<Schema::LoginSidMembers>().Key(group, user).Delete();

diff --git a/ydb/core/tx/schemeshard/ut_helpers/ls_checks.cpp b/ydb/core/tx/schemeshard/ut_helpers/ls_checks.cpp
@@ -4,6 +4,7 @@
 #include <ydb/core/scheme/scheme_tablecell.h>
 #include <ydb/core/scheme/scheme_tabledefs.h>
 #include <ydb/core/scheme/scheme_types_proto.h>
+#include <ydb/library/login/protos/login.pb.h>
 #include <ydb/public/lib/scheme_types/scheme_type_id.h>
 #include <ydb/public/api/protos/ydb_cms.pb.h>
 #include <ydb/core/protos/pqconfig.pb.h>
@@ -1243,6 +1244,22 @@ TCheckFunc HasOwner(const TString& owner) {
     };
 }
 
+TCheckFunc HasGroup(const TString& group, const TSet<TString> members) {
+    return [=](const NKikimrScheme::TEvDescribeSchemeResult& record) {
+        std::optional<TSet<TString>> actualMembers;
+        for (const auto& sid : record.GetPathDescription().GetDomainDescription().GetSecurityState().GetSids()) {
+            if (sid.GetName() == group) {
+                actualMembers.emplace();
+                for (const auto& member : sid.GetMembers()) {
+                    actualMembers->insert(member);
+                }
+            }
+        }
+        UNIT_ASSERT_C(actualMembers.has_value(), "Group " + group + " not found");
+        UNIT_ASSERT_VALUES_EQUAL(members, actualMembers.value());
+    };
+}
+
 void CheckRight(const NKikimrScheme::TEvDescribeSchemeResult& record, const TString& right, bool mustHave, bool isEffective) {
     const auto& self = record.GetPathDescription().GetSelf();
     TSecurityObject src(self.GetOwner(), isEffective ? self.GetEffectiveACL() : self.GetACL(), false);
@@ -1263,9 +1280,9 @@ void CheckRight(const NKikimrScheme::TEvDescribeSchemeResult& record, const TStr
             }
         }
 
-        UNIT_ASSERT_C(!(has ^ mustHave), "" << record.GetPath() << " " << (mustHave ? "no " : "") << "ace found"
+        UNIT_ASSERT_C(!(has ^ mustHave), "" << record.GetPath() << "ace check fail"
             << ", got " << src.ShortDebugString()
-            << ", required " << required.ShortDebugString());
+            << ", required " << (mustHave ? "" : "no ") << required.ShortDebugString());
     }
 }
 

diff --git a/ydb/core/tx/schemeshard/ut_helpers/ls_checks.h b/ydb/core/tx/schemeshard/ut_helpers/ls_checks.h
@@ -174,6 +174,7 @@ namespace NLs {
     void NoBackupInFly(const NKikimrScheme::TEvDescribeSchemeResult& record);
     TCheckFunc BackupHistoryCount(ui64 count);
 
+    TCheckFunc HasGroup(const TString& group, const TSet<TString> members);
     TCheckFunc HasOwner(const TString& owner);
     TCheckFunc HasRight(const TString& right);
     TCheckFunc HasNoRight(const TString& right);