diff --git a/AGENTS.md b/AGENTS.md index d332b8f8..9da7144b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -39,13 +39,15 @@ to action scope. ## Architecture: Concepts That Span Multiple Modules -UIs and the Telegram bot are thin clients over an API server composed of deep modules. Two architectural -rules are load-bearing and easy to violate if you only read one module: +UIs are thin clients over an API server composed of deep modules; Telegram is an outbound notification +channel only. Two architectural rules are load-bearing and easy to violate if you only read one module: -1. **The Request Workflow Module is the single source of truth for state transitions.** Telegram callbacks, - the web admin panel, and the guest-admin app must all route through the *same* workflow service — never - mutate `HardwareRequest.status` directly. The Telegram module in particular must call the workflow - module, not the database. This is what keeps web and bot behavior consistent and audited. +1. **The Request Workflow Module is the single source of truth for state transitions.** The web admin + panel, the guest-admin app and the `/control/` review page must all route through the *same* workflow + service — never mutate `HardwareRequest.status` directly. **Chat is not a decision surface**: the + Telegram accept/reject buttons and their callback route were removed, so there is no bot path into the + state machine to keep consistent any more. Re-introducing one means going through the workflow module, + never the database. 2. **The Inventory Availability Module owns all quantity math.** Reserve / issue / return / mark-lost all flow through it. No other module computes available/reserved/issued counts. The invariant "availability @@ -60,9 +62,10 @@ rules are load-bearing and easy to violate if you only read one module: Manager are both retired** — handover is a custom role, and `print_manager` survives only as the frozen legacy fallback in `_MEMBERSHIP_ROLE_ACTIONS` (migrations and enum archaeology under **Handover roles** in `docs/INVARIANTS.md`). Inventory Manager is membership-only and covers the full hardware lifecycle but - not printing, staff, or makerspace settings. Also verifies Telegram actors and blocks - restricted/suspended users. Interface: `can(actor, action, resource)`, - `scope_by_makerspace(actor, query)`, `assertTelegramActorCan(...)`. + not printing, staff, or makerspace settings. Also blocks restricted/suspended users. Interface: + `can(actor, action, resource)`, `scope_by_makerspace(actor, query)`. (It no longer verifies Telegram + actors — that went with the callback route. `assertTelegramActorCan` never existed in the code at all; + this line asserted it for months.) - **Request Workflow** — owns the state machine, emits audit logs, triggers Telegram alerts, coordinates inventory reservation/issue/return. - **Inventory Availability** — quantity math + asset status for QR-tracked tools. @@ -72,8 +75,9 @@ rules are load-bearing and easy to violate if you only read one module: - **Check-In API Client** — **RETIRED** (`73a480c`, Part M7). `apps/checkin/` no longer exists and there is no `CHECKIN_MODE` setting. Requester identity now comes from authenticated member accounts, so there is no external verify dependency left to fail safe on. -- **Telegram Integration** — sends per-makerspace group alerts and processes accept/reject callbacks - (delegating to Request Workflow). +- **Telegram Integration** — sends per-makerspace group alerts. **Outbound only.** The webhook route is + retained but accept-and-ignores every callback, because a deployment that already ran `setWebhook` would + otherwise have Telegram retry a 404 for hours; no chat message may carry an inline keyboard. ## Request State Machine @@ -198,6 +202,19 @@ starting a build.** These are the rules you must not violate without having read `./scripts/dev-local.sh test` with `spaceworks-db` (:5433), `spaceworks-redis` (:6379) and `spaceworks-minio` (:9200) up. **Never run two `pytest` procs against one DB**, and never run the full suite concurrently with `codex review` (it runs its own). +- **`tests/backup` and `tests/tenant_migration` need a pg client whose MAJOR equals the server's (16), + and the host may not have one.** `postgres_client.client_binary` resolves + `/usr/lib/postgresql/{major}/bin` (Debian/PGDG) or `/usr/pgsql-{major}/bin` (RHEL) and otherwise + fails closed — so on Arch, where neither path exists and `/usr/bin/pg_dump` is whatever `pacman` + ships, every one of those tests refuses with `PostgresClientUnavailable`. That is the environment, + **not a regression**. Run them in Docker instead, and note the DATABASE_URL override: the backend + container runs as the least-privilege `spaceworks_app` role, which has no CREATEDB, so pytest cannot + build a test database as itself. + + ```bash + ./scripts/dev-docker.sh exec -e DATABASE_URL=postgres://makerspace:makerspace@db:5432/makerspace_manager \ + -T backend pytest tests/backup tests/tenant_migration -q + ``` - **Chain every new migration off the ACTUAL leaf** — `ls backend/apps//migrations/`, never the number a spec quotes. - **Commits sit local and unpushed on `dev`; pushing is the owner's call alone.** Ask @@ -212,8 +229,11 @@ starting a build.** These are the rules you must not violate without having read ```bash ./scripts/dev-docker.sh up -d --build # default: all in Docker, live reload -./scripts/dev-docker.sh exec backend pytest -./scripts/dev-local.sh infra && ./scripts/dev-local.sh test # host fallback: faster pytest +./scripts/dev-local.sh infra && ./scripts/dev-local.sh test # host: faster pytest, most of the suite + +# In Docker, pytest needs the DB OWNER: the backend runs as `spaceworks_app`, which has no CREATEDB. +./scripts/dev-docker.sh exec -e DATABASE_URL=postgres://makerspace:makerspace@db:5432/makerspace_manager \ + -T backend pytest ``` Public inventory page: `http://localhost:5000/m/makerspace`. API: `http://localhost:8000/api` — Swagger UI @@ -243,7 +263,8 @@ Stack (in use): - **API documentation:** drf-spectacular / OpenAPI (snapshot `frontend/openapi-schema.json` + generated `frontend/src/generated/api.ts`; regenerate both when routes/models change — spectacular needs `--format openapi-json`). -- **Telegram:** request alerts, test alerts, authenticated webhook accept/reject callbacks. +- **Telegram:** request alerts and test alerts. Outbound only — the webhook acknowledges and discards + callbacks; decisions are made in the staff console or `/control/`. ## Current source map — in `docs/SOURCE-MAP.md` diff --git a/CLAUDE.md b/CLAUDE.md index d332b8f8..9da7144b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -39,13 +39,15 @@ to action scope. ## Architecture: Concepts That Span Multiple Modules -UIs and the Telegram bot are thin clients over an API server composed of deep modules. Two architectural -rules are load-bearing and easy to violate if you only read one module: +UIs are thin clients over an API server composed of deep modules; Telegram is an outbound notification +channel only. Two architectural rules are load-bearing and easy to violate if you only read one module: -1. **The Request Workflow Module is the single source of truth for state transitions.** Telegram callbacks, - the web admin panel, and the guest-admin app must all route through the *same* workflow service — never - mutate `HardwareRequest.status` directly. The Telegram module in particular must call the workflow - module, not the database. This is what keeps web and bot behavior consistent and audited. +1. **The Request Workflow Module is the single source of truth for state transitions.** The web admin + panel, the guest-admin app and the `/control/` review page must all route through the *same* workflow + service — never mutate `HardwareRequest.status` directly. **Chat is not a decision surface**: the + Telegram accept/reject buttons and their callback route were removed, so there is no bot path into the + state machine to keep consistent any more. Re-introducing one means going through the workflow module, + never the database. 2. **The Inventory Availability Module owns all quantity math.** Reserve / issue / return / mark-lost all flow through it. No other module computes available/reserved/issued counts. The invariant "availability @@ -60,9 +62,10 @@ rules are load-bearing and easy to violate if you only read one module: Manager are both retired** — handover is a custom role, and `print_manager` survives only as the frozen legacy fallback in `_MEMBERSHIP_ROLE_ACTIONS` (migrations and enum archaeology under **Handover roles** in `docs/INVARIANTS.md`). Inventory Manager is membership-only and covers the full hardware lifecycle but - not printing, staff, or makerspace settings. Also verifies Telegram actors and blocks - restricted/suspended users. Interface: `can(actor, action, resource)`, - `scope_by_makerspace(actor, query)`, `assertTelegramActorCan(...)`. + not printing, staff, or makerspace settings. Also blocks restricted/suspended users. Interface: + `can(actor, action, resource)`, `scope_by_makerspace(actor, query)`. (It no longer verifies Telegram + actors — that went with the callback route. `assertTelegramActorCan` never existed in the code at all; + this line asserted it for months.) - **Request Workflow** — owns the state machine, emits audit logs, triggers Telegram alerts, coordinates inventory reservation/issue/return. - **Inventory Availability** — quantity math + asset status for QR-tracked tools. @@ -72,8 +75,9 @@ rules are load-bearing and easy to violate if you only read one module: - **Check-In API Client** — **RETIRED** (`73a480c`, Part M7). `apps/checkin/` no longer exists and there is no `CHECKIN_MODE` setting. Requester identity now comes from authenticated member accounts, so there is no external verify dependency left to fail safe on. -- **Telegram Integration** — sends per-makerspace group alerts and processes accept/reject callbacks - (delegating to Request Workflow). +- **Telegram Integration** — sends per-makerspace group alerts. **Outbound only.** The webhook route is + retained but accept-and-ignores every callback, because a deployment that already ran `setWebhook` would + otherwise have Telegram retry a 404 for hours; no chat message may carry an inline keyboard. ## Request State Machine @@ -198,6 +202,19 @@ starting a build.** These are the rules you must not violate without having read `./scripts/dev-local.sh test` with `spaceworks-db` (:5433), `spaceworks-redis` (:6379) and `spaceworks-minio` (:9200) up. **Never run two `pytest` procs against one DB**, and never run the full suite concurrently with `codex review` (it runs its own). +- **`tests/backup` and `tests/tenant_migration` need a pg client whose MAJOR equals the server's (16), + and the host may not have one.** `postgres_client.client_binary` resolves + `/usr/lib/postgresql/{major}/bin` (Debian/PGDG) or `/usr/pgsql-{major}/bin` (RHEL) and otherwise + fails closed — so on Arch, where neither path exists and `/usr/bin/pg_dump` is whatever `pacman` + ships, every one of those tests refuses with `PostgresClientUnavailable`. That is the environment, + **not a regression**. Run them in Docker instead, and note the DATABASE_URL override: the backend + container runs as the least-privilege `spaceworks_app` role, which has no CREATEDB, so pytest cannot + build a test database as itself. + + ```bash + ./scripts/dev-docker.sh exec -e DATABASE_URL=postgres://makerspace:makerspace@db:5432/makerspace_manager \ + -T backend pytest tests/backup tests/tenant_migration -q + ``` - **Chain every new migration off the ACTUAL leaf** — `ls backend/apps//migrations/`, never the number a spec quotes. - **Commits sit local and unpushed on `dev`; pushing is the owner's call alone.** Ask @@ -212,8 +229,11 @@ starting a build.** These are the rules you must not violate without having read ```bash ./scripts/dev-docker.sh up -d --build # default: all in Docker, live reload -./scripts/dev-docker.sh exec backend pytest -./scripts/dev-local.sh infra && ./scripts/dev-local.sh test # host fallback: faster pytest +./scripts/dev-local.sh infra && ./scripts/dev-local.sh test # host: faster pytest, most of the suite + +# In Docker, pytest needs the DB OWNER: the backend runs as `spaceworks_app`, which has no CREATEDB. +./scripts/dev-docker.sh exec -e DATABASE_URL=postgres://makerspace:makerspace@db:5432/makerspace_manager \ + -T backend pytest ``` Public inventory page: `http://localhost:5000/m/makerspace`. API: `http://localhost:8000/api` — Swagger UI @@ -243,7 +263,8 @@ Stack (in use): - **API documentation:** drf-spectacular / OpenAPI (snapshot `frontend/openapi-schema.json` + generated `frontend/src/generated/api.ts`; regenerate both when routes/models change — spectacular needs `--format openapi-json`). -- **Telegram:** request alerts, test alerts, authenticated webhook accept/reject callbacks. +- **Telegram:** request alerts and test alerts. Outbound only — the webhook acknowledges and discards + callbacks; decisions are made in the staff console or `/control/`. ## Current source map — in `docs/SOURCE-MAP.md` diff --git a/README.md b/README.md index a62d493d..22385163 100644 --- a/README.md +++ b/README.md @@ -243,7 +243,7 @@ cannot be removed. **Default** means it is on when you install without choosing | | [`member_accounts`](docs/MODULES.md#member_accounts) | | | Member sign-up and member sign-in | | **Notifications** | [`notifications`](docs/MODULES.md#notifications) | | | The in-app inbox | | | [`email`](docs/MODULES.md#email) | | | Outbound email | -| | [`telegram`](docs/MODULES.md#telegram) | | | Telegram alerts and accept/reject buttons | +| | [`telegram`](docs/MODULES.md#telegram) | | | Telegram group alerts (outbound only) | | | [`slack`](docs/MODULES.md#slack) | | | Slack alerts | | | [`mattermost`](docs/MODULES.md#mattermost) | | | Mattermost alerts | | | [`discord`](docs/MODULES.md#discord) | | | Discord alerts | @@ -321,6 +321,26 @@ People still get named, two ways: It is enough to issue them a tool, register them for an event or run a machine job for them, and it keeps every handover attributable to a real person, which the hardware rules require. +#### Who may ask to borrow something + +Borrow requests are only ever a *proposal* — staff still accept them, and staff acceptance is what +reserves stock — so who may submit one is a separate switch from who may sign in. There are three +states, and you never set them both: + +| Member accounts | Account-less requests | Who may submit | +| --- | --- | --- | +| on | off | **Members** — an active member of that makerspace | +| off | off | **Account holders** — anyone signed in (the default without the module) | +| off | **on** | **Anyone** — no account at all | + +The last row is opt-in per makerspace, and turning **Member accounts** on turns it back off: enabling +membership means asking for membership, so a stranger must not still walk past it. Account-less +submissions ask for a name, email and phone, require an `Idempotency-Key`, and are rate-limited per IP +and per email address; every one of them is recorded against a single shared requester principal, which +is deliberately excluded from every per-person ranking so a hundred strangers never add up to one +fictional "top borrower". `setup.sh` asks this question during first-run setup, and it can be changed +later with `manage.py set_request_access`. + ### Choosing which ways in you offer **`/control/` → Platform login methods** switches the four credential kinds independently: password, @@ -354,8 +374,8 @@ Two things worth knowing before you set this up: - **Webhook URLs are write-only.** They are stored encrypted and never shown again, so an edit that only renames a room can leave the field blank. - **Telegram rooms share the makerspace's bot** — add the same bot to each group and paste each - group's chat ID. That is what keeps the accept/reject buttons working, since Telegram sends every - button press back to one address. + group's chat ID. Delivery is outbound only: chat is not a decision surface, so no alert carries + accept/reject buttons and decisions are made in the staff console or `/control/`. A makerspace that has added no rooms keeps using the single webhook under **Chat webhooks**, exactly as before. Nothing changes until you add your first room. diff --git a/VERSION b/VERSION index 8bd6ba8c..a3df0a69 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.7.5 +0.8.0 diff --git a/backend/.env.example b/backend/.env.example index eadcf0b5..456c995f 100644 --- a/backend/.env.example +++ b/backend/.env.example @@ -107,6 +107,12 @@ PUBLIC_IMAGE_URL_TTL_SECONDS=300 # Public request workflow throttle scopes. THROTTLE_REQUEST_SUBMIT=10/min THROTTLE_REQUEST_STATUS=60/min +THROTTLE_PUBLIC_REQUEST_SUBMIT=10/min +THROTTLE_ANONYMOUS_REQUEST_IP_BURST=2/min +THROTTLE_ANONYMOUS_REQUEST_IP_HOUR=10/hour +THROTTLE_ANONYMOUS_REQUEST_EMAIL=3/day +ANONYMOUS_REQUEST_OUTSTANDING_LIMIT=50 +ANONYMOUS_REQUEST_IDEMPOTENCY_KEY_MAX_LENGTH=128 # Email defaults use the console backend for local development. EMAIL_BACKEND=django.core.mail.backends.console.EmailBackend diff --git a/backend/apps/accounts/admin.py b/backend/apps/accounts/admin.py index 2313be8e..88fd478f 100644 --- a/backend/apps/accounts/admin.py +++ b/backend/apps/accounts/admin.py @@ -16,6 +16,10 @@ from rest_framework.exceptions import APIException from apps.accounts.models import NativeAppRegistration, User +from apps.accounts.principal_guards import ( + refuse_anonymous_requester_access_mutation, + refuse_anonymous_requester_credential, +) from apps.accounts.transition_services import ( WalkInTransitionError, transition_walk_in_to_account, @@ -78,6 +82,17 @@ class UserAdmin(SuperuserOnlyModelAdmin, DjangoUserAdmin, ModelAdmin): def user_change_password(self, request, id, form_url=""): """Make the inherited password form an explicit account transition.""" user = self.get_object(request, id) + if request.method == "POST" and user is not None: + try: + refuse_anonymous_requester_credential(user) + except APIException as exc: + self.message_user(request, _api_exception_message(exc), level=messages.ERROR) + return HttpResponseRedirect( + reverse( + f"{self.admin_site.name}:{user._meta.app_label}_{user._meta.model_name}_change", + args=(user.pk,), + ) + ) if ( request.method != "POST" or user is None @@ -171,16 +186,28 @@ def restrict_access(self, request, queryset): status = form.cleaned_data["status"] reason = form.cleaned_data["reason"] for user in queryset: - user.access_status = status - user.restriction_reason = reason - user.save(update_fields=["access_status", "restriction_reason"]) - audit.record( - request.user, - "user.access_restricted", - target=user, - meta={"status": user.access_status, "reason": user.restriction_reason}, - ) - success_count += 1 + try: + refuse_anonymous_requester_access_mutation(user) + except APIException as exc: + self.message_user( + request, + f"{user.username}: {_api_exception_message(exc)}", + level=messages.ERROR, + ) + else: + user.access_status = status + user.restriction_reason = reason + user.save(update_fields=["access_status", "restriction_reason"]) + audit.record( + request.user, + "user.access_restricted", + target=user, + meta={ + "status": user.access_status, + "reason": user.restriction_reason, + }, + ) + success_count += 1 self.message_user( request, @@ -196,6 +223,7 @@ def restore_access(self, request, queryset): try: with transaction.atomic(): locked = User.objects.select_for_update().get(pk=user.pk) + refuse_anonymous_requester_access_mutation(locked) if locked.access_status != User.AccessStatus.ACTIVE and locked.is_active: memberships = MakerspaceMembership.objects.select_related( "makerspace" diff --git a/backend/apps/accounts/claim_pre_auth_guard.py b/backend/apps/accounts/claim_pre_auth_guard.py index 9fb6ad52..a5482dad 100644 --- a/backend/apps/accounts/claim_pre_auth_guard.py +++ b/backend/apps/accounts/claim_pre_auth_guard.py @@ -47,6 +47,12 @@ "apps.events.throttles.CollaborativeRegistrationThrottle": { "_tier", "allow_request", "get_cache_key" }, + # The account-less borrow-request budgets. Both override `get_cache_key` only, to + # return None for an authenticated caller: they sit in `throttle_classes` beside the + # member throttle, and DRF applies every class on every request, so without the skip a + # makerspace behind one NAT would rate-limit its own signed-in members by egress IP. + "apps.hardware_requests.throttles.AnonymousRequestIpBurstThrottle": {"get_cache_key"}, + "apps.hardware_requests.throttles.AnonymousRequestIpHourThrottle": {"get_cache_key"}, "apps.machines.permissions.IsActiveRequester": {"has_permission"}, "apps.accounts.views_device.IsDeviceAccessToken": {"has_permission"}, "apps.makerspaces.throttles.MemberImagePresignThrottle": {"get_cache_key"}, diff --git a/backend/apps/accounts/principal_guards.py b/backend/apps/accounts/principal_guards.py new file mode 100644 index 00000000..dad62255 --- /dev/null +++ b/backend/apps/accounts/principal_guards.py @@ -0,0 +1,30 @@ +"""Relational guards for inert system principals stored in ``accounts.User``.""" + +from rest_framework.exceptions import PermissionDenied + + +ANONYMOUS_REQUESTER_CREDENTIAL_ERROR = ( + "This is an anonymous-request system principal, not an account." +) +ANONYMOUS_REQUESTER_ACCESS_ERROR = ( + "An anonymous-request system principal's access status cannot be changed." +) + + +def is_anonymous_requester(user) -> bool: + """Use the database-unique relationship, never a mutable username convention.""" + if user is None or user.pk is None: + return False + from apps.makerspaces.models import Makerspace + + return Makerspace.objects.filter(anonymous_requester_id=user.pk).exists() + + +def refuse_anonymous_requester_credential(user) -> None: + if is_anonymous_requester(user): + raise PermissionDenied(ANONYMOUS_REQUESTER_CREDENTIAL_ERROR) + + +def refuse_anonymous_requester_access_mutation(user) -> None: + if is_anonymous_requester(user): + raise PermissionDenied(ANONYMOUS_REQUESTER_ACCESS_ERROR) diff --git a/backend/apps/accounts/services_password_reset.py b/backend/apps/accounts/services_password_reset.py index bd03809e..7bfe468a 100644 --- a/backend/apps/accounts/services_password_reset.py +++ b/backend/apps/accounts/services_password_reset.py @@ -11,6 +11,7 @@ from apps.accounts import audit_events from apps.accounts.models import PasswordResetEnvelope, PasswordResetEnvelopeStatus, User +from apps.accounts.principal_guards import is_anonymous_requester from apps.accounts.password_reset_crypto import ( credential_fingerprint, fixed_dummy_digest, @@ -232,6 +233,7 @@ def _credential_state_matches(user, envelope, normalized): and user.is_active and user.access_status == User.AccessStatus.ACTIVE and not user.is_walk_in + and not is_anonymous_requester(user) and current_email == normalized and hmac.compare_digest( envelope.credential_fingerprint, diff --git a/backend/apps/accounts/services_password_reset_drain.py b/backend/apps/accounts/services_password_reset_drain.py index dca4a637..2eebf6ce 100644 --- a/backend/apps/accounts/services_password_reset_drain.py +++ b/backend/apps/accounts/services_password_reset_drain.py @@ -12,6 +12,7 @@ from django.utils import timezone from apps.accounts.models import PasswordResetEnvelope, PasswordResetEnvelopeStatus, User +from apps.accounts.principal_guards import is_anonymous_requester from apps.accounts.password_reset_crypto import ( credential_fingerprint, generate_otp, @@ -234,6 +235,7 @@ def _recoverable(user): and user.is_active and user.access_status == User.AccessStatus.ACTIVE and not user.is_walk_in + and not is_anonymous_requester(user) ) diff --git a/backend/apps/accounts/services_phone.py b/backend/apps/accounts/services_phone.py index b3052cc8..310d9db1 100644 --- a/backend/apps/accounts/services_phone.py +++ b/backend/apps/accounts/services_phone.py @@ -29,6 +29,7 @@ from apps.accounts import audit_events from apps.accounts.models import User +from apps.accounts.principal_guards import is_anonymous_requester from apps.accounts.models_phone import PhoneChallengePurpose, PhoneVerificationChallenge from apps.accounts.phone_numbers import normalize_or_none from apps.integrations.sms import send_sms, sms_configured @@ -150,7 +151,8 @@ def start_link(user, raw_phone): """Send a code to a number an authenticated user wants to attach.""" if not sms_configured(): raise SmsUnavailable - if User.objects.filter(pk=user.pk, is_walk_in=True).exists(): + current = User.objects.filter(pk=user.pk).first() + if current is None or current.is_walk_in or is_anonymous_requester(current): raise serializers.ValidationError({"detail": GENERIC_CONFIRM_ERROR}) phone_e164 = normalize_or_none(raw_phone) if phone_e164 is None: @@ -207,10 +209,14 @@ def confirm_link(user, raw_phone, code): failure = {"detail": GENERIC_CONFIRM_ERROR} # This is the fifth guarded credential-writer for walk-ins. confirm_link is # the chokepoint because a verified phone is itself a login identity. - elif locked_user.is_walk_in: + elif locked_user.is_walk_in or is_anonymous_requester(locked_user): audit_events.record_auth_event( - locked_user, - "member.phone_link_refused_walk_in", + None if not locked_user.is_walk_in else locked_user, + ( + "member.phone_link_refused_walk_in" + if locked_user.is_walk_in + else "member.phone_link_refused_anonymous_requester" + ), target=locked_user, meta={}, ) diff --git a/backend/apps/accounts/services_social_identity.py b/backend/apps/accounts/services_social_identity.py index c976490d..166f5c72 100644 --- a/backend/apps/accounts/services_social_identity.py +++ b/backend/apps/accounts/services_social_identity.py @@ -5,6 +5,7 @@ from apps.accounts.models import User from apps.accounts.models_social import SocialIdentity, SocialSurface +from apps.accounts.principal_guards import is_anonymous_requester class SocialResolutionError(Exception): @@ -62,6 +63,7 @@ def resolve_social_identity( or not verified or matched.email_verified_at is None or matched.is_walk_in + or is_anonymous_requester(matched) ): raise SocialResolutionError("account_link_required", 409) user = matched @@ -107,6 +109,8 @@ def _explicit_link(identity, user, provider, subject): # social-identity link is created through this function. if user.is_walk_in: raise SocialResolutionError("walk_in_record", 403) + if is_anonymous_requester(user): + raise SocialResolutionError("anonymous_requester_record", 403) if identity is not None: if identity.user_id != user.pk: raise SocialResolutionError("identity_conflict", 409) diff --git a/backend/apps/accounts/views_password.py b/backend/apps/accounts/views_password.py index b57625f2..5d75019b 100644 --- a/backend/apps/accounts/views_password.py +++ b/backend/apps/accounts/views_password.py @@ -18,6 +18,7 @@ from rest_framework.views import APIView from apps.accounts.models import User +from apps.accounts.principal_guards import is_anonymous_requester from apps.accounts.password_reset_crypto import normalize_email from apps.accounts.serializers_password_reset import ( ChangePasswordResponseSerializer, @@ -94,7 +95,11 @@ def post(self, request, *args, **kwargs): new_password = serializer.validated_data["new_password"] user = request.user - if user.is_walk_in or not user.check_password(current_password): + if ( + user.is_walk_in + or is_anonymous_requester(user) + or not user.check_password(current_password) + ): raise serializers.ValidationError( {"current_password": "Current password is incorrect."} ) @@ -111,7 +116,11 @@ def post(self, request, *args, **kwargs): with transaction.atomic(): locked = User.objects.select_for_update().get(pk=user.pk) - if locked.is_walk_in or not locked.check_password(current_password): + if ( + locked.is_walk_in + or is_anonymous_requester(locked) + or not locked.check_password(current_password) + ): raise serializers.ValidationError( {"current_password": "Current password is incorrect."} ) @@ -238,6 +247,7 @@ def _verified_legacy_email(user, token, *, expected_email=None): and user.is_active and user.access_status == User.AccessStatus.ACTIVE and not user.is_walk_in + and not is_anonymous_requester(user) ): return None try: diff --git a/backend/apps/admin_api/services_user_access.py b/backend/apps/admin_api/services_user_access.py index 8c7df00f..376abb57 100644 --- a/backend/apps/admin_api/services_user_access.py +++ b/backend/apps/admin_api/services_user_access.py @@ -9,6 +9,7 @@ from apps.accounts import rbac from apps.accounts.models import User +from apps.accounts.principal_guards import refuse_anonymous_requester_credential from apps.admin_api.permissions import hidden_space_manager_reset_break_glass from apps.audit import services as audit from apps.makerspaces.models import MakerspaceMembership @@ -25,6 +26,7 @@ def reset_user_password(actor, target_pk, password=None, data=None): target = _target_for_reset(actor, target_pk, is_superadmin) if target.is_superuser or target.role == User.Role.SUPERADMIN: raise PermissionDenied("Cannot reset a superadmin's password here.") + refuse_anonymous_requester_credential(target) # A walk-in is a person record staff typed at the counter, not an account, and this # service HANDS BACK a usable temporary password -- so without this it is a one-click # way to turn one into a login, available to any space manager in the makerspace and @@ -55,6 +57,7 @@ def reset_user_password(actor, target_pk, password=None, data=None): # Close the stale-read interleaving where the walk-in migration marks this # user after the check above but before this password write. target = User.objects.select_for_update().get(pk=target.pk) + refuse_anonymous_requester_credential(target) if target.is_walk_in: raise PermissionDenied( "This is a walk-in record, not an account. It has no password to reset." diff --git a/backend/apps/admin_api/views_user_access.py b/backend/apps/admin_api/views_user_access.py index e0eb59ef..4b26a6cb 100644 --- a/backend/apps/admin_api/views_user_access.py +++ b/backend/apps/admin_api/views_user_access.py @@ -5,6 +5,7 @@ from rest_framework.views import APIView from apps.accounts.models import User +from apps.accounts.principal_guards import refuse_anonymous_requester_access_mutation from apps.admin_api.permissions import ( IsActiveStaff, IsActiveSuperAdmin, @@ -35,6 +36,7 @@ class RestrictUserView(APIView): def post(self, request, pk, *args, **kwargs): user = get_object_or_404(User, pk=pk) require_user_access_mutation(request.user, user) + refuse_anonymous_requester_access_mutation(user) serializer = RestrictUserSerializer(data=request.data) serializer.is_valid(raise_exception=True) user.access_status = serializer.validated_data["status"] @@ -82,6 +84,7 @@ def post(self, request, pk, *args, **kwargs): with transaction.atomic(): user = get_object_or_404(User.objects.select_for_update(), pk=pk) require_user_access_mutation(request.user, user) + refuse_anonymous_requester_access_mutation(user) if user.access_status != User.AccessStatus.ACTIVE and user.is_active: memberships = user.makerspace_memberships.select_related( "makerspace" diff --git a/backend/apps/audit/anchors_object_storage.py b/backend/apps/audit/anchors_object_storage.py index c76dc81c..59f96cf1 100644 --- a/backend/apps/audit/anchors_object_storage.py +++ b/backend/apps/audit/anchors_object_storage.py @@ -31,8 +31,14 @@ def __init__(self): self.retention_days = int( getattr(settings, "AUDIT_ATTESTATION_RETENTION_DAYS", 0) ) + # `or "COMPLIANCE"` because BLANK is not the same as absent here. Compose passes + # `AUDIT_ATTESTATION_S3_OBJECT_LOCK_MODE: ${...:-}`, which makes the variable + # present-but-empty, and django-environ only applies its default when a variable + # is ABSENT -- so the setting arrives as "" and the getattr default never fires. + # Every deployment that left the mode unset in .env therefore refused to anchor. + # COMPLIANCE is the stricter of the two modes, so defaulting to it fails safe. self.lock_mode = str( - getattr(settings, "AUDIT_ATTESTATION_S3_OBJECT_LOCK_MODE", "COMPLIANCE") + getattr(settings, "AUDIT_ATTESTATION_S3_OBJECT_LOCK_MODE", "") or "COMPLIANCE" ).upper() if not self.bucket or self.retention_days < 1: raise AnchorError( diff --git a/backend/apps/backup/settings_policy.py b/backend/apps/backup/settings_policy.py index 06f69607..143a5f66 100644 --- a/backend/apps/backup/settings_policy.py +++ b/backend/apps/backup/settings_policy.py @@ -42,6 +42,7 @@ class SettingPolicy: BEHIND_TRUSTED_PROXY CACHE_URL CELERY_BROKER_URL CELERY_RESULT_BACKEND CELERY_TASK_ALWAYS_EAGER CONN_MAX_AGE CORS_ALLOWED_ORIGINS CRON_SECRET CSRF_COOKIE_SECURE CSRF_TRUSTED_ORIGINS DATABASE_URL DATA_EXPORT_DEADLINE_SECONDS +ANONYMOUS_REQUEST_IDEMPOTENCY_KEY_MAX_LENGTH ANONYMOUS_REQUEST_OUTSTANDING_LIMIT DATA_EXPORT_DOWNLOAD_TTL_SECONDS DATA_EXPORT_PAGE_SIZE DATA_EXPORT_RETENTION_SECONDS DEBUG DEFAULT_FROM_EMAIL DEVICE_ANDROID_ATTESTATION_VERIFY_TOKEN DEVICE_ANDROID_ATTESTATION_VERIFY_URL DEVICE_APPLE_ATTESTATION_VERIFY_TOKEN @@ -66,7 +67,9 @@ class SettingPolicy: SOCIAL_AUTH_CLOCK_SKEW_SECONDS SOCIAL_AUTH_JWKS_CACHE_SECONDS SOCIAL_AUTH_JWKS_MAX_BYTES SOCIAL_AUTH_JWKS_TIMEOUT_SECONDS SOCIAL_AUTH_NONCE_TTL_SECONDS SOCIAL_GOOGLE_JWKS_URL STORAGE_PRESIGN_METHOD STRIPE_CONNECT_REDIRECT_URI TELEGRAM_API_URL TELEGRAM_BOT_TOKEN -TELEGRAM_WEBHOOK_SECRET THROTTLE_ARCHIVE_RECIPIENT_VERIFY THROTTLE_BOOKING_SUBMIT THROTTLE_CLIENT_PUBLIC +TELEGRAM_WEBHOOK_SECRET THROTTLE_ANONYMOUS_REQUEST_EMAIL +THROTTLE_ANONYMOUS_REQUEST_IP_BURST THROTTLE_ANONYMOUS_REQUEST_IP_HOUR +THROTTLE_ARCHIVE_RECIPIENT_VERIFY THROTTLE_BOOKING_SUBMIT THROTTLE_CLIENT_PUBLIC THROTTLE_CLIENT_STANDARD THROTTLE_CLIENT_TRUSTED THROTTLE_DATA_EXPORT_CREATE THROTTLE_DEVICE_ATTESTATION_CHALLENGE THROTTLE_DEVICE_LOGIN THROTTLE_DEVICE_LOGIN_USER THROTTLE_DEVICE_REFRESH THROTTLE_EMAIL_VERIFICATION_CONFIRM diff --git a/backend/apps/data_export/classification.py b/backend/apps/data_export/classification.py index ae2bb6a4..1f7691b5 100644 --- a/backend/apps/data_export/classification.py +++ b/backend/apps/data_export/classification.py @@ -21,7 +21,7 @@ "events.EventCollaborator": "id event makerspace status invited_by responded_by created_at responded_at", "events.EventRegistration": "id event checkin_token name email phone member registered_via_makerspace payment_via_makerspace host_waiver host_waiver_accepted_at host_waiver_version_accepted email_exact_hash email_hash_generation custom_answers status created_at", "evidence.EvidencePhoto": "id makerspace evidence_type object_key content_type size_bytes uploaded_by created_at", - "hardware_requests.HardwareRequest": "id makerspace requester requester_username requester_name requester_contact_email requester_contact_phone status requested_for rejection_reason accepted_by accepted_at assigned_box issued_by issued_at return_due_at return_reminder_sent_at issue_evidence issue_remark closed_by closed_at public_token created_at updated_at", + "hardware_requests.HardwareRequest": "id makerspace requester requester_username requester_name requester_contact_email requester_contact_phone requester_contact_verified anonymous_idempotency_key_fingerprint anonymous_payload_fingerprint status requested_for rejection_reason accepted_by accepted_at assigned_box issued_by issued_at return_due_at return_reminder_sent_at issue_evidence issue_remark closed_by closed_at public_token created_at updated_at", "hardware_requests.HardwareRequestItem": "id request product requested_quantity accepted_quantity issued_quantity returned_quantity damaged_quantity missing_quantity needs_fix_quantity", "hardware_requests.HardwareRequestItemAsset": "id request_item asset outcome issued_at returned_at return_event", "hardware_requests.PublicProblemReport": "id makerspace loan request requester note outcome triage_note created_at resolved_at resolved_by", @@ -64,7 +64,7 @@ "maintenance.MaintenanceLog": "id machine performed_by performed_at summary cost parts_note created_at", "maintenance.MaintenanceLogDocument": "id log object_key size_bytes uploaded_by created_at", "maintenance.MaintenanceSchedule": "id machine description interval_days next_due is_active created_by created_at updated_at", - "makerspaces.Makerspace": "id name slug public_code location map_url geofence_latitude geofence_longitude geofence_radius_m geofence_enabled public_inventory_enabled public_stats_enabled public_stats_show_holder_names public_print_status_lookup_policy membership_policy membership_dues_amount referrals_enabled filament_low_stock_threshold_grams superadmin_access_enabled staff_notifications_enabled booking_requester_notifications_enabled logo_key cover_image_key frontend_domain frontend_domain_status domain_verification_token domain_verified_at frontend_domain_changed_at hidden_from_central_directory public_api_key cors_allowed_origins enabled_modules enabled_features resource_limit_overrides storage_bytes_used theme_config branding_config telegram_group_chat_id telegram_bot_token smtp_host smtp_port smtp_username smtp_password smtp_use_tls smtp_use_ssl smtp_from_email slack_webhook_url mattermost_webhook_url discord_webhook_url default_loan_days presence_preset_minutes created_by archived_at archived_by lifecycle_state created_at updated_at", + "makerspaces.Makerspace": "id name slug public_code anonymous_requests_enabled anonymous_requester location map_url geofence_latitude geofence_longitude geofence_radius_m geofence_enabled public_inventory_enabled public_stats_enabled public_stats_show_holder_names public_print_status_lookup_policy membership_policy membership_dues_amount referrals_enabled filament_low_stock_threshold_grams superadmin_access_enabled staff_notifications_enabled booking_requester_notifications_enabled logo_key cover_image_key frontend_domain frontend_domain_status domain_verification_token domain_verified_at frontend_domain_changed_at hidden_from_central_directory public_api_key cors_allowed_origins enabled_modules enabled_features resource_limit_overrides storage_bytes_used theme_config branding_config telegram_group_chat_id telegram_bot_token smtp_host smtp_port smtp_username smtp_password smtp_use_tls smtp_use_ssl smtp_from_email slack_webhook_url mattermost_webhook_url discord_webhook_url default_loan_days presence_preset_minutes created_by archived_at archived_by lifecycle_state created_at updated_at", "makerspaces.MakerspaceMembership": "id makerspace user role assigned_role receives_notifications can_refer can_verify verified_at verified_by status activated_at activated_by revoked_at revoked_by revocation_reason waiver_accepted_at waiver_version_accepted accepted_waiver witnessed_waiver witnessed_waiver_version witnessed_at witnessed_by witnessed_actor_snapshot verified_actor_snapshot activated_actor_snapshot revoked_actor_snapshot created_at", "makerspaces.MakerspaceRole": "id makerspace name slug granted_actions legacy_role is_default is_protected created_at updated_at", "makerspaces.MakerspaceWaiver": "id makerspace body version is_active created_by created_at superseded_at", diff --git a/backend/apps/data_export/references.py b/backend/apps/data_export/references.py index e7c8dbe4..7350f7d9 100644 --- a/backend/apps/data_export/references.py +++ b/backend/apps/data_export/references.py @@ -112,6 +112,7 @@ def require_raw_user(fidelity, *, model, row_pk, field, user_id, existing_user_i ("accounts.MemberClaimCode", "issued_by"), ("accounts.MemberClaimCode", "revoked_by"), ("accounts.OidcBrowserAttempt", "intended_user"), + ("makerspaces.Makerspace", "anonymous_requester"), ("makerspaces.MakerspaceMembership", "user"), ("makerspaces.MakerspaceMembership", "verified_by"), ("makerspaces.MakerspaceMembership", "activated_by"), diff --git a/backend/apps/encryption/registry.py b/backend/apps/encryption/registry.py index d83486e2..15afdf6a 100644 --- a/backend/apps/encryption/registry.py +++ b/backend/apps/encryption/registry.py @@ -21,7 +21,7 @@ def _fields(label, names, path, classification, indexes): SOURCE_FIELDS = ( - *_fields("hardware_requests.HardwareRequest", ("requester_username", "requester_name", "requester_contact_email", "requester_contact_phone"), "makerspace_id", "source", ((150, 120, 254, 32), ("none", "bloom", "bloom_exact", "none"))), + *_fields("hardware_requests.HardwareRequest", ("requester_username", "requester_name", "requester_contact_email", "requester_contact_phone"), "makerspace_id", "source", ((150, 200, 254, 32), ("none", "bloom", "bloom_exact", "none"))), *_fields("events.EventRegistration", ("name", "email", "phone"), "event.makerspace_id", "source", ((200, 254, 32), ("none", "event_exact", "none"))), *_fields("bookings.Booking", ("name", "email", "phone", "note"), "space.makerspace_id", "source", ((200, 254, 32, None), ("none", "none", "none", "none"))), *_fields("machines.MachineServiceRequest", ("requester_name", "contact_email", "contact_phone"), "makerspace_id", "source", ((None, None, None), ("bloom", "bloom_exact", "none"))), diff --git a/backend/apps/hardware_requests/admin_request_review.py b/backend/apps/hardware_requests/admin_request_review.py new file mode 100644 index 00000000..cdd77bf3 --- /dev/null +++ b/backend/apps/hardware_requests/admin_request_review.py @@ -0,0 +1,132 @@ +"""One request, one decision — the review surface that replaced bulk accept/reject. + +Accepting a borrow request RESERVES inventory and rejecting one closes a person's ask, and +neither is a judgement you can make about twenty rows from a checkbox column. The bulk +`accept_selected` / `reject_selected` actions are gone; a decision is made here, on a page +that shows who is asking, whether their contact was ever verified, and exactly what they +want, with a per-item accepted quantity you can lower before you commit stock. + +The mutations still go through `request_workflow`, so the state machine, the audit entry +and the notification fan-out are identical to every other surface — only the *entry point* +changed. See the module-level note in `apps.hardware_requests.workflow` for why nothing may +mutate `HardwareRequest.status` directly. +""" + +from django.contrib import messages +from django.http import Http404 +from django.shortcuts import redirect +from django.template.response import TemplateResponse +from django.urls import reverse + +from apps.hardware_requests.admin_workflow import WORKFLOW_EXCEPTIONS +from apps.hardware_requests.models import HardwareRequest +from apps.hardware_requests.request_workflow import accept_request, reject_request + + +class RequestReviewAdminMixin: + """Adds `/review/` to a HardwareRequest ModelAdmin.""" + + review_template = "admin/hardware_requests/review.html" + + def get_urls(self): + from django.urls import path + + return [ + path( + "/review/", + # `admin_view` applies the admin's own authentication and the + # never-cache headers. Without it this is an ordinary view that any + # logged-in user could reach, which for a state-changing POST is the + # whole ballgame. + self.admin_site.admin_view(self.review_view), + name="hardware_requests_hardwarerequest_review", + ), + *super().get_urls(), + ] + + def _review_object(self, request, request_id): + # `self.get_queryset(request)` and NOT `HardwareRequest.objects`: the superadmin + # queryset excludes hard-hidden makerspaces, and reaching around it here would + # make this URL the one place that ignores the control-plane hidden-tenant rule. + queryset = self.get_queryset(request).prefetch_related("items__product") + obj = queryset.filter(pk=request_id).first() + if obj is None: + raise Http404("No hardware request matches the given query.") + return obj + + def review_view(self, request, request_id): + obj = self._review_object(request, request_id) + if not self.has_change_permission(request, obj): + raise Http404("No hardware request matches the given query.") + + if request.method == "POST": + return self._apply_review(request, obj) + + return TemplateResponse( + request, + self.review_template, + { + **self.admin_site.each_context(request), + "title": f"Review hardware request #{obj.pk}", + "hardware_request": obj, + "items": list(obj.items.all()), + "opts": self.model._meta, + "is_pending": obj.status == HardwareRequest.Status.PENDING_APPROVAL, + }, + ) + + def _apply_review(self, request, obj): + changelist = reverse("admin:hardware_requests_hardwarerequest_changelist") + if "reject" in request.POST: + reason = request.POST.get("reason", "").strip() + if not reason: + self.message_user(request, "Rejection reason is required.", level=messages.ERROR) + return redirect(request.path) + try: + reject_request(request.user, obj, reason) + except WORKFLOW_EXCEPTIONS as exc: + self.message_user(request, f"{obj.pk}: {exc}", level=messages.ERROR) + return redirect(request.path) + self.message_user(request, f"Rejected hardware request #{obj.pk}.", level=messages.SUCCESS) + return redirect(changelist) + + try: + accepted = self._accepted_quantities(request, obj) + except ValueError as exc: + self.message_user(request, str(exc), level=messages.ERROR) + return redirect(request.path) + + try: + accept_request(request.user, obj, accepted=accepted) + except WORKFLOW_EXCEPTIONS as exc: + self.message_user(request, f"{obj.pk}: {exc}", level=messages.ERROR) + return redirect(request.path) + self.message_user(request, f"Accepted hardware request #{obj.pk}.", level=messages.SUCCESS) + return redirect(changelist) + + @staticmethod + def _accepted_quantities(request, obj): + """Parse the per-item inputs into the map `accept_request` expects. + + Returns `None` when the form carried no quantities at all, which `accept_request` + reads as "accept everything as requested" — the same default the API uses for an + omitted `accepted_quantities`. Anything present is parsed as an int here rather + than handed to the workflow as a string, so a malformed field is an error on this + page instead of a 500 inside the service. + """ + quantities = {} + for item in obj.items.all(): + raw = request.POST.get(f"accepted_quantity_{item.pk}") + if raw is None or raw == "": + continue + try: + value = int(raw) + except (TypeError, ValueError): + raise ValueError(f"Accepted quantity for item {item.pk} must be a whole number.") + if value < 0 or value > item.requested_quantity: + raise ValueError( + f"Accepted quantity for item {item.pk} must be between 0 and " + f"{item.requested_quantity}." + ) + quantities[item.pk] = value + return quantities or None diff --git a/backend/apps/hardware_requests/admin_requests.py b/backend/apps/hardware_requests/admin_requests.py index a571f3bf..44007fd8 100644 --- a/backend/apps/hardware_requests/admin_requests.py +++ b/backend/apps/hardware_requests/admin_requests.py @@ -3,6 +3,8 @@ from django.contrib import messages from django.contrib.admin.helpers import ACTION_CHECKBOX_NAME from django.template.response import TemplateResponse +from django.urls import reverse +from django.utils.html import format_html from unfold.admin import ModelAdmin, TabularInline from apps.hardware_requests.admin_workflow import WORKFLOW_EXCEPTIONS @@ -10,8 +12,8 @@ assign_box, set_return_due as workflow_set_return_due, ) +from apps.hardware_requests.admin_request_review import RequestReviewAdminMixin from apps.hardware_requests.models import HardwareRequest, HardwareRequestItem -from apps.hardware_requests.request_workflow import accept_request, reject_request from config.admin_access import SuperuserOnlyModelAdmin from apps.encryption.admin_search import ScopedPiiAdminSearchMixin @@ -48,10 +50,14 @@ def has_add_permission(self, request, obj=None): @admin.register(HardwareRequest) -class HardwareRequestAdmin(ScopedPiiAdminSearchMixin, SuperuserOnlyModelAdmin, ModelAdmin): +class HardwareRequestAdmin( + RequestReviewAdminMixin, ScopedPiiAdminSearchMixin, SuperuserOnlyModelAdmin, ModelAdmin +): + # Accepting reserves stock and rejecting closes a person's ask, so neither is a + # checkbox-column decision any more: both moved to the one-by-one review page + # (`admin_request_review`). What survives here is genuinely bulk-shaped work on + # requests whose decision has ALREADY been made. actions = [ - "accept_selected", - "reject_selected", "assign_box_selected", "set_return_due", ] @@ -62,8 +68,11 @@ class HardwareRequestAdmin(ScopedPiiAdminSearchMixin, SuperuserOnlyModelAdmin, M "requester_identity", "return_due_at", "created_at", + "review_link", ) - list_filter = ("status", "makerspace") + # `requester_contact_verified` is filterable so the unverified submissions -- which + # are exactly the account-less ones -- can be pulled up as a group before handover. + list_filter = ("status", "makerspace", "requester_contact_verified") search_fields = ( "requested_for", "rejection_reason", @@ -73,6 +82,7 @@ class HardwareRequestAdmin(ScopedPiiAdminSearchMixin, SuperuserOnlyModelAdmin, M "makerspace", "requester", "requester_username", + "requester_contact_verified", "status", "requested_for", "rejection_reason", @@ -96,80 +106,32 @@ class HardwareRequestAdmin(ScopedPiiAdminSearchMixin, SuperuserOnlyModelAdmin, M @admin.display(description="Requester") def requester_identity(self, obj): - return obj.requester_name or obj.requester_contact_email or "-" + label = obj.requester_name or obj.requester_contact_email or "-" + if obj.requester_contact_verified: + return label + # An account-less submitter typed this address themselves and nothing has ever + # proved it is theirs -- staff acceptance does not prove it either. Marking it + # here is what stops the queue from reading like a list of known people. + return f"{label} (unverified contact)" + + @admin.display(description="Review") + def review_link(self, obj): + if obj.status != HardwareRequest.Status.PENDING_APPROVAL: + return "-" + url = reverse( + "admin:hardware_requests_hardwarerequest_review", args=[obj.pk] + ) + return format_html('Review', url) # Requests are created by the public submit flow and mutated only through the - # workflow services (the actions below). Direct add hits required readonly fields - # and direct delete bypasses reservation/audit/notification cleanup. + # workflow services. Direct add hits required readonly fields and direct delete + # bypasses reservation/audit/notification cleanup. def has_add_permission(self, request): return False def has_delete_permission(self, request, obj=None): return False - @admin.action(description="Accept selected requests") - def accept_selected(self, request, queryset): - success_count = 0 - for hardware_request in queryset: - try: - accept_request(request.user, hardware_request) - except WORKFLOW_EXCEPTIONS as exc: - self.message_user( - request, - f"{hardware_request.pk}: {exc}", - level=messages.ERROR, - ) - else: - success_count += 1 - - if success_count: - self.message_user( - request, - f"Accepted {success_count} hardware request(s).", - level=messages.SUCCESS, - ) - - @admin.action(description="Reject selected requests (with reason)") - def reject_selected(self, request, queryset): - if "apply" not in request.POST: - return self._intermediate_action_response( - request, - queryset, - "admin/hardware_requests/reject_action.html", - "Reject selected hardware requests", - "reject_selected", - ) - - reason = request.POST.get("reason", "").strip() - if not reason: - self.message_user( - request, - "Rejection reason is required.", - level=messages.ERROR, - ) - return None - - success_count = 0 - for hardware_request in queryset: - try: - reject_request(request.user, hardware_request, reason) - except WORKFLOW_EXCEPTIONS as exc: - self.message_user( - request, - f"{hardware_request.pk}: {exc}", - level=messages.ERROR, - ) - else: - success_count += 1 - - if success_count: - self.message_user( - request, - f"Rejected {success_count} hardware request(s).", - level=messages.SUCCESS, - ) - return None - @admin.action(description="Assign box to selected requests") def assign_box_selected(self, request, queryset): if "apply" not in request.POST: diff --git a/backend/apps/hardware_requests/exceptions.py b/backend/apps/hardware_requests/exceptions.py index d12eb3a3..15aff5af 100644 --- a/backend/apps/hardware_requests/exceptions.py +++ b/backend/apps/hardware_requests/exceptions.py @@ -12,6 +12,8 @@ EventInvalidTransition, ) from apps.hardware_requests.workflow import ( + AnonymousRequestIdempotencyConflict, + AnonymousRequestOutstandingLimit, BoxUnavailable, BoxValidationError, EvidenceNotUploaded, @@ -49,6 +51,17 @@ class ErrorSerializer(serializers.Serializer): _EXCEPTION_MAP = { + AnonymousRequestIdempotencyConflict: ( + status.HTTP_409_CONFLICT, + "anonymous_request_idempotency_conflict", + "This idempotency key was already used for a different request.", + ), + AnonymousRequestOutstandingLimit: ( + status.HTTP_429_TOO_MANY_REQUESTS, + "anonymous_request_outstanding_limit", + "This makerspace is not accepting more anonymous requests until pending " + "requests are reviewed.", + ), ClaimCodeError: ( status.HTTP_400_BAD_REQUEST, "invalid_claim_code", diff --git a/backend/apps/hardware_requests/migrations/0024_anonymous_request_submission.py b/backend/apps/hardware_requests/migrations/0024_anonymous_request_submission.py new file mode 100644 index 00000000..5a214746 --- /dev/null +++ b/backend/apps/hardware_requests/migrations/0024_anonymous_request_submission.py @@ -0,0 +1,42 @@ +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [("hardware_requests", "0023_scoped_pii_text_fields")] + + operations = [ + migrations.AddField( + model_name="hardwarerequest", + name="anonymous_idempotency_key_fingerprint", + field=models.CharField(blank=True, default="", max_length=64), + ), + migrations.AddField( + model_name="hardwarerequest", + name="anonymous_payload_fingerprint", + field=models.CharField(blank=True, default="", max_length=64), + ), + migrations.AddField( + model_name="hardwarerequest", + name="requester_contact_verified", + field=models.BooleanField(default=True), + ), + migrations.AddIndex( + model_name="hardwarerequest", + index=models.Index( + condition=( + models.Q(status="pending_approval") + & ~models.Q(anonymous_idempotency_key_fingerprint="") + ), + fields=["makerspace", "status"], + name="hwreq_anon_pending_idx", + ), + ), + migrations.AddConstraint( + model_name="hardwarerequest", + constraint=models.UniqueConstraint( + condition=~models.Q(anonymous_idempotency_key_fingerprint=""), + fields=("makerspace", "anonymous_idempotency_key_fingerprint"), + name="uniq_hwreq_anon_idempotency", + ), + ), + ] diff --git a/backend/apps/hardware_requests/models.py b/backend/apps/hardware_requests/models.py index eea25d27..79aae510 100644 --- a/backend/apps/hardware_requests/models.py +++ b/backend/apps/hardware_requests/models.py @@ -30,6 +30,17 @@ class Status(models.TextChoices): requester_name = models.TextField(blank=True, default="") requester_contact_email = models.TextField(blank=True) requester_contact_phone = models.TextField(blank=True) + requester_contact_verified = models.BooleanField(default=True) + anonymous_idempotency_key_fingerprint = models.CharField( + max_length=64, + blank=True, + default="", + ) + anonymous_payload_fingerprint = models.CharField( + max_length=64, + blank=True, + default="", + ) status = models.CharField( max_length=32, choices=Status.choices, @@ -105,6 +116,14 @@ class Meta: status__in=["issued", "partially_returned"], ), ), + models.Index( + fields=["makerspace", "status"], + name="hwreq_anon_pending_idx", + condition=( + models.Q(status="pending_approval") + & ~models.Q(anonymous_idempotency_key_fingerprint="") + ), + ), ] constraints = [ models.UniqueConstraint( @@ -117,6 +136,11 @@ class Meta: ), name="uniq_active_loan_per_box", ), + models.UniqueConstraint( + fields=["makerspace", "anonymous_idempotency_key_fingerprint"], + condition=~models.Q(anonymous_idempotency_key_fingerprint=""), + name="uniq_hwreq_anon_idempotency", + ), ] diff --git a/backend/apps/hardware_requests/notifications.py b/backend/apps/hardware_requests/notifications.py index b6961541..85d3ee5a 100644 --- a/backend/apps/hardware_requests/notifications.py +++ b/backend/apps/hardware_requests/notifications.py @@ -16,7 +16,6 @@ def notify_request_submitted(request): requester_key="request_received", staff_event="submitted", text_builder=_build_submitted_request_message, - reply_markup_builder=_submitted_reply_markup, ) @@ -84,7 +83,6 @@ def _notify( requester_key, staff_event, text_builder, - reply_markup_builder=None, sync=False, ): logger.info( @@ -107,12 +105,7 @@ def build(): .get(pk=request_id) ) emails = _email_deliveries(row, requester_key, staff_event) - markup = reply_markup_builder(row) if reply_markup_builder else None - return LifecyclePayload( - text=text_builder(row), - emails=emails, - telegram_reply_markup=markup, - ) + return LifecyclePayload(text=text_builder(row), emails=emails) return notify_lifecycle( request.makerspace, @@ -125,7 +118,10 @@ def build(): def _email_deliveries(request, requester_key, staff_event): deliveries = [] - if request.requester_contact_email: + if request.requester_contact_email and request.requester_contact_verified: + # Account-less contact is only a claim made by the submitter. Sending any + # lifecycle message before verification would turn this endpoint into an + # email-bomb against arbitrary third parties. Staff delivery remains below. rendered = render_email(request, requester_key) deliveries.append( EmailDelivery( @@ -177,20 +173,6 @@ def render_email(request, key): ) -def _submitted_reply_markup(request): - return { - "inline_keyboard": [ - [ - {"text": "Accept", "callback_data": f"accept:{request.pk}"}, - { - "text": "Reject", - "callback_data": f"reject:{request.pk}:Rejected from Telegram.", - }, - ] - ] - } - - def _build_submitted_request_message(request): lines = [ f"New hardware request #{request.pk}", @@ -200,6 +182,11 @@ def _build_submitted_request_message(request): lines.append(f"Email: {request.requester_contact_email}") if request.requester_contact_phone: lines.append(f"Phone: {request.requester_contact_phone}") + if not request.requester_contact_verified: + # The same warning the staff console and /control/ carry. Telegram is where a + # request is FIRST seen, so omitting it here would mean the one surface that + # reaches staff instantly is the one that implies the contact is trustworthy. + lines.append("Contact NOT verified - confirm identity before handing over.") if request.requested_for: lines.append(f"Requested for: {_clamp(request.requested_for, 300)}") items = list(request.items.all()) @@ -212,6 +199,10 @@ def _build_submitted_request_message(request): lines.append(f"- ...and {len(items) - len(shown)} more") else: lines.append("Items: None") + # Telegram is a NOTIFICATION channel, not a decision surface: the accept/reject + # buttons that used to ride on this message are gone, so the alert has to say where + # the decision is actually made or it becomes a dead end. + lines.append("Review and decide in the staff console.") return _clamp("\n".join(lines), 4000) diff --git a/backend/apps/hardware_requests/public_views.py b/backend/apps/hardware_requests/public_views.py index 367a2eb2..a69ae279 100644 --- a/backend/apps/hardware_requests/public_views.py +++ b/backend/apps/hardware_requests/public_views.py @@ -1,31 +1,45 @@ +import json import uuid from types import SimpleNamespace -from drf_spectacular.utils import extend_schema +from django.conf import settings +from drf_spectacular.utils import OpenApiParameter, extend_schema from rest_framework import generics, status -from rest_framework.exceptions import ValidationError -from rest_framework.permissions import AllowAny, IsAuthenticated +from rest_framework.exceptions import NotAuthenticated, Throttled, ValidationError +from rest_framework.permissions import AllowAny from rest_framework.response import Response from rest_framework.views import APIView +from apps.accounts.audit_events import fingerprint from apps.apiclients.throttling import ClientTierRateThrottle, MemberPrincipalRateThrottle from apps.hardware_requests import workflow from apps.hardware_requests.models import HardwareRequest +from apps.hardware_requests.request_workflow import ( + RequesterSnapshot, + anonymous_idempotency_replay, +) from apps.hardware_requests.serializers import ( PublicRequestStatusSerializer, RequestSubmitResponseSerializer, RequestSubmitSerializer, ) +from apps.hardware_requests.throttles import ( + AnonymousRequestEmailThrottle, + AnonymousRequestIpBurstThrottle, + AnonymousRequestIpHourThrottle, +) from apps.hardware_requests.view_helpers import ( ERROR_404, PUBLIC_ERROR_RESPONSES, request_queryset, ) from apps.inventory.models import InventoryProduct +from apps.makerspaces.anonymous_requesters import get_or_create_anonymous_requester from apps.makerspaces.lookup import get_public_makerspace -from apps.makerspaces.servability import servable_queryset from apps.makerspaces.platform import module_enabled -from apps.presence.guard import require_active_member_presence +from apps.makerspaces.request_access import anonymous_requests_allowed +from apps.makerspaces.servability import servable_queryset +from apps.presence.guard import require_active_account, require_active_member_presence from apps.openapi import ( PUBLIC_API_AUTH_PARAMETERS, PUBLIC_REQUEST_STATUS_EXAMPLE, @@ -34,39 +48,121 @@ class RequestSubmitView(APIView): - permission_classes = [IsAuthenticated] - throttle_classes = [MemberPrincipalRateThrottle] + permission_classes = [AllowAny] + # Every IP/principal budget is declared here so DRF applies it in APIView.initial(), + # BEFORE the handler runs. `check_throttles` is deliberately NOT overridden: it is a + # pre-auth lifecycle hook, `apps.accounts.claim_route_guard` refuses any route that + # replaces one, and overriding it here had already cost the endpoint its floor -- + # anonymous throttles were selected inside post(), which is *after* the + # `anonymous_requests_allowed` refusal, so every makerspace that had not opted in + # served an UNTHROTTLED 401 that still paid for a makerspace lookup. + # + # Each class self-selects by auth state (see `_AnonymousIpThrottle`), so listing them + # together does not double-charge anyone. Consequence worth knowing: the honeypot no + # longer precedes the IP budget, so a bot loud enough to exhaust it sees 429 instead + # of the honeypot's fake success. Being rate-limited before being fingerprinted is + # the safer of the two, and the honeypot still absorbs every bot under the limit. + throttle_classes = [ + MemberPrincipalRateThrottle, + AnonymousRequestIpBurstThrottle, + AnonymousRequestIpHourThrottle, + ] throttle_scope = "public_request_submit" @extend_schema( tags=["Public requests"], summary="Submit public borrow request", - parameters=PUBLIC_API_AUTH_PARAMETERS, + auth=[{"jwtAuth": []}, {}], + parameters=[ + *PUBLIC_API_AUTH_PARAMETERS, + OpenApiParameter( + name="Idempotency-Key", + type=str, + location=OpenApiParameter.HEADER, + required=False, + description=( + "Required for account-less submissions. Reusing a key with the same " + "payload returns the original request; a different payload is rejected." + ), + ), + ], request=RequestSubmitSerializer, responses={201: RequestSubmitResponseSerializer, **PUBLIC_ERROR_RESPONSES}, examples=[PUBLIC_REQUEST_SUBMIT_EXAMPLE], ) def post(self, request, makerspace_slug, *args, **kwargs): makerspace = get_public_makerspace(makerspace_slug) - _require_module(makerspace, "request_workflow") - require_active_member_presence(request.user, makerspace) - # Honeypot check FIRST, on the raw payload: a bot that fills `website` must get a - # normal-looking success even if it also garbled a required field — otherwise a - # validation error would reveal that the honeypot was the rejection trigger. - if _honeypot_filled(request.data): - decoy = SimpleNamespace( - public_token=uuid.uuid4(), - status=HardwareRequest.Status.PENDING_APPROVAL, - ) - return Response( - RequestSubmitResponseSerializer(decoy).data, - status=status.HTTP_201_CREATED, - ) - serializer = RequestSubmitSerializer(data=request.data) + anonymous_submission = not request.user.is_authenticated + if anonymous_submission: + if not anonymous_requests_allowed(makerspace): + # Raising DRF's own exception preserves the previous IsAuthenticated + # response body as well as its 401 status for every non-opted-in space. + # + # `anonymous_requests_allowed` re-derives the answer rather than reading + # the column, so a row that somehow carries BOTH the flag and the + # `membership` module -- raw SQL, an old backup, a restore that predates + # the model rule -- still fails closed here instead of admitting a + # stranger past the membership requirement. + raise NotAuthenticated() + # Resolving the opt-in flag is unavoidable because disabled spaces must + # retain their 401. The IP budgets were already charged in initial(); from + # here the raw honeypot precedes module checks, serializer work, product + # queries and principal creation. + if _honeypot_filled(request.data): + return _honeypot_response() + _require_module(makerspace, "request_workflow") + else: + _require_module(makerspace, "request_workflow") + if module_enabled(makerspace, "membership"): + require_active_member_presence(request.user, makerspace) + else: + # Waiver acceptance lives on MakerspaceMembership and cannot be recorded + # with membership off. In this configuration the flow is public request -> + # STAFF ACCEPT, and staff acceptance is the proposal-time control. + require_active_account(request.user, makerspace) + if _honeypot_filled(request.data): + return _honeypot_response() + + serializer = RequestSubmitSerializer( + data=request.data, + context={"anonymous_submission": anonymous_submission}, + ) serializer.is_valid(raise_exception=True) data = serializer.validated_data data.pop("website", None) + idempotency_key_fingerprint = "" + payload_fingerprint = "" + if anonymous_submission: + request.anonymous_contact_email = data["contact_email"] + _enforce_throttles(request, self, (AnonymousRequestEmailThrottle,)) + idempotency_key = str(request.headers.get("Idempotency-Key", "")).strip() + if not idempotency_key: + raise ValidationError( + {"idempotency_key": "This header is required for anonymous submissions."} + ) + if len(idempotency_key) > settings.ANONYMOUS_REQUEST_IDEMPOTENCY_KEY_MAX_LENGTH: + raise ValidationError( + { + "idempotency_key": ( + "Ensure this header has no more than " + f"{settings.ANONYMOUS_REQUEST_IDEMPOTENCY_KEY_MAX_LENGTH} characters." + ) + } + ) + idempotency_key_fingerprint = fingerprint(idempotency_key) + payload_fingerprint = _anonymous_payload_fingerprint(data) + replay = anonymous_idempotency_replay( + makerspace, + idempotency_key_fingerprint, + payload_fingerprint, + ) + if replay is not None: + return Response( + RequestSubmitResponseSerializer(replay).data, + status=status.HTTP_201_CREATED, + ) + product_ids = [item["product_id"] for item in data["items"]] products = _requestable_products(product_ids, makerspace) if len(products) != len(product_ids): @@ -74,6 +170,27 @@ def post(self, request, makerspace_slug, *args, **kwargs): {"items": "One or more products are unavailable for request."} ) + if anonymous_submission: + requester_principal = get_or_create_anonymous_requester(makerspace) + contact_snapshot = RequesterSnapshot( + username="", + name=data["contact_name"].strip(), + email=data["contact_email"], + phone=data.get("contact_phone", ""), + contact_verified=False, + ) + audit_actor = None + else: + requester_principal = request.user + contact_snapshot = RequesterSnapshot( + username=request.user.username, + name=request.user.display_name, + email=request.user.email, + phone=request.user.phone, + contact_verified=True, + ) + audit_actor = request.user + hardware_request = workflow.submit_request( makerspace, [ @@ -84,7 +201,11 @@ def post(self, request, makerspace_slug, *args, **kwargs): for item in data["items"] ], data["requested_for"], - requester=request.user, + requester_principal=requester_principal, + contact_snapshot=contact_snapshot, + audit_actor=audit_actor, + idempotency_key_fingerprint=idempotency_key_fingerprint, + payload_fingerprint=payload_fingerprint, ) return Response( RequestSubmitResponseSerializer(hardware_request).data, @@ -92,6 +213,39 @@ def post(self, request, makerspace_slug, *args, **kwargs): ) +def _honeypot_response(): + decoy = SimpleNamespace( + public_token=uuid.uuid4(), + status=HardwareRequest.Status.PENDING_APPROVAL, + ) + return Response( + RequestSubmitResponseSerializer(decoy).data, + status=status.HTTP_201_CREATED, + ) + + +def _enforce_throttles(request, view, throttle_types): + waits = [] + for throttle_type in throttle_types: + throttle = throttle_type() + if not throttle.allow_request(request, view): + waits.append(throttle.wait()) + if waits: + durations = [wait for wait in waits if wait is not None] + raise Throttled(wait=max(durations) if durations else None) + + +def _anonymous_payload_fingerprint(data): + canonical = { + "contact_email": data["contact_email"], + "contact_name": data["contact_name"].strip(), + "contact_phone": data.get("contact_phone", ""), + "items": sorted(data["items"], key=lambda item: item["product_id"]), + "requested_for": data["requested_for"], + } + return fingerprint(json.dumps(canonical, sort_keys=True, separators=(",", ":"))) + + class RequestStatusView(generics.RetrieveAPIView): permission_classes = [AllowAny] throttle_classes = [ClientTierRateThrottle] diff --git a/backend/apps/hardware_requests/request_workflow.py b/backend/apps/hardware_requests/request_workflow.py index 50084498..4f3e30e6 100644 --- a/backend/apps/hardware_requests/request_workflow.py +++ b/backend/apps/hardware_requests/request_workflow.py @@ -1,3 +1,6 @@ +from dataclasses import dataclass + +from django.conf import settings from django.db import transaction from django.utils import timezone @@ -5,33 +8,67 @@ from apps.hardware_requests import notifications from apps.hardware_requests.models import HardwareRequest, HardwareRequestItem from apps.hardware_requests.workflow_errors import ( + AnonymousRequestIdempotencyConflict, + AnonymousRequestOutstandingLimit, InvalidTransition, RequestValidationError, RequesterBlocked, ) from apps.hardware_requests.workflow_utils import locked_request from apps.inventory import availability +from apps.makerspaces.models import Makerspace from apps.notifications.emit import emit_notification +@dataclass(frozen=True) +class RequesterSnapshot: + """Identity captured on a request independently from its durable principal.""" + + username: str + name: str + email: str + phone: str + contact_verified: bool + + def submit_request( makerspace, items, requested_for="", *, - requester, + requester_principal, + contact_snapshot, + audit_actor, + idempotency_key_fingerprint="", + payload_fingerprint="", ): with transaction.atomic(): from apps.encryption.write_fence import assert_mapped_write_allowed assert_mapped_write_allowed(makerspace.id) + if not contact_snapshot.contact_verified: + # Serialize the tenant ceiling with creation. A count followed by INSERT + # without this lock lets concurrent requests all observe spare capacity. + makerspace = Makerspace.objects.select_for_update().get(pk=makerspace.pk) + existing = anonymous_idempotency_replay( + makerspace, + idempotency_key_fingerprint, + payload_fingerprint, + ) + if existing is not None: + return existing + _enforce_anonymous_outstanding_limit(makerspace) + request = HardwareRequest.objects.create( makerspace=makerspace, - requester=requester, - requester_username=requester.username, - requester_name=requester.display_name, - requester_contact_email=requester.email, - requester_contact_phone=requester.phone, + requester=requester_principal, + requester_username=contact_snapshot.username, + requester_name=contact_snapshot.name, + requester_contact_email=contact_snapshot.email, + requester_contact_phone=contact_snapshot.phone, + requester_contact_verified=contact_snapshot.contact_verified, + anonymous_idempotency_key_fingerprint=idempotency_key_fingerprint, + anonymous_payload_fingerprint=payload_fingerprint, status=HardwareRequest.Status.PENDING_APPROVAL, requested_for=requested_for, ) @@ -46,7 +83,7 @@ def submit_request( ] ) audit.record( - requester, + audit_actor, "request.submitted", makerspace=makerspace, target=request, @@ -62,6 +99,35 @@ def submit_request( return request +def anonymous_idempotency_replay(makerspace, key_fingerprint, payload_fingerprint): + if not key_fingerprint: + raise RequestValidationError("An Idempotency-Key header is required.") + existing = HardwareRequest.objects.filter( + makerspace=makerspace, + anonymous_idempotency_key_fingerprint=key_fingerprint, + ).first() + if existing is None: + return None + if existing.anonymous_payload_fingerprint != payload_fingerprint: + raise AnonymousRequestIdempotencyConflict( + "This idempotency key was already used for a different request." + ) + return existing + + +def _enforce_anonymous_outstanding_limit(makerspace): + limit = settings.ANONYMOUS_REQUEST_OUTSTANDING_LIMIT + outstanding = HardwareRequest.objects.filter( + makerspace=makerspace, + status=HardwareRequest.Status.PENDING_APPROVAL, + ).exclude(anonymous_idempotency_key_fingerprint="").count() + if outstanding >= limit: + raise AnonymousRequestOutstandingLimit( + "This makerspace is not accepting more anonymous requests until " + "pending requests are reviewed." + ) + + def accept_request(actor, request, accepted=None): with transaction.atomic(): locked = locked_request(request) diff --git a/backend/apps/hardware_requests/serializers.py b/backend/apps/hardware_requests/serializers.py index 471c127a..8bf22a41 100644 --- a/backend/apps/hardware_requests/serializers.py +++ b/backend/apps/hardware_requests/serializers.py @@ -6,20 +6,65 @@ class RequestItemInputSerializer(serializers.Serializer): product_id = serializers.IntegerField() - quantity = serializers.IntegerField(min_value=1) + quantity = serializers.IntegerField(min_value=1, max_value=99) class RequestSubmitSerializer(serializers.Serializer): + CONTACT_FIELDS = ("contact_name", "contact_email", "contact_phone") + website = serializers.CharField(required=False, allow_blank=True, write_only=True) + contact_name = serializers.CharField( + required=False, + allow_blank=True, + max_length=200, + help_text="Required for an account-less submission.", + ) + contact_email = serializers.EmailField( + required=False, + allow_blank=True, + max_length=254, + help_text="Required for an account-less submission; normalized to lowercase.", + ) + contact_phone = serializers.CharField( + required=False, + allow_blank=True, + max_length=32, + ) requested_for = serializers.CharField( required=False, allow_blank=True, default="", + max_length=500, ) - items = RequestItemInputSerializer(many=True, allow_empty=False) + items = serializers.ListField( + child=RequestItemInputSerializer(), + allow_empty=False, + max_length=20, + ) + + def to_internal_value(self, data): + if not self.context.get("anonymous_submission", False): + # Authenticated identity remains account-derived. Removing these fields + # before child validation makes them genuinely ignored, including an + # oversized spoof value that must not turn into a validation side channel. + data = data.copy() + for field_name in self.CONTACT_FIELDS: + data.pop(field_name, None) + return super().to_internal_value(data) + + def validate_contact_email(self, value): + return value.strip().lower() def validate(self, attrs): attrs["website"] = attrs.get("website", "") + if self.context.get("anonymous_submission", False): + errors = {} + if not attrs.get("contact_name", "").strip(): + errors["contact_name"] = "This field is required." + if not attrs.get("contact_email", "").strip(): + errors["contact_email"] = "This field is required." + if errors: + raise serializers.ValidationError(errors) product_ids = [item["product_id"] for item in attrs["items"]] if len(product_ids) != len(set(product_ids)): raise serializers.ValidationError( @@ -101,6 +146,7 @@ class AdminRequestSerializer(serializers.Serializer): requester_display = serializers.SerializerMethodField() requester_contact_email = serializers.EmailField(read_only=True) requester_contact_phone = serializers.CharField(read_only=True) + requester_contact_verified = serializers.BooleanField(read_only=True) status = serializers.CharField(read_only=True) requested_for = serializers.CharField(read_only=True) rejection_reason = serializers.CharField(read_only=True) diff --git a/backend/apps/hardware_requests/throttles.py b/backend/apps/hardware_requests/throttles.py new file mode 100644 index 00000000..995b6101 --- /dev/null +++ b/backend/apps/hardware_requests/throttles.py @@ -0,0 +1,49 @@ +"""Independent abuse budgets for account-less hardware-request proposals.""" + +from rest_framework.throttling import SimpleRateThrottle + +from apps.accounts.audit_events import fingerprint + + +class _AnonymousIpThrottle(SimpleRateThrottle): + """Per-IP budget for ACCOUNT-LESS submissions only. + + Returning None for an authenticated caller is what lets these sit in the view's + `throttle_classes` beside the member throttle: DRF applies every class on every + request, and an authenticated member already has a per-principal budget. Without the + skip, a makerspace behind one NAT would rate-limit its own signed-in members by the + shared egress IP. + """ + + def get_cache_key(self, request, view): + user = getattr(request, "user", None) + if getattr(user, "is_authenticated", False): + return None + ident = self.get_ident(request) + return None if not ident else self.cache_format % { + "scope": self.scope, + "ident": ident, + } + + +class AnonymousRequestIpBurstThrottle(_AnonymousIpThrottle): + scope = "anonymous_request_ip_burst" + + +class AnonymousRequestIpHourThrottle(_AnonymousIpThrottle): + scope = "anonymous_request_ip_hour" + + +class AnonymousRequestEmailThrottle(SimpleRateThrottle): + scope = "anonymous_request_email" + + def get_cache_key(self, request, view): + email = str(getattr(request, "anonymous_contact_email", "") or "").strip().lower() + if not email: + return None + # Cache keys are operational data at rest. The normalized email is never + # embedded directly; this follows the password-reset/phone throttle pattern. + return self.cache_format % { + "scope": self.scope, + "ident": fingerprint(email), + } diff --git a/backend/apps/hardware_requests/workflow.py b/backend/apps/hardware_requests/workflow.py index 90a78e3b..d1fac415 100644 --- a/backend/apps/hardware_requests/workflow.py +++ b/backend/apps/hardware_requests/workflow.py @@ -10,6 +10,8 @@ ) from apps.hardware_requests.return_workflow import return_items from apps.hardware_requests.workflow_errors import ( + AnonymousRequestIdempotencyConflict, + AnonymousRequestOutstandingLimit, BoxUnavailable, BoxValidationError, EvidenceNotUploaded, @@ -20,6 +22,8 @@ ) __all__ = [ + "AnonymousRequestIdempotencyConflict", + "AnonymousRequestOutstandingLimit", "BoxUnavailable", "BoxValidationError", "EvidenceNotUploaded", diff --git a/backend/apps/hardware_requests/workflow_errors.py b/backend/apps/hardware_requests/workflow_errors.py index 9f7d3950..f857292d 100644 --- a/backend/apps/hardware_requests/workflow_errors.py +++ b/backend/apps/hardware_requests/workflow_errors.py @@ -10,6 +10,14 @@ class RequestValidationError(Exception): pass +class AnonymousRequestIdempotencyConflict(Exception): + pass + + +class AnonymousRequestOutstandingLimit(Exception): + pass + + class ReturnValidationError(Exception): pass diff --git a/backend/apps/integrations/dispatch_channels.py b/backend/apps/integrations/dispatch_channels.py index 81400563..c3f15695 100644 --- a/backend/apps/integrations/dispatch_channels.py +++ b/backend/apps/integrations/dispatch_channels.py @@ -250,7 +250,6 @@ def _deliver_notification(log) -> NotificationDeliveryLog: ok = send_message( log.makerspace, log.text_body, - reply_markup=(log.payload or {}).get("reply_markup"), destination=log.destination, ) else: diff --git a/backend/apps/integrations/models_destinations.py b/backend/apps/integrations/models_destinations.py index 2e72c590..326b7228 100644 --- a/backend/apps/integrations/models_destinations.py +++ b/backend/apps/integrations/models_destinations.py @@ -13,13 +13,15 @@ webhook URL being saved as a chat id. Two nullable columns and a per-channel check constraint make the wrong row unrepresentable. -**Telegram destinations carry NO bot token.** Telegram is bidirectional: the accept/reject -buttons post back to a single registered webhook authenticated by one -`TELEGRAM_WEBHOOK_SECRET`, so a second bot's callbacks cannot be authenticated or routed. -A per-destination token would create rooms that can send but whose buttons are dead, which -in a staff room reads as a broken accept rather than a configuration limit. One bot added -to many groups gives per-machine rooms and keeps callbacks working. Per-bot destinations -would need per-bot webhook secrets and inbound routing — its own phase. +**Telegram destinations carry NO bot token.** The original reason was inbound: accept/reject +buttons posted back to a single registered webhook authenticated by one +`TELEGRAM_WEBHOOK_SECRET`, so a second bot's callbacks could not be authenticated or routed. +**Those buttons are gone** — chat is a notification channel now, not a decision surface — so +that argument has expired, and the rule stands on the outbound half instead: one bot identity +per makerspace across all of its rooms is what makes per-machine rooms read as a single +sender rather than a handful of unrelated bots, and it keeps the token surface to one secret +per tenant. If a button is ever reintroduced, per-bot destinations need per-bot webhook +secrets and inbound routing first — its own phase, exactly as before. **No scope links means space-wide**, which is deliberately the OPPOSITE default to role machine-scope. An unscoped *role* must see nothing (access fails closed); an unscoped diff --git a/backend/apps/integrations/notify.py b/backend/apps/integrations/notify.py index ba559a9c..2a5104e2 100644 --- a/backend/apps/integrations/notify.py +++ b/backend/apps/integrations/notify.py @@ -37,7 +37,6 @@ class EmailDelivery: class LifecyclePayload: text: str emails: tuple[EmailDelivery, ...] = () - telegram_reply_markup: dict | None = None # What this alert is about, for destination scoping. It rides on the payload rather # than being a `notify_lifecycle` parameter because only `build()` has resolved the # domain object — the caller often has just a primary key. `None` means the alert @@ -135,12 +134,6 @@ def _run_guarded(makerspace, feature, event, build, sync): if not enabled[channel]: continue try: - payload_data = ( - {"reply_markup": payload.telegram_reply_markup} - if channel == NotificationChannel.TELEGRAM - and payload.telegram_reply_markup - else None - ) logs = dispatch_channel( makerspace=makerspace, channel=channel, @@ -153,7 +146,6 @@ def _run_guarded(makerspace, feature, event, build, sync): if channel == NotificationChannel.NATIVE_PUSH else chat_text ), - payload=payload_data, sync=sync, scope=payload.scope, ) diff --git a/backend/apps/integrations/telegram.py b/backend/apps/integrations/telegram.py index ae034b4d..f564b904 100644 --- a/backend/apps/integrations/telegram.py +++ b/backend/apps/integrations/telegram.py @@ -14,13 +14,15 @@ class TelegramDeliveryError(Exception): def resolve_bot_token(makerspace): - """The bot a makerspace posts as. + """The bot a makerspace posts as: its own token, else the deployment's. - Destinations deliberately do NOT override this (D16). Telegram is bidirectional: the - accept/reject buttons post back to one registered webhook authenticated by a single - `TELEGRAM_WEBHOOK_SECRET`, so a second bot's callbacks could not be authenticated or - routed — a room on its own bot could send, but its buttons would be dead. One bot in - many groups gives per-machine rooms and keeps callbacks working. + Destinations deliberately do NOT override this (D16). The original reason was + inbound: accept/reject buttons posted back to one registered webhook authenticated by + a single `TELEGRAM_WEBHOOK_SECRET`, so a second bot's callbacks could not be + authenticated or routed. **Those buttons are gone** — chat is no longer an action + surface — but the rule stands on its outbound half: one bot identity per makerspace + across all of its rooms is what makes per-machine rooms read as one sender rather + than as a handful of unrelated bots, and it keeps the token surface to one secret. """ token = ( makerspace.get_telegram_bot_token() @@ -36,7 +38,7 @@ def resolve_chat_id(makerspace, destination=None): return getattr(makerspace, "telegram_group_chat_id", "") -def send_message(makerspace, text, reply_markup=None, destination=None): +def send_message(makerspace, text, destination=None): token = resolve_bot_token(makerspace) chat_id = resolve_chat_id(makerspace, destination) if not token or not chat_id: @@ -51,9 +53,10 @@ def send_message(makerspace, text, reply_markup=None, destination=None): return False base_url = getattr(settings, "TELEGRAM_API_URL", "https://api.telegram.org").rstrip("/") + # No `reply_markup`: Telegram is a notification channel here, and an inline keyboard + # is by definition an action surface. The accept/reject buttons were removed with the + # callback route that served them. payload = {"chat_id": chat_id, "text": trim_for_channel("telegram", text)} - if reply_markup: - payload["reply_markup"] = reply_markup try: body = json.dumps(payload).encode() req = urllib_request.Request( diff --git a/backend/apps/integrations/views.py b/backend/apps/integrations/views.py index 2bf43681..3af616ed 100644 --- a/backend/apps/integrations/views.py +++ b/backend/apps/integrations/views.py @@ -1,4 +1,5 @@ import hmac +import logging from django.conf import settings from django.shortcuts import get_object_or_404 @@ -10,8 +11,6 @@ from apps.accounts import rbac from apps.accounts.models import User -from apps.hardware_requests import workflow -from apps.hardware_requests.models import HardwareRequest from apps.integrations.serializers import ( TelegramTestAlertSerializer, TelegramWebhookSerializer, @@ -19,46 +18,53 @@ from apps.integrations.telegram import TelegramDeliveryError, send_message from apps.makerspaces.models import Makerspace from apps.makerspaces.guards import require_module -from apps.tenant_migration.gate_runtime import tenant_write + +logger = logging.getLogger(__name__) class TelegramWebhookView(APIView): + """Accept-and-ignore. Telegram is a notification channel, not an action surface. + + The accept/reject buttons are gone and this endpoint no longer touches the request + workflow. **The route is kept anyway**, deliberately: a deployment that has already + called `setWebhook` has this URL registered with Telegram, we cannot call + `deleteWebhook` on its behalf, and Telegram retries a non-2xx response for hours. A + 200 acknowledgement is the graceful retirement; a 404 would be a permanent error loop + in someone else's infrastructure. + + The secret check stays for the same reason it was added -- `from.id` in the payload is + attacker-controllable and the endpoint must stay closed to strangers -- and because + the moment a callback DID something again, an endpoint that had quietly stopped + checking would be the vulnerability. + """ + permission_classes = [AllowAny] throttle_classes = [ScopedRateThrottle] throttle_scope = "telegram_webhook" @extend_schema( tags=["Telegram"], - summary="Receive Telegram callback webhook", + summary="Acknowledge a Telegram webhook (no action is taken)", + description=( + "Retained so an already-registered webhook does not retry forever. Callback " + "queries are acknowledged and discarded: accepting and rejecting borrow " + "requests happens in the staff console, never from chat." + ), auth=[], request=TelegramWebhookSerializer, - responses={200: OpenApiResponse(description="Webhook processed.")}, + responses={200: OpenApiResponse(description="Acknowledged; no action taken.")}, ) def post(self, request, *args, **kwargs): if not _webhook_secret_ok(request): return Response({"detail": "Invalid webhook secret."}, status=403) serializer = TelegramWebhookSerializer(data=request.data) serializer.is_valid(raise_exception=True) - callback = serializer.validated_data.get("callback_query") - if not callback: - return Response({"detail": "Ignored."}) - - actor = _telegram_actor(callback) - action, request_id, reason = _parse_callback(callback.get("data", "")) - hardware_request = get_object_or_404(HardwareRequest, pk=request_id) - with tenant_write(hardware_request.makerspace_id): - require_module(hardware_request.makerspace, "telegram") - if action == "accept": - if not rbac.can(actor, rbac.Action.ACCEPT_REQUEST, hardware_request.makerspace_id): - return Response({"detail": "Permission denied."}, status=403) - workflow.accept_request(actor, hardware_request) - elif action == "reject": - if not rbac.can(actor, rbac.Action.REJECT_REQUEST, hardware_request.makerspace_id): - return Response({"detail": "Permission denied."}, status=403) - workflow.reject_request(actor, hardware_request, reason or "Rejected from Telegram.") - else: - return Response({"detail": "Unsupported action."}, status=400) - return Response({"detail": "Processed."}) + if serializer.validated_data.get("callback_query"): + logger.info( + "Telegram callback ignored; chat is not an action surface.", + extra={"has_callback": True}, + ) + return Response({"detail": "Ignored."}) class TelegramTestAlertView(APIView): @@ -114,42 +120,11 @@ def post(self, request, *args, **kwargs): def _webhook_secret_ok(request): # Telegram echoes the secret_token configured at setWebhook time in this header. - # Fail closed when unset: `from.id` in the payload is attacker-controllable, so - # without this the accept/reject workflow could be driven by anyone. + # Fail closed when unset. Nothing behind this endpoint acts any more, but `from.id` + # in the payload is attacker-controllable, and an endpoint that quietly stopped + # authenticating is exactly what would turn a future callback into a vulnerability. secret = settings.TELEGRAM_WEBHOOK_SECRET if not secret: return False provided = request.headers.get("X-Telegram-Bot-Api-Secret-Token", "") return hmac.compare_digest(provided, secret) - - -def _telegram_actor(callback): - from rest_framework.exceptions import PermissionDenied - - telegram_id = str((callback.get("from") or {}).get("id") or "") - # Guard the empty id: filtering on "" would match every user with a blank - # telegram_user_id. Also require active standing so a suspended/restricted or - # deactivated staffer can't drive accept/reject from an old inline button. - if not telegram_id: - raise PermissionDenied("Telegram actor is not linked to a staff user.") - actor = User.objects.filter( - telegram_user_id=telegram_id, - is_active=True, - access_status=User.AccessStatus.ACTIVE, - ).first() - if not actor: - raise PermissionDenied("Telegram actor is not linked to an active staff user.") - return actor - - -def _parse_callback(data): - parts = str(data).split(":", 2) - if len(parts) < 2: - return None, None, "" - action = parts[0] - try: - request_id = int(parts[1]) - except ValueError: - request_id = None - reason = parts[2] if len(parts) > 2 else "" - return action, request_id, reason diff --git a/backend/apps/makerspaces/admin_capabilities.py b/backend/apps/makerspaces/admin_capabilities.py index db4df0fd..3992bb9b 100644 --- a/backend/apps/makerspaces/admin_capabilities.py +++ b/backend/apps/makerspaces/admin_capabilities.py @@ -6,6 +6,7 @@ from apps.makerspaces.capabilities import FEATURE_DEFINITIONS, validate_capabilities from apps.makerspaces.admin_images import MakerspaceAdminForm as ImageMakerspaceAdminForm from apps.makerspaces.module_registry import MODULES +from apps.makerspaces.request_access import effective_policy class CapabilityMatrixWidget(forms.CheckboxSelectMultiple): @@ -80,6 +81,12 @@ def __init__(self, *args, **kwargs): "modules": sorted(set(self.instance.enabled_modules or [])), "features": sorted(set(self.instance.enabled_features or [])), } + # Captured here, from the UNMODIFIED instance: `clean_capabilities` rewrites + # `enabled_modules` in place, so by `save_model` the old policy is gone. Ticking + # `membership` in this matrix forces account-less requests off inside + # `Makerspace.save()`, and the module/feature lists alone cannot tell an + # `anyone -> members` change from an `accounts -> members` one. + self.request_access_before = effective_policy(self.instance) def clean_capabilities(self): values = self.cleaned_data["capabilities"] @@ -102,11 +109,23 @@ def save_model(self, request, obj, form, change): "modules": sorted(set(obj.enabled_modules or [])), "features": sorted(set(obj.enabled_features or [])), } - if change and before != after: + request_access_before = getattr(form, "request_access_before", None) + request_access_after = effective_policy(obj) + request_access_changed = ( + request_access_before is not None + and request_access_before != request_access_after + ) + if change and (before != after or request_access_changed): + meta = {"before": before, "after": after} + if request_access_changed: + meta["request_access"] = { + "before": request_access_before, + "after": request_access_after, + } audit.record( request.user, "makerspace.capabilities_changed", makerspace=obj, target=obj, - meta={"before": before, "after": after}, - ) \ No newline at end of file + meta=meta, + ) diff --git a/backend/apps/makerspaces/anonymous_requesters.py b/backend/apps/makerspaces/anonymous_requesters.py new file mode 100644 index 00000000..b7dde6e7 --- /dev/null +++ b/backend/apps/makerspaces/anonymous_requesters.py @@ -0,0 +1,70 @@ +"""One inert requester principal per makerspace for account-less loan requests.""" + +import uuid + +from django.db import transaction + +from apps.accounts.models import User +from apps.makerspaces.models import Makerspace + +SENTINEL_DISPLAY_NAME = "Anonymous requester (system principal)" + + +def anonymous_requester_ids(makerspace_ids=None): + """The sentinel user ids, for excluding them from per-PERSON aggregates. + + Every account-less request in a makerspace points at ONE principal, so any report + that groups by `requester_id` folds every unrelated stranger into a single fictional + human -- one "repeat offender", one "top borrower". That row is not a person: it + cannot be contacted, cannot be ranked against real borrowers, and must never be + restricted (doing so would restrict every future account-less requester at once, + which is why `accounts.principal_guards.refuse_anonymous_requester_access_mutation` + exists). Reports exclude these ids instead. + + One query. `makerspace_ids=None` means every makerspace, which is what the + organization-wide rankings need. + """ + queryset = Makerspace.objects.exclude(anonymous_requester__isnull=True) + if makerspace_ids is not None: + queryset = queryset.filter(pk__in=makerspace_ids) + return set(queryset.values_list("anonymous_requester_id", flat=True)) + + +def get_or_create_anonymous_requester(makerspace): + """Return the makerspace's sentinel, creating it under the tenant row lock. + + The makerspace lock is deliberately first, matching membership and walk-in + creation. Besides serializing two first requests, this keeps the shared lock order + from turning a concurrent identity operation into a deadlock. + """ + with transaction.atomic(): + locked_space = Makerspace.objects.select_for_update().get(pk=makerspace.pk) + if locked_space.anonymous_requester_id is not None: + existing = User.objects.get(pk=locked_space.anonymous_requester_id) + # The caller handed us its own instance and will keep using it. Without + # this the row is created against `locked_space` and the caller's copy + # still reports `anonymous_requester = None`, which reads as "this space + # has no principal" at the very moment it just acquired one. + makerspace.anonymous_requester = existing + return existing + + # Reuse the existing member_ allocation namespace. The relationship is + # the marker; inventing an anonymous_* namespace would contradict the migration + # contract that reserves walkin_ and member_ for historical identity discovery. + user = User( + username=f"member_{uuid.uuid4().hex}", + display_name=SENTINEL_DISPLAY_NAME, + email="", + phone="", + phone_e164="", + role=User.Role.REQUESTER, + is_active=False, + ) + user.set_unusable_password() + user.save() + + locked_space.anonymous_requester = user + locked_space.save(update_fields=["anonymous_requester"]) + # Same reason as above: keep the caller's instance consistent with the row. + makerspace.anonymous_requester = user + return user diff --git a/backend/apps/makerspaces/management/commands/list_modules.py b/backend/apps/makerspaces/management/commands/list_modules.py index 75122907..fa8f1ef4 100644 --- a/backend/apps/makerspaces/management/commands/list_modules.py +++ b/backend/apps/makerspaces/management/commands/list_modules.py @@ -1,8 +1,11 @@ +import json + from django.core.management.base import BaseCommand, CommandError from apps.makerspaces.models import Makerspace from apps.makerspaces.module_install import module_status from apps.makerspaces.module_profiles import PROFILES +from apps.makerspaces.request_access import effective_policy class Command(BaseCommand): @@ -10,9 +13,31 @@ class Command(BaseCommand): def add_arguments(self, parser): parser.add_argument("--makerspace", default=None, help="Makerspace slug (default: the only one).") + parser.add_argument( + "--json", + action="store_true", + help="Emit machine-readable state instead of the operator listing.", + ) def handle(self, *args, **options): makerspace = resolve_makerspace(options["makerspace"]) + if options["json"]: + # `setup.sh` reads its module state back from here rather than parsing the + # `*`/`+`/`-` marks below. That listing is formatted for a human and has + # already been re-laid-out once; a shell script keying on its punctuation + # would break silently the next time it is. + self.stdout.write( + json.dumps( + { + "makerspace": makerspace.slug, + "installed": sorted( + row["key"] for row in module_status(makerspace) if row["installed"] + ), + "request_access": effective_policy(makerspace), + } + ) + ) + return self.stdout.write(f"Modules for {makerspace.slug}:") for row in module_status(makerspace): if row["core"]: diff --git a/backend/apps/makerspaces/management/commands/set_request_access.py b/backend/apps/makerspaces/management/commands/set_request_access.py new file mode 100644 index 00000000..e5a41440 --- /dev/null +++ b/backend/apps/makerspaces/management/commands/set_request_access.py @@ -0,0 +1,79 @@ +"""Set who may submit borrow requests, and report the policy that actually resulted. + +Called by `setup.sh` AFTER the module tick list has been applied, never before: the +answer to "who can submit?" is only partly this flag, and the rest is the `membership` +module the operator may have just ticked. Deriving the answer from the live row is what +keeps the installer from reporting a policy the database does not have. +""" + +from django.core.management.base import BaseCommand, CommandError + +from apps.makerspaces.management.commands.list_modules import resolve_makerspace +from apps.makerspaces.request_access import ( + ACCOUNTS, + ANYONE, + MEMBERS, + POLICY_LABELS, + RequestAccessConflict, + effective_policy, + set_anonymous_requests, +) + +MODES = (MEMBERS, ACCOUNTS, ANYONE) + + +class Command(BaseCommand): + help = "Set who may submit borrow requests for a makerspace." + + def add_arguments(self, parser): + parser.add_argument("--makerspace", default=None, help="Makerspace slug (default: the only one).") + parser.add_argument( + "--mode", + required=True, + choices=MODES, + help=( + "members = active members only (requires the membership module); " + "accounts = any signed-in account; anyone = no account needed." + ), + ) + + def handle(self, *args, **options): + makerspace = resolve_makerspace(options["makerspace"]) + mode = options["mode"] + try: + # Only `anyone` is a request to OPEN the flag. `members` and `accounts` are + # both "an account is required", and which of the two you get is decided by + # the membership module, not by this command -- so both close the flag and + # then report what the module state actually produced. + resulting = set_anonymous_requests(makerspace, mode == ANYONE) + except RequestAccessConflict as exc: + raise CommandError(str(exc)) from exc + + self.stdout.write( + self.style.SUCCESS( + f"Borrow requests for {makerspace.slug}: {POLICY_LABELS[resulting]}." + ) + ) + if resulting != mode: + # Never silently: asking for `members` on a space without the membership + # module leaves submission open to any signed-in account, and an operator + # who is not told that believes they closed something they did not. + self.stdout.write( + self.style.WARNING( + f"You asked for '{mode}' but the live module state produces " + f"'{resulting}'. " + + ( + "Install the `membership` module to restrict submission to " + "members." + if resulting == ACCOUNTS + else "Uninstall the `membership` module to allow account-less " + "requests." + ) + ) + ) + return None + + +def current_policy(makerspace) -> str: + """Shared with the installer's read-back step.""" + return effective_policy(makerspace) diff --git a/backend/apps/makerspaces/migrations/0066_makerspace_anonymous_requester_and_more.py b/backend/apps/makerspaces/migrations/0066_makerspace_anonymous_requester_and_more.py new file mode 100644 index 00000000..92c5fc25 --- /dev/null +++ b/backend/apps/makerspaces/migrations/0066_makerspace_anonymous_requester_and_more.py @@ -0,0 +1,26 @@ +# Generated by Django 6.0.8 on 2026-08-31 04:01 + +import django.db.models.deletion +from django.conf import settings +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('makerspaces', '0065_rename_accounts_module_key'), + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ] + + operations = [ + migrations.AddField( + model_name='makerspace', + name='anonymous_requester', + field=models.OneToOneField(blank=True, null=True, on_delete=django.db.models.deletion.PROTECT, related_name='+', to=settings.AUTH_USER_MODEL), + ), + migrations.AddField( + model_name='makerspace', + name='anonymous_requests_enabled', + field=models.BooleanField(default=False), + ), + ] diff --git a/backend/apps/makerspaces/migrations/0067_reconcile_anonymous_requests_with_membership.py b/backend/apps/makerspaces/migrations/0067_reconcile_anonymous_requests_with_membership.py new file mode 100644 index 00000000..40d43b1d --- /dev/null +++ b/backend/apps/makerspaces/migrations/0067_reconcile_anonymous_requests_with_membership.py @@ -0,0 +1,43 @@ +"""Close the membership + account-less-requests pair on rows that already carry both. + +`Makerspace.save()` now makes the combination unreachable, but that only guards writes +from here on. A row written before the rule — by the `/control/` capability matrix, a +module install, or a restored backup — can still be sitting in the impossible state, and +`RequestSubmitView` would have admitted a stranger past the membership requirement. + +Fixed in SQL rather than through the model on purpose: this must not depend on `save()` +(the thing being backfilled for), and it must not fan out per-row on a large install. +""" + +from django.db import migrations + + +def close_anonymous_requests_where_membership_installed(apps, schema_editor): + Makerspace = apps.get_model("makerspaces", "Makerspace") + # `enabled_modules` is a JSONField holding a list of keys, so containment is the + # right test — `contains` compiles to the jsonb @> operator. + Makerspace.objects.filter( + anonymous_requests_enabled=True, + enabled_modules__contains=["membership"], + ).update(anonymous_requests_enabled=False) + + +def noop_reverse(apps, schema_editor): + """Deliberately not reversed. + + Re-opening account-less requests on a makerspace that has `membership` installed is + the security hole this migration exists to close; a downgrade must not reinstate it. + Reversing the code change is enough to restore the old behaviour for anyone who + re-enables the flag on purpose. + """ + + +class Migration(migrations.Migration): + dependencies = [("makerspaces", "0066_makerspace_anonymous_requester_and_more")] + + operations = [ + migrations.RunPython( + close_anonymous_requests_where_membership_installed, + noop_reverse, + ), + ] diff --git a/backend/apps/makerspaces/models_makerspace.py b/backend/apps/makerspaces/models_makerspace.py index 89009aa4..469488d1 100644 --- a/backend/apps/makerspaces/models_makerspace.py +++ b/backend/apps/makerspaces/models_makerspace.py @@ -16,7 +16,8 @@ ) from apps.makerspaces.module_registry import default_enabled_module_keys from apps.makerspaces.provenance import validate_actor_snapshot -from apps.makerspaces.secrets import decrypt_value, encrypt_value +from apps.makerspaces.request_access import reconcile_enabled_modules +from apps.makerspaces.models_makerspace_secrets import MakerspaceSecretsMixin from apps.makerspaces.validators import ( DEFAULT_PRESENCE_PRESETS, validate_google_maps_url, @@ -33,7 +34,7 @@ ) -class Makerspace(models.Model): +class Makerspace(MakerspaceSecretsMixin, models.Model): class LifecycleState(models.TextChoices): ACTIVE = "active", "Active" IMPORTING = "importing", "Importing" @@ -74,6 +75,15 @@ class PublicPrintStatusLookupPolicy(models.TextChoices): geofence_radius_m = models.PositiveIntegerField(default=25, validators=[MinValueValidator(1)]) geofence_enabled = models.BooleanField(default=False) public_inventory_enabled = models.BooleanField(default=True) + # Account-less requests are an unauthenticated write surface, so this is an + # independent opt-in. In particular, it must not follow the membership module: + # recommended installs omit that module and must stay closed after an upgrade. + # + # The reverse direction IS coupled, and `save()` enforces it: installing + # `membership` forces this off, because the anonymous branch of RequestSubmitView + # runs before any membership guard and would otherwise walk straight past the + # requirement the operator just switched on. See `request_access`. + anonymous_requests_enabled = models.BooleanField(default=False) public_stats_enabled = models.BooleanField(default=False) public_stats_show_holder_names = models.BooleanField(default=False) public_print_status_lookup_policy = models.CharField( @@ -170,6 +180,13 @@ class PublicPrintStatusLookupPolicy(models.TextChoices): on_delete=models.SET_NULL, related_name="created_makerspaces", ) + anonymous_requester = models.OneToOneField( + settings.AUTH_USER_MODEL, + null=True, + blank=True, + on_delete=models.PROTECT, + related_name="+", + ) # Soft-delete state. archived_at IS NOT NULL ⇒ archived (single source of truth; no # separate boolean). Operational reachability also requires lifecycle_state=ACTIVE; # importing/aborted rows stay visible only to narrow import/recovery operations. @@ -221,6 +238,22 @@ def geofence_effective(self) -> bool: def save(self, *args, **kwargs): self.public_code = (self.public_code or "").upper() self.frontend_domain = normalize_frontend_domain(self.frontend_domain) + # Membership and account-less requests are mutually exclusive, and this is the + # ONE chokepoint every writer passes through: module install/uninstall, profile + # application, the /control/ capability matrix, setup_instance, seed_demo and a + # plain obj.save(). Enforcing it here rather than in each of them is what makes + # the state unreachable instead of merely discouraged -- see + # `request_access` for why the pair is impossible. + reconciled = reconcile_enabled_modules( + self.enabled_modules, self.anonymous_requests_enabled + ) + if reconciled != self.anonymous_requests_enabled: + self.anonymous_requests_enabled = reconciled + # A partial save that did not name this field would otherwise change the + # attribute in memory and leave the row in the impossible state. + update_fields = kwargs.get("update_fields") + if update_fields is not None: + kwargs["update_fields"] = [*update_fields, "anonymous_requests_enabled"] super().save(*args, **kwargs) def clean(self): @@ -258,35 +291,3 @@ def clean(self): ) if self.geofence_enabled and not self.geofence_effective: raise ValidationError({"geofence_enabled": "Set both latitude and longitude before enabling the geofence."}) - - def set_telegram_bot_token(self, raw): - self.telegram_bot_token = encrypt_value(raw) - - def get_telegram_bot_token(self): - return decrypt_value(self.telegram_bot_token) - - def set_smtp_password(self, raw): - self.smtp_password = encrypt_value(raw) - - def get_smtp_password(self): - return decrypt_value(self.smtp_password) - - def set_slack_webhook_url(self, raw): - self.slack_webhook_url = encrypt_value(raw) - - def get_slack_webhook_url(self): - return decrypt_value(self.slack_webhook_url) - - def set_mattermost_webhook_url(self, raw): - self.mattermost_webhook_url = encrypt_value(raw) - - def get_mattermost_webhook_url(self): - return decrypt_value(self.mattermost_webhook_url) - - def set_discord_webhook_url(self, raw): - self.discord_webhook_url = encrypt_value(raw) - - def get_discord_webhook_url(self): - return decrypt_value(self.discord_webhook_url) - - diff --git a/backend/apps/makerspaces/models_makerspace_secrets.py b/backend/apps/makerspaces/models_makerspace_secrets.py new file mode 100644 index 00000000..24cd566c --- /dev/null +++ b/backend/apps/makerspaces/models_makerspace_secrets.py @@ -0,0 +1,51 @@ +"""Per-makerspace integration secrets: the encrypt-on-set, decrypt-on-get pair. + +Split out of `models_makerspace.py` so that file stays under the ~300-line ceiling +before new fields land on `Makerspace`. These ten methods are one repeated shape over +five columns and carry no field definitions, so moving them changes no schema and +needs no migration. + +They stay a mixin rather than module-level helpers because every one of them reads and +writes an attribute of the row it is called on; `makerspace.get_smtp_password()` is the +call site everywhere, and a free function would have to be handed the instance anyway. +""" + +from apps.makerspaces.secrets import decrypt_value, encrypt_value + + +class MakerspaceSecretsMixin: + """Encrypted accessors for the outbound-integration credentials. + + The columns hold ciphertext; nothing outside these methods should read them + directly, which is what keeps `API_CLIENT_ENC_KEY` the single decryption door. + """ + + def set_telegram_bot_token(self, raw): + self.telegram_bot_token = encrypt_value(raw) + + def get_telegram_bot_token(self): + return decrypt_value(self.telegram_bot_token) + + def set_smtp_password(self, raw): + self.smtp_password = encrypt_value(raw) + + def get_smtp_password(self): + return decrypt_value(self.smtp_password) + + def set_slack_webhook_url(self, raw): + self.slack_webhook_url = encrypt_value(raw) + + def get_slack_webhook_url(self): + return decrypt_value(self.slack_webhook_url) + + def set_mattermost_webhook_url(self, raw): + self.mattermost_webhook_url = encrypt_value(raw) + + def get_mattermost_webhook_url(self): + return decrypt_value(self.mattermost_webhook_url) + + def set_discord_webhook_url(self, raw): + self.discord_webhook_url = encrypt_value(raw) + + def get_discord_webhook_url(self): + return decrypt_value(self.discord_webhook_url) diff --git a/backend/apps/makerspaces/module_install.py b/backend/apps/makerspaces/module_install.py index b3df4118..3ca89176 100644 --- a/backend/apps/makerspaces/module_install.py +++ b/backend/apps/makerspaces/module_install.py @@ -16,6 +16,7 @@ from apps.makerspaces.capabilities import prune_features, validate_capabilities from apps.makerspaces.models import Makerspace from apps.makerspaces.module_profiles import MINIMAL, profile_modules +from apps.makerspaces.request_access import effective_policy from apps.makerspaces.module_registry import ( BY_KEY, MODULES, @@ -124,6 +125,12 @@ def apply_profile(makerspace, profile, actor=None): def _apply(locked, modules, actor): before = sorted(set(locked.enabled_modules or [])) before_features = sorted(set(locked.enabled_features or [])) + # Installing `membership` FORCES account-less requests off inside `Makerspace.save()` + # below, because the pair is impossible. That flip is a change of who may submit a + # borrow request, so it has to be audited -- and it can only be audited here: `save()` + # has no actor, and the capability meta records module and feature lists only, which + # cannot distinguish a previous `anyone` policy from `accounts`. + before_request_access = effective_policy(locked) # Drop features whose modules are going away BEFORE validating, or the validation # refuses the change outright -- see `prune_features` for why this is a removal # rather than an error or a silent keep. @@ -134,18 +141,30 @@ def _apply(locked, modules, actor): locked.enabled_modules = canonical_modules locked.enabled_features = canonical_features locked.save(update_fields=["enabled_modules", "enabled_features"]) - if before != canonical_modules or before_features != sorted(canonical_features): + after_request_access = effective_policy(locked) + request_access_changed = before_request_access != after_request_access + if ( + before != canonical_modules + or before_features != sorted(canonical_features) + or request_access_changed + ): + meta = { + "before": {"modules": before, "features": before_features}, + "after": {"modules": canonical_modules, "features": sorted(canonical_features)}, + # Named explicitly: a feature removed as a consequence of a module going + # away is the kind of change an operator will otherwise discover only + # when something stops charging. + "features_dropped_with_modules": dropped_features, + } + if request_access_changed: + meta["request_access"] = { + "before": before_request_access, + "after": after_request_access, + } audit.record( actor, "makerspace.capabilities_changed", makerspace=locked, target=locked, - meta={ - "before": {"modules": before, "features": before_features}, - "after": {"modules": canonical_modules, "features": sorted(canonical_features)}, - # Named explicitly: a feature removed as a consequence of a module going - # away is the kind of change an operator will otherwise discover only - # when something stops charging. - "features_dropped_with_modules": dropped_features, - }, + meta=meta, ) diff --git a/backend/apps/makerspaces/module_registry.py b/backend/apps/makerspaces/module_registry.py index f0341c86..1fd814f8 100644 --- a/backend/apps/makerspaces/module_registry.py +++ b/backend/apps/makerspaces/module_registry.py @@ -62,7 +62,7 @@ frontend_workflows=("printing_requests",), ), ModuleDefinition( - "telegram", "Telegram", "Per-makerspace Telegram group alerts and callbacks.", + "telegram", "Telegram", "Per-makerspace Telegram group alerts (outbound only).", "integrations", GUARD, group=GROUP_NOTIFICATIONS, frontend_workflows=("telegram_alerts",), ), ModuleDefinition( @@ -246,6 +246,27 @@ def _validate_registry(): raise ImproperlyConfiguredRegistry( f"{definition.key} is core, which already implies default_enabled." ) + # A core module is present in EVERY makerspace and cannot be switched off. If it + # declared a dependency on an optional key, either that key is really core (and + # is mislabelled), or the core module stops working the moment an operator + # uninstalls something they were told was optional. `9e496997` was that bug + # without the declaration: core `request_workflow` hard-required a + # MakerspaceMembership row, and the default `recommended` profile ships no + # `membership` module -- so a fresh self-host install could not accept a public + # borrow request at all. This catches the declared form of it at import time; + # `tests/makerspaces/test_core_module_independence.py` covers the undeclared form. + if definition.is_core: + optional_requirements = [ + key + for key in definition.requires_modules + if key in BY_KEY and not BY_KEY[key].is_core + ] + if optional_requirements: + raise ImproperlyConfiguredRegistry( + f"{definition.key} is core but requires optional module(s): " + f"{', '.join(sorted(optional_requirements))}. A core module must " + "work with every optional module uninstalled." + ) if definition.enforcement not in {GUARD, FEATURE_PARENT, NONE}: raise ImproperlyConfiguredRegistry( f"{definition.key} has unknown enforcement {definition.enforcement!r}." diff --git a/backend/apps/makerspaces/platform.py b/backend/apps/makerspaces/platform.py index 5c28c4e2..7c8f8ba8 100644 --- a/backend/apps/makerspaces/platform.py +++ b/backend/apps/makerspaces/platform.py @@ -8,6 +8,7 @@ from apps.makerspaces.servability import is_servable, servable_queryset from apps.makerspaces.capabilities import FEATURE_MODULES, FEATURES from apps.makerspaces.module_registry import is_frontend_exposed, module_available, module_workflows +from apps.makerspaces.request_access import ANYONE, effective_policy from apps.separability.registry import runtime_active # Derived from the module registry, which also decides frontend exposure: an @@ -185,6 +186,17 @@ def bootstrap_payload(makerspace): "public_stats_enabled": makerspace.public_stats_enabled, "membership_policy": makerspace.membership_policy, } + # Emitted ONLY for the opt-in `anyone` policy, following the `/api/v1/config` + # `member_accounts` precedent: every deployment that has not opted in keeps a + # byte-for-byte identical payload. Absent therefore means "an account is required", + # which is exactly what every client assumed before this policy existed. + # + # The public borrow form needs this: an account-less submission must collect contact + # details and send an Idempotency-Key, and without knowing the policy the client + # cannot tell whether to ask for them -- it would post a members-shaped body and take + # a 400. + if effective_policy(makerspace) == ANYONE: + makerspace_payload["request_access"] = ANYONE # Advisory geofence: expose the flag ONLY when configured AND the feature is on, so # dormant/self-host bootstrap payloads stay byte-for-byte unchanged (self-host # invariant) and a disabled feature cannot leave the client asking for coordinates diff --git a/backend/apps/makerspaces/request_access.py b/backend/apps/makerspaces/request_access.py new file mode 100644 index 00000000..eec65387 --- /dev/null +++ b/backend/apps/makerspaces/request_access.py @@ -0,0 +1,121 @@ +"""Who may submit a borrow request — the single answer, in one place. + +Three states, and they are DERIVED, not stored. Only ``anonymous_requests_enabled`` is a +column; the rest falls out of whether the ``membership`` module is installed: + +=========== ===================== ============================================ +`membership` `anonymous_requests_…` who may submit +=========== ===================== ============================================ +on off ``members`` — an active MakerspaceMembership +off off ``accounts`` — any active authenticated account +off on ``anyone`` — no account at all +on **on** impossible — see below +=========== ===================== ============================================ + +The fourth row is the reason this module exists. `RequestSubmitView` takes the anonymous +branch *before* it reaches any membership guard, so a row carrying both settings would let +a stranger walk straight past the membership requirement the operator had just switched on +— the opposite of what enabling `membership` means. Turning one on therefore turns the +other off, and it is enforced at three depths so no write path can reconstruct the state: + +1. ``Makerspace.save()`` calls :func:`reconcile_enabled_modules` — the model cannot be + persisted in the impossible state by ANY caller (module install/uninstall, profile + application, the ``/control/`` capability matrix, ``setup_instance``, ``seed_demo``, a + plain ``obj.save()``). +2. :func:`set_anonymous_requests` is the only *deliberate* writer, and it takes the row + lock so a concurrent membership install cannot land between its check and its save. +3. :func:`anonymous_requests_allowed` re-derives the answer at request time, so a row + written by raw SQL or restored from an old backup still fails closed. + +Forced, never refused: refusing a membership install because of an unrelated public-access +flag would block a legitimate module change from the console and the setup tick list. The +change is audited with the before/after policy so it is never silent. +""" + +MEMBERSHIP_MODULE = "membership" + +MEMBERS = "members" +ACCOUNTS = "accounts" +ANYONE = "anyone" + +POLICY_LABELS = { + MEMBERS: "active members of this makerspace", + ACCOUNTS: "anyone with a signed-in account", + ANYONE: "anyone, no account needed", +} + + +def membership_installed(enabled_modules) -> bool: + """Pure predicate over the stored key list. + + Deliberately NOT ``platform.module_enabled``: that also asks whether the deployment + still ships the app, and this is the fail-closed direction. A tenant that asked for + membership must not have strangers admitted just because the app is tombstoned, and + the model layer cannot import ``platform`` anyway (``platform`` imports the models). + """ + return MEMBERSHIP_MODULE in set(enabled_modules or []) + + +def reconcile_enabled_modules(enabled_modules, anonymous_requests_enabled) -> bool: + """The resulting ``anonymous_requests_enabled``. Membership wins.""" + return bool(anonymous_requests_enabled) and not membership_installed(enabled_modules) + + +def policy_for(enabled_modules, anonymous_requests_enabled) -> str: + if reconcile_enabled_modules(enabled_modules, anonymous_requests_enabled): + return ANYONE + return MEMBERS if membership_installed(enabled_modules) else ACCOUNTS + + +def effective_policy(makerspace) -> str: + """Who may submit to this makerspace right now.""" + return policy_for(makerspace.enabled_modules, makerspace.anonymous_requests_enabled) + + +def anonymous_requests_allowed(makerspace) -> bool: + """Request-time check. Re-derived rather than trusting the column alone.""" + return effective_policy(makerspace) == ANYONE + + +class RequestAccessConflict(Exception): + """Account-less requests were asked for while `membership` is installed.""" + + +def set_anonymous_requests(makerspace, enabled, *, actor=None): + """Deliberately set the flag under the row lock, and audit the policy change. + + Returns the effective policy AFTER the write. Raises :class:`RequestAccessConflict` + when asked to open account-less requests on a makerspace that has `membership` + installed — an explicit operator request is refused loudly rather than silently + downgraded, which is the opposite of the module-install path (that one forces, because + the operator was asking about modules, not about this flag). + """ + from django.db import transaction + + from apps.audit import services as audit + from apps.makerspaces.models import Makerspace + + with transaction.atomic(): + locked = Makerspace.objects.select_for_update().get(pk=makerspace.pk) + before = effective_policy(locked) + wanted = bool(enabled) + if wanted and membership_installed(locked.enabled_modules): + raise RequestAccessConflict( + "The membership module is installed, so borrow requests require an " + "account. Uninstall `membership` first, or leave account-less requests " + "off." + ) + if locked.anonymous_requests_enabled != wanted: + locked.anonymous_requests_enabled = wanted + locked.save(update_fields=["anonymous_requests_enabled"]) + after = effective_policy(locked) + if before != after: + audit.record( + actor, + "makerspace.request_access_changed", + makerspace=locked, + target=locked, + meta={"before": before, "after": after}, + ) + makerspace.refresh_from_db(fields=["anonymous_requests_enabled"]) + return after diff --git a/backend/apps/makerspaces/views.py b/backend/apps/makerspaces/views.py index 759271e9..549c5b74 100644 --- a/backend/apps/makerspaces/views.py +++ b/backend/apps/makerspaces/views.py @@ -33,6 +33,13 @@ "membership_policy": serializers.ChoiceField( choices=Makerspace.MembershipPolicy.choices ), + # `required=False` for the same reason as `geofence_enabled`: the key is + # emitted ONLY when the makerspace opted into account-less requests, so + # every other deployment keeps a byte-for-byte identical payload. Absent + # means an account is required. + "request_access": serializers.ChoiceField( + choices=[("anyone", "anyone")], required=False + ), }, ), "frontend": inline_serializer( diff --git a/backend/apps/openapi.py b/backend/apps/openapi.py index 0c66f22a..53f5d95b 100644 --- a/backend/apps/openapi.py +++ b/backend/apps/openapi.py @@ -38,7 +38,7 @@ PUBLIC_REQUEST_SUBMIT_EXAMPLE = OpenApiExample( "Submit public equipment request", value={ - "requester_name": "Shaan Shoukath", + "contact_name": "Shaan Shoukath", "contact_email": "shaans@example.com", "contact_phone": "+919876543210", "requested_for": "Electronics workshop diagnostics", diff --git a/backend/apps/operations/accountability.py b/backend/apps/operations/accountability.py index 9200fb72..5185b494 100644 --- a/backend/apps/operations/accountability.py +++ b/backend/apps/operations/accountability.py @@ -3,20 +3,44 @@ from django.utils import timezone from apps.accounts.models import User +from apps.hardware_requests.display import requester_label from apps.hardware_requests.models import ( HardwareRequest, PublicProblemReport, PublicToolLoan, RequesterAccountability, ) +from apps.makerspaces.anonymous_requesters import anonymous_requester_ids + + +def _anonymous_accountability(makerspace_id, principals): + """Damage and loss recorded against account-less requests, as one total. + + Not a ranking row and deliberately not shaped like one: there is no person to name, + contact or restrict here, only a count of what strangers did not bring back. + """ + totals = RequesterAccountability.objects.filter( + makerspace_id=makerspace_id, requester_id__in=principals + ).aggregate( + damaged=Count("id", filter=Q(issue_type=RequesterAccountability.IssueType.DAMAGED)), + missing=Count("id", filter=Q(issue_type=RequesterAccountability.IssueType.MISSING)), + total_issues=Count("id"), + total_quantity=Coalesce(Sum("quantity"), 0), + ) + return {key: value or 0 for key, value in totals.items()} def accountability_data(makerspace_id, *, limit=200): - repeat_offenders, repeat_truncated = _repeat_offenders(makerspace_id, limit) + principals = anonymous_requester_ids([makerspace_id]) + repeat_offenders, repeat_truncated = _repeat_offenders(makerspace_id, limit, principals) overdue, overdue_truncated = _overdue_loans(makerspace_id, limit) problem_reports, problems_truncated = _problem_reports(makerspace_id, limit) return { "repeat_offenders": repeat_offenders, + # The damage and loss that the excluded principal was carrying, kept as a total + # rather than dropped: an accountability panel that silently lost every + # account-less incident would be worse than one that named a fictional person. + "anonymous_accountability": _anonymous_accountability(makerspace_id, principals), "overdue": overdue, "restrictions": _restricted_requesters(makerspace_id), "problem_reports": problem_reports, @@ -57,9 +81,12 @@ def _problem_reports(makerspace_id, limit): ], len(rows) > limit -def _repeat_offenders(makerspace_id, limit): +def _repeat_offenders(makerspace_id, limit, principals): rows = list( + # Excluded BEFORE the limit, not after: filtering a materialized page would let + # the principal push a real repeat offender off the end of the list. RequesterAccountability.objects.filter(makerspace_id=makerspace_id) + .exclude(requester_id__in=principals) .values( "requester_id", "requester__username", @@ -125,7 +152,10 @@ def _overdue_requests(makerspace_id, now, limit): { "type": "request", "reference_id": request.id, - "requester_username": request.requester.username, + # `requester_label` prefers the request's own contact snapshot, so an + # account-less overdue loan shows the email the borrower actually gave + # instead of the principal's internal `member_` username. + "requester_username": requester_label(request), "label": _request_label(request), "due_at": request.return_due_at, } @@ -160,6 +190,10 @@ def _request_label(request): def _restricted_requesters(makerspace_id): + # The principal can never legitimately appear here -- restrict/restore both refuse it + # via `refuse_anonymous_requester_access_mutation` -- but it is excluded anyway, so a + # row created before that guard, or by a direct DB edit, cannot render as a person. + principals = anonymous_requester_ids([makerspace_id]) return [ { "requester_id": user.id, @@ -167,7 +201,7 @@ def _restricted_requesters(makerspace_id): "access_status": user.access_status, "restriction_reason": user.restriction_reason, } - for user in User.objects.filter( + for user in User.objects.exclude(pk__in=principals).filter( accountability_records__makerspace_id=makerspace_id, access_status__in=[ User.AccessStatus.RESTRICTED, diff --git a/backend/apps/operations/org_report_identity.py b/backend/apps/operations/org_report_identity.py index d3d3a9ef..0efba82f 100644 --- a/backend/apps/operations/org_report_identity.py +++ b/backend/apps/operations/org_report_identity.py @@ -5,6 +5,7 @@ from apps.hardware_requests.display import label_from_candidates from apps.hardware_requests.models import HardwareRequest, HardwareRequestItem +from apps.makerspaces.anonymous_requesters import anonymous_requester_ids from apps.makerspaces.models import MakerspaceMembership, MembershipRequest from apps.operations.report_scope import ReportScope, scope_queryset @@ -19,6 +20,10 @@ def globally_ranked_borrowers(scope: ReportScope, *, date_range, limit): makerspace_field="request__makerspace_id", ) items = _range(items, "request__issued_at", date_range) + # Same reason as the per-space ranking: the shared account-less principal is not a + # person and must not occupy a rank. Excluded before the slice so it cannot displace + # a real borrower from the top `limit`. + items = items.exclude(request__requester_id__in=anonymous_requester_ids()) ranked = list( items.values("request__requester_id") .annotate( diff --git a/backend/apps/operations/reports_inventory.py b/backend/apps/operations/reports_inventory.py index de8273dd..0e388583 100644 --- a/backend/apps/operations/reports_inventory.py +++ b/backend/apps/operations/reports_inventory.py @@ -1,6 +1,7 @@ from django.db.models import Count, Sum from apps.boxes.models import QrScanEvent +from apps.makerspaces.anonymous_requesters import anonymous_requester_ids from apps.hardware_requests.display import label_from_candidates, requester_label from apps.hardware_requests.models import HardwareRequest, HardwareRequestItem from apps.inventory.models import InventoryAsset, InventoryProduct @@ -179,6 +180,10 @@ def _top_borrowers(makerspace_id, aggregate, limit=None, date_range=None): qs = ( _apply_date_range(_items(makerspace_id), "request__issued_at", date_range) .filter(issued_quantity__gt=0) + # Every account-less request in a space shares ONE requester principal, so + # without this they rank as a single fictional "top borrower". Excluded before + # the grouping so the ranking and any limit are computed over real people only. + .exclude(request__requester_id__in=anonymous_requester_ids()) .values(*values) .annotate( requests=Count("request_id", distinct=True), diff --git a/backend/apps/presence/guard.py b/backend/apps/presence/guard.py index 21f6194e..b0a13481 100644 --- a/backend/apps/presence/guard.py +++ b/backend/apps/presence/guard.py @@ -2,8 +2,8 @@ from django.utils import timezone -from apps.accounts.models import User from apps.accounts.claim_sessions import claim_context +from apps.accounts.models import User from apps.makerspaces.models import MakerspaceMembership, MakerspaceWaiver from apps.makerspaces.servability import is_servable from apps.makerspaces.waiver_state import current_acceptance @@ -28,12 +28,26 @@ class PresenceRequired(Exception): @dataclass(frozen=True) class ActiveMemberPresence: - membership: MakerspaceMembership + # Membership is absent only for the request-proposal path when the makerspace has + # deliberately disabled its community membership module. + membership: MakerspaceMembership | None accepted_waiver: MakerspaceWaiver | None - # None only when the deployment has tombstoned check-in; no caller reads it. + # Also None for the identity-only request path; no current caller reads it there. session: PresenceSession | None +def require_active_account(user, makerspace): + """Require a servable makerspace and an unrestricted authenticated account. + + This is deliberately narrower than `require_active_member`: public borrow requests + are proposals that staff must accept, so a makerspace without the community + membership module can admit them without inventing a tenant-binding membership row. + Hardware and facility actions must keep using the member guards below. + """ + _require_active_identity(user, makerspace) + return ActiveMemberPresence(None, None, None) + + def require_active_member(user, makerspace): """Identity, membership and waiver -- the half that is not about being here. @@ -49,14 +63,7 @@ def require_active_member(user, makerspace): Keeping it as the single implementation of the membership and waiver rules is the point: two copies would drift, and the copy that drifted would be an auth rule. """ - if not is_servable(makerspace) or not ( - user - and user.is_authenticated - and user.pk - and user.is_active - and user.access_status == User.AccessStatus.ACTIVE - ): - raise MemberPresenceRequired() + _require_active_identity(user, makerspace) membership = MakerspaceMembership.objects.filter( user=user, makerspace=makerspace, status="active" ).select_related("accepted_waiver").first() @@ -73,6 +80,17 @@ def require_active_member(user, makerspace): return ActiveMemberPresence(membership, waiver, None) +def _require_active_identity(user, makerspace): + if not is_servable(makerspace) or not ( + user + and user.is_authenticated + and user.pk + and user.is_active + and user.access_status == User.AccessStatus.ACTIVE + ): + raise MemberPresenceRequired() + + def require_active_member_presence(user, makerspace): """Membership, then waiver, then an open check-in session. @@ -81,13 +99,15 @@ def require_active_member_presence(user, makerspace): changes behaviour instead of only removing a surface, so it is worth being explicit about why. - Six member-facing surfaces call this as a bare precondition -- self-checkout, staff - direct handout, public request submit, public booking, and the two public - machine-service surfaces. A deployment that does not ship check-in has no session for - any of them to find, so leaving the requirement hard would not make those flows - stricter, it would make every one of them refuse forever. That is a broken install, - which is exactly the outcome `separability.E007` exists to reject; a tombstone is - supposed to yield a smaller system, not a stuck one. + Member-facing hardware and facility surfaces call this as a bare precondition -- + self-checkout, staff direct handout, public booking, and the two public + machine-service surfaces. Public request submission also calls it while the + membership module is enabled; with that module off, the staff-reviewed proposal + path uses `require_active_account` instead. A deployment that does not ship check-in + has no session for any guarded surface to find, so leaving the requirement hard + would not make those flows stricter, it would make every one of them refuse forever. + That is a broken install, which is exactly the outcome `separability.E007` exists to + reject; a tombstone is supposed to yield a smaller system, not a stuck one. Membership and the waiver are still enforced, so the identity and liability factors are untouched, and so are the Hard Rules' non-negotiables (a box QR scan and an diff --git a/backend/apps/tenant_migration/gate_policy.py b/backend/apps/tenant_migration/gate_policy.py index d063e670..cd134e60 100644 --- a/backend/apps/tenant_migration/gate_policy.py +++ b/backend/apps/tenant_migration/gate_policy.py @@ -19,6 +19,12 @@ ), "backup-recovery-state": "Deployment recovery must be able to quarantine/recover the deployment.", "stripe-connect-webhook": "Connect account events are platform routing state, not one source tenant.", + "telegram-webhook": ( + "Acknowledge-and-discard: the callback route was removed when chat stopped being an " + "action surface, so this writes nothing and cannot break quiescence. It must keep " + "answering 200 even mid-migration, or Telegram retries an already-registered webhook " + "for hours against a tenant that is being moved." + ), "auth-login": "Global user session state is excluded from tenant quiescence.", "auth-refresh": "Global user session state is excluded from tenant quiescence.", "auth-logout": "Global user session state is excluded from tenant quiescence.", @@ -64,6 +70,11 @@ "apps.accounts.views_social.SocialNonceView.post": "Global social-login nonce state.", "apps.accounts.views_social.SocialLoginView.post": "Global social identity and session state.", "apps.payments.views_connect.StripeConnectWebhookView.post": "Platform Connect routing state.", + "apps.integrations.views.TelegramWebhookView.post": ( + "Writes nothing at all: the callback route was removed when chat stopped being an " + "action surface, and the view only acknowledges so an already-registered webhook " + "stops retrying. No tenant state is reachable from it to quiesce." + ), } diff --git a/backend/apps/tenant_migration/source_gate_guards.py b/backend/apps/tenant_migration/source_gate_guards.py index 3a4afaaf..400bbab2 100644 --- a/backend/apps/tenant_migration/source_gate_guards.py +++ b/backend/apps/tenant_migration/source_gate_guards.py @@ -113,6 +113,8 @@ def validate_webhook_coverage(apps_dir=APPS_DIR): exemptions = { "apps.payments.views_connect.StripeConnectWebhookView.post": HTTP_EXEMPTIONS["stripe-connect-webhook"], + "apps.integrations.views.TelegramWebhookView.post": + HTTP_EXEMPTIONS["telegram-webhook"], } stale = set(exemptions) - actual if stale: diff --git a/backend/apps/tenant_migration/tenant_dump_catalog.py b/backend/apps/tenant_migration/tenant_dump_catalog.py index fe4253a1..646fe59c 100644 --- a/backend/apps/tenant_migration/tenant_dump_catalog.py +++ b/backend/apps/tenant_migration/tenant_dump_catalog.py @@ -38,7 +38,7 @@ class TenantDumpCatalogError(AssertionError): # SHA-256 of the ordered model/table/field graph produced by ``catalog_schema``. # Updating it is an explicit review act; runtime introspection never blesses drift. -CATALOG_SCHEMA_SHA256 = "0c8a32be94c4f8a4a862818470ae216bbf30ecf5cf9acb201d8d6a8615fd4bf3" +CATALOG_SCHEMA_SHA256 = "2f17b431d479fbbb508361dae0ebd465cc87163946bf64e2efeead65557d0188" def catalog_models(apps_registry=apps): diff --git a/backend/apps/tenant_migration/tenant_dump_field_snapshot.py b/backend/apps/tenant_migration/tenant_dump_field_snapshot.py index a4903ec8..d754a9c4 100644 --- a/backend/apps/tenant_migration/tenant_dump_field_snapshot.py +++ b/backend/apps/tenant_migration/tenant_dump_field_snapshot.py @@ -23,7 +23,7 @@ 'accounts.SocialLoginNonce': frozenset('attestation_challenge client_platform consumed_at created_at delivery device_grant expires_at id nonce_digest origin provider surface'.split()), 'accounts.PlatformSocialAuthSettings': frozenset('apple_key_id apple_native_app_ids apple_private_key apple_service_id apple_team_id google_android_client_id google_ios_client_id google_web_client_id id updated_at'.split()), 'accounts.OidcProvider': frozenset('allow_auto_link client_id created_at display_name id is_enabled issuer jwks_url slug updated_at'.split()), - 'makerspaces.Makerspace': frozenset('archived_at archived_by booking_requester_notifications_enabled branding_config cors_allowed_origins cover_image_key created_at created_by default_loan_days discord_webhook_url domain_verification_token domain_verified_at enabled_features enabled_modules filament_low_stock_threshold_grams frontend_domain frontend_domain_changed_at frontend_domain_status geofence_enabled geofence_latitude geofence_longitude geofence_radius_m hidden_from_central_directory id lifecycle_state location logo_key map_url mattermost_webhook_url membership_dues_amount membership_policy name presence_preset_minutes public_api_key public_code public_inventory_enabled public_print_status_lookup_policy public_stats_enabled public_stats_show_holder_names referrals_enabled resource_limit_overrides slack_webhook_url slug smtp_from_email smtp_host smtp_password smtp_port smtp_use_ssl smtp_use_tls smtp_username staff_notifications_enabled storage_bytes_used superadmin_access_enabled telegram_bot_token telegram_group_chat_id theme_config updated_at'.split()), + 'makerspaces.Makerspace': frozenset('anonymous_requester anonymous_requests_enabled archived_at archived_by booking_requester_notifications_enabled branding_config cors_allowed_origins cover_image_key created_at created_by default_loan_days discord_webhook_url domain_verification_token domain_verified_at enabled_features enabled_modules filament_low_stock_threshold_grams frontend_domain frontend_domain_changed_at frontend_domain_status geofence_enabled geofence_latitude geofence_longitude geofence_radius_m hidden_from_central_directory id lifecycle_state location logo_key map_url mattermost_webhook_url membership_dues_amount membership_policy name presence_preset_minutes public_api_key public_code public_inventory_enabled public_print_status_lookup_policy public_stats_enabled public_stats_show_holder_names referrals_enabled resource_limit_overrides slack_webhook_url slug smtp_from_email smtp_host smtp_password smtp_port smtp_use_ssl smtp_use_tls smtp_username staff_notifications_enabled storage_bytes_used superadmin_access_enabled telegram_bot_token telegram_group_chat_id theme_config updated_at'.split()), 'makerspaces.MakerspaceMembership': frozenset('accepted_waiver activated_actor_snapshot activated_at activated_by assigned_role can_refer can_verify created_at id makerspace receives_notifications revocation_reason revoked_actor_snapshot revoked_at revoked_by role status user verified_actor_snapshot verified_at verified_by waiver_accepted_at waiver_version_accepted witnessed_actor_snapshot witnessed_at witnessed_by witnessed_waiver witnessed_waiver_version'.split()), 'makerspaces.MakerspaceRole': frozenset('created_at granted_actions id is_default is_protected legacy_role makerspace name slug updated_at'.split()), 'makerspaces.MakerspaceWaiver': frozenset('body created_at created_by id is_active makerspace superseded_at version'.split()), @@ -58,7 +58,7 @@ 'inventory.Category': frozenset('created_at display_order icon id makerspace name slug updated_at'.split()), 'inventory.InventoryProduct': frozenset('available_quantity box category created_at damaged_quantity description id image_key is_archived is_public issued_quantity lost_quantity makerspace name needs_fix_quantity public_availability_mode public_self_checkout_enabled reserved_quantity show_public_count storage_location total_quantity tracking_mode updated_at'.split()), 'inventory.InventoryAsset': frozenset('asset_tag box created_at id makerspace notes product public_self_checkout_enabled serial_number status updated_at'.split()), - 'hardware_requests.HardwareRequest': frozenset('accepted_at accepted_by assigned_box closed_at closed_by created_at id issue_evidence issue_remark issued_at issued_by makerspace public_token rejection_reason requested_for requester requester_contact_email requester_contact_phone requester_name requester_username return_due_at return_reminder_sent_at status updated_at'.split()), + 'hardware_requests.HardwareRequest': frozenset('accepted_at accepted_by anonymous_idempotency_key_fingerprint anonymous_payload_fingerprint assigned_box closed_at closed_by created_at id issue_evidence issue_remark issued_at issued_by makerspace public_token rejection_reason requested_for requester requester_contact_email requester_contact_phone requester_contact_verified requester_name requester_username return_due_at return_reminder_sent_at status updated_at'.split()), 'hardware_requests.HardwareRequestItem': frozenset('accepted_quantity damaged_quantity id issued_quantity missing_quantity needs_fix_quantity product request requested_quantity returned_quantity'.split()), 'hardware_requests.ReturnEvent': frozenset('actor box created_at evidence id makerspace remark request'.split()), 'hardware_requests.RequesterAccountability': frozenset('created_at created_by description evidence_photo id issue_type makerspace quantity request request_item requester'.split()), diff --git a/backend/apps/tenant_migration/unique_values.py b/backend/apps/tenant_migration/unique_values.py index 561b1172..cfcd8b0d 100644 --- a/backend/apps/tenant_migration/unique_values.py +++ b/backend/apps/tenant_migration/unique_values.py @@ -208,6 +208,12 @@ def _warranty_document_key(row, target, source_value): field="object_key", generator=_maintenance_document_key, ), + ("makerspaces.Makerspace", "field:anonymous_requester"): _policy( + NULL, + "The anonymous-request principal is a per-deployment system row, not a person: " + "the target creates its own lazily on the first account-less request, so importing " + "the source's would carry an inert User the target must never resolve to a human.", + ), ("makerspaces.Makerspace", "field:public_code"): _policy( UniqueValueDisposition.TARGET_CREATION, "Target creation preserves or regenerates the public code.", diff --git a/backend/config/settings.py b/backend/config/settings.py index b45510d1..46e4c8e1 100644 --- a/backend/config/settings.py +++ b/backend/config/settings.py @@ -102,6 +102,20 @@ def normalize_platform_domain_suffix(raw): # by the operator's SMS vendor, so this is a cost ceiling rather than a fair-use quota. # Blank disables the cap entirely. OTP_SMS_DAILY_CAP = env.int("OTP_SMS_DAILY_CAP", default=200) +ANONYMOUS_REQUEST_OUTSTANDING_LIMIT = env.int( + "ANONYMOUS_REQUEST_OUTSTANDING_LIMIT", + default=50, +) +ANONYMOUS_REQUEST_IDEMPOTENCY_KEY_MAX_LENGTH = env.int( + "ANONYMOUS_REQUEST_IDEMPOTENCY_KEY_MAX_LENGTH", + default=128, +) +if ANONYMOUS_REQUEST_OUTSTANDING_LIMIT < 1: + raise ImproperlyConfigured("ANONYMOUS_REQUEST_OUTSTANDING_LIMIT must be positive.") +if ANONYMOUS_REQUEST_IDEMPOTENCY_KEY_MAX_LENGTH < 1: + raise ImproperlyConfigured( + "ANONYMOUS_REQUEST_IDEMPOTENCY_KEY_MAX_LENGTH must be positive." + ) STORAGE_PRESIGN_METHOD = env("STORAGE_PRESIGN_METHOD", default="post") CRON_SECRET = env("CRON_SECRET", default="") ADMIN_SITE_NAME = env("ADMIN_SITE_NAME", default="Space Works") @@ -764,6 +778,18 @@ def cache_config(cache_url): "THROTTLE_PUBLIC_REQUEST_SUBMIT", default="10/min", ), + "anonymous_request_ip_burst": env( + "THROTTLE_ANONYMOUS_REQUEST_IP_BURST", + default="2/min", + ), + "anonymous_request_ip_hour": env( + "THROTTLE_ANONYMOUS_REQUEST_IP_HOUR", + default="10/hour", + ), + "anonymous_request_email": env( + "THROTTLE_ANONYMOUS_REQUEST_EMAIL", + default="3/day", + ), "print_request_submit": env("THROTTLE_PRINT_REQUEST_SUBMIT", default="10/min"), "public_tool_checkout": env("THROTTLE_PUBLIC_TOOL_CHECKOUT", default="10/min"), "public_tool_return": env("THROTTLE_PUBLIC_TOOL_RETURN", default="10/min"), @@ -933,7 +959,7 @@ def cache_config(cache_url): # `tests/test_version_consistency.py`. It cannot simply READ that file: the backend # image is built with `context: ./backend`, so the repo root is outside the build # context and the file does not exist inside the container. - "VERSION": "0.7.5", + "VERSION": "0.8.0", "ENUM_NAME_OVERRIDES": { "QrPrintBatchStatusEnum": [ ("draft", "Draft"), diff --git a/backend/templates/admin/hardware_requests/reject_action.html b/backend/templates/admin/hardware_requests/reject_action.html deleted file mode 100644 index 1c40dc59..00000000 --- a/backend/templates/admin/hardware_requests/reject_action.html +++ /dev/null @@ -1,22 +0,0 @@ -{% extends "admin/base_site.html" %} - -{% block content %} -
- {% csrf_token %} - - {% for obj in queryset %} - - {% endfor %} - -
-
- - -
-
- -
- -
-
-{% endblock %} diff --git a/backend/templates/admin/hardware_requests/review.html b/backend/templates/admin/hardware_requests/review.html new file mode 100644 index 00000000..78ea0982 --- /dev/null +++ b/backend/templates/admin/hardware_requests/review.html @@ -0,0 +1,76 @@ +{% extends "admin/base_site.html" %} + +{% block content %} +
+

Requester

+ + + + + + + + + + + + +
Name{{ hardware_request.requester_name|default:"—" }}
Email{{ hardware_request.requester_contact_email|default:"—" }}
Phone{{ hardware_request.requester_contact_phone|default:"—" }}
Contact + {% if hardware_request.requester_contact_verified %} + Verified + {% else %} + NOT VERIFIED — this contact is only a claim made by whoever + submitted the request. Nothing proves the address or number belongs to them, and + accepting does not verify it. Confirm identity before handing anything over. + {% endif %} +
Makerspace{{ hardware_request.makerspace }}
Status{{ hardware_request.status }}
Requested for{{ hardware_request.requested_for|default:"—" }}
Submitted{{ hardware_request.created_at }}
+
+ +{% if is_pending %} +
+ {% csrf_token %} + +
+

Items

+ + + + + + {% for item in items %} + + + + + + {% empty %} + + {% endfor %} + +
ProductRequestedAccept
{{ item.product.name }}{{ item.requested_quantity }} + +
This request has no items.
+
+ +
+

Reject instead

+
+ + +
+
+ +
+ + +
+
+{% else %} +

+ This request is {{ hardware_request.status }}, so there is nothing left + to review. Accept and reject apply to pending requests only. +

+{% endif %} +{% endblock %} diff --git a/backend/tests/backup/test_archive_digests.py b/backend/tests/backup/test_archive_digests.py index 68beb5c3..98b9a30e 100644 --- a/backend/tests/backup/test_archive_digests.py +++ b/backend/tests/backup/test_archive_digests.py @@ -100,7 +100,17 @@ def snapshot(_archive, root, _modes, selected_recipients): "storage": {"objects": objects}, } + real_run = archive_builder.subprocess.run + def encrypt(command, **_kwargs): + if "-o" not in command: + # Not an age invocation. This fake replaces the SHARED subprocess module, so + # every other binary the code runs lands here too -- including + # `postgres_client._binary_major`'s ` --version` probe. Delegate + # instead of stubbing: a bare SimpleNamespace has no `.stdout`, which turned a + # clean PostgresClientUnavailable into an AttributeError on every host without + # a versioned client directory (any non-Debian/RHEL host). + return real_run(command, **_kwargs) output = Path(command[command.index("-o") + 1]) shutil.copyfile(command[-1], output) return SimpleNamespace(returncode=0) diff --git a/backend/tests/backup/test_archive_recipient_selection.py b/backend/tests/backup/test_archive_recipient_selection.py index d2fd333c..0812b41e 100644 --- a/backend/tests/backup/test_archive_recipient_selection.py +++ b/backend/tests/backup/test_archive_recipient_selection.py @@ -83,8 +83,18 @@ def snapshot(_archive, root, _modes, selected): monkeypatch.setattr(archive_builder, "_snapshot_payload", snapshot) calls = [] + real_run = archive_builder.subprocess.run + def encrypt(args, **_kwargs): calls.append(args) + if "-o" not in args: + # Not an age invocation. This fake replaces the SHARED subprocess module, so + # every other binary the code runs lands here too -- including + # `postgres_client._binary_major`'s ` --version` probe. Delegate + # instead of stubbing: a bare SimpleNamespace has no `.stdout`, which turned a + # clean PostgresClientUnavailable into an AttributeError on every host without + # a versioned client directory (any non-Debian/RHEL host). + return real_run(args, **_kwargs) shutil.copyfile(args[-1], args[args.index("-o") + 1]) return SimpleNamespace(returncode=0) diff --git a/backend/tests/backup/test_compound_archive_e2.py b/backend/tests/backup/test_compound_archive_e2.py index c7feb508..0f1c2dc8 100644 --- a/backend/tests/backup/test_compound_archive_e2.py +++ b/backend/tests/backup/test_compound_archive_e2.py @@ -116,12 +116,18 @@ def tenant_payload(makerspace_id, root): monkeypatch.setattr(archive_payload, "_tenant_payload", tenant_payload) commands = [] + real_run = archive_builder.subprocess.run def fake_age(command, **_kwargs): commands.append(command) if "-o" not in command: - # Not an age invocation (E7 added other subprocess calls); nothing to seal. - return SimpleNamespace(returncode=0) + # Not an age invocation. This fake replaces the SHARED subprocess module, so + # every other binary the code runs lands here too -- including + # `postgres_client._binary_major`'s ` --version` probe. Delegate + # instead of stubbing: a bare SimpleNamespace has no `.stdout`, which turned a + # clean PostgresClientUnavailable into an AttributeError on every host without + # a versioned client directory (any non-Debian/RHEL host). + return real_run(command, **_kwargs) output = Path(command[command.index("-o") + 1]) payload = _kwargs.get("input") if payload is None: diff --git a/backend/tests/backup/test_producer_capability_gate_p1.py b/backend/tests/backup/test_producer_capability_gate_p1.py index 86ab7447..4ea82f5f 100644 --- a/backend/tests/backup/test_producer_capability_gate_p1.py +++ b/backend/tests/backup/test_producer_capability_gate_p1.py @@ -239,7 +239,17 @@ def snapshot(_archive, root, _modes, _selected): (root / "database.dump").write_bytes(b"ordinary backup") return {"format": "spaceworks-phase5a-v3", "storage": {"objects": []}} + real_run = archive_builder.subprocess.run + def encrypt(command, **_kwargs): + if "-o" not in command: + # Not an age invocation. This fake replaces the SHARED subprocess module, so + # every other binary the code runs lands here too -- including + # `postgres_client._binary_major`'s ` --version` probe. Delegate + # instead of stubbing: a bare SimpleNamespace has no `.stdout`, which turned a + # clean PostgresClientUnavailable into an AttributeError on every host without + # a versioned client directory (any non-Debian/RHEL host). + return real_run(command, **_kwargs) shutil.copyfile(command[-1], command[command.index("-o") + 1]) return SimpleNamespace(returncode=0) diff --git a/backend/tests/encryption/test_rollout.py b/backend/tests/encryption/test_rollout.py index 7e548a2e..a69154c1 100644 --- a/backend/tests/encryption/test_rollout.py +++ b/backend/tests/encryption/test_rollout.py @@ -132,7 +132,12 @@ def test_closed_global_fence_rejects_orm_bulk_and_raw_bypass_paths(): def test_rollback_rejects_an_overflow_without_touching_source_or_index(): actor = _actor() with enabled_encryption(): - space, user, row = _request("x" * 121) + # Must exceed the REGISTRY limit for this field (`registry.SOURCE_FIELDS` declares + # requester_name at 200), not the column: the model field is a TextField, so the + # database would accept any length and `validate_legacy_values` is the only thing + # standing between a rollback and a silently truncated legacy row. This read 121 + # while the registered limit was 120, and went quiet when the limit moved to 200. + space, user, row = _request("x" * 201) operation = close_global("decrypt_rollback", actor.pk, all_makerspaces=True) with pytest.raises(ValidationError): call_command("decrypt_scoped_pii", makerspace=space.pk, model=row._meta.label, diff --git a/backend/tests/encryption/test_write_fence.py b/backend/tests/encryption/test_write_fence.py index 0e163d31..31f0337a 100644 --- a/backend/tests/encryption/test_write_fence.py +++ b/backend/tests/encryption/test_write_fence.py @@ -213,7 +213,17 @@ def test_mapped_service_paths_fail_with_the_typed_503_exception(monkeypatch): ) with pytest.raises(PiiWriteFenced): request_workflow.submit_request( - space, [], requester=actor + space, + [], + requester_principal=actor, + contact_snapshot=request_workflow.RequesterSnapshot( + username=actor.username, + name=actor.display_name, + email=actor.email, + phone=actor.phone, + contact_verified=True, + ), + audit_actor=actor, ) with pytest.raises(PiiWriteFenced): service_workflow.submit( diff --git a/backend/tests/makerspaces/test_core_module_independence.py b/backend/tests/makerspaces/test_core_module_independence.py new file mode 100644 index 00000000..e1164eb8 --- /dev/null +++ b/backend/tests/makerspaces/test_core_module_independence.py @@ -0,0 +1,194 @@ +"""The core loan spine must survive any single optional module being uninstalled. + +**What this proves, precisely.** For every optional module `M`, a makerspace built from +`core + (every other optional module)` can still run the loan spine end to end: browse the +public catalogue, submit a borrow request, see it in the staff queue, accept it, and read +its public status. Plus the strongest single case — `core` and nothing else. + +**What it does NOT prove.** It is not a general proof that "no core module hard-depends on +an optional module's data". A true static proof is not available in this codebase: an app +label is not the unit of module ownership (`hardware_requests` owns core `request_workflow` +AND optional `guest_handover`; `admin_api` owns core `staff_admin` and many optional +surfaces), so no per-file rule can separate a legitimate optional-module gate from a core +module reaching for optional data. The declared half of the property is checked at import +time instead, by `module_registry._validate_registry` (a core module may not name an +optional one in `requires_modules`). + +This is the regression that `9e496997` shipped and nothing caught: core `request_workflow` +called `require_active_member_presence` unconditionally, that guard hard-requires a +`MakerspaceMembership` row, and the default `recommended` profile installs no `membership` +module — so a fresh self-host install returned 403 `membership_required` to every public +borrow request. The `no_membership` case below fails without that fix. + +**The identity is built to match each configuration, and that is load-bearing.** With +`membership` installed the spine legitimately requires an active member with an open +presence session; a test that always used a plain account would be rejected by the +membership guard in 25 of 26 cases and would prove nothing about the module under test. +""" + +from datetime import timedelta + +import pytest +from django.urls import reverse +from django.utils import timezone +from rest_framework.test import APIClient + +from apps.accounts.models import User +from apps.inventory.models import InventoryProduct +from apps.makerspaces.models import Makerspace, MakerspaceMembership +from apps.makerspaces.module_registry import BY_KEY, MODULES, core_module_keys +from apps.presence.models import PresenceSession + +pytestmark = pytest.mark.django_db + +CORE = frozenset(core_module_keys()) +OPTIONAL = tuple(sorted(key for key in BY_KEY if key not in CORE)) + + +def transitive_dependents(key): + """Every module that would break if `key` were removed, at any depth. + + `module_registry_helpers.dependents_of` answers one level only. Removing `machines` + while leaving `printing` installed would produce a capability set + `validate_capabilities` rejects, so the closure is what makes each configuration in + this matrix a legal one. + """ + removed = {key} + changed = True + while changed: + changed = False + for definition in MODULES: + if definition.key in removed: + continue + if removed & set(definition.requires_modules): + removed.add(definition.key) + changed = True + return removed - {key} + + +def configuration_without(key): + dropped = {key, *transitive_dependents(key)} + return sorted(CORE | ({*OPTIONAL} - dropped)) + + +def _space(slug, modules): + return Makerspace.objects.create( + name=slug, + slug=slug, + enabled_modules=list(modules), + public_inventory_enabled=True, + ) + + +def _requester(slug, space): + """An account that is legitimately allowed to submit under THIS configuration.""" + user = User.objects.create_user( + username=f"cmi-{slug}", + email=f"cmi-{slug}@example.test", + display_name="Spine Requester", + access_status=User.AccessStatus.ACTIVE, + ) + if "membership" in (space.enabled_modules or []): + # Membership installed => the spine legitimately demands an active member with an + # open presence session. No MakerspaceWaiver row is created, so the waiver branch + # of `require_active_member` is not exercised here; that rule has its own tests. + membership = MakerspaceMembership.objects.create( + makerspace=space, user=user, status="active", + ) + now = timezone.now() + PresenceSession.objects.create( + member=user, + makerspace=space, + membership=membership, + started_at=now, + expires_at=now + timedelta(hours=2), + ) + return user + + +def _staff(slug): + return User.objects.create_user( + username=f"cmi-staff-{slug}", + email=f"cmi-staff-{slug}@example.test", + role=User.Role.SUPERADMIN, + is_staff=True, + is_superuser=True, + access_status=User.AccessStatus.ACTIVE, + ) + + +def _client(user=None): + client = APIClient() + if user is not None: + client.force_authenticate(user) + return client + + +def run_loan_spine(slug, modules): + """Browse -> submit -> staff queue -> accept -> public status. Returns nothing; + every step asserts, so the failing step names itself.""" + space = _space(slug, modules) + product = InventoryProduct.objects.create( + makerspace=space, + name="Torque wrench", + total_quantity=3, + available_quantity=3, + is_public=True, + ) + + catalog = _client().get(reverse("inventory:public-inventory", args=[space.slug])) + assert catalog.status_code == 200, f"catalog: {catalog.status_code} {catalog.data}" + + submit = _client(_requester(slug, space)).post( + reverse("hardware_requests:request-submit", args=[space.slug]), + {"requested_for": "Spine check", "items": [{"product_id": product.pk, "quantity": 1}]}, + format="json", + ) + assert submit.status_code == 201, f"submit: {submit.status_code} {submit.data}" + public_token = submit.data["public_token"] + + staff = _client(_staff(slug)) + pending = staff.get(reverse("hardware_requests:pending-requests", args=[space.id])) + assert pending.status_code == 200, f"queue: {pending.status_code} {pending.data}" + assert pending.data["count"] == 1, pending.data + + request_id = pending.data["results"][0]["id"] + accepted = staff.post(reverse("hardware_requests:request-accept", args=[request_id]), {}, format="json") + assert accepted.status_code == 200, f"accept: {accepted.status_code} {accepted.data}" + assert accepted.data["status"] == "accepted" + + status_response = _client().get( + reverse("hardware_requests:request-status", args=[public_token]) + ) + assert status_response.status_code == 200, f"status: {status_response.status_code}" + + +def test_the_loan_spine_runs_on_a_core_only_makerspace(): + """The strongest single case: every optional module uninstalled at once.""" + run_loan_spine("core-only", sorted(CORE)) + + +@pytest.mark.parametrize("missing", OPTIONAL) +def test_the_loan_spine_survives_each_optional_module_being_uninstalled(missing): + run_loan_spine(f"no-{missing.replace('_', '-')}", configuration_without(missing)) + + +def test_every_optional_module_is_actually_covered_by_the_matrix(): + """Guards the guard: a module added to the registry joins the matrix automatically, + and this fails loudly if the core/optional split ever computes to nothing.""" + assert OPTIONAL, "no optional modules found — the matrix would be vacuous" + assert CORE.isdisjoint(OPTIONAL) + assert len(CORE) + len(OPTIONAL) == len(BY_KEY) + + +def test_a_core_module_may_not_declare_a_dependency_on_an_optional_one(): + """The declared half, pinned here so the registry rule cannot be quietly removed.""" + for definition in MODULES: + if not definition.is_core: + continue + optional_requirements = [ + key for key in definition.requires_modules if not BY_KEY[key].is_core + ] + assert not optional_requirements, ( + f"{definition.key} is core but requires {optional_requirements}" + ) diff --git a/backend/tests/makerspaces/test_request_access.py b/backend/tests/makerspaces/test_request_access.py new file mode 100644 index 00000000..ac8fb6f6 --- /dev/null +++ b/backend/tests/makerspaces/test_request_access.py @@ -0,0 +1,282 @@ +"""The membership / account-less-requests pair is impossible, at every depth. + +`RequestSubmitView` takes its anonymous branch BEFORE any membership guard runs, so a +makerspace carrying both `membership` and `anonymous_requests_enabled` would let a +stranger walk past the membership requirement the operator had just switched on. These +pin the three enforcement depths independently, because each one covers writers the +others do not: the model rule covers every save, the service covers the deliberate +operator request, and the view re-derives so a row written behind both still fails closed. +""" + +import json +from io import StringIO + +import pytest +from django.core.management import call_command +from django.core.management.base import CommandError +from django.urls import reverse +from rest_framework.test import APIClient + +from apps.audit.models import AuditLog +from apps.inventory.models import InventoryProduct +from apps.makerspaces.models import Makerspace +from apps.makerspaces.module_install import install_module, uninstall_module +from apps.makerspaces.request_access import ( + ACCOUNTS, + ANYONE, + MEMBERS, + RequestAccessConflict, + effective_policy, + set_anonymous_requests, +) + +pytestmark = pytest.mark.django_db + + +def _space(slug, *, modules, anonymous=False): + return Makerspace.objects.create( + name=slug, + slug=slug, + enabled_modules=list(modules), + anonymous_requests_enabled=anonymous, + ) + + +CORE_PLUS = ["public_inventory", "request_workflow", "staff_admin", "scanner", + "evidence_uploads", "qr_management"] + + +# --------------------------------------------------------------------------- model + + +def test_saving_with_membership_forces_account_less_requests_off(): + space = _space("ra-model", modules=[*CORE_PLUS, "membership"], anonymous=True) + + space.refresh_from_db() + assert space.anonymous_requests_enabled is False + assert effective_policy(space) == MEMBERS + + +def test_a_partial_save_cannot_leave_the_impossible_pair_on_the_row(): + """`save(update_fields=[...])` that does not name the flag must still persist it.""" + space = _space("ra-partial", modules=CORE_PLUS, anonymous=True) + assert space.anonymous_requests_enabled is True + + space.enabled_modules = [*CORE_PLUS, "membership"] + space.save(update_fields=["enabled_modules"]) + + space.refresh_from_db() + assert space.anonymous_requests_enabled is False + + +def test_membership_off_leaves_account_less_requests_alone(): + space = _space("ra-open", modules=CORE_PLUS, anonymous=True) + + space.refresh_from_db() + assert space.anonymous_requests_enabled is True + assert effective_policy(space) == ANYONE + + +def test_policy_without_membership_and_without_the_flag_is_accounts(): + space = _space("ra-accounts", modules=CORE_PLUS) + assert effective_policy(space) == ACCOUNTS + + +# ------------------------------------------------------------------- module install + + +def test_installing_membership_closes_account_less_requests(): + space = _space("ra-install", modules=CORE_PLUS, anonymous=True) + + install_module(space, "membership") + + space.refresh_from_db() + assert space.anonymous_requests_enabled is False + + +def test_the_forced_close_is_audited_with_the_before_and_after_policy(): + """The flip happens inside `Makerspace.save()`, which has no actor, so it is audited on + the module-change path instead. Without this the capability meta carries module and + feature lists only -- which cannot distinguish a previous `anyone` policy from + `accounts` -- and the operator loses the record that installing membership closed an + unauthenticated write surface.""" + space = _space("ra-forced-audit", modules=CORE_PLUS, anonymous=True) + + install_module(space, "membership") + + entry = AuditLog.objects.filter(action="makerspace.capabilities_changed").latest("id") + assert entry.meta["request_access"] == {"before": ANYONE, "after": MEMBERS} + + +def test_installing_membership_records_the_accounts_to_members_move_too(): + """Not only the forced-off case: installing membership moves `accounts` -> `members` + even when account-less requests were already off, and that is still a change in who + may submit.""" + space = _space("ra-accounts-to-members", modules=CORE_PLUS) + + install_module(space, "membership") + + entry = AuditLog.objects.filter(action="makerspace.capabilities_changed").latest("id") + assert entry.meta["request_access"] == {"before": ACCOUNTS, "after": MEMBERS} + + +def test_an_unrelated_module_change_records_no_policy_meta(): + """The key is omitted rather than emitted-as-unchanged, so an auditor reading the log + sees a request-access entry only where the policy actually moved.""" + space = _space("ra-unrelated-audit", modules=CORE_PLUS) + + install_module(space, "telegram") + + entry = AuditLog.objects.filter(action="makerspace.capabilities_changed").latest("id") + assert "request_access" not in entry.meta + + +def test_uninstalling_membership_does_not_reopen_account_less_requests(): + """Forcing off is one-way. Re-opening an unauthenticated write surface is an + explicit operator act, never a side effect of removing an unrelated module.""" + space = _space("ra-uninstall", modules=[*CORE_PLUS, "membership", "member_accounts"]) + + uninstall_module(space, "membership") + + space.refresh_from_db() + assert space.anonymous_requests_enabled is False + assert effective_policy(space) == ACCOUNTS + + +# ------------------------------------------------------------------------- service + + +def test_set_anonymous_requests_refuses_while_membership_is_installed(): + space = _space("ra-refuse", modules=[*CORE_PLUS, "membership"]) + + with pytest.raises(RequestAccessConflict): + set_anonymous_requests(space, True) + + space.refresh_from_db() + assert space.anonymous_requests_enabled is False + + +def test_set_anonymous_requests_audits_the_policy_change(): + space = _space("ra-audit", modules=CORE_PLUS) + + resulting = set_anonymous_requests(space, True) + + assert resulting == ANYONE + entry = AuditLog.objects.filter(action="makerspace.request_access_changed").latest("id") + assert entry.meta["before"] == ACCOUNTS + assert entry.meta["after"] == ANYONE + + +def test_setting_the_same_value_twice_writes_no_second_audit_row(): + space = _space("ra-idempotent", modules=CORE_PLUS) + set_anonymous_requests(space, True) + before = AuditLog.objects.filter(action="makerspace.request_access_changed").count() + + set_anonymous_requests(space, True) + + assert AuditLog.objects.filter(action="makerspace.request_access_changed").count() == before + + +# ------------------------------------------------------------------------- command + + +def test_command_reports_the_policy_the_module_state_actually_produces(): + space = _space("ra-cmd-accounts", modules=CORE_PLUS) + out = StringIO() + + call_command("set_request_access", "--makerspace", space.slug, "--mode", MEMBERS, stdout=out) + + text = out.getvalue() + # `members` was asked for, but without the membership module the honest answer is + # `accounts`, and the operator must be told rather than left believing otherwise. + assert "signed-in account" in text + assert "You asked for 'members'" in text + + +def test_command_refuses_anyone_when_membership_is_installed(): + space = _space("ra-cmd-conflict", modules=[*CORE_PLUS, "membership"]) + + with pytest.raises(CommandError): + call_command("set_request_access", "--makerspace", space.slug, "--mode", ANYONE) + + space.refresh_from_db() + assert space.anonymous_requests_enabled is False + + +def test_list_modules_json_reports_installed_keys_and_request_access(): + space = _space("ra-json", modules=CORE_PLUS, anonymous=True) + out = StringIO() + + call_command("list_modules", "--makerspace", space.slug, "--json", stdout=out) + + payload = json.loads(out.getvalue()) + assert payload["makerspace"] == space.slug + assert payload["request_access"] == ANYONE + assert "request_workflow" in payload["installed"] + + +# ---------------------------------------------------------------------------- view + + +def test_an_anonymous_submission_is_refused_when_membership_is_installed(): + """The hole this closes: the row carries BOTH, written behind the model rule.""" + space = _space("ra-view", modules=[*CORE_PLUS, "membership"]) + # Straight to the column, bypassing save() exactly as raw SQL or an old restore would. + Makerspace.objects.filter(pk=space.pk).update(anonymous_requests_enabled=True) + product = InventoryProduct.objects.create( + makerspace=space, name="Multimeter", total_quantity=2, available_quantity=2, is_public=True, + ) + + response = APIClient().post( + reverse("hardware_requests:request-submit", args=[space.slug]), + { + "contact_name": "Stranger", + "contact_email": "stranger@example.test", + "items": [{"product_id": product.id, "quantity": 1}], + }, + format="json", + HTTP_IDEMPOTENCY_KEY="ra-view-key", + ) + + assert response.status_code == 401 + + +# --------------------------------------------------------------- /control/ matrix + + +def test_the_control_capability_matrix_audits_the_forced_policy_change(): + """The `/control/` matrix does NOT go through `module_install._apply` -- the admin + mixin saves the model directly -- so it needs its own before/after capture. The model + still forces account-less requests off, but module and feature lists alone cannot tell + an `anyone -> members` change from an `accounts -> members` one. + """ + from types import SimpleNamespace + + from django.contrib.admin import ModelAdmin + from django.contrib.admin.sites import AdminSite + + from apps.accounts.models import User + from apps.makerspaces.admin_capabilities import ( + MakerspaceAdminForm, + MakerspaceCapabilityAdminMixin, + ) + + class _CapabilityAdmin(MakerspaceCapabilityAdminMixin, ModelAdmin): + """The mixin relies on `super().save_model`, so it needs a real ModelAdmin.""" + + space = _space("ra-control-matrix", modules=CORE_PLUS, anonymous=True) + assert effective_policy(space) == ANYONE + + form = MakerspaceAdminForm(instance=space) + # Captured from the unmodified instance, before `clean_capabilities` rewrites it. + assert form.request_access_before == ANYONE + + actor = User.objects.create_user(username="ra-control-actor", is_superuser=True) + space.enabled_modules = [*CORE_PLUS, "membership", "member_accounts"] + admin = _CapabilityAdmin(Makerspace, AdminSite()) + admin.save_model(SimpleNamespace(user=actor), space, form, change=True) + + space.refresh_from_db() + assert space.anonymous_requests_enabled is False + entry = AuditLog.objects.filter(action="makerspace.capabilities_changed").latest("id") + assert entry.meta["request_access"] == {"before": ANYONE, "after": MEMBERS} diff --git a/backend/tests/operations/test_anonymous_requester_isolation.py b/backend/tests/operations/test_anonymous_requester_isolation.py new file mode 100644 index 00000000..7716c683 --- /dev/null +++ b/backend/tests/operations/test_anonymous_requester_isolation.py @@ -0,0 +1,227 @@ +"""Account-less requests must not collapse into one fictional person downstream. + +Every anonymous submission in a makerspace points at the SAME +`Makerspace.anonymous_requester` principal, because that is what gives the request a +requester FK without inventing an account. Any report that groups by `requester_id` then +folds a hundred unrelated strangers into a single row: one "repeat offender", one "top +borrower", one restrictable user whose restriction would hit every future account-less +requester at once. + +The principal is excluded from every per-PERSON ranking, and — because losing the data +entirely would be worse than naming a fake person — the damage and loss it was carrying is +reported as a separate total instead. +""" + +import pytest +from datetime import timedelta + +from django.utils import timezone + +from apps.accounts.models import User +from apps.hardware_requests.models import ( + HardwareRequest, + HardwareRequestItem, + RequesterAccountability, +) +from apps.inventory.models import InventoryProduct +from apps.makerspaces.anonymous_requesters import ( + anonymous_requester_ids, + get_or_create_anonymous_requester, +) +from apps.makerspaces.models import Makerspace +from apps.operations import reports +from apps.operations.accountability import accountability_data + +pytestmark = pytest.mark.django_db + + +def _space(slug): + return Makerspace.objects.create(name=slug, slug=slug) + + +def _member(username): + return User.objects.create_user( + username=username, email=f"{username}@example.test", display_name=username, + ) + + +def _product(space): + return InventoryProduct.objects.create( + makerspace=space, name="Oscilloscope", total_quantity=50, available_quantity=50, + ) + + +def _issued_request(space, product, requester, *, quantity=1, name="", email=""): + request = HardwareRequest.objects.create( + makerspace=space, + requester=requester, + requester_username="" if name else requester.username, + requester_name=name, + requester_contact_email=email, + requester_contact_verified=not name, + status=HardwareRequest.Status.ISSUED, + issued_at=timezone.now(), + ) + HardwareRequestItem.objects.create( + request=request, + product=product, + requested_quantity=quantity, + accepted_quantity=quantity, + issued_quantity=quantity, + ) + return request + + +def _recorder(): + """The staffer who recorded the incident. + + Irrelevant to what these tests assert -- they are about the *requester* side -- but + `created_by` is a non-null PROTECT FK, so every row needs one. Reused rather than + created per call: a test records several incidents, and `username` is unique. + """ + user, _ = User.objects.get_or_create( + username="iso-recorder", + defaults={ + "email": "iso-recorder@example.test", + "display_name": "iso-recorder", + }, + ) + return user + + +def _accountability(space, requester, issue_type, quantity=1): + """An incident recorded against `requester`. + + `request` and `request_item` are both non-null on the model -- an incident is always + against one specific issued line -- so the helper creates the loan it is recording + rather than making every caller build one it does not otherwise care about. + """ + request = _issued_request(space, _product(space), requester, quantity=quantity) + return RequesterAccountability.objects.create( + makerspace=space, + requester=requester, + request=request, + request_item=HardwareRequestItem.objects.get(request=request), + issue_type=issue_type, + quantity=quantity, + created_by=_recorder(), + ) + + +def test_the_shared_principal_never_appears_as_a_top_borrower(): + space = _space("iso-top-borrowers") + product = _product(space) + principal = get_or_create_anonymous_requester(space) + member = _member("iso-real-borrower") + + _issued_request(space, product, member, quantity=1) + # Three strangers, one principal — this is the collapse being prevented. Left in, the + # principal would out-rank the real borrower three to one. + for index in range(3): + _issued_request( + space, product, principal, quantity=5, + name=f"Stranger {index}", email=f"stranger{index}@example.test", + ) + + rows = reports._top_borrowers(space.id, aggregate=False) + + assert rows[0] == ["holder", "requests", "items_borrowed"] + holders = [row[0] for row in rows[1:]] + assert holders == ["iso-real-borrower"] + assert principal.username not in holders + + +def test_the_export_header_is_unchanged_by_the_exclusion(): + """The report registry pins these three columns; excluding rows must not touch them.""" + space = _space("iso-header") + product = _product(space) + principal = get_or_create_anonymous_requester(space) + _issued_request(space, product, principal, name="Stranger", email="s@example.test") + + rows = reports._top_borrowers(space.id, aggregate=False) + + assert rows[0] == ["holder", "requests", "items_borrowed"] + assert rows[1:] == [] + + +def test_the_shared_principal_never_appears_as_a_repeat_offender(): + space = _space("iso-offenders") + principal = get_or_create_anonymous_requester(space) + member = _member("iso-real-offender") + _accountability(space, member, RequesterAccountability.IssueType.DAMAGED) + for _ in range(4): + _accountability(space, principal, RequesterAccountability.IssueType.MISSING, quantity=2) + + data = accountability_data(space.id) + + assert [row["requester_id"] for row in data["repeat_offenders"]] == [member.id] + + +def test_the_excluded_damage_and_loss_is_still_reported_as_a_total(): + """Excluding the principal must not silently delete accountability history.""" + space = _space("iso-aggregate") + principal = get_or_create_anonymous_requester(space) + _accountability(space, principal, RequesterAccountability.IssueType.DAMAGED, quantity=2) + _accountability(space, principal, RequesterAccountability.IssueType.MISSING, quantity=3) + + data = accountability_data(space.id) + + assert data["anonymous_accountability"] == { + "damaged": 1, "missing": 1, "total_issues": 2, "total_quantity": 5, + } + + +def test_the_aggregate_is_zero_shaped_when_there_are_no_account_less_incidents(): + space = _space("iso-aggregate-empty") + _accountability(space, _member("iso-only-member"), RequesterAccountability.IssueType.DAMAGED) + + data = accountability_data(space.id) + + assert data["anonymous_accountability"] == { + "damaged": 0, "missing": 0, "total_issues": 0, "total_quantity": 0, + } + + +def test_an_overdue_account_less_loan_shows_the_contact_not_the_internal_username(): + """The principal's username is `member_`, which tells staff nothing.""" + space = _space("iso-overdue") + product = _product(space) + principal = get_or_create_anonymous_requester(space) + request = _issued_request( + space, product, principal, name="Ada Lovelace", email="ada@example.test", + ) + request.return_due_at = timezone.now() - timedelta(days=2) + request.save(update_fields=["return_due_at"]) + + data = accountability_data(space.id) + + overdue = [row for row in data["overdue"] if row["reference_id"] == request.id] + assert overdue, data["overdue"] + assert overdue[0]["requester_username"] == "Ada Lovelace" + assert not overdue[0]["requester_username"].startswith("member_") + + +def test_a_restricted_principal_is_never_listed_as_a_restricted_person(): + """Restrict/restore already refuse the principal; this is the read-side backstop for + a row written before that guard, or by a direct database edit.""" + space = _space("iso-restricted") + principal = get_or_create_anonymous_requester(space) + _accountability(space, principal, RequesterAccountability.IssueType.MISSING) + User.objects.filter(pk=principal.pk).update( + access_status=User.AccessStatus.RESTRICTED, restriction_reason="edited directly", + ) + + data = accountability_data(space.id) + + assert [row["requester_id"] for row in data["restrictions"]] == [] + + +def test_anonymous_requester_ids_is_scoped_and_global(): + one, two = _space("iso-scope-one"), _space("iso-scope-two") + first = get_or_create_anonymous_requester(one) + second = get_or_create_anonymous_requester(two) + + assert anonymous_requester_ids([one.id]) == {first.id} + assert anonymous_requester_ids() >= {first.id, second.id} + # A makerspace that has never taken an account-less request has no principal at all. + assert anonymous_requester_ids([_space("iso-scope-three").id]) == set() diff --git a/backend/tests/presence/test_guard_m3.py b/backend/tests/presence/test_guard_m3.py index 2db615bd..776db23c 100644 --- a/backend/tests/presence/test_guard_m3.py +++ b/backend/tests/presence/test_guard_m3.py @@ -9,6 +9,7 @@ MemberPresenceRequired, PresenceRequired, WaiverAcceptanceRequired, + require_active_account, require_active_member_presence, ) @@ -20,6 +21,23 @@ def setup_member(space): return user, membership +@pytest.mark.django_db +def test_active_account_guard_keeps_identity_checks_without_requiring_membership(): + space = Makerspace.objects.create(name="Account Guard", slug="account-guard") + user = User.objects.create_user(username="account-only", password="password") + + result = require_active_account(user, space) + + assert result.membership is None + assert result.accepted_waiver is None + assert result.session is None + + user.access_status = User.AccessStatus.RESTRICTED + user.save(update_fields=["access_status"]) + with pytest.raises(MemberPresenceRequired): + require_active_account(user, space) + + @pytest.mark.django_db def test_guard_has_stable_membership_waiver_and_presence_contract(): space = Makerspace.objects.create(name="Guard", slug="guard") diff --git a/backend/tests/test_anonymous_request_submit_b2.py b/backend/tests/test_anonymous_request_submit_b2.py new file mode 100644 index 00000000..c29a89a6 --- /dev/null +++ b/backend/tests/test_anonymous_request_submit_b2.py @@ -0,0 +1,285 @@ +"""B2-B: account-less request submission and its release-blocking abuse limits.""" + +import pytest +from django.core.cache import cache +from django.urls import reverse +from rest_framework.test import APIClient, APIRequestFactory + +from apps.accounts.audit_events import fingerprint +from apps.accounts.models import User +from apps.audit.models import AuditLog +from apps.hardware_requests.models import HardwareRequest +from apps.hardware_requests.throttles import AnonymousRequestEmailThrottle +from apps.inventory.models import InventoryProduct +from apps.makerspaces.models import Makerspace +from apps.makerspaces.module_profiles import RECOMMENDED, profile_modules + + +pytestmark = pytest.mark.django_db + + +@pytest.fixture(autouse=True) +def _clear_throttle_cache(): + cache.clear() + yield + cache.clear() + + +def _space(slug, *, anonymous=True): + return Makerspace.objects.create( + name=slug, + slug=slug, + enabled_modules=profile_modules(RECOMMENDED), + enabled_features=["inventory.self_checkout"], + anonymous_requests_enabled=anonymous, + ) + + +def _product(space, name="Logic analyzer"): + return InventoryProduct.objects.create( + makerspace=space, + name=name, + total_quantity=100, + available_quantity=100, + is_public=True, + ) + + +def _payload(product, *, email="Ada@Example.Test"): + return { + "contact_name": "Ada Lovelace", + "contact_email": email, + "contact_phone": "+44 (0)20 1234 5678", + "requested_for": "Bench diagnostics", + "items": [{"product_id": product.pk, "quantity": 1}], + } + + +def _submit(space, payload, key, *, ip="198.51.100.10", client=None): + return (client or APIClient()).post( + reverse("hardware_requests:request-submit", args=[space.slug]), + payload, + format="json", + HTTP_IDEMPOTENCY_KEY=key, + REMOTE_ADDR=ip, + ) + + +def test_anonymous_submit_requires_opt_in_and_succeeds_when_enabled(): + disabled = _space("anonymous-disabled", anonymous=False) + disabled_response = _submit(disabled, _payload(_product(disabled)), "disabled") + + assert disabled_response.status_code == 401 + assert str(disabled_response.data["detail"]) == "Authentication credentials were not provided." + assert not HardwareRequest.objects.filter(makerspace=disabled).exists() + + enabled = _space("anonymous-enabled") + enabled_response = _submit(enabled, _payload(_product(enabled)), "enabled") + + assert enabled_response.status_code == 201, enabled_response.data + row = HardwareRequest.objects.get(makerspace=enabled) + # The view resolves its own Makerspace instance, so the principal is created + # against that row; this local copy predates it. + enabled.refresh_from_db() + assert row.requester == enabled.anonymous_requester + assert row.requester_username == "" + assert row.requester_name == "Ada Lovelace" + assert row.requester_contact_email == "ada@example.test" + assert row.requester_contact_verified is False + + +def test_authenticated_submit_ignores_contact_spoofing_and_keeps_account_identity(): + space = _space("authenticated-submit", anonymous=False) + product = _product(space) + user = User.objects.create_user( + username="real-account", + display_name="Real Account", + email="REAL@EXAMPLE.TEST", + phone="trusted phone", + ) + client = APIClient() + client.force_authenticate(user) + payload = _payload(product) + payload.update( + { + "contact_name": "x" * 201, + "contact_email": "not-an-email", + "contact_phone": "x" * 33, + } + ) + + response = _submit(space, payload, "ignored-for-auth", client=client) + + assert response.status_code == 201, response.data + row = HardwareRequest.objects.get(makerspace=space) + assert row.requester == user + assert row.requester_username == user.username + assert row.requester_name == user.display_name + assert row.requester_contact_email == user.email + assert row.requester_contact_phone == user.phone + assert row.requester_contact_verified is True + assert AuditLog.objects.get(action="request.submitted").actor == user + + +def test_anonymous_per_ip_burst_throttle_fires(): + space = _space("anonymous-ip-burst") + product = _product(space) + + responses = [ + _submit( + space, + _payload(product, email=f"person{index}@example.test"), + f"ip-burst-{index}", + ) + for index in range(3) + ] + + assert [response.status_code for response in responses] == [201, 201, 429] + + +def test_anonymous_per_ip_hour_throttle_fires(monkeypatch): + from apps.hardware_requests.throttles import ( + AnonymousRequestIpBurstThrottle, + AnonymousRequestIpHourThrottle, + ) + + monkeypatch.setattr(AnonymousRequestIpBurstThrottle, "rate", "100/min", raising=False) + monkeypatch.setattr(AnonymousRequestIpHourThrottle, "rate", "2/hour", raising=False) + space = _space("anonymous-ip-hour") + product = _product(space) + + responses = [ + _submit( + space, + _payload(product, email=f"hour{index}@example.test"), + f"ip-hour-{index}", + ) + for index in range(3) + ] + + assert [response.status_code for response in responses] == [201, 201, 429] + + +def test_anonymous_per_email_throttle_uses_only_a_fingerprint(): + space = _space("anonymous-email-limit") + product = _product(space) + email = "Target@Example.Test" + responses = [ + _submit( + space, + _payload(product, email=email), + f"email-{index}", + ip=f"198.51.100.{index + 1}", + ) + for index in range(4) + ] + + assert [response.status_code for response in responses] == [201, 201, 201, 429] + + request = APIRequestFactory().post("/", {}, format="json") + request.anonymous_contact_email = email.lower() + key = AnonymousRequestEmailThrottle().get_cache_key(request, object()) + assert email.lower() not in key + assert fingerprint(email) in key + + +def test_anonymous_audit_is_unattributed_and_principal_is_not_snapshotted(): + space = _space("anonymous-attribution") + response = _submit(space, _payload(_product(space)), "attribution") + + assert response.status_code == 201, response.data + row = HardwareRequest.objects.get(makerspace=space) + space.refresh_from_db() + assert row.requester == space.anonymous_requester + assert row.requester_username == "" + assert space.anonymous_requester.username not in { + row.requester_username, + row.requester_name, + row.requester_contact_email, + row.requester_contact_phone, + } + assert AuditLog.objects.get(action="request.submitted", target_id=str(row.pk)).actor is None + + +def test_unverified_anonymous_contact_never_receives_lifecycle_mail(monkeypatch): + from apps.hardware_requests import notifications + + space = _space("anonymous-no-requester-mail") + response = _submit(space, _payload(_product(space)), "no-requester-mail") + assert response.status_code == 201, response.data + row = HardwareRequest.objects.get(makerspace=space) + monkeypatch.setattr(notifications, "staff_emails_for_feature", lambda *args, **kwargs: []) + + def unexpected_requester_render(*args, **kwargs): + raise AssertionError("unverified requester mail must not be rendered") + + monkeypatch.setattr(notifications, "render_email", unexpected_requester_render) + assert notifications._email_deliveries(row, "request_received", "submitted") == () + + +def test_anonymous_idempotent_double_submit_creates_one_request_and_one_audit(): + space = _space("anonymous-idempotent") + payload = _payload(_product(space)) + + first = _submit(space, payload, "same-retry-key") + second = _submit(space, payload, "same-retry-key") + + assert first.status_code == second.status_code == 201 + assert first.data == second.data + assert HardwareRequest.objects.filter(makerspace=space).count() == 1 + assert AuditLog.objects.filter(action="request.submitted", makerspace=space).count() == 1 + + +def test_reused_idempotency_key_with_different_payload_is_a_typed_conflict(): + space = _space("anonymous-idempotency-conflict") + product = _product(space) + first = _submit(space, _payload(product), "reused-key") + changed = _payload(product) + changed["requested_for"] = "A different purpose" + second = _submit(space, changed, "reused-key") + + assert first.status_code == 201, first.data + assert second.status_code == 409 + assert second.data["code"] == "anonymous_request_idempotency_conflict" + assert HardwareRequest.objects.filter(makerspace=space).count() == 1 + + +def test_outstanding_anonymous_ceiling_returns_typed_error(settings): + settings.ANONYMOUS_REQUEST_OUTSTANDING_LIMIT = 1 + space = _space("anonymous-capacity") + product = _product(space) + + first = _submit(space, _payload(product), "capacity-first") + second = _submit( + space, + _payload(product, email="grace@example.test"), + "capacity-second", + ) + + assert first.status_code == 201, first.data + assert second.status_code == 429 + assert second.data["code"] == "anonymous_request_outstanding_limit" + assert HardwareRequest.objects.filter(makerspace=space).count() == 1 + + +def test_the_refusal_path_is_throttled_not_just_the_accepted_one(): + """An unopted-in space must not serve an UNBOUNDED 401. + + The throttles used to be selected inside `post()`, which runs *after* the + `anonymous_requests_allowed` refusal -- so every makerspace that had not opted in + (the default) answered unauthenticated POSTs forever, each one still paying for a + makerspace lookup. They are declared in `throttle_classes` now, so DRF charges the + IP budget in `initial()`, before the handler and before that refusal. + """ + disabled = _space("anonymous-refusal-throttled", anonymous=False) + payload = _payload(_product(disabled)) + + statuses = [ + _submit(disabled, payload, f"refusal-{index}", ip="203.0.113.77").status_code + for index in range(3) + ] + + # 2/min burst: the first two are refused for being account-less, the third for + # exhausting the budget. Before the fix this was [401, 401, 401]. + assert statuses[:2] == [401, 401] + assert statuses[2] == 429 diff --git a/backend/tests/test_anonymous_request_submit_b2_limits.py b/backend/tests/test_anonymous_request_submit_b2_limits.py new file mode 100644 index 00000000..3745ccab --- /dev/null +++ b/backend/tests/test_anonymous_request_submit_b2_limits.py @@ -0,0 +1,80 @@ +"""B2-B boundary caps that do not need request-workflow database setup.""" + +import pytest +from django.conf import settings as django_settings + +from apps.encryption.registry import field_for +from apps.hardware_requests.models import HardwareRequest +from apps.hardware_requests.serializers import RequestSubmitSerializer + + +def _payload(): + return { + "contact_name": "Ada Lovelace", + "contact_email": "Ada@Example.Test", + "contact_phone": "+44 (0)20 1234 5678", + "requested_for": "Bench diagnostics", + "items": [{"product_id": 1, "quantity": 1}], + } + + +@pytest.mark.parametrize( + ("field", "value"), + [ + ("contact_name", "x" * 201), + ("contact_email", f"{'a' * 243}@example.com"), + ("contact_phone", "x" * 33), + ("requested_for", "x" * 501), + ("items", [{"product_id": index, "quantity": 1} for index in range(21)]), + ("items", [{"product_id": 1, "quantity": 100}]), + ], +) +def test_anonymous_serializer_rejects_every_hard_cap(field, value): + payload = _payload() + payload[field] = value + serializer = RequestSubmitSerializer( + data=payload, + context={"anonymous_submission": True}, + ) + + assert not serializer.is_valid() + assert field in serializer.errors + + +@pytest.mark.parametrize("missing_field", ["contact_name", "contact_email"]) +def test_anonymous_serializer_requires_name_and_email(missing_field): + payload = _payload() + payload.pop(missing_field) + serializer = RequestSubmitSerializer( + data=payload, + context={"anonymous_submission": True}, + ) + + assert not serializer.is_valid() + assert missing_field in serializer.errors + + +def test_authenticated_contact_fields_are_ignored_before_validation(): + payload = _payload() + payload.update( + { + "contact_name": "x" * 201, + "contact_email": "not-an-email", + "contact_phone": "x" * 33, + } + ) + serializer = RequestSubmitSerializer( + data=payload, + context={"anonymous_submission": False}, + ) + + assert serializer.is_valid(), serializer.errors + assert not set(RequestSubmitSerializer.CONTACT_FIELDS) & serializer.validated_data.keys() + + +def test_default_anonymous_abuse_rates_and_encryption_limit_are_configured(): + rates = django_settings.REST_FRAMEWORK["DEFAULT_THROTTLE_RATES"] + assert rates["anonymous_request_ip_burst"] == "2/min" + assert rates["anonymous_request_ip_hour"] == "10/hour" + assert rates["anonymous_request_email"] == "3/day" + assert field_for(HardwareRequest, "requester_name").max_length == 200 diff --git a/backend/tests/test_hardware_admin_actions.py b/backend/tests/test_hardware_admin_actions.py index a939fc45..5bd152cb 100644 --- a/backend/tests/test_hardware_admin_actions.py +++ b/backend/tests/test_hardware_admin_actions.py @@ -83,32 +83,99 @@ def action_payload(action, hardware_request, **extra): } -def test_accept_action_moves_pending_request_to_accepted(): +def review_url(hardware_request): + return reverse( + "admin:hardware_requests_hardwarerequest_review", args=[hardware_request.pk] + ) + + +def test_the_bulk_accept_and_reject_actions_no_longer_exist(): + """Accepting reserves stock and rejecting closes a person's ask. Neither is a + checkbox-column decision any more -- both moved to the one-by-one review page.""" + superadmin = make_superadmin("hardware-no-bulk-superadmin") + hardware_request = make_hardware_request() + + for action in ("accept_selected", "reject_selected"): + response = admin_client(superadmin).post( + changelist_url(), action_payload(action, hardware_request), follow=True + ) + hardware_request.refresh_from_db() + assert hardware_request.status == HardwareRequest.Status.PENDING_APPROVAL, action + messages = [str(message) for message in get_messages(response.wsgi_request)] + assert "No action selected." in messages, action + + +def test_the_review_page_accepts_one_request(): superadmin = make_superadmin() hardware_request = make_hardware_request() + response = admin_client(superadmin).post(review_url(hardware_request), {"accept": "1"}) + + assert response.status_code == 302 + hardware_request.refresh_from_db() + assert hardware_request.status == HardwareRequest.Status.ACCEPTED + + +def test_the_review_page_honours_a_lowered_accepted_quantity(): + superadmin = make_superadmin("hardware-partial-superadmin") + hardware_request = make_hardware_request() + item = hardware_request.items.get() + item.requested_quantity = 3 + item.save(update_fields=["requested_quantity"]) + response = admin_client(superadmin).post( - changelist_url(), - action_payload("accept_selected", hardware_request), + review_url(hardware_request), + {"accept": "1", f"accepted_quantity_{item.pk}": "2"}, ) assert response.status_code == 302 + item.refresh_from_db() + assert item.accepted_quantity == 2 + + +def test_the_review_page_refuses_a_quantity_above_what_was_requested(): + superadmin = make_superadmin("hardware-overrequest-superadmin") + hardware_request = make_hardware_request() + item = hardware_request.items.get() + + response = admin_client(superadmin).post( + review_url(hardware_request), + {"accept": "1", f"accepted_quantity_{item.pk}": "99"}, + follow=True, + ) + hardware_request.refresh_from_db() - assert hardware_request.status == HardwareRequest.Status.ACCEPTED + assert hardware_request.status == HardwareRequest.Status.PENDING_APPROVAL + messages = [str(message) for message in get_messages(response.wsgi_request)] + assert any("must be between 0 and" in message for message in messages) + + +def test_the_review_page_refuses_a_non_numeric_quantity(): + """Parsed here rather than handed to the workflow, so a malformed field is an error + on the page instead of a 500 inside the service.""" + superadmin = make_superadmin("hardware-nan-superadmin") + hardware_request = make_hardware_request() + item = hardware_request.items.get() + + response = admin_client(superadmin).post( + review_url(hardware_request), + {"accept": "1", f"accepted_quantity_{item.pk}": "two"}, + follow=True, + ) + hardware_request.refresh_from_db() + assert hardware_request.status == HardwareRequest.Status.PENDING_APPROVAL + messages = [str(message) for message in get_messages(response.wsgi_request)] + assert any("whole number" in message for message in messages) -def test_reject_action_with_reason_moves_pending_request_to_rejected(): + +def test_the_review_page_rejects_with_a_reason(): superadmin = make_superadmin("hardware-reject-superadmin") hardware_request = make_hardware_request() response = admin_client(superadmin).post( - changelist_url(), - action_payload( - "reject_selected", - hardware_request, - apply="1", - reason="Unavailable this week.", - ), + review_url(hardware_request), + {"reject": "1", "reason": "Unavailable this week."}, ) assert response.status_code == 302 @@ -117,19 +184,13 @@ def test_reject_action_with_reason_moves_pending_request_to_rejected(): assert hardware_request.rejection_reason == "Unavailable this week." -def test_reject_action_with_empty_reason_does_not_change_status_and_reports_error(): +def test_the_review_page_refuses_to_reject_without_a_reason(): superadmin = make_superadmin("hardware-empty-reject-superadmin") hardware_request = make_hardware_request() - client = admin_client(superadmin) - - response = client.post( - changelist_url(), - action_payload( - "reject_selected", - hardware_request, - apply="1", - reason=" ", - ), + + response = admin_client(superadmin).post( + review_url(hardware_request), + {"reject": "1", "reason": " "}, follow=True, ) @@ -139,23 +200,14 @@ def test_reject_action_with_empty_reason_does_not_change_status_and_reports_erro assert "Rejection reason is required." in messages -def test_hardware_request_admin_blocks_direct_add_and_delete(): - """Regression: requests are workflow-driven; the admin must not expose - direct add (broken readonly form) or delete (bypasses reservation/audit).""" - superadmin = make_superadmin("hardware-add-delete-superadmin") - # has_*_permission=False raises PermissionDenied (rendered as 403); the test - # client must not re-raise it. - client = Client(raise_request_exception=False) - client.force_login(superadmin) +def test_the_review_page_is_closed_to_a_non_superadmin(): + """`admin_site.admin_view` plus the model's superuser gate. Without both, this URL + would be the one state-changing surface that skipped them.""" + staffer = make_user("hardware-review-staffer", is_staff=True) + hardware_request = make_hardware_request() - add_url = reverse("admin:hardware_requests_hardwarerequest_add") - add_response = client.get(add_url) - assert add_response.status_code == 403 + response = admin_client(staffer).get(review_url(hardware_request)) - hardware_request = make_hardware_request() - delete_url = reverse( - "admin:hardware_requests_hardwarerequest_delete", - args=[hardware_request.pk], - ) - delete_response = client.get(delete_url) - assert delete_response.status_code == 403 + assert response.status_code in (302, 403, 404) + hardware_request.refresh_from_db() + assert hardware_request.status == HardwareRequest.Status.PENDING_APPROVAL diff --git a/backend/tests/test_notification_destinations.py b/backend/tests/test_notification_destinations.py index 4f858089..e21c1617 100644 --- a/backend/tests/test_notification_destinations.py +++ b/backend/tests/test_notification_destinations.py @@ -333,19 +333,22 @@ def test_a_telegram_room_shares_the_makerspace_bot(): assert telegram.resolve_bot_token(space) == "space-token" -def test_inbound_callbacks_do_not_depend_on_which_room_sent_the_message(): - """G6: the reason one bot per makerspace is enough. - - An accept/reject button posts back to a single registered webhook authenticated by - one deployment-wide secret. Routing resolves the ACTOR from `from.id` and the request - from the callback data — the chat the button was pressed in is never consulted — so - adding rooms cannot strand a callback. If this ever starts reading a chat id, per-room - Telegram destinations need per-bot webhook secrets and inbound routing first. +def test_no_room_can_turn_a_telegram_alert_into_an_action_surface(): + """G6, restated for a one-way channel. + + This used to pin inbound callback routing: an accept/reject button posted back to a + single registered webhook, and routing never consulted the chat it came from, so + adding rooms could not strand a callback. **The buttons are gone.** What has to hold + now is simpler and stricter — no room, however configured, gets an inline keyboard, + because an inline keyboard IS the action surface that was removed. If a button is + ever reintroduced, per-room destinations need per-bot webhook secrets and inbound + routing before it ships. """ - from apps.integrations.views import _parse_callback + import inspect - assert _parse_callback("accept:42") == ("accept", 42, "") - assert _parse_callback("reject:42:out of stock") == ("reject", 42, "out of stock") + from apps.integrations import telegram + + assert "reply_markup" not in inspect.signature(telegram.send_message).parameters @override_settings(TELEGRAM_BOT_TOKEN="deployment-token") diff --git a/backend/tests/test_notification_dispatch.py b/backend/tests/test_notification_dispatch.py index a20f4a60..d6db7400 100644 --- a/backend/tests/test_notification_dispatch.py +++ b/backend/tests/test_notification_dispatch.py @@ -160,18 +160,20 @@ def test_telegram_payload_is_durable_and_passed_to_sender(monkeypatch): monkeypatch.setattr(limits, "is_self_host", lambda: True) sender = Mock(return_value=True) monkeypatch.setattr("apps.integrations.telegram.send_message", sender) - markup = {"inline_keyboard": [[{"text": "Open", "url": "https://safe.test"}]]} + # An arbitrary payload: the `payload` column carries delivery-time context (today, + # the alert's `scope`, which push resolves its recipients from), and the property + # under test is that it survives to the log row intact. + payload = {"scope": {"kind": "machine", "id": 7}} - log = dispatch(space, "telegram", payload={"reply_markup": markup}, sync=True) + log = dispatch(space, "telegram", payload=payload, sync=True) log.refresh_from_db() - assert log.payload == {"reply_markup": markup} + assert log.payload == payload assert log.status == NotificationDeliveryStatus.SENT # destination=None is the legacy makerspace-column path: this space has no rooms, so - # the chat id still comes off the makerspace exactly as it did. - sender.assert_called_once_with( - space, "Booking confirmed.", reply_markup=markup, destination=None - ) + # the chat id still comes off the makerspace exactly as it did. No `reply_markup`: + # the Telegram sender has no such parameter any more. + sender.assert_called_once_with(space, "Booking confirmed.", destination=None) def test_async_dispatch_enqueues_after_commit( diff --git a/backend/tests/test_notification_fanout.py b/backend/tests/test_notification_fanout.py index 89c372b7..b590f3d5 100644 --- a/backend/tests/test_notification_fanout.py +++ b/backend/tests/test_notification_fanout.py @@ -182,7 +182,7 @@ def test_raising_build_and_dispatch_never_escape(monkeypatch): } -def test_hardware_submitted_adapter_calls_one_fanout_with_original_buttons(monkeypatch): +def test_hardware_submitted_adapter_calls_one_fanout_and_carries_no_buttons(monkeypatch): space = make_space("fanout-hardware-adapter") requester = User.objects.create_user(username="fanout-hardware-requester") request = HardwareRequest.objects.create( @@ -206,13 +206,9 @@ def capture(makerspace, **kwargs): hardware_notifications.notify_request_submitted(request) assert len(calls) == 1 assert calls[0][1]["event"] == "submitted" - assert calls[0][2].telegram_reply_markup == { - "inline_keyboard": [[ - {"text": "Accept", "callback_data": f"accept:{request.pk}"}, - { - "text": "Reject", - "callback_data": f"reject:{request.pk}:Rejected from Telegram.", - }, - ]] - } + # No inline keyboard any more: chat is a notification channel, not a decision + # surface, so the adapter's whole telegram_reply_markup path was removed with the + # callback route it fed. + assert not hasattr(calls[0][2], "telegram_reply_markup") + assert "Review and decide in the staff console." in calls[0][2].text diff --git a/backend/tests/test_platform_bootstrap.py b/backend/tests/test_platform_bootstrap.py index 6f16ca86..0adf53b6 100644 --- a/backend/tests/test_platform_bootstrap.py +++ b/backend/tests/test_platform_bootstrap.py @@ -244,3 +244,47 @@ def test_staff_origin_scope_filters_makerspace_list_and_blocks_cross_tenant_targ assert cross_list.status_code == 403 assert own_detail.status_code == 200 assert cross_detail.status_code == 403 + + +def test_bootstrap_omits_request_access_unless_the_space_opted_in(): + """Absent means "an account is required", which is what every client assumed before + the policy existed. Emitting it unconditionally would change the payload for every + deployment, and the byte-for-byte dormant-payload invariant forbids that -- the same + reason `/api/v1/config` emits `member_accounts` only when off.""" + makerspace = make_space("platform-request-access-default") + makerspace.enabled_modules = ["public_inventory", "request_workflow"] + makerspace.save() + + response = APIClient().get(f"/api/v1/bootstrap?slug={makerspace.slug}") + + assert response.status_code == 200 + assert "request_access" not in response.data["makerspace"] + + +def test_bootstrap_publishes_request_access_when_account_less_requests_are_on(): + """The public borrow form reads this to decide whether to collect contact details and + send an Idempotency-Key. Without it the client posts a member-shaped body and takes a + 400 on a space that advertises "no account needed".""" + makerspace = make_space("platform-request-access-anyone") + makerspace.enabled_modules = ["public_inventory", "request_workflow"] + makerspace.anonymous_requests_enabled = True + makerspace.save() + + response = APIClient().get(f"/api/v1/bootstrap?slug={makerspace.slug}") + + assert response.status_code == 200 + assert response.data["makerspace"]["request_access"] == "anyone" + + +def test_bootstrap_withholds_request_access_when_membership_makes_it_impossible(): + """Fails closed on the read path too: a row carrying both settings (raw SQL, an old + backup) must not advertise account-less submission the view would refuse.""" + makerspace = make_space("platform-request-access-impossible") + makerspace.enabled_modules = ["public_inventory", "request_workflow", "membership"] + makerspace.anonymous_requests_enabled = True + makerspace.save() + + response = APIClient().get(f"/api/v1/bootstrap?slug={makerspace.slug}") + + assert response.status_code == 200 + assert "request_access" not in response.data["makerspace"] diff --git a/backend/tests/test_privileged_script_modes.py b/backend/tests/test_privileged_script_modes.py index 82243da2..62d0b680 100644 --- a/backend/tests/test_privileged_script_modes.py +++ b/backend/tests/test_privileged_script_modes.py @@ -11,8 +11,8 @@ """ from pathlib import Path -import os import re +import subprocess import pytest @@ -35,6 +35,36 @@ def _shipped_shell_scripts(): return sorted(ROOT.glob("scripts/*.sh")) + [ROOT / "setup.sh", ROOT / "install.sh"] +# The executable bit must be read from GIT, not the working tree. This repository has +# `core.fileMode = false`, so `chmod +x` never reaches a commit and `os.access()` reports +# a local bit that no clone and no release tarball will ever see. That blind spot already +# cost a production fix: `scripts/restore.sh` was chmod'd 0755 and guarded here on +# 2026-08-25, the guard went green, and `git archive HEAD` still shipped it 0644 - so +# `import-backup.sh` still died at `exec .../restore.sh` with "Permission denied", after +# recording the restore intent and releasing its lock. `install.sh` unpacks the GitHub +# tarball with git's modes and chmods only itself and the compose wrapper, so for every +# other script git's mode IS the shipped mode. Set it with `git update-index --chmod=+x`. +def _git_file_mode(name): + completed = subprocess.run( + ["git", "ls-files", "-s", "--", name], + cwd=ROOT, + capture_output=True, + text=True, + check=True, + ) + assert completed.stdout.strip(), f"{name} is not tracked by git" + return completed.stdout.split(None, 1)[0] + + +def _assert_ships_executable(name): + mode = _git_file_mode(name) + assert mode == "100755", ( + f"{name} is recorded in git as {mode}, so the release tarball ships it " + "without its executable bit and running it gives 'Permission denied'. " + f"Fix with: git update-index --chmod=+x {name}" + ) + + @pytest.mark.parametrize( "name", ["install.sh"] @@ -42,10 +72,7 @@ def _shipped_shell_scripts(): ) def test_operator_entry_point_is_executable(name): """The curl entry point and every privileged shell script ship executable.""" - assert os.access(ROOT / name, os.X_OK), ( - f"{name} is shipped without its executable bit; an operator running it " - "directly gets 'Permission denied'." - ) + _assert_ships_executable(name) def test_no_shipped_script_directly_executes_a_non_executable_sibling(): @@ -56,8 +83,10 @@ def test_no_shipped_script_directly_executes_a_non_executable_sibling(): continue for target in _DIRECT_EXECUTION.findall(script.read_text(encoding="utf-8")): checked += 1 - assert os.access(ROOT / target, os.X_OK), ( - f"{script.name} directly executes {target}, which is not executable; " - "that handoff dies with 'Permission denied'." + mode = _git_file_mode(target) + assert mode == "100755", ( + f"{script.name} directly executes {target}, which git records as " + f"{mode}; that handoff dies with 'Permission denied' in the release " + f"tarball. Fix with: git update-index --chmod=+x {target}" ) assert checked, "The direct-execution scan matched nothing; the pattern has rotted." diff --git a/backend/tests/test_request_membership_module_b1.py b/backend/tests/test_request_membership_module_b1.py new file mode 100644 index 00000000..fa67c2c3 --- /dev/null +++ b/backend/tests/test_request_membership_module_b1.py @@ -0,0 +1,158 @@ +"""B1: request proposals stay usable when the community membership module is off. + +Only public borrow-request submission gets this narrow exception. The other guarded +surfaces move hardware, reserve facility capacity, or create member participation, so +their tenant-binding MakerspaceMembership requirement is pinned below. +""" + +from datetime import timedelta + +import pytest +from django.urls import reverse +from django.utils import timezone +from rest_framework.test import APIClient + +from apps.accounts.models import User +from apps.events.models import Event +from apps.hardware_requests.models import HardwareRequest +from apps.inventory.models import InventoryProduct +from apps.makerspaces.models import Makerspace, MakerspaceWaiver +from apps.makerspaces.module_profiles import EVERYTHING, RECOMMENDED, profile_modules + + +pytestmark = pytest.mark.django_db + + +def _space(slug, modules): + return Makerspace.objects.create( + name=slug, + slug=slug, + enabled_modules=modules, + enabled_features=["inventory.self_checkout"], + ) + + +def _user(username): + return User.objects.create_user( + username=username, + email=f"{username}@example.test", + display_name=username, + phone="+15550101010", + ) + + +def _client(user=None): + client = APIClient() + if user is not None: + client.force_authenticate(user) + return client + + +def _product(space): + return InventoryProduct.objects.create( + makerspace=space, + name="Logic analyzer", + total_quantity=1, + available_quantity=1, + is_public=True, + ) + + +def _request_payload(product): + return { + "requested_for": "Bench diagnostics", + "items": [{"product_id": product.pk, "quantity": 1}], + } + + +def test_membership_off_allows_an_authenticated_non_member_to_submit_request(): + modules = profile_modules(RECOMMENDED) + assert "membership" not in modules + space = _space("request-membership-off", modules) + product = _product(space) + MakerspaceWaiver.objects.create( + makerspace=space, + is_active=True, + version="1", + body="Staff acceptance controls this proposal path.", + ) + + response = _client(_user("request-outsider")).post( + reverse("hardware_requests:request-submit", args=[space.slug]), + _request_payload(product), + format="json", + ) + + assert response.status_code == 201, response.data + assert HardwareRequest.objects.filter(makerspace=space).count() == 1 + + +def test_membership_on_still_refuses_an_authenticated_non_member(): + space = _space("request-membership-on", profile_modules(EVERYTHING)) + product = _product(space) + + response = _client(_user("request-non-member")).post( + reverse("hardware_requests:request-submit", args=[space.slug]), + _request_payload(product), + format="json", + ) + + assert response.status_code == 403 + assert response.data["code"] == "membership_required" + assert not HardwareRequest.objects.filter(makerspace=space).exists() + + +def test_membership_off_request_submission_still_requires_authentication(): + space = _space("request-anonymous", profile_modules(RECOMMENDED)) + + response = _client().post( + reverse("hardware_requests:request-submit", args=[space.slug]), + _request_payload(_product(space)), + format="json", + ) + + assert response.status_code == 401 + assert not HardwareRequest.objects.filter(makerspace=space).exists() + + +def test_membership_off_does_not_relax_other_physical_action_surfaces(): + """A future consistency cleanup must not widen these cross-tenant actions.""" + modules = [key for key in profile_modules(EVERYTHING) if key != "membership"] + space = _space("physical-actions-stay-member-only", modules) + client = _client(_user("physical-action-outsider")) + urls = [ + reverse("hardware_requests:public-tool-evidence-url", args=[space.slug]), + reverse("hardware_requests:public-tool-checkout", args=[space.slug]), + reverse("hardware_requests:public-tool-return", args=[space.slug]), + reverse("public-machine-service-request-submit", args=[space.slug]), + reverse("public-printer-service-upload", args=[space.slug]), + reverse("public-printer-service-request", args=[space.slug]), + ] + + for url in urls: + response = client.post(url, {}, format="json") + assert response.status_code == 403, (url, response.data) + assert response.data["code"] == "membership_required", (url, response.data) + + +def test_membership_off_does_not_relax_event_registration(): + modules = [key for key in profile_modules(EVERYTHING) if key != "membership"] + space = _space("events-stay-member-only", modules) + starts_at = timezone.now() + timedelta(days=1) + event = Event.objects.create( + makerspace=space, + title="Safety induction", + starts_at=starts_at, + ends_at=starts_at + timedelta(hours=1), + is_public=True, + status=Event.Status.PUBLISHED, + ) + + response = _client(_user("event-outsider")).post( + reverse("public-event-register", args=[space.slug, event.public_token]), + {}, + format="json", + ) + + assert response.status_code == 403 + assert response.data["code"] == "membership_required" diff --git a/backend/tests/test_telegram_integration.py b/backend/tests/test_telegram_integration.py index e2b51adf..09eebe91 100644 --- a/backend/tests/test_telegram_integration.py +++ b/backend/tests/test_telegram_integration.py @@ -61,12 +61,21 @@ def test_telegram_webhook_throttles_rapid_requests(settings, monkeypatch): HTTP_X_TELEGRAM_BOT_API_SECRET_TOKEN=WEBHOOK_SECRET, ) - assert first.status_code == 403 + # 200 now, not 403: the callback is acknowledged and discarded rather than + # rejected during actor resolution, which no longer happens. + assert first.status_code == 200 assert second.status_code == 429 @override_settings(TELEGRAM_WEBHOOK_SECRET=WEBHOOK_SECRET) -def test_telegram_accept_callback_routes_through_workflow(monkeypatch): +def test_an_accept_callback_no_longer_changes_request_state(monkeypatch): + """Chat is a notification channel, not a decision surface. + + This replaces `test_telegram_accept_callback_routes_through_workflow`. The actor + resolution and RBAC checks it used to assert went with the route: there is nothing + left to authorize, so the property worth pinning is that a well-formed, correctly + secreted accept callback from a genuinely authorized admin still moves nothing. + """ makerspace = make_space("telegram") admin = make_member("telegram-admin", makerspace) admin.telegram_user_id = "42" @@ -89,13 +98,18 @@ def test_telegram_accept_callback_routes_through_workflow(monkeypatch): HTTP_X_TELEGRAM_BOT_API_SECRET_TOKEN=WEBHOOK_SECRET, ) + # 200, not 403: an already-registered webhook must be acknowledged rather than + # retried for hours by Telegram. assert response.status_code == 200 + assert response.data["detail"] == "Ignored." hardware_request.refresh_from_db() - assert hardware_request.status == HardwareRequest.Status.ACCEPTED + assert hardware_request.status == HardwareRequest.Status.PENDING_APPROVAL @override_settings(TELEGRAM_WEBHOOK_SECRET=WEBHOOK_SECRET) -def test_unlinked_telegram_actor_is_denied(): +def test_an_unlinked_telegram_actor_is_acknowledged_and_ignored(): + """Previously 403 via actor resolution. There is no actor to resolve now, and an + unauthenticated stranger who guessed the secret still cannot make anything happen.""" response = authenticated_client( User.objects.create_user( username="placeholder", @@ -109,7 +123,8 @@ def test_unlinked_telegram_actor_is_denied(): HTTP_X_TELEGRAM_BOT_API_SECRET_TOKEN=WEBHOOK_SECRET, ) - assert response.status_code == 403 + assert response.status_code == 200 + assert response.data["detail"] == "Ignored." @override_settings(TELEGRAM_WEBHOOK_SECRET=WEBHOOK_SECRET) @@ -131,30 +146,6 @@ def test_telegram_webhook_rejects_missing_or_wrong_secret(): assert response.status_code == 403 -@override_settings(TELEGRAM_WEBHOOK_SECRET=WEBHOOK_SECRET) -def test_suspended_telegram_actor_cannot_act(): - makerspace = make_space("telegram-suspended") - admin = make_member("telegram-suspended-admin", makerspace) - admin.telegram_user_id = "77" - admin.access_status = User.AccessStatus.SUSPENDED - admin.save(update_fields=["telegram_user_id", "access_status"]) - product = make_product(makerspace) - hardware_request = make_accepted_request(makerspace, product, 1) - hardware_request.status = HardwareRequest.Status.PENDING_APPROVAL - hardware_request.save(update_fields=["status"]) - - response = authenticated_client(admin).post( - WEBHOOK_URL, - {"callback_query": {"from": {"id": 77}, "data": f"accept:{hardware_request.id}"}}, - format="json", - HTTP_X_TELEGRAM_BOT_API_SECRET_TOKEN=WEBHOOK_SECRET, - ) - - assert response.status_code == 403 - hardware_request.refresh_from_db() - assert hardware_request.status == HardwareRequest.Status.PENDING_APPROVAL - - def test_suspended_user_cannot_send_telegram_test_alert(): makerspace = make_space("telegram-test-alert") admin = make_member("telegram-alert-admin", makerspace) @@ -276,7 +267,10 @@ def test_submitted_request_telegram_alert_includes_contact_and_items( assert "Bench Meter: 2" in text assert "Logic Analyzer: 3" in text assert "parse_mode" not in sent.call_args.kwargs - assert sent.call_args.kwargs["reply_markup"]["inline_keyboard"] + # No inline keyboard: an accept/reject button IS an action surface, and chat is + # not one any more. The alert now points staff at the console instead. + assert "reply_markup" not in sent.call_args.kwargs + assert "Review and decide in the staff console." in text def test_submitted_request_telegram_delivery_error_is_swallowed( @@ -343,7 +337,10 @@ def test_submitted_request_telegram_message_stays_within_limit( text = sent.call_args.args[1] assert len(text) <= 4096 - assert sent.call_args.kwargs["reply_markup"]["inline_keyboard"] + # No inline keyboard: an accept/reject button IS an action surface, and chat is + # not one any more. The alert now points staff at the console instead. + assert "reply_markup" not in sent.call_args.kwargs + assert "Review and decide in the staff console." in text @override_settings(TELEGRAM_WEBHOOK_SECRET="") diff --git a/docker-compose.dev.yml b/docker-compose.dev.yml index fb70b422..b89381eb 100644 --- a/docker-compose.dev.yml +++ b/docker-compose.dev.yml @@ -47,17 +47,30 @@ services: # Migrations run from the mounted tree, so a migration you just wrote applies # without rebuilding the image. + # These three inherit `DEBUG: ${DEBUG:-False}` from the base &backend-env anchor -- only + # `backend` above overrides DEBUG to True. Since the trusted-proxy guard landed in settings.py + # ("TRUSTED_PROXY_COUNT must be explicitly set when DEBUG is False"), that made every one of + # them crash on boot and took `dev-docker.sh up --build` with them. Declared here rather than + # defaulted in the base anchor on purpose: the guard exists so a PRODUCTION topology has to be + # stated explicitly, and a base-level default would silently satisfy it. None of these three + # serves HTTP, and dev is direct access, so 0 is the honest value. migrate: + environment: + TRUSTED_PROXY_COUNT: "0" volumes: - ./backend:/app # Celery has no autoreload: restart these two after changing task code # (`./scripts/dev-docker.sh restart worker beat`). worker: + environment: + TRUSTED_PROXY_COUNT: "0" volumes: - ./backend:/app beat: + environment: + TRUSTED_PROXY_COUNT: "0" volumes: - ./backend:/app diff --git a/docs/DEV-WORKFLOW.md b/docs/DEV-WORKFLOW.md index a71dcff8..3df07531 100644 --- a/docs/DEV-WORKFLOW.md +++ b/docs/DEV-WORKFLOW.md @@ -152,10 +152,20 @@ The default path needs nothing installed on the host but Docker. Migrations run # After changing package.json, recreate the node_modules volume: ./scripts/dev-docker.sh up -d --build -V frontend -# Tests -./scripts/dev-docker.sh exec backend pytest +# Tests. The DATABASE_URL override is REQUIRED: the backend container runs as the +# least-privilege `spaceworks_app` role, which has no CREATEDB, so pytest cannot build +# its test database as itself (every django_db test errors in setup without this). +./scripts/dev-docker.sh exec -e DATABASE_URL=postgres://makerspace:makerspace@db:5432/makerspace_manager \ + -T backend pytest ``` +`tests/backup` and `tests/tenant_migration` are **Docker-only in practice**: they need a PostgreSQL +client whose major equals the server's (16), which `postgres_client.client_binary` looks for under +`/usr/lib/postgresql/{major}/bin` or `/usr/pgsql-{major}/bin`. Neither path exists on Arch, so on the +host they all refuse with `PostgresClientUnavailable` — the environment, not a regression. The backend +image installs client 16 *and* 17 on purpose (`pg_dump` must be >= every supported source server; +`pg_restore` must be <= the target, because 17+ emits a `transaction_timeout` GUC that 16 rejects). + Host fallback (faster `pytest` / one-off `manage.py`; needs `backend/.venv` + `npm install`): ```bash diff --git a/docs/INVARIANTS.md b/docs/INVARIANTS.md index 5fb2f909..48d0bb0d 100644 --- a/docs/INVARIANTS.md +++ b/docs/INVARIANTS.md @@ -1053,6 +1053,77 @@ from `index.css` and `tailwind.config.ts`. **Console parity principle.** Every backend lifecycle capability reachable in the Django `/control/` admin must have a React staff-console surface — a capability with no console surface is a latent dead/broken feature for normal staff. New workflow actions ship their staff UI in the same batch. + +**Who may submit a borrow request is DERIVED, and `membership` + account-less requests is an impossible +pair.** `apps/makerspaces/request_access.py` is the only answer to the question. Three states fall out of +one column and one module: `membership` on → **members**; both off → **accounts** (any active signed-in +account); `anonymous_requests_enabled` on with `membership` off → **anyone**. The fourth combination must +not exist, because `RequestSubmitView` takes its anonymous branch *before* any membership guard runs, so a +row carrying both would walk a stranger straight past the membership requirement the operator had just +switched on. It is enforced at three depths and all three are load-bearing: `Makerspace.save()` reconciles +the pair so **no writer** can persist it (module install/uninstall, `apply_profile`, the `/control/` +capability matrix, `setup_instance`, `seed_demo`, a bare `obj.save()`) — and it appends the field to +`update_fields` so a partial save cannot leave the row inconsistent; `set_anonymous_requests` takes the row +lock and **refuses** an explicit request to open account-less submission while `membership` is installed +(the module path *forces*, the operator path *refuses* — the operator was asking about this flag, so +silently ignoring them would be the wrong answer); and `anonymous_requests_allowed` re-derives at request +time so a row from raw SQL or an old restore still fails closed. Migration +`makerspaces/0067` closed the pair on existing rows and is deliberately **not reversible**. Turning +`membership` off never re-opens account-less requests — opening an unauthenticated write surface is an +explicit act, never a side effect. + +**A core module must work with every optional module uninstalled.** Two halves. The *declared* half is +checked at import time: `module_registry._validate_registry` refuses a registry where an `is_core` module +names a non-core key in `requires_modules`. The *undeclared* half — core code reaching for a row an optional +module owns — has no sound static check in this codebase, because an app label is not the unit of module +ownership (`hardware_requests` owns core `request_workflow` **and** optional `guest_handover`; `admin_api` +owns core `staff_admin` and many optional surfaces), so no per-file rule can tell a legitimate +optional-module gate from a genuine violation. It is covered behaviourally instead, by +`tests/makerspaces/test_core_module_independence.py`: the core loan spine (catalogue → submit → staff queue +→ accept → public status) is driven over HTTP on a core-only makerspace and on `core + every optional +module except M`, for every optional `M`, with the **test identity built to match each configuration** — +with `membership` installed the spine legitimately needs an active member and an open presence session, and +a test that always used a plain account would be refused before reaching the code under test and would +prove nothing. This is the regression `9e496997` shipped: core `request_workflow` called +`require_active_member_presence` unconditionally and the default `recommended` profile ships no +`membership`, so a fresh self-host install 403'd every public borrow request. Do not weaken the claim in +that file's docstring; it says what it proves and what it does not. + +**Telegram is an OUTBOUND channel. Chat is not a decision surface.** The inline accept/reject keyboard and +the callback route that served it are gone: `LifecyclePayload` has no `telegram_reply_markup`, +`telegram.send_message` has no `reply_markup` parameter, and `TelegramWebhookView` acknowledges and +discards every callback. The webhook **route is kept on purpose** — a deployment that already ran +`setWebhook` has the URL registered, we cannot call `deleteWebhook` for them, and Telegram retries a +non-2xx for hours, so a 200 is the graceful retirement and a 404 would be a permanent error loop in +someone else's infrastructure. The secret check stays even though nothing acts behind it, because an +endpoint that had quietly stopped authenticating is exactly what would turn a reintroduced callback into a +vulnerability. The one-bot-per-makerspace rule (D16) survives on its **outbound** half only — one sender +identity across a tenant's rooms, one token secret — since its original inbound justification expired with +the buttons. Reintroducing a button means per-bot webhook secrets and inbound routing first. + +**Accepting and rejecting a borrow request are one-at-a-time acts.** The `/control/` bulk `accept_selected` +and `reject_selected` actions were removed; `admin_request_review.RequestReviewAdminMixin` serves a +per-request review page instead, showing requester identity, contact-verification state and per-item +accepted quantities. Accepting reserves stock and rejecting closes a person's ask, and neither is a +judgement made about twenty rows from a checkbox column. The mutations still go through +`request_workflow`, so the state machine, audit entry and notification fan-out are unchanged — only the +entry point moved. The review URL must keep `admin_site.admin_view`, must load through +`self.get_queryset(request)` (the superadmin queryset excludes hard-hidden makerspaces), must re-check +`has_change_permission`, and must parse quantities as ints before calling the workflow. + +**The shared account-less requester principal is not a person, and no per-PERSON aggregate may rank it.** +Every anonymous submission in a makerspace points at one `Makerspace.anonymous_requester`, so grouping by +`requester_id` folds every unrelated stranger into a single fictional human. `_repeat_offenders`, +`_top_borrowers`, `globally_ranked_borrowers` and `_restricted_requesters` exclude +`anonymous_requesters.anonymous_requester_ids(...)` **before** any limit or slice, so the principal cannot +displace a real borrower from the top N. The excluded damage and loss is not dropped: `accountability_data` +reports it as an additive `anonymous_accountability` total, because an accountability panel that silently +lost every account-less incident would be worse than one naming a fake person. Per-request rows keep +showing, but route their `requester_username` through `display.requester_label` so an overdue account-less +loan shows the contact the borrower gave rather than the principal's internal `member_`. Restricting +the principal is refused at the write side by +`accounts.principal_guards.refuse_anonymous_requester_access_mutation` — it would restrict every future +account-less requester at once — and the read-side exclusion is the backstop for rows predating that guard. ## Handover roles and the retired Guest Admin **Guest Admin is no longer a built-in role** (migration `makerspaces/0052`); handover staff get a **custom @@ -1632,12 +1703,14 @@ Load-bearing details that carried over unchanged: - **Registering for an event does NOT require a `PresenceSession`; check-in does.** `require_active_member` (identity + membership + waiver) was split out of - `require_active_member_presence`, and **only** `PublicEventRegistrationView` switched. The other - **nine invocations across six surfaces** — self-checkout ×3, direct handout, public request submit, - public booking, and the two machine-service surfaces — still require a session, because those are - hardware and facility acts where "is this member here right now" is the whole question. Signing up - is planning to attend; presence is proven later by the staff-scanned QR, which is stronger evidence - than a self-declared session. + `require_active_member_presence`, and **only** `PublicEventRegistrationView` switched wholesale. The + hardware and facility invocations — self-checkout ×3, direct handout, public booking, and the two + machine-service surfaces — still require a session, because "is this member here right now" is the + whole question. Request submission also keeps that guard while `membership` is enabled; with + `membership` off it uses the separate identity-only guard because a request is only a proposal and + staff acceptance is the control. There is no waiver check on that branch because waiver acceptance + can only be recorded on a membership row. Signing up for an event is planning to attend; presence is + proven later by the staff-scanned QR, which is stronger evidence than a self-declared session. - **`events.member_history.registrations_for_space` is the ONE answer to "which registrations does this member hold in this space"**, shared by the profile counts, the profile's recent-attended list, member activity and the QR lookup. It filters on **durable provenance** diff --git a/docs/LANDING-PAGE-FEATURES.md b/docs/LANDING-PAGE-FEATURES.md index 364142f6..e76c0795 100644 --- a/docs/LANDING-PAGE-FEATURES.md +++ b/docs/LANDING-PAGE-FEATURES.md @@ -293,7 +293,7 @@ supported installation without them. | Membership | Join requests, waivers, referrals, profiles, directory and activity | People can still borrow, but enrolment and community features disappear | | Notifications | In-app inbox and unread state | No in-app alerts; separately installed outbound channels can still send | | Email | Makerspace email through its SMTP account | No tenant email; platform recovery and verification mail still sends | -| Telegram | Group alerts, test delivery and accept/reject buttons | No Telegram alerts or chat-based request decisions | +| Telegram | Group alerts and test delivery | No Telegram alerts | | Slack | Slack incoming-webhook destination | No Slack notification surface | | Mattermost | Mattermost incoming-webhook destination | No Mattermost notification surface | | Discord | Discord incoming-webhook destination | No Discord notification surface | diff --git a/docs/MODULES.md b/docs/MODULES.md index 58cbebef..f4a9c1bb 100644 --- a/docs/MODULES.md +++ b/docs/MODULES.md @@ -86,6 +86,13 @@ thirteen modules under it are optional. - **Without it** — not an option. Every handover in the system — staff issue, front-desk handout, member self-checkout — records itself through this workflow, and it is the only place a request's status is allowed to change. +- **Who may submit is set by `membership`, not by this module.** Being core, this module is present in every + makerspace and therefore must work with `membership` uninstalled: members only when `membership` is on, + any signed-in account when it is off, and account-less strangers only when an operator has explicitly + opted in (`manage.py set_request_access --mode anyone`, which is refused while `membership` is installed). + See the `membership` module below, and **Who may submit a borrow request** in `docs/INVARIANTS.md`. +- **Accept and reject are one request at a time.** There is no bulk accept or bulk reject anywhere — + `/control/` serves a per-request review page, and Telegram alerts carry no buttons. - **Data** — core; not separately purgeable. ### staff_admin @@ -310,6 +317,14 @@ Required by `printing`. no directory. `payments.membership` becomes inert. - **Deliberately does not require `member_accounts`.** Identity can come from external OIDC or a staff-created person record, so the two are independent switches. +- **It decides who may submit a borrow request, and installing it closes account-less requests.** With this + module on, submitting requires an active member of the makerspace. With it off, any signed-in account may + submit — a request is only a proposal that staff must accept, and waiver acceptance cannot be recorded + without a membership row anyway. Turning it ON therefore forces `anonymous_requests_enabled` OFF (audited, + never silent): the account-less path bypasses the membership check entirely, so leaving both on would let + a stranger walk past the requirement you just switched on. Turning it back off does **not** re-open + account-less requests — that is an explicit choice, made with + `manage.py set_request_access --mode anyone`. - **Data** — purgeable: join requests and member profiles with their projects and imagery. Memberships, waivers and acceptance evidence **stay** — they are core RBAC and liability state. @@ -361,11 +376,13 @@ stored credential**, so re-enabling needs no re-entry. ### telegram -- **What it is** — per-makerspace Telegram group alerts, and the accept/reject buttons on them. -- **What it adds** — the `telegram_alerts` workflow, the webhook that processes callbacks, and test - alerts. Callbacks route through the request workflow like every other actor. -- **Without it** — no Telegram alerts and no accept/reject from chat; the same decisions are made in the - console. +- **What it is** — per-makerspace Telegram group alerts. Outbound only. +- **What it adds** — the `telegram_alerts` workflow and test alerts. **No accept/reject buttons**: chat is + a notification channel, not a decision surface, so an alert names the request and points staff at the + console. The webhook route survives but acknowledges and discards every callback, which stops Telegram + retrying against a deployment that already ran `setWebhook`. +- **Without it** — no Telegram alerts. Request decisions are unaffected; they are made in the staff console + or `/control/` either way. - **Data** — purgeable: Telegram destinations and their chat ids. Delivery logs survive — the record that a message was attempted is history; the credential is the secret. diff --git a/docs/PROJECT-STATUS.md b/docs/PROJECT-STATUS.md index 1b0e6f46..bea08440 100644 --- a/docs/PROJECT-STATUS.md +++ b/docs/PROJECT-STATUS.md @@ -80,4 +80,5 @@ Stack (in use): `frontend/src/generated/api.ts`; regenerate both when routes/models change — spectacular needs `--format openapi-json`). - **Admin theme:** django-unfold; site name via `ADMIN_SITE_NAME` (default "Space Works"). -- **Telegram:** request alerts, test alerts, authenticated webhook accept/reject callbacks. +- **Telegram:** request alerts and test alerts. Outbound only — the webhook acknowledges and discards + callbacks; decisions are made in the staff console or `/control/`. diff --git a/frontend/openapi-schema.json b/frontend/openapi-schema.json index 59bb7a0f..6c7f72f8 100644 --- a/frontend/openapi-schema.json +++ b/frontend/openapi-schema.json @@ -2,7 +2,7 @@ "openapi": "3.0.3", "info": { "title": "Space Works API", - "version": "0.7.5", + "version": "0.8.0", "description": "Multi-tenant makerspace hardware loan system.\n\nPublic flow: browse inventory, search with `q`, page with `page`, sign in as a member, submit a borrow request, then track it by public token.\n\nAdmin flow: authenticate with JWT, manage makerspaces, inventory, staff, QR labels, bulk imports, request review, issue, and return.\n\nAuthentication: staff/admin endpoints use `Authorization: Bearer `. Public browser endpoints can use `X-Publishable-Key` when public key hardening is enabled. Server API clients send `X-Client-Id`, `X-Timestamp`, `X-Nonce`, and `X-Signature`. Generate a unique, unpredictable `X-Nonce` for every request and sign the byte sequence `METHOD\\nFULL_PATH\\nTIMESTAMP\\nNONCE\\nBODY` with HMAC-SHA256. The nonce uses 1-128 characters from `A-Z`, `a-z`, `0-9`, `.`, `_`, `~`, and `-`. A deployment may temporarily accept the legacy nonce-less signed format while `APICLIENT_REQUIRE_NONCE` is disabled." }, "paths": { @@ -29203,7 +29203,8 @@ "/api/v1/integrations/telegram/webhook": { "post": { "operationId": "api_v1_integrations_telegram_webhook_create", - "summary": "Receive Telegram callback webhook", + "description": "Retained so an already-registered webhook does not retry forever. Callback queries are acknowledged and discarded: accepting and rejecting borrow requests happens in the staff console, never from chat.", + "summary": "Acknowledge a Telegram webhook (no action is taken)", "tags": [ "Telegram" ], @@ -29228,7 +29229,7 @@ }, "responses": { "200": { - "description": "Webhook processed." + "description": "Acknowledged; no action taken." } } } @@ -33157,6 +33158,14 @@ "operationId": "api_v1_public_requests_create", "summary": "Submit public borrow request", "parameters": [ + { + "in": "header", + "name": "Idempotency-Key", + "schema": { + "type": "string" + }, + "description": "Required for account-less submissions. Reusing a key with the same payload returns the original request; a different payload is rejected." + }, { "in": "header", "name": "X-Nonce", @@ -33194,7 +33203,7 @@ "examples": { "SubmitPublicEquipmentRequest": { "value": { - "requester_name": "Shaan Shoukath", + "contact_name": "Shaan Shoukath", "contact_email": "shaans@example.com", "contact_phone": "+919876543210", "requested_for": "Electronics workshop diagnostics", @@ -33225,7 +33234,8 @@ "security": [ { "jwtAuth": [] - } + }, + {} ], "responses": { "201": { @@ -34683,6 +34693,10 @@ "type": "string", "readOnly": true }, + "requester_contact_verified": { + "type": "boolean", + "readOnly": true + }, "status": { "type": "string", "readOnly": true @@ -34787,6 +34801,7 @@ "requested_for", "requester_contact_email", "requester_contact_phone", + "requester_contact_verified", "requester_display", "requester_name", "requester_username", @@ -51844,6 +51859,13 @@ "detail" ] }, + "RequestAccessEnum": { + "enum": [ + "anyone" + ], + "type": "string", + "description": "* `anyone` - anyone" + }, "RequestItemInput": { "type": "object", "properties": { @@ -51852,6 +51874,7 @@ }, "quantity": { "type": "integer", + "maximum": 99, "minimum": 1 } }, @@ -51867,15 +51890,40 @@ "type": "string", "writeOnly": true }, + "contact_name": { + "type": "string", + "description": "Required for an account-less submission.", + "maxLength": 200 + }, + "contact_email": { + "description": "Required for an account-less submission; normalized to lowercase.", + "oneOf": [ + { + "type": "string", + "format": "email", + "maxLength": 254 + }, + { + "type": "string", + "maxLength": 0 + } + ] + }, + "contact_phone": { + "type": "string", + "maxLength": 32 + }, "requested_for": { "type": "string", - "default": "" + "default": "", + "maxLength": 500 }, "items": { "type": "array", "items": { "$ref": "#/components/schemas/RequestItemInput" - } + }, + "maxItems": 20 } }, "required": [ @@ -54029,6 +54077,9 @@ }, "membership_policy": { "$ref": "#/components/schemas/MembershipPolicyEnum" + }, + "request_access": { + "$ref": "#/components/schemas/RequestAccessEnum" } }, "required": [ @@ -55238,4 +55289,4 @@ "description": "Persistent staff inbox notifications." } ] -} +} \ No newline at end of file diff --git a/frontend/public/fonts/InstrumentSans-Variable.woff2 b/frontend/public/fonts/InstrumentSans-Variable.woff2 deleted file mode 100644 index 8611e41b..00000000 Binary files a/frontend/public/fonts/InstrumentSans-Variable.woff2 and /dev/null differ diff --git a/frontend/src/components/ThemeToggle.tsx b/frontend/src/components/ThemeToggle.tsx index 9db5e273..01c798f7 100644 --- a/frontend/src/components/ThemeToggle.tsx +++ b/frontend/src/components/ThemeToggle.tsx @@ -1,5 +1,7 @@ import { useEffect, useState } from "react"; import { readStorage, writeStorage } from "../lib/safeStorage"; +import { MoonIcon, SunIcon } from "./icons"; +import { IconButton } from "./ui/IconButton"; const THEME_KEY = "makerspace.theme"; @@ -7,7 +9,7 @@ function applyTheme(theme: "light" | "dark") { document.documentElement.classList.toggle("dark", theme === "dark"); } -export function ThemeToggle() { +export function ThemeToggle({ variant = "text" }: { variant?: "text" | "icon" }) { const [theme, setTheme] = useState<"light" | "dark">(() => { const stored = readStorage(THEME_KEY); return stored === "dark" ? "dark" : "light"; @@ -18,6 +20,17 @@ export function ThemeToggle() { writeStorage(THEME_KEY, theme); }, [theme]); + if (variant === "icon") { + return ( + setTheme((current) => (current === "dark" ? "light" : "dark"))} + > + {theme === "dark" ? : } + + ); + } + return ( + + + + ) : ( + + )} + + + , + document.body, + ); +} diff --git a/frontend/src/components/ui/DataTable.tsx b/frontend/src/components/ui/DataTable.tsx index c6aa2bd4..09ab4b22 100644 --- a/frontend/src/components/ui/DataTable.tsx +++ b/frontend/src/components/ui/DataTable.tsx @@ -72,7 +72,7 @@ export function DataTable({ {selectionEnabled ? ( - + changeAll(event.target.checked)} /> ) : null} @@ -81,6 +81,7 @@ export function DataTable({ key={column.key} className={`px-3 py-2 font-semibold ${column.className ?? ""}`} aria-sort={sort?.key === column.key ? ariaSort(sort.direction) : undefined} + scope="col" > {column.sortable ? ( + ); +} + +export function IconLink({ children, className, label, ...props }: IconLinkProps) { + return ( + + {children} + + ); +} diff --git a/frontend/src/components/ui/Metric.tsx b/frontend/src/components/ui/Metric.tsx new file mode 100644 index 00000000..efc65faa --- /dev/null +++ b/frontend/src/components/ui/Metric.tsx @@ -0,0 +1,12 @@ +export function Metric({ label, value, danger = false }: { + label: string; + value?: string | number; + danger?: boolean; +}) { + return ( +
+

