sbp commented on code in PR #1186: URL: https://github.com/apache/tooling-trusted-releases/pull/1186#discussion_r3124809329
########## tests/unit/test_archived_project_writer_guards.py: ########## @@ -0,0 +1,326 @@ +# 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 unittest.mock as mock +from types import SimpleNamespace + +import pytest + +import atr.models.safe as safe +import atr.models.sql as sql +import atr.storage as storage +import atr.storage.writers.announce as announce +import atr.storage.writers.checks as checks +import atr.storage.writers.distributions as distributions +import atr.storage.writers.release as release +import atr.storage.writers.revision as revision +import atr.storage.writers.sbom as sbom +import atr.storage.writers.vote as vote + + +def _retired_project(key: str = "project") -> SimpleNamespace: + return SimpleNamespace( + key=key, + status=sql.ProjectStatus.RETIRED, + committee_key=key, + display_name=key.capitalize(), + short_display_name=key.capitalize(), + ) + + +def _retired_release(project_key: str = "project", version: str = "1.0.0") -> SimpleNamespace: + return SimpleNamespace( + key=sql.release_key(project_key, version), + project=_retired_project(project_key), + version=version, + ) + + +def _query_returning(obj: object) -> mock.MagicMock: + query = mock.MagicMock() + query.demand = mock.AsyncMock(return_value=obj) + query.get = mock.AsyncMock(return_value=obj) + return query + + +def _async_context_manager(value: object) -> mock.MagicMock: + ctx = mock.MagicMock() + ctx.__aenter__ = mock.AsyncMock(return_value=value) + ctx.__aexit__ = mock.AsyncMock(return_value=None) + return ctx + + +# --- revision -------------------------------------------------------------- Review Comment: Section comments break the function ordering script. Perhaps the function ordering script should be fixed to accommodate these, rather than removing all comments everywhere, though! ########## atr/api/__init__.py: ########## @@ -1661,6 +1665,54 @@ async def vote_tabulate( ).model_dump(mode="json"), 200 +class _TestProjectStatusArgs(pydantic.BaseModel): Review Comment: Whoops. We never prefix classes with an underscore, but I notice that our code conventions don't presently reflect that. (They only mention type variables.) I'll update the conventions. ########## tests/unit/test_create_revision.py: ########## @@ -111,6 +110,7 @@ async def test_clone_from_older_revision_skips_merge_without_intervening_change( release = mock.MagicMock() release.phase = sql.ReleasePhase.RELEASE_PREVIEW release.project = mock.MagicMock() + release.project.status = sql.ProjectStatus.ACTIVE Review Comment: These tests now fail: ``` tests/unit/test_create_revision.py ..F.F........ [ 27%] ``` With problems in `test_intervening_revision_with_path_provenance_verifies_sources` and `test_modify_path_provenance_flows_into_write_files_data`. ########## tests/unit/test_archived_project_writer_guards.py: ########## @@ -0,0 +1,326 @@ +# 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 unittest.mock as mock +from types import SimpleNamespace + +import pytest + +import atr.models.safe as safe +import atr.models.sql as sql +import atr.storage as storage +import atr.storage.writers.announce as announce +import atr.storage.writers.checks as checks +import atr.storage.writers.distributions as distributions +import atr.storage.writers.release as release +import atr.storage.writers.revision as revision +import atr.storage.writers.sbom as sbom +import atr.storage.writers.vote as vote + + +def _retired_project(key: str = "project") -> SimpleNamespace: + return SimpleNamespace( + key=key, + status=sql.ProjectStatus.RETIRED, + committee_key=key, + display_name=key.capitalize(), + short_display_name=key.capitalize(), + ) + + +def _retired_release(project_key: str = "project", version: str = "1.0.0") -> SimpleNamespace: + return SimpleNamespace( + key=sql.release_key(project_key, version), + project=_retired_project(project_key), + version=version, + ) + + +def _query_returning(obj: object) -> mock.MagicMock: + query = mock.MagicMock() + query.demand = mock.AsyncMock(return_value=obj) + query.get = mock.AsyncMock(return_value=obj) + return query + + +def _async_context_manager(value: object) -> mock.MagicMock: + ctx = mock.MagicMock() + ctx.__aenter__ = mock.AsyncMock(return_value=value) + ctx.__aexit__ = mock.AsyncMock(return_value=None) + return ctx + + +# --- revision -------------------------------------------------------------- + + [email protected] +async def test_revision_create_revision_with_quarantine_blocks_retired( + monkeypatch: pytest.MonkeyPatch, +) -> None: + writer = object.__new__(revision.CommitteeParticipant) + writer._CommitteeParticipant__data = mock.MagicMock() + writer._CommitteeParticipant__asf_uid = "tester" + writer._CommitteeParticipant__committee_key = "project" + + inner_data = mock.MagicMock() + inner_data.release = mock.MagicMock(return_value=_query_returning(_retired_release())) + monkeypatch.setattr(revision.db, "session", lambda: _async_context_manager(inner_data)) + + with pytest.raises(storage.AccessError, match="archived"): + await writer.create_revision_with_quarantine( + safe.ProjectKey("project"), + safe.VersionKey("1.0.0"), + "tester", + allowed_phases=frozenset({sql.ReleasePhase.RELEASE_CANDIDATE_DRAFT}), + ) + + [email protected] +async def test_revision_clear_quarantine_blocks_retired() -> None: + data = mock.MagicMock() + data.project = mock.MagicMock(return_value=_query_returning(_retired_project())) + writer = object.__new__(revision.CommitteeParticipant) + writer._CommitteeParticipant__data = data + writer._CommitteeParticipant__asf_uid = "tester" + writer._CommitteeParticipant__committee_key = "project" + + with pytest.raises(storage.AccessError, match="archived"): + await writer.clear_quarantine( + safe.ProjectKey("project"), + safe.VersionKey("1.0.0"), + 42, + ) + + +# --- release --------------------------------------------------------------- + + [email protected] +async def test_release_delete_blocks_retired() -> None: + data = mock.MagicMock() + data.release = mock.MagicMock(return_value=_query_returning(_retired_release())) + writer = object.__new__(release.CommitteeParticipant) + writer._CommitteeParticipant__data = data + writer._CommitteeParticipant__asf_uid = "tester" + writer._CommitteeParticipant__committee_key = "project" + + with pytest.raises(storage.AccessError, match="archived"): + await writer.delete(safe.ProjectKey("project"), safe.VersionKey("1.0.0")) + + [email protected] +async def test_release_import_from_svn_blocks_retired() -> None: + data = mock.MagicMock() + data.project = mock.MagicMock(return_value=_query_returning(_retired_project())) + writer = object.__new__(release.CommitteeParticipant) + writer._CommitteeParticipant__data = data + writer._CommitteeParticipant__asf_uid = "tester" + writer._CommitteeParticipant__committee_key = "project" + + with pytest.raises(storage.AccessError, match="archived"): + await writer.import_from_svn( + safe.ProjectKey("project"), + safe.VersionKey("1.0.0"), + safe.RelPath("svn/url"), + "r1", + None, + ) + + [email protected] +async def test_release_promote_to_candidate_blocks_retired() -> None: + data = mock.MagicMock() + data.release = mock.MagicMock(return_value=_query_returning(_retired_release())) + writer = object.__new__(release.CommitteeParticipant) + writer._CommitteeParticipant__data = data + writer._CommitteeParticipant__asf_uid = "tester" + writer._CommitteeParticipant__committee_key = "project" + + with pytest.raises(storage.AccessError, match="archived"): + await writer.promote_to_candidate( + safe.ReleaseKey("project-1.0.0"), + safe.RevisionNumber("00001"), + ) + + +# --- vote ------------------------------------------------------------------ + + [email protected] +async def test_vote_resolve_release_blocks_retired_after_merge() -> None: + data = mock.MagicMock() + merged = _retired_release() + data.merge = mock.AsyncMock(return_value=merged) + writer = object.__new__(vote.CommitteeMember) + writer._CommitteeMember__data = data + writer._CommitteeMember__asf_uid = "tester" + writer._CommitteeMember__committee_key = "project" + + stub_release = SimpleNamespace(project=SimpleNamespace(status=sql.ProjectStatus.ACTIVE, key="project")) + with pytest.raises(storage.AccessError, match="archived"): + await writer.resolve_release( + safe.ProjectKey("project"), + stub_release, + None, + "passed", + SimpleNamespace(), + "Chair", + "body", + ) + data.merge.assert_awaited_once_with(stub_release) + + [email protected] +async def test_vote_send_resolution_blocks_retired() -> None: + writer = object.__new__(vote.CommitteeMember) + writer._CommitteeMember__data = mock.MagicMock() + writer._CommitteeMember__asf_uid = "tester" + writer._CommitteeMember__committee_key = "project" + + rel = _retired_release() + with pytest.raises(storage.AccessError, match="archived"): + await writer.send_resolution( + rel, + "passed", + "body", + "tester", + "Chair", + SimpleNamespace(), + ) + + +# --- announce -------------------------------------------------------------- + + [email protected] +async def test_announce_release_blocks_retired(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr( + announce.util, + "permitted_announce_recipients", + lambda _uid: {"[email protected]"}, + ) + data = mock.MagicMock() + data.release = mock.MagicMock(return_value=_query_returning(_retired_release())) + writer = object.__new__(announce.CommitteeMember) + writer._CommitteeMember__data = data + writer._CommitteeMember__asf_uid = "tester" + writer._CommitteeMember__committee_key = "project" + writer._CommitteeParticipant__data = data + writer._CommitteeParticipant__committee_key = "project" + + with pytest.raises(storage.AccessError, match="archived"): + await writer.release( + safe.ProjectKey("project"), + safe.VersionKey("1.0.0"), + safe.RevisionNumber("00001"), + "[email protected]", + "body", + None, + "tester", + "Chair", + ) + + +# --- distributions --------------------------------------------------------- + + [email protected] +async def test_distributions_automate_blocks_retired() -> None: + data = mock.MagicMock() + data.project = mock.MagicMock(return_value=_query_returning(_retired_project())) + writer = object.__new__(distributions.CommitteeMember) + writer._CommitteeMember__data = data + writer._CommitteeMember__asf_uid = "tester" + writer._CommitteeMember__committee_key = "project" + + platform = next(iter(sql.DistributionPlatform)) + with pytest.raises(storage.AccessError, match="archived"): + await writer.automate( + safe.ReleaseKey("project-1.0.0"), + platform, + "project", + None, + safe.ProjectKey("project"), + safe.VersionKey("1.0.0"), + "release", + None, + safe.Alphanumeric("pkg"), + safe.VersionKey("1.0.0"), + False, + ) + + [email protected] +async def test_distributions_record_blocks_retired_via_release_lookup() -> None: + data = mock.MagicMock() + data.release = mock.MagicMock(return_value=_query_returning(_retired_release())) + writer = object.__new__(distributions.CommitteeMember) + writer._CommitteeMember__data = data + writer._CommitteeMember__asf_uid = "tester" + writer._CommitteeMember__committee_key = "project" + + platform = next(iter(sql.DistributionPlatform)) + with pytest.raises(storage.AccessError, match="archived"): + await writer.record( + safe.ReleaseKey("project-1.0.0"), + platform, + None, + safe.Alphanumeric("pkg"), + safe.VersionKey("1.0.0"), + False, + False, + None, + ) + + +# --- sbom ------------------------------------------------------------------ + + [email protected] +async def test_sbom_augment_blocks_retired() -> None: + data = mock.MagicMock() + data.project = mock.MagicMock(return_value=_query_returning(_retired_project())) + writer = object.__new__(sbom.CommitteeParticipant) + writer._CommitteeParticipant__data = data + writer._CommitteeParticipant__asf_uid = "tester" + writer._CommitteeParticipant__committee_key = "project" + + with pytest.raises(storage.AccessError, match="archived"): + await writer.augment_cyclonedx( + safe.ProjectKey("project"), + safe.VersionKey("1.0.0"), + safe.RevisionNumber("00001"), + safe.RelPath("artifact.tar.gz"), + ) + + +# --- checks ---------------------------------------------------------------- + + [email protected] +async def test_checks_ignore_add_propagates_chokepoint_block() -> None: + data = mock.MagicMock() + data.project = mock.MagicMock(return_value=_query_returning(_retired_project())) + writer = object.__new__(checks.CommitteeMember) + writer._CommitteeMember__data = data Review Comment: Presumably only one or the other is needed? (`CommitteeMember` vs `CommitteeParticipant`) ########## playwright/test.py: ########## @@ -573,6 +579,46 @@ def test_all(page: Page, credentials: Credentials, skip_slow: bool) -> None: logging.info(f"Tests took {round(finish - start, 3)} seconds") +def test_archive_01_banner_absent_when_active(page: Page, credentials: Credentials) -> None: Review Comment: Not sure we necessarily need Playwright tests for this feature. Could they be e2e tests? Also, if these crash half way through, then I think we get stuck with a `RETIRED` project and then all later runs will break. (This is why I had to add all the other state resetting stuff in these tests.) ########## tests/unit/test_archived_project_writer_guards.py: ########## @@ -0,0 +1,326 @@ +# 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 unittest.mock as mock +from types import SimpleNamespace + +import pytest + +import atr.models.safe as safe +import atr.models.sql as sql +import atr.storage as storage +import atr.storage.writers.announce as announce +import atr.storage.writers.checks as checks +import atr.storage.writers.distributions as distributions +import atr.storage.writers.release as release +import atr.storage.writers.revision as revision +import atr.storage.writers.sbom as sbom +import atr.storage.writers.vote as vote + + +def _retired_project(key: str = "project") -> SimpleNamespace: + return SimpleNamespace( + key=key, + status=sql.ProjectStatus.RETIRED, + committee_key=key, + display_name=key.capitalize(), + short_display_name=key.capitalize(), + ) + + +def _retired_release(project_key: str = "project", version: str = "1.0.0") -> SimpleNamespace: + return SimpleNamespace( + key=sql.release_key(project_key, version), + project=_retired_project(project_key), + version=version, + ) + + +def _query_returning(obj: object) -> mock.MagicMock: + query = mock.MagicMock() + query.demand = mock.AsyncMock(return_value=obj) + query.get = mock.AsyncMock(return_value=obj) + return query + + +def _async_context_manager(value: object) -> mock.MagicMock: + ctx = mock.MagicMock() + ctx.__aenter__ = mock.AsyncMock(return_value=value) + ctx.__aexit__ = mock.AsyncMock(return_value=None) + return ctx + + +# --- revision -------------------------------------------------------------- Review Comment: But note also that although these tests are nicely group-prefixed, they're also in entirely the wrong order. ########## atr/util.py: ########## @@ -382,6 +360,16 @@ def email_from_uid(uid: str) -> str | None: return None +def ensure_project_active(project: sql.Project) -> None: Review Comment: This function is only ever used in `storage`, and imports `storage`, so I think it would be happier in `storage` somewhere. Probably just at the root is fine, or whatever makes sense. ########## tests/e2e/tokens/helpers.py: ########## @@ -26,7 +26,7 @@ def delete_token_by_label(page: Page, label: str) -> None: row = get_token_row_by_label(page, label) if row.count() > 0: - row.get_by_role("button", name="Delete").click() Review Comment: Why did these get renamed? I don't see the corresponding change elsewhere. ########## atr/storage/writers/project.py: ########## @@ -186,17 +186,55 @@ async def create(self, committee_key: safe.CommitteeKey, display_name: str, labe else: raise + async def archive(self, project_key: safe.ProjectKey) -> None: + project = await self.__data.project(key=str(project_key), status=sql.ProjectStatus.ACTIVE).get() + + if not project: + raise storage.AccessError(f"Project '{project_key}' not found.") + + if project.committee_key: + committee_projects = await self.__data.project( + committee_key=project.committee_key, status=sql.ProjectStatus.ACTIVE Review Comment: This correctly checks that the `status` is `ACTIVE`, but e.g. `category_add` and `category_remove` in this file don't have those checks. I may have forgotten to add the checks elsewhere, too. ########## atr/storage/writers/project.py: ########## @@ -186,17 +186,55 @@ async def create(self, committee_key: safe.CommitteeKey, display_name: str, labe else: raise + async def archive(self, project_key: safe.ProjectKey) -> None: Review Comment: This function doesn't seem to have any protections. What if there's a release in progress, e.g. holding a vote or something? Shouldn't the function hold a lock (or do some complex atomic query) and ensure that there are no active releases? I notice it doesn't actually say that in https://github.com/apache/tooling-trusted-releases/issues/510 but it would seem like it would cause a problem if a project were archived when it was mid-release. ########## atr/get/projects.py: ########## @@ -94,28 +95,50 @@ async def add_project( @get.typed -async def projects(_session: web.Public, _projects: Literal["projects"]) -> str: +async def projects(session: web.Public, _projects: Literal["projects"]) -> str: """ URL: /projects Main project directory page. """ async with db.session() as data: - projects = await data.project(_committee=True).order_by(sql.Project.name).all() - - delete_forms: dict[str, htm.Element] = {} - for project in projects: - delete_forms[str(project.key)] = await form.render( - model_cls=shared.projects.DeleteSelectedProject, - action=util.as_url(post.projects.delete), - form_classes=".d-inline-block.m-0", - submit_classes="btn-sm btn-outline-danger", - submit_label="Delete project", - empty=True, - defaults={"project_key": str(project.key)}, - confirm="Are you sure you want to delete this project? This cannot be undone.", - ) + projects = await data.project(_committee=True, _releases=True).order_by(sql.Project.name).all() + + committee_project_counts: Counter[str] = Counter(str(p.committee.key) for p in projects if p.committee) Review Comment: Does this need to count `ACTIVE` projects only? Do we even need to track inactive projects in the database, necessarily? Perhaps we could have a different table for them or something. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
