[PM-37886] refactor: Update Key Connector vault unlock to use SDK's keyConnectorUrl method#2748
[PM-37886] refactor: Update Key Connector vault unlock to use SDK's keyConnectorUrl method#2748matt-livefront wants to merge 4 commits into
Conversation
matt-livefront
added
ai-review
github-actions
Bot
added
the
app:password-manager
Bitwarden Claude Code ReviewOverall Assessment: APPROVE This PR migrates Code Review DetailsNo new findings. Previous IMPORTANT finding on |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2748 +/- ##
==========================================
+ Coverage 87.81% 87.82% +0.01%
==========================================
Files 1707 1704 -3
Lines 165707 165842 +135
==========================================
+ Hits 145517 145657 +140
+ Misses 20190 20185 -5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…eyConnectorUrl method
matt-livefront
force-pushed
the
matt/PM-37886-key-connector-url
branch
from
8517d4c to
221b3f5
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| coordinator.hideLoadingOverlay() | ||
| await coordinator.handleEvent(.didCompleteAuth) | ||
| } catch StateServiceError.noAccountCryptographicState { | ||
| coordinator.hideLoadingOverlay() |
There was a problem hiding this comment.
🎨 To one of the things Claude generated for this, is there a way we could test to make sure the overlay is hidden here?
🤔 Would the hiding of the overlay be something we could defer?
| coordinator.hideLoadingOverlay() | ||
| await coordinator.handleEvent(.didCompleteAuth) | ||
| } catch StateServiceError.noAccountCryptographicState { | ||
| coordinator.hideLoadingOverlay() |
There was a problem hiding this comment.
🎨 To one of the things Claude generated for this, is there a way we could test to make sure the overlay is hidden here?
🤔 Would the hiding of the overlay be something we could defer?
There was a problem hiding this comment.
🎨 To one of the things Claude generated for this, is there a way we could test to make sure the overlay is hidden here?
Yep, I updated the tests for this.
🤔 Would the hiding of the overlay be something we could
defer?
That would be ideal. At one point we ran into a problem where defer was causing the loading overlay to not hide when transitioning from the auth flow to the vault, which is why they are manually hidden in some places like this before completing auth. I'd like to clean this up, but I'd prefer to do it separately. I created a ticket: https://bitwarden.atlassian.net/browse/PM-38748.
|
Warning @matt-livefront Uploading code coverage report failed. Please check the "Upload to codecov.io" step of Process Test Reports job for more details. |
3 participants
🎟️ Tracking
PM-37886
📔 Objective
Migrates the Key Connector vault unlock to let the SDK handle Key Connector communication, removing the app's direct
GET /user-keysnetwork call.Changes
AuthRepository.unlockVaultWithKeyConnectorKey: switches fromInitUserCryptoMethod.keyConnector(masterKey:userKey:)(which required a priorgetMasterKeyFromKeyConnectorAPI call) toInitUserCryptoMethod.keyConnectorUrl(url:keyConnectorKeyWrappedUserKey:), which delegates the Key Connector fetch to the SDK.getMasterKeyFromKeyConnectorfromKeyConnectorServiceandKeyConnectorAPIServiceprotocols and implementations.KeyConnectorUserKeyRequestandKeyConnectorUserKeyResponseModel.