cynkra / dm

Working with relational data models in R
https://dm.cynkra.com
Other
497 stars 50 forks source link

Learning of FK-constraint on Postgres fails for non-owner of the relation #1879

Open nbenn opened 1 year ago

nbenn commented 1 year ago

@krlmlr Could you maybe comment on

https://github.com/cynkra/dm/blob/64449a5fb19c4286eb73a8ac73ccaca54c015091/R/postgres.R#L53

This requires the role issuing the query to have USAGE privileges for the role that owns the relation for the corresponding FK-constraint to be picked up by dm_meta_raw(). Admittedly, I'm fairly new to the topic of DB access privileges, but this seems unnecessarily restrictive. Couldn't you easily find yourself in the situation where the user/role owning theses objects is much more capable than the user/role trying to "learn" them?

krlmlr commented 1 year ago

Does that help:

https://stackoverflow.com/q/16830740/946850

The file you linked to comes directly from Postgres, with small adaptations:

psql -c '\d+ information_schema.constraint_column_usage'
nbenn commented 1 year ago

According to relevant docs, the view generated by the code that serves as basis for this query is defined such that

Only those columns are shown that are contained in a table owned by a currently enabled role.

Given my current understanding, this does not mean that the offending WHERE clause cannot safely be excluded.

nbenn commented 1 year ago

@krlmlr Any update here? I believe there is a mismatch in semantics.

Calling dm_from_con(con, learn_keys = TRUE) my expectation is (and the docs do not point in any other direction) that PKs, FKs, etc. are returned for all tables that the current user has access to (and not is owner of).

You are using query code, which is specifically restricted to tables that are owned by the current user (as explicitly stated in the docs).

In a setting where the con user has access to tables but does not own these tables this then yields "wrong" results.

krlmlr commented 1 year ago

We found the code that needs adoption. Would you like to take a stab, do you need help?

krlmlr commented 1 year ago

We also need a reprex.

nbenn commented 1 year ago

The reprex, as requested. I'm sorry for the delay.

General set up:

CREATE DATABASE fk_vis_reprex;

CREATE USER fk_vis_owner WITH PASSWORD 'pwd123';
CREATE USER fk_vis_reader WITH PASSWORD 'pwd345';

GRANT CONNECT, TEMPORARY, CREATE ON DATABASE fk_vis_reprex TO fk_vis_owner;
GRANT CONNECT, TEMPORARY ON DATABASE fk_vis_reprex TO fk_vis_reader;

then, with a connection of our user fk_vis_owner, some more set up (as psql -d fk_vis_reprex -U fk_vis_owner):

CREATE SCHEMA fk_vis;

GRANT USAGE ON SCHEMA fk_vis TO fk_vis_reader;
ALTER DEFAULT PRIVILEGES IN SCHEMA fk_vis GRANT SELECT ON TABLES TO fk_vis_reader;

CREATE TABLE fk_vis.item (
  item INT GENERATED ALWAYS AS IDENTITY,
  name VARCHAR(255) NOT NULL,
  PRIMARY KEY(item)
);

CREATE TABLE fk_vis.customer (
  customer INT GENERATED ALWAYS AS IDENTITY,
  name VARCHAR(255) NOT NULL,
  PRIMARY KEY(customer)
);

CREATE TABLE fk_vis.sale (
  sale INT GENERATED ALWAYS AS IDENTITY,
  item INT,
  customer INT,
  PRIMARY KEY(sale),
  CONSTRAINT fk_item
    FOREIGN KEY(item)
    REFERENCES fk_vis.item(item),
  CONSTRAINT fk_customer
    FOREIGN KEY(customer)
    REFERENCES fk_vis.customer(customer)
);

With this, dm::dm_from_con(., learn_keys = TRUE) gives us:

From what I understand, there is no restriction for getting this information as the "reader" user (as psql -d fk_vis_reprex -U fk_vis_reader), for the example of FK constraints:

SELECT conrelid::regclass AS table_name,
       conname AS foreign_key,
       pg_get_constraintdef(oid)
FROM   pg_constraint
WHERE  contype = 'f'
AND    connamespace = 'fk_vis'::regnamespace
ORDER  BY conrelid::regclass::text, contype DESC;
#>  table_name  | foreign_key |                    pg_get_constraintdef
#>  -------------+-------------+-------------------------------------------------------------
#>  fk_vis.sale | fk_item     | FOREIGN KEY (item) REFERENCES fk_vis.item(item)
#>  fk_vis.sale | fk_customer | FOREIGN KEY (customer) REFERENCES fk_vis.customer(customer)
salim-b commented 3 months ago

Just ran into the same issue. My (PG)SQL knowledge is very limited, but I noticed that NocoDB suffers from the same issue and its proposed solution might serve as inspiration here (see the PGSQL query changes).