Change SQL path to match spec

[dain]

Description

Update the function resolution usage of SQL path to match the specification. The path is now stored in views, and view expression (for pushdown). If the view does not have a path then the only system.builtin is visible. Unauthorized SQL path element are now skipped in function resolution, so authorized functions later in the path are now considered. In order to make thes changes, function security APIs and implementations have been changed as follows

• No security checks are performed for functions in system.builtin. These functions are considered safe for all users and are typically required for the engine to function properly.
• Return boolean from catalog access check calls instead of throwing as this is not longer a fatal issues
• Return boolean from function check calls instead of throwing as this is not longer a fatal issues
• Merge simple function checks with table function check as functions draw from the same namespace
• Add canCreateViewWithExecuteFunction for use in ViewAccessControl which replaces the former checkCanGrant method
• The plugin loader now fails loading if a system or catalog access control defines any of the removed methods as a safety mechanism, so are required for any existing implementations.
• File based access control is not allowed to have a function kind element in rules, since it is not supported anymore
• Path is stored with view-like structures, and there are specialized methods on Session to create a new session for the view context.

Release notes

(x) Release notes are required, with the following suggested text:

# Security
* Security checks are no longer executed for functions in `system.builtin`. ({issue}`19160`)
* Function rules in file based access control no longer support function kind.  See documentation for more details.  ({issue}`19160`)

# SPI
* Function security checks now return a boolean instead of throwing an exception. ({issue}`19160`)
* Add SQL path field to `ConnectorViewDefinition`, `ConnectorMaterializedViewDefinition`, and `ViewExpression`. ({issue}`19160`)

[findepi]

Merge simple function checks with table function check as functions draw from the same namespace

I agree that they draw from the same namespace but that doesn't mean implementors want to have same logic.
For example, it's easy to imagine security checks that allow all scalars by default (because scalars are inherently safe due to how they operate on values processed by the query anyway), and disallow all table functions by default (as these are inherently not safe, especially the leaf table functions).

Can you advise how such a logic should be implemented?

Further, the change seems to introduce a side-effect which may be considered a security problem.
In previous Trino version, file-based AC required full catalog access to execute a table function (even if no function rules were defined).

trino/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/FileBasedSystemAccessControl.java

Lines 1103 to 1108 in 1f46063

	AccessMode requiredCatalogAccess = switch (functionKind) {
	case SCALAR, AGGREGATE, WINDOW -> READ_ONLY;
	case TABLE -> ALL;
	};
	Identity identity = context.getIdentity();
	return canAccessCatalog(context, functionName.getCatalogName(), requiredCatalogAccess) &&

Trino 429 lowers that to require read only access.
(This hopefully isn't a big deal given all currently existing builtin table function are read-only, but worth calling out as a breaking change in release notes for anyone that has custom table functions in their plugins)

[martint]

(because scalars are inherently safe due to how they operate on values processed by the query anyway), and disallow all table functions by default (as these are inherently not safe, especially the leaf table functions)

This assumption is incorrect. There’s nothing about scalar functions that makes them inherently safe vs table functions. The only difference between them is that one processes single values, while the other processes sequences of values. Both could very easily do unsafe things if they wanted to and their implementation allowed it (e.g., make external calls, read files, etc.)

In fact, one of the changes we’re planning in the future will allow scalar functions to be callable in batches, making them no different than a table function.

[okayhooni]

Hello @dain , is it intended to use delegate.canCreateViewWithExecuteFunction() instead of delegate.canExecuteFunction() here..? (I guess it is intended to use VIEW DEFINER permission by default)