Shazwazza / UmbracoIdentity

ASP.NET Identity implementation for Umbraco's native member data
MIT License
111 stars 58 forks source link

AAD authorize attribute behaviour #130

Open aaron-macdonald-acara opened 3 years ago

aaron-macdonald-acara commented 3 years ago

Hello

I'm using Umbraco 8.9.1 and have just installed this package and run through the installation instructions for authenticating members with AAD:

https://shazwazza.com/post/configuring-azure-active-directory-login-with-umbraco-members/

Logging in via AAD using the account controller is working (although I had to reference the 'upn' claim to get an email address as the default code did not work).

I note that the [Authorize] attribute results in erroneous behavior when trying to authenticate so I assume it is not intended to be used.

If I use [MemberAuthorize] I get a 403 when not logged in and there's no authentication redirect. I'm assuming this authentication redirect should occur.

I was able to get the authentication redirect to occur by overriding the MemberAuthorize attribute and setting a challenge result when handling the unauthorized request (note the ChallengeResult class had be moved out of the template account controller and made public).

Override class is below, I'm sure it can be improved.


namespace Umbraco.Web.Mvc
{
    /// <summary>
    /// Attribute for attributing controller actions to restrict them
    /// to just authenticated members, and optionally of a particular type and/or group
    /// </summary>
    public sealed class MemberAuthorizeRedirectingAttribute : AuthorizeAttribute
    {
        /// <summary>
        /// Comma delimited list of allowed member types
        /// </summary>
        public string AllowType { get; set; }

        /// <summary>
        /// Comma delimited list of allowed member groups
        /// </summary>
        public string AllowGroup { get; set; }

        /// <summary>
        /// Comma delimited list of allowed members
        /// </summary>
        public string AllowMembers { get; set; }

        protected override bool AuthorizeCore(HttpContextBase httpContext)
        {
            if (AllowMembers.IsNullOrWhiteSpace())
                AllowMembers = "";
            if (AllowGroup.IsNullOrWhiteSpace())
                AllowGroup = "";
            if (AllowType.IsNullOrWhiteSpace())
                AllowType = "";

            var members = new List<int>();
            foreach (var s in AllowMembers.Split(','))
            {
                if (int.TryParse(s, out var id))
                {
                    members.Add(id);
                }
            }

            var helper = Current.Factory.GetInstance<MembershipHelper>();
            return helper.IsMemberAuthorized(AllowType.Split(','), AllowGroup.Split(','), members);

        }

        /// <summary>
        /// Override method to throw exception instead of returning a 401 result
        /// </summary>
        /// <param name="filterContext"></param>
        protected override void HandleUnauthorizedRequest(AuthorizationContext filterContext)
        {                                   
            filterContext.Result = new ChallengeResult(
                $"https://sts.windows.net/{ConfigurationManager.AppSettings["azureAd:tenantId"]}/",
                (filterContext.Controller as Controller).Url.SurfaceAction<UmbracoIdentityAccountController>("ExternalLoginCallback", 
                new { ReturnUrl = filterContext.HttpContext.Request.RawUrl }));

            //throw new HttpException(403, "Resource restricted: either member is not logged on or is not of a permitted type or group.");
        }
    }
`
Shazwazza commented 3 years ago

Hi, some replies below:

I note that the [Authorize] attribute results in erroneous behavior when trying to authenticate so I assume it is not intended to be used.

Can you elaborate. All the MVC AuthorizeAttribute does in .net is ensure the current principle is authenticated (i.e. HttpContext.User.Identity.IsAuthenticated which will be true if your member is logged in) and if not returns a HttpUnauthorizedResult, see source code here https://github.com/aspnet/AspNetWebStack/blob/main/src/System.Web.Mvc/AuthorizeAttribute.cs#L124

For WebApi this is similar, the source for that is here: https://github.com/aspnet/AspNetWebStack/blob/main/src/System.Web.Http/AuthorizeAttribute.cs

MemberAuthorize is part of the Umbraco source code and comes in 2x flavors too, one for MVC and one for WebApi (be sure you are using the correct one).

both just inherit from aspnet framework's corresponding AuthorizeAttribute.

This library just uses aspnet's OWIN auth behavior. Be sure that you don't have FormsAuthentication still enabled in your web.config https://github.com/Shazwazza/UmbracoIdentity/wiki#config. The OWIN handlers installed have their own logic for handling specific responses like 401 responses which may then redirect if those OWIN options are specified. All redirection with Identity occurs from within these middlewares.