diff --git a/CHANGELOG b/CHANGELOG index 53258e7fd1a..a8aaec1e566 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -10,6 +10,8 @@ v 7.13.0 (unreleased) - Update ssl_ciphers in Nginx example to remove DHE settings. This will deny forward secrecy for Android 2.3.7, Java 6 and OpenSSL 0.9.8 - Convert CRLF newlines to LF when committing using the web editor. - API request /projects/:project_id/merge_requests?state=closed will return only closed merge requests without merged one. If you need ones that were merged - use state=merged. + - Allow Administrators to filter the user list by those with or without Two-factor Authentication enabled. + - Show a user's Two-factor Authentication status in the administration area. v 7.12.0 (unreleased) - Fix Error 500 when one user attempts to access a personal, internal snippet (Stan Hu) diff --git a/app/models/user.rb b/app/models/user.rb index 29f43051464..22cd15bf971 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -50,12 +50,12 @@ # bitbucket_access_token :string(255) # bitbucket_access_token_secret :string(255) # location :string(255) -# public_email :string(255) default(""), not null # encrypted_otp_secret :string(255) # encrypted_otp_secret_iv :string(255) # encrypted_otp_secret_salt :string(255) -# otp_required_for_login :boolean +# otp_required_for_login :boolean default(FALSE), not null # otp_backup_codes :text +# public_email :string(255) default(""), not null # dashboard :integer default(0) # @@ -80,6 +80,7 @@ class User < ActiveRecord::Base devise :two_factor_authenticatable, otp_secret_encryption_key: File.read(Rails.root.join('.secret')).chomp + alias_attribute :two_factor_enabled, :otp_required_for_login devise :two_factor_backupable, otp_number_of_backup_codes: 10 serialize :otp_backup_codes, JSON @@ -193,11 +194,13 @@ class User < ActiveRecord::Base mount_uploader :avatar, AvatarUploader # Scopes - scope :admins, -> { where(admin: true) } + scope :admins, -> { where(admin: true) } scope :blocked, -> { with_state(:blocked) } scope :active, -> { with_state(:active) } scope :not_in_project, ->(project) { project.users.present? ? where("id not in (:ids)", ids: project.users.map(&:id) ) : all } scope :without_projects, -> { where('id NOT IN (SELECT DISTINCT(user_id) FROM members)') } + scope :with_two_factor, -> { where(two_factor_enabled: true) } + scope :without_two_factor, -> { where(two_factor_enabled: false) } # # Class methods @@ -247,9 +250,16 @@ class User < ActiveRecord::Base def filter(filter_name) case filter_name - when "admins"; self.admins - when "blocked"; self.blocked - when "wop"; self.without_projects + when 'admins' + self.admins + when 'blocked' + self.blocked + when 'two_factor_disabled' + self.without_two_factor + when 'two_factor_enabled' + self.with_two_factor + when 'wop' + self.without_projects else self.active end @@ -316,18 +326,6 @@ class User < ActiveRecord::Base @reset_token end - # Check if the user has enabled Two-factor Authentication - def two_factor_enabled? - otp_required_for_login - end - - # Set whether or not Two-factor Authentication is enabled for the current user - # - # setting - Boolean - def two_factor_enabled=(setting) - self.otp_required_for_login = setting - end - def namespace_uniq namespace_name = self.username existing_namespace = Namespace.by_path(namespace_name) diff --git a/app/views/admin/users/index.html.haml b/app/views/admin/users/index.html.haml index 45dee86b017..9c1bec7c84d 100644 --- a/app/views/admin/users/index.html.haml +++ b/app/views/admin/users/index.html.haml @@ -13,6 +13,14 @@ = link_to admin_users_path(filter: "admins") do Admins %small.pull-right= User.admins.count + %li.filter-two-factor-enabled{class: "#{'active' if params[:filter] == 'two_factor_enabled'}"} + = link_to admin_users_path(filter: 'two_factor_enabled') do + 2FA Enabled + %small.pull-right= User.with_two_factor.count + %li.filter-two-factor-disabled{class: "#{'active' if params[:filter] == 'two_factor_disabled'}"} + = link_to admin_users_path(filter: 'two_factor_disabled') do + 2FA Disabled + %small.pull-right= User.without_two_factor.count %li{class: "#{'active' if params[:filter] == "blocked"}"} = link_to admin_users_path(filter: "blocked") do Blocked diff --git a/db/migrate/20150620233230_add_default_otp_required_for_login_value.rb b/db/migrate/20150620233230_add_default_otp_required_for_login_value.rb new file mode 100644 index 00000000000..8eed8678b2f --- /dev/null +++ b/db/migrate/20150620233230_add_default_otp_required_for_login_value.rb @@ -0,0 +1,11 @@ +class AddDefaultOtpRequiredForLoginValue < ActiveRecord::Migration + def up + execute %q{UPDATE users SET otp_required_for_login = FALSE WHERE otp_required_for_login IS NULL} + + change_column :users, :otp_required_for_login, :boolean, default: false, null: false + end + + def down + change_column :users, :otp_required_for_login, :boolean, null: true + end +end diff --git a/db/schema.rb b/db/schema.rb index f063a4868b1..3a5af6a76d4 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20150610065936) do +ActiveRecord::Schema.define(version: 20150620233230) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -499,7 +499,7 @@ ActiveRecord::Schema.define(version: 20150610065936) do t.string "encrypted_otp_secret" t.string "encrypted_otp_secret_iv" t.string "encrypted_otp_secret_salt" - t.boolean "otp_required_for_login" + t.boolean "otp_required_for_login", default: false, null: false t.text "otp_backup_codes" t.string "public_email", default: "", null: false t.integer "dashboard", default: 0 diff --git a/spec/features/admin/admin_users_spec.rb b/spec/features/admin/admin_users_spec.rb index 7f5cb30cb94..86717761582 100644 --- a/spec/features/admin/admin_users_spec.rb +++ b/spec/features/admin/admin_users_spec.rb @@ -16,6 +16,46 @@ describe "Admin::Users", feature: true do expect(page).to have_content(@user.email) expect(page).to have_content(@user.name) end + + describe 'Two-factor Authentication filters' do + it 'counts users who have enabled 2FA' do + create(:user, two_factor_enabled: true) + + visit admin_users_path + + page.within('.filter-two-factor-enabled small') do + expect(page).to have_content('1') + end + end + + it 'filters by users who have enabled 2FA' do + user = create(:user, two_factor_enabled: true) + + visit admin_users_path + click_link '2FA Enabled' + + expect(page).to have_content(user.email) + end + + it 'counts users who have not enabled 2FA' do + create(:user, two_factor_enabled: false) + + visit admin_users_path + + page.within('.filter-two-factor-disabled small') do + expect(page).to have_content('2') # Including admin + end + end + + it 'filters by users who have not enabled 2FA' do + user = create(:user, two_factor_enabled: false) + + visit admin_users_path + click_link '2FA Disabled' + + expect(page).to have_content(user.email) + end + end end describe "GET /admin/users/new" do diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 9f7c83f3476..b80273c053d 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -50,12 +50,12 @@ # bitbucket_access_token :string(255) # bitbucket_access_token_secret :string(255) # location :string(255) -# public_email :string(255) default(""), not null # encrypted_otp_secret :string(255) # encrypted_otp_secret_iv :string(255) # encrypted_otp_secret_salt :string(255) -# otp_required_for_login :boolean +# otp_required_for_login :boolean default(FALSE), not null # otp_backup_codes :text +# public_email :string(255) default(""), not null # dashboard :integer default(0) # @@ -210,30 +210,6 @@ describe User do end end - describe '#two_factor_enabled' do - it 'returns two-factor authentication status' do - enabled = build_stubbed(:user, two_factor_enabled: true) - disabled = build_stubbed(:user) - - expect(enabled).to be_two_factor_enabled - expect(disabled).not_to be_two_factor_enabled - end - end - - describe '#two_factor_enabled=' do - it 'enables two-factor authentication' do - user = build_stubbed(:user, two_factor_enabled: false) - expect { user.two_factor_enabled = true }. - to change { user.two_factor_enabled? }.to(true) - end - - it 'disables two-factor authentication' do - user = build_stubbed(:user, two_factor_enabled: true) - expect { user.two_factor_enabled = false }. - to change { user.two_factor_enabled? }.to(false) - end - end - describe 'authentication token' do it "should have authentication token" do user = create(:user) @@ -308,18 +284,44 @@ describe User do end end - describe 'filter' do - before do - User.delete_all - @user = create :user - @admin = create :user, admin: true - @blocked = create :user, state: :blocked + describe '.filter' do + let(:user) { double } + + it 'filters by active users by default' do + expect(User).to receive(:active).and_return([user]) + + expect(User.filter(nil)).to include user end - it { expect(User.filter("admins")).to eq([@admin]) } - it { expect(User.filter("blocked")).to eq([@blocked]) } - it { expect(User.filter("wop")).to include(@user, @admin, @blocked) } - it { expect(User.filter(nil)).to include(@user, @admin) } + it 'filters by admins' do + expect(User).to receive(:admins).and_return([user]) + + expect(User.filter('admins')).to include user + end + + it 'filters by blocked' do + expect(User).to receive(:blocked).and_return([user]) + + expect(User.filter('blocked')).to include user + end + + it 'filters by two_factor_disabled' do + expect(User).to receive(:without_two_factor).and_return([user]) + + expect(User.filter('two_factor_disabled')).to include user + end + + it 'filters by two_factor_enabled' do + expect(User).to receive(:with_two_factor).and_return([user]) + + expect(User.filter('two_factor_enabled')).to include user + end + + it 'filters by wop' do + expect(User).to receive(:without_projects).and_return([user]) + + expect(User.filter('wop')).to include user + end end describe :not_in_project do