DefaultEventService requires HttpContext and throws NRE when sending events from session cleanup

[gingters]

Which version of Duende IdentityServer are you using?
6.1.2

Which version of .NET are you using?

.NET 6.0

Describe the bug

An audit requirement needs an audit trail entry when a user signs in or out, and also when the user's session expired.
For that, we implemented IEventSink to write audit trails based on the Identity Server events system.

We then overrode the DefaultSessionCoordinationService and in ProcessExpirationAsync we created and raised a new UserSessionExpiredEvent, similar to the already existing UserLoginSuccessEvent and UserLogoutSuccessEvent.
This will allow to track if a user logged out manually or if the session was automatically terminated.

The session expiry is checked in an IHostedService (ServerSideSessionCleanupHost) and since that does not run within an http request, is no HttpContext available. However, the DefaultEventService requires an HttpContext and throws a NullReferenceException when the HttpContextAccessor does not provide a HttpContext instance (DefaultEventService, line 115 and following).

To Reproduce
Create a new class for a UserSessionExpiredEvent. Make the .ctor accept a UserSession object and have the event carry the subject Id as a payload.
When configuring IdentityServer, set options.Events.Raise[Category]Events to true for the specified category of that new event (i.e. Information).
Override the DefaultSessionCoordinationService. Inject an additional IEventService in the .ctor and store it in an _eventService field.
Override ProcessExpirationAsync, call the base implementation and then call await _eventService.RaiseAsync(new SessionExpiredEvent(session));

Ideally, set the session expiration to a short amount of time. Create a new session, and have the cleanup remove the session. Raising the event will throw.

Expected behavior

It should be possible to raise an Event using the DefaultEventService from an IHostedService like the ServerSideSessionCleanupHost, without an exception.

[josephdecock]

Hi, thanks for this bug report. We've been fixing a related issue for our upcoming 6.3 release, and we're going to fix this for 6.3 as well.

[kplyasunov]

@josephdecock Hi, I think additional fix may be required here, there still seems to exist an unguarded instance of calling HttpContext in the DefaultEventService.cs (line 114):

Could you please re-check it?

[josephdecock]

When there is no HttpContext available, what value should use as the TraceIdentifier? Initial thought was null, but I see our other similar checks are setting the value to the string "unknown". @brockallen and @AndersAbel, I think you guys have previously thought about this more than me ... care to weigh in?

[brockallen]

If we don't have one, then we don't have one... so "none" or "unknown" is fine w/ me.

[josephdecock]

@kplyasunov I just created a PR for a fix for this issue (see above) which would go out as a patch release of our latest release (probably 7.0.5). Does that work for you? If you're not already using or in a position to upgrade to the 7.0.x release, we could consider a backport as well.

[kplyasunov]

@josephdecock Thanks! For now we have found a way to workaround the issue, so it is not urgent, but it would be nice to have this fixed in a future release.

[RolandGuijt]

Looks like this is resolved. The fix will be in a release soon. Closing this issue for now. But if there's anything else, free free to reopen.