add email_scope option for email uniqueness validator #5094

flynfish · 2019-07-09T19:27:24Z

This is a reboot of the closed PR #4793 with some tests. Issue #4767 has a lot of information about why this would be useful.

Description from #4793

per subdomain auth;
soft deletion and paranoid like support;
example from https://github.com/plataformatec/devise/wiki/How-To:-Allow-users-to-sign-in-using-their-username-or-email-address.

ACPK · 2019-08-31T04:53:10Z

@flynfish @tegon - Do you know the status of this? I'd like to implement Device for a project that uses subdomains.

flynfish · 2019-09-05T17:11:57Z

@ACPK I haven't heard anything else from @tegon yet. We are using my fork at the moment until this can hopefully be merged.

tegon · 2019-09-05T17:28:43Z

Sorry guys, I wasn't able to look at this yet, but I'll definitely take a look as soon as I can.

christianrolle · 2019-09-13T08:37:36Z

@tegon scoping the email makes totally sense in a multi tenant scenario.
We're gonna fork it with that important enhancement as well. Hope we're able to come back to trunk soon.

For multi tenancy the email validation scoping is crucial. This enhancement is actually based on the currently open PR: heartcombo#5094 As soon as the feature is merged, one should use the Devise trunk again.

bananatron · 2019-10-28T12:57:20Z

Scoping email validations is HUGE - thanks so much for this!

tegon · 2019-11-26T14:31:04Z

test/rails_app/lib/shared_user.rb

@@ -7,7 +7,7 @@ module SharedUser
    devise :database_authenticatable, :confirmable, :lockable, :recoverable,
           :registerable, :rememberable, :timeoutable,
           :trackable, :validatable, :omniauthable, password_length: 7..72,
-           reconfirmable: false
+           reconfirmable: false, email_scope: [:username]


I would rather create another test model with this option so that we can keep both behaviors being tested. Something similar to https://github.com/plataformatec/devise/blob/master/test/rails_app/lib/shared_user_without_email.rb

tegon · 2019-11-26T14:32:40Z

test/integration/registerable_test.rb

@@ -124,7 +124,9 @@ def user_sign_up
    #    https://github.com/mongoid/mongoid/issues/756
    (pending "Fails on Mongoid < 2.1"; break) if defined?(Mongoid) && Mongoid::VERSION.to_f < 2.1

-    create_user
+    user = create_user
+    user.update_attribute(:username, nil)


Would you mind to explain why this change was needed?

I don't remember now...I'll pick this back up in the next few days along with the other comments you made.

tegon · 2019-11-26T14:39:50Z

test/models/validatable_test.rb

@@ -26,6 +26,18 @@ class ValidatableTest < ActiveSupport::TestCase
    assert user.valid?
  end

+  test 'should allow duplicate email when email_scope attribute does not match' do


I believe something like this should be enough:

existing_user = create_user user = new_user(email: existing_user.email, username: "New username") assert user.valid?

christianrolle · 2020-04-01T14:36:49Z

@flynfish Did you have the chance to push this change forward? We certainly should get it done.

flynfish · 2020-04-01T17:51:30Z

@flynfish Did you have the chance to push this change forward? We certainly should get it done.

I didn't have a chance, but thanks for the reminder. I'm gonna set a goal to get the comments addressed in the next week.

donny-nyc · 2020-06-04T14:38:51Z

@flynfish we're looking to enable this in our project. Would you recommend forking for now?

Unnumbered · 2020-07-03T04:14:24Z

any news?

xhocquet · 2020-08-12T19:47:35Z

Hey @tegon , I'm stepping in to try and push this over the finish line for my colleague @flynfish

I've split the test case out and reverted some unnecessary changes caused by conflating the two test users. Could you take another look and let me know what you think? Thanks! 🙏

xhocquet · 2020-09-01T22:07:56Z

Hey @carlosantoniodasilva , I noticed you were actively merging PRs, could you help us get this one across the finish line? I think the community would appreciate this feature!

carlosantoniodasilva · 2020-09-02T22:58:49Z

@xhocquet hey, yes! Appreciate the ping. I've recently started to catch up on some of the issues/PRs here, just want to wrap up a few things on the other projects and then Devise is next. I'm putting this one high in the list to follow-up on first, please stay put for a little longer, I should get to it soon.

danzanzini · 2020-09-29T13:32:27Z

Hello @carlosantoniodasilva, I was taking a look at this PR and I haven't found where these changes impact the migration generated to create users.

As far as I know, the migration generated by Devise adds a database index to ensure email uniqueness. Without changing this index, this feature will not work since the new user creation will be blocked at the database layer.

Another thing to take into consideration is that the recoverable module may not work well because of the duplicated e-mails.
As described in this wiki post:

it may be the case that users can have multiple accounts with the same e-mail address. This will cause the “recoverable” module not to work well, as it will only include a link in the password reset e-mail to the first account for which it finds a matching e-mail.

I'm not so sure about all of this, but can you take a look when reviewing this PR?
If you find that these points are blockers for this PR, I will be glad to help to find possible solutions and work on them.

Thank you

xhocquet · 2020-10-01T14:58:05Z

Thanks for the feedback @danzanzini , some thoughts -

By following the referenced wiki post, the migration created by Devise generators would already be insufficient since they would not contain the 'username' for example. One option could be to add a commented out row with the scoped index in the migration to guide developers to the right usage?

Given the migration gotcha and the incompatibility with the recoverable module, maybe it makes sense to create a new wiki for scoped emails outlining the caveats?

danzanzini · 2020-10-02T00:48:16Z

@xhocquet creating a wiki article is a good idea and should solve all these problems, I think it will also remove the need for the commented line in the migration since it would be noise for most users.

I'll work on a first version and then bring it here for discussion.

Thank you :)

danzanzini · 2020-10-05T22:45:17Z

@xhocquet Here is the ~~first version of the wiki page~~ wiki page, feel free to make any adjustments you find necessary.

Since this feature is not merged, I haven't added it to the wiki index.

xhocquet · 2020-10-06T15:59:10Z

@danzanzini Small typo fix but otherwise the wiki looks great! I'm not sure if there's other considerations for this change, but the existing page seems to reflect the two you mentioned succinctly.

Edit: Realized I changed the title and therefore the link above became invalid. New wiki link

vickodin · 2020-11-24T14:29:21Z

On the other hand, we have next 2 options in config/initializers/devise.rb

  # ==> Configuration for any authentication mechanism
  # Configure which keys are used when authenticating a user. The default is
  # just :email. You can configure it to use [:username, :subdomain], so for
  # authenticating a user, both parameters are required. Remember that those
  # parameters are used only when authenticating and not when retrieving from
  # session. If you need permissions, you should implement that in a before filter.
  # You can also supply a hash where the value is a boolean determining whether
  # or not authentication should be aborted when the value is not present.
  # config.authentication_keys = [:email]

  # Configure parameters from the request object used for authentication. Each entry
  # given should be a request method and it will automatically be passed to the
  # find_for_authentication method and considered in your model lookup. For instance,
  # if you set :request_keys to [:subdomain], :subdomain will be used on authentication.
  # The same considerations mentioned for authentication_keys also apply to request_keys.
  # config.request_keys = []

I'm not yet understand why we need request_keys, but we can use:
config.authentication_keys = [:account_id, :email] instead of email_scope

What do you think?

xhocquet · 2020-11-24T15:17:26Z

@vickodin I think your suggestion is not directly related to the problem this PR is trying to solve. The problem we are trying to solve is that multiple users with the same email cannot be created because of Devise validations.

The configs you point to could be used to lookup these users after they are created. Personally we would not use this and would instead use domain-based scoping to load our user records.

vickodin · 2020-11-24T16:04:06Z

@vickodin I think your suggestion is not directly related to the problem this PR is trying to solve. The problem we are trying to solve is that multiple users with the same email cannot be created because of Devise validations.

The configs you point to could be used to lookup these users after they are created. Personally we would not use this and would instead use domain-based scoping to load our user records.

@xhocquet Thank you for explanations!

Then when this PR will be merged?

Thanks in advance!

vickodin

Looks good!

xhocquet · 2021-01-05T17:58:52Z

Hey @carlosantoniodasilva, happy new year! Hoping to bump this in your notifications 😄

carlosantoniodasilva · 2021-01-05T21:58:22Z

@xhocquet happy new year! Consider it bumped 😉

There may be more to it than just the validation so I need to look into it a bit more carefully in the coming weeks.

nexxus-vi · 2021-02-03T14:59:36Z

+1, we need this please :)

poetry-like-code · 2021-03-10T17:49:05Z

Although this request seems to be dragging for morethan 3 years now, i don't see anyone venting out any frustration, kudos to everyone for that. :)
And +1, we need this too :) kindly fast-track.

stephenpassero · 2021-04-26T16:05:08Z

@carlosantoniodasilva I noticed @poetry-like-code brought up this PR recently, but didn't see any changes. Is there any update on getting this merged?

Thanks!

carlosantoniodasilva · 2021-04-26T22:23:46Z

@stephenpassero appreciate the ping, unfortunately no updates at this time yet.

This is still where it stands:

There may be more to it than just the validation so I need to look into it a bit more carefully in the coming weeks.

Unfortunately 3+ months and I haven't stopped to look into this carefully yet, but I'm bumping it again in my list here. First I have to get a release out there. 😅

akshaysmurthy · 2021-09-06T10:53:24Z

Until this is merged, what is the recommendation for people like me who wants to implement this (email uniqueness on a scope) in our app today? 😄

danzanzini · 2021-09-06T13:12:40Z

Hi @akshaysmurthy, I did these steps to implement it in my app:

Changed the unique index of the email column:

def up
  remove_index :users, :email
  add_index :users, [:email, :tenant_id], unique: true
end

Overwrote the will_save_change_to_email? method so Devise skips its built-in validations. This change may impact other parts of your application if this method is used anywhere else.

# user.rb
def will_save_change_to_email?
  false
end

Created my own email validations:

# user.rb

validates :email, uniqueness: { scope: :tenant_id }
validates :email, format: /\A[^@\s]+@[^@\s]+\z/

Hope it helps

mrabetr · 2021-09-24T07:52:43Z

when is this due to be merged please? thanks

flynfish · 2021-09-24T19:15:21Z

@tegon @vickodin @carlosantoniodasilva it would be really helpful to the community if we could get this merged or if you could provide any insight into what we can do to help out further

carlosantoniodasilva · 2021-09-24T19:55:57Z

@danzanzini thanks for the detailed steps there.

@akshaysmurthy the simplest thing you can do if you need any further customization: don't use validatable, write your own email/password validations that include the uniqueness scope.

@mrabetr there's no due date for merging PRs. ;)

@flynfish at this point I cannot provide any insights, without actually stopping to look into the problem and think about it more carefully, my apologies. Like I said above, multi tenancy might be more than just the validation so I want to consider the problem better, it's just not been high in my priority list, all things considered. In any case, validatable is composed of just a few validations that you can easily add to your model to customize things further for now.

I am unfortunately not in a position to provide any timeline for when I can work on this. Again, I apologize to everyone for not having given this proper attention yet, but I want to reassure that I'm keeping an eye on each and every comment.

❤️💜💛💚💙

bvogel · 2023-05-23T16:26:05Z

Yeah, another year and a tad have passed, making this probably the longest outstanding PR in this project with the most interest. Any update? (i.e. bump!)

flamontagne · 2023-06-15T19:39:13Z

Hi @akshaysmurthy, I did these steps to implement it in my app:

Changed the unique index of the email column:
def up
  remove_index :users, :email
  add_index :users, [:email, :tenant_id], unique: true
end
Overwrote the will_save_change_to_email? method so Devise skips its built-in validations. This change may impact other parts of your application if this method is used anywhere else.
# user.rb
def will_save_change_to_email?
  false
end
Created my own email validations:
# user.rb

validates :email, uniqueness: { scope: :tenant_id }
validates :email, format: /\A[^@\s]+@[^@\s]+\z/
Hope it helps

This is a great workaround, however redifining will_save_change_to_email? will also mess with the Confirmable plugin.

Instead of redefining will_save_change_to_email? I think it's better to disable the validatable plugin. Just make sure you add all the validators you need (for email and password columns) in the User model.

bvogel · 2023-08-22T11:46:50Z

@carlosantoniodasilva if you aren't willing to implement this, please be honest enough to at least give an explanation and close this PR.

flamontagne · 2023-09-10T23:31:03Z

A word of caution: I just found out that the workaround mentionned in this thread will also mess badly with the password reset function. If the same email address is associated with two or more tenants, the password reset link in the email may point to the WRONG tenant. (e.g. wrongsubdomain.myapp.com/reset-password?token=blablabla...)

bvogel · 2024-01-24T13:30:26Z

As this just hit us again, @carlosantoniodasilva - any news and/or decisions on how to move forward with this?

justwiebe · 2024-03-12T21:21:29Z

@flamontagne In my case, I just needed to support soft-deleted users, so I was able to fix the password reset link by hacking send_reset_password_instructions

class User < ApplicationRecord
  def self.send_reset_password_instructions(attributes = {})
    recoverable = where(deleted_at: nil).find_or_initialize_with_errors(reset_password_keys, attributes, :not_found)
    recoverable.send_reset_password_instructions if recoverable.persisted?
    recoverable
  end
end

fa11enangel · 2024-03-26T16:17:51Z

@justwiebe thank you for the hint. I've updated the wiki subdomain tutorial to add a password reset instruction:

see: https://github.com/heartcombo/devise/wiki/How-to:-Scope-login-to-subdomain

@flamontagne you can have a look at it. It should help you to find a solution.

Maybe it is a good idea, to fill up attributes for the method based on request_keys: [:subdomain] given in User model as it is used for authentication.

xhocquet mentioned this pull request Jul 9, 2019

Validatable should allow email uniqueness to be scoped #4767

Open

tegon added the Needs review label Aug 2, 2019

christianrolle mentioned this pull request Sep 13, 2019

Add email validation scoping #5136

Open

tegon reviewed Nov 26, 2019

View reviewed changes

karthik-sivadas mentioned this pull request Dec 1, 2019

Update user model specs chatwoot/chatwoot#334

Merged

flynfish and others added 3 commits August 12, 2020 14:49

Adds email_scope option to email uniqueness validator.

3118aab

Fix tests

361efa2

Split new scope validation to separate test user. CR comments

bd4580f

xhocquet force-pushed the 4767-add-email-scope branch from 11ab701 to bd4580f Compare August 12, 2020 19:49

flynfish requested a review from tegon August 12, 2020 19:57

xhocquet and others added 2 commits August 13, 2020 15:58

Add mongoid user stub

ee21b9f

Merge branch 'master' into 4767-add-email-scope

92facc7

vickodin approved these changes Nov 25, 2020

View reviewed changes

tegon removed their request for review May 5, 2021 14:02

add email_scope option for email uniqueness validator #5094

Are you sure you want to change the base?

add email_scope option for email uniqueness validator #5094

Conversation

flynfish commented Jul 9, 2019

ACPK commented Aug 31, 2019 • edited Loading

flynfish commented Sep 5, 2019

tegon commented Sep 5, 2019 • edited Loading

christianrolle commented Sep 13, 2019

bananatron commented Oct 28, 2019

tegon Nov 26, 2019

Choose a reason for hiding this comment

tegon Nov 26, 2019 • edited Loading

Choose a reason for hiding this comment

flynfish Dec 4, 2019

Choose a reason for hiding this comment

tegon Nov 26, 2019

Choose a reason for hiding this comment

christianrolle commented Apr 1, 2020

flynfish commented Apr 1, 2020

donny-nyc commented Jun 4, 2020

Unnumbered commented Jul 3, 2020

xhocquet commented Aug 12, 2020

xhocquet commented Sep 1, 2020

carlosantoniodasilva commented Sep 2, 2020 • edited Loading

danzanzini commented Sep 29, 2020 • edited Loading

xhocquet commented Oct 1, 2020

danzanzini commented Oct 2, 2020

danzanzini commented Oct 5, 2020 • edited Loading

xhocquet commented Oct 6, 2020 • edited Loading

vickodin commented Nov 24, 2020

xhocquet commented Nov 24, 2020

vickodin commented Nov 24, 2020

vickodin left a comment

Choose a reason for hiding this comment

xhocquet commented Jan 5, 2021

carlosantoniodasilva commented Jan 5, 2021

nexxus-vi commented Feb 3, 2021

poetry-like-code commented Mar 10, 2021 • edited Loading

stephenpassero commented Apr 26, 2021

carlosantoniodasilva commented Apr 26, 2021

akshaysmurthy commented Sep 6, 2021 • edited Loading

danzanzini commented Sep 6, 2021 • edited Loading

mrabetr commented Sep 24, 2021

flynfish commented Sep 24, 2021

carlosantoniodasilva commented Sep 24, 2021

bvogel commented May 23, 2023 • edited Loading

flamontagne commented Jun 15, 2023

bvogel commented Aug 22, 2023

flamontagne commented Sep 10, 2023

bvogel commented Jan 24, 2024

justwiebe commented Mar 12, 2024

fa11enangel commented Mar 26, 2024 • edited Loading

ACPK commented Aug 31, 2019 •

edited

Loading

tegon commented Sep 5, 2019 •

edited

Loading

tegon Nov 26, 2019 •

edited

Loading

carlosantoniodasilva commented Sep 2, 2020 •

edited

Loading

danzanzini commented Sep 29, 2020 •

edited

Loading

danzanzini commented Oct 5, 2020 •

edited

Loading

xhocquet commented Oct 6, 2020 •

edited

Loading

poetry-like-code commented Mar 10, 2021 •

edited

Loading

akshaysmurthy commented Sep 6, 2021 •

edited

Loading

danzanzini commented Sep 6, 2021 •

edited

Loading

bvogel commented May 23, 2023 •

edited

Loading

fa11enangel commented Mar 26, 2024 •

edited

Loading