-
Notifications
You must be signed in to change notification settings - Fork 46
Add OpenTelemetry support for passkey operations, Fixes AB#3412004 #2795
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
- Set project-level archives name in build.gradle - Improve PasskeyWebListener to set up WebView message listener - Refactor WebViewAuthorizationFragment to handle console messages with logging levels - Update request headers management in WebViewAuthorizationFragment for passkey protocol - Clean up WebViewMessageListener by removing unused default listener
…oved script handling and logging; add JsScriptRecord for script management; update CommonFlight to disable passkey feature by default.
…rieval logging; streamline credential request handling.
- Added PasskeyReplyChannel for communication between JavaScript and native code. - Updated CredentialManagerHandler to create and retrieve passkeys. - Enhanced PasskeyWebListener to handle WebAuthn requests and responses. - Introduced js-bridge.js for JavaScript integration with WebAuthn. - Created unit tests for PasskeyReplyChannel to ensure correct message formatting and error handling. - Removed unnecessary logging statements and improved error handling.
…r handling and message formatting; enhance test coverage for success and error scenarios.
…te Logger class to disable Logcat logging by default; improve logging conditions in Logger.java; ensure proper newline at end of files in CredentialManagerHandler and JsScriptRecord.
…d clean up request headers
…nstructions for modifying js-bridge.js
…ract Passkey protocol header injection logic into a separate method for improved readability and maintainability.
… with project property for better version management. Refactor FidoChallengeField to support multiple Passkey protocol versions; improve error handling for unsupported versions. Clean up CredentialManagerHandler by removing unnecessary exception handling; streamline credential creation and retrieval logic. Add unit tests for PasskeyWebListener; cover message handling, credential flows, and error scenarios. Add webkitVersion variable in versions.gradle for centralized version management.
…abled by default.
…ent; streamline logic for injecting headers based on flight feature and broker requests.
…dler for improved readability; format code for better clarity.
…ureAD/microsoft-authentication-library-common-for-android into pedroro/passkey-reg-prototype
- Introduced a set of supported passkey protocol versions in FidoConstants. - Updated throwIfInvalidProtocolVersion method to validate against the new set.
…Channel, PasskeyWebListener, and WebViewAuthorizationFragment
…ving error handling, and updating WebView authorization logic
…cking and span management in PasskeyReplyChannel, PasskeyWebListener, and AzureActiveDirectoryWebViewClient
…fido' in the path for specific cases
… handling and telemetry integration
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 adds telemetry instrumentation to the Passkey/WebAuthn authentication flow and improves code organization by caching span context in the WebView client.
Key changes:
- Added telemetry support with new span name (
PasskeyWebListener) and attributes (passkey_operation_type,passkey_dom_exception_name) - Refactored
AzureActiveDirectoryWebViewClientto cacheSpanContextas a field, eliminating repeated conditional checks - Enhanced error handling in passkey flows with new
ClientExceptionerror codes (UNSUPPORTED_OPERATION,REQUEST_IN_PROGRESS)
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| common4j/src/main/com/microsoft/identity/common/java/opentelemetry/SpanName.java | Added PasskeyWebListener enum value for passkey operation telemetry spans |
| common4j/src/main/com/microsoft/identity/common/java/opentelemetry/AttributeName.java | Added passkey_operation_type and passkey_dom_exception_name attributes for passkey telemetry classification |
| common4j/src/main/com/microsoft/identity/common/java/exception/ClientException.java | Added UNSUPPORTED_OPERATION and REQUEST_IN_PROGRESS error code constants for passkey validation |
| common/src/main/java/com/microsoft/identity/common/internal/ui/webview/AzureActiveDirectoryWebViewClient.java | Cached SpanContext as field mSpanContext and added getter; refactored span creation methods to use cached context instead of repeated conditional checks |
| common/src/main/java/com/microsoft/identity/common/internal/providers/oauth2/PasskeyWebListener.kt | Added spanContext parameter and propagated it to PasskeyReplyChannel instances; replaced string error messages with structured ClientException instances |
| common/src/main/java/com/microsoft/identity/common/internal/providers/oauth2/PasskeyReplyChannel.kt | Added spanContext parameter; instrumented postSuccess() and postError() with telemetry spans, attributes, and status codes; removed separate send() method by inlining span logic |
| common/src/test/java/com/microsoft/identity/common/internal/providers/oauth2/PasskeyReplyChannelTest.kt | Updated test to handle postError() signature change (now accepts Throwable instead of String); added test coverage for Get* credential exception types; improved test naming for clarity |
Comments suppressed due to low confidence (1)
common/src/test/java/com/microsoft/identity/common/internal/providers/oauth2/PasskeyReplyChannelTest.kt:304
- Missing test coverage for telemetry span creation and attribute setting in
PasskeyReplyChannel. The PR adds telemetry instrumentation topostSuccess()andpostError()methods (creating spans, setting attributes, recording status), but the existing tests don't verify any of this telemetry behavior.
Issue: The tests at lines 58-304 verify message formatting and exception handling but don't check:
- Whether spans are created with the correct name (
SpanName.PasskeyWebListener) - Whether
passkey_operation_typeattribute is set correctly - Whether
passkey_dom_exception_nameattribute is set for errors - Whether span status is set to OK for success or ERROR for failures
- Whether spans are properly ended
Impact: Telemetry regressions could go undetected, impacting observability and debugging capabilities.
Recommendation: Add test cases that mock the telemetry infrastructure to verify:
- Span creation with parent context
- Attribute setting with correct values
- Status code setting (OK/ERROR)
- Exception recording
- Span ending in all code paths
Example test structure:
@Test
fun `postSuccess sets telemetry attributes correctly`() {
// Mock OTelUtility and Span
// Verify span created with PasskeyWebListener name
// Verify passkey_operation_type attribute set
// Verify StatusCode.OK set
// Verify span.end() called
} @Test
fun `send throws exception when postMessage fails`() {
// Given
val testJson = """{"key": "value"}"""
val expectedException = RuntimeException("PostMessage failed")
every { mockReplyProxy.postMessage(any<String>()) } throws expectedException
// When/Then - Should throw the exception
val thrownException = assertThrows(RuntimeException::class.java) {
passkeyReplyChannel.postSuccess(testJson)
}
assertEquals("PostMessage failed", thrownException.message)
verify { mockReplyProxy.postMessage(any<String>()) }
}
common4j/src/main/com/microsoft/identity/common/java/opentelemetry/AttributeName.java
Show resolved
Hide resolved
...src/main/java/com/microsoft/identity/common/internal/providers/oauth2/PasskeyReplyChannel.kt
Show resolved
Hide resolved
...ava/com/microsoft/identity/common/internal/ui/webview/AzureActiveDirectoryWebViewClient.java
Outdated
Show resolved
Hide resolved
...ava/com/microsoft/identity/common/internal/ui/webview/AzureActiveDirectoryWebViewClient.java
Outdated
Show resolved
Hide resolved
...src/main/java/com/microsoft/identity/common/internal/providers/oauth2/PasskeyReplyChannel.kt
Show resolved
Hide resolved
...test/java/com/microsoft/identity/common/internal/providers/oauth2/PasskeyReplyChannelTest.kt
Outdated
Show resolved
Hide resolved
...ava/com/microsoft/identity/common/internal/ui/webview/AzureActiveDirectoryWebViewClient.java
Outdated
Show resolved
Hide resolved
...src/main/java/com/microsoft/identity/common/internal/providers/oauth2/PasskeyReplyChannel.kt
Show resolved
Hide resolved
...src/main/java/com/microsoft/identity/common/internal/providers/oauth2/PasskeyReplyChannel.kt
Outdated
Show resolved
Hide resolved
...ava/com/microsoft/identity/common/internal/ui/webview/AzureActiveDirectoryWebViewClient.java
Outdated
Show resolved
Hide resolved
…/webview/AzureActiveDirectoryWebViewClient.java Co-authored-by: Copilot <[email protected]>
…/webview/AzureActiveDirectoryWebViewClient.java Co-authored-by: Copilot <[email protected]>
…oviders/oauth2/PasskeyWebListener.kt Co-authored-by: Copilot <[email protected]>
…dant tests from PasskeyReplyChannelTest
… better clarity; remove redundant test from PasskeyReplyChannelTest
|
❌ Invalid work item number: AB#3412004 Click here to learn more. |
|
✅ Work item link check complete. Description contains link AB#3412004 to an Azure Boards work item. |
…on unsupported request types
| /** | ||
| * Passkey operation type (e.g., registration, authentication). | ||
| */ | ||
| passkey_operation_type, |
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.
Just to make sure, we should not expect to see the type be authentication until support is extended on the ESTS side to allow authentication, right? auth will be represented by the telemetry set for the legacy fido classes?
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.
Yes, all operations should remain of type "registration" until ESTS decides to update their code to include authentication as well. At that point, a new type will be introduced. The idea of defining it now is simply to be prepared for when it’s needed. All authentication requests will continue to stay within the Fido span.
This pull request introduces OpenTelemetry tracing to the passkey (WebAuthN) flow, ensuring that all interactions with the JavaScript reply channel and WebView client are properly traced for observability. It also refactors error handling in the passkey reply channel to use exceptions rather than plain strings, and propagates the tracing context (
SpanContext) throughout the relevant classes. Additionally, the pull request cleans up and simplifies some internal logic and improves test coverage.AB#3412004