From 502d6464b07154d74eecbeddbf2cd6dba841380f Mon Sep 17 00:00:00 2001 From: Nick Thomas Date: Wed, 23 Aug 2017 13:01:11 +0100 Subject: [PATCH 1/3] Allow v4 API GET requests for groups to be unauthenticated --- lib/api/groups.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/api/groups.rb b/lib/api/groups.rb index 49c3b2278c7..892fd239df4 100644 --- a/lib/api/groups.rb +++ b/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 @@ -48,10 +48,10 @@ module API end get do groups = if params[:owned] - current_user.owned_groups - elsif current_user.admin + current_user ? current_user.owned_groups : Group.none + elsif current_user&.admin? Group.all - elsif params[:all_available] + elsif params[:all_available] || current_user.nil? GroupsFinder.new(current_user).execute else current_user.groups From 061472864ceaa4dc837eebcaa583f7b81d4e7e54 Mon Sep 17 00:00:00 2001 From: Nick Thomas Date: Tue, 22 Aug 2017 16:10:49 +0100 Subject: [PATCH 2/3] Fix group and project search for anonymous users --- app/assets/javascripts/api.js | 15 ++++++---- app/views/search/_form.html.haml | 2 +- ...and-project-search-for-anonymous-users.yml | 5 ++++ doc/api/groups.md | 9 ++++-- spec/features/search_spec.rb | 26 +++++++++++++++++ spec/javascripts/api_spec.js | 28 +++++++++++++++++-- spec/javascripts/project_title_spec.js | 6 ++-- spec/requests/api/groups_spec.rb | 21 ++++++++++++-- 8 files changed, 96 insertions(+), 16 deletions(-) create mode 100644 changelogs/unreleased/31409-fix-group-and-project-search-for-anonymous-users.yml diff --git a/app/assets/javascripts/api.js b/app/assets/javascripts/api.js index 56f91e95bb9..3d0ba65fd36 100644 --- a/app/assets/javascripts/api.js +++ b/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)); diff --git a/app/views/search/_form.html.haml b/app/views/search/_form.html.haml index 3139be1cd37..a4a5cec1314 100644 --- a/app/views/search/_form.html.haml +++ b/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" diff --git a/changelogs/unreleased/31409-fix-group-and-project-search-for-anonymous-users.yml b/changelogs/unreleased/31409-fix-group-and-project-search-for-anonymous-users.yml new file mode 100644 index 00000000000..06e8180db64 --- /dev/null +++ b/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 diff --git a/doc/api/groups.md b/doc/api/groups.md index 2b3d8e125c8..c2daa8bc029 100644 --- a/doc/api/groups.md +++ b/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 diff --git a/spec/features/search_spec.rb b/spec/features/search_spec.rb index 6742d77937f..31d509455ba 100644 --- a/spec/features/search_spec.rb +++ b/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 diff --git a/spec/javascripts/api_spec.js b/spec/javascripts/api_spec.js index 867322ce8ae..8c68ceff914 100644 --- a/spec/javascripts/api_spec.js +++ b/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); diff --git a/spec/javascripts/project_title_spec.js b/spec/javascripts/project_title_spec.js index cc336180ff7..3d36bb3e4d4 100644 --- a/spec/javascripts/project_title_spec.js +++ b/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', diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb index 313c38cd29c..a7557c7fb22 100644 --- a/spec/requests/api/groups_spec.rb +++ b/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') From 2adff699cea2cf1e60180d7eae73dfe5e8a09235 Mon Sep 17 00:00:00 2001 From: Nick Thomas Date: Thu, 24 Aug 2017 11:33:06 +0100 Subject: [PATCH 3/3] Refactor complicated API group finding rules into GroupsFinder --- app/finders/groups_finder.rb | 36 ++++++++++++++++++++++++++++++------ lib/api/groups.rb | 12 ++---------- 2 files changed, 32 insertions(+), 16 deletions(-) diff --git a/app/finders/groups_finder.rb b/app/finders/groups_finder.rb index e6fb112e7f2..88d71b0a87b 100644 --- a/app/finders/groups_finder.rb +++ b/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 diff --git a/lib/api/groups.rb b/lib/api/groups.rb index 892fd239df4..e56427304a6 100644 --- a/lib/api/groups.rb +++ b/lib/api/groups.rb @@ -47,16 +47,8 @@ module API use :pagination end get do - groups = if params[:owned] - current_user ? current_user.owned_groups : Group.none - elsif current_user&.admin? - Group.all - elsif params[:all_available] || current_user.nil? - 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])