Browse Source

Reset does not let you bypass 2FA

pull/313/head
baarkerlounger 3 years ago
parent
commit
ec113f0ec0
  1. 16
      app/controllers/auth/passwords_controller.rb
  2. 20
      app/controllers/auth/sessions_controller.rb
  3. 5
      app/controllers/users_controller.rb
  4. 2
      app/views/devise/passwords/edit.html.erb
  5. 2
      app/views/devise/passwords/reset_password.html.erb
  6. 8
      app/views/devise/sessions/new.html.erb
  7. 6
      config/routes.rb
  8. 72
      spec/features/admin_panel_spec.rb
  9. 55
      spec/features/admin_user_spec.rb
  10. 1
      spec/features/user_spec.rb
  11. 65
      spec/requests/active_admin/devise/passwords_controller_spec.rb
  12. 65
      spec/requests/auth/passwords_controller_spec.rb

16
app/controllers/auth/passwords_controller.rb

@ -24,12 +24,24 @@ class Auth::PasswordsController < Devise::PasswordsController
def edit
super
render "users/reset_password"
render "devise/passwords/reset_password"
end
protected
def resource_class_name
resource_class.name.underscore
end
def after_sending_reset_password_instructions_path_for(_resource)
confirmations_reset_path(email: params.dig("user", "email"))
confirmations_reset_path(email: params.dig(resource_class_name, "email"))
end
def after_resetting_password_path_for(resource)
if Devise.sign_in_after_reset_password
resource_class == AdminUser ? admin_user_two_factor_authentication_path : after_sign_in_path_for(resource)
else
new_session_path(resource_name)
end
end
end

20
app/controllers/auth/sessions_controller.rb

@ -3,12 +3,12 @@ class Auth::SessionsController < Devise::SessionsController
def create
self.resource = resource_class.new
if params.dig("user", "email").empty?
if params.dig(resource_class_name, "email").empty?
resource.errors.add :email, "Enter an email address"
elsif !email_valid?(params.dig("user", "email"))
elsif !email_valid?(params.dig(resource_class_name, "email"))
resource.errors.add :email, "Enter an email address in the correct format, like name@example.com"
end
if params.dig("user", "password").empty?
if params.dig(resource_class_name, "password").empty?
resource.errors.add :password, "Enter a password"
end
if resource.errors.present?
@ -20,7 +20,19 @@ class Auth::SessionsController < Devise::SessionsController
private
def resource_class
request.path.include?("admin") ? AdminUser : User
end
def resource_class_name
resource_class.name.underscore
end
def after_sign_in_path_for(resource)
params.dig("user", "start").present? ? case_logs_path : super
if resource_class == AdminUser
admin_user_two_factor_authentication_path
else
params.dig(resource_class_name, "start").present? ? case_logs_path : super
end
end
end

5
app/controllers/users_controller.rb

@ -12,7 +12,8 @@ class UsersController < ApplicationController
redirect_to user_path(@user)
elsif user_params.key?("password")
format_error_messages
render "devise/passwords/edit", status: :unprocessable_entity
@minimum_password_length = User.password_length.min
render "devise/passwords/edit", locals: { resource: @user, resource_name: "user" }, status: :unprocessable_entity
else
format_error_messages
render :edit, status: :unprocessable_entity
@ -41,7 +42,7 @@ class UsersController < ApplicationController
def edit_password
@minimum_password_length = User.password_length.min
render "devise/passwords/edit"
render "devise/passwords/edit", locals: { resource: @user, resource_name: "user" }
end
private

2
app/views/devise/passwords/edit.html.erb

@ -7,7 +7,7 @@
) %>
<% end %>
<%= form_for(@user, as: :user, html: { method: :patch }) do |f| %>
<%= form_for(resource, as: resource_name, html: { method: :patch }) do |f| %>
<div class="govuk-grid-row">
<div class="govuk-grid-column-two-thirds">
<%= f.govuk_error_summary(presenter: ErrorSummaryFullMessagesPresenter) %>

2
app/views/users/reset_password.html.erb → app/views/devise/passwords/reset_password.html.erb

@ -7,7 +7,7 @@
) %>
<% end %>
<%= form_for(@user, as: :user, url: password_path(User), html: { method: :put }) do |f| %>
<%= form_for(resource, as: resource_name, url: password_path(resource_name), html: { method: :put }) do |f| %>
<%= f.hidden_field :reset_password_token %>
<div class="govuk-grid-row">
<div class="govuk-grid-column-two-thirds">

8
app/views/devise/sessions/new.html.erb

@ -1,4 +1,10 @@
<% content_for :title, "Sign in to your account to submit CORE data" %>
<% if resource_name == :admin_user %>
<% title = "CORE Admin Sign in" %>
<% else %>
<% title = "Sign in to your account to submit CORE data" %>
<% end %>
<% content_for :title, title %>
<%= form_for(resource, as: resource_name, url: session_path(resource_name)) do |f| %>
<div class="govuk-grid-row">

6
config/routes.rb

@ -2,14 +2,14 @@ Rails.application.routes.draw do
devise_for :admin_users, {
path: :admin,
controllers: {
sessions: "active_admin/devise/sessions",
passwords: "active_admin/devise/passwords",
sessions: "auth/sessions",
passwords: "auth/passwords",
unlocks: "active_admin/devise/unlocks",
registrations: "active_admin/devise/registrations",
confirmations: "active_admin/devise/confirmations",
two_factor_authentication: "auth/two_factor_authentication",
},
path_names: { sign_in: "login", sign_out: "logout", two_factor_authentication: "two-factor-authentication" },
path_names: { sign_in: "sign-in", sign_out: "sign-out", two_factor_authentication: "two-factor-authentication" },
sign_out_via: %i[delete get],
}

72
spec/features/admin_panel_spec.rb

@ -11,6 +11,12 @@ RSpec.describe "Admin Panel" do
allow(notify_client).to receive(:send_sms).and_return(true)
end
it "shows the admin sign in page" do
visit("/admin")
expect(page).to have_current_path("/admin/sign-in")
expect(page).to have_content("CORE Admin Sign in")
end
context "with a valid 2FA code" do
before do
allow(SecureRandom).to receive(:random_number).and_return(otp)
@ -23,7 +29,7 @@ RSpec.describe "Admin Panel" do
expect(notify_client).to receive(:send_sms).with(
hash_including(phone_number: admin.phone, template_id: mfa_template_id),
)
click_button("Login")
click_button("Sign in")
fill_in("code", with: otp)
click_button("Submit")
expect(page).to have_content("Dashboard")
@ -32,7 +38,7 @@ RSpec.describe "Admin Panel" do
context "but it is more than 15 minutes old" do
it "does not authenticate successfully" do
click_button("Login")
click_button("Sign in")
admin.update!(direct_otp_sent_at: 16.minutes.ago)
fill_in("code", with: otp)
click_button("Submit")
@ -49,7 +55,7 @@ RSpec.describe "Admin Panel" do
visit("/admin")
fill_in("admin_user[email]", with: admin.email)
fill_in("admin_user[password]", with: admin.password)
click_button("Login")
click_button("Sign in")
fill_in("code", with: otp)
click_button("Submit")
expect(page).to have_content("Check your phone")
@ -64,7 +70,7 @@ RSpec.describe "Admin Panel" do
visit("/admin")
fill_in("admin_user[email]", with: admin.email)
fill_in("admin_user[password]", with: admin.password)
click_button("Login")
click_button("Sign in")
end
it "displays the resend view" do
@ -88,14 +94,68 @@ RSpec.describe "Admin Panel" do
visit("/admin")
fill_in("admin_user[email]", with: admin.email)
fill_in("admin_user[password]", with: admin.password)
click_button("Login")
click_button("Sign in")
fill_in("code", with: otp)
click_button("Submit")
click_link("Logout")
visit("/admin")
fill_in("admin_user[email]", with: admin.email)
fill_in("admin_user[password]", with: admin.password)
click_button("Login")
click_button("Sign in")
expect(page).to have_content("Check your phone")
end
end
context "when the admin has forgotten their password" do
let!(:admin_user) { FactoryBot.create(:admin_user, last_sign_in_at: Time.zone.now) }
let(:notify_client) { instance_double(Notifications::Client) }
let(:reset_password_token) { "MCDH5y6Km-U7CFPgAMVS" }
let(:devise_notify_mailer) { DeviseNotifyMailer.new }
before do
allow(DeviseNotifyMailer).to receive(:new).and_return(devise_notify_mailer)
allow(devise_notify_mailer).to receive(:notify_client).and_return(notify_client)
allow(notify_client).to receive(:send_email).and_return(true)
allow(Devise.token_generator).to receive(:generate).and_return(reset_password_token)
end
it " is redirected to the reset password page when they click the reset password link" do
visit("/admin")
click_link("reset your password")
expect(page).to have_current_path("/admin/password/new")
end
it " is shown an error message if they submit without entering an email address" do
visit("/admin/password/new")
click_button("Send email")
expect(page).to have_selector("#error-summary-title")
expect(page).to have_selector("#user-email-field-error")
expect(page).to have_title("Error")
end
it " is redirected to admin login page after reset email is sent" do
visit("/admin/password/new")
fill_in("admin_user[email]", with: admin_user.email)
click_button("Send email")
expect(page).to have_content("Check your email")
end
it " is sent a reset password email via Notify" do
expect(notify_client).to receive(:send_email).with(
{
email_address: admin_user.email,
template_id: admin_user.reset_password_notify_template,
personalisation: {
name: admin_user.email,
email: admin_user.email,
organisation: "",
link: "http://localhost:3000/admin/password/edit?reset_password_token=#{reset_password_token}",
},
},
)
visit("/admin/password/new")
fill_in("admin_user[email]", with: admin_user.email)
click_button("Send email")
end
end
end

