Merge branch 'rs-dev-issue-2414' into 'master'

Allow Admin to filter users by 2FA status

> ![Screen_Shot_2015-06-19_at_4.38.12_PM](https://gitlab.com/gitlab-org/gitlab-ce/uploads/deba7f2a6b8d1548c1d1ac401e0e35a1/Screen_Shot_2015-06-19_at_4.38.12_PM.png)

Closes internal https://dev.gitlab.org/gitlab/gitlabhq/issues/2414

See merge request !852

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)

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
@@ -198,6 +199,8 @@ class User < ActiveRecord::Base
   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)

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

db/migrate/20150620233230_add_default_otp_required_for_login_value.rb

+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

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

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

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 'filters by admins' do
+      expect(User).to receive(:admins).and_return([user])
+
+      expect(User.filter('admins')).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 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