Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sample code for MVC context in authz handlers. #15218

Closed
wants to merge 5 commits into from
Closed

Sample code for MVC context in authz handlers. #15218

wants to merge 5 commits into from

Conversation

serpent5
Copy link
Contributor

@serpent5 serpent5 commented Oct 20, 2019

@serpent5
Copy link
Contributor Author

I've added some sample code and questions into the .md file. Is it possible to get someone to review the questions before I attempt to write the content that sits around this code?

@Rick-Anderson
Copy link
Contributor

Review questions from @serpent5

Questions:
1. Is the code shown below a valid demonstration?
2. Should this just assume endpoint-routing, or should it also cover that disabling endpoint-routing gives the 2.2 behavior?
3. Should this call out that filters can also be accessed directly from the Metadata collection?
However, I wonder if that kind of detail would be better placed in the MVC pages. The sample code here might be just a way of "getting started" with this stuff.
4. Should there be a suggestion of injecting IHttpContextAccessor for non-MVC stuff like HttpRequest?

@HaoK please review

@Rick-Anderson
Copy link
Contributor

@serpent5 check my commit please

@serpent5
Copy link
Contributor Author

@Rick-Anderson Yeah, looks fine. I plan to add some explanation around the code if the approach gets approved. I'll make sure that I ask questions here rather than as markdown comments going forward too.

@HaoK
Copy link
Member

HaoK commented Oct 21, 2019

@pranavkm can you help answer some best practices questions around MVC/endpoint routing for this doc update

@Rick-Anderson
Copy link
Contributor

I'll make sure that I ask questions here rather than as markdown comments going forward too.

Ha, they often get ignored here. They're more likely to get noticed in the file diffs. I often do both places.

@Rick-Anderson
Copy link
Contributor

@pranavkm can you help answer some best practices questions around MVC/endpoint routing for this doc update

@IGx89
Copy link

IGx89 commented Oct 22, 2019

Would love to see it include code showing how to access RouteData, like the 2.2 docs describe. Haven't found a way in 3.0 to do that yet. Use case would be a route such as /api/user/{userId}/xxx that an authorization handler limits to just the current authenticated user's userId (stored in context.User.Claims). Doing so requires getting access to the value of that route parameter.

@serpent5
Copy link
Contributor Author

@IGx89 You can achieve that by injecting IHttpContextAccessor and using HttpContext.GetRouteData(). I don't know if that's the recommended approach, so I encourage you to wait for input from the experts here, but that might be a working temporary solution, at least.

@Rick-Anderson Rick-Anderson marked this pull request as ready for review October 22, 2019 23:15
@pranavkm
Copy link
Contributor

Questions:

  1. Is the code shown below a valid demonstration?

I guess. Is the intent to show how to identify what MVC endpoint is meant to be executed? If so, this works.

  1. Should this just assume endpoint-routing, or should it also cover that disabling endpoint-routing gives the 2.2 behavior?

I wouldn't until there's user asks for this.

  1. Should this call out that filters can also be accessed directly from the Metadata collection? However, I wonder if that kind of detail would be better placed in the MVC pages. The sample code here might be just a way of "getting started" with this stuff.

You may access attributes decorating the endpoint which includes filter attributes. Do you happen to know what sort of metadata is it that users are attempting to read?

  1. Should there be a suggestion of injecting IHttpContextAccessor for non-MVC stuff like HttpRequest?

@rynowak \ @JamesNK \ @davidfowl what do you guys think about this? You used to be able to get to HttpContext by downcasting https://github.com/aspnet/AspNetCore/blob/master/src/Security/Authorization/Core/src/AuthorizationHandlerContext.cs#L55 to the ActionContext when auth was executing as a filter. With endpoint routing, you're a bit hosed here. Should we consider passing more data as the resource here:

https://github.com/aspnet/AspNetCore/blob/master/src/Security/Authorization/Policy/src/AuthorizationMiddleware.cs#L67

@davidfowl
Copy link
Member

@rynowak \ @JamesNK \ @davidfowl what do you guys think about this? You used to be able to get to HttpContext by downcasting https://github.com/aspnet/AspNetCore/blob/master/src/Security/Authorization/Core/src/AuthorizationHandlerContext.cs#L55 to the ActionContext when auth was executing as a filter. With endpoint routing, you're a bit hosed here. Should we consider passing more data as the resource here:

Yes this is a regression that we might want to fix, but fixing it will break new consumers it's kinda lose lose. If we had made exposed the HttpContext then the user could get the endpoint from it so we may have to lean on the accessor now.

@pranavkm
Copy link
Contributor

Yes this is a regression that we might want to fix, but fixing it will break new consumers

We could put it behind a compat flag. Another option might be a type that allows explicit casts to EndpointMetadata. I'll file an issue for this in AspNetCore and we can consider it there.

@serpent5 \ @Rick-Anderson for the moment, we can point users at using IHttpContextAccessor

@serpent5
Copy link
Contributor Author

Is the intent to show how to identify what MVC endpoint is meant to be executed?

I'm just looking to update the Accessing MVC request context in handlers section for 3.0. That lead me to look at the properties that were available on AuthorizationFilterContext. I understand it that ModelState and Result are completely unavailable now that authz happens before MVC picks up the endpoint. HttpContext is covered now thanks to your input above and that also provides access to RouteData. The last piece I see is providing ActionDescriptor, which is where the code in this PR comes in.

Do you happen to know what sort of metadata is it that users are attempting to read?

No, I don't. I'm not in a great position to know about that. I mentioned this approach because I saw it as a more direct way to get to the filter attributes as compared to going via the ActionDescriptor.

From the docs issues, I've seen requests for a couple of AuthorizationFilterContext capabilities:

  • Being able to set the Result. I believe this just isn't possible in an authz handler anymore.
  • Being able to get RouteData. I believe this can be achieved with HttpContext.GetRouteData() or HttpContext.GetRouteValue(...), but I imagine this isn't entirely equivalent.

How should we proceed with this? Which of these should the section in question cover?:

  • Using the approach in this PR's sample code for getting to ActionDescriptor et al.
  • Using the same approach for getting to specific attributes.
  • Injecting IHttpContextAccessor.

There's a page on using DI with authz handlers, so the IHttpContextAccessor part could just be a note with a link to that. Perhaps that page could even be updated to show an example of using it.

@Rick-Anderson
Copy link
Contributor

@pranavkm can you respond to the last question?

@Rick-Anderson

This comment has been minimized.

1 similar comment
@Rick-Anderson
Copy link
Contributor

@pranavkm or @anur can you respond to the last question?

@Rick-Anderson
Copy link
Contributor

@pranavkm can you respond to the last question?

@pranavkm
Copy link
Contributor

pranavkm commented Dec 3, 2019

Should there be a suggestion of injecting IHttpContextAccessor for non-MVC stuff like HttpRequest?

Is that the last question?

@Rick-Anderson
Copy link
Contributor

@serpent5 what do you need on this besides a review?

@serpent5
Copy link
Contributor Author

serpent5 commented Dec 4, 2019

Sorry for any confusion I've caused here.

It was this question (ok, it's two questions but really it's the same one 😅):

How should we proceed with this? Which of these should the section in question cover?:

  • Using the approach in this PR's sample code for getting to ActionDescriptor et al.
  • Using the same approach for getting to specific attributes.
  • Injecting IHttpContextAccessor.

There's a page on using DI with authz handlers, so the IHttpContextAccessor part could just be a note with a link to that. Perhaps that page could even be updated to show an example of using it.

After revisiting this today, I think it might be enough to just cover injecting IHttpContextAccessor, which might cover most use-cases. You guys are way better placed to know whether it's useful to also document the approach shown in the code sample this PR shows.

@Rick-Anderson Rick-Anderson removed the request for review from pranavkm December 12, 2019 18:40
@Rick-Anderson
Copy link
Contributor

@serpent5 can you update the questions embedded in the PR'd file?

@serpent5
Copy link
Contributor Author

@serpent5 can you update the questions embedded in the PR'd file?

Sure. I’ll do this tomorrow evening.

@scottaddie
Copy link
Member

Closing, as this was addressed in #17829.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Security-PU Must be reviewed by product unit security.
Projects
None yet
7 participants