Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API: return stats for api (re)imports #5635

Merged
merged 21 commits into from
Jan 5, 2022

Conversation

valentijnscholten
Copy link
Member

@valentijnscholten valentijnscholten commented Dec 23, 2021

This PR adds some improvements to Reimport (and Import), mainly focused on returning statistics. These can be useful in ci/cd situations, for example to do follow up actions if there were new (critical) findings, or to store statistics in a ci/cd platform to show trends.

Changes:

  • Remove unused IMPORT_UPDATED_FINDING;
  • Add IMPORT_UNTOUCHED_FINDING to record which findings were untouched by a reimport;
  • Add stats property to Test;
  • Add stats property to Test_Import;
  • Add Serializer classes to return statistics in API responses for import_scan and reimport_scan;
  • Add index to help with statistics lookup queries;
  • Update UI logic to not show untouched actions in status history of finding (to avoid cluttering on frequent reimports);
  • Add test case and logic to handle IMPORT_UNTOUCHED_FINDING action;
  • Update existing test cases to assert correctly on IMPORT_UNTOUCHED_FINDING;
  • Make sure both the swagger and openapi3 specs are correctly rendering the response schema;
  • Workaround problems caused by Re-importing the same report leaves the duplicates in status mitigated #3958
  • Remove @override_settings(TRACK_IMPORT_HISTORY=True) from tests as True has been the default for a while

At some point during this PR I had added the stats property also to Product_Type, Product and Engagement. But this would become slow when serializing lists of these models. I tried to work with prefetching, but that is slow on these big joins. I tried using a seperate query for the statistics, but this would be hard to do at the right moment in the serializer classes (after paging, after filtering, but before serializing). So I removed this part and focused just on (re-)import statistics, which was the original goal of this PR.

A more generic solution for statistics on Product_Type, Product, Engagement and Test serializer has to be a different PR. The more I work with statistics on these models, the more I see that all this prefetching and querying is hard to maintain and hard to keep performant for all usecases. So maybe we should introduce a statistics table ('materialized view'), similar to product grade, that gets updated on modifications but can return statistics very easily without complex joins or slow performance.

The statistics are quite verbose as I think it's important to have the severity in there as well to be able to make decisions based on for example the number of critical vulnerabilities still open or that were added. It also means that the stats are correct even if some parsers are importing findings as not active or is_mitigated.

For import there is only the after part of the statistics:

   "after":{
      "info":{
         "active":0,
         "verified":0,
         "duplicate":0,
         "false_p":0,
         "out_of_scope":0,
         "is_mitigated":0,
         "risk_accepted":0,
         "total":0
      },
      "low":{
         "active":3,
         "verified":4,
         "duplicate":0,
         "false_p":0,
         "out_of_scope":0,
         "is_mitigated":1,
         "risk_accepted":0,
         "total":4
      },
      "medium":{
         "active":1,
         "verified":1,
         "duplicate":0,
         "false_p":0,
         "out_of_scope":0,
         "is_mitigated":0,
         "risk_accepted":0,
         "total":1
      },
      "high":{
         "active":0,
         "verified":0,
         "duplicate":0,
         "false_p":0,
         "out_of_scope":0,
         "is_mitigated":0,
         "risk_accepted":0,
         "total":0
      },
      "critical":{
         "active":0,
         "verified":0,
         "duplicate":0,
         "false_p":0,
         "out_of_scope":0,
         "is_mitigated":0,
         "risk_accepted":0,
         "total":0
      },
      "total":{
         "active":4,
         "verified":5,
         "duplicate":0,
         "false_p":0,
         "out_of_scope":0,
         "is_mitigated":1,
         "risk_accepted":0,
         "total":5
      }

For reimport, there is also the before and delta parts.

{
   "before":{
      "info":{
         "active":0,
         "verified":0,
         "duplicate":0,
         "false_p":0,
         "out_of_scope":0,
         "is_mitigated":0,
         "risk_accepted":0,
         "total":0
      },
      "low":{
         "active":3,
         "verified":3,
         "duplicate":0,
         "false_p":0,
         "out_of_scope":0,
         "is_mitigated":0,
         "risk_accepted":0,
         "total":3
      },
      "medium":{
         "active":1,
         "verified":1,
         "duplicate":0,
         "false_p":0,
         "out_of_scope":0,
         "is_mitigated":0,
         "risk_accepted":0,
         "total":1
      },
      "high":{
         "active":0,
         "verified":0,
         "duplicate":0,
         "false_p":0,
         "out_of_scope":0,
         "is_mitigated":0,
         "risk_accepted":0,
         "total":0
      },
      "critical":{
         "active":0,
         "verified":0,
         "duplicate":0,
         "false_p":0,
         "out_of_scope":0,
         "is_mitigated":0,
         "risk_accepted":0,
         "total":0
      },
      "total":{
         "active":4,
         "verified":4,
         "duplicate":0,
         "false_p":0,
         "out_of_scope":0,
         "is_mitigated":0,
         "risk_accepted":0,
         "total":4
      }
   },
   "delta":{
      "created":{
         "info":{
            "active":0,
            "verified":0,
            "duplicate":0,
            "false_p":0,
            "out_of_scope":0,
            "is_mitigated":0,
            "risk_accepted":0,
            "total":0
         },
         "low":{
            "active":1,
            "verified":1,
            "duplicate":0,
            "false_p":0,
            "out_of_scope":0,
            "is_mitigated":0,
            "risk_accepted":0,
            "total":1
         },
         "medium":{
            "active":0,
            "verified":0,
            "duplicate":0,
            "false_p":0,
            "out_of_scope":0,
            "is_mitigated":0,
            "risk_accepted":0,
            "total":0
         },
         "high":{
            "active":0,
            "verified":0,
            "duplicate":0,
            "false_p":0,
            "out_of_scope":0,
            "is_mitigated":0,
            "risk_accepted":0,
            "total":0
         },
         "critical":{
            "active":0,
            "verified":0,
            "duplicate":0,
            "false_p":0,
            "out_of_scope":0,
            "is_mitigated":0,
            "risk_accepted":0,
            "total":0
         },
         "total":{
            "active":1,
            "verified":1,
            "duplicate":0,
            "false_p":0,
            "out_of_scope":0,
            "is_mitigated":0,
            "risk_accepted":0,
            "total":1
         }
      },
      "closed":{
         "info":{
            "active":0,
            "verified":0,
            "duplicate":0,
            "false_p":0,
            "out_of_scope":0,
            "is_mitigated":0,
            "risk_accepted":0,
            "total":0
         },
         "low":{
            "active":0,
            "verified":1,
            "duplicate":0,
            "false_p":0,
            "out_of_scope":0,
            "is_mitigated":1,
            "risk_accepted":0,
            "total":1
         },
         "medium":{
            "active":0,
            "verified":0,
            "duplicate":0,
            "false_p":0,
            "out_of_scope":0,
            "is_mitigated":0,
            "risk_accepted":0,
            "total":0
         },
         "high":{
            "active":0,
            "verified":0,
            "duplicate":0,
            "false_p":0,
            "out_of_scope":0,
            "is_mitigated":0,
            "risk_accepted":0,
            "total":0
         },
         "critical":{
            "active":0,
            "verified":0,
            "duplicate":0,
            "false_p":0,
            "out_of_scope":0,
            "is_mitigated":0,
            "risk_accepted":0,
            "total":0
         },
         "total":{
            "active":0,
            "verified":1,
            "duplicate":0,
            "false_p":0,
            "out_of_scope":0,
            "is_mitigated":1,
            "risk_accepted":0,
            "total":1
         }
      },
      "reactivated":{
         "info":{
            "active":0,
            "verified":0,
            "duplicate":0,
            "false_p":0,
            "out_of_scope":0,
            "is_mitigated":0,
            "risk_accepted":0,
            "total":0
         },
         "low":{
            "active":0,
            "verified":0,
            "duplicate":0,
            "false_p":0,
            "out_of_scope":0,
            "is_mitigated":0,
            "risk_accepted":0,
            "total":0
         },
         "medium":{
            "active":0,
            "verified":0,
            "duplicate":0,
            "false_p":0,
            "out_of_scope":0,
            "is_mitigated":0,
            "risk_accepted":0,
            "total":0
         },
         "high":{
            "active":0,
            "verified":0,
            "duplicate":0,
            "false_p":0,
            "out_of_scope":0,
            "is_mitigated":0,
            "risk_accepted":0,
            "total":0
         },
         "critical":{
            "active":0,
            "verified":0,
            "duplicate":0,
            "false_p":0,
            "out_of_scope":0,
            "is_mitigated":0,
            "risk_accepted":0,
            "total":0
         },
         "total":{
            "active":0,
            "verified":0,
            "duplicate":0,
            "false_p":0,
            "out_of_scope":0,
            "is_mitigated":0,
            "risk_accepted":0,
            "total":0
         }
      },
      "left untouched":{
         "info":{
            "active":0,
            "verified":0,
            "duplicate":0,
            "false_p":0,
            "out_of_scope":0,
            "is_mitigated":0,
            "risk_accepted":0,
            "total":0
         },
         "low":{
            "active":2,
            "verified":2,
            "duplicate":0,
            "false_p":0,
            "out_of_scope":0,
            "is_mitigated":0,
            "risk_accepted":0,
            "total":2
         },
         "medium":{
            "active":1,
            "verified":1,
            "duplicate":0,
            "false_p":0,
            "out_of_scope":0,
            "is_mitigated":0,
            "risk_accepted":0,
            "total":1
         },
         "high":{
            "active":0,
            "verified":0,
            "duplicate":0,
            "false_p":0,
            "out_of_scope":0,
            "is_mitigated":0,
            "risk_accepted":0,
            "total":0
         },
         "critical":{
            "active":0,
            "verified":0,
            "duplicate":0,
            "false_p":0,
            "out_of_scope":0,
            "is_mitigated":0,
            "risk_accepted":0,
            "total":0
         },
         "total":{
            "active":3,
            "verified":3,
            "duplicate":0,
            "false_p":0,
            "out_of_scope":0,
            "is_mitigated":0,
            "risk_accepted":0,
            "total":3
         }
      }
   },
   "after":{
      "info":{
         "active":0,
         "verified":0,
         "duplicate":0,
         "false_p":0,
         "out_of_scope":0,
         "is_mitigated":0,
         "risk_accepted":0,
         "total":0
      },
      "low":{
         "active":3,
         "verified":4,
         "duplicate":0,
         "false_p":0,
         "out_of_scope":0,
         "is_mitigated":1,
         "risk_accepted":0,
         "total":4
      },
      "medium":{
         "active":1,
         "verified":1,
         "duplicate":0,
         "false_p":0,
         "out_of_scope":0,
         "is_mitigated":0,
         "risk_accepted":0,
         "total":1
      },
      "high":{
         "active":0,
         "verified":0,
         "duplicate":0,
         "false_p":0,
         "out_of_scope":0,
         "is_mitigated":0,
         "risk_accepted":0,
         "total":0
      },
      "critical":{
         "active":0,
         "verified":0,
         "duplicate":0,
         "false_p":0,
         "out_of_scope":0,
         "is_mitigated":0,
         "risk_accepted":0,
         "total":0
      },
      "total":{
         "active":4,
         "verified":5,
         "duplicate":0,
         "false_p":0,
         "out_of_scope":0,
         "is_mitigated":1,
         "risk_accepted":0,
         "total":5
      }
   }

@github-actions github-actions bot added the apiv2 label Dec 23, 2021
@github-actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@github-actions github-actions bot added the New Migration Adding a new migration file. Take care when merging. label Jan 2, 2022
@valentijnscholten valentijnscholten changed the title return stats for api (re)imports reimport: return stats for api (re)imports Jan 2, 2022
@valentijnscholten valentijnscholten removed the New Migration Adding a new migration file. Take care when merging. label Jan 2, 2022
This reverts commit 0b7781e.
@github-actions github-actions bot added the New Migration Adding a new migration file. Take care when merging. label Jan 2, 2022
@valentijnscholten valentijnscholten marked this pull request as ready for review January 2, 2022 15:31
@valentijnscholten valentijnscholten changed the title reimport: return stats for api (re)imports API: return stats for api (re)imports Jan 2, 2022
Copy link
Contributor

@Maffooch Maffooch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a fantastic addition

Copy link
Contributor

@mtesauro mtesauro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved

@StefanFl
Copy link
Contributor

StefanFl commented Jan 5, 2022

Pretty cool 😃

@StefanFl StefanFl merged commit 3750383 into DefectDojo:dev Jan 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apiv2 New Migration Adding a new migration file. Take care when merging. ui unittests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants