Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AuthPicker: redirect oauth client to grant page #17136

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion apps/oauth2/lib/Controller/LoginRedirectorController.php
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ public function authorize($client_id,
$this->session->set('oauth.state', $state);

$targetUrl = $this->urlGenerator->linkToRouteAbsolute(
'core.ClientFlowLogin.showAuthPickerPage',
'core.ClientFlowLogin.grantPage',
[
'clientIdentifier' => $client->getClientIdentifier(),
]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ public function testAuthorize() {
->expects($this->once())
->method('linkToRouteAbsolute')
->with(
'core.ClientFlowLogin.showAuthPickerPage',
'core.ClientFlowLogin.grantPage',
[
'clientIdentifier' => 'MyClientIdentifier',
]
Expand Down
80 changes: 4 additions & 76 deletions core/Controller/ClientFlowLoginController.php
Original file line number Diff line number Diff line change
Expand Up @@ -160,89 +160,21 @@ private function stateTokenForbiddenResponse() {
}

/**
* @PublicPage
* @NoAdminRequired
* @NoCSRFRequired
* @NoSameSiteCookieRequired
* @UseSession
*
* @param string $clientIdentifier
*
* @return StandaloneTemplateResponse
*/
public function showAuthPickerPage($clientIdentifier = '') {
$clientName = $this->getClientName();
$client = null;
if ($clientIdentifier !== '') {
$client = $this->clientMapper->getByIdentifier($clientIdentifier);
$clientName = $client->getName();
}

// No valid clientIdentifier given and no valid API Request (APIRequest header not set)
$clientRequest = $this->request->getHeader('OCS-APIREQUEST');
if ($clientRequest !== 'true' && $client === null) {
return new StandaloneTemplateResponse(
$this->appName,
'error',
[
'errors' =>
[
[
'error' => 'Access Forbidden',
'hint' => 'Invalid request',
],
],
],
'guest'
);
}

public function grantPage($clientIdentifier = '') {
$stateToken = $this->random->generate(
64,
ISecureRandom::CHAR_LOWER.ISecureRandom::CHAR_UPPER.ISecureRandom::CHAR_DIGITS
);
$this->session->set(self::STATE_NAME, $stateToken);

$csp = new Http\ContentSecurityPolicy();
if ($client) {
$csp->addAllowedFormActionDomain($client->getRedirectUri());
} else {
$csp->addAllowedFormActionDomain('nc://*');
}

$response = new StandaloneTemplateResponse(
$this->appName,
'loginflow/authpicker',
[
'client' => $clientName,
'clientIdentifier' => $clientIdentifier,
'instanceName' => $this->defaults->getName(),
'urlGenerator' => $this->urlGenerator,
'stateToken' => $stateToken,
'serverHost' => $this->getServerPath(),
'oauthState' => $this->session->get('oauth.state'),
],
'guest'
);

$response->setContentSecurityPolicy($csp);
return $response;
}

/**
* @NoAdminRequired
* @NoCSRFRequired
* @NoSameSiteCookieRequired
* @UseSession
*
* @param string $stateToken
* @param string $clientIdentifier
* @return StandaloneTemplateResponse
*/
public function grantPage($stateToken = '',
$clientIdentifier = '') {
if (!$this->isValidToken($stateToken)) {
return $this->stateTokenForbiddenResponse();
}

$clientName = $this->getClientName();
$client = null;
if ($clientIdentifier !== '') {
Expand Down Expand Up @@ -373,11 +305,7 @@ public function generateAppPassword($stateToken,
/**
* @PublicPage
*/
public function apptokenRedirect(string $stateToken, string $user, string $password) {
if (!$this->isValidToken($stateToken)) {
return $this->stateTokenForbiddenResponse();
}

public function apptokenRedirect(string $user, string $password) {
try {
$token = $this->tokenProvider->getToken($password);
if ($token->getLoginName() !== $user) {
Expand Down
39 changes: 5 additions & 34 deletions core/Controller/ClientFlowLoginV2Controller.php
Original file line number Diff line number Diff line change
Expand Up @@ -109,16 +109,17 @@ public function landing(string $token): Response {
$this->session->set(self::TOKEN_NAME, $token);

return new RedirectResponse(
$this->urlGenerator->linkToRouteAbsolute('core.ClientFlowLoginV2.showAuthPickerPage')
$this->urlGenerator->linkToRouteAbsolute('core.ClientFlowLoginV2.grantPage')
);
}

/**
* @NoCSRFRequired
* @PublicPage
* @NoAdminRequired
* @UseSession
* @NoCSRFRequired
* @NoSameSiteCookieRequired
*/
public function showAuthPickerPage(): StandaloneTemplateResponse {
public function grantPage(): StandaloneTemplateResponse {
try {
$flow = $this->getFlowByLoginToken();
} catch (LoginFlowV2NotFoundException $e) {
Expand All @@ -131,36 +132,6 @@ public function showAuthPickerPage(): StandaloneTemplateResponse {
);
$this->session->set(self::STATE_NAME, $stateToken);

return new StandaloneTemplateResponse(
$this->appName,
'loginflowv2/authpicker',
[
'client' => $flow->getClientName(),
'instanceName' => $this->defaults->getName(),
'urlGenerator' => $this->urlGenerator,
'stateToken' => $stateToken,
],
'guest'
);
}

/**
* @NoAdminRequired
* @UseSession
* @NoCSRFRequired
* @NoSameSiteCookieRequired
*/
public function grantPage(string $stateToken): StandaloneTemplateResponse {
if (!$this->isValidStateToken($stateToken)) {
return $this->stateTokenForbiddenResponse();
}

try {
$flow = $this->getFlowByLoginToken();
} catch (LoginFlowV2NotFoundException $e) {
return $this->loginTokenForbiddenResponse();
}

return new StandaloneTemplateResponse(
$this->appName,
'loginflowv2/grant',
Expand Down
43 changes: 41 additions & 2 deletions core/Controller/LoginController.php
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,12 @@
use OC\Authentication\Login\Chain;
use OC\Authentication\Login\LoginData;
use OC\Authentication\WebAuthn\Manager as WebAuthnManager;
use OC\Core\Service\LoginFlowV2Service;
use OC\Security\Bruteforce\Throttler;
use OC\User\Session;
use OC_App;
use OC_Util;
use OCA\OAuth2\Db\ClientMapper;
use OCP\AppFramework\Controller;
use OCP\AppFramework\Http;
use OCP\AppFramework\Http\DataResponse;
Expand Down Expand Up @@ -83,6 +85,10 @@ class LoginController extends Controller {
private $initialStateService;
/** @var WebAuthnManager */
private $webAuthnManager;
/** @var ClientMapper */
private $clientMapper;
/** @var LoginFlowV2Service */
private $loginFlowV2Service;

public function __construct(?string $appName,
IRequest $request,
Expand All @@ -96,7 +102,9 @@ public function __construct(?string $appName,
Throttler $throttler,
Chain $loginChain,
IInitialStateService $initialStateService,
WebAuthnManager $webAuthnManager) {
WebAuthnManager $webAuthnManager,
ClientMapper $clientMapper,
LoginFlowV2Service $loginFlowV2Service) {
parent::__construct($appName, $request);
$this->userManager = $userManager;
$this->config = $config;
Expand All @@ -109,6 +117,8 @@ public function __construct(?string $appName,
$this->loginChain = $loginChain;
$this->initialStateService = $initialStateService;
$this->webAuthnManager = $webAuthnManager;
$this->clientMapper = $clientMapper;
$this->loginFlowV2Service = $loginFlowV2Service;
}

/**
Expand Down Expand Up @@ -174,8 +184,35 @@ public function showLoginForm(string $user = null, string $redirect_url = null):
$this->config->getSystemValue('login_form_autocomplete', true) === true
);

$csp = new Http\ContentSecurityPolicy();

if (!empty($redirect_url)) {
$this->initialStateService->provideInitialState('core', 'loginRedirectUrl', $redirect_url);

$clientName = null;
if (strpos($redirect_url, $this->urlGenerator->linkToRoute('core.ClientFlowLogin.grantPage')) === 0) {
parse_str(parse_url($redirect_url, PHP_URL_QUERY), $clientFlowQuery);
if (isset($clientFlowQuery['clientIdentifier'])) {
$client = $this->clientMapper->getByIdentifier($clientFlowQuery['clientIdentifier']);
$clientName = $client->getName();
$csp->addAllowedFormActionDomain($client->getRedirectUri());
} else {
$userAgent = $this->request->getHeader('USER_AGENT');
$clientName = $userAgent !== '' ? $userAgent : 'unknown';
$csp->addAllowedFormActionDomain('nc://*');
}
} elseif (strpos($redirect_url, $this->urlGenerator->linkToRoute('core.ClientFlowLoginV2.grantPage')) === 0) {
$flowV2Token = $this->session->get(ClientFlowLoginV2Controller::TOKEN_NAME);
$flowV2 = $this->loginFlowV2Service->getByLoginToken($flowV2Token);
$clientName = $flowV2->getClientName();
}

if ($clientName) {
$this->initialStateService->provideInitialState('core', 'loginGrantParams', [
'client' => Util::sanitizeHTML($clientName),
'instanceName' => Util::sanitizeHTML($this->defaults->getName()),
]);
}
}

$this->initialStateService->provideInitialState(
Expand All @@ -199,9 +236,11 @@ public function showLoginForm(string $user = null, string $redirect_url = null):
$parameters = [
'alt_login' => OC_App::getAlternativeLogIns(),
];
return new TemplateResponse(
$response = new TemplateResponse(
$this->appName, 'login', $parameters, 'guest'
);
$response->setContentSecurityPolicy($csp);
return $response;
}

/**
Expand Down
2 changes: 1 addition & 1 deletion core/js/dist/install.js

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion core/js/dist/install.js.map

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion core/js/dist/login.js

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion core/js/dist/login.js.map

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion core/js/dist/main.js

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion core/js/dist/main.js.map

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion core/js/dist/maintenance.js

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion core/js/dist/maintenance.js.map

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion core/js/dist/recommendedapps.js

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion core/js/dist/recommendedapps.js.map

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion core/js/dist/unified-search.js

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion core/js/dist/unified-search.js.map

Large diffs are not rendered by default.

6 changes: 2 additions & 4 deletions core/routes.php
Original file line number Diff line number Diff line change
Expand Up @@ -54,15 +54,13 @@
['name' => 'login#showLoginForm', 'url' => '/login', 'verb' => 'GET'],
['name' => 'login#logout', 'url' => '/logout', 'verb' => 'GET'],
// Original login flow used by all clients
['name' => 'ClientFlowLogin#showAuthPickerPage', 'url' => '/login/flow', 'verb' => 'GET'],
['name' => 'ClientFlowLogin#grantPage', 'url' => '/login/flow', 'verb' => 'GET'],
['name' => 'ClientFlowLogin#generateAppPassword', 'url' => '/login/flow', 'verb' => 'POST'],
['name' => 'ClientFlowLogin#grantPage', 'url' => '/login/flow/grant', 'verb' => 'GET'],
['name' => 'ClientFlowLogin#apptokenRedirect', 'url' => '/login/flow/apptoken', 'verb' => 'POST'],
// NG login flow used by desktop client in case of Kerberos/fancy 2fa (smart cards for example)
['name' => 'ClientFlowLoginV2#poll', 'url' => '/login/v2/poll', 'verb' => 'POST'],
['name' => 'ClientFlowLoginV2#showAuthPickerPage', 'url' => '/login/v2/flow', 'verb' => 'GET'],
['name' => 'ClientFlowLoginV2#grantPage', 'url' => '/login/v2/flow', 'verb' => 'GET'],
['name' => 'ClientFlowLoginV2#landing', 'url' => '/login/v2/flow/{token}', 'verb' => 'GET'],
['name' => 'ClientFlowLoginV2#grantPage', 'url' => '/login/v2/grant', 'verb' => 'GET'],
['name' => 'ClientFlowLoginV2#generateAppPassword', 'url' => '/login/v2/grant', 'verb' => 'POST'],
['name' => 'ClientFlowLoginV2#init', 'url' => '/login/v2', 'verb' => 'POST'],
['name' => 'TwoFactorChallenge#selectChallenge', 'url' => '/login/selectchallenge', 'verb' => 'GET'],
Expand Down
Loading