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

Minor modifications to PKCS5KeyFile #265

Merged
merged 7 commits into from
Aug 19, 2016
Merged

Conversation

joval
Copy link

@joval joval commented Aug 19, 2016

Just wanted to get this out of the way before tackling the logger factory issue.

David Solin added 2 commits August 19, 2016 12:18
… was used, which prevented decryption with a subsequent correct passphrase.

Also, made it possible for subclasses of PKCS5KeyFile to access the decrypted ASN.1 data directly (useful if you want to decrypt then store a PKCS5 key for later use elsewhere).
And I think I made the code a little prettier.
@hierynomus
Copy link
Owner

I would like to see an added testcase ;) Prevents stuff from breaking again when refactoring

@hierynomus
Copy link
Owner

Also codacy shows that you've introduced 2 new code issues with this PR

@joval
Copy link
Author

joval commented Aug 19, 2016

I'm not sure what to make of the two codacy issues:

  1. I put the new field up with all the other fields, following your convention of putting the factory inner class definition up top. Although, I'll move the new inner exception classes to the top since that also appears to be the convention in the code-base.

  2. I moved the decryption to its own method, and actually (I thought) simplified it! Now it complains?

I will add a test case.

@hierynomus
Copy link
Owner

I've just added codacy, so there might still be some things in the ruleset that can be tweaked ;) I agree with keeping the static factory classes on top.

As for the NPath complexity, it complains that it is still too high. Reduction can normally be achieved by extracting into different methods for different paths :)

}
} else if (line.startsWith("DEK-Info: ")) {
int ptr = line.indexOf(",");
if (ptr == -1) {
throw new IOException("Unrecognized DEK-Info: " + line.substring(10));
throw new FormatException("Unrecognized DEK-Info: " + line.substring(10));
} else {
String algorithm = line.substring(10,ptr);
Copy link
Owner

Choose a reason for hiding this comment

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

That one seems like a false positive, i.e. this is a local variable not a field.

@joval
Copy link
Author

joval commented Aug 19, 2016

I transplanted the test... I need to put the test into my build workflow so this doesn't happen again.

Do you really want me to make the new test class less readable per Codacy?

@hierynomus
Copy link
Owner

Actually the question would be, why are they static final fields if they're
needed by one test? I do agree with the codestyle here, move them to the
top, or make them local variables.

2016-08-19 22:11 GMT+02:00 JovalCM notifications@github.com:

I transplanted the test... I need to put the test into my build workflow
so this doesn't happen again.

Do you really want me to make the new test class less readable per Codacy?


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#265 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAHLo8hKE2Pidw7Vjx63QpuwtGGZglKYks5qhg3rgaJpZM4Jorpz
.

@joval
Copy link
Author

joval commented Aug 19, 2016

I just copied from the OpenSSHKeyFileTest. The id_dsa file (used by that test) is an encrypted PKCS5 key file, and its test checks the exact use-case that was broken. Constants are static final fields in all the keyfile tests.

@hierynomus hierynomus merged commit e5ec84c into hierynomus:master Aug 19, 2016
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.

2 participants