diff --git a/README.md b/README.md index a8240c9..8740423 100644 --- a/README.md +++ b/README.md @@ -12,6 +12,15 @@ cd plugin-oauth2 composer install ``` +# Updating from version 3.0.x + +Some settings are now mandatory, clients that do not follow them are refused (the reason is written in Galette logs): +- each client must declare its `redirect_uri`: the callback URL of the client application, exactly as the application sends it. Use a list if the application uses several URLs. +- each client must have its own `password`; the `global` password is no longer used, and the `abc123` example password is refused. +- only the "authorization code" and "refresh token" grants are available. +- scopes from the `scopes` entry are checked by default on the authorization screen; they are no longer given if the member unchecks them. +- members already logged in to the authorization screen will have to log in again. + # Updating to version 3.0.0 Before updating to version 3.0.0, please take care of the following: @@ -43,19 +52,24 @@ Rename `config/config.yml.dist` to `config/config.yml` and edit according to you ``` global: - password: abc123 + title: 'Galette' galette_flarum: + password: 'a-long-random-secret' title: 'Forum Flarum' + redirect_uri: 'http://192.168.1.99/flarum/public/auth/passport' redirect_logout: 'http://192.168.1.99/flarum/public' galette_nc: + password: 'another-long-random-secret' title: 'Nextcloud' + redirect_uri: 'http://192.168.1.99/nextcloud/apps/sociallogin/custom_oauth2/galette' redirect_logout: 'http://192.168.1.99/nextcloud' scopes: - member:groups -galette_xxxxx: ``` +`password` and `redirect_uri` are mandatory for each client. `redirect_uri` can be a list of URLs. + The corresponding Flarum configuration: ![Flarum configuration example](examples/flarum.png) diff --git a/_dependencies.php b/_dependencies.php index cadd1ea..ac891c3 100644 --- a/_dependencies.php +++ b/_dependencies.php @@ -22,7 +22,6 @@ use GaletteOAuth2\Repositories\ClientRepository; use GaletteOAuth2\Repositories\RefreshTokenRepository; use GaletteOAuth2\Repositories\ScopeRepository; -use GaletteOAuth2\Repositories\UserRepository; use GaletteOAuth2\Tools\Config; use League\OAuth2\Server\AuthorizationServer; use League\OAuth2\Server\Grant\AuthCodeGrant; @@ -114,8 +113,7 @@ function (ContainerInterface $container) { new DateInterval('PT10M'), ); - // Enable the password grant on the server - // with a token TTL of 1 hour + // Enable the authorization code grant on the server $server->enableGrantType( $grant, // access tokens will expire after 1 hour @@ -133,27 +131,6 @@ function (ContainerInterface $container) { new DateInterval('PT1H'), ); - //-- - $userRepository = new UserRepository($container); // instance of UserRepositoryInterface - $grant = new \League\OAuth2\Server\Grant\PasswordGrant( - $userRepository, - $refreshTokenRepository, - ); - - $grant->setRefreshTokenTTL(new \DateInterval('P1M')); // refresh tokens will expire after 1 month - - // Enable the password grant on the server - $server->enableGrantType( - $grant, - new \DateInterval('PT1H'), // access tokens will expire after 1 hour - ); - - // Enable the client credentials grant on the server - $server->enableGrantType( - new \League\OAuth2\Server\Grant\ClientCredentialsGrant(), - new \DateInterval('PT1H'), // access tokens will expire after 1 hour - ); - return $server; }, ); diff --git a/config/config.yml.dist b/config/config.yml.dist index 0c31f6b..62b6ecb 100644 --- a/config/config.yml.dist +++ b/config/config.yml.dist @@ -1,14 +1,17 @@ global: title: 'Galette' - password: abc123 encryption_key: 'your-encryption-key-here' # Replace with a secure random string galette_flarum: + password: '' # required: a long random secret, shared with the client application title: 'Forum Flarum' redirect_logout: 'http://192.168.1.99/flarum/public' + redirect_uri: 'http://192.168.1.99/flarum/public/auth/passport' authorize: teamonly galette_nc: + password: '' # required: one secret per client title: 'Le cloud de votre association' redirect_logout: 'http://192.168.1.99/nextcloud' + redirect_uri: 'http://192.168.1.99/nextcloud/apps/sociallogin/custom_oauth2/galette' authorize: teamonly scopes: - member diff --git a/lib/GaletteOAuth2/Controllers/ApiController.php b/lib/GaletteOAuth2/Controllers/ApiController.php index 29aecda..b04a458 100755 --- a/lib/GaletteOAuth2/Controllers/ApiController.php +++ b/lib/GaletteOAuth2/Controllers/ApiController.php @@ -71,8 +71,9 @@ public function user(Request $request, Response $response): Response $this->container, $oauth_user_id, UserHelper::getAuthorization($this->config, $client_id), + //only scopes the user has consented to, stored in the token UserHelper::mergeScopes( - $this->config, + null, $client_id, $rep->getAttribute('oauth_scopes') ), @@ -88,8 +89,6 @@ public function user(Request $request, Response $response): Response return $response->withStatus(401); } - Debug::log('api/user() return data = ' . Debug::printVar($data)); - $response->getBody()->write(json_encode($data)); Debug::log('api/user() exit.'); diff --git a/lib/GaletteOAuth2/Controllers/AuthorizationController.php b/lib/GaletteOAuth2/Controllers/AuthorizationController.php index 16892e7..b9d96b7 100755 --- a/lib/GaletteOAuth2/Controllers/AuthorizationController.php +++ b/lib/GaletteOAuth2/Controllers/AuthorizationController.php @@ -75,36 +75,6 @@ public function authorize(Request $request, Response $response): Response|Respon $queryParams = $request->getQueryParams(); $client_id = $queryParams['client_id']; - //Save redirect_uri (it's not possible with Sessions) - //FIXME [JC]: I really do not like the idea of using a file on disk; - // this may also cause severe issues in case of concurrent logins - if (isset($queryParams['redirect_uri'])) { - $key = $client_id . '.redirect_uri'; - if (!isset($this->session->$client_id)) { - $this->session->$client_id = new \stdClass(); - } - $this->session->$client_id->redirect_uri = $queryParams['redirect_uri']; - $v = $queryParams['redirect_uri']; - - if ($this->config->get($key, '') === '') { - $filename = OAUTH2_PREFIX . '_' . $key . '.txt'; - Debug::log("Auto add redirect_uri to cache $filename: $v"); - - $this->config->set($key, $v); - $stream = fopen(GALETTE_CACHE_DIR . '/' . $filename, 'w+'); - fwrite( - $stream, - $v - ); - fclose($stream); - - Analog::log( - 'Auto add redirect_uri ok.', - Analog::DEBUG - ); - } - } - // Validate the HTTP request and return an AuthorizationRequest object. // The auth request object can be serialized into a user's session $authRequest = $server->validateAuthorizationRequest($request); @@ -147,10 +117,7 @@ public function authorize(Request $request, Response $response): Response|Respon } catch (OAuthServerException $exception) { return $exception->generateHttpResponse($response); } catch (Exception $exception) { - $body = $response->getBody(); - $body->write($exception->getMessage()); - - return $response->withStatus(500)->withBody($body); + return $this->errorResponse($response, $exception); } } @@ -191,7 +158,10 @@ public function doAuthorize(Request $request, Response $response): Response|Resp $req_scopes = []; $srepo = new ScopeRepository(); foreach ($scopes as $scope) { - $req_scopes[] = $srepo->getScopeEntityByIdentifier($scope); + $scope_entity = $srepo->getScopeEntityByIdentifier($scope); + if ($scope_entity !== null) { + $req_scopes[] = $scope_entity; + } } $authRequest->setScopes($req_scopes); } else { @@ -217,10 +187,7 @@ public function doAuthorize(Request $request, Response $response): Response|Resp } catch (OAuthServerException $exception) { return $exception->generateHttpResponse($response); } catch (Exception $exception) { - $body = $response->getBody(); - $body->write($exception->getMessage()); - - return $response->withStatus(500)->withBody($body); + return $this->errorResponse($response, $exception); } finally { $this->login->logout(); } @@ -243,15 +210,22 @@ public function token(Request $request, Response $response): Response|ResponseIn // All instances of OAuthServerException can be converted to a PSR-7 response return $exception->generateHttpResponse($response); } catch (Exception $exception) { - Debug::log( - 'authorization/Exception: ' - . $exception->getMessage() . '
' . $exception->getTraceAsString() - ); // Catch unexpected exceptions - $body = $response->getBody(); - $body->write($exception->getMessage()); - - return $response->withStatus(500)->withBody($body); + return $this->errorResponse($response, $exception); } } + + /** + * Log an unexpected error, without disclosing its details + */ + private function errorResponse(Response $response, Exception $exception): ResponseInterface + { + Analog::log( + 'OAuth2 error: ' . $exception->getMessage() . "\n" . $exception->getTraceAsString(), + Analog::ERROR + ); + $response->getBody()->write(_T('An error occurred', 'oauth2')); + + return $response->withStatus(500); + } } diff --git a/lib/GaletteOAuth2/Controllers/LoginController.php b/lib/GaletteOAuth2/Controllers/LoginController.php index 5f3264a..f4b7c16 100755 --- a/lib/GaletteOAuth2/Controllers/LoginController.php +++ b/lib/GaletteOAuth2/Controllers/LoginController.php @@ -111,6 +111,7 @@ public function doLogin(Request $request, Response $response): Response //Try login //FIXME: for both isLoggedIn and user_id, we can rely on login object stored in session $this->session->isLoggedIn = 'no'; + unset($this->session->client_id); $this->session->user_id = $uid = UserHelper::login($this->container, $params['login'], $params['password']); Debug::log("UserHelper::login({$params['login']}) return '{$uid}'"); @@ -155,6 +156,7 @@ public function doLogin(Request $request, Response $response): Response //FIXME: for both isLoggedIn and user_id, we can rely on login object stored in session $this->session->isLoggedIn = 'yes'; + $this->session->client_id = $client_id; // User is logged in, redirect them to authorize $url_params = [ @@ -183,6 +185,7 @@ public function logout(Request $request, Response $response): Response unset( $this->session->user_id, $this->session->isLoggedIn, + $this->session->client_id, $this->session->request_args ); session_destroy(); diff --git a/lib/GaletteOAuth2/Middleware/Authentication.php b/lib/GaletteOAuth2/Middleware/Authentication.php index 6086934..5e027f0 100755 --- a/lib/GaletteOAuth2/Middleware/Authentication.php +++ b/lib/GaletteOAuth2/Middleware/Authentication.php @@ -72,12 +72,14 @@ public function __invoke(Request $request, RequestHandler $handler): Response } $loggedIn = $this->session->isLoggedIn ?? ''; + //login rights have been checked for one client only + $loggedClient = $this->session->client_id ?? null; - if ('yes' !== $loggedIn) { + if ('yes' !== $loggedIn || $loggedClient !== $client_id) { $url = $this->routeparser->urlFor( OAUTH2_PREFIX . '_login', [], - ['redirect_url' => $_SERVER['REQUEST_URI']], + ['redirect_url' => $request->getUri()->getPath() . '?' . $request->getUri()->getQuery()], ); Debug::log("Redirect to {$url}"); diff --git a/lib/GaletteOAuth2/Repositories/ClientRepository.php b/lib/GaletteOAuth2/Repositories/ClientRepository.php index edc5c41..cfc86d2 100755 --- a/lib/GaletteOAuth2/Repositories/ClientRepository.php +++ b/lib/GaletteOAuth2/Repositories/ClientRepository.php @@ -10,13 +10,13 @@ namespace GaletteOAuth2\Repositories; +use Analog\Analog; use DI\Container; use GaletteOAuth2\Entities\ClientEntity; use GaletteOAuth2\Tools\Config; use GaletteOAuth2\Tools\Debug; use League\OAuth2\Server\Entities\ClientEntityInterface; use League\OAuth2\Server\Repositories\ClientRepositoryInterface; -use RKA\Session; /** * Client Repository @@ -26,15 +26,15 @@ */ final class ClientRepository implements ClientRepositoryInterface { + private const string EXAMPLE_PASSWORD = 'abc123'; + private Container $container; private Config $config; - private Session $session; public function __construct(Container $container) { $this->container = $container; $this->config = $this->container->get(Config::class); - $this->session = $this->container->get('oauth_session'); } /** @@ -42,55 +42,94 @@ public function __construct(Container $container) */ public function clientExists(?string $client_id): bool { - if (empty($client_id)) { + if (empty($client_id) || $client_id === 'global') { + return false; + } + if ($this->config->get($client_id) === '') { + return false; + } + if (count($this->getRedirectUris($client_id)) === 0) { + Analog::log( + sprintf( + 'OAuth2: no redirect_uri configured for client "%1$s", add "redirect_uri" to its entry in config.yml', + $client_id + ), + Analog::ERROR + ); return false; } - return $this->config->get($client_id) !== ''; + return true; + } + + /** + * Get redirect URIs allowed for a client + * + * @return string[] + */ + private function getRedirectUris(string $client_id): array + { + $uris = $this->config->get("{$client_id}.redirect_uri"); + if (is_string($uris)) { + $uris = [$uris]; + } + if (!is_array($uris)) { + return []; + } + + return array_values( + array_filter( + $uris, + fn($uri) => is_string($uri) && $uri !== '' + ) + ); } - public function getClientEntity(string $client_id): ClientEntityInterface + public function getClientEntity(string $client_id): ?ClientEntityInterface { + if (!$this->clientExists($client_id)) { + return null; + } + $client = new ClientEntity(); $client->setIdentifier($this->config->get("{$client_id}.id", $client_id)); $client->setName($client_id); - if (isset($this->session->$client_id)) { - $redirect_uri = $this->session->$client_id->redirect_uri; - } else { - $filename = OAUTH2_PREFIX . '_' . $client_id . '.redirect_uri.txt'; - $redirect_uri = file_get_contents(GALETTE_CACHE_DIR . '/' . $filename); - } - $cid = $this->config->get("{$client_id}.redirect_uri"); - /*$redirect_uri = $this->config->get("{$clientIdentifier}.redirect_uri"); - if (empty($redirect_uri)) { - $filename = OAUTH2_PREFIX . '_' . $clientIdentifier . '.redirect_uri.txt'; - $redirect_uri = file_get_contents(GALETTE_CACHE_DIR . '/' . $filename); - }*/ - $client->setRedirectUri($redirect_uri); + $client->setRedirectUri($this->getRedirectUris($client_id)); $client->setConfidential(); - Debug::log('getClientEntity() ' . Debug::printVar($client)); - return $client; } public function validateClient(string $clientIdentifier, ?string $clientSecret, ?string $grantType): bool { - if (!preg_match('/galette_/', $clientIdentifier)) { + if (!preg_match('/galette_/', $clientIdentifier) || !$this->clientExists($clientIdentifier)) { Debug::log("validateClient({$clientIdentifier}) denied"); return false; } $password = $this->config->get($clientIdentifier . '.password'); - if (!$password) { - $password = $this->config->get('global.password'); + if (!is_string($password) || $password === '') { + Analog::log( + sprintf( + 'OAuth2: no password configured for client "%1$s", add "password" to its entry in config.yml', + $clientIdentifier + ), + Analog::ERROR + ); + return false; } - $pwd = password_hash($password, PASSWORD_BCRYPT); - if (password_verify($clientSecret, $pwd) === false) { + if ($password === self::EXAMPLE_PASSWORD) { + Analog::log( + sprintf( + 'OAuth2: client "%1$s" still uses the example password, set a strong one in config.yml', + $clientIdentifier + ), + Analog::ERROR + ); return false; } - return true; + return hash_equals($password, (string)$clientSecret); } } diff --git a/lib/GaletteOAuth2/Repositories/UserRepository.php b/lib/GaletteOAuth2/Repositories/UserRepository.php deleted file mode 100755 index f02e63e..0000000 --- a/lib/GaletteOAuth2/Repositories/UserRepository.php +++ /dev/null @@ -1,52 +0,0 @@ - - * @author Johan Cwiklinski - */ -final class UserRepository implements UserRepositoryInterface -{ - private Container $container; - - public function __construct(Container $container) - { - $this->container = $container; - } - - public function getUserEntityByUserCredentials( - string $username, - string $password, - string $grantType, - ClientEntityInterface $clientEntity - ): ?UserEntityInterface { - Debug::log("getUserEntityByUserCredentials({$username}, '***', {$grantType}) "); - $user_id = UserHelper::login($this->container, $username, $password); - if ($user_id !== false) { - $user = new UserEntity(); - $user->setIdentifier((string)$user_id); - return $user; - } - - return null; - } -} diff --git a/lib/GaletteOAuth2/Tools/Debug.php b/lib/GaletteOAuth2/Tools/Debug.php index 2c0ace2..7dcd2a5 100644 --- a/lib/GaletteOAuth2/Tools/Debug.php +++ b/lib/GaletteOAuth2/Tools/Debug.php @@ -21,6 +21,15 @@ */ final class Debug { + private const array HIDDEN_PARAMS = [ + 'password', + 'client_secret', + 'code', + 'refresh_token', + 'access_token', + 'code_verifier', + ]; + public static function printVar($expression, bool $return = true) { $export = print_r($expression, true); @@ -46,21 +55,35 @@ public static function log(string $txt): void ); } + /** + * Hide secrets from parameters before they are logged + * + * @param array $params Request parameters + * + * @return array + */ + public static function hideSecrets(array $params): array + { + foreach (self::HIDDEN_PARAMS as $name) { + if (isset($params[$name])) { + $params[$name] = 'HIDDEN'; + } + } + return $params; + } + public static function logRequest(string $fct, Request $request): void { $msg = sprintf( "%s - URI: %s", $fct, - $request->getUri() + $request->getUri()->getPath() ); if (count($qp = $request->getQueryParams()) > 0) { - $msg .= "\nGET dump: " . self::printVar($qp); + $msg .= "\nGET dump: " . self::printVar(self::hideSecrets($qp)); } if (count($post = (array)$request->getParsedBody()) > 0) { - if (isset($post['password'])) { - $post['password'] = 'HIDDEN'; - } - $msg .= "\nPOST dump: " . self::printVar($post); + $msg .= "\nPOST dump: " . self::printVar(self::hideSecrets($post)); } $msg .= "\n"; Analog::log( diff --git a/tests/GaletteOAuth2/Controllers/tests/units/AuthorizationController.php b/tests/GaletteOAuth2/Controllers/tests/units/AuthorizationController.php new file mode 100644 index 0000000..c1948a1 --- /dev/null +++ b/tests/GaletteOAuth2/Controllers/tests/units/AuthorizationController.php @@ -0,0 +1,260 @@ + + */ +class AuthorizationController extends GaletteRoutingTestCase +{ + protected int $seed = 20260926180000; + protected bool $load_plugins = true; + + /** + * Set up tests + * + * @return void + */ + public function setUp(): void + { + global $session; + parent::setUp(); + + $this->session = $this->container->get('oauth_session'); + $session = $this->session; + } + + /** + * Tear down tests + * + * @return void + */ + public function tearDown(): void + { + unset( + $this->session->isLoggedIn, + $this->session->user_id, + $this->session->client_id, + $this->session->request_args + ); + parent::tearDown(); + } + + /** + * Mark user as logged in the OAuth session + * + * @param string $client_id Client the login has been checked for + * + * @return void + */ + private function logUserIn(string $client_id = 'galette_flarum'): void + { + $this->session->isLoggedIn = 'yes'; + $this->session->user_id = 1; + $this->session->client_id = $client_id; + } + + /** + * Build authorization request parameters + * + * @param string $redirect_uri Redirect URI + * + * @return array + */ + private function getAuthorizeParams(string $redirect_uri): array + { + return [ + 'response_type' => 'code', + 'client_id' => 'galette_flarum', + 'redirect_uri' => $redirect_uri, + 'scope' => 'member', + 'state' => '7d627422092a7a5ac413ac597312b9b4', + ]; + } + + /** + * Test authorization form with the configured redirect URI + * + * @return void + */ + public function testAuthorize(): void + { + $this->logUserIn(); + + $request = $this->createRequest( + route_name: OAUTH2_PREFIX . '_authorize', + query_params: $this->getAuthorizeParams('http://flarum.localhost/auth/passport') + ); + $test_response = $this->app->handle($request); + + $this->expectOK($test_response); + $this->assertStringContainsString( + 'Forum Flarum is requesting access to the following details', + (string)$test_response->getBody() + ); + } + + /** + * Test a login checked for a client cannot be used for another one + * + * @return void + */ + public function testAuthorizeRequiresLoginForSameClient(): void + { + $this->logUserIn('galette_nc'); + + $request = $this->createRequest( + route_name: OAUTH2_PREFIX . '_authorize', + query_params: $this->getAuthorizeParams('http://flarum.localhost/auth/passport') + ); + $test_response = $this->app->handle($request); + + $this->assertSame(302, $test_response->getStatusCode()); + $this->assertStringContainsString( + OAUTH2_PREFIX . '/login', + $test_response->getHeaderLine('Location') + ); + + $request = $this->createRequest( + route_name: OAUTH2_PREFIX . '_doAuthorize', + method: 'POST', + query_params: $this->getAuthorizeParams('http://flarum.localhost/auth/passport') + ); + $request = $request->withParsedBody(['approve' => '', 'scopes' => ['member']]); + $test_response = $this->app->handle($request); + + $this->assertSame(302, $test_response->getStatusCode()); + $this->assertStringContainsString( + OAUTH2_PREFIX . '/login', + $test_response->getHeaderLine('Location') + ); + } + + /** + * Test authorization form refuses a redirect URI that is not configured + * + * @return void + */ + public function testAuthorizeRefusesUnknownRedirectUri(): void + { + $this->logUserIn(); + + $request = $this->createRequest( + route_name: OAUTH2_PREFIX . '_authorize', + query_params: $this->getAuthorizeParams('https://attacker.example/cb') + ); + $test_response = $this->app->handle($request); + + $this->assertSame(401, $test_response->getStatusCode()); + $this->assertFalse($test_response->hasHeader('Location')); + $this->assertFileDoesNotExist( + GALETTE_CACHE_DIR . '/' . OAUTH2_PREFIX . '_galette_flarum.redirect_uri.txt' + ); + } + + /** + * Test approval redirects to the configured redirect URI + * + * @return void + */ + public function testDoAuthorize(): void + { + $this->logUserIn(); + + $request = $this->createRequest( + route_name: OAUTH2_PREFIX . '_doAuthorize', + method: 'POST', + query_params: $this->getAuthorizeParams('http://flarum.localhost/auth/passport') + ); + $request = $request->withParsedBody(['approve' => '', 'scopes' => ['member']]); + $test_response = $this->app->handle($request); + + $this->assertSame(302, $test_response->getStatusCode()); + $this->assertStringStartsWith( + 'http://flarum.localhost/auth/passport?code=', + $test_response->getHeaderLine('Location') + ); + } + + /** + * Test approval refuses a redirect URI that is not configured + * + * @return void + */ + public function testDoAuthorizeRefusesUnknownRedirectUri(): void + { + $this->logUserIn(); + + $request = $this->createRequest( + route_name: OAUTH2_PREFIX . '_doAuthorize', + method: 'POST', + query_params: $this->getAuthorizeParams('https://attacker.example/cb') + ); + $request = $request->withParsedBody(['approve' => '', 'scopes' => ['member']]); + $test_response = $this->app->handle($request); + + $this->assertSame(401, $test_response->getStatusCode()); + $this->assertFalse($test_response->hasHeader('Location')); + } + + /** + * Grant types that must not be accepted + * + * @return array>> + */ + public static function disabledGrantsProvider(): array + { + return [ + 'password' => [ + [ + 'grant_type' => 'password', + 'username' => 'admin', + 'password' => 'admin', + ] + ], + 'client credentials' => [ + ['grant_type' => 'client_credentials'] + ], + ]; + } + + /** + * Test only authorization code and refresh token grants are available + * + * @param array $params Token request parameters + * + * @return void + */ + #[DataProvider('disabledGrantsProvider')] + public function testTokenRefusesDisabledGrants(array $params): void + { + $request = $this->createRequest( + route_name: OAUTH2_PREFIX . '_token', + method: 'POST', + content_type: 'application/x-www-form-urlencoded' + ); + $request = $request->withParsedBody( + $params + [ + 'client_id' => 'galette_cli', + 'client_secret' => 'cli-secret-for-tests', + 'scope' => 'member', + ] + ); + $test_response = $this->app->handle($request); + + $this->assertSame(400, $test_response->getStatusCode()); + $body = json_decode((string)$test_response->getBody(), true); + $this->assertSame('unsupported_grant_type', $body['error']); + } +} diff --git a/tests/GaletteOAuth2/GaletteOAuth2.php b/tests/GaletteOAuth2/GaletteOAuth2.php index a803d30..d69053e 100644 --- a/tests/GaletteOAuth2/GaletteOAuth2.php +++ b/tests/GaletteOAuth2/GaletteOAuth2.php @@ -38,11 +38,13 @@ public function setUp(): void /** - * Test stripAccents + * Run the whole authorization code flow * - * @return void + * @param string[] $checked_scopes Scopes checked on the consent screen + * + * @return array Resource owner data */ - public function testFlow(): void + private function runFlow(array $checked_scopes): array { $member_one = $this->getMemberOne(); $data = $this->dataAdherentOne(); @@ -51,7 +53,7 @@ public function testFlow(): void $provider = new \Galette\OAuth2\Client\Provider\Galette([ //information related to the app where you will use galette-oauth2 'clientId' => 'galette_cli', // The client ID assigned to you - 'clientSecret' => 'abc123', // The client password assigned to you + 'clientSecret' => 'cli-secret-for-tests', // The client password assigned to you 'redirectUri' => 'http://localhost:8888', // The return URL you specified for your app //information related to the galette instance you want to connect to 'instance' => 'http://localhost:8888', // The instance of Galette you want to connect to @@ -98,7 +100,8 @@ public function testFlow(): void $response = $guzzle->request('POST', $authorizationUrl, [ 'form_params' => [ - 'approve' => true + 'approve' => true, + 'scopes' => $checked_scopes ] ]); @@ -134,7 +137,34 @@ public function testFlow(): void $this->assertSame($member_one->id, $resourceOwner->getId()); $this->assertSame($data['login_adh'], $resourceOwner->getUsername()); $this->assertSame($data['email_adh'], $resourceOwner->getEmail()); + + return $resourceOwner_array; + } + + /** + * Test authorization code flow, all scopes checked + * + * @return void + */ + public function testFlow(): void + { + $resourceOwner_array = $this->runFlow(['member', 'member:localization', 'member:due_date']); + + $this->assertArrayHasKey('address', $resourceOwner_array); //due date scope is requested from configuration file $this->assertArrayHasKey('due_date', $resourceOwner_array); } + + /** + * Test scopes unchecked on the consent screen are not given + * + * @return void + */ + public function testFlowWithUncheckedScope(): void + { + $resourceOwner_array = $this->runFlow(['member', 'member:localization', 'member:unknown']); + + $this->assertArrayHasKey('address', $resourceOwner_array); + $this->assertArrayNotHasKey('due_date', $resourceOwner_array); + } } diff --git a/tests/GaletteOAuth2/Repositories/tests/units/ClientRepository.php b/tests/GaletteOAuth2/Repositories/tests/units/ClientRepository.php index 7b78314..0537dd9 100644 --- a/tests/GaletteOAuth2/Repositories/tests/units/ClientRepository.php +++ b/tests/GaletteOAuth2/Repositories/tests/units/ClientRepository.php @@ -8,6 +8,7 @@ namespace GaletteOAuth2\Repositories\tests\units; +use Analog\Analog; use Galette\Tests\GaletteTestCase; use PHPUnit\Framework\Attributes\DataProvider; @@ -62,6 +63,7 @@ public static function invalidClientIdsProvider(): array 'unknown client' => ['unknown_client'], 'galette_unknown' => ['galette_unknown'], 'random string' => ['some_random_string'], + 'reserved global entry' => ['global'], ]; } @@ -96,4 +98,112 @@ public function testClientExistsWithInvalidClients(?string $client_id): void "Client '$client_id' should not exist in configuration" ); } + + /** + * Test the redirect URI always comes from the configuration + * + * @return void + */ + public function testGetClientEntityUsesConfiguredRedirectUri(): void + { + //values that used to be trusted: session and cache file + $this->session->galette_flarum = new \stdClass(); + $this->session->galette_flarum->redirect_uri = 'https://attacker.example/cb'; + $cache_file = GALETTE_CACHE_DIR . '/' . OAUTH2_PREFIX . '_galette_nc.redirect_uri.txt'; + file_put_contents($cache_file, 'https://attacker.example/cb'); + + try { + $clientRepository = new \GaletteOAuth2\Repositories\ClientRepository($this->container); + + $client = $clientRepository->getClientEntity('galette_flarum'); + $this->assertNotNull($client); + $this->assertSame(['http://flarum.localhost/auth/passport'], $client->getRedirectUri()); + + $client = $clientRepository->getClientEntity('galette_nc'); + $this->assertNotNull($client); + $this->assertSame( + ['http://localhost/nextcloud/apps/sociallogin/custom_oauth2/galette'], + $client->getRedirectUri() + ); + + $client = $clientRepository->getClientEntity('galette_cli'); + $this->assertNotNull($client); + $this->assertCount(4, $client->getRedirectUri()); + } finally { + unset($this->session->galette_flarum); + unlink($cache_file); + } + } + + /** + * Test a client without configured redirect URI is refused + * + * @return void + */ + public function testClientWithoutRedirectUriIsRefused(): void + { + $clientRepository = new \GaletteOAuth2\Repositories\ClientRepository($this->container); + + $this->assertFalse($clientRepository->clientExists('galette_noredirect')); + $this->expectLogEntry( + Analog::ERROR, + 'OAuth2: no redirect_uri configured for client "galette_noredirect"' + ); + + $this->assertNull($clientRepository->getClientEntity('galette_noredirect')); + $this->expectLogEntry( + Analog::ERROR, + 'OAuth2: no redirect_uri configured for client "galette_noredirect"' + ); + } + + /** + * Test client secret validation + * + * @return void + */ + public function testValidateClient(): void + { + $clientRepository = new \GaletteOAuth2\Repositories\ClientRepository($this->container); + + $this->assertTrue($clientRepository->validateClient('galette_cli', 'cli-secret-for-tests', 'authorization_code')); + $this->assertFalse($clientRepository->validateClient('galette_cli', 'wrong-secret', 'authorization_code')); + $this->assertFalse($clientRepository->validateClient('galette_cli', '', 'authorization_code')); + $this->assertFalse($clientRepository->validateClient('galette_cli', null, 'authorization_code')); + //secret of another client + $this->assertFalse($clientRepository->validateClient('galette_cli', 'flarum-secret-for-tests', 'authorization_code')); + $this->assertFalse($clientRepository->validateClient('unknown_client', 'cli-secret-for-tests', 'authorization_code')); + } + + /** + * Test client without its own password is refused, even with the global one + * + * @return void + */ + public function testValidateClientWithoutPassword(): void + { + $clientRepository = new \GaletteOAuth2\Repositories\ClientRepository($this->container); + + $this->assertFalse($clientRepository->validateClient('galette_nopassword', 'abc123', 'authorization_code')); + $this->expectLogEntry( + Analog::ERROR, + 'OAuth2: no password configured for client "galette_nopassword"' + ); + } + + /** + * Test client with the default example password is refused + * + * @return void + */ + public function testValidateClientWithDefaultPassword(): void + { + $clientRepository = new \GaletteOAuth2\Repositories\ClientRepository($this->container); + + $this->assertFalse($clientRepository->validateClient('galette_defaultpassword', 'abc123', 'authorization_code')); + $this->expectLogEntry( + Analog::ERROR, + 'OAuth2: client "galette_defaultpassword" still uses the example password' + ); + } } diff --git a/tests/GaletteOAuth2/Tools/tests/units/Debug.php b/tests/GaletteOAuth2/Tools/tests/units/Debug.php new file mode 100644 index 0000000..1ab5ce7 --- /dev/null +++ b/tests/GaletteOAuth2/Tools/tests/units/Debug.php @@ -0,0 +1,54 @@ + + */ +class Debug extends TestCase +{ + /** + * Test secrets are hidden from logged parameters + * + * @return void + */ + public function testHideSecrets(): void + { + $params = [ + 'grant_type' => 'authorization_code', + 'client_id' => 'galette_cli', + 'client_secret' => 'cli-secret-for-tests', + 'code' => 'def50200abcdef', + 'refresh_token' => 'def50200123456', + 'access_token' => 'eyJ0eXAiOiJKV1Qi', + 'code_verifier' => 'verifier', + 'password' => 'mypassword', + 'redirect_uri' => 'http://localhost:8888', + ]; + + $this->assertSame( + [ + 'grant_type' => 'authorization_code', + 'client_id' => 'galette_cli', + 'client_secret' => 'HIDDEN', + 'code' => 'HIDDEN', + 'refresh_token' => 'HIDDEN', + 'access_token' => 'HIDDEN', + 'code_verifier' => 'HIDDEN', + 'password' => 'HIDDEN', + 'redirect_uri' => 'http://localhost:8888', + ], + \GaletteOAuth2\Tools\Debug::hideSecrets($params) + ); + } +} diff --git a/tests/config/config.yml b/tests/config/config.yml index 0362614..7e20130 100644 --- a/tests/config/config.yml +++ b/tests/config/config.yml @@ -1,12 +1,18 @@ global: + # no longer used as a fallback for clients without their own password password: abc123 + redirect_uri: 'http://localhost/global' galette_flarum: + password: flarum-secret-for-tests title: 'Forum Flarum' redirect_logout: 'http://flarum.localhost' + redirect_uri: 'http://flarum.localhost/auth/passport' authorize: uptodate galette_nc: + password: nc-secret-for-tests title: 'Nextcloud' redirect_logout: 'http://localhost/nextcloud' + redirect_uri: 'http://localhost/nextcloud/apps/sociallogin/custom_oauth2/galette' authorize: teamonly scopes: - member @@ -14,9 +20,26 @@ galette_nc: - member:phones - member:groups galette_cli: - password: abc123 + password: cli-secret-for-tests title: CLI for testing redirect_logout: 'http://localhost' + # 127.0.0.1 is a loopback address: any port is accepted + redirect_uri: + - 'http://localhost:8888' + - 'http://127.0.0.1/callback' + - 'http://127.0.0.1/oauth2-test-callback' + - 'http://127.0.0.1/plugins/oauth2/test-callback' authorize: teamonly scopes: - member:due_date +galette_noredirect: + password: noredirect-secret-for-tests + title: Client without redirect URI + authorize: teamonly +galette_nopassword: + title: Client without password + redirect_uri: 'http://localhost/nopassword' +galette_defaultpassword: + password: abc123 + title: Client with default password + redirect_uri: 'http://localhost/defaultpassword' diff --git a/tests/e2e/specs/oauth2-flow.spec.ts b/tests/e2e/specs/oauth2-flow.spec.ts index 9721c22..8b89c62 100644 --- a/tests/e2e/specs/oauth2-flow.spec.ts +++ b/tests/e2e/specs/oauth2-flow.spec.ts @@ -162,7 +162,7 @@ test.describe('OAuth2 Plugin', () => { code: authorizationCode!, redirect_uri: redirectUri, client_id: clientId, - client_secret: 'abc123', + client_secret: 'cli-secret-for-tests', }, });