Implement Request credentials #399

aguacongas · 2018-03-27T17:04:07Z

#357 Implements fetch request credentials

dnfclas · 2018-03-27T17:04:23Z

All CLA requirements met.

SteveSandersonMS · 2018-03-29T10:28:43Z

Thanks for contributing this, @aguacongas! I appreciate the thorough PR with tests.

On consideration, I think I would propose a slightly different way to implement this feature. My concern about the approach here is that it's very specialised:

It only covers the case where you're calling one of the *JsonAsync methods, and not if you're calling say GetStringAsync or just SendAsync
It only covers setting the credentials arg on fetch, and only to a set of predefined values. It's likely that people will want to configure other aspects of fetch too (such as mode or referrer or integrity - see https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/fetch for a list of current options, which will no doubt expand in the future). This would end up creating an explosion of method overload combinations on the *JsonAsync methods.

If I'm understanding the design of HttpClient correctly, it appears that HttpRequestMessage.Properties is designed as an arbitrary bag of additional HttpMessageHandler-specific metadata. As such the approach I'd prefer would be:

Declare some static readonly string on BrowserHttpMessageHandler such as public readonly static string FetchArgs = "BrowserHttpMessageHandler.FetchArgs";
Update the interop code so that BrowserHttpMessageHandler supplies the value request.Properties[FetchArgs] to the JS-side code
Update the JS-side code so that if the supplied "fetchArgs" value is nonnull, its properties are all added to the options object passed when we call fetch

Then, developers can construct a HttpRequestMessage and set:

myMessage.Properties[BrowserHttpMessageHandler.FetchArgs] = new
{ 
    credentials = "include",
    integrity = "sha256-BpfBw7ivV8q2jLiT13fxDYAe2tJllusRSZ273h2nFSE=",
    ... other options ...
};

... and then the JS-side code will call:

fetch(url, { ... existing args ..., credentials: 'include', integrity: 'sha256-BpfBw7ivV8q2jLiT13fxDYAe2tJllusRSZ273h2nFSE=', ... other options ... })...

... which allows a way to control all aspects of fetch, including future APIs that we don't even know about yet. It also avoids any arbitrary distinction between *JsonAsync and the regular *Async methods on HttpClient.

Drawback: I know this means you'd need to call SendAsync and do your own JSON-serialization, but I don't know of a way to avoid that while remaining consistent with the broader design of HttpClient. Developers who commonly need to specify particular custom arguments with their HTTP calls can create their own wrapper around SendAsync that takes whatever custom arguments (e.g., Credentials, Mode, Referrer, Integrity, ...) that they need. We can't really put credentials directly onto GetJsonAsync in the core library here for the reasons described above, i.e., it's not consistent with HttpClient design and wouldn't scale to other flags and options that you might want to pass through to fetch.

What do you think?

aguacongas · 2018-03-29T15:35:46Z

Hi @SteveSandersonMS ,

Just, why public readonly static string FetchArgs = "BrowserHttpMessageHandler.FetchArgs"; and not public const string FetchArgs = "BrowserHttpMessageHandler.FetchArgs"; ?

And does it means the user cannot use GetJsonAsync methods if it need to use some fetch options then, or any others HttpClient methods not accepting HttpReqestMessage ?
Or is the user have to create its BrowserHttpMessageHandler to override each message with its fetch options ?

SteveSandersonMS · 2018-03-30T09:01:12Z

@aguacongas

Re const vs static - either is fine here really.

And does it means the user cannot use GetJsonAsync methods if it need to use some fetch options then

In the general case, it means the user needs to construct a HttpRequestMessage and pass it to SendAsync. This allows for any HTTP method, any combination of headers, any payload (JSON or otherwise), etc.

I know there will be times where it's not as convenient as using a higher-level method like GetJsonAsync but as mentioned above, developers can create their own wrapper methods around this if they have specific needs (such as setting the credentials option, or integrity, or anything else).

