[Users] Consolidate password confirmation into singular route (#813)

app/controllers/application_controller.rb

@@ -165,6 +165,7 @@ class ApplicationController < ActionController::Base
 
   def set_current_user
     SessionLoader.new(request).load
+    session.send(:load!) unless session.send(:loaded?)
   end
 
   def reset_current_user
@@ -173,6 +174,14 @@ class ApplicationController < ActionController::Base
     CurrentUser.safe_mode = Danbooru.config.safe_mode?
   end
 
+  def requires_reauthentication
+    return redirect_to(new_session_path(url: request.fullpath)) if CurrentUser.user.is_anonymous?
+    last_authenticated_at = session[:last_authenticated_at]
+    if last_authenticated_at.blank? || Time.zone.parse(last_authenticated_at) < 1.hour.ago
+      redirect_to(confirm_password_session_path(url: request.fullpath))
+    end
+  end
+
   def user_access_check(method)
     if !CurrentUser.user.send(method) || CurrentUser.user.is_banned? || IpBan.is_banned?(CurrentUser.ip_addr)
       access_denied

app/controllers/maintenance/user/api_keys_controller.rb

@@ -3,17 +3,14 @@
 module Maintenance
   module User
     class ApiKeysController < ApplicationController
+      before_action :requires_reauthentication
       before_action :member_only
-      before_action :authenticate!, except: [:show]
+      before_action :load_apikey
-      rescue_from ::SessionLoader::AuthenticationFailure, with: :authentication_failed
       respond_to :html
 
       def show
       end
 
-      def view
-      end
-
       def update
         @api_key.regenerate!
         redirect_to(user_api_key_path(CurrentUser.user), notice: "API key regenerated")
@@ -24,19 +21,10 @@ module Maintenance
         redirect_to(CurrentUser.user)
       end
 
-      protected
+      private
 
-      def authenticate!
+      def load_apikey
-        if ::User.authenticate(CurrentUser.user.name, params[:user][:password]) == CurrentUser.user
+        @api_key = CurrentUser.user.api_key || ApiKey.generate!(CurrentUser.user)
-          @api_key = CurrentUser.user.api_key || ApiKey.generate!(CurrentUser.user)
-          @password = params[:user][:password]
-        else
-          raise ::SessionLoader::AuthenticationFailure
-        end
-      end
-
-      def authentication_failed
-        redirect_to(user_api_key_path(CurrentUser.user), notice: "Password was incorrect.")
       end
     end
   end

app/controllers/sessions_controller.rb

@@ -6,26 +6,31 @@ class SessionsController < ApplicationController
   end
 
   def create
+    sparams = params.fetch(:session, {}).slice(:url, :name, :password, :remember)
     if RateLimiter.check_limit("login:#{request.remote_ip}", 15, 12.hours)
       DanbooruLogger.add_attributes("user.login" => "rate_limited")
-      return redirect_to(new_session_path, :notice => "Username/Password was incorrect")
+      return redirect_to(new_session_path, notice: "Username/Password was incorrect")
     end
-    session_creator = SessionCreator.new(session, cookies, params[:name], params[:password], request.remote_ip, params[:remember], request.ssl?)
+    session_creator = SessionCreator.new(request, session, cookies, sparams[:name], sparams[:password], sparams[:remember].to_s.truthy?)
 
     if session_creator.authenticate
-      url = params[:url] if params[:url] && params[:url].start_with?("/") && !params[:url].start_with?("//")
+      url = sparams[:url] if sparams[:url] && sparams[:url].start_with?("/") && !sparams[:url].start_with?("//")
       DanbooruLogger.add_attributes("user.login" => "success")
-      redirect_to(url || posts_path, :notice => "You are now logged in")
+      redirect_to(url || posts_path)
     else
       RateLimiter.hit("login:#{request.remote_ip}", 6.hours)
       DanbooruLogger.add_attributes("user.login" => "fail")
-      redirect_to(new_session_path, :notice => "Username/Password was incorrect")
+      redirect_back(fallback_location: new_session_path, notice: "Username/Password was incorrect")
     end
   end
 
   def destroy
     session.delete(:user_id)
-    cookies.delete :remember
+    cookies.delete(:remember)
-    redirect_to(posts_path, :notice => "You are now logged out")
+    session.delete(:last_authenticated_at)
+    redirect_to(posts_path, notice: "You are now logged out")
+  end
+
+  def confirm_password
   end
 end

app/javascript/src/styles/specific/sessions.scss

@@ -1,8 +1,6 @@
-
-
 div#c-sessions {
   div#a-new {
-    label#remember-label {
+    label[for=session_remember] {
       display: inline;
       font-weight: normal;
       font-style: italic;

app/javascript/src/styles/specific/users.scss

@@ -237,7 +237,7 @@ div#c-users {
   margin-bottom: 1em;
 }
 
-.session_form {
+.simple_form.session {
   box-sizing: border-box;
   max-width: 360px;
   margin: 0;
@@ -302,4 +302,4 @@ div#c-users {
     span { display: list-item; }
     .password-warning { font-weight: bold; }
   }
-}
+}

app/logical/session_creator.rb

@@ -1,16 +1,15 @@
 # frozen_string_literal: true
 
 class SessionCreator
-  attr_reader :session, :cookies, :name, :password, :ip_addr, :remember, :secure
+  attr_reader :request, :session, :cookies, :name, :password, :remember
 
-  def initialize(session, cookies, name, password, ip_addr, remember = false, secure = false)
+  def initialize(request, session, cookies, name, password, remember = false)
+    @request = request
     @session = session
     @cookies = cookies
     @name = name
     @password = password
-    @ip_addr = ip_addr
     @remember = remember
-    @secure = secure
   end
 
   def authenticate
@@ -18,12 +17,13 @@ class SessionCreator
       user = User.find_by_name(name)
 
       session[:user_id] = user.id
+      session[:last_authenticated_at] = Time.now.utc.to_s
       session[:ph] = user.password_token
-      user.update_column(:last_ip_addr, ip_addr) unless user.is_blocked?
+      user.update_column(:last_ip_addr, request.remote_ip) unless user.is_blocked?
 
       if remember
         verifier = ActiveSupport::MessageVerifier.new(Danbooru.config.remember_key, serializer: JSON, digest: "SHA256")
-        cookies.encrypted[:remember] = {value: verifier.generate("#{user.id}:#{user.password_token}", purpose: "rbr", expires_in: 14.days), expires: Time.now + 14.days, httponly: true, same_site: :lax, secure: Rails.env.production?}
+        cookies.encrypted[:remember] = { value: verifier.generate("#{user.id}:#{user.password_token}", purpose: "rbr", expires_in: 14.days), expires: Time.now + 14.days, httponly: true, same_site: :lax, secure: Rails.env.production? }
       end
       return true
     else

app/views/maintenance/user/api_keys/show.html.erb

@@ -1,12 +1,35 @@
 <div id="c-maintenance-user-api-keys">
   <div id="a-show">
     <h1>API Key</h1>
-    <p>You must re-enter your password to view or change your API key.</p>
 
-    <%= custom_form_for CurrentUser.user, url: view_user_api_key_path(CurrentUser.user), method: :post do |f| %>
+    <h2><b>Your API key is like your password</b></h2>
-      <%= f.input :password, :as => :password, :input_html => {:autocomplete => "off"} %>
+    <p>
-      <%= f.button :submit, "Submit" %>
+      Anyone who has it has full access to your account. Don't give your API key
-    <% end %>
+      to third-party apps you don't trust, and don't post your API key in public places.
+    </p>
+
+    <table class="striped">
+      <thead>
+      <tr>
+        <th>API Key</th>
+        <th>Created</th>
+        <th>Updated</th>
+        <th>Actions</th>
+      </tr>
+      </thead>
+
+      <tbody>
+      <tr id="api-key-<%= @api_key.id %>">
+        <td id="api-key"><code><%= @api_key.key %></code></td>
+        <td id="api-key-created"><%= compact_time(@api_key.created_at) %></td>
+        <td id="api-key-updated"><%= compact_time(@api_key.updated_at) %></td>
+        <td>
+          <%= button_to("Regenerate", user_api_key_path(CurrentUser.user), method: :put) %>
+          <%= button_to("Delete", user_api_key_path(CurrentUser.user), method: :delete) %>
+        </td>
+      </tr>
+      </tbody>
+    </table>
   </div>
 </div>
 

app/views/maintenance/user/api_keys/view.html.erb

@@ -1,38 +0,0 @@
-<div id="c-maintenance-user-api-keys">
-  <div id="a-view">
-    <h1>API Key</h1>
-
-    <p>
-      <h2><b>Your API key is like your password</b></h2>
-      Anyone who has it has full access to your account. Don't give your API key
-      to third-party apps you don't trust, and don't post your API key in public places.
-    </p>
-
-    <table class="striped">
-      <thead>
-        <tr>
-          <th>API Key</th>
-          <th>Created</th>
-          <th>Updated</th>
-          <th>Actions</th>
-        </tr>
-      </thead>
-
-      <tbody>
-        <tr>
-          <td id="api-key"><code><%= @api_key.key %></code></td>
-          <td id="api-key-created"><%= compact_time @api_key.created_at %></td>
-          <td id="api-key-updated"><%= compact_time @api_key.updated_at %></td>
-          <td>
-            <%= button_to "Regenerate", user_api_key_path(CurrentUser.user), method: :put, params: { 'user[password]': @password } %>
-            <%= button_to "Delete", user_api_key_path(CurrentUser.user), method: :delete, params: { 'user[password]': @password } %>
-          </td>
-        </tr>
-      </tbody>
-    </table>
-  </div>
-</div>
-
-<% content_for(:page_title) do %>
-  API Key
-<% end %>

app/views/sessions/confirm_password.html.erb

@@ -0,0 +1,16 @@
+<% content_for(:page_title) do %>
+  Confirm Password
+<% end %>
+
+
+<%= render "secondary_links" %>
+<div id="c-sessions"><div id="a-confirm-password">
+  <h1>Confirm password</h1>
+  <p>You must re-enter your password to continue.</p>
+  <%= simple_form_for(:session, url: session_path) do |f| %>
+    <%= f.input(:url, as: :hidden, input_html: { value: params[:url] }) %>
+    <%= f.input(:name, as: :hidden, input_html: { value: CurrentUser.user.name }) %>
+    <%= f.input(:password, hint: link_to("Forgot password?", new_maintenance_user_password_reset_path), input_html: { autocomplete: "current-password" }) %>
+    <%= f.submit("Continue") %>
+  <% end %>
+</div></div>

app/views/sessions/new.html.erb

@@ -1,38 +1,22 @@
 <div id="c-sessions"><div id="a-new">
-  <%= form_tag(session_path, :class => "simple_form session_form") do %>
+  <%= simple_form_for(:session, url: session_path) do |f| %>
     <h1>Sign in</h1>
-
+    <%= f.input(:url, as: :hidden, input_html: { value: params[:url] }) %>
-    <%= hidden_field_tag "url", params[:url] %>
+    <%= f.input(:name, label: "Username") %>
-
+    <%= f.input(:password) %>
-    <div class="input">
+    <%= f.input(:remember, as: :boolean, input_html: { checked: "checked" }) %>
-      <label for="name">Username</label>
+    <%= f.submit("Continue") %>
-      <%= text_field_tag :name %>
-    </div>
-
-    <div class="input">
-      <label for="password">Password</label>
-      <%= password_field_tag :password %>
-    </div>
-
-    <div class="input">
-      <%= check_box_tag :remember, "1", true %>
-      <label for="remember" id="remember-label">Remember</label>
-    </div>
-
-    <div class="input">
-      <%= submit_tag "Submit", :data => { :disable_with => "Signing in..." } %>
-    </div>
   <% end %>
 
   <section class="session_info">
     <h3>
       Don't have an account?<br />
