From b63ed7cff664bc1ee0bf70912fffd4814f757079 Mon Sep 17 00:00:00 2001 From: Florent Dubois Date: Wed, 6 Dec 2017 12:07:08 +0100 Subject: Allow date parameters on Issues and Notes API for group owners - Allow `created_at` and `updated_at` parameters on Issues API - Allow `created_at` on Issue Notes API Closes gitlab-org/gitlab-ce#40059 --- changelogs/unreleased/fix-api-group-createdat.yml | 5 +++++ doc/api/issues.md | 2 +- doc/api/notes.md | 2 +- lib/api/helpers/notes_helpers.rb | 2 +- lib/api/issues.rb | 6 +++--- 5 files changed, 11 insertions(+), 6 deletions(-) create mode 100644 changelogs/unreleased/fix-api-group-createdat.yml diff --git a/changelogs/unreleased/fix-api-group-createdat.yml b/changelogs/unreleased/fix-api-group-createdat.yml new file mode 100644 index 00000000000..cec651b47da --- /dev/null +++ b/changelogs/unreleased/fix-api-group-createdat.yml @@ -0,0 +1,5 @@ +--- +title: Allow date parameters on Issues and Notes API for group owners +merge_request: +author: Florent Dubois +type: fixed diff --git a/doc/api/issues.md b/doc/api/issues.md index 103eaa5655f..f4c0f4ea65b 100644 --- a/doc/api/issues.md +++ b/doc/api/issues.md @@ -470,7 +470,7 @@ POST /projects/:id/issues | `assignee_ids` | Array[integer] | no | The ID of the users to assign issue | | `milestone_id` | integer | no | The global ID of a milestone to assign issue | | `labels` | string | no | Comma-separated label names for an issue | -| `created_at` | string | no | Date time string, ISO 8601 formatted, e.g. `2016-03-11T03:45:40Z` (requires admin or project owner rights) | +| `created_at` | string | no | Date time string, ISO 8601 formatted, e.g. `2016-03-11T03:45:40Z` (requires admin or project/group owner rights) | | `due_date` | string | no | Date time string in the format YEAR-MONTH-DAY, e.g. `2016-03-11` | | `merge_request_to_resolve_discussions_of` | integer | no | The IID of a merge request in which to resolve all issues. This will fill the issue with a default description and mark all discussions as resolved. When passing a description or title, these values will take precedence over the default values.| | `discussion_to_resolve` | string | no | The ID of a discussion to resolve. This will fill in the issue with a default description and mark the discussion as resolved. Use in combination with `merge_request_to_resolve_discussions_of`. | diff --git a/doc/api/notes.md b/doc/api/notes.md index c271d46688f..44940bdd9e5 100644 --- a/doc/api/notes.md +++ b/doc/api/notes.md @@ -100,7 +100,7 @@ Parameters: - `id` (required) - The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) - `issue_id` (required) - The IID of an issue - `body` (required) - The content of a note -- `created_at` (optional) - Date time string, ISO 8601 formatted, e.g. 2016-03-11T03:45:40Z +- `created_at` (optional) - Date time string, ISO 8601 formatted, e.g. 2016-03-11T03:45:40Z (requires admin or project/group owner rights) ```bash curl --request POST --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/projects/5/issues/11/notes?body=note diff --git a/lib/api/helpers/notes_helpers.rb b/lib/api/helpers/notes_helpers.rb index e2984b08eca..7885e3fa1e1 100644 --- a/lib/api/helpers/notes_helpers.rb +++ b/lib/api/helpers/notes_helpers.rb @@ -94,7 +94,7 @@ module API if opts[:created_at] opts.delete(:created_at) unless - current_user.admin? || parent.owned_by?(current_user) + (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] diff --git a/lib/api/issues.rb b/lib/api/issues.rb index bda05d1795b..2d2250acaa2 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -173,7 +173,7 @@ module API authorize! :create_issue, user_project # Setting created_at time or iid only allowed for admins and project owners - unless current_user.admin? || user_project.owner == current_user + unless current_user.admin? || user_project.owner == current_user || current_user.owned_groups.include?(user_project.owner) params.delete(:created_at) params.delete(:iid) end @@ -216,8 +216,8 @@ module API issue = user_project.issues.find_by!(iid: params.delete(:issue_iid)) authorize! :update_issue, issue - # Setting created_at time only allowed for admins and project owners - unless current_user.admin? || user_project.owner == current_user + # Setting created_at time only allowed for admins and project/group owners + unless current_user.admin? || user_project.owner == current_user || current_user.owned_groups.include?(user_project.owner) params.delete(:updated_at) end -- cgit v1.2.1 From aff7dccc1f13e86b44dfa1530c6b5068dbb18f00 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Wed, 22 Aug 2018 13:10:54 +0100 Subject: Use policies to determine if attributes can be set in the API This is more idiomatic than checking membership explicitly. --- app/policies/group_policy.rb | 2 + app/policies/project_policy.rb | 4 ++ lib/api/helpers/notes_helpers.rb | 5 +- lib/api/issues.rb | 7 +- spec/policies/group_policy_spec.rb | 1 + spec/policies/project_policy_spec.rb | 1 + spec/requests/api/issues_spec.rb | 61 ++++++++++++++-- spec/support/shared_examples/requests/api/notes.rb | 82 +++++++++++++++++++--- 8 files changed, 137 insertions(+), 26 deletions(-) diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb index a8d7a05f509..bb800929ea9 100644 --- a/app/policies/group_policy.rb +++ b/app/policies/group_policy.rb @@ -72,6 +72,8 @@ class GroupPolicy < BasePolicy enable :admin_namespace enable :admin_group_member enable :change_visibility_level + + enable :set_note_created_at end rule { can?(:read_nested_project_resources) }.policy do diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index 00c58f15013..fd6cc504a3b 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -143,6 +143,10 @@ class ProjectPolicy < BasePolicy enable :destroy_merge_request enable :destroy_issue enable :remove_pages + + enable :set_issue_iid + enable :set_issue_created_at + enable :set_note_created_at end rule { can?(:guest_access) }.policy do diff --git a/lib/api/helpers/notes_helpers.rb b/lib/api/helpers/notes_helpers.rb index 7885e3fa1e1..7b1f5c2584b 100644 --- a/lib/api/helpers/notes_helpers.rb +++ b/lib/api/helpers/notes_helpers.rb @@ -92,10 +92,7 @@ module API parent = noteable_parent(noteable) - if opts[:created_at] - opts.delete(:created_at) unless - (current_user.admin? || user_project.owner == current_user || current_user.owned_groups.include?(user_project.owner)) - end + opts.delete(:created_at) unless current_user.can?(:set_note_created_at, policy_object) opts[:updated_at] = opts[:created_at] if opts[:created_at] diff --git a/lib/api/issues.rb b/lib/api/issues.rb index 2d2250acaa2..cedfd2fbaa0 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -172,11 +172,8 @@ module API authorize! :create_issue, user_project - # Setting created_at time or iid only allowed for admins and project owners - unless current_user.admin? || user_project.owner == current_user || current_user.owned_groups.include?(user_project.owner) - params.delete(:created_at) - params.delete(:iid) - end + params.delete(:created_at) unless current_user.can?(:set_issue_created_at, user_project) + params.delete(:iid) unless current_user.can?(:set_issue_iid, user_project) issue_params = declared_params(include_missing: false) diff --git a/spec/policies/group_policy_spec.rb b/spec/policies/group_policy_spec.rb index 615fea11f26..f276535265b 100644 --- a/spec/policies/group_policy_spec.rb +++ b/spec/policies/group_policy_spec.rb @@ -31,6 +31,7 @@ describe GroupPolicy do :admin_namespace, :admin_group_member, :change_visibility_level, + :set_note_created_at, (Gitlab::Database.postgresql? ? :create_subgroup : nil) ].compact end diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb index dd3fa4e6a51..b7ec35d6ec5 100644 --- a/spec/policies/project_policy_spec.rb +++ b/spec/policies/project_policy_spec.rb @@ -64,6 +64,7 @@ describe ProjectPolicy do %i[ change_namespace change_visibility_level rename_project remove_project archive_project remove_fork_project destroy_merge_request destroy_issue + set_issue_iid set_issue_created_at set_note_created_at ] end diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb index 28ba00c7293..f64815feffa 100644 --- a/spec/requests/api/issues_spec.rb +++ b/spec/requests/api/issues_spec.rb @@ -1023,6 +1023,20 @@ describe API::Issues do 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 it 'ignores the given internal ID' do post api("/projects/#{project.id}/issues", user2), @@ -1154,14 +1168,47 @@ describe API::Issues do end end - context 'when an admin or owner makes the request' do - it 'accepts the creation date to be set' do - creation_time = 2.weeks.ago - post api("/projects/#{project.id}/issues", user), - title: 'new issue', labels: 'label, label2', created_at: creation_time + context 'setting created_at' do + let(:creation_time) { 2.weeks.ago } + let(:params) { { title: 'new issue', labels: 'label, label2', created_at: creation_time } } - expect(response).to have_gitlab_http_status(201) - expect(Time.parse(json_response['created_at'])).to be_like_time(creation_time) + context 'by an admin' do + 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 diff --git a/spec/support/shared_examples/requests/api/notes.rb b/spec/support/shared_examples/requests/api/notes.rb index 1b563021244..0e20dfe0725 100644 --- a/spec/support/shared_examples/requests/api/notes.rb +++ b/spec/support/shared_examples/requests/api/notes.rb @@ -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!' end - context 'when an admin or owner makes the request' do - it 'accepts the creation date to be set' do - creation_time = 2.weeks.ago - post api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/notes", user), - body: 'hi!', created_at: creation_time + context 'setting created_at' do + let(:creation_time) { 2.weeks.ago } + let(:params) { { 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) - 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) + if parent_type == 'projects' + context 'by a project 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 + + 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 -- cgit v1.2.1 From 2f5ce7d2bdf07724f9f7bbe9aed8efd0725165aa Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Wed, 22 Aug 2018 13:25:16 +0100 Subject: Set MR number in changelog --- changelogs/unreleased/fix-api-group-createdat.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelogs/unreleased/fix-api-group-createdat.yml b/changelogs/unreleased/fix-api-group-createdat.yml index cec651b47da..384b8d91182 100644 --- a/changelogs/unreleased/fix-api-group-createdat.yml +++ b/changelogs/unreleased/fix-api-group-createdat.yml @@ -1,5 +1,5 @@ --- title: Allow date parameters on Issues and Notes API for group owners -merge_request: +merge_request: 21342 author: Florent Dubois type: fixed -- cgit v1.2.1 From 0f269f9c99e72e082fef6283387366b2831702d8 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Thu, 23 Aug 2018 10:29:24 +0100 Subject: Document that created_at works for group owners on Discussions --- changelogs/unreleased/fix-api-group-createdat.yml | 2 +- doc/api/discussions.md | 16 ++++++++-------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/changelogs/unreleased/fix-api-group-createdat.yml b/changelogs/unreleased/fix-api-group-createdat.yml index 384b8d91182..e628facf1bf 100644 --- a/changelogs/unreleased/fix-api-group-createdat.yml +++ b/changelogs/unreleased/fix-api-group-createdat.yml @@ -1,5 +1,5 @@ --- -title: Allow date parameters on Issues and Notes API for group owners +title: Allow date parameters on Issues, Notes, and Discussions API for group owners merge_request: 21342 author: Florent Dubois type: fixed diff --git a/doc/api/discussions.md b/doc/api/discussions.md index 65e2f9d6cd9..a1e1ff1419d 100644 --- a/doc/api/discussions.md +++ b/doc/api/discussions.md @@ -136,7 +136,7 @@ Parameters: | `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) | | `issue_iid` | integer | yes | The IID of an issue | | `body` | string | yes | The content of a discussion | -| `created_at` | string | no | Date time string, ISO 8601 formatted, e.g. 2016-03-11T03:45:40Z | +| `created_at` | string | no | Date time string, ISO 8601 formatted, e.g. 2016-03-11T03:45:40Z (requires admin or project/group owner rights) | ```bash curl --request POST --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/projects/5/issues/11/discussions?body=comment @@ -159,7 +159,7 @@ Parameters: | `discussion_id` | integer | yes | The ID of a discussion | | `note_id` | integer | yes | The ID of a discussion note | | `body` | string | yes | The content of a discussion | -| `created_at` | string | no | Date time string, ISO 8601 formatted, e.g. 2016-03-11T03:45:40Z | +| `created_at` | string | no | Date time string, ISO 8601 formatted, e.g. 2016-03-11T03:45:40Z (requires admin or project/group owner rights) | ```bash curl --request POST --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/projects/5/issues/11/discussions/6a9c1750b37d513a43987b574953fceb50b03ce7/notes?body=comment @@ -342,7 +342,7 @@ Parameters: | `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) | | `snippet_id` | integer | yes | The ID of an snippet | | `body` | string | yes | The content of a discussion | -| `created_at` | string | no | Date time string, ISO 8601 formatted, e.g. 2016-03-11T03:45:40Z | +| `created_at` | string | no | Date time string, ISO 8601 formatted, e.g. 2016-03-11T03:45:40Z (requires admin or project/group owner rights) | ```bash curl --request POST --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/projects/5/snippets/11/discussions?body=comment @@ -365,7 +365,7 @@ Parameters: | `discussion_id` | integer | yes | The ID of a discussion | | `note_id` | integer | yes | The ID of a discussion note | | `body` | string | yes | The content of a discussion | -| `created_at` | string | no | Date time string, ISO 8601 formatted, e.g. 2016-03-11T03:45:40Z | +| `created_at` | string | no | Date time string, ISO 8601 formatted, e.g. 2016-03-11T03:45:40Z (requires admin or project/group owner rights) | ```bash curl --request POST --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/projects/5/snippets/11/discussions/6a9c1750b37d513a43987b574953fceb50b03ce7/notes?body=comment @@ -601,7 +601,7 @@ Parameters: | `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) | | `merge_request_iid` | integer | yes | The IID of a merge request | | `body` | string | yes | The content of a discussion | -| `created_at` | string | no | Date time string, ISO 8601 formatted, e.g. 2016-03-11T03:45:40Z | +| `created_at` | string | no | Date time string, ISO 8601 formatted, e.g. 2016-03-11T03:45:40Z (requires admin or project/group owner rights) | | `position` | hash | no | Position when creating a diff note | | `position[base_sha]` | string | yes | Base commit SHA in the source branch | | `position[start_sha]` | string | yes | SHA referencing commit in target branch | @@ -659,7 +659,7 @@ Parameters: | `discussion_id` | integer | yes | The ID of a discussion | | `note_id` | integer | yes | The ID of a discussion note | | `body` | string | yes | The content of a discussion | -| `created_at` | string | no | Date time string, ISO 8601 formatted, e.g. 2016-03-11T03:45:40Z | +| `created_at` | string | no | Date time string, ISO 8601 formatted, e.g. 2016-03-11T03:45:40Z (requires admin or project/group owner rights) | ```bash curl --request POST --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/projects/5/merge_requests/11/discussions/6a9c1750b37d513a43987b574953fceb50b03ce7/notes?body=comment @@ -894,7 +894,7 @@ Parameters: | `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) | | `commit_id` | integer | yes | The ID of a commit | | `body` | string | yes | The content of a discussion | -| `created_at` | string | no | Date time string, ISO 8601 formatted, e.g. 2016-03-11T03:45:40Z | +| `created_at` | string | no | Date time string, ISO 8601 formatted, e.g. 2016-03-11T03:45:40Z (requires admin or project/group owner rights) | | `position` | hash | no | Position when creating a diff note | | `position[base_sha]` | string | yes | Base commit SHA in the source branch | | `position[start_sha]` | string | yes | SHA referencing commit in target branch | @@ -930,7 +930,7 @@ Parameters: | `discussion_id` | integer | yes | The ID of a discussion | | `note_id` | integer | yes | The ID of a discussion note | | `body` | string | yes | The content of a discussion | -| `created_at` | string | no | Date time string, ISO 8601 formatted, e.g. 2016-03-11T03:45:40Z | +| `created_at` | string | no | Date time string, ISO 8601 formatted, e.g. 2016-03-11T03:45:40Z (requires admin or project/group owner rights) | ```bash curl --request POST --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/projects/5/commits/11/discussions/6a9c1750b37d513a43987b574953fceb50b03ce7/notes?body=comment -- cgit v1.2.1