From 97424ea544d0954e582a356586270e983d3bbb7a Mon Sep 17 00:00:00 2001
From: Sean McGivern <sean@gitlab.com>
Date: Tue, 10 May 2016 18:03:55 +0100
Subject: [PATCH] Restrict starred projects to viewable ones
`User#starred_projects` doesn't perform any visibility checks. This has
a couple of problems:
1. It assumes a user can always view all of their starred projects in
perpetuity (project not changed to private, access revoked, etc.).
2. It assumes that we'll only ever allow a user to star a project they
can view. This is currently the case, but bugs happen.
Add `User#viewable_starred_projects` to filter the starred projects by
those the user either has explicit access to, or are public or
internal. Then use that in all places where we list the user's starred
projects.
---
.../dashboard/projects_controller.rb | 2 +-
app/controllers/dashboard_controller.rb | 2 +-
app/models/user.rb | 5 ++++
lib/api/projects.rb | 2 +-
spec/models/user_spec.rb | 24 +++++++++++++++++++
spec/requests/api/projects_spec.rb | 21 +++++++---------
6 files changed, 41 insertions(+), 15 deletions(-)
diff --git a/app/controllers/dashboard/projects_controller.rb b/app/controllers/dashboard/projects_controller.rb
index 71acc244a91..c08eb811532 100644
--- a/app/controllers/dashboard/projects_controller.rb
+++ b/app/controllers/dashboard/projects_controller.rb
@@ -28,7 +28,7 @@ class Dashboard::ProjectsController < Dashboard::ApplicationController
end
def starred
- @projects = current_user.starred_projects.sorted_by_activity
+ @projects = current_user.viewable_starred_projects.sorted_by_activity
@projects = filter_projects(@projects)
@projects = @projects.includes(:namespace, :forked_from_project, :tags)
@projects = @projects.sort(@sort = params[:sort])
diff --git a/app/controllers/dashboard_controller.rb b/app/controllers/dashboard_controller.rb
index 1dce4a21729..4dda4e51f6a 100644
--- a/app/controllers/dashboard_controller.rb
+++ b/app/controllers/dashboard_controller.rb
@@ -25,7 +25,7 @@ class DashboardController < Dashboard::ApplicationController
def load_events
projects =
if params[:filter] == "starred"
- current_user.starred_projects
+ current_user.viewable_starred_projects
else
current_user.authorized_projects
end
diff --git a/app/models/user.rb b/app/models/user.rb
index 1e4814641d1..a0115957f00 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -381,6 +381,11 @@ class User < ActiveRecord::Base
Project.where("projects.id IN (#{projects_union.to_sql})")
end
+ def viewable_starred_projects
+ starred_projects.where("projects.visibility_level IN (?) OR projects.id IN (#{projects_union.to_sql})",
+ [Gitlab::VisibilityLevel::PUBLIC, Gitlab::VisibilityLevel::INTERNAL])
+ end
+
def owned_projects
@owned_projects ||=
Project.where('namespace_id IN (?) OR namespace_id = ?',
diff --git a/lib/api/projects.rb b/lib/api/projects.rb
index cc2c7a0c503..9b595772675 100644
--- a/lib/api/projects.rb
+++ b/lib/api/projects.rb
@@ -44,7 +44,7 @@ module API
# Example Request:
# GET /projects/starred
get '/starred' do
- @projects = current_user.starred_projects
+ @projects = current_user.viewable_starred_projects
@projects = filter_projects(@projects)
@projects = paginate @projects
present @projects, with: Entities::Project
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 26d4e139396..06d1ca3b7da 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -233,6 +233,8 @@ describe User, models: true do
@project = create :project, namespace: @user.namespace
@project_2 = create :project, group: create(:group) # Grant MASTER access to the user
@project_3 = create :project, group: create(:group) # Grant DEVELOPER access to the user
+ @project_4 = create :project, group: create(:group)
+ @project_5 = create :project, group: create(:group)
@project_2.team << [@user, :master]
@project_3.team << [@user, :developer]
@@ -782,4 +784,26 @@ describe User, models: true do
it { is_expected.to eq([private_project]) }
end
+
+ describe '#viewable_starred_projects' do
+ let(:user) { create(:user) }
+ let(:public_project) { create(:project, :public) }
+ let(:private_project) { create(:project, :private) }
+ let(:private_viewable_project) { create(:project, :private) }
+ let(:viewable?) { -> (project) { user.can?(:read_project, project) } }
+ let(:projects) { [public_project, private_project, private_viewable_project] }
+
+ before do
+ private_viewable_project.team << [user, Gitlab::Access::MASTER]
+ projects.each { |project| user.toggle_star(project) }
+ end
+
+ it 'returns only starred projects the user can view' do
+ expect(user.viewable_starred_projects).to all(satisfy(&viewable?))
+ end
+
+ it 'rejects only starred projects the user can not view' do
+ expect(projects - user.viewable_starred_projects).not_to include(satisfy(&viewable?))
+ end
+ end
end
diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb
index 66193eac051..f167813e07d 100644
--- a/spec/requests/api/projects_spec.rb
+++ b/spec/requests/api/projects_spec.rb
@@ -10,20 +10,20 @@ describe API::API, api: true do
let(:admin) { create(:admin) }
let(:project) { create(:project, creator_id: user.id, namespace: user.namespace) }
let(:project2) { create(:project, path: 'project2', creator_id: user.id, namespace: user.namespace) }
- let(:project3) { create(:project, path: 'project3', creator_id: user.id, namespace: user.namespace) }
let(:snippet) { create(:project_snippet, :public, author: user, project: project, title: 'example') }
let(:project_member) { create(:project_member, :master, user: user, project: project) }
let(:project_member2) { create(:project_member, :developer, user: user3, project: project) }
let(:user4) { create(:user) }
let(:project3) do
create(:project,
+ :private,
name: 'second_project',
path: 'second_project',
creator_id: user.id,
namespace: user.namespace,
merge_requests_enabled: false,
issues_enabled: false, wiki_enabled: false,
- snippets_enabled: false, visibility_level: 0)
+ snippets_enabled: false)
end
let(:project_member3) do
create(:project_member,
@@ -164,21 +164,18 @@ describe API::API, api: true do
end
describe 'GET /projects/starred' do
+ let(:public_project) { create(:project, :public) }
+
before do
- admin.starred_projects << project
- admin.save!
+ project_member2
+ user3.update_attributes(starred_projects: [project, project2, project3, public_project])
end
- it 'should return the starred projects' do
- get api('/projects/all', admin)
+ it 'should return the starred projects viewable by the user' do
+ get api('/projects/starred', user3)
expect(response.status).to eq(200)
expect(json_response).to be_an Array
-
- expect(json_response).to satisfy do |response|
- response.one? do |entry|
- entry['name'] == project.name
- end
- end
+ expect(json_response.map { |project| project['id'] }).to contain_exactly(project.id, public_project.id)
end
end
--
GitLab