move Permited Atributes to Policy #1200
Conversation
|
Warning Rate limit exceeded@lodewiges has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 3 minutes and 53 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR refactors attribute permission handling across eight controllers and their corresponding policies, centralizing attribute whitelisting from controller methods to policy-driven permitted_attributes methods. Each controller now delegates parameter permitting to its policy instead of managing rules directly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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% 78.18% +0.49%
===========================================
Files 54 54
Lines 1340 1375 +35
===========================================
+ Hits 1041 1075 +34
- Misses 299 300 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.