{label}

+

{value ?? 0}

+
+ ); +} diff --git a/frontend/src/components/ui/Modal.tsx b/frontend/src/components/ui/Modal.tsx index eee8177c..d0240d59 100644 --- a/frontend/src/components/ui/Modal.tsx +++ b/frontend/src/components/ui/Modal.tsx @@ -1,6 +1,14 @@ import type React from "react"; -import { useEffect, useId, useRef } from "react"; -import { focusFirstDialogElement, trapDialogFocus } from "./dialogFocus"; +import { useEffect, useId, useRef, useSyncExternalStore } from "react"; +import { + consumePendingRestore, + focusFirstDialogElement, + getSnapshot, + popDialog, + pushDialog, + subscribe, + trapDialogFocus, +} from "./dialogFocus"; type ModalProps = { open: boolean; @@ -22,7 +30,13 @@ export function Modal({ backdrop = "plain", }: ModalProps) { const titleId = useId(); + const layerRef = useRef(null); const panelRef = useRef(null); + const tokenRef = useRef(null); + const top = useSyncExternalStore(subscribe, getSnapshot); + const amTop = top === tokenRef.current; + const amTopRef = useRef(amTop); + amTopRef.current = amTop; const maxWidthClass = size === "xl" ? "max-w-4xl" : "max-w-lg"; const backdropClass = backdrop === "blur" ? "bg-ink/35 backdrop-blur-sm" : "bg-ink/40"; @@ -36,10 +50,16 @@ export function Modal({ useEffect(() => { if (!open) return; const previousFocus = document.activeElement instanceof HTMLElement ? document.activeElement : null; + const token = pushDialog({ + previousFocus, + getPanel: () => panelRef.current, + getLayer: () => layerRef.current, + }); + tokenRef.current = token; const panel = panelRef.current; - if (panel) focusFirstDialogElement(panel); const handleKeyDown = (event: KeyboardEvent) => { + if (!amTopRef.current) return; if (event.key === "Escape") onCloseRef.current(); if (panel) trapDialogFocus(event, panel); }; @@ -47,14 +67,29 @@ export function Modal({ document.addEventListener("keydown", handleKeyDown); return () => { document.removeEventListener("keydown", handleKeyDown); - previousFocus?.focus(); + popDialog(token); + if (tokenRef.current === token) tokenRef.current = null; }; }, [open]); + useEffect(() => { + if (!amTop) return; + consumePendingRestore(panelRef.current); + // Initial focus lives HERE, not in the push effect. A dialog that opens while another sits + // above it renders with `inert` on its layer, and `inert` is only cleared by the re-render + // that follows becoming topmost -- so focusing from the push effect silently fails, and + // consumePendingRestore no-ops because nothing was popped. This effect runs post-commit, + // once `inert` is gone. jsdom implements no `inert` at all, so no unit test can catch this. + const panel = panelRef.current; + if (panel && !panel.contains(document.activeElement)) focusFirstDialogElement(panel); + }, [amTop]); + if (!open) return null; return (
{ if (event.target === event.currentTarget) { @@ -65,7 +100,7 @@ export function Modal({
{ export default function QrScanner({ onScan, onClose }: QrScannerProps) { const titleId = useId(); + const layerRef = useRef(null); const panelRef = useRef(null); + const tokenRef = useRef(null); + const top = useSyncExternalStore(subscribe, getSnapshot); + const amTop = top === tokenRef.current; + const amTopRef = useRef(amTop); + amTopRef.current = amTop; const videoRef = useRef(null); const streamRef = useRef(null); const intervalRef = useRef(null); @@ -59,10 +73,16 @@ export default function QrScanner({ onScan, onClose }: QrScannerProps) { }, [onScan]); useEffect(() => { const previousFocus = document.activeElement instanceof HTMLElement ? document.activeElement : null; + const token = pushDialog({ + previousFocus, + getPanel: () => panelRef.current, + getLayer: () => layerRef.current, + }); + tokenRef.current = token; const panel = panelRef.current; - if (panel) focusFirstDialogElement(panel); const handleKeyDown = (event: KeyboardEvent) => { + if (!amTopRef.current) return; if (event.key === "Escape") close(); if (panel) trapDialogFocus(event, panel); }; @@ -70,10 +90,23 @@ export default function QrScanner({ onScan, onClose }: QrScannerProps) { document.addEventListener("keydown", handleKeyDown); return () => { document.removeEventListener("keydown", handleKeyDown); - previousFocus?.focus(); + popDialog(token); + if (tokenRef.current === token) tokenRef.current = null; }; }, []); + useEffect(() => { + if (!amTop) return; + consumePendingRestore(panelRef.current); + // Initial focus lives HERE, not in the push effect. A dialog that opens while another sits + // above it renders with `inert` on its layer, and `inert` is only cleared by the re-render + // that follows becoming topmost -- so focusing from the push effect silently fails, and + // consumePendingRestore no-ops because nothing was popped. This effect runs post-commit, + // once `inert` is gone. jsdom implements no `inert` at all, so no unit test can catch this. + const panel = panelRef.current; + if (panel && !panel.contains(document.activeElement)) focusFirstDialogElement(panel); + }, [amTop]); + useEffect(() => { let cancelled = false; @@ -179,8 +212,8 @@ export default function QrScanner({ onScan, onClose }: QrScannerProps) { }; return createPortal( -
{ if (event.target === event.currentTarget) close(); }}> -
+
{ if (event.target === event.currentTarget) close(); }}> +

QR scanner

{error ? (
diff --git a/frontend/src/components/ui/dialogFocus.test.ts b/frontend/src/components/ui/dialogFocus.test.ts new file mode 100644 index 00000000..645919f3 --- /dev/null +++ b/frontend/src/components/ui/dialogFocus.test.ts @@ -0,0 +1,122 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; + +import { + consumePendingRestore, + getSnapshot, + isTopmost, + popDialog, + pushDialog, +} from "./dialogFocus"; + +const tokens: symbol[] = []; + +function addLayer(zIndex = "0") { + const layer = document.createElement("div"); + layer.style.zIndex = zIndex; + document.body.append(layer); + return layer; +} + +function pushLayer(layer: HTMLElement, previousFocus: HTMLElement | null = null) { + const panel = document.createElement("div"); + layer.append(panel); + const token = pushDialog({ + previousFocus, + getPanel: () => panel, + getLayer: () => layer, + }); + tokens.push(token); + return token; +} + +afterEach(() => { + while (tokens.length) popDialog(tokens.pop()!); + consumePendingRestore(null); + document.body.replaceChildren(); + vi.restoreAllMocks(); +}); + +describe("dialog focus store", () => { + it("tracks the top through push and order-independent, idempotent pops", () => { + const first = pushLayer(addLayer("10")); + const second = pushLayer(addLayer("20")); + + expect(getSnapshot()).toBe(second); + expect(isTopmost(second)).toBe(true); + + popDialog(first); + popDialog(first); + popDialog(Symbol("unknown")); + expect(getSnapshot()).toBe(second); + + popDialog(second); + expect(getSnapshot()).toBeNull(); + }); + + it("uses document order to break equal-z ties", () => { + const earlierLayer = addLayer("100"); + const laterLayer = addLayer("100"); + + const later = pushLayer(laterLayer); + pushLayer(earlierLayer); + + expect(getSnapshot()).toBe(later); + }); + + it("lets a higher z-index win regardless of push order", () => { + const highLayer = addLayer("200"); + const lowLayer = addLayer("50"); + + const high = pushLayer(highLayer); + pushLayer(lowLayer); + + expect(getSnapshot()).toBe(high); + }); + + it("does not queue focus restoration when a non-top entry is popped", () => { + const lowerOpener = document.createElement("button"); + const currentFocus = document.createElement("button"); + const topPanel = document.createElement("div"); + const topFirst = document.createElement("button"); + document.body.append(lowerOpener, currentFocus); + + const lower = pushLayer(addLayer("10"), lowerOpener); + const topLayer = addLayer("20"); + topPanel.append(topFirst); + topLayer.append(topPanel); + const top = pushDialog({ + previousFocus: null, + getPanel: () => topPanel, + getLayer: () => topLayer, + }); + tokens.push(top); + + currentFocus.focus(); + popDialog(lower); + consumePendingRestore(topPanel); + + expect(document.activeElement).toBe(currentFocus); + }); + + it("restores a connected opener immediately when the last dialog pops", () => { + const opener = document.createElement("button"); + document.body.append(opener); + const token = pushLayer(addLayer("10"), opener); + + popDialog(token); + + expect(document.activeElement).toBe(opener); + }); + + it("falls back to document.body when the last dialog's opener is detached", () => { + const opener = document.createElement("button"); + document.body.append(opener); + const token = pushLayer(addLayer("10"), opener); + opener.remove(); + const bodyFocus = vi.spyOn(document.body, "focus"); + + popDialog(token); + + expect(bodyFocus).toHaveBeenCalledOnce(); + }); +}); diff --git a/frontend/src/components/ui/dialogFocus.ts b/frontend/src/components/ui/dialogFocus.ts index 08e77c9a..45574c9a 100644 --- a/frontend/src/components/ui/dialogFocus.ts +++ b/frontend/src/components/ui/dialogFocus.ts @@ -31,3 +31,106 @@ export function trapDialogFocus(event: KeyboardEvent, panel: HTMLElement) { first.focus(); } } + +export type DialogEntry = { + token: symbol; + previousFocus: HTMLElement | null; + getPanel: () => HTMLElement | null; + getLayer: () => HTMLElement | null; +}; + +let stack: DialogEntry[] = []; +const listeners = new Set<() => void>(); +let topToken: symbol | null = null; +let pendingRestore: { previousFocus: HTMLElement | null } | null = null; + +function recomputeTop() { + let topEntry: DialogEntry | null = null; + let topLayer: HTMLElement | null = null; + let topZIndex = Number.NEGATIVE_INFINITY; + + for (const entry of stack) { + const layer = entry.getLayer(); + if (!layer?.isConnected) continue; + + const parsedZIndex = Number.parseFloat(getComputedStyle(layer).zIndex); + const zIndex = Number.isNaN(parsedZIndex) ? 0 : parsedZIndex; + const followsCurrent = + topLayer !== null && + Boolean(topLayer.compareDocumentPosition(layer) & Node.DOCUMENT_POSITION_FOLLOWING); + + if (topEntry === null || zIndex > topZIndex || (zIndex === topZIndex && followsCurrent)) { + topEntry = entry; + topLayer = layer; + topZIndex = zIndex; + } + } + + topToken = topEntry?.token ?? null; +} + +function notifyListeners() { + listeners.forEach((listener) => listener()); +} + +function restoreFocus(previousFocus: HTMLElement | null) { + if (previousFocus?.isConnected) { + previousFocus.focus(); + } else { + document.body.focus(); + } +} + +export function subscribe(callback: () => void) { + listeners.add(callback); + return () => listeners.delete(callback); +} + +export function getSnapshot() { + return topToken; +} + +export function pushDialog(entry: Omit) { + const token = Symbol("dialog"); + stack.push({ ...entry, token }); + recomputeTop(); + notifyListeners(); + return token; +} + +export function popDialog(token: symbol) { + const index = stack.findIndex((entry) => entry.token === token); + if (index === -1) return; + + const wasTopmost = topToken === token; + const [entry] = stack.splice(index, 1); + recomputeTop(); + + if (wasTopmost) { + if (topToken === null) { + pendingRestore = null; + restoreFocus(entry.previousFocus); + } else { + pendingRestore = { previousFocus: entry.previousFocus }; + } + } + + notifyListeners(); +} + +export function isTopmost(token: symbol) { + return topToken === token; +} + +export function consumePendingRestore(panel: HTMLElement | null) { + const pending = pendingRestore; + if (!pending) return; + pendingRestore = null; + + const previousFocus = pending.previousFocus; + if (panel && previousFocus?.isConnected && panel.contains(previousFocus)) { + previousFocus.focus(); + return; + } + if (panel) focusFirstDialogElement(panel); +} diff --git a/frontend/src/components/ui/dialogStack.test.tsx b/frontend/src/components/ui/dialogStack.test.tsx new file mode 100644 index 00000000..e8344574 --- /dev/null +++ b/frontend/src/components/ui/dialogStack.test.tsx @@ -0,0 +1,119 @@ +import { fireEvent, render, screen, waitFor } from "@testing-library/react"; +import { useState } from "react"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +import { Modal } from "./Modal"; +import QrScanner from "./QrScanner"; + +type DialogHarnessProps = { + openerOutside?: boolean; + showOpener?: boolean; + showOuter?: boolean; +}; + +function DialogHarness({ + openerOutside = false, + showOpener = true, + showOuter = true, +}: DialogHarnessProps) { + const [scannerOpen, setScannerOpen] = useState(false); + + const opener = ( + + ); + + return ( + <> + + {openerOutside && showOpener ? opener : null} + undefined} title="Outer modal"> + + {!openerOutside && showOpener ? opener : null} + + {scannerOpen ? undefined} onClose={() => setScannerOpen(false)} /> : null} + + ); +} + +beforeEach(() => { + const track = { stop: vi.fn() }; + const stream = { getTracks: () => [track] } as unknown as MediaStream; + Object.defineProperty(navigator, "mediaDevices", { + configurable: true, + value: { getUserMedia: vi.fn().mockResolvedValue(stream) }, + }); + vi.spyOn(HTMLMediaElement.prototype, "play").mockResolvedValue(undefined); +}); + +afterEach(() => { + vi.restoreAllMocks(); +}); + +async function openScanner() { + const opener = screen.getByRole("button", { name: "Open scanner" }); + opener.focus(); + fireEvent.click(opener); + await screen.findByRole("dialog", { name: "QR scanner" }); + return opener; +} + +describe("dialog stack", () => { + it("closes only the scanner on Escape and restores its exact opener", async () => { + render(); + const opener = await openScanner(); + + fireEvent.keyDown(document, { key: "Escape" }); + + await waitFor(() => expect(screen.queryByRole("dialog", { name: "QR scanner" })).toBeNull()); + expect(screen.getByRole("dialog", { name: "Outer modal" })).toBeInTheDocument(); + expect(document.activeElement).toBe(opener); + }); + + it("focuses the modal's first control when the scanner opener was removed", async () => { + const view = render(); + const opener = await openScanner(); + view.rerender(); + expect(opener.isConnected).toBe(false); + + fireEvent.keyDown(document, { key: "Escape" }); + + await waitFor(() => expect(document.activeElement).toBe(screen.getByRole("button", { name: "Modal fallback" }))); + expect(document.activeElement).not.toBe(screen.getByRole("button", { name: "Behind modal" })); + }); + + it("rejects an opener outside the underlying modal", async () => { + render(); + const opener = await openScanner(); + + fireEvent.keyDown(document, { key: "Escape" }); + + await waitFor(() => expect(document.activeElement).toBe(screen.getByRole("button", { name: "Modal fallback" }))); + expect(document.activeElement).not.toBe(opener); + }); + + it("keeps scanner focus when the outer modal unmounts first", async () => { + const view = render(); + await openScanner(); + const scannerControl = screen.getByRole("button", { name: "Done" }); + scannerControl.focus(); + + view.rerender(); + + expect(screen.getByRole("dialog", { name: "QR scanner" })).toBeInTheDocument(); + expect(document.activeElement).toBe(scannerControl); + expect(document.activeElement?.isConnected).toBe(true); + }); + + it("makes the covered modal layer inert and exposes only the scanner as modal", async () => { + render(); + await openScanner(); + + const modalDialog = screen.getByText("Outer modal").closest('[role="dialog"]'); + const modalLayer = modalDialog?.parentElement; + expect(modalDialog).not.toHaveAttribute("aria-modal"); + expect(modalLayer).toHaveAttribute("inert"); + expect(screen.getByRole("dialog", { name: "QR scanner" })).toHaveAttribute("aria-modal", "true"); + }); +}); diff --git a/frontend/src/components/ui/index.ts b/frontend/src/components/ui/index.ts index 7257690e..201b7418 100644 --- a/frontend/src/components/ui/index.ts +++ b/frontend/src/components/ui/index.ts @@ -1,5 +1,6 @@ export { Badge } from "./Badge"; export { BulkActionToolbar } from "./BulkActionToolbar"; +export { CameraCapture } from "./CameraCapture"; export { Card } from "./Card"; export { CollapsibleSection } from "./CollapsibleSection"; export { ConfirmDialog } from "./ConfirmDialog"; @@ -7,7 +8,12 @@ export { DataTable } from "./DataTable"; export type { DataTableColumn } from "./DataTable"; export { DetailDrawer } from "./DetailDrawer"; export { EmptyState } from "./EmptyState"; +export { ErrorBlock } from "./ErrorBlock"; +export { ErrorText } from "./ErrorText"; +export { Field } from "./Field"; export { FilterBar } from "./FilterBar"; +export { IconButton, IconLink } from "./IconButton"; +export { Metric } from "./Metric"; export { Modal } from "./Modal"; export { Skeleton, SkeletonRows } from "./Skeleton"; export { Spinner } from "./Spinner"; diff --git a/frontend/src/features/bookings/PublicBookingForm.tsx b/frontend/src/features/bookings/PublicBookingForm.tsx index 316d87a0..19d3bea1 100644 --- a/frontend/src/features/bookings/PublicBookingForm.tsx +++ b/frontend/src/features/bookings/PublicBookingForm.tsx @@ -1,5 +1,6 @@ -import { cloneElement, useEffect, useRef, useState, type FormEvent, type ReactElement } from "react"; +import { useEffect, useRef, useState, type FormEvent } from "react"; +import { Field } from "../../components/ui"; import { StructuredApiError } from "../../lib/api"; import { CustomFormFields } from "../forms/CustomFormFields"; import { @@ -110,13 +111,3 @@ export function PublicBookingForm({ makerspaceSlug, space }: {
); } - -function Field({ label, error, children }: { label: string; error: string; children: ReactElement<{ "aria-invalid"?: boolean }> }) { - return ( - - ); -} diff --git a/frontend/src/features/inventory/BorrowRequestCard.tsx b/frontend/src/features/inventory/BorrowRequestCard.tsx index c94c8927..06202991 100644 --- a/frontend/src/features/inventory/BorrowRequestCard.tsx +++ b/frontend/src/features/inventory/BorrowRequestCard.tsx @@ -12,6 +12,22 @@ type BorrowRequestCardProps = { onClear: () => void; onRequestedForChange: (value: string) => void; onSubmit: () => void; + // Account-less mode: the makerspace opted into requests without an account, so the + // borrower has no profile to read a name and email from and must supply them here. + accountLess?: boolean; + contactName?: string; + contactEmail?: string; + contactPhone?: string; + onContactNameChange?: (value: string) => void; + onContactEmailChange?: (value: string) => void; + onContactPhoneChange?: (value: string) => void; + // Honeypot, carried by every other public form (booking, printing, events). A bot that + // autofills it gets the server's decoy response instead of a real request. + website?: string; + onWebsiteChange?: (value: string) => void; + // The ONLY handle an account-less requester has on their request: their contact details + // are unverified so no lifecycle email is sent, and status lookup is by token. + publicToken?: string; }; export function BorrowRequestCard({ @@ -25,6 +41,16 @@ export function BorrowRequestCard({ onClear, onRequestedForChange, onSubmit, + accountLess = false, + contactName = "", + contactEmail = "", + contactPhone = "", + onContactNameChange, + onContactEmailChange, + onContactPhoneChange, + website = "", + onWebsiteChange, + publicToken, }: BorrowRequestCardProps) { return ( @@ -44,8 +70,9 @@ export function BorrowRequestCard({ {items.length === 0 ? (

- Add public items from the inventory list, then submit the request with - your signed-in member account. + {accountLess + ? "Add public items from the inventory list, then leave your name and email to submit the request." + : "Add public items from the inventory list, then submit the request with your signed-in member account."}

) : (
@@ -67,6 +94,59 @@ export function BorrowRequestCard({ )}
+ {accountLess ? ( +
+ + + +
+ ) : null} +
diff --git a/frontend/src/features/inventory/PublicEvidenceUpload.tsx b/frontend/src/features/inventory/PublicEvidenceUpload.tsx index e2596d06..7ab49195 100644 --- a/frontend/src/features/inventory/PublicEvidenceUpload.tsx +++ b/frontend/src/features/inventory/PublicEvidenceUpload.tsx @@ -1,5 +1,6 @@ import { useEffect, useState } from "react"; +import { CameraCapture } from "../../components/ui"; import { requestPublicEvidenceUpload, uploadPublicEvidenceFile, @@ -19,12 +20,17 @@ export function PublicEvidenceUpload({ const [status, setStatus] = useState<"idle" | "uploading" | "done" | "error">("idle"); const [error, setError] = useState(""); const [fileName, setFileName] = useState(""); + const [cameraOpen, setCameraOpen] = useState(false); + const [cameraSupported] = useState( + () => typeof navigator !== "undefined" && !!navigator.mediaDevices?.getUserMedia, + ); const label = evidenceType === "issue" ? "Issue photo" : "Return photo"; useEffect(() => { setStatus("idle"); setError(""); setFileName(""); + setCameraOpen(false); onUploaded(null); }, [evidenceType, onUploaded]); @@ -51,25 +57,46 @@ export function PublicEvidenceUpload({ return (
- - {status === "uploading" ?

Uploading {fileName}...

: null} - {status === "done" ?

Photo uploaded

: null} - {status === "error" ?

{error}

: null} +
+ + {cameraSupported ? ( + + ) : null} +
+ setCameraOpen(false)} onCapture={(file) => { void handleFile(file); }} label={label} /> + {/* ONE persistent live region, not three conditional ones: a role="status" element that is + inserted at the same moment its text appears is frequently not announced, because the + region was not in the DOM for the screen reader to observe changing. */} +

+ {status === "uploading" + ? `Uploading ${fileName}...` + : status === "done" + ? "Photo uploaded" + : status === "error" + ? error + : ""} +

); } diff --git a/frontend/src/features/inventory/PublicInventoryPage.tsx b/frontend/src/features/inventory/PublicInventoryPage.tsx index 712b47e6..3ad148c9 100644 --- a/frontend/src/features/inventory/PublicInventoryPage.tsx +++ b/frontend/src/features/inventory/PublicInventoryPage.tsx @@ -6,12 +6,10 @@ import { MakerspaceBrand } from "../../components/MakerspaceBrand"; import { MakerspaceMapLink } from "../../components/MakerspaceMapLink"; import { SpaceWorksBadge } from "../../components/SpaceWorksLogo"; import { ThemeToggle } from "../../components/ThemeToggle"; -import { Card } from "../../components/ui/Card"; +import { ChartIcon, UserIcon } from "../../components/icons"; +import { Card, Field, IconLink } from "../../components/ui"; import { useTenant, useTenantPath } from "../../lib/tenant"; -import type { - Product, - RequestCartItem, -} from "../../types/inventory"; +import type { Product, RequestCartItem } from "../../types/inventory"; import { ProductCard } from "./ProductCard"; import { ProductQuickViewModal } from "./ProductQuickViewModal"; import { @@ -24,11 +22,7 @@ import { } from "./PublicInventoryParts"; import { PublicRequestPanel } from "./PublicRequestPanel"; import { SkipLink } from "../../components/SkipLink"; -import { - usePublicCategories, - usePublicInventory, - useTenantBootstrap, -} from "./usePublicInventory"; +import { usePublicCategories, usePublicInventory, useTenantBootstrap } from "./usePublicInventory"; const PAGE_SIZE = 24; @@ -133,7 +127,7 @@ export function PublicInventoryPage() { return (
-
+

Public Inventory @@ -155,40 +149,44 @@ export function PublicInventoryPage() { className="mt-1" />

-
- -
- {inventoryQuery.data?.count ?? "-"} listed items +
+
+ {bootstrap?.makerspace.public_stats_enabled ? ( + + + + ) : null} + + + + +
+
+ +
+ {inventoryQuery.data?.count ?? "-"} listed items +
+ {modules.has("printing") ? ( + + Request a 3D print + + ) : null} + {modules.has("events") ? ( + + Events + + ) : null} + {modules.has("machines") ? ( + + Machines + + ) : null} + {modules.has("bookings") ? ( + + Book a space + + ) : null}
- {bootstrap?.makerspace.public_stats_enabled ? ( - - Stats - - ) : null} - {modules.has("printing") ? ( - - Request a 3D print - - ) : null} - {modules.has("events") ? ( - - Events - - ) : null} - {modules.has("machines") ? ( - - Machines - - ) : null} - {modules.has("bookings") ? ( - - Book a space - - ) : null} - - - Staff login -
@@ -203,16 +201,15 @@ export function PublicInventoryPage() {
-
- setSearchInput(event.target.value)} - /> + + + setSearchInput(event.target.value)} + /> + @@ -292,6 +289,7 @@ export function PublicInventoryPage() { makerspaceSlug={makerspaceSlug} onClear={() => setCart({})} disabled={!requestEnabled} + requestAccess={bootstrap?.makerspace.request_access} />
diff --git a/frontend/src/features/inventory/PublicRequestPanel.tsx b/frontend/src/features/inventory/PublicRequestPanel.tsx index 7be88366..67aa549e 100644 --- a/frontend/src/features/inventory/PublicRequestPanel.tsx +++ b/frontend/src/features/inventory/PublicRequestPanel.tsx @@ -1,9 +1,10 @@ -import { useMemo, useState } from "react"; -import { useMutation, useQueryClient } from "@tanstack/react-query"; +import { useMemo, useRef, useState } from "react"; +import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query"; import { Card } from "../../components/ui/Card"; import type { RequestCartItem } from "../../types/inventory"; import { BorrowRequestCard } from "./BorrowRequestCard"; +import { getAccessToken, refreshAccessToken } from "../../lib/api"; import { submitPublicRequest } from "./api"; import { invalidatePublicInventory } from "../staff/queryInvalidation"; import { PublicToolScanPanel } from "./PublicToolScanPanel"; @@ -15,18 +16,60 @@ type PublicRequestPanelProps = { makerspaceSlug: string; onClear: () => void; disabled?: boolean; + // The makerspace's policy, not the caller's state. Present only when the space opted + // into account-less borrow requests. + requestAccess?: "anyone"; }; +// The header is required for account-less submissions, and it is what makes a retry +// idempotent: the same key with the same payload returns the original request. Held for +// the lifetime of one composed request and rotated only after a successful submit, so a +// network retry of the SAME attempt cannot create a second request. +function newIdempotencyKey() { + const cryptoRef = globalThis.crypto; + if (cryptoRef && typeof cryptoRef.randomUUID === "function") { + return cryptoRef.randomUUID(); + } + return `req-${Date.now()}-${Math.random().toString(36).slice(2, 12)}`; +} + export function PublicRequestPanel({ items, makerspaceSlug, onClear, disabled = false, + requestAccess, }: PublicRequestPanelProps) { const queryClient = useQueryClient(); const [activeTab, setActiveTab] = useState("borrow"); const [requestedFor, setRequestedFor] = useState(""); const [submitted, setSubmitted] = useState(false); + const [contactName, setContactName] = useState(""); + const [contactEmail, setContactEmail] = useState(""); + const [contactPhone, setContactPhone] = useState(""); + const [website, setWebsite] = useState(""); + const [publicToken, setPublicToken] = useState(""); + const idempotencyKey = useRef(newIdempotencyKey()); + // A signed-in member who RELOADED this page holds no in-memory access token -- it lives + // behind the refresh cookie, and unlike `MemberArea` this page never hydrates. Without + // the probe they would be classified as anonymous and their request filed against the + // SHARED anonymous principal, which every per-person view excludes: it would vanish from + // their own activity and take the unverified-contact path instead of notifying them. + const sessionProbe = useQuery({ + queryKey: ["public-request-session", makerspaceSlug], + queryFn: async () => (getAccessToken() ? true : refreshAccessToken()), + enabled: requestAccess === "anyone" && !disabled, + staleTime: Infinity, + retry: false, + }); + // Policy AND caller state. `tenantPublicRequest` still attaches Authorization when a + // token is in memory, and the backend then takes the AUTHENTICATED branch and ignores + // these contact fields -- so asking for them would promise something the stored request + // does not honour. Until the probe settles we assume a member: claiming "no account + // needed" and then discovering a session would be the worse way round. + const authenticated = Boolean(getAccessToken()); + const accountLess = + requestAccess === "anyone" && sessionProbe.isFetched && !authenticated; const totalItems = useMemo( () => items.reduce((total, item) => total + item.quantity, 0), [items], @@ -34,18 +77,40 @@ export function PublicRequestPanel({ const submitMutation = useMutation({ mutationFn: () => - submitPublicRequest(makerspaceSlug, { - requested_for: requestedFor.trim(), - items: items.map((item) => ({ - product_id: item.productId, - quantity: item.quantity, - })), - }), + submitPublicRequest( + makerspaceSlug, + { + requested_for: requestedFor.trim(), + items: items.map((item) => ({ + product_id: item.productId, + quantity: item.quantity, + })), + website, + ...(accountLess + ? { + contact_name: contactName.trim(), + contact_email: contactEmail.trim(), + contact_phone: contactPhone.trim(), + } + : {}), + }, + accountLess ? idempotencyKey.current : undefined, + ), onSuccess: (response) => { invalidatePublicInventory(queryClient, makerspaceSlug); - void response; + // Kept before the form is cleared: this token is the account-less requester's only + // way back to the request. + setPublicToken(response?.public_token ?? ""); setSubmitted(true); onClear(); + setContactName(""); + setContactEmail(""); + setContactPhone(""); + setRequestedFor(""); + setWebsite(""); + // Only after the server accepted it: reusing the key for the NEXT request would + // return this one back instead of creating anything. + idempotencyKey.current = newIdempotencyKey(); }, }); @@ -72,9 +137,13 @@ export function PublicRequestPanel({ : `status-box min-h-11 w-full py-2 ${tone.idle}`; } + const contactReady = + !accountLess || + (contactName.trim().length > 0 && contactEmail.trim().length > 0); const canSubmit = requestedFor.trim().length > 0 && items.length > 0 && + contactReady && !submitMutation.isPending; return ( @@ -93,10 +162,22 @@ export function PublicRequestPanel({ <>

- Member borrowing + {accountLess && activeTab === "borrow" + ? "Borrow something" + : "Member borrowing"}

- Requests use your signed-in member account. An active membership, waiver acceptance, and current presence are required. + {/* Three states, because the requirements genuinely differ. Scoped to the + borrow tab: scanning a tool is self-checkout, which DOES require an + authenticated member with active presence, so "no account needed" would + be false there until it 401s. And an `anyone` policy necessarily has the + membership module off, so the membership/waiver/presence sentence cannot + be true on such a space even for a signed-in visitor. */} + {activeTab === "borrow" && accountLess + ? "No account needed. Leave your name and email so staff can reach you about the request; they review it before anything is handed over." + : activeTab === "borrow" && requestAccess === "anyone" + ? "You are signed in, so this request is filed against your account. Staff review it before anything is handed over." + : "Requests use your signed-in member account. An active membership, waiver acceptance, and current presence are required."}

@@ -140,6 +221,16 @@ export function PublicRequestPanel({ onClear={onClear} onRequestedForChange={setRequestedFor} onSubmit={() => submitMutation.mutate()} + accountLess={accountLess} + contactName={contactName} + contactEmail={contactEmail} + contactPhone={contactPhone} + onContactNameChange={setContactName} + onContactEmailChange={setContactEmail} + onContactPhoneChange={setContactPhone} + website={website} + onWebsiteChange={setWebsite} + publicToken={publicToken} />
) : null} diff --git a/frontend/src/features/inventory/PublicRequestPanelAccountLess.test.tsx b/frontend/src/features/inventory/PublicRequestPanelAccountLess.test.tsx new file mode 100644 index 00000000..b19362e9 --- /dev/null +++ b/frontend/src/features/inventory/PublicRequestPanelAccountLess.test.tsx @@ -0,0 +1,181 @@ +/** + * The account-less borrow form must actually be submittable. + * + * `setup.sh` offers "anyone, no account needed" and the backend implements it, but the + * public form used to send only `{requested_for, items}`. The backend REQUIRES + * `contact_name`, `contact_email` and an `Idempotency-Key` header on an anonymous + * submission, so every visitor to an opted-in makerspace received a 400 — an advertised + * mode that could not be used. These pin the client half of that contract: the fields are + * collected, the header is sent, and a members-only space is left untouched. + */ +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { fireEvent, render, screen, waitFor } from "@testing-library/react"; +import { beforeEach, describe, expect, it, vi } from "vitest"; + +const { submitPublicRequest, getAccessToken, refreshAccessToken } = vi.hoisted(() => ({ + submitPublicRequest: vi.fn(), + getAccessToken: vi.fn(), + refreshAccessToken: vi.fn(), +})); + +vi.mock("./api", async () => { + const actual = await vi.importActual("./api"); + return { ...actual, submitPublicRequest }; +}); + +vi.mock("../../lib/api", async () => { + const actual = await vi.importActual("../../lib/api"); + return { ...actual, getAccessToken, refreshAccessToken }; +}); + +import { PublicRequestPanel } from "./PublicRequestPanel"; + +const ITEMS = [{ productId: 7, name: "Logic analyzer", quantity: 1 }]; + +function fill(label: RegExp, value: string) { + fireEvent.change(screen.getByLabelText(label), { target: { value } }); +} + +function renderPanel(accountLess: boolean) { + // The panel derives account-less mode from policy AND the absence of an access token. + const queryClient = new QueryClient({ + defaultOptions: { queries: { retry: false }, mutations: { retry: false } }, + }); + return render( + + {}} + /> + , + ); +} + +describe("account-less borrow requests", () => { + beforeEach(() => { + submitPublicRequest.mockReset(); + submitPublicRequest.mockResolvedValue({ public_token: "tok-abc-123" }); + getAccessToken.mockReset(); + getAccessToken.mockReturnValue(""); + refreshAccessToken.mockReset(); + // No session to restore: the panel probes the refresh cookie before deciding, so an + // anonymous visitor is only known to be anonymous once this settles. + refreshAccessToken.mockResolvedValue(false); + }); + + // The account-less form appears only after the session probe settles. + async function awaitAccountLessForm() { + await waitFor(() => expect(screen.getByLabelText(/your name/i)).toBeTruthy()); + } + + it("collects contact details and sends them with an idempotency key", async () => { + renderPanel(true); + await awaitAccountLessForm(); + + fill(/your name/i, "Ada Lovelace"); + fill(/^email$/i, "ada@example.test"); + fill(/request purpose/i, "Bench diagnostics"); + fireEvent.click(screen.getByRole("button", { name: /submit request/i })); + + await waitFor(() => expect(submitPublicRequest).toHaveBeenCalledTimes(1)); + const [slug, payload, idempotencyKey] = submitPublicRequest.mock.calls[0]; + expect(slug).toBe("makerspace"); + expect(payload).toMatchObject({ + requested_for: "Bench diagnostics", + contact_name: "Ada Lovelace", + contact_email: "ada@example.test", + }); + expect(payload.items).toEqual([{ product_id: 7, quantity: 1 }]); + // Required by the backend; without it an anonymous submission is refused outright. + expect(idempotencyKey).toBeTruthy(); + }); + + it("keeps submit disabled until a name and an email are given", async () => { + renderPanel(true); + await awaitAccountLessForm(); + + fill(/request purpose/i, "Bench diagnostics"); + expect(screen.getByRole("button", { name: /submit request/i })).toBeDisabled(); + + fill(/your name/i, "Ada Lovelace"); + expect(screen.getByRole("button", { name: /submit request/i })).toBeDisabled(); + + fill(/^email$/i, "ada@example.test"); + expect(screen.getByRole("button", { name: /submit request/i })).toBeEnabled(); + }); + + it("shows the request token afterwards, the only handle an account-less visitor has", async () => { + renderPanel(true); + await awaitAccountLessForm(); + + fill(/your name/i, "Ada Lovelace"); + fill(/^email$/i, "ada@example.test"); + fill(/request purpose/i, "Bench diagnostics"); + fireEvent.click(screen.getByRole("button", { name: /submit request/i })); + + // Unverified contacts get no lifecycle email and there is no signed-in area to return + // to, so discarding the token would strand the requester. + await waitFor(() => expect(screen.getByText("tok-abc-123")).toBeTruthy()); + }); + + it("sends the honeypot field so autofill bots get the decoy response", async () => { + const { container } = renderPanel(true); + await awaitAccountLessForm(); + + fill(/your name/i, "Ada Lovelace"); + fill(/^email$/i, "ada@example.test"); + fill(/request purpose/i, "Bench diagnostics"); + // Queried by name, not by role: the wrapper is aria-hidden precisely so assistive + // technology never offers it, which is what makes it a honeypot. + const honeypot = container.querySelector('input[name="website"]'); + expect(honeypot).not.toBeNull(); + fireEvent.change(honeypot as HTMLInputElement, { + target: { value: "http://spam.example" }, + }); + fireEvent.click(screen.getByRole("button", { name: /submit request/i })); + + await waitFor(() => expect(submitPublicRequest).toHaveBeenCalledTimes(1)); + expect(submitPublicRequest.mock.calls[0][1].website).toBe("http://spam.example"); + }); + + it("gives a signed-in member the member form even on an opted-in space", async () => { + // `tenantPublicRequest` still attaches Authorization, so the backend takes the + // authenticated branch and ignores contact fields. Asking for them would promise + // something the stored request does not honour. + getAccessToken.mockReturnValue("an-access-token"); + renderPanel(true); + + expect(screen.queryByLabelText(/your name/i)).toBeNull(); + // An `anyone` policy has the membership module off, so the copy must NOT claim + // membership, waiver and presence are required. + await waitFor(() => + expect(screen.getByText(/filed against your account/i)).toBeTruthy(), + ); + expect(screen.queryByLabelText(/your name/i)).toBeNull(); + }); + + it("does not promise 'no account needed' on the scan tab", async () => { + renderPanel(true); + await waitFor(() => expect(screen.getByText(/no account needed/i)).toBeTruthy()); + + fireEvent.click(screen.getByRole("button", { name: /scan a tool/i })); + + // Self-checkout requires an authenticated member with active presence. + expect(screen.queryByText(/no account needed/i)).toBeNull(); + }); + + it("asks a members-only space for no contact details and sends no key", async () => { + renderPanel(false); + + expect(screen.queryByLabelText(/your name/i)).toBeNull(); + fill(/request purpose/i, "Bench diagnostics"); + fireEvent.click(screen.getByRole("button", { name: /submit request/i })); + + await waitFor(() => expect(submitPublicRequest).toHaveBeenCalledTimes(1)); + const [, payload, idempotencyKey] = submitPublicRequest.mock.calls[0]; + expect(payload).not.toHaveProperty("contact_name"); + expect(idempotencyKey).toBeUndefined(); + }); +}); diff --git a/frontend/src/features/inventory/api.ts b/frontend/src/features/inventory/api.ts index 97fdb34b..7b5a0d6b 100644 --- a/frontend/src/features/inventory/api.ts +++ b/frontend/src/features/inventory/api.ts @@ -72,7 +72,19 @@ export async function submitPublicRequest( payload: { requested_for: string; items: { product_id: number; quantity: number }[]; + // Account-less submissions only. The backend requires name and email (phone is + // optional) and rejects the request without them. + contact_name?: string; + contact_email?: string; + contact_phone?: string; + // Honeypot. The serializer pops it and a filled value gets the decoy response, so it + // is sent on every submission, authenticated or not -- the backend checks both. + website?: string; }, + // Required by the backend for account-less submissions, and the reason a retry cannot + // create a second request: the same key with the same payload returns the original, + // a different payload is refused. + idempotencyKey?: string, ): Promise { return tenantPublicRequest( slug, @@ -80,6 +92,7 @@ export async function submitPublicRequest( { method: "POST", body: JSON.stringify(payload), + ...(idempotencyKey ? { headers: { "Idempotency-Key": idempotencyKey } } : {}), }, ); } diff --git a/frontend/src/features/staff/ApiClientCreateCard.tsx b/frontend/src/features/staff/ApiClientCreateCard.tsx index 32f3fcbe..92f6e104 100644 --- a/frontend/src/features/staff/ApiClientCreateCard.tsx +++ b/frontend/src/features/staff/ApiClientCreateCard.tsx @@ -1,3 +1,4 @@ +import { Field } from "../../components/ui"; import { ApiClientScopePicker } from "./ApiClientScopePicker"; import type { ApiClientCreateResponse, ApiClientScopeOption } from "./apiClientsApi"; import { splitOrigins } from "./apiClientsApi"; @@ -48,18 +49,21 @@ export function ApiClientCreateCard({

API clients

- onLabelChange(event.target.value)} - /> -