Added support for authentication against Active Directory subdomains.

Review Request #2838 — Created Jan. 30, 2012 and submitted

Information

Review Board

Reviewers

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".
I have this running fine with multiple AD subdomains configured in my organization.
Description From Last Updated

userdomain could be an optional argument

DE delyn

userdomain could be an optional argument

DE delyn

userdomain could be an optional argument

DE delyn

userdomain could be an optional argument I do not see a reason to limit group search to userdomain. On our …

DE delyn

userdomain could be an optional argument

DE delyn

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

userdomain not required

DE delyn

not mandatory to configure authentication on several domains

DE delyn

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 32 E225 missing whitespace around operator

reviewbotreviewbot

Col: 25 W291 trailing whitespace

reviewbotreviewbot

Col: 75 W291 trailing whitespace

reviewbotreviewbot

Col: 80 E501 line too long (90 > 79 characters)

reviewbotreviewbot

Remove this blank line.

chipx86chipx86

Blank line after this.

chipx86chipx86

Blank line after.

chipx86chipx86

'string' is kind of unspecific. Maybe domain? I think this should allow either comma-separated or space-separated domains. Can be done …

chipx86chipx86

Remove the parens.

chipx86chipx86

Blank line before.

chipx86chipx86

Blank line after. I'd prefer user_subdomain.

chipx86chipx86

if '@' in username:

chipx86chipx86

This can be one statement: username, subdomain = username.split('@', 1)

chipx86chipx86

if '\\' in username:

chipx86chipx86

Same comment as above.

chipx86chipx86

I'd prefer user_domain.

chipx86chipx86

Blank line before. Also should be: if user_subdomain:

chipx86chipx86

Can you make this specific to mention Active Directory?

chipx86chipx86

This seems to be here twice.

chipx86chipx86

"... to find the DC, specify the domain ..."

chipx86chipx86

Col: 80 E501 line too long (83 > 79 characters)

reviewbotreviewbot
DE
  1. 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
  2. reviewboard/accounts/backends.py (Diff revision 1)
     
     
    Show all issues
    userdomain could be an optional argument
  3. reviewboard/accounts/backends.py (Diff revision 1)
     
     
    Show all issues
    userdomain could be an optional argument
  4. reviewboard/accounts/backends.py (Diff revision 1)
     
     
    Show all issues
    userdomain could be an optional argument
  5. reviewboard/accounts/backends.py (Diff revision 1)
     
     
    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
  6. reviewboard/accounts/backends.py (Diff revision 1)
     
     
    Show all issues
    userdomain could be an optional argument
  7. reviewboard/accounts/backends.py (Diff revision 1)
     
     
     
     
     
    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...
  8. reviewboard/accounts/backends.py (Diff revision 1)
     
     
    Show all issues
    userdomain not required
  9. reviewboard/accounts/forms.py (Diff revision 1)
     
     
    Show all issues
    not mandatory to configure authentication on several domains
  10. 
      
AI
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/accounts/backends.py
        reviewboard/accounts/forms.py
      Ignored Files:
    
    
  2. reviewboard/accounts/backends.py (Diff revision 2)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  3. reviewboard/accounts/backends.py (Diff revision 2)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  4. reviewboard/accounts/backends.py (Diff revision 2)
     
     
    Show all issues
    Col: 32
     E225 missing whitespace around operator
    
  5. reviewboard/accounts/backends.py (Diff revision 2)
     
     
    Show all issues
    Col: 25
     W291 trailing whitespace
    
  6. reviewboard/accounts/backends.py (Diff revision 2)
     
     
    Show all issues
    Col: 75
     W291 trailing whitespace
    
  7. reviewboard/accounts/forms.py (Diff revision 2)
     
     
    Show all issues
    Col: 80
     E501 line too long (90 > 79 characters)
    
  8. 
      
AI
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/accounts/backends.py
        reviewboard/accounts/forms.py
      Ignored Files:
    
    
  2. 
      
chipx86
  1. 
      
  2. reviewboard/accounts/backends.py (Diff revision 3)
     
     
    Show all issues
    Remove this blank line.
  3. reviewboard/accounts/backends.py (Diff revision 3)
     
     
    Show all issues
    Blank line after this.
  4. reviewboard/accounts/backends.py (Diff revision 3)
     
     
    Show all issues
    Blank line after.
  5. reviewboard/accounts/backends.py (Diff revision 3)
     
     
    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).
    1. 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.
  6. reviewboard/accounts/backends.py (Diff revision 3)
     
     
    Show all issues
    Remove the parens.
  7. reviewboard/accounts/backends.py (Diff revision 3)
     
     
    Show all issues
    Blank line before.
  8. reviewboard/accounts/backends.py (Diff revision 3)
     
     
    Show all issues
    Blank line after.
    
    I'd prefer user_subdomain.
  9. reviewboard/accounts/backends.py (Diff revision 3)
     
     
    Show all issues
    if '@' in username:
  10. reviewboard/accounts/backends.py (Diff revision 3)
     
     
     
     
    Show all issues
    This can be one statement:
    
    username, subdomain = username.split('@', 1)
  11. reviewboard/accounts/backends.py (Diff revision 3)
     
     
    Show all issues
    if '\\' in username:
  12. reviewboard/accounts/backends.py (Diff revision 3)
     
     
     
     
    Show all issues
    Same comment as above.
  13. reviewboard/accounts/backends.py (Diff revision 3)
     
     
    Show all issues
    I'd prefer user_domain.
    1. userdomain is used in many other places in the existing code, prefer to keep it.
  14. reviewboard/accounts/backends.py (Diff revision 3)
     
     
    Show all issues
    Blank line before.
    
    Also should be:
    
        if user_subdomain:
  15. reviewboard/accounts/backends.py (Diff revision 3)
     
     
    Show all issues
    Can you make this specific to mention Active Directory?
  16. reviewboard/accounts/backends.py (Diff revision 3)
     
     
    Show all issues
    This seems to be here twice.
  17. reviewboard/accounts/forms.py (Diff revision 3)
     
     
    Show all issues
    "... to find the DC, specify the domain ..."
  18. 
      
AI
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/accounts/backends.py
        reviewboard/accounts/forms.py
      Ignored Files:
    
    
  2. reviewboard/accounts/backends.py (Diff revision 4)
     
     
    Show all issues
    Col: 80
     E501 line too long (83 > 79 characters)
    
  3. 
      
AI
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/accounts/backends.py
        reviewboard/accounts/forms.py
      Ignored Files:
    
    
  2. 
      
DE
  1. regarding latest comments and changes, I cannot see any improvements
  2. 
      
david
  1. Ship It!
  2. 
      
AI
Review request changed
Status:
Completed
Change Summary:
Pushed to master (541e42b). Thanks!