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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01365DYKu14YtsBKv2ycW6eG
This commit is contained in:
parent
0b58b903d2
commit
2958779067
4 changed files with 160 additions and 22 deletions
43
backend/alembic/versions/c1d2e3f4a5b6_grant_dimensions.py
Normal file
43
backend/alembic/versions/c1d2e3f4a5b6_grant_dimensions.py
Normal file
|
|
@ -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")
|
||||||
|
|
@ -1,22 +1,27 @@
|
||||||
from datetime import datetime
|
from datetime import datetime
|
||||||
|
|
||||||
from sqlalchemy import Column, DateTime, ForeignKey, Integer, UniqueConstraint
|
from sqlalchemy import Column, DateTime, ForeignKey, Integer
|
||||||
|
|
||||||
from app.database import Base
|
from app.database import Base
|
||||||
|
|
||||||
|
|
||||||
class CategoryGrant(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
|
A grant names any combination of exam, discipline tag and category, and
|
||||||
and delete questions filed under the granted category, and nothing else.
|
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"
|
__tablename__ = "category_grants"
|
||||||
__table_args__ = (UniqueConstraint("category_id", "user_id", name="uq_category_grant"),)
|
|
||||||
|
|
||||||
id = Column(Integer, primary_key=True, index=True)
|
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)
|
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)
|
granted_by = Column(Integer, ForeignKey("users.id", ondelete="SET NULL"), nullable=True)
|
||||||
created_at = Column(DateTime, default=datetime.utcnow)
|
created_at = Column(DateTime, default=datetime.utcnow)
|
||||||
|
|
|
||||||
|
|
@ -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,
|
from app.services.quiz_builder import (bank_query, category_descendants, filtered_bank_query,
|
||||||
CreateFromBankRequest, GenerateTestRequest, create_saved_test, exam_scope_predicate, generate_test)
|
CreateFromBankRequest, GenerateTestRequest, create_saved_test, exam_scope_predicate, generate_test)
|
||||||
from app.utils.auth import get_current_user, require_moderator
|
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)
|
is_question_manager, manageable_categories, question_in_scope, require_question_manager)
|
||||||
|
|
||||||
router = APIRouter()
|
router = APIRouter()
|
||||||
|
|
@ -100,7 +100,7 @@ def edit_question(
|
||||||
question = db.query(Question).filter(Question.id == question_id).first()
|
question = db.query(Question).filter(Question.id == question_id).first()
|
||||||
if not question:
|
if not question:
|
||||||
raise HTTPException(status_code=404, detail="Question not found")
|
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)
|
_snapshot_question(db, question, current_user.id)
|
||||||
if "question_category_id" in data.model_fields_set:
|
if "question_category_id" in data.model_fields_set:
|
||||||
# Moving a question out of your scope would put it beyond your reach.
|
# 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")
|
raise HTTPException(400, "No questions selected")
|
||||||
if len(ids) > 500:
|
if len(ids) > 500:
|
||||||
raise HTTPException(400, "Select at most 500 questions per action")
|
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":
|
if data.action == "category":
|
||||||
assert_can_manage_category(scope, data.category_id)
|
assert_can_manage_category(scope, data.category_id)
|
||||||
|
|
||||||
|
|
@ -742,7 +742,7 @@ def list_question_versions(
|
||||||
from app.models.question import QuestionVersion
|
from app.models.question import QuestionVersion
|
||||||
|
|
||||||
scope = require_question_manager(db, current_user)
|
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(
|
rows = db.query(QuestionVersion).filter(
|
||||||
QuestionVersion.question_id == question_id
|
QuestionVersion.question_id == question_id
|
||||||
).order_by(QuestionVersion.created_at.desc(), QuestionVersion.id.desc()).limit(MAX_VERSIONS).all()
|
).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
|
from app.models.question import QuestionVersion
|
||||||
|
|
||||||
scope = require_question_manager(db, current_user)
|
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()
|
question = db.query(Question).filter(Question.id == question_id).first()
|
||||||
if not question:
|
if not question:
|
||||||
raise HTTPException(404, "Question not found")
|
raise HTTPException(404, "Question not found")
|
||||||
|
|
|
||||||
|
|
@ -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
|
A moderator or admin manages every question. Everyone else manages only what
|
||||||
questions filed under a category they hold a grant on, or under any descendant
|
their grants cover. A grant names any combination of exam, discipline tag and
|
||||||
of it. `manageable_categories` returns None to mean "no restriction".
|
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 fastapi import HTTPException
|
||||||
from sqlalchemy.orm import Session
|
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.question_category import QuestionCategory, QuestionCategoryLink
|
||||||
from app.models.user import User
|
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]:
|
def _descendants(db: Session, roots: set[int]) -> set[int]:
|
||||||
"""Expand category ids to include every category beneath them."""
|
"""Expand category ids to include every category beneath them."""
|
||||||
|
|
@ -30,6 +37,69 @@ def _descendants(db: Session, roots: set[int]) -> set[int]:
|
||||||
return seen
|
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:
|
def manageable_categories(db: Session, user: User) -> set[int] | None:
|
||||||
"""Category ids this user may edit questions in; None means all of them."""
|
"""Category ids this user may edit questions in; None means all of them."""
|
||||||
if user.is_moderator:
|
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:
|
def is_question_manager(db: Session, user: User) -> bool:
|
||||||
"""True when the user may edit questions somewhere."""
|
"""True when the user may edit questions somewhere."""
|
||||||
scope = manageable_categories(db, user)
|
if user.is_moderator:
|
||||||
return scope is None or bool(scope)
|
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:
|
def require_question_manager(db: Session, user: User) -> set[int] | None:
|
||||||
"""Gate a question-management route; returns the caller's category scope."""
|
"""Gate a question-management route; returns the caller's category scope."""
|
||||||
scope = manageable_categories(db, user)
|
if not is_question_manager(db, user):
|
||||||
if scope is not None and not scope:
|
raise HTTPException(403, "Question management requires moderator access or a grant")
|
||||||
raise HTTPException(403, "Question management requires moderator access or a category grant")
|
return manageable_categories(db, user)
|
||||||
return scope
|
|
||||||
|
|
||||||
|
|
||||||
def assert_can_manage_category(scope: set[int] | None, category_id: int | None, what: str = "category") -> None:
|
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}")
|
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:
|
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)."""
|
"""Every target question must sit in a granted category (primary or additional)."""
|
||||||
if scope is None or not question_ids:
|
if scope is None or not question_ids:
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue