Skip to content

Commit

Permalink
Forbid to remove group with access (#13930)
Browse files Browse the repository at this point in the history
  • Loading branch information
@kunga
kunga authored Jan 29, 2025
1 parent 7102021 commit 3d42b93
Show file tree
Hide file tree
Showing 9 changed files with 208 additions and 46 deletions.
2 changes: 1 addition & 1 deletion ydb/core/security/ticket_parser_ut.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ Y_UNIT_TEST_SUITE(TTicketParserTest) {
UNIT_ASSERT(result->Token->IsExist("group2"));
UNIT_ASSERT_VALUES_EQUAL(result->Token->GetGroupSIDs().size(), 3);

provider.RemoveGroup({.Group = "group2"});
provider.RemoveGroup("group2");
provider.CreateGroup({.Group = "group3"});
provider.AddGroupMembership({.Group = "group3", .Member = "user1"});
runtime->Send(new IEventHandle(MakeTicketParserID(), sender, new TEvTicketParser::TEvUpdateLoginSecurityState(provider.GetSecurityState())), 0);
Expand Down
73 changes: 50 additions & 23 deletions ydb/core/tx/schemeshard/schemeshard__operation_alter_login.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -182,18 +182,10 @@ class TAlterLogin: public TSubOperationBase {
}
case NKikimrSchemeOp::TAlterLogin::kRemoveGroup: {
const auto& removeGroup = alterLogin.GetRemoveGroup();
const TString& group = removeGroup.GetGroup();
auto response = context.SS->LoginProvider.RemoveGroup({
.Group = group,
.MissingOk = removeGroup.GetMissingOk()
});
auto response = RemoveGroup(context, removeGroup, db);
if (response.Error) {
result->SetStatus(NKikimrScheme::StatusPreconditionFailed, response.Error);
} else {
db.Table<Schema::LoginSids>().Key(group).Delete();
for (const TString& parent : response.TouchedGroups) {
db.Table<Schema::LoginSidMembers>().Key(parent, group).Delete();
}
result->SetStatus(NKikimrScheme::StatusSuccess);
}
break;
Expand Down Expand Up @@ -249,20 +241,8 @@ class TAlterLogin: public TSubOperationBase {
return {.Error = "User not found"};
}

auto subTree = context.SS->ListSubTree(context.SS->RootPathId(), context.Ctx);
for (auto pathId : subTree) {
TPathElement::TPtr path = context.SS->PathsById.at(pathId);
if (path->Owner == user) {
auto pathStr = TPath::Init(pathId, context.SS).PathString();
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"};
}
if (auto canRemove = CanRemoveSid(context, user, "User"); canRemove.Error) {
return canRemove;
}

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

NLogin::TLoginProvider::TBasicResponse RemoveGroup(TOperationContext& context, const NKikimrSchemeOp::TLoginRemoveGroup& removeGroup, NIceDb::TNiceDb& db) {
const TString& group = removeGroup.GetGroup();

if (!context.SS->LoginProvider.CheckGroupExists(group)) {
if (removeGroup.GetMissingOk()) {
return {}; // success
}
return {.Error = "Group not found"};
}

if (auto canRemove = CanRemoveSid(context, group, "Group"); canRemove.Error) {
return canRemove;
}

auto removeGroupResponse = context.SS->LoginProvider.RemoveGroup(group);
if (removeGroupResponse.Error) {
return removeGroupResponse;
}

db.Table<Schema::LoginSids>().Key(group).Delete();
for (const TString& parent : removeGroupResponse.TouchedGroups) {
db.Table<Schema::LoginSidMembers>().Key(parent, group).Delete();
}

return {}; // success
}

NLogin::TLoginProvider::TBasicResponse CanRemoveSid(TOperationContext& context, const TString sid, const TString& sidType) {
auto subTree = context.SS->ListSubTree(context.SS->RootPathId(), context.Ctx);
for (auto pathId : subTree) {
TPathElement::TPtr path = context.SS->PathsById.at(pathId);
if (path->Owner == sid) {
auto pathStr = TPath::Init(pathId, context.SS).PathString();
return {.Error = TStringBuilder() <<
sidType << " " << sid << " owns " << pathStr << " and can't be removed"};
}
NACLib::TACL acl(path->ACL);
if (acl.HasAccess(sid)) {
auto pathStr = TPath::Init(pathId, context.SS).PathString();
return {.Error = TStringBuilder() <<
sidType << " " << sid << " has an ACL record on " << pathStr << " and can't be removed"};
}
}

return {}; // success
}

void AddIsUserAdmin(const TString& user, NLogin::TLoginProvider& loginProvider, TParts& additionalParts) {
const auto& adminSids = AppData()->AdministrationAllowedSIDs;
bool isAdmin = adminSids.empty();
Expand Down
12 changes: 12 additions & 0 deletions ydb/core/tx/schemeshard/ut_helpers/helpers.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1993,6 +1993,18 @@ namespace NSchemeShardUT_Private {
TestModificationResults(runtime, txId, expectedResults);
}

void CreateAlterLoginRemoveGroup(TTestActorRuntime& runtime, ui64 txId, const TString& database, const TString& group, const TVector<TExpectedResult>& expectedResults) {
auto modifyTx = std::make_unique<TEvSchemeShard::TEvModifySchemeTransaction>(txId, TTestTxConfig::SchemeShard);
auto transaction = modifyTx->Record.AddTransaction();
transaction->SetWorkingDir(database);
transaction->SetOperationType(NKikimrSchemeOp::EOperationType::ESchemeOpAlterLogin);
auto createGroup = transaction->MutableAlterLogin()->MutableRemoveGroup();
createGroup->SetGroup(group);

AsyncSend(runtime, TTestTxConfig::SchemeShard, modifyTx.release());
TestModificationResults(runtime, txId, expectedResults);
}

void AlterLoginAddGroupMembership(TTestActorRuntime& runtime, ui64 txId, const TString& database, const TString& member, const TString& group, const TVector<TExpectedResult>& expectedResults) {
auto modifyTx = std::make_unique<TEvSchemeShard::TEvModifySchemeTransaction>(txId, TTestTxConfig::SchemeShard);
auto transaction = modifyTx->Record.AddTransaction();
Expand Down
3 changes: 3 additions & 0 deletions ydb/core/tx/schemeshard/ut_helpers/helpers.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -534,6 +534,9 @@ namespace NSchemeShardUT_Private {

void CreateAlterLoginCreateGroup(TTestActorRuntime& runtime, ui64 txId, const TString& database,
const TString& group, const TVector<TExpectedResult>& expectedResults = {{NKikimrScheme::StatusSuccess}});

void CreateAlterLoginRemoveGroup(TTestActorRuntime& runtime, ui64 txId, const TString& database,
const TString& group, const TVector<TExpectedResult>& expectedResults = {{NKikimrScheme::StatusSuccess}});

void AlterLoginAddGroupMembership(TTestActorRuntime& runtime, ui64 txId, const TString& database,
const TString& member, const TString& group,
Expand Down
12 changes: 11 additions & 1 deletion ydb/core/tx/schemeshard/ut_helpers/ls_checks.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1248,7 +1248,7 @@ 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) {
if (sid.GetName() == group && sid.GetType() == NLoginProto::ESidType::GROUP) {
actualMembers.emplace();
for (const auto& member : sid.GetMembers()) {
actualMembers->insert(member);
Expand All @@ -1260,6 +1260,16 @@ TCheckFunc HasGroup(const TString& group, const TSet<TString> members) {
};
}

TCheckFunc HasNoGroup(const TString& group) {
return [=](const NKikimrScheme::TEvDescribeSchemeResult& record) {
for (const auto& sid : record.GetPathDescription().GetDomainDescription().GetSecurityState().GetSids()) {
if (sid.GetName() == group && sid.GetType() == NLoginProto::ESidType::GROUP) {
UNIT_FAIL("Group " + group + " exists");
}
}
};
}

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);
Expand Down
1 change: 1 addition & 0 deletions ydb/core/tx/schemeshard/ut_helpers/ls_checks.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,7 @@ namespace NLs {
TCheckFunc BackupHistoryCount(ui64 count);

TCheckFunc HasGroup(const TString& group, const TSet<TString> members);
TCheckFunc HasNoGroup(const TString& group);
TCheckFunc HasOwner(const TString& owner);
TCheckFunc HasRight(const TString& right);
TCheckFunc HasNoRight(const TString& right);
Expand Down
125 changes: 118 additions & 7 deletions ydb/core/tx/schemeshard/ut_login/ut_login.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ void CheckToken(const TString& token, const NKikimrScheme::TEvDescribeSchemeResu

Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {

Y_UNIT_TEST(Login) {
Y_UNIT_TEST(UserLogin) {
TTestBasicRuntime runtime;
TTestEnv env(runtime);
ui64 txId = 100;
Expand Down Expand Up @@ -107,7 +107,7 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {
}
}

Y_UNIT_TEST(RemoveLogin) {
Y_UNIT_TEST(RemoveUser) {
TTestBasicRuntime runtime;
TTestEnv env(runtime);
ui64 txId = 100;
Expand All @@ -125,12 +125,12 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {
}
}

Y_UNIT_TEST(RemoveLogin_NonExisting) {
Y_UNIT_TEST(RemoveUser_NonExisting) {
TTestBasicRuntime runtime;
TTestEnv env(runtime);
ui64 txId = 100;

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

Y_UNIT_TEST(RemoveLogin_Groups) {
Y_UNIT_TEST(RemoveUser_Groups) {
TTestBasicRuntime runtime;
TTestEnv env(runtime);
ui64 txId = 100;
Expand Down Expand Up @@ -193,7 +193,7 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {
}
}

Y_UNIT_TEST(RemoveLogin_Owner) {
Y_UNIT_TEST(RemoveUser_Owner) {
TTestBasicRuntime runtime;
TTestEnv env(runtime);
ui64 txId = 100;
Expand Down Expand Up @@ -237,7 +237,7 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {
}
}

Y_UNIT_TEST(RemoveLogin_Acl) {
Y_UNIT_TEST(RemoveUser_Acl) {
TTestBasicRuntime runtime;
TTestEnv env(runtime);
ui64 txId = 100;
Expand Down Expand Up @@ -299,6 +299,117 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {
}
}

Y_UNIT_TEST(RemoveGroup) {
TTestBasicRuntime runtime;
TTestEnv env(runtime);
ui64 txId = 100;

CreateAlterLoginCreateUser(runtime, ++txId, "/MyRoot", "user1", "password1");
CreateAlterLoginCreateGroup(runtime, ++txId, "/MyRoot", "group1");
AlterLoginAddGroupMembership(runtime, ++txId, "/MyRoot", "user1", "group1");
TestDescribeResult(DescribePath(runtime, "/MyRoot"),
{NLs::HasGroup("group1", {"user1"})});

CreateAlterLoginRemoveGroup(runtime, ++txId, "/MyRoot", "group1");
TestDescribeResult(DescribePath(runtime, "/MyRoot"),
{NLs::HasNoGroup("group1")});
}

Y_UNIT_TEST(RemoveGroup_NonExisting) {
TTestBasicRuntime runtime;
TTestEnv env(runtime);
ui64 txId = 100;

for (bool missingOk : {false, true}) {
auto modifyTx = std::make_unique<TEvSchemeShard::TEvModifySchemeTransaction>(txId, TTestTxConfig::SchemeShard);
auto transaction = modifyTx->Record.AddTransaction();
transaction->SetWorkingDir("/MyRoot");
transaction->SetOperationType(NKikimrSchemeOp::EOperationType::ESchemeOpAlterLogin);

auto removeUser = transaction->MutableAlterLogin()->MutableRemoveGroup();
removeUser->SetGroup("group1");
removeUser->SetMissingOk(missingOk);

AsyncSend(runtime, TTestTxConfig::SchemeShard, modifyTx.release());
TestModificationResults(runtime, txId, TVector<TExpectedResult>{{
missingOk ? NKikimrScheme::StatusSuccess : NKikimrScheme::StatusPreconditionFailed,
missingOk ? "" : "Group not found"
}});
}
}

Y_UNIT_TEST(RemoveGroup_Owner) {
TTestBasicRuntime runtime;
TTestEnv env(runtime);
ui64 txId = 100;

CreateAlterLoginCreateGroup(runtime, ++txId, "/MyRoot", "group1");
TestDescribeResult(DescribePath(runtime, "/MyRoot"),
{NLs::HasGroup("group1", {})});

AsyncMkDir(runtime, ++txId, "/MyRoot", "Dir1");
AsyncModifyACL(runtime, ++txId, "/MyRoot", "Dir1", NACLib::TDiffACL{}.SerializeAsString(), "group1");
TestModificationResult(runtime, txId, NKikimrScheme::StatusSuccess);
TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1"),
{NLs::HasOwner("group1")});

CreateAlterLoginRemoveGroup(runtime, ++txId, "/MyRoot", "group1",
TVector<TExpectedResult>{{NKikimrScheme::StatusPreconditionFailed, "Group group1 owns /MyRoot/Dir1 and can't be removed"}});
TestDescribeResult(DescribePath(runtime, "/MyRoot"),
{NLs::HasGroup("group1", {})});
TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1"),
{NLs::HasOwner("group1")});

AsyncModifyACL(runtime, ++txId, "/MyRoot", "Dir1", NACLib::TDiffACL{}.SerializeAsString(), "root@builtin");
TestModificationResult(runtime, txId, NKikimrScheme::StatusSuccess);
TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1"),
{NLs::HasOwner("root@builtin")});

CreateAlterLoginRemoveGroup(runtime, ++txId, "/MyRoot", "group1");
TestDescribeResult(DescribePath(runtime, "/MyRoot"),
{NLs::HasNoGroup("group1")});
}

Y_UNIT_TEST(RemoveGroup_Acl) {
TTestBasicRuntime runtime;
TTestEnv env(runtime);
ui64 txId = 100;

CreateAlterLoginCreateGroup(runtime, ++txId, "/MyRoot", "group1");
TestDescribeResult(DescribePath(runtime, "/MyRoot"),
{NLs::HasGroup("group1", {})});

AsyncMkDir(runtime, ++txId, "/MyRoot", "Dir1");
{
NACLib::TDiffACL diffACL;
diffACL.AddAccess(NACLib::EAccessType::Allow, NACLib::GenericUse, "group1");
AsyncModifyACL(runtime, ++txId, "/MyRoot", "Dir1", diffACL.SerializeAsString(), "");
}
TestModificationResult(runtime, txId, NKikimrScheme::StatusSuccess);
TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1"),
{NLs::HasRight("+U:group1")});

CreateAlterLoginRemoveGroup(runtime, ++txId, "/MyRoot", "group1",
TVector<TExpectedResult>{{NKikimrScheme::StatusPreconditionFailed, "Group group1 has an ACL record on /MyRoot/Dir1 and can't be removed"}});
TestDescribeResult(DescribePath(runtime, "/MyRoot"),
{NLs::HasGroup("group1", {})});
TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1"),
{NLs::HasRight("+U:group1")});

{
NACLib::TDiffACL diffACL;
diffACL.RemoveAccess(NACLib::EAccessType::Allow, NACLib::GenericUse, "group1");
AsyncModifyACL(runtime, ++txId, "/MyRoot", "Dir1", diffACL.SerializeAsString(), "");
}
TestModificationResult(runtime, txId, NKikimrScheme::StatusSuccess);
TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1"),
{NLs::HasNoRight("+U:group1")});

CreateAlterLoginRemoveGroup(runtime, ++txId, "/MyRoot", "group1");
TestDescribeResult(DescribePath(runtime, "/MyRoot"),
{NLs::HasNoGroup("group1")});
}

Y_UNIT_TEST(TestExternalLogin) {
TTestBasicRuntime runtime;
TTestEnv env(runtime);
Expand Down
18 changes: 10 additions & 8 deletions ydb/library/login/login.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,10 @@ bool TLoginProvider::CheckUserExists(const TString& user) {
return CheckSubjectExists(user, ESidType::USER);
}

bool TLoginProvider::CheckGroupExists(const TString& group) {
return CheckSubjectExists(group, ESidType::GROUP);
}

TLoginProvider::TBasicResponse TLoginProvider::ModifyUser(const TModifyUserRequest& request) {
TBasicResponse response;

Expand Down Expand Up @@ -278,31 +282,29 @@ TLoginProvider::TRenameGroupResponse TLoginProvider::RenameGroup(const TRenameGr
return response;
}

TLoginProvider::TRemoveGroupResponse TLoginProvider::RemoveGroup(const TRemoveGroupRequest& request) {
TLoginProvider::TRemoveGroupResponse TLoginProvider::RemoveGroup(const TString& group) {
TRemoveGroupResponse response;

auto itGroupModify = Sids.find(request.Group);
auto itGroupModify = Sids.find(group);
if (itGroupModify == Sids.end() || itGroupModify->second.Type != ESidType::GROUP) {
if (!request.MissingOk) {
response.Error = "Group not found";
}
response.Error = "Group not found";
return response;
}

auto itChildToParentIndex = ChildToParentIndex.find(request.Group);
auto itChildToParentIndex = ChildToParentIndex.find(group);
if (itChildToParentIndex != ChildToParentIndex.end()) {
for (const TString& parent : itChildToParentIndex->second) {
auto itGroup = Sids.find(parent);
if (itGroup != Sids.end()) {
response.TouchedGroups.emplace_back(itGroup->first);
itGroup->second.Members.erase(request.Group);
itGroup->second.Members.erase(group);
}
}
ChildToParentIndex.erase(itChildToParentIndex);
}

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

Sids.erase(itGroupModify);
Expand Down
Loading

0 comments on commit 3d42b93

Please sign in to comment.