heartsentwined / ember-auth

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

Auth Token not appended to url #115

Open flynfish opened 11 years ago

flynfish commented 11 years ago

I just upgraded to .9.0.6 and when I log in, the sign in endpoint is call correctly, but when it transitions to my signInRoute and tries to load the user endpoint the auth_token is not sent as a parameter:

Auth.js

App.Auth = Ember.Auth.extend({
  request: 'jquery',
  response: 'json',
  strategy: 'token',
  session: 'cookie',
  tokenKey: 'auth_token',
  tokenLocation: 'param',
  tokenIdKey: 'user_id',
  signInEndPoint: '/users/sign_in',
  signOutEndPoint: '/users/sign_out',
  modules: ['emberModel', 'actionRedirectable', 'authRedirectable', 'rememberable'],
  baseUrl: App.baseUrl,
  emberModel: {
    userModel: 'App.User'
  },
  actionRedirectable: {
    signInRoute: 'instances',
    signInSmart: true,
    signInBlacklist: ["signup", "login", "forgotPassword", "resetPassword"],
    signOutRoute: 'homepage'
  },
  authRedirectable: {
    route: 'login'
  },
  rememberable: {
    tokenKey: 'remember_token',
    period: 7,
    autoRecall: true
  }
});
App.AuthSignInView = Ember.View.extend({
  templateName: 'auth/sign-in',
  layoutName: 'auth/layout',
  email: null,
  password: null,
  remember: false,
  submit: function(event, view) {
    event.preventDefault();
    event.stopPropagation();
    return this.get("parentView.auth").signIn({
      data: {
        user_login: {
          email: this.get('context.email'),
          password: this.get('context.password'),
          remember_me: true
        }
      }
    }).then(function(){
      console.log("succesful login??");
    }, function(){
        //displays error message here

    });
  },
  didInsertElement: function(){
    this.$('input:first').focus();
  }
});

no_auth_token

heartsentwined commented 11 years ago

What is the ember-model version?

flynfish commented 11 years ago

ah, that's it. I am on 0.0.9 because I had a strange issue when I tried upgrading to 0.0.10 the other week.

heartsentwined commented 11 years ago

Blame ember-model either for changing the find-by-id api or for being buggy then :)

flynfish commented 11 years ago

I will blame it being buggy :) (but its probably something I am doing wrong instead)

flynfish commented 11 years ago

@heartsentwined I just upgraded to ember-model 0.0.10 and still have the same issue

flynfish commented 11 years ago

I've also tried using the latest ember-model from master branch, which has your commit but still doesn't work.

heartsentwined commented 11 years ago

huh? Has something changed in the adapters too?

heartsentwined commented 11 years ago

The integration model itself is pretty minimal, first it patches the Ember.RESTAdapter:

    Ember.RESTAdapter.reopen
      _ajax: (url, params, method, settings) ->
        super url, params, method, self.auth._strategy.serialize(settings || {})

Then it makes the find user call via App.Member.fetch(id):

  findUser: ->
    return unless @auth.userId? && (model = Em.get @config.userModel)
    model.fetch(@auth.userId).then (user) => @user = user

So it should be one of those going wrong...

flynfish commented 11 years ago

I'm not sure. I was using 0.0.9 before upgrading ember-auth and it worked fine. On Oct 15, 2013 6:27 PM, "heartsentwined" notifications@github.com wrote:

huh? Has something changed in the adapters too?

— Reply to this email directly or view it on GitHubhttps://github.com/heartsentwined/ember-auth/issues/115#issuecomment-26384194 .

flynfish commented 11 years ago

@heartsentwined I've tried to debug the issue but didn't really get anywhere. If I put a console.log() in the fetch callback in findUser nothing shows up in the console.

findUser: function () {
    var model, this$;
    if (!(null != get$(get$(this, 'auth'), 'userId') && (model = Em.get(get$(get$(this, 'config'), 'userModel')))))
      return;
    return model.fetch(get$(get$(this, 'auth'), 'userId')).then((this$ = this, function (user) {
      console.log(user); //nothing is printed to the console
      return set$(this$, 'user', user);
    }));
  },
flynfish commented 11 years ago

116 is related I think

ekampp commented 11 years ago

116 isn't quite related. In that issue, the promise from the signInEndpoint is never fulfilled. So it's unknown is the auth token is attached.

ekampp commented 11 years ago

I'm not seing the problem. It seems to append the auth token fine for me, using ember-data. Perhaps this is confined to the ember-model mixin, somehow?

heartsentwined commented 11 years ago

Got it. It is a priority problem. ember-model, unlike the other two persistence libs, requires you to declare an instance of the adapter alongside model declarations.

App.User = Ember.Model.extend({
  id: attr(),
  name: attr(),
  comments: hasMany("App.Comment", {key: 'comment_ids'})
});

App.User.url = "/users";
App.User.adapter = Ember.RESTAdapter.create(); // here

This creates an undefined behavior: if your adapters are created before ember-auth's emberModel module has a chance to initialize itself and patch the _ajax method, then it would only have ember-model's original _ajax method, which does not include session data injection.

A clearer debugging example. Let's say you decide to declare adapters in an initializer.

Em.onLoad 'Ember.Application', (application) ->
  application.initializer
    name: 'ember-model-adapter'
    before: 'ember-auth' # note this line

    initialize: (container, app) ->
      EmberAuthRailsDemo.User.adapter = Em.RESTAdapter.create()

This would reproduce your observation.

However, if you make it after ember-auth has finished initializing:

Em.onLoad 'Ember.Application', (application) ->
  application.initializer
    name: 'ember-model-adapter'
    after: 'ember-auth-load' # changed here!

    initialize: (container, app) ->
      EmberAuthRailsDemo.User.adapter = Em.RESTAdapter.create()

The _ajax method in the created adapter has now been patched.

I'm not sure how to solve this problem in a way that doesn't require users to do anything. It's pretty much the same reason behind the requirement of this ordering:

#= require jquery
#= require handlebars
#= require ember
#= require ember-model-latest
#
#= require_self
#
#= require_tree ./models
#= require_tree ./controllers
#= require_tree ./views
#= require_tree ./helpers
#= require_tree ./templates
#= require_tree ./routes
#= require ./router

window.EmberAuthRailsDemo = Em.Application.create()

The "pre-req"s, i.e. jquery, ember, etc, must be loaded before the create() call. The others, however, can wait, because they are only extended, so the code is not immediately executed. Ember's DI will ensure that they get executed only on app load.

heartsentwined commented 11 years ago

Indeed the same happened with ember-auth itself, pre 9.x. It required you to create() an auth instance immediately when you write the config, so the ordering was critical (right after the Em.Application.create(), before declarations of route, controller, etc). Now that ember-auth uses DI too, you can drop it in whatever order you want.

For this reason I think this should be an issue over at ember-model itself, i.e. to change the syntax to support extending adapters when declaring them. It's pretty much the same story (and limitations) between these two versions of ember-data:

# earlier versions
EmberAuthRailsDemo.Store = DS.Store.extend
  adapter: DS.RESTAdapter.create() # note the create
# later versions
EmberAuthRailsDemo.Store = DS.Store.extend
  adapter: DS.RESTAdapter.extend() # note the extend
NiQ-B commented 11 years ago

Was a little reluctant to piggyback on this open issue and if you need me to open a separate ticket I will, but I am having the same issue but with EPF.

DEBUG: ------------------------------- DEBUG: Ember : 1.2.0-beta.1+canary.7471b745 DEBUG: Handlebars : 1.0.0 DEBUG: jQuery : 2.0.3 DEBUG: EPF : 0.1.4 DEBUG: -------------------------------

lib load order //= require ember //= require ./lib/epf

//= require ember-auth //= require ember-auth-request-jquery //= require ember-auth-response-json //= require ember-auth-strategy-token //= require ember-auth-session-cookie

//= require ember-auth-module-epf

@auth.createSession({auth_token: '**'}) is working as expected and creating a session.

@auth.get('signedIn') returns -> true

flynfish commented 11 years ago

@heartsentwined How would you suggest we fix it? Do we have to wait till Ember-Model is updated? I tried adding the initializer like the example you gave:

Em.onLoad 'Ember.Application', (application) ->
  application.initializer
    name: 'ember-model-adapter'
    after: 'ember-auth-load' # changed here!

    initialize: (container, app) ->
      App.User.adapter = Em.RESTAdapter.create()

But this causes other problems. In the screenshot of my original post above, it used to call the User show endpoint, the instances endpoint and cloud_hosts endpoint. Now after adding the initializer it calls the instances endpoint 3 times and doesn't call the other endpoints. Also, the baseURL that I had set with port 4000 is no longer getting picked up so the instances endpoint being called has the wrong port:

XHR finished loading: "http://localhost:3000/instances?auth_token=pUpqbZo2ZApuBNfqpLxz".

heartsentwined commented 11 years ago

@flynfish I don't know what happened at Ember.Model's end, perhaps they need some bootstrapping themselves? Perhaps you can manually patch the Em.RESTAdapters that you need to create? From the module source code

patch: ->
  self = this
  Ember.RESTAdapter.reopen
    _ajax: (url, params, method, settings) ->
      super url, params, method, self.get('auth._strategy').serialize(settings || {})

(slightly modified to take away emberscript-specific idiom)

@niquelbarron I'll come back on the epf issue once I have got an epf-backed skeleton app to test that out.

natevw commented 11 years ago

What would the proper way to get the EmberModelAuthModule instance from userspace outside of the closure where patch was intended to get called?

Basically, we're trying to workaround this issue and can extend our RESTAdapter with whatever _ajax method is necessary, but I'm not familiar enough with Ember's architecture to figure out where the 'auth._strategy' object is living. I see Ember.Auth.EmberModelAuthModule but it is an "(unknown mixin)".

natevw commented 11 years ago

I'm not terribly happy with this as it creates a new RESTAdapter object every request, but here's a way I found of making use of the patched _ajax method:

App.RESTAdapter = Ember.RESTAdapter.extend({
  buildURL: function(klass, id) { /* … */ },
  _ajax: function(url, params, method, settings) {
      return Ember.RESTAdapter.create()._ajax(url, params, method, settings);
  }
});

Unfortunately, the actual patched method is indeed buggy, using a random undeclared _ajax variable. Is there any reason why your patch doesn't use the ember this._super call instead of the CoffeeScript one?

patch: ->
  self = this
  Ember.RESTAdapter.reopen
    _ajax: (url, params, method, settings) ->
      @._super url, params, method, self.get('auth._strategy').serialize(settings || {})
natevw commented 11 years ago

Also the way you rely on your patch in https://github.com/ebryn/ember-model/commit/7e091abe1ba1a6b3cdaab30b5df0490fa4af0ce5 makes the url and params arguments go unused in the final request. You either need to make your self.get('auth._strategy').serialize take in the url/params and add them alongside the data, or submit a patch to ember-model that doesn't skip adding those parts into the settings object when one is provided.

A partial fix would be to have your patch do:

var settings = this.ajaxSettings(url, method);
return this._super(url, params, method, self.get('auth._strategy').serialize(settings || {}));

…but if params are provided they will get clobbered by ember-model's _ajax call. This is really quite broken as it sits :-(

natevw commented 11 years ago

Okay, I think I've found a workaround of sorts for our situation. Obviously this is not the real solution but in case it helps somebody else move along.

First of all, patch the served-to-user "ember-auth-module-ember-module.js" (or minified if that's what you're using) to replace the broken patch mixin with this hook:

  patch: function() {
    var self;
    self = this;

    window.HACK_EmberAuthSettings = function (opts) {
        return self.get('auth._strategy').serialize(opts);
    }
    /*
    return Ember.RESTAdapter.reopen({
      _ajax: function(url, params, method, settings) {
        return _ajax.__super__.constructor.call(this, url, params, method, self.get('auth._strategy').serialize(settings || {}));
      }
    });
    */
  }

This basically just gets rid of the broken call on the undefined _ajax object, and instead unburies the serialize helper function from wherever it properly gets hooked in, right to a global. Wheee!

With that exposed, you can then add this hack into your app code:

// HACK: ember-auth expects to fixup the jQuery options right before flight, so monkey patch at that level
var _origAJAX = Ember.$.ajax;
Ember.$.ajax = function (opts) {
    return _origAJAX.call(this, HACK_EmberAuthSettings(opts));
};

(Beware this will break any jQuery/Ember AJAX requests made before the patch code above runs.)

ekampp commented 11 years ago

@natevw you can make a fork, and implement the patch, then make a pull request, or people can just point to your version of the repo to get the fix? I think that would be even easier for people?

natevw commented 11 years ago

That is not a real fix. Do not use it unless you understand how grievous a hack it is.

ekampp commented 11 years ago

I'm not intending to use it, but others might be. Just a friendly suggestion :smile:

heartsentwined commented 11 years ago

@natevw

What would the proper way to get the EmberModelAuthModule instance from userspace outside of the closure where patch was intended to get called?

Not sure how "outside" you're referring to. If it's still within ember, then the recommended way is DI through its container. #119 has a mini-tutorial for beginners.

If it's totally outside ember, you can use this hack, which basically hard-codes through ember's internal container.


Thanks also for pointing out the flaw of my ember-model patch. I can push a fix, but: which should take priority? e.g. the url param? vs a { url: '' } in the final settings param?

mykoweb commented 10 years ago

Apologies for piggy-backing on this issue, but has this issue been resolved? I'm using Ember 1.3.1.1, Ember Data 1.0.0.beta.5, and ember-auth-rails 5.0.1. I see the correct GET call to my Rails backend, but the auth_token param is not being sent.

Started GET "/api/v1/users/1" for 127.0.0.1 at 2014-01-30 10:02:12 -0800
Processing by Api::V1::UsersController#show as JSON
  Parameters: {"id"=>"1"} # params missing auth_token
mykoweb commented 10 years ago

After some more digging, I realized that it was an issue with how I set up ember-auth-rememberable. The auth_token param is now being sent.