Devise + Mongoid store wrong data in session #2882

Merged
merged 3 commits into from Feb 14, 2015
@hauleth

I'm using Devise 3.2.3 + Mongoid 4.0.0.beta1 and when I try to call current_user I get:

The operation: #<Moped::Protocol::Query
  @length=125
  @request_id=18
  @response_to=0
  @op_code=2004
  @flags=[]
  @full_collection_name="orodruin_development.users"
  @skip=0
  @limit=-1
  @selector={"$query"=>{"_id"=>{"$oid"=>BSON::ObjectId('530a1ec84e49550fbd000000')}}, "$orderby"=>{:_id=>1}}
  @fields=nil>
failed with error 10068: "invalid operator: $oid"

See https://github.com/mongodb/mongo/blob/master/docs/errors.md
for details about this error.

Bug is somewhere where this is set:

warden.user.user.key: [[{"$oid"=>BSON::ObjectId('530a1ec84e49550fbd000000')}], "$2a$10$/J1DMorvMdGZm6aQicsYL."]
@josevalim
Plataformatec member

Hrm, this is very weird. Why is mongoid returning such a weird object on to_key? I assume the error is coming from here:

https://github.com/plataformatec/devise/blob/master/lib/devise/models/authenticatable.rb#L203-L205

@hauleth

After some studies:

  • #to_key return array of BSON::ObjectIds.
  • I've tried to flatten whole [record.to_key, record.authenticatable_salt] and this changes nothing (except that array is now flatten).

After that I've create ugly hack that do what is needed:

        def stringify(item)
          if item.kind_of?(Array)
            item.first.to_s
          else
            item
          end
        end

        def serialize_into_session(record)
          [stringify(record.to_key), record.authenticatable_salt]
        end
@hauleth

You should really consider using Appraisal for your tests (as Devise is Rails specific).

@josevalim
Plataformatec member

This is very weird. Active Model specifies that the value returned by to_key needs later to be reusable in order to lookup records but it seems it doesn't work for mongoid?

@josevalim
Plataformatec member

Appraisal is nice but in this case all you need to do is BUNDLE_GEMFILE=... and that is simpler than adding an extra dependency.

@hauleth

Mongoid #to_key also is reusable as a lookup key, but only till serialized. As AR #to_key returns Fixnum, Mongoid returns BSON::ObjectId which serialize to JSON as {"$oid": "blahblahblah"}. And this is problem (this doesn't deserialize the same way).

@josevalim
Plataformatec member

What happens if you just call .to_s on it? I guess it would work for both AR and Mongoid.

@hauleth
@josevalim
Plataformatec member

Hrm... I just noticed it actually needs to get the first element of the array out. :( That's very specific to Mongoid and we would probably need to move it to the adapter instead then.

@hauleth
@hauleth hauleth referenced this pull request in ianwhite/orm_adapter Feb 24, 2014
Open

Add #simple_key that is needed for Mongoid #34

@florentmorin

Despite your patch, the same problem appear using Devise + Mongoid 4.0.0 beta1 + Rails 4.1 rc1.
No problem with Rails 4.0.

@josevalim josevalim referenced this pull request Mar 29, 2014
Closed

devise + mongoid #2949

@Fudoshiki

when i entered email and password:

Moped::Errors::QueryFailure in Admin::ProjectsController#index

The operation: #{"_id"=>{"$oid"=>BSON::ObjectId('534513085533310d78000000')}}, "$orderby"=>{:_id=>1}} @fields=nil> failed with error 17287: "Can't canonicalize query: BadValue unknown operator: $oid" See https://github.com/mongodb/mongo/blob/master/docs/errors.md for details about this error.

flash: {"discard"=>["alert"], "flashes"=>{"alert"=>"You need to sign in or sign up before continuing.", "notice"=>"Signed in successfully."}}
session_id: "3116c43405690393af17a3de29ea3264"
warden.user.user.key: [[{"$oid"=>"534513085533310d78000000"}], "$2a$10$43i0DRs4mYb.Lk4ovl.OxO"]

@ahmeij

If you want to work with Devise, mongoid and rails 4.1 while this is being worked on add the following to your user model:

class << self
  def serialize_from_session(key, salt)
    record = to_adapter.get(key.to_s)
    record if record && record.authenticatable_salt == salt
  end
end

The .to_s on the key resolves mongoid's BSON::ObjectId problem.

Let me know if you want this as a pull request, an extra to_s on an active_record key should not break stuff I guess (have not tested that)

@felipero

That's an issue with the json cookies serializer of rails 4.1. See it here:
#2949 (comment)

@everaldo

I'm experiencing the same problem.

For know I will implement the solution proposed by @ahmeij .

When a final patch would be launched, I would be thankful if someone could notify me on comments.

@tranvictor

@ahmeij Your solution inspired me a lot. However, it doesn't fully solve the problem. I followed your solution and got the sign_in worked but current_user and user_signed_in? helpers dont even work. current_user always returns nil and user_signed_in? returns false in all cases.

I came up with this solution, it works perfectly for me. You guys can give a try:

class << self
  def serialize_from_session(key, salt)
    record = to_adapter.get(key[0]['$oid'])
    record if record && record.authenticatable_salt == salt
  end
end

Note: This solution only works with mongoid ~> 4.0.0. I will try to make a better patch which works with all version of mongoid and activerecords.

@qwlong

@tranvictor your solution solve my problem, thanks

@anlek

Any progress on this?

@itsluke

@tranvictor I was having the same issues as you, but I found a little tweak of your code worked well:

class << self
  def serialize_from_session(key, salt)
    record = to_adapter.get(key[0].to_param)
    record if record && record.authenticatable_salt == salt
  end
end
@btrepp

I have no idea what else this will break, but a workaround thats having promise for me is this on the user model.

#Hack for mongoid object ids with devise
def to_key
    key = respond_to?(:id) && id.to_s
    key ? [key] : nil
end

It's just calling to_s on the id, as mongoid lets you query that way or via the BSON::Id object it returns. I have no idea how this may effect other libraries though.

@ekampp

@thisbeluke your solution fixed the problem for me. Thanks! 👍

@lcguida

@tranvictor code worked for me.

@ahmeij stopped giving me errors, but user_signed_in? mehtod would always return false after login.

@ghost

@thisbeluke and @tranvictor's solutions work in different cases for me. In my application, in development, key is [{"$oid"=>"53adc74563616ef6e2290000"}] but when testing with rspec, key is [BSON::ObjectId('53adc74563616ef6e2290000')].

Has anyone experienced anything similar?

@tranvictor

@stewartlsmith you are right, I faced the same situation as yours. I had to use an if statement to get the right key for each case. That's a bad solution but I did it for hot patch.

@PaBLoX-CL

@stewartlsmith, same here. @thisbeluke solution worked when testing, but didn't work in the development environment, on the other hand @tranvictor solution worked when in development, but when testing with rspec it failed with:

  3) PatientsController GET new assigns a new patient as @patient
     Failure/Error: get :new, {}, valid_session
     NoMethodError:
       undefined method `[]' for BSON::ObjectId('53c00d576c75736736040000'):BSON::ObjectId
     # ./app/models/user.rb:47:in `serialize_from_session'
     # ./spec/controllers/patients_controller_spec.rb:62:in `block (3 levels) in <top (required)>'
     # -e:1:in `<main>'

I solved it using an horrible if elsif statement in devises ' user model, checking for both environments and now both works. But I hope this get solved soon :)

@arthurnn

This will be addressed in newer versions of Mongoid.

For now, you can add the following in your model to solve the problem:

  def to_key
    if key = super
      key = key.map(&:to_s)
    end
    key
  end
@timoschilling

defining id as a String could be a solution

class User
  include Mongoid::Document
  field :_id, type: String
end
@tranvictor

@timoschilling would that affect mongodb performance? I know that they use BSON for performance so using String instead of BSON would have a side effect right?

@timoschilling

@tranvictor there is a small performance impact, how large it is depend on your collection size and your relation count. But that impact should not be a problem

@attilagyorffy

@arthurnn @timoschilling - None of these suggestions seem to be working at this point. I'm trying to get the current_user as part of Doorkeeper and its resource_owner_authenticator block (although I don't think that would matter much):

current_user
Moped::Errors::QueryFailure: The operation: #<Moped::Protocol::Query
  @length=142
  @request_id=2
  @response_to=0
  @op_code=2004
  @flags=[]
  @full_collection_name="mixtapes_development.users"
  @skip=0
  @limit=-1
  @selector={"$query"=>{"_id"=>{"$oid"=>"544cc81966756b699c000000"}}, "$orderby"=>{:_id=>1}}
  @fields=nil>
failed with error 17287: "Can't canonicalize query: BadValue unknown operator: $oid"

See https://github.com/mongodb/mongo/blob/master/docs/errors.md
for details about this error.
from /usr/local/var/rbenv/versions/2.1.3/lib/ruby/gems/2.1.0/gems/moped-2.0.0/lib/moped/operation/read.rb:50:in `block in execute'

The libraries in question:

  • rails (4.1.5)
  • devise (3.4.0)
  • mongoid (4.0.0 d90a946)
  • moped (2.0.0)

Thank you.

@timoschilling

Do you use a Model like this:

class User
  include Mongoid::Document
  field :_id, type: String
end

Is long time ago that I use Mongo, and I don't have access to a project with it, but in my mind the query should look like this:

@selector={"$query"=>{"_id"=>"544cc81966756b699c000000"}, "$orderby"=>{:_id=>1}}
@attilagyorffy

@timoschilling Yeah, my model is in fact a Mongoid::Document and it has the field :_id, type: String addition in it too. Yet my query looks like this:

@selector={"$query"=>{"_id"=>{"$oid"=>"544cc81966756b699c000000"}}, "$orderby"=>{:_id=>1}}

Note the additional $oid key in the _id attribute.

@timoschilling

@liquid and the $oid shouldn't be there.

@attilagyorffy

@timoschilling yeah that's what i thought too. Never mind, I've disabled the JSON cookie serialised as per #2949 (comment) - I'm planning on using this as part of a pure JSON API anyway. Thank you for the quick responses anyway, greatly appreciated. Here's a 🍰 in return of your help. :))))

@hauleth
@sahin

any updates on this?

def self.serialize_from_session(key, salt)
    record = to_adapter.get(key[0]['$oid'])
    record if record && record.authenticatable_salt == salt
  end

  def to_key
    if key = super
      key = key.map(&:to_s)
    end
    key
  end

@tranvictor I added these, it worked but now, our tests are failing, I think in our test sign_in (devise testhelper in Devise::TestHelpers is not working)

@ElBayaa

the proposed hack by @ahmeij and @tranvictor worked fine for me except when I choose the "remember me" option then the same error appear when retrieving the user from the cookie. so I also changed the serialize_from_cookie so now my User.rb file contains the following:

  def self.serialize_from_session(key, salt)
    record = to_adapter.get(key[0]["$oid"])
    record if record && record.authenticatable_salt == salt
  end

  def self.serialize_from_cookie(id, remember_token)
    record = to_adapter.get(id[0]["$oid"])
    record if record && !record.remember_expired? &&
              Devise.secure_compare(record.rememberable_value, remember_token)
  end
@herophuong

@sahin I also got the same problem with testing using sign_in helper. Overriding serialize_into_session makes session variable seem to look right but the authentication still fails.

May be we should create a new issue for this?

@Zhomart

I used this hack:

# config/initializers/ext/bson.rb
module BSON
  class ObjectId
    def to_json(*args)
      return to_s if args.blank?
      args[0].generate(to_s)
    end

    alias :as_json :to_json
  end
end
@hoang1417

@tranvictor's solution worked for me with Rails 4.1.5, Mongoid 4.0.0 and Devise 3.4. Thanks a lot!

User.rb

  def self.serialize_from_session(key, salt)
    record = to_adapter.get(key[0]["$oid"])
    record if record && record.authenticatable_salt == salt
  end

key (extracted from session["warden.user.user.key"]) has value [{"$oid"=>"5475596d4c65730452000000"}] so for now I think using key[0]["$oid"] the only way to work around.

In my case, just this method is good enough for all Devise functions work well.

@josevalim josevalim merged commit d2658c6 into plataformatec:master Feb 14, 2015

1 check passed

Details default The Travis CI build passed
@hauleth hauleth deleted the hauleth:fix-mongoid-10068 branch Feb 14, 2015
@arthurnn

❤️

@arthurnn arthurnn added a commit to mongodb/mongoid that referenced this pull request Feb 14, 2015
@arthurnn arthurnn We should be able to use doc.to_key to query.
`to_key` method on ActiveModel and ActiveRecord returns an key that can
be can be used to query the db to find that object. We should follow
that API, and not return an ObjectId object.

This fixes an annoying bug when using Mongoid + Devise, where devise use
the to_key method to serialize the user in the session.

[related plataformatec/devise#2882]
[fixes #3626]
658a5d4
@arthurnn

Just fixed this bug on Mongoid side mongoid/mongoid@658a5d4 , it will be included in the next release , 4.0.2
thanks all

@almnes

@arthurnn any specific date on when 4.0.2 will be released?

@arthurnn

@almnes , soon, probably today or tomorrow.

@josevalim
Plataformatec member

Ping for a 4.0.2 release. :)

@josevalim
Plataformatec member
@BM5k BM5k added a commit to BM5k/devise that referenced this pull request Jun 7, 2015
@hauleth hauleth Should fix #2882 e18771a
@miguelgraz

I know I'm late to the party here but just wanted to say thanks to everyone on this thread, I have a case where I have some data migrated from Postgres and therefore some records with integer IDs and other with ObjectID IDs, the information here helped me put together this little hack that fixed my problem, on my User model:

  def self.serialize_from_session(key, salt)
    record = to_adapter.get(key[0])
    record ||= to_adapter.get(key[0].to_i)
    record if record && record.authenticatable_salt == salt
  end
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment