55

Using ASP.NET MVC I am creating a custom Authorize attribute to take care of some custom authorization logic. I have looked at a lot of examples and it is pretty straight forward but my question is which method is best to override, AuthorizeCore or OnAuthorization? I have seen many examples overriding one or the other. Is there a difference?

Nick Olsen
  • 6,299
  • 11
  • 53
  • 75
  • 1
    Checkout the source code in MVC4, http://aspnetwebstack.codeplex.com/SourceControl/changeset/view/e0115a823029#src%2fSystem.Web.Mvc%2fAuthorizeAttribute.cs – user1736525 Oct 14 '12 at 01:57

2 Answers2

79

The clue is in the return types:

AuthorizeCore returns a boolean - it is decision making code. This should be limited to looking at the user's identity and testing which roles they are in etc. etc. Basically it should answer the question:

Do I want this user to proceed?

It should not perform any additional activities "on the side".

OnAuthorize returns void - this is where you put any functionality that needs to occur at this point. e.g. Write to a log, store some data in session etc etc.

BonyT
  • 10,750
  • 5
  • 31
  • 52
  • 15
    Unfortunately AuthorizeCore does not contain AuthorizationContext that I need (for accessing RouteData and making decisions based on it), therefore the only approach I see is using OnAuthorize. – gw0 Sep 02 '11 at 11:21
  • 4
    Why on Earth isn't `AuthorizationContext` passed into `AuthorizeCore`? This seems to be a major flaw. – Jez Oct 10 '12 at 15:35
  • 4
    @Jaz - `AuthorizeCore` is called from two places, one is from `OnAuthorize`, the other is from `OnCacheAuthorization`. In the later case, there is no `AuthorizationContext`, and it must be thread-safe. – Erik Funkenbusch Oct 14 '12 at 03:53
  • 6
    @gw0 - You can access the route data through the `HttpContextBase` which is passed to `AuthorizeCore`: `((MvcHandler)httpContext.Handler).RequestContext.RouteData` – Vincent Sels Jan 14 '14 at 14:03
17

You should put any code that must run regardless of whether the user is being authorized for the first time, or if they are using a cached authorization in AuthorizeCore.

If you look at the source code, you can see that AuthorizeCore gets called by both OnAuthorize and OnCacheAuthorization. This allows the authorization to be cached but still allow certain actions and to make the actual decisions about the authorization.

If you need something from the AuthorizationContext then you can create a property to hold the information and then access that in the AuthorizeCore method.

Erik Funkenbusch
  • 92,674
  • 28
  • 195
  • 291
  • I wish I could highlight the last sentence of your answer. You really should bold it. Referring to @gw0's comment in the accepted answer, the thing that is really unfortunate is that the suggestion to use the wrong override for the wrong reason was voted up. – David Peden Feb 01 '13 at 04:08
  • 4
    The documentation for AuthorizeAttribute (MSDN) clearly states (under Thread Safety) _"Any instance members are not guaranteed to be thread safe."_ So I guess that holding information in a property is not an option. – bvgheluwe Aug 11 '13 at 13:53
  • @BartVG - I am not sure I follow. What does thread safety have to do with this discussion? All that text means is that you have to synchronize access to objects if it's going to be used in a multi-threaded manner. – Erik Funkenbusch Aug 12 '13 at 05:38
  • @Mystere Man : thread safety is something you mentionned in your comment on [this answer](http://stackoverflow.com/a/6860804/803336) – bvgheluwe Aug 12 '13 at 08:30
  • @BartVG - Yes, but that has nothing to do with the properties. Thread safety means that multiple threads may read and write to the instance at the same time, thus they need to be synchronized to prevent race conditions. – Erik Funkenbusch Aug 12 '13 at 08:58
  • @user3285954 - You wouldn't save the context. You would only save the piece of information you needed, and lock that. However, you can also add the information you need to the HttpContext in the filterContext, so you can access it there. – Erik Funkenbusch May 27 '14 at 16:16
  • MSDN warns specifically not to store state in an instance of the type *unless it applies to all requests*. Instead use the "items" property: https://msdn.microsoft.com/en-us/library/system.web.mvc.authorizeattribute(v=vs.118).aspx – Mark Sowul Sep 14 '15 at 20:37