From 7f97a1277de78bcd86d68978e9ec29a2548fc144 Mon Sep 17 00:00:00 2001 From: Valery Sizov Date: Fri, 24 Oct 2014 19:24:49 +0300 Subject: [PATCH 1/8] internal snippets: fix exposing of title --- CHANGELOG | 5 +++++ VERSION | 2 +- app/finders/snippets_finder.rb | 2 ++ spec/finders/snippets_finder_spec.rb | 7 +++++++ 4 files changed, 15 insertions(+), 1 deletion(-) diff --git a/CHANGELOG b/CHANGELOG index 5a494cccc6..4428bae4eb 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,6 +1,11 @@ +v 7.4.2 + - Fix internal snippet exposing for unauthenticated users + v 7.4.1 - Fix LDAP authentication for Git HTTP access - Fix LDAP config lookup for provider 'ldap' + - Fix public snippets + - Fix 500 error on projects with nested submodules v 7.4.0 - Refactored membership logic diff --git a/VERSION b/VERSION index 815da58b7a..f8cb1fa110 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -7.4.1 +7.4.2 diff --git a/app/finders/snippets_finder.rb b/app/finders/snippets_finder.rb index b29ab6cf40..4b0c69f2d2 100644 --- a/app/finders/snippets_finder.rb +++ b/app/finders/snippets_finder.rb @@ -29,6 +29,8 @@ class SnippetsFinder def by_user(current_user, user, scope) snippets = user.snippets.fresh.non_expired + return snippets.are_public unless current_user + if user == current_user case scope when 'are_internal' then diff --git a/spec/finders/snippets_finder_spec.rb b/spec/finders/snippets_finder_spec.rb index 5af7696818..c645cbc964 100644 --- a/spec/finders/snippets_finder_spec.rb +++ b/spec/finders/snippets_finder_spec.rb @@ -64,6 +64,13 @@ describe SnippetsFinder do snippets = SnippetsFinder.new.execute(user, filter: :by_user, user: user) snippets.should include(@snippet1, @snippet2, @snippet3) end + + it "returns only public snippets if unauthenticated user" do + snippets = SnippetsFinder.new.execute(nil, filter: :by_user, user: user) + snippets.should include(@snippet3) + snippets.should_not include(@snippet2, @snippet1) + end + end context 'by_project filter' do From 912715c3e452723dd7dda3430f57e69b7f1605af Mon Sep 17 00:00:00 2001 From: Valery Sizov Date: Wed, 29 Oct 2014 15:59:57 +0200 Subject: [PATCH 2/8] update changelog && bump version --- CHANGELOG | 4 ++++ VERSION | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG b/CHANGELOG index 4428bae4eb..9884b47581 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,3 +1,7 @@ +v 7.4.3 + - Fix raw snippets view + - Fix security issue for member api + v 7.4.2 - Fix internal snippet exposing for unauthenticated users diff --git a/VERSION b/VERSION index f8cb1fa110..0f4a1d6e34 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -7.4.2 +7.4.3 From 43fba7f0c518942d060e7a7dd9f7ec3065ac1f69 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 28 Oct 2014 16:00:03 +0200 Subject: [PATCH 3/8] Add failing test that should be green after group members api get fixed Signed-off-by: Dmitriy Zaporozhets --- spec/requests/api/groups_spec.rb | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb index 42ccad71aa..f56caeaf5a 100644 --- a/spec/requests/api/groups_spec.rb +++ b/spec/requests/api/groups_spec.rb @@ -220,13 +220,27 @@ describe API::API, api: true do context "when a member of the group" do it "should return ok and add new member" do - count_before=group_no_members.group_members.count new_user = create(:user) - post api("/groups/#{group_no_members.id}/members", owner), user_id: new_user.id, access_level: GroupMember::MASTER + + expect { + post api("/groups/#{group_no_members.id}/members", owner), + user_id: new_user.id, access_level: GroupMember::MASTER + }.to change { group_no_members.members.count }.by(1) + response.status.should == 201 json_response['name'].should == new_user.name json_response['access_level'].should == GroupMember::MASTER - group_no_members.group_members.count.should == count_before + 1 + end + + it "should not allow guest to modify group members" do + new_user = create(:user) + + expect { + post api("/groups/#{group_with_members.id}/members", guest), + user_id: new_user.id, access_level: GroupMember::MASTER + }.not_to change { group_with_members.members.count } + + response.status.should == 403 end it "should return error if member already exists" do From 995d198419b6f196b014ccb47d0298b419b53e6d Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 29 Oct 2014 13:31:23 +0200 Subject: [PATCH 4/8] Split group members api Signed-off-by: Dmitriy Zaporozhets --- lib/api/api.rb | 1 + lib/api/group_members.rb | 74 ++++++++++++++ lib/api/groups.rb | 51 ---------- spec/requests/api/group_members_spec.rb | 130 ++++++++++++++++++++++++ spec/requests/api/groups_spec.rb | 124 ---------------------- 5 files changed, 205 insertions(+), 175 deletions(-) create mode 100644 lib/api/group_members.rb create mode 100644 spec/requests/api/group_members_spec.rb diff --git a/lib/api/api.rb b/lib/api/api.rb index 2c7cd9038c..d26667ba3f 100644 --- a/lib/api/api.rb +++ b/lib/api/api.rb @@ -27,6 +27,7 @@ module API helpers APIHelpers mount Groups + mount GroupMembers mount Users mount Projects mount Repositories diff --git a/lib/api/group_members.rb b/lib/api/group_members.rb new file mode 100644 index 0000000000..24c141e9b7 --- /dev/null +++ b/lib/api/group_members.rb @@ -0,0 +1,74 @@ +module API + class GroupMembers < Grape::API + before { authenticate! } + + resource :groups do + helpers do + def find_group(id) + group = Group.find(id) + + if can?(current_user, :read_group, group) + group + else + render_api_error!("403 Forbidden - #{current_user.username} lacks sufficient access to #{group.name}", 403) + end + end + + def validate_access_level?(level) + Gitlab::Access.options_with_owner.values.include? level.to_i + end + end + + # Get a list of group members viewable by the authenticated user. + # + # Example Request: + # GET /groups/:id/members + get ":id/members" do + group = find_group(params[:id]) + members = group.group_members + users = (paginate members).collect(&:user) + present users, with: Entities::GroupMember, group: group + end + + # Add a user to the list of group members + # + # Parameters: + # id (required) - group id + # user_id (required) - the users id + # access_level (required) - Project access level + # Example Request: + # POST /groups/:id/members + post ":id/members" do + required_attributes! [:user_id, :access_level] + unless validate_access_level?(params[:access_level]) + render_api_error!("Wrong access level", 422) + end + group = find_group(params[:id]) + if group.group_members.find_by(user_id: params[:user_id]) + render_api_error!("Already exists", 409) + end + group.add_users([params[:user_id]], params[:access_level]) + member = group.group_members.find_by(user_id: params[:user_id]) + present member.user, with: Entities::GroupMember, group: group + end + + # Remove member. + # + # Parameters: + # id (required) - group id + # user_id (required) - the users id + # + # Example Request: + # DELETE /groups/:id/members/:user_id + delete ":id/members/:user_id" do + group = find_group(params[:id]) + member = group.group_members.find_by(user_id: params[:user_id]) + if member.nil? + render_api_error!("404 Not Found - user_id:#{params[:user_id]} not a member of group #{group.name}",404) + else + member.destroy + end + end + end + end +end diff --git a/lib/api/groups.rb b/lib/api/groups.rb index 4841e04689..f0ab6938b1 100644 --- a/lib/api/groups.rb +++ b/lib/api/groups.rb @@ -97,57 +97,6 @@ module API not_found! end end - - # Get a list of group members viewable by the authenticated user. - # - # Example Request: - # GET /groups/:id/members - get ":id/members" do - group = find_group(params[:id]) - members = group.group_members - users = (paginate members).collect(&:user) - present users, with: Entities::GroupMember, group: group - end - - # Add a user to the list of group members - # - # Parameters: - # id (required) - group id - # user_id (required) - the users id - # access_level (required) - Project access level - # Example Request: - # POST /groups/:id/members - post ":id/members" do - required_attributes! [:user_id, :access_level] - unless validate_access_level?(params[:access_level]) - render_api_error!("Wrong access level", 422) - end - group = find_group(params[:id]) - if group.group_members.find_by(user_id: params[:user_id]) - render_api_error!("Already exists", 409) - end - group.add_users([params[:user_id]], params[:access_level]) - member = group.group_members.find_by(user_id: params[:user_id]) - present member.user, with: Entities::GroupMember, group: group - end - - # Remove member. - # - # Parameters: - # id (required) - group id - # user_id (required) - the users id - # - # Example Request: - # DELETE /groups/:id/members/:user_id - delete ":id/members/:user_id" do - group = find_group(params[:id]) - member = group.group_members.find_by(user_id: params[:user_id]) - if member.nil? - render_api_error!("404 Not Found - user_id:#{params[:user_id]} not a member of group #{group.name}",404) - else - member.destroy - end - end end end end diff --git a/spec/requests/api/group_members_spec.rb b/spec/requests/api/group_members_spec.rb new file mode 100644 index 0000000000..b266f56a9d --- /dev/null +++ b/spec/requests/api/group_members_spec.rb @@ -0,0 +1,130 @@ +require 'spec_helper' + +describe API::API, api: true do + include ApiHelpers + + let(:owner) { create(:user) } + let(:reporter) { create(:user) } + let(:developer) { create(:user) } + let(:master) { create(:user) } + let(:guest) { create(:user) } + let(:stranger) { create(:user) } + + let!(:group_with_members) do + group = create(:group) + group.add_users([reporter.id], GroupMember::REPORTER) + group.add_users([developer.id], GroupMember::DEVELOPER) + group.add_users([master.id], GroupMember::MASTER) + group.add_users([guest.id], GroupMember::GUEST) + group + end + + let!(:group_no_members) { create(:group) } + + before do + group_with_members.add_owner owner + group_no_members.add_owner owner + end + + describe "GET /groups/:id/members" do + context "when authenticated as user that is part or the group" do + it "each user: should return an array of members groups of group3" do + [owner, master, developer, reporter, guest].each do |user| + get api("/groups/#{group_with_members.id}/members", user) + response.status.should == 200 + json_response.should be_an Array + json_response.size.should == 5 + json_response.find { |e| e['id']==owner.id }['access_level'].should == GroupMember::OWNER + json_response.find { |e| e['id']==reporter.id }['access_level'].should == GroupMember::REPORTER + json_response.find { |e| e['id']==developer.id }['access_level'].should == GroupMember::DEVELOPER + json_response.find { |e| e['id']==master.id }['access_level'].should == GroupMember::MASTER + json_response.find { |e| e['id']==guest.id }['access_level'].should == GroupMember::GUEST + end + end + + it "users not part of the group should get access error" do + get api("/groups/#{group_with_members.id}/members", stranger) + response.status.should == 403 + end + end + end + + describe "POST /groups/:id/members" do + context "when not a member of the group" do + it "should not add guest as member of group_no_members when adding being done by person outside the group" do + post api("/groups/#{group_no_members.id}/members", reporter), user_id: guest.id, access_level: GroupMember::MASTER + response.status.should == 403 + end + end + + context "when a member of the group" do + it "should return ok and add new member" do + new_user = create(:user) + + expect { + post api("/groups/#{group_no_members.id}/members", owner), + user_id: new_user.id, access_level: GroupMember::MASTER + }.to change { group_no_members.members.count }.by(1) + + response.status.should == 201 + json_response['name'].should == new_user.name + json_response['access_level'].should == GroupMember::MASTER + end + + it "should not allow guest to modify group members" do + new_user = create(:user) + + expect { + post api("/groups/#{group_with_members.id}/members", guest), + user_id: new_user.id, access_level: GroupMember::MASTER + }.not_to change { group_with_members.members.count } + + response.status.should == 403 + end + + it "should return error if member already exists" do + post api("/groups/#{group_with_members.id}/members", owner), user_id: master.id, access_level: GroupMember::MASTER + response.status.should == 409 + end + + it "should return a 400 error when user id is not given" do + post api("/groups/#{group_no_members.id}/members", owner), access_level: GroupMember::MASTER + response.status.should == 400 + end + + it "should return a 400 error when access level is not given" do + post api("/groups/#{group_no_members.id}/members", owner), user_id: master.id + response.status.should == 400 + end + + it "should return a 422 error when access level is not known" do + post api("/groups/#{group_no_members.id}/members", owner), user_id: master.id, access_level: 1234 + response.status.should == 422 + end + end + end + + describe "DELETE /groups/:id/members/:user_id" do + context "when not a member of the group" do + it "should not delete guest's membership of group_with_members" do + random_user = create(:user) + delete api("/groups/#{group_with_members.id}/members/#{owner.id}", random_user) + response.status.should == 403 + end + end + + context "when a member of the group" do + it "should delete guest's membership of group" do + count_before=group_with_members.group_members.count + delete api("/groups/#{group_with_members.id}/members/#{guest.id}", owner) + response.status.should == 200 + group_with_members.group_members.count.should == count_before - 1 + end + + it "should return a 404 error when user id is not known" do + delete api("/groups/#{group_with_members.id}/members/1328", owner) + response.status.should == 404 + end + end + end +end diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb index f56caeaf5a..8dfd2cd650 100644 --- a/spec/requests/api/groups_spec.rb +++ b/spec/requests/api/groups_spec.rb @@ -165,128 +165,4 @@ describe API::API, api: true do end end end - - describe "members" do - let(:owner) { create(:user) } - let(:reporter) { create(:user) } - let(:developer) { create(:user) } - let(:master) { create(:user) } - let(:guest) { create(:user) } - let!(:group_with_members) do - group = create(:group) - group.add_users([reporter.id], GroupMember::REPORTER) - group.add_users([developer.id], GroupMember::DEVELOPER) - group.add_users([master.id], GroupMember::MASTER) - group.add_users([guest.id], GroupMember::GUEST) - group - end - let!(:group_no_members) { create(:group) } - - before do - group_with_members.add_owner owner - group_no_members.add_owner owner - end - - describe "GET /groups/:id/members" do - context "when authenticated as user that is part or the group" do - it "each user: should return an array of members groups of group3" do - [owner, master, developer, reporter, guest].each do |user| - get api("/groups/#{group_with_members.id}/members", user) - response.status.should == 200 - json_response.should be_an Array - json_response.size.should == 5 - json_response.find { |e| e['id']==owner.id }['access_level'].should == GroupMember::OWNER - json_response.find { |e| e['id']==reporter.id }['access_level'].should == GroupMember::REPORTER - json_response.find { |e| e['id']==developer.id }['access_level'].should == GroupMember::DEVELOPER - json_response.find { |e| e['id']==master.id }['access_level'].should == GroupMember::MASTER - json_response.find { |e| e['id']==guest.id }['access_level'].should == GroupMember::GUEST - end - end - - it "users not part of the group should get access error" do - get api("/groups/#{group_with_members.id}/members", user1) - response.status.should == 403 - end - end - end - - describe "POST /groups/:id/members" do - context "when not a member of the group" do - it "should not add guest as member of group_no_members when adding being done by person outside the group" do - post api("/groups/#{group_no_members.id}/members", reporter), user_id: guest.id, access_level: GroupMember::MASTER - response.status.should == 403 - end - end - - context "when a member of the group" do - it "should return ok and add new member" do - new_user = create(:user) - - expect { - post api("/groups/#{group_no_members.id}/members", owner), - user_id: new_user.id, access_level: GroupMember::MASTER - }.to change { group_no_members.members.count }.by(1) - - response.status.should == 201 - json_response['name'].should == new_user.name - json_response['access_level'].should == GroupMember::MASTER - end - - it "should not allow guest to modify group members" do - new_user = create(:user) - - expect { - post api("/groups/#{group_with_members.id}/members", guest), - user_id: new_user.id, access_level: GroupMember::MASTER - }.not_to change { group_with_members.members.count } - - response.status.should == 403 - end - - it "should return error if member already exists" do - post api("/groups/#{group_with_members.id}/members", owner), user_id: master.id, access_level: GroupMember::MASTER - response.status.should == 409 - end - - it "should return a 400 error when user id is not given" do - post api("/groups/#{group_no_members.id}/members", owner), access_level: GroupMember::MASTER - response.status.should == 400 - end - - it "should return a 400 error when access level is not given" do - post api("/groups/#{group_no_members.id}/members", owner), user_id: master.id - response.status.should == 400 - end - - it "should return a 422 error when access level is not known" do - post api("/groups/#{group_no_members.id}/members", owner), user_id: master.id, access_level: 1234 - response.status.should == 422 - end - end - end - - describe "DELETE /groups/:id/members/:user_id" do - context "when not a member of the group" do - it "should not delete guest's membership of group_with_members" do - random_user = create(:user) - delete api("/groups/#{group_with_members.id}/members/#{owner.id}", random_user) - response.status.should == 403 - end - end - - context "when a member of the group" do - it "should delete guest's membership of group" do - count_before=group_with_members.group_members.count - delete api("/groups/#{group_with_members.id}/members/#{guest.id}", owner) - response.status.should == 200 - group_with_members.group_members.count.should == count_before - 1 - end - - it "should return a 404 error when user id is not known" do - delete api("/groups/#{group_with_members.id}/members/1328", owner) - response.status.should == 404 - end - end - end - end end From a2dfff418bf2532ebb5aee88414107929b17eefd Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 29 Oct 2014 13:38:00 +0200 Subject: [PATCH 5/8] Dont allow guests..developers to manage group members Signed-off-by: Dmitriy Zaporozhets --- lib/api/group_members.rb | 10 ++++++++-- spec/requests/api/group_members_spec.rb | 12 +++++++++--- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/lib/api/group_members.rb b/lib/api/group_members.rb index 24c141e9b7..d596517c81 100644 --- a/lib/api/group_members.rb +++ b/lib/api/group_members.rb @@ -39,14 +39,18 @@ module API # Example Request: # POST /groups/:id/members post ":id/members" do + group = find_group(params[:id]) + authorize! :manage_group, group required_attributes! [:user_id, :access_level] + unless validate_access_level?(params[:access_level]) render_api_error!("Wrong access level", 422) end - group = find_group(params[:id]) + if group.group_members.find_by(user_id: params[:user_id]) render_api_error!("Already exists", 409) end + group.add_users([params[:user_id]], params[:access_level]) member = group.group_members.find_by(user_id: params[:user_id]) present member.user, with: Entities::GroupMember, group: group @@ -62,7 +66,9 @@ module API # DELETE /groups/:id/members/:user_id delete ":id/members/:user_id" do group = find_group(params[:id]) - member = group.group_members.find_by(user_id: params[:user_id]) + authorize! :manage_group, group + member = group.group_members.find_by(user_id: params[:user_id]) + if member.nil? render_api_error!("404 Not Found - user_id:#{params[:user_id]} not a member of group #{group.name}",404) else diff --git a/spec/requests/api/group_members_spec.rb b/spec/requests/api/group_members_spec.rb index b266f56a9d..4957186f60 100644 --- a/spec/requests/api/group_members_spec.rb +++ b/spec/requests/api/group_members_spec.rb @@ -115,16 +115,22 @@ describe API::API, api: true do context "when a member of the group" do it "should delete guest's membership of group" do - count_before=group_with_members.group_members.count - delete api("/groups/#{group_with_members.id}/members/#{guest.id}", owner) + expect { + delete api("/groups/#{group_with_members.id}/members/#{guest.id}", owner) + }.to change { group_with_members.members.count }.by(-1) + response.status.should == 200 - group_with_members.group_members.count.should == count_before - 1 end it "should return a 404 error when user id is not known" do delete api("/groups/#{group_with_members.id}/members/1328", owner) response.status.should == 404 end + + it "should not allow guest to modify group members" do + delete api("/groups/#{group_with_members.id}/members/#{master.id}", guest) + response.status.should == 403 + end end end end From 1ab1526d92d6084acfd459e82c9ce490e9e29807 Mon Sep 17 00:00:00 2001 From: Valery Sizov Date: Mon, 27 Oct 2014 11:51:31 +0200 Subject: [PATCH 6/8] Fix raw view for public snippets --- app/controllers/snippets_controller.rb | 2 +- features/snippets/public_snippets.feature | 5 +++++ features/steps/snippets/public_snippets.rb | 8 ++++++++ 3 files changed, 14 insertions(+), 1 deletion(-) diff --git a/app/controllers/snippets_controller.rb b/app/controllers/snippets_controller.rb index 987694260c..bf3312fedc 100644 --- a/app/controllers/snippets_controller.rb +++ b/app/controllers/snippets_controller.rb @@ -9,7 +9,7 @@ class SnippetsController < ApplicationController before_filter :set_title - skip_before_filter :authenticate_user!, only: [:index, :user_index, :show] + skip_before_filter :authenticate_user!, only: [:index, :user_index, :show, :raw] respond_to :html diff --git a/features/snippets/public_snippets.feature b/features/snippets/public_snippets.feature index 6964badc41..c2afb63b6d 100644 --- a/features/snippets/public_snippets.feature +++ b/features/snippets/public_snippets.feature @@ -3,3 +3,8 @@ Feature: Public snippets Given There is public "Personal snippet one" snippet And I visit snippet page "Personal snippet one" Then I should see snippet "Personal snippet one" + + Scenario: Unauthenticated user should see raw public snippets + Given There is public "Personal snippet one" snippet + And I visit snippet raw page "Personal snippet one" + Then I should see raw snippet "Personal snippet one" diff --git a/features/steps/snippets/public_snippets.rb b/features/steps/snippets/public_snippets.rb index 956aa4a3e7..67669dc0a6 100644 --- a/features/steps/snippets/public_snippets.rb +++ b/features/steps/snippets/public_snippets.rb @@ -7,10 +7,18 @@ class Spinach::Features::PublicSnippets < Spinach::FeatureSteps page.should have_no_xpath("//i[@class='public-snippet']") end + step 'I should see raw snippet "Personal snippet one"' do + page.should have_text(snippet.content) + end + step 'I visit snippet page "Personal snippet one"' do visit snippet_path(snippet) end + step 'I visit snippet raw page "Personal snippet one"' do + visit raw_snippet_path(snippet) + end + def snippet @snippet ||= PersonalSnippet.find_by!(title: "Personal snippet one") end From 482cdcf492ba2ae883b08f4199f6ed46927b5e6f Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 29 Oct 2014 16:34:28 +0200 Subject: [PATCH 7/8] Explicitly require addressable gem feature you need. Fixes Buildbox CI integration Signed-off-by: Dmitriy Zaporozhets --- app/models/project_services/buildbox_service.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/models/project_services/buildbox_service.rb b/app/models/project_services/buildbox_service.rb index b0f8e28c97..0ab67b79fe 100644 --- a/app/models/project_services/buildbox_service.rb +++ b/app/models/project_services/buildbox_service.rb @@ -12,6 +12,8 @@ # properties :text # +require "addressable/uri" + class BuildboxService < CiService prop_accessor :project_url, :token From a00cfa191a3c21097937bead94d2d1dec1ec55c1 Mon Sep 17 00:00:00 2001 From: Valery Sizov Date: Wed, 29 Oct 2014 16:51:57 +0200 Subject: [PATCH 8/8] update changelog --- CHANGELOG | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG b/CHANGELOG index 9cebbd7889..aa9b820de9 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,6 +1,7 @@ v 7.4.3 - Fix raw snippets view - Fix security issue for member api + - Fix buildbox integration v 7.4.2 - Fix internal snippet exposing for unauthenticated users