From 4771fb9422da8543ff30cbb26a68f1d4ef6ad73b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Maia?= Date: Mon, 5 Oct 2026 22:23:32 -0300 Subject: [PATCH 1/4] fix(policy): keep an explicitly saved default as an enforceable system rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An administrator could not enforce the built-in default of a policy. Saving the default with allowChildOverride = false deleted the system rule, and reading an explicit default always returned an overridable layer, so a group or user rule still changed the effective value. - PolicySource::saveSystemPolicy() stores every explicit value with its override flag, including a value equal to the default. - PolicySource::loadSystemPolicy() applies the stored flag to an explicit default. A default stored without a flag predates explicit defaults, when the API could not enforce one, so it stays overridable. - PolicyService::saveSystem() treats null as "no explicit system rule" and clears it, for single and compound writes. Policy Workbench already removes a rule this way, so it needs no change. The API documentation of both system write endpoints now states the difference between null and an explicit default. Resolves: #8916 Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: André Maia --- lib/Controller/PolicyController.php | 4 +- lib/Service/Policy/PolicyService.php | 14 ++- lib/Service/Policy/Runtime/PolicySource.php | 52 ++------- .../policies/boolean_policy_layers.feature | 43 +++++++ .../Unit/Service/Policy/PolicyServiceTest.php | 60 ++++++++++ .../Policy/Runtime/PolicySourceTest.php | 106 ++++++++++++++---- 6 files changed, 210 insertions(+), 69 deletions(-) diff --git a/lib/Controller/PolicyController.php b/lib/Controller/PolicyController.php index 52bd9a49d4..16d139df28 100644 --- a/lib/Controller/PolicyController.php +++ b/lib/Controller/PolicyController.php @@ -244,7 +244,7 @@ public function listUserPolicies(string $policyKey): DataResponse { * Save a system-level policy value * * @param string $policyKey Policy identifier to persist at the system layer. - * @param null|bool|int|float|string|array $value Policy value to persist. Null resets the policy to its default system value. + * @param null|bool|int|float|string|array $value Policy value to persist as an explicit system rule, even when it equals the default. Null removes the explicit system rule: the built-in default applies and lower layers may override it. * @param bool $allowChildOverride Whether lower layers may override this system default. * @return DataResponse|DataResponse * @@ -336,7 +336,7 @@ public function setGroup(string $groupId, string $policyKey, null|bool|int|float * them is stored, so the result does not depend on the order of the values. * * @param string $parentPolicyKey Policy identifier the other settings are grouped under. - * @param array> $values Values to persist, keyed by policy identifier. Null resets that policy to its default system value. + * @param array> $values Values to persist, keyed by policy identifier, each as an explicit system rule even when it equals the default. Null removes the explicit system rule of that policy: the built-in default applies and lower layers may override it. * @param array $allowChildOverride Whether lower layers may override each saved value, keyed by policy identifier. * @return DataResponse|DataResponse * diff --git a/lib/Service/Policy/PolicyService.php b/lib/Service/Policy/PolicyService.php index a1991173ad..b201f03eb3 100644 --- a/lib/Service/Policy/PolicyService.php +++ b/lib/Service/Policy/PolicyService.php @@ -167,13 +167,21 @@ public function listUserPolicies(string|\BackedEnum $policyKey): array { return $this->source->listUserPoliciesByKey($definition->key()); } + /** + * A null value removes the explicit system rule, so the built-in default + * applies and lower layers may override it. Any other value, including one + * equal to the default, is stored as an explicit rule with its own + * override flag. + */ public function saveSystem(string|\BackedEnum $policyKey, mixed $value, bool $allowChildOverride = false): ResolvedPolicy { + if ($value === null) { + return $this->clearSystem($policyKey); + } + $context = $this->contextFactory->forCurrentUser(); $definition = $this->registry->get($policyKey); $this->assertScopeSupported($definition, PolicySpec::SCOPE_SYSTEM); - $normalizedValue = $value === null - ? $definition->normalizeValue($definition->defaultSystemValue()) - : $definition->normalizeValue($value); + $normalizedValue = $definition->normalizeValue($value); $definition->validateValueForPersistence($normalizedValue, $context); $this->source->saveSystemPolicy($definition->key(), $normalizedValue, $allowChildOverride); diff --git a/lib/Service/Policy/Runtime/PolicySource.php b/lib/Service/Policy/Runtime/PolicySource.php index 837f7edbd0..6297161bd1 100644 --- a/lib/Service/Policy/Runtime/PolicySource.php +++ b/lib/Service/Policy/Runtime/PolicySource.php @@ -65,26 +65,14 @@ public function loadSystemPolicy(string $policyKey): ?PolicyLayer { return $layer->setAllowChildOverride(true); } - if ($value === $defaultValue) { - $allowChildOverride = $this->appConfig->getAppValueString( - $this->getSystemAllowOverrideConfigKey($definition->getAppConfigKey()), - '0', - ) === '1'; - - if ($allowChildOverride) { - // Explicitly persisted default value ("let users choose") - return $layer - ->setAllowChildOverride(true) - ->setAllowedValues([]); - } - + // A default value stored without an override flag predates explicit + // defaults: the API could not enforce it then, so it stays overridable. + $allowOverrideConfigKey = $this->getSystemAllowOverrideConfigKey($definition->getAppConfigKey()); + if ($value === $defaultValue && !$this->appConfig->hasAppKey($allowOverrideConfigKey)) { return $layer->setAllowChildOverride(true); } - $allowChildOverride = $this->appConfig->getAppValueString( - $this->getSystemAllowOverrideConfigKey($definition->getAppConfigKey()), - '0', - ) === '1'; + $allowChildOverride = $this->appConfig->getAppValueString($allowOverrideConfigKey, '0') === '1'; return $layer ->setAllowChildOverride($allowChildOverride) @@ -672,31 +660,11 @@ private function hydrateExplicitSystemRuleCounts(array $policyKeys, array &$coun #[\Override] public function saveSystemPolicy(string $policyKey, mixed $value, bool $allowChildOverride = false): void { $definition = $this->registry->get($policyKey); - $normalizedValue = $definition->normalizeValue($value); - $defaultValue = $definition->normalizeValue($definition->defaultSystemValue()); - $allowOverrideConfigKey = $this->getSystemAllowOverrideConfigKey($definition->getAppConfigKey()); - - $valuesAreEqual = $normalizedValue === $defaultValue; - if (!$valuesAreEqual && is_string($normalizedValue) && is_string($defaultValue)) { - $d1 = json_decode($normalizedValue, true); - $d2 = json_decode($defaultValue, true); - if (is_array($d1) && is_array($d2)) { - $valuesAreEqual = $d1 === $d2; - } - } - if ($valuesAreEqual) { - if ($allowChildOverride) { - $this->writeSystemValue($definition->getAppConfigKey(), $normalizedValue); - $this->appConfig->setAppValueString($allowOverrideConfigKey, '1'); - return; - } - - $this->clearSystemPolicy($policyKey); - return; - } - - $this->writeSystemValue($definition->getAppConfigKey(), $normalizedValue); - $this->appConfig->setAppValueString($allowOverrideConfigKey, $allowChildOverride ? '1' : '0'); + $this->writeSystemValue($definition->getAppConfigKey(), $definition->normalizeValue($value)); + $this->appConfig->setAppValueString( + $this->getSystemAllowOverrideConfigKey($definition->getAppConfigKey()), + $allowChildOverride ? '1' : '0', + ); } #[\Override] diff --git a/tests/integration/features/policies/boolean_policy_layers.feature b/tests/integration/features/policies/boolean_policy_layers.feature index be77b17ae0..1fbbe43e0d 100644 --- a/tests/integration/features/policies/boolean_policy_layers.feature +++ b/tests/integration/features/policies/boolean_policy_layers.feature @@ -69,3 +69,46 @@ Feature: policies/boolean_policy_layers And the response should be a JSON array with the following mandatory values | key | value | | (jq).ocs.data.error | Not allowed to manage this user policy | + + Scenario: An explicitly saved default can be enforced and a null value removes it + Given as user "admin" + And user "signer1" exists + And sending "delete" to ocs "/apps/libresign/api/v1/policies/user/signer1/show_confetti_after_signing" + And the response should have a status code 200 + + When sending "post" to ocs "/apps/libresign/api/v1/policies/system/show_confetti_after_signing" + | value | true | + | allowChildOverride | false | + Then the response should have a status code 200 + And the response should be a JSON array with the following mandatory values + | key | value | + | (jq).ocs.data.policy.effectiveValue | true | + + When sending "put" to ocs "/apps/libresign/api/v1/policies/user/signer1/show_confetti_after_signing" + | value | false | + Then the response should have a status code 200 + + Given as user "signer1" + When sending "get" to ocs "/apps/libresign/api/v1/policies/effective" + Then the response should have a status code 200 + And the response should be a JSON array with the following mandatory values + | key | value | + | (jq).ocs.data.policies.show_confetti_after_signing.effectiveValue | true | + | (jq).ocs.data.policies.show_confetti_after_signing.sourceScope | global | + + Given as user "admin" + When sending "post" to ocs "/apps/libresign/api/v1/policies/system/show_confetti_after_signing" + | value | null | + Then the response should have a status code 200 + + Given as user "signer1" + When sending "get" to ocs "/apps/libresign/api/v1/policies/effective" + Then the response should have a status code 200 + And the response should be a JSON array with the following mandatory values + | key | value | + | (jq).ocs.data.policies.show_confetti_after_signing.effectiveValue | false | + | (jq).ocs.data.policies.show_confetti_after_signing.sourceScope | user_policy | + + Given as user "admin" + And sending "delete" to ocs "/apps/libresign/api/v1/policies/user/signer1/show_confetti_after_signing" + And the response should have a status code 200 diff --git a/tests/php/Unit/Service/Policy/PolicyServiceTest.php b/tests/php/Unit/Service/Policy/PolicyServiceTest.php index aaf5636496..314f41cbc8 100644 --- a/tests/php/Unit/Service/Policy/PolicyServiceTest.php +++ b/tests/php/Unit/Service/Policy/PolicyServiceTest.php @@ -2427,6 +2427,66 @@ public function testASingleKeyWriteDoesNotDependOnTheOtherSettings(): void { $this->newService()->saveSystem(SignatureRejectionPolicy::KEY_COMMENT_VISIBILITY, 'public'); } + /** + * A null value means there is no explicit system rule: the built-in + * default applies and lower layers may override it. + */ + public function testSavingNullRemovesTheExplicitSystemRule(): void { + $this->givenSystemRejectionPolicy([]); + $this->source + ->expects($this->once()) + ->method('clearSystemPolicy') + ->with(SignatureRejectionPolicy::KEY_VISIBILITY); + $this->source->expects($this->never())->method('saveSystemPolicy'); + + $this->newService()->saveSystem(SignatureRejectionPolicy::KEY_VISIBILITY, null); + } + + /** + * A value equal to the built-in default is still an explicit rule, with + * its own override flag. + */ + public function testSavingTheDefaultValueKeepsAnExplicitSystemRule(): void { + $this->givenSystemRejectionPolicy([]); + $this->source + ->expects($this->once()) + ->method('saveSystemPolicy') + ->with(SignatureRejectionPolicy::KEY_VISIBILITY, 'requester', false); + $this->source->expects($this->never())->method('clearSystemPolicy'); + + $this->newService()->saveSystem(SignatureRejectionPolicy::KEY_VISIBILITY, 'requester'); + } + + public function testACompoundWriteTellsANullApartFromAnExplicitDefault(): void { + $this->givenSystemRejectionPolicy([ + SignatureRejectionPolicy::KEY_ENABLED => true, + SignatureRejectionPolicy::KEY_VISIBILITY => 'public', + SignatureRejectionPolicy::KEY_COMMENT_MODE => 'optional', + SignatureRejectionPolicy::KEY_COMMENT_VISIBILITY => 'public', + ]); + $this->source + ->expects($this->once()) + ->method('clearSystemPolicy') + ->with(SignatureRejectionPolicy::KEY_VISIBILITY); + $this->source + ->expects($this->once()) + ->method('saveSystemPolicy') + ->with(SignatureRejectionPolicy::KEY_COMMENT_VISIBILITY, 'requester', false); + + $saved = $this->newService()->saveSystemCompound(SignatureRejectionPolicy::KEY_ENABLED, [ + SignatureRejectionPolicy::KEY_VISIBILITY => null, + SignatureRejectionPolicy::KEY_COMMENT_VISIBILITY => 'requester', + ], [ + SignatureRejectionPolicy::KEY_VISIBILITY => false, + SignatureRejectionPolicy::KEY_COMMENT_VISIBILITY => false, + ]); + + $this->assertSame( + [SignatureRejectionPolicy::KEY_VISIBILITY, SignatureRejectionPolicy::KEY_COMMENT_VISIBILITY], + array_keys($saved), + ); + } + public function testACompoundWriteIsPersistedAsASingleTransaction(): void { $this->givenSystemRejectionPolicy([ SignatureRejectionPolicy::KEY_ENABLED => true, diff --git a/tests/php/Unit/Service/Policy/Runtime/PolicySourceTest.php b/tests/php/Unit/Service/Policy/Runtime/PolicySourceTest.php index 5949bf7d85..c37f5714e3 100644 --- a/tests/php/Unit/Service/Policy/Runtime/PolicySourceTest.php +++ b/tests/php/Unit/Service/Policy/Runtime/PolicySourceTest.php @@ -15,7 +15,9 @@ use OCA\Libresign\Db\PermissionSetBindingMapper; use OCA\Libresign\Db\PermissionSetMapper; use OCA\Libresign\Service\IdentifyMethodService; +use OCA\Libresign\Service\Policy\Contract\IPolicySource; use OCA\Libresign\Service\Policy\Model\PolicyContext; +use OCA\Libresign\Service\Policy\Model\PolicyLayer; use OCA\Libresign\Service\Policy\Provider\ApprovalGroups\ApprovalGroupsPolicy; use OCA\Libresign\Service\Policy\Provider\CollectMetadata\CollectMetadataPolicy; use OCA\Libresign\Service\Policy\Provider\Confetti\ConfettiPolicy; @@ -43,6 +45,7 @@ use OCA\Libresign\Service\Policy\Provider\ValidationAccess\ValidationAccessPolicy; use OCA\Libresign\Service\Policy\Provider\Worker\SigningModePolicy; use OCA\Libresign\Service\Policy\Provider\Worker\WorkerConfigPolicy; +use OCA\Libresign\Service\Policy\Runtime\DefaultPolicyResolver; use OCA\Libresign\Service\Policy\Runtime\PolicyRegistry; use OCA\Libresign\Service\Policy\Runtime\PolicySource; use OCA\Libresign\Tests\Unit\TestCase; @@ -620,15 +623,15 @@ public function testListGroupPoliciesByKeyForTargetsScopesBindingsToProvidedGrou $this->assertSame('ordered_numeric', $result[0]['policy']->getValue()); } - public function testSaveSystemPolicyDeletesAppConfigWhenValueMatchesDefault(): void { + public function testSaveSystemPolicyPersistsAnEnforcedDefault(): void { $this->setStoredAppConfigString('policy.signature_flow.system', 'ordered_numeric'); $this->setStoredAppConfigString('policy.signature_flow.system.allow_child_override', '1'); $source = $this->getSource(); $source->saveSystemPolicy('signature_flow', 'none'); - $this->assertAppConfigMissing('policy.signature_flow.system'); - $this->assertAppConfigMissing('policy.signature_flow.system.allow_child_override'); + $this->assertStoredAppConfigString('policy.signature_flow.system', 'none'); + $this->assertStoredAppConfigString('policy.signature_flow.system.allow_child_override', '0'); } public function testClearSystemPolicyDeletesStoredSystemConfig(): void { @@ -658,7 +661,7 @@ public function testSaveSystemPolicyNormalizesAndPersistsAppConfigValue(): void $this->assertStoredAppConfigString('policy.signature_flow.system.allow_child_override', '1'); } - public function testSaveSystemPolicyDeletesIdentifyMethodsWhenValueMatchesServiceDefault(): void { + public function testSaveSystemPolicyKeepsIdentifyMethodsEqualToTheServiceDefaultAsAnExplicitRule(): void { $this->setStoredAppConfigArray(IdentifyMethodsPolicy::SYSTEM_APP_CONFIG_KEY, [ [ 'name' => 'account', @@ -685,8 +688,8 @@ public function testSaveSystemPolicyDeletesIdentifyMethodsWhenValueMatchesServic false, ); - $this->assertAppConfigMissing(IdentifyMethodsPolicy::SYSTEM_APP_CONFIG_KEY); - $this->assertAppConfigMissing(IdentifyMethodsPolicy::SYSTEM_APP_CONFIG_KEY . '.allow_child_override'); + $this->assertTrue($this->coreAppConfig->hasKey(Application::APP_ID, IdentifyMethodsPolicy::SYSTEM_APP_CONFIG_KEY)); + $this->assertStoredAppConfigString(IdentifyMethodsPolicy::SYSTEM_APP_CONFIG_KEY . '.allow_child_override', '0'); } #[DataProvider('providerSaveSystemPolicyBusinessRules')] @@ -694,35 +697,25 @@ public function testSaveSystemPolicyBusinessRulesWithDataProvider( string $policyKey, string $inputValue, bool $allowChildOverride, - bool $expectDelete, string $expectedValue, string $expectedAppConfigKey, string $expectedAllowOverrideValue, ): void { $source = $this->getSource(); - if ($expectDelete) { - $source->saveSystemPolicy($policyKey, $inputValue, $allowChildOverride); - - $this->assertAppConfigMissing($expectedAppConfigKey); - $this->assertAppConfigMissing($expectedAppConfigKey . '.allow_child_override'); - return; - } - $source->saveSystemPolicy($policyKey, $inputValue, $allowChildOverride); $this->assertStoredAppConfigString($expectedAppConfigKey, $expectedValue); $this->assertStoredAppConfigString($expectedAppConfigKey . '.allow_child_override', $expectedAllowOverrideValue); } - /** @return array */ + /** @return array */ public static function providerSaveSystemPolicyBusinessRules(): array { return [ - 'deletes_when_value_matches_default_and_override_disabled' => [ + 'persists_explicit_default_when_override_disabled' => [ SignatureFlowPolicy::KEY, 'none', false, - true, 'none', 'policy.signature_flow.system', '0', @@ -731,16 +724,14 @@ public static function providerSaveSystemPolicyBusinessRules(): array { SignatureFlowPolicy::KEY, 'none', true, - false, 'none', 'policy.signature_flow.system', '1', ], - 'deletes_when_json_is_semantically_equal_to_default' => [ + 'persists_json_semantically_equal_to_default' => [ ApprovalGroupsPolicy::KEY, '[ "admin" ]', false, - true, '["admin"]', ApprovalGroupsPolicy::SYSTEM_APP_CONFIG_KEY, '0', @@ -749,7 +740,6 @@ public static function providerSaveSystemPolicyBusinessRules(): array { SignatureFlowPolicy::KEY, 'ordered_numeric', false, - false, 'ordered_numeric', 'policy.signature_flow.system', '0', @@ -757,6 +747,78 @@ public static function providerSaveSystemPolicyBusinessRules(): array { ]; } + /** + * The stored system rule decides whether a lower layer that disagrees + * changes the effective value; checking the layer alone would not prove it. + */ + #[DataProvider('providerExplicitSystemRules')] + public function testTheSystemRuleDecidesWhetherALowerLayerChangesTheEffectiveValue( + ?string $storedValue, + ?string $storedAllowChildOverride, + string $conflictingScope, + string $expectedEffectiveValue, + ): void { + if ($storedValue !== null) { + $this->setStoredAppConfigString('policy.signature_flow.system', $storedValue); + } + if ($storedAllowChildOverride !== null) { + $this->setStoredAppConfigString('policy.signature_flow.system.allow_child_override', $storedAllowChildOverride); + } + $conflictingLayer = (new PolicyLayer()) + ->setScope($conflictingScope === 'group' ? 'group' : 'user') + ->setValue('ordered_numeric') + ->setAllowChildOverride(true) + ->setVisibleToChild(true); + + $source = $this->createMock(IPolicySource::class); + $source->method('loadSystemPolicy')->willReturn($this->getSource()->loadSystemPolicy(SignatureFlowPolicy::KEY)); + $source->method('loadGroupPolicies')->willReturn($conflictingScope === 'group' ? [$conflictingLayer] : []); + $source->method('loadUserPolicy')->willReturn($conflictingScope === 'user_policy' ? $conflictingLayer : null); + + $resolved = (new DefaultPolicyResolver($source))->resolve( + $this->registry->get(SignatureFlowPolicy::KEY), + PolicyContext::fromUserId('john'), + ); + + $this->assertSame($expectedEffectiveValue, $resolved->getEffectiveValue()); + } + + /** @return iterable */ + public static function providerExplicitSystemRules(): iterable { + foreach (['group', 'user_policy'] as $conflictingScope) { + yield "no explicit rule, $conflictingScope wins" => [null, null, $conflictingScope, 'ordered_numeric']; + yield "explicit default enforced, $conflictingScope ignored" => ['none', '0', $conflictingScope, 'none']; + yield "explicit default overridable, $conflictingScope wins" => ['none', '1', $conflictingScope, 'ordered_numeric']; + yield "explicit value enforced, $conflictingScope ignored" => ['parallel', '0', $conflictingScope, 'parallel']; + yield "explicit value overridable, $conflictingScope wins" => ['parallel', '1', $conflictingScope, 'ordered_numeric']; + yield "legacy default without override flag, $conflictingScope wins" => ['none', null, $conflictingScope, 'ordered_numeric']; + } + } + + public function testAnEnforcedDefaultSavedThroughTheSourceIsReadBackEnforced(): void { + $source = $this->getSource(); + $source->saveSystemPolicy(SignatureFlowPolicy::KEY, 'none', false); + + $layer = $source->loadSystemPolicy(SignatureFlowPolicy::KEY); + + $this->assertNotNull($layer); + $this->assertSame('global', $layer->getScope()); + $this->assertSame('none', $layer->getValue()); + $this->assertFalse($layer->isAllowChildOverride()); + $this->assertSame(['none'], $layer->getAllowedValues()); + } + + public function testALegacyDefaultWithoutOverrideFlagStaysOverridable(): void { + $this->setStoredAppConfigString('policy.signature_flow.system', 'none'); + + $layer = $this->getSource()->loadSystemPolicy(SignatureFlowPolicy::KEY); + + $this->assertNotNull($layer); + $this->assertSame('global', $layer->getScope()); + $this->assertTrue($layer->isAllowChildOverride()); + $this->assertSame([], $layer->getAllowedValues()); + } + public function testLoadSystemPolicyRespectsPersistedAllowChildOverride(): void { $this->setStoredAppConfigString('policy.signature_flow.system', 'ordered_numeric'); $this->setStoredAppConfigString('policy.signature_flow.system.allow_child_override', '1'); From 07ac1765c6eeece5497909dc0789589849efef43 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Maia?= Date: Mon, 5 Oct 2026 22:23:32 -0300 Subject: [PATCH 2/4] chore(openapi): regenerate the spec for the system policy null contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Part of #8916 Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: André Maia --- openapi-administration.json | 4 ++-- openapi-full.json | 4 ++-- src/types/openapi/openapi-administration.ts | 4 ++-- src/types/openapi/openapi-full.ts | 4 ++-- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/openapi-administration.json b/openapi-administration.json index 33820cb38b..6df2a5cbe5 100644 --- a/openapi-administration.json +++ b/openapi-administration.json @@ -2816,7 +2816,7 @@ "properties": { "value": { "nullable": true, - "description": "Policy value to persist. Null resets the policy to its default system value.", + "description": "Policy value to persist as an explicit system rule, even when it equals the default. Null removes the explicit system rule: the built-in default applies and lower layers may override it.", "anyOf": [ { "type": "boolean" @@ -2974,7 +2974,7 @@ "values": { "type": "object", "default": {}, - "description": "Values to persist, keyed by policy identifier. Null resets that policy to its default system value.", + "description": "Values to persist, keyed by policy identifier, each as an explicit system rule even when it equals the default. Null removes the explicit system rule of that policy: the built-in default applies and lower layers may override it.", "additionalProperties": { "nullable": true, "anyOf": [ diff --git a/openapi-full.json b/openapi-full.json index 310cb1cb7d..19c48d970a 100644 --- a/openapi-full.json +++ b/openapi-full.json @@ -15329,7 +15329,7 @@ "properties": { "value": { "nullable": true, - "description": "Policy value to persist. Null resets the policy to its default system value.", + "description": "Policy value to persist as an explicit system rule, even when it equals the default. Null removes the explicit system rule: the built-in default applies and lower layers may override it.", "anyOf": [ { "type": "boolean" @@ -15487,7 +15487,7 @@ "values": { "type": "object", "default": {}, - "description": "Values to persist, keyed by policy identifier. Null resets that policy to its default system value.", + "description": "Values to persist, keyed by policy identifier, each as an explicit system rule even when it equals the default. Null removes the explicit system rule of that policy: the built-in default applies and lower layers may override it.", "additionalProperties": { "nullable": true, "anyOf": [ diff --git a/src/types/openapi/openapi-administration.ts b/src/types/openapi/openapi-administration.ts index 6f93d643ac..bf104c9cce 100644 --- a/src/types/openapi/openapi-administration.ts +++ b/src/types/openapi/openapi-administration.ts @@ -1349,7 +1349,7 @@ export interface operations { requestBody?: { content: { "application/json": { - /** @description Policy value to persist. Null resets the policy to its default system value. */ + /** @description Policy value to persist as an explicit system rule, even when it equals the default. Null removes the explicit system rule: the built-in default applies and lower layers may override it. */ value?: (boolean | number | string | { [key: string]: Record; }) | null; @@ -1410,7 +1410,7 @@ export interface operations { content: { "application/json": { /** - * @description Values to persist, keyed by policy identifier. Null resets that policy to its default system value. + * @description Values to persist, keyed by policy identifier, each as an explicit system rule even when it equals the default. Null removes the explicit system rule of that policy: the built-in default applies and lower layers may override it. * @default {} */ values?: { diff --git a/src/types/openapi/openapi-full.ts b/src/types/openapi/openapi-full.ts index 8c2b8639b2..36d407c87c 100644 --- a/src/types/openapi/openapi-full.ts +++ b/src/types/openapi/openapi-full.ts @@ -7139,7 +7139,7 @@ export interface operations { requestBody?: { content: { "application/json": { - /** @description Policy value to persist. Null resets the policy to its default system value. */ + /** @description Policy value to persist as an explicit system rule, even when it equals the default. Null removes the explicit system rule: the built-in default applies and lower layers may override it. */ value?: (boolean | number | string | { [key: string]: Record; }) | null; @@ -7200,7 +7200,7 @@ export interface operations { content: { "application/json": { /** - * @description Values to persist, keyed by policy identifier. Null resets that policy to its default system value. + * @description Values to persist, keyed by policy identifier, each as an explicit system rule even when it equals the default. Null removes the explicit system rule of that policy: the built-in default applies and lower layers may override it. * @default {} */ values?: { From 6775af2abb40f3e35c2fbc5bd7d4462ce23ff45e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Maia?= Date: Tue, 6 Oct 2026 21:35:36 -0300 Subject: [PATCH 3/4] test(policy): reset system rules with null instead of the default value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Saving a value equal to the default now keeps it as an explicit system rule, so the admin initial state scenarios and the validation access end-to-end test can no longer use the default value to clear the rule. They send null, which is how the workbench removes a system rule. Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: André Maia --- ...-validation-access-rule-management.spec.ts | 4 +-- .../features/admin/initial_state.feature | 32 +++++++++---------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/playwright/e2e/policy-workbench-validation-access-rule-management.spec.ts b/playwright/e2e/policy-workbench-validation-access-rule-management.spec.ts index 903161bb72..e87b1a0b2d 100644 --- a/playwright/e2e/policy-workbench-validation-access-rule-management.spec.ts +++ b/playwright/e2e/policy-workbench-validation-access-rule-management.spec.ts @@ -13,10 +13,10 @@ test.describe.configure({ mode: 'serial', retries: 0, timeout: 120000 }) test.beforeAll(async () => { const ctx = await makeAdminContext() - // Reset make_validation_url_private to its default value so the "Everyone" + // Remove any system rule for make_validation_url_private so the "Everyone" // scope option is available in the workbench UI even when the test is re-run. await ctx.post('./ocs/v2.php/apps/libresign/api/v1/policies/system/make_validation_url_private', { - data: { value: false, allowChildOverride: false }, + data: { value: null }, failOnStatusCode: false, }) await ctx.dispose() diff --git a/tests/integration/features/admin/initial_state.feature b/tests/integration/features/admin/initial_state.feature index 0aa0d59214..d76f8872e3 100644 --- a/tests/integration/features/admin/initial_state.feature +++ b/tests/integration/features/admin/initial_state.feature @@ -6,7 +6,7 @@ Feature: admin/initial_state And sending "delete" to ocs "/apps/libresign/api/v1/policies/user/admin/identify_methods" And the response should have a status code 200 And sending "post" to ocs "/apps/libresign/api/v1/policies/system/identify_methods" - | value | (string){"factors":[]} | + | value | null | And the response should have a status code 200 When sending "get" to "/settings/admin/libresign" Then the response should contain the initial state "libresign-effective_policies" json that match with: @@ -31,7 +31,7 @@ Feature: admin/initial_state Scenario Outline: Invalid identify methods updates preserve the default contract Given as user "admin" And sending "post" to ocs "/apps/libresign/api/v1/policies/system/identify_methods" - | value | (string){"factors":[]} | + | value | null | And the response should have a status code 200 When sending "post" to ocs "/apps/libresign/api/v1/policies/system/identify_methods" | value | (string) | @@ -51,7 +51,7 @@ Feature: admin/initial_state Scenario: Updated identify methods are exposed in admin initial state Given as user "admin" And sending "post" to ocs "/apps/libresign/api/v1/policies/system/identify_methods" - | value | (string){"factors":[]} | + | value | null | And the response should have a status code 200 When sending "post" to ocs "/apps/libresign/api/v1/policies/system/identify_methods" | value | (string){"factors":[{"name":"account","enabled":true,"requirement":"required","signatureMethods":{"clickToSign":{"enabled":true}}},{"name":"email","enabled":false,"requirement":"optional"}]} | @@ -64,13 +64,13 @@ Feature: admin/initial_state | (jq)(.policies.identify_methods.effectiveValue.factors \| map(select(.name == "email")) \| .[0].enabled) | false | | (jq)(.policies.identify_methods.effectiveValue.factors \| map(select(.name == "email")) \| .[0].requirement) | optional | And sending "post" to ocs "/apps/libresign/api/v1/policies/system/identify_methods" - | value | (string){"factors":[]} | + | value | null | And the response should have a status code 200 Scenario: Deleting custom identify methods clears the custom rule badge data Given as user "admin" And sending "post" to ocs "/apps/libresign/api/v1/policies/system/identify_methods" - | value | (string){"factors":[]} | + | value | null | And the response should have a status code 200 And sending "post" to ocs "/apps/libresign/api/v1/policies/system/identify_methods" | value | (string){"factors":[{"name":"account","enabled":true,"requirement":"required","signatureMethods":{"clickToSign":{"enabled":true}}},{"name":"email","enabled":false,"requirement":"optional"}]} | @@ -82,7 +82,7 @@ Feature: admin/initial_state | (jq).policies.identify_methods.sourceScope | global | | (jq).policies.identify_methods.everyoneCount | 1 | And sending "post" to ocs "/apps/libresign/api/v1/policies/system/identify_methods" - | value | (string){"factors":[]} | + | value | null | And the response should have a status code 200 When sending "get" to "/settings/admin/libresign" Then the response should contain the initial state "libresign-effective_policies" json that match with: @@ -106,37 +106,37 @@ Feature: admin/initial_state And run the command "config:app:delete libresign config_path" with result code 0 And run the command "config:app:delete libresign tsa_settings.password" with result code 0 And sending "post" to ocs "/apps/libresign/api/v1/policies/system/tsa_settings" - | value | (string){"url":"","policy_oid":"","auth_type":"none","username":""} | + | value | null | And the response should have a status code 200 And sending "post" to ocs "/apps/libresign/api/v1/policies/system/collect_metadata" - | value | false | + | value | null | And the response should have a status code 200 And sending "post" to ocs "/apps/libresign/api/v1/policies/system/legal_information" - | value | | + | value | null | And the response should have a status code 200 And sending "post" to ocs "/apps/libresign/api/v1/policies/system/docmdp" | value | 2 | And the response should have a status code 200 And sending "post" to ocs "/apps/libresign/api/v1/policies/system/signature_flow" - | value | none | + | value | null | And the response should have a status code 200 And sending "post" to ocs "/apps/libresign/api/v1/policies/system/identification_documents" - | value | {"enabled":false,"approvers":["admin"]} | + | value | null | And the response should have a status code 200 And sending "post" to ocs "/apps/libresign/api/v1/policies/system/approval_group" - | value | (string)["admin"] | + | value | null | And the response should have a status code 200 And sending "post" to ocs "/apps/libresign/api/v1/policies/system/envelope_enabled" - | value | true | + | value | null | And the response should have a status code 200 And sending "post" to ocs "/apps/libresign/api/v1/policies/system/show_confetti_after_signing" - | value | true | + | value | null | And the response should have a status code 200 And sending "post" to ocs "/apps/libresign/api/v1/policies/system/crl_external_validation_enabled" - | value | true | + | value | null | And the response should have a status code 200 And sending "post" to ocs "/apps/libresign/api/v1/policies/system/signature_stamp" - | value | (string){"template":"Signed with LibreSign\n{{SignerCommonName}}\nIssuer: {{IssuerCommonName}}\nDate: {{ServerSignatureDate}}","template_font_size":9.8,"signature_font_size":20,"signature_width":350,"signature_height":100,"background_type":"default","render_mode":"default"} | + | value | null | And the response should have a status code 200 And the following libresign app config is set | certificate_engine | openssl | From 481967c0b58d87d40392c4fe57df7be2ae2ebda0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Maia?= Date: Sat, 17 Oct 2026 18:51:08 -0300 Subject: [PATCH 4/4] fix(policy): keep an enforced empty default as an explicit system rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An empty stored value was always read as "no explicit rule", so saving legal_information (default '') as enforced left a group or user layer in control and orphaned its override flag. The policy API writes the flag with every value, so only an empty value without it is treated as absent. Signed-off-by: André Maia --- lib/Service/Policy/Runtime/PolicySource.php | 13 +-- .../Policy/Runtime/PolicySourceTest.php | 84 +++++++++++++++---- 2 files changed, 76 insertions(+), 21 deletions(-) diff --git a/lib/Service/Policy/Runtime/PolicySource.php b/lib/Service/Policy/Runtime/PolicySource.php index 6297161bd1..ab0249420a 100644 --- a/lib/Service/Policy/Runtime/PolicySource.php +++ b/lib/Service/Policy/Runtime/PolicySource.php @@ -44,10 +44,12 @@ public function loadSystemPolicy(string $policyKey): ?PolicyLayer { ? $this->readSystemValue($definition->getAppConfigKey(), $defaultValue) : null; - // An empty string means the key was written directly (bypassing the - // policy API) or cleared via a migration path. Treat it the same as - // having no explicit system value so the default applies cleanly. - if ($storedValue === '') { + // The policy API stores the override flag with every value, so an empty + // string without it was written directly or cleared via a migration + // path. Treat it as having no explicit system value. + $allowOverrideConfigKey = $this->getSystemAllowOverrideConfigKey($definition->getAppConfigKey()); + $hasAllowOverrideFlag = $this->appConfig->hasAppKey($allowOverrideConfigKey); + if ($storedValue === '' && !$hasAllowOverrideFlag) { $hasExplicitSystemValue = false; $storedValue = null; } @@ -67,8 +69,7 @@ public function loadSystemPolicy(string $policyKey): ?PolicyLayer { // A default value stored without an override flag predates explicit // defaults: the API could not enforce it then, so it stays overridable. - $allowOverrideConfigKey = $this->getSystemAllowOverrideConfigKey($definition->getAppConfigKey()); - if ($value === $defaultValue && !$this->appConfig->hasAppKey($allowOverrideConfigKey)) { + if ($value === $defaultValue && !$hasAllowOverrideFlag) { return $layer->setAllowChildOverride(true); } diff --git a/tests/php/Unit/Service/Policy/Runtime/PolicySourceTest.php b/tests/php/Unit/Service/Policy/Runtime/PolicySourceTest.php index c37f5714e3..c8287400a2 100644 --- a/tests/php/Unit/Service/Policy/Runtime/PolicySourceTest.php +++ b/tests/php/Unit/Service/Policy/Runtime/PolicySourceTest.php @@ -764,23 +764,11 @@ public function testTheSystemRuleDecidesWhetherALowerLayerChangesTheEffectiveVal if ($storedAllowChildOverride !== null) { $this->setStoredAppConfigString('policy.signature_flow.system.allow_child_override', $storedAllowChildOverride); } - $conflictingLayer = (new PolicyLayer()) - ->setScope($conflictingScope === 'group' ? 'group' : 'user') - ->setValue('ordered_numeric') - ->setAllowChildOverride(true) - ->setVisibleToChild(true); - $source = $this->createMock(IPolicySource::class); - $source->method('loadSystemPolicy')->willReturn($this->getSource()->loadSystemPolicy(SignatureFlowPolicy::KEY)); - $source->method('loadGroupPolicies')->willReturn($conflictingScope === 'group' ? [$conflictingLayer] : []); - $source->method('loadUserPolicy')->willReturn($conflictingScope === 'user_policy' ? $conflictingLayer : null); - - $resolved = (new DefaultPolicyResolver($source))->resolve( - $this->registry->get(SignatureFlowPolicy::KEY), - PolicyContext::fromUserId('john'), + $this->assertSame( + $expectedEffectiveValue, + $this->resolveAgainstConflictingLayer(SignatureFlowPolicy::KEY, $conflictingScope, 'ordered_numeric'), ); - - $this->assertSame($expectedEffectiveValue, $resolved->getEffectiveValue()); } /** @return iterable */ @@ -795,6 +783,54 @@ public static function providerExplicitSystemRules(): iterable { } } + /** + * legal_information defaults to an empty string, which used to be read + * as "no explicit rule" even when the policy API stored it enforced. + */ + #[DataProvider('providerExplicitEmptyDefaultRules')] + public function testAnEmptyDefaultIsEnforcedOnlyWhenStoredWithItsOverrideFlag( + ?string $storedValue, + ?string $storedAllowChildOverride, + string $conflictingScope, + string $expectedEffectiveValue, + ): void { + if ($storedValue !== null) { + $this->setStoredAppConfigString(LegalInformationPolicy::SYSTEM_APP_CONFIG_KEY, $storedValue); + } + if ($storedAllowChildOverride !== null) { + $this->setStoredAppConfigString(LegalInformationPolicy::SYSTEM_APP_CONFIG_KEY . '.allow_child_override', $storedAllowChildOverride); + } + + $this->assertSame( + $expectedEffectiveValue, + $this->resolveAgainstConflictingLayer(LegalInformationPolicy::KEY, $conflictingScope, 'Group text'), + ); + } + + /** @return iterable */ + public static function providerExplicitEmptyDefaultRules(): iterable { + foreach (['group', 'user_policy'] as $conflictingScope) { + yield "no explicit rule, $conflictingScope wins" => [null, null, $conflictingScope, 'Group text']; + yield "empty default enforced, $conflictingScope ignored" => ['', '0', $conflictingScope, '']; + yield "empty default overridable, $conflictingScope wins" => ['', '1', $conflictingScope, 'Group text']; + yield "empty value written without override flag, $conflictingScope wins" => ['', null, $conflictingScope, 'Group text']; + yield "explicit value enforced, $conflictingScope ignored" => ['Company text', '0', $conflictingScope, 'Company text']; + } + } + + public function testAnEnforcedEmptyDefaultSavedThroughTheSourceIsReadBackEnforced(): void { + $source = $this->getSource(); + $source->saveSystemPolicy(LegalInformationPolicy::KEY, '', false); + + $layer = $source->loadSystemPolicy(LegalInformationPolicy::KEY); + + $this->assertNotNull($layer); + $this->assertSame('global', $layer->getScope()); + $this->assertSame('', $layer->getValue()); + $this->assertFalse($layer->isAllowChildOverride()); + $this->assertSame([''], $layer->getAllowedValues()); + } + public function testAnEnforcedDefaultSavedThroughTheSourceIsReadBackEnforced(): void { $source = $this->getSource(); $source->saveSystemPolicy(SignatureFlowPolicy::KEY, 'none', false); @@ -2321,6 +2357,24 @@ public function testLoadAllRuleCountsCountsExplicitSystemRuleEvenWhenValueMatche $this->assertSame(1, $result['signature_flow']['everyoneCount']); } + private function resolveAgainstConflictingLayer(string $policyKey, string $conflictingScope, string $conflictingValue): mixed { + $conflictingLayer = (new PolicyLayer()) + ->setScope($conflictingScope === 'group' ? 'group' : 'user') + ->setValue($conflictingValue) + ->setAllowChildOverride(true) + ->setVisibleToChild(true); + + $source = $this->createMock(IPolicySource::class); + $source->method('loadSystemPolicy')->willReturn($this->getSource()->loadSystemPolicy($policyKey)); + $source->method('loadGroupPolicies')->willReturn($conflictingScope === 'group' ? [$conflictingLayer] : []); + $source->method('loadUserPolicy')->willReturn($conflictingScope === 'user_policy' ? $conflictingLayer : null); + + return (new DefaultPolicyResolver($source))->resolve( + $this->registry->get($policyKey), + PolicyContext::fromUserId('john'), + )->getEffectiveValue(); + } + private function setStoredAppConfigString(string $key, string $value): void { $this->coreAppConfig->setValueString(Application::APP_ID, $key, $value); }