From 4c8a657b529eac422968764b82b124ba402155d6 Mon Sep 17 00:00:00 2001 From: Dragomir Penev Date: Mon, 21 Apr 2025 17:18:02 +0300 Subject: [PATCH 1/3] Make username mandatory --- src/charm.py | 13 ++++++++----- tests/unit/test_charm.py | 22 +++++++++++++++------- 2 files changed, 23 insertions(+), 12 deletions(-) diff --git a/src/charm.py b/src/charm.py index d2c2b1ba5a..c3abf14b81 100755 --- a/src/charm.py +++ b/src/charm.py @@ -1424,7 +1424,9 @@ def _on_get_password(self, event: ActionEvent) -> None: If no user is provided, the password of the operator user is returned. """ - username = event.params.get("username", USER) + if not (username := event.params.get("username")): + event.fail("The action requires a username") + return if username not in PASSWORD_USERS and self.is_ldap_enabled: event.fail("The action can be run only for system users when LDAP is enabled") return @@ -1444,13 +1446,14 @@ def _on_set_password(self, event: ActionEvent) -> None: event.fail("The action can be run only on leader unit") return - username = event.params.get("username", USER) - if username not in SYSTEM_USERS and self.is_ldap_enabled: - event.fail("The action can be run only for system users when LDAP is enabled") + if not (username := event.params.get("username")): + event.fail("The action requires a username") return if username not in SYSTEM_USERS: event.fail( - f"The action can be run only for system users:" + "The action can be run only for system users when LDAP is enabled" + if self.is_ldap_enabled + else "The action can be run only for system users:" f" {', '.join(SYSTEM_USERS)} not {username}" ) return diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index 025ab68e01..fa90fe99b0 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -838,7 +838,8 @@ def test_on_get_password(harness): mock_event.reset_mock() del mock_event.params["username"] harness.charm._on_get_password(mock_event) - mock_event.set_results.assert_called_once_with({"password": "test-password"}) + mock_event.fail.assert_called_once() + mock_event.set_results.assert_not_called() # Also test providing the username option. mock_event.reset_mock() @@ -854,14 +855,16 @@ def test_on_set_password(harness): patch("charm.PostgresqlOperatorCharm.postgresql") as _postgresql, patch("charm.Patroni.are_all_members_ready") as _are_all_members_ready, patch("charm.PostgresqlOperatorCharm._on_leader_elected"), + patch("charm.new_password", return_value="newpass"), ): # Create a mock event. mock_event = MagicMock(params={}) # Set some values for the other mocks. - _are_all_members_ready.side_effect = [False, True, True, True, True] - _postgresql.update_user_password = PropertyMock( - side_effect=[PostgreSQLUpdateUserPasswordError, None, None, None] + _are_all_members_ready.return_value = False + _postgresql.update_user_password = PropertyMock() + _postgresql.update_user_password.return_value.side_effect = ( + PostgreSQLUpdateUserPasswordError ) # Test trying to set a password through a non leader unit. @@ -878,6 +881,7 @@ def test_on_set_password(harness): _set_secret.assert_not_called() # Test without providing the username option but without all cluster members ready. + _are_all_members_ready.return_value = True mock_event.reset_mock() del mock_event.params["username"] harness.charm._on_set_password(mock_event) @@ -891,14 +895,17 @@ def test_on_set_password(harness): _set_secret.assert_not_called() # Test without providing the username option. + mock_event.reset_mock() + _postgresql.update_user_password.return_value.side_effect = None harness.charm._on_set_password(mock_event) - assert _set_secret.call_args_list[0][0][1] == "operator-password" + mock_event.fail.assert_called_once() + _set_secret.assert_not_called() # Also test providing the username option. _set_secret.reset_mock() mock_event.params["username"] = "replication" harness.charm._on_set_password(mock_event) - assert _set_secret.call_args_list[0][0][1] == "replication-password" + _set_secret.assert_called_once_with("app", "replication-password", "newpass") # And test providing both the username and password options. _set_secret.reset_mock() @@ -2129,7 +2136,8 @@ def test_on_get_password_secrets(harness): mock_event.reset_mock() del mock_event.params["username"] harness.charm._on_get_password(mock_event) - mock_event.set_results.assert_called_once_with({"password": "test-password"}) + mock_event.fail.assert_called_once() + mock_event.set_results.assert_not_called() # Also test providing the username option. mock_event.reset_mock() From 1558f3d236bc3f59a1d1ea1c1815149e71bec978 Mon Sep 17 00:00:00 2001 From: Dragomir Penev Date: Mon, 21 Apr 2025 21:13:42 +0300 Subject: [PATCH 2/3] Second get password method --- tests/integration/ha_tests/helpers.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/integration/ha_tests/helpers.py b/tests/integration/ha_tests/helpers.py index c22cc85c2c..d3274f4aa0 100644 --- a/tests/integration/ha_tests/helpers.py +++ b/tests/integration/ha_tests/helpers.py @@ -525,7 +525,9 @@ async def get_password(ops_test: OpsTest, app: str, down_unit: str | None = None if unit.name != down_unit: unit_name = unit.name break - action = await ops_test.model.units.get(unit_name).run_action("get-password") + action = await ops_test.model.units.get(unit_name).run_action( + "get-password", username="operator" + ) action = await action.wait() return action.results["password"] From 46c88d9ed57ee27f0661a8bda383f75bd00091fc Mon Sep 17 00:00:00 2001 From: Dragomir Penev Date: Thu, 24 Apr 2025 19:07:28 +0300 Subject: [PATCH 3/3] Default in get-password --- src/charm.py | 4 +--- tests/integration/ha_tests/helpers.py | 4 +--- tests/unit/test_charm.py | 6 ++---- 3 files changed, 4 insertions(+), 10 deletions(-) diff --git a/src/charm.py b/src/charm.py index 339a428964..da921284c4 100755 --- a/src/charm.py +++ b/src/charm.py @@ -1487,9 +1487,7 @@ def _on_get_password(self, event: ActionEvent) -> None: If no user is provided, the password of the operator user is returned. """ - if not (username := event.params.get("username")): - event.fail("The action requires a username") - return + username = event.params.get("username", USER) if username not in PASSWORD_USERS and self.is_ldap_enabled: event.fail("The action can be run only for system users when LDAP is enabled") return diff --git a/tests/integration/ha_tests/helpers.py b/tests/integration/ha_tests/helpers.py index d3274f4aa0..c22cc85c2c 100644 --- a/tests/integration/ha_tests/helpers.py +++ b/tests/integration/ha_tests/helpers.py @@ -525,9 +525,7 @@ async def get_password(ops_test: OpsTest, app: str, down_unit: str | None = None if unit.name != down_unit: unit_name = unit.name break - action = await ops_test.model.units.get(unit_name).run_action( - "get-password", username="operator" - ) + action = await ops_test.model.units.get(unit_name).run_action("get-password") action = await action.wait() return action.results["password"] diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index c4c5875c35..a81aa7eb35 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -838,8 +838,7 @@ def test_on_get_password(harness): mock_event.reset_mock() del mock_event.params["username"] harness.charm._on_get_password(mock_event) - mock_event.fail.assert_called_once() - mock_event.set_results.assert_not_called() + mock_event.set_results.assert_called_once_with({"password": "test-password"}) # Also test providing the username option. mock_event.reset_mock() @@ -2165,8 +2164,7 @@ def test_on_get_password_secrets(harness): mock_event.reset_mock() del mock_event.params["username"] harness.charm._on_get_password(mock_event) - mock_event.fail.assert_called_once() - mock_event.set_results.assert_not_called() + mock_event.set_results.assert_called_once_with({"password": "test-password"}) # Also test providing the username option. mock_event.reset_mock()