Skip to content

Move Aaguid call in webauthn Setup form #11248

Merged
mdiarra3 merged 5 commits intomainfrom
update-aaguid-call
Sep 19, 2024
Merged

Move Aaguid call in webauthn Setup form #11248
mdiarra3 merged 5 commits intomainfrom
update-aaguid-call

Conversation

@mdiarra3
Copy link
Contributor

No description provided.

@mdiarra3 mdiarra3 requested a review from a team September 17, 2024 14:48
@aduth
Copy link
Contributor

aduth commented Sep 17, 2024

Context: #11138 (comment)

Looks like there's an issue to be resolved from the build.

@mdiarra3
Copy link
Contributor Author

It looks like attestation response could also throw an error when we call for authenticator data. So we would need to do a begin and rescue clause as well.

@aduth
Copy link
Contributor

aduth commented Sep 17, 2024

It looks like attestation response could also throw an error when we call for authenticator data. So we would need to do a begin and rescue clause as well.

I'm not really sure I follow from the code how that could happen, since attestation_response is an attr_reader so shouldn't have any side-effects on its own.

@aduth
Copy link
Contributor

aduth commented Sep 17, 2024

Oh, I see, it's not the call to attestation_response, it's attestation_response's internal logic when we drill down into authenticator_data, if it's not a valid response.

Comment on lines +163 to +169
def aaguid
begin
@aaguid ||= attestation_response&.authenticator_data&.aaguid
rescue StandardError
nil
end
end
Copy link
Contributor

@aduth aduth Sep 17, 2024

Choose a reason for hiding this comment

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

  1. We can use a method-level rescue to flatten this.
  2. I think we can be more targeted in the error we're looking for. Based on where I suspect it's being raised, maybe we can rescue WebAuthn::AuthenticatorDataFormatError ?
  3. Memoizing won't do much here if the value is nil, since ||= will still evaluate for a nil value
Suggested change
def aaguid
begin
@aaguid ||= attestation_response&.authenticator_data&.aaguid
rescue StandardError
nil
end
end
def aaguid
attestation_response&.authenticator_data&.aaguid
rescue WebAuthn::AuthenticatorDataFormatError
nil
end

Comment on lines +165 to +166
rescue WebAuthn::AuthenticatorDataFormatError
nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok looks like we are getting StandardError after all? 🤷

From adding a binding.pry and running failing spec rspec ./spec/forms/webauthn_setup_form_spec.rb:226.

From: /Users/andrewmduthie/Documents/Code/identity-idp/app/forms/webauthn_setup_form.rb:167 WebauthnSetupForm#aaguid:

    163: def aaguid
    164:   attestation_response&.authenticator_data&.aaguid
    165: rescue => e
    166:   binding.pry
 => 167:   nil
    168: end

[1] pry(#<WebauthnSetupForm>)> e
=> #<StandardError: StandardError>

Curious where this is being raised...

Suggested change
rescue WebAuthn::AuthenticatorDataFormatError
nil
rescue StandardError
nil

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh.... it's because it's what we stub 😕

before do
allow(WebAuthn::AttestationStatement).to receive(:from).and_raise(StandardError)
end

I'm not sure that stub accurately reflects reality.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea I think we can change it to reflect webauthn data error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But it doesnt explain why the other tests are failing as well.

Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@mdiarra3 mdiarra3 merged commit 983e216 into main Sep 19, 2024
@mdiarra3 mdiarra3 deleted the update-aaguid-call branch September 19, 2024 14:30
AShukla-GSA pushed a commit that referenced this pull request Sep 30, 2024
* changelog: Internal, Webauthn Setup, move aaguid call in webauthn setup form

* remove aaguid

* do rescue for aaguid

* rescue authenticator data format

* change to standard erorr
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants