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..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; } @@ -65,26 +67,13 @@ 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. + if ($value === $defaultValue && !$hasAllowOverrideFlag) { 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 +661,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/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/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/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?: { 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 | 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..c8287400a2 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,114 @@ 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); + } + + $this->assertSame( + $expectedEffectiveValue, + $this->resolveAgainstConflictingLayer(SignatureFlowPolicy::KEY, $conflictingScope, 'ordered_numeric'), + ); + } + + /** @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']; + } + } + + /** + * 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); + + $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'); @@ -2259,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); }