Commit aff7dccc authored by Sean McGivern's avatar Sean McGivern

Use policies to determine if attributes can be set in the API

This is more idiomatic than checking membership explicitly.
parent b63ed7cf
...@@ -72,6 +72,8 @@ class GroupPolicy < BasePolicy ...@@ -72,6 +72,8 @@ class GroupPolicy < BasePolicy
enable :admin_namespace enable :admin_namespace
enable :admin_group_member enable :admin_group_member
enable :change_visibility_level enable :change_visibility_level
enable :set_note_created_at
end end
rule { can?(:read_nested_project_resources) }.policy do rule { can?(:read_nested_project_resources) }.policy do
......
...@@ -143,6 +143,10 @@ class ProjectPolicy < BasePolicy ...@@ -143,6 +143,10 @@ class ProjectPolicy < BasePolicy
enable :destroy_merge_request enable :destroy_merge_request
enable :destroy_issue enable :destroy_issue
enable :remove_pages enable :remove_pages
enable :set_issue_iid
enable :set_issue_created_at
enable :set_note_created_at
end end
rule { can?(:guest_access) }.policy do rule { can?(:guest_access) }.policy do
......
...@@ -92,10 +92,7 @@ module API ...@@ -92,10 +92,7 @@ module API
parent = noteable_parent(noteable) parent = noteable_parent(noteable)
if opts[:created_at] opts.delete(:created_at) unless current_user.can?(:set_note_created_at, policy_object)
opts.delete(:created_at) unless
(current_user.admin? || user_project.owner == current_user || current_user.owned_groups.include?(user_project.owner))
end
opts[:updated_at] = opts[:created_at] if opts[:created_at] opts[:updated_at] = opts[:created_at] if opts[:created_at]
......
...@@ -172,11 +172,8 @@ module API ...@@ -172,11 +172,8 @@ module API
authorize! :create_issue, user_project authorize! :create_issue, user_project
# Setting created_at time or iid only allowed for admins and project owners params.delete(:created_at) unless current_user.can?(:set_issue_created_at, user_project)
unless current_user.admin? || user_project.owner == current_user || current_user.owned_groups.include?(user_project.owner) params.delete(:iid) unless current_user.can?(:set_issue_iid, user_project)
params.delete(:created_at)
params.delete(:iid)
end
issue_params = declared_params(include_missing: false) issue_params = declared_params(include_missing: false)
......
...@@ -31,6 +31,7 @@ describe GroupPolicy do ...@@ -31,6 +31,7 @@ describe GroupPolicy do
:admin_namespace, :admin_namespace,
:admin_group_member, :admin_group_member,
:change_visibility_level, :change_visibility_level,
:set_note_created_at,
(Gitlab::Database.postgresql? ? :create_subgroup : nil) (Gitlab::Database.postgresql? ? :create_subgroup : nil)
].compact ].compact
end end
......
...@@ -64,6 +64,7 @@ describe ProjectPolicy do ...@@ -64,6 +64,7 @@ describe ProjectPolicy do
%i[ %i[
change_namespace change_visibility_level rename_project remove_project change_namespace change_visibility_level rename_project remove_project
archive_project remove_fork_project destroy_merge_request destroy_issue archive_project remove_fork_project destroy_merge_request destroy_issue
set_issue_iid set_issue_created_at set_note_created_at
] ]
end end
......
...@@ -1023,6 +1023,20 @@ describe API::Issues do ...@@ -1023,6 +1023,20 @@ describe API::Issues do
end end
end end
context 'by a group owner' do
let(:group) { create(:group) }
let(:group_project) { create(:project, :public, namespace: group) }
it 'sets the internal ID on the new issue' do
group.add_owner(user2)
post api("/projects/#{group_project.id}/issues", user2),
title: 'new issue', iid: 9001
expect(response).to have_gitlab_http_status(201)
expect(json_response['iid']).to eq 9001
end
end
context 'by another user' do context 'by another user' do
it 'ignores the given internal ID' do it 'ignores the given internal ID' do
post api("/projects/#{project.id}/issues", user2), post api("/projects/#{project.id}/issues", user2),
...@@ -1154,14 +1168,47 @@ describe API::Issues do ...@@ -1154,14 +1168,47 @@ describe API::Issues do
end end
end end
context 'when an admin or owner makes the request' do context 'setting created_at' do
it 'accepts the creation date to be set' do let(:creation_time) { 2.weeks.ago }
creation_time = 2.weeks.ago let(:params) { { title: 'new issue', labels: 'label, label2', created_at: creation_time } }
post api("/projects/#{project.id}/issues", user),
title: 'new issue', labels: 'label, label2', created_at: creation_time
expect(response).to have_gitlab_http_status(201) context 'by an admin' do
expect(Time.parse(json_response['created_at'])).to be_like_time(creation_time) it 'sets the creation time on the new issue' do
post api("/projects/#{project.id}/issues", admin), params
expect(response).to have_gitlab_http_status(201)
expect(Time.parse(json_response['created_at'])).to be_like_time(creation_time)
end
end
context 'by a project owner' do
it 'sets the creation time on the new issue' do
post api("/projects/#{project.id}/issues", user), params
expect(response).to have_gitlab_http_status(201)
expect(Time.parse(json_response['created_at'])).to be_like_time(creation_time)
end
end
context 'by a group owner' do
it 'sets the creation time on the new issue' do
group = create(:group)
group_project = create(:project, :public, namespace: group)
group.add_owner(user2)
post api("/projects/#{group_project.id}/issues", user2), params
expect(response).to have_gitlab_http_status(201)
expect(Time.parse(json_response['created_at'])).to be_like_time(creation_time)
end
end
context 'by another user' do
it 'ignores the given creation time' do
post api("/projects/#{project.id}/issues", user2), params
expect(response).to have_gitlab_http_status(201)
expect(Time.parse(json_response['created_at'])).not_to be_like_time(creation_time)
end
end end
end end
......
...@@ -111,17 +111,79 @@ shared_examples 'noteable API' do |parent_type, noteable_type, id_name| ...@@ -111,17 +111,79 @@ shared_examples 'noteable API' do |parent_type, noteable_type, id_name|
post api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/notes", user), body: 'hi!' post api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/notes", user), body: 'hi!'
end end
context 'when an admin or owner makes the request' do context 'setting created_at' do
it 'accepts the creation date to be set' do let(:creation_time) { 2.weeks.ago }
creation_time = 2.weeks.ago let(:params) { { body: 'hi!', created_at: creation_time } }
post api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/notes", user),
body: 'hi!', created_at: creation_time context 'by an admin' do
it 'sets the creation time on the new note' do
admin = create(:admin)
post api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/notes", admin), params
expect(response).to have_gitlab_http_status(201)
expect(json_response['body']).to eq('hi!')
expect(json_response['author']['username']).to eq(admin.username)
expect(Time.parse(json_response['created_at'])).to be_like_time(creation_time)
expect(Time.parse(json_response['updated_at'])).to be_like_time(creation_time)
end
end
expect(response).to have_gitlab_http_status(201) if parent_type == 'projects'
expect(json_response['body']).to eq('hi!') context 'by a project owner' do
expect(json_response['author']['username']).to eq(user.username) it 'sets the creation time on the new note' do
expect(Time.parse(json_response['created_at'])).to be_like_time(creation_time) post api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/notes", user), params
expect(Time.parse(json_response['updated_at'])).to be_like_time(creation_time)
expect(response).to have_gitlab_http_status(201)
expect(json_response['body']).to eq('hi!')
expect(json_response['author']['username']).to eq(user.username)
expect(Time.parse(json_response['created_at'])).to be_like_time(creation_time)
expect(Time.parse(json_response['updated_at'])).to be_like_time(creation_time)
end
end
context 'by a group owner' do
it 'sets the creation time on the new note' do
user2 = create(:user)
group = create(:group)
group.add_owner(user2)
parent.update!(namespace: group)
user2.refresh_authorized_projects
post api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/notes", user2), params
expect(response).to have_gitlab_http_status(201)
expect(json_response['body']).to eq('hi!')
expect(json_response['author']['username']).to eq(user2.username)
expect(Time.parse(json_response['created_at'])).to be_like_time(creation_time)
expect(Time.parse(json_response['updated_at'])).to be_like_time(creation_time)
end
end
elsif parent_type == 'groups'
context 'by a group owner' do
it 'sets the creation time on the new note' do
post api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/notes", user), params
expect(response).to have_gitlab_http_status(201)
expect(json_response['body']).to eq('hi!')
expect(json_response['author']['username']).to eq(user.username)
expect(Time.parse(json_response['created_at'])).to be_like_time(creation_time)
expect(Time.parse(json_response['updated_at'])).to be_like_time(creation_time)
end
end
end
context 'by another user' do
it 'ignores the given creation time' do
user2 = create(:user)
parent.add_developer(user2)
post api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/notes", user2), params
expect(response).to have_gitlab_http_status(201)
expect(json_response['body']).to eq('hi!')
expect(json_response['author']['username']).to eq(user2.username)
expect(Time.parse(json_response['created_at'])).not_to be_like_time(creation_time)
expect(Time.parse(json_response['updated_at'])).not_to be_like_time(creation_time)
end
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment