Make note/submission edit-own and edit-others' permissions explicit

Split note:edit/note:delete and submission:edit/submission:delete into
four independent permissions each - edit_self/delete_self (acting on
your own note or submission) and edit_other/delete_other (acting on
someone else's). Previously "own" access was an unconditional, unrevokable
ownership check with no permission behind it, and a prior round had
accidentally granted coordinator submission:edit/delete by default
(inconsistent with notes, which were correctly own-only) - both are fixed
here: self-service now goes through a real, default-granted-to-everyone
permission, and acting on someone else's note/submission is an explicit
elevated grant that nobody gets by default.

The Role Management permission editor now shows "Edit Own / Delete Own /
Edit Others' / Delete Others'" as four clear, independently toggleable
options instead of one ambiguous "Edit"/"Delete" checkbox.

migrate_role_permissions.py renames the existing permission rows in place
(rather than leaving orphaned duplicates) and includes a one-time,
idempotent correction that revokes the earlier over-grant from coordinator.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
2026-07-18 21:17:47 +08:00
parent db2c414c1a
commit 960753b3d6
5 changed files with 108 additions and 33 deletions
+66 -13
View File
@@ -45,13 +45,17 @@ PERMISSIONS = [
("review", "publish", "Can approve a submission"), ("review", "publish", "Can approve a submission"),
("review", "retake", "Can request a retake on a submission"), ("review", "retake", "Can request a retake on a submission"),
("submission", "create", "Can submit work for a task"), ("submission", "create", "Can submit work for a task"),
("submission", "edit", "Can edit a submission's notes"), ("submission", "edit_self", "Can edit your own submission notes"),
("submission", "delete", "Can delete a submission"), ("submission", "delete_self", "Can delete your own submission"),
("submission", "edit_other", "Can edit another user's submission notes"),
("submission", "delete_other", "Can delete another user's submission"),
("upload", "create", "Can upload task attachments"), ("upload", "create", "Can upload task attachments"),
("upload", "delete", "Can delete task attachments"), ("upload", "delete", "Can delete task attachments"),
("note", "create", "Can add task notes"), ("note", "create", "Can add task notes"),
("note", "edit", "Can edit task notes"), ("note", "edit_self", "Can edit your own note"),
("note", "delete", "Can delete task notes"), ("note", "delete_self", "Can delete your own note"),
("note", "edit_other", "Can edit another user's note"),
("note", "delete_other", "Can delete another user's note"),
("note", "view_internal", "Can view internal notes"), ("note", "view_internal", "Can view internal notes"),
("note", "view_client", "Can view client notes"), ("note", "view_client", "Can view client notes"),
] ]
@@ -79,30 +83,35 @@ SYSTEM_ROLE_DESCRIPTIONS = {
# allowed DIRECTOR - now both use this one permission, so director loses the # allowed DIRECTOR - now both use this one permission, so director loses the
# bulk-only status-change access they had before (can be re-granted via a # bulk-only status-change access they had before (can be re-granted via a
# custom role if that capability is actually wanted). # custom role if that capability is actually wanted).
# submission:edit/delete are brand new endpoints (didn't exist before this # note/submission edit_self+delete_self are granted to every system role -
# permission was added), so there's no "today's behavior" to preserve - # today, editing/deleting your OWN note or submission is an unconditional
# granted to coordinator as the natural production-lead capability; artists # right with no role check at all, so faithful migration means everyone
# keep editing/deleting their own submissions via the ownership check. # gets it by default. edit_other/delete_other (acting on someone ELSE's
# note or submission) are the elevated capability and are granted to
# nobody by default - not even coordinator.
SELF_SERVICE_GRANTS = {
("note", "edit_self"), ("note", "delete_self"),
("submission", "edit_self"), ("submission", "delete_self"),
}
SYSTEM_ROLE_GRANTS = { SYSTEM_ROLE_GRANTS = {
"coordinator": { "coordinator": SELF_SERVICE_GRANTS | {
("asset", "create"), ("asset", "edit"), ("asset", "delete"), ("asset", "create"), ("asset", "edit"), ("asset", "delete"),
("shot", "create"), ("shot", "edit"), ("shot", "delete"), ("shot", "create"), ("shot", "edit"), ("shot", "delete"),
("task", "create"), ("task", "edit"), ("task", "delete"), ("task", "change_status"), ("task", "create"), ("task", "edit"), ("task", "delete"), ("task", "change_status"),
("assignment", "create"), ("assignment", "edit"), ("assignment", "delete"), ("assignment", "create"), ("assignment", "edit"), ("assignment", "delete"),
("review", "publish"), ("review", "retake"), ("review", "publish"), ("review", "retake"),
("submission", "edit"), ("submission", "delete"),
("upload", "create"), ("upload", "delete"), ("upload", "create"), ("upload", "delete"),
("note", "create"), ("note", "view_internal"), ("note", "view_client"), ("note", "create"), ("note", "view_internal"), ("note", "view_client"),
}, },
"director": { "director": SELF_SERVICE_GRANTS | {
("review", "publish"), ("review", "retake"), ("review", "publish"), ("review", "retake"),
("upload", "create"), ("upload", "create"),
("note", "create"), ("note", "view_internal"), ("note", "view_client"), ("note", "create"), ("note", "view_internal"), ("note", "view_client"),
}, },
"artist": { "artist": SELF_SERVICE_GRANTS | {
("note", "view_internal"), ("note", "view_internal"),
}, },
"developer": { "developer": SELF_SERVICE_GRANTS | {
("upload", "create"), ("upload", "create"),
("note", "create"), ("note", "view_internal"), ("note", "create"), ("note", "view_internal"),
}, },
@@ -131,6 +140,32 @@ def migrate_role_permissions():
)) ))
db.commit() db.commit()
# 0b. One-time rename: note/submission edit and delete were renamed
# to edit_other/delete_other to make explicit that they only govern
# acting on someone else's note or submission (your own is always
# editable/deletable via ownership, no permission needed). Renaming
# the existing rows in place (rather than leaving the old ones
# orphaned and adding new ones) preserves their ids; idempotent -
# a no-op once renamed. Not a general rename mechanism.
RENAMES = [
("note", "edit", "edit_other", "Can edit another user's note"),
("note", "delete", "delete_other", "Can delete another user's note"),
("submission", "edit", "edit_other", "Can edit another user's submission notes"),
("submission", "delete", "delete_other", "Can delete another user's submission"),
]
renamed = 0
for resource, old_action, new_action, new_description in RENAMES:
perm = db.query(Permission).filter(
Permission.resource == resource, Permission.action == old_action
).first()
if perm:
perm.action = new_action
perm.description = new_description
renamed += 1
db.commit()
if renamed:
logger.info(f"Permissions: {renamed} renamed to *_other")
# 1. Seed permissions (idempotent by resource+action) # 1. Seed permissions (idempotent by resource+action)
permissions_by_key = {} permissions_by_key = {}
created_permissions = 0 created_permissions = 0
@@ -171,6 +206,24 @@ def migrate_role_permissions():
db.commit() db.commit()
logger.info(f"role_permissions: {linked} new links created") logger.info(f"role_permissions: {linked} new links created")
# 3b. One-time correction: an earlier version of this script granted
# coordinator submission:edit/submission:delete by default, which was
# wrong (those should be own-only by default, same as note:edit/
# delete). Revoke them if still present, idempotent - a no-op once
# corrected. Not a general revoke mechanism, just fixing this one
# past mistake.
coordinator = roles_by_name.get("coordinator")
revoked = 0
if coordinator:
for key in [("submission", "edit"), ("submission", "delete")]:
perm = permissions_by_key.get(key)
if perm and perm in coordinator.permissions:
coordinator.permissions.remove(perm)
revoked += 1
db.commit()
if revoked:
logger.info(f"role_permissions: {revoked} over-grant(s) revoked from coordinator (submission edit/delete correction)")
# 4. Backfill user_roles from users.role (idempotent - only add missing links) # 4. Backfill user_roles from users.role (idempotent - only add missing links)
users = db.query(User).all() users = db.query(User).all()
backfilled = 0 backfilled = 0
+22 -12
View File
@@ -1260,9 +1260,11 @@ async def update_task_note(
if not note: if not note:
raise HTTPException(status_code=404, detail="Note not found") raise HTTPException(status_code=404, detail="Note not found")
# Users can only update their own notes, unless they have admin permission or note:edit # Editing your own note requires note:edit_self; editing someone else's requires note:edit_other
if (note.user_id != current_user.id and not current_user.is_admin if not current_user.is_admin:
and not user_has_permission(current_user, 'note', 'edit', db)): is_own = note.user_id == current_user.id
action = 'edit_self' if is_own else 'edit_other'
if not user_has_permission(current_user, 'note', action, db):
raise HTTPException(status_code=403, detail="Not authorized to update this note") raise HTTPException(status_code=403, detail="Not authorized to update this note")
note.content = note_update.content note.content = note_update.content
@@ -1308,9 +1310,11 @@ async def delete_task_note(
if not note: if not note:
raise HTTPException(status_code=404, detail="Note not found") raise HTTPException(status_code=404, detail="Note not found")
# Users can only delete their own notes, unless they have admin permission or note:delete # Deleting your own note requires note:delete_self; deleting someone else's requires note:delete_other
if (note.user_id != current_user.id and not current_user.is_admin if not current_user.is_admin:
and not user_has_permission(current_user, 'note', 'delete', db)): is_own = note.user_id == current_user.id
action = 'delete_self' if is_own else 'delete_other'
if not user_has_permission(current_user, 'note', action, db):
raise HTTPException(status_code=403, detail="Not authorized to delete this note") raise HTTPException(status_code=403, detail="Not authorized to delete this note")
db.delete(note) db.delete(note)
@@ -1645,9 +1649,12 @@ async def update_task_submission(
if not submission: if not submission:
raise HTTPException(status_code=404, detail="Submission not found") raise HTTPException(status_code=404, detail="Submission not found")
# Users can only update their own submissions, unless they have admin permission or submission:edit # Editing your own submission requires submission:edit_self; editing
if (submission.user_id != current_user.id and not current_user.is_admin # someone else's requires submission:edit_other
and not user_has_permission(current_user, 'submission', 'edit', db)): if not current_user.is_admin:
is_own = submission.user_id == current_user.id
action = 'edit_self' if is_own else 'edit_other'
if not user_has_permission(current_user, 'submission', action, db):
raise HTTPException(status_code=403, detail="Not authorized to update this submission") raise HTTPException(status_code=403, detail="Not authorized to update this submission")
if submission_update.notes is not None: if submission_update.notes is not None:
@@ -1692,9 +1699,12 @@ async def delete_task_submission(
if not submission: if not submission:
raise HTTPException(status_code=404, detail="Submission not found") raise HTTPException(status_code=404, detail="Submission not found")
# Users can only delete their own submissions, unless they have admin permission or submission:delete # Deleting your own submission requires submission:delete_self; deleting
if (submission.user_id != current_user.id and not current_user.is_admin # someone else's requires submission:delete_other
and not user_has_permission(current_user, 'submission', 'delete', db)): if not current_user.is_admin:
is_own = submission.user_id == current_user.id
action = 'delete_self' if is_own else 'delete_other'
if not user_has_permission(current_user, 'submission', action, db):
raise HTTPException(status_code=403, detail="Not authorized to delete this submission") raise HTTPException(status_code=403, detail="Not authorized to delete this submission")
submission.deleted_at = datetime.utcnow() submission.deleted_at = datetime.utcnow()
@@ -148,6 +148,10 @@ const ACTION_LABELS: Record<string, string> = {
view_internal: 'View Internal', view_internal: 'View Internal',
view_client: 'View Client', view_client: 'View Client',
change_status: 'Change Status', change_status: 'Change Status',
edit_self: 'Edit Own',
delete_self: 'Delete Own',
edit_other: "Edit Others'",
delete_other: "Delete Others'",
} }
function resourceIcon(resource: string) { function resourceIcon(resource: string) {
+4 -2
View File
@@ -157,11 +157,13 @@ const showDeleteDialog = ref(false)
const isOwnNote = computed(() => authStore.user?.id === props.note.user_id) const isOwnNote = computed(() => authStore.user?.id === props.note.user_id)
const canEdit = computed(() => { const canEdit = computed(() => {
return isOwnNote.value || authStore.user?.is_admin || hasPermission('note', 'edit') if (authStore.user?.is_admin) return true
return isOwnNote.value ? hasPermission('note', 'edit_self') : hasPermission('note', 'edit_other')
}) })
const canDelete = computed(() => { const canDelete = computed(() => {
return isOwnNote.value || authStore.user?.is_admin || hasPermission('note', 'delete') if (authStore.user?.is_admin) return true
return isOwnNote.value ? hasPermission('note', 'delete_self') : hasPermission('note', 'delete_other')
}) })
function getInitials(firstName: string, lastName: string): string { function getInitials(firstName: string, lastName: string): string {
@@ -150,8 +150,14 @@ const authStore = useAuthStore()
const { hasPermission } = usePermission() const { hasPermission } = usePermission()
const isOwnSubmission = computed(() => authStore.user?.id === props.submission.user_id) const isOwnSubmission = computed(() => authStore.user?.id === props.submission.user_id)
const canEdit = computed(() => isOwnSubmission.value || authStore.user?.is_admin || hasPermission('submission', 'edit')) const canEdit = computed(() => {
const canDelete = computed(() => isOwnSubmission.value || authStore.user?.is_admin || hasPermission('submission', 'delete')) if (authStore.user?.is_admin) return true
return isOwnSubmission.value ? hasPermission('submission', 'edit_self') : hasPermission('submission', 'edit_other')
})
const canDelete = computed(() => {
if (authStore.user?.is_admin) return true
return isOwnSubmission.value ? hasPermission('submission', 'delete_self') : hasPermission('submission', 'delete_other')
})
const editing = ref(false) const editing = ref(false)
const editNotes = ref('') const editNotes = ref('')