Updates to security definer functions

[cymed]

General

• Fix a bug
• Maintenance / sustainability
• Add Documentation

Describe your changes

Security definer functions are a potential point of attack and should therefore be managed carefully. When the search_path is not defined, any user that can CREATE on public can gain access to SUPERUSER privileges through them.

This PR

• Removes the security definer flag where possible
• Truncates tww_od on plugin test instead of drop/create (easier grants management)
• revokes CREATE ON DATABASE from PUBLIC
• revokes CONNECT ON DATABASE from PUBLIC
• grants CONNECT ON DATABASE to tww_viewer
• Alters the OWNER of the materialized views that are to be updated by tww_user to tww_user (for Postgres<=16)
• -GRANTs MAINTAIN onthe materialized views to tww_user` (for Postgres>=17) see Postgres 17 docs

To-Do

• When introducing database-specific roles, we must also tackle how to grant OWNER to the corresponding to the database specific role

Screenshots

Issue ticket number and link

Checklist before requesting a review

• I have performed a self-review of my code
• If it is a core feature, I have added thorough tests.
• CI Tests are green
• The documentation is up to date with the proposed change.
• My work is ready for review

Checklist before merge

• A review has been performed
• Comments are resolved
• Documentation is ready

[cymed]

Does anyone know why hstore is not loaded?

[ponceta]

db-1  | psql:/src/datamodel/scripts/../changelogs/0001/90_audit.sql:22: NOTICE:  table "logged_actions" does not exist, skipping

I don't understand how master is fine and your branch is not.

Do you need a rebase of your branch?

Looks like the hstore extension is not pushed even if it is pushed on other branches like : #528

[cymed]

The errors stem from me not including public in the search_path for SECURITY DEFINER functions, but installing the extensions on public. What is best practice @3nids?

[ponceta]

As discussed with @cymed, take a common look with @3nids on TEKSI support to check what is the best approach on that matter, knowing that hstore should probably be removed in favor of jsonb.

Other extensions like postgis and uuid-ossp will probably use this search path too.

Many applications still provide all their tables in the public schema for simplicity reasons, security can be put at another level (for example avoiding any direct connection to the database from an untrusted network) could be a "physical security".

[3nids]

The modifications of the changelogs are fine, good catch.

Regarding the addition of the 99_post_all.sql, I would think this does not belong here, but rather in the role script.

[3nids]

Regarding the revokation, I would simply remove this right from the roles we are actually creating.

[cymed]

Regarding the revokation, I would simply remove this right from the roles we are actually creating.

As long as CONNECT and CREATE ON SCHEMA public is open to PUBLIC, removing the rights from the roles we are actually creating changes nothing, as everyone is member of PUBLIC and therefore has access

[cymed]

What do you think about revoking connect from PUBLIC per default and granting CONNECT to tww_viewer?

[3nids]

Interesting, I'll try to grab some expertise on the topic and come back.

[cymed]

I suggest GRANTing CREATE ON DATABASE to tww_sysadmin

[sjib]

@cymed Is this something that should be included in the next Release? If yes what are the stumbling blocks?

[cymed]

The problem is the CI: Even after granting tww_user to postgres, refreshing the materialized view fails

[cymed]

did we alter the plugin CI tests @ponceta ?

[cymed]

@ponceta ready for review