Summary

Fix for AD account backend group enumeration issue

Review Request #1152 — Created Oct. 5, 2009 and submitted

Information

Owner

smat

Repository

Review Board

Branch

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

This patch updates the search query to use CN rather than sAMAccountName so that groups which have CNs that differ from their sAMAccountNames (e.g. if a "pre-Windows 2000" group name is set in Active Directory) will be included in the search results. The memberOf property used here to find groups specifies groups by DN, so searching on the CN component should always be the correct behavior (as far possible, using the full DN would be safer).

Testing Done

Tested on local installation of Review Board 1.0.3

Change Summary:

Changed incorrect references to "DN" to "CN".

Description:

~		This patch updates the search query to use DN rather than sAMAccountName so that groups which have DNs that differ from their sAMAccountNames (e.g. if a "pre-Windows 2000" group name is set in Active Directory) will be included in the search results. The memberOf property used here to find groups specifies groups by DN, so searching on the DN should always be the correct behavior.
	~	This patch updates the search query to use CN rather than sAMAccountName so that groups which have CNs that differ from their sAMAccountNames (e.g. if a "pre-Windows 2000" group name is set in Active Directory) will be included in the search results. The memberOf property used here to find groups specifies groups by DN, so searching on the CN component should always be the correct behavior (as far possible, using the full DN would be safer).

Diff:

Revision 2 (+2 -1)

Show changes

reviewboard/accounts/backends.py

Change Summary:

Updated comment.

Diff:

Revision 3 (+2 -1)

Show changes

reviewboard/accounts/backends.py

Are there any chances of regressions from this? It'll be important to know for documentation purposes.

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

Please make sure all lines wrap to about 79 characters.

Also, blank line before this.

Change Summary:

Fixed formatting as per Christian's instructions. I am assuming the 79 characters starts from the beginning of the comment as this would result in absurdly formatted comments for deeply indented sections.

Diff:

Revision 4 (+5 -1)

Show changes

reviewboard/accounts/backends.py

Christian,

The only scenario I can think of in which this would cause a problem for existing installations is if the sAMAccountName for the group with the cn specified by memberOf does not match that cn, but a group with a sAMAccountName identical to the first group's cn exists - in which case the group returned by the search is not actually the group specified by memberOf.

smat Oct. 12, 2009, 3:04 p.m.

To clarify: in the situation that I described, authentication for users might succeed currently but would fail if this patch is applied. I don't believe that this is a reason not to change the behavior as currently those users are authenticated because RB believes they have group memberships the users don't actually have.

chipx86 Oct. 12, 2009, 7:11 p.m.
```
Fair enough. Thanks.
```

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

It actually is 79 from the start of the line. We have our terminals set to 80 characters and don't like to see wrapping.

Not a big deal though. I'll format before committing. Thanks!

Ship it!

```
Committed to release-1.0.x (r88ab896)
```