Project report notification subscription public permission fix #213
Conversation
…ctSchema in participant.py
WalkthroughA boolean field named Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant PublicProjectSchema
Client ->> API: Request project data
API ->> PublicProjectSchema: Populate fields (including is_project_notification_subscription_allowed)
PublicProjectSchema -->> API: Return project data with new field
API -->> Client: Respond with project data (now includes is_project_notification_subscription_allowed)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
echo/server/dembrane/api/participant.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ussaama
PR: Dembrane/echo#205
File: echo/frontend/src/lib/query.ts:1444-1506
Timestamp: 2025-07-10T12:48:20.663Z
Learning: ussaama prefers string concatenation over template literals for simple cases where readability is clearer, even when linting tools suggest template literals. Human readability takes precedence over strict linting rules in straightforward concatenation scenarios.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci-check-server
|
|
||
| is_conversation_allowed: bool | ||
| is_get_reply_enabled: bool | ||
| is_project_notification_subscription_allowed: bool |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
LGTM - clean addition, but needs authorization integration!
The new field follows proper naming conventions and is positioned logically with other permission fields. However, the notification subscription endpoints (lines 261-322) don't currently check this permission field.
Consider adding authorization checks to the notification endpoints:
@ParticipantRouter.post("/report/subscribe")
async def subscribe_notifications(data: NotificationSubscriptionRequest) -> dict:
+ # Check if project allows notification subscriptions
+ try:
+ project = project_service.get_by_id_or_raise(data.project_id)
+ if not project.get("is_project_notification_subscription_allowed", False):
+ raise HTTPException(status_code=403, detail="Project notification subscription not allowed")
+ except ProjectNotFoundException as e:
+ raise HTTPException(status_code=404, detail="Project not found") from e
+
"""
Subscribe multiple users to project notifications.Also verify that the service layer populates this field correctly.
🏁 Script executed:
#!/bin/bash
# Check if the service layer populates the new field
rg -A 5 -B 5 "is_project_notification_subscription_allowed" --type pyLength of output: 907
Ensure Notification Subscription Authorization is Enforced
The new is_project_notification_subscription_allowed: bool field is added to the public project schema but isn’t used in any of the notification subscription endpoints (participant.py around lines 261–322). We need to:
• Enforce this permission in both the /report/subscribe and /report/unsubscribe endpoints by:
– Loading the project (e.g. via project_service.get_by_id_or_raise)
– Checking project.is_project_notification_subscription_allowed
– Throwing HTTPException(status_code=403, ...) if the flag is false
• Update the service layer (where projects are fetched) so that the is_project_notification_subscription_allowed field is populated on each project returned.
Suggested diff for the subscribe endpoint:
@ParticipantRouter.post("/report/subscribe")
async def subscribe_notifications(data: NotificationSubscriptionRequest) -> dict:
+ # Enforce project-level notification subscription permission
+ try:
+ project = project_service.get_by_id_or_raise(data.project_id)
+ except ProjectNotFoundException as e:
+ raise HTTPException(status_code=404, detail="Project not found") from e
+
+ if not project.get("is_project_notification_subscription_allowed", False):
+ raise HTTPException(
+ status_code=403,
+ detail="Project notification subscription is not allowed"
+ )
"""
Subscribe multiple users to project notifications.
…Repeat similar logic in the unsubscribe endpoint. Finally, verify in the service implementation that is_project_notification_subscription_allowed is included in the project payload returned to the API layer.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| is_project_notification_subscription_allowed: bool | |
| @ParticipantRouter.post("/report/subscribe") | |
| async def subscribe_notifications(data: NotificationSubscriptionRequest) -> dict: | |
| # Enforce project-level notification subscription permission | |
| try: | |
| project = project_service.get_by_id_or_raise(data.project_id) | |
| except ProjectNotFoundException as e: | |
| raise HTTPException(status_code=404, detail="Project not found") from e | |
| if not project.get("is_project_notification_subscription_allowed", False): | |
| raise HTTPException( | |
| status_code=403, | |
| detail="Project notification subscription is not allowed" | |
| ) | |
| """ | |
| Subscribe multiple users to project notifications. | |
| … |
🤖 Prompt for AI Agents
In echo/server/dembrane/api/participant.py around lines 261 to 322, the new
field is_project_notification_subscription_allowed is added to the project
schema but not enforced in the /report/subscribe and /report/unsubscribe
endpoints. Fix this by loading the project using
project_service.get_by_id_or_raise in both endpoints, checking the project's
is_project_notification_subscription_allowed flag, and raising an HTTPException
with status 403 if the flag is false. Also, update the service layer to ensure
this field is populated on each project returned to the API layer.
ussaama
deleted the
report-notification-subscription-public-permission-fix
branch
Add is_project_notification_subscription_allowed field to PublicProjectSchema in participant.py
Summary by CodeRabbit