Summary

Added support for authentication against Active Directory subdomains.

Review Request #2838 — Created Jan. 30, 2012 and submitted Aug. 6, 2013, 3:18 p.m.

Information

Owner

aineko

Repository

Review Board

Branch

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

Added support for authentication against Active Directory subdomains.

This patch enables authentication against specific AD subdomain 
extracted from the username. E.g. for configured domain name 
"example.com" user "north\tom" will be authenticated as "tom" against 
"north.example.com".

Testing Done

I have this running fine with multiple AD subdomains configured in my organization.

Issues

Description	From	Last Updated
userdomain could be an optional argument	DE delyn	March 6, 2013, 1:21 a.m.
userdomain could be an optional argument	DE delyn	March 6, 2013, 1:23 a.m.
userdomain could be an optional argument	DE delyn	March 6, 2013, 1:24 a.m.
userdomain could be an optional argument I do not see a reason to limit group search to userdomain. On our …	DE delyn	March 6, 2013, 1:28 a.m.
userdomain could be an optional argument	DE delyn	March 6, 2013, 1:29 a.m.
on our side, active directory port is not 389 and AD_FIND_DC_FROM_DNS is not available providing host with host:port should be …	DE delyn	March 6, 2013, 1:30 a.m.
userdomain not required	DE delyn	March 6, 2013, 1:30 a.m.
not mandatory to configure authentication on several domains	DE delyn	March 6, 2013, 1:30 a.m.
Col: 1 W293 blank line contains whitespace	reviewbot	March 12, 2013, 9:18 a.m.
Col: 1 W293 blank line contains whitespace	reviewbot	March 12, 2013, 9:18 a.m.
Col: 32 E225 missing whitespace around operator	reviewbot	March 12, 2013, 9:18 a.m.
Col: 25 W291 trailing whitespace	reviewbot	March 12, 2013, 9:18 a.m.
Col: 75 W291 trailing whitespace	reviewbot	March 12, 2013, 9:18 a.m.
Col: 80 E501 line too long (90 > 79 characters)	reviewbot	March 12, 2013, 9:18 a.m.
Remove this blank line.	chipx86	March 19, 2013, 11:26 a.m.
Blank line after this.	chipx86	March 19, 2013, 11:26 a.m.
Blank line after.	chipx86	March 19, 2013, 11:26 a.m.
'string' is kind of unspecific. Maybe domain? I think this should allow either comma-separated or space-separated domains. Can be done …	chipx86	March 19, 2013, 11:30 a.m.
Remove the parens.	chipx86	March 19, 2013, 11:30 a.m.
Blank line before.	chipx86	March 19, 2013, 11:31 a.m.
Blank line after. I'd prefer user_subdomain.	chipx86	March 19, 2013, 11:32 a.m.
if '@' in username:	chipx86	March 19, 2013, 11:32 a.m.
This can be one statement: username, subdomain = username.split('@', 1)	chipx86	March 19, 2013, 11:33 a.m.
if '\\' in username:	chipx86	March 19, 2013, 11:33 a.m.
Same comment as above.	chipx86	March 19, 2013, 11:33 a.m.
I'd prefer user_domain.	chipx86	March 19, 2013, 11:34 a.m.
Blank line before. Also should be: if user_subdomain:	chipx86	March 19, 2013, 11:34 a.m.
Can you make this specific to mention Active Directory?	chipx86	March 19, 2013, 11:34 a.m.
This seems to be here twice.	chipx86	March 19, 2013, 11:34 a.m.
"... to find the DC, specify the domain ..."	chipx86	March 19, 2013, 11:35 a.m.
Col: 80 E501 line too long (83 > 79 characters)	reviewbot	March 19, 2013, 11:25 a.m.

I do not know whenever it is possible to sumbit an update of this review if I'm not the initial submitter
I'll try

reviewboard/accounts/backends.py (Diff revision 1)
The issue has been resolved. Show all issues
```
userdomain could be an optional argument
```
reviewboard/accounts/backends.py (Diff revision 1)
The issue has been resolved. Show all issues
```
userdomain could be an optional argument
```
reviewboard/accounts/backends.py (Diff revision 1)
The issue has been resolved. Show all issues
```
userdomain could be an optional argument
```

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

The issue has been resolved. Show all issues

userdomain could be an optional argument

I do not see a reason to limit group search to userdomain. On our side many groups are defined on another domain. So it will prevent finding all group in which user belongs to.

queries can be made at forest level (AD_DOMAIN_NAME), don't know if some companies restrict such query

reviewboard/accounts/backends.py (Diff revision 1)
The issue has been resolved. Show all issues
```
userdomain could be an optional argument
```

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

The issue has been resolved. Show all issues

on our side, active directory port is not 389 and AD_FIND_DC_FROM_DNS is not available

providing host with host:port should be a good option

Moreover if dcs could be a list, they can be define in user interface and splitted by spaces
host1:port host2:port...

reviewboard/accounts/backends.py (Diff revision 1)
The issue has been resolved. Show all issues
```
userdomain not required
```
reviewboard/accounts/forms.py (Diff revision 1)
The issue has been resolved. Show all issues
```
not mandatory to configure authentication on several domains
```

Change Summary:

Incorporating patch from Nicolas Dély

Diff:

Revision 2 (+46 -16)

Show changes

	reviewboard/accounts/backends.py
	reviewboard/accounts/forms.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/accounts/backends.py
    reviewboard/accounts/forms.py
  Ignored Files:

reviewboard/accounts/backends.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 1
 W293 blank line contains whitespace
```
reviewboard/accounts/backends.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 1
 W293 blank line contains whitespace
```
reviewboard/accounts/backends.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 32
 E225 missing whitespace around operator
```
reviewboard/accounts/backends.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 25
 W291 trailing whitespace
```
reviewboard/accounts/backends.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 75
 W291 trailing whitespace
```
reviewboard/accounts/forms.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (90 > 79 characters)
```

Change Summary:

Fixing style, courtesy of Nicolas Dély

Diff:

Revision 3 (+44 -14)

Show changes

	reviewboard/accounts/backends.py
	reviewboard/accounts/forms.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/accounts/backends.py
    reviewboard/accounts/forms.py
  Ignored Files:

reviewboard/accounts/backends.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Remove this blank line.
```
reviewboard/accounts/backends.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Blank line after this.
```
reviewboard/accounts/backends.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Blank line after.
```

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

The issue has been resolved. Show all issues

'string' is kind of unspecific. Maybe domain?

I think this should allow either comma-separated or space-separated domains. Can be done with a regex using re.split (though, make that a compiled regex in the class).

aineko March 19, 2013, 11:35 a.m.

string -> dc_entry

I would argue that strict specification of the format is better than trying to parse spaces, and then commas, semicolons, tabs, etc... I have updated the description for the field to give explicit example with spaces.

reviewboard/accounts/backends.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Remove the parens.
```
reviewboard/accounts/backends.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Blank line before.
```
reviewboard/accounts/backends.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Blank line after.

I'd prefer user_subdomain.
```
reviewboard/accounts/backends.py (Diff revision 3)
The issue has been resolved. Show all issues
```
if '@' in username:
```
reviewboard/accounts/backends.py (Diff revision 3)
The issue has been resolved. Show all issues
```
This can be one statement:

username, subdomain = username.split('@', 1)
```
reviewboard/accounts/backends.py (Diff revision 3)
The issue has been resolved. Show all issues
```
if '\\' in username:
```
reviewboard/accounts/backends.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Same comment as above.
```
reviewboard/accounts/backends.py (Diff revision 3)
The issue has been dropped. Show all issues
```
I'd prefer user_domain.
```
1. AI
  
  aineko March 19, 2013, 11:35 a.m.
  userdomain is used in many other places in the existing code, prefer to keep it.
reviewboard/accounts/backends.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Blank line before.

Also should be:

    if user_subdomain:
```
reviewboard/accounts/backends.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Can you make this specific to mention Active Directory?
```
reviewboard/accounts/backends.py (Diff revision 3)
The issue has been resolved. Show all issues
```
This seems to be here twice.
```
reviewboard/accounts/forms.py (Diff revision 3)
The issue has been resolved. Show all issues
```
"... to find the DC, specify the domain ..."
```

Change Summary:

Updates per review from Christian

Diff:

Revision 4 (+43 -14)

Show changes

	reviewboard/accounts/backends.py
	reviewboard/accounts/forms.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/accounts/backends.py
    reviewboard/accounts/forms.py
  Ignored Files:

reviewboard/accounts/backends.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (83 > 79 characters)
```

Diff:

Revision 5 (+44 -14)

Show changes

	reviewboard/accounts/backends.py
	reviewboard/accounts/forms.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/accounts/backends.py
    reviewboard/accounts/forms.py
  Ignored Files:

Ship it!

regarding latest comments and changes, I cannot see any improvements

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to master (541e42b). Thanks!