Hi! Before submitting your PR for review, and/or before merging it, please go through the checklists below. These represent the more critical elements of our code quality guidelines. The rest of the list can be found in [CONTRIBUTING.md]#2550
Merged
gregory-casamento merged 178 commits intomasterfrom Sep 24, 2018
Conversation
…c-2017-06-12 Adds pull request #1460 to release candidate
Deploy int to staging
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)
**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
Deploy stages/rc-2017-08-07 to production
…7-to-staging Deploy stages/rc-2017-08-07 to staging
Deploy rc-2017-08-21 to staging
Deploy rc-2017-08-21 to production
**Why**: Error in production due to "request.protocol" being nil, using the asset_url helper in the view should provide access to the request
Deploy rc-2017-09-04 to staging
Deploy stages/rc-2017-09-04-patch-1 to prod
Promote RC 39 to staging
Promote RC 39 to production
Disable international voice dialing (cherry picked from commit 26c9202)
Enable voice calling to Mexico (cherry picked from commit 657c442)
Promote stages/rc-2017-10-02 to staging
Promote RC-40 to production
**Why**: CSRF issues in prod (cherry picked from commit d899452e696559868bd3e47091a0be6019997a91)
Deploy RC-40.1 to staging
**Why**: The session encryptor was not unlocking the user access keys when it was initializing. This meant that if an old session that was created with the old non-deterministic access key was decrypted, that key would be used to unlock the session encryptor's user access key, setting the random_r to the value of the random_r for the non-deterministic key.
**Why**: Duping the key means that when encrypting, the key will use it's randomly generated random_r, and when decrypting, it will derive random_r from the ciphertext. This means the session encryptor can use non-deterministic user access keys to encrypt and decrypt session data.
**Why**: We are moving away from the user access keys in favor of 2L-KMS which involves aes encrypted ciphertexts wrapped by KMS
**Why**: Because it doesn't make sense to generate 10 byte salts and digest them when we can generate 32 byte salts directly
Deploy RC 63.1 to int
Deploy RC 63.2 to int
This reverts commit cb67632.
Deploy RC 63.3 to int
This reverts commit 48c848a.
**Why**: We can only send 4k of data to KMS for encryption. We need to make sure we don't exceed that regardless of which method we use so we know we can use KMS without errors. **How**: Raise an argument error regardless of the encyption method.
Deploy RC 63.4 to int
Deploy RC 64 to int
Deploy RC 65 to int
**Why**: We want to start with the right set of vendors for proofing. **How**: We initialize `@vendor` to `nil` rather than a truthy value.
Int: Fix Idv::Proofer vendor initialization
**Why**: Sometimes, we have an anonymous user. They don't have any configured phones. **How**: Add a method that returns `nil`
**Why**: To prevent a 500 error when a user visits the `/account_reset/confirm_request` path while signed out
Deploy RC 65 patch 2 to Int
Deploy stages/rc-2018-09-13 to int
**Why**: We are getting an error raised if no phone configuration exists. **How**: Check for nil and return a blank string.
Deploy RC 66 Patch 1 to int
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
16 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.
When adding a new controller that requires the user to be fully
authenticated, make sure to add
before_action :confirm_two_factor_authenticatedas the first callback.
Unsafe migrations are implemented over several PRs and over several
deploys to avoid production errors. The strong_migrations gem
will warn you about unsafe migrations and has great step-by-step instructions
for various scenarios.
Indexes were added if necessary. This article provides a good overview
of indexes in Rails.
Verified that the changes don't affect other apps (such as the dashboard)
When relevant, a rake task is created to populate the necessary DB columns
in the various environments right before deploying, taking into account the users
who might not have interacted with this column yet (such as users who have not
set a password yet)
Migrations against existing tables have been tested against a copy of the
production database. See LG-228 Make migrations safer and more resilient #2127 for an example when a migration caused deployment
issues. In that case, all the migration did was add a new column and an index to
the Users table, which might seem innocuous.
The changes are compatible with data that was encrypted with the old code.
GET requests are not vulnerable to CSRF attacks (i.e. they don't change
state or result in destructive behavior).
When adding user data to the session, use the
user_sessionhelperinstead of the
sessionhelper so the data does not persist beyond the user'ssession.
Tests added for this feature/bug
Prefer feature/integration specs over controller specs
When adding code that reads data, write tests for nil values, empty strings,
and invalid inputs.