ASP.NET Core and ApplicationContext.User authentication #688

Closed
dazinator opened this Issue Jan 24, 2017 · 20 comments

Comments

Projects
None yet
3 participants
@dazinator
Contributor

dazinator commented Jan 24, 2017

First let me describe the setup!

I have an asp.net core project, with an MVC controller method - the method isn't important aside from the fact it uses authentication via an [Authorize] attribute:

        [Authorize]
        [HttpPost]
        public async Task<IActionResult> Foo([FromBody]SomeCriteria crit)
        {
          // 
        }

Cookie authentication is pretty common, and in asp.net core it looks like this in Startup.cs

            app.UseCookieAuthentication(new CookieAuthenticationOptions() { Events = new CustomCookieAuthenticationEvents() });

Notice I have set a custom events class - this is an optional thing you can implement if you want to intercept the standard cookie authentication lifetime events.

Here is mine:

 public class CustomCookieAuthenticationEvents : CookieAuthenticationEvents
    {

        private readonly Task _completedTask;

        public CustomCookieAuthenticationEvents()
        {
            _completedTask = Task.FromResult(true);

            this.OnSignedIn = LoginCslaUser;
            this.OnSigningOut = LogoutCslaUser;
            this.OnValidatePrincipal = Validate;
          
        }

        public Task LoginCslaUser(CookieSignedInContext context)
        {
            // sets the user for CSLA purposes.
            Csla.ApplicationContext.User = context.Principal;
            return _completedTask;
        }


        public Task LogoutCslaUser(CookieSigningOutContext context)
        {
            Csla.ApplicationContext.User = new UnauthenticatedPrincipal();
            return _completedTask;
        }

        public Task Validate(CookieValidatePrincipalContext context)
        {
            Csla.ApplicationContext.User = context.Principal;           
            return _completedTask;
        }
    }


Pretty sure I am handling the Validate() method incorrectly at the present, however that shouldn't matter for the purposes of this issue.

Now to the crux..

When the controller method is called, the [Authorise] attribute does it's job, and cookie authentication runs. The CustomCookieAuthenticationEvents class is invoked and sets ApplicationContext.User to a claims principle.

However as soon as the mvc controller method is actually entered, ApplicationContext.User returns a GenericPrinciple again.

I am guessing this is because CSLA is trying to detect HttpContext.Current which may not be working for asp.net core applications which rely on IHttpContextAccessor to get hold of HttpContext instance. I have not looked at the CSLA code, but I assume CSLA is then treating the application like a desktop application, and storing the principle on the current thread.

@dazinator

This comment has been minimized.

Show comment Hide comment
@dazinator

dazinator Jan 24, 2017

Contributor

I am wondering if there is any way / extensibility point I could use to make CSLA use my own backing store for ApplicationContext.User, rather than its own default logic

Contributor

dazinator commented Jan 24, 2017

I am wondering if there is any way / extensibility point I could use to make CSLA use my own backing store for ApplicationContext.User, rather than its own default logic

@dazinator

This comment has been minimized.

Show comment Hide comment
@dazinator

dazinator Jan 24, 2017

Contributor

Oh i think IContextManager is what I am after

Contributor

dazinator commented Jan 24, 2017

Oh i think IContextManager is what I am after

@dazinator

This comment has been minimized.

Show comment Hide comment
@dazinator

dazinator Jan 24, 2017

Contributor

