Page MenuHomePhorge
Files F117900929

D4866.1775380686.diff
No OneTemporary
Actions

Authored By
Unknown
Size
15 KB
Referenced Files
None
Subscribers
None

D4866.1775380686.diff
View Options

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,52 @@
/**
* 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) {
+ // Note: We don't want 401 or 400 codes here, use 422 which is used in our API
+ $code = $e->getHttpStatusCode();
+ return self::errorResponse($code < 500 ? 422 : 500, $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 @@
+<?php
+
+namespace Tests\Browser;
+
+use Tests\Browser;
+use Tests\Browser\Components\Toast;
+use Tests\Browser\Pages\Home;
+use Tests\TestCaseDusk;
+
+class AuthorizeTest extends TestCaseDusk
+{
+ private $client;
+
+ /**
+ * {@inheritDoc}
+ */
+ public function setUp(): void
+ {
+ parent::setUp();
+
+ // Create a client for tests
+ $this->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('Client authentication failed', $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('The requested scope is invalid, unknown, or malformed', $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);
+ }
}

File Metadata

Mime Type
text/plain
Expires
Sun, Apr 5, 9:18 AM (15 h, 55 m)
Storage Engine
blob
Storage Format
Raw Data
Storage Handle
18832954
Default Alt Text
D4866.1775380686.diff (15 KB)

Event Timeline