Summary

Add logging for decorator webapi_check_local_site when return PERMISSION_DENIED

Review Request #7868 — Created Jan. 15, 2016 and submitted March 31, 2016, 11:24 a.m.

Information

Owner

lehoangm

Repository

Review Board

Branch

master

Bugs

3108

Depends On

Reviewers

Groups

reviewboard

People

david

Description

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

Testing Done

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

Issues

Description	From	Last Updated
Col: 45 W291 trailing whitespace	reviewbot	Jan. 15, 2016, 12:48 p.m.
Col: 71 W291 trailing whitespace	reviewbot	Jan. 15, 2016, 12:48 p.m.
Col: 41 W291 trailing whitespace	reviewbot	Jan. 15, 2016, 12:48 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	Jan. 15, 2016, 12:48 p.m.
Col: 37 W291 trailing whitespace	reviewbot	Jan. 15, 2016, 12:48 p.m.
Col: 21 E128 continuation line under-indented for visual indent	reviewbot	Jan. 15, 2016, 12:48 p.m.
Col: 21 E128 continuation line under-indented for visual indent	reviewbot	Jan. 15, 2016, 12:48 p.m.
Col: 21 E128 continuation line under-indented for visual indent	reviewbot	Jan. 15, 2016, 12:48 p.m.
Col: 21 E128 continuation line under-indented for visual indent	reviewbot	Jan. 15, 2016, 12:48 p.m.
Col: 21 E128 continuation line under-indented for visual indent	reviewbot	Jan. 15, 2016, 12:49 p.m.
Col: 41 E127 continuation line over-indented for visual indent	reviewbot	Jan. 15, 2016, 12:50 p.m.
Col: 37 E127 continuation line over-indented for visual indent	reviewbot	Jan. 15, 2016, 12:50 p.m.
Col: 33 E127 continuation line over-indented for visual indent	reviewbot	Jan. 15, 2016, 12:50 p.m.
Python imports should be grouped into three categories, with a blank line between each: Standard library Third-party packages This module …	david	Jan. 15, 2016, 1:22 p.m.
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)	david	Jan. 15, 2016, 1:47 p.m.
How about: '%s %s by %s: API token does not have access to local site %s' % (request.method, request.path_info, request.user.username, …	david	Jan. 15, 2016, 1:47 p.m.
How about: '%s %s by %s: API token is limited to a local site' % (request.method, request.path_info, request.user.username)	david	Jan. 15, 2016, 1:47 p.m.
Col: 37 E128 continuation line under-indented for visual indent	reviewbot	Jan. 15, 2016, 1:54 p.m.
Col: 33 E128 continuation line under-indented for visual indent	reviewbot	Jan. 15, 2016, 1:54 p.m.
Col: 29 E128 continuation line under-indented for visual indent	reviewbot	Jan. 15, 2016, 1:54 p.m.
Col: 37 E128 continuation line under-indented for visual indent	reviewbot	Jan. 15, 2016, 1:55 p.m.
Col: 33 E128 continuation line under-indented for visual indent	reviewbot	Jan. 15, 2016, 1:55 p.m.
Col: 29 E128 continuation line under-indented for visual indent	reviewbot	Jan. 15, 2016, 1:55 p.m.
Col: 45 E127 continuation line over-indented for visual indent	reviewbot	Jan. 15, 2016, 1:59 p.m.
Col: 67 W291 trailing whitespace	reviewbot	Jan. 15, 2016, 1:59 p.m.
Col: 49 E127 continuation line over-indented for visual indent	reviewbot	Jan. 15, 2016, 1:59 p.m.
Col: 41 E127 continuation line over-indented for visual indent	reviewbot	Jan. 15, 2016, 1:59 p.m.
Col: 63 W291 trailing whitespace	reviewbot	Jan. 15, 2016, 1:59 p.m.
Col: 45 E127 continuation line over-indented for visual indent	reviewbot	Jan. 15, 2016, 1:59 p.m.
Col: 37 E127 continuation line over-indented for visual indent	reviewbot	Jan. 15, 2016, 1:59 p.m.
Col: 55 W291 trailing whitespace	reviewbot	Jan. 15, 2016, 1:59 p.m.
Col: 41 E127 continuation line over-indented for visual indent	reviewbot	Jan. 15, 2016, 1:59 p.m.
The arguments within the (...) should be aligned (second and third rows shouldn't be indented), but actually, in the case …	chipx86	Jan. 15, 2016, 2:03 p.m.
Col: 45 E127 continuation line over-indented for visual indent	reviewbot	Jan. 15, 2016, 2:03 p.m.
Col: 45 E127 continuation line over-indented for visual indent	reviewbot	Jan. 15, 2016, 2:03 p.m.
Col: 41 E127 continuation line over-indented for visual indent	reviewbot	Jan. 15, 2016, 2:03 p.m.
Col: 41 E127 continuation line over-indented for visual indent	reviewbot	Jan. 15, 2016, 2:03 p.m.
Col: 37 E127 continuation line over-indented for visual indent	reviewbot	Jan. 15, 2016, 2:03 p.m.
Col: 37 E127 continuation line over-indented for visual indent	reviewbot	Jan. 15, 2016, 2:03 p.m.

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/decorators.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/decorators.py

reviewboard/webapi/decorators.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 45
 W291 trailing whitespace
```
reviewboard/webapi/decorators.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 71
 W291 trailing whitespace
```
reviewboard/webapi/decorators.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 41
 W291 trailing whitespace
```
reviewboard/webapi/decorators.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (81 > 79 characters)
```
reviewboard/webapi/decorators.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 37
 W291 trailing whitespace
```
reviewboard/webapi/decorators.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 21
 E128 continuation line under-indented for visual indent
```
reviewboard/webapi/decorators.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 21
 E128 continuation line under-indented for visual indent
```
reviewboard/webapi/decorators.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 21
 E128 continuation line under-indented for visual indent
```
reviewboard/webapi/decorators.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 21
 E128 continuation line under-indented for visual indent
```
reviewboard/webapi/decorators.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 21
 E128 continuation line under-indented for visual indent
```

Change Summary:

Update styling issues

Diff:

Revision 2 (+20)

Show changes

reviewboard/webapi/decorators.py

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/decorators.py



Tool: Pyflakes
Processed Files:
    reviewboard/webapi/decorators.py

reviewboard/webapi/decorators.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 41
 E127 continuation line over-indented for visual indent
```
reviewboard/webapi/decorators.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 37
 E127 continuation line over-indented for visual indent
```
reviewboard/webapi/decorators.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 33
 E127 continuation line over-indented for visual indent
```

Change Summary:

Fixing over indented issues

Diff:

Revision 3 (+20)

Show changes

reviewboard/webapi/decorators.py

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/decorators.py



Tool: Pyflakes
Processed Files:
    reviewboard/webapi/decorators.py

Change Summary:: Change bug info
Bugs:: https://hellosplat.com/s/beanbag/tickets/3108/
3108

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.

LE

lehoangm Jan. 15, 2016, 1:57 p.m.
```
Updated
```

reviewboard/webapi/decorators.py (Diff revision 3)

The issue has been resolved. Show all issues

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

Standard library
Third-party packages
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
...

reviewboard/webapi/decorators.py (Diff revision 3)

The issue has been resolved. 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)

reviewboard/webapi/decorators.py (Diff revision 3)

The issue has been resolved. 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)

reviewboard/webapi/decorators.py (Diff revision 3)

The issue has been resolved. 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)

Change Summary:

Update description and logging message

Description:

~		Manually tested that the server does log events when
~		- User is not allowed too access the local site
	~	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
-		- User tries to access a local site with invalid token
-		- User tries to access with a valid token but with no local site name

Testing Done:

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

Diff:

Revision 4 (+13)

Show changes

reviewboard/webapi/decorators.py

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/decorators.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/decorators.py

reviewboard/webapi/decorators.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 37
 E128 continuation line under-indented for visual indent
```
reviewboard/webapi/decorators.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 33
 E128 continuation line under-indented for visual indent
```
reviewboard/webapi/decorators.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 29
 E128 continuation line under-indented for visual indent
```

Change Summary:

Update indentation

Diff:

Revision 5 (+13)

Show changes

reviewboard/webapi/decorators.py

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/decorators.py



Tool: Pyflakes
Processed Files:
    reviewboard/webapi/decorators.py

reviewboard/webapi/decorators.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Col: 37
 E128 continuation line under-indented for visual indent
```
reviewboard/webapi/decorators.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Col: 33
 E128 continuation line under-indented for visual indent
```
reviewboard/webapi/decorators.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Col: 29
 E128 continuation line under-indented for visual indent
```

Change Summary:

Update indentation

Diff:

Revision 6 (+16)

Show changes

reviewboard/webapi/decorators.py

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/decorators.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/decorators.py

reviewboard/webapi/decorators.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Col: 45
 E127 continuation line over-indented for visual indent
```
reviewboard/webapi/decorators.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Col: 67
 W291 trailing whitespace
```
reviewboard/webapi/decorators.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Col: 49
 E127 continuation line over-indented for visual indent
```
reviewboard/webapi/decorators.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Col: 41
 E127 continuation line over-indented for visual indent
```
reviewboard/webapi/decorators.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Col: 63
 W291 trailing whitespace
```
reviewboard/webapi/decorators.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Col: 45
 E127 continuation line over-indented for visual indent
```
reviewboard/webapi/decorators.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Col: 37
 E127 continuation line over-indented for visual indent
```
reviewboard/webapi/decorators.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Col: 55
 W291 trailing whitespace
```
reviewboard/webapi/decorators.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Col: 41
 E127 continuation line over-indented for visual indent
```

Diff:

Revision 7 (+16)

Show changes

reviewboard/webapi/decorators.py

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/decorators.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/decorators.py

reviewboard/webapi/decorators.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Col: 45
 E127 continuation line over-indented for visual indent
```
reviewboard/webapi/decorators.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Col: 45
 E127 continuation line over-indented for visual indent
```
reviewboard/webapi/decorators.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Col: 41
 E127 continuation line over-indented for visual indent
```
reviewboard/webapi/decorators.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Col: 41
 E127 continuation line over-indented for visual indent
```
reviewboard/webapi/decorators.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Col: 37
 E127 continuation line over-indented for visual indent
```
reviewboard/webapi/decorators.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Col: 37
 E127 continuation line over-indented for visual indent
```

Change Summary:

pep8 update

Diff:

Revision 8 (+16)

Show changes

reviewboard/webapi/decorators.py

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/decorators.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/decorators.py

Fix it!

reviewboard/webapi/decorators.py (Diff revision 7)

An issue was opened. 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.

lehoangm Jan. 15, 2016, 2:09 p.m.

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

lehoangm Jan. 15, 2016, 2:13 p.m.

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

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

Status:: Completed
Change Summary:: Pushed to release-2.5.x (0c3e9fd)