fix(policy): keep an explicitly saved default as an enforceable system rule

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<string, mixed> $value Policy value to persist. Null resets the policy to its default system value.
+	 * @param null|bool|int|float|string|array<string, mixed> $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<Http::STATUS_OK, LibresignSystemPolicyWriteResponse, array{}>|DataResponse<Http::STATUS_BAD_REQUEST, LibresignErrorResponse, array{}>
 	 *
@@ -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<string, null|bool|int|float|string|array<string, mixed>> $values Values to persist, keyed by policy identifier. Null resets that policy to its default system value.
+	 * @param array<string, null|bool|int|float|string|array<string, mixed>> $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<string, bool> $allowChildOverride Whether lower layers may override each saved value, keyed by policy identifier.
 	 * @return DataResponse<Http::STATUS_OK, LibresignEffectiveCompoundPolicyWriteResponse, array{}>|DataResponse<Http::STATUS_BAD_REQUEST, LibresignErrorResponse, array{}>
 	 *

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);

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]

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": [

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": [

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()

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<string, never>;
                     }) | 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?: {

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<string, never>;
                     }) | 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?: {

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)<payload> |
@@ -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                  |