From 384839bfe132e70d9b49d8e347ce00c48d0f6d60 Mon Sep 17 00:00:00 2001 From: Pepe Barbe Date: Tue, 21 Aug 2012 11:20:16 -0500 Subject: [PATCH 1/4] Initial commit of change of semantics for module The postgresql_user module has several drawbacks: * No granularity for privileges * PostgreSQL semantics force working on one database at time, at least for Tables. Which means that a single call can't remove all the privileges for a user, and a user can't be removed until all the privileges are removed, forcing a module failure with no way to work around the issue. Changes: * Added the ability to specify granular privileges for database and tables within the database * Report if user was removed, and add an option to disable failing if user is not removed. --- postgresql_user | 191 ++++++++++++++++++++++++++++++++++++------------ 1 file changed, 143 insertions(+), 48 deletions(-) diff --git a/postgresql_user b/postgresql_user index 56b0abd58ee..51d971db966 100755 --- a/postgresql_user +++ b/postgresql_user @@ -34,35 +34,14 @@ def user_exists(cursor, user): return cursor.rowcount > 0 -def user_add(cursor, user, password, db): +def user_add(cursor, user, password): """Create a new user with write access to the database""" query = "CREATE USER %(user)s with PASSWORD '%(password)s'" cursor.execute(query % {"user": user, "password": password}) - grant_privileges(cursor, user, db) return True - -def has_privileges(cursor, user, db): - """Check if the user has create privileges on the database""" - query = "SELECT has_database_privilege(%(user)s, %(db)s, 'CREATE')" - cursor.execute(query, {'user': user, 'db': db}) - return cursor.fetchone()[0] - - -def grant_privileges(cursor, user, db): - """Grant all privileges on the database""" - query = "GRANT ALL PRIVILEGES ON DATABASE %(db)s TO %(user)s" - cursor.execute(query % {'user': user, 'db': db}) - - -def revoke_privileges(cursor, user, db): - """Revoke all privileges on the database""" - query = "REVOKE ALL PRIVILEGES ON DATABASE %(db)s FROM %(user)s" - cursor.execute(query % {'user': user, 'db': db}) - - -def user_mod(cursor, user, password, db): - """Update password and permissions""" +def user_chpass(cursor, user, password): + """Change user password""" changed = False # Handle passwords. @@ -79,28 +58,131 @@ def user_mod(cursor, user, password, db): if current_pass_hash != new_pass_hash: changed = True - # Handle privileges. - # For now, we just check if the user has access to the database - if not has_privileges(cursor, user, db): - grant_privileges(cursor, user, db) - changed = True - return changed +def user_delete(cursor, user): + """Try to remove a user. Returns True if successful otherwise False""" + cursor.execute("SAVEPOINT ansible_pgsql_user_delete") + try: + cursor.execute("DROP USER %s" % user) + except: + cursor.execute("ROLLBACK TO SAVEPOINT ansible_pgsql_user_delete") + cursor.execute("RELEASE SAVEPOINT ansible_pgsql_user_delete") + return False + + cursor.execute("RELEASE SAVEPOINT ansible_pgsql_user_delete") + return True + +def has_table_privilege(cursor, user, table, priv): + query = 'SELECT has_table_privilege(%s, %s, %s)' + cursor.execute(query, user, table, priv) + return cursor.fetchone()[0] + +def grant_table_privilege(cursor, user, table, priv): + if has_table_privilege(cursor, user, table, priv): + return False + query = 'GRANT %s ON TABLE %s TO %s' % (priv, table, user) + cursor.execute(query) + return True + +def revoke_table_privilege(cursor, user, table, priv): + if not has_table_privilege(cursor, user, table, priv): + return False + query = 'REVOKE %s ON TABLE %s FROM %s' % (priv, table, user) + cursor.execute(query) + return True + + +def has_database_privilege(cursor, user, db, priv): + query = 'SELECT has_database_privilege(%s, %s, %s)' + cursor.execute(query, user, db, priv) + return cursor.fetchone()[0] + +def grant_database_privilege(cursor, user, db, priv): + if has_database_privilege(cursor, user, db, priv): + return False + query = 'GRANT %s ON DATABASE %s TO %s' % (priv, db, user) + cursor.execute(query) + return True -def user_delete(cursor, user, db): - """Delete a user, first revoking privileges""" - revoke_privileges(cursor, user, db) - cursor.execute("DROP USER %(user)s" % {'user': user}) +def revoke_database_privilege(cursor, user, db, priv): + if not has_database_privilege(cursor, user, db, priv): + return False + query = 'REVOKE %s ON DATABASE %s FROM %s' % (priv, db, user) + cursor.execute(query) return True +def revoke_privileges(cursor, user, privs): + if privs is None: + return False + changed = False + for type_ in privs: + revoke_func = { + 'table':revoke_table_privilege, + 'database':revoke_database_privilege + }[type_] + for name, privileges in privs[type_].iteritem(): + for privilege in privileges: + changed = revoke_func(cursor, user, name, privilege)\ + or changed + + return changed + +def grant_privileges(cursor, user, privs): + if privs is None: + return False + + changed = False + for type_ in privs: + grant_func = { + 'table':grant_table_privilege, + 'database':grant_database_privilege + }[type_] + for name, privileges in privs[type_].iteritem(): + for privilege in privileges: + changed = grant_func(cursor, user, name, privilege)\ + or changed + + return changed + +def parse_privs(privs, db): + """ + Parse privilege string to determine permissions for database db. + Format: + + privileges[/privileges/...] + + Where: + + privileges := DATABASE_PRIVILEGES[,DATABASE_PRIVILEGES,...] | + TABLE_NAME:TABLE_PRIVILEGES[,TABLE_PRIVILEGES,...] + """ + if privs is None: + return privs + + privs = { + 'database':{}, + 'table':{} + } + for token in privs.split('/'): + if ':' not in token: + type_ = 'database' + name = db + privileges = token + else: + type_ = 'table' + name, privileges = token.split(':', 1) + privileges = privileges.split(',') + + privs[type_][name] = privileges + + return privs # =========================================== # Module execution. # - def main(): module = AnsibleModule( argument_spec=dict( @@ -110,13 +192,19 @@ def main(): user=dict(required=True, aliases=['name']), password=dict(default=None), state=dict(default="present", choices=["absent", "present"]), - db=dict(required=True), + priv=dict(default=None), + db=dict(default=''), + fail_on_user=dict(default=True) ) ) user = module.params["user"] password = module.params["password"] state = module.params["state"] + fail_on_user = module.params["fail_on_user"] db = module.params["db"] + if db == '' and module.params["priv"] is not None: + module.fail_json(msg="privileges require a database to be specified") + privs = parse_privs(module.params["priv"], db) if not postgresqldb_found: module.fail_json(msg="the python psycopg2 module is required") @@ -127,7 +215,8 @@ def main(): params_map = { "login_host":"host", "login_user":"user", - "login_password":"password" + "login_password":"password", + "db":"database" } kw = dict( (params_map[k], v) for (k, v) in module.params.iteritems() if k in params_map and v != "" ) @@ -136,24 +225,30 @@ def main(): cursor = db_connection.cursor() except Exception, e: module.fail_json(msg="unable to connect to database: %s" % e) - + + changed = False if state == "present": if user_exists(cursor, user): - changed = user_mod(cursor, user, password, db) + changed = user_chpass(cursor, user, password) else: if password is None: msg = "password parameter required when adding a user" module.fail_json(msg=msg) - changed = user_add(cursor, user, password, db) - - elif state == "absent": + changed = user_add(cursor, user, password) + changed = grant_privileges(cursor, user, privs) or changed + else: if user_exists(cursor, user): - changed = user_delete(cursor, user, db) - else: - changed = False - # Commit the database changes - db_connection.commit() - module.exit_json(changed=changed, user=user) + changed = revoke_privileges(cursor, user, privs) + user_removed = user_delete(cursor, user) + changed = changed or user_removed + + if fail_on_user and not user_removed: + msg = "unabel to remove user" + module.fail_json(msg=msg) + + if changed: + db_connection.commit() + module.exit_json(changed=changed, user=user, user_removed=user_removed) # this is magic, see lib/ansible/module_common.py #<> From a7e1ca6a6f30cc67af53f8aef817d30152788577 Mon Sep 17 00:00:00 2001 From: Pepe Barbe Date: Tue, 21 Aug 2012 14:23:45 -0500 Subject: [PATCH 2/4] Add fail_on_user option fail_on_user option can be used to ignore silently if the user cannot be removed because of remaining privilege dependencies to other objects in the database. By default it will fail, so that this new behavior won't surprise unsuspecting users. --- postgresql_user | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/postgresql_user b/postgresql_user index 51d971db966..dbef34e0ec5 100755 --- a/postgresql_user +++ b/postgresql_user @@ -194,13 +194,13 @@ def main(): state=dict(default="present", choices=["absent", "present"]), priv=dict(default=None), db=dict(default=''), - fail_on_user=dict(default=True) + fail_on_user=dict(default='yes') ) ) user = module.params["user"] password = module.params["password"] state = module.params["state"] - fail_on_user = module.params["fail_on_user"] + fail_on_user = module.params["fail_on_user"] == 'yes' db = module.params["db"] if db == '' and module.params["priv"] is not None: module.fail_json(msg="privileges require a database to be specified") @@ -221,12 +221,14 @@ def main(): kw = dict( (params_map[k], v) for (k, v) in module.params.iteritems() if k in params_map and v != "" ) try: - db_connection = psycopg2.connect(database=db, **kw) + db_connection = psycopg2.connect(**kw) cursor = db_connection.cursor() except Exception, e: module.fail_json(msg="unable to connect to database: %s" % e) + kw = dict(user=user) changed = False + user_removed = False if state == "present": if user_exists(cursor, user): changed = user_chpass(cursor, user, password) @@ -241,14 +243,16 @@ def main(): changed = revoke_privileges(cursor, user, privs) user_removed = user_delete(cursor, user) changed = changed or user_removed - - if fail_on_user and not user_removed: - msg = "unabel to remove user" - module.fail_json(msg=msg) + if fail_on_user and not user_removed: + msg = "unabel to remove user" + module.fail_json(msg=msg) + kw['user_removed'] = user_removed if changed: db_connection.commit() - module.exit_json(changed=changed, user=user, user_removed=user_removed) + + kw['changed'] = changed + module.exit_json(**kw) # this is magic, see lib/ansible/module_common.py #<> From 511ab8697c4af4eefef00d638c56502d66938245 Mon Sep 17 00:00:00 2001 From: Pepe Barbe Date: Tue, 21 Aug 2012 14:25:19 -0500 Subject: [PATCH 3/4] Query for all active privileges instead Use a different method to query for current privileges at the table and database level. This method is more robust if newer privileges are added in future versions and also supports the ALL wildcard. --- postgresql_user | 72 ++++++++++++++++++++++++++++++++++--------------- 1 file changed, 51 insertions(+), 21 deletions(-) diff --git a/postgresql_user b/postgresql_user index dbef34e0ec5..8bde46aedcb 100755 --- a/postgresql_user +++ b/postgresql_user @@ -16,6 +16,8 @@ # You should have received a copy of the GNU General Public License # along with Ansible. If not, see . +import re + try: import psycopg2 except ImportError: @@ -75,42 +77,70 @@ def user_delete(cursor, user): def has_table_privilege(cursor, user, table, priv): query = 'SELECT has_table_privilege(%s, %s, %s)' - cursor.execute(query, user, table, priv) + cursor.execute(query, (user, table, priv)) return cursor.fetchone()[0] +def get_table_privileges(cursor, user, table): + if '.' in table: + schema, table = table.split('.', 1) + else: + schema = 'public' + query = '''SELECT privilege_type FROM information_schema.role_table_grants + WHERE grantee=%s AND table_name=%s AND table_schema=%s''' + cursor.execute(query, (user, table, schema)) + return set([x[0] for x in cursor.fetchall()]) + + def grant_table_privilege(cursor, user, table, priv): - if has_table_privilege(cursor, user, table, priv): - return False + prev_priv = get_table_privileges(cursor, user, table) query = 'GRANT %s ON TABLE %s TO %s' % (priv, table, user) cursor.execute(query) - return True + curr_priv = get_table_privileges(cursor, user, table) + return len(curr_priv) > len(prev_priv) def revoke_table_privilege(cursor, user, table, priv): - if not has_table_privilege(cursor, user, table, priv): - return False + prev_priv = get_table_privileges(cursor, user, table) query = 'REVOKE %s ON TABLE %s FROM %s' % (priv, table, user) cursor.execute(query) - return True + curr_priv = get_table_privileges(cursor, user, table) + return len(curr_priv) < len(prev_priv) + +def get_database_privileges(cursor, user, db): + priv_map = { + 'C':'CREATE', + 'T':'TEMPORARY', + 'c':'CONNECT', + } + query = 'SELECT datacl FROM pg_database WHERE datname = %s' + cursor.execute(query, (db,)) + datacl = cursor.fetchone()[0] + r = re.search('%s=(C?T?c?)/[a-z]+\,?' % user, datacl) + if r is None: + return [] + o = [] + for v in r.group(1): + o.append(priv_map[v]) + return o def has_database_privilege(cursor, user, db, priv): query = 'SELECT has_database_privilege(%s, %s, %s)' - cursor.execute(query, user, db, priv) + cursor.execute(query, (user, db, priv)) return cursor.fetchone()[0] def grant_database_privilege(cursor, user, db, priv): - if has_database_privilege(cursor, user, db, priv): - return False + prev_priv = get_database_privileges(cursor, user, db) query = 'GRANT %s ON DATABASE %s TO %s' % (priv, db, user) cursor.execute(query) - return True + curr_priv = get_database_privileges(cursor, user, db) + return len(curr_priv) > len(prev_priv) def revoke_database_privilege(cursor, user, db, priv): - if not has_database_privilege(cursor, user, db, priv): - return False + prev_priv = get_database_privileges(cursor, user, db) query = 'REVOKE %s ON DATABASE %s FROM %s' % (priv, db, user) cursor.execute(query) - return True + curr_priv = get_database_privileges(cursor, user, db) + return len(curr_priv) < len(prev_priv) def revoke_privileges(cursor, user, privs): if privs is None: @@ -122,7 +152,7 @@ def revoke_privileges(cursor, user, privs): 'table':revoke_table_privilege, 'database':revoke_database_privilege }[type_] - for name, privileges in privs[type_].iteritem(): + for name, privileges in privs[type_].iteritems(): for privilege in privileges: changed = revoke_func(cursor, user, name, privilege)\ or changed @@ -139,7 +169,7 @@ def grant_privileges(cursor, user, privs): 'table':grant_table_privilege, 'database':grant_database_privilege }[type_] - for name, privileges in privs[type_].iteritem(): + for name, privileges in privs[type_].iteritems(): for privilege in privileges: changed = grant_func(cursor, user, name, privilege)\ or changed @@ -161,7 +191,7 @@ def parse_privs(privs, db): if privs is None: return privs - privs = { + o_privs = { 'database':{}, 'table':{} } @@ -169,15 +199,15 @@ def parse_privs(privs, db): if ':' not in token: type_ = 'database' name = db - privileges = token + priv_set = set(x.strip() for x in token.split(',')) else: type_ = 'table' name, privileges = token.split(':', 1) - privileges = privileges.split(',') + priv_set = set(x.strip() for x in privileges.split(',')) - privs[type_][name] = privileges + o_privs[type_][name] = priv_set - return privs + return o_privs # =========================================== # Module execution. From 9e275529d661a8f31eef299989d248b95eb385e9 Mon Sep 17 00:00:00 2001 From: Pepe Barbe Date: Wed, 22 Aug 2012 12:19:55 -0500 Subject: [PATCH 4/4] Typo --- postgresql_user | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/postgresql_user b/postgresql_user index 8bde46aedcb..d883cc57cc8 100755 --- a/postgresql_user +++ b/postgresql_user @@ -274,7 +274,7 @@ def main(): user_removed = user_delete(cursor, user) changed = changed or user_removed if fail_on_user and not user_removed: - msg = "unabel to remove user" + msg = "unable to remove user" module.fail_json(msg=msg) kw['user_removed'] = user_removed