Skip to content

feat: account-level suspension for under-16 age review#235

Open
mbradley wants to merge 2 commits into
mainfrom
feat/account-suspension
Open

feat: account-level suspension for under-16 age review#235
mbradley wants to merge 2 commits into
mainfrom
feat/account-suspension

Conversation

@mbradley
Copy link
Copy Markdown
Member

@mbradley mbradley commented May 18, 2026

Summary

  • Adds status (active/suspended/banned), suspended_reason, and suspended_at columns to the users table
  • Auth flow includes account_status in UCAN token facts and account status response when non-active
  • Service-token admin endpoints (GET/PUT /api/admin/users/:pubkey/status) for relay-manager to suspend/unsuspend accounts
  • Signer daemon refuses to sign events or encrypt/decrypt for non-active users (DB query per request, no cache)
  • Suspended and banned users can still log in to see restriction/ban notices

Relates to divinevideo/support-trust-safety#142

Context

Track C of the under-16 system coordination doc. The relay-manager side (case table, state machine, admin queue) and the mobile client (PR #3531 with restrictedMinorReview) are already built. This gives Keycast the ability to mark accounts as suspended and enforce that through the signer.

Design spec: docs/superpowers/specs/2026-05-18-account-suspension-design.md

Test plan

  • Run bun run db:reset and verify new columns exist
  • cargo test --lib -- all 184 unit tests pass
  • Integration tests for admin endpoints (7 tests in user_status_admin_test.rs)
  • Manual: suspend a user via admin PUT, verify get_account_status returns status
  • Manual: verify suspended user can still log in but signer refuses sign_event
  • Manual: unsuspend user, verify signing works again
  • Verify KEYCAST_SERVICE_TOKEN env var is set in staging before deploy

@mbradley mbradley force-pushed the feat/account-suspension branch 4 times, most recently from 98c932f to e8dfe6c Compare May 18, 2026 18:00
@mbradley mbradley marked this pull request as ready for review May 18, 2026 18:01
@mbradley mbradley requested a review from dcadenas May 18, 2026 18:04
Copy link
Copy Markdown
Contributor

@dcadenas dcadenas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The suspension work needs to cover every path that can still sign or issue user tokens before this can merge. The current implementation mixes user accounts and signing keys in one signer path, leaves the direct HTTP signing fallback unchecked, and lets several token issuance paths omit the new account-status fact.


Additional findings not shown inline

GitHub only allows PR review comments on lines present in the current diff hunk, so these accepted findings are included in the review body instead.

  • Check account status before the HTTP signing slow path
    let signed_event = unsigned_event

    Check account status before the direct unsigned_event.sign(&keys) fallback.

The cached signer path has the new gate, but this slow path can still sign for a suspended or banned user with a valid UCAN when the handler cache is unavailable or misses.

Add the same (tenant_id, user_pubkey) status check before decrypting or signing with the personal key.

  • Centralize account status propagation for user token issuance
    let access_token = super::auth::generate_server_signed_ucan(

    Make user-token issuance require the current UserStatus instead of letting each call site pass None.

OAuth code exchange, refresh-token issuance, and popup or headless login paths can still issue UCANs for suspended or banned users without the account_status fact this PR added.

Fetch the status inside the user-token helper or require callers to pass it explicitly, then cover the OAuth, refresh, popup, and headless paths with tests that decode the UCAN facts.


/// Check that the user's account is active (not suspended or banned).
/// Queries the DB on each request for immediate enforcement.
async fn check_user_active(&self) -> SignerResult<()> {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not run the account-status check against self.user_keys.public_key() for regular team authorizations.

That key can be the stored signing key, not the user account pubkey, so check_user_active() can reject valid non-OAuth signing, encryption, and decryption requests with User not found.

Gate this check to OAuth or user-backed handlers, or pass the real user pubkey separately instead of using the signing key as the account identity.

@mbradley mbradley force-pushed the feat/account-suspension branch 3 times, most recently from d8cdea7 to b0049dd Compare May 18, 2026 18:57
@mbradley mbradley force-pushed the feat/account-suspension branch from b0049dd to 2f5f24b Compare May 18, 2026 19:02
@mbradley mbradley requested a review from dcadenas May 20, 2026 12:03
Copy link
Copy Markdown
Contributor

@dcadenas dcadenas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The suspension feature still leaves signing and UCAN issuance paths open for suspended or banned users, so those gates need to be enforced before merge. The inline comments point to the two remaining bypasses that should be fixed in the authoritative paths.


Additional required changes

  • Gate HTTP signing on account status before either signing path (api/src/api/http/auth.rs:3216)
    Validate the user's account status before either signing path can run.
    Right now sign_event can fall through to the DB/decrypt slow path and call unsigned_event.sign(&keys) without checking users.status.
    A suspended or banned user with a still-valid token can still get an event signed when the cached signer handler is missing or misses.
    Load the user status after extracting tenant_id and user_pubkey, deny non-active users before the cached-handler branch and the slow path, and add a test that covers a suspended user on the slow path.

Comment thread api/src/api/http/oauth.rs Outdated
&auth_state.state.server_keys,
false, // Refresh tokens are not first-party
None,
None,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make user UCAN issuance require account status instead of letting callers pass None.
This refresh path passes None, and the OAuth authorization and popup login paths do the same, so suspended or banned users can still receive UCANs without an account_status fact.
Move the status lookup into the user-token helper or make the status argument non-optional for all user-token callers, while keeping admin and other non-user token paths separate.
Add coverage for refresh, OAuth code/headless issuance, and popup login with a suspended user.

Address PR #235 review feedback:

- Check account status before both sign_event paths (fast/slow)
- Pass UserStatus to all UCAN issuance paths (refresh, OAuth code
  exchange, popup login, verify_email, change_key, claim)
- Add integration tests for suspended user signing denial
  and get_public_key allowance
@mbradley mbradley requested a review from dcadenas May 20, 2026 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants