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

Diff:

Revision 2 (+87 -1)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

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

    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)
     
     

    Undo this.

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

    Swap these so the keys are in alphabetical order.

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

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

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

  8. 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. 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. 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. This should be a rule in a stylesheet.

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

  4. 
      
gojeffcho
brennie
  1. 
      
  2. reviewboard/admin/views.py (Diff revision 5)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

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

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

    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)
     
     

    "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. nit: blank line between these.

  6. 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)
     
     
     
     

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

  8. 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)
     
     

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

  3. Only one space of indentation here.

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

    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. 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. Only one space of indentation here.

  7. Only one space of indentation here.

  8. class="entry"

  9. 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. Can you add a screenshot?

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

    Nit: no blank line here.

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

    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. There is an extra space between blocktrans and %

  6. There is an extra space between blocktrans and %

  7. 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)
     
     
     
     

    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)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    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. Can you make it so has-errors is the red one?

  3. 
      
gojeffcho
Review request changed

Checks run (2 succeeded)

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

    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. 
      
Loading...