From a04feff9a780d77ca172ba7558a7d0cc4e01dc36 Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Fri, 18 Aug 2017 12:16:43 +0200 Subject: [PATCH] Properly allow \OCP\Authentication\IApacheBackend to specify logout URL Any `\OCP\Authentication\IApacheBackend` previously had to implement `getLogoutAttribute` which returns a string. This string is directly injected into the logout `` tag, so returning something like `href="foo"` would result in ``. This is rather error prone and also in Nextcloud 12 broken as the logout entry has been moved with 054e161eb5f4a5c5c13ee322ae8e93ce66f01b13 inside the navigation manager where one cannot simply inject attributes. Thus this feature is broken in Nextcloud 12 which effectively leads to the bug described at nextcloud/user_saml#112, people cannot logout anymore when using SAML using SLO. Basically in case of SAML you have a SLO url which redirects you to the IdP and properly logs you out there as well. Instead of monkey patching the Navigation manager I decided to instead change `\OCP\Authentication\IApacheBackend` to use `\OCP\Authentication\IApacheBackend::getLogoutUrl` instead where it can return a string with the appropriate logout URL. Since this functionality is only prominently used in the SAML plugin. Any custom app would need a small change but I'm not aware of any and there's simply no way to fix this properly otherwise. Signed-off-by: Lukas Reschke --- .../TwoFactorChallengeController.php | 8 +++---- core/templates/twofactorselectchallenge.php | 2 +- core/templates/twofactorshowchallenge.php | 2 +- lib/private/NavigationManager.php | 24 +++++++++---------- lib/private/legacy/user.php | 12 ++++------ lib/public/Authentication/IApacheBackend.php | 13 +++++----- .../TwoFactorChallengeControllerTest.php | 8 +++---- tests/lib/NavigationManagerTest.php | 2 +- 8 files changed, 34 insertions(+), 37 deletions(-) diff --git a/core/Controller/TwoFactorChallengeController.php b/core/Controller/TwoFactorChallengeController.php index 9f379ad30d..af69b55173 100644 --- a/core/Controller/TwoFactorChallengeController.php +++ b/core/Controller/TwoFactorChallengeController.php @@ -69,8 +69,8 @@ class TwoFactorChallengeController extends Controller { /** * @return string */ - protected function getLogoutAttribute() { - return OC_User::getLogoutAttribute(); + protected function getLogoutUrl() { + return OC_User::getLogoutUrl(); } /** @@ -89,7 +89,7 @@ class TwoFactorChallengeController extends Controller { 'providers' => $providers, 'backupProvider' => $backupProvider, 'redirect_url' => $redirect_url, - 'logout_attribute' => $this->getLogoutAttribute(), + 'logout_url' => $this->getLogoutUrl(), ]; return new TemplateResponse($this->appName, 'twofactorselectchallenge', $data, 'guest'); } @@ -131,7 +131,7 @@ class TwoFactorChallengeController extends Controller { 'error_message' => $errorMessage, 'provider' => $provider, 'backupProvider' => $backupProvider, - 'logout_attribute' => $this->getLogoutAttribute(), + 'logout_url' => $this->getLogoutUrl(), 'redirect_url' => $redirect_url, 'template' => $tmpl->fetchPage(), ]; diff --git a/core/templates/twofactorselectchallenge.php b/core/templates/twofactorselectchallenge.php index 431f4c78c2..a1e626567e 100644 --- a/core/templates/twofactorselectchallenge.php +++ b/core/templates/twofactorselectchallenge.php @@ -19,7 +19,7 @@

- >t('Cancel log in')) ?> + t('Cancel log in')) ?> - >t('Cancel log in')) ?> + t('Cancel log in')) ?> . + * Gets the current logout URL * - * @return string with one or more HTML attributes. - * @since 6.0.0 + * @return string + * @since 12.0.3 */ - public function getLogoutAttribute(); + public function getLogoutUrl(); /** * Return the id of the current user diff --git a/tests/Core/Controller/TwoFactorChallengeControllerTest.php b/tests/Core/Controller/TwoFactorChallengeControllerTest.php index bef343f904..ed6452316f 100644 --- a/tests/Core/Controller/TwoFactorChallengeControllerTest.php +++ b/tests/Core/Controller/TwoFactorChallengeControllerTest.php @@ -76,10 +76,10 @@ class TwoFactorChallengeControllerTest extends TestCase { $this->session, $this->urlGenerator, ]) - ->setMethods(['getLogoutAttribute']) + ->setMethods(['getLogoutUrl']) ->getMock(); $this->controller->expects($this->any()) - ->method('getLogoutAttribute') + ->method('getLogoutUrl') ->willReturn('logoutAttribute'); } @@ -106,7 +106,7 @@ class TwoFactorChallengeControllerTest extends TestCase { 'providers' => $providers, 'backupProvider' => 'backup', 'redirect_url' => '/some/url', - 'logout_attribute' => 'logoutAttribute', + 'logout_url' => 'logoutAttribute', ], 'guest'); $this->assertEquals($expected, $this->controller->selectChallenge('/some/url')); @@ -155,7 +155,7 @@ class TwoFactorChallengeControllerTest extends TestCase { 'error' => true, 'provider' => $provider, 'backupProvider' => $backupProvider, - 'logout_attribute' => 'logoutAttribute', + 'logout_url' => 'logoutAttribute', 'template' => '', 'redirect_url' => '/re/dir/ect/url', 'error_message' => null, diff --git a/tests/lib/NavigationManagerTest.php b/tests/lib/NavigationManagerTest.php index de432e1eaf..edab6070f9 100644 --- a/tests/lib/NavigationManagerTest.php +++ b/tests/lib/NavigationManagerTest.php @@ -260,7 +260,7 @@ class NavigationManagerTest extends TestCase { [ 'id' => 'logout', 'order' => 99999, - 'href' => null, + 'href' => \OC_User::getLogoutUrl(), 'icon' => '/apps/core/img/actions/logout.svg', 'name' => 'Log out', 'active' => false,