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

Authenticated but unauthorised users are being redirected to login page instead of 403 page. #6681

Open
DamienLaw opened this issue Mar 30, 2023 · 1 comment
Milestone

Comments

@DamienLaw
Copy link

DamienLaw commented Mar 30, 2023

The behaviour of AbpAuthorizationFilter for users who have logged in but doesn't have the required permission(s) is to redirect them to the login page. My unauthorised users were so confused as to why they have to re-login over and over again thinking it's a bug with the system.

catch (AbpAuthorizationException ex)
{
Logger.Warn(ex.ToString(), ex);
await _eventBus.TriggerAsync(this, new AbpHandledExceptionData(ex));
if (ActionResultHelper.IsObjectResult(context.ActionDescriptor.GetMethodInfo().ReturnType))
{
context.Result = new ObjectResult(new AjaxResponse(_errorInfoBuilder.BuildForException(ex), true))
{
StatusCode = context.HttpContext.User.Identity.IsAuthenticated
? (int) System.Net.HttpStatusCode.Forbidden
: (int) System.Net.HttpStatusCode.Unauthorized
};
}
else
{
context.Result = new ChallengeResult();
}
}

This behaviour is different from the original ASP.NET Core that returns a 403 which I think is a more correct way.
https://github.com/dotnet/aspnetcore/blob/bec278eabea54f63da15e10e654bdfa4168a2479/src/Security/Authorization/Policy/src/PolicyEvaluator.cs#L94-L108

public virtual async Task<PolicyAuthorizationResult> AuthorizeAsync(AuthorizationPolicy policy, AuthenticateResult authenticationResult, HttpContext context, object? resource)
{
	ArgumentNullException.ThrowIfNull(policy);

	var result = await _authorization.AuthorizeAsync(context.User, resource, policy);
	if (result.Succeeded)
	{
		return PolicyAuthorizationResult.Success();
	}


	// If authentication was successful, return forbidden, otherwise challenge
	return (authenticationResult.Succeeded)
		? PolicyAuthorizationResult.Forbid(result.Failure)
		: PolicyAuthorizationResult.Challenge();
}

There have been several attempts by others to retain ASP.NET Core original's behaviour:
https://stackoverflow.com/questions/51027406/asp-net-boilerplate-net-core-2-0-abpauthorizationfilter-challengeresult-una
https://support.aspnetzero.com/QA/Questions/7382/Redirect-authenticated-user-without-permission-to-a-page-instead-of-login
https://support.aspnetzero.com/QA/Questions/4049#answer-447ec9e8-b664-4f5a-b75f-313a2a2236f3

I think it's better to prompt users with an error instead of silently redirecting them back to the login page which might lead them to think they have been logged out for some reasons.
Somewhere along the lines in AbpAuthorizationFilter.cs above, we could do:

if (ActionResultHelper.IsObjectResult(context.ActionDescriptor.GetMethodInfo().ReturnType))
{
    ...
}
else
{
    if (context.HttpContext.User.Identity?.IsAuthenticated ?? false)
    {
        context.Result = new StatusCodeResult((int)System.Net.HttpStatusCode.Forbidden);

        context.HttpContext.Items.Add("error", ex.Message); // store the error message so that it could be retrieved and displayed in Error 403 cshtml. Supposed to only work using 'app.UseStatusCodePagesWithReExecute("/Error/{0}");' instead of 'app.UseStatusCodePagesWithRedirects("/Error/{0}");'
    }
    else
    {
        context.Result = new ChallengeResult();
    }
}
@ismcagdas ismcagdas added this to the Backlog milestone Mar 31, 2023
@ismcagdas
Copy link
Member

@DamienLaw thank you for your suggestion, we will consider it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants