periodically refresh keys from ekm by DenBond7 · Pull Request #1761 · FlowCrypt/flowcrypt-android

DenBond7 · 2022-04-26T13:35:17Z

This PR added the ability to periodically refresh keys from ekm

close #1740

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

...pt/src/main/java/com/flowcrypt/email/jetpack/viewmodel/RefreshPrivateKeysFromEkmViewModel.kt

DenBond7 · 2022-04-28T16:14:06Z

@tomholub I've added a few places that maybe should be discussed.

tomholub

Here you go. It's not bad but it's missing one important part: also update keys that are not usable.

...pt/src/main/java/com/flowcrypt/email/jetpack/viewmodel/RefreshPrivateKeysFromEkmViewModel.kt

FlowCrypt/src/main/java/com/flowcrypt/email/model/KeyImportDetails.kt

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

tomholub

.

tomholub · 2022-04-29T12:05:03Z

Force-pushing makes it very hard to do code reviews 😞 because then I have to start reviewing all code again, instead of just reviewing delta from last review.

DenBond7 · 2022-04-29T12:08:44Z

Force-pushing makes it very hard to do code reviews disappointed because then I have to start reviewing all code again, instead of just reviewing delta from last review.

Ah, sorry. I didn't know that. I will not do it anymore. I've used the logic of wip (work in progress) comment. And pushed it to see what I have to do and what I already have. Sorry again.

tomholub · 2022-04-29T12:19:02Z

Force-pushing makes it very hard to do code reviews disappointed because then I have to start reviewing all code again, instead of just reviewing delta from last review.

Ah, sorry. I didn't know that. I will not do it anymore. I've used the logic of wip (work in progress) comment. And pushed it to see what I have to do and what I already have. Sorry again.

No worries. Generally, to make changes in the PR, it's best to add more commits to it. And if you need to bring changes from elsewhere, merge them into here with git merge. It doesn't really matter much before the PR receives its first review, but after that, GitHub forgets what I already reviewed after each force push.

tomholub

looks good - will take another look

...pt/src/main/java/com/flowcrypt/email/jetpack/viewmodel/RefreshPrivateKeysFromEkmViewModel.kt

…again)

…ForNotExistedUser().| #1740

...roidTest/java/com/flowcrypt/email/ui/activity/enterprise/MainSignInFragmentEnterpriseTest.kt

DenBond7 added 10 commits April 26, 2022 16:09

Added RefreshPrivateKeysFromEkmViewModel.| #1740

c7d050c

Refactored code.| #1740

8a9cdfa

Added retrieving missing passphrase to refresh keys from EKM.| #1740

fb16c08

Fixed FlowCryptMockWebServerRule.| #1740

08a8dd0

Added RefreshKeysFromEkmFlowTest.(not completed)| #1740

33d1999

Completed RefreshKeysFromEkmFlowTest.| #1740

40da28a

Refactored code.| #1740

f937f0f

Refactored code. Step 2.| #1740

5bb839f

Refactored code. Step 3.| #1740

e8bd906

Refactored code. Step 4.| #1740

9eff2f4

DenBond7 marked this pull request as ready for review April 28, 2022 16:10

DenBond7 requested a review from IvanPizhenko as a code owner April 28, 2022 16:10

DenBond7 added this to the 1.3.3: Periodic refresh from EKM milestone Apr 28, 2022

DenBond7 requested a review from tomholub April 28, 2022 16:11

DenBond7 commented Apr 28, 2022

View reviewed changes

...pt/src/main/java/com/flowcrypt/email/jetpack/viewmodel/RefreshPrivateKeysFromEkmViewModel.kt Outdated Show resolved Hide resolved

DenBond7 commented Apr 28, 2022

View reviewed changes

...pt/src/main/java/com/flowcrypt/email/jetpack/viewmodel/RefreshPrivateKeysFromEkmViewModel.kt Outdated Show resolved Hide resolved

DenBond7 commented Apr 28, 2022

View reviewed changes

...pt/src/main/java/com/flowcrypt/email/jetpack/viewmodel/RefreshPrivateKeysFromEkmViewModel.kt Outdated Show resolved Hide resolved

tomholub reviewed Apr 28, 2022

View reviewed changes

tomholub reviewed Apr 29, 2022

View reviewed changes

tomholub mentioned this pull request Apr 29, 2022

consider eventually allowing EKM setup without FORBID_STORING_PASS_PHRASE #1765

Open

wip

9f47a47

DenBond7 force-pushed the issue_1740_periodically_refresh_keys_from_EKM branch from 61f73f5 to 9f47a47 Compare April 29, 2022 12:02

Refactored code. Step 5.| #1740

83ed2ed

tomholub reviewed Apr 29, 2022

View reviewed changes

...pt/src/main/java/com/flowcrypt/email/jetpack/viewmodel/RefreshPrivateKeysFromEkmViewModel.kt Show resolved Hide resolved

tomholub previously approved these changes Apr 29, 2022

View reviewed changes

DenBond7 mentioned this pull request Apr 29, 2022

Remove publi key field from the 'keys' table as redundant #1766

Closed

Fixed some UI tests.| #1740

98878de

DenBond7 dismissed stale reviews from IvanPizhenko and tomholub via 98878de May 2, 2022 13:21

tomholub previously approved these changes May 2, 2022

View reviewed changes

IvanPizhenko previously approved these changes May 2, 2022

View reviewed changes

DenBond7 marked this pull request as draft May 3, 2022 05:25

debugging

3d4704c

DenBond7 dismissed stale reviews from IvanPizhenko and tomholub via 3d4704c May 3, 2022 05:43

DenBond7 added 14 commits May 3, 2022 10:05

debugging 1

137c90e

debugging 2

2a180bb

debugging 3

76371b3

debugging 4

d840773

Refactored code

4038d2a

debugging 6

3326a65

debugging 7

0ca3ba9

debugging 8

56b8629

debugging 9

158b39c

debugging 10

216dfe1

Reverted back some changes. Temporary disabled some tests.

58abbd8

Reverted back changes in ci-instrumentation-tests-with-mailserver.sh

759b814

Reverted back changes in ci-instrumentation-tests-with-mailserver.sh(…

657604b

…again)

Fixed ImportRecipientsFromSourceFragmentTest.testFetchKeyFromAttester…

859f89f

…ForNotExistedUser().| #1740

DenBond7 marked this pull request as ready for review May 3, 2022 16:16

DenBond7 requested review from IvanPizhenko and tomholub May 3, 2022 16:16

tomholub approved these changes May 3, 2022

View reviewed changes

...roidTest/java/com/flowcrypt/email/ui/activity/enterprise/MainSignInFragmentEnterpriseTest.kt Show resolved Hide resolved

DenBond7 mentioned this pull request May 3, 2022

Fix tests for fes.localhost #1772

Closed

tomholub merged commit 6e934a0 into master May 3, 2022

tomholub deleted the issue_1740_periodically_refresh_keys_from_EKM branch May 3, 2022 16:47

Conversation

DenBond7 commented Apr 26, 2022 • 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!

Uh oh!

DenBond7 commented Apr 28, 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!

Uh oh!

Uh oh!

Uh oh!

tomholub left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

DenBond7 commented Apr 29, 2022

Uh oh!

tomholub commented Apr 29, 2022

Uh oh!

tomholub left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

DenBond7 commented Apr 26, 2022 •

edited by tomholub

Loading

tomholub commented Apr 29, 2022 •

edited

Loading