Skip to content

Feature flag EnableStrictAclCheck #14075

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Feb 3, 2025
Merged

Feature flag EnableStrictAclCheck #14075

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d3b9323
Feature flag EnableStrictAclCheck
azevaykin Jan 31, 2025
b517454
Test CreateAndDropUser
azevaykin Feb 2, 2025
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions ydb/core/kqp/ut/scheme/kqp_scheme_ut.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4294,8 +4294,9 @@ Y_UNIT_TEST_SUITE(KqpScheme) {
}
}

Y_UNIT_TEST(CreateAndDropUser) {
Y_UNIT_TEST_TWIN(CreateAndDropUser, StrictAclCheck) {
TKikimrRunner kikimr;
kikimr.GetTestServer().GetRuntime()->GetAppData(0).FeatureFlags.SetEnableStrictAclCheck(StrictAclCheck);
auto db = kikimr.GetTableClient();
{
// Drop non-existing user force
Expand Down Expand Up @@ -4387,8 +4388,12 @@ Y_UNIT_TEST_SUITE(KqpScheme) {
DROP USER user2;
)";
result = session.ExecuteSchemeQuery(query).GetValueSync();
UNIT_ASSERT_VALUES_EQUAL(result.GetStatus(), EStatus::PRECONDITION_FAILED);
UNIT_ASSERT_STRING_CONTAINS(result.GetIssues().ToString(), "Error: User user2 has an ACL record on /Root and can't be removed");
if (!StrictAclCheck) {
UNIT_ASSERT_VALUES_EQUAL_C(result.GetStatus(), EStatus::SUCCESS, result.GetIssues().ToString());
} else {
UNIT_ASSERT_VALUES_EQUAL(result.GetStatus(), EStatus::PRECONDITION_FAILED);
UNIT_ASSERT_STRING_CONTAINS(result.GetIssues().ToString(), "Error: User user2 has an ACL record on /Root and can't be removed");
}
}
}

Expand Down
1 change: 1 addition & 0 deletions ydb/core/protos/feature_flags.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,4 +188,5 @@ message TFeatureFlags {
optional bool EnableTopicTransfer = 163 [default = false];
optional bool EnableViewExport = 164 [default = false];
optional bool EnableColumnStore = 165 [default = false];
optional bool EnableStrictAclCheck = 166 [default = false];
}
4 changes: 4 additions & 0 deletions ydb/core/tx/schemeshard/schemeshard__operation_alter_login.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,10 @@ class TAlterLogin: public TSubOperationBase {
}

NLogin::TLoginProvider::TBasicResponse CanRemoveSid(TOperationContext& context, const TString sid, const TString& sidType) {
if (!AppData()->FeatureFlags.GetEnableStrictAclCheck()) {
return {};
}

auto subTree = context.SS->ListSubTree(context.SS->RootPathId(), context.Ctx);
for (auto pathId : subTree) {
TPathElement::TPtr path = context.SS->PathsById.at(pathId);
Expand Down
4 changes: 2 additions & 2 deletions ydb/core/tx/schemeshard/schemeshard__operation_modify_acl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ class TModifyACL: public TSubOperationBase {
return result;
}

if (acl) {
if (acl && AppData()->FeatureFlags.GetEnableStrictAclCheck()) {
NACLib::TDiffACL diffACL(acl);
for (const NACLibProto::TDiffACE& diffACE : diffACL.GetDiffACE()) {
if (static_cast<NACLib::EDiffType>(diffACE.GetDiffType()) == NACLib::EDiffType::Add) {
Expand All @@ -68,7 +68,7 @@ class TModifyACL: public TSubOperationBase {
} // remove diff type is allowed in any case
}
}
if (owner) {
if (owner && AppData()->FeatureFlags.GetEnableStrictAclCheck()) {
if (!CheckSidExistsOrIsNonYdb(context.SS->LoginProvider.Sids, owner)) {
result->SetError(NKikimrScheme::StatusPreconditionFailed,
TStringBuilder() << "Owner SID " << owner << " not found");
Expand Down
1 change: 1 addition & 0 deletions ydb/core/tx/schemeshard/ut_helpers/test_env.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -587,6 +587,7 @@ NSchemeShardUT_Private::TTestEnv::TTestEnv(TTestActorRuntime& runtime, const TTe
app.FeatureFlags.SetEnableTableDatetime64(true);
app.FeatureFlags.SetEnableVectorIndex(true);
app.FeatureFlags.SetEnableColumnStore(true);
app.FeatureFlags.SetEnableStrictAclCheck(opts.EnableStrictAclCheck_);
app.SetEnableMoveIndex(opts.EnableMoveIndex_);
app.SetEnableChangefeedInitialScan(opts.EnableChangefeedInitialScan_);
app.SetEnableNotNullDataColumns(opts.EnableNotNullDataColumns_);
Expand Down
1 change: 1 addition & 0 deletions ydb/core/tx/schemeshard/ut_helpers/test_env.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ namespace NSchemeShardUT_Private {
OPTION(std::optional<bool>, EnableBackupService, std::nullopt);
OPTION(std::optional<bool>, EnableTopicTransfer, std::nullopt);
OPTION(bool, SetupKqpProxy, false);
OPTION(bool, EnableStrictAclCheck, false);

#undef OPTION
};
Expand Down
130 changes: 73 additions & 57 deletions ydb/core/tx/schemeshard/ut_login/ut_login.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,9 +107,9 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {
}
}

Y_UNIT_TEST(RemoveUser) {
Y_UNIT_TEST_FLAG(RemoveUser, StrictAclCheck) {
TTestBasicRuntime runtime;
TTestEnv env(runtime);
TTestEnv env(runtime, TTestEnvOptions().EnableStrictAclCheck(StrictAclCheck));
ui64 txId = 100;
CreateAlterLoginCreateUser(runtime, ++txId, "/MyRoot", "user1", "password1");
CreateAlterLoginCreateUser(runtime, ++txId, "/MyRoot", "user2", "password2");
Expand All @@ -125,9 +125,9 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {
}
}

Y_UNIT_TEST(RemoveUser_NonExisting) {
Y_UNIT_TEST_FLAG(RemoveUser_NonExisting, StrictAclCheck) {
TTestBasicRuntime runtime;
TTestEnv env(runtime);
TTestEnv env(runtime, TTestEnvOptions().EnableStrictAclCheck(StrictAclCheck));
ui64 txId = 100;

for (bool missingOk : {false, true}) {
Expand All @@ -148,9 +148,9 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {
}
}

Y_UNIT_TEST(RemoveUser_Groups) {
Y_UNIT_TEST_FLAG(RemoveUser_Groups, StrictAclCheck) {
TTestBasicRuntime runtime;
TTestEnv env(runtime);
TTestEnv env(runtime, TTestEnvOptions().EnableStrictAclCheck(StrictAclCheck));
ui64 txId = 100;
CreateAlterLoginCreateUser(runtime, ++txId, "/MyRoot", "user1", "password1");
CreateAlterLoginCreateUser(runtime, ++txId, "/MyRoot", "user2", "password2");
Expand Down Expand Up @@ -193,9 +193,9 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {
}
}

Y_UNIT_TEST(RemoveUser_Owner) {
Y_UNIT_TEST_FLAG(RemoveUser_Owner, StrictAclCheck) {
TTestBasicRuntime runtime;
TTestEnv env(runtime);
TTestEnv env(runtime, TTestEnvOptions().EnableStrictAclCheck(StrictAclCheck));
ui64 txId = 100;
CreateAlterLoginCreateUser(runtime, ++txId, "/MyRoot", "user1", "password1");
CreateAlterLoginCreateUser(runtime, ++txId, "/MyRoot", "user2", "password2");
Expand All @@ -213,15 +213,17 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {
// 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 owns /MyRoot/Dir1/DirSub1 and can't be removed"}});
if (StrictAclCheck) {
CreateAlterLoginRemoveUser(runtime, ++txId, "/MyRoot", "user1",
TVector<TExpectedResult>{{NKikimrScheme::StatusPreconditionFailed, "User user1 owns /MyRoot/Dir1/DirSub1 and can't be removed"}});

// check user still exists and has their rights:
{
TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1/DirSub1"),
{NLs::HasOwner("user1")});
auto resultLogin = Login(runtime, "user1", "password1");
UNIT_ASSERT_VALUES_EQUAL(resultLogin.GetError(), "");
// check user still exists and has their rights:
{
TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1/DirSub1"),
{NLs::HasOwner("user1")});
auto resultLogin = Login(runtime, "user1", "password1");
UNIT_ASSERT_VALUES_EQUAL(resultLogin.GetError(), "");
}
}

AsyncModifyACL(runtime, ++txId, "/MyRoot/Dir1", "DirSub1", NACLib::TDiffACL().SerializeAsString(), "user2");
Expand All @@ -237,9 +239,9 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {
}
}

Y_UNIT_TEST(RemoveUser_Acl) {
Y_UNIT_TEST_FLAG(RemoveUser_Acl, StrictAclCheck) {
TTestBasicRuntime runtime;
TTestEnv env(runtime);
TTestEnv env(runtime, TTestEnvOptions().EnableStrictAclCheck(StrictAclCheck));
ui64 txId = 100;
CreateAlterLoginCreateUser(runtime, ++txId, "/MyRoot", "user1", "password1");
CreateAlterLoginCreateUser(runtime, ++txId, "/MyRoot", "user2", "password2");
Expand All @@ -262,17 +264,19 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {
// 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 an ACL record on /MyRoot/Dir1 and can't be removed"}});
if (StrictAclCheck) {
CreateAlterLoginRemoveUser(runtime, ++txId, "/MyRoot", "user1",
TVector<TExpectedResult>{{NKikimrScheme::StatusPreconditionFailed, "User user1 has an 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(), "");
// 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(), "");
}
}

{
Expand All @@ -299,9 +303,9 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {
}
}

Y_UNIT_TEST(RemoveGroup) {
Y_UNIT_TEST_FLAG(RemoveGroup, StrictAclCheck) {
TTestBasicRuntime runtime;
TTestEnv env(runtime);
TTestEnv env(runtime, TTestEnvOptions().EnableStrictAclCheck(StrictAclCheck));
ui64 txId = 100;

CreateAlterLoginCreateUser(runtime, ++txId, "/MyRoot", "user1", "password1");
Expand All @@ -315,9 +319,9 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {
{NLs::HasNoGroup("group1")});
}

Y_UNIT_TEST(RemoveGroup_NonExisting) {
Y_UNIT_TEST_FLAG(RemoveGroup_NonExisting, StrictAclCheck) {
TTestBasicRuntime runtime;
TTestEnv env(runtime);
TTestEnv env(runtime, TTestEnvOptions().EnableStrictAclCheck(StrictAclCheck));
ui64 txId = 100;

for (bool missingOk : {false, true}) {
Expand All @@ -338,9 +342,9 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {
}
}

Y_UNIT_TEST(RemoveGroup_Owner) {
Y_UNIT_TEST_FLAG(RemoveGroup_Owner, StrictAclCheck) {
TTestBasicRuntime runtime;
TTestEnv env(runtime);
TTestEnv env(runtime, TTestEnvOptions().EnableStrictAclCheck(StrictAclCheck));
ui64 txId = 100;

CreateAlterLoginCreateGroup(runtime, ++txId, "/MyRoot", "group1");
Expand All @@ -353,12 +357,14 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {
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")});
if (StrictAclCheck) {
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);
Expand All @@ -370,9 +376,9 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {
{NLs::HasNoGroup("group1")});
}

Y_UNIT_TEST(RemoveGroup_Acl) {
Y_UNIT_TEST_FLAG(RemoveGroup_Acl, StrictAclCheck) {
TTestBasicRuntime runtime;
TTestEnv env(runtime);
TTestEnv env(runtime, TTestEnvOptions().EnableStrictAclCheck(StrictAclCheck));
ui64 txId = 100;

CreateAlterLoginCreateGroup(runtime, ++txId, "/MyRoot", "group1");
Expand All @@ -389,12 +395,14 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {
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")});
if (StrictAclCheck) {
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;
Expand Down Expand Up @@ -430,22 +438,21 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {
CheckSecurityState(describe, {.PublicKeysSize = 1, .SidsSize = 0});
}

Y_UNIT_TEST(AddAccess_NonExisting) {
Y_UNIT_TEST_FLAG(AddAccess_NonExisting, StrictAclCheck) {
TTestBasicRuntime runtime;
TTestEnv env(runtime);
TTestEnv env(runtime, TTestEnvOptions().EnableStrictAclCheck(StrictAclCheck));
ui64 txId = 100;

AsyncMkDir(runtime, ++txId, "/MyRoot", "Dir1");
TestModificationResult(runtime, txId, NKikimrScheme::StatusAccepted);

{
NACLib::TDiffACL diffACL;
diffACL.AddAccess(NACLib::EAccessType::Allow, NACLib::GenericUse, "user1");
NACLib::TDiffACL diffACL;
diffACL.AddAccess(NACLib::EAccessType::Allow, NACLib::GenericUse, "user1");

if (StrictAclCheck) {
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"}});
}
Expand All @@ -454,11 +461,20 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {

TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1"),
{NLs::HasNoRight("+U:user1"), NLs::HasNoEffectiveRight("+U:user1"), NLs::HasOwner("root@builtin")});

AsyncModifyACL(runtime, ++txId, "/MyRoot", "Dir1", diffACL.SerializeAsString(), "");
TestModificationResult(runtime, txId, NKikimrScheme::StatusSuccess);

AsyncModifyACL(runtime, ++txId, "/MyRoot", "Dir1", NACLib::TDiffACL{}.SerializeAsString(), "user1");
TestModificationResult(runtime, txId, NKikimrScheme::StatusSuccess);

TestDescribeResult(DescribePath(runtime, "/MyRoot/Dir1"),
{NLs::HasRight("+U:user1"), NLs::HasEffectiveRight("+U:user1"), NLs::HasOwner("user1")});
}

Y_UNIT_TEST(AddAccess_NonYdb) {
Y_UNIT_TEST_FLAG(AddAccess_NonYdb, StrictAclCheck) {
TTestBasicRuntime runtime;
TTestEnv env(runtime);
TTestEnv env(runtime, TTestEnvOptions().EnableStrictAclCheck(StrictAclCheck));
ui64 txId = 100;

AsyncMkDir(runtime, ++txId, "/MyRoot", "Dir1");
Expand Down
Loading