fix: remove long-unused desktop option

- Remove desktop option - Remove handling code - Remove tests - Add repair step to remove possible config DB entry Signed-off-by: Git'Fellow <[email protected]> Signed-off-by: Arthur Schiwon <[email protected]>
nextcloud · May 30, 2024 · 1ca53c6 · 1ca53c6
1 parent 6ba3fba
commit 1ca53c6
Show file tree

Hide file tree

Showing 7 changed files with 38 additions and 43 deletions.
diff --git a/appinfo/app.php b/appinfo/app.php
@@ -110,31 +110,6 @@
 	$redirectSituation = true;
 }
 
-// If a request to OCS or remote.php is sent by the official desktop clients it can
-// be intercepted as it supports SAML. All other clients don't yet and thus we
-// require the usage of application specific passwords there.
-//
-// However, it is an opt-in setting to use SAML for the desktop clients. For better
-// UX (users don't have to reauthenticate) we default to disallow the access via
-// SAML at the moment.
-$useSamlForDesktopClients = $config->getAppValue('user_saml', 'general-use_saml_auth_for_desktop', '0');
-if ($useSamlForDesktopClients === '1') {
-	$currentUrl = substr(explode('?', $request->getRequestUri(), 2)[0], strlen(\OC::$WEBROOT));
-	if (substr($currentUrl, 0, 12) === '/remote.php/' || substr($currentUrl, 0, 5) === '/ocs/') {
-		if (!$userSession->isLoggedIn() && $request->isUserAgent([\OCP\IRequest::USER_AGENT_CLIENT_DESKTOP])) {
-			$redirectSituation = true;
-
-			if (preg_match('/^.*\/(\d+\.\d+\.\d+).*$/', $request->getHeader('USER_AGENT'), $matches) === 1) {
-				$versionstring = $matches[1];
-
-				if (version_compare($versionstring, '2.5.0', '>=') === true) {
-					$redirectSituation = false;
-				}
-			}
-		}
-	}
-}
-
 $multipleUserBackEnds = $samlSettings->allowMultipleUserBackEnds();
 $configuredIdps = $samlSettings->getListOfIdps();
 $showLoginOptions = ($multipleUserBackEnds || count($configuredIdps) > 1) && $type === 'saml';

diff --git a/appinfo/info.xml b/appinfo/info.xml
@@ -43,6 +43,9 @@ While theoretically any other authentication provider implementing either one of
 		<post-migration>
 			<step>OCA\User_SAML\Migration\RememberLocalGroupsForPotentialMigrations</step>
 		</post-migration>
+		<live-migration>
+			<step>OCA\User_SAML\Migration\CleanupRemovedConfig</step>
+		</live-migration>
 	</repair-steps>
 	<commands>
 		<command>OCA\User_SAML\Command\ConfigCreate</command>

