Merged
Conversation
The `RECAPTCHA_SIGN_IN` has a discriminator block that does the following: 1. Return `nil` if captcha was not performed (this puts the user in the default bucket) 2. Return the user's UUID if the user is present 3. Return a random value if no user is present The implementation of the bucket method checks if the discriminator is blank and returns `nil` if it is ([ref](https://github.com/18F/identity-idp/blob/983e21648dc7695f11c37e3eff7997ef4bd90f1e/lib/ab_test.rb#L46)). The `blank?` method has a special implementation implementation for `String` ([ref](https://github.com/rails/rails/blob/dfd1e951aa1aeef06c39fffb2994db8a8fa1914f/activesupport/lib/active_support/core_ext/object/blank.rb#L141-L163)) that returns `true` if the string consists of only whitespace. The random value here was generated using `SecureRandom.gen_random`. This value was then used in the `AbTest#bucket` to randomnly sort unidentified users into AB test buckets. `SecureRandom.gen_random` returns a string of random bytes which can include `" "`, `"\n"`, `"\r"`, etc. The combination of the above meant that the discriminator could generate value that caused the `AbTest#bucket` to return `nil` for `RECAPTCHA_SIGN_IN` when it was intended to return a bucket. This lead to a flaky test. This commit changes the call to generate a random string such that it only ever generates alphanumeric strings which will never be blank. [skip changelog]
Contributor
Alternatively, maybe this should check |
Contributor
Author
|
I figured it was safest to change the implementation in the ReCaptcha bucket to avoid any possible side-effects. I'm not sure why we use Another thought is that |
Contributor
|
That seems fine, yeah 👍 |
AShukla-GSA
pushed a commit
that referenced
this pull request
Sep 30, 2024
The `RECAPTCHA_SIGN_IN` A/B test has a discriminator block that does the following: 1. Return `nil` if captcha was not performed 2. Return the user's UUID if the user is present 3. Return a random value if no user is present with the intent of sorting them into a bucket The implementation of `AbTest#bucket` checks if the discriminator resolved by this discriminator block is blank and returns `nil` if it is ([ref](https://github.com/18F/identity-idp/blob/983e21648dc7695f11c37e3eff7997ef4bd90f1e/lib/ab_test.rb#L46)). The `blank?` method has a special implementation for `String` ([ref](https://github.com/rails/rails/blob/dfd1e951aa1aeef06c39fffb2994db8a8fa1914f/activesupport/lib/active_support/core_ext/object/blank.rb#L141-L163)) that returns `true` if the string consists of only whitespace. The random value here was generated using `SecureRandom.gen_random`. This value was then used in the `AbTest#bucket` to randomly sort unidentified users into AB test buckets. `SecureRandom.gen_random` returns a string of random bytes which can include `" "`, `"\n"`, `"\r"`, etc. The combination of the above meant that the discriminator could generate a value that caused the `AbTest#bucket` to return `nil` for `RECAPTCHA_SIGN_IN` when it was intended to return a bucket. This lead to a flaky test ([this spec](https://github.com/18F/identity-idp/blob/983e21648dc7695f11c37e3eff7997ef4bd90f1e/spec/config/initializers/ab_tests_spec.rb#L197-L203) tests that the bucket is not nil; it is nil in the case where `SecureRandom.gen_random` returns a blank string). This commit changes the call to `SecureRandom` so that it only ever generates alphanumeric strings which will never be blank. [skip changelog]
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
2 participants
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.
The
RECAPTCHA_SIGN_INA/B test has a discriminator block that does the following:nilif captcha was not performedThe implementation of
AbTest#bucketchecks if the discriminator resolved by this discriminator block is blank and returnsnilif it is (ref).The
blank?method has a special implementation forString(ref) that returnstrueif the string consists of only whitespace.The random value here was generated using
SecureRandom.gen_random. This value was then used in theAbTest#bucketto randomly sort unidentified users into AB test buckets.SecureRandom.gen_randomreturns a string of random bytes which can include" ","\n","\r", etc.The combination of the above meant that the discriminator could generate a value that caused the
AbTest#bucketto returnnilforRECAPTCHA_SIGN_INwhen it was intended to return a bucket. This lead to a flaky test (this spec tests that the bucket is not nil; it is nil in the case whereSecureRandom.gen_randomreturns a blank string).This commit changes the call to
SecureRandomso that it only ever generates alphanumeric strings which will never be blank.