Merge 14454da into bafd581

ydb-platform · Dec 31, 2024 · 8fdf845 · 8fdf845
2 parents bafd581 + 14454da
commit 8fdf845
Show file tree

Hide file tree

Showing 7 changed files with 147 additions and 156 deletions.
diff --git a/ydb/core/tx/schemeshard/schemeshard__operation_alter_login.cpp b/ydb/core/tx/schemeshard/schemeshard__operation_alter_login.cpp
@@ -203,38 +203,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 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);

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);

diff --git 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,111 +116,13 @@ 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
-        {
-            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_SubTree) {
-        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(), "");
-
-        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", "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")});
-
         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) {
@@ -246,6 +148,33 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {
         }
     }
 
+    Y_UNIT_TEST(RemoveLogin_Groups) {
+        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(), "");
+
+        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"})});
+
+        CreateAlterLoginRemoveUser(runtime, ++txId, "/MyRoot", "user1");
+
+        // check user has been removed:
+        {
+            TestDescribeResult(DescribePath(runtime, "/MyRoot"),
+            {NLs::HasGroup("group", {"user2"})});
+            auto resultLogin = Login(runtime, "user1", "password1");
+            UNIT_ASSERT_VALUES_EQUAL(resultLogin.GetError(), "Cannot find user: user1");
+        }
+    }
+
     Y_UNIT_TEST(RemoveLogin_Owner) {
         TTestBasicRuntime runtime;
         TTestEnv env(runtime);
@@ -257,13 +186,11 @@ 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");
+        AsyncModifyACL(runtime, ++txId, "/MyRoot/Dir1", "DirSub1", NACLib::TDiffACL{}.SerializeAsString(), "user1");
         TestModificationResult(runtime, txId, NKikimrScheme::StatusSuccess);
 
         TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1/DirSub1"),
-            {NLs::HasRight("+U:user1"), NLs::HasEffectiveRight("+U:user1"), NLs::HasOwner("user1")});
+            {NLs::HasOwner("user1")});
 
         // Cerr << DescribePath(runtime, TTestTxConfig::SchemeShard, "/MyRoot/Dir1").DebugString() << Endl;
         // Cerr << DescribePath(runtime, TTestTxConfig::SchemeShard, "/MyRoot/Dir1").DebugString() << Endl;
@@ -274,7 +201,7 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {
         // check user still exists and has their rights:
         {
             TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1/DirSub1"),
-                {NLs::HasRight("+U:user1"), NLs::HasEffectiveRight("+U:user1"), NLs::HasOwner("user1")});
+                {NLs::HasOwner("user1")});
             auto resultLogin = Login(runtime, "user1", "password1");
             UNIT_ASSERT_VALUES_EQUAL(resultLogin.GetError(), "");
         }
@@ -286,7 +213,69 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {
         // check user has been removed:
         {
             TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1/DirSub1"),
-                {NLs::HasNoRight("+U:user1"), NLs::HasNoEffectiveRight("+U:user1"), NLs::HasOwner("user2")});
+                {NLs::HasOwner("user2")});
+            auto resultLogin = Login(runtime, "user1", "password1");
+            UNIT_ASSERT_VALUES_EQUAL(resultLogin.GetError(), "Cannot find user: user1");
+        }
+    }
+
+    Y_UNIT_TEST(RemoveLogin_Acl) {
+        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(), "");
+
+        AsyncMkDir(runtime, ++txId, "/MyRoot", "Dir1/DirSub1");
+
+        {
+            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<TExpectedResult>{{NKikimrScheme::StatusPreconditionFailed, "User user1 has 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::HasNoRight("+U:user1"), NLs::HasEffectiveRight("+U:user1")});
+            auto resultLogin = Login(runtime, "user1", "password1");
+            UNIT_ASSERT_VALUES_EQUAL(resultLogin.GetError(), "");
+        }
+
+        {
+            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")});
             auto resultLogin = Login(runtime, "user1", "password1");
             UNIT_ASSERT_VALUES_EQUAL(resultLogin.GetError(), "Cannot find user: user1");
         }