Skip to content
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.

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

UUID bug (ex: Testing race condition) #1366

Closed
janhohner opened this issue Feb 14, 2020 · 3 comments · Fixed by #2089
Closed

UUID bug (ex: Testing race condition) #1366

janhohner opened this issue Feb 14, 2020 · 3 comments · Fixed by #2089
Labels

Comments

@janhohner
Copy link

janhohner commented Feb 14, 2020

Hey,

the old issue text is below for reference but I've debugged the issue further and it seems to stem from a problem using UUIDs as primary keys.

I have been getting random test failures on tests that involve a user having permissions. The test case (not really relevant anymore, see below) expects to be able to access a page (http status 200) which works most of the time but because of problems sometimes returns 403.

Long story short, after hours of logging everything and anything I could find in our code and the laravel-permissions code I've started logging the queries and found the culprit:

    [10] => Array
        (
            [query] => select `permissions`.*, `model_has_permissions`.`model_uuid` as `pivot_model_uuid`, `model_has_permissions`.`permission_id` as `pivot_permission_id`, `model_has_permissions`.`model_type` as `pivot_model_type` from `permissions` inner join `model_has_permissions` on `permissions`.`id` = `model_has_permissions`.`permission_id` where `model_has_permissions`.`model_uuid` in (0) and `model_has_permissions`.`model_type` = ?
            [bindings] => Array
                (
                    [0] => App\User
                )

            [time] => 0.62
        )

(The interesting part is the where `model_has_permissions`.`model_uuid` in (0))
We've had that problem before in our core application: Laravel seems to cast the UUIDs to integer which returns really weird results which funnily enough work most of the time. But sometimes it just breaks. The solution is to set public $keyType = 'string'; on the models, which is something I can not do on the Permission and Role models.

One solution might be to to set $keyType on the models and do additional changes to the ones described in https://docs.spatie.be/laravel-permission/v3/installation-laravel/ to the migrations to change all keys to UUID. That could solve the problem.

Has anybody had the same issue or knows how to properly deal with this?

Hey,

I've been getting random test failures on tests that involve a user having permissions. The following test case sometimes fails (it should return 200 but gets 403 because somehow the permissions seem to be missing from the user):

public function test_when_calling_styleguide_as_a_user_with_the_styleguide_permission_then_it_should_show_the_styleguide()
    {
        // arrange
        $faker = Factory::create();

        app()[PermissionRegistrar::class]->forgetCachedPermissions();

        Permission::create(['name' => 'styleguide']);

        $this->app->make(PermissionRegistrar::class)->registerPermissions();

        $user = User::create([
            'email' => $faker->unique()->safeEmail,
            'email_verified_at' => now()->subYear(),
            'tos_accepted_at' => now(),
        ]);
        $user->givePermissionTo('styleguide');

        // act
        $response = $this->actingAs($user)
            ->get(route('styleguide'));

        // assert
        $response->assertStatus(200);
    }

The route called is defined as follows:

Route::middleware(['auth', 'permission:styleguide'])->group(function () {
    Route::get('styleguide', 'StaticPageController@styleguide')->name('styleguide');
});

and the controller method is a simple

public function styleguide()
{
    return view('welcome');
}

Does anybody have an idea what's going wrong here? I've hit a wall.

@janhohner janhohner changed the title Testing race condition UUID bug (ex: Testing race condition) Feb 14, 2020
@drbyte
Copy link
Collaborator

drbyte commented Feb 14, 2020

The solution is to set public $keyType = 'string'; on the models, which is something I can not do on the Permission and Role models.

You can if you extend them, can't you?

@drbyte drbyte added the support label Feb 14, 2020
@janhohner
Copy link
Author

janhohner commented Feb 14, 2020

We found the bug in our code - all our models extend a BaseModel which sets the $keyType except for the User model which already extends Laravel's Authenticatable class. This resulted in Laravel casting our userid to int. Sorry for the trouble!

@drbyte
Copy link
Collaborator

drbyte commented Feb 14, 2020

We found the bug in our code - all our models extend a BaseModel which sets the $keyType except for the User model which already extends Laravel's Authenticatable class. This resulted in Laravel casting our userid to int. Sorry for the trouble!

Thanks for the update. I'm considering some UUID updates to the docs, and I'll mention this there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants