From 2eba64ec472cf58e09fd153f8d3cb19c5c458731 Mon Sep 17 00:00:00 2001 From: Steve Urciuoli Date: Wed, 13 Jun 2018 22:53:09 -0400 Subject: [PATCH 01/25] LG-352 Create a new CloudHSM key sharing script to grant IDP access to generated keys **Why**: To separate the key creator from the IDP user **How**: Create the CloudhsmKeySharer script to interface to cloudhsm_mgmt_util using the credentials and key handle output by the CloudhsmKeyGenerator. Use the greenletters gem to automate interaction with the commandline utility and obscure the password. --- .reek | 2 + .rubocop.yml | 1 + lib/cloudhsm/cloudhsm_key_generator.rb | 104 ++++++++++++--- lib/cloudhsm/cloudhsm_key_sharer.rb | 123 ++++++++++++++++++ lib/tasks/cloudhsm.rake | 7 +- .../cloudhsm/cloudhsm_key_generator_spec.rb | 86 ++++++++---- spec/lib/cloudhsm/cloudhsm_key_sharer_spec.rb | 61 +++++++++ 7 files changed, 338 insertions(+), 46 deletions(-) create mode 100644 lib/cloudhsm/cloudhsm_key_sharer.rb create mode 100644 spec/lib/cloudhsm/cloudhsm_key_sharer_spec.rb diff --git a/.reek b/.reek index f6ea86bbe25..9c975cc9f6c 100644 --- a/.reek +++ b/.reek @@ -89,6 +89,7 @@ TooManyInstanceVariables: - OpenidConnectRedirector - Idv::VendorResult - CloudhsmKeyGenerator + - CloudhsmKeySharer TooManyStatements: max_statements: 6 exclude: @@ -122,6 +123,7 @@ TooManyMethods: - SessionDecorator - HolidayService - PhoneDeliveryPresenter + - CloudhsmKeyGenerator UncommunicativeMethodName: exclude: - PhoneConfirmationFlow diff --git a/.rubocop.yml b/.rubocop.yml index 9af4903ae03..00517fc0e97 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -68,6 +68,7 @@ Metrics/ClassLength: - app/services/analytics.rb - app/services/idv/session.rb - app/presenters/two_factor_auth_code/phone_delivery_presenter.rb + - lib/cloudhsm/cloudhsm_key_generator.rb Metrics/LineLength: Description: Limit lines to 100 characters. diff --git a/lib/cloudhsm/cloudhsm_key_generator.rb b/lib/cloudhsm/cloudhsm_key_generator.rb index 6f82cda8847..6040f93c2fc 100644 --- a/lib/cloudhsm/cloudhsm_key_generator.rb +++ b/lib/cloudhsm/cloudhsm_key_generator.rb @@ -1,38 +1,45 @@ require 'greenletters' require 'io/console' -# Can be run standalone (without idp or rails) or through the rake task cloudhsm -# generates saml_.key, saml_.crt, +# In prod this script is run through the cloudhsm rake task which will run on a RamDisk +# generates saml_.key, saml_.crt +# saml_.scr (cached credentials and key handle for the key_sharer script) # and saml_.txt (a transcript of the cloudhsm interaction) -# the program interactively asks for username, password, and openssl.conf location +# the program interactively asks for username, password (hidden), idp username, +# and openssl.conf location class CloudhsmKeyGenerator KEY_MGMT_UTIL = '/opt/cloudhsm/bin/key_mgmt_util'.freeze def initialize @username, @password, @openssl_conf, @timestamp = initialize_settings - output = File.open("saml_#{@timestamp}.txt", 'w') - @kmu = Greenletters::Process.new(KEY_MGMT_UTIL, transcript: output) - @kmu.start! - @kmu.wait_for(:output, /Command:/) + @idp_username = prompt_for_idp_username + @kmu = run_key_mgmt_util + wait_for_command_to_finish end def generate_saml_key saml_label = create_key_and_crt_files login_to_hsm - wrapping_key_handle = generate_symmetric_wrapping_key - import_private_key(saml_label, wrapping_key_handle) + key_handle = import_private_key(saml_label) exit_hsm - saml_label + cache_credentials_and_private_key_handle(key_handle) + [saml_label, key_handle] end - def cleanup - File.delete("saml_#{@timestamp}.crt") - File.delete("saml_#{@timestamp}.txt") - File.delete("saml_#{@timestamp}.key") + private + + def import_private_key(saml_label) + wrapping_key_handle = generate_symmetric_wrapping_key + import_wrapped_key(saml_label, wrapping_key_handle) end - private + def run_key_mgmt_util + output = File.open("saml_#{@timestamp}.txt", 'w') + kmu = Greenletters::Process.new(KEY_MGMT_UTIL, transcript: output) + kmu.start! + kmu + end def initialize_settings username, password = prompt_for_username_and_password @@ -56,13 +63,13 @@ def create_key_and_crt_files def login_to_hsm @kmu << "loginHSM -u CU -s #{@username} -p #{@password}\n" @kmu.wait_for(:output, /SUCCESS/) + wait_for_command_to_finish end def generate_symmetric_wrapping_key @kmu << "genSymKey -t 31 -s 16 -sess -l wrapping_key_for_import\n" - wrapping_key_handle = wait_for_wrapping_key_handle @kmu.wait_for(:output, /SUCCESS/) - wrapping_key_handle + wait_for_wrapping_key_handle end def wait_for_wrapping_key_handle @@ -71,16 +78,28 @@ def wait_for_wrapping_key_handle matching.matched =~ /Key Handle: (\d+)/ wrapping_key_handle = Regexp.last_match[1] end + wait_for_command_to_finish wrapping_key_handle end - def import_private_key(saml_label, wrapping_key_handle) + def import_wrapped_key(saml_label, wrapping_key_handle) @kmu << "importPrivateKey -f #{saml_label}.key -l #{saml_label} -w #{wrapping_key_handle}\n" @kmu.wait_for(:output, /SUCCESS/) + wait_for_private_key_handle + end + + def wait_for_private_key_handle + key_handle = nil + @kmu.wait_for(:output, /Key Handle: \d+/) do |_process, matching| + matching.matched =~ /Key Handle: (\d+)/ + key_handle = Regexp.last_match[1] + end + wait_for_command_to_finish + key_handle end def exit_hsm - @kmu << 'exit' + @kmu << "exit\n" Kernel.puts "Certificate written to 'saml_#{@timestamp}.crt'.\n" \ "Transcript written to 'saml_#{@timestamp}.txt'.\n" \ "Key generation complete.\n" @@ -94,16 +113,61 @@ def prompt_for_username_and_password [username, password] end + def prompt_for_idp_username + Kernel.puts 'Please enter the CloudHSM username used by the IDP' + user_input + end + def prompt_for_openssl_conf Kernel.puts 'Please enter the full path to openssl.conf' openssl_conf = user_input - raise 'openssl.conf not found' unless File.exist?(openssl_conf) + raise "openssl.conf not found at (#{openssl_conf})" unless File.exist?(openssl_conf) openssl_conf end def user_input gets.chomp end + + def wait_for_command_to_finish + @kmu.wait_for(:output, /Command:/) + end + + def cache_credentials_and_private_key_handle(key_handle) + File.open("saml_#{@timestamp}.scr", 'w') do |file| + file.puts("#{@username}:#{@password}:#{key_handle}:#{@idp_username}") + end + end end CloudhsmKeyGenerator.new.generate_saml_key if $PROGRAM_NAME == __FILE__ + +# Command: loginHSM -u CU -s username -p password + +# Cfm3LoginHSM returned: 0x00 : HSM Return: SUCCESS + +# Cluster Error Status +# Node id 1 and err state 0x00000000 : HSM Return: SUCCESS + +# Command: genSymKey -t 31 -s 16 -sess -l wrapping_key_for_import + +# Cfm3GenerateSymmetricKey returned: 0x00 : HSM Return: SUCCESS + +# Symmetric Key Created. Key Handle: 1234 + +# Cluster Error Status +# Node id 1 and err state 0x00000000 : HSM Return: SUCCESS + +# Command: importPrivateKey -f saml_20180721214537.key -l saml_20180721214537 -w 1234 +# BER encoded key length is 1218 + +# Cfm3WrapHostKey returned: 0x00 : HSM Return: SUCCESS + +# Cfm3CreateUnwrapTemplate returned: 0x00 : HSM Return: SUCCESS + +# Cfm3UnWrapKey returned: 0x00 : HSM Return: SUCCESS + +# Private Key Imported. Key Handle: 5678 + +# Cluster Error Status +# Node id 1 and err state 0x00000000 : HSM Return: SUCCESS diff --git a/lib/cloudhsm/cloudhsm_key_sharer.rb b/lib/cloudhsm/cloudhsm_key_sharer.rb new file mode 100644 index 00000000000..7e412caeb41 --- /dev/null +++ b/lib/cloudhsm/cloudhsm_key_sharer.rb @@ -0,0 +1,123 @@ +require 'greenletters' + +# In prod this script is run through the cloudhsm rake task which will run on a RamDisk +# Grants access to the key generated by cloudhsm_key_generator to the idp user +# Uses the saml key label and cached output from the generator (credentials/key handle) +# Outputs to file saml_.shr (a transcript of the cloudhsm interaction) + +class CloudhsmKeySharer + CLOUDHSM_MGMT_UTIL = \ + '/opt/cloudhsm/bin/cloudhsm_mgmt_util /opt/cloudhsm/etc/cloudhsm_mgmt_util.cfg'.freeze + + def initialize(saml_label) + @saml_label = saml_label + @username, @password, @key_handle, @idp_username = read_credentials_and_private_key_handle + @kmu = run_cloudhsm_mgmt_util + wait_for_command_to_finish + enable_encrypted + end + + def share_saml_key + login_to_hsm + idp_user_id = idp_user_id_from_list_users + raise "User #{@idp_username} not found" unless idp_user_id + share_key(@key_handle, idp_user_id) + exit_hsm + end + + private + + def read_credentials_and_private_key_handle + File.read("#{@saml_label}.scr").to_s.chomp.split(':') + end + + def run_cloudhsm_mgmt_util + output = File.open("#{@saml_label}.shr", 'w') + kmu = Greenletters::Process.new(CLOUDHSM_MGMT_UTIL, transcript: output) + kmu.start! + kmu + end + + def enable_encrypted + @kmu << "enable_e2e\n" + @kmu.wait_for(:output, /E2E enabled/) + wait_for_command_to_finish + end + + def idp_user_id_from_list_users + @kmu << "listUsers\n" + user_id = nil + @kmu.wait_for(:output, /\d+\s+CU\s+#{@idp_username}/) do |_process, matching| + user_id = matching.matched.split[0] + end + wait_for_command_to_finish + user_id + end + + def share_key(key_handle, user_id) + @kmu << "shareKey #{key_handle} #{user_id} 1\n" + @kmu.wait_for(:output, %r{Do you want to continue\(y/n\)\?}) + @kmu << "y\n" + @kmu.wait_for(:output, /success on server/) + wait_for_command_to_finish + end + + def login_to_hsm + @kmu << "loginHSM CU #{@username} #{@password}\n" + @kmu.wait_for(:output, /loginHSM success/) + wait_for_command_to_finish + end + + def exit_hsm + @kmu << "quit\n" + Kernel.puts "Key shared with the idp user successfully\n" \ + "Transcript written to '#{@saml_label}.shr'.\n" + end + + def wait_for_command_to_finish + @kmu.wait_for(:output, /aws-cloudhsm>/) + end +end + +CloudhsmKeySharer.new(ARGV[0]).share_saml_key if $PROGRAM_NAME == __FILE__ + +# cat saml_20180721031604.scr +# gen_user:password:1234:idp_user + +# /opt/cloudhsm/bin/cloudhsm_mgmt_util /opt/cloudhsm/etc/cloudhsm_mgmt_util.cfg + +# Connecting to the server(s), it may take time +# depending on the server(s) load, please wait... +# Connecting to server '127.0.0.1': hostname '127.0.0.1', port 2225... +# Connected to server '127.0.0.1': hostname '127.0.0.1', port 2225. + +# aws-cloudhsm>enable_e2e +# Server 0(127.0.0.1) is in e2e mode now... + +# aws-cloudhsm>loginHSM CU idp_generator password +# loginHSM success on server 0(127.0.0.1) + +# aws-cloudhsm>listUsers +# Users on server 0(127.0.0.1): +# Number of users found:5 + +# User Id User Type User Name MofnPubKey LoginFailureCnt 2FA +# 1 CO admin NO 0 NO +# 2 AU app_user NO 0 NO +# 3 AU jane_doe NO 0 NO +# 4 CU idp_user NO 0 NO +# 5 CU gen_user NO 0 NO + +# aws-cloudhsm>shareKey 1234 4 1 +# *************************CAUTION******************************** +# This is a CRITICAL operation, should be done on all nodes in the +# cluster. Cav server does NOT synchronize these changes with the +# nodes on which this operation is not executed or failed, please +# ensure this operation is executed on all nodes in the cluster. +# **************************************************************** + +# Do you want to continue(y/n)?y +# shareKey success on server 0(127.0.0.1) + +# aws-cloudhsm>quit +# disconnecting from servers, please wait... diff --git a/lib/tasks/cloudhsm.rake b/lib/tasks/cloudhsm.rake index e1efeb6cacd..78204b2c1c8 100644 --- a/lib/tasks/cloudhsm.rake +++ b/lib/tasks/cloudhsm.rake @@ -1,6 +1,9 @@ +# In prod this script will run on a RamDisk + namespace :cloudhsm do - desc 'Generate a new saml key' + desc 'Generate a new saml key and grant idp access to the key' task generate_saml_key: :environment do - CloudhsmKeyGenerator.new.generate_saml_key + saml_key_label, _handle = CloudhsmKeyGenerator.new.generate_saml_key + CloudhsmKeySharer.new(saml_key_label).share_saml_key end end diff --git a/spec/lib/cloudhsm/cloudhsm_key_generator_spec.rb b/spec/lib/cloudhsm/cloudhsm_key_generator_spec.rb index f263932478d..728a04fcf29 100644 --- a/spec/lib/cloudhsm/cloudhsm_key_generator_spec.rb +++ b/spec/lib/cloudhsm/cloudhsm_key_generator_spec.rb @@ -3,61 +3,99 @@ describe CloudhsmKeyGenerator do let(:subject) { CloudhsmKeyGenerator.new } + let(:wrapping_key) { '1234' } + let(:key_owner_name) { 'username' } + let(:key_owner_password) { 'password' } + let(:share_with_username) { 'username2' } + let(:key_handle) { '5678' } + before(:each) { mock_cloudhsm } describe '#generate_saml_key' do it 'generates saml secret key, crt, and transcript' do - label = subject.generate_saml_key + greenletters = mock_cloudhsm + label, handle = subject.generate_saml_key saml_key = "#{label}.key" saml_crt = "#{label}.crt" transcript = "#{label}.txt" + secrets_fn = "#{label}.scr" + + expect(greenletters).to have_received(:<<). + with("loginHSM -u CU -s #{key_owner_name} -p #{key_owner_password}\n").once + expect(greenletters).to have_received(:<<). + with("genSymKey -t 31 -s 16 -sess -l wrapping_key_for_import\n").once + expect(greenletters).to have_received(:<<). + with("importPrivateKey -f #{label}.key -l #{label} -w #{wrapping_key}\n").once + expect(greenletters).to have_received(:<<).with("exit\n").once + expect(handle).to eq(key_handle) expect(File.exist?(saml_key)).to eq(true) expect(File.exist?(saml_crt)).to eq(true) expect(File.exist?(transcript)).to eq(true) - subject.cleanup + expect(File.read(secrets_fn)). + to eq("#{key_owner_name}:#{key_owner_password}:#{key_handle}:#{share_with_username}\n") + cleanup(label) end it 'raises an error if the openssl call fails' do allow_any_instance_of(CloudhsmKeyGenerator).to receive(:user_input).and_return(__FILE__) - expect { subject.generate_saml_key }.to raise_error(RuntimeError, 'Call to openssl failed') + + label = nil + expect do + label, _handle = subject.generate_saml_key + end.to raise_error(RuntimeError, 'Call to openssl failed') + + cleanup(label) end it 'raises an error if the openssl.conf is not found' do - allow_any_instance_of(CloudhsmKeyGenerator).to receive(:user_input).and_return('filenotfound') + allow_any_instance_of(CloudhsmKeyGenerator).to receive(:user_input). + and_return(key_owner_name, 'bad_filename', share_with_username) + + label = nil + expect { label, _handle = subject.generate_saml_key }. + to raise_error(RuntimeError, 'openssl.conf not found at (bad_filename)') - expect { subject.generate_saml_key }.to raise_error(RuntimeError, 'openssl.conf not found') + cleanup(label) end it 'creates a saml key label with a 12 digit timestamp' do - label = subject.generate_saml_key + label, _handle = subject.generate_saml_key expect(label =~ /\d{1,12}/).to_not be_nil - subject.cleanup + cleanup(label) end end - describe '#cleanup' do - it 'removes all the files if we request cleanup' do - label = subject.generate_saml_key - subject.cleanup + def mock_cloudhsm + allow(STDIN).to receive(:noecho).and_return(key_owner_password) - saml_key = "#{label}.key" - saml_crt = "#{label}.crt" - transcript = "#{label}.txt" - expect(File.exist?(saml_key)).to eq(false) - expect(File.exist?(saml_crt)).to eq(false) - expect(File.exist?(transcript)).to eq(false) - end - end + greenletters = instance_double(Greenletters::Process) + allow(Greenletters::Process).to receive(:new).and_return(greenletters) + allow(greenletters).to receive(:start!).and_return(true) + allow(greenletters).to receive(:wait_for).and_return(true) + allow(greenletters).to receive(:<<).and_return(true) + allow(greenletters).to receive(:wait_for).with(:output, /Key Handle: \d+/). + and_yield(nil, StringScanner.new('')) + allow_any_instance_of(StringScanner).to receive(:matched). + and_return("Key Handle: #{wrapping_key}\n", "Key Handle: #{key_handle}\n") - def mock_cloudhsm - allow(STDIN).to receive(:noecho).and_return('password') - allow_any_instance_of(Greenletters::Process).to receive(:wait_for).and_return(true) - allow_any_instance_of(Greenletters::Process).to receive(:<<).and_return(true) allow_any_instance_of(CloudhsmKeyGenerator).to receive(:user_input). - and_return('config/openssl.conf') + and_return(key_owner_name, 'config/openssl.conf', share_with_username) + + greenletters + end + + def cleanup(label) + safe_delete("#{label}.crt") + safe_delete("#{label}.txt") + safe_delete("#{label}.key") + safe_delete("#{label}.scr") + end + + def safe_delete(fn) + File.delete(fn) if File.exist?(fn) end end diff --git a/spec/lib/cloudhsm/cloudhsm_key_sharer_spec.rb b/spec/lib/cloudhsm/cloudhsm_key_sharer_spec.rb new file mode 100644 index 00000000000..a066873cf5f --- /dev/null +++ b/spec/lib/cloudhsm/cloudhsm_key_sharer_spec.rb @@ -0,0 +1,61 @@ +require 'rails_helper' +require 'cloudhsm/cloudhsm_key_sharer' + +describe CloudhsmKeySharer do + let(:saml_label) { 'saml_20180614001957' } + let(:subject) { CloudhsmKeySharer.new(saml_label) } + let(:key_handle) { '1234' } + let(:key_owner_name) { 'username' } + let(:key_owner_password) { 'password' } + let(:shared_with_username) { 'username2' } + let(:shared_with_user_id) { '4' } + let(:transcript_fn) { "#{saml_label}.shr" } + + describe '#share_saml_key' do + it 'shares saml key and generates transcript' do + greenletters = mock_cloudhsm + + subject.share_saml_key + + expect(greenletters).to have_received(:<<).with("enable_e2e\n").once + expect(greenletters).to have_received(:<<). + with("loginHSM CU #{key_owner_name} #{key_owner_password}\n").once + expect(greenletters).to have_received(:<<).with("listUsers\n").once + expect(greenletters).to have_received(:<<). + with("shareKey #{key_handle} #{shared_with_user_id} 1\n").once + expect(greenletters).to have_received(:<<).with("y\n").once + expect(greenletters).to have_received(:<<).with("quit\n").once + expect(File.exist?(transcript_fn)).to eq(true) + + File.delete(transcript_fn) + end + + it 'raises an error if the user is not found in listUsers' do + greenletters = mock_cloudhsm + allow(greenletters).to receive(:wait_for).with(:output, /\d+\s+CU\s+#{shared_with_username}/). + and_return(false) + + expect { subject.share_saml_key }. + to raise_error(RuntimeError, "User #{shared_with_username} not found") + end + end + + def mock_cloudhsm + greenletters = instance_double(Greenletters::Process) + allow(Greenletters::Process).to receive(:new).and_return(greenletters) + + allow(greenletters).to receive(:<<).and_return(true) + allow(greenletters).to receive(:start!).and_return(true) + allow(greenletters).to receive(:wait_for).and_return(true) + allow(greenletters).to receive(:wait_for).with(:output, /\d+\s+CU\s+#{shared_with_username}/). + and_yield(nil, StringScanner.new('')) + + allow_any_instance_of(StringScanner).to receive(:matched). + and_return("#{shared_with_user_id} CU #{shared_with_username} NO 0 NO\n") + + allow(File).to receive(:read).with("#{saml_label}.scr"). + and_return("#{key_owner_name}:#{key_owner_password}:#{key_handle}:#{shared_with_username}") + + greenletters + end +end From 5c9082258aa0d568da2c4f54e9d4339461c4b047 Mon Sep 17 00:00:00 2001 From: Steve Urciuoli Date: Thu, 26 Jul 2018 22:21:56 -0400 Subject: [PATCH 02/25] LG-504 Create a health checker for account reset notifications **Why**: When a user requests an account reset we tell them their request will be processed in 24 hours. We have to be sure that the asyncronous processing of the grant notifications are occurring on a timely basis. **How**: Add a health checker to the health_controller. Search for a single record in account_reset_requests where the request was not serviced in 26 hours (24 hours + a 2 hour buffer to service the requests). Return a bad health status if such a record is found. The table has an index that is optimized for this type of query while factoring in timestamps, cancellations, and requests already granted. --- app/controllers/health/health_controller.rb | 1 + app/services/account_reset_health_checker.rb | 35 +++++++++++ .../health/health_controller_spec.rb | 1 + .../account_reset_health_checker_spec.rb | 58 +++++++++++++++++++ 4 files changed, 95 insertions(+) create mode 100644 app/services/account_reset_health_checker.rb create mode 100644 spec/services/account_reset_health_checker_spec.rb diff --git a/app/controllers/health/health_controller.rb b/app/controllers/health/health_controller.rb index 0fbcb5eabbd..d9fd4f5fec4 100644 --- a/app/controllers/health/health_controller.rb +++ b/app/controllers/health/health_controller.rb @@ -6,6 +6,7 @@ def health_checker checkers = { database: DatabaseHealthChecker, workers: WorkerHealthChecker, + account_reset: AccountResetHealthChecker, } # Don't run worker health checks if we're not using workers (i.e. if the # queue adapter is inline or async) diff --git a/app/services/account_reset_health_checker.rb b/app/services/account_reset_health_checker.rb new file mode 100644 index 00000000000..4a19670e392 --- /dev/null +++ b/app/services/account_reset_health_checker.rb @@ -0,0 +1,35 @@ +module AccountResetHealthChecker + module_function + + Summary = Struct.new(:healthy, :result) do + def as_json(*args) + to_h.as_json(*args) + end + + alias_method :healthy?, :healthy + end + + # @return [Summary] + def check + rec = find_request_not_serviced_within_26_hours + Summary.new(rec.nil?, rec) + end + + # @api private + def find_request_not_serviced_within_26_hours + records = AccountResetRequest.where( + sql, tvalue: Time.zone.now - Figaro.env.account_reset_wait_period_days.to_i.days - 2.hours + ).order('requested_at ASC').limit(1) + records.first + end + + def sql + <<~SQL + cancelled_at IS NULL AND + granted_at IS NULL AND + requested_at < :tvalue AND + request_token IS NOT NULL AND + granted_token IS NULL + SQL + end +end diff --git a/spec/controllers/health/health_controller_spec.rb b/spec/controllers/health/health_controller_spec.rb index 06b3e54de3d..9fa15add5d9 100644 --- a/spec/controllers/health/health_controller_spec.rb +++ b/spec/controllers/health/health_controller_spec.rb @@ -26,6 +26,7 @@ expect(json[:healthy]).to eq(true) expect(json[:statuses][:workers][:all_healthy]).to eq(true) expect(json[:statuses][:database][:healthy]).to eq(true) + expect(json[:statuses][:account_reset][:healthy]).to eq(true) end end diff --git a/spec/services/account_reset_health_checker_spec.rb b/spec/services/account_reset_health_checker_spec.rb new file mode 100644 index 00000000000..47a4de67999 --- /dev/null +++ b/spec/services/account_reset_health_checker_spec.rb @@ -0,0 +1,58 @@ +require 'rails_helper' + +RSpec.describe AccountResetHealthChecker do + let(:wait_period) { Figaro.env.account_reset_wait_period_days.to_i.days } + let(:buffer_period) { 2.hours } # window buffer to allow servicing requests after 24 hours + describe '.check' do + subject(:summary) { AccountResetHealthChecker.check } + + context 'when there are no requests' do + it 'returns a healthy check' do + expect(summary.result).to eq(nil) + expect(summary.healthy).to eq(true) + end + end + + context 'when a request is not serviced on time' do + before do + AccountResetRequest.create(id: 1, + user_id: 2, + requested_at: Time.zone.now - wait_period - buffer_period, + request_token: 'foo') + end + + it 'returns an unhealthy check' do + expect(summary.healthy).to eq(false) + end + end + + context 'when an old request was cancelled' do + before do + AccountResetRequest.create(id: 1, + user_id: 2, + requested_at: Time.zone.now - wait_period - buffer_period, + request_token: 'foo', + cancelled_at: Time.zone.now) + end + + it 'returns an unhealthy check' do + expect(summary.healthy).to eq(true) + end + end + + context 'when all requests are serviced on time' do + before do + AccountResetRequest.create(id: 1, + user_id: 2, + requested_at: Time.zone.now - wait_period - buffer_period, + request_token: 'foo', + granted_at: Time.zone.now - buffer_period, + granted_token: 'bar') + end + + it 'returns a healthy check' do + expect(summary.healthy).to eq(true) + end + end + end +end From 640da71612edc60cfa650ae7b05f83255e7c32f0 Mon Sep 17 00:00:00 2001 From: Steve Urciuoli Date: Sat, 28 Jul 2018 17:48:08 -0400 Subject: [PATCH 03/25] LG-519 Fix: session timeout prevents return to SP and loses SP branding **Why**: Users can't return to their service provider. They get stuck at the login.gov account page and don't know what to do. **How**: After timeout put the request_id back on the url. Fix the broken feature spec / test. --- app/controllers/users/sessions_controller.rb | 3 ++- spec/features/saml/loa1_sso_spec.rb | 6 ++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/app/controllers/users/sessions_controller.rb b/app/controllers/users/sessions_controller.rb index 3372aae0914..11e7019ed04 100644 --- a/app/controllers/users/sessions_controller.rb +++ b/app/controllers/users/sessions_controller.rb @@ -38,13 +38,14 @@ def active def timeout analytics.track_event(Analytics::SESSION_TIMED_OUT) + request_id = sp_session[:request_id] sign_out flash[:notice] = t( 'session_timedout', app: APP_NAME, minutes: Figaro.env.session_timeout_in_minutes ) - redirect_to root_url + redirect_to root_url(request_id: request_id) end private diff --git a/spec/features/saml/loa1_sso_spec.rb b/spec/features/saml/loa1_sso_spec.rb index 4e0b80685e7..e700001bc12 100644 --- a/spec/features/saml/loa1_sso_spec.rb +++ b/spec/features/saml/loa1_sso_spec.rb @@ -119,8 +119,10 @@ visit saml_authn_request sp_request_id = ServiceProviderRequest.last.uuid - page.set_rack_session(sp: {}) - visit new_user_session_url(request_id: sp_request_id) + + visit timeout_path + expect(current_url).to eq root_url(request_id: sp_request_id) + fill_in_credentials_and_submit(user.email, user.password) click_submit_default click_continue From cc87649f0f64979cb474f573f47b5b7bb71a90af Mon Sep 17 00:00:00 2001 From: Steve Urciuoli Date: Sat, 28 Jul 2018 13:58:58 -0400 Subject: [PATCH 04/25] LG-512 Add a failure to proof url to service providers for LOA3 **Why**: So the SP can offer a custom page when a user fails to proof on LOA3. **How**: Add a new column to service providers. Update any supporting classes. --- app/decorators/identity_decorator.rb | 4 ++ app/models/null_service_provider.rb | 2 + config/service_providers.yml | 1 + ...ailure_to_proof_url_to_service_provider.rb | 9 ++++ db/schema.rb | 3 +- spec/decorators/identity_decorator_spec.rb | 45 ++++++++++++++----- spec/models/null_service_provider_spec.rb | 6 +++ 7 files changed, 58 insertions(+), 12 deletions(-) create mode 100644 db/migrate/20180728122856_add_failure_to_proof_url_to_service_provider.rb diff --git a/app/decorators/identity_decorator.rb b/app/decorators/identity_decorator.rb index f652ea53921..63c65cc6eeb 100644 --- a/app/decorators/identity_decorator.rb +++ b/app/decorators/identity_decorator.rb @@ -10,6 +10,10 @@ def event_partial 'accounts/identity_item' end + def failure_to_proof_url + identity.sp_metadata[:failure_to_proof_url] + end + def return_to_sp_url identity.sp_metadata[:return_to_sp_url] end diff --git a/app/models/null_service_provider.rb b/app/models/null_service_provider.rb index 2998c226224..39e0f7a4adf 100644 --- a/app/models/null_service_provider.rb +++ b/app/models/null_service_provider.rb @@ -29,6 +29,8 @@ def logo; end def friendly_name; end + def failure_to_proof_url; end + def return_to_sp_url; end def redirect_uris diff --git a/config/service_providers.yml b/config/service_providers.yml index 451b8f3a9c2..627f1e81566 100644 --- a/config/service_providers.yml +++ b/config/service_providers.yml @@ -23,6 +23,7 @@ test: assertion_consumer_logout_service_url: 'http://example.com/test/saml/decode_slo_request' block_encryption: 'aes256-cbc' sp_initiated_login_url: 'https://example.com/auth/saml/login' + failure_to_proof_url: 'https://example.com/' friendly_name: 'Test SP' cert: 'saml_test_sp' logo: 'generic.svg' diff --git a/db/migrate/20180728122856_add_failure_to_proof_url_to_service_provider.rb b/db/migrate/20180728122856_add_failure_to_proof_url_to_service_provider.rb new file mode 100644 index 00000000000..70b471ec641 --- /dev/null +++ b/db/migrate/20180728122856_add_failure_to_proof_url_to_service_provider.rb @@ -0,0 +1,9 @@ +class AddFailureToProofUrlToServiceProvider < ActiveRecord::Migration[5.1] + def up + add_column :service_providers, :failure_to_proof_url, :text + end + + def down + remove_column :service_providers, :failure_to_proof_url + end +end diff --git a/db/schema.rb b/db/schema.rb index 0500a0e1cf0..bf2f5a85943 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20180720152009) do +ActiveRecord::Schema.define(version: 20180728122856) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -179,6 +179,7 @@ t.boolean "native", default: false, null: false t.string "redirect_uris", default: [], array: true t.integer "agency_id" + t.text "failure_to_proof_url" t.index ["issuer"], name: "index_service_providers_on_issuer", unique: true end diff --git a/spec/decorators/identity_decorator_spec.rb b/spec/decorators/identity_decorator_spec.rb index b075c175b16..55032322260 100644 --- a/spec/decorators/identity_decorator_spec.rb +++ b/spec/decorators/identity_decorator_spec.rb @@ -3,21 +3,21 @@ describe IdentityDecorator do include ActionView::Helpers::TagHelper - let(:user) { create(:user) } - let(:service_provider) { 'http://localhost:3000' } - let(:identity) { create(:identity, :active, user: user, service_provider: service_provider) } + describe '#return_to_sp_url' do + let(:user) { create(:user) } + let(:service_provider) { 'http://localhost:3000' } + let(:identity) { create(:identity, :active, user: user, service_provider: service_provider) } - subject { IdentityDecorator.new(identity) } + subject { IdentityDecorator.new(identity) } - describe '#return_to_sp_url' do - context 'for an sp without a return URL' do - context 'for an sp with a return URL' do - it 'returns the return url for the sp' do - return_to_sp_url = ServiceProvider.from_issuer(service_provider).return_to_sp_url - expect(subject.return_to_sp_url).to eq(return_to_sp_url) - end + context 'for an sp with a return URL' do + it 'returns the return url for the sp' do + return_to_sp_url = ServiceProvider.from_issuer(service_provider).return_to_sp_url + expect(subject.return_to_sp_url).to eq(return_to_sp_url) end + end + context 'for an sp without a return URL' do let(:service_provider) { 'https://rp2.serviceprovider.com/auth/saml/metadata' } it 'returns nil' do @@ -25,4 +25,27 @@ end end end + + describe '#failure_to_proof_url' do + let(:user) { create(:user) } + let(:service_provider) { 'https://rp1.serviceprovider.com/auth/saml/metadata' } + let(:identity) { create(:identity, :active, user: user, service_provider: service_provider) } + + subject { IdentityDecorator.new(identity) } + + context 'for an sp with a failure to proof url' do + it 'returns the failure_to_proof_url for the sp' do + failure_to_proof_url = ServiceProvider.from_issuer(service_provider).failure_to_proof_url + expect(subject.failure_to_proof_url).to eq(failure_to_proof_url) + end + end + + context 'for an sp without a failure to proof URL' do + let(:service_provider) { 'http://localhost:3000' } + + it 'returns nil' do + expect(subject.failure_to_proof_url).to eq(nil) + end + end + end end diff --git a/spec/models/null_service_provider_spec.rb b/spec/models/null_service_provider_spec.rb index 7035562b026..ae5ec717123 100644 --- a/spec/models/null_service_provider_spec.rb +++ b/spec/models/null_service_provider_spec.rb @@ -58,6 +58,12 @@ end end + describe '#failure_to_proof_url' do + it 'returns nil' do + expect(subject.failure_to_proof_url).to be_nil + end + end + describe '#issuer' do it 'returns the issuer argument' do expect(subject.issuer).to eq 'foo' From 21ba61a95b8e8fdfd14e05013788b3f491b4ce01 Mon Sep 17 00:00:00 2001 From: Steve Urciuoli Date: Mon, 30 Jul 2018 13:55:43 -0400 Subject: [PATCH 05/25] LG-523 Update release script to remove workers and .sh on recycle **Why**: The worker instances are no longer used and the devops scripts were renamed --- bin/release | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/bin/release b/bin/release index ab0ebc620ed..76ccacbb58e 100755 --- a/bin/release +++ b/bin/release @@ -98,8 +98,7 @@ def deploy_to_int Dir.chdir "#{ENV['HOME']}/login-dot-gov/identity-devops" do puts "Deploying the `stages/int` branch to the `int` environment" ENV['AWS_PROFILE'] = 'identitysandbox.gov' - run "bin/asg-recycle.sh int idp" - run "bin/asg-recycle.sh int worker" + run "bin/asg-recycle int idp" end remove_login_dot_gov_dir @@ -111,8 +110,7 @@ def deploy_to_staging Dir.chdir "#{ENV['HOME']}/login-dot-gov/identity-devops" do puts "Deploying the `stages/staging` branch to the `staging` environment" ENV['AWS_PROFILE'] = 'login.gov' - run "bin/asg-recycle.sh staging idp" - run "bin/asg-recycle.sh staging worker" + run "bin/asg-recycle staging idp" end remove_login_dot_gov_dir @@ -124,8 +122,7 @@ def deploy_to_prod Dir.chdir "#{ENV['HOME']}/login-dot-gov/identity-devops" do puts "Deploying the `stages/prod` branch to the `prod` environment" ENV['AWS_PROFILE'] = 'login.gov' - run "bin/asg-recycle.sh prod idp" - run "bin/asg-recycle.sh prod worker" + run "bin/asg-recycle prod idp" end remove_login_dot_gov_dir From 4ebdba30dbec1e645ad95eb0ab1df81c165d4104 Mon Sep 17 00:00:00 2001 From: Jonathan Hooper Date: Wed, 1 Aug 2018 07:55:48 -0400 Subject: [PATCH 06/25] Hardcode session encryption cost for migration (#2395) **Why**: In #2353 we changed the scrypt cost with changed the scrypt cost which affected the session encryptor causing sessions encrypted by old and new hosts to be incompatible. This commit hardcodes the cost in the deprecated encryptor so that the sessions will be compatible between hosts. --- .../encryption/encryptors/deprecated_session_encryptor.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/encryption/encryptors/deprecated_session_encryptor.rb b/app/services/encryption/encryptors/deprecated_session_encryptor.rb index fac387466f2..893de29cbc5 100644 --- a/app/services/encryption/encryptors/deprecated_session_encryptor.rb +++ b/app/services/encryption/encryptors/deprecated_session_encryptor.rb @@ -18,7 +18,7 @@ def self.load_or_init_user_access_key key = Figaro.env.session_encryption_key user_access_key = UserAccessKey.new( - password: key, salt: OpenSSL::Digest::SHA256.hexdigest(key) + password: key, salt: OpenSSL::Digest::SHA256.hexdigest(key), cost: '4000$8$4$' ) @user_access_key_scrypt_hash = user_access_key.as_scrypt_hash user_access_key From e5e29bbf7577576efeb33d6dbea78b1bf25b9cab Mon Sep 17 00:00:00 2001 From: Steve Urciuoli Date: Wed, 1 Aug 2018 22:19:16 -0400 Subject: [PATCH 07/25] LG-525 Fix IDV for users without phones **Why**: Users are unable to complete IDV if they setup their account with an authenticator app. **How**: Allow sms verification if it is in the middle of IDV verification by checking a session variable. Fix the spec / test. --- .../otp_verification_controller.rb | 6 +++++- spec/features/idv/steps/phone_step_spec.rb | 6 ++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/app/controllers/two_factor_authentication/otp_verification_controller.rb b/app/controllers/two_factor_authentication/otp_verification_controller.rb index de65c743a6e..8e661ac75f5 100644 --- a/app/controllers/two_factor_authentication/otp_verification_controller.rb +++ b/app/controllers/two_factor_authentication/otp_verification_controller.rb @@ -26,7 +26,7 @@ def create private def confirm_two_factor_enabled - return if confirmation_context? || phone_enabled? + return if confirming_phone? || phone_enabled? if current_user.two_factor_enabled? && !phone_enabled? && user_signed_in? return redirect_to user_two_factor_authentication_url @@ -35,6 +35,10 @@ def confirm_two_factor_enabled redirect_to phone_setup_url end + def confirming_phone? + idv_context? || confirmation_context? + end + def phone_enabled? current_user.phone_enabled? end diff --git a/spec/features/idv/steps/phone_step_spec.rb b/spec/features/idv/steps/phone_step_spec.rb index 4cd104b912d..a4b775e522d 100644 --- a/spec/features/idv/steps/phone_step_spec.rb +++ b/spec/features/idv/steps/phone_step_spec.rb @@ -36,6 +36,12 @@ expect(page).to have_content(t('idv.titles.otp_delivery_method')) expect(page).to have_current_path(idv_otp_delivery_method_path) + + choose_idv_otp_delivery_method_sms + + expect(page).to have_content(t('devise.two_factor_authentication.header_text')) + expect(page).to_not have_content(t('devise.two_factor_authentication.totp_header_text')) + expect(page).to_not have_content(t('two_factor_authentication.login_options_link_text')) end end From 32bfe58ba515e272c84e4b1caff96264d0adf521 Mon Sep 17 00:00:00 2001 From: Jonathan Hooper Date: Thu, 2 Aug 2018 09:47:34 -0400 Subject: [PATCH 08/25] Write 2L KMS encrypted sessions (#2373) **Why**: We are moving away from the user access keys in favor of 2L-KMS which involves aes encrypted ciphertexts wrapped by KMS --- .../encryptors/session_encryptor.rb | 6 +++- .../encryptors/session_encryptor_spec.rb | 29 +++++++++---------- 2 files changed, 19 insertions(+), 16 deletions(-) diff --git a/app/services/encryption/encryptors/session_encryptor.rb b/app/services/encryption/encryptors/session_encryptor.rb index df9b39b73ec..496871d8b3f 100644 --- a/app/services/encryption/encryptors/session_encryptor.rb +++ b/app/services/encryption/encryptors/session_encryptor.rb @@ -3,7 +3,11 @@ module Encryptors class SessionEncryptor include Encodable - delegate :encrypt, to: :deprecated_encryptor + def encrypt(plaintext) + aes_ciphertext = AesEncryptor.new.encrypt(plaintext, aes_encryption_key) + kms_ciphertext = KmsClient.new.encrypt(aes_ciphertext) + encode(kms_ciphertext) + end def decrypt(ciphertext) return deprecated_encryptor.decrypt(ciphertext) if legacy?(ciphertext) diff --git a/spec/services/encryption/encryptors/session_encryptor_spec.rb b/spec/services/encryption/encryptors/session_encryptor_spec.rb index 95efbf641aa..6d5cd79fd4e 100644 --- a/spec/services/encryption/encryptors/session_encryptor_spec.rb +++ b/spec/services/encryption/encryptors/session_encryptor_spec.rb @@ -4,15 +4,19 @@ let(:plaintext) { '{ "foo": "bar" }' } describe '#encrypt' do - it 'returns ciphertext created by the deprecated session encryptor' do - expected_ciphertext = '123abc' - - deprecated_encryptor = Encryption::Encryptors::DeprecatedSessionEncryptor.new - expect(deprecated_encryptor).to receive(:encrypt). - with(plaintext). - and_return(expected_ciphertext) - expect(Encryption::Encryptors::DeprecatedSessionEncryptor).to receive(:new). - and_return(deprecated_encryptor) + it 'returns a KMS wrapped AES encrypted ciphertext' do + aes_encryptor = instance_double(Encryption::Encryptors::AesEncryptor) + kms_client = instance_double(Encryption::KmsClient) + allow(aes_encryptor).to receive(:encrypt). + with(plaintext, Figaro.env.session_encryption_key[0...32]). + and_return('aes output') + allow(kms_client).to receive(:encrypt). + with('aes output'). + and_return('kms output') + allow(Encryption::Encryptors::AesEncryptor).to receive(:new).and_return(aes_encryptor) + allow(Encryption::KmsClient).to receive(:new).and_return(kms_client) + + expected_ciphertext = Base64.strict_encode64('kms output') ciphertext = subject.encrypt(plaintext) @@ -30,12 +34,7 @@ end context 'with a 2L-KMS ciphertext' do - let(:ciphertext) do - key = Figaro.env.session_encryption_key[0...32] - aes_ciphertext = Encryption::Encryptors::AesEncryptor.new.encrypt(plaintext, key) - kms_ciphertext = Encryption::KmsClient.new.encrypt(aes_ciphertext) - Base64.strict_encode64(kms_ciphertext) - end + let(:ciphertext) { Encryption::Encryptors::SessionEncryptor.new.encrypt(plaintext) } it 'decrypts the ciphertext' do expect(subject.decrypt(ciphertext)).to eq(plaintext) From dac82a694e35be5c3d34ec6fa960466cbfe3c0a0 Mon Sep 17 00:00:00 2001 From: Jonathan Hooper Date: Thu, 2 Aug 2018 09:47:48 -0400 Subject: [PATCH 09/25] LG-490 Use 32 byte salts for passwords (#2372) **Why**: Because it doesn't make sense to generate 10 byte salts and digest them when we can generate 32 byte salts directly --- app/services/encryption/password_verifier.rb | 2 +- .../encryption/password_verifier_spec.rb | 27 ++----------------- 2 files changed, 3 insertions(+), 26 deletions(-) diff --git a/app/services/encryption/password_verifier.rb b/app/services/encryption/password_verifier.rb index 9762bc7c2ea..f45d1402fd1 100644 --- a/app/services/encryption/password_verifier.rb +++ b/app/services/encryption/password_verifier.rb @@ -29,7 +29,7 @@ def to_s end def self.digest(password) - salt = Devise.friendly_token[0, 20] + salt = SecureRandom.hex(32) uak = UserAccessKey.new(password: password, salt: salt) uak.build PasswordDigest.new( diff --git a/spec/services/encryption/password_verifier_spec.rb b/spec/services/encryption/password_verifier_spec.rb index e1d67eae9fd..fa93fbcb490 100644 --- a/spec/services/encryption/password_verifier_spec.rb +++ b/spec/services/encryption/password_verifier_spec.rb @@ -3,8 +3,8 @@ describe Encryption::PasswordVerifier do describe '.digest' do it 'creates a digest from the password' do - salt = '1' * 20 - allow(Devise).to receive(:friendly_token).and_return(salt) + salt = '1' * 64 # 32 hex encoded bytes is 64 characters + allow(SecureRandom).to receive(:hex).once.with(32).and_return(salt) digest = described_class.digest('saltypickles') @@ -42,29 +42,6 @@ expect(result).to eq(false) end - it 'allows verification of a password with a 32 byte salt' do - # Once all new password digests are 32 bytes, this test can be torn down - # as it will be covered by the tests above - - password = 'saltypickles' - password_digest = { - encrypted_password: '8a5c5b165fd3a2fce81bc914d91a106b76dfdd9e8c2addf0e0f27424a32ca4cb', - encryption_key: 'VUl6QFRZeQZ5Xl9IUVsBZmRndkNkXHJDYgAFRX9nYVl8c3paUWhyX2p - oegBqaFgAeVpfWWpmcgRRXnJHfANAaFJdfQR/eHJbfXVASn0AYnx9dlRifAJ2BFF4dkFmdWZ - /Y3VASn0DfUlqZHp2aWdEAWZlWHRhWmZnY2dbAFMAfQFiW0hCVWNEAmFoan9TSnEEZUpcdmR - nanZ/dHZ/agJqR1VkWEd+XlgCUnRUXFFnYkdqXVQBZHRiUVMCantRAVRef3ZYZmNnW0JTdHJ - RfltYR353akVRAAV9VHZmAmUBZkBlXlxnZXVUe1RKWAJVXGp2Y1tqSmQBVFlnXWpiYkp6eWZ - dQEd/eFhHYWVYd2VKflhpA3lKZGZlSH5dal1+eHZJZWQACXlZR1lUd3ZeeVpfWVNnelhmewB - VZ2p7C3hqf3lhXV0XYDp0YmBnL0dlZQcKLQtUXA=='.gsub(/\s/, ''), - password_salt: '6bb7555423136772304b40c10afe11e459c4021a1a47dfd11fcc955e0a2161e2', - password_cost: '4000$8$4$', - }.to_json - - result = described_class.verify(password: password, digest: password_digest) - - expect(result).to eq(true) - end - it 'allows verification of a legacy password with a 20 byte salt' do # Legacy passwords had 20 bytes salts, which were SHA256 digested to get # to a 32 byte salt (64 char hexdigest). This test verifies that the From a14b8905e6e33dfd337a9a037165dd2c1604ffcd Mon Sep 17 00:00:00 2001 From: Jonathan Hooper Date: Thu, 2 Aug 2018 09:52:24 -0400 Subject: [PATCH 10/25] Revert "Merge pull request #2351 from 18F/mb-refactor-redirect-uri-validation" (#2400) This reverts commit 1087dd8416dc37198250280391e6e2fc982f97ac, reversing changes made to bd698faac2c3b17f78a214d682ea368c42795d73. --- .reek | 2 + .../concerns/secure_headers_concern.rb | 7 +- .../authorization_controller.rb | 2 +- .../service_provider_session_decorator.rb | 24 +- app/forms/openid_connect_authorize_form.rb | 29 +- app/forms/openid_connect_logout_form.rb | 41 +-- app/services/openid_connect_redirector.rb | 97 +++++++ app/validators/redirect_uri_validator.rb | 32 --- config/service_providers.yml | 5 +- .../openid_connect/logout_controller_spec.rb | 6 +- .../openid_connect/openid_connect_spec.rb | 2 +- .../redirect_uri_validation_spec.rb | 259 ------------------ .../saml/redirect_uri_validation_spec.rb | 27 -- .../openid_connect_authorize_form_spec.rb | 4 +- spec/forms/openid_connect_logout_form_spec.rb | 4 +- .../openid_connect_redirector_spec.rb | 150 ++++++++++ 16 files changed, 302 insertions(+), 389 deletions(-) create mode 100644 app/services/openid_connect_redirector.rb delete mode 100644 app/validators/redirect_uri_validator.rb delete mode 100644 spec/features/openid_connect/redirect_uri_validation_spec.rb delete mode 100644 spec/features/saml/redirect_uri_validation_spec.rb create mode 100644 spec/services/openid_connect_redirector_spec.rb diff --git a/.reek b/.reek index 12bda872d89..3276cc4e715 100644 --- a/.reek +++ b/.reek @@ -3,6 +3,7 @@ Attribute: ControlParameter: exclude: - CustomDeviseFailureApp#i18n_message + - OpenidConnectRedirector#initialize - NoRetryJobs#call - PhoneFormatter#self.format - Users::TwoFactorAuthenticationController#invalid_phone_number @@ -88,6 +89,7 @@ TooManyConstants: TooManyInstanceVariables: exclude: - OpenidConnectAuthorizeForm + - OpenidConnectRedirector - Idv::VendorResult - CloudhsmKeyGenerator TooManyStatements: diff --git a/app/controllers/concerns/secure_headers_concern.rb b/app/controllers/concerns/secure_headers_concern.rb index 23da64ea5c3..74a11e2078b 100644 --- a/app/controllers/concerns/secure_headers_concern.rb +++ b/app/controllers/concerns/secure_headers_concern.rb @@ -5,20 +5,17 @@ def apply_secure_headers_override return if stored_url_for_user.blank? authorize_params = URIService.params(stored_url_for_user) + authorize_form = OpenidConnectAuthorizeForm.new(authorize_params) return unless authorize_form.valid? - redirect_uri = authorize_params[:redirect_uri] - override_content_security_policy_directives( - form_action: ["'self'", redirect_uri].compact, + form_action: ["'self'", authorize_form.sp_redirect_uri].compact, preserve_schemes: true ) end - private - def stored_url_for_user sp_session[:request_url] end diff --git a/app/controllers/openid_connect/authorization_controller.rb b/app/controllers/openid_connect/authorization_controller.rb index ad130f16596..3d499416659 100644 --- a/app/controllers/openid_connect/authorization_controller.rb +++ b/app/controllers/openid_connect/authorization_controller.rb @@ -52,7 +52,7 @@ def track_authorize_analytics(result) def apply_secure_headers_override override_content_security_policy_directives( - form_action: ["'self'", authorization_params[:redirect_uri]].compact, + form_action: ["'self'", @authorize_form.sp_redirect_uri].compact, preserve_schemes: true ) end diff --git a/app/decorators/service_provider_session_decorator.rb b/app/decorators/service_provider_session_decorator.rb index 1e31ad4321a..8e91dce7f04 100644 --- a/app/decorators/service_provider_session_decorator.rb +++ b/app/decorators/service_provider_session_decorator.rb @@ -92,12 +92,8 @@ def sp_agency end def sp_return_url - if sp.redirect_uris.present? && valid_oidc_request? - URIService.add_params( - oidc_redirect_uri, - error: 'access_denied', - state: request_params[:state] - ) + if sp.redirect_uris.present? && openid_connect_redirector.valid? + openid_connect_redirector.decline_redirect_uri else sp.return_to_sp_url end @@ -131,19 +127,7 @@ def request_url sp_session[:request_url] || service_provider_request.url end - def valid_oidc_request? - authorize_form .valid? - end - - def authorize_form - OpenidConnectAuthorizeForm.new(request_params) - end - - def oidc_redirect_uri - request_params[:redirect_uri] - end - - def request_params - @request_params ||= URIService.params(request_url) + def openid_connect_redirector + @_openid_connect_redirector ||= OpenidConnectRedirector.from_request_url(request_url) end end diff --git a/app/forms/openid_connect_authorize_form.rb b/app/forms/openid_connect_authorize_form.rb index 83596f7876b..4ee64191fd9 100644 --- a/app/forms/openid_connect_authorize_form.rb +++ b/app/forms/openid_connect_authorize_form.rb @@ -2,7 +2,6 @@ class OpenidConnectAuthorizeForm include ActiveModel::Model include ActionView::Helpers::TranslationHelper - include RedirectUriValidator SIMPLE_ATTRS = %i[ client_id @@ -34,6 +33,7 @@ class OpenidConnectAuthorizeForm validate :validate_acr_values validate :validate_client_id + validate :validate_redirect_uri validate :validate_scope def initialize(params) @@ -43,6 +43,10 @@ def initialize(params) instance_variable_set(:"@#{key}", params[key]) end @prompt ||= 'select_account' + + @openid_connect_redirector = OpenidConnectRedirector.new( + redirect_uri: redirect_uri, service_provider: service_provider, state: state, errors: errors + ) end def submit @@ -55,6 +59,10 @@ def loa3_requested? loa == 3 end + def sp_redirect_uri + openid_connect_redirector.validated_input_redirect_uri + end + def service_provider @_service_provider ||= ServiceProvider.from_issuer(client_id) end @@ -71,15 +79,13 @@ def link_identity_to_service_provider(current_user, rails_session_id) end def success_redirect_uri - uri = redirect_uri unless errors.include?(:redirect_uri) code = identity&.session_uuid - - URIService.add_params(uri, code: code, state: state) if code + openid_connect_redirector.success_redirect_uri(code: code) if code end private - attr_reader :identity, :success, :already_linked + attr_reader :identity, :success, :openid_connect_redirector, :already_linked def requested_attributes @requested_attributes ||= @@ -101,6 +107,10 @@ def validate_client_id errors.add(:client_id, t('openid_connect.authorization.errors.bad_client_id')) end + def validate_redirect_uri + openid_connect_redirector.validate + end + def validate_scope return if scope.present? errors.add(:scope, t('openid_connect.authorization.errors.no_valid_scope')) @@ -131,14 +141,7 @@ def result_uri end def error_redirect_uri - uri = redirect_uri unless errors.include?(:redirect_uri) - - URIService.add_params( - uri, - error: 'invalid_request', - error_description: errors.full_messages.join(' '), - state: state - ) + openid_connect_redirector.error_redirect_uri end end # rubocop:enable Metrics/ClassLength diff --git a/app/forms/openid_connect_logout_form.rb b/app/forms/openid_connect_logout_form.rb index 98bc7d2af4a..1b21e5bd241 100644 --- a/app/forms/openid_connect_logout_form.rb +++ b/app/forms/openid_connect_logout_form.rb @@ -1,7 +1,6 @@ class OpenidConnectLogoutForm include ActiveModel::Model include ActionView::Helpers::TranslationHelper - include RedirectUriValidator ATTRS = %i[ id_token_hint @@ -17,6 +16,7 @@ class OpenidConnectLogoutForm validates :post_logout_redirect_uri, presence: true validates :state, presence: true, length: { minimum: RANDOM_VALUE_MINIMUM_LENGTH } + validate :validate_redirect_uri validate :validate_identity def initialize(params) @@ -25,6 +25,7 @@ def initialize(params) end @identity = load_identity + @openid_connect_redirector = build_openid_connect_redirector end def submit @@ -38,6 +39,7 @@ def submit private attr_reader :identity, + :openid_connect_redirector, :success def load_identity @@ -56,6 +58,20 @@ def identity_from_payload(payload) AgencyIdentityLinker.sp_identity_from_uuid_and_sp(uuid, sp) end + def build_openid_connect_redirector + OpenidConnectRedirector.new( + redirect_uri: post_logout_redirect_uri, + service_provider: service_provider, + state: state, + errors: errors, + error_attr: :post_logout_redirect_uri + ) + end + + def validate_redirect_uri + openid_connect_redirector.validate + end + def validate_identity errors.add(:id_token_hint, t('openid_connect.logout.errors.id_token_hint')) unless identity end @@ -74,23 +90,10 @@ def extra_analytics_attributes end def redirect_uri - success ? logout_redirect_uri : error_redirect_uri - end - - def logout_redirect_uri - uri = post_logout_redirect_uri unless errors.include?(:redirect_uri) - - URIService.add_params(uri, state: state) - end - - def error_redirect_uri - uri = post_logout_redirect_uri unless errors.include?(:redirect_uri) - - URIService.add_params( - uri, - error: 'invalid_request', - error_description: errors.full_messages.join(' '), - state: state - ) + if success + openid_connect_redirector.logout_redirect_uri + else + openid_connect_redirector.error_redirect_uri + end end end diff --git a/app/services/openid_connect_redirector.rb b/app/services/openid_connect_redirector.rb new file mode 100644 index 00000000000..4e5d1b0ebbf --- /dev/null +++ b/app/services/openid_connect_redirector.rb @@ -0,0 +1,97 @@ +class OpenidConnectRedirector + include ActionView::Helpers::TranslationHelper + + def self.from_request_url(request_url) + params = URIService.params(request_url) + + new( + redirect_uri: params[:redirect_uri], + service_provider: ServiceProvider.from_issuer(params[:client_id]), + state: params[:state] + ) + end + + def initialize(redirect_uri:, service_provider:, state:, errors: nil, error_attr: :redirect_uri) + @redirect_uri = redirect_uri + @service_provider = service_provider + @state = state + @errors = errors || ActiveModel::Errors.new(self) + @error_attr = error_attr + end + + def valid? + validate + errors.blank? + end + + def validate + validate_redirect_uri + validate_redirect_uri_matches_sp_redirect_uri + end + + def success_redirect_uri(code:) + URIService.add_params(validated_input_redirect_uri, code: code, state: state) + end + + def decline_redirect_uri + URIService.add_params( + validated_input_redirect_uri, + error: 'access_denied', + state: state + ) + end + + def error_redirect_uri + URIService.add_params( + validated_input_redirect_uri, + error: 'invalid_request', + error_description: errors.full_messages.join(' '), + state: state + ) + end + + def logout_redirect_uri + URIService.add_params(validated_input_redirect_uri, state: state) + end + + def validated_input_redirect_uri + redirect_uri if redirect_uri_matches_sp_redirect_uri? + end + + private + + attr_reader :redirect_uri, :service_provider, :state, :errors, :error_attr + + def validate_redirect_uri + _uri = URI(redirect_uri) + rescue ArgumentError, URI::InvalidURIError + errors.add(error_attr, t('openid_connect.authorization.errors.redirect_uri_invalid')) + end + + def validate_redirect_uri_matches_sp_redirect_uri + return if redirect_uri_matches_sp_redirect_uri? + errors.add(error_attr, t('openid_connect.authorization.errors.redirect_uri_no_match')) + end + + # rubocop:disable Metrics/AbcSize + # rubocop:disable Metrics/CyclomaticComplexity + # rubocop:disable Metrics/MethodLength + def redirect_uri_matches_sp_redirect_uri? + return unless redirect_uri.present? && service_provider.active? + parsed_redirect_uri = URI(redirect_uri) + service_provider.redirect_uris.any? do |sp_redirect_uri| + parsed_sp_redirect_uri = URI(sp_redirect_uri) + + parsed_redirect_uri.scheme == parsed_sp_redirect_uri.scheme && + parsed_redirect_uri.port == parsed_sp_redirect_uri.port && + parsed_redirect_uri.host == parsed_sp_redirect_uri.host && + parsed_redirect_uri.path.start_with?(parsed_sp_redirect_uri.path) + end + rescue URI::Error + false + end + + # rubocop:enable Metrics/AbcSize + # rubocop:enable Metrics/CyclomaticComplexity + # rubocop:enable Metrics/MethodLength +end diff --git a/app/validators/redirect_uri_validator.rb b/app/validators/redirect_uri_validator.rb deleted file mode 100644 index a28c91816c8..00000000000 --- a/app/validators/redirect_uri_validator.rb +++ /dev/null @@ -1,32 +0,0 @@ -module RedirectUriValidator - extend ActiveSupport::Concern - - included do - attr_reader :redirect_uri, :post_logout_redirect_uri, :service_provider - - validate :allowed_redirect_uri - end - - private - - def allowed_redirect_uri - return if any_registered_sp_redirect_uris_identical_to_the_requested_uri? - - errors.add(:redirect_uri, t('openid_connect.authorization.errors.redirect_uri_no_match')) - end - - def any_registered_sp_redirect_uris_identical_to_the_requested_uri? - service_provider.redirect_uris.any? do |sp_redirect_uri| - parsed_sp_redirect_uri = URI.parse(sp_redirect_uri) - - parsed_sp_redirect_uri == parsed_redirect_uri - end - rescue ArgumentError, URI::InvalidURIError - errors.add(:redirect_uri, t('openid_connect.authorization.errors.redirect_uri_invalid')) - end - - def parsed_redirect_uri - requested_uri = post_logout_redirect_uri || redirect_uri - URI.parse(requested_uri) - end -end diff --git a/config/service_providers.yml b/config/service_providers.yml index 451b8f3a9c2..d853dc94075 100644 --- a/config/service_providers.yml +++ b/config/service_providers.yml @@ -47,7 +47,6 @@ test: 'urn:gov:gsa:openidconnect:test': redirect_uris: - 'gov.gsa.openidconnect.test://result' - - 'gov.gsa.openidconnect.test://result/logout' cert: 'saml_test_sp' friendly_name: 'Example iOS App' agency: '18F' @@ -58,7 +57,7 @@ test: 'urn:gov:gsa:openidconnect:sp:server': agency_id: 2 redirect_uris: - - 'http://localhost:7654/auth/result' + - 'http://localhost:7654/' - 'https://example.com' cert: 'saml_test_sp' friendly_name: 'Test SP' @@ -633,7 +632,7 @@ production: attribute_bundle: - x509_subject - x509_presented - + # My Move.mil 'urn:gov:gsa:openidconnect.profiles:sp:sso:dod:mymovemilprod': agency_id: 8 diff --git a/spec/controllers/openid_connect/logout_controller_spec.rb b/spec/controllers/openid_connect/logout_controller_spec.rb index 67bda6e2382..38500e8c69f 100644 --- a/spec/controllers/openid_connect/logout_controller_spec.rb +++ b/spec/controllers/openid_connect/logout_controller_spec.rb @@ -80,15 +80,11 @@ it 'tracks analytics' do stub_analytics - - errors = { - redirect_uri: [t('openid_connect.authorization.errors.redirect_uri_no_match')], - } expect(@analytics).to receive(:track_event). with(Analytics::LOGOUT_INITIATED, success: false, client_id: service_provider, - errors: errors, + errors: hash_including(:post_logout_redirect_uri), sp_initiated: true, oidc: true) diff --git a/spec/features/openid_connect/openid_connect_spec.rb b/spec/features/openid_connect/openid_connect_spec.rb index 314a4ca0a65..eeea3adcfa3 100644 --- a/spec/features/openid_connect/openid_connect_spec.rb +++ b/spec/features/openid_connect/openid_connect_spec.rb @@ -437,7 +437,7 @@ def sign_in_get_id_token response_type: 'code', acr_values: Saml::Idp::Constants::LOA1_AUTHN_CONTEXT_CLASSREF, scope: 'openid email', - redirect_uri: 'gov.gsa.openidconnect.test://result', + redirect_uri: 'gov.gsa.openidconnect.test://result/auth', state: state, prompt: 'select_account', nonce: nonce, diff --git a/spec/features/openid_connect/redirect_uri_validation_spec.rb b/spec/features/openid_connect/redirect_uri_validation_spec.rb deleted file mode 100644 index 2a3f4949044..00000000000 --- a/spec/features/openid_connect/redirect_uri_validation_spec.rb +++ /dev/null @@ -1,259 +0,0 @@ -require 'rails_helper' - -describe 'redirect_uri validation' do - context 'when the redirect_uri in the request does not match one that is registered' do - it 'displays error instead of branded landing page' do - visit_idp_from_sp_with_loa1_with_disallowed_redirect_uri - current_host = URI.parse(page.current_url).host - - expect(current_host).to eq 'www.example.com' - expect(page). - to have_content t('openid_connect.authorization.errors.redirect_uri_no_match') - end - end - - context 'when the redirect_uri is not a valid URI' do - it 'displays error instead of branded landing page' do - visit_idp_from_sp_with_loa1_with_invalid_redirect_uri - current_host = URI.parse(page.current_url).host - - expect(current_host).to eq 'www.example.com' - expect(page). - to have_content t('openid_connect.authorization.errors.redirect_uri_invalid') - end - end - - context 'when the service_provider is not active' do - it 'displays error instead of branded landing page' do - visit_idp_from_inactive_sp - current_host = URI.parse(page.current_url).host - - expect(current_host).to eq 'www.example.com' - expect(page). - to have_content t('openid_connect.authorization.errors.bad_client_id') - end - end - - context 'when redirect_uri is present in params but the request is not from an SP' do - it 'does not provide a link to the redirect_uri' do - visit sign_up_start_path(request_id: '123', redirect_uri: 'evil.com') - - expect(page).to_not have_link t('links.back_to_sp') - - visit new_user_session_path(request_id: '123', redirect_uri: 'evil.com') - - expect(page).to_not have_link t('links.back_to_sp') - end - end - - context 'when new non-SP request with redirect_uri is made after initial SP request' do - it 'does not provide a link to the new redirect_uri' do - state = SecureRandom.hex - visit_idp_from_sp_with_loa1_with_valid_redirect_uri(state: state) - visit sign_up_start_path(request_id: '123', redirect_uri: 'evil.com') - sp_redirect_uri = "http://localhost:7654/auth/result?error=access_denied&state=#{state}" - - expect(page). - to have_link(t('links.back_to_sp', sp: 'Test SP'), href: sp_redirect_uri) - - visit new_user_session_path(request_id: '123', redirect_uri: 'evil.com') - - expect(page). - to have_link(t('links.back_to_sp', sp: 'Test SP'), href: sp_redirect_uri) - end - end - - context 'when the user is already signed in directly' do - it 'displays error instead of redirecting' do - sign_in_and_2fa_user - - visit_idp_from_inactive_sp - current_host = URI.parse(page.current_url).host - - expect(current_host).to eq 'www.example.com' - expect(page). - to have_content t('openid_connect.authorization.errors.bad_client_id') - - visit_idp_from_sp_with_loa1_with_invalid_redirect_uri - current_host = URI.parse(page.current_url).host - - expect(current_host).to eq 'www.example.com' - expect(page). - to have_content t('openid_connect.authorization.errors.redirect_uri_invalid') - - visit_idp_from_sp_with_loa1_with_disallowed_redirect_uri - current_host = URI.parse(page.current_url).host - - expect(current_host).to eq 'www.example.com' - expect(page). - to have_content t('openid_connect.authorization.errors.redirect_uri_no_match') - end - end - - context 'when the user is already signed in via an SP' do - it 'displays error instead of redirecting' do - allow(FeatureManagement).to receive(:prefill_otp_codes?).and_return(true) - user = create(:user, :signed_up) - visit_idp_from_sp_with_loa1_with_valid_redirect_uri - click_link t('links.sign_in') - fill_in_credentials_and_submit(user.email, user.password) - click_submit_default - click_continue - - visit_idp_from_inactive_sp - current_host = URI.parse(page.current_url).host - - expect(current_host).to eq 'www.example.com' - expect(page). - to have_content t('openid_connect.authorization.errors.bad_client_id') - - visit_idp_from_sp_with_loa1_with_invalid_redirect_uri - current_host = URI.parse(page.current_url).host - - expect(current_host).to eq 'www.example.com' - expect(page). - to have_content t('openid_connect.authorization.errors.redirect_uri_invalid') - - visit_idp_from_sp_with_loa1_with_disallowed_redirect_uri - current_host = URI.parse(page.current_url).host - - expect(current_host).to eq 'www.example.com' - expect(page). - to have_content t('openid_connect.authorization.errors.redirect_uri_no_match') - end - end - - context 'when the SP has multiple registered redirect_uris and the second one is requested' do - it 'considers the request valid and redirects to the one requested' do - allow(FeatureManagement).to receive(:prefill_otp_codes?).and_return(true) - user = create(:user, :signed_up) - visit_idp_from_sp_with_loa1_with_second_valid_redirect_uri - click_link t('links.sign_in') - fill_in_credentials_and_submit(user.email, user.password) - click_submit_default - click_continue - - redirect_host = URI.parse(current_url).host - redirect_scheme = URI.parse(current_url).scheme - - expect(redirect_host).to eq('example.com') - expect(redirect_scheme).to eq('https') - end - end - - context 'when the SP does not have any registered redirect_uris' do - it 'considers the request invalid and does not redirect if the user signs in' do - allow(FeatureManagement).to receive(:prefill_otp_codes?).and_return(true) - user = create(:user, :signed_up) - visit_idp_from_sp_that_does_not_have_redirect_uris - current_host = URI.parse(page.current_url).host - - expect(current_host).to eq 'www.example.com' - expect(page). - to have_content t('openid_connect.authorization.errors.redirect_uri_no_match') - - visit new_user_session_path - fill_in_credentials_and_submit(user.email, user.password) - click_submit_default - click_continue - - expect(page).to have_current_path account_path - end - end - - def visit_idp_from_sp_with_loa1_with_disallowed_redirect_uri(state: SecureRandom.hex) - client_id = 'urn:gov:gsa:openidconnect:sp:server' - nonce = SecureRandom.hex - - visit openid_connect_authorize_path( - client_id: client_id, - response_type: 'code', - acr_values: Saml::Idp::Constants::LOA1_AUTHN_CONTEXT_CLASSREF, - scope: 'openid email', - redirect_uri: 'https://example.com.evil.com/auth/result', - state: state, - prompt: 'select_account', - nonce: nonce - ) - end - - def visit_idp_from_sp_with_loa1_with_invalid_redirect_uri(state: SecureRandom.hex) - client_id = 'urn:gov:gsa:openidconnect:sp:server' - nonce = SecureRandom.hex - - visit openid_connect_authorize_path( - client_id: client_id, - response_type: 'code', - acr_values: Saml::Idp::Constants::LOA1_AUTHN_CONTEXT_CLASSREF, - scope: 'openid email', - redirect_uri: ':aaaa', - state: state, - prompt: 'select_account', - nonce: nonce - ) - end - - def visit_idp_from_inactive_sp(state: SecureRandom.hex) - client_id = 'inactive' - nonce = SecureRandom.hex - - visit openid_connect_authorize_path( - client_id: client_id, - response_type: 'code', - acr_values: Saml::Idp::Constants::LOA1_AUTHN_CONTEXT_CLASSREF, - scope: 'openid email', - redirect_uri: 'http://localhost:7654/auth/result', - state: state, - prompt: 'select_account', - nonce: nonce - ) - end - - def visit_idp_from_sp_with_loa1_with_valid_redirect_uri(state: SecureRandom.hex) - client_id = 'urn:gov:gsa:openidconnect:sp:server' - nonce = SecureRandom.hex - - visit openid_connect_authorize_path( - client_id: client_id, - response_type: 'code', - acr_values: Saml::Idp::Constants::LOA1_AUTHN_CONTEXT_CLASSREF, - scope: 'openid email', - redirect_uri: 'http://localhost:7654/auth/result', - state: state, - prompt: 'select_account', - nonce: nonce - ) - end - - def visit_idp_from_sp_with_loa1_with_second_valid_redirect_uri(state: SecureRandom.hex) - client_id = 'urn:gov:gsa:openidconnect:sp:server' - nonce = SecureRandom.hex - - visit openid_connect_authorize_path( - client_id: client_id, - response_type: 'code', - acr_values: Saml::Idp::Constants::LOA1_AUTHN_CONTEXT_CLASSREF, - scope: 'openid email', - redirect_uri: 'https://example.com', - state: state, - prompt: 'select_account', - nonce: nonce - ) - end - - def visit_idp_from_sp_that_does_not_have_redirect_uris(state: SecureRandom.hex) - client_id = 'http://test.host' - nonce = SecureRandom.hex - - visit openid_connect_authorize_path( - client_id: client_id, - response_type: 'code', - acr_values: Saml::Idp::Constants::LOA1_AUTHN_CONTEXT_CLASSREF, - scope: 'openid email', - redirect_uri: 'http://test.host', - state: state, - prompt: 'select_account', - nonce: nonce - ) - end -end diff --git a/spec/features/saml/redirect_uri_validation_spec.rb b/spec/features/saml/redirect_uri_validation_spec.rb deleted file mode 100644 index 8b1c5199428..00000000000 --- a/spec/features/saml/redirect_uri_validation_spec.rb +++ /dev/null @@ -1,27 +0,0 @@ -require 'rails_helper' - -describe 'redirect_uri validation' do - include SamlAuthHelper - - context 'when redirect_uri param is included in SAML request' do - it 'uses the return_to_sp_url URL and not the redirect_uri' do - allow(FeatureManagement).to receive(:prefill_otp_codes?).and_return(true) - user = create(:user, :signed_up) - visit api_saml_auth_path( - SAMLRequest: CGI.unescape(saml_request(saml_settings)), redirect_uri: 'http://evil.com' - ) - sp = ServiceProvider.find_by(issuer: 'http://localhost:3000') - - expect(page). - to have_link t('links.back_to_sp', sp: sp.friendly_name), href: sp.return_to_sp_url - - click_link t('links.sign_in') - fill_in_credentials_and_submit(user.email, user.password) - click_submit_default - click_continue - click_submit_default - - expect(current_url).to eq sp.acs_url - end - end -end diff --git a/spec/forms/openid_connect_authorize_form_spec.rb b/spec/forms/openid_connect_authorize_form_spec.rb index 4ff1fa7fd0e..6522593872d 100644 --- a/spec/forms/openid_connect_authorize_form_spec.rb +++ b/spec/forms/openid_connect_authorize_form_spec.rb @@ -180,9 +180,9 @@ end end - context 'with a redirect_uri that only partially matches any registered redirect_uri' do + context 'with a redirect_uri that adds on to the registered redirect_uri' do let(:redirect_uri) { 'gov.gsa.openidconnect.test://result/more/extra' } - it { expect(valid?).to eq(false) } + it { expect(valid?).to eq(true) } end end diff --git a/spec/forms/openid_connect_logout_form_spec.rb b/spec/forms/openid_connect_logout_form_spec.rb index 09f9914e908..1a385df42ed 100644 --- a/spec/forms/openid_connect_logout_form_spec.rb +++ b/spec/forms/openid_connect_logout_form_spec.rb @@ -137,7 +137,7 @@ it 'is not valid' do expect(valid?).to eq(false) - expect(form.errors[:redirect_uri]).to be_present + expect(form.errors[:post_logout_redirect_uri]).to be_present end end @@ -146,7 +146,7 @@ it 'is not valid' do expect(valid?).to eq(false) - expect(form.errors[:redirect_uri]). + expect(form.errors[:post_logout_redirect_uri]). to include(t('openid_connect.authorization.errors.redirect_uri_no_match')) end end diff --git a/spec/services/openid_connect_redirector_spec.rb b/spec/services/openid_connect_redirector_spec.rb new file mode 100644 index 00000000000..0c1c621e956 --- /dev/null +++ b/spec/services/openid_connect_redirector_spec.rb @@ -0,0 +1,150 @@ +require 'rails_helper' + +RSpec.describe OpenidConnectRedirector do + include Rails.application.routes.url_helpers + + let(:redirect_uri) { 'http://localhost:7654/' } + let(:state) { SecureRandom.hex } + let(:service_provider) { ServiceProvider.from_issuer('urn:gov:gsa:openidconnect:sp:server') } + let(:errors) { ActiveModel::Errors.new(nil) } + + subject(:redirector) do + OpenidConnectRedirector.new( + redirect_uri: redirect_uri, + service_provider: service_provider, + state: state, + errors: errors + ) + end + + describe '.from_request_url' do + it 'builds a redirector from an OpenID request_url' do + request_url = openid_connect_authorize_url( + client_id: service_provider.issuer, + redirect_uri: redirect_uri, + state: state + ) + + result = OpenidConnectRedirector.from_request_url(request_url) + + expect(result).to be_a(OpenidConnectRedirector) + expect(result.send(:redirect_uri)).to eq(redirect_uri) + expect(result.send(:service_provider)).to eq(service_provider) + expect(result.send(:state)).to eq(state) + end + end + + describe '#validate' do + context 'with a redirect_uri that spoofs a hostname' do + let(:redirect_uri) { 'https://example.com.evilish.com/' } + + it 'is invalid' do + redirector.validate + expect(errors[:redirect_uri]). + to include(t('openid_connect.authorization.errors.redirect_uri_no_match')) + end + end + + context 'with a valid redirect_uri' do + let(:redirect_uri) { 'http://localhost:7654/result/more/extra' } + it 'is valid' do + redirector.validate + expect(errors).to be_empty + end + end + + context 'with a malformed redirect_uri' do + let(:redirect_uri) { ':aaaa' } + it 'has errors' do + redirector.validate + expect(errors[:redirect_uri]). + to include(t('openid_connect.authorization.errors.redirect_uri_invalid')) + end + end + + context 'with a redirect_uri not registered to the service provider' do + let(:redirect_uri) { 'http://localhost:3000/test' } + it 'has errors' do + redirector.validate + expect(errors[:redirect_uri]). + to include(t('openid_connect.authorization.errors.redirect_uri_no_match')) + end + end + end + + describe '#success_redirect_uri' do + it 'adds the code and state to the URL' do + code = SecureRandom.hex + expect(redirector.success_redirect_uri(code: code)). + to eq(URIService.add_params(redirect_uri, code: code, state: state)) + end + end + + describe '#decline_redirect_uri' do + it 'adds the state and access_denied to the URL' do + expect(redirector.decline_redirect_uri). + to eq(URIService.add_params(redirect_uri, state: state, error: 'access_denied')) + end + end + + describe '#error_redirect_uri' do + before { expect(errors).to receive(:full_messages).and_return(['some attribute is missing']) } + + it 'adds the errors to the URL' do + expect(redirector.error_redirect_uri). + to eq(URIService.add_params(redirect_uri, + state: state, + error: 'invalid_request', + error_description: 'some attribute is missing')) + end + end + + describe '#logout_redirect_uri' do + it 'adds the state to the URL' do + expect(redirector.logout_redirect_uri). + to eq(URIService.add_params(redirect_uri, state: state)) + end + end + + describe '#validated_input_redirect_uri' do + let(:service_provider) { ServiceProvider.new(redirect_uris: redirect_uris, active: true) } + + subject(:validated_input_redirect_uri) { redirector.validated_input_redirect_uri } + + context 'when the service provider has no redirect URIs' do + let(:redirect_uris) { [] } + + it 'is nil' do + expect(validated_input_redirect_uri).to be_nil + end + end + + context 'when the service provider has 2 redirect URIs' do + let(:redirect_uris) { %w[http://localhost:1234/result my-app://result] } + + context 'when a URL matching the first redirect_uri is passed in' do + let(:redirect_uri) { 'http://localhost:1234/result/more' } + + it 'is that URL' do + expect(validated_input_redirect_uri).to eq(redirect_uri) + end + end + + context 'when a URL matching the second redirect_uri is passed in' do + let(:redirect_uri) { 'my-app://result/more' } + + it 'is that URL' do + expect(validated_input_redirect_uri).to eq(redirect_uri) + end + end + + context 'when a URL matching the neither redirect_uri is passed in' do + let(:redirect_uri) { 'https://example.com' } + + it 'is nil' do + expect(validated_input_redirect_uri).to be_nil + end + end + end + end +end From ca074f891fe31e12474b463777b11b0a62abd104 Mon Sep 17 00:00:00 2001 From: Moncef Belyamani Date: Thu, 2 Aug 2018 10:31:25 -0400 Subject: [PATCH 11/25] Allow exception logs to capture nil user **Why**: For some reason, the exception notification gem returns `nil` sometimes for `@kontroller.analytics_user`. **How**: Use an instance of AnonymousUser when a user doesn't exist --- app/views/exception_notifier/_session.text.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/exception_notifier/_session.text.erb b/app/views/exception_notifier/_session.text.erb index 496e1aa9abc..3446c186bdd 100644 --- a/app/views/exception_notifier/_session.text.erb +++ b/app/views/exception_notifier/_session.text.erb @@ -16,7 +16,7 @@ Referer: <%= @request.referer %> <% session['user_return_to'] = session['user_return_to']&.split('?')&.first %> Session: <%= session %> -<% user = @kontroller.analytics_user %> +<% user = @kontroller.analytics_user || AnonymousUser.new %> User UUID: <%= user.uuid %> User's Country (based on phone): <%= Phonelib.parse(user.phone).country %> From 0afe7d00ddec86b30a58c42f575ad170ab01d95d Mon Sep 17 00:00:00 2001 From: Moncef Belyamani Date: Fri, 3 Aug 2018 11:19:32 -0400 Subject: [PATCH 12/25] Remove unused code that inflated our session size **Why**: This is the root cause for the KMS errors we experienced in production yesterday. We were storing the OIDC request params in the session for no reason. The more parameters an agency included in their initial OIDC request, the bigger the session size. In the case of USAJOBS, storing this data in the session increased the length of the string that was sent to KMS after 2FA from 2948 to 4128. There were exactly 2 SAML requests during the outage, so they played an insignificant role. This most likely affected every single USAJOBS session, which accounted for 74% of requests during that time. I don't believe TTP was affected. The rest of the requests (4% of total requests) came from 3 other SPs that I haven't looked into yet. Note that this only solves the problem for LOA1 OIDC requests (as they are currently made). SAML requests remain much larger than 4096 bytes. In general, this KMS limit problem can be solved by one or more of these solutions, which we have considered in the past: - Storing the SP requests in the DB instead of the session - Only encrypting the info that needs to be encrypted, as opposed to the entire session --- app/controllers/openid_connect/authorization_controller.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/controllers/openid_connect/authorization_controller.rb b/app/controllers/openid_connect/authorization_controller.rb index 3d499416659..c0be8f8155c 100644 --- a/app/controllers/openid_connect/authorization_controller.rb +++ b/app/controllers/openid_connect/authorization_controller.rb @@ -62,8 +62,6 @@ def identity_needs_verification? end def build_authorize_form_from_params - user_session[:openid_auth_request] = authorization_params if user_session - @authorize_form = OpenidConnectAuthorizeForm.new(authorization_params) @authorize_decorator = OpenidConnectAuthorizeDecorator.new( From c4ed00d41cc18ca09ef4380bcec2f7696cc24b76 Mon Sep 17 00:00:00 2001 From: James G Smith Date: Mon, 6 Aug 2018 12:21:48 -0400 Subject: [PATCH 13/25] Catch sending too much to kms (#2411) **Why**: We can only send 4k of data to KMS for encryption. We need to make sure we don't exceed that regardless of which method we use so we know we can use KMS without errors. **How**: Raise an argument error regardless of the encyption method. --- app/services/encryption/kms_client.rb | 45 ++++- spec/services/encryption/kms_client_spec.rb | 184 ++++++++++++++------ spec/support/aws_kms_client.rb | 15 ++ 3 files changed, 191 insertions(+), 53 deletions(-) diff --git a/app/services/encryption/kms_client.rb b/app/services/encryption/kms_client.rb index abd27bc6ebb..0aaaffaff9a 100644 --- a/app/services/encryption/kms_client.rb +++ b/app/services/encryption/kms_client.rb @@ -1,3 +1,5 @@ +require 'base64' + module Encryption class KmsClient include Encodable @@ -27,16 +29,51 @@ def use_kms?(ciphertext) end def encrypt_kms(plaintext) - ciphertext_blob = aws_client.encrypt( + if plaintext.bytesize > 4096 + encrypt_in_chunks(plaintext) + else + KEY_TYPE[:KMS] + encrypt_raw_kms(plaintext) + end + end + + # chunk plaintext into ~4096 byte chunks, but not less than 1024 bytes in a chunk if chunking. + # we do this by counting how many chunks we have and adding one. + # :reek:FeatureEnvy + def encrypt_in_chunks(plaintext) + plain_size = plaintext.bytesize + number_chunks = plain_size / 4096 + chunk_size = plain_size / (1 + number_chunks) + ciphertext_set = plaintext.scan(/.{1,#{chunk_size}}/m).map(&method(:encrypt_raw_kms)) + KEY_TYPE[:KMS] + ciphertext_set.map { |chunk| Base64.strict_encode64(chunk) }.to_json + end + + def encrypt_raw_kms(plaintext) + raise ArgumentError, 'kms plaintext exceeds 4096 bytes' if plaintext.bytesize > 4096 + aws_client.encrypt( key_id: Figaro.env.aws_kms_key_id, plaintext: plaintext ).ciphertext_blob - KEY_TYPE[:KMS] + ciphertext_blob end + # :reek:DuplicateMethodCall def decrypt_kms(ciphertext) - kms_input = ciphertext.sub(KEY_TYPE[:KMS], '') - aws_client.decrypt(ciphertext_blob: kms_input).plaintext + raw_ciphertext = ciphertext.sub(KEY_TYPE[:KMS], '') + if raw_ciphertext[0] == '[' && raw_ciphertext[-1] == ']' + decrypt_chunked_kms(raw_ciphertext) + else + decrypt_raw_kms(raw_ciphertext) + end + end + + def decrypt_chunked_kms(raw_ciphertext) + ciphertext_set = JSON.parse(raw_ciphertext).map { |chunk| Base64.strict_decode64(chunk) } + ciphertext_set.map(&method(:decrypt_raw_kms)).join('') + rescue JSON::ParserError, ArgumentError + decrypt_raw_kms(raw_ciphertext) + end + + def decrypt_raw_kms(raw_ciphertext) + aws_client.decrypt(ciphertext_blob: raw_ciphertext).plaintext rescue Aws::KMS::Errors::InvalidCiphertextException raise EncryptionError, 'Aws::KMS::Errors::InvalidCiphertextException' end diff --git a/spec/services/encryption/kms_client_spec.rb b/spec/services/encryption/kms_client_spec.rb index 9d52eaa79ea..94892355de6 100644 --- a/spec/services/encryption/kms_client_spec.rb +++ b/spec/services/encryption/kms_client_spec.rb @@ -4,79 +4,165 @@ let(:password_pepper) { '1' * 32 } let(:local_plaintext) { 'local plaintext' } let(:local_ciphertext) { 'local ciphertext' } - let(:kms_plaintext) { 'kms plaintext' } - let(:kms_ciphertext) { 'kms ciphertext' } - let(:kms_enabled) { true } - - before do - allow(Figaro.env).to receive(:password_pepper).and_return(password_pepper) - - encryptor = Encryption::Encryptors::AesEncryptor.new - allow(encryptor).to receive(:encrypt). - with(local_plaintext, password_pepper). - and_return(local_ciphertext) - allow(encryptor).to receive(:decrypt). - with(local_ciphertext, password_pepper). - and_return(local_plaintext) - allow(Encryption::Encryptors::AesEncryptor).to receive(:new).and_return(encryptor) - - stub_aws_kms_client(kms_plaintext, kms_ciphertext) - allow(FeatureManagement).to receive(:use_kms?).and_return(kms_enabled) - end - describe '#encrypt' do - context 'with KMS enabled' do - it 'uses KMS to encrypt the plaintext' do - result = subject.encrypt(kms_plaintext) + context 'with kms plaintext less than 4k' do + let(:kms_plaintext) { 'kms plaintext' } + let(:kms_ciphertext) { 'kms ciphertext' } + let(:kms_enabled) { true } + + before do + allow(Figaro.env).to receive(:password_pepper).and_return(password_pepper) + + encryptor = Encryption::Encryptors::AesEncryptor.new + allow(encryptor).to receive(:encrypt). + with(local_plaintext, password_pepper). + and_return(local_ciphertext) + allow(encryptor).to receive(:decrypt). + with(local_ciphertext, password_pepper). + and_return(local_plaintext) + allow(Encryption::Encryptors::AesEncryptor).to receive(:new).and_return(encryptor) + + stub_aws_kms_client(kms_plaintext, kms_ciphertext) + allow(FeatureManagement).to receive(:use_kms?).and_return(kms_enabled) + end + + describe '#encrypt' do + context 'with KMS enabled' do + it 'uses KMS to encrypt the plaintext' do + result = subject.encrypt(kms_plaintext) + + expect(result).to eq('KMSx' + kms_ciphertext) + end + end + + context 'without KMS enabled' do + let(:kms_enabled) { false } - expect(result).to eq('KMSx' + kms_ciphertext) + it 'uses the password pepper to encrypt the plaintext and signs it' do + result = subject.encrypt(local_plaintext) + + expect(result).to eq(local_ciphertext) + end end end - context 'without KMS enabled' do - let(:kms_enabled) { false } + describe '#decrypt' do + context 'with KMS enabled' do + it 'uses KMS to decrypt a ciphertext encrypted with KMS' do + result = subject.decrypt('KMSx' + kms_ciphertext) + + expect(result).to eq(kms_plaintext) + end - it 'uses the password pepper to encrypt the plaintext and signs it' do - result = subject.encrypt(local_plaintext) + it 'uses the password pepper to decrypt a legacy ciphertext encrypted without KMS' do + result = subject.decrypt(local_ciphertext) - expect(result).to eq(local_ciphertext) + expect(result).to eq(local_plaintext) + end + end + + context 'without KMS enabled' do + let(:kms_enabled) { false } + + it 'uses the password pepper to decrypt a ciphertext' do + result = subject.decrypt(local_ciphertext) + + expect(result).to eq(local_plaintext) + end + end + end + + describe '#looks_like_kms?' do + it 'returns true for kms encrypted data' do + expect(subject.class.looks_like_kms?('KMSx' + kms_ciphertext)).to eq(true) + end + + it 'returns false for non kms encrypted data' do + expect(subject.class.looks_like_kms?('abcdef.' + kms_ciphertext)).to eq(false) end end end - describe '#decrypt' do - context 'with KMS enabled' do - it 'uses KMS to decrypt a ciphertext encrypted with KMS' do - result = subject.decrypt('KMSx' + kms_ciphertext) + context 'with kms plaintext greater than 4k' do + let(:long_kms_plaintext) { SecureRandom.random_bytes(4096 * 1.8) } + let(:long_kms_plaintext_bytesize) { long_kms_plaintext.bytesize } + let(:long_kms_plaintext_chunksize) { long_kms_plaintext_bytesize / 2 } + let(:kms_ciphertext) { %w[chunk1 chunk2].map { |c| Base64.strict_encode64(c) }.to_json } + let(:kms_enabled) { true } + + before do + allow(Figaro.env).to receive(:password_pepper).and_return(password_pepper) + + encryptor = Encryption::Encryptors::AesEncryptor.new + allow(encryptor).to receive(:encrypt). + with(local_plaintext, password_pepper). + and_return(local_ciphertext) + allow(encryptor).to receive(:decrypt). + with(local_ciphertext, password_pepper). + and_return(local_plaintext) + allow(Encryption::Encryptors::AesEncryptor).to receive(:new).and_return(encryptor) + + stub_mapped_aws_kms_client( + long_kms_plaintext[0..long_kms_plaintext_chunksize - 1] => 'chunk1', + long_kms_plaintext[long_kms_plaintext_chunksize..-1] => 'chunk2' + ) + allow(FeatureManagement).to receive(:use_kms?).and_return(kms_enabled) + end + + describe '#encrypt' do + context 'with KMS enabled' do + it 'uses KMS to encrypt the plaintext' do + result = subject.encrypt(long_kms_plaintext) - expect(result).to eq(kms_plaintext) + expect(result).to eq('KMSx' + kms_ciphertext) + end end - it 'uses the password pepper to decrypt a legacy ciphertext encrypted without KMS' do - result = subject.decrypt(local_ciphertext) + context 'without KMS enabled' do + let(:kms_enabled) { false } - expect(result).to eq(local_plaintext) + it 'uses the password pepper to encrypt the plaintext and signs it' do + result = subject.encrypt(local_plaintext) + + expect(result).to eq(local_ciphertext) + end end end - context 'without KMS enabled' do - let(:kms_enabled) { false } + describe '#decrypt' do + context 'with KMS enabled' do + it 'uses KMS to decrypt a ciphertext encrypted with KMS' do + result = subject.decrypt('KMSx' + kms_ciphertext) + + expect(result).to eq(long_kms_plaintext) + end - it 'uses the password pepper to decrypt a ciphertext' do - result = subject.decrypt(local_ciphertext) + it 'uses the password pepper to decrypt a legacy ciphertext encrypted without KMS' do + result = subject.decrypt(local_ciphertext) - expect(result).to eq(local_plaintext) + expect(result).to eq(local_plaintext) + end end - end - end - describe '#looks_like_kms?' do - it 'returns true for kms encrypted data' do - expect(subject.class.looks_like_kms?('KMSx' + kms_ciphertext)).to eq(true) + context 'without KMS enabled' do + let(:kms_enabled) { false } + + it 'uses the password pepper to decrypt a ciphertext' do + result = subject.decrypt(local_ciphertext) + + expect(result).to eq(local_plaintext) + end + end end - it 'returns false for non kms encrypted data' do - expect(subject.class.looks_like_kms?('abcdef.' + kms_ciphertext)).to eq(false) + describe '#looks_like_kms?' do + it 'returns true for kms encrypted data' do + expect(subject.class.looks_like_kms?('KMSx' + kms_ciphertext)).to eq(true) + end + + it 'returns false for non kms encrypted data' do + expect(subject.class.looks_like_kms?('abcdef.' + kms_ciphertext)).to eq(false) + end end end end diff --git a/spec/support/aws_kms_client.rb b/spec/support/aws_kms_client.rb index 9eb5ad119f6..ab320b4236f 100644 --- a/spec/support/aws_kms_client.rb +++ b/spec/support/aws_kms_client.rb @@ -10,6 +10,21 @@ def stub_aws_kms_client(random_key = random_str, ciphered_key = random_str) [random_key, ciphered_key] end + def stub_mapped_aws_kms_client(forward = {}) + reverse = forward.invert + aws_key_id = Figaro.env.aws_kms_key_id + Aws.config[:kms] = { + stub_responses: { + encrypt: lambda { |context| + { ciphertext_blob: forward[context.params[:plaintext]], key_id: aws_key_id } + }, + decrypt: lambda { |context| + { plaintext: reverse[context.params[:ciphertext_blob]], key_id: aws_key_id } + }, + }, + } + end + def stub_aws_kms_client_invalid_ciphertext(ciphered_key = random_str) aws_key_id = Figaro.env.aws_kms_key_id Aws.config[:kms] = { From 7ec05e01a02c69b9f895cc7c99932f231af10dcd Mon Sep 17 00:00:00 2001 From: Steve Urciuoli Date: Tue, 7 Aug 2018 14:41:12 -0400 Subject: [PATCH 14/25] LG-533 Add a New Redirect URI for the DOE - Fossil Energy SP **Why**: Per request from the agency. **How**: Update service_providers.yml --- config/service_providers.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/config/service_providers.yml b/config/service_providers.yml index 4e2f2fe4bcc..d9d1a4da731 100644 --- a/config/service_providers.yml +++ b/config/service_providers.yml @@ -706,6 +706,7 @@ production: - 'https://fossil.energy.gov/fergas-fe/#/certify_redirect' - 'https://fossil.energy.gov/fergas-fe/#/login' - 'https://fossil.energy.gov/fergas-fe/#/login_redirect' + - 'https://fossil.energy.gov/fergas-fe/#/notice/13' - 'https://fossil.energy.gov/fergas-fe/#/notice/15' restrict_to_deploy_env: 'prod' From 37242df2fb7a23e3acee8c6a44b774f0b779cc0d Mon Sep 17 00:00:00 2001 From: James Smith Date: Tue, 7 Aug 2018 14:26:48 -0400 Subject: [PATCH 15/25] [LG-499] Rake task copies phone info to new table **Why**: We want to make sure all phone configurations are present in the new table before we start reading data from the table. **How**: We use a rake task that processes users in batches to make sure a phone configuration row exists for the user. --- .../populate_phone_configurations_table.rb | 28 ++++++++++++ lib/tasks/migrate_phone_configurations.rake | 7 +++ ...opulate_phone_configurations_table_spec.rb | 43 +++++++++++++++++++ 3 files changed, 78 insertions(+) create mode 100644 app/services/populate_phone_configurations_table.rb create mode 100644 lib/tasks/migrate_phone_configurations.rake create mode 100644 spec/services/populate_phone_configurations_table_spec.rb diff --git a/app/services/populate_phone_configurations_table.rb b/app/services/populate_phone_configurations_table.rb new file mode 100644 index 00000000000..672e7aa85d2 --- /dev/null +++ b/app/services/populate_phone_configurations_table.rb @@ -0,0 +1,28 @@ +class PopulatePhoneConfigurationsTable + def call + # we don't have a uniqueness constraint in the database to let us blindly insert + # everything in a single SQL statement. So we have to load by batches and copy + # over. Much slower, but doesn't duplicate information. + User.in_batches(of: 1000) { |relation| process_batch(relation) } + end + + private + + # :reek:FeatureEnvy + def process_batch(relation) + User.transaction do + relation.each do |user| + next if user.phone_configuration.present? || user.phone.blank? + user.create_phone_configuration(phone_info_for_user(user)) + end + end + end + + def phone_info_for_user(user) + { + phone: user.phone, + confirmed_at: user.phone_confirmed_at, + delivery_preference: user.otp_delivery_preference, + } + end +end diff --git a/lib/tasks/migrate_phone_configurations.rake b/lib/tasks/migrate_phone_configurations.rake new file mode 100644 index 00000000000..f857e0e96f2 --- /dev/null +++ b/lib/tasks/migrate_phone_configurations.rake @@ -0,0 +1,7 @@ +namespace :adhoc do + desc 'Copy phone configurations to the new table' + task populate_phone_configurations: :environment do + Rails.logger = Logger.new(STDOUT) + PopulatePhoneConfigurationsTable.new.call + end +end diff --git a/spec/services/populate_phone_configurations_table_spec.rb b/spec/services/populate_phone_configurations_table_spec.rb new file mode 100644 index 00000000000..da4f3e9c384 --- /dev/null +++ b/spec/services/populate_phone_configurations_table_spec.rb @@ -0,0 +1,43 @@ +require 'rails_helper' + +describe PopulatePhoneConfigurationsTable do + let(:subject) { described_class.new } + + describe '#call' do + context 'a user with no phone' do + let!(:user) { create(:user) } + + it 'migrates nothing' do + subject.call + expect(user.reload.phone_configuration).to be_nil + end + end + + context 'a user with a phone' do + let!(:user) { create(:user, :with_phone) } + + context 'and no phone_configuration entry' do + before(:each) do + user.phone_configuration.delete + user.reload + end + + it 'migrates the phone' do + subject.call + configuration = user.reload.phone_configuration + expect(configuration.phone).to eq user.phone + expect(configuration.confirmed_at).to eq user.phone_confirmed_at + expect(configuration.delivery_preference).to eq user.otp_delivery_preference + end + end + + context 'and an existing phone_configuration entry' do + it 'adds no new rows' do + expect(PhoneConfiguration.where(user_id: user.id).count).to eq 1 + subject.call + expect(PhoneConfiguration.where(user_id: user.id).count).to eq 1 + end + end + end + end +end From 5f1cf55d1878d2860875e265689d9097c2f79fdf Mon Sep 17 00:00:00 2001 From: Moncef Belyamani Date: Fri, 10 Aug 2018 09:13:47 -0400 Subject: [PATCH 16/25] Update aws-sdk-kms from 1.6.0 to 1.7.0 --- Gemfile.lock | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index d2cfe118289..ff25e2ebd52 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -138,13 +138,13 @@ GEM arel (8.0.0) ast (2.4.0) aws-eventstream (1.0.1) - aws-partitions (1.96.0) - aws-sdk-core (3.22.1) + aws-partitions (1.97.0) + aws-sdk-core (3.24.0) aws-eventstream (~> 1.0) aws-partitions (~> 1.0) aws-sigv4 (~> 1.0) jmespath (~> 1.0) - aws-sdk-kms (1.6.0) + aws-sdk-kms (1.7.0) aws-sdk-core (~> 3) aws-sigv4 (~> 1.0) aws-sdk-s3 (1.17.0) From 6aaef474426fd67cb68e0d54dd805cfddea5484a Mon Sep 17 00:00:00 2001 From: Moncef Belyamani Date: Fri, 10 Aug 2018 09:13:59 -0400 Subject: [PATCH 17/25] Update i18n-tasks from 0.9.21 to 0.9.23 --- Gemfile.lock | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index ff25e2ebd52..91990694d3e 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -217,6 +217,7 @@ GEM daemons (1.2.4) database_cleaner (1.7.0) debug_inspector (0.0.3) + deepl-rb (2.1.0) derailed (0.1.0) derailed_benchmarks derailed_benchmarks (1.3.4) @@ -308,7 +309,7 @@ GEM hashdiff (0.3.7) hashie (3.5.7) heapy (0.1.3) - highline (1.7.10) + highline (2.0.0) hiredis (0.6.1) htmlentities (4.3.4) http_accept_language (2.1.1) @@ -317,11 +318,12 @@ GEM httpi (2.4.3) rack socksify - i18n (1.0.1) + i18n (1.1.0) concurrent-ruby (~> 1.0) - i18n-tasks (0.9.21) + i18n-tasks (0.9.23) activesupport (>= 4.0.2) ast (>= 2.1.0) + deepl-rb (>= 2.1.0) easy_translate (>= 0.5.1) erubi highline (>= 1.7.3) From fab2eb5bd8a65937af4aabf8e4f6e325d65565bf Mon Sep 17 00:00:00 2001 From: Moncef Belyamani Date: Fri, 10 Aug 2018 09:14:19 -0400 Subject: [PATCH 18/25] Update phonelib from 0.6.23 to 0.6.24 --- Gemfile.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile.lock b/Gemfile.lock index 91990694d3e..f897e87aedf 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -392,7 +392,7 @@ GEM parser (2.5.1.2) ast (~> 2.4.0) pg (1.0.0) - phonelib (0.6.23) + phonelib (0.6.24) pkcs11 (0.2.7) powerpack (0.1.2) premailer (1.11.1) From 1014b682d7d2ebce0ae343f6e35c148178563619 Mon Sep 17 00:00:00 2001 From: Moncef Belyamani Date: Fri, 10 Aug 2018 09:14:29 -0400 Subject: [PATCH 19/25] Update recaptcha from 4.8.0 to 4.11.1 --- Gemfile.lock | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index f897e87aedf..f491c3a4e59 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -336,7 +336,7 @@ GEM io-like (0.3.0) jaro_winkler (1.5.1) jmespath (1.4.0) - json (1.8.6) + json (2.1.0) json-jwt (1.9.4) activesupport aes_key_wrap @@ -468,7 +468,7 @@ GEM readthis (2.2.0) connection_pool (~> 2.1) redis (>= 3.0, < 5.0) - recaptcha (4.8.0) + recaptcha (4.11.1) json redis (3.3.5) reek (4.8.1) From d09d6e650028f40a3d83a7ea58bf1f7d517e7d52 Mon Sep 17 00:00:00 2001 From: Moncef Belyamani Date: Fri, 10 Aug 2018 09:15:04 -0400 Subject: [PATCH 20/25] Update ruby-progressbar from 1.9.0 to 1.10.0 --- Gemfile.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile.lock b/Gemfile.lock index f491c3a4e59..b4f274ce542 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -514,7 +514,7 @@ GEM ruby-progressbar (~> 1.7) unicode-display_width (~> 1.0, >= 1.0.1) ruby-graphviz (1.2.3) - ruby-progressbar (1.9.0) + ruby-progressbar (1.10.0) ruby-saml (1.8.0) nokogiri (>= 1.5.10) ruby_dep (1.5.0) From b73f751af639e5d007a6b53aadcb664c08d89eec Mon Sep 17 00:00:00 2001 From: Moncef Belyamani Date: Fri, 10 Aug 2018 09:15:15 -0400 Subject: [PATCH 21/25] Update sidekiq from 5.1.3 to 5.2.1 --- Gemfile.lock | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index b4f274ce542..964e8c27975 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -208,7 +208,7 @@ GEM descendants_tracker (~> 0.0.1) colorize (0.8.1) concurrent-ruby (1.0.5) - connection_pool (2.2.1) + connection_pool (2.2.2) crack (0.4.3) safe_yaml (~> 1.0.0) crass (1.0.4) @@ -553,9 +553,8 @@ GEM shellany (0.0.1) shoulda-matchers (3.1.2) activesupport (>= 4.0.0) - sidekiq (5.1.3) - concurrent-ruby (~> 1.0) - connection_pool (~> 2.2, >= 2.2.0) + sidekiq (5.2.1) + connection_pool (~> 2.2, >= 2.2.2) rack-protection (>= 1.5.0) redis (>= 3.3.5, < 5) simple_form (4.0.1) From 2384b75fe9d2cc7e4524a85055db90a050d251ef Mon Sep 17 00:00:00 2001 From: Moncef Belyamani Date: Fri, 10 Aug 2018 09:15:25 -0400 Subject: [PATCH 22/25] Update twilio-ruby from 5.11.1 to 5.12.1 --- Gemfile.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile.lock b/Gemfile.lock index 964e8c27975..3e1572c5a97 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -607,7 +607,7 @@ GEM thread_safe (0.3.6) tilt (2.0.8) timecop (0.9.1) - twilio-ruby (5.11.1) + twilio-ruby (5.12.1) faraday (~> 0.9) jwt (>= 1.5, <= 2.5) nokogiri (>= 1.6, < 2.0) From 37cb5408bfacdfe72f6fa292631964a77b72ef69 Mon Sep 17 00:00:00 2001 From: Moncef Belyamani Date: Fri, 10 Aug 2018 11:57:36 -0400 Subject: [PATCH 23/25] LG-454 Refactor AccountReset::CancelController (#2385) * LG-454 Refactor AccountReset::CancelController **Why**: It was not adhering to our controller design convention, and was breaking the analytics format contract. **How**: - Create a `AccountReset::Cancel` class that validates the token, and if successful, notifies the user via email and SMS if the user has a phone, then updates the user's account reset request to reflect that it was cancelled. - Rename `cancel` to `create` in the controller to adhere to our CRUD-only guideline. Benefits: - The controller is a lot leaner and cleaner. - It keeps all the actions in one place, as opposed to having some actions in the controller (the notifications) and some in `AccountResetService` (updating the request in the DB). - It respects the Analytics API, and logs all error scenarios, including missing tokens. --- .../account_reset/cancel_controller.rb | 26 ++-- app/services/account_reset/cancel.rb | 67 +++++++++++ app/services/account_reset_service.rb | 9 -- config/locales/errors/en.yml | 3 + config/locales/errors/es.yml | 3 + config/locales/errors/fr.yml | 3 + config/routes.rb | 2 +- .../account_reset/cancel_controller_spec.rb | 83 +++++++------ .../account_reset/cancel_request_spec.rb | 54 +++++++++ spec/services/account_reset/cancel_spec.rb | 112 ++++++++++++++++++ spec/services/account_reset_service_spec.rb | 35 +----- spec/support/account_reset_helper.rb | 12 ++ 12 files changed, 316 insertions(+), 93 deletions(-) create mode 100644 app/services/account_reset/cancel.rb create mode 100644 spec/features/account_reset/cancel_request_spec.rb create mode 100644 spec/services/account_reset/cancel_spec.rb create mode 100644 spec/support/account_reset_helper.rb diff --git a/app/controllers/account_reset/cancel_controller.rb b/app/controllers/account_reset/cancel_controller.rb index 8cbe8659e7f..44f66f8cb90 100644 --- a/app/controllers/account_reset/cancel_controller.rb +++ b/app/controllers/account_reset/cancel_controller.rb @@ -1,30 +1,20 @@ module AccountReset class CancelController < ApplicationController - def cancel - account_reset = AccountResetService.cancel_request(params[:token]) - if account_reset - handle_success(account_reset.user) - else - handle_failure - end + def create + result = AccountReset::Cancel.new(params[:token]).call + + analytics.track_event(Analytics::ACCOUNT_RESET, result.to_h) + + handle_success if result.success? + redirect_to root_url end private - def handle_success(user) - analytics.track_event(Analytics::ACCOUNT_RESET, - event: :cancel, token_valid: true, user_id: user.uuid) + def handle_success sign_out if current_user - UserMailer.account_reset_cancel(user.email).deliver_later - phone = user.phone - SmsAccountResetCancellationNotifierJob.perform_now(phone: phone) if phone.present? flash[:success] = t('devise.two_factor_authentication.account_reset.successful_cancel') end - - def handle_failure - return if params[:token].blank? - analytics.track_event(Analytics::ACCOUNT_RESET, event: :cancel, token_valid: false) - end end end diff --git a/app/services/account_reset/cancel.rb b/app/services/account_reset/cancel.rb new file mode 100644 index 00000000000..77bba914934 --- /dev/null +++ b/app/services/account_reset/cancel.rb @@ -0,0 +1,67 @@ +module AccountReset + class Cancel + include ActiveModel::Model + + validates :token, presence: { message: I18n.t('errors.account_reset.cancel_token_missing') } + validate :valid_token + + def initialize(token) + @token = token + end + + def call + @success = valid? + + if success + notify_user_via_email_of_account_reset_cancellation + notify_user_via_phone_of_account_reset_cancellation if phone.present? + update_account_reset_request + end + + FormResponse.new(success: success, errors: errors.messages, extra: extra_analytics_attributes) + end + + private + + attr_reader :success, :token + + def valid_token + return if account_reset_request + + errors.add(:token, I18n.t('errors.account_reset.cancel_token_invalid')) if token + end + + def notify_user_via_email_of_account_reset_cancellation + UserMailer.account_reset_cancel(user.email).deliver_later + end + + def notify_user_via_phone_of_account_reset_cancellation + SmsAccountResetCancellationNotifierJob.perform_now(phone: phone) + end + + def update_account_reset_request + account_reset_request.update!(cancelled_at: Time.zone.now, + request_token: nil, + granted_token: nil) + end + + def account_reset_request + @account_reset_request ||= AccountResetRequest.find_by(request_token: token) + end + + def user + account_reset_request&.user || AnonymousUser.new + end + + def phone + user.phone + end + + def extra_analytics_attributes + { + event: 'cancel', + user_id: user.uuid, + } + end + end +end diff --git a/app/services/account_reset_service.rb b/app/services/account_reset_service.rb index 901bdc5ee18..43bc7d4f638 100644 --- a/app/services/account_reset_service.rb +++ b/app/services/account_reset_service.rb @@ -12,15 +12,6 @@ def create_request granted_token: nil) end - def self.cancel_request(token) - account_reset = token.blank? ? nil : AccountResetRequest.find_by(request_token: token) - return false unless account_reset - account_reset.update(cancelled_at: Time.zone.now, - request_token: nil, - granted_token: nil) - account_reset - end - def self.report_fraud(token) account_reset = token.blank? ? nil : AccountResetRequest.find_by(request_token: token) return false unless account_reset diff --git a/config/locales/errors/en.yml b/config/locales/errors/en.yml index 45f350fd4ee..0ec588520c6 100644 --- a/config/locales/errors/en.yml +++ b/config/locales/errors/en.yml @@ -1,6 +1,9 @@ --- en: errors: + account_reset: + cancel_token_invalid: cancel token is invalid + cancel_token_missing: cancel token is missing confirm_password_incorrect: Incorrect password. invalid_authenticity_token: Oops, something went wrong. Please try again. invalid_totp: Invalid code. Please try again. diff --git a/config/locales/errors/es.yml b/config/locales/errors/es.yml index 70f7cd81f22..303a54eaa22 100644 --- a/config/locales/errors/es.yml +++ b/config/locales/errors/es.yml @@ -1,6 +1,9 @@ --- es: errors: + account_reset: + cancel_token_invalid: NOT TRANSLATED YET + cancel_token_missing: NOT TRANSLATED YET confirm_password_incorrect: La contraseña es incorrecta. invalid_authenticity_token: "¡Oops! Algo salió mal. Inténtelo de nuevo." invalid_totp: El código es inválido. Vuelva a intentarlo. diff --git a/config/locales/errors/fr.yml b/config/locales/errors/fr.yml index b6615b07d3b..038d0fd5486 100644 --- a/config/locales/errors/fr.yml +++ b/config/locales/errors/fr.yml @@ -1,6 +1,9 @@ --- fr: errors: + account_reset: + cancel_token_invalid: NOT TRANSLATED YET + cancel_token_missing: NOT TRANSLATED YET confirm_password_incorrect: Mot de passe incorrect. invalid_authenticity_token: Oups, une erreur s'est produite. Veuillez essayer de nouveau. diff --git a/config/routes.rb b/config/routes.rb index de8be2635cd..0317f73f01d 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -58,7 +58,7 @@ get '/account_reset/request' => 'account_reset/request#show' post '/account_reset/request' => 'account_reset/request#create' - get '/account_reset/cancel' => 'account_reset/cancel#cancel' + get '/account_reset/cancel' => 'account_reset/cancel#create' get '/account_reset/report_fraud' => 'account_reset/report_fraud#update' get '/account_reset/confirm_request' => 'account_reset/confirm_request#show' get '/account_reset/delete_account' => 'account_reset/delete_account#show' diff --git a/spec/controllers/account_reset/cancel_controller_spec.rb b/spec/controllers/account_reset/cancel_controller_spec.rb index 529b375fb76..a3b2a5c3777 100644 --- a/spec/controllers/account_reset/cancel_controller_spec.rb +++ b/spec/controllers/account_reset/cancel_controller_spec.rb @@ -1,67 +1,84 @@ require 'rails_helper' describe AccountReset::CancelController do - let(:user) { create(:user, :signed_up, phone: '+1 (703) 555-0000') } + include AccountResetHelper + + let(:user) { create(:user, :signed_up) } before do TwilioService::Utils.telephony_service = FakeSms end - describe '#cancel' do + describe '#create' do it 'logs a good token to the analytics' do - AccountResetService.new(user).create_request + token = create_account_reset_request_for(user) stub_analytics + analytics_hash = { + success: true, + errors: {}, + event: 'cancel', + user_id: user.uuid, + } + expect(@analytics).to receive(:track_event). - with(Analytics::ACCOUNT_RESET, - event: :cancel, token_valid: true, user_id: user.uuid) + with(Analytics::ACCOUNT_RESET, analytics_hash) - post :cancel, params: { token: AccountResetRequest.all[0].request_token } + post :create, params: { token: token } end it 'logs a bad token to the analytics' do stub_analytics + analytics_hash = { + success: false, + errors: { token: [t('errors.account_reset.cancel_token_invalid')] }, + event: 'cancel', + user_id: 'anonymous-uuid', + } + expect(@analytics).to receive(:track_event). - with(Analytics::ACCOUNT_RESET, event: :cancel, token_valid: false) + with(Analytics::ACCOUNT_RESET, analytics_hash) - post :cancel, params: { token: 'FOO' } + post :create, params: { token: 'FOO' } end - it 'redirects to the root' do - post :cancel - expect(response).to redirect_to root_url - end + it 'logs a missing token to the analytics' do + stub_analytics + analytics_hash = { + success: false, + errors: { token: [t('errors.account_reset.cancel_token_missing')] }, + event: 'cancel', + user_id: 'anonymous-uuid', + } - it 'sends an SMS if there is a phone' do - AccountResetService.new(user).create_request - allow(SmsAccountResetCancellationNotifierJob).to receive(:perform_now) + expect(@analytics).to receive(:track_event). + with(Analytics::ACCOUNT_RESET, analytics_hash) - post :cancel, params: { token: AccountResetRequest.all[0].request_token } + post :create + end - expect(SmsAccountResetCancellationNotifierJob).to have_received(:perform_now).with( - phone: user.phone - ) + it 'redirects to the root without a flash message when the token is missing or invalid' do + post :create + expect(response).to redirect_to root_url end - it 'does not send an SMS if there is no phone' do - AccountResetService.new(user).create_request - allow(SmsAccountResetCancellationNotifierJob).to receive(:perform_now) - user.phone = nil - user.save! - user.phone_configuration.destroy + it 'redirects to the root with a flash message when the token is valid' do + token = create_account_reset_request_for(user) - post :cancel, params: { token: AccountResetRequest.all[0].request_token } + post :create, params: { token: token } - expect(SmsAccountResetCancellationNotifierJob).to_not have_received(:perform_now) + expect(flash[:success]). + to eq t('devise.two_factor_authentication.account_reset.successful_cancel') + expect(response).to redirect_to root_url end - it 'sends an email' do - AccountResetService.new(user).create_request + it 'signs the user out if signed in and if the token is valid' do + stub_sign_in(user) + + token = create_account_reset_request_for(user) - @mailer = instance_double(ActionMailer::MessageDelivery, deliver_later: true) - allow(UserMailer).to receive(:account_reset_cancel).with(user.email). - and_return(@mailer) + expect(controller).to receive(:sign_out) - post :cancel, params: { token: AccountResetRequest.all[0].request_token } + post :create, params: { token: token } end end end diff --git a/spec/features/account_reset/cancel_request_spec.rb b/spec/features/account_reset/cancel_request_spec.rb new file mode 100644 index 00000000000..d2524fac848 --- /dev/null +++ b/spec/features/account_reset/cancel_request_spec.rb @@ -0,0 +1,54 @@ +require 'rails_helper' + +describe 'Account Reset Request: Cancellation' do + context 'user cancels right away from the first email' do + it 'cancels the request and does not delete the user', email: true do + TwilioService::Utils.telephony_service = FakeSms + user = create(:user, :signed_up) + signin(user.email, user.password) + click_link t('two_factor_authentication.login_options_link_text') + click_link t('devise.two_factor_authentication.account_reset.link') + click_button t('account_reset.request.yes_continue') + open_last_email + click_email_link_matching(/cancel\?token/) + + expect(page).to have_current_path new_user_session_path + expect(page). + to have_content t('devise.two_factor_authentication.account_reset.successful_cancel') + + signin(user.email, user.password) + + expect(page). + to have_current_path login_two_factor_path(otp_delivery_preference: 'sms', reauthn: 'false') + end + end + + context 'user cancels from the second email after the request has been granted' do + it 'cancels the request and does not delete the user', email: true do + TwilioService::Utils.telephony_service = FakeSms + user = create(:user, :signed_up) + signin(user.email, user.password) + click_link t('two_factor_authentication.login_options_link_text') + click_link t('devise.two_factor_authentication.account_reset.link') + click_button t('account_reset.request.yes_continue') + reset_email + + Timecop.travel(Time.zone.now + 2.days) do + AccountResetService.grant_tokens_and_send_notifications + open_last_email + click_email_link_matching(/cancel\?token/) + + expect(page).to have_current_path new_user_session_path + expect(page). + to have_content t('devise.two_factor_authentication.account_reset.successful_cancel') + + signin(user.email, user.password) + + expect(page). + to have_current_path( + login_two_factor_path(otp_delivery_preference: 'sms', reauthn: 'false') + ) + end + end + end +end diff --git a/spec/services/account_reset/cancel_spec.rb b/spec/services/account_reset/cancel_spec.rb new file mode 100644 index 00000000000..f59132af926 --- /dev/null +++ b/spec/services/account_reset/cancel_spec.rb @@ -0,0 +1,112 @@ +require 'rails_helper' + +describe AccountReset::Cancel do + include AccountResetHelper + + let(:user) { create(:user, :signed_up) } + + before { TwilioService::Utils.telephony_service = FakeSms } + + it 'validates presence of token' do + request = AccountReset::Cancel.new(nil).call + + expect(request.success?).to eq false + end + + it 'validates validity of token' do + request = AccountReset::Cancel.new('foo').call + + expect(request.success?).to eq false + end + + context 'when the token is valid' do + context 'when the user has a phone enabled for SMS' do + it 'notifies the user via SMS of the account reset cancellation' do + token = create_account_reset_request_for(user) + allow(SmsAccountResetCancellationNotifierJob).to receive(:perform_now) + + AccountReset::Cancel.new(token).call + + expect(SmsAccountResetCancellationNotifierJob). + to have_received(:perform_now).with(phone: user.phone) + end + end + + context 'when the user does not have a phone enabled for SMS' do + it 'does not notify the user via SMS' do + token = create_account_reset_request_for(user) + allow(SmsAccountResetCancellationNotifierJob).to receive(:perform_now) + user.update!(phone: nil) + + AccountReset::Cancel.new(token).call + + expect(SmsAccountResetCancellationNotifierJob).to_not have_received(:perform_now) + end + end + + it 'notifies the user via email of the account reset cancellation' do + token = create_account_reset_request_for(user) + + @mailer = instance_double(ActionMailer::MessageDelivery, deliver_later: true) + expect(UserMailer).to receive(:account_reset_cancel).with(user.email). + and_return(@mailer) + + AccountReset::Cancel.new(token).call + end + + it 'updates the account_reset_request' do + token = create_account_reset_request_for(user) + account_reset_request = AccountResetRequest.find_by(user_id: user.id) + + AccountReset::Cancel.new(token).call + account_reset_request.reload + + expect(account_reset_request.request_token).to_not be_present + expect(account_reset_request.granted_token).to_not be_present + expect(account_reset_request.requested_at).to be_present + expect(account_reset_request.cancelled_at).to be_present + end + end + + context 'when the token is not valid' do + context 'when the user has a phone enabled for SMS' do + it 'does not notify the user via SMS of the account reset cancellation' do + allow(SmsAccountResetCancellationNotifierJob).to receive(:perform_now) + + AccountReset::Cancel.new('foo').call + + expect(SmsAccountResetCancellationNotifierJob).to_not have_received(:perform_now) + end + end + + context 'when the user does not have a phone enabled for SMS' do + it 'does not notify the user via SMS' do + allow(SmsAccountResetCancellationNotifierJob).to receive(:perform_now) + user.update!(phone: nil) + + AccountReset::Cancel.new('foo').call + + expect(SmsAccountResetCancellationNotifierJob).to_not have_received(:perform_now) + end + end + + it 'does not notify the user via email of the account reset cancellation' do + expect(UserMailer).to_not receive(:account_reset_cancel) + + AccountReset::Cancel.new('foo').call + end + + it 'does not update the account_reset_request' do + create_account_reset_request_for(user) + account_reset_request = AccountResetRequest.find_by(user_id: user.id) + + AccountReset::Cancel.new('foo').call + account_reset_request.reload + + expect(account_reset_request.request_token).to be_present + expect(account_reset_request.granted_token).to_not be_present + expect(account_reset_request.requested_at).to be_present + expect(account_reset_request.cancelled_at).to_not be_present + end + end +end diff --git a/spec/services/account_reset_service_spec.rb b/spec/services/account_reset_service_spec.rb index 6ad96b17490..55f63f306d2 100644 --- a/spec/services/account_reset_service_spec.rb +++ b/spec/services/account_reset_service_spec.rb @@ -1,17 +1,13 @@ require 'rails_helper' describe AccountResetService do - include Rails.application.routes.url_helpers + include AccountResetHelper let(:user) { create(:user) } let(:subject) { AccountResetService.new(user) } let(:user2) { create(:user) } let(:subject2) { AccountResetService.new(user2) } - before do - allow(Figaro.env).to receive(:account_reset_wait_period_days).and_return('1') - end - describe '#create_request' do it 'creates a new account reset request on the user' do subject.create_request @@ -34,31 +30,6 @@ end end - describe '#cancel_request' do - it 'removes tokens from a account reset request' do - subject.create_request - cancel = AccountResetService.cancel_request(user.account_reset_request.request_token) - arr = AccountResetRequest.find_by(user_id: user.id) - expect(arr.request_token).to_not be_present - expect(arr.granted_token).to_not be_present - expect(arr.requested_at).to be_present - expect(arr.cancelled_at).to be_present - expect(arr).to eq(cancel) - end - - it 'does not raise an error for a cancel request with a blank token' do - AccountResetService.cancel_request('') - end - - it 'does not raise an error for a cancel request with a nil token' do - AccountResetService.cancel_request('') - end - - it 'does not raise an error for a cancel request with a bad token' do - AccountResetService.cancel_request('ABC') - end - end - describe '#report_fraud' do it 'removes tokens from the request' do subject.create_request @@ -112,7 +83,7 @@ it 'does not send notifications when the request was cancelled' do subject.create_request - AccountResetService.cancel_request(AccountResetRequest.all[0].request_token) + cancel_request_for(user) after_waiting_the_full_wait_period do notifications_sent = AccountResetService.grant_tokens_and_send_notifications @@ -152,7 +123,7 @@ it 'does not send notifications when the request was cancelled' do subject.create_request - AccountResetService.cancel_request(AccountResetRequest.all[0].request_token) + cancel_request_for(user) notifications_sent = AccountResetService.grant_tokens_and_send_notifications expect(notifications_sent).to eq(0) diff --git a/spec/support/account_reset_helper.rb b/spec/support/account_reset_helper.rb new file mode 100644 index 00000000000..ff32b8695ce --- /dev/null +++ b/spec/support/account_reset_helper.rb @@ -0,0 +1,12 @@ +module AccountResetHelper + def create_account_reset_request_for(user) + AccountResetService.new(user).create_request + account_reset_request = AccountResetRequest.find_by(user_id: user.id) + account_reset_request.request_token + end + + def cancel_request_for(user) + account_reset_request = AccountResetRequest.find_by(user_id: user.id) + account_reset_request.update(cancelled_at: Time.zone.now) + end +end From 0445461e9c50275e649b959c5d30a55fd0650c4a Mon Sep 17 00:00:00 2001 From: John Donmoyer Date: Fri, 10 Aug 2018 14:44:54 -0500 Subject: [PATCH 24/25] Change the indexes in Rails link in PR template Whenever there is a new PR the github slack integration shows a preview image from a piece of documentation we link to in our PR template. It adds a lot of noise to the channel and makes the posts take up a lot of space. Until github makes a change in their integration (sounds like others [others want this too](https://github.com/integrations/slack/issues/487)) we could change the link to a shortened version to purposely break this image preview. --- .github/PULL_REQUEST_TEMPLATE.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 6ee07393260..b447dd6b50f 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -19,7 +19,7 @@ will warn you about unsafe migrations and has great step-by-step instructions for various scenarios. - [ ] Indexes were added if necessary. This article provides a good overview -of [indexes in Rails](https://semaphoreci.com/blog/2017/05/09/faster-rails-is-your-database-properly-indexed.html). +of [indexes in Rails](https://goo.gl/1DARYi). - [ ] Verified that the changes don't affect other apps (such as the dashboard) From 3fc6420593759bb9379d061739f1389949ab8d7a Mon Sep 17 00:00:00 2001 From: Jonathan Hooper Date: Mon, 13 Aug 2018 09:44:28 -0500 Subject: [PATCH 25/25] LG-542 Don't allow LOA3 users to request delete account (#2421) **Why**: To disable account reset for LOA3 users while we determine how the LOA3 account reset process will work --- .../account_reset/request_controller.rb | 6 ++ .../two_factor_login_options_presenter.rb | 5 ++ app/services/account_reset_service.rb | 2 +- .../options/index.html.slim | 3 +- .../account_reset/delete_account_spec.rb | 66 +++++++++++++++++++ 5 files changed, 80 insertions(+), 2 deletions(-) create mode 100644 spec/features/account_reset/delete_account_spec.rb diff --git a/app/controllers/account_reset/request_controller.rb b/app/controllers/account_reset/request_controller.rb index 0eb7e428faa..3b16b6d6f7f 100644 --- a/app/controllers/account_reset/request_controller.rb +++ b/app/controllers/account_reset/request_controller.rb @@ -4,6 +4,7 @@ class RequestController < ApplicationController before_action :check_account_reset_enabled before_action :confirm_two_factor_enabled + before_action :confirm_user_not_verified def show; end @@ -21,6 +22,11 @@ def check_account_reset_enabled redirect_to root_url unless FeatureManagement.account_reset_enabled? end + def confirm_user_not_verified + # IAL2 users should not be able to reset account to comply with AAL2 reqs + redirect_to account_url if decorated_user.identity_verified? + end + def reset_session_with_email email = current_user.email sign_out diff --git a/app/presenters/two_factor_login_options_presenter.rb b/app/presenters/two_factor_login_options_presenter.rb index ae06a833f18..5e2cb40dd78 100644 --- a/app/presenters/two_factor_login_options_presenter.rb +++ b/app/presenters/two_factor_login_options_presenter.rb @@ -45,6 +45,11 @@ def options end end + def should_display_account_reset_or_cancel_link? + # IAL2 users should not be able to reset account to comply with AAL2 reqs + !current_user.decorate.identity_verified? + end + def account_reset_or_cancel_link account_reset_token_valid? ? account_reset_cancel_link : account_reset_link end diff --git a/app/services/account_reset_service.rb b/app/services/account_reset_service.rb index 43bc7d4f638..43e5fad94cb 100644 --- a/app/services/account_reset_service.rb +++ b/app/services/account_reset_service.rb @@ -58,7 +58,7 @@ def self.send_notifications_with_sql(users_sql) def self.reset_and_notify(arr) user = arr.user return false unless AccountResetService.new(user).grant_request - UserMailer.account_reset_granted(user, arr).deliver_later + UserMailer.account_reset_granted(user, arr.reload).deliver_later true end private_class_method :reset_and_notify diff --git a/app/views/two_factor_authentication/options/index.html.slim b/app/views/two_factor_authentication/options/index.html.slim index 75c6c1f3ed5..a035884faae 100644 --- a/app/views/two_factor_authentication/options/index.html.slim +++ b/app/views/two_factor_authentication/options/index.html.slim @@ -23,5 +23,6 @@ p.mt-tiny.mb3 = @presenter.info = f.button :submit, t('forms.buttons.continue') br -p = @presenter.account_reset_or_cancel_link +- if @presenter.should_display_account_reset_or_cancel_link? + p = @presenter.account_reset_or_cancel_link = render 'shared/cancel', link: destroy_user_session_path diff --git a/spec/features/account_reset/delete_account_spec.rb b/spec/features/account_reset/delete_account_spec.rb new file mode 100644 index 00000000000..e9635fdf13c --- /dev/null +++ b/spec/features/account_reset/delete_account_spec.rb @@ -0,0 +1,66 @@ +require 'rails_helper' + +describe 'Account Reset Request: Delete Account', email: true do + let(:user) { create(:user, :signed_up) } + + before do + TwilioService::Utils.telephony_service = FakeSms + end + + context 'as an LOA1 user' do + it 'allows the user to delete their account after 24 hours' do + signin(user.email, user.password) + click_link t('two_factor_authentication.login_options_link_text') + click_link t('devise.two_factor_authentication.account_reset.link') + click_button t('account_reset.request.yes_continue') + reset_email + + Timecop.travel(Time.zone.now + 2.days) do + AccountResetService.grant_tokens_and_send_notifications + open_last_email + click_email_link_matching(/delete_account\?token/) + + expect(page).to have_content(t('account_reset.delete_account.title')) + expect(page).to have_current_path(account_reset_delete_account_path) + + click_on t('account_reset.delete_account.delete_button') + + expect(page).to have_content( + strip_tags( + t( + 'account_reset.confirm_delete_account.info', + email: user.email, + link: t('account_reset.confirm_delete_account.link_text') + ) + ) + ) + expect(page).to have_current_path(account_reset_confirm_delete_account_path) + expect(User.where(id: user.id)).to be_empty + end + end + end + + context 'as an LOA3 user' do + let(:user) do + create( + :profile, + :active, + :verified, + pii: { first_name: 'John', ssn: '111223333' } + ).user + end + + it 'does not allow the user to delete their account from 2FA screen' do + signin(user.email, user.password) + click_link t('two_factor_authentication.login_options_link_text') + + # Account reset link should not be present + expect(page).to_not have_content(t('devise.two_factor_authentication.account_reset.link')) + + # Visiting account reset directly should redirect to 2FA + visit account_reset_request_path + + expect(page.current_path).to eq(login_two_factor_path(otp_delivery_preference: :sms)) + end + end +end