Change the ICurrentUserService to Scoped

[IliyanAng]

ICurrentUser was changed to Singleton by PR #168

But the current user service shouldn't share a single instance in the whole application. With the current behavior being singleton, the UserId is not always guaranteed to be the one of the current user.

As an example, inject ICurrentUser in a method that has heavy computation. You get the UserId at the start of the method and start the execution of the heavy task. In the same time , the web server is accessed by another user and the CurrentUserId is then changed to the second user's ID.

We go back to the long running task for User 1. By the end of it, we check again the currentUserService UserId. This time the Id is the one of the User 2, so we get inconsistent behavior.

---

The integration tests at the moment are not accounting for this as in them we set the CurrentUserService in them as Transient

CustomWebApplicationFactory.cs

`builder.ConfigureServices((builder, services) =>
{

        services
            .Remove<ICurrentUserService>()
            .AddTransient(provider => Mock.Of<ICurrentUserService>(s =>
                s.UserId == GetCurrentUserId()));
    });`

Also the linked David Fowler article in #168 doesn't mention anywhere that we should use a Singleton for this case.

https://github.com/davidfowl/AspNetCoreDiagnosticScenarios/blob/master/AspNetCoreGuidance.md#do-not-store-ihttpcontextaccessorhttpcontext-in-a-field

[thomaslem]

@IliyanAng Did you test your example? Because _httpContextAccessor.HttpContext gets the current context. Which is already scoped to the request. And from my tests I can not reproduce your example.

[IliyanAng]

Hey @thomaslem , sorry for the late reply.
I did more extensive testing today and confirm you are right. I was shoehorning some of my previous tests.

But imo the service should still stay scoped. You basically want and expect scoped behavior for the CurrentUser service and thus far it is working only because the UserId is depended httpcontext.

public string? UserId => _httpContextAccessor.HttpContext?.User?.FindFirstValue(ClaimTypes.NameIdentifier);

Add any other dependency and you're no longer tethered to the scoped request.
(like any data from a db. It will be possible to share state and the service might not be cleanly recreated between requests, as one would expect. Which will cause bugs and leaks)

Thank you for bringing it up!

(ngl due to tunnel vision I wasted the whole Sunday afternoon trying to prove you wrong 🥲 )