diff --git a/ydb/core/kqp/ut/scheme/kqp_scheme_ut.cpp b/ydb/core/kqp/ut/scheme/kqp_scheme_ut.cpp index f9cd4148955a..f2c38d58d261 100644 --- 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 index 1dad4a2a9ad4..0f9e2c9673cd 100644 --- 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 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 index deb9d5ae0f9f..db50c3a7f431 100644 --- a/ydb/core/tx/schemeshard/schemeshard__operation_alter_login.cpp +++ b/ydb/core/tx/schemeshard/schemeshard__operation_alter_login.cpp @@ -203,6 +203,12 @@ 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); @@ -210,31 +216,6 @@ class TAlterLogin: public TSubOperationBase { 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().Key(user).Delete(); for (const TString& group : removeUserResponse.TouchedGroups) { db.Table().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 index af31c7d49c7c..94eeb6975d51 100644 --- 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 #include #include +#include #include #include #include @@ -1243,6 +1244,22 @@ TCheckFunc HasOwner(const TString& owner) { }; } +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) { + 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 index c46f6bc5c610..1ed97ba6ca3c 100644 --- 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 members); 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 94a2ffffa5f2..7c9656b62396 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(BasicLogin) { + Y_UNIT_TEST(Login) { TTestBasicRuntime runtime; TTestEnv env(runtime); ui64 txId = 100; @@ -116,58 +116,39 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) { auto resultLogin = Login(runtime, "user1", "password1"); UNIT_ASSERT_VALUES_EQUAL(resultLogin.error(), ""); - AsyncMkDir(runtime, ++txId, "/MyRoot", "Dir1"); - TestModificationResult(runtime, txId, NKikimrScheme::StatusAccepted); - AsyncMkDir(runtime, ++txId, "/MyRoot", "Dir2"); - TestModificationResult(runtime, txId, NKikimrScheme::StatusAccepted); - AsyncMkDir(runtime, ++txId, "/MyRoot/Dir1", "DirSub1"); - TestModificationResult(runtime, txId, NKikimrScheme::StatusAccepted); - AsyncMkDir(runtime, ++txId, "/MyRoot/Dir1", "DirSub2"); - TestModificationResult(runtime, txId, NKikimrScheme::StatusAccepted); - - NACLib::TDiffACL diffACL; - diffACL.AddAccess(NACLib::EAccessType::Allow, NACLib::GenericUse, "user1"); - diffACL.AddAccess(NACLib::EAccessType::Allow, NACLib::GenericUse, "user2"); - AsyncModifyACL(runtime, ++txId, "", "MyRoot", diffACL.SerializeAsString(), ""); - TestModificationResult(runtime, txId, NKikimrScheme::StatusSuccess); - AsyncModifyACL(runtime, ++txId, "/MyRoot", "Dir1", diffACL.SerializeAsString(), ""); - TestModificationResult(runtime, txId, NKikimrScheme::StatusSuccess); - AsyncModifyACL(runtime, ++txId, "/MyRoot/Dir1", "DirSub2", diffACL.SerializeAsString(), ""); - TestModificationResult(runtime, txId, NKikimrScheme::StatusSuccess); - - TestDescribeResult(DescribePath(runtime, "/MyRoot"), - {NLs::HasRight("+U:user1"), NLs::HasEffectiveRight("+U:user1")}); - TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1"), - {NLs::HasRight("+U:user1"), NLs::HasEffectiveRight("+U:user1")}); - TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir2"), - {NLs::HasNoRight("+U:user1"), NLs::HasEffectiveRight("+U:user1")}); - TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1/DirSub1"), - {NLs::HasNoRight("+U:user1"), NLs::HasEffectiveRight("+U:user1")}); - TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1/DirSub2"), - {NLs::HasRight("+U:user1"), NLs::HasEffectiveRight("+U:user1")}); - CreateAlterLoginRemoveUser(runtime, ++txId, "/MyRoot", "user1"); - // Cerr << DescribePath(runtime, TTestTxConfig::SchemeShard, "/MyRoot/Dir1/DirSub2").DebugString() << Endl; - // Cerr << DescribePath(runtime, TTestTxConfig::SchemeShard, "/MyRoot/Dir1").DebugString() << Endl; - - for (auto path : {"/MyRoot", "/MyRoot/Dir1", "/MyRoot/Dir2", "/MyRoot/Dir1/DirSub1", "/MyRoot/Dir1/DirSub2"}) { - TestDescribeResult(DescribePath(runtime, path), - {NLs::HasNoRight("+U:user1"), NLs::HasNoEffectiveRight("+U:user1")}); - } - - // check login + // check user has been removed: { auto resultLogin = Login(runtime, "user1", "password1"); UNIT_ASSERT_VALUES_EQUAL(resultLogin.GetError(), "Cannot find user: user1"); } + } - // another still has access - TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1"), - {NLs::HasRight("+U:user2"), NLs::HasEffectiveRight("+U:user2")}); + Y_UNIT_TEST(RemoveLogin_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()->MutableRemoveUser(); + removeUser->SetUser("user1"); + removeUser->SetMissingOk(missingOk); + + AsyncSend(runtime, TTestTxConfig::SchemeShard, modifyTx.release()); + TestModificationResults(runtime, txId, TVector{{ + missingOk ? NKikimrScheme::StatusSuccess : NKikimrScheme::StatusPreconditionFailed, + missingOk ? "" : "User not found" + }}); + } } - Y_UNIT_TEST(RemoveLogin_SubTree) { + Y_UNIT_TEST(RemoveLogin_Groups) { TTestBasicRuntime runtime; TTestEnv env(runtime); ui64 txId = 100; @@ -176,77 +157,87 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) { auto resultLogin = Login(runtime, "user1", "password1"); UNIT_ASSERT_VALUES_EQUAL(resultLogin.error(), ""); - AsyncMkDir(runtime, ++txId, "/MyRoot", "Dir1"); - TestModificationResult(runtime, txId, NKikimrScheme::StatusAccepted); - AsyncMkDir(runtime, ++txId, "/MyRoot", "Dir2"); - TestModificationResult(runtime, txId, NKikimrScheme::StatusAccepted); - AsyncMkDir(runtime, ++txId, "/MyRoot/Dir1", "DirSub1"); - TestModificationResult(runtime, txId, NKikimrScheme::StatusAccepted); - AsyncMkDir(runtime, ++txId, "/MyRoot/Dir1", "DirSub2"); - TestModificationResult(runtime, txId, NKikimrScheme::StatusAccepted); + AsyncMkDir(runtime, ++txId, "/MyRoot", "Dir1/DirSub1"); + + CreateAlterLoginCreateGroup(runtime, ++txId, "/MyRoot", "group"); + AlterLoginAddGroupMembership(runtime, ++txId, "/MyRoot", "user1", "group"); + AlterLoginAddGroupMembership(runtime, ++txId, "/MyRoot", "user2", "group"); + TestDescribeResult(DescribePath(runtime, "/MyRoot"), + {NLs::HasGroup("group", {"user1", "user2"})}); NACLib::TDiffACL diffACL; - diffACL.AddAccess(NACLib::EAccessType::Allow, NACLib::GenericUse, "user1"); - diffACL.AddAccess(NACLib::EAccessType::Allow, NACLib::GenericUse, "user2"); + diffACL.AddAccess(NACLib::EAccessType::Allow, NACLib::GenericUse, "group"); AsyncModifyACL(runtime, ++txId, "/MyRoot", "Dir1", diffACL.SerializeAsString(), ""); TestModificationResult(runtime, txId, NKikimrScheme::StatusSuccess); - - TestDescribeResult(DescribePath(runtime, "/MyRoot"), - {NLs::HasNoRight("+U:user1"), NLs::HasNoEffectiveRight("+U:user1")}); - TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1"), - {NLs::HasRight("+U:user1"), NLs::HasEffectiveRight("+U:user1")}); - TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir2"), - {NLs::HasNoRight("+U:user1"), NLs::HasNoEffectiveRight("+U:user1")}); - TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1/DirSub1"), - {NLs::HasNoRight("+U:user1"), NLs::HasEffectiveRight("+U:user1")}); - TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1/DirSub2"), - {NLs::HasNoRight("+U:user1"), NLs::HasEffectiveRight("+U:user1")}); + TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1"),{ + NLs::HasNoRight("+U:user1"), NLs::HasNoEffectiveRight("+U:user1"), + NLs::HasRight("+U:group"), NLs::HasEffectiveRight("+U:group")}); + TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1/DirSub1"),{ + NLs::HasNoRight("+U:user1"), NLs::HasNoEffectiveRight("+U:user1"), + NLs::HasNoRight("+U:group"), NLs::HasEffectiveRight("+U:group")}); CreateAlterLoginRemoveUser(runtime, ++txId, "/MyRoot", "user1"); - // Cerr << DescribePath(runtime, TTestTxConfig::SchemeShard, "/MyRoot/Dir1/DirSub2").DebugString() << Endl; - // Cerr << DescribePath(runtime, TTestTxConfig::SchemeShard, "/MyRoot/Dir1").DebugString() << Endl; - - for (auto path : {"/MyRoot", "/MyRoot/Dir1", "/MyRoot/Dir2", "/MyRoot/Dir1/DirSub1", "/MyRoot/Dir1/DirSub2"}) { - TestDescribeResult(DescribePath(runtime, path), - {NLs::HasNoRight("+U:user1"), NLs::HasNoEffectiveRight("+U:user1")}); - } - - // check login + // check user has been removed: { + TestDescribeResult(DescribePath(runtime, "/MyRoot"), + {NLs::HasGroup("group", {"user2"})}); + TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1"),{ + NLs::HasNoRight("+U:user1"), NLs::HasNoEffectiveRight("+U:user1"), + NLs::HasRight("+U:group"), NLs::HasEffectiveRight("+U:group")}); + TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1/DirSub1"),{ + NLs::HasNoRight("+U:user1"), NLs::HasNoEffectiveRight("+U:user1"), + NLs::HasNoRight("+U:group"), NLs::HasEffectiveRight("+U:group")}); auto resultLogin = Login(runtime, "user1", "password1"); UNIT_ASSERT_VALUES_EQUAL(resultLogin.GetError(), "Cannot find user: user1"); } - - // another still has access - TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1"), - {NLs::HasRight("+U:user2"), NLs::HasEffectiveRight("+U:user2")}); } - Y_UNIT_TEST(RemoveLogin_NonExisting) { + Y_UNIT_TEST(RemoveLogin_Owner) { TTestBasicRuntime runtime; TTestEnv env(runtime); ui64 txId = 100; + CreateAlterLoginCreateUser(runtime, ++txId, "/MyRoot", "user1", "password1"); + CreateAlterLoginCreateUser(runtime, ++txId, "/MyRoot", "user2", "password2"); + auto resultLogin = Login(runtime, "user1", "password1"); + UNIT_ASSERT_VALUES_EQUAL(resultLogin.error(), ""); - 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); + AsyncMkDir(runtime, ++txId, "/MyRoot", "Dir1/DirSub1"); - auto removeUser = transaction->MutableAlterLogin()->MutableRemoveUser(); - removeUser->SetUser("user1"); - removeUser->SetMissingOk(missingOk); + AsyncModifyACL(runtime, ++txId, "/MyRoot/Dir1", "DirSub1", NACLib::TDiffACL{}.SerializeAsString(), "user1"); + TestModificationResult(runtime, txId, NKikimrScheme::StatusSuccess); - AsyncSend(runtime, TTestTxConfig::SchemeShard, modifyTx.release()); - TestModificationResults(runtime, txId, TVector{{ - missingOk ? NKikimrScheme::StatusSuccess : NKikimrScheme::StatusPreconditionFailed, - missingOk ? "" : "User not found" - }}); + TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1/DirSub1"), + {NLs::HasOwner("user1")}); + + // Cerr << DescribePath(runtime, TTestTxConfig::SchemeShard, "/MyRoot/Dir1").DebugString() << Endl; + // Cerr << DescribePath(runtime, TTestTxConfig::SchemeShard, "/MyRoot/Dir1").DebugString() << Endl; + + CreateAlterLoginRemoveUser(runtime, ++txId, "/MyRoot", "user1", + TVector{{NKikimrScheme::StatusPreconditionFailed, "User user1 owns /MyRoot/Dir1/DirSub1 and can't be removed"}}); + + // check user still exists and has their rights: + { + TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1/DirSub1"), + {NLs::HasOwner("user1")}); + auto resultLogin = Login(runtime, "user1", "password1"); + UNIT_ASSERT_VALUES_EQUAL(resultLogin.GetError(), ""); + } + + AsyncModifyACL(runtime, ++txId, "/MyRoot/Dir1", "DirSub1", NACLib::TDiffACL().SerializeAsString(), "user2"); + TestModificationResult(runtime, txId, NKikimrScheme::StatusSuccess); + CreateAlterLoginRemoveUser(runtime, ++txId, "/MyRoot", "user1"); + + // check user has been removed: + { + TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1/DirSub1"), + {NLs::HasOwner("user2")}); + auto resultLogin = Login(runtime, "user1", "password1"); + UNIT_ASSERT_VALUES_EQUAL(resultLogin.GetError(), "Cannot find user: user1"); } } - Y_UNIT_TEST(RemoveLogin_Owner) { + Y_UNIT_TEST(RemoveLogin_Acl) { TTestBasicRuntime runtime; TTestEnv env(runtime); ui64 txId = 100; @@ -257,36 +248,52 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) { AsyncMkDir(runtime, ++txId, "/MyRoot", "Dir1/DirSub1"); - NACLib::TDiffACL diffACL; - diffACL.AddAccess(NACLib::EAccessType::Allow, NACLib::GenericUse, "user1"); - AsyncModifyACL(runtime, ++txId, "/MyRoot/Dir1", "DirSub1", diffACL.SerializeAsString(), "user1"); - TestModificationResult(runtime, txId, NKikimrScheme::StatusSuccess); - - TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1/DirSub1"), - {NLs::HasRight("+U:user1"), NLs::HasEffectiveRight("+U:user1"), NLs::HasOwner("user1")}); + { + NACLib::TDiffACL diffACL; + diffACL.AddAccess(NACLib::EAccessType::Allow, NACLib::GenericUse, "user1"); + AsyncModifyACL(runtime, ++txId, "/MyRoot", "Dir1", diffACL.SerializeAsString(), ""); + TestModificationResult(runtime, txId, NKikimrScheme::StatusSuccess); + TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1"), + {NLs::HasRight("+U:user1"), NLs::HasEffectiveRight("+U:user1")}); + TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1/DirSub1"), + {NLs::HasNoRight("+U:user1"), NLs::HasEffectiveRight("+U:user1")}); + } // Cerr << DescribePath(runtime, TTestTxConfig::SchemeShard, "/MyRoot/Dir1").DebugString() << Endl; // Cerr << DescribePath(runtime, TTestTxConfig::SchemeShard, "/MyRoot/Dir1").DebugString() << Endl; CreateAlterLoginRemoveUser(runtime, ++txId, "/MyRoot", "user1", - TVector{{NKikimrScheme::StatusPreconditionFailed, "User user1 owns /MyRoot/Dir1/DirSub1 and can't be removed"}}); + TVector{{NKikimrScheme::StatusPreconditionFailed, "User user1 has an ACL record on /MyRoot/Dir1 and can't be removed"}}); // check user still exists and has their rights: { + TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1"), + {NLs::HasRight("+U:user1"), NLs::HasEffectiveRight("+U:user1")}); TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1/DirSub1"), - {NLs::HasRight("+U:user1"), NLs::HasEffectiveRight("+U:user1"), NLs::HasOwner("user1")}); + {NLs::HasNoRight("+U:user1"), NLs::HasEffectiveRight("+U:user1")}); auto resultLogin = Login(runtime, "user1", "password1"); UNIT_ASSERT_VALUES_EQUAL(resultLogin.GetError(), ""); } - AsyncModifyACL(runtime, ++txId, "/MyRoot/Dir1", "DirSub1", NACLib::TDiffACL().SerializeAsString(), "user2"); - TestModificationResult(runtime, txId, NKikimrScheme::StatusSuccess); + { + NACLib::TDiffACL diffACL; + diffACL.RemoveAccess(NACLib::EAccessType::Allow, NACLib::GenericUse, "user1"); + AsyncModifyACL(runtime, ++txId, "/MyRoot", "Dir1", diffACL.SerializeAsString(), ""); + TestModificationResult(runtime, txId, NKikimrScheme::StatusSuccess); + TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1"), + {NLs::HasNoRight("+U:user1"), NLs::HasNoEffectiveRight("+U:user1")}); + TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1/DirSub1"), + {NLs::HasNoRight("+U:user1"), NLs::HasNoEffectiveRight("+U:user1")}); + } + CreateAlterLoginRemoveUser(runtime, ++txId, "/MyRoot", "user1"); // check user has been removed: { + TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1"), + {NLs::HasNoRight("+U:user1"), NLs::HasNoEffectiveRight("+U:user1")}); TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1/DirSub1"), - {NLs::HasNoRight("+U:user1"), NLs::HasNoEffectiveRight("+U:user1"), NLs::HasOwner("user2")}); + {NLs::HasNoRight("+U:user1"), NLs::HasNoEffectiveRight("+U:user1")}); auto resultLogin = Login(runtime, "user1", "password1"); UNIT_ASSERT_VALUES_EQUAL(resultLogin.GetError(), "Cannot find user: user1"); } diff --git a/ydb/core/tx/schemeshard/ut_login_large/ut_login_large.cpp b/ydb/core/tx/schemeshard/ut_login_large/ut_login_large.cpp index 84c3496e9d4b..bdc864ae35e8 100644 --- a/ydb/core/tx/schemeshard/ut_login_large/ut_login_large.cpp +++ b/ydb/core/tx/schemeshard/ut_login_large/ut_login_large.cpp @@ -1,4 +1,6 @@ +#include #include +#include #include using namespace NKikimr; @@ -8,23 +10,25 @@ using namespace NSchemeShardUT_Private; struct TLogStopwatch { TLogStopwatch(TString message) : Message(std::move(message)) - , Started(TAppData::TimeProvider->Now()) + , Started(std::chrono::steady_clock::now()) {} ~TLogStopwatch() { - Cerr << "[STOPWATCH] " << Message << " in " << (TAppData::TimeProvider->Now() - Started).MilliSeconds() << "ms" << Endl; + std::chrono::steady_clock::time_point ended = std::chrono::steady_clock::now(); + Cerr << "[STOPWATCH] " << Message << " in " << NFmt::TDelay(TDuration::MicroSeconds(std::chrono::duration_cast(ended - Started).count())) << Endl; } private: TString Message; - TInstant Started; + std::chrono::steady_clock::time_point Started; }; Y_UNIT_TEST_SUITE(TSchemeShardLoginLargeTest) { Y_UNIT_TEST(RemoveLogin_Many) { const size_t pathsToCreate = 10'000; - const size_t usersToAdd = 200; // 2M ACL rules in total + const size_t usersWithAccess = 200; // 2M ACL rules in total + const size_t usersTotal = 300; TTestBasicRuntime runtime; TTestEnv env(runtime); @@ -32,17 +36,17 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginLargeTest) { runtime.SetLogPriority(NKikimrServices::FLAT_TX_SCHEMESHARD, NActors::NLog::PRI_NOTICE); runtime.SetDispatchedEventsLimit(100'000'000'000); - for (auto userId : xrange(usersToAdd)) { + for (auto userId : xrange(usersTotal)) { CreateAlterLoginCreateUser(runtime, ++txId, "/MyRoot", "user" + std::to_string(userId), "password" + std::to_string(userId)); } auto resultLogin = Login(runtime, "user0", "password0"); UNIT_ASSERT_VALUES_EQUAL(resultLogin.error(), ""); { - TLogStopwatch stopwatch(TStringBuilder() << "Created " << pathsToCreate << " paths"); + TLogStopwatch stopwatch(TStringBuilder() << "Created " << pathsToCreate << " paths with " << usersWithAccess * pathsToCreate << " acls"); NACLib::TDiffACL diffACL; - for (auto userId : xrange(usersToAdd)) { + for (auto userId : xrange(usersWithAccess)) { diffACL.AddAccess(NACLib::EAccessType::Allow, NACLib::GenericUse, "user" + std::to_string(userId)); } @@ -79,26 +83,45 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginLargeTest) { Cerr << DescribePath(runtime, TTestTxConfig::SchemeShard, "/MyRoot").DebugString() << Endl; { - TLogStopwatch stopwatch(TStringBuilder() << "Added single root acl"); + TLogStopwatch stopwatch(TStringBuilder() << "Created single user userx"); + CreateAlterLoginCreateUser(runtime, ++txId, "/MyRoot", "userx", "passwordX"); + } + + { + TLogStopwatch stopwatch(TStringBuilder() << "Added single root acl userx"); NACLib::TDiffACL diffACL; - diffACL.AddAccess(NACLib::EAccessType::Allow, NACLib::GenericUse, "userX"); + diffACL.AddAccess(NACLib::EAccessType::Allow, NACLib::GenericUse, "userx"); AsyncModifyACL(runtime, ++txId, "", "MyRoot", diffACL.SerializeAsString(), ""); TestModificationResult(runtime, txId, NKikimrScheme::StatusSuccess); } { - TLogStopwatch stopwatch(TStringBuilder() << "Removed single root acl"); + TLogStopwatch stopwatch(TStringBuilder() << "Removed single root acl userx"); NACLib::TDiffACL diffACL; - diffACL.RemoveAccess(NACLib::EAccessType::Allow, NACLib::GenericUse, "userX"); + diffACL.RemoveAccess(NACLib::EAccessType::Allow, NACLib::GenericUse, "userx"); AsyncModifyACL(runtime, ++txId, "", "MyRoot", diffACL.SerializeAsString(), ""); TestModificationResult(runtime, txId, NKikimrScheme::StatusSuccess); } + + { + TLogStopwatch stopwatch(TStringBuilder() << "Removed single user userx"); + CreateAlterLoginRemoveUser(runtime, ++txId, "/MyRoot", "userx"); + } - for (auto userId : xrange(Min(usersToAdd, 3))) + // removing users without access: + for (auto userId : xrange(usersWithAccess, Min(usersTotal, usersWithAccess + 3))) { TLogStopwatch stopwatch(TStringBuilder() << "Removed user" + std::to_string(userId)); CreateAlterLoginRemoveUser(runtime, ++txId, "/MyRoot", "user" + std::to_string(userId)); } + + // removing users with access (failing with a error): + for (auto userId : xrange(Min(usersWithAccess, 3))) + { + TLogStopwatch stopwatch(TStringBuilder() << "Don't removed user" + std::to_string(userId)); + CreateAlterLoginRemoveUser(runtime, ++txId, "/MyRoot", "user" + std::to_string(userId), + {{NKikimrScheme::StatusPreconditionFailed}}); + } } } diff --git a/ydb/library/aclib/aclib.cpp b/ydb/library/aclib/aclib.cpp index 0a05d7c0f493..a63a2bf480d0 100644 --- a/ydb/library/aclib/aclib.cpp +++ b/ydb/library/aclib/aclib.cpp @@ -379,22 +379,6 @@ std::pair TACL::RemoveAccess(const NACLibProto::TACE& filter) { return modified; } -bool TACL::TryRemoveAccess(const NACLib::TSID& sid) { - auto* ACL = MutableACE(); - - auto newEnd = std::remove_if(ACL->begin(), ACL->end(), [&sid](const NACLibProto::TACE& ace) { - return ace.GetSID() == sid; - }); - - if (newEnd == ACL->end()) { - return false; - } - - ACL->erase(newEnd, ACL->end()); - - return true; -} - bool TACL::HasAccess(const NACLib::TSID& sid) { for (const auto& ace : GetACE()) { if (ace.GetSID() == sid) { diff --git a/ydb/library/aclib/aclib.h b/ydb/library/aclib/aclib.h index 13d2f0d42c6f..41437222f315 100644 --- a/ydb/library/aclib/aclib.h +++ b/ydb/library/aclib/aclib.h @@ -117,7 +117,6 @@ class TACL : public NACLibProto::TACL { std::pair AddAccess(EAccessType type, ui32 access, const TSID& sid, ui32 inheritance = InheritObject | InheritContainer); std::pair RemoveAccess(NACLib::EAccessType type, ui32 access, const NACLib::TSID& sid, ui32 inheritance = InheritObject | InheritContainer); std::pair RemoveAccess(const NACLibProto::TACE& filter); - bool TryRemoveAccess(const NACLib::TSID& sid); bool HasAccess(const NACLib::TSID& sid); std::pair ClearAccess(); std::pair ApplyDiff(const NACLibProto::TDiffACL& diffACL); diff --git a/ydb/library/aclib/benchmark/b_aclib.cpp b/ydb/library/aclib/benchmark/b_aclib.cpp new file mode 100644 index 000000000000..ee57e61c62a5 --- /dev/null +++ b/ydb/library/aclib/benchmark/b_aclib.cpp @@ -0,0 +1,47 @@ +#include +#include +#include + +namespace NACLib { + +namespace { + struct TFixture : public benchmark::Fixture { + void SetUp(::benchmark::State& state) + { + const size_t count = state.range(0); + + for (auto userId : xrange(count)) { + Obj.AddAccess(EAccessType::Allow, EAccessRights::SelectRow, "user" + std::to_string(userId), EInheritanceType::InheritContainer); + } + + Serialized = Obj.SerializeAsString(); + } + + TString Serialized; + TACL Obj; + }; +} + +BENCHMARK_DEFINE_F(TFixture, Deserialize)(benchmark::State& state) { + for (auto _ : state) { + benchmark::DoNotOptimize( NACLib::TACL(Serialized)); + } +} + +BENCHMARK_DEFINE_F(TFixture, HasAccess)(benchmark::State& state) { + for (auto _ : state) { + benchmark::DoNotOptimize(Obj.HasAccess("userxxx")); + } +} + +BENCHMARK_REGISTER_F(TFixture, Deserialize) + ->ArgsProduct({ + {20, 200, 2000}}) + ->Unit(benchmark::kMicrosecond); + +BENCHMARK_REGISTER_F(TFixture, HasAccess) + ->ArgsProduct({ + {20, 200, 2000}}) + ->Unit(benchmark::kMicrosecond); + +} diff --git a/ydb/library/aclib/benchmark/ya.make b/ydb/library/aclib/benchmark/ya.make new file mode 100644 index 000000000000..4b14ad1f3c31 --- /dev/null +++ b/ydb/library/aclib/benchmark/ya.make @@ -0,0 +1,14 @@ +G_BENCHMARK() + +TAG(ya:fat) +SIZE(LARGE) + +SRCS( + b_aclib.cpp +) + +PEERDIR( + ydb/library/aclib +) + +END() diff --git a/ydb/library/aclib/ya.make b/ydb/library/aclib/ya.make index 1e5aa26ea42a..ce03bd72bd0e 100644 --- a/ydb/library/aclib/ya.make +++ b/ydb/library/aclib/ya.make @@ -15,4 +15,5 @@ END() RECURSE_FOR_TESTS( ut + benchmark )