Readonly User #637
Readonly User #637
Conversation
WalkthroughThis pull request introduces a new "Read Only" user role across the application, enhancing workspace user management and permission controls. The changes span multiple components in both the API and client-side code, adding a new Changes
Sequence DiagramsequenceDiagram
participant User
participant WorkspaceController
participant FormPolicy
participant Workspace
User->>WorkspaceController: Request to add/update user
WorkspaceController->>Workspace: Check user role
Workspace-->>WorkspaceController: Return role status
WorkspaceController->>FormPolicy: Authorize actions
FormPolicy->>FormPolicy: Check read-only status
FormPolicy-->>WorkspaceController: Return authorization result
WorkspaceController-->>User: Respond with allowed/restricted actions
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
![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
🧹 Outside diff range and nitpick comments (11)
client/components/pages/auth/components/RegisterForm.vue (2)
Line range hint
159-165: Add validation rules for the required hear_about_us fieldWhile the hear_about_us field is marked as required in the template, there's no corresponding validation in the form data structure. Consider adding validation rules to ensure data integrity.
form: useForm({ name: "", email: "", hear_about_us: "", password: "", password_confirmation: "", agree_terms: false, + rules: { + hear_about_us: { required: true } + } }),
203-206: Consider extracting invite-related logicThe invite-related logic, including setting the hear_about_us field, could be extracted into a separate method for better maintainability and readability.
mounted() { + this.handleAppSumoLicense() + this.handleInviteFlow() }, methods: { + handleAppSumoLicense() { + if ( + this.$route.query.appsumo_license !== undefined && + this.$route.query.appsumo_license + ) { + this.form.appsumo_license = this.$route.query.appsumo_license + } + }, + handleInviteFlow() { + if (this.$route.query?.invite_token) { + if (this.$route.query?.email) { + this.form.email = this.$route.query?.email + this.form.hear_about_us = 'invite' + this.disableEmail = true + } + this.form.invite_token = this.$route.query?.invite_token + } + },api/app/Policies/TemplatePolicy.php (1)
20-20: Add unit tests for readonly user permissionsThe readonly permission checks have been added consistently across create, update, and delete methods. Please ensure these new conditions are covered by unit tests.
Would you like me to help generate unit tests for these scenarios:
- Readonly user attempting to create a template
- Readonly user attempting to update their own template
- Readonly admin attempting to delete a template
Also applies to: 30-30, 40-40
api/app/Policies/FormPolicy.php (1)
40-40: Consider adding integration testsThe interaction between readonly status and form ownership creates multiple permission scenarios that should be tested thoroughly.
Would you like me to help generate integration tests covering:
- Readonly user attempting various form operations
- Form ownership checks combined with readonly status
- Edge cases like null users or deleted forms
Also applies to: 50-50, 60-60, 70-70, 80-80
client/components/pages/admin/EditWorkSpaceUser.vue (1)
21-22: Consider adding role change confirmationWhile the role options are correctly implemented, changing a user's role to "Read Only" could impact their access to existing resources. Consider adding a confirmation dialog when downgrading user permissions.
Example implementation:
const updateUserRole = () => { + if (props.user.pivot.role !== userNewRole.value) { + if (!confirm(`Are you sure you want to change ${props.user.name}'s role from ${props.user.pivot.role} to ${userNewRole.value}?`)) { + return + } + } updatingUserRoleState.value = true // ... rest of the function }client/pages/forms/[slug]/show/share.vue (1)
6-6: Consider adding explanatory message for read-only usersWhile hiding the regenerate-form-link component for read-only users is correct, consider showing a message explaining why the feature is unavailable.
Example implementation:
- <regenerate-form-link - v-if="!user.is_readonly" - class="sm:w-1/2 flex" - :form="props.form" - /> + <div class="sm:w-1/2 flex"> + <regenerate-form-link + v-if="!user.is_readonly" + :form="props.form" + /> + <p v-else class="text-gray-500 text-sm"> + Form link regeneration is not available for read-only users + </p> + </div>api/app/Http/Controllers/WorkspaceUserController.php (1)
Line range hint
89-96: Add safety checks for role changes.The current implementation could potentially leave a workspace without admins if:
- The last admin is changed to read-only
- All users are set to read-only roles
Consider adding these safety checks:
public function updateUserRole(Request $request, $workspaceId, $userId) { $workspace = Workspace::findOrFail($workspaceId); $user = User::findOrFail($userId); $this->authorize('adminAction', $workspace); $this->validate($request, [ 'role' => 'required|in:' . implode(',', User::ROLES), ]); + // Prevent removing the last admin + if ($user->workspaceRole($workspace) === 'admin' && $request->role !== 'admin') { + $adminCount = $workspace->users() + ->wherePivot('role', 'admin') + ->count(); + if ($adminCount <= 1) { + return $this->error([ + 'message' => 'Cannot change role. Workspace must have at least one admin.' + ]); + } + } + $workspace->users()->sync([ $user->id => [ 'role' => $request->role, ], ], false);client/components/pages/forms/show/ExtraMenu.vue (1)
Line range hint
134-173: Consider refactoring for better maintainability.While the implementation correctly handles read-only permissions, the nested array structure with multiple spread operators makes the code harder to maintain.
Consider refactoring to improve readability:
const items = computed(() => { + const getMainPageItems = () => props.isMainPage ? [ + { + label: 'View form', + icon: 'i-heroicons-eye-16-solid', + click: () => { + if (props.form.visibility === 'draft') { + showDraftFormWarningNotification() + } else { + window.open(props.form.share_url, '_blank') + } + } + }, + { + label: 'Copy link to share', + icon: 'i-heroicons-clipboard-document-check-20-solid', + click: () => copyLink() + } + ] : [] + + const getEditItems = () => !user.value.is_readonly ? [ + ...(props.isMainPage ? [{ + label: 'Edit', + icon: 'i-heroicons-pencil-square-20-solid', + to: { name: 'forms-slug-edit', params: { slug: props.form.slug } } + }] : []), + { + label: 'Duplicate form', + icon: 'i-heroicons-document-duplicate-20-solid', + click: () => duplicateForm() + } + ] : [] + + const getManageItems = () => !user.value.is_readonly ? [ + ...(props.isMainPage ? [] : [{ + label: 'Create Template', + icon: 'i-heroicons-document-plus-20-solid', + click: () => { showFormTemplateModal.value = true } + }]), + { + label: 'Change workspace', + icon: 'i-heroicons-building-office-2-20-solid', + click: () => { showFormWorkspaceModal.value = true } + } + ] : [] + + const getDeleteItems = () => !user.value.is_readonly ? [{ + label: 'Delete form', + icon: 'i-heroicons-trash-20-solid', + click: () => { showDeleteFormModal.value = true }, + class: 'text-red-800 hover:bg-red-50 hover:text-red-600 group', + iconClass: 'text-red-900 group-hover:text-red-800' + }] : [] + + return [ + getMainPageItems(), + getEditItems(), + getManageItems(), + getDeleteItems() + ].filter(group => group.length > 0) })api/app/Models/User.php (1)
26-30: Consider adding role validationThe
ROLESarray provides a single source of truth for valid roles, which is good practice. Consider adding validation to ensure only these roles can be assigned to users.+ protected static function boot() + { + parent::boot(); + static::saving(function ($user) { + $workspaces = $user->workspaces; + foreach ($workspaces as $workspace) { + if (!in_array($workspace->pivot->role, self::ROLES)) { + throw new \InvalidArgumentException('Invalid role assigned'); + } + } + }); + }client/pages/home.vue (1)
11-11: Consider adding tooltip for disabled stateWhile hiding the button for read-only users is correct, consider showing a disabled button with a tooltip explaining why it's not available.
-v-if="!user.is_readonly" +v-bind:class="{ 'opacity-50 cursor-not-allowed': user.is_readonly }" +v-bind:disabled="user.is_readonly" +v-tooltip="user.is_readonly ? 'Read-only users cannot create forms' : ''"client/pages/forms/[slug]/show.vue (1)
286-292: Consider improving readability of the conditional tabs logicWhile the spread operator implementation is functional, consider these improvements for better readability and maintainability:
- ...user.value.is_readonly ? [] : [ - { - name: "Integrations", - route: "forms-slug-show-integrations", - params: { 'slug': slug } - }, - ], + // Only show Integrations tab for non-readonly users + ...(!user.value.is_readonly ? [{ + name: "Integrations", + route: "forms-slug-show-integrations", + params: { slug } // Use object shorthand + }] : []),The changes:
- Invert the condition for better readability
- Add a comment explaining the conditional logic
- Use object shorthand for the params object
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
api/app/Http/Controllers/WorkspaceUserController.php(3 hunks)api/app/Http/Resources/UserResource.php(1 hunks)api/app/Models/User.php(2 hunks)api/app/Policies/FormPolicy.php(5 hunks)api/app/Policies/TemplatePolicy.php(3 hunks)client/components/open/tables/OpenTable.vue(3 hunks)client/components/pages/admin/AddUserToWorkspace.vue(1 hunks)client/components/pages/admin/EditWorkSpaceUser.vue(1 hunks)client/components/pages/auth/components/RegisterForm.vue(3 hunks)client/components/pages/forms/show/ExtraMenu.vue(3 hunks)client/pages/forms/[slug]/show.vue(4 hunks)client/pages/forms/[slug]/show/share.vue(2 hunks)client/pages/home.vue(2 hunks)client/pages/settings.vue(1 hunks)
🔇 Additional comments (17)
client/components/pages/auth/components/RegisterForm.vue (2)
Line range hint 27-35: LGTM: Conditional rendering of hear_about_us field
The conditional rendering of the hear_about_us field is well implemented. It's appropriately hidden when users come through invites, maintaining a clean user experience.
Line range hint 1-1: Verify completeness of read-only user implementation
While this file contains changes for tracking user sources, it doesn't include any changes related to the read-only user functionality mentioned in the PR title. Please verify if additional changes are needed in this component to support read-only users.
api/app/Policies/TemplatePolicy.php (1)
20-20: Verify the implementation of is_readonly attribute
The readonly checks are correctly implemented across all methods. However, we should verify the implementation of the is_readonly attribute in the User model.
Also applies to: 30-30, 40-40
✅ Verification successful
is_readonly implementation is correctly implemented but through a different mechanism
The is_readonly check is implemented through the workspace role system:
- The User model defines
ROLE_READONLY = 'readonly'as a constant - The
getIsReadonlyAttribute()method checks if the user has any workspace with the 'readonly' role - The role is stored in the pivot table between users and workspaces
- The implementation is consistent with the policy checks
The readonly checks in TemplatePolicy are valid and work as intended with this implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the is_readonly attribute implementation
ast-grep --pattern 'function getIsReadonlyAttribute'
Length of output: 54
Script:
#!/bin/bash
# Let's search for is_readonly in User model and related files using different approaches
echo "=== Searching for is_readonly attribute or property ==="
rg -l "is_readonly" --type php
echo -e "\n=== Checking User model implementation ==="
fd -e php -g "*User.php" --exec cat {}
echo -e "\n=== Checking database migrations for is_readonly column ==="
fd -e php -g "*_users_table.php" --exec cat {}
Length of output: 9277
Script:
#!/bin/bash
# Let's check the workspace_user table migration and relationship since is_readonly is based on workspace role
echo "=== Checking workspace_user table migration ==="
fd -e php -g "*workspace_user*" --exec cat {}
echo -e "\n=== Checking Workspace model ==="
fd -e php -g "*Workspace.php" --exec cat {}
Length of output: 5971
api/app/Http/Resources/UserResource.php (1)
22-22: Verify frontend handling of is_readonly field
The field is correctly added to the personal data array. Let's verify its usage in frontend components.
✅ Verification successful
Frontend correctly handles the is_readonly field
The is_readonly field is properly utilized across multiple frontend components to control user permissions and UI elements:
- Used in conditional rendering (
v-if) to hide actions for readonly users in various form views - Applied in menu generation logic to filter available actions
- Integrated with table components to control interactive features
- Consistently used in settings and home pages for permission-based UI adjustments
The implementation aligns with the backend policies where is_readonly restricts form and template operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for is_readonly usage in frontend components
rg "is_readonly" --type vue
Length of output: 55
Script:
#!/bin/bash
# Search for is_readonly usage in frontend files, trying different extensions
rg "is_readonly" --type js --type ts --type jsx --type tsx
# Also search in any file within resources/js directory which typically contains frontend code
fd . "resources/js" -t f -x rg "is_readonly" {}
Length of output: 234
Script:
#!/bin/bash
# First, let's find frontend-related directories and files
fd -t d "resources"
# Then search for any file containing is_readonly
rg "is_readonly" --type-list # List available file types
rg "is_readonly" # Search in all files
Length of output: 2025
api/app/Policies/FormPolicy.php (1)
50-50: Verify the implementation of ownsForm method
The readonly checks are consistently combined with ownsForm checks. Let's verify the implementation of the ownsForm method.
Also applies to: 60-60, 70-70, 80-80
✅ Verification successful
ownsForm implementation is correct and consistent
The ownsForm method in User model is properly implemented to check workspace ownership through the relationship. The method verifies if the user has access to the workspace that owns the form by checking the existence of the workspace relationship. This implementation is correctly used in combination with the readonly checks throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for ownsForm method implementation
ast-grep --pattern 'function ownsForm'
Length of output: 40
Script:
#!/bin/bash
# Search for ownsForm method using ripgrep for better coverage
rg "ownsForm" -A 3
Length of output: 1907
client/components/pages/admin/AddUserToWorkspace.vue (1)
51-52: LGTM: Role options addition looks correct
The addition of the "Read Only" role option is properly implemented and maintains consistency with existing role options.
Let's verify the role value matches the backend expectations:
✅ Verification successful
LGTM: The "readonly" role value matches backend expectations
The backend code confirms that "readonly" is a valid role value:
- It's defined as a constant
ROLE_READONLY = 'readonly'in the User model - It's included in the
ROLESarray used for validation - The WorkspaceUserController validates role values against this array
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for role validation or constants in the backend
rg -t php "readonly" --glob "app/**"
Length of output: 185
Script:
#!/bin/bash
# Search for role validation or constants in the backend with broader scope
rg -t php "readonly|READONLY" app/
# Search for role-related constants or enums
rg -t php "class.*Role|enum.*Role|const.*ROLE" app/
# Search for role validation in API endpoints or controllers
rg -t php "role.*=.*|->role|role:" app/
Length of output: 267
Script:
#!/bin/bash
# First, let's find the backend directory structure
fd --type directory api backend server
# Search for PHP files in the repository
fd -e php
# Look for role-related files in any directory
rg -g "!node_modules" "readonly|READONLY"
# Search for role validation in API routes or controllers
rg -g "!node_modules" "role.*=.*|->role|role:"
Length of output: 87551
Script:
#!/bin/bash
# Search for role-related code in API routes and controllers
fd -e php -e js -e vue . | xargs rg -l "role|ROLE"
# Search for role validation in API endpoints
fd -e php "Controller.php" api/app/Http/Controllers | xargs rg "role"
# Search for role-related models and migrations
fd -e php "Migration.php" api/database/migrations | xargs rg "role"
# Search for role-related tests
fd -e php "Test.php" api/tests | xargs rg "role"
Length of output: 72048
Script:
#!/bin/bash
# Search for role constants and validation in User model
rg -A 5 "ROLE|role" api/app/Models/User.php
# Search for role validation in UserWorkspace model
rg -A 5 "ROLE|role" api/app/Models/UserWorkspace.php
# Search for role-related code in workspace controllers
rg -A 5 "role.*=.*|->role|role:" api/app/Http/Controllers/WorkspaceUserController.php
Length of output: 1723
client/pages/forms/[slug]/show/share.vue (1)
58-59: LGTM: Auth store integration looks correct
The auth store integration and computed property for user state are properly implemented.
client/pages/settings.vue (1)
65-78: LGTM! Clean implementation of conditional tab rendering.
The implementation elegantly handles the conditional rendering of sensitive tabs for read-only users while maintaining the existing tab management logic.
Let's verify the consistency of read-only checks across the frontend:
✅ Verification successful
The read-only check implementation is consistent across the frontend
The verification shows that is_readonly checks are consistently implemented across the codebase:
- All sensitive UI elements are properly guarded with
user.is_readonlyoruser.value.is_readonlychecks - The pattern is uniformly applied in:
- Navigation menus/tabs (settings.vue, forms/show components)
- Action buttons (home.vue, forms/show/share.vue)
- Table operations (OpenTable.vue)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other UI components that might need similar read-only checks
rg "is_readonly" --type vue
Length of output: 55
Script:
#!/bin/bash
# Search for files with .vue extension and then grep for is_readonly
fd '\.vue$' | xargs rg "is_readonly"
Length of output: 634
api/app/Http/Controllers/WorkspaceUserController.php (1)
34-34: LGTM! Dynamic role validation is more maintainable.
Using User::ROLES for validation is safer and automatically stays in sync with role definitions.
Let's verify the role definitions and their usage:
Also applies to: 90-90
✅ Verification successful
Role validation change is correctly implemented and safe
The verification confirms that:
User::ROLESis properly defined as a constant array containing all three roles:ROLE_ADMIN,ROLE_USER, andROLE_READONLY- The roles are used consistently throughout the codebase
- The validation rule using
implode(',', User::ROLES)will correctly validate against all defined roles
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find role definitions and their usage
ast-grep --pattern 'const ROLES = $_'
rg "ROLE_READONLY" --type php
Length of output: 297
Script:
#!/bin/bash
# Let's check the complete role definitions and validation usage
cat api/app/Models/User.php
Length of output: 6647
api/app/Models/User.php (2)
24-24: LGTM: Role constant follows naming convention
The ROLE_READONLY constant follows the established naming pattern of other role constants.
128-131: LGTM: Efficient role check implementation
The getIsReadonlyAttribute method efficiently checks for readonly role using exists().
client/components/open/tables/OpenTable.vue (3)
32-32: LGTM: Proper conditional rendering
The action column is correctly hidden for read-only users.
217-219: LGTM: Clean computed property implementation
The hasActions computed property provides a clean way to determine action visibility.
185-185: LGTM: Proper user state management
User state is correctly retrieved from the auth store.
client/pages/home.vue (1)
250-251: LGTM: Clean state management setup
The user state is properly retrieved from the auth store using the composition API.
client/pages/forms/[slug]/show.vue (2)
35-35: LGTM! Appropriate access control for read-only users
The conditional rendering correctly prevents read-only users from accessing edit functionality through both the extra menu and edit button.
Also applies to: 102-102
249-250: LGTM! Clean store integration
The auth store integration and computed property implementation follow Vue.js best practices for reactive state management.
api/app/Policies/FormPolicy.php
Outdated
| @@ -37,7 +37,7 @@ public function view(User $user, Form $form) | |||
| */ | |||
| public function create(User $user) | |||
| { | |||
| return true; | |||
| return !$user->is_readonly; | |||
There was a problem hiding this comment.
Add null check for consistency with TemplatePolicy
The create method should include a null check for consistency with TemplatePolicy and to prevent potential security issues.
- return !$user->is_readonly;
+ return $user !== null && !$user->is_readonly;📝 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.
| return !$user->is_readonly; | |
| return $user !== null && !$user->is_readonly; |
…logic - Introduced a private method `canPerformWriteOperation` in both FormPolicy and TemplatePolicy to encapsulate the logic for determining if a user can perform write operations on the respective models. - Updated the `update`, `delete`, `restore`, and `forceDelete` methods in FormPolicy to use the new method for improved readability and maintainability. - Simplified the `update` and `delete` methods in TemplatePolicy to leverage the centralized write operation logic. This refactoring enhances code clarity and reduces duplication across policy classes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
api/app/Policies/TemplatePolicy.php (3)
18-18: Add return type-hint and enhance null safetyThe logic is correct, but could be more robust with type-hinting and null coalescing.
Consider applying these improvements:
- public function create(User $user) + public function create(User $user): bool { - return $user !== null && !$user->is_readonly; + return $user !== null && !($user->is_readonly ?? false); }
24-27: Consider breaking down the complex authorization conditionWhile the logic is correct, the complex boolean condition could be split for better readability and maintainability.
Consider this more readable approach:
private function canPerformWriteOperation(User $user, Template $template): bool { - return ($user->admin || $user->template_editor || $template->creator_id === $user->id) && !$user->is_readonly; + if ($user->is_readonly) { + return false; + } + + return $user->admin + || ($user->template_editor ?? false) + || $template->creator_id === $user->id; }
31-31: Add return type hints and documentationThe refactoring to use
canPerformWriteOperationis good, but the methods would benefit from explicit return type hints.Apply these improvements:
- public function update(User $user, Template $template) + /** + * Determine if the user can update the template. + * + * @param User $user + * @param Template $template + * @return bool + */ + public function update(User $user, Template $template): bool { return $this->canPerformWriteOperation($user, $template); } - public function delete(User $user, Template $template) + /** + * Determine if the user can delete the template. + * + * @param User $user + * @param Template $template + * @return bool + */ + public function delete(User $user, Template $template): bool { return $this->canPerformWriteOperation($user, $template); }Also applies to: 36-36
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
api/app/Policies/FormPolicy.php(5 hunks)api/app/Policies/TemplatePolicy.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/app/Policies/FormPolicy.php
🔇 Additional comments (1)
api/app/Policies/TemplatePolicy.php (1)
18-18: Verify consistent read-only handling across policies
Let's ensure the read-only user handling is consistent across all policy classes.
Also applies to: 26-26, 31-31, 36-36
✅ Verification successful
Let me check the actual implementation in these policy files to verify consistency.
Read-only user handling is consistent across policies
The verification shows that both FormPolicy and TemplatePolicy implement consistent read-only user handling:
- Both check
!$user->is_readonlyin theircreate()methods - Both include
!$user->is_readonlyas part of theircanPerformWriteOperation()methods - The only differences are in the additional authorization logic specific to each resource type, which is correct
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar policy classes and their read-only handling
echo "Searching for other policy classes with read-only checks..."
rg -l "is_readonly" "api/app/Policies/"
echo "\nChecking for consistent implementation patterns..."
ast-grep --pattern 'class $_ {
$$$
private function canPerformWriteOperation($_, $_) {
$$$
}
$$$
}'
Length of output: 463
Script:
#!/bin/bash
# Check the actual implementation of read-only checks in both policy files
echo "FormPolicy implementation:"
rg "is_readonly" api/app/Policies/FormPolicy.php -B 2 -A 2
echo "\nTemplatePolicy implementation:"
rg "is_readonly" api/app/Policies/TemplatePolicy.php -B 2 -A 2
Length of output: 826
- Updated FormController to authorize form creation based on workspace context. - Removed the `is_readonly` attribute from UserResource and integrated it into WorkspaceResource for better encapsulation. - Refactored User model to eliminate the `getIsReadonlyAttribute` method, shifting readonly logic to the Workspace model. - Adjusted FormPolicy and TemplatePolicy to utilize workspace readonly checks for user permissions. - Updated various frontend components to reference workspace readonly status instead of user readonly status, enhancing clarity and consistency in permission handling. These changes improve the management of user permissions in relation to workspaces, ensuring a more robust and maintainable authorization system.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
api/app/Http/Controllers/Forms/FormController.php (1)
The
userIsOwnerflag should reflect the user's actual role in the workspaceThe current implementation has the following issues:
- The
userIsOwnerflag is hardcoded totruein the show method- The codebase has proper role checks through
isReadonlyUser()method- The form policy's
view()method usesownsForm()which checks workspace membership- Readonly users can view forms but are restricted from write operations
The flag should be set based on the user's actual role instead of being hardcoded:
'userIsOwner' => !$workspace->isReadonlyUser($request->user())🔗 Analysis chain
Line range hint
56-77: Verify readonly user access in show methodThe show method should potentially handle readonly users differently, especially regarding the
userIsOwnerflag which is currently hardcoded totrue.Let's check for similar patterns:
Let me check how the authorization is implemented to understand how readonly users are handled.
Let me check how the
ownsFormmethod is implemented to understand the ownership check.
Let me try a different approach to find the
ownsFormmethod.
Let me check how readonly users are determined in the workspace.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for userIsOwner assignments rg "'userIsOwner'\s*=>\s*true" -A 2Length of output: 844
Script:
#!/bin/bash # Search for the Form policy class and its view method fd "FormPolicy.php" --exec cat {}Length of output: 2024
Script:
#!/bin/bash # Search for ownsForm method implementation ast-grep --pattern 'function ownsForm($_) { $$$ }'Length of output: 54
Script:
#!/bin/bash # Search for ownsForm method using ripgrep rg "function ownsForm" -A 5Length of output: 374
Script:
#!/bin/bash # Search for isReadonlyUser method rg "function isReadonlyUser" -A 5Length of output: 417
♻️ Duplicate comments (1)
api/app/Policies/FormPolicy.php (1)
39-42:⚠️ Potential issueAdd null check in create method
For consistency with other policies and to prevent potential security issues, we should add a null check.
- public function create(User $user, Workspace $workspace) - { - return !$workspace->isReadonlyUser($user); - } + public function create(?User $user, Workspace $workspace) + { + return $user !== null && !$workspace->isReadonlyUser($user); + }
🧹 Nitpick comments (6)
client/components/open/tables/OpenTable.vue (2)
Line range hint
32-40: Consider making the actions column width configurableThe actions column width is hardcoded to 100px. Consider extracting this to a prop or computed property for better maintainability and reusability.
- style="width: 100px" + :style="{ width: actionsColumnWidth }"Add to props:
props: { actionsColumnWidth: { type: String, default: '100px' } }
218-220: Consider granular action permissionsCurrently, the entire actions column is hidden for readonly users. Consider implementing granular permission checks for individual actions instead, as some actions (like view details) might still be relevant for readonly users.
This could be implemented by:
- Moving permission checks to individual actions in the RecordOperations component
- Always showing the actions column but conditionally rendering specific actions
- Adding a prop to configure which actions are available for readonly users
client/pages/home.vue (2)
11-11: LGTM: Consistent implementation of read-only access controlThe "Create a new form" button is consistently hidden for read-only users in both locations. The implementation aligns well with the PR's objective.
However, consider improving the empty state UX for read-only users.
Consider adding explanatory text for read-only users when no forms exist:
<div v-if="!formsLoading && enrichedForms.length === 0" class="flex flex-wrap justify-center max-w-4xl"> <img class="w-56" src="/img/pages/forms/search_notfound.png" alt="search-not-found"> <h3 class="w-full mt-4 text-center text-gray-900 font-semibold">No forms found</h3> + <p v-if="workspace.is_readonly" class="mt-2 w-full text-center text-gray-600"> + You have read-only access to this workspace. Please contact a workspace administrator to create new forms. + </p> <div v-if="isFilteringForms && enrichedForms.length === 0 && search" class="mt-2 w-full text-center"> Your search "{{ search }}" did not match any forms. Please try again. </div>Also applies to: 90-90
Line range hint
11-34: Consider extracting the create form button to a componentThe "Create a new form" button is duplicated with identical markup and logic. Consider extracting it to a reusable component to improve maintainability.
Create a new component
CreateFormButton.vue:<template> <v-button v-if="!workspace.is_readonly" v-track.create_form_click :class="className" :to="{ name: 'forms-create' }" > <svg class="w-4 h-4 text-white inline mr-1 -mt-1" viewBox="0 0 14 14" fill="none" xmlns="http://www.w3.org/2000/svg" > <path d="M6.99996 1.1665V12.8332M1.16663 6.99984H12.8333" stroke="currentColor" stroke-width="1.67" stroke-linecap="round" stroke-linejoin="round" /> </svg> Create a new form </v-button> </template> <script setup> import { useWorkspacesStore } from '@/stores/workspaces' const props = defineProps({ className: { type: String, default: '' } }) const workspacesStore = useWorkspacesStore() const workspace = computed(() => workspacesStore.getCurrent) </script>Then use it in both locations:
- <v-button - v-if="!workspace.is_readonly" - v-track.create_form_click - :to="{ name: 'forms-create' }" - > - <svg>...</svg> - Create a new form - </v-button> + <CreateFormButton />- <v-button - v-if="!workspace.is_readonly && forms.length === 0" - v-track.create_form_click - class="mt-4" - :to="{ name: 'forms-create' }" - > - <svg>...</svg> - Create a new form - </v-button> + <CreateFormButton + v-if="forms.length === 0" + class="mt-4" + />Also applies to: 90-113
api/app/Models/Workspace.php (1)
199-199: Minor: Consistent arrow function spacingFor consistency with the codebase style, add a space after
fn.- return $this->owners->filter(fn($owner) => $owner->is_subscribed); + return $this->owners->filter(fn ($owner) => $owner->is_subscribed);api/app/Http/Controllers/Forms/FormController.php (1)
Line range hint
108-134: Review store method's error handling for readonly usersWhile the authorization check has been updated, consider adding specific error messages or handling for readonly users when they attempt to create forms.
Suggested enhancement:
$this->authorize('create', [Form::class, $workspace]); + +// Consider adding specific handling for readonly users +if ($workspace->isReadonlyUser(Auth::user())) { + return $this->error([ + 'message' => 'Readonly users cannot create forms.' + ]); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
api/app/Http/Controllers/Forms/FormController.php(1 hunks)api/app/Http/Resources/WorkspaceResource.php(1 hunks)api/app/Models/User.php(1 hunks)api/app/Models/Workspace.php(1 hunks)api/app/Policies/FormPolicy.php(6 hunks)api/app/Policies/TemplatePolicy.php(1 hunks)client/components/open/tables/OpenTable.vue(3 hunks)client/components/pages/forms/show/ExtraMenu.vue(4 hunks)client/components/pages/settings/WorkSpaceUser.vue(1 hunks)client/pages/forms/[slug]/show.vue(4 hunks)client/pages/forms/[slug]/show/share.vue(2 hunks)client/pages/home.vue(2 hunks)client/pages/settings.vue(2 hunks)client/pages/settings/workspace.vue(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- api/app/Models/User.php
- client/pages/forms/[slug]/show/share.vue
- client/pages/forms/[slug]/show.vue
- api/app/Policies/TemplatePolicy.php
- client/pages/settings.vue
- client/components/pages/forms/show/ExtraMenu.vue
🔇 Additional comments (11)
client/components/pages/settings/WorkSpaceUser.vue (2)
8-8: LGTM! The readonly restriction is correctly implemented.
The condition prevents readonly users from accessing the invite functionality.
Line range hint 1-300: Consider implementing consistent readonly role restrictions across all user management functions.
While the invite button is correctly restricted, other user management functionalities should also respect the readonly role:
- The EditWorkSpaceUser component should be conditionally rendered based on readonly status
- User management functions (remove, leave, delete) should include readonly role checks
- The isWorkspaceAdmin computed property should account for the readonly role
Here's a suggested implementation:
const isWorkspaceAdmin = computed(() => {
if (!users.value) return false
const user = users.value.find((user) => user.id === authStore.user.id)
- return user && user.pivot.role === "admin"
+ return user && user.pivot.role === "admin" && !workspace.value.is_readonly
})
<EditWorkSpaceUser
+ v-if="!workspace.is_readonly"
:user="selectedUser"
:show-edit-user-modal="showEditUserModal"
@close="showEditUserModal = false"
@fetch-users="getWorkspaceUsers"
/>
const removeUser = (index) => {
const user = users.value[index]
+ if (workspace.value.is_readonly) {
+ useAlert().error("Readonly users cannot remove workspace members")
+ return
+ }
useAlert().confirm(
// ... rest of the function
)
}Let's verify if there are any other components that need similar readonly role restrictions:
client/components/open/tables/OpenTable.vue (1)
185-186: LGTM: Clean store integration
The store integration follows Vue 3 composition API best practices and properly establishes the required store references.
client/pages/settings/workspace.vue (2)
12-15: LGTM: Proper implementation of read-only access control
The conditional rendering of workspace settings components based on workspace.is_readonly correctly implements access control for read-only users.
103-103: Verify workspace store initialization
The computed property looks good, but let's verify the store initialization to ensure workspace.is_readonly is always available when needed.
✅ Verification successful
Let me gather more information about the workspace store implementation and usage.
Based on the gathered information, I can now provide the final response:
Workspace store and is_readonly property are properly handled
The is_readonly property is consistently used across the codebase for access control and UI rendering. It's properly:
- Initialized from the backend via
WorkspaceResource - Used in multiple components for conditional rendering
- Accessed through the workspace store which is correctly implemented
The computed property in the settings page is safe to use as the workspace store properly manages the current workspace state.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if workspace store properly initializes the is_readonly property
# Look for store initialization and type definitions
# Check store initialization
rg -A 5 "defineStore\(['\"']workspaces"
# Check type definitions
ast-grep --pattern 'interface $_ {
$$$
is_readonly: $_
$$$
}'
Length of output: 496
Script:
#!/bin/bash
# Check the full implementation of workspaces store
cat client/stores/workspaces.js
# Look for workspace type definitions in TypeScript files
fd -e ts -e d.ts --exec cat {}
# Check for is_readonly usage in the codebase
rg "is_readonly" -A 2
Length of output: 8626
api/app/Http/Resources/WorkspaceResource.php (1)
21-21: 🛠️ Refactor suggestion
Add null check for request user
The isReadonlyUser call should handle cases where there is no authenticated user.
- 'is_readonly' => $this->isReadonlyUser($request->user()),
+ 'is_readonly' => $request->user() ? $this->isReadonlyUser($request->user()) : false,Let's verify how this field is used in the frontend:
api/app/Policies/FormPolicy.php (2)
44-49: LGTM: Well-structured permission check
The canPerformWriteOperation method effectively centralizes the write permission logic by checking both ownership and readonly status.
Line range hint 59-89: Verify all write operations are protected
The implementation consistently uses canPerformWriteOperation for update, delete, restore, and forceDelete operations.
Let's verify we haven't missed any write operations:
✅ Verification successful
Let me analyze the potential write operations by searching for method implementations in the Form model.
Let me try a different approach using ripgrep to find write operations in the Form model.
Let me check the Form model's write-related methods and the controllers to ensure complete coverage.
All write operations are properly protected
After analyzing the codebase, I can confirm that all write operations are properly protected:
- The FormPolicy correctly implements
canPerformWriteOperationchecks for all modification methods (update, delete, restore, forceDelete) - The FormController properly enforces these policies through
authorizecalls for all write operations:store: Usesauthorize('create')update: Usesauthorize('update')destroy: Usesauthorize('delete')updateWorkspace: Usesauthorize('update')
The implementation is consistent and secure, with no gaps in the authorization checks for write operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other potential write operations in Form model and controllers
rg "function.*Form" --type php
Length of output: 9591
Script:
#!/bin/bash
# Search for method implementations in Form model
ast-grep --pattern 'function $_($$$) { $$$ }' api/app/Models/Forms/Form.php
Length of output: 77
Script:
#!/bin/bash
# Search for write operations in Form model with context
rg "function" api/app/Models/Forms/Form.php -B 1 -A 5
Length of output: 4577
Script:
#!/bin/bash
# Search for write operations in FormController
rg "function (create|store|update|delete|destroy|restore|force)" api/app/Http/Controllers/Forms/FormController.php -A 5
Length of output: 922
api/app/Models/Workspace.php (1)
207-213: LGTM: Efficient role check implementation
The isReadonlyUser method is well implemented using an efficient exists() query.
Let's verify the ROLE_READONLY constant is properly defined:
✅ Verification successful
Verification successful: Role constant and usage patterns are well-defined
The verification confirms:
ROLE_READONLYconstant is properly defined inUsermodel as'readonly'isReadonlyUser()is used appropriately in:WorkspaceResourcefor read-only status in API responsesFormPolicyfor permission checks, preventing read-only users from modifying forms
The implementation is correct and follows best practices with no N+1 query concerns since the method uses an efficient exists() query.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check User model for role constant definition
rg "ROLE_READONLY" --type php
# Check for potential N+1 query issues by looking for usage patterns
rg "isReadonlyUser.*\)" --type php
Length of output: 660
api/app/Http/Controllers/Forms/FormController.php (2)
113-113: Authorization enhancement for readonly user support
The change to include workspace context in the authorization check ([Form::class, $workspace]) aligns with implementing readonly user functionality. This allows for more granular permission control at the workspace level.
Let's verify the consistency of authorization checks across the codebase:
✅ Verification successful
Authorization checks are properly implemented for readonly user support
The authorization implementation is consistent and correct:
- The
createpolicy correctly checks for readonly users via!$workspace->isReadonlyUser($user) - All write operations (update/delete) use
canPerformWriteOperationwhich includes readonly user checks - Read operations (view/viewAny) are properly separated from write operations
- The workspace context is appropriately used where needed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Form::class authorization patterns
rg "authorize\('create'.*Form::class" -A 2
# Search for workspace-related authorization checks
rg "authorize\('.*'.*\[Form::class,.*workspace" -A 2
Length of output: 617
Script:
#!/bin/bash
# Search for all authorize calls in FormController to check other permission checks
rg "authorize\(" api/app/Http/Controllers/Forms/FormController.php -A 2
# Search for Form policy to understand authorization rules
fd "FormPolicy" --exec cat {}
Length of output: 3351
Line range hint 40-54: Consider adding readonly check in index method
While the method checks workspace view permission, consider adding explicit readonly user handling for consistency with the store method's changes. This could affect how forms are presented to readonly users.
Let's verify if readonly checks exist in similar list/index methods:
| hasActions() { | ||
| return !this.workspace.is_readonly | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null checks and documentation for hasActions computed property
The computed property should handle cases where workspace might be null and document its purpose.
hasActions() {
- return !this.workspace.is_readonly
+ // Controls visibility of action buttons based on workspace readonly status
+ return this.workspace?.is_readonly === false
},📝 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.
| hasActions() { | |
| return !this.workspace.is_readonly | |
| }, | |
| hasActions() { | |
| // Controls visibility of action buttons based on workspace readonly status | |
| return this.workspace?.is_readonly === false | |
| }, |
|
Fixed |
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Refactor
Chores