Skip to content
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

Remove non-functioning code and instead throw a more helpful platform-not-supported error #23617

Merged
merged 4 commits into from
Aug 22, 2024

Conversation

dotMorten
Copy link
Contributor

@dotMorten dotMorten commented Jul 15, 2024

Description of Change

Removes old UWP code that will never work in a WinUI context. Instead throws a more helpful PlatformNotSupportedException and flags the API as unsupported on Windows.
The current experience is you first get a "add protocol to manifest" error, but addressing that, you then just get a COMException and you're no closer to finding out what the problem is.
This error will lead users to start looking for alternatives, rather than trying to troubleshoot the problem.

Issues Fixed

Fixes #23616

…-not-supported error

Also mark the APIs not supported on Windows.
@dotMorten dotMorten requested a review from a team as a code owner July 15, 2024 23:03
@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Jul 15, 2024
Copy link
Contributor

Hey there @dotMorten! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

var decl = doc.Root.XPathSelectElements($"//uap:Extension[@Category='windows.protocol']/uap:Protocol[@Name='{scheme}']", namespaceManager);

return decl != null && decl.Any();
throw new PlatformNotSupportedException("This implementation of WebAuthenticator does not support Windows. See https://github.com/microsoft/WindowsAppSDK/issues/441 for more details.");
Copy link
Contributor Author

@dotMorten dotMorten Jul 15, 2024

Choose a reason for hiding this comment

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

Let me know if you have a better location to direct people to?
You could direct to https://learn.microsoft.com/en-us/dotnet/maui/platform-integration/communication/authentication?view=net-maui-8.0&tabs=windows#get-started, but that just directs to a closed maui issue which is just as confusing, and in turn that just directs to the above referenced issue, which is where the problem really needs to be addressed first.

The doc could be the better place to direct people to, but in that case the doc needs improvements.

@mattleibow
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

Copy link
Member

@jfversluis jfversluis left a comment

Choose a reason for hiding this comment

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

Build errors across the board!

@dotMorten
Copy link
Contributor Author

dotMorten commented Aug 13, 2024

🤦What was I thinking??? Fixed now

@dotMorten dotMorten requested a review from jfversluis August 13, 2024 16:24
@mattleibow
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@dotMorten
Copy link
Contributor Author

@mattleibow @jfversluis I'm a little bit confused here. The build is failing on building unit tests that would never work on Windows: https://github.com/dotnet/maui/blob/main/src/Essentials/test/DeviceTests/Tests/WebAuthenticator_Tests.cs#L12
First of all the callback scheme isn't defined for the Windows app, so it'll immediately fail with a manifest error. Secondly, if you do add that to the manifest, the code would still throw the COM Exception I'm trying to get rid off and replace with the better exception.
So I was a bit surprised this test actually even compiles for Windows, and can't see how this test could ever have passed. Is this because of the Traits.InteractionTypes.Human attribute? How was this test ever passing?

That leaves me a few options:

  1. Ignore the warning using #pragma warning disable CA1416
  2. Exclude the test from windows using #if !WINDOWS
  3. 1 + Modify the test to test for the expected exception thrown on Windows.

@jfversluis
Copy link
Member

Yep looks like because those require human interaction and thus are automatically excluded: https://github.com/dotnet/maui/blob/61e984eb01fa8fa7fd7133a68bd76008865a01a0/src/Essentials/test/DeviceTests/Traits.cs#L41C27-L41C36

Out of the options I think 3 might be best. That way, when we do implement it, we'll end up with a failing test and make sure to implement it/enable the existing test(s) for Windows

@dotMorten
Copy link
Contributor Author

@jfversluis Thanks. I've made the recommended changes. I don't have a clue how to actually run these human tests though, so going off of the behavior I saw by running the code in a separate app.

@mattleibow
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@mattleibow mattleibow merged commit b1e4bd9 into dotnet:main Aug 22, 2024
97 checks passed
@dotMorten dotMorten deleted the dotMorten/win_auth_notsupported branch August 22, 2024 18:20
Redth pushed a commit that referenced this pull request Aug 22, 2024
…-not-supported error (#23617)

* Remove non-functioning code and instead throw a more helpful platform-not-supported error

Also mark the APIs not supported on Windows.

* Fix build failures

* Update tests for new webauth behavior
@samhouts samhouts added fixed-in-net9.0-nightly This may be available in a nightly release! fixed-in-net8.0-nightly This may be available in a nightly release! labels Aug 27, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Oct 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
community ✨ Community Contribution fixed-in-net8.0-nightly This may be available in a nightly release! fixed-in-net9.0-nightly This may be available in a nightly release!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WebAuthenticator on Windows throws unhelpful errors
4 participants