Update LDAP account sync to allow for more flexibility in name attributes.

 






 

 


  Review Request #2389
  —
  Created June 1, 2011 and submitted

 


















 

  

   Information

  





 
 

  junk
 









 
 

  Review Board
 









 
 

  
 









 
 

  2122
 









 
 

  
 






 




 

  

   Reviewers

  





 
 

  reviewboard
 









 
 

  
 






 








 





  


 
 

  
Added the following LDAP options:

* LDAP_GIVEN_NAME_ATTRIBUTE
* LDAP_SURNAME_ATTRIBUTE
* LDAP_FULL_NAME_ATTRIBUTE

While comments made in http://reviews.reviewboard.org/r/399/ are true (sn and givenName are "standard"), they're not required or mandated to use the same naming conventions.  They are listed as "good practice" and "acceptable" in the related RFCs but are up to the whim of the sysadmin building the system.  Because of this, having flexibility is beneficial.  None of these fields are required and the Full Name Attribute will always be ignored if a response is given for Given Name or Surname.

 








  


 
 

  
Tested against LDAP dev LDAP server with long names (greate than two) and changing name attributes for first_name and last_name.

 







 









 





  








 









 






  

 








 














 



 

  

   





 JU





   

   

  

 



 















 





 

  

   




chipx86




   

   





  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/accounts/backends.py
       


        (Diff revision 1)


       
      
     
    
        
    
     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    





    
    Might be better like:

given_name_attr = settings.LDAP_GIVEN_NAME_ATTRIBUTE or 'givenName'

first_name = user_info.get(given_name_attr, [username])[0]
    
  
    
 
    

 
    
  



 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/accounts/backends.py
       


        (Diff revision 1)


       
      
     
    
        
    
     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    





    
    Here too.

Shouldn't this be LDAP_SURNAME_ATTRIBUTE?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Lack of sleep and fixed by using user_info.get() correctly.
      
 
      

      2. 


 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/accounts/backends.py
       


        (Diff revision 1)


       
      
     
    
        
    
     
     
    

     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    





    
    Both of these are the same.

Also, multi-line conditionals should use parens, rather than \
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Lack of sleep and fixed by making LDAP_FULL_NAME_ATTRIBUTE take precedence.  It was required (and cleaner) once initial values were set for givenName and sn.
      
 
      

      2. 


 
    


    

 
    

    5. 






  5. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/accounts/backends.py
       


        (Diff revision 1)


       
      
     
    
        
    
     
     
    

     
     
    

     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    





    
    first_name, last_name = full_name.split(' ', 1)
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      "Longer is cleaner and often easier to understand" was drilled into me at a previous company.  Fixed.
      
 
      

      2. 


 
    


    

 
    

    6. 






  6. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/accounts/forms.py
       


        (Diff revision 1)


       
      
     
    
        
    
     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    





    
    What are these showing when using defaults? Just empty?

Maybe instead, these should default to "givenName" and stuff, which would simplify the logic above?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Done and it vastly simplified all of the logic.
      
 
      

      2. 


 
    


    

 
    

    7. 




  7. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    8. 




  

 
















 



 

  

   





 JU





   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  









  

   

    Change Summary:
   

   

    
This is why one should sleep before submitting code after long days...

Vastly simplified the change and tested with multiple cases:
* bad/missing attribute for given name
* bad/missing attribute for surname
* single entry for full name

Full Name Attribute now takes precedence.

   

  








  

   

    Diff:
   

   

    
 Revision 2 (+62 -23)
Show changes
 
    reviewboard/accounts/backends.py 
    reviewboard/accounts/forms.py 
    reviewboard/admin/siteconfig.py 
 

   

  












  

 
















 





 

  

   





 JU





   

   





  

 



 

  


   

   

    
 
junk


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ping?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Sorry, missed the updates on this. Giving it another pass now.
      
 
      

      2. 


 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   




chipx86




   

   





  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/accounts/backends.py
       


        (Diff revision 2)


       
      
     
    
        
    
     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    





    
    If settings.LDAP_GIVEN_NAME_ATTRIBUTE isn't set, this will probably fail, particularly on existing setups that don't have this saved. It also won't have the 'givenName' default.

So try:

given_name_attr = getattr(settings, 'LDAP_GIVEN_NAME_ATTRIBUTE', 'givenName')
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/accounts/backends.py
       


        (Diff revision 2)


       
      
     
    
        
    
     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    





    
    Should also have the getattr method.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/accounts/backends.py
       


        (Diff revision 2)


       
      
     
    
        
    
     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    





    
    Same. We can't rely on this being set.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 




  5. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    6. 




  

 
















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    I made the minor changes and pushed as cdc9fee. Thanks!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   





 RA





   

   







  

 



 

  


   

   

    
 
raghu182


    
   


  

  

  



    




  1. 
 
    
  
    
   
    testing.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Please do not use this server for testing purposes. http://demo.reviewboard.org/ is available for that.
      
 
      

      2. 


 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3.