Skip to content

issue #1060 pgp decrypt method that mimics error behavior#1216

Merged
DenBond7 merged 5 commits intomasterfrom
ip-1060-take2
May 10, 2021
Merged

issue #1060 pgp decrypt method that mimics error behavior#1216
DenBond7 merged 5 commits intomasterfrom
ip-1060-take2

Conversation

@IvanPizhenko
Copy link
Copy Markdown
Contributor

@IvanPizhenko IvanPizhenko commented May 6, 2021 •
edited
Loading

This PR introduces 100% jvm based method for the decryption.

close #1060

Tests (delete all except exactly one):

  • Tests added or updated

To be filled by reviewers

I have reviewed that this PR... (tick whichever items you personally focused on during this review):

  • addresses the issue it closes (if any)
  • code is readable and understandable
  • is accompanied with tests, or tests are not needed
  • is free of vulnerabilities

@IvanPizhenko IvanPizhenko requested review from DenBond7 and tomholub May 6, 2021 22:31
@IvanPizhenko IvanPizhenko requested a review from seisvelas as a code owner May 6, 2021 22:31
@DenBond7 DenBond7 added this to the 1.1.8 milestone May 7, 2021
@DenBond7 DenBond7 added the PR submitted PR is submitted for this issue label May 7, 2021
tomholub
tomholub reviewed May 7, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator

@tomholub tomholub left a comment

Choose a reason for hiding this comment

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

Thank you - this is what I needed, but needs some improvements. I didn't look at the tests yet (will later).

It seems FORMAT error are not handled? For example, what will happen if I enter "abcd".toByteArray() as the data? It seems like it would throw "OTHER / wrong key" which is not correct.

Comment thread FlowCrypt/src/main/java/com/flowcrypt/email/security/pgp/PgpArmor.kt
Comment thread FlowCrypt/src/main/java/com/flowcrypt/email/security/pgp/PgpMsg.kt Outdated
Comment thread FlowCrypt/src/main/java/com/flowcrypt/email/security/pgp/PgpMsg.kt Outdated
Comment thread FlowCrypt/src/main/java/com/flowcrypt/email/security/pgp/PgpMsg.kt
DenBond7
DenBond7 requested changes May 7, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator

@DenBond7 DenBond7 left a comment

Choose a reason for hiding this comment

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

Leaved a few comments. Please look at them.

Comment thread FlowCrypt/src/main/java/com/flowcrypt/email/extensions/kotlin/ByteArrayExt.kt Outdated
Comment thread FlowCrypt/src/main/java/com/flowcrypt/email/security/pgp/PgpMsg.kt
Comment thread FlowCrypt/src/main/java/com/flowcrypt/email/security/pgp/PgpMsg.kt Outdated
Comment thread FlowCrypt/src/main/java/com/flowcrypt/email/security/pgp/PgpMsg.kt Outdated
Comment thread FlowCrypt/src/main/java/com/flowcrypt/email/security/pgp/PgpMsg.kt
Comment thread FlowCrypt/src/test/java/com/flowcrypt/email/security/pgp/PgpMsgTest.kt Outdated
Comment thread FlowCrypt/src/test/java/com/flowcrypt/email/security/pgp/PgpMsgTest.kt Outdated
@IvanPizhenko IvanPizhenko mentioned this pull request May 8, 2021
Open
@IvanPizhenko
Copy link
Copy Markdown
Contributor Author

IvanPizhenko commented May 9, 2021

@tomholub @DenBond7 made some fixes here, please review

@IvanPizhenko IvanPizhenko changed the title issue #1060 gp decrypt method that mimics error behavior issue #1060 pgp decrypt method that mimics error behavior May 9, 2021
@IvanPizhenko IvanPizhenko mentioned this pull request May 9, 2021
Closed
@DenBond7 DenBond7 self-requested a review May 10, 2021 06:19
DenBond7
DenBond7 previously approved these changes May 10, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator

@DenBond7 DenBond7 left a comment

Choose a reason for hiding this comment

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

Thank you. Looks good. Approved

Comment thread FlowCrypt/src/main/java/com/flowcrypt/email/security/pgp/PgpMsg.kt
@DenBond7
Copy link
Copy Markdown
Collaborator

DenBond7 commented May 10, 2021

@tomholub Please review it. I will merge it after your approving

tomholub
tomholub reviewed May 10, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator

@tomholub tomholub left a comment

Choose a reason for hiding this comment

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

Looks good, with tiny changes, from my end it's good to merge it. I'll let Den do the final review and merging. Thank you!

Comment thread FlowCrypt/src/main/java/com/flowcrypt/email/security/pgp/PgpMsg.kt Outdated
Comment thread FlowCrypt/src/main/java/com/flowcrypt/email/security/pgp/PgpMsg.kt Outdated
Comment thread FlowCrypt/src/main/java/com/flowcrypt/email/security/pgp/PgpMsg.kt
Comment thread FlowCrypt/src/test/java/com/flowcrypt/email/security/pgp/PgpMsgTest.kt
Comment thread FlowCrypt/src/test/java/com/flowcrypt/email/security/pgp/PgpMsgTest.kt Outdated
Comment thread FlowCrypt/src/test/java/com/flowcrypt/email/security/pgp/PgpMsgTest.kt
@DenBond7 DenBond7 dismissed their stale review May 10, 2021 06:49

Changed the approving status. I'll approve it after Tom. Due to difficulty of the current issue(decryption process).

@tomholub
Copy link
Copy Markdown
Collaborator

tomholub commented May 10, 2021

Ok Den, after the above changes, I'll do my approving review, then you can do yours.

@IvanPizhenko
Copy link
Copy Markdown
Contributor Author

IvanPizhenko commented May 10, 2021

@tomholub @DenBond7 I've made some more fixes according to Tom's comments, so please check them

tomholub
tomholub approved these changes May 10, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator

@tomholub tomholub left a comment

Choose a reason for hiding this comment

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

Thank you! Looks good to me. Will let Den take another look and merge it.

@DenBond7 DenBond7 merged commit 84a9bc1 into master May 10, 2021
@DenBond7 DenBond7 deleted the ip-1060-take2 branch May 10, 2021 18:18
@IvanPizhenko IvanPizhenko mentioned this pull request May 11, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DenBond7 DenBond7 DenBond7 approved these changes

@tomholub tomholub tomholub approved these changes

@seisvelas seisvelas Awaiting requested review from seisvelas

Assignees

No one assigned

Labels

PR submitted PR is submitted for this issue

Projects

None yet

Milestone

1.1.8: Decrypt files with Kotlin

Development

Successfully merging this pull request may close these issues.

implement pgp decrypt method that mimics error behavior

3 participants

@IvanPizhenko @DenBond7 @tomholub