Add logging for decorator webapi_check_local_site when return PERMISSION_DENIED

Review Request #7868 — Created Jan. 15, 2016 and submitted

Information

Review Board
master

Reviewers

Add logging for decorator webapi_check_local_site when return PERMISSION_DENIED
decorator webapi_check_local_site covers a large numbers of permission checks. When user does not have permission, we return 403 and we are expecting meaningful output in the server-side logs. Current implementation of the decorator is lack of the logging capability. This patch adds 3 logging statements when the decorator returns PERMISSION_DENIED

I manually tested that the server does log events when
- User is not allowed too access the local site
- User tries to access a local site with invalid token
- User tries to access with a valid token but with no local site name

Description From Last Updated

Col: 45 W291 trailing whitespace

reviewbotreviewbot

Col: 71 W291 trailing whitespace

reviewbotreviewbot

Col: 41 W291 trailing whitespace

reviewbotreviewbot

Col: 80 E501 line too long (81 > 79 characters)

reviewbotreviewbot

Col: 37 W291 trailing whitespace

reviewbotreviewbot

Col: 21 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 21 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 21 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 21 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 21 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 41 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 37 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 33 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Python imports should be grouped into three categories, with a blank line between each: Standard library Third-party packages This module …

daviddavid

How about: '%s %s by %s: User does not have access to local site %s' % (request.method, request.path_info, request.user.username, local_site_name)

daviddavid

How about: '%s %s by %s: API token does not have access to local site %s' % (request.method, request.path_info, request.user.username, …

daviddavid

How about: '%s %s by %s: API token is limited to a local site' % (request.method, request.path_info, request.user.username)

daviddavid

Col: 37 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 33 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 29 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 37 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 33 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 29 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 45 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 67 W291 trailing whitespace

reviewbotreviewbot

Col: 49 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 41 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 63 W291 trailing whitespace

reviewbotreviewbot

Col: 45 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 37 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 55 W291 trailing whitespace

reviewbotreviewbot

Col: 41 E127 continuation line over-indented for visual indent

reviewbotreviewbot

The arguments within the (...) should be aligned (second and third rows shouldn't be indented), but actually, in the case …

chipx86chipx86

Col: 45 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 45 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 41 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 41 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 37 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 37 E127 continuation line over-indented for visual indent

reviewbotreviewbot
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/decorators.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/decorators.py
    
    
  2. reviewboard/webapi/decorators.py (Diff revision 1)
     
     
    Show all issues
    Col: 45
     W291 trailing whitespace
    
  3. reviewboard/webapi/decorators.py (Diff revision 1)
     
     
    Show all issues
    Col: 71
     W291 trailing whitespace
    
  4. reviewboard/webapi/decorators.py (Diff revision 1)
     
     
    Show all issues
    Col: 41
     W291 trailing whitespace
    
  5. reviewboard/webapi/decorators.py (Diff revision 1)
     
     
    Show all issues
    Col: 80
     E501 line too long (81 > 79 characters)
    
  6. reviewboard/webapi/decorators.py (Diff revision 1)
     
     
    Show all issues
    Col: 37
     W291 trailing whitespace
    
  7. reviewboard/webapi/decorators.py (Diff revision 1)
     
     
    Show all issues
    Col: 21
     E128 continuation line under-indented for visual indent
    
  8. reviewboard/webapi/decorators.py (Diff revision 1)
     
     
    Show all issues
    Col: 21
     E128 continuation line under-indented for visual indent
    
  9. reviewboard/webapi/decorators.py (Diff revision 1)
     
     
    Show all issues
    Col: 21
     E128 continuation line under-indented for visual indent
    
  10. reviewboard/webapi/decorators.py (Diff revision 1)
     
     
    Show all issues
    Col: 21
     E128 continuation line under-indented for visual indent
    
  11. reviewboard/webapi/decorators.py (Diff revision 1)
     
     
    Show all issues
    Col: 21
     E128 continuation line under-indented for visual indent
    
  12. 
      
LE
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/decorators.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/decorators.py
    
    
  2. reviewboard/webapi/decorators.py (Diff revision 2)
     
     
    Show all issues
    Col: 41
     E127 continuation line over-indented for visual indent
    
  3. reviewboard/webapi/decorators.py (Diff revision 2)
     
     
    Show all issues
    Col: 37
     E127 continuation line over-indented for visual indent
    
  4. reviewboard/webapi/decorators.py (Diff revision 2)
     
     
    Show all issues
    Col: 33
     E127 continuation line over-indented for visual indent
    
  5. 
      
LE
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/decorators.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/decorators.py
    
    
  2. 
      
LE
david
  1. Can you make some changes to your review request description?

    What you currently have in the "Description" field is better for "Testing Done". The "Description" field should give an overview of what was wrong, what we want it to do, and how we changed the code to get there. See https://www.reviewboard.org/docs/codebase/dev/writing-good-descriptions/ for a longer explanation and a bunch of examples.

  2. reviewboard/webapi/decorators.py (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Python imports should be grouped into three categories, with a blank line between each:

    1. Standard library
    2. Third-party packages
    3. This module

    Plus we start each file with an extra one for unicode_literals

    In this case, that means you should have:

    from __future__ import unicode_literals
    
    import logging
    
    from django.http import HttpRequest
    ...
    
  3. reviewboard/webapi/decorators.py (Diff revision 3)
     
     
     
     
     
     
     
    Show all issues

    How about:

    '%s %s by %s: User does not have access to local site %s' % (request.method, request.path_info, request.user.username, local_site_name)
    
  4. reviewboard/webapi/decorators.py (Diff revision 3)
     
     
     
     
     
     
    Show all issues

    How about:

    '%s %s by %s: API token does not have access to local site %s' % (request.method, request.path_info, request.user.username, local_site_name)
    
  5. reviewboard/webapi/decorators.py (Diff revision 3)
     
     
     
     
     
    Show all issues

    How about:

    '%s %s by %s: API token is limited to a local site' % (request.method, request.path_info, request.user.username)
    
  6. 
      
LE
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/decorators.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/decorators.py
    
    
  2. reviewboard/webapi/decorators.py (Diff revision 4)
     
     
    Show all issues
    Col: 37
     E128 continuation line under-indented for visual indent
    
  3. reviewboard/webapi/decorators.py (Diff revision 4)
     
     
    Show all issues
    Col: 33
     E128 continuation line under-indented for visual indent
    
  4. reviewboard/webapi/decorators.py (Diff revision 4)
     
     
    Show all issues
    Col: 29
     E128 continuation line under-indented for visual indent
    
  5. 
      
LE
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/decorators.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/decorators.py
    
    
  2. reviewboard/webapi/decorators.py (Diff revision 5)
     
     
    Show all issues
    Col: 37
     E128 continuation line under-indented for visual indent
    
  3. reviewboard/webapi/decorators.py (Diff revision 5)
     
     
    Show all issues
    Col: 33
     E128 continuation line under-indented for visual indent
    
  4. reviewboard/webapi/decorators.py (Diff revision 5)
     
     
    Show all issues
    Col: 29
     E128 continuation line under-indented for visual indent
    
  5. 
      
LE
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/decorators.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/decorators.py
    
    
  2. reviewboard/webapi/decorators.py (Diff revision 6)
     
     
    Show all issues
    Col: 45
     E127 continuation line over-indented for visual indent
    
  3. reviewboard/webapi/decorators.py (Diff revision 6)
     
     
    Show all issues
    Col: 67
     W291 trailing whitespace
    
  4. reviewboard/webapi/decorators.py (Diff revision 6)
     
     
    Show all issues
    Col: 49
     E127 continuation line over-indented for visual indent
    
  5. reviewboard/webapi/decorators.py (Diff revision 6)
     
     
    Show all issues
    Col: 41
     E127 continuation line over-indented for visual indent
    
  6. reviewboard/webapi/decorators.py (Diff revision 6)
     
     
    Show all issues
    Col: 63
     W291 trailing whitespace
    
  7. reviewboard/webapi/decorators.py (Diff revision 6)
     
     
    Show all issues
    Col: 45
     E127 continuation line over-indented for visual indent
    
  8. reviewboard/webapi/decorators.py (Diff revision 6)
     
     
    Show all issues
    Col: 37
     E127 continuation line over-indented for visual indent
    
  9. reviewboard/webapi/decorators.py (Diff revision 6)
     
     
    Show all issues
    Col: 55
     W291 trailing whitespace
    
  10. reviewboard/webapi/decorators.py (Diff revision 6)
     
     
    Show all issues
    Col: 41
     E127 continuation line over-indented for visual indent
    
  11. 
      
LE
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/decorators.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/decorators.py
    
    
  2. reviewboard/webapi/decorators.py (Diff revision 7)
     
     
    Show all issues
    Col: 45
     E127 continuation line over-indented for visual indent
    
  3. reviewboard/webapi/decorators.py (Diff revision 7)
     
     
    Show all issues
    Col: 45
     E127 continuation line over-indented for visual indent
    
  4. reviewboard/webapi/decorators.py (Diff revision 7)
     
     
    Show all issues
    Col: 41
     E127 continuation line over-indented for visual indent
    
  5. reviewboard/webapi/decorators.py (Diff revision 7)
     
     
    Show all issues
    Col: 41
     E127 continuation line over-indented for visual indent
    
  6. reviewboard/webapi/decorators.py (Diff revision 7)
     
     
    Show all issues
    Col: 37
     E127 continuation line over-indented for visual indent
    
  7. reviewboard/webapi/decorators.py (Diff revision 7)
     
     
    Show all issues
    Col: 37
     E127 continuation line over-indented for visual indent
    
  8. 
      
LE
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/decorators.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/decorators.py
    
    
  2. 
      
chipx86
  1. 
      
  2. reviewboard/webapi/decorators.py (Diff revision 7)
     
     
     
     
     
     
    Show all issues

    The arguments within the (...) should be aligned (second and third rows shouldn't be indented), but actually, in the case of logging statements, each of these should just be an argument to the logging call.

    I'd suggest a rewording of these to make it more clear that the user is being denied access, rather than just that they don't have access. How about:

    logging.warning('User "%s" was denied access to Local Site "%s" (%s %s)',
                    request.user, local_site_name,
                    request.method, request.path_info)
    

    Same general format with the ones below.

    1. Format like this

      request.user, local_site_name,
      request.method, request.path_info)

      Might make the code goes beyond 80 characters and upset reviewbot.
      I am not sure I should override that decision

    2. Should we update to

      logging.warning('%s %s: User "%s" was denied access to Local Site "%s"'
      % (request.method,
      request.path_info,
      request.user.username,
      local_site_name))

      So that we have consitent format: Mehod URL: Message

  3. 
      
david
  1. I'm going to make some formatting changes and push this. Thanks!

  2. 
      
LE
Review request changed
Status:
Completed
Change Summary:
Pushed to release-2.5.x (0c3e9fd)