Deployment to production: patch for 1.008#1547
Closed
gemfarmer wants to merge 72 commits intostages/prodfrom
Closed
Deployment to production: patch for 1.008#1547gemfarmer wants to merge 72 commits intostages/prodfrom
gemfarmer wants to merge 72 commits intostages/prodfrom
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.
…-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
Merge RC 1.008 to staging
**Why**: For consistency with the behavior during account creation cancellation. **How**: - Add a new controller and route for signing the user out without having to go through `SamlIdpController#logout` since there is no single logout to be performed in this scenario - Replace the URL of the Cancel link to point to this new route (cherry picked from commit ae68355 #1526)
**Why**: We noticed some users were running into an exception while trying to set their password. It turned out they were signing up for multiple accounts in the same session, but using different browsers, which is a common use case for users starting the process in a mobile app, and then confirming their email in a separate app or mobile browser. The bug was that we were only storing the request in the DB if the session didn't already contain the `request_id`, and we also deleted the request after a successful redirect back to the SP. Here's a concrete example, which I wrote a test for: - User starts by entering their email in browser 1, request 1 is stored in the DB and in the session - User confirms their email in browser 2, which looks up request 1 in the DB, and stores it in the session - User continues the account creation process successfully and continues on to the SP. At this point, request 1 is deleted from the DB - User goes back to browser 1 and makes a new request while the original session is still alive. Since the session is still alive and still contains request 1, the app doesn't store this new request, but the same `request_id` is passed on to the email confirmation - User confirms their email in browser 2, which passes because we don't validate the request_id at this point. - User tries to create their password, but when they submit the form, they get an exception because the app is trying to look up the request in the DB that matches the orginal `request_id`, but it was deleted earlier. **How**: Always store a request in the DB every time a new request is made by the SP. Don't reuse requests. The reason we reused requests before was to make it easier to delete requests in the DB that were no longer needed, but I obviously didn't think it through. We'll need to come up with a rake task or some other way to prevent the `ServiceProviderRequests` table from growing too large. (cherry picked from commit e38a3df #1542)
Contributor
Author
|
Blocked until #1547 is merged |
Contributor
|
This is a much bigger diff than the staging PR (#1546) -- is that because the production branch is farther behind than staging? |
Contributor
Author
|
I guess so. I'm not entirely sure why there is such a large gap between |
**Why**: New Relic reported errors in `CompletionsController#update` that were due to users starting the account creation process in the CBP Jobs iPhone app, but opened their email and continued the account creation in a desktop browser. Because they started in the iPhone app, the OIDC redirect URI was set to the iPhone app, so when they clicked "Continue" on the Completions page, the redirect failed and made another request to CompletionsController#update, but since `session[:sp]` was deleted, the app threw an exception because it was trying to redirect to `sp_session[:request_url]`, which was `nil`. **How**: Compare the user agent to the redirect URI. If the URI doesn't start with `http`, we can assume it is a mobile app. If the URI and user agent match, i.e. if the URI is for a web app, and the user agent is a desktop device, or if the URI is for a mobile app and the user agent is a smartphone or tablet, then we redirect the user back to the SP. If they don't match, we sign the user out, redirect them to the sign in page, and display a flash message instructing them to go back to the app on their smartphone or tablet and sign in to continue.
**Why**: Don't mention smartphone or tablet to keep it simple, and also because they can sign in via the web app on the desktop as well.
Deployment to staging. 1.008.1 and 1.008.2
Contributor
Author
|
Deployed! Closing this issue |
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
9 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.
Deploy patch to production for 1.008. These changes will also be part 1.009.
Changes include: