Add support for included credentials in webhook targets.

 






 

 


  Review Request #8916
  —
  Created April 27, 2017 and submitted 

 


















 

  

   Information

  





 
 

  david
 









 
 

  Review Board
 









 
 

  release-2.5.x
 









 
 

  
 









 
 

  
 









 
 

  427b75d...
 






 




 

  

   Reviewers

  





 
 

  reviewboard
 









 
 

  
 






 








 





  


 
 

  
Sometimes, a user may need to dispatch a webhook to an authenticated endpoint.

Django (somewhat sensibly) does not allow inline credentials in URLFields, both

in forms and models, but in this case we want to allow it.


This change makes use of a new backport of Django 1.8's URLValidator, which

supports credentials. The URL is then split and credentials (if included) are

pulled out and added to a HTTPBasicAuthHandler for the url open.

 








  


 
 

  
    

  • Verified that I was able to save webhook targets that included credentials
    
  in the URL.
    • 

  • Checked that things that definitely weren't URLs were still rejected.
    • 

  • Ran unit tests.
    • 


 







 









 





  








 






 
 
 

  

   

    

     
    

    
    
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
    

   

   

    
     

      Description
      From
      Last Updated
     

    
    



     

      
       
       
'django.utils.six.moves.urllib.request.urlopen' imported but unused

      		
      
       reviewbotreviewbot
      
      
       
      
     




     

      
       
       
*args

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
**kwargs

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
Shouldn't you be able to just set this on the field itself above using validators=?

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
This file's missing a file docstring. (Should also be in in docs/manual/coderef/index.rst).

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
Hmm, I don't know how else we'd handle this, but I sure wish we didn't have to copy this... This …

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
This will be more readable using the attribute names instead of indexes.

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
F811 redefinition of unused 'URLValidator' from line 7

      		
      
       reviewbotreviewbot
      
      
       
      
     




     

      
       
       
E501 line too long (80 > 79 characters)

      		
      
       reviewbotreviewbot
      
      
       
      
     




     

      
       
       
E501 line too long (80 > 79 characters)

      		
      
       reviewbotreviewbot
      
      
       
      
     




     

      
       
       
you can use url_parts.hostname and urlparts.port instead of splitting this.

      		
      
       brenniebrennie
      
      
       
      
     




     

      
       
       
This raises an exception: AttributeError: 'SplitResult' object has no attribute 'params' Instead of url_parts.params there should have been a url_params.fragment: …

      		
      
       
 YX
yxejamir
      
      
       
      
     


    
   

  

 







 






  

 



















 




 

  


   

   

    
 
Checks run (1 failed, 1 succeeded, 1 failed with error)


    
   


  

  

  





 

 JSHint
 passed.
 





 

 PEP8 Style Checker
 internal error.
 





 

 Pyflakes
 failed.
 



















 
Pyflakes

 


 







  

 





 














 



 

  

   




david




   

   

  

 



 















 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/notifications/models.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    *args
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/notifications/models.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    **kwargs
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/notifications/models.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Shouldn't you be able to just set this on the field itself above using validators=?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Django sees a models.URLField and instantiates a forms.URLField. Setting validators on the model field will add it to the form field in addition to the default validators, where what we want to do is replace it.
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      Maybe what we should do then, since this isn't an isolated problem for this form, is to move the validator into Djblets and have a subclas of URLField that overrides default_validators. Then we can easily just drop this into this form or any other where we need this down the road.
      
 
      

      3. 


 
    


    

 
    

    5. 






  5. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/notifications/validators.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    This file's missing a file docstring. (Should also be in in docs/manual/coderef/index.rst).
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    6. 






  6. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/notifications/validators.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Hmm, I don't know how else we'd handle this, but I sure wish we didn't have to copy this... This regex has come up a few times now in Django security updates. Is there any way we can do a pass to parse out credentials and then let the parent validate the rest?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      I can't see any way of doing that that wouldn't duplicate the regex in some form.
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      So I looked into the validation logic in Django 1.6 and in newer versions. They've completely redone the logic in 1.8, which adds support for user/pass validation and just generally fixes a lot of other validation issues (better handling newer TLDs and such). Maybe we could copy the newer one to djblets.util.compat.django.core.validators and do:
      

      

      from django.core.validators import URLValidator


# Some note about Django 1.8 vs. older versions...
if not hasattr(URLValidator, 'hostname_re'):  # <-- hostname_re was introduced in the new validator
    class URLValidator(...)
        ...

      

      

      Then we could import from there, ensuring we're using this validator only on Django 1.6 and using the upstream one on 1.8+.
      
 
      

      3. 


 
    


    

 
    

    7. 






  7. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/notifications/webhooks.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    This will be more readable using the attribute names instead of indexes.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    8. 




  8. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    9. 




  

 
















 



 

  

   




david




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  











 



  

   
Description:

   
    
     
Sometimes, a user may need to dispatch a webhook to an authenticated endpoint.
     Django (somewhat sensibly) does not allow inline credentials in URLFields, both
     in forms and models, but in this case we want to allow it.
     

 ~   
This change adds a new validator that works like Django's URLValidator but
 ~   includes the credentials within the regex. The URL is then split and
 ~   credentials (if included) are pulled out and added to a HTTPBasicAuthHandler
   ~ 
This change makes use of a new backport of Django 1.8's URLValidator, which
   ~ supports credentials. The URL is then split and credentials (if included) are
   ~ pulled out and added to a HTTPBasicAuthHandler for the url open.
 -   for the url open.

   		
  




  

   
Commit:

   
    
-9d987acd1c709167ce61d38855d604b8629abf49
+bac403535da00dc334c5dcd6ba4435efd2fd8b6c

   		
  




  

   
Diff:

   
    
 Revision 3 (+63 -20)
Show changes
 
    reviewboard/notifications/forms.py 
    reviewboard/notifications/models.py 
    reviewboard/notifications/tests.py 
    reviewboard/notifications/webhooks.py 
 

   		
  



 









 

  
Checks run (1 failed, 1 succeeded)



  

 flake8
 failed.
 





  

 JSHint
 passed.
 





 







 
flake8

 


 















  

 
















 



 

  

   




david




   

   

  

 



 















 





 

  

   




brennie




   

   









  

 



 

  


   

   

    
 
brennie


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/notifications/webhooks.py
      


       (Diff revision 4)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    you can use url_parts.hostname and urlparts.port instead of splitting this.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      This isn't splitting out the port, it's splitting out the credentials.
      
 
      

      2. 


 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




david




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  






 

  Status:
  

  Closed (submitted)

  
 




 
Change Summary:

 
Pushed to release-2.5.x (44f046e)





 


 










  

 
















 





 

  

   





 YX





   

   







  

 



 

  


   

   

    
 
yxejamir


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/notifications/webhooks.py
      


       (Diff revision 4)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    This raises an exception:
    

    

    AttributeError: 'SplitResult' object has no attribute 'params'

    

    

    Instead of url_parts.params there should have been a url_params.fragment:
    

    

                    url = urlunsplit(
                    (url_parts.scheme, netloc, url_parts.path,
                     url_parts.query, url_parts.fragment))

    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 









     

    

   

  

  

   
   Loading...