🐛(global) enforce full edit rights on thread mutations#623
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds thread-level abilities: new ThreadAbilities enum, Thread.get_abilities() and ThreadAccess.objects.editable_by(), exposes read-only Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as "API View / Serializer"
participant Model as "Thread.get_abilities"
participant Query as "ThreadAccess.objects.editable_by"
participant DB
Client->>API: Request thread resource or perform mutation
API->>Model: call get_abilities(user, mailbox_id?)
Model->>Query: exists/editable_by(user, mailbox_id?)
Query->>DB: filter ThreadAccess + MailboxAccess (EDITOR & mailbox roles)
DB-->>Query: existence / ids
Query-->>Model: editable result
Model-->>API: abilities dict (e.g., { edit: true/false })
API->>DB: guarded mutation (select_for_update/atomic) if abilities permit
DB-->>API: mutation result or ValidationError
API-->>Client: Response (includes `abilities`)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/backend/core/api/openapi.json (1)
8960-8966: Document the concreteabilities.editcontract explicitly.The open-ended object makes generated clients treat abilities as arbitrary keys, while this PR introduces a concrete
editcapability. Defining it explicitly will make the API contract clearer and safer for consumers.♻️ Suggested OpenAPI schema refinement
"abilities": { "type": "object", - "additionalProperties": { - "type": "boolean" - }, + "description": "Thread-level capabilities for the authenticated user.", + "properties": { + "edit": { + "type": "boolean", + "description": "Whether the current user can mutate this thread." + } + }, + "required": [ + "edit" + ], + "additionalProperties": true, "readOnly": true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/openapi.json` around lines 8960 - 8966, Update the OpenAPI schema for the "abilities" object to explicitly declare the concrete "edit" capability: inside the abilities schema (the "abilities" object definition) add a "properties" entry with "edit": { "type": "boolean" } and keep the existing readOnly flag; you may retain additionalProperties: { "type": "boolean" } if you want to allow other dynamic flags, or set additionalProperties: false to lock the contract to only the documented keys—ensure the "abilities" object in openapi.json includes the "edit" property so generated clients see it as a concrete boolean field.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/backend/core/api/permissions.py`:
- Around line 126-137: The write-check for nested create was added but
updates/deletes on ThreadEvent still bypass it; update has_object_permission so
that when handling ThreadEvent object writes (e.g., view.action in
['update','partial_update','destroy'] or request.method in
['PUT','PATCH','DELETE'] and the object is a ThreadEvent) you enforce the same
"full edit" requirement used for create: call
models.ThreadAccess.objects.editable_by(request.user).filter(thread_id=thread_id).exists()
(use obj.thread_id or obj.thread.id to get thread_id) instead of the current
author-only check in the ThreadEvent branch; keep the author check for
non-write/read cases as appropriate.
In `@src/backend/core/api/serializers.py`:
- Around line 628-642: The serializer's get_abilities (method name:
get_abilities on ThreadSerializer) must not compute global abilities when
mailbox_id is absent: change it to return an empty dict unless mailbox_id is
present in self.context, and only call instance.get_abilities(request.user,
mailbox_id=...) when that mailbox_id exists; also update all manual
ThreadSerializer(...) instantiations (e.g., the split action in viewsets.thread
where ThreadSerializer is called with {"request": request}) to pass the active
mailbox id into the context (e.g., include "mailbox_id": mailbox_id) so
abilities are always computed scoped to the active mailbox.
In `@src/backend/core/tests/api/test_messages_flag.py`:
- Line 687: The test currently uses MailboxFactory(users_admin=[user]) but
misses the regression where a viewer-level mailbox should not allow changing
flags even if ThreadAccess=EDITOR; add a test case (or extend the existing one
near MailboxFactory usage in test_messages_flag.py) that creates a mailbox with
the user as VIEWER on the mailbox while the thread grants EDITOR, then call the
change-flag endpoint and assert the response is 200 OK and that updated_threads
== 0 (mirror the assertions in test_api_flag_mark_messages_no_permission);
ensure you reference the same endpoint invocation and response fields used
elsewhere in this file so the new check runs alongside the existing happy-path
assertions without changing other fixtures.
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-actions.tsx`:
- Around line 34-38: The split-thread action is still gated by
selectedThread.user_role === "editor" but should follow the same full-edit check
used for reply/delete; update the logic where canSplitThread is computed to use
the existing canEditThread (the value from useAbility(Abilities.CAN_EDIT_THREAD,
selectedThread ?? null)) instead of checking selectedThread.user_role, and
ensure any UI condition rendering "Split thread from here" (and the duplicate
check later in the file) uses that canEditThread-based boolean so the split
action is hidden unless the user has CAN_EDIT_THREAD.
---
Nitpick comments:
In `@src/backend/core/api/openapi.json`:
- Around line 8960-8966: Update the OpenAPI schema for the "abilities" object to
explicitly declare the concrete "edit" capability: inside the abilities schema
(the "abilities" object definition) add a "properties" entry with "edit": {
"type": "boolean" } and keep the existing readOnly flag; you may retain
additionalProperties: { "type": "boolean" } if you want to allow other dynamic
flags, or set additionalProperties: false to lock the contract to only the
documented keys—ensure the "abilities" object in openapi.json includes the
"edit" property so generated clients see it as a concrete boolean field.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9d3e5833-fb46-4d7c-8011-428a618f38b7
⛔ Files ignored due to path filters (3)
src/frontend/src/features/api/gen/models/index.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_abilities.tsis excluded by!**/gen/**
📒 Files selected for processing (20)
src/backend/core/api/openapi.jsonsrc/backend/core/api/permissions.pysrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/flag.pysrc/backend/core/api/viewsets/label.pysrc/backend/core/api/viewsets/thread.pysrc/backend/core/enums.pysrc/backend/core/models.pysrc/backend/core/tests/api/test_labels.pysrc/backend/core/tests/api/test_messages_flag.pysrc/backend/core/tests/api/test_thread_event_permissions.pysrc/backend/core/tests/api/test_thread_split.pysrc/backend/core/tests/api/test_threads_flag.pysrc/backend/core/tests/api/test_threads_list.pysrc/frontend/src/features/layouts/components/thread-view/components/thread-action-bar/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-labels-widget/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-message/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-actions.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-header.tsxsrc/frontend/src/hooks/use-ability.ts
jbpenrath
force-pushed
the
fix/thread-right-permission
branch
2 times, most recently
from
c053f44 to
76f20bc
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/backend/core/api/serializers.py (1)
656-668:⚠️ Potential issue | 🟠 MajorDon't compute
abilitieswithout a mailbox scope.
get_user_role()andget_labels()already bail out whenmailbox_idis missing, but this method still callsinstance.get_abilities(..., mailbox_id=None). ManualThreadSerializer(...)responses such as the split response insrc/backend/core/api/viewsets/thread.pyonly pass{"request": request}, so they can emitabilities.editfrom another mailbox. Return{}unlessmailbox_idis present, and pass it explicitly from manual serializer call sites.🔧 Suggested hardening
`@extend_schema_field`(serializers.DictField(child=serializers.BooleanField())) def get_abilities(self, instance): """Return the current user's abilities on the thread, scoped to the mailbox context when provided. Frontend components use this to hide mutating actions (archive, spam, delete, reply...) from users with read-only access. """ request = self.context.get("request") - if request is None: + mailbox_id = self.context.get("mailbox_id") + if request is None or mailbox_id is None: return {} - return instance.get_abilities( - request.user, mailbox_id=self.context.get("mailbox_id") - ) + return instance.get_abilities(request.user, mailbox_id=mailbox_id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/serializers.py` around lines 656 - 668, The get_abilities serializer method should not call instance.get_abilities with mailbox_id=None; update get_abilities in serializers.py to read mailbox_id = self.context.get("mailbox_id") and immediately return {} if mailbox_id is falsy, otherwise call instance.get_abilities(request.user, mailbox_id=mailbox_id); also update any manual ThreadSerializer(...) usages (e.g., the split response in viewsets/thread.py and other manual serializer call sites) to pass mailbox_id into the serializer context so abilities are computed only within the proper mailbox scope.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/backend/core/api/viewsets/thread_access.py`:
- Around line 67-82: When deleting in perform_destroy, wrap the last-editor
existence check and delete in a transaction and acquire a DB lock to prevent
concurrent races: start a transaction.atomic block, lock either the parent
thread row (e.g., Thread.objects.select_for_update().get(pk=instance.thread.pk))
or the editor rows
(models.ThreadAccess.objects.select_for_update().filter(thread=instance.thread,
role=enums.ThreadAccessRoleChoices.EDITOR)), then perform the same
exclude(id=instance.id).exists() check and raise ValidationError if no remaining
editors, finally call super().perform_destroy(instance) inside the same
transaction so the check and delete are atomic.
In
`@src/frontend/src/features/layouts/components/thread-panel/components/thread-panel-header.tsx`:
- Around line 55-60: The bulk-action gate (canEditSelection) currently uses
Array.prototype.some to allow actions if any selected thread is editable, but
handlers still submit the full selectedThreadIds via threadIdsToMark, causing
read-only IDs to be sent; fix by making the gate and payload agree: either
change the canEditSelection memo to require every selected id be editable
(replace .some with .every and keep selectedThreadIds as-is), or keep the some
check but update the archive/spam/trash handlers (the functions that build
threadIdsToMark) to filter selectedThreadIds down to only those threads found
via threads?.results with thread?.abilities?.edit === true before submitting.
Ensure you update references to canEditSelection, selectedThreadIds,
threads?.results, and the threadIdsToMark construction in the handlers so the UI
gate matches the actual payload.
---
Duplicate comments:
In `@src/backend/core/api/serializers.py`:
- Around line 656-668: The get_abilities serializer method should not call
instance.get_abilities with mailbox_id=None; update get_abilities in
serializers.py to read mailbox_id = self.context.get("mailbox_id") and
immediately return {} if mailbox_id is falsy, otherwise call
instance.get_abilities(request.user, mailbox_id=mailbox_id); also update any
manual ThreadSerializer(...) usages (e.g., the split response in
viewsets/thread.py and other manual serializer call sites) to pass mailbox_id
into the serializer context so abilities are computed only within the proper
mailbox scope.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: be0df757-3ac5-4efe-8f0c-5b9039635292
⛔ Files ignored due to path filters (4)
src/frontend/package-lock.jsonis excluded by!**/package-lock.jsonsrc/frontend/src/features/api/gen/models/index.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_abilities.tsis excluded by!**/gen/**
📒 Files selected for processing (27)
src/backend/core/api/openapi.jsonsrc/backend/core/api/permissions.pysrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/flag.pysrc/backend/core/api/viewsets/label.pysrc/backend/core/api/viewsets/thread.pysrc/backend/core/api/viewsets/thread_access.pysrc/backend/core/enums.pysrc/backend/core/models.pysrc/backend/core/tests/api/test_labels.pysrc/backend/core/tests/api/test_messages_flag.pysrc/backend/core/tests/api/test_thread_access.pysrc/backend/core/tests/api/test_thread_event_permissions.pysrc/backend/core/tests/api/test_thread_split.pysrc/backend/core/tests/api/test_threads_flag.pysrc/backend/core/tests/api/test_threads_list.pysrc/frontend/package.jsonsrc/frontend/public/locales/common/en-US.jsonsrc/frontend/public/locales/common/fr-FR.jsonsrc/frontend/src/features/layouts/components/thread-panel/components/thread-panel-header.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-accesses-widget/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-action-bar/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-labels-widget/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-message/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-actions.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-header.tsxsrc/frontend/src/hooks/use-ability.ts
✅ Files skipped from review due to trivial changes (6)
- src/frontend/package.json
- src/frontend/src/features/layouts/components/thread-view/components/thread-message/index.tsx
- src/frontend/public/locales/common/en-US.json
- src/frontend/public/locales/common/fr-FR.json
- src/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-header.tsx
- src/backend/core/tests/api/test_messages_flag.py
🚧 Files skipped from review as they are similar to previous changes (10)
- src/frontend/src/features/layouts/components/thread-view/components/thread-labels-widget/index.tsx
- src/backend/core/tests/api/test_thread_event_permissions.py
- src/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-actions.tsx
- src/frontend/src/hooks/use-ability.ts
- src/backend/core/tests/api/test_thread_split.py
- src/backend/core/api/openapi.json
- src/frontend/src/features/layouts/components/thread-view/components/thread-action-bar/index.tsx
- src/backend/core/api/viewsets/label.py
- src/backend/core/api/permissions.py
- src/backend/core/api/viewsets/flag.py
jbpenrath
force-pushed
the
fix/thread-right-permission
branch
from
76f20bc to
eeb0398
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/backend/core/api/serializers.py (1)
656-668:⚠️ Potential issue | 🟠 MajorScope
Thread.abilitiesto an explicit mailbox.Line 667 still falls back to
mailbox_id=None, which makesabilities.editreflect any editable mailbox. On manual serializer paths that don't injectmailbox_id, a read-only mailbox can therefore render thread actions as editable again.🔧 Minimal hardening
request = self.context.get("request") if request is None: return {} - return instance.get_abilities( - request.user, mailbox_id=self.context.get("mailbox_id") - ) + mailbox_id = self.context.get("mailbox_id") or request.query_params.get( + "mailbox_id" + ) + if mailbox_id is None: + return {} + return instance.get_abilities(request.user, mailbox_id=mailbox_id)Please also keep manual
ThreadSerializer(...)call sites passingmailbox_idexplicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/serializers.py` around lines 656 - 668, The get_abilities serializer currently calls instance.get_abilities with mailbox_id defaulting to None which leaks editability across mailboxes; change get_abilities so it reads mailbox_id = self.context.get("mailbox_id") and if mailbox_id is None return {} (i.e. do not call instance.get_abilities with None), then call instance.get_abilities(request.user, mailbox_id=mailbox_id) only when mailbox_id is present; also audit and update manual ThreadSerializer(...) call sites to pass mailbox_id explicitly so the serializer always has an explicit mailbox scope.
🧹 Nitpick comments (5)
src/frontend/src/features/layouts/components/thread-panel/components/thread-panel-header.tsx (1)
57-62: Use the shared thread ability constant here too.This reintroduces the ability name as a string literal even though
use-ability.tsnow centralizes it. ReusingAbilities.CAN_EDIT_THREADhere will keep the selection gate aligned if the ability key ever changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/src/features/layouts/components/thread-panel/components/thread-panel-header.tsx` around lines 57 - 62, The selection permission check in the canEditSelection useMemo reuses the literal "edit" when checking thread?.abilities?.edit; update this to use the centralized ability constant exported from use-ability (Abilities.CAN_EDIT_THREAD) so the check reads thread?.abilities?.[Abilities.CAN_EDIT_THREAD] === true (locate the canEditSelection useMemo, selectedThreadIds and threads?.results lookup in thread-panel-header.tsx and import Abilities from use-ability.ts if not already).src/backend/core/tests/api/test_thread_access.py (1)
801-816: Make the race test actually overlap the two deletes.Both workers can currently run serially and still satisfy the
204/400assertion, so removing the lock may stay green by chance. Add a barrier (or another pause around the critical section) beforeclient.delete(...)so this fails reliably when the last-editor check stops being atomic.🔬 Minimal improvement
+ start_barrier = threading.Barrier(2) + def delete_access(name, url): try: + start_barrier.wait(timeout=5) client = APIClient() client.force_authenticate(user=user) results[name] = client.delete(url)As per coding guidelines, "Unit tests should focus on a single use case, keep assertions minimal, and cover all possible cases."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/tests/api/test_thread_access.py` around lines 801 - 816, The race test's threads (delete_access, t1, t2) may run serially, so add synchronization to force overlap: create a threading.Barrier(2) (or similar event) in the test scope and have delete_access call barrier.wait() right after client.force_authenticate(user=user) and before client.delete(url) so both threads block and then perform client.delete concurrently; keep the existing finally block that closes the DB connection.src/backend/core/api/viewsets/thread.py (1)
873-879: Useself.get_serializer()here to keep the response context aligned.Manually building
ThreadSerializer(...)duplicatesget_serializer_context()and drops the default DRF context keys (view,format). That makes the split response easier to drift from list/retrieve serialization.♻️ Minimal refactor
- serializer = serializers.ThreadSerializer( - new_thread, - context={ - "request": request, - "mailbox_id": request.query_params.get("mailbox_id"), - }, - ) + serializer = self.get_serializer(new_thread)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/viewsets/thread.py` around lines 873 - 879, Replace the manual instantiation of ThreadSerializer with the view's serializer factory to preserve DRF context: instead of calling serializers.ThreadSerializer(new_thread, context={...}), use self.get_serializer(...) so the serializer gets the full get_serializer_context() (which includes view, request, format) while still passing or augmenting mailbox_id from request.query_params; ensure you pass new_thread as the instance to self.get_serializer to keep behavior identical.src/backend/core/tests/api/test_threads_list.py (1)
269-323: Please add direct coverage forabilities.editon the list payload.These tests prove the backend rejects forbidden mutations, but the frontend now hides the same controls off
ThreadSerializer.abilities.edit. A focused list-endpoint assertion forVIEWER mailbox + EDITOR thread => falseandEDITOR + EDITOR => truewould lock down the new UI contract too. As per coding guidelines, "Unit tests should focus on a single use case, keep assertions minimal, and cover all possible cases".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/tests/api/test_threads_list.py` around lines 269 - 323, Add assertions against the list endpoint payload to verify ThreadSerializer.abilities.edit for the two cases; after creating the thread with MailboxFactory(users_read=[user]) and ThreadAccessFactory(..., role=EDITOR), call reverse("threads-list") and GET it via api_client, then assert response.data[0]["abilities"]["edit"] is False for the VIEWER mailbox case; then update the MailboxAccess role to EDITOR, GET the list again and assert response.data[0]["abilities"]["edit"] is True. Use the existing test setup (user, mailbox, thread, ThreadAccessFactory, MailboxAccess) and the same URL helper to keep the checks minimal and focused.src/frontend/src/features/message/use-starred.tsx (1)
22-31: Consider centralizing the 0/partial/full toast builder.This branching is now copied across
use-starred.tsx,use-archive.tsx,use-spam.tsx, anduse-trash.tsx. Pulling it intouse-flag(or a small shared helper) would reduce translation drift and keep future message changes consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/src/features/message/use-starred.tsx` around lines 22 - 31, Extract the duplicated 0/partial/full toast message logic from use-starred.tsx (the thread and message functions) into a shared helper (e.g., getFlagToastMessage or use-flag) that accepts parameters: entity type ('thread' | 'message'), the i18n translator t, updatedCount and submittedCount; replace the inline thread/message lambdas in use-starred.tsx with calls to that helper and update use-archive.tsx, use-spam.tsx, and use-trash.tsx to use the same helper so translations and branching are centralized and consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/frontend/src/features/message/use-flag.tsx`:
- Around line 40-45: The comparison is mixing thread-scoped
response.updated_threads with a message-scoped submitted count
(data.message_ids.length), causing incorrect toast types; update the logic in
use-flag (references: responseData.updated_threads, submittedCount,
data.thread_ids, data.message_ids, toastIdSuffix) to compute submittedCount in
the same unit as updated_threads (i.e., convert message_ids to a unique thread
count or use only data.thread_ids when available), and preserve undefined as the
fallback success path when the API does not return a thread-scoped counter;
apply the same normalization in the other occurrences noted (lines ~99-102 and
~129-130) so comparisons only occur between like-for-like counts.
- Around line 36-45: The onSuccess handler in use-flag.tsx currently calls
options?.onSuccess with the original request payload (data) even when the server
wrote zero or only some of the requested IDs; compute the effective result from
the response (use response.data.updated_threads or any server-returned
list/count) and pass that reconciled result to downstream handlers instead of
the original data, or alternatively only invoke options?.onSuccess when
updatedCount === submittedCount; update the onSuccess branch around the existing
options?.onSuccess?.(data) call to use the response-derived effective IDs/count
(and adjust toast logic to use response.data.updated_threads as you already do)
so callers receive accurate state.
---
Duplicate comments:
In `@src/backend/core/api/serializers.py`:
- Around line 656-668: The get_abilities serializer currently calls
instance.get_abilities with mailbox_id defaulting to None which leaks
editability across mailboxes; change get_abilities so it reads mailbox_id =
self.context.get("mailbox_id") and if mailbox_id is None return {} (i.e. do not
call instance.get_abilities with None), then call
instance.get_abilities(request.user, mailbox_id=mailbox_id) only when mailbox_id
is present; also audit and update manual ThreadSerializer(...) call sites to
pass mailbox_id explicitly so the serializer always has an explicit mailbox
scope.
---
Nitpick comments:
In `@src/backend/core/api/viewsets/thread.py`:
- Around line 873-879: Replace the manual instantiation of ThreadSerializer with
the view's serializer factory to preserve DRF context: instead of calling
serializers.ThreadSerializer(new_thread, context={...}), use
self.get_serializer(...) so the serializer gets the full
get_serializer_context() (which includes view, request, format) while still
passing or augmenting mailbox_id from request.query_params; ensure you pass
new_thread as the instance to self.get_serializer to keep behavior identical.
In `@src/backend/core/tests/api/test_thread_access.py`:
- Around line 801-816: The race test's threads (delete_access, t1, t2) may run
serially, so add synchronization to force overlap: create a threading.Barrier(2)
(or similar event) in the test scope and have delete_access call barrier.wait()
right after client.force_authenticate(user=user) and before client.delete(url)
so both threads block and then perform client.delete concurrently; keep the
existing finally block that closes the DB connection.
In `@src/backend/core/tests/api/test_threads_list.py`:
- Around line 269-323: Add assertions against the list endpoint payload to
verify ThreadSerializer.abilities.edit for the two cases; after creating the
thread with MailboxFactory(users_read=[user]) and ThreadAccessFactory(...,
role=EDITOR), call reverse("threads-list") and GET it via api_client, then
assert response.data[0]["abilities"]["edit"] is False for the VIEWER mailbox
case; then update the MailboxAccess role to EDITOR, GET the list again and
assert response.data[0]["abilities"]["edit"] is True. Use the existing test
setup (user, mailbox, thread, ThreadAccessFactory, MailboxAccess) and the same
URL helper to keep the checks minimal and focused.
In
`@src/frontend/src/features/layouts/components/thread-panel/components/thread-panel-header.tsx`:
- Around line 57-62: The selection permission check in the canEditSelection
useMemo reuses the literal "edit" when checking thread?.abilities?.edit; update
this to use the centralized ability constant exported from use-ability
(Abilities.CAN_EDIT_THREAD) so the check reads
thread?.abilities?.[Abilities.CAN_EDIT_THREAD] === true (locate the
canEditSelection useMemo, selectedThreadIds and threads?.results lookup in
thread-panel-header.tsx and import Abilities from use-ability.ts if not
already).
In `@src/frontend/src/features/message/use-starred.tsx`:
- Around line 22-31: Extract the duplicated 0/partial/full toast message logic
from use-starred.tsx (the thread and message functions) into a shared helper
(e.g., getFlagToastMessage or use-flag) that accepts parameters: entity type
('thread' | 'message'), the i18n translator t, updatedCount and submittedCount;
replace the inline thread/message lambdas in use-starred.tsx with calls to that
helper and update use-archive.tsx, use-spam.tsx, and use-trash.tsx to use the
same helper so translations and branching are centralized and consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7cb47a75-1c14-4d80-a0c6-52838f2b4c80
⛔ Files ignored due to path filters (4)
src/frontend/package-lock.jsonis excluded by!**/package-lock.jsonsrc/frontend/src/features/api/gen/models/index.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_abilities.tsis excluded by!**/gen/**
📒 Files selected for processing (33)
src/backend/core/api/openapi.jsonsrc/backend/core/api/permissions.pysrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/flag.pysrc/backend/core/api/viewsets/label.pysrc/backend/core/api/viewsets/thread.pysrc/backend/core/api/viewsets/thread_access.pysrc/backend/core/enums.pysrc/backend/core/models.pysrc/backend/core/tests/api/test_labels.pysrc/backend/core/tests/api/test_messages_flag.pysrc/backend/core/tests/api/test_thread_access.pysrc/backend/core/tests/api/test_thread_event_permissions.pysrc/backend/core/tests/api/test_thread_split.pysrc/backend/core/tests/api/test_threads_flag.pysrc/backend/core/tests/api/test_threads_list.pysrc/frontend/package.jsonsrc/frontend/public/locales/common/en-US.jsonsrc/frontend/public/locales/common/fr-FR.jsonsrc/frontend/src/features/layouts/components/thread-panel/components/thread-panel-header.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-accesses-widget/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-action-bar/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-labels-widget/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-message/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-actions.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-header.tsxsrc/frontend/src/features/message/use-archive.tsxsrc/frontend/src/features/message/use-flag.tsxsrc/frontend/src/features/message/use-spam.tsxsrc/frontend/src/features/message/use-starred.tsxsrc/frontend/src/features/message/use-trash.tsxsrc/frontend/src/features/providers/modal-store/controlled-modal.tsxsrc/frontend/src/hooks/use-ability.ts
✅ Files skipped from review due to trivial changes (5)
- src/frontend/package.json
- src/backend/core/enums.py
- src/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-header.tsx
- src/backend/core/api/openapi.json
- src/frontend/public/locales/common/en-US.json
🚧 Files skipped from review as they are similar to previous changes (11)
- src/backend/core/tests/api/test_thread_split.py
- src/backend/core/tests/api/test_thread_event_permissions.py
- src/frontend/public/locales/common/fr-FR.json
- src/frontend/src/features/layouts/components/thread-view/components/thread-labels-widget/index.tsx
- src/frontend/src/features/layouts/components/thread-view/components/thread-action-bar/index.tsx
- src/frontend/src/features/layouts/components/thread-view/components/thread-accesses-widget/index.tsx
- src/backend/core/api/viewsets/thread_access.py
- src/backend/core/tests/api/test_labels.py
- src/backend/core/tests/api/test_threads_flag.py
- src/backend/core/api/viewsets/label.py
- src/backend/core/models.py
jbpenrath
force-pushed
the
fix/thread-right-permission
branch
4 times, most recently
from
07e9736 to
8cd6f21
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/backend/core/api/serializers.py (1)
656-677:⚠️ Potential issue | 🟠 MajorFail closed when
mailbox_idis missing in the fallback ability path.When
_can_editis absent, this still callsinstance.get_abilities(..., mailbox_id=None), which computes edit rights across all accessible mailboxes. That can reportabilities.edit = truefor a viewer mailbox if the same thread is editable elsewhere. Return{}unless the active mailbox is known, and keep passingmailbox_idexplicitly from manualThreadSerializer(...)call sites.🔧 Minimal hardening
def get_abilities(self, instance): ... request = self.context.get("request") - if request is None: + mailbox_id = self.context.get("mailbox_id") + if request is None or mailbox_id is None: return {} return instance.get_abilities( - request.user, mailbox_id=self.context.get("mailbox_id") + request.user, mailbox_id=mailbox_id )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/serializers.py` around lines 656 - 677, The fallback in ThreadSerializer.get_abilities uses instance.get_abilities(request.user, mailbox_id=None) when the _can_edit annotation is missing, which exposes edit rights across all mailboxes; change the logic so that if _can_edit is None and there is no mailbox_id in self.context you return {} (i.e., fail closed) instead of calling instance.get_abilities, and ensure any manual ThreadSerializer(...) call sites continue to pass mailbox_id explicitly into the serializer context; locate the get_abilities method on the serializer, the getattr(instance, "_can_edit", None) branch, and the instance.get_abilities(...) call to implement this guard.
🧹 Nitpick comments (1)
src/frontend/src/features/message/use-flag.tsx (1)
44-48: Toast type computed inconsistently between hook and component.The hook computes
typeat line 44-47 and embeds it intoastId, but the component independently computestoastTypeat line 120 with different logic:
Scenario Hook typeComponent toastTypeupdatedCountundefined'success''info'Full success 'success''info'This causes the
toastIdsuffix (--success) to mismatch the rendered toast style (info). Consider either:
- Passing
typefrom the hook to the component as a prop, or- Aligning the default to
'success'in the component♻️ Option 1: Pass type as prop
type FlagUpdateSuccessToastProps = { flag: FlagEnum; threadIds?: Thread['id'][]; messageIds?: Message['id'][]; mailboxId?: Mailbox['id']; toastId: string; messages?: FlagToastMessages; onUndo?: (data: ChangeFlagRequestRequest) => void; updatedCount?: number; + type?: 'success' | 'warning' | 'error'; } -const FlagUpdateSuccessToast = ({ flag, threadIds = [], messageIds = [], mailboxId, toastId, messages, onUndo, updatedCount, closeToast }: FlagUpdateSuccessToastProps & Partial<ToastContentProps>) => { +const FlagUpdateSuccessToast = ({ flag, threadIds = [], messageIds = [], mailboxId, toastId, messages, onUndo, updatedCount, type = 'success', closeToast }: FlagUpdateSuccessToastProps & Partial<ToastContentProps>) => {Then in
useFlag, passtypeto the component and use it directly fortoastType.Also applies to: 120-120
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/src/features/message/use-flag.tsx` around lines 44 - 48, The hook useFlag computes a `type` and embeds it in `toastId`, but the component recomputes `toastType` with different defaults causing mismatched IDs/styles; fix by passing the computed `type` from useFlag into the component (add a prop like `toastType` or `type` on the component render) and use that prop instead of recomputing `toastType` in the component, ensuring the hook's `type`, the `toastId` and the rendered toast style are all derived from the same value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/backend/core/api/permissions.py`:
- Around line 327-333: The current shortcut lets any user with mailbox access
delete the mailbox-scoped ThreadAccess because it checks view.action ==
"destroy" and obj.mailbox.accesses.filter(user=request.user).exists(); change
this so only the owner-scoped access can self-remove: require that the
ThreadAccess being deleted belongs to the requesting user (e.g., check obj.user
== request.user) before returning True, otherwise remove this branch so the
normal full-edit permission logic applies; reference ThreadAccess uniqueness
(thread, mailbox) to justify the obj.user check and update the block in
permissions.py accordingly.
---
Duplicate comments:
In `@src/backend/core/api/serializers.py`:
- Around line 656-677: The fallback in ThreadSerializer.get_abilities uses
instance.get_abilities(request.user, mailbox_id=None) when the _can_edit
annotation is missing, which exposes edit rights across all mailboxes; change
the logic so that if _can_edit is None and there is no mailbox_id in
self.context you return {} (i.e., fail closed) instead of calling
instance.get_abilities, and ensure any manual ThreadSerializer(...) call sites
continue to pass mailbox_id explicitly into the serializer context; locate the
get_abilities method on the serializer, the getattr(instance, "_can_edit", None)
branch, and the instance.get_abilities(...) call to implement this guard.
---
Nitpick comments:
In `@src/frontend/src/features/message/use-flag.tsx`:
- Around line 44-48: The hook useFlag computes a `type` and embeds it in
`toastId`, but the component recomputes `toastType` with different defaults
causing mismatched IDs/styles; fix by passing the computed `type` from useFlag
into the component (add a prop like `toastType` or `type` on the component
render) and use that prop instead of recomputing `toastType` in the component,
ensuring the hook's `type`, the `toastId` and the rendered toast style are all
derived from the same value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 56725baf-297b-4501-9346-ed0322e89ac8
⛔ Files ignored due to path filters (4)
src/frontend/package-lock.jsonis excluded by!**/package-lock.jsonsrc/frontend/src/features/api/gen/models/index.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_abilities.tsis excluded by!**/gen/**
📒 Files selected for processing (36)
src/backend/core/api/openapi.jsonsrc/backend/core/api/permissions.pysrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/flag.pysrc/backend/core/api/viewsets/label.pysrc/backend/core/api/viewsets/thread.pysrc/backend/core/api/viewsets/thread_access.pysrc/backend/core/enums.pysrc/backend/core/models.pysrc/backend/core/tests/api/test_labels.pysrc/backend/core/tests/api/test_messages_flag.pysrc/backend/core/tests/api/test_thread_access.pysrc/backend/core/tests/api/test_thread_event_permissions.pysrc/backend/core/tests/api/test_thread_split.pysrc/backend/core/tests/api/test_threads_flag.pysrc/backend/core/tests/api/test_threads_list.pysrc/backend/e2e/management/commands/e2e_demo.pysrc/e2e/src/__tests__/thread-starred-read.spec.tssrc/frontend/package.jsonsrc/frontend/public/locales/common/en-US.jsonsrc/frontend/public/locales/common/fr-FR.jsonsrc/frontend/src/features/layouts/components/thread-panel/components/thread-panel-header.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-accesses-widget/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-action-bar/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-event-input/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-labels-widget/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-message/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-actions.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-header.tsxsrc/frontend/src/features/message/use-archive.tsxsrc/frontend/src/features/message/use-flag.tsxsrc/frontend/src/features/message/use-spam.tsxsrc/frontend/src/features/message/use-starred.tsxsrc/frontend/src/features/message/use-trash.tsxsrc/frontend/src/features/providers/modal-store/controlled-modal.tsxsrc/frontend/src/hooks/use-ability.ts
✅ Files skipped from review due to trivial changes (6)
- src/frontend/src/features/layouts/components/thread-view/components/thread-event-input/index.tsx
- src/backend/core/enums.py
- src/frontend/package.json
- src/frontend/public/locales/common/fr-FR.json
- src/frontend/src/features/providers/modal-store/controlled-modal.tsx
- src/frontend/public/locales/common/en-US.json
🚧 Files skipped from review as they are similar to previous changes (15)
- src/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-header.tsx
- src/frontend/src/features/layouts/components/thread-view/components/thread-accesses-widget/index.tsx
- src/frontend/src/features/message/use-trash.tsx
- src/frontend/src/features/message/use-spam.tsx
- src/backend/core/tests/api/test_thread_split.py
- src/backend/core/tests/api/test_thread_event_permissions.py
- src/frontend/src/features/message/use-starred.tsx
- src/frontend/src/features/layouts/components/thread-view/components/thread-labels-widget/index.tsx
- src/backend/core/api/openapi.json
- src/frontend/src/features/layouts/components/thread-view/components/thread-message/index.tsx
- src/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-actions.tsx
- src/backend/core/api/viewsets/thread_access.py
- src/backend/core/api/viewsets/label.py
- src/backend/core/tests/api/test_thread_access.py
- src/backend/core/api/viewsets/thread.py
A user with VIEWER MailboxAccess on a shared mailbox could still mutate threads that the mailbox had EDITOR ThreadAccess to: the permission check only looked at ThreadAccess.role, never at MailboxAccess.role. Both roles must now be satisfied (EDITOR on ThreadAccess AND a role in MAILBOX_ROLES_CAN_EDIT on MailboxAccess) for archive, spam, trash, label, split, refresh_summary and thread-event writes. Personal actions (unread, starred) intentionally stay open to any mailbox access since they only mutate the caller's own ThreadAccess row. The rule is centralised in ThreadAccessQuerySet.editable_by(user, mailbox_id) so viewsets and permission classes share a single source of truth, and exposed to the frontend via a new Thread.abilities.edit field consumed by use-ability, which gates the matching UI controls.
Upgrade @gouvfr-lasuite/cunningham-react and @gouvfr-lasuite/ui-kit to its latest version
jbpenrath
force-pushed
the
fix/thread-right-permission
branch
from
8cd6f21 to
a7dc4b4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/backend/core/api/serializers.py (1)
656-677:⚠️ Potential issue | 🟡 MinorDon't fall back to cross-mailbox
abilities.edithere.When
context["mailbox_id"]is missing, this path asksThread.get_abilities()for the user's editability across any mailbox. That can surface edit controls for a mailbox-scoped thread view where the user is only a viewer on the active mailbox but an editor somewhere else.🔧 Minimal hardening
def get_abilities(self, instance): """Return the current user's abilities on the thread, scoped to the mailbox context when provided. Frontend components use this to hide mutating actions (archive, spam, delete, reply...) from users with read-only access. @@ request = self.context.get("request") - if request is None: + mailbox_id = self.context.get("mailbox_id") + if mailbox_id is None and request is not None: + mailbox_id = request.query_params.get("mailbox_id") + if request is None or mailbox_id is None: return {} - return instance.get_abilities( - request.user, mailbox_id=self.context.get("mailbox_id") - ) + return instance.get_abilities(request.user, mailbox_id=mailbox_id)Please also confirm the manual
ThreadSerializer(...)call sites always pass the active mailbox id.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/serializers.py` around lines 656 - 677, The serializer's get_abilities currently falls back to instance.get_abilities without a mailbox scope, allowing cross-mailbox edit flags; change get_abilities (method in the serializer) to only call instance.get_abilities when self.context contains a mailbox_id (use mailbox_id = self.context.get("mailbox_id") and if not present return {}), and avoid passing None/omitting mailbox_id to instance.get_abilities; also audit all manual ThreadSerializer(...) instantiations to ensure they pass the active mailbox_id in the context so mailbox-scoped abilities are always provided.
🧹 Nitpick comments (2)
src/backend/core/tests/api/test_labels.py (2)
665-700: Clarify test intent: mailbox role is VIEWER, not thread role.The first two test names/docstrings say “viewer thread access,” but
ThreadAccessisEDITOR; only mailbox access isVIEWER. Also, Line 678’s inline note says mailbox has edit access, which is the opposite of the setup. Renaming these tests/comments will avoid confusion.Proposed naming/comment cleanup
-def test_add_threads_cross_mailbox_viewer_thread_access_forbidden( +def test_add_threads_cross_mailbox_viewer_mailbox_access_forbidden( self, api_client, label, user ): - """User is EDITOR/ADMIN on label.mailbox (can manage labels) but - only has VIEWER ThreadAccess to the thread (via a different + """User is EDITOR/ADMIN on label.mailbox (can manage labels) but + only has VIEWER mailbox access to the thread mailbox (with EDITOR + ThreadAccess in that mailbox). Mutating shared thread state must be blocked. - role=enums.ThreadAccessRoleChoices.EDITOR, # Mailbox has edit access + role=enums.ThreadAccessRoleChoices.EDITOR, # ThreadAccess editor; mailbox role is VIEWER )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/tests/api/test_labels.py` around lines 665 - 700, The tests test_add_threads_cross_mailbox_viewer_thread_access_forbidden and test_remove_threads_cross_mailbox_viewer_thread_access_forbidden have misleading names/docstrings and an inline note: the mailbox role is VIEWER (other_mailbox.accesses.create(..., role=models.MailboxRoleChoices.VIEWER)) while the thread access is set to EDITOR (thread.accesses.create(..., role=enums.ThreadAccessRoleChoices.EDITOR)), so update the test names, docstrings, and the inline comment to clearly state that the mailbox role is VIEWER and the thread has EDITOR access (e.g., rename to *_cross_mailbox_mailbox_viewer_thread_edit_access_* or adjust docstrings to say "mailbox role is VIEWER but thread access is EDITOR"), and correct the inline comment that currently reads "Mailbox has edit access" to reflect the actual setup.
665-755: Add a same-mailboxThreadAccess=VIEWERnegative case to complete permission coverage.These tests validate mailbox scoping well, but they don’t directly assert the thread-role half of
editable_byin the label mailbox itself. Adding one add/remove case where the user can manage labels onlabel.mailboxbut the target thread hasThreadAccessRoleChoices.VIEWERin that same mailbox would close that gap.Suggested additional tests
+ def test_add_threads_same_mailbox_viewer_thread_access_forbidden(self, api_client, label): + thread = ThreadFactory() + thread.accesses.create( + mailbox=label.mailbox, + role=enums.ThreadAccessRoleChoices.VIEWER, + ) + url = reverse("labels-add-threads", args=[label.pk]) + response = api_client.post(url, {"thread_ids": [str(thread.id)]}, format="json") + assert response.status_code == status.HTTP_200_OK + assert not label.threads.filter(pk=thread.pk).exists() + + def test_remove_threads_same_mailbox_viewer_thread_access_forbidden(self, api_client, label): + thread = ThreadFactory() + thread.accesses.create( + mailbox=label.mailbox, + role=enums.ThreadAccessRoleChoices.VIEWER, + ) + label.threads.add(thread) + url = reverse("labels-remove-threads", args=[label.pk]) + response = api_client.post(url, {"thread_ids": [str(thread.id)]}, format="json") + assert response.status_code == status.HTTP_200_OK + assert label.threads.filter(pk=thread.pk).exists()As per coding guidelines "Unit tests should focus on a single use case, keep assertions minimal, and cover all possible cases".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/tests/api/test_labels.py` around lines 665 - 755, Add two tests mirroring the existing cross-mailbox VIEWER cases but where the ThreadAccess=VIEWER is set on the label's own mailbox: implement test_add_threads_same_mailbox_viewer_thread_access_forbidden and test_remove_threads_same_mailbox_viewer_thread_access_forbidden that create a Thread via ThreadFactory, call thread.accesses.create(mailbox=label.mailbox, role=enums.ThreadAccessRoleChoices.VIEWER), then POST to reverse("labels-add-threads", args=[label.pk]) and reverse("labels-remove-threads", args=[label.pk]) with {"thread_ids": [str(thread.id)]} and assert response.status_code == status.HTTP_200_OK and that label.threads.count() remains unchanged (0 for add, 1 if pre-added for remove).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-action-bar/index.tsx`:
- Around line 39-41: The current client-side heuristic (hasOnlyOneEditor derived
from selectedThread?.accesses and ThreadAccessRoleChoices.editor) is incorrect
because editability depends on both ThreadAccess.role and MailboxAccess.role;
update the code to stop recomputing this locally and instead consume a
backend-provided signal (e.g., selectedThread.effectiveEditorCount or
selectedThread.isLastEffectiveEditor) and use that in the canLeaveThread
calculation (replace hasOnlyOneEditor usage), adjust usages in the
canLeaveThread expression and anywhere else referencing hasOnlyOneEditor so the
UI relies on the new backend field (or boolean) that represents the actual
effective-editor count/last-editor status.
---
Duplicate comments:
In `@src/backend/core/api/serializers.py`:
- Around line 656-677: The serializer's get_abilities currently falls back to
instance.get_abilities without a mailbox scope, allowing cross-mailbox edit
flags; change get_abilities (method in the serializer) to only call
instance.get_abilities when self.context contains a mailbox_id (use mailbox_id =
self.context.get("mailbox_id") and if not present return {}), and avoid passing
None/omitting mailbox_id to instance.get_abilities; also audit all manual
ThreadSerializer(...) instantiations to ensure they pass the active mailbox_id
in the context so mailbox-scoped abilities are always provided.
---
Nitpick comments:
In `@src/backend/core/tests/api/test_labels.py`:
- Around line 665-700: The tests
test_add_threads_cross_mailbox_viewer_thread_access_forbidden and
test_remove_threads_cross_mailbox_viewer_thread_access_forbidden have misleading
names/docstrings and an inline note: the mailbox role is VIEWER
(other_mailbox.accesses.create(..., role=models.MailboxRoleChoices.VIEWER))
while the thread access is set to EDITOR (thread.accesses.create(...,
role=enums.ThreadAccessRoleChoices.EDITOR)), so update the test names,
docstrings, and the inline comment to clearly state that the mailbox role is
VIEWER and the thread has EDITOR access (e.g., rename to
*_cross_mailbox_mailbox_viewer_thread_edit_access_* or adjust docstrings to say
"mailbox role is VIEWER but thread access is EDITOR"), and correct the inline
comment that currently reads "Mailbox has edit access" to reflect the actual
setup.
- Around line 665-755: Add two tests mirroring the existing cross-mailbox VIEWER
cases but where the ThreadAccess=VIEWER is set on the label's own mailbox:
implement test_add_threads_same_mailbox_viewer_thread_access_forbidden and
test_remove_threads_same_mailbox_viewer_thread_access_forbidden that create a
Thread via ThreadFactory, call thread.accesses.create(mailbox=label.mailbox,
role=enums.ThreadAccessRoleChoices.VIEWER), then POST to
reverse("labels-add-threads", args=[label.pk]) and
reverse("labels-remove-threads", args=[label.pk]) with {"thread_ids":
[str(thread.id)]} and assert response.status_code == status.HTTP_200_OK and that
label.threads.count() remains unchanged (0 for add, 1 if pre-added for remove).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4bc7eeb5-b3ef-4fb1-8920-fb895923b9bb
⛔ Files ignored due to path filters (4)
src/frontend/package-lock.jsonis excluded by!**/package-lock.jsonsrc/frontend/src/features/api/gen/models/index.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_abilities.tsis excluded by!**/gen/**
📒 Files selected for processing (36)
src/backend/core/api/openapi.jsonsrc/backend/core/api/permissions.pysrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/flag.pysrc/backend/core/api/viewsets/label.pysrc/backend/core/api/viewsets/thread.pysrc/backend/core/api/viewsets/thread_access.pysrc/backend/core/enums.pysrc/backend/core/models.pysrc/backend/core/tests/api/test_labels.pysrc/backend/core/tests/api/test_messages_flag.pysrc/backend/core/tests/api/test_thread_access.pysrc/backend/core/tests/api/test_thread_event_permissions.pysrc/backend/core/tests/api/test_thread_split.pysrc/backend/core/tests/api/test_threads_flag.pysrc/backend/core/tests/api/test_threads_list.pysrc/backend/e2e/management/commands/e2e_demo.pysrc/e2e/src/__tests__/thread-starred-read.spec.tssrc/frontend/package.jsonsrc/frontend/public/locales/common/en-US.jsonsrc/frontend/public/locales/common/fr-FR.jsonsrc/frontend/src/features/layouts/components/thread-panel/components/thread-panel-header.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-accesses-widget/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-action-bar/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-event-input/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-labels-widget/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-message/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-actions.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-header.tsxsrc/frontend/src/features/message/use-archive.tsxsrc/frontend/src/features/message/use-flag.tsxsrc/frontend/src/features/message/use-spam.tsxsrc/frontend/src/features/message/use-starred.tsxsrc/frontend/src/features/message/use-trash.tsxsrc/frontend/src/features/providers/modal-store/controlled-modal.tsxsrc/frontend/src/hooks/use-ability.ts
✅ Files skipped from review due to trivial changes (7)
- src/frontend/src/features/layouts/components/thread-view/components/thread-event-input/index.tsx
- src/frontend/package.json
- src/frontend/src/features/layouts/components/thread-view/components/thread-message/index.tsx
- src/frontend/src/features/providers/modal-store/controlled-modal.tsx
- src/frontend/src/features/message/use-archive.tsx
- src/backend/core/api/viewsets/label.py
- src/frontend/public/locales/common/en-US.json
🚧 Files skipped from review as they are similar to previous changes (16)
- src/backend/core/tests/api/test_thread_event_permissions.py
- src/e2e/src/tests/thread-starred-read.spec.ts
- src/frontend/src/features/message/use-spam.tsx
- src/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-header.tsx
- src/frontend/src/features/layouts/components/thread-panel/components/thread-panel-header.tsx
- src/backend/core/tests/api/test_thread_split.py
- src/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-actions.tsx
- src/frontend/src/features/layouts/components/thread-view/components/thread-labels-widget/index.tsx
- src/backend/core/api/openapi.json
- src/frontend/src/features/message/use-trash.tsx
- src/backend/core/api/viewsets/thread_access.py
- src/backend/core/tests/api/test_threads_list.py
- src/backend/core/models.py
- src/frontend/public/locales/common/fr-FR.json
- src/backend/core/api/permissions.py
- src/frontend/src/features/message/use-flag.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
A user with VIEWER MailboxAccess on a shared mailbox could still mutate threads that the mailbox had EDITOR ThreadAccess to: the permission check only looked at ThreadAccess.role, never at MailboxAccess.role.
Both roles must now be satisfied (EDITOR on ThreadAccess AND a role in MAILBOX_ROLES_CAN_EDIT on MailboxAccess) for archive, spam, trash, label, split, refresh_summary and thread-event writes. Personal actions (unread, starred) intentionally stay open to any mailbox access since they only mutate the caller's own ThreadAccess row.
The rule is centralised in ThreadAccessQuerySet.editable_by(user, mailbox_id) so viewsets and permission classes share a single source of truth, and exposed to the frontend via a new Thread.abilities.edit field consumed by use-ability, which gates the matching UI controls.
Summary by CodeRabbit
New Features
Bug Fixes