From 960753b3d6a39d7b0f8263f949a76ce649672bc3 Mon Sep 17 00:00:00 2001 From: indigo Date: Sat, 18 Jul 2026 21:17:47 +0800 Subject: [PATCH] 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 --- backend/migrate_role_permissions.py | 79 ++++++++++++++++--- backend/routers/tasks.py | 42 ++++++---- .../src/components/role/RoleFormDialog.vue | 4 + frontend/src/components/task/NoteItem.vue | 6 +- .../src/components/task/SubmissionCard.vue | 10 ++- 5 files changed, 108 insertions(+), 33 deletions(-) diff --git a/backend/migrate_role_permissions.py b/backend/migrate_role_permissions.py index e0255b3..662c13f 100644 --- a/backend/migrate_role_permissions.py +++ b/backend/migrate_role_permissions.py @@ -45,13 +45,17 @@ PERMISSIONS = [ ("review", "publish", "Can approve a submission"), ("review", "retake", "Can request a retake on a submission"), ("submission", "create", "Can submit work for a task"), - ("submission", "edit", "Can edit a submission's notes"), - ("submission", "delete", "Can delete a submission"), + ("submission", "edit_self", "Can edit your own submission notes"), + ("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", "delete", "Can delete task attachments"), ("note", "create", "Can add task notes"), - ("note", "edit", "Can edit task notes"), - ("note", "delete", "Can delete task notes"), + ("note", "edit_self", "Can edit your own note"), + ("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_client", "Can view client notes"), ] @@ -79,30 +83,35 @@ SYSTEM_ROLE_DESCRIPTIONS = { # 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 # custom role if that capability is actually wanted). -# submission:edit/delete are brand new endpoints (didn't exist before this -# permission was added), so there's no "today's behavior" to preserve - -# granted to coordinator as the natural production-lead capability; artists -# keep editing/deleting their own submissions via the ownership check. +# note/submission edit_self+delete_self are granted to every system role - +# today, editing/deleting your OWN note or submission is an unconditional +# right with no role check at all, so faithful migration means everyone +# 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 = { - "coordinator": { + "coordinator": SELF_SERVICE_GRANTS | { ("asset", "create"), ("asset", "edit"), ("asset", "delete"), ("shot", "create"), ("shot", "edit"), ("shot", "delete"), ("task", "create"), ("task", "edit"), ("task", "delete"), ("task", "change_status"), ("assignment", "create"), ("assignment", "edit"), ("assignment", "delete"), ("review", "publish"), ("review", "retake"), - ("submission", "edit"), ("submission", "delete"), ("upload", "create"), ("upload", "delete"), ("note", "create"), ("note", "view_internal"), ("note", "view_client"), }, - "director": { + "director": SELF_SERVICE_GRANTS | { ("review", "publish"), ("review", "retake"), ("upload", "create"), ("note", "create"), ("note", "view_internal"), ("note", "view_client"), }, - "artist": { + "artist": SELF_SERVICE_GRANTS | { ("note", "view_internal"), }, - "developer": { + "developer": SELF_SERVICE_GRANTS | { ("upload", "create"), ("note", "create"), ("note", "view_internal"), }, @@ -131,6 +140,32 @@ def migrate_role_permissions(): )) 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) permissions_by_key = {} created_permissions = 0 @@ -171,6 +206,24 @@ def migrate_role_permissions(): db.commit() 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) users = db.query(User).all() backfilled = 0 diff --git a/backend/routers/tasks.py b/backend/routers/tasks.py index c7a34d9..3718b55 100644 --- a/backend/routers/tasks.py +++ b/backend/routers/tasks.py @@ -1260,10 +1260,12 @@ async def update_task_note( if not note: raise HTTPException(status_code=404, detail="Note not found") - # Users can only update their own notes, unless they have admin permission or note:edit - if (note.user_id != current_user.id and not current_user.is_admin - and not user_has_permission(current_user, 'note', 'edit', db)): - raise HTTPException(status_code=403, detail="Not authorized to update this note") + # Editing your own note requires note:edit_self; editing someone else's requires note:edit_other + if not current_user.is_admin: + 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") note.content = note_update.content db.commit() @@ -1308,10 +1310,12 @@ async def delete_task_note( if not note: raise HTTPException(status_code=404, detail="Note not found") - # Users can only delete their own notes, unless they have admin permission or note:delete - if (note.user_id != current_user.id and not current_user.is_admin - and not user_has_permission(current_user, 'note', 'delete', db)): - raise HTTPException(status_code=403, detail="Not authorized to delete this note") + # Deleting your own note requires note:delete_self; deleting someone else's requires note:delete_other + if not current_user.is_admin: + 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") db.delete(note) db.commit() @@ -1645,10 +1649,13 @@ async def update_task_submission( if not submission: raise HTTPException(status_code=404, detail="Submission not found") - # Users can only update their own submissions, unless they have admin permission or submission:edit - if (submission.user_id != current_user.id and not current_user.is_admin - and not user_has_permission(current_user, 'submission', 'edit', db)): - raise HTTPException(status_code=403, detail="Not authorized to update this submission") + # Editing your own submission requires submission:edit_self; editing + # someone else's requires submission:edit_other + 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") if submission_update.notes is not None: submission.notes = submission_update.notes @@ -1692,10 +1699,13 @@ async def delete_task_submission( if not submission: raise HTTPException(status_code=404, detail="Submission not found") - # Users can only delete their own submissions, unless they have admin permission or submission:delete - if (submission.user_id != current_user.id and not current_user.is_admin - and not user_has_permission(current_user, 'submission', 'delete', db)): - raise HTTPException(status_code=403, detail="Not authorized to delete this submission") + # Deleting your own submission requires submission:delete_self; deleting + # someone else's requires submission:delete_other + 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") submission.deleted_at = datetime.utcnow() submission.deleted_by = current_user.id diff --git a/frontend/src/components/role/RoleFormDialog.vue b/frontend/src/components/role/RoleFormDialog.vue index 56d5b37..680d06e 100644 --- a/frontend/src/components/role/RoleFormDialog.vue +++ b/frontend/src/components/role/RoleFormDialog.vue @@ -148,6 +148,10 @@ const ACTION_LABELS: Record = { view_internal: 'View Internal', view_client: 'View Client', change_status: 'Change Status', + edit_self: 'Edit Own', + delete_self: 'Delete Own', + edit_other: "Edit Others'", + delete_other: "Delete Others'", } function resourceIcon(resource: string) { diff --git a/frontend/src/components/task/NoteItem.vue b/frontend/src/components/task/NoteItem.vue index 96625b8..be03548 100644 --- a/frontend/src/components/task/NoteItem.vue +++ b/frontend/src/components/task/NoteItem.vue @@ -157,11 +157,13 @@ const showDeleteDialog = ref(false) const isOwnNote = computed(() => authStore.user?.id === props.note.user_id) 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(() => { - 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 { diff --git a/frontend/src/components/task/SubmissionCard.vue b/frontend/src/components/task/SubmissionCard.vue index 15d176d..88552a5 100644 --- a/frontend/src/components/task/SubmissionCard.vue +++ b/frontend/src/components/task/SubmissionCard.vue @@ -150,8 +150,14 @@ const authStore = useAuthStore() const { hasPermission } = usePermission() const isOwnSubmission = computed(() => authStore.user?.id === props.submission.user_id) -const canEdit = computed(() => isOwnSubmission.value || authStore.user?.is_admin || hasPermission('submission', 'edit')) -const canDelete = computed(() => isOwnSubmission.value || authStore.user?.is_admin || hasPermission('submission', 'delete')) +const canEdit = computed(() => { + 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 editNotes = ref('')