From dae0a93ce84a1d0a218799aabbbb4540b43ffa62 Mon Sep 17 00:00:00 2001 From: Bubka <858858+Bubka@users.noreply.github.com> Date: Wed, 22 Mar 2023 15:39:51 +0100 Subject: [PATCH] Fix duplicate detection being made on all twofaccounts in db --- app/Services/TwoFAccountService.php | 10 ++- .../Controllers/TwoFAccountControllerTest.php | 89 ++++++++++++++++--- 2 files changed, 82 insertions(+), 17 deletions(-) diff --git a/app/Services/TwoFAccountService.php b/app/Services/TwoFAccountService.php index daf207e1..d0c7bdb9 100644 --- a/app/Services/TwoFAccountService.php +++ b/app/Services/TwoFAccountService.php @@ -6,6 +6,7 @@ use App\Helpers\Helpers; use App\Models\TwoFAccount; use Illuminate\Support\Collection; +use Illuminate\Support\Facades\Auth; use Illuminate\Support\Facades\Log; class TwoFAccountService @@ -93,17 +94,18 @@ public static function delete($ids) : int } /** - * Return the given collection with items marked as Duplicates (using id=-1) if a similar record exists in database + * Return the given collection with items marked as Duplicates (using id=-1) if similar records exist + * in the authenticated user accounts * * @param \Illuminate\Support\Collection $twofaccounts * @return \Illuminate\Support\Collection */ private static function markAsDuplicate(Collection $twofaccounts) : Collection { - $storage = TwoFAccount::all(); + $userTwofaccounts = Auth::user()->twofaccounts; - $twofaccounts = $twofaccounts->map(function ($twofaccount, $key) use ($storage) { - if ($storage->contains(function ($value, $key) use ($twofaccount) { + $twofaccounts = $twofaccounts->map(function ($twofaccount, $key) use ($userTwofaccounts) { + if ($userTwofaccounts->contains(function ($value, $key) use ($twofaccount) { return $value->secret == $twofaccount->secret && $value->service == $twofaccount->service && $value->account == $twofaccount->account diff --git a/tests/Api/v1/Controllers/TwoFAccountControllerTest.php b/tests/Api/v1/Controllers/TwoFAccountControllerTest.php index 12b60671..7770bdee 100644 --- a/tests/Api/v1/Controllers/TwoFAccountControllerTest.php +++ b/tests/Api/v1/Controllers/TwoFAccountControllerTest.php @@ -541,7 +541,7 @@ public function test_update_twofaccount_of_another_user_is_forbidden() /** * @test */ - public function test_import_valid_gauth_payload_returns_success_with_consistent_resources() + public function test_migrate_valid_gauth_payload_returns_success_with_consistent_resources() { $response = $this->actingAs($this->user, 'api-guard') ->json('POST', '/api/v1/twofaccounts/migration', [ @@ -577,7 +577,7 @@ public function test_import_valid_gauth_payload_returns_success_with_consistent_ /** * @test */ - public function test_import_with_invalid_gauth_payload_returns_validation_error() + public function test_migrate_with_invalid_gauth_payload_returns_validation_error() { $response = $this->actingAs($this->user, 'api-guard') ->json('POST', '/api/v1/twofaccounts/migration', [ @@ -589,9 +589,9 @@ public function test_import_with_invalid_gauth_payload_returns_validation_error( /** * @test */ - public function test_import_gauth_payload_with_duplicates_returns_negative_ids() + public function test_migrate_payload_with_duplicates_returns_negative_ids() { - $twofaccount = TwoFAccount::factory()->create([ + $twofaccount = TwoFAccount::factory()->for($this->user)->create([ 'otp_type' => 'totp', 'account' => OtpTestData::ACCOUNT, 'service' => OtpTestData::SERVICE, @@ -604,21 +604,84 @@ public function test_import_gauth_payload_with_duplicates_returns_negative_ids() ]); $response = $this->actingAs($this->user, 'api-guard') - ->json('POST', '/api/v1/twofaccounts/migration', [ + ->json('POST', '/api/v1/twofaccounts/migration?withSecret=1', [ 'payload' => MigrationTestData::GOOGLE_AUTH_MIGRATION_URI, ]) ->assertOk() ->assertJsonFragment([ - 'id' => -1, - 'service' => OtpTestData::SERVICE, - 'account' => OtpTestData::ACCOUNT, + 'id' => -1, + 'service' => OtpTestData::SERVICE, + 'account' => OtpTestData::ACCOUNT, + 'otp_type' => 'totp', + 'secret' => OtpTestData::SECRET, + 'digits' => OtpTestData::DIGITS_DEFAULT, + 'algorithm' => OtpTestData::ALGORITHM_DEFAULT, + 'period' => OtpTestData::PERIOD_DEFAULT, + 'counter' => null, + ]) + ->assertJsonFragment([ + 'id' => 0, + 'service' => OtpTestData::SERVICE . '_bis', + 'account' => OtpTestData::ACCOUNT . '_bis', + 'otp_type' => 'totp', + 'secret' => OtpTestData::SECRET, + 'digits' => OtpTestData::DIGITS_DEFAULT, + 'algorithm' => OtpTestData::ALGORITHM_DEFAULT, + 'period' => OtpTestData::PERIOD_DEFAULT, + 'counter' => null, ]); } /** * @test */ - public function test_import_invalid_gauth_payload_returns_bad_request() + public function test_migrate_identify_duplicates_in_authenticated_user_twofaccounts_only() + { + $twofaccount = TwoFAccount::factory()->for($this->anotherUser)->create([ + 'otp_type' => 'totp', + 'account' => OtpTestData::ACCOUNT, + 'service' => OtpTestData::SERVICE, + 'secret' => OtpTestData::SECRET, + 'algorithm' => OtpTestData::ALGORITHM_DEFAULT, + 'digits' => OtpTestData::DIGITS_DEFAULT, + 'period' => OtpTestData::PERIOD_DEFAULT, + 'legacy_uri' => OtpTestData::TOTP_SHORT_URI, + 'icon' => '', + ]); + + $response = $this->actingAs($this->user, 'api-guard') + ->json('POST', '/api/v1/twofaccounts/migration?withSecret=1', [ + 'payload' => MigrationTestData::GOOGLE_AUTH_MIGRATION_URI, + ]) + ->assertOk() + ->assertJsonFragment([ + 'id' => 0, + 'account' => OtpTestData::ACCOUNT, + 'service' => OtpTestData::SERVICE, + 'otp_type' => 'totp', + 'secret' => OtpTestData::SECRET, + 'algorithm' => OtpTestData::ALGORITHM_DEFAULT, + 'digits' => OtpTestData::DIGITS_DEFAULT, + 'period' => OtpTestData::PERIOD_DEFAULT, + 'icon' => null, + ]) + ->assertJsonFragment([ + 'id' => 0, + 'service' => OtpTestData::SERVICE . '_bis', + 'account' => OtpTestData::ACCOUNT . '_bis', + 'otp_type' => 'totp', + 'secret' => OtpTestData::SECRET, + 'digits' => OtpTestData::DIGITS_DEFAULT, + 'algorithm' => OtpTestData::ALGORITHM_DEFAULT, + 'period' => OtpTestData::PERIOD_DEFAULT, + 'counter' => null, + ]); + } + + /** + * @test + */ + public function test_migrate_invalid_gauth_payload_returns_bad_request() { $response = $this->actingAs($this->user, 'api-guard') ->json('POST', '/api/v1/twofaccounts/migration', [ @@ -633,7 +696,7 @@ public function test_import_invalid_gauth_payload_returns_bad_request() /** * @test */ - public function test_import_valid_aegis_json_file_returns_success() + public function test_migrate_valid_aegis_json_file_returns_success() { $file = LocalFile::fake()->validAegisJsonFile(); @@ -685,7 +748,7 @@ public function test_import_valid_aegis_json_file_returns_success() * * @dataProvider invalidAegisJsonFileProvider */ - public function test_import_invalid_aegis_json_file_returns_bad_request($file) + public function test_migrate_invalid_aegis_json_file_returns_bad_request($file) { $response = $this->withHeaders(['Content-Type' => 'multipart/form-data']) ->actingAs($this->user, 'api-guard') @@ -715,7 +778,7 @@ public function invalidAegisJsonFileProvider() * * @dataProvider validPlainTextFileProvider */ - public function test_import_valid_plain_text_file_returns_success($file) + public function test_migrate_valid_plain_text_file_returns_success($file) { $response = $this->withHeaders(['Content-Type' => 'multipart/form-data']) ->actingAs($this->user, 'api-guard') @@ -780,7 +843,7 @@ public function validPlainTextFileProvider() * * @dataProvider invalidPlainTextFileProvider */ - public function test_import_invalid_plain_text_file_returns_bad_request($file) + public function test_migrate_invalid_plain_text_file_returns_bad_request($file) { $response = $this->withHeaders(['Content-Type' => 'multipart/form-data']) ->actingAs($this->user, 'api-guard')