#50 ✓resolved
James Le Cuirot

Record should be reloaded before maintaining the session

Reported by James Le Cuirot | February 25th, 2009 @ 11:56 AM

In our application, a user is notified by e-mail every time they update their profile. This is done with an AR observer. To prevent authlogic always triggering this, we added the condition user.changes.except("updated_at", "perishable_token", "persistence_token").any?. However, when doing a password reset, it would always send two e-mails. I knew that the record was being saved twice due to session maintenance but I couldn't figure out why the above condition wasn't preventing the second e-mail. I eventually realised what it must be.

authlogic does session maintenance in after_save. In AR, a record's changes are only cleared AFTER after_save. So any changes you might have made to a record are still present in the changes hash from the first save.

I managed to fix it by reloading the record (with reload) in create_session!. I'm not sure if this is also required in update_sessions!. It may be sufficient to somehow clear the changes hash instead of completely reloading the record but reloading seems like the safest option.

Comments and changes to this ticket

  • James Le Cuirot

    James Le Cuirot February 25th, 2009 @ 02:12 PM

    Hmmm just had a thought. My suggestion is okay for observers but what about other after_save callbacks? If acts_as_authentic is called before those are created then authlogic won't be the last in the chain. You don't want to reload before those other callbacks because they won't be expecting it. What you can do though if pass self.class.find(self.id) to create instead of just self. That way you pass a copy of the record instead and the current instance will remain there for the other callbacks to use.

    I tried this but then some of my password reset tests started failing. This is partially factory_girl's fault. I think regular tests fixtures are written directly to the database but factory_girl calls the save method. Ideally, it should call save_without_session_maintenance. The problem is that a new perishable token gets created but the current instance doesn't receive it (only the copy does) and so when the factory gives you a new user, the token in that instance is already stale.

    I therefore propose the attached patch. It does self.class.find(self.id) as suggested above but then assigns the new perishable token to the current instance. Not all models will have a perishable token field and class_eval is a pain so the last line is a bit ugly. Maybe you can think of a better way.

    I hope I'm making sense here!

  • James Le Cuirot

    James Le Cuirot February 25th, 2009 @ 02:19 PM

    Oh and just wondering... all this could be avoided if session maintenance is done in before_save instead but I guess you have a good reason for doing it in after_save?

  • Ben Johnson

    Ben Johnson February 25th, 2009 @ 02:39 PM

    • State changed from “new” to “resolved”

    The reason it's in an after_save is because of the following validation check for the session:

    if attempted_record.new_record?
      errors.add_to_base(I18n.t('error_messages.new_record', :default => "You can not login with a new record"))
      return false
    end
    

    To be honest, that statement really isn't necessary, because at that point you are passing a record, so if you want to pass a new record go for it, you better make sure that record will save. Anyways, the after_save is just insurance to be absolutely sure the record will save into the database. Regardless, if it doesn't no harm made.

    I'll just pushed the change to move it to a before save.

  • James Le Cuirot

    James Le Cuirot February 25th, 2009 @ 03:57 PM

    Sorry, I feel like I'm spamming you here. It's just I eventually decided that clearing and restoring the changes hash would actually be a less problematic solution. Here's a patch for that.

    I'm also thinking that using save_without_callbacks might avoid the whole mess but you can't just call that in save_without_session_maintenance because that would break existing code.

  • James Le Cuirot

    James Le Cuirot February 25th, 2009 @ 04:00 PM

    Gah sorry, didn't see your comment before I posted that. Thanks for doing that, it really makes the whole thing less painful. :)

  • Ben Johnson

    Ben Johnson February 25th, 2009 @ 04:03 PM

    Did that fix the problem?

  • James Le Cuirot

    James Le Cuirot February 25th, 2009 @ 04:26 PM

    Actually I'm getting tons of ActiveRecord::RecordNotSaved errors in my tests and I can't figure out why. I'll keep digging.

  • James Le Cuirot

    James Le Cuirot February 25th, 2009 @ 04:33 PM

    Wait, the changes you made aren't sufficient, right? By moving it into before_save, I meant that authlogic should no longer call save on the record at all, because AR is about to write to the database anyway.

  • James Le Cuirot

    James Le Cuirot February 25th, 2009 @ 04:43 PM

    Figured it out. You need to add "return true" to the end of create_session! because if the last condition returns false then the method returns false and the save is aborted. This didn't affect after_save because you need to raise an exception to roll back the transaction at that stage.

  • James Le Cuirot

    James Le Cuirot February 25th, 2009 @ 04:44 PM

    Sorry, I meant to add that you should remove the attempted_record.save_without_session_maintenance(false) if attempted_record && attempted_record.changed? line in the valid? method of base.rb.

  • James Le Cuirot

    James Le Cuirot February 27th, 2009 @ 11:12 AM

    I know you're a busy guy but I should add that I think authlogic is broken in its current state.

  • Ben Johnson

    Ben Johnson February 27th, 2009 @ 01:05 PM

    Alright, I made some updates, that should do it. Try it out and let me know. Thanks!

  • James Le Cuirot

    James Le Cuirot March 2nd, 2009 @ 07:31 AM

    You're still returning false at the end of create_session!. You need to add "return true" there.

    This should be causing your tests to fail and I've been trying to figure out why it's not. I added a "puts" at the end of create_session! to see what is going on. It's always finding a session. This shouldn't be the case for tests like test_resetting_password_when_logged_out. I don't understand why this is happening but maybe it has something to do with your mock controller.

  • James Le Cuirot

    James Le Cuirot March 2nd, 2009 @ 07:43 AM

    Sorry I missed the ! there so what I meant was, it's never finding a session and therefore creating a new one every time. I did a "puts" again to check, and the call to find is indeed returning nil every single time. There's something seriously wrong here.

  • James Le Cuirot

    James Le Cuirot March 2nd, 2009 @ 08:54 AM

    Okay, ignore most of what I just said. I was horrendously confused. Part of it was due to an odd line in your code though. ;) In create_sessions!, you check to see whether a session already exists to determine whether or not you should create a new one. But (as I observed above), there will never be a session because this method only gets called when @_logged_out is true. In other words, you've already done the check so you don't need to do it again, right?

    Despite this, you still need to return true at the end of that method. In my application, I have an after_validation callback in my session model that checks whether the user model has been verified. If I call save on a user that isn't verified, the fact that it won't create a session shouldn't stop the user from getting saved anyway.

  • Ben Johnson

    Ben Johnson March 2nd, 2009 @ 12:23 PM

    Ok, I pushed out some changes, let me know if that fixes it. I see your point with the double checking though.

  • James Le Cuirot

    James Le Cuirot March 2nd, 2009 @ 12:42 PM

    Cool, that's cleaned it up a bit. Still no "return true" though? Is there a problem with doing that or do you not know why I'm suggesting it? It just occurred to me that you should add to update_sessions! as well.

  • Ben Johnson

    Ben Johnson March 2nd, 2009 @ 12:47 PM

    Neither of those methods are returning false, create_session! returns true and update_sessions! returns an array, which should be treated like true. The callbacks arent explicitly looking for true, they explicitly look for false. Am I correct?

  • James Le Cuirot

    James Le Cuirot March 2nd, 2009 @ 12:58 PM

    Both will return false if the validation on the session model fails. In the save method, you set result to false in this case. Also, I suspect that you may not want to call yield on the result in this case either.

  • Ben Johnson

    Ben Johnson March 2nd, 2009 @ 01:20 PM

    I'm not sure waht you mean about yield, can you fork authlogic and make the changes you are referring to, this way I don't get confused and keep making commits. Thanks.

  • Ben Johnson

    Ben Johnson March 2nd, 2009 @ 02:05 PM

    Everything should be applied. Did that fix it?

  • James Le Cuirot

    James Le Cuirot March 2nd, 2009 @ 06:18 PM

    We got there in the end. :) I'll fork from now on. Thanks for putting up with me.

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile ยป

Object based authentication solution that handles all of the non sense for you. It's as easy as ActiveRecord is with a database.

People watching this ticket

Referenced by

Pages