From f3c120d25a7c9ab31609f93a0549c5174c519baf Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 28 Oct 2014 16:00:03 +0200 Subject: [PATCH 1/3] 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 42ccad71aaf..f56caeaf5ad 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 f6491508fe54c75c8db8db17b27d6d7912198a7a Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 29 Oct 2014 13:31:23 +0200 Subject: [PATCH 2/3] 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 2c7cd9038c3..d26667ba3f7 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 00000000000..24c141e9b71 --- /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 4841e04689d..f0ab6938b1c 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 00000000000..b266f56a9dd --- /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 f56caeaf5ad..8dfd2cd650e 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 eea6a8a17deb384bfe6b9b83462df6cb5f7f17ad Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 29 Oct 2014 13:38:00 +0200 Subject: [PATCH 3/3] 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 24c141e9b71..d596517c816 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 b266f56a9dd..4957186f605 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