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 `<a>` tag, so returning something like `href="foo"` would result
in `<a href="foo">`.

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 <lukas@statuscode.ch>

8 files changed, 34 insertions, 37 deletions

diff --git a/core/Controller/TwoFactorChallengeController.php b/core/Controller/TwoFactorChallengeController.php
index 9f379ad30d7..af69b55173e 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 431f4c78c22..a1e626567e7 100644
--- a/core/templates/twofactorselectchallenge.php
+++ b/core/templates/twofactorselectchallenge.php
@@ -19,7 +19,7 @@
 		</ul>
 	</p>
 	<p class="two-factor-link">
-		<a class="button" <?php print_unescaped($_['logout_attribute']); ?>><?php p($l->t('Cancel log in')) ?></a>
+		<a class="button" href="<?php print_unescaped($_['logout_url']); ?>"><?php p($l->t('Cancel log in')) ?></a>
 		<?php if (!is_null($_['backupProvider'])): ?>
 		<a class="button" href="<?php p(\OC::$server->getURLGenerator()->linkToRoute('core.TwoFactorChallenge.showChallenge',
 												[
diff --git a/core/templates/twofactorshowchallenge.php b/core/templates/twofactorshowchallenge.php
index ec760ead7c7..fbfeeb4255a 100644
--- a/core/templates/twofactorshowchallenge.php
+++ b/core/templates/twofactorshowchallenge.php
@@ -22,7 +22,7 @@ $template = $_['template'];
 	<?php endif; ?>
 	<?php print_unescaped($template); ?>
 	<p class="two-factor-link">
-		<a class="button" <?php print_unescaped($_['logout_attribute']); ?>><?php p($l->t('Cancel log in')) ?></a>
+		<a class="button" href="<?php print_unescaped($_['logout_url']); ?>"><?php p($l->t('Cancel log in')) ?></a>
 		<?php if (!is_null($_['backupProvider'])): ?>
 		<a class="button" href="<?php p(\OC::$server->getURLGenerator()->linkToRoute('core.TwoFactorChallenge.showChallenge',
 												[
diff --git a/lib/private/NavigationManager.php b/lib/private/NavigationManager.php
index b854b44b340..ffeaca2b81e 100644
--- a/lib/private/NavigationManager.php
+++ b/lib/private/NavigationManager.php
@@ -187,18 +187,18 @@ class NavigationManager implements INavigationManager {
 				'icon' => $this->urlGenerator->imagePath('settings', 'admin.svg'),
 			]);
 
-			// Logout
-			$this->add([
-				'type' => 'settings',
-				'id' => 'logout',
-				'order' => 99999,
-				'href' => $this->urlGenerator->linkToRouteAbsolute(
-					'core.login.logout',
-					['requesttoken' => \OCP\Util::callRegister()]
-				),
-				'name' => $l->t('Log out'),
-				'icon' => $this->urlGenerator->imagePath('core', 'actions/logout.svg'),
-			]);
+			$logoutUrl = \OC_User::getLogoutUrl();
+			if($logoutUrl !== '') {
+				// Logout
+				$this->add([
+					'type' => 'settings',
+					'id' => 'logout',
+					'order' => 99999,
+					'href' => $logoutUrl,
+					'name' => $l->t('Log out'),
+					'icon' => $this->urlGenerator->imagePath('core', 'actions/logout.svg'),
+				]);
+			}
 
 			if ($this->isSubadmin()) {
 				// User management
diff --git a/lib/private/legacy/user.php b/lib/private/legacy/user.php
index 210e428a2e0..97f302c826e 100644
--- a/lib/private/legacy/user.php
+++ b/lib/private/legacy/user.php
@@ -281,16 +281,14 @@ class OC_User {
 	}
 
 	/**
-	 * Supplies an attribute to the logout hyperlink. The default behaviour
-	 * is to return an href with '?logout=true' appended. However, it can
-	 * supply any attribute(s) which are valid for <a>.
+	 * Returns the current logout URL valid for the currently logged-in user
 	 *
-	 * @return string with one or more HTML attributes.
+	 * @return string
 	 */
-	public static function getLogoutAttribute() {
+	public static function getLogoutUrl() {
 		$backend = self::findFirstActiveUsedBackend();
 		if ($backend) {
-			return $backend->getLogoutAttribute();
+			return $backend->getLogoutUrl();
 		}
 
 		$logoutUrl = \OC::$server->getURLGenerator()->linkToRouteAbsolute(
@@ -300,7 +298,7 @@ class OC_User {
 			]
 		);
 
-		return 'href="'.$logoutUrl.'"';
+		return $logoutUrl;
 	}
 
 	/**
diff --git a/lib/public/Authentication/IApacheBackend.php b/lib/public/Authentication/IApacheBackend.php
index 908bc5ace3d..7d43d438cbb 100644
--- a/lib/public/Authentication/IApacheBackend.php
+++ b/lib/public/Authentication/IApacheBackend.php
@@ -39,21 +39,20 @@ namespace OCP\Authentication;
 interface IApacheBackend {
 
 	/**
-	 * In case the user has been authenticated by Apache true is returned.
+	 * In case the user has been authenticated by a module true is returned.
 	 *
-	 * @return boolean whether Apache reports a user as currently logged in.
+	 * @return boolean whether the module reports a user as currently logged in.
 	 * @since 6.0.0
 	 */
 	public function isSessionActive();
 
 	/**
-	 * Creates an attribute which is added to the logout hyperlink. It can
-	 * supply any attribute(s) which are valid for <a>.
+	 * 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 bef343f9043..ed6452316ff 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' => '<html/>',
 			'redirect_url' => '/re/dir/ect/url',
 			'error_message' => null,
diff --git a/tests/lib/NavigationManagerTest.php b/tests/lib/NavigationManagerTest.php
index de432e1eaf2..edab6070f9e 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,