Skip to content

Commit

Permalink
fix(systemtags): prevent tag edition if restricted
Browse files Browse the repository at this point in the history
Signed-off-by: skjnldsv <[email protected]>
  • Loading branch information
skjnldsv committed Mar 5, 2025
1 parent ef62e17 commit 6ef85aa
Show file tree
Hide file tree
Showing 6 changed files with 122 additions and 18 deletions.
18 changes: 17 additions & 1 deletion apps/dav/lib/SystemTag/SystemTagPlugin.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
use OCA\DAV\Connector\Sabre\FilesPlugin;
use OCA\DAV\Connector\Sabre\Node;
use OCP\AppFramework\Http;
use OCP\IAppConfig;
use OCP\IGroupManager;
use OCP\IUser;
use OCP\IUserSession;
Expand Down Expand Up @@ -68,6 +69,7 @@ public function __construct(
protected IGroupManager $groupManager,
protected IUserSession $userSession,
private ISystemTagObjectMapper $tagMapper,
private IAppConfig $appConfig,
) {
}

Expand Down Expand Up @@ -176,12 +178,19 @@ private function createTag($data, $contentType = 'application/json') {
}
}

$currentUserId = $this->userSession->getUser()?->getUID();
if ($userVisible === false || $userAssignable === false || !empty($groups)) {
if (!$this->userSession->isLoggedIn() || !$this->groupManager->isAdmin($this->userSession->getUser()->getUID())) {
if (!$this->userSession->isLoggedIn() || !$this->groupManager->isAdmin($currentUserId)) {
throw new BadRequest('Not sufficient permissions');
}
}

// By default, system tags creation is not restricted to admins
if (!$this->appConfig->getValueBool('systemtags', 'restrict_creation_to_admin', false)
|| $this->groupManager->isAdmin($currentUserId || '')) {

Check failure on line 190 in apps/dav/lib/SystemTag/SystemTagPlugin.php

View workflow job for this annotation

GitHub Actions / static-code-analysis

InvalidArgument

apps/dav/lib/SystemTag/SystemTagPlugin.php:190:36: InvalidArgument: Argument 1 of OCP\IGroupManager::isAdmin expects string, but bool provided (see https://psalm.dev/004)

Check failure on line 190 in apps/dav/lib/SystemTag/SystemTagPlugin.php

View workflow job for this annotation

GitHub Actions / static-code-analysis

TypeDoesNotContainType

apps/dav/lib/SystemTag/SystemTagPlugin.php:190:54: TypeDoesNotContainType: Operand of type '' is always falsy (see https://psalm.dev/056)
throw new Forbidden('You don’t have right to create tags');
}

try {
$tag = $this->tagManager->createTag($tagName, $userVisible, $userAssignable);
if (!empty($groups)) {
Expand Down Expand Up @@ -421,6 +430,13 @@ public function handleUpdateProperties($path, PropPatch $propPatch) {
return false;
}

$currentUserId = $this->userSession->getUser()?->getUID();
// By default, system tags edition is not restricted to admins
if (!$this->appConfig->getValueBool('systemtags', 'restrict_creation_to_admin', false)
|| $this->groupManager->isAdmin($currentUserId || '')) {

Check failure on line 436 in apps/dav/lib/SystemTag/SystemTagPlugin.php

View workflow job for this annotation

GitHub Actions / static-code-analysis

InvalidArgument

apps/dav/lib/SystemTag/SystemTagPlugin.php:436:37: InvalidArgument: Argument 1 of OCP\IGroupManager::isAdmin expects string, but bool provided (see https://psalm.dev/004)

Check failure on line 436 in apps/dav/lib/SystemTag/SystemTagPlugin.php

View workflow job for this annotation

GitHub Actions / static-code-analysis

TypeDoesNotContainType

apps/dav/lib/SystemTag/SystemTagPlugin.php:436:55: TypeDoesNotContainType: Operand of type '' is always falsy (see https://psalm.dev/056)
throw new Forbidden('You don’t have right to create tags');
}

$tag = $node->getSystemTag();
$name = $tag->getName();
$userVisible = $tag->isUserVisible();
Expand Down
17 changes: 16 additions & 1 deletion apps/dav/tests/unit/SystemTag/SystemTagPluginTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
use OCA\DAV\SystemTag\SystemTagPlugin;
use OCA\DAV\SystemTag\SystemTagsByIdCollection;
use OCA\DAV\SystemTag\SystemTagsObjectMappingCollection;
use OCP\IAppConfig;
use OCP\IGroupManager;
use OCP\IUser;
use OCP\IUserSession;
Expand Down Expand Up @@ -71,6 +72,11 @@ class SystemTagPluginTest extends \Test\TestCase {
*/
private $tagMapper;

/**
* @var IAppConfig
*/
private $appConfig;

protected function setUp(): void {
parent::setUp();
$this->tree = $this->getMockBuilder(Tree::class)
Expand Down Expand Up @@ -98,11 +104,20 @@ protected function setUp(): void {
$this->tagMapper = $this->getMockBuilder(ISystemTagObjectMapper::class)
->getMock();

$this->appConfig = $this->getMockBuilder(IAppConfig::class)
->getMock();

$this->appConfig
->expects($this->any())
->method('getValueBool')
->willReturnCallback(fn ($a, $b, $default) => $default);

$this->plugin = new SystemTagPlugin(
$this->tagManager,
$this->groupManager,
$this->userSession,
$this->tagMapper
$this->tagMapper,
$this->appConfig,
);
$this->plugin->initialize($this->server);
}
Expand Down
4 changes: 4 additions & 0 deletions apps/settings/tests/Settings/Admin/ServerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down
29 changes: 17 additions & 12 deletions apps/systemtags/src/components/SystemTagPicker.vue
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
<!-- Search or create input -->
<div class="systemtags-picker__input">
<NcTextField :value.sync="input"
:label="canCreateTag ? t('systemtags', 'Search or create tag') : t('systemtags', 'Search tag')"
:label="canEditOrCreateTag ? t('systemtags', 'Search or create tag') : t('systemtags', 'Search tag')"
data-cy-systemtags-picker-input>
<TagIcon :size="20" />
</NcTextField>
Expand All @@ -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"
Expand All @@ -67,8 +68,8 @@
</li>

<!-- Create new tag -->
<li v-if="canCreateTag">
<NcButton v-if="canCreateTag"
<li v-if="canEditOrCreateTag && input.trim() !== ''">
<NcButton v-if="canEditOrCreateTag"
:disabled="status === Status.CREATING_TAG"
alignment="start"
class="systemtags-picker__tag-create"
Expand All @@ -88,7 +89,7 @@
<!-- Note -->
<div class="systemtags-picker__note">
<NcNoteCard v-if="!hasChanges" type="info">
{{ canCreateTag ? t('systemtags', 'Select or create tags to apply to all selected files'): t('systemtags', 'Select 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" />
Expand Down Expand Up @@ -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'
Expand All @@ -150,11 +153,9 @@ 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 { getNodeSystemTags, setNodeSystemTags } from '../utils'
import logger from '../logger.ts'
import { loadState } from '@nextcloud/initial-state'
import { getCurrentUser } from '@nextcloud/auth'

const debounceUpdateTag = debounce(updateTag, 500)
const mainBackgroundColor = getComputedStyle(document.body)
Expand All @@ -172,7 +173,7 @@ enum Status {
DONE = 'done',
}

const restrictSystemTagsCreationToAdmin = loadState<false|true>('settings', 'restrictSystemTagsCreationToAdmin', false) === true
const restrictSystemTagsCreationToAdmin = loadState<false|true>('systemtags', 'restrictSystemTagsCreationToAdmin', false) === true

export default defineComponent({
name: 'SystemTagPicker',
Expand Down Expand Up @@ -209,7 +210,7 @@ export default defineComponent({
Status,
t,
// Either tag creation is not restricted to admins or the current user is an admin
canCreateTag: !restrictSystemTagsCreationToAdmin || getCurrentUser()?.isAdmin,
canEditOrCreateTag: !restrictSystemTagsCreationToAdmin || getCurrentUser()?.isAdmin,
}
},

Expand Down Expand Up @@ -247,7 +248,7 @@ export default defineComponent({
return this.toAdd.length > 0 || this.toRemove.length > 0
},

canCreateTag(): boolean {
canEditOrCreateTag(): boolean {

Check failure on line 251 in apps/systemtags/src/components/SystemTagPicker.vue

View workflow job for this annotation

GitHub Actions / NPM lint

Duplicate key 'canEditOrCreateTag'. May cause name collision in script or template tag
return this.input.trim() !== ''
&& !this.tags.some(tag => tag.displayName.trim().toLocaleLowerCase() === this.input.trim().toLocaleLowerCase())
},
Expand Down Expand Up @@ -370,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: {
Expand Down Expand Up @@ -428,7 +433,7 @@ export default defineComponent({
},

async onNewTag() {
if (!this.canCreateTag) {
if (!this.canEditOrCreateTag) {
// Should not happen ™
showError(t('systemtags', 'Only admins can create new tags'))
return
Expand Down
14 changes: 10 additions & 4 deletions apps/systemtags/src/components/SystemTagsCreationControl.vue
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,13 @@
</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, regular accounts will be restricted from editing or creating new tags but will still be able to assign and remove them from their files.') }}
</p>

<NcCheckboxRadioSwitch type="switch"
:checked.sync="systemTagsCreationRestrictedToAdmin"
@update:checked="updateSystemTagsDefault">
{{ t('settings', 'Restrict tag creation to admins only') }}
{{ t('settings', 'Restrict tag edition and creation to admins only') }}
</NcCheckboxRadioSwitch>
</div>
</template>
Expand All @@ -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'

Expand All @@ -37,14 +38,19 @@ export default {
NcCheckboxRadioSwitch,
},

setup() {
return {
t,
}
},

data() {
return {
// By default, system tags creation is not restricted to admins
systemTagsCreationRestrictedToAdmin: loadState<true|false>('settings', 'restrictSystemTagsCreationToAdmin', false) === true,
}
},
methods: {
t,
async updateSystemTagsDefault(isRestricted: boolean) {
try {
const responseData = await updateSystemTagsAdminRestriction(isRestricted)
Expand Down
58 changes: 58 additions & 0 deletions cypress/e2e/systemtags/files-bulk-action.cy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand All @@ -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')
Expand Down Expand Up @@ -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')
Expand Down Expand Up @@ -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')
})
})
})

0 comments on commit 6ef85aa

Please sign in to comment.