diff options
author | Richard Steinmetz <richard@steinmetz.cloud> | 2025-02-17 14:34:01 +0100 |
---|---|---|
committer | Richard Steinmetz <richard@steinmetz.cloud> | 2025-04-01 11:25:52 +0200 |
commit | 246da73a363c11d02eed69e80e76d7c9a9a04c7b (patch) | |
tree | 925d2a3109f1cd3327a0721380471c773d7d2b04 | |
parent | b03ffab5f0f39139c71cb2b8c370ca3f3d1ad391 (diff) | |
download | nextcloud-server-246da73a363c11d02eed69e80e76d7c9a9a04c7b.tar.gz nextcloud-server-246da73a363c11d02eed69e80e76d7c9a9a04c7b.zip |
fix(oauth2): retain support for legacy ownCloud clientsfix/oauth2/retain-legacy-oc-client-support
Signed-off-by: Richard Steinmetz <richard@steinmetz.cloud>
-rw-r--r-- | apps/dav/appinfo/v1/webdav.php | 3 | ||||
-rw-r--r-- | apps/dav/lib/Connector/Sabre/BearerAuth.php | 10 | ||||
-rw-r--r-- | apps/dav/lib/Server.php | 3 | ||||
-rw-r--r-- | apps/dav/tests/unit/Connector/Sabre/BearerAuthTest.php | 8 | ||||
-rw-r--r-- | apps/oauth2/appinfo/info.xml | 4 | ||||
-rw-r--r-- | apps/oauth2/composer/composer/autoload_classmap.php | 1 | ||||
-rw-r--r-- | apps/oauth2/composer/composer/autoload_static.php | 1 | ||||
-rw-r--r-- | apps/oauth2/lib/Command/ImportLegacyOcClient.php | 76 | ||||
-rw-r--r-- | apps/oauth2/lib/Controller/LoginRedirectorController.php | 15 | ||||
-rw-r--r-- | apps/oauth2/openapi.json | 9 | ||||
-rw-r--r-- | apps/oauth2/tests/Controller/LoginRedirectorControllerTest.php | 84 | ||||
-rw-r--r-- | core/Controller/ClientFlowLoginController.php | 20 | ||||
-rw-r--r-- | core/templates/loginflow/authpicker.php | 2 | ||||
-rw-r--r-- | core/templates/loginflow/grant.php | 1 | ||||
-rw-r--r-- | lib/private/Repair/Owncloud/MigrateOauthTables.php | 21 | ||||
-rw-r--r-- | openapi.json | 9 | ||||
-rw-r--r-- | tests/Core/Controller/ClientFlowLoginControllerTest.php | 10 |
17 files changed, 264 insertions, 13 deletions
diff --git a/apps/dav/appinfo/v1/webdav.php b/apps/dav/appinfo/v1/webdav.php index fe47ba74652..baeae66bb20 100644 --- a/apps/dav/appinfo/v1/webdav.php +++ b/apps/dav/appinfo/v1/webdav.php @@ -61,7 +61,8 @@ $authPlugin = new \Sabre\DAV\Auth\Plugin($authBackend); $bearerAuthPlugin = new BearerAuth( Server::get(IUserSession::class), Server::get(ISession::class), - Server::get(IRequest::class) + Server::get(IRequest::class), + Server::get(IConfig::class), ); $authPlugin->addBackend($bearerAuthPlugin); diff --git a/apps/dav/lib/Connector/Sabre/BearerAuth.php b/apps/dav/lib/Connector/Sabre/BearerAuth.php index 07bb3e052a5..e189d8fa128 100644 --- a/apps/dav/lib/Connector/Sabre/BearerAuth.php +++ b/apps/dav/lib/Connector/Sabre/BearerAuth.php @@ -7,6 +7,7 @@ namespace OCA\DAV\Connector\Sabre; use OCP\AppFramework\Http; use OCP\Defaults; +use OCP\IConfig; use OCP\IRequest; use OCP\ISession; use OCP\IUserSession; @@ -19,6 +20,7 @@ class BearerAuth extends AbstractBearer { private IUserSession $userSession, private ISession $session, private IRequest $request, + private IConfig $config, private string $principalPrefix = 'principals/users/', ) { // setup realm @@ -57,6 +59,14 @@ class BearerAuth extends AbstractBearer { * @param ResponseInterface $response */ public function challenge(RequestInterface $request, ResponseInterface $response): void { + // Legacy ownCloud clients still authenticate via OAuth2 + $enableOcClients = $this->config->getSystemValueBool('oauth2.enable_oc_clients', false); + $userAgent = $request->getHeader('User-Agent'); + if ($enableOcClients && $userAgent !== null && str_contains($userAgent, 'mirall')) { + parent::challenge($request, $response); + return; + } + $response->setStatus(Http::STATUS_UNAUTHORIZED); } } diff --git a/apps/dav/lib/Server.php b/apps/dav/lib/Server.php index a14b49c178e..9ea18c029c8 100644 --- a/apps/dav/lib/Server.php +++ b/apps/dav/lib/Server.php @@ -157,7 +157,8 @@ class Server { $bearerAuthBackend = new BearerAuth( \OCP\Server::get(IUserSession::class), \OCP\Server::get(ISession::class), - \OCP\Server::get(IRequest::class) + \OCP\Server::get(IRequest::class), + \OCP\Server::get(IConfig::class), ); $authPlugin->addBackend($bearerAuthBackend); // because we are throwing exceptions this plugin has to be the last one diff --git a/apps/dav/tests/unit/Connector/Sabre/BearerAuthTest.php b/apps/dav/tests/unit/Connector/Sabre/BearerAuthTest.php index 06c070454af..99c2a461557 100644 --- a/apps/dav/tests/unit/Connector/Sabre/BearerAuthTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/BearerAuthTest.php @@ -7,10 +7,12 @@ namespace OCA\DAV\Tests\unit\Connector\Sabre; use OC\User\Session; use OCA\DAV\Connector\Sabre\BearerAuth; +use OCP\IConfig; use OCP\IRequest; use OCP\ISession; use OCP\IUser; use OCP\IUserSession; +use PHPUnit\Framework\MockObject\MockObject; use Sabre\HTTP\RequestInterface; use Sabre\HTTP\ResponseInterface; use Test\TestCase; @@ -28,17 +30,21 @@ class BearerAuthTest extends TestCase { /** @var BearerAuth */ private $bearerAuth; + private IConfig&MockObject $config; + protected function setUp(): void { parent::setUp(); $this->userSession = $this->createMock(Session::class); $this->session = $this->createMock(ISession::class); $this->request = $this->createMock(IRequest::class); + $this->config = $this->createMock(IConfig::class); $this->bearerAuth = new BearerAuth( $this->userSession, $this->session, - $this->request + $this->request, + $this->config, ); } diff --git a/apps/oauth2/appinfo/info.xml b/apps/oauth2/appinfo/info.xml index 5e4994fafa8..6ab30292ab0 100644 --- a/apps/oauth2/appinfo/info.xml +++ b/apps/oauth2/appinfo/info.xml @@ -33,6 +33,10 @@ </post-migration> </repair-steps> + <commands> + <command>OCA\OAuth2\Command\ImportLegacyOcClient</command> + </commands> + <settings> <admin>OCA\OAuth2\Settings\Admin</admin> </settings> diff --git a/apps/oauth2/composer/composer/autoload_classmap.php b/apps/oauth2/composer/composer/autoload_classmap.php index cba0ef43dfc..f5fc5bfe251 100644 --- a/apps/oauth2/composer/composer/autoload_classmap.php +++ b/apps/oauth2/composer/composer/autoload_classmap.php @@ -8,6 +8,7 @@ $baseDir = $vendorDir; return array( 'Composer\\InstalledVersions' => $vendorDir . '/composer/InstalledVersions.php', 'OCA\\OAuth2\\BackgroundJob\\CleanupExpiredAuthorizationCode' => $baseDir . '/../lib/BackgroundJob/CleanupExpiredAuthorizationCode.php', + 'OCA\\OAuth2\\Command\\ImportLegacyOcClient' => $baseDir . '/../lib/Command/ImportLegacyOcClient.php', 'OCA\\OAuth2\\Controller\\LoginRedirectorController' => $baseDir . '/../lib/Controller/LoginRedirectorController.php', 'OCA\\OAuth2\\Controller\\OauthApiController' => $baseDir . '/../lib/Controller/OauthApiController.php', 'OCA\\OAuth2\\Controller\\SettingsController' => $baseDir . '/../lib/Controller/SettingsController.php', diff --git a/apps/oauth2/composer/composer/autoload_static.php b/apps/oauth2/composer/composer/autoload_static.php index c4905bc96c4..c4843fa1990 100644 --- a/apps/oauth2/composer/composer/autoload_static.php +++ b/apps/oauth2/composer/composer/autoload_static.php @@ -23,6 +23,7 @@ class ComposerStaticInitOAuth2 public static $classMap = array ( 'Composer\\InstalledVersions' => __DIR__ . '/..' . '/composer/InstalledVersions.php', 'OCA\\OAuth2\\BackgroundJob\\CleanupExpiredAuthorizationCode' => __DIR__ . '/..' . '/../lib/BackgroundJob/CleanupExpiredAuthorizationCode.php', + 'OCA\\OAuth2\\Command\\ImportLegacyOcClient' => __DIR__ . '/..' . '/../lib/Command/ImportLegacyOcClient.php', 'OCA\\OAuth2\\Controller\\LoginRedirectorController' => __DIR__ . '/..' . '/../lib/Controller/LoginRedirectorController.php', 'OCA\\OAuth2\\Controller\\OauthApiController' => __DIR__ . '/..' . '/../lib/Controller/OauthApiController.php', 'OCA\\OAuth2\\Controller\\SettingsController' => __DIR__ . '/..' . '/../lib/Controller/SettingsController.php', diff --git a/apps/oauth2/lib/Command/ImportLegacyOcClient.php b/apps/oauth2/lib/Command/ImportLegacyOcClient.php new file mode 100644 index 00000000000..acdc57cf991 --- /dev/null +++ b/apps/oauth2/lib/Command/ImportLegacyOcClient.php @@ -0,0 +1,76 @@ +<?php + +declare(strict_types=1); + +/** + * SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +namespace OCA\OAuth2\Command; + +use OCA\OAuth2\Db\Client; +use OCA\OAuth2\Db\ClientMapper; +use OCP\IConfig; +use OCP\Security\ICrypto; +use Symfony\Component\Console\Command\Command; +use Symfony\Component\Console\Input\InputArgument; +use Symfony\Component\Console\Input\InputInterface; +use Symfony\Component\Console\Output\OutputInterface; + +class ImportLegacyOcClient extends Command { + private const ARGUMENT_CLIENT_ID = 'client-id'; + private const ARGUMENT_CLIENT_SECRET = 'client-secret'; + + public function __construct( + private readonly IConfig $config, + private readonly ICrypto $crypto, + private readonly ClientMapper $clientMapper, + ) { + parent::__construct(); + } + + protected function configure(): void { + $this->setName('oauth2:import-legacy-oc-client'); + $this->setDescription('This command is only required to be run on instances which were migrated from ownCloud without the oauth2.enable_oc_clients system config! Import a legacy Oauth2 client from an ownCloud instance and migrate it. The data is expected to be straight out of the database table oc_oauth2_clients.'); + $this->addArgument( + self::ARGUMENT_CLIENT_ID, + InputArgument::REQUIRED, + 'Value of the "identifier" column', + ); + $this->addArgument( + self::ARGUMENT_CLIENT_SECRET, + InputArgument::REQUIRED, + 'Value of the "secret" column', + ); + } + + public function isEnabled(): bool { + return $this->config->getSystemValueBool('oauth2.enable_oc_clients', false); + } + + protected function execute(InputInterface $input, OutputInterface $output): int { + /** @var string $clientId */ + $clientId = $input->getArgument(self::ARGUMENT_CLIENT_ID); + + /** @var string $clientSecret */ + $clientSecret = $input->getArgument(self::ARGUMENT_CLIENT_SECRET); + + // Should not happen but just to be sure + if (empty($clientId) || empty($clientSecret)) { + return 1; + } + + $hashedClientSecret = bin2hex($this->crypto->calculateHMAC($clientSecret)); + + $client = new Client(); + $client->setName('ownCloud Desktop Client'); + $client->setRedirectUri('http://localhost:*'); + $client->setClientIdentifier($clientId); + $client->setSecret($hashedClientSecret); + $this->clientMapper->insert($client); + + $output->writeln('<info>Client imported successfully</info>'); + return 0; + } +} diff --git a/apps/oauth2/lib/Controller/LoginRedirectorController.php b/apps/oauth2/lib/Controller/LoginRedirectorController.php index db233e7956e..7241b35cdcf 100644 --- a/apps/oauth2/lib/Controller/LoginRedirectorController.php +++ b/apps/oauth2/lib/Controller/LoginRedirectorController.php @@ -20,6 +20,7 @@ use OCP\AppFramework\Http\Attribute\UseSession; use OCP\AppFramework\Http\RedirectResponse; use OCP\AppFramework\Http\TemplateResponse; use OCP\IAppConfig; +use OCP\IConfig; use OCP\IL10N; use OCP\IRequest; use OCP\ISession; @@ -45,6 +46,7 @@ class LoginRedirectorController extends Controller { private IL10N $l, private ISecureRandom $random, private IAppConfig $appConfig, + private IConfig $config, ) { parent::__construct($appName, $request); } @@ -55,6 +57,7 @@ class LoginRedirectorController extends Controller { * @param string $client_id Client ID * @param string $state State of the flow * @param string $response_type Response type for the flow + * @param string $redirect_uri URI to redirect to after the flow (is only used for legacy ownCloud clients) * @return TemplateResponse<Http::STATUS_OK, array{}>|RedirectResponse<Http::STATUS_SEE_OTHER, array{}> * * 200: Client not found @@ -65,7 +68,8 @@ class LoginRedirectorController extends Controller { #[UseSession] public function authorize($client_id, $state, - $response_type): TemplateResponse|RedirectResponse { + $response_type, + string $redirect_uri = ''): TemplateResponse|RedirectResponse { try { $client = $this->clientMapper->getByIdentifier($client_id); } catch (ClientNotFoundException $e) { @@ -81,6 +85,13 @@ class LoginRedirectorController extends Controller { return new RedirectResponse($url); } + $enableOcClients = $this->config->getSystemValueBool('oauth2.enable_oc_clients', false); + + $providedRedirectUri = ''; + if ($enableOcClients && $client->getRedirectUri() === 'http://localhost:*') { + $providedRedirectUri = $redirect_uri; + } + $this->session->set('oauth.state', $state); if (in_array($client->getName(), $this->appConfig->getValueArray('oauth2', 'skipAuthPickerApplications', []))) { @@ -95,6 +106,7 @@ class LoginRedirectorController extends Controller { [ 'stateToken' => $stateToken, 'clientIdentifier' => $client->getClientIdentifier(), + 'providedRedirectUri' => $providedRedirectUri, ] ); } else { @@ -102,6 +114,7 @@ class LoginRedirectorController extends Controller { 'core.ClientFlowLogin.showAuthPickerPage', [ 'clientIdentifier' => $client->getClientIdentifier(), + 'providedRedirectUri' => $providedRedirectUri, ] ); } diff --git a/apps/oauth2/openapi.json b/apps/oauth2/openapi.json index cdf52de3f5e..76cd2da3150 100644 --- a/apps/oauth2/openapi.json +++ b/apps/oauth2/openapi.json @@ -65,6 +65,15 @@ "schema": { "type": "string" } + }, + { + "name": "redirect_uri", + "in": "query", + "description": "URI to redirect to after the flow (is only used for legacy ownCloud clients)", + "schema": { + "type": "string", + "default": "" + } } ], "responses": { diff --git a/apps/oauth2/tests/Controller/LoginRedirectorControllerTest.php b/apps/oauth2/tests/Controller/LoginRedirectorControllerTest.php index afa5aae4f07..1e090665735 100644 --- a/apps/oauth2/tests/Controller/LoginRedirectorControllerTest.php +++ b/apps/oauth2/tests/Controller/LoginRedirectorControllerTest.php @@ -13,6 +13,7 @@ use OCA\OAuth2\Exceptions\ClientNotFoundException; use OCP\AppFramework\Http\RedirectResponse; use OCP\AppFramework\Http\TemplateResponse; use OCP\IAppConfig; +use OCP\IConfig; use OCP\IL10N; use OCP\IRequest; use OCP\ISession; @@ -32,6 +33,7 @@ class LoginRedirectorControllerTest extends TestCase { private IL10N&MockObject $l; private ISecureRandom&MockObject $random; private IAppConfig&MockObject $appConfig; + private IConfig&MockObject $config; private LoginRedirectorController $loginRedirectorController; @@ -45,6 +47,7 @@ class LoginRedirectorControllerTest extends TestCase { $this->l = $this->createMock(IL10N::class); $this->random = $this->createMock(ISecureRandom::class); $this->appConfig = $this->createMock(IAppConfig::class); + $this->config = $this->createMock(IConfig::class); $this->loginRedirectorController = new LoginRedirectorController( 'oauth2', @@ -55,6 +58,7 @@ class LoginRedirectorControllerTest extends TestCase { $this->l, $this->random, $this->appConfig, + $this->config, ); } @@ -77,9 +81,15 @@ class LoginRedirectorControllerTest extends TestCase { 'core.ClientFlowLogin.showAuthPickerPage', [ 'clientIdentifier' => 'MyClientIdentifier', + 'providedRedirectUri' => '', ] ) ->willReturn('https://example.com/?clientIdentifier=foo'); + $this->config + ->expects($this->once()) + ->method('getSystemValueBool') + ->with('oauth2.enable_oc_clients', false) + ->willReturn(false); $expected = new RedirectResponse('https://example.com/?clientIdentifier=foo'); $this->assertEquals($expected, $this->loginRedirectorController->authorize('MyClientId', 'MyState', 'code')); @@ -124,9 +134,15 @@ class LoginRedirectorControllerTest extends TestCase { [ 'stateToken' => 'MyStateToken', 'clientIdentifier' => 'MyClientIdentifier', + 'providedRedirectUri' => '', ] ) ->willReturn('https://example.com/?clientIdentifier=foo'); + $this->config + ->expects($this->once()) + ->method('getSystemValueBool') + ->with('oauth2.enable_oc_clients', false) + ->willReturn(false); $expected = new RedirectResponse('https://example.com/?clientIdentifier=foo'); $this->assertEquals($expected, $this->loginRedirectorController->authorize('MyClientId', 'MyState', 'code')); @@ -150,6 +166,74 @@ class LoginRedirectorControllerTest extends TestCase { $this->assertEquals($expected, $this->loginRedirectorController->authorize('MyClientId', 'MyState', 'wrongcode')); } + public function testAuthorizeWithLegacyOcClient(): void { + $client = new Client(); + $client->setClientIdentifier('MyClientIdentifier'); + $client->setRedirectUri('http://localhost:*'); + $this->clientMapper + ->expects($this->once()) + ->method('getByIdentifier') + ->with('MyClientId') + ->willReturn($client); + $this->session + ->expects($this->once()) + ->method('set') + ->with('oauth.state', 'MyState'); + $this->urlGenerator + ->expects($this->once()) + ->method('linkToRouteAbsolute') + ->with( + 'core.ClientFlowLogin.showAuthPickerPage', + [ + 'clientIdentifier' => 'MyClientIdentifier', + 'providedRedirectUri' => 'http://localhost:30000', + ] + ) + ->willReturn('https://example.com/?clientIdentifier=foo&providedRedirectUri=http://localhost:30000'); + $this->config + ->expects($this->once()) + ->method('getSystemValueBool') + ->with('oauth2.enable_oc_clients', false) + ->willReturn(true); + + $expected = new RedirectResponse('https://example.com/?clientIdentifier=foo&providedRedirectUri=http://localhost:30000'); + $this->assertEquals($expected, $this->loginRedirectorController->authorize('MyClientId', 'MyState', 'code', 'http://localhost:30000')); + } + + public function testAuthorizeNotForwardingUntrustedURIs(): void { + $client = new Client(); + $client->setClientIdentifier('MyClientIdentifier'); + $this->clientMapper + ->expects($this->once()) + ->method('getByIdentifier') + ->with('MyClientId') + ->willReturn($client); + $this->session + ->expects($this->once()) + ->method('set') + ->with('oauth.state', 'MyState'); + $this->urlGenerator + ->expects($this->once()) + ->method('linkToRouteAbsolute') + ->with( + 'core.ClientFlowLogin.showAuthPickerPage', + [ + 'clientIdentifier' => 'MyClientIdentifier', + 'providedRedirectUri' => '', + ] + ) + ->willReturn('https://example.com/?clientIdentifier=foo'); + $this->config + ->expects($this->once()) + ->method('getSystemValueBool') + ->with('oauth2.enable_oc_clients', false) + ->willReturn(false); + + $expected = new RedirectResponse('https://example.com/?clientIdentifier=foo'); + $this->assertEquals($expected, $this->loginRedirectorController->authorize('MyClientId', 'MyState', 'code', 'http://untrusted-uri.com')); + } + + public function testClientNotFound(): void { $clientNotFound = new ClientNotFoundException('could not find client test123', 0); $this->clientMapper diff --git a/core/Controller/ClientFlowLoginController.php b/core/Controller/ClientFlowLoginController.php index 8d84ac100fa..99074e6ff59 100644 --- a/core/Controller/ClientFlowLoginController.php +++ b/core/Controller/ClientFlowLoginController.php @@ -26,6 +26,7 @@ use OCP\Authentication\Exceptions\InvalidTokenException; use OCP\Authentication\Token\IToken; use OCP\Defaults; use OCP\EventDispatcher\IEventDispatcher; +use OCP\IConfig; use OCP\IL10N; use OCP\IRequest; use OCP\ISession; @@ -55,6 +56,7 @@ class ClientFlowLoginController extends Controller { private ICrypto $crypto, private IEventDispatcher $eventDispatcher, private ITimeFactory $timeFactory, + private IConfig $config, ) { parent::__construct($appName, $request); } @@ -89,7 +91,7 @@ class ClientFlowLoginController extends Controller { #[NoCSRFRequired] #[UseSession] #[FrontpageRoute(verb: 'GET', url: '/login/flow')] - public function showAuthPickerPage(string $clientIdentifier = '', string $user = '', int $direct = 0): StandaloneTemplateResponse { + public function showAuthPickerPage(string $clientIdentifier = '', string $user = '', int $direct = 0, string $providedRedirectUri = ''): StandaloneTemplateResponse { $clientName = $this->getClientName(); $client = null; if ($clientIdentifier !== '') { @@ -142,6 +144,7 @@ class ClientFlowLoginController extends Controller { 'oauthState' => $this->session->get('oauth.state'), 'user' => $user, 'direct' => $direct, + 'providedRedirectUri' => $providedRedirectUri, ], 'guest' ); @@ -161,6 +164,7 @@ class ClientFlowLoginController extends Controller { string $stateToken = '', string $clientIdentifier = '', int $direct = 0, + string $providedRedirectUri = '', ): Response { if (!$this->isValidToken($stateToken)) { return $this->stateTokenForbiddenResponse(); @@ -197,6 +201,7 @@ class ClientFlowLoginController extends Controller { 'serverHost' => $this->getServerPath(), 'oauthState' => $this->session->get('oauth.state'), 'direct' => $direct, + 'providedRedirectUri' => $providedRedirectUri, ], 'guest' ); @@ -211,6 +216,7 @@ class ClientFlowLoginController extends Controller { public function generateAppPassword( string $stateToken, string $clientIdentifier = '', + string $providedRedirectUri = '', ): Response { if (!$this->isValidToken($stateToken)) { $this->session->remove(self::STATE_NAME); @@ -270,7 +276,19 @@ class ClientFlowLoginController extends Controller { $accessToken->setCodeCreatedAt($this->timeFactory->now()->getTimestamp()); $this->accessTokenMapper->insert($accessToken); + $enableOcClients = $this->config->getSystemValueBool('oauth2.enable_oc_clients', false); + $redirectUri = $client->getRedirectUri(); + if ($enableOcClients && $redirectUri === 'http://localhost:*') { + // Sanity check untrusted redirect URI provided by the client first + if (!preg_match('/^http:\/\/localhost:[0-9]+$/', $providedRedirectUri)) { + $response = new Response(); + $response->setStatus(Http::STATUS_FORBIDDEN); + return $response; + } + + $redirectUri = $providedRedirectUri; + } if (parse_url($redirectUri, PHP_URL_QUERY)) { $redirectUri .= '&'; diff --git a/core/templates/loginflow/authpicker.php b/core/templates/loginflow/authpicker.php index 94aace60cba..47e3113604d 100644 --- a/core/templates/loginflow/authpicker.php +++ b/core/templates/loginflow/authpicker.php @@ -31,7 +31,7 @@ $urlGenerator = $_['urlGenerator']; <br/> <p id="redirect-link"> - <form id="login-form" action="<?php p($urlGenerator->linkToRoute('core.ClientFlowLogin.grantPage', ['stateToken' => $_['stateToken'], 'clientIdentifier' => $_['clientIdentifier'], 'oauthState' => $_['oauthState'], 'user' => $_['user'], 'direct' => $_['direct']])) ?>" method="get"> + <form id="login-form" action="<?php p($urlGenerator->linkToRoute('core.ClientFlowLogin.grantPage', ['stateToken' => $_['stateToken'], 'clientIdentifier' => $_['clientIdentifier'], 'oauthState' => $_['oauthState'], 'user' => $_['user'], 'direct' => $_['direct'], 'providedRedirectUri' => $_['providedRedirectUri']])) ?>" method="get"> <input type="submit" class="login primary icon-confirm-white" value="<?php p($l->t('Log in')) ?>" disabled> </form> </p> diff --git a/core/templates/loginflow/grant.php b/core/templates/loginflow/grant.php index dd2fb8d1a8e..6beafccc96e 100644 --- a/core/templates/loginflow/grant.php +++ b/core/templates/loginflow/grant.php @@ -35,6 +35,7 @@ $urlGenerator = $_['urlGenerator']; <input type="hidden" name="requesttoken" value="<?php p($_['requesttoken']) ?>" /> <input type="hidden" name="stateToken" value="<?php p($_['stateToken']) ?>" /> <input type="hidden" name="oauthState" value="<?php p($_['oauthState']) ?>" /> + <input type="hidden" name="providedRedirectUri" value="<?php p($_['providedRedirectUri']) ?>"> <?php if ($_['direct']) { ?> <input type="hidden" name="direct" value="1" /> <?php } ?> diff --git a/lib/private/Repair/Owncloud/MigrateOauthTables.php b/lib/private/Repair/Owncloud/MigrateOauthTables.php index cd5087cb588..de26a907e02 100644 --- a/lib/private/Repair/Owncloud/MigrateOauthTables.php +++ b/lib/private/Repair/Owncloud/MigrateOauthTables.php @@ -15,6 +15,7 @@ use OCA\OAuth2\Db\AccessTokenMapper; use OCP\AppFramework\Utility\ITimeFactory; use OCP\Authentication\Token\IToken; use OCP\DB\QueryBuilder\IQueryBuilder; +use OCP\IConfig; use OCP\Migration\IOutput; use OCP\Migration\IRepairStep; use OCP\Security\ICrypto; @@ -29,6 +30,7 @@ class MigrateOauthTables implements IRepairStep { private ISecureRandom $random, private ITimeFactory $timeFactory, private ICrypto $crypto, + private IConfig $config, ) { } @@ -169,7 +171,12 @@ class MigrateOauthTables implements IRepairStep { $schema = new SchemaWrapper($this->db); } - $output->info('Delete clients (and their related access tokens) with the redirect_uri starting with oc:// or ending with *'); + $enableOcClients = $this->config->getSystemValueBool('oauth2.enable_oc_clients', false); + if ($enableOcClients) { + $output->info('Delete clients (and their related access tokens) with the redirect_uri starting with oc://'); + } else { + $output->info('Delete clients (and their related access tokens) with the redirect_uri starting with oc:// or ending with *'); + } // delete the access tokens $qbDeleteAccessTokens = $this->db->getQueryBuilder(); @@ -178,10 +185,12 @@ class MigrateOauthTables implements IRepairStep { ->from('oauth2_clients') ->where( $qbSelectClientId->expr()->iLike('redirect_uri', $qbDeleteAccessTokens->createNamedParameter('oc://%', IQueryBuilder::PARAM_STR)) - ) - ->orWhere( + ); + if (!$enableOcClients) { + $qbSelectClientId->orWhere( $qbSelectClientId->expr()->iLike('redirect_uri', $qbDeleteAccessTokens->createNamedParameter('%*', IQueryBuilder::PARAM_STR)) ); + } $qbDeleteAccessTokens->delete('oauth2_access_tokens') ->where( @@ -194,10 +203,12 @@ class MigrateOauthTables implements IRepairStep { $qbDeleteClients->delete('oauth2_clients') ->where( $qbDeleteClients->expr()->iLike('redirect_uri', $qbDeleteClients->createNamedParameter('oc://%', IQueryBuilder::PARAM_STR)) - ) - ->orWhere( + ); + if (!$enableOcClients) { + $qbDeleteClients->orWhere( $qbDeleteClients->expr()->iLike('redirect_uri', $qbDeleteClients->createNamedParameter('%*', IQueryBuilder::PARAM_STR)) ); + } $qbDeleteClients->executeStatement(); // Migrate legacy refresh tokens from oc diff --git a/openapi.json b/openapi.json index a82c05fd331..c7ca00d5157 100644 --- a/openapi.json +++ b/openapi.json @@ -22147,6 +22147,15 @@ "schema": { "type": "string" } + }, + { + "name": "redirect_uri", + "in": "query", + "description": "URI to redirect to after the flow (is only used for legacy ownCloud clients)", + "schema": { + "type": "string", + "default": "" + } } ], "responses": { diff --git a/tests/Core/Controller/ClientFlowLoginControllerTest.php b/tests/Core/Controller/ClientFlowLoginControllerTest.php index 1f4575208b8..591a3027e96 100644 --- a/tests/Core/Controller/ClientFlowLoginControllerTest.php +++ b/tests/Core/Controller/ClientFlowLoginControllerTest.php @@ -22,6 +22,7 @@ use OCP\AppFramework\Http\StandaloneTemplateResponse; use OCP\AppFramework\Utility\ITimeFactory; use OCP\Defaults; use OCP\EventDispatcher\IEventDispatcher; +use OCP\IConfig; use OCP\IL10N; use OCP\IRequest; use OCP\ISession; @@ -48,6 +49,7 @@ class ClientFlowLoginControllerTest extends TestCase { private ICrypto&MockObject $crypto; private IEventDispatcher&MockObject $eventDispatcher; private ITimeFactory&MockObject $timeFactory; + private IConfig&MockObject $config; private ClientFlowLoginController $clientFlowLoginController; @@ -73,6 +75,7 @@ class ClientFlowLoginControllerTest extends TestCase { $this->crypto = $this->createMock(ICrypto::class); $this->eventDispatcher = $this->createMock(IEventDispatcher::class); $this->timeFactory = $this->createMock(ITimeFactory::class); + $this->config = $this->createMock(IConfig::class); $this->clientFlowLoginController = new ClientFlowLoginController( 'core', @@ -89,6 +92,7 @@ class ClientFlowLoginControllerTest extends TestCase { $this->crypto, $this->eventDispatcher, $this->timeFactory, + $this->config, ); } @@ -163,7 +167,8 @@ class ClientFlowLoginControllerTest extends TestCase { 'serverHost' => 'https://example.com', 'oauthState' => 'OauthStateToken', 'user' => '', - 'direct' => 0 + 'direct' => 0, + 'providedRedirectUri' => '', ], 'guest' ); @@ -233,7 +238,8 @@ class ClientFlowLoginControllerTest extends TestCase { 'serverHost' => 'https://example.com', 'oauthState' => 'OauthStateToken', 'user' => '', - 'direct' => 0 + 'direct' => 0, + 'providedRedirectUri' => '', ], 'guest' ); |