diff --git a/src/app/Auth/PassportClient.php b/src/app/Auth/PassportClient.php --- a/src/app/Auth/PassportClient.php +++ b/src/app/Auth/PassportClient.php @@ -16,9 +16,7 @@ /** * The allowed scopes for tokens instantiated by this client - * - * @return Array - * */ + */ public function getAllowedScopes(): array { if ($this->allowed_scopes) { diff --git a/src/app/Console/Kernel.php b/src/app/Console/Kernel.php --- a/src/app/Console/Kernel.php +++ b/src/app/Console/Kernel.php @@ -37,6 +37,9 @@ // https://laravel.com/docs/10.x/upgrade#redis-cache-tags $schedule->command('cache:prune-stale-tags')->hourly(); + + // This removes passport expired/revoked tokens and auth codes from the database + $schedule->command('passport:purge')->dailyAt('06:30'); } /** diff --git a/src/app/Http/Controllers/API/AuthController.php b/src/app/Http/Controllers/API/AuthController.php --- a/src/app/Http/Controllers/API/AuthController.php +++ b/src/app/Http/Controllers/API/AuthController.php @@ -92,44 +92,50 @@ /** * Approval request for the oauth authorization endpoint * - * * * The user is authenticated via the regular login page * * We assume implicit consent in the Authorization page * * Ultimately we return an authorization code to the caller via the redirect_uri * * The implementation is based on Laravel\Passport\Http\Controllers\AuthorizationController * - * @param \Illuminate\Http\Request $request The API request. + * @param ServerRequestInterface $psrRequest PSR request + * @param \Illuminate\Http\Request $request The API request + * @param AuthorizationServer $server Authorization server * * @return \Illuminate\Http\JsonResponse */ - public function oauthApprove( - ServerRequestInterface $psrRequest, - Request $request, - AuthorizationServer $server - ) { - if ($request->response_type != "code") { - return response()->json(['status' => 'error', 'errors' => ["invalid response_type"]], 422); - } - - $user = Auth::guard()->user(); - if (!$user) { - \Log::warning("User not found"); - return response()->json(['status' => 'error', 'message' => self::trans('auth.failed')], 401); + public function oauthApprove(ServerRequestInterface $psrRequest, Request $request, AuthorizationServer $server) + { + if ($request->response_type != 'code') { + return self::errorResponse(422, self::trans('validation.invalidvalueof', ['attribute' => 'response_type'])); } try { + // league/oauth2-server/src/Grant/ code expects GET parameters, but we're using POST here + $psrRequest = $psrRequest->withQueryParams($request->input()); + $authRequest = $server->validateAuthorizationRequest($psrRequest); + + $user = Auth::guard()->user(); + + // TODO I'm not sure if we should still execute this to deny the request + $authRequest->setUser(new \Laravel\Passport\Bridge\User($user->getAuthIdentifier())); + $authRequest->setAuthorizationApproved(true); + + // This will generate a 302 redirect to the redirect_uri with the generated authorization code + $response = $server->completeAuthorizationRequest($authRequest, new Psr7Response()); + } catch (\League\OAuth2\Server\Exception\OAuthServerException $e) { + return self::errorResponse($e->getHttpStatusCode(), $e->getMessage()); } catch (\Exception $e) { - return response()->json(['status' => 'error', 'message' => "Failed to validate"], 401); + return self::errorResponse(422, self::trans('auth.error.invalidrequest')); } - //TODO I'm not sure if we should still execute this to deny the request - $authRequest->setUser(new \Laravel\Passport\Bridge\User($user->getAuthIdentifier())); - $authRequest->setAuthorizationApproved(true); + // TODO: Support various response_mode modes - # This will generate a 302 redirect to the redirect_uri with the generated authorization code - return $server->completeAuthorizationRequest($authRequest, new Psr7Response()); + return response()->json([ + 'status' => 'success', + 'redirectUrl' => $response->getHeader('Location')[0], + ]); } /** diff --git a/src/app/Providers/AppServiceProvider.php b/src/app/Providers/AppServiceProvider.php --- a/src/app/Providers/AppServiceProvider.php +++ b/src/app/Providers/AppServiceProvider.php @@ -16,6 +16,8 @@ */ public function register(): void { + // This must be here, not in PassportServiceProvider + Passport::ignoreRoutes(); } /** diff --git a/src/app/Providers/PassportServiceProvider.php b/src/app/Providers/PassportServiceProvider.php --- a/src/app/Providers/PassportServiceProvider.php +++ b/src/app/Providers/PassportServiceProvider.php @@ -17,8 +17,8 @@ */ public function boot() { + // Passport::ignoreRoutes() is in the AppServiceProvider Passport::enablePasswordGrant(); - Passport::ignoreRoutes(); Passport::tokensCan([ 'api' => 'Access API', diff --git a/src/composer.json b/src/composer.json --- a/src/composer.json +++ b/src/composer.json @@ -39,7 +39,7 @@ }, "require-dev": { "code-lts/doctum": "^5.5.1", - "laravel/dusk": "~7.9.1", + "laravel/dusk": "~8.2.2", "mockery/mockery": "^1.5", "larastan/larastan": "^2.0", "phpstan/phpstan": "^1.4", diff --git a/src/resources/js/user/routes.js b/src/resources/js/user/routes.js --- a/src/resources/js/user/routes.js +++ b/src/resources/js/user/routes.js @@ -87,7 +87,7 @@ meta: { requiresAuth: true, perm: 'files' } }, { - path: '/authorize', + path: '/oauth/authorize', name: 'authorize', component: AuthorizeComponent, meta: { requiresAuth: true } diff --git a/src/resources/lang/en/auth.php b/src/resources/lang/en/auth.php --- a/src/resources/lang/en/auth.php +++ b/src/resources/lang/en/auth.php @@ -19,6 +19,7 @@ 'logoutsuccess' => 'Successfully logged out.', 'error.password' => "Invalid password", + 'error.invalidrequest' => "Invalid authorization request.", 'error.geolocation' => "Country code mismatch", 'error.nofound' => "User not found", 'error.2fa' => "Second factor failure", diff --git a/src/resources/lang/en/validation.php b/src/resources/lang/en/validation.php --- a/src/resources/lang/en/validation.php +++ b/src/resources/lang/en/validation.php @@ -130,6 +130,7 @@ 'url' => 'The :attribute must be a valid URL.', 'uuid' => 'The :attribute must be a valid UUID.', + 'invalidvalueof' => 'Invalid value of request property: :attribute.', '2fareq' => 'Second factor code is required.', '2fainvalid' => 'Second factor code is invalid.', 'emailinvalid' => 'The specified email address is invalid.', diff --git a/src/tests/Browser/AuthorizeTest.php b/src/tests/Browser/AuthorizeTest.php new file mode 100644 --- /dev/null +++ b/src/tests/Browser/AuthorizeTest.php @@ -0,0 +1,81 @@ +client = \App\Auth\PassportClient::firstOrCreate( + ['id' => 'test'], + [ + 'user_id' => null, + 'name' => 'Test', + 'secret' => '123', + 'provider' => 'users', + 'redirect' => 'https://kolab.org', + 'personal_access_client' => 0, + 'password_client' => 0, + 'revoked' => false, + 'allowed_scopes' => ['oauth'], + ] + ); + } + + /** + * {@inheritDoc} + */ + public function tearDown(): void + { + $this->client->delete(); + + parent::tearDown(); + } + + /** + * Test /oauth/authorize page + */ + public function testAuthorize(): void + { + $url = '/oauth/authorize?' . http_build_query([ + 'client_id' => $this->client->id, + 'response_type' => 'code', + 'scope' => 'oauth', + 'state' => 'state', + 'nonce' => 'nonce', + ]); + + $this->browse(function (Browser $browser) use ($url) { + $redirect_check = "window.location.host == 'kolab.org'" + . " && window.location.search.match(/^\?code=[a-f0-9]+&state=state/)"; + + // Unauthenticated user + $browser->visit($url) + ->on(new Home()) + ->submitLogon('john@kolab.org', 'simple123') + ->waitUntil($redirect_check); + + // Authenticated user + $browser->visit($url) + ->waitUntil($redirect_check); + + // Error handling (invalid response_type) + $browser->visit('oauth/authorize?response_type=invalid') + ->assertErrorPage(422) + ->assertToast(Toast::TYPE_ERROR, 'Invalid value of request property: response_type.'); + }); + } +} diff --git a/src/tests/Browser/Components/Error.php b/src/tests/Browser/Components/Error.php --- a/src/tests/Browser/Components/Error.php +++ b/src/tests/Browser/Components/Error.php @@ -16,6 +16,7 @@ 403 => "Access denied", 404 => "Not found", 405 => "Method not allowed", + 422 => "Unprocessable Content", 500 => "Internal server error", ]; diff --git a/src/tests/Feature/Controller/AuthTest.php b/src/tests/Feature/Controller/AuthTest.php --- a/src/tests/Feature/Controller/AuthTest.php +++ b/src/tests/Feature/Controller/AuthTest.php @@ -326,4 +326,144 @@ $response = $this->withHeaders(['Authorization' => 'Bearer ' . $new_token])->get("api/auth/info"); $response->assertStatus(200); } + + /** + * Test OAuth2 Authorization Code Flow + */ + public function testOAuthAuthorizationCodeFlow(): void + { + $user = $this->getTestUser('john@kolab.org'); + + // Request unauthenticated, testing that it requires auth + $response = $this->post("api/oauth/approve"); + $response->assertStatus(401); + + // Request authenticated, invalid POST data + $post = ['response_type' => 'unknown']; + $response = $this->actingAs($user)->post("api/oauth/approve", $post); + $response->assertStatus(422); + + $json = $response->json(); + + $this->assertSame('error', $json['status']); + $this->assertSame('Invalid value of request property: response_type.', $json['message']); + + // Request authenticated, invalid POST data + $post = [ + 'client_id' => 'unknown', + 'response_type' => 'code', + 'scope' => 'oauth', // space-separated + 'state' => 'state', // optional + 'nonce' => 'nonce', // optional + ]; + $response = $this->actingAs($user)->post("api/oauth/approve", $post); + $response->assertStatus(422); + + $json = $response->json(); + + $this->assertSame('error', $json['status']); + $this->assertSame('Invalid authorization request.', $json['message']); + + $client = \App\Auth\PassportClient::find(\config('auth.synapse.client_id')); + $post['client_id'] = $client->id; + + // Request authenticated, invalid scope + $post['scope'] = 'unknown'; + $response = $this->actingAs($user)->post("api/oauth/approve", $post); + $response->assertStatus(422); + + $json = $response->json(); + + $this->assertSame('error', $json['status']); + $this->assertSame('Invalid authorization request.', $json['message']); + + // Request authenticated, valid POST data + $post['scope'] = implode(' ', $client->getAllowedScopes()); + + $response = $this->actingAs($user)->post("api/oauth/approve", $post); + $response->assertStatus(200); + + $json = $response->json(); + + $url = $json['redirectUrl']; + parse_str(parse_url($url, \PHP_URL_QUERY), $params); + + $this->assertTrue(str_starts_with($url, $client->redirect . '?')); + $this->assertCount(2, $params); + $this->assertSame('state', $params['state']); + $this->assertMatchesRegularExpression('/^[a-f0-9]{50,}$/', $params['code']); + $this->assertSame('success', $json['status']); + + // Note: We do not validate the code trusting Passport to do the right thing. Should we not? + + // Token endpoint tests + + // Valid authorization code, but invalid secret + $post = [ + 'grant_type' => 'authorization_code', + 'client_id' => \config('auth.synapse.client_id'), + 'client_secret' => 'invalid', + // 'redirect_uri' => '', + 'code' => $params['code'], + ]; + + // Note: This is a 'web' route, not 'api' + $response = $this->post("/oauth/token", $post); + $response->assertStatus(401); + + $json = $response->json(); + + $this->assertSame('invalid_client', $json['error']); + $this->assertTrue(!empty($json['error_description'])); + + // Valid authorization code + $post['client_secret'] = \config('auth.synapse.client_secret'); + $response = $this->post("/oauth/token", $post); + $response->assertStatus(200); + + $params = $response->json(); + + $this->assertSame('Bearer', $params['token_type']); + $this->assertTrue(!empty($params['access_token'])); + $this->assertTrue(!empty($params['refresh_token'])); + $this->assertTrue(!empty($params['expires_in'])); + + // TODO: Validate token scope and that it works or not on specific API endpoints + + // Invalid authorization code + // Note: The code is being revoked on use, so we expect it does not work anymore + $response = $this->post("/oauth/token", $post); + $response->assertStatus(400); + + $json = $response->json(); + + $this->assertSame('invalid_request', $json['error']); + $this->assertTrue(!empty($json['error_description'])); + + // Token refresh + unset($post['code']); + $post['grant_type'] = 'refresh_token'; + $post['refresh_token'] = $params['refresh_token']; + + $response = $this->post("/oauth/token", $post); + $response->assertStatus(200); + + $json = $response->json(); + + $this->assertSame('Bearer', $json['token_type']); + $this->assertTrue(!empty($json['access_token'])); + $this->assertTrue(!empty($json['refresh_token'])); + $this->assertTrue(!empty($json['expires_in'])); + $this->assertNotEquals($json['access_token'], $params['access_token']); + $this->assertNotEquals($json['refresh_token'], $params['refresh_token']); + } + + /** + * Test to make sure Passport routes are disabled + */ + public function testPassportDisabledRoutes(): void + { + $this->post("/oauth/authorize", [])->assertStatus(405); + $this->post("/oauth/token/refresh", [])->assertStatus(405); + } }