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

Setup SSO for Admin app #3138

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft
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
2 changes: 2 additions & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,8 @@ gem "devise-authy", ">= 1.10.0"
gem "pundit", "~> 0.3"
gem "devise_zxcvbn", ">= 4.4.1"
gem "devise-security", "~> 0.18.0"
gem "omniauth-rails_csrf_protection"
gem "omniauth-oauth2"

# GOV.UK Notify support (for mailers)
gem "mail-notify", "~> 2.0"
Expand Down
19 changes: 19 additions & 0 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -352,6 +352,8 @@ GEM
money (6.16.0)
i18n (>= 0.6.4, <= 2)
multi_json (1.15.0)
multi_xml (0.7.1)
bigdecimal (~> 3.1)
multipart-post (2.3.0)
mustermann (3.0.0)
ruby2_keywords (~> 0.0.1)
Expand Down Expand Up @@ -379,6 +381,21 @@ GEM
racc (~> 1.4)
notifications-ruby-client (6.0.0)
jwt (>= 1.5, < 3)
oauth2 (1.4.11)
faraday (>= 0.17.3, < 3.0)
jwt (>= 1.0, < 3.0)
multi_json (~> 1.3)
multi_xml (~> 0.5)
rack (>= 1.2, < 4)
omniauth (1.9.2)
hashie (>= 3.4.6)
rack (>= 1.6.2, < 3)
omniauth-oauth2 (1.3.1)
oauth2 (~> 1.0)
omniauth (~> 1.2)
omniauth-rails_csrf_protection (0.1.2)
actionpack (>= 4.2)
omniauth (>= 1.3.1)
orm_adapter (0.5.0)
pagy (7.0.11)
paper_trail (12.2.0)
Expand Down Expand Up @@ -775,6 +792,8 @@ DEPENDENCIES
matrix
nilify_blanks
nokogiri
omniauth-oauth2
omniauth-rails_csrf_protection
paper_trail (~> 12.2.0)
paper_trail-association_tracking
pdf-inspector
Expand Down
57 changes: 4 additions & 53 deletions app/controllers/admin/admins_controller.rb
Original file line number Diff line number Diff line change
@@ -1,54 +1,14 @@
class Admin::AdminsController < Admin::UsersController
before_action :find_resource, except: [:index, :new, :create, :login_as_assessor, :login_as_user]
class Admin::AdminsController < Admin::BaseController
before_action :find_resource, except: [:index, :login_as_assessor, :login_as_user]

def index
params[:search] ||= AdminSearch::DEFAULT_SEARCH
params[:search].permit!
authorize Admin, :index?
@search = AdminSearch.new(Admin.all)
.search(params[:search])
@resources = @search.results.page(params[:page])
end

def new
@resource = Admin.new
authorize @resource, :create?
end

def create
@resource = Admin.new(resource_params)
@resource.skip_password_validation = true
authorize @resource, :create?

@resource.save

render_flash_message_for(@resource, message: @resource.errors.none? ? nil : @resource.errors.messages.values.flatten.uniq.join("<br />"))

location = @resource.persisted? ? admin_admins_path : nil
respond_with :admin, @resource, location: location
end

def update
authorize @resource, :update?

if resource_params[:password].present?
@resource.update(resource_params)
else
@resource.skip_password_validation = true
@resource.update_without_password(resource_params)
end

render_flash_message_for(@resource, message: @resource.errors.none? ? nil : @resource.errors.messages.values.flatten.uniq.join("<br />"))

respond_with :admin, @resource, location: admin_admins_path
end

def destroy
authorize @resource, :destroy?
@resource.soft_delete!

render_flash_message_for(@resource, message: @resource.errors.none? ? nil : @resource.errors.messages.values.flatten.uniq.join("<br />"))

respond_with :admin, @resource, location: admin_admins_path
render template: "admin/users/index"
end

# NOTE: debug abilities for Admin - BEGIN
Expand All @@ -74,13 +34,4 @@ def login_as_user
def find_resource
@resource = Admin.find(params[:id])
end

def resource_params
params.require(:admin)
.permit(:email,
:password,
:password_confirmation,
:first_name,
:last_name)
end
end
7 changes: 0 additions & 7 deletions app/controllers/admins/confirmations_controller.rb
Original file line number Diff line number Diff line change
@@ -1,9 +1,2 @@
class Admins::ConfirmationsController < Devise::ConfirmationsController
include PasswordSettable

private

def password_reset_path(token)
edit_admin_password_path(reset_password_token: token, locals: { password_not_set: true })
end
end
33 changes: 33 additions & 0 deletions app/controllers/admins/omniauth_callbacks_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
class Admins::OmniauthCallbacksController < Devise::OmniauthCallbacksController
skip_before_action :verify_authenticity_token, only: :developer

Check failure

Code scanning / CodeQL

CSRF protection weakened or disabled High

Potential CSRF vulnerability due to forgery protection being disabled or weakened.

Copilot Autofix AI about 1 month ago

To fix the CSRF vulnerability, we should re-enable CSRF protection for the developer action. This can be done by removing the skip_before_action :verify_authenticity_token, only: :developer line. Additionally, we can add a safeguard to ensure that the developer action is only accessible in development environments by using a more secure approach, such as an environment-specific route constraint.

Suggested changeset 1
app/controllers/admins/omniauth_callbacks_controller.rb

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/app/controllers/admins/omniauth_callbacks_controller.rb b/app/controllers/admins/omniauth_callbacks_controller.rb
--- a/app/controllers/admins/omniauth_callbacks_controller.rb
+++ b/app/controllers/admins/omniauth_callbacks_controller.rb
@@ -1,3 +1,2 @@
 class Admins::OmniauthCallbacksController < Devise::OmniauthCallbacksController
-  skip_before_action :verify_authenticity_token, only: :developer
 
EOF
@@ -1,3 +1,2 @@
class Admins::OmniauthCallbacksController < Devise::OmniauthCallbacksController
skip_before_action :verify_authenticity_token, only: :developer

Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options

def dbt_staff_sso
FindAndUpdateWithAuth.new(auth: request.env["omniauth.auth"], model: Admin).run.tap do |result|
if result.success
sign_in_and_redirect result.user, event: :authentication
set_flash_message(:notice, :success, kind: "DBT Staff SSO")
else
set_flash_message(:notice, :error, kind: result.errors)
redirect_to new_admin_session_url
end
end
end

def developer
raise "developer strategy should only be used in development!" unless Rails.env.development?

FindAndUpdateWithAuth.new(auth: request.env["omniauth.auth"], model: Admin).run.tap do |result|
if result.success
sign_in_and_redirect result.user, event: :authentication
set_flash_message(:notice, :success, kind: "Developer SSO")
else
set_flash_message(:notice, :error, kind: result.errors)
redirect_to new_admin_session_url
end
end
end

def failure
redirect_to admin_root_path
end
end
2 changes: 2 additions & 0 deletions app/controllers/admins/sessions_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
class Admins::SessionsController < Devise::SessionsController
end
8 changes: 8 additions & 0 deletions app/controllers/application_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,14 @@ def after_sign_in_path_for(resource)
end
end

def after_sign_out_path_for(scope)
if scope == :admin
new_admin_session_path
else
super
end
end

def admin_in_read_only_mode?
@admin_in_read_only_mode ||= (admin_signed_in? || assessor_signed_in?) &&
session["warden.user.user.key"] &&
Expand Down
52 changes: 52 additions & 0 deletions app/interactors/find_and_update_with_auth.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
class FindAndUpdateWithAuth
attr_reader :success, :errors, :user

def initialize(auth:, model:)
@model = model
@sso_uid = auth.uid
@sso_info = auth.info
@sso_provider = auth.provider
end

def run
@user = find_or_create_user
@success = @user.persisted?
@errors = @user.errors unless @success
self
end

private

# 1 - check if they have already set up sso; when they have update them to ensure we capture the current email/first_name/last_name
# 2 - check if the user already existed prior to sso; when they do update them with the auth details
# 3 - finally if they dont exist; create them from the auth details
def find_or_create_user
find_and_update_with_auth(sso_provider: @sso_provider, sso_uid: @sso_uid) || find_and_update_with_auth(email: @sso_info.email) || create_from_auth
end

def find_and_update_with_auth(conditions)
raise "conditions should not be nil: #{conditions}" if conditions.compact.empty?

existing = @model.find_by(**conditions)
return unless existing

@model.update(
existing.id,
sso_provider: @sso_provider,
sso_uid: @sso_uid,
email: @sso_info.email,
first_name: @sso_info.first_name,
last_name: @sso_info.last_name,
)
end

def create_from_auth
@model.create(
email: @sso_info.email,
first_name: @sso_info.first_name,
last_name: @sso_info.last_name,
sso_uid: @sso_uid,
sso_provider: @sso_provider,
)
end
end
10 changes: 3 additions & 7 deletions app/models/admin.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,11 @@ class Admin < ApplicationRecord
include PgSearch::Model
include AutosaveTokenGeneration

# Include default devise modules. Others available are:
# :confirmable, :lockable, :timeoutable and :omniauthable

devise :authy_authenticatable, :database_authenticatable,
:recoverable, :trackable, :validatable, :confirmable,
:zxcvbnable, :lockable, :timeoutable, :session_limitable
include PasswordValidator
devise :trackable, :timeoutable, :session_limitable
devise :omniauthable, omniauth_providers: %i[dbt_staff_sso developer]

validates :first_name, :last_name, presence: true
validates :email, format: { with: Devise.email_regexp }

has_many :form_answer_attachments, as: :attachable

Expand Down
10 changes: 2 additions & 8 deletions app/views/admin/admins/_list.html.slim
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
th.sortable
= sort_link f, 'Signed in on', @search, :last_sign_in_at
th.sortable
= sort_link f, "Confirmed on", @search, :confirmed_at
= sort_link f, 'Single sign on ID', @search, :sso_uid
tbody
- if AdminDecorator.decorate_collection(resources).none?
tr
Expand All @@ -30,10 +30,4 @@
= admin.formatted_last_sign_in_at_long
span.hidden-lg
= admin.formatted_last_sign_in_at_short
td
- if admin.confirmed_at.present?
small.text-muted
= l admin.confirmed_at, format: :date_at_time
- else
small.text-danger
' Not confirmed
td = admin.sso_uid
5 changes: 3 additions & 2 deletions app/views/admin/users/index.html.slim
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,9 @@
= link_to "Edit assessors’ access to the system", suspension_status_admin_assessors_path, class: "btn btn-default btn-md"
' &nbsp;

= link_to public_send("new_admin_#{controller_name.singularize}_path"), class: 'new-user btn btn-secondary btn-md' do
= "+ Add #{controller_name == "users" ? "applicant" : controller_name.singularize}"
- unless controller_name == "admins"
= link_to public_send("new_admin_#{controller_name.singularize}_path"), class: 'new-user btn btn-secondary btn-md' do
= "+ Add #{controller_name == "users" ? "applicant" : controller_name.singularize}"
.clear

- if @search.query?
Expand Down
17 changes: 8 additions & 9 deletions app/views/admins/sessions/new.html.slim
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,15 @@
= resource.model_name
' - Sign in

= simple_form_for(resource, as: resource_name, url: session_path(resource_name)) do |f|
- unless devise_mapping.omniauthable?
= simple_form_for(resource, as: resource_name, url: session_path(resource_name)) do |f|
.govuk-form-group
= f.input :email, required: false, autofocus: true, label: "Email", input_html: {class: "govuk-input--width-20"}

.govuk-form-group
= f.input :email, required: false, autofocus: true, label: "Email", input_html: {class: "govuk-input--width-20"}
.govuk-form-group
= f.input :password, required: false, label: "Password", input_html: {class: "govuk-input--width-10"}

.govuk-form-group
= f.input :password, required: false, label: "Password", input_html: {class: "govuk-input--width-10"}

= f.input :remember_me, as: :boolean if devise_mapping.rememberable?

= f.button :submit, "Sign in", class: "button large"
= f.input :remember_me, as: :boolean if devise_mapping.rememberable?
= f.button :submit, "Sign in", class: "button large"

= render "admins/shared/links"
9 changes: 6 additions & 3 deletions app/views/admins/shared/_links.html.slim
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@
p
= link_to "Didn't receive unlock instructions?", new_unlock_path(resource_name), class: 'govuk-link'
- if devise_mapping.omniauthable?
- resource_class.omniauth_providers.each do |provider|
p
= link_to "Sign in with #{provider.to_s.titleize}", omniauth_authorize_path(resource_name, provider), class: 'govuk-link'
- if Rails.env.development?
= link_to "Sign in with Developer", omniauth_authorize_path(resource_name, :developer), class: 'govuk-link', method: :post
- else
- resource_class.omniauth_providers.excluding(:developer).each do |provider|
p
= link_to "Sign in with #{provider.to_s.titleize}", omniauth_authorize_path(resource_name, provider), class: 'govuk-link', method: :post
8 changes: 0 additions & 8 deletions app/views/layouts/application-admin.html.slim
Original file line number Diff line number Diff line change
Expand Up @@ -91,10 +91,6 @@ html.no-js
strong = current_subject.decorate.full_name
br
small = current_subject.email
li.divider
- unless current_admin.authy_enabled
li
= link_to "Enable 2FA", admin_enable_authy_path
li
= button_to "Sign out",
destroy_admin_session_path,
Expand All @@ -110,10 +106,6 @@ html.no-js
strong = current_subject.decorate.full_name
br
small = current_subject.email
li.divider
- unless current_admin.authy_enabled
li
= link_to "Enable 2FA", admin_enable_authy_path
li
= link_to "Sign out",
destroy_admin_session_path,
Expand Down
7 changes: 7 additions & 0 deletions config/initializers/devise.rb
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,13 @@
# manager.default_strategies(scope: :user).unshift :some_external_strategy
# end

config.omniauth :dbt_staff_sso,
ENV.fetch("DBT_STAFF_SSO_APP_ID", nil), # remove the nils once we have these set in ci etc
ENV.fetch("DBT_STAFF_SSO_APP_SECRET", nil),
info_fields: OmniAuth::Strategies::DbtStaffSso::INFO_FIELD_NAMES

config.omniauth :developer, fields: [:first_name, :last_name, :email] if Rails.env.development?

config.warden do |manager|
manager.failure_app = QaeFailureApp
end
Expand Down
14 changes: 6 additions & 8 deletions config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,12 @@
unlocks: "users/unlocks",
}

devise_for :admins, controllers: {
confirmations: "admins/confirmations",
devise_authy: "admin/devise_authy",
}, path_names: {
verify_authy: "/verify-token",
enable_authy: "/enable-two-factor",
verify_authy_installation: "/verify-installation",
}
devise_for :admins, controllers: { omniauth_callbacks: "admins/omniauth_callbacks" }

devise_scope :admin do
get "admins/sign_in", to: "devise/sessions#new", as: :new_admin_session
delete "admins/sign_out", to: "devise/sessions#destroy", as: :destroy_admin_session
end

authenticate :admin do
mount Sidekiq::Web => "/sidekiq"
Expand Down
6 changes: 6 additions & 0 deletions db/migrate/20241028142655_add_omniauth_to_admins.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
class AddOmniauthToAdmins < ActiveRecord::Migration[7.0]
def change
add_column :admins, :sso_provider, :string
add_column :admins, :sso_uid, :string
end
end
1 change: 0 additions & 1 deletion db/seeds.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
unless Admin.exists?
admin_args = {
email: "[email protected]",
password: "^#ur9EkLm@1W+OaDvg",
first_name: "First name",
last_name: "Last name",
confirmed_at: DateTime.now
Expand Down
Loading
Loading