Add pg_catalog.has_table_privilege function #15965

jeeminso · 2024-05-07T08:37:49Z

Summary of the changes / Why this improves CrateDB

Resolves #15748.

Initially discussed to go by extending NodeContext with ClusterState but since system tables are not available in cluster state, went by extending NodeContext with Schemas.

Checklist

Added an entry in the latest docs/appendices/release-notes/<x.y.0>.rst for user facing changes
Updated documentation & sql_features table for user facing changes
Touched code is covered by tests
CLA is signed
This does not contain breaking changes, or if it does:
- It is released within a major release
- It is recorded in the latest docs/appendices/release-notes/<x.y.0>.rst
- It was marked as deprecated in an earlier release if possible
- You've thought about the consequences and other components are adapted
  (E.g. AdminUI)

jeeminso · 2024-05-10T09:18:30Z

A bug:

cr> select pg_catalog.has_schema_privilege('crate', 'unknown_schema', 'usage');                                                                                       
+------+
| true |
+------+
| TRUE | --> an unknown schema exception should be thrown
+------+
SELECT 1 row in set (0.002 sec)

This causes test failures that were about to be added:

    @Test
     public void test_user_with_cluster_dql_do_not_have_privilege_on_unknown_table() {
         sqlExpressions = new SqlExpressions(tableSources, null, TEST_USER_WITH_DQL_ON_CLUSTER, List.of(), null);
         assertEvaluate("has_table_privilege('testUserWithClusterDQL', 'sys.unknown_table', 'USAGE')", false);
     }

     @Test
     public void test_user_with_sys_schema_dql_do_not_have_privilege_on_unknown_table_in_sys_schema() {
         sqlExpressions = new SqlExpressions(tableSources, null, TEST_USER_WITH_SYS_DQL, List.of(), null);
         assertEvaluate("has_table_privilege('testUserWithSysDQL', 'sys.unknown_table', 'USAGE')", false);
     }

In order to fix the bug, we need to use Schemas to validate whether the given schema exists from HasPrivilegeFunction.evaluate hence this PR will be merged first(which will introduce Schemas to the function).

update: these tests actually tests RolePrivileges.matchPrivilege, not relevant here.

server/src/main/java/io/crate/expression/scalar/HasTablePrivilegeFunction.java

jeeminso · 2024-05-13T09:17:11Z

Could you review this PR @mfussenegger ?

I thought it was safe to have the nodeContext instances without Schemas by looking at their usages.
looking back at the integration tests, the main purpose is to test Schemas.oidToName. I think I can move them to SchemasTest and explicitly test oidToName with different types of relations.

mfussenegger · 2024-05-13T09:22:28Z

libs/shared/src/main/java/io/crate/common/FiveFunction.java

