0

I extended AuthorizeAttribute with my own class.

I specifically override the OnAuthorization method:

  • I create a ClaimsPrincipal (if user is recognized) and ...
    • assign the ClaimsPrincipal to filterContext.HttpContext.User
    • and if this.Users IsNullOrEmpty I set this.Users to the ClaimsPrincipal.Identity.Name
  • ...and I finish with a call to base.OnAuthorization

I use this AuthorizeAttribute extension by adding an instance to the GlobalFilterCollection: globalFilterCollection.Add(new AuthorizationAttributeExtension());

The app is started for the first time...

Request 1 comes in from User 1, User1 ClaimsPrincipal is created, filterContext.HttpContext.User is set to User1, and because this.Users IsNullOrEmpty, this.Users is set to User1.Identity.Name. Therefore base.AuthorizeCore will return true. User 1 gets in.

Request 2 comes in from User 2, User2 ClaimsPrincipal is created, filterContext.HttpContext.User is set to User2, and because this.Users is not NullOrEmpty (still has the value from User 1)!, this.Users remains User1.Identity.Name. Therefore base.AuthorizeCore will return false (User 1 <> User 2). User 2 cannot get in!

Questions:

Why does this.Users still have a value from a previous Request? Is that by-design? Why?

I thought this.Users might be empty for each new request? How does this.Users get cleared?

Maybe I should not check for IsNullOrEmpty, just set this.Users to "User1,User2", which would allow both Users... But if I get 100's of unique users...

Or maybe I shouldn't be setting this.Users at all; after all this.Users seems like it's about authorizing only specific users; I don't need only specific users, I need any authenticated user to get in ; maybe I should leave this.Users empty!

Community
  • 1
  • 1
Nate Anderson
  • 18,334
  • 18
  • 100
  • 135
  • You are completely misusing the `Users` property. It is supposed to be set statically in advance while you seem to alter its value. Why would you even need to do that? Setting the principal is enough to let or deny users. – Wiktor Zychla Sep 24 '16 at 21:48
  • So @WiktorZychla , you agree with this line of thinking: "Or maybe I shouldn't be setting this.Users at all; after all this.Users seems like it's about authorizing only specific users; I don't need only specific users, I need any authenticated user to get in ; maybe I should leave this.Users empty!"? If you can provide an answer with that rationale, and no contradictory answers come in, I can accept that as correct. – Nate Anderson Sep 24 '16 at 22:27
  • @WiktorZychla , and even though I am using it incorrectly here I wonder what is the lifecycle of an AuthorizeAttribute instance (and therefore its `Users` property), and is that lifecycle any different among these techniques to use an AuthorizeAttribute: 1) Adding to GlobalFilterCollection 2) Decorating 2a) Controllers and/or 2b) Actions ? – Nate Anderson Sep 24 '16 at 22:36
  • Because you don't control the lifetime of a filter attribute, you can't rely on setting its attributes between calls. Therefore, it doesn't really matter how and when it is created. And yes, you should never set the attribute and then rely on it. – Wiktor Zychla Sep 25 '16 at 12:23
  • I want to learn more about the lifespan of a filter attribute. I understand I should not. Now I am curious about what I could do. But I guess after searching [for Attribute lifespan information](http://stackoverflow.com/a/8937793/1175496), I find more advice which suggest "don't do it", like [in this guidance](http://www.asp.net/whitepapers/mvc3-release-notes#RTM-BC) "Therefore, any custom action filters which improperly store instance state might be broken." – Nate Anderson Sep 25 '16 at 14:29

0 Answers0