Merge branch 'rs-validators' into 'master'

Add more custom validators

These custom validators allow us to DRY up our models a bit.

See merge request !1944

app/models/application_setting.rb

@@ -43,12 +43,12 @@ class ApplicationSetting < ActiveRecord::Base
 
   validates :home_page_url,
     allow_blank: true,
-    format: { with: /\A#{URI.regexp(%w(http https))}\z/, message: "should be a valid url" },
+    url: true,
     if: :home_page_url_column_exist
 
   validates :after_sign_out_path,
     allow_blank: true,
-    format: { with: /\A#{URI.regexp(%w(http https))}\z/, message: "should be a valid url" }
+    url: true
 
   validates :admin_notification_email,
     allow_blank: true,

app/models/broadcast_message.rb

@@ -16,12 +16,12 @@
 class BroadcastMessage < ActiveRecord::Base
   include Sortable
 
-  validates :message, presence: true
+  validates :message,   presence: true
   validates :starts_at, presence: true
-  validates :ends_at, presence: true
+  validates :ends_at,   presence: true
 
-  validates :color, format: { with: /\A\#[0-9A-Fa-f]{3}{1,2}+\Z/ }, allow_blank: true
-  validates :font,  format: { with: /\A\#[0-9A-Fa-f]{3}{1,2}+\Z/ }, allow_blank: true
+  validates :color, allow_blank: true, color: true
+  validates :font,  allow_blank: true, color: true
 
   def self.current
     where("ends_at > :now AND starts_at < :now", now: Time.zone.now).last

app/models/ci/web_hook.rb

@@ -20,8 +20,7 @@ module Ci
     # HTTParty timeout
     default_timeout 10
 
-    validates :url, presence: true,
-                    format: { with: URI::regexp(%w(http https)), message: "should be a valid url" }
+    validates :url, presence: true, url: true
 
     def execute(data)
       parsed_url = URI.parse(url)

app/models/hooks/web_hook.rb

@@ -31,8 +31,7 @@ class WebHook < ActiveRecord::Base
   # HTTParty timeout
   default_timeout Gitlab.config.gitlab.webhook_timeout
 
-  validates :url, presence: true,
-                  format: { with: /\A#{URI.regexp(%w(http https))}\z/, message: "should be a valid url" }
+  validates :url, presence: true, url: true
 
   def execute(data, hook_name)
     parsed_url = URI.parse(url)

app/models/label.rb

@@ -27,9 +27,7 @@ class Label < ActiveRecord::Base
   has_many :label_links, dependent: :destroy
   has_many :issues, through: :label_links, source: :target, source_type: 'Issue'
 
-  validates :color,
-            format: { with: /\A#[0-9A-Fa-f]{6}\Z/ },
-            allow_blank: false
+  validates :color, color: true, allow_blank: false
   validates :project, presence: true, unless: Proc.new { |service| service.template? }
 
   # Don't allow '?', '&', and ',' for label titles

app/models/namespace.rb

@@ -23,19 +23,17 @@ class Namespace < ActiveRecord::Base
 
   validates :owner, presence: true, unless: ->(n) { n.type == "Group" }
   validates :name,
-    presence: true, uniqueness: true,
     length: { within: 0..255 },
-    format: { with: Gitlab::Regex.namespace_name_regex,
-              message: Gitlab::Regex.namespace_name_regex_message }
+    namespace_name: true,
+    presence: true,
+    uniqueness: true
 
   validates :description, length: { within: 0..255 }
   validates :path,
-    uniqueness: { case_sensitive: false },
-    presence: true,
     length: { within: 1..255 },
-    exclusion: { in: Gitlab::Blacklist.path },
-    format: { with: Gitlab::Regex.namespace_regex,
-              message: Gitlab::Regex.namespace_regex_message }
+    namespace: true,
+    presence: true,
+    uniqueness: { case_sensitive: false }
 
   delegate :name, to: :owner, allow_nil: true, prefix: true
 

app/models/note.rb

@@ -44,7 +44,7 @@ class Note < ActiveRecord::Base
   validates :note, :project, presence: true
   validates :note, uniqueness: { scope: [:author, :noteable_type, :noteable_id] }, if: ->(n) { n.is_award }
   validates :note, inclusion: { in: Emoji.emojis_names }, if: ->(n) { n.is_award }
-  validates :line_code, format: { with: /\A[a-z0-9]+_\d+_\d+\Z/ }, allow_blank: true
+  validates :line_code, line_code: true, allow_blank: true
   # Attachments are deprecated and are handled by Markdown uploader
   validates :attachment, file_size: { maximum: :max_attachment_size }
 

app/models/project.rb

@@ -152,7 +152,7 @@ class Project < ActiveRecord::Base
   validates_uniqueness_of :name, scope: :namespace_id
   validates_uniqueness_of :path, scope: :namespace_id
   validates :import_url,
-    format: { with: /\A#{URI.regexp(%w(ssh git http https))}\z/, message: 'should be a valid url' },
+    url: { protocols: %w(ssh git http https) },
     if: :external_import?
   validates :star_count, numericality: { greater_than_or_equal_to: 0 }
   validate :check_limit, on: :create

app/models/project_services/bamboo_service.rb

@@ -23,10 +23,7 @@ class BambooService < CiService
 
   prop_accessor :bamboo_url, :build_key, :username, :password
 
-  validates :bamboo_url,
-    presence: true,
-    format: { with: /\A#{URI.regexp}\z/ },
-    if: :activated?
+  validates :bamboo_url, presence: true, url: true, if: :activated?
   validates :build_key, presence: true, if: :activated?
   validates :username,
     presence: true,
@@ -84,7 +81,7 @@ class BambooService < CiService
   def supported_events
     %w(push)
   end
-  
+
   def build_info(sha)
     url = URI.parse("#{bamboo_url}/rest/api/latest/result?label=#{sha}")
 

app/models/project_services/drone_ci_service.rb

@@ -19,14 +19,11 @@
 #
 
 class DroneCiService < CiService
-  
+
   prop_accessor :drone_url, :token, :enable_ssl_verification
-  validates :drone_url,
-    presence: true,
-    format: { with: /\A#{URI.regexp(%w(http https))}\z/, message: "should be a valid url" }, if: :activated?
-  validates :token,
-    presence: true,
-    if: :activated?
+
+  validates :drone_url, presence: true, url: true, if: :activated?
+  validates :token, presence: true, if: :activated?
 
   after_save :compose_service_hook, if: :activated?
 
@@ -58,16 +55,16 @@ class DroneCiService < CiService
   end
 
   def merge_request_status_path(iid, sha = nil, ref = nil)
-    url = [drone_url, 
-           "gitlab/#{project.namespace.path}/#{project.path}/pulls/#{iid}", 
+    url = [drone_url,
+           "gitlab/#{project.namespace.path}/#{project.path}/pulls/#{iid}",
            "?access_token=#{token}"]
 
     URI.join(*url).to_s
   end
 
   def commit_status_path(sha, ref)
-    url = [drone_url, 
-           "gitlab/#{project.namespace.path}/#{project.path}/commits/#{sha}", 
+    url = [drone_url,
+           "gitlab/#{project.namespace.path}/#{project.path}/commits/#{sha}",
            "?branch=#{URI::encode(ref.to_s)}&access_token=#{token}"]
 
     URI.join(*url).to_s
@@ -114,15 +111,15 @@ class DroneCiService < CiService
   end
 
   def merge_request_page(iid, sha, ref)
-    url = [drone_url, 
+    url = [drone_url,
            "gitlab/#{project.namespace.path}/#{project.path}/redirect/pulls/#{iid}"]
 
     URI.join(*url).to_s
   end
 
   def commit_page(sha, ref)
-    url = [drone_url, 
-           "gitlab/#{project.namespace.path}/#{project.path}/redirect/commits/#{sha}", 
+    url = [drone_url,
+           "gitlab/#{project.namespace.path}/#{project.path}/redirect/commits/#{sha}",
            "?branch=#{URI::encode(ref.to_s)}"]
 
     URI.join(*url).to_s
@@ -163,10 +160,10 @@ class DroneCiService < CiService
   end
 
   def push_valid?(data)
-    opened_merge_requests = project.merge_requests.opened.where(source_project_id: project.id, 
+    opened_merge_requests = project.merge_requests.opened.where(source_project_id: project.id,
                                                                 source_branch: Gitlab::Git.ref_name(data[:ref]))
 
-    opened_merge_requests.empty? && data[:total_commits_count] > 0 && 
+    opened_merge_requests.empty? && data[:total_commits_count] > 0 &&
       !Gitlab::Git.blank_ref?(data[:after])
   end
 

app/models/project_services/external_wiki_service.rb

@@ -22,10 +22,8 @@ class ExternalWikiService < Service
   include HTTParty
 
   prop_accessor :external_wiki_url
-  validates :external_wiki_url,
-            presence: true,
-            format: { with: /\A#{URI.regexp}\z/ },
-            if: :activated?
+
+  validates :external_wiki_url, presence: true, url: true, if: :activated?
 
   def title
     'External Wiki'

app/models/project_services/teamcity_service.rb

@@ -23,16 +23,16 @@ class TeamcityService < CiService
 
   prop_accessor :teamcity_url, :build_type, :username, :password
 
-  validates :teamcity_url,
-    presence: true,
-    format: { with: /\A#{URI.regexp}\z/ }, if: :activated?
+  validates :teamcity_url, presence: true, url: true, if: :activated?
   validates :build_type, presence: true, if: :activated?
   validates :username,
     presence: true,
-    if: ->(service) { service.password? }, if: :activated?
+    if: ->(service) { service.password? },
+    if: :activated?
   validates :password,
     presence: true,
-    if: ->(service) { service.username? }, if: :activated?
+    if: ->(service) { service.username? },
+    if: :activated?
 
   attr_accessor :response
 

app/models/sent_notification.rb

@@ -21,7 +21,7 @@ class SentNotification < ActiveRecord::Base
   validates :reply_key, uniqueness: true
   validates :noteable_id, presence: true, unless: :for_commit?
   validates :commit_id, presence: true, if: :for_commit?
-  validates :line_code, format: { with: /\A[a-z0-9]+_\d+_\d+\Z/ }, allow_blank: true
+  validates :line_code, line_code: true, allow_blank: true
 
   class << self
     def reply_key

app/models/user.rb

@@ -148,11 +148,9 @@ class User < ActiveRecord::Base
   validates :bio, length: { maximum: 255 }, allow_blank: true
   validates :projects_limit, presence: true, numericality: { greater_than_or_equal_to: 0 }
   validates :username,
+    namespace: true,
     presence: true,
-    uniqueness: { case_sensitive: false },
-    exclusion: { in: Gitlab::Blacklist.path },
-    format: { with: Gitlab::Regex.namespace_regex,
-              message: Gitlab::Regex.namespace_regex_message }
+    uniqueness: { case_sensitive: false }
 
   validates :notification_level, inclusion: { in: Notification.notification_levels }, presence: true
   validate :namespace_uniq, if: ->(user) { user.username_changed? }

app/validators/color_validator.rb

@@ -0,0 +1,20 @@
+# ColorValidator
+#
+# Custom validator for web color codes. It requires the leading hash symbol and
+# will accept RGB triplet or hexadecimal formats.
+#
+# Example:
+#
+#   class User < ActiveRecord::Base
+#     validates :background_color, allow_blank: true, color: true
+#   end
+#
+class ColorValidator < ActiveModel::EachValidator
+  PATTERN = /\A\#[0-9A-Fa-f]{3}{1,2}+\Z/.freeze
+
+  def validate_each(record, attribute, value)
+    unless value =~ PATTERN
+      record.errors.add(attribute, "must be a valid color code")
+    end
+  end
+end

app/validators/email_validator.rb

@@ -1,3 +1,5 @@
+# EmailValidator
+#
 # Based on https://github.com/balexand/email_validator
 #
 # Extended to use only strict mode with following allowed characters:
@@ -6,15 +8,10 @@
 # See http://www.remote.org/jochen/mail/info/chars.html
 #
 class EmailValidator < ActiveModel::EachValidator
-  @@default_options = {}
-
-  def self.default_options
-    @@default_options
-  end
+  PATTERN = /\A\s*([-a-z0-9+._']{1,64})@((?:[-a-z0-9]+\.)+[a-z]{2,})\s*\z/i.freeze
 
   def validate_each(record, attribute, value)
-    options = @@default_options.merge(self.options)
-    unless value =~ /\A\s*([-a-z0-9+._']{1,64})@((?:[-a-z0-9]+\.)+[a-z]{2,})\s*\z/i
+    unless value =~ PATTERN
       record.errors.add(attribute, options[:message] || :invalid)
     end
   end

app/validators/line_code_validator.rb

@@ -0,0 +1,12 @@
+# LineCodeValidator
+#
+# Custom validator for GitLab line codes.
+class LineCodeValidator < ActiveModel::EachValidator
+  PATTERN = /\A[a-z0-9]+_\d+_\d+\z/.freeze
+
+  def validate_each(record, attribute, value)
+    unless value =~ PATTERN
+      record.errors.add(attribute, "must be a valid line code")
+    end
+  end
+end

app/validators/namespace_name_validator.rb

@@ -0,0 +1,10 @@
+# NamespaceNameValidator
+#
+# Custom validator for GitLab namespace name strings.
+class NamespaceNameValidator < ActiveModel::EachValidator
+  def validate_each(record, attribute, value)
+    unless value =~ Gitlab::Regex.namespace_name_regex
+      record.errors.add(attribute, Gitlab::Regex.namespace_name_regex_message)
+    end
+  end
+end

app/validators/namespace_validator.rb

@@ -0,0 +1,50 @@
+# NamespaceValidator
+#
+# Custom validator for GitLab namespace values.
+#
+# Values are checked for formatting and exclusion from a list of reserved path
+# names.
+class NamespaceValidator < ActiveModel::EachValidator
+  RESERVED = %w(
+    admin
+    all
+    assets
+    ci
+    dashboard
+    files
+    groups
+    help
+    hooks
+    issues
+    merge_requests
+    notes
+    profile
+    projects
+    public
+    repository
+    s
+    search
+    services
+    snippets
+    teams
+    u
+    unsubscribes
+    users
+  ).freeze
+
+  def validate_each(record, attribute, value)
+    unless value =~ Gitlab::Regex.namespace_regex
+      record.errors.add(attribute, Gitlab::Regex.namespace_regex_message)
+    end
+
+    if reserved?(value)
+      record.errors.add(attribute, "#{value} is a reserved name")
+    end
+  end
+
+  private
+
+  def reserved?(value)
+    RESERVED.include?(value)
+  end
+end

app/validators/url_validator.rb

@@ -0,0 +1,36 @@
+# UrlValidator
+#
+# Custom validator for URLs.
+#
+# By default, only URLs for the HTTP(S) protocols will be considered valid.
+# Provide a `:protocols` option to configure accepted protocols.
+#
+# Example:
+#
+#   class User < ActiveRecord::Base
+#     validates :personal_url, url: true
+#
+#     validates :ftp_url, url: { protocols: %w(ftp) }
+#
+#     validates :git_url, url: { protocols: %w(http https ssh git) }
+#   end
+#
+class UrlValidator < ActiveModel::EachValidator
+  def validate_each(record, attribute, value)
+    unless valid_url?(value)
+      record.errors.add(attribute, "must be a valid URL")
+    end
+  end
+
+  private
+
+  def default_options
+    @default_options ||= { protocols: %w(http https) }
+  end
+
+  def valid_url?(value)
+    options = default_options.merge(self.options)
+
+    value =~ /\A#{URI.regexp(options[:protocols])}\z/
+  end
+end

features/steps/admin/labels.rb

@@ -71,7 +71,7 @@ class Spinach::Features::AdminIssuesLabels < Spinach::FeatureSteps
 
   step 'I should see label color error message' do
     page.within '.label-form' do
-      expect(page).to have_content 'Color is invalid'
+      expect(page).to have_content 'Color must be a valid color code'
     end
   end
 

features/steps/project/issues/labels.rb

@@ -55,7 +55,7 @@ class Spinach::Features::ProjectIssuesLabels < Spinach::FeatureSteps
 
   step 'I should see label color error message' do
     page.within '.label-form' do
-      expect(page).to have_content 'Color is invalid'
+      expect(page).to have_content 'Color must be a valid color code'
     end
   end
 

lib/gitlab/blacklist.rb

@@ -1,34 +0,0 @@
-module Gitlab
-  module Blacklist
-    extend self
-
-    def path
-      %w(
-        admin
-        dashboard
-        files
-        groups
-        help
-        profile
-        projects
-        search
-        public
-        assets
-        u
-        s
-        teams
-        merge_requests
-        issues
-        users
-        snippets
-        services
-        repository
-        hooks
-        notes
-        unsubscribes
-        all
-        ci
-      )
-    end
-  end
-end

spec/models/application_setting_spec.rb

@@ -36,6 +36,22 @@ describe ApplicationSetting, models: true do
 
   it { expect(setting).to be_valid }
 
+  describe 'validations' do
+    let(:http)  { 'http://example.com' }
+    let(:https) { 'https://example.com' }
+    let(:ftp)   { 'ftp://example.com' }
+
+    it { is_expected.to allow_value(nil).for(:home_page_url) }
+    it { is_expected.to allow_value(http).for(:home_page_url) }
+    it { is_expected.to allow_value(https).for(:home_page_url) }
+    it { is_expected.not_to allow_value(ftp).for(:home_page_url) }
+
+    it { is_expected.to allow_value(nil).for(:after_sign_out_path) }
+    it { is_expected.to allow_value(http).for(:after_sign_out_path) }
+    it { is_expected.to allow_value(https).for(:after_sign_out_path) }
+    it { is_expected.not_to allow_value(ftp).for(:after_sign_out_path) }
+  end
+
   context 'restricted signup domains' do
     it 'set single domain' do
       setting.restricted_signup_domains_raw = 'example.com'

spec/models/broadcast_message_spec.rb

@@ -20,6 +20,21 @@ describe BroadcastMessage do
 
   it { is_expected.to be_valid }
 
+  describe 'validations' do
+    let(:triplet) { '#000' }
+    let(:hex)     { '#AABBCC' }
+
+    it { is_expected.to allow_value(nil).for(:color) }
+    it { is_expected.to allow_value(triplet).for(:color) }
+    it { is_expected.to allow_value(hex).for(:color) }
+    it { is_expected.not_to allow_value('000').for(:color) }
+
+    it { is_expected.to allow_value(nil).for(:font) }
+    it { is_expected.to allow_value(triplet).for(:font) }
+    it { is_expected.to allow_value(hex).for(:font) }
+    it { is_expected.not_to allow_value('000').for(:font) }
+  end
+
   describe :current do
     it "should return last message if time match" do
       broadcast_message = create(:broadcast_message, starts_at: Time.now.yesterday, ends_at: Time.now.tomorrow)

spec/models/user_spec.rb

@@ -91,7 +91,23 @@ describe User do
   end
 
   describe 'validations' do
-    it { is_expected.to validate_presence_of(:username) }
+    describe 'username' do
+      it 'validates presence' do
+        expect(subject).to validate_presence_of(:username)
+      end
+
+      it 'rejects blacklisted names' do
+        user = build(:user, username: 'dashboard')
+
+        expect(user).not_to be_valid
+        expect(user.errors.values).to eq [['dashboard is a reserved name']]
+      end
+
+      it 'validates uniqueness' do
+        expect(subject).to validate_uniqueness_of(:username)
+      end
+    end
+
     it { is_expected.to validate_presence_of(:projects_limit) }
     it { is_expected.to validate_numericality_of(:projects_limit) }
     it { is_expected.to allow_value(0).for(:projects_limit) }

spec/requests/api/labels_spec.rb

@@ -47,7 +47,7 @@ describe API::API, api: true  do
            name: 'Foo',
            color: '#FFAA'
       expect(response.status).to eq(400)
-      expect(json_response['message']['color']).to eq(['is invalid'])
+      expect(json_response['message']['color']).to eq(['must be a valid color code'])
     end
 
     it 'should return 400 for too long color code' do
@@ -55,7 +55,7 @@ describe API::API, api: true  do
            name: 'Foo',
            color: '#FFAAFFFF'
       expect(response.status).to eq(400)
-      expect(json_response['message']['color']).to eq(['is invalid'])
+      expect(json_response['message']['color']).to eq(['must be a valid color code'])
     end
 
     it 'should return 400 for invalid name' do
@@ -151,12 +151,12 @@ describe API::API, api: true  do
       expect(json_response['message']['title']).to eq(['is invalid'])
     end
 
-    it 'should return 400 for invalid name' do
+    it 'should return 400 when color code is too short' do
       put api("/projects/#{project.id}/labels", user),
           name: 'label1',
           color: '#FF'
       expect(response.status).to eq(400)
-      expect(json_response['message']['color']).to eq(['is invalid'])
+      expect(json_response['message']['color']).to eq(['must be a valid color code'])
     end
 
     it 'should return 400 for too long color code' do
@@ -164,7 +164,7 @@ describe API::API, api: true  do
            name: 'Foo',
            color: '#FFAAFFFF'
       expect(response.status).to eq(400)
-      expect(json_response['message']['color']).to eq(['is invalid'])
+      expect(json_response['message']['color']).to eq(['must be a valid color code'])
     end
   end
 end

spec/requests/api/users_spec.rb

@@ -153,7 +153,7 @@ describe API::API, api: true  do
       expect(json_response['message']['projects_limit']).
         to eq(['must be greater than or equal to 0'])
       expect(json_response['message']['username']).
-        to eq([Gitlab::Regex.send(:namespace_regex_message)])
+        to eq([Gitlab::Regex.namespace_regex_message])
     end
 
     it "shouldn't available for non admin users" do
@@ -296,7 +296,7 @@ describe API::API, api: true  do
       expect(json_response['message']['projects_limit']).
         to eq(['must be greater than or equal to 0'])
       expect(json_response['message']['username']).
-        to eq([Gitlab::Regex.send(:namespace_regex_message)])
+        to eq([Gitlab::Regex.namespace_regex_message])
     end
 
     context "with existing user" do