Skip to content

Commit

Permalink
πŸžπŸ”¨Authentication Prevent a condition when double-tapping sign-in
Browse files Browse the repository at this point in the history
- #1809

OK, so my working assumption is that the bug is caused by two requests
being processed at the same time for a new email address, and the first
one creates the `AuthenticationMethod` and the second one *tries* to,
and fails; resulting in the validation error.

This requeries the database and retries the rest of the work.

Would love to have names for methods to pull this out into, rather than
copy-pasting the method body but oh well.
  • Loading branch information
zspencer committed Oct 12, 2023
1 parent 2c1a024 commit f931f7d
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 15 deletions.
10 changes: 10 additions & 0 deletions app/models/authenticated_session.rb
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,16 @@ def save
end

false
rescue ActiveRecord::RecordInvalid
self.authentication_method = nil
if one_time_password.nil?
authentication_method.send_one_time_password!(space)
false
elsif authentication_method.verify?(one_time_password)
session[:person_id] = authentication_method.person.id
authentication_method.confirm!
true
end
end

# If we don't have a OTP _or_ a way of issuing one, there's nothin' we can do.
Expand Down
47 changes: 32 additions & 15 deletions spec/models/authenticated_session_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -71,29 +71,46 @@
expect { authenticated_session.save }.not_to change(AuthenticationMethod, :count)
end
end
end

context "and the correct otp is provided" do
let(:one_time_password) { "123456" }
context "when a race condition caused the authentication method was created in another thread" do
let(:authentication_method) { nil }

it "reloads the authentication method and tries again" do
sad_authentication_method = instance_double(AuthenticationMethod)
allow(sad_authentication_method).to receive(:send_one_time_password!).with(space).and_raise(ActiveRecord::RecordInvalid)
happy_authentication_method = instance_double(AuthenticationMethod)
allow(happy_authentication_method).to receive(:send_one_time_password!).with(space)
allow(AuthenticationMethod).to receive(:find_or_initialize_by).with(contact_method: :email, contact_location: "[email protected]").and_return(sad_authentication_method, happy_authentication_method)

before do
allow(authentication_method).to receive(:verify?)
.with(one_time_password).and_return(true)
authenticated_session.save

allow(authentication_method).to receive(:person)
.and_return(build_stubbed(:person, id: SecureRandom.uuid))
expect(sad_authentication_method).to have_received(:send_one_time_password!).with(space)
expect(happy_authentication_method).to have_received(:send_one_time_password!).with(space)
end
end

it "confirms the authentication method" do
authenticated_session.save
context "and the correct otp is provided" do
let(:one_time_password) { "123456" }

expect(authentication_method).to have_received(:confirm!)
end
before do
allow(authentication_method).to receive(:verify?)
.with(one_time_password).and_return(true)

it "populates the persons id in the session" do
authenticated_session.save
allow(authentication_method).to receive(:person)
.and_return(build_stubbed(:person, id: SecureRandom.uuid))
end

expect(session[:person_id]).to eql(authentication_method.person.id)
it "confirms the authentication method" do
authenticated_session.save

expect(authentication_method).to have_received(:confirm!)
end

it "populates the persons id in the session" do
authenticated_session.save

expect(session[:person_id]).to eql(authentication_method.person.id)
end
end
end
end
Expand Down

0 comments on commit f931f7d

Please sign in to comment.