Skip to content

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

Open
maia-andre wants to merge 3 commits into
LibreSign:mainfrom
maia-andre:fix/system-policy-explicit-default-8916
Open

maia-andre wants to merge 3 commits into
LibreSign:mainfrom
maia-andre:fix/system-policy-explicit-default-8916

Conversation

@maia-andre

Copy link
Copy Markdown
Contributor

Resolves: #8916

📝 Summary

An administrator could not enforce the built-in default of a policy. Saving it with allowChildOverride = false was lost on both sides:

  • Write: PolicySource::saveSystemPolicy() deleted the system rule when the value equaled the default and the override was disabled.
  • Read: PolicySource::loadSystemPolicy() returned an overridable layer for an explicit default, whatever its flag. A group or user rule still changed the effective value.

What changes:

  • saveSystemPolicy() stores every explicit value with its override flag, including a value equal to the default. The special case that deleted the rule, including its JSON comparison, is gone.
  • 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 (as agreed in the issue).
  • PolicyService::saveSystem(): null now means "no explicit system rule" and clears it (clearSystem()), instead of being turned into the default and saved.
    • Compound system writes go through the same path, so null and an explicit default differ there too. The composite validation already treated null as the default, which is what the effective value becomes.
    • Policy Workbench already removes a system rule by posting value: null, so the frontend needs no change. Saving the default with "do not allow overrides" now just works.
  • API documentation: both system write endpoints now describe null as removing the explicit rule. Before, they said it reset the value to its default.

Direct reads and bulk loading share loadSystemPolicy() (resolveMany() goes through resolveCore()), so they stay consistent. Rule counts already come from the stored key, so an enforced default is counted (existing testLoadAllRuleCountsCountsExplicitSystemRuleEvenWhenValueMatchesDefault).

I also checked the internal callers of saveSystem():

  • The background, reminder, footer and DocMDP services pass the current override flag, so their behavior does not change.
  • AEngineHandler (engine none) saves a value different from the default, which was already an enforced rule.
  • SignatureTextService::save() has no production caller.

Composition (2 commits): the fix is 70 lines of production code and 209 lines of tests. The regenerated OpenAPI/TypeScript commit is 16 lines, descriptions only.

🧪 How to test

  1. POST /ocs/v2.php/apps/libresign/api/v1/policies/system/show_confetti_after_signing with value = true, allowChildOverride = false (the default value, enforced).
  2. As admin, PUT …/policies/user/signer1/show_confetti_after_signing with value = false.
  3. As signer1, GET …/policies/effective: show_confetti_after_signing is true from global. On main it is false from user_policy.
  4. POST …/policies/system/show_confetti_after_signing with value = null, then the same GET as signer1: now false from user_policy.

This is the new scenario in features/policies/boolean_policy_layers.feature. It fails on main at step 3.

Validation evidence

  • Focused regression/acceptance test:
    • PolicySourceTest resolves the issue's matrix through DefaultPolicyResolver, against a conflicting group layer and a conflicting user layer, checking the effective value. The matrix covers no rule, an explicit default enforced or overridable, an explicit value enforced or overridable, and a legacy default without flag.
    • A save-then-read test checks the stored rule.
    • PolicyServiceTest: null clears and an explicit default saves, also in a compound write that mixes both.
    • Behat: the scenario above. The existing tests that expected the deletion now expect an enforced rule.
  • Relevant lint/type/static-analysis checks: composer cs:check is clean, and psalm adds no issue.
  • Broader suite required by the changed surface: the whole PHPUnit unit suite (4088 tests) and features/policies/boolean_policy_layers.feature (4 scenarios) pass.
  • CI reviewed

⚙️ API / Back-end changes

  • An explicit system default keeps its override flag. null removes the explicit system rule (single and compound writes).
  • Unit and/or integration tests added or updated
  • Authorization and negative paths checked when applicable: a lower user rule is ignored under an enforced default and applies again once the rule is removed
  • Capabilities updated when applicable: not applicable
  • Documentation updated when applicable: endpoint descriptions
  • API documentation regenerated with composer openapi when necessary

✅ Checklist

  • I have read and followed the contribution guide.
  • The diff does not contain unrelated refactors or generated changes.
  • Commits follow Conventional Commits where applicable.
  • All commits include the required DCO sign-off.

🤖 AI (if applicable)

  • The content of this PR was partially or fully generated using AI

…m rule

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: LibreSign#8916
Assisted-by: Claude Code:claude-opus-5-5
Signed-off-by: André Maia <andrefnkmm@gmail.com>
Part of LibreSign#8916
Assisted-by: Claude Code:claude-opus-5-5

Signed-off-by: André Maia <andrefnkmm@gmail.com>
@maia-andre
maia-andre requested a review from a team as a code owner October 6, 2026 01:27
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 <andrefnkmm@gmail.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow system policies to enforce an explicitly configured default value

1 participant