Skip to content

Update IDV report to support multiple issuers (LG-10875) #9148

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
zachmargolis merged 5 commits into from
Sep 6, 2023
Merged

Update IDV report to support multiple issuers (LG-10875) #9148

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bcf8220
Update IDV report to support multiple issuers (LG-10875)
zachmargolis Sep 5, 2023
25bcfb4
lintfix
zachmargolis Sep 5, 2023
d4d656c
Add changelog
zachmargolis Sep 5, 2023
3ca0ec8
Update YARD params docs
zachmargolis Sep 6, 2023
3f571c3
Merge remote-tracking branch 'origin/main' into margolis-multiple-iss…
zachmargolis Sep 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/jobs/reports/identity_verification_report.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ def perform(report_date)

def report_maker
Reporting::IdentityVerificationReport.new(
issuer: nil,
issuers: [],
time_range: report_date.all_day,
slice: 4.hours,
)
Expand Down
14 changes: 7 additions & 7 deletions lib/reporting/authentication_report.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ module Reporting
class AuthenticationReport
include Reporting::CloudwatchQueryQuoting

attr_reader :issuer, :time_range
attr_reader :issuers, :time_range

module Events
OIDC_AUTH_REQUEST = 'OpenID Connect: authorization request'
Expand All @@ -28,17 +28,17 @@ def self.all_events
end
end

