users: refactor password reset flow.

The old password reset flow:

* User requests a password reset.
* Danbooru generates a password reset nonce.
* Danbooru emails user a password reset confirmation link.
* User follows link to password reset confirmation page.
* The link contains a nonce authenticating the user.
* User confirms password reset.
* Danbooru resets user's password to a random string.
* Danbooru emails user their new password in plaintext.

The new password reset flow:

* User requests a password reset.
* Danbooru emails user a password reset link.
* User follows link to password edit page.
* The link contains a signed_user_id param authenticating the user.
* User changes their own password.
This commit is contained in:
evazion
2020-03-08 21:03:36 -05:00
parent f25bace766
commit 5625458f69
30 changed files with 133 additions and 395 deletions

View File

@@ -1,38 +0,0 @@
module Maintenance
module User
class PasswordResetsController < ApplicationController
def new
@nonce = UserPasswordResetNonce.new
end
def create
@nonce = UserPasswordResetNonce.create(nonce_params)
if @nonce.errors.any?
redirect_to new_maintenance_user_password_reset_path, :notice => @nonce.errors.full_messages.join("; ")
else
redirect_to new_maintenance_user_password_reset_path, :notice => "Email request sent"
end
end
def edit
@nonce = UserPasswordResetNonce.where(:email => params[:email], :key => params[:key]).first
end
def update
@nonce = UserPasswordResetNonce.where(:email => params[:email], :key => params[:key]).first
if @nonce
@nonce.reset_user!
@nonce.destroy
redirect_to new_maintenance_user_password_reset_path, :notice => "Password reset; email delivered with new password"
else
redirect_to new_maintenance_user_password_reset_path, :notice => "Invalid key"
end
end
def nonce_params
params.fetch(:nonce, {}).permit([:email])
end
end
end
end

View File

@@ -0,0 +1,14 @@
class PasswordResetsController < ApplicationController
respond_to :html, :xml, :json
def create
@user = User.find_by_name(params.dig(:user, :name))
UserMailer.password_reset(@user).deliver_later
flash[:notice] = "Password reset email sent. Check your email"
respond_with(@user, location: new_session_path)
end
def show
end
end

View File

@@ -26,6 +26,6 @@ class PasswordsController < ApplicationController
end end
def user_params def user_params
params.require(:user).permit(%i[old_password password password_confirmation]) params.require(:user).permit(%i[signed_user_id old_password password password_confirmation])
end end
end end

View File

@@ -337,7 +337,7 @@ module ApplicationHelper
def nav_link_match(controller, url) def nav_link_match(controller, url)
url =~ case controller url =~ case controller
when "sessions", "users", "maintenance/user/password_resets", "admin/users" when "sessions", "users", "admin/users"
/^\/(session|users)/ /^\/(session|users)/
when "comments" when "comments"

View File

@@ -8,12 +8,12 @@ module Danbooru
@verifier = ActiveSupport::MessageVerifier.new(secret, serializer: JSON, digest: "SHA256") @verifier = ActiveSupport::MessageVerifier.new(secret, serializer: JSON, digest: "SHA256")
end end
def generate(*options) def generate(*args, **options)
verifier.generate(*options, purpose: purpose) verifier.generate(*args, purpose: purpose, **options)
end end
def verified(*options) def verify(*args, **options)
verifier.verified(*options, purpose: purpose) verifier.verify(*args, purpose: purpose, **options)
end end
end end
end end

View File

@@ -19,7 +19,6 @@ module DanbooruMaintenance
end end
def weekly def weekly
safely { UserPasswordResetNonce.prune! }
safely { TagRelationshipRetirementService.find_and_retire! } safely { TagRelationshipRetirementService.find_and_retire! }
safely { ApproverPruner.dmail_inactive_approvers! } safely { ApproverPruner.dmail_inactive_approvers! }
end end

View File

@@ -16,6 +16,8 @@ class SessionLoader
if has_api_authentication? if has_api_authentication?
load_session_for_api load_session_for_api
elsif params[:signed_user_id]
load_param_user(params[:signed_user_id])
elsif session[:user_id] elsif session[:user_id]
load_session_user load_session_user
elsif cookie_password_hash_valid? elsif cookie_password_hash_valid?
@@ -79,6 +81,11 @@ class SessionLoader
end end
end end
def load_param_user(signed_user_id)
session[:user_id] = Danbooru::MessageVerifier.new(:login).verify(signed_user_id)
load_session_user
end
def load_session_user def load_session_user
user = User.find_by_id(session[:user_id]) user = User.find_by_id(session[:user_id])
CurrentUser.user = user if user CurrentUser.user = user if user

View File

@@ -0,0 +1,3 @@
class ApplicationMailer < ActionMailer::Base
default from: "#{Danbooru.config.canonical_app_name} <#{Danbooru.config.contact_email}>", content_type: "text/html"
end

View File

@@ -1,17 +0,0 @@
module Maintenance
module User
class PasswordResetMailer < ActionMailer::Base
def reset_request(user, nonce)
@user = user
@nonce = nonce
mail(:to => @user.email, :subject => "#{Danbooru.config.app_name} password reset request", :from => Danbooru.config.contact_email)
end
def confirmation(user, new_password)
@user = user
@new_password = new_password
mail(:to => @user.email, :subject => "#{Danbooru.config.app_name} password reset confirmation", :from => Danbooru.config.contact_email)
end
end
end
end

View File

@@ -1,10 +1,14 @@
class UserMailer < ActionMailer::Base class UserMailer < ApplicationMailer
add_template_helper ApplicationHelper add_template_helper ApplicationHelper
add_template_helper UsersHelper add_template_helper UsersHelper
default :from => Danbooru.config.contact_email, :content_type => "text/html"
def dmail_notice(dmail) def dmail_notice(dmail)
@dmail = dmail @dmail = dmail
mail(:to => "#{dmail.to.name} <#{dmail.to.email}>", :subject => "#{Danbooru.config.app_name} - Message received from #{dmail.from.name}") mail(:to => "#{dmail.to.name} <#{dmail.to.email}>", :subject => "#{Danbooru.config.app_name} - Message received from #{dmail.from.name}")
end end
def password_reset(user)
@user = user
mail to: "#{@user.name} <#{@user.email}>", subject: "#{Danbooru.config.app_name} password reset request"
end
end end

View File

@@ -121,8 +121,7 @@ class Dmail < ApplicationRecord
end end
def valid_key?(key) def valid_key?(key)
decoded_id = verifier.verified(key) id == verifier.verify(key)
id == decoded_id
end end
def visible_to?(user, key) def visible_to?(user, key)

View File

@@ -68,7 +68,7 @@ class User < ApplicationRecord
has_bit_flags BOOLEAN_ATTRIBUTES, :field => "bit_prefs" has_bit_flags BOOLEAN_ATTRIBUTES, :field => "bit_prefs"
attr_accessor :password, :old_password attr_accessor :password, :old_password, :signed_user_id
after_initialize :initialize_attributes, if: :new_record? after_initialize :initialize_attributes, if: :new_record?
validates :name, user_name: true, on: :create validates :name, user_name: true, on: :create
@@ -185,36 +185,15 @@ class User < ApplicationRecord
def encrypt_password_on_update def encrypt_password_on_update
return if password.blank? return if password.blank?
return if old_password.blank?
if bcrypt_password == User.sha1(old_password) if signed_user_id.present? && id == Danbooru::MessageVerifier.new(:login).verify(signed_user_id)
self.bcrypt_password_hash = User.bcrypt(password)
elsif old_password.present? && bcrypt_password == User.sha1(old_password)
self.bcrypt_password_hash = User.bcrypt(password) self.bcrypt_password_hash = User.bcrypt(password)
return true
else else
errors[:old_password] << "is incorrect" errors[:old_password] << "is incorrect"
return false
end end
end end
def reset_password
consonants = "bcdfghjklmnpqrstvqxyz"
vowels = "aeiou"
pass = ""
6.times do
pass << consonants[rand(21), 1]
pass << vowels[rand(5), 1]
end
pass << rand(100).to_s
update_column(:bcrypt_password_hash, User.bcrypt(pass))
pass
end
def reset_password_and_deliver_notice
new_password = reset_password
Maintenance::User::PasswordResetMailer.confirmation(self, new_password).deliver_now
end
end end
module AuthenticationMethods module AuthenticationMethods
@@ -637,14 +616,6 @@ class User < ApplicationRecord
end end
module SearchMethods module SearchMethods
def with_email(email)
if email.blank?
where("FALSE")
else
where("email = ?", email)
end
end
def search(params) def search(params)
q = super q = super

View File

@@ -1,28 +0,0 @@
class UserPasswordResetNonce < ApplicationRecord
has_secure_token :key
validates_presence_of :email
validate :validate_existence_of_email
after_create :deliver_notice
def self.prune!
where("created_at < ?", 1.week.ago).destroy_all
end
def deliver_notice
Maintenance::User::PasswordResetMailer.reset_request(user, self).deliver_now
end
def validate_existence_of_email
if !User.with_email(email).exists?
errors[:email] << "is invalid"
end
end
def reset_user!
user.reset_password_and_deliver_notice
end
def user
@user ||= User.with_email(email).first
end
end

View File

@@ -1,5 +0,0 @@
<h1>Password Reset Confirmation</h1>
<p>The password for the user "<%= @user.name %>" for the website <%= Danbooru.config.app_name %> has been reset. It is now <code><%= @new_password %></code>.</p>
<p>Please log in to the website and <%= link_to "change your password", edit_user_url(@user, :host => Danbooru.config.hostname, :only_path => false) %> as soon as possible.</p>

View File

@@ -1,4 +0,0 @@
<h1>Password Reset Request</h1>
<p>Someone has requested that the password for "<%= @user.name %>" for the website <%= Danbooru.config.app_name %> be reset. If you did not request this, then you can ignore this email.</p>
<p>To reset your password, please visit <%= link_to "this link", edit_maintenance_user_password_reset_url(:host => Danbooru.config.hostname, :only_path => false, :key => @nonce.key, :email => @nonce.email) %>.</p>

View File

@@ -1,19 +0,0 @@
<% page_title "Reset Password" %>
<%= render "sessions/secondary_links" %>
<div id="c-maintenance-user-password-resets">
<div id="a-edit">
<h1>Reset Password</h1>
<% if @nonce %>
<%= form_tag(maintenance_user_password_reset_path, :method => :put) do %>
<%= hidden_field_tag :email, params[:email] %>
<%= hidden_field_tag :key, params[:key] %>
<p>Do you wish to reset your password? A new password will be emailed to you.</p>
<%= submit_tag "Reset" %>
<% end %>
<% else %>
<p>Invalid key</p>
<% end %>
</div>
</div>

View File

@@ -1,20 +0,0 @@
<% page_title "Reset Password" %>
<%= render "sessions/secondary_links" %>
<div id="c-maintenance-user-password-resets">
<div id="a-new">
<h1>Reset Password</h1>
<p>If you supplied an email address when signing up, <%= Danbooru.config.app_name %> can reset your password. You will receive an email confirming your request for a new password.</p>
<p>If you didn't supply a valid email address, you are out of luck.</p>
<%= form_tag(maintenance_user_password_reset_path, :class => "simple_form") do %>
<div class="input email required">
<label for="nonce_email" class="required">Email</label>
<%= text_field :nonce, :email %>
</div>
<%= submit_tag "Submit" %>
<% end %>
</div>
</div>

View File

@@ -0,0 +1,22 @@
<% page_title "Reset Password" %>
<%= render "sessions/secondary_links" %>
<div id="c-password-resets">
<div id="a-show">
<h1>Reset Password</h1>
<p>
Enter your username below to reset your password. You will be sent an
email containing a link to reset your password.
</p>
<p>
If your account doesn't have a valid email address, then your password can't be reset.
</p>
<%= edit_form_for(:user, url: password_reset_path, action: :post) do |f| %>
<%= f.input :name, label: "Username", input_html: { "data-autocomplete": "user" } %>
<%= f.submit "Submit" %>
<% end %>
</div>
</div>

View File

