Conversation
**Why**: A user should be aware of sensitive account changes **How**: Create a new new_personal_key event when a user changes their personal key.
| def create | ||
| user_session[:personal_key] = create_new_code | ||
| analytics.track_event(Analytics::PROFILE_PERSONAL_KEY_CREATE) | ||
| Event.create(user_id: current_user.id, event_type: :new_personal_key) |
There was a problem hiding this comment.
Would it make more sense to call this in the personal key generator so the event get created every time a new personal key is generate? This is missing the event that occurs when the personal key is created on sign up and during identity verification.
That'd look more like phone and email, where we capture those events on sign up.
There was a problem hiding this comment.
We could do that but the hacker one finding only identified when the personal key changed and that's how this PR was scoped.
There was a problem hiding this comment.
FWIW looking at the other events, we don't have 'create' events for the individual items, only 'change' events, perhaps implying that they are covered by the 'create' event for the account. This PR is consistent with that pattern.
There was a problem hiding this comment.
Well, we trigger those events on sign up as well though. IIRC there are events for confirming email and phone that show up after you create your account.
There was a problem hiding this comment.
aaahhh, then might as well handle both cases here so we don't have to revisit...
There was a problem hiding this comment.
To me these extra conditions seem redundant since they are mandatory steps vs someone requesting a new key on your account. Moreover they aren't giving the user any useful information for security purposes and perhaps would create more confusion.
There was a problem hiding this comment.
I'm comfortable with the scoping as-is. It addresses the H1 report and the use case described.
We don't include in the event history that a password was created or email confirmed. The only events that show up at creation time are: Account created and phone confirmed
We're taking a good look at /account and account management in general, so we can solve for the larger problems there. Let's keep this to addressing "changing" the key.
amathews-fs
deleted the
stevegsa-account-history-should-log-new-personal-key
branch
4 participants
Why: A user should be aware of sensitive account changes
How: Create a new new_personal_key event when a user changes their personal key.
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_toin a controller, use_url, not_path.When adding user data to the session, use the
user_sessionhelperinstead of the
sessionhelper so the data does not persist beyond the user'ssession.
When adding a new controller that requires the user to be fully
authenticated, make sure to add
before_action :confirm_two_factor_authenticated.