jasontaylordev / CleanArchitecture

Clean Architecture Solution Template for ASP.NET Core
MIT License
17k stars 3.64k forks source link

CurrentUserService Always Returns Null UserId #119

Closed clockwiseq closed 4 years ago

clockwiseq commented 4 years ago

Describe the bug After logging in, the CurrentUserService always returns a null UserId value.

To Reproduce Steps to reproduce the behavior:

  1. Clone or download repo
  2. Unzip
  3. Open solution in VS2019
  4. Start debugging
  5. Login as administrator@localhost (seeded user account)
  6. Put a breakpoint on CommonUserService line 12
  7. Refresh a page, voila, null.

Expected behavior UserId should have the Id of the current logged in user.

Screenshots None

Additional context I tried injecting the service into a controller with the same result. Although, if I just hit the HttpContext directly on an action of the HomeController, I can get to the UserId:

public IActionResult Privacy()
 {
     // Value is NULL
     var userId = _currentUserService.UserId;

     // Value is NOT NULL
     var uid = HttpContext.User.FindFirstValue(ClaimTypes.NameIdentifier);

     return View();
 }
bennetritters commented 4 years ago

Hello @clockwiseq ,

I have had the same issue in the past, and a little digging got me to realize that in order for the HttpContext to be able to recover the Users via claims the Controller needs the AuthorizeAttribute.

Simply add [Authorize] ontop on the Controller or Function.

shareonline commented 4 years ago

But isn't ICurrentUserService used by AuditableEntity? I'm using AzureB2C authentication, and AuditableEntity will not get updated with anything when it is always null. It is only populated when used in the WebUI project?

flensrocker commented 4 years ago

I use a different approach for ICurrentUserService, because most of my library code is not only used in ASP.Net Core but also in WPF and Xamarin.

public interface ICurrentUserService
{
  string UserId { get; set; }
}

Yes, I have a setter on the current userid...

A default implementation:

public class CurrentUserService
{
  public string UserId { get; set; }
}

A useful extension:

public static void SetCurrentUser(this IServiceProvider serviceProvider, ClaimsPrincipal user)
{
  var currentUserService = serviceProvider.GetRequiredService<ICurrentUserService>();
  if ((user is null) || !user.Identity.IsAuthenticated)
    currentUserService.UserId = null;
  else
    currentUserService.UserId = user.FindFirstValue(ClaimTypes.NameIdentifier);
}

In my ASP projects I use a middleware:

public class CurrentUserMiddleware
{
  private readonly RequestDelegate _next;

  public CurrentUserMiddleware(RequestDelegate next)
  {
    _next = next;
  }

  public async Task InvokeAsync(HttpContext httpContext)
  {
    httpContext.RequestServices.SetCurrentUser(httpContext.User);
    await _next(httpContext);
  }
}

In my other (WPF) projects I use another implementation which sets the current user derived from the Windows user or whatever I need.

In my login-code I can set the current user after successful authentication.

UnitTests: In each scope I create in a test, I can use the extension to set the needed userId for this test.

shareonline commented 4 years ago

I use a different approach for ICurrentUserService, because most of my library code is not only used in ASP.Net Core but also in WPF and Xamarin.

public interface ICurrentUserService
{
  string UserId { get; set; }
}

Yes, I have a setter on the current userid...

A default implementation:

public class CurrentUserService
{
  public string UserId { get; set; }
}

A useful extension:

public static void SetCurrentUser(this IServiceProvider serviceProvider, ClaimsPrincipal user)
{
  var currentUserService = serviceProvider.GetRequiredService<ICurrentUserService>();
  if ((user is null) || !user.Identity.IsAuthenticated)
    currentUserService.UserId = null;
  else
    currentUserService.UserId = user.FindFirstValue(ClaimTypes.NameIdentifier);
}

In my ASP projects I use a middleware:

public class CurrentUserMiddleware
{
  private readonly RequestDelegate _next;

  public CurrentUserMiddleware(RequestDelegate next)
  {
    _next = next;
  }

  public async Task InvokeAsync(HttpContext httpContext)
  {
    httpContext.RequestServices.SetCurrentUser(httpContext.User);
    await _next(httpContext);
  }
}

In my other (WPF) projects I use another implementation which sets the current user derived from the Windows user or whatever I need.

In my login-code I can set the current user after successful authentication.

UnitTests: In each scope I create in a test, I can use the extension to set the needed userId for this test.

Nice and simple approach! thank you :)

flensrocker commented 4 years ago

@clockwiseq As mentioned in https://github.com/jasontaylordev/CleanArchitecture/issues/132#issuecomment-631357951 there could be a problem with the request pipeline where the authorization middleware, which populates the HttpContext.User from a JWT or cookie runs after the ICurrentUserService is instantiated. I can't confirm it with a pristine generated template, but you can try the lazy evaluation of the userId in your example.

xontab commented 4 years ago

I agree with @flensrocker. Created a PR (https://github.com/jasontaylordev/CleanArchitecture/pull/168) to use the accessor directly as it is safer that way. Since HttpContext is using AsyncLocal and the CurrentUserService is using DI, they both use a different code path. Thus there might be synchronisation issues where the wrong HttpContext is cached when CurrentUserService is initialised. Thus it is advisable to always read directly from the HttpContext regardless of the lifecycle of the CurrentUserService. -https://github.com/davidfowl/AspNetCoreDiagnosticScenarios/blob/master/AspNetCoreGuidance.md#do-not-store-ihttpcontextaccessorhttpcontext-in-a-field

However not sure if this is related to the particular reported issue.

MHarrison22 commented 4 years ago

Argh, this was driving me nuts! @xontab's PR resolved the issue for me, thank you!

yannickrondeau commented 2 years ago

@flensrocker Are you still using the middleware as a way to retrieve the current user for Asp application? We've been struggling with the CurrentUserService lately, mainly because some of our request are added to a queue and processed later. Therefore, the HttpContext is empty and the CurrentUserService.UserId returns a null value, which cause other issues.

We've tried to figure out a better way of handling it, and the example you gave is promising.

Note : You folks, let me know if I should open a new ticket regarding that or not.

flensrocker commented 2 years ago

@YannickRondeau yes, I use a similar kind of middleware like my example. But I don't queue requests. If you do that, you may have to store the userId (or whatever you need) with the queued request. Whenever you leave the request/response pipeline the HttpContext isn't valid anymore.

yannickrondeau commented 2 years ago

@flensrocker That make sens. We're currently trying to find a solution to that.. The issue with the whole current clean architecture is that usage of the HttpContext within the CurrentUserService which is used in the Infrastructure layer. We're using it with Hangfire which isn't working very well because of that.

I've tried searching and looking at Hangfire Filter, using Job Activator, etc. But nothing's working as of now. If you have any idea of how this could be done, I'll take it!

flensrocker commented 2 years ago

@YannickRondeau I'm sorry, but I don't know Hangfire.