Skip to content

LG-320 Account History should log when their personal key is changed #2217

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
stevegsa merged 1 commit into from
Jun 6, 2018
Merged

LG-320 Account History should log when their personal key is changed #2217

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions app/controllers/users/personal_keys_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ def update
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)
Copy link
Copy Markdown
Contributor

@jmhooper jmhooper Jun 4, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@stevegsa stevegsa Jun 4, 2018

Choose a reason for hiding this comment

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

We could do that but the hacker one finding only identified when the personal key changed and that's how this PR was scoped.

Copy link
Copy Markdown
Contributor

@davemcorwin davemcorwin Jun 4, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@jmhooper jmhooper Jun 4, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@davemcorwin davemcorwin Jun 4, 2018

Choose a reason for hiding this comment

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

aaahhh, then might as well handle both cases here so we don't have to revisit...

Copy link
Copy Markdown
Contributor Author

@stevegsa stevegsa Jun 5, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@jmhooper jmhooper Jun 6, 2018

Choose a reason for hiding this comment

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

@andrewhughey: Any thoughts on ☝️

Copy link
Copy Markdown
Contributor

@andrewhughey andrewhughey Jun 6, 2018

Choose a reason for hiding this comment

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

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.

flash[:success] = t('notices.send_code.personal_key') if params[:resend].present?
redirect_to manage_personal_key_url
end
Expand Down
1 change: 1 addition & 0 deletions app/models/event.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ class Event < ApplicationRecord
usps_mail_sent: 9,
piv_cac_enabled: 10,
piv_cac_disabled: 11,
new_personal_key: 12,
}

validates :event_type, presence: true
Expand Down
1 change: 1 addition & 0 deletions config/locales/event_types/en.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ en:
authenticator_enabled: Authenticator app enabled
eastern_timestamp: "%{timestamp} (Eastern)"
email_changed: Email address changed
new_personal_key: Personal key changed
password_changed: Password changed
phone_changed: Phone number changed
phone_confirmed: Phone confirmed
Expand Down
1 change: 1 addition & 0 deletions config/locales/event_types/es.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ es:
authenticator_enabled: App de autenticación permitido
eastern_timestamp: "%{timestamp} (hora del Este)"
email_changed: Email cambiado
new_personal_key: Clave personal cambiado
password_changed: Contraseña cambiada
phone_changed: Número de teléfono cambiado
phone_confirmed: Teléfono confirmado
Expand Down
1 change: 1 addition & 0 deletions config/locales/event_types/fr.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ fr:
authenticator_enabled: Application d'authentification activée
eastern_timestamp: "%{timestamp} (Eastern)"
email_changed: Adresse courriel modifiée
new_personal_key: Clé personnelle modifié
password_changed: Mot de passe modifié
phone_changed: Numéro de téléphone modifié
phone_confirmed: Numéro de téléphone confirmé
Expand Down
2 changes: 2 additions & 0 deletions spec/controllers/users/personal_keys_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,8 @@

expect(generator).to receive(:create)
expect(@analytics).to receive(:track_event).with(Analytics::PROFILE_PERSONAL_KEY_CREATE)
expect(Event).to receive(:create).
with(user_id: subject.current_user.id, event_type: :new_personal_key)

post :create

Expand Down
5 changes: 4 additions & 1 deletion spec/features/account_history_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@
let(:identity_with_link_timestamp) { identity_with_link.decorate.happened_at_in_words }
let(:usps_mail_sent_again_timestamp) { usps_mail_sent_again_event.decorate.happened_at_in_words }
let(:identity_without_link_timestamp) { identity_without_link.decorate.happened_at_in_words }
let(:new_personal_key_event) { create(:event, event_type: :new_personal_key,
user: user, created_at: Time.zone.now - 40.days) }

before do
sign_in_and_2fa_user(user)
Expand All @@ -40,7 +42,7 @@
end

scenario 'viewing account history' do
[account_created_event, usps_mail_sent_event, usps_mail_sent_again_event].each do |event|
[account_created_event, usps_mail_sent_event, usps_mail_sent_again_event, new_personal_key_event].each do |event|
decorated_event = event.decorate
expect(page).to have_content(decorated_event.event_type)
expect(page).to have_content(decorated_event.happened_at_in_words)
Expand Down Expand Up @@ -70,5 +72,6 @@ def build_account_history
usps_mail_sent_again_event
identity_with_link
identity_without_link
new_personal_key_event
end
end