From 930cc7a6dd70d5076722d65a1a85da864ddf766f Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 22 Mar 2019 09:22:54 -0700
Subject: [PATCH] Generalize Legalpad validation logic for "Require Signature"

Summary:
See downstream <https://phabricator.wikimedia.org/T208254>.

I can't actually reproduce any issue here (we only show this field when creating a document, and only if the viewer is an administrator), so maybe this relied on some changes or was originally reported against older code.

Regardless, the validation isn't quite right: it requires administrator privileges to apply this transaction at all, but should only require administrator privileges to change the value.

Test Plan:
Edited Legalpad documents as an administrator and non-administrator before and after the change, with and without signatures being required.

Couldn't reproduce the original issue, but this version is generally more correct/robust.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20311
---
 ...padDocumentRequireSignatureTransaction.php | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/src/applications/legalpad/xaction/LegalpadDocumentRequireSignatureTransaction.php b/src/applications/legalpad/xaction/LegalpadDocumentRequireSignatureTransaction.php
index 3819f38a70..9932baab80 100644
--- a/src/applications/legalpad/xaction/LegalpadDocumentRequireSignatureTransaction.php
+++ b/src/applications/legalpad/xaction/LegalpadDocumentRequireSignatureTransaction.php
@@ -55,11 +55,22 @@ final class LegalpadDocumentRequireSignatureTransaction
   public function validateTransactions($object, array $xactions) {
     $errors = array();
 
-    $is_admin = $this->getActor()->getIsAdmin();
+    $old = (bool)$object->getRequireSignature();
+    foreach ($xactions as $xaction) {
+      $new = (bool)$xaction->getNewValue();
 
-    if (!$is_admin) {
-      $errors[] = $this->newInvalidError(
-        pht('Only admins may require signature.'));
+      if ($old === $new) {
+        continue;
+      }
+
+      $is_admin = $this->getActor()->getIsAdmin();
+      if (!$is_admin) {
+        $errors[] = $this->newInvalidError(
+          pht(
+            'Only administrators may change whether a document '.
+            'requires a signature.'),
+          $xaction);
+      }
     }
 
     return $errors;