From defb8660c08a904a385b584280f72fc6a5a94c6e Mon Sep 17 00:00:00 2001 From: Patricio Cano Date: Thu, 14 Jul 2016 13:19:40 -0500 Subject: [PATCH] Added the ability to block sign ups using a domain blacklist. --- .../admin/application_settings_controller.rb | 4 + app/models/application_setting.rb | 35 +++++++-- app/models/user.rb | 48 +++++++----- .../application_settings/_form.html.haml | 73 ++++++++++++++++--- ...omain_blacklist_to_application_settings.rb | 22 ++++++ db/schema.rb | 2 + lib/api/entities.rb | 2 + spec/fixtures/blacklist.txt | 3 + spec/models/application_setting_spec.rb | 27 +++++++ spec/models/user_spec.rb | 53 +++++++++++++- 10 files changed, 232 insertions(+), 37 deletions(-) create mode 100644 db/migrate/20160713205315_add_domain_blacklist_to_application_settings.rb create mode 100644 spec/fixtures/blacklist.txt diff --git a/app/controllers/admin/application_settings_controller.rb b/app/controllers/admin/application_settings_controller.rb index 23ba83aba0e..3e27320ee5c 100644 --- a/app/controllers/admin/application_settings_controller.rb +++ b/app/controllers/admin/application_settings_controller.rb @@ -64,6 +64,7 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController params[:application_setting][:disabled_oauth_sign_in_sources] = AuthHelper.button_based_providers.map(&:to_s) - Array(enabled_oauth_sign_in_sources) + params.delete(:domain_blacklist_raw) if params[:domain_blacklist_file] params.require(:application_setting).permit( :default_projects_limit, @@ -112,6 +113,9 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController :container_registry_token_expire_delay, :repository_storage, :enabled_git_access_protocol, + :domain_blacklist_enabled, + :domain_blacklist_raw, + :domain_blacklist_file, restricted_visibility_levels: [], import_sources: [], disabled_oauth_sign_in_sources: [] diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index c6f77cc055f..84b1b54eeae 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -9,7 +9,9 @@ class ApplicationSetting < ActiveRecord::Base serialize :import_sources serialize :disabled_oauth_sign_in_sources, Array serialize :restricted_signup_domains, Array - attr_accessor :restricted_signup_domains_raw + serialize :domain_blacklist, Array + + attr_accessor :restricted_signup_domains_raw, :domain_blacklist_raw validates :session_expire_delay, presence: true, @@ -62,6 +64,10 @@ class ApplicationSetting < ActiveRecord::Base validates :enabled_git_access_protocol, inclusion: { in: %w(ssh http), allow_blank: true, allow_nil: true } + validates :domain_blacklist, + presence: true, + if: :domain_blacklist_enabled? + validates_each :restricted_visibility_levels do |record, attr, value| unless value.nil? value.each do |level| @@ -154,18 +160,35 @@ class ApplicationSetting < ActiveRecord::Base self.restricted_signup_domains.join("\n") unless self.restricted_signup_domains.nil? end - def restricted_signup_domains_raw=(values) - self.restricted_signup_domains = [] - self.restricted_signup_domains = values.split( - /\s*[,;]\s* # comma or semicolon, optionally surrounded by whitespace + def domain_blacklist_raw + self.domain_blacklist.join("\n") unless self.domain_blacklist.nil? + end + + def splitter + /\s*[,;]\s* # comma or semicolon, optionally surrounded by whitespace | # or \s # any whitespace character | # or [\r\n] # any number of newline characters - /x) + /x + end + + def restricted_signup_domains_raw=(values) + self.restricted_signup_domains = [] + self.restricted_signup_domains = values.split(splitter) self.restricted_signup_domains.reject! { |d| d.empty? } end + def domain_blacklist_raw=(values) + self.domain_blacklist = [] + self.domain_blacklist = values.split(splitter) + self.domain_blacklist.reject! { |d| d.empty? } + end + + def domain_blacklist_file=(file) + self.domain_blacklist_raw = file.read + end + def runners_registration_token ensure_runners_registration_token! end diff --git a/app/models/user.rb b/app/models/user.rb index 3d0a033785c..b0c5d84fc40 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -111,7 +111,7 @@ class User < ActiveRecord::Base validates :avatar, file_size: { maximum: 200.kilobytes.to_i } before_validation :generate_password, on: :create - before_validation :restricted_signup_domains, on: :create + before_validation :signup_domain_valid?, on: :create before_validation :sanitize_attrs before_validation :set_notification_email, if: ->(user) { user.email_changed? } before_validation :set_public_email, if: ->(user) { user.public_email_changed? } @@ -760,27 +760,41 @@ class User < ActiveRecord::Base Project.where(id: events) end - def restricted_signup_domains - email_domains = current_application_settings.restricted_signup_domains + def match_domain(email_domains) + email_domains.any? do |domain| + escaped = Regexp.escape(domain).gsub('\*', '.*?') + regexp = Regexp.new "^#{escaped}$", Regexp::IGNORECASE + email_domain = Mail::Address.new(self.email).domain + email_domain =~ regexp + end + end - unless email_domains.blank? - match_found = email_domains.any? do |domain| - escaped = Regexp.escape(domain).gsub('\*', '.*?') - regexp = Regexp.new "^#{escaped}$", Regexp::IGNORECASE - email_domain = Mail::Address.new(self.email).domain - email_domain =~ regexp - end + def signup_domain_valid? + valid = true - unless match_found - self.errors.add :email, - 'is not whitelisted. ' + - 'Email domains valid for registration are: ' + - email_domains.join(', ') - return false + if current_application_settings.domain_blacklist_enabled? + blocked_domains = current_application_settings.domain_blacklist + if match_domain(blocked_domains) + self.errors.add :email, 'is not from an allowed domain.' + valid = false end end - true + allowed_domains = current_application_settings.restricted_signup_domains + unless allowed_domains.blank? + if match_domain(allowed_domains) + self.errors.clear + valid = true + else + self.errors.add :email, + 'is not whitelisted. ' + + 'Email domains valid for registration are: ' + + allowed_domains.join(', ') + valid = false + end + end + + return valid end def can_be_removed? diff --git a/app/views/admin/application_settings/_form.html.haml b/app/views/admin/application_settings/_form.html.haml index 538d8176ce7..9443fe5e1d3 100644 --- a/app/views/admin/application_settings/_form.html.haml +++ b/app/views/admin/application_settings/_form.html.haml @@ -109,7 +109,7 @@ Newly registered users will by default be external %fieldset - %legend Sign-in Restrictions + %legend Sign-up Restrictions .form-group .col-sm-offset-2.col-sm-10 .checkbox @@ -122,6 +122,49 @@ = f.label :send_user_confirmation_email do = f.check_box :send_user_confirmation_email Send confirmation email on sign-up + .form-group + = f.label :restricted_signup_domains, 'Restricted domains for sign-ups', class: 'control-label col-sm-2' + .col-sm-10 + = f.text_area :restricted_signup_domains_raw, placeholder: 'domain.com', class: 'form-control' + .help-block ONLY users with e-mail addresses that match these domain(s) will be able to sign-up. Wildcards allowed. Use separate lines for multiple entries. Ex: domain.com, *.domain.com + .form-group + = f.label :domain_blacklist_enabled, 'Domain Blacklist', class: 'control-label col-sm-2' + .col-sm-10 + .checkbox + = f.label :domain_blacklist_enabled do + = f.check_box :domain_blacklist_enabled + Enable domain blacklist for sign ups + .form-group + .col-sm-offset-2.col-sm-10 + .radio + = label_tag :blacklist_type_file do + = radio_button_tag :blacklist_type, :file, @application_setting.domain_blacklist.blank? + .option-title + Upload blacklist file + .radio + = label_tag :blacklist_type_raw do + = radio_button_tag :blacklist_type, :raw, @application_setting.domain_blacklist.present? + .option-title + Enter blacklist manually + .form-group.blacklist-file + = f.label :domain_blacklist_file, 'Blacklist file', class: 'control-label col-sm-2' + .col-sm-10 + = f.file_field :domain_blacklist_file, class: 'form-control', accept: '.txt,.conf' + .help-block Users with e-mail addresses that match these domain(s) will NOT be able to sign-up. Wildcards allowed. Use separate lines or commas for multiple entries. + .form-group.blacklist-raw + = f.label :domain_blacklist, 'Blacklisted domains', class: 'control-label col-sm-2' + .col-sm-10 + = f.text_area :domain_blacklist_raw, placeholder: 'domain.com', class: 'form-control', rows: 10 + .help-block Users with e-mail addresses that match these domain(s) will NOT be able to sign-up. Wildcards allowed. Use separate lines for multiple entries. Ex: domain.com, *.domain.com + + .form-group + = f.label :after_sign_up_text, class: 'control-label col-sm-2' + .col-sm-10 + = f.text_area :after_sign_up_text, class: 'form-control', rows: 4 + .help-block Markdown enabled + + %fieldset + %legend Sign-in Restrictions .form-group .col-sm-offset-2.col-sm-10 .checkbox @@ -147,11 +190,6 @@ .col-sm-10 = f.number_field :two_factor_grace_period, min: 0, class: 'form-control', placeholder: '0' .help-block Amount of time (in hours) that users are allowed to skip forced configuration of two-factor authentication - .form-group - = f.label :restricted_signup_domains, 'Restricted domains for sign-ups', class: 'control-label col-sm-2' - .col-sm-10 - = f.text_area :restricted_signup_domains_raw, placeholder: 'domain.com', class: 'form-control' - .help-block Only users with e-mail addresses that match these domain(s) will be able to sign-up. Wildcards allowed. Use separate lines for multiple entries. Ex: domain.com, *.domain.com .form-group = f.label :home_page_url, 'Home page URL', class: 'control-label col-sm-2' .col-sm-10 @@ -167,11 +205,6 @@ .col-sm-10 = f.text_area :sign_in_text, class: 'form-control', rows: 4 .help-block Markdown enabled - .form-group - = f.label :after_sign_up_text, class: 'control-label col-sm-2' - .col-sm-10 - = f.text_area :after_sign_up_text, class: 'form-control', rows: 4 - .help-block Markdown enabled .form-group = f.label :help_page_text, class: 'control-label col-sm-2' .col-sm-10 @@ -353,3 +386,21 @@ .form-actions = f.submit 'Save', class: 'btn btn-save' + +:javascript + function showBlacklistType() { + if ($("input[name='blacklist_type']:checked").val() == "file") + { + $(".blacklist-file").show(); + $(".blacklist-raw").hide(); + } + else + { + $(".blacklist-file").hide(); + $(".blacklist-raw").show(); + } + } + + $("input[name='blacklist_type']").click(showBlacklistType); + + showBlacklistType(); \ No newline at end of file diff --git a/db/migrate/20160713205315_add_domain_blacklist_to_application_settings.rb b/db/migrate/20160713205315_add_domain_blacklist_to_application_settings.rb new file mode 100644 index 00000000000..ecdd1bd7e5e --- /dev/null +++ b/db/migrate/20160713205315_add_domain_blacklist_to_application_settings.rb @@ -0,0 +1,22 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddDomainBlacklistToApplicationSettings < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # When using the methods "add_concurrent_index" or "add_column_with_default" + # you must disable the use of transactions as these methods can not run in an + # existing transaction. When using "add_concurrent_index" make sure that this + # method is the _only_ method called in the migration, any other changes + # should go in a separate migration. This ensures that upon failure _only_ the + # index creation fails and can be retried or reverted easily. + # + # To disable transactions uncomment the following line and remove these + # comments: + # disable_ddl_transaction! + + def change + add_column :application_settings, :domain_blacklist_enabled, :boolean, default: false + add_column :application_settings, :domain_blacklist, :text + end +end diff --git a/db/schema.rb b/db/schema.rb index 8882377f9f4..25d94f283c9 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -88,6 +88,8 @@ ActiveRecord::Schema.define(version: 20160716115710) do t.text "after_sign_up_text" t.string "repository_storage", default: "default" t.string "enabled_git_access_protocol" + t.boolean "domain_blacklist_enabled", default: false + t.text "domain_blacklist" end create_table "audit_events", force: :cascade do |t| diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 3c79a00eb8c..4cd388658be 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -413,6 +413,8 @@ module API expose :default_snippet_visibility expose :default_group_visibility expose :restricted_signup_domains + expose :domain_blacklist_enabled + expose :domain_blacklist expose :user_oauth_applications expose :after_sign_out_path expose :container_registry_token_expire_delay diff --git a/spec/fixtures/blacklist.txt b/spec/fixtures/blacklist.txt new file mode 100644 index 00000000000..baeb11eda9a --- /dev/null +++ b/spec/fixtures/blacklist.txt @@ -0,0 +1,3 @@ +example.com +test.com +foo.bar \ No newline at end of file diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb index 2ea1320267c..582d9a8d8cd 100644 --- a/spec/models/application_setting_spec.rb +++ b/spec/models/application_setting_spec.rb @@ -73,4 +73,31 @@ describe ApplicationSetting, models: true do expect(setting.restricted_signup_domains).to eq(['example.com', '*.example.com']) end end + + context 'blacklisted signup domains' do + it 'set single domain' do + setting.domain_blacklist_raw = 'example.com' + expect(setting.domain_blacklist).to eq(['example.com']) + end + + it 'set multiple domains with spaces' do + setting.domain_blacklist_raw = 'example.com *.example.com' + expect(setting.domain_blacklist).to eq(['example.com', '*.example.com']) + end + + it 'set multiple domains with newlines and a space' do + setting.domain_blacklist_raw = "example.com\n *.example.com" + expect(setting.domain_blacklist).to eq(['example.com', '*.example.com']) + end + + it 'set multiple domains with commas' do + setting.domain_blacklist_raw = "example.com, *.example.com" + expect(setting.domain_blacklist).to eq(['example.com', '*.example.com']) + end + + it 'set multiple domain with file' do + setting.domain_blacklist_file = File.open(Rails.root.join('spec/fixtures/', 'blacklist.txt')) + expect(setting.domain_blacklist).to eq(%w(example.com test.com foo.bar)) + end + end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index fc74488ac0e..79f77d116a7 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -89,7 +89,7 @@ describe User, models: true do end describe 'email' do - context 'when no signup domains listed' do + context 'when no signup domains white listed' do before do allow_any_instance_of(ApplicationSetting).to receive(:restricted_signup_domains).and_return([]) end @@ -100,7 +100,7 @@ describe User, models: true do end end - context 'when a signup domain is listed and subdomains are allowed' do + context 'when a signup domain is white listed and subdomains are allowed' do before do allow_any_instance_of(ApplicationSetting).to receive(:restricted_signup_domains).and_return(['example.com', '*.example.com']) end @@ -121,7 +121,7 @@ describe User, models: true do end end - context 'when a signup domain is listed and subdomains are not allowed' do + context 'when a signup domain is white listed and subdomains are not allowed' do before do allow_any_instance_of(ApplicationSetting).to receive(:restricted_signup_domains).and_return(['example.com']) end @@ -142,6 +142,53 @@ describe User, models: true do end end + context 'domain blacklist' do + before do + allow_any_instance_of(ApplicationSetting).to receive(:domain_blacklist_enabled?).and_return(true) + allow_any_instance_of(ApplicationSetting).to receive(:domain_blacklist).and_return(['example.com']) + end + + context 'when a signup domain is black listed' do + it 'accepts info@test.com' do + user = build(:user, email: 'info@test.com') + expect(user).to be_valid + end + + it 'rejects info@example.com' do + user = build(:user, email: 'info@example.com') + expect(user).not_to be_valid + end + end + + context 'when a signup domain is black listed but a wildcard subdomain is allowed' do + before do + allow_any_instance_of(ApplicationSetting).to receive(:domain_blacklist).and_return(['test.example.com']) + allow_any_instance_of(ApplicationSetting).to receive(:restricted_signup_domains).and_return(['*.example.com']) + end + + it 'should give priority to whitelist and allow info@test.example.com' do + user = build(:user, email: 'info@test.example.com') + expect(user).to be_valid + end + end + + context 'with both lists containing a domain' do + before do + allow_any_instance_of(ApplicationSetting).to receive(:restricted_signup_domains).and_return(['test.com']) + end + + it 'accepts info@test.com' do + user = build(:user, email: 'info@test.com') + expect(user).to be_valid + end + + it 'rejects info@example.com' do + user = build(:user, email: 'info@example.com') + expect(user).not_to be_valid + end + end + end + context 'owns_notification_email' do it 'accepts temp_oauth_email emails' do user = build(:user, email: "temp-email-for-oauth@example.com")