Fix some bad assumptions for web API auth backends.

 






 

 


  Review Request #6077
  —
  Created July 7, 2014 and submitted 

 


















 

  

   Information

  





 
 

  chipx86
 









 
 

  Djblets
 









 
 

  release-0.8.x
 









 
 

  
 









 
 

  
 









 
 

  faaa074...
 






 




 

  

   Reviewers

  





 
 

  djblets
 









 
 

  
 






 








 





  


 
 

  
The web API auth backend support made a couple bad assumptions that

made it hard to write additional backends.


If the Basic Auth backend was first, and it encountered something that

wasn't a Basic Auth authorization header, it would log failures, even

though it wasn't the target backend. We now perform the realm check

first before trying to parse it.


The base WebAPIAuthBackend class itself was assuming in some places that

backends would want a username and a password. Working around that

assumption meant overriding login_with_credentials(). Now, that function

no longer cares about the specific contents of credentials, and farms

out to the Django auth backends and to a new validate_credentials() for

any specifics, making it easier to override.


Along with this, we now log all credential information passed to the

API, being careful to mask any sensitive information.

 








  


 
 

  
Tested this with a new auth backend that didn't deal in usernames and

passwords, and also didn't replace the Basic Auth backend. No longer saw

extra log messages, and didn't have to replace login_with_credentials().

 







 









 





  








 






 
 
 

  

   

    

     
    

    
    
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
    

   

   

    
     

      Description
      From
      Last Updated
     

    
    



     

      
       
       
Care to add _RE to the end of this name?

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
Can we just pass this inline in the logging call? logging.debug( 'Attempting authentication on API: %s', ', '.join([ '%s=%s' % …

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
This could probably be simplified, because the username won't be None: if (request.user.is_authenticated() and request.user.username == credentials.get('username')):

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
Can we move all of this into the try?

      		
      
       daviddavid
      
      
       
      
     


    
   

  

 







 






  

 








 














 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      djblets/webapi/auth.py
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Care to add _RE to the end of this name?
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      djblets/webapi/auth.py
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Can we just pass this inline in the logging call?
    

    

    logging.debug(
    'Attempting authentication on API: %s',
    ', '.join([
        '%s=%s' % pair
        for pair in six.iteritems(cleaned_credentials)
    ]),
    extra={'request': request})

    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      There are two log statements using it, so I'd rather not define it twice.
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      Ah, didn't notice the second one.
      
 
      

      3. 


 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      djblets/webapi/auth.py
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    This could probably be simplified, because the username won't be None:
    

    

    if (request.user.is_authenticated() and
    request.user.username == credentials.get('username')):

    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 






  5. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      djblets/webapi/auth.py
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Can we move all of this into the try?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      I don't see a reason to. We already have checked in the calling function at this point that HTTP_AUTHORIZATION exists. If you split and there's nothing in the split string there, you'll still get one item (the full string). Even with an empty string, you're guaranteed a result with that split:
      

      >>> ''.split(' ')
['']

      
 
      

      2. 


 
    


    

 
    

    6. 




  6. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    7. 




  

 
















 



 

  

   




chipx86




   

   

  

 



 















 



 

  

   




chipx86




   

   

  

 



 















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




chipx86




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  






 

  Status:
  

  Closed (submitted)

  
 




 
Change Summary:

 
Pushed to release-0.8.x (a08a381)





 


 










  

 
















 





 

  

   




reviewbot




   

   





  

 



 

  


   

   

    
 
reviewbot


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Tool: Pyflakes
Processed Files:
    djblets/webapi/auth.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/webapi/auth.py



Tool: Pyflakes
Processed Files:
    djblets/webapi/auth.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/webapi/auth.py


    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   




reviewbot




   

   





  

 



 

  


   

   

    
 
reviewbot


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Tool: Pyflakes
Processed Files:
    djblets/webapi/auth.py
    djblets/configforms/views.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/webapi/auth.py
    djblets/configforms/views.py


    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 









     

    

   

  

  

   
   Loading...