Merge branch '31409-fix-group-and-project-search-for-anonymous-users' into 'master'

Fix group and project search for anonymous users

Closes #31409

See merge request !13745

app/assets/javascripts/api.js

@@ -55,13 +55,18 @@ const Api = {
   // Return projects list. Filtered by query
   projects(query, options, callback) {
     const url = Api.buildUrl(Api.projectsPath);
+    const defaults = {
+      search: query,
+      per_page: 20,
+    };
+
+    if (gon.current_user_id) {
+      defaults.membership = true;
+    }
+
     return $.ajax({
       url,
-      data: Object.assign({
-        search: query,
-        per_page: 20,
-        membership: true,
-      }, options),
+      data: Object.assign(defaults, options),
       dataType: 'json',
     })
       .done(projects => callback(projects));

app/finders/groups_finder.rb

@@ -1,3 +1,19 @@
+# GroupsFinder
+#
+# Used to filter Groups by a set of params
+#
+# Arguments:
+#   current_user - which user is requesting groups
+#   params:
+#     owned: boolean
+#     parent: Group
+#     all_available: boolean (defaults to true)
+#
+# Users with full private access can see all groups. The `owned` and `parent`
+# params can be used to restrict the groups that are returned.
+#
+# Anonymous users will never return any `owned` groups. They will return all
+# public groups instead, even if `all_available` is set to false.
 class GroupsFinder < UnionFinder
   def initialize(current_user = nil, params = {})
     @current_user = current_user
@@ -16,13 +32,13 @@ class GroupsFinder < UnionFinder
   attr_reader :current_user, :params
 
   def all_groups
+    return [owned_groups] if params[:owned]
+    return [Group.all] if current_user&.full_private_access?
+
     groups = []
-
-    if current_user
-      groups << Gitlab::GroupHierarchy.new(groups_for_ancestors, groups_for_descendants).all_groups
-    end
-    groups << Group.unscoped.public_to_user(current_user)
-
+    groups << Gitlab::GroupHierarchy.new(groups_for_ancestors, groups_for_descendants).all_groups if current_user
+    groups << Group.unscoped.public_to_user(current_user) if include_public_groups?
+    groups << Group.none if groups.empty?
     groups
   end
 
@@ -39,4 +55,12 @@ class GroupsFinder < UnionFinder
 
     groups.where(parent: params[:parent])
   end
+
+  def owned_groups
+    current_user&.groups || Group.none
+  end
+
+  def include_public_groups?
+    current_user.nil? || params.fetch(:all_available, true)
+  end
 end

app/views/search/_form.html.haml

@@ -11,5 +11,5 @@
         %span.sr-only
           Clear search
     - unless params[:snippets].eql? 'true'
-      = render 'filter' if current_user
+      = render 'filter'
     = button_tag "Search", class: "btn btn-success btn-search"

changelogs/unreleased/31409-fix-group-and-project-search-for-anonymous-users.yml

@@ -0,0 +1,5 @@
+---
+title: Fix group and project search for anonymous users
+merge_request: 13745
+author:
+type: fixed

doc/api/groups.md

@@ -2,7 +2,8 @@
 
 ## List groups
 
-Get a list of groups. (As user: my groups or all available, as admin: all groups).
+Get a list of visible groups for the authenticated user. When accessed without
+authentication, only public groups are returned.
 
 Parameters:
 
@@ -43,7 +44,8 @@ You can search for groups by name or path, see below.
 
 ## List a group's projects
 
-Get a list of projects in this group.
+Get a list of projects in this group. When accessed without authentication, only
+public projects are returned.
 
 ```
 GET /groups/:id/projects
@@ -109,7 +111,8 @@ Example response:
 
 ## Details of a group
 
-Get all details of a group.
+Get all details of a group. This endpoint can be accessed without authentication
+if the group is publicly accessible.
 
 ```
 GET /groups/:id

lib/api/groups.rb

@@ -2,7 +2,7 @@ module API
   class Groups < Grape::API
     include PaginationParams
 
-    before { authenticate! }
+    before { authenticate_non_get! }
 
     helpers do
       params :optional_params_ce do
@@ -47,16 +47,8 @@ module API
         use :pagination
       end
       get do
-        groups = if params[:owned]
-                   current_user.owned_groups
-                 elsif current_user.admin
-                   Group.all
-                 elsif params[:all_available]
-                   GroupsFinder.new(current_user).execute
-                 else
-                   current_user.groups
-                 end
-
+        find_params = { all_available: params[:all_available], owned: params[:owned] }
+        groups = GroupsFinder.new(current_user, find_params).execute
         groups = groups.search(params[:search]) if params[:search].present?
         groups = groups.where.not(id: params[:skip_groups]) if params[:skip_groups].present?
         groups = groups.reorder(params[:order_by] => params[:sort])

spec/features/search_spec.rb

@@ -281,4 +281,30 @@ describe "Search"  do
       expect(page).to have_selector('.commit-row-description', count: 9)
     end
   end
+
+  context 'anonymous user' do
+    let(:project) { create(:project, :public) }
+
+    before do
+      sign_out(user)
+    end
+
+    it 'preserves the group being searched in' do
+      visit search_path(group_id: project.namespace.id)
+
+      fill_in 'search', with: 'foo'
+      click_button 'Search'
+
+      expect(find('#group_id').value).to eq(project.namespace.id.to_s)
+    end
+
+    it 'preserves the project being searched in' do
+      visit search_path(project_id: project.id)
+
+      fill_in 'search', with: 'foo'
+      click_button 'Search'
+
+      expect(find('#project_id').value).to eq(project.id.to_s)
+    end
+  end
 end

spec/javascripts/api_spec.js

@@ -17,7 +17,7 @@ describe('Api', () => {
 
   beforeEach(() => {
     originalGon = window.gon;
-    window.gon = dummyGon;
+    window.gon = Object.assign({}, dummyGon);
   });
 
   afterEach(() => {
@@ -98,14 +98,36 @@ describe('Api', () => {
   });
 
   describe('projects', () => {
-    it('fetches projects', (done) => {
+    it('fetches projects with membership when logged in', (done) => {
+      const query = 'dummy query';
+      const options = { unused: 'option' };
+      const expectedUrl = `${dummyUrlRoot}/api/${dummyApiVersion}/projects.json?simple=true`;
+      window.gon.current_user_id = 1;
+      const expectedData = Object.assign({
+        search: query,
+        per_page: 20,
+        membership: true,
+      }, options);
+      spyOn(jQuery, 'ajax').and.callFake((request) => {
+        expect(request.url).toEqual(expectedUrl);
+        expect(request.dataType).toEqual('json');
+        expect(request.data).toEqual(expectedData);
+        return sendDummyResponse();
+      });
+
+      Api.projects(query, options, (response) => {
+        expect(response).toBe(dummyResponse);
+        done();
+      });
+    });
+
+    it('fetches projects without membership when not logged in', (done) => {
       const query = 'dummy query';
       const options = { unused: 'option' };
       const expectedUrl = `${dummyUrlRoot}/api/${dummyApiVersion}/projects.json?simple=true`;
       const expectedData = Object.assign({
         search: query,
         per_page: 20,
-        membership: true,
       }, options);
       spyOn(jQuery, 'ajax').and.callFake((request) => {
         expect(request.url).toEqual(expectedUrl);

spec/javascripts/project_title_spec.js

@@ -7,6 +7,7 @@ import '~/project_select';
 import '~/project';
 
 describe('Project Title', () => {
+  const dummyApiVersion = 'v3000';
   preloadFixtures('issues/open-issue.html.raw');
   loadJSONFixtures('projects.json');
 
@@ -14,7 +15,7 @@ describe('Project Title', () => {
     loadFixtures('issues/open-issue.html.raw');
 
     window.gon = {};
-    window.gon.api_version = 'v3';
+    window.gon.api_version = dummyApiVersion;
 
     // eslint-disable-next-line no-new
     new Project();
@@ -37,9 +38,10 @@ describe('Project Title', () => {
 
     it('toggles dropdown', () => {
       const $menu = $('.js-dropdown-menu-projects');
+      window.gon.current_user_id = 1;
       $('.js-projects-dropdown-toggle').click();
       expect($menu).toHaveClass('open');
-      expect(reqUrl).toBe('/api/v3/projects.json?simple=true');
+      expect(reqUrl).toBe(`/api/${dummyApiVersion}/projects.json?simple=true`);
       expect(reqData).toEqual({
         search: '',
         order_by: 'last_activity_at',

spec/requests/api/groups_spec.rb

@@ -20,10 +20,15 @@ describe API::Groups do
 
   describe "GET /groups" do
     context "when unauthenticated" do
-      it "returns authentication error" do
+      it "returns public groups" do
         get api("/groups")
 
-        expect(response).to have_http_status(401)
+        expect(response).to have_http_status(200)
+        expect(response).to include_pagination_headers
+        expect(json_response).to be_an Array
+        expect(json_response.length).to eq(1)
+        expect(json_response)
+          .to satisfy_one { |group| group['name'] == group1.name }
       end
     end
 
@@ -165,6 +170,18 @@ describe API::Groups do
   end
 
   describe "GET /groups/:id" do
+    context 'when unauthenticated' do
+      it 'returns 404 for a private group' do
+        get api("/groups/#{group2.id}")
+        expect(response).to have_http_status(404)
+      end
+
+      it 'returns 200 for a public group' do
+        get api("/groups/#{group1.id}")
+        expect(response).to have_http_status(200)
+      end
+    end
+
     context "when authenticated as user" do
       it "returns one of user1's groups" do
         project = create(:project, namespace: group2, path: 'Foo')