Skip to content
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.

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

LG-14198: AAGUID logging #11138

Merged
merged 10 commits into from
Sep 3, 2024
5 changes: 4 additions & 1 deletion app/forms/webauthn_setup_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ def generic_error_message

private

attr_reader :success, :transports, :invalid_transports, :protocol
attr_reader :success, :transports, :aaguid, :invalid_transports, :protocol
attr_accessor :user, :challenge, :attestation_object, :client_data_json,
:name, :platform_authenticator, :authenticator_data_flags, :device_name

Expand Down Expand Up @@ -110,6 +110,7 @@ def valid_attestation_response?(protocol)
)

begin
@aaguid = attestation_response.authenticator_data.aaguid
Copy link
Member

Choose a reason for hiding this comment

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

Couple minor comments:

  • If we're not expecting this to raise an exception, I think it'd be best to limit the rescue'd begin block to code which is expected to raise.
  • Assigning it as a side-effect of validation feels like it creates a fragile relationship between assignment and use. Since we have attestation_response as a consumed parameter in submit and the value is derived from that, we could create a method which derives directly from that and avoids another instance variable.
def aaguid
  attestation_response&.authenticator_data&.aaguid
end

attestation_response.valid?(@challenge.pack('c*'), original_origin)
rescue StandardError
false
Expand Down Expand Up @@ -141,6 +142,7 @@ def create_webauthn_configuration
platform_authenticator: platform_authenticator,
transports: transports.presence,
authenticator_data_flags: authenticator_data_flags,
aaguid: aaguid,
)
end

Expand Down Expand Up @@ -172,6 +174,7 @@ def extra_analytics_attributes
pii_like_keypaths: [[:mfa_method_counts, :phone]],
authenticator_data_flags: authenticator_data_flags,
unknown_transports: invalid_transports.presence,
aaguid: aaguid,
Copy link
Member

Choose a reason for hiding this comment

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

Can we add corresponding YARDoc updates for method documentation in analytics_events.rb?

I would have also expected this to be enforced by UndocumentedParamsChecker and fail the build, which makes me worried we might not have full coverage for the code which would log this event.

}.compact
end
end
2 changes: 2 additions & 0 deletions app/forms/webauthn_verification_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ def valid_assertion_response?
client_data_json.blank? ||
signature.blank? ||
challenge.blank?

WebAuthn::AuthenticatorAssertionResponse.new(
authenticator_data: Base64.decode64(authenticator_data),
client_data_json: Base64.decode64(client_data_json),
Expand Down Expand Up @@ -165,6 +166,7 @@ def extra_analytics_attributes
{
webauthn_configuration_id: webauthn_configuration&.id,
frontend_error: webauthn_error.presence,
webauthn_aaguid: webauthn_configuration&.aaguid,
}.compact
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class AddAaguidToWebauthnConfiguration < ActiveRecord::Migration[7.1]
def change
add_column :webauthn_configurations, :aaguid, :string
end
end
3 changes: 2 additions & 1 deletion db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema[7.1].define(version: 2024_08_22_122355) do
ActiveRecord::Schema[7.1].define(version: 2024_08_28_182041) do
# These are extensions that must be enabled in order to support this database
enable_extension "citext"
enable_extension "pg_stat_statements"
Expand Down Expand Up @@ -656,6 +656,7 @@
t.boolean "platform_authenticator"
t.string "transports", array: true
t.jsonb "authenticator_data_flags"
t.string "aaguid"
t.index ["user_id"], name: "index_webauthn_configurations_on_user_id"
end

Expand Down
73 changes: 44 additions & 29 deletions spec/forms/webauthn_setup_form_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,10 @@
let(:user_session) { { webauthn_challenge: webauthn_challenge } }
let(:device_name) { 'Chrome 119 on macOS' }
let(:domain_name) { 'localhost:3000' }
let(:attestation) { attestation_object }
let(:params) do
{
attestation_object: attestation_object,
attestation_object: attestation,
client_data_json: setup_client_data_json,
name: 'mykey',
platform_authenticator: false,
Expand All @@ -26,42 +27,51 @@

describe '#submit' do
context 'when the input is valid' do
it 'returns FormResponse with success: true and creates a webauthn configuration' do
extra_attributes = {
enabled_mfa_methods_count: 1,
mfa_method_counts: { webauthn: 1 },
multi_factor_auth_method: 'webauthn',
authenticator_data_flags: {
up: true,
uv: false,
be: true,
bs: true,
at: false,
ed: true,
},
pii_like_keypaths: [[:mfa_method_counts, :phone]],
}
context 'security key' do
it 'returns FormResponse with success: true and creates a webauthn configuration' do
extra_attributes = {
enabled_mfa_methods_count: 1,
mfa_method_counts: { webauthn: 1 },
multi_factor_auth_method: 'webauthn',
authenticator_data_flags: {
up: true,
uv: false,
be: true,
bs: true,
at: false,
ed: true,
},
pii_like_keypaths: [[:mfa_method_counts, :phone]],
}

expect(subject.submit(params).to_h).to eq(
success: true,
errors: {},
**extra_attributes,
)
expect(subject.submit(params).to_h).to eq(
success: true,
errors: {},
**extra_attributes,
)

user.reload
user.reload

expect(user.webauthn_configurations.roaming_authenticators.count).to eq(1)
expect(user.webauthn_configurations.roaming_authenticators.first.transports).to eq(['usb'])
end
expect(user.webauthn_configurations.roaming_authenticators.count).to eq(1)
expect(user.webauthn_configurations.roaming_authenticators.first.transports).
to eq(['usb'])
end

it 'sends a recovery information changed event' do
expect(PushNotification::HttpPush).to receive(:deliver).
with(PushNotification::RecoveryInformationChangedEvent.new(user: user))
it 'sends a recovery information changed event' do
expect(PushNotification::HttpPush).to receive(:deliver).
with(PushNotification::RecoveryInformationChangedEvent.new(user: user))

subject.submit(params)
subject.submit(params)
end

it 'does not contains uuid' do
result = subject.submit(params)
expect(result.extra[:aaguid]).to eq nil
end
end

context 'with platform authenticator' do
let(:attestation) { platform_auth_attestation_object }
let(:params) do
super().merge(platform_authenticator: true, transports: 'internal,hybrid')
end
Expand Down Expand Up @@ -114,6 +124,11 @@
expect(result.to_h[:authenticator_data_flags]).to be_nil
end
end

it 'contains uuid' do
result = subject.submit(params)
expect(result.extra[:aaguid]).to eq aaguid
end
end

context 'with invalid transports' do
Expand Down
16 changes: 11 additions & 5 deletions spec/forms/webauthn_verification_form_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
let(:screen_lock_error) { nil }
let(:platform_authenticator) { false }
let(:client_data_json) { verification_client_data_json }
let(:webauthn_aaguid) { nil }
let!(:webauthn_configuration) do
return if !user
create(
Expand All @@ -18,6 +19,7 @@
credential_id: credential_id,
credential_public_key: credential_public_key,
platform_authenticator: platform_authenticator,
aaguid: webauthn_aaguid,
)
end

Expand Down Expand Up @@ -45,20 +47,24 @@
subject(:result) { form.submit }

context 'when the input is valid' do
it 'returns successful result' do
expect(result.to_h).to eq(
success: true,
webauthn_configuration_id: webauthn_configuration.id,
)
context 'security key' do
it 'returns successful result' do
expect(result.to_h).to eq(
success: true,
webauthn_configuration_id: webauthn_configuration.id,
)
end
end

context 'for platform authenticator' do
let(:platform_authenticator) { true }
let(:webauthn_aaguid) { aaguid }

it 'returns successful result' do
expect(result.to_h).to eq(
success: true,
webauthn_configuration_id: webauthn_configuration.id,
webauthn_aaguid: aaguid,
)
end
end
Expand Down
13 changes: 13 additions & 0 deletions spec/support/features/webauthn_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,15 @@ def attestation_object
HEREDOC
end

def platform_auth_attestation_object
<<~HEREDOC.chomp
o2NmbXRkbm9uZWdhdHRTdG10oGhhdXRoRGF0YVikSZYN5YgOjGh0NBcPZHZgW4/krrmihjLHmVzz
uoMdl2NFAAAAAK3OAAI1vMYKZIsLJfHwVQMAIOa31Ugh6EPoj4z6b+ibq6rVF1CZ9ygzSNvMrFmY
aPLtpQECAyYgASFYIO6a1uIfDkbqg/pm7bHZG0oRGyCEuWZrCWd2v/2IqXCaIlggKQEHbAiyBZxS
1HSBwwdjNCE4prYoHdzJWQILvDrIySo=
HEREDOC
end

def setup_client_data_json
<<~HEREDOC.chomp
eyJjaGFsbGVuZ2UiOiJncjEycndSVVVIWnFvNkZFSV9ZbEFnIiwibmV3X2tleXNfbWF5X2JlX2
Expand All @@ -198,6 +207,10 @@ def authenticator_data
'SZYN5YgOjGh0NBcPZHZgW4/krrmihjLHmVzzuoMdl2MBAAAAcQ=='
end

def aaguid
'adce0002-35bc-c60a-648b-0b25f1f05503'
end

def signature
<<~HEREDOC.chomp
MEYCIQC7VHQpZasv8URBC/VYKWcuv4MrmV82UfsESKTGgV3r+QIhAO8iAduYC7XDHJjpKkrSKb
Expand Down