LG-15183: Associate user_id for reCAPTCHA result analytics of failed sign-in#11580
Merged
LG-15183: Associate user_id for reCAPTCHA result analytics of failed sign-in#11580
Conversation
…sign-in changelog: Internal, Anti-Fraud, Associate user_id for reCAPTCHA result analytics of failed sign-in
aduth
commented
Dec 3, 2024
Comment on lines
-327
to
+329
| 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) |
Contributor
Author
There was a problem hiding this comment.
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.
zachmargolis
reviewed
Dec 3, 2024
Match ApplicationController#analytics_user visibility
zachmargolis
approved these changes
Dec 3, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎫 Ticket
LG-15183
🛠 Summary of changes
Fixes an issue where the "reCAPTCHA verify result received" event does not correctly associate the
user_idif the sign-in attempt is unsuccessful for a reason other than reCAPTCHA.This happens because the user is not yet signed-in. For "Email and Password Authentication" events, we work around this by explicitly passing the
user_idfrom the user associated with the given email parameter. The changes proposed here instead override the baseApplicationController#analytics_userto use this parameter-based user during the submission, so that all analytics logging will have that user associated.📜 Testing Plan
Verify that both "Email and Password Authentication" and "reCAPTCHA verify result received" events include the
user_idproperty of the user associated with the submitted email address:make watch_eventsin a separate terminal processniluser_id)make watch_eventsprocess, observe events "Email and Password Authentication" and "reCAPTCHA verify result received" both have associateduser_id, regardless if you entered a correct password for the account