feat: lock down private workflows for non-workspace members #725
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis update introduces user authorization logic to the Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ 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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
daras_ai_v2/base.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (python)
- GitHub Check: test (3.10.12, 1.8.3)
🔇 Additional comments (2)
daras_ai_v2/base.py (2)
7-7: LGTM - Import addition is correct.The
dedentimport is appropriately added for text formatting in the unauthorized message.
398-402: Excellent security implementation with early authorization check.The early authorization check with immediate return is a best practice for security. This prevents unauthorized users from accessing any workflow data or functionality.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app_users/models.py(2 hunks)daras_ai_v2/base.py(6 hunks)daras_ai_v2/meta_content.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- daras_ai_v2/base.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: nikochiko
PR: GooeyAI/gooey-server#725
File: daras_ai_v2/base.py:343-376
Timestamp: 2025-07-04T07:34:01.107Z
Learning: In the Gooey.AI codebase, `pr.public_access` field is guaranteed to be non-null by database constraints, so null checks are not needed when accessing this field or converting it to WorkflowAccessLevel.
Learnt from: nikochiko
PR: GooeyAI/gooey-server#725
File: daras_ai_v2/base.py:343-376
Timestamp: 2025-07-04T07:34:01.107Z
Learning: In the Gooey.AI codebase, `pr.public_access` field is guaranteed to be non-null by database constraints (IntegerField with default value), so null checks are not needed when accessing this field or converting it to WorkflowAccessLevel.
🧬 Code Graph Analysis (1)
app_users/models.py (1)
workspaces/models.py (2)
Workspace(105-449)WorkspaceMembership(452-535)
⏰ 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). (4)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (actions)
- GitHub Check: Analyze (python)
- GitHub Check: test (3.10.12, 1.8.3)
🔇 Additional comments (3)
daras_ai_v2/meta_content.py (1)
220-238: LGTM! Excellent security enhancement for search engine indexing.The authorization check for anonymous users is well-implemented and aligns perfectly with the PR objectives. By checking
page.is_user_authorized(None)first, the function ensures that private workflows are not indexed or followed by search engines, which is crucial for maintaining access control consistency.The early return pattern with
nofollowandnoindexflags set toTruefor unauthorized pages is a solid defensive approach that prevents search engines from accessing content that anonymous users cannot view.app_users/models.py (2)
19-19: LGTM - Import addition supports new functionality.The addition of
WorkspaceMembershipto the import is necessary for the newcached_membershipsproperty.
243-243: LGTM - Refactoring supports membership-based authorization.The refactoring to derive workspaces from
cached_membershipsis logical and enables access to both workspace and membership data needed for authorization checks.
app_users/models.py
Outdated
| @cached_property | ||
| def cached_memberships(self) -> dict[int, "WorkspaceMembership"]: | ||
| from workspaces.models import WorkspaceMembership | ||
|
|
||
| memberships = ( | ||
| WorkspaceMembership.objects.select_related("workspace") | ||
| .filter(user=self, workspace__deleted__isnull=True) | ||
| .order_by("-workspace__is_personal", "-workspace__created_at") | ||
| ) | ||
| if not memberships: | ||
| self.get_or_create_personal_workspace() | ||
| memberships = memberships.all() # refresh | ||
| assert memberships | ||
|
|
||
| return list( | ||
| Workspace.objects.filter( | ||
| memberships__user=self, memberships__deleted__isnull=True | ||
| ).order_by("-is_personal", "-created_at") | ||
| ) or [self.get_or_create_personal_workspace()[0]] | ||
| return {m.workspace.id: m for m in memberships} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize queryset refresh pattern and improve error handling.
The implementation has a few areas for improvement:
- Inefficient queryset refresh: The
memberships.all()call forces unnecessary queryset evaluation just to refresh the queryset. - Production-unsafe assertion: The
assertstatement could cause issues in production if the assumption fails. - Ordering consideration: The ordering by
workspace__created_atsorts by workspace creation date, not membership join date - verify this is intended.
Apply this diff to optimize the queryset handling and improve error handling:
- if not memberships:
- self.get_or_create_personal_workspace()
- memberships = memberships.all() # refresh
- assert memberships
+ if not memberships.exists():
+ self.get_or_create_personal_workspace()
+ # Re-query to include the newly created membership
+ memberships = (
+ WorkspaceMembership.objects.select_related("workspace")
+ .filter(user=self, workspace__deleted__isnull=True)
+ .order_by("-workspace__is_personal", "-workspace__created_at")
+ )
+ if not memberships.exists():
+ raise ValueError("Failed to create personal workspace membership")This change:
- Uses
exists()to check membership presence without forcing evaluation - Re-queries instead of relying on queryset refresh
- Replaces
assertwith a proper exception that provides better error context
📝 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.
| @cached_property | |
| def cached_memberships(self) -> dict[int, "WorkspaceMembership"]: | |
| from workspaces.models import WorkspaceMembership | |
| memberships = ( | |
| WorkspaceMembership.objects.select_related("workspace") | |
| .filter(user=self, workspace__deleted__isnull=True) | |
| .order_by("-workspace__is_personal", "-workspace__created_at") | |
| ) | |
| if not memberships: | |
| self.get_or_create_personal_workspace() | |
| memberships = memberships.all() # refresh | |
| assert memberships | |
| return list( | |
| Workspace.objects.filter( | |
| memberships__user=self, memberships__deleted__isnull=True | |
| ).order_by("-is_personal", "-created_at") | |
| ) or [self.get_or_create_personal_workspace()[0]] | |
| return {m.workspace.id: m for m in memberships} | |
| @cached_property | |
| def cached_memberships(self) -> dict[int, "WorkspaceMembership"]: | |
| from workspaces.models import WorkspaceMembership | |
| memberships = ( | |
| WorkspaceMembership.objects.select_related("workspace") | |
| .filter(user=self, workspace__deleted__isnull=True) | |
| .order_by("-workspace__is_personal", "-workspace__created_at") | |
| ) | |
| if not memberships.exists(): | |
| self.get_or_create_personal_workspace() | |
| # Re-query to include the newly created membership | |
| memberships = ( | |
| WorkspaceMembership.objects.select_related("workspace") | |
| .filter(user=self, workspace__deleted__isnull=True) | |
| .order_by("-workspace__is_personal", "-workspace__created_at") | |
| ) | |
| if not memberships.exists(): | |
| raise ValueError("Failed to create personal workspace membership") | |
| return {m.workspace.id: m for m in memberships} |
🤖 Prompt for AI Agents
In app_users/models.py around lines 245 to 259, the current method inefficiently
refreshes the queryset by calling memberships.all() and uses an assert statement
that is unsafe for production. To fix this, replace the check for memberships
with memberships.exists() to avoid unnecessary evaluation, then re-query the
memberships after creating the personal workspace instead of refreshing the old
queryset. Also, replace the assert statement with a proper exception that
clearly indicates the failure to retrieve memberships, providing better error
handling and context.
daras_ai_v2/base.py
Outdated
|
|
||
| membership = user and user.cached_memberships.get(pr.workspace_id, None) | ||
| role = membership and WorkspaceRole(membership.role) | ||
| match pr.workspace_access, role: |
There was a problem hiding this comment.
i believe all members should be able to see any run in their workspace including WorkflowAccessLevel.VIEW_ONLY ones. especially because we also dont really have a mechanism for an admin to grant access to a member
|
done! @devxpy |
Q/A checklist
How to check import time?
You can visualize this using tuna:
To measure import time for a specific library:
To reduce import times, import libraries that take a long time inside the functions that use them instead of at the top of the file:
Legal Boilerplate
Look, I get it. The entity doing business as “Gooey.AI” and/or “Dara.network” was incorporated in the State of Delaware in 2020 as Dara Network Inc. and is gonna need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Dara Network Inc can use, modify, copy, and redistribute my contributions, under its choice of terms.