Merge pull request #6177 from nextcloud/properly-add-slo-url

Properly allow \OCP\Authentication\IApacheBackend to specify logout URL

core/Controller/TwoFactorChallengeController.php

@@ -71,8 +71,8 @@ class TwoFactorChallengeController extends Controller {
 	/**
 	 * @return string
 	 */
-	protected function getLogoutAttribute() {
+	protected function getLogoutUrl() {
-		return OC_User::getLogoutAttribute();
+		return OC_User::getLogoutUrl($this->urlGenerator);
 	}
 
 	/**
@@ -91,7 +91,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');
 	}
@@ -133,7 +133,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(),
 		];

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',
 												[

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',
 												[

lib/private/NavigationManager.php

@@ -187,18 +187,18 @@ class NavigationManager implements INavigationManager {
 				'icon' => $this->urlGenerator->imagePath('settings', 'admin.svg'),
 			]);
 
-			// Logout
+			$logoutUrl = \OC_User::getLogoutUrl($this->urlGenerator);
-			$this->add([
+			if($logoutUrl !== '') {
-				'type' => 'settings',
+				// Logout
-				'id' => 'logout',
+				$this->add([
-				'order' => 99999,
+					'type' => 'settings',
-				'href' => $this->urlGenerator->linkToRouteAbsolute(
+					'id' => 'logout',
-					'core.login.logout',
+					'order' => 99999,
-					['requesttoken' => \OCP\Util::callRegister()]
+					'href' => $logoutUrl,
-				),
+					'name' => $l->t('Log out'),
-				'name' => $l->t('Log out'),
+					'icon' => $this->urlGenerator->imagePath('core', 'actions/logout.svg'),
-				'icon' => $this->urlGenerator->imagePath('core', 'actions/logout.svg'),
+				]);
-			]);
+			}
 
 			if ($this->isSubadmin()) {
 				// User management

lib/private/legacy/user.php

@@ -281,26 +281,25 @@ class OC_User {
 	}
 
 	/**
-	 * Supplies an attribute to the logout hyperlink. The default behaviour
+	 * Returns the current logout URL valid for the currently logged-in user
-	 * is to return an href with '?logout=true' appended. However, it can
-	 * supply any attribute(s) which are valid for <a>.
 	 *
-	 * @return string with one or more HTML attributes.
+	 * @param \OCP\IURLGenerator $urlGenerator
+	 * @return string
 	 */
-	public static function getLogoutAttribute() {
+	public static function getLogoutUrl(\OCP\IURLGenerator $urlGenerator) {
 		$backend = self::findFirstActiveUsedBackend();
 		if ($backend) {
-			return $backend->getLogoutAttribute();
+			return $backend->getLogoutUrl();
 		}
 
-		$logoutUrl = \OC::$server->getURLGenerator()->linkToRouteAbsolute(
+		$logoutUrl = $urlGenerator->linkToRouteAbsolute(
 			'core.login.logout',
 			[
 				'requesttoken' => \OCP\Util::callRegister(),
 			]
 		);
 
-		return 'href="'.$logoutUrl.'"';
+		return $logoutUrl;
 	}
 
 	/**

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
+	 * Gets the current logout URL
-	 * supply any attribute(s) which are valid for <a>.
 	 *
-	 * @return string with one or more HTML attributes.
+	 * @return string
-	 * @since 6.0.0
+	 * @since 12.0.3
 	 */
-	public function getLogoutAttribute();
+	public function getLogoutUrl();
 
 	/**
 	 * Return the id of the current user

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,

tests/lib/NavigationManagerTest.php

@@ -217,6 +217,16 @@ class NavigationManagerTest extends TestCase {
 		$this->urlGenerator->expects($this->any())->method('linkToRoute')->willReturnCallback(function() {
 			return "/apps/test/";
 		});
+		$this->urlGenerator
+			->expects($this->once())
+			->method('linkToRouteAbsolute')
+			->with(
+				'core.login.logout',
+				[
+					'requesttoken' => \OCP\Util::callRegister(),
+				]
+			)
+			->willReturn('https://example.com/logout');
 		$user = $this->createMock(IUser::class);
 		$user->expects($this->any())->method('getUID')->willReturn('user001');
 		$this->userSession->expects($this->any())->method('getUser')->willReturn($user);
@@ -260,7 +270,7 @@ class NavigationManagerTest extends TestCase {
 			[
 				'id' => 'logout',
 				'order' => 99999,
-				'href' => null,
+				'href' => 'https://example.com/logout',
 				'icon' => '/apps/core/img/actions/logout.svg',
 				'name' => 'Log out',
 				'active' => false,