Integrated WKD in the app by DenBond7 · Pull Request #1399 · FlowCrypt/flowcrypt-android

DenBond7 · 2021-08-11T09:44:45Z

This PR added support of WKD in the app

close #1201

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

…1201

tomholub · 2021-08-11T09:57:42Z

FlowCrypt/src/main/java/com/flowcrypt/email/api/wkd/WkdClient.kt

-    val matchingKeys = keys.keyRings.asSequence().filter {
-      for (userId in it.publicKey.userIDs) {
-        try {
-          val parsed = BetterInternetAddress(userId)
-          if (parsed.emailAddress.toLowerCase(Locale.ROOT) == lowerCaseEmail) return@filter true
-        } catch (ex: Exception) {
-          // ignore
+  suspend fun lookupEmail(context: Context, email: String): Response<String> =
+    withContext(Dispatchers.IO) {
+      val pgpPublicKeyRingCollection = rawLookupEmail(context, email)
+      val lowerCaseEmail = email.toLowerCase(Locale.ROOT)
+      val firstMatchedKey = pgpPublicKeyRingCollection?.keyRings?.asSequence()?.filter {
+        for (userId in it.publicKey.userIDs) {
+          try {
+            val parsed = BetterInternetAddress(userId)
+            if (parsed.emailAddress.toLowerCase(Locale.ROOT) == lowerCaseEmail) return@filter true
+          } catch (ex: Exception) {
+            ex.printStackTrace()
+          }
        }
-      }
-      false
-    }.toList()
-    return if (matchingKeys.isNotEmpty()) PGPPublicKeyRingCollection(matchingKeys) else null
-  }
+        false


Please still return an array from WkdClient, and then pick the key in PubLookup. To pick the key, instead of blindly the first one, let's use the first one that is .usableForEncryption, and add a comment in the code pointing to the issues to fix it.

That way, after we refactor code to allow for more than one pubkey, we can leave WkdClient class and tests untouched.

.usableForEncryption - it seems that we don't have such code. How do we determine that?

I see. Let's leave usableForEncryption for later. For now, have WkdClient return an array, and choose the first result that is not expired and not revoked. (with a comment linking to #480 )

pgpainless/pgpainless#165

@DenBond7 you can use KeyRingInfo.getEncryptionSubkeys(purpose), where purpose is encryption. If it returns > 1 element, then the key is usable for encryption. Same for #480 later.

…1201

…o localhost:1212. | #1201

…. | #1201

IvanPizhenko

Some questions here

FlowCrypt/src/test/java/com/flowcrypt/email/api/email/WkdClientTest.kt

DenBond7 · 2021-08-14T17:14:24Z

@tomholub That's an important issue. I will merge it after your approve.

tomholub · 2021-08-14T20:31:49Z

"after your approval" or "after you approve" :-) I'll take a look

tomholub

Suggestions below to make distinction between public key sources clearer.

FlowCrypt/src/main/java/com/flowcrypt/email/api/retrofit/FlowcryptApiRepository.kt

FlowCrypt/src/main/java/com/flowcrypt/email/api/util/PubLookup.kt

tomholub · 2021-08-14T21:06:06Z

class AccountKeysInfoViewModel(application: Application) : AccountViewModel(application) {

This should maybe be called AccountPublicKeyServersViewModel. I don't have a good name for it.

tomholub · 2021-08-14T21:09:02Z

I have a general question. This getPub method (which we should call pubLookup instead to mimic the browser) has three usages. But none of the three usages look like they are related to fetching public keys in the message compose view.

How does the compose view fetch messages? Does it use the ContactsViewModel?

tomholub · 2021-08-14T21:10:08Z

Ok I see it now, it's the ContactsViewModel. I'll add a comment for clarity.

tomholub

Looks good, with some minor adjustments for increased clarity, as above.

DenBond7 · 2021-08-16T07:51:04Z

@tomholub I've added the requested changes. Please look at them.

Integrated WKD support in the app. Temporary disabled some tests.| #1201

440af72

DenBond7 added this to the 1.2.0: Enterprise features milestone Aug 11, 2021

Renamed CreateMessageActivityTestTest to CreateMessageActivityTest.| #…

cfdc190

…1201

tomholub reviewed Aug 11, 2021

View reviewed changes

DenBond7 added 7 commits August 11, 2021 15:02

Refactored code to use PubLookup.| #1201

3b75bd9

Added CreateMessageActivityWkdTest(not completed). Refactored code.| #…

ac073eb

…1201

Added more tests. Refactored code.| #1201

9ed061d

Refactored code.| #1201

4e9af72

Added one more test. Refactored code.| #1201

75bf2c0

Modified ci-wait-for-emulator.sh to route all traffic for localhost t…

1664798

…o localhost:1212. | #1201

Modified PubLookup to get first matching key by 'usableForEncryption'…

7ba8e44

…. | #1201

IvanPizhenko reviewed Aug 12, 2021

View reviewed changes

FlowCrypt/src/test/java/com/flowcrypt/email/api/email/WkdClientTest.kt Show resolved Hide resolved

DenBond7 added 3 commits August 13, 2021 09:50

Switched to use "platforms;android-30" fro tests on CI.| #1201

aa0d7fc

Modified existed tests.| #1201

81a102c

Restored WkdClientTest with a few changes.| #1201

7976139

DenBond7 marked this pull request as ready for review August 13, 2021 13:43

DenBond7 requested review from IvanPizhenko and tomholub August 13, 2021 13:43

This was referenced Aug 13, 2021

Bump detekt-gradle-plugin from 1.17.1 to 1.18.0 #1400

Merged

Bump classgraph from 4.8.112 to 4.8.113 #1398

Closed

IvanPizhenko previously approved these changes Aug 14, 2021

View reviewed changes

tomholub reviewed Aug 14, 2021

View reviewed changes

updated comment, renamed variable

835d7a3

tomholub dismissed IvanPizhenko’s stale review via 835d7a3 August 14, 2021 21:05

tomholub reviewed Aug 14, 2021

View reviewed changes

Tom J and others added 2 commits August 14, 2021 21:13

updated comment

658e2a3

Refactored code.| #1201

a7ed316

DenBond7 requested a review from tomholub August 16, 2021 07:51

tomholub approved these changes Aug 16, 2021

View reviewed changes

tomholub merged commit b97d345 into master Aug 16, 2021

tomholub deleted the issue_1201_wkd_support branch August 16, 2021 08:32

DenBond7 mentioned this pull request Jun 7, 2022

Added an ability to allow attester search only for domains.| #1827 #1831

Merged

4 tasks

DenBond7 mentioned this pull request Jan 26, 2023

Use 'flowcrypt.com/shared-tenant-fes' instead of 'flowcrypt.com/api' #2171

Merged

5 tasks

Conversation

DenBond7 commented Aug 11, 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!

tomholub Aug 11, 2021

Choose a reason for hiding this comment

Uh oh!

DenBond7 Aug 11, 2021

Choose a reason for hiding this comment

Uh oh!

DenBond7 Aug 11, 2021

Choose a reason for hiding this comment

Uh oh!

tomholub Aug 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomholub Aug 11, 2021

Choose a reason for hiding this comment

Uh oh!

tomholub Aug 11, 2021

Choose a reason for hiding this comment

Uh oh!

IvanPizhenko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DenBond7 commented Aug 14, 2021

Uh oh!

tomholub commented Aug 14, 2021

Uh oh!

tomholub left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tomholub commented Aug 14, 2021

Uh oh!

tomholub commented Aug 14, 2021

Uh oh!

tomholub commented Aug 14, 2021

Uh oh!

tomholub left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DenBond7 commented Aug 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

DenBond7 commented Aug 11, 2021 •

edited by tomholub

Loading

tomholub Aug 11, 2021 •

edited

Loading

tomholub left a comment •

edited

Loading