Summary

Add error handling for when LDAP "Full Name Attribute" has no white space.

Review Request #9211 — Created Sept. 23, 2017 and submitted Oct. 21, 2017, 2:31 p.m.

Information

Owner

rcreagha

Repository

Review Board

Branch

release-2.5.x

Bugs

4489

Depends On

Commit

3096001...

Reviewers

Groups

reviewboard, students

People

Description

When setting up LDAP authentication, an uncaught exception may be thrown
when splitting the full name into first name and last name. This happens
when the full name contains no space.

If this exception occurs it will now assign the full name to the first
name, and make the last name an empty string.
Because there was a comment just above my changes explaining how full
name is split between first and last name, I added to it to explain what
happens to first and last name in this situation.

Testing Done

I was not able to properly reproduce and test this manually because I
could not get a LDAP server up and running.

I have written a unit test that provides the modified function,
get_or_create_user(), with a user that has a full name containing no
spaces and verifies that the first and last name are assigned as
expected.
This test passes with this fix, and fails on 2.5.x without the fix.

Issues

Description	From	Last Updated
The description looks great, though it needs to wrap to 70-75 characters. I do want to see some testing in …	chipx86	Oct. 13, 2017, 7:17 p.m.
Blank line between these (any time you have a block-creating statement like try, if/else, etc. and another statement).	chipx86	Oct. 6, 2017, 5:03 p.m.
W293 blank line contains whitespace	reviewbot	Oct. 6, 2017, 5:44 p.m.
Can we add a blank line (with just a #) between these to separate the paragraphs?	david	Oct. 19, 2017, 11:19 a.m.
Should end in a period.	david	Oct. 19, 2017, 11:19 a.m.

flake8 passed.

JSHint passed.

The issue has been resolved. Show all issues

The description looks great, though it needs to wrap to 70-75 characters.

I do want to see some testing in here, though. I understand the issue with setting up an LDAP server (no fun), but we do have infrastructure for simulating LDAP connections in our unit tests, so you should at least have a unit test for this (one that fails prior to your fix and succeeds after).

reviewboard/accounts/backends.py (Diff revision 1)

The issue has been resolved. Show all issues

Blank line between these (any time you have a block-creating statement like try, if/else, etc. and another statement).

Summary:

-	Add error handling for exception thrown when LDAP "Full Name Attribute" has no white space.
+	Add error handling for when LDAP "Full Name Attribute" has no white space.

Description:

~		When setting up LDAP authentication, an uncaught exception may be thrown when splitting the full name into first name and last name. This happens when the full name contains no space.
	~	When setting up LDAP authentication, an uncaught exception may be thrown when
	+	splitting the full name into first name and last name. This happens when the
	+	full name contains no space.

~		If this exception occurs it will now assign the full name to the first name, and make the last name an empty string.
	~	If this exception occurs it will now assign the full name to the first name,
	+	and make the last name an empty string.
	+	Because there was a comment just above my changes explaining how full name
	+	is split between first and last name, I added to it to explain what happens
	+	to first and last name in this situation.

Testing Done:

~		Unfortunately this has not been tested. I was told to skip setting up a LDAP server, which is needed to properly reproduce the bug and test this fix, because my efforts to get one working properly were unsuccessful.
~		The only "testing" that was done was to create a new file with the same situation of using str.split(' ',1) on a string containing no space, finding out the exact error, and catching it.
	~	I was not able to properly reproduce and test this manually because I could
	~	not get a LDAP server up and running.
	+
	+	I plan on writing a unit test for this fix, but I'm still working on
	+	figuring out how to do so.

Commit:

-	abc2eeb4f35f3bdc8dd6c710b5dc912688db8886
+	d114a8d0c35de59be3767e4d25c50c69723e5057

Diff:

Revision 2 (+10 -1)

Show changes

reviewboard/accounts/backends.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/accounts/backends.py (Diff revision 2)
The issue has been resolved. Show all issues
```
W293 blank line contains whitespace
```

Commit:

-	d114a8d0c35de59be3767e4d25c50c69723e5057
+	8de170ac48f85951bf54c9062484b1c94a8f6a8f

Diff:

Revision 3 (+10 -1)

Show changes

reviewboard/accounts/backends.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Testing Done:

		I was not able to properly reproduce and test this manually because I could
		not get a LDAP server up and running.

~		I plan on writing a unit test for this fix, but I'm still working on
~		figuring out how to do so.
	~	I have written a unit test that provides the modified function,
	~	get_or_create_user(), with a user that has a full name containing no spaces
	+	and verifies that the first and last name are assigned as expected.
	+	This test passes with this fix, and fails on 2.5.x without the fix.

Commit:

-	8de170ac48f85951bf54c9062484b1c94a8f6a8f
+	cdc046c877250b13e1e52d8ffb2e567968ac385f

Diff:

Revision 4 (+52 -1)

Show changes

	reviewboard/accounts/backends.py
	reviewboard/accounts/tests.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Description:

~		When setting up LDAP authentication, an uncaught exception may be thrown when
~		splitting the full name into first name and last name. This happens when the
~		full name contains no space.
	~	When setting up LDAP authentication, an uncaught exception may be thrown
	~	when splitting the full name into first name and last name. This happens
	~	when the full name contains no space.

~		If this exception occurs it will now assign the full name to the first name,
~		and make the last name an empty string.
~		Because there was a comment just above my changes explaining how full name
~		is split between first and last name, I added to it to explain what happens
~		to first and last name in this situation.
	~	If this exception occurs it will now assign the full name to the first
	~	name, and make the last name an empty string.
	~	Because there was a comment just above my changes explaining how full
	~	name is split between first and last name, I added to it to explain what
	~	happens to first and last name in this situation.

Testing Done:

~		I was not able to properly reproduce and test this manually because I could
~		not get a LDAP server up and running.
	~	I was not able to properly reproduce and test this manually because I
	~	could not get a LDAP server up and running.

		I have written a unit test that provides the modified function,
~		get_or_create_user(), with a user that has a full name containing no spaces
~		and verifies that the first and last name are assigned as expected.
	~	get_or_create_user(), with a user that has a full name containing no
	~	spaces and verifies that the first and last name are assigned as
	+	expected.
		This test passes with this fix, and fails on 2.5.x without the fix.

reviewboard/accounts/backends.py (Diff revision 4)

The issue has been resolved. Show all issues

Can we add a blank line (with just a #) between these to separate the paragraphs?

reviewboard/accounts/backends.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Should end in a period.
```

Commit:

-	cdc046c877250b13e1e52d8ffb2e567968ac385f
+	30960013268239b57d8747ece940fab4a13bf3fe

Diff:

Revision 5 (+53 -1)

Show changes

	reviewboard/accounts/backends.py
	reviewboard/accounts/tests.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-2.5.x (d7e9803)