Skip to content

Commit f14b341

Browse files
ijonijon
authored
25-1: security: fix query service for alter-login and domain_login_only (#15405)
Fix database selection logic in query service for user/group administration (AlterLogin) operations (in regards to `domain_login_only` mode).
1 parent 3381008 commit f14b341

File tree

6 files changed

+70
-98
lines changed

6 files changed

+70
-98
lines changed

‎ydb/core/kqp/executer_actor/kqp_scheme_executer.cpp

Lines changed: 9 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -178,15 +178,6 @@ class TKqpSchemeExecuter : public TActorBootstrapped<TKqpSchemeExecuter> {
178178
});
179179
}
180180

181-
TString GetDatabaseForLoginOperation() const {
182-
const auto domainLoginOnly = AppData()->AuthConfig.GetDomainLoginOnly();
183-
const auto domain = AppData()->DomainsInfo ? AppData()->DomainsInfo->GetDomain() : nullptr;
184-
const auto domainName = domain ? domain->Name : "";
185-
TString database;
186-
return NSchemeHelpers::SetDatabaseForLoginOperation(database, domainLoginOnly, domainName, Database)
187-
? database : Database;
188-
}
189-
190181
void MakeSchemeOperationRequest() {
191182
using TRequest = TEvTxUserProxy::TEvProposeTransaction;
192183

@@ -248,21 +239,21 @@ class TKqpSchemeExecuter : public TActorBootstrapped<TKqpSchemeExecuter> {
248239
case NKqpProto::TKqpSchemeOperation::kCreateUser: {
249240
const auto& modifyScheme = schemeOp.GetCreateUser();
250241
ev->Record.MutableTransaction()->MutableModifyScheme()->CopyFrom(modifyScheme);
251-
ev->Record.SetDatabaseName(GetDatabaseForLoginOperation());
242+
ev->Record.SetDatabaseName(NSchemeHelpers::SelectDatabaseForAlterLoginOperations(AppData(), Database));
252243
break;
253244
}
254245

255246
case NKqpProto::TKqpSchemeOperation::kAlterUser: {
256247
const auto& modifyScheme = schemeOp.GetAlterUser();
257248
ev->Record.MutableTransaction()->MutableModifyScheme()->CopyFrom(modifyScheme);
258-
ev->Record.SetDatabaseName(GetDatabaseForLoginOperation());
249+
ev->Record.SetDatabaseName(NSchemeHelpers::SelectDatabaseForAlterLoginOperations(AppData(), Database));
259250
break;
260251
}
261252

262253
case NKqpProto::TKqpSchemeOperation::kDropUser: {
263254
const auto& modifyScheme = schemeOp.GetDropUser();
264255
ev->Record.MutableTransaction()->MutableModifyScheme()->CopyFrom(modifyScheme);
265-
ev->Record.SetDatabaseName(GetDatabaseForLoginOperation());
256+
ev->Record.SetDatabaseName(NSchemeHelpers::SelectDatabaseForAlterLoginOperations(AppData(), Database));
266257
break;
267258
}
268259
case NKqpProto::TKqpSchemeOperation::kCreateExternalTable: {
@@ -284,35 +275,35 @@ class TKqpSchemeExecuter : public TActorBootstrapped<TKqpSchemeExecuter> {
284275
case NKqpProto::TKqpSchemeOperation::kCreateGroup: {
285276
const auto& modifyScheme = schemeOp.GetCreateGroup();
286277
ev->Record.MutableTransaction()->MutableModifyScheme()->CopyFrom(modifyScheme);
287-
ev->Record.SetDatabaseName(GetDatabaseForLoginOperation());
278+
ev->Record.SetDatabaseName(NSchemeHelpers::SelectDatabaseForAlterLoginOperations(AppData(), Database));
288279
break;
289280
}
290281

291282
case NKqpProto::TKqpSchemeOperation::kAddGroupMembership: {
292283
const auto& modifyScheme = schemeOp.GetAddGroupMembership();
293284
ev->Record.MutableTransaction()->MutableModifyScheme()->CopyFrom(modifyScheme);
294-
ev->Record.SetDatabaseName(GetDatabaseForLoginOperation());
285+
ev->Record.SetDatabaseName(NSchemeHelpers::SelectDatabaseForAlterLoginOperations(AppData(), Database));
295286
break;
296287
}
297288

298289
case NKqpProto::TKqpSchemeOperation::kRemoveGroupMembership: {
299290
const auto& modifyScheme = schemeOp.GetRemoveGroupMembership();
300291
ev->Record.MutableTransaction()->MutableModifyScheme()->CopyFrom(modifyScheme);
301-
ev->Record.SetDatabaseName(GetDatabaseForLoginOperation());
292+
ev->Record.SetDatabaseName(NSchemeHelpers::SelectDatabaseForAlterLoginOperations(AppData(), Database));
302293
break;
303294
}
304295

305296
case NKqpProto::TKqpSchemeOperation::kRenameGroup: {
306297
const auto& modifyScheme = schemeOp.GetRenameGroup();
307298
ev->Record.MutableTransaction()->MutableModifyScheme()->CopyFrom(modifyScheme);
308-
ev->Record.SetDatabaseName(GetDatabaseForLoginOperation());
299+
ev->Record.SetDatabaseName(NSchemeHelpers::SelectDatabaseForAlterLoginOperations(AppData(), Database));
309300
break;
310301
}
311302

312303
case NKqpProto::TKqpSchemeOperation::kDropGroup: {
313304
const auto& modifyScheme = schemeOp.GetDropGroup();
314305
ev->Record.MutableTransaction()->MutableModifyScheme()->CopyFrom(modifyScheme);
315-
ev->Record.SetDatabaseName(GetDatabaseForLoginOperation());
306+
ev->Record.SetDatabaseName(NSchemeHelpers::SelectDatabaseForAlterLoginOperations(AppData(), Database));
316307
break;
317308
}
318309

@@ -619,7 +610,7 @@ class TKqpSchemeExecuter : public TActorBootstrapped<TKqpSchemeExecuter> {
619610
<< "Error creating temporary directory: "
620611
<< result->Get()->Result.Issues().ToString(true));
621612
}
622-
613+
623614
CreateSessionDirectory();
624615
}
625616

‎ydb/core/kqp/gateway/kqp_gateway.h

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -190,8 +190,6 @@ class IKqpGateway : public NYql::IKikimrGateway {
190190
public:
191191
virtual TString GetDatabase() = 0;
192192
virtual TString GetDatabaseId() = 0;
193-
virtual bool GetDomainLoginOnly() = 0;
194-
virtual TMaybe<TString> GetDomainName() = 0;
195193

196194
/* Scheme */
197195
virtual NThreading::TFuture<TKqpTableProfilesResult> GetTableProfiles() = 0;

‎ydb/core/kqp/gateway/kqp_ic_gateway.cpp

Lines changed: 16 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -773,21 +773,6 @@ class TKikimrIcGateway : public IKqpGateway {
773773
ClientAddress = clientAddress;
774774
}
775775

776-
bool GetDomainLoginOnly() override {
777-
TAppData* appData = AppData(ActorSystem);
778-
return appData && appData->AuthConfig.GetDomainLoginOnly();
779-
}
780-
781-
TMaybe<TString> GetDomainName() override {
782-
TAppData* appData = AppData(ActorSystem);
783-
if (GetDomainLoginOnly()) {
784-
if (appData->DomainsInfo && appData->DomainsInfo->Domain) {
785-
return appData->DomainsInfo->GetDomain()->Name;
786-
}
787-
}
788-
return {};
789-
}
790-
791776
TVector<NKikimrKqp::TKqpTableMetadataProto> GetCollectedSchemeData() override {
792777
return MetadataLoader->GetCollectedSchemeData();
793778
}
@@ -1361,8 +1346,8 @@ class TKikimrIcGateway : public IKqpGateway {
13611346
return InvalidCluster<TGenericResult>(cluster);
13621347
}
13631348

1364-
TString database;
1365-
if (!GetDatabaseForLoginOperation(database)) {
1349+
TString database = NSchemeHelpers::SelectDatabaseForAlterLoginOperations(AppData(ActorSystem), Database);
1350+
if (database.empty()) {
13661351
return MakeFuture(ResultFromError<TGenericResult>("Couldn't get domain name"));
13671352
}
13681353

@@ -1407,8 +1392,8 @@ class TKikimrIcGateway : public IKqpGateway {
14071392
return InvalidCluster<TGenericResult>(cluster);
14081393
}
14091394

1410-
TString database;
1411-
if (!GetDatabaseForLoginOperation(database)) {
1395+
TString database = NSchemeHelpers::SelectDatabaseForAlterLoginOperations(AppData(ActorSystem), Database);
1396+
if (database.empty()) {
14121397
return MakeFuture(ResultFromError<TGenericResult>("Couldn't get domain name"));
14131398
}
14141399

@@ -1456,8 +1441,8 @@ class TKikimrIcGateway : public IKqpGateway {
14561441
return InvalidCluster<TGenericResult>(cluster);
14571442
}
14581443

1459-
TString database;
1460-
if (!GetDatabaseForLoginOperation(database)) {
1444+
TString database = NSchemeHelpers::SelectDatabaseForAlterLoginOperations(AppData(ActorSystem), Database);
1445+
if (database.empty()) {
14611446
return MakeFuture(ResultFromError<TGenericResult>("Couldn't get domain name"));
14621447
}
14631448

@@ -1532,8 +1517,8 @@ class TKikimrIcGateway : public IKqpGateway {
15321517
if (!Owner.CheckCluster(cluster)) {
15331518
return InvalidCluster<TGenericResult>(cluster);
15341519
}
1535-
TString database;
1536-
if (!Owner.GetDatabaseForLoginOperation(database)) {
1520+
const auto appData = AppData(Owner.ActorSystem);
1521+
if (!(appData && appData->DomainsInfo && appData->DomainsInfo->Domain)) {
15371522
return MakeFuture(ResultFromError<TGenericResult>("Couldn't get domain name"));
15381523
}
15391524
NMetadata::IClassBehaviour::TPtr cBehaviour(NMetadata::IClassBehaviour::TFactory::Construct(settings.GetTypeId()));
@@ -1651,8 +1636,8 @@ class TKikimrIcGateway : public IKqpGateway {
16511636
return InvalidCluster<TGenericResult>(cluster);
16521637
}
16531638

1654-
TString database;
1655-
if (!GetDatabaseForLoginOperation(database)) {
1639+
TString database = NSchemeHelpers::SelectDatabaseForAlterLoginOperations(AppData(ActorSystem), Database);
1640+
if (database.empty()) {
16561641
return MakeFuture(ResultFromError<TGenericResult>("Couldn't get domain name"));
16571642
}
16581643

@@ -1691,8 +1676,8 @@ class TKikimrIcGateway : public IKqpGateway {
16911676
return InvalidCluster<TGenericResult>(cluster);
16921677
}
16931678

1694-
TString database;
1695-
if (!GetDatabaseForLoginOperation(database)) {
1679+
TString database = NSchemeHelpers::SelectDatabaseForAlterLoginOperations(AppData(ActorSystem), Database);
1680+
if (database.empty()) {
16961681
return MakeFuture(ResultFromError<TGenericResult>("Couldn't get domain name"));
16971682
}
16981683

@@ -1783,8 +1768,8 @@ class TKikimrIcGateway : public IKqpGateway {
17831768
return InvalidCluster<TGenericResult>(cluster);
17841769
}
17851770

1786-
TString database;
1787-
if (!GetDatabaseForLoginOperation(database)) {
1771+
TString database = NSchemeHelpers::SelectDatabaseForAlterLoginOperations(AppData(ActorSystem), Database);
1772+
if (database.empty()) {
17881773
return MakeFuture(ResultFromError<TGenericResult>("Couldn't get domain name"));
17891774
}
17901775

@@ -1824,8 +1809,8 @@ class TKikimrIcGateway : public IKqpGateway {
18241809
return InvalidCluster<TGenericResult>(cluster);
18251810
}
18261811

1827-
TString database;
1828-
if (!GetDatabaseForLoginOperation(database)) {
1812+
TString database = NSchemeHelpers::SelectDatabaseForAlterLoginOperations(AppData(ActorSystem), Database);
1813+
if (database.empty()) {
18291814
return MakeFuture(ResultFromError<TGenericResult>("Couldn't get domain name"));
18301815
}
18311816

@@ -2331,10 +2316,6 @@ class TKikimrIcGateway : public IKqpGateway {
23312316
return cluster == Cluster;
23322317
}
23332318

2334-
bool GetDatabaseForLoginOperation(TString& database) {
2335-
return NSchemeHelpers::SetDatabaseForLoginOperation(database, GetDomainLoginOnly(), GetDomainName(), GetDatabase());
2336-
}
2337-
23382319
bool GetPathPair(const TString& tableName, std::pair<TString, TString>& pathPair,
23392320
TString& error, bool createDir)
23402321
{

‎ydb/core/kqp/gateway/utils/scheme_helpers.cpp

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,10 @@
11
#include "scheme_helpers.h"
22

3+
#include <ydb/core/base/appdata.h>
34
#include <ydb/core/base/path.h>
45
#include <ydb/core/base/table_index.h>
56
#include <ydb/core/protos/external_sources.pb.h>
7+
#include <ydb/core/protos/auth.pb.h>
68

79
namespace NKikimr::NKqp::NSchemeHelpers {
810

@@ -56,14 +58,24 @@ TVector<TString> CreateIndexTablePath(const TString& tableName, const NYql::TInd
5658
return paths;
5759
}
5860

59-
bool SetDatabaseForLoginOperation(TString& result, bool getDomainLoginOnly, TMaybe<TString> domainName,
60-
const TString& database)
61-
{
62-
if (getDomainLoginOnly && !domainName) {
63-
return false;
61+
TString GetDomainDatabase(const TAppData* appData) {
62+
if (appData->DomainsInfo && appData->DomainsInfo->Domain) {
63+
if (const auto& name = appData->DomainsInfo->GetDomain()->Name) {
64+
return "/" + name;
65+
}
66+
}
67+
return {};
68+
}
69+
70+
// DomainLoginOnly setting determine what database should handle user|group administration operations (AlterLogin).
71+
// DomainLoginOnly = false -- database where request is directed to
72+
// DomainLoginOnly = true -- domain (root) database
73+
TString SelectDatabaseForAlterLoginOperations(const TAppData* appData, const TString& requestDatabase) {
74+
if (appData->AuthConfig.GetDomainLoginOnly()) {
75+
return GetDomainDatabase(appData);
76+
} else {
77+
return requestDatabase;
6478
}
65-
result = domainName ? "/" + *domainName : database;
66-
return true;
6779
}
6880

6981
void FillCreateExternalTableColumnDesc(NKikimrSchemeOp::TExternalTableDescription& externalTableDesc,

‎ydb/core/kqp/gateway/utils/scheme_helpers.h

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
#pragma once
22
#include <ydb/core/kqp/provider/yql_kikimr_gateway.h>
33
#include <ydb/core/protos/flat_scheme_op.pb.h>
4+
#include <ydb/core/base/appdata_fwd.h>
45

56
#include <util/generic/string.h>
67
#include <util/string/join.h>
@@ -24,8 +25,9 @@ bool SplitTablePath(const TString& tableName, const TString& database, std::pair
2425

2526
TVector<TString> CreateIndexTablePath(const TString& tableName, const NYql::TIndexDescription& index);
2627

27-
bool SetDatabaseForLoginOperation(TString& result, bool getDomainLoginOnly, TMaybe<TString> domainName,
28-
const TString& database);
28+
TString GetDomainDatabase(const TAppData* appData);
29+
30+
TString SelectDatabaseForAlterLoginOperations(const TAppData* appData, const TString& requestDatabase);
2931

3032
void FillCreateExternalTableColumnDesc(NKikimrSchemeOp::TExternalTableDescription& externalTableDesc,
3133
const TString& name,

0 commit comments

Comments
 (0)