Merge branch 'mc_rocha-add-valid-owner-schedule-type-335662' into 'master'

Add owner_valid to Dast::ProfileScheduleType

See merge request gitlab-org/gitlab!71214

app/models/preloaders/users_max_access_level_in_projects_preloader.rb

+# frozen_string_literal: true
+
+module Preloaders
+  # This class preloads the max access level (role) for the users within the given projects and
+  # stores the values in requests store via the ProjectTeam class.
+  class UsersMaxAccessLevelInProjectsPreloader
+    def initialize(projects:, users:)
+      @projects = projects
+      @users = users
+    end
+
+    def execute
+      return unless @projects.present? && @users.present?
+
+      access_levels.each do |(project_id, user_id), access_level|
+        project = projects_by_id[project_id]
+
+        project.team.write_member_access_for_user_id(user_id, access_level)
+      end
+    end
+
+    private
+
+    def access_levels
+      ProjectAuthorization
+        .where(project_id: project_ids, user_id: user_ids)
+        .group(:project_id, :user_id)
+        .maximum(:access_level)
+    end
+
+    # Use reselect to override the existing select to prevent
+    # the error `subquery has too many columns`
+    # NotificationsController passes in an Array so we need to check the type
+    def project_ids
+      @projects.is_a?(ActiveRecord::Relation) ? @projects.reselect(:id) : @projects
+    end
+
+    def user_ids
+      @users.is_a?(ActiveRecord::Relation) ? @users.reselect(:id) : @users
+    end
+
+    def projects_by_id
+      @projects_by_id ||= @projects.index_by(&:id)
+    end
+  end
+end

doc/api/graphql/reference/index.md

