Merged
Conversation
Latest logo for the Small Business Administration
**Why**: Personal key confirmation modal lacks conversion helper present in server-side code. **How**: Add corresponding client-side code to coerce mistakenly read characters into their Crockford Base32 equivalents.
**Why**: Personal key confirmation modal lacks conversion helper present in server-side code. **How**: Add corresponding client-side code to coerce mistakenly read characters into their Crockford Base32 equivalents.
**Why**: We want to remove the token from the piv/cac service so that it doesn't leak into logs or other services. **How**: When there's an error, we store the messaging in the flash and redirect to the same page to show the error and provide another attempt.
**Why:** The phone verification process for IdV used to mirror USPS verification. We changed that functionality, but never properly cleaned up some of the dead code. This commit does that, and also prepares to drop the unnecessary `phone_confirmed` column from the profile model.
**Why**: We are no longer reading from these columns in favor of the encrypted_recovery_code_digest column. This commit ignores the columns in preparation for dropping them in a following release
…to-pk-confirm LG-508 Add client-side Crockford Base32 encoding helper
**Why**: Broken corners are bad and we should feel bad. **How**: Hide the overflow.
…adius LG-555 Fix border radius on Account boxes
**Why**: We want to roll out piv/cac across the government, but in a measured way since we don't want to advertise it to folk we know that we can't support their piv/cac. **How**: Allow a configurable list of domains that we know we should be able to support. Allow suffix matching if starting with a `.` so we can support an agency that uses multiple subdomains, but also allow full matching for those agencies for which we know we can only support a subset. We assume that a subset of an agency for our purposes here will share an email domain. We don't require a user to have a proper email *and* a relationship with a particular SP. We require at least one of a relationship with a particular SP and a supported email domain.
**Why**: To help us better understand how people use the product and where they are failing. The main change here is to capture the controller where the user decided to cancel verification. This will allow us to see if there's a particular blocker in the flow. This information is used by other controllers, so I refactored a bit to reduce duplication. I also added a visit event for the come back later page, and made sure that the call to track the visit event in the IdV Sessions Controller was the first thing in the method. Otherwise, if an error happens before the analytics call, it wouldn't be easy to tell whether or not the user visited (or was redirected to) the sessions controller. For troubleshooting, it's very helpful to have events for all page visits.
**Why**: Due to a bug that has since been fixed, there still exist users in the database whose `otp_delivery_preference` is set to `voice` even though our Twilio account does not allow us to call their phone number. This means that every time they sign in, we try to make a call, which results in a Twilio error, then the user has to choose a different 2FA option. I had noticed this scenario a while back and fixed it, when the country in question was India, but this time, people in Bermuda are experiencing this. The reason why it was failing for Bermuda is because it uses the same `+1` dialing code as the US, and we were treating all countries that use `+1` as having "area codes", and we were trying to match the area code by calling the `area_code` method on the parsed phone. It's possible that this was working with `phony_rails`, but it doesn't with `Phonelib` (which is accurate). **How**: Given that we have a list of all countries in our YAML file and in the UI dropdown, and that Phonelib knows which country the number comes from, we don't need to treat countries with `+1` differently. This allows us to remove all code related to unsupported area codes. Instead of looking for an area code, we look up the 2-letter code from the parsed phone, and find a match in our YAML file.
Track additional IdV analytics
…page **Why**: So users redirect back to their service provider rather than getting stuck at the account page. **How**: Fix the before filter check_already_authenticated which incorrectly redirects to the account page instead of the service provider. Add a feature spec that tests for this.
…redirect-to-account-page LG-554 Fix already authenticated users redirecting to account page
**Why**: To be able to test SP-initiated logout locally
Update redirect_uri list for OIDC Sinatra dev app
**Why**: Per request from the agency. **How**: Update service_providers.yml
* BL-43: Remove equifax from application. **Why**: Remove Equifax since we are no longer using it for IDV. **How**: Remove all references to Equifax in all files. Remove tests for equifax.
…domains [LG-487] piv/cac available based on email domain
LG-562 Add a new redirect_uri for logout with the CBP ROAM SP
**Why**: I found a bug that I can consistently reproduce in Chrome (but not in other browsers for some reason): If you already have an identity associated with an SP, and are using the "remember me" feature, you won't be able to sign in during this scenario: 1. Visit the SP 2. Click through to the IdP 3. Click "Sign in" but don't sign in yet 4. Wait 15 minutes, or until the page refreshes and says "For your security, we clear what you entered..." 5. Enter your email and password and submit the form Result: Chrome will prevent the redirect back to the SP because the redirect URI is not in the CSP headers. **How**: Store the SP info in the session when the sign in page is visited because the CSP headers only get updated if the SP info is in the session, but if you stay idle for 15 minutes, the session gets deleted from Redis.
**Why**: Our lower environment websites (dev, int, qa, staging, etc.) are public, and sometimes people visit them without realizing they are not on secure.login.gov. When that happens, we want to make sure they know they are not on the production site by displaying a banner at the top of the site that says this is a FAKE site.
LG-544 Prevent calling unsupported countries
LG-460 Display fake banner in lower environments.
LG-559 Allow sign in via remember me after idling
…cac-token [LG-423] Redirect piv/cac errors to cleanup url
**Why**: Phonelib's `country_code` method returns the dialing code prefix, which can be the same across multiple countries, such as the `1` dialing code, which is used by the USA, Canada, and many Carribean countries. By using the `country` method, which returns the 2-letter country abbreviation, it allows to more easily determine the country.
Use 2-letter phone country code for analytics
**Why**: - We value lean controllers with minimal business logic - To fix a bug where users without a phone would see a message that an SMS was sent to their phone - To fix a bug where users who had not yet set up 2FA were redirected to the phone setup page instead of the 2FA setup page - To fix the French translation for the SMS message - To capture analytics about what kind of 2FA options the user has configured, to see if phone users request account deletion more than TOTP users, for example. - To capture analytics for visiting the account deletion page
**Why**: In the IdV flow, we know the number will be a US number so we don't have to validate delivery methods against the phone number. Additionally, we don't need to update the user's delivery preference. For these reasons it makes more sense for the IdV flow to have it's own form.
Refactor and fix account reset requests
Adds in the logo for the Small Business Administration
**Why**: The browser UX might take a while, and we don't want the user to feel that everything is frozen up. **How**: When the user clicks on the button to present their piv/cac cert, we intercept the event and change the content to be a GIF spinner. We let the event continue so the browser follows through with the request.
…-presentation [LG-256] Add spinner when presenting piv/cac cert
**Why**: Per request from the agency. This is in preparation for exact matching on post_logout_redirect_uri **How**: Update service_providers.yml
…r-ttp LG-582 Add a logout redirect uri for the Trusted Traveler Program SP
**Why**: The OTP rate limiter was doing an exact match on `second_factor_locked_out` after rate limitting a user. If the system clock advances to the next second between the user being locked out and the check against the current time, this test fails. This happens frequently on CI. This commit uses rspec's `be_within.of` matcher instead of using an exact match.
* [LG-487] Email based Suggesting scoped by agency **Why**: Because user messaging is done based on SPs, we want to make sure we restrict our suggestion of piv/cac use for folks to those interacting with select SPs *and* email domains. **How**: Switch from an OR to an AND. Moves some code around.
* Copy phone info to new table sans crypto **Why**: Each time we decrypt/encrypt in AWS, we call KMS. Since we don't really need to know the content of the phone number to do the copy, we don't need to risk running into KMS limits during the copy. **How**: We copy the encrypted data rather than going through the crypto round-trip.
**Why**: Autoloader was reloading the `Idv::Proofer` class when there were changes to the app. As a result, the configurations set in the initializer were being wiped out since the initializer was not re-run when the class was autoloaded. Since the configurations in the proofer class that were being set are all around fetching vendors, they could be replaced with code that lazily instantiates the list of vendors. That is what this PR does. This PR also has some code to do some refactoring and simplify some of the logic.
* [LG-500] Read phone configuration from table **Why**: We want to transition to the phone_configurations table so that we can support multiple phones per user. For this PR, we read phone information from the new table without changing how we write data. **How**: We change everywhere that we read phone info from the user model to read it from the table.
* [LG-487] Scope piv/cac push by SP/email **Why**: We want to manage our rollout of PIV/CAC support. We want people able to configure piv/cac during account creation if they are at the right SP with the right email domain. **How**: Pass the user through the service provider check.
**Why**: To provide a path forward for the user to complete their action in the event remote proofing failed **How**: Implement the new design in the failure partials and create a new partial specifically for returning to the failure_to_proof_url supplied by the SP.
…of-screens LG-557 Update LOA3 "failure to proof" screens
**Why**: RRB has completed testing in the integration environment and is ready to go live to production. **How**: Add the new configuration to service_providers.yml and add the new cert.
LG-604 Add RRB LOA3 SP to Production
Change return to sp url for RRB LOA3 in Production
gregory-casamento
approved these changes
Aug 27, 2018
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.
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
Controllers
authenticated, make sure to add
before_action :confirm_two_factor_authenticatedas the first callback.
Database
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.
Encryption
Routes
state or result in destructive behavior).
Session
user_sessionhelperinstead of the
sessionhelper so the data does not persist beyond the user'ssession.
Testing
and invalid inputs.