From b6837dd6a976798b2830f16011977bd3029921d0 Mon Sep 17 00:00:00 2001 From: Jasper Mayone Date: Mon, 12 Oct 2026 13:09:59 -0400 Subject: [PATCH 1/4] feat: read minor and guardian_verified from the weave directory --- krater/weave/live.py | 3 +++ krater/weave/stub.py | 33 +++++++++++++++++++++++++------ krater/weave/stub_users.json | 30 ++++++++++++++++++++++++++++ krater/weave/types.py | 9 ++++++++- tests/weave/test_live_client.py | 35 +++++++++++++++++++++++++++++++++ tests/weave/test_stub_client.py | 21 ++++++++++++++++++++ 6 files changed, 124 insertions(+), 7 deletions(-) diff --git a/krater/weave/live.py b/krater/weave/live.py index ede96e6..0199ed9 100644 --- a/krater/weave/live.py +++ b/krater/weave/live.py @@ -285,6 +285,9 @@ def _parse_user(self, data: object) -> WeaveUser: roles=self._roles.krater_roles(roles=data.get("roles"), groups=data.get("groups")), # Fail closed: only an explicit `true` counts as active. active=data.get("active") is True, + # Only a real boolean counts; a missing or malformed value is "Weave didn't say". + minor=_optional_bool(data.get("minor")), + guardian_verified=_optional_bool(data.get("guardian_verified")), ) diff --git a/krater/weave/stub.py b/krater/weave/stub.py index 89b8aa0..50ea982 100644 --- a/krater/weave/stub.py +++ b/krater/weave/stub.py @@ -5,10 +5,13 @@ picker page; `exchange_code` treats the authorization code as the chosen user's `sub` directly. Each fixture user has the same fields a real Weave sends: `sub`, `name`, `email`, `email_verified`, -`slack_id`, optional `slack_member`, `groups` (group slugs) and `roles` (Weave role keys; leave it out -to exercise the group-slug fallback). Sign-in and the directory lookups translate them with the same -`RoleMapping` the live client uses. Tests change a user mid-test with `set_roles`, `set_active` or -`remove_user`. The app refuses stub mode in production. +`slack_id`, optional `slack_member`, `groups` (group slugs), `roles` (Weave role keys; leave it out +to exercise the group-slug fallback), and optional `minor` and `guardian_verified` (leave them out +for "Weave didn't say"). +Sign-in and the directory lookups translate them with the same `RoleMapping` the live client uses. +Tests change a user mid-test with `set_roles`, `set_active`, `set_minor`, `set_guardian_verified` +or `remove_user`. The app +refuses stub mode in production. """ from __future__ import annotations @@ -44,6 +47,8 @@ class StubUser: groups: tuple[str, ...] roles: tuple[str, ...] | None # None: Weave sends no `roles` field active: bool + minor: bool | None = None # None: Weave sends no `minor` field + guardian_verified: bool | None = None # None: Weave sends no `guardian_verified` field class StubWeaveClient: @@ -71,6 +76,8 @@ def __init__(self, stub_users_file: str | Path | None = None, *, role_mapping: R groups=tuple(entry.get("groups", [])), roles=None if raw_roles is None else tuple(raw_roles), active=entry.get("active", True), + minor=entry.get("minor"), + guardian_verified=entry.get("guardian_verified"), ) self._users_by_sub[user.sub] = user @@ -130,9 +137,11 @@ def put_user( roles: Iterable[str] | None = (), groups: Iterable[str] = (), active: bool = True, + minor: bool | None = None, + guardian_verified: bool | None = None, ) -> None: - """Add or replace a user. `roles` are Weave role keys (`None` for no `roles` field at all) and - `groups` are group slugs, as a real Weave would send them.""" + """Add or replace a user. `roles` are Weave role keys (`None` for no `roles` field at all), + `groups` are group slugs, and `minor` or `guardian_verified` is `None` when Weave sends no such field.""" self._users_by_sub[sub] = StubUser( sub=sub, name=name, @@ -143,6 +152,8 @@ def put_user( groups=tuple(groups), roles=None if roles is None else tuple(roles), active=active, + minor=minor, + guardian_verified=guardian_verified, ) def set_roles(self, sub: str, roles: Iterable[str] | None, *, groups: Iterable[str] | None = None) -> None: @@ -158,6 +169,14 @@ def set_active(self, sub: str, active: bool) -> None: """Lock (`False`) or unlock a user, as a Weave admin would.""" self._users_by_sub[sub] = dataclasses.replace(self._users_by_sub[sub], active=active) + def set_minor(self, sub: str, minor: bool | None) -> None: + """Change what Weave says about `sub`'s age: `True` (under 18), `False`, or `None` (no field).""" + self._users_by_sub[sub] = dataclasses.replace(self._users_by_sub[sub], minor=minor) + + def set_guardian_verified(self, sub: str, verified: bool | None) -> None: + """Change whether Weave says a Weave admin verified `sub`'s guardian (`None`: no field).""" + self._users_by_sub[sub] = dataclasses.replace(self._users_by_sub[sub], guardian_verified=verified) + def remove_user(self, sub: str) -> None: """Make the directory answer 404 for `sub`, as Weave does for a user Krater may not see.""" self._users_by_sub.pop(sub, None) @@ -172,6 +191,8 @@ def _record(self, user: StubUser) -> WeaveUser: slack_member=user.slack_member, roles=self._mapping.krater_roles(roles=user.roles, groups=user.groups), active=user.active, + minor=user.minor, + guardian_verified=user.guardian_verified, ) diff --git a/krater/weave/stub_users.json b/krater/weave/stub_users.json index ffee211..26d4669 100644 --- a/krater/weave/stub_users.json +++ b/krater/weave/stub_users.json @@ -5,6 +5,7 @@ "email": "mia@example.com", "email_verified": true, "slack_id": "U0001MEMBER", + "minor": false, "groups": ["ganymede-members"], "roles": ["member"] }, @@ -23,6 +24,7 @@ "email": "rae@example.com", "email_verified": true, "slack_id": "U0002REVIEW", + "minor": false, "groups": ["ganymede-members", "krater-reviewers"], "roles": ["member", "reviewer"] }, @@ -40,6 +42,7 @@ "email": "ana@example.com", "email_verified": true, "slack_id": "U0004ADMIN", + "minor": false, "groups": ["ganymede-members", "krater-reviewers", "krater-admins"], "roles": ["member", "reviewer", "admin"] }, @@ -49,6 +52,7 @@ "email": "nico@example.com", "email_verified": true, "slack_id": null, + "minor": false, "groups": ["ganymede-members"], "roles": [] }, @@ -58,6 +62,7 @@ "email": "uma@example.com", "email_verified": false, "slack_id": null, + "minor": false, "groups": [], "roles": ["member"] }, @@ -68,6 +73,31 @@ "email_verified": true, "slack_id": "U0005GUEST", "slack_member": false, + "minor": false, + "groups": ["ganymede-members"], + "roles": ["member"] + }, + { + "sub": "PWLMINORONE", + "name": "Milo Minor", + "email": "milo@example.com", + "email_verified": true, + "slack_id": "U0006MINOR", + "slack_member": true, + "minor": true, + "guardian_verified": false, + "groups": ["ganymede-members"], + "roles": ["member"] + }, + { + "sub": "PWLMINORVERIFIED", + "name": "Vera Verified", + "email": "vera@example.com", + "email_verified": true, + "slack_id": "U0007MINOR", + "slack_member": true, + "minor": true, + "guardian_verified": true, "groups": ["ganymede-members"], "roles": ["member"] } diff --git a/krater/weave/types.py b/krater/weave/types.py index 296a262..0b3003d 100644 --- a/krater/weave/types.py +++ b/krater/weave/types.py @@ -30,7 +30,12 @@ class WeaveIdentity: @dataclass(frozen=True) class WeaveUser: """A directory record. `roles` holds Krater role names. `slack_member` is `None` when Weave - didn't say. `active` is false for an account Weave has locked or deactivated.""" + didn't say. `active` is false for an account Weave has locked or deactivated. + + `minor` is true when Weave knows the member is under 18, false when Weave knows they are not, and + `None` when Weave didn't say. `guardian_verified` is true when a Weave admin recorded a signed + guardian form plus a call or video check, and `None` when Weave didn't say. Older Weave versions + leave both fields out. Krater's guardian gate (`krater.services.guardian`) reads them.""" sub: str name: str @@ -40,3 +45,5 @@ class WeaveUser: slack_member: bool | None roles: frozenset[str] active: bool + minor: bool | None = None + guardian_verified: bool | None = None diff --git a/tests/weave/test_live_client.py b/tests/weave/test_live_client.py index 962e48c..eac4e52 100644 --- a/tests/weave/test_live_client.py +++ b/tests/weave/test_live_client.py @@ -329,6 +329,41 @@ def test_get_user_parses_a_directory_record(fake_weave: FakeWeave, live_client: ) +def test_get_user_reads_the_minor_and_guardian_verified_fields( + fake_weave: FakeWeave, live_client: LiveWeaveClient +) -> None: + fake_weave.directory_users["PWLDIR0001"] = {**DIRECTORY_USER, "minor": True, "guardian_verified": False} + + user = live_client.get_user("PWLDIR0001") + + assert user is not None + assert user.minor is True + assert user.guardian_verified is False + + +def test_a_record_without_the_age_fields_says_nothing_about_age( + fake_weave: FakeWeave, live_client: LiveWeaveClient +) -> None: + # Older Weave versions leave both fields out. + fake_weave.directory_users["PWLDIR0001"] = DIRECTORY_USER + + user = live_client.get_user("PWLDIR0001") + + assert user is not None + assert user.minor is None + assert user.guardian_verified is None + + +def test_non_boolean_age_fields_count_as_missing(fake_weave: FakeWeave, live_client: LiveWeaveClient) -> None: + fake_weave.directory_users["PWLDIR0001"] = {**DIRECTORY_USER, "minor": "yes", "guardian_verified": 1} + + user = live_client.get_user("PWLDIR0001") + + assert user is not None + assert user.minor is None + assert user.guardian_verified is None + + def test_get_user_answers_none_on_404(live_client: LiveWeaveClient) -> None: assert live_client.get_user("PWLNOBODY") is None diff --git a/tests/weave/test_stub_client.py b/tests/weave/test_stub_client.py index e8c40c3..12751eb 100644 --- a/tests/weave/test_stub_client.py +++ b/tests/weave/test_stub_client.py @@ -106,3 +106,24 @@ def test_the_stub_quilt_token_is_a_fixed_fake_and_counts_calls() -> None: assert stub.quilt_token() == STUB_QUILT_TOKEN stub.invalidate_quilt_token() assert (stub.quilt_token_requests, stub.quilt_token_invalidations) == (1, 1) + + +def test_the_fixture_has_a_minor_with_and_without_a_verified_guardian(stub_client: StubWeaveClient) -> None: + unverified = stub_client.get_user("PWLMINORONE") + verified = stub_client.get_user("PWLMINORVERIFIED") + unknown = stub_client.get_user("PWLMEMBERTWO") + + assert unverified is not None and unverified.minor is True and unverified.guardian_verified is False + assert verified is not None and verified.minor is True and verified.guardian_verified is True + assert unknown is not None and unknown.minor is None and unknown.guardian_verified is None + + +def test_set_minor_and_set_guardian_verified_change_the_record(stub_client: StubWeaveClient) -> None: + stub_client.set_minor("PWLMEMBERONE", True) + stub_client.set_guardian_verified("PWLMEMBERONE", True) + + record = stub_client.get_user("PWLMEMBERONE") + + assert record is not None + assert record.minor is True + assert record.guardian_verified is True From d444b5004dee5aa9d9d6dbae5d2b7abc7e1f68ec Mon Sep 17 00:00:00 2001 From: Jasper Mayone Date: Mon, 12 Oct 2026 13:10:00 -0400 Subject: [PATCH 2/4] feat: refuse budget grants while a minor has no verified guardian --- krater/services/errors.py | 28 ++ krater/services/guardian.py | 142 +++++++++ krater/services/projects.py | 40 ++- krater/services/slack_reviews.py | 10 +- krater/web/routers/projects.py | 24 +- tests/services/test_admin.py | 182 +++++++---- tests/services/test_concurrency.py | 11 +- tests/services/test_guardian.py | 337 ++++++++++++++++++++ tests/services/test_quilt_events.py | 117 ++++--- tests/services/test_reviews.py | 68 +++- tests/services/test_screenshots.py | 75 +++-- tests/services/test_slack_notify.py | 10 +- tests/services/test_slack_reviews.py | 39 +++ tests/services/test_validation_and_state.py | 33 +- tests/services/test_workflow_happy_path.py | 19 +- tests/web/conftest.py | 5 +- tests/web/test_gallery.py | 15 +- 17 files changed, 967 insertions(+), 188 deletions(-) create mode 100644 krater/services/guardian.py create mode 100644 tests/services/test_guardian.py diff --git a/krater/services/errors.py b/krater/services/errors.py index 119ddba..14e7ef2 100644 --- a/krater/services/errors.py +++ b/krater/services/errors.py @@ -38,3 +38,31 @@ def __init__(self, errors: dict[str, str]) -> None: class NotFound(DomainError): """The requested record doesn't exist (or isn't visible to this actor).""" + + +class GuardianVerificationRequired(InvalidState): + """Krater can't grant budget: a team member is under 18 without a verified guardian in Weave, or Weave's + directory doesn't return them. + + `members` holds `(label, reason)` for each such member, so reviewers and admins see who blocks the grant and + why (docs/SPEC.md "Guardian verification").""" + + def __init__(self, members: list[tuple[str, str]]) -> None: + self.members = members + listed = "; ".join(f"{label}: {reason}" for label, reason in members) + super().__init__( + "Krater can't grant budget to this project yet. These team members must be cleared in Weave first: " + + listed + + ". A Weave admin records a verified guardian (the signed form and the call) on the member's Weave " + "admin page." + ) + + +class GuardianCheckUnavailable(InvalidState): + """Krater couldn't ask Weave about the team members' ages, so it grants no budget (fail closed).""" + + def __init__(self) -> None: + super().__init__( + "Krater couldn't reach Weave to check the team members' ages, so it granted no budget. " + "Try again in a minute." + ) diff --git a/krater/services/guardian.py b/krater/services/guardian.py new file mode 100644 index 0000000..f522a0b --- /dev/null +++ b/krater/services/guardian.py @@ -0,0 +1,142 @@ +"""The guardian gate: the safeguard for team members under 18 (docs/SPEC.md "Guardian verification"). + +Weave asks a parent or guardian to consent by email for a member under 18. An email link proves control of an +inbox, not parenthood. So a Weave admin also records a stronger check in Weave: a signed guardian form plus a +short call or video check with the guardian. Weave's directory then says `guardian_verified: true`. Krater +stores none of this. It reads the directory and grants no budget to a project while a team member is a minor +without a verified guardian. + +- **The team** is the project's submitter plus the credited builders on the revision being decided. +- **The rule.** A member blocks the grant when Weave says `minor: true` and `guardian_verified` is not + `true` (false or missing). The directory is asked fresh at decision time, the same way role checks are. +- **No directory record.** Weave's directory answers 404 for anyone the Krater app may not serve. That includes + a minor whose guardian consent is missing or was revoked, so a 404 blocks the grant too. +- **Unknown age.** Older Weave versions send a record without `minor`. Weave ships the fields first, so Krater + treats that record as "not known to be a minor" and logs a warning, rather than blocking every grant while + the rollout finishes. +- **Weave down.** If Weave can't be reached, the grant fails (`GuardianCheckUnavailable`). Krater never + guesses. +- **No override.** This is a safety rule, so an admin override (`admin_decide`, `admin_adjust_budget`) can't + bypass it. A Weave admin verifies the guardian first. + +`require_team_cleared` is the gate. `krater.services.projects` calls it on every grant: a proposal approval, an +amendment approval that raises the ceiling, and an admin budget increase. The web and Slack review paths both +reach it through `record_review`. +""" + +from __future__ import annotations + +import logging +from dataclasses import dataclass + +import sqlalchemy as sa +from sqlalchemy.orm import Session + +from krater.models import Project, ProjectRevision, User +from krater.services.errors import GuardianCheckUnavailable, GuardianVerificationRequired +from krater.weave import WeaveClient, WeaveUnavailableError + +logger = logging.getLogger(__name__) + + +#: Why a team member blocks a grant, as shown to reviewers and admins. +REASON_NO_VERIFIED_GUARDIAN = "under 18 without a verified guardian in Weave" +REASON_NOT_IN_WEAVE = "not available in Weave (no access to Krater)" + + +@dataclass(frozen=True) +class PendingMember: + """A team member who blocks a budget grant: a minor whose guardian Weave hasn't verified, or a member + Weave's directory doesn't return. `reason` is one of the `REASON_*` texts.""" + + user: User + name: str + reason: str = REASON_NO_VERIFIED_GUARDIAN + + @property + def label(self) -> str: + """The member's name and Patchwork ID, for messages to reviewers and admins.""" + sub = self.user.weave_sub + return f"{self.name} ({sub})" if self.name and self.name != sub else sub + + +def team_users(session: Session, project: Project, *, revision: ProjectRevision | None = None) -> list[User]: + """The project's team: its submitter, then the credited builders on `revision` (the current revision by + default). In a stable order, without duplicates.""" + revision = revision if revision is not None else project.current_revision + users = [project.submitter] + builder_ids = [ + uid for uid in (revision.credited_builder_ids if revision is not None else []) if uid != project.submitter_id + ] + if builder_ids: + users.extend(session.scalars(sa.select(User).where(User.id.in_(builder_ids)).order_by(User.weave_sub))) + return users + + +def members_needing_verification( + session: Session, + weave_client: WeaveClient, + project: Project, + *, + revision: ProjectRevision | None = None, + fresh: bool = True, +) -> list[PendingMember]: + """Every team member who blocks a grant: Weave lists them as `minor: true` without `guardian_verified: true`, + or Weave's directory has no record of them (404). A record without `minor` doesn't block; it is logged. + + `fresh` (the default) skips the directory client's cache, as every decision must; only a page view may + pass `fresh=False`. Raises `WeaveUnavailableError` if Weave can't be reached. + """ + pending: list[PendingMember] = [] + for user in team_users(session, project, revision=revision): + record = weave_client.get_user(user.weave_sub, fresh=fresh) + if record is None: + # Weave hides anyone Krater may not serve, a minor without guardian consent included. + pending.append(PendingMember(user=user, name="", reason=REASON_NOT_IN_WEAVE)) + continue + if record.minor is None: + logger.warning( + "Weave didn't say whether team member %s of project %s is a minor (no minor field); treating them " + "as not known to be a minor", + user.weave_sub, + project.id, + ) + continue + if record.minor and record.guardian_verified is not True: + pending.append(PendingMember(user=user, name=record.name)) + return pending + + +def require_team_cleared( + session: Session, weave_client: WeaveClient, project: Project, *, revision: ProjectRevision | None = None +) -> None: + """The gate before any budget grant. Raises `GuardianVerificationRequired` (listing the members) if a team + member is a minor without a verified guardian in Weave, and `GuardianCheckUnavailable` if Weave can't be + reached. Both are `InvalidState`, so the web and Slack paths show the message and roll back.""" + try: + pending = members_needing_verification(session, weave_client, project, revision=revision, fresh=True) + except WeaveUnavailableError as exc: + raise GuardianCheckUnavailable() from exc + if pending: + raise GuardianVerificationRequired([(member.label, member.reason) for member in pending]) + + +def pending_for_display(session: Session, weave_client: WeaveClient, project: Project) -> list[PendingMember] | None: + """`members_needing_verification` for a page view: may use the directory cache, and gives `None` instead + of failing the page when Weave can't be reached.""" + try: + return members_needing_verification(session, weave_client, project, fresh=False) + except WeaveUnavailableError: + logger.warning("Weave unavailable; can't show guardian status for project %s", project.id) + return None + + +__all__ = [ + "REASON_NOT_IN_WEAVE", + "REASON_NO_VERIFIED_GUARDIAN", + "PendingMember", + "members_needing_verification", + "pending_for_display", + "require_team_cleared", + "team_users", +] diff --git a/krater/services/projects.py b/krater/services/projects.py index de50d7d..0de0c11 100644 --- a/krater/services/projects.py +++ b/krater/services/projects.py @@ -40,9 +40,10 @@ RevisionKind, RevisionOutcome, ) -from krater.services import approval_policy, audit, budget, quilt_events +from krater.services import approval_policy, audit, budget, guardian, quilt_events from krater.services.actor import Actor from krater.services.errors import InvalidState, NotAllowed, NotFound, ValidationFailed +from krater.weave import WeaveClient _TERMINAL_STATUSES = (ProjectStatus.COMPLETED, ProjectStatus.WITHDRAWN) @@ -490,6 +491,7 @@ def record_review( decision: ReviewDecision, reason: str | None = None, source: ReviewSource, + weave_client: WeaveClient, ) -> Review: """Record a reviewer's decision on `revision`, then apply its effect if the decision resolves it. @@ -501,6 +503,10 @@ def record_review( (see module docstring for what that opens up next); an `approve` applies the revision's approval effect only once `approval_policy.is_satisfied` says the policy is now met -- otherwise the revision just stays `pending` with one more recorded approval. + + An approval that grants budget first passes the guardian gate (`guardian.require_team_cleared`, asking + `weave_client` fresh). If the gate refuses, this raises and the caller rolls back, review included: the + reviewer approves again once an admin has recorded the verifications. """ project = revision.project @@ -551,7 +557,7 @@ def record_review( if decision is ReviewDecision.REJECT: _apply_reject(session, revision) elif approval_policy.is_satisfied(session, revision): - _apply_approve(session, actor, revision) + _apply_approve(session, actor, revision, weave_client=weave_client) quilt_events.sync_project(session, project) return review @@ -575,11 +581,23 @@ def _apply_reject(session: Session, revision: ProjectRevision) -> None: session.flush() -def _apply_approve(session: Session, actor: Actor, revision: ProjectRevision) -> None: - """Apply the effect of a revision being approved (by policy satisfaction or an admin override).""" - revision.outcome = RevisionOutcome.APPROVED +def _apply_approve(session: Session, actor: Actor, revision: ProjectRevision, *, weave_client: WeaveClient) -> None: + """Apply the effect of a revision being approved (by policy satisfaction or an admin override). + + A proposal approval, and an amendment approval that raises the ceiling, grant budget: both pass the + guardian gate first (`guardian.require_team_cleared`). There is no override: an admin decision takes the + same path. A completion approval only reclaims budget, so it has no gate. + """ project = revision.project stage = approval_policy.stage_for(revision) + if stage is not ApprovalStage.COMPLETION: + grants = revision.kind is not RevisionKind.AMENDMENT or ( + revision.budget_requested_cents > budget.ceiling_cents(session, project) + ) + if grants: + guardian.require_team_cleared(session, weave_client, project, revision=revision) + + revision.outcome = RevisionOutcome.APPROVED if stage is ApprovalStage.COMPLETION: project.status = ProjectStatus.COMPLETED @@ -670,6 +688,7 @@ def admin_decide( revision: ProjectRevision, decision: ReviewDecision, reason: str, + weave_client: WeaveClient, ) -> ProjectRevision: """Admin override: decide `revision` directly, bypassing `ApprovalPolicy`. @@ -678,6 +697,9 @@ def admin_decide( sign-off. `reason` is required regardless of `decision`. Applies the same approve/reject effect as a policy-satisfying reviewer decision would (see `record_review`), and writes an `AuditEvent`. Still requires `revision` to be the project's current, submitted, still-`pending` revision. + + The override bypasses the approval policy, not the guardian gate: an approval that grants budget still + raises `GuardianVerificationRequired` while a team member under 18 has no verification. """ if not actor.is_admin: raise NotAllowed("Only an admin may override a review decision.") @@ -702,7 +724,7 @@ def admin_decide( _apply_reject(session, revision) action = "admin_reject" else: - _apply_approve(session, actor, revision) + _apply_approve(session, actor, revision, weave_client=weave_client) action = "admin_approve" audit.record( @@ -724,12 +746,14 @@ def admin_adjust_budget( project: Project, amount_cents: int, reason: str, + weave_client: WeaveClient, ) -> BudgetEntry: """Admin override: adjust a project's budget ceiling by a signed amount. Admin only. Only on an `approved` or `pending_completion_review` project. `reason` is required. Raises `ValidationFailed` if `amount_cents` is zero or would take the ceiling below zero, and `NotAllowed` for - an increase on the admin's own project (see `admin_decide`; a cut is fine). Writes an `AuditEvent`. + an increase on the admin's own project (see `admin_decide`; a cut is fine). An increase grants budget, + so it passes the guardian gate (`guardian.require_team_cleared`) first. Writes an `AuditEvent`. """ if not actor.is_admin: raise NotAllowed("Only an admin may adjust a project's budget.") @@ -746,6 +770,8 @@ def admin_adjust_budget( if budget.ceiling_cents(session, project) + amount_cents < 0: raise ValidationFailed({"amount_cents": "This would take the project's budget ceiling below zero."}) + if amount_cents > 0: + guardian.require_team_cleared(session, weave_client, project) entry = budget.add_entry( session, diff --git a/krater/services/slack_reviews.py b/krater/services/slack_reviews.py index 4503a4f..396f00d 100644 --- a/krater/services/slack_reviews.py +++ b/krater/services/slack_reviews.py @@ -132,8 +132,16 @@ def _process_decision( return try: + # The guardian gate runs inside `record_review`, the same rule as the web path. Its refusal is an + # `InvalidState`, so the clicker sees which members need a verification. project_service.record_review( - session, actor, revision=revision, decision=decision, reason=reason, source=ReviewSource.SLACK + session, + actor, + revision=revision, + decision=decision, + reason=reason, + source=ReviewSource.SLACK, + weave_client=weave_client, ) except DomainError as exc: session.rollback() diff --git a/krater/web/routers/projects.py b/krater/web/routers/projects.py index 0953293..2376d30 100644 --- a/krater/web/routers/projects.py +++ b/krater/web/routers/projects.py @@ -29,7 +29,7 @@ RevisionOutcome, User, ) -from krater.services import people, pricing, slack_membership +from krater.services import guardian, people, pricing, slack_membership from krater.services import projects as project_service from krater.services import screenshots as screenshot_service from krater.services.actor import Actor @@ -313,6 +313,13 @@ def _build_detail_context( screenshots = _screenshot_entries(get_object_store(), current) if current is not None else [] + # Team members under 18 without a verified guardian in Weave block every budget grant (docs/SPEC.md + # "Guardian verification"). A finished project gets no more budget, so it doesn't ask. `None` means Weave + # couldn't say right now; the decision itself still checks Weave fresh. + guardian_pending = ( + guardian.pending_for_display(session, weave_client, project) if project.status not in _TERMINAL_STATUSES else [] + ) + # For each revision that used the budget estimator (`budget_estimate` set), whether its actually # -requested budget has drifted far enough from that stored estimate to flag for reviewers -- e.g. # the submitter estimated one thing, then hand-edited the requested amount well past it. @@ -331,6 +338,7 @@ def _build_detail_context( "project": project, "summary": summary, "screenshots": screenshots, + "guardian_pending": guardian_pending, "revisions": revisions, "estimate_mismatch_by_revision": estimate_mismatch_by_revision, "names_by_id": names_by_id, @@ -751,7 +759,13 @@ def record_review( revision = project.current_revision try: project_service.record_review( - db_session, actor, revision=revision, decision=decision_enum, reason=reason or None, source=ReviewSource.WEB + db_session, + actor, + revision=revision, + decision=decision_enum, + reason=reason or None, + source=ReviewSource.WEB, + weave_client=weave_client, ) except ValidationFailed as exc: db_session.rollback() @@ -805,7 +819,9 @@ def admin_decide( revision = project.current_revision try: - project_service.admin_decide(db_session, actor, revision=revision, decision=decision_enum, reason=reason) + project_service.admin_decide( + db_session, actor, revision=revision, decision=decision_enum, reason=reason, weave_client=weave_client + ) except ValidationFailed as exc: db_session.rollback() context = _build_detail_context( @@ -866,7 +882,7 @@ def admin_adjust_budget( try: project_service.admin_adjust_budget( - db_session, actor, project=project, amount_cents=amount_cents, reason=reason + db_session, actor, project=project, amount_cents=amount_cents, reason=reason, weave_client=weave_client ) except (ValidationFailed, NotAllowed) as exc: # An admin's NotAllowed here is the own-project rule, worth showing next to the amount; anyone else's diff --git a/tests/services/test_admin.py b/tests/services/test_admin.py index b7f99f2..4e72f50 100644 --- a/tests/services/test_admin.py +++ b/tests/services/test_admin.py @@ -28,16 +28,23 @@ def _submitted_project(db_session: Session, member: Actor, *, budget_requested_c return projects.submit(db_session, member, project=project) -def _approved_project(db_session: Session, member: Actor, admin: Actor, *, budget_requested_cents: int = 10_000): +def _approved_project(db_session: Session, member: Actor, admin: Actor, weave, *, budget_requested_cents: int = 10_000): project = _submitted_project(db_session, member, budget_requested_cents=budget_requested_cents) projects.admin_decide( - db_session, admin, revision=project.current_revision, decision=ReviewDecision.APPROVE, reason="ok" + db_session, + admin, + revision=project.current_revision, + decision=ReviewDecision.APPROVE, + reason="ok", + weave_client=weave, ) db_session.refresh(project) return project -def test_admin_decide_bypasses_policy_and_approves_directly(db_session: Session, member: Actor, admin: Actor) -> None: +def test_admin_decide_bypasses_policy_and_approves_directly( + db_session: Session, member: Actor, admin: Actor, weave +) -> None: db_session.add(ApprovalPolicy(stage=ApprovalStage.PROPOSAL, min_approvals=5)) db_session.flush() project = _submitted_project(db_session, member) @@ -48,6 +55,7 @@ def test_admin_decide_bypasses_policy_and_approves_directly(db_session: Session, revision=project.current_revision, decision=ReviewDecision.APPROVE, reason="Admin override: looks fine.", + weave_client=weave, ) assert revision.outcome is RevisionOutcome.APPROVED @@ -61,35 +69,53 @@ def test_admin_decide_bypasses_policy_and_approves_directly(db_session: Session, @pytest.mark.parametrize("decision", [ReviewDecision.APPROVE, ReviewDecision.REJECT]) -def test_an_admin_cannot_decide_their_own_project(db_session: Session, member: Actor, decision: ReviewDecision) -> None: +def test_an_admin_cannot_decide_their_own_project( + db_session: Session, member: Actor, decision: ReviewDecision, weave +) -> None: """Otherwise an admin could approve, and so fund, their own proposal with nobody else signing off.""" submitter_admin = Actor(user=member.user, groups=frozenset({GROUP_MEMBER, GROUP_ADMIN})) project = _submitted_project(db_session, submitter_admin) with pytest.raises(NotAllowed, match="another admin"): projects.admin_decide( - db_session, submitter_admin, revision=project.current_revision, decision=decision, reason="Mine." + db_session, + submitter_admin, + revision=project.current_revision, + decision=decision, + reason="Mine.", + weave_client=weave, ) assert project.current_revision.outcome is RevisionOutcome.PENDING -def test_an_admin_cannot_decide_a_project_that_credits_them(db_session: Session, member: Actor, admin: Actor) -> None: +def test_an_admin_cannot_decide_a_project_that_credits_them( + db_session: Session, member: Actor, admin: Actor, weave +) -> None: project = _submitted_project(db_session, member) project.current_revision.credited_builder_ids = [admin.user.id] db_session.flush() with pytest.raises(NotAllowed): projects.admin_decide( - db_session, admin, revision=project.current_revision, decision=ReviewDecision.APPROVE, reason="x" + db_session, + admin, + revision=project.current_revision, + decision=ReviewDecision.APPROVE, + reason="x", + weave_client=weave, ) -def test_admin_decide_reject_opens_next_draft_and_audits(db_session: Session, member: Actor, admin: Actor) -> None: +def test_admin_decide_reject_opens_next_draft_and_audits( + db_session: Session, member: Actor, admin: Actor, weave +) -> None: project = _submitted_project(db_session, member) revision = project.current_revision - projects.admin_decide(db_session, admin, revision=revision, decision=ReviewDecision.REJECT, reason="Not yet.") + projects.admin_decide( + db_session, admin, revision=revision, decision=ReviewDecision.REJECT, reason="Not yet.", weave_client=weave + ) db_session.refresh(project) assert project.status is ProjectStatus.CHANGES_REQUESTED @@ -100,94 +126,126 @@ def test_admin_decide_reject_opens_next_draft_and_audits(db_session: Session, me assert event.reason == "Not yet." -def test_admin_decide_requires_a_reason(db_session: Session, member: Actor, admin: Actor) -> None: +def test_admin_decide_requires_a_reason(db_session: Session, member: Actor, admin: Actor, weave) -> None: project = _submitted_project(db_session, member) with pytest.raises(ValidationFailed): projects.admin_decide( - db_session, admin, revision=project.current_revision, decision=ReviewDecision.APPROVE, reason="" + db_session, + admin, + revision=project.current_revision, + decision=ReviewDecision.APPROVE, + reason="", + weave_client=weave, ) -def test_admin_decide_requires_admin(db_session: Session, member: Actor, reviewer: Actor) -> None: +def test_admin_decide_requires_admin(db_session: Session, member: Actor, reviewer: Actor, weave) -> None: project = _submitted_project(db_session, member) with pytest.raises(NotAllowed): projects.admin_decide( - db_session, reviewer, revision=project.current_revision, decision=ReviewDecision.APPROVE, reason="x" + db_session, + reviewer, + revision=project.current_revision, + decision=ReviewDecision.APPROVE, + reason="x", + weave_client=weave, ) -def test_admin_adjust_budget_up_and_down(db_session: Session, member: Actor, admin: Actor) -> None: - project = _approved_project(db_session, member, admin, budget_requested_cents=10_000) +def test_admin_adjust_budget_up_and_down(db_session: Session, member: Actor, admin: Actor, weave) -> None: + project = _approved_project(db_session, member, admin, budget_requested_cents=10_000, weave=weave) - projects.admin_adjust_budget(db_session, admin, project=project, amount_cents=5_000, reason="Extra credit.") + projects.admin_adjust_budget( + db_session, admin, project=project, amount_cents=5_000, reason="Extra credit.", weave_client=weave + ) assert budget.ceiling_cents(db_session, project) == 15_000 - projects.admin_adjust_budget(db_session, admin, project=project, amount_cents=-3_000, reason="Trim.") + projects.admin_adjust_budget( + db_session, admin, project=project, amount_cents=-3_000, reason="Trim.", weave_client=weave + ) assert budget.ceiling_cents(db_session, project) == 12_000 events = db_session.query(AuditEvent).filter_by(project_id=project.id, action="admin_adjust_budget").all() assert len(events) == 2 -def test_admin_adjust_budget_cannot_take_ceiling_below_zero(db_session: Session, member: Actor, admin: Actor) -> None: - project = _approved_project(db_session, member, admin, budget_requested_cents=1_000) +def test_admin_adjust_budget_cannot_take_ceiling_below_zero( + db_session: Session, member: Actor, admin: Actor, weave +) -> None: + project = _approved_project(db_session, member, admin, budget_requested_cents=1_000, weave=weave) with pytest.raises(ValidationFailed): - projects.admin_adjust_budget(db_session, admin, project=project, amount_cents=-2_000, reason="Too much.") + projects.admin_adjust_budget( + db_session, admin, project=project, amount_cents=-2_000, reason="Too much.", weave_client=weave + ) assert budget.ceiling_cents(db_session, project) == 1_000 def test_admin_adjust_budget_only_on_approved_or_pending_completion( - db_session: Session, member: Actor, admin: Actor + db_session: Session, member: Actor, admin: Actor, weave ) -> None: project = _submitted_project(db_session, member) # status: pending_review with pytest.raises(InvalidState): - projects.admin_adjust_budget(db_session, admin, project=project, amount_cents=100, reason="x") + projects.admin_adjust_budget( + db_session, admin, project=project, amount_cents=100, reason="x", weave_client=weave + ) -def test_admin_adjust_budget_requires_reason(db_session: Session, member: Actor, admin: Actor) -> None: - project = _approved_project(db_session, member, admin) +def test_admin_adjust_budget_requires_reason(db_session: Session, member: Actor, admin: Actor, weave) -> None: + project = _approved_project(db_session, member, admin, weave=weave) with pytest.raises(ValidationFailed): - projects.admin_adjust_budget(db_session, admin, project=project, amount_cents=100, reason=" ") + projects.admin_adjust_budget( + db_session, admin, project=project, amount_cents=100, reason=" ", weave_client=weave + ) -def test_admin_adjust_budget_requires_admin(db_session: Session, member: Actor, admin: Actor, reviewer: Actor) -> None: - project = _approved_project(db_session, member, admin) +def test_admin_adjust_budget_requires_admin( + db_session: Session, member: Actor, admin: Actor, reviewer: Actor, weave +) -> None: + project = _approved_project(db_session, member, admin, weave=weave) with pytest.raises(NotAllowed): - projects.admin_adjust_budget(db_session, reviewer, project=project, amount_cents=100, reason="x") + projects.admin_adjust_budget( + db_session, reviewer, project=project, amount_cents=100, reason="x", weave_client=weave + ) def test_an_admin_cannot_add_budget_to_their_own_project_but_can_cut_it( - db_session: Session, member: Actor, admin: Actor + db_session: Session, member: Actor, admin: Actor, weave ) -> None: submitter_admin = Actor(user=member.user, groups=frozenset({GROUP_MEMBER, GROUP_ADMIN})) - project = _approved_project(db_session, submitter_admin, admin, budget_requested_cents=10_000) + project = _approved_project(db_session, submitter_admin, admin, budget_requested_cents=10_000, weave=weave) with pytest.raises(NotAllowed): - projects.admin_adjust_budget(db_session, submitter_admin, project=project, amount_cents=5_000, reason="More.") - projects.admin_adjust_budget(db_session, submitter_admin, project=project, amount_cents=-2_000, reason="Less.") + projects.admin_adjust_budget( + db_session, submitter_admin, project=project, amount_cents=5_000, reason="More.", weave_client=weave + ) + projects.admin_adjust_budget( + db_session, submitter_admin, project=project, amount_cents=-2_000, reason="Less.", weave_client=weave + ) assert budget.ceiling_cents(db_session, project) == 8_000 -def test_admin_adjust_budget_refuses_zero(db_session: Session, member: Actor, admin: Actor) -> None: - project = _approved_project(db_session, member, admin) +def test_admin_adjust_budget_refuses_zero(db_session: Session, member: Actor, admin: Actor, weave) -> None: + project = _approved_project(db_session, member, admin, weave=weave) with pytest.raises(ValidationFailed) as exc_info: - projects.admin_adjust_budget(db_session, admin, project=project, amount_cents=0, reason="Nothing.") + projects.admin_adjust_budget( + db_session, admin, project=project, amount_cents=0, reason="Nothing.", weave_client=weave + ) assert "amount_cents" in exc_info.value.errors assert db_session.query(AuditEvent).filter_by(project_id=project.id, action="admin_adjust_budget").count() == 0 -def test_reclaim_budget_reduces_ceiling(db_session: Session, member: Actor, admin: Actor) -> None: - project = _approved_project(db_session, member, admin, budget_requested_cents=10_000) +def test_reclaim_budget_reduces_ceiling(db_session: Session, member: Actor, admin: Actor, weave) -> None: + project = _approved_project(db_session, member, admin, budget_requested_cents=10_000, weave=weave) entry = projects.reclaim_budget(db_session, admin, project=project, amount_cents=4_000, reason="Stalled.") @@ -199,24 +257,24 @@ def test_reclaim_budget_reduces_ceiling(db_session: Session, member: Actor, admi assert event.reason == "Stalled." -def test_reclaim_budget_amount_must_be_positive(db_session: Session, member: Actor, admin: Actor) -> None: - project = _approved_project(db_session, member, admin, budget_requested_cents=10_000) +def test_reclaim_budget_amount_must_be_positive(db_session: Session, member: Actor, admin: Actor, weave) -> None: + project = _approved_project(db_session, member, admin, budget_requested_cents=10_000, weave=weave) with pytest.raises(ValidationFailed): projects.reclaim_budget(db_session, admin, project=project, amount_cents=0, reason="x") -def test_reclaim_budget_cannot_exceed_ceiling(db_session: Session, member: Actor, admin: Actor) -> None: - project = _approved_project(db_session, member, admin, budget_requested_cents=1_000) +def test_reclaim_budget_cannot_exceed_ceiling(db_session: Session, member: Actor, admin: Actor, weave) -> None: + project = _approved_project(db_session, member, admin, budget_requested_cents=1_000, weave=weave) with pytest.raises(ValidationFailed): projects.reclaim_budget(db_session, admin, project=project, amount_cents=5_000, reason="x") def test_withdraw_by_submitter_reclaims_remaining_budget_without_audit( - db_session: Session, member: Actor, admin: Actor + db_session: Session, member: Actor, admin: Actor, weave ) -> None: - project = _approved_project(db_session, member, admin, budget_requested_cents=10_000) + project = _approved_project(db_session, member, admin, budget_requested_cents=10_000, weave=weave) db_session.add( SpendSnapshot(project_id=project.id, estimated_spend_cents=2_000, source=SpendSource.SKYPILOT_COST_REPORT) ) @@ -232,8 +290,10 @@ def test_withdraw_by_submitter_reclaims_remaining_budget_without_audit( assert db_session.query(AuditEvent).filter_by(project_id=project.id, action="admin_withdraw").count() == 0 -def test_withdraw_by_admin_writes_audit_and_requires_reason(db_session: Session, member: Actor, admin: Actor) -> None: - project = _approved_project(db_session, member, admin, budget_requested_cents=5_000) +def test_withdraw_by_admin_writes_audit_and_requires_reason( + db_session: Session, member: Actor, admin: Actor, weave +) -> None: + project = _approved_project(db_session, member, admin, budget_requested_cents=5_000, weave=weave) with pytest.raises(ValidationFailed): projects.withdraw(db_session, admin, project=project, reason="") @@ -247,16 +307,16 @@ def test_withdraw_by_admin_writes_audit_and_requires_reason(db_session: Session, def test_withdraw_requires_submitter_or_admin( - db_session: Session, member: Actor, admin: Actor, reviewer: Actor + db_session: Session, member: Actor, admin: Actor, reviewer: Actor, weave ) -> None: - project = _approved_project(db_session, member, admin) + project = _approved_project(db_session, member, admin, weave=weave) with pytest.raises(NotAllowed): projects.withdraw(db_session, reviewer, project=project, reason="Not yours.") -def test_withdraw_from_a_terminal_state_is_invalid(db_session: Session, member: Actor, admin: Actor) -> None: - project = _approved_project(db_session, member, admin) +def test_withdraw_from_a_terminal_state_is_invalid(db_session: Session, member: Actor, admin: Actor, weave) -> None: + project = _approved_project(db_session, member, admin, weave=weave) projects.withdraw(db_session, member, project=project) db_session.refresh(project) @@ -277,8 +337,10 @@ def _set_cap(db_session: Session, actor: Actor, project, cap_cents: int | None, ) -def test_an_admin_can_set_and_clear_a_projects_hourly_cap(db_session: Session, member: Actor, admin: Actor) -> None: - project = _approved_project(db_session, member, admin) +def test_an_admin_can_set_and_clear_a_projects_hourly_cap( + db_session: Session, member: Actor, admin: Actor, weave +) -> None: + project = _approved_project(db_session, member, admin, weave=weave) _set_cap(db_session, admin, project, 1_200) assert project.max_hourly_cost_cents == 1_200 @@ -304,9 +366,9 @@ def test_the_hourly_cap_can_be_set_before_approval(db_session: Session, member: def test_setting_the_hourly_cap_needs_an_admin_and_a_reason( - db_session: Session, member: Actor, admin: Actor, reviewer: Actor + db_session: Session, member: Actor, admin: Actor, reviewer: Actor, weave ) -> None: - project = _approved_project(db_session, member, admin) + project = _approved_project(db_session, member, admin, weave=weave) with pytest.raises(NotAllowed): _set_cap(db_session, reviewer, project, 800) @@ -316,8 +378,10 @@ def test_setting_the_hourly_cap_needs_an_admin_and_a_reason( @pytest.mark.parametrize("cap_cents", [0, -100]) -def test_the_hourly_cap_must_be_above_zero(db_session: Session, member: Actor, admin: Actor, cap_cents: int) -> None: - project = _approved_project(db_session, member, admin) +def test_the_hourly_cap_must_be_above_zero( + db_session: Session, member: Actor, admin: Actor, cap_cents: int, weave +) -> None: + project = _approved_project(db_session, member, admin, weave=weave) with pytest.raises(ValidationFailed) as exc_info: _set_cap(db_session, admin, project, cap_cents) @@ -325,8 +389,8 @@ def test_the_hourly_cap_must_be_above_zero(db_session: Session, member: Actor, a assert "cap_cents" in exc_info.value.errors -def test_a_finished_projects_hourly_cap_cannot_change(db_session: Session, member: Actor, admin: Actor) -> None: - project = _approved_project(db_session, member, admin) +def test_a_finished_projects_hourly_cap_cannot_change(db_session: Session, member: Actor, admin: Actor, weave) -> None: + project = _approved_project(db_session, member, admin, weave=weave) projects.withdraw(db_session, member, project=project) with pytest.raises(InvalidState): @@ -334,10 +398,10 @@ def test_a_finished_projects_hourly_cap_cannot_change(db_session: Session, membe def test_an_admin_can_lower_but_not_raise_their_own_projects_cap( - db_session: Session, member: Actor, admin: Actor + db_session: Session, member: Actor, admin: Actor, weave ) -> None: submitter_admin = Actor(user=member.user, groups=frozenset({GROUP_MEMBER, GROUP_ADMIN})) - project = _approved_project(db_session, submitter_admin, admin) + project = _approved_project(db_session, submitter_admin, admin, weave=weave) with pytest.raises(NotAllowed): _set_cap(db_session, submitter_admin, project, 1_000) diff --git a/tests/services/test_concurrency.py b/tests/services/test_concurrency.py index 24b4fa8..e122c95 100644 --- a/tests/services/test_concurrency.py +++ b/tests/services/test_concurrency.py @@ -63,7 +63,7 @@ def _cleanup(engine: Engine, *, project_id: uuid.UUID, user_ids: list[uuid.UUID] session.commit() -def test_two_concurrent_approvals_do_not_double_apply_the_budget(engine: Engine) -> None: +def test_two_concurrent_approvals_do_not_double_apply_the_budget(engine: Engine, weave) -> None: """Two reviewers approve the same revision at (as close to) the same instant. Without a lock on the project row, both threads can each read the revision as still-`pending`, each independently see the (default, 1-approval) policy as satisfied by their own review, and each run `_apply_approve` -- @@ -74,6 +74,8 @@ def test_two_concurrent_approvals_do_not_double_apply_the_budget(engine: Engine) submitter = _make_user(setup, "sub") reviewer_a = _make_user(setup, "r1") reviewer_b = _make_user(setup, "r2") + # The guardian gate asks Weave about the team; a member Weave doesn't return would block the grant. + weave.put_user(submitter.weave_sub, roles=["member"], minor=False) project = projects.create_project( setup, Actor(submitter, _MEMBER), title="Race", write_up="w", budget_requested_cents=10_000 ) @@ -95,7 +97,12 @@ def worker(reviewer_id: uuid.UUID) -> None: barrier.wait() # both threads start their `record_review` call at the same time try: projects.record_review( - session, actor, revision=revision, decision=ReviewDecision.APPROVE, source=ReviewSource.WEB + session, + actor, + revision=revision, + decision=ReviewDecision.APPROVE, + source=ReviewSource.WEB, + weave_client=weave, ) session.commit() with lock: diff --git a/tests/services/test_guardian.py b/tests/services/test_guardian.py new file mode 100644 index 0000000..4a62da5 --- /dev/null +++ b/tests/services/test_guardian.py @@ -0,0 +1,337 @@ +"""The guardian gate (`krater.services.guardian`): no budget grant while a team member is a minor without a +verified guardian in Weave (docs/SPEC.md "Guardian verification").""" + +from __future__ import annotations + +import logging + +import pytest +from sqlalchemy.orm import Session + +from krater.models import BudgetEntry, ProjectStatus, ReviewDecision, ReviewSource, RevisionOutcome +from krater.services import budget, guardian, projects +from krater.services.actor import GROUP_MEMBER, Actor +from krater.services.errors import GuardianCheckUnavailable, GuardianVerificationRequired +from krater.weave import StubWeaveClient, WeaveUnavailableError + + +class _DownWeave: + """A Weave that can't be reached.""" + + def get_user(self, sub: str, *, fresh: bool = False): + raise WeaveUnavailableError("down") + + +class _SpyWeave: + """Records the `fresh` flag of every directory lookup, then asks the real stub.""" + + def __init__(self, inner: StubWeaveClient) -> None: + self.inner = inner + self.fresh_flags: list[bool] = [] + + def get_user(self, sub: str, *, fresh: bool = False): + self.fresh_flags.append(fresh) + return self.inner.get_user(sub, fresh=fresh) + + +@pytest.fixture +def guardian_logs(caplog, monkeypatch): + """Captures the gate's warnings. Alembic's `fileConfig` (run by the `engine` fixture) disables loggers that + already exist, so turn this one back on.""" + monkeypatch.setattr(guardian.logger, "disabled", False) + caplog.set_level(logging.WARNING, logger="krater.services.guardian") + return caplog + + +@pytest.fixture +def minor(make_actor, weave: StubWeaveClient) -> Actor: + """A Ganymede member Weave lists as under 18, with no verified guardian.""" + actor = make_actor(groups=frozenset({GROUP_MEMBER}), display_name="Milo Minor") + weave.set_minor(actor.user.weave_sub, True) + return actor + + +def _submitted(db_session: Session, submitter: Actor, *, budget_cents: int = 10_000, builders=()): + project = projects.create_project( + db_session, submitter, title="Rover", write_up="A rover.", budget_requested_cents=budget_cents + ) + if builders: + projects.update_draft( + db_session, submitter, project=project, credited_builder_ids=[b.user.id for b in builders] + ) + return projects.submit(db_session, submitter, project=project) + + +def _approve(db_session: Session, reviewer: Actor, project, weave_client) -> None: + projects.record_review( + db_session, + reviewer, + revision=project.current_revision, + decision=ReviewDecision.APPROVE, + source=ReviewSource.WEB, + weave_client=weave_client, + ) + + +def _ledger_count(db_session: Session, project) -> int: + return db_session.query(BudgetEntry).filter(BudgetEntry.project_id == project.id).count() + + +# -- Proposal approval --------------------------------------------------------------------------------- + + +def test_a_minor_without_a_verified_guardian_blocks_the_proposal_approval( + db_session: Session, minor: Actor, reviewer: Actor, weave: StubWeaveClient +) -> None: + project = _submitted(db_session, minor) + + with pytest.raises(GuardianVerificationRequired) as exc_info: + _approve(db_session, reviewer, project, weave) + + assert exc_info.value.members == [(f"Milo Minor ({minor.user.weave_sub})", guardian.REASON_NO_VERIFIED_GUARDIAN)] + assert "verified guardian in Weave" in str(exc_info.value) + assert project.status is ProjectStatus.PENDING_REVIEW + assert project.current_revision.outcome is RevisionOutcome.PENDING + assert _ledger_count(db_session, project) == 0 + + +def test_a_minor_with_a_verified_guardian_can_be_approved( + db_session: Session, minor: Actor, reviewer: Actor, weave: StubWeaveClient +) -> None: + weave.set_guardian_verified(minor.user.weave_sub, True) + project = _submitted(db_session, minor) + + _approve(db_session, reviewer, project, weave) + + assert project.status is ProjectStatus.APPROVED + assert budget.ceiling_cents(db_session, project) == 10_000 + + +def test_a_false_guardian_verified_still_blocks( + db_session: Session, minor: Actor, reviewer: Actor, weave: StubWeaveClient +) -> None: + weave.set_guardian_verified(minor.user.weave_sub, False) + project = _submitted(db_session, minor) + + with pytest.raises(GuardianVerificationRequired): + _approve(db_session, reviewer, project, weave) + + +def test_an_adult_is_not_blocked(db_session: Session, member: Actor, reviewer: Actor, weave: StubWeaveClient) -> None: + weave.set_minor(member.user.weave_sub, False) + project = _submitted(db_session, member) + + _approve(db_session, reviewer, project, weave) + + assert project.status is ProjectStatus.APPROVED + + +def test_a_missing_minor_field_counts_as_not_known_to_be_a_minor_and_is_logged( + db_session: Session, member: Actor, reviewer: Actor, weave: StubWeaveClient, guardian_logs +) -> None: + project = _submitted(db_session, member) + + _approve(db_session, reviewer, project, weave) + + assert project.status is ProjectStatus.APPROVED + assert member.user.weave_sub in guardian_logs.text + assert "no minor field" in guardian_logs.text + + +def test_a_team_member_weave_does_not_return_blocks_the_grant( + db_session: Session, member: Actor, reviewer: Actor, weave: StubWeaveClient +) -> None: + # Weave's directory answers 404 for anyone Krater may not serve, such as a minor whose consent was revoked. + project = _submitted(db_session, member) + weave.remove_user(member.user.weave_sub) + + with pytest.raises(GuardianVerificationRequired) as exc_info: + _approve(db_session, reviewer, project, weave) + + assert exc_info.value.members == [(member.user.weave_sub, guardian.REASON_NOT_IN_WEAVE)] + assert "not available in Weave (no access to Krater)" in str(exc_info.value) + assert project.status is ProjectStatus.PENDING_REVIEW + + +def test_a_credited_builder_weave_does_not_return_blocks_the_grant( + db_session: Session, member: Actor, minor: Actor, reviewer: Actor, weave: StubWeaveClient +) -> None: + # A minor whose guardian consent was revoked: Weave hides them, so the 404 must not let the grant through. + project = _submitted(db_session, member, builders=[minor]) + weave.remove_user(minor.user.weave_sub) + + with pytest.raises(GuardianVerificationRequired) as exc_info: + _approve(db_session, reviewer, project, weave) + + assert exc_info.value.members == [(minor.user.weave_sub, guardian.REASON_NOT_IN_WEAVE)] + + +def test_both_reasons_are_listed_together( + db_session: Session, minor: Actor, make_actor, reviewer: Actor, weave: StubWeaveClient +) -> None: + hidden = make_actor(groups=frozenset({GROUP_MEMBER})) + project = _submitted(db_session, minor, builders=[hidden]) + weave.remove_user(hidden.user.weave_sub) + + pending = guardian.members_needing_verification(db_session, weave, project) + + assert [(member.user.id, member.reason) for member in pending] == [ + (minor.user.id, guardian.REASON_NO_VERIFIED_GUARDIAN), + (hidden.user.id, guardian.REASON_NOT_IN_WEAVE), + ] + + +def test_a_minor_credited_builder_blocks_the_approval( + db_session: Session, member: Actor, minor: Actor, reviewer: Actor, weave: StubWeaveClient +) -> None: + project = _submitted(db_session, member, builders=[minor]) + + with pytest.raises(GuardianVerificationRequired) as exc_info: + _approve(db_session, reviewer, project, weave) + + assert exc_info.value.members == [(f"Milo Minor ({minor.user.weave_sub})", guardian.REASON_NO_VERIFIED_GUARDIAN)] + + +def test_the_gate_asks_weave_fresh(db_session: Session, member: Actor, reviewer: Actor, weave: StubWeaveClient) -> None: + project = _submitted(db_session, member) + spy = _SpyWeave(weave) + + _approve(db_session, reviewer, project, spy) + + assert spy.fresh_flags == [True] + + +def test_the_grant_fails_closed_when_weave_is_down(db_session: Session, member: Actor, reviewer: Actor) -> None: + project = _submitted(db_session, member) + + with pytest.raises(GuardianCheckUnavailable): + _approve(db_session, reviewer, project, _DownWeave()) + + assert project.status is ProjectStatus.PENDING_REVIEW + + +def test_a_rejection_does_not_need_the_gate(db_session: Session, minor: Actor, reviewer: Actor) -> None: + project = _submitted(db_session, minor) + + projects.record_review( + db_session, + reviewer, + revision=project.current_revision, + decision=ReviewDecision.REJECT, + reason="Too vague.", + source=ReviewSource.WEB, + weave_client=_DownWeave(), + ) + + assert project.status is ProjectStatus.CHANGES_REQUESTED + + +# -- Admin overrides: no bypass ------------------------------------------------------------------------ + + +def test_an_admin_approval_cannot_bypass_the_gate( + db_session: Session, minor: Actor, admin: Actor, weave: StubWeaveClient +) -> None: + project = _submitted(db_session, minor) + + with pytest.raises(GuardianVerificationRequired): + projects.admin_decide( + db_session, + admin, + revision=project.current_revision, + decision=ReviewDecision.APPROVE, + reason="Urgent.", + weave_client=weave, + ) + + assert project.status is ProjectStatus.PENDING_REVIEW + + +def _approved(db_session: Session, submitter: Actor, reviewer: Actor, weave: StubWeaveClient): + """An approved project. The submitter is cleared for the approval, then made a minor without a + verified guardian, as when Weave learns a birth date later.""" + project = _submitted(db_session, submitter) + _approve(db_session, reviewer, project, weave) + weave.set_minor(submitter.user.weave_sub, True) + return project + + +def test_an_admin_budget_increase_is_blocked_but_a_cut_is_not( + db_session: Session, member: Actor, reviewer: Actor, admin: Actor, weave: StubWeaveClient +) -> None: + project = _approved(db_session, member, reviewer, weave) + + with pytest.raises(GuardianVerificationRequired): + projects.admin_adjust_budget( + db_session, admin, project=project, amount_cents=5_000, reason="More.", weave_client=weave + ) + projects.admin_adjust_budget( + db_session, admin, project=project, amount_cents=-2_000, reason="Less.", weave_client=weave + ) + + assert budget.ceiling_cents(db_session, project) == 8_000 + + +# -- Amendments and completion ------------------------------------------------------------------------- + + +def _amendment(db_session: Session, project, submitter: Actor, *, budget_cents: int): + projects.start_amendment(db_session, submitter, project=project) + projects.update_draft(db_session, submitter, project=project, budget_requested_cents=budget_cents) + projects.submit(db_session, submitter, project=project) + return project.current_revision + + +def test_an_amendment_that_raises_the_ceiling_is_blocked( + db_session: Session, member: Actor, reviewer: Actor, weave: StubWeaveClient +) -> None: + project = _approved(db_session, member, reviewer, weave) + _amendment(db_session, project, member, budget_cents=20_000) + + with pytest.raises(GuardianVerificationRequired): + _approve(db_session, reviewer, project, weave) + + +def test_an_amendment_that_lowers_the_ceiling_is_not_blocked( + db_session: Session, member: Actor, reviewer: Actor, weave: StubWeaveClient +) -> None: + project = _approved(db_session, member, reviewer, weave) + revision = _amendment(db_session, project, member, budget_cents=4_000) + + _approve(db_session, reviewer, project, weave) + + assert revision.outcome is RevisionOutcome.APPROVED + assert budget.ceiling_cents(db_session, project) == 4_000 + + +def test_a_completion_approval_is_not_blocked( + db_session: Session, member: Actor, reviewer: Actor, weave: StubWeaveClient +) -> None: + project = _approved(db_session, member, reviewer, weave) + projects.start_completion(db_session, member, project=project) + projects.submit_completion(db_session, member, project=project) + + _approve(db_session, reviewer, project, weave) + + assert project.status is ProjectStatus.COMPLETED + + +# -- Page display -------------------------------------------------------------------------------------- + + +def test_pending_for_display_lists_the_members_and_uses_the_cache( + db_session: Session, minor: Actor, weave: StubWeaveClient +) -> None: + project = _submitted(db_session, minor) + spy = _SpyWeave(weave) + + pending = guardian.pending_for_display(db_session, spy, project) + + assert [member.user.id for member in pending] == [minor.user.id] + assert spy.fresh_flags == [False] + + +def test_pending_for_display_is_none_when_weave_is_down(db_session: Session, minor: Actor) -> None: + project = _submitted(db_session, minor) + + assert guardian.pending_for_display(db_session, _DownWeave(), project) is None diff --git a/tests/services/test_quilt_events.py b/tests/services/test_quilt_events.py index 3426251..b66f73e 100644 --- a/tests/services/test_quilt_events.py +++ b/tests/services/test_quilt_events.py @@ -61,16 +61,21 @@ def _submit(session: Session, member: Actor, *, budget_cents: int = 100_000, tit return projects.submit(session, member, project=project) -def _approve(session: Session, reviewer: Actor, project: Project) -> Project: +def _approve(session: Session, reviewer: Actor, project: Project, weave) -> Project: projects.record_review( - session, reviewer, revision=project.current_revision, decision=ReviewDecision.APPROVE, source=ReviewSource.WEB + session, + reviewer, + revision=project.current_revision, + decision=ReviewDecision.APPROVE, + source=ReviewSource.WEB, + weave_client=weave, ) session.refresh(project) return project -def _approved(session: Session, member: Actor, reviewer: Actor, *, budget_cents: int = 100_000) -> Project: - return _approve(session, reviewer, _submit(session, member, budget_cents=budget_cents)) +def _approved(session: Session, member: Actor, reviewer: Actor, weave, *, budget_cents: int = 100_000) -> Project: + return _approve(session, reviewer, _submit(session, member, budget_cents=budget_cents), weave=weave) def _spend(session: Session, project: Project, cents: int) -> None: @@ -131,7 +136,7 @@ def test_the_first_submission_sends_submission_created(db_session: Session, memb def test_a_rejection_and_resubmission_send_updates_not_a_second_created( - db_session: Session, member: Actor, reviewer: Actor + db_session: Session, member: Actor, reviewer: Actor, weave ) -> None: project = _submit(db_session, member, budget_cents=10_000) projects.record_review( @@ -141,6 +146,7 @@ def test_a_rejection_and_resubmission_send_updates_not_a_second_created( decision=ReviewDecision.REJECT, reason="Too vague.", source=ReviewSource.WEB, + weave_client=weave, ) projects.update_draft(db_session, member, project=project, title="Rover 2", budget_requested_cents=20_000) projects.submit(db_session, member, project=project) @@ -158,8 +164,10 @@ def test_a_rejection_and_resubmission_send_updates_not_a_second_created( assert rows[2].id == event_id(SUBMISSION_UPDATED, project.id, 1) -def test_approval_sends_the_status_and_budget_committed(db_session: Session, member: Actor, reviewer: Actor) -> None: - project = _approved(db_session, member, reviewer, budget_cents=50_000) +def test_approval_sends_the_status_and_budget_committed( + db_session: Session, member: Actor, reviewer: Actor, weave +) -> None: + project = _approved(db_session, member, reviewer, budget_cents=50_000, weave=weave) rows = _rows(db_session, project) assert [row.type for row in rows] == [SUBMISSION_CREATED, SUBMISSION_UPDATED, BUDGET_COMMITTED] @@ -175,24 +183,24 @@ def test_approval_sends_the_status_and_budget_committed(db_session: Session, mem def test_an_approval_that_does_not_meet_the_policy_yet_sends_nothing( - db_session: Session, member: Actor, reviewer: Actor + db_session: Session, member: Actor, reviewer: Actor, weave ) -> None: db_session.add(ApprovalPolicy(stage=ApprovalStage.PROPOSAL, min_approvals=2)) project = _submit(db_session, member) before = _types(db_session, project) - _approve(db_session, reviewer, project) + _approve(db_session, reviewer, project, weave=weave) assert _types(db_session, project) == before def test_an_amendment_increase_sends_the_new_request_then_the_added_budget( - db_session: Session, member: Actor, reviewer: Actor + db_session: Session, member: Actor, reviewer: Actor, weave ) -> None: - project = _approved(db_session, member, reviewer, budget_cents=100_000) + project = _approved(db_session, member, reviewer, budget_cents=100_000, weave=weave) projects.start_amendment(db_session, member, project=project) projects.update_draft(db_session, member, project=project, budget_requested_cents=150_000) projects.submit(db_session, member, project=project) - _approve(db_session, reviewer, project) + _approve(db_session, reviewer, project, weave=weave) rows = _rows(db_session, project)[3:] assert [row.type for row in rows] == [SUBMISSION_UPDATED, BUDGET_COMMITTED] @@ -200,12 +208,14 @@ def test_an_amendment_increase_sends_the_new_request_then_the_added_budget( assert rows[1].payload["amount_cents"] == 50_000 -def test_an_amendment_decrease_sends_budget_released(db_session: Session, member: Actor, reviewer: Actor) -> None: - project = _approved(db_session, member, reviewer, budget_cents=100_000) +def test_an_amendment_decrease_sends_budget_released( + db_session: Session, member: Actor, reviewer: Actor, weave +) -> None: + project = _approved(db_session, member, reviewer, budget_cents=100_000, weave=weave) projects.start_amendment(db_session, member, project=project) projects.update_draft(db_session, member, project=project, budget_requested_cents=60_000) projects.submit(db_session, member, project=project) - _approve(db_session, reviewer, project) + _approve(db_session, reviewer, project, weave=weave) last = _rows(db_session, project)[-1] assert last.type == BUDGET_RELEASED @@ -216,12 +226,12 @@ def test_an_amendment_decrease_sends_budget_released(db_session: Session, member } -def test_an_unchanged_amendment_budget_is_skipped(db_session: Session, member: Actor, reviewer: Actor) -> None: - project = _approved(db_session, member, reviewer, budget_cents=100_000) +def test_an_unchanged_amendment_budget_is_skipped(db_session: Session, member: Actor, reviewer: Actor, weave) -> None: + project = _approved(db_session, member, reviewer, budget_cents=100_000, weave=weave) projects.start_amendment(db_session, member, project=project) projects.update_draft(db_session, member, project=project, write_up="A bigger rover.") projects.submit(db_session, member, project=project) - _approve(db_session, reviewer, project) + _approve(db_session, reviewer, project, weave=weave) zero = db_session.scalars( sa.select(BudgetEntry).where(BudgetEntry.project_id == project.id, BudgetEntry.amount_cents == 0) @@ -232,9 +242,9 @@ def test_an_unchanged_amendment_budget_is_skipped(db_session: Session, member: A def test_a_rejected_amendment_puts_the_approved_request_back( - db_session: Session, member: Actor, reviewer: Actor + db_session: Session, member: Actor, reviewer: Actor, weave ) -> None: - project = _approved(db_session, member, reviewer, budget_cents=100_000) + project = _approved(db_session, member, reviewer, budget_cents=100_000, weave=weave) projects.start_amendment(db_session, member, project=project) projects.update_draft(db_session, member, project=project, budget_requested_cents=150_000) projects.submit(db_session, member, project=project) @@ -245,6 +255,7 @@ def test_a_rejected_amendment_puts_the_approved_request_back( decision=ReviewDecision.REJECT, reason="No.", source=ReviewSource.WEB, + weave_client=weave, ) rows = _rows(db_session, project)[3:] @@ -255,11 +266,15 @@ def test_a_rejected_amendment_puts_the_approved_request_back( def test_admin_adjustments_and_reclaims_follow_the_ledger( - db_session: Session, member: Actor, reviewer: Actor, admin: Actor + db_session: Session, member: Actor, reviewer: Actor, admin: Actor, weave ) -> None: - project = _approved(db_session, member, reviewer, budget_cents=100_000) - projects.admin_adjust_budget(db_session, admin, project=project, amount_cents=25_000, reason="More GPUs.") - projects.admin_adjust_budget(db_session, admin, project=project, amount_cents=-5_000, reason="Less.") + project = _approved(db_session, member, reviewer, budget_cents=100_000, weave=weave) + projects.admin_adjust_budget( + db_session, admin, project=project, amount_cents=25_000, reason="More GPUs.", weave_client=weave + ) + projects.admin_adjust_budget( + db_session, admin, project=project, amount_cents=-5_000, reason="Less.", weave_client=weave + ) projects.reclaim_budget(db_session, admin, project=project, amount_cents=20_000, reason="Stalled.") rows = _rows(db_session, project)[3:] @@ -272,13 +287,13 @@ def test_admin_adjustments_and_reclaims_follow_the_ledger( def test_completion_sends_the_statuses_and_releases_the_unspent_budget( - db_session: Session, member: Actor, reviewer: Actor + db_session: Session, member: Actor, reviewer: Actor, weave ) -> None: - project = _approved(db_session, member, reviewer, budget_cents=100_000) + project = _approved(db_session, member, reviewer, budget_cents=100_000, weave=weave) _spend(db_session, project, 30_000) projects.start_completion(db_session, member, project=project) projects.submit_completion(db_session, member, project=project) - _approve(db_session, reviewer, project) + _approve(db_session, reviewer, project, weave=weave) rows = _rows(db_session, project)[3:] assert [(row.type, row.payload) for row in rows] == [ @@ -292,8 +307,10 @@ def test_completion_sends_the_statuses_and_releases_the_unspent_budget( ] -def test_withdrawal_sends_the_status_and_releases_the_rest(db_session: Session, member: Actor, reviewer: Actor) -> None: - project = _approved(db_session, member, reviewer, budget_cents=100_000) +def test_withdrawal_sends_the_status_and_releases_the_rest( + db_session: Session, member: Actor, reviewer: Actor, weave +) -> None: + project = _approved(db_session, member, reviewer, budget_cents=100_000, weave=weave) _spend(db_session, project, 10_000) projects.withdraw(db_session, member, project=project) @@ -317,9 +334,9 @@ def test_withdrawing_a_pending_submission_sends_only_the_status(db_session: Sess def test_spend_is_sent_as_the_cumulative_total_only_when_it_changes( - db_session: Session, member: Actor, reviewer: Actor + db_session: Session, member: Actor, reviewer: Actor, weave ) -> None: - project = _approved(db_session, member, reviewer) + project = _approved(db_session, member, reviewer, weave=weave) _spend(db_session, project, 1_000) _spend(db_session, project, 1_000) _spend(db_session, project, 2_500) @@ -339,11 +356,13 @@ def test_spend_is_sent_as_the_cumulative_total_only_when_it_changes( def test_a_release_is_capped_at_what_quilt_still_has_committed( - db_session: Session, member: Actor, reviewer: Actor, admin: Actor + db_session: Session, member: Actor, reviewer: Actor, admin: Actor, weave ) -> None: - project = _approved(db_session, member, reviewer, budget_cents=100_000) + project = _approved(db_session, member, reviewer, budget_cents=100_000, weave=weave) _spend(db_session, project, 80_000) # Quilt draws down 80_000; 20_000 stays committed - projects.admin_adjust_budget(db_session, admin, project=project, amount_cents=-50_000, reason="Cut.") + projects.admin_adjust_budget( + db_session, admin, project=project, amount_cents=-50_000, reason="Cut.", weave_client=weave + ) last = _rows(db_session, project)[-1] assert (last.type, last.payload["amount_cents"], last.state) == (BUDGET_RELEASED, 20_000, QuiltOutboxState.PENDING) @@ -356,12 +375,14 @@ def test_a_release_is_capped_at_what_quilt_still_has_committed( def test_a_skipped_release_is_never_sent_later( - db_session: Session, member: Actor, reviewer: Actor, admin: Actor + db_session: Session, member: Actor, reviewer: Actor, admin: Actor, weave ) -> None: - project = _approved(db_session, member, reviewer, budget_cents=10_000) + project = _approved(db_session, member, reviewer, budget_cents=10_000, weave=weave) _spend(db_session, project, 10_000) projects.reclaim_budget(db_session, admin, project=project, amount_cents=5_000, reason="Cut.") - projects.admin_adjust_budget(db_session, admin, project=project, amount_cents=50_000, reason="More.") + projects.admin_adjust_budget( + db_session, admin, project=project, amount_cents=50_000, reason="More.", weave_client=weave + ) rows = _rows(db_session, project) assert [(row.type, row.state) for row in rows[-2:]] == [ @@ -374,13 +395,17 @@ def test_a_skipped_release_is_never_sent_later( def test_a_new_commitment_covers_earlier_overspend_before_the_release_cap( - db_session: Session, member: Actor, reviewer: Actor, admin: Actor + db_session: Session, member: Actor, reviewer: Actor, admin: Actor, weave ) -> None: - project = _approved(db_session, member, reviewer, budget_cents=10_000) + project = _approved(db_session, member, reviewer, budget_cents=10_000, weave=weave) _spend(db_session, project, 12_000) # 2_000 over the commitment - projects.admin_adjust_budget(db_session, admin, project=project, amount_cents=5_000, reason="More.") + projects.admin_adjust_budget( + db_session, admin, project=project, amount_cents=5_000, reason="More.", weave_client=weave + ) # Quilt uses 2_000 of the new 5_000 to cover the overspend, so 3_000 stays committed. - projects.admin_adjust_budget(db_session, admin, project=project, amount_cents=-4_000, reason="Cut.") + projects.admin_adjust_budget( + db_session, admin, project=project, amount_cents=-4_000, reason="Cut.", weave_client=weave + ) last = _rows(db_session, project)[-1] assert (last.type, last.payload["amount_cents"], last.state) == (BUDGET_RELEASED, 3_000, QuiltOutboxState.PENDING) @@ -412,8 +437,8 @@ def test_a_failed_submit_writes_nothing(db_session: Session, member: Actor) -> N assert _rows(db_session, project) == [] -def test_sync_is_idempotent(db_session: Session, member: Actor, reviewer: Actor) -> None: - project = _approved(db_session, member, reviewer) +def test_sync_is_idempotent(db_session: Session, member: Actor, reviewer: Actor, weave) -> None: + project = _approved(db_session, member, reviewer, weave=weave) _spend(db_session, project, 500) count = len(_rows(db_session, project)) @@ -424,14 +449,14 @@ def test_sync_is_idempotent(db_session: Session, member: Actor, reviewer: Actor) def test_backfill_enqueues_existing_projects_once( - db_session: Session, member: Actor, reviewer: Actor, admin: Actor + db_session: Session, member: Actor, reviewer: Actor, admin: Actor, weave ) -> None: draft = projects.create_project(db_session, member, title="Draft", write_up="x", budget_requested_cents=5) - completed = _approved(db_session, member, reviewer, budget_cents=100_000) + completed = _approved(db_session, member, reviewer, budget_cents=100_000, weave=weave) _spend(db_session, completed, 40_000) projects.start_completion(db_session, member, project=completed) projects.submit_completion(db_session, member, project=completed) - _approve(db_session, reviewer, completed) + _approve(db_session, reviewer, completed, weave=weave) pending = _submit(db_session, member, title="Pending") live_budget_ids = {row.id for row in _rows(db_session, completed) if row.type.startswith("budget.")} diff --git a/tests/services/test_reviews.py b/tests/services/test_reviews.py index b0169b4..8da200a 100644 --- a/tests/services/test_reviews.py +++ b/tests/services/test_reviews.py @@ -33,7 +33,7 @@ def _submitted_project(db_session: Session, member: Actor, *, budget_requested_c return project -def test_non_reviewer_cannot_record_a_review(db_session: Session, member: Actor, make_actor) -> None: +def test_non_reviewer_cannot_record_a_review(db_session: Session, member: Actor, make_actor, weave) -> None: project = _submitted_project(db_session, member) plain_member = make_actor(groups=frozenset({"ganymede:member"})) @@ -44,10 +44,11 @@ def test_non_reviewer_cannot_record_a_review(db_session: Session, member: Actor, revision=project.current_revision, decision=ReviewDecision.APPROVE, source=ReviewSource.WEB, + weave_client=weave, ) -def test_submitter_cannot_review_their_own_submission(db_session: Session, make_actor) -> None: +def test_submitter_cannot_review_their_own_submission(db_session: Session, make_actor, weave) -> None: submitter_and_reviewer = make_actor(groups=frozenset({"ganymede:member", "ganymede:reviewer"})) project = _submitted_project(db_session, submitter_and_reviewer) @@ -58,10 +59,11 @@ def test_submitter_cannot_review_their_own_submission(db_session: Session, make_ revision=project.current_revision, decision=ReviewDecision.APPROVE, source=ReviewSource.WEB, + weave_client=weave, ) -def test_credited_builder_cannot_review(db_session: Session, member: Actor, make_actor) -> None: +def test_credited_builder_cannot_review(db_session: Session, member: Actor, make_actor, weave) -> None: builder = make_actor(groups=frozenset({"ganymede:member", "ganymede:reviewer"})) project = _submitted_project(db_session, member) project.current_revision.credited_builder_ids = [builder.user.id] @@ -74,10 +76,13 @@ def test_credited_builder_cannot_review(db_session: Session, member: Actor, make revision=project.current_revision, decision=ReviewDecision.APPROVE, source=ReviewSource.WEB, + weave_client=weave, ) -def test_double_review_by_the_same_reviewer_is_blocked(db_session: Session, member: Actor, reviewer: Actor) -> None: +def test_double_review_by_the_same_reviewer_is_blocked( + db_session: Session, member: Actor, reviewer: Actor, weave +) -> None: project = _submitted_project(db_session, member) # First approval alone would satisfy the default policy and close out the revision, so use a # multi-approval policy to keep it open for a second decision attempt. @@ -90,6 +95,7 @@ def test_double_review_by_the_same_reviewer_is_blocked(db_session: Session, memb revision=project.current_revision, decision=ReviewDecision.APPROVE, source=ReviewSource.WEB, + weave_client=weave, ) with pytest.raises(InvalidState): @@ -99,11 +105,12 @@ def test_double_review_by_the_same_reviewer_is_blocked(db_session: Session, memb revision=project.current_revision, decision=ReviewDecision.APPROVE, source=ReviewSource.WEB, + weave_client=weave, ) def test_concurrent_double_review_is_blocked_by_the_db_constraint( - db_session: Session, member: Actor, reviewer: Actor, monkeypatch: pytest.MonkeyPatch + db_session: Session, member: Actor, reviewer: Actor, monkeypatch: pytest.MonkeyPatch, weave ) -> None: """Simulates two concurrent requests that both pass the `_has_existing_review` pre-check before either has inserted its row (e.g. a double-click, or a retried Slack action): the `reviews` unique @@ -127,6 +134,7 @@ def test_concurrent_double_review_is_blocked_by_the_db_constraint( revision=project.current_revision, decision=ReviewDecision.APPROVE, source=ReviewSource.WEB, + weave_client=weave, ) db_session.commit() @@ -138,6 +146,7 @@ def test_concurrent_double_review_is_blocked_by_the_db_constraint( revision=project.current_revision, decision=ReviewDecision.APPROVE, source=ReviewSource.WEB, + weave_client=weave, ) db_session.rollback() @@ -151,7 +160,7 @@ def test_concurrent_double_review_is_blocked_by_the_db_constraint( assert review_count == 1 -def test_reject_requires_a_reason(db_session: Session, member: Actor, reviewer: Actor) -> None: +def test_reject_requires_a_reason(db_session: Session, member: Actor, reviewer: Actor, weave) -> None: project = _submitted_project(db_session, member) with pytest.raises(ValidationFailed): @@ -161,11 +170,12 @@ def test_reject_requires_a_reason(db_session: Session, member: Actor, reviewer: revision=project.current_revision, decision=ReviewDecision.REJECT, source=ReviewSource.WEB, + weave_client=weave, ) def test_a_single_reject_rejects_the_revision_immediately_and_opens_the_next_draft( - db_session: Session, member: Actor, reviewer: Actor + db_session: Session, member: Actor, reviewer: Actor, weave ) -> None: project = _submitted_project(db_session, member, budget_requested_cents=10_000) rejected_revision = project.current_revision @@ -177,6 +187,7 @@ def test_a_single_reject_rejects_the_revision_immediately_and_opens_the_next_dra decision=ReviewDecision.REJECT, reason="Needs more detail.", source=ReviewSource.WEB, + weave_client=weave, ) db_session.refresh(project) @@ -190,7 +201,9 @@ def test_a_single_reject_rejects_the_revision_immediately_and_opens_the_next_dra assert new_draft.budget_requested_cents == 10_000 -def test_resubmit_after_reject_does_not_count_old_reviews(db_session: Session, member: Actor, make_actor) -> None: +def test_resubmit_after_reject_does_not_count_old_reviews( + db_session: Session, member: Actor, make_actor, weave +) -> None: """A reviewer's rejection of an earlier revision must not carry over to (or block approval of) the resubmitted one -- only the current revision's own reviews count toward the policy.""" reviewer_a = make_actor(groups=frozenset({"ganymede:member", "ganymede:reviewer"})) @@ -206,6 +219,7 @@ def test_resubmit_after_reject_does_not_count_old_reviews(db_session: Session, m decision=ReviewDecision.REJECT, reason="Not ready.", source=ReviewSource.WEB, + weave_client=weave, ) db_session.refresh(project) assert project.status is ProjectStatus.CHANGES_REQUESTED @@ -218,14 +232,19 @@ def test_resubmit_after_reject_does_not_count_old_reviews(db_session: Session, m # The default policy (1 approval, any reviewer) is satisfied by reviewer_b's single approval, even # though reviewer_a already rejected the *first* revision. projects.record_review( - db_session, reviewer_b, revision=second_revision, decision=ReviewDecision.APPROVE, source=ReviewSource.WEB + db_session, + reviewer_b, + revision=second_revision, + decision=ReviewDecision.APPROVE, + source=ReviewSource.WEB, + weave_client=weave, ) db_session.refresh(project) assert project.status is ProjectStatus.APPROVED assert project.approved_revision_id == second_revision.id -def test_cannot_review_a_non_current_revision(db_session: Session, member: Actor, reviewer: Actor) -> None: +def test_cannot_review_a_non_current_revision(db_session: Session, member: Actor, reviewer: Actor, weave) -> None: project = _submitted_project(db_session, member) old_revision = project.current_revision @@ -236,6 +255,7 @@ def test_cannot_review_a_non_current_revision(db_session: Session, member: Actor decision=ReviewDecision.REJECT, reason="Needs work.", source=ReviewSource.WEB, + weave_client=weave, ) with pytest.raises(InvalidState): @@ -245,10 +265,11 @@ def test_cannot_review_a_non_current_revision(db_session: Session, member: Actor revision=old_revision, decision=ReviewDecision.APPROVE, source=ReviewSource.WEB, + weave_client=weave, ) -def test_cannot_review_a_draft_revision(db_session: Session, member: Actor, reviewer: Actor) -> None: +def test_cannot_review_a_draft_revision(db_session: Session, member: Actor, reviewer: Actor, weave) -> None: project = projects.create_project(db_session, member, title="T", write_up="W", budget_requested_cents=1_000) with pytest.raises(InvalidState): @@ -258,11 +279,12 @@ def test_cannot_review_a_draft_revision(db_session: Session, member: Actor, revi revision=project.current_revision, decision=ReviewDecision.APPROVE, source=ReviewSource.WEB, + weave_client=weave, ) def test_withdraw_supersedes_the_pending_revision_and_a_review_cannot_bring_it_back( - db_session: Session, member: Actor, reviewer: Actor + db_session: Session, member: Actor, reviewer: Actor, weave ) -> None: """A withdrawn project's still-`pending` submitted revision must not be approvable back to life: `withdraw` has to mark it `superseded`, and `record_review` has to reject it regardless.""" @@ -282,6 +304,7 @@ def test_withdraw_supersedes_the_pending_revision_and_a_review_cannot_bring_it_b revision=pending_revision, decision=ReviewDecision.APPROVE, source=ReviewSource.WEB, + weave_client=weave, ) db_session.refresh(project) assert project.status is ProjectStatus.WITHDRAWN @@ -298,27 +321,39 @@ def test_withdrawn_project_does_not_appear_in_the_review_queue( assert project.current_revision not in queue -def test_admin_decide_cannot_resurrect_a_withdrawn_project(db_session: Session, member: Actor, admin: Actor) -> None: +def test_admin_decide_cannot_resurrect_a_withdrawn_project( + db_session: Session, member: Actor, admin: Actor, weave +) -> None: project = _submitted_project(db_session, member) pending_revision = project.current_revision projects.withdraw(db_session, member, project=project) with pytest.raises(InvalidState): projects.admin_decide( - db_session, admin, revision=pending_revision, decision=ReviewDecision.APPROVE, reason="override" + db_session, + admin, + revision=pending_revision, + decision=ReviewDecision.APPROVE, + reason="override", + weave_client=weave, ) db_session.refresh(project) assert project.status is ProjectStatus.WITHDRAWN def test_cannot_review_a_completion_revision_while_project_is_not_in_completion_review( - db_session: Session, member: Actor, admin: Actor, reviewer: Actor + db_session: Session, member: Actor, admin: Actor, reviewer: Actor, weave ) -> None: """A completion revision must only be decided while the project is `pending_completion_review` -- e.g. not after an admin has withdrawn the project out from under an in-flight completion review.""" project = _submitted_project(db_session, member) projects.admin_decide( - db_session, admin, revision=project.current_revision, decision=ReviewDecision.APPROVE, reason="ok" + db_session, + admin, + revision=project.current_revision, + decision=ReviewDecision.APPROVE, + reason="ok", + weave_client=weave, ) db_session.refresh(project) projects.start_completion(db_session, member, project=project) @@ -339,4 +374,5 @@ def test_cannot_review_a_completion_revision_while_project_is_not_in_completion_ revision=completion_revision, decision=ReviewDecision.APPROVE, source=ReviewSource.WEB, + weave_client=weave, ) diff --git a/tests/services/test_screenshots.py b/tests/services/test_screenshots.py index 16f5fd5..f14f6a3 100644 --- a/tests/services/test_screenshots.py +++ b/tests/services/test_screenshots.py @@ -27,7 +27,7 @@ def store() -> FakeObjectStore: } -def _completion_draft(db_session: Session, member: Actor, reviewer: Actor): +def _completion_draft(db_session: Session, member: Actor, reviewer: Actor, weave): """An approved project with a fresh, unsubmitted completion draft.""" project = projects.create_project( db_session, member, title="Rover", write_up="A rover.", budget_requested_cents=100_000 @@ -39,6 +39,7 @@ def _completion_draft(db_session: Session, member: Actor, reviewer: Actor): revision=project.current_revision, decision=ReviewDecision.APPROVE, source=ReviewSource.WEB, + weave_client=weave, ) db_session.refresh(project) assert project.status is ProjectStatus.APPROVED @@ -70,18 +71,18 @@ def _confirm_upload( def test_non_submitter_cannot_presign( - db_session: Session, member: Actor, reviewer: Actor, store: FakeObjectStore + db_session: Session, member: Actor, reviewer: Actor, store: FakeObjectStore, weave ) -> None: - project, _draft = _completion_draft(db_session, member, reviewer) + project, _draft = _completion_draft(db_session, member, reviewer, weave=weave) with pytest.raises(NotAllowed): screenshots.presign_screenshot(db_session, reviewer, project=project, content_type="image/png", store=store) def test_non_submitter_cannot_confirm( - db_session: Session, member: Actor, reviewer: Actor, store: FakeObjectStore + db_session: Session, member: Actor, reviewer: Actor, store: FakeObjectStore, weave ) -> None: - project, draft = _completion_draft(db_session, member, reviewer) + project, draft = _completion_draft(db_session, member, reviewer, weave=weave) upload = screenshots.presign_screenshot(db_session, member, project=project, content_type="image/png", store=store) store.put(upload.key, content_type="image/png", size_bytes=100) @@ -90,9 +91,9 @@ def test_non_submitter_cannot_confirm( def test_non_submitter_cannot_remove( - db_session: Session, member: Actor, reviewer: Actor, store: FakeObjectStore + db_session: Session, member: Actor, reviewer: Actor, store: FakeObjectStore, weave ) -> None: - project, _draft = _completion_draft(db_session, member, reviewer) + project, _draft = _completion_draft(db_session, member, reviewer, weave=weave) key = _confirm_upload(db_session, member, project=project, store=store) with pytest.raises(NotAllowed): @@ -108,9 +109,9 @@ def test_cannot_presign_on_a_non_completion_draft(db_session: Session, member: A def test_cannot_presign_once_completion_is_submitted( - db_session: Session, member: Actor, reviewer: Actor, store: FakeObjectStore + db_session: Session, member: Actor, reviewer: Actor, store: FakeObjectStore, weave ) -> None: - project, draft = _completion_draft(db_session, member, reviewer) + project, draft = _completion_draft(db_session, member, reviewer, weave=weave) projects.update_draft(db_session, member, project=project, write_up="Done.") projects.submit_completion(db_session, member, project=project) @@ -123,8 +124,10 @@ def test_cannot_presign_once_completion_is_submitted( # -------------------------------------------------------------------------------------------------- -def test_at_most_six_screenshots(db_session: Session, member: Actor, reviewer: Actor, store: FakeObjectStore) -> None: - project, _draft = _completion_draft(db_session, member, reviewer) +def test_at_most_six_screenshots( + db_session: Session, member: Actor, reviewer: Actor, store: FakeObjectStore, weave +) -> None: + project, _draft = _completion_draft(db_session, member, reviewer, weave=weave) for _ in range(screenshots.MAX_SCREENSHOTS): _confirm_upload(db_session, member, project=project, store=store) @@ -133,10 +136,10 @@ def test_at_most_six_screenshots(db_session: Session, member: Actor, reviewer: A def test_confirm_also_enforces_the_limit( - db_session: Session, member: Actor, reviewer: Actor, store: FakeObjectStore + db_session: Session, member: Actor, reviewer: Actor, store: FakeObjectStore, weave ) -> None: """Two presigns can be issued before either confirms; confirm re-checks the limit, not just presign.""" - project, draft = _completion_draft(db_session, member, reviewer) + project, draft = _completion_draft(db_session, member, reviewer, weave=weave) for _ in range(screenshots.MAX_SCREENSHOTS - 1): _confirm_upload(db_session, member, project=project, store=store) @@ -157,18 +160,18 @@ def test_confirm_also_enforces_the_limit( def test_presign_rejects_a_disallowed_content_type( - db_session: Session, member: Actor, reviewer: Actor, store: FakeObjectStore + db_session: Session, member: Actor, reviewer: Actor, store: FakeObjectStore, weave ) -> None: - project, _draft = _completion_draft(db_session, member, reviewer) + project, _draft = _completion_draft(db_session, member, reviewer, weave=weave) with pytest.raises(ValidationFailed): screenshots.presign_screenshot(db_session, member, project=project, content_type="image/gif", store=store) def test_confirm_rejects_a_missing_object( - db_session: Session, member: Actor, reviewer: Actor, store: FakeObjectStore + db_session: Session, member: Actor, reviewer: Actor, store: FakeObjectStore, weave ) -> None: - project, draft = _completion_draft(db_session, member, reviewer) + project, draft = _completion_draft(db_session, member, reviewer, weave=weave) with pytest.raises(ValidationFailed): screenshots.confirm_screenshot( @@ -177,11 +180,11 @@ def test_confirm_rejects_a_missing_object( def test_confirm_rejects_an_oversized_object( - db_session: Session, member: Actor, reviewer: Actor, store: FakeObjectStore + db_session: Session, member: Actor, reviewer: Actor, store: FakeObjectStore, weave ) -> None: """The presigned policy is storage's own enforcement; confirm re-checks server-side too, since a storage backend that doesn't honor every policy condition must not let an oversized object in.""" - project, draft = _completion_draft(db_session, member, reviewer) + project, draft = _completion_draft(db_session, member, reviewer, weave=weave) upload = screenshots.presign_screenshot(db_session, member, project=project, content_type="image/png", store=store) store.put(upload.key, content_type="image/png", size_bytes=screenshots.MAX_SCREENSHOT_BYTES + 1) @@ -193,13 +196,13 @@ def test_confirm_rejects_an_oversized_object( def test_confirm_rejects_a_mismatched_content_type( - db_session: Session, member: Actor, reviewer: Actor, store: FakeObjectStore + db_session: Session, member: Actor, reviewer: Actor, store: FakeObjectStore, weave ) -> None: """Defense in depth: `head`'s reported content type is checked again at confirm time, independent of whatever the presigned policy required at upload time (a real S3-compatible backend does enforce that condition -- confirmed against SeaweedFS in tests/live/test_storage_live.py -- but this check doesn't rely on it).""" - project, draft = _completion_draft(db_session, member, reviewer) + project, draft = _completion_draft(db_session, member, reviewer, weave=weave) upload = screenshots.presign_screenshot(db_session, member, project=project, content_type="image/png", store=store) store.put(upload.key, content_type="text/plain", size_bytes=10) @@ -210,12 +213,12 @@ def test_confirm_rejects_a_mismatched_content_type( def test_confirm_rejects_bytes_that_dont_match_the_declared_signature( - db_session: Session, member: Actor, reviewer: Actor, store: FakeObjectStore + db_session: Session, member: Actor, reviewer: Actor, store: FakeObjectStore, weave ) -> None: """A client can legitimately sign a presigned POST for `image/png` and then upload arbitrary bytes under that label (the stored Content-Type comes from the signed form field, not the bytes -- see docs/dev/storage.md); the magic-byte check is what actually catches that.""" - project, draft = _completion_draft(db_session, member, reviewer) + project, draft = _completion_draft(db_session, member, reviewer, weave=weave) upload = screenshots.presign_screenshot(db_session, member, project=project, content_type="image/png", store=store) store.put(upload.key, content_type="image/png", size_bytes=100, content=b"not actually a png file at all!") @@ -228,9 +231,9 @@ def test_confirm_rejects_bytes_that_dont_match_the_declared_signature( @pytest.mark.parametrize("content_type", sorted(screenshots.ALLOWED_CONTENT_TYPES)) def test_confirm_accepts_a_valid_signature_for_every_allowed_type( - db_session: Session, member: Actor, reviewer: Actor, store: FakeObjectStore, content_type: str + db_session: Session, member: Actor, reviewer: Actor, store: FakeObjectStore, content_type: str, weave ) -> None: - project, draft = _completion_draft(db_session, member, reviewer) + project, draft = _completion_draft(db_session, member, reviewer, weave=weave) key = _confirm_upload(db_session, member, project=project, store=store, content_type=content_type) @@ -238,9 +241,9 @@ def test_confirm_accepts_a_valid_signature_for_every_allowed_type( def test_confirm_rejects_a_key_outside_this_draft( - db_session: Session, member: Actor, reviewer: Actor, store: FakeObjectStore + db_session: Session, member: Actor, reviewer: Actor, store: FakeObjectStore, weave ) -> None: - project, _draft = _completion_draft(db_session, member, reviewer) + project, _draft = _completion_draft(db_session, member, reviewer, weave=weave) store.put("projects/someone-elses/rev/x.png", content_type="image/png", size_bytes=10) with pytest.raises(NotAllowed): @@ -249,8 +252,10 @@ def test_confirm_rejects_a_key_outside_this_draft( ) -def test_confirm_is_idempotent(db_session: Session, member: Actor, reviewer: Actor, store: FakeObjectStore) -> None: - project, draft = _completion_draft(db_session, member, reviewer) +def test_confirm_is_idempotent( + db_session: Session, member: Actor, reviewer: Actor, store: FakeObjectStore, weave +) -> None: + project, draft = _completion_draft(db_session, member, reviewer, weave=weave) key = _confirm_upload(db_session, member, project=project, store=store) screenshots.confirm_screenshot(db_session, member, project=project, key=key, store=store) @@ -263,9 +268,9 @@ def test_confirm_is_idempotent(db_session: Session, member: Actor, reviewer: Act def test_remove_deletes_from_the_draft_and_storage( - db_session: Session, member: Actor, reviewer: Actor, store: FakeObjectStore + db_session: Session, member: Actor, reviewer: Actor, store: FakeObjectStore, weave ) -> None: - project, draft = _completion_draft(db_session, member, reviewer) + project, draft = _completion_draft(db_session, member, reviewer, weave=weave) key = _confirm_upload(db_session, member, project=project, store=store) assert store.head(key) is not None @@ -276,12 +281,12 @@ def test_remove_deletes_from_the_draft_and_storage( def test_remove_is_best_effort_against_storage_failures( - db_session: Session, member: Actor, reviewer: Actor, store: FakeObjectStore + db_session: Session, member: Actor, reviewer: Actor, store: FakeObjectStore, weave ) -> None: """Storage being unreachable shouldn't block dropping the key from the draft.""" from krater.storage.errors import StorageUnavailableError - project, draft = _completion_draft(db_session, member, reviewer) + project, draft = _completion_draft(db_session, member, reviewer, weave=weave) key = _confirm_upload(db_session, member, project=project, store=store) def _broken_delete(_key: str) -> None: @@ -294,9 +299,9 @@ def _broken_delete(_key: str) -> None: def test_cannot_remove_a_key_not_on_the_draft( - db_session: Session, member: Actor, reviewer: Actor, store: FakeObjectStore + db_session: Session, member: Actor, reviewer: Actor, store: FakeObjectStore, weave ) -> None: - project, draft = _completion_draft(db_session, member, reviewer) + project, draft = _completion_draft(db_session, member, reviewer, weave=weave) with pytest.raises(InvalidState): screenshots.remove_screenshot( diff --git a/tests/services/test_slack_notify.py b/tests/services/test_slack_notify.py index fd64bf8..461e533 100644 --- a/tests/services/test_slack_notify.py +++ b/tests/services/test_slack_notify.py @@ -95,6 +95,7 @@ def test_no_feed_line_for_a_resubmission_after_rejection( decision=ReviewDecision.REJECT, reason="no", source=ReviewSource.WEB, + weave_client=weave, ) db_session.refresh(project) projects.update_draft(db_session, member, project=project, write_up="revised") @@ -120,6 +121,7 @@ def test_no_feed_line_for_an_amendment( revision=project.current_revision, decision=ReviewDecision.APPROVE, source=ReviewSource.WEB, + weave_client=weave, ) db_session.refresh(project) projects.start_amendment(db_session, member, project=project) @@ -143,7 +145,12 @@ def test_notify_decision_updates_the_message_and_drops_the_buttons( slack_notify.notify_revision_submitted(db_session, slack_client, weave, revision=revision, feed_channel_id=None) projects.record_review( - db_session, reviewer, revision=revision, decision=ReviewDecision.APPROVE, source=ReviewSource.WEB + db_session, + reviewer, + revision=revision, + decision=ReviewDecision.APPROVE, + source=ReviewSource.WEB, + weave_client=weave, ) db_session.refresh(revision) assert revision.outcome is RevisionOutcome.APPROVED @@ -478,6 +485,7 @@ def test_notify_decision_escapes_the_title_and_reject_reason( decision=ReviewDecision.REJECT, reason=_INJECTION, source=ReviewSource.WEB, + weave_client=weave, ) db_session.refresh(revision) diff --git a/tests/services/test_slack_reviews.py b/tests/services/test_slack_reviews.py index f1e708a..ad8644c 100644 --- a/tests/services/test_slack_reviews.py +++ b/tests/services/test_slack_reviews.py @@ -144,6 +144,7 @@ def test_approve_that_completes_the_project_archives_the_channel( revision=project.current_revision, decision=ReviewDecision.APPROVE, source=ReviewSource.WEB, + weave_client=weave, ) db_session.refresh(project) assert project.status is ProjectStatus.APPROVED @@ -225,3 +226,41 @@ def _down(sub: str, *, fresh: bool = False): db_session.refresh(revision) assert revision.outcome is RevisionOutcome.PENDING assert "Weave" in slack_client.ephemeral_messages[0][1] + + +def test_a_slack_approval_is_refused_while_a_minor_has_no_verified_guardian( + db_session: Session, member: Actor, reviewer: Actor, weave: StubWeaveClient +) -> None: + # The same service rule as the web path: `record_review` runs the guardian gate. + reviewer.user.slack_user_id = "U_REVIEWER" + weave.set_minor(member.user.weave_sub, True) + project, revision = _submitted_revision(db_session, member) + slack_client = FakeSlackClient() + + _approve(db_session, slack_client, weave, revision, "U_REVIEWER") + + db_session.refresh(revision) + db_session.refresh(project) + assert revision.outcome is RevisionOutcome.PENDING + assert project.status is ProjectStatus.PENDING_REVIEW + assert revision.reviews == [] + assert len(slack_client.ephemeral_messages) == 1 + text = slack_client.ephemeral_messages[0][1] + assert "verified guardian in Weave" in text + assert member.user.weave_sub in text + + +def test_a_slack_approval_goes_through_once_weave_verifies_the_guardian( + db_session: Session, member: Actor, reviewer: Actor, weave: StubWeaveClient +) -> None: + reviewer.user.slack_user_id = "U_REVIEWER" + weave.set_minor(member.user.weave_sub, True) + weave.set_guardian_verified(member.user.weave_sub, True) + _project, revision = _submitted_revision(db_session, member) + slack_client = FakeSlackClient() + + _approve(db_session, slack_client, weave, revision, "U_REVIEWER") + + db_session.refresh(revision) + assert revision.outcome is RevisionOutcome.APPROVED + assert slack_client.ephemeral_messages == [] diff --git a/tests/services/test_validation_and_state.py b/tests/services/test_validation_and_state.py index 3875bda..c0f3e68 100644 --- a/tests/services/test_validation_and_state.py +++ b/tests/services/test_validation_and_state.py @@ -54,12 +54,17 @@ def test_submit_fails_without_a_draft(db_session: Session, member: Actor, review def test_submit_on_a_completion_draft_directs_to_submit_completion( - db_session: Session, member: Actor, admin: Actor + db_session: Session, member: Actor, admin: Actor, weave ) -> None: project = projects.create_project(db_session, member, title="x", write_up="y", budget_requested_cents=100) project = projects.submit(db_session, member, project=project) projects.admin_decide( - db_session, admin, revision=project.current_revision, decision=ReviewDecision.APPROVE, reason="ok" + db_session, + admin, + revision=project.current_revision, + decision=ReviewDecision.APPROVE, + reason="ok", + weave_client=weave, ) db_session.refresh(project) projects.start_completion(db_session, member, project=project) @@ -83,12 +88,17 @@ def test_start_completion_requires_an_approved_project(db_session: Session, memb def test_start_amendment_fails_with_a_draft_already_in_progress( - db_session: Session, member: Actor, admin: Actor + db_session: Session, member: Actor, admin: Actor, weave ) -> None: project = projects.create_project(db_session, member, title="x", write_up="y", budget_requested_cents=100) project = projects.submit(db_session, member, project=project) projects.admin_decide( - db_session, admin, revision=project.current_revision, decision=ReviewDecision.APPROVE, reason="ok" + db_session, + admin, + revision=project.current_revision, + decision=ReviewDecision.APPROVE, + reason="ok", + weave_client=weave, ) db_session.refresh(project) projects.start_amendment(db_session, member, project=project) @@ -97,11 +107,16 @@ def test_start_amendment_fails_with_a_draft_already_in_progress( projects.start_amendment(db_session, member, project=project) -def test_submit_completion_requires_a_completion_draft(db_session: Session, member: Actor, admin: Actor) -> None: +def test_submit_completion_requires_a_completion_draft(db_session: Session, member: Actor, admin: Actor, weave) -> None: project = projects.create_project(db_session, member, title="x", write_up="y", budget_requested_cents=100) project = projects.submit(db_session, member, project=project) projects.admin_decide( - db_session, admin, revision=project.current_revision, decision=ReviewDecision.APPROVE, reason="ok" + db_session, + admin, + revision=project.current_revision, + decision=ReviewDecision.APPROVE, + reason="ok", + weave_client=weave, ) db_session.refresh(project) @@ -138,7 +153,7 @@ def test_review_queue_requires_reviewer(db_session: Session, member: Actor) -> N def test_review_queue_excludes_own_projects_credited_builders_and_already_reviewed( - db_session: Session, member: Actor, reviewer: Actor, make_actor + db_session: Session, member: Actor, reviewer: Actor, make_actor, weave ) -> None: other_member = make_actor(groups=frozenset({"ganymede:member"})) @@ -173,13 +188,14 @@ def test_review_queue_excludes_own_projects_credited_builders_and_already_review decision=ReviewDecision.REJECT, reason="not ready", source=ReviewSource.WEB, + weave_client=weave, ) queue_after = {r.id for r in projects.review_queue(db_session, reviewer)} assert visible.current_revision.id not in queue_after # rejected -> no longer pending anyway def test_project_summary_reports_policy_explanation_while_pending( - db_session: Session, member: Actor, reviewer: Actor + db_session: Session, member: Actor, reviewer: Actor, weave ) -> None: project = projects.create_project(db_session, member, title="x", write_up="y", budget_requested_cents=1_000) project = projects.submit(db_session, member, project=project) @@ -196,6 +212,7 @@ def test_project_summary_reports_policy_explanation_while_pending( revision=project.current_revision, decision=ReviewDecision.APPROVE, source=ReviewSource.WEB, + weave_client=weave, ) db_session.refresh(project) summary = projects.project_summary(db_session, project=project) diff --git a/tests/services/test_workflow_happy_path.py b/tests/services/test_workflow_happy_path.py index 7326d8b..3fdcd3a 100644 --- a/tests/services/test_workflow_happy_path.py +++ b/tests/services/test_workflow_happy_path.py @@ -20,7 +20,9 @@ from krater.services.actor import Actor -def test_happy_path_proposal_amendment_and_completion(db_session: Session, member: Actor, reviewer: Actor) -> None: +def test_happy_path_proposal_amendment_and_completion( + db_session: Session, member: Actor, reviewer: Actor, weave +) -> None: # --- Draft & submit the proposal -------------------------------------------------------- project = projects.create_project(db_session, member, title="", write_up="", budget_requested_cents=0) assert project.status is ProjectStatus.DRAFT @@ -51,6 +53,7 @@ def test_happy_path_proposal_amendment_and_completion(db_session: Session, membe revision=proposal_revision, decision=ReviewDecision.APPROVE, source=ReviewSource.WEB, + weave_client=weave, ) assert review.reviewer_groups == sorted(reviewer.groups) @@ -74,7 +77,12 @@ def test_happy_path_proposal_amendment_and_completion(db_session: Session, membe assert amendment.submitted_at is not None projects.record_review( - db_session, reviewer, revision=amendment, decision=ReviewDecision.APPROVE, source=ReviewSource.WEB + db_session, + reviewer, + revision=amendment, + decision=ReviewDecision.APPROVE, + source=ReviewSource.WEB, + weave_client=weave, ) db_session.refresh(project) assert project.status is ProjectStatus.APPROVED @@ -105,7 +113,12 @@ def test_happy_path_proposal_amendment_and_completion(db_session: Session, membe db_session.flush() projects.record_review( - db_session, reviewer, revision=completion, decision=ReviewDecision.APPROVE, source=ReviewSource.WEB + db_session, + reviewer, + revision=completion, + decision=ReviewDecision.APPROVE, + source=ReviewSource.WEB, + weave_client=weave, ) db_session.refresh(project) assert project.status is ProjectStatus.COMPLETED diff --git a/tests/web/conftest.py b/tests/web/conftest.py index 69284dc..a1784dc 100644 --- a/tests/web/conftest.py +++ b/tests/web/conftest.py @@ -53,7 +53,9 @@ def _submit(user: User, **kwargs) -> Project: @pytest.fixture -def approved_project(db_session: Session, submitted_project: Callable[..., Project]) -> Callable[..., Project]: +def approved_project( + db_session: Session, submitted_project: Callable[..., Project], weave_stub +) -> Callable[..., Project]: """Factory: create, submit and get `reviewer` to approve a project for `user`.""" def _approve(user: User, reviewer: User, **kwargs) -> Project: @@ -64,6 +66,7 @@ def _approve(user: User, reviewer: User, **kwargs) -> Project: revision=project.current_revision, decision=ReviewDecision.APPROVE, source=ReviewSource.WEB, + weave_client=weave_stub, ) db_session.refresh(project) return project diff --git a/tests/web/test_gallery.py b/tests/web/test_gallery.py index c801324..d4253ba 100644 --- a/tests/web/test_gallery.py +++ b/tests/web/test_gallery.py @@ -15,7 +15,9 @@ from tests.web.conftest import PNG_SIGNATURE, actor_for -def _completed_project(db_session: Session, approved_project, member, reviewer, *, with_screenshot: bool = False): +def _completed_project( + db_session: Session, approved_project, member, reviewer, weave_stub, *, with_screenshot: bool = False +): project = approved_project(member, reviewer, title="Gallery Project") project_service.start_completion(db_session, actor_for(member), project=project) project_service.update_draft( @@ -41,6 +43,7 @@ def _completed_project(db_session: Session, approved_project, member, reviewer, revision=project.current_revision, decision=ReviewDecision.APPROVE, source=ReviewSource.WEB, + weave_client=weave_stub, ) db_session.refresh(project) return project @@ -67,11 +70,11 @@ def test_gallery_index_shows_only_completed_projects( def test_gallery_detail_shows_a_completed_project( - client: TestClient, login_as, approved_project, db_session: Session + client: TestClient, login_as, approved_project, db_session: Session, weave_stub ) -> None: member = login_as(MEMBER_SUB) reviewer = login_as(REVIEWER_SUB) - project = _completed_project(db_session, approved_project, member, reviewer) + project = _completed_project(db_session, approved_project, member, reviewer, weave_stub=weave_stub) index_response = client.get("/gallery") assert "Gallery Project" in index_response.text @@ -99,11 +102,13 @@ def test_gallery_detail_404s_for_an_unknown_project(client: TestClient) -> None: def test_gallery_shows_screenshots_as_presigned_urls( - client: TestClient, login_as, approved_project, db_session: Session + client: TestClient, login_as, approved_project, db_session: Session, weave_stub ) -> None: member = login_as(MEMBER_SUB) reviewer = login_as(REVIEWER_SUB) - project = _completed_project(db_session, approved_project, member, reviewer, with_screenshot=True) + project = _completed_project( + db_session, approved_project, member, reviewer, with_screenshot=True, weave_stub=weave_stub + ) key = project.current_revision.screenshot_keys[0] index_response = client.get("/gallery") From 8447450015be122652015b17cfd320594d541989 Mon Sep 17 00:00:00 2001 From: Jasper Mayone Date: Mon, 12 Oct 2026 13:10:00 -0400 Subject: [PATCH 3/4] feat: show members who need a verified guardian on the project page --- krater/web/static/css/main.css | 13 +++ krater/web/templates/projects/detail.html | 18 ++++ tests/web/test_guardian_gate.py | 126 ++++++++++++++++++++++ 3 files changed, 157 insertions(+) create mode 100644 tests/web/test_guardian_gate.py diff --git a/krater/web/static/css/main.css b/krater/web/static/css/main.css index d1d08bd..bdab58d 100644 --- a/krater/web/static/css/main.css +++ b/krater/web/static/css/main.css @@ -711,6 +711,7 @@ a.stat .stat__value { .policy-note, .compute-hint, .budget-banner, +.guardian-banner, .notice { margin: 0 0 var(--space-4); padding: var(--space-3) var(--space-4); @@ -747,6 +748,18 @@ a.stat .stat__value { background: var(--warn-soft); } +/* A team member under 18 needs a guardian verification before Krater grants budget. */ +.guardian-banner { + border-color: var(--warn-soft); + border-left-color: var(--warn); + background: var(--warn-soft); +} + +.guardian-banner ul { + margin: var(--space-2) 0 0; + padding-left: var(--space-5); +} + /* ── Flash messages ── */ .flash-list { diff --git a/krater/web/templates/projects/detail.html b/krater/web/templates/projects/detail.html index 945ddc4..b0da1f0 100644 --- a/krater/web/templates/projects/detail.html +++ b/krater/web/templates/projects/detail.html @@ -20,6 +20,24 @@

