Respond to API requests with 503 when read-only mode

Review Request #8657 — Created Jan. 21, 2017 and submitted

khp
Review Board
master
8648
8861, 8824, 8847, 8812, 8810, 8811, 8677
e04ec08...
reviewboard, students

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.

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

  • 0
  • 0
  • 20
  • 1
  • 21
Description From Last Updated
  1. 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
    
    
  2. reviewboard/webapi/base.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. reviewboard/webapi/tests/test_base.py (Diff revision 1)
     
     
     'SiteConfiguration' imported but unused
    
  4. reviewboard/webapi/tests/test_base.py (Diff revision 1)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  5. 
      
brennie
  1. 
      
  2. reviewboard/webapi/base.py (Diff revision 1)
     
     
     
     
     

    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).

    1. 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?

    2. 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.

    3. Thanks!!

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

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

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

      defaults.update({
          ...
          'site_read_only': False,
      
  4. reviewboard/webapi/base.py (Diff revision 1)
     
     

    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.

  5. 
      
  1. 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
    
    
  2. reviewboard/webapi/tests/test_base.py (Diff revision 2)
     
     
    Col: 39
     E703 statement ends with a semicolon
    
  3. 
      
  1. 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
    
    
  2. 
      
brennie
  1. 
      
  2. reviewboard/webapi/tests/test_base.py (Diff revision 3)
     
     

    Missing docstring.

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

    Docstrings for tests should begin with "Testing"

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

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

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

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

  5. 
      
  1. 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
    
    
  2. reviewboard/webapi/tests/test_base.py (Diff revision 4)
     
     
    Col: 45
     E201 whitespace after '['
    
  3. 
      
  1. 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
    
    
  2. 
      
brennie
  1. 
      
  2. Can you wrap your description at 72 characters?

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

    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."

    1. That sounds better =)

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

    "these"

  5. 
      
chipx86
  1. 
      
  2. reviewboard/webapi/base.py (Diff revision 5)
     
     

    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.

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

    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?

    1. 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?

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

    Blank line between these.

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

    Blank line between these.

  6. 
      
chipx86
  1. 
      
  2. reviewboard/webapi/tests/test_base.py (Diff revision 5)
     
     
     
     

    Two blank lines.

  3. 
      
  1. 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
    
    
  2. 
      
  1. 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
    
    
  2. 
      
brennie
  1. 
      
  2. reviewboard/admin/read_only.py (Diff revision 7)
     
     
     
     

    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.

    1. 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?

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

    Docstring.

  4. 
      
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-4.0.x (329d0a0)
Loading...