From 3e84b7c5bf1e2d9579771f2eb1e2a950ed2556c3 Mon Sep 17 00:00:00 2001 From: Philip Cunningham <pcunningham@gitlab.com> Date: Fri, 12 Feb 2021 05:06:27 +0000 Subject: [PATCH] Add DastProfileUpdate GraphQL mutation Allow users to update existing Dast::Profiles. --- config/known_invalid_graphql_queries.yml | 1 - .../graphql/reference/gitlab_schema.graphql | 73 ++++++ doc/api/graphql/reference/gitlab_schema.json | 207 ++++++++++++++++++ doc/api/graphql/reference/index.md | 11 + ee/app/graphql/ee/types/mutation_type.rb | 1 + .../graphql/mutations/dast/profiles/update.rb | 109 +++++++++ .../services/dast/profiles/update_service.rb | 44 ++-- .../mutations/dast/profiles/update_spec.rb | 106 +++++++++ .../mutations/dast/profiles/update_spec.rb | 55 +++++ .../dast/profiles/update_service_spec.rb | 32 ++- 10 files changed, 618 insertions(+), 21 deletions(-) create mode 100644 ee/app/graphql/mutations/dast/profiles/update.rb create mode 100644 ee/spec/graphql/mutations/dast/profiles/update_spec.rb create mode 100644 ee/spec/requests/api/graphql/mutations/dast/profiles/update_spec.rb diff --git a/config/known_invalid_graphql_queries.yml b/config/known_invalid_graphql_queries.yml index 8c9ba5cb83a5..3dc4b10a6a86 100644 --- a/config/known_invalid_graphql_queries.yml +++ b/config/known_invalid_graphql_queries.yml @@ -2,5 +2,4 @@ filenames: - ee/app/assets/javascripts/oncall_schedules/graphql/mutations/update_oncall_schedule_rotation.mutation.graphql - ee/app/assets/javascripts/security_configuration/api_fuzzing/graphql/api_fuzzing_ci_configuration.query.graphql - - ee/app/assets/javascripts/on_demand_scans/graphql/dast_profile_update.mutation.graphql - ee/app/assets/javascripts/security_configuration/api_fuzzing/graphql/create_api_fuzzing_configuration.mutation.graphql diff --git a/doc/api/graphql/reference/gitlab_schema.graphql b/doc/api/graphql/reference/gitlab_schema.graphql index 233cc4279666..8b7af8cc5aad 100644 --- a/doc/api/graphql/reference/gitlab_schema.graphql +++ b/doc/api/graphql/reference/gitlab_schema.graphql @@ -5967,6 +5967,78 @@ type DastProfileRunPayload { pipelineUrl: String } +""" +Autogenerated input type of DastProfileUpdate +""" +input DastProfileUpdateInput { + """ + A unique identifier for the client performing the mutation. + """ + clientMutationId: String + + """ + ID of the scanner profile to be associated. + """ + dastScannerProfileId: DastScannerProfileID + + """ + ID of the site profile to be associated. + """ + dastSiteProfileId: DastSiteProfileID + + """ + The description of the profile. Defaults to an empty string. + """ + description: String = "" + + """ + The project the profile belongs to. + """ + fullPath: ID! + + """ + ID of the profile to be deleted. + """ + id: DastProfileID! + + """ + The name of the profile. + """ + name: String + + """ + Run scan using profile after update. Defaults to false. + """ + runAfterUpdate: Boolean = false +} + +""" +Autogenerated return type of DastProfileUpdate +""" +type DastProfileUpdatePayload { + """ + A unique identifier for the client performing the mutation. + """ + clientMutationId: String + + """ + The updated profile. + """ + dastProfile: DastProfile + + """ + Errors encountered during execution of the mutation. + """ + errors: [String!]! + + """ + The URL of the pipeline that was created. Requires the input argument + `runAfterUpdate` to be set to `true` when calling the mutation, otherwise no + pipeline will be created. + """ + pipelineUrl: String +} + enum DastScanTypeEnum { """ Active DAST scan. This scan will make active attacks against the target site. @@ -16652,6 +16724,7 @@ type Mutation { dastProfileCreate(input: DastProfileCreateInput!): DastProfileCreatePayload dastProfileDelete(input: DastProfileDeleteInput!): DastProfileDeletePayload dastProfileRun(input: DastProfileRunInput!): DastProfileRunPayload + dastProfileUpdate(input: DastProfileUpdateInput!): DastProfileUpdatePayload dastScannerProfileCreate(input: DastScannerProfileCreateInput!): DastScannerProfileCreatePayload dastScannerProfileDelete(input: DastScannerProfileDeleteInput!): DastScannerProfileDeletePayload dastScannerProfileUpdate(input: DastScannerProfileUpdateInput!): DastScannerProfileUpdatePayload diff --git a/doc/api/graphql/reference/gitlab_schema.json b/doc/api/graphql/reference/gitlab_schema.json index f11ed31c1a7d..5a003571b2c6 100644 --- a/doc/api/graphql/reference/gitlab_schema.json +++ b/doc/api/graphql/reference/gitlab_schema.json @@ -16233,6 +16233,186 @@ "enumValues": null, "possibleTypes": null }, + { + "kind": "INPUT_OBJECT", + "name": "DastProfileUpdateInput", + "description": "Autogenerated input type of DastProfileUpdate", + "fields": null, + "inputFields": [ + { + "name": "id", + "description": "ID of the profile to be deleted.", + "type": { + "kind": "NON_NULL", + "name": null, + "ofType": { + "kind": "SCALAR", + "name": "DastProfileID", + "ofType": null + } + }, + "defaultValue": null + }, + { + "name": "fullPath", + "description": "The project the profile belongs to.", + "type": { + "kind": "NON_NULL", + "name": null, + "ofType": { + "kind": "SCALAR", + "name": "ID", + "ofType": null + } + }, + "defaultValue": null + }, + { + "name": "name", + "description": "The name of the profile.", + "type": { + "kind": "SCALAR", + "name": "String", + "ofType": null + }, + "defaultValue": null + }, + { + "name": "description", + "description": "The description of the profile. Defaults to an empty string.", + "type": { + "kind": "SCALAR", + "name": "String", + "ofType": null + }, + "defaultValue": "\"\"" + }, + { + "name": "dastSiteProfileId", + "description": "ID of the site profile to be associated.", + "type": { + "kind": "SCALAR", + "name": "DastSiteProfileID", + "ofType": null + }, + "defaultValue": null + }, + { + "name": "dastScannerProfileId", + "description": "ID of the scanner profile to be associated.", + "type": { + "kind": "SCALAR", + "name": "DastScannerProfileID", + "ofType": null + }, + "defaultValue": null + }, + { + "name": "runAfterUpdate", + "description": "Run scan using profile after update. Defaults to false.", + "type": { + "kind": "SCALAR", + "name": "Boolean", + "ofType": null + }, + "defaultValue": "false" + }, + { + "name": "clientMutationId", + "description": "A unique identifier for the client performing the mutation.", + "type": { + "kind": "SCALAR", + "name": "String", + "ofType": null + }, + "defaultValue": null + } + ], + "interfaces": null, + "enumValues": null, + "possibleTypes": null + }, + { + "kind": "OBJECT", + "name": "DastProfileUpdatePayload", + "description": "Autogenerated return type of DastProfileUpdate", + "fields": [ + { + "name": "clientMutationId", + "description": "A unique identifier for the client performing the mutation.", + "args": [ + + ], + "type": { + "kind": "SCALAR", + "name": "String", + "ofType": null + }, + "isDeprecated": false, + "deprecationReason": null + }, + { + "name": "dastProfile", + "description": "The updated profile.", + "args": [ + + ], + "type": { + "kind": "OBJECT", + "name": "DastProfile", + "ofType": null + }, + "isDeprecated": false, + "deprecationReason": null + }, + { + "name": "errors", + "description": "Errors encountered during execution of the mutation.", + "args": [ + + ], + "type": { + "kind": "NON_NULL", + "name": null, + "ofType": { + "kind": "LIST", + "name": null, + "ofType": { + "kind": "NON_NULL", + "name": null, + "ofType": { + "kind": "SCALAR", + "name": "String", + "ofType": null + } + } + } + }, + "isDeprecated": false, + "deprecationReason": null + }, + { + "name": "pipelineUrl", + "description": "The URL of the pipeline that was created. Requires the input argument `runAfterUpdate` to be set to `true` when calling the mutation, otherwise no pipeline will be created.", + "args": [ + + ], + "type": { + "kind": "SCALAR", + "name": "String", + "ofType": null + }, + "isDeprecated": false, + "deprecationReason": null + } + ], + "inputFields": null, + "interfaces": [ + + ], + "enumValues": null, + "possibleTypes": null + }, { "kind": "ENUM", "name": "DastScanTypeEnum", @@ -46493,6 +46673,33 @@ "isDeprecated": false, "deprecationReason": null }, + { + "name": "dastProfileUpdate", + "description": null, + "args": [ + { + "name": "input", + "description": null, + "type": { + "kind": "NON_NULL", + "name": null, + "ofType": { + "kind": "INPUT_OBJECT", + "name": "DastProfileUpdateInput", + "ofType": null + } + }, + "defaultValue": null + } + ], + "type": { + "kind": "OBJECT", + "name": "DastProfileUpdatePayload", + "ofType": null + }, + "isDeprecated": false, + "deprecationReason": null + }, { "name": "dastScannerProfileCreate", "description": null, diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index ce312944cf14..82156c81b165 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -963,6 +963,17 @@ Autogenerated return type of DastProfileRun. | `errors` | String! => Array | Errors encountered during execution of the mutation. | | `pipelineUrl` | String | URL of the pipeline that was created. | +### DastProfileUpdatePayload + +Autogenerated return type of DastProfileUpdate. + +| Field | Type | Description | +| ----- | ---- | ----------- | +| `clientMutationId` | String | A unique identifier for the client performing the mutation. | +| `dastProfile` | DastProfile | The updated profile. | +| `errors` | String! => Array | Errors encountered during execution of the mutation. | +| `pipelineUrl` | String | The URL of the pipeline that was created. Requires the input argument `runAfterUpdate` to be set to `true` when calling the mutation, otherwise no pipeline will be created. | + ### DastScannerProfile Represents a DAST scanner profile. diff --git a/ee/app/graphql/ee/types/mutation_type.rb b/ee/app/graphql/ee/types/mutation_type.rb index cc111f56591a..17a0dd0ba79f 100644 --- a/ee/app/graphql/ee/types/mutation_type.rb +++ b/ee/app/graphql/ee/types/mutation_type.rb @@ -43,6 +43,7 @@ module MutationType mount_mutation ::Mutations::InstanceSecurityDashboard::RemoveProject mount_mutation ::Mutations::DastOnDemandScans::Create mount_mutation ::Mutations::Dast::Profiles::Create + mount_mutation ::Mutations::Dast::Profiles::Update mount_mutation ::Mutations::Dast::Profiles::Delete mount_mutation ::Mutations::Dast::Profiles::Run mount_mutation ::Mutations::DastSiteProfiles::Create diff --git a/ee/app/graphql/mutations/dast/profiles/update.rb b/ee/app/graphql/mutations/dast/profiles/update.rb new file mode 100644 index 000000000000..b1c8a2e45150 --- /dev/null +++ b/ee/app/graphql/mutations/dast/profiles/update.rb @@ -0,0 +1,109 @@ +# frozen_string_literal: true + +module Mutations + module Dast + module Profiles + class Update < BaseMutation + include FindsProject + + graphql_name 'DastProfileUpdate' + + ProfileID = ::Types::GlobalIDType[::Dast::Profile] + SiteProfileID = ::Types::GlobalIDType[::DastSiteProfile] + ScannerProfileID = ::Types::GlobalIDType[::DastScannerProfile] + + field :dast_profile, ::Types::Dast::ProfileType, + null: true, + description: 'The updated profile.' + + field :pipeline_url, GraphQL::STRING_TYPE, + null: true, + description: 'The URL of the pipeline that was created. Requires the input ' \ + 'argument `runAfterUpdate` to be set to `true` when calling the ' \ + 'mutation, otherwise no pipeline will be created.' + + argument :id, ProfileID, + required: true, + description: 'ID of the profile to be deleted.' + + argument :full_path, GraphQL::ID_TYPE, + required: true, + description: 'The project the profile belongs to.' + + argument :name, GraphQL::STRING_TYPE, + required: false, + description: 'The name of the profile.' + + argument :description, GraphQL::STRING_TYPE, + required: false, + description: 'The description of the profile. Defaults to an empty string.', + default_value: '' + + argument :dast_site_profile_id, SiteProfileID, + required: false, + description: 'ID of the site profile to be associated.' + + argument :dast_scanner_profile_id, ScannerProfileID, + required: false, + description: 'ID of the scanner profile to be associated.' + + argument :run_after_update, GraphQL::BOOLEAN_TYPE, + required: false, + description: 'Run scan using profile after update. Defaults to false.', + default_value: false + + authorize :create_on_demand_dast_scan + + def resolve(full_path:, id:, name:, description:, dast_site_profile_id: nil, dast_scanner_profile_id: nil, run_after_update: false) + project = authorized_find!(full_path) + raise Gitlab::Graphql::Errors::ResourceNotAvailable, 'Feature disabled' unless allowed?(project) + + dast_profile = find_dast_profile(project.id, id) + authorize!(dast_profile) + + params = { + dast_profile: dast_profile, + name: name, + description: description, + dast_site_profile_id: as_model_id(SiteProfileID, dast_site_profile_id), + dast_scanner_profile_id: as_model_id(ScannerProfileID, dast_scanner_profile_id), + run_after_update: run_after_update + }.compact + + response = ::Dast::Profiles::UpdateService.new( + container: project, + current_user: current_user, + params: params + ).execute + + { errors: response.errors, **response.payload } + end + + private + + def allowed?(project) + project.feature_available?(:security_on_demand_scans) && + Feature.enabled?(:dast_saved_scans, project, default_enabled: :yaml) + end + + def as_model_id(klass, value) + return unless value + + # TODO: remove explicit coercion once compatibility layer is removed + # See: https://gitlab.com/gitlab-org/gitlab/-/issues/257883 + klass.coerce_isolated_input(value).model_id + end + + def find_dast_profile(project_id, id) + # TODO: remove this line once the compatibility layer is removed + # See: https://gitlab.com/gitlab-org/gitlab/-/issues/257883 + id = ProfileID.coerce_isolated_input(id).model_id + + ::Dast::ProfilesFinder.new(project_id: project_id, id: id) + .execute + .first + end + end + end + end +end diff --git a/ee/app/services/dast/profiles/update_service.rb b/ee/app/services/dast/profiles/update_service.rb index 1a7afee3cad2..bbaa41820f91 100644 --- a/ee/app/services/dast/profiles/update_service.rb +++ b/ee/app/services/dast/profiles/update_service.rb @@ -7,12 +7,16 @@ class UpdateService < BaseContainerService def execute return unauthorized unless allowed? - return ServiceResponse.error(message: 'ID parameter missing') unless params[:id].present? - return ServiceResponse.error(message: 'Profile not found for given parameters') unless dast_profile + return error('Profile parameter missing') unless dast_profile + return error(dast_profile.errors.full_messages) unless dast_profile.update(dast_profile_params) - return ServiceResponse.error(message: dast_profile.errors.full_messages) unless dast_profile.update(dast_profile_params) + return success(dast_profile: dast_profile, pipeline_url: nil) unless params[:run_after_update] - ServiceResponse.success(payload: dast_profile) + response = create_scan(dast_profile) + + return response if response.error? + + success(dast_profile: dast_profile, pipeline_url: response.payload.fetch(:pipeline_url)) end private @@ -23,24 +27,38 @@ def allowed? can?(current_user, :create_on_demand_dast_scan, container) end + def error(message, opts = {}) + ServiceResponse.error(message: message, **opts) + end + + def success(payload) + ServiceResponse.success(payload: payload) + end + def unauthorized - ServiceResponse.error( - message: 'You are not authorized to update this profile', - http_status: 403 - ) + error('You are not authorized to update this profile', http_status: 403) end def dast_profile - strong_memoize(:dast_profile) do - Dast::ProfilesFinder.new(project_id: container.id, id: params[:id]) - .execute - .first - end + params[:dast_profile] end def dast_profile_params params.slice(:dast_site_profile_id, :dast_scanner_profile_id, :name, :description) end + + def create_scan(dast_profile) + params = { + dast_site_profile: dast_profile.dast_site_profile, + dast_scanner_profile: dast_profile.dast_scanner_profile + } + + ::DastOnDemandScans::CreateService.new( + container: container, + current_user: current_user, + params: params + ).execute + end end end end diff --git a/ee/spec/graphql/mutations/dast/profiles/update_spec.rb b/ee/spec/graphql/mutations/dast/profiles/update_spec.rb new file mode 100644 index 000000000000..cef77057715d --- /dev/null +++ b/ee/spec/graphql/mutations/dast/profiles/update_spec.rb @@ -0,0 +1,106 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Mutations::Dast::Profiles::Update do + include GraphqlHelpers + + let_it_be(:project) { create(:project) } + let_it_be(:user) { create(:user) } + let_it_be(:dast_profile, reload: true) { create(:dast_profile, project: project) } + + let(:dast_profile_gid) { dast_profile.to_global_id } + + let(:params) do + { + id: dast_profile_gid, + dast_site_profile_id: global_id_of(create(:dast_site_profile, project: project)), + dast_scanner_profile_id: global_id_of(create(:dast_scanner_profile, project: project)), + name: SecureRandom.hex, + description: SecureRandom.hex + } + end + + subject(:mutation) { described_class.new(object: nil, context: { current_user: user }, field: nil) } + + before do + stub_licensed_features(security_on_demand_scans: true) + end + + specify { expect(described_class).to require_graphql_authorizations(:create_on_demand_dast_scan) } + + describe '#resolve' do + subject { mutation.resolve(**params.merge(full_path: project.full_path)) } + + shared_examples 'an unrecoverable failure' do |parameter| + it 'raises an exception' do + expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable) + end + end + + context 'when the feature is licensed' do + context 'when the project does not exist' do + before do + allow_next_instance_of(ProjectsFinder) do |finder| + allow(finder).to receive(:execute).and_return(nil) + end + end + + it_behaves_like 'an unrecoverable failure' + end + + context 'when the user cannot read the project' do + it_behaves_like 'an unrecoverable failure' + end + + context 'when the user can update a DAST profile' do + before do + project.add_developer(user) + end + + it 'returns the profile' do + expect(subject[:dast_profile]).to be_a(Dast::Profile) + end + + it 'updates the profile' do + subject + + updated_dast_profile = dast_profile.reload + + aggregate_failures do + expect(global_id_of(updated_dast_profile.dast_site_profile)).to eq(params[:dast_site_profile_id]) + expect(global_id_of(updated_dast_profile.dast_scanner_profile)).to eq(params[:dast_scanner_profile_id]) + expect(updated_dast_profile.name).to eq(params[:name]) + expect(updated_dast_profile.description).to eq(params[:description]) + end + end + + context 'when the dast_profile does not exist' do + let(:dast_profile_gid) { Gitlab::GlobalId.build(nil, model_name: 'Dast::Profile', id: 'does_not_exist') } + + it_behaves_like 'an unrecoverable failure' + end + + context 'when updating fails' do + it 'returns an error' do + allow_next_instance_of(::Dast::Profiles::UpdateService) do |service| + allow(service).to receive(:execute).and_return( + ServiceResponse.error(message: 'Profile failed to update') + ) + end + + expect(subject[:errors]).to include('Profile failed to update') + end + end + + context 'when the feature is not enabled' do + before do + stub_feature_flags(dast_saved_scans: false) + end + + it_behaves_like 'an unrecoverable failure' + end + end + end + end +end diff --git a/ee/spec/requests/api/graphql/mutations/dast/profiles/update_spec.rb b/ee/spec/requests/api/graphql/mutations/dast/profiles/update_spec.rb new file mode 100644 index 000000000000..985b8970772b --- /dev/null +++ b/ee/spec/requests/api/graphql/mutations/dast/profiles/update_spec.rb @@ -0,0 +1,55 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'Updating a DAST Profile' do + include GraphqlHelpers + + let!(:dast_profile) { create(:dast_profile, project: project) } + + let(:mutation_name) { :dast_profile_update } + + let(:mutation) do + graphql_mutation( + mutation_name, + full_path: project.full_path, + id: global_id_of(dast_profile), + name: 'updated dast_profiles.name', + run_after_update: true + ) + end + + it_behaves_like 'an on-demand scan mutation when user cannot run an on-demand scan' + + it_behaves_like 'an on-demand scan mutation when user can run an on-demand scan' do + it 'returns a non-nil dastProfile' do + subject + + expect(mutation_response['dastProfile']).not_to be_nil + end + + it 'returns a non-nil pipelineUrl' do + subject + + expect(mutation_response['pipelineUrl']).not_to be_nil + end + + it 'updates the dast_profile' do + expect { subject }.to change { dast_profile.reload.name }.to('updated dast_profiles.name') + end + + context 'when updating fails' do + it 'returns an error' do + allow_next_instance_of(::Dast::Profiles::UpdateService) do |service| + allow(service).to receive(:execute).and_return( + ServiceResponse.error(message: 'Profile failed to update') + ) + end + + subject + + expect(mutation_response['errors']).to include('Profile failed to update') + end + end + end +end diff --git a/ee/spec/services/dast/profiles/update_service_spec.rb b/ee/spec/services/dast/profiles/update_service_spec.rb index a78ff8e33be3..bb4d4070b2cc 100644 --- a/ee/spec/services/dast/profiles/update_service_spec.rb +++ b/ee/spec/services/dast/profiles/update_service_spec.rb @@ -3,15 +3,15 @@ require 'spec_helper' RSpec.describe Dast::Profiles::UpdateService do - let_it_be(:project) { create(:project) } + let_it_be(:project) { create(:project, :repository) } let_it_be(:user) { create(:user) } - let_it_be(:dast_profile) { create(:dast_profile, project: project) } + let_it_be(:dast_profile, reload: true) { create(:dast_profile, project: project) } let_it_be(:dast_site_profile) { create(:dast_site_profile, project: project) } let_it_be(:dast_scanner_profile) { create(:dast_scanner_profile, project: project) } - let_it_be(:params) do + let(:default_params) do { - id: dast_profile.id, + dast_profile: dast_profile, dast_site_profile_id: dast_site_profile.id, dast_scanner_profile_id: dast_scanner_profile.id, name: SecureRandom.hex, @@ -19,6 +19,8 @@ } end + let(:params) { default_params } + subject do described_class.new( container: project, @@ -81,7 +83,7 @@ end it 'updates the dast_profile' do - updated_dast_profile = subject.payload.reload + updated_dast_profile = subject.payload[:dast_profile].reload aggregate_failures do expect(updated_dast_profile.dast_site_profile.id).to eq(params[:dast_site_profile_id]) @@ -91,13 +93,29 @@ end end - context 'when id param is missing' do + context 'when param run_after_update: true' do + let(:params) { default_params.merge(run_after_update: true) } + + it 'calls DastOnDemandScans::CreateService' do + params = { dast_site_profile: dast_site_profile, dast_scanner_profile: dast_scanner_profile } + + expect(DastOnDemandScans::CreateService).to receive(:new).with(hash_including(params: params)).and_call_original + + subject + end + + it 'creates a ci_pipeline' do + expect { subject }.to change { Ci::Pipeline.count }.by(1) + end + end + + context 'when dast_profile param is missing' do let(:params) { {} } it 'communicates failure' do aggregate_failures do expect(subject.status).to eq(:error) - expect(subject.message).to eq('ID parameter missing') + expect(subject.message).to eq('Profile parameter missing') end end end -- GitLab