Skip to content

Conversation

patriknordlen
Copy link
Contributor

@patriknordlen patriknordlen commented Jul 1, 2017

This PR addresses (or at least attempts to address :) ) #215.

It adds the model "System_Settings" for keeping system settings in the database, along with views and forms to change them in the web UI.
Also changes have been made to all components using these settings so that they refer to the settings from the model rather than settings.py.

Settings that have been added to the model and migrated in the application are:

  • Enabling/disabling findings deduplication (ENABLE_DEDUPLICATOIN)
  • Enabling/disabling JIRA integration (ENABLE_JIRA)
  • Enabling/disabling S finding severity naming (S_FINDING_SEVERITY_NAMING)
  • Specifying URL prefix (URL_PREFIX)
  • Specifying timezone (TIME_ZONE)
  • Specifying team name (TEAM_NAME)

Settings that are missing from this PR that were part of the original issue:

  • Mail Server - from what I understand this is used internally by Django so might require another approach to get to work?

Caveats:

  • Setting url_prefix and time_zone in the web UI needs a restart to take effect.
  • If the specified url_prefix does not point to a valid defectdojo instance this will render the web UI unusable and correct url_prefix needs to be set directly in the database.
  • Database will be populated with default settings using a fixture on a new install but I haven't written anything to migrate existing settings in settings.py automatically.

There's, uh, also a couple of additional commits related to look and feel + bug fixes:

  • I changed the behaviour for help_text to be displayed as a popover over a question mark next to the field name.
  • The report builder's functionality to add hidden inputs when a user has tried to apply filter terms wasn't marked as safe so was HTML escaped, so any filter terms the user entered would not be applied to the generated report. Fixed by escaping individual parameters from the original query string but marking the final output as safe.
  • The search was throwing an error due to wrong watson property being called. Fixed by calling the correct property instead. :)

I'm used to straight and honest PR feedback so don't hold back - I'm new at contributing to this project and I'm sure I've gotten a thing or two wrong.

@devGregA
Copy link
Contributor

devGregA commented Jul 2, 2017

@patriknordlen thank you so much for doing this! Will clone and review shortly.

@devGregA
Copy link
Contributor

devGregA commented Jul 3, 2017

@patriknordlen just wanted to provide an update. Will be able to clone and review on Wednesday

@patriknordlen
Copy link
Contributor Author

Cool! Don't sweat it :)

@devGregA
Copy link
Contributor

devGregA commented Jul 5, 2017

Hi @patriknordlen thank you again for working on this!
The only thing I think would should add is a section in upgrades on readthedocs documenting what folks should due to upgrade their installs https://github.com/OWASP/django-DefectDojo/blob/master/docs/upgrading.rst. I think we should put a date entry so it is easier for people to see in a separate section such as:

July 5th

load the systems settings fixture `python manage.py loaddata system_settings.json'
remove the following from settings.py

I'm happy to merge as is though and due the documentation if that is confusing / you don't want to :)
Also if you would like dojo swag as a reward for knocking this out please email [email protected] with the address to mail to.

@devGregA devGregA self-requested a review July 5, 2017 18:31
@patriknordlen
Copy link
Contributor Author

patriknordlen commented Jul 5, 2017

Thanks for the review @devGregA! I'll go make the documentation changes tomorrow and submit, so let's do the merge after that. :)

@patriknordlen
Copy link
Contributor Author

@devGregA documentation added - please have a look.

<span class="text-error">
{% else %}<span>{% endif %}{{ finding.severity }}</span></td>
<td>
<span class="label severity severity-{{ finding.severity|safe }}">
Copy link
Contributor

@devGregA devGregA Jul 6, 2017

Choose a reason for hiding this comment

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

@patriknordlen last question I promise 😂 , would you please explain this change? I understand the nav / label changes but I'm not sure why we're marking the severity as safe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha, that's ok! 😄

That's a good catch, it's from my initial misunderstanding of what the safe function did 🙄

I'll change that and go through any other uses of |safe in my commits to make sure they're actually needed!

@patriknordlen
Copy link
Contributor Author

There, done!

@devGregA devGregA merged commit a5b333d into DefectDojo:master Jul 7, 2017
@devGregA
Copy link
Contributor

devGregA commented Jul 7, 2017

@patriknordlen merged :)

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.

2 participants