Summary

Add support for included credentials in webhook targets.

Review Request #8916 — Created April 27, 2017 and submitted June 8, 2017, 4:01 p.m.

Information

Owner

david

Repository

Review Board

Branch

release-2.5.x

Bugs

Depends On

Commit

427b75d...

Reviewers

Groups

reviewboard

People

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

Testing Done


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.

Issues

Description	From	Last Updated
'django.utils.six.moves.urllib.request.urlopen' imported but unused	reviewbot	April 27, 2017, 1:09 p.m.
*args	chipx86	May 30, 2017, 11:43 a.m.
**kwargs	chipx86	May 30, 2017, 11:43 a.m.
Shouldn't you be able to just set this on the field itself above using validators=?	chipx86	May 26, 2017, 1:42 p.m.
This file's missing a file docstring. (Should also be in in docs/manual/coderef/index.rst).	chipx86	May 30, 2017, 11:27 a.m.
Hmm, I don't know how else we'd handle this, but I sure wish we didn't have to copy this... This …	chipx86	May 26, 2017, 1:42 p.m.
This will be more readable using the attribute names instead of indexes.	chipx86	May 30, 2017, 2:25 p.m.
F811 redefinition of unused 'URLValidator' from line 7	reviewbot	May 30, 2017, 2:30 p.m.
E501 line too long (80 > 79 characters)	reviewbot	May 30, 2017, 2:31 p.m.
E501 line too long (80 > 79 characters)	reviewbot	May 30, 2017, 2:31 p.m.
you can use url_parts.hostname and urlparts.port instead of splitting this.	brennie	June 1, 2017, 12:45 p.m.
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	Sept. 10, 2020, 1:24 a.m.

JSHint passed.

PEP8 Style Checker internal error.

Pyflakes failed.

Pyflakes

reviewboard/notifications/webhooks.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'django.utils.six.moves.urllib.request.urlopen' imported but unused
```

Commit:

52b1ac421a622e81e189668bd63520b07c776596

9d987acd1c709167ce61d38855d604b8629abf49

Diff:

Revision 2 (+85 -20)

Show changes

	reviewboard/notifications/forms.py
	reviewboard/notifications/models.py
	reviewboard/notifications/tests.py
	reviewboard/notifications/validators.py
	reviewboard/notifications/webhooks.py

Checks run (2 succeeded, 1 failed with error)

JSHint passed.

PEP8 Style Checker internal error.

Pyflakes passed.

reviewboard/notifications/models.py (Diff revision 2)
The issue has been resolved. Show all issues
```
*args
```
reviewboard/notifications/models.py (Diff revision 2)
The issue has been resolved. Show all issues
```
**kwargs
```

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

The issue has been dropped. Show all issues

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

david May 26, 2017, 1:42 p.m.

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.

chipx86 May 26, 2017, 1:54 p.m.

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.

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

The issue has been dropped. Show all issues

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

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

The issue has been dropped. 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?

david May 26, 2017, 1:42 p.m.

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

chipx86 May 26, 2017, 1:54 p.m.

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

reviewboard/notifications/webhooks.py (Diff revision 2)
The issue has been resolved. Show all issues
```
This will be more readable using the attribute names instead of indexes.
```

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

reviewboard/notifications/forms.py (Diff revision 3)
The issue has been resolved. Show all issues
```
F811 redefinition of unused 'URLValidator' from line 7
```
reviewboard/notifications/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
E501 line too long (80 > 79 characters)
```
reviewboard/notifications/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
E501 line too long (80 > 79 characters)
```

Commit:

bac403535da00dc334c5dcd6ba4435efd2fd8b6c

427b75da2e9d42c35e11eb5a8cea4908b6e51186

Diff:

Revision 4 (+64 -20)

Show changes

	reviewboard/notifications/forms.py
	reviewboard/notifications/models.py
	reviewboard/notifications/tests.py
	reviewboard/notifications/webhooks.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

reviewboard/notifications/webhooks.py (Diff revision 4)

The issue has been dropped. Show all issues

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

david June 1, 2017, 12:43 p.m.

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

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-2.5.x (44f046e)

Fix it!

reviewboard/notifications/webhooks.py (Diff revision 4)

An issue was opened. 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))