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

Improve cookie behavior on .NET control #611

Closed
Symbai opened this issue Nov 8, 2020 · 11 comments
Closed

Improve cookie behavior on .NET control #611

Symbai opened this issue Nov 8, 2020 · 11 comments
Labels
feature request feature request

Comments

@Symbai
Copy link

Symbai commented Nov 8, 2020

The current design choice of the WebView2 cookie in .NET (Core) 1.0.674-prerelease control is terrible. It acts like this comes from C++ world. There are several things that will frustrate .NET developers:

CookieList

1.

GetCookiesAsync returns a CookieList class. Why not taking an existing C# collection type such as List? And even if there was a good reason to introduce an own collection class, why is there no enumerator on this collection? Without we cannot do things like this:

foreach (var cookie in await webView.CoreWebView2.CookieManager.GetCookiesAsync("..."))
{
   //....
}

2.

The next extremely unexpected design choice is that Count property returns an uint. This makes sense because it cannot be negative. However all collections in .NET return an int on their Count property. Because it returns an uint, a very common for loop will throw an exception in Visual Studio:

for (int i = 0; i < cookieList.Count; i++)
{
   var cookie = cookieList.GetValueAtIndex(i); // <--- throws
}

At least this one can easily be fixed on our end by changing the int to uint on our for counter. But it rather should follow other long existing collections and use an int. And throw an OutOfIndexException on GetValueAtIndex when people do stupid things like GetValueAtIndex(-1). As of now, this doesn't really fit into .NET, again.

3.

Why is there no Indexer? (If you would have chosen List<T> you would have get all of this for free). It would make things a lot easier for us:

var cookie = cookieList[0];

CoreWebView2Cookie

The next thing is that instead of returning a collection of System.Net.Cookie you decided to make your own Cookie class. This forces us to generate a System.Net.Cookie based on your Cookie class every time we want to use that cookie outside of your control. For example to pass it to a WebClient or HttpWebRequest

Well again, there MIGHT be a reason why you wanted to do that. However, when we would want to create a System.Net.Cookie from it we run into another issue:

DateTime

Your choice for for example Expires property is to make it type double. The System.Net.Cookie however wants it to be DateTime instead. And DateTime is also a lot easier to handle in .NET because we have littler helper properties on this struct such as Day / Month etc without doing all the calculation from a double ourselves.

Examples

Current behavior (using cookie for web request)

CookieContainer cookieContainer = new CookieContainer();
var cookieList = await webView.CoreWebView2.CookieManager.GetCookiesAsync("...");
for (uint i = 0; i < cookieList.Count; i++)
{
    var cookie = cookieList.GetValueAtIndex(i);
    cookieContainer.Add(new Cookie
    {
        Domain = cookie.Domain,
        Expires = DateTime.FromOADate(cookie.Expires),
        Path = cookie.Path,
        Value = cookie.Value,
        Name = cookie.Name,
    });
}

HttpWebRequest httpWebRequest = (HttpWebRequest)WebRequest.Create("...");
httpWebRequest.CookieContainer = cookieContainer;

Expected behavior

CookieContainer cookieContainer = new CookieContainer();
foreach(var cookie in await webView.CoreWebView2.CookieManager.GetCookiesAsync("..."))
    cookieContainer.Add(cookie);

HttpWebRequest httpWebRequest = (HttpWebRequest)WebRequest.Create("...");
httpWebRequest.CookieContainer = cookieContainer;

Current behavior (retrieving all cookies which expire this month)

var cookieList = await window!.webView.CoreWebView2.CookieManager.GetCookiesAsync("...");
var expiredCookies = new List<CoreWebView2Cookie>();
for (uint i = 0; i < cookieList.Count; i++)
{
    var cookie = cookieList.GetValueAtIndex(i);
    if (DateTime.FromOADate(cookie.Expires).Month = DateTime.Now.Month)
        expiredCookies.Add(cookie);
}

Expected behavior (retrieving all cookies which expire this month)

var cookies = await webView.CoreWebView2.CookieManager.GetCookiesAsync("...").Where(x => x.Expires.Month == DateTime.Now.Month).ToList()
@Symbai Symbai added the feature request feature request label Nov 8, 2020
@dmcgloin
Copy link

Totally agree with all points made in this feature request. Thanks for the detailed write up, @Symbai

@champnic
Copy link
Member

Hey @Symbai, thanks for the feedback. These seem like the come from a C++ world because they do come from a C++ world. However, we already have some .NET cookie improvements done that will be available in the next SDK release, including returning the cookies as a List, and support for easier conversion between CoreWebView2Cookie and System.Net.Cookie. I believe this should cover all of your points, but I'll keep this open until that support lands and you can try it out!

Also, be sure to keep an eye on our Announcements repo and our incoming API reviews (for example, the Cookie API review) - this is great feedback to get, and the earlier we get it usually makes it easier to incorporate into our designs. Thanks!

@ivoloshin
Copy link

ivoloshin commented Nov 18, 2020

I also would like to see better cookie handling so I can open one instance of the browser, take the cookies it has collected via login, then when when opening a new instance of the browser, pass those cookies to it so it can already be "logged in". My preferable implementation is to just assign the cookie manager to the new instance, but copying cookies will also work...

Edit: it seems that I got it to work by copying the cookies from one browser to the other... When can we expect this API to make into production?

@ShaunLoganOracle
Copy link

Piling on: I concur with @Symbai (good write up!). I just went through the same pain points you documented while writing some C#/.NET code to interact with the CookieManager . Your for loop: for (uint i = 0; i < cookieList.Count; i++) ... is eerily identical to mine.

@ShaunLoganOracle
Copy link

FWIW - I am pretty sure that there is a loss of fidelity if one converts from CoreWebView2CookieList to CookieCollection and then re-creates CoreWebView2Cookie instances from the .NET Cookies and adds them to a (say brand new) WebView2 instance via CookieManager.AddOrUpdateCookie()
See issue #644

@ivoloshin
Copy link

FWIW - I am pretty sure that there is a loss of fidelity if one converts from CoreWebView2CookieList to CookieCollection

@ShaunLoganOracle Since the cookie properties are all value types, I think storing cookie values somewhere else (as i did in a cookie variable) shouldn't be a big deal, except that when creating a new cookie from scratch to populate it in the new browser, I found that there's not a way to set one specific property - IsSession, however, I was able to re-create cookies "manually" in the new browser without setting that variable and they worked fine for me. I'm assuming because IsSession is a bool, it's false by default.

Here's the code:

var newCookie = browserControl.CoreWebView2.CookieManager.CreateCookie(cookie.Name, cookie.Value, cookie.Domain, cookie.Path);
newCookie.IsHttpOnly = cookie.IsHttpOnly;
newCookie.Expires = cookie.Expires;
newCookie.IsSecure = cookie.IsSecure;
newCookie.SameSite = CoreWebView2CookieSameSiteKind.None;
browserControl.CoreWebView2.CookieManager.AddOrUpdateCookie(newCookie);

@Symbai
Copy link
Author

Symbai commented Nov 23, 2020

With 1.0.707-prerelease everything have been addressed, so I'm closing this issue. Thanks to the outstanding devs ❤👍 I love the feedback reaction so far.

@Symbai Symbai closed this as completed Nov 23, 2020
@SandhyaArun
Copy link

I also would like to see better cookie handling so I can open one instance of the browser, take the cookies it has collected via login, then when when opening a new instance of the browser, pass those cookies to it so it can already be "logged in". My preferable implementation is to just assign the cookie manager to the new instance, but copying cookies will also work...

I really wanted this feature too and it seems to be not available in the latest release.

@ahmed-salem-me
Copy link

@Symbai , can you plz advise on how to create a session cookie? with the IsSession being readonly?, thanks

@Symbai
Copy link
Author

Symbai commented Oct 5, 2022

I'm not using WebView2 anymore and this has been nearly 2 years ago, I cannot give you an answer I'm sorry. You can try asking your question in discussions: https://github.com/MicrosoftEdge/WebView2Feedback/discussions

@ahmed-salem-me
Copy link

@Symbai no worries and thanks for your reply

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request feature request
Projects
None yet
Development

No branches or pull requests

7 participants