diff --git a/lib/Migration/CleanupRemovedConfig.php b/lib/Migration/CleanupRemovedConfig.php
@@ -0,0 +1,32 @@
+<?php
+
+declare(strict_types=1);
+/**
+ * SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+namespace OCA\User_SAML\Migration;
+
+use OCP\IConfig;
+use OCP\Migration\IOutput;
+use OCP\Migration\IRepairStep;
+
+class CleanupRemovedConfig implements IRepairStep {
+
+	public function __construct(protected IConfig $config) {
+	}
+
+	/**
+	 * @inheritDoc
+	 */
+	public function getName() {
+		return 'Cleans up config keys that are not used anymore';
+	}
+
+	/**
+	 * @inheritDoc
+	 */
+	public function run(IOutput $output) {
+		$this->config->deleteAppValue('user_saml', 'general-use_saml_auth_for_desktop');
+	}
+}
diff --git a/lib/SAMLSettings.php b/lib/SAMLSettings.php
@@ -20,7 +20,7 @@ class SAMLSettings {
 	private const LOADED_ALL = 2;
 
 	// list of global settings which are valid for every idp:
-	// 'general-require_provisioned_account', 'general-allow_multiple_user_back_ends', 'general-use_saml_auth_for_desktop'
+	// 'general-require_provisioned_account', 'general-allow_multiple_user_back_ends'
 
 	// IdP-specific keys
 	public const IDP_CONFIG_KEYS = [

diff --git a/lib/Settings/Admin.php b/lib/Settings/Admin.php
@@ -194,12 +194,6 @@ public function getForm() {
 				'global' => true,
 				'value' => $this->config->getAppValue('user_saml', 'general-require_provisioned_account', 0)
 			];
-			$generalSettings['use_saml_auth_for_desktop'] = [
-				'text' => $this->l10n->t('Use SAML auth for the %s desktop clients (requires user re-authentication)', [$this->defaults->getName()]),
-				'type' => 'checkbox',
-				'global' => true,
-				'value' => $this->config->getAppValue('user_saml', 'general-use_saml_auth_for_desktop', 0)
-			];
 			$generalSettings['idp0_display_name'] = [
 				'text' => $this->l10n->t('Optional display name of the identity provider (default: "SSO & SAML log in")'),
 				'type' => 'line',

diff --git a/tests/integration/features/bootstrap/FeatureContext.php b/tests/integration/features/bootstrap/FeatureContext.php
@@ -111,7 +111,6 @@ public function theSettingIsSetTo($settingName,
 			'type',
 			'general-require_provisioned_account',
 			'general-allow_multiple_user_back_ends',
-			'general-use_saml_auth_for_desktop',
 			'localGroupsCheckForMigration',
 		])) {
 			$this->changedSettings[] = $settingName;

diff --git a/tests/unit/Settings/AdminTest.php b/tests/unit/Settings/AdminTest.php
@@ -47,9 +47,9 @@ public function formDataProvider() {
 		$this->l10n
 			->expects($this->any())
 			->method('t')
-			->will($this->returnCallback(function ($text, $parameters = []) {
+			->willReturnCallback(function ($text, $parameters = []) {
 				return vsprintf($text, $parameters);
-			}));
+			});
 
 		$serviceProviderFields = [
 			'x509cert' => 'X.509 certificate of the Service Provider',
@@ -94,11 +94,6 @@ public function formDataProvider() {
 				'type' => 'checkbox',
 				'global' => true,
 			],
-			'use_saml_auth_for_desktop' => [
-				'text' => 'Use SAML auth for the Nextcloud desktop clients (requires user re-authentication)',
-				'type' => 'checkbox',
-				'global' => true,
-			],
 			'allow_multiple_user_back_ends' => [
 				'text' => $this->l10n->t('Allow the use of multiple user back-ends (e.g. LDAP)'),
 				'type' => 'checkbox',
@@ -231,7 +226,6 @@ public function testGetFormWithoutType() {
 			->willReturn('');
 
 		$params = $this->formDataProvider();
-		unset($params['general']['use_saml_auth_for_desktop']);
 		unset($params['general']['idp0_display_name']);
 		unset($params['general']['allow_multiple_user_back_ends']);
 		$params['type'] = '';
@@ -253,7 +247,6 @@ public function testGetFormWithSaml() {
 			->withConsecutive(
 				['user_saml', 'type'],
 				['user_saml', 'general-require_provisioned_account'],
-				['user_saml', 'general-use_saml_auth_for_desktop'],
 				['user_saml', 'general-allow_multiple_user_back_ends'],
 			)
 			->willReturnOnConsecutiveCalls('saml', 0, 0, '');
@@ -265,7 +258,6 @@ public function testGetFormWithSaml() {
 		$params = $this->formDataProvider();
 		$params['type'] = 'saml';
 		$params['general']['require_provisioned_account']['value'] = 0;
-		$params['general']['use_saml_auth_for_desktop']['value'] = 0;
 		$params['general']['allow_multiple_user_back_ends']['value'] = '';
 
 		$expected = new TemplateResponse('user_saml', 'admin', $params);