Revised email re-confirmation feature #1266
Conversation
|
+1 |
|
Definitely +1, thanks! |
|
+1 |
|
+1 Pleeeeease! :) |
|
+1 |
1 similar comment
|
+1 |
…ded reconfirmable explanations
Previously, if a user submitted their new email twice, they would bypass the reconfirmation requirement and wind up auto-confirmed.
|
Rebased from master -- the code has not changed since August 14. |
|
+1 |
3 similar comments
|
+1 |
|
+1 |
|
+1 |
|
Jose, are there any plans to merge this pull request? Is there anything specific in this code that you are not happy with? |
|
I brought this pull request up to date with the latest Devise master. All tests are passing. @enrico: I've pinged @josevalim via PM and he expressed that he hopes to review and merge this in the near future. |
|
+1 |
3 similar comments
|
+1 |
|
+1 |
|
+1 |
|
Guys, I was wondering: what if instead of having one extra field, we actually had an e-mail tables, similar to Facebook? So we could actually have different e-mails and each of them could be confirmed or not. I think this would solve this issue in a similar way but with a more elegant solution. |
|
Jose, I agree that having a separate email table would be a better design (I was wondering why basic devise wasn't designed this way when I found out about it, especially when I was trying to integrate it with omniauth), but I suspect it's going to take quite some work to accomplish. Currently we have a broken system, whereby a user can totally lock himself out of our site if they're not careful about which email address they type in the edit box... My vote would be to merge this change in the next release (with email and unconfirmed_email in the same table), and then (in a later release) re-think/re-implement email-based authentication with a separate table altogether (it has implications on how to validate, expire , tie emails to other auth mechanisms, like omniauth etc.) Just my $0.02 |
|
I agree with @enrico on how to proceed. I think your suggestion is ultimately the right way to go, but the code will take some time to put together and has many far-reaching implications related to current implementations. It may even be something that should wait for a 2.0 release, assuming SemVer. In the meantime, this feature is useful and stable (as far as I can tell from the past few months of use on my own). |
|
It is a point to consider but the problem is that we would have two conflicting features. |
|
+1 Would be really nice |
|
I agree there should be email tables. This is approximately how I did it in another project: User has_many emails each email has its own verified etc path, and one of them is the 'primary' email (ie the one we mail to as an app) User.email itself must not be user-settable; it either doesn't exist (and you use a 'primary' boolean in the emails table), or it's a cached version of whichever (validated) email was most recently selected as primary. Until one is validated, it's nil. Also while we're at it: omniauth should be intrinsically a has_many too. Users can have many Identities. Identities are not user-editable — they are provided exclusively via backend. If a user has identity and doesn't have a password, then nothing should require a password (including edits), but they could add one on the new-from-omniauth registration flow or in edit — at which point it would be required but only for confirmation-type uses (ie edit not login). Identities get saved into a table that's basically a replica of the omniauth hash, plus the usuals (user_id, lock_version, timestamps). This is what I'm implementing now, actually… though I'm not sure I grok Devise enough to put it into the gem itself. |
|
@saizai Devise is completely agnostic to how you implement Omniauth internally. That's the beauty of it, it doesn't need to be something backed into Devise. |
|
Also just re email: I strongly suggest bundling in something like https://github.com/Empact/validates_email_veracity_of rather than the current (bad) regexp. That plugin lets you do full-spec regexp only, but it also lets you do things like check whether the domain is actually valid for emailing and the like. Much more robust. |
|
We could add the plugin to a list of recommendations as I believe this is a |
| end | ||
|
|
||
| def email_exists_in_unconfirmed_emails?(record) | ||
| count = record.class.where(:unconfirmed_email => record.email).count |
There was a problem hiding this comment.
Does this really matters? If the e-mail for the other user is unconfirmed, it is fine to be duplicated. We would just need to change how confirmable works in case someone is trying to confirm an e-mail that was already given to someone else.
|
As a sidenote, I've been using devise with a separate emails table just by adding the confirmable to the email model. Takes a little extra coding for the email add/edit views but works fine. |
|
@aaronchi would you be willing to share that code somewhere? I'm planning to implement something similar on a project I'm working on and I'd like to see how you're doing it. |
|
+1 |
1 similar comment
|
+1 |
This code is largely taken from Mandaryn's implementation from several months ago that was not merged. The discussion around that code is still relevant. It can be found here: #1120
I've modified the code to more thoroughly test backward compatibility with prior installations (for users who are already using confirmable). I've also used #headers_for to send the confirmation email to the appropriate email address depending on if the :reconfirmable setting is in use.
Any feedback is definitely appreciated. Thanks!