Skip to content
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.

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

Include the model name when filtering encrypted attributes. #44361

Merged
merged 1 commit into from Feb 25, 2022

Conversation

jorgemanrubia
Copy link
Contributor

@jorgemanrubia jorgemanrubia commented Feb 8, 2022

For example, when encrypting Person#name it will add person.name as a filter parameter, instead of just name. This prevents unintended filtering of parameters with a matching name in other models.

Addresses the second concern in #44330.

CC @rafaelfranca @nvasilevski

Copy link
Contributor

@nvasilevski nvasilevski left a comment

Choose a reason for hiding this comment

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

Thanks ❤️
The change looks good

activerecord/lib/active_record/encryption/configurable.rb Outdated Show resolved Hide resolved
@rafaelfranca
Copy link
Member

This only filters if the parameter is inside a form with the model name right? If anyone set a different name in the form_with the parameter will not be filtered anymore. Is that ok?

@jorgemanrubia
Copy link
Contributor Author

@rafaelfranca yes, you are correct. It will work as long as the resource/param name matches the model/attribute name. I think it's a sensible default, even knowing it won't work in all cases. The previous default was probably too coarse grained. I rebased and fixed conflicts.

@rafaelfranca
Copy link
Member

👍🏽 . There is a failing test though.

@jorgemanrubia jorgemanrubia force-pushed the qualify-encrypted-filter-params branch 3 times, most recently from 3a9f4a1 to d191135 Compare February 11, 2022 09:54
@jorgemanrubia
Copy link
Contributor Author

Fixed that test @rafaelfranca. Build failing in an unrelated error now.

@nvasilevski
Copy link
Contributor

just wanted to bump this PR. Sorry for the noise
The failure does seem unrelated but perhaps worth rebasing just to have a green CI

For example, when encrypting `Person#name` it will add `person.name` as a filter
parameter, instead of just `name`. This prevents unintended filtering of parameters
with a matching name in other models.

Closes rails#44330
@rafaelfranca rafaelfranca merged commit 8137932 into rails:main Feb 25, 2022
rafaelfranca added a commit that referenced this pull request Feb 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants