summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorRémy Coutable <remy@rymai.me>2016-06-24 12:01:48 +0200
committerRémy Coutable <remy@rymai.me>2016-06-24 12:01:48 +0200
commitaec3475df94bc9681a723c344f3df05972ebe68c (patch)
tree3ed0b3d284fc4c0657f06489a5403f1c30126b30
parent4477dc249e563e60e126d4f5ad2692297a9584c1 (diff)
downloadgitlab-ce-aec3475df94bc9681a723c344f3df05972ebe68c.tar.gz
Fix an information disclosure when requesting access to a group containing private projects
The issue was with the `User#groups` and `User#projects` associations which goes through the `User#group_members` and `User#project_members`. Initially I chose to use a secure approach by storing the requester's user ID in `Member#created_by_id` instead of `Member#user_id` because I was aware that there was a security risk since I didn't know the codebase well enough. Then during the review, we decided to change that and directly store the requester's user ID into `Member#user_id` (for the sake of simplifying the code I believe), meaning that every `group_members` / `project_members` association would include the requesters by default... My bad for not checking that all the `group_members` / `project_members` associations and the ones that go through them (e.g. `Group#users` and `Project#users`) were made safe with the `where(requested_at: nil)` / `where(members: { requested_at: nil })` scopes. Now they are all secure. Signed-off-by: Rémy Coutable <remy@rymai.me>
-rw-r--r--app/controllers/dashboard/groups_controller.rb2
-rw-r--r--app/models/group.rb2
-rw-r--r--app/models/user.rb4
-rw-r--r--app/views/admin/users/groups.html.haml5
-rw-r--r--spec/features/groups/members/user_requests_access_spec.rb15
-rw-r--r--spec/models/user_spec.rb20
6 files changed, 42 insertions, 6 deletions
diff --git a/app/controllers/dashboard/groups_controller.rb b/app/controllers/dashboard/groups_controller.rb
index 71ba6153021..de6bc689bb7 100644
--- a/app/controllers/dashboard/groups_controller.rb
+++ b/app/controllers/dashboard/groups_controller.rb
@@ -1,5 +1,5 @@
class Dashboard::GroupsController < Dashboard::ApplicationController
def index
- @group_members = current_user.group_members.page(params[:page])
+ @group_members = current_user.group_members.includes(:source).page(params[:page])
end
end
diff --git a/app/models/group.rb b/app/models/group.rb
index e66e04371b2..c70c719e338 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -11,7 +11,7 @@ class Group < Namespace
has_many :users, -> { where(members: { requested_at: nil }) }, through: :group_members
has_many :owners,
- -> { where(members: { access_level: Gitlab::Access::OWNER }) },
+ -> { where(members: { requested_at: nil, access_level: Gitlab::Access::OWNER }) },
through: :group_members,
source: :user
diff --git a/app/models/user.rb b/app/models/user.rb
index 04b220ee13c..599b2fb1191 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -57,7 +57,7 @@ class User < ActiveRecord::Base
# Groups
has_many :members, dependent: :destroy
- has_many :group_members, dependent: :destroy, source: 'GroupMember'
+ has_many :group_members, -> { where(requested_at: nil) }, dependent: :destroy, source: 'GroupMember'
has_many :groups, through: :group_members
has_many :owned_groups, -> { where members: { access_level: Gitlab::Access::OWNER } }, through: :group_members, source: :group
has_many :masters_groups, -> { where members: { access_level: Gitlab::Access::MASTER } }, through: :group_members, source: :group
@@ -65,7 +65,7 @@ class User < ActiveRecord::Base
# Projects
has_many :groups_projects, through: :groups, source: :projects
has_many :personal_projects, through: :namespace, source: :projects
- has_many :project_members, dependent: :destroy, class_name: 'ProjectMember'
+ has_many :project_members, -> { where(requested_at: nil) }, dependent: :destroy, class_name: 'ProjectMember'
has_many :projects, through: :project_members
has_many :created_projects, foreign_key: :creator_id, class_name: 'Project'
has_many :users_star_projects, dependent: :destroy
diff --git a/app/views/admin/users/groups.html.haml b/app/views/admin/users/groups.html.haml
index b0a709a568a..8f6d13b881a 100644
--- a/app/views/admin/users/groups.html.haml
+++ b/app/views/admin/users/groups.html.haml
@@ -1,11 +1,12 @@
- page_title "Groups", @user.name, "Users"
= render 'admin/users/head'
-- if @user.group_members.present?
+- group_members = @user.group_members.includes(:source)
+- if group_members.any?
.panel.panel-default
.panel-heading Groups:
%ul.well-list
- - @user.group_members.each do |group_member|
+ - group_members.each do |group_member|
- group = group_member.group
%li.group_member
%span{class: ("list-item-name" unless group_member.owner?)}
diff --git a/spec/features/groups/members/user_requests_access_spec.rb b/spec/features/groups/members/user_requests_access_spec.rb
index 1ea607cbca0..4944301c938 100644
--- a/spec/features/groups/members/user_requests_access_spec.rb
+++ b/spec/features/groups/members/user_requests_access_spec.rb
@@ -4,6 +4,7 @@ feature 'Groups > Members > User requests access', feature: true do
let(:user) { create(:user) }
let(:owner) { create(:user) }
let(:group) { create(:group, :public) }
+ let!(:project) { create(:project, :private, namespace: group) }
background do
group.add_owner(owner)
@@ -24,6 +25,20 @@ feature 'Groups > Members > User requests access', feature: true do
expect(page).not_to have_content 'Leave Group'
end
+ scenario 'user does not see private projects' do
+ perform_enqueued_jobs { click_link 'Request Access' }
+
+ expect(page).not_to have_content project.name
+ end
+
+ scenario 'user does not see group in the Dashboard > Groups page' do
+ perform_enqueued_jobs { click_link 'Request Access' }
+
+ visit dashboard_groups_path
+
+ expect(page).not_to have_content group.name
+ end
+
scenario 'user is not listed in the group members page' do
click_link 'Request Access'
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 73bee535fe3..328254ed56b 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -31,6 +31,26 @@ describe User, models: true do
it { is_expected.to have_many(:spam_logs).dependent(:destroy) }
it { is_expected.to have_many(:todos).dependent(:destroy) }
it { is_expected.to have_many(:award_emoji).dependent(:destroy) }
+
+ describe '#group_members' do
+ it 'does not include group memberships for which user is a requester' do
+ user = create(:user)
+ group = create(:group, :public)
+ group.request_access(user)
+
+ expect(user.group_members).to be_empty
+ end
+ end
+
+ describe '#project_members' do
+ it 'does not include project memberships for which user is a requester' do
+ user = create(:user)
+ project = create(:project, :public)
+ project.request_access(user)
+
+ expect(user.project_members).to be_empty
+ end
+ end
end
describe 'validations' do