Missing column `two_factor_confirmed_at` when migrations executed during installation

oltdaniel commented 6 months ago

Jetstream Version

5.0.5

Jetstream Stack

Livewire

Laravel Version

11.6.0

PHP Version

8.3.6

Database Driver & Version

No response

Description

Jetstream uses Fortify for the 2FA feature and the 2FA confirmation is enabled by default when installed. The affected column here checks that feature for the migration as follows:

// database/migrations/2024_05_05_084453_add_two_factor_columns_to_users_table.php
// [...]
            if (Fortify::confirmsTwoFactorAuthentication()) {
                $table->timestamp('two_factor_confirmed_at')
                    ->after('two_factor_recovery_codes')
                    ->nullable();
            }
[...]

If you confirm to migrate the new migrations during the installation of jetstream, this somehow fails and causes this column to be missing in the database, causing queries that expect this column to exist to fail. If you execute the command sail artisan migrate after the installation of jetstream is done, everything works as expected.

Steps To Reproduce

I used a sail setup for this and tested it with two identical projects, expect for executing migrations during the installation prompt or not.

curl -s "https://laravel.build/project001?with=pgsql,redis,meilisearch,mailpit,minio" | bash (less features shouldn't make a difference)
sail up
sail composer require laravel/jetstream
sail php artisan jetstream:install livewire --teams NOTE: Confirm to run migrations when asked to.
Register, enable 2FA and submit the confirm form. The view should crash due to missing column two_factor_confirmed_at.

jeffagostinho commented 6 months ago

Following...

I don't know if it's related, but I can access and enable 2FA, but even when I configure the QRcode correctly when typing the generated code, I always receive the message "The provided two factor authentication code was invalid."

driesvints commented 6 months ago

I can reproduce this. Sail doesn't matter, I've reproduced this on a fresh laravel app. I really wonder what's going on because by default 2FA is enabled through the fortify config with Features::twoFactorAuthentication. So this really shouldn't happen...

Would appreciate any insight here.

github-actions[bot] commented 6 months ago

Thank you for reporting this issue!

As Laravel is an open source project, we rely on the community to help us diagnose and fix issues as it is not possible to research and fix every issue reported to us via GitHub.

If possible, please make a pull request fixing the issue you have described, along with corresponding tests. All pull requests are promptly reviewed by the Laravel team.

Thank you!

danbracey commented 6 months ago

Can confirm this happens when clicking 'cancel' as well

willvincent commented 6 months ago

Unclear why this is happening, the fortify config is in place with 'confirm' set to true at the point of install when the message about migrations being added pops up asking if it should run those.

Starting tinker in another terminal while that prompt is awaiting response, I'm able to verify that Fortify::confirmsTwoFactorAuthentication() returns true, so that field should appropriately be added to the user table, however it does not get added. The workaround right now is to do a subsequent php artisan migrate:fresh

I wonder how necessary it really is to only conditionally include that confirmation date field in the user table, sure if someone weren't using confirmation (not the default) that field isn't necessary, but since it's nullable anyway, does it really hurt anything to just include it regardless? That obviously is more of a fortify issue..

ravibpatel commented 6 months ago

I tried adding var_dump(config('fortify-options')); before this line and it returned null. It is still unclear why it does that, though. I tried adding var_dump before and sometimes it returned the value as expected, but before this step it always returns null.

Executing the "migrate:fresh" command in another process fixed the issue. So instead of following.

$this->call('migrate:fresh', ['--force' => true]);

I tried following and it works.

(new Process([$this->phpBinary(), 'artisan', 'migrate:fresh', '--force'], base_path()))
    ->setTimeout(null)
    ->run(function ($type, $output) {
        $this->output->write($output);
    });

willvincent commented 6 months ago

That makes sense. Like how tinker is unaware of changes to config or code without being restarted.

The new process method seems like an appropriate solution... Albeit a bit ugly

laravel / jetstream