Skip to content

Commit dffa90f

Browse files
julien-ncjulien-nc
committed
feat(oauth2): preserve query params and fragments in redirect URL, support legacy redirect URL
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
1 parent 00238c3 commit dffa90f

4 files changed

Lines changed: 108 additions & 10 deletions

File tree

‎apps/oauth2/lib/Controller/LoginRedirectorController.php‎

Lines changed: 37 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
#[OpenAPI(scope: OpenAPI::SCOPE_DEFAULT)]
3131
class LoginRedirectorController extends Controller {
3232
private const PKCE_STRING_PATTERN = '/^[A-Za-z0-9._~-]{43,128}$/';
33+
private const LEGACY_LOCALHOST_REDIRECT_PATTERN = '/^http:\/\/localhost:[0-9]+$/';
3334

3435
/**
3536
* @param string $appName
@@ -53,6 +54,38 @@ public function __construct(
5354
parent::__construct($appName, $request);
5455
}
5556

57+
private function buildErrorRedirectResponse(
58+
string $registeredRedirectUri,
59+
string $providedRedirectUri,
60+
string $error,
61+
string $state,
62+
?string $errorDescription = null,
63+
): RedirectResponse {
64+
$redirectUri = $registeredRedirectUri;
65+
$enableOcClients = $this->config->getSystemValueBool('oauth2.enable_oc_clients', false);
66+
if ($enableOcClients && $redirectUri === 'http://localhost:*' && preg_match(self::LEGACY_LOCALHOST_REDIRECT_PATTERN, $providedRedirectUri) === 1) {
67+
$redirectUri = $providedRedirectUri;
68+
}
69+
70+
$fragment = '';
71+
$fragmentPosition = strpos($redirectUri, '#');
72+
if ($fragmentPosition !== false) {
73+
$fragment = substr($redirectUri, $fragmentPosition);
74+
$redirectUri = substr($redirectUri, 0, $fragmentPosition);
75+
}
76+
77+
$params = [
78+
'error' => $error,
79+
];
80+
if ($errorDescription !== null) {
81+
$params['error_description'] = $errorDescription;
82+
}
83+
$params['state'] = $state;
84+
85+
$separator = str_contains($redirectUri, '?') ? '&' : '?';
86+
return new RedirectResponse($redirectUri . $separator . http_build_query($params) . $fragment);
87+
}
88+
5689
/**
5790
* Authorize the user
5891
*
@@ -86,27 +119,22 @@ public function authorize($client_id,
86119
}
87120

88121
if ($response_type !== 'code') {
89-
//Fail
90-
$url = $client->getRedirectUri() . '?error=unsupported_response_type&state=' . \urlencode($state);
91-
return new RedirectResponse($url);
122+
return $this->buildErrorRedirectResponse($client->getRedirectUri(), $redirect_uri, 'unsupported_response_type', $state);
92123
}
93124

94125
if ($code_challenge === '' && $code_challenge_method !== '') {
95-
$url = $client->getRedirectUri() . '?error=invalid_request&error_description=code_challenge+required&state=' . \urlencode($state);
96-
return new RedirectResponse($url);
126+
return $this->buildErrorRedirectResponse($client->getRedirectUri(), $redirect_uri, 'invalid_request', $state, 'code_challenge required');
97127
}
98128

99129
if ($code_challenge !== '' && preg_match(self::PKCE_STRING_PATTERN, $code_challenge) !== 1) {
100-
$url = $client->getRedirectUri() . '?error=invalid_request&error_description=Invalid+code_challenge&state=' . \urlencode($state);
101-
return new RedirectResponse($url);
130+
return $this->buildErrorRedirectResponse($client->getRedirectUri(), $redirect_uri, 'invalid_request', $state, 'Invalid code_challenge');
102131
}
103132

104133
$effectiveCodeChallengeMethod = $code_challenge_method === '' && $code_challenge !== ''
105134
? 'plain'
106135
: $code_challenge_method;
107136
if ($effectiveCodeChallengeMethod !== '' && $effectiveCodeChallengeMethod !== 'S256') {
108-
$url = $client->getRedirectUri() . '?error=invalid_request&error_description=Transform+algorithm+not+supported&state=' . \urlencode($state);
109-
return new RedirectResponse($url);
137+
return $this->buildErrorRedirectResponse($client->getRedirectUri(), $redirect_uri, 'invalid_request', $state, 'Transform algorithm not supported');
110138
}
111139

112140
$enableOcClients = $this->config->getSystemValueBool('oauth2.enable_oc_clients', false);

‎apps/oauth2/tests/Controller/LoginRedirectorControllerTest.php‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -176,6 +176,28 @@ public function testAuthorizeWrongResponseType(): void {
176176
$this->assertEquals($expected, $this->loginRedirectorController->authorize('MyClientId', 'MyState', 'wrongcode'));
177177
}
178178

179+
public function testAuthorizeWrongResponseTypePreservesExistingQuery(): void {
180+
$client = new Client();
181+
$client->setClientIdentifier('MyClientIdentifier');
182+
$client->setRedirectUri('http://foo.bar?hello=world');
183+
$this->clientMapper
184+
->expects($this->once())
185+
->method('getByIdentifier')
186+
->with('MyClientId')
187+
->willReturn($client);
188+
$this->session
189+
->expects($this->never())
190+
->method('set');
191+
$this->config
192+
->expects($this->once())
193+
->method('getSystemValueBool')
194+
->with('oauth2.enable_oc_clients', false)
195+
->willReturn(false);
196+
197+
$expected = new RedirectResponse('http://foo.bar?hello=world&error=unsupported_response_type&state=MyState');
198+
$this->assertEquals($expected, $this->loginRedirectorController->authorize('MyClientId', 'MyState', 'wrongcode'));
199+
}
200+
179201
public function testAuthorizeRejectsCodeChallengeMethodWithoutChallenge(): void {
180202
$client = new Client();
181203
$client->setClientIdentifier('MyClientIdentifier');
@@ -207,10 +229,38 @@ public function testAuthorizeRejectsPkceWithoutMethodBecausePlainIsUnsupported()
207229
->method('set');
208230

209231
$codeChallenge = str_repeat('a', 43);
232+
$this->config
233+
->expects($this->once())
234+
->method('getSystemValueBool')
235+
->with('oauth2.enable_oc_clients', false)
236+
->willReturn(false);
210237
$expected = new RedirectResponse('http://foo.bar?error=invalid_request&error_description=Transform+algorithm+not+supported&state=MyState');
211238
$this->assertEquals($expected, $this->loginRedirectorController->authorize('MyClientId', 'MyState', 'code', '', $codeChallenge));
212239
}
213240

241+
public function testAuthorizeRejectsPkceWithoutMethodForLegacyOcClientUsingProvidedRedirectUri(): void {
242+
$client = new Client();
243+
$client->setClientIdentifier('MyClientIdentifier');
244+
$client->setRedirectUri('http://localhost:*');
245+
$this->clientMapper
246+
->expects($this->once())
247+
->method('getByIdentifier')
248+
->with('MyClientId')
249+
->willReturn($client);
250+
$this->session
251+
->expects($this->never())
252+
->method('set');
253+
$this->config
254+
->expects($this->once())
255+
->method('getSystemValueBool')
256+
->with('oauth2.enable_oc_clients', false)
257+
->willReturn(true);
258+
259+
$codeChallenge = str_repeat('a', 43);
260+
$expected = new RedirectResponse('http://localhost:30000?error=invalid_request&error_description=Transform+algorithm+not+supported&state=MyState');
261+
$this->assertEquals($expected, $this->loginRedirectorController->authorize('MyClientId', 'MyState', 'code', 'http://localhost:30000', $codeChallenge));
262+
}
263+
214264
public function testAuthorizeRejectsUnsupportedCodeChallengeMethod(): void {
215265
$client = new Client();
216266
$client->setClientIdentifier('MyClientIdentifier');
@@ -225,6 +275,11 @@ public function testAuthorizeRejectsUnsupportedCodeChallengeMethod(): void {
225275
->method('set');
226276

227277
$codeChallenge = str_repeat('a', 43);
278+
$this->config
279+
->expects($this->once())
280+
->method('getSystemValueBool')
281+
->with('oauth2.enable_oc_clients', false)
282+
->willReturn(false);
228283
$expected = new RedirectResponse('http://foo.bar?error=invalid_request&error_description=Transform+algorithm+not+supported&state=MyState');
229284
$this->assertEquals($expected, $this->loginRedirectorController->authorize('MyClientId', 'MyState', 'code', '', $codeChallenge, 'plain'));
230285
}
@@ -241,6 +296,11 @@ public function testAuthorizeRejectsInvalidCodeChallengeFormat(): void {
241296
$this->session
242297
->expects($this->never())
243298
->method('set');
299+
$this->config
300+
->expects($this->once())
301+
->method('getSystemValueBool')
302+
->with('oauth2.enable_oc_clients', false)
303+
->willReturn(false);
244304

245305
$expected = new RedirectResponse('http://foo.bar?error=invalid_request&error_description=Invalid+code_challenge&state=MyState');
246306
$this->assertEquals($expected, $this->loginRedirectorController->authorize('MyClientId', 'MyState', 'code', '', 'short', 'S256'));

‎core/Controller/ClientFlowLoginController.php‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -324,6 +324,13 @@ public function generateAppPassword(
324324
$redirectUri = $providedRedirectUri;
325325
}
326326

327+
$fragment = '';
328+
$fragmentPosition = strpos($redirectUri, '#');
329+
if ($fragmentPosition !== false) {
330+
$fragment = substr($redirectUri, $fragmentPosition);
331+
$redirectUri = substr($redirectUri, 0, $fragmentPosition);
332+
}
333+
327334
if (parse_url($redirectUri, PHP_URL_QUERY)) {
328335
$redirectUri .= '&';
329336
} else {
@@ -335,6 +342,7 @@ public function generateAppPassword(
335342
urlencode($this->session->get('oauth.state')),
336343
urlencode($code)
337344
);
345+
$redirectUri .= $fragment;
338346
$this->session->remove('oauth.state');
339347
} else {
340348
$redirectUri = 'nc://login/server:' . $this->getServerPath() . '&user:' . urlencode($loginName) . '&password:' . urlencode($token);

‎tests/Core/Controller/ClientFlowLoginControllerTest.php‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,8 @@
1414
use OC\Authentication\Token\IProvider;
1515
use OC\Authentication\Token\IToken;
1616
use OC\Core\Controller\ClientFlowLoginController;
17-
use OCA\OAuth2\Db\AccessTokenMapper;
1817
use OCA\OAuth2\Db\AccessToken;
18+
use OCA\OAuth2\Db\AccessTokenMapper;
1919
use OCA\OAuth2\Db\Client;
2020
use OCA\OAuth2\Db\ClientMapper;
2121
use OCP\AppFramework\Http;
@@ -423,6 +423,8 @@ public function testGeneratePasswordWithPassword(): void {
423423
* @testWith
424424
* ["https://example.com/redirect.php", "https://example.com/redirect.php?state=MyOauthState&code=MyAccessCode"]
425425
* ["https://example.com/redirect.php?hello=world", "https://example.com/redirect.php?hello=world&state=MyOauthState&code=MyAccessCode"]
426+
* ["https://example.com/redirect.php#target", "https://example.com/redirect.php?state=MyOauthState&code=MyAccessCode#target"]
427+
* ["https://example.com/redirect.php?hello=world#target", "https://example.com/redirect.php?hello=world&state=MyOauthState&code=MyAccessCode#target"]
426428
*
427429
*/
428430
public function testGeneratePasswordWithPasswordForOauthClient($redirectUri, $redirectUrl): void {

0 commit comments

Comments
 (0)