Sustainsys / Saml2

Saml2 Authentication services for ASP.NET
Other
960 stars 602 forks source link

SignInCommand null Logger reference #731

Closed davvves closed 7 years ago

davvves commented 7 years ago

When running SignInCommand.Run, the calls to options.SPOptions.Logger do not check if Logger exists, leading to a null reference exception.

explunit commented 7 years ago

It looks like the Options create a "stub" logger if one was not provided: https://github.com/KentorIT/authservices/blob/v0.21.2/Kentor.AuthServices/Configuration/Options.cs#L53-L56

Can you tell more detail about how the logger becomes null in your setup?

AndersAbel commented 7 years ago

@explunit is right. The design is that there always should be a logger, at least the null one. The label changes above reflect that when looking at the code I found it quite counter-intuitive before I remembered the reason.

When using the Mvc or HttpModule packages the default is a null logger. When using the Owin package the default is to use the configured owin/katana logger. Since the defaults are different the automatic creation has been defferred to the Options and KentorAuthServicesAuthenticationOptions classes. I guess you have your own IOptions implementation? Then you need to add that check yourself.

davvves commented 7 years ago

@AndersAbel @explunit The setup is to create the SPOptions first, and then use the normal KentorAuthServicesAuthenticationOptions constructor as such:

var authServicesOptions = new KentorAuthServicesAuthenticationOptions(false) { SPOptions = spOptions };

As far as I can tell this constructor is not giving a logger to the SPOptions property.

AndersAbel commented 7 years ago

No, you're right. The creation of the logger is actually deferred to the KentorAuthServicesAuthenticationMiddleware() constructor because that's where the Owin dependency injection/resolution can be accessed to create an owin logger. But still it shouldn't be possible to get in a situation where the logger is null.

Can you please provide more details on when you hit the null reference exception? Can you provide the stack trace?

davvves commented 7 years ago

Yep, give me a few hours and I'll get back to that point. Thanks Anders.

davvves commented 7 years ago

Inside the CreateAuthServicesOptions call below: I'm creating an SPOptions instance first with nothing set for the Logger property. Then calling the constructor as such: var authServicesOptions = new KentorAuthServicesAuthenticationOptions(false) { SPOptions = spOptions }; Then calling the sign in command:

public ActionResult SignIn(<params>)
        {
            var result = CommandFactory.GetCommand(CommandFactory.SignInCommandName).Run(
                Request.ToHttpRequestData(),
                this.samlOptionsService.CreateAuthServicesOptions(<params>));

            if (result.HandledResult)
            {
                throw new NotSupportedException("The MVC controller doesn't support setting CommandResult.HandledResult.");
            }

            result.ApplyCookies(Response);
            return result.ToActionResult();
        }

Stacktrace (identifying info has been deleted):

at Kentor.AuthServices.WebSso.SignInCommand.Run(HttpRequestData request, IOptions options)
at <namespace>.AuthServicesController.SignIn(<params>) in <path>\Controllers\AuthServicesController.cs:line 44
at lambda_method(Closure , ControllerBase , Object[] )
at System.Web.Mvc.ActionMethodDispatcher.Execute(ControllerBase controller, Object[] parameters)
at System.Web.Mvc.ReflectedActionDescriptor.Execute(ControllerContext controllerContext, IDictionary`2 parameters)
at System.Web.Mvc.ControllerActionInvoker.InvokeActionMethod(ControllerContext controllerContext, ActionDescriptor actionDescriptor, IDictionary`2 parameters)
at System.Web.Mvc.Async.AsyncControllerActionInvoker.<BeginInvokeSynchronousActionMethod>b__39(IAsyncResult asyncResult, ActionInvocation innerInvokeState)
at System.Web.Mvc.Async.AsyncResultWrapper.WrappedAsyncResult`2.CallEndDelegate(IAsyncResult asyncResult)
at System.Web.Mvc.Async.AsyncResultWrapper.WrappedAsyncResultBase`1.End()
at System.Web.Mvc.Async.AsyncControllerActionInvoker.EndInvokeActionMethod(IAsyncResult asyncResult)
at System.Web.Mvc.Async.AsyncControllerActionInvoker.AsyncInvocationWithFilters.<InvokeActionMethodFilterAsynchronouslyRecursive>b__3d()
at System.Web.Mvc.Async.AsyncControllerActionInvoker.AsyncInvocationWithFilters.<>c_DisplayClass46.<InvokeActionMethodFilterAsynchronouslyRecursive>b_3f()
AndersAbel commented 7 years ago

Ok, now I see. You've done an own frontend instead of using the owin middleware, but you've used the owin options class. Then you're on your own and won't be saved by the automatic creation of a logger. You need to set it by yourself. Or if you're not using the owin middleware, switch to using the built in Options class instead of KentorAuthServicesAuthenticationOptions. Options will set a null logger for you.