Merge branch 'philipcunningham-use-graphql-for-dast-profile-in-controller-300995' into 'master'

Use GraphQL to prefetch data for DastProfile#edit

See merge request gitlab-org/gitlab!67809

doc/api/graphql/reference/index.md

@@ -12025,6 +12025,18 @@ four standard [pagination arguments](#connection-pagination-arguments):
 | <a id="projectcontainerrepositoriesname"></a>`name` | [`String`](#string) | Filter the container repositories by their name. |
 | <a id="projectcontainerrepositoriessort"></a>`sort` | [`ContainerRepositorySort`](#containerrepositorysort) | Sort container repositories by this criteria. |
 
+##### `Project.dastProfile`
+
+DAST Profile associated with the project.
+
+Returns [`DastProfile`](#dastprofile).
+
+###### Arguments
+
+| Name | Type | Description |
+| ---- | ---- | ----------- |
+| <a id="projectdastprofileid"></a>`id` | [`DastProfileID!`](#dastprofileid) | ID of the DAST Profile. |
+
 ##### `Project.dastSiteProfile`
 
 DAST Site Profile associated with the project.

ee/app/assets/javascripts/on_demand_scans/components/on_demand_scans_form.vue

@@ -143,8 +143,8 @@ export default {
       scannerProfiles: [],
       siteProfiles: [],
       selectedBranch: this.dastScan?.branch?.name ?? this.defaultBranch,
-      selectedScannerProfileId: this.dastScan?.scannerProfileId || null,
-      selectedSiteProfileId: this.dastScan?.siteProfileId || null,
+      selectedScannerProfileId: this.dastScan?.dastScannerProfile.id || null,
+      selectedSiteProfileId: this.dastScan?.dastSiteProfile.id || null,
       loading: false,
       errorType: null,
       errors: [],

ee/app/controllers/projects/on_demand_scans_controller.rb

@@ -3,6 +3,7 @@
 module Projects
   class OnDemandScansController < Projects::ApplicationController
     include SecurityAndCompliancePermissions
+    include API::Helpers::GraphqlHelpers
 
     before_action :authorize_read_on_demand_scans!, only: :index
     before_action :authorize_create_on_demand_dast_scan!, only: [:new, :edit]
@@ -16,16 +17,30 @@ module Projects
     end
 
     def edit
-      dast_profile = Dast::ProfilesFinder.new(project_id: @project.id, id: params[:id]).execute.first! # rubocop: disable CodeReuse/ActiveRecord
-
-      @dast_profile = {
-        id: dast_profile.to_global_id.to_s,
-        name: dast_profile.name,
-        description: dast_profile.description,
-        branch: { name: dast_profile.branch_name },
-        site_profile_id: DastSiteProfile.new(id: dast_profile.dast_site_profile_id).to_global_id.to_s,
-        scanner_profile_id: DastScannerProfile.new(id: dast_profile.dast_scanner_profile_id).to_global_id.to_s
+      global_id = Gitlab::GlobalId.as_global_id(params[:id], model_name: 'Dast::Profile')
+
+      query = %(
+          {
+            project(fullPath: "#{project.full_path}") {
+              dastProfile(id: "#{global_id}") {
+                id
+                name
+                description
+                branch { name }
+                dastSiteProfile { id }
+                dastScannerProfile { id }
+              }
+            }
           }
+        )
+
+      @dast_profile = run_graphql!(
+        query: query,
+        context: { current_user: current_user },
+        transform: -> (result) { result.dig('data', 'project', 'dastProfile') }
+      )
+
+      return render_404 unless @dast_profile
     end
   end
 end

ee/app/graphql/ee/types/project_type.rb

@@ -60,6 +60,12 @@ module EE
               description: 'Find iteration cadences.',
               resolver: ::Resolvers::Iterations::CadencesResolver
 
+        field :dast_profile,
+              ::Types::Dast::ProfileType,
+              null: true,
+              resolver: ::Resolvers::AppSec::Dast::ProfileResolver.single,
+              description: 'DAST Profile associated with the project.'
+
         field :dast_profiles,
               ::Types::Dast::ProfileType.connection_type,
               null: true,

ee/app/graphql/resolvers/app_sec/dast/profile_resolver.rb

@@ -10,6 +10,12 @@ module Resolvers
 
         type ::Types::Dast::ProfileType.connection_type, null: true
 
+        when_single do
+          argument :id, ::Types::GlobalIDType[::Dast::Profile],
+                   required: true,
+                   description: 'ID of the DAST Profile.'
+        end
+
         def resolve_with_lookahead(**args)
           apply_lookahead(find_dast_profiles(args))
         end
@@ -24,7 +30,15 @@ module Resolvers
         end
 
         def find_dast_profiles(args)
-          ::Dast::ProfilesFinder.new(project_id: project.id).execute
+          params = { project_id: project.id }
+
+          if args[:id]
+            # TODO: remove this coercion when the compatibility layer is removed
+            # See: https://gitlab.com/gitlab-org/gitlab/-/issues/257883
+            params[:id] = ::Types::GlobalIDType[::Dast::Profile].coerce_isolated_input(args[:id]).model_id
+          end
+
+          ::Dast::ProfilesFinder.new(params).execute
         end
       end
     end

ee/spec/frontend/on_demand_scans/components/on_demand_scans_form_spec.js

@@ -23,6 +23,7 @@ import { scannerProfiles, siteProfiles } from '../mocks/mock_data';
 const helpPagePath = '/application_security/dast/index#on-demand-scans';
 const projectPath = 'group/project';
 const defaultBranch = 'main';
+const selectedBranch = 'some-other-branch';
 const profilesLibraryPath = '/security/configuration/dast_scans';
 const scannerProfilesLibraryPath = '/security/configuration/dast_scans#scanner-profiles';
 const siteProfilesLibraryPath = '/security/configuration/dast_scans#site-profiles';
@@ -44,8 +45,8 @@ const dastScan = {
   branch: { name: 'dev' },
   name: 'My daily scan',
   description: 'Tests for SQL injections',
-  scannerProfileId: passiveScannerProfile.id,
-  siteProfileId: validatedSiteProfile.id,
+  dastScannerProfile: { id: passiveScannerProfile.id },
+  dastSiteProfile: { id: validatedSiteProfile.id },
 };
 
 useLocalStorageSpy();
@@ -83,9 +84,14 @@ describe('OnDemandScansForm', () => {
   const findCancelButton = () => findByTestId('on-demand-scan-cancel-button');
   const findProfileSummary = () => findByTestId('selected-profile-summary');
 
+  const hasSiteProfileAttributes = () => {
+    expect(findScannerProfilesSelector().attributes('value')).toBe(dastScan.dastScannerProfile.id);
+    expect(findSiteProfilesSelector().attributes('value')).toBe(dastScan.dastSiteProfile.id);
+  };
+
   const setValidFormData = () => {
     findNameInput().vm.$emit('input', 'My daily scan');
-    findBranchInput().vm.$emit('input', 'some-other-branch');
+    findBranchInput().vm.$emit('input', selectedBranch);
     findScannerProfilesSelector().vm.$emit('input', passiveScannerProfile.id);
     findSiteProfilesSelector().vm.$emit('input', nonValidatedSiteProfile.id);
     return wrapper.vm.$nextTick();
@@ -230,6 +236,26 @@ describe('OnDemandScansForm', () => {
   });
 
   describe('when editing an existing scan', () => {
+    describe('when the branch is not present', () => {
+      /**
+       * It is possible for pre-fetched data not to have a branch, so we must
+       * handle this path.
+       */
+      beforeEach(() => {
+        createShallowComponent({
+          propsData: {
+            ...dastScan,
+            branch: null,
+          },
+        });
+      });
+
+      it('sets the branch to the default', () => {
+        expect(findBranchInput().props('value')).toBe(defaultBranch);
+      });
+    });
+
+    describe('when the branch is present', () => {
       beforeEach(() => {
         createShallowComponent({
           propsData: {
@@ -246,8 +272,8 @@ describe('OnDemandScansForm', () => {
         expect(findNameInput().attributes('value')).toBe(dastScan.name);
         expect(findBranchInput().props('value')).toBe(dastScan.branch.name);
         expect(findDescriptionInput().attributes('value')).toBe(dastScan.description);
-      expect(findScannerProfilesSelector().attributes('value')).toBe(dastScan.scannerProfileId);
-      expect(findSiteProfilesSelector().attributes('value')).toBe(dastScan.siteProfileId);
+        hasSiteProfileAttributes();
+      });
     });
   });
 
@@ -264,7 +290,7 @@ describe('OnDemandScansForm', () => {
             name: 'My daily scan',
             selectedScannerProfileId: 'gid://gitlab/DastScannerProfile/1',
             selectedSiteProfileId: 'gid://gitlab/DastSiteProfile/1',
-            selectedBranch: 'some-other-branch',
+            selectedBranch,
           }),
         ],
       ]);
@@ -276,8 +302,8 @@ describe('OnDemandScansForm', () => {
         JSON.stringify({
           name: dastScan.name,
           description: dastScan.description,
-          selectedScannerProfileId: dastScan.scannerProfileId,
-          selectedSiteProfileId: dastScan.siteProfileId,
+          selectedScannerProfileId: dastScan.dastScannerProfile.id,
+          selectedSiteProfileId: dastScan.dastSiteProfile.id,
         }),
       );
 
@@ -286,8 +312,7 @@ describe('OnDemandScansForm', () => {
 
       expect(findNameInput().attributes('value')).toBe(dastScan.name);
       expect(findDescriptionInput().attributes('value')).toBe(dastScan.description);
-      expect(findScannerProfilesSelector().attributes('value')).toBe(dastScan.scannerProfileId);
-      expect(findSiteProfilesSelector().attributes('value')).toBe(dastScan.siteProfileId);
+      hasSiteProfileAttributes();
     });
   });
 
@@ -345,7 +370,7 @@ describe('OnDemandScansForm', () => {
               variables: {
                 input: {
                   name: 'My daily scan',
-                  branchName: 'some-other-branch',
+                  branchName: selectedBranch,
                   dastScannerProfileId: passiveScannerProfile.id,
                   dastSiteProfileId: nonValidatedSiteProfile.id,
                   fullPath: projectPath,
@@ -384,7 +409,7 @@ describe('OnDemandScansForm', () => {
                 input: {
                   id: 1,
                   name: 'My daily scan',
-                  branchName: 'some-other-branch',
+                  branchName: selectedBranch,
                   description: 'Tests for SQL injections',
                   dastScannerProfileId: passiveScannerProfile.id,
                   dastSiteProfileId: nonValidatedSiteProfile.id,
@@ -602,16 +627,15 @@ describe('OnDemandScansForm', () => {
       localStorage.setItem(
         LOCAL_STORAGE_KEY,
         JSON.stringify({
-          selectedScannerProfileId: dastScan.scannerProfileId,
-          selectedSiteProfileId: dastScan.siteProfileId,
+          selectedScannerProfileId: dastScan.dastScannerProfile.id,
+          selectedSiteProfileId: dastScan.dastSiteProfile.id,
         }),
       );
 
       createShallowComponent();
       await wrapper.vm.$nextTick();
 
-      expect(findScannerProfilesSelector().attributes('value')).toBe(dastScan.scannerProfileId);
-      expect(findSiteProfilesSelector().attributes('value')).toBe(dastScan.siteProfileId);
+      hasSiteProfileAttributes();
     });
   });
 

ee/spec/graphql/resolvers/app_sec/dast/profile_resolver_spec.rb

@@ -20,6 +20,22 @@ RSpec.describe Resolvers::AppSec::Dast::ProfileResolver do
     expect(described_class).to have_nullable_graphql_type(Types::Dast::ProfileType.connection_type)
   end
 
+  context 'when resolving a single DAST profile' do
+    subject { sync(dast_profile(id: gid)) }
+
+    context 'when the DAST profile exists' do
+      let(:gid) { dast_profile1.to_global_id }
+
+      it { is_expected.to eq dast_profile1 }
+    end
+
+    context 'when the DAST profile does not exist' do
+      let(:gid) { Gitlab::GlobalId.as_global_id(non_existing_record_id, model_name: 'Dast::Profile') }
+
+      it { is_expected.to be_nil }
+    end
+  end
+
   context 'when resolving multiple DAST profiles' do
     subject { sync(dast_profiles) }
 
@@ -45,4 +61,8 @@ RSpec.describe Resolvers::AppSec::Dast::ProfileResolver do
   def dast_profiles
     resolve(described_class, obj: project, ctx: { current_user: current_user })
   end
+
+  def dast_profile(id:)
+    resolve(described_class.single, obj: project, args: { id: id }, ctx: { current_user: current_user })
+  end
 end

ee/spec/requests/api/graphql/project/dast_profile_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'Query.project(fullPath).dastProfile' do
+  include GraphqlHelpers
+
+  let_it_be(:project) { create(:project, :repository) }
+  let_it_be(:current_user) { create(:user) }
+  let_it_be(:dast_profile) { create(:dast_profile, project: project) }
+
+  let(:query) do
+    fields = all_graphql_fields_for('DastProfile')
+
+    graphql_query_for(
+      :project,
+      { full_path: project.full_path },
+      query_graphql_field(:dast_profile, { id: global_id_of(dast_profile) }, fields)
+    )
+  end
+
+  subject do
+    post_graphql(query, current_user: current_user)
+  end
+
+  before do
+    stub_licensed_features(security_on_demand_scans: true)
+  end
+
+  context 'when a user does not have access to the project' do
+    it 'returns a null project' do
+      subject
+
+      expect(graphql_data_at(:project)).to be_nil
+    end
+  end
+
+  context 'when a user does not have access to the dast_profile' do
+    before do
+      project.add_guest(current_user)
+    end
+
+    it 'returns a null dast_profile' do
+      subject
+
+      expect(graphql_data_at(:project, :dast_profile)).to be_nil
+    end
+  end
+
+  context 'when a user has access to the dast_profile' do
+    before do
+      project.add_developer(current_user)
+    end
+
+    it 'returns a dast_profile' do
+      subject
+
+      expect(graphql_data_at(:project, :dast_profile, :id)).to eq(dast_profile.to_global_id.to_s)
+    end
+
+    context 'when on demand scan licensed feature is not available' do
+      before do
+        stub_licensed_features(security_on_demand_scans: false)
+      end
+
+      it 'returns a null dast_profile' do
+        subject
+
+        expect(graphql_data_at(:project, :dast_profile)).to be_nil
+      end
+    end
+  end
+end

ee/spec/requests/projects/on_demand_scans_controller_spec.rb

@@ -5,7 +5,7 @@ require 'spec_helper'
 RSpec.describe Projects::OnDemandScansController, type: :request do
   include GraphqlHelpers
 
-  let_it_be(:project) { create(:project) }
+  let_it_be(:project) { create(:project, :repository) }
 
   let(:user) { create(:user) }
 
@@ -82,7 +82,7 @@ RSpec.describe Projects::OnDemandScansController, type: :request do
   end
 
   describe 'GET #edit' do
-    let_it_be(:dast_profile) { create(:dast_profile, project: project) }
+    let_it_be(:dast_profile) { create(:dast_profile, project: project, branch_name: project.default_branch_or_main) }
 
     let(:dast_profile_id) { dast_profile.id }
     let(:edit_path) { edit_project_on_demand_scan_path(project, id: dast_profile_id) }
@@ -108,9 +108,9 @@ RSpec.describe Projects::OnDemandScansController, type: :request do
             id: global_id_of(dast_profile),
             name: dast_profile.name,
             description: dast_profile.description,
-            branch: { name: dast_profile.branch_name },
-            site_profile_id: global_id_of(DastSiteProfile.new(id: dast_profile.dast_site_profile_id)),
-            scanner_profile_id: global_id_of(DastScannerProfile.new(id: dast_profile.dast_scanner_profile_id))
+            branch: { name: project.default_branch_or_main },
+            dastSiteProfile: { id: global_id_of(DastSiteProfile.new(id: dast_profile.dast_site_profile_id)) },
+            dastScannerProfile: { id: global_id_of(DastScannerProfile.new(id: dast_profile.dast_scanner_profile_id)) }
           }.to_json
 
           on_demand_div = Nokogiri::HTML.parse(response.body).at_css('div#js-on-demand-scans-app')