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

Validatable should allow email uniqueness to be scoped #4767

Open
kylefox opened this Issue Jan 28, 2018 · 13 comments

Comments

Projects
None yet
7 participants
@kylefox
Copy link

kylefox commented Jan 28, 2018

A common scenario is to scope email to a parent model (i.e. Account), but the Validatable module does not easily allow this.

Judging by the posts on StackOverflow, many people struggle with this issue. It would be a Good Thing if Devise could support this common functionality out of the box.

The How to: Scope login to subdomain article recommends completely removing :validatable from the model. This results in all email and password validations being removed — seems like throwing the baby out with the bathwater.

The wiki goes on to recommend that if you want to keep any of the validations, you should cherry-pick the ones you want — presumably by copy/pasting portions of validatable.rb. This approach seems error-prone, unmaintainable, and goes against the "flexible" philosophy in Devise's tagline 🙂

I propose validatable support a new option called email_scope that allows an :email uniqueness scope to be specified. For example:

class User < ApplicationRecord
  belongs_to :account

  devise :database_authenticatable, :validatable, email_scope: :account
end

(The caveats about indexes would still apply. I don't think it's necessary to change how Devise's migrations work — Rails already makes these schema changes trivial, after all).

I'm happy to help out with this feature if there's general agreement that this functionality should be included, and totally open to discussing other ways this scenario could be better addressed by Devise. Thanks!

@argent-smith

This comment has been minimized.

Copy link

argent-smith commented Feb 19, 2018

👍 … makes real sense in case of «soft-deleted» authenticatables as well.

@one-more-alex

This comment has been minimized.

Copy link

one-more-alex commented Mar 2, 2018

+1 to having scope option.

While have no scope I am doing some terrible recreation of validator:

# redefine uniqueness validator to confirm soft deletion
  self._validators[:email].reject!{|v| v.class == ActiveRecord::Validations::UniquenessValidator}
  self._validate_callbacks.delete self._validate_callbacks.find{|v| v.filter.class == ActiveRecord::Validations::UniquenessValidator && v.filter.attributes == [:email]}
  validates_uniqueness_of :email, allow_blank: true, if: :will_save_change_to_email?, scope: :deleted_at

Probably somebody knows proper way ;)...
But sure the Devise code change (not monkey patch or substitution I did) has priority.

@one-more-alex

This comment has been minimized.

Copy link

one-more-alex commented Mar 2, 2018

I even made commit request for this: #4793
Welcome to merge ;)
But probably some test addition maybe required to be clear and a number of Wiki pages correction (subdomain auth, auth bu login or email, etc).

@tegon

This comment has been minimized.

Copy link
Member

tegon commented Mar 29, 2018

Hi everyone, thanks for the feedback.

Although I can see the value of this, I'm not sure it's a good idea to add a configuration option to support it. If we merge #4793, with the email_scope option, other people may want to pass other options to the email validator. For example, conditions and case_sensitive can be useful sometimes. I feel like we might be rewriting the Rails validations API, which is not good.

The validatable module is meant to include some useful defaults for validations, but I don't see a way it can be flexible enough to be extended for multiple contexts. That's why the application has different requirements for validations, we ask to remove the module and implement them by hand - the module isn't that complicated, and is something that isn't likely to change that often.

@tegon tegon added the Discussion label Mar 29, 2018

@kylefox

This comment has been minimized.

Copy link
Author

kylefox commented Mar 29, 2018

If we merge #4793, with the email_scope option, other people may want to pass other options to the email validator

With all due respect, I think you might be worrying about a problem you don't have 😇 Can you defer deciding whether (or not) to support options for the other validators until someone actually asks for it? It seems to me like customizing case_sensitive would be much less common than wanting to scope email uniqueness (a very common use-case in multi-tenant apps, which is where Devise shines 👌).

(BTW — validatable allows an email_regexp option, which seems even more edge-case than scoping emails)

Devise supports multiple User models, making it great for multi-tenant apps — expect for the lack of a scope on the uniqueness constraint. Devise's own documentation even has an icky workaround for this specific use-case, which IMO is a pretty good indicator of missing functionality — if you're scoping logins to subdomains, when wouldn't you want to also scope email uniqueness?

The validatable module is meant to include some useful defaults for validations, but I don't see a way it can be flexible enough to be extended for multiple contexts.

Scoping emails by some "container" seems like a very reasonable use-case to support. I think it's fine to support this (extremely common) scenario without having to support customization of the other validators. It seems overly troublesome to toss out the entire validatable module just to make multi-tenancy work.

the module isn't that complicated

This is true, but the module contains validation besides just email. For example, it seems needlessly error-prone to ask developers to re-implement (or copy/paste) their own password validation simply because they want to scope emails by :account. Because I need to scope emails, I will now miss out on any improvements (or security fixes) related to how passwords are validated. IMO it should be extremely difficult to opt-out of Devise's default password validation — it shouldn't happen as a side effect of having to scope emails.

Anyway, that's just my 2¢ 🙂 Thanks again for the discussion.

@one-more-alex

This comment has been minimized.

Copy link

one-more-alex commented Mar 30, 2018

When I made the PR I thought about passing not only email_scope.
Just was not sure if it will be acceptable.
It may be like:
validates_uniqueness_of :email, {allow_blank: true, if: :will_save_change_to_email?}.merge(other_options_scope_case_etc)

I do not see a need to touch Rails validators code here...
If you about some non-common things of validating, then developers still free to substitute the module.

I think people will mostly afraid to substitute the Validatable module. Who knows what will be in future when some updates may broke the authentication ). Just words "authentication", "authorization" and "validations" have close meaning, as for me.
Not experienced and newbie developers will fill safer if they will just pass some options instead of support the validation in whole.

Probably you may have two choices for developers: pass some extra options or substitute module in whole. But can't insist here.

@tegon

This comment has been minimized.

Copy link
Member

tegon commented Apr 4, 2018

@kylefox

With all due respect, I think you might be worrying about a problem you don't have 😇 Can you defer deciding whether (or not) to support options for the other validators until someone actually asks for it?

I agree, and I think this is valuable especially for projects. But for libraries, we have to think long-term, because we have to support any API we introduce for future versions. Of course, other use cases seems less common but are still valid (like a regular expression for passwords). But it seems like we already went in this direction - I didn't remember the email_regexp, thanks for pointing that out, btw - so I guess it's ok to continue with #4793.

Devise supports multiple User models, making it great for multi-tenant apps — expect for the lack of a scope on the uniqueness constraint. Devise's own documentation even has an icky workaround for this specific use-case, which IMO is a pretty good indicator of missing functionality — if you're scoping logins to subdomains, when wouldn't you want to also scope email uniqueness?

The Wiki is maintained by the community, which means someone needed this behavior and created this article, which may be useful for others. I agree with you that this may a signal of a missing functionality. But I have a question: are there advantages of using the same table for both models? I mean, do they share behavior and/or data except for the devise part? I ask this because at least in the applications I've worked on the users where completely distinct (e.g customers and managers).

This is true, but the module contains validation besides just email. For example, it seems needlessly error-prone to ask developers to re-implement (or copy/paste) their own password validation simply because they want to scope emails by :account. Because I need to scope emails, I will now miss out on any improvements (or security fixes) related to how passwords are validated. IMO it should be extremely difficult to opt-out of Devise's default password validation — it shouldn't happen as a side effect of having to scope emails.

Like I said, this is something that's very unlikely to change. We only have a length validator for password, and if we do add extra validations at some point, it's probably going to break backward compatibility, so we'd need an upgrade guide or something to help in the update either way.

Thanks for the discussion, a lot of good points were raised here. I'll continue to review @one-more-alex PR, but the index part still bothers me a lot. Since we're going to officially support this, I think we should help users with this in some way. Maybe some comments in the migration template might be enough, but I'm not sure. If you have suggestions, I'd love to hear them.

@tegon tegon added the PR attached label Apr 4, 2018

@kylefox

This comment has been minimized.

Copy link
Author

kylefox commented Apr 5, 2018

@tegon Thanks again for the thoughtful reply.

But for libraries, we have to think long-term, because we have to support any API we introduce for future versions.

Totally understand & appreciate that 👍

The Wiki is maintained by the community, which means someone needed this behavior and created this article

Good point, I hadn't considered this. I guess a wiki article doesn't necessarily mean it's "officially supported" functionality. Maybe I'll take a crack at updating this specific article.

are there advantages of using the same table for both models?

Sorry, I should clarify — I'm definitely not using the same table for different Devise users.

An online store like Shopify would be an example of what I'm doing. Here's a simplified example. Note the validates_uniqueness_of configuration:

class Shop < ApplicationRecord
  has_many :products
  has_many :customers
end

class Customer < ApplicationRecord
  belongs_to :shop

  devise :database_authenticatable, :registerable # ...etc...

  # This is the heart of the issue.
  # It's possible for "me@example.com" to have many unrelated Customer records.
  # me@example.com at Shop A, me@example.com at Shop B, etc.
  # So the unique constraint is [:email, :shop_id] rather than just :email
  validates_uniqueness_of :email, scope: :shop
end

Another example would be Slack — I can use the same email to login to two different teams using two different passwords. Hope that clarifies.

but the index part still bothers me a lot

Hmm, I hadn't considered the indexes — does that PR make any changes to the index? 🤔

I don't think the migration template should be changed. It should stay like this IMO:

add_index :users, :email, unique: true

It's easy enough to change in the migration, if needed. And worst-case scenario it can be changed to a composite index manually in a later migration. This could all be explained in any documentation that describes how to use the email_scope option.

@tegon

This comment has been minimized.

Copy link
Member

tegon commented Apr 5, 2018

@kylefox thanks for clarifying the use case for this. It makes a lot more sense now 👍

I agree with the index part, the best we can do is to document how the migration should be changed if the email_scope option is defined.

@hudakh

This comment has been minimized.

Copy link

hudakh commented Oct 22, 2018

I don't see any recent updates on this :( It would be very useful for us!

@tegon

This comment has been minimized.

Copy link
Member

tegon commented Dec 21, 2018

@hudakh The opened PR needs tests and some other changes before it can be merged.

@olbrich

This comment has been minimized.

Copy link

olbrich commented Dec 23, 2018

FYI, https://github.com/devise-security/devise-security will scope emails by authentication keys.

@tegon tegon removed the Discussion label Dec 27, 2018

@tegon tegon added Needs PR Feature and removed PR attached labels Jan 28, 2019

@theshashiverma

This comment has been minimized.

Copy link

theshashiverma commented Jan 31, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.