diff --git a/backend/app/routers/contact.py b/backend/app/routers/contact.py index c48a6e3..55a96e8 100644 --- a/backend/app/routers/contact.py +++ b/backend/app/routers/contact.py @@ -10,7 +10,9 @@ from sqlalchemy.orm import Session from app.database import get_db from app.config import settings +from app.models.user import User from app.services import captcha +from app.utils.auth import require_admin router = APIRouter() log = logging.getLogger(__name__) @@ -75,9 +77,15 @@ async def _email_admin(name: str, email: str, type_: str, message: str): @router.get("/submissions") def list_submissions( db: Session = Depends(get_db), + current_user: User = Depends(require_admin), ): - """Admin: list all contact submissions.""" - from app.utils.auth import require_admin + """Every message the contact form has taken. Administrators only. + + It was open to the internet: `require_admin` was imported inside the + function body and never used as a dependency, so the import read as + protection and was none. Anyone who guessed the path could read every + sender's name, address and message. + """ rows = db.execute(text( "SELECT id, name, email, type, message, read, created_at FROM contact_submissions ORDER BY created_at DESC" )).fetchall() @@ -85,7 +93,9 @@ def list_submissions( { "id": r.id, "name": r.name, "email": r.email, "type": r.type, "message": r.message, "read": r.read, - "created_at": r.created_at.isoformat(), + # Some early rows have no timestamp, and a listing that raises on + # one of them shows none of the others. + "created_at": r.created_at.isoformat() if r.created_at else None, } for r in rows ] @@ -95,7 +105,9 @@ def list_submissions( def mark_read( submission_id: int, db: Session = Depends(get_db), + current_user: User = Depends(require_admin), ): + """Mark one as read. Administrators only, for the same reason as the list.""" result = db.execute(text("UPDATE contact_submissions SET read=1 WHERE id=:id"), {"id": submission_id}) db.commit() if result.rowcount == 0: diff --git a/backend/tests/test_contact_privacy.py b/backend/tests/test_contact_privacy.py new file mode 100644 index 0000000..4224470 --- /dev/null +++ b/backend/tests/test_contact_privacy.py @@ -0,0 +1,78 @@ +"""Who may read the contact form's messages. + +Run: DATABASE_URL=sqlite:// PYTHONPATH=backend python -m unittest discover -s backend/tests +""" +import os +os.environ.setdefault("DATABASE_URL", "sqlite://") + +import unittest + +from fastapi import FastAPI +from fastapi.testclient import TestClient +from sqlalchemy import create_engine, text +from sqlalchemy.orm import Session +from sqlalchemy.pool import StaticPool + +from app.database import get_db +from app.models.user import User +from app.routers import contact +from app.utils.auth import get_current_user + + +class ContactPrivacyTests(unittest.TestCase): + """The listing was open to the internet. + + `require_admin` was imported inside the function body and never used as a + dependency, so the import read as protection and was none: anyone who + guessed the path could read every sender's name, address and message. + """ + + def setUp(self): + self.engine = create_engine("sqlite://", connect_args={"check_same_thread": False}, + poolclass=StaticPool) + # Only the table this router touches, plus users. `create_all` pulls in + # every model in the metadata, and one of them carries a raw-SQL table + # SQLite cannot build. + User.__table__.create(self.engine, checkfirst=True) + self.db = Session(self.engine) + self.db.execute(text( + "CREATE TABLE IF NOT EXISTS contact_submissions (" + "id INTEGER PRIMARY KEY, name TEXT, email TEXT, type TEXT, message TEXT," + " read INTEGER DEFAULT 0, created_at TIMESTAMP)")) + self.db.execute(text( + "INSERT INTO contact_submissions (id, name, email, type, message, read, created_at)" + " VALUES (1, 'A Sender', 'sender@example.test', 'question', 'Private message', 0, NULL)")) + self.db.commit() + + self.admin = User(id=1, email="admin@example.test", name="Admin", role="admin") + self.learner = User(id=2, email="learner@example.test", name="Learner", role="user") + self.db.add_all([self.admin, self.learner]) + self.db.commit() + + self.user = self.learner + app = FastAPI() + app.include_router(contact.router, prefix="/contact") + app.dependency_overrides[get_db] = lambda: self.db + app.dependency_overrides[get_current_user] = lambda: self.user + self.client = TestClient(app) + + def tearDown(self): + self.db.close() + self.engine.dispose() + + def test_a_learner_may_not_read_or_touch_the_messages(self): + self.assertEqual(self.client.get("/contact/submissions").status_code, 403) + self.assertEqual(self.client.put("/contact/submissions/1/read").status_code, 403) + + def test_an_administrator_may(self): + self.user = self.admin + listing = self.client.get("/contact/submissions") + self.assertEqual(listing.status_code, 200, listing.text) + self.assertEqual(listing.json()[0]["email"], "sender@example.test") + # A row with no timestamp does not take the whole listing down with it. + self.assertIsNone(listing.json()[0]["created_at"]) + self.assertEqual(self.client.put("/contact/submissions/1/read").status_code, 200) + + +if __name__ == "__main__": + unittest.main() diff --git a/backend/tests/test_related_privacy.py b/backend/tests/test_related_privacy.py index 3ba5bf2..e77c89b 100644 --- a/backend/tests/test_related_privacy.py +++ b/backend/tests/test_related_privacy.py @@ -97,6 +97,14 @@ class PrivacyTests(unittest.TestCase): self.assertEqual(self.chat(4).status_code, 200) def test_tutor_course_attempt_modes_pool_and_review(self): + # The tutor's site switch lives in Redis, which these tests share with + # the running site — so an administrator turning the tutor off in the + # interface used to turn this test red. What is under test here is who + # may reach the tutor when it is *on*, so it says so. + from app.services import site_settings + site_settings.set_flag("tutor_in_quiz", True) + self.addCleanup(site_settings.set_flag, "tutor_in_quiz", False) + self.enroll() self.assertEqual(self.chat(5).status_code, 403) for mode in ('exam', None): diff --git a/frontend/src/pages/QuizPage.jsx b/frontend/src/pages/QuizPage.jsx index ddc9e8f..6f8ad92 100644 --- a/frontend/src/pages/QuizPage.jsx +++ b/frontend/src/pages/QuizPage.jsx @@ -570,7 +570,10 @@ export default function QuizPage() { // Suspending is not abandoning: it ends on the session's own analysis, where // what has been answered so far is scored and the Resume button sits. A // course quiz still returns to the course it belongs to. - const [tutorAllowed, setTutorAllowed] = useState(true) + //: Withheld until the server says otherwise. Defaulting to allowed made the + //: tutor's button appear for a moment on every session and then vanish on a + //: site that has it switched off, which reads as a bug rather than a policy. + const [tutorAllowed, setTutorAllowed] = useState(false) useEffect(() => { let live = true