From 2aecc5c7871f0c22e42bb98595f12e10b5b6e223 Mon Sep 17 00:00:00 2001 From: Adolfo Delorenzo Date: Tue, 24 Mar 2026 13:46:03 -0600 Subject: [PATCH] fix(04-rbac): revise plans based on checker feedback --- .planning/phases/04-rbac/04-01-PLAN.md | 29 ++++++++++++++---- .planning/phases/04-rbac/04-02-PLAN.md | 17 ++++++----- .planning/phases/04-rbac/04-03-PLAN.md | 41 ++++++++++++++++++-------- 3 files changed, 60 insertions(+), 27 deletions(-) diff --git a/.planning/phases/04-rbac/04-01-PLAN.md b/.planning/phases/04-rbac/04-01-PLAN.md index 1fc4980..7c50e02 100644 --- a/.planning/phases/04-rbac/04-01-PLAN.md +++ b/.planning/phases/04-rbac/04-01-PLAN.md @@ -16,6 +16,7 @@ files_modified: - migrations/versions/006_rbac_roles.py - tests/unit/test_rbac_guards.py - tests/unit/test_invitations.py + - tests/unit/test_portal_auth.py autonomous: true requirements: - RBAC-01 @@ -54,6 +55,9 @@ must_haves: - path: "tests/unit/test_invitations.py" provides: "Unit tests for HMAC token and invitation flow" min_lines: 40 + - path: "tests/unit/test_portal_auth.py" + provides: "Unit tests for auth/verify endpoint returning role + tenant_ids claims" + min_lines: 30 key_links: - from: "packages/shared/shared/api/rbac.py" to: "packages/shared/shared/models/auth.py" @@ -266,10 +270,11 @@ class AuditEvent(Base): # Reuse for impersonation logging in Plan 03 - Task 3: Unit tests for RBAC guards and invitation system + Task 3: Unit tests for RBAC guards, invitation system, and portal auth tests/unit/test_rbac_guards.py, - tests/unit/test_invitations.py + tests/unit/test_invitations.py, + tests/unit/test_portal_auth.py - test_platform_admin_passes: platform_admin caller gets through require_platform_admin @@ -285,6 +290,9 @@ class AuditEvent(Base): # Reuse for impersonation logging in Plan 03 - test_invite_accept_creates_user: accepting invite creates PortalUser + UserTenantRole - test_invite_accept_rejects_expired: expired invitation returns error - test_invite_resend_updates_token: resend generates new token_hash and extends expires_at + - test_auth_verify_returns_role: auth/verify response contains role field (not is_admin) + - test_auth_verify_returns_tenant_ids: auth/verify response contains tenant_ids list + - test_auth_verify_returns_active_tenant: auth/verify response contains active_tenant_id Create `tests/unit/test_rbac_guards.py`: @@ -309,18 +317,27 @@ class AuditEvent(Base): # Reuse for impersonation logging in Plan 03 - Test invitation acceptance via POST /api/portal/invitations/accept (mock DB session with pending invitation) - Test resend updates token_hash and extends expires_at + Create `tests/unit/test_portal_auth.py`: + - Test the updated auth/verify endpoint returns role (not is_admin) in the response + - Test auth/verify returns tenant_ids as a list of UUID strings for a user with UserTenantRole memberships + - Test auth/verify returns active_tenant_id as the first tenant ID (or None for users with no memberships) + - Test auth/verify for platform_admin returns all tenant IDs from the tenants table + - Test auth/verify for customer_admin returns only tenant IDs from their UserTenantRole rows + - Mock the DB session with appropriate PortalUser (role field) and UserTenantRole rows + - Use httpx.AsyncClient with the app, following existing test patterns (make_app(session) factory) + Follow existing test patterns: use `make_app(session)` factory from tests/ or direct httpx.AsyncClient with app. - cd /home/adelorenzo/repos/konstruct && pytest tests/unit/test_rbac_guards.py tests/unit/test_invitations.py -x -v + cd /home/adelorenzo/repos/konstruct && pytest tests/unit/test_rbac_guards.py tests/unit/test_invitations.py tests/unit/test_portal_auth.py -x -v - All RBAC guard unit tests pass. All invitation token and API unit tests pass. Coverage includes platform_admin bypass, tenant membership checks, token tampering, and expiry validation. + All RBAC guard unit tests pass. All invitation token and API unit tests pass. All portal auth unit tests pass verifying role + tenant_ids claims. Coverage includes platform_admin bypass, tenant membership checks, token tampering, expiry validation, and auth/verify response shape. -- `pytest tests/unit/test_rbac_guards.py tests/unit/test_invitations.py -x -v` — all pass +- `pytest tests/unit/test_rbac_guards.py tests/unit/test_invitations.py tests/unit/test_portal_auth.py -x -v` — all pass - `python -c "from shared.models.auth import UserRole; assert len(UserRole) == 3"` — enum has 3 values - `python -c "from shared.api.rbac import require_platform_admin"` — guard imports clean - `python -c "from shared.invite_token import generate_invite_token, validate_invite_token"` — token utils import clean @@ -335,7 +352,7 @@ class AuditEvent(Base): # Reuse for impersonation logging in Plan 03 - HMAC invite tokens generate and validate with 48h TTL - SMTP email utility exists (sync, for Celery) - Auth/verify returns role + tenant_ids -- All unit tests pass +- All unit tests pass (including test_portal_auth.py for JWT callback claims) diff --git a/.planning/phases/04-rbac/04-02-PLAN.md b/.planning/phases/04-rbac/04-02-PLAN.md index 782454a..446aef0 100644 --- a/.planning/phases/04-rbac/04-02-PLAN.md +++ b/.planning/phases/04-rbac/04-02-PLAN.md @@ -11,7 +11,7 @@ files_modified: - packages/portal/components/nav.tsx - packages/portal/components/tenant-switcher.tsx - packages/portal/components/impersonation-banner.tsx - - packages/portal/app/(dashboard)/invite/[token]/page.tsx + - packages/portal/app/invite/[token]/page.tsx - packages/portal/app/(dashboard)/users/page.tsx - packages/portal/app/(dashboard)/admin/users/page.tsx - packages/portal/app/(dashboard)/layout.tsx @@ -52,8 +52,8 @@ must_haves: - path: "packages/portal/components/impersonation-banner.tsx" provides: "Impersonation indicator banner" min_lines: 15 - - path: "packages/portal/app/(dashboard)/invite/[token]/page.tsx" - provides: "Invite acceptance page with password form" + - path: "packages/portal/app/invite/[token]/page.tsx" + provides: "Invite acceptance page with password form (outside dashboard layout — no auth required)" min_lines: 40 - path: "packages/portal/app/(dashboard)/users/page.tsx" provides: "Per-tenant user management page" @@ -253,13 +253,13 @@ const navItems = [ /* dashboard, tenants, agents, usage, billing, api-keys */ ]; Task 3: Invite acceptance page + user management pages - packages/portal/app/(dashboard)/invite/[token]/page.tsx, + packages/portal/app/invite/[token]/page.tsx, packages/portal/app/(dashboard)/users/page.tsx, packages/portal/app/(dashboard)/admin/users/page.tsx - Create `packages/portal/app/(dashboard)/invite/[token]/page.tsx`: - - NOTE: This page must be accessible WITHOUT authentication. If the (dashboard) layout requires auth, create this at `packages/portal/app/invite/[token]/page.tsx` instead (outside the dashboard layout group). Check existing route structure. + Create `packages/portal/app/invite/[token]/page.tsx`: + - IMPORTANT: This page is created OUTSIDE the (dashboard) route group because the (dashboard) layout enforces authentication. Invite acceptance must be accessible to unauthenticated users who are creating their account. - Reads `token` from URL params - On load, validates token client-side by calling a GET endpoint or just displays the form (server validates on submit) - Form fields: Password (min 8 chars), Confirm Password @@ -289,7 +289,7 @@ const navItems = [ /* dashboard, tenants, agents, usage, billing, api-keys */ ]; cd /home/adelorenzo/repos/konstruct/packages/portal && npx tsc --noEmit 2>&1 | head -30 - Invite acceptance page renders password form and submits to accept endpoint. Per-tenant users page lists users with invite/resend capability. Platform admin users page shows cross-tenant user list with filters. All pages compile without TypeScript errors. + Invite acceptance page renders password form and submits to accept endpoint. Page is outside (dashboard) group so unauthenticated users can access it. Per-tenant users page lists users with invite/resend capability. Platform admin users page shows cross-tenant user list with filters. All pages compile without TypeScript errors. @@ -299,6 +299,7 @@ const navItems = [ /* dashboard, tenants, agents, usage, billing, api-keys */ ]; - `cd packages/portal && npx next build` — build succeeds - Login as platform_admin: JWT contains role="platform_admin", sees all nav items - Login as customer_operator: does not see Billing/API Keys/Users in nav, /billing redirects to /agents +- Visit /invite/{token} while logged out: page renders without auth redirect @@ -307,7 +308,7 @@ const navItems = [ /* dashboard, tenants, agents, usage, billing, api-keys */ ]; - Nav hides restricted items based on role - Tenant switcher works for multi-tenant users (no page reload) - Impersonation banner renders when impersonating -- Invite acceptance page accepts token and creates account +- Invite acceptance page at /invite/[token] (outside dashboard layout) accepts token and creates account without requiring auth - User management pages exist for tenant admin and platform admin - Portal builds and TypeScript compiles clean diff --git a/.planning/phases/04-rbac/04-03-PLAN.md b/.planning/phases/04-rbac/04-03-PLAN.md index 97ea499..10ce70e 100644 --- a/.planning/phases/04-rbac/04-03-PLAN.md +++ b/.planning/phases/04-rbac/04-03-PLAN.md @@ -28,6 +28,7 @@ must_haves: - "Every tenant-scoped endpoint returns 403 for customer_admin accessing a different tenant" - "Platform admin gets 200 on any tenant's endpoints regardless of membership" - "Customer operator gets 200 on read-only endpoints (GET agents, GET usage) for their tenant" + - "Customer operator can POST a test message to an agent (POST /tenants/{tid}/agents/{aid}/test) and get 200" - "Impersonation actions are logged in audit_events with platform admin user_id" - "Full invite flow works end-to-end: create invitation -> accept -> login -> correct role" artifacts: @@ -40,8 +41,8 @@ must_haves: key_links: - from: "packages/shared/shared/api/portal.py" to: "packages/shared/shared/api/rbac.py" - via: "Depends(require_tenant_admin) on mutating endpoints" - pattern: "Depends\\(require_tenant_admin\\)|Depends\\(require_platform_admin\\)" + via: "Depends(require_tenant_admin) on mutating endpoints, Depends(require_tenant_member) on test-message endpoint" + pattern: "Depends\\(require_tenant_admin\\)|Depends\\(require_platform_admin\\)|Depends\\(require_tenant_member\\)" - from: "packages/shared/shared/api/billing.py" to: "packages/shared/shared/api/rbac.py" via: "Depends(require_tenant_admin) on billing endpoints" @@ -53,10 +54,10 @@ must_haves: --- -Wire RBAC guards to ALL existing portal API endpoints, add impersonation audit logging, add user listing endpoints, and create comprehensive integration tests proving every endpoint enforces role-based authorization. +Wire RBAC guards to ALL existing portal API endpoints, add test-message endpoint for operators, add impersonation audit logging, add user listing endpoints, and create comprehensive integration tests proving every endpoint enforces role-based authorization. Purpose: Defense in depth — the UI hides things, but the API MUST enforce authorization. This plan completes the server-side enforcement layer and validates the entire RBAC system end-to-end. -Output: All portal endpoints guarded, impersonation logged, integration tests for RBAC + invite flow. +Output: All portal endpoints guarded, test-message endpoint for operators, impersonation logged, integration tests for RBAC + invite flow. @@ -104,6 +105,9 @@ POST /api/portal/tenants/{tid}/agents # require_tenant_admin GET /api/portal/tenants/{tid}/agents/{aid} # require_tenant_member PUT /api/portal/tenants/{tid}/agents/{aid} # require_tenant_admin DELETE /api/portal/tenants/{tid}/agents/{aid} # require_tenant_admin + +# NEW — Test message (per locked decision: operators can send test messages) +POST /api/portal/tenants/{tid}/agents/{aid}/test # require_tenant_member (operators included) ``` From packages/shared/shared/api/billing.py, channels.py, llm_keys.py, usage.py: @@ -126,7 +130,7 @@ class AuditEvent(Base): - Task 1: Wire RBAC guards to all existing API endpoints + impersonation + user listing + Task 1: Wire RBAC guards to all existing API endpoints + test-message endpoint + impersonation + user listing packages/shared/shared/api/portal.py, packages/shared/shared/api/billing.py, @@ -149,6 +153,7 @@ class AuditEvent(Base): - `GET /tenants/{tenant_id}/agents/{agent_id}` — add `Depends(require_tenant_member)`. - `PUT /tenants/{tenant_id}/agents/{agent_id}` — add `Depends(require_tenant_admin)`. - `DELETE /tenants/{tenant_id}/agents/{agent_id}` — add `Depends(require_tenant_admin)`. + - ADD new endpoint: `POST /tenants/{tenant_id}/agents/{agent_id}/test` — requires `Depends(require_tenant_member)` (NOT require_tenant_admin). This is the test-message endpoint per locked decision: "operators can send test messages to agents." Accepts `{message: str}` body. Dispatches the message through the agent orchestrator pipeline (or a lightweight test handler) for the specified agent, returns the agent's response. This allows operators to QA agent behavior without agent CRUD access. - ADD new endpoint: `GET /tenants/{tenant_id}/users` — requires require_tenant_admin. Queries UserTenantRole JOIN PortalUser WHERE tenant_id matches. Returns list of {id, name, email, role, created_at}. Also queries PortalInvitation WHERE tenant_id AND status='pending' to include pending invites. - ADD new endpoint: `GET /admin/users` — requires require_platform_admin. Queries ALL PortalUser with their UserTenantRole associations. Supports optional query params: tenant_id filter, role filter. Returns list with tenant membership info. @@ -189,8 +194,11 @@ from shared.api.billing import billing_router from shared.api.channels import channels_router routes = [r.path for r in portal_router.routes] print(f'Portal routes: {len(routes)}') +# Verify test-message endpoint exists +test_routes = [r.path for r in portal_router.routes if 'test' in r.path] +assert test_routes, 'Missing /test endpoint for agent test messages' +print(f'Test-message route: {test_routes}') # Verify at least one route has dependencies -import inspect for r in portal_router.routes: if hasattr(r, 'dependant') and r.dependant.dependencies: print(f' {r.path} has {len(r.dependant.dependencies)} dependencies') @@ -199,7 +207,7 @@ else: print('WARNING: No routes have dependencies') " - Every portal API endpoint has an RBAC guard. Mutating endpoints require tenant_admin or platform_admin. Read-only tenant endpoints allow tenant_member. Global endpoints require platform_admin. Impersonation endpoint logs to audit trail. User listing endpoints exist for both per-tenant and global views. + Every portal API endpoint has an RBAC guard. Mutating endpoints require tenant_admin or platform_admin. Read-only tenant endpoints allow tenant_member. Test-message endpoint (POST /tenants/{tid}/agents/{aid}/test) allows tenant_member including operators. Global endpoints require platform_admin. Impersonation endpoint logs to audit trail. User listing endpoints exist for both per-tenant and global views. @@ -212,6 +220,7 @@ else: - Platform admin with correct headers gets 200 on all endpoints - Customer admin gets 200 on own-tenant endpoints, 403 on other tenants - Customer operator gets 200 on GET endpoints, 403 on POST/PUT/DELETE + - Customer operator gets 200 on POST /tenants/{tid}/agents/{aid}/test (test message — exception to POST restriction) - Missing role headers return 401/422 (FastAPI Header() validation) - Impersonation endpoint logs AuditEvent row - Full invite flow: admin creates invite -> token generated -> accept with password -> new user can login -> new user has correct role and tenant membership @@ -237,6 +246,7 @@ else: | POST /tenants/{tid}/agents | 201 | 201 | 403 | 403 | | PUT /tenants/{tid}/agents/{aid} | 200 | 200 | 403 | 403 | | DELETE /tenants/{tid}/agents/{aid} | 204 | 204 | 403 | 403 | + | POST /tenants/{tid}/agents/{aid}/test | 200 | 200 | 403 | 200 | | GET /tenants/{tid}/users | 200 | 200 | 403 | 403 | | GET /admin/users | 200 | 403 | 403 | 403 | @@ -244,6 +254,7 @@ else: - Request with NO role headers -> 422 (missing required header) - Impersonation endpoint creates AuditEvent row - Billing, channels, llm_keys, usage endpoints follow same pattern (at least one representative test per router) + - Specific test: customer_operator can POST to /test endpoint but NOT to agent CRUD POST Create `tests/integration/test_invite_flow.py`: - Set up: create a tenant, create a customer_admin user with membership @@ -263,7 +274,7 @@ else: cd /home/adelorenzo/repos/konstruct && pytest tests/integration/test_portal_rbac.py tests/integration/test_invite_flow.py -x -v - All RBAC integration tests pass — every endpoint returns correct status code for each role. Full invite flow works end-to-end. Expired invites are rejected. Resend works. Double-accept prevented. + All RBAC integration tests pass — every endpoint returns correct status code for each role. Operator test-message endpoint returns 200. Full invite flow works end-to-end. Expired invites are rejected. Resend works. Double-accept prevented. @@ -279,8 +290,9 @@ else: - Verify: can access /admin/users (global user management) - Verify: can impersonate a tenant (banner appears, can exit) 4. Create a customer_admin invite from the Users page - 5. Open the invite link in an incognito window - - Verify: activation page shows, can set password + 5. Open the invite link in an incognito window (URL will be /invite/{token} — NOT inside dashboard) + - Verify: activation page shows without requiring login + - Verify: can set password and complete account creation - Verify: after activation, redirected to login 6. Login as the new customer admin: - Verify: sees Dashboard, Employees, Usage, Billing, API Keys, Users (no Tenants, no Platform) @@ -290,11 +302,12 @@ else: - Verify: sees only Employees and Usage in nav - Verify: navigating to /billing redirects to /agents - Verify: cannot see Billing, API Keys, Users in sidebar + - Verify: can click "Test" on an agent to send a test message (per locked decision) 9. If user has multiple tenants, verify tenant switcher appears and switches context 10. Run: `pytest tests/ -x` — all tests pass Human confirms all verification steps pass or reports issues - All three roles behave correctly in portal UI and API. Invitation flow works end-to-end. Full test suite green. + All three roles behave correctly in portal UI and API. Operators can send test messages but not edit agents. Invitation flow works end-to-end. Full test suite green. @@ -305,16 +318,18 @@ else: - `pytest tests/ -x` — entire test suite green (no regressions) - Every mutating endpoint returns 403 without proper role headers - Platform admin bypasses all tenant membership checks +- Operator gets 200 on POST /tenants/{tid}/agents/{aid}/test - All portal API endpoints enforce role-based authorization via Depends() guards -- Customer operators cannot mutate any data via API (403 on POST/PUT/DELETE) +- Customer operators cannot mutate any data via API (403 on POST/PUT/DELETE) EXCEPT test-message endpoint +- Customer operators CAN send test messages to agents (POST /tenants/{tid}/agents/{aid}/test returns 200) - Customer admins can only access their own tenant's data (403 on other tenants) - Platform admin has unrestricted access to all endpoints - Impersonation actions logged in audit_events table - User listing endpoints exist for per-tenant and global views -- Integration tests comprehensively cover the RBAC matrix +- Integration tests comprehensively cover the RBAC matrix including test-message operator access - Full invite flow works end-to-end in integration tests - Human verification confirms visual role-based behavior in portal