move Permited Atributes to Policy #1200
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughRefactors strong-parameter handling: controllers delegate attribute permitting to policy methods (e.g., Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
![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
🤖 Fix all issues with AI agents
In @app/policies/invoice_policy.rb:
- Around line 14-19: permitted_attributes currently permits :rows alongside
rows_attributes which allows unintended mass-assignment of the association; edit
the permitted_attributes method to remove the :rows symbol so only
rows_attributes (i.e. { rows_attributes: %i[name amount price] }) is allowed,
leaving the rest of the array (:user_id, :activity_id, :name_override,
:email_override) unchanged.
In @app/policies/product_policy.rb:
- Around line 10-15: Remove the top-level :id from the array returned by
permitted_attributes in ProductPolicy; keep the nested :id inside the
product_prices_attributes hash and preserve :_destroy there, so modify the
permitted_attributes method to list only :name, :category, :color, :requires_age
and the nested { product_prices_attributes: %i[id product_id price_list_id price
_destroy] } while ensuring the top-level :id is not included.
🧹 Nitpick comments (2)
app/controllers/credit_mutations_controller.rb (1)
35-45: Consider making helper methods private.The
which_redirect?andmodel_includesmethods (lines 35-41) appear to be internal helpers but are currently public. Consider moving them below theprivatekeyword along withcredit_mutation_paramsfor consistency.♻️ Suggested refactor
end end + private + def which_redirect? request.referer || @mutation.user end def model_includes %i[user activity created_by] end def credit_mutation_params params.require(:credit_mutation).permit(policy(CreditMutation).permitted_attributes) end endapp/controllers/users_controller.rb (1)
91-91: Inline permit vs helper method - minor inconsistency.The
updateaction inlines theparams.require(:user).permit(...)call, whilecreateuses theuser_paramshelper. This is functionally correct but slightly inconsistent in style.Consider extracting to a helper for consistency (optional)
def update @user = User.find(params[:id]) authorize @user - if @user.update(params.require(:user).permit(policy(@user).permitted_attributes_for_update)) + if @user.update(user_update_params) flash[:success] = 'Gebruiker geupdate' else flash[:error] = "Gebruiker updaten mislukt; #{@user.errors.full_messages.join(', ')}" end redirect_to @user endThen add a private helper:
def user_update_params params.require(:user).permit(policy(@user).permitted_attributes_for_update) end
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
app/controllers/activities_controller.rbapp/controllers/credit_mutations_controller.rbapp/controllers/invoices_controller.rbapp/controllers/orders_controller.rbapp/controllers/price_lists_controller.rbapp/controllers/products_controller.rbapp/controllers/sofia_accounts_controller.rbapp/controllers/users_controller.rbapp/policies/activity_policy.rbapp/policies/application_policy.rbapp/policies/credit_mutation_policy.rbapp/policies/invoice_policy.rbapp/policies/order_policy.rbapp/policies/price_list_policy.rbapp/policies/product_policy.rbapp/policies/sofia_account_policy.rbapp/policies/user_policy.rb
🧰 Additional context used
🧬 Code graph analysis (11)
app/policies/sofia_account_policy.rb (8)
app/policies/activity_policy.rb (1)
permitted_attributes(52-54)app/policies/application_policy.rb (1)
permitted_attributes(41-43)app/policies/credit_mutation_policy.rb (1)
permitted_attributes(20-22)app/policies/invoice_policy.rb (1)
permitted_attributes(14-19)app/policies/order_policy.rb (1)
permitted_attributes(20-22)app/policies/price_list_policy.rb (1)
permitted_attributes(40-42)app/policies/product_policy.rb (1)
permitted_attributes(10-15)app/policies/user_policy.rb (1)
permitted_attributes(26-28)
app/policies/invoice_policy.rb (8)
app/policies/activity_policy.rb (1)
permitted_attributes(52-54)app/policies/application_policy.rb (1)
permitted_attributes(41-43)app/policies/credit_mutation_policy.rb (1)
permitted_attributes(20-22)app/policies/order_policy.rb (1)
permitted_attributes(20-22)app/policies/price_list_policy.rb (1)
permitted_attributes(40-42)app/policies/product_policy.rb (1)
permitted_attributes(10-15)app/policies/sofia_account_policy.rb (1)
permitted_attributes(22-24)app/policies/user_policy.rb (1)
permitted_attributes(26-28)
app/controllers/products_controller.rb (2)
app/policies/application_policy.rb (1)
permitted_attributes(41-43)app/policies/product_policy.rb (1)
permitted_attributes(10-15)
app/controllers/sofia_accounts_controller.rb (2)
app/policies/application_policy.rb (1)
permitted_attributes(41-43)app/policies/sofia_account_policy.rb (1)
permitted_attributes(22-24)
app/controllers/price_lists_controller.rb (2)
app/policies/application_policy.rb (1)
permitted_attributes(41-43)app/policies/price_list_policy.rb (1)
permitted_attributes(40-42)
app/policies/application_policy.rb (8)
app/policies/activity_policy.rb (1)
permitted_attributes(52-54)app/policies/credit_mutation_policy.rb (1)
permitted_attributes(20-22)app/policies/invoice_policy.rb (1)
permitted_attributes(14-19)app/policies/order_policy.rb (1)
permitted_attributes(20-22)app/policies/price_list_policy.rb (1)
permitted_attributes(40-42)app/policies/product_policy.rb (1)
permitted_attributes(10-15)app/policies/sofia_account_policy.rb (1)
permitted_attributes(22-24)app/policies/user_policy.rb (1)
permitted_attributes(26-28)
app/controllers/orders_controller.rb (3)
app/controllers/users_controller.rb (1)
update(87-98)app/policies/order_policy.rb (2)
permitted_attributes_for_create(24-26)permitted_attributes_for_update(28-30)app/policies/user_policy.rb (1)
permitted_attributes_for_update(30-32)
app/controllers/activities_controller.rb (3)
app/controllers/users_controller.rb (1)
update(87-98)app/policies/activity_policy.rb (2)
permitted_attributes(52-54)permitted_attributes_for_update(56-58)app/policies/application_policy.rb (1)
permitted_attributes(41-43)
app/controllers/users_controller.rb (3)
app/policies/user_policy.rb (3)
permitted_attributes_for_update(30-32)permitted_attributes_for_update_with_sofia_account(34-38)permitted_attributes(26-28)app/policies/application_policy.rb (1)
permitted_attributes(41-43)app/policies/sofia_account_policy.rb (1)
permitted_attributes(22-24)
app/controllers/invoices_controller.rb (3)
app/helpers/application_helper.rb (1)
remove_empty(19-27)app/policies/application_policy.rb (1)
permitted_attributes(41-43)app/policies/invoice_policy.rb (1)
permitted_attributes(14-19)
app/controllers/credit_mutations_controller.rb (3)
app/policies/application_policy.rb (1)
permitted_attributes(41-43)app/policies/credit_mutation_policy.rb (1)
permitted_attributes(20-22)app/policies/user_policy.rb (1)
permitted_attributes(26-28)
⏰ 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). (2)
- GitHub Check: Agent
- GitHub Check: Build
🔇 Additional comments (21)
app/policies/application_policy.rb (1)
41-43: LGTM!Good default implementation returning an empty array. This ensures no attributes are permitted unless explicitly whitelisted by subclass policies, following the principle of least privilege.
app/policies/price_list_policy.rb (1)
40-42: LGTM!Clean implementation following the established pattern. Minimal whitelist with only the
:nameattribute.app/policies/credit_mutation_policy.rb (1)
20-22: LGTM!Clean implementation. The permitted attributes align well with the
create?authorization logic that enforces activity presence for main bartenders.app/controllers/invoices_controller.rb (2)
30-30: LGTM!The refactoring correctly delegates parameter whitelisting to the policy layer via
invoice_params, maintaining the existingremove_emptyprocessing for empty value handling.
83-85: LGTM!The
invoice_paramsmethod correctly follows the policy-driven pattern usingpolicy(Invoice).permitted_attributes. Using the classInvoice(rather than an instance) is appropriate here since this is used in thecreateaction before an instance exists.app/policies/order_policy.rb (1)
19-30: LGTM!Good separation of concerns with action-specific permitted attributes:
- Base
permitted_attributesfor common fieldspermitted_attributes_for_createextends base with nestedorder_rows_attributesincludingproduct_idpermitted_attributes_for_updaterestricts to onlyproduct_countchanges, appropriately preventing product_id modification on updatesThe composition pattern using
permitted_attributes + [...]inpermitted_attributes_for_createis clean and maintainable.app/controllers/products_controller.rb (2)
8-8: LGTM!Both
createandupdateactions correctly use the refactoredproduct_paramsmethod.Also applies to: 21-21
34-36: LGTM!The
product_paramsmethod correctly usespolicy(Product).permitted_attributes. SinceProductPolicyhas a singlepermitted_attributesmethod (without action-specific variants), using the class-based policy lookup is appropriate for both create and update actions.app/controllers/orders_controller.rb (3)
19-19: LGTM!The create action correctly uses
order_paramswith thecreated_bymerge, delegating attribute whitelisting to the policy layer.
46-46: LGTM!The update action correctly uses the dedicated
order_params_for_updatehelper.
86-92: LGTM!Good implementation of action-specific parameter helpers:
order_paramsuses class-basedpolicy(Order)withpermitted_attributes_for_create(appropriate before instance exists)order_params_for_updateuses instance-basedpolicy(@order)withpermitted_attributes_for_update(proper Pundit pattern for existing records)app/controllers/credit_mutations_controller.rb (1)
17-17: LGTM!The create action correctly uses
credit_mutation_paramswith thecreated_bymerge, consistent with the pattern used inorders_controller.rb.app/controllers/price_lists_controller.rb (1)
79-81: LGTM!The refactor correctly delegates attribute whitelisting to the policy. Using
policy(PriceList)with the class is appropriate here sincepermitted_attributesinPriceListPolicydoesn't depend on the record's state.app/policies/user_policy.rb (1)
26-38: LGTM!The three permission methods appropriately separate concerns:
permitted_attributesfor creation includesprovider/sub_providerpermitted_attributes_for_updateexcludes provider fields (correct - shouldn't change after creation)permitted_attributes_for_update_with_sofia_accountcorrectly uses conditional logic for treasurer-only fieldsThe nested
sofia_account_attributessyntax is correct for Rails strong parameters.app/controllers/users_controller.rb (2)
75-76: LGTM!Correctly uses
user_paramswhich delegates to policy-based whitelisting for user creation.
168-170: LGTM!The
user_paramshelper correctly delegates to the policy for attribute whitelisting during user creation.app/policies/sofia_account_policy.rb (1)
22-24: LGTM!The
permitted_attributesmethod correctly defines the whitelist for direct Sofia account updates, including password management attributes. This is consistent with the policy-driven approach used across the codebase.app/policies/activity_policy.rb (1)
52-58: LGTM!The separation between create and update permissions is appropriate:
permitted_attributesallows full configuration on creationpermitted_attributes_for_updaterestricts to onlytitle, preventing changes tostart_time,end_time, andprice_list_idafter creationThis restrictive update policy appears intentional to maintain data integrity for activities that may have associated orders and invoices.
app/controllers/sofia_accounts_controller.rb (1)
18-18: LGTM! Clean delegation to policy for permitted attributes.The implementation correctly uses
policy(SofiaAccount)(class-based) since there's no instance available at create time, and the method naming follows Rails conventions.Also applies to: 259-261
app/controllers/activities_controller.rb (2)
48-48: LGTM! Correct class-based policy delegation for create action.Using
policy(Activity)(class-based) is appropriate here since the instance doesn't exist yet at parameter parsing time.Also applies to: 180-182
64-64: LGTM! Correct instance-based policy delegation for update action.Using
policy(@activity)allows the policy to make context-aware decisions about permitted attributes based on the specific record being updated. This pattern correctly differs from the create action approach.Also applies to: 184-186
There was a problem hiding this comment.
Pull request overview
This PR refactors the application to move permitted attributes from controller private methods to policy classes, centralizing authorization logic. This change follows a common Rails pattern where Pundit policies not only handle authorization but also define which attributes are permitted for different operations.
Changes:
- Added
permitted_attributesmethods to all policy classes (ApplicationPolicy, UserPolicy, SofiaAccountPolicy, ProductPolicy, PriceListPolicy, OrderPolicy, InvoicePolicy, CreditMutationPolicy, and ActivityPolicy) - Updated all controller methods to call policy permitted_attributes methods instead of locally defined private methods
- Renamed controller private methods from
permitted_attributesto more descriptive names likeuser_params,product_params, etc.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| app/policies/application_policy.rb | Added base permitted_attributes method returning empty array |
| app/policies/user_policy.rb | Added three permitted attributes methods for different update scenarios |
| app/policies/sofia_account_policy.rb | Added permitted_attributes method for sofia account creation |
| app/policies/product_policy.rb | Added permitted_attributes method including nested product_prices_attributes |
| app/policies/price_list_policy.rb | Added simple permitted_attributes method for name field |
| app/policies/order_policy.rb | Added three permitted attributes methods for base, create, and update operations |
| app/policies/invoice_policy.rb | Added permitted_attributes method including nested rows_attributes |
| app/policies/credit_mutation_policy.rb | Added permitted_attributes method for credit mutation fields |
| app/policies/activity_policy.rb | Added two permitted attributes methods for create and update scenarios |
| app/controllers/users_controller.rb | Updated to use policy permitted_attributes; renamed method to user_params |
| app/controllers/sofia_accounts_controller.rb | Updated to use policy permitted_attributes; renamed method to sofia_account_params |
| app/controllers/products_controller.rb | Updated to use policy permitted_attributes; renamed method to product_params |
| app/controllers/price_lists_controller.rb | Updated to use policy permitted_attributes; renamed method to price_list_params |
| app/controllers/orders_controller.rb | Updated to use policy permitted_attributes; renamed methods to order_params and order_params_for_update |
| app/controllers/invoices_controller.rb | Updated to use policy permitted_attributes; renamed method to invoice_params |
| app/controllers/credit_mutations_controller.rb | Updated to use policy permitted_attributes; renamed method to credit_mutation_params |
| app/controllers/activities_controller.rb | Updated to use policy permitted_attributes; added activity_params and activity_params_for_update methods |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
app/controllers/orders_controller.rb
Outdated
| params.require(:order).permit(%i[user_id paid_with_cash paid_with_pin activity_id], | ||
| order_rows_attributes: %i[id product_id product_count]) | ||
| def order_params | ||
| params.require(:order).permit(policy(Order).permitted_attributes_for_create) |
There was a problem hiding this comment.
The policy is called with the Order class instead of an instance. This will cause an error when trying to access permitted_attributes_for_create from the policy, as Pundit expects to initialize policies with a user and a record. You should pass an instance like Order.new here.
| params.require(:order).permit(policy(Order).permitted_attributes_for_create) | |
| params.require(:order).permit(policy(Order.new).permitted_attributes_for_create) |
| def permitted_attributes | ||
| %i[name] | ||
| end |
There was a problem hiding this comment.
The newly added permitted_attributes method lacks test coverage. Since this repository has comprehensive controller tests, consider adding tests to verify that the permitted_attributes method returns the correct attributes.
| def permitted_attributes | ||
| %i[description amount user_id activity_id] | ||
| end |
There was a problem hiding this comment.
The newly added permitted_attributes method lacks test coverage. Since this repository has comprehensive controller tests, consider adding tests to verify that the permitted_attributes method returns the correct attributes.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## staging #1200 +/- ##
===========================================
- Coverage 77.68% 77.61% -0.08%
===========================================
Files 54 54
Lines 1340 1407 +67
===========================================
+ Hits 1041 1092 +51
- Misses 299 315 +16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/policies/order_policy.rb`:
- Around line 28-30: Remove the top-level :id from the permitted attributes in
permitted_attributes_for_update; change the returned array to only include the
nested order_rows_attributes with its %i[id product_count] (keep the nested :id
so existing order_rows can be matched), i.e., update the
permitted_attributes_for_update method to stop permitting the order's primary
key from the request body while retaining order_rows_attributes' ids.
| def permitted_attributes_for_update | ||
| [:id, { order_rows_attributes: %i[id product_count] }] | ||
| end |
There was a problem hiding this comment.
Remove :id from the top-level permitted attributes for update.
The top-level :id should not be permitted—the order's ID is determined by the route parameter (params[:id]), not the request body. Including it unnecessarily permits mass-assignment of the primary key, which violates the principle of permitting only what's needed.
The nested :id in order_rows_attributes is correct for identifying existing records to update.
Proposed fix
def permitted_attributes_for_update
- [:id, { order_rows_attributes: %i[id product_count] }]
+ [{ order_rows_attributes: %i[id product_count] }]
end📝 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.
| def permitted_attributes_for_update | |
| [:id, { order_rows_attributes: %i[id product_count] }] | |
| end | |
| def permitted_attributes_for_update | |
| [{ order_rows_attributes: %i[id product_count] }] | |
| end |
🤖 Prompt for AI Agents
In `@app/policies/order_policy.rb` around lines 28 - 30, Remove the top-level :id
from the permitted attributes in permitted_attributes_for_update; change the
returned array to only include the nested order_rows_attributes with its %i[id
product_count] (keep the nested :id so existing order_rows can be matched),
i.e., update the permitted_attributes_for_update method to stop permitting the
order's primary key from the request body while retaining order_rows_attributes'
ids.
move Permited Atributes to Policy
Fixes #191
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.