From af4400a74d1d5c8928e89003e3a0cbcd61653627 Mon Sep 17 00:00:00 2001 From: Bubka <858858+Bubka@users.noreply.github.com> Date: Wed, 8 Mar 2023 09:41:18 +0100 Subject: [PATCH] Update Authorizations, Logs and Tests for TwoFAccounts management --- .../v1/Controllers/TwoFAccountController.php | 10 +- app/Models/TwoFAccount.php | 23 +- app/Policies/TwoFAccountPolicy.php | 60 ++++- app/Services/TwoFAccountService.php | 2 +- database/factories/TwoFAccountFactory.php | 2 +- .../Controllers/TwoFAccountControllerTest.php | 225 +++++++++++++----- .../Services/TwoFAccountServiceTest.php | 217 ++++++++++------- 7 files changed, 379 insertions(+), 160 deletions(-) diff --git a/app/Api/v1/Controllers/TwoFAccountController.php b/app/Api/v1/Controllers/TwoFAccountController.php index bb74e2e2..7a4a6d6e 100644 --- a/app/Api/v1/Controllers/TwoFAccountController.php +++ b/app/Api/v1/Controllers/TwoFAccountController.php @@ -133,7 +133,7 @@ public function reorder(TwoFAccountReorderRequest $request) $validated = $request->validated(); $twofaccounts = TwoFAccount::whereIn('id', $validated['orderedIds'])->get(); - $this->authorize('updateEach', [$twofaccounts[0], $twofaccounts]); + $this->authorize('updateEach', [new TwoFAccount(), $twofaccounts]); TwoFAccount::setNewOrder($validated['orderedIds']); @@ -172,7 +172,7 @@ public function export(TwoFAccountBatchRequest $request) } $twofaccounts = TwoFAccounts::export($validated['ids']); - $this->authorize('viewEach', [$twofaccounts[0], $twofaccounts]); + $this->authorize('viewEach', [new TwoFAccount(), $twofaccounts]); return new TwoFAccountExportCollection($twofaccounts); } @@ -250,7 +250,7 @@ public function withdraw(TwoFAccountBatchRequest $request) $ids = Helpers::commaSeparatedToArray($validated['ids']); $twofaccounts = TwoFAccount::whereIn('id', $ids)->get(); - $this->authorize('updateEach', [$twofaccounts[0], $twofaccounts]); + $this->authorize('updateEach', [new TwoFAccount(), $twofaccounts]); TwoFAccounts::withdraw($ids); @@ -267,7 +267,7 @@ public function destroy(TwoFAccount $twofaccount) { $this->authorize('delete', $twofaccount); - TwoFAccounts::delete($twofaccount->id); + $twofaccount->delete(); return response()->json(null, 204); } @@ -292,7 +292,7 @@ public function batchDestroy(TwoFAccountBatchRequest $request) $ids = Helpers::commaSeparatedToArray($validated['ids']); $twofaccounts = TwoFAccount::whereIn('id', $ids)->get(); - $this->authorize('deleteEach', [$twofaccounts[0], $twofaccounts]); + $this->authorize('deleteEach', [new TwoFAccount(), $twofaccounts]); TwoFAccounts::delete($validated['ids']); diff --git a/app/Models/TwoFAccount.php b/app/Models/TwoFAccount.php index 1dadd235..2181310d 100644 --- a/app/Models/TwoFAccount.php +++ b/app/Models/TwoFAccount.php @@ -177,9 +177,15 @@ protected static function boot() } }); - // static::deleted(function ($model) { - // Log::info(sprintf('TwoFAccount #%d deleted', $model->id)); - // }); + static::created(function (object $model) { + Log::info(sprintf('TwoFAccount ID #%d created for user ID #%s', $model->id, $model->user_id)); + }); + static::updated(function (object $model) { + Log::info(sprintf('TwoFAccount ID #%d updated by user ID #%s', $model->id, $model->user_id)); + }); + static::deleted(function (object $model) { + Log::info(sprintf('TwoFAccount ID #%d deleted ', $model->id)); + }); } /** @@ -408,7 +414,9 @@ public function fillWithOtpParameters(array $parameters, bool $skipIconFetching $this->enforceAsSteam(); } - if (! $this->icon && Auth::user()->preferences['getOfficialIcons'] && ! $skipIconFetching) { + $user = is_null($this->user) ? Auth::user() : $this->user; + + if (! $this->icon && $user->preferences['getOfficialIcons'] && ! $skipIconFetching) { $this->icon = $this->getDefaultIcon(); } @@ -460,7 +468,9 @@ public function fillWithURI(string $uri, bool $isSteamTotp = false, bool $skipIc self::setIcon($this->generator->getParameter('image')); } - if (! $this->icon && Auth::user()->preferences['getOfficialIcons'] && ! $skipIconFetching) { + $user = is_null($this->user) ? Auth::user() : $this->user; + + if (! $this->icon && $user->preferences['getOfficialIcons'] && ! $skipIconFetching) { $this->icon = $this->getDefaultIcon(); } @@ -697,8 +707,9 @@ private function storeRemoteImageAsIcon(string $url) : string|null private function getDefaultIcon() { $logoService = App::make(LogoService::class); + $user = is_null($this->user) ? Auth::user() : $this->user; - return Auth::user()->preferences['getOfficialIcons'] ? $logoService->getIcon($this->service) : null; + return $user->preferences['getOfficialIcons'] ? $logoService->getIcon($this->service) : null; } /** diff --git a/app/Policies/TwoFAccountPolicy.php b/app/Policies/TwoFAccountPolicy.php index 0e2040d7..a705d846 100644 --- a/app/Policies/TwoFAccountPolicy.php +++ b/app/Policies/TwoFAccountPolicy.php @@ -5,6 +5,7 @@ use App\Models\TwoFAccount; use App\Models\User; use Illuminate\Auth\Access\HandlesAuthorization; +use Illuminate\Support\Facades\Log; class TwoFAccountPolicy { @@ -30,7 +31,13 @@ public function viewAny(User $user) */ public function view(User $user, TwoFAccount $twofaccount) { - return $this->isOwnerOf($user, $twofaccount); + $can = $this->isOwnerOf($user, $twofaccount); + + if (! $can) { + Log::notice(sprintf('User ID #%s cannot view twofaccount ID #%s', $user->id, $twofaccount->id)); + } + + return $can; } /** @@ -43,7 +50,16 @@ public function view(User $user, TwoFAccount $twofaccount) */ public function viewEach(User $user, TwoFAccount $twofaccount, $twofaccounts) { - return $this->isOwnerOfEach($user, $twofaccounts); + $can = $this->isOwnerOfEach($user, $twofaccounts); + + if (! $can) { + $ids = $twofaccounts->map(function ($twofaccount, $key) { + return $twofaccount->id; + }); + Log::notice(sprintf('User ID #%s cannot view all twofaccounts in IDs #%s', $user->id, implode(',', $ids->toArray()))); + } + + return $can; } /** @@ -54,6 +70,8 @@ public function viewEach(User $user, TwoFAccount $twofaccount, $twofaccounts) */ public function create(User $user) { + // Log::notice(sprintf('User ID #%s cannot create twofaccounts', $user->id)); + return true; } @@ -66,7 +84,13 @@ public function create(User $user) */ public function update(User $user, TwoFAccount $twofaccount) { - return $this->isOwnerOf($user, $twofaccount); + $can = $this->isOwnerOf($user, $twofaccount); + + if (! $can) { + Log::notice(sprintf('User ID #%s cannot update twofaccount ID #%s', $user->id, $twofaccount->id)); + } + + return $can; } /** @@ -79,7 +103,16 @@ public function update(User $user, TwoFAccount $twofaccount) */ public function updateEach(User $user, TwoFAccount $twofaccount, $twofaccounts) { - return $this->isOwnerOfEach($user, $twofaccounts); + $can = $this->isOwnerOfEach($user, $twofaccounts); + + if (! $can) { + $ids = $twofaccounts->map(function ($twofaccount, $key) { + return $twofaccount->id; + }); + Log::notice(sprintf('User ID #%s cannot update all twofaccounts in IDs #%s', $user->id, implode(',', $ids->toArray()))); + } + + return $can; } /** @@ -91,7 +124,13 @@ public function updateEach(User $user, TwoFAccount $twofaccount, $twofaccounts) */ public function delete(User $user, TwoFAccount $twofaccount) { - return $this->isOwnerOf($user, $twofaccount); + $can = $this->isOwnerOf($user, $twofaccount); + + if (! $can) { + Log::notice(sprintf('User ID #%s cannot delete twofaccount ID #%s', $user->id, $twofaccount->id)); + } + + return $can; } /** @@ -104,7 +143,16 @@ public function delete(User $user, TwoFAccount $twofaccount) */ public function deleteEach(User $user, TwoFAccount $twofaccount, $twofaccounts) { - return $this->isOwnerOfEach($user, $twofaccounts); + $can = $this->isOwnerOfEach($user, $twofaccounts); + + if (! $can) { + $ids = $twofaccounts->map(function ($twofaccount, $key) { + return $twofaccount->id; + }); + Log::notice(sprintf('User ID #%s cannot delete all twofaccounts in IDs #%s', $user->id, implode(',', $ids->toArray()))); + } + + return $can; } /** diff --git a/app/Services/TwoFAccountService.php b/app/Services/TwoFAccountService.php index 2573e502..acdd0fea 100644 --- a/app/Services/TwoFAccountService.php +++ b/app/Services/TwoFAccountService.php @@ -42,7 +42,7 @@ public static function withdraw($ids) : void ['group_id' => null] ); - Log::info(sprintf('TwoFAccounts #%s withdrawn', implode(',#', $ids))); + Log::info(sprintf('TwoFAccounts IDs #%s withdrawn', implode(',', $ids))); } /** diff --git a/database/factories/TwoFAccountFactory.php b/database/factories/TwoFAccountFactory.php index 76d09d7b..b128a047 100644 --- a/database/factories/TwoFAccountFactory.php +++ b/database/factories/TwoFAccountFactory.php @@ -18,7 +18,7 @@ class TwoFAccountFactory extends Factory public function definition() { $account = $this->faker->safeEmail(); - $service = $this->faker->unique()->domainName(); + $service = $this->faker->domainName(); $secret = Base32::encodeUpper($this->faker->regexify('[A-Z0-9]{8}')); return [ diff --git a/tests/Api/v1/Controllers/TwoFAccountControllerTest.php b/tests/Api/v1/Controllers/TwoFAccountControllerTest.php index 5b21b454..8a1f1009 100644 --- a/tests/Api/v1/Controllers/TwoFAccountControllerTest.php +++ b/tests/Api/v1/Controllers/TwoFAccountControllerTest.php @@ -23,14 +23,19 @@ class TwoFAccountControllerTest extends FeatureTestCase { /** - * @var \App\Models\User + * @var \App\Models\User|\Illuminate\Contracts\Auth\Authenticatable */ - protected $user; + protected $user, $anotherUser; /** - * @var \App\Models\Group + * @var App\Models\Group */ - protected $group; + protected $userGroupA, $userGroupB, $anotherUserGroupA, $anotherUserGroupB; + + /** + * @var App\Models\TwoFAccount + */ + protected $twofaccountA, $twofaccountB, $twofaccountC, $twofaccountD; private const VALID_RESOURCE_STRUCTURE_WITHOUT_SECRET = [ 'id', @@ -129,8 +134,27 @@ public function setUp() : void { parent::setUp(); - $this->user = User::factory()->create(); - $this->group = Group::factory()->create(); + $this->user = User::factory()->create(); + $this->userGroupA = Group::factory()->for($this->user)->create(); + $this->userGroupB = Group::factory()->for($this->user)->create(); + + $this->twofaccountA = TwoFAccount::factory()->for($this->user)->create([ + 'group_id' => $this->userGroupA->id, + ]); + $this->twofaccountB = TwoFAccount::factory()->for($this->user)->create([ + 'group_id' => $this->userGroupA->id, + ]); + + $this->anotherUser = User::factory()->create(); + $this->anotherUserGroupA = Group::factory()->for($this->anotherUser)->create(); + $this->anotherUserGroupB = Group::factory()->for($this->anotherUser)->create(); + + $this->twofaccountC = TwoFAccount::factory()->for($this->anotherUser)->create([ + 'group_id' => $this->anotherUserGroupA->id, + ]); + $this->twofaccountD = TwoFAccount::factory()->for($this->anotherUser)->create([ + 'group_id' => $this->anotherUserGroupB->id, + ]); } /** @@ -138,16 +162,26 @@ public function setUp() : void * * @dataProvider indexUrlParameterProvider */ - public function test_index_returns_twofaccount_collection($urlParameter, $expected) + public function test_index_returns_user_twofaccounts_only($urlParameter, $expected) { - TwoFAccount::factory()->count(3)->create(); - $response = $this->actingAs($this->user, 'api-guard') ->json('GET', '/api/v1/twofaccounts' . $urlParameter) ->assertOk() - ->assertJsonCount(3, $key = null) + ->assertJsonCount(2, $key = null) ->assertJsonStructure([ '*' => $expected, + ]) + ->assertJsonFragment([ + 'id' => $this->twofaccountA->id, + ]) + ->assertJsonFragment([ + 'id' => $this->twofaccountB->id, + ]) + ->assertJsonMissing([ + 'id' => $this->twofaccountC->id, + ]) + ->assertJsonMissing([ + 'id' => $this->twofaccountD->id, ]); } @@ -171,12 +205,10 @@ public function indexUrlParameterProvider() /** * @test */ - public function test_show_twofaccount_returns_twofaccount_resource_with_secret() + public function test_show_returns_twofaccount_resource_with_secret() { - $twofaccount = TwoFAccount::factory()->create(); - $response = $this->actingAs($this->user, 'api-guard') - ->json('GET', '/api/v1/twofaccounts/' . $twofaccount->id) + ->json('GET', '/api/v1/twofaccounts/' . $this->twofaccountA->id) ->assertOk() ->assertJsonStructure(self::VALID_RESOURCE_STRUCTURE_WITH_SECRET); } @@ -184,12 +216,10 @@ public function test_show_twofaccount_returns_twofaccount_resource_with_secret() /** * @test */ - public function test_show_twofaccount_returns_twofaccount_resource_without_secret() + public function test_show_returns_twofaccount_resource_without_secret() { - $twofaccount = TwoFAccount::factory()->create(); - $response = $this->actingAs($this->user, 'api-guard') - ->json('GET', '/api/v1/twofaccounts/' . $twofaccount->id . '?withSecret=0') + ->json('GET', '/api/v1/twofaccounts/' . $this->twofaccountA->id . '?withSecret=0') ->assertOk() ->assertJsonStructure(self::VALID_RESOURCE_STRUCTURE_WITHOUT_SECRET); } @@ -232,6 +262,19 @@ public function test_show_missing_twofaccount_returns_not_found() ]); } + /** + * @test + */ + public function test_show_twofaccount_of_another_user_is_forbidden() + { + $response = $this->actingAs($this->user, 'api-guard') + ->json('GET', '/api/v1/twofaccounts/' . $this->twofaccountC->id) + ->assertForbidden() + ->assertJsonStructure([ + 'message', + ]); + } + /** * @dataProvider accountCreationProvider * @test @@ -331,14 +374,15 @@ public function test_store_with_invalid_uri_returns_validation_error() public function test_store_assigns_created_account_when_default_group_is_a_specific_one() { // Set the default group to a specific one - Settings::set('defaultGroup', $this->group->id); + $this->user['preferences->defaultGroup'] = $this->userGroupA->id; + $this->user->save(); $response = $this->actingAs($this->user, 'api-guard') ->json('POST', '/api/v1/twofaccounts', [ 'uri' => OtpTestData::TOTP_SHORT_URI, ]) ->assertJsonFragment([ - 'group_id' => $this->group->id, + 'group_id' => $this->userGroupA->id, ]); } @@ -348,16 +392,17 @@ public function test_store_assigns_created_account_when_default_group_is_a_speci public function test_store_assigns_created_account_when_default_group_is_the_active_one() { // Set the default group to be the active one - Settings::set('defaultGroup', -1); + $this->user['preferences->defaultGroup'] = -1; // Set the active group - Settings::set('activeGroup', $this->group->id); + $this->user['preferences->activeGroup'] = $this->userGroupA->id; + $this->user->save(); $response = $this->actingAs($this->user, 'api-guard') ->json('POST', '/api/v1/twofaccounts', [ 'uri' => OtpTestData::TOTP_SHORT_URI, ]) ->assertJsonFragment([ - 'group_id' => $this->group->id, + 'group_id' => $this->userGroupA->id, ]); } @@ -367,7 +412,8 @@ public function test_store_assigns_created_account_when_default_group_is_the_act public function test_store_assigns_created_account_when_default_group_is_no_group() { // Set the default group to No group - Settings::set('defaultGroup', 0); + $this->user['preferences->defaultGroup'] = 0; + $this->user->save(); $response = $this->actingAs($this->user, 'api-guard') ->json('POST', '/api/v1/twofaccounts', [ @@ -384,7 +430,8 @@ public function test_store_assigns_created_account_when_default_group_is_no_grou public function test_store_assigns_created_account_when_default_group_does_not_exist() { // Set the default group to a non-existing one - Settings::set('defaultGroup', 1000); + $this->user['preferences->defaultGroup'] = 1000; + $this->user->save(); $response = $this->actingAs($this->user, 'api-guard') ->json('POST', '/api/v1/twofaccounts', [ @@ -400,10 +447,8 @@ public function test_store_assigns_created_account_when_default_group_does_not_e */ public function test_update_totp_returns_success_with_updated_resource() { - $twofaccount = TwoFAccount::factory()->create(); - $response = $this->actingAs($this->user, 'api-guard') - ->json('PUT', '/api/v1/twofaccounts/' . $twofaccount->id, OtpTestData::ARRAY_OF_FULL_VALID_PARAMETERS_FOR_CUSTOM_TOTP) + ->json('PUT', '/api/v1/twofaccounts/' . $this->twofaccountA->id, OtpTestData::ARRAY_OF_FULL_VALID_PARAMETERS_FOR_CUSTOM_TOTP) ->assertOk() ->assertJsonFragment(self::JSON_FRAGMENTS_FOR_CUSTOM_TOTP); } @@ -413,10 +458,8 @@ public function test_update_totp_returns_success_with_updated_resource() */ public function test_update_hotp_returns_success_with_updated_resource() { - $twofaccount = TwoFAccount::factory()->create(); - $response = $this->actingAs($this->user, 'api-guard') - ->json('PUT', '/api/v1/twofaccounts/' . $twofaccount->id, OtpTestData::ARRAY_OF_FULL_VALID_PARAMETERS_FOR_CUSTOM_HOTP) + ->json('PUT', '/api/v1/twofaccounts/' . $this->twofaccountA->id, OtpTestData::ARRAY_OF_FULL_VALID_PARAMETERS_FOR_CUSTOM_HOTP) ->assertOk() ->assertJsonFragment(self::JSON_FRAGMENTS_FOR_CUSTOM_HOTP); } @@ -439,10 +482,23 @@ public function test_update_twofaccount_with_invalid_data_returns_validation_err $twofaccount = TwoFAccount::factory()->create(); $response = $this->actingAs($this->user, 'api-guard') - ->json('PUT', '/api/v1/twofaccounts/' . $twofaccount->id, self::ARRAY_OF_INVALID_PARAMETERS) + ->json('PUT', '/api/v1/twofaccounts/' . $this->twofaccountA->id, self::ARRAY_OF_INVALID_PARAMETERS) ->assertStatus(422); } + /** + * @test + */ + public function test_update_twofaccount_of_another_user_is_forbidden() + { + $response = $this->actingAs($this->user, 'api-guard') + ->json('PUT', '/api/v1/twofaccounts/' . $this->twofaccountC->id, OtpTestData::ARRAY_OF_FULL_VALID_PARAMETERS_FOR_CUSTOM_HOTP) + ->assertForbidden() + ->assertJsonStructure([ + 'message', + ]); + } + /** * @test */ @@ -721,11 +777,9 @@ public function invalidPlainTextFileProvider() */ public function test_reorder_returns_success() { - TwoFAccount::factory()->count(3)->create(); - $response = $this->actingAs($this->user, 'api-guard') ->json('POST', '/api/v1/twofaccounts/reorder', [ - 'orderedIds' => [3, 2, 1], + 'orderedIds' => [$this->twofaccountB->id, $this->twofaccountA->id], ]) ->assertStatus(200) ->assertJsonStructure([ @@ -738,8 +792,6 @@ public function test_reorder_returns_success() */ public function test_reorder_with_invalid_data_returns_validation_error() { - TwoFAccount::factory()->count(3)->create(); - $response = $this->actingAs($this->user, 'api-guard') ->json('POST', '/api/v1/twofaccounts/reorder', [ 'orderedIds' => '3,2,1', @@ -747,6 +799,21 @@ public function test_reorder_with_invalid_data_returns_validation_error() ->assertStatus(422); } + /** + * @test + */ + public function test_reorder_twofaccounts_of_another_user_is_forbidden() + { + $response = $this->actingAs($this->user, 'api-guard') + ->json('POST', '/api/v1/twofaccounts/reorder', [ + 'orderedIds' => [$this->twofaccountB->id, $this->twofaccountD->id], + ]) + ->assertForbidden() + ->assertJsonStructure([ + 'message', + ]); + } + /** * @test */ @@ -792,7 +859,7 @@ public function test_preview_with_unreachable_image_returns_success() */ public function test_get_otp_using_totp_twofaccount_id_returns_consistent_resource() { - $twofaccount = TwoFAccount::factory()->create([ + $twofaccount = TwoFAccount::factory()->for($this->user)->create([ 'otp_type' => 'totp', 'account' => OtpTestData::ACCOUNT, 'service' => OtpTestData::SERVICE, @@ -851,7 +918,7 @@ public function test_get_otp_by_posting_totp_parameters_returns_consistent_resou */ public function test_get_otp_using_hotp_twofaccount_id_returns_consistent_resource() { - $twofaccount = TwoFAccount::factory()->create([ + $twofaccount = TwoFAccount::factory()->for($this->user)->create([ 'otp_type' => 'hotp', 'account' => OtpTestData::ACCOUNT, 'service' => OtpTestData::SERVICE, @@ -929,7 +996,7 @@ public function test_get_otp_using_indecipherable_twofaccount_id_returns_bad_req { Settings::set('useEncryption', true); - $twofaccount = TwoFAccount::factory()->create(); + $twofaccount = TwoFAccount::factory()->for($this->user)->create(); DB::table('twofaccounts') ->where('id', $twofaccount->id) @@ -980,15 +1047,26 @@ public function test_get_otp_by_posting_invalid_parameters_returns_validation_er /** * @test */ - public function test_count_returns_right_number_of_twofaccount() + public function test_get_otp_of_another_user_twofaccount_is_forbidden() { - TwoFAccount::factory()->count(3)->create(); + $response = $this->actingAs($this->user, 'api-guard') + ->json('GET', '/api/v1/twofaccounts/'.$this->twofaccountC->id.'/otp') + ->assertForbidden() + ->assertJsonStructure([ + 'message', + ]); + } + /** + * @test + */ + public function test_count_returns_right_number_of_twofaccounts() + { $response = $this->actingAs($this->user, 'api-guard') ->json('GET', '/api/v1/twofaccounts/count') ->assertStatus(200) ->assertExactJson([ - 'count' => 3, + 'count' => 2, ]); } @@ -997,11 +1075,8 @@ public function test_count_returns_right_number_of_twofaccount() */ public function test_withdraw_returns_success() { - TwoFAccount::factory()->count(3)->create(); - $ids = DB::table('twofaccounts')->pluck('id')->implode(','); - $response = $this->actingAs($this->user, 'api-guard') - ->json('PATCH', '/api/v1/twofaccounts/withdraw?ids=1,2,3' . $ids) + ->json('PATCH', '/api/v1/twofaccounts/withdraw?ids=1,2') ->assertOk() ->assertJsonStructure([ 'message', @@ -1013,8 +1088,9 @@ public function test_withdraw_returns_success() */ public function test_withdraw_too_many_ids_returns_bad_request() { - TwoFAccount::factory()->count(102)->create(); - $ids = DB::table('twofaccounts')->pluck('id')->implode(','); + TwoFAccount::factory()->count(102)->for($this->user)->create(); + + $ids = DB::table('twofaccounts')->where('user_id', $this->user->id)->pluck('id')->implode(','); $response = $this->actingAs($this->user, 'api-guard') ->json('PATCH', '/api/v1/twofaccounts/withdraw?ids=' . $ids) @@ -1030,10 +1106,8 @@ public function test_withdraw_too_many_ids_returns_bad_request() */ public function test_destroy_twofaccount_returns_success() { - $twofaccount = TwoFAccount::factory()->create(); - $response = $this->actingAs($this->user, 'api-guard') - ->json('DELETE', '/api/v1/twofaccounts/' . $twofaccount->id) + ->json('DELETE', '/api/v1/twofaccounts/' . $this->twofaccountA->id) ->assertNoContent(); } @@ -1042,23 +1116,35 @@ public function test_destroy_twofaccount_returns_success() */ public function test_destroy_missing_twofaccount_returns_not_found() { - $twofaccount = TwoFAccount::factory()->create(); - $response = $this->actingAs($this->user, 'api-guard') ->json('DELETE', '/api/v1/twofaccounts/1000') ->assertNotFound(); } + /** + * @test + */ + public function test_destroy_twofaccount_of_another_user_is_forbidden() + { + $response = $this->actingAs($this->user, 'api-guard') + ->json('DELETE', '/api/v1/twofaccounts/' . $this->twofaccountC->id) + ->assertForbidden() + ->assertJsonStructure([ + 'message', + ]); + } + /** * @test */ public function test_batch_destroy_twofaccount_returns_success() { - TwoFAccount::factory()->count(3)->create(); - $ids = DB::table('twofaccounts')->pluck('id')->implode(','); + TwoFAccount::factory()->count(3)->for($this->user)->create(); + + $ids = DB::table('twofaccounts')->where('user_id', $this->user->id)->pluck('id')->implode(','); $response = $this->actingAs($this->user, 'api-guard') - ->json('DELETE', '/api/v1/twofaccounts?ids=' . $ids) + ->json('DELETE', '/api/v1/twofaccounts?ids=' . $this->twofaccountA->id . ',' . $this->twofaccountB->id) ->assertNoContent(); } @@ -1067,8 +1153,9 @@ public function test_batch_destroy_twofaccount_returns_success() */ public function test_batch_destroy_too_many_twofaccounts_returns_bad_request() { - TwoFAccount::factory()->count(102)->create(); - $ids = DB::table('twofaccounts')->pluck('id')->implode(','); + TwoFAccount::factory()->count(102)->for($this->user)->create(); + + $ids = DB::table('twofaccounts')->where('user_id', $this->user->id)->pluck('id')->implode(','); $response = $this->actingAs($this->user, 'api-guard') ->json('DELETE', '/api/v1/twofaccounts?ids=' . $ids) @@ -1078,4 +1165,24 @@ public function test_batch_destroy_too_many_twofaccounts_returns_bad_request() 'reason', ]); } + + /** + * @test + */ + public function test_batch_destroy_twofaccount_of_another_user_is_forbidden() + { + TwoFAccount::factory()->count(2)->for($this->anotherUser)->create(); + + $ids = DB::table('twofaccounts') + ->where('user_id', $this->anotherUser->id) + ->pluck('id') + ->implode(','); + + $response = $this->actingAs($this->user, 'api-guard') + ->json('DELETE', '/api/v1/twofaccounts?ids=' . $ids) + ->assertForbidden() + ->assertJsonStructure([ + 'message', + ]); + } } diff --git a/tests/Feature/Services/TwoFAccountServiceTest.php b/tests/Feature/Services/TwoFAccountServiceTest.php index 4240b80a..77c400a5 100644 --- a/tests/Feature/Services/TwoFAccountServiceTest.php +++ b/tests/Feature/Services/TwoFAccountServiceTest.php @@ -5,6 +5,7 @@ use App\Facades\TwoFAccounts; use App\Models\Group; use App\Models\TwoFAccount; +use App\Models\User; use Tests\Data\MigrationTestData; use Tests\Data\OtpTestData; use Tests\FeatureTestCase; @@ -16,19 +17,19 @@ class TwoFAccountServiceTest extends FeatureTestCase { /** - * App\Models\TwoFAccount $customTotpTwofaccount + * @var \App\Models\User|\Illuminate\Contracts\Auth\Authenticatable */ - protected $customTotpTwofaccount; + protected $user; /** - * App\Models\Group $group + * @var \App\Models\TwoFAccount */ - protected $group; + protected $customTotpTwofaccount, $customHotpTwofaccount; /** - * App\Models\TwoFAccount $customTotpTwofaccount + * @var \App\Models\Group */ - protected $customHotpTwofaccount; + protected $userGroupA, $userGroupB; /** * @test @@ -36,36 +37,36 @@ class TwoFAccountServiceTest extends FeatureTestCase public function setUp() : void { parent::setUp(); + + $this->user = User::factory()->create(); + $this->userGroupA = Group::factory()->for($this->user)->create(); + $this->userGroupB = Group::factory()->for($this->user)->create(); - $this->customTotpTwofaccount = new TwoFAccount; - $this->customTotpTwofaccount->legacy_uri = OtpTestData::TOTP_FULL_CUSTOM_URI; - $this->customTotpTwofaccount->service = OtpTestData::SERVICE; - $this->customTotpTwofaccount->account = OtpTestData::ACCOUNT; - $this->customTotpTwofaccount->icon = OtpTestData::ICON_PNG; - $this->customTotpTwofaccount->otp_type = 'totp'; - $this->customTotpTwofaccount->secret = OtpTestData::SECRET; - $this->customTotpTwofaccount->digits = OtpTestData::DIGITS_CUSTOM; - $this->customTotpTwofaccount->algorithm = OtpTestData::ALGORITHM_CUSTOM; - $this->customTotpTwofaccount->period = OtpTestData::PERIOD_CUSTOM; - $this->customTotpTwofaccount->counter = null; - $this->customTotpTwofaccount->save(); + $this->customTotpTwofaccount = TwoFAccount::factory()->for($this->user)->create([ + 'legacy_uri' => OtpTestData::TOTP_FULL_CUSTOM_URI, + 'service' => OtpTestData::SERVICE, + 'account' => OtpTestData::ACCOUNT, + 'icon' => OtpTestData::ICON_PNG, + 'otp_type' => 'totp', + 'secret' => OtpTestData::SECRET, + 'digits' => OtpTestData::DIGITS_CUSTOM, + 'algorithm' => OtpTestData::ALGORITHM_CUSTOM, + 'period' => OtpTestData::PERIOD_CUSTOM, + 'counter' => null, + ]); - $this->customHotpTwofaccount = new TwoFAccount; - $this->customHotpTwofaccount->legacy_uri = OtpTestData::HOTP_FULL_CUSTOM_URI; - $this->customHotpTwofaccount->service = OtpTestData::SERVICE; - $this->customHotpTwofaccount->account = OtpTestData::ACCOUNT; - $this->customHotpTwofaccount->icon = OtpTestData::ICON_PNG; - $this->customHotpTwofaccount->otp_type = 'hotp'; - $this->customHotpTwofaccount->secret = OtpTestData::SECRET; - $this->customHotpTwofaccount->digits = OtpTestData::DIGITS_CUSTOM; - $this->customHotpTwofaccount->algorithm = OtpTestData::ALGORITHM_CUSTOM; - $this->customHotpTwofaccount->period = null; - $this->customHotpTwofaccount->counter = OtpTestData::COUNTER_CUSTOM; - $this->customHotpTwofaccount->save(); - - $this->group = new Group; - $this->group->name = 'MyGroup'; - $this->group->save(); + $this->customHotpTwofaccount = TwoFAccount::factory()->for($this->user)->create([ + 'legacy_uri' => OtpTestData::HOTP_FULL_CUSTOM_URI, + 'service' => OtpTestData::SERVICE, + 'account' => OtpTestData::ACCOUNT, + 'icon' => OtpTestData::ICON_PNG, + 'otp_type' => 'hotp', + 'secret' => OtpTestData::SECRET, + 'digits' => OtpTestData::DIGITS_CUSTOM, + 'algorithm' => OtpTestData::ALGORITHM_CUSTOM, + 'period' => null, + 'counter' => OtpTestData::COUNTER_CUSTOM, + ]); } /** @@ -74,7 +75,17 @@ public function setUp() : void public function test_withdraw_comma_separated_ids_deletes_relation() { $twofaccounts = collect([$this->customHotpTwofaccount, $this->customTotpTwofaccount]); - $this->group->twofaccounts()->saveMany($twofaccounts); + $this->userGroupA->twofaccounts()->saveMany($twofaccounts); + + $this->assertDatabaseHas('twofaccounts', [ + 'id' => $this->customHotpTwofaccount->id, + 'group_id' => $this->userGroupA->id, + ]); + + $this->assertDatabaseHas('twofaccounts', [ + 'id' => $this->customTotpTwofaccount->id, + 'group_id' => $this->userGroupA->id, + ]); TwoFAccounts::withdraw($this->customHotpTwofaccount->id . ',' . $this->customTotpTwofaccount->id); @@ -92,11 +103,20 @@ public function test_withdraw_comma_separated_ids_deletes_relation() /** * @test */ - public function test_withdraw_array_of_model_ids_deletes_relation() + public function test_withdraw_array_of_ids_deletes_relation() { $twofaccounts = collect([$this->customHotpTwofaccount, $this->customTotpTwofaccount]); - $this->group->twofaccounts()->saveMany($twofaccounts); + $this->userGroupA->twofaccounts()->saveMany($twofaccounts); + $this->assertDatabaseHas('twofaccounts', [ + 'id' => $this->customHotpTwofaccount->id, + 'group_id' => $this->userGroupA->id, + ]); + + $this->assertDatabaseHas('twofaccounts', [ + 'id' => $this->customTotpTwofaccount->id, + 'group_id' => $this->userGroupA->id, + ]); TwoFAccounts::withdraw([$this->customHotpTwofaccount->id, $this->customTotpTwofaccount->id]); $this->assertDatabaseHas('twofaccounts', [ @@ -116,7 +136,12 @@ public function test_withdraw_array_of_model_ids_deletes_relation() public function test_withdraw_single_id_deletes_relation() { $twofaccounts = collect([$this->customHotpTwofaccount, $this->customTotpTwofaccount]); - $this->group->twofaccounts()->saveMany($twofaccounts); + $this->userGroupA->twofaccounts()->saveMany($twofaccounts); + + $this->assertDatabaseHas('twofaccounts', [ + 'id' => $this->customTotpTwofaccount->id, + 'group_id' => $this->userGroupA->id, + ]); TwoFAccounts::withdraw($this->customTotpTwofaccount->id); @@ -137,50 +162,10 @@ public function test_withdraw_missing_ids_returns_void() /** * @test */ - public function test_delete_comma_separated_ids() + public function test_migrate_from_gauth_returns_correct_accounts() { - TwoFAccounts::delete($this->customHotpTwofaccount->id . ',' . $this->customTotpTwofaccount->id); + $this->actingAs($this->user); - $this->assertDatabaseMissing('twofaccounts', [ - 'id' => $this->customTotpTwofaccount->id, - ]); - $this->assertDatabaseMissing('twofaccounts', [ - 'id' => $this->customHotpTwofaccount->id, - ]); - } - - /** - * @test - */ - public function test_delete_array_of_ids() - { - TwoFAccounts::delete([$this->customTotpTwofaccount->id, $this->customHotpTwofaccount->id]); - - $this->assertDatabaseMissing('twofaccounts', [ - 'id' => $this->customTotpTwofaccount->id, - ]); - $this->assertDatabaseMissing('twofaccounts', [ - 'id' => $this->customHotpTwofaccount->id, - ]); - } - - /** - * @test - */ - public function test_delete_single_id() - { - TwoFAccounts::delete($this->customTotpTwofaccount->id); - - $this->assertDatabaseMissing('twofaccounts', [ - 'id' => $this->customTotpTwofaccount->id, - ]); - } - - /** - * @test - */ - public function test_convert_migration_from_gauth_returns_correct_accounts() - { $twofaccounts = TwoFAccounts::migrate(MigrationTestData::GOOGLE_AUTH_MIGRATION_URI); $this->assertCount(2, $twofaccounts); @@ -207,8 +192,10 @@ public function test_convert_migration_from_gauth_returns_correct_accounts() /** * @test */ - public function test_convert_migration_from_gauth_returns_flagged_duplicates() + public function test_migrate_from_gauth_returns_flagged_duplicates() { + $this->actingAs($this->user); + $parameters = [ 'service' => OtpTestData::SERVICE, 'account' => OtpTestData::ACCOUNT, @@ -237,9 +224,75 @@ public function test_convert_migration_from_gauth_returns_flagged_duplicates() /** * @test */ - public function test_convert_invalid_migration_from_gauth_returns_InvalidMigrationData_exception() + public function test_migrate_invalid_migration_from_gauth_returns_InvalidMigrationData_exception() { $this->expectException(\App\Exceptions\InvalidMigrationDataException::class); $twofaccounts = TwoFAccounts::migrate(MigrationTestData::GOOGLE_AUTH_MIGRATION_URI_WITH_INVALID_DATA); } + + /** + * @test + */ + public function test_delete_comma_separated_ids() + { + $twofaccounts = TwoFAccount::factory()->count(2)->for($this->user)->create(); + + $this->assertDatabaseHas('twofaccounts', [ + 'id' => $twofaccounts[0]->id, + ]); + $this->assertDatabaseHas('twofaccounts', [ + 'id' => $twofaccounts[1]->id, + ]); + + TwoFAccounts::delete($twofaccounts[0]->id . ',' . $twofaccounts[1]->id); + + $this->assertDatabaseMissing('twofaccounts', [ + 'id' => $twofaccounts[0]->id, + ]); + $this->assertDatabaseMissing('twofaccounts', [ + 'id' => $twofaccounts[1]->id, + ]); + } + + /** + * @test + */ + public function test_delete_array_of_ids() + { + $twofaccounts = TwoFAccount::factory()->count(2)->for($this->user)->create(); + + $this->assertDatabaseHas('twofaccounts', [ + 'id' => $twofaccounts[0]->id, + ]); + $this->assertDatabaseHas('twofaccounts', [ + 'id' => $twofaccounts[1]->id, + ]); + + TwoFAccounts::delete([$twofaccounts[0]->id, $twofaccounts[1]->id]); + + $this->assertDatabaseMissing('twofaccounts', [ + 'id' => $twofaccounts[0]->id, + ]); + $this->assertDatabaseMissing('twofaccounts', [ + 'id' => $twofaccounts[1]->id, + ]); + } + + /** + * @test + */ + public function test_delete_single_id() + { + $twofaccount = TwoFAccount::factory()->for($this->user)->create(); + + $this->assertDatabaseHas('twofaccounts', [ + 'id' => $twofaccount->id, + ]); + + TwoFAccounts::delete($twofaccount->id); + + $this->assertDatabaseMissing('twofaccounts', [ + 'id' => $twofaccount->id, + ]); + } }