-      <%= link_to "Sign up here.", new_user_path() %>
+      <%= link_to("Sign up here.", new_user_path) %>
     </h3>
     <br />
-    
+
-    <h3><%= link_to "Reset Password", new_maintenance_user_password_reset_path %></h2>
+    <h3><%= link_to("Reset Password", new_maintenance_user_password_reset_path) %></h3>
-    <h3><%= link_to "Login Reminder", new_maintenance_user_login_reminder_path %></h2>
+    <h3><%= link_to("Login Reminder", new_maintenance_user_login_reminder_path) %></h3>
   </section>
 </div></div>
 

config/routes.rb

@@ -267,7 +267,9 @@ Rails.application.routes.draw do
   end
   resource :related_tag, :only => [:show, :update]
   match "related_tag/bulk", to: "related_tags#bulk", via: [:get, :post]
-  resource :session, only: [:new, :create, :destroy]
+  resource :session, only: %i[new create destroy] do
+    get :confirm_password, on: :collection
+  end
   resources :stats, only: [:index]
   resources :tags, constraints: id_name_constraint do
     resource :correction, :only => [:new, :create, :show], :controller => "tag_corrections"
@@ -291,9 +293,7 @@ Rails.application.routes.draw do
   resources :uploads
   resources :users do
     resource :password, :only => [:edit], :controller => "maintenance/user/passwords"
-    resource :api_key, :only => [:show, :view, :update, :destroy], :controller => "maintenance/user/api_keys" do
+    resource :api_key, only: %i[show update destroy], controller: "maintenance/user/api_keys"
-      post :view
-    end
     resources :staff_notes, only: [:index, :new, :create], controller: "admin/staff_notes"
 
     collection do

test/functional/application_controller_test.rb

@@ -108,7 +108,7 @@ class ApplicationControllerTest < ActionDispatch::IntegrationTest
           token = css_select("form input[name=authenticity_token]").first["value"]
 
           # login
-          post session_path, params: { authenticity_token: token, name: @user.name, password: "6cQE!wbA" }
+          post session_path, params: { authenticity_token: token, session: { name: @user.name, password: "6cQE!wbA" } }
           assert_redirected_to posts_path
 
           # try to submit a form with cookies but without the csrf token
@@ -124,7 +124,7 @@ class ApplicationControllerTest < ActionDispatch::IntegrationTest
       should "succeed" do
         user = create(:user, password: "6cQE!wbA")
 
-        post session_path, params: { name: user.name, password: "6cQE!wbA" }
+        post session_path, params: { session: { name: user.name, password: "6cQE!wbA" } }
         get edit_user_path(user)
 
         assert_response :success

test/functional/maintenance/user/api_keys_controller_test.rb

@@ -8,61 +8,42 @@ module Maintenance
       context "An api keys controller" do
         setup do
           @user = create(:privileged_user, password: "6cQE!wbA")
-          ApiKey.generate!(@user)
+          @api_key = ApiKey.generate!(@user)
         end
 
-        context "#show" do
+        context "show action" do
-          should "render" do
+          should "let a user see their own API keys" do
-            get_auth maintenance_user_api_key_path, @user, params: {user_id: @user.id}
+            get_auth maintenance_user_api_key_path(@user.id), @user
             assert_response :success
+            assert_select "#api-key-#{@api_key.id}", count: 1
+          end
+
+          should "not let a user see API keys belonging to other users" do
+            get_auth maintenance_user_api_key_path(@user.id), create(:user)
+            assert_response :success
+            assert_select "#api-key-#{@api_key.id}", count: 0
+          end
+
+          should "redirect to the confirm password page if the user hasn't recently authenticated" do
+            post session_path, params: { session: { name: @user.name, password: @user.password } }
+            travel_to 2.hours.from_now do
+              get maintenance_user_api_key_path(@user.id)
+            end
+            assert_redirected_to confirm_password_session_path(url: maintenance_user_api_key_path(@user.id))
           end
         end
 
