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

OAuth2 API feedback #4904

Open
dotMorten opened this issue Nov 21, 2024 · 9 comments
Open

OAuth2 API feedback #4904

dotMorten opened this issue Nov 21, 2024 · 9 comments
Assignees
Labels
UWP Gap Issues where functionality available in UWP is missing for Win32 apps

Comments

@dotMorten
Copy link
Contributor

Started to try and use the 1.7exp1 OAuth APIs and got a little bit of feedback.

I must have missed this in the earlier design review, but now seeing this:
Image

This took me a while to discover that I had to do this, and only because as the author of WinUIEx, I have the same requirement in my library, and it's not a fun requirement to have, as I know from experience it has tripped up a lot of users. My hope was that the Windows Apps SDK could solve this at a lower level and not have this same pitfall.

Secondary the example isn't great. It is better to do this in the static main before the application fully starts up, so you don't have to do the forced termination. Perhaps a simple solution could be to just have this be a standard part of the auto-generated main?

Lastly, my app hangs after calling CompleteAuthRequest and VS shows me this message shortly later:
Image
Without the debugger attached the process just hangs for quite a while before shutting down.

@AjitSurana AjitSurana self-assigned this Nov 21, 2024
@codendone codendone added UWP Gap Issues where functionality available in UWP is missing for Win32 apps and removed needs-triage labels Nov 21, 2024
@dotMorten
Copy link
Contributor Author

One additional feedback item that a customer just reported in WinUIEx: Relying on the state parameter can have problems with some OAuth services. See dotMorten/WinUIEx#195 (comment)
I see that this API uses the same trick as WinUIEx to rely on roundtripping the state parameter to resume the correct process.

@akanpatel2206 akanpatel2206 self-assigned this Nov 22, 2024
@akanpatel2206
Copy link
Contributor

Started to try and use the 1.7exp1 OAuth APIs and got a little bit of feedback.

I must have missed this in the earlier design review, but now seeing this: Image

This took me a while to discover that I had to do this, and only because as the author of WinUIEx, I have the same requirement in my library, and it's not a fun requirement to have, as I know from experience it has tripped up a lot of users. My hope was that the Windows Apps SDK could solve this at a lower level and not have this same pitfall.

Secondary the example isn't great. It is better to do this in the static main before the application fully starts up, so you don't have to do the forced termination. Perhaps a simple solution could be to just have this be a standard part of the auto-generated main?

Lastly, my app hangs after calling CompleteAuthRequest and VS shows me this message shortly later: Image Without the debugger attached the process just hangs for quite a while before shutting down.

Thanks for trying out the feature @dotMorten
We have noted down the feedback and created some tasks for the same. Regarding the part where the app hangs for CompleteAuthRequest, would it be possible to share the code sample?
Asking this since I didn't face this on the sample I created, https://github.com/microsoft/WindowsAppSDK-Samples/tree/user/akanpatel2206/OAuth2_samples/Samples/OAuth.
So, your test sample can add to the learning and help debugging the issue.

@akanpatel2206
Copy link
Contributor

One additional feedback item that a customer just reported in WinUIEx: Relying on the state parameter can have problems with some OAuth services. See dotMorten/WinUIEx#195 (comment) I see that this API uses the same trick as WinUIEx to rely on roundtripping the state parameter to resume the correct process.

Hi @dotMorten
Thanks for bringing attention to this.
But as per RFC 6749, https://www.rfc-editor.org/rfc/rfc6749#section-4.1.2, I see that the server should respond back with the same expected state value.
Image

Let me know your thoughts.

@akanpatel2206
Copy link
Contributor

Started to try and use the 1.7exp1 OAuth APIs and got a little bit of feedback.

I must have missed this in the earlier design review, but now seeing this: Image

This took me a while to discover that I had to do this, and only because as the author of WinUIEx, I have the same requirement in my library, and it's not a fun requirement to have, as I know from experience it has tripped up a lot of users. My hope was that the Windows Apps SDK could solve this at a lower level and not have this same pitfall.

Secondary the example isn't great. It is better to do this in the static main before the application fully starts up, so you don't have to do the forced termination. Perhaps a simple solution could be to just have this be a standard part of the auto-generated main?

Lastly, my app hangs after calling CompleteAuthRequest and VS shows me this message shortly later: Image Without the debugger attached the process just hangs for quite a while before shutting down.

For the app hangs part in CompleteAuthRequest, if you are using the same test app, can you check the configuration.
Image

@akanpatel2206
Copy link
Contributor

Started to try and use the 1.7exp1 OAuth APIs and got a little bit of feedback.

I must have missed this in the earlier design review, but now seeing this: Image

This took me a while to discover that I had to do this, and only because as the author of WinUIEx, I have the same requirement in my library, and it's not a fun requirement to have, as I know from experience it has tripped up a lot of users. My hope was that the Windows Apps SDK could solve this at a lower level and not have this same pitfall.

Secondary the example isn't great. It is better to do this in the static main before the application fully starts up, so you don't have to do the forced termination. Perhaps a simple solution could be to just have this be a standard part of the auto-generated main?

Lastly, my app hangs after calling CompleteAuthRequest and VS shows me this message shortly later: Image Without the debugger attached the process just hangs for quite a while before shutting down.

For the app hangs part in CompleteAuthRequest, if you are using the same test app, can you check the configuration? While downloading this test app to a new device, we faced an issue saying class_not_registered, and it was fixed using this.
Image

@dotMorten
Copy link
Contributor Author

Sorry will try and get you a repro tomorrow. With respect to configuration settings I’m using C# and those settings doesn’t really apply.

@akanpatel
Copy link

Started to try and use the 1.7exp1 OAuth APIs and got a little bit of feedback.

I must have missed this in the earlier design review, but now seeing this: Image

This took me a while to discover that I had to do this, and only because as the author of WinUIEx, I have the same requirement in my library, and it's not a fun requirement to have, as I know from experience it has tripped up a lot of users. My hope was that the Windows Apps SDK could solve this at a lower level and not have this same pitfall.

Secondary the example isn't great. It is better to do this in the static main before the application fully starts up, so you don't have to do the forced termination. Perhaps a simple solution could be to just have this be a standard part of the auto-generated main?

Lastly, my app hangs after calling CompleteAuthRequest and VS shows me this message shortly later: Image Without the debugger attached the process just hangs for quite a while before shutting down.

In the sample provided under
https://github.com/microsoft/WindowsAppSDK-Samples/blob/user/akanpatel2206/OAuth2_samples/Samples/OAuth/cs-winui-packaged/TestOAuthCSharp/StartProgram.cs

I'm handling the instance redirection in main. If the redirect is true, we don't create a new instance of the app.

static void Main(string[] args)
{
WinRT.ComWrappersSupport.InitializeComWrappers();
AppInstance.GetCurrent().Activated += OnActivated;

        bool isRedirect = DecideRedirection().GetAwaiter().GetResult();

        if (!isRedirect)
        {
            Microsoft.UI.Xaml.Application.Start((p) =>
            {
                dispatcherQueue = DispatcherQueue.GetForCurrentThread();
                var context = new DispatcherQueueSynchronizationContext(dispatcherQueue);
                SynchronizationContext.SetSynchronizationContext(context);
                new App();
            });
        }
    }

I will merge the samples to release/experimental branch for the same and update the document to modify the example. Let me know if this example looks fine to you.

@akanpatel2206
Copy link
Contributor

hi @dotMorten Gentle reminder for the update on the sample you have created? Also, if you can share the code for which the app hangs, it would be helpful in debugging the issue.

@dotMorten
Copy link
Contributor Author

dotMorten commented Dec 11, 2024

@akanpatel2206 Sorry! Was a busy couple of weeks, and I got away from here.

Just run this sample: OAuthRepro.zip
Click the sign in button, and sign in in the browser (using a mocked oauth server - no password required). When it tries to resume the app after signing in, you'll see the crash.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
UWP Gap Issues where functionality available in UWP is missing for Win32 apps
Projects
None yet
Development

No branches or pull requests

5 participants