OAuth2Manager Implementation #4828
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Samples: https://github.com/microsoft/WindowsAppSDK-Samples/tree/user/akanpatel2206/OAuth2_samples/Samples/OAuth |
| return; // Avoid unnecessary construction/activation | ||
| } | ||
|
|
||
| for (auto&& entry : WwwFormUrlDecoder(str)) |
There was a problem hiding this comment.
Nit: See structured binding on cppreference, so you can say for (auto& [name, value] : WwwFormUrlDecoder(str))
| } | ||
| catch (...) | ||
| { | ||
| co_return implementation::TokenRequestResult::MakeFailure(std::move(response), |
There was a problem hiding this comment.
For these "catch and return" points, consider using LOG_CAUGHT_EXCEPTION() so you can see what's happening.
There was a problem hiding this comment.
The official Winappsdk docs mention how we can pull out the relevant hr code and message.
https://github.com/microsoft/WindowsAppSDK/blob/743f4551dcd8eda8db1136d7f61a8ecc7f338325/docs/Coding-Guidelines/Languages-CPP.md
Would it also be helpful if we can fire a telemetry with the error codes for debugging and tracking metrics?
There was a problem hiding this comment.
Used LOG_CAUGHT_EXCEPTION();
…icrosoft/WindowsAppSDK into user/akanpatel2206/OAuth2_impl
|
/azp run |
|
Commenter does not have sufficient privileges for PR 4828 in repo microsoft/WindowsAppSDK |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| WINRT_ASSERT(m_finalized); | ||
|
|
||
| // Per RFC 6749 section 3.1, the auth endpoint URI *MAY* contain a query string, which must be retained | ||
| std::wstring result{ authEndpoint.RawUri() }; |
There was a problem hiding this comment.
I'm surprised there isn't a "URI builder" that would do this for you! Looks fine as-is.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
dev/Common/Common.vcxitems.filters
Outdated
| @@ -43,6 +43,9 @@ | |||
| </ClInclude> | |||
| <ClInclude Include="$(MSBuildThisFileDirectory)winrt_WindowsAppRuntime.h"> | |||
| <Filter>Header Files</Filter> | |||
| </ClInclude> | |||
| <ClInclude Include="$(MSBuildThisFileDirectory)TelemetryHelper.h"> | |||
dev/Common/Common.vcxitems
Outdated
| @@ -27,6 +27,7 @@ | |||
| <ClInclude Include="$(MSBuildThisFileDirectory)Microsoft.Utf8.h" /> | |||
| <ClInclude Include="$(MSBuildThisFileDirectory)NotificationTelemetryHelper.h" /> | |||
| <ClInclude Include="$(MSBuildThisFileDirectory)Security.IntegrityLevel.h" /> | |||
| <ClInclude Include="$(MSBuildThisFileDirectory)TelemetryHelper.h" /> | |||
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
API spec: #4772
Issue: #441
This new WinAppSDK version of the AuthManager is different than the existing public
WebAuthenticationBroker
API. We've opted to follow OAuth best practices more closely - e.g. using user's default browser as part of version 1. The best practices for the API are taken from the
IETF(Internet Engineering Task Force) OAuth 2.0 Authorization Framework RFC 6749,
Proof Key for Code Exchange (PKCE) RFC 7636,
and OAuth 2.0 for Native Apps RFC 8252.
It is recommended to read through the mentioned RFCs for better understanding of the design used in the API spec and the implementation code.
Please go through the API spec to understand the implementation.
Testing
Successful local build
Produced an experimental NuGet package leveraging the aggregator build pipeline, and tested it using a sample app.
Note: The Interactive Experiences package has been incorporated as a dependency in the Foundation repo to utilize Microsoft.UI API features.
Pending Action Items:
Ensure that Maestro recognizes the dependency updates being made to the Foundation branch (the Interactive Experiences package has been integrated into the Foundation repo)
Unit tests
Publishing Public/Private symbols
Create sample apps for WinAppSDK-samples repo.
Test support for C# projections
A microsoft employee must use /azp run to validate using the pipelines below.
WARNING:
Comments made by azure-pipelines bot maybe inaccurate.
Please see pipeline link to verify that the build is being ran.
For status checks on the main branch, please use TransportPackage-Foundation-PR
(https://microsoft.visualstudio.com/ProjectReunion/_build?definitionId=81063&_a=summary)
and run the build against your PR branch with the default parameters.