Fix lockscreen's 'device locked' crash on Android 12 and 12L devices #6784
Conversation
| // Exception found: | ||
| // 1. Stop collecting. | ||
| // 2. Remove the system key if we were creating it. | ||
| // 3. Throw the exception and remove the prompt reference | ||
| if (!checkSystemKeyExists) { | ||
| lockScreenKeyRepository.deleteSystemKey() | ||
| } | ||
| prompt = null | ||
| throw exception | ||
| } | ||
| } | ||
| } | ||
| // Generates the system key on successful authentication | ||
| if (buildVersionSdkIntProvider.get() >= Build.VERSION_CODES.M) { | ||
| lockScreenKeyRepository.ensureSystemKey() | ||
| } |
There was a problem hiding this comment.
Since we started using CryptoObject, the system key is created when that CryptoObject is instantiated. Instead of ensuring the system key is created on success, make sure it's removed on error if we were on the process of enabling biometric auth.
| /** | ||
| * Wait until the device is unlocked. There seems to be a behavior change on Android 12 that makes [KeyguardManager.isDeviceLocked] return `false` even | ||
| * after an Activity's `onResume` method. If we mix that with the system keys needing the device to be unlocked before they're used, we get crashes. | ||
| * See issue [#6768](https://github.com/vector-im/element-android/issues/6768). | ||
| */ | ||
| @SuppressLint("NewApi") | ||
| @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) | ||
| internal suspend fun waitUntilKeyguardIsUnlocked() { | ||
| if (versionProvider.get() < Build.VERSION_CODES.S) return | ||
| withTimeoutOrNull(5.seconds) { | ||
| while (keyguardManager.isDeviceLocked) { | ||
| delay(50.milliseconds) | ||
| } | ||
| } |
There was a problem hiding this comment.
This is the actual code that fixes the issue.
There was a problem hiding this comment.
do you think it's worth extracting this to a dedicated class/usecase? would also mean we could avoid the VisibleForTesting
not a blocker for me
There was a problem hiding this comment.
Actually, the @VisibleForTesting annotation is no longer needed, I forgot to revert it to private.
I also thought about making it an external component, but it seemed too specific since it's only used here and it's a terrible hack that shouldn't be used anywhere if there's an alternative.
There was a problem hiding this comment.
I don't have tooo strong of an opinion here, up to you!
I would lean towards extracting as ViewModels tend to collect logic, requiring more setup for unit tests
jmartinesp
force-pushed
the
fix/jorgem/lockscreen-device-locked
branch
from
f801ace to
a533a8c
Compare
ouchadam
added
the
Z-NextRelease
|
if the PR is set to be based on #6780 we'll get a smaller diff (and it'll automatically update the base branch when the base is merged) |
| val mockAuthChannel = Channel<Boolean>(capacity = 1) | ||
| val biometricUtils = spyk(createBiometricHelper(createDefaultConfiguration(isBiometricsEnabled = true))) { | ||
| every { createAuthChannel() } returns mockAuthChannel | ||
| every { authenticateWithPromptInternal(any(), any(), any()) } returns mockk() | ||
| } | ||
| every { lockScreenKeyRepository.deleteSystemKey() } returns Unit |
There was a problem hiding this comment.
can also use justRun/coJustRun for these Unit cases
| if (state.canUseBiometricAuth && state.isBiometricKeyInvalidated) { | ||
| lockScreenListener?.onBiometricKeyInvalidated() | ||
| } else if (state.showBiometricPromptAutomatically) { | ||
| viewModel.stateFlow.distinctUntilChangedBy { it.showBiometricPromptAutomatically } |
There was a problem hiding this comment.
I'm wondering if we should expose this as a ViewEvent, the ViewModel.init would register to the state and emit the event in onEach rather than the Fragment handling the logic (would have the benefit of being testable at the ViewModel layer)
what do you think? (could be a separate PR if you agree)
There was a problem hiding this comment.
I think we don't need to observe the field, just checking it once on init, inside the launched coroutine should be enough: we only want to display the prompt automatically once when the user enters the lockscreen, so triggering the event several times depending on external changes might be weird behaviour.
There was a problem hiding this comment.
I see! in that case flow.first { it.showBiometricPromptAutomatically } might be better than distinct + filter
it's also worth highlighting init is probably a bad place to emit events as it requires the fragments to be able to start observing before the ViewModel has been created, otherwise it creates race conditions 😅
other ViewModels have dedicated actions that correlate to the lifecycle like Resumed/Visible
There was a problem hiding this comment.
Do you mean something like this?
init {
...
observeStateChanges()
}
private fun observeStateChanges() {
viewModelScope.launch {
if (stateFlow.firstOrNull { it.showBiometricPromptAutomatically } != null) {
_viewEvents.post(LockScreenViewEvent.ShowBiometricPromptAutomatically)
}
}
viewModelScope.launch {
if (stateFlow.firstOrNull { it.isBiometricKeyInvalidated } != null) {
onBiometricKeyInvalidated()
}
}
}There was a problem hiding this comment.
yep ^^^ but if it's possible for the view event to trigger before a fragment has a chance to start observing we'll need another ViewAction when onCreate or onResume happens to trigger the logic instead of init
There was a problem hiding this comment.
I added an OnUIReady action to be triggered when the fragment's onViewCreated is called, right after it starts observing events.
jmartinesp
force-pushed
the
fix/jorgem/lockscreen-device-locked
branch
from
a533a8c to
9888e15
Compare
|
SonarCloud Quality Gate failed. |
Type of change
Content
Fixes #6768.
Depends on #6769.
Motivation and context
On Android 12 and 12L (APIs 31 and 32) there seems to be a bug that causes
KeyguardManager.isDeviceLockedto be true for a few milliseconds even after the lock screen has been dismissed and the app is already displayed.Since we added
setRequiresDeviceUnlocked(true)to the biometric key, it can only be used after the device is unlocked andKeyStorewill throw an exception if we try to access the key while it's locked. In this case, it's causing crashes when trying to updateLockScreenViewModel's state based on the biometric keys info.I also tried to simplify the implementation of the lock screen feature by removing some redundant components (LockScreenConfigurationProvider).
Screenshots / GIFs
Tests
If the app didn't crash, the fix is working.
Tested devices
Checklist