Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 5 additions & 7 deletions app/controllers/api/irs_attempts_api_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ class IrsAttemptsApiController < ApplicationController
def create
start_time = Time.zone.now.to_f
if timestamp
if IdentityConfig.store.irs_attempt_api_aws_s3_enabled
if s3_helper.attempts_serve_events_from_s3
if IrsAttemptApiLogFile.find_by(requested_time: timestamp_key(key: timestamp))
log_file_record = IrsAttemptApiLogFile.find_by(
requested_time: timestamp_key(key: timestamp),
Expand All @@ -29,10 +29,8 @@ def create
headers['X-Payload-Key'] = log_file_record.encrypted_key
headers['X-Payload-IV'] = log_file_record.iv

bucket_name = IdentityConfig.store.irs_attempt_api_bucket_name

requested_data = s3_client.get_object(
bucket: bucket_name,
requested_data = s3_helper.s3_client.get_object(
bucket: s3_helper.attempts_bucket_name,
key: log_file_record.filename,
)

Expand Down Expand Up @@ -102,8 +100,8 @@ def redis_client
@redis_client ||= IrsAttemptsApi::RedisClient.new
end

def s3_client
@s3_client ||= JobHelpers::S3Helper.new.s3_client
def s3_helper
@s3_helper ||= JobHelpers::S3Helper.new
end

def valid_auth_tokens
Expand Down
12 changes: 6 additions & 6 deletions app/jobs/irs_attempts_events_batch_job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,7 @@ class IrsAttemptsEventsBatchJob < ApplicationJob
queue_as :default

def perform(timestamp = Time.zone.now - 1.hour)
enabled = IdentityConfig.store.irs_attempt_api_enabled &&
IdentityConfig.store.irs_attempt_api_aws_s3_enabled &&
IdentityConfig.store.irs_attempt_api_bucket_name
enabled = IdentityConfig.store.irs_attempt_api_enabled && s3_helper.attempts_s3_write_enabled
return nil unless enabled

events = IrsAttemptsApi::RedisClient.new.read_events(timestamp: timestamp)
Expand All @@ -16,10 +14,8 @@ def perform(timestamp = Time.zone.now - 1.hour)
data: event_values, timestamp: timestamp, public_key_str: public_key,
)

bucket_name = IdentityConfig.store.irs_attempt_api_bucket_name

create_and_upload_to_attempts_s3_resource(
bucket_name: bucket_name, filename: result.filename,
bucket_name: s3_helper.attempts_bucket_name, filename: result.filename,
encrypted_data: result.encrypted_data
)

Expand All @@ -42,4 +38,8 @@ def create_and_upload_to_attempts_s3_resource(bucket_name:, filename:, encrypted
def redis_client
@redis_client ||= IrsAttemptsApi::RedisClient.new
end

def s3_helper
@s3_helper ||= JobHelpers::S3Helper.new
end
end
12 changes: 12 additions & 0 deletions app/jobs/job_helpers/s3_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,5 +25,17 @@ def s3_client
compute_checksums: false,
)
end

def attempts_bucket_name
IdentityConfig.store.irs_attempt_api_bucket_name
end

def attempts_s3_write_enabled
(attempts_bucket_name && attempts_bucket_name != 'default-placeholder')
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's probably fine since the configuration value appears to be nil-able, but I'd be concerned if attempts_bucket_name could be an empty string, since the left-hand of this would pass. #present? might be a good safety guard.

Also, the parentheses don't seem necessary.

Suggested change
(attempts_bucket_name && attempts_bucket_name != 'default-placeholder')
attempts_bucket_name.present? && attempts_bucket_name != 'default-placeholder'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to these suggestions

end

def attempts_serve_events_from_s3
IdentityConfig.store.irs_attempt_api_aws_s3_enabled && attempts_s3_write_enabled
end
end
end
2 changes: 1 addition & 1 deletion config/application.yml.default
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,7 @@ development:
irs_attempt_api_enabled: true
irs_attempt_api_aws_s3_enabled: false
irs_attempt_api_auth_tokens: 'abc123'
irs_attempt_api_bucket_name: please-change-me
irs_attempt_api_bucket_name: default-placeholder
logins_per_ip_limit: 5
logo_upload_enabled: true
max_bad_passwords: 5
Expand Down
63 changes: 61 additions & 2 deletions spec/jobs/job_helpers/s3_helper_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@
"https://s3.region-name.amazonaws.com/#{bucket_name}/#{prefix}?param=true&signature=123"
end

it 'downloads by extracing prefix and bucket from s3 URLs' do
it 'downloads by extracting prefix and bucket from s3 URLs' do
expect(s3_helper.download(url)).to eq(body)
end
end
Expand All @@ -62,7 +62,7 @@
"https://#{bucket_name}.s3.region-name.amazonaws.com/#{prefix}?param=true&signature=123"
end

it 'downloads by extracing prefix and bucket from s3 URLs' do
it 'downloads by extracting prefix and bucket from s3 URLs' do
expect(s3_helper.download(url)).to eq(body)
end
end
Expand All @@ -83,4 +83,63 @@
expect(s3_helper.download(url).encoding.name).to eq('ASCII-8BIT')
end
end

let(:valid_bucket_name) { 'valid-attempts-api-s3-bucket' }

describe '#attempts_bucket_methods' do
subject(:attempts_s3_write_enabled) { s3_helper.attempts_s3_write_enabled }

context 'with no bucket name' do
it 'should return nil' do
allow(IdentityConfig.store).to receive(:irs_attempt_api_bucket_name).and_return(nil)
is_expected.to eq(nil)
end
end

context 'with a default bucket name' do
it 'should return false' do
allow(IdentityConfig.store).to receive(:irs_attempt_api_bucket_name).
and_return('default-placeholder')
is_expected.to eq(false)
end
end

context 'with a valid bucket name' do
it 'should return true' do
allow(IdentityConfig.store).to receive(:irs_attempt_api_bucket_name).
and_return(valid_bucket_name)
is_expected.to eq(true)
end
end
end

describe '#attempts_serve_events_from_s3' do
subject(:attempts_serve_events_from_s3) { s3_helper.attempts_serve_events_from_s3 }

context 'with s3 disabled and a valid s3 bucket' do
it 'should return false' do
allow(IdentityConfig.store).to receive(:irs_attempt_api_aws_s3_enabled).and_return(false)
allow(IdentityConfig.store).to receive(:irs_attempt_api_bucket_name).
and_return(valid_bucket_name)
is_expected.to eq(false)
end
end

context 'with s3 disabled and no s3 bucket' do
it 'should return false' do
allow(IdentityConfig.store).to receive(:irs_attempt_api_aws_s3_enabled).and_return(false)
allow(IdentityConfig.store).to receive(:irs_attempt_api_bucket_name).and_return(nil)
is_expected.to eq(false)
end
end

context 'with s3 enabled and a valid s3 bucket' do
it 'should return true' do
allow(IdentityConfig.store).to receive(:irs_attempt_api_aws_s3_enabled).and_return(true)
allow(IdentityConfig.store).to receive(:irs_attempt_api_bucket_name).
and_return('valid-attempts-api-s3-bucket')
is_expected.to eq(true)
end
end
end
end