Skip to content
Open
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
14 changes: 14 additions & 0 deletions app/controllers/application_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ def current_user = nil # TODO: this is a temp hack to fix partials until /backen
helper_method :detected_country_alpha2

before_action :invalidate_v1_sessions, :authenticate_identity!, :set_honeybadger_context
before_action :require_two_factor_enrollment!

before_action :set_paper_trail_whodunnit
before_action :touch_session_last_seen_at
Expand Down Expand Up @@ -44,6 +45,19 @@ def authenticate_identity!
end
end

def require_two_factor_enrollment!
return unless identity_signed_in?
return unless current_identity.two_factor_enrollment_required?

flash[:error] = "Your account requires two-factor authentication. Set up an authenticator app or passkey to continue."
if request.headers["HX-Request"]
response.headers["HX-Redirect"] = security_path
head :ok
else
redirect_to security_path
end
end

def set_honeybadger_context
return unless current_identity

Expand Down
4 changes: 4 additions & 0 deletions app/controllers/authorized_applications_controller.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
class AuthorizedApplicationsController < ApplicationController
include AhoyAnalytics

# Rendered as a panel on the security page, which is the enrollment gate's
# destination — and revoking app access is safe for un-enrolled users.
skip_before_action :require_two_factor_enrollment!

def index
@access_tokens = current_identity.access_tokens
.includes(:application)
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/backend/identities_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,7 @@ def set_identity
end

def identity_params
permitted = [ :first_name, :last_name, :legal_first_name, :legal_last_name, :primary_email, :phone_number, :birthday, :country, :hq_override, :ysws_eligible, :permabanned, :disallow_slack ]
permitted = [ :first_name, :last_name, :legal_first_name, :legal_last_name, :primary_email, :phone_number, :birthday, :country, :hq_override, :ysws_eligible, :permabanned, :disallow_slack, :two_factor_required ]
permitted << :can_hq_officialize if current_user&.super_admin?
params.require(:identity).permit(permitted)
end
Expand Down
7 changes: 7 additions & 0 deletions app/controllers/identities_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,13 @@ def toggle_2fa
return
end

# Admin-required 2FA can't be disabled by the user
if current_identity.two_factor_required?
flash[:error] = "Two-factor authentication is required for your account and can't be disabled."
redirect_to security_path
return
end

# Disabling 2FA requires step-up auth
redirect_to new_step_up_path(action_type: "disable_2fa")
end
Expand Down
2 changes: 2 additions & 0 deletions app/controllers/identity_backup_codes_controller.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
class IdentityBackupCodesController < ApplicationController
skip_before_action :require_two_factor_enrollment!

def index
@backup_codes = current_identity.backup_codes.active.order(created_at: :desc)

Expand Down
2 changes: 2 additions & 0 deletions app/controllers/identity_sessions_controller.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
class IdentitySessionsController < ApplicationController
skip_before_action :require_two_factor_enrollment!

def index
@sessions = current_identity.sessions
.where(signed_out_at: nil)
Expand Down
2 changes: 2 additions & 0 deletions app/controllers/identity_totps_controller.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
class IdentityTotpsController < ApplicationController
skip_before_action :require_two_factor_enrollment!

def index
@totp = current_identity.totp

Expand Down
2 changes: 2 additions & 0 deletions app/controllers/identity_webauthn_credentials_controller.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
class IdentityWebauthnCredentialsController < ApplicationController
include WebauthnAuthenticatable

skip_before_action :require_two_factor_enrollment!

def index
@webauthn_credentials = current_identity.webauthn_credentials.order(created_at: :desc)
render layout: request.headers["HX-Request"] ? "htmx" : false
Expand Down
1 change: 1 addition & 0 deletions app/controllers/logins_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ class LoginsController < ApplicationController
WEBAUTHN_SESSION_KEY = :webauthn_authentication_challenge

skip_before_action :authenticate_identity!
skip_before_action :require_two_factor_enrollment!
before_action :set_return_to, only: [ :new, :create ]
before_action :set_attempt, except: [ :new, :create ]
before_action :validate_browser_token, except: [ :new, :create ]
Expand Down
2 changes: 2 additions & 0 deletions app/controllers/sessions_controller.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
class SessionsController < ApplicationController
skip_before_action :require_two_factor_enrollment!

def logout
flash[:info] = "You've been logged out. Nice seeing you!"
sign_out
Expand Down
1 change: 1 addition & 0 deletions app/controllers/static_pages_controller.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
class StaticPagesController < ApplicationController
skip_before_action :authenticate_identity!, only: [ :external_api_docs, :welcome, :oauth_welcome ]
skip_before_action :require_two_factor_enrollment!, only: [ :security ]

def home
@sso_apps = SAMLService::Entities.service_providers.values.select do |sp|
Expand Down
16 changes: 16 additions & 0 deletions app/controllers/step_up_controller.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
class StepUpController < ApplicationController
include WebauthnAuthenticatable

skip_before_action :require_two_factor_enrollment!

helper_method :step_up_cancel_path

WEBAUTHN_SESSION_KEY = :step_up_webauthn_challenge
Expand Down Expand Up @@ -147,6 +149,11 @@ def complete_step_up(action_type, return_to)
case action_type
when "remove_totp"
totp = current_identity.totp
if current_identity.two_factor_required? && (current_identity.two_factor_methods - [ totp ]).empty?
redirect_to security_path, alert: "Two-factor authentication is required for your account. Add another method before removing this one."
return
end

totp&.destroy
TwoFactorMailer.authentication_method_disabled(current_identity).deliver_later

Expand All @@ -159,6 +166,11 @@ def complete_step_up(action_type, return_to)
redirect_to security_path, notice: "Two-factor authentication disabled"

when "disable_2fa"
if current_identity.two_factor_required?
redirect_to security_path, alert: "Two-factor authentication is required for your account and can't be disabled."
return
end

current_identity.update!(use_two_factor_authentication: false)
TwoFactorMailer.required_authentication_disabled(current_identity).deliver_later
consume_step_up!
Expand All @@ -175,6 +187,10 @@ def complete_step_up(action_type, return_to)
when "remove_passkey"
credential_id = session.delete(:pending_destroy_credential_id)
credential = current_identity.webauthn_credentials.find_by(id: credential_id) if credential_id
if credential && current_identity.two_factor_required? && (current_identity.two_factor_methods - [ credential ]).empty?
redirect_to security_path, alert: "Two-factor authentication is required for your account. Add another method before removing this one."
return
end
if credential
credential.destroy
consume_step_up!
Expand Down
6 changes: 5 additions & 1 deletion app/models/identity.rb
Original file line number Diff line number Diff line change
Expand Up @@ -370,9 +370,13 @@ def has_two_factor_method? = two_factor_methods.any?
def primary_two_factor_method = two_factor_methods.first

def requires_two_factor?
use_two_factor_authentication? && has_two_factor_method?
(use_two_factor_authentication? || two_factor_required?) && has_two_factor_method?
end

# Admin-set override without any enrolled method: the user must enroll before
# they can use HCA (enforced by ApplicationController#require_two_factor_enrollment!)
def two_factor_enrollment_required? = two_factor_required? && !has_two_factor_method?

def legacy_migrated? = legacy_migrated_at.present?

def suggested_aadhaar_password
Expand Down
1 change: 1 addition & 0 deletions app/views/backend/identities/edit.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
<label><%= f.check_box :hq_override %> hq override <span style="color: var(--foreground2);">— bypass normal eligibility rules</span></label>
<label><%= f.check_box :ysws_eligible %> ysws eligible</label>
<label><%= f.check_box :disallow_slack %> disallow slack <span style="color: var(--foreground2);">— prevent SAML into Slack</span></label>
<label><%= f.check_box :two_factor_required %> require 2fa <span style="color: var(--foreground2);">— must enroll a 2FA method; blocks OAuth/SAML until enrolled</span></label>
<% super_admin_tool do %>
<label><%= f.check_box :permabanned %> permabanned <span style="color: var(--foreground2);">— permanently ban</span></label>
<label><%= f.check_box :can_hq_officialize %> can officialize apps</label>
Expand Down
3 changes: 2 additions & 1 deletion app/views/backend/identities/show.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,8 @@
<% if @identity.came_in_through_adult_program %><span is-="badge" variant-="foreground2">adult program</span><% end %>
<% if @identity.developer_mode %><span is-="badge" variant-="foreground2">developer</span><% end %>
<% if @identity.disallow_slack %><span is-="badge" variant-="red">no slack</span><% end %>
<% unless @identity.hq_override || @identity.can_hq_officialize || @identity.is_alum || @identity.came_in_through_adult_program || @identity.developer_mode || @identity.disallow_slack %>
<% if @identity.two_factor_required %><span is-="badge" variant-="<%= @identity.has_two_factor_method? ? 'blue' : 'red' %>">2fa required<%= " — not enrolled" unless @identity.has_two_factor_method? %></span><% end %>
<% unless @identity.hq_override || @identity.can_hq_officialize || @identity.is_alum || @identity.came_in_through_adult_program || @identity.developer_mode || @identity.disallow_slack || @identity.two_factor_required %>
<span style="color: var(--foreground2);">none</span>
<% end %>
</row>
Expand Down
4 changes: 3 additions & 1 deletion app/views/identity_totps/index.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@
<strong><%= t(".require_2fa_title") %></strong>
<p class="status-detail"><%= t(".require_2fa_description") %></p>
</div>
<% if current_identity.use_two_factor_authentication? %>
<% if current_identity.two_factor_required? %>
<span class="status-detail"><%= t(".required_by_hq") %></span>
<% elsif current_identity.use_two_factor_authentication? %>
<%= button_to t(".disable_requirement"),
toggle_2fa_identity_path,
method: :post,
Expand Down
7 changes: 7 additions & 0 deletions app/views/static_pages/security.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,13 @@

<div class="page-sections">

<% if current_identity.two_factor_enrollment_required? %>
<section class="section-card" style="border-color: var(--danger, #e5484d);">
<h3><%= t ".enrollment_required_heading" %></h3>
<p><%= t ".enrollment_required_description" %></p>
</section>
<% end %>

<section class="section-card">
<h3><%= t ".sessions" %></h3>
<%= render Components::BootlegTurbo.new(identity_sessions_path, id: "sessions-container", hx_swap: "innerHTML") %>
Expand Down
3 changes: 3 additions & 0 deletions config/locales/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -479,6 +479,8 @@ en:
security_link: Security
security:
heading: Security
enrollment_required_heading: Two-factor authentication required
enrollment_required_description: Your account requires two-factor authentication. Set up an authenticator app or passkey below to continue using Hack Club Auth.
sessions: Active Sessions
mfa: Authenticator App
webauthn: Passkeys
Expand Down Expand Up @@ -514,6 +516,7 @@ en:
require_2fa_description: When enabled, you must use a second factor (authenticator app or backup code) when signing in with an email code.
disable_requirement: Disable Requirement
enable_requirement: Enable Requirement
required_by_hq: Required by Hack Club HQ — this can't be disabled.
setup_description: Add an extra layer of security to email sign-ins by requiring a code from an authenticator app.
setup_authenticator: Set up two-factor authentication
show:
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class AddTwoFactorRequiredToIdentities < ActiveRecord::Migration[8.0]
def change
add_column :identities, :two_factor_required, :boolean, default: false, null: false
end
end
3 changes: 2 additions & 1 deletion db/schema.rb

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

45 changes: 45 additions & 0 deletions spec/models/identity_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -88,4 +88,49 @@
expect(other).not_to be_valid
end
end

describe "#requires_two_factor?" do
it "is false by default" do
expect(identity.requires_two_factor?).to be false
end

it "is true when the user enabled 2FA and has a method" do
identity.update!(use_two_factor_authentication: true)
identity.totps.create!.mark_verified!

expect(identity.requires_two_factor?).to be true
end

it "is true when the admin override is set and a method exists, even without the user flag" do
identity.update!(two_factor_required: true)
identity.totps.create!.mark_verified!

expect(identity.requires_two_factor?).to be true
end

it "is false when the admin override is set but no method exists" do
identity.update!(two_factor_required: true)

expect(identity.requires_two_factor?).to be false
end
end

describe "#two_factor_enrollment_required?" do
it "is true when the override is set and no method is enrolled" do
identity.update!(two_factor_required: true)

expect(identity.two_factor_enrollment_required?).to be true
end

it "is false once a method is enrolled" do
identity.update!(two_factor_required: true)
identity.totps.create!.mark_verified!

expect(identity.two_factor_enrollment_required?).to be false
end

it "is false without the override" do
expect(identity.two_factor_enrollment_required?).to be false
end
end
end
92 changes: 92 additions & 0 deletions spec/requests/two_factor_enrollment_gate_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
require "rails_helper"

RSpec.describe "Two-factor enrollment gate", type: :request do
let(:identity) { create(:identity, two_factor_required: true) }
let(:session) do
identity.sessions.create!(
session_token: SecureRandom.hex(32),
expires_at: 1.week.from_now
)
end

before do
allow_any_instance_of(ApplicationController).to receive(:current_identity).and_return(identity)
allow_any_instance_of(ApplicationController).to receive(:current_session).and_return(session)
allow_any_instance_of(ApplicationController).to receive(:identity_signed_in?).and_return(true)
end

context "when the identity has no 2FA method enrolled" do
it "redirects the home page to the security page" do
get "/"

expect(response).to redirect_to(security_path)
expect(flash[:error]).to include("requires two-factor authentication")
end

it "blocks OAuth authorization" do
get "/oauth/authorize"

expect(response).to redirect_to(security_path)
end

it "responds to HTMX requests with an HX-Redirect header" do
get "/", headers: { "HX-Request" => "true" }

expect(response).to have_http_status(:ok)
expect(response.headers["HX-Redirect"]).to eq(security_path)
end

it "allows the security page" do
get "/security"

expect(response).to have_http_status(:ok)
end

it "allows TOTP enrollment" do
get "/identity_totps"

expect(response).to have_http_status(:ok)
end

it "allows the authorized applications panel on the security page" do
get "/authorized_applications", headers: { "HX-Request" => "true" }

expect(response).to have_http_status(:ok)
expect(response.headers["HX-Redirect"]).to be_nil
end

it "allows passkey enrollment" do
get "/passkeys"

expect(response).to have_http_status(:ok)
end

it "allows logging out" do
delete "/logout"

expect(response).to redirect_to(welcome_path)
end
end

context "when the identity has a verified TOTP" do
before do
identity.totps.create!.mark_verified!
end

it "does not redirect" do
get "/"

expect(response).to have_http_status(:ok)
end
end

context "when the identity does not have the override" do
let(:identity) { create(:identity) }

it "does not redirect" do
get "/"

expect(response).to have_http_status(:ok)
end
end
end
Loading