Skip to content

Create a report of integrations using deprecated LOA ACR values #10562

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
vrajmohan merged 1 commit into from
May 9, 2024
Merged

Create a report of integrations using deprecated LOA ACR values #10562

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
47 changes: 47 additions & 0 deletions lib/reporting/protocols_report.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ def as_tables
overview_table,
protocols_table,
saml_signature_issues_table,
loa_acr_requests_table,
]
end

Expand All @@ -66,6 +67,10 @@ def as_emailable_reports
title: 'SAML Signature Issues',
table: saml_signature_issues_table,
),
Reporting::EmailableReport.new(
title: 'LOA ACR Requests',
table: loa_acr_requests_table,
),
]
end

Expand Down Expand Up @@ -210,6 +215,48 @@ def saml_signature_issues_table
]
end

def loa_acr_requests_table
[
['Count of integrations using LOA', 'List of issuers with the issue'],
[
loa_issuers_data.length,
loa_issuers_data.join(', '),
],
]
end

def loa_issuers_data
@loa_issuers_data ||= begin
cloudwatch_client.fetch(
query: loa_issuers_query,
from: time_range.begin,
to: time_range.end,
).
map { |slice| slice['issuer'] }.
Copy link
Copy Markdown
Contributor

@Sgtpluck Sgtpluck May 9, 2024

Choose a reason for hiding this comment

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

we don't need to fix this in this PR, but i have been having a hard time with the word slice in this context (and other places it's used in maps) especially since we have a different slice var on the instance. we should probably go back and change the references to something that make more sense in the local context, but it's not blocking.

Copy link
Copy Markdown
Contributor Author

@vrajmohan vrajmohan May 9, 2024

Choose a reason for hiding this comment

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

I inherited the naming from mfa_report.rb. The fetched results from CloudWatch are row slices, ie they could be a full row from the original query (if the time slice and the period coincided) or they could be a row split into time slices. We could use row or row_slice. I don't like row as it hides the fact that it could have been split up. I would prefer row_slice, or leaving it as slice.

Copy link
Copy Markdown
Contributor

@Sgtpluck Sgtpluck May 9, 2024

Choose a reason for hiding this comment

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

i understand why slice was used. i still think reusing the variable slice can be confusing at a glance.

uniq
Copy link
Copy Markdown
Contributor

@Sgtpluck Sgtpluck May 9, 2024

Choose a reason for hiding this comment

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

[question] since you're running dedup in the query, is this necessary?

Copy link
Copy Markdown
Contributor Author

@vrajmohan vrajmohan May 9, 2024

Choose a reason for hiding this comment

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

It is needed because the same issuer may be present in different time slices; the dedup would only make them unique within a slice.

end
end

def loa_issuers_query
params = {
event: quote([SAML_AUTH_EVENT, OIDC_AUTH_EVENT]),
}

format(<<~QUERY, params)
fields
coalesce(properties.event_properties.service_provider, properties.event_properties.client_id) as issuer,
properties.event_properties.acr_values as acr
| parse @message '"authn_context":[*]' as authn
| filter
name IN %{event}
AND (authn like /ns\\/assurance\\/loa/ OR acr like /ns\\/assurance\\/loa/)
AND properties.event_properties.success= 1
| display issuer
| sort issuer
| dedup issuer
QUERY
end

def to_percent(numerator, denominator)
(100.0 * numerator / denominator).round(2)
end
Expand Down
21 changes: 20 additions & 1 deletion spec/lib/reporting/protocols_report_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,22 @@
'invalid_signature_count' => '2',
},
]
loa_issuers_query_response = [
{
'issuer' => 'Issuer1',
},
{
'issuer' => 'Issuer2',
},
{
'issuer' => 'Issuer3',
},
]
cloudwatch_client = instance_double('Reporting::CloudwatchClient')
allow(cloudwatch_client).to receive(:fetch).and_return(
protocol_query_response,
saml_signature_query_response,
loa_issuers_query_response,
)

allow(report).to receive(:cloudwatch_client).and_return(cloudwatch_client)
Expand All @@ -81,7 +93,7 @@
describe '#to_csvs' do
it 'generates a csv' do
csv_string_list = report.to_csvs
expect(csv_string_list.count).to be 3
expect(csv_string_list.count).to be 4

csvs = csv_string_list.map { |csv| CSV.parse(csv) }

Expand Down Expand Up @@ -180,6 +192,13 @@ def expected_tables(strings: false)
['Not signing SAML authentication requests', string_or_num(strings, 2), 'Issuer1, Issuer3'],
['Incorrectly signing SAML authentication requests', string_or_num(strings, 1), 'Issuer1'],
],
[
['Count of integrations using LOA', 'List of issuers with the issue'],
[
string_or_num(strings, 3),
'Issuer1, Issuer2, Issuer3',
],
],
]
end

Expand Down