-
Notifications
You must be signed in to change notification settings - Fork 494
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
Support Devise paranoid mode #310
base: main
Are you sure you want to change the base?
Conversation
The overridden methods in UserPasswordsController were inadvertently missing out on Devise's "paranoid mode", which protects against user enumeration attacks. Current dependency is Devise ~> 3.4.1 according to the gemspec, which calls a separate method to determine redirect location. All existing specs still pass. Added a spec for nonexistent users. The situation with existing users requires a good deal more setup since it would trigger the e-mail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rebase against master & fix comments
expect(flash[:notice]).to eq I18n.t(:send_paranoid_instructions, scope: [:devise, :user_passwords, :spree_user]) | ||
end | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove empty line
end | ||
|
||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove empty line
|
||
context 'with paranoid mode' do | ||
before { Devise.paranoid = true } | ||
after { Devise.paranoid = false } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add empty line after after
|
||
context 'when resetting password' do | ||
it 'puts an error on the object' do | ||
spree_post :create, spree_user: {email: '[email protected]'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add empty line after spree_post
& add space after hash key and hash value
before { Devise.paranoid = true } | ||
after { Devise.paranoid = false } | ||
it 'does not indicate whether the user exists' do | ||
spree_post :create, spree_user: {email: '[email protected]'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add empty line & fix hash formatting
it 'does not indicate whether the user exists' do | ||
spree_post :create, spree_user: {email: '[email protected]'} | ||
expect(response).to redirect_to spree.login_path | ||
expect(flash[:notice]).to eq I18n.t(:send_paranoid_instructions, scope: [:devise, :user_passwords, :spree_user]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use %i[]
instead of [:a, :b]
spree_post :create, spree_user: {email: '[email protected]'} | ||
expect(response).to be_success | ||
expect(assigns(:spree_user).kind_of?(Spree::User)).to eq true | ||
expect(assigns(:spree_user).errors.messages[:email].first).to eq I18n.t(:not_found, scope: [:errors, :messages]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use %i[]
instead of [:a, :b]
The overridden methods in UserPasswordsController were inadvertently
missing out on Devise's "paranoid mode", which protects against user
enumeration attacks.
Current dependency is Devise ~> 3.4.1 according to the gemspec, which
calls a separate method to determine redirect location.
All existing specs still pass. Added a spec for nonexistent users. The
situation with existing users requires a good deal more setup since it
would trigger the e-mail.