So this is basically what I have got, the only tricky bit is working out what to do with SetUser() - as asp.net core want's a claims principle, but CSLA takes an IPrinciple.

    /// <summary>
    /// Application context manager that uses HttpContextAccessor whenr esolving HttpContext
    /// to store context values.
    /// </summary>
    public class HttpContextAccessorContextMananger : IContextManager
    {

        private const string _localContextName = "Csla.LocalContext";
        private const string _clientContextName = "Csla.ClientContext";
        private const string _globalContextName = "Csla.GlobalContext";

        private readonly IServiceProvider _serviceProvider;

        public HttpContextAccessorContextMananger(IServiceProvider serviceProvider)
        {
            _serviceProvider = serviceProvider;
        }


        protected virtual HttpContext GetHttpContext()
        {
            var httpContextAccessor = (IHttpContextAccessor)_serviceProvider.GetService(typeof(IHttpContextAccessor));
            if (httpContextAccessor != null)
            {
                return httpContextAccessor.HttpContext;
            }

            return null;
        }


        /// <summary>
        /// Gets a value indicating whether this
        /// context manager is valid for use in
        /// the current environment.
        /// </summary>
        public bool IsValid
        {
            get { return GetHttpContext() != null; }
        }

        /// <summary>
        /// Gets the current principal.
        /// </summary>
        public System.Security.Principal.IPrincipal GetUser()
        {
            return GetHttpContext()?.User;
        }

        /// <summary>
        /// Sets the current principal.
        /// </summary>
        /// <param name="principal">Principal object.</param>
        public void SetUser(System.Security.Principal.IPrincipal principal)
        {
            GetHttpContext().User = (ClaimsPrincipal)principal;
        }

        /// <summary>
        /// Gets the local context.
        /// </summary>
        public ContextDictionary GetLocalContext()
        {
            return (ContextDictionary)GetHttpContext().Items[_localContextName];
        }

        /// <summary>
        /// Sets the local context.
        /// </summary>
        /// <param name="localContext">Local context.</param>
        public void SetLocalContext(ContextDictionary localContext)
        {
            GetHttpContext().Items[_localContextName] = localContext;
        }

        /// <summary>
        /// Gets the client context.
        /// </summary>
        public ContextDictionary GetClientContext()
        {
            return (ContextDictionary)GetHttpContext().Items[_clientContextName];
        }

        /// <summary>
        /// Sets the client context.
        /// </summary>
        /// <param name="clientContext">Client context.</param>
        public void SetClientContext(ContextDictionary clientContext)
        {
            GetHttpContext().Items[_clientContextName] = clientContext;
        }

        /// <summary>
        /// Gets the global context.
        /// </summary>
        public ContextDictionary GetGlobalContext()
        {
            return (ContextDictionary)GetHttpContext().Items[_globalContextName];
        }

        /// <summary>
        /// Sets the global context.
        /// </summary>
        /// <param name="globalContext">Global context.</param>
        public void SetGlobalContext(ContextDictionary globalContext)
        {
            GetHttpContext().Items[_globalContextName] = globalContext;
        }
    }

and in startup.cs


            Csla.ApplicationContext.ContextManager = new HttpContextAccessorContextMananger(app.ApplicationServices);


Contributor

dazinator commented Jan 24, 2017

So this is basically what I have got, the only tricky bit is working out what to do with SetUser() - as asp.net core want's a claims principle, but CSLA takes an IPrinciple.

    /// <summary>
    /// Application context manager that uses HttpContextAccessor whenr esolving HttpContext
    /// to store context values.
    /// </summary>
    public class HttpContextAccessorContextMananger : IContextManager
    {

        private const string _localContextName = "Csla.LocalContext";
        private const string _clientContextName = "Csla.ClientContext";
        private const string _globalContextName = "Csla.GlobalContext";

        private readonly IServiceProvider _serviceProvider;

        public HttpContextAccessorContextMananger(IServiceProvider serviceProvider)
        {
            _serviceProvider = serviceProvider;
        }


        protected virtual HttpContext GetHttpContext()
        {
            var httpContextAccessor = (IHttpContextAccessor)_serviceProvider.GetService(typeof(IHttpContextAccessor));
            if (httpContextAccessor != null)
            {
                return httpContextAccessor.HttpContext;
            }

            return null;
        }


        /// <summary>
        /// Gets a value indicating whether this
        /// context manager is valid for use in
        /// the current environment.
        /// </summary>
        public bool IsValid
        {
            get { return GetHttpContext() != null; }
        }

        /// <summary>
        /// Gets the current principal.
        /// </summary>
        public System.Security.Principal.IPrincipal GetUser()
        {
            return GetHttpContext()?.User;
        }

        /// <summary>
        /// Sets the current principal.
        /// </summary>
        /// <param name="principal">Principal object.</param>
        public void SetUser(System.Security.Principal.IPrincipal principal)
        {
            GetHttpContext().User = (ClaimsPrincipal)principal;
        }

        /// <summary>
        /// Gets the local context.
        /// </summary>
        public ContextDictionary GetLocalContext()
        {
            return (ContextDictionary)GetHttpContext().Items[_localContextName];
        }

        /// <summary>
        /// Sets the local context.
        /// </summary>
        /// <param name="localContext">Local context.</param>
        public void SetLocalContext(ContextDictionary localContext)
        {
            GetHttpContext().Items[_localContextName] = localContext;
        }

        /// <summary>
        /// Gets the client context.
        /// </summary>
        public ContextDictionary GetClientContext()
        {
            return (ContextDictionary)GetHttpContext().Items[_clientContextName];
        }

        /// <summary>
        /// Sets the client context.
        /// </summary>
        /// <param name="clientContext">Client context.</param>
        public void SetClientContext(ContextDictionary clientContext)
        {
            GetHttpContext().Items[_clientContextName] = clientContext;
        }

        /// <summary>
        /// Gets the global context.
        /// </summary>
        public ContextDictionary GetGlobalContext()
        {
            return (ContextDictionary)GetHttpContext().Items[_globalContextName];
        }

        /// <summary>
        /// Sets the global context.
        /// </summary>
        /// <param name="globalContext">Global context.</param>
        public void SetGlobalContext(ContextDictionary globalContext)
        {
            GetHttpContext().Items[_globalContextName] = globalContext;
        }
    }

