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

feat: support for with-credentials xhr flag #498

Closed
wants to merge 4 commits into from

Conversation

n3squik
Copy link

@n3squik n3squik commented Mar 14, 2019

Add support for xhr's with-credential flag (specs).

A new optional parameter is added to clients constructors. If not set the default value is false, so it does not change the prior behavior.

Usage:

const WITH_CREDENTIALS = true;
const client = new SomeServiceClient('https://www.some-domain.com:8443', WITH_CREDENTIALS);

Related issues: #176, #351

@jmzwcn
Copy link

jmzwcn commented Mar 16, 2019

seems this flag is unnecessary, give your credentials-value is enough directly.

@n3squik
Copy link
Author

n3squik commented Mar 16, 2019

seems this flag is unnecessary, give your credentials-value is enough directly.

@jmzwcn could you elaborate a bit? where can I pass my credentials-value directly?

@stanley-cheung
Copy link
Collaborator

This is necessary. The xhr.with-credentials flag has special meaning in the browser, which will include domain cookies to be attached to the request. We just never got around to adding an API for it.

@jmzwcn
Copy link

jmzwcn commented Mar 19, 2019

Hope to avoid non-compatible and reduce too many parameters to easily use.

@stanley-cheung
Copy link
Collaborator

@n3squik Thanks for your contributions. I might have to think about this a little more, especially around how exactly we are gonna do this. For the internal use case, we don't have the extra credentials parameter in the constructor. We tried to detect whether certain metadata is being set and that indicates that we also want to .setWithCredentials(true).

The challenge in here (the open-source use case) is that it's hard to define what that condition might be. We might have to do it with setting a specific parameter in the options parameter in the constructor. I really want to avoid adding another parameter the stub client constructor without an absolutely necessary reason.

Let's keep this PR open - I will come back on this asap.

@andrewtikhonov
Copy link

andrewtikhonov commented Apr 10, 2019

withCredentials: true is needed for browsers to trigger authentication as a result of XMLHttprequest. If not set, the browser is silent. Currently it's not possible to properly trigger any browser driven auth as a result of gRPC call, neither Negotiate (NTLM, Kerberos) nor Basic.

Something like below should be enough if you don't want to change the arguments.

    if (options.withCredentials) {
        xhrIo.setWithCredentials(true);
    }

@Kupstas
Copy link

Kupstas commented May 22, 2019

Any new information?

@AaronTriplett
Copy link

Checking in on this, currently blocked. Any chance we can get this merged?

@stanley-cheung
Copy link
Collaborator

Done in #604. Thanks for your contributions anyways. Sorry for the huge delay.

@stanley-cheung
Copy link
Collaborator

Before cutting the next release, is it possible that someone can verify if this works for your use case?

var client = new EchoServiceClient('localhost', null, {
  'withCredentials': true
});

I received a report that even with #604, cookie is not being attached to a cross-domain request. So I was wondering if we are still missing something.

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

Successfully merging this pull request may close these issues.

None yet

6 participants