Added support for authentication against Active Directory subdomains.

Review Request #2838 — Created Jan. 31, 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)
     
     
    userdomain could be an optional argument
  3. reviewboard/accounts/backends.py (Diff revision 1)
     
     
    userdomain could be an optional argument
  4. reviewboard/accounts/backends.py (Diff revision 1)
     
     
    userdomain could be an optional argument
  5. reviewboard/accounts/backends.py (Diff revision 1)
     
     
    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)
     
     
    userdomain could be an optional argument
  7. reviewboard/accounts/backends.py (Diff revision 1)
     
     
     
     
     
    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)
     
     
    userdomain not required
  9. reviewboard/accounts/forms.py (Diff revision 1)
     
     
    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)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  3. reviewboard/accounts/backends.py (Diff revision 2)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  4. reviewboard/accounts/backends.py (Diff revision 2)
     
     
    Col: 32
     E225 missing whitespace around operator
    
  5. reviewboard/accounts/backends.py (Diff revision 2)
     
     
    Col: 25
     W291 trailing whitespace
    
  6. reviewboard/accounts/backends.py (Diff revision 2)
     
     
    Col: 75
     W291 trailing whitespace
    
  7. reviewboard/accounts/forms.py (Diff revision 2)
     
     
    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)
     
     
    Remove this blank line.
  3. reviewboard/accounts/backends.py (Diff revision 3)
     
     
    Blank line after this.
  4. reviewboard/accounts/backends.py (Diff revision 3)
     
     
    Blank line after.
  5. reviewboard/accounts/backends.py (Diff revision 3)
     
     
    '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)
     
     
    Remove the parens.
  7. reviewboard/accounts/backends.py (Diff revision 3)
     
     
    Blank line before.
  8. reviewboard/accounts/backends.py (Diff revision 3)
     
     
    Blank line after.
    
    I'd prefer user_subdomain.
  9. reviewboard/accounts/backends.py (Diff revision 3)
     
     
    if '@' in username:
  10. reviewboard/accounts/backends.py (Diff revision 3)
     
     
     
     
    This can be one statement:
    
    username, subdomain = username.split('@', 1)
  11. reviewboard/accounts/backends.py (Diff revision 3)
     
     
    if '\\' in username:
  12. reviewboard/accounts/backends.py (Diff revision 3)
     
     
     
     
    Same comment as above.
  13. reviewboard/accounts/backends.py (Diff revision 3)
     
     
    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)
     
     
    Blank line before.
    
    Also should be:
    
        if user_subdomain:
  15. reviewboard/accounts/backends.py (Diff revision 3)
     
     
    Can you make this specific to mention Active Directory?
  16. reviewboard/accounts/backends.py (Diff revision 3)
     
     
    This seems to be here twice.
  17. reviewboard/accounts/forms.py (Diff revision 3)
     
     
    "... 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)
     
     
    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: Closed (submitted)

Change Summary:

Pushed to master (541e42b). Thanks!
Loading...