Merge pull request #48864 from nextcloud/fix/invalid-app-config

fix(settings): Do not use `null` on `string` parameter for sharing disclaimer

apps/settings/lib/Settings/Admin/Sharing.php

@@ -62,7 +62,7 @@ class Sharing implements IDelegatedSettings {
 			'enforceExpireDate' => $this->getHumanBooleanConfig('core', 'shareapi_enforce_expire_date'),
 			'excludeGroups' => $this->config->getAppValue('core', 'shareapi_exclude_groups', 'no'),
 			'excludeGroupsList' => json_decode($excludedGroups, true) ?? [],
-			'publicShareDisclaimerText' => $this->config->getAppValue('core', 'shareapi_public_link_disclaimertext', null),
+			'publicShareDisclaimerText' => $this->config->getAppValue('core', 'shareapi_public_link_disclaimertext'),
 			'enableLinkPasswordByDefault' => $this->getHumanBooleanConfig('core', 'shareapi_enable_link_password_by_default'),
 			'defaultPermissions' => (int)$this->config->getAppValue('core', 'shareapi_default_permissions', (string)Constants::PERMISSION_ALL),
 			'defaultInternalExpireDate' => $this->getHumanBooleanConfig('core', 'shareapi_default_internal_expire_date'),

apps/settings/src/components/AdminSettingsSharingForm.vue

@@ -170,7 +170,7 @@
 			<NcCheckboxRadioSwitch type="switch" :checked.sync="publicShareDisclaimerEnabled">
 				{{ t('settings', 'Show disclaimer text on the public link upload page (only shown when the file list is hidden)') }}
 			</NcCheckboxRadioSwitch>
-			<div v-if="typeof settings.publicShareDisclaimerText === 'string'"
+			<div v-if="publicShareDisclaimerEnabled"
 				aria-describedby="settings-sharing-privary-related-disclaimer-hint"
 				class="sharing__sub-section">
 				<NcTextArea class="sharing__input"
@@ -231,7 +231,7 @@ interface IShareSettings {
 	enforceExpireDate: boolean
 	excludeGroups: string
 	excludeGroupsList: string[]
-	publicShareDisclaimerText?: string
+	publicShareDisclaimerText: string
 	enableLinkPasswordByDefault: boolean
 	defaultPermissions: number
 	defaultInternalExpireDate: boolean
@@ -252,8 +252,10 @@ export default defineComponent({
 		SelectSharingPermissions,
 	},
 	data() {
+		const settingsData = loadState<IShareSettings>('settings', 'sharingSettings')
 		return {
-			settingsData: loadState<IShareSettings>('settings', 'sharingSettings'),
+			settingsData,
+			publicShareDisclaimerEnabled: settingsData.publicShareDisclaimerText !== '',
 		}
 	},
 	computed: {
@@ -272,26 +274,24 @@ export default defineComponent({
 				},
 			})
 		},
-		publicShareDisclaimerEnabled: {
-			get() {
-				return typeof this.settingsData.publicShareDisclaimerText === 'string'
-			},
-			set(value) {
-				if (value) {
-					this.settingsData.publicShareDisclaimerText = ''
-				} else {
-					this.onUpdateDisclaimer()
-				}
-			},
+	},
+
+	watch: {
+		publicShareDisclaimerEnabled() {
+			// When disabled we just remove the disclaimer content
+			if (this.publicShareDisclaimerEnabled === false) {
+				this.onUpdateDisclaimer('')
+			}
 		},
 	},
+
 	methods: {
 		t,
 
-		onUpdateDisclaimer: debounce(function(value?: string) {
+		onUpdateDisclaimer: debounce(function(value: string) {
 			const options = {
 				success() {
-					if (value) {
+					if (value !== '') {
 						showSuccess(t('settings', 'Changed disclaimer text'))
 					} else {
 						showSuccess(t('settings', 'Deleted disclaimer text'))
@@ -301,7 +301,7 @@ export default defineComponent({
 					showError(t('settings', 'Could not set disclaimer text'))
 				},
 			}
-			if (!value) {
+			if (value === '') {
 				window.OCP.AppConfig.deleteKey('core', 'shareapi_public_link_disclaimertext', options)
 			} else {
 				window.OCP.AppConfig.setValue('core', 'shareapi_public_link_disclaimertext', value, options)

apps/settings/tests/Settings/Admin/SharingTest.php

@@ -20,9 +20,9 @@ use Test\TestCase;
 class SharingTest extends TestCase {
 	/** @var Sharing */
 	private $admin;
-	/** @var IConfig */
+	/** @var IConfig&MockObject */
 	private $config;
-	/** @var IL10N|MockObject */
+	/** @var IL10N&MockObject */
 	private $l10n;
 	/** @var IManager|MockObject */
 	private $shareManager;
@@ -35,9 +35,7 @@ class SharingTest extends TestCase {
 
 	protected function setUp(): void {
 		parent::setUp();
-		/** @var IConfig|MockObject */
 		$this->config = $this->getMockBuilder(IConfig::class)->getMock();
-		/** @var IL10N|MockObject */
 		$this->l10n = $this->getMockBuilder(IL10N::class)->getMock();
 
 		/** @var IManager|MockObject */
@@ -82,7 +80,7 @@ class SharingTest extends TestCase {
 				['core', 'shareapi_expire_after_n_days', '7', '7'],
 				['core', 'shareapi_enforce_expire_date', 'no', 'no'],
 				['core', 'shareapi_exclude_groups', 'no', 'no'],
-				['core', 'shareapi_public_link_disclaimertext', null, 'Lorem ipsum'],
+				['core', 'shareapi_public_link_disclaimertext', '', 'Lorem ipsum'],
 				['core', 'shareapi_enable_link_password_by_default', 'no', 'yes'],
 				['core', 'shareapi_default_permissions', (string)Constants::PERMISSION_ALL, Constants::PERMISSION_ALL],
 				['core', 'shareapi_default_internal_expire_date', 'no', 'no'],
@@ -98,50 +96,53 @@ class SharingTest extends TestCase {
 			->willReturn(false);
 
 		$this->appManager->method('isEnabledForUser')->with('files_sharing')->willReturn(false);
+
+		$initialStateCalls = [];
 		$this->initialState
 			->expects($this->exactly(3))
 			->method('provideInitialState')
-			->withConsecutive(
-				['sharingAppEnabled', false],
-				['sharingDocumentation', ''],
-				[
-					'sharingSettings',
-					[
-						'allowGroupSharing' => true,
-						'allowLinks' => true,
-						'allowPublicUpload' => true,
-						'allowResharing' => true,
-						'allowShareDialogUserEnumeration' => true,
-						'restrictUserEnumerationToGroup' => false,
-						'restrictUserEnumerationToPhone' => false,
-						'restrictUserEnumerationFullMatch' => true,
-						'restrictUserEnumerationFullMatchUserId' => true,
-						'restrictUserEnumerationFullMatchEmail' => true,
-						'restrictUserEnumerationFullMatchIgnoreSecondDN' => false,
-						'enforceLinksPassword' => false,
-						'onlyShareWithGroupMembers' => false,
-						'enabled' => true,
-						'defaultExpireDate' => false,
-						'expireAfterNDays' => '7',
-						'enforceExpireDate' => false,
-						'excludeGroups' => 'no',
-						'excludeGroupsList' => [],
-						'publicShareDisclaimerText' => 'Lorem ipsum',
-						'enableLinkPasswordByDefault' => true,
-						'defaultPermissions' => Constants::PERMISSION_ALL,
-						'defaultInternalExpireDate' => false,
-						'internalExpireAfterNDays' => '7',
-						'enforceInternalExpireDate' => false,
-						'defaultRemoteExpireDate' => false,
-						'remoteExpireAfterNDays' => '7',
-						'enforceRemoteExpireDate' => false,
-						'allowLinksExcludeGroups' => [],
-						'onlyShareWithGroupMembersExcludeGroupList' => [],
-						'enforceLinksPasswordExcludedGroups' => [],
-						'enforceLinksPasswordExcludedGroupsEnabled' => false,
-					]
-				],
-			);
+			->willReturnCallback(function (string $key) use (&$initialStateCalls) {
+				$initialStateCalls[$key] = func_get_args();
+			});
+		
+		$expectedInitialStateCalls = [
+			'sharingAppEnabled' => false,
+			'sharingDocumentation' => '',
+			'sharingSettings' => [
+				'allowGroupSharing' => true,
+				'allowLinks' => true,
+				'allowPublicUpload' => true,
+				'allowResharing' => true,
+				'allowShareDialogUserEnumeration' => true,
+				'restrictUserEnumerationToGroup' => false,
+				'restrictUserEnumerationToPhone' => false,
+				'restrictUserEnumerationFullMatch' => true,
+				'restrictUserEnumerationFullMatchUserId' => true,
+				'restrictUserEnumerationFullMatchEmail' => true,
+				'restrictUserEnumerationFullMatchIgnoreSecondDN' => false,
+				'enforceLinksPassword' => false,
+				'onlyShareWithGroupMembers' => false,
+				'enabled' => true,
+				'defaultExpireDate' => false,
+				'expireAfterNDays' => '7',
+				'enforceExpireDate' => false,
+				'excludeGroups' => 'no',
+				'excludeGroupsList' => [],
+				'publicShareDisclaimerText' => 'Lorem ipsum',
+				'enableLinkPasswordByDefault' => true,
+				'defaultPermissions' => Constants::PERMISSION_ALL,
+				'defaultInternalExpireDate' => false,
+				'internalExpireAfterNDays' => '7',
+				'enforceInternalExpireDate' => false,
+				'defaultRemoteExpireDate' => false,
+				'remoteExpireAfterNDays' => '7',
+				'enforceRemoteExpireDate' => false,
+				'allowLinksExcludeGroups' => [],
+				'onlyShareWithGroupMembersExcludeGroupList' => [],
+				'enforceLinksPasswordExcludedGroups' => [],
+				'enforceLinksPasswordExcludedGroupsEnabled' => false,
+			]
+		];
 
 		$expected = new TemplateResponse(
 			'settings',
@@ -151,6 +152,7 @@ class SharingTest extends TestCase {
 		);
 
 		$this->assertEquals($expected, $this->admin->getForm());
+		$this->assertEquals(sort($expectedInitialStateCalls), sort($initialStateCalls), 'Provided initial state does not match');
 	}
 
 	public function testGetFormWithExcludedGroups(): void {
@@ -175,7 +177,7 @@ class SharingTest extends TestCase {
 				['core', 'shareapi_expire_after_n_days', '7', '7'],
 				['core', 'shareapi_enforce_expire_date', 'no', 'no'],
 				['core', 'shareapi_exclude_groups', 'no', 'yes'],
-				['core', 'shareapi_public_link_disclaimertext', null, 'Lorem ipsum'],
+				['core', 'shareapi_public_link_disclaimertext', '', 'Lorem ipsum'],
 				['core', 'shareapi_enable_link_password_by_default', 'no', 'yes'],
 				['core', 'shareapi_default_permissions', (string)Constants::PERMISSION_ALL, Constants::PERMISSION_ALL],
 				['core', 'shareapi_default_internal_expire_date', 'no', 'no'],
@@ -191,50 +193,53 @@ class SharingTest extends TestCase {
 			->willReturn(false);
 
 		$this->appManager->method('isEnabledForUser')->with('files_sharing')->willReturn(true);
+
+		$initialStateCalls = [];
 		$this->initialState
 			->expects($this->exactly(3))
 			->method('provideInitialState')
-			->withConsecutive(
-				['sharingAppEnabled', true],
-				['sharingDocumentation', ''],
-				[
-					'sharingSettings',
-					[
-						'allowGroupSharing' => true,
-						'allowLinks' => true,
-						'allowPublicUpload' => true,
-						'allowResharing' => true,
-						'allowShareDialogUserEnumeration' => true,
-						'restrictUserEnumerationToGroup' => false,
-						'restrictUserEnumerationToPhone' => false,
-						'restrictUserEnumerationFullMatch' => true,
-						'restrictUserEnumerationFullMatchUserId' => true,
-						'restrictUserEnumerationFullMatchEmail' => true,
-						'restrictUserEnumerationFullMatchIgnoreSecondDN' => false,
-						'enforceLinksPassword' => false,
-						'onlyShareWithGroupMembers' => false,
-						'enabled' => true,
-						'defaultExpireDate' => false,
-						'expireAfterNDays' => '7',
-						'enforceExpireDate' => false,
-						'excludeGroups' => 'yes',
-						'excludeGroupsList' => ['NoSharers','OtherNoSharers'],
-						'publicShareDisclaimerText' => 'Lorem ipsum',
-						'enableLinkPasswordByDefault' => true,
-						'defaultPermissions' => Constants::PERMISSION_ALL,
-						'defaultInternalExpireDate' => false,
-						'internalExpireAfterNDays' => '7',
-						'enforceInternalExpireDate' => false,
-						'defaultRemoteExpireDate' => false,
-						'remoteExpireAfterNDays' => '7',
-						'enforceRemoteExpireDate' => false,
-						'allowLinksExcludeGroups' => [],
-						'onlyShareWithGroupMembersExcludeGroupList' => [],
-						'enforceLinksPasswordExcludedGroups' => [],
-						'enforceLinksPasswordExcludedGroupsEnabled' => false,
-					]
-				],
-			);
+			->willReturnCallback(function (string $key) use (&$initialStateCalls) {
+				$initialStateCalls[$key] = func_get_args();
+			});
+
+		$expectedInitialStateCalls = [
+			'sharingAppEnabled' => true,
+			'sharingDocumentation' => '',
+			'sharingSettings' => [
+				'allowGroupSharing' => true,
+				'allowLinks' => true,
+				'allowPublicUpload' => true,
+				'allowResharing' => true,
+				'allowShareDialogUserEnumeration' => true,
+				'restrictUserEnumerationToGroup' => false,
+				'restrictUserEnumerationToPhone' => false,
+				'restrictUserEnumerationFullMatch' => true,
+				'restrictUserEnumerationFullMatchUserId' => true,
+				'restrictUserEnumerationFullMatchEmail' => true,
+				'restrictUserEnumerationFullMatchIgnoreSecondDN' => false,
+				'enforceLinksPassword' => false,
+				'onlyShareWithGroupMembers' => false,
+				'enabled' => true,
+				'defaultExpireDate' => false,
+				'expireAfterNDays' => '7',
+				'enforceExpireDate' => false,
+				'excludeGroups' => 'yes',
+				'excludeGroupsList' => ['NoSharers','OtherNoSharers'],
+				'publicShareDisclaimerText' => 'Lorem ipsum',
+				'enableLinkPasswordByDefault' => true,
+				'defaultPermissions' => Constants::PERMISSION_ALL,
+				'defaultInternalExpireDate' => false,
+				'internalExpireAfterNDays' => '7',
+				'enforceInternalExpireDate' => false,
+				'defaultRemoteExpireDate' => false,
+				'remoteExpireAfterNDays' => '7',
+				'enforceRemoteExpireDate' => false,
+				'allowLinksExcludeGroups' => [],
+				'onlyShareWithGroupMembersExcludeGroupList' => [],
+				'enforceLinksPasswordExcludedGroups' => [],
+				'enforceLinksPasswordExcludedGroupsEnabled' => false,
+			],
+		];
 
 		$expected = new TemplateResponse(
 			'settings',
@@ -244,6 +249,7 @@ class SharingTest extends TestCase {
 		);
 
 		$this->assertEquals($expected, $this->admin->getForm());
+		$this->assertEquals(sort($expectedInitialStateCalls), sort($initialStateCalls), 'Provided initial state does not match');
 	}
 
 	public function testGetSection(): void {

build/psalm-baseline.xml

@@ -1041,11 +1041,6 @@
       <code><![CDATA[isReady]]></code>
     </UndefinedInterfaceMethod>
   </file>
-  <file src="apps/settings/lib/Settings/Admin/Sharing.php">
-    <NullArgument>
-      <code><![CDATA[null]]></code>
-    </NullArgument>
-  </file>
   <file src="apps/sharebymail/lib/ShareByMailProvider.php">
     <InvalidArgument>
       <code><![CDATA[$share->getId()]]></code>