Commit f8f97503 authored by Dmitriy Zaporozhets's avatar Dmitriy Zaporozhets

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
parents 24b048db 45ee9009
...@@ -53,6 +53,8 @@ v 7.10.0 (unreleased) ...@@ -53,6 +53,8 @@ v 7.10.0 (unreleased)
- Fix admin user projects lists. - Fix admin user projects lists.
- Don't leak private group existence by redirecting from namespace controller to group controller. - 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) - 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 v 7.9.2
- Contains no changes - Contains no changes
......
...@@ -24,8 +24,8 @@ class Namespace < ActiveRecord::Base ...@@ -24,8 +24,8 @@ class Namespace < ActiveRecord::Base
validates :name, validates :name,
presence: true, uniqueness: true, presence: true, uniqueness: true,
length: { within: 0..255 }, length: { within: 0..255 },
format: { with: Gitlab::Regex.name_regex, format: { with: Gitlab::Regex.namespace_name_regex,
message: Gitlab::Regex.name_regex_message } message: Gitlab::Regex.namespace_name_regex_message }
validates :description, length: { within: 0..255 } validates :description, length: { within: 0..255 }
validates :path, validates :path,
...@@ -33,8 +33,8 @@ class Namespace < ActiveRecord::Base ...@@ -33,8 +33,8 @@ class Namespace < ActiveRecord::Base
presence: true, presence: true,
length: { within: 1..255 }, length: { within: 1..255 },
exclusion: { in: Gitlab::Blacklist.path }, exclusion: { in: Gitlab::Blacklist.path },
format: { with: Gitlab::Regex.path_regex, format: { with: Gitlab::Regex.namespace_regex,
message: Gitlab::Regex.path_regex_message } message: Gitlab::Regex.namespace_regex_message }
delegate :name, to: :owner, allow_nil: true, prefix: true delegate :name, to: :owner, allow_nil: true, prefix: true
...@@ -44,21 +44,36 @@ class Namespace < ActiveRecord::Base ...@@ -44,21 +44,36 @@ class Namespace < ActiveRecord::Base
scope :root, -> { where('type IS NULL') } scope :root, -> { where('type IS NULL') }
def self.by_path(path) class << self
def by_path(path)
where('lower(path) = :value', value: path.downcase).first where('lower(path) = :value', value: path.downcase).first
end end
# Case insensetive search for namespace by path or name # Case insensetive search for namespace by path or name
def self.find_by_path_or_name(path) def find_by_path_or_name(path)
find_by("lower(path) = :path OR lower(name) = :path", path: path.downcase) find_by("lower(path) = :path OR lower(name) = :path", path: path.downcase)
end end
def self.search(query) def search(query)
where("name LIKE :query OR path LIKE :query", query: "%#{query}%") where("name LIKE :query OR path LIKE :query", query: "%#{query}%")
end end
def self.global_id def clean_path(path)
'GLN' 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 to_param def to_param
......
...@@ -124,12 +124,12 @@ class Project < ActiveRecord::Base ...@@ -124,12 +124,12 @@ class Project < ActiveRecord::Base
presence: true, presence: true,
length: { within: 0..255 }, length: { within: 0..255 },
format: { with: Gitlab::Regex.project_name_regex, format: { with: Gitlab::Regex.project_name_regex,
message: Gitlab::Regex.project_regex_message } message: Gitlab::Regex.project_name_regex_message }
validates :path, validates :path,
presence: true, presence: true,
length: { within: 0..255 }, length: { within: 0..255 },
format: { with: Gitlab::Regex.path_regex, format: { with: Gitlab::Regex.project_path_regex,
message: Gitlab::Regex.path_regex_message } message: Gitlab::Regex.project_path_regex_message }
validates :issues_enabled, :merge_requests_enabled, validates :issues_enabled, :merge_requests_enabled,
:wiki_enabled, inclusion: { in: [true, false] } :wiki_enabled, inclusion: { in: [true, false] }
validates :issues_tracker_id, length: { maximum: 255 }, allow_blank: true validates :issues_tracker_id, length: { maximum: 255 }, allow_blank: true
......
...@@ -33,8 +33,8 @@ class Snippet < ActiveRecord::Base ...@@ -33,8 +33,8 @@ class Snippet < ActiveRecord::Base
validates :file_name, validates :file_name,
presence: true, presence: true,
length: { within: 0..255 }, length: { within: 0..255 },
format: { with: Gitlab::Regex.path_regex, format: { with: Gitlab::Regex.file_name_regex,
message: Gitlab::Regex.path_regex_message } message: Gitlab::Regex.file_name_regex_message }
validates :content, presence: true validates :content, presence: true
validates :visibility_level, inclusion: { in: Gitlab::VisibilityLevel.values } validates :visibility_level, inclusion: { in: Gitlab::VisibilityLevel.values }
......
...@@ -129,8 +129,8 @@ class User < ActiveRecord::Base ...@@ -129,8 +129,8 @@ class User < ActiveRecord::Base
presence: true, presence: true,
uniqueness: { case_sensitive: false }, uniqueness: { case_sensitive: false },
exclusion: { in: Gitlab::Blacklist.path }, exclusion: { in: Gitlab::Blacklist.path },
format: { with: Gitlab::Regex.username_regex, format: { with: Gitlab::Regex.namespace_regex,
message: Gitlab::Regex.username_regex_message } message: Gitlab::Regex.namespace_regex_message }
validates :notification_level, inclusion: { in: Notification.notification_levels }, presence: true validates :notification_level, inclusion: { in: Notification.notification_levels }, presence: true
validate :namespace_uniq, if: ->(user) { user.username_changed? } validate :namespace_uniq, if: ->(user) { user.username_changed? }
...@@ -229,22 +229,6 @@ class User < ActiveRecord::Base ...@@ -229,22 +229,6 @@ class User < ActiveRecord::Base
def build_user(attrs = {}) def build_user(attrs = {})
User.new(attrs) User.new(attrs)
end 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 end
# #
......
...@@ -12,10 +12,10 @@ module Files ...@@ -12,10 +12,10 @@ module Files
file_name = File.basename(path) file_name = File.basename(path)
file_path = path file_path = path
unless file_name =~ Gitlab::Regex.path_regex unless file_name =~ Gitlab::Regex.file_name_regex
return error( return error(
'Your changes could not be committed, because the file name ' + 'Your changes could not be committed, because the file name ' +
Gitlab::Regex.path_regex_message Gitlab::Regex.file_name_regex_message
) )
end end
......
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
...@@ -74,7 +74,7 @@ class Spinach::Features::ProjectSourceBrowseFiles < Spinach::FeatureSteps ...@@ -74,7 +74,7 @@ class Spinach::Features::ProjectSourceBrowseFiles < Spinach::FeatureSteps
end end
step 'I fill the new file name with an illegal name' do 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 end
step 'I fill the commit message' do step 'I fill the commit message' do
......
...@@ -161,7 +161,7 @@ module Gitlab ...@@ -161,7 +161,7 @@ module Gitlab
text text
end 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})" PROJ_STR = "(?<project>#{NAME_STR}/#{NAME_STR})"
REFERENCE_PATTERN = %r{ REFERENCE_PATTERN = %r{
......
...@@ -86,7 +86,7 @@ module Gitlab ...@@ -86,7 +86,7 @@ module Gitlab
def user_attributes def user_attributes
{ {
name: auth_hash.name, name: auth_hash.name,
username: ::User.clean_username(auth_hash.username), username: ::Namespace.clean_path(auth_hash.username),
email: auth_hash.email, email: auth_hash.email,
password: auth_hash.password, password: auth_hash.password,
password_confirmation: auth_hash.password, password_confirmation: auth_hash.password,
......
...@@ -2,49 +2,66 @@ module Gitlab ...@@ -2,49 +2,66 @@ module Gitlab
module Regex module Regex
extend self extend self
def username_regex NAMESPACE_REGEX_STR = '(?:[a-zA-Z0-9_\.][a-zA-Z0-9_\-\.]*[a-zA-Z0-9_\-]|[a-zA-Z0-9_])'.freeze
default_regex
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 end
def username_regex_message def namespace_name_regex_message
default_regex_message "can contain only letters, digits, '_', '-', '.' and space."
end end
def project_name_regex 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 end
def project_regex_message def project_name_regex_message
"can contain only letters, digits, '_', '-' and '.' and space. " \ "can contain only letters, digits, '_', '-', '.' and space. " \
"It must start with letter, digit or '_'." "It must start with letter, digit or '_'."
end 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 end
def name_regex_message def project_path_regex_message
"can contain only letters, digits, '_', '-' and '.' and space." "can contain only letters, digits, '_', '-' and '.'. " \
"Cannot start with '-' or end in '.git'" \
end end
def path_regex
default_regex def file_name_regex
@file_name_regex ||= /\A[a-zA-Z0-9_\-\.]*\z/.freeze
end end
def path_regex_message def file_name_regex_message
default_regex_message "can contain only letters, digits, '_', '-' and '.'. "
end end
def archive_formats_regex def archive_formats_regex
#|zip|tar| tar.gz | tar.bz2 | # |zip|tar| tar.gz | tar.bz2 |
/(zip|tar|tar\.gz|tgz|gz|tar\.bz2|tbz|tbz2|tb2|bz2)/ @archive_formats_regex ||= /(zip|tar|tar\.gz|tgz|gz|tar\.bz2|tbz|tbz2|tb2|bz2)/.freeze
end end
def git_reference_regex def git_reference_regex
# Valid git ref regex, see: # Valid git ref regex, see:
# https://www.kernel.org/pub/software/scm/git/docs/git-check-ref-format.html # https://www.kernel.org/pub/software/scm/git/docs/git-check-ref-format.html
%r{ @git_reference_regex ||= %r{
(?! (?!
(?# doesn't begins with) (?# doesn't begins with)
\/| (?# rule #6) \/| (?# rule #6)
...@@ -60,18 +77,7 @@ module Gitlab ...@@ -60,18 +77,7 @@ module Gitlab
(?# doesn't end with) (?# doesn't end with)
(?<!\.lock) (?# rule #1) (?<!\.lock) (?# rule #1)
(?<![\/.]) (?# rule #6-7) (?<![\/.]) (?# rule #6-7)
}x }x.freeze
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/
end end
end end
end end
require 'spec_helper' require 'spec_helper'
describe Gitlab::Regex do describe Gitlab::Regex do
describe 'path regex' do describe 'project path regex' do
it { expect('gitlab-ce').to match(Gitlab::Regex.path_regex) } it { expect('gitlab-ce').to match(Gitlab::Regex.project_path_regex) }
it { expect('gitlab_git').to match(Gitlab::Regex.path_regex) } it { expect('gitlab_git').to match(Gitlab::Regex.project_path_regex) }
it { expect('_underscore.js').to match(Gitlab::Regex.path_regex) } it { expect('_underscore.js').to match(Gitlab::Regex.project_path_regex) }
it { expect('100px.com').to match(Gitlab::Regex.path_regex) } it { expect('100px.com').to match(Gitlab::Regex.project_path_regex) }
it { expect('?gitlab').not_to match(Gitlab::Regex.path_regex) } it { expect('?gitlab').not_to match(Gitlab::Regex.project_path_regex) }
it { expect('git lab').not_to match(Gitlab::Regex.path_regex) } it { expect('git lab').not_to match(Gitlab::Regex.project_path_regex) }
it { expect('gitlab.git').not_to match(Gitlab::Regex.path_regex) } it { expect('gitlab.git').not_to match(Gitlab::Regex.project_path_regex) }
end end
describe 'project name regex' do describe 'project name regex' do
......
...@@ -33,8 +33,6 @@ describe Namespace do ...@@ -33,8 +33,6 @@ describe Namespace do
it { is_expected.to respond_to(:to_param) } it { is_expected.to respond_to(:to_param) }
end end
it { expect(Namespace.global_id).to eq('GLN') }
describe :to_param do describe :to_param do
it { expect(namespace.to_param).to eq(namespace.path) } it { expect(namespace.to_param).to eq(namespace.path) }
end end
...@@ -85,4 +83,14 @@ describe Namespace do ...@@ -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('WOW')).to eq(@namespace) }
it { expect(Namespace.find_by_path_or_name('unknown')).to eq(nil) } it { expect(Namespace.find_by_path_or_name('unknown')).to eq(nil) }
end 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 end
...@@ -307,16 +307,6 @@ describe User do ...@@ -307,16 +307,6 @@ describe User do
end end
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 describe 'all_ssh_keys' do
it { is_expected.to have_many(:keys).dependent(:destroy) } it { is_expected.to have_many(:keys).dependent(:destroy) }
......
...@@ -247,12 +247,12 @@ describe API::API, api: true do ...@@ -247,12 +247,12 @@ describe API::API, api: true do
expect(json_response['message']['name']).to eq([ expect(json_response['message']['name']).to eq([
'can\'t be blank', 'can\'t be blank',
'is too short (minimum is 0 characters)', '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([ expect(json_response['message']['path']).to eq([
'can\'t be blank', 'can\'t be blank',
'is too short (minimum is 0 characters)', 'is too short (minimum is 0 characters)',
Gitlab::Regex.send(:default_regex_message) Gitlab::Regex.send(:project_path_regex_message)
]) ])
end end
......
...@@ -140,7 +140,7 @@ describe API::API, api: true do ...@@ -140,7 +140,7 @@ describe API::API, api: true do
expect(json_response['message']['projects_limit']). expect(json_response['message']['projects_limit']).
to eq(['must be greater than or equal to 0']) to eq(['must be greater than or equal to 0'])
expect(json_response['message']['username']). expect(json_response['message']['username']).
to eq([Gitlab::Regex.send(:default_regex_message)]) to eq([Gitlab::Regex.send(:namespace_regex_message)])
end end
it "shouldn't available for non admin users" do it "shouldn't available for non admin users" do
...@@ -266,7 +266,7 @@ describe API::API, api: true do ...@@ -266,7 +266,7 @@ describe API::API, api: true do
expect(json_response['message']['projects_limit']). expect(json_response['message']['projects_limit']).
to eq(['must be greater than or equal to 0']) to eq(['must be greater than or equal to 0'])
expect(json_response['message']['username']). expect(json_response['message']['username']).
to eq([Gitlab::Regex.send(:default_regex_message)]) to eq([Gitlab::Regex.send(:namespace_regex_message)])
end end
context "with existing user" do context "with existing user" do
......
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