Skip to content

LG-15183: Associate user_id for reCAPTCHA result analytics of failed sign-in #11580

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
aduth merged 4 commits into from
Dec 3, 2024
Merged

LG-15183: Associate user_id for reCAPTCHA result analytics of failed sign-in #11580

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0aff3b9
LG-15183: Associate user_id for reCAPTCHA result analytics of failed …
aduth Dec 3, 2024
c1117f2
Add FakeAnalytics#reset! to support resetting analytics stub
aduth Dec 3, 2024
2d26004
Assert SessionsController user through stub_analytics
aduth Dec 3, 2024
5df0cef
Move SessionsController#analytics_user to public
aduth Dec 3, 2024
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
10 changes: 9 additions & 1 deletion app/controllers/users/sessions_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ class SessionsController < Devise::SessionsController
before_action :check_user_needs_redirect, only: [:new]
before_action :apply_secure_headers_override, only: [:new, :create]
before_action :clear_session_bad_password_count_if_window_expired, only: [:create]
before_action :set_analytics_user_from_params, only: :create
before_action :allow_csp_recaptcha_src, if: :recaptcha_enabled?

after_action :add_recaptcha_resource_hints, if: :recaptcha_enabled?
Expand Down Expand Up @@ -58,6 +59,10 @@ def destroy
end
end

def analytics_user
@analytics_user || AnonymousUser.new
end

private

def clear_session_bad_password_count_if_window_expired
Expand Down Expand Up @@ -168,6 +173,10 @@ def auth_params
params.require(:user).permit(:email, :password)
end

def set_analytics_user_from_params
@analytics_user = user_from_params
end

