Fixed HTTP Digest backend unauthenticated user being added as reviewer issue- Bug-4929

Review Request #11980 — Created Jan. 22, 2022 and updated

Information

Review Board
release-4.0.x

Reviewers

Currently, non-existing username is allowed to be added as a reviewer in Review Board review request even though the username does not exist in the HTTP Digest .htpasswd file.
The fix involves creating a new helper function that checks if the username is found in the .htpasswd file and have both the authenticate and get_or_create_user functions call this helper function. It will not create a user if the username is not found, and hence it will not allow non-existing username to be added as a reviewer.

Added a test suite for the HTTPDigestBackend class that covers the three methods in the class.

As for manual checking, I attached a couple of files to show the output on the review request.
The first screenshot shows error when adding non-exsiting user as a reviewer.
The second screenshot shows that an existing user is added as a reviewer successfully.
The third screenshot shows all the users I have on the list.

Summary ID Author
Fixed HTTP Digest backend unauthenticated user being added as reviewer issue
d3f72f29b4240371814453c4871c6b4222765b9f sng06
Removed commented-out code
e94f5aaafab924bb77e19a0d9e5fe7dbd040698f sng06
Fixed styling issues
22357e51b94a85c0140cc1deb396a0d7451e21d4 sng06
Changed the argument name to match what
is used in the method definition
ee394285fea704e661d3bf31d5c6e46cfc27c7d7 sng06
Changed method name
f9b8b3d225be7264488ef766d7a05f98545510ac sng06
Added docstrings to htpasswd_get_user method
9090a5b60a84f135d00d8dfc6528025c6daae068 sng06
Deleted unecessary text in docstrings
53eb03ab994f835fc785b4247d3e1ffbcd3488de sng06
Added unit test suite for HTTPDigestBackend class
e0266bb57abd53ec55051eb42ab936c478b2d94e sng06
Fixed over-indented linting issue
48f095c6203d7c074de8ac61498efb922be7c1d0 sng06
Addressed code review feedback
b7239042929242c264824f95dfd3cdcdbc1eafd9 sng06
Changed the htpassword_user_info type to dict in the method parameter
12be423a6edd6f4869f4fbf748dc7d317137bf84 sng06

Description From Last Updated

E265 block comment should start with '# '

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E501 line too long (92 > 79 characters)

reviewbotreviewbot

E501 line too long (103 > 79 characters)

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E501 line too long (84 > 79 characters)

reviewbotreviewbot

E501 line too long (90 > 79 characters)

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W391 blank line at end of file

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E501 line too long (92 > 79 characters)

reviewbotreviewbot

E501 line too long (103 > 79 characters)

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E501 line too long (84 > 79 characters)

reviewbotreviewbot

E501 line too long (90 > 79 characters)

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W391 blank line at end of file

reviewbotreviewbot

We need to use the same name for the argument as what's used in the method definition itself (htpassword_user_info).

mike_conleymike_conley

E127 continuation line over-indented for visual indent

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

We need to add an entry for the new htpassword_user_info arg.

daviddavid

This should be shortened so it can fit in one line. If you need additional detail, add another paragraph.

daviddavid

Since this is unused, let's just get rid of it. It's needed in the get_or_create_user method because that's the API …

daviddavid

dictionary -> dict

daviddavid

Please put full-module imports above from imports within the section.

daviddavid

When we have to wrap the docstring for test cases, please put the final """ on its own line.

daviddavid

These can be condensed: self.spy_on(self.backend._htpasswd_get_user, call_fake=lambda obj: _user_info) self.spy_on(self.backend.get_or_create_user, call_fake=lambda obj, username, request: \ User(username=username))

daviddavid

When we have to wrap the docstring for test cases, please put the final """ on its own line.

daviddavid

Similar thing here as above.

daviddavid

These can use lambdas too.

daviddavid

These can use lambdas too

daviddavid

If you create this with NamedTemporaryFile(mode='w'), you don't need to mark these as bytestrings.

daviddavid

Same here.

daviddavid
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

sheenaNg
Review request changed
Commits:
Summary ID Author
Fixed HTTP Digest backend unauthenticated user being added as reviewer issue
d13eacca0a862c1e23fa36110fc74e98c33b7978 sng06
Fixed HTTP Digest backend unauthenticated user being added as reviewer issue
d13eacca0a862c1e23fa36110fc74e98c33b7978 sng06
Removed commented-out code
56e8ccaaa2f9c367f53fd7349c3c16e7bb9f9b53 sng06

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

sheenaNg
mike_conley
  1. Thanks, Sheena! Looking good so far - looking forward to seeing those tests. Just one note below.

  2. Show all issues

    We need to use the same name for the argument as what's used in the method definition itself (htpassword_user_info).

  3. 
      
sheenaNg
sheenaNg
Review request changed
Change Summary:

Changed the method name, added docstrings to htpasswd_get_user method, and added test suite for the HTTPDigestBackend class.

Testing Done:
~  

Will be adding some unit tests after the code sprint.

  ~

Added a test suite for the HTTPDigestBackend class that covers the three methods in the class.

   
   

As for manual checking, I attached a couple of files to show the output on the review request.

    The first screenshot shows error when adding non-exsiting user as a reviewer.
    The second screenshot shows that an existing user is added as a reviewer successfully.
    The third screenshot shows all the users I have on the list.

Commits:
Summary ID Author
Fixed HTTP Digest backend unauthenticated user being added as reviewer issue
d13eacca0a862c1e23fa36110fc74e98c33b7978 sng06
Removed commented-out code
56e8ccaaa2f9c367f53fd7349c3c16e7bb9f9b53 sng06
Fixed styling issues
e2ea3e38d38548a6f869ef16a48466dac8cfd862 sng06
Changed the argument name to match what
is used in the method definition
7cb6fc6f995d4609066500d96e293487c5eda816 sng06
Fixed HTTP Digest backend unauthenticated user being added as reviewer issue
d13eacca0a862c1e23fa36110fc74e98c33b7978 sng06
Removed commented-out code
56e8ccaaa2f9c367f53fd7349c3c16e7bb9f9b53 sng06
Fixed styling issues
e2ea3e38d38548a6f869ef16a48466dac8cfd862 sng06
Changed the argument name to match what
is used in the method definition
7cb6fc6f995d4609066500d96e293487c5eda816 sng06
Changed method name
a076a4b7ac8d58361c7ab1c96886a2cc59cad47d sng06
Added docstrings to htpasswd_get_user method
01fa738db6f34ec5b875a66896be9866d2e982c4 sng06
Deleted unecessary text in docstrings
b9c005b75065fabe4c4ee4133c603a76f376a240 sng06
Added unit test suite for HTTPDigestBackend class
381c2575d9abaeeab18a9e873b6fb4bca912bf2f sng06

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

sheenaNg
david
  1. 
      
  2. reviewboard/accounts/backends/http_digest.py (Diff revision 6)
     
     
     
     
    Show all issues

    We need to add an entry for the new htpassword_user_info arg.

  3. Show all issues

    This should be shortened so it can fit in one line. If you need additional detail, add another paragraph.

  4. Show all issues

    Since this is unused, let's just get rid of it. It's needed in the get_or_create_user method because that's the API contract that this class is fulfilling, but here this is just a helper method.

  5. Show all issues

    dictionary -> dict

  6. Show all issues

    Please put full-module imports above from imports within the section.

  7. Show all issues

    When we have to wrap the docstring for test cases, please put the final """ on its own line.

  8. reviewboard/accounts/tests/test_http_digest_auth_backend.py (Diff revision 6)
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    These can be condensed:

    self.spy_on(self.backend._htpasswd_get_user,
                call_fake=lambda obj: _user_info)
    self.spy_on(self.backend.get_or_create_user,
                call_fake=lambda obj, username, request: \
                    User(username=username))
    
  9. Show all issues

    When we have to wrap the docstring for test cases, please put the final """ on its own line.

  10. reviewboard/accounts/tests/test_http_digest_auth_backend.py (Diff revision 6)
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Similar thing here as above.

  11. Show all issues

    These can use lambdas too.

  12. Show all issues

    These can use lambdas too

  13. Show all issues

    If you create this with NamedTemporaryFile(mode='w'), you don't need to mark these as bytestrings.

  14. Show all issues

    Same here.

  15. 
      
sheenaNg
Review request changed
Change Summary:

Addressed code review feedback.

Commits:
Summary ID Author
Fixed HTTP Digest backend unauthenticated user being added as reviewer issue
d13eacca0a862c1e23fa36110fc74e98c33b7978 sng06
Removed commented-out code
56e8ccaaa2f9c367f53fd7349c3c16e7bb9f9b53 sng06
Fixed styling issues
e2ea3e38d38548a6f869ef16a48466dac8cfd862 sng06
Changed the argument name to match what
is used in the method definition
7cb6fc6f995d4609066500d96e293487c5eda816 sng06
Changed method name
a076a4b7ac8d58361c7ab1c96886a2cc59cad47d sng06
Added docstrings to htpasswd_get_user method
01fa738db6f34ec5b875a66896be9866d2e982c4 sng06
Deleted unecessary text in docstrings
b9c005b75065fabe4c4ee4133c603a76f376a240 sng06
Added unit test suite for HTTPDigestBackend class
381c2575d9abaeeab18a9e873b6fb4bca912bf2f sng06
Fixed over-indented linting issue
ba6b5d054bde6c084726791dd29abb96529fb716 sng06
Fixed HTTP Digest backend unauthenticated user being added as reviewer issue
d3f72f29b4240371814453c4871c6b4222765b9f sng06
Removed commented-out code
e94f5aaafab924bb77e19a0d9e5fe7dbd040698f sng06
Fixed styling issues
22357e51b94a85c0140cc1deb396a0d7451e21d4 sng06
Changed the argument name to match what
is used in the method definition
ee394285fea704e661d3bf31d5c6e46cfc27c7d7 sng06
Changed method name
f9b8b3d225be7264488ef766d7a05f98545510ac sng06
Added docstrings to htpasswd_get_user method
9090a5b60a84f135d00d8dfc6528025c6daae068 sng06
Deleted unecessary text in docstrings
53eb03ab994f835fc785b4247d3e1ffbcd3488de sng06
Added unit test suite for HTTPDigestBackend class
e0266bb57abd53ec55051eb42ab936c478b2d94e sng06
Fixed over-indented linting issue
48f095c6203d7c074de8ac61498efb922be7c1d0 sng06
Addressed code review feedback
b7239042929242c264824f95dfd3cdcdbc1eafd9 sng06
Changed the htpassword_user_info type to dict in the method parameter
12be423a6edd6f4869f4fbf748dc7d317137bf84 sng06

Checks run (2 succeeded)

flake8 passed.
JSHint passed.