From 0e205e428c45b4b23cb903a71f35f7bc7b1fd5aa Mon Sep 17 00:00:00 2001 From: Elizabeth Danzberger Date: Thu, 22 Oct 2026 22:34:40 -0400 Subject: [PATCH 1/5] fix(config): default app value to empty string getAppValueArray() passed an empty array as the default, which IConfig returns verbatim for an unset key and explode() then rejects. Assisted-by: ClaudeCode:claude-opus-5[1m] Signed-off-by: Elizabeth Danzberger --- lib/AppConfig.php | 2 +- tests/lib/AppConfigTest.php | 16 +++++++++++++--- tests/psalm-baseline.xml | 3 --- 3 files changed, 14 insertions(+), 7 deletions(-) diff --git a/lib/AppConfig.php b/lib/AppConfig.php index e03021de9d..6fedaf68d3 100644 --- a/lib/AppConfig.php +++ b/lib/AppConfig.php @@ -93,7 +93,7 @@ public function getAppValue($key, $defaultValue = null) { * @return list|string */ public function getAppValueArray($key) { - $value = $this->config->getAppValue($this->getAppNamespace($key), $key, []); + $value = $this->config->getAppValue($this->getAppNamespace($key), $key, ''); if (array_key_exists($key, self::APP_SETTING_TYPES) && self::APP_SETTING_TYPES[$key] === 'array') { $value = $value !== '' ? explode(',', $value) : []; } diff --git a/tests/lib/AppConfigTest.php b/tests/lib/AppConfigTest.php index 2da1ade055..aade7a8161 100644 --- a/tests/lib/AppConfigTest.php +++ b/tests/lib/AppConfigTest.php @@ -33,7 +33,7 @@ public function setUp(): void { public function testGetAppValueArrayWithValues() { $this->config->expects($this->once()) ->method('getAppValue') - ->with('files', 'watermark_allGroupsList', []) + ->with('files', 'watermark_allGroupsList', '') ->willReturn('1,2,3'); $result = $this->appConfig->getAppValueArray('watermark_allGroupsList'); @@ -44,7 +44,7 @@ public function testGetAppValueArrayWithValues() { public function testGetAppValueArrayWithSingleValue() { $this->config->expects($this->once()) ->method('getAppValue') - ->with('files', 'watermark_allTagsList', []) + ->with('files', 'watermark_allTagsList', '') ->willReturn('1'); $result = $this->appConfig->getAppValueArray('watermark_allTagsList'); @@ -55,7 +55,7 @@ public function testGetAppValueArrayWithSingleValue() { public function testGetAppValueArrayWithNoneValue() { $this->config->expects($this->once()) ->method('getAppValue') - ->with('files', 'watermark_allTagsList', []) + ->with('files', 'watermark_allTagsList', '') ->willReturn(''); $result = $this->appConfig->getAppValueArray('watermark_allTagsList'); @@ -63,6 +63,16 @@ public function testGetAppValueArrayWithNoneValue() { $this->assertSame([], $result); } + public function testGetAppValueArrayWithUnsetKey() { + $this->config->expects($this->once()) + ->method('getAppValue') + ->willReturnCallback(fn ($app, $key, $default) => $default); + + $result = $this->appConfig->getAppValueArray('watermark_allTagsList'); + + $this->assertSame([], $result); + } + public function testGetPreviewConversionTimeoutReturnsDefault(): void { $this->config->expects($this->once()) ->method('getAppValue') diff --git a/tests/psalm-baseline.xml b/tests/psalm-baseline.xml index 9e0fe758a0..1cae694dee 100644 --- a/tests/psalm-baseline.xml +++ b/tests/psalm-baseline.xml @@ -1,9 +1,6 @@ - - - From 135eba7358ee045d376c0a1b366a493d0b71f4c4 Mon Sep 17 00:00:00 2001 From: Elizabeth Danzberger Date: Thu, 22 Oct 2026 22:34:58 -0400 Subject: [PATCH 2/5] refactor(config): consolidate secure view tag keys The keys holding the secure view tag ids were listed in several places. Assisted-by: ClaudeCode:claude-opus-5[1m] Signed-off-by: Elizabeth Danzberger --- lib/AppConfig.php | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/AppConfig.php b/lib/AppConfig.php index 6fedaf68d3..1e6dd2b13b 100644 --- a/lib/AppConfig.php +++ b/lib/AppConfig.php @@ -55,9 +55,10 @@ class AppConfig { 'watermark_linkTagsList' => 'array' ]; - private const INTEGER_LIST_KEYS = [ - 'watermark_allTagsList' => true, - 'watermark_linkTagsList' => true, + /** Config keys holding the system tag ids that enforce secure view */ + public const SECURE_VIEW_TAG_KEYS = [ + 'watermark_allTagsList', + 'watermark_linkTagsList', ]; public function __construct( @@ -130,7 +131,7 @@ public function getAppSettings() { $value = $value === 'yes' ? true : $value; $result[$key] = $value === 'no' ? false : $value; } - if (!empty(self::INTEGER_LIST_KEYS[$key])) { + if (in_array($key, self::SECURE_VIEW_TAG_KEYS, true)) { $result[$key] = array_map('intval', $result[$key] ?? []); } } From 9bb7cefde157b9b09d3cacbc2f5f32df304837c4 Mon Sep 17 00:00:00 2001 From: Elizabeth Danzberger Date: Thu, 22 Oct 2026 22:35:06 -0400 Subject: [PATCH 3/5] fix(secureview): ignore deleted tags in admin settings Tag ids without a matching system tag are dropped before they reach the tag picker, which cannot resolve them and fails to render. Assisted-by: ClaudeCode:claude-opus-5[1m] Signed-off-by: Elizabeth Danzberger --- lib/Settings/Admin.php | 22 +++++- tests/lib/Settings/AdminTest.php | 115 +++++++++++++++++++++++++++++++ 2 files changed, 136 insertions(+), 1 deletion(-) create mode 100644 tests/lib/Settings/AdminTest.php diff --git a/lib/Settings/Admin.php b/lib/Settings/Admin.php index b6c4ad4dc8..0c547aef5c 100644 --- a/lib/Settings/Admin.php +++ b/lib/Settings/Admin.php @@ -16,6 +16,7 @@ use OCP\AppFramework\Http\TemplateResponse; use OCP\IConfig; use OCP\Settings\ISettings; +use OCP\SystemTag\ISystemTagManager; class Admin implements ISettings { public function __construct( @@ -27,6 +28,7 @@ public function __construct( private FontService $fontService, private InitialStateService $initialStateService, private TokenManager $tokenManager, + private ISystemTagManager $systemTagManager, ) { } @@ -50,7 +52,7 @@ public function getForm(): TemplateResponse { 'external_apps' => $this->config->getAppValue('richdocuments', 'external_apps'), 'canonical_webroot' => $this->config->getAppValue('richdocuments', 'canonical_webroot'), 'disable_certificate_verification' => $this->config->getAppValue('richdocuments', 'disable_certificate_verification', '') === 'yes', - 'settings' => $this->appConfig->getAppSettings(), + 'settings' => $this->getAppSettingsWithExistingTags(), 'demo_servers' => $this->demoService->fetchDemoServers(), 'web_server' => strtolower($_SERVER['SERVER_SOFTWARE']), 'os_family' => PHP_VERSION_ID >= 70200 ? PHP_OS_FAMILY : PHP_OS, @@ -67,6 +69,24 @@ public function getForm(): TemplateResponse { ); } + /** + * Tags that no longer exist are dropped from the secure view settings, as + * the tag picker cannot resolve them and fails to render. + */ + private function getAppSettingsWithExistingTags(): array { + $settings = $this->appConfig->getAppSettings(); + $existingTagIds = array_map('intval', array_keys($this->systemTagManager->getAllTags())); + + foreach (AppConfig::SECURE_VIEW_TAG_KEYS as $key) { + if (!isset($settings[$key])) { + continue; + } + $settings[$key] = array_values(array_intersect($settings[$key], $existingTagIds)); + } + + return $settings; + } + #[\Override] public function getSection(): string { return 'richdocuments'; diff --git a/tests/lib/Settings/AdminTest.php b/tests/lib/Settings/AdminTest.php new file mode 100644 index 0000000000..335b986196 --- /dev/null +++ b/tests/lib/Settings/AdminTest.php @@ -0,0 +1,115 @@ +appConfig = $this->createMock(AppConfig::class); + $this->systemTagManager = $this->createMock(ISystemTagManager::class); + + $this->admin = new Admin( + $this->createMock(IConfig::class), + $this->appConfig, + $this->createMock(TemplateManager::class), + $this->createMock(CapabilitiesService::class), + $this->createMock(DemoService::class), + $this->createMock(FontService::class), + $this->createMock(InitialStateService::class), + $this->createMock(TokenManager::class), + $this->systemTagManager, + ); + } + + /** + * @param list $existingTagIds + */ + private function existingTags(array $existingTagIds): void { + $tags = []; + foreach ($existingTagIds as $tagId) { + $tags[$tagId] = $this->createMock(ISystemTag::class); + } + + $this->systemTagManager->method('getAllTags')->willReturn($tags); + } + + private function settingsFromForm(): array { + return $this->admin->getForm()->getParams()['settings']['settings']; + } + + public function testGetFormDropsDeletedTags(): void { + $this->appConfig->method('getAppSettings')->willReturn([ + 'watermark_allTagsList' => [1, 2, 3], + 'watermark_linkTagsList' => [2, 4], + ]); + $this->existingTags([1, 3]); + + $settings = $this->settingsFromForm(); + + self::assertSame([1, 3], $settings['watermark_allTagsList']); + self::assertSame([], $settings['watermark_linkTagsList']); + } + + public function testGetFormKeepsExistingTags(): void { + $this->appConfig->method('getAppSettings')->willReturn([ + 'watermark_allTagsList' => [1, 2], + 'watermark_linkTagsList' => [3], + ]); + $this->existingTags([1, 2, 3]); + + $settings = $this->settingsFromForm(); + + self::assertSame([1, 2], $settings['watermark_allTagsList']); + self::assertSame([3], $settings['watermark_linkTagsList']); + } + + public function testGetFormWithoutConfiguredTagLists(): void { + $this->appConfig->method('getAppSettings')->willReturn([ + 'watermark_enabled' => true, + ]); + $this->existingTags([1]); + + $settings = $this->settingsFromForm(); + + self::assertTrue($settings['watermark_enabled']); + self::assertArrayNotHasKey('watermark_allTagsList', $settings); + self::assertArrayNotHasKey('watermark_linkTagsList', $settings); + } + + public function testGetFormKeepsTagListsIndexedSequentially(): void { + $this->appConfig->method('getAppSettings')->willReturn([ + 'watermark_allTagsList' => [1, 2, 3], + ]); + $this->existingTags([1, 3]); + + self::assertSame([0, 1], array_keys($this->settingsFromForm()['watermark_allTagsList'])); + } +} From 96697de6162dfcb96c66c4dfbb6e1979949fac16 Mon Sep 17 00:00:00 2001 From: Elizabeth Danzberger Date: Thu, 22 Oct 2026 22:35:06 -0400 Subject: [PATCH 4/5] fix(secureview): prune deleted tags from config Assisted-by: ClaudeCode:claude-opus-5[1m] Signed-off-by: Elizabeth Danzberger --- lib/AppInfo/Application.php | 3 + lib/Listener/TagDeletedListener.php | 42 +++++++ tests/lib/Listener/TagDeletedListenerTest.php | 114 ++++++++++++++++++ 3 files changed, 159 insertions(+) create mode 100644 lib/Listener/TagDeletedListener.php create mode 100644 tests/lib/Listener/TagDeletedListenerTest.php diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php index 7ac7f8d59d..5c55f7d091 100644 --- a/lib/AppInfo/Application.php +++ b/lib/AppInfo/Application.php @@ -28,6 +28,7 @@ use OCA\Richdocuments\Listener\ReferenceListener; use OCA\Richdocuments\Listener\RegisterTemplateFileCreatorListener; use OCA\Richdocuments\Listener\ShareLinkListener; +use OCA\Richdocuments\Listener\TagDeletedListener; use OCA\Richdocuments\Middleware\WOPIMiddleware; use OCA\Richdocuments\Notification\Notifier; use OCA\Richdocuments\Preview\EMF; @@ -65,6 +66,7 @@ use OCP\Security\CSP\AddContentSecurityPolicyEvent; use OCP\Security\FeaturePolicy\AddFeaturePolicyEvent; use OCP\Server; +use OCP\SystemTag\ManagerEvent; class Application extends App implements IBootstrap { public const APPNAME = 'richdocuments'; @@ -95,6 +97,7 @@ public function register(IRegistrationContext $context): void { $context->registerEventListener(SabrePluginAddEvent::class, AddSabrePluginListener::class); $context->registerEventListener(BeforeSabrePubliclyLoadedEvent::class, AddSabrePluginListener::class); $context->registerEventListener(AddMissingIndicesEvent::class, AddMissingIndicesListener::class); + $context->registerEventListener(ManagerEvent::EVENT_DELETE, TagDeletedListener::class); $context->registerReferenceProvider(OfficeTargetReferenceProvider::class); $context->registerSensitiveMethods(WopiMapper::class, [ 'getPathForToken', diff --git a/lib/Listener/TagDeletedListener.php b/lib/Listener/TagDeletedListener.php new file mode 100644 index 0000000000..fa483de759 --- /dev/null +++ b/lib/Listener/TagDeletedListener.php @@ -0,0 +1,42 @@ + */ +final class TagDeletedListener implements IEventListener { + + public function __construct( + private AppConfig $appConfig, + ) { + } + + #[\Override] + public function handle(Event $event): void { + if (!$event instanceof ManagerEvent || $event->getEvent() !== ManagerEvent::EVENT_DELETE) { + return; + } + + $tagId = $event->getTag()->getId(); + + foreach (AppConfig::SECURE_VIEW_TAG_KEYS as $key) { + $tagIds = $this->appConfig->getAppValueArray($key); + $remaining = array_values(array_diff($tagIds, [$tagId])); + if ($remaining === $tagIds) { + continue; + } + $this->appConfig->setAppValue($key, implode(',', $remaining)); + } + } +} diff --git a/tests/lib/Listener/TagDeletedListenerTest.php b/tests/lib/Listener/TagDeletedListenerTest.php new file mode 100644 index 0000000000..f457863186 --- /dev/null +++ b/tests/lib/Listener/TagDeletedListenerTest.php @@ -0,0 +1,114 @@ +appConfig = $this->createMock(AppConfig::class); + + $this->listener = new TagDeletedListener($this->appConfig); + } + + private function deletionOf(string $tagId): ManagerEvent { + return $this->tagEvent(ManagerEvent::EVENT_DELETE, $tagId); + } + + private function tagEvent(string $eventName, string $tagId): ManagerEvent { + $tag = $this->createMock(ISystemTag::class); + $tag->method('getId')->willReturn($tagId); + + return new ManagerEvent($eventName, $tag); + } + + private function configuredTagLists(array $allTags, array $linkTags): void { + $this->appConfig->method('getAppValueArray') + ->willReturnMap([ + ['watermark_allTagsList', $allTags], + ['watermark_linkTagsList', $linkTags], + ]); + } + + public function testHandleRemovesTagFromBothLists(): void { + $this->configuredTagLists(['1', '2', '3'], ['2', '4']); + + $written = []; + $this->appConfig->expects(self::exactly(2)) + ->method('setAppValue') + ->willReturnCallback(function (string $key, string $value) use (&$written): void { + $written[$key] = $value; + }); + + $this->listener->handle($this->deletionOf('2')); + + self::assertSame([ + 'watermark_allTagsList' => '1,3', + 'watermark_linkTagsList' => '4', + ], $written); + } + + public function testHandleOnlyWritesTheListContainingTheTag(): void { + $this->configuredTagLists(['1', '2', '3'], ['4']); + + $this->appConfig->expects(self::once()) + ->method('setAppValue') + ->with('watermark_allTagsList', '1,3'); + + $this->listener->handle($this->deletionOf('2')); + } + + public function testHandleKeepsConfigUntouchedForUnusedTag(): void { + $this->configuredTagLists(['1', '2'], ['3']); + + $this->appConfig->expects(self::never()) + ->method('setAppValue'); + + $this->listener->handle($this->deletionOf('4')); + } + + public function testHandleWithoutConfiguredTagLists(): void { + $this->configuredTagLists([], []); + + $this->appConfig->expects(self::never()) + ->method('setAppValue'); + + $this->listener->handle($this->deletionOf('1')); + } + + public function testHandleIgnoresOtherTagManagerEvents(): void { + $this->configuredTagLists(['1', '2'], ['1']); + + $this->appConfig->expects(self::never()) + ->method('setAppValue'); + + $this->listener->handle($this->tagEvent(ManagerEvent::EVENT_CREATE, '1')); + $this->listener->handle($this->tagEvent(ManagerEvent::EVENT_UPDATE, '1')); + } + + public function testHandleIgnoresUnrelatedEvent(): void { + $this->appConfig->expects(self::never()) + ->method('getAppValueArray'); + $this->appConfig->expects(self::never()) + ->method('setAppValue'); + + $this->listener->handle(new Event()); + } +} From 1a113e5a876b76f89937437c6bb96afc7dd51ecf Mon Sep 17 00:00:00 2001 From: Elizabeth Danzberger Date: Thu, 22 Oct 2026 22:35:06 -0400 Subject: [PATCH 5/5] test(secureview): cover deleted tags in settings Assisted-by: ClaudeCode:claude-opus-5[1m] Signed-off-by: Elizabeth Danzberger --- cypress/e2e/settings.spec.js | 143 +++++++++++++++++++++++++++++++++++ 1 file changed, 143 insertions(+) diff --git a/cypress/e2e/settings.spec.js b/cypress/e2e/settings.spec.js index 3b01fdda26..5cfbca5e8e 100644 --- a/cypress/e2e/settings.spec.js +++ b/cypress/e2e/settings.spec.js @@ -78,3 +78,146 @@ describe('Office admin settings', function() { .should('be.visible') }) }) + +const serverUrl = Cypress.config('baseUrl').replace(/\/index.php\/?$/g, '') +const davUrl = serverUrl + '/remote.php/dav/systemtags/' +const watermarkUrl = serverUrl + '/index.php/apps/richdocuments/settings/watermark' + +/** + * Create a system tag and resolve its id + * + * @param {string} name the tag name + * @return {Cypress.Chainable} the id of the created tag + */ +function createSystemTag(name) { + return cy.request('/csrftoken') + .then(({ body }) => body.token) + .then((requesttoken) => cy.request({ + method: 'POST', + url: davUrl, + headers: { requesttoken, 'Content-Type': 'application/json' }, + body: { name, userVisible: true, userAssignable: true }, + })) + .then(({ headers }) => Number.parseInt(headers['content-location'].split('/').pop(), 10)) +} + +/** + * Delete a system tag + * + * @param {number} tagId the id of the tag to delete + */ +function deleteSystemTag(tagId) { + cy.request('/csrftoken') + .then(({ body }) => body.token) + .then((requesttoken) => cy.request({ + method: 'DELETE', + url: davUrl + tagId, + headers: { requesttoken }, + failOnStatusCode: false, + })) +} + +describe('Secure view tag enforcement', function() { + let tagId + + beforeEach(function() { + cy.login(new User('admin', 'admin')) + cy.intercept({ + method: 'POST', + url: '/index.php/apps/richdocuments/ajax/admin.php', + }).as('updateSettings') + cy.intercept({ + method: 'POST', + url: '/index.php/apps/richdocuments/settings/watermark', + }).as('updateWatermark') + + createSystemTag('secured').then((id) => { + tagId = id + }) + + cy.visit('/settings/admin/richdocuments') + cy.get('#wopi_url') + .clear() + .type(collaboraUrl + '{enter}') + cy.wait('@updateSettings').its('response.statusCode').should('equal', 200) + cy.contains('.notecard', 'Collabora Online server is reachable.') + cy.get('#secure-view-settings').scrollIntoView().should('be.visible') + }) + + afterEach(function() { + deleteSystemTag(tagId) + resetWatermarkSettings() + }) + + /** + * Restore the secure view settings to their defaults + */ + function resetWatermarkSettings() { + cy.request('/csrftoken') + .then(({ body }) => body.token) + .then((requesttoken) => cy.request({ + method: 'POST', + url: watermarkUrl, + headers: { requesttoken }, + body: { settings: { watermark: { enabled: false, allTags: false, allTagsList: [] } } }, + })) + } + + /** + * The tag dropdown, only rendered once secure view is enforced on tagged files + * + * @return {Cypress.Chainable} the dropdown element + */ + function tagSelect() { + return cy.get('#secure-view-settings .checkbox-details .v-select').first() + } + + /** + * Enable secure view and enforce it on the tag created for this test + */ + function enforceSecureViewOnTag() { + cy.contains('.settings-entry', 'Enable secure view') + .find('input[type=checkbox]') + .check({ force: true }) + cy.wait('@updateWatermark') + + cy.contains('.settings-entry', 'Enforce secure view on tagged files') + .find('input[type=checkbox]') + .check({ force: true }) + cy.wait('@updateWatermark') + + tagSelect().click() + cy.contains('.vs__dropdown-menu li', 'secured').click() + cy.wait('@updateWatermark') + tagSelect().find('.vs__selected').should('contain', 'secured') + } + + it('Keeps the tag dropdown usable after an enforced tag is deleted', function() { + enforceSecureViewOnTag() + + deleteSystemTag(tagId) + cy.reload() + + tagSelect().find('.vs__selected').should('not.exist') + + tagSelect().click() + cy.get('.vs__dropdown-menu').should('be.visible') + }) + + it('Allows enforcing another tag after the enforced tag is deleted', function() { + enforceSecureViewOnTag() + + deleteSystemTag(tagId) + createSystemTag('replacement').then((id) => { + tagId = id + }) + cy.reload() + + tagSelect().click() + cy.contains('.vs__dropdown-menu li', 'replacement').click() + cy.wait('@updateWatermark').its('response.statusCode').should('equal', 200) + + cy.reload() + tagSelect().find('.vs__selected').should('contain', 'replacement') + }) +})