Commit 2afd95a0 authored by Douwe Maan's avatar Douwe Maan

Merge branch 'streamline-email-validation' into 'master'

Validate email addresses using Devise.email_regexp

Also:
- Get rid of legacy `:strict_mode`
- Get rid of custom `:email` validator
- Add some shared examples to spec emails validation

This supersedes !2754 and fixes #3851.

See merge request !2771
parents 0807bd51 b3635ee4
...@@ -20,6 +20,7 @@ v 8.5.0 (unreleased) ...@@ -20,6 +20,7 @@ v 8.5.0 (unreleased)
- Fix label links for a merge request pointing to issues list - Fix label links for a merge request pointing to issues list
- Don't vendor minified JS - Don't vendor minified JS
- Increase project import timeout to 15 minutes - Increase project import timeout to 15 minutes
- Be more permissive with email address validation: it only has to contain a single '@'
- Display 404 error on group not found - Display 404 error on group not found
- Track project import failure - Track project import failure
- Support Two-factor Authentication for LDAP users - Support Two-factor Authentication for LDAP users
......
...@@ -71,8 +71,8 @@ class ApplicationSetting < ActiveRecord::Base ...@@ -71,8 +71,8 @@ class ApplicationSetting < ActiveRecord::Base
url: true url: true
validates :admin_notification_email, validates :admin_notification_email,
allow_blank: true, email: true,
email: true allow_blank: true
validates :two_factor_grace_period, validates :two_factor_grace_period,
numericality: { greater_than_or_equal_to: 0 } numericality: { greater_than_or_equal_to: 0 }
......
...@@ -15,7 +15,7 @@ class Email < ActiveRecord::Base ...@@ -15,7 +15,7 @@ class Email < ActiveRecord::Base
belongs_to :user belongs_to :user
validates :user_id, presence: true validates :user_id, presence: true
validates :email, presence: true, email: { strict_mode: true }, uniqueness: true validates :email, presence: true, uniqueness: true, email: true
validate :unique_email, if: ->(email) { email.email_changed? } validate :unique_email, if: ->(email) { email.email_changed? }
before_validation :cleanup_email before_validation :cleanup_email
......
...@@ -39,7 +39,6 @@ class Member < ActiveRecord::Base ...@@ -39,7 +39,6 @@ class Member < ActiveRecord::Base
if: :invite? if: :invite?
}, },
email: { email: {
strict_mode: true,
allow_nil: true allow_nil: true
}, },
uniqueness: { uniqueness: {
......
...@@ -146,11 +146,8 @@ class User < ActiveRecord::Base ...@@ -146,11 +146,8 @@ class User < ActiveRecord::Base
# Validations # Validations
# #
validates :name, presence: true validates :name, presence: true
# Note that a 'uniqueness' and presence check is provided by devise :validatable for email. We do not need to validates :notification_email, presence: true, email: true
# duplicate that here as the validation framework will have duplicate errors in the event of a failure. validates :public_email, presence: true, uniqueness: true, email: true, allow_blank: true
validates :email, presence: true, email: { strict_mode: true }
validates :notification_email, presence: true, email: { strict_mode: true }
validates :public_email, presence: true, email: { strict_mode: true }, allow_blank: true, uniqueness: true
validates :bio, length: { maximum: 255 }, allow_blank: true validates :bio, length: { maximum: 255 }, allow_blank: true
validates :projects_limit, presence: true, numericality: { greater_than_or_equal_to: 0 } validates :projects_limit, presence: true, numericality: { greater_than_or_equal_to: 0 }
validates :username, validates :username,
......
# EmailValidator
#
# Based on https://github.com/balexand/email_validator
#
# Extended to use only strict mode with following allowed characters:
# ' - apostrophe
#
# See http://www.remote.org/jochen/mail/info/chars.html
#
class EmailValidator < ActiveModel::EachValidator class EmailValidator < ActiveModel::EachValidator
PATTERN = /\A\s*([-a-z0-9+._']{1,64})@((?:[-a-z0-9]+\.)+[a-z]{2,})\s*\z/i.freeze
def validate_each(record, attribute, value) def validate_each(record, attribute, value)
unless value =~ PATTERN record.errors.add(attribute, :invalid) unless value =~ Devise.email_regexp
record.errors.add(attribute, options[:message] || :invalid)
end
end end
end end
...@@ -74,6 +74,10 @@ describe ApplicationSetting, models: true do ...@@ -74,6 +74,10 @@ describe ApplicationSetting, models: true do
.only_integer .only_integer
.is_greater_than(0) .is_greater_than(0)
end end
it_behaves_like 'an object with email-formated attributes', :admin_notification_email do
subject { setting }
end
end end
context 'restricted signup domains' do context 'restricted signup domains' do
......
# == Schema Information
#
# Table name: emails
#
# id :integer not null, primary key
# user_id :integer not null
# email :string(255) not null
# created_at :datetime
# updated_at :datetime
#
require 'spec_helper'
describe Email, models: true do
describe 'validations' do
it_behaves_like 'an object with email-formated attributes', :email do
subject { build(:email) }
end
end
end
...@@ -31,6 +31,10 @@ describe Member, models: true do ...@@ -31,6 +31,10 @@ describe Member, models: true do
it { is_expected.to validate_presence_of(:source) } it { is_expected.to validate_presence_of(:source) }
it { is_expected.to validate_inclusion_of(:access_level).in_array(Gitlab::Access.values) } it { is_expected.to validate_inclusion_of(:access_level).in_array(Gitlab::Access.values) }
it_behaves_like 'an object with email-formated attributes', :invite_email do
subject { build(:project_member) }
end
context "when an invite email is provided" do context "when an invite email is provided" do
let(:member) { build(:project_member, invite_email: "user@example.com", user: nil) } let(:member) { build(:project_member, invite_email: "user@example.com", user: nil) }
...@@ -159,7 +163,7 @@ describe Member, models: true do ...@@ -159,7 +163,7 @@ describe Member, models: true do
describe "#generate_invite_token" do describe "#generate_invite_token" do
let!(:member) { create(:project_member, invite_email: "user@example.com", user: nil) } let!(:member) { create(:project_member, invite_email: "user@example.com", user: nil) }
it "sets the invite token" do it "sets the invite token" do
expect { member.generate_invite_token }.to change { member.invite_token} expect { member.generate_invite_token }.to change { member.invite_token}
end end
......
...@@ -119,37 +119,15 @@ describe User, models: true do ...@@ -119,37 +119,15 @@ describe User, models: true do
it { is_expected.to validate_length_of(:bio).is_within(0..255) } it { is_expected.to validate_length_of(:bio).is_within(0..255) }
describe 'email' do it_behaves_like 'an object with email-formated attributes', :email do
it 'accepts info@example.com' do subject { build(:user) }
user = build(:user, email: 'info@example.com') end
expect(user).to be_valid
end
it 'accepts info+test@example.com' do
user = build(:user, email: 'info+test@example.com')
expect(user).to be_valid
end
it "accepts o'reilly@example.com" do
user = build(:user, email: "o'reilly@example.com")
expect(user).to be_valid
end
it 'rejects test@test@example.com' do
user = build(:user, email: 'test@test@example.com')
expect(user).to be_invalid
end
it 'rejects mailto:test@example.com' do
user = build(:user, email: 'mailto:test@example.com')
expect(user).to be_invalid
end
it "rejects lol!'+=?><#$%^&*()@gmail.com" do it_behaves_like 'an object with email-formated attributes', :public_email, :notification_email do
user = build(:user, email: "lol!'+=?><#$%^&*()@gmail.com") subject { build(:user).tap { |user| user.emails << build(:email, email: email_value) } }
expect(user).to be_invalid end
end
describe 'email' do
context 'when no signup domains listed' do context 'when no signup domains listed' do
before { allow(current_application_settings).to receive(:restricted_signup_domains).and_return([]) } before { allow(current_application_settings).to receive(:restricted_signup_domains).and_return([]) }
it 'accepts any email' do it 'accepts any email' do
......
# Specifications for behavior common to all objects with an email attribute.
# Takes a list of email-format attributes and requires:
# - subject { "the object with a attribute= setter" }
# Note: You have access to `email_value` which is the email address value
# being currently tested).
shared_examples 'an object with email-formated attributes' do |*attributes|
attributes.each do |attribute|
describe "specifically its :#{attribute} attribute" do
%w[
info@example.com
info+test@example.com
o'reilly@example.com
mailto:test@example.com
lol!'+=?><#$%^&*()@gmail.com
].each do |valid_email|
context "with a value of '#{valid_email}'" do
let(:email_value) { valid_email }
it 'is valid' do
subject.send("#{attribute}=", valid_email)
expect(subject).to be_valid
end
end
end
%w[
foobar
test@test@example.com
].each do |invalid_email|
context "with a value of '#{invalid_email}'" do
let(:email_value) { invalid_email }
it 'is invalid' do
subject.send("#{attribute}=", invalid_email)
expect(subject).to be_invalid
end
end
end
end
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment