diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 490cafc3d..68d84c5f8 100644 --- a/app/controllers/application_controller.rb +++ b/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 diff --git a/app/controllers/maintenance/user/api_keys_controller.rb b/app/controllers/maintenance/user/api_keys_controller.rb index f2809e1dc..f6b0e5d02 100644 --- a/app/controllers/maintenance/user/api_keys_controller.rb +++ b/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] - rescue_from ::SessionLoader::AuthenticationFailure, with: :authentication_failed + before_action :load_apikey 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! - if ::User.authenticate(CurrentUser.user.name, params[:user][:password]) == 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.") + def load_apikey + @api_key = CurrentUser.user.api_key || ApiKey.generate!(CurrentUser.user) end end end diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index a95243a6f..0a62a3f34 100644 --- a/app/controllers/sessions_controller.rb +++ b/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 - redirect_to(posts_path, :notice => "You are now logged out") + cookies.delete(:remember) + session.delete(:last_authenticated_at) + redirect_to(posts_path, notice: "You are now logged out") + end + + def confirm_password end end diff --git a/app/javascript/src/styles/specific/sessions.scss b/app/javascript/src/styles/specific/sessions.scss index ea217013f..21d502d51 100644 --- a/app/javascript/src/styles/specific/sessions.scss +++ b/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; diff --git a/app/javascript/src/styles/specific/users.scss b/app/javascript/src/styles/specific/users.scss index 22af7125b..52fe1b601 100644 --- a/app/javascript/src/styles/specific/users.scss +++ b/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; } } -} \ No newline at end of file +} diff --git a/app/logical/session_creator.rb b/app/logical/session_creator.rb index c43ea5f41..119dd58a9 100644 --- a/app/logical/session_creator.rb +++ b/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 diff --git a/app/views/maintenance/user/api_keys/show.html.erb b/app/views/maintenance/user/api_keys/show.html.erb index 516321041..0fdf9dba1 100644 --- a/app/views/maintenance/user/api_keys/show.html.erb +++ b/app/views/maintenance/user/api_keys/show.html.erb @@ -1,12 +1,35 @@

API Key

-

You must re-enter your password to view or change your API key.

- <%= custom_form_for CurrentUser.user, url: view_user_api_key_path(CurrentUser.user), method: :post do |f| %> - <%= f.input :password, :as => :password, :input_html => {:autocomplete => "off"} %> - <%= f.button :submit, "Submit" %> - <% end %> +

Your API key is like your password

+

+ 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. +

+ + + + + + + + + + + + + + + + + + + +
API KeyCreatedUpdatedActions
<%= @api_key.key %><%= compact_time(@api_key.created_at) %><%= compact_time(@api_key.updated_at) %> + <%= button_to("Regenerate", user_api_key_path(CurrentUser.user), method: :put) %> + <%= button_to("Delete", user_api_key_path(CurrentUser.user), method: :delete) %> +
diff --git a/app/views/maintenance/user/api_keys/view.html.erb b/app/views/maintenance/user/api_keys/view.html.erb deleted file mode 100644 index b51506592..000000000 --- a/app/views/maintenance/user/api_keys/view.html.erb +++ /dev/null @@ -1,38 +0,0 @@ -
-
-

API Key

- -

-

Your API key is like your password

- 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. -

- - - - - - - - - - - - - - - - - - - -
API KeyCreatedUpdatedActions
<%= @api_key.key %><%= compact_time @api_key.created_at %><%= compact_time @api_key.updated_at %> - <%= 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 } %> -
-
-
- -<% content_for(:page_title) do %> - API Key -<% end %> diff --git a/app/views/sessions/confirm_password.html.erb b/app/views/sessions/confirm_password.html.erb new file mode 100644 index 000000000..23a642058 --- /dev/null +++ b/app/views/sessions/confirm_password.html.erb @@ -0,0 +1,16 @@ +<% content_for(:page_title) do %> + Confirm Password +<% end %> + + +<%= render "secondary_links" %> +
+

Confirm password

+

You must re-enter your password to continue.

+ <%= 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 %> +
diff --git a/app/views/sessions/new.html.erb b/app/views/sessions/new.html.erb index e7698cb13..1e97827cb 100644 --- a/app/views/sessions/new.html.erb +++ b/app/views/sessions/new.html.erb @@ -1,38 +1,22 @@
- <%= form_tag(session_path, :class => "simple_form session_form") do %> + <%= simple_form_for(:session, url: session_path) do |f| %>

Sign in

- - <%= hidden_field_tag "url", params[:url] %> - -
- - <%= text_field_tag :name %> -
- -
- - <%= password_field_tag :password %> -
- -
- <%= check_box_tag :remember, "1", true %> - -
- -
- <%= submit_tag "Submit", :data => { :disable_with => "Signing in..." } %> -
+ <%= f.input(:url, as: :hidden, input_html: { value: params[:url] }) %> + <%= f.input(:name, label: "Username") %> + <%= f.input(:password) %> + <%= f.input(:remember, as: :boolean, input_html: { checked: "checked" }) %> + <%= f.submit("Continue") %> <% end %>

Don't have an account?
- <%= link_to "Sign up here.", new_user_path() %> + <%= link_to("Sign up here.", new_user_path) %>


- -

<%= link_to "Reset Password", new_maintenance_user_password_reset_path %>

-

<%= link_to "Login Reminder", new_maintenance_user_login_reminder_path %>

+ +

<%= link_to("Reset Password", new_maintenance_user_password_reset_path) %>

+

<%= link_to("Login Reminder", new_maintenance_user_login_reminder_path) %>

diff --git a/config/routes.rb b/config/routes.rb index 776f15029..530c3b935 100644 --- a/config/routes.rb +++ b/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 - post :view - end + resource :api_key, only: %i[show update destroy], controller: "maintenance/user/api_keys" resources :staff_notes, only: [:index, :new, :create], controller: "admin/staff_notes" collection do diff --git a/test/functional/application_controller_test.rb b/test/functional/application_controller_test.rb index 1e92e0701..87e2d73f5 100644 --- a/test/functional/application_controller_test.rb +++ b/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 diff --git a/test/functional/maintenance/user/api_keys_controller_test.rb b/test/functional/maintenance/user/api_keys_controller_test.rb index 22b03bb40..1237d917d 100644 --- a/test/functional/maintenance/user/api_keys_controller_test.rb +++ b/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 - should "render" do - get_auth maintenance_user_api_key_path, @user, params: {user_id: @user.id} + context "show action" do + should "let a user see their own API keys" do + 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 "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 + context "update action" 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 diff --git a/test/functional/sessions_controller_test.rb b/test/functional/sessions_controller_test.rb index 66f016a92..c2f1acce4 100644 --- a/test/functional/sessions_controller_test.rb +++ b/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) diff --git a/test/test_helper.rb b/test/test_helper.rb index d26cd5ad0..f903b1c59 100644 --- a/test/test_helper.rb +++ b/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