Skip to content

Add analytics property for WebAuthn sign-in frontend error#9611

Merged
aduth merged 2 commits intomainfrom
aduth-webauthn-frontend-error
Nov 17, 2023
Merged

Add analytics property for WebAuthn sign-in frontend error#9611
aduth merged 2 commits intomainfrom
aduth-webauthn-frontend-error

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Nov 16, 2023

🎫 Ticket

Related to LG-11399

🛠 Summary of changes

This adds a new property frontend_error to unsuccessful WebAuthn sign-ins.

While it doesn't appear this property is actively used in any dashboards, the availability of this value inadvertently regressed as part of #9572, which previously logged the specific error name as part of the error_details property.

An alternative implementation could try to incorporate this as the "type" of the error detail, but the case mismatch between e.g. incoming NotAllowedError and expected detail type formatting of not_allowed_error could create some confusion.

📜 Testing Plan

  1. (Prerequisite) Have an account with Security Key or Face or Touch Unlock as an associated MFA
  2. Run make watch_events in a separate terminal process
  3. Go to http://localhost:3000
  4. Sign in
  5. (If not prompted for MFA, click "Forget all browsers", confirm, sign out, and start over from Step 3)
  6. When prompted WebAuthn, click "Use face or touch unlock" or "Use security key"
  7. Cancel the dialog that's shown
  8. Observe in event log that you see "Multi-Factor Authentication" event including "NotAllowedError"

Copy link
Contributor Author

@aduth aduth Nov 16, 2023

Choose a reason for hiding this comment

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

The "alternative" approach described in the original comment could look something like...

Suggested change
error_details: { webauthn_error: { webauthn_error: true } },
error_details: { webauthn_error: { NotAllowedError: true } },

Or:

Suggested change
error_details: { webauthn_error: { webauthn_error: true } },
error_details: { webauthn_error: { not_allowed_error: true } },

Or some extra processing to trim the redundant suffix:

Suggested change
error_details: { webauthn_error: { webauthn_error: true } },
error_details: { webauthn_error: { not_allowed: true } },

Copy link
Contributor

Choose a reason for hiding this comment

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

Does the third version leave the door open for other types of webauthn_errors? I'm not sure if that sort of scalability is likely but if it is I think that's a strong candidate. It seems pretty descriptive regardless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think I like it as far as providing the actual detail relevant for the field, but my main concern is that it's not obvious the mapping between the original JavaScript-derived error name and the resulting logged key name, which would I expect would be derived by some opaque logic like javascript_error_name.underscore.tr('_error', ''). And the "Error" suffix of JavaScript error names is only a standard convention, not strictly mandatory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm actually thinking I might go a different direction with this and use ActiveRecord's built-in absence validator for this, which seems like a nice compromise between the challenges above and trying to avoid having a redundant webauthn_error subproperty.

I'll plan tackle that in a quick follow-on pull request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll plan tackle that in a quick follow-on pull request.

See #9614

Copy link
Contributor Author

@aduth aduth Nov 16, 2023

Choose a reason for hiding this comment

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

If this is being logged on the Multi-Factor Authentication event, it should probably be documented in the arguments for that method, and qualified as relating specifically to "WebAuthn".

Though maybe that extra friction is enough to warrant persuing the alternative instead.

@aduth aduth requested a review from a team November 17, 2023 13:16
@aduth
Copy link
Contributor Author

aduth commented Nov 17, 2023

I'm open to suggestions, but fairly happy with the solution proposed here currently, and I'd like to try to get this included in the next production deploy to ensure continuity of the data for error types.

Copy link
Contributor

@kevinsmaster5 kevinsmaster5 left a comment

Choose a reason for hiding this comment

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

Looks good! My vote for error_details ver no. 3
Tested locally
Screenshot 2023-11-17 at 10 01 10 AM (2)

