Forbid to remove group with access (#13930)

9 files changed, 208 insertions, 46 deletions

diff --git a/ydb/core/security/ticket_parser_ut.cpp b/ydb/core/security/ticket_parser_ut.cpp
index 442c8628e2..72d72ff7bd 100644
--- a/ydb/core/security/ticket_parser_ut.cpp
+++ b/ydb/core/security/ticket_parser_ut.cpp
@@ -258,7 +258,7 @@ Y_UNIT_TEST_SUITE(TTicketParserTest) {
         UNIT_ASSERT(result->Token->IsExist("group2"));
         UNIT_ASSERT_VALUES_EQUAL(result->Token->GetGroupSIDs().size(), 3);
 
-        provider.RemoveGroup({.Group = "group2"});
+        provider.RemoveGroup("group2");
         provider.CreateGroup({.Group = "group3"});
         provider.AddGroupMembership({.Group = "group3", .Member = "user1"});
         runtime->Send(new IEventHandle(MakeTicketParserID(), sender, new TEvTicketParser::TEvUpdateLoginSecurityState(provider.GetSecurityState())), 0);
diff --git a/ydb/core/tx/schemeshard/schemeshard__operation_alter_login.cpp b/ydb/core/tx/schemeshard/schemeshard__operation_alter_login.cpp
index 00b51f0a74..7c2e43e0e0 100644
--- a/ydb/core/tx/schemeshard/schemeshard__operation_alter_login.cpp
+++ b/ydb/core/tx/schemeshard/schemeshard__operation_alter_login.cpp
@@ -182,18 +182,10 @@ public:
                 }
                 case NKikimrSchemeOp::TAlterLogin::kRemoveGroup: {
                     const auto& removeGroup = alterLogin.GetRemoveGroup();
-                    const TString& group = removeGroup.GetGroup();
-                    auto response = context.SS->LoginProvider.RemoveGroup({
-                        .Group = group,
-                        .MissingOk = removeGroup.GetMissingOk()
-                    });
+                    auto response = RemoveGroup(context, removeGroup, db);
                     if (response.Error) {
                         result->SetStatus(NKikimrScheme::StatusPreconditionFailed, response.Error);
                     } else {
-                        db.Table<Schema::LoginSids>().Key(group).Delete();
-                        for (const TString& parent : response.TouchedGroups) {
-                            db.Table<Schema::LoginSidMembers>().Key(parent, group).Delete();
-                        }
                         result->SetStatus(NKikimrScheme::StatusSuccess);
                     }
                     break;
@@ -249,20 +241,8 @@ public:
             return {.Error = "User not found"};
         }
 
-        auto subTree = context.SS->ListSubTree(context.SS->RootPathId(), context.Ctx);
-        for (auto pathId : subTree) {
-            TPathElement::TPtr path = context.SS->PathsById.at(pathId);
-            if (path->Owner == user) {
-                auto pathStr = TPath::Init(pathId, context.SS).PathString();
-                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"};
-            }
+        if (auto canRemove = CanRemoveSid(context, user, "User"); canRemove.Error) {
+            return canRemove;
         }
 
         auto removeUserResponse = context.SS->LoginProvider.RemoveUser(user);
@@ -278,6 +258,53 @@ public:
         return {}; // success
     }
 
+    NLogin::TLoginProvider::TBasicResponse RemoveGroup(TOperationContext& context, const NKikimrSchemeOp::TLoginRemoveGroup& removeGroup, NIceDb::TNiceDb& db) {
+        const TString& group = removeGroup.GetGroup();
+
+        if (!context.SS->LoginProvider.CheckGroupExists(group)) {
+            if (removeGroup.GetMissingOk()) {
+                return {}; // success
+            }
+            return {.Error = "Group not found"};
+        }
+
+        if (auto canRemove = CanRemoveSid(context, group, "Group"); canRemove.Error) {
+            return canRemove;
+        }
+
+        auto removeGroupResponse = context.SS->LoginProvider.RemoveGroup(group);
+        if (removeGroupResponse.Error) {
+            return removeGroupResponse;
+        }
+
+        db.Table<Schema::LoginSids>().Key(group).Delete();
+        for (const TString& parent : removeGroupResponse.TouchedGroups) {
+            db.Table<Schema::LoginSidMembers>().Key(parent, group).Delete();
+        }
+
+        return {}; // success
+    }
+
+    NLogin::TLoginProvider::TBasicResponse CanRemoveSid(TOperationContext& context, const TString sid, const TString& sidType) {
+        auto subTree = context.SS->ListSubTree(context.SS->RootPathId(), context.Ctx);
+        for (auto pathId : subTree) {
+            TPathElement::TPtr path = context.SS->PathsById.at(pathId);
+            if (path->Owner == sid) {
+                auto pathStr = TPath::Init(pathId, context.SS).PathString();
+                return {.Error = TStringBuilder() <<
+                    sidType << " " << sid << " owns " << pathStr << " and can't be removed"};
+            }
+            NACLib::TACL acl(path->ACL);
+            if (acl.HasAccess(sid)) {
+                auto pathStr = TPath::Init(pathId, context.SS).PathString();
+                return {.Error = TStringBuilder() << 
+                    sidType << " " << sid << " has an ACL record on " << pathStr << " and can't be removed"};
+            }
+        }
+
+        return {}; // success
+    }
+
     void AddIsUserAdmin(const TString& user, NLogin::TLoginProvider& loginProvider, TParts& additionalParts) {
         const auto& adminSids = AppData()->AdministrationAllowedSIDs;
         bool isAdmin = adminSids.empty();
diff --git a/ydb/core/tx/schemeshard/ut_helpers/helpers.cpp b/ydb/core/tx/schemeshard/ut_helpers/helpers.cpp
index ac12c4ccb7..f7b04092c3 100644
--- a/ydb/core/tx/schemeshard/ut_helpers/helpers.cpp
+++ b/ydb/core/tx/schemeshard/ut_helpers/helpers.cpp
@@ -1993,6 +1993,18 @@ namespace NSchemeShardUT_Private {
         TestModificationResults(runtime, txId, expectedResults);
     }
 
+    void CreateAlterLoginRemoveGroup(TTestActorRuntime& runtime, ui64 txId, const TString& database, const TString& group, const TVector<TExpectedResult>& expectedResults) {
+        auto modifyTx = std::make_unique<TEvSchemeShard::TEvModifySchemeTransaction>(txId, TTestTxConfig::SchemeShard);
+        auto transaction = modifyTx->Record.AddTransaction();
+        transaction->SetWorkingDir(database);
+        transaction->SetOperationType(NKikimrSchemeOp::EOperationType::ESchemeOpAlterLogin);
+        auto createGroup = transaction->MutableAlterLogin()->MutableRemoveGroup();
+        createGroup->SetGroup(group);
+
+        AsyncSend(runtime, TTestTxConfig::SchemeShard, modifyTx.release());
+        TestModificationResults(runtime, txId, expectedResults);
+    }
+
     void AlterLoginAddGroupMembership(TTestActorRuntime& runtime, ui64 txId, const TString& database, const TString& member, const TString& group, const TVector<TExpectedResult>& expectedResults) {
         auto modifyTx = std::make_unique<TEvSchemeShard::TEvModifySchemeTransaction>(txId, TTestTxConfig::SchemeShard);
         auto transaction = modifyTx->Record.AddTransaction();
diff --git a/ydb/core/tx/schemeshard/ut_helpers/helpers.h b/ydb/core/tx/schemeshard/ut_helpers/helpers.h
index a9f4e9bce2..bcca433cc1 100644
--- a/ydb/core/tx/schemeshard/ut_helpers/helpers.h
+++ b/ydb/core/tx/schemeshard/ut_helpers/helpers.h
@@ -534,6 +534,9 @@ namespace NSchemeShardUT_Private {
 
     void CreateAlterLoginCreateGroup(TTestActorRuntime& runtime, ui64 txId, const TString& database,
         const TString& group, const TVector<TExpectedResult>& expectedResults = {{NKikimrScheme::StatusSuccess}});
+    
+    void CreateAlterLoginRemoveGroup(TTestActorRuntime& runtime, ui64 txId, const TString& database,
+        const TString& group, const TVector<TExpectedResult>& expectedResults = {{NKikimrScheme::StatusSuccess}});
 
     void AlterLoginAddGroupMembership(TTestActorRuntime& runtime, ui64 txId, const TString& database,
         const TString& member, const TString& group,
diff --git a/ydb/core/tx/schemeshard/ut_helpers/ls_checks.cpp b/ydb/core/tx/schemeshard/ut_helpers/ls_checks.cpp
index 94eeb6975d..25804068e4 100644
--- a/ydb/core/tx/schemeshard/ut_helpers/ls_checks.cpp
+++ b/ydb/core/tx/schemeshard/ut_helpers/ls_checks.cpp
@@ -1248,7 +1248,7 @@ 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) {
+            if (sid.GetName() == group && sid.GetType() == NLoginProto::ESidType::GROUP) {
                 actualMembers.emplace();
                 for (const auto& member : sid.GetMembers()) {
                     actualMembers->insert(member);
@@ -1260,6 +1260,16 @@ TCheckFunc HasGroup(const TString& group, const TSet<TString> members) {
     };
 }
 
+TCheckFunc HasNoGroup(const TString& group) {
+    return [=](const NKikimrScheme::TEvDescribeSchemeResult& record) {
+        for (const auto& sid : record.GetPathDescription().GetDomainDescription().GetSecurityState().GetSids()) {
+            if (sid.GetName() == group && sid.GetType() == NLoginProto::ESidType::GROUP) {
+                UNIT_FAIL("Group " + group + " exists");
+            }
+        }
+    };
+}
+
 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);
diff --git a/ydb/core/tx/schemeshard/ut_helpers/ls_checks.h b/ydb/core/tx/schemeshard/ut_helpers/ls_checks.h
index 1ed97ba6ca..1813edca62 100644
--- a/ydb/core/tx/schemeshard/ut_helpers/ls_checks.h
+++ b/ydb/core/tx/schemeshard/ut_helpers/ls_checks.h
@@ -175,6 +175,7 @@ namespace NLs {
     TCheckFunc BackupHistoryCount(ui64 count);
 
     TCheckFunc HasGroup(const TString& group, const TSet<TString> members);
+    TCheckFunc HasNoGroup(const TString& group);
     TCheckFunc HasOwner(const TString& owner);
     TCheckFunc HasRight(const TString& right);
     TCheckFunc HasNoRight(const TString& right);
diff --git a/ydb/core/tx/schemeshard/ut_login/ut_login.cpp b/ydb/core/tx/schemeshard/ut_login/ut_login.cpp
index bf6fb1aaf0..bce1aa86ab 100644
--- a/ydb/core/tx/schemeshard/ut_login/ut_login.cpp
+++ b/ydb/core/tx/schemeshard/ut_login/ut_login.cpp
@@ -73,7 +73,7 @@ void CheckToken(const TString& token, const NKikimrScheme::TEvDescribeSchemeResu
 
 Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {
 
-    Y_UNIT_TEST(Login) {
+    Y_UNIT_TEST(UserLogin) {
         TTestBasicRuntime runtime;
         TTestEnv env(runtime);
         ui64 txId = 100;
@@ -107,7 +107,7 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {
         }
     }
 
-    Y_UNIT_TEST(RemoveLogin) {
+    Y_UNIT_TEST(RemoveUser) {
         TTestBasicRuntime runtime;
         TTestEnv env(runtime);
         ui64 txId = 100;
@@ -125,12 +125,12 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {
         }
     }
 
-    Y_UNIT_TEST(RemoveLogin_NonExisting) {
+    Y_UNIT_TEST(RemoveUser_NonExisting) {
         TTestBasicRuntime runtime;
         TTestEnv env(runtime);
         ui64 txId = 100;
 
-        for (bool missingOk : { false, true}) {
+        for (bool missingOk : {false, true}) {
             auto modifyTx = std::make_unique<TEvSchemeShard::TEvModifySchemeTransaction>(txId, TTestTxConfig::SchemeShard);
             auto transaction = modifyTx->Record.AddTransaction();
             transaction->SetWorkingDir("/MyRoot");
@@ -148,7 +148,7 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {
         }
     }
 
-    Y_UNIT_TEST(RemoveLogin_Groups) {
+    Y_UNIT_TEST(RemoveUser_Groups) {
         TTestBasicRuntime runtime;
         TTestEnv env(runtime);
         ui64 txId = 100;
@@ -193,7 +193,7 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {
         }
     }
 
-    Y_UNIT_TEST(RemoveLogin_Owner) {
+    Y_UNIT_TEST(RemoveUser_Owner) {
         TTestBasicRuntime runtime;
         TTestEnv env(runtime);
         ui64 txId = 100;
@@ -237,7 +237,7 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {
         }
     }
 
-    Y_UNIT_TEST(RemoveLogin_Acl) {
+    Y_UNIT_TEST(RemoveUser_Acl) {
         TTestBasicRuntime runtime;
         TTestEnv env(runtime);
         ui64 txId = 100;
@@ -299,6 +299,117 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {
         }
     }
 
+    Y_UNIT_TEST(RemoveGroup) {
+        TTestBasicRuntime runtime;
+        TTestEnv env(runtime);
+        ui64 txId = 100;
+
+        CreateAlterLoginCreateUser(runtime, ++txId, "/MyRoot", "user1", "password1");
+        CreateAlterLoginCreateGroup(runtime, ++txId, "/MyRoot", "group1");
+        AlterLoginAddGroupMembership(runtime, ++txId, "/MyRoot", "user1", "group1");
+        TestDescribeResult(DescribePath(runtime, "/MyRoot"),
+            {NLs::HasGroup("group1", {"user1"})});
+        
+        CreateAlterLoginRemoveGroup(runtime, ++txId, "/MyRoot", "group1");
+        TestDescribeResult(DescribePath(runtime, "/MyRoot"),
+            {NLs::HasNoGroup("group1")});
+    }
+
+    Y_UNIT_TEST(RemoveGroup_NonExisting) {
+        TTestBasicRuntime runtime;
+        TTestEnv env(runtime);
+        ui64 txId = 100;
+        
+        for (bool missingOk : {false, true}) {
+            auto modifyTx = std::make_unique<TEvSchemeShard::TEvModifySchemeTransaction>(txId, TTestTxConfig::SchemeShard);
+            auto transaction = modifyTx->Record.AddTransaction();
+            transaction->SetWorkingDir("/MyRoot");
+            transaction->SetOperationType(NKikimrSchemeOp::EOperationType::ESchemeOpAlterLogin);
+
+            auto removeUser = transaction->MutableAlterLogin()->MutableRemoveGroup();
+            removeUser->SetGroup("group1");
+            removeUser->SetMissingOk(missingOk);
+
+            AsyncSend(runtime, TTestTxConfig::SchemeShard, modifyTx.release());
+            TestModificationResults(runtime, txId, TVector<TExpectedResult>{{
+                missingOk ? NKikimrScheme::StatusSuccess : NKikimrScheme::StatusPreconditionFailed,
+                missingOk ? "" : "Group not found"
+            }});
+        }
+    }
+
+    Y_UNIT_TEST(RemoveGroup_Owner) {
+        TTestBasicRuntime runtime;
+        TTestEnv env(runtime);
+        ui64 txId = 100;
+
+        CreateAlterLoginCreateGroup(runtime, ++txId, "/MyRoot", "group1");
+        TestDescribeResult(DescribePath(runtime, "/MyRoot"),
+            {NLs::HasGroup("group1", {})});
+
+        AsyncMkDir(runtime, ++txId, "/MyRoot", "Dir1");
+        AsyncModifyACL(runtime, ++txId, "/MyRoot", "Dir1", NACLib::TDiffACL{}.SerializeAsString(), "group1");
+        TestModificationResult(runtime, txId, NKikimrScheme::StatusSuccess);
+        TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1"),
+                {NLs::HasOwner("group1")});
+
+        CreateAlterLoginRemoveGroup(runtime, ++txId, "/MyRoot", "group1",
+            TVector<TExpectedResult>{{NKikimrScheme::StatusPreconditionFailed, "Group group1 owns /MyRoot/Dir1 and can't be removed"}});
+        TestDescribeResult(DescribePath(runtime, "/MyRoot"),
+            {NLs::HasGroup("group1", {})});
+        TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1"),
+                {NLs::HasOwner("group1")});
+
+        AsyncModifyACL(runtime, ++txId, "/MyRoot", "Dir1", NACLib::TDiffACL{}.SerializeAsString(), "root@builtin");
+        TestModificationResult(runtime, txId, NKikimrScheme::StatusSuccess);
+        TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1"),
+                {NLs::HasOwner("root@builtin")});
+        
+        CreateAlterLoginRemoveGroup(runtime, ++txId, "/MyRoot", "group1");
+        TestDescribeResult(DescribePath(runtime, "/MyRoot"),
+            {NLs::HasNoGroup("group1")});
+    }
+
+    Y_UNIT_TEST(RemoveGroup_Acl) {
+        TTestBasicRuntime runtime;
+        TTestEnv env(runtime);
+        ui64 txId = 100;
+
+        CreateAlterLoginCreateGroup(runtime, ++txId, "/MyRoot", "group1");
+        TestDescribeResult(DescribePath(runtime, "/MyRoot"),
+            {NLs::HasGroup("group1", {})});
+
+        AsyncMkDir(runtime, ++txId, "/MyRoot", "Dir1");
+        {
+            NACLib::TDiffACL diffACL;
+            diffACL.AddAccess(NACLib::EAccessType::Allow, NACLib::GenericUse, "group1");
+            AsyncModifyACL(runtime, ++txId, "/MyRoot", "Dir1", diffACL.SerializeAsString(), "");
+        }
+        TestModificationResult(runtime, txId, NKikimrScheme::StatusSuccess);
+        TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1"),
+                {NLs::HasRight("+U:group1")});
+
+        CreateAlterLoginRemoveGroup(runtime, ++txId, "/MyRoot", "group1",
+            TVector<TExpectedResult>{{NKikimrScheme::StatusPreconditionFailed, "Group group1 has an ACL record on /MyRoot/Dir1 and can't be removed"}});
+        TestDescribeResult(DescribePath(runtime, "/MyRoot"),
+            {NLs::HasGroup("group1", {})});
+        TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1"),
+                {NLs::HasRight("+U:group1")});
+
+        {
+            NACLib::TDiffACL diffACL;
+            diffACL.RemoveAccess(NACLib::EAccessType::Allow, NACLib::GenericUse, "group1");
+            AsyncModifyACL(runtime, ++txId, "/MyRoot", "Dir1", diffACL.SerializeAsString(), "");
+        }
+        TestModificationResult(runtime, txId, NKikimrScheme::StatusSuccess);
+        TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1"),
+                {NLs::HasNoRight("+U:group1")});
+        
+        CreateAlterLoginRemoveGroup(runtime, ++txId, "/MyRoot", "group1");
+        TestDescribeResult(DescribePath(runtime, "/MyRoot"),
+            {NLs::HasNoGroup("group1")});
+    }
+
     Y_UNIT_TEST(TestExternalLogin) {
         TTestBasicRuntime runtime;
         TTestEnv env(runtime);
diff --git a/ydb/library/login/login.cpp b/ydb/library/login/login.cpp
index dd779cdac5..e0236e1be3 100644
--- a/ydb/library/login/login.cpp
+++ b/ydb/library/login/login.cpp
@@ -100,6 +100,10 @@ bool TLoginProvider::CheckUserExists(const TString& user) {
     return CheckSubjectExists(user, ESidType::USER);
 }
 
+bool TLoginProvider::CheckGroupExists(const TString& group) {
+    return CheckSubjectExists(group, ESidType::GROUP);
+}
+
 TLoginProvider::TBasicResponse TLoginProvider::ModifyUser(const TModifyUserRequest& request) {
     TBasicResponse response;
 
@@ -278,31 +282,29 @@ TLoginProvider::TRenameGroupResponse TLoginProvider::RenameGroup(const TRenameGr
     return response;
 }
 
-TLoginProvider::TRemoveGroupResponse TLoginProvider::RemoveGroup(const TRemoveGroupRequest& request) {
+TLoginProvider::TRemoveGroupResponse TLoginProvider::RemoveGroup(const TString& group) {
     TRemoveGroupResponse response;
 
-    auto itGroupModify = Sids.find(request.Group);
+    auto itGroupModify = Sids.find(group);
     if (itGroupModify == Sids.end() || itGroupModify->second.Type != ESidType::GROUP) {
-        if (!request.MissingOk) {
-            response.Error = "Group not found";
-        }
+        response.Error = "Group not found";
         return response;
     }
 
-    auto itChildToParentIndex = ChildToParentIndex.find(request.Group);
+    auto itChildToParentIndex = ChildToParentIndex.find(group);
     if (itChildToParentIndex != ChildToParentIndex.end()) {
         for (const TString& parent : itChildToParentIndex->second) {
             auto itGroup = Sids.find(parent);
             if (itGroup != Sids.end()) {
                 response.TouchedGroups.emplace_back(itGroup->first);
-                itGroup->second.Members.erase(request.Group);
+                itGroup->second.Members.erase(group);
             }
         }
         ChildToParentIndex.erase(itChildToParentIndex);
     }
 
     for (const TString& member : itGroupModify->second.Members) {
-        ChildToParentIndex[member].erase(request.Group);
+        ChildToParentIndex[member].erase(group);
     }
 
     Sids.erase(itGroupModify);
diff --git a/ydb/library/login/login.h b/ydb/library/login/login.h
index a40b509b90..1ec45941bf 100644
--- a/ydb/library/login/login.h
+++ b/ydb/library/login/login.h
@@ -140,11 +140,6 @@ public:
         std::vector<TString> TouchedGroups;
     };
 
-    struct TRemoveGroupRequest : TBasicRequest {
-        TString Group;
-        bool MissingOk;
-    };
-
     struct TRemoveGroupResponse : TBasicResponse {
         std::vector<TString> TouchedGroups;
     };
@@ -202,7 +197,8 @@ public:
     TBasicResponse AddGroupMembership(const TAddGroupMembershipRequest& request);
     TBasicResponse RemoveGroupMembership(const TRemoveGroupMembershipRequest& request);
     TRenameGroupResponse RenameGroup(const TRenameGroupRequest& request);
-    TRemoveGroupResponse RemoveGroup(const TRemoveGroupRequest& request);
+    TRemoveGroupResponse RemoveGroup(const TString& group);
+    bool CheckGroupExists(const TString& group);
 
     void UpdatePasswordCheckParameters(const TPasswordComplexity& passwordComplexity);
     void UpdateAccountLockout(const TAccountLockout::TInitializer& accountLockoutInitializer);