and in startup.cs


            Csla.ApplicationContext.ContextManager = new HttpContextAccessorContextMananger(app.ApplicationServices);


@dazinator dazinator referenced this issue in dazinator/PersonalBlog Jan 24, 2017

Closed

Blog about using CSLA in asp.net core #1

@jonnybee

This comment has been minimized.

Show comment Hide comment
@jonnybee

jonnybee Jan 24, 2017

Owner

ClaimsPrincipal implements IPrincipal so you may be able to cast this as appropriate.

See also http://andrewlock.net/introduction-to-authentication-with-asp-net-core/

Owner

jonnybee commented Jan 24, 2017

ClaimsPrincipal implements IPrincipal so you may be able to cast this as appropriate.

See also http://andrewlock.net/introduction-to-authentication-with-asp-net-core/

@dazinator

This comment has been minimized.

Show comment Hide comment
@dazinator

dazinator Jan 24, 2017

Contributor

Yeah - I am just blindly upcasting the IPrinciple to a ClaimsPrinciple at the moment, as I think it's safe to assume within my partciular application, that we will only use ClaimsPrinciple. If anyone was to implement / use a custom IPrinciple not derived from ClaimsPrinciple then they would have to tweak this:

GetHttpContext().User = (ClaimsPrincipal)principal;
Contributor

dazinator commented Jan 24, 2017

Yeah - I am just blindly upcasting the IPrinciple to a ClaimsPrinciple at the moment, as I think it's safe to assume within my partciular application, that we will only use ClaimsPrinciple. If anyone was to implement / use a custom IPrinciple not derived from ClaimsPrinciple then they would have to tweak this:

GetHttpContext().User = (ClaimsPrincipal)principal;
@dazinator

This comment has been minimized.

Show comment Hide comment
@dazinator

dazinator Jan 24, 2017

Contributor

I have a question..

Csla.ApplicationContext.ContextManager is static (app domain level).

In my asp.net core application, aside from standard MVC stuff, I also have some background hangfire jobs that run, and they run on a background thread, and HttpContext isn't available.

In this instance, CSLA is still making various calls to my Csla.ApplicationContext.ContextManager - and my implementation can't find a HttpContext and so it does the following:

  1. GetClientContext - it just returns null.
  2. SetClientContext - I can't do anything here as HttpContext is null.
  3. SetGlobalContext - I can't do anything here as HttpContext is null.

I am wondering If i need to handle HttpContext being null in my ContextManager implementation and fallback to using the current thread? Or should CSLA do that for me?

My ContextManager also implements IsValid but it never get's called - so I am wondering what the purpose of IsValid is..

 /// <summary>
        /// Gets a value indicating whether this
        /// context manager is valid for use in
        /// the current environment.
        /// </summary>
        public bool IsValid
        {
            get
            {
                return GetHttpContext() != null;
            }
        }
Contributor

dazinator commented Jan 24, 2017

I have a question..

Csla.ApplicationContext.ContextManager is static (app domain level).

In my asp.net core application, aside from standard MVC stuff, I also have some background hangfire jobs that run, and they run on a background thread, and HttpContext isn't available.

In this instance, CSLA is still making various calls to my Csla.ApplicationContext.ContextManager - and my implementation can't find a HttpContext and so it does the following:

  1. GetClientContext - it just returns null.
  2. SetClientContext - I can't do anything here as HttpContext is null.
  3. SetGlobalContext - I can't do anything here as HttpContext is null.

I am wondering If i need to handle HttpContext being null in my ContextManager implementation and fallback to using the current thread? Or should CSLA do that for me?

My ContextManager also implements IsValid but it never get's called - so I am wondering what the purpose of IsValid is..

 /// <summary>
        /// Gets a value indicating whether this
        /// context manager is valid for use in
        /// the current environment.
        /// </summary>
        public bool IsValid
        {
            get
            {
                return GetHttpContext() != null;
            }
        }
@dazinator

This comment has been minimized.

Show comment Hide comment
@dazinator

dazinator Jan 24, 2017

Contributor

Doh...

Spotted this:

#if !(ANDROID || IOS) && !NETFX_CORE

I should be setting ApplicationContext.WebContextManager not ContextManager.

Contributor

dazinator commented Jan 24, 2017

Doh...

Spotted this:

#if !(ANDROID || IOS) && !NETFX_CORE

I should be setting ApplicationContext.WebContextManager not ContextManager.

@dazinator dazinator closed this Jan 24, 2017

@dazinator dazinator reopened this Jan 24, 2017

@dazinator

This comment has been minimized.

Show comment Hide comment
@dazinator

dazinator Jan 24, 2017

Contributor

Leaving this open, in case CSLA wants to support asp.net core in future, then it will need some support package with a custom IContextManager like this. Unless @rockfordlhotka you see this living in a seperate repo?

Contributor

dazinator commented Jan 24, 2017

Leaving this open, in case CSLA wants to support asp.net core in future, then it will need some support package with a custom IContextManager like this. Unless @rockfordlhotka you see this living in a seperate repo?

@rockfordlhotka

This comment has been minimized.

Show comment Hide comment
@rockfordlhotka

rockfordlhotka Jan 24, 2017

Owner

I think it would be ideal to have this as part of the Csla.Web.Mvc assembly for ASP.NET Core, yes.

I plan to have such an assembly - mostly now just waiting for the churn around project.json vs csproj to settle with the release of VS2017. And perhaps wait for netstandard 2.0 as well - no sense putting in lots of work now just to redo it all in a couple months.

Owner

rockfordlhotka commented Jan 24, 2017

I think it would be ideal to have this as part of the Csla.Web.Mvc assembly for ASP.NET Core, yes.

I plan to have such an assembly - mostly now just waiting for the churn around project.json vs csproj to settle with the release of VS2017. And perhaps wait for netstandard 2.0 as well - no sense putting in lots of work now just to redo it all in a couple months.

@rockfordlhotka rockfordlhotka changed the title from [4.6.500] - Asp.net core and ApplicationContext.User authentication to ASP.NET Core and ApplicationContext.User authentication Jan 24, 2017

@dazinator

This comment has been minimized.

Show comment Hide comment
@dazinator

dazinator Jan 24, 2017

Contributor

Ok nice.

I ended up writing some relevant extension methods in the end to enable CSLA to be more intuitively setup within the context of an asp.net core application's startup class.

Usage from startup.cs:

        public override void ConfigureServices(IServiceCollection services)
        {     
            services.ConfigureCsla((options, sp) => {
                options.ObjectFactoryLoader = new ServiceProviderObjectFactoryLoader(sp);
                // could configure other CSLA hooks / options in future.
            });
        }

       public void Configure(IApplicationBuilder app, IHostingEnvironment env, ILoggerFactory loggerFactory)
        {
            // ommitted for brevity
             app.UseCsla();              
        }

So in ConfigureServices() some options can be registered that control what hooks CSLA will use.
Then in Configure() the UseCsla() method grabs those options, then set's up CSLA appropriately.

And the extension method itself:

  public static class CslaConfiguration
    {
        public static IServiceCollection ConfigureCsla(this IServiceCollection services, Action<CslaOptions, IServiceProvider> setupAction = null)
        {
            services.AddSingleton<CslaOptions>((sp) =>
            {
                var options = new CslaOptions();
                setupAction?.Invoke(options, sp);
                return options;
            });

            return services;
        }

        public static IApplicationBuilder UseCsla(this IApplicationBuilder appBuilder)
        {
            // grab the options
            var options = appBuilder.ApplicationServices.GetRequiredService<CslaOptions>();

            // configure csla according to those options.
            Csla.Server.FactoryDataPortal.FactoryLoader = options.ObjectFactoryLoader;
            Csla.ApplicationContext.WebContextManager = options.WebContextManager ?? new HttpContextAccessorContextMananger(appBuilder.ApplicationServices);
            
            return appBuilder;
        }

        public class CslaOptions
        {
            public IObjectFactoryLoader ObjectFactoryLoader { get; set; }

            public IContextManager WebContextManager { get; set; }           

        }
    }

The thing I like about this is that it centralises the configuration of CSLA as the consumer sees all relevant configuration options in on place (on the options class)..

The other thing I like is that the consumer is giving access to the IServiceProvider when configuring the csla options, so its easy for them to implement csla extensibility points that rely on an IServiceProvider for DI.

Contributor

dazinator commented Jan 24, 2017

Ok nice.

I ended up writing some relevant extension methods in the end to enable CSLA to be more intuitively setup within the context of an asp.net core application's startup class.

Usage from startup.cs:

        public override void ConfigureServices(IServiceCollection services)
        {     
            services.ConfigureCsla((options, sp) => {
                options.ObjectFactoryLoader = new ServiceProviderObjectFactoryLoader(sp);
                // could configure other CSLA hooks / options in future.
            });
        }

       public void Configure(IApplicationBuilder app, IHostingEnvironment env, ILoggerFactory loggerFactory)
        {
            // ommitted for brevity
             app.UseCsla();              
        }

So in ConfigureServices() some options can be registered that control what hooks CSLA will use.
Then in Configure() the UseCsla() method grabs those options, then set's up CSLA appropriately.

And the extension method itself:

  public static class CslaConfiguration
    {
        public static IServiceCollection ConfigureCsla(this IServiceCollection services, Action<CslaOptions, IServiceProvider> setupAction = null)
        {
            services.AddSingleton<CslaOptions>((sp) =>
            {
                var options = new CslaOptions();
                setupAction?.Invoke(options, sp);
                return options;
            });

            return services;
        }

        public static IApplicationBuilder UseCsla(this IApplicationBuilder appBuilder)
        {
            // grab the options
            var options = appBuilder.ApplicationServices.GetRequiredService<CslaOptions>();

            // configure csla according to those options.
            Csla.Server.FactoryDataPortal.FactoryLoader = options.ObjectFactoryLoader;
            Csla.ApplicationContext.WebContextManager = options.WebContextManager ?? new HttpContextAccessorContextMananger(appBuilder.ApplicationServices);
            
            return appBuilder;
        }

        public class CslaOptions
        {
            public IObjectFactoryLoader ObjectFactoryLoader { get; set; }

            public IContextManager WebContextManager { get; set; }           

        }
    }

The thing I like about this is that it centralises the configuration of CSLA as the consumer sees all relevant configuration options in on place (on the options class)..

The other thing I like is that the consumer is giving access to the IServiceProvider when configuring the csla options, so its easy for them to implement csla extensibility points that rely on an IServiceProvider for DI.

@rockfordlhotka

This comment has been minimized.

Show comment Hide comment
@rockfordlhotka

rockfordlhotka Jan 24, 2017

Owner

What you've done here ties into #525 I think.

Owner

rockfordlhotka commented Jan 24, 2017

What you've done here ties into #525 I think.

@dazinator

This comment has been minimized.

Show comment Hide comment
@dazinator

dazinator Jan 24, 2017

Contributor

Potentially!
I have chucked out a blog post covering this topic for the time being, in case anyone else stumbles accross this problem: http://darrelltunnell.net/blog/2017/01/24/csla-and-asp.net-core/

Contributor

dazinator commented Jan 24, 2017

Potentially!
I have chucked out a blog post covering this topic for the time being, in case anyone else stumbles accross this problem: http://darrelltunnell.net/blog/2017/01/24/csla-and-asp.net-core/

@rockfordlhotka

This comment has been minimized.

Show comment Hide comment
@rockfordlhotka

rockfordlhotka Sep 7, 2017

Owner

I'm changing the default (not web) context manager to use AsyncLocal (#729). I wonder if that would be the right solution for ASP.NET Core as well?

I suppose not really though, because the solution in @dazinator's blog post is probably still the best answer.

Owner

rockfordlhotka commented Sep 7, 2017

