Merge branch 'master' into admin-edit-identities

CHANGELOG

@@ -11,6 +11,8 @@ v 7.13.0 (unreleased)
   - Admin can edit and remove user identities
   - 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)

README.md

@@ -2,7 +2,7 @@
 
 The source of GitLab Community Edition is [hosted on GitLab.com](https://gitlab.com/gitlab-org/gitlab-ce/) and there are mirrors to make [contributing](CONTRIBUTING.md) as easy as possible.
 
-# ![logo](https://about.gitlab.com/images/gitlab_logo.png) GitLab
+# ![logo](https://about.gitlab.com/images/logo.svg) GitLab
 
 ## Open source software to collaborate on code
 
@@ -101,4 +101,4 @@ Please see [Getting help for GitLab](https://about.gitlab.com/getting-help/) on
 ## Is it awesome?
 
 Thanks for [asking this question](https://twitter.com/supersloth/status/489462789384056832) Joshua.
-[These people](https://twitter.com/gitlab/favorites) seem to like it.
\ No newline at end of file
+[These people](https://twitter.com/gitlab/favorites) seem to like it.

VERSION

-7.12.0.pre
\ No newline at end of file
+7.13.0.pre

app/assets/stylesheets/generic/header.scss

@@ -3,6 +3,8 @@
  *
  */
 header {
+  transition-duration: .3s;
+
   &.navbar-empty {
     background: #FFF;
     border-bottom: 1px solid #EEE;
@@ -67,28 +69,34 @@ header {
     float: left;
     height: $header-height;
     width: $sidebar_width;
+    transition-duration: .3s;
 
     a {
       float: left;
       height: $header-height;
       width: 100%;
       padding: ($header-height - 36 ) / 2 8px;
-
-      h3 {
-        width: 158px;
-        float: left;
-        margin: 0;
-        margin-left: 14px;
-        font-size: 18px;
-        line-height: $header-height - 14;
-        font-weight: normal;
-      }
+      overflow: hidden;
 
       img {
         width: 36px;
         height: 36px;
         float: left;
       }
+
+      .gitlab-text-container {
+        width: 230px;
+
+        h3 {
+          width: 158px;
+          float: left;
+          margin: 0;
+          margin-left: 14px;
+          font-size: 18px;
+          line-height: $header-height - 14;
+          font-weight: normal;
+        }
+      }
     }
 
     &:hover {

app/assets/stylesheets/generic/sidebar.scss

@@ -4,12 +4,14 @@
     top: 0;
     left: 0;
     height: 100%;
+    transition-duration: .3s;
   }
 }
 
 .sidebar-wrapper {
   z-index: 99;
   background: $background-color;
+  transition-duration: .3s;
 }
 
 .content-wrapper {
@@ -19,8 +21,10 @@
 }
 
 .nav-sidebar {
+  transition-duration: .3s;
   margin: 0;
   list-style: none;
+  overflow: hidden;
 
   &.navbar-collapse {
     padding: 0px !important;
@@ -34,9 +38,6 @@
   @include border-radius(6px);
 }
 
-.nav-sidebar li {
-}
-
 .nav-sidebar li {
   &.separate-item {
     padding-top: 10px;
@@ -48,7 +49,7 @@
     display: block;
     text-decoration: none;
     padding: 8px 15px;
-    font-size: 13px;
+    font-size: 14px;
     line-height: 20px;
     padding-left: 16px;
 
@@ -79,6 +80,7 @@
 
 @mixin expanded-sidebar {
   padding-left: $sidebar_width;
+  transition-duration: .3s;
 
   .sidebar-wrapper {
     width: $sidebar_width;
@@ -89,6 +91,10 @@
       top: $header-height;
       width: $sidebar_width;
     }
+
+    .nav-sidebar li a{
+      width: 230px;
+    }
   }
 
   .content-wrapper {
@@ -98,6 +104,7 @@
 
 @mixin folded-sidebar {
   padding-left: 50px;
+  transition-duration: .3s;
 
   .sidebar-wrapper {
     width: $sidebar_collapsed_width;
@@ -109,10 +116,10 @@
       width: $sidebar_collapsed_width;
 
       li a {
-        padding-left: 18px;
         font-size: 14px;
         padding: 8px 15px;
-        text-align: center;
+        text-align: left;
+        padding-left: 16px;
 
 
         & > span {
@@ -144,6 +151,7 @@
   height: 28px;
   text-align: center;
   line-height: 28px;
+  transition-duration: .3s;
 }
 
 .collapse-nav a:hover {
@@ -180,8 +188,10 @@
   bottom: 0;
   width: 100%;
   padding: 10px;
+  overflow: hidden;
 
   .username {
     margin-top: 5px;
+    width: 230px;
   }
 }

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)

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

app/views/layouts/header/_default.html.haml

@@ -3,7 +3,8 @@
     .header-logo
       = link_to root_path, class: 'home', title: 'Dashboard', id: 'js-shortcuts-home', data: {toggle: 'tooltip', placement: 'bottom'} do
         = brand_header_logo
-        %h3 GitLab
+        .gitlab-text-container
+          %h3 GitLab
     .header-content
       %button.navbar-toggle{type: 'button'}
         %span.sr-only Toggle navigation

app/views/layouts/header/_public.html.haml

@@ -3,7 +3,8 @@
     .header-logo
       = link_to explore_root_path, class: "home" do
         = brand_header_logo
-        %h3 GitLab
+        .gitlab-text-container
+          %h3 GitLab
     .header-content
       - unless current_controller?('sessions')
         .pull-right

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

features/steps/groups.rb

@@ -128,14 +128,14 @@ class Spinach::Features::Groups < Spinach::FeatureSteps
   end
 
   step 'I change group "Owned" avatar' do
-    attach_file(:group_avatar, File.join(Rails.root, 'public', 'gitlab_logo.png'))
+    attach_file(:group_avatar, File.join(Rails.root, 'spec', 'fixtures', 'banana_sample.gif'))
     click_button "Save group"
     Group.find_by(name: "Owned").reload
   end
 
   step 'I should see new group "Owned" avatar' do
     expect(Group.find_by(name: "Owned").avatar).to be_instance_of AvatarUploader
-    expect(Group.find_by(name: "Owned").avatar.url).to eq "/uploads/group/avatar/#{ Group.find_by(name:"Owned").id }/gitlab_logo.png"
+    expect(Group.find_by(name: "Owned").avatar.url).to eq "/uploads/group/avatar/#{ Group.find_by(name:"Owned").id }/banana_sample.gif"
   end
 
   step 'I should see the "Remove avatar" button' do
@@ -143,7 +143,7 @@ class Spinach::Features::Groups < Spinach::FeatureSteps
   end
 
   step 'I have group "Owned" avatar' do
-    attach_file(:group_avatar, File.join(Rails.root, 'public', 'gitlab_logo.png'))
+    attach_file(:group_avatar, File.join(Rails.root, 'spec', 'fixtures', 'banana_sample.gif'))
     click_button "Save group"
     Group.find_by(name: "Owned").reload
   end

features/steps/profile/profile.rb

@@ -27,14 +27,14 @@ class Spinach::Features::Profile < Spinach::FeatureSteps
   end
 
   step 'I change my avatar' do
-    attach_file(:user_avatar, File.join(Rails.root, 'public', 'gitlab_logo.png'))
+    attach_file(:user_avatar, File.join(Rails.root, 'spec', 'fixtures', 'banana_sample.gif'))
     click_button "Save changes"
     @user.reload
   end
 
   step 'I should see new avatar' do
     expect(@user.avatar).to be_instance_of AvatarUploader
-    expect(@user.avatar.url).to eq "/uploads/user/avatar/#{ @user.id }/gitlab_logo.png"
+    expect(@user.avatar.url).to eq "/uploads/user/avatar/#{ @user.id }/banana_sample.gif"
   end
 
   step 'I should see the "Remove avatar" button' do
@@ -42,7 +42,7 @@ class Spinach::Features::Profile < Spinach::FeatureSteps
   end
 
   step 'I have an avatar' do
-    attach_file(:user_avatar, File.join(Rails.root, 'public', 'gitlab_logo.png'))
+    attach_file(:user_avatar, File.join(Rails.root, 'spec', 'fixtures', 'banana_sample.gif'))
     click_button "Save changes"
     @user.reload
   end

features/steps/project/project.rb

@@ -28,7 +28,7 @@ class Spinach::Features::Project < Spinach::FeatureSteps
   step 'I change the project avatar' do
     attach_file(
       :project_avatar,
-      File.join(Rails.root, 'public', 'gitlab_logo.png')
+      File.join(Rails.root, 'spec', 'fixtures', 'banana_sample.gif')
     )
     click_button 'Save changes'
     @project.reload
@@ -37,7 +37,7 @@ class Spinach::Features::Project < Spinach::FeatureSteps
   step 'I should see new project avatar' do
     expect(@project.avatar).to be_instance_of AvatarUploader
     url = @project.avatar.url
-    expect(url).to eq "/uploads/project/avatar/#{ @project.id }/gitlab_logo.png"
+    expect(url).to eq "/uploads/project/avatar/#{ @project.id }/banana_sample.gif"
   end
 
   step 'I should see the "Remove avatar" button' do
@@ -47,7 +47,7 @@ class Spinach::Features::Project < Spinach::FeatureSteps
   step 'I have an project avatar' do
     attach_file(
       :project_avatar,
-      File.join(Rails.root, 'public', 'gitlab_logo.png')
+      File.join(Rails.root, 'spec', 'fixtures', 'banana_sample.gif')
     )
     click_button 'Save changes'
     @project.reload

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/helpers/application_helper_spec.rb

@@ -40,15 +40,15 @@ describe ApplicationHelper do
   end
 
   describe 'project_icon' do
-    avatar_file_path = File.join(Rails.root, 'public', 'gitlab_logo.png')
+    avatar_file_path = File.join(Rails.root, 'spec', 'fixtures', 'banana_sample.gif')
 
     it 'should return an url for the avatar' do
       project = create(:project)
       project.avatar = File.open(avatar_file_path)
       project.save!
-      avatar_url = "http://localhost/uploads/project/avatar/#{ project.id }/gitlab_logo.png"
+      avatar_url = "http://localhost/uploads/project/avatar/#{ project.id }/banana_sample.gif"
       expect(project_icon("#{project.namespace.to_param}/#{project.to_param}").to_s).to eq(
-        "<img alt=\"Gitlab logo\" src=\"#{avatar_url}\" />"
+        "<img alt=\"Banana sample\" src=\"#{avatar_url}\" />"
       )
     end
 
@@ -65,14 +65,14 @@ describe ApplicationHelper do
   end
 
   describe 'avatar_icon' do
-    avatar_file_path = File.join(Rails.root, 'public', 'gitlab_logo.png')
+    avatar_file_path = File.join(Rails.root, 'spec', 'fixtures', 'banana_sample.gif')
 
     it 'should return an url for the avatar' do
       user = create(:user)
       user.avatar = File.open(avatar_file_path)
       user.save!
       expect(avatar_icon(user.email).to_s).
-        to match("/uploads/user/avatar/#{ user.id }/gitlab_logo.png")
+        to match("/uploads/user/avatar/#{ user.id }/banana_sample.gif")
     end
 
     it 'should return an url for the avatar with relative url' do
@@ -83,7 +83,7 @@ describe ApplicationHelper do
       user.avatar = File.open(avatar_file_path)
       user.save!
       expect(avatar_icon(user.email).to_s).
-        to match("/gitlab/uploads/user/avatar/#{ user.id }/gitlab_logo.png")
+        to match("/gitlab/uploads/user/avatar/#{ user.id }/banana_sample.gif")
     end
 
     it 'should call gravatar_icon when no avatar is present' do

spec/helpers/groups_helper.rb

@@ -2,14 +2,14 @@ require 'spec_helper'
 
 describe GroupsHelper do
   describe 'group_icon' do
-    avatar_file_path = File.join(Rails.root, 'public', 'gitlab_logo.png')
+    avatar_file_path = File.join(Rails.root, 'spec', 'fixtures', 'banana_sample.gif')
 
     it 'should return an url for the avatar' do
       group = create(:group)
       group.avatar = File.open(avatar_file_path)
       group.save!
       expect(group_icon(group.path).to_s).
-        to match("/uploads/group/avatar/#{ group.id }/gitlab_logo.png")
+        to match("/uploads/group/avatar/#{ group.id }/banana_sample.gif")
     end
 
     it 'should give default avatar_icon when no avatar is present' 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