diff --git a/ydb/core/security/ticket_parser_ut.cpp b/ydb/core/security/ticket_parser_ut.cpp index 442c8628e21b..72d72ff7bd7f 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 00b51f0a743a..7c2e43e0e03b 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 @@ class TAlterLogin: public TSubOperationBase { } 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().Key(group).Delete(); - for (const TString& parent : response.TouchedGroups) { - db.Table().Key(parent, group).Delete(); - } result->SetStatus(NKikimrScheme::StatusSuccess); } break; @@ -249,20 +241,8 @@ class TAlterLogin: public TSubOperationBase { 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 @@ class TAlterLogin: public TSubOperationBase { 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().Key(group).Delete(); + for (const TString& parent : removeGroupResponse.TouchedGroups) { + db.Table().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 ac12c4ccb7a8..f7b04092c3e3 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& expectedResults) { + auto modifyTx = std::make_unique(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& expectedResults) { auto modifyTx = std::make_unique(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 a9f4e9bce29b..bcca433cc1cc 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& expectedResults = {{NKikimrScheme::StatusSuccess}}); + + void CreateAlterLoginRemoveGroup(TTestActorRuntime& runtime, ui64 txId, const TString& database, + const TString& group, const TVector& 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 94eeb6975d51..25804068e46c 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 members) { return [=](const NKikimrScheme::TEvDescribeSchemeResult& record) { std::optional> 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 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 1ed97ba6ca3c..1813edca62b5 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 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 bf6fb1aaf0fe..bce1aa86ab36 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(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(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{{ + 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{{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{{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 dd779cdac5b8..e0236e1be3cd 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 a40b509b9064..1ec45941bfaf 100644 --- a/ydb/library/login/login.h +++ b/ydb/library/login/login.h @@ -140,11 +140,6 @@ class TLoginProvider { std::vector TouchedGroups; }; - struct TRemoveGroupRequest : TBasicRequest { - TString Group; - bool MissingOk; - }; - struct TRemoveGroupResponse : TBasicResponse { std::vector TouchedGroups; }; @@ -202,7 +197,8 @@ class TLoginProvider { 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);