Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement pg_catalog.has_table_privilege #15748

Closed
hlcianfagna opened this issue Mar 22, 2024 · 7 comments · Fixed by #15965
Closed

Implement pg_catalog.has_table_privilege #15748

hlcianfagna opened this issue Mar 22, 2024 · 7 comments · Fixed by #15965

Comments

@hlcianfagna
Copy link
Contributor

Problem Statement

Some tools (see labels) rely on this function.

Possible Solutions

Implement it as defined in https://www.postgresql.org/docs/7.3/functions-misc.html
Please note this function has 2 signatures.
Theobald Software (Xtract Universal) uses (table, access)
Metabase uses (user, table, access)

Considered Alternatives

Mock them with:

CREATE FUNCTION pg_catalog.has_table_privilege("user" TEXT,"table" TEXT,"access" TEXT)
RETURNS BOOLEAN LANGUAGE JAVASCRIPT AS 'function has_table_privilege(user,table,access){return true;}';

CREATE FUNCTION pg_catalog.has_table_privilege("table" TEXT,"access" TEXT)
RETURNS BOOLEAN LANGUAGE JAVASCRIPT AS 'function has_table_privilege(table,access){return true;}';
@matriv
Copy link
Contributor

matriv commented Apr 9, 2024

I've tried out to implement this, and the issue I'm facing is the overload of the function where the user passes tableOid instead of a table, view, foreign table name.
The way we handle oids for schemas is that we use the following code:

Policy matchSchema(Permission permission, int oid) {
        Policy result = Policy.REVOKE;
        for (var privilege : privileges) {
            Subject ident = privilege.subject();
            if (ident.permission() != permission) {
                continue;
            }
            if (ident.securable() == SCHEMA && OidHash.schemaOid(ident.ident()) == oid) {
                return privilege.policy();
            }
            if (ident.securable() == CLUSTER) {
                result = privilege.policy();
            }
        }
        return result;
    }

So we create the oid for every schema in the granted (or denied) privileges of the user and we try to match it against the oid passed as argument to the has_schema_privilege function.
This doesn't work for table oid, because to create a table oid you need the RelationInfo, and not a simple ident string, because it needs to know the type of the table (table, view, foreign table).
To retrieve this we need to use Schemas.findRelation(), which mean we need to pass Schemas to the HasTablePrivilegeFunction and also to the whole hierarchy of those functions.
Additionally, to call Schemas.findRelation() we need to construct a QualifiedName (easy, just split the privilege subject on ".") but we also need a SearchPath, which means we also need to pass down to all
all the calls from HashTablePrivilegeFunction.

  • Maybe I missing something for this ^^?
  • Maybe we should just not support the oid overload for now,
  • Change the way we crate oids for tables? why do we need the type to generate the oid? we cannot have a clash of FQNs, right? (same view and table or foreign table under the same schema). Did a quick test to remove the type from the oid generation and the only think that needs to adjust is the expected oid in varius pg_catalog related tests.

@jeeminso
Copy link
Contributor

jeeminso commented May 1, 2024

Less performant but another option could be to regenerate oids for all existing tables in order to find out the oid -> table mapping.

@matriv
Copy link
Contributor

matriv commented May 1, 2024

I don't get what you mean, the hash function is one way. We need to iterate over the table privileges of the user, and for every table listed in the user privileges, produce the oid, and compare it to the oid passed to the hasTablePrivileges() function.

@jeeminso jeeminso self-assigned this May 3, 2024
@jeeminso
Copy link
Contributor

jeeminso commented May 3, 2024

Since you mentioned that the problem was with the user providing oid instead of names, I thought we could implement something like

    public String oidToName(int oid) {
        for (SchemaInfo schema : this) {
            if (oid == OidHash.schemaOid(schema.name())) {
                return schema.name();
            }
            for (RelationInfo relation : schema.getTables()) {
                if (oid == OidHash.relationOid(relation)) {
                    return relation.ident().sqlFqn();
                }
            }
        }
        return null;
    }

Maybe we should just not support the oid overload for now

I also think we could consider reducing the scope.

@matriv
Copy link
Contributor

matriv commented May 6, 2024

the function should work for tables and views:

matriv=> create table t(a int);
CREATE TABLE
matriv=> create view v as select * from t;
CREATE VIEW
matriv=> select has_table_privilege('v', 'SELECT');
 has_table_privilege 
---------------------
 t
(1 row)

So if an oid is passed we need to somehow determine to which table or view matches.
To do that we need to iterate over all available tables and views in the ClusterState, produce the oid for each one of them and check if it matches the provided oid (and break of course if match is found).
Then with the FQN of the table/view at hand, we can just call Roles#hasPrivilege() with that FQN (which will check all the applicable permission for the user and the FQN, he might have privileges for Cluster, or for schema and not explicitly for the table)

@matriv
Copy link
Contributor

matriv commented May 6, 2024

Going over just the privileges of the user, won't work as the oid passed would be of a table (or view) so if the user has privs for the schema of that table (or view) the provided oid won't match anything in the user's privileges, and we will return wrong result.

@jeeminso
Copy link
Contributor

It's implemented by #15965, Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants