Skip to content

Only allow email parameter in SessionsController#new#7690

Merged
mitchellhenke merged 1 commit intomainfrom
mitchellhenke/explicit-password-hashing
Jan 24, 2023
Merged

Only allow email parameter in SessionsController#new#7690
mitchellhenke merged 1 commit intomainfrom
mitchellhenke/explicit-password-hashing

Conversation

@mitchellhenke
Copy link
Contributor

@mitchellhenke mitchellhenke commented Jan 24, 2023

🛠 Summary of changes

Following some of the changes in #7621 and #7657, we noticed that the password= function can unnecessarily digest and encrypt the password, which is an expensive operation. This PR overrides the sign_in_params method from Devise so that we only pass the email parameter to avoid an unnecessary call to password=

@mitchellhenke mitchellhenke force-pushed the mitchellhenke/explicit-password-hashing branch from 4b83856 to 5d9eeb8 Compare January 24, 2023 18:08
@mitchellhenke mitchellhenke force-pushed the mitchellhenke/explicit-password-hashing branch from 5d9eeb8 to 0aef49d Compare January 24, 2023 20:22
@mitchellhenke mitchellhenke changed the title Require explicitly digesting and encrypting password Only allow email parameter in SessionsController#new Jan 24, 2023
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For any auth attempt, we want to run an scrypt hash exactly once.

This and the line above are helpful because they catch both the case where we may hash multiple times and the case where we hash zero times.

changelog: Internal, Performance, Only allow email parameter in sessions controller
@mitchellhenke mitchellhenke force-pushed the mitchellhenke/explicit-password-hashing branch from 0aef49d to 55765d8 Compare January 24, 2023 20:45
@mitchellhenke mitchellhenke marked this pull request as ready for review January 24, 2023 20:45
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

@mitchellhenke mitchellhenke merged commit 634448c into main Jan 24, 2023
@mitchellhenke mitchellhenke deleted the mitchellhenke/explicit-password-hashing branch January 24, 2023 21:14
@mdiarra3 mdiarra3 mentioned this pull request Jan 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants