From 44a7699dd367e1617604fbdd41285dc2560e4ae4 Mon Sep 17 00:00:00 2001 From: Daniil Demin Date: Thu, 19 Sep 2024 16:26:29 +0300 Subject: [PATCH] Views: throw a human-readable error in case of a missing `WITH (security_invoker = true)` (#9320) --- .../kqp/gateway/behaviour/view/manager.cpp | 25 +++--- ydb/core/kqp/ut/view/view_ut.cpp | 84 +++++++++++++++++++ ydb/library/yql/sql/v1/SQLv1.g.in | 4 +- ydb/library/yql/sql/v1/sql_query.cpp | 6 +- ydb/library/yql/sql/v1/sql_translation.cpp | 64 ++------------ ydb/library/yql/sql/v1/sql_ut.cpp | 18 ---- ydb/services/metadata/abstract/parsing.h | 6 +- 7 files changed, 116 insertions(+), 91 deletions(-) diff --git a/ydb/core/kqp/gateway/behaviour/view/manager.cpp b/ydb/core/kqp/gateway/behaviour/view/manager.cpp index fa052618ff91..069697d53baf 100644 --- a/ydb/core/kqp/gateway/behaviour/view/manager.cpp +++ b/ydb/core/kqp/gateway/behaviour/view/manager.cpp @@ -14,11 +14,6 @@ using TYqlConclusionStatus = TViewManager::TYqlConclusionStatus; using TInternalModificationContext = TViewManager::TInternalModificationContext; using TExternalModificationContext = TViewManager::TExternalModificationContext; -TString GetByKeyOrDefault(const NYql::TCreateObjectSettings& container, const TString& key) { - const auto value = container.GetFeaturesExtractor().Extract(key); - return value ? *value : TString{}; -} - TYqlConclusionStatus CheckFeatureFlag(const TInternalModificationContext& context) { auto* const actorSystem = context.GetExternalData().GetActorSystem(); if (!actorSystem) { @@ -48,6 +43,16 @@ std::pair SplitPathByObjectId(const TString& objectId) { return pathPair; } +void ValidateOptions(NYql::TFeaturesExtractor& features) { + // Current implementation does not persist the security_invoker option value. + if (features.Extract("security_invoker") != "true") { + ythrow TBadArgumentException() << "security_invoker option must be explicitly enabled"; + } + if (!features.IsFinished()) { + ythrow TBadArgumentException() << "Unknown property: " << features.GetRemainedParamsString(); + } +} + void FillCreateViewProposal(NKikimrSchemeOp::TModifyScheme& modifyScheme, const NYql::TCreateObjectSettings& settings, const TExternalModificationContext& context) { @@ -58,12 +63,12 @@ void FillCreateViewProposal(NKikimrSchemeOp::TModifyScheme& modifyScheme, auto& viewDesc = *modifyScheme.MutableCreateView(); viewDesc.SetName(pathPair.second); - viewDesc.SetQueryText(GetByKeyOrDefault(settings, "query_text")); - NSQLTranslation::Serialize(context.GetTranslationSettings(), *viewDesc.MutableCapturedContext()); - if (!settings.GetFeaturesExtractor().IsFinished()) { - ythrow TBadArgumentException() << "Unknown property: " << settings.GetFeaturesExtractor().GetRemainedParamsString(); - } + auto& features = settings.GetFeaturesExtractor(); + viewDesc.SetQueryText(features.Extract("query_text").value_or("")); + ValidateOptions(features); + + NSQLTranslation::Serialize(context.GetTranslationSettings(), *viewDesc.MutableCapturedContext()); } void FillDropViewProposal(NKikimrSchemeOp::TModifyScheme& modifyScheme, diff --git a/ydb/core/kqp/ut/view/view_ut.cpp b/ydb/core/kqp/ut/view/view_ut.cpp index 05002dd05ed0..111c2faee84f 100644 --- a/ydb/core/kqp/ut/view/view_ut.cpp +++ b/ydb/core/kqp/ut/view/view_ut.cpp @@ -213,6 +213,90 @@ Y_UNIT_TEST_SUITE(TCreateAndDropViewTest) { UNIT_ASSERT_STRING_CONTAINS(creationResult.GetIssues().ToString(), "Error: Cannot divide type String and String"); } + Y_UNIT_TEST(ParsingSecurityInvoker) { + TKikimrRunner kikimr(TKikimrSettings().SetWithSampleTables(false)); + EnableViewsFeatureFlag(kikimr); + auto session = kikimr.GetQueryClient().GetSession().ExtractValueSync().GetSession(); + + constexpr const char* path = "TheView"; + constexpr const char* query = "SELECT 1"; + + auto fail = [&](const char* options) { + const TString creationQuery = std::format(R"( + CREATE VIEW {} {} AS {}; + )", + path, + options, + query + ); + + const auto creationResult = session.ExecuteQuery( + creationQuery, + NQuery::TTxControl::NoTx() + ).ExtractValueSync(); + + UNIT_ASSERT_C(!creationResult.IsSuccess(), creationQuery); + UNIT_ASSERT_STRING_CONTAINS( + creationResult.GetIssues().ToString(), "security_invoker option must be explicitly enabled" + ); + }; + fail(""); + fail("WITH security_invoker"); + fail("WITH security_invoker = false"); + fail("WITH SECURITY_INVOKER = true"); // option name is case-sensitive + fail("WITH (security_invoker)"); + fail("WITH (security_invoker = false)"); + fail("WITH (security_invoker = true, security_invoker = false)"); + + auto succeed = [&](const char* options) { + const TString creationQuery = std::format(R"( + CREATE VIEW {} {} AS {}; + DROP VIEW {}; + )", + path, + options, + query, + path + ); + ExecuteQuery(session, creationQuery); + }; + succeed("WITH security_invoker = true"); + succeed("WITH (security_invoker = true)"); + succeed("WITH (security_invoker = tRuE)"); // bool parsing is flexible enough + succeed("WITH (security_invoker = false, security_invoker = true)"); + + { + // literal named expression + const TString creationQuery = std::format(R"( + $value = "true"; + CREATE VIEW {} WITH security_invoker = $value AS {}; + DROP VIEW {}; + )", + path, + query, + path + ); + ExecuteQuery(session, creationQuery); + } + { + // evaluated expression + const TString creationQuery = std::format(R"( + $lambda = ($x) -> {{ + RETURN CAST($x as String) + }}; + $value = $lambda(true); + + CREATE VIEW {} WITH security_invoker = $value AS {}; + DROP VIEW {}; + )", + path, + query, + path + ); + ExecuteQuery(session, creationQuery); + } + } + Y_UNIT_TEST(ListCreatedView) { TKikimrRunner kikimr(TKikimrSettings().SetWithSampleTables(false)); EnableViewsFeatureFlag(kikimr); diff --git a/ydb/library/yql/sql/v1/SQLv1.g.in b/ydb/library/yql/sql/v1/SQLv1.g.in index a71ee4275864..d980276162a5 100644 --- a/ydb/library/yql/sql/v1/SQLv1.g.in +++ b/ydb/library/yql/sql/v1/SQLv1.g.in @@ -593,7 +593,7 @@ alter_external_data_source_action: drop_external_data_source_stmt: DROP EXTERNAL DATA SOURCE (IF EXISTS)? object_ref; create_view_stmt: CREATE VIEW object_ref - with_table_settings + create_object_features? AS select_stmt ; @@ -621,7 +621,7 @@ drop_object_stmt: DROP OBJECT (IF EXISTS)? object_ref ; drop_object_features: WITH object_features; -object_feature_value: id_or_type | bind_parameter | STRING_VALUE; +object_feature_value: id_or_type | bind_parameter | STRING_VALUE | bool_value; object_feature_kv: an_id_or_type EQUALS object_feature_value; object_feature_flag: an_id_or_type; object_feature: object_feature_kv | object_feature_flag; diff --git a/ydb/library/yql/sql/v1/sql_query.cpp b/ydb/library/yql/sql/v1/sql_query.cpp index e06a4b39cc1d..bef0f01d46c1 100644 --- a/ydb/library/yql/sql/v1/sql_query.cpp +++ b/ydb/library/yql/sql/v1/sql_query.cpp @@ -1233,8 +1233,10 @@ bool TSqlQuery::Statement(TVector& blocks, const TRule_sql_stmt_core& } std::map features; - if (!ParseViewOptions(features, node.GetRule_with_table_settings4())) { - return false; + if (node.HasBlock4()) { + if (!ParseObjectFeatures(features, node.GetBlock4().GetRule_create_object_features1().GetRule_object_features2())) { + return false; + } } if (!ParseViewQuery(features, node.GetRule_select_stmt6())) { return false; diff --git a/ydb/library/yql/sql/v1/sql_translation.cpp b/ydb/library/yql/sql/v1/sql_translation.cpp index 9a79d1c07c53..cf5e73619828 100644 --- a/ydb/library/yql/sql/v1/sql_translation.cpp +++ b/ydb/library/yql/sql/v1/sql_translation.cpp @@ -1647,16 +1647,6 @@ namespace { return true; } - bool StoreBool(const TRule_table_setting_value& from, TDeferredAtom& to, TContext& ctx) { - if (!from.HasAlt_table_setting_value6()) { - return false; - } - // bool_value - const TString value = to_lower(ctx.Token(from.GetAlt_table_setting_value6().GetRule_bool_value1().GetToken1())); - to = TDeferredAtom(BuildLiteralBool(ctx.Pos(), FromString(value)), ctx); - return true; - } - bool StoreSplitBoundary(const TRule_literal_value_list& boundary, TVector>& to, TSqlExpression& expr, TContext& ctx) { TVector boundaryKeys; @@ -1783,26 +1773,6 @@ namespace { return true; } - bool StoreViewOptionsEntry(const TIdentifier& id, - const TRule_table_setting_value& value, - std::map& features, - TContext& ctx) { - const auto name = to_lower(id.Name); - const auto publicName = to_upper(name); - - if (features.find(name) != features.end()) { - ctx.Error(ctx.Pos()) << publicName << " is a duplicate"; - return false; - } - - if (!StoreBool(value, features[name], ctx)) { - ctx.Error(ctx.Pos()) << "Value of " << publicName << " must be a bool"; - return false; - } - - return true; - } - template struct TPatternComponent { TBasicString Prefix; @@ -4372,7 +4342,7 @@ bool TSqlTranslation::BindParameterClause(const TRule_bind_parameter& node, TDef } bool TSqlTranslation::ObjectFeatureValueClause(const TRule_object_feature_value& node, TDeferredAtom& result) { - // object_feature_value: an_id_or_type | bind_parameter; + // object_feature_value: id_or_type | bind_parameter | STRING_VALUE | bool_value; switch (node.Alt_case()) { case TRule_object_feature_value::kAltObjectFeatureValue1: { @@ -4397,6 +4367,12 @@ bool TSqlTranslation::ObjectFeatureValueClause(const TRule_object_feature_value& result = TDeferredAtom(Ctx.Pos(), strValue->Content); break; } + case TRule_object_feature_value::kAltObjectFeatureValue4: + { + TString value = Ctx.Token(node.GetAlt_object_feature_value4().GetRule_bool_value1().GetToken1()); + result = TDeferredAtom(BuildLiteralBool(Ctx.Pos(), FromString(value)), Ctx); + break; + } case TRule_object_feature_value::ALT_NOT_SET: Y_ABORT("You should change implementation according to grammar changes"); } @@ -4586,32 +4562,6 @@ bool TSqlTranslation::ValidateExternalTable(const TCreateTableParameters& params return true; } -bool TSqlTranslation::ParseViewOptions(std::map& features, - const TRule_with_table_settings& options) { - const auto& firstEntry = options.GetRule_table_settings_entry3(); - if (!StoreViewOptionsEntry(IdEx(firstEntry.GetRule_an_id1(), *this), - firstEntry.GetRule_table_setting_value3(), - features, - Ctx)) { - return false; - } - for (const auto& block : options.GetBlock4()) { - const auto& entry = block.GetRule_table_settings_entry2(); - if (!StoreViewOptionsEntry(IdEx(entry.GetRule_an_id1(), *this), - entry.GetRule_table_setting_value3(), - features, - Ctx)) { - return false; - } - } - if (const auto securityInvoker = features.find("security_invoker"); - securityInvoker == features.end() || securityInvoker->second.Build()->GetLiteralValue() != "true") { - Ctx.Error(Ctx.Pos()) << "SECURITY_INVOKER option must be explicitly enabled"; - return false; - } - return true; -} - bool TSqlTranslation::ParseViewQuery( std::map& features, const TRule_select_stmt& query diff --git a/ydb/library/yql/sql/v1/sql_ut.cpp b/ydb/library/yql/sql/v1/sql_ut.cpp index 90dae8d2ea58..c0b775ba3a69 100644 --- a/ydb/library/yql/sql/v1/sql_ut.cpp +++ b/ydb/library/yql/sql/v1/sql_ut.cpp @@ -6592,24 +6592,6 @@ Y_UNIT_TEST_SUITE(TViewSyntaxTest) { UNIT_ASSERT_C(res.Root, res.Issues.ToString()); } - Y_UNIT_TEST(CreateViewNoSecurityInvoker) { - NYql::TAstParseResult res = SqlToYql(R"( - USE plato; - CREATE VIEW TheView AS SELECT 1; - )" - ); - UNIT_ASSERT_STRING_CONTAINS(res.Issues.ToString(), "Unexpected token 'AS' : syntax error"); - } - - Y_UNIT_TEST(CreateViewSecurityInvokerTurnedOff) { - NYql::TAstParseResult res = SqlToYql(R"( - USE plato; - CREATE VIEW TheView WITH (security_invoker = FALSE) AS SELECT 1; - )" - ); - UNIT_ASSERT_STRING_CONTAINS(res.Issues.ToString(), "SECURITY_INVOKER option must be explicitly enabled"); - } - Y_UNIT_TEST(CreateViewFromTable) { constexpr const char* path = "/PathPrefix/TheView"; constexpr const char* query = R"( diff --git a/ydb/services/metadata/abstract/parsing.h b/ydb/services/metadata/abstract/parsing.h index 6dfe093be319..73c9f7507068 100644 --- a/ydb/services/metadata/abstract/parsing.h +++ b/ydb/services/metadata/abstract/parsing.h @@ -74,8 +74,10 @@ class TObjectSettingsImpl { NNodes::TCoNameValueTuple tuple = maybeTuple.Cast(); if (auto maybeAtom = tuple.Value().template Maybe()) { Features.emplace(tuple.Name().Value(), maybeAtom.Cast().Value()); - } else if (auto maybeDataCtor = tuple.Value().template Maybe()) { - Features.emplace(tuple.Name().Value(), maybeDataCtor.Cast().Literal().Cast().Value()); + } else if (auto maybeInt = tuple.Value().template Maybe()) { + Features.emplace(tuple.Name().Value(), maybeInt.Cast().Literal().Cast().Value()); + } else if (auto maybeBool = tuple.Value().template Maybe()) { + Features.emplace(tuple.Name().Value(), maybeBool.Cast().Literal().Cast().Value()); } } }