RedHatInsights / insights-api-common-rails

Header, Encryption, RBAC, Serialization, Pagination and other common behavior for Insights microservices built with Rails
Apache License 2.0
3 stars 25 forks source link

Make `Parameters#params_for_update` only include writeable fields in permit-hash #213

Closed lindgrenj6 closed 3 years ago

lindgrenj6 commented 3 years ago

So this was a fun one, after updating Sources API to use the common gem's params_for_update method I had some specs failing.

After digging into it more, the test that was failing was here: https://github.com/lindgrenj6/sources-api/blob/b06b2db350482cd3f7738101bdbf51751362d1a2/spec/requests/api/v3.0/sources_spec.rb#L330-L340

The problem is that the sanctified_permit_param method was returning _every field in the api_docdefinition due to the face that Enum#each always returns the array, e.g. attributes in this case, which is a truthy value. It was allowing any field through. Here is the value of strong_params_hash on the "Source" record when updating:

[1] pry(#<Api::V3x0::SourcesController>)> pp strong_params_hash
["availability_status",
 "created_at",
 "id",
 "imported",
 "last_available_at",
 "last_checked_at",
 "name",
 "source_ref",
 "source_type_id",
 "uid",
 "updated_at",
 "version"]

The intended behavior appeared to be only adding fields that are in the attributes array (e.g. anything that is writeable) would be included, and switching the check from Enum#each to Enum#any? does the trick, since it returns whether or not the attribute can be written. Here is the strong_params_hash after the change:

[1] pry(#<Api::V3x0::SourcesController>)> pp strong_params_hash
["availability_status",
 "imported",
 "last_available_at",
 "last_checked_at",
 "name",
 "source_ref"]

I'm not sure how this change will effect Catalog/Topology, so we should probably run tests against this PR for all repos using it before merging + releasing. It seems like the new behavior is what was intended - but I want to make sure.

lindgrenj6 commented 3 years ago

cc @syncrou

dippy-bot commented 3 years ago

Checked commit https://github.com/lindgrenj6/insights-api-common-rails/commit/23dab65bfbaf202486e1837f1c7e148917e3a153 with ruby 2.5.7, rubocop 0.82.0, haml-lint 0.35.0, and yamllint 1 file checked, 0 offenses detected Everything looks fine. :cookie: