Merge pull request #51288 from nextcloud/fix/admin-tag-color-prevent

fix(systemtags): unify restrict_creation_to_admin handling

Author: skjnldsv

apps/dav/lib/SystemTag/SystemTagPlugin.php

@@ -19,6 +19,7 @@
 use OCP\SystemTag\ISystemTagObjectMapper;
 use OCP\SystemTag\TagAlreadyExistsException;
 use OCP\SystemTag\TagCreationForbiddenException;
+use OCP\SystemTag\TagUpdateForbiddenException;
 use OCP\Util;
 use Sabre\DAV\Exception\BadRequest;
 use Sabre\DAV\Exception\Conflict;
@@ -191,7 +192,7 @@ private function createTag($data, $contentType = 'application/json') {
 		} catch (TagAlreadyExistsException $e) {
 			throw new Conflict('Tag already exists', 0, $e);
 		} catch (TagCreationForbiddenException $e) {
-			throw new Forbidden('You don’t have right to create tags', 0, $e);
+			throw new Forbidden('You don’t have permissions to create tags', 0, $e);
 		}
 	}
 
@@ -472,7 +473,11 @@ public function handleUpdateProperties($path, PropPatch $propPatch) {
 			}
 
 			if ($updateTag) {
-				$node->update($name, $userVisible, $userAssignable, $color);
+				try {
+					$node->update($name, $userVisible, $userAssignable, $color);
+				} catch (TagUpdateForbiddenException $e) {
+					throw new Forbidden('You don’t have permissions to update tags', 0, $e);
+				}
 			}
 
 			return true;

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

@@ -55,7 +55,7 @@ public function getForm() {
 		$this->initialStateService->provideInitialState('profileEnabledByDefault', $this->isProfileEnabledByDefault($this->config));
 
 		// Basic settings
-		$this->initialStateService->provideInitialState('restrictSystemTagsCreationToAdmin', $this->appConfig->getValueString('systemtags', 'restrict_creation_to_admin', 'true'));
+		$this->initialStateService->provideInitialState('restrictSystemTagsCreationToAdmin', $this->appConfig->getValueBool('systemtags', 'restrict_creation_to_admin', false));
 
 		return new TemplateResponse('settings', 'settings/admin/server', [
 			'profileEnabledGlobally' => $this->profileManager->isProfileEnabled(),

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

@@ -85,6 +85,10 @@ public function testGetForm(): void {
 			->expects($this->any())
 			->method('getValueString')
 			->willReturnCallback(fn ($a, $b, $default) => $default);
+		$this->appConfig
+			->expects($this->any())
+			->method('getValueBool')
+			->willReturnCallback(fn ($a, $b, $default) => $default);
 		$this->profileManager
 			->expects($this->exactly(2))
 			->method('isProfileEnabled')

apps/systemtags/lib/Listeners/BeforeTemplateRenderedListener.php

@@ -9,18 +9,29 @@
 
 use OCA\Files_Sharing\Event\BeforeTemplateRenderedEvent;
 use OCA\SystemTags\AppInfo\Application;
+use OCP\AppFramework\Services\IInitialState;
 use OCP\EventDispatcher\Event;
 use OCP\EventDispatcher\IEventListener;
+use OCP\IAppConfig;
 use OCP\Util;
 
 /**
  * @template-implements IEventListener<BeforeTemplateRenderedEvent>
  */
 class BeforeTemplateRenderedListener implements IEventListener {
+	public function __construct(
+		private IAppConfig $appConfig,
+		private IInitialState $initialState,
+	) {
+	}
+
 	public function handle(Event $event): void {
 		if (!$event instanceof BeforeTemplateRenderedEvent) {
 			return;
 		}
 		Util::addInitScript(Application::APP_ID, 'init');
+
+		$restrictSystemTagsCreationToAdmin = $this->appConfig->getValueBool(Application::APP_ID, 'restrict_creation_to_admin', false);
+		$this->initialState->provideInitialState('restrictSystemTagsCreationToAdmin', $restrictSystemTagsCreationToAdmin);
 	}
 }

apps/systemtags/lib/Listeners/LoadAdditionalScriptsListener.php

@@ -9,18 +9,29 @@
 
 use OCA\Files\Event\LoadAdditionalScriptsEvent;
 use OCA\SystemTags\AppInfo\Application;
+use OCP\AppFramework\Services\IInitialState;
 use OCP\EventDispatcher\Event;
 use OCP\EventDispatcher\IEventListener;
+use OCP\IAppConfig;
 use OCP\Util;
 
 /**
  * @template-implements IEventListener<LoadAdditionalScriptsEvent>
  */
 class LoadAdditionalScriptsListener implements IEventListener {
+	public function __construct(
+		private IAppConfig $appConfig,
+		private IInitialState $initialState,
+	) {
+	}
+
 	public function handle(Event $event): void {
 		if (!$event instanceof LoadAdditionalScriptsEvent) {
 			return;
 		}
 		Util::addInitScript(Application::APP_ID, 'init');
+
+		$restrictSystemTagsCreationToAdmin = $this->appConfig->getValueBool(Application::APP_ID, 'restrict_creation_to_admin', false);
+		$this->initialState->provideInitialState('restrictSystemTagsCreationToAdmin', $restrictSystemTagsCreationToAdmin);
 	}
 }

apps/systemtags/src/components/SystemTagPicker.vue

@@ -25,7 +25,7 @@
 			<!-- Search or create input -->
 			<div class="systemtags-picker__input">
 				<NcTextField :value.sync="input"
-					:label="t('systemtags', 'Search or create tag')"
+					:label="canEditOrCreateTag ? t('systemtags', 'Search or create tag') : t('systemtags', 'Search tag')"
 					data-cy-systemtags-picker-input>
 					<TagIcon :size="20" />
 				</NcTextField>
@@ -49,7 +49,8 @@
 					</NcCheckboxRadioSwitch>
 
 					<!-- Color picker -->
-					<NcColorPicker :data-cy-systemtags-picker-tag-color="tag.id"
+					<NcColorPicker v-if="canEditOrCreateTag"
+						:data-cy-systemtags-picker-tag-color="tag.id"
 						:value="`#${tag.color}`"
 						:shown="openedPicker === tag.id"
 						class="systemtags-picker__tag-color"
@@ -68,7 +69,7 @@
 
 				<!-- Create new tag -->
 				<li>
-					<NcButton v-if="canCreateTag"
+					<NcButton v-if="canEditOrCreateTag && canCreateTag"
 						:disabled="status === Status.CREATING_TAG"
 						alignment="start"
 						class="systemtags-picker__tag-create"
@@ -88,7 +89,7 @@
 			<!-- Note -->
 			<div class="systemtags-picker__note">
 				<NcNoteCard v-if="!hasChanges" type="info">
-					{{ t('systemtags', 'Select or create tags to apply to all selected files') }}
+					{{ canEditOrCreateTag ? t('systemtags', 'Select or create tags to apply to all selected files'): t('systemtags', 'Select tags to apply to all selected files') }}
 				</NcNoteCard>
 				<NcNoteCard v-else type="info">
 					<span v-html="statusMessage" />
@@ -127,7 +128,9 @@ import type { Tag, TagWithId } from '../types'
 
 import { defineComponent } from 'vue'
 import { emit } from '@nextcloud/event-bus'
+import { getCurrentUser } from '@nextcloud/auth'
 import { getLanguage, n, t } from '@nextcloud/l10n'
+import { loadState } from '@nextcloud/initial-state'
 import { showError, showInfo } from '@nextcloud/dialogs'
 import debounce from 'debounce'
 import domPurify from 'dompurify'
@@ -149,9 +152,9 @@ import PencilIcon from 'vue-material-design-icons/Pencil.vue'
 import PlusIcon from 'vue-material-design-icons/Plus.vue'
 import TagIcon from 'vue-material-design-icons/Tag.vue'
 
-import { createTag, fetchTag, fetchTags, getTagObjects, setTagObjects, updateTag } from '../services/api'
-import { getNodeSystemTags, setNodeSystemTags } from '../utils'
-import { elementColor, invertTextColor, isDarkModeEnabled } from '../utils/colorUtils'
+import { createTag, fetchTag, fetchTags, getTagObjects, setTagObjects, updateTag } from '../services/api.ts'
+import { elementColor, invertTextColor, isDarkModeEnabled } from '../utils/colorUtils.ts'
+import { getNodeSystemTags, setNodeSystemTags } from '../utils.ts'
 import logger from '../logger.ts'
 
 const debounceUpdateTag = debounce(updateTag, 500)
@@ -170,6 +173,8 @@ enum Status {
 	DONE = 'done',
 }
 
+const restrictSystemTagsCreationToAdmin = loadState('systemtags', 'restrictSystemTagsCreationToAdmin', false)
+
 export default defineComponent({
 	name: 'SystemTagPicker',
 
@@ -204,6 +209,8 @@ export default defineComponent({
 			emit,
 			Status,
 			t,
+			// Either tag creation is not restricted to admins or the current user is an admin
+			canEditOrCreateTag: !restrictSystemTagsCreationToAdmin || getCurrentUser()?.isAdmin,
 		}
 	},
 
@@ -364,6 +371,10 @@ export default defineComponent({
 			})
 			return acc
 		}, {} as TagListCount) as TagListCount
+
+		if (!this.canEditOrCreateTag) {
+			logger.debug('System tag creation is restricted to admins and the current user is not an admin')
+		}
 	},
 
 	methods: {
@@ -422,6 +433,12 @@ export default defineComponent({
 		},
 
 		async onNewTag() {
+			if (!this.canEditOrCreateTag) {
+				// Should not happen ™
+				showError(t('systemtags', 'Only admins can create new tags'))
+				return
+			}
+
 			this.status = Status.CREATING_TAG
 			try {
 				const payload: Tag = {

apps/systemtags/src/components/SystemTags.vue

@@ -189,7 +189,8 @@ export default Vue.extend({
 				this.sortedTags.unshift(createdTag)
 				this.selectedTags.push(createdTag)
 			} catch (error) {
-				if(loadState('settings', 'restrictSystemTagsCreationToAdmin', '0') === '1') {
+				const systemTagsCreationRestrictedToAdmin = loadState<true|false>('settings', 'restrictSystemTagsCreationToAdmin', false) === true
+				if (systemTagsCreationRestrictedToAdmin) {
 					showError(t('systemtags', 'System admin disabled tag creation. You can only use existing ones.'))
 					return
 				}

apps/systemtags/src/components/SystemTagsCreationControl.vue

@@ -6,17 +6,17 @@
 <template>
 	<div id="system-tags-creation-control">
 		<h4 class="inlineblock">
-			{{ t('settings', 'System tag creation') }}
+			{{ t('settings', 'System tag management') }}
 		</h4>
 
 		<p class="settings-hint">
-			{{ t('settings', 'If enabled, regular accounts will be restricted from creating new tags but will still be able to assign and remove them from their files.') }}
+			{{ t('settings', 'If enabled, only administrators can create and edit tags. Accounts can still assign and remove them from files.') }}
 		</p>
 
 		<NcCheckboxRadioSwitch type="switch"
 			:checked.sync="systemTagsCreationRestrictedToAdmin"
 			@update:checked="updateSystemTagsDefault">
-			{{ t('settings', 'Restrict tag creation to admins only') }}
+			{{ t('settings', 'Restrict tag creation and editing to administrators') }}
 		</NcCheckboxRadioSwitch>
 	</div>
 </template>
@@ -25,8 +25,9 @@
 import { loadState } from '@nextcloud/initial-state'
 import { showError, showSuccess } from '@nextcloud/dialogs'
 import { t } from '@nextcloud/l10n'
-import logger from '../logger.ts'
+
 import { updateSystemTagsAdminRestriction } from '../services/api.js'
+import logger from '../logger.ts'
 
 import NcCheckboxRadioSwitch from '@nextcloud/vue/components/NcCheckboxRadioSwitch'
 
@@ -37,14 +38,19 @@ export default {
 		NcCheckboxRadioSwitch,
 	},
 
+	setup() {
+		return {
+			t,
+		}
+	},
+
 	data() {
 		return {
 			// By default, system tags creation is not restricted to admins
-			systemTagsCreationRestrictedToAdmin: loadState('settings', 'restrictSystemTagsCreationToAdmin', '0') === '1',
+			systemTagsCreationRestrictedToAdmin: loadState('settings', 'restrictSystemTagsCreationToAdmin', false),
 		}
 	},
 	methods: {
-		t,
 		async updateSystemTagsDefault(isRestricted: boolean) {
 			try {
 				const responseData = await updateSystemTagsAdminRestriction(isRestricted)

apps/systemtags/src/files_actions/bulkSystemTagsAction.ts

@@ -9,13 +9,9 @@ import { FileAction } from '@nextcloud/files'
 import { isPublicShare } from '@nextcloud/sharing/public'
 import { spawnDialog } from '@nextcloud/dialogs'
 import { t } from '@nextcloud/l10n'
-import { getCurrentUser } from '@nextcloud/auth'
-import { loadState } from '@nextcloud/initial-state'
 
 import TagMultipleSvg from '@mdi/svg/svg/tag-multiple.svg?raw'
 
-const restrictSystemTagsCreationToAdmin = loadState<'0'|'1'>('settings', 'restrictSystemTagsCreationToAdmin', '0') === '1'
-
 /**
  * Spawn a dialog to add or remove tags from multiple nodes.
  * @param nodes Nodes to modify tags for
@@ -38,11 +34,6 @@ export const action = new FileAction({
 
 	// If the app is disabled, the action is not available anyway
 	enabled(nodes) {
-		// By default, everyone can create system tags
-		if (restrictSystemTagsCreationToAdmin && getCurrentUser()?.isAdmin !== true) {
-			return false
-		}
-
 		if (isPublicShare()) {
 			return false
 		}

cypress/e2e/systemtags/files-bulk-action.cy.ts

@@ -75,6 +75,11 @@ describe('Systemtags: Files bulk action', { testIsolation: false }, () => {
 		resetTags()
 	})
 
+	after(() => {
+		resetTags()
+		cy.runOccCommand('config:app:set systemtags restrict_creation_to_admin --value 0')
+	})
+
 	it('Can assign tag to selection', () => {
 		cy.login(user1)
 		cy.visit('/apps/files')
@@ -87,6 +92,7 @@ describe('Systemtags: Files bulk action', { testIsolation: false }, () => {
 
 		triggerTagManagementDialogAction()
 		cy.get('[data-cy-systemtags-picker-tag]').should('have.length', 5)
+		cy.get('[data-cy-systemtags-picker-tag-color]').should('have.length', 5)
 
 		cy.intercept('PROPFIND', '/remote.php/dav/systemtags/*/files').as('getTagData')
 		cy.intercept('PROPPATCH', '/remote.php/dav/systemtags/*/files').as('assignTagData')
@@ -115,6 +121,7 @@ describe('Systemtags: Files bulk action', { testIsolation: false }, () => {
 
 		triggerTagManagementDialogAction()
 		cy.get('[data-cy-systemtags-picker-tag]').should('have.length', 5)
+		cy.get('[data-cy-systemtags-picker-tag-color]').should('have.length', 5)
 
 		cy.intercept('PROPFIND', '/remote.php/dav/systemtags/*/files').as('getTagData')
 		cy.intercept('PROPPATCH', '/remote.php/dav/systemtags/*/files').as('assignTagData')
@@ -353,4 +360,55 @@ describe('Systemtags: Files bulk action', { testIsolation: false }, () => {
 			expectInlineTagForFile('file5.txt', [newTag])
 		})
 	})
+
+	it('Cannot create tag if restriction is in place', () => {
+		let tagId: string
+
+		cy.runOccCommand('config:app:set systemtags restrict_creation_to_admin --value 1')
+		cy.runOccCommand('tag:add testTag public --output json').then(({ stdout }) => {
+			const tag = JSON.parse(stdout)
+			tagId = tag.id
+		})
+
+		cy.createRandomUser().then((user1) => {
+			files.forEach((file) => {
+				cy.uploadContent(user1, new Blob([]), 'text/plain', '/' + file)
+			})
+
+			cy.login(user1)
+			cy.visit('/apps/files')
+
+			files.forEach((file) => {
+				getRowForFile(file).should('be.visible')
+			})
+			selectAllFiles()
+
+			triggerTagManagementDialogAction()
+
+			cy.findByRole('textbox', { name: 'Search or create tag' }).should('not.exist')
+			cy.findByRole('textbox', { name: 'Search tag' }).should('be.visible')
+
+			cy.get('[data-cy-systemtags-picker-input]').type('testTag')
+
+			cy.get('[data-cy-systemtags-picker-tag]').should('have.length', 1)
+			cy.get('[data-cy-systemtags-picker-button-create]').should('not.exist')
+			cy.get('[data-cy-systemtags-picker-tag-color]').should('not.exist')
+
+			// Assign the tag
+			cy.intercept('PROPFIND', '/remote.php/dav/systemtags/*/files').as('getTagData')
+			cy.intercept('PROPPATCH', '/remote.php/dav/systemtags/*/files').as('assignTagData')
+
+			cy.get(`[data-cy-systemtags-picker-tag="${tagId}"]`).should('be.visible')
+				.findByRole('checkbox').click({ force: true })
+			cy.get('[data-cy-systemtags-picker-button-submit]').click()
+
+			cy.wait('@getTagData')
+			cy.wait('@assignTagData')
+
+			cy.get('[data-cy-systemtags-picker]').should('not.exist')
+
+			// Finally, reset the restriction
+			cy.runOccCommand('config:app:set systemtags restrict_creation_to_admin --value 0')
+		})
+	})
 })