-
-
djblets/webapi/resources/base.py (Diff revision 1) Col: 13 E128 continuation line under-indented for visual indent
Added debug logging for djblets webapi permission errors.
Review Request #7927 — Created Jan. 30, 2016 and submitted
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 |
reviewbot | |
Saying "view" might be confusing because this is part of the API. Perhaps just say "API resource"? We should probably … |
david | |
Can you make this message more consistent with the other one? |
david | |
Can we write the logging message sa: "%s %s: user %s does not have permission for the "%s" method on … |
brennie | |
Likewise, can we write the message as: "%s %s: user %s does not have permission to access this resource." |
brennie | |
(Opening an issue here because we don't have "general comments" enabled on this instance yet). There's a typo in your … |
mike_conley | |
Your format string doesn't quite match the arguments. The first one is the method, and the fourth one is a … |
david | |
We probably don't need to mention the method twice. Perhaps: logging.debug('%s %s: user %s is missing required permission "%s"', request.method, … |
david | |
We don't need to list the method twice. |
david |
Testing Done: |
|
---|
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+10) |
-
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
-
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.
-
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). -
djblets/webapi/resources/base.py (Diff revision 2) Can you make this message more consistent with the other one?
Description: |
|
||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Bugs: |
|
||||||||||||
Commit: |
|
||||||||||||
Diff: |
Revision 3 (+11) |
-
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
-
-
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."
-
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."
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+11) |
-
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
-
This looks good to me - just a small typo in the description. Can you please update that?
-
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!
Description: |
|
---|
-
-
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.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+11) |
-
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
-
-
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)
-