Fido2: Fix instant use of screen lock #3237
Conversation
We avoid calling to the db onCreate. This change has the same UI, as setTheme was always appCompat_DayNigh_NoActionBar called after the check for instant login
We alread have Transport.shouldBeUsedInstantly to define a transport allowed to be instant. And, only screen lock may be allowed ATM
Screen lock was effectively force as instant as soon as a key with the rpId was registered with screen lock. As so, even if the allowList is empty (this isn't an authentication for a given account, and the user may want to login with a hardware key) or if the allowList contains a key that isn't for a screen-lock (e.g. USB)!
| return true | ||
| } | ||
| } catch (e: Exception) { | ||
| } catch (_: Exception) { |
There was a problem hiding this comment.
I've mainly added a comment here
| else -> null | ||
| } | ||
|
|
||
| private val service: GmsService |
| setTheme(org.microg.gms.base.core.R.style.Theme_Translucent) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This part was doing work on the UI thread, including DB requests, and redundant: setTheme is called right after the condition to set AppCompat_DayNight_NoAction_Bar, the statusBarColor seems to not have any effect, and setting the background color can be done in the suspended handleRequest
| // Else, the user must be able to choose | ||
| if (!requiresPrivilege && allowInstant) { | ||
| val instantTransport = transportHandlers.firstOrNull { it.isSupported && it.shouldBeUsedInstantly(options) } | ||
| if (instantTransport != null && instantTransport.transport in INSTANT_SUPPORTED_TRANSPORTS) { |
There was a problem hiding this comment.
INSTANT_SUPPORTED_TRANSPORTS was redundant: this is already what shouldBeUsedInstantly does. (And only one transport never return false: the screen lock, but that isn't relevant)
| } | ||
| } | ||
| } | ||
| database.getKnownRegistrationInfo(options.rpId).forEach { localSavedUserKey.add(it.userJson) } |
There was a problem hiding this comment.
Here was the first part of bug: "Store any key for that website in localSavedUserKey"
| val preselectedTransport = knownRegistrationTransports.singleOrNull() ?: allowedTransports.singleOrNull() | ||
| if (database.wasUsed()) { | ||
| if (localSavedUserKey.isNotEmpty()) { | ||
| R.id.signInSelectionFragment |
There was a problem hiding this comment.
And here the 2nd part "If there is any key for that website in localSavedUserKey, go to signInSelection" => we can't use a USB or NFC key, even if it is the only allowed key
| ) | ||
| } else { | ||
| startTransportHandling(SCREEN_LOCK) | ||
| } |
There was a problem hiding this comment.
This is the part where we want to be able to choose which account to use during sign-in, but we don't want that view during registration
The previous selection of user account was with the other bug, where it was "there is any key for a website => we go to the account selection for screen lock login".
The current behavior is now:
- "we have an allowCredential => use it"
- "we don't have an allowCredential, go to transport selection, select screen_lock, select user account"
- "we don't have an allowCredential, go to the last transport for this account login or go back to transport selection, select screen_lock, select user account"
1 participant
I had 2 problems with the current implementation:
This is because of
localSavedUserKeythat makes the authentication use instantly the screen lock if we have a key saved for the domain.This PR fixes these 2 issues. And does a little cleanup, and a few comments to help understand what does what
MEMO: I use fido-core as a library.