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

Sdk user story #5952

Merged
merged 22 commits into from
May 30, 2022
Merged
Changes from 1 commit
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Change in KeysBackupService: isEnabled and isStucked are now fun,…
… and `state` has been renamed to `getState` and is now a fun.
bmarty committed May 20, 2022
commit ede784684f6be4ee8b2327c7b64667375b8e0046
Original file line number Diff line number Diff line change
@@ -113,7 +113,7 @@ class KeysBackupTest : InstrumentedTest {

val stateObserver = StateObserver(keysBackup)

assertFalse(keysBackup.isEnabled)
assertFalse(keysBackup.isEnabled())

val megolmBackupCreationInfo = testHelper.doSync<MegolmBackupCreationInfo> {
keysBackup.prepareKeysBackupVersion(null, null, it)
@@ -143,21 +143,21 @@ class KeysBackupTest : InstrumentedTest {

val stateObserver = StateObserver(keysBackup)

assertFalse(keysBackup.isEnabled)
assertFalse(keysBackup.isEnabled())

val megolmBackupCreationInfo = testHelper.doSync<MegolmBackupCreationInfo> {
keysBackup.prepareKeysBackupVersion(null, null, it)
}

assertFalse(keysBackup.isEnabled)
assertFalse(keysBackup.isEnabled())

// Create the version
val version = testHelper.doSync<KeysVersion> {
keysBackup.createKeysBackupVersion(megolmBackupCreationInfo, it)
}

// Backup must be enable now
assertTrue(keysBackup.isEnabled)
assertTrue(keysBackup.isEnabled())

// Check that it's signed with MSK
val versionResult = testHelper.doSync<KeysVersionResult?> {
@@ -441,8 +441,8 @@ class KeysBackupTest : InstrumentedTest {

// - The new device must see the previous backup as not trusted
assertNotNull(testData.aliceSession2.cryptoService().keysBackupService().keysBackupVersion)
assertFalse(testData.aliceSession2.cryptoService().keysBackupService().isEnabled)
assertEquals(KeysBackupState.NotTrusted, testData.aliceSession2.cryptoService().keysBackupService().state)
assertFalse(testData.aliceSession2.cryptoService().keysBackupService().isEnabled())
assertEquals(KeysBackupState.NotTrusted, testData.aliceSession2.cryptoService().keysBackupService().getState())

// - Trust the backup from the new device
testHelper.doSync<Unit> {
@@ -458,7 +458,7 @@ class KeysBackupTest : InstrumentedTest {

// - Backup must be enabled on the new device, on the same version
assertEquals(testData.prepareKeysBackupDataResult.version, testData.aliceSession2.cryptoService().keysBackupService().keysBackupVersion?.version)
assertTrue(testData.aliceSession2.cryptoService().keysBackupService().isEnabled)
assertTrue(testData.aliceSession2.cryptoService().keysBackupService().isEnabled())

// - Retrieve the last version from the server
val keysVersionResult = testHelper.doSync<KeysBackupLastVersionResult> {
@@ -504,8 +504,8 @@ class KeysBackupTest : InstrumentedTest {

// - The new device must see the previous backup as not trusted
assertNotNull(testData.aliceSession2.cryptoService().keysBackupService().keysBackupVersion)
assertFalse(testData.aliceSession2.cryptoService().keysBackupService().isEnabled)
assertEquals(KeysBackupState.NotTrusted, testData.aliceSession2.cryptoService().keysBackupService().state)
assertFalse(testData.aliceSession2.cryptoService().keysBackupService().isEnabled())
assertEquals(KeysBackupState.NotTrusted, testData.aliceSession2.cryptoService().keysBackupService().getState())

// - Trust the backup from the new device with the recovery key
testHelper.doSync<Unit> {
@@ -521,7 +521,7 @@ class KeysBackupTest : InstrumentedTest {

// - Backup must be enabled on the new device, on the same version
assertEquals(testData.prepareKeysBackupDataResult.version, testData.aliceSession2.cryptoService().keysBackupService().keysBackupVersion?.version)
assertTrue(testData.aliceSession2.cryptoService().keysBackupService().isEnabled)
assertTrue(testData.aliceSession2.cryptoService().keysBackupService().isEnabled())

// - Retrieve the last version from the server
val keysVersionResult = testHelper.doSync<KeysBackupLastVersionResult> {
@@ -565,8 +565,8 @@ class KeysBackupTest : InstrumentedTest {

// - The new device must see the previous backup as not trusted
assertNotNull(testData.aliceSession2.cryptoService().keysBackupService().keysBackupVersion)
assertFalse(testData.aliceSession2.cryptoService().keysBackupService().isEnabled)
assertEquals(KeysBackupState.NotTrusted, testData.aliceSession2.cryptoService().keysBackupService().state)
assertFalse(testData.aliceSession2.cryptoService().keysBackupService().isEnabled())
assertEquals(KeysBackupState.NotTrusted, testData.aliceSession2.cryptoService().keysBackupService().getState())

// - Try to trust the backup from the new device with a wrong recovery key
val latch = CountDownLatch(1)
@@ -579,8 +579,8 @@ class KeysBackupTest : InstrumentedTest {

// - The new device must still see the previous backup as not trusted
assertNotNull(testData.aliceSession2.cryptoService().keysBackupService().keysBackupVersion)
assertFalse(testData.aliceSession2.cryptoService().keysBackupService().isEnabled)
assertEquals(KeysBackupState.NotTrusted, testData.aliceSession2.cryptoService().keysBackupService().state)
assertFalse(testData.aliceSession2.cryptoService().keysBackupService().isEnabled())
assertEquals(KeysBackupState.NotTrusted, testData.aliceSession2.cryptoService().keysBackupService().getState())

stateObserver.stopAndCheckStates(null)
testData.cleanUp(testHelper)
@@ -612,8 +612,8 @@ class KeysBackupTest : InstrumentedTest {

// - The new device must see the previous backup as not trusted
assertNotNull(testData.aliceSession2.cryptoService().keysBackupService().keysBackupVersion)
assertFalse(testData.aliceSession2.cryptoService().keysBackupService().isEnabled)
assertEquals(KeysBackupState.NotTrusted, testData.aliceSession2.cryptoService().keysBackupService().state)
assertFalse(testData.aliceSession2.cryptoService().keysBackupService().isEnabled())
assertEquals(KeysBackupState.NotTrusted, testData.aliceSession2.cryptoService().keysBackupService().getState())

// - Trust the backup from the new device with the password
testHelper.doSync<Unit> {
@@ -629,7 +629,7 @@ class KeysBackupTest : InstrumentedTest {

// - Backup must be enabled on the new device, on the same version
assertEquals(testData.prepareKeysBackupDataResult.version, testData.aliceSession2.cryptoService().keysBackupService().keysBackupVersion?.version)
assertTrue(testData.aliceSession2.cryptoService().keysBackupService().isEnabled)
assertTrue(testData.aliceSession2.cryptoService().keysBackupService().isEnabled())

// - Retrieve the last version from the server
val keysVersionResult = testHelper.doSync<KeysBackupLastVersionResult> {
@@ -676,8 +676,8 @@ class KeysBackupTest : InstrumentedTest {

// - The new device must see the previous backup as not trusted
assertNotNull(testData.aliceSession2.cryptoService().keysBackupService().keysBackupVersion)
assertFalse(testData.aliceSession2.cryptoService().keysBackupService().isEnabled)
assertEquals(KeysBackupState.NotTrusted, testData.aliceSession2.cryptoService().keysBackupService().state)
assertFalse(testData.aliceSession2.cryptoService().keysBackupService().isEnabled())
assertEquals(KeysBackupState.NotTrusted, testData.aliceSession2.cryptoService().keysBackupService().getState())

// - Try to trust the backup from the new device with a wrong password
val latch = CountDownLatch(1)
@@ -690,8 +690,8 @@ class KeysBackupTest : InstrumentedTest {

// - The new device must still see the previous backup as not trusted
assertNotNull(testData.aliceSession2.cryptoService().keysBackupService().keysBackupVersion)
assertFalse(testData.aliceSession2.cryptoService().keysBackupService().isEnabled)
assertEquals(KeysBackupState.NotTrusted, testData.aliceSession2.cryptoService().keysBackupService().state)
assertFalse(testData.aliceSession2.cryptoService().keysBackupService().isEnabled())
assertEquals(KeysBackupState.NotTrusted, testData.aliceSession2.cryptoService().keysBackupService().getState())

stateObserver.stopAndCheckStates(null)
testData.cleanUp(testHelper)
@@ -969,7 +969,7 @@ class KeysBackupTest : InstrumentedTest {

val stateObserver = StateObserver(keysBackup)

assertFalse(keysBackup.isEnabled)
assertFalse(keysBackup.isEnabled())

// Wait for keys backup to be finished
val latch0 = CountDownLatch(1)
@@ -993,7 +993,7 @@ class KeysBackupTest : InstrumentedTest {
// - Make alice back up her keys to her homeserver
keysBackupTestHelper.prepareAndCreateKeysBackupData(keysBackup)

assertTrue(keysBackup.isEnabled)
assertTrue(keysBackup.isEnabled())

testHelper.await(latch0)

@@ -1012,8 +1012,8 @@ class KeysBackupTest : InstrumentedTest {
testHelper.await(latch2)

// -> That must fail and her backup state must be WrongBackUpVersion
assertEquals(KeysBackupState.WrongBackUpVersion, keysBackup.state)
assertFalse(keysBackup.isEnabled)
assertEquals(KeysBackupState.WrongBackUpVersion, keysBackup.getState())
assertFalse(keysBackup.isEnabled())

stateObserver.stopAndCheckStates(null)
cryptoTestData.cleanUp(testHelper)
@@ -1069,7 +1069,7 @@ class KeysBackupTest : InstrumentedTest {
// - Try to backup all in aliceSession2, it must fail
val keysBackup2 = aliceSession2.cryptoService().keysBackupService()

assertFalse("Backup should not be enabled", keysBackup2.isEnabled)
assertFalse("Backup should not be enabled", keysBackup2.isEnabled())

val stateObserver2 = StateObserver(keysBackup2)

@@ -1088,8 +1088,8 @@ class KeysBackupTest : InstrumentedTest {
assertFalse(isSuccessful)

// Backup state must be NotTrusted
assertEquals("Backup state must be NotTrusted", KeysBackupState.NotTrusted, keysBackup2.state)
assertFalse("Backup should not be enabled", keysBackup2.isEnabled)
assertEquals("Backup state must be NotTrusted", KeysBackupState.NotTrusted, keysBackup2.getState())
assertFalse("Backup should not be enabled", keysBackup2.isEnabled())

// - Validate the old device from the new one
aliceSession2.cryptoService().setDeviceVerification(
@@ -1103,7 +1103,7 @@ class KeysBackupTest : InstrumentedTest {
keysBackup2.addListener(object : KeysBackupStateListener {
override fun onStateChange(newState: KeysBackupState) {
// Check the backup completes
if (keysBackup2.state == KeysBackupState.ReadyToBackUp) {
if (keysBackup2.getState() == KeysBackupState.ReadyToBackUp) {
// Remove itself from the list of listeners
keysBackup2.removeListener(this)

@@ -1121,7 +1121,7 @@ class KeysBackupTest : InstrumentedTest {
}

// -> It must success
assertTrue(aliceSession2.cryptoService().keysBackupService().isEnabled)
assertTrue(aliceSession2.cryptoService().keysBackupService().isEnabled())

stateObserver.stopAndCheckStates(null)
stateObserver2.stopAndCheckStates(null)
@@ -1146,17 +1146,17 @@ class KeysBackupTest : InstrumentedTest {

val stateObserver = StateObserver(keysBackup)

assertFalse(keysBackup.isEnabled)
assertFalse(keysBackup.isEnabled())

val keyBackupCreationInfo = keysBackupTestHelper.prepareAndCreateKeysBackupData(keysBackup)

assertTrue(keysBackup.isEnabled)
assertTrue(keysBackup.isEnabled())

// Delete the backup
testHelper.doSync<Unit> { keysBackup.deleteBackup(keyBackupCreationInfo.version, it) }

// Backup is now disabled
assertFalse(keysBackup.isEnabled)
assertFalse(keysBackup.isEnabled())

stateObserver.stopAndCheckStates(null)
cryptoTestData.cleanUp(testHelper)
Original file line number Diff line number Diff line change
@@ -106,7 +106,7 @@ internal class KeysBackupTestHelper(

Assert.assertNotNull(megolmBackupCreationInfo)

Assert.assertFalse("Key backup should not be enabled before creation", keysBackup.isEnabled)
Assert.assertFalse("Key backup should not be enabled before creation", keysBackup.isEnabled())

// Create the version
val keysVersion = testHelper.doSync<KeysVersion> {
@@ -116,7 +116,7 @@ internal class KeysBackupTestHelper(
Assert.assertNotNull("Key backup version should not be null", keysVersion.version)

// Backup must be enable now
Assert.assertTrue(keysBackup.isEnabled)
Assert.assertTrue(keysBackup.isEnabled())

stateObserver.stopAndCheckStates(null)
return PrepareKeysBackupDataResult(megolmBackupCreationInfo, keysVersion.version)
@@ -128,7 +128,7 @@ internal class KeysBackupTestHelper(
*/
fun waitForKeysBackupToBeInState(session: Session, state: KeysBackupState) {
// If already in the wanted state, return
if (session.cryptoService().keysBackupService().state == state) {
if (session.cryptoService().keysBackupService().getState() == state) {
return
}

Original file line number Diff line number Diff line change
@@ -204,10 +204,13 @@ interface KeysBackupService {
callback: MatrixCallback<ImportRoomKeysResult>)

val keysBackupVersion: KeysVersionResult?

val currentBackupVersion: String?
val isEnabled: Boolean
val isStucked: Boolean
val state: KeysBackupState
get() = keysBackupVersion?.version

fun isEnabled(): Boolean
fun isStucked(): Boolean
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it should be isStuck() instead. The version with "ed" seems to not exist.

Copy link
Contributor

Choose a reason for hiding this comment

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

When renaming a method which is exposed as an API, should we add an entry in the changelog?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes, will add.

Copy link
Member Author

Choose a reason for hiding this comment

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

fun getState(): KeysBackupState

// For gossiping
fun saveBackupRecoveryKey(recoveryKey: String?, version: String?)
Original file line number Diff line number Diff line change
@@ -137,17 +137,11 @@ internal class DefaultKeysBackupService @Inject constructor(

private var keysBackupStateListener: KeysBackupStateListener? = null

override val isEnabled: Boolean
get() = keysBackupStateManager.isEnabled
override fun isEnabled(): Boolean = keysBackupStateManager.isEnabled

override val isStucked: Boolean
get() = keysBackupStateManager.isStucked
override fun isStucked(): Boolean = keysBackupStateManager.isStucked

override val state: KeysBackupState
get() = keysBackupStateManager.state

override val currentBackupVersion: String?
get() = keysBackupVersion?.version
override fun getState(): KeysBackupState = keysBackupStateManager.state

override fun addListener(listener: KeysBackupStateListener) {
keysBackupStateManager.addListener(listener)
@@ -291,7 +285,7 @@ internal class DefaultKeysBackupService @Inject constructor(
this.callback = object : MatrixCallback<Unit> {
private fun eventuallyRestartBackup() {
// Do not stay in KeysBackupState.Unknown but check what is available on the homeserver
if (state == KeysBackupState.Unknown) {
if (getState() == KeysBackupState.Unknown) {
checkAndStartKeysBackup()
}
}
@@ -383,7 +377,7 @@ internal class DefaultKeysBackupService @Inject constructor(
}

// If backup is finished, notify the main listener
if (state === KeysBackupState.ReadyToBackUp) {
if (getState() === KeysBackupState.ReadyToBackUp) {
backupAllGroupSessionsCallback?.onSuccess(Unit)
resetBackupAllGroupSessionsListeners()
}
@@ -915,12 +909,12 @@ internal class DefaultKeysBackupService @Inject constructor(
*/
fun maybeBackupKeys() {
when {
isStucked -> {
isStucked() -> {
// If not already done, or in error case, check for a valid backup version on the homeserver.
// If there is one, maybeBackupKeys will be called again.
checkAndStartKeysBackup()
}
state == KeysBackupState.ReadyToBackUp -> {
getState() == KeysBackupState.ReadyToBackUp -> {
keysBackupStateManager.state = KeysBackupState.WillBackUp

// Wait between 0 and 10 seconds, to avoid backup requests from
@@ -933,8 +927,8 @@ internal class DefaultKeysBackupService @Inject constructor(
uiHandler.post { backupKeys() }
}
}
else -> {
Timber.v("maybeBackupKeys: Skip it because state: $state")
else -> {
Timber.v("maybeBackupKeys: Skip it because state: ${getState()}")
}
}
}
@@ -1018,9 +1012,9 @@ internal class DefaultKeysBackupService @Inject constructor(
}

override fun checkAndStartKeysBackup() {
if (!isStucked) {
if (!isStucked()) {
// Try to start or restart the backup only if it is in unknown or bad state
Timber.w("checkAndStartKeysBackup: invalid state: $state")
Timber.w("checkAndStartKeysBackup: invalid state: ${getState()}")

return
}
@@ -1259,16 +1253,16 @@ internal class DefaultKeysBackupService @Inject constructor(
Timber.v("backupKeys")

// Sanity check, as this method can be called after a delay, the state may have change during the delay
if (!isEnabled || backupOlmPkEncryption == null || keysBackupVersion == null) {
if (!isEnabled() || backupOlmPkEncryption == null || keysBackupVersion == null) {
Timber.v("backupKeys: Invalid configuration")
backupAllGroupSessionsCallback?.onFailure(IllegalStateException("Invalid configuration"))
resetBackupAllGroupSessionsListeners()
return
}

if (state === KeysBackupState.BackingUp) {
if (getState() === KeysBackupState.BackingUp) {
// Do nothing if we are already backing up
Timber.v("backupKeys: Invalid state: $state")
Timber.v("backupKeys: Invalid state: ${getState()}")
return
}

Original file line number Diff line number Diff line change
@@ -165,7 +165,7 @@ class SecurityBootstrapTest : VerificationTestBase() {
assert(uiSession.cryptoService().crossSigningService().isCrossSigningInitialized())
assert(uiSession.cryptoService().crossSigningService().canCrossSign())
assert(uiSession.cryptoService().crossSigningService().allPrivateKeysKnown())
assert(uiSession.cryptoService().keysBackupService().isEnabled)
assert(uiSession.cryptoService().keysBackupService().isEnabled())
assert(uiSession.cryptoService().keysBackupService().currentBackupVersion != null)
assert(uiSession.sharedSecretStorageService().isRecoverySetup())
assert(uiSession.sharedSecretStorageService().isMegolmKeyInBackup())
Original file line number Diff line number Diff line change
@@ -66,7 +66,7 @@ fun Session.startSyncing(context: Context) {
*/
fun Session.hasUnsavedKeys(): Boolean {
return cryptoService().inboundGroupSessionsCount(false) > 0 &&
cryptoService().keysBackupService().state != KeysBackupState.ReadyToBackUp
cryptoService().keysBackupService().getState() != KeysBackupState.ReadyToBackUp
}

fun Session.cannotLogoutSafely(): Boolean {
Original file line number Diff line number Diff line change
@@ -60,7 +60,7 @@ class KeysBackupSettingsViewModel @AssistedInject constructor(@Assisted initialS
init {
setState {
this.copy(
keysBackupState = keysBackupService.state,
keysBackupState = keysBackupService.getState(),
keysBackupVersion = keysBackupService.keysBackupVersion
)
}
@@ -206,7 +206,7 @@ class KeysBackupSettingsViewModel @AssistedInject constructor(@Assisted initialS
}

fun canExit(): Boolean {
val currentBackupState = keysBackupService.state
val currentBackupState = keysBackupService.getState()

return currentBackupState == KeysBackupState.Unknown ||
currentBackupState == KeysBackupState.CheckingBackUpOnHomeserver
Original file line number Diff line number Diff line change
@@ -401,8 +401,8 @@ class MessageActionsViewModel @AssistedInject constructor(
if (vectorPreferences.developerMode()) {
if (timelineEvent.isEncrypted() && timelineEvent.root.mCryptoError != null) {
val keysBackupService = session.cryptoService().keysBackupService()
if (keysBackupService.state == KeysBackupState.NotTrusted ||
(keysBackupService.state == KeysBackupState.ReadyToBackUp &&
if (keysBackupService.getState() == KeysBackupState.NotTrusted ||
(keysBackupService.getState() == KeysBackupState.ReadyToBackUp &&
keysBackupService.canRestoreKeys())
) {
add(EventSharedAction.UseKeyBackup)
Original file line number Diff line number Diff line change
@@ -81,7 +81,7 @@ class ServerBackupStatusViewModel @AssistedInject constructor(@Assisted initialS

init {
session.cryptoService().keysBackupService().addListener(this)
keysBackupState.value = session.cryptoService().keysBackupService().state
keysBackupState.value = session.cryptoService().keysBackupService().getState()
val liveUserAccountData = session.flow().liveUserAccountData(setOf(MASTER_KEY_SSSS_NAME, USER_SIGNING_KEY_SSSS_NAME, SELF_SIGNING_KEY_SSSS_NAME))
val liveCrossSigningInfo = session.flow().liveCrossSigningInfo(session.myUserId)
val liveCrossSigningPrivateKeys = session.flow().liveCrossSigningPrivateKeys()
@@ -116,7 +116,7 @@ class ServerBackupStatusViewModel @AssistedInject constructor(@Assisted initialS
}

viewModelScope.launch {
keyBackupFlow.tryEmit(session.cryptoService().keysBackupService().state)
keyBackupFlow.tryEmit(session.cryptoService().keysBackupService().getState())
}
}

@@ -148,7 +148,7 @@ class ServerBackupStatusViewModel @AssistedInject constructor(@Assisted initialS

override fun onStateChange(newState: KeysBackupState) {
viewModelScope.launch {
keyBackupFlow.tryEmit(session.cryptoService().keysBackupService().state)
keyBackupFlow.tryEmit(session.cryptoService().keysBackupService().getState())
}
keysBackupState.value = newState
}
Original file line number Diff line number Diff line change
@@ -76,7 +76,7 @@ class SignoutCheckViewModel @AssistedInject constructor(

val quad4SIsSetup = session.sharedSecretStorageService().isRecoverySetup()
val allKeysKnown = session.cryptoService().crossSigningService().allPrivateKeysKnown()
val backupState = session.cryptoService().keysBackupService().state
val backupState = session.cryptoService().keysBackupService().getState()
setState {
copy(
userId = session.myUserId,