diff options
author | Markus Koller <markus-koller@gmx.ch> | 2017-01-25 00:09:58 +0300 |
---|---|---|
committer | Alexis Reigel <mail@koffeinfrei.org> | 2017-04-06 11:01:13 +0300 |
commit | a3430f011f1adceaef8484f38a57018712a18ad2 (patch) | |
tree | ae69438c98358e214c39517ad4ceddf60d15c65a | |
parent | 57374feabe1428b2ea06a6a3cac244612128095d (diff) |
Support 2FA requirement per-group
20 files changed, 433 insertions, 21 deletions
diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb index cea3d088e94..f28bbdeff5a 100644 --- a/app/controllers/admin/groups_controller.rb +++ b/app/controllers/admin/groups_controller.rb @@ -72,7 +72,9 @@ class Admin::GroupsController < Admin::ApplicationController :name, :path, :request_access_enabled, - :visibility_level + :visibility_level, + :require_two_factor_authentication, + :two_factor_grace_period ] end end diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index b197fd2157e..28c4380ca84 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -267,11 +267,19 @@ class ApplicationController < ActionController::Base end def two_factor_authentication_required? - current_application_settings.require_two_factor_authentication + current_application_settings.require_two_factor_authentication || + current_user.try(:require_two_factor_authentication) end def two_factor_grace_period - current_application_settings.two_factor_grace_period + if current_user.try(:require_two_factor_authentication) + [ + current_application_settings.two_factor_grace_period, + current_user.two_factor_grace_period + ].min + else + current_application_settings.two_factor_grace_period + end end def two_factor_grace_period_expired? diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 05f9ee1ee90..5f90df579a8 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -150,7 +150,9 @@ class GroupsController < Groups::ApplicationController :visibility_level, :parent_id, :create_chat_team, - :chat_team_name + :chat_team_name, + :require_two_factor_authentication, + :two_factor_grace_period ] end diff --git a/app/models/group.rb b/app/models/group.rb index 60274386103..106084175ff 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -27,11 +27,14 @@ class Group < Namespace validates :avatar, file_size: { maximum: 200.kilobytes.to_i } + validates :two_factor_grace_period, presence: true, numericality: { greater_than_or_equal_to: 0 } + mount_uploader :avatar, AvatarUploader has_many :uploads, as: :model, dependent: :destroy after_create :post_create_hook after_destroy :post_destroy_hook + after_save :update_two_factor_requirement class << self # Searches for groups matching the given query. @@ -223,4 +226,12 @@ class Group < Namespace type: public? ? 'O' : 'I' # Open vs Invite-only } end + + protected + + def update_two_factor_requirement + return unless require_two_factor_authentication_changed? || two_factor_grace_period_changed? + + users.find_each(&:update_two_factor_requirement) + end end diff --git a/app/models/members/group_member.rb b/app/models/members/group_member.rb index 446f9f8f8a7..483425cd30f 100644 --- a/app/models/members/group_member.rb +++ b/app/models/members/group_member.rb @@ -3,11 +3,16 @@ class GroupMember < Member belongs_to :group, foreign_key: 'source_id' + delegate :update_two_factor_requirement, to: :user + # Make sure group member points only to group as it source default_value_for :source_type, SOURCE_TYPE validates :source_type, format: { with: /\ANamespace\z/ } default_scope { where(source_type: SOURCE_TYPE) } + after_create :update_two_factor_requirement, unless: :invite? + after_destroy :update_two_factor_requirement, unless: :invite? + def self.access_level_roles Gitlab::Access.options_with_owner end diff --git a/app/models/user.rb b/app/models/user.rb index 95a766f2ede..564e99df77b 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -963,6 +963,15 @@ class User < ActiveRecord::Base super end + def update_two_factor_requirement + periods = groups.where(require_two_factor_authentication: true).pluck(:two_factor_grace_period) + + self.require_two_factor_authentication = periods.any? + self.two_factor_grace_period = periods.min || User.column_defaults['two_factor_grace_period'] + + save + end + private def ci_projects_union diff --git a/app/views/admin/groups/_form.html.haml b/app/views/admin/groups/_form.html.haml index 589f4557b52..d9f05003904 100644 --- a/app/views/admin/groups/_form.html.haml +++ b/app/views/admin/groups/_form.html.haml @@ -13,7 +13,7 @@ .col-sm-offset-2.col-sm-10 = render 'shared/allow_request_access', form: f - = render 'groups/group_lfs_settings', f: f + = render 'groups/group_admin_settings', f: f - if @group.new_record? .form-group diff --git a/app/views/groups/_group_admin_settings.html.haml b/app/views/groups/_group_admin_settings.html.haml new file mode 100644 index 00000000000..2ace1e2dd1e --- /dev/null +++ b/app/views/groups/_group_admin_settings.html.haml @@ -0,0 +1,28 @@ +- if current_user.admin? + .form-group + = f.label :lfs_enabled, 'Large File Storage', class: 'control-label' + .col-sm-10 + .checkbox + = f.label :lfs_enabled do + = f.check_box :lfs_enabled, checked: @group.lfs_enabled? + %strong + Allow projects within this group to use Git LFS + = link_to icon('question-circle'), help_page_path('workflow/lfs/manage_large_binaries_with_git_lfs') + %br/ + %span.descr This setting can be overridden in each project. + +- if can? current_user, :admin_group, @group + .form-group + = f.label :require_two_factor_authentication, 'Two-factor authentication', class: 'control-label col-sm-2' + .col-sm-10 + .checkbox + = f.label :require_two_factor_authentication do + = f.check_box :require_two_factor_authentication + %strong + Require all users in this group to setup Two-factor authentication + = link_to icon('question-circle'), help_page_path('security/two_factor_authentication', anchor: 'enforcing-2fa-for-all-users-in-a-group') + .form-group + .col-sm-offset-2.col-sm-10 + .checkbox + = f.text_field :two_factor_grace_period, class: 'form-control' + .help-block Amount of time (in hours) that users are allowed to skip forced configuration of two-factor authentication diff --git a/app/views/groups/_group_lfs_settings.html.haml b/app/views/groups/_group_lfs_settings.html.haml deleted file mode 100644 index 3c622ca5c3c..00000000000 --- a/app/views/groups/_group_lfs_settings.html.haml +++ /dev/null @@ -1,11 +0,0 @@ -- if current_user.admin? - .form-group - .col-sm-offset-2.col-sm-10 - .checkbox - = f.label :lfs_enabled do - = f.check_box :lfs_enabled, checked: @group.lfs_enabled? - %strong - Allow projects within this group to use Git LFS - = link_to icon('question-circle'), help_page_path('workflow/lfs/manage_large_binaries_with_git_lfs') - %br/ - %span.descr This setting can be overridden in each project. diff --git a/app/views/groups/edit.html.haml b/app/views/groups/edit.html.haml index 80a77dab97f..00ff40224ba 100644 --- a/app/views/groups/edit.html.haml +++ b/app/views/groups/edit.html.haml @@ -27,7 +27,7 @@ .col-sm-offset-2.col-sm-10 = render 'shared/allow_request_access', form: f - = render 'group_lfs_settings', f: f + = render 'group_admin_settings', f: f .form-group %hr diff --git a/changelogs/unreleased/feature-enforce-2fa-per-group.yml b/changelogs/unreleased/feature-enforce-2fa-per-group.yml new file mode 100644 index 00000000000..6dd99e4245f --- /dev/null +++ b/changelogs/unreleased/feature-enforce-2fa-per-group.yml @@ -0,0 +1,4 @@ +--- +title: Support 2FA requirement per-group +merge_request: 8763 +author: Markus Koller diff --git a/db/migrate/20170124193147_add_two_factor_columns_to_namespaces.rb b/db/migrate/20170124193147_add_two_factor_columns_to_namespaces.rb new file mode 100644 index 00000000000..ca4429c676c --- /dev/null +++ b/db/migrate/20170124193147_add_two_factor_columns_to_namespaces.rb @@ -0,0 +1,21 @@ +class AddTwoFactorColumnsToNamespaces < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_column_with_default(:namespaces, :require_two_factor_authentication, :boolean, default: false) + add_column_with_default(:namespaces, :two_factor_grace_period, :integer, default: 48) + + add_concurrent_index(:namespaces, :require_two_factor_authentication) + end + + def down + remove_column(:namespaces, :require_two_factor_authentication) + remove_column(:namespaces, :two_factor_grace_period) + + remove_index(:namespaces, :require_two_factor_authentication) if index_exists?(:namespaces, :require_two_factor_authentication) + end +end diff --git a/db/migrate/20170124193205_add_two_factor_columns_to_users.rb b/db/migrate/20170124193205_add_two_factor_columns_to_users.rb new file mode 100644 index 00000000000..bef1b2062c8 --- /dev/null +++ b/db/migrate/20170124193205_add_two_factor_columns_to_users.rb @@ -0,0 +1,17 @@ +class AddTwoFactorColumnsToUsers < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_column_with_default(:users, :require_two_factor_authentication, :boolean, default: false) + add_column_with_default(:users, :two_factor_grace_period, :integer, default: 48) + end + + def down + remove_column(:users, :require_two_factor_authentication) + remove_column(:users, :two_factor_grace_period) + end +end diff --git a/db/schema.rb b/db/schema.rb index ccf18d07179..2d2507828d4 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -692,6 +692,8 @@ ActiveRecord::Schema.define(version: 20170402231018) do t.text "description_html" t.boolean "lfs_enabled" t.integer "parent_id" + t.boolean "require_two_factor_authentication", default: false, null: false + t.integer "two_factor_grace_period", default: 48, null: false end add_index "namespaces", ["created_at"], name: "index_namespaces_on_created_at", using: :btree @@ -702,6 +704,7 @@ ActiveRecord::Schema.define(version: 20170402231018) do add_index "namespaces", ["parent_id", "id"], name: "index_namespaces_on_parent_id_and_id", unique: true, using: :btree add_index "namespaces", ["path"], name: "index_namespaces_on_path", using: :btree add_index "namespaces", ["path"], name: "index_namespaces_on_path_trigram", using: :gin, opclasses: {"path"=>"gin_trgm_ops"} + add_index "namespaces", ["require_two_factor_authentication"], name: "index_namespaces_on_require_two_factor_authentication", using: :btree add_index "namespaces", ["type"], name: "index_namespaces_on_type", using: :btree create_table "notes", force: :cascade do |t| @@ -1246,6 +1249,8 @@ ActiveRecord::Schema.define(version: 20170402231018) do t.boolean "authorized_projects_populated" t.boolean "ghost" t.boolean "notified_of_own_activity" + t.boolean "require_two_factor_authentication", default: false, null: false + t.integer "two_factor_grace_period", default: 48, null: false end add_index "users", ["admin"], name: "index_users_on_admin", using: :btree diff --git a/doc/security/img/two_factor_authentication_group_settings.png b/doc/security/img/two_factor_authentication_group_settings.png Binary files differnew file mode 100644 index 00000000000..a1b3c58bfdc --- /dev/null +++ b/doc/security/img/two_factor_authentication_group_settings.png diff --git a/doc/security/two_factor_authentication.md b/doc/security/two_factor_authentication.md index c8499380c18..f02f7b807cf 100644 --- a/doc/security/two_factor_authentication.md +++ b/doc/security/two_factor_authentication.md @@ -8,7 +8,7 @@ their phone. You can read more about it here: [Two-factor Authentication (2FA)](../profile/two_factor_authentication.md) -## Enabling 2FA +## Enforcing 2FA for all users Users on GitLab, can enable it without any admin's intervention. If you want to enforce everyone to setup 2FA, you can choose from two different ways: @@ -28,6 +28,21 @@ period to `0`. --- +## Enforcing 2FA for all users in a group + +If you want to enforce 2FA only for certain groups, you can enable it in the +group settings and specify a grace period as above. To change this setting you +need to be administrator or owner of the group. + +If there are multiple 2FA requirements (i.e. group + all users, or multiple +groups) the shortest grace period will be used. + +--- + +![Two factor authentication group settings](img/two_factor_authentication_group_settings.png) + +--- + ## Disabling 2FA for everyone There may be some special situations where you want to disable 2FA for everyone diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb index 81cbccd5436..64cfb87da5d 100644 --- a/spec/controllers/application_controller_spec.rb +++ b/spec/controllers/application_controller_spec.rb @@ -100,8 +100,6 @@ describe ApplicationController do end describe '#route_not_found' do - let(:controller) { ApplicationController.new } - it 'renders 404 if authenticated' do allow(controller).to receive(:current_user).and_return(user) expect(controller).to receive(:not_found) @@ -115,4 +113,203 @@ describe ApplicationController do controller.send(:route_not_found) end end + + context 'two-factor authentication' do + let(:controller) { ApplicationController.new } + + describe '#check_2fa_requirement' do + subject { controller.send :check_2fa_requirement } + + it 'does not redirect if 2FA is not required' do + allow(controller).to receive(:two_factor_authentication_required?).and_return(false) + expect(controller).not_to receive(:redirect_to) + + subject + end + + it 'does not redirect if user is not logged in' do + allow(controller).to receive(:two_factor_authentication_required?).and_return(true) + allow(controller).to receive(:current_user).and_return(nil) + expect(controller).not_to receive(:redirect_to) + + subject + end + + it 'does not redirect if user has 2FA enabled' do + allow(controller).to receive(:two_factor_authentication_required?).and_return(true) + allow(controller).to receive(:current_user).twice.and_return(user) + allow(user).to receive(:two_factor_enabled?).and_return(true) + expect(controller).not_to receive(:redirect_to) + + subject + end + + it 'does not redirect if 2FA setup can be skipped' do + allow(controller).to receive(:two_factor_authentication_required?).and_return(true) + allow(controller).to receive(:current_user).twice.and_return(user) + allow(user).to receive(:two_factor_enabled?).and_return(false) + allow(controller).to receive(:skip_two_factor?).and_return(true) + expect(controller).not_to receive(:redirect_to) + + subject + end + + it 'redirects to 2FA setup otherwise' do + allow(controller).to receive(:two_factor_authentication_required?).and_return(true) + allow(controller).to receive(:current_user).twice.and_return(user) + allow(user).to receive(:two_factor_enabled?).and_return(false) + allow(controller).to receive(:skip_two_factor?).and_return(false) + allow(controller).to receive(:profile_two_factor_auth_path) + expect(controller).to receive(:redirect_to) + + subject + end + end + + describe '#two_factor_authentication_required?' do + subject { controller.send :two_factor_authentication_required? } + + it 'returns false if no 2FA requirement is present' do + allow(controller).to receive(:current_user).and_return(nil) + + expect(subject).to be_falsey + end + + it 'returns true if a 2FA requirement is set in the application settings' do + stub_application_setting require_two_factor_authentication: true + allow(controller).to receive(:current_user).and_return(nil) + + expect(subject).to be_truthy + end + + it 'returns true if a 2FA requirement is set on the user' do + user.require_two_factor_authentication = true + allow(controller).to receive(:current_user).and_return(user) + + expect(subject).to be_truthy + end + end + + describe '#two_factor_grace_period' do + subject { controller.send :two_factor_grace_period } + + it 'returns the grace period from the application settings' do + stub_application_setting two_factor_grace_period: 23 + allow(controller).to receive(:current_user).and_return(nil) + + expect(subject).to eq 23 + end + + context 'with a 2FA requirement set on the user' do + let(:user) { create :user, require_two_factor_authentication: true, two_factor_grace_period: 23 } + + it 'returns the user grace period if lower than the application grace period' do + stub_application_setting two_factor_grace_period: 24 + allow(controller).to receive(:current_user).and_return(user) + + expect(subject).to eq 23 + end + + it 'returns the application grace period if lower than the user grace period' do + stub_application_setting two_factor_grace_period: 22 + allow(controller).to receive(:current_user).and_return(user) + + expect(subject).to eq 22 + end + end + end + + describe '#two_factor_grace_period_expired?' do + subject { controller.send :two_factor_grace_period_expired? } + + before do + allow(controller).to receive(:current_user).and_return(user) + end + + it 'returns false if the user has not started their grace period yet' do + expect(subject).to be_falsey + end + + context 'with grace period started' do + let(:user) { create :user, otp_grace_period_started_at: 2.hours.ago } + + it 'returns true if the grace period has expired' do + allow(controller).to receive(:two_factor_grace_period).and_return(1) + + expect(subject).to be_truthy + end + + it 'returns false if the grace period is still active' do + allow(controller).to receive(:two_factor_grace_period).and_return(3) + + expect(subject).to be_falsey + end + end + end + + describe '#two_factor_skippable' do + subject { controller.send :two_factor_skippable? } + + before do + allow(controller).to receive(:current_user).and_return(user) + end + + it 'returns false if 2FA is not required' do + allow(controller).to receive(:two_factor_authentication_required?).and_return(false) + + expect(subject).to be_falsey + end + + it 'returns false if the user has already enabled 2FA' do + allow(controller).to receive(:two_factor_authentication_required?).and_return(true) + allow(user).to receive(:two_factor_enabled?).and_return(true) + + expect(subject).to be_falsey + end + + it 'returns false if the 2FA grace period has expired' do + allow(controller).to receive(:two_factor_authentication_required?).and_return(true) + allow(user).to receive(:two_factor_enabled?).and_return(false) + allow(controller).to receive(:two_factor_grace_period_expired?).and_return(true) + + expect(subject).to be_falsey + end + + it 'returns true otherwise' do + allow(controller).to receive(:two_factor_authentication_required?).and_return(true) + allow(user).to receive(:two_factor_enabled?).and_return(false) + allow(controller).to receive(:two_factor_grace_period_expired?).and_return(false) + + expect(subject).to be_truthy + end + end + + describe '#skip_two_factor?' do + subject { controller.send :skip_two_factor? } + + it 'returns false if 2FA setup was not skipped' do + allow(controller).to receive(:session).and_return({}) + + expect(subject).to be_falsey + end + + context 'with 2FA setup skipped' do + before do + allow(controller).to receive(:session).and_return({ skip_tfa: 2.hours.from_now }) + end + + it 'returns false if the grace period has expired' do + Timecop.freeze(3.hours.from_now) do + expect(subject).to be_falsey + end + end + + it 'returns true if the grace period is still active' do + Timecop.freeze(1.hour.from_now) do + expect(subject).to be_truthy + end + end + end + end + end end diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 5d87938235a..8ffde6f7fbb 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -55,6 +55,8 @@ describe Group, models: true do it { is_expected.to validate_uniqueness_of(:name).scoped_to(:parent_id) } it { is_expected.to validate_presence_of :path } it { is_expected.not_to validate_presence_of :owner } + it { is_expected.to validate_presence_of :two_factor_grace_period } + it { is_expected.to validate_numericality_of(:two_factor_grace_period).is_greater_than_or_equal_to(0) } end describe '.visible_to_user' do @@ -315,4 +317,44 @@ describe Group, models: true do to include(master.id, developer.id) end end + + describe '#update_two_factor_requirement' do + let(:user) { create(:user) } + + before do + group.add_user(user, GroupMember::OWNER) + end + + it 'is called when require_two_factor_authentication is changed' do + expect_any_instance_of(User).to receive(:update_two_factor_requirement) + + group.update!(require_two_factor_authentication: true) + end + + it 'is called when two_factor_grace_period is changed' do + expect_any_instance_of(User).to receive(:update_two_factor_requirement) + + group.update!(two_factor_grace_period: 23) + end + + it 'is not called when other attributes are changed' do + expect_any_instance_of(User).not_to receive(:update_two_factor_requirement) + + group.update!(description: 'foobar') + end + + it 'calls #update_two_factor_requirement on each group member' do + other_user = create(:user) + group.add_user(other_user, GroupMember::OWNER) + + calls = 0 + allow_any_instance_of(User).to receive(:update_two_factor_requirement) do + calls += 1 + end + + group.update!(require_two_factor_authentication: true, two_factor_grace_period: 23) + + expect(calls).to eq 2 + end + end end diff --git a/spec/models/members/group_member_spec.rb b/spec/models/members/group_member_spec.rb index 370aeb9e0a9..024380b7ebb 100644 --- a/spec/models/members/group_member_spec.rb +++ b/spec/models/members/group_member_spec.rb @@ -61,7 +61,7 @@ describe GroupMember, models: true do describe '#after_accept_request' do it 'calls NotificationService.accept_group_access_request' do - member = create(:group_member, user: build_stubbed(:user), requested_at: Time.now) + member = create(:group_member, user: build(:user), requested_at: Time.now) expect_any_instance_of(NotificationService).to receive(:new_group_member) @@ -75,4 +75,19 @@ describe GroupMember, models: true do it { is_expected.to eq 'Group' } end end + + describe '#update_two_factor_requirement' do + let(:user) { build :user } + let(:group_member) { build :group_member, user: user } + + it 'is called after creation and deletion' do + expect(user).to receive(:update_two_factor_requirement) + + group_member.save + + expect(user).to receive(:update_two_factor_requirement) + + group_member.destroy + end + end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index a9e37be1157..b2f686a1819 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -1521,4 +1521,46 @@ describe User, models: true do end end end + + describe '#update_two_factor_requirement' do + let(:user) { create :user } + + context 'with 2FA requirement on groups' do + let(:group1) { create :group, require_two_factor_authentication: true, two_factor_grace_period: 23 } + let(:group2) { create :group, require_two_factor_authentication: true, two_factor_grace_period: 32 } + + before do + group1.add_user(user, GroupMember::OWNER) + group2.add_user(user, GroupMember::OWNER) + + user.update_two_factor_requirement + end + + it 'requires 2FA' do + expect(user.require_two_factor_authentication).to be true + end + + it 'uses the shortest grace period' do + expect(user.two_factor_grace_period).to be 23 + end + end + + context 'without 2FA requirement on groups' do + let(:group) { create :group } + + before do + group.add_user(user, GroupMember::OWNER) + + user.update_two_factor_requirement + end + + it 'does not require 2FA' do + expect(user.require_two_factor_authentication).to be false + end + + it 'falls back to the default grace period' do + expect(user.two_factor_grace_period).to be 48 + end + end + end end |