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

Error handling for Generate Report #2274

Merged
merged 24 commits into from
Jan 23, 2024
Merged

Conversation

madelondohmen
Copy link
Contributor

@madelondohmen madelondohmen commented Jan 4, 2024

Changes

This code adds error handling in the following two situations while generating a report:

  • Changing the OOI in the URL to a non-existing one
  • Changing the date in the URL to a date in the past with no data

Issue link

Closes #1972

Note: Only the first two errors of issue #1972 have been solved with this PR. Error 3 and 4 couldn't be reproduced, so they've been ignored for now.

Extra info

I've added a try-except in generate_report.py in the "generate_reports_for_oois" method. If an error occurs while trying to get the OOI data, the failed OOI will be saved in "error_oois". This way, the loop can continue to get the other OOIs (which are not failing).

After that, an error will be shown to the user with the failed OOI and the date. The report will still be generated with the succesfully obtained data from the other OOIs.

I've also added a try-except in base.py in the "get_oois" method, so the other error (that occured after changing the URL into a non-existing OOI) would be handled as well. All OOIs that do not fail, will be returned, so the report can still be generated.

Demo

Wrong date:
afbeelding

Wrong OOI:
afbeelding

Wrong date and OOI:
afbeelding


Code Checklist

  • All the commits in this PR are properly PGP-signed and verified;
  • This PR only contains functionality relevant to the issue; tickets have been created for newly discovered issues.
  • I have written unit tests for the changes or fixes I made.
  • For any non-trivial functionality, I have added integration and/or end-to-end tests.
  • I have performed a self-review of my code and refactored it to the best of my abilities.

Communication

  • I have informed others of any required .env changes files if required and changed the .env-dist accordingly.
  • I have made corresponding changes to the documentation, if necessary.

Checklist for code reviewers:

Copy-paste the checklist from the docs/source/templates folder into your comment.


Checklist for QA:

Copy-paste the checklist from the docs/source/templates folder into your comment.

@madelondohmen madelondohmen requested a review from a team as a code owner January 4, 2024 10:20
@@ -71,7 +74,13 @@ def setup(self, request, *args, **kwargs):
self.selected_report_types = request.GET.getlist("report_type", [])

def get_oois(self) -> List[OOI]:
Copy link
Contributor

Choose a reason for hiding this comment

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

This will conflict later, depends on merging prio with:

def get_oois(self) -> List[OOI]:
if "all" in self.selected_oois:
return self.octopoes_api_connector.list(
self.get_ooi_types(),
valid_time=self.valid_time,
limit=OOIList.HARD_LIMIT,
scan_level=self.get_ooi_scan_levels(),
scan_profile_type=self.get_ooi_profile_types(),
).items
return [self.get_single_ooi(ooi_id) for ooi_id in self.selected_oois]

rocky/reports/views/base.py Outdated Show resolved Hide resolved
rocky/reports/views/generate_report.py Outdated Show resolved Hide resolved
Copy link
Contributor

@ammar92 ammar92 left a comment

Choose a reason for hiding this comment

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

Small suggestions to improve even more, but should be ready for QA testing

rocky/reports/views/generate_report.py Outdated Show resolved Hide resolved
rocky/reports/views/generate_report.py Outdated Show resolved Hide resolved
@stephanie0x00
Copy link
Contributor

stephanie0x00 commented Jan 15, 2024

Checklist for QA:

  • I have checked out this branch, and successfully ran a fresh make reset.
  • I confirmed that there are no unintended functional regressions in this branch:
    • I have managed to pass the onboarding flow
    • Objects and Findings are created properly
    • Tasks are created and completed properly
  • [] I confirmed that the PR's advertised feature or hotfix works as intended.

What works:

Onboarding flow works, tasks and findings are created.

What doesn't work:

The 500 errors are catched for the DNS report, but not for all other reports.

date-in-the-past

Bug or feature?:

error-500.tar.gz

  1. Noticed that the aggregate report doesn't give a warning when not all required plugins are enabled. The 'normal' report does give this warning.

  2. The fixes do not appear to be applicable to all reports. They can be triggered for the DNS report, but not on the system vulnerability report for example.

  3. Identified a 'ValueError' when making a typo in:

  • the report_type parameter in the URL
  • ooi parameter in the URL, specifically in the 'Hostname' part.

The URL for the report_type parameter error with 'report_type=dns-repoooort':

http://127.0.0.1:8000/en/ee/reports/generate-report/view/?observed_at=2024-01-15&plugin=dns-sec&plugin=dns-records&ooi=Hostname|internet|mispo.es&report_type=dns-repoooort

The URL for the ooi parameter error with 'ooi=Hostnameeeeee|internet|mispo.es':

http://127.0.0.1:8000/en/ee/reports/generate-report/view/?observed_at=2024-01-15&plugin=dns-sec&plugin=dns-recooordds&ooi=Hostnameeeeee|internet|mispo.es&report_type=dns-report

500-error-valueerror

Environment:


Request Method: GET
Request URL: http://127.0.0.1:8000/en/ee/reports/generate-report/view/?observed_at=2024-01-15&plugin=dns-sec&plugin=dns-records&ooi=Hostname%7Cinternet%7Cmispo.es&report_type=dns-repoooort

Django Version: 4.2.7
Python Version: 3.11.7
Installed Applications:
['whitenoise.runserver_nostatic',
 'django.contrib.admin',
 'django.contrib.auth',
 'django.contrib.contenttypes',
 'django.contrib.sessions',
 'django.contrib.messages',
 'django.contrib.staticfiles',
 'django.forms',
 'django_otp',
 'django_otp.plugins.otp_static',
 'django_otp.plugins.otp_totp',
 'two_factor',
 'account',
 'tools',
 'fmea',
 'crisis_room',
 'onboarding',
 'katalogus',
 'django_password_validators',
 'django_password_validators.password_history',
 'rest_framework',
 'tagulous',
 'compressor',
 'reports',
 'csp']
Installed Middleware:
['django.middleware.security.SecurityMiddleware',
 'whitenoise.middleware.WhiteNoiseMiddleware',
 'django.contrib.sessions.middleware.SessionMiddleware',
 'django.middleware.locale.LocaleMiddleware',
 'django.middleware.common.CommonMiddleware',
 'django.middleware.csrf.CsrfViewMiddleware',
 'django.contrib.auth.middleware.AuthenticationMiddleware',
 'django_otp.middleware.OTPMiddleware',
 'rocky.middleware.auth_required.AuthRequiredMiddleware',
 'django.contrib.messages.middleware.MessageMiddleware',
 'django.middleware.clickjacking.XFrameOptionsMiddleware',
 'rocky.middleware.onboarding.OnboardingMiddleware',
 'csp.middleware.CSPMiddleware']



Traceback (most recent call last):
  File "/usr/local/lib/python3.11/site-packages/django/core/handlers/exception.py", line 55, in inner
    response = get_response(request)
               ^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/django/core/handlers/base.py", line 197, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/django/views/generic/base.py", line 98, in view
    self.setup(request, *args, **kwargs)
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/app/rocky/reports/views/generate_report.py", line 138, in setup
    self.plugins = self.get_required_optional_plugins(get_plugins_for_report_ids(self.selected_report_types))
                                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/app/rocky/reports/report_types/helpers.py", line 80, in get_plugins_for_report_ids
    reports = get_reports(reports)
              ^^^^^^^^^^^^^^^^^^^^
  File "/app/rocky/reports/report_types/helpers.py", line 70, in get_reports
    return [get_report_by_id(report_id) for report_id in report_ids]
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/app/rocky/reports/report_types/helpers.py", line 70, in <listcomp>
    return [get_report_by_id(report_id) for report_id in report_ids]
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/app/rocky/reports/report_types/helpers.py", line 66, in get_report_by_id
    raise ValueError(f"Report with id {report_id} not found")
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Exception Type: ValueError at /en/ee/reports/generate-report/view/
Exception Value: Report with id dns-repoooort not found

@madelondohmen madelondohmen self-assigned this Jan 15, 2024
ammar92
ammar92 previously approved these changes Jan 18, 2024
Copy link
Contributor

@ammar92 ammar92 left a comment

Choose a reason for hiding this comment

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

No more remarks

ammar92
ammar92 previously approved these changes Jan 18, 2024
@stephanie0x00
Copy link
Contributor

Checklist for QA:

  • I have checked out this branch, and successfully ran a fresh make reset.
  • I confirmed that there are no unintended functional regressions in this branch:
    • I have managed to pass the onboarding flow
    • Objects and Findings are created properly
    • Tasks are created and completed properly
  • I confirmed that the PR's advertised feature or hotfix works as intended.

What works:

The error handling seems to work as expected, thus it can be merged. The found items can also be solved in separate PRs.

What doesn't work:

'Normal' report:

  • The timestamp warning works when working in the browser. However one can still download an report with an outdated timestamp. E.g. when I change the date to: 2020-04-16 in the URL I see the warning in the browser. However when I download this report it shows the date from the past. A malicious user could use this to tamper with timestamp on his reports and pretend to me compliant at a previous point in time.

report-adjusted-timestamp

Bug or feature?:

  • When refreshing the page you get the Error notification twice.

report-error-twice

  • Adding [] to the parameter report_type make that specific report type disappear, similar to the ooi parameter.

@dekkers dekkers merged commit 4a35dac into main Jan 23, 2024
50 checks passed
@dekkers dekkers deleted the fix/error-handling-generate-report branch January 23, 2024 15:54
jpbruinsslot added a commit that referenced this pull request Jan 29, 2024
* main:
  Fix/394 Introduce clearance level control for objects imported by CSV (#2390)
  Update RabbitMQ to the latest version (#2392)
  Show created at and data from in reports (#2370)
  Rename invalid rpki finding to expired (#2377)
  Convert `docker-compose` to `docker compose` (#2341)
  Remove uWSGI (#2366)
  Prevent double github actions (#2374)
  Fix WEASYPRINT_BASEURL default value and change ports in docker-compose.yml (#2373)
  Remove debian11 packages (#2358)
  Error handling for Generate Report (#2274)
  Update dependencies (#2348)
  Add token authentication (#2349)
  Adds CAA records to the model, boefje, normalizer, adds a check bit and a finding (#2315)
  Check for sudo in install and update script (#2360)
  Feat/normalizer mimetype upload deeplink (#2220)
  Add hrefs to Basic Security overview (#2330)
  Render dicts and list ooi attrs as jsonfield (#2355)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Catch 500 errors in reports
7 participants