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

Rest Framework Tests: Improve speed and repeatability #10503

Merged
merged 4 commits into from
Jul 8, 2024

Conversation

Maffooch
Copy link
Contributor

@Maffooch Maffooch commented Jul 3, 2024

  • ViewSets: Support get_queryset function for more flexible queryset manipulation
  • Authorization helpers: Specify order by id for most queries. For queries that already had a specified ordering, it was left alone if the field was unique on the model, such as Products. Otherwise, the ordering was changed to id
  • Viewsets: enagegement checklist viewset was not used, so removed
  • Tests: Accomodate the enforced ordering
  • Tests: Reduce logs for CICD tests to speed up testing overall by eliminating write operations
  • Tests: Enable FailFast mode ("Tells Django to stop running the test suite after first failed test.")
  • Tests: Enable shuffle tests to identify areas of tests that are dependent on each other
  • Tests: Enable parallel tests to drastically reduce testing time
    • In local testing, I went from ~240 seconds to ~45 seconds

[sc-6766]

@Maffooch Maffooch added the performance performance improvement or bug report label Jul 3, 2024
Copy link

dryrunsecurity bot commented Jul 3, 2024

Hi there 👋, @DryRunSecurity here, below is a summary of our analysis and findings.

DryRun Security Status Findings
Server-Side Request Forgery Analyzer 0 findings
Configured Codepaths Analyzer 0 findings
IDOR Analyzer 0 findings
Sensitive Files Analyzer 1 finding
SQL Injection Analyzer 0 findings
Authn/Authz Analyzer 52 findings
Secrets Analyzer 0 findings

Note

🟢 Risk threshold not exceeded.

Change Summary (click to expand)

The following is a summary of changes in this pull request made by me, your security buddy 🤖. Note that this summary is auto-generated and not meant to be a definitive list of security issues but rather a helpful summary from a security perspective.

Summary:

The changes in this pull request focus on improving the reliability, efficiency, and security of the DefectDojo application's unit tests and API endpoints. The key changes include:

  1. Optimizing the unit test execution by adding parallel test running, excluding non-parallel tests, and using the --failfast option.
  2. Enhancing the authorization and access control mechanisms for various entities, such as engagements, findings, JIRA projects and issues, and product-related data.
  3. Introducing new API endpoints for various features, including notifications, surveys, announcements, and more, while ensuring proper authorization and schema validation.
  4. Improving the overall test coverage and robustness by adding new test cases for various API endpoints and functionality.

These changes demonstrate a strong commitment to application security, as they focus on ensuring that the application's core functionality and data access are properly secured and tested. The changes address potential security concerns, such as unauthorized access, data leakage, and input validation, which are crucial for maintaining the overall security posture of the DefectDojo application.

Files Changed:

  1. docker/entrypoint-unit-tests.sh: Optimized the unit test execution by adding parallel test running, excluding non-parallel tests, and using the --failfast option.
  2. docker-compose.override.unit_tests_cicd.yml: Improved the test environment configuration by setting environment variables for logging, database, and Celery.
  3. docker/entrypoint-unit-tests-devDocker.sh: Implemented similar optimizations for the unit test execution as in the entrypoint-unit-tests.sh file.
  4. dojo/engagement/queries.py: Improved the authorization logic for retrieving authorized engagements.
  5. dojo/cred/queries.py: Optimized the retrieval of authorized credential mappings.
  6. dojo/api_v2/views.py: Introduced new API endpoints and enhanced the authorization and access control mechanisms.
  7. dojo/endpoint/queries.py: Improved the authorization logic for retrieving authorized endpoints and endpoint statuses.
  8. dojo/group/queries.py: Optimized the retrieval of authorized group members.
  9. dojo/finding/queries.py: Enhanced the authorization logic for retrieving authorized findings and stub findings.
  10. dojo/product_type/queries.py: Optimized the retrieval of authorized product type members and groups.
  11. dojo/risk_acceptance/queries.py: Optimized the retrieval of authorized risk acceptances.
  12. dojo/product/queries.py: Improved the authorization logic for various product-related entities.
  13. dojo/jira_link/queries.py: Enhanced the authorization logic for JIRA projects and issues.
  14. dojo/tool_product/queries.py: Optimized the retrieval of authorized tool product settings.
  15. dojo/test/queries.py: Improved the authorization logic for tests and test imports.
  16. dojo/urls.py: Updated the Django URL configuration to include new API endpoints.
  17. unittests/dojo_test_case.py: Minor changes to the test utility methods.
  18. unittests/test_apiv2_notifications.py: Added unit tests for the Notifications API.
  19. unittests/test_apiv2_scan_import_options.py: Added unit tests for the scan import API endpoint.
  20. unittests/test_import_reimport.py: Added unit tests for the import and reimport functionality.
  21. unittests/test_jira_config_engagement.py: Added unit tests for the Jira integration with engagements.
  22. unittests/tools/test_deepfence_threatmapper_parser.py: Added unit tests for the Deepfence Threatmapper parser.
  23. unittests/test_rest_framework.py: Added new test classes and enhanced the existing tests for various API endpoints.

Powered by DryRun Security

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

@Maffooch
Copy link
Contributor Author

Maffooch commented Jul 3, 2024

@kiblik I was not planning on figuring out the shuffle issue quite yet, but addressing inconsistent ordering in API responses largely corrected those problems. I did not intended to steal your thunder, and I hope you do not feel that way 🙏🏼

I also did not plan on the parallel flag either, but I ran it just for kicks and encountered a different error this time around. It keyed me into to looking at how files were passed as class variables. Files, stack traces, and a few other things are not able to be serialized. Anyway, I thought you'd like to know what the underlying issue was 😄

Copy link
Contributor

@cneill cneill left a comment

Choose a reason for hiding this comment

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

Just a couple spots I wasn't sure about - otherwise this looks good! Very excited for faster, more stable test runs 😄

engagement = self.add_engagement_without_jira_project(expected_delta_jira_project_db=0)
self.empty_jira_project_for_engagement(engagement, expected_delta_jira_project_db=0)
self.assertEqual(jira_mock.call_count, 0)
with contextlib.suppress(ValueError):
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a comment about why suppressing ValueErrors is necessary here?

# expecting ValueError as we can't delete existing JIRA Projects
self.empty_jira_project_for_engagement(engagement, expected_delta_jira_project_db=0, expect_error=True)
self.assertEqual(jira_mock.call_count, 1)
with contextlib.suppress(ValueError):
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above - could you add a quick comment explaining this line?

@kiblik
Copy link
Contributor

kiblik commented Jul 3, 2024

@kiblik I was not planning on figuring out the shuffle issue quite yet, but addressing inconsistent ordering in API responses largely corrected those problems. I did not intended to steal your thunder, and I hope you do not feel that way 🙏🏼

I also did not plan on the parallel flag either, but I ran it just for kicks and encountered a different error this time around. It keyed me into to looking at how files were passed as class variables. Files, stack traces, and a few other things are not able to be serialized. Anyway, I thought you'd like to know what the underlying issue was 😄

No hard feelings 😄 I will be happy when it will be solved

@Maffooch Maffooch merged commit cf864b1 into DefectDojo:bugfix Jul 8, 2024
125 checks passed
@Maffooch Maffooch deleted the order branch July 16, 2024 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apiv2 docker performance performance improvement or bug report unittests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants