postgresql_privs - Improved roles management

[RealGreenDragon]

SUMMARY

I checked that when privileges are granted/revoked to/from PUBLIC role postgresql_privs module is not idempotent (always return changed=True).

The cause are following checks added in PR #858 (issue #857):

community.postgresql/plugins/modules/postgresql_privs.py

Line 819 in 7199adc

if roles == 'PUBLIC':

community.postgresql/plugins/modules/postgresql_privs.py

Line 1123 in 7199adc

if p.roles.upper() == 'PUBLIC':

Such checks is useless, as:

• each grant/revoke if make changes modifies at least an aclitem
• an empty aclitem implies some default privileges (see documentation)
• a grant to PUBLIC results in an aclitem with empty grantee

As in manipulate_privs function aclitems before and after queries are compared, any grant/revoke to/from every role (included PUBLIC) is correctly detected. If there are no changes than privileges are sure already granted/revoked (otherwise there is a serious PostgreSQL bug...).

Documentation about privileges and ACLs is here (for PostgreSQL 15, similar in other versions):

PostgreSQL grants privileges on some types of objects to PUBLIC by default when the objects are created. No privileges are granted to PUBLIC by default on tables, table columns, sequences, foreign data wrappers, foreign servers, large objects, schemas, tablespaces, or configuration parameters. For other types of objects, the default privileges granted to PUBLIC are as follows: CONNECT and TEMPORARY (create temporary tables) privileges for databases; EXECUTE privilege for functions and procedures; and USAGE privilege for languages and data types (including domains). The object owner can, of course, REVOKE both default and expressly granted privileges.
...
An empty grantee field in an aclitem stands for PUBLIC.
...
If the “Access privileges” column is empty for a given object, it means the object has default privileges (that is, its privileges entry in the relevant system catalog is null). Default privileges always include all privileges for the owner, and can include some privileges for PUBLIC depending on the object type, as explained above.

My PR improve general roles management in postgresql_privs module, following the changelog:

• roles provided to module are checked only once in main (no need to check them twice in main and manipulate_privs)
• role_exists function improved:
  • it is now a Connection class method (style is now uniform at other Connection class methods)
  • it now return True if role is an implicit role (case insensitive check)
  • it now supports all implicit roles: PUBLIC, CURRENT_USER, SESSION_USER, CURRENT_ROLE (documentation)
• removed any hardcoded PUBLIC check outside role_exists function
• updated documentation:
  • removed references to hardcoded always changed when roles=PUBLIC
  • added all implicit roles: PUBLIC, CURRENT_USER, SESSION_USER, CURRENT_ROLE (documentation)
• removed test (postgresql_privs_initial.yml) added in PR #858 as:
  • PUBLIC can be passed now both lowercase and uppercase
  • CONNECT on databases is already granted to PUBLIC (I checked it adding a warn and "=Tc/postgres" ACL entry is already present, maybe granted by default or before in test pipeline) so it cannot result changed=True
• added tests (postgresql_privs_initial.yml) to grant/revoke CREATE on databases (sure not granted by default and before in pipeline) to PIBLIC and SESSION_USER (in lowercase)
• fixed test "grant privileges on some table in schemas with special names" (postgresql_privs_general.yml) as now roles=PUBLIC not implies changed=True anymore and in previous test "grant privileges on all tables in schemas with special names" a privileges superset has be already granted

ISSUE TYPE

• Feature Pull Request

COMPONENT NAME

postgresql_privs

ADDITIONAL INFORMATION

Example of ACLs changes when GRANT CREATE ON DATABASE to PUBLIC:

test=# SELECT datacl FROM pg_catalog.pg_database WHERE datname = 'postgres';
 datacl
--------

(1 row)

test=# GRANT CREATE ON DATABASE postgres TO PUBLIC;
GRANT

test=# SELECT datacl FROM pg_catalog.pg_database WHERE datname = 'postgres';
          datacl
---------------------------
 {=CTc/test,test=CTc/test}
(1 row)

[RealGreenDragon]

Obviously the string "X.X.X" in comments added must be replaced with the collection release that will include this PR.

[RealGreenDragon]

Ready for review

[betanummeric]

Role names containing a , are supported by PostgreSQL. The safest way to deal with that would probably be to extend the roles module argument to also take a list of strings, something like:

if isinstance(p.roles, list):
  roles_raw = list(p.roles)
else:
  roles_raw = p.roles.split(',')

[RealGreenDragon]

Thanks for advice.
I tried to implement it but at last I had to revert.

The approach is possible but Ansible Module needs a fixed type for parameter, that can be in this case 'str' or 'list':

• If I use 'str' and provide a 'list', the list is converted to str so an 'eval' is needed to convert in list again (very dangerous).
• If I use 'list' and provide an 'str', it is converted to a list of sigle 'str' chars or to a list of 'str' words (unfortunely with commas considered as a separate word, as "['role1', ',', 'role2']).

The only way usable is use 'list' but this implies to forbid comma separated list, so allowing only one element as string or multiple elements in a list (as done by other core modules as yum, apt, etc.).
This is surely possible, but resulting in a breaking change, so I'm unable to implement it in a minor change PR.
I leaved the commits with modified code in branch history for reference in future.

[betanummeric]

@RealGreenDragon
We could still make this work by making the ansible module argument type='raw', which disables type checking by ansible for that. Then we should be able to check the type "on our own" with something like to code example from my comment above.

I'm unresolving this in case you want to implement it in this PR. Doing it in some other PR would also be fine for me.

[RealGreenDragon]

I fixed what requested, thanks for all advices.
I'm unable to implement list support for parameter "roles" as here.
I'm waiting to know collection version to replace at X.X.X.

Seems RHEL tests fails now for "500: Internal server error": can someone check?

[Andersson007]

@RealGreenDragon thanks for pushing forward

So do we expect these changes keep backwards compatibility? i.e. users playbooks will not break after upgrading the collection?

[RealGreenDragon]

@Andersson007 I confirm this PR is a minor change. I only added other implicit roles (before not supported) and fixed idempotence when roles=PUBLIC (now it reports changed only when really changed). In addiction I refactored a bit 'roles' parameter management, but without breacking changes.

[betanummeric]

PostgreSQL allows almost all role names when quoted:

test=# drop role "SESSION_USER";
DROP ROLE
Time: 2.079 ms
test=# create role "PUBLIC";
CREATE ROLE
Time: 1.997 ms
test=# create role "CURRENT_USER";
CREATE ROLE
Time: 1.829 ms
test=# create role "CURRENT_ROLE";
CREATE ROLE
Time: 1.719 ms
test=# create role "SESSION_ROLE";
CREATE ROLE
Time: 1.854 ms
test=# create role "public";
ERROR:  role name "public" is reserved
LINE 1: create role "public";
                    ^
Time: 1.049 ms
test=# create role "current_user";
CREATE ROLE
Time: 2.511 ms

So if someone was using such an confusing role name, this PR would break it. But I think we can treat it as a minor change.

[RealGreenDragon]

@betanummeric All advices implemented.
RHEL tests still failed, seems py3.9 cannot be installed.

[Andersson007]

I approved the PR and it's about general stuff.
@hunleyd @betanummeric do you think it's ready for merge? No pressure: considering importance of the module, feel free to add more comments of course

[Andersson007]

cc @hunleyd @betanummeric

[hunleyd]

@Andersson007 assuming the open conversation from @betanummeric is addressed, i'm ok with merging it

[hunleyd]

merged as per @Andersson007's comment on matrix

[betanummeric]

Yes, this looks ready to be merged.

[Andersson007]

@RealGreenDragon thanks for the great contribution!
@hunleyd @betanummeric @jchancojr thanks for reviewing!