55
spec/features/admin_user_spec.rb

@ -1,55 +0,0 @@
require "rails_helper"
RSpec.describe "Admin Features" do
let!(:admin_user) { FactoryBot.create(:admin_user, last_sign_in_at: Time.zone.now) }
let(:notify_client) { instance_double(Notifications::Client) }
let(:reset_password_token) { "MCDH5y6Km-U7CFPgAMVS" }
let(:devise_notify_mailer) { DeviseNotifyMailer.new }
before do
allow(DeviseNotifyMailer).to receive(:new).and_return(devise_notify_mailer)
allow(devise_notify_mailer).to receive(:notify_client).and_return(notify_client)
allow(notify_client).to receive(:send_email).and_return(true)
allow(Devise.token_generator).to receive(:generate).and_return(reset_password_token)
end
context "when the admin has forgotten their password" do
it " is redirected to the reset password page when they click the reset password link" do
visit("/admin")
click_link("Forgot your password?")
expect(page).to have_current_path("/admin/password/new")
end
it " is shown an error message if they submit without entering an email address" do
visit("/admin/password/new")
click_button("Reset My Password")
expect(page).to have_selector("#error_explanation")
expect(page).to have_content("can't be blank")
end
it " is redirected to admin login page after reset email is sent" do
visit("/admin/password/new")
fill_in("admin_user[email]", with: admin_user.email)
click_button("Reset My Password")
expect(page).to have_current_path("/admin/login")
end
it " is sent a reset password email via Notify" do
expect(notify_client).to receive(:send_email).with(
{
email_address: admin_user.email,
template_id: admin_user.reset_password_notify_template,
personalisation: {
name: admin_user.email,
email: admin_user.email,
organisation: "",
link: "http://localhost:3000/admin/password/edit?reset_password_token=#{reset_password_token}",
},
},
)
visit("/admin/password/new")
fill_in("admin_user[email]", with: admin_user.email)
click_button("Reset My Password")
end
end
end

1
spec/features/user_spec.rb

@ -18,6 +18,7 @@ RSpec.describe "User Features" do
it " is required to log in" do
visit("/logs")
expect(page).to have_current_path("/users/sign-in")
expect(page).to have_content("Sign in to your account to submit CORE data")
end
it "does not see the default devise error message" do

65
spec/requests/active_admin/devise/passwords_controller_spec.rb

@ -1,65 +0,0 @@
require "rails_helper"
RSpec.describe ActiveAdmin::Devise::PasswordsController, type: :request do
let(:admin_user) { FactoryBot.create(:admin_user) }
let(:headers) { { "Accept" => "text/html" } }
let(:page) { Capybara::Node::Simple.new(response.body) }
let(:new_value) { "new-password" }
let(:notify_client) { instance_double(Notifications::Client) }
let(:devise_notify_mailer) { DeviseNotifyMailer.new }
before do
allow(DeviseNotifyMailer).to receive(:new).and_return(devise_notify_mailer)
allow(devise_notify_mailer).to receive(:notify_client).and_return(notify_client)
allow(notify_client).to receive(:send_email).and_return(true)
end
describe "reset password" do
it "renders the user edit password view" do
_raw, enc = Devise.token_generator.generate(AdminUser, :reset_password_token)
get "/admin/password/edit?reset_password_token=#{enc}"
expect(page).to have_css("h2", text: "DLUHC CORE Change your password")
end
context "when passwords entered don't match" do
let(:raw) { admin_user.send_reset_password_instructions }
let(:params) do
{
id: admin_user.id,
admin_user: {
password: new_value,
password_confirmation: "something_else",
reset_password_token: raw,
},
}
end
it "shows an error" do
put "/admin/password", headers: headers, params: params
expect(response).to have_http_status(:unprocessable_entity)
expect(page).to have_content("doesn't match Password")
end
end
context "when passwords is reset" do
let(:raw) { admin_user.send_reset_password_instructions }
let(:params) do
{
id: admin_user.id,
admin_user: {
password: new_value,
password_confirmation: new_value,
reset_password_token: raw,
},
}
end
it "updates the password" do
expect {
put "/admin/password", headers: headers, params: params
admin_user.reload
}.to change(admin_user, :encrypted_password)
end
end
end
end

65
spec/requests/auth/passwords_controller_spec.rb

@ -2,7 +2,6 @@ require "rails_helper"
require_relative "../../support/devise"
RSpec.describe Auth::PasswordsController, type: :request do
let(:params) { { user: { email: } } }
let(:page) { Capybara::Node::Simple.new(response.body) }
let(:notify_client) { instance_double(Notifications::Client) }
let(:devise_notify_mailer) { DeviseNotifyMailer.new }
@ -13,6 +12,9 @@ RSpec.describe Auth::PasswordsController, type: :request do
allow(notify_client).to receive(:send_email).and_return(true)
end
context "when a regular user" do
let(:params) { { user: { email: } } }
context "when a password reset is requested for a valid email" do
let(:user) { FactoryBot.create(:user) }
let(:email) { user.email }
@ -70,3 +72,64 @@ RSpec.describe Auth::PasswordsController, type: :request do
end
end
end
context "when an admin user" do
let(:admin_user) { FactoryBot.create(:admin_user) }
describe "reset password" do
let(:new_value) { "new-password" }
it "renders the user edit password view" do
_raw, enc = Devise.token_generator.generate(AdminUser, :reset_password_token)
get "/admin/password/edit?reset_password_token=#{enc}"
expect(page).to have_css("h1", text: "Reset your password")
end
context "when passwords entered don't match" do
let(:raw) { admin_user.send_reset_password_instructions }
let(:params) do
{
id: admin_user.id,
admin_user: {
password: new_value,
password_confirmation: "something_else",
reset_password_token: raw,
},
}
end
it "shows an error" do
put "/admin/password", headers: headers, params: params
expect(response).to have_http_status(:unprocessable_entity)
expect(page).to have_content("doesn't match Password")
end
end
context "when passwords is reset" do
let(:raw) { admin_user.send_reset_password_instructions }
let(:params) do
{
id: admin_user.id,
admin_user: {
password: new_value,
password_confirmation: new_value,
reset_password_token: raw,
},
}
end
it "updates the password" do
expect {
put "/admin/password", headers: headers, params: params
admin_user.reload
}.to change(admin_user, :encrypted_password)
end
it "sends you to the 2FA page" do
put "/admin/password", headers: headers, params: params
expect(response).to redirect_to("/admin/two-factor-authentication")
end
end
end
end
end

Loading…
Cancel
Save