Summary

Sandbox reviewboard.accounts.AuthBackend

Review Request #6399 — Created Oct. 4, 2014 and submitted Nov. 10, 2014, 5:30 p.m.

Information

Owner

justy777

Repository

Review Board

Branch

master

Bugs

Depends On

~~6559~~

Commit

bc8a283...

Reviewers

Groups

reviewboard, students

People

Description

Extensions that create an AuthBackend subclass (using the AuthBackendHook) can throw exceptions inside Reviewboard. The parts of Reviewboard that call those methods have been sandboxed.

Now when an AuthBackend subclass from an extension throws an exception; Reviewboard logs the errors with enough information to find which method in the AuthBackend subclass threw the exception.

Testing Done

Unit tests have been written to make sure that functions from an AuthBackend subclass have been called, and when an exception is thrown it gets logged.

The tests fail without the sanboxing, and succeed with it.

Issues

Description	From	Last Updated
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Oct. 23, 2014, 2:17 a.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Oct. 23, 2014, 2:33 a.m.
Col: 5 E303 too many blank lines (2)	reviewbot	Oct. 23, 2014, 2:33 a.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Oct. 23, 2014, 2:33 a.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Oct. 23, 2014, 2:33 a.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Oct. 23, 2014, 2:33 a.m.
Col: 80 E501 line too long (105 > 79 characters)	reviewbot	Oct. 23, 2014, 2:33 a.m.
Col: 80 E501 line too long (111 > 79 characters)	reviewbot	Oct. 23, 2014, 2:33 a.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Oct. 23, 2014, 2:33 a.m.
Col: 5 E303 too many blank lines (2)	reviewbot	Oct. 23, 2014, 2:33 a.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Oct. 23, 2014, 2:33 a.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Oct. 23, 2014, 2:33 a.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	Oct. 23, 2014, 2:33 a.m.
Col: 31 E128 continuation line under-indented for visual indent	reviewbot	Oct. 23, 2014, 2:33 a.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	Oct. 23, 2014, 2:37 a.m.
Col: 80 E501 line too long (87 > 79 characters)	reviewbot	Oct. 23, 2014, 2:37 a.m.
Col: 31 E128 continuation line under-indented for visual indent	reviewbot	Oct. 23, 2014, 2:37 a.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	Oct. 23, 2014, 2:40 a.m.
Col: 80 E501 line too long (87 > 79 characters)	reviewbot	Oct. 23, 2014, 2:40 a.m.
Col: 31 E128 continuation line under-indented for visual indent	reviewbot	Oct. 23, 2014, 2:40 a.m.
Col: 5 E303 too many blank lines (2)	reviewbot	Oct. 23, 2014, 2:42 a.m.
Can you include the backend name in here? Right now it's not possible to know which backend is failing from …	david	Oct. 23, 2014, 2:08 p.m.
Here too?	david	Oct. 23, 2014, 2:08 p.m.
Here too?	david	Oct. 23, 2014, 2:08 p.m.
Here too?	david	Oct. 23, 2014, 2:08 p.m.
There should also be a blank line here. Undo this.	brennie	Oct. 23, 2014, 1:38 p.m.
There should be 2 blank lines between classes. Undo this.	brennie	Oct. 23, 2014, 1:38 p.m.
You can skip the try/catch and just call form.clean_old_password(). If an exception is raised and not caught in a test …	david	Oct. 23, 2014, 1:46 p.m.
Same here.	david	Oct. 23, 2014, 1:46 p.m.
Same here.	david	Oct. 23, 2014, 1:46 p.m.
Same here.	david	Oct. 23, 2014, 1:46 p.m.
Same here.	david	Oct. 23, 2014, 1:46 p.m.
Same here.	david	Oct. 23, 2014, 1:46 p.m.
Same here.	david	Oct. 23, 2014, 1:46 p.m.
Can you include the backend name in here?	david	Oct. 23, 2014, 2:10 p.m.
Can you include the backend name in here?	david	Oct. 23, 2014, 2:13 p.m.
And here.	david	Oct. 23, 2014, 2:12 p.m.
And here.	david	Oct. 23, 2014, 2:12 p.m.
Col: 28 E127 continuation line over-indented for visual indent	reviewbot	Oct. 23, 2014, 2:29 p.m.
Col: 55 W291 trailing whitespace	reviewbot	Oct. 23, 2014, 2:27 p.m.
Col: 13 E113 unexpected indentation	reviewbot	Oct. 23, 2014, 2:27 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Oct. 23, 2014, 2:29 p.m.
Col: 52 W291 trailing whitespace	reviewbot	Oct. 23, 2014, 2:27 p.m.
Col: 28 E127 continuation line over-indented for visual indent	reviewbot	Oct. 23, 2014, 2:29 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Oct. 23, 2014, 2:29 p.m.
Auth backends have a user-visible name, so you can use backend.name instead of backend.__class__.name. Here and all the other logging …	david	Oct. 23, 2014, 4:19 p.m.
There shouldn't be a blank line here (django, djblets, and kgb are all "third party" packages)	david	Oct. 23, 2014, 4:20 p.m.
What's the point of this?	david	Oct. 23, 2014, 4:34 p.m.
L before R	david	Oct. 23, 2014, 4:24 p.m.
Use # for comments. """ is a docstring.	david	Oct. 23, 2014, 5:04 p.m.
This should be commented too.	david	Oct. 23, 2014, 5:04 p.m.
Col: 9 E265 block comment should start with '# '	reviewbot	Oct. 23, 2014, 5:15 p.m.
Col: 9 E265 block comment should start with '# '	reviewbot	Oct. 23, 2014, 5:15 p.m.
Col: 9 E265 block comment should start with '# '	reviewbot	Oct. 23, 2014, 5:15 p.m.
Col: 9 E265 block comment should start with '# '	reviewbot	Oct. 23, 2014, 5:31 p.m.
Col: 9 E265 block comment should start with '# '	reviewbot	Oct. 23, 2014, 5:31 p.m.
Col: 9 E265 block comment should start with '# '	reviewbot	Oct. 23, 2014, 5:31 p.m.
Add a trailing comma here.	david	Oct. 23, 2014, 6:23 p.m.
Add a trailing comma here.	david	Oct. 23, 2014, 6:23 p.m.
No # at the end.	david	Oct. 23, 2014, 6:23 p.m.
Add a trailing comma here.	david	Oct. 23, 2014, 6:23 p.m.
No # at the end.	david	Oct. 23, 2014, 6:23 p.m.
Add a trailing comma here.	david	Oct. 23, 2014, 6:23 p.m.
No # at the end.	david	Oct. 23, 2014, 6:23 p.m.
This will catch the ValidationError that's raised inside the try.	david	Oct. 24, 2014, 5:05 p.m.
Blank line between these.	chipx86	Oct. 27, 2014, 1:04 p.m.
The logging errors should use %r with backend, so that the log message will include the specific class and any …	chipx86	Oct. 27, 2014, 1:22 p.m.
These are human-readable strings. They need to be proper sentences, and should be targeted to the end user. Unlike with …	chipx86	Oct. 27, 2014, 1:14 p.m.
Blank line. Same below.	chipx86	Oct. 27, 2014, 1:14 p.m.
Blank line.	chipx86	Oct. 27, 2014, 1:15 p.m.
No need for Class, and Test is too generic. Should be something like SandboxAuthBackend.	chipx86	Oct. 27, 2014, 1:28 p.m.
No point in storing this in a variable.	chipx86	Oct. 27, 2014, 1:22 p.m.
This should be something like: self.assertEqual(e.message, 'blah blah') If the string still wraps, this may be better: self.assertEqual( e.message, 'blah …	chipx86	Oct. 27, 2014, 1:20 p.m.
It's unclear how this relates to the function being tested. Looking through the change, I realize they're not. Instead, these …	chipx86	Oct. 27, 2014, 4:52 p.m.
blank line before this.	chipx86	Oct. 27, 2014, 1:18 p.m.
Use %r, and start the arguments on a new line.	chipx86	Oct. 27, 2014, 1:26 p.m.
Use %r, and start the arguments on a new line.	chipx86	Oct. 27, 2014, 1:26 p.m.
Use %r, and start the arguments on a new line.	chipx86	Oct. 27, 2014, 1:26 p.m.
Use %r, and start the arguments on a new line.	chipx86	Oct. 27, 2014, 1:26 p.m.
There's no need for this, since we'll end up with either a value or an exception in the next block.	chipx86	Oct. 28, 2014, 10:22 a.m.
Blank line between these.	chipx86	Oct. 28, 2014, 10:04 a.m.
Blank line between these.	chipx86	Oct. 28, 2014, 10:05 a.m.
Similar to my comments in the columns change, you don't actually need extensions for these tests. You only need extensions …	chipx86	Oct. 28, 2014, 10:07 a.m.
There's nothing here to test for breakages, since these functions don't raise any exceptions. On all these changes, you'll need …	chipx86	Oct. 28, 2014, 10:21 a.m.
Two blank lines at the top level between classes.	chipx86	Oct. 28, 2014, 10:08 a.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Oct. 28, 2014, 10:02 a.m.
This isn't actually testing anything.	chipx86	Oct. 28, 2014, 11:38 a.m.
Again, extensions are useless here.	chipx86	Oct. 28, 2014, 10:19 a.m.
Extensions are useless here too.	chipx86	Oct. 28, 2014, 10:16 a.m.
If we're not testing this function, we should remove it. Otherwise, it should throw something. Same in all other cases …	chipx86	Oct. 28, 2014, 10:16 a.m.
Same comments.	chipx86	Oct. 28, 2014, 10:16 a.m.
Col: 43 E128 continuation line under-indented for visual indent	reviewbot	Oct. 28, 2014, 12:44 p.m.
Col: 43 E128 continuation line under-indented for visual indent	reviewbot	Oct. 28, 2014, 12:44 p.m.
Should have a trailing period.	chipx86	Oct. 30, 2014, 12:29 a.m.
Should just be: super(SandboxTests, self).tearDown()	chipx86	Oct. 30, 2014, 12:30 a.m.
We have no verification that authenticate() is ever called. We should use spy_on() and check that it was called with …	chipx86	Oct. 30, 2014, 12:34 a.m.
These docstrings should be in this format: """Testing AuthBackend sandboxing for <func>"""	chipx86	Oct. 30, 2014, 1:07 a.m.
For this, you can use: self.assertRaisesMessage( ValidationError, "<message here>", lambda: form.clean_old_password())	chipx86	Oct. 30, 2014, 12:36 a.m.
We have no verification that update_password() is ever called. We should use spy_on() and check that it was called with …	chipx86	Oct. 30, 2014, 12:41 a.m.
We have no verification that update_name() is ever called. We should use spy_on() and check that it was called with …	chipx86	Oct. 30, 2014, 12:41 a.m.
We do this in all the tests, so it can move into setUp().	chipx86	Oct. 30, 2014, 12:45 a.m.
We do this a lot, so it can probably just move into setUp().	chipx86	Oct. 30, 2014, 12:45 a.m.
We have no verification that update_email() is ever called. We should use spy_on() and check that it was called with …	chipx86	Oct. 30, 2014, 12:41 a.m.
This should be using super(...) still. Why was it changed?	chipx86	Oct. 30, 2014, 12:47 a.m.
This should say: """Testing AuthBackendHook sandboxes AuthBackend registration""" Note the "sandboxes" part, which will help give a clue if tests …	chipx86	Oct. 30, 2014, 12:48 a.m.
For API resource unit tests, we don't want to introduce any new classes. Instead, the test should be part of …	chipx86	Nov. 5, 2014, 1:40 a.m.
This docstring isn't correct, and doesn't match the rest of the docstrings in this class. It should describe the URL, …	chipx86	Nov. 4, 2014, 9:10 p.m.
This should be treated as a failure, not success.	david	Nov. 7, 2014, 4:47 p.m.
I mentioned this in the other review request, but this isn't correct.	david	Nov. 10, 2014, 2:22 a.m.

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/extensions/tests.py



Tool: Pyflakes
Processed Files:
    reviewboard/extensions/tests.py

reviewboard/extensions/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```

reviewboard/extensions/tests.py (Diff revision 1)

Is it possible for get_or_create(username) to fail?

justy777 Oct. 19, 2014, 12:42 p.m.

The whole point is to get it to throw an exception.

reviewboard/extensions/tests.py (Diff revision 1)

Is it possible for get_or_create(username) to fail?

Description:: Unit tests for reviewboard.accounts.AuthBackend, and then sandboxing call to methods on the reviewboard.accounts.AuthBackend subclass
+
+
TODO:
+ 1. Make test initially fail.
+ 2. Sandbox AuthBackend extension
+ 3. Check test passes afterwards.
Testing Done:: ~
Ran unit tests.
~
Ran extension unit tests.

Summary:

[WIP] reviewboard.accounts.AuthBackend sandboxing

reviewboard.accounts.AuthBackend sandboxing

Description:

		Unit tests for reviewboard.accounts.AuthBackend, and then sandboxing call to methods on the reviewboard.accounts.AuthBackend subclass

~		TODO:
~		1. Make test initially fail.
~		2. Sandbox AuthBackend extension
~		3. Check test passes afterwards.
	~	methods sandboxed:
	~	authenticate()
	~	get_or_create_user()
	~	query_users()
	+	search_users()
	+	update_name()
	+	update_email()
	+	update_password()

Testing Done:

~		Ran extension unit tests.
	~	Ran unit tests; they fail without the sandboxing and succeed with it.

Commit:

683cb794d5b653a473f814f604e7f98baa567831

5703bab58d83d61914386f9aa9f233dd52ece154

Diff:

Revision 2 (+223 -16)

	reviewboard/accounts/forms/pages.py
	reviewboard/extensions/tests.py
	reviewboard/webapi/resources/review_request.py
	reviewboard/webapi/resources/review_request_draft.py
	reviewboard/webapi/resources/user.py

	reviewboard/accounts/tests.py
	reviewboard/accounts/forms/pages.py
	reviewboard/extensions/tests.py
	reviewboard/webapi/resources/review_request.py
	reviewboard/webapi/resources/review_request_draft.py
	reviewboard/webapi/resources/user.py
	reviewboard/webapi/tests/test_review_request.py
	reviewboard/webapi/tests/test_review_request_draft.py
	1 more

		Unit tests for reviewboard.accounts.AuthBackend, and then sandboxing call to methods on the reviewboard.accounts.AuthBackend subclass

	+	Hook sandboxed: AuthBackendHook
	+
		methods sandboxed:
		authenticate()
		get_or_create_user()
		query_users()
		search_users()
		update_name()
		update_email()
		update_password()
-
-		TODO:
-		regsiter_authbackend()
-		unregister_authbackend()

~		Unit tests for reviewboard.accounts.AuthBackend, and then sandboxing call to methods on the reviewboard.accounts.AuthBackend subclass
	~	Extensions that create a AuthBackend subclass (using the AuthBackendHook) can throw exceptions inside Reviewboard. The parts of Reviewboard that call those methods have been sandboxed.

~		Hook sandboxed: AuthBackendHook
	~	Now when an AuthBackend subclass from an extension throw an exception; Reviewboard logs the errors with enough information to find which method in the AuthBackend subclass threw the exception.
-
-		methods sandboxed:
-		authenticate()
-		get_or_create_user()
-		query_users()
-		search_users()
-		update_name()
-		update_email()
-		update_password()