Merge branch '17249-starred' into 'master'

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.

Closes #17249.

See merge request !4108

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])

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

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})",
+                           [Project::PUBLIC, Project::INTERNAL])
+  end
+
   def owned_projects
     @owned_projects ||=
       Project.where('namespace_id IN (?) OR namespace_id = ?',

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

spec/models/user_spec.rb

@@ -782,4 +782,23 @@ 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(:empty_project, :public) }
+    let(:private_project) { create(:empty_project, :private) }
+    let(:private_viewable_project) { create(:empty_project, :private) }
+
+    before do
+      private_viewable_project.team << [user, Gitlab::Access::MASTER]
+
+      [public_project, private_project, private_viewable_project].each do |project|
+        user.toggle_star(project)
+      end
+    end
+
+    it 'returns only starred projects the user can view' do
+      expect(user.viewable_starred_projects).not_to include(private_project)
+    end
+  end
 end

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