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

postgresql_privs - Improved roles management #502

Merged
merged 33 commits into from
Jul 3, 2023
Merged
Show file tree
Hide file tree
Changes from 31 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
fc7bb34
postgresql_privs - fixed idempotence when roles=PUBLIC
RealGreenDragon Jun 9, 2023
11c2d65
Fix
RealGreenDragon Jun 9, 2023
bcef2e5
Fixed fragment
RealGreenDragon Jun 10, 2023
77da0c7
postgresql_privs - Improved roles management
RealGreenDragon Jun 10, 2023
0e11a2e
Added module.warn for debug
RealGreenDragon Jun 10, 2023
42045c1
Hotfix
RealGreenDragon Jun 10, 2023
3c0526b
Removed debug code
RealGreenDragon Jun 10, 2023
7142458
Replaced tests of PR 857
RealGreenDragon Jun 10, 2023
0655765
Moved role_exists inside Connection class.
RealGreenDragon Jun 10, 2023
c88d507
Fixed tests
RealGreenDragon Jun 10, 2023
aaf1361
Fixed fragment
RealGreenDragon Jun 10, 2023
a3b3372
Fixed tests
RealGreenDragon Jun 10, 2023
c931255
Fixed tests
RealGreenDragon Jun 10, 2023
9d87bc9
Fixed implicit roles management
RealGreenDragon Jun 10, 2023
5b4ed7c
Fixed comments.
RealGreenDragon Jun 10, 2023
b7629bf
Fixed doc
RealGreenDragon Jun 10, 2023
f1fcc08
Fixed tests
RealGreenDragon Jun 10, 2023
fec8b67
Fixed "grant privileges on some table in schemas with special names" …
RealGreenDragon Jun 10, 2023
3156bff
Fixed VALID_IMPLICIT_ROLES
RealGreenDragon Jun 10, 2023
f5cb7b4
code style fix
RealGreenDragon Jun 11, 2023
faa0dc9
Fixed comment
RealGreenDragon Jun 12, 2023
495cdc6
fixed changelog fragment
RealGreenDragon Jun 13, 2023
e408a01
Fixed docs
RealGreenDragon Jun 13, 2023
240fd4c
added list as accepted formats of roles parameter
RealGreenDragon Jun 13, 2023
d03f2e8
Fixed docs
RealGreenDragon Jun 13, 2023
6330577
Fixed tests
RealGreenDragon Jun 13, 2023
e8a7dbc
Revert
RealGreenDragon Jun 13, 2023
35e1f91
Merge branch 'privs_public_role' of https://github.com/RealGreenDrago…
RealGreenDragon Jun 13, 2023
123e5b3
Revert "added list as accepted formats of roles parameter" and next c…
RealGreenDragon Jun 13, 2023
4478816
Update plugins/modules/postgresql_privs.py
RealGreenDragon Jun 15, 2023
d1adbdd
Update plugins/modules/postgresql_privs.py
RealGreenDragon Jun 15, 2023
c0c598c
Added fail_json at any SQL execute exception
RealGreenDragon Jun 20, 2023
8300ac6
Added replace for SQL injection
RealGreenDragon Jun 20, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions changelogs/fragments/502_postgresql_privs.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
minor_changes:
- "postgresql_privs - added support for implicit roles CURRENT_ROLE, CURRENT_USER, and SESSION_USER (https://github.com/ansible-collections/community.postgresql/pull/502)."
- "postgresql_privs - added idempotence when roles=PUBLIC (https://github.com/ansible-collections/community.postgresql/pull/502)."
99 changes: 50 additions & 49 deletions plugins/modules/postgresql_privs.py
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,9 @@
roles:
description:
- Comma separated list of role (user/group) names to set permissions for.
- The special value C(PUBLIC) can be provided instead to set permissions
for the implicitly defined PUBLIC group.
- Roles C(PUBLIC), C(CURRENT_ROLE), C(CURRENT_USER), C(SESSION_USER) are implicitly defined in PostgreSQL.
- C(CURRENT_USER) and C(SESSION_USER) implicit roles are supported since collection version 3.1.0 and PostgreSQL 9.5.
- C(CURRENT_ROLE) implicit role is supported since collection version 3.1.0 and PostgreSQL 14.
type: str
required: true
aliases:
Expand Down Expand Up @@ -133,7 +134,6 @@
C(present) and I(grant_option) to C(false) (see examples).
- Note that when revoking privileges from a role R, this role may still have
access via privileges granted to any role R is a member of including C(PUBLIC).
- Note that when you use C(PUBLIC) role, the module always reports that the state has been changed.
- Note that when revoking privileges from a role R, you do so as the user
specified via I(login_user). If R has been granted the same privileges by
another user also, R can still access database objects via these privileges.
Expand Down Expand Up @@ -424,6 +424,10 @@
'FUNCTIONS': ('ALL', 'EXECUTE'),
'TYPES': ('ALL', 'USAGE'),
'SCHEMAS': ('CREATE', 'USAGE'), }
VALID_IMPLICIT_ROLES = {'PUBLIC': 0,
'CURRENT_USER': 95000,
'SESSION_USER': 95000,
'CURRENT_ROLE': 140000, }

executed_queries = []

Expand All @@ -432,19 +436,6 @@ class Error(Exception):
pass


def role_exists(module, cursor, rolname):
"""Check user exists or not"""
query = "SELECT 1 FROM pg_roles WHERE rolname = '%s'" % rolname
try:
cursor.execute(query)
return cursor.rowcount > 0

except Exception as e:
module.fail_json(msg="Cannot execute SQL '%s': %s" % (query, to_native(e)))

return False


# We don't have functools.partial in Python < 2.5
def partial(f, *args, **kwargs):
"""Partial function application"""
Expand Down Expand Up @@ -477,6 +468,11 @@ def __init__(self, params, module):
self.cursor = self.connection.cursor()
self.pg_version = self.connection.server_version

# implicit roles in current pg version
self.pg_implicit_roles = tuple(
implicit_role for implicit_role, version_min in VALID_IMPLICIT_ROLES.items() if self.pg_version >= version_min
)

def commit(self):
self.connection.commit()

Expand All @@ -488,8 +484,23 @@ def encoding(self):
"""Connection encoding in Python-compatible form"""
return psycopg2.extensions.encodings[self.connection.encoding]

# Methods for implicit roles managements

def is_implicit_role(self, rolname):
return rolname.upper() in self.pg_implicit_roles

# Methods for querying database objects

def role_exists(self, rolname):
# check if rolname is a implicit role
if self.is_implicit_role(rolname):
return True

# check if rolname is present in pg_catalog.pg_roles
query = "SELECT 1 FROM pg_catalog.pg_roles WHERE rolname = %s"
self.cursor.execute(query, (rolname,))
return self.cursor.rowcount > 0

# PostgreSQL < 9.0 doesn't support "ALL TABLES IN SCHEMA schema"-like
# phrases in GRANT or REVOKE statements, therefore alternative methods are
# provided here.
Expand Down Expand Up @@ -698,9 +709,8 @@ def manipulate_privs(self, obj_type, privs, objs, orig_objs, roles, target_roles
or None if type is "group".
:param objs: List of database objects to grant/revoke
privileges for.
:param orig_objs: ALL_IN_SCHEMA or None
:param roles: Either a list of role names or "PUBLIC"
for the implicitly defined "PUBLIC" group
:param orig_objs: ALL_IN_SCHEMA or None.
:param roles: List of role names.
:param target_roles: List of role names to grant/revoke
default privileges as.
:param state: "present" to grant privileges, "absent" to revoke.
Expand Down Expand Up @@ -778,26 +788,11 @@ def manipulate_privs(self, obj_type, privs, objs, orig_objs, roles, target_roles
set_what = '%s ON %s %s' % (','.join(privs), obj_type.replace('_', ' '), ','.join(obj_ids))

# for_whom: SQL-fragment specifying for whom to set the above
if roles == 'PUBLIC':
for_whom = 'PUBLIC'
else:
for_whom = []
for r in roles:
if not role_exists(self.module, self.cursor, r):
if fail_on_role:
self.module.fail_json(msg="Role '%s' does not exist" % r.strip())

else:
self.module.warn("Role '%s' does not exist, pass it" % r.strip())
else:
for_whom.append('"%s"' % r)

if not for_whom:
return False

for_whom = ','.join(for_whom)
if not roles:
return False
for_whom = ','.join(roles)

# as_who:
# as_who: SQL-fragment specifying to who to set the above
as_who = None
if target_roles:
as_who = ','.join('"%s"' % r for r in target_roles)
Expand All @@ -816,8 +811,6 @@ def manipulate_privs(self, obj_type, privs, objs, orig_objs, roles, target_roles

executed_queries.append(query)
self.cursor.execute(query)
if roles == 'PUBLIC':
return True

status_after = get_status(objs)

Expand Down Expand Up @@ -1120,17 +1113,25 @@ def main():
objs = [obj.replace(':', ',') for obj in objs]

# roles
if p.roles.upper() == 'PUBLIC':
roles = 'PUBLIC'
else:
roles = p.roles.split(',')

if len(roles) == 1 and not role_exists(module, conn.cursor, roles[0]):
roles = []
roles_raw = p.roles.split(',')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(',')

Copy link
Contributor Author

@RealGreenDragon RealGreenDragon Jun 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

for r in roles_raw:
if conn.role_exists(r):
if conn.is_implicit_role(r):
# Some implicit roles (as PUBLIC) works in uppercase without double quotes and in lowercase with double quotes.
# Other implicit roles (as SESSION_USER) works only in uppercase without double quotes.
# So the approach that works for all implicit roles is uppercase without double quotes.
roles.append('%s' % r.upper())
else:
roles.append('"%s"' % r)
else:
if fail_on_role:
module.fail_json(msg="Role '%s' does not exist" % roles[0].strip())
module.fail_json(msg="Role '%s' does not exist" % r)
else:
module.warn("Role '%s' does not exist, nothing to do" % roles[0].strip())
module.exit_json(changed=False, queries=executed_queries)
module.warn("Role '%s' does not exist, pass it" % r)
if not roles:
module.warn("No valid roles provided, nothing to do")
module.exit_json(changed=False, queries=executed_queries)

# check if target_roles is set with type: default_privs
if p.target_roles and not p.type == 'default_privs':
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1554,7 +1554,7 @@
register: result
- assert:
that:
- result is changed
- result is not changed
- name: check permissions on tables in schemas with special names
become: true
become_user: "{{ pg_user }}"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -343,22 +343,144 @@
target_roles: "{{ db_user_with_dots2 }}"
trust_input: false

# Bugfix for https://github.com/ansible-collections/community.general/issues/857
- name: Test passing lowercase PUBLIC role
# https://github.com/ansible-collections/community.postgresql/pull/502 - role PUBLIC
- name: Test passing lowercase PUBLIC role - Grant CREATE ON DATABASE - Test
become_user: "{{ pg_user }}"
become: true
postgresql_privs:
db: "{{ db_name }}"
login_user: "{{ pg_user }}"
state: present
type: 'database'
privs: 'connect'
privs: 'create'
role: 'public'
register: result

- assert:
that:
- result is changed
- result.queries == ["GRANT CONNECT ON database \"{{ db_name }}\" TO PUBLIC;"]
- result.queries == ["GRANT CREATE ON database \"{{ db_name }}\" TO PUBLIC;"]

- name: Test passing lowercase PUBLIC role - Grant CREATE ON DATABASE - Idempotence
become_user: "{{ pg_user }}"
become: true
postgresql_privs:
db: "{{ db_name }}"
login_user: "{{ pg_user }}"
state: present
type: 'database'
privs: 'create'
role: 'public'
register: result

- assert:
that:
- result is not changed
- result.queries == ["GRANT CREATE ON database \"{{ db_name }}\" TO PUBLIC;"]

- name: Test passing lowercase PUBLIC role - Revoke CREATE ON DATABASE - Test
become_user: "{{ pg_user }}"
become: true
postgresql_privs:
db: "{{ db_name }}"
login_user: "{{ pg_user }}"
state: absent
type: 'database'
privs: 'create'
role: 'public'
register: result

- assert:
that:
- result is changed
- result.queries == ["REVOKE CREATE ON database \"{{ db_name }}\" FROM PUBLIC;"]

- name: Test passing lowercase PUBLIC role - Revoke CREATE ON DATABASE - Test
become_user: "{{ pg_user }}"
become: true
postgresql_privs:
db: "{{ db_name }}"
login_user: "{{ pg_user }}"
state: absent
type: 'database'
privs: 'create'
role: 'public'
register: result

- assert:
that:
- result is not changed
- result.queries == ["REVOKE CREATE ON database \"{{ db_name }}\" FROM PUBLIC;"]

# https://github.com/ansible-collections/community.postgresql/pull/502 - role SESSION_USER
# first revoke after grant, as the privilege is already granted
- name: Test passing lowercase SESSION_USER role - Revoke CREATE ON DATABASE - Test
become_user: "{{ pg_user }}"
become: true
postgresql_privs:
db: "{{ db_name }}"
login_user: "{{ pg_user }}"
state: absent
type: 'database'
privs: 'create'
role: 'session_user'
register: result

- assert:
that:
- result is changed
- result.queries == ["REVOKE CREATE ON database \"{{ db_name }}\" FROM SESSION_USER;"]

- name: Test passing lowercase SESSION_USER role - Revoke CREATE ON DATABASE - Test
become_user: "{{ pg_user }}"
become: true
postgresql_privs:
db: "{{ db_name }}"
login_user: "{{ pg_user }}"
state: absent
type: 'database'
privs: 'create'
role: 'session_user'
register: result

- assert:
that:
- result is not changed
- result.queries == ["REVOKE CREATE ON database \"{{ db_name }}\" FROM SESSION_USER;"]

- name: Test passing lowercase SESSION_USER role - Grant CREATE ON DATABASE - Test
become_user: "{{ pg_user }}"
become: true
postgresql_privs:
db: "{{ db_name }}"
login_user: "{{ pg_user }}"
state: present
type: 'database'
privs: 'create'
role: 'session_user'
register: result

- assert:
that:
- result is changed
- result.queries == ["GRANT CREATE ON database \"{{ db_name }}\" TO SESSION_USER;"]

- name: Test passing lowercase SESSION_USER role - Grant CREATE ON DATABASE - Idempotence
become_user: "{{ pg_user }}"
become: true
postgresql_privs:
db: "{{ db_name }}"
login_user: "{{ pg_user }}"
state: present
type: 'database'
privs: 'create'
role: 'session_user'
register: result

- assert:
that:
- result is not changed
- result.queries == ["GRANT CREATE ON database \"{{ db_name }}\" TO SESSION_USER;"]

#
# Cleanup
Expand Down