diff options
author | Stan Hu <stanhu@gmail.com> | 2019-08-27 20:31:59 +0300 |
---|---|---|
committer | Stan Hu <stanhu@gmail.com> | 2019-08-29 08:49:58 +0300 |
commit | 680f437715dcf7a8871aa997559cf57362b43217 (patch) | |
tree | 83ee049647b181ed9e74ad3d221f5d78106c5c8b | |
parent | 549e95b8f921dfb30bc7982e9957ce9ccdfd916e (diff) |
Fix snippets API not working with visibility level
When a restricted visibility level of `private` is set in the instance,
creating a snippet with the `visibility` level would always fail.
This happened because:
1. `params[:visibility]` was a string (e.g. "public")
2. `CreateSnippetService` and `UpdateSnippetService` only looked
at `params[:visibility_level]`, which was `nil`.
To fix this, we:
1. Make `CreateSnippetService` look at the newly-built
`snippet.visibility_level`, since the right value is assigned by the
`VisibilityLevel#visibility=` method.
2. Modify `UpdateSnippetService` to handle both `visibility_level` and
`visibility` parameters.
Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/66050
-rw-r--r-- | app/services/base_service.rb | 4 | ||||
-rw-r--r-- | app/services/create_snippet_service.rb | 2 | ||||
-rw-r--r-- | app/services/groups/create_service.rb | 4 | ||||
-rw-r--r-- | app/services/update_snippet_service.rb | 2 | ||||
-rw-r--r-- | changelogs/unreleased/sh-fix-snippet-visibility-api.yml | 5 | ||||
-rw-r--r-- | spec/requests/api/project_snippets_spec.rb | 25 | ||||
-rw-r--r-- | spec/requests/api/snippets_spec.rb | 63 | ||||
-rw-r--r-- | spec/services/create_snippet_service_spec.rb | 13 | ||||
-rw-r--r-- | spec/services/update_snippet_service_spec.rb | 15 |
9 files changed, 108 insertions, 25 deletions
diff --git a/app/services/base_service.rb b/app/services/base_service.rb index 3e968c8f707..c39edd5c114 100644 --- a/app/services/base_service.rb +++ b/app/services/base_service.rb @@ -44,6 +44,10 @@ class BaseService model.errors.add(:visibility_level, "#{level_name} has been restricted by your GitLab administrator") end + def visibility_level + params[:visibility].is_a?(String) ? Gitlab::VisibilityLevel.level_value(params[:visibility]) : params[:visibility_level] + end + private def error(message, http_status = nil) diff --git a/app/services/create_snippet_service.rb b/app/services/create_snippet_service.rb index 6e5bf823cc7..0aa76df35ba 100644 --- a/app/services/create_snippet_service.rb +++ b/app/services/create_snippet_service.rb @@ -12,7 +12,7 @@ class CreateSnippetService < BaseService PersonalSnippet.new(params) end - unless Gitlab::VisibilityLevel.allowed_for?(current_user, params[:visibility_level]) + unless Gitlab::VisibilityLevel.allowed_for?(current_user, snippet.visibility_level) deny_visibility_level(snippet) return snippet end diff --git a/app/services/groups/create_service.rb b/app/services/groups/create_service.rb index e78e5d5fc2c..1dd22d7a3ae 100644 --- a/app/services/groups/create_service.rb +++ b/app/services/groups/create_service.rb @@ -68,9 +68,5 @@ module Groups true end - - def visibility_level - params[:visibility].present? ? Gitlab::VisibilityLevel.level_value(params[:visibility]) : params[:visibility_level] - end end end diff --git a/app/services/update_snippet_service.rb b/app/services/update_snippet_service.rb index 2969c360de5..a294812ef9e 100644 --- a/app/services/update_snippet_service.rb +++ b/app/services/update_snippet_service.rb @@ -12,7 +12,7 @@ class UpdateSnippetService < BaseService def execute # check that user is allowed to set specified visibility_level - new_visibility = params[:visibility_level] + new_visibility = visibility_level if new_visibility && new_visibility.to_i != snippet.visibility_level unless Gitlab::VisibilityLevel.allowed_for?(current_user, new_visibility) diff --git a/changelogs/unreleased/sh-fix-snippet-visibility-api.yml b/changelogs/unreleased/sh-fix-snippet-visibility-api.yml new file mode 100644 index 00000000000..5cfb9cdedc0 --- /dev/null +++ b/changelogs/unreleased/sh-fix-snippet-visibility-api.yml @@ -0,0 +1,5 @@ +--- +title: Fix snippets API not working with visibility level +merge_request: 32286 +author: +type: fixed diff --git a/spec/requests/api/project_snippets_spec.rb b/spec/requests/api/project_snippets_spec.rb index 29f69b6ce20..a543ef8de78 100644 --- a/spec/requests/api/project_snippets_spec.rb +++ b/spec/requests/api/project_snippets_spec.rb @@ -96,6 +96,28 @@ describe API::ProjectSnippets do } end + context 'with a regular user' do + let(:user) { create(:user) } + + before do + project.add_developer(user) + stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::PUBLIC, Gitlab::VisibilityLevel::PRIVATE]) + params['visibility'] = 'internal' + end + + it 'creates a new snippet' do + post api("/projects/#{project.id}/snippets/", user), params: params + + expect(response).to have_gitlab_http_status(201) + snippet = ProjectSnippet.find(json_response['id']) + expect(snippet.content).to eq(params[:code]) + expect(snippet.description).to eq(params[:description]) + expect(snippet.title).to eq(params[:title]) + expect(snippet.file_name).to eq(params[:file_name]) + expect(snippet.visibility_level).to eq(Snippet::INTERNAL) + end + end + it 'creates a new snippet' do post api("/projects/#{project.id}/snippets/", admin), params: params @@ -167,12 +189,13 @@ describe API::ProjectSnippets do new_content = 'New content' new_description = 'New description' - put api("/projects/#{snippet.project.id}/snippets/#{snippet.id}/", admin), params: { code: new_content, description: new_description } + put api("/projects/#{snippet.project.id}/snippets/#{snippet.id}/", admin), params: { code: new_content, description: new_description, visibility: 'private' } expect(response).to have_gitlab_http_status(200) snippet.reload expect(snippet.content).to eq(new_content) expect(snippet.description).to eq(new_description) + expect(snippet.visibility).to eq('private') end it 'returns 404 for invalid snippet id' do diff --git a/spec/requests/api/snippets_spec.rb b/spec/requests/api/snippets_spec.rb index d600076e9fb..cc05b8d5b45 100644 --- a/spec/requests/api/snippets_spec.rb +++ b/spec/requests/api/snippets_spec.rb @@ -193,18 +193,32 @@ describe API::Snippets do } end - it 'creates a new snippet' do - expect do - post api("/snippets/", user), params: params - end.to change { PersonalSnippet.count }.by(1) + shared_examples 'snippet creation' do + it 'creates a new snippet' do + expect do + post api("/snippets/", user), params: params + end.to change { PersonalSnippet.count }.by(1) + + expect(response).to have_gitlab_http_status(201) + expect(json_response['title']).to eq(params[:title]) + expect(json_response['description']).to eq(params[:description]) + expect(json_response['file_name']).to eq(params[:file_name]) + expect(json_response['visibility']).to eq(params[:visibility]) + end + end + + context 'with restricted visibility settings' do + before do + stub_application_setting(restricted_visibility_levels: + [Gitlab::VisibilityLevel::INTERNAL, + Gitlab::VisibilityLevel::PRIVATE]) + end - expect(response).to have_gitlab_http_status(201) - expect(json_response['title']).to eq(params[:title]) - expect(json_response['description']).to eq(params[:description]) - expect(json_response['file_name']).to eq(params[:file_name]) - expect(json_response['visibility']).to eq(params[:visibility]) + it_behaves_like 'snippet creation' end + it_behaves_like 'snippet creation' + it 'returns 400 for missing parameters' do params.delete(:title) @@ -253,18 +267,33 @@ describe API::Snippets do create(:personal_snippet, author: user, visibility_level: visibility_level) end - it 'updates snippet' do - new_content = 'New content' - new_description = 'New description' + shared_examples 'snippet updates' do + it 'updates a snippet' do + new_content = 'New content' + new_description = 'New description' - put api("/snippets/#{snippet.id}", user), params: { content: new_content, description: new_description } + put api("/snippets/#{snippet.id}", user), params: { content: new_content, description: new_description, visibility: 'internal' } - expect(response).to have_gitlab_http_status(200) - snippet.reload - expect(snippet.content).to eq(new_content) - expect(snippet.description).to eq(new_description) + expect(response).to have_gitlab_http_status(200) + snippet.reload + expect(snippet.content).to eq(new_content) + expect(snippet.description).to eq(new_description) + expect(snippet.visibility).to eq('internal') + end end + context 'with restricted visibility settings' do + before do + stub_application_setting(restricted_visibility_levels: + [Gitlab::VisibilityLevel::PUBLIC, + Gitlab::VisibilityLevel::PRIVATE]) + end + + it_behaves_like 'snippet updates' + end + + it_behaves_like 'snippet updates' + it 'returns 404 for invalid snippet id' do put api("/snippets/1234", user), params: { title: 'foo' } diff --git a/spec/services/create_snippet_service_spec.rb b/spec/services/create_snippet_service_spec.rb index 9b83f65a17e..7d2491b3a49 100644 --- a/spec/services/create_snippet_service_spec.rb +++ b/spec/services/create_snippet_service_spec.rb @@ -34,6 +34,19 @@ describe CreateSnippetService do expect(snippet.errors.any?).to be_falsey expect(snippet.visibility_level).to eq(Gitlab::VisibilityLevel::PUBLIC) end + + describe "when visibility level is passed as a string" do + before do + @opts[:visibility] = 'internal' + @opts.delete(:visibility_level) + end + + it "assigns the correct visibility level" do + snippet = create_snippet(nil, @user, @opts) + expect(snippet.errors.any?).to be_falsey + expect(snippet.visibility_level).to eq(Gitlab::VisibilityLevel::INTERNAL) + end + end end describe 'usage counter' do diff --git a/spec/services/update_snippet_service_spec.rb b/spec/services/update_snippet_service_spec.rb index 0678f54c195..19b35dca6a7 100644 --- a/spec/services/update_snippet_service_spec.rb +++ b/spec/services/update_snippet_service_spec.rb @@ -32,12 +32,25 @@ describe UpdateSnippetService do expect(@snippet.visibility_level).to eq(old_visibility) end - it 'admins should be able to update to pubic visibility' do + it 'admins should be able to update to public visibility' do old_visibility = @snippet.visibility_level update_snippet(@project, @admin, @snippet, @opts) expect(@snippet.visibility_level).not_to eq(old_visibility) expect(@snippet.visibility_level).to eq(Gitlab::VisibilityLevel::PUBLIC) end + + describe "when visibility level is passed as a string" do + before do + @opts[:visibility] = 'internal' + @opts.delete(:visibility_level) + end + + it "assigns the correct visibility level" do + update_snippet(@project, @user, @snippet, @opts) + expect(@snippet.errors.any?).to be_falsey + expect(@snippet.visibility_level).to eq(Gitlab::VisibilityLevel::INTERNAL) + end + end end describe 'usage counter' do |