-        context "#view" do
+        context "update action" do
-          context "with a correct password" do
-            should "succeed" do
-              post_auth view_maintenance_user_api_key_path(user_id: @user.id), @user, params: { user: { password: "6cQE!wbA" } }
-              assert_response :success
-            end
-
-            # hard to test this in integrationtest
-            # context "if the user doesn't already have an api key" do
-            #   setup do
-            #     ::User.any_instance.stubs(:api_key).returns(nil)
-            #     cookies[:user_name] = @user.name
-            #     cookies[:password_hash] = @user.bcrypt_cookie_password_hash
-            #   end
-
-            #   should "generate one" do
-            #     ApiKey.expects(:generate!)
-
-            #     assert_difference("ApiKey.count", 1) do
-            #       post view_maintenance_user_api_key_path(user_id: @user.id), params: { user: { password: "6cQE!wbA" } }
-            #     end
-
-            #     assert_not_nil(@user.reload.api_key)
-            #   end
-            # end
-
-            should "not generate another API key if the user already has one" do
-              assert_difference("ApiKey.count", 0) do
-                post_auth view_maintenance_user_api_key_path(user_id: @user.id), @user, params: { user: { password: "6cQE!wbA" } }
-              end
-            end
-          end
-        end
-
-        context "#update" do
           should "regenerate the API key" do
             old_key = @user.api_key
-            put_auth maintenance_user_api_key_path, @user, params: {user_id: @user.id, user: { password: "6cQE!wbA" } }
+            put_auth maintenance_user_api_key_path, @user
             assert_not_equal(old_key.key, @user.reload.api_key.key)
           end
         end
 
-        context "#destroy" do
+        context "destroy action" do
           should "delete the API key" do
-            delete_auth maintenance_user_api_key_path, @user, params: {user_id: @user.id, user: { password: "6cQE!wbA" } }
+            delete_auth maintenance_user_api_key_path, @user
             assert_nil(@user.reload.api_key)
           end
         end

test/functional/sessions_controller_test.rb

@@ -15,7 +15,7 @@ class SessionsControllerTest < ActionDispatch::IntegrationTest
       should "create a new session" do
         user = create(:user)
 
-        post session_path, params: { name: user.name, password: "6cQE!wbA" }
+        post session_path, params: { session: { name: user.name, password: "6cQE!wbA" } }
         user.reload
 
         assert_redirected_to(posts_path)
@@ -34,7 +34,7 @@ class SessionsControllerTest < ActionDispatch::IntegrationTest
 
       should "fail when provided an invalid password" do
         user = create(:user, password: "6cQE!wbA", password_confirmation: "6cQE!wbA")
-        post session_path, params: { name: user.name, password: "yyy" }
+        post session_path, params: { session: { name: user.name, password: "yyy" } }
 
         assert_nil(session[:user_id])
         assert_equal("Username/Password was incorrect", flash[:notice])
@@ -45,7 +45,7 @@ class SessionsControllerTest < ActionDispatch::IntegrationTest
       should "clear the session" do
         user = create(:user)
 
-        post session_path, params: { name: user.name, password: "6cQE!wbA" }
+        post session_path, params: { session: { name: user.name, password: "6cQE!wbA" } }
         assert_not_nil(session[:user_id])
 
         delete_auth(session_path, user)

test/test_helper.rb

@@ -95,7 +95,7 @@ end
 
 class ActionDispatch::IntegrationTest
   def method_authenticated(method_name, url, user, options)
-    post session_path, params: { name: user.name, password: user.password }
+    post session_path, params: { session: { name: user.name, password: user.password } }
     self.send(method_name, url, **options)
   end