Add support for included credentials in webhook targets.

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

David Trowbridge
Review Board
release-2.5.x
427b75d...
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.
  • 0
  • 0
  • 7
  • 4
  • 11
Description From Last Updated
Checks run (1 failed, 1 succeeded, 1 failed with error)
JSHint passed.
PEP8 Style Checker internal error.
Pyflakes failed.

Pyflakes

David Trowbridge
Christian Hammond
  1. 
      
  2. reviewboard/notifications/models.py (Diff revision 2)
     
     

    **kwargs

  3. reviewboard/notifications/models.py (Diff revision 2)
     
     

    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. 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.

  4. reviewboard/notifications/validators.py (Diff revision 2)
     
     

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

  5. reviewboard/notifications/validators.py (Diff revision 2)
     
     
     
     

    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. 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+.

  6. reviewboard/notifications/webhooks.py (Diff revision 2)
     
     
     
     
     

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

  7. 
      
David Trowbridge
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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

David Trowbridge
Barret Rennie
  1. 
      
  2. reviewboard/notifications/webhooks.py (Diff revision 4)
     
     

    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.

  3. 
      
Christian Hammond
  1. Ship It!
  2. 
      
David Trowbridge
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.5.x (44f046e)
Loading...