support multiple pub keys per recipient #1523

DenBond7 · 2021-10-26T15:23:03Z

This PR added support multiple pub keys for recipient

close #1188
close #1539
close #1540
close #1541
close #1542
close #1566
close #1570

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

…WhichMatched().| #1188

…1188

DenBond7 · 2021-11-24T08:08:51Z

@tomholub @IvanPizhenko It's ready for a review. Sorry, that is a huge and terrible PR. But I could not split it into a few smaller PRs. One thing relates to another one and so on. I had to change too many things.

tomholub

Here you go.

General comment - the codebase is quite massive when I feel maybe less code could achieve the same thing. Often there are bits of functionality duplicated, like once suspend and once not, once xxxFlow and once not and so on. I hope, with some refactoring (not now) it could be streamlined. Else it will be a lot of work for you to make changes if many things are implemented twice to use in similar but a bit different ways.

...m/flowcrypt/email/ui/activity/ImportPgpContactActivityDisallowAttesterSearchForDomainTest.kt

.../src/androidTest/java/com/flowcrypt/email/ui/activity/PreviewImportPgpContactActivityTest.kt

FlowCrypt/src/main/java/com/flowcrypt/email/database/FlowCryptRoomDatabase.kt

FlowCrypt/src/main/java/com/flowcrypt/email/database/dao/PubKeyDao.kt

FlowCrypt/src/main/java/com/flowcrypt/email/ui/activity/ImportPublicKeyActivity.kt

tomholub · 2021-11-24T19:32:56Z

FlowCrypt/src/main/java/com/flowcrypt/email/ui/activity/fragment/MessageDetailsFragment.kt

+      } else {
+        //todo-denbond7 temporary disable updating
+        button.gone()
+      }/*else if (existingRecipientWithPubKeys.publicKeys.firstOrNull()?.fingerprint.isNullOrEmpty()
        || keyDetails?.fingerprint?.equals(
-          existingPgpContact.fingerprint!!, ignoreCase = true
+          existingRecipientWithPubKeys.publicKeys.firstOrNull()?.fingerprint, ignoreCase = true
        ) == true
      ) {
        initUpdateContactButton(block, button)
      } else {
        initReplaceContactButton(block, button)
-      }
+      }*/


Here it should be similar to other places:

if we already have this particular public key (by fingerprint) saved for this recipient, then we compare by last modified. If the one we already have is newer or same, we tell the user. If the received one is newer, we offer import (with "update")

if we don't have that pubkey for this recipient by fingerprint, offer import (with "save")

So it's not firstOrNull but we instead check if it's in the list, and then whether it's newer or not.

Interesting. It seems I've disabled some functionality in MessageDetailsFragment.kt during migration and forgotten to fix it.(too many changes in this PR :( ) I have to fix that.

Please file a separate issue for the future to add a test for this. Since it could be disabled without noticing, it's not covered by any test.

tomholub · 2021-11-24T19:34:25Z

FlowCrypt/src/main/java/com/flowcrypt/email/ui/activity/fragment/MessageDetailsFragment.kt

