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

Credentials are always set as 'same-origin' #6413

Closed
optikalefx opened this issue Sep 6, 2019 · 11 comments
Closed

Credentials are always set as 'same-origin' #6413

optikalefx opened this issue Sep 6, 2019 · 11 comments

Comments

@optikalefx
Copy link

Reproduction

https://github.com/emberjs/data/blob/master/packages/adapter/addon/rest.js#L1356

Description

Our authentication requires the use of credentials: 'include'. Which works fine with fetch normally. However using Ember Data it is setting credentials to same-origin no matter what you pass it. It's not possible to override just this setting.

The only option we have is to overwrite the entire ajaxOptions function. Which I've done, and it works, but it's scary since ajaxOptions does so much. It would be much safer to be able to pass this option in, and ED optionally use it.

Versions

"ember-source": "~3.12.0"
"ember-cli": "~3.12.0"
"ember-data": "~3.12.0"
@optikalefx
Copy link
Author

I've also had to copy-paste the following functions from ED source in order to over-write ajaxOptions properly.

isPlainObject, add, serializeQueryParams and fetchOptions

@Exelord
Copy link

Exelord commented Sep 7, 2019

I confirm. Just noticed this issue. But from what I debugged the issue is not with config but with fetch polyfill.

if you override _fetchRequest to 👍

_fetchRequest(options) {
    return fetch(options.url, options);
  },

it works :)

@snewcomer
Copy link
Contributor

Is overriding ajaxOptions in your adapter like so an acceptable solution for you @optikalefx?

ajaxOptions(url, method, options) {
  const options = this._super(...arguments);
  options.credentials = 'include';
}

@Exelord
Copy link

Exelord commented Sep 8, 2019

This doesn't work as well. The issue is in ember-fetch polyfill

@snewcomer
Copy link
Contributor

snewcomer commented Sep 8, 2019

@Exelord 👍 I'm assuming for the original _fetchRequest, in my example above, it is safe to assume the correct options with credentials are passed through to the applicable fetch function? However, it seems you are saying either ember-fetch (which is a simple addon) or the the whatwg-fetch polyfill isn't parsing these options?

For clarity, do you have preferNative turned on - https://github.com/ember-cli/ember-fetch#allow-native-fetch?

@Exelord
Copy link

Exelord commented Sep 8, 2019

Oh sorry, I didn't explain it well. I have custom options

ajaxOptions(url, method, options) {
  const options = this._super(...arguments);
  return { ...options, credentials: 'include' };
}

and fetch override

_fetchRequest(options) {
    return fetch(options.url, options);
},

without the override, ember-fetch is called and it looks like it does not respect this config option.

No, I dont have preferNative flag on. Do you recommend to enable it?

@snewcomer
Copy link
Contributor

Oh good! If preferNative also results in the same behaviour, then we can assume it is a general problem. If credentials: 'include' works with preferNative: true in ember-cli-build, then it might be the polyfill that we need to look at.

Another note - fetch request headers are read only after they are set. Probably doesn't matter here though.

@optikalefx
Copy link
Author

Couple things. This does work well for my case.

ajaxOptions() {
		return {
			...this._super(...arguments),
			credentials: 'include'
		};
	},

Without the modifying _fetchRequest.

However, in making this switch away from jQuery and ember-ajax over to fetch. The content-type header is wrong, and makes PATCH and POST fail to send data. Rather, your server would need to be configured to read the vnd+json content-type.

So in addition to over-ridding ajaxOptions to send credentials. I had to to override the header for content-type to be application/json. It seems ember-fetch or somewhere in there is overriding the content-type header, and doing so in a case sensitive way.

If you have the header 'Content-Type': 'application/json', it fails. ember-fetch is adding, and therefore you have to over-ride with 'content-type': 'application/json',

@snewcomer
Copy link
Contributor

@optikalefx That is great to hear! Also note I merged a fix to Content-Type headers in a few weeks ago. So hopefully it fixes what you were encountering - #6341

@Exelord
Copy link

Exelord commented Sep 9, 2019

With native fetch it works all fine. Its definietly polyfill issue. I also noticed that with a polyfill, received cookies are not "set" but they are sent with the request. They are like "invisible". So weird!

With native fetch all works fine.

allthesignals added a commit to NYCPlanning/labs-applicant-portal that referenced this issue Apr 7, 2020
This commit forces fetch to include credentials. There is an issue with allowing credentials with new version of Ember Data. See:

See emberjs/data#6413
@runspired
Copy link
Contributor

these sorts of issues are always difficult work arounds in adapters today. The new RequestManager streamlines this and gives full access to the request and response: emberjs/rfcs#860

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

No branches or pull requests

4 participants