From b28fe4d479e2b647f4b4d08e89449b6485bafe8f Mon Sep 17 00:00:00 2001 From: Rodrigo Pizarro <11396307+BaaltRodrigo@users.noreply.github.com> Date: Tue, 31 Oct 2023 12:32:21 -0300 Subject: [PATCH] fix: public routes cannot get guests users (#22) --- app/Exceptions/Handler.php | 4 +- app/Http/Middleware/FirebaseAuth.php | 21 +++++-- app/Policies/CalendarPolicy.php | 15 +++-- routes/api.php | 4 +- tests/Feature/CalendarPrivacyTest.php | 88 +++++++++++++++++++++++++++ tests/Feature/CalendarTest.php | 12 ++-- 6 files changed, 126 insertions(+), 18 deletions(-) create mode 100644 tests/Feature/CalendarPrivacyTest.php diff --git a/app/Exceptions/Handler.php b/app/Exceptions/Handler.php index 04d4005..6add857 100644 --- a/app/Exceptions/Handler.php +++ b/app/Exceptions/Handler.php @@ -34,7 +34,9 @@ public function register(): void * Firebase error handling */ $this->renderable(function (FailedToVerifyToken $e, Request $request) { - return response()->json(['status' => 'error','message' => 'Invalid token'], Response::HTTP_UNAUTHORIZED); + return response()->json( + ['status' => 'error', 'message' => 'Invalid or malformed token'], + Response::HTTP_UNAUTHORIZED); }); } } diff --git a/app/Http/Middleware/FirebaseAuth.php b/app/Http/Middleware/FirebaseAuth.php index b3ab3d6..f6099c3 100644 --- a/app/Http/Middleware/FirebaseAuth.php +++ b/app/Http/Middleware/FirebaseAuth.php @@ -21,23 +21,32 @@ public function __construct() /** * Handle an incoming request. + * This middleware has * + * @param \Illuminate\Http\Request $request + * @param string $mode strict|optional If the auth methods is optional, the user is not required to be authenticated * @param \Closure(\Illuminate\Http\Request): (\Symfony\Component\HttpFoundation\Response) $next */ - public function handle(Request $request, Closure $next): Response + public function handle(Request $request, Closure $next, ?string $mode = 'strict'): Response { // Check for the token in the header $token = $request->bearerToken(); + // If the auth is optional, the user is not required to be authenticated + $is_optional = $mode === 'optional'; + if (!$token) { - return response()->json(['status' => 'error','message' => 'Token not provided'], Response::HTTP_UNAUTHORIZED); + return $is_optional + ? $next($request) + : response()->json(['status' => 'error','message' => 'Token not provided'], Response::HTTP_UNAUTHORIZED); } - - // Verify the token + + // From here, the token is present and we can proceed to verify it + // Verify the token as the route is without optional route $verifiedIdToken = $this->auth->verifyIdToken( $token, - $checkIfRevoked = true, - $leewayInSeconds = 3600 // Handle 1 hour of delay from the client + false, // Check if the token is revoked + 3600 // Handle 1 hour of delay from the client ); // Set the user in the request and proceed diff --git a/app/Policies/CalendarPolicy.php b/app/Policies/CalendarPolicy.php index f5481fa..23c8381 100644 --- a/app/Policies/CalendarPolicy.php +++ b/app/Policies/CalendarPolicy.php @@ -19,12 +19,19 @@ public function __construct() // } - public function view(User $user, Calendar $calendar) + public function view(?User $user, Calendar $calendar) { - // Check if user owns calendar - $is_owner = $user->id === $calendar->user_id; + // Early exit to public calendar with or without user + if ($calendar->is_public) { + return Response::allow(); + } + + // If user is null, then it is a guest + if ($user === null) { + return Response::deny('You are not allowed to view this calendar'); + } - return $is_owner || $calendar->is_public + return $user->id === $calendar->user_id ? Response::allow() : Response::deny('You are not allowed to view this calendar'); } diff --git a/routes/api.php b/routes/api.php index 37676b0..6f1ca20 100644 --- a/routes/api.php +++ b/routes/api.php @@ -65,7 +65,9 @@ // Show for calendars needs to be outside middleware because there // are public calendars that don't need to be authenticated Route::prefix('/calendars/{calendar}')->group(function () { - Route::get('/', [CalendarController::class, 'show'])->name('calendars.show'); + Route::get('/', [CalendarController::class, 'show']) + ->middleware('auth.firebase:optional') + ->name('calendars.show'); Route::get('/sections', [CalendarController::class, 'calendarSections'])->name('calendars.sections'); Route::match(['put', 'patch'], '/sections', [CalendarController::class, 'updateSections']) ->middleware('auth.firebase') diff --git a/tests/Feature/CalendarPrivacyTest.php b/tests/Feature/CalendarPrivacyTest.php new file mode 100644 index 0000000..45b8d53 --- /dev/null +++ b/tests/Feature/CalendarPrivacyTest.php @@ -0,0 +1,88 @@ +createUser(); + $calendar = Calendar::factory()->create([ + 'user_id' => $user->id, + 'is_public' => true + ]); + // Does not use any user to fetch the calendar + + $response = $this->getJson(route('calendars.show', $calendar->uuid)); + + $response->assertOk(); + } + + public function test_it_allows_other_users_to_view_public_calendars(): void + { + $user1 = User::create(['id' => 'unused-user']); + $calendar = Calendar::factory()->create([ + 'user_id' => $user1->id, + 'is_public' => true + ]); + $firebaseUser = $this->createUser(); + $this->actingAsFirebaseUser($firebaseUser); + + $response = $this->getJson(route('calendars.show', $calendar->uuid)); + + $response->assertOk(); + } + + public function test_it_allows_owners_to_view_own_calendars(): void + { + $user = $this->createUser(); + $calendar = Calendar::factory()->create([ + 'user_id' => $user->id, + 'is_public' => false + ]); + $this->actingAsFirebaseUser($user); + + $response = $this->getJson(route('calendars.show', $calendar->uuid)); + + $response->assertOk(); + } + + public function test_it_denies_guests_to_see_private_calendars(): void + { + $user = $this->createUser(); + $calendar = Calendar::factory()->create([ + 'user_id' => $user->id, + 'is_public' => false + ]); + // Does not use any user to fetch the calendar + + $response = $this->getJson(route('calendars.show', $calendar->uuid)); + + $response->assertForbidden(); + } + + public function test_it_denies_other_users_to_see_private_calendars(): void + { + $user = User::create(['id' => 'owner-user']); + $calendar = Calendar::factory()->create([ + 'user_id' => $user->id, + 'is_public' => false + ]); + $firebaseUser = $this->createUser(); + $this->actingAsFirebaseUser($firebaseUser); + + $response = $this->getJson(route('calendars.show', $calendar->uuid)); + + $response->assertForbidden(); + } + +} \ No newline at end of file diff --git a/tests/Feature/CalendarTest.php b/tests/Feature/CalendarTest.php index 731340b..7d83440 100644 --- a/tests/Feature/CalendarTest.php +++ b/tests/Feature/CalendarTest.php @@ -183,16 +183,16 @@ public function test_it_allows_owners_to_delete_calendar_section(): void ]); } - public function test_it_denies_access_to_private_calendars(): void + public function test_it_denies_when_guests_create_calendars(): void { - $calendar = Calendar::factory()->create([ - 'user_id' => null, - 'is_public' => false + $calendar = Calendar::factory()->make([ + 'user_id' => null ]); - $response = $this->getJson(route('calendars.show', $calendar->uuid)); + $response = $this->postJson(route('calendars.store'), $calendar->toArray()); - $response->assertStatus(Response::HTTP_FORBIDDEN); + $response->assertStatus(Response::HTTP_UNAUTHORIZED); + $this->assertDatabaseMissing('calendars', ['uuid' => $calendar->uuid]); } public function test_it_denies_delete_non_owned_calendars(): void