# @param [String] isssuer
# @param [Array<String>] issuers
# @param [Range<Time>] time_range
def initialize(
issuer:,
issuers:,
time_range:,
verbose: false,
progress: false,
slice: 3.hours,
threads: 5
)
@issuer = issuer
@issuers = issuers
@time_range = time_range
@verbose = verbose
@progress = progress
Expand All @@ -59,7 +59,7 @@ def to_csv
CSV.generate do |csv|
csv << ['Report Timeframe', "#{time_range.begin} to #{time_range.end}"]
csv << ['Report Generated', Date.today.to_s] # rubocop:disable Rails/Date
csv << ['Issuer', issuer]
csv << ['Issuer', issuers.join(', ')]
csv << []
csv << ['Metric', 'Number of accounts', '% of total from start']
csv << [
Expand Down Expand Up @@ -161,7 +161,7 @@ def fetch_results

def query
params = {
issuer: quote(issuer),
issuers: quote(issuers),
event_names: quote(Events.all_events),
email_confirmation: quote(Events::EMAIL_CONFIRMATION),
}
Expand All @@ -170,7 +170,7 @@ def query
fields
name
, properties.user_id AS user_id
| filter properties.service_provider = %{issuer}
| filter properties.service_provider IN %{issuers}
Copy link
Copy Markdown
Contributor

@n1zyy n1zyy Sep 6, 2023

Choose a reason for hiding this comment

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

Just confirming: Does using an array here just intrinsically work? Without looking through the parser code I would have guessed we had to .join(', ') here. I'm inferring (based on tests) that it gets evaluated by a smarter parser.

Copy link
Copy Markdown
Contributor Author

@zachmargolis zachmargolis Sep 6, 2023

Choose a reason for hiding this comment

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

Good question! It's the call to quote() above that formats it as an array

| filter (name = %{email_confirmation} and properties.event_properties.success = 1)
or (name != %{email_confirmation})
| filter name in %{event_names}
Expand Down
8 changes: 4 additions & 4 deletions lib/reporting/command_line_options.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ class CommandLineOptions
# @return [Hash]
def parse!(argv, out: STDOUT, require_issuer: true)
date = nil
issuer = nil
issuers = []
verbose = false
progress = true
period = nil
Expand Down Expand Up @@ -49,7 +49,7 @@ def parse!(argv, out: STDOUT, require_issuer: true)
end

opts.on('--issuer=ISSUER') do |issuer_v|
issuer = issuer_v
issuers << issuer_v
end

opts.on('--[no-]verbose', 'log details STDERR, default to off') do |verbose_v|
Expand Down Expand Up @@ -89,13 +89,13 @@ def parse!(argv, out: STDOUT, require_issuer: true)

parser.parse!(argv)

if !date || (require_issuer && !issuer)
if !date || (require_issuer && issuers.empty?)
out.puts parser
exit 1
else
{
time_range: time_range(date:, period:),
issuer: issuer,
issuers: issuers,
verbose: verbose,
progress: progress,
slice: slice || 3.hours,
Expand Down
14 changes: 7 additions & 7 deletions lib/reporting/identity_verification_report.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ module Reporting
class IdentityVerificationReport
include Reporting::CloudwatchQueryQuoting

attr_reader :issuer, :time_range
attr_reader :issuers, :time_range

module Events
IDV_DOC_AUTH_WELCOME = 'IdV: doc auth welcome visited'
Expand All @@ -36,17 +36,17 @@ module Results
IDV_FINAL_RESOLUTION_IN_PERSON = 'IdV: final resolution - In Person Proofing'
end

# @param [String] isssuer
# @param [Array<String>] issuers
# @param [Range<Time>] date
def initialize(
issuer:,
issuers:,
time_range:,
verbose: false,
progress: false,
slice: 3.hours,
threads: 5
)
@issuer = issuer
@issuers = issuers
@time_range = time_range
@verbose = verbose
@progress = progress
Expand All @@ -66,7 +66,7 @@ def to_csv
CSV.generate do |csv|
csv << ['Report Timeframe', "#{time_range.begin} to #{time_range.end}"]
csv << ['Report Generated', Date.today.to_s] # rubocop:disable Rails/Date
csv << ['Issuer', issuer] if issuer.present?
csv << ['Issuer', issuers.join(', ')] if issuers.present?
csv << []
csv << ['Metric', '# of Users']
csv << []
Expand Down Expand Up @@ -174,7 +174,7 @@ def fetch_results

def query
params = {
issuer: issuer && quote(issuer),
issuers: issuers.present? && quote(issuers),
event_names: quote(Events.all_events),
usps_enrollment_status_updated: quote(Events::USPS_ENROLLMENT_STATUS_UPDATED),
gpo_verification_submitted: quote(Events::GPO_VERIFICATION_SUBMITTED),
Expand All @@ -185,7 +185,7 @@ def query
fields
name
, properties.user_id AS user_id
#{issuer.present? ? '| filter properties.service_provider = %{issuer}' : ''}
#{issuers.present? ? '| filter properties.service_provider IN %{issuers}' : ''}
| filter name in %{event_names}
| filter (name = %{usps_enrollment_status_updated} and properties.event_properties.passed = 1)
or (name != %{usps_enrollment_status_updated})
Expand Down
3 changes: 2 additions & 1 deletion lib/reporting/monthly_proofing_report.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,15 @@ def self.all_events
end
end

# @param [Array<String>] issuers
# @param [Range<Time>] date
def initialize(
time_range:,
verbose: false,
progress: false,
slice: 3.hours,
threads: 5,
issuer: nil # rubocop:disable Lint/UnusedMethodArgument
issuers: [] # rubocop:disable Lint/UnusedMethodArgument
)
@time_range = time_range
@verbose = verbose
Expand Down
4 changes: 2 additions & 2 deletions spec/lib/reporting/authentication_report_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
let(:issuer) { 'my:example:issuer' }
let(:time_range) { Date.new(2022, 1, 1).all_day }

subject(:report) { Reporting::AuthenticationReport.new(issuer:, time_range:) }
subject(:report) { Reporting::AuthenticationReport.new(issuers: [issuer], time_range:) }

before do
cloudwatch_client = double(
Expand Down Expand Up @@ -73,7 +73,7 @@

describe '#cloudwatch_client' do
let(:opts) { {} }
let(:subject) { described_class.new(issuer:, time_range:, **opts) }
let(:subject) { described_class.new(issuers: [issuer], time_range:, **opts) }
let(:default_args) do
{
num_threads: 5,
Expand Down
20 changes: 18 additions & 2 deletions spec/lib/reporting/command_line_options_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,23 @@
it 'returns the parsed options' do
expect(parse!).to match(
time_range: Date.new(2023, 1, 1).in_time_zone('UTC').all_day,
issuer: issuer,
issuers: [issuer],
verbose: false,
progress: true,
slice: 3.hours,
threads: 5,
)
end
end

context 'with --date and multiple --issuer tags' do
let(:argv) { %W[--date 2023-1-1 --issuer #{issuer} --issuer #{issuer2}] }
let(:issuer2) { 'my:other:example:issuer' }

it 'returns the parsed options' do
expect(parse!).to match(
time_range: Date.new(2023, 1, 1).in_time_zone('UTC').all_day,
issuers: [issuer, issuer2],
verbose: false,
progress: true,
slice: 3.hours,
Expand All @@ -72,7 +88,7 @@
it 'returns the parsed options' do
expect(parse!).to match(
time_range: Date.new(2023, 1, 1).in_time_zone('UTC').all_day,
issuer: nil,
issuers: [],
verbose: false,
progress: true,
slice: 3.hours,
Expand Down
8 changes: 5 additions & 3 deletions spec/lib/reporting/identity_verification_report_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@
let(:issuer) { 'my:example:issuer' }
let(:time_range) { Date.new(2022, 1, 1).all_day }

subject(:report) { Reporting::IdentityVerificationReport.new(issuer:, time_range:) }
subject(:report) do
Reporting::IdentityVerificationReport.new(issuers: Array(issuer), time_range:)
end

# rubocop:disable Layout/LineLength
before do
Expand Down Expand Up @@ -102,7 +104,7 @@
it 'includes an issuer filter' do
result = subject.query

expect(result).to include('| filter properties.service_provider = "my:example:issuer"')
expect(result).to include('| filter properties.service_provider IN ["my:example:issuer"]')
end
end

Expand All @@ -119,7 +121,7 @@

describe '#cloudwatch_client' do
let(:opts) { {} }
let(:subject) { described_class.new(issuer:, time_range:, **opts) }
let(:subject) { described_class.new(issuers: Array(issuer), time_range:, **opts) }
let(:default_args) do
{
num_threads: 5,
Expand Down