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

Support OpenOffice password decryption #157

Merged
merged 8 commits into from
Feb 6, 2015
Merged

Conversation

myplaceonline
Copy link

Support OpenOffice password decryption when encrypted with
AES-256-CBC (no padding) + PBKDF2/HmacSHA1 key and
SHA-256 password hash.

AES-256-CBC (no padding) + PBKDF2/HmacSHA1 key and
SHA-256 password hash.
@myplaceonline
Copy link
Author

Forgot to add that to use this feature, simply pass in the password in the options hash. For example, as shown in the test in the commit:

Roo::LibreOffice.new(File.join(TESTDIR, "encrypted-letmein.ods"), :password => "letmein")

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) when pulling 3d5adc3 on myplaceonline:master into 63996f0 on roo-rb:master.

@myplaceonline
Copy link
Author

@tdeo Tried to add it, but got the following error:

  1) Error:
TestRoo#test_openoffice_encryption:
NoMethodError: undefined method `expect' for #<TestRoo:0x000000026ba9c0>
    /work/myplaceonline/src/src/roo/test/test_roo.rb:1961:in `test_openoffice_encryption'

Using:

      expect {
        Roo::LibreOffice.new(File.join(TESTDIR, "encrypted-letmein.ods"), :password => "badpassword")
      }.to raise_error ArgumentError, "Invalid password or other data error"

@simonoff
Copy link
Member

I don't like the quality of this code.

@myplaceonline
Copy link
Author

@simonoff Can you please elaborate which parts you don't like so that I can improve them?

the cipher. Also add more comments and reformat.
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.23%) when pulling a22a413 on myplaceonline:master into 63996f0 on roo-rb:master.

@myplaceonline
Copy link
Author

@simonoff The only obviously poor quality thing I could find was that there was superfluous file I/O. Pushed a new commit which optimizes this to stream from the zip file directly into the decryption cipher. Also added more comments and reformatted code to smaller column width and added a few more sanity checks.

@simonoff
Copy link
Member

@myplaceonline it's additional 150 lines of code with 7 inherited if. Can you extract this lines into few methods?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) when pulling 966f3ce on myplaceonline:master into a0dd800 on roo-rb:master.

@myplaceonline
Copy link
Author

@simonoff Extracted into some logical methods

@myplaceonline
Copy link
Author

@simonoff I tried to merge in commit 85ca578 but it is currently failing tests:

$ git clone git@github.com:roo-rb/roo.git
...
$ cd roo
$ rake
...
  1) Failure:
TestBase#test_to_csv_with_separator [/tmp/roo/test/test_generic_spreadsheet.rb:124]:
--- expected
+++ actual
@@ -1,17 +1 @@
-";;;;;;
-;;;;;;
-;;;;;;
-;;;;;;
-1961-11-21;;;;;;
-;;;;;;
-;;;;;;
-;;\"thisisc8\";;;;\"thisisg8\"
-;;;;;;
-;;;;;;
-;;;;;;
-41;42;43;44;45;;
-;;;;;;
-;;;;;;
-;;43;44;45;;
-;;\"dreiundvierzig\";\"vierundvierzig\";\"fuenfundvierzig\";;
-"
+""
...

@myplaceonline
Copy link
Author

@simonoff Test failure is caused by lines 136-137 in 85ca578#diff-e7ac28e488d98ee522c91f631315b536L136

The problem is that in the change to using a default parameter with sheet = default_sheet and removing sheet ||= default_sheet, it will break any callers that explicitly pass nil for sheet, as test_to_csv_with_separator does.

As in the case of test_to_csv_with_separator, this could be done by anyone that wants to pass a positional argument (such as the separator character in this case) and accept the defaults for previous arguments. Therefore, the multiple changes to use parameter defaults instead of ||= in commit 85ca578 effectively break the APIs for such callers.

If it is decided to continue with commit 85ca578, then the test can be fixed with:

    assert_equal expected_csv_with_semicolons,@oo.to_csv(nil, @oo.default_sheet, ';')

I will open an issue for this so it's documented as a separate issue...

@myplaceonline
Copy link
Author

Opened Issue #159

@myplaceonline
Copy link
Author

@simonoff I've merged in all the latest commits, please review when you have a chance (noting that the Travis CI build errors are caused by issue #159 which is separate).

@ebertech
Copy link

ebertech commented Dec 4, 2014

aren't there laws against using camelCasing in Ruby (TM) programs? getCipherKey? decryptIfNecessary?

@myplaceonline
Copy link
Author

@ebertech Updated, thanks

@ebertech
Copy link

ebertech commented Dec 4, 2014

@myplaceonline you're welcome! yeah opensource!

@myplaceonline
Copy link
Author

@simonoff Issue #159 fixed, changes merged, CI build passed, ready to merge

@myplaceonline myplaceonline reopened this Dec 7, 2014
simonoff added a commit that referenced this pull request Feb 6, 2015
Support OpenOffice password decryption
@simonoff simonoff merged commit 3a370f8 into roo-rb:master Feb 6, 2015
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 20d77db on myplaceonline:master into * on roo-rb:master*.

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.

6 participants