diff --git a/Gemfile.lock b/Gemfile.lock index 6a56f0935..ba1c8fc02 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -12,6 +12,8 @@ PATH katalyst-tables (>= 3.5) pagy (>= 8.0) rails (>= 7.1) + rotp + rqrcode stimulus-rails turbo-rails (>= 2.0) view_component @@ -130,6 +132,7 @@ GEM regexp_parser (>= 1.5, < 3.0) xpath (~> 3.2) cbor (0.5.9.8) + chunky_png (1.4.0) compare-xml (0.66) nokogiri (~> 1.8) concurrent-ruby (1.3.4) @@ -352,6 +355,11 @@ GEM reline (0.5.12) io-console (~> 0.5) rexml (3.3.9) + rotp (6.3.0) + rqrcode (2.2.0) + chunky_png (~> 1.0) + rqrcode_core (~> 1.0) + rqrcode_core (1.2.0) rspec-core (3.13.2) rspec-support (~> 3.13.0) rspec-expectations (3.13.3) diff --git a/app/controllers/admin/admin_users_controller.rb b/app/controllers/admin/admin_users_controller.rb index 5f038c0e4..d5ccbf0b3 100644 --- a/app/controllers/admin/admin_users_controller.rb +++ b/app/controllers/admin/admin_users_controller.rb @@ -88,6 +88,8 @@ class Collection < Admin::Collection attribute :email, :string attribute :last_sign_in_at, :date attribute :sign_in_count, :integer + attribute :passkey, :boolean, scope: :has_passkey + attribute :mfa, :boolean, scope: :has_otp end end end diff --git a/app/controllers/admin/otps_controller.rb b/app/controllers/admin/otps_controller.rb new file mode 100644 index 000000000..fa88b8621 --- /dev/null +++ b/app/controllers/admin/otps_controller.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +module Admin + class OtpsController < ApplicationController + before_action :set_admin_user + + def new + @admin_user.otp_secret = ROTP::Base32.random + + render :new, locals: { admin: @admin_user } + end + + def create + @admin_user.otp_secret = otp_params[:otp_secret] + + if @admin_user.otp.verify(otp_params[:token]) + @admin_user.save + + redirect_to admin_admin_user_path(@admin_user), status: :see_other + else + @admin_user.errors.add(:token, :invalid) + + respond_to do |format| + format.html { redirect_to admin_admin_user_path(@admin_user), status: :see_other } + format.turbo_stream { render locals: { admin: @admin_user } } + end + end + end + + def destroy + @admin_user.update!(otp_secret: nil) + + redirect_to admin_admin_user_path(@admin_user), status: :see_other + end + + private + + def otp_params + params.require(:admin).permit(:otp_secret, :token) + end + + def set_admin_user + @admin_user = Admin::User.find(params[:admin_user_id]) + + if current_admin == @admin_user + request.variant = :self + else + head(:forbidden) + end + end + end +end diff --git a/app/controllers/admin/sessions_controller.rb b/app/controllers/admin/sessions_controller.rb index 1256bb36d..6bf7ae508 100644 --- a/app/controllers/admin/sessions_controller.rb +++ b/app/controllers/admin/sessions_controller.rb @@ -14,15 +14,20 @@ def new end def create - if (admin_user = webauthn_authenticate! || params_authenticate!) - record_sign_in!(admin_user) - - session[:admin_user_id] = admin_user.id - - redirect_to(url_from(params[:redirect].presence) || admin_dashboard_path, status: :see_other) + if session_params[:response].present? + create_session_with_webauthn + elsif session_params[:token].present? + create_session_with_token + elsif session_params[:password].present? + create_session_with_password + elsif session_params[:email].present? + # conversational flow, ask for password regardless of email + admin_user = Admin::User.new(session_params.slice(:email)) + + render(:password, status: :unprocessable_content, locals: { admin_user: }) else - admin_user = Admin::User.new(session_params.slice(:email, :password)) - admin_user.errors.add(:email, "Invalid email or password") + # invalid request, re-render new + admin_user = Admin::User.new render(:new, status: :unprocessable_content, locals: { admin_user: }) end @@ -38,16 +43,66 @@ def destroy private + def create_session_with_password + # constant time lookup for user with password verification + admin_user = Admin::User.authenticate_by(session_params.slice(:email, :password)) + + if admin_user.present? && admin_user.requires_otp? + session[:pending_admin_user_id] = admin_user.id + + render(:otp, status: :unprocessable_content, locals: { admin_user: }) + elsif admin_user.present? + admin_sign_in(admin_user) + else + admin_user = Admin::User.new(session_params.slice(:email, :password)) + admin_user.errors.add(:email, :invalid) + + render(:new, status: :unprocessable_content, locals: { admin_user: }) + end + end + + def create_session_with_token + # assume that the previous step injected the user's ID into the session and remove it regardless of outcome + admin_user = Admin::User.find_by(id: session.delete(:pending_admin_user_id)) + + if admin_user&.otp&.verify(session_params[:token], + drift_ahead: 15, + drift_behind: 15, + after: admin_user.current_sign_in_at) + admin_sign_in(admin_user) + else + admin_user = Admin::User.new + admin_user.errors.add(:email, :invalid) + + render(:new, status: :unprocessable_content, locals: { admin_user: }) + end + end + + def create_session_with_webauthn + if (admin_user = webauthn_authenticate!) + admin_sign_in(admin_user) + else + admin_user = Admin::User.new + admin_user.errors.add(:email, :invalid) + + render(:new, status: :unprocessable_content, locals: { admin_user: }) + end + end + def redirect_authenticated redirect_to(admin_dashboard_path, status: :see_other) end - def session_params - params.require(:admin).permit(:email, :password, :response) + def admin_sign_in(admin_user) + record_sign_in!(admin_user) + + session[:admin_user_id] = admin_user.id + + redirect_to(url_from(params[:redirect].presence) || admin_dashboard_path, status: :see_other) end - def params_authenticate! - Admin::User.authenticate_by(session_params.slice(:email, :password)) + def session_params + params.require(:admin).permit(:email, :password, :token, :response) end def update_last_sign_in(admin_user) diff --git a/app/models/admin/user.rb b/app/models/admin/user.rb index 77e2ebc96..2e6c705d9 100644 --- a/app/models/admin/user.rb +++ b/app/models/admin/user.rb @@ -3,6 +3,7 @@ module Admin class User < ApplicationRecord include Koi::Model::Archivable + include Koi::Model::OTP def self.model_name ActiveModel::Name.new(self, nil, "Admin") @@ -27,5 +28,26 @@ def self.model_name where("email LIKE :query OR name LIKE :query", query: "%#{query}%") end end + + scope :has_otp, ->(otp) do + if otp + where.not(otp_secret: nil) + else + where(otp_secret: nil) + end + end + + scope :has_passkey, ->(passkey) do + if passkey + where(id: Admin::Credential.select(:admin_id)) + else + where.not(id: Admin::Credential.select(:admin_id)) + end + end + + def passkey + credentials.any? + end + alias passkey? passkey end end diff --git a/app/models/concerns/koi/model/otp.rb b/app/models/concerns/koi/model/otp.rb new file mode 100644 index 000000000..cd9067671 --- /dev/null +++ b/app/models/concerns/koi/model/otp.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +module Koi + module Model + module OTP + extend ActiveSupport::Concern + + included do + attribute :token, :string + end + + def requires_otp? + otp_secret.present? + end + + def otp + ROTP::TOTP.new(otp_secret) if otp_secret.present? + end + end + end +end diff --git a/app/views/admin/admin_users/index.html.erb b/app/views/admin/admin_users/index.html.erb index 8bedd81ab..d50b05435 100644 --- a/app/views/admin/admin_users/index.html.erb +++ b/app/views/admin/admin_users/index.html.erb @@ -18,6 +18,9 @@ <% row.boolean :credentials, label: "Passkey" do |cell| %> <%= cell.value.any? ? "Yes" : "No" %> <% end %> + <% row.boolean :otp, label: "MFA" do |cell| %> + <%= cell.value.present? ? "Yes" : "No" %> + <% end %> <% end %> <%= table_pagination_with(collection:) %> diff --git a/app/views/admin/admin_users/show.html+self.erb b/app/views/admin/admin_users/show.html+self.erb index d68197b73..3adae51b9 100644 --- a/app/views/admin/admin_users/show.html+self.erb +++ b/app/views/admin/admin_users/show.html+self.erb @@ -4,11 +4,25 @@ <%= render Koi::Header::ShowComponent.new(resource: admin) %> <% end %> -<%= render Koi::SummaryListComponent.new(model: admin, class: "item-table") do |builder| %> +<%= render Koi::SummaryTableComponent.new(model: admin) do |builder| %> <%= builder.text :name %> <%= builder.text :email %> <%= builder.date :created_at %> - <%= builder.date :last_sign_in_at, label: { text: "Last sign in" } %> + <%= builder.date :last_sign_in_at, label: "Last sign in" %> + <%= builder.boolean :passkey %> + <%= builder.boolean :otp, label: "MFA" do |otp| %> + + <%= otp %> + <% if otp.value %> + <%= button_to("Remove", admin_admin_user_otp_path(admin), + class: "button button--text", + method: :delete, + form: { data: { turbo_confirm: "Are you sure?" } }) %> + <% else %> + <%= kpop_link_to "Add", new_admin_admin_user_otp_path(admin) %> + <% end %> + + <% end %> <% end %>
diff --git a/app/views/admin/admin_users/show.html.erb b/app/views/admin/admin_users/show.html.erb index 8318c05f5..76c7fc2e8 100644 --- a/app/views/admin/admin_users/show.html.erb +++ b/app/views/admin/admin_users/show.html.erb @@ -4,18 +4,16 @@ <%= render Koi::Header::ShowComponent.new(resource: admin) %> <% end %> -<%= render Koi::SummaryListComponent.new(model: admin, class: "item-table") do |builder| %> +<%= render Koi::SummaryTableComponent.new(model: admin, class: "item-table") do |builder| %> <%= builder.text :name %> <%= builder.text :email %> <%= builder.date :created_at %> - <%= builder.date :last_sign_in_at, label: { text: "Last sign in" } %> + <%= builder.date :last_sign_in_at, label: "Last sign in" %> + <%= builder.boolean :passkey %> + <%= builder.boolean :otp, label: "MFA" %> <%= builder.boolean :archived? %> <% end %> -

Passkeys

- -<%= render "admin/credentials/credentials", admin: %> -
<% if admin.archived? %> <%= button_to "Delete", admin_admin_user_path(admin), diff --git a/app/views/admin/otps/_form.html.erb b/app/views/admin/otps/_form.html.erb new file mode 100644 index 000000000..bcec86259 --- /dev/null +++ b/app/views/admin/otps/_form.html.erb @@ -0,0 +1,31 @@ +<%# locals: (admin:) %> + +<%= form_with(id: dom_id(admin, :otp), + model: admin, + url: admin_admin_user_otp_path(admin), + method: :post, + class: "flow") do |form| %> +
+

MFA protects your account by requiring you to enter a six-digit + token that changes every 30 seconds. If someone knows or guesses your + password they also need to know the current token to log in.

+

In general, we recommend using Passkeys over MFA. Passkeys offer better + security than a password + MFA, and they are easier to use.

+

Add an MFA authenticator to your account

+
    +
  1. Install an MFA app. Most password managers support MFA.
    2. +
  2. Scan this code using your mobile device or password manager:
    + <%== RQRCode::QRCode.new(admin.otp.provisioning_uri(admin.email)).as_svg( + color: "000", + shape_rendering: "crispEdges", + module_size: 3, + use_path: true, + ) %> +
    3. +
  3. Enter the token shown in your app into the field below:
    4. +
+
+ <%= form.hidden_field :otp_secret %> + <%= form.govuk_text_field :token %> + <%= form.admin_save %> +<% end %> diff --git a/app/views/admin/otps/create.turbo_stream.erb b/app/views/admin/otps/create.turbo_stream.erb new file mode 100644 index 000000000..24efc5858 --- /dev/null +++ b/app/views/admin/otps/create.turbo_stream.erb @@ -0,0 +1,5 @@ +<%# locals: (admin:) %> + +<%= turbo_stream.replace(dom_id(admin, :otp)) do %> + <%= render("form", admin:) %> +<% end %> diff --git a/app/views/admin/otps/new.html.erb b/app/views/admin/otps/new.html.erb new file mode 100644 index 000000000..a89aa2a60 --- /dev/null +++ b/app/views/admin/otps/new.html.erb @@ -0,0 +1,5 @@ +<%# locals: (admin:) %> + +<%= render Kpop::ModalComponent.new(title: "Configure MFA") do %> + <%= render("form", admin:) %> +<% end %> diff --git a/app/views/admin/sessions/new.html.erb b/app/views/admin/sessions/new.html.erb index 35dda4322..ae21f74fc 100644 --- a/app/views/admin/sessions/new.html.erb +++ b/app/views/admin/sessions/new.html.erb @@ -1,4 +1,7 @@ +<%# locals: (admin_user:) %> + <%= render "layouts/koi/navigation_header" %> + <%= form_with( model: admin_user, url: admin_session_path, @@ -6,7 +9,7 @@ controller: "webauthn-authentication", webauthn_authentication_options_value: { publicKey: webauthn_auth_options }, }, - ) do |f| %> + ) do |form| %> <% (redirect = flash[:redirect] || params[:redirect]) && flash.delete(:redirect) %> <% unless flash.empty? %>
@@ -17,15 +20,12 @@
<% end %> - <%= f.govuk_fieldset legend: nil do %> - <%# 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 %> + <%# note, autocomplete off is ignored by browsers but required by PCI-DSS %> + <%= form.govuk_email_field :email, autofocus: true, autocomplete: "off" %> + <%= form.hidden_field :response, data: { webauthn_authentication_target: "response" } %> + <%= hidden_field_tag(:redirect, redirect) %>
- <%= f.admin_save "Log in" %> - <%= f.button "🔑", type: :button, class: "button button--secondary", data: { action: "webauthn-authentication#authenticate" } %> + <%= form.admin_save "Next" %> + <%= form.button "🔑", type: :button, class: "button button--secondary", data: { action: "webauthn-authentication#authenticate" } %>
<% end %> diff --git a/app/views/admin/sessions/otp.html.erb b/app/views/admin/sessions/otp.html.erb new file mode 100644 index 000000000..8cef55d50 --- /dev/null +++ b/app/views/admin/sessions/otp.html.erb @@ -0,0 +1,10 @@ +<%# locals: (admin_user:) %> + +<%= render "layouts/koi/navigation_header" %> + +<%= form_with(model: admin_user, url: admin_session_path, method: :post) do |form| %> + <%# note, autocomplete off is ignored by browsers but required by PCI-DSS %> + <%= form.govuk_text_field :token, autofocus: true, autocomplete: "off" %> + <%= hidden_field_tag(:redirect, params[:redirect]) %> + <%= form.admin_save "Next" %> +<% end %> diff --git a/app/views/admin/sessions/password.html.erb b/app/views/admin/sessions/password.html.erb new file mode 100644 index 000000000..44a7b7d21 --- /dev/null +++ b/app/views/admin/sessions/password.html.erb @@ -0,0 +1,11 @@ +<%# locals: (admin_user:) %> + +<%= render "layouts/koi/navigation_header" %> + +<%= form_with(model: admin_user, url: admin_session_path) do |form| %> + <%= form.hidden_field(:email) %> + <%# note, autocomplete off is ignored by browsers but required by PCI-DSS %> + <%= form.govuk_password_field :password, autofocus: true, autocomplete: "off" %> + <%= hidden_field_tag(:redirect, params[:redirect]) %> + <%= form.admin_save "Next" %> +<% end %> diff --git a/config/initializers/inflections.rb b/config/initializers/inflections.rb new file mode 100644 index 000000000..fd162a813 --- /dev/null +++ b/config/initializers/inflections.rb @@ -0,0 +1,6 @@ +# frozen_string_literal: true + +ActiveSupport::Inflector.inflections do |inflect| + inflect.acronym "MFA" + inflect.acronym "OTP" +end diff --git a/config/locales/koi.en.yml b/config/locales/koi.en.yml index c92de571c..2071802de 100644 --- a/config/locales/koi.en.yml +++ b/config/locales/koi.en.yml @@ -14,6 +14,11 @@ en: labels: new: New search: Search + activerecord: + errors: + models: + admin: + invalid: "Invalid login credentials" helpers: hint: default: diff --git a/config/routes.rb b/config/routes.rb index e7c515c4c..ce5486689 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -10,6 +10,7 @@ resources :url_rewrites resources :admin_users do resources :credentials, only: %i[new create destroy] + resource :otp, only: %i[new create destroy] resources :tokens, only: %i[create] get :archived, on: :collection put :archive, on: :collection diff --git a/db/migrate/20241214060913_add_otp_secret_to_admin_users.rb b/db/migrate/20241214060913_add_otp_secret_to_admin_users.rb new file mode 100644 index 000000000..23019c148 --- /dev/null +++ b/db/migrate/20241214060913_add_otp_secret_to_admin_users.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class AddOTPSecretToAdminUsers < ActiveRecord::Migration[8.0] + def change + add_column :admins, :otp_secret, :string + end +end diff --git a/katalyst-koi.gemspec b/katalyst-koi.gemspec index 89fb8304c..56e10be16 100644 --- a/katalyst-koi.gemspec +++ b/katalyst-koi.gemspec @@ -28,6 +28,8 @@ Gem::Specification.new do |s| # Authorization s.add_dependency "bcrypt" + s.add_dependency "rotp" + s.add_dependency "rqrcode" s.add_dependency "webauthn" # Third party libraries for admin pages diff --git a/lib/koi/engine.rb b/lib/koi/engine.rb index 5d60bcdbe..c9bc4af9f 100644 --- a/lib/koi/engine.rb +++ b/lib/koi/engine.rb @@ -7,6 +7,8 @@ require "katalyst/navigation" require "katalyst/tables" require "pagy" +require "rotp" +require "rqrcode" require "stimulus-rails" require "turbo-rails" require "webauthn" diff --git a/spec/factories/admins.rb b/spec/factories/admins.rb index 0837e1683..6451801fe 100644 --- a/spec/factories/admins.rb +++ b/spec/factories/admins.rb @@ -5,5 +5,6 @@ email { Faker::Internet.email } name { Faker::Name.name } password { Faker::Internet.password } + otp_secret { ROTP::Base32.random } end end diff --git a/spec/requests/admin/otps_controller_spec.rb b/spec/requests/admin/otps_controller_spec.rb new file mode 100644 index 000000000..2bbcf39ed --- /dev/null +++ b/spec/requests/admin/otps_controller_spec.rb @@ -0,0 +1,94 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe Admin::OtpsController do + subject { action && response } + + let(:admin) { create(:admin, otp_secret: nil) } + + include_context "with admin session" + + describe "GET /admin/admin_users/:admin_user_id/otp/new" do + let(:action) { get new_admin_admin_user_otp_path(admin), as: :turbo_stream } + + it_behaves_like "requires admin" + + it "renders successfully" do + action + expect(response).to have_http_status(:success) + end + + context "with another user's id" do + let(:session_for) { create(:admin) } + + it "returns an error" do + action + expect(response).to have_http_status(:forbidden) + end + end + end + + describe "POST /admin/admin_users/:admin_user_id/otp" do + let(:action) do + admin.otp_secret = ROTP::Base32.random + + post admin_admin_user_otp_path(admin), + params: { admin: { otp_secret: admin.otp_secret, token: admin.otp.now } }, + as: :turbo_stream + end + + it_behaves_like "requires admin" + + it "redirects to user" do + action + expect(response).to have_http_status(:see_other).and(redirect_to(admin_admin_user_path(admin))) + end + + it "sets otp secret" do + expect { action }.to(change { admin.reload.otp_secret }) + end + + context "with token mismatch" do + let(:action) do + admin.otp_secret = ROTP::Base32.random + + post admin_admin_user_otp_path(admin), + params: { admin: { otp_secret: admin.otp_secret, token: "000000" } }, + as: :turbo_stream + end + + it "updates the form" do + action + html = Nokogiri::HTML.fragment(response.body) + root = Capybara::Node::Simple.new(html) + expect(root).to have_css("turbo-stream[action='replace'][target='otp_admin_#{admin.id}']") + end + + it "uses the same secret when re-rendering" do + action + html = Nokogiri::HTML.fragment(response.body) + secret = html.at_css("input[name='admin[otp_secret]']") + expect(secret.attributes["value"].value).to eq(admin.otp_secret) + end + + it "does not set the otp secret" do + expect { action }.not_to(change { admin.reload.otp_secret }) + end + end + end + + describe "DELETE /admin/admin_users/:id/otp" do + let(:action) do + delete admin_admin_user_otp_path(admin), as: :turbo_stream + end + + let(:admin) { create(:admin) } + + it_behaves_like "requires admin" + + it "removes the otp secret" do + expect { action }.to(change { admin.reload.otp_secret }.to(nil)) + end + end +end diff --git a/spec/requests/admin/sessions_controller_spec.rb b/spec/requests/admin/sessions_controller_spec.rb index f115aaa97..b3caeecfb 100644 --- a/spec/requests/admin/sessions_controller_spec.rb +++ b/spec/requests/admin/sessions_controller_spec.rb @@ -32,65 +32,74 @@ as: :turbo_stream end - context "with username/password" do - let(:session_params) do - { - email: admin.email, - password: admin.password, - } - end + it "accepts email and prompts for password" do + post admin_session_path, params: { admin: { email: admin.email } }, as: :turbo_stream + expect(response).to have_http_status(:unprocessable_content).and(have_rendered("password")) + end - it "renders successfully" do - action - expect(response).to redirect_to(admin_dashboard_path) - end + it "accepts invalid emails without leaking account existence" do + post admin_session_path, params: { admin: { email: "invalid@example.com" } }, as: :turbo_stream + expect(response).to have_http_status(:unprocessable_content).and(have_rendered("password")) + end - it "creates the admin session" do - action - expect(session[:admin_user_id]).to be_present - end + it "fails on invalid email" do + post admin_session_path, params: { admin: { email: "invalid@example.com" } }, as: :turbo_stream + post admin_session_path, params: { admin: { email: "invalid@example.com", password: admin.password } }, + as: :turbo_stream + expect(response).to have_http_status(:unprocessable_content).and(have_rendered("new")) + end - it "updates login metadata" do - expect { action }.to(change { admin.reload.current_sign_in_at }) - end + it "fails on invalid password" do + post admin_session_path, params: { admin: { email: admin.email } }, as: :turbo_stream + post admin_session_path, params: { admin: { email: admin.email, password: "invalid" } }, as: :turbo_stream + expect(response).to have_http_status(:unprocessable_content).and(have_rendered("new")) end - context "with archived user" do - let(:admin) { create(:admin, archived: true) } - let(:session_params) do - { - email: admin.email, - password: admin.password, - } - end + it "accepts email and password and prompts for otp" do + post admin_session_path, params: { admin: { email: admin.email } }, as: :turbo_stream + post admin_session_path, params: { admin: { email: admin.email, password: admin.password } }, as: :turbo_stream + expect(response).to have_http_status(:unprocessable_content).and(have_rendered("otp")) + end - it "renders an error" do - action - expect(response).to have_http_status(:unprocessable_content) - end + it "fails on invalid otp" do + post admin_session_path, params: { admin: { email: admin.email } }, as: :turbo_stream + post admin_session_path, params: { admin: { email: admin.email, password: admin.password } }, as: :turbo_stream + post admin_session_path, params: { admin: { token: "000000" } }, as: :turbo_stream + expect(response).to have_http_status(:unprocessable_content).and(have_rendered("new")) + end - it "does not create the admin session" do - action - expect(session[:admin_user_id]).not_to be_present - end + it "accepts otp and redirects to dashboard" do + post admin_session_path, params: { admin: { email: admin.email } }, as: :turbo_stream + post admin_session_path, params: { admin: { email: admin.email, password: admin.password } }, as: :turbo_stream + post admin_session_path, params: { admin: { token: admin.otp.now } }, as: :turbo_stream + expect(response).to have_http_status(:see_other).and(redirect_to(admin_dashboard_path)) end - context "with invalid credentials" do - let(:session_params) do - { - email: admin.email, - password: "invalid", - } - end + it "accepts otp and updates login metadata" do + expect do + post admin_session_path, params: { admin: { email: admin.email } }, as: :turbo_stream + post admin_session_path, params: { admin: { email: admin.email, password: admin.password } }, as: :turbo_stream + post admin_session_path, params: { admin: { token: admin.otp.now } }, as: :turbo_stream + end.to(change { admin.reload.current_sign_in_at }) + end - it "renders an error" do - action - expect(response).to have_http_status(:unprocessable_content) + context "with no otp present" do + let(:admin) { create(:admin, otp_secret: "") } + + it "accepts otp and redirects to dashboard" do + post admin_session_path, params: { admin: { email: admin.email } }, as: :turbo_stream + post admin_session_path, params: { admin: { email: admin.email, password: admin.password } }, as: :turbo_stream + expect(response).to have_http_status(:see_other).and(redirect_to(admin_dashboard_path)) end + end - it "does not create the admin session" do - action - expect(session[:admin_user_id]).not_to be_present + context "with archived user" do + let(:admin) { create(:admin, archived: true) } + + it "fails after email and password" do + post admin_session_path, params: { admin: { email: admin.email } }, as: :turbo_stream + post admin_session_path, params: { admin: { email: admin.email, password: admin.password } }, as: :turbo_stream + expect(response).to have_http_status(:unprocessable_content).and(have_rendered("new")) end end diff --git a/spec/system/admin/authentication_spec.rb b/spec/system/admin/authentication_spec.rb index 0a9e7e621..6896c839f 100644 --- a/spec/system/admin/authentication_spec.rb +++ b/spec/system/admin/authentication_spec.rb @@ -3,13 +3,31 @@ require "rails_helper" RSpec.describe "admin/authentication" do - it "supports password login" do + it "supports password login with 2fa" do admin = create(:admin) visit "/admin" fill_in "Email", with: admin.email + click_on "Next" + fill_in "Password", with: admin.password - click_on "Log in" + click_on "Next" + + fill_in "Token", with: admin.otp.now + click_on "Next" + + expect(page).to have_current_path("/admin/dashboard") + end + + it "supports password login without 2fa" do + admin = create(:admin, otp_secret: "") + visit "/admin" + + fill_in "Email", with: admin.email + click_on "Next" + + fill_in "Password", with: admin.password + click_on "Next" expect(page).to have_current_path("/admin/dashboard") end @@ -19,8 +37,13 @@ visit "/admin/admin_users" fill_in "Email", with: admin.email + click_on "Next" + fill_in "Password", with: admin.password - click_on "Log in" + click_on "Next" + + fill_in "Token", with: admin.otp.now + click_on "Next" expect(page).to have_current_path("/admin/admin_users") end diff --git a/spec/system/admin/invitation_spec.rb b/spec/system/admin/invitation_spec.rb index b790f5f56..6eba709e5 100644 --- a/spec/system/admin/invitation_spec.rb +++ b/spec/system/admin/invitation_spec.rb @@ -9,13 +9,16 @@ def encode_token(**args) it "creates an invitation" do admin = create(:admin) - visit "/admin" + visit "/admin/admin_users/new" fill_in "Email", with: admin.email + click_on "Next" + fill_in "Password", with: admin.password - click_on "Log in" + click_on "Next" - visit "/admin/admin_users/new" + fill_in "Token", with: admin.otp.now + click_on "Next" fill_in "Email", with: "john.doe@gmail.com" fill_in "Name", with: "John Doe" diff --git a/spec/system/index/filtering_spec.rb b/spec/system/index/filtering_spec.rb index c5bb47ba8..40fbe931b 100644 --- a/spec/system/index/filtering_spec.rb +++ b/spec/system/index/filtering_spec.rb @@ -10,8 +10,13 @@ visit "/admin" fill_in "Email", with: admin.email + click_on "Next" + fill_in "Password", with: admin.password - click_on "Log in" + click_on "Next" + + fill_in "Token", with: admin.otp.now + click_on "Next" %i[first second third].map do |n| create(:post, name: n, title: n.to_s.titleize) diff --git a/spec/system/index/ordinal_spec.rb b/spec/system/index/ordinal_spec.rb index faa02282a..71411ee03 100644 --- a/spec/system/index/ordinal_spec.rb +++ b/spec/system/index/ordinal_spec.rb @@ -9,8 +9,13 @@ visit "/admin" fill_in "Email", with: admin.email + click_on "Next" + fill_in "Password", with: admin.password - click_on "Log in" + click_on "Next" + + fill_in "Token", with: admin.otp.now + click_on "Next" %i[first second third].each_with_index do |n, i| create(:banner, name: n, ordinal: i) diff --git a/spec/system/index/pagination_spec.rb b/spec/system/index/pagination_spec.rb index 040feec0b..52c0ff6df 100644 --- a/spec/system/index/pagination_spec.rb +++ b/spec/system/index/pagination_spec.rb @@ -12,8 +12,13 @@ visit "/admin" fill_in "Email", with: admin.email + click_on "Next" + fill_in "Password", with: admin.password - click_on "Log in" + click_on "Next" + + fill_in "Token", with: admin.otp.now + click_on "Next" end context "when there are more than 20 results" do diff --git a/spec/system/index/redirect_spec.rb b/spec/system/index/redirect_spec.rb index df3132cec..a0e26eab1 100644 --- a/spec/system/index/redirect_spec.rb +++ b/spec/system/index/redirect_spec.rb @@ -10,8 +10,13 @@ visit "/admin" fill_in "Email", with: admin.email + click_on "Next" + fill_in "Password", with: admin.password - click_on "Log in" + click_on "Next" + + fill_in "Token", with: admin.otp.now + click_on "Next" end it "can redirect to index via turbo" do diff --git a/spec/system/index/sorting_spec.rb b/spec/system/index/sorting_spec.rb index 6d60a6288..117c31915 100644 --- a/spec/system/index/sorting_spec.rb +++ b/spec/system/index/sorting_spec.rb @@ -9,8 +9,13 @@ visit "/admin" fill_in "Email", with: admin.email + click_on "Next" + fill_in "Password", with: admin.password - click_on "Log in" + click_on "Next" + + fill_in "Token", with: admin.otp.now + click_on "Next" %i[first second third].map do |n| create(:post, name: n, title: n.to_s.titleize) diff --git a/spec/system/index/table_spec.rb b/spec/system/index/table_spec.rb index 149aca85f..4e890f9c4 100644 --- a/spec/system/index/table_spec.rb +++ b/spec/system/index/table_spec.rb @@ -9,8 +9,13 @@ visit "/admin" fill_in "Email", with: admin.email + click_on "Next" + fill_in "Password", with: admin.password - click_on "Log in" + click_on "Next" + + fill_in "Token", with: admin.otp.now + click_on "Next" end it "renders a table" do