Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: implement SHA-512 algorithm to ProtectSheet #1115

Merged
merged 7 commits into from
Jan 8, 2022
Merged

Conversation

Jonham
Copy link
Contributor

@Jonham Jonham commented Jan 5, 2022

implement SHA-512 algorithm to protectSheet and add ValidateSheetProtect

Description

add genISOPasswordHash, which implements SHA-512 algorithm salt hash generation that supports workbookProtection and worksheetProtection.

Motivation and Context

Excel 2007 supports hash password. Since Excel 2013, SHA-512 algorithm are the default method in creating workbook protection and worksheet protection. Although worksheet protection.password works well with genSheetPasswd (Excel 2013, 2019 both support), workbook protection only support SHA-512. And next I will implement protectWorkbook.

How Has This Been Tested

I had set password to workbook protection and worksheet protection on file test/Worksheet-Workbook-Protection.xlsx with Mac Excel 2019.
unzip it and read both protection xml lines to create func TestGenISOPasswordHash test1 and test2.
test3 just run the algorithm twice to test whether hash value match.

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

    no document about ProtectSheet

  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@codecov-commenter
Copy link

codecov-commenter commented Jan 5, 2022

Codecov Report

Merging #1115 (b2f0e49) into master (e37e060) will decrease coverage by 0.11%.
The diff coverage is 73.91%.

❗ Current head b2f0e49 differs from pull request most recent head 51cd742. Consider uploading reports for the commit 51cd742 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1115      +/-   ##
==========================================
- Coverage   98.12%   98.00%   -0.12%     
==========================================
  Files          29       29              
  Lines       18554    18643      +89     
==========================================
+ Hits        18206    18271      +65     
- Misses        236      258      +22     
- Partials      112      114       +2     
Flag Coverage Δ
unittests 98.00% <73.91%> (-0.12%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
errors.go 100.00% <ø> (ø)
sheet.go 97.97% <50.00%> (-2.03%) ⬇️
crypt.go 79.41% <94.00%> (+1.83%) ⬆️
cell.go 97.13% <0.00%> (ø)
styles.go 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e37e060...51cd742. Read the comment docs.

@xuri xuri added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 5, 2022
Copy link
Member

@xuri xuri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your PR, I've left some comments.

crypt.go Outdated Show resolved Hide resolved
crypt.go Outdated Show resolved Hide resolved
sheet.go Outdated Show resolved Hide resolved
sheet.go Outdated Show resolved Hide resolved
sheet.go Outdated Show resolved Hide resolved
sheet.go Outdated Show resolved Hide resolved
crypt.go Outdated Show resolved Hide resolved
crypt_test.go Outdated Show resolved Hide resolved
sheet.go Outdated Show resolved Hide resolved
sheet.go Outdated Show resolved Hide resolved
Copy link
Member

@xuri xuri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the test spreadsheet attachment test/Worksheet-Workbook-Protection.xlsx, just test and assert with XML snippets is enough.

sheet_test.go Outdated Show resolved Hide resolved
@Jonham Jonham requested a review from xuri January 6, 2022 00:37
@Jonham
Copy link
Contributor Author

Jonham commented Jan 6, 2022

I've add some test case and rewrite follow review comments.

@Jonham Jonham changed the title feat: implement SHA-512 algorithm to ProtectSheet, add ValidateSheetProtect feat: implement SHA-512 algorithm to ProtectSheet Jan 6, 2022
@Jonham
Copy link
Contributor Author

Jonham commented Jan 6, 2022

sorry for my carelessness. I will run go test and go test -coverprofile=cover.out before commit next time.

- Make ProtectSheet support specify multi kind of hash algorithm
- Make UnprotectSheet support remove sheet protection with password verification.
- Following exported error constants or enumeration has been changed for typo fix:
	* `ErrDataValidationFormulaLenth` to `ErrDataValidationFormulaLength`
	* `ErrUnsupportEncryptMechanism` to `ErrUnsupportedEncryptMechanism`
	* `DataValidationTypeTextLeng` to `DataValidationTypeTextLength`
@xuri
Copy link
Member

xuri commented Jan 7, 2022

I made some changes based on your branch.

@xuri xuri merged commit af5c4d0 into qax-os:master Jan 8, 2022
@xuri xuri requested review from xuri and removed request for xuri January 16, 2022 06:08
jenbonzhang pushed a commit to jenbonzhang/excelize that referenced this pull request Oct 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
No open projects
Status: Feature
Development

Successfully merging this pull request may close these issues.

3 participants