Skip to content

LG-767 Alert a user on personal key sign in#2630

Merged
jgsmith-usds merged 7 commits intomasterfrom
jmhooper-personal-key-event-alerts
Nov 5, 2018
Merged

LG-767 Alert a user on personal key sign in#2630
jgsmith-usds merged 7 commits intomasterfrom
jmhooper-personal-key-event-alerts

Conversation

@jmhooper
Copy link
Contributor

Why: So that users will know when their personal key is being used
to sign in and be aware of any suspicious activity around their personal
key.

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.

**Why**: So that users will know when their personal key is being used
to sign in and be aware of any suspicious activity around their personal
key.
@jmhooper
Copy link
Contributor Author

Email:
image

SMS:
image

@jprisby1 @konklone: Now we gotta figure out what we actually want to say here.

@konklone
Copy link
Contributor

Where do the links at the bottom go to? (Help Center and contact us.)

I would make this a little friendlier, so that in the common case it's not alarming, while making it clear in the uncommon case that it needs to be given attention.

It should probably also indicate to the user that this changed their personal key and the user should have saved a new one -- so that if the user does not have a new one, they know to perk up.

For example:

Your personal key was used to sign in to your login.gov account.

This email is to notify you that your personal key was just used to log in to your account. That personal key is no longer valid, and a new one has been created for your account.

If this was you, please make sure you have saved your new personal key.

If this was not you, then [instructions related to both logging in and contacting us].

One thing we'll talk about later this afternoon - the viability of including a SP-specific contact link/email at the bottom to get both us and our SP pinged.

Another thing I'll put into Slack or JIRA - a link to partially lock the account and do some other stuff to make it easier to respond during an active account takeover.

@jmhooper
Copy link
Contributor Author

Here's where the links go:

They can be changed if we know what we want them changed to; I lifted them from the password changed email.

SP specific links in email will be a little bit trickier and will require some additional considerations. I'd suggest a different ticket and PR for that change. Same for a link to partially lock the account.

@clantosswett
Copy link

@jgsmith-usds Here is the updated text for the email and SMS notification. Take a look and let me know if you have you any questions. @konklone it might be good to get your input on this as well.

@jgsmith-usds jgsmith-usds merged commit 4667492 into master Nov 5, 2018
jgsmith-usds pushed a commit that referenced this pull request Nov 5, 2018
**Why**: So that users will know when their personal key is being used
to sign in and be aware of any suspicious activity around their personal
key.
@jmhooper jmhooper deleted the jmhooper-personal-key-event-alerts branch February 15, 2019 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants