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

Upload fallback keys with Crypto V2 #1697

Merged
merged 1 commit into from
Jan 27, 2023
Merged

Upload fallback keys with Crypto V2 #1697

merged 1 commit into from
Jan 27, 2023

Conversation

Anderas
Copy link
Contributor

@Anderas Anderas commented Jan 26, 2023

Upload fallback keys via keys/upload whenever present in the body of UploadKeysRequest.

Additionally:

  • move MXKeysQueryScheduler into MXCryptoRequests (rather than the other way around), which hides away the details of making keys/query requests
  • rename MXCryptoSDKLogger
  • additional log messages

@Anderas Anderas requested review from a team and pixlwave and removed request for a team January 26, 2023 16:37
@codecov
Copy link

codecov bot commented Jan 26, 2023

Codecov Report

Base: 37.52% // Head: 37.53% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (d9db756) compared to base (aebb192).
Patch coverage: 62.79% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #1697   +/-   ##
========================================
  Coverage    37.52%   37.53%           
========================================
  Files          609      609           
  Lines        95117    95137   +20     
  Branches     41283    41297   +14     
========================================
+ Hits         35695    35710   +15     
- Misses       58383    58386    +3     
- Partials      1039     1041    +2     
Impacted Files Coverage Δ
...ixSDK/Background/Crypto/MXBackgroundCryptoV2.swift 0.00% <0.00%> (ø)
MatrixSDK/Crypto/MXCryptoV2.swift 14.64% <0.00%> (-0.21%) ⬇️
...trixSDK/Crypto/CryptoMachine/MXCryptoMachine.swift 15.20% <62.50%> (-0.68%) ⬇️
...rixSDK/Crypto/CryptoMachine/MXCryptoRequests.swift 37.27% <80.00%> (+1.52%) ⬆️
...ixSDK/Crypto/CryptoMachine/MXCryptoSDKLogger.swift 83.33% <100.00%> (ø)
...trixSDK/Crypto/Migration/MXCryptoMigrationV2.swift 84.34% <100.00%> (ø)
...ypto/CryptoMachine/MXCryptoRequestsUnitTests.swift 93.45% <100.00%> (+0.52%) ⬆️
MatrixSDK/JSONModels/MXJSONModel.h 90.62% <0.00%> (-3.13%) ⬇️
...SDK/Crypto/Verification/MXKeyVerificationManager.m 53.76% <0.00%> (-0.36%) ⬇️
...Data/Store/MXRealmCryptoStore/MXRealmCryptoStore.m 69.93% <0.00%> (-0.22%) ⬇️
... and 4 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@pixlwave pixlwave left a comment

Choose a reason for hiding this comment

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

Code looks good to me, although I don't have a huge amount of context around the changes :)

@@ -60,6 +60,8 @@ class MXBackgroundCryptoV2: MXBackgroundCrypto {
- to-device events : \(syncResponse.toDevice?.events.count ?? 0)
- devices changed : \(syncResponse.deviceLists?.changed?.count ?? 0)
- devices left : \(syncResponse.deviceLists?.left?.count ?? 0)
- one time keys : \(syncResponse.deviceOneTimeKeysCount?[kMXKeySignedCurve25519Type] ?? 0)
- fallback keys : \(syncResponse.unusedFallbackKeys ?? [])
Copy link
Member

Choose a reason for hiding this comment

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

I'm sure its not, but double checking: Is this going to log keys that are sensitive?

Copy link

Choose a reason for hiding this comment

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

Those keys are public, after all they get uploaded to the server.

Copy link
Contributor Author

@Anderas Anderas Jan 27, 2023

Choose a reason for hiding this comment

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

Yea, actually in this particular log we only print the count of keys (or type in the case of fallback keys), but I have also started logging the actual keys elsewhere so your question is valid (and it is safe as we only print the public keys)

@Anderas Anderas merged commit 735fac4 into develop Jan 27, 2023
@Anderas Anderas deleted the andy/changes branch January 27, 2023 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants