Skip to content

Conversation

@npalrecha
Copy link
Contributor

@npalrecha npalrecha commented Dec 13, 2018

You are required to use attr_accessible in order to instantiate and set encrypted_password in the same line using .new. encrypted_password shouldn't be mass-assignable. This change removes the necessity of encrypted_password to be mass-assignable.

changed > 0 to .positive?
@olbrich
Copy link
Contributor

olbrich commented Dec 13, 2018

Please add a description/explanation for why this change is necessary / useful.

@shaicoleman
Copy link

This fixes a bug where devise-security skips the password history check in certain cases,
e.g. using the protected_attributes_continued gem and not having :encrypted_password in attr_accessible.

There are other instances in the codebase where there's mass assignment, but this is a start

@olbrich olbrich added this to the 0.14.0 milestone Jan 12, 2019
@olbrich olbrich self-assigned this Jan 12, 2019
@olbrich olbrich added the #️⃣.#️⃣.#️⃣ Patch backwards-compatible bug fixes label Jan 13, 2019
@natebird natebird requested a review from olbrich January 23, 2019 14:34
@dillonwelch
Copy link
Contributor

Can you add a regression test?

@devise-security devise-security deleted a comment from coveralls Mar 2, 2019
@devise-security devise-security deleted a comment from coveralls Mar 2, 2019
@devise-security devise-security deleted a comment from coveralls Mar 2, 2019
Copy link
Contributor

@olbrich olbrich left a comment

Choose a reason for hiding this comment

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

@oniofchaos I don't think additional regression tests are necessary. This method is pretty well covered.

@devise-security devise-security deleted a comment from coveralls Mar 2, 2019
@olbrich olbrich merged commit f68836f into devise-security:master Mar 2, 2019
@npalrecha npalrecha deleted the bugfix/avoid-mass-assignment branch March 11, 2019 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🐛 bug #️⃣.#️⃣.#️⃣ Patch backwards-compatible bug fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants