Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: MFA for admins #635

Merged
merged 3 commits into from
Dec 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
2 changes: 2 additions & 0 deletions app/controllers/admin/admin_users_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
52 changes: 52 additions & 0 deletions app/controllers/admin/otps_controller.rb
Original file line number Diff line number Diff line change
@@ -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
79 changes: 67 additions & 12 deletions app/controllers/admin/sessions_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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,
Comment on lines +69 to +70
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(opt) Use constants for magic numbers

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)
Expand Down
22 changes: 22 additions & 0 deletions app/models/admin/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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
21 changes: 21 additions & 0 deletions app/models/concerns/koi/model/otp.rb
Original file line number Diff line number Diff line change
@@ -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
sfnelson marked this conversation as resolved.
Show resolved Hide resolved

def otp
ROTP::TOTP.new(otp_secret) if otp_secret.present?
end
end
end
end
3 changes: 3 additions & 0 deletions app/views/admin/admin_users/index.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -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:) %>
18 changes: 16 additions & 2 deletions app/views/admin/admin_users/show.html+self.erb
Original file line number Diff line number Diff line change
Expand Up @@ -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| %>
<span class="repel">
<%= 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 %>
sfnelson marked this conversation as resolved.
Show resolved Hide resolved
</span>
<% end %>
<% end %>

<div class="repel">
Expand Down
10 changes: 4 additions & 6 deletions app/views/admin/admin_users/show.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -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 %>

<h3>Passkeys</h3>

<%= render "admin/credentials/credentials", admin: %>

<div class="actions">
<% if admin.archived? %>
<%= button_to "Delete", admin_admin_user_path(admin),
Expand Down
31 changes: 31 additions & 0 deletions app/views/admin/otps/_form.html.erb
Original file line number Diff line number Diff line change
@@ -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| %>
<section class="flow prose">
<p>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.</p>
<p>In general, we recommend using Passkeys over MFA. Passkeys offer better
security than a password + MFA, and they are easier to use.</p>
<p><strong>Add an MFA authenticator to your account</strong></p>
<ol class="flow">
<li>Install an MFA app. Most password managers support MFA.</li>
<li>Scan this code using your mobile device or password manager:<br>
<%== RQRCode::QRCode.new(admin.otp.provisioning_uri(admin.email)).as_svg(
color: "000",
shape_rendering: "crispEdges",
module_size: 3,
use_path: true,
) %>
</li>
<li>Enter the token shown in your app into the field below:</li>
</ol>
</section>
<%= form.hidden_field :otp_secret %>
<%= form.govuk_text_field :token %>
<%= form.admin_save %>
<% end %>
5 changes: 5 additions & 0 deletions app/views/admin/otps/create.turbo_stream.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<%# locals: (admin:) %>

<%= turbo_stream.replace(dom_id(admin, :otp)) do %>
<%= render("form", admin:) %>
<% end %>
5 changes: 5 additions & 0 deletions app/views/admin/otps/new.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<%# locals: (admin:) %>

<%= render Kpop::ModalComponent.new(title: "Configure MFA") do %>
<%= render("form", admin:) %>
<% end %>
20 changes: 10 additions & 10 deletions app/views/admin/sessions/new.html.erb
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
<%# locals: (admin_user:) %>

<%= render "layouts/koi/navigation_header" %>

<%= form_with(
model: admin_user,
url: admin_session_path,
data: {
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? %>
<div class="govuk-error-summary">
Expand All @@ -17,15 +20,12 @@
</ul>
</div>
<% 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) %>
<div class="actions-group">
<%= 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" } %>
</div>
<% end %>
Loading
Loading