+ * @param <R> the return type
+ */
+@FunctionalInterface
+public interface FiveFunction<A, B, C, D, E, R> {


I'd tend to instead create a concrete interface - getting the impression that this is taking the generic approach a bit too far.

mfussenegger · 2024-05-13T09:23:32Z

server/src/main/java/io/crate/expression/scalar/HasTablePrivilegeFunction.java

+
+    public static final FunctionName NAME = new FunctionName(PgCatalogSchemaInfo.NAME, "has_table_privilege");
+
+    private static final FiveFunction<Roles, Role, Object, Collection<Permission>, Provider<Schemas>, Boolean> CHECK_BY_TABLE_NAME =


Can we make these regular static functions, and then use method refs instead?

In general this has-privilege stuff is a bit odd given the mix of inheritance and composition. Wouldn't it be clearer to use either one approach instead of both?

I agree, I thinkg that composition is the way to go, (taking into account those four/fiveFunction ifaces)

mfussenegger · 2024-05-13T09:24:27Z

server/src/main/java/io/crate/metadata/NodeContext.java

@@ -31,12 +33,23 @@ public class NodeContext {
    private final Functions functions;
    private final long serverStartTimeInMs;
    private final Roles roles;
+    private final Provider<Schemas> schemasProvider;
+
+    public static NodeContext withoutSchemas(Functions functions, Roles roles) {


I think allowing schemas to be absent will make it a lot harder to reason about - as you'll need to know what kind of NodeContext instance you've got, and if you can access the schema.

We could avoid this by by making it mandatory.
One way to do that would be to move away from guice for the Schemas creation, see de8ce16

Note that tests are broken on the commit, they fail with:

Could not find a suitable constructor in io.crate.metadata.Schemas. Classes must have either one (and only one) constructor annotated with @Inject or a zero-argument constructor that is not private. at io.crate.metadata.Schemas.class(Unknown Source) at _unknown_

You'd also have to go through all classes that get Schemas injected in the constructor and replace it with NodeContext and lazy retrieve Schemas on use.

An alternative approach could be to try to break the cyclic dependency, by removing/adapting the dependencies of the Schemas class. E.g. maybe it could take Provider<DocSchemaInfoFactory>

Thank you, continuing on your first approach #15988.

I'd tend to agree with @mfussenegger.
That we're currently using different NodeContext instances already is in my opinion already a bad design, and instead of moving this further I'd prefer to get rid of this. This looks cumbersome to me, even when protecting fishy calls by the exception.

Related to the suggestions of getting rid of the Schema injections. Wouldn't it even possible to get rid of injection in the related classes at all instead of moving the resolving from directly binding Schemas to resolving it over the bound NodeContext? Not sure if this is feasible (related to time spent on this feature), but as I think our overall goal is in this direction, maybe worth doing.

Thanks for your input @seut , I actually needed to make more related classes away from injection in order to fix test failures for #15988. Please check the PR once it is ready as I am not sure if I understood your suggestion fully.

mfussenegger · 2024-05-13T09:27:41Z

server/src/main/java/io/crate/metadata/Schemas.java

@@ -513,4 +515,32 @@ public boolean viewExists(RelationName relationName) {
        ViewsMetadata views = clusterService.state().metadata().custom(ViewsMetadata.TYPE);
        return views != null && views.getView(relationName) != null;
    }
+
+    public String oidToName(int oid) {


Suggested change

public String oidToName(int oid) {

@Nullable

public RelationName getRelation(int oid) {

I'd make this specific to tables and return a RelationName, not a string to make it more general useful.

The schema lookup is afaik currently also not needed?

matriv

Thank you for working on this @jeeminso!
I've left some comments and most importantly, to use the correct permission types for has_table_privilege and not those for the schema.

Please also add docs and changelog entry.

server/src/main/java/io/crate/expression/scalar/HasTablePrivilegeFunction.java

server/src/main/java/io/crate/expression/scalar/HasPrivilegeFunction.java

server/src/main/java/io/crate/expression/scalar/HasTablePrivilegeFunction.java

server/src/main/java/io/crate/metadata/Schemas.java

jeeminso · 2024-05-23T18:19:50Z

Edge case:

cr> select pg_catalog.has_table_privilege('crate', 'sys.summits', 'insert');                                                                                                   
+------+
| true |
+------+
| TRUE |
+------+

matriv · 2024-05-23T19:10:10Z

Edge case:

cr> select pg_catalog.has_table_privilege('crate', 'sys.summits', 'insert');                                                                                                   
+------+
| true |
+------+
| TRUE |
+------+

Maybe we should check this on the Roles level, if the table is in sys,information_schema,pg_catalog we return false for DDL/DML, independently of what are the privileges assigned to a user, but I'd say we can deal with this as a followup.

matriv · 2024-05-23T19:16:13Z

Maybe we should check this on the Roles level, if the table is in sys,information_schema,pg_catalog we return false for DDL/DML, independently of what are the privileges assigned to a user, but I'd say we can deal with this as a followup.

On second thought, if we do that we are going to throw a different error message than we do now, when a user tries to insert/update/delete on those tables, so maybe we should only treat this for the has_table_privilege function, or even not at all, meaning that the user might have insert privs on a sys table but that doesn't mean it can indeed insert rows there.

jeeminso · 2024-05-23T19:49:43Z

Maybe we should check this on the Roles level, if the table is in sys,information_schema,pg_catalog we return false for DDL/DML, independently of what are the privileges assigned to a user, but I'd say we can deal with this as a followup.

On second thought, if we do that we are going to throw a different error message than we do now, when a user tries to insert/update/delete on those tables, so maybe we should only treat this for the has_table_privilege function, or even not at all, meaning that the user might have insert privs on a sys table but that doesn't mean it can indeed insert rows there.

We seem to have two concepts, operations and privileges, where the error messages you mentioned are from operation checks on the given table,

crate/server/src/main/java/io/crate/metadata/Schemas.java

Line 243 in ce58888

Operation.blockedRaiseException(relationInfo, operation);

so I suspect it could be doable without affecting other parts.

I think one way to handle this is to have the newly introduced Schemas.getRelation(oid) to accept Operation parameter like Schemas.findRelation with some sort of permission to operation mapping.

Anyways, as you said this could be continued as a follow up.

jeeminso · 2024-05-24T00:48:59Z

Thanks for reviewing @matriv please check again.

matriv

thank you! left some more minor comments.

docs/general/builtins/scalar-functions.rst

server/src/test/java/io/crate/expression/scalar/HasTablePrivilegeFunctionTest.java

server/src/test/java/io/crate/integrationtests/HasTablePrivilegeFunctionIntegrationTest.java

matriv · 2024-05-24T07:50:43Z

Maybe we should check this on the Roles level, if the table is in sys,information_schema,pg_catalog we return false for DDL/DML, independently of what are the privileges assigned to a user, but I'd say we can deal with this as a followup.

On second thought, if we do that we are going to throw a different error message than we do now, when a user tries to insert/update/delete on those tables, so maybe we should only treat this for the has_table_privilege function, or even not at all, meaning that the user might have insert privs on a sys table but that doesn't mean it can indeed insert rows there.

We seem to have two concepts, operations and privileges, where the error messages you mentioned are from operation checks on the given table,

crate/server/src/main/java/io/crate/metadata/Schemas.java

Line 243 in ce58888

Operation.blockedRaiseException(relationInfo, operation);

so I suspect it could be doable without affecting other parts.
I think one way to handle this is to have the newly introduced Schemas.getRelation(oid) to accept Operation parameter like Schemas.findRelation with some sort of permission to operation mapping.

Anyways, as you said this could be continued as a follow up.

In postgres if you grant the permission on a static table you get a true returned, so we don't need to do anything, privileges and operation related restrictions are orthogonal:

matriv=> select has_table_privilege('pg_roles', 'insert');
 has_table_privilege
---------------------
 t
(1 row)

jeeminso force-pushed the jeeminso/table-privilege branch 5 times, most recently from 51e79d2 to b352520 Compare May 8, 2024 14:17

jeeminso commented May 10, 2024

View reviewed changes

server/src/main/java/io/crate/expression/scalar/HasTablePrivilegeFunction.java Outdated Show resolved Hide resolved

jeeminso requested a review from mfussenegger May 13, 2024 09:17

mfussenegger reviewed May 13, 2024

View reviewed changes

jeeminso force-pushed the jeeminso/table-privilege branch 7 times, most recently from be9361b to b849b7e Compare May 22, 2024 23:55

jeeminso marked this pull request as ready for review May 23, 2024 13:57

jeeminso requested a review from matriv May 23, 2024 14:25

matriv reviewed May 23, 2024

View reviewed changes

jeeminso force-pushed the jeeminso/table-privilege branch 2 times, most recently from 158f303 to 4c6486d Compare May 24, 2024 00:48

jeeminso requested a review from matriv May 24, 2024 00:49

matriv approved these changes May 24, 2024

View reviewed changes

Add pg_catalog.has_table_privilege function

ed19d37

jeeminso force-pushed the jeeminso/table-privilege branch from 4c6486d to ed19d37 Compare May 24, 2024 12:19

jeeminso added the ready-to-merge Let Mergify merge the PR once approved and checks pass label May 24, 2024

mergify bot merged commit 0dcd1a0 into master May 24, 2024
17 checks passed

mergify bot deleted the jeeminso/table-privilege branch May 24, 2024 12:46

jeeminso mentioned this pull request May 24, 2024

Implement pg_catalog.has_table_privilege #15748

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pg_catalog.has_table_privilege function #15965

Add pg_catalog.has_table_privilege function #15965

jeeminso commented May 7, 2024 •

edited

jeeminso commented May 10, 2024 •

edited

jeeminso commented May 13, 2024 •

edited

mfussenegger May 13, 2024

mfussenegger May 13, 2024

mfussenegger May 13, 2024

matriv May 15, 2024

mfussenegger May 13, 2024

jeeminso May 14, 2024

seut May 15, 2024

jeeminso May 17, 2024

mfussenegger May 13, 2024

matriv left a comment

jeeminso commented May 23, 2024 •

edited

matriv commented May 23, 2024

matriv commented May 23, 2024

jeeminso commented May 23, 2024

jeeminso commented May 24, 2024

matriv left a comment

matriv commented May 24, 2024


		public static final FunctionName NAME = new FunctionName(PgCatalogSchemaInfo.NAME, "has_table_privilege");

		private static final FiveFunction<Roles, Role, Object, Collection<Permission>, Provider<Schemas>, Boolean> CHECK_BY_TABLE_NAME =

	public String oidToName(int oid) {
	@Nullable
	public RelationName getRelation(int oid) {

Add pg_catalog.has_table_privilege function #15965

Add pg_catalog.has_table_privilege function #15965

Conversation

jeeminso commented May 7, 2024 • edited

Summary of the changes / Why this improves CrateDB

Checklist

jeeminso commented May 10, 2024 • edited

jeeminso commented May 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matriv left a comment

Choose a reason for hiding this comment

jeeminso commented May 23, 2024 • edited

matriv commented May 23, 2024

matriv commented May 23, 2024

jeeminso commented May 23, 2024

jeeminso commented May 24, 2024

matriv left a comment

Choose a reason for hiding this comment

matriv commented May 24, 2024

jeeminso commented May 7, 2024 •

edited

jeeminso commented May 10, 2024 •

edited

jeeminso commented May 13, 2024 •

edited

jeeminso commented May 23, 2024 •

edited