[WIP] [PoC] Migrate from protected attributes to strong parameters by mayorova · Pull Request #4238 · 3scale/porta

mayorova · 2026-02-27T18:55:20Z

Closed in favor of #4248 and #4249

What this PR does / why we need it:

WIP, please do not review yet.

Which issue(s) this PR fixes

TBD

Verification steps

Special notes for your reviewer:

mayorova · 2026-03-05T17:16:17Z

+    @user_params ||= begin
+                       # User.builtin_fields: %w(username email title first_name last_name job_role)
+                       allowed_attrs = User.builtin_fields | user.defined_extra_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?)


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).

mayorova · 2026-03-05T17:45:00Z

    @user = @account.users.find(params[:id]).decorate
  end

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


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.

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?

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.

What is the comment about then?

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.

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.

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

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

mayorova · 2026-03-05T17:55:59Z

    assert user.reload.active?
  end
+
+  test "update password" do


Some new tests to verify that the new behavior matches the previous one in master.

mayorova · 2026-03-06T13:08:56Z

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


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.

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

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.

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

mayorova · 2026-03-06T13:14:50Z

+    @permitted_user_params ||= begin
+                                 allowed_attrs = user.defined_builtin_fields_names | %i(password password_confirmation)
+                                 # TODO: are these parameters needed?
+                                 # allowed_attrs |= %i(conditions cas_identifier open_id service_conditions)


Here and in all other commented-out lines - these attributes appear in the original attr_accessible:

porta/app/models/user.rb

Line 113 in 284d16c

:conditions, :cas_identifier, :open_id, :service_conditions,

However, I couldn't find information about the intended usage for these attributes. I think most of them are legacy and unused - possibly open_id is used for the hidden legacy Partner signups, and cas_identifier appeared in one test, but is probably also dead code.

I have listed them commented out just to provide this explanation and here your thoughts on these, but my idea is actually to remove these lines, because I think we don't need them.

mayorova · 2026-03-06T13:34:15Z

    defined_extra_fields.map(&:name)
  end

+  def defined_fields_names


Probably need unit tests for these new methods.

mayorova · 2026-03-06T13:38:32Z


  def build_user
-    @user = @invitation.make_user(filter_readonly_params(params[:user], User))
+    allowed_attrs = @invitation.account.users.new.defined_fields_names | %i(password password_confirmation)


