Look into: RequireAntiForgeryCheck: false

[brockallen]

With this fix the RequireAntiForgeryCheck: false stopped working for the local API when set at the controller action level attribute like this:

// GET productapi/v1/image/5
[BffApi(false)]
[HttpGet("{id:length(24)}")]
public Task<ActionResult> Get(string id) => (

It now requires the anti-forgery header for this action and returns 401. If rollback to 1.2.1 it works as expected.

Originally posted by @dgrozenok in #102 (comment)

[brockallen]

Also, it would help to have more automated tests around the various attributes at the various levels.

[brockallen]

Hmm.. so I did some digging and I found an odd behavior: GetOrderedMetadata does not return attributes in the order I would have expected. The order is: 1) controller, 2) action, 3) route. This is different from how MVC used to work. In any event, it doesn't matter if we use GetOrderedMetadata or GetMetadata, since they work the same (in essence). So I think more investigation is in order.

@ArturDorochowicz -- interesting info for you.

[dgrozenok]

In addition to the action attribute BffApi(false) I also have this in the Startup class:

app.UseEndpoints(endpoints =>
        {
            endpoints.MapControllers()
                .RequireAuthorization("ApiScope")
                .AsBffApiEndpoint();

[ArturDorochowicz]

I'm on vacation and have limited ability to help. My quick thoughts below. 1. The issue was real. BFF used the least important metadata and that was wrong. 2. MapControllers does not create a single endpoint. Each action (more or less) is an endpoint. If you add metadata to an endpoint, you're adding it to the action and it makes sense that it has higher priority than attributes. 3. You seem to be expecting an mvc global filter like behavior. In that case maybe have an mvc filter (that existing attribute can be that, i think) and explain this in the docs. wt., 21 cze 2022, 01:12 użytkownik Brock Allen ***@***.***> napisał:

…

Hmm.. so I did some digging and I found an odd behavior: GetOrderedMetadata does not return attributes in the order I would have expected. The order is: 1) controller, 2) action, 3) route. This is different from how MVC used to work. In any event, it doesn't matter if we use GetOrderedMetadata or GetMetadata, since they work the same (in essence). So I think more investigation is in order. @ArturDorochowicz <https://github.com/ArturDorochowicz> -- interesting info for you. — Reply to this email directly, view it on GitHub <#103 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADV444HUMLDKYS4QFXNSLLVQD3ETANCNFSM5ZJK4IKQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[brockallen]

BFF used the least important metadata

Perhaps some of the confusion is the definition of this. Our assumption was that the order was at the route config (in startup), then controller, then action. And to me the action is most important. But of course, this issue has raise the fact that there is no hierarchy and instead it's just a flat list. This makes me wonder why there's an API called GetOrderedMetadata at all if it's just a flat list. I suspect someone thinks there's an order, but is that documented? Based on the order I see, it's not intuitive so it needs formal docs.

In any event -- we are going to roll back the patch given that our assumptions were incorrect. We will then address your issue @ArturDorochowicz with a v2.0 because we expect that it will require a breaking change.

[ArturDorochowicz]

If you revert the change, controller attributes will be more important than action attributes. I thought you wanted mvc like behavior... And that's definitely not it. wt., 21 cze 2022, 15:54 użytkownik Brock Allen ***@***.***> napisał:

…

BFF used the least important metadata Perhaps some of the confusion is the definition of this. Our assumption was that the order was at the route config (in startup), then controller, then action. And to me the action is most important. But of course, this issue has raise the fact that there is no hierarchy and instead it's just a flat list. This makes me wonder why there's an API called GetOrderedMetadata at all if it's just a flat list. I suspect someone thinks there's an order, but is that documented? Based on the order I see, it's not intuitive so it needs formal docs. In any event -- we are going to roll back the patch given that our assumptions were incorrect. We will then address your issue @ArturDorochowicz <https://github.com/ArturDorochowicz> with a v2.0 because we expect that it will require a breaking change. — Reply to this email directly, view it on GitHub <#103 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADV44YRN7AK63HUOLASSILVQHCSJANCNFSM5ZJK4IKQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[leastprivilege]

OK - sorry for the confusion. We will roll-back the change and re-design this feature.

[dgrozenok]

Interesting. I didn't realize the configuration of an endpoint is at the same level as the action. I do agree though that from the user of the package standpoint I would prefer the mvc global filter-like behavior with the hierarchy starting from the Startup, then controller, and the action being the most specific.

[leastprivilege]

Agreed. The plan is to change the behavior so it is inline with RequireAuthorization, [Authorize] and [AllowAnonymous].

That should feel more predictable for most people.

[brockallen]

I do agree though that from the user of the package standpoint I would prefer the mvc global filter-like behavior with the hierarchy starting from the Startup, then controller, and the action being the most specific.

Yes, and given that attributes added to the route show up as action-level shows that this implementation detail is not well understood, so we're going to need to take the [AllowAnonymous] approach (given that they're all essentially flat).

[brockallen]

Also, it would help to have more automated tests around the various attributes at the various levels.

Looks like we already had tests for the non-YARP anti-xsrf.