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

Devise + Mongoid store wrong data in session #2882

Merged
merged 3 commits into from
Feb 14, 2015
Merged

Devise + Mongoid store wrong data in session #2882

merged 3 commits into from
Feb 14, 2015

Conversation

hauleth
Copy link
Contributor

@hauleth hauleth commented Feb 23, 2014

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
Copy link
Contributor

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
Copy link
Contributor Author

hauleth commented Feb 23, 2014

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
Copy link
Contributor Author

hauleth commented Feb 23, 2014

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

@josevalim
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

hauleth commented Feb 23, 2014

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
Copy link
Contributor

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

@hauleth
Copy link
Contributor Author

hauleth commented Feb 23, 2014

As my fix showed, BSON::ObjectId#to_s seems to return clean id. With this
hack everything seems OK.

@josevalim
Copy link
Contributor

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
Copy link
Contributor Author

hauleth commented Feb 23, 2014

After some thought it maybe look better as [item].flatten.first.to_s.
Then there is no need for this Array checking. But on the other hand, we
lose information that it was Fixnum (which wasn't that important at all).

@florentmorin
Copy link

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 mentioned this pull request Mar 29, 2014
@Fudoshiki
Copy link
Contributor

when i entered email and password:

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

The operation: #<Moped::Protocol::Query @Length=124 @request_id=6 @response_to=0 @op_code=2004 @flags=[] @full_collection_name="mystand_development.users" @Skip=0 @limit=-1 @selector={"$query"=>{"_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
Copy link
Contributor

ahmeij commented Apr 14, 2014

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
Copy link

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

@everaldo
Copy link

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
Copy link

@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
Copy link

qwlong commented Apr 26, 2014

@tranvictor your solution solve my problem, thanks

@anlek
Copy link

anlek commented Apr 29, 2014

Any progress on this?

@itsluke
Copy link

itsluke commented May 1, 2014

@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
Copy link

btrepp commented May 11, 2014

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
Copy link

ekampp commented May 12, 2014

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

@pablox-cl
Copy link

@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
Copy link

arthurnn commented Sep 9, 2014

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
Copy link
Contributor

defining id as a String could be a solution

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

@tranvictor
Copy link

@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
Copy link
Contributor

@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
Copy link

@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
Copy link
Contributor

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
Copy link

@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
Copy link
Contributor

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

@attilagyorffy
Copy link

@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
Copy link
Contributor Author

hauleth commented Oct 27, 2014

The cake is lie.

Dnia 26 paź 2014 o godz. 18:59 Attila Györffy notifications@github.com napisał(a):

@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. :))))


Reply to this email directly or view it on GitHub.

@sahin
Copy link

sahin commented Nov 5, 2014

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
Copy link

ElBayaa commented Nov 11, 2014

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

@lephuongbg
Copy link

@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
Copy link

Zhomart commented Nov 15, 2014

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
Copy link

@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 added a commit that referenced this pull request Feb 14, 2015
Devise + Mongoid store wrong data in session
@josevalim josevalim merged commit d2658c6 into heartcombo:master Feb 14, 2015
@hauleth hauleth deleted the fix-mongoid-10068 branch February 14, 2015 10:15
@arthurnn
Copy link

❤️

arthurnn added a commit to mongodb/mongoid that referenced this pull request Feb 14, 2015
`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 heartcombo/devise#2882]
[fixes #3626]
@arthurnn
Copy link

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

@w8m8
Copy link

w8m8 commented Feb 16, 2015

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

@arthurnn
Copy link

@almnes , soon, probably today or tomorrow.

@josevalim
Copy link
Contributor

Ping for a 4.0.2 release. :)

@arthurnn
Copy link

@josevalim
Copy link
Contributor

Obrigado!

José Valimwww.plataformatec.com.br
http://www.plataformatec.com.br/Founder and Lead Developer

@miguelgraz
Copy link

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

karunkumar1ly pushed a commit to edcast/devise that referenced this pull request Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet