ahinkle's avatar

How to test a database transction lock with `lockForUpdate()`?

We identified a bug where if a user initiated two password reset requests simultaneously through a third-party system, the system attempted to create the user account twice.

Here is how it works:

User Initiates Multiple Requests Simultaneously:

Request 1 (Click)

Request 2 (Click)

Database Checks:

Request 1: Verifies user existence in the database (False)

Request 2: Verifies user existence in the database (False)

External System Checks (e.g., BigCommerce):

Request 1: Confirms user existence at BigCommerce (True)

Request 2: Confirms user existence at BigCommerce (True)

User Creation and Password Reset Preparation:

Request 1: Creates the user and prepares for password reset (Sent)

Request 2: Attempts to create the user and prepare for password reset, resulting in an exception - SQL Integrity constraint violation, users.email must be unique

As a consequence, the user receives an 'Error' message. I have implemented tests to address this issue, but I am uncertain whether the problem lies in the code OR the test case.

        DB::beginTransaction();

        try {
            $user = User::where('email', $this->email)
                ->lockForUpdate() // Prevent multiple requests from creating accounts. 
                ->first();

            if (! $user) {
                $user = $this->findViaBigcommerce();
            }

            DB::commit();
        } catch (\Exception $e) {
            DB::rollBack();

            throw $e;
        }

Tests:

    public function test_it_creates_reset_token_on_bigcommerce_account()
    {
        Mail::fake();
        $u = User::factory()->make();
        Http::fake([
            '*/v3/customers*' => $this->fakeBigCommerceCustomerResponse($u),
        ]);
        $this->assertSame(0, User::count());

        $this->post('/api/v1/auth/forgot-password', ['email' => $u->email])
            ->assertStatus(204);

        Mail::assertQueued(PasswordResetEmail::class);
        $this->assertSame(1, User::count());
    }

    public function test_it_prevents_simultanous_requests()
    {
        // This isn't throwing an exception..
        $this->expectException(\Illuminate\Database\QueryException::class);
        
        $u = User::factory()->create();

       // Fake a lock on the user.
       User::where('email', $u->email)
            ->lockForUpdate()
            ->first();

        $this->post('/api/v1/auth/forgot-password', ['email' => $u->email]);
    }
0 likes
2 replies
ahinkle's avatar

Is it appropriate to give this a friendly bump? Unsure about the protocol. I'm more than willing to provide any additional information that might help in better understanding the question. :)

amaclean's avatar

Without looking too much into this... Try using a Real Time Facade of the User model.

In the test_it_prevents_simultanous_requests test, you'll have ->andThrow(new \Illuminate\Database\QueryException()); or something similar. You can remove the user creation and user selection queries from the test.

https://laravel.com/docs/10.x/facades#real-time-facades

1 like

Please or to participate in this conversation.