Merge branch 'username-period' into 'master'

Don't allow username to end in period.

The current behavior doesn't do username referencing and mentioning in sentences like "I discussed with with @douwe." since `douwe.` is matched as a username.

Addresses private issue https://dev.gitlab.org/gitlab/gitlabhq/issues/2174.

See merge request !438

16 files changed, 179 insertions, 98 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 6d6b114a8b9..6e963fe965d 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -53,6 +53,8 @@ v 7.10.0 (unreleased)
   - Fix admin user projects lists.
   - Don't leak private group existence by redirecting from namespace controller to group controller.
   - Ability to skip some items from backup (database, respositories or uploads)
+  - Fix "Hello @username." references not working by no longer allowing usernames to end in period.
+
 
 v 7.9.2
   - Contains no changes
diff --git a/app/models/namespace.rb b/app/models/namespace.rb
index 35280889a86..dd74165f887 100644
--- a/app/models/namespace.rb
+++ b/app/models/namespace.rb
@@ -24,8 +24,8 @@ class Namespace < ActiveRecord::Base
   validates :name,
     presence: true, uniqueness: true,
     length: { within: 0..255 },
-    format: { with: Gitlab::Regex.name_regex,
-              message: Gitlab::Regex.name_regex_message }
+    format: { with: Gitlab::Regex.namespace_name_regex,
+              message: Gitlab::Regex.namespace_name_regex_message }
 
   validates :description, length: { within: 0..255 }
   validates :path,
@@ -33,8 +33,8 @@ class Namespace < ActiveRecord::Base
     presence: true,
     length: { within: 1..255 },
     exclusion: { in: Gitlab::Blacklist.path },
-    format: { with: Gitlab::Regex.path_regex,
-              message: Gitlab::Regex.path_regex_message }
+    format: { with: Gitlab::Regex.namespace_regex,
+              message: Gitlab::Regex.namespace_regex_message }
 
   delegate :name, to: :owner, allow_nil: true, prefix: true
 
@@ -44,21 +44,36 @@ class Namespace < ActiveRecord::Base
 
   scope :root, -> { where('type IS NULL') }
 
-  def self.by_path(path)
-    where('lower(path) = :value', value: path.downcase).first
-  end
+  class << self
+    def by_path(path)
+      where('lower(path) = :value', value: path.downcase).first
+    end
 
-  # Case insensetive search for namespace by path or name
-  def self.find_by_path_or_name(path)
-    find_by("lower(path) = :path OR lower(name) = :path", path: path.downcase)
-  end
+    # Case insensetive search for namespace by path or name
+    def find_by_path_or_name(path)
+      find_by("lower(path) = :path OR lower(name) = :path", path: path.downcase)
+    end
 
-  def self.search(query)
-    where("name LIKE :query OR path LIKE :query", query: "%#{query}%")
-  end
+    def search(query)
+      where("name LIKE :query OR path LIKE :query", query: "%#{query}%")
+    end
+
+    def clean_path(path)
+      path.gsub!(/@.*\z/,             "")
+      path.gsub!(/\.git\z/,           "")
+      path.gsub!(/\A-/,               "")
+      path.gsub!(/.\z/,               "")
+      path.gsub!(/[^a-zA-Z0-9_\-\.]/, "")
+
+      counter = 0
+      base = path
+      while Namespace.by_path(path).present?
+        counter += 1
+        path = "#{base}#{counter}"
+      end
 
-  def self.global_id
-    'GLN'
+      path
+    end
   end
 
   def to_param
diff --git a/app/models/project.rb b/app/models/project.rb
index c50b8a12621..79572f255db 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -124,12 +124,12 @@ class Project < ActiveRecord::Base
     presence: true,
     length: { within: 0..255 },
     format: { with: Gitlab::Regex.project_name_regex,
-              message: Gitlab::Regex.project_regex_message }
+              message: Gitlab::Regex.project_name_regex_message }
   validates :path,
     presence: true,
     length: { within: 0..255 },
-    format: { with: Gitlab::Regex.path_regex,
-              message: Gitlab::Regex.path_regex_message }
+    format: { with: Gitlab::Regex.project_path_regex,
+              message: Gitlab::Regex.project_path_regex_message }
   validates :issues_enabled, :merge_requests_enabled,
             :wiki_enabled, inclusion: { in: [true, false] }
   validates :issues_tracker_id, length: { maximum: 255 }, allow_blank: true
diff --git a/app/models/snippet.rb b/app/models/snippet.rb
index 3fb2ec1d66c..b35e72c4bdb 100644
--- a/app/models/snippet.rb
+++ b/app/models/snippet.rb
@@ -33,8 +33,8 @@ class Snippet < ActiveRecord::Base
   validates :file_name,
     presence: true,
     length: { within: 0..255 },
-    format: { with: Gitlab::Regex.path_regex,
-              message: Gitlab::Regex.path_regex_message }
+    format: { with: Gitlab::Regex.file_name_regex,
+              message: Gitlab::Regex.file_name_regex_message }
   validates :content, presence: true
   validates :visibility_level, inclusion: { in: Gitlab::VisibilityLevel.values }
 
diff --git a/app/models/user.rb b/app/models/user.rb
index 979150b4d68..515f29ea0ba 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -129,8 +129,8 @@ class User < ActiveRecord::Base
     presence: true,
     uniqueness: { case_sensitive: false },
     exclusion: { in: Gitlab::Blacklist.path },
-    format: { with: Gitlab::Regex.username_regex,
-              message: Gitlab::Regex.username_regex_message }
+    format: { with: Gitlab::Regex.namespace_regex,
+              message: Gitlab::Regex.namespace_regex_message }
 
   validates :notification_level, inclusion: { in: Notification.notification_levels }, presence: true
   validate :namespace_uniq, if: ->(user) { user.username_changed? }
@@ -229,22 +229,6 @@ class User < ActiveRecord::Base
     def build_user(attrs = {})
       User.new(attrs)
     end
-
-    def clean_username(username)
-      username.gsub!(/@.*\z/,             "")
-      username.gsub!(/\.git\z/,           "")
-      username.gsub!(/\A-/,               "")
-      username.gsub!(/[^a-zA-Z0-9_\-\.]/, "")
-
-      counter = 0
-      base = username
-      while User.by_login(username).present? || Namespace.by_path(username).present?
-        counter += 1
-        username = "#{base}#{counter}"
-      end
-
-      username
-    end
   end
 
   #
diff --git a/app/services/files/create_service.rb b/app/services/files/create_service.rb
index eeafefc25af..23833aa78ec 100644
--- a/app/services/files/create_service.rb
+++ b/app/services/files/create_service.rb
@@ -12,10 +12,10 @@ module Files
       file_name = File.basename(path)
       file_path = path
 
-      unless file_name =~ Gitlab::Regex.path_regex
+      unless file_name =~ Gitlab::Regex.file_name_regex
         return error(
           'Your changes could not be committed, because the file name ' +
-          Gitlab::Regex.path_regex_message
+          Gitlab::Regex.file_name_regex_message
         )
       end
 
diff --git a/db/migrate/20150324133047_remove_periods_at_ends_of_usernames.rb b/db/migrate/20150324133047_remove_periods_at_ends_of_usernames.rb
new file mode 100644
index 00000000000..7ce53c2a0d6
--- /dev/null
+++ b/db/migrate/20150324133047_remove_periods_at_ends_of_usernames.rb
@@ -0,0 +1,76 @@
+class RemovePeriodsAtEndsOfUsernames < ActiveRecord::Migration
+  include Gitlab::ShellAdapter
+
+  class Namespace < ActiveRecord::Base
+    class << self
+      def by_path(path)
+        where('lower(path) = :value', value: path.downcase).first
+      end
+
+      def clean_path(path)
+        path = path.dup
+        path.gsub!(/@.*\z/,             "")
+        path.gsub!(/\.git\z/,           "")
+        path.gsub!(/\A-/,               "")
+        path.gsub!(/.\z/,               "")
+        path.gsub!(/[^a-zA-Z0-9_\-\.]/, "")
+
+        counter = 0
+        base = path
+        while Namespace.by_path(path).present?
+          counter += 1
+          path = "#{base}#{counter}"
+        end
+
+        path
+      end
+    end
+  end
+
+  def up
+    changed_paths = {}
+
+    select_all("SELECT id, username FROM users WHERE username LIKE '%.'").each do |user|
+      username_was = user["username"]
+      username = Namespace.clean_path(username_was)
+      changed_paths[username_was] = username
+
+      username = quote_string(username)
+      execute "UPDATE users SET username = '#{username}' WHERE id = #{user["id"]}"
+      execute "UPDATE namespaces SET path = '#{username}', name = '#{username}' WHERE type IS NULL AND owner_id = #{user["id"]}"
+    end
+
+    select_all("SELECT id, path FROM namespaces WHERE type = 'Group' AND path LIKE '%.'").each do |group|
+      path_was = group["path"]
+      path = Namespace.clean_path(path_was)
+      changed_paths[path_was] = path
+
+      path = quote_string(path)
+      execute "UPDATE namespaces SET path = '#{path}' WHERE id = #{group["id"]}"
+    end
+
+    changed_paths.each do |path_was, path|
+      if gitlab_shell.mv_namespace(path_was, path)
+        # If repositories moved successfully we need to remove old satellites
+        # and send update instructions to users.
+        # However we cannot allow rollback since we moved namespace dir
+        # So we basically we mute exceptions in next actions
+        begin
+          gitlab_shell.rm_satellites(path_was)
+          # We cannot send update instructions since models and mailers
+          # can't safely be used from migrations as they may be written for 
+          # later versions of the database.
+          # send_update_instructions
+        rescue
+          # Returning false does not rollback after_* transaction but gives
+          # us information about failing some of tasks
+          false
+        end
+      else
+        # if we cannot move namespace directory we should rollback
+        # db changes in order to prevent out of sync between db and fs
+        raise Exception.new('namespace directory cannot be moved')
+      end
+    end
+  end
+end
diff --git a/features/steps/project/source/browse_files.rb b/features/steps/project/source/browse_files.rb
index 557555aee58..caf6c73ee06 100644
--- a/features/steps/project/source/browse_files.rb
+++ b/features/steps/project/source/browse_files.rb
@@ -74,7 +74,7 @@ class Spinach::Features::ProjectSourceBrowseFiles < Spinach::FeatureSteps
   end
 
   step 'I fill the new file name with an illegal name' do
-    fill_in :file_name, with: '.git'
+    fill_in :file_name, with: 'Spaces Not Allowed'
   end
 
   step 'I fill the commit message' do
diff --git a/lib/gitlab/markdown.rb b/lib/gitlab/markdown.rb
index 1dbea48ac14..f1e2ae74a3a 100644
--- a/lib/gitlab/markdown.rb
+++ b/lib/gitlab/markdown.rb
@@ -161,7 +161,7 @@ module Gitlab
       text
     end
 
-    NAME_STR = '[a-zA-Z0-9_][a-zA-Z0-9_\-\.]*'
+    NAME_STR = Gitlab::Regex::NAMESPACE_REGEX_STR
     PROJ_STR = "(?<project>#{NAME_STR}/#{NAME_STR})"
 
     REFERENCE_PATTERN = %r{
diff --git a/lib/gitlab/oauth/user.rb b/lib/gitlab/oauth/user.rb
index c023d275703..2f5c217d764 100644
--- a/lib/gitlab/oauth/user.rb
+++ b/lib/gitlab/oauth/user.rb
@@ -86,7 +86,7 @@ module Gitlab
       def user_attributes
         {
           name:                       auth_hash.name,
-          username:                   ::User.clean_username(auth_hash.username),
+          username:                   ::Namespace.clean_path(auth_hash.username),
           email:                      auth_hash.email,
           password:                   auth_hash.password,
           password_confirmation:      auth_hash.password,
diff --git a/lib/gitlab/regex.rb b/lib/gitlab/regex.rb
index cf6e260f257..0571574aa4f 100644
--- a/lib/gitlab/regex.rb
+++ b/lib/gitlab/regex.rb
@@ -2,49 +2,66 @@ module Gitlab
   module Regex
     extend self
 
-    def username_regex
-      default_regex
+    NAMESPACE_REGEX_STR = '(?:[a-zA-Z0-9_\.][a-zA-Z0-9_\-\.]*[a-zA-Z0-9_\-]|[a-zA-Z0-9_])'.freeze
+
+    def namespace_regex
+      @namespace_regex ||= /\A#{NAMESPACE_REGEX_STR}\z/.freeze
+    end
+
+    def namespace_regex_message
+      "can contain only letters, digits, '_', '-' and '.'. " \
+      "Cannot start with '-' or end in '.'." \
+    end
+
+
+    def namespace_name_regex
+      @namespace_name_regex ||= /\A[a-zA-Z0-9_\-\. ]*\z/.freeze
     end
 
-    def username_regex_message
-      default_regex_message
+    def namespace_name_regex_message
+      "can contain only letters, digits, '_', '-', '.' and space."
     end
 
+
     def project_name_regex
-      /\A[a-zA-Z0-9_.][a-zA-Z0-9_\-\. ]*\z/
+      @project_name_regex ||= /\A[a-zA-Z0-9_.][a-zA-Z0-9_\-\. ]*\z/.freeze
     end
 
-    def project_regex_message
-      "can contain only letters, digits, '_', '-' and '.' and space. " \
+    def project_name_regex_message
+      "can contain only letters, digits, '_', '-', '.' and space. " \
       "It must start with letter, digit or '_'."
     end
 
-    def name_regex
-      /\A[a-zA-Z0-9_\-\. ]*\z/
+
+    def project_path_regex
+      @project_path_regex ||= /\A[a-zA-Z0-9_.][a-zA-Z0-9_\-\.]*(?<!\.git)\z/.freeze
     end
 
-    def name_regex_message
-      "can contain only letters, digits, '_', '-' and '.' and space."
+    def project_path_regex_message
+      "can contain only letters, digits, '_', '-' and '.'. " \
+      "Cannot start with '-' or end in '.git'" \
     end
 
-    def path_regex
-      default_regex
+
+    def file_name_regex
+      @file_name_regex ||= /\A[a-zA-Z0-9_\-\.]*\z/.freeze
     end
 
-    def path_regex_message
-      default_regex_message
+    def file_name_regex_message
+      "can contain only letters, digits, '_', '-' and '.'. "
     end
 
+
     def archive_formats_regex
-      #|zip|tar|    tar.gz    |         tar.bz2         |
-      /(zip|tar|tar\.gz|tgz|gz|tar\.bz2|tbz|tbz2|tb2|bz2)/
+      #                           |zip|tar|    tar.gz    |         tar.bz2         |
+      @archive_formats_regex ||= /(zip|tar|tar\.gz|tgz|gz|tar\.bz2|tbz|tbz2|tb2|bz2)/.freeze
     end
 
     def git_reference_regex
       # Valid git ref regex, see:
       # https://www.kernel.org/pub/software/scm/git/docs/git-check-ref-format.html
 
-      %r{
+      @git_reference_regex ||= %r{
         (?!
            (?# doesn't begins with)
            \/|                    (?# rule #6)
@@ -60,18 +77,7 @@ module Gitlab
         (?# doesn't end with)
         (?<!\.lock)               (?# rule #1)
         (?<![\/.])                (?# rule #6-7)
-      }x
-    end
-
-    protected
-
-    def default_regex_message
-      "can contain only letters, digits, '_', '-' and '.'. " \
-      "Cannot start with '-' or end in '.git'" \
-    end
-
-    def default_regex
-      /\A[a-zA-Z0-9_.][a-zA-Z0-9_\-\.]*(?<!\.git)\z/
+      }x.freeze
     end
   end
 end
diff --git a/spec/lib/gitlab/regex_spec.rb b/spec/lib/gitlab/regex_spec.rb
index 1db9f15b790..727884c41c5 100644
--- a/spec/lib/gitlab/regex_spec.rb
+++ b/spec/lib/gitlab/regex_spec.rb
@@ -1,14 +1,14 @@
 require 'spec_helper'
 
 describe Gitlab::Regex do
-  describe 'path regex' do
-    it { expect('gitlab-ce').to match(Gitlab::Regex.path_regex) }
-    it { expect('gitlab_git').to match(Gitlab::Regex.path_regex) }
-    it { expect('_underscore.js').to match(Gitlab::Regex.path_regex) }
-    it { expect('100px.com').to match(Gitlab::Regex.path_regex) }
-    it { expect('?gitlab').not_to match(Gitlab::Regex.path_regex) }
-    it { expect('git lab').not_to match(Gitlab::Regex.path_regex) }
-    it { expect('gitlab.git').not_to match(Gitlab::Regex.path_regex) }
+  describe 'project path regex' do
+    it { expect('gitlab-ce').to match(Gitlab::Regex.project_path_regex) }
+    it { expect('gitlab_git').to match(Gitlab::Regex.project_path_regex) }
+    it { expect('_underscore.js').to match(Gitlab::Regex.project_path_regex) }
+    it { expect('100px.com').to match(Gitlab::Regex.project_path_regex) }
+    it { expect('?gitlab').not_to match(Gitlab::Regex.project_path_regex) }
+    it { expect('git lab').not_to match(Gitlab::Regex.project_path_regex) }
+    it { expect('gitlab.git').not_to match(Gitlab::Regex.project_path_regex) }
   end
 
   describe 'project name regex' do
diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb
index ed6845c82cc..e87432fdf62 100644
--- a/spec/models/namespace_spec.rb
+++ b/spec/models/namespace_spec.rb
@@ -33,8 +33,6 @@ describe Namespace do
     it { is_expected.to respond_to(:to_param) }
   end
 
-  it { expect(Namespace.global_id).to eq('GLN') }
-
   describe :to_param do
     it { expect(namespace.to_param).to eq(namespace.path) }
   end
@@ -85,4 +83,14 @@ describe Namespace do
     it { expect(Namespace.find_by_path_or_name('WOW')).to eq(@namespace) }
     it { expect(Namespace.find_by_path_or_name('unknown')).to eq(nil) }
   end
+
+  describe ".clean_path" do
+
+    let!(:user)       { create(:user, username: "johngitlab-etc") }
+    let!(:namespace)  { create(:namespace, path: "JohnGitLab-etc1") }
+
+    it "cleans the path and makes sure it's available" do
+      expect(Namespace.clean_path("-john+gitlab-ETC%.git@gmail.com")).to eq("johngitlab-ETC2")
+    end
+  end
 end
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 10e90cae143..24384e8bf22 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -307,16 +307,6 @@ describe User do
     end
   end
 
-  describe ".clean_username" do
-
-    let!(:user) { create(:user, username: "johngitlab-etc") }
-    let!(:namespace) { create(:namespace, path: "JohnGitLab-etc1") }
-
-    it "cleans a username and makes sure it's available" do
-      expect(User.clean_username("-john+gitlab-ETC%.git@gmail.com")).to eq("johngitlab-ETC2")
-    end
-  end
-
   describe 'all_ssh_keys' do
     it { is_expected.to have_many(:keys).dependent(:destroy) }
 
diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb
index f28dfea3ccf..b713f1fe898 100644
--- a/spec/requests/api/projects_spec.rb
+++ b/spec/requests/api/projects_spec.rb
@@ -247,12 +247,12 @@ describe API::API, api: true  do
       expect(json_response['message']['name']).to eq([
         'can\'t be blank',
         'is too short (minimum is 0 characters)',
-        Gitlab::Regex.project_regex_message
+        Gitlab::Regex.project_name_regex_message
       ])
       expect(json_response['message']['path']).to eq([
         'can\'t be blank',
         'is too short (minimum is 0 characters)',
-        Gitlab::Regex.send(:default_regex_message)
+        Gitlab::Regex.send(:project_path_regex_message)
       ])
     end
 
diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb
index 081400cdedd..e6d5545f812 100644
--- a/spec/requests/api/users_spec.rb
+++ b/spec/requests/api/users_spec.rb
@@ -140,7 +140,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(:default_regex_message)])
+          to eq([Gitlab::Regex.send(:namespace_regex_message)])
     end
 
     it "shouldn't available for non admin users" do
@@ -266,7 +266,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(:default_regex_message)])
+          to eq([Gitlab::Regex.send(:namespace_regex_message)])
     end
 
     context "with existing user" do