devise
devise copied to clipboard
heartcombo
Validatable should allow email uniqueness to be scoped
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!
👍 … makes real sense in case of «soft-deleted» authenticatables as well.
+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.
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).
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.
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.
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.
@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 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 "[email protected]" to have many unrelated Customer records.
# [email protected] at Shop A, [email protected] 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.
@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.
FYI, https://github.com/devise-security/devise-security will scope emails by authentication keys.
@kylefox @argent-smith @one-more-alex @tegon @hudakh please have a look on this https://github.com/plataformatec/devise/issues/5017
Hey @tegon . My coworker @flynfish had a need for this patch and opened a PR with tests. Would love to see it reviewed and this issue close out: #5094 Cheers!
@xhocquet @flynfish thanks, I've added it to my list and will review it when I can. Sorry for the delay.
Just chiming in to say that I'd find this feature incredibly useful as well :) Thanks everyone!
@tegon Do you have an ETA when this can be reviewed and https://github.com/plataformatec/devise/pull/5094 merged?
I would not encourage others to use public forks like above. Devise is heavily maintained and by using forked repos you open yourself up to security vulnerabilities over time.
@xhocquet Unfortunately, I don't have an ETA. I'm struggling to find the time to work on OSS lately, but since a lot of you are asking for this feature I'll try to review it this week.
@tegon I know quite a few of us would greatly appreciate that! Thanks for your time, I understand the struggle 😄
Since this gem is so crucial to the Ruby ecosystem, it may also be wise to consider bringing on some additional maintainers to spread out the load. I don't think you would have a problem finding willing contributors considering how foundational devise is for many Rails applications. Thanks again!
@DavidGeismarLtd there's an open PR but this is the gist of it at the moment: https://github.com/heartcombo/devise/pull/5094#issuecomment-754927228
