Skip to content

[LG-587] Add email address table#2503

Merged
jgsmith-usds merged 3 commits intomasterfrom
jgs/lg-587-email-table
Sep 11, 2018
Merged

[LG-587] Add email address table#2503
jgsmith-usds merged 3 commits intomasterfrom
jgs/lg-587-email-table

Conversation

@jgsmith-usds
Copy link
Contributor

@jgsmith-usds jgsmith-usds commented Sep 10, 2018

Why:
We want to support multiple email addresses per account.

How:
We start by creating a table that lets us store more than
one email address for a user. We tie in to the user model
because we need to support Devise for creating accounts.

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

  • When adding a new controller that requires the user to be fully
    authenticated, make sure to add before_action :confirm_two_factor_authenticated
    as 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

  • The changes are compatible with data that was encrypted with the old code.

Routes

  • GET requests are not vulnerable to CSRF attacks (i.e. they don't change
    state or result in destructive behavior).

Session

  • 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.

Testing

  • 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.

@jgsmith-usds
[LG-587] Add email address table
e980fad 
**Why**:
We want to support multiple email addresses per account.

**How**:
We start by creating a table that lets us store more than
one email address for a user. We tie in to the user model
because we need to support Devise for creating accounts.
stevegsa
stevegsa reviewed Sep 10, 2018
View reviewed changes
app/models/concerns/email_address_callback.rb Outdated
elsif encrypted_email.present?
create_full_email_address_record
elsif unconfirmed_email.present?
create_pending_email_address_record
Copy link
Contributor

@stevegsa stevegsa Sep 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs coverage here

Copy link
Contributor Author

@jgsmith-usds jgsmith-usds Sep 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may end up removing it as dead code.

stevegsa
stevegsa approved these changes Sep 11, 2018
View reviewed changes
Copy link
Contributor

@stevegsa stevegsa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jgsmith-usds jgsmith-usds merged commit 8c3024e into master Sep 11, 2018
@jgsmith-usds jgsmith-usds deleted the jgs/lg-587-email-table branch September 11, 2018 21:18
monfresh
monfresh reviewed Sep 13, 2018
View reviewed changes
app/models/concerns/email_address_callback.rb
email_fingerprint: email_fingerprint
)
end
end
Copy link
Contributor

@monfresh monfresh Sep 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reasoning behind this callback?

gregory-casamento added a commit that referenced this pull request Sep 24, 2018
@gregory-casamento
Deploy RC 67 to int (#2549)
661e280 
* LG-599 LG-603 Allow user to config webauthn at account creation. Allow user to use webauthn for 2FA.

Why: So we can support strong authentication via hardware security keys as a new 2FA option

How: Generate a challenge on the server side and present the browser with a list of credentials that are registered to the user. Use the Credential Manager API (navigator.credentials.get) to ask the browser to sign the challenge if the authenticator contains one of the given credentials and the user gives consent.  The web page then automatically submits the form with the signed assertion for the server to verify. Use the webauthn gem to verify the assertion response. Upon success the user is authenticated.

* Add Guardfile for faster local testing

**Why**: We have guard-rspec in our Gemfile, but never set up the
Guardfile.

The byebug update was necessary for zeus to work on my computer.

* LG-641 Handle nil request_id properly

**Why**: In SessionsController, we were using `.empty?` instead of
`.blank?`. I forgot that `fetch` with a default value only works if
the hash key itself is not present, not if it's present and has a nil
value.

* LG-529 Remove IdV background job (#2452)

**Why**: The background job does not actually run in the background
since we are using the inline adapter, but it does add a lot of
complexity to our code. Since we are not using the background job, this
commit moves the proofing logic into the foreground in the idv step
objects.

* Lint and more coverage

* More coverage

* LG-609 Change language on phone / address confirmation page

**Why**: Users are confused / frustrated why we’re asking for the phone number again

**How**: Change language on the page to make it feel like users are verifying their identity rather than repeating an action.

* LG-654 Redact entire SSN from /account page

**Why**: These are the most sensitive digits. A variety of identity proofing operations often require only the last 4 digits, including for things like pulling a credit report.

**How**: Replace the last four digits with asterisks.

* Removed unnecessary code

* Translations

* [LG-587] Add email address table (#2503)

**Why**:
We want to support multiple email addresses per account.

**How**:
We start by creating a table that lets us store more than
one email address for a user. We tie in to the user model
because we need to support Devise for creating accounts.

* Update webauthn setup to match master branch

**Why**: PR #2501 was not rebased against master, and caused specs to
fail on master and on other branches based on master.

**How**:
- Update webauthn setup controller to use the new personal key
policy object and fix the logic so that LOA3 users don't see the
personal key before the IdV flow.
- Update French translations

* LG-625 Cancel flow modification (#2470)

**Why**: The current cancel flow takes the user back to the user's account. It will be more useful for them to go back to the SP.

**How**: Replace the link to the accounts page with the failure_to_proof_url which will use the return_to_sp link if a failure to proof url is not provided.  Thus, it will satisfy requirements for both LOA1 and LOA3.

* Update redirect URI for sp-oidc-sinatra app

**Why**: We now require exact matches, and the config didn't have the
exact redirect URI

* LG-515 Move 2FA localizations out of Devise file

**Why**: The Devise locale folder is for Devise-specific strings. None
of the strings we had under the `two_factor_authentication` section
were related to Devise. We have a separate folder for 2FA localizations:
`config/locales/two_factor_authentication`

**How**:
- Cut and paste anything under `devise.two_factor_authentication`
into the `two_factor_authentication` folder
- Run `make normalize_yaml`
- Do a search and replace for `devise.two_factor_authentication` with
`two_factor_authentication`

* [LG-588] Rake task populating email addresses

**Why**:
We want to support multiple email addresses per account. This
requires that we store email addresses in their own table.

**How**:
A rake task to copy existing email information to the new table.

* 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.

* Fix PIV/CAC and Webauthn errors when disabled (#2519)

**Why**: When the feature flags are disabled the routes are not valid so references to the urls throw an error unless guarded with a feature flag.

**How**: Add feature flag checks for all PIV/CAC and webauthn urls and test coverage

* LG-442 Remove unused sidekiq and worker code

* LG-567 Make password strength spacing consistent (#2515)

* adding blank space to pw feedback
to ensure the div that shows additional information is always present and the continue button stays in place while typing

* removing unnecessary block class

* add space to empty div template so it appears

* LG-627 Don't automatically select SMS during IdV (#2511)

**Why**: When a user uses a different number for address verification
than the one they used for 2FA they are asked to choose if they'd like
to get the code sent to them via text message or voice message. Text
message is auto-selected but it should default to no selection so users
don't think text message is the only option or that it's automatically
compatible with their phone.

* Make the margin classes apply to desktop-only (#2510)

**Why**: The last screen of the LOA3 flow before you are taken back to the SP was looking pretty cramped on mobile. This changes the margin classes that are used to be the mobile-friendly variant so the information spans the full width.

* LG-558 Make jurisdiction step non-re-entrant (#2518)

**Why**: The jurisdiction step is at the beginning flow to prevent users
who are in an unsupported jurisdiction from having to enter all of their
info. It was re-entrant which meant that anytime you returned to the
beginning of the flow, you had to complete it, even if you had already
verified your info. This doesn't make sense so this commit configures
the step such that you cannot return to the jurisdiction step after
completing it.

* Add trailing period to a french idv translation (#2527)

**Why**: So it is consistent with the other translations. This is left
over from #2511.

* LG-665 Remove unused Report Fraud code

**Why**: Originally, we were thinking of providing two options for
canceling an account deletion request:
- I didn’t generate the request (and want to report fraud)
- I generated the request but want to cancel

We ended up implementing the first option but never used it. We never
provided a link to the `account_reset/report_fraud` endpoint in an
email or on any of our sites.

* LG-669 Retry once when Twilio request times out

**Why**: We have noticed a few instances of Twilio requests timing out,
about 10 per day on average, and Twilio support recommended retrying
the requests to see if they still time out. I also added some logging
temporarily so we can give Twilio support more info if the retries
don't work.

To measure the impact of this change, we will check the frequency of the
analytics event with name "Twilio Phone Validation Failed" and following
event_properties:
```ruby
{
   error: "[HTTP 4815162342] 4815162342 : timeout",
   code: 4815162342
}
```

* LG-650 Report IdV vendor timeouts to the user (#2526)

**Why**: Now that the IdV requests are happening in the foreground, we
want to stop long running requests and report a timeout to the user
rather than letting the long running requests lead to the user's request
timing out.

* Update capybara-screenshot to point to rubygems

**Why**: To have the most stable version. I was working on another PR
and I started having bundler issues due to capybara-screenshot.

Note that by updating capybara-screenshot, capybara was also updated
to 3.7.2, which is good because we were quite behind at 2.17.0.
Version 3 changes the way text is matched. It no longer normalizes
whitespace and returns text as close as possible to "as rendered", which
allows us to find bugs such as extra spaces in our localizations.

Version 3 also uses puma by default, so I added the gem.

More upgrade details here:
https://github.com/teamcapybara/capybara/blob/master/UPGRADING.md

* LG-684 Rescue Faraday::ConnectionFailed errors

**Why**: Rarely (16 times since the beginning of the year), the app is
not able to connect to the Twilio servers and the Faraday gem raises a
`Faraday::ConnectionFailed` error. This results in the user seeing the
500 error page. For a better experience, we rescue the error, try to
connect to Twilio again, and if it still fails, we display a helpful
message to the user and ask them to try again.

* LG-624 Add HOMES.mil logo and help text in INT (#2533)

**Why**: For HOMES.mil integration testing

**How**: Update the help text hooks for the new service provider friendly names. Update the locale files

* LG-651 Allow newrelic_rpm to be updated

**Why**: The `newrelic_rpm` gem added a call to `SecureRandom.hex(8)`
on 6/13/18:
newrelic/newrelic-ruby-agent@60ef61c

Some of our tests stub calls to `SecureRandom`, and because we have
New Relic tracers in an initializer, the newly-added call to
`SecureRandom` in the gem caused the tests to fail because they are
not expecting that additional call.

RSpec tells us to stub the call with a default value first in this case.

* [LG-576] List phone configurations (#2492)

**Why**:
We want to show all of the phones that a user is using.

**How**:
In the account management page and the various mfa login and
management selection views, we allow multiple phones. We tweak
a few other places to help make things a bit more consistent.

* LG-677 Add confirm screen on account reset cancel (#2525)

**Why**: Some phones will visit the SMS link to retrieve a thumbnail and this triggers a cancel.

**How**: Add a confirm screen and have it call the old logic via a post request.  Prevent token leakage by placing a valid token in session.

* Get link right for adding a phone

**Why**:
The link accidently included a `path:` component, making a
bad link.

**How**:
Remove the `path:` part of the call and add tests to make sure
we don't introduce it back in.

* LG-637 Set max length of state id number to 25 (#2530)

**Why**: Because according to AAMVA this is the longest a state id
number can be, and they perform this validation on the requests that we
send to them.

* LG-553 LOA3: Cancel verification doesn't remove pending status (#2537)

**Why**: When a user chooses to verify by mail and then returns to say they can't receive mail at this address we should remove their verification pending status

**How**: Ensure that if they are on the phone verification page they have effectively cancelled their pending status.

* Add TTP exact matching logout redirect uri (#2539)

* Add vermont to list of supported states (#2542)

**Why**: Because AAMVA says it is supported now

* LG-666 Refactor AccountResetService

**Why**: To make it more consistent with the other classes under the
AccountReset namespace.

* LG-507: Remove PIV/CAC feature flag

**Why**: The PIV/CAC feature will not be disabled going forward, so we
are removing code that checks whether or not it is enabled.

**How**: I am searching through the code and removing all references to
the FeatureManagement.piv_cac_enabled? class method where appropriate.
I am also removing any tests or changing tests as needed.

* Pad list items in account management screen

**Why**:
We want to make it easier to distinguish between headers and
list items.

**How**:
For now, add a new class that pads left. Later, we'll make the
list of configurations a proper <ul/>.

* LG-695 Enable Webauthn when there is no SP (#2544)

**Why**: To allow us to test in production without enabling the feature for all users.

**How**: Update the policy object to include the current SP and enable webauthn when the SP is nil.

* Add support for proofing SC, MO, and RI (#2546)

**Why**: Because AAMVA has started supporting these states

* LG-543 Retry failed Twilio Verify requests

**Why**: Sometimes, requests to the Twilio Verify service fail to
connect or time out. Twilio suggested that we retry the request to see
if it works the second time. We had implemented retries for the Twilio
REST API earlier. Now, we are adding it to the Verify service as well.

**How**:
- Use Faraday to catch and rescue timeouts and connection failures, and
raise our own custom `VerifyError`. Typhoeus does not raise exceptions,
and therefore does not support retries out of the box. Since the
`twilio-ruby` gem uses Faraday, and we rescue Faraday errors in
`TwilioService`, I thought we'd use Faraday here too for consistency.
- Use dependency injection to specify the connection class, which makes
testing easier.
- Include "Twilio Verify" in the custom error message to make it easier
to differentiate from timeout and failed connection errors raised by
the Twilio REST service.

* LG-518 Identity Verification via Document Authentication and Selfie Matching (#2451)

**Why**: To enhance the security of the identity verification services we offer.

**How**: Interface to external vendors to verify document authenticity and selfie matching.  Perform resolution on the document PII along with a user supplied ssn and phone number.  Provide options to replace the existing LOA3 flow or allow both to coexist so doc auth can be tested by manually entering the path /verify/doc_auth in the browser.  Create a new framework that automatically implements our coding guidelines, minimizes the code we write, and seamlessly manages session state via a state machine with pluggable flows and steps.

* LG-615 Update personal key page design and language (#2529)

**Why**: To emphasize that we will never ask the user to 'verify' the key, and it will only be used in the account recovery flow.

**How**: Update the HTML, CSS, and language.  Add new images.

* Enable webauthn in prod (#2547)
@snyk-bot snyk-bot mentioned this pull request May 8, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@monfresh monfresh monfresh left review comments

+1 more reviewer

@stevegsa stevegsa stevegsa approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

status - ready for review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jgsmith-usds @monfresh @stevegsa