Check permissions before exposing user two factor enabled

Merge branch 'security-dblessing_2fa_member_status-15-0-14-10' into '14-10-stable-ee'

See merge request gitlab-org/security/gitlab!2525

Changelog: security

app/serializers/member_user_entity.rb

@@ -16,7 +16,7 @@ class MemberUserEntity < UserEntity
     user.blocked?
   end
 
-  expose :two_factor_enabled do |user|
+  expose :two_factor_enabled, if: -> (user) { current_user_can_manage_members? || current_user?(user) } do |user|
     user.two_factor_enabled?
   end
 
@@ -25,6 +25,18 @@ class MemberUserEntity < UserEntity
       user.status.emoji
     end
   end
+
+  private
+
+  def current_user_can_manage_members?
+    return false unless options[:source]
+
+    Ability.allowed?(options[:current_user], :"admin_#{options[:source].to_ability_name}_member", options[:source])
+  end
+
+  def current_user?(user)
+    options[:current_user] == user
+  end
 end
 
 MemberUserEntity.prepend_mod_with('MemberUserEntity')

ee/spec/helpers/projects/project_members_helper_spec.rb

@@ -34,7 +34,7 @@ RSpec.describe Projects::ProjectMembersHelper do
 
       expect(project.members.count).to eq(3)
 
-      expect { call_project_members_app_data_json }.not_to exceed_query_limit(control_count).with_threshold(7) # existing n+1
+      expect { call_project_members_app_data_json }.not_to exceed_query_limit(control_count).with_threshold(9) # existing n+1
     end
   end
 

ee/spec/serializers/member_user_entity_spec.rb

@@ -12,7 +12,7 @@ RSpec.describe MemberUserEntity do
   let(:source) { nil }
 
   it 'matches json schema' do
-    expect(entity.to_json).to match_schema('entities/member_user')
+    expect(entity.to_json).to match_schema('entities/member_user_default')
   end
 
   context 'when using on-call management' do

spec/fixtures/api/schemas/entities/member.json

@@ -53,7 +53,7 @@
     },
     "user": {
       "allOf": [
-        { "$ref": "member_user.json" }
+        { "$ref": "member_user_default.json" }
       ]
     },
     "state": { "type": "integer" },

spec/fixtures/api/schemas/entities/member_user_default.json

+{
+  "type": "object",
+  "required": [
+    "id",
+    "name",
+    "username",
+    "created_at",
+    "last_activity_on",
+    "avatar_url",
+    "web_url",
+    "blocked",
+    "show_status"
+  ],
+  "properties": {
+    "id": { "type": "integer" },
+    "name": { "type": "string" },
+    "username": { "type": "string" },
+    "created_at": { "type": ["string"] },
+    "avatar_url": { "type": ["string", "null"] },
+    "web_url": { "type": "string" },
+    "blocked": { "type": "boolean" },
+    "two_factor_enabled": { "type": "boolean" },
+    "availability": { "type": ["string", "null"] },
+    "last_activity_on": { "type": ["string", "null"] },
+    "status": {
+      "type": "object",
+      "required": ["emoji"],
+      "properties": {
+        "emoji": { "type": "string" }
+      },
+      "additionalProperties": false
+    },
+    "show_status": { "type": "boolean" }
+  }
+}

spec/serializers/member_user_entity_spec.rb

@@ -11,7 +11,7 @@ RSpec.describe MemberUserEntity do
   let(:entity_hash) { entity.as_json }
 
   it 'matches json schema' do
-    expect(entity.to_json).to match_schema('entities/member_user')
+    expect(entity.to_json).to match_schema('entities/member_user_default')
   end
 
   it 'correctly exposes `avatar_url`' do
@@ -27,10 +27,8 @@ RSpec.describe MemberUserEntity do
     expect(entity_hash[:blocked]).to be(true)
   end
 
-  it 'correctly exposes `two_factor_enabled`' do
-    allow(user).to receive(:two_factor_enabled?).and_return(true)
-
-    expect(entity_hash[:two_factor_enabled]).to be(true)
+  it 'does not expose `two_factor_enabled` by default' do
+    expect(entity_hash[:two_factor_enabled]).to be(nil)
   end
 
   it 'correctly exposes `status.emoji`' do
@@ -44,4 +42,66 @@ RSpec.describe MemberUserEntity do
   it 'correctly exposes `last_activity_on`' do
     expect(entity_hash[:last_activity_on]).to be(user.last_activity_on)
   end
+
+  context 'when options includes a source' do
+    let(:current_user) { create(:user) }
+    let(:options) { { current_user: current_user, source: source } }
+    let(:entity) { described_class.new(user, options) }
+
+    shared_examples 'correctly exposes user two_factor_enabled' do
+      context 'when the current_user has a role lower than minimum manage member role' do
+        before do
+          source.add_user(current_user, Gitlab::Access::DEVELOPER)
+        end
+
+        it 'does not expose user two_factor_enabled' do
+          expect(entity_hash[:two_factor_enabled]).to be(nil)
+        end
+
+        it 'matches json schema' do
+          expect(entity.to_json).to match_schema('entities/member_user_default')
+        end
+      end
+
+      context 'when the current user has a minimum manage member role or higher' do
+        before do
+          source.add_user(current_user, minimum_manage_member_role)
+        end
+
+        it 'matches json schema' do
+          expect(entity.to_json).to match_schema('entities/member_user_for_admin_member')
+        end
+
+        it 'exposes user two_factor_enabled' do
+          expect(entity_hash[:two_factor_enabled]).to be(false)
+        end
+      end
+
+      context 'when the current user is self' do
+        let(:current_user) { user }
+
+        it 'exposes user two_factor_enabled' do
+          expect(entity_hash[:two_factor_enabled]).to be(false)
+        end
+
+        it 'matches json schema' do
+          expect(entity.to_json).to match_schema('entities/member_user_for_admin_member')
+        end
+      end
+    end
+
+    context 'when the source is a group' do
+      let(:source) { create(:group) }
+      let(:minimum_manage_member_role) { Gitlab::Access::OWNER }
+
+      it_behaves_like 'correctly exposes user two_factor_enabled'
+    end
+
+    context 'when the source is a project' do
+      let(:source) { create(:project) }
+      let(:minimum_manage_member_role) { Gitlab::Access::MAINTAINER }
+
+      it_behaves_like 'correctly exposes user two_factor_enabled'
+    end
+  end
 end