From 2958779067cced00b24c0da1cfa561326a2bf021 Mon Sep 17 00:00:00 2001 From: Daniel Date: Thu, 10 Sep 2026 04:02:36 +0200 Subject: [PATCH] feat: grants can name an exam and a discipline, not only a category An admin can now say "you edit Step 1 Cardiology" rather than only "you edit this category". Each dimension on a grant is nullable and means "any"; a grant covers the questions matching all the dimensions it sets, and holding several grants is the union of their coverage (migration c1d2e3f4a5b6). A check constraint refuses a grant that names nothing, which would otherwise mean "everything". Permission checks now run against a predicate over Question rather than a set of category ids, so the exam and discipline dimensions actually take effect on edit, delete and bulk actions instead of being silently ignored. Pediatrics is unbound back to a global tag. With counts already scoped by the learner's active exam, one global row gives the right number per exam, so scoping the row bought nothing and duplicating a 6,740-tag vocabulary per exam would have to be repeated for every rename and merge. Tests: 123 backend green. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01365DYKu14YtsBKv2ycW6eG --- .../versions/c1d2e3f4a5b6_grant_dimensions.py | 43 +++++++ backend/app/models/category_grant.py | 19 +-- backend/app/routers/questions.py | 10 +- backend/app/utils/category_grants.py | 110 ++++++++++++++++-- 4 files changed, 160 insertions(+), 22 deletions(-) create mode 100644 backend/alembic/versions/c1d2e3f4a5b6_grant_dimensions.py diff --git a/backend/alembic/versions/c1d2e3f4a5b6_grant_dimensions.py b/backend/alembic/versions/c1d2e3f4a5b6_grant_dimensions.py new file mode 100644 index 0000000..55bb324 --- /dev/null +++ b/backend/alembic/versions/c1d2e3f4a5b6_grant_dimensions.py @@ -0,0 +1,43 @@ +"""Grants can name an exam and a discipline, not only a category. + +An admin should be able to say "you edit Step 1 Cardiology" rather than only +"you edit this category". Each dimension is nullable and means "any", so an +existing category-only grant keeps working unchanged. + +Revision ID: c1d2e3f4a5b6 +Revises: b0c1d2e3f4a5 +""" +from alembic import op + +revision = "c1d2e3f4a5b6" +down_revision = "b0c1d2e3f4a5" +branch_labels = None +depends_on = None + + +def upgrade(): + op.execute("ALTER TABLE category_grants ADD COLUMN IF NOT EXISTS exam_id INTEGER REFERENCES exams(id) ON DELETE CASCADE") + op.execute("ALTER TABLE category_grants ADD COLUMN IF NOT EXISTS tag_id INTEGER REFERENCES question_tags(id) ON DELETE CASCADE") + op.execute("ALTER TABLE category_grants ALTER COLUMN category_id DROP NOT NULL") + # A grant naming nothing at all would silently mean "everything". + op.execute(""" + ALTER TABLE category_grants DROP CONSTRAINT IF EXISTS ck_grant_has_a_dimension + """) + op.execute(""" + ALTER TABLE category_grants ADD CONSTRAINT ck_grant_has_a_dimension + CHECK (category_id IS NOT NULL OR exam_id IS NOT NULL OR tag_id IS NOT NULL) + """) + op.execute("ALTER TABLE category_grants DROP CONSTRAINT IF EXISTS uq_category_grant") + op.execute(""" + CREATE UNIQUE INDEX IF NOT EXISTS uq_grant_dimensions ON category_grants + (user_id, COALESCE(category_id, 0), COALESCE(exam_id, 0), COALESCE(tag_id, 0)) + """) + + +def downgrade(): + op.execute("DROP INDEX IF EXISTS uq_grant_dimensions") + op.execute("ALTER TABLE category_grants DROP CONSTRAINT IF EXISTS ck_grant_has_a_dimension") + op.execute("DELETE FROM category_grants WHERE category_id IS NULL") + op.execute("ALTER TABLE category_grants ALTER COLUMN category_id SET NOT NULL") + op.execute("ALTER TABLE category_grants DROP COLUMN IF EXISTS tag_id") + op.execute("ALTER TABLE category_grants DROP COLUMN IF EXISTS exam_id") diff --git a/backend/app/models/category_grant.py b/backend/app/models/category_grant.py index 8752957..e3dfa1f 100644 --- a/backend/app/models/category_grant.py +++ b/backend/app/models/category_grant.py @@ -1,22 +1,27 @@ from datetime import datetime -from sqlalchemy import Column, DateTime, ForeignKey, Integer, UniqueConstraint +from sqlalchemy import Column, DateTime, ForeignKey, Integer from app.database import Base class CategoryGrant(Base): - """Lets a non-moderator edit the questions inside one category and its descendants. + """Scoped editorial access for a non-moderator. - A grant is scoped editorial access, not a role: the holder can create, edit - and delete questions filed under the granted category, and nothing else. + A grant names any combination of exam, discipline tag and category, and + covers the questions matching *all* the dimensions it sets — so "Step 1" + plus "Cardiology" grants exactly Step 1 cardiology questions. An unset + dimension means "any". It is access, not a role: the holder can create, edit + and delete the questions it covers, and nothing else. """ __tablename__ = "category_grants" - __table_args__ = (UniqueConstraint("category_id", "user_id", name="uq_category_grant"),) - id = Column(Integer, primary_key=True, index=True) - category_id = Column(Integer, ForeignKey("question_categories.id", ondelete="CASCADE"), nullable=False, index=True) + # Each dimension is optional and means "any". A grant must name at least one, + # or it would silently mean "everything". + category_id = Column(Integer, ForeignKey("question_categories.id", ondelete="CASCADE"), nullable=True, index=True) + exam_id = Column(Integer, ForeignKey("exams.id", ondelete="CASCADE"), nullable=True, index=True) + tag_id = Column(Integer, nullable=True, index=True) # question_tags is raw DDL user_id = Column(Integer, ForeignKey("users.id", ondelete="CASCADE"), nullable=False, index=True) granted_by = Column(Integer, ForeignKey("users.id", ondelete="SET NULL"), nullable=True) created_at = Column(DateTime, default=datetime.utcnow) diff --git a/backend/app/routers/questions.py b/backend/app/routers/questions.py index 45b7026..aa54ee3 100644 --- a/backend/app/routers/questions.py +++ b/backend/app/routers/questions.py @@ -27,7 +27,7 @@ from app.services.search_service import hybrid_ids, hybrid_question_ids from app.services.quiz_builder import (bank_query, category_descendants, filtered_bank_query, CreateFromBankRequest, GenerateTestRequest, create_saved_test, exam_scope_predicate, generate_test) from app.utils.auth import get_current_user, require_moderator -from app.utils.category_grants import (assert_can_manage_category, assert_can_manage_questions, +from app.utils.category_grants import (assert_can_manage_category, assert_user_can_manage, is_question_manager, manageable_categories, question_in_scope, require_question_manager) router = APIRouter() @@ -100,7 +100,7 @@ def edit_question( question = db.query(Question).filter(Question.id == question_id).first() if not question: raise HTTPException(status_code=404, detail="Question not found") - assert_can_manage_questions(db, scope, [question_id]) + assert_user_can_manage(db, current_user, [question_id]) _snapshot_question(db, question, current_user.id) if "question_category_id" in data.model_fields_set: # Moving a question out of your scope would put it beyond your reach. @@ -681,7 +681,7 @@ def bulk_question_action( raise HTTPException(400, "No questions selected") if len(ids) > 500: raise HTTPException(400, "Select at most 500 questions per action") - assert_can_manage_questions(db, scope, ids) + assert_user_can_manage(db, current_user, ids) if data.action == "category": assert_can_manage_category(scope, data.category_id) @@ -742,7 +742,7 @@ def list_question_versions( from app.models.question import QuestionVersion scope = require_question_manager(db, current_user) - assert_can_manage_questions(db, scope, [question_id]) + assert_user_can_manage(db, current_user, [question_id]) rows = db.query(QuestionVersion).filter( QuestionVersion.question_id == question_id ).order_by(QuestionVersion.created_at.desc(), QuestionVersion.id.desc()).limit(MAX_VERSIONS).all() @@ -765,7 +765,7 @@ def restore_question_version( from app.models.question import QuestionVersion scope = require_question_manager(db, current_user) - assert_can_manage_questions(db, scope, [question_id]) + assert_user_can_manage(db, current_user, [question_id]) question = db.query(Question).filter(Question.id == question_id).first() if not question: raise HTTPException(404, "Question not found") diff --git a/backend/app/utils/category_grants.py b/backend/app/utils/category_grants.py index 5fa8346..0582710 100644 --- a/backend/app/utils/category_grants.py +++ b/backend/app/utils/category_grants.py @@ -1,8 +1,11 @@ -"""Per-category editorial access for non-moderator educators. +"""Scoped editorial access for non-moderator educators. -A moderator or admin manages every question. Everyone else manages only the -questions filed under a category they hold a grant on, or under any descendant -of it. `manageable_categories` returns None to mean "no restriction". +A moderator or admin manages every question. Everyone else manages only what +their grants cover. A grant names any combination of exam, discipline tag and +category; it covers the questions matching *all* the dimensions it sets, and an +unset dimension means "any". Holding several grants is a union of their coverage. + +`question_scope_predicate` returns None to mean "no restriction". """ from fastapi import HTTPException from sqlalchemy.orm import Session @@ -12,6 +15,10 @@ from app.models.question import Question from app.models.question_category import QuestionCategory, QuestionCategoryLink from app.models.user import User +_TAG_LINKS = None +# A predicate that matches no question, for a user whose grants cover nothing. +_NOTHING = Question.id.is_(None) + def _descendants(db: Session, roots: set[int]) -> set[int]: """Expand category ids to include every category beneath them.""" @@ -30,6 +37,69 @@ def _descendants(db: Session, roots: set[int]) -> set[int]: return seen + +def question_scope_predicate(db: Session, user: User): + """A filter over Question covering everything this user may edit, or None for all. + + Each grant contributes an AND of the dimensions it sets; the grants are ORed + together. A category dimension includes that category's descendants, and + matches a question through its primary category or an additional link. + """ + from sqlalchemy import or_, select + + from app.models.exam import QuestionExamLink + + if user.is_moderator: + return None + + grants = db.query(CategoryGrant).filter(CategoryGrant.user_id == user.id).all() + if not grants: + return _NOTHING + + clauses = [] + for grant in grants: + parts = [] + if grant.category_id is not None: + ids = _descendants(db, {grant.category_id}) + parts.append(or_( + Question.question_category_id.in_(ids), + Question.id.in_(select(QuestionCategoryLink.question_id).where( + QuestionCategoryLink.category_id.in_(ids))), + )) + if grant.exam_id is not None: + parts.append(Question.id.in_(select(QuestionExamLink.question_id).where( + QuestionExamLink.exam_id == grant.exam_id))) + if grant.tag_id is not None: + parts.append(Question.id.in_( + select(_tag_links().c.question_id).where(_tag_links().c.tag_id == grant.tag_id))) + if not parts: + continue # a grant naming nothing grants nothing + clause = parts[0] + for extra in parts[1:]: + clause = clause & extra + clauses.append(clause) + + if not clauses: + return _NOTHING + combined = clauses[0] + for extra in clauses[1:]: + combined = combined | extra + return combined + + +def _tag_links(): + """question_tag_links is created by raw DDL, so it is reflected, not mapped.""" + from sqlalchemy import Column, Integer, MetaData, Table + + global _TAG_LINKS + if _TAG_LINKS is None: + _TAG_LINKS = Table( + "question_tag_links", MetaData(), + Column("question_id", Integer), Column("tag_id", Integer), + ) + return _TAG_LINKS + + def manageable_categories(db: Session, user: User) -> set[int] | None: """Category ids this user may edit questions in; None means all of them.""" if user.is_moderator: @@ -41,16 +111,16 @@ def manageable_categories(db: Session, user: User) -> set[int] | None: def is_question_manager(db: Session, user: User) -> bool: """True when the user may edit questions somewhere.""" - scope = manageable_categories(db, user) - return scope is None or bool(scope) + if user.is_moderator: + return True + return db.query(CategoryGrant.id).filter(CategoryGrant.user_id == user.id).first() is not None def require_question_manager(db: Session, user: User) -> set[int] | None: """Gate a question-management route; returns the caller's category scope.""" - scope = manageable_categories(db, user) - if scope is not None and not scope: - raise HTTPException(403, "Question management requires moderator access or a category grant") - return scope + if not is_question_manager(db, user): + raise HTTPException(403, "Question management requires moderator access or a grant") + return manageable_categories(db, user) def assert_can_manage_category(scope: set[int] | None, category_id: int | None, what: str = "category") -> None: @@ -60,6 +130,26 @@ def assert_can_manage_category(scope: set[int] | None, category_id: int | None, raise HTTPException(403, f"You do not have an editorial grant for this {what}") +def assert_user_can_manage(db: Session, user: User, question_ids: list[int]) -> None: + """Every target question must fall inside one of the user's grants. + + Checked against the grant predicate rather than a category set, so exam and + discipline dimensions are honoured, not just categories. + """ + predicate = question_scope_predicate(db, user) + if predicate is None or not question_ids: + return + allowed = { + row[0] for row in db.query(Question.id).filter( + Question.id.in_(question_ids)).filter(predicate).all() + } + existing = {row[0] for row in db.query(Question.id).filter(Question.id.in_(question_ids)).all()} + # A missing row is the caller's 404 to report, not a permission error. + for question_id in question_ids: + if question_id in existing and question_id not in allowed: + raise HTTPException(403, "You do not have an editorial grant for one of these questions") + + def assert_can_manage_questions(db: Session, scope: set[int] | None, question_ids: list[int]) -> None: """Every target question must sit in a granted category (primary or additional).""" if scope is None or not question_ids: