Add Security Center Widget on Admin Dashboard

Review Request #10189 — Created Oct. 1, 2018 and updated

Information

Review Board
master
f2d9531...

Reviewers

Add a new Security Center Widget in the Admin Dashboard which gets data
from SecurityCheckRunner() and displays the count of errors as well
as the short error messages from each one. The widget also has a 'More'
button which goes to the Security Checklist page and a 'Reload' button
which reruns SecurityCheckRunner() and refreshes the widget data.

  • Added and registered the widget and confirmed it appears on dashboard
  • Tested by appending datetime.now() to the error_msgs returned by
    SecurityCheckRunner and ensuring it updated on hitting Reload
  • Used the above method to also confirm refreshes on the dashboard

Description From Last Updated

Can you add a screenshot?

brenniebrennie

W293 blank line contains whitespace

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E712 comparison to False should be 'if cond is False:' or 'if not cond:'

reviewbotreviewbot

E501 line too long (85 > 79 characters)

reviewbotreviewbot

F401 'reviewboard.admin.security_checks.SecurityCheckRunner' imported but unused

reviewbotreviewbot

E302 expected 2 blank lines, found 1

reviewbotreviewbot

W292 no newline at end of file

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

Can you reformat as: results = [ item['err_msg'] for item in runner.run() if not item['result'] ] Also, since we are …

brenniebrennie

Undo this.

brenniebrennie

Swap these so the keys are in alphabetical order.

brenniebrennie

Django template variables follow the same naming conventions as Python, so this variable name should be in lower_snake_case.

brenniebrennie

I don't see you using {% static %} anywhere in the template so you shouldn't need to load the staticfiles …

brenniebrennie

Comments should be full sentences (i.e., be sentence case and end in a period).

brenniebrennie

Indentation is off here. This should be indented as: $('#reload-security-widget').click(function(evt) { $('.security-center-content-widget').load('feed/security/?reload=1'); evt.preventDefault(); });

brenniebrennie

Also, this should go first in case there is an exception that occurs within $() or $.load. We also probably …

brenniebrennie

We are using this variable immediately so let's just inline it.

brenniebrennie

This should be a rule in a stylesheet.

brenniebrennie

This should be flush with the first column. We only indent inside the {% block %}.

brenniebrennie

And here.

brenniebrennie

And here.

brenniebrennie

How about "Run security checks and render the result with the given template." ?

brenniebrennie

Missing args, returns.

brenniebrennie

"This widget displays ..."

brenniebrennie

nit: blank line between these.

brenniebrennie

We shouldn't need feedtags (or rather, I don't see any tags in use).

brenniebrennie

error is not even passed into this template. Can an error occur?

brenniebrennie

We use HTML5 which does not have self closing tags. ie, no / here.

brenniebrennie

Should not be indented relative to the above line, i.e., they should start on the same column.

brenniebrennie

Only one space of indentation here.

brenniebrennie

These need to be marked for translation: {% if count == 1 %} {% blocktrans %} 1 security issue was …

brenniebrennie

This shouldn't render correctly? .count isn't a thing. Also why the nbsp? And this shouldn't need <br> since <div> is …

brenniebrennie

Only one space of indentation here.

brenniebrennie

Only one space of indentation here.

brenniebrennie

class="entry"

brenniebrennie

This has no matching {% if %}. This template shouldn't even be rendering. Have you verified it still works? You …

brenniebrennie

Nit: no blank line here.

brenniebrennie

This will not work. If you attempt to interpolate a variable var inside a {% blocktrans %} block without doing …

brenniebrennie

Since we're not doing any interpolation anymore, we can use {% trans "1 security issue was found:" %} instead.

brenniebrennie

There is an extra space between blocktrans and %

brenniebrennie

There is an extra space between blocktrans and %

brenniebrennie

If we use {# ... #} we can have the comment not render to the client, which means less bytes …

brenniebrennie

Can you make it so has-errors is the red one?

brenniebrennie

Can we add a case where count == 0 to be something like "No security issues were found". In that …

brenniebrennie

In your widgets.py you have commas between your list items in your actions list but here you don't. Keep your …

cdkushnicdkushni
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

gojeffcho
Review request changed
Change Summary:

Fix formatting errors.

Commit:
a4871139eb12e4e9d4f4467915255bfe1df59671
5c915a188f08ce98b1f2ecbacab52ad528f3a3dd

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

gojeffcho
brennie
  1. 
      
  2. reviewboard/admin/views.py (Diff revision 3)
     
     
     
    Show all issues

    Can you reformat as:

    results = [
        item['err_msg']
        for item in runner.run()
        if not item['result']
    ]
    

    Also, since we are explicitly not collecting the results and instead collecting the error messages, we should probably call this error_messages or err_msgs.

  3. reviewboard/admin/widgets.py (Diff revision 3)
     
     
    Show all issues

    Undo this.

  4. reviewboard/admin/widgets.py (Diff revision 3)
     
     
     
    Show all issues

    Swap these so the keys are in alphabetical order.

  5. Show all issues

    Django template variables follow the same naming conventions as Python, so this variable name should be in lower_snake_case.

  6. Show all issues

    I don't see you using {% static %} anywhere in the template so you shouldn't need to load the staticfiles template library.

  7. Show all issues

    Comments should be full sentences (i.e., be sentence case and end in a period).

  8. Show all issues

    Indentation is off here. This should be indented as:

        $('#reload-security-widget').click(function(evt) {
            $('.security-center-content-widget').load('feed/security/?reload=1');
            evt.preventDefault();
        });
    
  9. Show all issues

    Also, this should go first in case there is an exception that occurs within $() or $.load.

    We also probably want to do evt.stopPropagation() as well.

  10. Show all issues

    We are using this variable immediately so let's just inline it.

    1. I did not inline this as it's used twice.

    2. I shouldn't review before I've had my coffee.

  11. 
      
gojeffcho
brennie
  1. 
      
  2. Show all issues

    This should be a rule in a stylesheet.

  3. Show all issues

    This should be flush with the first column. We only indent inside the {% block %}.

  4. Show all issues

    And here.

  5. Show all issues

    And here.

  6. 
      
gojeffcho
brennie
  1. 
      
  2. reviewboard/admin/views.py (Diff revision 5)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    How about "Run security checks and render the result with the given template." ?

  3. reviewboard/admin/views.py (Diff revision 5)
     
     
     
    Show all issues

    Missing args, returns.

    1. Could I get more fidelity on what you mean by this? Again, comparing this code to the others in the same file, it looks to be in-style but I may be missing or overlooking something.

    2. It is in style for the file, but we don't want to add new code without proper documentation even if the surrounding file is lacking.

      The docstring should look something like this:

      """Run security checks and pass error messages to template.
      
      Args:
          request (django.http.HttpRequest):
              The HTTP request from the client.
      
          template_name (unicode):
              The template to render the result of the security
              checks with.
      
      Returns:
          django.http.HttpResponse:
          The rendered response.
      """
      
    3. It should be noted we use these docstrings to autogenerate our documentation (see here). So if Args/Returns are missing the documentation will be incomplete.

  4. reviewboard/admin/widgets.py (Diff revision 5)
     
     
    Show all issues

    "This widget displays ..."

    1. I left this in its current format, because it seems to follow the format and voice of the other widget descriptions in this file. If you feel that it should be changed to the style of your suggestion, I can implement this.

    2. It is technically a sentence fragment, since there is no subject, and we don't like to add new things that don't match our current documentation style, even if it clashes with the rest of the documentation in the file. It creates more work down the line to migrate docs that we could have written in our new style in the first place.

  5. Show all issues

    nit: blank line between these.

  6. Show all issues

    We shouldn't need feedtags (or rather, I don't see any tags in use).

  7. reviewboard/templates/admin/security_feed.html (Diff revision 5)
     
     
     
     
    Show all issues

    error is not even passed into this template. Can an error occur?

  8. Show all issues

    We use HTML5 which does not have self closing tags. ie, no / here.

  9. 
      
gojeffcho
gojeffcho
brennie
  1. 
      
  2. reviewboard/admin/views.py (Diff revision 7)
     
     
    Show all issues

    Should not be indented relative to the above line, i.e., they should start on the same column.

  3. Show all issues

    Only one space of indentation here.

  4. reviewboard/templates/admin/security_feed.html (Diff revision 7)
     
     
     
     
     
     
     
    Show all issues

    These need to be marked for translation:

    {% if count == 1 %}
    {%  blocktrans  %}
    1 security issue was found:
    {%  endblocktrans %}
    {% else %}
    {%  blocktrans with count=count %}
    {{count}} security issues were found:
    {%  endblocktrans %}
    {% endif %}
    
  5. Show all issues

    This shouldn't render correctly? .count isn't a thing.

    Also why the nbsp?

    And this shouldn't need <br> since <div> is display: block by default.

  6. Show all issues

    Only one space of indentation here.

  7. Show all issues

    Only one space of indentation here.

  8. Show all issues

    class="entry"

  9. Show all issues

    This has no matching {% if %}. This template shouldn't even be rendering. Have you verified it still works?

    You should re-run all testing with each revision.

  10. 
      
gojeffcho
brennie
  1. 
      
  2. Show all issues

    Can you add a screenshot?

  3. reviewboard/admin/views.py (Diff revision 8)
     
     
    Show all issues

    Nit: no blank line here.

  4. reviewboard/templates/admin/security_feed.html (Diff revision 8)
     
     
     
     
    Show all issues

    This will not work. If you attempt to interpolate a variable var inside a {% blocktrans %} block without doing {% blocktrans with var=... %} it will not render.

    Also, we know its 1. Let's just use 1 instead of forcing an interpolation.

  5. Show all issues

    There is an extra space between blocktrans and %

  6. Show all issues

    There is an extra space between blocktrans and %

  7. Show all issues

    If we use {# ... #} we can have the comment not render to the client, which means less bytes over the wire.

  8. 
      
gojeffcho
gojeffcho
gojeffcho
brennie
  1. Teeny tiny nits :)

  2. reviewboard/templates/admin/security_feed.html (Diff revisions 8 - 9)
     
     
     
     
    Show all issues

    Since we're not doing any interpolation anymore, we can use {% trans "1 security issue was found:" %} instead.

  3. reviewboard/templates/admin/security_feed.html (Diff revision 9)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Can we add a case where count == 0 to be something like "No security issues were found". In that case we don't need to iterate over err_msgs.

    In this case the font colour probably shouldn't be red. I would suggest doing something like:

    <div class="summary{% if count > 0 %} has-errors{% endif %}">
    ...
    </div>
    

    and add a .summary { &.has-errors: { ... } } rule in the stylesheet that overrides the colour.

  4. 
      
gojeffcho
brennie
  1. 
      
  2. Show all issues

    Can you make it so has-errors is the red one?

  3. 
      
gojeffcho
Review request changed
Change Summary:

i r dum

Commit:
901d2ef85b84a19b47fa88b4d689c4eee3525981
f2d9531688ee6be578bfa2922011b63f20d4b1da

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
cdkushni
  1. 
      
  2. reviewboard/admin/views.py (Diff revision 11)
     
     
    Show all issues

    In your widgets.py you have commas between your list items in your actions list but here you don't. Keep your syntax consistent.

    1. This is a list comprehension, so commas are not required.

  3.