Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: bulletproofing crypto box to cc migration (WPB-14250) #3123

Merged
merged 20 commits into from
Nov 26, 2024

Conversation

yamilmedina
Copy link
Contributor

@yamilmedina yamilmedina commented Nov 22, 2024

StoryWPB-14250 [Android] implement fall guards for CC migration


PR Submission Checklist for internal contributors

  • The PR Title

    • conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
    • contains a reference JIRA issue number like SQPIT-764
    • answers the question: If merged, this PR will: ... ³
  • The PR Description

    • is free of optional paragraphs and you have filled the relevant parts to the best of your ability

What's new in this PR?

Issues

When enabling core crypto storage, if there are any Proteus clients, we need to migrate them from CryptoBox.
Things usually don't go as planned, so we need to have a recovery plan in place.

Causes (Optional)

There might be some errors while migrating.

Solutions

Implement a recovery plan for this case:

  • Catch possible exceptions from migration, we were not handling it and assuming success
  • Perform logout, using a new LogoutReason, so we can act (cleanup) accordingly
    • Cleanup local crypto files
    • Cleanup from Metadata all related client info (retained id, current id, prekeys, etc.)
    • Set the refresh token to needs update.

If everything goes smoothly, the user will be prompted to login again, preserving their local history.

Dependencies (Optional)

Needs releases with:

  • GitHub link to other pull request

Testing

Test Coverage (Optional)

  • I have added automated test to this contribution

Notes (Optional)

Note

This approach seems "more correct", since if we try to create a new device only -as the ticket suggested- we will run into the issue of the refresh token not being valid anymore, since it was associated with the broken client that we were trying to migrate. And we can't associate the previous refresh token with a different client, we get a 403.

  • We will avoid other edge cases that we might not sure.
  • All login cases will be covered (2FA, SCIM, etc.)
  • We can expand this handling in the future (if we want) to other cases that we want to recover.

PR Post Submission Checklist for internal contributors (Optional)

  • Wire's Github Workflow has automatically linked the PR to a JIRA issue

PR Post Merge Checklist for internal contributors

  • If any soft of configuration variable was introduced by this PR, it has been added to the relevant documents and the CI jobs have been updated.

References
  1. https://sparkbox.com/foundry/semantic_commit_messages
  2. https://github.com/wireapp/.github#usage
  3. E.g. feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.

Copy link
Contributor

github-actions bot commented Nov 22, 2024

Test Results

3 267 tests  +3   3 160 ✅ +3   4m 59s ⏱️ +31s
  557 suites +2     107 💤 ±0 
  557 files   +2       0 ❌ ±0 

Results for commit 5b2361e. ± Comparison against base commit 7440f44.

♻️ This comment has been updated with latest results.

@datadog-wireapp
Copy link

datadog-wireapp bot commented Nov 22, 2024

Datadog Report

Branch report: chore/bulletproofing-cc-migration-rc
Commit report: 5d07895
Test service: kalium-jvm

✅ 0 Failed, 3160 Passed, 107 Skipped, 37.03s Total Time

Copy link
Member

@vitorhugods vitorhugods left a comment

Choose a reason for hiding this comment

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

LGTM. Would just wait for the dummy exception to be removed :D

@codecov-commenter
Copy link

codecov-commenter commented Nov 25, 2024

Codecov Report

Attention: Patch coverage is 69.23077% with 16 lines in your changes missing coverage. Please review.

Project coverage is 52.69%. Comparing base (7440f44) to head (5b2361e).

Files with missing lines Patch % Lines
.../kalium/logic/data/client/ProteusClientProvider.kt 60.00% 8 Missing ⚠️
...ture/client/ProteusMigrationRecoveryHandlerImpl.kt 72.72% 3 Missing ⚠️
.../com/wire/kalium/logic/data/logout/LogoutReason.kt 0.00% 2 Missing ⚠️
...om/wire/kalium/logic/data/session/SessionMapper.kt 0.00% 2 Missing ⚠️
...kalium/logic/sync/slow/SlowSyncCriteriaProvider.kt 0.00% 1 Missing ⚠️
Additional details and impacted files
@@                  Coverage Diff                  @@
##           release/candidate    #3123      +/-   ##
=====================================================
+ Coverage              52.64%   52.69%   +0.05%     
=====================================================
  Files                   1321     1322       +1     
  Lines                  51615    51662      +47     
  Branches                4781     4781              
=====================================================
+ Hits                   27172    27224      +52     
+ Misses                 22487    22480       -7     
- Partials                1956     1958       +2     
Files with missing lines Coverage Δ
...kalium/cryptography/exceptions/ProteusException.kt 50.00% <100.00%> (+2.00%) ⬆️
...om/wire/kalium/logic/feature/auth/LogoutUseCase.kt 95.52% <100.00%> (+2.41%) ⬆️
...lium/logic/feature/client/RegisterClientUseCase.kt 86.11% <100.00%> (+0.12%) ⬆️
.../com/wire/kalium/persistence/model/LogoutReason.kt 100.00% <100.00%> (ø)
...kalium/logic/sync/slow/SlowSyncCriteriaProvider.kt 80.55% <0.00%> (-2.31%) ⬇️
.../com/wire/kalium/logic/data/logout/LogoutReason.kt 0.00% <0.00%> (ø)
...om/wire/kalium/logic/data/session/SessionMapper.kt 48.10% <0.00%> (-1.25%) ⬇️
...ture/client/ProteusMigrationRecoveryHandlerImpl.kt 72.72% <72.72%> (ø)
.../kalium/logic/data/client/ProteusClientProvider.kt 38.33% <60.00%> (+38.33%) ⬆️

... and 4 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7440f44...5b2361e. Read the comment docs.

Copy link
Contributor

@Garzas Garzas left a comment

Choose a reason for hiding this comment

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

Looking good 🏅
Just 2 questions about error code and tests

@@ -199,3 +199,6 @@ class ProteusException(message: String?, val code: Code, val intCode: Int?, caus
}
}
}

class ProteusStorageMigrationException(override val message: String, val rootCause: Throwable? = null) :
ProteusException(message, Int.MIN_VALUE, null)
Copy link
Contributor

Choose a reason for hiding this comment

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

Small suggestion, maybe we should have some static error code variable to not duplicate codes in future?

Copy link

sonarcloud bot commented Nov 26, 2024

@yamilmedina yamilmedina added this pull request to the merge queue Nov 26, 2024
Merged via the queue into release/candidate with commit a307ae3 Nov 26, 2024
29 checks passed
@yamilmedina yamilmedina deleted the chore/bulletproofing-cc-migration-rc branch November 26, 2024 17:15
github-actions bot pushed a commit that referenced this pull request Nov 26, 2024
* feat: cleaning all data if failure

* feat: cleaning all data if failure

* feat: clean up code

* feat: clean up code

* chore: add plan b, logout user

* chore: add plan b, logout user, cleanup code

* chore: cleanup code

* chore: cleanup code

* chore: cleanup code detekt

* chore: cleanup code detekt

* chore: remove exception for testing

* chore: tests for new branches

* chore: tests for new branches

* chore: solve layer issue

* chore: more test covereage

* chore: layering

* chore: solve missing reference

* fix: deadlock while cleanup

* fix: tests

* fix: tests detekt
github-merge-queue bot pushed a commit that referenced this pull request Nov 27, 2024
* chore: bulletproofing crypto box to cc migration (WPB-14250) (#3123)

* feat: cleaning all data if failure

* feat: cleaning all data if failure

* feat: clean up code

* feat: clean up code

* chore: add plan b, logout user

* chore: add plan b, logout user, cleanup code

* chore: cleanup code

* chore: cleanup code

* chore: cleanup code detekt

* chore: cleanup code detekt

* chore: remove exception for testing

* chore: tests for new branches

* chore: tests for new branches

* chore: solve layer issue

* chore: more test covereage

* chore: layering

* chore: solve missing reference

* fix: deadlock while cleanup

* fix: tests

* fix: tests detekt

* chore: empty commit bump

---------

Co-authored-by: Yamil Medina <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants