aspnet / WebHooks

[Archived] Libraries to create and consume web hooks on ASP.NET Core. Project moved to https://github.com/aspnet/AspLabs
Apache License 2.0
627 stars 439 forks source link

WebHookSecurityFilter should use `DefaultIdConfigurationKey` in log when id is null or empty #244

Closed pranavkm closed 5 years ago

pranavkm commented 6 years ago

https://github.com/aspnet/WebHooks/blob/a05a07c420e6316963f41d7bad3fc38e6b74464c/src/Microsoft.AspNetCore.WebHooks.Receivers/Filters/WebHookSecurityFilter.cs#L288-L292

This prints Could not find a valid configuration for the 'github' WebHook receiver, instance '' when the key is invalid.

pranavkm commented 6 years ago

Also https://github.com/aspnet/WebHooks/blob/a05a07c420e6316963f41d7bad3fc38e6b74464c/src/Microsoft.AspNetCore.WebHooks.Receivers/Filters/WebHookSecurityFilter.cs#L219-L220

pranavkm commented 6 years ago

Typo: https://github.com/aspnet/WebHooks/blob/a05a07c420e6316963f41d7bad3fc38e6b74464c/src/Microsoft.AspNetCore.WebHooks.Receivers/Filters/WebHookSecurityFilter.cs#L215

Marusyk commented 6 years ago

Hello, do you mean that when the key is invalid should it return other message? I would be glad to fix that if you clarify requirements. Thx

pranavkm commented 6 years ago

@Marusyk no, the intent was to use the term "default id" or something along those lines when the key is the default value. Right now it prints empty quotes, which isn't what the user configures in the log. Consider

[GitHubWebHook]
public IActionResult Blah() {..}

The log message you see in this case is Could not find a valid configuration for the 'github' WebHook receiver, instance ''. Having '' instance isn't very intuitive. A more appropriate message might be Could not find a valid configuration for the 'github' WebHook receiver, default instance.

dougbu commented 6 years ago

Self-assigning this and two other bugs because we want them done in RC1.

@mkArtakMSFT please add cost: XS label in this repo. This issue and #247 fit in that bucket. Perhaps #245 too.

aspnet-hello commented 5 years ago

This issue was moved to aspnet/AspLabs#46