From 31a0d9add3b555d6b41fdae09558c73ec57ed24e Mon Sep 17 00:00:00 2001 From: Stephen Nelson Date: Thu, 21 Nov 2024 11:46:01 +1030 Subject: [PATCH] AdminAuthentication middleware for authenticating admins Resolves a potential issue where a non-authenticated user could enumerate modules by checking for 307 vs 404 responses. Ensures that client apps get authentication correct. --- Gemfile.lock | 2 +- app/controllers/admin/sessions_controller.rb | 19 ++++--- app/controllers/admin/tokens_controller.rb | 51 ++++++++++--------- .../koi/controller/is_admin_controller.rb | 11 ++-- app/views/admin/admin_users/show.html.erb | 2 +- app/views/admin/sessions/new.html.erb | 9 ++-- .../admin/tokens/create.turbo_stream.erb | 2 +- app/views/admin/tokens/show.html.erb | 3 +- config/locales/koi.en.yml | 3 +- config/routes.rb | 17 +++---- katalyst-koi.gemspec | 2 +- lib/koi/engine.rb | 1 + lib/koi/middleware/admin_authentication.rb | 46 +++++++++++++++++ spec/requests/admin/authentication_spec.rb | 21 ++++++++ .../admin/sessions_controller_spec.rb | 2 +- spec/requests/admin/tokens_controller_spec.rb | 48 ++++++++--------- spec/system/admin/authentication_spec.rb | 27 ++++++++++ spec/system/admin/invitation_spec.rb | 2 +- 18 files changed, 181 insertions(+), 87 deletions(-) create mode 100644 lib/koi/middleware/admin_authentication.rb create mode 100644 spec/requests/admin/authentication_spec.rb create mode 100644 spec/system/admin/authentication_spec.rb diff --git a/Gemfile.lock b/Gemfile.lock index 3cc08ce2..e51f0007 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,7 +1,7 @@ PATH remote: . specs: - katalyst-koi (4.13.2) + katalyst-koi (4.14.0) bcrypt importmap-rails katalyst-content diff --git a/app/controllers/admin/sessions_controller.rb b/app/controllers/admin/sessions_controller.rb index 56ba5e9c..ed20f591 100644 --- a/app/controllers/admin/sessions_controller.rb +++ b/app/controllers/admin/sessions_controller.rb @@ -4,14 +4,13 @@ module Admin class SessionsController < ApplicationController include Koi::Controller::HasWebauthn - skip_before_action :authenticate_admin, only: %i[new create] + before_action :redirect_authenticated, only: %i[new], if: :admin_signed_in? + before_action :authenticate_local_admin, only: %i[new], if: :authenticate_local_admins? layout "koi/login" def new - return redirect_to admin_dashboard_path if admin_signed_in? - - render :new, locals: { admin_user: Admin::User.new } + render locals: { admin_user: Admin::User.new } end def create @@ -20,12 +19,12 @@ def create session[:admin_user_id] = admin_user.id - redirect_to admin_dashboard_path, notice: I18n.t("koi.auth.login") + redirect_to(params[:redirect].presence || admin_dashboard_path, status: :see_other) else admin_user = Admin::User.new(session_params.slice(:email, :password)) admin_user.errors.add(:email, "Invalid email or password") - render :new, status: :unprocessable_content, locals: { admin_user: } + render(:new, status: :unprocessable_content, locals: { admin_user: }) end end @@ -34,11 +33,15 @@ def destroy session[:admin_user_id] = nil - redirect_to admin_dashboard_path, notice: I18n.t("koi.auth.logout") + redirect_to new_admin_session_path end private + def redirect_authenticated + redirect_to(admin_dashboard_path, status: :see_other) + end + def session_params params.require(:admin).permit(:email, :password, :response) end @@ -65,6 +68,8 @@ def record_sign_in!(admin_user) end def record_sign_out!(admin_user) + return unless admin_user + update_last_sign_in(admin_user) admin_user.current_sign_in_at = nil diff --git a/app/controllers/admin/tokens_controller.rb b/app/controllers/admin/tokens_controller.rb index b557efe3..9e9b4797 100644 --- a/app/controllers/admin/tokens_controller.rb +++ b/app/controllers/admin/tokens_controller.rb @@ -4,49 +4,54 @@ module Admin class TokensController < ApplicationController include Koi::Controller::JsonWebToken - skip_before_action :authenticate_admin, only: %i[show update] + before_action :set_admin, only: %i[create] before_action :set_token, only: %i[show update] + before_action :invalid_token, only: %i[show update], unless: :token_valid? def show - return redirect_to new_admin_session_path, notice: I18n.t("koi.auth.token_invalid") if @token.blank? - - admin = Admin::User.find(@token[:admin_id]) - - if token_utilised?(admin, @token) - return redirect_to new_admin_session_path, notice: I18n.t("koi.auth.token_invalid") - end - - render locals: { admin:, token: params[:token] }, layout: "koi/login" + render locals: { admin: @admin, token: params[:token] }, layout: "koi/login" end def create - admin = Admin::User.find(params[:id]) - token = encode_token(admin_id: admin.id, exp: 5.minutes.from_now.to_i, iat: Time.current.to_i) + token = encode_token(admin_id: @admin.id, exp: 30.minutes.from_now.to_i, iat: Time.current.to_i) render locals: { token: } end def update - return redirect_to admin_dashboard_path, status: :see_other if admin_signed_in? - - if @token.blank? - return redirect_to new_admin_session_path, status: :see_other, notice: I18n.t("koi.auth.token_invalid") - end - - admin = Admin::User.find(@token[:admin_id]) - sign_in_admin(admin) + sign_in_admin(@admin) - redirect_to admin_admin_user_path(admin) + redirect_to admin_admin_user_path(@admin), status: :see_other, notice: t("koi.auth.token_consumed") end private + def set_admin + @admin = Admin::User.find(params[:admin_user_id]) + end + def set_token @token = decode_token(params[:token]) + + # constant time token validation requires that we always try to retrieve a user + @admin = Admin::User.find_by(id: @token&.fetch(:admin_id) || "") + end + + def token_valid? + return false unless @token.present? && @admin.present? + + # Ensure that the user has not signed in since the token was generated + if @admin.current_sign_in_at.present? + @admin.current_sign_in_at.to_i < @token[:iat] + elsif @admin.last_sign_in_at.present? + @admin.last_sign_in_at.to_i < @token[:iat] + else + true # first sign in + end end - def token_utilised?(admin, token) - admin.current_sign_in_at.present? || (admin.last_sign_in_at.present? && admin.last_sign_in_at.to_i > token[:iat]) + def invalid_token + redirect_to(new_admin_session_path, status: :see_other, notice: I18n.t("koi.auth.token_invalid")) end def sign_in_admin(admin) diff --git a/app/controllers/concerns/koi/controller/is_admin_controller.rb b/app/controllers/concerns/koi/controller/is_admin_controller.rb index ea1a5997..5c09ab31 100644 --- a/app/controllers/concerns/koi/controller/is_admin_controller.rb +++ b/app/controllers/concerns/koi/controller/is_admin_controller.rb @@ -31,9 +31,6 @@ def authenticate_local_admins(value) helper :all layout -> { turbo_frame_layout || "koi/application" } - - before_action :authenticate_local_admin, if: -> { Koi::Controller::IsAdminController.authenticate_local_admins } - before_action :authenticate_admin, unless: :admin_signed_in? end class << self @@ -47,10 +44,14 @@ def authenticate_local_admin session[:admin_user_id] = Admin::User.where(email: %W[#{ENV.fetch('USER', nil)}@katalyst.com.au admin@katalyst.com.au]).first&.id + + flash.delete(:redirect) if (redirect = flash[:redirect]) + + redirect_to(redirect || admin_dashboard_path, status: :see_other) end - def authenticate_admin - redirect_to new_admin_session_path, status: :temporary_redirect + def authenticate_local_admins? + IsAdminController.authenticate_local_admins end def turbo_frame_layout diff --git a/app/views/admin/admin_users/show.html.erb b/app/views/admin/admin_users/show.html.erb index cf374a47..4913d1a4 100644 --- a/app/views/admin/admin_users/show.html.erb +++ b/app/views/admin/admin_users/show.html.erb @@ -17,7 +17,7 @@ method: :delete, form: { data: { turbo_confirm: "Are you sure?" } } %> <% end %> - <%= button_to "Generate login link", invite_admin_admin_user_path(admin), class: "button button--primary", form: { id: "invite" } %> + <%= button_to "Generate login link", admin_admin_user_tokens_path(admin), class: "button button--primary", form: { id: "invite" } %>

