Make URL validation optional

[odise]

I just wonder why the URL schema for is so strict and limited to outlook.office365.com and outlook.office.com? I'm currently facing a use case with an URL schema like https://<corporate-name>.webhook.office.com which prevents me from using the library. Could give me some background here? Thanks a bunch.

[atc0005]

@odise: I just wonder why the URL schema for is so strict and limited to outlook.office365.com and outlook.office.com?

Ignorance.

The original library checked for https://outlook.office.com/webhook/ as a prefix explicitly and failed on any other patterns. When I found the library I was also reading through Microsoft's documentation and saw repeat mentions (either there or 3rd-party guides, I forget) of outlook.office365.com as an additional valid webhook domain. URL validation was extended to cover both use cases.

I wasn't aware of webhook.office.com, nor the larger https://<corporate-name>.webhook.office.com URL schema. I think it makes perfect sense to extend the validation, or modify it to better handle valid domains.

Having an option to disable it entirely could also work, but at first glance this looks like an API change would be needed.

Do you happen to have any references for webhook.office.com? I wonder now what other patterns exist.

[odise]

@atc0005 thank you for the swift reply. I'm not deep into MS Teams at all and also haven't seen any explicit URL schema definition so far. Actually I'm also not sure whether it makes sense to restrict the lib to some predefined patterns. But that's just my humble opinion of cause. So in the end I'm ok with adding my schema to the validation or/and skip validation entirely.

[atc0005]

@odise: Actually I'm also not sure whether it makes sense to restrict the lib to some predefined patterns.

Fair question/concern. The intent of the validation is to help prevent "odd" failure results when attempting to use the tool. If an invalid pattern can be (reliably) detected and reported early, this might save frustrating troubleshooting efforts later.

I think I understand what you may be saying though: you believe that the validation should occur within the client code and not the library itself?

But that's just my humble opinion of cause.

Sure. Feedback is appreciated. I'm just trying to see if there is a way to satisfy both concerns.

I checked earlier and couldn't find mention of webhook.office.com, but I didn't throw a lot of effort at it. How did you learn about it? For example, our org uses Teams fairly heavily, so I create our webhook connectors using Teams and it provides them with the https://outlook.office.com/webhook/ URL pattern.

Are you using Teams and it's giving you something different, or perhaps using a dashboard somewhere that generates them in the pattern you shared?

[odise]

I think I understand what you may be saying though: you believe that the validation should occur within the client code and not the library itself?

Yeah more or less like that. I mean you could even think of using a local mock endpoint for integration testing and that validation would make it impossible.

Are you using Teams and it's giving you something different, or perhaps using a dashboard somewhere that generates them in the pattern you shared?

I just generated the web hook URL from the Teams channel connector menu. The company is a pretty compliancy driven organisation so it might be that some parts are not following the standardised Outlook universe.

[atc0005]

Yeah more or less like that. I mean you could even think of using a local mock endpoint for integration testing and that validation would make it impossible.

Good point.

I'm certainly open to suggestions. I'd like to see some sort of validation remain to ensure that the webhook URL is in at least a usable format.

An earlier implementation of the code applied both url.Parse and the strings.HasPrefix check against the known webhook domain at the time:

https://github.com/dasrick/go-teams-notify/blob/1f93e29ec9f60cb01ee83e66dc35871a6a63956b/send.go

What jumps out as options:

• weaken the current validation to only enforce url.Parse requirement
  • this seems the most flexible, but might surprise anyone relying on the existing stricter validation requirements
• add another case entry to match a regex pattern for *.webhook.office.com
  • seems fragile?
• create a variant Send* method that skips webhook URL validation
  • generic and flexible enough?

I still consider myself fairly new to Go, so I suspect that there are likely other, better ways to handle this and I am personally open to those ideas. My main concern is not breaking the API or expectations for other consumers of the package.

[odise]

Maybe the best way of not breaking the API would be to add an optional flag to teamsClient , implement a setter and check it in SendWithContext like:

type teamsClient struct {
	httpClient *http.Client
        skipWebUrlValidation bool
}
func (c teamsClient) SkipWebUrlValidationOnSend(skip bool) {
        c.skipWebUrlValidation = skip
}
// set skipWebUrlValidation = false by default
mstClient := goteamsnotify.NewClient()
// optionally disable URL validation
mstClient.SkipWebUrlValidationOnSend(true)
mstClient.Send(webhookUrl, msgCard)

WDYT?

[atc0005]

@odise

I like it. Very clean and with minimal changes required. Would you like to submit a PR for this? If not, I can probably work this in later today or early tomorrow.

[atc0005]

@odise

I like it. Very clean and with minimal changes required. Would you like to submit a PR for this? If not, I can probably work this in later today or early tomorrow.

Other tasks ran longer than expected today, so I'm out of time for dev work. I'll plan to work on this in the AM and cut a new release with the changes. If you'd like to submit a PR instead, please let me know.

[odise]

@atc0005 please have a look at #69.

[atc0005]

Thoughts:

1. implemented hook to skip webhook URL validation #69 to resolve the immediate problem
2. further work to update docs, tests

While looking over the tests, I'm seeing some issues that may need to be worked through so I'll look at that after merging this. I'm optimistic that everything will be sorted for a new release this AM.

[atc0005]

@odise

I may be missing something, but if NewClient() is used to create a new client the SkipWebhookURLValidationOnSend() method is unavailable. Since teamsClient is unexported, it does not look like package consumers can directly instantiate teamsClient directly.

It looks like we'll have to add SkipWebhookURLValidationOnSend() to the API interface to expose it for use?

[odise]

@atc0005 you are absolutely right. #73 should fix that. Sorry for the inconvenience.

[atc0005]

@odise: @atc0005 you are absolutely right. #73 should fix that. Sorry for the inconvenience.

No problem, just wanted to make sure I wasn't overlooking something.

[atc0005]

Thoughts:

1. implemented hook to skip webhook URL validation #69 to resolve the immediate problem
2. further work to update docs, tests

While looking over the tests, I'm seeing some issues that may need to be worked through so I'll look at that after merging this. I'm optimistic that everything will be sorted for a new release this AM.

@odise Unless I missed something, all items related to this are now complete. Cutting a new v2.4.0 release soon.