Summary:

Respond to API requests with 503 when read-only mode

Review Request #8657 — Created Jan. 21, 2017 and submitted Jan. 10, 2018, 10:25 p.m.

Information
Owner:	khp
Repository:	Review Board
Branch:	master
Bugs:
Depends On:	~~8648~~
Blocks:	8861, 8824, 8847, 8812, 8810, 8811, 8677
Commit:	e04ec08...
Reviewers
Groups:	reviewboard, students
People:

Description

Read-only mode is a setting an admin can enable to prevent writes to

the database. This can be used when the site is under maintenence or

being upgraded. The previous commit added the siteconfig attribute and

a checkbox to toggle the mode. This commit adds a new 503 error to

Review Board's webapi. Furthermore, all requests that hit RB's API

using PUT, POST, DELETE methods while read-only mode is on, and

the user is not a superuser, will return the new 503 error. A utility

file read_only.py has been added which deals with the logic and

caching of determining when a user should be affected by read-only

mode.

Testing Done

Added new Python unit tests that send API requests as superuser and
non-superuser
Ran the Python unit test suite
Check that modifying/deleting review requests and comments works
properly

Issues

Description	From	Last Updated
Can you wrap your description at 72 characters?	brennie	March 14, 2017, 9:16 a.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Jan. 22, 2017, 4:20 p.m.
request.user should always be an object. If the user is logged in, it will be a User. Otherwise it will …	brennie	Jan. 30, 2017, 9:03 p.m.
This needs a default value in reviewboard/admin/siteconfig.py	brennie	Jan. 22, 2017, 4:31 p.m.
The performance difference between set and tuple here is probably negligible, but we should probably just use tuple (i.e., ('POST', …	brennie	Jan. 22, 2017, 4:31 p.m.
'SiteConfiguration' imported but unused	reviewbot	Jan. 22, 2017, 4:21 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Jan. 22, 2017, 4:22 p.m.
Col: 39 E703 statement ends with a semicolon	reviewbot	Feb. 20, 2017, 11:09 p.m.
Missing docstring.	brennie	March 1, 2017, 4:14 a.m.
Docstrings for tests should begin with "Testing"	brennie	March 1, 2017, 4:15 a.m.
Can you pass the boolean params as keywords? It makes the test cases clearer.	brennie	March 1, 2017, 4:33 a.m.
Col: 45 E201 whitespace after '['	reviewbot	March 12, 2017, 7:17 a.m.
How about "If read-only mode is enabled, all PUT, POST, and DELETE requests will be rejected with a 503 Service …	brennie	March 14, 2017, 9:17 a.m.
"these"	brennie	March 14, 2017, 9:17 a.m.
There are other mutable types of requests that may be supported down the road. I'd change this to request.method not …	chipx86	March 14, 2017, 9:23 a.m.
Can you verify that all unit tests for Review Board pass? My concern is that the siteconfig changes made in …	chipx86	March 16, 2017, 7:57 a.m.
Blank line between these.	chipx86	March 14, 2017, 9:22 a.m.
Blank line between these.	chipx86	March 14, 2017, 9:22 a.m.
Two blank lines.	chipx86	March 14, 2017, 9:22 a.m.
I don't know that we need to cache this, since SiteConfiguration.objects.get_current() will only hit the database the first time its …	brennie	April 6, 2017, 7:53 p.m.
Docstring.	brennie	April 6, 2017, 9:32 a.m.

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/base.py
    reviewboard/webapi/tests/test_base.py
    reviewboard/webapi/errors.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/base.py
    reviewboard/webapi/tests/test_base.py
    reviewboard/webapi/errors.py

reviewboard/webapi/base.py (Diff revision 1)

Show all issues

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

reviewboard/webapi/tests/test_base.py (Diff revision 1)
Show all issues
```
 'SiteConfiguration' imported but unused
```
reviewboard/webapi/tests/test_base.py (Diff revision 1)
Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```

reviewboard/webapi/base.py (Diff revision 1)

Show all issues

request.user should always be an object. If the user is logged in, it will be a User. Otherwise it will be an AnonymousUser (for which is_superuser will be False).

khp Jan. 22, 2017, 4:32 p.m.

Hey Barret, I added this in because the unit tests were failing since the are manually created and do not include a user (as far as I could tell).

AttributeError: 'WSGIRequest' object has no attribute 'user'

Not only my new test, but at least the other tests in the file were failing without the check. Any alternative ideas?

brennie Jan. 25, 2017, 9:28 p.m.

So when you use a RequestFactory, the default middleware do not get added to the request. Review Board uses AuthenticationMiddleware which adds the user attribute to requests. In the feature tests you will have to add request.user = AnonymousUser() aroun line 338.

KH

khp Jan. 29, 2017, 6:58 a.m.
```
Thanks!!
```

reviewboard/webapi/base.py (Diff revision 1)

Show all issues

This needs a default value in reviewboard/admin/siteconfig.py

khp Jan. 22, 2017, 4:32 p.m.

I believe this is set in the previous commit. There, I set

defaults.update({
    ...
    'site_read_only': False,

reviewboard/webapi/base.py (Diff revision 1)

Show all issues

The performance difference between set and tuple here is probably negligible, but we should probably just use tuple (i.e., ('POST', 'PUT', 'DELETE')).

set is (I believe) backed by a red-black tree and so will have to allocate memory for the nodes, as well as do bunch of other overhead work.

KH

khp Jan. 22, 2017, 4:32 p.m.
```
Done.
```

Change Summary:

Updated according to comments; formatting and update test by adding user to requests

Commit:

-	2f476e2153e798fcc9e0865c05a7c996aa3ae595
+	e04ec08999f4d629fd32c727a7316a130b175dd0

Diff:

Revision 2 (+117 -6)

Show changes

	reviewboard/admin/forms.py
	reviewboard/admin/siteconfig.py
	reviewboard/webapi/base.py
	reviewboard/webapi/errors.py
	reviewboard/webapi/tests/test_base.py

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/admin/siteconfig.py
    reviewboard/admin/forms.py
    reviewboard/webapi/tests/test_base.py
    reviewboard/webapi/errors.py
    reviewboard/webapi/base.py



Tool: Pyflakes
Processed Files:
    reviewboard/admin/siteconfig.py
    reviewboard/admin/forms.py
    reviewboard/webapi/tests/test_base.py
    reviewboard/webapi/errors.py
    reviewboard/webapi/base.py

reviewboard/webapi/tests/test_base.py (Diff revision 2)
Show all issues
```
Col: 39
 E703 statement ends with a semicolon
```

Change Summary:

Removed semi-colon

Diff:

Revision 3 (+108 -5)

Show changes

	reviewboard/webapi/base.py
	reviewboard/webapi/errors.py
	reviewboard/webapi/tests/test_base.py

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/base.py
    reviewboard/webapi/tests/test_base.py
    reviewboard/webapi/errors.py



Tool: Pyflakes
Processed Files:
    reviewboard/webapi/base.py
    reviewboard/webapi/tests/test_base.py
    reviewboard/webapi/errors.py

reviewboard/webapi/tests/test_base.py (Diff revision 3)
Show all issues
```
Missing docstring.
```
1. KH
  
  khp March 1, 2017, 9:32 a.m.
  Done.

reviewboard/webapi/tests/test_base.py (Diff revision 3)

Show all issues

Docstrings for tests should begin with "Testing"

khp March 1, 2017, 9:32 a.m.

Done. Reworded all of them to match the change to "Testing" better.

reviewboard/webapi/tests/test_base.py (Diff revision 3)

Show all issues

Can you pass the boolean params as keywords? It makes the test cases clearer.

khp March 1, 2017, 9:32 a.m.

Thanks, I was thinking that too. Is the way its done in the new diff ok?

Change Summary:

Updated with responses with Barret's comments. Also updated tests to include an argument for expected outcome instead of coding an expected behaviour into the test.

Diff:

Revision 4 (+140 -5)

Show changes

	reviewboard/webapi/base.py
	reviewboard/webapi/errors.py
	reviewboard/webapi/tests/test_base.py

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/base.py
    reviewboard/webapi/tests/test_base.py
    reviewboard/webapi/errors.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/base.py
    reviewboard/webapi/tests/test_base.py
    reviewboard/webapi/errors.py

reviewboard/webapi/tests/test_base.py (Diff revision 4)
Show all issues
```
Col: 45
 E201 whitespace after '['
```

Change Summary:

Fix spacing

Diff:

Revision 5 (+140 -5)

Show changes

	reviewboard/webapi/base.py
	reviewboard/webapi/errors.py
	reviewboard/webapi/tests/test_base.py

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/base.py
    reviewboard/webapi/tests/test_base.py
    reviewboard/webapi/errors.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/base.py
    reviewboard/webapi/tests/test_base.py
    reviewboard/webapi/errors.py

Show all issues

Can you wrap your description at 72 characters?

KH

khp March 14, 2017, 9:42 a.m.
```
Done.
```

reviewboard/webapi/base.py (Diff revision 5)

Show all issues

How about

"If read-only mode is enabled, all PUT, POST, and DELETE requests will be rejected with a 503 Service Unavailable unless the user is a superuser."

KH

khp March 14, 2017, 9:42 a.m.
```
That sounds better =)
```

reviewboard/webapi/base.py (Diff revision 5)
Show all issues
```
"these"
```

reviewboard/webapi/base.py (Diff revision 5)

Show all issues

There are other mutable types of requests that may be supported down the road. I'd change this to request.method not in ('GET', 'HEAD', 'OPTIONS'), since those are all safe.

reviewboard/webapi/tests/test_base.py (Diff revision 5)

Show all issues

Can you verify that all unit tests for Review Board pass? My concern is that the siteconfig changes made in the unit tests could leak. Just in case, could you have the tearDown() method reset site_read_only back to defaults?

khp March 14, 2017, 9:42 a.m.

Ran ./tests/runtests.py and js-tests with the new changes and everything looks fine other than in py-tests, there are 2 failed tests due to perforce not being setup and 107 skipped. Is that sufficient?

reviewboard/webapi/tests/test_base.py (Diff revision 5)
Show all issues
```
Blank line between these.
```
reviewboard/webapi/tests/test_base.py (Diff revision 5)
Show all issues
```
Blank line between these.
```

reviewboard/webapi/tests/test_base.py (Diff revision 5)
Show all issues
```
Two blank lines.
```

Change Summary:

Revised according to comments.

Description:

~		Read-only mode is a setting an admin can enable to prevent writes to the database. This can be used when the site is under maintenence or being upgraded. The previous commit added the siteconfig attribute and a checkbox to toggle the mode. This commit adds a new 503 error to Review Board's webapi. Furthermore, all requests that hit RB's API using `PUT`, `POST`, `DELETE` methods while read-only mode is on, and the user is not a superuser, will return the new 503 error.
	~	Read-only mode is a setting an admin can enable to prevent writes to
	+	the database. This can be used when the site is under maintenence or
	+	being upgraded. The previous commit added the siteconfig attribute and
	+	a checkbox to toggle the mode. This commit adds a new 503 error to
	+	Review Board's webapi. Furthermore, all requests that hit RB's API
	+	using `PUT`, `POST`, `DELETE` methods while read-only mode is on, and
	+	the user is not a superuser, will return the new 503 error.

Testing Done:

~		Added new Python unit tests that send API requests as superuser and non-superuser
	~	Added new Python unit tests that send API requests as superuser and
	+	non-superuser
		Ran the Python unit test suite
~		Check that modifying/deleting review requests and comments works properly
	~	Check that modifying/deleting review requests and comments works
	+	properly

Diff:

Revision 6 (+151 -5)

Show changes

	reviewboard/webapi/base.py
	reviewboard/webapi/errors.py
	reviewboard/webapi/tests/test_base.py

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/base.py
    reviewboard/webapi/tests/test_base.py
    reviewboard/webapi/errors.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/base.py
    reviewboard/webapi/tests/test_base.py
    reviewboard/webapi/errors.py

Change Summary:

Moved read_only.py from next diff into this one.

Diff:

Revision 7 (+178 -5)

Show changes

	reviewboard/admin/read_only.py
	reviewboard/webapi/base.py
	reviewboard/webapi/errors.py
	reviewboard/webapi/tests/test_base.py

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/base.py
    reviewboard/admin/read_only.py
    reviewboard/webapi/tests/test_base.py
    reviewboard/webapi/errors.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/base.py
    reviewboard/admin/read_only.py
    reviewboard/webapi/tests/test_base.py
    reviewboard/webapi/errors.py

Description:

		Read-only mode is a setting an admin can enable to prevent writes to
		the database. This can be used when the site is under maintenence or
		being upgraded. The previous commit added the siteconfig attribute and
		a checkbox to toggle the mode. This commit adds a new 503 error to
		Review Board's webapi. Furthermore, all requests that hit RB's API
		using `PUT`, `POST`, `DELETE` methods while read-only mode is on, and
~		the user is not a superuser, will return the new 503 error.
	~	the user is not a superuser, will return the new 503 error. A utility
	+	file `read_only.py` has been added which deals with the logic and
	+	caching of determining when a user should be affected by read-only
	+	mode.

Ship it!

reviewboard/admin/read_only.py (Diff revision 7)

Show all issues

I don't know that we need to cache this, since SiteConfiguration.objects.get_current() will only hit the database the first time its called. Every other call is cached.

khp April 6, 2017, 9:51 a.m.

Hey Barret, this optimization was something Christian suggested here: https://reviews.reviewboard.org/r/8677/#comment36453
Since this potentially gets called multiple times per page load, perhaps it's worth keeping this around?

reviewboard/webapi/tests/test_base.py (Diff revision 7)
Show all issues
```
Docstring.
```

Change Summary:

Added docstring to a test method

Diff:

Revision 8 (+199 -5)

Show changes

	reviewboard/admin/read_only.py
	reviewboard/webapi/base.py
	reviewboard/webapi/errors.py
	reviewboard/webapi/tests/test_base.py

Checks run (3 succeeded)

JSHint passed.

PEP8 Style Checker passed.

Pyflakes passed.

Status: Closed (submitted)

Change Summary:

Pushed to release-4.0.x (329d0a0)