fix(backend): correct attachment access check for group chat members#901
fix(backend): correct attachment access check for group chat members#901joyway1978 wants to merge 1 commit intowecode-ai:mainfrom
Conversation
Fixed attachment access permission check in _ensure_attachment_access() and get_all_task_attachments() endpoints: 1. Changed MemberStatus.APPROVED to MemberStatus.APPROVED.value to ensure proper string comparison in SQLAlchemy queries. The status column stores string values, but the code was comparing against the Enum object directly, which could cause query mismatches. 2. Added ResourceMember.copied_resource_id == 0 condition to exclude share records and only consider actual group chat members. This aligns with the logic in TaskMemberService.is_member() which also excludes share records. This fixes the issue where some group chat members could not access attachments while others could, depending on how their membership status was stored.
📝 WalkthroughWalkthroughUpdated task membership verification logic in attachment access control. Modified two functions to compare membership status against its enumeration value and filter out copied resource records, affecting both individual attachment access checks and task-wide attachment listing operations. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/app/api/endpoints/adapter/attachments.py (1)
95-103: Promote the non-copy sentinel to a named constant.
0is now duplicated across both permission checks. Naming it once makes the rule explicit and reduces drift if this membership logic changes again.Small cleanup
ATTACHMENT_PREVIEW_TEXT_LIMIT = 4000 +RESOURCE_MEMBER_NOT_COPIED = 0 ... - ResourceMember.copied_resource_id == 0, + ResourceMember.copied_resource_id + == RESOURCE_MEMBER_NOT_COPIED, ... - ResourceMember.copied_resource_id == 0, + ResourceMember.copied_resource_id == RESOURCE_MEMBER_NOT_COPIED,As per coding guidelines, "Python: Extract magic numbers to constants".
Also applies to: 686-694
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/endpoints/adapter/attachments.py` around lines 95 - 103, Replace the magic literal 0 used in membership checks with a named constant: add a module-level constant such as NON_COPY_RESOURCE_ID = 0 (or COPIED_RESOURCE_NONE = 0) near the other constants/imports, then update the ResourceMember.copied_resource_id comparisons (e.g., in the task_member query and the other check at the 686-694 range) to use that constant instead of the literal 0 so the intent is explicit and changes are centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/api/endpoints/adapter/attachments.py`:
- Around line 95-103: The query using ResourceMember.status ==
MemberStatus.APPROVED.value should be changed to accept both uppercase and
lowercase statuses (e.g., "APPROVED" and "approved") like the variant-list
pattern in base_service.py; update the filter in the task_member query (and the
similar filter at lines 686-694) to check ResourceMember.status against both
variants (or use a case-insensitive comparison) while keeping the other filters
(ResourceMember.resource_type == ResourceType.TASK, resource_id ==
subtask.task_id, user_id == current_user.id, copied_resource_id == 0) unchanged
so approved members stored in either case are included.
---
Nitpick comments:
In `@backend/app/api/endpoints/adapter/attachments.py`:
- Around line 95-103: Replace the magic literal 0 used in membership checks with
a named constant: add a module-level constant such as NON_COPY_RESOURCE_ID = 0
(or COPIED_RESOURCE_NONE = 0) near the other constants/imports, then update the
ResourceMember.copied_resource_id comparisons (e.g., in the task_member query
and the other check at the 686-694 range) to use that constant instead of the
literal 0 so the intent is explicit and changes are centralized.
🪄 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: 1aaa8800-95be-4a1c-bad9-b20cc7e783d7
📒 Files selected for processing (1)
backend/app/api/endpoints/adapter/attachments.py
| # Exclude share records (copied_resource_id > 0), only consider actual group chat members | ||
| task_member = ( | ||
| db.query(ResourceMember) | ||
| .filter( | ||
| ResourceMember.resource_type == ResourceType.TASK, | ||
| ResourceMember.resource_id == subtask.task_id, | ||
| ResourceMember.user_id == current_user.id, | ||
| ResourceMember.status == MemberStatus.APPROVED, | ||
| ResourceMember.status == MemberStatus.APPROVED.value, | ||
| ResourceMember.copied_resource_id == 0, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the referenced documentation in base_service.py
cat -n backend/app/services/share/base_service.py | sed -n '620,650p'Repository: wecode-ai/Wegent
Length of output: 1740
🏁 Script executed:
# Search for all status value assignments in the codebase
rg "status\s*=\s*['\"]" backend/app/models/ -A 1 -B 1 | head -50Repository: wecode-ai/Wegent
Length of output: 42
🏁 Script executed:
# Check for any mixed-case status values (not all-uppercase or all-lowercase)
rg "[Pp][Ee][Nn][Dd][Ii][Nn][Gg]|[Aa][Pp][Pp][Rr][Oo][Vv][Ee][Dd]|[Rr][Ee][Jj][Ee][Cc][Tt][Ee][Dd]" backend/app --type py | grep -i statusRepository: wecode-ai/Wegent
Length of output: 27208
🏁 Script executed:
# Check ResourceMember model definition for status enum values
cat -n backend/app/models/resource_member.py | sed -n '40,60p'Repository: wecode-ai/Wegent
Length of output: 767
🏁 Script executed:
# Search database migration files for status value patterns
fd -t f "\.py" backend/alembic/versions/ -x grep -l status {} \; | head -5Repository: wecode-ai/Wegent
Length of output: 216
🏁 Script executed:
# Check the exact implementation of approved_status_variants in base_service.py
rg -A 5 "approved_status_variants" backend/app/services/share/base_service.pyRepository: wecode-ai/Wegent
Length of output: 1357
🏁 Script executed:
# Check the attachments.py file to see both changed lines and their context
cat -n backend/app/api/endpoints/adapter/attachments.py | sed -n '85,110p'Repository: wecode-ai/Wegent
Length of output: 1396
🏁 Script executed:
# Check the second range in attachments.py (around line 686-694)
cat -n backend/app/api/endpoints/adapter/attachments.py | sed -n '680,700p'Repository: wecode-ai/Wegent
Length of output: 970
🏁 Script executed:
# Search for when variant-list pattern was introduced - check if it's a recent defensive addition
git log --oneline -S "approved_status_variants" backend/app/services/share/base_service.py | head -5Repository: wecode-ai/Wegent
Length of output: 137
Status comparison needs to handle both uppercase and lowercase variants.
The code switched from a variant-list pattern (used elsewhere in base_service.py) to a direct equality check against MemberStatus.APPROVED.value ("approved"). Per the existing codebase pattern, ResourceMember.status can be stored as either "APPROVED" or "approved", so this query will silently reject members whose status is uppercase. Restore parity with the variant-list approach used in base_service.py:
Suggested fix
+from app.models.resource_member import MemberStatus, ResourceMember
+from app.models.share_link import ResourceType
+
+# At lines 95-106
+approved_status_variants = [
+ MemberStatus.APPROVED.value,
+ MemberStatus.APPROVED.value.upper(),
+]
task_member = (
db.query(ResourceMember)
.filter(
ResourceMember.resource_type == ResourceType.TASK,
ResourceMember.resource_id == subtask.task_id,
ResourceMember.user_id == current_user.id,
- ResourceMember.status == MemberStatus.APPROVED.value,
+ ResourceMember.status.in_(approved_status_variants),
ResourceMember.copied_resource_id == 0,
)
.first()
)
+# At lines 687-698 (same pattern)
+approved_status_variants = [
+ MemberStatus.APPROVED.value,
+ MemberStatus.APPROVED.value.upper(),
+]
is_member = (
db.query(ResourceMember)
.filter(
ResourceMember.resource_type == ResourceType.TASK,
ResourceMember.resource_id == task_id,
ResourceMember.user_id == current_user.id,
- ResourceMember.status == MemberStatus.APPROVED.value,
+ ResourceMember.status.in_(approved_status_variants),
ResourceMember.copied_resource_id == 0,
)
.first()
is not None
)Also applies to: 686-694
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/api/endpoints/adapter/attachments.py` around lines 95 - 103, The
query using ResourceMember.status == MemberStatus.APPROVED.value should be
changed to accept both uppercase and lowercase statuses (e.g., "APPROVED" and
"approved") like the variant-list pattern in base_service.py; update the filter
in the task_member query (and the similar filter at lines 686-694) to check
ResourceMember.status against both variants (or use a case-insensitive
comparison) while keeping the other filters (ResourceMember.resource_type ==
ResourceType.TASK, resource_id == subtask.task_id, user_id == current_user.id,
copied_resource_id == 0) unchanged so approved members stored in either case are
included.
1 participant
Fixed attachment access permission check in _ensure_attachment_access() and get_all_task_attachments() endpoints:
Changed MemberStatus.APPROVED to MemberStatus.APPROVED.value to ensure proper string comparison in SQLAlchemy queries. The status column stores string values, but the code was comparing against the Enum object directly, which could cause query mismatches.
Added ResourceMember.copied_resource_id == 0 condition to exclude share records and only consider actual group chat members. This aligns with the logic in TaskMemberService.is_member() which also excludes share records.
This fixes the issue where some group chat members could not access attachments while others could, depending on how their membership status was stored.
Summary by CodeRabbit