Commit a3430f01 authored by Markus Koller's avatar Markus Koller Committed by Alexis Reigel

Support 2FA requirement per-group

parent 57374fea
......@@ -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
......@@ -267,12 +267,20 @@ 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
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?
date = current_user.otp_grace_period_started_at
......
......@@ -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
......
......@@ -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
......@@ -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
......
......@@ -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
......
......@@ -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
......
- if current_user.admin?
.form-group
.col-sm-offset-2.col-sm-10
= 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?
......@@ -9,3 +10,19 @@
= 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
......@@ -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
......
---
title: Support 2FA requirement per-group
merge_request: 8763
author: Markus Koller
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
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
......@@ -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
......
......@@ -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
......
......@@ -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
......@@ -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
......@@ -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
......@@ -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
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