def process_locked_out_user
presenter = TwoFactorAuthCode::MaxAttemptsReachedPresenter.new(
'generic_login_attempts',
Expand Down Expand Up @@ -210,7 +219,6 @@ def track_authentication_attempt
analytics.email_and_password_auth(
**recaptcha_response,
success: success,
user_id: user.uuid,
user_locked_out: user_locked_out?(user),
rate_limited: rate_limited?,
captcha_validation_performed: captcha_validation_performed?,
Expand Down
3 changes: 0 additions & 3 deletions app/services/analytics_events.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -478,7 +478,6 @@ def edit_password_visit(required_password_change: false, **extra)

# @param [Boolean] success Whether form validation was successful
# @param [Hash] error_details Details for errors that occurred in unsuccessful submission
# @param [String] user_id UUID for user associated with attempted email address
# @param [Boolean] user_locked_out if the user is currently locked out of their second factor
# @param [Boolean] rate_limited Whether the user has exceeded user IP rate limiting
# @param [Boolean] valid_captcha_result Whether user passed the reCAPTCHA check or was exempt
Expand All @@ -491,7 +490,6 @@ def edit_password_visit(required_password_change: false, **extra)
# Tracks authentication attempts at the email/password screen
def email_and_password_auth(
success:,
user_id:,
user_locked_out:,
rate_limited:,
valid_captcha_result:,
Expand All @@ -507,7 +505,6 @@ def email_and_password_auth(
'Email and Password Authentication',
success:,
error_details:,
user_id:,
user_locked_out:,
rate_limited:,
valid_captcha_result:,
Expand Down
36 changes: 12 additions & 24 deletions spec/controllers/users/sessions_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,14 +87,13 @@
end

it 'tracks the successful authentication for existing user' do
stub_analytics
stub_analytics(user:)

response

expect(@analytics).to have_logged_event(
'Email and Password Authentication',
success: true,
user_id: user.uuid,
user_locked_out: false,
rate_limited: false,
valid_captcha_result: true,
Expand Down Expand Up @@ -161,14 +160,13 @@
end

it 'tracks as not being from a new device' do
stub_analytics
stub_analytics(user:)

response

expect(@analytics).to have_logged_event(
'Email and Password Authentication',
success: true,
user_id: user.uuid,
user_locked_out: false,
rate_limited: false,
valid_captcha_result: true,
Expand Down Expand Up @@ -222,8 +220,8 @@
attempt_window_max: 12.hours.in_minutes,
},
)
stub_analytics
user = create(:user, :fully_registered)
stub_analytics(user:)

travel_to (3.hours + 1.minute).ago do
2.times do
Expand Down Expand Up @@ -252,7 +250,6 @@
expect(@analytics).to have_logged_event(
'Email and Password Authentication',
success: false,
user_id: user.uuid,
user_locked_out: false,
rate_limited: true,
valid_captcha_result: true,
Expand All @@ -267,15 +264,14 @@
it 'tracks the unsuccessful authentication for existing user' do
user = create(:user, :fully_registered)

stub_analytics
stub_analytics(user:)
expect(SCrypt::Engine).to receive(:hash_secret).once.and_call_original

post :create, params: { user: { email: user.email.upcase, password: 'invalid_password' } }

expect(@analytics).to have_logged_event(
'Email and Password Authentication',
success: false,
user_id: user.uuid,
user_locked_out: false,
rate_limited: false,
valid_captcha_result: true,
Expand All @@ -288,15 +284,14 @@
end

it 'tracks the authentication attempt for nonexistent user' do
stub_analytics
stub_analytics(user: kind_of(AnonymousUser))
expect(SCrypt::Engine).to receive(:hash_secret).once.and_call_original

post :create, params: { user: { email: 'foo@example.com', password: 'password' } }

expect(@analytics).to have_logged_event(
'Email and Password Authentication',
success: false,
user_id: 'anonymous-uuid',
user_locked_out: false,
rate_limited: false,
valid_captcha_result: true,
Expand All @@ -314,14 +309,13 @@
second_factor_locked_at: Time.zone.now,
)

stub_analytics
stub_analytics(user:)

post :create, params: { user: { email: user.email.upcase, password: user.password } }

expect(@analytics).to have_logged_event(
'Email and Password Authentication',
success: false,
user_id: user.uuid,
user_locked_out: true,
rate_limited: false,
valid_captcha_result: true,
Expand Down Expand Up @@ -365,15 +359,14 @@
it 'tracks unsuccessful authentication for failed reCAPTCHA' do
user = create(:user, :fully_registered)

stub_analytics
stub_analytics(user:)

post :create, params: { user: { email: user.email, password: user.password, score: 0.1 } }

expect(@analytics).to have_logged_event(
'Email and Password Authentication',
success: false,
error_details: { recaptcha_token: { blank: true } },
user_id: user.uuid,
user_locked_out: false,
rate_limited: false,
valid_captcha_result: false,
Expand All @@ -400,14 +393,13 @@
:fully_registered,
)

stub_analytics
stub_analytics(user:)

post :create, params: { user: { email: user.email.upcase, password: 'invalid' } }
post :create, params: { user: { email: user.email.upcase, password: 'invalid' } }
expect(@analytics).to have_logged_event(
'Email and Password Authentication',
success: false,
user_id: user.uuid,
user_locked_out: false,
rate_limited: false,
valid_captcha_result: true,
Expand All @@ -420,14 +412,13 @@

it 'tracks the presence of SP request_url in session' do
subject.session[:sp] = { request_url: mock_valid_site }
stub_analytics
stub_analytics(user: kind_of(AnonymousUser))

post :create, params: { user: { email: 'foo@example.com', password: 'password' } }

expect(@analytics).to have_logged_event(
'Email and Password Authentication',
success: false,
user_id: 'anonymous-uuid',
user_locked_out: false,
rate_limited: false,
valid_captcha_result: true,
Expand Down Expand Up @@ -592,14 +583,13 @@
}.to_json,
)

stub_analytics
stub_analytics(user:)

post :create, params: { user: { email: user.email, password: user.password } }

expect(@analytics).to have_logged_event(
'Email and Password Authentication',
success: true,
user_id: user.uuid,
user_locked_out: false,
rate_limited: false,
valid_captcha_result: true,
Expand Down Expand Up @@ -718,14 +708,13 @@
expires: 2.days.from_now,
}

stub_analytics
stub_analytics(user:)

post :create, params: { user: { email: user.email, password: user.password } }

expect(@analytics).to have_logged_event(
'Email and Password Authentication',
success: true,
user_id: user.uuid,
user_locked_out: false,
rate_limited: false,
valid_captcha_result: true,
Expand All @@ -746,14 +735,13 @@
value: RememberDeviceCookie.new(user_id: user.id, created_at: 2.days.ago).to_json,
}

stub_analytics
stub_analytics(user:)

post :create, params: { user: { email: user.email, password: user.password } }

expect(@analytics).to have_logged_event(
'Email and Password Authentication',
success: true,
user_id: user.uuid,
user_locked_out: false,
rate_limited: false,
valid_captcha_result: true,
Expand Down
2 changes: 1 addition & 1 deletion spec/support/analytics_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ def stub_analytics(user: nil)
end

stub.to receive(:analytics).and_wrap_original do |original|
expect(original.call.user).to eq(user) if user
expect(original.call.user).to match(user) if user
analytics
end

Expand Down
4 changes: 4 additions & 0 deletions spec/support/fake_analytics.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,10 @@ def track_event(event, attributes = {})
def browser_attributes
{}
end

def reset!
@events = Hash.new
end
end

RSpec.configure do |c|
Expand Down
26 changes: 22 additions & 4 deletions spec/support/shared_examples/sign_in.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,16 +324,34 @@ def user_with_broken_personal_key(scenario)
fake_analytics = FakeAnalytics.new
allow_any_instance_of(ApplicationController).to receive(:analytics).
and_wrap_original do |original|
original_analytics = original.call
if original_analytics.request.params[:controller] == 'users/sessions' &&
original_analytics.request.params[:action] == 'create'
expect(original_analytics.user).to eq(user)
if original.receiver.instance_of?(Users::SessionsController) &&
original.receiver.action_name == 'create'
expect(original.call.user).to eq(user)
Comment on lines -327 to +329

@aduth aduth Dec 3, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

These changes are necessary because the unsuccessful submission will result in a redirect back to the sign-in page, which counterintuitively keeps original_analytics.request.params[:action] == 'create' even in the processing of the #new action, since the action is processed as a result of the submission's redirect. Since user_id is expected to be nil on the "Sign in page visited" event, the previous implementation would raise an exception. These changes are to limit assertions to only analytics logged during #create.

asserted_expected_user = true
end

fake_analytics
end

fill_in_credentials_and_submit(user.email, 'wrongpassword')
expect(asserted_expected_user).to eq(true)
expect(fake_analytics).to have_logged_event(
'reCAPTCHA verify result received',
recaptcha_result: {
assessment_id: kind_of(String),
success: true,
score: 1.0,
errors: [],
reasons: [],
},
evaluated_as_valid: true,
score_threshold: 0.2,
recaptcha_action: 'sign_in',
form_class: 'RecaptchaMockForm',
)
asserted_expected_user = false
fake_analytics.reset!

fill_in :user_recaptcha_mock_score, with: '0.1'
fill_in_credentials_and_submit(user.email, user.password)
expect(asserted_expected_user).to eq(true)
Expand Down