Merge branch '20732_member_exists_409' into 'master'
Add member: Always return 409 when a member exists Closes #20732 See merge request !9093
This commit is contained in:
commit
d8345a5d43
|
@ -0,0 +1,4 @@
|
|||
---
|
||||
title: 'Add member: Always return 409 when a member exists'
|
||||
merge_request:
|
||||
author:
|
|
@ -12,3 +12,4 @@ changes are in V4:
|
|||
- Endpoints under `projects/merge_request/:id` have been removed (use: `projects/merge_requests/:id`)
|
||||
- Project snippets do not return deprecated field `expires_at`
|
||||
- Endpoints under `projects/:id/keys` have been removed (use `projects/:id/deploy_keys`)
|
||||
- Status 409 returned for POST `project/:id/members` when a member already exists
|
||||
|
|
|
@ -7,6 +7,7 @@ module API
|
|||
version 'v3', using: :path do
|
||||
mount ::API::V3::DeployKeys
|
||||
mount ::API::V3::Issues
|
||||
mount ::API::V3::Members
|
||||
mount ::API::V3::MergeRequests
|
||||
mount ::API::V3::Projects
|
||||
mount ::API::V3::ProjectSnippets
|
||||
|
|
|
@ -56,16 +56,9 @@ module API
|
|||
|
||||
member = source.members.find_by(user_id: params[:user_id])
|
||||
|
||||
# We need this explicit check because `source.add_user` doesn't
|
||||
# currently return the member created so it would return 201 even if
|
||||
# the member already existed...
|
||||
# The `source_type == 'group'` check is to ensure back-compatibility
|
||||
# but 409 behavior should be used for both project and group members in 9.0!
|
||||
conflict!('Member already exists') if source_type == 'group' && member
|
||||
conflict!('Member already exists') if member
|
||||
|
||||
unless member
|
||||
member = source.add_user(params[:user_id], params[:access_level], current_user: current_user, expires_at: params[:expires_at])
|
||||
end
|
||||
member = source.add_user(params[:user_id], params[:access_level], current_user: current_user, expires_at: params[:expires_at])
|
||||
|
||||
if member.persisted? && member.valid?
|
||||
present member.user, with: Entities::Member, member: member
|
||||
|
|
|
@ -0,0 +1,134 @@
|
|||
module API
|
||||
module V3
|
||||
class Members < Grape::API
|
||||
include PaginationParams
|
||||
|
||||
before { authenticate! }
|
||||
|
||||
helpers ::API::Helpers::MembersHelpers
|
||||
|
||||
%w[group project].each do |source_type|
|
||||
params do
|
||||
requires :id, type: String, desc: "The #{source_type} ID"
|
||||
end
|
||||
resource source_type.pluralize do
|
||||
desc 'Gets a list of group or project members viewable by the authenticated user.' do
|
||||
success ::API::Entities::Member
|
||||
end
|
||||
params do
|
||||
optional :query, type: String, desc: 'A query string to search for members'
|
||||
use :pagination
|
||||
end
|
||||
get ":id/members" do
|
||||
source = find_source(source_type, params[:id])
|
||||
|
||||
users = source.users
|
||||
users = users.merge(User.search(params[:query])) if params[:query]
|
||||
|
||||
present paginate(users), with: ::API::Entities::Member, source: source
|
||||
end
|
||||
|
||||
desc 'Gets a member of a group or project.' do
|
||||
success ::API::Entities::Member
|
||||
end
|
||||
params do
|
||||
requires :user_id, type: Integer, desc: 'The user ID of the member'
|
||||
end
|
||||
get ":id/members/:user_id" do
|
||||
source = find_source(source_type, params[:id])
|
||||
|
||||
members = source.members
|
||||
member = members.find_by!(user_id: params[:user_id])
|
||||
|
||||
present member.user, with: ::API::Entities::Member, member: member
|
||||
end
|
||||
|
||||
desc 'Adds a member to a group or project.' do
|
||||
success ::API::Entities::Member
|
||||
end
|
||||
params do
|
||||
requires :user_id, type: Integer, desc: 'The user ID of the new member'
|
||||
requires :access_level, type: Integer, desc: 'A valid access level (defaults: `30`, developer access level)'
|
||||
optional :expires_at, type: DateTime, desc: 'Date string in the format YEAR-MONTH-DAY'
|
||||
end
|
||||
post ":id/members" do
|
||||
source = find_source(source_type, params[:id])
|
||||
authorize_admin_source!(source_type, source)
|
||||
|
||||
member = source.members.find_by(user_id: params[:user_id])
|
||||
|
||||
# We need this explicit check because `source.add_user` doesn't
|
||||
# currently return the member created so it would return 201 even if
|
||||
# the member already existed...
|
||||
# The `source_type == 'group'` check is to ensure back-compatibility
|
||||
# but 409 behavior should be used for both project and group members in 9.0!
|
||||
conflict!('Member already exists') if source_type == 'group' && member
|
||||
|
||||
unless member
|
||||
member = source.add_user(params[:user_id], params[:access_level], current_user: current_user, expires_at: params[:expires_at])
|
||||
end
|
||||
if member.persisted? && member.valid?
|
||||
present member.user, with: ::API::Entities::Member, member: member
|
||||
else
|
||||
# This is to ensure back-compatibility but 400 behavior should be used
|
||||
# for all validation errors in 9.0!
|
||||
render_api_error!('Access level is not known', 422) if member.errors.key?(:access_level)
|
||||
render_validation_error!(member)
|
||||
end
|
||||
end
|
||||
|
||||
desc 'Updates a member of a group or project.' do
|
||||
success ::API::Entities::Member
|
||||
end
|
||||
params do
|
||||
requires :user_id, type: Integer, desc: 'The user ID of the new member'
|
||||
requires :access_level, type: Integer, desc: 'A valid access level'
|
||||
optional :expires_at, type: DateTime, desc: 'Date string in the format YEAR-MONTH-DAY'
|
||||
end
|
||||
put ":id/members/:user_id" do
|
||||
source = find_source(source_type, params[:id])
|
||||
authorize_admin_source!(source_type, source)
|
||||
|
||||
member = source.members.find_by!(user_id: params[:user_id])
|
||||
attrs = attributes_for_keys [:access_level, :expires_at]
|
||||
|
||||
if member.update_attributes(attrs)
|
||||
present member.user, with: ::API::Entities::Member, member: member
|
||||
else
|
||||
# This is to ensure back-compatibility but 400 behavior should be used
|
||||
# for all validation errors in 9.0!
|
||||
render_api_error!('Access level is not known', 422) if member.errors.key?(:access_level)
|
||||
render_validation_error!(member)
|
||||
end
|
||||
end
|
||||
|
||||
desc 'Removes a user from a group or project.'
|
||||
params do
|
||||
requires :user_id, type: Integer, desc: 'The user ID of the member'
|
||||
end
|
||||
delete ":id/members/:user_id" do
|
||||
source = find_source(source_type, params[:id])
|
||||
|
||||
# This is to ensure back-compatibility but find_by! should be used
|
||||
# in that casse in 9.0!
|
||||
member = source.members.find_by(user_id: params[:user_id])
|
||||
|
||||
# This is to ensure back-compatibility but this should be removed in
|
||||
# favor of find_by! in 9.0!
|
||||
not_found!("Member: user_id:#{params[:user_id]}") if source_type == 'group' && member.nil?
|
||||
|
||||
# This is to ensure back-compatibility but 204 behavior should be used
|
||||
# for all DELETE endpoints in 9.0!
|
||||
if member.nil?
|
||||
{ message: "Access revoked", id: params[:user_id].to_i }
|
||||
else
|
||||
::Members::DestroyService.new(source, current_user, declared_params).execute
|
||||
|
||||
present member.user, with: ::API::Entities::Member, member: member
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
|
@ -145,11 +145,11 @@ describe API::Members, api: true do
|
|||
end
|
||||
end
|
||||
|
||||
it "returns #{source_type == 'project' ? 201 : 409} if member already exists" do
|
||||
it "returns 409 if member already exists" do
|
||||
post api("/#{source_type.pluralize}/#{source.id}/members", master),
|
||||
user_id: master.id, access_level: Member::MASTER
|
||||
|
||||
expect(response).to have_http_status(source_type == 'project' ? 201 : 409)
|
||||
expect(response).to have_http_status(409)
|
||||
end
|
||||
|
||||
it 'returns 400 when user_id is not given' do
|
||||
|
|
|
@ -0,0 +1,342 @@
|
|||
require 'spec_helper'
|
||||
|
||||
describe API::Members, api: true do
|
||||
include ApiHelpers
|
||||
|
||||
let(:master) { create(:user) }
|
||||
let(:developer) { create(:user) }
|
||||
let(:access_requester) { create(:user) }
|
||||
let(:stranger) { create(:user) }
|
||||
|
||||
let(:project) do
|
||||
create(:empty_project, :public, :access_requestable, creator_id: master.id, namespace: master.namespace) do |project|
|
||||
project.team << [developer, :developer]
|
||||
project.team << [master, :master]
|
||||
project.request_access(access_requester)
|
||||
end
|
||||
end
|
||||
|
||||
let!(:group) do
|
||||
create(:group, :public, :access_requestable) do |group|
|
||||
group.add_developer(developer)
|
||||
group.add_owner(master)
|
||||
group.request_access(access_requester)
|
||||
end
|
||||
end
|
||||
|
||||
shared_examples 'GET /:sources/:id/members' do |source_type|
|
||||
context "with :sources == #{source_type.pluralize}" do
|
||||
it_behaves_like 'a 404 response when source is private' do
|
||||
let(:route) { get v3_api("/#{source_type.pluralize}/#{source.id}/members", stranger) }
|
||||
end
|
||||
|
||||
%i[master developer access_requester stranger].each do |type|
|
||||
context "when authenticated as a #{type}" do
|
||||
it 'returns 200' do
|
||||
user = public_send(type)
|
||||
get v3_api("/#{source_type.pluralize}/#{source.id}/members", user)
|
||||
|
||||
expect(response).to have_http_status(200)
|
||||
expect(json_response.size).to eq(2)
|
||||
expect(json_response.map { |u| u['id'] }).to match_array [master.id, developer.id]
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
it 'does not return invitees' do
|
||||
create(:"#{source_type}_member", invite_token: '123', invite_email: 'test@abc.com', source: source, user: nil)
|
||||
|
||||
get v3_api("/#{source_type.pluralize}/#{source.id}/members", developer)
|
||||
|
||||
expect(response).to have_http_status(200)
|
||||
expect(json_response.size).to eq(2)
|
||||
expect(json_response.map { |u| u['id'] }).to match_array [master.id, developer.id]
|
||||
end
|
||||
|
||||
it 'finds members with query string' do
|
||||
get v3_api("/#{source_type.pluralize}/#{source.id}/members", developer), query: master.username
|
||||
|
||||
expect(response).to have_http_status(200)
|
||||
expect(json_response.count).to eq(1)
|
||||
expect(json_response.first['username']).to eq(master.username)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
shared_examples 'GET /:sources/:id/members/:user_id' do |source_type|
|
||||
context "with :sources == #{source_type.pluralize}" do
|
||||
it_behaves_like 'a 404 response when source is private' do
|
||||
let(:route) { get v3_api("/#{source_type.pluralize}/#{source.id}/members/#{developer.id}", stranger) }
|
||||
end
|
||||
|
||||
context 'when authenticated as a non-member' do
|
||||
%i[access_requester stranger].each do |type|
|
||||
context "as a #{type}" do
|
||||
it 'returns 200' do
|
||||
user = public_send(type)
|
||||
get v3_api("/#{source_type.pluralize}/#{source.id}/members/#{developer.id}", user)
|
||||
|
||||
expect(response).to have_http_status(200)
|
||||
# User attributes
|
||||
expect(json_response['id']).to eq(developer.id)
|
||||
expect(json_response['name']).to eq(developer.name)
|
||||
expect(json_response['username']).to eq(developer.username)
|
||||
expect(json_response['state']).to eq(developer.state)
|
||||
expect(json_response['avatar_url']).to eq(developer.avatar_url)
|
||||
expect(json_response['web_url']).to eq(Gitlab::Routing.url_helpers.user_url(developer))
|
||||
|
||||
# Member attributes
|
||||
expect(json_response['access_level']).to eq(Member::DEVELOPER)
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
shared_examples 'POST /:sources/:id/members' do |source_type|
|
||||
context "with :sources == #{source_type.pluralize}" do
|
||||
it_behaves_like 'a 404 response when source is private' do
|
||||
let(:route) do
|
||||
post v3_api("/#{source_type.pluralize}/#{source.id}/members", stranger),
|
||||
user_id: access_requester.id, access_level: Member::MASTER
|
||||
end
|
||||
end
|
||||
|
||||
context 'when authenticated as a non-member or member with insufficient rights' do
|
||||
%i[access_requester stranger developer].each do |type|
|
||||
context "as a #{type}" do
|
||||
it 'returns 403' do
|
||||
user = public_send(type)
|
||||
post v3_api("/#{source_type.pluralize}/#{source.id}/members", user),
|
||||
user_id: access_requester.id, access_level: Member::MASTER
|
||||
|
||||
expect(response).to have_http_status(403)
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
context 'when authenticated as a master/owner' do
|
||||
context 'and new member is already a requester' do
|
||||
it 'transforms the requester into a proper member' do
|
||||
expect do
|
||||
post v3_api("/#{source_type.pluralize}/#{source.id}/members", master),
|
||||
user_id: access_requester.id, access_level: Member::MASTER
|
||||
|
||||
expect(response).to have_http_status(201)
|
||||
end.to change { source.members.count }.by(1)
|
||||
expect(source.requesters.count).to eq(0)
|
||||
expect(json_response['id']).to eq(access_requester.id)
|
||||
expect(json_response['access_level']).to eq(Member::MASTER)
|
||||
end
|
||||
end
|
||||
|
||||
it 'creates a new member' do
|
||||
expect do
|
||||
post v3_api("/#{source_type.pluralize}/#{source.id}/members", master),
|
||||
user_id: stranger.id, access_level: Member::DEVELOPER, expires_at: '2016-08-05'
|
||||
|
||||
expect(response).to have_http_status(201)
|
||||
end.to change { source.members.count }.by(1)
|
||||
expect(json_response['id']).to eq(stranger.id)
|
||||
expect(json_response['access_level']).to eq(Member::DEVELOPER)
|
||||
expect(json_response['expires_at']).to eq('2016-08-05')
|
||||
end
|
||||
end
|
||||
|
||||
it "returns #{source_type == 'project' ? 201 : 409} if member already exists" do
|
||||
post v3_api("/#{source_type.pluralize}/#{source.id}/members", master),
|
||||
user_id: master.id, access_level: Member::MASTER
|
||||
|
||||
expect(response).to have_http_status(source_type == 'project' ? 201 : 409)
|
||||
end
|
||||
|
||||
it 'returns 400 when user_id is not given' do
|
||||
post v3_api("/#{source_type.pluralize}/#{source.id}/members", master),
|
||||
access_level: Member::MASTER
|
||||
|
||||
expect(response).to have_http_status(400)
|
||||
end
|
||||
|
||||
it 'returns 400 when access_level is not given' do
|
||||
post v3_api("/#{source_type.pluralize}/#{source.id}/members", master),
|
||||
user_id: stranger.id
|
||||
|
||||
expect(response).to have_http_status(400)
|
||||
end
|
||||
|
||||
it 'returns 422 when access_level is not valid' do
|
||||
post v3_api("/#{source_type.pluralize}/#{source.id}/members", master),
|
||||
user_id: stranger.id, access_level: 1234
|
||||
|
||||
expect(response).to have_http_status(422)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
shared_examples 'PUT /:sources/:id/members/:user_id' do |source_type|
|
||||
context "with :sources == #{source_type.pluralize}" do
|
||||
it_behaves_like 'a 404 response when source is private' do
|
||||
let(:route) do
|
||||
put v3_api("/#{source_type.pluralize}/#{source.id}/members/#{developer.id}", stranger),
|
||||
access_level: Member::MASTER
|
||||
end
|
||||
end
|
||||
|
||||
context 'when authenticated as a non-member or member with insufficient rights' do
|
||||
%i[access_requester stranger developer].each do |type|
|
||||
context "as a #{type}" do
|
||||
it 'returns 403' do
|
||||
user = public_send(type)
|
||||
put v3_api("/#{source_type.pluralize}/#{source.id}/members/#{developer.id}", user),
|
||||
access_level: Member::MASTER
|
||||
|
||||
expect(response).to have_http_status(403)
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
context 'when authenticated as a master/owner' do
|
||||
it 'updates the member' do
|
||||
put v3_api("/#{source_type.pluralize}/#{source.id}/members/#{developer.id}", master),
|
||||
access_level: Member::MASTER, expires_at: '2016-08-05'
|
||||
|
||||
expect(response).to have_http_status(200)
|
||||
expect(json_response['id']).to eq(developer.id)
|
||||
expect(json_response['access_level']).to eq(Member::MASTER)
|
||||
expect(json_response['expires_at']).to eq('2016-08-05')
|
||||
end
|
||||
end
|
||||
|
||||
it 'returns 409 if member does not exist' do
|
||||
put v3_api("/#{source_type.pluralize}/#{source.id}/members/123", master),
|
||||
access_level: Member::MASTER
|
||||
|
||||
expect(response).to have_http_status(404)
|
||||
end
|
||||
|
||||
it 'returns 400 when access_level is not given' do
|
||||
put v3_api("/#{source_type.pluralize}/#{source.id}/members/#{developer.id}", master)
|
||||
|
||||
expect(response).to have_http_status(400)
|
||||
end
|
||||
|
||||
it 'returns 422 when access level is not valid' do
|
||||
put v3_api("/#{source_type.pluralize}/#{source.id}/members/#{developer.id}", master),
|
||||
access_level: 1234
|
||||
|
||||
expect(response).to have_http_status(422)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
shared_examples 'DELETE /:sources/:id/members/:user_id' do |source_type|
|
||||
context "with :sources == #{source_type.pluralize}" do
|
||||
it_behaves_like 'a 404 response when source is private' do
|
||||
let(:route) { delete v3_api("/#{source_type.pluralize}/#{source.id}/members/#{developer.id}", stranger) }
|
||||
end
|
||||
|
||||
context 'when authenticated as a non-member or member with insufficient rights' do
|
||||
%i[access_requester stranger].each do |type|
|
||||
context "as a #{type}" do
|
||||
it 'returns 403' do
|
||||
user = public_send(type)
|
||||
delete v3_api("/#{source_type.pluralize}/#{source.id}/members/#{developer.id}", user)
|
||||
|
||||
expect(response).to have_http_status(403)
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
context 'when authenticated as a member and deleting themself' do
|
||||
it 'deletes the member' do
|
||||
expect do
|
||||
delete v3_api("/#{source_type.pluralize}/#{source.id}/members/#{developer.id}", developer)
|
||||
|
||||
expect(response).to have_http_status(200)
|
||||
end.to change { source.members.count }.by(-1)
|
||||
end
|
||||
end
|
||||
|
||||
context 'when authenticated as a master/owner' do
|
||||
context 'and member is a requester' do
|
||||
it "returns #{source_type == 'project' ? 200 : 404}" do
|
||||
expect do
|
||||
delete v3_api("/#{source_type.pluralize}/#{source.id}/members/#{access_requester.id}", master)
|
||||
|
||||
expect(response).to have_http_status(source_type == 'project' ? 200 : 404)
|
||||
end.not_to change { source.requesters.count }
|
||||
end
|
||||
end
|
||||
|
||||
it 'deletes the member' do
|
||||
expect do
|
||||
delete v3_api("/#{source_type.pluralize}/#{source.id}/members/#{developer.id}", master)
|
||||
|
||||
expect(response).to have_http_status(200)
|
||||
end.to change { source.members.count }.by(-1)
|
||||
end
|
||||
end
|
||||
|
||||
it "returns #{source_type == 'project' ? 200 : 404} if member does not exist" do
|
||||
delete v3_api("/#{source_type.pluralize}/#{source.id}/members/123", master)
|
||||
|
||||
expect(response).to have_http_status(source_type == 'project' ? 200 : 404)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
it_behaves_like 'GET /:sources/:id/members', 'project' do
|
||||
let(:source) { project }
|
||||
end
|
||||
|
||||
it_behaves_like 'GET /:sources/:id/members', 'group' do
|
||||
let(:source) { group }
|
||||
end
|
||||
|
||||
it_behaves_like 'GET /:sources/:id/members/:user_id', 'project' do
|
||||
let(:source) { project }
|
||||
end
|
||||
|
||||
it_behaves_like 'GET /:sources/:id/members/:user_id', 'group' do
|
||||
let(:source) { group }
|
||||
end
|
||||
|
||||
it_behaves_like 'POST /:sources/:id/members', 'project' do
|
||||
let(:source) { project }
|
||||
end
|
||||
|
||||
it_behaves_like 'POST /:sources/:id/members', 'group' do
|
||||
let(:source) { group }
|
||||
end
|
||||
|
||||
it_behaves_like 'PUT /:sources/:id/members/:user_id', 'project' do
|
||||
let(:source) { project }
|
||||
end
|
||||
|
||||
it_behaves_like 'PUT /:sources/:id/members/:user_id', 'group' do
|
||||
let(:source) { group }
|
||||
end
|
||||
|
||||
it_behaves_like 'DELETE /:sources/:id/members/:user_id', 'project' do
|
||||
let(:source) { project }
|
||||
end
|
||||
|
||||
it_behaves_like 'DELETE /:sources/:id/members/:user_id', 'group' do
|
||||
let(:source) { group }
|
||||
end
|
||||
|
||||
context 'Adding owner to project' do
|
||||
it 'returns 403' do
|
||||
expect do
|
||||
post v3_api("/projects/#{project.id}/members", master),
|
||||
user_id: stranger.id, access_level: Member::OWNER
|
||||
|
||||
expect(response).to have_http_status(422)
|
||||
end.to change { project.members.count }.by(0)
|
||||
end
|
||||
end
|
||||
end
|
Loading…
Reference in New Issue