Skip to content

Fido2: Fix instant use of screen lock #3237

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.

Sign up for GitHub

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

Open
p1gp1g wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Fido2: Fix instant use of screen lock #3237

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0c4aa6d
Avoid unnecessary work in UI thread
p1gp1g Jan 18, 2026
c3124d0
Remove unused attribute
p1gp1g Jan 18, 2026
9dc940b
Avoid redundant check for instant transport
p1gp1g Jan 18, 2026
1999c7f
Simplify instant transport check
p1gp1g Jan 18, 2026
c526a24
Do not force instant login with SCREEN_LOCK
p1gp1g Jan 18, 2026
b3e55e4
Allow choosing account during screen lock authentication
p1gp1g Jan 18, 2026
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
11 changes: 8 additions & 3 deletions ...c/main/kotlin/org/microg/gms/fido/core/transport/screenlock/ScreenLockTransportHandler.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,13 +259,18 @@ class ScreenLockTransportHandler(private val activity: FragmentActivity, callbac

override fun shouldBeUsedInstantly(options: RequestOptions): Boolean {
if (options.type != RequestOptionsType.SIGN) return false
for (descriptor in options.signOptions.allowList.orEmpty()) {
// If there is an allow list, it means we try to login for a defined user.
// So if we have one of the allowed keys to login with the screen lock, we use it.
//
// We DON'T want to use instant connection if the allowList is empty, as the user
// may want to login with different user accounts, maybe on a hardware key
options.signOptions.allowList?.forEach {
try {
val (type, data) = CredentialId.decodeTypeAndData(descriptor.id)
val (type, data) = CredentialId.decodeTypeAndData(it.id)
if (type == 1.toByte() && store.containsKey(options.rpId, data)) {
return true
}
} catch (e: Exception) {
} catch (_: Exception) {
Copy link
Copy Markdown
Contributor Author

@p1gp1g p1gp1g Jan 18, 2026

Choose a reason for hiding this comment

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

I've mainly added a comment here

// Ignore
}
}
Expand Down
38 changes: 11 additions & 27 deletions play-services-fido/core/src/main/kotlin/org/microg/gms/fido/core/ui/AuthenticatorActivity.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@
package org.microg.gms.fido.core.ui

import android.content.Intent
import android.graphics.Color
import android.graphics.drawable.ColorDrawable
import android.os.Build.VERSION.SDK_INT
import android.os.Bundle
import android.util.Base64
Expand All @@ -24,7 +22,6 @@ import com.google.android.gms.fido.fido2.api.common.AuthenticationExtensionsPrfO
import com.google.android.gms.fido.fido2.api.common.ErrorCode.*
import kotlinx.coroutines.CancellationException
import kotlinx.coroutines.Job
import org.microg.gms.common.GmsService
import org.microg.gms.fido.core.*
import org.microg.gms.fido.core.transport.Transport
import org.microg.gms.fido.core.transport.Transport.*
Expand All @@ -37,6 +34,7 @@ import org.microg.gms.fido.core.transport.usb.UsbTransportHandler
import org.microg.gms.utils.getApplicationLabel
import org.microg.gms.utils.getFirstSignatureDigest
import org.microg.gms.utils.toBase64
import androidx.core.graphics.drawable.toDrawable

const val TAG = "FidoUi"

Expand All @@ -54,8 +52,6 @@ class AuthenticatorActivity : AppCompatActivity(), TransportHandlerCallback {
else -> null
}

private val service: GmsService
Copy link
Copy Markdown
Contributor Author

@p1gp1g p1gp1g Jan 18, 2026

Choose a reason for hiding this comment

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

This wasn't used

get() = GmsService.byServiceId(intent.getIntExtra(KEY_SERVICE, GmsService.UNKNOWN.SERVICE_ID))
private val database by lazy { Database(this) }
private val transportHandlers by lazy {
setOfNotNull(
Expand Down Expand Up @@ -95,19 +91,6 @@ class AuthenticatorActivity : AppCompatActivity(), TransportHandlerCallback {

Log.d(TAG, "onCreate caller=$callerPackage options=$options")

val requiresPrivilege =
options is BrowserRequestOptions && !database.isPrivileged(callerPackage, callerSignature)

// Check if we can directly open screen lock handling
if (!requiresPrivilege) {
val instantTransport = transportHandlers.firstOrNull { it.isSupported && it.shouldBeUsedInstantly(options) }
if (instantTransport != null && instantTransport.transport in INSTANT_SUPPORTED_TRANSPORTS) {
window.setBackgroundDrawable(ColorDrawable(0))
window.statusBarColor = Color.TRANSPARENT
setTheme(org.microg.gms.base.core.R.style.Theme_Translucent)
}
}

Copy link
Copy Markdown
Contributor Author

@p1gp1g p1gp1g Jan 18, 2026

Choose a reason for hiding this comment

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

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

setTheme(androidx.appcompat.R.style.Theme_AppCompat_DayNight_NoActionBar)
setContentView(R.layout.fido_authenticator_activity)

Expand Down Expand Up @@ -136,10 +119,15 @@ class AuthenticatorActivity : AppCompatActivity(), TransportHandlerCallback {
Log.d(TAG, "origin=$origin, appName=$appName")

// Check if we can directly open screen lock handling
// If the request contains an allow list, the request is for a dedicated account.
// If we have one of the allowed keys we instant login
// 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) {
Copy link
Copy Markdown
Contributor Author

@p1gp1g p1gp1g Jan 18, 2026

Choose a reason for hiding this comment

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

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)

startTransportHandling(instantTransport.transport, true)
transportHandlers.firstOrNull {
it.isSupported && it.shouldBeUsedInstantly(options)
}?.let {
window.setBackgroundDrawable(0.toDrawable())
startTransportHandling(it.transport, true)
return
}
}
Expand All @@ -154,7 +142,6 @@ class AuthenticatorActivity : AppCompatActivity(), TransportHandlerCallback {
val next = if (!requiresPrivilege) {
val knownRegistrationTransports = mutableSetOf<Transport>()
val allowedTransports = mutableSetOf<Transport>()
val localSavedUserKey = mutableSetOf<String>()
if (options.type == RequestOptionsType.SIGN) {
for (descriptor in options.signOptions.allowList.orEmpty()) {
val knownTransport = database.getKnownRegistrationTransport(options.rpId, descriptor.id.toBase64(Base64.URL_SAFE, Base64.NO_WRAP, Base64.NO_PADDING))
Expand All @@ -177,13 +164,11 @@ class AuthenticatorActivity : AppCompatActivity(), TransportHandlerCallback {
}
}
}
database.getKnownRegistrationInfo(options.rpId).forEach { localSavedUserKey.add(it.userJson) }
Copy link
Copy Markdown
Contributor Author

@p1gp1g p1gp1g Jan 18, 2026

Choose a reason for hiding this comment

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

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
Copy link
Copy Markdown
Contributor Author

@p1gp1g p1gp1g Jan 18, 2026

Choose a reason for hiding this comment

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

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 when (preselectedTransport) {
when (preselectedTransport) {
SCREEN_LOCK -> R.id.signInSelectionFragment
USB -> R.id.usbFragment
NFC -> R.id.nfcFragment
else -> R.id.transportSelectionFragment
Expand Down Expand Up @@ -358,7 +343,6 @@ class AuthenticatorActivity : AppCompatActivity(), TransportHandlerCallback {
const val KEY_CALLER = "caller"

val IMPLEMENTED_TRANSPORTS = setOf(USB, SCREEN_LOCK, NFC)
val INSTANT_SUPPORTED_TRANSPORTS = setOf(SCREEN_LOCK)
}
}

Expand Down
13 changes: 11 additions & 2 deletions ...vices-fido/core/src/main/kotlin/org/microg/gms/fido/core/ui/TransportSelectionFragment.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,10 @@ import android.view.LayoutInflater
import android.view.View
import android.view.ViewGroup
import androidx.navigation.fragment.findNavController
import com.google.android.gms.fido.fido2.api.common.BrowserPublicKeyCredentialRequestOptions
import com.google.android.gms.fido.fido2.api.common.PublicKeyCredentialRequestOptions
import org.microg.gms.fido.core.R
import org.microg.gms.fido.core.databinding.FidoTransportSelectionFragmentBinding
import org.microg.gms.fido.core.transport.Transport
import org.microg.gms.fido.core.transport.Transport.SCREEN_LOCK

class TransportSelectionFragment : AuthenticatorActivityFragment() {
Expand All @@ -31,7 +32,15 @@ class TransportSelectionFragment : AuthenticatorActivityFragment() {
findNavController().navigate(R.id.openUsbFragment, arguments.withIsFirst(false))
}
binding.setOnScreenLockClick {
startTransportHandling(SCREEN_LOCK)
if (options is PublicKeyCredentialRequestOptions ||
options is BrowserPublicKeyCredentialRequestOptions) {
findNavController().navigate(
R.id.signInSelectionFragment,
arguments.withIsFirst(false)
)
} else {
startTransportHandling(SCREEN_LOCK)
}
Copy link
Copy Markdown
Contributor Author

@p1gp1g p1gp1g Jan 18, 2026

Choose a reason for hiding this comment

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

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"

}
return binding.root
}
Expand Down