Basically this is a relatively safe low-level design (not going to trap us in the future) that others can build higher-level methods on top of.

According to the discussion

aguacongas · 2018-03-31T17:36:35Z

@SteveSandersonMS

I uptated according to the discussion. I didn't update the test layout but if works for one options it should for others.

SteveSandersonMS · 2018-04-02T13:54:24Z

src/Microsoft.AspNetCore.Blazor.Browser.JS/src/Services/Http.ts

-registerFunction(`${httpClientFullTypeName}.Send`, (id: number, method: string, requestUri: string, body: string | null, headersJson: string | null) => {
-  sendAsync(id, method, requestUri, body, headersJson);
+registerFunction(`${httpClientFullTypeName}.Send`, (id: number, method: string, requestUri: string, body: string | null, headersJson: string | null, fetchArgs: RequestInit | null) => {
+    sendAsync(id, method, requestUri, body, headersJson, fetchArgs);


Could you please fix the indentation to be consistent with project conventions for .ts files? (e.g., 2 spaces per indentation level)

SteveSandersonMS

Thanks for this, @aguacongas! Looks like there's a few things remaining to be done before this PR could be merged, but the general approach looks good :)

SteveSandersonMS · 2018-04-02T13:56:35Z

src/Microsoft.AspNetCore.Blazor.Browser/Http/BrowserHttpMessageHandler.cs

+        public const string FetchArgs = "BrowserHttpMessageHandler.FetchArgs";
+
+        public BrowserHttpMessageHandler()
+        { }


Redundant empty constructor can be removed

SteveSandersonMS · 2018-04-02T13:57:08Z

src/Microsoft.AspNetCore.Blazor.Browser/Http/BrowserHttpMessageHandler.cs

@@ -36,17 +42,31 @@ public class BrowserHttpMessageHandler : HttpMessageHandler
                _pendingRequests.Add(id, tcs);
            }

+            request.Properties.TryGetValue(FetchArgs, out object fetchArgs);


Please use out var fetchArgs for consistency with project conventions

SteveSandersonMS · 2018-04-02T13:57:32Z

src/Microsoft.AspNetCore.Blazor.Browser/Http/BrowserHttpMessageHandler.cs


            return await tcs.Task;
        }

+        private static string GetDescription(Enum value)


I don't think we need this method any more

SteveSandersonMS · 2018-04-02T13:59:33Z

test/testapps/TestServer/Controllers/PersonController.cs

@@ -32,6 +33,23 @@ public IEnumerable<string> Get()
            }
        }

+        [HttpPost("xhrf")]


Are these methods GetAntiForgery and PostAntiForgery used anywhere? Could you clarify their purpose?

I added these methods to test antiforgery cookies submition, the e2e test is missing. I'm going to add it

SteveSandersonMS · 2018-04-02T14:00:00Z

test/testapps/TestServer/Startup.cs

@@ -17,6 +19,13 @@ public Startup(IConfiguration configuration)
        // This method gets called by the runtime. Use this method to add services to the container.
        public void ConfigureServices(IServiceCollection services)
        {
+            services.AddLogging(builder =>


Doesn't seem related to the rest of the PR - please remove

SteveSandersonMS · 2018-04-02T14:01:14Z

test/testapps/TestServer/Startup.cs

                        .WithExposedHeaders("MyCustomHeader");
                });
            });
+
+            services.AddAntiforgery(options =>


I'm guessing this is meant to be used from an E2E test somewhere, but there doesn't appear to be one included in the PR. Is there still something to be finished here?

yes the e2e test is missing.

I tried to add an e2e test but cookies aren't recieved when selenium runs the test. If I run the test manualy, it works, but not with selenium. The code is :

[Fact] public void CanPerformPostRequestWithFetchCredentials() { new SelectElement(Browser.FindElement(By.Id("request-credentials"))) .SelectByValue("include"); IssueRequest("GET", "/api/person/xhrf"); Assert.Equal("OK", _responseStatus.Text); var token = _responseBody.Text; AddRequestHeader("X-XSRF-TOKEN", token); IssueRequest("POST", "/api/person/xhrf", token); Assert.Equal("OK", _responseStatus.Text); Assert.Equal($"You posted: {token}", _responseBody.Text); }

Any idea ?

I'm not sure about that. Maybe Selenium requires additional config to enable cookies.

Instead of doing an E2E test about XSRF tokens, would it be simpler for the E2E test to cover a simpler fetch option such as referrer?

SteveSandersonMS · 2018-04-04T09:15:24Z

Thanks @aguacongas for finishing this off! It's now merged.

aguacongas · 2018-04-04T11:36:21Z

@SteveSandersonMS You're welcome. When do you plan to released 0.2.0 ?

danroth27 · 2018-04-05T03:59:15Z

When do you plan to released 0.2.0 ?

We're planning to ship sometime next week.

aguacongas added 4 commits March 25, 2018 15:12

#357 Send cookies

578fa33

#357 Replace string 'include' by enum

b8f1975

#357 implements with request credentials enum

d5ad538

#357 single quote

937f687

Merge remote-tracking branch 'upstream/dev' into dev

8bd97d5

SteveSandersonMS and others added 2 commits March 31, 2018 19:17

Add extension methods for HTTP JSON requests/responses

3c51328

#357 Implements fetch options

33e2397

According to the discussion

#357 remove debug logs

7d2ffb8

SteveSandersonMS reviewed Apr 2, 2018

View reviewed changes

aguacongas added 3 commits April 4, 2018 09:45

#357 cleanup and add fetch args e2e test

47f5700

#357 cleanup and add fetch args e2e test

21a940d

#357 fix indentation

17431ba

SteveSandersonMS closed this Apr 4, 2018

danroth27 mentioned this pull request Apr 19, 2018

Add topic on calling web APIs from Blazor aspnet/Blazor.Docs#71

Closed

guardrex mentioned this pull request Feb 1, 2019

Call web APIs from Blazor topic dotnet/AspNetCore.Docs#10743

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Request credentials #399

Implement Request credentials #399

aguacongas commented Mar 27, 2018

dnfclas commented Mar 27, 2018 •

edited

Loading

SteveSandersonMS commented Mar 29, 2018 •

edited

Loading

aguacongas commented Mar 29, 2018 •

edited

Loading

SteveSandersonMS commented Mar 30, 2018

aguacongas commented Mar 31, 2018

SteveSandersonMS Apr 2, 2018

SteveSandersonMS left a comment

SteveSandersonMS Apr 2, 2018

SteveSandersonMS Apr 2, 2018

SteveSandersonMS Apr 2, 2018

SteveSandersonMS Apr 2, 2018

aguacongas Apr 2, 2018

SteveSandersonMS Apr 2, 2018

SteveSandersonMS Apr 2, 2018

aguacongas Apr 2, 2018

aguacongas Apr 2, 2018

SteveSandersonMS Apr 2, 2018

aguacongas Apr 4, 2018

SteveSandersonMS commented Apr 4, 2018

aguacongas commented Apr 4, 2018

danroth27 commented Apr 5, 2018

Implement Request credentials #399

Implement Request credentials #399

Conversation

aguacongas commented Mar 27, 2018

dnfclas commented Mar 27, 2018 • edited Loading

SteveSandersonMS commented Mar 29, 2018 • edited Loading

aguacongas commented Mar 29, 2018 • edited Loading

SteveSandersonMS commented Mar 30, 2018

aguacongas commented Mar 31, 2018

Choose a reason for hiding this comment

SteveSandersonMS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveSandersonMS commented Apr 4, 2018

aguacongas commented Apr 4, 2018

danroth27 commented Apr 5, 2018

dnfclas commented Mar 27, 2018 •

edited

Loading

SteveSandersonMS commented Mar 29, 2018 •

edited

Loading

aguacongas commented Mar 29, 2018 •

edited

Loading