Skip to content

Commit d543026

Browse files
kungasckungasc
authored
Check SID existence in ACL operation (#12719)
1 parent fc48c46 commit d543026

File tree

7 files changed

+175
-3
lines changed

7 files changed

+175
-3
lines changed

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

Lines changed: 52 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3401,6 +3401,23 @@ Y_UNIT_TEST_SUITE(KqpScheme) {
34013401
TKikimrRunner kikimr;
34023402
auto db = kikimr.GetTableClient();
34033403
auto session = db.CreateSession().GetValueSync().GetSession();
3404+
{
3405+
auto query = TStringBuilder() << R"(
3406+
--!syntax_v1
3407+
CREATE USER user1 PASSWORD 'password1';
3408+
CREATE USER user2 PASSWORD 'password2';
3409+
CREATE USER user3 PASSWORD 'password3';
3410+
CREATE USER user4 PASSWORD 'password4';
3411+
CREATE USER user5 PASSWORD 'password5';
3412+
CREATE USER user6 PASSWORD 'password6';
3413+
CREATE USER user7 PASSWORD 'password7';
3414+
CREATE USER user8 PASSWORD 'password8';
3415+
CREATE USER user9 PASSWORD 'password9';
3416+
)";
3417+
auto session = db.CreateSession().GetValueSync().GetSession();
3418+
auto result = session.ExecuteSchemeQuery(query).GetValueSync();
3419+
UNIT_ASSERT_VALUES_EQUAL_C(result.GetStatus(), EStatus::SUCCESS, result.GetIssues().ToString());
3420+
}
34043421
{
34053422
auto query = TStringBuilder() << R"(
34063423
--!syntax_v1
@@ -3794,6 +3811,14 @@ Y_UNIT_TEST_SUITE(KqpScheme) {
37943811
TKikimrRunner kikimr;
37953812
auto db = kikimr.GetTableClient();
37963813
auto session = db.CreateSession().GetValueSync().GetSession();
3814+
{
3815+
auto query = TStringBuilder() << R"(
3816+
--!syntax_v1
3817+
CREATE USER user1 PASSWORD 'password1';
3818+
)";
3819+
auto result = session.ExecuteSchemeQuery(query).GetValueSync();
3820+
UNIT_ASSERT_VALUES_EQUAL_C(result.GetStatus(), EStatus::SUCCESS, result.GetIssues().ToString());
3821+
}
37973822
{
37983823
auto query = TStringBuilder() << R"(
37993824
--!syntax_v1
@@ -3843,7 +3868,15 @@ Y_UNIT_TEST_SUITE(KqpScheme) {
38433868
TKikimrRunner kikimr;
38443869
auto db = kikimr.GetTableClient();
38453870
auto session = db.CreateSession().GetValueSync().GetSession();
3846-
3871+
3872+
{
3873+
auto query = TStringBuilder() << R"(
3874+
--!syntax_v1
3875+
CREATE USER ydbuser PASSWORD 'password1';
3876+
)";
3877+
auto result = session.ExecuteSchemeQuery(query).GetValueSync();
3878+
UNIT_ASSERT_VALUES_EQUAL_C(result.GetStatus(), EStatus::SUCCESS, result.GetIssues().ToString());
3879+
}
38473880
{
38483881
{
38493882
const TString query = R"(
@@ -3883,6 +3916,7 @@ Y_UNIT_TEST_SUITE(KqpScheme) {
38833916
auto runnerSettings = TKikimrSettings().SetAppConfig(appConfig);
38843917
TTestHelper testHelper(runnerSettings);
38853918
auto client = testHelper.GetKikimr().GetQueryClient();
3919+
auto db = testHelper.GetKikimr().GetTableClient();
38863920

38873921
TVector<TTestHelper::TColumnSchema> schema = {
38883922
TTestHelper::TColumnSchema().SetName("id").SetType(NScheme::NTypeIds::Int32).SetNullable(false),
@@ -3893,6 +3927,15 @@ Y_UNIT_TEST_SUITE(KqpScheme) {
38933927
testTable.SetName("/Root/MyApp/Orders").SetPrimaryKey({ "id" }).SetSchema(schema);
38943928
testHelper.CreateTable(testTable);
38953929

3930+
{
3931+
auto query = TStringBuilder() << R"(
3932+
--!syntax_v1
3933+
CREATE USER ydbuser PASSWORD 'password1';
3934+
)";
3935+
auto session = db.CreateSession().GetValueSync().GetSession();
3936+
auto result = session.ExecuteSchemeQuery(query).GetValueSync();
3937+
UNIT_ASSERT_VALUES_EQUAL_C(result.GetStatus(), EStatus::SUCCESS, result.GetIssues().ToString());
3938+
}
38963939
{
38973940
{
38983941
const TString query = R"(
@@ -3918,6 +3961,14 @@ Y_UNIT_TEST_SUITE(KqpScheme) {
39183961
TKikimrRunner kikimr;
39193962
auto db = kikimr.GetTableClient();
39203963
auto session = db.CreateSession().GetValueSync().GetSession();
3964+
{
3965+
auto query = TStringBuilder() << R"(
3966+
--!syntax_v1
3967+
CREATE USER user1 PASSWORD 'password1';
3968+
)";
3969+
auto result = session.ExecuteSchemeQuery(query).GetValueSync();
3970+
UNIT_ASSERT_VALUES_EQUAL_C(result.GetStatus(), EStatus::SUCCESS, result.GetIssues().ToString());
3971+
}
39213972
{
39223973
auto query = TStringBuilder() << R"(
39233974
--!syntax_v1

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

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1826,6 +1826,23 @@ Y_UNIT_TEST_SUITE(KqpQueryService) {
18261826
}
18271827
};
18281828

1829+
{
1830+
auto query = TStringBuilder() << R"(
1831+
--!syntax_v1
1832+
CREATE USER user1 PASSWORD 'password1';
1833+
CREATE USER user2 PASSWORD 'password2';
1834+
CREATE USER user3 PASSWORD 'password3';
1835+
CREATE USER user4 PASSWORD 'password4';
1836+
CREATE USER user5 PASSWORD 'password5';
1837+
CREATE USER user6 PASSWORD 'password6';
1838+
CREATE USER user7 PASSWORD 'password7';
1839+
CREATE USER user8 PASSWORD 'password8';
1840+
CREATE USER user9 PASSWORD 'password9';
1841+
)";
1842+
auto result = session.ExecuteSchemeQuery(query).GetValueSync();
1843+
UNIT_ASSERT_VALUES_EQUAL_C(result.GetStatus(), EStatus::SUCCESS, result.GetIssues().ToString());
1844+
}
1845+
18291846
auto result = db.ExecuteQuery(R"(
18301847
GRANT ROW SELECT ON `/Root` TO user1;
18311848
)", TTxControl::NoTx()).ExtractValueSync();

‎ydb/core/tx/schemeshard/schemeshard__operation_modify_acl.cpp‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,11 @@ namespace {
66
using namespace NKikimr;
77
using namespace NSchemeShard;
88

9+
bool CheckSidExistsOrIsNonYdb(const std::unordered_map<TString, NLogin::TLoginProvider::TSidRecord>& sids, const TString& sid) {
10+
// non-YDB user's sid format is <login>@<subsystem>
11+
return sid.Contains('@') || sids.contains(sid);
12+
}
13+
914
class TModifyACL: public TSubOperationBase {
1015
public:
1116
using TSubOperationBase::TSubOperationBase;
@@ -51,6 +56,26 @@ class TModifyACL: public TSubOperationBase {
5156
return result;
5257
}
5358

59+
if (acl) {
60+
NACLib::TDiffACL diffACL(acl);
61+
for (const NACLibProto::TDiffACE& diffACE : diffACL.GetDiffACE()) {
62+
if (static_cast<NACLib::EDiffType>(diffACE.GetDiffType()) == NACLib::EDiffType::Add) {
63+
if (!CheckSidExistsOrIsNonYdb(context.SS->LoginProvider.Sids, diffACE.GetACE().GetSID())) {
64+
result->SetError(NKikimrScheme::StatusPreconditionFailed,
65+
TStringBuilder() << "SID " << diffACE.GetACE().GetSID() << " not found");
66+
return result;
67+
}
68+
} // remove diff type is allowed in any case
69+
}
70+
}
71+
if (owner) {
72+
if (!CheckSidExistsOrIsNonYdb(context.SS->LoginProvider.Sids, owner)) {
73+
result->SetError(NKikimrScheme::StatusPreconditionFailed,
74+
TStringBuilder() << "Owner SID " << owner << " not found");
75+
return result;
76+
}
77+
}
78+
5479
THashSet<TPathId> subTree;
5580
if (acl || (owner && path.Base()->IsTable())) {
5681
subTree = context.SS->ListSubTree(path.Base()->PathId, context.Ctx);

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

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,7 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {
112112
TTestEnv env(runtime);
113113
ui64 txId = 100;
114114
CreateAlterLoginCreateUser(runtime, ++txId, "/MyRoot", "user1", "password1");
115+
CreateAlterLoginCreateUser(runtime, ++txId, "/MyRoot", "user2", "password2");
115116
auto resultLogin = Login(runtime, "user1", "password1");
116117
UNIT_ASSERT_VALUES_EQUAL(resultLogin.error(), "");
117118

@@ -171,6 +172,7 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {
171172
TTestEnv env(runtime);
172173
ui64 txId = 100;
173174
CreateAlterLoginCreateUser(runtime, ++txId, "/MyRoot", "user1", "password1");
175+
CreateAlterLoginCreateUser(runtime, ++txId, "/MyRoot", "user2", "password2");
174176
auto resultLogin = Login(runtime, "user1", "password1");
175177
UNIT_ASSERT_VALUES_EQUAL(resultLogin.error(), "");
176178

@@ -249,6 +251,7 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {
249251
TTestEnv env(runtime);
250252
ui64 txId = 100;
251253
CreateAlterLoginCreateUser(runtime, ++txId, "/MyRoot", "user1", "password1");
254+
CreateAlterLoginCreateUser(runtime, ++txId, "/MyRoot", "user2", "password2");
252255
auto resultLogin = Login(runtime, "user1", "password1");
253256
UNIT_ASSERT_VALUES_EQUAL(resultLogin.error(), "");
254257

@@ -308,6 +311,53 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {
308311
auto describe = DescribePath(runtime, TTestTxConfig::SchemeShard, "/MyRoot");
309312
CheckSecurityState(describe, {.PublicKeysSize = 1, .SidsSize = 0});
310313
}
314+
315+
Y_UNIT_TEST(AddAccess_NonExisting) {
316+
TTestBasicRuntime runtime;
317+
TTestEnv env(runtime);
318+
ui64 txId = 100;
319+
320+
AsyncMkDir(runtime, ++txId, "/MyRoot", "Dir1");
321+
TestModificationResult(runtime, txId, NKikimrScheme::StatusAccepted);
322+
323+
{
324+
NACLib::TDiffACL diffACL;
325+
diffACL.AddAccess(NACLib::EAccessType::Allow, NACLib::GenericUse, "user1");
326+
AsyncModifyACL(runtime, ++txId, "/MyRoot", "Dir1", diffACL.SerializeAsString(), "");
327+
TestModificationResults(runtime, txId, {{NKikimrScheme::StatusPreconditionFailed, "SID user1 not found"}});
328+
}
329+
330+
{
331+
AsyncModifyACL(runtime, ++txId, "/MyRoot", "Dir1", NACLib::TDiffACL{}.SerializeAsString(), "user1");
332+
TestModificationResults(runtime, txId, {{NKikimrScheme::StatusPreconditionFailed, "Owner SID user1 not found"}});
333+
}
334+
335+
CreateAlterLoginCreateUser(runtime, ++txId, "/MyRoot", "user1", "password1");
336+
337+
TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1"),
338+
{NLs::HasNoRight("+U:user1"), NLs::HasNoEffectiveRight("+U:user1"), NLs::HasOwner("root@builtin")});
339+
}
340+
341+
Y_UNIT_TEST(AddAccess_NonYdb) {
342+
TTestBasicRuntime runtime;
343+
TTestEnv env(runtime);
344+
ui64 txId = 100;
345+
346+
AsyncMkDir(runtime, ++txId, "/MyRoot", "Dir1");
347+
TestModificationResult(runtime, txId, NKikimrScheme::StatusAccepted);
348+
349+
{
350+
NACLib::TDiffACL diffACL;
351+
diffACL.AddAccess(NACLib::EAccessType::Allow, NACLib::GenericUse, "user1@staff");
352+
AsyncModifyACL(runtime, ++txId, "/MyRoot", "Dir1", diffACL.SerializeAsString(), "");
353+
TestModificationResult(runtime, txId, NKikimrScheme::StatusSuccess);
354+
}
355+
356+
{
357+
AsyncModifyACL(runtime, ++txId, "/MyRoot", "Dir1", NACLib::TDiffACL{}.SerializeAsString(), "user1@staff");
358+
TestModificationResult(runtime, txId, NKikimrScheme::StatusSuccess);
359+
}
360+
}
311361

312362
Y_UNIT_TEST(DisableBuiltinAuthMechanism) {
313363
TTestBasicRuntime runtime;

‎ydb/core/viewer/viewer_ut.cpp‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1599,7 +1599,7 @@ Y_UNIT_TEST_SUITE(Viewer) {
15991599
void Success(TEvTicketParser::TEvAuthorizeTicket::TPtr& ev) {
16001600
++AuthorizeTicketSuccesses;
16011601
NACLib::TUserToken::TUserTokenInitFields args;
1602-
args.UserSID = "user_name";
1602+
args.UserSID = "username";
16031603
args.GroupSIDs.push_back("group_name");
16041604
TIntrusivePtr<NACLib::TUserToken> userToken = MakeIntrusive<NACLib::TUserToken>(args);
16051605
LOG_INFO_S(*TlsActivationContext, NKikimrServices::TICKET_PARSER, "Send TEvAuthorizeTicketResult success");
@@ -1616,7 +1616,8 @@ Y_UNIT_TEST_SUITE(Viewer) {
16161616
}
16171617

16181618
void GrantConnect(TClient& client) {
1619-
client.GrantConnect("user_name");
1619+
client.CreateUser("/Root", "username", "password");
1620+
client.GrantConnect("username");
16201621

16211622
const auto alterAttrsStatus = client.AlterUserAttributes("/", "Root", {
16221623
{ "folder_id", "test_folder_id" },

‎ydb/services/ydb/ydb_ut.cpp‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -421,6 +421,10 @@ Y_UNIT_TEST_SUITE(TGRpcClientLowTest) {
421421
TString location = TStringBuilder() << "localhost:" << grpc;
422422
auto clientConfig = NGRpcProxy::TGRpcClientConfig(location);
423423

424+
{
425+
TClient client(*server.ServerSettings);
426+
client.CreateUser("/Root", "qqq", "password");
427+
}
424428
{
425429
NYdbGrpc::TGRpcClientLow clientLow;
426430
auto connection = clientLow.CreateGRpcServiceConnection<Ydb::Scheme::V1::SchemeService>(clientConfig);

‎ydb/tests/functional/ydb_cli/test_ydb_backup.py‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -272,6 +272,23 @@ def scheme_listdir(self, path):
272272
if not is_system_object(child)
273273
]
274274

275+
def create_user(self, user, password="password"):
276+
yatest.common.execute(
277+
[
278+
backup_bin(),
279+
"--verbose",
280+
"--endpoint", "grpc://localhost:%d" % self.cluster.nodes[1].grpc_port,
281+
"--database", "/Root",
282+
"yql",
283+
"--script", f"CREATE USER {user} PASSWORD '{password}'",
284+
]
285+
)
286+
287+
def create_users(self):
288+
self.create_user("alice")
289+
self.create_user("bob")
290+
self.create_user("eve")
291+
275292

276293
class TestBackupSingle(BaseTestBackupInFiles):
277294
def test_single_table_backup(self):
@@ -825,6 +842,7 @@ def test_single_table(self):
825842
session = self.driver.table_client.session().create()
826843

827844
# Create table and modify permissions on it
845+
self.create_users()
828846
create_table_with_data(session, "folder/table")
829847
modify_permissions(self.driver.scheme_client, "folder/table")
830848

@@ -879,6 +897,7 @@ def test_single_table(self):
879897
class TestPermissionsBackupRestoreFolderWithTable(BaseTestBackupInFiles):
880898
def test_folder_with_table(self):
881899
# Create folder and modify permissions on it
900+
self.create_users()
882901
self.driver.scheme_client.make_directory("/Root/folder")
883902
modify_permissions(self.driver.scheme_client, "folder")
884903

@@ -935,6 +954,7 @@ def test_folder_with_table(self):
935954
class TestPermissionsBackupRestoreDontOverwriteOnAlreadyExisting(BaseTestBackupInFiles):
936955
def test_dont_overwrite_on_already_existing(self):
937956
# Create folder and modify permissions on it
957+
self.create_users()
938958
self.driver.scheme_client.make_directory("/Root/folder")
939959
modify_permissions(self.driver.scheme_client, "folder")
940960

@@ -1034,6 +1054,7 @@ def test_dont_overwrite_on_already_existing(self):
10341054
class TestPermissionsBackupRestoreSchemeOnly(BaseTestBackupInFiles):
10351055
def test_scheme_only(self):
10361056
# Create folder and modify permissions on it
1057+
self.create_users()
10371058
self.driver.scheme_client.make_directory("/Root/folder")
10381059
modify_permissions(self.driver.scheme_client, "folder")
10391060

@@ -1091,6 +1112,7 @@ def test_scheme_only(self):
10911112
class TestPermissionsBackupRestoreEmptyDir(BaseTestBackupInFiles):
10921113
def test_empty_dir(self):
10931114
# Create empty folder and modify permissions on it
1115+
self.create_users()
10941116
self.driver.scheme_client.make_directory("/Root/folder")
10951117
modify_permissions(self.driver.scheme_client, "folder")
10961118

@@ -1141,6 +1163,7 @@ def test_restore_acl_option(self):
11411163
session = self.driver.table_client.session().create()
11421164

11431165
# Create table and modify permissions on it
1166+
self.create_users()
11441167
create_table_with_data(session, "folder/table")
11451168
modify_permissions(self.driver.scheme_client, "folder/table")
11461169

@@ -1208,6 +1231,7 @@ def test_restore_no_data(self):
12081231
session = self.driver.table_client.session().create()
12091232

12101233
# Create table and modify permissions on it
1234+
self.create_users()
12111235
create_table_with_data(session, "folder/table")
12121236
modify_permissions(self.driver.scheme_client, "folder/table")
12131237

0 commit comments

Comments
 (0)