diff --git a/lib/charms/postgresql_k8s/v1/postgresql.py b/lib/charms/postgresql_k8s/v1/postgresql.py index 4fcfbada21..32d677f307 100644 --- a/lib/charms/postgresql_k8s/v1/postgresql.py +++ b/lib/charms/postgresql_k8s/v1/postgresql.py @@ -55,9 +55,6 @@ ROLE_BACKUP = "charmed_backup" ROLE_DBA = "charmed_dba" -# Groups to distinguish database permissions -PERMISSIONS_GROUP_ADMIN = "admin" - INVALID_EXTRA_USER_ROLE_BLOCKING_MESSAGE = "invalid role(s) for extra user roles" REQUIRED_PLUGINS = { @@ -246,7 +243,7 @@ def create_database( Identifier(database) ) ) - for user_to_grant_access in [user, PERMISSIONS_GROUP_ADMIN, *self.system_users]: + for user_to_grant_access in [user, *self.system_users]: cursor.execute( SQL("GRANT ALL PRIVILEGES ON DATABASE {} TO {};").format( Identifier(database), Identifier(user_to_grant_access) @@ -291,20 +288,18 @@ def create_user( """ try: # Separate roles and privileges from the provided extra user roles. - admin_role = False roles = privileges = None if extra_user_roles: - admin_role = PERMISSIONS_GROUP_ADMIN in extra_user_roles valid_privileges, valid_roles = self.list_valid_privileges_and_roles() roles = [ role for role in extra_user_roles - if role in valid_roles and role != PERMISSIONS_GROUP_ADMIN + if role in valid_roles ] privileges = { extra_user_role for extra_user_role in extra_user_roles - if extra_user_role not in roles and extra_user_role != PERMISSIONS_GROUP_ADMIN + if extra_user_role not in roles } invalid_privileges = [ privilege for privilege in privileges if privilege not in valid_privileges @@ -321,8 +316,8 @@ def create_user( if cursor.fetchone() is not None: user_definition = "ALTER ROLE {}" else: - user_definition = "CREATE ROLE {}" - user_definition += f"WITH {'NOLOGIN' if user == 'admin' else 'LOGIN'}{' SUPERUSER' if admin else ''} ENCRYPTED PASSWORD '{password}'{'IN ROLE admin CREATEDB' if admin_role else ''}" + user_definition = "CREATE ROLE {} " + user_definition += f"WITH LOGIN{' SUPERUSER' if admin else ''} ENCRYPTED PASSWORD '{password}'" if privileges: user_definition += f" {' '.join(privileges)}" cursor.execute(SQL("BEGIN;")) @@ -804,22 +799,15 @@ def set_up_database(self, temp_location: Optional[str] = None) -> None: cursor.execute(f"CREATE TABLESPACE temp LOCATION '{temp_location}';") cursor.execute("GRANT CREATE ON TABLESPACE temp TO public;") - cursor.execute("SELECT TRUE FROM pg_roles WHERE rolname='admin';") - if cursor.fetchone() is None: - # Allow access to the postgres database only to the system users. - cursor.execute("REVOKE ALL PRIVILEGES ON DATABASE postgres FROM PUBLIC;") - cursor.execute("REVOKE CREATE ON SCHEMA public FROM PUBLIC;") - for user in self.system_users: - cursor.execute( - SQL("GRANT ALL PRIVILEGES ON DATABASE postgres TO {};").format( - Identifier(user) - ) + # Allow access to the postgres database only to the system users. + cursor.execute("REVOKE ALL PRIVILEGES ON DATABASE postgres FROM PUBLIC;") + cursor.execute("REVOKE CREATE ON SCHEMA public FROM PUBLIC;") + for user in self.system_users: + cursor.execute( + SQL("GRANT ALL PRIVILEGES ON DATABASE postgres TO {};").format( + Identifier(user) ) - self.create_user( - PERMISSIONS_GROUP_ADMIN, - extra_user_roles=[ROLE_READ, ROLE_DML], ) - cursor.execute("GRANT CONNECT ON DATABASE postgres TO admin;") except psycopg2.Error as e: logger.error(f"Failed to set up databases: {e}") raise PostgreSQLDatabasesSetupError() from e @@ -907,7 +895,7 @@ def build_postgresql_group_map(group_map: Optional[str]) -> List[Tuple]: ldap_group = mapping_parts[0] psql_group = mapping_parts[1] - if psql_group in [*ACCESS_GROUPS, PERMISSIONS_GROUP_ADMIN]: + if psql_group in ACCESS_GROUPS: logger.warning(f"Tried to assign LDAP users to forbidden group: {psql_group}") continue diff --git a/src/charm.py b/src/charm.py index b5820d28c8..6e927f81ed 100755 --- a/src/charm.py +++ b/src/charm.py @@ -1724,8 +1724,6 @@ def _start_primary(self, event: StartEvent) -> None: # noqa: C901 # This event can be run on a replica if the machines are restarted. # For that case, check whether the postgres user already exits. users = self.postgresql.list_users() - if "postgres" not in users: - self.postgresql.create_user("postgres", new_password(), admin=True) # Create the backup user. if BACKUP_USER not in users: self.postgresql.create_user( diff --git a/tests/integration/new_relations/test_new_relations_1.py b/tests/integration/new_relations/test_new_relations_1.py index 194805798a..5be0beeef7 100644 --- a/tests/integration/new_relations/test_new_relations_1.py +++ b/tests/integration/new_relations/test_new_relations_1.py @@ -2,8 +2,6 @@ # See LICENSE file for licensing details. import asyncio import logging -import secrets -import string from pathlib import Path import psycopg2 @@ -484,97 +482,10 @@ async def test_relation_with_no_database_name(ops_test: OpsTest): await ops_test.model.wait_for_idle(apps=APP_NAMES, status="active", raise_on_blocked=True) -async def test_admin_role(ops_test: OpsTest): - """Test that the admin role gives access to all the databases.""" - all_app_names = [DATA_INTEGRATOR_APP_NAME] - all_app_names.extend(APP_NAMES) +async def test_invalid_extra_user_roles(ops_test: OpsTest): async with ops_test.fast_forward(): await ops_test.model.deploy(DATA_INTEGRATOR_APP_NAME, base=CHARM_BASE) await ops_test.model.wait_for_idle(apps=[DATA_INTEGRATOR_APP_NAME], status="blocked") - await ops_test.model.applications[DATA_INTEGRATOR_APP_NAME].set_config({ - "database-name": DATA_INTEGRATOR_APP_NAME.replace("-", "_"), - "extra-user-roles": "admin", - }) - await ops_test.model.wait_for_idle(apps=[DATA_INTEGRATOR_APP_NAME], status="blocked") - await ops_test.model.add_relation(DATA_INTEGRATOR_APP_NAME, DATABASE_APP_NAME) - await ops_test.model.wait_for_idle(apps=all_app_names, status="active") - - # Check that the user can access all the databases. - for database in [ - DATABASE_DEFAULT_NAME, - f"{APPLICATION_APP_NAME.replace('-', '_')}_database", - "another_application_database", - ]: - logger.info(f"connecting to the following database: {database}") - connection_string = await build_connection_string( - ops_test, DATA_INTEGRATOR_APP_NAME, "postgresql", database=database - ) - connection = None - should_fail = False - try: - with psycopg2.connect(connection_string) as connection, connection.cursor() as cursor: - # Check the version that the application received is the same on the - # database server. - cursor.execute("SELECT version();") - data = cursor.fetchone()[0].split(" ")[1] - - # Get the version of the database and compare with the information that - # was retrieved directly from the database. - version = await get_application_relation_data( - ops_test, DATA_INTEGRATOR_APP_NAME, "postgresql", "version" - ) - assert version == data - - # Write some data (it should fail in the default database name). - random_name = ( - f"test_{''.join(secrets.choice(string.ascii_lowercase) for _ in range(10))}" - ) - should_fail = database == DATABASE_DEFAULT_NAME - cursor.execute(f"CREATE SCHEMA test; CREATE TABLE test.{random_name}(data TEXT);") - if should_fail: - assert False, ( - f"failed to run a statement in the following database: {database}" - ) - except psycopg2.errors.InsufficientPrivilege as e: - if not should_fail: - logger.exception(e) - assert False, ( - f"failed to connect to or run a statement in the following database: {database}" - ) - finally: - if connection is not None: - connection.close() - - # Test the creation and deletion of databases. - connection_string = await build_connection_string( - ops_test, DATA_INTEGRATOR_APP_NAME, "postgresql", database=DATABASE_DEFAULT_NAME - ) - connection = psycopg2.connect(connection_string) - connection.autocommit = True - cursor = connection.cursor() - random_name = f"test_{''.join(secrets.choice(string.ascii_lowercase) for _ in range(10))}" - cursor.execute(f"CREATE DATABASE {random_name};") - cursor.execute(f"DROP DATABASE {random_name};") - try: - cursor.execute(f"DROP DATABASE {DATABASE_DEFAULT_NAME};") - assert False, ( - f"the admin extra user role was able to drop the `{DATABASE_DEFAULT_NAME}` system database" - ) - except psycopg2.errors.InsufficientPrivilege: - # Ignore the error, as the admin extra user role mustn't be able to drop - # the "postgres" system database. - pass - finally: - connection.close() - - -async def test_invalid_extra_user_roles(ops_test: OpsTest): - async with ops_test.fast_forward(): - # Remove the relation between the database and the first data integrator. - await ops_test.model.applications[DATABASE_APP_NAME].remove_relation( - DATABASE_APP_NAME, DATA_INTEGRATOR_APP_NAME - ) - await ops_test.model.wait_for_idle(apps=APP_NAMES, status="active", raise_on_blocked=True) another_data_integrator_app_name = f"another-{DATA_INTEGRATOR_APP_NAME}" data_integrator_apps_names = [DATA_INTEGRATOR_APP_NAME, another_data_integrator_app_name] diff --git a/tests/integration/new_relations/test_relations_coherence.py b/tests/integration/new_relations/test_relations_coherence.py index 74fd202fa6..15221bdae2 100644 --- a/tests/integration/new_relations/test_relations_coherence.py +++ b/tests/integration/new_relations/test_relations_coherence.py @@ -82,40 +82,6 @@ async def test_relations(ops_test: OpsTest, charm): f"{DATABASE_APP_NAME}:database", f"{DATA_INTEGRATOR_APP_NAME}:postgresql" ) - # Re-relation with admin role with checking permission - await ops_test.model.applications[DATA_INTEGRATOR_APP_NAME].set_config({ - "database-name": DATA_INTEGRATOR_APP_NAME.replace("-", "_"), - "extra-user-roles": "admin", - }) - await ops_test.model.wait_for_idle(apps=[DATA_INTEGRATOR_APP_NAME], status="blocked") - await ops_test.model.add_relation(DATA_INTEGRATOR_APP_NAME, DATABASE_APP_NAME) - await ops_test.model.wait_for_idle(apps=APP_NAMES, status="active") - - connection_string = await build_connection_string( - ops_test, - DATA_INTEGRATOR_APP_NAME, - "postgresql", - database=DATA_INTEGRATOR_APP_NAME.replace("-", "_"), - ) - try: - connection = psycopg2.connect(connection_string) - connection.autocommit = True - cursor = connection.cursor() - random_name = ( - f"test_{''.join(secrets.choice(string.ascii_lowercase) for _ in range(10))}" - ) - cursor.execute(f"CREATE DATABASE {random_name};") - except psycopg2.errors.InsufficientPrivilege: - assert False, ( - f"failed connect to {random_name} or run a statement in the following database" - ) - finally: - connection.close() - - await ops_test.model.applications[DATABASE_APP_NAME].remove_relation( - f"{DATABASE_APP_NAME}:database", f"{DATA_INTEGRATOR_APP_NAME}:postgresql" - ) - # Re-relation again with user role and checking write data await ops_test.model.applications[DATA_INTEGRATOR_APP_NAME].set_config({ "database-name": DATA_INTEGRATOR_APP_NAME.replace("-", "_"), diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index 86a479e315..57e014000c 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -669,7 +669,7 @@ def test_on_start(harness): # Then test the event of a correct cluster bootstrapping. _restart_services_after_reboot.reset_mock() harness.charm.on.start.emit() - assert _postgresql.create_user.call_count == 4 # Considering the previous failed call. + assert _postgresql.create_user.call_count == 3 # Considering the previous failed call. _oversee_users.assert_called_once() _enable_disable_extensions.assert_called_once() _set_primary_status_message.assert_called_once()