Well, this is very ugly, I know :(
But I am not sure what's a better way to do it... An option could be to delegate the whole thing to filter_readonly_params - it goes to @invitation.account.users anyway, because it's the base collection for the new object.
However, I think that would be more obscure... Maybe refactor #make_user to first only build an empty user and return it, and then have another method to assign the required attributes?..

Or maybe the current way it not too bad?.. opinions welcome!

I think either modify #make_user to first generate an empty user.

Alternatively ai suggested:

account.fields_definitions.by_target('user').select { |f| f.extra? }

But I think creating the empty object and then getting and setting the values is more straightforward.

Creating a whole user object just to get the fields just feels wrong.

mayorova · 2026-03-09T12:55:00Z

    else
      user_data = strategy.user_data || {}
-      @user.assign_attributes(user_data.to_hash.compact)
+      user_attributes = user_data.to_hash.compact.slice(:username, :email)


I couldn't figure out what would be the scenario where we'd end up in this condition branch, and still user_data would be a real hash.
But based on the following this seems to be a reasonable thing to do:

From the ATTRIBUTES of user_data, only :username, :email and :authentication_id are the attributes that belong to the User model.

From the 3 attributes above, only :username and :email were included in the attr_accessible previously.

If tests pass, good enough.

Actually, as I was replying to this other comment: https://github.com/3scale/porta/pull/4249/changes#r2926129192 I realized, that this slice is not needed, because #to_hash already extracts just these two fields.

mayorova · 2026-03-11T01:27:50Z

    assert_equal signup_params[:org_name], account.name
    assert_equal signup_params[:org_name].downcase, account.subdomain
    assert_equal "#{signup_params[:org_name].downcase}-admin", account.self_subdomain
+    assert_equal signup_params[:from_email], account.from_email


This is a small change with respect to the original behavior - previously the :from_email field couln't be set on the create call, because it was a protected attribute. :support_email, however, could be set fine.

I think this is not very consistent, so now both (and other email fields) can be set on tenant creation.

mayorova · 2026-03-11T01:36:08Z

+    assert_equal 'approved', provider.state
    assert_not_equal update_params[:account][:from_email], provider.from_email
-    assert_equal 'approved',                           provider.state
+    assert_not_equal 'new-org-name', provider.org_name


This is different from the original behavior. So, the idea of the feature (from what I understand) is that when an account is scheduled fofr deletion, no changes to its fields are accepted, the only allowed action was resuming (updating :state_event attribute).

However, on master the restriction only affected a subset of attributes - the provider-specific [:from_email, :support_email, :finance_support_email, :site_access_code, :state_event, {annotations: {}}]. The standard fields could be updated fine.

I chose to restrict the modifications further, to make the behavior more consistent, and also in line with the UI which doesn't even show the "Edit" for when account is scheduled for deletion.

jlledom · 2026-03-11T16:03:58Z

    user.class.transaction do
      user.assign_attributes(attributes)
-      user.assign_attributes(protected_attributes, without_protection: can?(:update_role, user))
+      user.assign_attributes(protected_attributes) if can?(:update_role, user)


Mmm, this is not exactly the same, but I guess it'll work the same way, just without the warning in the logs in the case the condition is false.

Also, maybe we should rename it, protected_attributes sounds old.

jlledom · 2026-03-11T16:06:31Z

+    @user = @invitation.make_user
+    allowed_attrs = @user.defined_fields_names | %i(password password_confirmation)
+    user_params = params.fetch(:user, ActionController::Parameters.new).permit(*allowed_attrs)
+    @user.assign_attributes(user_params)


Just a nitpick, maybe it'd be better to extract it to its own method user_params to be consistent with the rest of the codebase.

Well, actually, the difference here is that we rely on @user that is set in the first line of the method. But we can pass it as argument to #user_params - it will not be the same as in other controllers, but I guess is still useful, to find the method quickly.

It should work like any other controller, for #create, create an empty User instance in the before_action callback or in the action itself, then assign the attributes by passing user_params as parameter.

I did it here: 1cd9a16

jlledom · 2026-03-11T16:15:32Z

    else
      user_data = strategy.user_data || {}
-      @user.assign_attributes(user_data.to_hash.compact)
+      user_attributes = user_data.to_hash.compact.slice(:username, :email)


If tests pass, good enough.

jlledom

A better review, AI assisted.

jlledom · 2026-03-11T16:51:01Z

  def update_resource(user, attributes)
    attributes.each do |attrs|
-      user.attributes = filter_readonly_params(attrs, User)
+      user.attributes = filter_readonly_params(attrs, User).permit!


Should we permit everything here?

Fixed in d8e75bc

jlledom · 2026-03-11T16:56:00Z

This suite is broken

jlledom · 2026-03-11T17:07:16Z

-    def build_account_with_attributes_for_provider_account(provider_account)
-      account = provider_account.buyers.new
-      account.validate_fields! if validate_fields
-      account.unflattened_attributes = account_attributes
-      vat_rate = account_attributes[:vat_rate]
-      account.vat_rate = vat_rate.to_f if vat_rate
-      account
-    end


Where did you move this to? and in particular, where is the vat_rate assignment happening now? Is it converted to float?

akostadinov reviewed Feb 27, 2026

View reviewed changes

Comment thread app/models/concerns/strong_params_permittable.rb Outdated

mayorova mentioned this pull request Mar 2, 2026

THREESCALE-12076: Upgrade Rails to 7.2 #4232

Open

mayorova force-pushed the strong-params-for-all branch 2 times, most recently from 723312f to ae41fa1 Compare March 2, 2026 18:07

mayorova commented Mar 5, 2026

View reviewed changes

mayorova added 2 commits March 5, 2026 23:27

Migrate protected attrs to strong params for User-related controllers

4ca06ef

Update test for User-related controllers

f30629d

mayorova force-pushed the strong-params-for-all branch from e0f0643 to f30629d Compare March 5, 2026 22:27

Fix tests related to user model

cbe7b54

mayorova commented Mar 6, 2026

View reviewed changes

mayorova added 2 commits March 6, 2026 15:20

Fix rspecs broken by duplicate field definitions creation

97be0c3

Fix more tests and refactor invitation signup flow

305efc5

mayorova force-pushed the strong-params-for-all branch from ca549bd to cf9d01f Compare March 9, 2026 12:52

Remove TODO comments

60f881c

mayorova force-pushed the strong-params-for-all branch from cf9d01f to 60f881c Compare March 9, 2026 12:53

mayorova commented Mar 9, 2026

View reviewed changes

mayorova added 6 commits March 9, 2026 14:33

Migrate to strong parameters for usage limits

a4a6540

Remove attr_accessible for application_key

dc62875

Remove attr_accessible for referrer_filter

563faf7

Migrate access tokens controller to strong params

b4c8211

Remove attr_accessible for line_item

30d5255

Migrate invoice to strong parameters

24a509b

mayorova force-pushed the strong-params-for-all branch 2 times, most recently from 1283f56 to 3304bae Compare March 10, 2026 11:30

Migrate signup flow to strong parameters

0ddc0c5

Fix invoice controller

269a25e

mayorova force-pushed the strong-params-for-all branch from 3304bae to 269a25e Compare March 10, 2026 17:33

Further fixes for account create/update

81f7063

mayorova force-pushed the strong-params-for-all branch from b5be795 to 81f7063 Compare March 11, 2026 01:25

mayorova commented Mar 11, 2026

View reviewed changes

mayorova added 2 commits March 11, 2026 15:03

Fix CMS groups update and add a test

ca63300

Small fixes

17c7232

jlledom reviewed Mar 11, 2026

View reviewed changes

mayorova closed this Mar 17, 2026

mayorova deleted the strong-params-for-all branch June 4, 2026 15:00

Conversation

mayorova commented Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlledom left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

mayorova commented Feb 27, 2026 •

edited

Loading