Gal Katz (4ed20a6f) at 19 Mar 14:01

... and 1 more commit

What does this MR do and why?

Fix feature category in the metrics related to Security Scan Profiles

Earlier we identified that scan profiles related components were incorrectly assigned wrong feature category ( #592069 ). This commit assigns the correct feature category to the corresponding metrics.

Fixes: #592069

Discussion: !227536 (comment 3164478674)

We have confirmed with product manager and product analyst.

References

Screenshots or screen recordings

N/A non functional maintenance change.

How to set up and validate locally

N/A non functional maintenance change.

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

What does this MR do and why?

Fix feature category in the metrics related to Security Scan Profiles

Earlier we identified that scan profiles related components were incorrectly assigned wrong feature category ( #592069 ). This commit assigns the correct feature category to the corresponding metrics.

Fixes: #592069

Discussion: !227536 (comment 3164478674)

We have confirmed with product manager and product analyst.

References

Screenshots or screen recordings

N/A non functional maintenance change.

How to set up and validate locally

N/A non functional maintenance change.

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

LGTM 🚀

@mbenayoun Looks much better. I added two small comments, and I agree with @tigerwnz's point about a potential additional N+1, similar to my previous comment in !222292 (comment 3156191644)

Suggestion: This method is doing too much. It's mixing validation, authorization, record lookups, and the actual business logic all in one place.
I usually prefer having basic validation like params[:guidelines].size > MAX_GUIDELINES, Component not found in this project in the mutation level, together with raise_resource_not_available_error! and graphql validation errors.
If you think this service will be reused, we can structure it like:

      def execute
        validation_error = validate
        return validation_error if validation_error

        create_security_context
      rescue ActiveRecord::RecordInvalid => e
        ServiceResponse.error(...)
      end

      def validate
        validate_permission ||
          validate_component ||
          validate_scan || 
          ...
      end

      def validate_permission
        return if Ability.allowed?(current_user, :create_ascp_security_context, project)
        ServiceResponse.error(message: 'Insufficient permissions', reason: :forbidden)
      end
      ...

Might also apply to create_component_service.rb

Did you consider using BaseService in this MR?
This will allow you to get initialize(project, user = nil, params = {}) and UnauthorizedError = ServiceResponse.error(message: 'You are not authorized to perform this action') for free, and will be consistent with other errors.

Thanks @nrotaru!

Hi @rossfuhrman 👋 Could you please take the maintainer review? Thank you

LGTM! 🚀

What does this MR do and why?

The scan profile related components were incorrectly categorised under the security_asset_inventories feature category, wheres they should be under the security_testing_configuration.

Fix the assigned feature category for scan profile backend components

• Updated the feature category assignment for all scan profile related backend components from security_asset_inventories to security_testing_configuration to properly reflect their functionality and purpose
• Updated the auto-generated docs by running bundle exec rake gitlab:custom_roles:compile_docs
• Updated workers definitions but running a rake task as linter suggested

Fixes: #592069

References

Screenshots or screen recordings

N/A. It is a maintenance backend change.

How to set up and validate locally

N/A. It is a maintenance backend change.

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

What does this MR do and why?

Adds the security_scan_profile_project_statuses table and Security::ScanProfileProjectStatus model to persist profile-specific scan status per project per scan type.

The existing analyzer_project_statuses table tracks inventory-level status across all enablement methods and must not be modified. This new table stores profile-scoped status separately, enabling scanner health indicators for security profiles.

Closes #593131

Implementation details

• Migration creates security_scan_profile_project_statuses on the gitlab_sec database with:
  • Unique index on [project_id, scan_type] (upsert target for future status update service)
  • Index on security_scan_profile_id (FK lookup)
  • Index on build_id (required for loose FK cleanup)
  • Direct FK to security_scan_profiles with cascade delete (same database)
  • Loose FKs for project_id (async_delete) and build_id (async_nullify) for cross-database references, matching the pattern used by analyzer_project_statuses
• Model inherits from SecApplicationRecord and provides three computed helpers:
  • stale? — true when status is not not_configured, last_scan_at is present, and older than 90 days
  • active? — true when success? and not stale
  • profile_failed? — true when failed? and consecutive_failure_count >= 3
• Associations added:
  • Security::ScanProfile — has_many :security_scan_profile_project_statuses
  • EE::Project — has_many :security_scan_profile_project_statuses
• Import/Export — registered security_scan_profile_project_statuses in all_models.yml (not exported, status is runtime data)

Cross-database considerations

MR acceptance checklist

• Tests added for model validations, enums, scopes, computed helpers, and loose FK cleanup
• Migration is safe (single create_table, no data migration, no locks on existing tables)
• Loose FK configuration validated (spec/lib/gitlab/database/loose_foreign_keys_spec.rb passes)
• All loose FK columns have indexes (build_id, project_id, security_scan_profile_id)
• Model registered in all_models.yml for Import/Export awareness
• No changes to existing analyzer_project_statuses or inventory behavior
• RuboCop passes on all changed files

Related

• Parent issue: #591555
• Implementation plan: #591916
• Blocked by: nothing
• Blocks: #593132, #593133

Changelog: added
EE: true

Thanks @nrotaru! Looks better. I added two small comments 🏓

Looks much better!

Suggestion: Can be removed as this feature is already enabled by default.

Suggestion: What do you think about moving the service initialization into the context? I believe it will be cleaner and make things easier to extend in the future

Question: Why do we need attr_accessor :scan_profile_eligibility_service? scan_profile_context holds the eligibility service now

Thanks for the input @rossfuhrman! Sounds good.