Commit 97424ea5 authored by Sean McGivern's avatar Sean McGivern

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.
parent 98d8e3fe
......@@ -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])
......
......@@ -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
......
......@@ -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 = ?',
......
......@@ -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
......
......@@ -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
......@@ -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
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment