arcus-azure / arcus.eventgrid

Azure Event Grid development in a breeze
https://eventgrid.arcus-azure.net/
MIT License
17 stars 6 forks source link

Endpoint validation is no longer working #172

Closed tomkerkhove closed 3 years ago

tomkerkhove commented 3 years ago

Describe the bug Endpoint validation is no longer working based on what I saw recently with @SamVanhoutte and we should check.

stijnmoreels commented 3 years ago

Is this about the updated EventGridAuthorizationAttribute? We have several web API integration tests to verify this. Which all seem to work. What seems to be the problem?

stijnmoreels commented 3 years ago

Mark that the attribute now uses our secret store to retrieve the secret.

tomkerkhove commented 3 years ago

As far as I recall, yes. However, it should block things if it can't find the secret, no?

But @SamVanhoutte can give more insights.

SamVanhoutte commented 3 years ago

Hello,

I have the code copied below and when I spin up the API (nothing specific towards Arcus in my Startup.cs), I see the following behavior:

Behavior

Sending a request without a x-api-key Expected: 401, because no secret header passed Actual: controller logic is being executed.

Sending a Subscription validation request, including the Aeg-Event-Type header with value SubscriptionValidation Expected: Response with validation code would be returned Actual: controller logic is being executed.

Code

[Route("[controller]")]
[ApiController]
public class ReviewController : ControllerBase
{
    private readonly ILogger<ReviewController> _logger;

    public ReviewController(ILogger<ReviewController> logger)
    {
        // Not sure the below line is needed, but added for trial purpose
        DynamicEventGridAuthorizationAttribute.RetrieveAuthenticationSecret = () => Task.FromResult("my-secret-key");
        _logger = logger;
    }

    [HttpPost("cycling/pic", Name= "Cycling_Picture_New")]
    [EventGridSubscriptionValidator]
    [EventGridAuthorization("x-api-key", "event-grid")]
    public async Task<IActionResult> HandleNewPicture(dynamic message)
    {
        var eventGridBatch = EventParser.Parse(message.ToString());
        // Removed code for brevity
        return Ok();
    }
}
stijnmoreels commented 3 years ago

Hmm, as I see it, we haven't yet released our new way to do this (using the secret store). The way it was tested in the repo here, is placing the secret retrieval in the startup. Not sure how to proceed with this @tomkerkhove , should we first release this new version instead trying to create a hotfix (if we can) for this?

tomkerkhove commented 3 years ago

Let's verify that it works in Sam his scenario and ship if it's OK! No need to hotfix this indeed.

SamVanhoutte commented 3 years ago

that is probably for the EventGridAuthorization attribute. but even if that one does not return 401, I would still expect the EventGridSubscriptionValidator to kick in as well , and return the validation code in the response.

stijnmoreels commented 3 years ago

that is probably for the EventGridAuthorization attribute. but even if that one does not return 401, I would still expect the EventGridSubscriptionValidator to kick in as well , and return the validation code in the response.

We can see if we get the same result with the current changes. Can you give us some more info on the dummy test project you set? If you could sent it internally to us, we could check it for you.

tomkerkhove commented 3 years ago

I think you can find that information here: https://github.com/arcus-azure/arcus.eventgrid/issues/172#issuecomment-881320398

stijnmoreels commented 3 years ago

Send you a mail, @SamVanhoutte with a test project that uses our new authorization attribute. Let us know if you experience any issues. Thanks.

SamVanhoutte commented 3 years ago

I have validated the EventGridAuthorization attribute behavior and can confirm that works. Did not find the time yet for the validation of the EventGridSubscriptionValidator , can look at that later today

stijnmoreels commented 3 years ago

I have validated the EventGridAuthorization attribute behavior and can confirm that works. Did not find the time yet for the validation of the EventGridSubscriptionValidator , can look at that later today

This subscription validation is not yet in the preview package, but is getting there: #175 We can close this one for now?