Add encryption of Service field - Closes #365

app/Console/Commands/Maintenance/FixServiceFieldEncryption.php

@@ -0,0 +1,128 @@
+<?php
+
+namespace App\Console\Commands\Maintenance;
+
+use App\Facades\Settings;
+use App\Models\TwoFAccount;
+use Exception;
+use Illuminate\Console\Command;
+use Illuminate\Support\Facades\Crypt;
+use Illuminate\Support\Facades\DB;
+use Illuminate\Support\Facades\Log;
+use Throwable;
+
+/**
+ * @codeCoverageIgnore
+ */
+class FixServiceFieldEncryption extends Command
+{
+    /**
+     * The name and signature of the console command.
+     *
+     * @var string
+     */
+    protected $signature = '2fauth:fix-service-encryption';
+
+    /**
+     * The console command description.
+     *
+     * @var string
+     */
+    protected $description = 'Check and encrypt 2FA accounts Service field';
+
+    /**
+     * Indicates whether the command should be shown in the Artisan command list.
+     *
+     * @var bool
+     */
+    protected $hidden = true;
+
+    /**
+     * The name of the migration that changed the data this command will try to fix
+     */
+    protected string $relatedMigration = '2024_08_08_133136_encrypt_twofaccount_service_field';
+
+    /**
+     * Create a new command instance.
+     *
+     * @return void
+     */
+    public function __construct()
+    {
+        parent::__construct();
+    }
+
+    /**
+     * Execute the console command.
+     *
+     * @return mixed
+     */
+    public function handle()
+    {
+        if (DB::table('migrations')->where('migration', $this->relatedMigration)->doesntExist()) {
+            $this->fail(sprintf('Migration %s has not been run, this command cannot be used', $this->relatedMigration));
+        }
+
+        if (! Settings::get('useEncryption')) {
+            $this->fail('Database encryption is Off, this command cannot be used');
+        }
+
+        $this->encryptServiceField();
+    }
+
+    /**
+     * Encrypts the Service field of all TwoFAccount records
+     */
+    protected function encryptServiceField() : void
+    {
+        $twofaccounts = TwoFAccount::all();
+        $fullyEncryptedTwofaccounts = $twofaccounts->whereNotIn('service', [__('errors.indecipherable')]);
+        $partiallyEncryptedTwofaccounts = $twofaccounts->where('service', __('errors.indecipherable'));
+
+        if ($fullyEncryptedTwofaccounts->count() === $twofaccounts->count()) {
+            $this->components->info('The Service field is fully encrypted');
+
+            return;
+        }
+        else {
+            $this->newLine();
+            $this->components->warn('The Service field is not fully encrypted, although it should be.');
+            $this->line('ID of corresponding records in the twofaccounts table:');
+            $this->line($partiallyEncryptedTwofaccounts->implode('id', ', '));
+
+            if ($this->confirm('Do you want to fix encryption of those records?', true)) {
+                $error = 0;
+                $partiallyEncryptedTwofaccounts->each(function (TwoFAccount $twofaccount, int $key) use (&$error) {
+                    // We don't want to encrypt the Service field with a different APP_KEY
+                    // than the one used to encrypt the legacy_uri, account and secret fields, the
+                    // model would be inconsistent. 
+                    if (str_starts_with($twofaccount->legacy_uri, 'otpauth://')) {
+                        $rawServiceValue = $twofaccount->getRawOriginal('service');
+                        $twofaccount->service = $rawServiceValue;
+                        $twofaccount->save();
+                        $this->components->task(sprintf('Fixing twofaccount record with ID #%s', $twofaccount->id));
+                    }
+                    else {
+                        $error += 1;
+                        $this->components->task(sprintf('Fixing twofaccount record with ID #%s', $twofaccount->id), function() { return false; });
+                        $this->components->error('Wrong encryption key: The current APP_KEY cannot decipher already encrypted fields, encrypting the Service field with this key would lead to inconsistent data encryption');
+                    }
+                });
+
+                $this->newLine();
+
+                if ($error > 0) {
+                    $this->error(sprintf('%s record%s could not be fixed, see log above for details.', $error, $error > 1 ? 's' : ''));
+                }
+
+                //$this->line('Task completed');
+            }
+            else {
+                $this->components->warn('No fix applied.');
+                $this->line('You can re-run this command at any time to fix inconsistent records.');
+            }
+
+            return;
+        }
+    }
+}

app/Models/TwoFAccount.php

@@ -299,6 +299,29 @@ public function setAccountAttribute($value)
         $this->attributes['account'] = $this->encryptOrReturn($value);
     }
 
+    /**
+     * Get service attribute
+     *
+     * @param  string  $value
+     * @return string
+     */
+    public function getServiceAttribute($value)
+    {
+        return $this->decryptOrReturn($value);
+    }
+
+    /**
+     * Set service attribute
+     *
+     * @param  string  $value
+     * @return void
+     */
+    public function setServiceAttribute($value)
+    {
+        // Encrypt when needed
+        $this->attributes['service'] = $this->encryptOrReturn($value);
+    }
+
     /**
      * Get secret attribute
      *
@@ -775,6 +798,7 @@ private function decryptOrReturn(mixed $value) : mixed
             try {
                 return Crypt::decryptString($value);
             } catch (Exception $ex) {
+                Log::debug(sprintf('Service field of twofaccount with id #%s cannot be deciphered', $this->id));
                 return __('errors.indecipherable');
             }
         } else {

app/Services/SettingService.php

@@ -210,6 +210,7 @@ private function updateRecords(bool $encrypted) : bool
 
         $twofaccounts->each(function ($item, $key) use (&$success, $encrypted) {
             try {
+                $item->service    = $encrypted ? Crypt::encryptString($item->service) : Crypt::decryptString($item->service);
                 $item->legacy_uri = $encrypted ? Crypt::encryptString($item->legacy_uri) : Crypt::decryptString($item->legacy_uri);
                 $item->account    = $encrypted ? Crypt::encryptString($item->account) : Crypt::decryptString($item->account);
                 $item->secret     = $encrypted ? Crypt::encryptString($item->secret) : Crypt::decryptString($item->secret);
@@ -231,6 +232,7 @@ private function updateRecords(bool $encrypted) : bool
                     DB::table('twofaccounts')
                         ->where('id', $item->id)
                         ->update([
+                            'service'    => $item->service,
                             'legacy_uri' => $item->legacy_uri,
                             'account'    => $item->account,
                             'secret'     => $item->secret,

database/migrations/2024_08_08_133135_alter_twofaccount_service_field_to_text.php

@@ -0,0 +1,28 @@
+<?php
+
+use Illuminate\Database\Migrations\Migration;
+use Illuminate\Database\Schema\Blueprint;
+use Illuminate\Support\Facades\Schema;
+
+return new class extends Migration
+{
+    /**
+     * Run the migrations.
+     */
+    public function up(): void
+    {
+        Schema::table('twofaccounts', function (Blueprint $table) {
+            $table->text('service')->nullable()->change();
+        });
+    }
+
+    /**
+     * Reverse the migrations.
+     */
+    public function down(): void
+    {
+        // If for any reason, the migration is rolled back while the field data are still
+        // encrypted, restoring from Text to String type would trunkate the data, making them
+        // definitly undecipherable. So we do not restore the original type.
+    }
+};

database/migrations/2024_08_08_133136_encrypt_twofaccount_service_field.php

@@ -0,0 +1,85 @@
+<?php
+
+use App\Facades\Settings;
+use App\Models\TwoFAccount;
+use Illuminate\Database\Migrations\Migration;
+use Illuminate\Support\Facades\Log;
+use Illuminate\Support\Facades\Schema;
+
+return new class extends Migration
+{
+    /**
+     * Run the migrations.
+     */
+    public function up(): void
+    {
+        if ($this->dbIsEncrypted() && Schema::getColumnType('twofaccounts', 'service') === 'text') {
+            $this->encryptServiceField();
+        }
+    }
+
+    /**
+     * Reverse the migrations.
+     */
+    public function down(): void
+    {
+        if ($this->dbIsEncrypted()) {
+            $this->decryptServiceField();
+        }
+    }
+
+    /**
+     * Return the encryption state of the database
+     */
+    protected function dbIsEncrypted() : bool
+    {
+        return Settings::get('useEncryption');
+    }
+
+    /**
+     * Update the Service field of all twofaccounts records to its encrypted form
+    */
+    protected function encryptServiceField() : void
+    {
+        foreach (TwoFAccount::all() as $twofaccount) {
+            Log::notice(sprintf('Migration: Trying to encrypt Service field for twofaccount with id #%s', $twofaccount->id));
+        
+            // We don't want to encrypt the Service field with a different APP_KEY
+            // than the one used to encrypt the legacy_uri, account and secret fields, the
+            // model would be inconsistent. 
+            if ($twofaccount->legacy_uri === __('errors.indecipherable')) {
+                Log::warning(sprintf('Migration: Service encryption failed for twofaccount with id #%s. The current APP_KEY cannot decipher already encrypted fields, encrypting the Service field with this key would lead to inconsistent model encryption', $twofaccount->id));
+            }
+            else {
+                $rawServiceValue = $twofaccount->getRawOriginal('service');
+                $twofaccount->service = $rawServiceValue;
+                $twofaccount->save()
+                    ? Log::notice(sprintf('Migration: Service encryption successful for twofaccount with id #%s',  $twofaccount->id))
+                    : Log::warning(sprintf('Migration: Model saving failed for twofaccount with id #%s. The Service field was successfully encrypted but the change was not persisted to db', $twofaccount->id));
+            }
+        }
+    }
+
+    /**
+     * Update the Service field of all twofaccounts records to a readable form
+    */
+    protected function decryptServiceField() : void
+    {
+        foreach (TwoFAccount::all() as $twofaccount) {
+            Log::notice(sprintf('Migration rollback: Trying to decipher Service field for twofaccount with id #%s', $twofaccount->id));
+
+            if ($twofaccount->legacy_uri === __('errors.indecipherable')) {
+                Log::warning(sprintf('Migration rollback: Service decipherement failed for twofaccount with id #%s', $twofaccount->id));
+            }
+            else {
+                DB::table('twofaccounts')
+                    ->where('id', $twofaccount->id)
+                    ->update([
+                        'service' => $twofaccount->service,
+                    ]);
+
+                Log::notice(sprintf('Migration rollback: Service decipherement successful for twofaccount with id #%s',  $twofaccount->id));
+            }
+        }
+    }
+};

tests/Feature/Console/FixServiceFieldEncryptionTest.php

@@ -0,0 +1,181 @@
+<?php
+
+namespace Tests\Feature\Console;
+
+use App\Console\Commands\Maintenance\FixServiceFieldEncryption;
+use App\Facades\Settings;
+use App\Models\TwoFAccount;
+use App\Models\User;
+use Illuminate\Support\Facades\DB;
+use PHPUnit\Framework\Attributes\CoversClass;
+use PHPUnit\Framework\Attributes\Test;
+use Tests\FeatureTestCase;
+
+/**
+ * FixServiceFieldEncryptionTest test class
+ */
+#[CoversClass(FixServiceFieldEncryption::class)]
+class FixServiceFieldEncryptionTest extends FeatureTestCase
+{
+    /**
+     * The name of the migration that changed the data this command will try to fix
+     */
+    protected string $relatedMigration = '2024_08_08_133136_encrypt_twofaccount_service_field';
+    
+    /**
+     * @var \App\Models\User|\Illuminate\Contracts\Auth\Authenticatable
+     */
+    protected $user;
+
+    /**
+     * @var string
+     */
+    protected $command = '2fauth:fix-service-encryption';
+
+    public function setUp() : void
+    {
+        parent::setUp();
+
+        $this->user = User::factory()->create();
+    }
+
+    #[Test]
+    public function test_it_does_not_run_if_migration_has_not_been_run()
+    {
+        DB::table('migrations')->where('migration', $this->relatedMigration)->delete();
+
+        $this->artisan($this->command)
+            ->assertFailed();
+    }
+
+    #[Test]
+    public function test_it_does_not_run_if_encryption_is_off()
+    {
+        Settings::set('useEncryption', false);
+
+        $this->artisan($this->command)
+            ->assertFailed();
+    }
+
+    #[Test]
+    public function test_it_tells_the_field_is_fully_encrypted_when_it_is()
+    {
+        TwoFAccount::factory()->for($this->user)->count(3)->create();
+
+        Settings::set('useEncryption', true);
+
+        $this->artisan($this->command)
+            ->expectsOutputToContain('The Service field is fully encrypted.')
+            ->assertSuccessful();
+    }
+
+    #[Test]
+    public function test_it_encrypts_the_field_of_all_records()
+    {
+        TwoFAccount::factory()->for($this->user)->count(3)->create();
+        $expectedServiceName = 'unencrypted_text';
+
+        Settings::set('useEncryption', true);
+
+        DB::table('twofaccounts')->update(['service' => $expectedServiceName]);
+        $twofaccounts = TwoFAccount::all();
+        foreach ($twofaccounts as $twofaccount) {
+            $this->assertEquals(__('errors.indecipherable'), $twofaccount->service);
+        }
+
+        $this->artisan($this->command)
+            ->expectsConfirmation('Do you want to fix encryption of those records?', 'yes')
+            ->assertSuccessful();
+
+        foreach ($twofaccounts as $twofaccount) {
+            $twofaccount->refresh();
+            $this->assertEquals($expectedServiceName, $twofaccount->service);
+        }
+    }
+
+    #[Test]
+    public function test_it_does_not_encrypt_the_field_without_confirmation()
+    {
+        TwoFAccount::factory()->for($this->user)->count(3)->create();
+        $expectedServiceName = 'unencrypted_text';
+
+        Settings::set('useEncryption', true);
+
+        DB::table('twofaccounts')->update(['service' => $expectedServiceName]);
+        $twofaccounts = TwoFAccount::all();
+        foreach ($twofaccounts as $twofaccount) {
+            $this->assertEquals(__('errors.indecipherable'), $twofaccount->service);
+        }
+
+        $this->artisan($this->command)
+            ->expectsConfirmation('Do you want to fix encryption of those records?', 'no')
+            ->assertSuccessful();
+
+        foreach ($twofaccounts as $twofaccount) {
+            $twofaccount->refresh();
+            $this->assertEquals(__('errors.indecipherable'), $twofaccount->service);
+        }
+    }
+
+    #[Test]
+    public function test_it_encrypts_the_field_of_invalid_records_only()
+    {
+        Settings::set('useEncryption', true);
+
+        $expectedServiceName = 'myService';
+        $twofaccounts = TwoFAccount::factory()->for($this->user)->count(3)->create([
+            'service' => $expectedServiceName
+        ]);
+                
+        $testedAccount = $twofaccounts[2];
+        DB::table('twofaccounts')->where('id', $testedAccount->id)->update(['service' => $expectedServiceName]);
+
+        $testedAccount->refresh();
+
+        $this->assertEquals($expectedServiceName, $twofaccounts[0]->service);
+        $this->assertEquals($expectedServiceName, $twofaccounts[1]->service);
+        $this->assertEquals(__('errors.indecipherable'), $testedAccount->service);
+
+        $this->artisan($this->command)
+            ->expectsConfirmation('Do you want to fix encryption of those records?', 'yes')
+            ->assertSuccessful();
+
+        $testedAccount->refresh();
+        
+        $this->assertEquals($expectedServiceName, $twofaccounts[0]->service);
+        $this->assertEquals($expectedServiceName, $twofaccounts[1]->service);
+        $this->assertEquals($expectedServiceName, $testedAccount->service);
+    }
+
+    #[Test]
+    public function test_it_does_not_encrypt_the_record_if_encryption_is_not_consistent()
+    {
+        Settings::set('useEncryption', true);
+
+        $expectedServiceName = 'myService';
+        $twofaccounts = TwoFAccount::factory()->for($this->user)->count(3)->create([
+            'service' => $expectedServiceName
+        ]);
+                
+        $testedAccount = $twofaccounts[2];
+
+        DB::table('twofaccounts')->where('id', $testedAccount->id)->update(['legacy_uri' => 'indecipherable_payload']);
+        DB::table('twofaccounts')->where('id', $testedAccount->id)->update(['service' => $expectedServiceName]);
+
+        $testedAccount->refresh();
+
+        $this->assertEquals($expectedServiceName, $twofaccounts[0]->service);
+        $this->assertEquals($expectedServiceName, $twofaccounts[1]->service);
+        $this->assertEquals(__('errors.indecipherable'), $testedAccount->service);
+
+        $this->artisan($this->command)
+            ->expectsConfirmation('Do you want to fix encryption of those records?', 'yes')
+            ->expectsOutput('1 record could not be fixed, see log above for details.');
+
+        $testedAccount->refresh();
+        
+        $this->assertEquals($expectedServiceName, $twofaccounts[0]->service);
+        $this->assertEquals($expectedServiceName, $twofaccounts[1]->service);
+        $this->assertEquals(__('errors.indecipherable'), $testedAccount->service);
+    }
+}