[Windows] WebAuthenticator: add OAuth support via app activation - #34887
[Windows] WebAuthenticator: add OAuth support via app activation#34887mattleibow wants to merge 20 commits into
Conversation
…s_oauth # Conflicts: # src/Essentials/src/WebAuthenticator/WebAuthenticator.windows.cs
Keep the community OAuth commits intact, and wire the Windows callback flow through the new app activation lifecycle event instead of special-casing startup. This preserves code/state handling, response decoding, and cancellation semantics while keeping MauiWinUIApplication generic. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
45bf1aa to
9dcdd3c
Compare
There was a problem hiding this comment.
Pull request overview
Ports Windows WebAuthenticator/OAuth handling to the new Windows app-activation lifecycle hook so protocol callbacks are completed inside Essentials without app-specific startup plumbing.
Changes:
- Implement Windows
WebAuthenticatorusing WinAppSDKOAuth2Managerand handle protocol activations viaPlatform.OnAppActivation. - Wire Essentials into the Windows lifecycle
OnAppActivation(...)hook so protocol callbacks can complete pending auth requests. - Update Essentials device test packaging (protocol registration) and adjust the sample server response to include
code/state.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Essentials/src/WebAuthenticator/WebAuthenticator.windows.cs | Adds Windows OAuth implementation and activation handling for completing auth requests. |
| src/Essentials/src/Platform/Platform.shared.cs | Exposes a Windows Platform.OnAppActivation(...) entrypoint for activation handling in Essentials. |
| src/Core/src/Hosting/EssentialsMauiAppBuilderExtensions.cs | Hooks the Windows lifecycle OnAppActivation event to the Essentials Platform handler. |
| src/Essentials/src/WebAuthenticator/WebAuthenticator.shared.cs | Removes Windows “unsupported” annotations now that Windows implementation exists. |
| src/Essentials/test/DeviceTests/Tests/WebAuthenticator_Tests.cs | Updates WebAuthenticator device tests to run on Windows (no longer expecting PlatformNotSupportedException). |
| src/Essentials/test/DeviceTests/Platforms/Windows/Package.appxmanifest | Registers the xamarinessentials protocol for Windows device tests. |
| src/Essentials/samples/Sample.Server.WebAuthenticator/Controllers/MobileAuthController.cs | Includes code and state values in callback payload to support code-based auth flows. |
| var value = Microsoft.Win32.Registry.ClassesRoot.OpenSubKey(callbackUrl.Scheme); | ||
| if (value is null || value.GetValue("URL Protocol") is null) | ||
| { | ||
| throw new InvalidOperationException($"The URI Scheme '{callbackUrl.Scheme}' is not registered. Call ActivationRegistrationManager.RegisterForProtocolActivation to register protocol activation."); | ||
| } |
There was a problem hiding this comment.
Registry.ClassesRoot.OpenSubKey(...) returns a RegistryKey that should be disposed/closed. Consider using using var value = ... (or a try/finally) before reading URL Protocol, to avoid leaking registry handles in unpackaged scenarios.
| var reader = doc.CreateReader(); | ||
| var namespaceManager = new XmlNamespaceManager(reader.NameTable); | ||
| namespaceManager.AddNamespace("x", PlatformUtils.AppManifestXmlns); | ||
| namespaceManager.AddNamespace("uap", "http://schemas.microsoft.com/appx/manifest/uap/windows10"); |
There was a problem hiding this comment.
IsUriProtocolDeclared hard-codes the UAP XML namespace string even though PlatformUtils.AppManifestUapXmlns already defines it (and is used elsewhere, e.g., Permissions). Using the shared constant would reduce duplication and avoid drift if the namespace ever changes.
| namespaceManager.AddNamespace("uap", "http://schemas.microsoft.com/appx/manifest/uap/windows10"); | |
| namespaceManager.AddNamespace("uap", PlatformUtils.AppManifestUapXmlns); |
| { | ||
| var docPath = FileSystemUtils.PlatformGetFullAppPackageFilePath(PlatformUtils.AppManifestFilename); | ||
| var doc = XDocument.Load(docPath, LoadOptions.None); | ||
| var reader = doc.CreateReader(); |
There was a problem hiding this comment.
doc.CreateReader() returns an XmlReader that implements IDisposable. Consider disposing it (e.g., using var reader = doc.CreateReader();) to avoid holding onto unmanaged resources longer than necessary when parsing the manifest.
| var reader = doc.CreateReader(); | |
| using var reader = doc.CreateReader(); |
🧪 PR Test EvaluationOverall Verdict: The lifecycle event plumbing is well-tested with a focused unit test, but the substantial new Windows
📊 Expand Full EvaluationPR Test Evaluation ReportPR: #34887 — Windows WebAuthenticator OAuth callbacks via app activation Overall VerdictThe new 1. Fix Coverage —
|
| Fix Area | Test Coverage |
|---|---|
WindowsLifecycle.OnAppActivation delegate |
✅ Unit test |
WindowsLifecycleBuilderExtensions.OnAppActivation |
✅ Unit test (implicitly) |
MauiWinUIApplication.OnAppActivation / RegisterForAppActivation |
✅ Unit test (event dispatch) |
EssentialsMauiAppBuilderExtensions wiring |
❌ None |
Platform.OnAppActivation dispatch |
❌ None |
WebAuthenticatorImplementation.OnAppActivation |
❌ None |
WebAuthenticatorImplementation.AuthenticateAsync |
❌ None (human-only device test) |
IsUserCancellation |
❌ None |
IsUriProtocolDeclared |
❌ None |
Recommendations
-
Add a unit test for
IsUserCancellation— it's a pure function with clear logic that covers"access_denied", error string containing"cancel", and description containing"cancel". Easily added to an Essentials unit test project. -
Add unit tests for
WebAuthenticatorImplementation.OnAppActivation— the null check, non-Protocol activation guard, andOAuth2Manager.CompleteAuthRequestreturning false are all testable using mocks/stubs without a real Windows activation. -
Consider a unit test for the
wasHandled = falsecase — the current unit test only verifies thatwasHandledistruewhen a handler returnstrue. A complementary test verifyingfalsepropagation (all handlers returnfalse) would complete the coverage. -
Address the "no active window → kill process" path —
System.Diagnostics.Process.GetCurrentProcess().Kill()inOnAppActivationis a surprising side effect in a headless activation scenario. Consider documenting this behavior with a comment and, if possible, making the process exit injectable/mockable for testing.
Warning
⚠️ Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
dc.services.visualstudio.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "dc.services.visualstudio.com"See Network Configuration for more information.
Note
🔒 Integrity filtering filtered 1 item
Integrity filtering activated and filtered the following item during workflow execution.
This happens when a tool call accesses a resource that does not meet the required integrity or secrecy level of the workflow.
- pr:[Windows] WebAuthenticator: add OAuth support via app activation #34887 (
pull_request_read: Resource 'pr:[Windows] WebAuthenticator: add OAuth support via app activation #34887' has lower integrity than agent requires. Agent would need to drop integrity tags [unapproved:all approved:all] to trust this resource.)
🧪 Test evaluation by Evaluate PR Tests
🧪 PR Test EvaluationOverall Verdict: The lifecycle event ordering tests are solid additions, but the Windows WebAuthenticator implementation (the central feature of this PR) has no automated test coverage — all WebAuthenticator device tests require human interaction and cannot run in CI.
📊 Expand Full EvaluationPR Test Evaluation ReportPR: #34887 — Windows WebAuthenticator + Overall VerdictThe new 1. Fix Coverage —
|
Update WebAuthenticator, Platform, EssentialsMauiAppBuilderExtensions, and PublicAPI.Unshipped.txt to use the renamed lifecycle event. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🧪 PR Test EvaluationOverall Verdict: The lifecycle event ordering tests are well-written and directly validate the new
📊 Expand Full EvaluationPR Test Evaluation ReportPR: #34887 — Adds 1. Fix Coverage —
|
| Platform | Fix Coverage | Test Coverage |
|---|---|---|
| Windows | ✅ Primary fix target | ✅ Lifecycle tests; |
| iOS | No change | ✅ Baseline lifecycle tests added |
| Android | No change | ✅ Baseline lifecycle tests added |
| MacCatalyst | No change | ❌ No lifecycle tests for MacCatalyst |
The PR adds iOS and Android lifecycle ordering tests as useful baselines. MacCatalyst is absent, though it shares the iOS lifecycle so this is a minor gap.
8. Assertion Quality — ✅
Lifecycle tests have strong, specific assertions:
Assert.True(activatedIndex < launchingIndex,
$"Expected OnAppInstanceActivated before OnLaunching. Log: [{string.Join(", ", log)}]");This format — using Assert.True with an explicit failure message containing the full log — is excellent for diagnosing failures. The "fires exactly once" assertion is also specific and regression-proof.
9. Fix-Test Alignment — ⚠️
The lifecycle ordering tests align well with the lifecycle-routing changes in MauiWinUIApplication.cs, WindowsLifecycle.cs, and WindowsLifecycleBuilderExtensions.cs.
However, the core OAuth activation logic in WebAuthenticatorImplementation.OnAppInstanceActivated (which handles the actual bug fix — redirecting protocol activations back to the OAuth flow) is not aligned with any automated test. The tests confirm the event fires, but don't verify it does the right thing when called with real OAuth activation arguments.
Recommendations
-
Add automated tests for
WebAuthenticatorImplementation.OnAppInstanceActivatedguard conditions — The null-arg, wrong-kind, and failed-completion paths are straightforward to test as device tests on Windows without human interaction (pass in mockAppActivationArgumentsdata). TheWindowStateManager.Default.GetActiveWindow() is nullpath (process self-termination) warrants a test or at minimum a comment explaining the expected behavior. -
Add unit tests for
IsUserCancellation— This is pure string matching logic and a prime candidate for a[Theory]-based unit test coveringaccess_denied, various cancel-string patterns, and non-cancellation errors. -
Consider testing the re-activation path in
MauiWinUIApplication.OnLaunched— The_application != null && _services != nullbranch that handles app re-activation without rebuilding is a new code path that has no test coverage.
Warning
⚠️ Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
dc.services.visualstudio.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "dc.services.visualstudio.com"See Network Configuration for more information.
Note
🔒 Integrity filtering filtered 2 items
Integrity filtering activated and filtered the following items during workflow execution.
This happens when a tool call accesses a resource that does not meet the required integrity or secrecy level of the workflow.
- pr:[Windows] WebAuthenticator: add OAuth support via app activation #34887 (
pull_request_read: Resource 'pr:[Windows] WebAuthenticator: add OAuth support via app activation #34887' has lower integrity than agent requires. Agent would need to drop integrity tags [approved:all unapproved:all] to trust this resource.) - pr:[Windows] WebAuthenticator: add OAuth support via app activation #34887 (
pull_request_read: Resource 'pr:[Windows] WebAuthenticator: add OAuth support via app activation #34887' has lower integrity than agent requires. Agent would need to drop integrity tags [unapproved:all approved:all] to trust this resource.)
🧪 Test evaluation by Evaluate PR Tests
Replace the Startup.cs + Controller pattern with a single Program.cs using top-level statements and minimal API endpoints. Remove IIS Express launch profile and unused Logging.Debug package. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Move the inline version from the sample csproj into eng/Versions.props and eng/NuGetVersions.targets, matching the pattern used by the other ASP.NET Core authentication packages. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🧪 PR Test EvaluationOverall Verdict: The lifecycle ordering tests are well-structured and directly verify the new
📊 Expand Full EvaluationPR Test Evaluation ReportPR: #34887 — [Windows] WebAuthenticator: add OAuth support via app activation Overall VerdictThe lifecycle event ordering tests are good and directly cover the 1. Fix Coverage —
|
| Currently untested | Could be | Why |
|---|---|---|
IsUserCancellation("access_denied", null) |
Unit test | Pure string logic, no platform context needed |
IsUserCancellation with "cancel" substrings |
Unit test | Same |
AuthenticateAsync throws TaskCanceledException on user cancellation |
Unit test (with mocked OAuth2Manager) |
Business logic, not platform |
IsUriProtocolDeclared / IsRegistryDeclared throwing |
Device test (controlled manifest) | Needs packaged context but not live OAuth |
4. Convention Compliance — ✅
LifecycleEventOrderTests.*.csuses[Fact](xUnit),[Category]attribute — correct.MauiProgram.csregistration pattern is consistent with the existing device test infrastructure.WebAuthenticator_Tests.csuses[Theory]/[InlineData]/[Trait]— correct, consistent with the existing Essentials device test style.TestCategory.Lifecycleis a new constant but is defined correctly alongside existing categories.- No convention violations detected by the automated script.
5. Flakiness Risk — ⚠️ Medium
- Lifecycle tests: ✅ Low risk — they check a static log populated during deterministic app startup.
- WebAuthenticator tests:
⚠️ Medium risk for the human tests — they depend on(xamarinessentialsauthsample.azurewebsites.net/redacted), an external server. If that server goes down or changes, the tests will fail. Since they're[Human]they won't run in automated CI, so this is an accepted risk, but worth noting for maintainability.
6. Duplicate Coverage — ✅ No duplicates
No overlapping tests found for the new OnAppInstanceActivated lifecycle event. The WebAuthenticator tests extend existing patterns.
7. Platform Scope — ✅
- The core fix is Windows-specific; the Windows lifecycle test directly covers it.
- Lifecycle ordering tests were also added for iOS and Android, which provides a useful baseline for those platforms even though the primary fix is Windows-only. This is a net positive.
- No iOS/Android coverage gap for the WebAuthenticator changes (those platforms use a different implementation path untouched by this PR).
8. Assertion Quality — ✅
- Lifecycle assertions include the full event log in the failure message:
$"Expected X before Y. Log: [{string.Join(", ", log)}]"— excellent for debugging CI failures. Assert.Equal(1, count)for "fires exactly once" is precise.- WebAuthenticator assertions check
AccessToken,RefreshToken,ExpiresIn, andResponseDecoder.CallCount— all directly relevant to the OAuth result.
9. Fix-Test Alignment — ⚠️
MauiWinUIApplication.cs↔LifecycleEventOrderTests.Windows.cs: ✅ Good alignment. The tests directly validate the event ordering enforced by the new activation hook placement.WebAuthenticator.windows.cs↔WebAuthenticator_Tests.cs:⚠️ Tests are human-only and cover the complete redirect flow, but don't isolate-test the new code paths added in this PR (cancellation detection, headless kill,OAuth2Managerintegration, protocol registration checks). If the Windows implementation regressed silently, CI would not catch it.EssentialsMauiAppBuilderExtensions.cs(Windows wiring): ❌ No test verifies that the newOnAppInstanceActivatedhook is wired toWebAuthenticatorImplementation.OnAppInstanceActivatedat the DI/lifecycle level.
Recommendations
- Add a unit test for
IsUserCancellation— this pure method covers meaningful business logic ("access_denied","cancel"in error vs description, case-insensitivity). A[Theory]xUnit test inEssentials.UnitTestswould cover this instantly without any platform setup. - Add a device test for
OnAppInstanceActivatedearly-return paths — test thatOnAppInstanceActivatedreturnsfalsefor non-ProtocolExtendedActivationKindvalues and for a Protocol URI thatOAuth2Managerdoesn't recognize. This ensures the method doesn't accidentally swallow non-OAuth activations. - Consider mocking
OAuth2Managerin a device test to verify that a failure response with"access_denied"maps toTaskCanceledExceptionand a generic failure maps toInvalidOperationException— without requiring a live redirect server.
Warning
⚠️ Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
dc.services.visualstudio.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "dc.services.visualstudio.com"See Network Configuration for more information.
Note
🔒 Integrity filtering filtered 2 items
Integrity filtering activated and filtered the following items during workflow execution.
This happens when a tool call accesses a resource that does not meet the required integrity or secrecy level of the workflow.
- pr:[Windows] WebAuthenticator: add OAuth support via app activation #34887 (
pull_request_read: Resource 'pr:[Windows] WebAuthenticator: add OAuth support via app activation #34887' has lower integrity than agent requires. Agent would need to drop integrity tags [unapproved:all approved:all] to trust this resource.) - pr:[Windows] WebAuthenticator: add OAuth support via app activation #34887 (
pull_request_read: Resource 'pr:[Windows] WebAuthenticator: add OAuth support via app activation #34887' has lower integrity than agent requires. Agent would need to drop integrity tags [approved:all unapproved:all] to trust this resource.)
🧪 Test evaluation by Evaluate PR Tests
In .NET 10, UseAuthorization() requires AddAuthorization() to be called explicitly — it is no longer auto-registered by the framework. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🧪 PR Test EvaluationOverall Verdict: ✅ Tests are adequate The lifecycle event ordering tests directly verify the core fix (that
📊 Expand Full EvaluationPR Test Evaluation ReportPR: #34887 — Windows OAuth activation event support Overall Verdict✅ Tests are adequate The lifecycle event order tests directly validate the critical change: that 1. Fix Coverage — ✅The new
The removal of 2. Edge Cases & Gaps —
|
🤖 AI Summary
📊 Review Session —
|
| Category | Tests | Snapshot diffs |
|---|---|---|
Essentials |
— | — |
📎 Download drop-deep-uitests artifact (TRX + snapshot diffs) |
🔍 Regression Cross-Reference
🔍 Regression Cross-Reference
🟢 No regression risks detected. No labeled bug-fix PRs in the last 6 months touched the modified files.
🔍 Pre-Flight — Context & Validation
Issue: N/A - PR supersedes #30056; no direct issue metadata available in local gh auth context.
PR: #34887 - [Windows] WebAuthenticator: add OAuth support via app activation
Platforms Affected: Windows primary; Android used for requested regression testing because shared Essentials/WebAuthenticator tests and samples are touched.
Files Changed: 20 implementation/sample/build/public API files detected by gate; 7 test/device-test files detected in the squashed PR.
Key Findings
- GitHub CLI was not authenticated, so PR metadata was gathered through GitHub MCP plus local squashed commit state.
- Gate already failed before executing tests:
verify-tests-fail.ps1detected deleted sample files as fix files, then failed on missingsrc/Essentials/samples/Sample.Server.WebAuthenticator/Controllers/MobileAuthController.cs. - Current PR fix combines Windows WebAuthenticator implementation with a substantial sample-server rewrite/deletion, which is unrelated to the core Windows activation fix and creates Android gate churn.
- Android Essentials device tests with
-TestFilter "WebAuthenticator_Tests"completed successfully on emulator API 30:Tests run: 297 Passed: 267 Failed: 0 Ignored: 30.
Code Review Summary
Verdict: NEEDS_CHANGES
Confidence: high
Errors: 2 | Warnings: 0 | Suggestions: 0
Key code review findings:
- ❌
src/Essentials/src/WebAuthenticator/WebAuthenticator.shared.cs:73adds a required Windows member to shipped public interfaceIPlatformWebAuthenticatorCallback, which is a breaking change for existing implementers. - ❌
src/Core/src/Hosting/EssentialsMauiAppBuilderExtensions.cs:78makes Essentials performFindOrRegisterForKey("MauiEssentials"), redirect, andProcess.Kill()for any unhandled activation, potentially forcing single-instance behavior for unrelated Windows app activations.
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #34887 | Add Windows WebAuthenticator using OAuth2Manager, new app-activation lifecycle hook, and sample-server rewrite/deletions |
❌ FAILED (Gate pre-test file existence); Android regression command ✅ PASSED | 27 files | Original PR; gate stopped because deleted sample files were classified as required fix files |
🔬 Code Review — Deep Analysis
Code Review — PR #34887
Independent Assessment
What this changes: Adds Windows WebAuthenticator support using WinAppSDK OAuth2Manager and app-activation callbacks, plus sample/test updates.
Inferred motivation: Enable Windows OAuth/protocol callback handling instead of throwing PlatformNotSupportedException.
Reconciliation with PR Narrative
Author claims: Ports Windows OAuth/WebAuthenticator work onto the new Windows app-activation lifecycle hook.
Agreement/disagreement: Matches the code, but two implementation details introduce broader API/lifecycle risks.
Findings
❌ Error — Breaking public interface change
src/Essentials/src/WebAuthenticator/WebAuthenticator.shared.cs:73
Adds a required member to shipped public Windows interface IPlatformWebAuthenticatorCallback. Existing implementers may fail to compile/load. Prefer a non-breaking pattern such as a default interface method, versioned interface, or helper.
❌ Error — Essentials forces Windows single-instance behavior
src/Core/src/Hosting/EssentialsMauiAppBuilderExtensions.cs:78
The fallback FindOrRegisterForKey("MauiEssentials") / redirect / Process.Kill() path runs for any unhandled activation, not just auth protocol callbacks. This can make all Windows MAUI apps using Essentials single-instance and kill secondary launches.
Devil's Advocate
The general direction is reasonable, and existing platform implementations also keep one pending auth operation. These two issues are still concrete: one affects public API compatibility, the other changes app lifecycle semantics outside WebAuthenticator.
Verdict: NEEDS_CHANGES
Confidence: high
Summary: Code should address the public interface break and scope Windows activation redirection to auth/protocol scenarios before merge. No GitHub comments were posted.
🔧 Fix — Analysis & Comparison
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | try-fix-1 | Surgical Windows activation bridge; keep sample server unchanged; replace breaking callback member with optional Windows hook; remove global Essentials redirect/kill fallback | ✅ Android regression passed; gate not re-run per instruction; statically addresses gate missing-file failure | EssentialsMauiAppBuilderExtensions.cs, WebAuthenticator.shared.cs, WebAuthenticator.windows.cs, restored sample server files |
Best immediate repair: fixes both code-review errors and the gate failure cause |
| 2 | try-fix-2 | Launcher + protocol callback only; avoid OAuth2Manager and direct OAuth sample changes |
✅ Android regression passed; gate not re-run per instruction; statically addresses gate missing-file failure | WebAuthenticator.windows.cs, sample view model/server restored |
Simpler and safer, but may not satisfy direct OAuth2/PKCE product goal |
| 3 | try-fix-3 | Explicit Windows activation handler service/dispatcher; WebAuthenticator registers scoped handler only while pending | ✅ Android regression passed; gate not re-run per instruction; statically addresses gate missing-file failure | Windows lifecycle/platform hosting + WebAuthenticator registration | Clean long-term design, but larger and likely needs design/API review |
| PR | PR #34887 | Windows WebAuthenticator via OAuth2Manager, new lifecycle hook, sample-server rewrite/deletions |
❌ Gate failed before testing; ✅ Android regression command passed in this environment | 27 files | Gate failure: deleted sample controller/startup files classified as fix files and then required to exist |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| maui-expert-reviewer | 1 | Yes | Prefer surgical repair: keep samples unchanged, use non-breaking optional Windows activation hook, scope activation handling to pending WebAuthenticator protocol callbacks |
| code-review sub-agent | 1 | Yes | Avoid breaking IPlatformWebAuthenticatorCallback; remove Essentials global single-instance redirect/Process.Kill behavior |
| orchestrator | 1 | Yes | Treat the Android gate failure as a deleted-file/gate-classification problem; avoid unrelated sample deletions in fix candidates |
Test command executed: pwsh .github/skills/run-device-tests/scripts/Run-DeviceTests.ps1 -Project Essentials -Platform android -TestFilter "WebAuthenticator_Tests"
Android regression result: ✅ Passed on emulator emulator-5554 API 30. XHarness exit code 0; Tests run: 297 Passed: 267 Inconclusive: 0 Failed: 0 Ignored: 30.
Gate handling: The gate phase was not re-run, per instruction. The existing gate artifact shows failure before test execution due to missing deleted sample file src/Essentials/samples/Sample.Server.WebAuthenticator/Controllers/MobileAuthController.cs.
Exhausted: Yes — remaining distinct approaches are variations of (1) surgical non-breaking activation hook, (2) simpler Launcher-only WebAuthenticator, or (3) larger activation dispatcher/service architecture.
Selected Fix: Candidate #1 — it is the smallest candidate that directly addresses the gate failure and the two high-confidence code-review errors while preserving the PR's Windows WebAuthenticator goal.
📋 Report — Final Recommendation
Comparative Report — PR #34887
Candidate ranking
| Rank | Candidate | Regression status | Assessment |
|---|---|---|---|
| 1 | pr-plus-reviewer |
No failed regression observed; gate not re-run per instruction | Best candidate. It preserves the PR's Windows OAuth/AppActivation goal while applying the expert feedback: non-breaking callback wiring, no global single-instance kill fallback, scoped pending-auth activation handling, non-empty OAuth client id handling, stable sample URL, and restored deleted sample files. |
| 2 | try-fix-1 |
Android regression passed | Strong surgical repair. It fixes the two original high-confidence review errors and restores the sample files that blocked the gate, but the recorded candidate does not address the newly found empty client_id OAuth2Manager issue or the transient dev-tunnel sample URL as explicitly as pr-plus-reviewer. |
| 3 | try-fix-2 |
Android regression passed | Safest runtime behavior because it avoids OAuth2Manager and uses Launcher plus protocol callback only, but it may not satisfy the PR's direct OAuth2/PKCE product goal. |
| 4 | try-fix-3 |
Android regression passed | Architecturally clean activation-dispatch direction, but it is larger than needed for this fix and likely requires additional design/API review. It is less suitable as the immediate winning candidate. |
| 5 | pr |
Gate failed before test execution; Android regression command passed | Raw PR should not win. It has major API/lifecycle/OAuth correctness issues and deleted sample files that triggered the known gate failure. |
Candidate details
pr
The raw PR implements Windows WebAuthenticator app-activation support with OAuth2Manager, adds a Windows activation lifecycle hook through Essentials, and rewrites/deletes parts of the sample server. It failed the gate before test execution because deleted sample files were classified as required fix files. The expert review also found major API compatibility, Windows lifecycle, and OAuth correctness issues.
pr-plus-reviewer
This candidate applies the expert review feedback in a sandbox copy. It keeps the PR's intended Windows WebAuthenticator/OAuth support but removes the breaking interface member, avoids Essentials-owned global single-instance behavior, handles only pending matching WebAuthenticator protocol callbacks, avoids an empty OAuth client id, restores a stable sample endpoint, and restores the deleted sample files that caused the gate blocker.
try-fix-1
This is the best STEP 6a candidate. It is small and directly addresses the initial code-review errors plus the deleted-file gate failure. It ranks below pr-plus-reviewer because it does not explicitly incorporate the later expert findings around OAuth2Manager's empty client id and the transient dev-tunnel sample endpoint.
try-fix-2
This candidate is simpler and avoids the OAuth2Manager correctness issue by relying on Launcher plus protocol activation. It is safe but less complete for the PR's stated Windows direct OAuth2/PKCE objective.
try-fix-3
This candidate has the cleanest long-term lifecycle architecture, but it is too broad for a quick PR repair and needs design review beyond the scope of the immediate fix.
Winning candidate
Winner: pr-plus-reviewer
Rationale: it is the only candidate that preserves the PR's intended Windows OAuth support while addressing all expert findings and the known gate blocker. The raw PR is disqualified by major correctness issues and gate failure; the try-fix candidates that passed Android are credible, but each either leaves later expert feedback unaddressed or gives up part of the intended feature.
kubaflo
left a comment
There was a problem hiding this comment.
Could you please check the ai's suggestions?
|
Looks like there has been significant progress on this. Is it safe to assume that WebAuthenticator will work in MAUI apps when .NET 11 is released? |
This comment has been minimized.
This comment has been minimized.
…ivation (#36415) > [!NOTE] > Are you waiting for the changes in this PR to be merged? > It would be very helpful if you could <a href=https://github.com/dotnet/maui/wiki/Testing-PR-Builds>test the resulting artifacts</a> from this PR and let us know in a comment if this change resolves your issue. Thank you! ### Root Cause `WebAuthenticator` is unsupported on Windows. A custom-protocol callback may launch a second app process, so Windows needs both protocol activation handling and cross-instance routing back to the process that owns the pending authentication. Windows App SDK also cannot reliably re-register an `AppInstance` after `UnregisterKey()` ([microsoft/WindowsAppSDK#4420](microsoft/WindowsAppSDK#4420)). Releasing the route after the first authentication caused a subsequent callback process to find no route owner. After a redirected callback completes, the existing MAUI window is not automatically restored or brought in front of the browser. The request must retain its originating window and perform a best-effort foreground activation only after a valid callback completes it. ### Description of Change Adds a Windows implementation that preserves the existing cross-platform contract: caller-provided authentication URL and callback URL in, `WebAuthenticatorResult` out. - opens the system browser with `Launcher.LaunchUriAsync` - handles protocol callbacks through `OnAppInstanceActivated` from #34883 - registers a WebAuthenticator-specific `AppInstance` route for the callback scheme - redirects transient callback processes to the instance that owns that route - keeps the route registered for the lifetime of the process while keeping pending authentication state request-scoped - validates packaged manifest and unpackaged protocol registration - forwards callbacks through the existing `IPlatformWebAuthenticatorCallback` - captures the `AppWindow` that started each authentication so multi-window callbacks return to the correct window - restores minimized or hidden windows without intermediate activation, then makes one best-effort foreground request This intentionally does **not** use `OAuth2Manager`. The caller still owns `state`, PKCE, provider parameters, and token exchange. ### Key Technical Details - route keys use `Microsoft.Maui.WebAuthenticator:{scheme}` - application-owned `AppInstance` keys are preserved - transient processes use `AppInstance.GetInstances()` to find the route owner without registering another key - final callback matching still uses `WebUtils.CanHandleCallback(...)` - only one WebAuthenticator request can be pending per app instance, matching the existing platform model - redirect completion is awaited before the transient callback process exits - the route remains registered for the process lifetime to avoid the `UnregisterKey()` re-registration issue - the persistent route is routing infrastructure; the TCS and expected callback URI determine whether a callback is currently handled - only the callback that wins `TrySetResult` may restore and foreground its captured window - cancellation, invalid callbacks, duplicate callbacks, and late callbacks do not request foreground activation - minimized windows use `OverlappedPresenter.Restore(false)` and hidden windows use `AppWindow.Show(false)` so `SetForegroundWindow` is the only explicit activation request - foreground work is isolated behind best-effort exception handling and never changes the OAuth result `RedirectActivationToAsync` already requests foreground permission for the route-owner process through `AllowSetForegroundWindow` in Windows App SDK [`AppInstance::QueueRequest`](https://github.com/microsoft/WindowsAppSDK/blob/91160b078668051e58ba1e19eb699f4efc8c5b20/dev/AppLifecycle/AppInstance.cpp). MAUI therefore does not duplicate that call. The foreground path intentionally does not use `AppWindow.DispatcherQueue`. [`AppWindow`](https://learn.microsoft.com/windows/windows-app-sdk/api/winrt/microsoft.ui.windowing.appwindow?view=windows-app-sdk-1.8) and [`OverlappedPresenter`](https://learn.microsoft.com/windows/windows-app-sdk/api/winrt/microsoft.ui.windowing.overlappedpresenter?view=windows-app-sdk-1.8) are agile, and a dispatcher queue is not guaranteed to be available in the redirected activation context. Packaged apps declare the callback scheme in `AppxManifest.xml`; unpackaged apps register it with `ActivationRegistrationManager.RegisterForProtocolActivation(...)`. ### Foreground Behavior Windows ultimately decides whether [`SetForegroundWindow`](https://learn.microsoft.com/windows/win32/api/winuser/nf-winuser-setforegroundwindow) succeeds. Foreground activation is therefore best effort by design: failure is diagnostic only and never changes a successful authentication result. ### Testing The routing revision passed Unit, Public API, Windows build, sample build, and all 14 WebAuthenticator Windows device tests. The foreground revision was validated with: - `Microsoft.Maui.Essentials` Windows build: 0 warnings, 0 errors - manual OAuth callback verification confirming that the existing MAUI window returns in front of the browser - runtime verification that accessing `AppWindow.DispatcherQueue` from the redirected activation context can fail with `COMException`; the final implementation does not depend on it Additional human-interaction validation remains useful for minimized, maximized, multi-window, cancellation followed by retry, invalid callback, and consecutive-authentication scenarios. ### Breaking Changes None. ### Issues Fixed - Alternative implementation to #34887 - Related to #30056 - Stacked on #34883 - Related Windows App SDK behavior: [microsoft/WindowsAppSDK#4420](microsoft/WindowsAppSDK#4420) Looking forward to your feedback, thanks! @mattleibow @dotMorten Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
closed in favour of #36640 |
…tor support (#36640) ### Description of Change .NET MAUI on Windows currently has two related gaps: it doesn't expose the Windows App SDK `AppActivationArguments` through its lifecycle events, and `WebAuthenticator` isn't supported on Windows. As a result, single-instancing, protocol and file activation, and browser-based authentication callbacks require additional app-specific plumbing. This PR addresses both areas by adding: - a Windows `OnAppInstanceActivated` lifecycle hook with handled semantics; - initial and subsequent `AppInstance` activation delivery through the MAUI lifecycle; - lifetime-safe UI dispatching for later activation callbacks; - Windows `WebAuthenticator` support using the system browser, protocol activation, callback routing, result completion, asynchronous activation redirection, and best-effort window foreground activation; - packaged manifest and unpackaged protocol-registration validation; - a Controls Sample implementation demonstrating single-instancing and activation redirection; - Windows lifecycle, WebAuthenticator routing, and callback validation tests. The implementation keeps the existing cross-platform `WebAuthenticator` contract and uses the established `Launcher` + callback route + pending task model. ### Related Work This PR fixes #2702 and is also related to #9973. It is a fresh, self-contained alternative to: #34883, which introduces the Windows AppInstance lifecycle hook; #34887, which adds Windows WebAuthenticator support as a stacked OAuth2Manager-based follow-up to #34883. This version combines both areas in one PR based on the current `net11.0` branch while preserving the existing MAUI WebAuthenticator model. Related to #9973. ### Testing - Debug and Release x64 builds passed for Core, Essentials, Controls Sample, Core DeviceTests, and Essentials DeviceTests. - `WebUtils_Tests`: 20/20 passed. - Windows WebAuthenticator routing helpers: 14/14 passed. - Windows lifecycle device tests: 3/3 passed. - Public API validation passed. - Controls Sample single-instance redirection was manually validated, including suspending the route owner and resuming it after a second instance attempted redirection. - Packaged and framework-dependent unpackaged browser/protocol callback transports completed successfully and returned to the existing runner without leaving a second window. - Application-owned routing completed `owned-ok` in the main process while preserving the `Pr36640.AppOwned` key; the transient process exited and the main process remained alive. - Sequential authentications using `pr36640-a` and `pr36640-b` both completed in one process, with the route key updating to `Microsoft.Maui.WebAuthenticator:pr36640-b`. - A focused compatibility harness confirmed the original abstract-member break (`CS0535`), source and binary compatibility with the default member, a default result of `false`, and an overriding result of `true`. - The public test endpoint returned its literal `ACCESS_TOKEN_VALUE` placeholder instead of `testtokenvalue`, so transport completed but that external-data assertion remained unsuitable as a deterministic pass criterion. ### Review Notes - **Asynchronous activation redirect:** the lifecycle callback now marks the transient activation handled immediately and starts an asynchronous helper. The helper awaits `RedirectActivationToAsync` and terminates the transient process only after the native operation completes or fails, removing the previous sync-over-async wait from the activation path. - **Why there is no destructive timeout:** a five-second `WaitAsync` followed by process termination was tested with the route owner suspended. The transient exited at about 5.4 seconds, but resuming the owner produced an unhandled Windows App Runtime failure in `RedirectionRequest.cpp` (`0x8003001E`, `wil::ResultException`). The managed timeout does not safely cancel the pending native redirection. This matches the Windows App SDK guidance that the caller must await redirection before exiting: https://learn.microsoft.com/windows/apps/windows-app-sdk/applifecycle/applifecycle-instancing - **Redirected activation lifetime:** Windows App SDK keeps the redirected activation request alive through the synchronous `AppInstance.Activated` event. MAUI now waits for the dispatched UI lifecycle handlers to finish before that event callback returns. This prevents the redirecting process from releasing marshaled resources before the UI consumes the activation, while preserving lifecycle exception behavior and avoiding a destructive timeout. - **Public interface compatibility:** the new Windows member on the already-shipped `IPlatformWebAuthenticatorCallback` has a default implementation returning `false`. Existing custom implementers therefore remain source- and binary-compatible, while implementations that opt in can override it. - **Warm activation delivery:** the same activation might theoretically reach both `OnLaunched` and `AppInstance.Activated`. This has not been reproduced and should be verified before adding deduplication. - **Application-owned `AppInstance` key:** when an app owns the key, it also owns activation routing and must redirect protocol callbacks to the instance that started authentication. The Controls Sample now calls this out explicitly. A general fail-fast cannot distinguish a complete cooperative router from an incomplete one without rejecting the supported app-owned scenario. - **Sequential callback schemes:** Windows App SDK 1.8 can rebind the current instance to a different key without calling `UnregisterKey`; the tested A-to-B flow completed successfully. MAUI intentionally avoids `UnregisterKey` because registering another key after it is unsafe (`microsoft/WindowsAppSDK#4420`). Thank you @kubaflo for suggesting a fresh PR and for the previous reviews.
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Description of Change
This ports the Windows OAuth/WebAuthenticator work from #30056 onto the new Windows app-activation lifecycle hook from #34883, so protocol callbacks are handled through Essentials instead of special-casing
MauiWinUIApplicationstartup.OnAppActivation(...)on Windows and forwards protocol activations toWebAuthenticatorOAuth2Manager.CompleteAuthRequest(...)while preserving response decoding pluscode/statevaluesTaskCanceledExceptionand keeps the unpackaged protocol-registration checks from the original community workIssues Fixed