From 19a0166f33441ce29c5f42a2e976a6b52f6db4a3 Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Thu, 13 May 2021 12:22:38 +1200 Subject: [PATCH 1/4] Fix services API returning nonexistent services The REST `GET projects/:id/services/:slug` API previously returned unsaved service objects. For example (note the "id": null): { "id": null, "title": "Asana", "slug": "asana", "created_at": null, "updated_at": null, "active": false, "commit_events": true, "push_events": true, "issues_events": true, "confidential_issues_events": true, "merge_requests_events": true, "tag_push_events": true, "note_events": true, "confidential_note_events": true, "pipeline_events": true, "wiki_page_events": true, "job_events": true, "comment_on_event_enabled": true, "properties": {} } This changes the API to return a 404 response when a project does not have the service. https://gitlab.com/gitlab-org/gitlab/-/issues/330818 --- lib/api/services.rb | 8 +++-- spec/requests/api/services_spec.rb | 36 +++++++++++++++++-- .../services_shared_context.rb | 3 +- 3 files changed, 41 insertions(+), 6 deletions(-) diff --git a/lib/api/services.rb b/lib/api/services.rb index fb45337cf05574..b44b9d781b019b 100644 --- a/lib/api/services.rb +++ b/lib/api/services.rb @@ -125,8 +125,12 @@ def service_attributes(service) requires :service_slug, type: String, values: SERVICES.keys, desc: 'The name of the service' end get ":id/services/:service_slug" do - service = user_project.find_or_initialize_service(params[:service_slug].underscore) - present service, with: Entities::ProjectService + type = ::Integration.service_name_to_type(params[:service_slug].underscore) + integration = user_project.integrations.by_type(type).first + + not_found!('Service') unless integration + + present integration, with: Entities::ProjectService end end diff --git a/spec/requests/api/services_spec.rb b/spec/requests/api/services_spec.rb index e52e1bf4695df5..ddaf10dd32e98f 100644 --- a/spec/requests/api/services_spec.rb +++ b/spec/requests/api/services_spec.rb @@ -114,21 +114,51 @@ describe "GET /projects/:id/services/#{service.dasherize}" do include_context service - # inject some properties into the service - let!(:initialized_service) { initialize_service(service) } + let!(:initialized_service) { initialize_service(service, active: true) } + + let_it_be(:project2) do + create(:project, creator_id: user.id, namespace: user.namespace) + end + + def deactive_service! + return initialized_service.update!(active: false) unless initialized_service.is_a?(PrometheusService) + + # PrometheusService sets `#active` itself within a `before_save`: + initialized_service.manual_configuration = false + initialized_service.save! + end it 'returns authentication error when unauthenticated' do get api("/projects/#{project.id}/services/#{dashed_service}") expect(response).to have_gitlab_http_status(:unauthorized) end - it "returns all properties of service #{service}" do + it "returns all properties of active service #{service}" do get api("/projects/#{project.id}/services/#{dashed_service}", user) + expect(initialized_service).to be_active expect(response).to have_gitlab_http_status(:ok) expect(json_response['properties'].keys).to match_array(service_instance.api_field_names) end + it "returns all properties of inactive service #{service}" do + deactive_service! + + get api("/projects/#{project.id}/services/#{dashed_service}", user) + + # binding.pry if initialized_service.active? + expect(initialized_service).not_to be_active + expect(response).to have_gitlab_http_status(:ok) + expect(json_response['properties'].keys).to match_array(service_instance.api_field_names) + end + + it "returns not found if service does not exist" do + get api("/projects/#{project2.id}/services/#{dashed_service}", user) + + expect(response).to have_gitlab_http_status(:not_found) + expect(json_response['message']).to eq('404 Service Not Found') + end + it "returns error when authenticated but not a project owner" do project.add_developer(user2) get api("/projects/#{project.id}/services/#{dashed_service}", user2) diff --git a/spec/support/shared_contexts/services_shared_context.rb b/spec/support/shared_contexts/services_shared_context.rb index 576261030a7e57..34c92367efa0d1 100644 --- a/spec/support/shared_contexts/services_shared_context.rb +++ b/spec/support/shared_contexts/services_shared_context.rb @@ -49,8 +49,9 @@ stub_jira_service_test if service == 'jira' end - def initialize_service(service) + def initialize_service(service, attrs = {}) service_item = project.find_or_initialize_service(service) + service_item.attributes = attrs service_item.properties = service_attrs service_item.save! service_item -- GitLab From a5c1c70c7bcccb3844b27f6df88b2d0c057de01f Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Thu, 13 May 2021 12:34:55 +1200 Subject: [PATCH 2/4] Add changelog entry --- .../330818-services-api-return-404-for-unsaved-services.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/330818-services-api-return-404-for-unsaved-services.yml diff --git a/changelogs/unreleased/330818-services-api-return-404-for-unsaved-services.yml b/changelogs/unreleased/330818-services-api-return-404-for-unsaved-services.yml new file mode 100644 index 00000000000000..8df02746b373c5 --- /dev/null +++ b/changelogs/unreleased/330818-services-api-return-404-for-unsaved-services.yml @@ -0,0 +1,5 @@ +--- +title: Fix services API returning non-existing services +merge_request: 61646 +author: +type: fixed -- GitLab From 03e8f9c29e2611e5109b509d92789c3a29a9e1e4 Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Thu, 13 May 2021 12:48:21 +1200 Subject: [PATCH 3/4] Remove pry comment --- spec/requests/api/services_spec.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/spec/requests/api/services_spec.rb b/spec/requests/api/services_spec.rb index ddaf10dd32e98f..8a269ad3273fa1 100644 --- a/spec/requests/api/services_spec.rb +++ b/spec/requests/api/services_spec.rb @@ -146,7 +146,6 @@ def deactive_service! get api("/projects/#{project.id}/services/#{dashed_service}", user) - # binding.pry if initialized_service.active? expect(initialized_service).not_to be_active expect(response).to have_gitlab_http_status(:ok) expect(json_response['properties'].keys).to match_array(service_instance.api_field_names) -- GitLab From b7c86219676ae88ef4f57bca0604abd0048f7cd8 Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Fri, 14 May 2021 12:16:02 +1200 Subject: [PATCH 4/4] Add reviewer feedback --- lib/api/services.rb | 5 ++--- spec/requests/api/services_spec.rb | 11 +++++++++++ 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/lib/api/services.rb b/lib/api/services.rb index b44b9d781b019b..8a7abe721ddcbe 100644 --- a/lib/api/services.rb +++ b/lib/api/services.rb @@ -125,10 +125,9 @@ def service_attributes(service) requires :service_slug, type: String, values: SERVICES.keys, desc: 'The name of the service' end get ":id/services/:service_slug" do - type = ::Integration.service_name_to_type(params[:service_slug].underscore) - integration = user_project.integrations.by_type(type).first + integration = user_project.find_or_initialize_service(params[:service_slug].underscore) - not_found!('Service') unless integration + not_found!('Service') unless integration&.persisted? present integration, with: Entities::ProjectService end diff --git a/spec/requests/api/services_spec.rb b/spec/requests/api/services_spec.rb index 8a269ad3273fa1..1f859622760f9d 100644 --- a/spec/requests/api/services_spec.rb +++ b/spec/requests/api/services_spec.rb @@ -158,6 +158,17 @@ def deactive_service! expect(json_response['message']).to eq('404 Service Not Found') end + it "returns not found if service exists but is in `Project#disabled_services`" do + expect_next_found_instance_of(Project) do |project| + expect(project).to receive(:disabled_services).at_least(:once).and_return([service]) + end + + get api("/projects/#{project.id}/services/#{dashed_service}", user) + + expect(response).to have_gitlab_http_status(:not_found) + expect(json_response['message']).to eq('404 Service Not Found') + end + it "returns error when authenticated but not a project owner" do project.add_developer(user2) get api("/projects/#{project.id}/services/#{dashed_service}", user2) -- GitLab