Summary:

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

Review Request #11980 — Created Jan. 22, 2022 and updated April 7, 2022, 12:27 a.m.

Information
Owner:	sheenaNg
Repository:	Review Board
Branch:	release-4.0.x
Bugs:
Depends On:
Reviewers
Groups:	reviewboard
People:

Description

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.

Testing Done

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		Author
	Fixed HTTP Digest backend unauthenticated user being added as reviewer issue	sng06
	Removed commented-out code	sng06
	Fixed styling issues	sng06
	Changed the argument name to match what	sng06
	Changed method name	sng06
	Added docstrings to htpasswd_get_user method	sng06
	Deleted unecessary text in docstrings	sng06
	Added unit test suite for HTTPDigestBackend class	sng06
	Fixed over-indented linting issue	sng06
	Addressed code review feedback	sng06
	Changed the htpassword_user_info type to dict in the method parameter	sng06

Screenshots

Files

Issues

Description	From	Last Updated
E265 block comment should start with '# '	reviewbot	Jan. 22, 2022, 1:46 p.m.
W293 blank line contains whitespace	reviewbot	Jan. 22, 2022, 1:46 p.m.
E501 line too long (92 > 79 characters)	reviewbot	Jan. 22, 2022, 1:46 p.m.
E501 line too long (103 > 79 characters)	reviewbot	Jan. 22, 2022, 1:46 p.m.
W293 blank line contains whitespace	reviewbot	Jan. 22, 2022, 1:46 p.m.
E501 line too long (80 > 79 characters)	reviewbot	Jan. 22, 2022, 1:46 p.m.
E501 line too long (84 > 79 characters)	reviewbot	Jan. 22, 2022, 1:46 p.m.
E501 line too long (90 > 79 characters)	reviewbot	Jan. 22, 2022, 1:46 p.m.
W293 blank line contains whitespace	reviewbot	Jan. 22, 2022, 1:46 p.m.
W293 blank line contains whitespace	reviewbot	Jan. 22, 2022, 1:46 p.m.
W391 blank line at end of file	reviewbot	Jan. 22, 2022, 1:46 p.m.
W293 blank line contains whitespace	reviewbot	Jan. 22, 2022, 5:53 p.m.
E501 line too long (92 > 79 characters)	reviewbot	Jan. 22, 2022, 5:53 p.m.
E501 line too long (103 > 79 characters)	reviewbot	Jan. 22, 2022, 5:53 p.m.
W293 blank line contains whitespace	reviewbot	Jan. 22, 2022, 5:53 p.m.
E501 line too long (84 > 79 characters)	reviewbot	Jan. 22, 2022, 5:53 p.m.
E501 line too long (90 > 79 characters)	reviewbot	Jan. 22, 2022, 5:53 p.m.
W293 blank line contains whitespace	reviewbot	Jan. 22, 2022, 5:53 p.m.
W293 blank line contains whitespace	reviewbot	Jan. 22, 2022, 5:53 p.m.
W391 blank line at end of file	reviewbot	Jan. 22, 2022, 5:51 p.m.
We need to use the same name for the argument as what's used in the method definition itself (htpassword_user_info).	mike_conley	Jan. 23, 2022, 11:06 a.m.
E127 continuation line over-indented for visual indent	reviewbot	Jan. 26, 2022, 10:42 p.m.
E127 continuation line over-indented for visual indent	reviewbot	Jan. 26, 2022, 10:42 p.m.
E127 continuation line over-indented for visual indent	reviewbot	Jan. 26, 2022, 10:42 p.m.
We need to add an entry for the new htpassword_user_info arg.	david	April 7, 2022, 12:26 a.m.
This should be shortened so it can fit in one line. If you need additional detail, add another paragraph.	david	April 7, 2022, 12:26 a.m.
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 …	david	April 7, 2022, 12:26 a.m.
dictionary -> dict	david	April 7, 2022, 12:26 a.m.
Please put full-module imports above from imports within the section.	david	April 7, 2022, 12:26 a.m.
When we have to wrap the docstring for test cases, please put the final """ on its own line.	david	April 7, 2022, 12:26 a.m.
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))	david	April 7, 2022, 12:27 a.m.
When we have to wrap the docstring for test cases, please put the final """ on its own line.	david	April 7, 2022, 12:27 a.m.
Similar thing here as above.	david	April 7, 2022, 12:27 a.m.
These can use lambdas too.	david	April 7, 2022, 12:27 a.m.
These can use lambdas too	david	April 7, 2022, 12:27 a.m.
If you create this with NamedTemporaryFile(mode='w'), you don't need to mark these as bytestrings.	david	April 7, 2022, 12:27 a.m.
Same here.	david	April 7, 2022, 12:27 a.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/accounts/backends/http_digest.py (Diff revision 1)
Show all issues
```
E265 block comment should start with '# '
```
reviewboard/accounts/backends/http_digest.py (Diff revision 1)
Show all issues
```
W293 blank line contains whitespace
```
reviewboard/accounts/backends/http_digest.py (Diff revision 1)
Show all issues
```
E501 line too long (92 > 79 characters)
```
reviewboard/accounts/backends/http_digest.py (Diff revision 1)
Show all issues
```
E501 line too long (103 > 79 characters)
```
reviewboard/accounts/backends/http_digest.py (Diff revision 1)
Show all issues
```
W293 blank line contains whitespace
```
reviewboard/accounts/backends/http_digest.py (Diff revision 1)
Show all issues
```
E501 line too long (80 > 79 characters)
```
reviewboard/accounts/backends/http_digest.py (Diff revision 1)
Show all issues
```
E501 line too long (84 > 79 characters)
```
reviewboard/accounts/backends/http_digest.py (Diff revision 1)
Show all issues
```
E501 line too long (90 > 79 characters)
```
reviewboard/accounts/backends/http_digest.py (Diff revision 1)
Show all issues
```
W293 blank line contains whitespace
```
reviewboard/accounts/backends/http_digest.py (Diff revision 1)
Show all issues
```
W293 blank line contains whitespace
```
reviewboard/accounts/backends/http_digest.py (Diff revision 1)
Show all issues
```
W391 blank line at end of file
```

Commits:

	Summary	Author
-	Fixed HTTP Digest backend unauthenticated user being added as reviewer issue	sng06
+	Fixed HTTP Digest backend unauthenticated user being added as reviewer issue	sng06
+	Removed commented-out code	sng06

Diff:

Revision 2 (+113 -71)

Show changes

reviewboard/accounts/backends/http_digest.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/accounts/backends/http_digest.py (Diff revision 2)
Show all issues
```
W293 blank line contains whitespace
```
reviewboard/accounts/backends/http_digest.py (Diff revision 2)
Show all issues
```
E501 line too long (92 > 79 characters)
```
reviewboard/accounts/backends/http_digest.py (Diff revision 2)
Show all issues
```
E501 line too long (103 > 79 characters)
```
reviewboard/accounts/backends/http_digest.py (Diff revision 2)
Show all issues
```
W293 blank line contains whitespace
```
reviewboard/accounts/backends/http_digest.py (Diff revision 2)
Show all issues
```
E501 line too long (84 > 79 characters)
```
reviewboard/accounts/backends/http_digest.py (Diff revision 2)
Show all issues
```
E501 line too long (90 > 79 characters)
```
reviewboard/accounts/backends/http_digest.py (Diff revision 2)
Show all issues
```
W293 blank line contains whitespace
```
reviewboard/accounts/backends/http_digest.py (Diff revision 2)
Show all issues
```
W293 blank line contains whitespace
```
reviewboard/accounts/backends/http_digest.py (Diff revision 2)
Show all issues
```
W391 blank line at end of file
```

Commits:

	Summary	Author
-	Fixed HTTP Digest backend unauthenticated user being added as reviewer issue	sng06
-	Removed commented-out code	sng06
+	Fixed HTTP Digest backend unauthenticated user being added as reviewer issue	sng06
+	Removed commented-out code	sng06
+	Fixed styling issues	sng06

Diff:

Revision 3 (+128 -78)

Show changes

reviewboard/accounts/backends/http_digest.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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

reviewboard/accounts/backends/http_digest.py (Diff revision 3)

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

Commits:

		Author
-	Fixed HTTP Digest backend unauthenticated user being added as reviewer issue	sng06
-	Removed commented-out code	sng06
-	Fixed styling issues	sng06
+	Fixed HTTP Digest backend unauthenticated user being added as reviewer issue	sng06
+	Removed commented-out code	sng06
+	Fixed styling issues	sng06
+	Changed the argument name to match what	sng06

Diff:

Revision 4 (+129 -79)

Show changes

reviewboard/accounts/backends/http_digest.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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:

		Author
-	Fixed HTTP Digest backend unauthenticated user being added as reviewer issue	sng06
-	Removed commented-out code	sng06
-	Fixed styling issues	sng06
-	Changed the argument name to match what	sng06
+	Fixed HTTP Digest backend unauthenticated user being added as reviewer issue	sng06
+	Removed commented-out code	sng06
+	Fixed styling issues	sng06
+	Changed the argument name to match what	sng06
+	Changed method name	sng06
+	Added docstrings to htpasswd_get_user method	sng06
+	Deleted unecessary text in docstrings	sng06
+	Added unit test suite for HTTPDigestBackend class	sng06

