Document Security - Coverage, Testing, and Bug-fixing (#2128)

Having a parallel project to complete cover Document Properties, I turned my attention to to Document Security. As happens, this particular change grew a bit over time.

Coverage and Testing Changes:
- Since the Security object has no members which are themselves objects, there is no need for a deep clone. The untested __clone method is removed.
- Almost all of the coverage for the Security Object came about through samples 11 and 41, not through formal tests with assertions. Formal tests have been added.
- All methods now use type-hinting via the function signature rather than doc block.
- Coverage is now 100%.

<!-- end of coverage and testing changes list -->

Bug:
- Xlsx Reader was not evaluating the Lock values correctly. This revelation came as a result of the new tests ...
- Which showed that Xlsx Reader was testing SimpleXmlElement as a boolean rather than the stringified version of that ...
- Which didn't matter all that much because Xlsx Writer was writing the values as 'true' or 'false' rather than '1' or '0', and (bool) 'false' is true.
- Xlsx Reader clearly needed a change. I was trying to avoid that while awaiting the namespacing change. At least this is restricted to a very small self-contained piece of the code.
- It is less clear whether Xlsx Writer should be changed. It is true that Excel itself uses 1/0 when writing; however it is equally true that it recognizes true/false as well as 1/0 when reading. For now, I have left Xlsx Writer alone to limit the change to what is absolutely needed.

<!-- end of bug list -->

Other Changes:
- I was at a complete loss as to what "lock revisions" was supposed to do, and it took a while to find anything on the web that explained it. Thank you, openpyxl, for coming through. I have documented it for PhpSpreadsheet now.

<!-- end of other changes list -->

Miscellaneous Note:
- There remains no support for Document Security in Xls Reader or Writer (nor in any of the other readers/writers except Xlsx).
- No Phpstan baseline changes, possibly for the first time in any of my PRs since Phpstan was introduced.

Co-authored-by: Mark Baker <mark@lange.demon.co.uk>

docs/topics/recipes.md

@@ -988,6 +988,9 @@ $security->setLockStructure(true);
 $security->setWorkbookPassword("PhpSpreadsheet");
 ```
 
+Note that there are additional methods setLockRevision and setRevisionsPassword
+which apply only to change tracking and history for shared workbooks.
+
 ### Worksheet
 
 An example on setting worksheet security:

src/PhpSpreadsheet/Document/Security.php

@@ -50,156 +50,87 @@ class Security
 
     /**
      * Is some sort of document security enabled?
-     *
-     * @return bool
      */
-    public function isSecurityEnabled()
+    public function isSecurityEnabled(): bool
     {
         return  $this->lockRevision ||
                 $this->lockStructure ||
                 $this->lockWindows;
     }
 
-    /**
+    public function getLockRevision(): bool
-     * Get LockRevision.
-     *
-     * @return bool
-     */
-    public function getLockRevision()
     {
         return $this->lockRevision;
     }
 
-    /**
+    public function setLockRevision(?bool $pValue): self
-     * Set LockRevision.
-     *
-     * @param bool $pValue
-     *
-     * @return $this
-     */
-    public function setLockRevision($pValue)
     {
+        if ($pValue !== null) {
             $this->lockRevision = $pValue;
+        }
 
         return $this;
     }
 
-    /**
+    public function getLockStructure(): bool
-     * Get LockStructure.
-     *
-     * @return bool
-     */
-    public function getLockStructure()
     {
         return $this->lockStructure;
     }
 
-    /**
+    public function setLockStructure(?bool $pValue): self
-     * Set LockStructure.
-     *
-     * @param bool $pValue
-     *
-     * @return $this
-     */
-    public function setLockStructure($pValue)
     {
+        if ($pValue !== null) {
             $this->lockStructure = $pValue;
+        }
 
         return $this;
     }
 
-    /**
+    public function getLockWindows(): bool
-     * Get LockWindows.
-     *
-     * @return bool
-     */
-    public function getLockWindows()
     {
         return $this->lockWindows;
     }
 
-    /**
+    public function setLockWindows(?bool $pValue): self
-     * Set LockWindows.
-     *
-     * @param bool $pValue
-     *
-     * @return $this
-     */
-    public function setLockWindows($pValue)
     {
+        if ($pValue !== null) {
             $this->lockWindows = $pValue;
+        }
 
         return $this;
     }
 
-    /**
+    public function getRevisionsPassword(): string
-     * Get RevisionsPassword (hashed).
-     *
-     * @return string
-     */
-    public function getRevisionsPassword()
     {
         return $this->revisionsPassword;
     }
 
-    /**
+    public function setRevisionsPassword(?string $pValue, bool $pAlreadyHashed = false): self
-     * Set RevisionsPassword.
-     *
-     * @param string $pValue
-     * @param bool $pAlreadyHashed If the password has already been hashed, set this to true
-     *
-     * @return $this
-     */
-    public function setRevisionsPassword($pValue, $pAlreadyHashed = false)
     {
+        if ($pValue !== null) {
             if (!$pAlreadyHashed) {
                 $pValue = PasswordHasher::hashPassword($pValue);
             }
             $this->revisionsPassword = $pValue;
+        }
 
         return $this;
     }
 
-    /**
+    public function getWorkbookPassword(): string
-     * Get WorkbookPassword (hashed).
-     *
-     * @return string
-     */
-    public function getWorkbookPassword()
     {
         return $this->workbookPassword;
     }
 
-    /**
+    public function setWorkbookPassword(?string $pValue, bool $pAlreadyHashed = false): self
-     * Set WorkbookPassword.
-     *
-     * @param string $pValue
-     * @param bool $pAlreadyHashed If the password has already been hashed, set this to true
-     *
-     * @return $this
-     */
-    public function setWorkbookPassword($pValue, $pAlreadyHashed = false)
     {
+        if ($pValue !== null) {
             if (!$pAlreadyHashed) {
                 $pValue = PasswordHasher::hashPassword($pValue);
             }
             $this->workbookPassword = $pValue;
+        }
 
         return $this;
     }
-
-    /**
-     * Implement PHP __clone to create a deep clone, not just a shallow copy.
-     */
-    public function __clone()
-    {
-        $vars = get_object_vars($this);
-        foreach ($vars as $key => $value) {
-            if (is_object($value)) {
-                $this->$key = clone $value;
-            } else {
-                $this->$key = $value;
-            }
-        }
-    }
 }

src/PhpSpreadsheet/Reader/Xlsx.php

@@ -1808,17 +1808,9 @@ class Xlsx extends BaseReader
             return;
         }
 
-        if ($xmlWorkbook->workbookProtection['lockRevision']) {
+        $excel->getSecurity()->setLockRevision(self::getLockValue($xmlWorkbook->workbookProtection, 'lockRevision'));
-            $excel->getSecurity()->setLockRevision((bool) $xmlWorkbook->workbookProtection['lockRevision']);
+        $excel->getSecurity()->setLockStructure(self::getLockValue($xmlWorkbook->workbookProtection, 'lockStructure'));
-        }
+        $excel->getSecurity()->setLockWindows(self::getLockValue($xmlWorkbook->workbookProtection, 'lockWindows'));
-
-        if ($xmlWorkbook->workbookProtection['lockStructure']) {
-            $excel->getSecurity()->setLockStructure((bool) $xmlWorkbook->workbookProtection['lockStructure']);
-        }
-
-        if ($xmlWorkbook->workbookProtection['lockWindows']) {
-            $excel->getSecurity()->setLockWindows((bool) $xmlWorkbook->workbookProtection['lockWindows']);
-        }
 
         if ($xmlWorkbook->workbookProtection['revisionsPassword']) {
             $excel->getSecurity()->setRevisionsPassword(
@@ -1835,6 +1827,18 @@ class Xlsx extends BaseReader
         }
     }
 
+    private static function getLockValue(SimpleXmlElement $protection, string $key): ?bool
+    {
+        $returnValue = null;
+        $protectKey = $protection[$key];
+        if (!empty($protectKey)) {
+            $protectKey = (string) $protectKey;
+            $returnValue = $protectKey !== 'false' && (bool) $protectKey;
+        }
+
+        return $returnValue;
+    }
+
     private function readFormControlProperties(Spreadsheet $excel, ZipArchive $zip, $dir, $fileWorksheet, $docSheet, array &$unparsedLoadedData): void
     {
         if (!$zip->locateName(dirname("$dir/$fileWorksheet") . '/_rels/' . basename($fileWorksheet) . '.rels')) {

tests/PhpSpreadsheetTests/Document/SecurityTest.php

@@ -0,0 +1,75 @@
+<?php
+
+namespace PhpOffice\PhpSpreadsheetTests\Document;
+
+use PhpOffice\PhpSpreadsheet\Spreadsheet;
+use PhpOffice\PhpSpreadsheetTests\Functional\AbstractFunctional;
+
+class SecurityTest extends AbstractFunctional
+{
+    public function testSecurity(): void
+    {
+        $spreadsheet = new Spreadsheet();
+        $spreadsheet->getActiveSheet()->getCell('A1')->setValue('Hello');
+        $security = $spreadsheet->getSecurity();
+        $security->setLockRevision(true);
+        $revisionsPassword = 'revpasswd';
+        $security->setRevisionsPassword($revisionsPassword);
+        $hashedRevisionsPassword = $security->getRevisionsPassword();
+        self::assertNotEquals($revisionsPassword, $hashedRevisionsPassword);
+        $security->setLockWindows(true);
+        $security->setLockStructure(true);
+        $workbookPassword = 'wbpasswd';
+        $security->setWorkbookPassword($workbookPassword);
+        $hashedWorkbookPassword = $security->getWorkbookPassword();
+        self::assertNotEquals($workbookPassword, $hashedWorkbookPassword);
+
+        $reloadedSpreadsheet = $this->writeAndReload($spreadsheet, 'Xlsx');
+        $reloadedSecurity = $reloadedSpreadsheet->getSecurity();
+        self::assertTrue($reloadedSecurity->getLockRevision());
+        self::assertTrue($reloadedSecurity->getLockWindows());
+        self::assertTrue($reloadedSecurity->getLockStructure());
+        self::assertSame($hashedWorkbookPassword, $reloadedSecurity->getWorkbookPassword());
+        self::assertSame($hashedRevisionsPassword, $reloadedSecurity->getRevisionsPassword());
+
+        $reloadedSecurity->setRevisionsPassword($hashedWorkbookPassword, true);
+        self::assertSame($hashedWorkbookPassword, $reloadedSecurity->getRevisionsPassword());
+        $reloadedSecurity->setWorkbookPassword($hashedRevisionsPassword, true);
+        self::assertSame($hashedRevisionsPassword, $reloadedSecurity->getWorkbookPassword());
+    }
+
+    public function providerLocks(): array
+    {
+        return [
+            [false, false, false],
+            [false, false, true],
+            [false, true, false],
+            [false, true, true],
+            [true, false, false],
+            [true, false, true],
+            [true, true, false],
+            [true, true, true],
+        ];
+    }
+
+    /**
+     * @dataProvider providerLocks
+     */
+    public function testLocks(bool $revision, bool $windows, bool $structure): void
+    {
+        $spreadsheet = new Spreadsheet();
+        $spreadsheet->getActiveSheet()->getCell('A1')->setValue('Hello');
+        $security = $spreadsheet->getSecurity();
+        $security->setLockRevision($revision);
+        $security->setLockWindows($windows);
+        $security->setLockStructure($structure);
+        $enabled = $security->isSecurityEnabled();
+        self::assertSame($enabled, $revision || $windows || $structure);
+
+        $reloadedSpreadsheet = $this->writeAndReload($spreadsheet, 'Xlsx');
+        $reloadedSecurity = $reloadedSpreadsheet->getSecurity();
+        self::assertSame($revision, $reloadedSecurity->getLockRevision());
+        self::assertSame($windows, $reloadedSecurity->getLockWindows());
+        self::assertSame($structure, $reloadedSecurity->getLockStructure());
+    }
+}