Merge branch '4269-public-api' into 'master'

Allow public access to some Project API endpoints

## What does this MR do?

This opens up a few endpoints in the Project API:

- `GET /projects/visible` (returns public projects only)
- `GET /projects/search/:query` (returns results only for public projects)
- `GET /projects/:id` (only if the project is public)
- `GET /projects/:id/events` (only if the project is public)
- `GET /projects/:id/users` (only if the project is public)

## Are there points in the code the reviewer needs to double check?

I've chosen to explicitly add `authenticate!` to GET methods that still need a current user.

## Does this MR meet the acceptance criteria?

- [x] [Changelog entry](https://docs.gitlab.com/ce/development/changelog.html) added
- [ ] [Documentation created/updated](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/doc_styleguide.md)
- [x] API support added
- Tests
  - [x] Added for this feature/bug
  - [ ] All builds are passing

Part of #4269

See merge request !7843

changelogs/unreleased/4269-public-api.yml

+---
+title: Allow public access to some Project API endpoints
+merge_request: 7843
+author:

lib/api/helpers.rb

@@ -141,6 +141,10 @@ module API
       unauthorized! unless current_user
     end
 
+    def authenticate_non_get!
+      authenticate! unless %w[GET HEAD].include?(route.route_method)
+    end
+
     def authenticate_by_gitlab_shell_token!
       input = params['secret_token'].try(:chomp)
       unless Devise.secure_compare(secret_token, input)
@@ -149,6 +153,7 @@ module API
     end
 
     def authenticated_as_admin!
+      authenticate!
       forbidden! unless current_user.is_admin?
     end
 

lib/api/projects.rb

@@ -3,7 +3,7 @@ module API
   class Projects < Grape::API
     include PaginationParams
 
-    before { authenticate! }
+    before { authenticate_non_get! }
 
     helpers do
       params :optional_params do
@@ -61,7 +61,7 @@ module API
         end
       end
 
-      desc 'Get a projects list for authenticated user' do
+      desc 'Get a list of visible projects for authenticated user' do
         success Entities::BasicProjectDetails
       end
       params do
@@ -70,15 +70,15 @@ module API
         use :filter_params
         use :pagination
       end
-      get do
-        projects = current_user.authorized_projects
+      get '/visible' do
+        projects = ProjectsFinder.new.execute(current_user)
         projects = filter_projects(projects)
-        entity = params[:simple] ? Entities::BasicProjectDetails : Entities::ProjectWithAccess
+        entity = params[:simple] || !current_user ? Entities::BasicProjectDetails : Entities::ProjectWithAccess
 
         present paginate(projects), with: entity, user: current_user
       end
 
-      desc 'Get a list of visible projects for authenticated user' do
+      desc 'Get a projects list for authenticated user' do
         success Entities::BasicProjectDetails
       end
       params do
@@ -87,8 +87,10 @@ module API
         use :filter_params
         use :pagination
       end
-      get '/visible' do
-        projects = ProjectsFinder.new.execute(current_user)
+      get do
+        authenticate!
+
+        projects = current_user.authorized_projects
         projects = filter_projects(projects)
         entity = params[:simple] ? Entities::BasicProjectDetails : Entities::ProjectWithAccess
 
@@ -103,6 +105,8 @@ module API
         use :pagination
       end
       get '/owned' do
+        authenticate!
+
         projects = current_user.owned_projects
         projects = filter_projects(projects)
 
@@ -117,6 +121,8 @@ module API
         use :pagination
       end
       get '/starred' do
+        authenticate!
+
         projects = current_user.viewable_starred_projects
         projects = filter_projects(projects)
 
@@ -132,6 +138,7 @@ module API
       end
       get '/all' do
         authenticated_as_admin!
+
         projects = Project.all
         projects = filter_projects(projects)
 
@@ -213,7 +220,8 @@ module API
         success Entities::ProjectWithAccess
       end
       get ":id" do
-        present user_project, with: Entities::ProjectWithAccess, user: current_user,
+        entity = current_user ? Entities::ProjectWithAccess : Entities::BasicProjectDetails
+        present user_project, with: entity, user: current_user,
                               user_can_admin_project: can?(current_user, :admin_project, user_project)
       end
 
@@ -433,7 +441,7 @@ module API
         use :pagination
       end
       get ':id/users' do
-        users = User.where(id: user_project.team.users.map(&:id))
+        users = user_project.team.users
         users = users.search(params[:search]) if params[:search].present?
 
         present paginate(users), with: Entities::UserBasic

spec/requests/api/api_helpers_spec.rb

@@ -47,7 +47,7 @@ describe API::Helpers, api: true do
   end
 
   def error!(message, status)
-    raise Exception
+    raise Exception.new("#{status} - #{message}")
   end
 
   describe ".current_user" do
@@ -290,4 +290,56 @@ describe API::Helpers, api: true do
       handle_api_exception(exception)
     end
   end
+
+  describe '.authenticate_non_get!' do
+    %w[HEAD GET].each do |method_name|
+      context "method is #{method_name}" do
+        before do
+          expect_any_instance_of(self.class).to receive(:route).and_return(double(route_method: method_name))
+        end
+
+        it 'does not raise an error' do
+          expect_any_instance_of(self.class).not_to receive(:authenticate!)
+
+          expect { authenticate_non_get! }.not_to raise_error
+        end
+      end
+    end
+
+    %w[POST PUT PATCH DELETE].each do |method_name|
+      context "method is #{method_name}" do
+        before do
+          expect_any_instance_of(self.class).to receive(:route).and_return(double(route_method: method_name))
+        end
+
+        it 'calls authenticate!' do
+          expect_any_instance_of(self.class).to receive(:authenticate!)
+
+          authenticate_non_get!
+        end
+      end
+    end
+  end
+
+  describe '.authenticate!' do
+    context 'current_user is nil' do
+      before do
+        expect_any_instance_of(self.class).to receive(:current_user).and_return(nil)
+      end
+
+      it 'returns a 401 response' do
+        expect { authenticate! }.to raise_error '401 - {"message"=>"401 Unauthorized"}'
+      end
+    end
+
+    context 'current_user is present' do
+      before do
+        expect_any_instance_of(self.class).to receive(:current_user).and_return(true)
+      end
+
+      it 'does not raise an error' do
+        expect { authenticate! }.not_to raise_error
+      end
+    end
+  end
 end

spec/requests/api/projects_spec.rb

@@ -200,32 +200,43 @@ describe API::API, api: true  do
   end
 
   describe 'GET /projects/visible' do
-    let(:public_project) { create(:project, :public) }
+    shared_examples_for 'visible projects response' do
+      it 'returns the visible projects' do
+        get api('/projects/visible', current_user)
+
+        expect(response).to have_http_status(200)
+        expect(json_response).to be_an Array
+        expect(json_response.map { |p| p['id'] }).to contain_exactly(*projects.map(&:id))
+      end
+    end
 
+    let!(:public_project) { create(:project, :public) }
     before do
-      public_project
       project
       project2
       project3
       project4
     end
 
-    it 'returns the projects viewable by the user' do
-      get api('/projects/visible', user)
-
-      expect(response).to have_http_status(200)
-      expect(json_response).to be_an Array
-      expect(json_response.map { |project| project['id'] }).
-        to contain_exactly(public_project.id, project.id, project2.id, project3.id)
+    context 'when unauthenticated' do
+      it_behaves_like 'visible projects response' do
+        let(:current_user) { nil }
+        let(:projects) { [public_project] }
+      end
     end
 
-    it 'shows only public projects when the user only has access to those' do
-      get api('/projects/visible', user2)
+    context 'when authenticated' do
+      it_behaves_like 'visible projects response' do
+        let(:current_user) { user }
+        let(:projects) { [public_project, project, project2, project3] }
+      end
+    end
 
-      expect(response).to have_http_status(200)
-      expect(json_response).to be_an Array
-      expect(json_response.map { |project| project['id'] }).
-        to contain_exactly(public_project.id)
+    context 'when authenticated as a different user' do
+      it_behaves_like 'visible projects response' do
+        let(:current_user) { user2 }
+        let(:projects) { [public_project] }
+      end
     end
   end
 
@@ -528,135 +539,150 @@ describe API::API, api: true  do
   end
 
   describe 'GET /projects/:id' do
-    before { project }
-    before { project_member }
-
-    it 'returns a project by id' do
-      group = create(:group)
-      link = create(:project_group_link, project: project, group: group)
+    context 'when unauthenticated' do
+      it 'returns the public projects' do
+        public_project = create(:project, :public)
 
-      get api("/projects/#{project.id}", user)
+        get api("/projects/#{public_project.id}")
 
-      expect(response).to have_http_status(200)
-      expect(json_response['id']).to eq(project.id)
-      expect(json_response['description']).to eq(project.description)
-      expect(json_response['default_branch']).to eq(project.default_branch)
-      expect(json_response['tag_list']).to be_an Array
-      expect(json_response['public']).to be_falsey
-      expect(json_response['archived']).to be_falsey
-      expect(json_response['visibility_level']).to be_present
-      expect(json_response['ssh_url_to_repo']).to be_present
-      expect(json_response['http_url_to_repo']).to be_present
-      expect(json_response['web_url']).to be_present
-      expect(json_response['owner']).to be_a Hash
-      expect(json_response['owner']).to be_a Hash
-      expect(json_response['name']).to eq(project.name)
-      expect(json_response['path']).to be_present
-      expect(json_response['issues_enabled']).to be_present
-      expect(json_response['merge_requests_enabled']).to be_present
-      expect(json_response['wiki_enabled']).to be_present
-      expect(json_response['builds_enabled']).to be_present
-      expect(json_response['snippets_enabled']).to be_present
-      expect(json_response['container_registry_enabled']).to be_present
-      expect(json_response['created_at']).to be_present
-      expect(json_response['last_activity_at']).to be_present
-      expect(json_response['shared_runners_enabled']).to be_present
-      expect(json_response['creator_id']).to be_present
-      expect(json_response['namespace']).to be_present
-      expect(json_response['avatar_url']).to be_nil
-      expect(json_response['star_count']).to be_present
-      expect(json_response['forks_count']).to be_present
-      expect(json_response['public_builds']).to be_present
-      expect(json_response['shared_with_groups']).to be_an Array
-      expect(json_response['shared_with_groups'].length).to eq(1)
-      expect(json_response['shared_with_groups'][0]['group_id']).to eq(group.id)
-      expect(json_response['shared_with_groups'][0]['group_name']).to eq(group.name)
-      expect(json_response['shared_with_groups'][0]['group_access_level']).to eq(link.group_access)
-      expect(json_response['only_allow_merge_if_build_succeeds']).to eq(project.only_allow_merge_if_build_succeeds)
-      expect(json_response['only_allow_merge_if_all_discussions_are_resolved']).to eq(project.only_allow_merge_if_all_discussions_are_resolved)
-    end
-
-    it 'returns a project by path name' do
-      get api("/projects/#{project.id}", user)
-      expect(response).to have_http_status(200)
-      expect(json_response['name']).to eq(project.name)
+        expect(response).to have_http_status(200)
+        expect(json_response['id']).to eq(public_project.id)
+        expect(json_response['description']).to eq(public_project.description)
+        expect(json_response.keys).not_to include('permissions')
+      end
     end
 
-    it 'returns a 404 error if not found' do
-      get api('/projects/42', user)
-      expect(response).to have_http_status(404)
-      expect(json_response['message']).to eq('404 Project Not Found')
-    end
+    context 'when authenticated' do
+      before do
+        project
+        project_member
+      end
 
-    it 'returns a 404 error if user is not a member' do
-      other_user = create(:user)
-      get api("/projects/#{project.id}", other_user)
-      expect(response).to have_http_status(404)
-    end
+      it 'returns a project by id' do
+        group = create(:group)
+        link = create(:project_group_link, project: project, group: group)
 
-    it 'handles users with dots' do
-      dot_user = create(:user, username: 'dot.user')
-      project = create(:project, creator_id: dot_user.id, namespace: dot_user.namespace)
+        get api("/projects/#{project.id}", user)
 
-      get api("/projects/#{dot_user.namespace.name}%2F#{project.path}", dot_user)
-      expect(response).to have_http_status(200)
-      expect(json_response['name']).to eq(project.name)
-    end
+        expect(response).to have_http_status(200)
+        expect(json_response['id']).to eq(project.id)
+        expect(json_response['description']).to eq(project.description)
+        expect(json_response['default_branch']).to eq(project.default_branch)
+        expect(json_response['tag_list']).to be_an Array
+        expect(json_response['public']).to be_falsey
+        expect(json_response['archived']).to be_falsey
+        expect(json_response['visibility_level']).to be_present
+        expect(json_response['ssh_url_to_repo']).to be_present
+        expect(json_response['http_url_to_repo']).to be_present
+        expect(json_response['web_url']).to be_present
+        expect(json_response['owner']).to be_a Hash
+        expect(json_response['owner']).to be_a Hash
+        expect(json_response['name']).to eq(project.name)
+        expect(json_response['path']).to be_present
+        expect(json_response['issues_enabled']).to be_present
+        expect(json_response['merge_requests_enabled']).to be_present
+        expect(json_response['wiki_enabled']).to be_present
+        expect(json_response['builds_enabled']).to be_present
+        expect(json_response['snippets_enabled']).to be_present
+        expect(json_response['container_registry_enabled']).to be_present
+        expect(json_response['created_at']).to be_present
+        expect(json_response['last_activity_at']).to be_present
+        expect(json_response['shared_runners_enabled']).to be_present
+        expect(json_response['creator_id']).to be_present
+        expect(json_response['namespace']).to be_present
+        expect(json_response['avatar_url']).to be_nil
+        expect(json_response['star_count']).to be_present
+        expect(json_response['forks_count']).to be_present
+        expect(json_response['public_builds']).to be_present
+        expect(json_response['shared_with_groups']).to be_an Array
+        expect(json_response['shared_with_groups'].length).to eq(1)
+        expect(json_response['shared_with_groups'][0]['group_id']).to eq(group.id)
+        expect(json_response['shared_with_groups'][0]['group_name']).to eq(group.name)
+        expect(json_response['shared_with_groups'][0]['group_access_level']).to eq(link.group_access)
+        expect(json_response['only_allow_merge_if_build_succeeds']).to eq(project.only_allow_merge_if_build_succeeds)
+        expect(json_response['only_allow_merge_if_all_discussions_are_resolved']).to eq(project.only_allow_merge_if_all_discussions_are_resolved)
+      end
+
+      it 'returns a project by path name' do
+        get api("/projects/#{project.id}", user)
+        expect(response).to have_http_status(200)
+        expect(json_response['name']).to eq(project.name)
+      end
 
-    describe 'permissions' do
-      context 'all projects' do
-        before { project.team << [user, :master] }
+      it 'returns a 404 error if not found' do
+        get api('/projects/42', user)
+        expect(response).to have_http_status(404)
+        expect(json_response['message']).to eq('404 Project Not Found')
+      end
 
-        it 'contains permission information' do
-          get api("/projects", user)
+      it 'returns a 404 error if user is not a member' do
+        other_user = create(:user)
+        get api("/projects/#{project.id}", other_user)
+        expect(response).to have_http_status(404)
+      end
 
-          expect(response).to have_http_status(200)
-          expect(json_response.first['permissions']['project_access']['access_level']).
-              to eq(Gitlab::Access::MASTER)
-          expect(json_response.first['permissions']['group_access']).to be_nil
-        end
+      it 'handles users with dots' do
+        dot_user = create(:user, username: 'dot.user')
+        project = create(:project, creator_id: dot_user.id, namespace: dot_user.namespace)
+
+        get api("/projects/#{dot_user.namespace.name}%2F#{project.path}", dot_user)
+        expect(response).to have_http_status(200)
+        expect(json_response['name']).to eq(project.name)
       end
 
-      context 'personal project' do
-        it 'sets project access and returns 200' do
-          project.team << [user, :master]
-          get api("/projects/#{project.id}", user)
+      describe 'permissions' do
+        context 'all projects' do
+          before { project.team << [user, :master] }
 
-          expect(response).to have_http_status(200)
-          expect(json_response['permissions']['project_access']['access_level']).
+          it 'contains permission information' do
+            get api("/projects", user)
+
+            expect(response).to have_http_status(200)
+            expect(json_response.first['permissions']['project_access']['access_level']).
             to eq(Gitlab::Access::MASTER)
-          expect(json_response['permissions']['group_access']).to be_nil
+            expect(json_response.first['permissions']['group_access']).to be_nil
+          end
         end
-      end
 
-      context 'group project' do
-        let(:project2) { create(:project, group: create(:group)) }
+        context 'personal project' do
+          it 'sets project access and returns 200' do
+            project.team << [user, :master]
+            get api("/projects/#{project.id}", user)
 
-        before { project2.group.add_owner(user) }
+            expect(response).to have_http_status(200)
+            expect(json_response['permissions']['project_access']['access_level']).
+            to eq(Gitlab::Access::MASTER)
+            expect(json_response['permissions']['group_access']).to be_nil
+          end
+        end
 
-        it 'sets the owner and return 200' do
-          get api("/projects/#{project2.id}", user)
+        context 'group project' do
+          let(:project2) { create(:project, group: create(:group)) }
 
-          expect(response).to have_http_status(200)
-          expect(json_response['permissions']['project_access']).to be_nil
-          expect(json_response['permissions']['group_access']['access_level']).
+          before { project2.group.add_owner(user) }
+
+          it 'sets the owner and return 200' do
+            get api("/projects/#{project2.id}", user)
+
+            expect(response).to have_http_status(200)
+            expect(json_response['permissions']['project_access']).to be_nil
+            expect(json_response['permissions']['group_access']['access_level']).
             to eq(Gitlab::Access::OWNER)
+          end
         end
       end
     end
   end
 
   describe 'GET /projects/:id/events' do
-    before { project_member2 }
-
-    context 'valid request' do
-      before do
+    shared_examples_for 'project events response' do
+      it 'returns the project events' do
+        member = create(:user)
+        create(:project_member, :developer, user: member, project: project)
         note = create(:note_on_issue, note: 'What an awesome day!', project: project)
         EventCreateService.new.leave_note(note, note.author)
-      end
 
-      it 'returns all events' do
-        get api("/projects/#{project.id}/events", user)
+        get api("/projects/#{project.id}/events", current_user)
 
         expect(response).to have_http_status(200)
 
@@ -669,24 +695,90 @@ describe API::API, api: true  do
 
         expect(last_event['action_name']).to eq('joined')
         expect(last_event['project_id'].to_i).to eq(project.id)
-        expect(last_event['author_username']).to eq(user3.username)
-        expect(last_event['author']['name']).to eq(user3.name)
+        expect(last_event['author_username']).to eq(member.username)
+        expect(last_event['author']['name']).to eq(member.name)
       end
     end
 
-    it 'returns a 404 error if not found' do
-      get api('/projects/42/events', user)
+    context 'when unauthenticated' do
+      it_behaves_like 'project events response' do
+        let(:project) { create(:project, :public) }
+        let(:current_user) { nil }
+      end
+    end
 
-      expect(response).to have_http_status(404)
-      expect(json_response['message']).to eq('404 Project Not Found')
+    context 'when authenticated' do
+      context 'valid request' do
+        it_behaves_like 'project events response' do
+          let(:current_user) { user }
+        end
+      end
+
+      it 'returns a 404 error if not found' do
+        get api('/projects/42/events', user)
+
+        expect(response).to have_http_status(404)
+        expect(json_response['message']).to eq('404 Project Not Found')
+      end
+
+      it 'returns a 404 error if user is not a member' do
+        other_user = create(:user)
+
+        get api("/projects/#{project.id}/events", other_user)
+
+        expect(response).to have_http_status(404)
+      end
     end
+  end
 
-    it 'returns a 404 error if user is not a member' do
-      other_user = create(:user)
+  describe 'GET /projects/:id/users' do
+    shared_examples_for 'project users response' do
+      it 'returns the project users' do
+        member = create(:user)
+        create(:project_member, :developer, user: member, project: project)
 
-      get api("/projects/#{project.id}/events", other_user)
+        get api("/projects/#{project.id}/users", current_user)
 
-      expect(response).to have_http_status(404)
+        expect(response).to have_http_status(200)
+        expect(json_response).to be_an Array
+        expect(json_response.size).to eq(1)
+
+        first_user = json_response.first
+
+        expect(first_user['username']).to eq(member.username)
+        expect(first_user['name']).to eq(member.name)
+        expect(first_user.keys).to contain_exactly(*%w[name username id state avatar_url web_url])
+      end
+    end
+
+    context 'when unauthenticated' do
+      it_behaves_like 'project users response' do
+        let(:project) { create(:project, :public) }
+        let(:current_user) { nil }
+      end
+    end
+
+    context 'when authenticated' do
+      context 'valid request' do
+        it_behaves_like 'project users response' do
+          let(:current_user) { user }
+        end
+      end
+
+      it 'returns a 404 error if not found' do
+        get api('/projects/42/users', user)
+
+        expect(response).to have_http_status(404)
+        expect(json_response['message']).to eq('404 Project Not Found')
+      end
+
+      it 'returns a 404 error if user is not a member' do
+        other_user = create(:user)
+
+        get api("/projects/#{project.id}/users", other_user)
+
+        expect(response).to have_http_status(404)
+      end
     end
   end
 
@@ -950,35 +1042,37 @@ describe API::API, api: true  do
     let!(:public)           { create(:empty_project, :public, name: "public #{query}") }
     let!(:unfound_public)   { create(:empty_project, :public, name: 'unfound public') }
 
+    shared_examples_for 'project search response' do |args = {}|
+      it 'returns project search responses' do
+        get api("/projects/search/#{query}", current_user)
+
+        expect(response).to have_http_status(200)
+        expect(json_response).to be_an Array
+        expect(json_response.size).to eq(args[:results])
+        json_response.each { |project| expect(project['name']).to match(args[:match_regex] || /.*query.*/) }
+      end
+    end
+
     context 'when unauthenticated' do
-      it 'returns authentication error' do
-        get api("/projects/search/#{query}")
-        expect(response).to have_http_status(401)
+      it_behaves_like 'project search response', results: 1 do
+        let(:current_user) { nil }
       end
     end
 
     context 'when authenticated' do
-      it 'returns an array of projects' do
-        get api("/projects/search/#{query}", user)
-        expect(response).to have_http_status(200)
-        expect(json_response).to be_an Array
-        expect(json_response.size).to eq(6)
-        json_response.each {|project| expect(project['name']).to match(/.*query.*/)}
+      it_behaves_like 'project search response', results: 6 do
+        let(:current_user) { user }
       end
     end
 
     context 'when authenticated as a different user' do
-      it 'returns matching public projects' do
-        get api("/projects/search/#{query}", user2)
-        expect(response).to have_http_status(200)
-        expect(json_response).to be_an Array
-        expect(json_response.size).to eq(2)
-        json_response.each {|project| expect(project['name']).to match(/(internal|public) query/)}
+      it_behaves_like 'project search response', results: 2, match_regex: /(internal|public) query/ do
+        let(:current_user) { user2 }
       end
     end
   end
 
-  describe 'PUT /projects/:id̈́' do
+  describe 'PUT /projects/:id' do
     before { project }
     before { user }
     before { user3 }