I'm changing the default (not web) context manager to use AsyncLocal (#729). I wonder if that would be the right solution for ASP.NET Core as well?

I suppose not really though, because the solution in @dazinator's blog post is probably still the best answer.

@dazinator

This comment has been minimized.

Show comment Hide comment
@dazinator

dazinator Sep 7, 2017

Contributor

If my memory serves, the default context manager should be irrelevent when running under asp.net core because the web context manager should override it (and the web context manager should be set on startup)

#if !(ANDROID || IOS) && !NETFX_CORE && !NETSTANDARD2_0

Just out of curiosity, why are there 2 context manager properties, why not a single Context Manager property that is set appropriately for the current platform on startup?

I noticed on that line that the web context manager check is compiled out of netstandard2.0 builds. Given that netstandard is implemented by a wide array of platforms (including platforms that run web applications like asp.net core) is it good / safe to assume that web is not appropriate? This problem of whether to include the web property or not would go away if you only had a single Context Manager property.

Contributor

dazinator commented Sep 7, 2017

If my memory serves, the default context manager should be irrelevent when running under asp.net core because the web context manager should override it (and the web context manager should be set on startup)

#if !(ANDROID || IOS) && !NETFX_CORE && !NETSTANDARD2_0

Just out of curiosity, why are there 2 context manager properties, why not a single Context Manager property that is set appropriately for the current platform on startup?

I noticed on that line that the web context manager check is compiled out of netstandard2.0 builds. Given that netstandard is implemented by a wide array of platforms (including platforms that run web applications like asp.net core) is it good / safe to assume that web is not appropriate? This problem of whether to include the web property or not would go away if you only had a single Context Manager property.

@rockfordlhotka

This comment has been minimized.

Show comment Hide comment
@rockfordlhotka

rockfordlhotka Sep 7, 2017

Owner

There's a lot of legacy in that code, and it could use some cleanup. I don't honestly remember why there are two, and it doesn't seem to make a lot of sense looking at it now...

You are right - what I did to the default context manager isn't relevant to the web. I'm just speculating as to whether what I did change would fix the web too, without having to worry about the stuff in your blog post.

The more I think about it though, the more I think you are on the right track. We don't know if AsyncLocal is safe within ASP.NET Core. If it was safe, then why would ASP.NET Core have the model they do?

Owner

rockfordlhotka commented Sep 7, 2017

There's a lot of legacy in that code, and it could use some cleanup. I don't honestly remember why there are two, and it doesn't seem to make a lot of sense looking at it now...

You are right - what I did to the default context manager isn't relevant to the web. I'm just speculating as to whether what I did change would fix the web too, without having to worry about the stuff in your blog post.

The more I think about it though, the more I think you are on the right track. We don't know if AsyncLocal is safe within ASP.NET Core. If it was safe, then why would ASP.NET Core have the model they do?

@rockfordlhotka rockfordlhotka added this to the 4.7.100 milestone Sep 7, 2017

@rockfordlhotka rockfordlhotka self-assigned this Sep 7, 2017

@dazinator

This comment has been minimized.

Show comment Hide comment
@dazinator

dazinator Sep 7, 2017

Contributor

We don't know if AsyncLocal is safe within ASP.NET Core. If it was safe, then why would ASP.NET Core have the model they do?

Well.. in asp.net core, we have to get the User / Principle from HttpContext, and the only way to get HttpContext is to be a middleware (i.e like owin middleware, but asp.net abandoned owin, so slightly different) or to use the IHttpContextAccessor which is registered as a singleton.

As IHttpContextAccessor is a singleton, I had a quick look to see how it manages thread safety, and low and behold it uses AsyncLocal:

https://github.com/aspnet/HttpAbstractions/blob/b3b846c27eee1364db416f35dec4556469509fb4/src/Microsoft.AspNetCore.Http/HttpContextAccessor.cs#L10

So in other words, as long as the IContextManager for asp.net core stores things in HttpContext (User, Items etc) - like my current implementation does, then as HttpContext is itself now accessed through / backed by an AsyncLocal - things should flow as desired!

Contributor

dazinator commented Sep 7, 2017

We don't know if AsyncLocal is safe within ASP.NET Core. If it was safe, then why would ASP.NET Core have the model they do?

Well.. in asp.net core, we have to get the User / Principle from HttpContext, and the only way to get HttpContext is to be a middleware (i.e like owin middleware, but asp.net abandoned owin, so slightly different) or to use the IHttpContextAccessor which is registered as a singleton.

As IHttpContextAccessor is a singleton, I had a quick look to see how it manages thread safety, and low and behold it uses AsyncLocal:

https://github.com/aspnet/HttpAbstractions/blob/b3b846c27eee1364db416f35dec4556469509fb4/src/Microsoft.AspNetCore.Http/HttpContextAccessor.cs#L10

So in other words, as long as the IContextManager for asp.net core stores things in HttpContext (User, Items etc) - like my current implementation does, then as HttpContext is itself now accessed through / backed by an AsyncLocal - things should flow as desired!

@rockfordlhotka

This comment has been minimized.

Show comment Hide comment
@rockfordlhotka

rockfordlhotka Sep 7, 2017

Owner

Excellent!

Owner

rockfordlhotka commented Sep 7, 2017

Excellent!

@dazinator

This comment has been minimized.

Show comment Hide comment
@dazinator

dazinator Sep 7, 2017

Contributor

Sorry - whilst I was looking through that file I saw something unrelated to this discussion but that looked odd - I wondered if this lock statement should have curly braces around both lines:

ContextManager.SetClientContext(clientContext);

UPDATE: Looking at that further.. I think you are right this file just needs a general tidy up :-)

Contributor

dazinator commented Sep 7, 2017

Sorry - whilst I was looking through that file I saw something unrelated to this discussion but that looked odd - I wondered if this lock statement should have curly braces around both lines:

ContextManager.SetClientContext(clientContext);

UPDATE: Looking at that further.. I think you are right this file just needs a general tidy up :-)

@rockfordlhotka

This comment has been minimized.

Show comment Hide comment
@rockfordlhotka

rockfordlhotka Sep 7, 2017

Owner

@dazinator I figured out/remembered why there might be two "active" context managers at one time (a web one and another).

In regular old ASP.NET it was possible for your code to sometimes have, and sometimes not have, access to HttpContext. I don't remember the scenario under which HttpContext.Current would return null, but it could happen.

As a result, each IContextManager has an IsValid property to indicate whether it is currently valid. And the ASP.NET one might return false, causing the code to fall back to the default manager.

I won't change that code, messy though it seems, because there's no point in causing possible regressions for regular ASP.NET 2.0-4.6 users.

However, I am going to add a new IContextManager implementation in the ASP.NET Core 2.0 assembly that works as you describe earlier in this thread. Along with the ConfigureCsla code.

Owner

rockfordlhotka commented Sep 7, 2017

@dazinator I figured out/remembered why there might be two "active" context managers at one time (a web one and another).

In regular old ASP.NET it was possible for your code to sometimes have, and sometimes not have, access to HttpContext. I don't remember the scenario under which HttpContext.Current would return null, but it could happen.

As a result, each IContextManager has an IsValid property to indicate whether it is currently valid. And the ASP.NET one might return false, causing the code to fall back to the default manager.

I won't change that code, messy though it seems, because there's no point in causing possible regressions for regular ASP.NET 2.0-4.6 users.

However, I am going to add a new IContextManager implementation in the ASP.NET Core 2.0 assembly that works as you describe earlier in this thread. Along with the ConfigureCsla code.

rockfordlhotka added a commit to rockfordlhotka/csla that referenced this issue Sep 7, 2017

rockfordlhotka added a commit to rockfordlhotka/csla that referenced this issue Sep 7, 2017

@rockfordlhotka

This comment has been minimized.

Show comment Hide comment
@rockfordlhotka

rockfordlhotka Sep 8, 2017

Owner

@dazinator I requested a review of my changes via the PR - I suspect you'll get an email to that effect.

If you have time, I'd really appreciate your thoughts on my changes (which are largely pulled from your blog post).

Owner

rockfordlhotka commented Sep 8, 2017

@dazinator I requested a review of my changes via the PR - I suspect you'll get an email to that effect.

If you have time, I'd really appreciate your thoughts on my changes (which are largely pulled from your blog post).

@rockfordlhotka rockfordlhotka referenced this issue in MarimerLLC/cslaforum Oct 11, 2017

Open

CSLA .NET 4.7 (coming soon) #442

@rockfordlhotka rockfordlhotka referenced this issue in MarimerLLC/cslaforum Mar 22, 2018

Open

CSLA .NET version 4.7.100 release #510

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