Use readonly production database config in console #1996

jmhooper · 2018-02-16T17:33:48Z

Why: So that console users in deployed environments will not be able
to modify records without explicitly enabling write access. Enabling
write access is done by setting ALLOW_CONSOLE_DB_WRITE_ACCESS=true.

How: This commit adds a class called ProductionDatabaseConfiguration
which is loaded in database.yml and used to determine the value of
configs such as the username, password, and pool size. When the rails
console is defined, it uses 2 new configs for user name and password
that should correspond to a read only user.

Hi! Before submitting your PR for review, and/or before merging it, please
go through the following checklist:

For DB changes, check for missing indexes, check to see if the changes
affect other apps (such as the dashboard), make sure the DB columns in the
various environments are properly populated, coordinate with devops, plan
migrations in separate steps.
For route changes, make sure GET requests don't change state or result in
destructive behavior. GET requests should only result in information being
read, not written.
For encryption changes, make sure it is compatible with data that was
encrypted with the old code.
For secrets changes, make sure to update the S3 secrets bucket with the
new configs in all environments.
Do not disable Rubocop or Reek offenses unless you are absolutely sure
they are false positives. If you're not sure how to fix the offense, please
ask a teammate.
When reading data, write tests for nil values, empty strings,
and invalid formats.
When calling redirect_to in a controller, use _url, not _path.
When adding user data to the session, use the user_session helper
instead of the session helper so the data does not persist beyond the user's
session.
When adding a new controller that requires the user to be fully
authenticated, make sure to add before_action :confirm_two_factor_authenticated.

jmhooper · 2018-02-16T17:37:04Z

We should not merge this until devops has provisioned a read only database user for all of the deployed environments and we have set the config in the secrets files to the username/password for that user.

Another thing to note is that I only made use of the readonly user when using the rails console, since that is how I read the issue. We can make the read only user the default though, and require all activities that wish to modify the db contents set the write access flag.

jgsmith-usds

Looks reasonable to me. Definitely should wait on DevOps provisioning before merging.

**Why**: So we can create a readonly database user to be used in the console after #1996

**Why**: So that console users in deployed environments will not be able to modify records without explicitly enabling write access. Enabling write access is done by setting ALLOW_CONSOLE_DB_WRITE_ACCESS=true. **How**: This commit adds a class called ProductionDatabaseConfiguration which is loaded in database.yml and used to determine the value of configs such as the username, password, and pool size. When the rails console is defined, it uses 2 new configs for user name and password that should correspond to a read only user.

* Add rake task to create readonly database user **Why**: So we can create a readonly database user to be used in the console after #1996 * Fall back to A record if MX record doesn't exist **Why**: Some email servers don't configure an MX record and rely on senders falling back to the A record, as recommended by the RFC. **How**: The email validation gem takes a parameter to do a check of the MX record and fall back to an A record if there is no MX record. * assume some jitter in timing when comparing time differences * LG-132 Updated OTP and Session timeouts (#2041) * Updated OTP and Session timeouts **Why**: In order to provide a bit more time for USAJobs users. * LG-132 Fixed tests **Why**: Changes to config broke a couple of them. * Select AWS SES region from a pool **Why**: We need to steadily increase our sending volume when moving between AWS regions to keep from being block as spam. This commit allows us to configure what percentage of mail gets sent from what region. * Validate phone number pattern before submission **Why**: We want to be able to leverage several phone validation options, so we are (1) moving the validation to the backend, (2) giving feedback to the user when the phone number is considered valid, (3) enabling "Send Code" only when the phone number is considered valid. **How**: We send a POST to an endpoint when the phone input field blurs or the country code changes. The result of the POST indicates if the number is valid. We can add more information later. * Validate Twilio requests to /api/voice/otp **Why**: To make sure that only requests that contain Twilio's signature are allowed. Reference: https://twilio-ruby.readthedocs.io/en/latest/usage/validation.html#rack-middleware * LG-123 Show OTP expiration to user on web page **Why**: So they know up front when it will expire. * LG-1 delete account: spanish and french translations (#2042) **Why**: Translations are not available for French and Spanish **How** Update the locale files

@monfresh

…lease 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) * Return to branded page when canceling sign in **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) * Allow multiple account creation in same session **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) * Respond with 404 for all nonexistent assets **Why**: Before, we were only responding with a 404 to requests for the HTML format, and we noticed 500 errors when browsers requested nonexistent PNG and CSS files. (cherry picked from commit 778ac7a #1543) * Detect when user agent doesn't match redirect_uri **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. * Update go_back_to_mobile_app i18n entry **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. * Resolve patch conflicts for 1.008.2 * Move _tag helpers into the template **Why**: Error in production due to "request.protocol" being nil, using the asset_url helper in the view should provide access to the request * Merge pull request #1691 from 18F/jmhooper-disable-international-voice Disable international voice dialing (cherry picked from commit 26c9202) * Merge pull request #1693 from 18F/jmhooper-enable-mexico-calling Enable voice calling to Mexico (cherry picked from commit 657c442) * Add deterministic decryption for session **Why**: CSRF issues in prod (cherry picked from commit d899452e696559868bd3e47091a0be6019997a91) * Unlock access key in session encryptor **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. * Use duped user access key in session_encryptor **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. * fix reek instance varaible assumption issue * rename the user access key method * fix session encryptor test * use duped access key in session encrypto * Add help text for TTP users **Why**: Many users don't realize they need to create a new login.gov account and that they can't use their old GOES credentials * Add help text for TTP users **Why**: Many users don't realize they need to create a new login.gov account and that they can't use their old GOES credentials (cherry picked from commit d393b5c) * Prepend set_locale before action **Why**: The set_locale before action sets the `I18n.locale` variable. Since that is part of the global state, it needs to be set before rendering any pages or pages will be rendered with the locale of the previous request. Prepending the `set_locale` action means it comes before actions that break the callback chain for things like CSRF errors and timeouts, assuring those pages are rendered in the correct language. (cherry picked from commit d6f5c74) * Use string key for Norway country dialing code **Why**: Because in yaml, `NO` without quotation marks means `false`. * Add link to service provider to account history **Why**: So that user's can navigate to an SP from their account history page. Note that this only applies to SP's that have a `return_to_sp_url`. SP's without do not have a link. * Handle nil redirect_uris gracefully **Why**: The IdP app expects the `redirect_uris` column in the ServiceProvider table to be an array and to have a default value of an empty array, but the dashboard app saves an empty value as `null`, and Rails does not have a built-in mechanism for converting the `nil` value into an empty array. **How**: Call `#compact` on the array before iterating through the URIs to get rid of any `nil` values since we don't care about them. * Add USAJOBS to staging **Why**: To begin testing, prepare for launch * Rescue from URI:Error instead of URI::BadURIError **Why**: So that the app will not break if there is a URI error adding the SP's url to the allowed origins in our CORS headers. Previously we were rescuing from `URI::InvalidURIError`, but the URI module has a number of URI error subclasses it can raise if there is an issue with the URI. ref: https://docs.ruby-lang.org/en/2.4.0/URI.html * Remove session & environment from exception email **Why**: They contain sensitive information, such as the session id and token. We can create a custom section later that includes some of the useful information from the environment section. * Add staging to allowed prefill envs __Why__ * We need to use staging to do some load testing however the code restricts that functionality to only certain envs. __How__ * Add idp.staging.login.gov to the list of exceptions. * Fix text to use prod domain __Why__ * The test was testing againg idp.staging.login.gov which after this change is an acceptable domain for prefilling. __How__ * Have the test use secure.login.gov instead. * Remove marketing site contact page from tests **Why**: Because it is external to the IDP code base. Changes to the production login.gov static site should not cause issues in our tests or our CI. * Add uniqueness index to identities table __Why__ * because each user should only have one identity per service provider __How__ * remove existing index and re-add it with uniqueness constraint * add unit tests for new index * change one sp name in user model spec to prevent duplicates * Use nil for empty user/pass in basic auth **Why**: Building a URI with an empty string for the username or password raises a URI::InvalidURIError * Capitalize TwiML tags; ignore slim-lint offense **Why**: TwiML tags are case-sensitive according to Twilio documentation, so we need to ignore the `TagCase` slim-lint offense for `app/views/voice/otp/show.xml.slim`. For some reason, using the exclude option under `TagCase` doesn't work, so I had to ignore the file globally. * Capitalize Response in the TwiML XML **Why**: The Response tag is part of the tags Twilio expects to be capitalized. * Redirect to SP after new personal key request **Why**: Requesting a new personal key during account creation should not prevent a user from going back to the SP. * Fix typo in USAJOBS name * Clarify custom message for USAJOBS customers **Why**: To make it clear that only people with existing USAJOBS accounts should use the same email address. * Make DB pool size configurable based on instance role __Why__ * Workers need a 5x larger pool than the IdP hosts hence we need a way to configure the pool size based on instance role. __How__ * Use a conditional to set the pool size based on the instance role type. * Translate USAJOBS "learn more" + other fixes **Why**: So that USAJOBS customers don't see "Not translated yet". * Translate TTP custom text **Why**: So that users don't see 'NOT TRANSLATED YET'. * Only filter string params **Why**: When Twilio contacts our `api/voice/otp endpoint`, they send a parameter with a symbol as a value, and symbols don't respond to the `replace` method. * Rollback change to session encryptor key caching **Why**: This appears to be causing CSRF issues in lower environments. We should investigate before we roll this out. * Sanitize improperly encoded form inputs **Why**: To reduce 500 errors. * Verify user authed before assigning personal key **Why**: Because without an authed user there is nothing to assign a personal key 2. Attempting to access this page while not authenticated results in a 500. This page fixes that to have the expected behavior, a redirect to the sign in page. * Speed up agency based uuids db migration script and create rake task **Why**: The db migration script is running too slow on prod data and the scripts in bin don't get copied to prod **How** Create a new rake task called agency_based_uuids and remove the in clause (which handled successive and increasing larger runs of a single agency's uuid priorities) on the sql and add 'on conflict do nothing'. There is no risk of duplicated rows because there is two unique indexes on the table. * Revert "Show attributes shared with sp's after sign up" This reverts commit 4b58322. * Update RRB Benefit Connect redirect URI * Add a NR method tracer to UserAccessKey#build **Why**: So that we can see calls to SCrypt in New Relic traces and track down redundant SCrypt calls. * Make AWS SES region configurable **Why**: So we can configure which region the app uses to send emails * Changed password validation to check in range (#2025) * Changed password validation to check in range **Why**: To correct an off-by-one error. * Increased minimum password length to 9 **Why**: 8 is simply too short to consistently be strong * Changed password length to 9 characters (with tests) **Why** it's too hard to have a good 8 character password. * LG-120 Updated alert messaging (#2026) **Why** To help clarify need for new account for USAJOBS * Deploy RC 53 to staging (#2050) * Add rake task to create readonly database user **Why**: So we can create a readonly database user to be used in the console after #1996 * Fall back to A record if MX record doesn't exist **Why**: Some email servers don't configure an MX record and rely on senders falling back to the A record, as recommended by the RFC. **How**: The email validation gem takes a parameter to do a check of the MX record and fall back to an A record if there is no MX record. * assume some jitter in timing when comparing time differences * LG-132 Updated OTP and Session timeouts (#2041) * Updated OTP and Session timeouts **Why**: In order to provide a bit more time for USAJobs users. * LG-132 Fixed tests **Why**: Changes to config broke a couple of them. * Select AWS SES region from a pool **Why**: We need to steadily increase our sending volume when moving between AWS regions to keep from being block as spam. This commit allows us to configure what percentage of mail gets sent from what region. * Validate phone number pattern before submission **Why**: We want to be able to leverage several phone validation options, so we are (1) moving the validation to the backend, (2) giving feedback to the user when the phone number is considered valid, (3) enabling "Send Code" only when the phone number is considered valid. **How**: We send a POST to an endpoint when the phone input field blurs or the country code changes. The result of the POST indicates if the number is valid. We can add more information later. * Validate Twilio requests to /api/voice/otp **Why**: To make sure that only requests that contain Twilio's signature are allowed. Reference: https://twilio-ruby.readthedocs.io/en/latest/usage/validation.html#rack-middleware * LG-123 Show OTP expiration to user on web page **Why**: So they know up front when it will expire. * LG-1 delete account: spanish and french translations (#2042) **Why**: Translations are not available for French and Spanish **How** Update the locale files * Re-add authenticable salt method **Why**: Devise depends on the authenticable salt method, namely for serializing a user into the session: https://github.com/plataformatec/devise/blob/master/lib/devise/models/authenticatable.rb#L233-L235 Credit to @monfresh for finding this bug. * Add a helpful comment * Define locale argument for VoiceOtpSenderJob **Why**: The controller calls it with that argument. I overlooked this when working on #2280 * Revert VoiceOtpSenderJob#send_otp **Why**: A bug was found in RC 61, but shortly after the release branch was created, #2276 was merged into master with changes incompatible with RC 61. When I fixed the bug in RC 61, I didn't notice VoiceOtpSenderJob had changed. This PR uses the old code for #send_otp to allow tests in RC 61 to pass. * LG-438 Remove csrf protection on the account reset delayed notifications API endpoint **Why**: The endpoint is already protected by an auth token **How**: skip_before_action :verify_authenticity_token * Ignore the old password columns on the user model (#2329) **Why**: The old deployed code does not realize the columns have been dropped, so it breaks when it tries to load them into the model. * Match host on redirect URIs **Why**: If we only test that the redirect starts with a valid string, then we are open to some SPs having redirects with incorrect hosts redirecting users to the wrong server. **How**: We parse the redirect URI and compare significant parts. * Fix 500 error on bad personal key * Match host on redirect URIs **Why**: If we only test that the redirect starts with a valid string, then we are open to some SPs having redirects with incorrect hosts redirecting users to the wrong server. **How**: We parse the redirect URI and compare significant parts. * Hardcode session encryption cost for migration (#2395) **Why**: In #2353 we changed the scrypt cost with changed the scrypt cost which affected the session encryptor causing sessions encrypted by old and new hosts to be incompatible. This commit hardcodes the cost in the deprecated encryptor so that the sessions will be compatible between hosts. * Revert "Merge pull request #2351 from 18F/mb-refactor-redirect-uri-validation" (#2400) This reverts commit 1087dd8, reversing changes made to bd698fa. * LG-525 Fix IDV for users without phones **Why**: Users are unable to complete IDV if they setup their account with an authenticator app. **How**: Allow sms verification if it is in the middle of IDV verification by checking a session variable. Fix the spec / test. * Revert "Merge pull request #2351 from 18F/mb-refactor-redirect-uri-validation" (#2400) This reverts commit 1087dd8, reversing changes made to bd698fa. * Write 2L KMS encrypted sessions (#2373) **Why**: We are moving away from the user access keys in favor of 2L-KMS which involves aes encrypted ciphertexts wrapped by KMS * LG-490 Use 32 byte salts for passwords (#2372) **Why**: Because it doesn't make sense to generate 10 byte salts and digest them when we can generate 32 byte salts directly * Revert "Write 2L KMS encrypted sessions (#2373)" This reverts commit cb67632. * Revert "Revert "Write 2L KMS encrypted sessions (#2373)"" This reverts commit 48c848a. * Catch sending too much to kms (#2411) **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. * Fix Idv::Proofer vendor initialization **Why**: We want to start with the right set of vendors for proofing. **How**: We initialize `@vendor` to `nil` rather than a truthy value. * Add nil phone_configuration to anonymous user **Why**: Sometimes, we have an anonymous user. They don't have any configured phones. **How**: Add a method that returns `nil` * Take into account nil user in SmsLoginOptionPolicy **Why**: To prevent a 500 error when a user visits the `/account_reset/confirm_request` path while signed out * Fix failure screens throwing 500 error with failure_to_proof_url * Return blank for nil phone numbers **Why**: We are getting an error raised if no phone configuration exists. **How**: Check for nil and return a blank string.

jmhooper added status - ready for review status - do not merge labels Feb 16, 2018

jmhooper requested review from monfresh, mzia, nickbristow, brodygov, jgsmith-usds, tbaxter-18f and stevegsa February 16, 2018 17:33

jmhooper force-pushed the jmhooper-readonly-db-users branch from 9f7ffc7 to f8ced69 Compare February 16, 2018 17:34

jgsmith-usds approved these changes Feb 16, 2018

View reviewed changes

jmhooper mentioned this pull request Mar 6, 2018

Add rake task to create readonly database user #2040

Merged

9 tasks

jmhooper added a commit that referenced this pull request Mar 7, 2018

Add rake task to create readonly database user

be4bcd7

**Why**: So we can create a readonly database user to be used in the console after #1996

jmhooper force-pushed the jmhooper-readonly-db-users branch from 68443a6 to da8fad0 Compare March 7, 2018 19:54

jmhooper removed the status - do not merge label Mar 7, 2018

jmhooper merged commit dc7f1f9 into master Mar 7, 2018

jmhooper deleted the jmhooper-readonly-db-users branch February 15, 2019 19:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use readonly production database config in console #1996

Use readonly production database config in console #1996

jmhooper commented Feb 16, 2018 •

edited

Loading

jmhooper commented Feb 16, 2018

jgsmith-usds left a comment

Use readonly production database config in console #1996

Use readonly production database config in console #1996

Conversation

jmhooper commented Feb 16, 2018 • edited Loading

jmhooper commented Feb 16, 2018

jgsmith-usds left a comment

Choose a reason for hiding this comment

jmhooper commented Feb 16, 2018 •

edited

Loading