Merged
Conversation
**Why**: The URL changed
Update link to Help Center question
**Why**: When a user reactivates their account, the personal key entry and re encryption of pii are being split out into separate steps Add personal key form partial, new form, alert **Why**: The personal key input form is separate from the rest of the presentation. Required for the new personal key entry form. Added an excalaimation point in the alert notice to match designs and other alert messages Adds verify password class and emphemera **Why**: We are splitting out the account reactivation flow, which requires an additional controller, shared views, new tests and routes Removes old account reactivation files **Why**: The flow isn't the same and these files are uneeded Spec updates, manage_reactivate to reactivate **Why**: The old account reactivation code was removed, so we can safely move manage_reactivate_account to reactivate_account
Break account reactivation into two-step process
Updated the help copy after user changes their password.
Update en.yml
Updated intro for password reset
**Why**: A bug was discovered where an OIDC Service Provider's configuration included attributes that are only meant for SAML, namely `assertion_consumer_logout_service_url`. That attribute was set to an empty string, but the logic in `SingleLogoutHandler#slo_not_implemented_at_sp?` was checking for `nil?` only, which caused `SamlIdpController#logout` to proceed all the way to call `generate_slo_request` with a nil URL (instead of returning early at `finish_slo_at_idp`), which resulted in an exception in `SecureHeadersWhitelister.extract_domain` due to the `nil` URL. **How**: In addition to the empty SAML attribute, reproducing this bug also requires that the user have an active identity with the OIDC SP. In SAML, identities get deactivated during the logout process, but OIDC identities remain active (which could be another bug). This is why the spec signs the user in fully to the SP first to create the identity, then signs out, then signs in again, but cancels the sign in process on the 2FA screen. Using `blank?` instead of `nil?` fixes the bug.
Improve defensiveness of logout logic
Updated intro for password reset
Update SAML SP URLs for RRB
Update timeout modal style
**Why**: During UAT testing with Equifax, we came across some ID verification issues, some of which required changes to the equifax gem, and some of which required changes to the IdP code. **How**: - Remove extra analytics attributes for vendor reasons in the Financials and Phone steps because Equifax does not return any reasons for those verifications. - Update `vendor_params` so that all keys are Strings for consistency - Update equifax gem to latest commit on master
Update IdV to work with Equifax
**Why**: Saving invalid forms to the database causes errors
**Why**: To avoid validating the request twice. By validating earlier in the process (with `validate_authorize_form`), we know that once it reaches the `index` action, it is valid. If it is not valid, it will never reach the `index` action. This means we don't need to validate the request both in the form's `check_submit` and `submit` methods. We can remove the `check_submit` method and modify the existing `submit` method to only validate and return the FormResponse, and link the identity in the `index` action once the user has signed in. This is consistent with what we do with SAML: First we validate the SAML request, and if it's valid, then we proceed to the auth action, and once the user is fully authenticated, we link the identity. Bonus points: This fixes the ABC complexity offense of the `index` method.
Handle invalid OIDC requests before persisting
**Why**: Unused code does not belong in the repo, and it's affecting our test coverage metrics. Code Climate says none of this file is being hit when running the test suite.
Remove unused file
**Why**: The user should know that they must reactivate their account following a password reset. Using a modal to provide 'heads up' information matches our current UX. A fallback alert message is also provided when JS is disabled Spec changes **Why**: Satisfy PR feedback Removes manual otp code entry **Why**: Only one of the specs in `password_recovery_via_recovery_code_spec.rb` needs to go through the otp code entry page Un-nest personal key verification bail out form **Why**: Having a form nested in a form is bad. Add cancel link to no pkey warning modal **Why**: User should have the option to cancel the modal and verify their personal key once they are warned that lack of said key will require identity reverification Add specs for modal cancel and pkey verify cancel **Why**: We should have confidence that a user attempting to close the reverify warning modal or electing to reverify their info once on the personal key verification page can do so
**Why**: Use crockford encoding to make them case-insensitive and normalize confusable letters and numbers.
Add reactivation warning modal, fallback, specs
**Why**: To have a clean output locally when running `make lint`.
**Why**: Sessions haven't been using Marshal since May 2017 or so.
**Why**: This rake task was only meant to be used temporarily to encrypt plain text OTP secret keys in legacy DBs when we weren't encrypting the `otp_secret_key` yet.
Remove unused rake task
Remove unused Marshal code
Update gems with bummr
…-mail Make verify-by-mail codes less error-prone
**Why**: TTP requested a new logo be added
Use HTTP headers for i18n
Added new logo for cbp trusted traveler program
Revert "Added new logo for cbp trusted traveler program"
**Why**: Use the database rather than rack-attack
**Why**: - Since we don't enforce uniqueness of phone number, a malicious user could create multiple accounts with the same number and request OTPs from each account. We want to keep track of the request count based on the phone number used and then lock out all users who have that phone number. - To keep things simple, we use the same lockout period for both max OTP attempts and max OTP requests
Lock users out temporarily after too many OTPs
**Why**: We don't need it, and having a custom setter for it was duplicating code in the User model
Remove OtpRequestsTracker#encrypted_phone
**Why**: To avoid hardcoding keys in the user session across multiple controllers, and to present an interface for managing account reactivation session data Spec to test confirm_personal_key before_action **Why**: CC reported that branch of logic as untested
Use AccountReactivationSession object
**Why**: Instead of having to rely on devops to deploy Figaro config changes before we can deploy code to our lower envs, we can make the `lockout_period_in_minutes` optional, and set a default value for it.
Set a default value for lockout period
amoose
approved these changes
Jul 6, 2017
Contributor
amoose
left a comment
amoose
left a comment
There was a problem hiding this comment.
LGTM 🚢 % flickering spec in Travis
Contributor
Author
|
Huh. The commit that fixed that flickering spec must have been merged after the RC was branched out. I'll restart the build. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.