-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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.
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
fix: project join for admin and members #6097
Conversation
WalkthroughThe pull request introduces modifications to the invitation and project joining processes within the application. It updates the permission checks for workspace members to allow both Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
apiserver/plane/app/views/project/invite.py (2)
Line range hint
67-85
: Add token expiration validationThe JWT token generation includes a timestamp, but there's no validation of token expiration during usage. This could lead to security issues with indefinitely valid invitation links.
Consider adding expiration validation:
+ expiry = datetime.now() + timedelta(days=7) # Configure as needed token=jwt.encode( - {"email": email, "timestamp": datetime.now().timestamp()}, + { + "email": email, + "timestamp": datetime.now().timestamp(), + "exp": expiry.timestamp() + }, settings.SECRET_KEY, algorithm="HS256", ),
Line range hint
191-236
: Add transaction management for bulk operationsThe creation of
ProjectMember
andIssueUserProperty
should be atomic to maintain data consistency.Consider wrapping the bulk operations in a transaction:
+ from django.db import transaction + + with transaction.atomic(): ProjectMember.objects.bulk_create(...) IssueUserProperty.objects.bulk_create(...)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apiserver/plane/app/views/project/invite.py
(1 hunks)
🔇 Additional comments (1)
apiserver/plane/app/views/project/invite.py (1)
139-139
: Verify role enum value comparisons
The role check has been updated to allow both ADMIN and MEMBER roles, which aligns with the PR objectives. However, let's verify the enum value comparisons are consistent across the codebase.
Consider using a more explicit role validation approach:
- if workspace_member.role not in [ROLE.ADMIN.value, ROLE.MEMBER.value]:
+ allowed_roles = {ROLE.ADMIN.value, ROLE.MEMBER.value}
+ if workspace_member.role not in allowed_roles:
This makes the allowed roles more maintainable and reusable across the codebase.
✅ Verification successful
Based on the search results, I can now generate the final response since we have found:
- The ROLE enum definition in
apiserver/plane/app/permissions/base.py
showing it's properly defined - Consistent imports of ROLE across the codebase
- The role check pattern being used consistently in other files
Role check implementation is correct
The role validation using role not in [ROLE.ADMIN.value, ROLE.MEMBER.value]
is implemented correctly and follows the codebase's patterns. The ROLE enum is properly defined in permissions/base.py with ADMIN = 20 and MEMBER = 15, and the .value comparison is the correct way to check enum values.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent role value comparisons in the codebase
# Search for role value comparisons
rg -A 2 "role.*in.*\[.*ROLE\."
# Search for direct role value comparisons
rg -A 2 "role.*==.*ROLE\."
# Search for ROLE enum definition to verify values
ast-grep --pattern 'class ROLE:
$$$
'
Length of output: 420
Script:
#!/bin/bash
# Let's search for the ROLE enum definition and its usage more broadly
# Find files containing ROLE definition
rg -l "class ROLE"
# Search for any ROLE enum usage patterns
rg -A 2 "ROLE\."
# Search for role-related validations or checks
rg -A 2 "role.*=|role.*in"
Length of output: 140866
Script:
#!/bin/bash
# Let's search for the ROLE enum definition and its usage in permissions
# Find ROLE enum definition
ast-grep --pattern 'class ROLE:
$$$
'
# Search for role-related imports and definitions
rg -A 2 "from.*ROLE|class.*ROLE"
# Search for role value comparisons specifically in permissions context
rg -A 2 "role.*in.*\[.*ROLE\.|role.*==.*ROLE\." apiserver/plane/app/permissions/
Length of output: 9002
Description
Update project join check to use enum values for ADMIN and MEMBER roles.
Summary by CodeRabbit
These changes collectively enhance the user experience by making project invitations more accessible and secure.