Authentication

diff --git a/app/views/admin/sessions/new.html.erb b/app/views/admin/sessions/new.html.erb index 39aa5e0d..35dda432 100644 --- a/app/views/admin/sessions/new.html.erb +++ b/app/views/admin/sessions/new.html.erb @@ -7,19 +7,22 @@ webauthn_authentication_options_value: { publicKey: webauthn_auth_options }, }, ) do |f| %> + <% (redirect = flash[:redirect] || params[:redirect]) && flash.delete(:redirect) %> <% unless flash.empty? %>
<% end %> <%= f.govuk_fieldset legend: nil do %> - <%= f.govuk_email_field :email, autofocus: true, autocomplete: "email" %> - <%= f.govuk_password_field :password, autocomplete: "current-password" %> + <%# note, autocomplete off is ignored by browsers but required by PCI-DSS %> + <%= f.govuk_email_field :email, autofocus: true, autocomplete: "off" %> + <%= f.govuk_password_field :password, autocomplete: "off" %> <%= f.hidden_field :response, data: { webauthn_authentication_target: "response" } %> + <%= hidden_field_tag(:redirect, redirect) %> <% end %>
<%= f.admin_save "Log in" %> diff --git a/app/views/admin/tokens/create.turbo_stream.erb b/app/views/admin/tokens/create.turbo_stream.erb index d343ac70..0373fcb9 100644 --- a/app/views/admin/tokens/create.turbo_stream.erb +++ b/app/views/admin/tokens/create.turbo_stream.erb @@ -1,6 +1,6 @@ <%= turbo_stream.replace "invite" do %>
- <%= text_field_tag :invite_link, admin_token_url(token), readonly: true, data: { clipboard_target: "source" } %> + <%= text_field_tag :invite_link, admin_session_token_url(token), readonly: true, data: { clipboard_target: "source" } %> diff --git a/app/views/admin/tokens/show.html.erb b/app/views/admin/tokens/show.html.erb index 465aa4ef..115e7023 100644 --- a/app/views/admin/tokens/show.html.erb +++ b/app/views/admin/tokens/show.html.erb @@ -1,7 +1,6 @@ <%= render "layouts/koi/navigation_header" %> -<%= form_with(url: accept_admin_session_path) do |form| %> - <%= tag.input name: :token, type: :hidden, value: token %> +<%= form_with(url: admin_session_token_path(token), method: :patch) do |form| %>

Welcome to Koi Admin

<%= render Koi::SummaryListComponent.new(model: admin, class: "item-table") do |builder| %> <%= builder.text :name %> diff --git a/config/locales/koi.en.yml b/config/locales/koi.en.yml index bd1a5107..c92de571 100644 --- a/config/locales/koi.en.yml +++ b/config/locales/koi.en.yml @@ -9,9 +9,8 @@ en: admin: "%e %B %Y" koi: auth: - login: "You have been logged in" - logout: "You have been logged out" token_invalid: "Token invalid or consumed already" + token_consumed: "Please create a password or passkey" labels: new: New search: Search diff --git a/config/routes.rb b/config/routes.rb index 135d80fc..e7c515c4 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -3,22 +3,19 @@ Rails.application.routes.draw do namespace :admin do resource :session, only: %i[new create destroy] do - post :accept, to: "tokens#update" + # JWT tokens contain periods + resources :tokens, param: :token, only: %i[show update], token: /[^\/]+/ end resources :url_rewrites resources :admin_users do resources :credentials, only: %i[new create destroy] - post :invite, on: :member, to: "tokens#create" + resources :tokens, only: %i[create] get :archived, on: :collection put :archive, on: :collection put :restore, on: :collection end - # JWT tokens have dots(represents the 3 parts of data) in them, so we need to allow them in the URL - # can by pass if we use token as a query param - get "token/:token", to: "tokens#show", as: :token, token: /[^\/]+/ - resource :cache, only: %i[destroy] resource :dashboard, only: %i[show] @@ -26,10 +23,8 @@ end scope :admin do - constraints ->(req) { req.session[:admin_user_id].present? } do - mount Katalyst::Content::Engine, at: "content" - mount Katalyst::Navigation::Engine, at: "navigation" - mount Flipper::UI.app(Flipper) => "flipper" if Object.const_defined?("Flipper::UI") - end + mount Katalyst::Content::Engine, at: "content" + mount Katalyst::Navigation::Engine, at: "navigation" + mount Flipper::UI.app(Flipper) => "flipper" if Object.const_defined?("Flipper::UI") end end diff --git a/katalyst-koi.gemspec b/katalyst-koi.gemspec index bf85a280..483b2422 100644 --- a/katalyst-koi.gemspec +++ b/katalyst-koi.gemspec @@ -3,7 +3,7 @@ # Describe your gem and declare its dependencies: Gem::Specification.new do |s| s.name = "katalyst-koi" - s.version = "4.13.2" + s.version = "4.14.0" s.authors = ["Katalyst Interactive"] s.email = ["developers@katalyst.com.au"] diff --git a/lib/koi/engine.rb b/lib/koi/engine.rb index 8e1d12b0..5d60bcdb 100644 --- a/lib/koi/engine.rb +++ b/lib/koi/engine.rb @@ -58,6 +58,7 @@ class Engine < ::Rails::Engine end initializer "koi.middleware" do |app| + app.middleware.use Koi::Middleware::AdminAuthentication app.middleware.use ::ActionDispatch::Static, root.join("public").to_s app.middleware.insert_before Rack::Sendfile, Koi::Middleware::UrlRedirect end diff --git a/lib/koi/middleware/admin_authentication.rb b/lib/koi/middleware/admin_authentication.rb new file mode 100644 index 00000000..ee5583ff --- /dev/null +++ b/lib/koi/middleware/admin_authentication.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +module Koi + module Middleware + class AdminAuthentication + def initialize(app) + @app = app + end + + def call(env) + if env["PATH_INFO"].starts_with?("/admin") + admin_call(env) + else + @app.call(env) + end + end + + def admin_call(env) + request = ActionDispatch::Request.new(env) + session = ActionDispatch::Request::Session.find(request) + + if requires_authentication?(request) && !authenticated?(session) + # Set the redirection path for returning the user to their requested path after login + if request.get? + request.flash[:redirect] = request.fullpath + request.commit_flash + end + + [303, { "Location" => "/admin/session/new" }, []] + else + @app.call(env) + end + end + + private + + def requires_authentication?(request) + !request.path.starts_with?("/admin/session") + end + + def authenticated?(session) + session[:admin_user_id].present? + end + end + end +end diff --git a/spec/requests/admin/authentication_spec.rb b/spec/requests/admin/authentication_spec.rb new file mode 100644 index 00000000..dceee044 --- /dev/null +++ b/spec/requests/admin/authentication_spec.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe "admin authentication" do + describe "GET /admin/dashboard" do + subject { action && response } + + let(:action) { get "/admin/dashboard" } + + it { is_expected.to have_http_status(:see_other).and(redirect_to("/admin/session/new")) } + end + + describe "GET /admin/guess" do + subject { action && response } + + let(:action) { get "/admin/guess" } + + it { is_expected.to have_http_status(:see_other).and(redirect_to("/admin/session/new")) } + end +end diff --git a/spec/requests/admin/sessions_controller_spec.rb b/spec/requests/admin/sessions_controller_spec.rb index d5007d30..52d04070 100644 --- a/spec/requests/admin/sessions_controller_spec.rb +++ b/spec/requests/admin/sessions_controller_spec.rb @@ -146,7 +146,7 @@ delete admin_session_path, as: :turbo_stream end - it_behaves_like "requires admin" + include_context "with admin session" it "renders successfully" do action diff --git a/spec/requests/admin/tokens_controller_spec.rb b/spec/requests/admin/tokens_controller_spec.rb index 07bc964f..0a68cb33 100644 --- a/spec/requests/admin/tokens_controller_spec.rb +++ b/spec/requests/admin/tokens_controller_spec.rb @@ -9,8 +9,24 @@ def jwt_token(**payload) JWT.encode(payload, Rails.application.secret_key_base) end - describe "GET /admin/token/:token" do - let(:action) { get admin_token_path(token) } + describe "POST /admin/admin_users/:id/tokens" do + let(:action) { post admin_admin_user_tokens_path(admin), as: :turbo_stream } + let(:admin) { create(:admin) } + + include_context "with admin session" + + it_behaves_like "requires admin" + + it { is_expected.to be_successful } + + it "renders the token" do + action + expect(response.body).to have_css("turbo-stream[action=replace][target='invite']") + end + end + + describe "GET /admin/session/tokens/:token" do + let(:action) { get admin_session_token_path(token) } let(:admin) { create(:admin, password: "") } let(:token) { jwt_token(admin_id: admin.id, exp: 5.seconds.from_now.to_i, iat: Time.current.to_i) } @@ -40,24 +56,8 @@ def jwt_token(**payload) end end - describe "POST /admin/admin_users/:id/invite" do - let(:action) { post invite_admin_admin_user_path(admin), as: :turbo_stream } - let(:admin) { create(:admin) } - - include_context "with admin session" - - it_behaves_like "requires admin" - - it { is_expected.to be_successful } - - it "renders the token" do - action - expect(response.body).to have_css("turbo-stream[action=replace][target='invite']") - end - end - - describe "POST /admin/session/accept" do - let(:action) { post accept_admin_session_path, params: { token: } } + describe "PATCH /admin/session/tokens/:token" do + let(:action) { patch admin_session_token_path(token) } let(:admin) { create(:admin, password: "") } let(:token) { jwt_token(admin_id: admin.id, exp: 5.seconds.from_now.to_i, iat: Time.current.to_i) } @@ -66,13 +66,5 @@ def jwt_token(**payload) it "updates the admin login details" do expect { action }.to change { admin.reload.current_sign_in_at }.from(nil).to be_present end - - context "when admin is signed in" do - let(:admin) { create(:admin) } - - include_context "with admin session" - - it { is_expected.to redirect_to(admin_dashboard_path) } - end end end diff --git a/spec/system/admin/authentication_spec.rb b/spec/system/admin/authentication_spec.rb new file mode 100644 index 00000000..0a9e7e62 --- /dev/null +++ b/spec/system/admin/authentication_spec.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe "admin/authentication" do + it "supports password login" do + admin = create(:admin) + visit "/admin" + + fill_in "Email", with: admin.email + fill_in "Password", with: admin.password + click_on "Log in" + + expect(page).to have_current_path("/admin/dashboard") + end + + it "supports redirect after login" do + admin = create(:admin) + visit "/admin/admin_users" + + fill_in "Email", with: admin.email + fill_in "Password", with: admin.password + click_on "Log in" + + expect(page).to have_current_path("/admin/admin_users") + end +end diff --git a/spec/system/admin/invitation_spec.rb b/spec/system/admin/invitation_spec.rb index 303e8ecf..b790f5f5 100644 --- a/spec/system/admin/invitation_spec.rb +++ b/spec/system/admin/invitation_spec.rb @@ -31,7 +31,7 @@ def encode_token(**args) admin = create(:admin, password: "") token = encode_token(admin_id: admin.id, exp: 1.hour.from_now.to_i, ist: Time.current.to_i) - visit "admin/token/#{token}" + visit "admin/session/tokens/#{token}" expect(page).to have_content(/Welcome to Koi Admin/)