{{ project.title or "Untitled proposal" }} {{ macros.status_badge(project.st
Remaining
{{ summary.remaining_cents | dollars }}
+{% if guardian_pending %} +
+

Krater can't grant budget to this project yet. These team members must be cleared in + Weave first:

+
    + {% for member in guardian_pending %} +
  • {{ member.label }}: {{ member.reason }}
    • + {% endfor %} +
+

For a member under 18, a Weave admin records the signed guardian form and the call or video check on the + member's Weave admin page. A member who is not available in Weave needs their access to Krater back in Weave. + Then a reviewer or admin can approve again.

+
+{% elif guardian_pending is none %} +

Krater couldn't reach Weave to check whether a team member needs a verified guardian. + Krater checks again before it grants budget.

+{% endif %} +
diff --git a/tests/web/test_guardian_gate.py b/tests/web/test_guardian_gate.py new file mode 100644 index 0000000..7d009b6 --- /dev/null +++ b/tests/web/test_guardian_gate.py @@ -0,0 +1,126 @@ +"""HTTP-level tests for the guardian gate: the web approve paths refuse a grant while a team member is a minor +without a verified guardian in Weave, and the project page says who needs one. The rule itself is covered in +tests/services/test_guardian.py.""" + +from __future__ import annotations + +from fastapi.testclient import TestClient +from sqlalchemy.orm import Session + +from krater.models import ProjectStatus +from krater.weave import StubWeaveClient, WeaveUnavailableError +from tests.conftest import ADMIN_SUB, MEMBER_SUB, REVIEWER_SUB, get_csrf_token + +# Fixture users from krater/weave/stub_users.json: both under 18, one with a guardian Weave has verified. +MINOR_SUB = "PWLMINORONE" +VERIFIED_MINOR_SUB = "PWLMINORVERIFIED" + + +def _post_decision(client: TestClient, project, path: str, **data: str): + csrf = get_csrf_token(client.get(f"/projects/{project.id}").text) + return client.post(f"/projects/{project.id}/{path}", data={"csrf_token": csrf, **data}, follow_redirects=False) + + +def test_a_reviewer_approval_is_refused_and_names_the_member( + client: TestClient, login_as, submitted_project, db_session: Session +) -> None: + project = submitted_project(login_as(MINOR_SUB)) + + login_as(REVIEWER_SUB) + response = _post_decision(client, project, "review", decision="approve") + + assert response.status_code == 303 + assert response.headers["location"] == f"/projects/{project.id}" + db_session.refresh(project) + assert project.status is ProjectStatus.PENDING_REVIEW + followed = client.get(response.headers["location"]) + assert "verified guardian in Weave" in followed.text + assert "Milo Minor (PWLMINORONE)" in followed.text + + +def test_an_admin_approval_cannot_bypass_the_gate( + client: TestClient, login_as, submitted_project, db_session: Session +) -> None: + project = submitted_project(login_as(MINOR_SUB)) + + login_as(ADMIN_SUB) + response = _post_decision(client, project, "admin-decide", decision="approve", reason="Urgent.") + + assert response.status_code == 303 + db_session.refresh(project) + assert project.status is ProjectStatus.PENDING_REVIEW + + +def test_a_minor_with_a_verified_guardian_can_be_approved( + client: TestClient, login_as, submitted_project, db_session: Session +) -> None: + project = submitted_project(login_as(VERIFIED_MINOR_SUB)) + + login_as(REVIEWER_SUB) + response = _post_decision(client, project, "review", decision="approve") + + assert response.status_code == 303 + db_session.refresh(project) + assert project.status is ProjectStatus.APPROVED + + +def test_the_project_page_shows_who_needs_a_verified_guardian(client: TestClient, login_as, submitted_project) -> None: + project = submitted_project(login_as(MINOR_SUB)) + + login_as(REVIEWER_SUB) + page = client.get(f"/projects/{project.id}") + + assert page.status_code == 200 + assert 'class="guardian-banner"' in page.text + assert "Milo Minor (PWLMINORONE)" in page.text + assert "Weave admin page" in page.text + + +def test_the_project_page_has_no_notice_without_a_blocked_member( + client: TestClient, login_as, submitted_project +) -> None: + project = submitted_project(login_as(VERIFIED_MINOR_SUB)) + + login_as(REVIEWER_SUB) + page = client.get(f"/projects/{project.id}") + + assert page.status_code == 200 + assert "guardian-banner" not in page.text + + +def test_the_project_page_says_when_weave_could_not_be_checked( + client: TestClient, login_as, submitted_project, weave_stub: StubWeaveClient, monkeypatch +) -> None: + member = login_as(MEMBER_SUB) + project = submitted_project(member) + real_get_user = weave_stub.get_user + + def get_user(sub: str, *, fresh: bool = False): + # The viewer's own role check still works; only the team lookup fails. + if sub == member.weave_sub: + raise WeaveUnavailableError("down") + return real_get_user(sub, fresh=fresh) + + login_as(REVIEWER_SUB) + monkeypatch.setattr(weave_stub, "get_user", get_user) + page = client.get(f"/projects/{project.id}") + + assert page.status_code == 200 + assert "couldn" in page.text + assert "check whether a team member needs a verified guardian" in page.text + + +def test_a_member_weave_does_not_return_is_listed_and_blocks_the_approval( + client: TestClient, login_as, submitted_project, weave_stub: StubWeaveClient, db_session: Session +) -> None: + project = submitted_project(login_as(MEMBER_SUB)) + weave_stub.remove_user(MEMBER_SUB) + + login_as(REVIEWER_SUB) + page = client.get(f"/projects/{project.id}") + response = _post_decision(client, project, "review", decision="approve") + + assert "PWLMEMBERONE: not available in Weave (no access to Krater)" in page.text + assert response.status_code == 303 + db_session.refresh(project) + assert project.status is ProjectStatus.PENDING_REVIEW From 25b786101b1e9add9b33044c8b4f5e1776c9cdc3 Mon Sep 17 00:00:00 2001 From: Jasper Mayone Date: Mon, 12 Oct 2026 13:10:00 -0400 Subject: [PATCH 4/4] docs: describe the guardian gate --- CLAUDE.md | 3 ++ docs/SPEC.md | 55 +++++++++++++++++++++++++++++++-- docs/weave-integration.md | 65 ++++++++++++++++++++++++++++++++++----- 3 files changed, 114 insertions(+), 9 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index ca89aa6..5325915 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -69,6 +69,9 @@ tests/ mirrors krater/ layout of its own app (scope `directory`). Its token for Quilt (scope `quilt`) comes from `WeaveClient.quilt_token`. - **Events for Quilt go through the outbox.** A service that changes a project's status, title, ledger or spend calls `krater.services.quilt_events.sync_project` in the same transaction. Never post to Quilt from a request. +- **Every budget grant passes the guardian gate** (`krater.services.guardian.require_team_cleared`): no grant while a + team member is `minor` in Weave without `guardian_verified`, or the directory doesn't return them. Weave owns that + check; Krater stores no copy, and no admin override bypasses it (`docs/SPEC.md` "Guardian verification"). - **Every admin override writes an `AuditEvent`** (who, what, reason). Budget changes write a `BudgetEntry` (append-only; never update or delete ledger rows). - **Stub mode** (`KRATER_WEAVE_MODE=stub`) replaces Weave with a local fixture of fake users for dev and tests. The diff --git a/docs/SPEC.md b/docs/SPEC.md index 01bcc2f..1e60d35 100644 --- a/docs/SPEC.md +++ b/docs/SPEC.md @@ -163,8 +163,9 @@ Postgres. All money is stored as **integer cents** (`*_cents`). Names are sugges - rejects self-review (submitter or credited builder); - records the Review against the current revision; - asks `ApprovalPolicyService` whether the policy is now satisfied. -4. **Approve.** When the policy is satisfied, the revision is approved, a `BudgetEntry(initial_approval)` is written, and the - project moves to `approved`. +4. **Approve.** When the policy is satisfied, Krater runs the guardian gate (see "Guardian verification"). If the gate + passes, the revision is approved, a `BudgetEntry(initial_approval)` is written, and the project moves to + `approved`. If it fails, nothing is recorded and the reviewer sees which members need a verified guardian. 5. **Reject.** A reason is required. The project moves to `changes_requested`, and the submitter edits and resubmits (step 2, same project, new revision). 6. **Amend.** An `approved` project can have its scope or budget amended. This creates an `amendment` revision reviewed @@ -237,6 +238,53 @@ Uploads use presigned URLs, and only objects from approved completion revisions spend, through Quilt's patch API. The events go through an outbox table in the same transaction as the change. See [quilt-integration.md](quilt-integration.md). +## Guardian verification + +Added Oct 12, 2026, as a safeguard for Project Ganymede. Weave asks a parent or guardian to consent by email for a +member under 18. An email link proves control of an inbox, not parenthood. So a Weave admin also records a +stronger check in Weave: a signed guardian form plus a short call or video check with the guardian. The check +lives in Weave, so every Patchwork Labs app can use it. Krater stores none of it. + +**The rule.** Krater grants no budget to a project while a team member is a minor without a verified guardian, +or while Weave's directory doesn't return a team member. + +- **The team** is the submitter plus the credited builders on the revision that is decided. +- **The data** comes from Weave's directory record: `minor` (true when the member is under 18) and + `guardian_verified` (true when a Weave admin recorded the form and the call). A member blocks the grant when + `minor` is true and `guardian_verified` is not true. +- **No directory record.** Weave's directory answers 404 for anyone the Krater app may not serve. That includes a + minor whose guardian consent is missing or was revoked. So a 404 blocks the grant, and the member is listed as + "not available in Weave (no access to Krater)". +- **When.** Krater asks Weave's directory fresh at decision time, the same way it checks roles. It checks on + every grant: a proposal approval, an amendment approval that raises the ceiling, and an admin budget increase. + A rejection, a completion approval, a cut and a reclaim grant nothing, so they have no check. +- **One rule for every path.** The gate is in `krater.services.projects` (through `krater.services.guardian`). The + web Approve button, the Slack Approve button and the admin overrides all reach it. +- **No override.** This is a safety rule. An admin override can't bypass it, and Krater has no setting to turn it + off. A Weave admin verifies the guardian first, then a reviewer or admin approves again. +- **Unknown age.** Older Weave versions send a record without `minor`. Weave ships the fields before Krater needs + them, so Krater treats that record as "not known to be a minor". It logs a warning with the member's Patchwork + ID, so an operator can see when Weave is behind. +- **Weave down.** If Weave can't be reached, the grant fails with "try again". Krater never guesses. +- **The project page** shows a notice to everyone who can see the project. The notice lists the members who need + to be cleared in Weave and the reason for each, and says that a Weave admin records a verified guardian on the + member's Weave admin page. + +```mermaid +flowchart TD + A[Approval, amendment increase or admin budget increase] --> B[Ask Weave's directory, fresh, for each team member] + B -->|Weave can't be reached| X[Refuse: try again] + B -->|404, no record| N[Refuse the grant and list the member: not available in Weave] + B --> C{minor: true?} + C -->|missing| L[Log a warning, not known to be a minor] --> OK + C -->|false| OK[Member is clear] + C -->|true| D{guardian_verified: true?} + D -->|yes| OK + D -->|no or missing| R[Refuse the grant and list the member] + OK --> E{Every member clear?} + E -->|yes| G[Grant: write the BudgetEntry] +``` + ## Admin overrides At any point an admin can: @@ -250,6 +298,9 @@ can't decide it, add to its budget or raise its hourly cap, so an admin can neve else signing off. Another admin, or the normal review, has to do it. Cutting or reclaiming its budget and withdrawing it are still allowed. +An override also can't bypass the guardian gate (see "Guardian verification"). An admin approval or budget increase +fails the same way a reviewer approval does while a team member needs a verified guardian. + Every override writes an `AuditEvent` with who, what, when and a required reason. Budget changes also write a `BudgetEntry`. Overrides are posted in the project's channel so reviewers can see them. diff --git a/docs/weave-integration.md b/docs/weave-integration.md index 978d431..3444b6e 100644 --- a/docs/weave-integration.md +++ b/docs/weave-integration.md @@ -15,6 +15,9 @@ Krater depends on this Weave work. All of it is merged on Weave `main`: - app-defined roles: [patchworklabsorg/weave#165](https://github.com/patchworklabsorg/weave/pull/165); - the directory API: [patchworklabsorg/weave#166](https://github.com/patchworklabsorg/weave/pull/166). +The guardian gate also reads the directory fields `minor` and `guardian_verified`. Weave adds them now. Krater works +with a Weave that doesn't send them yet (see "Guardian verification" below). + A Weave superadmin must create the roles `member`, `reviewer` and `admin` on the Krater app page. An admin must add `directory` to the Krater app's scopes. | Capability | Used for | @@ -27,7 +30,7 @@ A Weave superadmin must create the roles `member`, `reviewer` and `admin` on the | Claim `groups` (scope `groups`) | Group slugs linked to the Krater app; the fallback when `roles` is absent | | Claims `slack_id`, `slack_member` (scope `slack`) | Mapping Slack clicks to users, and the Slack membership gate | | The client_credentials grant, scope `directory` | Krater's own token for the directory API | -| `GET /api/v1/directory/users/{sub}` | The fresh role check before every action, and names and emails when a page needs them | +| `GET /api/v1/directory/users/{sub}` | The fresh role check before every action, the guardian gate before every budget grant, and names and emails when a page needs them | | `GET /api/v1/directory/users?role=` | "Everyone with role X", e.g. reviewers to invite to Slack channels, or finding a member by email | Krater asks for the sign-in scopes `openid profile email groups roles slack`. @@ -98,8 +101,10 @@ Krater calls the directory with an access token of its own OAuth app: Endpoints: - `GET /api/v1/directory/users/{sub}` answers - `{sub, name, email, email_verified, slack_id, slack_member, groups, roles, active}`. A 404 means the user is unknown - or may not use Krater: Krater treats them as not a member. Only `active: true` counts as active. + `{sub, name, email, email_verified, slack_id, slack_member, groups, roles, active, minor, guardian_verified}`. A 404 + means the user is unknown or may not use Krater: Krater treats them as not a member. Only `active: true` counts as + active. `minor` and `guardian_verified` are booleans that older Weave versions leave out. Krater reads a value + that isn't a boolean as missing. - `GET /api/v1/directory/users?role=` (or `?group=`, which Krater doesn't use) answers `{"users": [...]}`, sorted by `sub`. Send exactly one parameter. A role or group that isn't linked to Krater answers 404: Krater treats it as an empty list and logs a warning. @@ -122,7 +127,7 @@ class WeaveClient(Protocol): `WeaveIdentity` (from the id_token) and `WeaveUser` (from the directory) carry `sub`, `name`, `email`, `email_verified`, `slack_id`, `slack_member` (`None` when Weave didn't say) and `roles` (Krater names). `WeaveUser` also -carries `active`. `exchange_code` verifies the id_token against Weave's JWKS (signature, `iss`, `aud`, `exp`, `nonce`). +carries `active`, `minor` and `guardian_verified` (each `None` when Weave didn't say). `exchange_code` verifies the id_token against Weave's JWKS (signature, `iss`, `aud`, `exp`, `nonce`). ### Sign-in (`krater.services.users.sign_in`) @@ -148,6 +153,49 @@ navigation only. The gate prefers Weave's `slack_member` from the fresh directory record. When Weave doesn't say, Krater asks Slack (see [SPEC.md](SPEC.md#roles--authentication)). +### Guardian verification + +Weave owns the guardian check for members under 18. A Weave admin records a signed guardian form plus a call or +video check on the member's Weave admin page. Weave's directory then sends `guardian_verified: true`. Krater stores +no copy. + +Before every budget grant, `krater.services.guardian.require_team_cleared` asks the directory by `sub`, with +`fresh=True`, for each team member (the submitter and the credited builders): + +| Directory record | Krater's answer | +| --- | --- | +| `minor: true`, `guardian_verified: true` | Clear | +| `minor: true`, `guardian_verified` false or missing | Blocks the grant, and the member is named in the error | +| `minor: false` | Clear | +| No record (404) | Blocks the grant, and the member is named as "not available in Weave (no access to Krater)" | +| A record without `minor` | Not known to be a minor: clear, with a warning in the log | +| Weave can't be reached | The grant fails ("try again") | + +A 404 blocks because Weave's directory hides anyone the Krater app may not serve, a minor whose guardian consent +is missing or was revoked included. Krater treats a record without `minor` as "not known to be a minor" because +Weave ships the field first. When every +deployment's Weave sends it, the warning in the log should stop. If it doesn't, check the Weave version. + +```mermaid +sequenceDiagram + participant R as Reviewer (web or Slack) + participant K as Krater (record_review) + participant W as Weave directory + R->>K: Approve + K->>W: GET /api/v1/directory/users/{sub} (fresh), for each team member + W-->>K: {minor, guardian_verified, ...} + alt a minor without guardian_verified: true, or a 404 + K-->>R: Refused: these members must be cleared in Weave + else Weave can't be reached + K-->>R: Refused: try again + else every member clear + K-->>R: Approved, budget granted + end +``` + +The project page uses the same lookup (from the short cache) to show which members block a grant, and why. If +Weave can't answer, the page says so and still loads. + ### Disabling someone Do it in Weave: remove their Krater roles or their access to the Krater app, and revoke their tokens. The next action @@ -158,8 +206,11 @@ can keep showing what they could see for up to 60 seconds. `KRATER_WEAVE_MODE=stub` (refused in production) replaces Weave with `krater/weave/stub_users.json`. Each fixture user has the fields a real Weave sends: `sub`, `name`, `email`, `email_verified`, `slack_id`, optional -`slack_member`, `groups` (slugs) and `roles` (keys). One user (`PWLREVIEWERTWO`) has no `roles` field, to exercise the -group fallback. The stub client answers the directory lookups from the same fixture with the same `RoleMapping`. +`slack_member`, `groups` (slugs), `roles` (keys) and optional `minor` and `guardian_verified`. One user +(`PWLREVIEWERTWO`) has no `roles` field, to exercise the group fallback. `PWLMINORONE` is a minor without a verified +guardian, `PWLMINORVERIFIED` is a minor with one, and `PWLMEMBERTWO` has no `minor` field. The stub client answers the +directory lookups from the same fixture with the same `RoleMapping`. -Tests change what Weave says mid-test with `StubWeaveClient.set_roles`, `set_active`, `remove_user` and `put_user`. The +Tests change what Weave says mid-test with `StubWeaveClient.set_roles`, `set_active`, `set_minor`, +`set_guardian_verified`, `remove_user` and `put_user`. The `weave_stub` fixture in `tests/conftest.py` is the stub the test app uses.