+    /*button.setOnClickListener { v ->
+      val RecipientWithPubKeys = block.keyDetails?.primaryRecipientWithPubKeys
+      if (RecipientWithPubKeys != null) {
+        recipientsViewModel.updateContact(RecipientWithPubKeys)


FlowCrypt/src/main/java/com/flowcrypt/email/ui/activity/fragment/base/CreateMessageFragment.kt

FlowCrypt/src/main/java/com/flowcrypt/email/ui/adapter/PgpContactAdapter.kt

tomholub

Per above

FlowCrypt/src/main/java/com/flowcrypt/email/database/FlowCryptRoomDatabase.kt

IvanPizhenko

Some comments. To say true, yes, it is really huge, and I don't have a lot to say here, since I'm not specialist in Android and not always get the rest of application logic. I've seen Tom made more meaningful comments.

FlowCrypt/src/main/java/com/flowcrypt/email/database/dao/RecipientDao.kt

FlowCrypt/src/main/java/com/flowcrypt/email/database/FlowCryptRoomDatabase.kt

FlowCrypt/src/main/java/com/flowcrypt/email/database/dao/RecipientDao.kt

FlowCrypt/src/main/java/com/flowcrypt/email/database/entity/PublicKeyEntity.kt

FlowCrypt/src/main/java/com/flowcrypt/email/jetpack/workmanager/sync/LoadRecipientsWorker.kt

FlowCrypt/src/main/java/com/flowcrypt/email/ui/activity/ImportPublicKeyActivity.kt

FlowCrypt/src/main/java/com/flowcrypt/email/ui/activity/fragment/ParseAndSavePubKeysFragment.kt

FlowCrypt/src/main/java/com/flowcrypt/email/ui/activity/fragment/PublicKeyDetailsFragment.kt

DenBond7 · 2021-11-25T08:36:30Z

General comment - the codebase is quite massive when I feel maybe less code could achieve the same thing. Often there are bits of functionality duplicated, like once suspend and once not, once xxxFlow and once not and so on.

Let me explain why we have the following situation. It relates to the current situation in code:

Our code needs architecture improvements (Important architecture changes #1035)
For now we have different ways to resolve the similar problems over the whole app

a) we have the old code that uses standard Java Threads. That's why we have to have for example this method

@Query("SELECT * FROM sqlite")
fun getRows(): List<Row>

b) we have code that uses coroutines. It requires the following version. Usually, such methods are used to fetch some info once. Like finding some entity and so on.

@Query("SELECT * FROM sqlite")
suspend fun getRows(): List<Row>

c) we have code that uses coroutines and Flow. Usually, such methods are used to fetch some info and observe changes. Like fetching a list and receiving updates: insert, delete, update items.

@Query("SELECT * FROM sqlite")
suspend fun getRowsFlow(): Flow<List<Row>>

I agree that we should drop a) and migrate to use b) and c) depending on our needs.

Just need to find some free time for that (between adding new functionality) :))

DenBond7 · 2021-11-25T08:38:37Z

@IvanPizhenko @tomholub By the way, please mark a conversation as resolved if you don't have any other comments and it looks ok after my changes. It will help me to see what is already done and what I should fix. Thanks!

…multiply_pub_keys_for_recipient # Conflicts: # FlowCrypt/src/main/java/com/flowcrypt/email/jetpack/viewmodel/ContactsViewModel.kt # FlowCrypt/src/main/java/com/flowcrypt/email/ui/activity/SelectRecipientsActivity.kt

tomholub · 2021-11-25T12:16:56Z

@IvanPizhenko @tomholub By the way, please mark a conversation as resolved if you don't have any other comments and it looks ok after my changes. It will help me to see what is already done and what I should fix. Thanks!

I've resolved some that don't need any code change. But let's do it the other way - if you made a code change that you believe resolves our concern, then please mark it as resolved. All of the comments are quite minor and you are the primary person who interacts with this codebase, so it's the most important that it works for you rather than that it works for us (except for things that affect functionality directly, but I guess there weren't any this time).

tomholub · 2021-11-25T12:20:14Z

General comment - the codebase is quite massive when I feel maybe less code could achieve the same thing. Often there are bits of functionality duplicated, like once suspend and once not, once xxxFlow and once not and so on.

Let me explain why we have the following situation. It relates to the current situation in code:
1. Our code needs architecture improvements ([Important architecture changes #1035](https://github.com/FlowCrypt/flowcrypt-android/issues/1035))

2. For now we have different ways to resolve the similar problems over the whole app
a) we have the old code that uses standard Java Threads. That's why we have to have for example this method
@Query("SELECT * FROM sqlite")
fun getRows(): List<Row>
b) we have code that uses coroutines. It requires the following version. Usually, such methods are used to fetch some info once. Like finding some entity and so on.
@Query("SELECT * FROM sqlite")
suspend fun getRows(): List<Row>
c) we have code that uses coroutines and Flow. Usually, such methods are used to fetch some info and observe changes. Like fetching a list and receiving updates: insert, delete, update items.
@Query("SELECT * FROM sqlite")
suspend fun getRowsFlow(): Flow<List<Row>>
I agree that we should drop a) and migrate to use b) and c) depending on our needs.

Just need to find some free time for that (between adding new functionality) :))

All good. I just wanted to make sure that we both don't like the situation. Yes, I know I haven't given you any time for refactoring lately. And it looks like there won't be any until at least February :-(

tomholub · 2021-11-25T12:20:51Z

because of #1550

tomholub

checkpoint review - so far good

tomholub · 2021-11-25T15:54:27Z

FlowCrypt/src/main/java/com/flowcrypt/email/database/dao/PubKeyDao.kt

+  companion object {
+    private const val SELECT_ALL_PUB_KEYS = "SELECT * FROM public_keys"
+  }


That means in the future we may be doing this for all. But that's also for another issue.

tomholub

.

DenBond7 added 7 commits October 23, 2021 12:44

Added PublicKeyEntity. Refactored code. WIP.| #1188

21ca549

Fixed using ContactsDao.getAllContactsWithPgpLD().| #1188

ae23630

Fixed using ContactsDao.getAllContactsWithPgp()/getAllContactsWithPgp…

77c8855

…WhichMatched().| #1188

Renamed ContactEntity to RecipientEntity.| #1188

faa2e23

Renamed ContactsDao to RecipientDao.| #1188

fda5a76

Refactored code.| #1188

91de167

Renamed ContactsViewModel to RecipientsViewModel.| #1188

905aacf

DenBond7 added this to the 1.2.5: WKD public key lifecycle milestone Oct 26, 2021

DenBond7 added 3 commits October 26, 2021 18:39

Fixed a few methods in RecipientDao. Refactored code.| #1188

55355f0

Fixed RecipientDao.getRecipientByEmail().| #1188

c36f3ad

Fixed some moments with adding/updating a recipient with a pub key.| #…

5803a9e

…1188

DenBond7 force-pushed the issue_1188_support_multiply_pub_keys_for_recipient branch 3 times, most recently from 5c30eb9 to 589c384 Compare October 28, 2021 16:11

tomholub changed the title ~~Issue 1188 support multiply pub keys for recipient~~ Issue 1188 support multiple pub keys per recipient Oct 28, 2021

DenBond7 force-pushed the issue_1188_support_multiply_pub_keys_for_recipient branch 15 times, most recently from b1ebbc3 to b53ba16 Compare November 1, 2021 16:05

DenBond7 requested a review from IvanPizhenko as a code owner November 24, 2021 08:04

DenBond7 mentioned this pull request Nov 24, 2021

app crash by importing a private key without an email address #1570

Closed

tomholub reviewed Nov 24, 2021

View reviewed changes

tomholub requested changes Nov 24, 2021

View reviewed changes

IvanPizhenko reviewed Nov 24, 2021

View reviewed changes

FlowCrypt/src/main/java/com/flowcrypt/email/database/FlowCryptRoomDatabase.kt Show resolved Hide resolved

IvanPizhenko reviewed Nov 24, 2021

View reviewed changes

DenBond7 added 2 commits November 25, 2021 10:40

Fixed merge conflicts.| #1188

c84d571

DenBond7 marked this pull request as draft November 25, 2021 08:42

Fixed some PR comments. Refactored code.| #1188

1a76ddd

DenBond7 mentioned this pull request Nov 25, 2021

AndroidStudio can't syntax-highlight multiple line SQL - report to maintainers #1574

Closed

DenBond7 mentioned this pull request Nov 25, 2021

Add a test for saving/updating a public key from on the message details screen #1575

Closed

DenBond7 added 3 commits November 25, 2021 16:39

Fixed some PR comments. Refactored code. Step 2.| #1188

e6e08c2

Restored some code.| #1188

b9a8b9d

Renamed a file.| #1188

35a6e09

tomholub reviewed Nov 25, 2021

View reviewed changes

Fixed importing/updating pub keys for PublicKeyMsgBlock.| #1188

1170e2f

tomholub reviewed Nov 25, 2021

View reviewed changes

tomholub mentioned this pull request Nov 25, 2021

us constants for SQL queries when used by more than one method #1576

Open

DenBond7 marked this pull request as ready for review November 25, 2021 17:44

DenBond7 requested a review from tomholub November 25, 2021 17:44

tomholub approved these changes Nov 25, 2021

View reviewed changes

tomholub merged commit 20cd64b into master Nov 25, 2021

tomholub deleted the issue_1188_support_multiply_pub_keys_for_recipient branch November 25, 2021 18:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support multiple pub keys per recipient #1523

support multiple pub keys per recipient #1523

DenBond7 commented Oct 26, 2021 •

edited by tomholub

Loading

DenBond7 commented Nov 24, 2021

tomholub left a comment

tomholub Nov 24, 2021

DenBond7 Nov 25, 2021

tomholub Nov 25, 2021

tomholub Nov 24, 2021

tomholub left a comment

IvanPizhenko left a comment •

edited

Loading

DenBond7 commented Nov 25, 2021

DenBond7 commented Nov 25, 2021

tomholub commented Nov 25, 2021

tomholub commented Nov 25, 2021

tomholub commented Nov 25, 2021

tomholub left a comment

tomholub Nov 25, 2021

tomholub left a comment

support multiple pub keys per recipient #1523

support multiple pub keys per recipient #1523

Conversation

DenBond7 commented Oct 26, 2021 • edited by tomholub Loading

To be filled by reviewers

DenBond7 commented Nov 24, 2021

tomholub left a comment

Choose a reason for hiding this comment

tomholub Nov 24, 2021

Choose a reason for hiding this comment

DenBond7 Nov 25, 2021

Choose a reason for hiding this comment

tomholub Nov 25, 2021

Choose a reason for hiding this comment

tomholub Nov 24, 2021

Choose a reason for hiding this comment

tomholub left a comment

Choose a reason for hiding this comment

IvanPizhenko left a comment • edited Loading

Choose a reason for hiding this comment

DenBond7 commented Nov 25, 2021

DenBond7 commented Nov 25, 2021

tomholub commented Nov 25, 2021

tomholub commented Nov 25, 2021

tomholub commented Nov 25, 2021

tomholub left a comment

Choose a reason for hiding this comment

tomholub Nov 25, 2021

Choose a reason for hiding this comment

tomholub left a comment

Choose a reason for hiding this comment

DenBond7 commented Oct 26, 2021 •

edited by tomholub

Loading

IvanPizhenko left a comment •

edited

Loading