Change Windows implementation with our own solution #287
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
frederikprijck
force-pushed
the
fix/logout
branch
from
07efe9e to
9301a98
Compare
frederikprijck
force-pushed
the
fix/logout
branch
from
7ddec43 to
6f15b4c
Compare
frederikprijck
changed the title
frederikprijck
commented
Oct 26, 2023
| @@ -52,6 +52,7 @@ jobs: | |||
| nuget pack nuget/Auth0.OidcClient.AndroidX.nuspec | |||
| nuget pack nuget/Auth0.OidcClient.Core.nuspec | |||
| nuget pack nuget/Auth0.OidcClient.iOS.nuspec | |||
| nuget pack nuget/Auth0.OidcClient.MAUI.nuspec | |||
There was a problem hiding this comment.
We are moving MAUI packaging to follow the same pattern as we do with other SDKs throuhg a nuspec file to better control platform specific dependencies.
frederikprijck
force-pushed
the
fix/logout
branch
2 times, most recently
from
5252733 to
d819e8c
Compare
frederikprijck
force-pushed
the
fix/logout
branch
from
4008bd7 to
3a4b412
Compare
frederikprijck
commented
Oct 26, 2023
Comment on lines
+80
to
+87
| public void OpenBrowser(Uri uri) | ||
| { | ||
| var process = new System.Diagnostics.Process(); | ||
| process.StartInfo.FileName = "rundll32.exe"; | ||
| process.StartInfo.Arguments = $"url.dll,FileProtocolHandler \"{uri.ToString().Replace("\"", "%22")}\""; | ||
| process.StartInfo.UseShellExecute = true; | ||
| process.Start(); | ||
| } |
There was a problem hiding this comment.
Before going to GA, we will want to have a decent security review on this. It works as expected, and is also used in other packages, but does look a bit funky.
If this turns out to be an issue before we go GA, we can swap for an embedded webview implementation.
frederikprijck
commented
Oct 27, 2023
| cancellationToken.ThrowIfCancellationRequested(); | ||
| } | ||
|
|
||
| var newUri = authorizeUri.ToString().IndexOf("logout", StringComparison.CurrentCultureIgnoreCase) > -1 ? StateModifier.MoveStateToReturnTo(authorizeUri) : authorizeUri; |
There was a problem hiding this comment.
We should make this check more resilient by ensuring we do not just check for the logout string, but actualy check if the full path is v2/logout.
adamjmcgrath
approved these changes
Oct 27, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
This PR replaces WinUIEx with our own implementation, heavily inspired on WinUIEx.
The reason this is being swapped is because of behavior in WinUIEx that does not allow us to use the package in its current state.
Additionally, we stripped things that we do not need, and added things we believe make sense in our case.
Checklist