Skip to content

Commit 3d42b93

Browse files
kungakunga
authored
Forbid to remove group with access (#13930)
1 parent 7102021 commit 3d42b93

File tree

9 files changed

+208
-46
lines changed

9 files changed

+208
-46
lines changed

‎ydb/core/security/ticket_parser_ut.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -258,7 +258,7 @@ Y_UNIT_TEST_SUITE(TTicketParserTest) {
258258
UNIT_ASSERT(result->Token->IsExist("group2"));
259259
UNIT_ASSERT_VALUES_EQUAL(result->Token->GetGroupSIDs().size(), 3);
260260

261-
provider.RemoveGroup({.Group = "group2"});
261+
provider.RemoveGroup("group2");
262262
provider.CreateGroup({.Group = "group3"});
263263
provider.AddGroupMembership({.Group = "group3", .Member = "user1"});
264264
runtime->Send(new IEventHandle(MakeTicketParserID(), sender, new TEvTicketParser::TEvUpdateLoginSecurityState(provider.GetSecurityState())), 0);

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

Lines changed: 50 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -182,18 +182,10 @@ class TAlterLogin: public TSubOperationBase {
182182
}
183183
case NKikimrSchemeOp::TAlterLogin::kRemoveGroup: {
184184
const auto& removeGroup = alterLogin.GetRemoveGroup();
185-
const TString& group = removeGroup.GetGroup();
186-
auto response = context.SS->LoginProvider.RemoveGroup({
187-
.Group = group,
188-
.MissingOk = removeGroup.GetMissingOk()
189-
});
185+
auto response = RemoveGroup(context, removeGroup, db);
190186
if (response.Error) {
191187
result->SetStatus(NKikimrScheme::StatusPreconditionFailed, response.Error);
192188
} else {
193-
db.Table<Schema::LoginSids>().Key(group).Delete();
194-
for (const TString& parent : response.TouchedGroups) {
195-
db.Table<Schema::LoginSidMembers>().Key(parent, group).Delete();
196-
}
197189
result->SetStatus(NKikimrScheme::StatusSuccess);
198190
}
199191
break;
@@ -249,20 +241,8 @@ class TAlterLogin: public TSubOperationBase {
249241
return {.Error = "User not found"};
250242
}
251243

252-
auto subTree = context.SS->ListSubTree(context.SS->RootPathId(), context.Ctx);
253-
for (auto pathId : subTree) {
254-
TPathElement::TPtr path = context.SS->PathsById.at(pathId);
255-
if (path->Owner == user) {
256-
auto pathStr = TPath::Init(pathId, context.SS).PathString();
257-
return {.Error = TStringBuilder() <<
258-
"User " << user << " owns " << pathStr << " and can't be removed"};
259-
}
260-
NACLib::TACL acl(path->ACL);
261-
if (acl.HasAccess(user)) {
262-
auto pathStr = TPath::Init(pathId, context.SS).PathString();
263-
return {.Error = TStringBuilder() <<
264-
"User " << user << " has an ACL record on " << pathStr << " and can't be removed"};
265-
}
244+
if (auto canRemove = CanRemoveSid(context, user, "User"); canRemove.Error) {
245+
return canRemove;
266246
}
267247

268248
auto removeUserResponse = context.SS->LoginProvider.RemoveUser(user);
@@ -278,6 +258,53 @@ class TAlterLogin: public TSubOperationBase {
278258
return {}; // success
279259
}
280260

261+
NLogin::TLoginProvider::TBasicResponse RemoveGroup(TOperationContext& context, const NKikimrSchemeOp::TLoginRemoveGroup& removeGroup, NIceDb::TNiceDb& db) {
262+
const TString& group = removeGroup.GetGroup();
263+
264+
if (!context.SS->LoginProvider.CheckGroupExists(group)) {
265+
if (removeGroup.GetMissingOk()) {
266+
return {}; // success
267+
}
268+
return {.Error = "Group not found"};
269+
}
270+
271+
if (auto canRemove = CanRemoveSid(context, group, "Group"); canRemove.Error) {
272+
return canRemove;
273+
}
274+
275+
auto removeGroupResponse = context.SS->LoginProvider.RemoveGroup(group);
276+
if (removeGroupResponse.Error) {
277+
return removeGroupResponse;
278+
}
279+
280+
db.Table<Schema::LoginSids>().Key(group).Delete();
281+
for (const TString& parent : removeGroupResponse.TouchedGroups) {
282+
db.Table<Schema::LoginSidMembers>().Key(parent, group).Delete();
283+
}
284+
285+
return {}; // success
286+
}
287+
288+
NLogin::TLoginProvider::TBasicResponse CanRemoveSid(TOperationContext& context, const TString sid, const TString& sidType) {
289+
auto subTree = context.SS->ListSubTree(context.SS->RootPathId(), context.Ctx);
290+
for (auto pathId : subTree) {
291+
TPathElement::TPtr path = context.SS->PathsById.at(pathId);
292+
if (path->Owner == sid) {
293+
auto pathStr = TPath::Init(pathId, context.SS).PathString();
294+
return {.Error = TStringBuilder() <<
295+
sidType << " " << sid << " owns " << pathStr << " and can't be removed"};
296+
}
297+
NACLib::TACL acl(path->ACL);
298+
if (acl.HasAccess(sid)) {
299+
auto pathStr = TPath::Init(pathId, context.SS).PathString();
300+
return {.Error = TStringBuilder() <<
301+
sidType << " " << sid << " has an ACL record on " << pathStr << " and can't be removed"};
302+
}
303+
}
304+
305+
return {}; // success
306+
}
307+
281308
void AddIsUserAdmin(const TString& user, NLogin::TLoginProvider& loginProvider, TParts& additionalParts) {
282309
const auto& adminSids = AppData()->AdministrationAllowedSIDs;
283310
bool isAdmin = adminSids.empty();

‎ydb/core/tx/schemeshard/ut_helpers/helpers.cpp

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1993,6 +1993,18 @@ namespace NSchemeShardUT_Private {
19931993
TestModificationResults(runtime, txId, expectedResults);
19941994
}
19951995

1996+
void CreateAlterLoginRemoveGroup(TTestActorRuntime& runtime, ui64 txId, const TString& database, const TString& group, const TVector<TExpectedResult>& expectedResults) {
1997+
auto modifyTx = std::make_unique<TEvSchemeShard::TEvModifySchemeTransaction>(txId, TTestTxConfig::SchemeShard);
1998+
auto transaction = modifyTx->Record.AddTransaction();
1999+
transaction->SetWorkingDir(database);
2000+
transaction->SetOperationType(NKikimrSchemeOp::EOperationType::ESchemeOpAlterLogin);
2001+
auto createGroup = transaction->MutableAlterLogin()->MutableRemoveGroup();
2002+
createGroup->SetGroup(group);
2003+
2004+
AsyncSend(runtime, TTestTxConfig::SchemeShard, modifyTx.release());
2005+
TestModificationResults(runtime, txId, expectedResults);
2006+
}
2007+
19962008
void AlterLoginAddGroupMembership(TTestActorRuntime& runtime, ui64 txId, const TString& database, const TString& member, const TString& group, const TVector<TExpectedResult>& expectedResults) {
19972009
auto modifyTx = std::make_unique<TEvSchemeShard::TEvModifySchemeTransaction>(txId, TTestTxConfig::SchemeShard);
19982010
auto transaction = modifyTx->Record.AddTransaction();

‎ydb/core/tx/schemeshard/ut_helpers/helpers.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -534,6 +534,9 @@ namespace NSchemeShardUT_Private {
534534

535535
void CreateAlterLoginCreateGroup(TTestActorRuntime& runtime, ui64 txId, const TString& database,
536536
const TString& group, const TVector<TExpectedResult>& expectedResults = {{NKikimrScheme::StatusSuccess}});
537+
538+
void CreateAlterLoginRemoveGroup(TTestActorRuntime& runtime, ui64 txId, const TString& database,
539+
const TString& group, const TVector<TExpectedResult>& expectedResults = {{NKikimrScheme::StatusSuccess}});
537540

538541
void AlterLoginAddGroupMembership(TTestActorRuntime& runtime, ui64 txId, const TString& database,
539542
const TString& member, const TString& group,

‎ydb/core/tx/schemeshard/ut_helpers/ls_checks.cpp

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1248,7 +1248,7 @@ TCheckFunc HasGroup(const TString& group, const TSet<TString> members) {
12481248
return [=](const NKikimrScheme::TEvDescribeSchemeResult& record) {
12491249
std::optional<TSet<TString>> actualMembers;
12501250
for (const auto& sid : record.GetPathDescription().GetDomainDescription().GetSecurityState().GetSids()) {
1251-
if (sid.GetName() == group) {
1251+
if (sid.GetName() == group && sid.GetType() == NLoginProto::ESidType::GROUP) {
12521252
actualMembers.emplace();
12531253
for (const auto& member : sid.GetMembers()) {
12541254
actualMembers->insert(member);
@@ -1260,6 +1260,16 @@ TCheckFunc HasGroup(const TString& group, const TSet<TString> members) {
12601260
};
12611261
}
12621262

1263+
TCheckFunc HasNoGroup(const TString& group) {
1264+
return [=](const NKikimrScheme::TEvDescribeSchemeResult& record) {
1265+
for (const auto& sid : record.GetPathDescription().GetDomainDescription().GetSecurityState().GetSids()) {
1266+
if (sid.GetName() == group && sid.GetType() == NLoginProto::ESidType::GROUP) {
1267+
UNIT_FAIL("Group " + group + " exists");
1268+
}
1269+
}
1270+
};
1271+
}
1272+
12631273
void CheckRight(const NKikimrScheme::TEvDescribeSchemeResult& record, const TString& right, bool mustHave, bool isEffective) {
12641274
const auto& self = record.GetPathDescription().GetSelf();
12651275
TSecurityObject src(self.GetOwner(), isEffective ? self.GetEffectiveACL() : self.GetACL(), false);

‎ydb/core/tx/schemeshard/ut_helpers/ls_checks.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -175,6 +175,7 @@ namespace NLs {
175175
TCheckFunc BackupHistoryCount(ui64 count);
176176

177177
TCheckFunc HasGroup(const TString& group, const TSet<TString> members);
178+
TCheckFunc HasNoGroup(const TString& group);
178179
TCheckFunc HasOwner(const TString& owner);
179180
TCheckFunc HasRight(const TString& right);
180181
TCheckFunc HasNoRight(const TString& right);

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

Lines changed: 118 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ void CheckToken(const TString& token, const NKikimrScheme::TEvDescribeSchemeResu
7373

7474
Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {
7575

76-
Y_UNIT_TEST(Login) {
76+
Y_UNIT_TEST(UserLogin) {
7777
TTestBasicRuntime runtime;
7878
TTestEnv env(runtime);
7979
ui64 txId = 100;
@@ -107,7 +107,7 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {
107107
}
108108
}
109109

110-
Y_UNIT_TEST(RemoveLogin) {
110+
Y_UNIT_TEST(RemoveUser) {
111111
TTestBasicRuntime runtime;
112112
TTestEnv env(runtime);
113113
ui64 txId = 100;
@@ -125,12 +125,12 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {
125125
}
126126
}
127127

128-
Y_UNIT_TEST(RemoveLogin_NonExisting) {
128+
Y_UNIT_TEST(RemoveUser_NonExisting) {
129129
TTestBasicRuntime runtime;
130130
TTestEnv env(runtime);
131131
ui64 txId = 100;
132132

133-
for (bool missingOk : { false, true}) {
133+
for (bool missingOk : {false, true}) {
134134
auto modifyTx = std::make_unique<TEvSchemeShard::TEvModifySchemeTransaction>(txId, TTestTxConfig::SchemeShard);
135135
auto transaction = modifyTx->Record.AddTransaction();
136136
transaction->SetWorkingDir("/MyRoot");
@@ -148,7 +148,7 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {
148148
}
149149
}
150150

151-
Y_UNIT_TEST(RemoveLogin_Groups) {
151+
Y_UNIT_TEST(RemoveUser_Groups) {
152152
TTestBasicRuntime runtime;
153153
TTestEnv env(runtime);
154154
ui64 txId = 100;
@@ -193,7 +193,7 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {
193193
}
194194
}
195195

196-
Y_UNIT_TEST(RemoveLogin_Owner) {
196+
Y_UNIT_TEST(RemoveUser_Owner) {
197197
TTestBasicRuntime runtime;
198198
TTestEnv env(runtime);
199199
ui64 txId = 100;
@@ -237,7 +237,7 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {
237237
}
238238
}
239239

240-
Y_UNIT_TEST(RemoveLogin_Acl) {
240+
Y_UNIT_TEST(RemoveUser_Acl) {
241241
TTestBasicRuntime runtime;
242242
TTestEnv env(runtime);
243243
ui64 txId = 100;
@@ -299,6 +299,117 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {
299299
}
300300
}
301301

302+
Y_UNIT_TEST(RemoveGroup) {
303+
TTestBasicRuntime runtime;
304+
TTestEnv env(runtime);
305+
ui64 txId = 100;
306+
307+
CreateAlterLoginCreateUser(runtime, ++txId, "/MyRoot", "user1", "password1");
308+
CreateAlterLoginCreateGroup(runtime, ++txId, "/MyRoot", "group1");
309+
AlterLoginAddGroupMembership(runtime, ++txId, "/MyRoot", "user1", "group1");
310+
TestDescribeResult(DescribePath(runtime, "/MyRoot"),
311+
{NLs::HasGroup("group1", {"user1"})});
312+
313+
CreateAlterLoginRemoveGroup(runtime, ++txId, "/MyRoot", "group1");
314+
TestDescribeResult(DescribePath(runtime, "/MyRoot"),
315+
{NLs::HasNoGroup("group1")});
316+
}
317+
318+
Y_UNIT_TEST(RemoveGroup_NonExisting) {
319+
TTestBasicRuntime runtime;
320+
TTestEnv env(runtime);
321+
ui64 txId = 100;
322+
323+
for (bool missingOk : {false, true}) {
324+
auto modifyTx = std::make_unique<TEvSchemeShard::TEvModifySchemeTransaction>(txId, TTestTxConfig::SchemeShard);
325+
auto transaction = modifyTx->Record.AddTransaction();
326+
transaction->SetWorkingDir("/MyRoot");
327+
transaction->SetOperationType(NKikimrSchemeOp::EOperationType::ESchemeOpAlterLogin);
328+
329+
auto removeUser = transaction->MutableAlterLogin()->MutableRemoveGroup();
330+
removeUser->SetGroup("group1");
331+
removeUser->SetMissingOk(missingOk);
332+
333+
AsyncSend(runtime, TTestTxConfig::SchemeShard, modifyTx.release());
334+
TestModificationResults(runtime, txId, TVector<TExpectedResult>{{
335+
missingOk ? NKikimrScheme::StatusSuccess : NKikimrScheme::StatusPreconditionFailed,
336+
missingOk ? "" : "Group not found"
337+
}});
338+
}
339+
}
340+
341+
Y_UNIT_TEST(RemoveGroup_Owner) {
342+
TTestBasicRuntime runtime;
343+
TTestEnv env(runtime);
344+
ui64 txId = 100;
345+
346+
CreateAlterLoginCreateGroup(runtime, ++txId, "/MyRoot", "group1");
347+
TestDescribeResult(DescribePath(runtime, "/MyRoot"),
348+
{NLs::HasGroup("group1", {})});
349+
350+
AsyncMkDir(runtime, ++txId, "/MyRoot", "Dir1");
351+
AsyncModifyACL(runtime, ++txId, "/MyRoot", "Dir1", NACLib::TDiffACL{}.SerializeAsString(), "group1");
352+
TestModificationResult(runtime, txId, NKikimrScheme::StatusSuccess);
353+
TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1"),
354+
{NLs::HasOwner("group1")});
355+
356+
CreateAlterLoginRemoveGroup(runtime, ++txId, "/MyRoot", "group1",
357+
TVector<TExpectedResult>{{NKikimrScheme::StatusPreconditionFailed, "Group group1 owns /MyRoot/Dir1 and can't be removed"}});
358+
TestDescribeResult(DescribePath(runtime, "/MyRoot"),
359+
{NLs::HasGroup("group1", {})});
360+
TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1"),
361+
{NLs::HasOwner("group1")});
362+
363+
AsyncModifyACL(runtime, ++txId, "/MyRoot", "Dir1", NACLib::TDiffACL{}.SerializeAsString(), "root@builtin");
364+
TestModificationResult(runtime, txId, NKikimrScheme::StatusSuccess);
365+
TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1"),
366+
{NLs::HasOwner("root@builtin")});
367+
368+
CreateAlterLoginRemoveGroup(runtime, ++txId, "/MyRoot", "group1");
369+
TestDescribeResult(DescribePath(runtime, "/MyRoot"),
370+
{NLs::HasNoGroup("group1")});
371+
}
372+
373+
Y_UNIT_TEST(RemoveGroup_Acl) {
374+
TTestBasicRuntime runtime;
375+
TTestEnv env(runtime);
376+
ui64 txId = 100;
377+
378+
CreateAlterLoginCreateGroup(runtime, ++txId, "/MyRoot", "group1");
379+
TestDescribeResult(DescribePath(runtime, "/MyRoot"),
380+
{NLs::HasGroup("group1", {})});
381+
382+
AsyncMkDir(runtime, ++txId, "/MyRoot", "Dir1");
383+
{
384+
NACLib::TDiffACL diffACL;
385+
diffACL.AddAccess(NACLib::EAccessType::Allow, NACLib::GenericUse, "group1");
386+
AsyncModifyACL(runtime, ++txId, "/MyRoot", "Dir1", diffACL.SerializeAsString(), "");
387+
}
388+
TestModificationResult(runtime, txId, NKikimrScheme::StatusSuccess);
389+
TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1"),
390+
{NLs::HasRight("+U:group1")});
391+
392+
CreateAlterLoginRemoveGroup(runtime, ++txId, "/MyRoot", "group1",
393+
TVector<TExpectedResult>{{NKikimrScheme::StatusPreconditionFailed, "Group group1 has an ACL record on /MyRoot/Dir1 and can't be removed"}});
394+
TestDescribeResult(DescribePath(runtime, "/MyRoot"),
395+
{NLs::HasGroup("group1", {})});
396+
TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1"),
397+
{NLs::HasRight("+U:group1")});
398+
399+
{
400+
NACLib::TDiffACL diffACL;
401+
diffACL.RemoveAccess(NACLib::EAccessType::Allow, NACLib::GenericUse, "group1");
402+
AsyncModifyACL(runtime, ++txId, "/MyRoot", "Dir1", diffACL.SerializeAsString(), "");
403+
}
404+
TestModificationResult(runtime, txId, NKikimrScheme::StatusSuccess);
405+
TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1"),
406+
{NLs::HasNoRight("+U:group1")});
407+
408+
CreateAlterLoginRemoveGroup(runtime, ++txId, "/MyRoot", "group1");
409+
TestDescribeResult(DescribePath(runtime, "/MyRoot"),
410+
{NLs::HasNoGroup("group1")});
411+
}
412+
302413
Y_UNIT_TEST(TestExternalLogin) {
303414
TTestBasicRuntime runtime;
304415
TTestEnv env(runtime);

‎ydb/library/login/login.cpp

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,10 @@ bool TLoginProvider::CheckUserExists(const TString& user) {
100100
return CheckSubjectExists(user, ESidType::USER);
101101
}
102102

103+
bool TLoginProvider::CheckGroupExists(const TString& group) {
104+
return CheckSubjectExists(group, ESidType::GROUP);
105+
}
106+
103107
TLoginProvider::TBasicResponse TLoginProvider::ModifyUser(const TModifyUserRequest& request) {
104108
TBasicResponse response;
105109

@@ -278,31 +282,29 @@ TLoginProvider::TRenameGroupResponse TLoginProvider::RenameGroup(const TRenameGr
278282
return response;
279283
}
280284

281-
TLoginProvider::TRemoveGroupResponse TLoginProvider::RemoveGroup(const TRemoveGroupRequest& request) {
285+
TLoginProvider::TRemoveGroupResponse TLoginProvider::RemoveGroup(const TString& group) {
282286
TRemoveGroupResponse response;
283287

284-
auto itGroupModify = Sids.find(request.Group);
288+
auto itGroupModify = Sids.find(group);
285289
if (itGroupModify == Sids.end() || itGroupModify->second.Type != ESidType::GROUP) {
286-
if (!request.MissingOk) {
287-
response.Error = "Group not found";
288-
}
290+
response.Error = "Group not found";
289291
return response;
290292
}
291293

292-
auto itChildToParentIndex = ChildToParentIndex.find(request.Group);
294+
auto itChildToParentIndex = ChildToParentIndex.find(group);
293295
if (itChildToParentIndex != ChildToParentIndex.end()) {
294296
for (const TString& parent : itChildToParentIndex->second) {
295297
auto itGroup = Sids.find(parent);
296298
if (itGroup != Sids.end()) {
297299
response.TouchedGroups.emplace_back(itGroup->first);
298-
itGroup->second.Members.erase(request.Group);
300+
itGroup->second.Members.erase(group);
299301
}
300302
}
301303
ChildToParentIndex.erase(itChildToParentIndex);
302304
}
303305

304306
for (const TString& member : itGroupModify->second.Members) {
305-
ChildToParentIndex[member].erase(request.Group);
307+
ChildToParentIndex[member].erase(group);
306308
}
307309

308310
Sids.erase(itGroupModify);

0 commit comments

Comments
 (0)