@@ -9675,6 +9675,7 @@ Represents a DAST profile schedule.
 | <a id="dastprofileschedulecadence"></a>`cadence` | [`DastProfileCadence`](#dastprofilecadence) | Cadence of the DAST profile schedule. |
 | <a id="dastprofilescheduleid"></a>`id` | [`DastProfileScheduleID!`](#dastprofilescheduleid) | ID of the DAST profile schedule. |
 | <a id="dastprofileschedulenextrunat"></a>`nextRunAt` | [`Time`](#time) | Next run time of the DAST profile schedule in the given timezone. |
+| <a id="dastprofilescheduleownervalid"></a>`ownerValid` | [`Boolean`](#boolean) | Status of the current owner of the DAST profile schedule. |
 | <a id="dastprofileschedulestartsat"></a>`startsAt` | [`Time`](#time) | Start time of the DAST profile schedule in the given timezone. |
 | <a id="dastprofilescheduletimezone"></a>`timezone` | [`String`](#string) | Time zone of the start time of the DAST profile schedule. |
 

ee/app/finders/dast/profiles_finder.rb

@@ -21,11 +21,9 @@ module Dast
 
     attr_reader :params
 
-    # rubocop: disable CodeReuse/ActiveRecord
     def default_relation
-      Dast::Profile.limit(100)
+      Dast::Profile.limit(100).with_schedule_owner
     end
-    # rubocop: enable CodeReuse/ActiveRecord
 
     def by_id(relation)
       return relation if params[:id].nil?

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

@@ -74,6 +74,8 @@ module EE
         field :dast_profiles,
               ::Types::Dast::ProfileType.connection_type,
               null: true,
+              extras: [:lookahead],
+              late_extensions: [::Gitlab::Graphql::Project::DastProfileConnectionExtension],
               resolver: ::Resolvers::AppSec::Dast::ProfileResolver,
               description: 'DAST Profiles associated with the project.'
 

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

@@ -21,19 +21,35 @@ module Resolvers
         end
 
         def resolve_with_lookahead(**args)
-          apply_lookahead(find_dast_profiles(args))
+          profiles = apply_lookahead(find_dast_profiles(args)).to_a
+          profiles.each do |profile|
+            profile.project = project
+
+            if node_selection&.selects?(:dast_profile_schedule) && profile.dast_profile_schedule
+              profile.dast_profile_schedule.project = project
+            end
           end
+          context[:project_dast_profiles] ||= []
+          context[:project_dast_profiles] += profiles
 
-        DAST_PROFILE_PRELOAD = {
-          dast_site_profile: [{ dast_site_profile: [:dast_site, :secret_variables] }],
-          dast_scanner_profile: [:dast_scanner_profile],
-          dast_profile_schedule: [:dast_profile_schedule]
-        }.freeze
+          # If we are querying a single profile, we should return the profile
+          # because the late_extensions won't be called
+          return profiles if single?
+
+          # We want to avoid resolving any fields on these profiles until all
+          # leaves at the name level have been resolved.
+          # See: DastProfileConnectionExtension for where this batch is consumed.
+          ::Gitlab::Graphql::Lazy.new { profiles }
+        end
 
         private
 
         def preloads
-          DAST_PROFILE_PRELOAD
+          {
+            dast_site_profile: [{ dast_site_profile: [:dast_site, :secret_variables] }],
+            dast_scanner_profile: [:dast_scanner_profile],
+            dast_profile_schedule: [{ dast_profile_schedule: [:owner] }]
+          }
         end
 
         def find_dast_profiles(args)

ee/app/graphql/types/dast/profile_schedule_type.rb

@@ -26,6 +26,9 @@ module Types
       field :next_run_at, Types::TimeType, null: true,
             description: 'Next run time of the DAST profile schedule in the given timezone.'
 
+      field :owner_valid, GraphQL::Types::Boolean, null: true,
+            description: 'Status of the current owner of the DAST profile schedule.', method: :owner_valid?
+
       def starts_at
         return unless object.starts_at && object.timezone
 

ee/app/models/dast/profile.rb

@@ -31,6 +31,10 @@ module Dast
       has_dast_profile_schedule ? joins(:dast_profile_schedule) : where.missing(:dast_profile_schedule)
     end
 
+    scope :with_schedule_owner, -> do
+      eager_load(dast_profile_schedule: [:owner])
+    end
+
     delegate :secret_ci_variables, to: :dast_site_profile
 
     sanitizes! :name, :description

ee/app/models/ee/project.rb

@@ -84,6 +84,7 @@ module EE
       has_many :dast_site_profiles
       has_many :dast_site_tokens
       has_many :dast_sites
+      has_many :dast_profiles, class_name: 'Dast::Profile'
 
       has_many :protected_environments
       has_many :software_license_policies, inverse_of: :project, class_name: 'SoftwareLicensePolicy'

ee/spec/graphql/types/dast/profile_schedule_type_spec.rb

@@ -5,7 +5,7 @@ require 'spec_helper'
 RSpec.describe GitlabSchema.types['DastProfileSchedule'] do
   include GraphqlHelpers
 
-  let_it_be(:fields) { %i[id active startsAt timezone nextRunAt cadence] }
+  let_it_be(:fields) { %i[id active startsAt timezone nextRunAt cadence ownerValid] }
   let_it_be(:project) { create(:project) }
   let_it_be(:user) { create(:user, developer_projects: [project]) }
   let_it_be(:object) { create(:dast_profile_schedule, project: project, owner: user) }
@@ -29,4 +29,10 @@ RSpec.describe GitlabSchema.types['DastProfileSchedule'] do
       expect(resolve_field(:next_run_at, object, current_user: user)).to eq(object.next_run_at.in_time_zone(object.timezone))
     end
   end
+
+  describe 'ownerValid' do
+    it 'returns if the owner is valid' do
+      expect(resolve_field(:owner_valid, object, current_user: user)).to eq(object.owner_valid?)
+    end
+  end
 end

ee/spec/models/dast/profile_schedule_spec.rb

@@ -254,4 +254,34 @@ RSpec.describe Dast::ProfileSchedule, type: :model do
       is_expected.not_to include(inactive_dast_profile_schedule)
     end
   end
+
+  describe '#owner_valid?' do
+    let_it_be(:owner) { create(:user) }
+
+    let(:dast_profile_schedule_with_owner) { create(:dast_profile_schedule, project: project, user_id: owner.id) }
+
+    subject { dast_profile_schedule_with_owner }
+
+    context 'when the feature is enabled' do
+      before do
+        stub_licensed_features(security_on_demand_scans: true)
+      end
+
+      context 'when the scheduler owner is not null and has the ability to create_on_demand_dast_scan' do
+        before do
+          project.add_developer(owner)
+        end
+
+        it { is_expected.to be_owner_valid }
+      end
+
+      context 'when the user_id is nil' do
+        let(:dast_profile_schedule_nil_owner) { create(:dast_profile_schedule, project: project, user_id: nil) }
+
+        subject { dast_profile_schedule_nil_owner }
+
+        it { is_expected.not_to be_owner_valid }
+      end
+    end
+  end
 end

ee/spec/requests/api/graphql/group/dast_profile_schedule_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'Query.group(fullPath).projects.dastProfiles.dastProfileSchedule' do
+  include GraphqlHelpers
+
+  let_it_be(:group) { create(:group) }
+  let_it_be(:current_user) { create(:user) }
+
+  let_it_be(:project) { create(:project, :repository, group: group) }
+  let_it_be(:dast_profile) { create(:dast_profile, project: project) }
+  let_it_be(:dast_profile_schedule) { create(:dast_profile_schedule, project: project, dast_profile: dast_profile, owner: current_user) }
+
+  let_it_be(:project2) { create(:project, :repository, group: group) }
+  let_it_be(:dast_profile2) { create(:dast_profile, project: project2) }
+  let_it_be(:dast_profile_schedule2) { create(:dast_profile_schedule, project: project2, dast_profile: dast_profile2, owner: current_user) }
+
+  let(:query) do
+    %(
+      query {
+        group(fullPath: "#{group.full_path}") {
+          projects {
+            edges {
+              node {
+                __typename
+                id
+                dastProfiles {
+                  edges {
+                    node {
+                      dastProfileSchedule {
+                        #{all_graphql_fields_for('DastProfileSchedule')}
+                      }
+                    }
+                  }
+                }
+              }
+            }
+          }
+        }
+      }
+    )
+  end
+
+  def run_query(query)
+    run_with_clean_state(query,
+                         context: { current_user: current_user },
+                         variables: {})
+  end
+
+  before do
+    stub_licensed_features(security_on_demand_scans: true)
+  end
+
+  context 'when a user has access to dast_profile_schedule' do
+    let_it_be(:plan_limits) { create(:plan_limits, :default_plan) }
+
+    before do
+      project.add_developer(current_user)
+    end
+
+    it 'returns a dast_profile_schedule' do
+      r = run_query(query).to_h
+      schedule = graphql_dig_at(r, :data,
+                                   :group,
+                                   :projects, :edges, :node,
+                                   :dast_profiles, :edges, :node,
+                                   :dast_profile_schedule, 0)
+
+      expect(schedule).to include('ownerValid' => eq(true))
+    end
+
+    it_behaves_like 'query dastProfiles.dastProfileSchedule shared examples', :avoids_n_plus_1_queries, create_new_project: true
+  end
+end

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'Query.project(fullPath).dastProfiles.dastProfileSchedule' do
+  include GraphqlHelpers
+  let_it_be(:plan_limits) { create(:plan_limits, :default_plan) }
+  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_it_be(:dast_profile_schedule) { create(:dast_profile_schedule, project: project, dast_profile: dast_profile, owner: current_user) }
+
+  let(:query) do
+    %(
+      query {
+        project(fullPath: "#{project.full_path}") {
+          dastProfiles {
+            edges {
+              node {
+                  dastProfileSchedule { #{all_graphql_fields_for('DastProfileSchedule')} }
+              }
+            }
+          }
+        }
+      }
+    )
+  end
+
+  def run_query(query)
+    run_with_clean_state(query,
+                         context: { current_user: current_user },
+                         variables: {})
+  end
+
+  subject { post_graphql(query, current_user: current_user) }
+
+  let(:project_data) { graphql_data_at(:project) }
+  let(:dast_profile_data) { graphql_data_at(:project, :dast_profiles, :edges) }
+
+  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(project_data).to be_nil
+    end
+  end
+
+  context 'when a user does not have access to dast_profile' do
+    before do
+      project.add_guest(current_user)
+    end
+
+    it 'returns an empty dast_profile' do
+      subject
+
+      expect(dast_profile_data).to be_empty
+    end
+  end
+
+  context 'when a user has access to dast_profile_schedule' do
+    before do
+      project.add_developer(current_user)
+    end
+
+    it 'returns a dast_profile_schedule' do
+      subject
+
+      expect(dast_profile_data[0].dig('node', 'dastProfileSchedule', 'ownerValid')).to eq(true)
+    end
+
+    it_behaves_like 'query dastProfiles.dastProfileSchedule shared examples', :avoids_n_plus_1_queries
+  end
+end

ee/spec/support/shared_examples/graphql/dast/dast_profile_schedule_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'query dastProfiles.dastProfileSchedule shared examples' do |create_new_project: false|
+  it 'avoids N+1 queries' do
+    profile_project = if create_new_project
+                        create(:project, :repository, group: group)
+                      else
+                        project
+                      end
+
+    profile_project.add_developer(current_user)
+    extra_users = create_list(:user, 6)
+    extra_users.each do |user|
+      profile_project.add_developer(user)
+    end
+
+    control = ActiveRecord::QueryRecorder.new(query_recorder_debug: true) do
+      run_query(query)
+    end
+
+    extra_users.each do |extra_user|
+      create(:dast_profile_schedule,
+             project: profile_project,
+             dast_profile: create(:dast_profile, project: profile_project), owner: extra_user)
+    end
+
+    expect { run_query(query) }.not_to exceed_query_limit(control)
+  end
+end

lib/gitlab/graphql/project/dast_profile_connection_extension.rb

+# frozen_string_literal: true
+module Gitlab
+  module Graphql
+    module Project
+      class DastProfileConnectionExtension < GraphQL::Schema::Field::ConnectionExtension
+        def after_resolve(value:, object:, context:, **rest)
+          preload_authorizations(context[:project_dast_profiles])
+          context[:project_dast_profiles] = nil
+          value
+        end
+
+        def preload_authorizations(dast_profiles)
+          return unless dast_profiles
+
+          projects = dast_profiles.map(&:project)
+          users = dast_profiles.filter_map { |dast_profile| dast_profile.dast_profile_schedule&.owner }
+          Preloaders::UsersMaxAccessLevelInProjectsPreloader.new(projects: projects, users: users).execute
+        end
+      end
+    end
+  end
+end

spec/lib/gitlab/import_export/all_models.yml

@@ -529,6 +529,7 @@ project:
 - vulnerability_feedback
 - vulnerability_identifiers
 - vulnerability_scanners
+- dast_profiles
 - dast_site_profiles
 - dast_scanner_profiles
 - dast_sites

spec/models/preloaders/users_max_access_level_in_projects_preloader_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+RSpec.describe Preloaders::UsersMaxAccessLevelInProjectsPreloader do
+  let_it_be(:user1) { create(:user) }
+  let_it_be(:user2) { create(:user) }
+
+  let_it_be(:project_1) { create(:project) }
+  let_it_be(:project_2) { create(:project) }
+  let_it_be(:project_3) { create(:project) }
+
+  let(:projects) { [project_1, project_2, project_3] }
+  let(:users) { [user1, user2] }
+
+  before do
+    project_1.add_developer(user1)
+    project_1.add_developer(user2)
+
+    project_2.add_developer(user1)
+    project_2.add_developer(user2)
+
+    project_3.add_developer(user1)
+    project_3.add_developer(user2)
+  end
+
+  context 'preload maximum access level to avoid querying project_authorizations', :request_store do
+    it 'avoids N+1 queries', :request_store do
+      Preloaders::UsersMaxAccessLevelInProjectsPreloader.new(projects: projects, users: users).execute
+
+      expect(count_queries).to eq(0)
+    end
+
+    it 'runs N queries without preloading' do
+      query_count_without_preload = count_queries
+
+      Preloaders::UsersMaxAccessLevelInProjectsPreloader.new(projects: projects, users: users).execute
+      count_queries_with_preload = count_queries
+
+      expect(count_queries_with_preload).to be < query_count_without_preload
+    end
+  end
+
+  def count_queries
+    ActiveRecord::QueryRecorder.new do
+      projects.each do |project|
+        user1.can?(:read_project, project)
+        user2.can?(:read_project, project)
+      end
+    end.count
+  end
+end