Views: throw a human-readable error in case of a missing WITH (security_invoker = true) (#9320)

jepett0 · jepett0 · commit 091633aa6b27 · 2024-09-27T06:56:11.000Z
diff --git 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<TString, TString> 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
@@ -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
@@ -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
@@ -1233,8 +1233,10 @@ bool TSqlQuery::Statement(TVector<TNodePtr>& blocks, const TRule_sql_stmt_core&
             }
 
             std::map<TString, TDeferredAtom> 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
@@ -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<bool>(value)), ctx);
-        return true;
-    }
-
     bool StoreSplitBoundary(const TRule_literal_value_list& boundary, TVector<TVector<TNodePtr>>& to,
             TSqlExpression& expr, TContext& ctx) {
         TVector<TNodePtr> boundaryKeys;
@@ -1783,26 +1773,6 @@ namespace {
         return true;
     }
 
-    bool StoreViewOptionsEntry(const TIdentifier& id,
-                               const TRule_table_setting_value& value,
-                               std::map<TString, TDeferredAtom>& 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<typename TChar>
     struct TPatternComponent {
         TBasicString<TChar> Prefix;
@@ -4394,7 +4364,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:
         {
@@ -4419,6 +4389,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<bool>(value)), Ctx);
+            break;
+        }
         case TRule_object_feature_value::ALT_NOT_SET:
             Y_ABORT("You should change implementation according to grammar changes");
     }
@@ -4608,32 +4584,6 @@ bool TSqlTranslation::ValidateExternalTable(const TCreateTableParameters& params
     return true;
 }
 
-bool TSqlTranslation::ParseViewOptions(std::map<TString, TDeferredAtom>& 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<TString, TDeferredAtom>& 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
@@ -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
@@ -74,8 +74,10 @@ class TObjectSettingsImpl {
                 NNodes::TCoNameValueTuple tuple = maybeTuple.Cast();
                 if (auto maybeAtom = tuple.Value().template Maybe<NNodes::TCoAtom>()) {
                     Features.emplace(tuple.Name().Value(), maybeAtom.Cast().Value());
-                } else if (auto maybeDataCtor = tuple.Value().template Maybe<NNodes::TCoIntegralCtor>()) {
-                    Features.emplace(tuple.Name().Value(), maybeDataCtor.Cast().Literal().Cast<NNodes::TCoAtom>().Value());
+                } else if (auto maybeInt = tuple.Value().template Maybe<NNodes::TCoIntegralCtor>()) {
+                    Features.emplace(tuple.Name().Value(), maybeInt.Cast().Literal().Cast<NNodes::TCoAtom>().Value());
+                } else if (auto maybeBool = tuple.Value().template Maybe<NNodes::TCoBool>()) {
+                    Features.emplace(tuple.Name().Value(), maybeBool.Cast().Literal().Cast<NNodes::TCoAtom>().Value());
                 }
             }
         }

Original file line number	Diff line number	Diff line change
`@@ -593,7 +593,7 @@ alter_external_data_source_action:`
`593`	`593`	`drop_external_data_source_stmt: DROP EXTERNAL DATA SOURCE (IF EXISTS)? object_ref;`
`594`	`594`
`595`	`595`	`create_view_stmt: CREATE VIEW object_ref`
`596`		`- with_table_settings`
	`596`	`+ create_object_features?`
`597`	`597`	`AS select_stmt`
`598`	`598`	`;`
`599`	`599`
`@@ -621,7 +621,7 @@ drop_object_stmt: DROP OBJECT (IF EXISTS)? object_ref`
`621`	`621`	`;`
`622`	`622`	`drop_object_features: WITH object_features;`
`623`	`623`
`624`		`-object_feature_value: id_or_type \| bind_parameter \| STRING_VALUE;`
	`624`	`+object_feature_value: id_or_type \| bind_parameter \| STRING_VALUE \| bool_value;`
`625`	`625`	`object_feature_kv: an_id_or_type EQUALS object_feature_value;`
`626`	`626`	`object_feature_flag: an_id_or_type;`
`627`	`627`	`object_feature: object_feature_kv \| object_feature_flag;`
Original file line number	Diff line number	Diff line change
`@@ -1233,8 +1233,10 @@ bool TSqlQuery::Statement(TVector<TNodePtr>& blocks, const TRule_sql_stmt_core&`
`1233`	`1233`	`}`
`1234`	`1234`
`1235`	`1235`	`std::map<TString, TDeferredAtom> features;`
`1236`		`- if (!ParseViewOptions(features, node.GetRule_with_table_settings4())) {`
`1237`		`- return false;`
	`1236`	`+ if (node.HasBlock4()) {`
	`1237`	`+ if (!ParseObjectFeatures(features, node.GetBlock4().GetRule_create_object_features1().GetRule_object_features2())) {`
	`1238`	`+ return false;`
	`1239`	`+ }`
`1238`	`1240`	`}`
`1239`	`1241`	`if (!ParseViewQuery(features, node.GetRule_select_stmt6())) {`
`1240`	`1242`	`return false;`
Original file line number	Diff line number	Diff line change
`@@ -74,8 +74,10 @@ class TObjectSettingsImpl {`
`74`	`74`	`NNodes::TCoNameValueTuple tuple = maybeTuple.Cast();`
`75`	`75`	`if (auto maybeAtom = tuple.Value().template Maybe<NNodes::TCoAtom>()) {`
`76`	`76`	`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());`
`79`	`81`	`}`
`80`	`82`	`}`
`81`	`83`	`}`