Add missing TimeProvider registration

[Prologh]

Add missing TimeProvider registration

• [x] You've read the Contributor Guide and Code of Conduct.
• [x] You've included unit or integration tests for your change, where applicable.
• [x] You've included inline docs for your change, where applicable.
• [x] There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Summary of the changes (Less than 80 chars)

Add missing TimeProvider registration

AddSignInManager() method on IdentityBuilder adds dependencies that use TimeProvider but does not register an implementation for it within service collection. That results in InvalidOperationException being thrown on application startup for invalid service descriptor, because of missing TimeProvider implementation.

Fixes #53938

I haven't been able to locate unit tests that would cover specifically that identity builder methods. Any guidance for that would be welcomed if there is a need to add test coverage for the change.

[martincostello]

I haven't been able to locate unit tests that would cover specifically that identity builder methods. Any guidance for that would be welcomed if there is a need to add test coverage for the change.

Looks like the existing tests live in the IdentityBuilderTest class, like this one:

https://github.com/dotnet/aspnetcore/blob/c52a7abf3a8a08213dea461c7d97f421b9d862e6/src/Identity/test/Identity.Test/IdentityBuilderTest.cs#L175-L197

[Prologh]

Thanks for pointing me in the right direction. I've added a new test method since EnsureDefaultServices() uses AddIdentity() which inside calls AddAuthentication() which in turn actually registers TimeProvider (see here). So, to test it out I have to use AddIdentityCore() which skips the additional services, but nevertheless should end up with resolvable dependencies. Let me know if it is too much.

Also, on a side note, I've noticed in the test class a rather often usage of Assert.NotNull() with a result of GetRequiredService() from service provider. This could be considered a code smell, since GetRequiredService() will throw when service in question is not found. Obviously this is out of scope of this PR or related issue. Just nitpicking :smirk:

[Prologh]

/azp run

[azure-pipelines[bot]]

Commenter does not have sufficient privileges for PR 53946 in repo dotnet/aspnetcore

[Prologh]

@martincostello Anything more I should do?

[martincostello]

If you close and re-open the PR, that will make the CI re-run.

[Prologh]

Sure. I made TimeProvider optional in both versions of PostConfigureSecurityStampValidatorOptions. I've also removed the assert for TimeProvider from test case.

[halter73]

/azp run

[azure-pipelines[bot]]

Azure Pipelines successfully started running 3 pipeline(s).