Summary

Added debug logging for djblets webapi permission errors.

Review Request #7927 — Created Jan. 30, 2016 and submitted March 31, 2016, 11:16 a.m.

Information

Owner

aquafemi

Repository

Djblets

Branch

master

Bugs

3108

Depends On

Commit

5fdd6eb...

Reviewers

Groups

djblets, students

People

Description

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.

Testing Done

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

Issues

Description	From	Last Updated
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	Jan. 30, 2016, 11:28 a.m.
Saying "view" might be confusing because this is part of the API. Perhaps just say "API resource"? We should probably …	david	Jan. 30, 2016, 6:47 p.m.
Can you make this message more consistent with the other one?	david	Jan. 30, 2016, 6:47 p.m.
Can we write the logging message sa: "%s %s: user %s does not have permission for the "%s" method on …	brennie	Jan. 31, 2016, 7:20 a.m.
Likewise, can we write the message as: "%s %s: user %s does not have permission to access this resource."	brennie	Jan. 31, 2016, 7:20 a.m.
(Opening an issue here because we don't have "general comments" enabled on this instance yet). There's a typo in your …	mike_conley	Jan. 31, 2016, 7:53 a.m.
Your format string doesn't quite match the arguments. The first one is the method, and the fourth one is a …	david	Jan. 31, 2016, 9 p.m.
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	March 11, 2016, 4:09 p.m.
We don't need to list the method twice.	david	March 11, 2016, 4:09 p.m.

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/resources/base.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 13
 E128 continuation line under-indented for visual indent
```

Testing Done:: ~
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
~
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

Commit:

1be2e6cca2472ee04444f293ad877f30483978f4

2f2ad5cfab5936d54a78e31b173060f3e1619919

Diff:

Revision 2 (+10)

Show changes

	djblets/webapi/decorators.py
	djblets/webapi/resources/base.py

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

Groups:: students

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)

The issue has been resolved. Show all issues

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)
The issue has been resolved. Show all issues
```
Can you make this message more consistent with the other one?
```

Description:

~		Added debug logging for djblets webapi permission errors.
	~	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 aunthenticated user doesn't have proper permissions to access an API resource.

Bugs:

3108

Commit:

2f2ad5cfab5936d54a78e31b173060f3e1619919

d1faf0df2597399e8c10048dbe47781dc2d09e14

Diff:

Revision 3 (+11)

Show changes

	djblets/webapi/decorators.py
	djblets/webapi/resources/base.py

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)

The issue has been resolved. Show all issues

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)

The issue has been resolved. Show all issues

Likewise, can we write the message as:

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

Commit:

d1faf0df2597399e8c10048dbe47781dc2d09e14

4899e640e92539c5b8d93f42be99e6acc4f64ed0

Diff:

Revision 4 (+11)

Show changes

	djblets/webapi/decorators.py
	djblets/webapi/resources/base.py

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

Ship it!

This looks good to me - just a small typo in the description. Can you please update that?

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

The issue has been resolved. Show all issues

(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!

AQ

aquafemi Jan. 31, 2016, 7:53 a.m.
```
Whoops!
```

Description:: 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 aunthenticated user doesn't have proper permissions to access an API resource.
~
Both logging statements covers cases where an authenticated user doesn't have proper permissions to access an API resource.

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

The issue has been resolved. Show all issues

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.

aquafemi Jan. 31, 2016, 2:07 p.m.

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

Commit:

4899e640e92539c5b8d93f42be99e6acc4f64ed0

5fdd6eb84540965cbf43821ce663e82f1423ef70

Diff:

Revision 5 (+11)

Show changes

	djblets/webapi/decorators.py
	djblets/webapi/resources/base.py

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)

The issue has been resolved. Show all issues

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)

aquafemi Feb. 2, 2016, 5:18 p.m.

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.

djblets/webapi/resources/base.py (Diff revision 5)
The issue has been resolved. Show all issues
```
We don't need to list the method twice.
```

Diff:

Revision 6 (+10)

Show changes

	djblets/webapi/decorators.py
	djblets/webapi/resources/base.py

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

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-0.9.x (309d4be)

~		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
	~	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