Skip to content

fix(backend): correct attachment access check for group chat members #901

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
joyway1978 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(backend): correct attachment access check for group chat members #901

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions backend/app/api/endpoints/adapter/attachments.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,13 +92,15 @@ def _ensure_attachment_access(db: Session, context, current_user: User) -> None:
has_access = True
else:
# Check if user is a task member using ResourceMember
# 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,
Comment on lines +95 to +103
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 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 -50

Repository: 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 status

Repository: 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 -5

Repository: 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.py

Repository: 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 -5

Repository: 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.

)
.first()
)
Expand Down Expand Up @@ -681,13 +683,15 @@ async def get_all_task_attachments(
from app.models.share_link import ResourceType

is_owner = task.user_id == current_user.id
# Exclude share records (copied_resource_id > 0), only consider actual group chat members
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,
ResourceMember.status == MemberStatus.APPROVED.value,
ResourceMember.copied_resource_id == 0,
)
.first()
is not None
Expand Down
Loading