Summary

Add Active Directory backend which under most circumstance should be easier to setup than the LDAP backend

Review Request #699 — Created Jan. 14, 2009 and submitted

Information

Owner

cstejerean

Repository

Review Board SVN (deprecated)

Branch

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

The LDAP backend is too confusing to setup for most users of active directory. This Active Directory backend should make it much easier to get up and running with ReviewBoard in an AD environment. A user can just specify the domain name (ie. example.com) and check the option to use DNS to find the DC servers and everything else will be taken care of for them. Finding the domain controllers from DNS requires PyDNS. There is also the option of specifying the domain controller manually.

An OU can be specified optionally which will restrict users to those that belong in that OU. Users can also be required to belong to a certain group and recursive group checks are implemented when specifying a recursion depth greater than 0.

Testing Done

A patched version of Review Board is deployed internally, authenticating against Active Directory using DNS to find the domain controllers and with users restricted to a specified group. Extensive manual testing was performed of several combinations but I haven't figured out how to mock everything needed for an automated test suite.

This is great. I'd love to get this for the 1.0 release.

Some changes before it goes in. Most are stylistic.

cstejerean Jan. 18, 2009, 12:20 p.m.

Thanks. I responded to a few of your comments where I had a question or needed to clarify something. Everything else should be addressed in the latest patch.

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

Shouldn't remove this line. 2 blank lines after imports.

/trunk/reviewboard/accounts/backends.py (Diff revision 1)
```
Two blank lines between classes.
```
/trunk/reviewboard/accounts/backends.py (Diff revision 1)
```
Remove the blank line after this class.
```

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

This can be condensed to:

return (settings.AD_RECURSION_DEPTH == -1 or
        depth <= settings.AD_RECURSION_DEPTH)

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

A little more context would be good here. It should have a prefix of the backend name, and include the function name.

Same with other logging statements in this file.

cstejerean Jan. 18, 2009, 12:19 p.m.

I added a bit more context to the other logging statements in the ActiveDirectory backend by prefixing them with the "Active Directory" string. 

I don't usually include function names in warning or error messages as I think they simply clutter up the log file for the person administering the application. If I need to debug something I can easily track down where the log statement occurs. Also, the other logging statements in LDAPBackend do not include the function name in the logging messages so for consistency I left that out. 

However I can easily change all the logging statements in that file to a format you think is more helpful. Should I add something like ClassName.function as a prefix to all the log statements in backends.py?

chipx86 Jan. 18, 2009, 12:25 p.m.

That's fine, the "Active Directory" string is enough. Good points with the logging. If these were "DEBUG" strings, then function names would be more useful, but as a warning it's fine.

/trunk/reviewboard/accounts/backends.py (Diff revision 1)
```
Space before "port="
```
/trunk/reviewboard/accounts/backends.py (Diff revision 1)
```
Space before "="
```

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

Why is this temporary? Can you document it, and include a "XXX" at the beginning of the comment to indicate it?

cstejerean Jan. 18, 2009, 12:08 p.m.

I forgot to remove this after debugging something. It's fixed in the latest patch.

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

I think these default to False.

cstejerean Jan. 18, 2009, 12:04 p.m.

I've copied that from the LDAPBackend. If they do default to False (which seems like the logical default) perhaps they should be removed from the other backends as well. Should I remove them just from the ActiveDirectoryBackend for now?

chipx86 Jan. 18, 2009, 12:26 p.m.

It's fine, we'll clean up all these in one go later.

/trunk/reviewboard/admin/checks.py (Diff revision 1)
```
No blank line here.
```

/trunk/reviewboard/admin/checks.py (Diff revision 1)

The wording seemed a little confusing to me at first. Maybe something like:

"PyDNS, which is required to find the domain controller, is not installed."

/trunk/reviewboard/admin/checks.py (Diff revision 1)
```
No blank line at the end of the file.
```
/trunk/reviewboard/admin/forms.py (Diff revision 1)
```
Should be in alphabetical order.
```

/trunk/reviewboard/admin/forms.py (Diff revision 1)

Can you reorder "Active Directory" above "LDAP"?

/trunk/reviewboard/admin/forms.py (Diff revision 1)

Good question. We should fix that :) Can you remove the comment though? We can fix this separately.

Any reason the code to disable the field is commented out?

cstejerean Jan. 18, 2009, 12:03 p.m.

Turns out get_can_enable_dns was broken and during debugging I commented out this line then forgot to fix the bug and uncomment it. The newest patch addresses this bug.

Change Summary:

This updated patch addresses the previous style issues, adds a bit more context to the logs, added start_tls to the ldap connection if TLS is requested for AD (which I missed previously) and fixes the bug in get_can_enable_dns by using "import DNS" instead of imp.find_module (I don't know much about how imp works but it fails to find the DNS module even when import DNS works).

Diff:

Revision 2 (+217 -1)

Show changes

	/trunk/reviewboard/accounts/backends.py
	/trunk/reviewboard/admin/checks.py
	/trunk/reviewboard/admin/forms.py
	/trunk/reviewboard/admin/siteconfig.py

Ship it!

This looks good. Thanks! Committed in r1678.