From 1ca53c6a71ff4ad7e0e147a451f93ed90b6c418f Mon Sep 17 00:00:00 2001 From: Git'Fellow <12234510+solracsf@users.noreply.github.com> Date: Fri, 13 Jan 2023 18:59:56 +0100 Subject: [PATCH 1/2] 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 <12234510+solracsf@users.noreply.github.com> Signed-off-by: Arthur Schiwon --- appinfo/app.php | 25 --------------- appinfo/info.xml | 3 ++ lib/Migration/CleanupRemovedConfig.php | 32 +++++++++++++++++++ lib/SAMLSettings.php | 2 +- lib/Settings/Admin.php | 6 ---- .../features/bootstrap/FeatureContext.php | 1 - tests/unit/Settings/AdminTest.php | 12 ++----- 7 files changed, 38 insertions(+), 43 deletions(-) create mode 100644 lib/Migration/CleanupRemovedConfig.php diff --git a/appinfo/app.php b/appinfo/app.php index 29986ff1..f238ba3a 100644 --- 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 index 95cea036..9df4fb24 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -43,6 +43,9 @@ While theoretically any other authentication provider implementing either one of OCA\User_SAML\Migration\RememberLocalGroupsForPotentialMigrations + + OCA\User_SAML\Migration\CleanupRemovedConfig + OCA\User_SAML\Command\ConfigCreate diff --git a/lib/Migration/CleanupRemovedConfig.php b/lib/Migration/CleanupRemovedConfig.php new file mode 100644 index 00000000..66ac2965 --- /dev/null +++ b/lib/Migration/CleanupRemovedConfig.php @@ -0,0 +1,32 @@ +config->deleteAppValue('user_saml', 'general-use_saml_auth_for_desktop'); + } +} diff --git a/lib/SAMLSettings.php b/lib/SAMLSettings.php index 90470a13..7eb464be 100644 --- 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 index a3380c60..26ef6afe 100644 --- 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 index 5758a60b..4c359169 100644 --- 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 index c29a0fb0..704ecfcb 100644 --- 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); From 8c1fa828c04c38f68f633aa3ade9e1b2b2bb0e55 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Fri, 31 May 2024 16:09:20 +0200 Subject: [PATCH 2/2] test(unit): streamline default value "for allow_multiple_user_back_ends" - getAppValue() returns strings Signed-off-by: Arthur Schiwon --- lib/Settings/Admin.php | 4 ++-- tests/unit/Settings/AdminTest.php | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/Settings/Admin.php b/lib/Settings/Admin.php index 26ef6afe..12e0079e 100644 --- a/lib/Settings/Admin.php +++ b/lib/Settings/Admin.php @@ -192,7 +192,7 @@ public function getForm() { 'text' => $this->l10n->t('Only allow authentication if an account exists on some other backend (e.g. LDAP).', [$this->defaults->getName()]), 'type' => 'checkbox', 'global' => true, - 'value' => $this->config->getAppValue('user_saml', 'general-require_provisioned_account', 0) + 'value' => $this->config->getAppValue('user_saml', 'general-require_provisioned_account', '0') ]; $generalSettings['idp0_display_name'] = [ 'text' => $this->l10n->t('Optional display name of the identity provider (default: "SSO & SAML log in")'), @@ -204,7 +204,7 @@ public function getForm() { 'type' => 'checkbox', 'hideForEnv' => true, 'global' => true, - 'value' => $this->config->getAppValue('user_saml', 'general-allow_multiple_user_back_ends') + 'value' => $this->config->getAppValue('user_saml', 'general-allow_multiple_user_back_ends', '0') ]; } diff --git a/tests/unit/Settings/AdminTest.php b/tests/unit/Settings/AdminTest.php index 704ecfcb..2fbdd61f 100644 --- a/tests/unit/Settings/AdminTest.php +++ b/tests/unit/Settings/AdminTest.php @@ -242,14 +242,14 @@ public function testGetFormWithSaml() { 2 => 'Provider 2', ]); $this->config - ->expects($this->exactly(4)) # mode + three global values + ->expects($this->exactly(3)) # mode + three global values ->method('getAppValue') ->withConsecutive( ['user_saml', 'type'], ['user_saml', 'general-require_provisioned_account'], ['user_saml', 'general-allow_multiple_user_back_ends'], ) - ->willReturnOnConsecutiveCalls('saml', 0, 0, ''); + ->willReturnOnConsecutiveCalls('saml', '0', '0'); $this->defaults ->expects($this->any()) ->method('getName') @@ -257,8 +257,8 @@ public function testGetFormWithSaml() { $params = $this->formDataProvider(); $params['type'] = 'saml'; - $params['general']['require_provisioned_account']['value'] = 0; - $params['general']['allow_multiple_user_back_ends']['value'] = ''; + $params['general']['require_provisioned_account']['value'] = '0'; + $params['general']['allow_multiple_user_back_ends']['value'] = '0'; $expected = new TemplateResponse('user_saml', 'admin', $params); $this->assertEquals($expected, $this->admin->getForm());