Skip to content

Commit f90b59c

Browse files
jepett0jepett0
authored
Views: throw a human-readable error in case of a missing WITH (security_invoker = true) (#9320)
1 parent 627fb4d commit f90b59c

File tree

9 files changed

+122
-115
lines changed

9 files changed

+122
-115
lines changed

‎ydb/core/kqp/gateway/behaviour/view/manager.cpp‎

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -14,11 +14,6 @@ using TYqlConclusionStatus = TViewManager::TYqlConclusionStatus;
1414
using TInternalModificationContext = TViewManager::TInternalModificationContext;
1515
using TExternalModificationContext = TViewManager::TExternalModificationContext;
1616

17-
TString GetByKeyOrDefault(const NYql::TCreateObjectSettings& container, const TString& key) {
18-
const auto value = container.GetFeaturesExtractor().Extract(key);
19-
return value ? *value : TString{};
20-
}
21-
2217
TYqlConclusionStatus CheckFeatureFlag(const TInternalModificationContext& context) {
2318
auto* const actorSystem = context.GetExternalData().GetActorSystem();
2419
if (!actorSystem) {
@@ -48,6 +43,16 @@ std::pair<TString, TString> SplitPathByObjectId(const TString& objectId) {
4843
return pathPair;
4944
}
5045

46+
void ValidateOptions(NYql::TFeaturesExtractor& features) {
47+
// Current implementation does not persist the security_invoker option value.
48+
if (features.Extract("security_invoker") != "true") {
49+
ythrow TBadArgumentException() << "security_invoker option must be explicitly enabled";
50+
}
51+
if (!features.IsFinished()) {
52+
ythrow TBadArgumentException() << "Unknown property: " << features.GetRemainedParamsString();
53+
}
54+
}
55+
5156
void FillCreateViewProposal(NKikimrSchemeOp::TModifyScheme& modifyScheme,
5257
const NYql::TCreateObjectSettings& settings,
5358
const TExternalModificationContext& context) {
@@ -58,12 +63,12 @@ void FillCreateViewProposal(NKikimrSchemeOp::TModifyScheme& modifyScheme,
5863

5964
auto& viewDesc = *modifyScheme.MutableCreateView();
6065
viewDesc.SetName(pathPair.second);
61-
viewDesc.SetQueryText(GetByKeyOrDefault(settings, "query_text"));
62-
NSQLTranslation::Serialize(context.GetTranslationSettings(), *viewDesc.MutableCapturedContext());
6366

64-
if (!settings.GetFeaturesExtractor().IsFinished()) {
65-
ythrow TBadArgumentException() << "Unknown property: " << settings.GetFeaturesExtractor().GetRemainedParamsString();
66-
}
67+
auto& features = settings.GetFeaturesExtractor();
68+
viewDesc.SetQueryText(features.Extract("query_text").value_or(""));
69+
ValidateOptions(features);
70+
71+
NSQLTranslation::Serialize(context.GetTranslationSettings(), *viewDesc.MutableCapturedContext());
6772
}
6873

6974
void FillDropViewProposal(NKikimrSchemeOp::TModifyScheme& modifyScheme,

‎ydb/core/kqp/ut/view/view_ut.cpp‎

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -213,6 +213,90 @@ Y_UNIT_TEST_SUITE(TCreateAndDropViewTest) {
213213
UNIT_ASSERT_STRING_CONTAINS(creationResult.GetIssues().ToString(), "Error: Cannot divide type String and String");
214214
}
215215

216+
Y_UNIT_TEST(ParsingSecurityInvoker) {
217+
TKikimrRunner kikimr(TKikimrSettings().SetWithSampleTables(false));
218+
EnableViewsFeatureFlag(kikimr);
219+
auto session = kikimr.GetQueryClient().GetSession().ExtractValueSync().GetSession();
220+
221+
constexpr const char* path = "TheView";
222+
constexpr const char* query = "SELECT 1";
223+
224+
auto fail = [&](const char* options) {
225+
const TString creationQuery = std::format(R"(
226+
CREATE VIEW {} {} AS {};
227+
)",
228+
path,
229+
options,
230+
query
231+
);
232+
233+
const auto creationResult = session.ExecuteQuery(
234+
creationQuery,
235+
NQuery::TTxControl::NoTx()
236+
).ExtractValueSync();
237+
238+
UNIT_ASSERT_C(!creationResult.IsSuccess(), creationQuery);
239+
UNIT_ASSERT_STRING_CONTAINS(
240+
creationResult.GetIssues().ToString(), "security_invoker option must be explicitly enabled"
241+
);
242+
};
243+
fail("");
244+
fail("WITH security_invoker");
245+
fail("WITH security_invoker = false");
246+
fail("WITH SECURITY_INVOKER = true"); // option name is case-sensitive
247+
fail("WITH (security_invoker)");
248+
fail("WITH (security_invoker = false)");
249+
fail("WITH (security_invoker = true, security_invoker = false)");
250+
251+
auto succeed = [&](const char* options) {
252+
const TString creationQuery = std::format(R"(
253+
CREATE VIEW {} {} AS {};
254+
DROP VIEW {};
255+
)",
256+
path,
257+
options,
258+
query,
259+
path
260+
);
261+
ExecuteQuery(session, creationQuery);
262+
};
263+
succeed("WITH security_invoker = true");
264+
succeed("WITH (security_invoker = true)");
265+
succeed("WITH (security_invoker = tRuE)"); // bool parsing is flexible enough
266+
succeed("WITH (security_invoker = false, security_invoker = true)");
267+
268+
{
269+
// literal named expression
270+
const TString creationQuery = std::format(R"(
271+
$value = "true";
272+
CREATE VIEW {} WITH security_invoker = $value AS {};
273+
DROP VIEW {};
274+
)",
275+
path,
276+
query,
277+
path
278+
);
279+
ExecuteQuery(session, creationQuery);
280+
}
281+
{
282+
// evaluated expression
283+
const TString creationQuery = std::format(R"(
284+
$lambda = ($x) -> {{
285+
RETURN CAST($x as String)
286+
}};
287+
$value = $lambda(true);
288+
289+
CREATE VIEW {} WITH security_invoker = $value AS {};
290+
DROP VIEW {};
291+
)",
292+
path,
293+
query,
294+
path
295+
);
296+
ExecuteQuery(session, creationQuery);
297+
}
298+
}
299+
216300
Y_UNIT_TEST(ListCreatedView) {
217301
TKikimrRunner kikimr(TKikimrSettings().SetWithSampleTables(false));
218302
EnableViewsFeatureFlag(kikimr);

‎ydb/library/yql/sql/v1/SQLv1.g.in‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -600,7 +600,7 @@ alter_external_data_source_action:
600600
drop_external_data_source_stmt: DROP EXTERNAL DATA SOURCE (IF EXISTS)? object_ref;
601601

602602
create_view_stmt: CREATE VIEW object_ref
603-
with_table_settings
603+
create_object_features?
604604
AS select_stmt
605605
;
606606

@@ -628,7 +628,7 @@ drop_object_stmt: DROP OBJECT (IF EXISTS)? object_ref
628628
;
629629
drop_object_features: WITH object_features;
630630

631-
object_feature_value: id_or_type | bind_parameter | STRING_VALUE;
631+
object_feature_value: id_or_type | bind_parameter | STRING_VALUE | bool_value;
632632
object_feature_kv: an_id_or_type EQUALS object_feature_value;
633633
object_feature_flag: an_id_or_type;
634634
object_feature: object_feature_kv | object_feature_flag;
@@ -750,11 +750,11 @@ local_index: LOCAL;
750750

751751
index_subtype: an_id;
752752

753-
with_index_settings: WITH LPAREN index_setting_entry (COMMA index_setting_entry)* COMMA? RPAREN;
753+
with_index_settings: WITH LPAREN index_setting_entry (COMMA index_setting_entry)* COMMA? RPAREN;
754754
index_setting_entry: an_id EQUALS index_setting_value;
755755
index_setting_value:
756756
id_or_type
757-
| STRING_VALUE
757+
| STRING_VALUE
758758
| integer
759759
| bool_value
760760
;

‎ydb/library/yql/sql/v1/SQLv1Antlr4.g.in‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -599,7 +599,7 @@ alter_external_data_source_action:
599599
drop_external_data_source_stmt: DROP EXTERNAL DATA SOURCE (IF EXISTS)? object_ref;
600600

601601
create_view_stmt: CREATE VIEW object_ref
602-
with_table_settings
602+
create_object_features?
603603
AS select_stmt
604604
;
605605

@@ -627,7 +627,7 @@ drop_object_stmt: DROP OBJECT (IF EXISTS)? object_ref
627627
;
628628
drop_object_features: WITH object_features;
629629

630-
object_feature_value: id_or_type | bind_parameter | STRING_VALUE;
630+
object_feature_value: id_or_type | bind_parameter | STRING_VALUE | bool_value;
631631
object_feature_kv: an_id_or_type EQUALS object_feature_value;
632632
object_feature_flag: an_id_or_type;
633633
object_feature: object_feature_kv | object_feature_flag;
@@ -749,11 +749,11 @@ local_index: LOCAL;
749749

750750
index_subtype: an_id;
751751

752-
with_index_settings: WITH LPAREN index_setting_entry (COMMA index_setting_entry)* COMMA? RPAREN;
752+
with_index_settings: WITH LPAREN index_setting_entry (COMMA index_setting_entry)* COMMA? RPAREN;
753753
index_setting_entry: an_id EQUALS index_setting_value;
754754
index_setting_value:
755755
id_or_type
756-
| STRING_VALUE
756+
| STRING_VALUE
757757
| integer
758758
| bool_value
759759
;

‎ydb/library/yql/sql/v1/sql_query.cpp‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1239,8 +1239,10 @@ bool TSqlQuery::Statement(TVector<TNodePtr>& blocks, const TRule_sql_stmt_core&
12391239
}
12401240

12411241
std::map<TString, TDeferredAtom> features;
1242-
if (!ParseViewOptions(features, node.GetRule_with_table_settings4())) {
1243-
return false;
1242+
if (node.HasBlock4()) {
1243+
if (!ParseObjectFeatures(features, node.GetBlock4().GetRule_create_object_features1().GetRule_object_features2())) {
1244+
return false;
1245+
}
12441246
}
12451247
if (!ParseViewQuery(features, node.GetRule_select_stmt6())) {
12461248
return false;

‎ydb/library/yql/sql/v1/sql_translation.cpp‎

Lines changed: 7 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -1795,16 +1795,6 @@ namespace {
17951795
return true;
17961796
}
17971797

1798-
bool StoreBool(const TRule_table_setting_value& from, TDeferredAtom& to, TContext& ctx) {
1799-
if (!from.HasAlt_table_setting_value6()) {
1800-
return false;
1801-
}
1802-
// bool_value
1803-
const TString value = to_lower(ctx.Token(from.GetAlt_table_setting_value6().GetRule_bool_value1().GetToken1()));
1804-
to = TDeferredAtom(BuildLiteralBool(ctx.Pos(), FromString<bool>(value)), ctx);
1805-
return true;
1806-
}
1807-
18081798
bool StoreSplitBoundary(const TRule_literal_value_list& boundary, TVector<TVector<TNodePtr>>& to,
18091799
TSqlExpression& expr, TContext& ctx) {
18101800
TVector<TNodePtr> boundaryKeys;
@@ -1931,26 +1921,6 @@ namespace {
19311921
return true;
19321922
}
19331923

1934-
bool StoreViewOptionsEntry(const TIdentifier& id,
1935-
const TRule_table_setting_value& value,
1936-
std::map<TString, TDeferredAtom>& features,
1937-
TContext& ctx) {
1938-
const auto name = to_lower(id.Name);
1939-
const auto publicName = to_upper(name);
1940-
1941-
if (features.find(name) != features.end()) {
1942-
ctx.Error(ctx.Pos()) << publicName << " is a duplicate";
1943-
return false;
1944-
}
1945-
1946-
if (!StoreBool(value, features[name], ctx)) {
1947-
ctx.Error(ctx.Pos()) << "Value of " << publicName << " must be a bool";
1948-
return false;
1949-
}
1950-
1951-
return true;
1952-
}
1953-
19541924
template<typename TChar>
19551925
struct TPatternComponent {
19561926
TBasicString<TChar> Prefix;
@@ -4625,7 +4595,7 @@ bool TSqlTranslation::BindParameterClause(const TRule_bind_parameter& node, TDef
46254595
}
46264596

46274597
bool TSqlTranslation::ObjectFeatureValueClause(const TRule_object_feature_value& node, TDeferredAtom& result) {
4628-
// object_feature_value: an_id_or_type | bind_parameter;
4598+
// object_feature_value: id_or_type | bind_parameter | STRING_VALUE | bool_value;
46294599
switch (node.Alt_case()) {
46304600
case TRule_object_feature_value::kAltObjectFeatureValue1:
46314601
{
@@ -4650,6 +4620,12 @@ bool TSqlTranslation::ObjectFeatureValueClause(const TRule_object_feature_value&
46504620
result = TDeferredAtom(Ctx.Pos(), strValue->Content);
46514621
break;
46524622
}
4623+
case TRule_object_feature_value::kAltObjectFeatureValue4:
4624+
{
4625+
TString value = Ctx.Token(node.GetAlt_object_feature_value4().GetRule_bool_value1().GetToken1());
4626+
result = TDeferredAtom(BuildLiteralBool(Ctx.Pos(), FromString<bool>(value)), Ctx);
4627+
break;
4628+
}
46534629
case TRule_object_feature_value::ALT_NOT_SET:
46544630
Y_ABORT("You should change implementation according to grammar changes");
46554631
}
@@ -4839,32 +4815,6 @@ bool TSqlTranslation::ValidateExternalTable(const TCreateTableParameters& params
48394815
return true;
48404816
}
48414817

4842-
bool TSqlTranslation::ParseViewOptions(std::map<TString, TDeferredAtom>& features,
4843-
const TRule_with_table_settings& options) {
4844-
const auto& firstEntry = options.GetRule_table_settings_entry3();
4845-
if (!StoreViewOptionsEntry(IdEx(firstEntry.GetRule_an_id1(), *this),
4846-
firstEntry.GetRule_table_setting_value3(),
4847-
features,
4848-
Ctx)) {
4849-
return false;
4850-
}
4851-
for (const auto& block : options.GetBlock4()) {
4852-
const auto& entry = block.GetRule_table_settings_entry2();
4853-
if (!StoreViewOptionsEntry(IdEx(entry.GetRule_an_id1(), *this),
4854-
entry.GetRule_table_setting_value3(),
4855-
features,
4856-
Ctx)) {
4857-
return false;
4858-
}
4859-
}
4860-
if (const auto securityInvoker = features.find("security_invoker");
4861-
securityInvoker == features.end() || securityInvoker->second.Build()->GetLiteralValue() != "true") {
4862-
Ctx.Error(Ctx.Pos()) << "SECURITY_INVOKER option must be explicitly enabled";
4863-
return false;
4864-
}
4865-
return true;
4866-
}
4867-
48684818
bool TSqlTranslation::ParseViewQuery(
48694819
std::map<TString, TDeferredAtom>& features,
48704820
const TRule_select_stmt& query

‎ydb/library/yql/sql/v1/sql_ut.cpp‎

Lines changed: 0 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -6705,24 +6705,6 @@ Y_UNIT_TEST_SUITE(TViewSyntaxTest) {
67056705
UNIT_ASSERT_C(res.Root, res.Issues.ToString());
67066706
}
67076707

6708-
Y_UNIT_TEST(CreateViewNoSecurityInvoker) {
6709-
NYql::TAstParseResult res = SqlToYql(R"(
6710-
USE plato;
6711-
CREATE VIEW TheView AS SELECT 1;
6712-
)"
6713-
);
6714-
UNIT_ASSERT_STRING_CONTAINS(res.Issues.ToString(), "Unexpected token 'AS' : syntax error");
6715-
}
6716-
6717-
Y_UNIT_TEST(CreateViewSecurityInvokerTurnedOff) {
6718-
NYql::TAstParseResult res = SqlToYql(R"(
6719-
USE plato;
6720-
CREATE VIEW TheView WITH (security_invoker = FALSE) AS SELECT 1;
6721-
)"
6722-
);
6723-
UNIT_ASSERT_STRING_CONTAINS(res.Issues.ToString(), "SECURITY_INVOKER option must be explicitly enabled");
6724-
}
6725-
67266708
Y_UNIT_TEST(CreateViewFromTable) {
67276709
constexpr const char* path = "/PathPrefix/TheView";
67286710
constexpr const char* query = R"(

‎ydb/library/yql/sql/v1/sql_ut_antlr4.cpp‎

Lines changed: 0 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -6702,24 +6702,6 @@ Y_UNIT_TEST_SUITE(TViewSyntaxTest) {
67026702
UNIT_ASSERT_C(res.Root, res.Issues.ToString());
67036703
}
67046704

6705-
Y_UNIT_TEST(CreateViewNoSecurityInvoker) {
6706-
NYql::TAstParseResult res = SqlToYql(R"(
6707-
USE plato;
6708-
CREATE VIEW TheView AS SELECT 1;
6709-
)"
6710-
);
6711-
UNIT_ASSERT_STRING_CONTAINS(res.Issues.ToString(), "mismatched input 'AS' expecting WITH");
6712-
}
6713-
6714-
Y_UNIT_TEST(CreateViewSecurityInvokerTurnedOff) {
6715-
NYql::TAstParseResult res = SqlToYql(R"(
6716-
USE plato;
6717-
CREATE VIEW TheView WITH (security_invoker = FALSE) AS SELECT 1;
6718-
)"
6719-
);
6720-
UNIT_ASSERT_STRING_CONTAINS(res.Issues.ToString(), "SECURITY_INVOKER option must be explicitly enabled");
6721-
}
6722-
67236705
Y_UNIT_TEST(CreateViewFromTable) {
67246706
constexpr const char* path = "/PathPrefix/TheView";
67256707
constexpr const char* query = R"(

‎ydb/services/metadata/abstract/parsing.h‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -74,8 +74,10 @@ class TObjectSettingsImpl {
7474
NNodes::TCoNameValueTuple tuple = maybeTuple.Cast();
7575
if (auto maybeAtom = tuple.Value().template Maybe<NNodes::TCoAtom>()) {
7676
Features.emplace(tuple.Name().Value(), maybeAtom.Cast().Value());
77-
} else if (auto maybeDataCtor = tuple.Value().template Maybe<NNodes::TCoIntegralCtor>()) {
78-
Features.emplace(tuple.Name().Value(), maybeDataCtor.Cast().Literal().Cast<NNodes::TCoAtom>().Value());
77+
} else if (auto maybeInt = tuple.Value().template Maybe<NNodes::TCoIntegralCtor>()) {
78+
Features.emplace(tuple.Name().Value(), maybeInt.Cast().Literal().Cast<NNodes::TCoAtom>().Value());
79+
} else if (auto maybeBool = tuple.Value().template Maybe<NNodes::TCoBool>()) {
80+
Features.emplace(tuple.Name().Value(), maybeBool.Cast().Literal().Cast<NNodes::TCoAtom>().Value());
7981
}
8082
}
8183
}

0 commit comments

Comments
 (0)