From 36ca4c303e19c336fa25a748e9a43252f657f900 Mon Sep 17 00:00:00 2001 From: Owen Leibman Date: Sat, 29 May 2021 00:39:05 -0700 Subject: [PATCH] Document Security - Coverage, Testing, and Bug-fixing 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%. 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. 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. 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. --- docs/topics/recipes.md | 3 + src/PhpSpreadsheet/Document/Security.php | 129 ++++-------------- src/PhpSpreadsheet/Reader/Xlsx.php | 34 +++-- .../Document/SecurityTest.php | 75 ++++++++++ 4 files changed, 131 insertions(+), 110 deletions(-) create mode 100644 tests/PhpSpreadsheetTests/Document/SecurityTest.php diff --git a/docs/topics/recipes.md b/docs/topics/recipes.md index bbdc29a818..6e14c08cc9 100644 --- a/docs/topics/recipes.md +++ b/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: diff --git a/src/PhpSpreadsheet/Document/Security.php b/src/PhpSpreadsheet/Document/Security.php index cef3db8c44..bdcc239373 100644 --- a/src/PhpSpreadsheet/Document/Security.php +++ b/src/PhpSpreadsheet/Document/Security.php @@ -50,156 +50,87 @@ public function __construct() /** * Is some sort of document security enabled? - * - * @return bool */ - public function isSecurityEnabled() + public function isSecurityEnabled(): bool { return $this->lockRevision || $this->lockStructure || $this->lockWindows; } - /** - * Get LockRevision. - * - * @return bool - */ - public function getLockRevision() + public function getLockRevision(): bool { return $this->lockRevision; } - /** - * Set LockRevision. - * - * @param bool $pValue - * - * @return $this - */ - public function setLockRevision($pValue) + public function setLockRevision(?bool $pValue): self { - $this->lockRevision = $pValue; + if ($pValue !== null) { + $this->lockRevision = $pValue; + } return $this; } - /** - * Get LockStructure. - * - * @return bool - */ - public function getLockStructure() + public function getLockStructure(): bool { return $this->lockStructure; } - /** - * Set LockStructure. - * - * @param bool $pValue - * - * @return $this - */ - public function setLockStructure($pValue) + public function setLockStructure(?bool $pValue): self { - $this->lockStructure = $pValue; + if ($pValue !== null) { + $this->lockStructure = $pValue; + } return $this; } - /** - * Get LockWindows. - * - * @return bool - */ - public function getLockWindows() + public function getLockWindows(): bool { return $this->lockWindows; } - /** - * Set LockWindows. - * - * @param bool $pValue - * - * @return $this - */ - public function setLockWindows($pValue) + public function setLockWindows(?bool $pValue): self { - $this->lockWindows = $pValue; + if ($pValue !== null) { + $this->lockWindows = $pValue; + } return $this; } - /** - * Get RevisionsPassword (hashed). - * - * @return string - */ - public function getRevisionsPassword() + public function getRevisionsPassword(): string { return $this->revisionsPassword; } - /** - * 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) + public function setRevisionsPassword(?string $pValue, bool $pAlreadyHashed = false): self { - if (!$pAlreadyHashed) { - $pValue = PasswordHasher::hashPassword($pValue); + if ($pValue !== null) { + if (!$pAlreadyHashed) { + $pValue = PasswordHasher::hashPassword($pValue); + } + $this->revisionsPassword = $pValue; } - $this->revisionsPassword = $pValue; return $this; } - /** - * Get WorkbookPassword (hashed). - * - * @return string - */ - public function getWorkbookPassword() + public function getWorkbookPassword(): string { return $this->workbookPassword; } - /** - * 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) + public function setWorkbookPassword(?string $pValue, bool $pAlreadyHashed = false): self { - if (!$pAlreadyHashed) { - $pValue = PasswordHasher::hashPassword($pValue); + if ($pValue !== null) { + if (!$pAlreadyHashed) { + $pValue = PasswordHasher::hashPassword($pValue); + } + $this->workbookPassword = $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; - } - } - } } diff --git a/src/PhpSpreadsheet/Reader/Xlsx.php b/src/PhpSpreadsheet/Reader/Xlsx.php index a7c71c7373..30f73451a0 100644 --- a/src/PhpSpreadsheet/Reader/Xlsx.php +++ b/src/PhpSpreadsheet/Reader/Xlsx.php @@ -1936,25 +1936,37 @@ private function readProtection(Spreadsheet $excel, SimpleXMLElement $xmlWorkboo return; } - if ($xmlWorkbook->workbookProtection['lockRevision']) { - $excel->getSecurity()->setLockRevision((bool) $xmlWorkbook->workbookProtection['lockRevision']); - } - - if ($xmlWorkbook->workbookProtection['lockStructure']) { - $excel->getSecurity()->setLockStructure((bool) $xmlWorkbook->workbookProtection['lockStructure']); - } - - if ($xmlWorkbook->workbookProtection['lockWindows']) { - $excel->getSecurity()->setLockWindows((bool) $xmlWorkbook->workbookProtection['lockWindows']); - } + $excel->getSecurity()->setLockRevision(self::getLockValue($xmlWorkbook->workbookProtection, 'lockRevision')); + $excel->getSecurity()->setLockStructure(self::getLockValue($xmlWorkbook->workbookProtection, 'lockStructure')); + $excel->getSecurity()->setLockWindows(self::getLockValue($xmlWorkbook->workbookProtection, 'lockWindows')); if ($xmlWorkbook->workbookProtection['revisionsPassword']) { $excel->getSecurity()->setRevisionsPassword((string) $xmlWorkbook->workbookProtection['revisionsPassword'], true); + $excel->getSecurity()->setRevisionsPassword( + (string) $xmlWorkbook->workbookProtection['revisionsPassword'], + true + ); } if ($xmlWorkbook->workbookProtection['workbookPassword']) { $excel->getSecurity()->setWorkbookPassword((string) $xmlWorkbook->workbookProtection['workbookPassword'], true); + $excel->getSecurity()->setWorkbookPassword( + (string) $xmlWorkbook->workbookProtection['workbookPassword'], + true + ); + } + } + + 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 diff --git a/tests/PhpSpreadsheetTests/Document/SecurityTest.php b/tests/PhpSpreadsheetTests/Document/SecurityTest.php new file mode 100644 index 0000000000..c2c6657755 --- /dev/null +++ b/tests/PhpSpreadsheetTests/Document/SecurityTest.php @@ -0,0 +1,75 @@ +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()); + } +}