Issue 1622 send encrypted msg through fes by DenBond7 · Pull Request #1629 · FlowCrypt/flowcrypt-android

DenBond7 · 2021-12-30T10:39:07Z

This PR added send encrypted msg through FES functionality.

close#1622

Tests(delete all except exactly one):

Difficult to test (explain why). It relates to background tasks. Tested manually.

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

…#1622

…ent as an attachment. Refactored code.| #1622

…r.| #1622

# Conflicts: # FlowCrypt/src/main/java/com/flowcrypt/email/security/pgp/PgpEncryptAndOrSign.kt

IvanPizhenko

I suggest to apply some changes, which simplify code

tomholub

Have you tested that also replying back from FES to such a message works smoothly?

DenBond7 · 2022-01-04T10:05:05Z

Have you tested that also replying back from FES to such a message works smoothly?

Do you mean check I it works or not at all?

I was able to send a new message, reply and forward with attachments.

But for that need to add a few temporary changes. Would you like to test that?

tomholub · 2022-01-04T10:45:16Z

Have you tested that also replying back from FES to such a message works smoothly?
Do you mean check I it works or not at all?
I was able to send a new message, reply and forward with attachments.
But for that need to add a few temporary changes. Would you like to test that?

I'd like you to test this scenario manually - make sure it works:

send a message from Android app through FES
open the received link on the other account, enter password. Make sure the message opens on fes.flowcrypt.com/...
click reply on fes.flowcrypt.com/...
compose a reply on the web page and send it back
make sure you receive the reply back in the android app

DenBond7 · 2022-01-04T14:36:10Z

@tomholub

I'd like you to test this scenario manually - make sure it works:

I'm trying to find a way do that. My private key for den@flowcrypt.com is expired since Jan 1

DenBond7 · 2022-01-04T15:00:22Z

send a message from Android app through FES
open the received link on the other account, enter password. Make sure the message opens on fes.flowcrypt.com/...
click reply on fes.flowcrypt.com/...

send a message from Android app through FES - works
open the received link on the other account, enter password. Make sure the message opens on fes.flowcrypt.com/...- works
click reply on fes.flowcrypt.com/...- I don't see reply button for a message that was send from Android. But I see this button for the web realization

It seems I have bug on my side. Can we debug this case?

@tomholub Please let me know what info I should provide.

DenBond7 · 2022-01-05T08:10:36Z

I've added changes. @tomholub, @IvanPizhenko Please re-review

tomholub · 2022-01-05T12:59:36Z

Difficult to test (explain why). It relates to background tasks. Tested manually.

We should file an issue to write test for this later. The functionality is not simple, and therefore easy to break during refactors or other development. Plus a customer relies on this. Having a test will help ensure it doesn't break.

tomholub

Thank you - it's improving - please see comments.

IvanPizhenko

I am ok with last changes (to the my extent of understanding of this PR). However @tomholub seems to have more comments, so let him approve and merge this PR when he is also ok.

IvanPizhenko · 2022-01-05T14:17:49Z

or I should also approve because I was requesting changes?

tomholub · 2022-01-05T14:26:46Z

Good question, I guess once it's ready I can force-merge it as an admin.

DenBond7 · 2022-01-05T16:03:49Z

However @tomholub seems to have more comments, so let him approve and merge this PR when he is also ok.

Good question, I guess once it's ready I can force-merge it as an admin.

I have enaugth permission to merge it as an admin. Anyway, I will do it only after @tomholub approving. It's important architecture/security issue.

DenBond7 · 2022-01-06T10:56:28Z

@tomholub Can I already merge it?

tomholub · 2022-01-06T11:06:09Z

yup

DenBond7and others added 4 commits December 27, 2021 15:34

Modified MessageEntity. Updated database version to 29.|#1622
441a8f7

Modified PgpEncryptAndOrSign. Added ability to encrypt with password.|…
6339081
…#1622

Modified a logic of creating encrypted message to send encrypted cont…
e65289d
…ent as an attachment. Refactored code.| #1622

Added PreparePasswordProtectedMsgWorker. Refactored code.|#1622
0bdac1e

DenBond7 added this to the 1.2.7: Customer feedback - must have II milestone Dec 30, 2021

DenBond7 added 7 commits December 30, 2021 12:50

Renamed BasePrepareMsgWorker to BaseMsgWorker.|#1622
c72f634

Renamed PasswordProtectedMsgWorker to HandlePasswordProtectedMsgWorke…
ef68da7
…r.| #1622

Improved HandlePasswordProtectedMsgWorker. Step 1.|#1622
9b1674f

Update labels for MessageState.NEW_PASSWORD_PROTECTED|#1622
0f50d11

Fixed files encryption(PgpEncryptAndOrSign).|#1622
3c8fce2

Fixed sending msgs with attachments via FES.|#1622
c979701

Fixed sending msgs with attachments(forwarded) via FES.|#1622
445bf56

DenBond7 modified the milestones: 1.2.7: Customer feedback - must have II, 1.2.8: Customer feedback - must have II Dec 30, 2021

DenBond7 added 5 commits January 3, 2022 09:16

Merge branch 'master' into issue_1622_send_encrypted_msg_through_fes
899fa88
# Conflicts: # FlowCrypt/src/main/java/com/flowcrypt/email/security/pgp/PgpEncryptAndOrSign.kt

Added handling errors + resend strategy.|#1622
bb1a66b

Updated "message.asc" MIME part.|#1622
5c2cdf2

Reverted back some changes.|#1622
9ff1602

Fixed MessageEntity(Parcelable implementation).|#1622
3285360

DenBond7 marked this pull request as ready for review January 3, 2022 11:55

DenBond7 requested a review from IvanPizhenko as a code owner January 3, 2022 11:55

DenBond7 requested a review from tomholub January 3, 2022 11:55

DenBond7 commented Jan 3, 2022
View reviewed changes

Comment thread...pt/src/main/java/com/flowcrypt/email/jetpack/workmanager/HandlePasswordProtectedMsgWorker.kt

DenBond7 commented Jan 3, 2022
View reviewed changes

Comment thread...pt/src/main/java/com/flowcrypt/email/jetpack/workmanager/HandlePasswordProtectedMsgWorker.kt Outdated

IvanPizhenko suggested changes Jan 3, 2022
View reviewed changes

tomholub reviewed Jan 3, 2022
View reviewed changes

DenBond7 added 2 commits January 4, 2022 10:21

Refactored code.|#1622
a7b0e7e

Refactored code. Step 2.|#1622
bc536a6

Refactored code. Step 3.|#1622
e3fc9f3

IvanPizhenko reviewed Jan 4, 2022
View reviewed changes

Comment threadFlowCrypt/src/main/java/com/flowcrypt/email/security/pgp/PgpEncryptAndOrSign.kt Outdated

tomholub mentioned this pull request Jan 4, 2022
debug / fix sending web portal message so that it can be replied online #1636
Closed

Refactored code. Step 4.|#1622
a9093b4

DenBond7 requested review from IvanPizhenko and tomholub January 5, 2022 08:09

tomholub reviewed Jan 5, 2022
View reviewed changes

tomholub mentioned this pull request Jan 5, 2022
automated test for sending password message through FES web portal #1639
Closed

IvanPizhenko reviewed Jan 5, 2022
View reviewed changes

Refactored code. Step 5.|#1622
115ce5f

tomholub approved these changes Jan 5, 2022
View reviewed changes

tomholub mentioned this pull request Jan 6, 2022
discussing / documenting / potentially refactoring outgoing message code #1643
Open

DenBond7 merged commit 0ca82f8 into masterJan 6, 2022

DenBond7 deleted the issue_1622_send_encrypted_msg_through_fes branch January 6, 2022 11:07

Uh oh!

Conversation

DenBond7 commented Dec 30, 2021• edited by tomholub Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

To be filled by reviewers

Uh oh!

Uh oh!

Uh oh!

IvanPizhenko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tomholub left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DenBond7 commented Jan 4, 2022

Uh oh!

tomholub commented Jan 4, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DenBond7 commented Jan 4, 2022

Uh oh!

DenBond7 commented Jan 4, 2022

Uh oh!

Uh oh!

DenBond7 commented Jan 5, 2022

Uh oh!

tomholub commented Jan 5, 2022

Uh oh!

tomholub left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

IvanPizhenko left a comment

Choose a reason for hiding this comment

Uh oh!

IvanPizhenko commented Jan 5, 2022

Uh oh!

tomholub commented Jan 5, 2022

Uh oh!

DenBond7 commented Jan 5, 2022

Uh oh!

DenBond7 commented Jan 6, 2022

Uh oh!

tomholub commented Jan 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

DenBond7 commented Dec 30, 2021•
edited by tomholub
Loading

tomholub commented Jan 4, 2022•
edited
Loading