Diff:

Revision 5 (+617 -93)

Show changes

	reviewboard/accounts/backends/http_digest.py
	reviewboard/accounts/tests/test_http_digest_auth_backend.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/accounts/backends/http_digest.py (Diff revision 5)
Show all issues
```
E127 continuation line over-indented for visual indent
```
reviewboard/accounts/tests/test_http_digest_auth_backend.py (Diff revision 5)
Show all issues
```
E127 continuation line over-indented for visual indent
```
reviewboard/accounts/tests/test_http_digest_auth_backend.py (Diff revision 5)
Show all issues
```
E127 continuation line over-indented for visual indent
```

Change Summary:

Fixed linting issues

Summary:

-	[WIP] Fixed HTTP Digest backend unauthenticated user being added as reviewer issue- Bug-4929
+	Fixed HTTP Digest backend unauthenticated user being added as reviewer issue- Bug-4929

Commits:

		Author
-	Fixed HTTP Digest backend unauthenticated user being added as reviewer issue	sng06
-	Removed commented-out code	sng06
-	Fixed styling issues	sng06
-	Changed the argument name to match what	sng06
-	Changed method name	sng06
-	Added docstrings to htpasswd_get_user method	sng06
-	Deleted unecessary text in docstrings	sng06
-	Added unit test suite for HTTPDigestBackend class	sng06
+	Fixed HTTP Digest backend unauthenticated user being added as reviewer issue	sng06
+	Removed commented-out code	sng06
+	Fixed styling issues	sng06
+	Changed the argument name to match what	sng06
+	Changed method name	sng06
+	Added docstrings to htpasswd_get_user method	sng06
+	Deleted unecessary text in docstrings	sng06
+	Added unit test suite for HTTPDigestBackend class	sng06
+	Fixed over-indented linting issue	sng06

Diff:

Revision 6 (+620 -96)

Show changes

	reviewboard/accounts/backends/http_digest.py
	reviewboard/accounts/tests/test_http_digest_auth_backend.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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.

reviewboard/accounts/backends/http_digest.py (Diff revision 6)

Show all issues

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

reviewboard/accounts/backends/http_digest.py (Diff revision 6)

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.

reviewboard/accounts/backends/http_digest.py (Diff revision 6)
Show all issues
```
dictionary -> dict
```
reviewboard/accounts/tests/test_http_digest_auth_backend.py (Diff revision 6)
Show all issues
```
Please put full-module imports above from imports within the section.
```

reviewboard/accounts/tests/test_http_digest_auth_backend.py (Diff revision 6)

Show all issues

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

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

reviewboard/accounts/tests/test_http_digest_auth_backend.py (Diff revision 6)

Show all issues

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

reviewboard/accounts/tests/test_http_digest_auth_backend.py (Diff revision 6)

Show all issues

Similar thing here as above.

reviewboard/accounts/tests/test_http_digest_auth_backend.py (Diff revision 6)
Show all issues
```
These can use lambdas too.
```
reviewboard/accounts/tests/test_http_digest_auth_backend.py (Diff revision 6)
Show all issues
```
These can use lambdas too
```

reviewboard/accounts/tests/test_http_digest_auth_backend.py (Diff revision 6)

Show all issues

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

reviewboard/accounts/tests/test_http_digest_auth_backend.py (Diff revision 6)
Show all issues
```
Same here.
```

Change Summary:

Addressed code review feedback.

Commits:

		Author
-	Fixed HTTP Digest backend unauthenticated user being added as reviewer issue	sng06
-	Removed commented-out code	sng06
-	Fixed styling issues	sng06
-	Changed the argument name to match what	sng06
-	Changed method name	sng06
-	Added docstrings to htpasswd_get_user method	sng06
-	Deleted unecessary text in docstrings	sng06
-	Added unit test suite for HTTPDigestBackend class	sng06
-	Fixed over-indented linting issue	sng06
+	Fixed HTTP Digest backend unauthenticated user being added as reviewer issue	sng06
+	Removed commented-out code	sng06
+	Fixed styling issues	sng06
+	Changed the argument name to match what	sng06
+	Changed method name	sng06
+	Added docstrings to htpasswd_get_user method	sng06
+	Deleted unecessary text in docstrings	sng06
+	Added unit test suite for HTTPDigestBackend class	sng06
+	Fixed over-indented linting issue	sng06
+	Addressed code review feedback	sng06
+	Changed the htpassword_user_info type to dict in the method parameter	sng06

Diff:

Revision 7 (+640 -174)

Show changes

	reviewboard/accounts/backends/http_digest.py
	reviewboard/accounts/tests/test_http_digest_auth_backend.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.