THREESCALE-12434: Migrate from protected attributes to strong parameters - Part 2#4249
THREESCALE-12434: Migrate from protected attributes to strong parameters - Part 2#4249mayorova wants to merge 8 commits into
Conversation
jlledom
left a comment
There was a problem hiding this comment.
Some comments come from the other PR. Also, I still see a lot of
38e277c to
db25ecf
Compare
| @account_params ||= begin | ||
| defined_fields_names = buyer_account.defined_fields_names | ||
| allowed_attrs = defined_fields_names - %w(billing_address) + %w(name) | ||
| nested_params = { extra_fields: buyer_account.defined_extra_fields_names } |
There was a problem hiding this comment.
So, this is different from other API controller, which have only plain parameters, i.e. not nested under extra_fields.
I only did this because there was an existing test in test/integration/admin/api/accounts_controller_test.rb which was passing extra params in this way:
params: update_params.merge(extra_fields: { my_field: 4 })
I also added a check that plain parameters would also work.
Now I'm not sure if other API controllers need to accept both ways too 🤔
a676b21 to
8b5a6de
Compare
0b8ead4 to
6c69623
Compare
498f50b to
706cacb
Compare
a08b299 to
d3304a9
Compare
e2771a2 to
1abbc5c
Compare
❌ 26 blocking issues (26 total)
@qltysh one-click actions:
|
279e452 to
7981703
Compare
4b5523f to
5f946c2
Compare
| application.unflattened_attributes = application_params | ||
| application.user_key = params[:user_key] if params[:user_key] | ||
| application.application_id = params[:application_id] if params[:application_id] | ||
| application.assign_attributes(application_params) |
There was a problem hiding this comment.
user_key and application_id used to be protected attributes. It is not needed to split their assignment anymore. Both are explicitly added to the permitted parameters.
| def application_attributes | ||
| current_account.fields.for(Cinstance) + %w|user_key application_id| | ||
| allowed_attrs = current_account.defined_fields_names_for(Cinstance) + | ||
| %w[user_key application_id redirect_url first_traffic_at first_daily_traffic_at] |
There was a problem hiding this comment.
redirect_url first_traffic_at first_daily_traffic_at are part of the old current_account.fields.for(Cinstance)
| end | ||
|
|
||
| def account_params | ||
| allowed_attrs = current_account.defined_fields_names_for(Account) - %w[billing_address] |
There was a problem hiding this comment.
Setting billing_address as a plain value (e.g. a string) is not supported.
Not sure, however, if we need to add the complex handling like in app/controllers/admin/api/accounts_controller.rb
There was a problem hiding this comment.
Was it possible before?
There was a problem hiding this comment.
According to Claude, it accepted a hash before, so if we want to preserve old behavior, we must implement it like this:
def account_params
allowed_attrs = current_account.defined_fields_names_for(Account) - %w[billing_address]
nested = { annotations: {} }
if current_account.defined_fields_names_for(Account).include?('billing_address')
nested[:billing_address] = %i[name address1 address2 city country state zip phone]
end
params.permit(*allowed_attrs, **nested)
endHowever, I'm fine with removing billing address entirely from the signup controller unless we know a scenario where this would be required.
There was a problem hiding this comment.
I made a test for signups controller in master (similar to the one for the account controller), and unfortunately, the same behavior is supported... both nested hash and individual fields work for setting billing address.
So, I'll have to add the same logic.
There was a problem hiding this comment.
maybe admin/api/users_controller.rb as well? This is what Bob thinks
There was a problem hiding this comment.
maybe
admin/api/users_controller.rbas well? This is what Bob thinks
admin/api/users_controller.rb is for provider's users, so we don't need billing address there.
| def flat_params | ||
| super.except(:id) | ||
| def user_params | ||
| defined_fields_names = current_account.provider_account.defined_fields_names_for(User) |
There was a problem hiding this comment.
These endpoints are for managing the users of the provider account (current_account in this case), so we need to check the fields definitions configured on the provider's provider (aka master account), hence current_account.provider_account.
There was a problem hiding this comment.
I wonder if it would be more understandable to have it as Account.master.defined_fields_names_for(User) in this case. But it is fine to stay, just a thought. I'm not sure which is better.
| super.except(:id) | ||
| def user_params | ||
| defined_fields_names = current_account.provider_account.defined_fields_names_for(User) | ||
| permission_attrs = [:member_permission_service_ids, { member_permission_service_ids: [], member_permission_ids: [] }] |
There was a problem hiding this comment.
member_permission_service_ids can be nil (meaning all services are enabled), or an array (including an empty array), so both plain and array-like parameters need to be permitted.
There was a problem hiding this comment.
ideally that would be a code comment for future readers. Although I think we have tests for that so one will notice.
| defined_fields_names = current_account.provider_account.defined_fields_names_for(User) | ||
| permission_attrs = [:member_permission_service_ids, { member_permission_service_ids: [], member_permission_ids: [] }] | ||
| allowed_attrs = defined_fields_names + %w[password password_confirmation cas_identifier] | ||
| allowed_attrs += permission_attrs if provider_key.present? || current_user.admin? |
There was a problem hiding this comment.
Permission attributes were only restricted for admin users here:
attr_accessible :member_permission_service_ids, :member_permission_ids, as: %i[admin]
There was a problem hiding this comment.
A bit worried about this, the security depends on we remembering to always add this check in all controllers that handle these attributes. Is this properly tested?
If we have to add such a check in controller, why not delegating this to cancancan like we do here?
porta/app/controllers/provider/admin/account/users_controller.rb
Lines 61 to 64 in 71e6ead
There was a problem hiding this comment.
I also thought about cancan, this role you found is really nice.
The issue though is this provider_key.present? which, if provider)key was used, results in no user being set thus cancan will reject the operation.
So it will be nice to have it but I would not require it for this PR.
On the other hand, previously provider_key auth was not handled, so not handling it in this PR will not be a regression.
An easy fix might be to set current_user to first admin when provider_key is used. But then this may require more investigation than necessary for this PR.
In summary, I'm ok with all options:
- keep as is
- use cancan and ignore
provider_key - use cancan and alter
provider_keyauth to set an admin user (or whatever alternative solution we can come up with)
There was a problem hiding this comment.
But we already have a ApiAuthentication::ByProviderKey module, I assume if the request came with a provider key, we already use it to get the proper user, and the user will have the proper cancancan permissions, so it should just work without worrying about how the user was authenticated.
There was a problem hiding this comment.
I checked this. When we authenticate via :provider_key, current_user is not set at all, so we can't use cancancan here. In fact, there are a few calls to authorize! in some actions, but the method is overwritten to just check if we are logged in...
What I wonder is: are we implementing all these ugly workarounds in each API controller in order to accept provider keys?
This is what claude says:
● So there are three different strategies in play:
1. Override authorize! to skip cancancan when no user (the 4 controllers we found)
2. Guard with if current_user — services_controller, signups_controller, web_hooks_failures_controller, api_docs_services_controller — cancancan is simply not called for provider_key requests
3. Call authorize! unconditionally — access_tokens_controller, authentication_providers_controller, backend_apis_controller, backend_apis/base_controller, registry/policies_controller, services/backend_usages_controller — these would hit cancancan with a nil user and likely fail on provider_key auth
4. Creative workaround — application_plans_controller builds its own Ability from current_account.admins.first, sidestepping the nil user entirely
This project is gonna kill me.
There was a problem hiding this comment.
This is what I told you :) that current_user is not set. But we can change the module to set first admin for example. Provider key means something like the provider root account. When you lost access to your admin for some reason, you can recover with the provider key.
I think that not so many controllers check user permissions. In all of them though, we would need special logic to handle provider_key auth. Again, a simple workaround might be setting current_user to the first admin in such case.
As this is not a regression though, I would suggest such improvements to be performed within another JIRA issue.
There was a problem hiding this comment.
I understand you now. Yeah, I think setting current_user to the impersonation admin when using provider_key would probably work out of the box. But seems something to be done in a separate PR.
There was a problem hiding this comment.
I don't quite understand what Claude is suggesting... But here are some points:
- I think the implementation was already confusing in the first place. There are two places where
attr_accessibleis set for these fields:
- in
models/user.rb:attr_accessible :member_permission_service_ids, :member_permission_ids, as: %i[admin] - in
models/user/permissions.rb:attr_accessible :member_permission_service_ids, :member_permission_ids, :allowed_sections, :allowed_service_ids
I don't really understand how this is expected to work based on the protected_attributes_continued documentation, Claude and Bob are also confused.
But according to the tests, this is what happens:
user.update_with_flattened_attributes(flat_params, as: current_user.try(:role))
Indeed, if provider_key is used, current_user would be nil, so, assign_attributes will be called with as: nil, and then, the role defaults to :default value.
Probably, because of the missing role in attr_accessible in permissions.rb, :default is actually considered the role that can update the fields due to this default.
So, with as: :admin and as: nil the update works, and with as: :member it doesn't apparently.
-
I ran the test
'set member permissions with provider key'on master, and the behavior is the same as in this branch. The tests'set group permissions as an admin'and'set group permissions as a member'also pass on both branches, so I think that effectively the behavior is the same. -
There are really just two controllers where these fields can potentially be set: the API one (this), and the UI one (
app/controllers/provider/admin/account/users_controller.rb). In the API one we need to handle the provider key explicitly, and in the UI one it is handled usingcan?, because there is always a valid current user.
So, I am not overly worried about:
the security depends on we remembering to always add this check in all controllers that handle these attributes
I am not expecting us to add more controllers for handling these fields.
Conclusion:
I think the current implementation achieves the same behavior as before, so I wouldn't complicate things any more.
There was a problem hiding this comment.
Yeah, not in this PR. What I thought was that in case we already have or add in the future permissions checks in other controllers, it might be worth having a current_user set with provider_key auth as well.
| @account_params ||= params.require(:account).except(:user) | ||
| defined_builtin_fields_names = current_account.defined_builtin_fields_names_for(Account) | ||
| defined_extra_fields_names = current_account.defined_extra_fields_names_for(Account) | ||
| allowed_attrs = defined_builtin_fields_names - %w[billing_address country] + %w[country_id] |
There was a problem hiding this comment.
billing_address and country fields do not even appear in the form as editable:
- for setting the country, there is a dropdown select with
country_idas values billing_addressis showed, but greyed out (it can not be directly modified via UI)
The test 'update account, including optional built-in and custom fields' of test/integration/buyers/accounts_controller_test.rb verifies the behavior for these fields.
| defined_builtin_fields_names = current_account.defined_builtin_fields_names_for(Account) | ||
| defined_extra_fields_names = current_account.defined_extra_fields_names_for(Account) | ||
| allowed_attrs = defined_builtin_fields_names - %w[billing_address country] + %w[country_id] | ||
| params.require(:account).permit(*allowed_attrs, extra_fields: defined_extra_fields_names) |
There was a problem hiding this comment.
Here and in other UI controllers - custom parameters (invented by the user) are submitted inside extra_fields, e.g. account[extra_fields][something]=value.
Other parameters (default or not) are submitted as account's attributes, e.g. account[org_name]=ok or account[vat_rate]=30
| def permitted_user_params | ||
| fields_names = current_account.defined_fields_names_for(User) | ||
| extra_fields_names = current_account.defined_extra_fields_names_for(User) | ||
| user_params.permit(*fields_names, :password, :password_confirmation, |
There was a problem hiding this comment.
We do not need to add role to the list, because it is not assigned massively anyway due to a permission check:
user.role = user_params.fetch(:role, user.role) if can?(:update_role, user)
There was a problem hiding this comment.
But maybe :role was being set in another line precisely because it was a protected attribute, not only because of the permission check. We could also use that permission check to decide whether we permit the attribute or not. The same we do in provider/admin/account/users_controller (see comment above). I don't have a strong opinion, though.
There was a problem hiding this comment.
user_params.fetch(:role, user.role) makes it perhaps harder to handle it like in the other controllers... unless they handle the same problem already, that I don't remember spotting but I might have missed it :)
| private | ||
|
|
||
| def user_params | ||
| flat_params.merge({signup_type: :minimal}) |
There was a problem hiding this comment.
This signup_type: :minimal was moved to signup_params method.
| @user.validate_fields! | ||
|
|
||
| @user.assign_attributes(user_params) | ||
| @user.role = user_params.fetch(:role, @user.role) |
There was a problem hiding this comment.
No need to set role explicitly, because it's not a protected attribute anymore.
| before_action :ensure_signup_possible | ||
|
|
||
| skip_before_action :login_required | ||
| skip_before_action :enable_analytics, only: :test |
There was a problem hiding this comment.
Removed because there is no :test action.
|
|
||
| def signup_params | ||
| Signup::SignupParams.new(plans: [plan], user_attributes: user_params, account_attributes: account_params, validate_fields: true) | ||
| Signup::SignupParams.new(plans: [plan], user_attributes: user_params.merge(signup_type: :new_signup, username: :admin), account_attributes: account_params.merge(sample_data: true), validate_fields: true) |
There was a problem hiding this comment.
Moved signup_type: :new_signup, username: :admin here from user_params.
I am just trying to follow the same pattern everywhere - user_params, account_params etc. just permit the parameters, and we add any extra fields in another place (either signup_params, or in the .update call itself.
Same for sample_data: true)
|
|
||
| def account_params | ||
| params.require(:account).except(:user).merge(sample_data: true) | ||
| allowed_attrs = master.defined_builtin_fields_names_for(Account) + %w[name subdomain self_subdomain] |
There was a problem hiding this comment.
This list (and the one for user params) was derived from checking how the signup form works: https://github.com/3scale/porta/blob/e80983fc2723d4281f688e4b3fdc2b3c5f251ccb/app/lib/fields/signup_form.rb
In any case, now this form is disabled by default both in on-premises and in SaaS:
#4242
| def handle_cache_response | ||
| expires_in 1.hour, public: true | ||
| fresh_when etag: default_params, last_modified: System::Application.config.boot_time | ||
| fresh_when etag: params.permit!.to_h, last_modified: System::Application.config.boot_time |
There was a problem hiding this comment.
Just maintaining the previous behavior.
| self.class.internal_fields | ||
| end | ||
|
|
||
| def special_fields |
There was a problem hiding this comment.
Removing all things related to special fields. They were only used for fetching password password_confirmation attributes or user, but it's much cleaner IMO to just include them in the controller's permitted params list directly.
Especially, as some controllers just accept password, and some others - both password and password_confirmation.
|
|
||
| def plan_id | ||
| @plan_id ||= params.require(:cinstance).permit(:plan_id).tap { |plan_params| plan_params.require(:plan_id) }[:plan_id] | ||
| @plan_id ||= cinstance_params.permit(:plan_id).require(:plan_id) |
There was a problem hiding this comment.
This simplified form seems to be doing the same as the original.
| sso_attributes.all? { |_key, value| value.present? } | ||
| end | ||
|
|
||
| def create_sso_authorization |
There was a problem hiding this comment.
This method was not used anywhere.
| end | ||
|
|
||
| def account_params | ||
| allowed_attrs = site_account.defined_fields_names_for(Account) - %w[country vat_rate] + %w[country_id] |
There was a problem hiding this comment.
- Country is set via a dropdown select, where the values are
country_id, socountry(the actual name of the builtin field) is removed. vat_ratecan not be updated by the developer by design.
|
|
||
| def user_params | ||
| defined_fields_names = current_account.defined_fields_names_for(User) | ||
| params.permit(*defined_fields_names, :password) |
There was a problem hiding this comment.
same question about password_confirmation, should we allow in APIs or not. But it is fine as is.
akostadinov
left a comment
There was a problem hiding this comment.
Awesome stuff! I added some questions but I suspect they are not very valid 👼
| if defined_fields_names.include?('billing_address') | ||
| allowed_attrs += %w[billing_address_name billing_address_address1 billing_address_address2 billing_address_city | ||
| billing_address_country billing_address_state billing_address_zip billing_address_phone] | ||
| nested_params[:billing_address] = %i[name address1 address2 city country state zip phone] |
There was a problem hiding this comment.
Why not same as above?
| nested_params[:billing_address] = %i[name address1 address2 city country state zip phone] | |
| nested_params[:billing_address] = %w[name address1 address2 city country state zip phone] |
There was a problem hiding this comment.
Not sure to be honest... But agree to make it more consistent.
| def contract_params | ||
| allowed_attrs = current_account.defined_fields_names_for(Cinstance) + | ||
| %w[user_key application_id redirect_url first_traffic_at first_daily_traffic_at] | ||
| params.permit(*allowed_attrs) |
There was a problem hiding this comment.
Why adding ["user_key", "application_id"] if those were not permitted before?
| end | ||
|
|
||
| def account_params | ||
| allowed_attrs = current_account.defined_fields_names_for(Account) - %w[billing_address] |
There was a problem hiding this comment.
According to Claude, it accepted a hash before, so if we want to preserve old behavior, we must implement it like this:
def account_params
allowed_attrs = current_account.defined_fields_names_for(Account) - %w[billing_address]
nested = { annotations: {} }
if current_account.defined_fields_names_for(Account).include?('billing_address')
nested[:billing_address] = %i[name address1 address2 city country state zip phone]
end
params.permit(*allowed_attrs, **nested)
endHowever, I'm fine with removing billing address entirely from the signup controller unless we know a scenario where this would be required.
| defined_fields_names = current_account.provider_account.defined_fields_names_for(User) | ||
| permission_attrs = [:member_permission_service_ids, { member_permission_service_ids: [], member_permission_ids: [] }] | ||
| allowed_attrs = defined_fields_names + %w[password password_confirmation cas_identifier] | ||
| allowed_attrs += permission_attrs if provider_key.present? || current_user.admin? |
There was a problem hiding this comment.
A bit worried about this, the security depends on we remembering to always add this check in all controllers that handle these attributes. Is this properly tested?
If we have to add such a check in controller, why not delegating this to cancancan like we do here?
porta/app/controllers/provider/admin/account/users_controller.rb
Lines 61 to 64 in 71e6ead
| def permitted_user_params | ||
| fields_names = current_account.defined_fields_names_for(User) | ||
| extra_fields_names = current_account.defined_extra_fields_names_for(User) | ||
| user_params.permit(*fields_names, :password, :password_confirmation, |
There was a problem hiding this comment.
But maybe :role was being set in another line precisely because it was a protected attribute, not only because of the permission check. We could also use that permission check to decide whether we permit the attribute or not. The same we do in provider/admin/account/users_controller (see comment above). I don't have a strong opinion, though.
| # This is just a sanity guard added when splitting invitation | ||
| # controllers. Remove when SURE. | ||
| raise 'Developer invitation used and worked on provider side!' unless @user.account.provider? |
|
@akostadinov @jlledom hopefully, I addressed your concerns in 3929c52 |
What this PR does / why we need it:
This is part 2 of the migration from protected attributes to strong parameters. See the first part in #4248
Protected attributes is an old Rails feature which was deprecated a long time ago. We were using protected_attributes_continued gem to keep it working, but now it's also discontinued and does not support Rails 7+, so it's a blocker for upgrading to Rails 7.2 for us.
This Part 2 handles the models that can have custom attributes through FieldsDefinitions - Account, User, Cinstance.
Which issue(s) this PR fixes
https://redhat.atlassian.net/browse/THREESCALE-12434
Verification steps
All tests should pass, and all features should work as before.
Special notes for your reviewer: