Skip to content
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.

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

Use UpdateUser class when updating users #1067

Merged
merged 3 commits into from Feb 9, 2017
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 16 additions & 6 deletions app/controllers/concerns/two_factor_authenticatable.rb
Expand Up @@ -38,7 +38,10 @@ def check_already_authenticated
def reset_attempt_count_if_user_no_longer_locked_out
return unless decorated_user.no_longer_blocked_from_entering_2fa_code?

current_user.update(second_factor_attempts_count: 0, second_factor_locked_at: nil)
UpdateUser.new(
user: current_user,
attributes: { second_factor_attempts_count: 0, second_factor_locked_at: nil }
).call
end

def handle_valid_otp
Expand Down Expand Up @@ -78,9 +81,13 @@ def render_show_after_invalid

def update_invalid_user
current_user.second_factor_attempts_count += 1
# set time lock if max attempts reached
current_user.second_factor_locked_at = Time.zone.now if current_user.max_login_attempts?
current_user.save
attributes = {}
attributes[:second_factor_locked_at] = Time.zone.now if current_user.max_login_attempts?

UpdateUser.new(
user: current_user,
attributes: attributes
).call
end

def handle_valid_otp_for_confirmation_context
Expand All @@ -93,7 +100,7 @@ def handle_valid_otp_for_authentication_context
mark_user_session_authenticated
bypass_sign_in current_user

current_user.update(second_factor_attempts_count: 0)
UpdateUser.new(user: current_user, attributes: { second_factor_attempts_count: 0 }).call
end

def assign_phone
Expand Down Expand Up @@ -127,7 +134,10 @@ def update_phone_attributes
if idv_context?
Idv::Session.new(user_session, current_user).params['phone_confirmed_at'] = current_time
else
current_user.update(phone: user_session[:unconfirmed_phone], phone_confirmed_at: current_time)
UpdateUser.new(
user: current_user,
attributes: { phone: user_session[:unconfirmed_phone], phone_confirmed_at: current_time }
).call
end
end

Expand Down
5 changes: 4 additions & 1 deletion app/controllers/sign_up/passwords_controller.rb
Expand Up @@ -23,7 +23,10 @@ def permitted_params

def process_successful_password_creation
@user.confirm
@user.update(reset_requested_at: nil, password: permitted_params[:password])
UpdateUser.new(
user: @user,
attributes: { reset_requested_at: nil, password: permitted_params[:password] }
).call
sign_in_and_redirect_user
end

Expand Down
6 changes: 3 additions & 3 deletions app/controllers/users/reset_passwords_controller.rb
Expand Up @@ -80,7 +80,6 @@ def build_user

def handle_successful_password_reset
update_user

mark_profile_inactive

flash[:notice] = t('devise.passwords.updated_not_active') if is_flashing_format?
Expand All @@ -101,8 +100,9 @@ def handle_unsuccessful_password_reset(result)
end

def update_user
resource.update(confirmed_at: Time.current) unless resource.confirmed?
resource.update(password: user_params[:password])
attributes = { password: user_params[:password] }
attributes[:confirmed_at] = Time.current unless resource.confirmed?
UpdateUser.new(user: resource, attributes: attributes).call
end

def mark_profile_inactive
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/users/totp_setup_controller.rb
Expand Up @@ -27,7 +27,7 @@ def disable
if current_user.totp_enabled?
analytics.track_event(Analytics::TOTP_USER_DISABLED)
create_user_event(:authenticator_disabled)
current_user.update(otp_secret_key: nil)
UpdateUser.new(user: current_user, attributes: { otp_secret_key: nil }).call
flash[:success] = t('notices.totp_disabled')
end
redirect_to profile_path
Expand Down
2 changes: 1 addition & 1 deletion app/forms/recovery_code_form.rb
Expand Up @@ -7,7 +7,7 @@ def initialize(user, code)
def submit
@success = valid_recovery_code?

user.update!(recovery_code: nil) if success
UpdateUser.new(user: user, attributes: { recovery_code: nil }).call if success

result
end
Expand Down
2 changes: 1 addition & 1 deletion app/forms/update_user_email_form.rb
Expand Up @@ -21,7 +21,7 @@ def submit(params)

if valid_form?
@success = true
@user.update(email: email)
UpdateUser.new(user: @user, attributes: { email: email }).call
else
@success = process_errors
end
Expand Down
2 changes: 1 addition & 1 deletion app/forms/update_user_password_form.rb
Expand Up @@ -11,7 +11,7 @@ def initialize(user)
def submit(params)
self.password = params[:password]

@success = valid? && user.update(params)
@success = valid? && UpdateUser.new(user: user, attributes: params).call

result
end
Expand Down
13 changes: 8 additions & 5 deletions app/services/idv/attempter.rb
Expand Up @@ -9,14 +9,17 @@ def initialize(current_user)
end

def increment
current_user.update!(
idv_attempts: current_user.idv_attempts + 1,
idv_attempted_at: Time.zone.now
)
UpdateUser.new(
user: current_user,
attributes: { idv_attempts: current_user.idv_attempts + 1, idv_attempted_at: Time.current }
).call
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the change from Time.zone.now to Time.current? I think we should be consistent and stick with one. Looks like we use the former more than the latter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

honestly: line length. I saw that we are using them both so thought I'd take the short cut. Seems like usage is kind of 50/50?

Copy link
Contributor

Choose a reason for hiding this comment

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

Up to you!

end

def reset
current_user.update!(idv_attempts: 0)
UpdateUser.new(
user: current_user,
attributes: { idv_attempts: 0 }
).call
end

def attempts
Expand Down
2 changes: 1 addition & 1 deletion app/services/key_rotator/attribute_encryption.rb
Expand Up @@ -7,7 +7,7 @@ def initialize(user)
end

def rotate
user.update_columns(encrypted_attributes)
UpdateUser.new(user: user, attributes: encrypted_attributes).call
end

private
Expand Down
2 changes: 1 addition & 1 deletion app/services/key_rotator/hmac_fingerprinter.rb
Expand Up @@ -14,7 +14,7 @@ def rotate(user:, pii_attributes: nil, profile: nil)

def rotate_email_fingerprint(user)
ee = EncryptedAttribute.new_from_decrypted(user.email)
user.update_columns(email_fingerprint: ee.fingerprint)
UpdateUser.new(user: user, attributes: { email_fingerprint: ee.fingerprint }).call
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is it important for this to be update_columns instead of update! ?

Behavior is slightly different but changing this breaks no tests. If we want to revert, I can create a ticket to write tests for this so we know not to change it.

cc @monfresh @pkarman

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't want the updated_at timestamp to change, since no user data is changing. No hard biz requirement around that. I just prefer maintenance tasks like this to be as transparent as possible. ref http://www.davidverhasselt.com/set-attributes-in-activerecord/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is your suggestion that we revert and write some tests later, or keep as is? Happy to do either.

Copy link
Contributor

Choose a reason for hiding this comment

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

update_columns is faster, but it doesn't perform any validations or callbacks. Given that we don't have any in the User model (except for set_default_role), I think it should be fine to use update_columns. I would vote to pick one for consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @monfresh when you say "pick one for consistency", what exactly are you referring to? Totally down with using update_columns in the key rotator classes, but assume we'd want to stick with update! elsewhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

Given that my decision to use update_columns was both for performance reasons (rotating millions of records can take time), and to avoid the updated_at timestamp, I would prefer to see it stay in this instance. ActiveRecord has multiple ways to do things, because a single method does not fit every need. I would rather keep it here. Adding more indirection doesn't help here IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, will revert change here and add tests separately so we don't make this change again! https://github.com/18F/identity-private/issues/1450

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted at 6ac6a7b

Copy link
Contributor

Choose a reason for hiding this comment

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

What I'm saying is that I think we can use update_columns everywhere because we don't have any validations in the User model. update! will never raise an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh huh that's interesting, sorry I merged before I saw this comment.

I feel like using an update method that does not run validations or callbacks makes sense in some situtations, like this one, but perhaps not all?

In general in any rails app I would expect records to be updated with update or update!, but I could be convinced otherwise, I supposed.

end

def rotate_ssn_signature(profile, pii_attributes)
Expand Down
13 changes: 8 additions & 5 deletions spec/controllers/sign_up/email_confirmations_controller_spec.rb
Expand Up @@ -92,7 +92,10 @@

it 'tracks expired token' do
user = create(:user, :unconfirmed)
user.update(confirmation_token: 'foo', confirmation_sent_at: Time.current - 2.days)
UpdateUser.new(
user: user,
attributes: { confirmation_token: 'foo', confirmation_sent_at: Time.current - 2.days }
).call

analytics_hash = {
success: false,
Expand All @@ -114,8 +117,7 @@

describe 'Valid email confirmation tokens' do
it 'tracks a valid email confirmation token event' do
user = create(:user, :unconfirmed)
user.update(confirmation_token: 'foo')
user = create(:user, :unconfirmed, confirmation_token: 'foo')

stub_analytics

Expand All @@ -135,8 +137,9 @@

describe 'User confirms new email' do
it 'tracks the event' do
user = create(:user, :signed_up)
user.update(
user = create(
:user,
:signed_up,
confirmation_token: 'foo',
confirmation_sent_at: Time.current,
unconfirmed_email: 'test@example.com'
Expand Down
14 changes: 4 additions & 10 deletions spec/controllers/sign_up/passwords_controller_spec.rb
Expand Up @@ -3,11 +3,8 @@
describe SignUp::PasswordsController do
describe '#create' do
it 'tracks a valid password event' do
user = create(:user, :unconfirmed)
token, = Devise.token_generator.generate(User, :confirmation_token)
user.update(
confirmation_token: token, confirmation_sent_at: Time.current
)
token = 'new token'
user = create(:user, confirmation_token: token, confirmation_sent_at: Time.current)

stub_analytics

Expand All @@ -29,11 +26,8 @@
end

it 'tracks an invalid password event' do
user = create(:user, :unconfirmed)
token, = Devise.token_generator.generate(User, :confirmation_token)
user.update(
confirmation_token: token, confirmation_sent_at: Time.current
)
token = 'new token'
user = create(:user, confirmation_token: token, confirmation_sent_at: Time.current)

stub_analytics

Expand Down
Expand Up @@ -19,7 +19,11 @@
end

it 'resets the second_factor_attempts_count' do
subject.current_user.update(second_factor_attempts_count: 1)
UpdateUser.new(
user: subject.current_user,
attributes: { second_factor_attempts_count: 1 }
).call

post :create, code: generate_totp_code(@secret)

expect(subject.current_user.reload.second_factor_attempts_count).to eq 0
Expand Down Expand Up @@ -73,13 +77,15 @@

context 'when the user lockout period expires' do
before do
sign_in_before_2fa
@secret = subject.current_user.generate_totp_secret
subject.current_user.otp_secret_key = @secret
subject.current_user.update(
user = create(
:user,
:signed_up,
second_factor_locked_at: Time.zone.now - Devise.direct_otp_valid_for - 1.second,
second_factor_attempts_count: 3
)
sign_in_before_2fa(user)
@secret = subject.current_user.generate_totp_secret
subject.current_user.otp_secret_key = @secret
end

describe 'when user submits an invalid TOTP' do
Expand Down
16 changes: 7 additions & 9 deletions spec/controllers/users/reset_passwords_controller_spec.rb
Expand Up @@ -70,16 +70,15 @@
stub_analytics
allow(@analytics).to receive(:track_event)

raw_reset_token, db_confirmation_token =
Devise.token_generator.generate(User, :reset_password_token)
user = create(
:user,
:signed_up,
reset_password_sent_at: Time.current - Devise.reset_password_within - 1.hour
reset_password_sent_at: Time.current - Devise.reset_password_within - 1.hour,
reset_password_token: db_confirmation_token
)

raw_reset_token, db_confirmation_token =
Devise.token_generator.generate(User, :reset_password_token)
user.update(reset_password_token: db_confirmation_token)

params = { password: 'short', reset_password_token: raw_reset_token }

put :update, reset_password_form: params
Expand Down Expand Up @@ -182,13 +181,12 @@

raw_reset_token, db_confirmation_token =
Devise.token_generator.generate(User, :reset_password_token)

profile = create(:profile, :active, :verified)
user = profile.user
user.update(
user = create(
:user,
reset_password_token: db_confirmation_token,
reset_password_sent_at: Time.current
)
_profile = create(:profile, :active, :verified, user: user)

stub_email_notifier(user)

Expand Down
9 changes: 5 additions & 4 deletions spec/controllers/verify_controller_spec.rb
Expand Up @@ -96,11 +96,12 @@

context 'user does not have an active profile and has exceeded IdV attempts' do
it 'allows direct access' do
profile = create(:profile)
user = profile.user
user.update(idv_attempts: 3, idv_attempted_at: Time.zone.now)
profile = create(
:profile,
user: create(:user, idv_attempts: 3, idv_attempted_at: Time.zone.now)
)

stub_sign_in(user)
stub_sign_in(profile.user)

get :fail

Expand Down
14 changes: 8 additions & 6 deletions spec/features/two_factor_authentication/sign_in_spec.rb
Expand Up @@ -116,9 +116,12 @@ def submit_prefilled_otp_code
expect(page).to have_content(/4:5\d/)

# let lockout period expire
user.update(
second_factor_locked_at: Time.zone.now - (Devise.direct_otp_valid_for + 1.second)
)
UpdateUser.new(
user: user,
attributes: {
second_factor_locked_at: Time.zone.now - (Devise.direct_otp_valid_for + 1.second),
}
).call

sign_in_before_2fa(user)
click_button t('forms.buttons.submit.default')
Expand All @@ -129,8 +132,7 @@ def submit_prefilled_otp_code

context 'user signs in while locked out' do
it 'signs the user out and lets them know they are locked out' do
user = create(:user, :signed_up)
user.update(second_factor_locked_at: Time.zone.now - 1.minute)
user = create(:user, :signed_up, second_factor_locked_at: Time.zone.now - 1.minute)
allow_any_instance_of(User).to receive(:max_login_attempts?).and_return(true)
signin(user.email, user.password)

Expand Down Expand Up @@ -196,7 +198,7 @@ def submit_prefilled_otp_code
# For example, when migrating users from another DB
it 'displays recovery code and redirects to profile' do
user = create(:user, :signed_up)
user.update!(recovery_code: nil)
UpdateUser.new(user: user, attributes: { recovery_code: nil }).call

sign_in_user(user)
click_button t('forms.buttons.submit.default')
Expand Down
5 changes: 3 additions & 2 deletions spec/features/visitors/password_recovery_spec.rb
Expand Up @@ -176,7 +176,7 @@ def reactivate_profile(password, recovery_code)

raw_reset_token, db_confirmation_token =
Devise.token_generator.generate(User, :reset_password_token)
@user.update(reset_password_token: db_confirmation_token)
UpdateUser.new(user: @user, attributes: { reset_password_token: db_confirmation_token }).call

visit edit_user_password_path(reset_password_token: raw_reset_token)
end
Expand Down Expand Up @@ -328,7 +328,8 @@ def reactivate_profile(password, recovery_code)

raw_reset_token, db_confirmation_token =
Devise.token_generator.generate(User, :reset_password_token)
user.update(reset_password_token: db_confirmation_token)

UpdateUser.new(user: user, attributes: { reset_password_token: db_confirmation_token }).call

visit edit_user_password_path(reset_password_token: raw_reset_token)

Expand Down
8 changes: 3 additions & 5 deletions spec/services/email_notifier_spec.rb
Expand Up @@ -6,8 +6,7 @@

context 'when the password has changed' do
it 'sends an email notifiying the user of the password change' do
user = create(:user, :signed_up)
user.update!(password: 'newValidPass!!00')
user = create(:user, :signed_up, password: 'newValidPass!!00')

expect(UserMailer).to receive(:password_changed).with(user).and_return(mailer)
expect(mailer).to receive(:deliver_later)
Expand All @@ -30,10 +29,9 @@

context 'when the user has changed and confirmed their new email' do
it 'notifies the old email address of the email change' do
user = create(:user, :signed_up)
user = build(:user, :signed_up)
old_email = user.email
user.email = 'new@example.com'
user.save!
UpdateUser.new(user: user, attributes: { email: 'new@example.com' }).call
user.confirm

expect(UserMailer).to receive(:email_changed).with(old_email).and_return(mailer)
Expand Down
4 changes: 2 additions & 2 deletions spec/support/features/session_helper.rb
Expand Up @@ -125,9 +125,9 @@ def perform_in_browser(name)
end

def sign_in_with_totp_enabled_user
user = create(:user, :signed_up, password: VALID_PASSWORD)
user = build(:user, :signed_up, password: VALID_PASSWORD)
@secret = user.generate_totp_secret
user.update(otp_secret_key: @secret)
UpdateUser.new(user: user, attributes: { otp_secret_key: @secret }).call
sign_in_user(user)
fill_in 'code', with: generate_totp_code(@secret)
click_submit_default
Expand Down