Skip to content

Align PIV/CAC setup-after-sign-in specs to user behavior#10939

Merged
aduth merged 2 commits intomainfrom
aduth-piv-cac-setup-after-signin-specs
Jul 17, 2024
Merged

Align PIV/CAC setup-after-sign-in specs to user behavior#10939
aduth merged 2 commits intomainfrom
aduth-piv-cac-setup-after-signin-specs

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Jul 15, 2024

🎫 Ticket

Supports LG-13477 (#10918)

🛠 Summary of changes

Updates feature specs targeting the expected behavior for prompting a user to add a PIV to their account after signing in when having previously failed logging in with PIV due to the card not being associated with a user.

Specifically:

  • Shifts specs to a new file, since there's several scenarios related to this behavior, and this helps keep the size of features/users/sign_in_spec.rb in check
  • Improves stub_piv_cac_service to behave more realistically to the deployed user behavior
    • Previously, the tests would manually "visit" the PKI service with the assumption that the user would always be redirected back to the current URL. This is not true for PIV/CAC setup after sign-in, which submits to the default PIV/CAC enrollment route and would therefore be redirected back there after returning from the PKI service
  • Removes unused code related to processing a token returned by the PKI service from PivCacSetupFromSignInController
    • As described under the previous bullet, these routes are never actually visited in the current live experience, and instead the user goes through /present_piv_cac (PivCacAuthenticationSetupController)

📜 Testing Plan

Verify specs pass:

rspec spec/features/sign_in/setup_piv_cac_after_sign_in_spec.rb

Ensure no regressions in the expected behavior:

  1. Prerequisite: Remove any PIV/CAC association with existing account
  2. Click “Sign in with your government employee ID”
  3. Click “Insert your PIV/CAC”
  4. Authenticate with your PIV
  5. See error message “Your PIV/CAC is not connected to an account”
  6. Click “Sign in” (or “Go back to sign in”)
  7. Proceed to sign in to your account
  8. See screen “Add your PIV or CAC”
  9. Enter a nickname in the “PIV/CAC nickname” field
  10. Click “Add PIV/CAC card”
  11. Observe you're still redirected to the account page successfully upon enrollment

aduth added 2 commits July 15, 2024 15:51
changelog: Internal, Automated Testing, Align PIV/CAC setup-after-sign-in specs to user behavior
@aduth
Copy link
Contributor Author

aduth commented Jul 15, 2024

I'm intentionally trying to keep the scope here limited, but follow-on tasks include:

  • Removing the visit_piv_cac_service spec helper and updating existing references to use click_on ... + follow_piv_cac_redirect pattern established here instead

@aduth aduth marked this pull request as ready for review July 15, 2024 20:47
@aduth aduth requested a review from a team July 15, 2024 20:47
def multi_factor_auth_added_piv_cac(
enabled_mfa_methods_count:,
in_account_creation_flow:,
method_name: :piv_cac,
Copy link
Contributor

Choose a reason for hiding this comment

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

With the event being mult_factor_auth_added_piv_cac, what information would be captured in method_name?

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'm honestly not really sure I see the value in logging method_name. I kept it here just for parity with how it worked previously. I could imagine a potential query where someone filters like filter properties.event_properties.method_name = 'piv_cac' without an event name, since some other events follow this pattern as well (e.g. 'Multi-Factor Authentication: Added phone'). But I kinda doubt that's common? We could create a follow-up ticket to investigate if we're querying that way and remove the redundant event property if unused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And to clarify, this change was necessary because it was flagged by our undocumented analytics parameters checker. It wasn't flagged previously because sign_in_spec.rb still exempts itself from these checks, but creating a new feature test file without the exemptions caused it to be caught.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is just the default, was added a long time ago but I do wonder if we still need it.

@aduth aduth merged commit ad97bfe into main Jul 17, 2024
@aduth aduth deleted the aduth-piv-cac-setup-after-signin-specs branch July 17, 2024 19:01
mitchellhenke pushed a commit that referenced this pull request Jul 31, 2024
* Align PIV/CAC setup-after-sign-in specs to user behavior

changelog: Internal, Automated Testing, Align PIV/CAC setup-after-sign-in specs to user behavior

* Remove unused code for PIV/CAC token processing
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.

3 participants