changelog: Internal, Analytics, Add analytics property for WebAuthn sign-in frontend error
@aduth aduth force-pushed the aduth-webauthn-frontend-error branch from 37338a9 to aadb3d4 Compare November 17, 2023 16:21
@aduth aduth merged commit 7f80d9d into main Nov 17, 2023
@aduth aduth deleted the aduth-webauthn-frontend-error branch November 17, 2023 16:44
aduth added a commit that referenced this pull request Nov 21, 2023
* LG-11553 Remove recovery PII re-encryption from `PersonalKeyVerificationController` (#9602)

The `PersonalKeyVerificationController` is used to verify a personal key as an MFA method and allow a user to sign in. When this is done a new personal key is issued.

This controller had code for re-encrypting the users profile with the newly issued personal key. However, a user with an active profile was never able to reach this path. The `check_personal_key_enabled` calls `TwoFactorAuthentication::PersonalKeyPolicy#enabled?`. This method returns false if the user has any profiles.

Since this code path is unreachable this commit removes it.

I was not able to find any tests covering this re-encryption behavior.

[skip changelog]

* LG-11573: Add RISC events for account suspension, account reinstatement (#9594)

* LG-11573: Add RISC events for account suspension, account reinstatement

changelog: Internal, User suspension, Add RISC events for user suspension

* feedback

* feedback

* feedback name changed

* LG-11534 Load the active profile from the session on broken personal key (#9601)

In #9509 we added the ability to specify which profile to fetch PII from when reading PII from the session.

This commit uses the active profiles PII when encrypting recovery PII for the active profile when the active profile has a broken personal key.

changelog: Internal, Pending and active profile, The active profile PII is fetched with the PII cacher when a user with a broken personal key on their active profile signs in.

* Restructure analytics error_details as hash (#9572)

changelog: Internal, Analytics, Adjust format of analytics logging to improve querying support

* Remove failure_reason from Attempts API stub (#9576)

* Remove Tracker#parse_failure_reason
**Why**: Simplifies codebase

* Remove failure_reason entirely
**Why**: Inconsitent structure, and completely unused

* Remove some more unused fake tracker code

changelog: Internal, Source code, Clean up unused error tracking code

* Use Icon List component for requested attributes consent (#9555)

* Support tag options for IconListItemComponent

* Use IconListComponent for requested attributes

changelog: Internal, Components, Use consistent design system component for icon list

* Use ButtonComponent for auth confirmation

* Remove seemingly-unnecessary assertion content scoping

* Update AAMVA test scripts (#9608)

- Require optparse, test it
- Use pretty-printing

changelog: Internal, Testing, Add AAMVA test script covered by specs

* LG 11432 Prevent duplicate F/T setup if user hits back button on second MFA prompt (#9587)

* changelog: User-Facing Improvements, Webauthn, Prevent duplicate F/T setup on second MFA prompt

* add spec coverage for platform auth redirect

* move test to a function

* move conditional to before_action

* clean up validate platform authenticator method

* LG 11145 Break up MFA selection presenter classes for Phone Presenters (#9560)

* changelog: Internal, tech debt, Break up MFA presenter class for phone

* split phone, voice, and sms presenter classes up

* add tests cases for sign_in and set_up phone presenter class

* split setup signin presenter spec for voice and sms

* lint fix

* remove old phone selection presenter spec

* rename phone presenter in spec

* remove unneeded configuration variable, leverage user for type method

* remove info method from phone sub classes

* update options presenter spec with newly split classes

* merge sms and voice presenters

* revise specs according to merged classes

* remove deprecated spec and lint fix

* change info to switch and fix regression with disabled? method

* lint fix

* fix spec

* remove deprecated translations from setup presenter

* move reader :method to phone sign in presenter

* fix lint

* fix lint

* clean up selection presenter class

* remove unneeded configuration setting

* remove configuration from set up presenter spec

* add sms and voice outage spec, standardize spec syntax

* clarify some syntax

* Refactor WebauthnVerificationForm to handle error messages (#9613)

changelog: Internal, Code Quality, Move error messages for WebAuthn verification to form class

* Add analytics property for WebAuthn sign-in frontend error (#9611)

* Add analytics property for WebAuthn sign-in frontend error

changelog: Internal, Analytics, Add analytics property for WebAuthn sign-in frontend error

* Document frontend_error

* Finalize cleanup for MFA selection presenters (#9612)

* Update missed references to SignIn base selection presenter

* Remove unused SelectionPresenter

* Swap base presenters to raising NotImplementedError

* Swap phone method comparison to use symbol

Consistency with logic elsewhere in class

* Raise on missing type method in base presenter classes

* Consolidate presenter classes to define only type method

* Rename phone selection presenter method to delivery_method

* Add changelog

changelog: Internal, Code Quality, Remove unused code related to MFA selection presenters

* Update call sites to use new delivery_method constructor argument

* Use setup-specific string for WebAuthn setup presenter

* Use ActiveRecord built-in validator for WebAuthn error validation (#9614)

* Use ActiveRecord built-in validator for WebAuthn error validation

changelog: Internal, Code Quality, Move error messages for WebAuthn verification to form class

* Update webauthn_verification_controller_spec.rb

* LG-11535 Encrypt the pending and active profile when a user updates their password (#9607)

In #9509 we added the ability to specify which profile to fetch PII from when reading PII from the session.

As a result of this change we can encrypt both the pending and active profile with user's password. This means both profiles can be decrypted on sign-in.

There are consequences for recovery with personal key. When the user changes their password and their data is encrypted their recovery PII is also encrypted as a consequence. The pending and active profile are both encrypted with a different personal key. In this commit I elected to display the pending profile personal key to the user so that profile is recoverable when it becomes active.

[skip changelog]

* LG-11542 Texas conditional hint text for id number (#9600)

* add hint for tx

* change name of file

* change pack name

* refactor show or hide hint function

* update state guidance spec

* changelog: User-Facing Improvements, State id page, tx specific id numner hint text

* use text content instead of inner html

* add translations for fr and es

* lint fix for translations

* update string

* normalize yaml

* add punctuation to en translation

* update es translation

* lint fix

* Update Prettier to v3.1 (#9618)

changelog: Internal, Code Formatting, Update Prettier code formatter to latest version

* Log service_provider for RackAttack events (#9620)

* Log service_provider for RackAttack events

changelog: Internal, Logging, Log service_provider for RackAttack events

* Update spec/requests/rack_attack_spec.rb

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>

* Update spec/requests/rack_attack_spec.rb

Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>

---------

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>

* Remove GPO verification rake task (#9621)

This was just glue to support running the job, and is no longer needed.

[skip changelog]

* LG-11520: Enable daily GPO expiration job (#9622)

changelog: Internal, Identity verification, Automatically expire inactive GPO profiles.

* Update State ID hint, move HTML out of translation strings into template (#9617)

**Why**: Keeping complex HTML inside translation strings is error-prone
and can be easy to accidentally mis-align across languages

* Refactor to support toggling for state-specific hints

- Add HTML view specs too

changelog: Accessibility, Screen reader support, Bring back screenreader support for State ID number hint

* LG-11477: doc class error for some identification card (#9597)

* LG-11477: Document issuer type check.

When document type is classified as 'Identification Card', it may mean a State issued identification card. It can also include US Passport Card,  VHIC, US Social Security Card, TWIC etc, all of them are not issued by a State.

So we also need to check whether the issuer type is StateProvince, not by a Country.

Driver's License/Identification Card issued by PR, VI, MP etc, do have issuer type of StateProvince.

So by checking IssuerType of the document, we effectively excludes IDs issued at Federal level which are not supported document.

* LG-11477: update test.

* LG-11477: generate error for doc type before metrics.

* LG-11477: use constant for string.

* changelog:  User-Facing Improvements, Doc Auth, Error for unsupported documents.

* LG-11477:  id type support logic update for mock client response.

* LG-11477: update fixtures with doc issuer type field.

* LG-11477: short circuit logic, no need to continue to check.

* LG-11477: null safe operation.

* LG-11477: minor code format change.

* LG-11477: no need in rails.

* LG-11477: test clean up.

* Update Stylelint config dependencies in preparation for release publish (#9619)

* Update stylelint-config-recommended-scss

changelog: Internal, Dependencies, Update dependencies to latest versions

* Prepare 3.0.0 release

* LG-11435: Track frontend analytics for changed country in phone input (#9616)

* Update JavaScript analytics examples to use new naming convention

* Add new event naming convention support to FrontendLogController

* LG-11435: Track frontend analytics for changed country in phone input

changelog: Internal, Analytics, Track event when country changed in phone input

* Use symbol list syntax

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>

* Improve handling of undefined country code

---------

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>

* LG-11025:  Only log acuantCaptureMode when using acuant SDK (#9610)

* log acuantCaptureMode as null when an image is uploaded

Co-authored-by: Dawei Wang<dawei.wang@gsa.gov>

* update spec to expect acuantCaptureMode is null for images uploaded during doc auth

* changelog: Internal, Analytics, Image added logs define acuantCaptureMode only if acuant is used to capture the image

* update js test upload test

* happy linting

* nullify  acuantCaptureMode in getAddAttemptAnalyticsPayload when image source is upload

* Prepare publish normalize-yaml to NPM (#9627)

* Bump YAML dependency to latest

* Bump normalize-yaml to 2.0.0

changelog: Internal, Packages, Publish `@18f/identity-normalize-yaml@2.0.0`

* Restore width collapse for unstyled buttons (#9632)

changelog: Bug Fixes, Buttons, Fix appearance of inline buttons

---------

Co-authored-by: Jonathan Hooper <jonathan.hooper@gsa.gov>
Co-authored-by: Osman Latif <109746710+olatifflexion@users.noreply.github.com>
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
Co-authored-by: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com>
Co-authored-by: Shannon A <20867088+svalexander@users.noreply.github.com>
Co-authored-by: Mitchell Henke <mitchell.henke@gsa.gov>
Co-authored-by: Matt Hinz <matt.hinz@gsa.gov>
Co-authored-by: dawei-nava <130466753+dawei-nava@users.noreply.github.com>
Co-authored-by: Amir Reavis-Bey <amir.reavis-bey@gsa.gov>
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