diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 545b46b..1fcd2dd 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -27,6 +27,7 @@ Brief description of changes - [ ] No new warnings generated - [ ] Tests added/updated and passing - [ ] Dependencies updated (if applicable) +- [ ] Any route taking a user/student/`*_id` (path, query, or body) calls the object-access helper (`assert_can_access_student`) or is intentionally public ## Related Issues diff --git a/README.md b/README.md index 6c4e488..e4a810d 100644 --- a/README.md +++ b/README.md @@ -308,7 +308,6 @@ ELO_MIN_RATING=400 ELO_MAX_RATING=2000 # External Services (Optional) -RAILS_APP_URL=https://your-rails-app.com WEBHOOK_SECRET=your-webhook-secret ``` @@ -411,24 +410,15 @@ python run_server.py ### Authentication -The API supports two authentication methods: - -1. **User Authentication (self-hosted JWT)** - - Required for user-facing endpoints - - Include the token in the Authorization header: - ``` - Authorization: Bearer - ``` - -2. **Service-to-Service Authentication (API Key)** - - Required for service-to-service calls - - Include the API key in the X-API-Key header: - ``` - X-API-Key: - ``` +All endpoints use self-hosted JWT authentication. Include the token in the Authorization header: +``` +Authorization: Bearer +``` **Note:** Health check endpoints (`/`, `/health`) do not require authentication. +There is no `X-API-Key` service-to-service auth in this codebase — it was documented but never implemented. If a service-to-service integration (e.g. a future Rails caller) is built later, it must use a scoped identity that still passes the object-ownership check (see `_docs/active/API_CONTRACTS.md`). + ### Error Handling The API returns standardized error responses: diff --git a/_docs/DEMO_USER_GUIDE.md b/_docs/DEMO_USER_GUIDE.md index 4387b3f..5c87b89 100644 --- a/_docs/DEMO_USER_GUIDE.md +++ b/_docs/DEMO_USER_GUIDE.md @@ -12,7 +12,7 @@ The AI Study Companion is a **persistent AI agent** that lives between tutoring - **Conversational Q&A**: Answers questions with full conversation context - **Smart Suggestions**: Prevents churn by suggesting next learning paths - **Proactive Engagement**: Nudges students at risk of churning -- **Seamless Integration**: RESTful API ready for Rails/React platform +- **Seamless Integration**: RESTful API used by the React platform (JWT auth); a Rails caller is aspirational and not implemented --- @@ -35,7 +35,7 @@ All accounts use password: `demo123` ### **Opening (1 minute)** -> "I'm going to show you the AI Study Companion - a persistent AI agent that lives between tutoring sessions. It remembers previous lessons, assigns adaptive practice, answers questions conversationally, and drives students back to human tutors when needed. Everything integrates seamlessly with our existing Rails/React platform via RESTful APIs." +> "I'm going to show you the AI Study Companion - a persistent AI agent that lives between tutoring sessions. It remembers previous lessons, assigns adaptive practice, answers questions conversationally, and drives students back to human tutors when needed. Everything integrates seamlessly with our React platform via RESTful APIs." **Key Points:** - Persistent AI companion (not just a chatbot) @@ -396,18 +396,18 @@ POST /api/v1/goals/{goal_id}/reset --- -## 🔗 Rails/React Platform Integration +## 🔗 React Platform Integration ### **How It Integrates** -The AI Study Companion is built as a **standalone FastAPI service** that integrates with your existing Rails/React platform via RESTful APIs. +The AI Study Companion is built as a **standalone FastAPI service** that integrates with the React frontend via RESTful APIs, using self-hosted JWT (not AWS Cognito). A Rails backend integration (sections below marked NOT IMPLEMENTED) is aspirational only — there is no Rails app in this codebase. #### **1. Authentication Integration** ```javascript // React Frontend -// Uses existing AWS Cognito JWT tokens -const token = await getCognitoToken(); // Your existing auth +// Uses self-hosted JWT tokens issued by this service +const token = getStoredAuthToken(); // Your existing auth fetch('https://api.pennygadget.ai/v1/progress/user123', { headers: { 'Authorization': `Bearer ${token}` @@ -416,15 +416,14 @@ fetch('https://api.pennygadget.ai/v1/progress/user123', { ``` **Backend Support:** -- Accepts AWS Cognito JWT tokens -- Validates tokens using `python-jose` +- Issues and validates self-hosted JWT tokens (HS256) - Extracts user info from token claims - Development mode supports mock tokens for testing -#### **2. Session Summary Integration** +#### **2. Session Summary Integration (NOT IMPLEMENTED — aspirational, future integration only)** ```ruby -# Rails Backend +# Rails Backend (aspirational, not implemented — no Rails app exists in this codebase) # After a tutoring session completes def create_session_summary(session) response = HTTParty.post( @@ -449,7 +448,7 @@ def create_session_summary(session) end ``` -**What This Enables:** +**What This Would Enable (if built):** - Automatic AI summaries after each session - Summaries stored in AI Companion database - Accessible via API for display in React frontend @@ -554,7 +553,7 @@ async function askQuestion(query) { - Follow-up question support - Persistent conversation history -#### **6. Webhook Integration (Event-Driven)** +#### **6. Webhook Integration (Event-Driven) (NOT IMPLEMENTED — aspirational, future integration only)** ```ruby # Rails Backend @@ -598,7 +597,7 @@ end - Automatic updates in Rails app - Event history and retry logic -#### **7. LMS Integration (Canvas/Blackboard)** +#### **7. LMS Integration (Canvas/Blackboard) (NOT IMPLEMENTED — aspirational, future integration only)** ```ruby # Rails Backend @@ -771,7 +770,7 @@ python scripts/verify_demo_users.py 6. **Visual Progress Tracking**: Interactive pie chart on dashboard with goal names and completion percentages 7. **Goal-Focused Practice**: Practice dropdown only shows subjects from goals; auto-creates goals if none exist 8. **Rich Q&A Formatting**: Markdown rendering for code blocks, lists, headings, and formatted explanations -9. **Seamless Integration**: RESTful API ready for Rails/React +9. **Seamless Integration**: RESTful API used by React (JWT auth); Rails is aspirational, not implemented 10. **Proactive Engagement**: Nudges at-risk students automatically 11. **Cross-Subject Learning**: Builds comprehensive learning paths 12. **Math Accuracy**: SymPy for reliable math problem generation @@ -786,7 +785,7 @@ python scripts/verify_demo_users.py All demo accounts are pre-configured and ready. The system demonstrates: - ✅ All retention enhancement requirements - ✅ Complete feature set -- ✅ Rails/React integration examples +- ✅ React integration examples (Rails integration examples are aspirational, not implemented) - ✅ Real-world use cases **Start with the Quick Demo Script (15 minutes) and expand as needed!** diff --git a/_docs/active/API_CONTRACTS.md b/_docs/active/API_CONTRACTS.md index d5701c0..9698781 100644 --- a/_docs/active/API_CONTRACTS.md +++ b/_docs/active/API_CONTRACTS.md @@ -1,19 +1,19 @@ # 🔌 API Contracts **Product:** AI Study Companion MVP -**Integration:** Rails/React Application +**Integration:** React Application **Version:** 1.0.0 --- ## Overview -This document defines the REST API contracts for integrating the AI Study Companion service with the existing Rails/React platform. All endpoints use JSON for request/response bodies. +This document defines the REST API contracts for the AI Study Companion service. All endpoints use JSON for request/response bodies. The real, implemented caller is the React frontend. A Rails backend integration is an aspirational future possibility only — it was never built, and the code below that describes it was never implemented. **Base URL:** `https://api.pennygadget.ai/v1` (or configured environment variable) **Authentication:** -- **Service-to-Service:** API Key in `X-API-Key` header -- **User Requests:** JWT token from AWS Cognito in `Authorization: Bearer ` header +- **All endpoints:** JWT token in `Authorization: Bearer ` header (self-hosted JWT, not AWS Cognito) +- **Not implemented:** The `X-API-Key` service-to-service header described elsewhere in this document was documented from the founding commit but never implemented in code. `settings.ai_service_api_key` has been removed. If a service-to-service integration is built later, it must use a scoped identity that still passes the object-ownership check (see #67/#68). --- @@ -496,7 +496,9 @@ Get multi-goal progress dashboard data. --- -## Rails Integration Examples +## Rails Integration Examples (NOT IMPLEMENTED — aspirational, future integration only) + +The Ruby examples below describe a Rails service-to-service caller using an `X-API-Key` header. This was never built: there is no Rails app in this codebase, and the backend does not check `X-API-Key` at all. Kept here only as a sketch of what a future service-to-service integration might look like; any real implementation must use a scoped identity that still passes the object-ownership check (#67/#68), not a shared static API key. ### Ruby Client Class @@ -687,15 +689,14 @@ export const useAIQuery = () => { ## Rate Limiting -- **Service-to-Service:** 1000 requests/minute per API key - **User Requests:** 100 requests/minute per user - **Headers:** `X-RateLimit-Limit`, `X-RateLimit-Remaining`, `X-RateLimit-Reset` --- -## Webhooks (Optional - POST-MVP) +## Webhooks (Optional - POST-MVP, NOT IMPLEMENTED) -For real-time updates, the service can send webhooks to Rails app: +Aspirational, not implemented. For real-time updates, the service could in the future send webhooks to a Rails app: ``` POST https://your-rails-app.com/webhooks/ai-service diff --git a/_docs/active/IMPLEMENTATION_PRIORITY.md b/_docs/active/IMPLEMENTATION_PRIORITY.md index e85c5ea..aa25925 100644 --- a/_docs/active/IMPLEMENTATION_PRIORITY.md +++ b/_docs/active/IMPLEMENTATION_PRIORITY.md @@ -267,29 +267,29 @@ def calculate_performance(answer, correct_answer, time_taken, hints_used): --- -### 🟢 **PRIORITY 9: Rails/React Integration Points** -**Question #2: Separate Service Integrating with Rails App** +### 🟢 **PRIORITY 9: React Integration Points** +**Question #2: Separate Service, React as the Real Caller (Rails is Aspirational, Not Implemented)** -**Decision:** RESTful API with clear contracts +**Decision:** RESTful API with clear contracts. The React frontend is the actual, implemented caller. A Rails app was planned early on but never built — there is no Rails app in this codebase. **Reasoning:** - **Separation of Concerns:** Microservice architecture - **Technology Flexibility:** Can use Python/Node.js for AI features -- **Scalability:** Independent scaling of AI service vs Rails app +- **Scalability:** Independent scaling of AI service vs any future caller **API Contract Design:** ``` -POST /api/v1/transcripts # Rails → AI Service (session complete) -GET /api/v1/summaries/:user_id # Rails → AI Service (fetch summaries) -POST /api/v1/practice/assign # Rails → AI Service (request practice) +POST /api/v1/transcripts # React → AI Service (session complete) +GET /api/v1/summaries/:user_id # React → AI Service (fetch summaries) +POST /api/v1/practice/assign # React → AI Service (request practice) POST /api/v1/qa/query # React → AI Service (student query) GET /api/v1/progress/:user_id # React → AI Service (dashboard) -POST /api/v1/overrides # Rails → AI Service (tutor override) +POST /api/v1/overrides # React → AI Service (tutor override) ``` **Authentication:** -- API keys for service-to-service (Rails → AI Service) -- JWT tokens for user-facing requests (React → AI Service) +- JWT tokens for user-facing requests (React → AI Service) — this is what's implemented. +- Service-to-service (e.g. a future Rails caller): not implemented. If built later, it must use a scoped identity that still passes the object-ownership check (#67/#68), not a shared static API key. --- diff --git a/_docs/active/PROJECT_STRUCTURE.md b/_docs/active/PROJECT_STRUCTURE.md index 5d31efa..6b5c818 100644 --- a/_docs/active/PROJECT_STRUCTURE.md +++ b/_docs/active/PROJECT_STRUCTURE.md @@ -321,13 +321,13 @@ LOG_LEVEL=INFO --- -## Integration with Rails App +## Integration with Rails App (NOT IMPLEMENTED — aspirational, future integration only) -### API Contract -The Rails app will call this service via REST API: +There is no Rails app in this codebase. A Rails caller was documented from early planning but never built; the real, implemented caller is the React frontend using JWT. The snippet below is kept only as a sketch of what a future service-to-service integration might look like. +### API Contract ```ruby -# Rails example +# Rails example (aspirational, not implemented) class AIServiceClient BASE_URL = ENV['AI_SERVICE_URL'] @@ -342,8 +342,8 @@ end ``` ### Authentication -- Service-to-service: API keys -- User requests: JWT tokens from Cognito +- User requests: self-hosted JWT tokens (`Authorization: Bearer `) — this is what's actually implemented. +- Service-to-service: not implemented. If built later, it must use a scoped identity that still passes the object-ownership check (#67/#68), not a shared static API key. --- diff --git a/_docs/guides/FRONTEND_INTEGRATION.md b/_docs/guides/FRONTEND_INTEGRATION.md index 28be667..90a06ef 100644 --- a/_docs/guides/FRONTEND_INTEGRATION.md +++ b/_docs/guides/FRONTEND_INTEGRATION.md @@ -20,37 +20,22 @@ npm install axios ## 🔐 **Authentication** -### **AWS Cognito Integration** +### **JWT Authentication** -The API uses JWT tokens from AWS Cognito for user authentication. +All endpoints use self-hosted JWT tokens (not AWS Cognito) for authentication: ```javascript -// Get token from Cognito -import { Auth } from 'aws-amplify'; - async function getAuthToken() { - try { - const session = await Auth.currentSession(); - return session.getIdToken().getJwtToken(); - } catch (error) { - console.error('Error getting token:', error); - return null; - } + return localStorage.getItem('authToken'); } // Store token localStorage.setItem('authToken', token); ``` -### **Service-to-Service Authentication** - -For service-to-service requests (e.g., from Rails backend), use API key: +### **Service-to-Service Authentication (NOT IMPLEMENTED)** -```javascript -const headers = { - 'X-API-Key': process.env.REACT_APP_API_KEY, -}; -``` +There is no `X-API-Key` service-to-service auth in this codebase — it was documented but never implemented, and the backend does not check this header. If a service-to-service integration (e.g. a future Rails backend) is built later, it must use a scoped identity that still passes the object-ownership check (see `_docs/active/API_CONTRACTS.md`, #67/#68). --- @@ -387,7 +372,6 @@ Create `.env` file: ```env REACT_APP_API_URL=http://localhost:8000/api/v1 -REACT_APP_API_KEY=your-api-key-here ``` --- diff --git a/examples/api-client/apiClient.js b/examples/api-client/apiClient.js index fa9319a..a31621e 100644 --- a/examples/api-client/apiClient.js +++ b/examples/api-client/apiClient.js @@ -15,7 +15,9 @@ function getAuthToken() { } /** - * Get API key for service-to-service requests + * Get API key for service-to-service requests. + * NOTE: The backend does not implement X-API-Key auth (documented but never + * built — see _docs/active/API_CONTRACTS.md). This fallback is inert. */ function getApiKey() { return process.env.REACT_APP_API_KEY || ''; diff --git a/examples/api-client/apiClientAxios.js b/examples/api-client/apiClientAxios.js index f8d97a1..326d226 100644 --- a/examples/api-client/apiClientAxios.js +++ b/examples/api-client/apiClientAxios.js @@ -23,6 +23,8 @@ const apiClient = axios.create({ apiClient.interceptors.request.use( (config) => { const token = localStorage.getItem('authToken'); + // NOTE: The backend does not implement X-API-Key auth (documented but + // never built — see _docs/active/API_CONTRACTS.md). This fallback is inert. const apiKey = process.env.REACT_APP_API_KEY; if (token) { diff --git a/migrations/004_add_parent_student_assignments.sql b/migrations/004_add_parent_student_assignments.sql new file mode 100644 index 0000000..098ea94 --- /dev/null +++ b/migrations/004_add_parent_student_assignments.sql @@ -0,0 +1,21 @@ +-- Add Parent-Student Assignments Table +-- Migration 004: parent<->student linkage for scoped parent dashboard access (#68) +-- Links are admin/seed-provisioned; there is no self-service linking endpoint. + +CREATE TABLE IF NOT EXISTS parent_student_assignments ( + parent_id UUID NOT NULL REFERENCES users(id) ON DELETE CASCADE, + student_id UUID NOT NULL REFERENCES users(id) ON DELETE CASCADE, + status VARCHAR(20) DEFAULT 'active' CHECK (status IN ('active', 'paused', 'completed')), + + created_at TIMESTAMP WITH TIME ZONE DEFAULT CURRENT_TIMESTAMP, + updated_at TIMESTAMP WITH TIME ZONE DEFAULT CURRENT_TIMESTAMP, + + PRIMARY KEY (parent_id, student_id) +); + +CREATE INDEX IF NOT EXISTS idx_psa_parent ON parent_student_assignments(parent_id); +CREATE INDEX IF NOT EXISTS idx_psa_student ON parent_student_assignments(student_id); +CREATE INDEX IF NOT EXISTS idx_psa_status ON parent_student_assignments(status); + +CREATE TRIGGER update_parent_student_assignments_updated_at BEFORE UPDATE ON parent_student_assignments + FOR EACH ROW EXECUTE FUNCTION update_updated_at_column(); diff --git a/pytest.ini b/pytest.ini index 18a0dae..05d8946 100644 --- a/pytest.ini +++ b/pytest.ini @@ -1,4 +1,8 @@ [pytest] testpaths = tests +# Per-test timeout so a hung test fails fast with a traceback instead of +# stalling CI indefinitely. 120s is well above the whole suite's runtime. +timeout = 120 +timeout_method = thread markers = eval: live eval cases (needs OPENROUTER_API_KEY; run with -m eval) diff --git a/requirements.txt b/requirements.txt index 940e9db..a513212 100644 --- a/requirements.txt +++ b/requirements.txt @@ -36,6 +36,7 @@ pytest==7.4.3 pytest-asyncio==0.21.1 pytest-cov==4.1.0 pytest-mock==3.12.0 +pytest-timeout==2.2.0 httpx==0.25.2 # For testing FastAPI # Development diff --git a/src/api/handlers/advanced_analytics.py b/src/api/handlers/advanced_analytics.py index ad2817a..8993ce3 100644 --- a/src/api/handlers/advanced_analytics.py +++ b/src/api/handlers/advanced_analytics.py @@ -11,6 +11,7 @@ from sqlalchemy.orm import Session as DBSession from src.api.middleware.auth import require_role +from src.api.middleware.authz import assert_can_access_student from src.config.database import get_db from src.services.analytics.ab_testing import ABTestingFramework from src.services.analytics.advanced import AdvancedAnalytics @@ -108,6 +109,8 @@ async def get_engagement_score( Calculates engagement based on sessions, practice, Q&A, and goals """ + assert_can_access_student(db, current_user, user_id) + analytics = AdvancedAnalytics(db) try: diff --git a/src/api/handlers/auth.py b/src/api/handlers/auth.py index 58e71ca..d33731b 100644 --- a/src/api/handlers/auth.py +++ b/src/api/handlers/auth.py @@ -19,7 +19,8 @@ router = APIRouter(prefix="/auth", tags=["auth"]) _EMAIL_RE = re.compile(r"^[^@\s]+@[^@\s]+\.[^@\s]+$") -_ALLOWED_REGISTER_ROLES = {"student", "tutor", "parent"} +# tutor/parent accounts are admin-provisioned, not self-registered (see #60) +_ALLOWED_REGISTER_ROLES = {"student"} _LOGIN_FAILED_MESSAGE = "Invalid email or password" diff --git a/src/api/handlers/dashboards.py b/src/api/handlers/dashboards.py index f569fef..05a0d14 100644 --- a/src/api/handlers/dashboards.py +++ b/src/api/handlers/dashboards.py @@ -13,8 +13,10 @@ from sqlalchemy.orm import Session as DBSession from src.api.middleware.auth import get_current_user, require_role +from src.api.middleware.authz import assert_can_access_student from src.config.database import get_db from src.models.goal import Goal +from src.models.parent_student import ParentStudentAssignment from src.models.session import Session as SessionModel from src.models.summary import Summary from src.models.user import User @@ -30,6 +32,8 @@ async def get_parent_dashboard( student_id: UUID, db: DBSession = Depends(get_db), + # Parent<->student access is relationship-enforced via + # assert_can_access_student (#68); links are admin/seed-provisioned. current_user: dict = Depends(require_role(["parent", "admin"])), ): """ @@ -37,12 +41,7 @@ async def get_parent_dashboard( Shows progress and recent activity """ - # Verify user is parent or admin - user_sub = current_user.get("sub") - db_user = db.query(User).filter(User.cognito_sub == user_sub).first() - - if not db_user: - raise HTTPException(status_code=404, detail="User not found") + db_user = assert_can_access_student(db, current_user, student_id) # Verify student exists student = db.query(User).filter(User.id == student_id).first() @@ -117,12 +116,15 @@ async def get_parent_dashboard( @router.get("/parent/students") async def get_parent_students( db: DBSession = Depends(get_db), + # Parent<->student access is relationship-enforced (#68); links are + # admin/seed-provisioned. current_user: dict = Depends(require_role(["parent", "admin"])), ): """ Get list of students for parent - For now, returns all students (in production, would filter by parent-student relationship) + Admins see all students. Parents see only students they are linked to + via ParentStudentAssignment. """ user_sub = current_user.get("sub") db_user = db.query(User).filter(User.cognito_sub == user_sub).first() @@ -130,8 +132,21 @@ async def get_parent_students( if not db_user: raise HTTPException(status_code=404, detail="User not found") - # Get all students (in production, filter by parent relationship) - students = db.query(User).filter(User.role == "student").all() + if db_user.role == "admin": + students = db.query(User).filter(User.role == "student").all() + else: + students = ( + db.query(User) + .join( + ParentStudentAssignment, + ParentStudentAssignment.student_id == User.id, + ) + .filter( + ParentStudentAssignment.parent_id == db_user.id, + User.role == "student", + ) + .all() + ) students_data = [ { diff --git a/src/api/handlers/enhancements.py b/src/api/handlers/enhancements.py index d95fcf7..4003b60 100644 --- a/src/api/handlers/enhancements.py +++ b/src/api/handlers/enhancements.py @@ -11,6 +11,7 @@ from sqlalchemy.orm import Session as DBSession from src.api.middleware.auth import get_current_user, require_role +from src.api.middleware.authz import assert_can_access_student from src.config.database import get_db from src.config.settings import settings from src.models.user import User @@ -35,33 +36,11 @@ async def get_conversation_history( Returns recent Q&A interactions for context - Security: Students can only access their own history. Tutors/admins can access any student's history. + Security: Students can only access their own history. Tutors can access + only their assigned students' history. Admins can access any student's + history. """ - from uuid import UUID - - # Get authenticated user from database - user_sub = current_user.get("sub") - db_user = db.query(User).filter(User.cognito_sub == user_sub).first() - - if not db_user: - raise HTTPException(status_code=404, detail="User not found") - - # Verify student_id exists - target_student = db.query(User).filter(User.id == UUID(student_id)).first() - if not target_student: - raise HTTPException(status_code=404, detail="Student not found") - - # Authorization check: Students can only access their own history - # Tutors and admins can access any student's history - user_role = db_user.role - if user_role == "student": - # Student can only access their own history - if str(db_user.id) != student_id: - raise HTTPException( - status_code=403, - detail="Access denied: You can only view your own conversation history", - ) - # Tutors and admins can access any student's history (for support purposes) + assert_can_access_student(db, current_user, student_id) conversation_history = ConversationHistory(db) @@ -91,6 +70,8 @@ async def get_conversation_context( Includes recent interactions and topics discussed """ + assert_can_access_student(db, current_user, student_id) + conversation_history = ConversationHistory(db) context = conversation_history.get_conversation_context( diff --git a/src/api/handlers/jobs.py b/src/api/handlers/jobs.py index 8caf426..0a2d7ce 100644 --- a/src/api/handlers/jobs.py +++ b/src/api/handlers/jobs.py @@ -12,9 +12,11 @@ from fastapi import APIRouter, Depends, HTTPException, WebSocket, WebSocketDisconnect from sqlalchemy.orm import Session as DBSession -from src.api.middleware.auth import get_current_user_optional -from src.config.database import get_db +from src.api.middleware.auth import get_current_user +from src.api.middleware.authz import assert_can_access_student +from src.config.database import SessionLocal, get_db from src.models.job import Job, JobStatus +from src.services.auth import InvalidTokenError, decode_token logger = logging.getLogger(__name__) @@ -28,7 +30,7 @@ async def get_job_status( job_id: UUID, db: DBSession = Depends(get_db), - current_user: dict = Depends(get_current_user_optional), + current_user: dict = Depends(get_current_user), ): """ Get job status by ID @@ -41,6 +43,8 @@ async def get_job_status( if not job: raise HTTPException(status_code=404, detail="Job not found") + assert_can_access_student(db, current_user, job.student_id or job.user_id) + response = { "job_id": str(job.id), "job_type": job.job_type, @@ -69,6 +73,30 @@ async def websocket_job_updates(websocket: WebSocket, job_id: UUID): """ await websocket.accept() + # Authenticate on connect: the token is a query param since WebSocket + # handshakes can't carry an Authorization header from browsers. Reject + # (close 1008) on a missing/invalid token, an unknown job, or no access. + db = SessionLocal() + try: + token = websocket.query_params.get("token") + current_user = decode_token(token) if token else None + job = db.query(Job).filter(Job.id == job_id).first() if current_user else None + if job is not None: + assert_can_access_student(db, current_user, job.student_id or job.user_id) + except (InvalidTokenError, HTTPException): + job = None + except Exception: + # Fail closed on any unexpected error (e.g. a DB error during the + # lookup) — never let an internal failure leave the socket hanging. + logger.exception("WebSocket auth failed for job %s", job_id) + job = None + finally: + db.close() + + if job is None: + await websocket.close(code=1008) + return + # Add to active connections if job_id not in active_connections: active_connections[job_id] = [] @@ -78,8 +106,6 @@ async def websocket_job_updates(websocket: WebSocket, job_id: UUID): try: # Get initial job state - from src.config.database import SessionLocal - db = SessionLocal() try: job = db.query(Job).filter(Job.id == job_id).first() diff --git a/src/api/handlers/messaging.py b/src/api/handlers/messaging.py index 4556dbb..42b530f 100644 --- a/src/api/handlers/messaging.py +++ b/src/api/handlers/messaging.py @@ -14,6 +14,7 @@ from sqlalchemy.orm import Session as DBSession from src.api.middleware.auth import get_current_user, require_role +from src.api.middleware.authz import assert_can_access_student from src.api.schemas.messaging import ( CreateThreadRequest, MessageResponse, @@ -239,15 +240,17 @@ async def list_threads( Tutors see threads they created Students see threads they're part of - If user_id is provided, use that user (for frontend compatibility) + If user_id is provided, it must be authorized via + assert_can_access_student (self, admin, or tutor-of-student). Otherwise, use the authenticated user from JWT token """ - # Support user_id query parameter for frontend compatibility if user_id: try: - db_user = db.query(User).filter(User.id == UUID(user_id)).first() + user_uuid = UUID(user_id) except ValueError: raise HTTPException(status_code=400, detail="Invalid user_id format") + assert_can_access_student(db, current_user, user_uuid) + db_user = db.query(User).filter(User.id == user_uuid).first() else: # Use authenticated user from JWT token user_sub = current_user.get("sub") diff --git a/src/api/handlers/nudges.py b/src/api/handlers/nudges.py index 689c707..ce81410 100644 --- a/src/api/handlers/nudges.py +++ b/src/api/handlers/nudges.py @@ -14,7 +14,8 @@ from sqlalchemy import func from sqlalchemy.orm import Session as DBSession -from src.api.middleware.auth import get_current_user_optional +from src.api.middleware.auth import get_current_user +from src.api.middleware.authz import assert_can_access_student from src.config.database import get_db from src.models.nudge import Nudge from src.models.user import User @@ -38,13 +39,15 @@ class NudgeEngageRequest(BaseModel): async def check_nudge( request: NudgeCheckRequest, db: DBSession = Depends(get_db), - current_user: dict = Depends(get_current_user_optional), + current_user: dict = Depends(get_current_user), ): """ Check if a student should receive a nudge Called by scheduled job or on login """ + assert_can_access_student(db, current_user, request.student_id) + engine = NudgeEngine(db) result = engine.should_send_nudge(request.student_id, request.check_type) @@ -127,7 +130,7 @@ async def check_nudge( async def get_user_nudges( user_id: UUID, db: DBSession = Depends(get_db), - current_user: dict = Depends(get_current_user_optional), + current_user: dict = Depends(get_current_user), ): """ Get active nudges for a user @@ -135,6 +138,8 @@ async def get_user_nudges( Called by React frontend on login/dashboard load Returns nudges that haven't been dismissed and are still relevant """ + assert_can_access_student(db, current_user, user_id) + from datetime import timedelta # Get recent nudges (last 7 days) that haven't been opened yet @@ -300,7 +305,7 @@ async def track_nudge_engagement( nudge_id: UUID, request: NudgeEngageRequest, db: DBSession = Depends(get_db), - current_user: dict = Depends(get_current_user_optional), + current_user: dict = Depends(get_current_user), ): """ Track nudge engagement (opened/clicked) @@ -311,6 +316,8 @@ async def track_nudge_engagement( if not nudge: raise HTTPException(status_code=404, detail="Nudge not found") + assert_can_access_student(db, current_user, nudge.user_id) + if request.engagement_type == "opened" and not nudge.opened_at: nudge.opened_at = datetime.now(timezone.utc) elif request.engagement_type == "clicked" and not nudge.clicked_at: diff --git a/src/api/handlers/overrides.py b/src/api/handlers/overrides.py index dd29247..1e82f13 100644 --- a/src/api/handlers/overrides.py +++ b/src/api/handlers/overrides.py @@ -14,20 +14,20 @@ from sqlalchemy.orm import Session as DBSession from src.api.middleware.auth import get_current_user, require_role +from src.api.middleware.authz import _normalize_uuid, assert_can_access_student from src.config.database import get_db from src.models.override import Override from src.models.practice import PracticeAssignment from src.models.summary import Summary -from src.models.user import User router = APIRouter(prefix="/overrides", tags=["overrides"]) class OverrideRequest(BaseModel): tutor_id: str - student_id: str + student_id: UUID override_type: str # "summary" | "practice" | "qa_answer" - target_id: str # ID of item being overridden + target_id: UUID # ID of item being overridden action: str new_content: Dict[str, Any] reason: Optional[str] = None @@ -44,10 +44,10 @@ async def create_override( Called by Rails app when tutor overrides AI """ - # Verify tutor - tutor = db.query(User).filter(User.id == request.tutor_id).first() - if not tutor or tutor.role not in ["tutor", "admin"]: - raise HTTPException(status_code=403, detail="Only tutors can create overrides") + # Caller must be the student's assigned tutor (or admin) - the + # authenticated caller's id, not the body-supplied tutor_id, is what + # gets persisted below. + db_user = assert_can_access_student(db, current_user, request.student_id) # Get the item being overridden original_content = {} @@ -61,6 +61,10 @@ async def create_override( summary = db.query(Summary).filter(Summary.id == request.target_id).first() if not summary: raise HTTPException(status_code=404, detail="Summary not found") + # The target must belong to the authorized student; otherwise a tutor + # assigned to student A could edit student B's summary via target_id. + if _normalize_uuid(summary.student_id) != _normalize_uuid(request.student_id): + raise HTTPException(status_code=403, detail="Access denied") original_content = { "next_steps": summary.next_steps, "narrative": summary.narrative, @@ -83,6 +87,9 @@ async def create_override( ) if not practice: raise HTTPException(status_code=404, detail="Practice assignment not found") + # The target must belong to the authorized student (see above). + if _normalize_uuid(practice.student_id) != _normalize_uuid(request.student_id): + raise HTTPException(status_code=403, detail="Access denied") original_content = { "question": practice.ai_question_text or (practice.bank_item.question_text if practice.bank_item else ""), @@ -103,8 +110,8 @@ async def create_override( # Create override record override = Override( id=uuid.uuid4(), - tutor_id=uuid.UUID(request.tutor_id), - student_id=uuid.UUID(request.student_id), + tutor_id=db_user.id, + student_id=request.student_id, override_type=request.override_type, action=request.action, summary_id=summary_id, @@ -132,8 +139,8 @@ async def create_override( "success": True, "data": { "override_id": str(override.id), - "tutor_id": request.tutor_id, - "student_id": request.student_id, + "tutor_id": str(override.tutor_id), + "student_id": str(request.student_id), "override_type": request.override_type, "action": request.action, "dashboard_updated": True, @@ -153,6 +160,8 @@ async def get_overrides( """ Get all overrides for a student (tutor view) """ + assert_can_access_student(db, current_user, student_id) + overrides = ( db.query(Override) .filter(Override.student_id == student_id) diff --git a/src/api/handlers/practice.py b/src/api/handlers/practice.py index da9ce01..431bca5 100644 --- a/src/api/handlers/practice.py +++ b/src/api/handlers/practice.py @@ -26,7 +26,8 @@ from sqlalchemy.orm import Session as DBSession from starlette.concurrency import run_in_threadpool -from src.api.middleware.auth import get_current_user_optional +from src.api.middleware.auth import get_current_user +from src.api.middleware.authz import assert_can_access_student from src.config.database import get_db from src.models.job import Job, JobStatus from src.models.practice import PracticeAssignment, PracticeBankItem, StudentRating @@ -58,7 +59,7 @@ class CompletePracticeRequest(BaseModel): class AsyncPracticeRequest(BaseModel): """Request body for async practice assignment""" - student_id: str + student_id: UUID subject: str topic: Optional[str] = None num_items: int = 5 @@ -73,7 +74,7 @@ async def assign_practice_async( request: AsyncPracticeRequest, background_tasks: BackgroundTasks, db: DBSession = Depends(get_db), - current_user: dict = Depends(get_current_user_optional), + current_user: dict = Depends(get_current_user), ): """ Assign practice items asynchronously (returns immediately with job ID) @@ -84,11 +85,13 @@ async def assign_practice_async( - Connect via WebSocket to /api/v1/jobs/{job_id}/ws for real-time updates - Provide a webhook_url to receive completion notification """ + assert_can_access_student(db, current_user, request.student_id) + job_service = PracticeJobService(db) # Create job job = job_service.create_job( - student_id=request.student_id, + student_id=str(request.student_id), subject=request.subject, topic=request.topic, num_items=request.num_items, @@ -111,19 +114,21 @@ async def assign_practice_async( @router.post("/assign") async def assign_practice( - student_id: str, + student_id: UUID, subject: str, topic: Optional[str] = None, num_items: int = 5, goal_tags: Optional[list[str]] = None, db: DBSession = Depends(get_db), - current_user: dict = Depends(get_current_user_optional), + current_user: dict = Depends(get_current_user), ): """ Assign adaptive practice items to a student Called by Rails app or React frontend """ + assert_can_access_student(db, current_user, student_id) + # Get subject - handle case-insensitive and suggest similar subjects if not found subject_obj = ( db.query(Subject) @@ -159,7 +164,7 @@ async def assign_practice( # Get student rating for this subject student_rating = adaptive_service.get_student_rating( - student_id, str(subject_obj.id) + str(student_id), str(subject_obj.id) ) # Select difficulty range @@ -270,7 +275,7 @@ async def assign_practice( break assignment = PracticeAssignment( id=uuid.uuid4(), - student_id=uuid.UUID(student_id), + student_id=student_id, source="bank", bank_item_id=bank_item.id, subject_id=subject_obj.id, @@ -348,7 +353,7 @@ async def assign_practice( assignment = PracticeAssignment( id=uuid.uuid4(), - student_id=uuid.UUID(student_id), + student_id=student_id, source="ai_generated", ai_question_text=ai_item_data["question_text"], ai_answer_text=ai_item_data["answer_text"], @@ -428,12 +433,12 @@ async def complete_practice( assignment_id: str = Query( ..., description="Practice assignment ID (for compatibility)" ), - item_id: str = Query( + item_id: UUID = Query( ..., description="Practice item ID (actual PracticeAssignment.id)" ), request: CompletePracticeRequest = ..., db: DBSession = Depends(get_db), - current_user: dict = Depends(get_current_user_optional), + current_user: dict = Depends(get_current_user), ): """ Record completion of a practice item @@ -454,6 +459,8 @@ async def complete_practice( detail=f"Practice assignment not found with item_id: {item_id}", ) + assert_can_access_student(db, current_user, assignment.student_id) + # Initialize adaptive service adaptive_service = AdaptivePracticeService(db) @@ -539,9 +546,9 @@ async def complete_practice( @router.post("/summary") async def get_practice_summary( assignment_id: str = Query(..., description="Practice assignment ID"), - student_id: str = Query(..., description="Student ID"), + student_id: UUID = Query(..., description="Student ID"), db: DBSession = Depends(get_db), - current_user: dict = Depends(get_current_user_optional), + current_user: dict = Depends(get_current_user), ): """ Get summary of practice session and determine if tutor notification is needed @@ -552,6 +559,8 @@ async def get_practice_summary( - Average attempts - Whether tutor help is needed """ + assert_can_access_student(db, current_user, student_id) + from sqlalchemy import func # Get all assignments for this assignment_id (all items in the session) diff --git a/src/api/handlers/summaries.py b/src/api/handlers/summaries.py index 2542362..8371f6a 100644 --- a/src/api/handlers/summaries.py +++ b/src/api/handlers/summaries.py @@ -12,11 +12,11 @@ from sqlalchemy.orm import Session as DBSession from src.api.middleware.auth import get_current_user, get_current_user_optional +from src.api.middleware.authz import assert_can_access_student from src.api.schemas.summaries import CreateSummaryRequest, SummaryResponse from src.config.database import get_db from src.models.session import Session as SessionModel from src.models.summary import Summary -from src.models.user import User from src.services.ai.summarizer import SessionSummarizer from src.services.goals.progress import GoalProgressService @@ -114,11 +114,7 @@ async def get_summaries( Get all summaries for a user (student or tutor view) """ # Verify user has access - user_sub = current_user.get("sub") - db_user = db.query(User).filter(User.cognito_sub == user_sub).first() - - if not db_user: - raise HTTPException(status_code=404, detail="User not found") + db_user = assert_can_access_student(db, current_user, user_id) # Get summaries query = db.query(Summary).filter(Summary.student_id == user_id) diff --git a/src/api/middleware/authz.py b/src/api/middleware/authz.py new file mode 100644 index 0000000..8fd2aab --- /dev/null +++ b/src/api/middleware/authz.py @@ -0,0 +1,82 @@ +""" +Shared object-access helper (security remediation Phase 1, #60). + +Endpoint handlers were each hand-rolling their own "does this caller own +this student's data" check. This centralizes that access model: + + - student -> may access only their own student_id + - tutor -> may access only students they have a TutorStudentAssignment + with + - parent -> may access only students they have a ParentStudentAssignment + with (#68); links are admin/seed-provisioned + - admin -> may access anything +""" + +import uuid + +from fastapi import HTTPException +from sqlalchemy.orm import Session + +from src.models.parent_student import ParentStudentAssignment +from src.models.tutor_student import TutorStudentAssignment +from src.models.user import User + + +def _normalize_uuid(value) -> str: + """Canonicalize a UUID (or UUID-like string, dashed or hex) for + comparison, so ids that are equal but differently formatted still + match.""" + return str(uuid.UUID(str(value))) + + +def assert_can_access_student( + db: Session, current_user: dict, target_student_id +) -> User: + """Raise HTTP 403 unless current_user may access target_student_id. + + Returns the caller's own db_user row on success, since callers + frequently need it right after the check. + """ + db_user = db.query(User).filter(User.cognito_sub == current_user.get("sub")).first() + if not db_user: + raise HTTPException(status_code=403, detail="Access denied") + + if target_student_id is None: + raise HTTPException(status_code=403, detail="Access denied") + + try: + normalized_target = _normalize_uuid(target_student_id) + except (ValueError, TypeError): + raise HTTPException(status_code=403, detail="Access denied") + + if db_user.role == "admin": + return db_user + + if _normalize_uuid(db_user.id) == normalized_target: + return db_user + + if db_user.role == "tutor": + assignment = ( + db.query(TutorStudentAssignment) + .filter( + TutorStudentAssignment.tutor_id == db_user.id, + TutorStudentAssignment.student_id == uuid.UUID(str(target_student_id)), + ) + .first() + ) + if assignment: + return db_user + + if db_user.role == "parent": + assignment = ( + db.query(ParentStudentAssignment) + .filter( + ParentStudentAssignment.parent_id == db_user.id, + ParentStudentAssignment.student_id == uuid.UUID(str(target_student_id)), + ) + .first() + ) + if assignment: + return db_user + + raise HTTPException(status_code=403, detail="Access denied") diff --git a/src/config/settings.py b/src/config/settings.py index 746f91a..e5f7061 100644 --- a/src/config/settings.py +++ b/src/config/settings.py @@ -58,11 +58,6 @@ class Settings(BaseSettings): default="http://localhost:8000", description="API base URL" ) - # API Keys - ai_service_api_key: Optional[str] = Field( - default=None, description="Service API key" - ) - # JWT Authentication jwt_secret: str = Field(default="", description="JWT signing secret") jwt_expiry_minutes: int = Field( @@ -124,7 +119,6 @@ class Settings(BaseSettings): # ======================================================================== # External Service URLs # ======================================================================== - rails_app_url: Optional[str] = Field(default=None, description="Rails app URL") webhook_secret: Optional[str] = Field(default=None, description="Webhook secret") # ======================================================================== diff --git a/src/models/__init__.py b/src/models/__init__.py index 4881971..4b6cae8 100644 --- a/src/models/__init__.py +++ b/src/models/__init__.py @@ -8,6 +8,7 @@ from src.models.messaging import Message, MessageThread from src.models.nudge import Nudge from src.models.override import Override +from src.models.parent_student import ParentStudentAssignment from src.models.practice import PracticeAssignment, PracticeBankItem, StudentRating from src.models.qa import QAInteraction from src.models.session import Session @@ -29,6 +30,7 @@ "Nudge", "Override", "TutorStudentAssignment", + "ParentStudentAssignment", "MessageThread", "Message", "Integration", diff --git a/src/models/parent_student.py b/src/models/parent_student.py new file mode 100644 index 0000000..5786136 --- /dev/null +++ b/src/models/parent_student.py @@ -0,0 +1,54 @@ +""" +Parent-Student Assignment Model +""" + +from sqlalchemy import Column, DateTime, ForeignKey, PrimaryKeyConstraint, String +from sqlalchemy.dialects.postgresql import UUID +from sqlalchemy.orm import relationship +from sqlalchemy.sql import func + +from src.models.base import Base + + +class ParentStudentAssignment(Base): + __tablename__ = "parent_student_assignments" + + parent_id = Column( + UUID(as_uuid=True), + ForeignKey("users.id", ondelete="CASCADE"), + nullable=False, + index=True, + ) + student_id = Column( + UUID(as_uuid=True), + ForeignKey("users.id", ondelete="CASCADE"), + nullable=False, + index=True, + ) + + status = Column( + String(20), default="active", index=True + ) # active, paused, completed + + created_at = Column( + DateTime(timezone=True), server_default=func.now(), nullable=False + ) + updated_at = Column( + DateTime(timezone=True), + server_default=func.now(), + onupdate=func.now(), + nullable=False, + ) + + __table_args__ = (PrimaryKeyConstraint("parent_id", "student_id"),) + + # Relationships + parent = relationship( + "User", foreign_keys=[parent_id], backref="parent_assignments" + ) + student = relationship( + "User", foreign_keys=[student_id], backref="parent_student_assignments" + ) + + def __repr__(self): + return f"" diff --git a/tests/_authz_utils.py b/tests/_authz_utils.py new file mode 100644 index 0000000..472d0e8 --- /dev/null +++ b/tests/_authz_utils.py @@ -0,0 +1,56 @@ +""" +Shared test utilities for creating authenticated users (security +remediation Phase 1, #60). + +Later-phase ownership/access-control tests that need an "owner" and an +unrelated "attacker" authenticated user can import these instead of +duplicating the _create_user/_token/_auth pattern already used in +tests/test_auth_http_ownership.py. +""" + +import uuid + +from src.services.auth import create_access_token +from tests.test_models import TestUser + + +def make_id() -> str: + return uuid.uuid4().hex + + +def create_user(db_session, email, role="student", cognito_sub=None): + user = TestUser( + id=make_id(), + cognito_sub=cognito_sub or make_id(), + email=email, + role=role, + ) + db_session.add(user) + db_session.commit() + return user + + +def token_for(user, **kw): + return create_access_token( + sub=user.cognito_sub, email=user.email, role=user.role, **kw + ) + + +def auth_headers(token): + return {"Authorization": f"Bearer {token}"} + + +def make_authed_pair(db_session, role="student"): + """Create two distinct authenticated users - an "owner" and an + unrelated "attacker" - for ownership-boundary tests. + + Returns (owner, owner_headers, attacker, attacker_headers). + """ + owner = create_user(db_session, f"owner-{make_id()}@example.com", role=role) + attacker = create_user(db_session, f"attacker-{make_id()}@example.com", role=role) + return ( + owner, + auth_headers(token_for(owner)), + attacker, + auth_headers(token_for(attacker)), + ) diff --git a/tests/test_auth_endpoints.py b/tests/test_auth_endpoints.py index 471e5b3..3509b0b 100644 --- a/tests/test_auth_endpoints.py +++ b/tests/test_auth_endpoints.py @@ -119,6 +119,30 @@ def test_register_rejects_admin_role(self, client): ) assert resp.status_code == 422 + def test_register_rejects_tutor_role(self, client): + # #60: tutor accounts are admin-provisioned, not self-registered. + resp = client.post( + "/api/v1/auth/register", + json={ + "email": "wannabetutor@example.com", + "password": "password123", + "role": "tutor", + }, + ) + assert resp.status_code == 422 + + def test_register_rejects_parent_role(self, client): + # #60: parent accounts are admin-provisioned, not self-registered. + resp = client.post( + "/api/v1/auth/register", + json={ + "email": "wannabeparent@example.com", + "password": "password123", + "role": "parent", + }, + ) + assert resp.status_code == 422 + class TestLogin: def test_login_happy_path(self, client, db_session): diff --git a/tests/test_authz.py b/tests/test_authz.py new file mode 100644 index 0000000..a4d50ca --- /dev/null +++ b/tests/test_authz.py @@ -0,0 +1,150 @@ +""" +Unit tests for src/api/middleware/authz.py:assert_can_access_student +(security remediation Phase 1, #60). +""" + +import pytest +from fastapi import HTTPException + +from src.api.middleware.authz import assert_can_access_student +from tests._authz_utils import create_user, make_id +from tests.test_models import TestTutorStudentAssignment + + +def _current_user(user): + return {"sub": user.cognito_sub, "email": user.email, "role": user.role} + + +def _assign(db_session, tutor, student): + assignment = TestTutorStudentAssignment( + tutor_id=tutor.id, + student_id=student.id, + subject_id=make_id(), + status="active", + ) + db_session.add(assignment) + db_session.commit() + return assignment + + +class TestAssertCanAccessStudent: + def test_owner_allowed(self, db_session): + student = create_user(db_session, "owner@example.com", role="student") + + result = assert_can_access_student( + db_session, _current_user(student), student.id + ) + + assert result.id.hex == student.id + + def test_unrelated_student_returns_403(self, db_session): + student_a = create_user(db_session, "a@example.com", role="student") + student_b = create_user(db_session, "b@example.com", role="student") + + with pytest.raises(HTTPException) as exc_info: + assert_can_access_student( + db_session, _current_user(student_a), student_b.id + ) + + assert exc_info.value.status_code == 403 + + def test_tutor_with_assignment_allowed(self, db_session): + tutor = create_user(db_session, "tutor@example.com", role="tutor") + student = create_user(db_session, "student-t@example.com", role="student") + _assign(db_session, tutor, student) + + result = assert_can_access_student(db_session, _current_user(tutor), student.id) + + assert result.id.hex == tutor.id + + def test_tutor_without_assignment_returns_403(self, db_session): + tutor = create_user(db_session, "tutor2@example.com", role="tutor") + student = create_user(db_session, "student-u@example.com", role="student") + + with pytest.raises(HTTPException) as exc_info: + assert_can_access_student(db_session, _current_user(tutor), student.id) + + assert exc_info.value.status_code == 403 + + def test_parent_treated_as_non_owner_returns_403(self, db_session): + parent = create_user(db_session, "parent@example.com", role="parent") + student = create_user(db_session, "student-v@example.com", role="student") + + with pytest.raises(HTTPException) as exc_info: + assert_can_access_student(db_session, _current_user(parent), student.id) + + assert exc_info.value.status_code == 403 + + def test_admin_allowed(self, db_session): + admin = create_user(db_session, "admin@example.com", role="admin") + student = create_user(db_session, "student-w@example.com", role="student") + + result = assert_can_access_student(db_session, _current_user(admin), student.id) + + assert result.id.hex == admin.id + + def test_unknown_sub_returns_403(self, db_session): + student = create_user(db_session, "student-x@example.com", role="student") + + with pytest.raises(HTTPException) as exc_info: + assert_can_access_student( + db_session, {"sub": "not-in-db", "email": "x@example.com"}, student.id + ) + + assert exc_info.value.status_code == 403 + + def test_none_target_returns_403_for_student(self, db_session): + student = create_user( + db_session, "none-target-student@example.com", role="student" + ) + + with pytest.raises(HTTPException) as exc_info: + assert_can_access_student(db_session, _current_user(student), None) + + assert exc_info.value.status_code == 403 + + def test_none_target_returns_403_for_tutor(self, db_session): + tutor = create_user(db_session, "none-target-tutor@example.com", role="tutor") + + with pytest.raises(HTTPException) as exc_info: + assert_can_access_student(db_session, _current_user(tutor), None) + + assert exc_info.value.status_code == 403 + + def test_none_target_returns_403_for_admin(self, db_session): + admin = create_user(db_session, "none-target-admin@example.com", role="admin") + + with pytest.raises(HTTPException) as exc_info: + assert_can_access_student(db_session, _current_user(admin), None) + + assert exc_info.value.status_code == 403 + + def test_malformed_target_returns_403_for_student(self, db_session): + student = create_user( + db_session, "malformed-target-student@example.com", role="student" + ) + + with pytest.raises(HTTPException) as exc_info: + assert_can_access_student(db_session, _current_user(student), "abc") + + assert exc_info.value.status_code == 403 + + def test_malformed_target_returns_403_for_tutor(self, db_session): + tutor = create_user( + db_session, "malformed-target-tutor@example.com", role="tutor" + ) + + with pytest.raises(HTTPException) as exc_info: + assert_can_access_student(db_session, _current_user(tutor), "abc") + + assert exc_info.value.status_code == 403 + + def test_malformed_target_returns_403_for_admin(self, db_session): + admin = create_user( + db_session, "malformed-target-admin@example.com", role="admin" + ) + + with pytest.raises(HTTPException) as exc_info: + assert_can_access_student(db_session, _current_user(admin), "abc") + + assert exc_info.value.status_code == 403 diff --git a/tests/test_authz_overrides.py b/tests/test_authz_overrides.py new file mode 100644 index 0000000..5001eee --- /dev/null +++ b/tests/test_authz_overrides.py @@ -0,0 +1,190 @@ +""" +HTTP-level authz tests for src/api/handlers/overrides.py (security +remediation Phase 4). + +Before this phase, POST /overrides/ trusted the body-supplied tutor_id and +student_id with no check that the caller was actually the student's +assigned tutor, and GET /overrides/{student_id} returned override history +to any authenticated tutor/admin regardless of assignment. These tests +pin the assert_can_access_student boundary on both routes. +""" + +import uuid + +import pytest + +from tests._authz_utils import auth_headers, create_user, make_id, token_for +from tests.test_models import TestPracticeAssignment, TestTutorStudentAssignment + + +@pytest.fixture(autouse=True) +def _jwt_secret(monkeypatch): + monkeypatch.setattr("src.config.settings.settings.jwt_secret", "test-secret") + + +def _assign(db_session, tutor, student): + assignment = TestTutorStudentAssignment( + tutor_id=tutor.id, + student_id=student.id, + subject_id=make_id(), + status="active", + ) + db_session.add(assignment) + db_session.commit() + return assignment + + +def _create_practice(db_session, student): + practice = TestPracticeAssignment( + id=make_id(), + student_id=student.id, + source="ai_generated", + ai_question_text="What is 2+2?", + ai_answer_text="4", + ) + db_session.add(practice) + db_session.commit() + return practice + + +class TestCreateOverrideAuthz: + def test_unrelated_tutor_returns_403_and_does_not_mutate(self, client, db_session): + tutor = create_user(db_session, "tutor-unrelated@example.com", role="tutor") + student = create_user(db_session, "student-ov-a@example.com", role="student") + practice = _create_practice(db_session, student) + token = token_for(tutor) + + resp = client.post( + "/api/v1/overrides/", + json={ + "tutor_id": tutor.id, + "student_id": student.id, + "override_type": "practice", + "target_id": practice.id, + "action": "edit", + "new_content": {"question": "hacked"}, + }, + headers=auth_headers(token), + ) + + assert resp.status_code == 403 + db_session.refresh(practice) + assert practice.overridden is False + assert practice.ai_question_text == "What is 2+2?" + + def test_assigned_tutor_cannot_override_other_students_target_via_target_id( + self, client, db_session + ): + # Tutor is legitimately assigned to student A (so + # assert_can_access_student passes on student_id=A), but the + # target_id points at student B's practice assignment. This is the + # BOLA the ownership check on the target guards against: the + # student_id check alone is not enough because target_id is never + # otherwise tied to student_id. + tutor = create_user(db_session, "tutor-bola@example.com", role="tutor") + student_a = create_user( + db_session, "student-ov-bola-a@example.com", role="student" + ) + student_b = create_user( + db_session, "student-ov-bola-b@example.com", role="student" + ) + _assign(db_session, tutor, student_a) + practice_b = _create_practice(db_session, student_b) + token = token_for(tutor) + + resp = client.post( + "/api/v1/overrides/", + json={ + "tutor_id": tutor.id, + "student_id": student_a.id, + "override_type": "practice", + "target_id": practice_b.id, + "action": "edit", + "new_content": {"question": "hacked"}, + }, + headers=auth_headers(token), + ) + + assert resp.status_code == 403 + db_session.refresh(practice_b) + assert practice_b.overridden is False + assert practice_b.ai_question_text == "What is 2+2?" + + def test_assigned_tutor_success_tutor_id_derived_from_jwt(self, client, db_session): + # override_type "qa_answer" is used here (rather than "practice" or + # "summary") to isolate the identity-derivation behavior under test + # from the target-lookup branches, which round-trip target_id + # through the real (Postgres-UUID) models and are not exercised by + # this authz-focused test file. + tutor = create_user(db_session, "tutor-assigned@example.com", role="tutor") + other_tutor = create_user(db_session, "tutor-other@example.com", role="tutor") + student = create_user(db_session, "student-ov-b@example.com", role="student") + _assign(db_session, tutor, student) + token = token_for(tutor) + + resp = client.post( + "/api/v1/overrides/", + json={ + # attacker-supplied identity - must be ignored in favor of + # the caller's own JWT-derived id. + "tutor_id": other_tutor.id, + "student_id": student.id, + "override_type": "qa_answer", + "target_id": make_id(), + "action": "edit", + "new_content": {"answer": "corrected answer"}, + }, + headers=auth_headers(token), + ) + + assert resp.status_code == 200 + body = resp.json() + assert body["data"]["tutor_id"] == str(uuid.UUID(tutor.id)) + assert body["data"]["tutor_id"] != str(uuid.UUID(other_tutor.id)) + + def test_malformed_target_id_returns_422(self, client, db_session): + tutor = create_user(db_session, "tutor-malformed@example.com", role="tutor") + student = create_user( + db_session, "student-malformed@example.com", role="student" + ) + _assign(db_session, tutor, student) + token = token_for(tutor) + + resp = client.post( + "/api/v1/overrides/", + json={ + "tutor_id": tutor.id, + "student_id": student.id, + "override_type": "practice", + "target_id": "not-a-uuid", + "action": "edit", + "new_content": {"question": "hacked"}, + }, + headers=auth_headers(token), + ) + + assert resp.status_code == 422 + + +class TestGetOverridesAuthz: + def test_unrelated_tutor_returns_403(self, client, db_session): + tutor = create_user(db_session, "tutor-hist-a@example.com", role="tutor") + student = create_user(db_session, "student-hist-a@example.com", role="student") + token = token_for(tutor) + + resp = client.get( + f"/api/v1/overrides/{student.id}", headers=auth_headers(token) + ) + + assert resp.status_code == 403 + + def test_admin_returns_2xx(self, client, db_session): + admin = create_user(db_session, "admin-hist@example.com", role="admin") + student = create_user(db_session, "student-hist-b@example.com", role="student") + token = token_for(admin) + + resp = client.get( + f"/api/v1/overrides/{student.id}", headers=auth_headers(token) + ) + + assert resp.status_code == 200 diff --git a/tests/test_authz_parent.py b/tests/test_authz_parent.py new file mode 100644 index 0000000..f84993a --- /dev/null +++ b/tests/test_authz_parent.py @@ -0,0 +1,120 @@ +""" +HTTP-level access-control tests for parent<->student linkage (#68). + +Parent<->student links are ADMIN/seed-provisioned; there is no +self-service linking endpoint. These tests cover: + + - GET /dashboards/parent/student/{student_id} + - GET /dashboards/parent/students + - GET /analytics/advanced/engagement/{user_id} +""" + +import uuid + +import pytest + +from tests._authz_utils import auth_headers, create_user, token_for +from tests.test_models import TestParentStudentAssignment + + +@pytest.fixture(autouse=True) +def _jwt_secret(monkeypatch): + monkeypatch.setattr("src.config.settings.settings.jwt_secret", "test-secret") + + +def _link(db_session, parent, student): + assignment = TestParentStudentAssignment( + parent_id=parent.id, + student_id=student.id, + status="active", + ) + db_session.add(assignment) + db_session.commit() + return assignment + + +class TestParentDashboardAuthz: + # No linked-parent -> 200 case for GET /parent/student/{student_id} + # here: same pre-existing test-harness limitation documented in + # tests/test_authz_reads.py's TestDashboardsParentLockedToAdmin - the + # handler layers the real src.models.user.User model against + # AnalyticsAggregator's student lookup, and no id format satisfies + # both under SQLite. The authz gate itself (assert_can_access_student) + # runs before that lookup and is exercised by the 403 case below plus + # TestEngagementParentAuthz, which shares the same helper. + + def test_unlinked_parent_returns_403(self, client, db_session): + parent = create_user(db_session, "unlink-parent-a@example.com", role="parent") + student = create_user( + db_session, "unlink-student-a@example.com", role="student" + ) + parent_headers = auth_headers(token_for(parent)) + + resp = client.get( + f"/api/v1/dashboards/parent/student/{student.id}", + headers=parent_headers, + ) + + assert resp.status_code == 403 + + +class TestParentStudentsListAuthz: + def test_parent_sees_only_linked_students(self, client, db_session): + parent = create_user(db_session, "list-parent-a@example.com", role="parent") + linked = create_user( + db_session, "list-linked-student@example.com", role="student" + ) + unlinked = create_user( + db_session, "list-unlinked-student@example.com", role="student" + ) + _link(db_session, parent, linked) + parent_headers = auth_headers(token_for(parent)) + + resp = client.get("/api/v1/dashboards/parent/students", headers=parent_headers) + + assert resp.status_code == 200 + student_ids = {s["student_id"] for s in resp.json()["data"]["students"]} + assert str(uuid.UUID(linked.id)) in student_ids + assert str(uuid.UUID(unlinked.id)) not in student_ids + + def test_admin_sees_all_students(self, client, db_session): + admin = create_user(db_session, "list-admin-a@example.com", role="admin") + create_user(db_session, "list-admin-student-a@example.com", role="student") + create_user(db_session, "list-admin-student-b@example.com", role="student") + admin_headers = auth_headers(token_for(admin)) + + resp = client.get("/api/v1/dashboards/parent/students", headers=admin_headers) + + assert resp.status_code == 200 + assert resp.json()["data"]["total"] >= 2 + + +class TestEngagementParentAuthz: + def test_linked_parent_returns_200(self, client, db_session): + parent = create_user(db_session, "eng-parent-a@example.com", role="parent") + student = create_user( + db_session, "eng-parent-student-a@example.com", role="student" + ) + _link(db_session, parent, student) + parent_headers = auth_headers(token_for(parent)) + + resp = client.get( + f"/api/v1/analytics/advanced/engagement/{student.id}", + headers=parent_headers, + ) + + assert resp.status_code == 200 + + def test_unlinked_parent_returns_403(self, client, db_session): + parent = create_user(db_session, "eng-parent-b@example.com", role="parent") + student = create_user( + db_session, "eng-parent-student-b@example.com", role="student" + ) + parent_headers = auth_headers(token_for(parent)) + + resp = client.get( + f"/api/v1/analytics/advanced/engagement/{student.id}", + headers=parent_headers, + ) + + assert resp.status_code == 403 diff --git a/tests/test_authz_practice_nudges_jobs.py b/tests/test_authz_practice_nudges_jobs.py new file mode 100644 index 0000000..907145f --- /dev/null +++ b/tests/test_authz_practice_nudges_jobs.py @@ -0,0 +1,498 @@ +""" +HTTP-level authz regression tests for Phase 2 of the broken-access-control +remediation (security remediation Phase 2). + +practice.py, nudges.py, and jobs.py handlers used to depend on +get_current_user_optional and never actually checked whether the caller was +allowed to touch the target student's data. These tests pin the fix: every +route now requires auth (401 with none) and enforces ownership via +assert_can_access_student (403 for an unrelated authenticated "attacker", +2xx for the resource owner). + +Note on IDs: like tests/test_auth_http_ownership.py, rows created through the +SQLite shadow models here use uuid.uuid4().hex (no dashes). The real models +these handlers query (User, PracticeAssignment, Nudge, Job, ...) have +UUID(as_uuid=True) columns whose SQLite bind processor normalizes lookup +values to that same dashless hex form, so a dashed id written via the shadow +model would silently never match. This is a pre-existing quirk of the +SQLite test harness, not an auth bug, and doesn't occur in production +(Postgres does native UUID compares either way). +""" + +import uuid +from datetime import datetime, timezone +from unittest.mock import patch + +import pytest +from sqlalchemy import JSON, Column, DateTime, Integer, String, Text +from starlette.websockets import WebSocketDisconnect + +from tests._authz_utils import make_authed_pair, make_id, token_for +from tests.test_models import TestBase, TestNudge, TestPracticeAssignment, TestSubject + + +# No TestJob shadow model exists yet in tests/test_models.py, and that file +# is off-limits for this task. Registering one here (before db_session's +# TestBase.metadata.create_all runs) creates the "jobs" table the real +# src.models.job.Job queries against, the same trick the rest of this test +# suite already relies on for practice_assignments/goals/nudges/etc. +class TestJob(TestBase): + __tablename__ = "jobs" + + id = Column(String(36), primary_key=True, default=make_id) + job_type = Column(String(50), nullable=False) + status = Column(String(20), nullable=False, default="pending") + user_id = Column(String(36), nullable=True) + student_id = Column(String(36), nullable=True) + parameters = Column(JSON, nullable=False, default={}) + result = Column(JSON, nullable=True) + error_message = Column(Text, nullable=True) + progress_percent = Column(Integer, default=0) + progress_message = Column(String(255), nullable=True) + webhook_url = Column(String(500), nullable=True) + created_at = Column(DateTime(timezone=True), default=datetime.utcnow) + updated_at = Column(DateTime(timezone=True), default=datetime.utcnow) + + +@pytest.fixture(autouse=True) +def _jwt_secret(monkeypatch): + monkeypatch.setattr("src.config.settings.settings.jwt_secret", "test-secret") + + +def _create_subject(db_session, name="Algebra"): + subject = TestSubject(id=make_id(), name=name, category="Math") + db_session.add(subject) + db_session.commit() + return subject + + +class TestPracticeAssignAuthz: + """POST /api/v1/practice/assign""" + + def test_no_auth_returns_401(self, client, db_session): + subject = _create_subject(db_session) + resp = client.post( + f"/api/v1/practice/assign?student_id={uuid.uuid4()}" + f"&subject={subject.name}&num_items=0" + ) + assert resp.status_code == 401 + + def test_attacker_returns_403(self, client, db_session): + subject = _create_subject(db_session) + owner, owner_headers, attacker, attacker_headers = make_authed_pair(db_session) + resp = client.post( + f"/api/v1/practice/assign?student_id={owner.id}" + f"&subject={subject.name}&num_items=0", + headers=attacker_headers, + ) + assert resp.status_code == 403 + + def test_malformed_student_id_returns_422(self, client, db_session): + subject = _create_subject(db_session) + owner, owner_headers, attacker, attacker_headers = make_authed_pair(db_session) + resp = client.post( + f"/api/v1/practice/assign?student_id=not-a-uuid" + f"&subject={subject.name}&num_items=0", + headers=owner_headers, + ) + assert resp.status_code == 422 + + def test_owner_returns_2xx(self, client, db_session): + subject = _create_subject(db_session) + owner, owner_headers, attacker, attacker_headers = make_authed_pair(db_session) + + # find_bank_items/get_student_rating live in the adaptive-practice + # *service* layer (src/services/practice/adaptive.py), which is out + # of scope for this handler-only authz task and also hits real-model + # UUID columns with raw-string filters that don't work over SQLite. + # Stub them so this test proves the authz gate + handler wiring + # without dragging an unrelated service module into scope. + with patch( + "src.api.handlers.practice.AdaptivePracticeService.find_bank_items", + return_value=[], + ), patch( + "src.api.handlers.practice.AdaptivePracticeService.get_student_rating", + return_value=1000, + ): + resp = client.post( + f"/api/v1/practice/assign?student_id={owner.id}" + f"&subject={subject.name}&num_items=0", + headers=owner_headers, + ) + assert resp.status_code < 300 + + +class TestPracticeCompleteAuthz: + """POST /api/v1/practice/complete""" + + def _make_assignment(self, db_session, student): + # student_rating_before is set so the handler skips its "or + # adaptive_service.get_student_rating(...)" fallback, which filters + # the real StudentRating model on a raw (unconverted) string id and + # hits the same SQLite/UUID quirk described in the module docstring. + assignment = TestPracticeAssignment( + id=make_id(), + student_id=student.id, + source="bank", + student_rating_before=1000, + assigned_at=datetime.now(timezone.utc), + created_at=datetime.now(timezone.utc), + ) + db_session.add(assignment) + db_session.commit() + return assignment + + def _body(self): + return { + "student_answer": "answer", + "correct": False, + "time_taken_seconds": 10, + "hints_used": 0, + } + + def test_no_auth_returns_401(self, client, db_session): + owner, owner_headers, attacker, attacker_headers = make_authed_pair(db_session) + assignment = self._make_assignment(db_session, owner) + resp = client.post( + f"/api/v1/practice/complete?assignment_id={assignment.id}" + f"&item_id={assignment.id}", + json=self._body(), + ) + assert resp.status_code == 401 + + def test_attacker_returns_403(self, client, db_session): + owner, owner_headers, attacker, attacker_headers = make_authed_pair(db_session) + assignment = self._make_assignment(db_session, owner) + resp = client.post( + f"/api/v1/practice/complete?assignment_id={assignment.id}" + f"&item_id={assignment.id}", + json=self._body(), + headers=attacker_headers, + ) + assert resp.status_code == 403 + + def test_owner_returns_2xx(self, client, db_session): + owner, owner_headers, attacker, attacker_headers = make_authed_pair(db_session) + assignment = self._make_assignment(db_session, owner) + resp = client.post( + f"/api/v1/practice/complete?assignment_id={assignment.id}" + f"&item_id={assignment.id}", + json=self._body(), + headers=owner_headers, + ) + assert resp.status_code < 300 + + +class TestPracticeSummaryAuthz: + """POST /api/v1/practice/summary""" + + def test_no_auth_returns_401(self, client, db_session): + owner, owner_headers, attacker, attacker_headers = make_authed_pair(db_session) + resp = client.post( + f"/api/v1/practice/summary?assignment_id={uuid.uuid4()}" + f"&student_id={owner.id}" + ) + assert resp.status_code == 401 + + def test_attacker_returns_403(self, client, db_session): + owner, owner_headers, attacker, attacker_headers = make_authed_pair(db_session) + resp = client.post( + f"/api/v1/practice/summary?assignment_id={uuid.uuid4()}" + f"&student_id={owner.id}", + headers=attacker_headers, + ) + assert resp.status_code == 403 + + def test_owner_returns_2xx(self, client, db_session): + owner, owner_headers, attacker, attacker_headers = make_authed_pair(db_session) + # completed + high performance_score keeps needs_tutor_help False, + # avoiding an unrelated raw-string User.id lookup deeper in the + # handler's tutor-notification branch (same SQLite/UUID quirk noted + # in the module docstring). + assignment = TestPracticeAssignment( + id=make_id(), + student_id=owner.id, + source="bank", + completed=True, + performance_score=1.0, + assigned_at=datetime.now(timezone.utc), + created_at=datetime.now(timezone.utc), + ) + db_session.add(assignment) + db_session.commit() + + resp = client.post( + f"/api/v1/practice/summary?assignment_id={uuid.uuid4()}" + f"&student_id={owner.id}", + headers=owner_headers, + ) + assert resp.status_code < 300 + + +class TestPracticeAssignAsyncAuthz: + """POST /api/v1/practice/assign/async""" + + def _body(self, student_id): + return { + "student_id": str(student_id), + "subject": "Algebra", + "num_items": 5, + } + + def test_no_auth_returns_401(self, client, db_session): + owner, owner_headers, attacker, attacker_headers = make_authed_pair(db_session) + resp = client.post("/api/v1/practice/assign/async", json=self._body(owner.id)) + assert resp.status_code == 401 + + def test_attacker_returns_403(self, client, db_session): + owner, owner_headers, attacker, attacker_headers = make_authed_pair(db_session) + resp = client.post( + "/api/v1/practice/assign/async", + json=self._body(owner.id), + headers=attacker_headers, + ) + assert resp.status_code == 403 + + def test_owner_returns_2xx(self, client, db_session): + owner, owner_headers, attacker, attacker_headers = make_authed_pair(db_session) + with patch( + "src.api.handlers.practice.PracticeJobService.process_job", + return_value={"success": True}, + ): + resp = client.post( + "/api/v1/practice/assign/async", + json=self._body(owner.id), + headers=owner_headers, + ) + assert resp.status_code < 300 + + +class TestNudgesCheckAuthz: + """POST /api/v1/nudges/check""" + + def _body(self, student_id): + return {"student_id": str(student_id), "check_type": "inactivity"} + + def test_no_auth_returns_401(self, client, db_session): + owner, owner_headers, attacker, attacker_headers = make_authed_pair(db_session) + resp = client.post("/api/v1/nudges/check", json=self._body(owner.id)) + assert resp.status_code == 401 + + def test_attacker_returns_403(self, client, db_session): + owner, owner_headers, attacker, attacker_headers = make_authed_pair(db_session) + resp = client.post( + "/api/v1/nudges/check", + json=self._body(owner.id), + headers=attacker_headers, + ) + assert resp.status_code == 403 + + def test_owner_returns_2xx(self, client, db_session): + owner, owner_headers, attacker, attacker_headers = make_authed_pair(db_session) + resp = client.post( + "/api/v1/nudges/check", + json=self._body(owner.id), + headers=owner_headers, + ) + assert resp.status_code < 300 + + +class TestNudgesGetUserAuthz: + """GET /api/v1/nudges/users/{user_id}""" + + def test_no_auth_returns_401(self, client, db_session): + owner, owner_headers, attacker, attacker_headers = make_authed_pair(db_session) + resp = client.get(f"/api/v1/nudges/users/{owner.id}") + assert resp.status_code == 401 + + def test_attacker_returns_403(self, client, db_session): + owner, owner_headers, attacker, attacker_headers = make_authed_pair(db_session) + resp = client.get(f"/api/v1/nudges/users/{owner.id}", headers=attacker_headers) + assert resp.status_code == 403 + + def test_owner_returns_2xx(self, client, db_session): + owner, owner_headers, attacker, attacker_headers = make_authed_pair(db_session) + # A recent unopened nudge short-circuits the handler's own + # "no nudges yet -> compute a login nudge" branch, which otherwise + # calls into NudgePersonalization.get_student_insights() and hits an + # unrelated real-vs-shadow-model schema mismatch on qa_interactions + # (the real QAInteraction model has a goal_id column the SQLite + # shadow model doesn't define) - not an authz concern. + db_session.add( + TestNudge( + id=make_id(), + user_id=owner.id, + type="login", + channel="in_app", + message="hi", + sent_at=datetime.now(timezone.utc), + ) + ) + db_session.commit() + + resp = client.get(f"/api/v1/nudges/users/{owner.id}", headers=owner_headers) + assert resp.status_code < 300 + + +class TestNudgesEngageAuthz: + """POST /api/v1/nudges/{nudge_id}/engage""" + + def _make_nudge(self, db_session, user): + nudge = TestNudge( + id=make_id(), + user_id=user.id, + type="login", + channel="in_app", + message="hi", + sent_at=datetime.now(timezone.utc), + ) + db_session.add(nudge) + db_session.commit() + return nudge + + def _body(self): + return {"engagement_type": "opened"} + + def test_no_auth_returns_401(self, client, db_session): + owner, owner_headers, attacker, attacker_headers = make_authed_pair(db_session) + nudge = self._make_nudge(db_session, owner) + resp = client.post(f"/api/v1/nudges/{nudge.id}/engage", json=self._body()) + assert resp.status_code == 401 + + def test_attacker_returns_403(self, client, db_session): + owner, owner_headers, attacker, attacker_headers = make_authed_pair(db_session) + nudge = self._make_nudge(db_session, owner) + resp = client.post( + f"/api/v1/nudges/{nudge.id}/engage", + json=self._body(), + headers=attacker_headers, + ) + assert resp.status_code == 403 + + def test_owner_returns_2xx(self, client, db_session): + owner, owner_headers, attacker, attacker_headers = make_authed_pair(db_session) + nudge = self._make_nudge(db_session, owner) + resp = client.post( + f"/api/v1/nudges/{nudge.id}/engage", + json=self._body(), + headers=owner_headers, + ) + assert resp.status_code < 300 + + +class TestJobsGetAuthz: + """GET /api/v1/jobs/{job_id}""" + + def _make_job(self, db_session, student, status="pending"): + job = TestJob( + id=make_id(), + job_type="practice_generation", + status=status, + student_id=student.id, + parameters={}, + ) + db_session.add(job) + db_session.commit() + return job + + def test_no_auth_returns_401(self, client, db_session): + owner, owner_headers, attacker, attacker_headers = make_authed_pair(db_session) + job = self._make_job(db_session, owner) + resp = client.get(f"/api/v1/jobs/{job.id}") + assert resp.status_code == 401 + + def test_attacker_returns_403(self, client, db_session): + owner, owner_headers, attacker, attacker_headers = make_authed_pair(db_session) + job = self._make_job(db_session, owner) + resp = client.get(f"/api/v1/jobs/{job.id}", headers=attacker_headers) + assert resp.status_code == 403 + + def test_owner_returns_2xx(self, client, db_session): + owner, owner_headers, attacker, attacker_headers = make_authed_pair(db_session) + job = self._make_job(db_session, owner) + resp = client.get(f"/api/v1/jobs/{job.id}", headers=owner_headers) + assert resp.status_code < 300 + + +class TestJobsWebsocketAuthz: + """WebSocket /api/v1/jobs/{job_id}/ws + + The handler's DB access goes through src.config.database.SessionLocal() + directly rather than the get_db dependency, so it bypasses the client + fixture's SQLite dependency-override by default. The no-token test never + reaches the DB (no token -> no job lookup). The attacker and owner tests + DO reach the job lookup (a valid token gets past decode), so they + monkeypatch SessionLocal to the in-memory test engine; otherwise the + handler hits the real configured DB (an empty Postgres in CI) and the + lookup errors. + """ + + def _make_job(self, db_session, student, status="pending"): + job = TestJob( + id=make_id(), + job_type="practice_generation", + status=status, + student_id=student.id, + parameters={}, + ) + db_session.add(job) + db_session.commit() + return job + + def test_no_token_connection_is_closed(self, client, db_session): + owner, owner_headers, attacker, attacker_headers = make_authed_pair(db_session) + job = self._make_job(db_session, owner) + + with pytest.raises(WebSocketDisconnect) as exc_info: + with client.websocket_connect(f"/api/v1/jobs/{job.id}/ws") as ws: + ws.receive_json() + + assert exc_info.value.code == 1008 + + def test_attacker_token_connection_is_closed(self, client, db_session, monkeypatch): + from tests.conftest import TestingSessionLocal + + monkeypatch.setattr("src.api.handlers.jobs.SessionLocal", TestingSessionLocal) + + owner, owner_headers, attacker, attacker_headers = make_authed_pair(db_session) + job = self._make_job(db_session, owner) + attacker_token = token_for(attacker) + + with pytest.raises(WebSocketDisconnect) as exc_info: + with client.websocket_connect( + f"/api/v1/jobs/{job.id}/ws?token={attacker_token}" + ) as ws: + ws.receive_json() + + assert exc_info.value.code == 1008 + + def test_owner_token_connection_is_accepted(self, client, db_session, monkeypatch): + """The owner-accept path bypasses the TestClient's SQLite dependency + override because the handler opens src.config.database.SessionLocal() + directly instead of using the get_db dependency. Point that at the + same in-memory SQLite engine the client/db_session fixtures use (via + tests.conftest.TestingSessionLocal, sharing the fixtures' StaticPool + connection) so the handler's auth check and initial-state query hit + the test data instead of a real DATABASE_URL. + """ + from tests.conftest import TestingSessionLocal + + monkeypatch.setattr("src.api.handlers.jobs.SessionLocal", TestingSessionLocal) + + owner, owner_headers, attacker, attacker_headers = make_authed_pair(db_session) + job = self._make_job(db_session, owner, status="completed") + db_session.query(TestJob).filter(TestJob.id == job.id).update( + {"result": {"ok": True}} + ) + db_session.commit() + owner_token = token_for(owner) + + with client.websocket_connect( + f"/api/v1/jobs/{job.id}/ws?token={owner_token}" + ) as ws: + status_msg = ws.receive_json() + assert status_msg["type"] == "status" + + completed_msg = ws.receive_json() + assert completed_msg["type"] == "completed" + assert completed_msg["result"] == {"ok": True} diff --git a/tests/test_authz_reads.py b/tests/test_authz_reads.py new file mode 100644 index 0000000..94ed35b --- /dev/null +++ b/tests/test_authz_reads.py @@ -0,0 +1,293 @@ +""" +HTTP-level access-control regression tests for Phase 3 handler fixes +(security remediation, #60). + +Covers: + - GET /summaries/{user_id} + - GET /enhancements/qa/conversation-context/{student_id} + - GET /messaging/threads?user_id= + - GET /dashboards/parent/student/{student_id} and /dashboards/parent/students + - GET /analytics/advanced/engagement/{user_id} +""" + +import pytest + +from tests._authz_utils import auth_headers, create_user, make_authed_pair, token_for +from tests.test_models import TestTutorStudentAssignment + + +@pytest.fixture(autouse=True) +def _jwt_secret(monkeypatch): + monkeypatch.setattr("src.config.settings.settings.jwt_secret", "test-secret") + + +def _assign(db_session, tutor, student): + assignment = TestTutorStudentAssignment( + tutor_id=tutor.id, + student_id=student.id, + subject_id=None, + status="active", + ) + db_session.add(assignment) + db_session.commit() + return assignment + + +class TestSummariesAuthz: + def test_attacker_returns_403(self, client, db_session): + owner, _, _, attacker_headers = make_authed_pair(db_session, role="student") + + resp = client.get(f"/api/v1/summaries/{owner.id}", headers=attacker_headers) + + assert resp.status_code == 403 + + def test_owner_returns_200(self, client, db_session): + owner, owner_headers, _, _ = make_authed_pair(db_session, role="student") + + resp = client.get(f"/api/v1/summaries/{owner.id}", headers=owner_headers) + + assert resp.status_code == 200 + + def test_tutor_with_assignment_returns_200(self, client, db_session): + student = create_user(db_session, "sum-student@example.com", role="student") + tutor = create_user(db_session, "sum-tutor@example.com", role="tutor") + _assign(db_session, tutor, student) + tutor_headers = auth_headers(token_for(tutor)) + + resp = client.get(f"/api/v1/summaries/{student.id}", headers=tutor_headers) + + assert resp.status_code == 200 + + +class TestConversationHistoryAuthz: + def test_attacker_returns_403(self, client, db_session): + owner, _, _, attacker_headers = make_authed_pair(db_session, role="student") + + resp = client.get( + f"/api/v1/enhancements/qa/conversation-history/{owner.id}", + headers=attacker_headers, + ) + + assert resp.status_code == 403 + + def test_owner_returns_200(self, client, db_session): + owner, owner_headers, _, _ = make_authed_pair(db_session, role="student") + + resp = client.get( + f"/api/v1/enhancements/qa/conversation-history/{owner.id}", + headers=owner_headers, + ) + + assert resp.status_code == 200 + + def test_tutor_without_assignment_returns_403(self, client, db_session): + student = create_user(db_session, "hist-student-a@example.com", role="student") + tutor = create_user(db_session, "hist-tutor-a@example.com", role="tutor") + tutor_headers = auth_headers(token_for(tutor)) + + resp = client.get( + f"/api/v1/enhancements/qa/conversation-history/{student.id}", + headers=tutor_headers, + ) + + assert resp.status_code == 403 + + def test_tutor_with_assignment_returns_200(self, client, db_session): + student = create_user(db_session, "hist-student-b@example.com", role="student") + tutor = create_user(db_session, "hist-tutor-b@example.com", role="tutor") + _assign(db_session, tutor, student) + tutor_headers = auth_headers(token_for(tutor)) + + resp = client.get( + f"/api/v1/enhancements/qa/conversation-history/{student.id}", + headers=tutor_headers, + ) + + assert resp.status_code == 200 + + def test_admin_returns_200(self, client, db_session): + student = create_user(db_session, "hist-student-c@example.com", role="student") + admin = create_user(db_session, "hist-admin-a@example.com", role="admin") + admin_headers = auth_headers(token_for(admin)) + + resp = client.get( + f"/api/v1/enhancements/qa/conversation-history/{student.id}", + headers=admin_headers, + ) + + assert resp.status_code == 200 + + +class TestConversationContextAuthz: + def test_attacker_returns_403(self, client, db_session): + owner, _, _, attacker_headers = make_authed_pair(db_session, role="student") + + resp = client.get( + f"/api/v1/enhancements/qa/conversation-context/{owner.id}", + params={"current_query": "hello"}, + headers=attacker_headers, + ) + + assert resp.status_code == 403 + + def test_owner_returns_200(self, client, db_session): + owner, owner_headers, _, _ = make_authed_pair(db_session, role="student") + + resp = client.get( + f"/api/v1/enhancements/qa/conversation-context/{owner.id}", + params={"current_query": "hello"}, + headers=owner_headers, + ) + + assert resp.status_code == 200 + + def test_tutor_with_assignment_returns_200(self, client, db_session): + student = create_user(db_session, "ctx-student@example.com", role="student") + tutor = create_user(db_session, "ctx-tutor@example.com", role="tutor") + _assign(db_session, tutor, student) + tutor_headers = auth_headers(token_for(tutor)) + + resp = client.get( + f"/api/v1/enhancements/qa/conversation-context/{student.id}", + params={"current_query": "hello"}, + headers=tutor_headers, + ) + + assert resp.status_code == 200 + + +class TestMessagingThreadsAuthz: + def test_attacker_supplied_user_id_returns_403(self, client, db_session): + owner, _, _, attacker_headers = make_authed_pair(db_session, role="student") + + resp = client.get( + "/api/v1/messaging/threads", + params={"user_id": owner.id}, + headers=attacker_headers, + ) + + assert resp.status_code == 403 + + def test_own_user_id_returns_200(self, client, db_session): + owner, owner_headers, _, _ = make_authed_pair(db_session, role="student") + + resp = client.get( + "/api/v1/messaging/threads", + params={"user_id": owner.id}, + headers=owner_headers, + ) + + assert resp.status_code == 200 + + def test_omitted_user_id_defaults_to_authenticated_user(self, client, db_session): + owner, owner_headers, _, _ = make_authed_pair(db_session, role="student") + + resp = client.get("/api/v1/messaging/threads", headers=owner_headers) + + assert resp.status_code == 200 + + def test_tutor_with_assignment_returns_200(self, client, db_session): + student = create_user(db_session, "msg-student@example.com", role="student") + tutor = create_user(db_session, "msg-tutor@example.com", role="tutor") + _assign(db_session, tutor, student) + tutor_headers = auth_headers(token_for(tutor)) + + resp = client.get( + "/api/v1/messaging/threads", + params={"user_id": student.id}, + headers=tutor_headers, + ) + + assert resp.status_code == 200 + + +class TestDashboardsParentLockedToAdmin: + # As of #68, parents may access dashboards.py's parent routes for + # students they are linked to via ParentStudentAssignment (admin/seed + # provisioned). An UNLINKED parent is still denied - not because the + # route is admin-only anymore, but because assert_can_access_student + # finds no relationship. See tests/test_authz_parent.py for the + # linked-parent coverage. + + def test_unlinked_parent_role_returns_403_for_student_dashboard( + self, client, db_session + ): + parent = create_user(db_session, "parent-a@example.com", role="parent") + student = create_user(db_session, "dash-student-a@example.com", role="student") + parent_headers = auth_headers(token_for(parent)) + + resp = client.get( + f"/api/v1/dashboards/parent/student/{student.id}", headers=parent_headers + ) + + assert resp.status_code == 403 + + # No admin -> 200 case for GET /parent/student/{student_id} here: the + # handler layers the real src.models.user.User model (which needs a + # hex-no-dash id to match under SQLite's UUID(as_uuid=True) shim) with + # AnalyticsAggregator's student lookup (which needs the exact dashed + # str(UUID) the route produces) against the same TestUser row - no id + # format satisfies both, so it 404s/500s under this harness regardless + # of authz. This is a pre-existing test-harness limitation, not an + # access-control bug; require_role(["parent", "admin"]) is exercised + # identically by test_admin_returns_200_for_students_list below, since + # both parent routes share the exact same role dependency. + + def test_unlinked_parent_sees_empty_list_for_students_list( + self, client, db_session + ): + parent = create_user(db_session, "parent-b@example.com", role="parent") + create_user(db_session, "dash-student-b@example.com", role="student") + parent_headers = auth_headers(token_for(parent)) + + resp = client.get("/api/v1/dashboards/parent/students", headers=parent_headers) + + assert resp.status_code == 200 + assert resp.json()["data"]["students"] == [] + + def test_admin_returns_200_for_students_list(self, client, db_session): + admin = create_user(db_session, "admin-b@example.com", role="admin") + admin_headers = auth_headers(token_for(admin)) + + resp = client.get("/api/v1/dashboards/parent/students", headers=admin_headers) + + assert resp.status_code == 200 + + +class TestEngagementAuthz: + def test_tutor_without_assignment_returns_403(self, client, db_session): + tutor = create_user(db_session, "eng-tutor-a@example.com", role="tutor") + student = create_user(db_session, "eng-student-a@example.com", role="student") + tutor_headers = auth_headers(token_for(tutor)) + + resp = client.get( + f"/api/v1/analytics/advanced/engagement/{student.id}", + headers=tutor_headers, + ) + + assert resp.status_code == 403 + + def test_tutor_with_assignment_returns_200(self, client, db_session): + tutor = create_user(db_session, "eng-tutor-b@example.com", role="tutor") + student = create_user(db_session, "eng-student-b@example.com", role="student") + _assign(db_session, tutor, student) + tutor_headers = auth_headers(token_for(tutor)) + + resp = client.get( + f"/api/v1/analytics/advanced/engagement/{student.id}", + headers=tutor_headers, + ) + + assert resp.status_code == 200 + + def test_admin_returns_200(self, client, db_session): + admin = create_user(db_session, "eng-admin-a@example.com", role="admin") + student = create_user(db_session, "eng-student-c@example.com", role="student") + admin_headers = auth_headers(token_for(admin)) + + resp = client.get( + f"/api/v1/analytics/advanced/engagement/{student.id}", + headers=admin_headers, + ) + + assert resp.status_code == 200 diff --git a/tests/test_models.py b/tests/test_models.py index ff55bf2..2486f6b 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -13,6 +13,7 @@ ForeignKey, Integer, Numeric, + PrimaryKeyConstraint, String, Text, ) @@ -253,6 +254,69 @@ class TestGoal(TestBase, TimestampMixin): subject = relationship("TestSubject", backref="goals") +class TestTutorStudentAssignment(TestBase): + """Test version of TutorStudentAssignment model""" + + __tablename__ = "tutor_student_assignments" + + tutor_id = Column( + String(36), ForeignKey("users.id", ondelete="CASCADE"), nullable=False + ) + student_id = Column( + String(36), ForeignKey("users.id", ondelete="CASCADE"), nullable=False + ) + subject_id = Column(String(36), ForeignKey("subjects.id"), nullable=True) + assigned_at = Column( + DateTime(timezone=True), server_default=func.now(), nullable=False + ) + status = Column(String(20), default="active") + notes = Column(Text) + start_date = Column(DateTime, nullable=True) + end_date = Column(DateTime, nullable=True) + + __table_args__ = (PrimaryKeyConstraint("tutor_id", "student_id", "subject_id"),) + + tutor = relationship( + "TestUser", foreign_keys=[tutor_id], backref="tutor_assignments" + ) + student = relationship( + "TestUser", foreign_keys=[student_id], backref="student_assignments" + ) + subject = relationship("TestSubject", backref="tutor_student_assignments") + + +class TestParentStudentAssignment(TestBase): + """Test version of ParentStudentAssignment model""" + + __tablename__ = "parent_student_assignments" + + parent_id = Column( + String(36), ForeignKey("users.id", ondelete="CASCADE"), nullable=False + ) + student_id = Column( + String(36), ForeignKey("users.id", ondelete="CASCADE"), nullable=False + ) + status = Column(String(20), default="active") + created_at = Column( + DateTime(timezone=True), server_default=func.now(), nullable=False + ) + updated_at = Column( + DateTime(timezone=True), + server_default=func.now(), + onupdate=func.now(), + nullable=False, + ) + + __table_args__ = (PrimaryKeyConstraint("parent_id", "student_id"),) + + parent = relationship( + "TestUser", foreign_keys=[parent_id], backref="parent_assignments" + ) + student = relationship( + "TestUser", foreign_keys=[student_id], backref="parent_student_assignments" + ) + + class TestStudentRating(TestBase): """Test version of StudentRating model"""