@@ -4,8 +4,14 @@
<div id="a-edit"> <div id="a-edit">
<h1>Change Password</h1> <h1>Change Password</h1>
<p>Enter a new password below.</p>
<%= edit_form_for(@user, url: user_password_path(@user)) do |f| %> <%= edit_form_for(@user, url: user_password_path(@user)) do |f| %>
<%= f.input :old_password, as: :password, hint: "Re-enter your current password." %> <% if params[:signed_user_id] %>
<%= f.input :signed_user_id, as: :hidden, input_html: { value: params[:signed_user_id] } %>
<% else %>
<%= f.input :old_password, as: :password, hint: "Re-enter your current password." %>
<% end %>
<%= f.input :password, label: "New password", hint: "Must be at least 5 characters long." %> <%= f.input :password, label: "New password", hint: "Must be at least 5 characters long." %>
<%= f.input :password_confirmation, label: "Confirm new password" %> <%= f.input :password_confirmation, label: "Confirm new password" %>
<%= f.submit "Save" %> <%= f.submit "Save" %>

View File

@@ -11,7 +11,7 @@
<%= simple_form_for(:session, url: session_path) do |f| %> <%= simple_form_for(:session, url: session_path) do |f| %>
<%= f.input :url, as: :hidden, input_html: { value: params[:url] } %> <%= f.input :url, as: :hidden, input_html: { value: params[:url] } %>
<%= f.input :name %> <%= f.input :name %>
<%= f.input :password, hint: link_to("Forgot password?", new_maintenance_user_password_reset_path), input_html: { autocomplete: "password" } %> <%= f.input :password, hint: link_to("Forgot password?", password_reset_path), input_html: { autocomplete: "password" } %>
<%= f.submit "Login" %> <%= f.submit "Login" %>
<% end %> <% end %>

View File

@@ -0,0 +1,22 @@
<!doctype html>
<html>
<body>
<h2>Hi <%= @user.name %>,</h2>
<p>
You recently requested your password to be reset for your <%= Danbooru.config.app_name %>
account. Click the link below to login to <%= Danbooru.config.app_name %>
and reset your password.
</p>
<p>
<%= link_to "Reset password", edit_user_password_url(@user, signed_user_id: Danbooru::MessageVerifier.new(:login).generate(@user.id, expires_in: 30.minutes)) %>
</p>
<p>
If you did not request for your <%= Danbooru.config.app_name %> password to
be reset, please ignore this email or reply to let us know. This link
will only be valid for the next 30 minutes.
</p>
</body>
</html>

View File

@@ -45,7 +45,6 @@ Rails.application.routes.draw do
namespace :user do namespace :user do
resource :count_fixes, only: [:new, :create] resource :count_fixes, only: [:new, :create]
resource :email_notification, :only => [:show, :destroy] resource :email_notification, :only => [:show, :destroy]
resource :password_reset, :only => [:new, :create, :edit, :update]
resource :deletion, :only => [:show, :destroy] resource :deletion, :only => [:show, :destroy]
resource :email_change, :only => [:new, :create] resource :email_change, :only => [:new, :create]
resource :api_key, :only => [:show, :view, :update, :destroy] do resource :api_key, :only => [:show, :view, :update, :destroy] do
@@ -153,6 +152,7 @@ Rails.application.routes.draw do
end end
resources :note_versions, :only => [:index, :show] resources :note_versions, :only => [:index, :show]
resource :note_previews, :only => [:show] resource :note_previews, :only => [:show]
resource :password_reset, only: [:create, :show]
resources :pixiv_ugoira_frame_data, only: [:index] resources :pixiv_ugoira_frame_data, only: [:index]
resources :pools do resources :pools do
member do member do

View File

@@ -0,0 +1,7 @@
require_relative "20110717010705_create_user_password_reset_nonces"
class DropUserPasswordResetNonces < ActiveRecord::Migration[6.0]
def change
revert CreateUserPasswordResetNonces
end
end

View File

