From 7991363087899429f5a1a3d545387275f3e03439 Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Wed, 18 Oct 2017 09:23:57 -0400 Subject: [PATCH 1/3] Extract seeds.rb code into ServiceProviderSeeder **Why**: So we can test it - Also adds specs with desired (but failing) behavior --- app/services/service_provider_seeder.rb | 26 ++++++++++ app/services/service_provider_updater.rb | 1 + db/seeds.rb | 13 +---- spec/services/service_provider_seeder_spec.rb | 47 +++++++++++++++++++ 4 files changed, 75 insertions(+), 12 deletions(-) create mode 100644 app/services/service_provider_seeder.rb create mode 100644 spec/services/service_provider_seeder_spec.rb diff --git a/app/services/service_provider_seeder.rb b/app/services/service_provider_seeder.rb new file mode 100644 index 00000000000..412c1da283b --- /dev/null +++ b/app/services/service_provider_seeder.rb @@ -0,0 +1,26 @@ +# Update ServiceProvider from config/service_providers.yml (all environments in rake db:seed) +class ServiceProviderSeeder + def initialize(rails_env: Rails.env, deploy_env: LoginGov::Hostdata.env) + @rails_env = rails_env + @deploy_env = deploy_env + end + + def run + content = ERB.new(Rails.root.join('config', 'service_providers.yml').read).result + service_providers = YAML.load(content).fetch(rails_env, {}) + + service_providers.each do |issuer, config| + next if Figaro.env.chef_env == 'prod' && config['allow_on_prod_chef_env'] != 'true' + ServiceProvider.find_or_create_by!(issuer: issuer) do |sp| + sp.approved = true + sp.active = true + sp.native = true + sp.attributes = config.except('allow_on_prod_chef_env') + end + end + end + + private + + attr_reader :rails_env, :deploy_env +end diff --git a/app/services/service_provider_updater.rb b/app/services/service_provider_updater.rb index e2491cc6e2b..a8b20fadff5 100644 --- a/app/services/service_provider_updater.rb +++ b/app/services/service_provider_updater.rb @@ -1,3 +1,4 @@ +# Update ServiceProvider table by pulling from the Dashboard app API (lower environments only) class ServiceProviderUpdater PROTECTED_ATTRIBUTES = %i[ created_at diff --git a/db/seeds.rb b/db/seeds.rb index 6445f22dd25..8a06c359cf3 100644 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -4,15 +4,4 @@ end # add config/service_providers.yml -content = ERB.new(Rails.root.join('config', 'service_providers.yml').read).result -service_providers = YAML.load(content).fetch(Rails.env, {}) - -service_providers.each do |issuer, config| - next if Figaro.env.chef_env == 'prod' && config['allow_on_prod_chef_env'] != 'true' - ServiceProvider.find_or_create_by!(issuer: issuer) do |sp| - sp.approved = true - sp.active = true - sp.native = true - sp.attributes = config.except('allow_on_prod_chef_env') - end -end +ServiceProviderSeeder.new.run diff --git a/spec/services/service_provider_seeder_spec.rb b/spec/services/service_provider_seeder_spec.rb new file mode 100644 index 00000000000..2c6c11da0c5 --- /dev/null +++ b/spec/services/service_provider_seeder_spec.rb @@ -0,0 +1,47 @@ +require 'rails_helper' + +RSpec.describe ServiceProviderSeeder do + subject(:instance) { ServiceProviderSeeder.new(rails_env: rails_env, deploy_env: deploy_env) } + let(:rails_env) { 'test' } + let(:deploy_env) { 'int' } + + describe '#run' do + before { ServiceProvider.delete_all } + + subject(:run) { instance.run } + + it 'inserts service providers into the database from service_providers.yml' do + expect { run }.to change { ServiceProvider.count } + end + + context 'when a service provider already exists in the database' do + before do + create( + :service_provider, + issuer: 'http://test.host', + acs_url: 'http://test.host/test/saml/decode_assertion_old' + ) + end + + it 'updates the attributes based on the current value of the yml file' do + expect { run }. + to change { ServiceProvider.from_issuer('http://test.host').acs_url }. + to('http://test.host/test/saml/decode_assertion') + end + end + + context 'when running in a production environment' do + let(:rails_env) { 'production' } + + it 'only adds/updates configs for that environment' do + run + + expect(ServiceProvider.find_by(issuer: 'urn:gov:dhs.cbp.jobs:openidconnect:aws-cbp-ttp')). + to be + + expect(ServiceProvider.find_by(issuer: 'urn:gov:dhs.cbp.jobs:openidconnect:cert')). + to eq(nil) + end + end + end +end From 27e9f397b1af0656861c795b89209b5f73e4f0f6 Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Mon, 16 Oct 2017 14:57:50 -0400 Subject: [PATCH 2/3] Stop reading old singular redirect_uri column **Why**: We've migrated to the new plural redirect_uris --- app/models/service_provider.rb | 4 ---- spec/models/service_provider_spec.rb | 4 ++-- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/app/models/service_provider.rb b/app/models/service_provider.rb index 68fea443217..f065650df5d 100644 --- a/app/models/service_provider.rb +++ b/app/models/service_provider.rb @@ -43,8 +43,4 @@ def encryption_opts def live? active? && approved? end - - def redirect_uris - super.presence || Array(redirect_uri) - end end diff --git a/spec/models/service_provider_spec.rb b/spec/models/service_provider_spec.rb index 3bff59af8fb..f4d864e1c9a 100644 --- a/spec/models/service_provider_spec.rb +++ b/spec/models/service_provider_spec.rb @@ -140,8 +140,8 @@ ) end - it 'is an array of the legacy redirect_uri' do - expect(service_provider.redirect_uris).to eq(%w[http://a.example.com]) + it 'ignores the old singular column and just uses the new plural one' do + expect(service_provider.redirect_uris).to eq([]) end end From bedcfe1309eb942b2f367ff73a475372e64067d8 Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Wed, 18 Oct 2017 09:51:04 -0400 Subject: [PATCH 3/3] Fix service provider seeding **Why**: - Update existing SPs with data in service_providers.yml - Stop relying on Figaro.env.chef_env, use LoginGov::Hostdata.env - Add ability to restrict SP to arbitrary envs for lower envs --- .reek | 2 + app/services/service_provider_seeder.rb | 24 +++++-- config/service_providers.yml | 12 ++-- spec/services/service_provider_seeder_spec.rb | 64 +++++++++++++++++-- 4 files changed, 85 insertions(+), 17 deletions(-) diff --git a/.reek b/.reek index bc5ef4b27cc..52c2e3a05bf 100644 --- a/.reek +++ b/.reek @@ -28,6 +28,7 @@ FeatureEnvy: - Idv::ProfileMaker#pii_from_applicant - Idv::Step#vendor_validator_result - IdvSession#vendor_result_timed_out? + - ServiceProviderSeeder#run InstanceVariableAssumption: exclude: - User @@ -41,6 +42,7 @@ NestedIterators: - FileEncryptor#encrypt - UserFlowExporter#self.massage_html - TwilioService#sanitize_phone_number + - ServiceProviderSeeder#run NilCheck: enabled: false LongParameterList: diff --git a/app/services/service_provider_seeder.rb b/app/services/service_provider_seeder.rb index 412c1da283b..cb5e3c06f80 100644 --- a/app/services/service_provider_seeder.rb +++ b/app/services/service_provider_seeder.rb @@ -6,21 +6,33 @@ def initialize(rails_env: Rails.env, deploy_env: LoginGov::Hostdata.env) end def run - content = ERB.new(Rails.root.join('config', 'service_providers.yml').read).result - service_providers = YAML.load(content).fetch(rails_env, {}) - service_providers.each do |issuer, config| - next if Figaro.env.chef_env == 'prod' && config['allow_on_prod_chef_env'] != 'true' + next unless write_service_provider?(config) + ServiceProvider.find_or_create_by!(issuer: issuer) do |sp| sp.approved = true sp.active = true sp.native = true - sp.attributes = config.except('allow_on_prod_chef_env') - end + end.update(config.except('restrict_to_deploy_env')) end end private attr_reader :rails_env, :deploy_env + + def service_providers + content = ERB.new(Rails.root.join('config', 'service_providers.yml').read).result + YAML.safe_load(content).fetch(rails_env, {}) + end + + def write_service_provider?(config) + return true if rails_env != 'production' + + restrict_env = config['restrict_to_deploy_env'] + + is_production_or_has_a_restriction = (deploy_env == 'prod' || restrict_env.present?) + + !is_production_or_has_a_restriction || (restrict_env == deploy_env) + end end diff --git a/config/service_providers.yml b/config/service_providers.yml index d3141a53d2e..2cec3e50b20 100644 --- a/config/service_providers.yml +++ b/config/service_providers.yml @@ -209,7 +209,7 @@ production: cert: 'sp_micropurchase' agency: 'TTS Acquisition' logo: '18f.svg' - allow_on_prod_chef_env: 'true' + restrict_to_deploy_env: 'prod' friendly_name: 'Micro-purchase' return_to_sp_url: 'https://micropurchase.18f.gov' attribute_bundle: @@ -248,6 +248,7 @@ production: friendly_name: 'CBP Jobs' agency: 'DHS' logo: 'cbp.png' + restrict_to_deploy_env: 'staging' 'urn:gov:dhs.cbp.jobs:openidconnect:cert:app': redirect_uris: @@ -255,6 +256,7 @@ production: friendly_name: 'CBP Jobs' agency: 'DHS' logo: 'cbp.png' + restrict_to_deploy_env: 'staging' 'urn:gov:dhs.cbp.jobs:openidconnect:prod': redirect_uris: @@ -262,7 +264,7 @@ production: friendly_name: 'CBP Jobs' agency: 'DHS' logo: 'cbp.png' - allow_on_prod_chef_env: 'true' + restrict_to_deploy_env: 'prod' return_to_sp_url: 'https://careers.cbp.dhs.gov' 'urn:gov:dhs.cbp.jobs:openidconnect:prod:app': @@ -271,7 +273,7 @@ production: friendly_name: 'CBP Jobs' agency: 'DHS' logo: 'cbp.png' - allow_on_prod_chef_env: 'true' + restrict_to_deploy_env: 'prod' # RRB 'urn:gov:gsa:SAML:2.0.profiles:sp:sso:RRB:BOS-Pre-Prod': @@ -309,7 +311,7 @@ production: 'urn:gov:dhs.cbp.jobs:openidconnect:aws-cbp-ttp': agency: 'DHS' - allow_on_prod_chef_env: 'true' + restrict_to_deploy_env: 'prod' block_encryption: 'aes256-cbc' cert: 'cbp_goes_prod' friendly_name: 'CBP Trusted Traveler Programs' @@ -325,4 +327,4 @@ production: logo: 'cbp.png' redirect_uris: - 'gov.dhs.cbp.pspd.oars.user.prod://result' - allow_on_prod_chef_env: 'true' + restrict_to_deploy_env: 'prod' diff --git a/spec/services/service_provider_seeder_spec.rb b/spec/services/service_provider_seeder_spec.rb index 2c6c11da0c5..a536580a837 100644 --- a/spec/services/service_provider_seeder_spec.rb +++ b/spec/services/service_provider_seeder_spec.rb @@ -14,6 +14,28 @@ expect { run }.to change { ServiceProvider.count } end + context 'with other existing service providers in the database' do + let!(:existing_provider) { create(:service_provider) } + + it 'sets approved, active and native on service providers from the yaml' do + run + + config_sp = ServiceProvider.from_issuer('http://test.host') + expect(config_sp.approved).to eq(true) + expect(config_sp.active).to eq(true) + expect(config_sp.native).to eq(true) + end + + it 'does not change approve, active and native on the other existing service providers' do + run + + existing_provider.reload + expect(existing_provider.approved).to_not eq(true) + expect(existing_provider.active).to_not eq(true) + expect(existing_provider.native).to_not eq(true) + end + end + context 'when a service provider already exists in the database' do before do create( @@ -33,14 +55,44 @@ context 'when running in a production environment' do let(:rails_env) { 'production' } - it 'only adds/updates configs for that environment' do - run + context 'in prod' do + let(:deploy_env) { 'prod' } + + it 'only writes configs with restrict_to_deploy_env for prod' do + run + + # restrict_to_deploy_env: prod + expect(ServiceProvider.find_by(issuer: 'urn:gov:dhs.cbp.jobs:openidconnect:aws-cbp-ttp')). + to be_present + + # restrict_to_deploy_env: staging + expect(ServiceProvider.find_by(issuer: 'urn:gov:dhs.cbp.jobs:openidconnect:cert')). + to eq(nil) + + # restrict_to_deploy_env: nil + expect(ServiceProvider.find_by(issuer: 'urn:gov:gsa:openidconnect:sp:sinatra')). + to eq(nil) + end + end + + context 'in another environment' do + let(:deploy_env) { 'staging' } + + it 'only writes configs with restrict_to_deploy_env for that env, or no restrictions' do + run + + # restrict_to_deploy_env: prod + expect(ServiceProvider.find_by(issuer: 'urn:gov:dhs.cbp.jobs:openidconnect:aws-cbp-ttp')). + to eq(nil) - expect(ServiceProvider.find_by(issuer: 'urn:gov:dhs.cbp.jobs:openidconnect:aws-cbp-ttp')). - to be + # restrict_to_deploy_env: staging + expect(ServiceProvider.find_by(issuer: 'urn:gov:dhs.cbp.jobs:openidconnect:cert')). + to be_present - expect(ServiceProvider.find_by(issuer: 'urn:gov:dhs.cbp.jobs:openidconnect:cert')). - to eq(nil) + # restrict_to_deploy_env: nil + expect(ServiceProvider.find_by(issuer: 'urn:gov:gsa:openidconnect:sp:sinatra')). + to be_present + end end end end