Skip to content

Instantly share code, notes, and snippets.

@godfat
Last active August 29, 2015 14:08
Show Gist options
  • Star 0 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save godfat/20d38d81cc2b815913b7 to your computer and use it in GitHub Desktop.
Save godfat/20d38d81cc2b815913b7 to your computer and use it in GitHub Desktop.
In reply to: https://twitter.com/plataformatec/status/525670283899072513
Hi,
This would probably be a long story, and I am not going to go
through all the details here. You're doing well in what
could be done, but in general, I oppose to build a user system
on top of some generic framework. The reason is that a user
system could be very specific to business logic, and it
could be very hard to customize devise in a way that works
well for that.
Honestly this could simply because I am not familiar with how
devise works, but it's really frustrating to track devise code.
For example, now I am getting this message when trying to do:
`sign_in_and_redirect(user)`
You have to confirm your account before continuing.
[updated: This is now fixed. See comments below. It's totally my own fault]
And I have no idea where this was triggered. The user passed
definitely has `confirmed_at` set, and the user could
successfully login with the form when tried again. It's simply
that calling `sign_in_and_redirect(user)` didn't work.
We're also stuck at 3.0.4 and cannot upgrade right now due to
the flow change from later devise, and I don't yet find a way to
make it work the same way for now. I believe this could be done,
but it's just costing me too much time and effort than
trying to do on my own.
In short, I don't think such user system would work if we're
building a business which would require a bunch of customization,
unless I am so familiar with devise to begin with. (for example,
if I developed devise, then of course I could find a way to do it)
and I think surely devise could have better implemented somehow.
I guess this is simply too ambitious and could hardly do right.
It's improving over time, definitely, but we're already stuck there...
To show a quick implementation improvement:
https://github.com/hassox/warden/blob/v1.2.3/lib/warden/proxy.rb#L212
I believe it's not a good idea to name a local variable the same as
the defining method. Without looking though line-by-line, it could look
like doing some crazy recursion. Which is not.
I saw things like that tracking through warden and devise.
Really a bad experience for me.
My two cents.
Thanks for listening to my complaining.
Also a quick suggestion for:
http://blog.plataformatec.com.br/2013/08/devise-3-1-now-with-more-secure-defaults/
Regarding "Store digested tokens in the database":
Since we're already using bcrypt, I think we could just use bcrypt for
this purpose. Just treat confirmation tokens as password, and of course
remove the tokens after it's used.
@godfat
Copy link
Author

godfat commented Oct 24, 2014

I put some puts in sign_in_and_redirect, and it's not called.
Huh? Then which method is sign_in_and_redirect really calling???

@godfat
Copy link
Author

godfat commented Oct 24, 2014

I detected that it's throwing :warden with {:scope=>:user, :message=>:unconfirmed}
But really, where is this sign_in_and_redirect defined? Why I can't find it?
Isn't that in devise helper?

@godfat
Copy link
Author

godfat commented Oct 24, 2014

@godfat
Copy link
Author

godfat commented Oct 24, 2014

Stupid me. Just realized that I was reading devise 3.3.0 instead of 3.0.4.

@godfat
Copy link
Author

godfat commented Oct 24, 2014

Finally tracked it down after looking at the right code.
It's because there's no update_columns in rails 3,
and so I was using self.class.where(:id => id).update_all(args),
and then the instance is not reflecting the value from database.
Silly me. It's totally my fault. After putting a quick reload now it works.
Should probably just change the instance without reloading though.

@godfat
Copy link
Author

godfat commented Oct 24, 2014

Replaced reload to: args.each{ |k, v| send("#{k}=", v) }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment