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

$.ajaxPrefilter hooking strategy too global? #270

Closed
jayphelps opened this Issue Aug 8, 2014 · 26 comments

Comments

Projects
None yet
8 participants
@jayphelps
Copy link

jayphelps commented Aug 8, 2014

Since ember-simple-auth uses $.ajaxPrefilter, literally any calls that end up using jQuery.ajax will trigger the prefilter callback and run against any Authorizer defined.

This steps on the toes of other libraries and code paths that rely on jQuery's AJAX helpers, as well as makes it impossible to conditionally authorize certain models different ways (or selectively not authorize some).

Any alternatives?

@marcoow

This comment has been minimized.

Copy link
Member

marcoow commented Aug 8, 2014

Using $.ajaxPrefilter is the only way I see to make sure that Ember Simple Auth can authorize every request. You can however opt-out of actually including e.g. the Authorization header by specifying your own authorizer (see e.g. the custom server example).

@marcoow marcoow added the question label Aug 8, 2014

@jayphelps

This comment has been minimized.

Copy link
Author

jayphelps commented Aug 8, 2014

@marcoow That's what I'm doing now and that's where it's biting me, actually. Third party library calls are hitting my Authorizer and I'm not sure of a solid approach of ignoring them? I also need to be able to conditionally authorize certain models but not others. (dealing with an existing backend that has issues if I provide authorization info on calls that don't need it....)

@marcoow

This comment has been minimized.

Copy link
Member

marcoow commented Aug 8, 2014

I think you'd have to base the decision on the URL pattern that the XHR is going to.

@jayphelps

This comment has been minimized.

Copy link
Author

jayphelps commented Aug 8, 2014

Yikes. What about a way of overriding the default $.ajaxPrefilter behavior via a hook? That way I can provide my own "tunnel" to route ember-data requests through, leaving jQuery alone.

@jayphelps

This comment has been minimized.

Copy link
Author

jayphelps commented Aug 8, 2014

A similar issue I ran into a couple months ago was that any jQuery-based ajax request that returns 401 triggers authorizationFailed.

@marcoow

This comment has been minimized.

Copy link
Member

marcoow commented Aug 8, 2014

Not sure what you mean with "hook". Can you lay out the idea a bit more?

@marcoow

This comment has been minimized.

Copy link
Member

marcoow commented Aug 8, 2014

Can you open a separate issue for the authorizationFailed problem? I think it's correct that the current behavior isn't perfect as it should only trigger authorizationFailed for requests that were authorized by the authorizer.

@jayphelps

This comment has been minimized.

Copy link
Author

jayphelps commented Aug 8, 2014

Sorry, hook is indeed ambiguous. Basically, I'm thinking about, at the very least, extracting this default behavior into an publicly exposed method or function that can be overridden to change it's behavior.

ember-data users could then opt-out of this and have their Adapter lookup the authorizer and do this work, leaving everyone else alone. An ember-data Adapter mixin could even be provided to facilitate this.

I'm out of cycles right now from a long day...so I'll give this some more thought tomorrow and maybe I'll be able to explain it clearer.

@marcoow

This comment has been minimized.

Copy link
Member

marcoow commented Aug 8, 2014

I think I get where you're heading to. Would be great if you could elaborate on the idea a bit more tomorrow then.

I think it's important that the default behavior of authorizing all request (regardless of where they're coming from, e.g. Ember Data, direct calls to $.ajax or whatever) doesn't require any additional setup. Also I'm not really sure yet why Ember Data users would want to opt out of the global hook.

@marcoow

This comment has been minimized.

Copy link
Member

marcoow commented Aug 28, 2014

@jayphelps: any updates on this? What you write above sound interesting although I'm not 100% sure where this is going ;)

@jayphelps

This comment has been minimized.

Copy link
Author

jayphelps commented Sep 1, 2014

@marcoow I still have some thoughts on it but I recently tried to upgrade to the latest simple-auth and found things have been completely restructured so I need to get up to speed first.

@vladikoff

This comment has been minimized.

Copy link
Contributor

vladikoff commented Oct 19, 2014

@jayphelps I agree that the $.ajaxPrefilter solution feels like a "global" hack and is bugging me. If ember-data is used then session should probably be injected into the RESTAdapter where headers can be properly set.

@marcoow

This comment has been minimized.

Copy link
Member

marcoow commented Oct 21, 2014

@vladikoff: Adding the auth header via the Ember Data adapter configuration API is basically the same thing (only difference is that it will only affect Ember Data requests not the ones people might be making directly with $.ajax etc.)

The nice thing is that $.ajaxPrefilter just works automatically and doesn't require any manual setup. Also I couldn't really think of a use case where it would be a problem if requests that are not made with Ember Data (or epf or whatever) have the authorization header as well.

I don't want to stop this conversation of course though, just wouldn't want to introduce additional complexity.

@jayphelps

This comment has been minimized.

Copy link
Author

jayphelps commented Oct 22, 2014

Great point about it "just working". I would like this behind a config flag of some sort though so we can opt-out and handle it in our adapter. I can't remember why, but this was indeed causing issues with our API; I think it was something to do with the way it handles caching auth requests vs. non-auth.

I still haven't upgraded to a newer version so can't say much more til I find the time.

@ur5us

This comment has been minimized.

Copy link

ur5us commented Feb 22, 2015

@jayphelps with a cookie based approach your session data is send on every request as well so I tend to agree with @marcoow that after being authenticated the auth headers should be send on every request and it shouldn't matter whether it's EmberData or pure jQuery /cc @vladikoff. Moreover, if auth headers break your caching strategy, that's probably app specific, no. Just my 2 cents

@lfridael

This comment has been minimized.

Copy link

lfridael commented Jul 9, 2015

Tying the library to JQuery feels restrictive.

What about apps that use https://github.com/visionmedia/superagent for Ajax operations? And what about future use cases where JQuery won't be available, such as during a Fastboot page render on the server?

@marcoow

This comment has been minimized.

Copy link
Member

marcoow commented Jul 9, 2015

Very true, especially with Ember becoming more and more independent of jQuery it would be cool to have a pluggable strategy in ESA. Since that this would most likely be a breaking change this should be done 1.0 though.

Suggestions welcome - I don't really have much time right now as I'm working on getting #522 ready.

@kimroen

This comment has been minimized.

Copy link
Contributor

kimroen commented Jul 15, 2015

There's some relevant discussion happening over in the rfc-proposal for replacing ic-ajax with ember-fetch in ember-cli.

@fsmanuel

This comment has been minimized.

Copy link
Contributor

fsmanuel commented Jul 15, 2015

What about adding a public function like shouldAuthorizeRequest that gives you the context and you can decide if authorization is required or not? The default should be true but you can overwrite it.

@marcoow

This comment has been minimized.

Copy link
Member

marcoow commented Jul 15, 2015

The problem is actually more about how request authorization is triggered at all. Currently the library uses $.ajaxPrefilter but when you're e.g. using the fetch API that simply doesn't exist so no requests would be authorized at all.

I think the best solution for now is probably to extract the $.ajaxPrefilter code into a dedicated initializer that could then be swapped for another one later on when e.g. using fetch. I guess the only question is whether that initializer would be loaded automatically (which would require a breaking change to make it optional and swappable later on) or whether users should be forced to manually call it with the 1.0 release.

@marcoow

This comment has been minimized.

Copy link
Member

marcoow commented Jul 20, 2015

will be fixed in #522

@marcoow marcoow closed this Jul 20, 2015

@jayphelps

This comment has been minimized.

Copy link
Author

jayphelps commented Jul 20, 2015

IMO it's odd to close a ticket that is not yet fixed.

@marcoow

This comment has been minimized.

Copy link
Member

marcoow commented Jul 20, 2015

yeah, that's true actually - was a bit over eager cleaning up the tickets…

@marcoow marcoow reopened this Jul 20, 2015

@abradner

This comment has been minimized.

Copy link

abradner commented Jul 21, 2015

I'm joining the party here late, I know, but I had an idea that might interest you:

First, it seems like we're trying to have an ideal solution for two opposing ideas:

  1. Have a drop-in, zero config auth toolbelt that just works
  2. Have an extensible auth platform that can handle all everything you throw at it.

Having both of these in the core library may well be either impossible or very convoluted, but the Ember team solved a similar problem fairly elegantly: Addons.

Under this model, the Authorization header can be opt-in under the core framework, but with another addon, say ember-simple-auth-automatic, users can get the current behaviour for free. This could be the recommended path for new users or even an enabled default in a vanilla install.

There are a lot of good reasons for having the auth header as opt-in, mostly to do with ajax calls to a third party, eg directly consuming Google's APIs while also using your own OAuth backend.

By being explicit when making a call, you can:

  • support interaction with multiple simultaneous OAuth providers in the future with little effort
  • ensure that the right tokens get sent to the right places
  • ensure you're not accidentally leaking your access tokens to a potentially malicious third party (and this could be a BIG trap).

As for the explicit calls, instances of $.ajax() could be trivially replaced with this.get('session.myAuthProvider').ajax()
and in true ember/semver form, that could be a feature introduced in the 1.x stream (initially with a feature flag while it's under development) with deprecation warnings when you directly use $.ajax(), then finally extracted into the addon for 2.0, keeping everyone happy.

@marcoow

This comment has been minimized.

Copy link
Member

marcoow commented Jul 21, 2015

Having the current automatic setup of $.ajaxPrefilter in a legacy compatibility addon is a great idea - added to #522.

@marcoow

This comment has been minimized.

Copy link
Member

marcoow commented Oct 16, 2015

$.ajaxPrefilter has been removed in 1.0

@marcoow marcoow closed this Oct 16, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.