@@ -3049,38 +3049,6 @@ CREATE SEQUENCE public.user_name_change_requests_id_seq
ALTER SEQUENCE public.user_name_change_requests_id_seq OWNED BY public.user_name_change_requests.id; ALTER SEQUENCE public.user_name_change_requests_id_seq OWNED BY public.user_name_change_requests.id;
--
-- Name: user_password_reset_nonces; Type: TABLE; Schema: public; Owner: -
--
CREATE TABLE public.user_password_reset_nonces (
id integer NOT NULL,
key character varying NOT NULL,
email character varying NOT NULL,
created_at timestamp without time zone NOT NULL,
updated_at timestamp without time zone NOT NULL
);
--
-- Name: user_password_reset_nonces_id_seq; Type: SEQUENCE; Schema: public; Owner: -
--
CREATE SEQUENCE public.user_password_reset_nonces_id_seq
START WITH 1
INCREMENT BY 1
NO MINVALUE
NO MAXVALUE
CACHE 1;
--
-- Name: user_password_reset_nonces_id_seq; Type: SEQUENCE OWNED BY; Schema: public; Owner: -
--
ALTER SEQUENCE public.user_password_reset_nonces_id_seq OWNED BY public.user_password_reset_nonces.id;
-- --
-- Name: users_id_seq; Type: SEQUENCE; Schema: public; Owner: - -- Name: users_id_seq; Type: SEQUENCE; Schema: public; Owner: -
-- --
@@ -4142,13 +4110,6 @@ ALTER TABLE ONLY public.user_feedback ALTER COLUMN id SET DEFAULT nextval('publi
ALTER TABLE ONLY public.user_name_change_requests ALTER COLUMN id SET DEFAULT nextval('public.user_name_change_requests_id_seq'::regclass); ALTER TABLE ONLY public.user_name_change_requests ALTER COLUMN id SET DEFAULT nextval('public.user_name_change_requests_id_seq'::regclass);
--
-- Name: user_password_reset_nonces id; Type: DEFAULT; Schema: public; Owner: -
--
ALTER TABLE ONLY public.user_password_reset_nonces ALTER COLUMN id SET DEFAULT nextval('public.user_password_reset_nonces_id_seq'::regclass);
-- --
-- Name: users id; Type: DEFAULT; Schema: public; Owner: - -- Name: users id; Type: DEFAULT; Schema: public; Owner: -
-- --
@@ -4506,14 +4467,6 @@ ALTER TABLE ONLY public.user_name_change_requests
ADD CONSTRAINT user_name_change_requests_pkey PRIMARY KEY (id); ADD CONSTRAINT user_name_change_requests_pkey PRIMARY KEY (id);
--
-- Name: user_password_reset_nonces user_password_reset_nonces_pkey; Type: CONSTRAINT; Schema: public; Owner: -
--
ALTER TABLE ONLY public.user_password_reset_nonces
ADD CONSTRAINT user_password_reset_nonces_pkey PRIMARY KEY (id);
-- --
-- Name: users users_pkey; Type: CONSTRAINT; Schema: public; Owner: - -- Name: users users_pkey; Type: CONSTRAINT; Schema: public; Owner: -
-- --
@@ -7323,6 +7276,7 @@ INSERT INTO "schema_migrations" (version) VALUES
('20200223042415'), ('20200223042415'),
('20200223234015'), ('20200223234015'),
('20200306202253'), ('20200306202253'),
('20200307021204'); ('20200307021204'),
('20200309035334');

View File

@@ -1,3 +0,0 @@
FactoryBot.define do
factory(:user_password_reset_nonce)
end

View File

@@ -1,112 +0,0 @@
require "test_helper"
module Maintenance
module User
class PasswordResetsControllerTest < ActionDispatch::IntegrationTest
context "A password resets controller" do
setup do
@user = create(:user, :email => "abc@com.net")
ActionMailer::Base.delivery_method = :test
ActionMailer::Base.deliveries.clear
end
should "render the new page" do
get new_maintenance_user_password_reset_path
assert_response :success
end
context "create action" do
context "given invalid parameters" do
setup do
post maintenance_user_password_reset_path, params: {:nonce => {:email => ""}}
end
should "not create a new nonce" do
assert_equal(0, UserPasswordResetNonce.count)
end
should "redirect to the new page" do
assert_redirected_to new_maintenance_user_password_reset_path
end
should "not deliver an email" do
assert_equal(0, ActionMailer::Base.deliveries.size)
end
end
context "given valid parameters" do
setup do
post maintenance_user_password_reset_path, params: {:nonce => {:email => @user.email}}
end
should "create a new nonce" do
assert_equal(1, UserPasswordResetNonce.where(:email => @user.email).count)
end
should "redirect to the new page" do
assert_redirected_to new_maintenance_user_password_reset_path
end
should "deliver an email to the supplied email address" do
assert_equal(1, ActionMailer::Base.deliveries.size)
end
end
end
context "edit action" do
context "with invalid parameters" do
setup do
get edit_maintenance_user_password_reset_path, params: {:email => "a@b.c"}
end
should "succeed silently" do
assert_response :success
end
end
context "with valid parameters" do
setup do
@user = create(:user)
@nonce = create(:user_password_reset_nonce, :email => @user.email)
ActionMailer::Base.deliveries.clear
get edit_maintenance_user_password_reset_path, params: {:email => @nonce.email, :key => @nonce.key}
end
should "succeed" do
assert_response :success
end
end
end
context "update action" do
context "with valid parameters" do
setup do
@user = create(:user)
@nonce = create(:user_password_reset_nonce, :email => @user.email)
ActionMailer::Base.deliveries.clear
@old_password = @user.bcrypt_password_hash
put maintenance_user_password_reset_path, params: {:email => @nonce.email, :key => @nonce.key}
end
should "succeed" do
assert_redirected_to new_maintenance_user_password_reset_path
end
should "send an email" do
assert_equal(1, ActionMailer::Base.deliveries.size)
end
should "change the password" do
@user.reload
assert_not_equal(@old_password, @user.bcrypt_password_hash)
end
should "delete the nonce" do
assert_equal(0, UserPasswordResetNonce.count)
end
end
end
end
end
end
end

View File

@@ -0,0 +1,25 @@
require 'test_helper'
class PasswordResetsControllerTest < ActionDispatch::IntegrationTest
context "The passwords resets controller" do
setup do
@user = create(:user)
end
context "show action" do
should "work" do
get password_reset_path
assert_response :success
end
end
context "create action" do
should "work" do
post password_reset_path, params: { user: { name: @user.name } }
assert_redirected_to new_session_path
assert_enqueued_email_with UserMailer, :password_reset, args: [@user]
end
end
end
end

View File

@@ -3,4 +3,9 @@ class UserMailerPreview < ActionMailer::Preview
dmail = User.admins.first.dmails.first dmail = User.admins.first.dmails.first
UserMailer.dmail_notice(dmail) UserMailer.dmail_notice(dmail)
end end
def password_reset
user = User.find(params[:id])
UserMailer.password_reset(user)
end
end end

View File

@@ -1,48 +0,0 @@
require 'test_helper'
class UserPasswordResetNonceTest < ActiveSupport::TestCase
context "Creating a new nonce" do
context "with a valid email" do
setup do
@user = FactoryBot.create(:user, :email => "aaa@b.net")
@nonce = FactoryBot.create(:user_password_reset_nonce, :email => @user.email)
end
should "validate" do
assert_equal([], @nonce.errors.full_messages)
end
should "populate the key with a random string" do
assert_equal(24, @nonce.key.size)
end
should "reset the password when reset" do
@nonce.user.expects(:reset_password_and_deliver_notice)
@nonce.reset_user!
end
end
context "with a blank email" do
setup do
@user = FactoryBot.create(:user, :email => "")
@nonce = UserPasswordResetNonce.new(:email => "")
end
should "not validate" do
@nonce.save
assert_equal(["Email can't be blank", "Email is invalid"], @nonce.errors.full_messages.sort)
end
end
context "with an invalid email" do
setup do
@nonce = UserPasswordResetNonce.new(:email => "z@z.net")
end
should "not validate" do
@nonce.save
assert_equal(["Email is invalid"], @nonce.errors.full_messages)
end
end
end
end

View File

@@ -221,12 +221,6 @@ class UserTest < ActiveSupport::TestCase
assert_equal(["Password is too short (minimum is 5 characters)"], @user.errors.full_messages) assert_equal(["Password is too short (minimum is 5 characters)"], @user.errors.full_messages)
end end
should "should be reset" do
@user = FactoryBot.create(:user)
new_pass = @user.reset_password
assert(User.authenticate(@user.name, new_pass), "Authentication should have succeeded")
end
should "not change the password if the password and old password are blank" do should "not change the password if the password and old password are blank" do
@user = FactoryBot.create(:user, :password => "67890") @user = FactoryBot.create(:user, :password => "67890")
@user.update(password: "", old_password: "") @user.update(password: "", old_password: "")