-
Notifications
You must be signed in to change notification settings - Fork 46
Fixing error in construction of LegacyFido2ApiManager #2654
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
Conversation
|
❌ Work item link check failed. Description does not contain AB#{ID}. Click here to Learn more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR prevents casting exceptions when creating LegacyFido2ApiManager by adding explicit runtime checks for the activity and fragment types, and updates credential logic to respect the presence of a legacy manager.
- Guard
LegacyFido2ApiManagerconstruction withinstanceofchecks on bothAuthorizationActivityandWebViewAuthorizationFragment - Change
preferImmediatelyAvailableCredentialsto only be true if a legacy manager exists - Record the bug fix in
changelog.txt
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| common/src/main/java/com/microsoft/identity/common/internal/ui/webview/AzureActiveDirectoryWebViewClient.java | Introduce currentActivity var and additional instanceof checks before calling LegacyFido2ApiManager |
| common/src/main/java/com/microsoft/identity/common/internal/fido/CredManFidoManager.kt | Update preferImmediatelyAvailableCredentials to include legacyManager != null |
| changelog.txt | Add a [PATCH] Fixing error in construction of LegacyFido2ApiManager entry |
Comments suppressed due to low confidence (3)
common/src/main/java/com/microsoft/identity/common/internal/ui/webview/AzureActiveDirectoryWebViewClient.java:233
- No unit test currently verifies the fallback path when the fragment is not a WebViewAuthorizationFragment. Adding a test for that branch would ensure we don’t hit a casting exception in other activity flows.
currentActivity instanceof AuthorizationActivity && ((AuthorizationActivity) currentActivity).getFragment() instanceof WebViewAuthorizationFragment
common/src/main/java/com/microsoft/identity/common/internal/ui/webview/AzureActiveDirectoryWebViewClient.java:233
- [nitpick] Consider extracting the fragment into a local variable (e.g.,
Fragment fm = ((AuthorizationActivity) currentActivity).getFragment()) to avoid repeated casts and improve readability.
currentActivity instanceof AuthorizationActivity && ((AuthorizationActivity) currentActivity).getFragment() instanceof WebViewAuthorizationFragment
common/src/main/java/com/microsoft/identity/common/internal/fido/CredManFidoManager.kt:87
- The variable 'legacyManager' is not defined in this scope, which will cause a compilation error. Consider passing 'legacyManager' into the CredManFidoManager constructor or otherwise making its presence available here.
preferImmediatelyAvailableCredentials = (legacyManager != null && getFlightsProvider().isFlightEnabled(CommonFlight.ENABLE_LEGACY_FIDO_SECURITY_KEY_LOGIC) && Build.VERSION.SDK_INT < Build.VERSION_CODES.UPSIDE_DOWN_CAKE)
Summary
While testing for their passkey enablement rollout, the Office team found a bug where if they're using OneAuth with fragment or dialog mode (and thus using their own Activity instead of AuthorizationActivity), they run into a casting exception in the process of creating a LegacyFido2ApiManager, as we need to pass a WebViewAuthorizationFragment into the constructor.
The devices which would run into this error would be:
The fix for this is to add explicit type checks before creating the LegacyFido2ApiManager. Because the legacy FIDO2 API does require a Fragment which has a particular object instantiated (which I had added into our WebViewAuthorizationFragment), we can't just pass any fragment from any activity. This means that for Office's case, for Android 13 and below devices, they are going to only use CredMan- which is actually a good thing, since CredMan now has support for security keys for all OS versions (we originally included the legacy API because this wasn't the case a while ago).
At some point, it would be best to just remove the legacy API logic, but that can be done once a larger majority of users are on Android 14+.
Noting again that brokered auth scenarios should remain unaffected because the legacy API path was never hit (the flight has remained off, and default value is false). The same should be the case for Android 14+.