From 203f74449c23ab78d0e35bd6fcd026438fa08bac Mon Sep 17 00:00:00 2001 From: Sergey Nikolaev Date: Sat, 19 Sep 2020 21:14:45 +0000 Subject: [PATCH] remove authPickerPage (v1 and v2) --- .../Controller/LoginRedirectorController.php | 2 +- .../LoginRedirectorControllerTest.php | 2 +- core/Controller/ClientFlowLoginController.php | 113 ++++------------ .../ClientFlowLoginV2Controller.php | 39 +----- core/Controller/LoginController.php | 43 ++++-- core/routes.php | 6 +- .../components/login/AppTokenLoginForm.vue | 112 ++++++++++++++++ core/src/views/Login.vue | 28 +++- core/templates/loginflow/authpicker.php | 66 --------- core/templates/loginflow/grant.php | 6 + core/templates/loginflowv2/authpicker.php | 50 ------- .../ClientFlowLoginControllerTest.php | 125 ------------------ .../ClientFlowLoginV2ControllerTest.php | 45 +------ tests/Core/Controller/LoginControllerTest.php | 10 +- 14 files changed, 217 insertions(+), 430 deletions(-) create mode 100644 core/src/components/login/AppTokenLoginForm.vue delete mode 100644 core/templates/loginflow/authpicker.php delete mode 100644 core/templates/loginflowv2/authpicker.php diff --git a/apps/oauth2/lib/Controller/LoginRedirectorController.php b/apps/oauth2/lib/Controller/LoginRedirectorController.php index 4254c9880bc8c..49b4de09373d3 100644 --- a/apps/oauth2/lib/Controller/LoginRedirectorController.php +++ b/apps/oauth2/lib/Controller/LoginRedirectorController.php @@ -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(), ] diff --git a/apps/oauth2/tests/Controller/LoginRedirectorControllerTest.php b/apps/oauth2/tests/Controller/LoginRedirectorControllerTest.php index c3dfa4114b312..9ca99bd4a2e9e 100644 --- a/apps/oauth2/tests/Controller/LoginRedirectorControllerTest.php +++ b/apps/oauth2/tests/Controller/LoginRedirectorControllerTest.php @@ -90,7 +90,7 @@ public function testAuthorize() { ->expects($this->once()) ->method('linkToRouteAbsolute') ->with( - 'core.ClientFlowLogin.showAuthPickerPage', + 'core.ClientFlowLogin.grantPage', [ 'clientIdentifier' => 'MyClientIdentifier', ] diff --git a/core/Controller/ClientFlowLoginController.php b/core/Controller/ClientFlowLoginController.php index c88c44f142370..084b88d66d958 100644 --- a/core/Controller/ClientFlowLoginController.php +++ b/core/Controller/ClientFlowLoginController.php @@ -161,101 +161,21 @@ private function stateTokenForbiddenResponse() { } /** - * @PublicPage + * @NoAdminRequired * @NoCSRFRequired + * @NoSameSiteCookieRequired * @UseSession * * @param string $clientIdentifier - * - * @return StandaloneTemplateResponse|RedirectResponse + * @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); - $oauthState = $this->session->get('oauth.state'); - if (!empty($oauthState)) { - $targetUrl = $this->urlGenerator->linkToRoute( - 'core.ClientFlowLogin.grantPage', - [ - 'stateToken' => $stateToken, - 'clientIdentifier' => $clientIdentifier, - 'oauthState' => $oauthState - ] - ); - return new RedirectResponse($targetUrl); - } - - $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(), - ], - '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 !== '') { @@ -386,11 +306,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) { @@ -410,7 +326,24 @@ public function apptokenRedirect(string $stateToken, string $user, string $passw } $redirectUri = 'nc://login/server:' . $this->getServerPath() . '&user:' . urlencode($user) . '&password:' . urlencode($password); - return new Http\RedirectResponse($redirectUri); + + $csp = new Http\ContentSecurityPolicy(); + $csp->addAllowedFormActionDomain('nc://*'); + + $response = new StandaloneTemplateResponse( + $this->appName, + 'loginflow/grant', + [ + 'client' => $token->getName(), + 'instanceName' => $this->defaults->getName(), + 'urlGenerator' => $this->urlGenerator, + 'redirectUri' => $redirectUri, + ], + 'guest' + ); + + $response->setContentSecurityPolicy($csp); + return $response; } private function getServerPath(): string { diff --git a/core/Controller/ClientFlowLoginV2Controller.php b/core/Controller/ClientFlowLoginV2Controller.php index 103d659ecd2ba..f3c96a8674174 100644 --- a/core/Controller/ClientFlowLoginV2Controller.php +++ b/core/Controller/ClientFlowLoginV2Controller.php @@ -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) { @@ -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', diff --git a/core/Controller/LoginController.php b/core/Controller/LoginController.php index 9cf3a678c78f2..2cff83df7263d 100644 --- a/core/Controller/LoginController.php +++ b/core/Controller/LoginController.php @@ -36,6 +36,7 @@ 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; @@ -86,6 +87,8 @@ class LoginController extends Controller { private $webAuthnManager; /** @var ClientMapper */ private $clientMapper; + /** @var LoginFlowV2Service */ + private $loginFlowV2Service; public function __construct(?string $appName, IRequest $request, @@ -100,7 +103,8 @@ public function __construct(?string $appName, Chain $loginChain, IInitialStateService $initialStateService, WebAuthnManager $webAuthnManager, - ClientMapper $clientMapper) { + ClientMapper $clientMapper, + LoginFlowV2Service $loginFlowV2Service) { parent::__construct($appName, $request); $this->userManager = $userManager; $this->config = $config; @@ -114,6 +118,7 @@ public function __construct(?string $appName, $this->initialStateService = $initialStateService; $this->webAuthnManager = $webAuthnManager; $this->clientMapper = $clientMapper; + $this->loginFlowV2Service = $loginFlowV2Service; } /** @@ -182,19 +187,11 @@ public function showLoginForm(string $user = null, string $redirect_url = null): if (!empty($redirect_url)) { $this->initialStateService->provideInitialState('core', 'loginRedirectUrl', $redirect_url); - $grant_url = $this->urlGenerator->linkToRoute('core.ClientFlowLogin.grantPage'); - if (strpos($redirect_url, $grant_url) === 0) { - parse_str(parse_url($redirect_url, PHP_URL_QUERY), $grant_query); - if (empty($grant_query['clientIdentifier'])) { - $userAgent = $this->request->getHeader('USER_AGENT'); - $clientName = $userAgent !== '' ? $userAgent : 'unknown'; - } else { - $client = $this->clientMapper->getByIdentifier($grant_query['clientIdentifier']); - $clientName = $client->getName(); - } + $client = $this->getClientName($redirect_url); + if ($client) { $this->initialStateService->provideInitialState('core', 'loginGrantParams', [ 'client' => Util::sanitizeHTML($clientName), - 'instanceName' => Util::sanitizeHTML($this->defaults->getName()) + 'instanceName' => Util::sanitizeHTML($this->defaults->getName()), ]); } } @@ -393,4 +390,26 @@ public function confirmPassword($password) { $this->session->set('last-password-confirm', $confirmTimestamp); return new DataResponse(['lastLogin' => $confirmTimestamp], Http::STATUS_OK); } + + /** + * @param string $redirect_url + * @return null|string + */ + private function getClientName($redirect_url) { + 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']); + return $client->getName(); + } else { + $userAgent = $this->request->getHeader('USER_AGENT'); + return $userAgent !== '' ? $userAgent : 'unknown'; + } + } else if (strpos($redirect_url, $this->urlGenerator->linkToRoute('core.ClientFlowLoginV2.grantPage')) === 0) { + $flowV2Token = $this->session->get(ClientFlowLoginV2Controller::TOKEN_NAME); + $flowV2 = $this->loginFlowV2Service->getByLoginToken($flowV2Token); + return $flowV2->getClientName(); + } + return null; + } } diff --git a/core/routes.php b/core/routes.php index 9fa378dc1d8ed..2e224dc3aa426 100644 --- a/core/routes.php +++ b/core/routes.php @@ -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'], diff --git a/core/src/components/login/AppTokenLoginForm.vue b/core/src/components/login/AppTokenLoginForm.vue new file mode 100644 index 0000000000000..ac730eab01313 --- /dev/null +++ b/core/src/components/login/AppTokenLoginForm.vue @@ -0,0 +1,112 @@ + + + + + diff --git a/core/src/views/Login.vue b/core/src/views/Login.vue index d4d87c57f9853..a7ddded492516 100644 --- a/core/src/views/Login.vue +++ b/core/src/views/Login.vue @@ -27,7 +27,7 @@

{{ t('core', 'If you are not trying to set up a new device or app, someone is trying to trick you into granting them access to your data. In this case do not proceed and instead contact your system administrator.') }}

-
{{ t('core', 'Log in with a device') }} +
+ + {{ t('core', 'Alternative log in using app token') }} +
+