Check SID existence in ACL operation

ydb/core/kqp/ut/scheme/kqp_scheme_ut.cpp

@@ -3401,6 +3401,23 @@ Y_UNIT_TEST_SUITE(KqpScheme) {
         TKikimrRunner kikimr;
         auto db = kikimr.GetTableClient();
         auto session = db.CreateSession().GetValueSync().GetSession();
+        {
+            auto query = TStringBuilder() << R"(
+            --!syntax_v1
+            CREATE USER user1 PASSWORD 'password1';
+            CREATE USER user2 PASSWORD 'password2';
+            CREATE USER user3 PASSWORD 'password3';
+            CREATE USER user4 PASSWORD 'password4';
+            CREATE USER user5 PASSWORD 'password5';
+            CREATE USER user6 PASSWORD 'password6';
+            CREATE USER user7 PASSWORD 'password7';
+            CREATE USER user8 PASSWORD 'password8';
+            CREATE USER user9 PASSWORD 'password9';
+            )";
+            auto session = db.CreateSession().GetValueSync().GetSession();
+            auto result = session.ExecuteSchemeQuery(query).GetValueSync();
+            UNIT_ASSERT_VALUES_EQUAL_C(result.GetStatus(), EStatus::SUCCESS, result.GetIssues().ToString());
+        }
         {
             auto query = TStringBuilder() << R"(
             --!syntax_v1
@@ -3794,6 +3811,14 @@ Y_UNIT_TEST_SUITE(KqpScheme) {
         TKikimrRunner kikimr;
         auto db = kikimr.GetTableClient();
         auto session = db.CreateSession().GetValueSync().GetSession();
+        {
+            auto query = TStringBuilder() << R"(
+            --!syntax_v1
+            CREATE USER user1 PASSWORD 'password1';
+            )";
+            auto result = session.ExecuteSchemeQuery(query).GetValueSync();
+            UNIT_ASSERT_VALUES_EQUAL_C(result.GetStatus(), EStatus::SUCCESS, result.GetIssues().ToString());
+        }
         {
             auto query = TStringBuilder() << R"(
             --!syntax_v1
@@ -3843,7 +3868,15 @@ Y_UNIT_TEST_SUITE(KqpScheme) {
         TKikimrRunner kikimr;
         auto db = kikimr.GetTableClient();
         auto session = db.CreateSession().GetValueSync().GetSession();
-
+
+        {
+            auto query = TStringBuilder() << R"(
+            --!syntax_v1
+            CREATE USER ydbuser PASSWORD 'password1';
+            )";
+            auto result = session.ExecuteSchemeQuery(query).GetValueSync();
+            UNIT_ASSERT_VALUES_EQUAL_C(result.GetStatus(), EStatus::SUCCESS, result.GetIssues().ToString());
+        }
         {
             {
                 const TString query = R"(
@@ -3883,6 +3916,7 @@ Y_UNIT_TEST_SUITE(KqpScheme) {
         auto runnerSettings = TKikimrSettings().SetAppConfig(appConfig);
         TTestHelper testHelper(runnerSettings);
         auto client = testHelper.GetKikimr().GetQueryClient();
+        auto db = testHelper.GetKikimr().GetTableClient();
 
         TVector<TTestHelper::TColumnSchema> schema = {
             TTestHelper::TColumnSchema().SetName("id").SetType(NScheme::NTypeIds::Int32).SetNullable(false),
@@ -3893,6 +3927,15 @@ Y_UNIT_TEST_SUITE(KqpScheme) {
         testTable.SetName("/Root/MyApp/Orders").SetPrimaryKey({ "id" }).SetSchema(schema);
         testHelper.CreateTable(testTable);
 
+        {
+            auto query = TStringBuilder() << R"(
+            --!syntax_v1
+            CREATE USER ydbuser PASSWORD 'password1';
+            )";
+            auto session = db.CreateSession().GetValueSync().GetSession();
+            auto result = session.ExecuteSchemeQuery(query).GetValueSync();
+            UNIT_ASSERT_VALUES_EQUAL_C(result.GetStatus(), EStatus::SUCCESS, result.GetIssues().ToString());
+        }
         {
             {
                 const TString query = R"(
@@ -3918,6 +3961,14 @@ Y_UNIT_TEST_SUITE(KqpScheme) {
         TKikimrRunner kikimr;
         auto db = kikimr.GetTableClient();
         auto session = db.CreateSession().GetValueSync().GetSession();
+        {
+            auto query = TStringBuilder() << R"(
+            --!syntax_v1
+            CREATE USER user1 PASSWORD 'password1';
+            )";
+            auto result = session.ExecuteSchemeQuery(query).GetValueSync();
+            UNIT_ASSERT_VALUES_EQUAL_C(result.GetStatus(), EStatus::SUCCESS, result.GetIssues().ToString());
+        }
         {
             auto query = TStringBuilder() << R"(
             --!syntax_v1

ydb/core/kqp/ut/service/kqp_qs_queries_ut.cpp

@@ -1826,6 +1826,23 @@ Y_UNIT_TEST_SUITE(KqpQueryService) {
             }
         };
 
+        {
+            auto query = TStringBuilder() << R"(
+            --!syntax_v1
+            CREATE USER user1 PASSWORD 'password1';
+            CREATE USER user2 PASSWORD 'password2';
+            CREATE USER user3 PASSWORD 'password3';
+            CREATE USER user4 PASSWORD 'password4';
+            CREATE USER user5 PASSWORD 'password5';
+            CREATE USER user6 PASSWORD 'password6';
+            CREATE USER user7 PASSWORD 'password7';
+            CREATE USER user8 PASSWORD 'password8';
+            CREATE USER user9 PASSWORD 'password9';
+            )";
+            auto result = session.ExecuteSchemeQuery(query).GetValueSync();
+            UNIT_ASSERT_VALUES_EQUAL_C(result.GetStatus(), EStatus::SUCCESS, result.GetIssues().ToString());
+        }
+
         auto result = db.ExecuteQuery(R"(
             GRANT ROW SELECT ON `/Root` TO user1;
         )", TTxControl::NoTx()).ExtractValueSync();

ydb/core/tx/schemeshard/schemeshard__operation_modify_acl.cpp

@@ -6,6 +6,11 @@ namespace {
 using namespace NKikimr;
 using namespace NSchemeShard;
 
+bool CheckSidExistsOrIsNonYdb(const std::unordered_map<TString, NLogin::TLoginProvider::TSidRecord>& sids, const TString& sid) {
+    // non-YDB user's sid format is <login>@<subsystem>
+    return sid.Contains('@') || sids.contains(sid);
+}
+
 class TModifyACL: public TSubOperationBase {
 public:
     using TSubOperationBase::TSubOperationBase;
@@ -51,6 +56,26 @@ class TModifyACL: public TSubOperationBase {
             return result;
         }
 
+        if (acl) {
+            NACLib::TDiffACL diffACL(acl);
+            for (const NACLibProto::TDiffACE& diffACE : diffACL.GetDiffACE()) {
+                if (static_cast<NACLib::EDiffType>(diffACE.GetDiffType()) == NACLib::EDiffType::Add) {
+                    if (!CheckSidExistsOrIsNonYdb(context.SS->LoginProvider.Sids, diffACE.GetACE().GetSID())) {
+                        result->SetError(NKikimrScheme::StatusPreconditionFailed,
+                            TStringBuilder() << "SID " << diffACE.GetACE().GetSID() << " not found");
+                        return result;
+                    }
+                } // remove diff type is allowed in any case
+            }
+        }
+        if (owner) {
+            if (!CheckSidExistsOrIsNonYdb(context.SS->LoginProvider.Sids, owner)) {
+                result->SetError(NKikimrScheme::StatusPreconditionFailed,
+                    TStringBuilder() << "Owner SID " << owner << " not found");
+                return result;
+            }
+        }
+
         THashSet<TPathId> subTree;
         if (acl || (owner && path.Base()->IsTable())) {
             subTree = context.SS->ListSubTree(path.Base()->PathId, context.Ctx);

ydb/core/tx/schemeshard/ut_login/ut_login.cpp

@@ -112,6 +112,7 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {
         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(), "");
 
@@ -171,6 +172,7 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {
         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(), "");
 
@@ -249,6 +251,7 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {
         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(), "");
 
@@ -308,6 +311,53 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {
         auto describe = DescribePath(runtime, TTestTxConfig::SchemeShard, "/MyRoot");
         CheckSecurityState(describe, {.PublicKeysSize = 1, .SidsSize = 0});
     }
+
+    Y_UNIT_TEST(AddAccess_NonExisting) {
+        TTestBasicRuntime runtime;
+        TTestEnv env(runtime);
+        ui64 txId = 100;
+
+        AsyncMkDir(runtime, ++txId, "/MyRoot", "Dir1");
+        TestModificationResult(runtime, txId, NKikimrScheme::StatusAccepted);
+
+        {
+            NACLib::TDiffACL diffACL;
+            diffACL.AddAccess(NACLib::EAccessType::Allow, NACLib::GenericUse, "user1");
+            AsyncModifyACL(runtime, ++txId, "/MyRoot", "Dir1", diffACL.SerializeAsString(), "");
+            TestModificationResults(runtime, txId, {{NKikimrScheme::StatusPreconditionFailed, "SID user1 not found"}});
+        }
+
+        {
+            AsyncModifyACL(runtime, ++txId, "/MyRoot", "Dir1", NACLib::TDiffACL{}.SerializeAsString(), "user1");
+            TestModificationResults(runtime, txId, {{NKikimrScheme::StatusPreconditionFailed, "Owner SID user1 not found"}});
+        }
+
+        CreateAlterLoginCreateUser(runtime, ++txId, "/MyRoot", "user1", "password1");
+
+        TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1"),
+            {NLs::HasNoRight("+U:user1"), NLs::HasNoEffectiveRight("+U:user1"), NLs::HasOwner("root@builtin")});
+    }
+
+    Y_UNIT_TEST(AddAccess_NonYdb) {
+        TTestBasicRuntime runtime;
+        TTestEnv env(runtime);
+        ui64 txId = 100;
+
+        AsyncMkDir(runtime, ++txId, "/MyRoot", "Dir1");
+        TestModificationResult(runtime, txId, NKikimrScheme::StatusAccepted);
+
+        {
+            NACLib::TDiffACL diffACL;
+            diffACL.AddAccess(NACLib::EAccessType::Allow, NACLib::GenericUse, "user1@staff");
+            AsyncModifyACL(runtime, ++txId, "/MyRoot", "Dir1", diffACL.SerializeAsString(), "");
+            TestModificationResult(runtime, txId, NKikimrScheme::StatusSuccess);
+        }
+
+        {
+            AsyncModifyACL(runtime, ++txId, "/MyRoot", "Dir1", NACLib::TDiffACL{}.SerializeAsString(), "user1@staff");
+            TestModificationResult(runtime, txId, NKikimrScheme::StatusSuccess);
+        }
+    }
 
     Y_UNIT_TEST(DisableBuiltinAuthMechanism) {
         TTestBasicRuntime runtime;

ydb/core/viewer/viewer_ut.cpp

@@ -1599,7 +1599,7 @@ Y_UNIT_TEST_SUITE(Viewer) {
         void Success(TEvTicketParser::TEvAuthorizeTicket::TPtr& ev) {
             ++AuthorizeTicketSuccesses;
             NACLib::TUserToken::TUserTokenInitFields args;
-            args.UserSID = "user_name";
+            args.UserSID = "username";
             args.GroupSIDs.push_back("group_name");
             TIntrusivePtr<NACLib::TUserToken> userToken = MakeIntrusive<NACLib::TUserToken>(args);
             LOG_INFO_S(*TlsActivationContext, NKikimrServices::TICKET_PARSER, "Send TEvAuthorizeTicketResult success");
@@ -1616,7 +1616,8 @@ Y_UNIT_TEST_SUITE(Viewer) {
     }
 
     void GrantConnect(TClient& client) {
-        client.GrantConnect("user_name");
+        client.CreateUser("/Root", "username", "password");
+        client.GrantConnect("username");
 
         const auto alterAttrsStatus = client.AlterUserAttributes("/", "Root", {
             { "folder_id", "test_folder_id" },

ydb/services/ydb/ydb_ut.cpp

@@ -421,6 +421,10 @@ Y_UNIT_TEST_SUITE(TGRpcClientLowTest) {
         TString location = TStringBuilder() << "localhost:" << grpc;
         auto clientConfig = NGRpcProxy::TGRpcClientConfig(location);
 
+        {
+            TClient client(*server.ServerSettings);
+            client.CreateUser("/Root", "qqq", "password");
+        }
         {
             NYdbGrpc::TGRpcClientLow clientLow;
             auto connection = clientLow.CreateGRpcServiceConnection<Ydb::Scheme::V1::SchemeService>(clientConfig);

ydb/tests/functional/ydb_cli/test_ydb_backup.py

@@ -272,6 +272,23 @@ def scheme_listdir(self, path):
             if not is_system_object(child)
         ]
 
+    def create_user(self, user, password="password"):
+        yatest.common.execute(
+            [
+                backup_bin(),
+                "--verbose",
+                "--endpoint", "grpc://localhost:%d" % self.cluster.nodes[1].grpc_port,
+                "--database", "/Root",
+                "yql",
+                "--script", f"CREATE USER {user} PASSWORD '{password}'",
+            ]
+        )
+
+    def create_users(self):
+        self.create_user("alice")
+        self.create_user("bob")
+        self.create_user("eve")
+
 
 class TestBackupSingle(BaseTestBackupInFiles):
     def test_single_table_backup(self):
@@ -825,6 +842,7 @@ def test_single_table(self):
         session = self.driver.table_client.session().create()
 
         # Create table and modify permissions on it
+        self.create_users()
         create_table_with_data(session, "folder/table")
         modify_permissions(self.driver.scheme_client, "folder/table")
 
@@ -879,6 +897,7 @@ def test_single_table(self):
 class TestPermissionsBackupRestoreFolderWithTable(BaseTestBackupInFiles):
     def test_folder_with_table(self):
         # Create folder and modify permissions on it
+        self.create_users()
         self.driver.scheme_client.make_directory("/Root/folder")
         modify_permissions(self.driver.scheme_client, "folder")
 
@@ -935,6 +954,7 @@ def test_folder_with_table(self):
 class TestPermissionsBackupRestoreDontOverwriteOnAlreadyExisting(BaseTestBackupInFiles):
     def test_dont_overwrite_on_already_existing(self):
         # Create folder and modify permissions on it
+        self.create_users()
         self.driver.scheme_client.make_directory("/Root/folder")
         modify_permissions(self.driver.scheme_client, "folder")
 
@@ -1034,6 +1054,7 @@ def test_dont_overwrite_on_already_existing(self):
 class TestPermissionsBackupRestoreSchemeOnly(BaseTestBackupInFiles):
     def test_scheme_only(self):
         # Create folder and modify permissions on it
+        self.create_users()
         self.driver.scheme_client.make_directory("/Root/folder")
         modify_permissions(self.driver.scheme_client, "folder")
 
@@ -1091,6 +1112,7 @@ def test_scheme_only(self):
 class TestPermissionsBackupRestoreEmptyDir(BaseTestBackupInFiles):
     def test_empty_dir(self):
         # Create empty folder and modify permissions on it
+        self.create_users()
         self.driver.scheme_client.make_directory("/Root/folder")
         modify_permissions(self.driver.scheme_client, "folder")
 
@@ -1141,6 +1163,7 @@ def test_restore_acl_option(self):
         session = self.driver.table_client.session().create()
 
         # Create table and modify permissions on it
+        self.create_users()
         create_table_with_data(session, "folder/table")
         modify_permissions(self.driver.scheme_client, "folder/table")
 
@@ -1208,6 +1231,7 @@ def test_restore_no_data(self):
         session = self.driver.table_client.session().create()
 
         # Create table and modify permissions on it
+        self.create_users()
         create_table_with_data(session, "folder/table")
         modify_permissions(self.driver.scheme_client, "folder/table")