AspNetIdentity icon indicating copy to clipboard operation
AspNetIdentity copied to clipboard

ASP.NET Identity CookieAuthentication middleware results in too many redirects for authorized users visiting login page with a ReturnUrl param

Open mansoor-omrani opened this issue 7 years ago • 14 comments

If you set an explicit LoginPath for CookieAuthenticationOptions, authenticated users who navigate a restricted area they are not authorized to access (e.g. because of not having the right role), will trap in a redirect loop.

Steps to produce the issue

  1. Create an ASP.NET MVC web application.
  2. Create an Area named 'Admin'.
  3. Add a controller named 'MainController'
  4. Decorate the controller with '[Authorize(Roles = "SomeRole")]'
  5. Customize CookieAuthentication in Start.Auth.cs by providing an explicit path for login page (CookieAuthenticationOptions.LoginPath property).
  6. Launch the application.
  7. Register.
  8. Login.
  9. Now, Navigate to "/admin/main".

[Authorize] filter in MainController sends the user to the login page (LoginPath) together with a ReturnUrl. There, CookieAuthentication middleware detects that current user is an authenticated user and there is also a ReturnUrl param in the Url, so, it redirects the user back to the ReturnUrl.

The surprising thing is that the Login() action in AccountController -AccountController.Login(string returnUrl) - is also executed each time the user is redirected to login page!

This issue does not happen if you do not set an explicit LoginPath for CookieAuthentication middleware. In this case, when an authenticated user is redirected from a restricted area to login page (with a ReturnUrl param), the View in login page is executed normally and returned. So, we can check Request.IsAuthenticated in the View and instead of the login form show a message such as "You are currently logged in as ...., click here to sign in as another user".

This is the same thing I wanted to do in my own project, but since I had set a LoginPath, I was facing the "too many redirects" error. It took me a half day until I found out the culprit is CookieAuthenticationOptions.LoginPath!

mansoor-omrani avatar Jan 13 '19 13:01 mansoor-omrani

Yes. This is why you must have [AllowAnonymous] on your login assets.

blowdart avatar Jan 14 '19 18:01 blowdart

Thank you for your reply.

No. It's not because of missing [AllowAnonymous] (when you create an MVC application, the Login() action in AccountController is already decorated with [AllowAnonymous]).

I ask you to test the case as I described to see it in action.

mansoor-omrani avatar Jan 14 '19 18:01 mansoor-omrani

Can you provide a code repo, rather than instructions? Unfortunately your instructions don't give version numbers, for MVC, or for VS, nor indicate templates used if any, or your patch level. So I have no way of knowing if I'm even close to your setup if I just go off your instructions.

blowdart avatar Jan 14 '19 18:01 blowdart

Sorry for not describing my setup or version numbers.

Yes. I can provide my code repo. But the code consists of multiple files. How can I write multiple code files here?

Some parts of the code are automatically generated by Visual Studio when you create an MVC project.

I respectfully recommend to follow the steps I described. They are straightforward.

The test I did was using VS 2017 community which I downloaded a month ago.

Create an MVC application by picking "ASP.NET Web Application" project template (not ASP.NET Core Web Application) in Visual Studio 2015/2017 and then choose MVC.

My feeling is that the issue applies to all MVC versions (not sure about MVC core, I didn't test it).

mansoor-omrani avatar Jan 14 '19 18:01 mansoor-omrani

You'd put it in a public github repo and link to it. You talk about cookie auth, but the default MVC template doesn't have auth in it, which is why I want to see your project, and why following the instructions doesn't get to where I think you want me to be.

blowdart avatar Jan 14 '19 18:01 blowdart

I got it. I create a repo for that in a moment.

mansoor-omrani avatar Jan 14 '19 18:01 mansoor-omrani

Hi back.

I created a test project and put it here

I need to say that I had set validateInterval parameter in CookieAuthenticationProvider.OnValidateIdentity to zero because I noticed 'Remember Me' feature does not work and I had heard that you need to set validateInterval to zero so that 'Remember Me' works.

That time I couldn't get why validateInterval would have a thing with 'Remember Me'. validateInterval is used to detect change password or 3rd party logins. But I ignored the reason since i had to make my project working no matter what the real technical reason is.

Now, when I created this MVC repo to show the issue here I noticed that the default value VS sets for validateInterval is 30. Any none-zero number for validateInterval resolves the issue and redirect loop does not happen.

At first I thought that this is my fault and I should not set validateInterval to zero. But again, I noticed when you don't specify an explicit LoginPath for your CookieAuthenticationOptions the redirect loop issue still occurs even if you set validateInterval to zero!

mansoor-omrani avatar Jan 14 '19 20:01 mansoor-omrani

Oh that's not what that option does. OnValidateIdentity is a setting where it goes back to the database and refreshes the identity, if you're setting that to zero it's doing it on every request and logging you out to log you back in again, and then, during that redirect, it's going to do it over and over and over again.

If you don't set a loginpath, that's more interesting, it may be the default is ending up on a page that isn't allow anonymous, probably because of the area. @HaoK can you take a look?

blowdart avatar Jan 14 '19 20:01 blowdart

I'm guessing the code in the cookie handler hasn't changed too much since Katana days (@Tratcher can confirm), but LoginPath appears to only be used in two places:

https://github.com/aspnet/AspNetKatana/blob/dev/src/Microsoft.Owin.Security.Cookies/CookieAuthenticationHandler.cs#L287

Its supposed to be set to the login page for your app, because it will refuse to do a redirect if the request isn't on the login page itself. That might explain why not setting it would cause issues.

The other place its used is on handling challenges to build the redirect to login page url:

https://github.com/aspnet/AspNetKatana/blob/dev/src/Microsoft.Owin.Security.Cookies/CookieAuthenticationHandler.cs#L348

HaoK avatar Jan 14 '19 21:01 HaoK

I think setting validateInterval to zero for OnValidateIdentity while being awkward should not make this issue. I will change it definitely in my personal project, but my project was working fine even with a zero validateInterval.

In the current scenario that I posted here the user navigates to /admin/main. He doesn't have 'Admin' role, so [Authorize] filter redirects him to /account/login. I think it doesn't matter how frequent his cookie is checked. /account/login is a new simple request. Why would it trap into a redirect loop?

mansoor-omrani avatar Jan 14 '19 21:01 mansoor-omrani

So there's a difference between not authenticated and not authorized, once they have logged in, you need to send them to a "You are not authorized to view this page" as opposed to the login page. From what I recall (its been ages), I think its a matter of returning 401 (Need login), vs 403 (Access denied)

401 when you already are logged in will result in this infinite login behavior (since you already are logged in but never will become an admin just by logging in over and over again)

HaoK avatar Jan 14 '19 21:01 HaoK

That's correct. But I'm not sending unauthorized users to login page. It is the [Authorize] filter.

mansoor-omrani avatar Jan 14 '19 21:01 mansoor-omrani

... 401 when you already are logged in will result in this infinite login behavior (since you already are logged in but never will become an admin just by logging in over and over again)

Even if you are logged in and navigate to /admin/main in the repo I provided with a none-zeo onvalidateInterval, the infinite redirect loop does not happen.

mansoor-omrani avatar Jan 14 '19 21:01 mansoor-omrani

The validation interval should just be delaying when the loop happens

HaoK avatar Jan 15 '19 02:01 HaoK