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

Fix account enumeration user account #36

Closed
wants to merge 2 commits into from

Conversation

cbldev
Copy link
Contributor

@cbldev cbldev commented Sep 11, 2023

Like the Devise paranoid mode: ensure the application returns consistent generic error messages in response to invalid account user email during the log in process.

Following Testing for Account Enumeration and Guessable User Account.

@abevoelker
Copy link
Owner

I like this idea! However I'd prefer if we kept the old behavior for if Devise.paranoid is false. We can add a new magic_link_sent_paranoid i18n key for your new message. Thoughts?

class Devise::Passwordless::SessionsController < Devise::SessionsController
  def create
    if (self.resource = resource_class.find_by(email: create_params[:email]))
      resource.send_magic_link(remember_me: create_params[:remember_me])
      if Devise.paranoid
        set_flash_message!(:notice, :magic_link_sent_paranoid)
      else
        set_flash_message!(:notice, :magic_link_sent)
      end
    else
      self.resource = resource_class.new(create_params)
      if Devise.paranoid
        set_flash_message!(:notice, :magic_link_sent_paranoid)
      else
        set_flash_message!(:alert, :not_found_in_database, now: true)
        render :new, status: devise_error_status
        return
      end
    end

    redirect_to(after_magic_link_sent_path_for(resource), status: devise_redirect_status)
  end
end

For a completed PR we'll need this:

  • Update the YAML generator to add new magic_link_sent_paranoid key (look for update_devise_yaml method) and update the generator spec so that gem tests pass (bundle exec rake)
  • Add feature tests to spec/dummy_app_config/shared_source/all/spec/system to test the new branching logic works on all paths (can test locally with https://github.com/nektos/act using act -j test, or pushing new commits to this PR branch should now re-run workflow tests now that I've approved them I think)
  • Add magic_link_sent_paranoid to YAML examples in README with a note about paranoid mode
  • This will be a breaking change + new behavior so add a bullet point with instructions to UPGRADING.md
  • Add a bullet point to CHANGELOG.md enhancements section and thank yourself

If you want to tackle that yourself, go ahead and I'll merge when complete! Otherwise I may not get to it until middle of the week or so

@abevoelker
Copy link
Owner

abevoelker commented Sep 15, 2023

This has now been implemented via c75933b

Closing this as complete - thank you for your contribution! 🤘

@abevoelker abevoelker closed this Sep 15, 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