This is an automated email from the ASF dual-hosted git repository. sbp pushed a commit to branch sbp in repository https://gitbox.apache.org/repos/asf/tooling-trusted-releases.git
commit 990ebe2cd82787a76767b308ebf9c19b8521b44a Author: Sean B. Palmer <[email protected]> AuthorDate: Fri Apr 3 15:44:05 2026 +0100 Improve the permissions model around bulk key deletion for admins --- atr/admin/__init__.py | 44 ++++---------- atr/storage/writers/keys.py | 49 +++++++++++++++ tests/unit/test_admin_delete_committee_keys.py | 60 ++++++++++++++++++ tests/unit/test_keys_writer.py | 84 +++++++++++++++++++++++++- 4 files changed, 205 insertions(+), 32 deletions(-) diff --git a/atr/admin/__init__.py b/atr/admin/__init__.py index 31f7a5f8..5b65e6d8 100644 --- a/atr/admin/__init__.py +++ b/atr/admin/__init__.py @@ -36,7 +36,6 @@ import jwt import pydantic import quart import sqlalchemy -import sqlalchemy.orm as orm import sqlmodel import atr.blueprints.admin as admin @@ -341,37 +340,20 @@ async def delete_committee_keys_post( """ committee_key = delete_form.committee_key - async with db.session() as data: - committee_query = data.committee(key=committee_key) - via = sql.validate_instrumented_attribute - committee_query.query = committee_query.query.options( - orm.selectinload(via(sql.Committee.public_signing_keys)).selectinload(via(sql.PublicSigningKey.committees)) - ) - committee = await committee_query.get() - - if not committee: - await quart.flash(f"Committee '{committee_key}' not found.", "error") - return await session.redirect(delete_committee_keys_get) - - keys_to_check = list(committee.public_signing_keys) - if not keys_to_check: - await quart.flash(f"Committee '{committee_key}' has no keys.", "info") - return await session.redirect(delete_committee_keys_get) - - num_removed = len(committee.public_signing_keys) - committee.public_signing_keys.clear() - await data.flush() - - unused_deleted = 0 - for key_obj in keys_to_check: - if not key_obj.committees: - await data.delete(key_obj) - unused_deleted += 1 - - await data.commit() + try: + async with storage.write(session) as write: + waca = write.as_committee_admin(committee_key) + num_unlinked, num_deleted = await waca.keys.delete_committee_keys() + except storage.AccessError as e: + await quart.flash(str(e), "error") + return await session.redirect(delete_committee_keys_get) + + if num_unlinked == 0: + await quart.flash(f"Committee '{committee_key}' has no keys.", "info") + else: await quart.flash( - f"Removed {util.plural(num_removed, 'key link')} for '{committee_key}'. " - f"Deleted {util.plural(unused_deleted, 'unused key')}.", + f"Removed {util.plural(num_unlinked, 'key link')} for '{committee_key}'. " + f"Deleted {util.plural(num_deleted, 'unused key')}.", "success", ) diff --git a/atr/storage/writers/keys.py b/atr/storage/writers/keys.py index a2ec0973..2b7d1313 100644 --- a/atr/storage/writers/keys.py +++ b/atr/storage/writers/keys.py @@ -32,11 +32,13 @@ import aiofiles.os import pgpy import pgpy.constants as constants import sqlalchemy.dialects.sqlite as sqlite +import sqlalchemy.orm as orm import sqlmodel import atr.config as config import atr.db as db import atr.log as log +import atr.models.basic as basic import atr.models.safe as safe import atr.models.sql as sql import atr.paths as paths @@ -752,3 +754,50 @@ class FoundationAdmin(CommitteeMember): @property def committee_key(self) -> str: return self.__committee_key + + async def delete_committee_keys(self) -> tuple[int, int]: + via = sql.validate_instrumented_attribute + committee_query = self.__data.committee(key=self.__committee_key) + committee_query.query = committee_query.query.options( + orm.selectinload(via(sql.Committee.public_signing_keys)).selectinload(via(sql.PublicSigningKey.committees)) + ) + committee = await committee_query.demand(storage.AccessError(f"Committee not found: {self.__committee_key}")) + + keys_to_check = list(committee.public_signing_keys) + if not keys_to_check: + return (0, 0) + + num_unlinked = len(keys_to_check) + fingerprints: list[basic.JSON] = [key_obj.fingerprint for key_obj in keys_to_check] + committee.public_signing_keys.clear() + await self.__data.flush() + + num_deleted = 0 + for key_obj in keys_to_check: + if not key_obj.committees: + await self.__data.delete(key_obj) + num_deleted += 1 + + await self.__data.commit() + self.__write_as.append_to_audit_log( + asf_uid=self.__asf_uid, + committee_key=self.__committee_key, + keys_unlinked=num_unlinked, + keys_deleted=num_deleted, + fingerprints=fingerprints, + ) + try: + await self._sync_committee_keys_file(self.__committee_key) + except Exception as e: + self.__write_as.append_to_audit_log( + action="delete_committee_keys_sync_failed", + asf_uid=self.__asf_uid, + committee_key=self.__committee_key, + keys_unlinked=num_unlinked, + keys_deleted=num_deleted, + fingerprints=fingerprints, + error=str(e), + ) + raise + + return (num_unlinked, num_deleted) diff --git a/tests/unit/test_admin_delete_committee_keys.py b/tests/unit/test_admin_delete_committee_keys.py new file mode 100644 index 00000000..621514cd --- /dev/null +++ b/tests/unit/test_admin_delete_committee_keys.py @@ -0,0 +1,60 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +import contextlib +import unittest.mock as mock +from types import SimpleNamespace + +import pytest + +import atr.admin as admin_routes +import atr.blueprints.admin as admin_blueprint +import atr.storage as storage + + [email protected] +async def test_delete_committee_keys_post_surfaces_storage_error(monkeypatch): + session = mock.MagicMock() + session.redirect = mock.AsyncMock(return_value="redirected") + delete_form = admin_routes.DeleteCommitteeKeysForm( + committee_key="alpha", + confirm_delete="DELETE KEYS", + csrf_token="csrf", + ) + delete_error = storage.AccessError("Failed to remove KEYS file for committee alpha: boom") + keys = SimpleNamespace(delete_committee_keys=mock.AsyncMock(side_effect=delete_error)) + waca = SimpleNamespace(keys=keys) + write = SimpleNamespace(as_committee_admin=mock.MagicMock(return_value=waca)) + flash = mock.AsyncMock() + + @contextlib.asynccontextmanager + async def fake_write(_session): + yield write + + session.form_validate = mock.AsyncMock(return_value=delete_form) + + monkeypatch.setattr(admin_routes.storage, "write", fake_write) + monkeypatch.setattr(admin_routes.quart, "flash", flash) + monkeypatch.setattr(admin_blueprint.common, "authenticate", mock.AsyncMock(return_value=session)) + + result = await admin_routes.delete_committee_keys_post() + + assert result == "redirected" + write.as_committee_admin.assert_called_once_with("alpha") + keys.delete_committee_keys.assert_awaited_once() + flash.assert_awaited_once_with(str(delete_error), "error") + session.redirect.assert_awaited_once_with(admin_routes.delete_committee_keys_get) diff --git a/tests/unit/test_keys_writer.py b/tests/unit/test_keys_writer.py index 2b25079e..f60433b6 100644 --- a/tests/unit/test_keys_writer.py +++ b/tests/unit/test_keys_writer.py @@ -21,6 +21,7 @@ from types import SimpleNamespace import pytest import atr.models.safe as safe +import atr.storage as storage import atr.storage.outcome as outcome import atr.storage.writers.keys as keys_writer @@ -28,6 +29,7 @@ import atr.storage.writers.keys as keys_writer class Query: def __init__(self, value): self._value = value + self.query = mock.MagicMock() async def get(self): return self._value @@ -46,15 +48,88 @@ class MockData: self.commit = mock.AsyncMock() self.delete = mock.AsyncMock() self.execute = mock.AsyncMock() + self.flush = mock.AsyncMock() def public_signing_key(self, **_kwargs): return Query(self._key) def committee(self, *, key: str, _public_signing_keys: bool = False): - assert _public_signing_keys is True return Query(self._committees_after_commit[key]) [email protected] +async def test_delete_committee_keys_audits_committed_delete_when_sync_fails(): + key_orphaned = SimpleNamespace(fingerprint="fp1", committees=[]) + initial_committee = SimpleNamespace(public_signing_keys=[key_orphaned]) + data = MockData(None, committees_after_commit={"alpha": initial_committee}) + writer, write_as = _make_foundation_admin(data, "alpha") + error_message = "Failed to remove KEYS file for committee alpha: boom" + + with mock.patch.object( + writer, + "_sync_committee_keys_file", + new=mock.AsyncMock(side_effect=storage.AccessError(error_message)), + ): + with pytest.raises(storage.AccessError, match="boom"): + await writer.delete_committee_keys() + + data.commit.assert_awaited_once() + assert write_as.append_to_audit_log.call_count == 2 + + delete_audit = write_as.append_to_audit_log.call_args_list[0].kwargs + sync_failure_audit = write_as.append_to_audit_log.call_args_list[1].kwargs + + assert delete_audit["committee_key"] == "alpha" + assert delete_audit["keys_unlinked"] == 1 + assert delete_audit["keys_deleted"] == 1 + assert delete_audit["fingerprints"] == ["fp1"] + + assert sync_failure_audit["action"] == "delete_committee_keys_sync_failed" + assert sync_failure_audit["committee_key"] == "alpha" + assert sync_failure_audit["error"] == error_message + + [email protected] +async def test_delete_committee_keys_removes_links_and_orphaned_keys(): + key_orphaned = SimpleNamespace(fingerprint="fp1", committees=[]) + key_shared = SimpleNamespace(fingerprint="fp2", committees=[SimpleNamespace(key="beta")]) + initial_committee = SimpleNamespace(public_signing_keys=[key_orphaned, key_shared]) + data = MockData(None, committees_after_commit={"alpha": initial_committee}) + writer, write_as = _make_foundation_admin(data, "alpha") + + with mock.patch.object(writer, "_sync_committee_keys_file", new_callable=mock.AsyncMock) as mock_sync: + num_unlinked, num_deleted = await writer.delete_committee_keys() + + assert num_unlinked == 2 + assert num_deleted == 1 + data.delete.assert_awaited_once_with(key_orphaned) + data.flush.assert_awaited_once() + data.commit.assert_awaited_once() + mock_sync.assert_awaited_once_with("alpha") + write_as.append_to_audit_log.assert_called_once() + audit_kwargs = write_as.append_to_audit_log.call_args[1] + assert audit_kwargs["asf_uid"] == "alice" + assert audit_kwargs["committee_key"] == "alpha" + assert audit_kwargs["keys_unlinked"] == 2 + assert audit_kwargs["keys_deleted"] == 1 + assert set(audit_kwargs["fingerprints"]) == {"fp1", "fp2"} + + [email protected] +async def test_delete_committee_keys_returns_zero_when_no_keys(): + empty_committee = SimpleNamespace(public_signing_keys=[]) + data = MockData(None, committees_after_commit={"alpha": empty_committee}) + writer, write_as = _make_foundation_admin(data, "alpha") + + num_unlinked, num_deleted = await writer.delete_committee_keys() + + assert num_unlinked == 0 + assert num_deleted == 0 + data.delete.assert_not_awaited() + data.commit.assert_not_awaited() + write_as.append_to_audit_log.assert_not_called() + + @pytest.mark.asyncio async def test_delete_key_removal_deletes_empty_keys_file(tmp_path): owned_key = SimpleNamespace( @@ -147,6 +222,13 @@ def _committee(key: str, public_signing_keys: list[object], *, is_podling: bool ) +def _make_foundation_admin(data: MockData, committee_key: str): + write = mock.MagicMock() + write.authorisation.asf_uid = "alice" + write_as = mock.MagicMock() + return keys_writer.FoundationAdmin(write, write_as, data, committee_key), write_as + + def _make_foundation_committer(data: MockData): write = mock.MagicMock() write.authorisation.asf_uid = "alice" --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
