Added debug logging for djblets webapi permission errors.

Review Request #7927 — Created Jan. 30, 2016 and submitted

aquafemi
Djblets
master
3108
5fdd6eb...
djblets, students

What this entailed was adding two logging statements at the debug level that log when a PERMISSION_DENIED error is returned. Before this diff, it wasn't clear enough from the server log who was denied from what. This diff attempts to make security auditing a bit more explicit and debuggable.

Both logging statements covers cases where an authenticated user doesn't have proper permissions to access an API resource.

Manual Testing:

Installed a reviewboard extension (rbmotd) and tried to enable it through an api call with bad permissions

Made an api call to access a draft that didn't belong to that user

Description From Last Updated

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

reviewbotreviewbot

Saying "view" might be confusing because this is part of the API. Perhaps just say "API resource"? We should probably ...

daviddavid

Can you make this message more consistent with the other one?

daviddavid

Can we write the logging message sa: "%s %s: user %s does not have permission for the "%s" method on ...

brenniebrennie

Likewise, can we write the message as: "%s %s: user %s does not have permission to access this resource."

brenniebrennie

(Opening an issue here because we don't have "general comments" enabled on this instance yet). There's a typo in your ...

mike_conleymike_conley

Your format string doesn't quite match the arguments. The first one is the method, and the fourth one is a ...

daviddavid

We probably don't need to mention the method twice. Perhaps: logging.debug('%s %s: user %s is missing required permission "%s"', request.method, ...

daviddavid

We don't need to list the method twice.

daviddavid
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        djblets/webapi/resources/base.py
        djblets/webapi/decorators.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        djblets/webapi/resources/base.py
        djblets/webapi/decorators.py
    
    
  2. djblets/webapi/resources/base.py (Diff revision 1)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  3. 
      
AQ
AQ
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        djblets/webapi/resources/base.py
        djblets/webapi/decorators.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        djblets/webapi/resources/base.py
        djblets/webapi/decorators.py
    
    
  2. 
      
AQ
david
  1. Please add the bug number to the "bugs" field.

    I'd also like you to flesh out your description a little bit more to explain why we're adding this logging (for security audit purposes) and mention which particular cases are covered.

  2. djblets/webapi/decorators.py (Diff revision 2)
     
     

    Saying "view" might be confusing because this is part of the API. Perhaps just say "API resource"?

    We should probably also log which permission was required (the perm variable).

  3. djblets/webapi/resources/base.py (Diff revision 2)
     
     

    Can you make this message more consistent with the other one?

  4. 
      
AQ
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        djblets/webapi/resources/base.py
        djblets/webapi/decorators.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        djblets/webapi/resources/base.py
        djblets/webapi/decorators.py
    
    
  2. 
      
brennie
  1. 
      
  2. djblets/webapi/decorators.py (Diff revision 3)
     
     
     
     
     

    Can we write the logging message sa:

    "%s %s: user %s does not have permission for the "%s" method on this resource."

  3. djblets/webapi/resources/base.py (Diff revision 3)
     
     
     
     
     

    Likewise, can we write the message as:

    "%s %s: user %s does not have permission to access this resource."

  4. 
      
AQ
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        djblets/webapi/resources/base.py
        djblets/webapi/decorators.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        djblets/webapi/resources/base.py
        djblets/webapi/decorators.py
    
    
  2. 
      
mike_conley
  1. This looks good to me - just a small typo in the description. Can you please update that?

  2. djblets/webapi/decorators.py (Diff revision 4)
     
     

    (Opening an issue here because we don't have "general comments" enabled on this instance yet). There's a typo in your Description, "aunthenticated" -> "authenticated".

    Otherwise, this looks good to me!

  3. 
      
AQ
david
  1. 
      
  2. djblets/webapi/decorators.py (Diff revision 4)
     
     
     
     
     

    Your format string doesn't quite match the arguments. The first one is the method, and the fourth one is a permission name which is missing.

    1. I see what you're saying. I'm logging the method and the path in the first sentence to keep consistency with the other log statement, but I think I have a way that can be consistent and informative. Can you take another look to see if my new change is correct and more clear? Thanks

  3. 
      
AQ
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        djblets/webapi/resources/base.py
        djblets/webapi/decorators.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        djblets/webapi/resources/base.py
        djblets/webapi/decorators.py
    
    
  2. 
      
david
  1. 
      
  2. djblets/webapi/decorators.py (Diff revision 5)
     
     
     
     
     

    We probably don't need to mention the method twice. Perhaps:

    logging.debug('%s %s: user %s is missing required permission "%s"',
                  request.method, request.path, request.user.username,
                  perm)
    
    1. I was going for a message that would show what the call was, "GET /path/to/resource" and also put it in the form of a readable sentence, "User did not have permission 'x' to this resource". I feel like this way it will stand out from a lot of the other stuff in the log.

  3. djblets/webapi/resources/base.py (Diff revision 5)
     
     
     
     
     

    We don't need to list the method twice.

  4. 
      
AQ
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        djblets/webapi/resources/base.py
        djblets/webapi/decorators.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        djblets/webapi/resources/base.py
        djblets/webapi/decorators.py
    
    
  2. 
      
brennie
  1. Ship It!
  2. 
      
david
  1. Ship It!
  2. 
      
AQ
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-0.9.x (309d4be)
Loading...