Skip to content

[WIP] [PoC] Migrate from protected attributes to strong parameters #4238

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.

Sign up for GitHub

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

Closed
mayorova wants to merge 17 commits into from
Closed

[WIP] [PoC] Migrate from protected attributes to strong parameters #4238

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
4ca06ef
Migrate protected attrs to strong params for User-related controllers
mayorova Mar 5, 2026
f30629d
Update test for User-related controllers
mayorova Mar 5, 2026
cbe7b54
Fix tests related to user model
mayorova Mar 6, 2026
97be0c3
Fix rspecs broken by duplicate field definitions creation
mayorova Mar 6, 2026
305efc5
Fix more tests and refactor invitation signup flow
mayorova Mar 6, 2026
60f881c
Remove TODO comments
mayorova Mar 9, 2026
a4a6540
Migrate to strong parameters for usage limits
mayorova Mar 9, 2026
dc62875
Remove attr_accessible for application_key
mayorova Mar 9, 2026
563faf7
Remove attr_accessible for referrer_filter
mayorova Mar 9, 2026
b4c8211
Migrate access tokens controller to strong params
mayorova Mar 9, 2026
30d5255
Remove attr_accessible for line_item
mayorova Mar 9, 2026
24a509b
Migrate invoice to strong parameters
mayorova Mar 9, 2026
0ddc0c5
Migrate signup flow to strong parameters
mayorova Mar 10, 2026
269a25e
Fix invoice controller
mayorova Mar 10, 2026
81f7063
Further fixes for account create/update
mayorova Mar 11, 2026
ca63300
Fix CMS groups update and add a test
mayorova Mar 11, 2026
17c7232
Small fixes
mayorova Mar 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/controllers/admin/api/application_plan_metric_limits_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ def usage_limit
end

def usage_limit_params
params.fetch(:usage_limit)
params.fetch(:usage_limit).permit(:period, :value)
end

end
32 changes: 22 additions & 10 deletions app/controllers/admin/api/buyers_users_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
class Admin::Api::BuyersUsersController < Admin::Api::BuyersBaseController
representer User

before_action :find_user, except: %i[create index]
before_action :build_new_user, only: %i[create]

attr_reader :user

# User List
# GET /admin/api/accounts/{account_id}/users.xml
def index
Expand All @@ -12,11 +17,9 @@ def index
# User Create
# POST /admin/api/accounts/{account_id}/users.xml
def create
user = new_user

authorize! :create, user

user.unflattened_attributes = flat_params
user.unflattened_attributes = user_params
user.signup_type = :api

user.save
Expand All @@ -37,7 +40,7 @@ def show
def update
authorize! :update, user

user.update_with_flattened_attributes(flat_params)
user.update_with_flattened_attributes(user_params)

respond_with(user)
end
Expand Down Expand Up @@ -108,19 +111,28 @@ def authorize!(*args)
current_user ? super : logged_in?
end

def new_user
@new_user ||= buyer.users.new
end

def users
@users ||= begin
conditions = params.slice(:state, :role)
buyer.users.where(conditions)
end
end

def user
@user ||= buyer.users.find(params[:id])
private

def build_new_user
@user = buyer.users.new
end

def find_user
@user = buyer.users.find(params[:id])
end

def user_params
@user_params ||= begin
allowed_attrs = user.defined_fields_names | %i(password password_confirmation)
flat_params.permit(*allowed_attrs)
end
end

end
2 changes: 1 addition & 1 deletion app/controllers/admin/api/providers_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ def show
# Provider Account Update
# PUT /admin/api/provider.xml
def update
current_account.update(provider_params, without_protection: true)
current_account.update(provider_params)
respond_with current_account
end

Expand Down
20 changes: 17 additions & 3 deletions app/controllers/admin/api/signups_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,30 @@ class Admin::Api::SignupsController < Admin::Api::BaseController
def create
authorize!(:create, Account) if current_user

@signup_result = Signup::DeveloperAccountManager.new(current_account).create(signup_params)
@signup_result = account_manager.create(signup_params)

check_creation_errors
respond_with(@signup_result.account, user_options: { with_apps: true })
end

private

def account_manager
@account_manager ||= Signup::DeveloperAccountManager.new(current_account)
end

def user_params
flat_params.merge({signup_type: :minimal})
@user_params ||= begin
allowed_attrs = account_manager.user.defined_fields_names | %i(password signup_type)
flat_params.permit(*allowed_attrs).merge({signup_type: :minimal})
end
end

def account_params
@account_params ||= begin
allowed_attrs = account_manager.account.defined_fields_names - %i(billing_address country) + %i(country_id)
flat_params.permit(*allowed_attrs)
end
end

def check_creation_errors
Expand All @@ -31,7 +45,7 @@ def check_creation_errors
end

def signup_params
Signup::SignupParams.new(plans: plans, user_attributes: user_params, account_attributes: flat_params, defaults: defaults)
Signup::SignupParams.new(plans: plans, user_attributes: user_params, account_attributes: account_params, defaults: defaults)
end

def defaults
Expand Down
30 changes: 20 additions & 10 deletions app/controllers/admin/api/users_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@ class Admin::Api::UsersController < Admin::Api::BaseController
representer User

before_action :can_create, only: :create
before_action :build_new_user, only: %i[create]
before_action :find_user, except: %i[create index]

attr_reader :user

# User List (provider account)
# GET /admin/api/users.xml
Expand All @@ -14,11 +18,9 @@ def index
# User Create (provider account)
# POST /admin/api/users.xml
def create
user = new_user

authorize! :create, user

user.unflattened_attributes = flat_params
user.unflattened_attributes = user_params
user.signup_type = :api

user.save
Expand All @@ -39,7 +41,7 @@ def show
def update
authorize! :update, user

user.update_with_flattened_attributes(flat_params, as: current_user.try(:role))
user.update_with_flattened_attributes(user_params)

respond_with(user)
end
Expand Down Expand Up @@ -110,19 +112,19 @@ def authorize!(*args)
current_user ? super : logged_in?
end

def new_user
@new_user ||= current_account.users.new
end

def users
@users ||= begin
conditions = params.slice(:state, :role)
current_account.users.but_impersonation_admin.where(conditions)
end
end

def user
@user ||= current_account.users.but_impersonation_admin.find(params[:id])
def build_new_user
@user = current_account.users.new
end

def find_user
@user = current_account.users.but_impersonation_admin.find(params[:id])
end

def can_create
Expand All @@ -134,4 +136,12 @@ def can_create
def flat_params
super.except(:id)
end

def user_params
@user_params ||= begin
allowed_attrs = user.defined_fields_names | %i(password password_confirmation cas_identifier)
allowed_attrs |= [member_permission_service_ids: [], member_permission_ids: [], allowed_sections: [], allowed_service_ids: []] if (provider_key.present? || current_user.admin?)

@mayorova mayorova Mar 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is used instead of the combination of these:

user.update_with_flattened_attributes(flat_params, as: current_user.try(:role))

attr_accessible :member_permission_service_ids, :member_permission_ids, as: %i[admin]

When provider_key is used (i.e. current_user is nil), the fields can be updated (verified by a new test).

flat_params.permit(*allowed_attrs)
end
end
end
4 changes: 0 additions & 4 deletions app/controllers/api/metric_visibilities_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,4 @@ def find_metric
def authorize_section
authorize! :manage, :plans
end

def usage_limit_params
params.require(:usage_limit)
end
end
2 changes: 1 addition & 1 deletion app/controllers/api/usage_limits_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,6 @@ def authorize_action
end

def usage_limit_params
params.require(:usage_limit)
params.require(:usage_limit).permit(:period, :value)
end
end
23 changes: 18 additions & 5 deletions app/controllers/buyers/accounts_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ class Buyers::AccountsController < Buyers::BaseController

before_action :set_plans, :only => %i[new create]
before_action :find_account, except: %i[index new create]
before_action :init_signup_account_manager, only: %i[create]

activate_menu :buyers, :accounts, :listing

Expand Down Expand Up @@ -41,7 +42,7 @@ def update
end

def create
signup_result = Signup::DeveloperAccountManager.new(current_account).create(signup_params)
signup_result = @account_manager.create(signup_params)
@buyer = signup_result.account

if signup_result.persisted?
Expand Down Expand Up @@ -84,7 +85,7 @@ def show

protected

attr_reader :account
attr_reader :account, :user

def find_account
with_deleted = %w[show resume].include?(action_name)
Expand All @@ -109,13 +110,19 @@ def signup_params
Signup::SignupParams.new(plans: [], user_attributes: user_params.merge(signup_type: :created_by_provider), account_attributes: account_params, validate_fields: false)
end

# TODO: using `permit` later
def account_params
@account_params ||= params.require(:account).except(:user)
@account_params ||= begin
allowed_attrs = account.defined_builtin_fields_names - %i(billing_address country) + %i(country_id)
params.require(:account).permit(*allowed_attrs, extra_fields: account.defined_extra_fields_names)
end
end

def user_params
params.require(:account).fetch(:user, {})
@user_params ||= begin
allowed_attrs = user.defined_builtin_fields_names | %i(password signup_type)
params.require(:account).fetch(:user, ActionController::Parameters.new)
.permit(*allowed_attrs, extra_fields: user.defined_extra_fields_names)
end
end

def set_plans
Expand All @@ -131,4 +138,10 @@ def presenter
user: current_user,
params: params)
end

def init_signup_account_manager
@account_manager = Signup::DeveloperAccountManager.new(current_account)
@account = @account_manager.account
@user = @account_manager.user
end
end
6 changes: 5 additions & 1 deletion app/controllers/buyers/invoices_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ def create
def update
@invoice = @account.invoices.find(params[:id])

if @invoice.update(params[:invoice])
if @invoice.update(invoice_params)
redirect_to admin_buyers_account_invoice_url(@account, @invoice), success: t('.success')
else
render :edit
Expand All @@ -52,4 +52,8 @@ def update
def find_account
@account = current_account.buyers.find(params[:account_id])
end

def invoice_params
params.require(:invoice).permit(:period)
end
end
13 changes: 9 additions & 4 deletions app/controllers/buyers/users_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ def update
# TODO: I think this controller is used only on provider side
user.validate_fields! if current_account.buyer?

user.attributes = user_params
user.attributes = permitted_user_params
user.role = user_params.fetch(:role, user.role) if can?(:update_role, user)

if user.save
Expand Down Expand Up @@ -80,10 +80,15 @@ def find_user
@user = @account.users.find(params[:id]).decorate
end

DEFAULT_PARAMS = %i[username email password password_confirmation role].freeze

@mayorova mayorova Mar 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic was actually problematic.
There are some fields under User model that are "standard", but not required:

optional_fields_are :title, :first_name, :last_name, :job_role

When they are added in FieldsDefinitions, the fields appear in the Edit form and can be updated. Here is what the params.require(:user) looks like in this case:

{"username"=>"uu222", "email"=>"uu12@example.com", "extra_fields"=>{"userhidden"=>"value 2", "userhiddenro"=>"a", "userro"=>"a", "userreq"=>""}, "title"=>"aa", "first_name"=>"bb", "last_name"=>"cc", "job_role"=>"dd", "password"=>"", "password_confirmation"=>"", "role"=>"member"}

However, the further permit filters them out in a way, that only the ones listed in DEFAULT_PARAMS are kept, along with the extra_fields, however, the extra_fields only contain the actually custom fields, so the user_params results in:

{"username"=>"uu222", "email"=>"uu12@example.com", "password"=>"", "password_confirmation"=>"", "role"=>"member", "extra_fields"=>#<ActionController::Parameters {"userhidden"=>"value 2", "userhiddenro"=>"a", "userro"=>"a", "userreq"=>""} permitted: true>}

Hence, title, first_name, last_name and job_role can never be updated through the UI - I think it's a bug.

@akostadinov akostadinov Mar 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, these special fields ('title', 'first_name', 'last_name' and 'job_role'), if defined as extra fields, need to be accepted at the lower level, not under extra_fields.

This looks like a bug in either UI or in the way params are permitted.

I would probably rather fix how UI pass them down but also checking for them and permitting them on the lower level is also good. I assume on the API side they already have to be passed udder extra fields?

@mayorova mayorova Mar 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, it depends on whether it's a UI controller or API.
In the API, all parameters (standard and custom) are accepted at the same level (as user attributes). In the UI, they are nested under extra_fields.

The comment is not about that, though.

@akostadinov akostadinov Mar 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the comment about then?

@mayorova mayorova Mar 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, the issue is not about where the parameters appear - at the "root" user level, or under extra_fields. As I explained, in the UI forms only truly custom (created by user) fields appear under extra_fields, others are at the "root" level along with username and password (they are actually normal attributes, that have a corresponding column in the DB).

The problem is that this DEFAULT_PARAMS doesn't include the "optional" fields - :title, :first_name, :last_name, :job_role. So, even if they are defined through the fields definitions, they can not be updated through the UI, because they are removed from the list of permitted user_params.

@mayorova mayorova Mar 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hang on, once I fix the tests and push the latest updates, I'll explain the new implementation. It has some drawbacks too, but hopefully it can work.

@akostadinov akostadinov Mar 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this explanation still sounds the same. And basically the fix is to either:

  • make the UI form send these special params under extra_fields where they will be permitted due to them being defined as extra fields
  • add these special params conditionally or undonditionally to the permitted user level params

@mayorova mayorova Mar 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope this comment will clarify things 🙏 https://github.com/3scale/porta/pull/4238/changes#r2895692403


def user_params
@user_params ||= params.require(:user).permit(*DEFAULT_PARAMS, extra_fields: [*user.defined_extra_fields_names])
@user_params ||= params.require(:user)
end

def permitted_user_params
@permitted_user_params ||= begin
allowed_attrs = user.defined_builtin_fields_names | %i(password password_confirmation)

@mayorova mayorova Mar 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, basically, the solution I've chosen for now is to rely on the FieldsDefinitions defined for the corresponding model (User in this case).

I'll try to explain...

So, here is how the fields are defined for User.

When a provider is created, the default fields definitions are created automatically, for User these are username and email (they are also required).
There are some other fields that are listed as optional. These appear in the dropdown in FieldsDefinitions, they are standard fields (and have corresponding columns in the users table), but are not created as Fields Definitions by default.

The union of required and optional fields (plus the "internal" ones - but there is only one for the Account model, not user) is referred to as "builtin" fields, as opposed to "extra" fields, that are completely custom and created by the user.

So, I decided, rather than list the hard-coded list of fields (which, apart from username and email may or may not be among the User model fields), just take the actual builtin fields defined for the User model.

There is a small downside in this approach - and it is that I had to adjust some tests to ensure that the default Fields Definitions are actually created (for example, with :simple_provider factory they are not), because without them even the default username and email are not returned. This should not cause problems in the actual application though, where the Fields Definitions should be correctly present always.

Also, it's worth noting that for the UI controllers I am splitting the #defined_builtin_fields_names which are at "root" level under "user" param, and #defined_extra_fields_names which go nested within extra_fields - this is how the fields appear in the UI forms.

In API controllers, on the contrary, all attributes - custom and "standard" appear at the same level, so for API controller params I'm using #defined_fields_names, which includes both "builtin" and "extra"/custom.

There is also the role attribute, which can only be modified under some conditions, so it's not included in the "permitted params" list - it's always handled separately.

@akostadinov akostadinov Mar 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is to accept all params flat at the user level, correct? This is fine with me.

@mayorova mayorova Mar 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is to accept all params flat at the user level, correct? This is fine with me.

No... please read again - nesting under extra_fields for UI controllers remains untouched. Because of this difference in the UI controller params I split the params list in two parts:

allowed_attrs = user.defined_builtin_fields_names | %i(password password_confirmation)
user_params.permit(*allowed_attrs, extra_fields: user.defined_extra_fields_names)

So, built-in and extra/custom treated separately. I don't see a point in changing this and resulting in many more changes - both in controllers and views.

@akostadinov akostadinov Mar 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thanks! That's also totally fine, I somehow looked only at this line and missed extra_fields: in the next line.

user_params.permit(*allowed_attrs, extra_fields: user.defined_extra_fields_names)
end
end

def redirect_back_or_show_detail(**opts)
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/finance/api/invoices_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ def charge
# Invoice Update
# PUT /api/invoices/{id}.xml
def update
invoice.update(invoice_params_update, without_protection: true)
invoice.update(invoice_params_update)
respond_with(invoice)
end

Expand Down
6 changes: 5 additions & 1 deletion app/controllers/finance/provider/invoices_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ def create
end

def update
if @invoice.update(params[:invoice])
if @invoice.update(invoice_params)
redirect_to admin_finance_invoice_url(@invoice), success: t('.success')
else
render :edit
Expand Down Expand Up @@ -96,4 +96,8 @@ def find_buyer(options = {})
def find_invoice
@invoice = collection.find(params[:id])
end

def invoice_params
@invoice_params ||= params.require(:invoice).permit(:friendly_id, :period)
end
end
Loading