use CustomURLValidator in custom_button

[hoptical]

What this PR does

This PR, overrides Django URLValidator with a CustomURLValidator. It just removes tld_re part from the regex, and the other behaviour remains the same.
The CustomURLValidator is defined in common.api_helpers.utils.py file and is utilized in custom_button.py.
Please inform me if it needs to be defined somewhere else or be implemented with some other methods.

Which issue(s) this PR fixes

Currently, URLValidator raises exception for URLs that don't have TLD. This leads to not being able to use containers URL for outgoing webhooks as they usually don't have TLD.

Checklist

• Tests updated
• Documentation added
• CHANGELOG.md updated

[CLAassistant]

All committers have signed the CLA.

[Konstantinov-Innokentii]

Hey @hoptical, thanks for the contribution. Could you please add test for validation of url without TLD and example of your use-case. Also your custom validator should be used only if DANGEROUS_WEBHOOKS_ENABLED is set to true, since this settings makes webhooks checks rules less strict and we don't want to allow less validation in our cloud.

[Konstantinov-Innokentii]

Is there any reason to override whole class? Isn't it enough to override just this part of URLValidator, removing tld_re from that?

 tld_re = (
        r'\.'                                # dot
        r'(?!-)'                             # can't start with a dash
        r'(?:[a-z' + ul + '-]{2,63}'         # domain label
        r'|xn--[a-z0-9]{1,59})'              # or punycode label
        r'(?<!-)'                            # can't end with a dash
        r'\.?'                               # may have a trailing dot
    )
    host_re = '(' + hostname_re + domain_re + tld_re + '|localhost)'

Also I propose to rename that to "URLValidatorWithoutTLD" or something more meaningful.

[hoptical]

I tried it. However, the overridden process is not effective unless we override the regex attribute. To override it I've to bring all other necessary variables (ip4_re, ip6_re, host_re).
I couldn't find a way to make it more concise. It would be great if you found a better way.

In terms of the class name, I will change it to what you proposed.

[joeyorlando]

hi @hoptical, thanks for this contribution. Be sure to sign the CLA and add a small note to the CHANGELOG.md summarizing this change. Thanks!

[Konstantinov-Innokentii]

Hello @hoptical, can I help you to finish this PR?

[hoptical]

hi @hoptical, thanks for this contribution. Be sure to sign the CLA and add a small note to the CHANGELOG.md summarizing this change. Thanks!

Hi @joeyorlando,

I've signed the CLA. But I guess my commit email is not assigned to my GitHub account, so I'm not sure if my CLA signing is working properly.

In terms of changelog, surely I do that.|

Thanks.

[hoptical]

Hello @hoptical, can I help you to finish this PR?

Hi @Konstantinov-Innokentii,
Sorry for the late response.

No, it's fine. I'll apply your review comments ASAP and get back to you.
Thanks.

[hoptical]

Hey @hoptical, thanks for the contribution. Could you please add test for validation of url without TLD and example of your use-case. Also your custom validator should be used only if DANGEROUS_WEBHOOKS_ENABLED is set to true, since this settings makes webhooks checks rules less strict and we don't want to allow less validation in our cloud.

I've added the DANGEROUS_WEBHOOKS_ENABLED option.

Regarding the tests, would you please guide me through the file to which I should add the test?

Furthermore, I'll update the changelog.md and squash the commits once you've approved the PR.

[Konstantinov-Innokentii]

@hoptical you can place validator tests in the common.tests folder, just create new test_urlvalidator_without_tld.py file there. If you want to test how validator works while making requests to the api add a new test in the apps/api/tests/test_custom_button.py file ( see test_create_custom_button for refefence).

[joeyorlando]

hi there @hoptical 👋 just wanted to check in regarding @Konstantinov-Innokentii's comment

[hoptical]

hi there @hoptical wave just wanted to check in regarding @Konstantinov-Innokentii's comment

Hi @joeyorlando,
Sure. I'm going to add tests, update the changelog and finish the PR.

[hoptical]

@joeyorlando I've added the unit tests in engine/common/tests/test_urlvalidator_without_tld.py.
It seems that there are some conflicts at engine/common/api_helpers/utils.py. Would you please resolve those conflicts and update the PR with the last changes of main? Then I'm able to update the changelog and finish the PR.

Additionally, please inform me if there are other works to do.
Thanks.

[joeyorlando]

@hoptical merge conflicts have been resolved 👍

[joeyorlando]

one other request. do you mind also adding a set of e2e integration tests for this new change?

here is the section in the docs on how to run them locally and here is an example of a test that was added in a recent PR

[Konstantinov-Innokentii]

Hello @hoptical. Thanks for tests. I was able to simplify your validator based on examples from tests and that discussion

class URLValidatorWithoutTLD(URLValidator):
    host_re = '(' + URLValidator.hostname_re + URLValidator.domain_re + URLValidator.tld_re + '|' + URLValidator.hostname_re + '|localhost)'

    regex = _lazy_re_compile(
        r"^(?:[a-z0-9.+-]*)://"  # scheme is validated separately
        r"(?:[^\s:@/]+(?::[^\s:@/]*)?@)?"  # user:pass authentication
        r"(?:" + URLValidator.ipv4_re + "|" + URLValidator.ipv6_re + "|" + host_re + ")"
        r"(?::[0-9]{1,5})?"  # port
        r"(?:[/?#][^\s]*)?"  # resource path
        r"\Z",re.IGNORECASE,
    )

Could you confirm, that this Validator works for you?

@joeyorlando, @hoptical I'm not sure we need to require e2e tests for such change. It affects only OSS, it's small and doesn't introduce any UI changes. Just API unittest in apps/api/tests/test_custom_button.py is enough.
It will look like that:

URL_WITH_TLD = "http://www.google.com"
URL_WITHOUT_TLD = "http://container:8080"


@pytest.mark.django_db
@pytest.mark.parametrize(
    "dangerous_webhooks,webhook_url,expected_status",
    [
        (True, URL_WITH_TLD, status.HTTP_201_CREATED),
        (True, URL_WITHOUT_TLD, status.HTTP_201_CREATED),
        (False, URL_WITH_TLD, status.HTTP_201_CREATED),
        (False, URL_WITHOUT_TLD, status.HTTP_400_BAD_REQUEST),
    ],
)
def test_url_without_tld_custom_button(
        custom_button_internal_api_setup,
        make_user_auth_headers,
        settings,
        dangerous_webhooks,
        webhook_url,
        expected_status,
):
    settings.DANGEROUS_WEBHOOKS_ENABLED = dangerous_webhooks

    user, token, _ = custom_button_internal_api_setup
    client = APIClient()
    url = reverse("api-internal:custom_button-list")

    data = {
        "name": "amixr_button",
        "webhook": webhook_url,
        "team": None,
    }
    response = client.post(url, data, format="json", **make_user_auth_headers(user, token))
    assert response.status_code == expected_status

[joeyorlando]

It affects only OSS

This will also have an impact on cloud deployments

it's small and doesn't introduce any UI changes

I believe the motivation for this PR was a bug that originated in the UI; inability to create a webhook which had these types of URLs, via the UI form.

[Konstantinov-Innokentii]

@joeyorlando It will not affect cloud. This feature will be under DANGEROUS_WEBHOOK_ENABLED flag, we will not allow create webhooks which will point to urls withour TLD in cloud (mostly docker containers urls).
It's not a bug, that UI form in cloud doesn't allow such kind of URLs.
It was discussed here

[hoptical]

@Konstantinov-Innokentii Thanks for your simplification and test recommendation.
Yes, it works properly. I've applied it, as well as the API test. I updated the changelog.md too.
Please review and inform me in case of necessary changes.

I recommend squashing the PR commits. What do you think?

[Konstantinov-Innokentii]

@hoptical all good, approved that. Last thing - ci/lint job fails on markdown with:

CHANGELOG.md:15:121 MD013/line-length Line length [Expected: 120; Actual: 237]
CHANGELOG.md:15:121 MD013/line-length Line length [Expected: 120; Actual: 237]
CHANGELOG.md:17 MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 2]

markdownlint - docs......................................................Failed
- hook id: markdownlint
- exit code: 1

CHANGELOG.md:15:161 MD013/line-length Line length [Expected: 160; Actual: 237]

run make lint to check that locally

Upd: I fixed that and some linting errors by myself, didn't want to bother you with minor stuff. For next contributions make sure you set pre-commit locally: make install-precommit-hook

[Konstantinov-Innokentii]

@hoptical merged, thanks for your contribution!

[hoptical]

@hoptical all good, approved that. Last thing - ci/lint job fails on markdown with:

CHANGELOG.md:15:121 MD013/line-length Line length [Expected: 120; Actual: 237]
CHANGELOG.md:15:121 MD013/line-length Line length [Expected: 120; Actual: 237]
CHANGELOG.md:17 MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 2]

markdownlint - docs......................................................Failed
- hook id: markdownlint
- exit code: 1

CHANGELOG.md:15:161 MD013/line-length Line length [Expected: 160; Actual: 237]

run make lint to check that locally

Upd: I fixed that and some linting errors by myself, didn't want to bother you with minor stuff. For next contributions make sure you set pre-commit locally: make install-precommit-hook

Thanks @Konstantinov-Innokentii
Sure I will.

[hoptical]

@joeyorlando Is something wrong with the merge process? I see that the PR is being added to Merge Queue but then is removed from the queue by GitHub-merge-queue.