Skip to content

Commit d6cb748

Browse files
jepett0jepett0
committed
Views: throw a human-readable error in case of a missing WITH (security_invoker = true) (#9320)
1 parent b9788ef commit d6cb748

File tree

7 files changed

+134
-92
lines changed

7 files changed

+134
-92
lines changed

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

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -12,11 +12,6 @@ namespace {
1212
using TYqlConclusionStatus = TViewManager::TYqlConclusionStatus;
1313
using TInternalModificationContext = TViewManager::TInternalModificationContext;
1414

15-
TString GetByKeyOrDefault(const NYql::TCreateObjectSettings& container, const TString& key) {
16-
const auto value = container.GetFeaturesExtractor().Extract(key);
17-
return value ? *value : TString{};
18-
}
19-
2015
TYqlConclusionStatus CheckFeatureFlag(TInternalModificationContext& context) {
2116
auto* const actorSystem = context.GetExternalData().GetActorSystem();
2217
if (!actorSystem) {
@@ -46,6 +41,16 @@ std::pair<TString, TString> SplitPathByObjectId(const TString& objectId) {
4641
return pathPair;
4742
}
4843

44+
void ValidateOptions(NYql::TFeaturesExtractor& features) {
45+
// Current implementation does not persist the security_invoker option value.
46+
if (features.Extract("security_invoker") != "true") {
47+
ythrow TBadArgumentException() << "security_invoker option must be explicitly enabled";
48+
}
49+
if (!features.IsFinished()) {
50+
ythrow TBadArgumentException() << "Unknown property: " << features.GetRemainedParamsString();
51+
}
52+
}
53+
4954
void FillCreateViewProposal(NKikimrSchemeOp::TModifyScheme& modifyScheme,
5055
const NYql::TCreateObjectSettings& settings,
5156
const TString& database) {
@@ -56,11 +61,10 @@ void FillCreateViewProposal(NKikimrSchemeOp::TModifyScheme& modifyScheme,
5661

5762
auto& viewDesc = *modifyScheme.MutableCreateView();
5863
viewDesc.SetName(pathPair.second);
59-
viewDesc.SetQueryText(GetByKeyOrDefault(settings, "query_text"));
6064

61-
if (!settings.GetFeaturesExtractor().IsFinished()) {
62-
ythrow TBadArgumentException() << "Unknown property: " << settings.GetFeaturesExtractor().GetRemainedParamsString();
63-
}
65+
auto& features = settings.GetFeaturesExtractor();
66+
viewDesc.SetQueryText(features.Extract("query_text").value_or(""));
67+
ValidateOptions(features);
6468
}
6569

6670
void FillDropViewProposal(NKikimrSchemeOp::TModifyScheme& modifyScheme,

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

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,20 @@ TString ReadWholeFile(const TString& path) {
5656
return file.ReadAll();
5757
}
5858

59+
NQuery::TExecuteQueryResult ExecuteQuery(NQuery::TSession& session, const TString& query) {
60+
const auto result = session.ExecuteQuery(
61+
query,
62+
NQuery::TTxControl::NoTx()
63+
).ExtractValueSync();
64+
65+
UNIT_ASSERT_C(result.IsSuccess(),
66+
"Failed to execute the following query:\n" << query << '\n'
67+
<< "The issues:\n" << result.GetIssues().ToString()
68+
);
69+
70+
return result;
71+
}
72+
5973
void ExecuteDataDefinitionQuery(TSession& session, const TString& script) {
6074
const auto result = session.ExecuteSchemeQuery(script).ExtractValueSync();
6175
UNIT_ASSERT_C(result.IsSuccess(), "Failed to execute the following DDL script:\n"
@@ -186,6 +200,90 @@ Y_UNIT_TEST_SUITE(TKQPViewTest) {
186200
UNIT_ASSERT_STRING_CONTAINS(creationResult.GetIssues().ToString(), "Error: Cannot divide type String and String");
187201
}
188202

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

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -587,7 +587,7 @@ alter_external_data_source_action:
587587
drop_external_data_source_stmt: DROP EXTERNAL DATA SOURCE (IF EXISTS)? object_ref;
588588

589589
create_view_stmt: CREATE VIEW object_ref
590-
with_table_settings
590+
create_object_features?
591591
AS select_stmt
592592
;
593593

@@ -615,7 +615,7 @@ drop_object_stmt: DROP OBJECT (IF EXISTS)? object_ref
615615
;
616616
drop_object_features: WITH object_features;
617617

618-
object_feature_value: id_or_type | bind_parameter | STRING_VALUE;
618+
object_feature_value: id_or_type | bind_parameter | STRING_VALUE | bool_value;
619619
object_feature_kv: an_id_or_type EQUALS object_feature_value;
620620
object_feature_flag: an_id_or_type;
621621
object_feature: object_feature_kv | object_feature_flag;

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

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1104,8 +1104,14 @@ bool TSqlQuery::Statement(TVector<TNodePtr>& blocks, const TRule_sql_stmt_core&
11041104
}
11051105

11061106
std::map<TString, TDeferredAtom> features;
1107-
ParseViewOptions(features, node.GetRule_with_table_settings4());
1108-
ParseViewQuery(features, node.GetRule_select_stmt6());
1107+
if (node.HasBlock4()) {
1108+
if (!ParseObjectFeatures(features, node.GetBlock4().GetRule_create_object_features1().GetRule_object_features2())) {
1109+
return false;
1110+
}
1111+
}
1112+
if (!ParseViewQuery(features, node.GetRule_select_stmt6())) {
1113+
return false;
1114+
}
11091115

11101116
const TString objectId = Id(node.GetRule_object_ref3().GetRule_id_or_at2(), *this).second;
11111117
constexpr const char* TypeId = "VIEW";

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

Lines changed: 8 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ TString CollectTokens(const TRule_select_stmt& selectStatement) {
5555

5656
NSQLTranslation::TTranslationSettings CreateViewTranslationSettings(const NSQLTranslation::TTranslationSettings& base) {
5757
NSQLTranslation::TTranslationSettings settings;
58-
58+
5959
settings.ClusterMapping = base.ClusterMapping;
6060
settings.Mode = NSQLTranslation::ESqlMode::LIMITED_VIEW;
6161

@@ -1596,16 +1596,6 @@ namespace {
15961596
return true;
15971597
}
15981598

1599-
bool StoreBool(const TRule_table_setting_value& from, TDeferredAtom& to, TContext& ctx) {
1600-
if (!from.HasAlt_table_setting_value6()) {
1601-
return false;
1602-
}
1603-
// bool_value
1604-
const TString value = to_lower(ctx.Token(from.GetAlt_table_setting_value6().GetRule_bool_value1().GetToken1()));
1605-
to = TDeferredAtom(BuildLiteralBool(ctx.Pos(), FromString<bool>(value)), ctx);
1606-
return true;
1607-
}
1608-
16091599
bool StoreSplitBoundary(const TRule_literal_value_list& boundary, TVector<TVector<TNodePtr>>& to,
16101600
TSqlExpression& expr, TContext& ctx) {
16111601
TVector<TNodePtr> boundaryKeys;
@@ -1732,26 +1722,6 @@ namespace {
17321722
return true;
17331723
}
17341724

1735-
bool StoreViewOptionsEntry(const TIdentifier& id,
1736-
const TRule_table_setting_value& value,
1737-
std::map<TString, TDeferredAtom>& features,
1738-
TContext& ctx) {
1739-
const auto name = to_lower(id.Name);
1740-
const auto publicName = to_upper(name);
1741-
1742-
if (features.find(name) != features.end()) {
1743-
ctx.Error(ctx.Pos()) << publicName << " is a duplicate";
1744-
return false;
1745-
}
1746-
1747-
if (!StoreBool(value, features[name], ctx)) {
1748-
ctx.Error(ctx.Pos()) << "Value of " << publicName << " must be a bool";
1749-
return false;
1750-
}
1751-
1752-
return true;
1753-
}
1754-
17551725
template<typename TChar>
17561726
struct TPatternComponent {
17571727
TBasicString<TChar> Prefix;
@@ -4268,7 +4238,7 @@ bool TSqlTranslation::BindParameterClause(const TRule_bind_parameter& node, TDef
42684238
}
42694239

42704240
bool TSqlTranslation::ObjectFeatureValueClause(const TRule_object_feature_value& node, TDeferredAtom& result) {
4271-
// object_feature_value: an_id_or_type | bind_parameter;
4241+
// object_feature_value: id_or_type | bind_parameter | STRING_VALUE | bool_value;
42724242
switch (node.Alt_case()) {
42734243
case TRule_object_feature_value::kAltObjectFeatureValue1:
42744244
{
@@ -4293,6 +4263,12 @@ bool TSqlTranslation::ObjectFeatureValueClause(const TRule_object_feature_value&
42934263
result = TDeferredAtom(Ctx.Pos(), strValue->Content);
42944264
break;
42954265
}
4266+
case TRule_object_feature_value::kAltObjectFeatureValue4:
4267+
{
4268+
TString value = Ctx.Token(node.GetAlt_object_feature_value4().GetRule_bool_value1().GetToken1());
4269+
result = TDeferredAtom(BuildLiteralBool(Ctx.Pos(), FromString<bool>(value)), Ctx);
4270+
break;
4271+
}
42964272
case TRule_object_feature_value::ALT_NOT_SET:
42974273
Y_ABORT("You should change implementation according to grammar changes");
42984274
}
@@ -4480,32 +4456,6 @@ bool TSqlTranslation::ValidateExternalTable(const TCreateTableParameters& params
44804456
return true;
44814457
}
44824458

4483-
bool TSqlTranslation::ParseViewOptions(std::map<TString, TDeferredAtom>& features,
4484-
const TRule_with_table_settings& options) {
4485-
const auto& firstEntry = options.GetRule_table_settings_entry3();
4486-
if (!StoreViewOptionsEntry(IdEx(firstEntry.GetRule_an_id1(), *this),
4487-
firstEntry.GetRule_table_setting_value3(),
4488-
features,
4489-
Ctx)) {
4490-
return false;
4491-
}
4492-
for (const auto& block : options.GetBlock4()) {
4493-
const auto& entry = block.GetRule_table_settings_entry2();
4494-
if (!StoreViewOptionsEntry(IdEx(entry.GetRule_an_id1(), *this),
4495-
entry.GetRule_table_setting_value3(),
4496-
features,
4497-
Ctx)) {
4498-
return false;
4499-
}
4500-
}
4501-
if (const auto securityInvoker = features.find("security_invoker");
4502-
securityInvoker == features.end() || securityInvoker->second.Build()->GetLiteralValue() != "true") {
4503-
Ctx.Error(Ctx.Pos()) << "SECURITY_INVOKER option must be explicitly enabled";
4504-
return false;
4505-
}
4506-
return true;
4507-
}
4508-
45094459
bool TSqlTranslation::ParseViewQuery(std::map<TString, TDeferredAtom>& features,
45104460
const TRule_select_stmt& query) {
45114461
const TString queryText = CollectTokens(query);

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

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

6333-
Y_UNIT_TEST(CreateViewNoSecurityInvoker) {
6334-
NYql::TAstParseResult res = SqlToYql(R"(
6335-
USE plato;
6336-
CREATE VIEW TheView AS SELECT 1;
6337-
)"
6338-
);
6339-
UNIT_ASSERT_STRING_CONTAINS(res.Issues.ToString(), "Unexpected token 'AS' : syntax error");
6340-
}
6341-
6342-
Y_UNIT_TEST(CreateViewSecurityInvokerTurnedOff) {
6343-
NYql::TAstParseResult res = SqlToYql(R"(
6344-
USE plato;
6345-
CREATE VIEW TheView WITH (security_invoker = FALSE) AS SELECT 1;
6346-
)"
6347-
);
6348-
UNIT_ASSERT_STRING_CONTAINS(res.Issues.ToString(), "SECURITY_INVOKER option must be explicitly enabled");
6349-
}
6350-
63516333
Y_UNIT_TEST(CreateViewFromTable) {
63526334
constexpr const char* path = "/PathPrefix/TheView";
63536335
constexpr const char* query = R"(

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

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -60,9 +60,11 @@ class TObjectSettingsImpl {
6060
if (auto maybeAtom = i.template Maybe<NYql::NNodes::TCoAtom>()) {
6161
Features.emplace(maybeAtom.Cast().StringValue(), "");
6262
} else if (auto maybeTuple = i.template Maybe<NNodes::TCoNameValueTuple>()) {
63-
auto tuple = maybeTuple.Cast();
64-
if (auto tupleValue = tuple.Value().template Maybe<NNodes::TCoAtom>()) {
65-
Features.emplace(tuple.Name().Value(), tupleValue.Cast().Value());
63+
NNodes::TCoNameValueTuple tuple = maybeTuple.Cast();
64+
if (auto maybeAtom = tuple.Value().template Maybe<NNodes::TCoAtom>()) {
65+
Features.emplace(tuple.Name().Value(), maybeAtom.Cast().Value());
66+
} else if (auto maybeBool = tuple.Value().template Maybe<NNodes::TCoBool>()) {
67+
Features.emplace(tuple.Name().Value(), maybeBool.Cast().Literal().Cast<NNodes::TCoAtom>().Value());
6668
}
6769
}
6870
}

0 commit comments

Comments
 (0)