heartsentwined / ember-auth

Authentication framework for ember.js.
http://ember-auth.herokuapp.com/
397 stars 43 forks source link

Authenticated-only routes don't work for routes that load model data #29

Closed seanrucker closed 11 years ago

seanrucker commented 11 years ago

The model hook on the route gets called before the redirect hook. At that point the user is not authenticated and the call to load the model fails. Any thoughts on how to get around this?

heartsentwined commented 11 years ago

Is that a change introduced by a newer ember version or something? Because this snippet from ember-auth-rails-demo works fine:

EmberAuthRailsDemo.UsersIndexRoute = Auth.Route.extend
  model: ->
    EmberAuthRailsDemo.User.find()
heartsentwined commented 11 years ago

Or like, worked fine; I haven't had time to update the demo to reflect 3.x branch yet.

seanrucker commented 11 years ago

I'm not sure when it was introduced. Here's an example of what I'm talking about:

App.IndexRoute = Auth.Route.extend({

  redirect: function() {
    console.log('redirect');
    this._super();
  },

  model: function() {
    console.log('model');
  },

  setupController: function() {
    console.log('setupController');
  }

});

When visiting that route the console spits out:

model redirect setupController

If you try to load something in the model hook you won't be authenticated yet and you will get an error.

There seems to be lots of discussion going on around this very issue here: https://github.com/emberjs/ember.js/issues/2041

heartsentwined commented 11 years ago

While we can surely wait for emberjs/ember.js#2041 to sort out the order that they want (otherwise if we fix this, and then they change the order, we'll start all over again), I was wondering if there is an even earlier / lower level hook that we can move auth logic to?

seanrucker commented 11 years ago

Lets also keep an eye on these:

https://github.com/emberjs/data/issues/838 https://gist.github.com/stefanpenner/9ccb0503e451a9792ed0

heartsentwined commented 11 years ago

As you have discovered, this relates to a deeper issue within ember itself. They are looking at activate/deactivate, enterFilter/exitFilter, reordering of those hooks, or others, and we're not sure which will come up.

Ref:

The current auth logic in router is the best temporary solution I can find for the current state. It is supposed to be already friendly to userland code, expecting:

App.FooRoute = Auth.Route.extend
  redirect: ->
    # logic here
    @_super()
    # or here, depending on needs

The authentication part of ember-auth is, itself, another "step" to perform at an ember app load. Due to ember's current architecture, all such steps (e.g. load model, load view, etc) are performed at a hard-coded order. It follows also that, whichever sequence that we decide on ember-auth will clash with some app use cases.

The problem, though, is that the sequence you described looks most logical, but is not implemented in ember.

heartsentwined commented 11 years ago

One solution that I can think of, is to extract the current Auth.Route logics (including your url auth one) into mixins. I am thinking of API like:

App.FooRoute = Em.Route.extend Auth.Mixin, Auth.Module.RememberMe,
  redirect: ->
    @authLogic()
    @rememberLogic()
  model: ->
    @rememberLogic()
    @authLogic()

where you can customize where, and in what order, they are executed. Auth.Route itself can be loaded with the mixins at their existing order behind the scenes.

heartsentwined commented 11 years ago

Took another stab at the issue today, and I got a simple workaround:

App.SecretRoute = Auth.Route.extend
  model: ->
    if Auth.get 'authToken' # only load model if user is logged in
      App.Secret.find()

In my trial setup, the redirect hook is indeed not working when the route tries to load an auth-only model (and fails). Wrapping it in a conditional to prevent a failing model load (seems to) fix the issue, for now.

Give it a try and let me know if it's working for you.

dcalhoun commented 11 years ago

@heartsentwined your last proposed solution seems to work for me for the time being.

seanrucker commented 11 years ago

This workaround falls apart if you are already authenticated and have a remember-me token, or if you are passing the authentication params in the URL.

For example, take the URL shown in the README for URL authentication: http://www.example.com/?auth[remember]=1&auth[key]=fja8hfhf4/#/posts/5

When you access that URL the route will first invoke the model hook. The model will not be loaded because the user is not authenticated yet. After that the redirect hook will be invoked and ember-auth will authenticate the user through the URL Authentication logic (and/or Remember Me logic). BUT the model hook is never invoked again after the user is successfully authenticated; therefore the model is never loaded.

The solution I can see at this time is to move the URL Authentication logic and the Remember Me logic into the model hook. The redirect logic should stay in the redirect hook.

I like your idea of moving the logic into mixins — seems cleaner than subclassing the Auth.route and using if-statements based on configuration params.

seanrucker commented 11 years ago

This seems to do the trick: https://github.com/heartsentwined/ember-auth/pull/41

heartsentwined commented 11 years ago

TL;DR

The current redirection logic is flawed and doomed to fail. (But a revamp is in the pipelines.)

Status

41 is close, but introduces unintended behavior. (See reason over there, and details below.) Otherwise, I have got a fix now, as soon as I am sure it works with other components, I'll push it upstream.

Technical Details

(Mostly for @seanrucker, since you are quite active and interested in getting this fixed.)

The redirection issue relates to four issues / bugs:

  1. model hook vs redirect hook call sequence
  2. ember-data will refuse to re-load a model if already tried (and got an error)
  3. resolving routes with dynamic segments, hence, dynamic models
  4. deserializing a route on entry vs transitioning to a route afterwards

Route hook call sequence

(1) is known. The best solution is hope that it got fixed, but since we're not sure of this yet, the best bet is to find a temporary hack. However, even if the call sequence is fixed, the way ember calls these hooks will make it impossible to stop a hook. Here is what ember does:

Call each hook separately, starting from parents to children. This applies to init, redirect, setupController, activate, deactivate, enter, and exit. Yes, I have dugged up and tested all of them.

Read that again - all the defined hooks are called separately. Including redirect. That is, even if we transitionTo another route within ember-auth, a userland redirect hook will still be called, although our transitionTo will take effect.

The model hook gets special treatment though, by default, only the child hook at the lowermost of the inheritance sequence will be called. This makes sense, since we only want to load one set of model(s) at any given route. However, this requires a _super() call if we put any logic (not just redirection) in our model hooks, as you have discovered in #41.

I'm not sure whether the redirect hook behavior is the intended one though, but #41 seems to rely on this special treatment (and avoiding the (un)intended behavior of the redirect hook). In that it seeks to avoid userland hook callbacks, by using the only hook that does not "chain" by default.

Model loading

ember-data will only try to load a particular model once from the server. If it fails, it will assume that subsequent tries will fail likewise, hence will stop trying. This makes perfect sense though, one should not repeatedly try to load 404 models, for instance.

Theoretically, if we have appended an auth token to the query string, then this should be considered a different request from the bare one. However, ember-data does not make this check.

For protected models though, it means we must stop ember-data from trying to load the model before getting authenticated. This is actually not particular to ember-auth, but to all ember apps with protected models.

However, since we cannot "wrap" the whole child model hook within our Auth.authToken conditional from ember-auth code, and since the model hook is still called before redirect, it appears that userland check of Auth.authToken is inevitable at present.

Routes with dynamic segments

The current setup only works with routes without dynamic segments, because it simply stores the routeName, and then transitions to it. However, all transitionTo, replaceWith, transitionToRoute calls are designed to work with dynamic segments by explicitly supplying the dynamic model(s). So the current code blows up on these.

Where do we get the models? This is, unfortunately, a strictly userland concern and there is no standard method for retrieving a model. We could make userland code supply us with a model callback, but this would certainly be code duplication along with, for example, the model hook.

How about retrieving this model hook and calling it directly? Unfortunately, this fails when such a hook is not explicitly defined in userland code. Ember defaults to Model.find(model_id) if a model hook is undefined; however, this default is heavily wrapped and hardcoded within the routing mechanism (check the ember-routing package for details). If ember-auth simply tries to retrieve a model hook, it will get undefined for these implicit ember calls; ember will not return a "default" hook.

There is also the issue of multiple dynamic segments - hence models - although this is easily fixed with apply().

Deserializing a route vs transitioning to a route

If you have been pondering on the above paragraph, you might have wondered where the params argument of the model hook has gone to, i.e. we are supposed to call the route's model hook with model(params), not a naked model(). It turns out that, even this params argument is heavily wrapped within the ember routing system. If you trace its origin, you will end up with a RouteRecognizer, which is completely internal and cannot be retrieved whatsoever. Dead end.

Why this matters is that ember treats an initial route entry and a subsequent transition to route differently. The former involves deserializing the URL into routes; while the latter is work within the StateManager. Potentially relevant implications / behavior observations for us:

  1. the model hook is only called on deserializations [init]
  2. the model hook takes a param argument [init]
  3. transitionTo (and friends) are only called on transitions [subsequent]
  4. transitionTo (and friends) take a variable amount of model instance arguments [subsequent]
  5. init is called in both cases [both]
  6. redirect is called in both cases, where relevant [both]

Implications that has affected me when trying to find a solution:

  1. Trying to record state (prevRoute etc) in transitionTo will fail for init
  2. Likewise model will fail for subsequent hooks
  3. Even worse, if we define a model hook, even if it does nothing, it will disable the default Model.find(model_id) behavior
  4. If we try to store the param argument (just a plain object), it will fail when applied to transitionTo
  5. In any case we cannot get the param argument
  6. Likewise the other way round

I have got a solution around this, basically hooking onto ember's *Location implementations and calling the router's handleURL() for deserializing cases; and hooking onto transitionTo (and friends) for the model instances. I will push it once I get it to work with other components. Should be up within two or three days.

This is a long write-up, but it's here for your reference, and you might find a better solution than I did.

seanrucker commented 11 years ago

Thanks for the detailed explanation! I'm looking forward to the solution you have in the works.

heartsentwined commented 11 years ago

@seanrucker almost fixed in 5.x, the part that I couldn't get rid of is the authenticated-conditional boilerplate. But this is due to emberjs/ember.js#2041, as you have observed, not ember-auth per se.

A minimal route with authenticated-only model is thus:

App.SomeRoute = Em.Route.extend
  model: ->
    # condition necessary
    # if SomeModel requires authentication to access
    if App.Auth.get('signedIn')
      SomeModel.find()

Two things in this example:

BC breaks in 5.x version is rather dramatic, hence I haven't written an upgrade guide yet. The docs should help you upgrade; otherwise please feel free to ask me how to convert a specific usage.

heartsentwined commented 11 years ago

Closing since remaining issue belongs to emberjs/ember.js#2041.