[identity] Add device sessionId in ExtendedUserLoginSuccessEvent and … - #1124
Conversation
…persist it in SignInLogs
There was a problem hiding this comment.
Pull request overview
This PR threads a per-login “device/session id” through the IdentityServer token issuance pipeline by storing it on the current HttpContext and then flowing it into both the issued token SessionId and the raised ExtendedUserLoginSuccessEvent. The integration tests are updated to capture IdentityServer events and assert that the raised success events contain the same SessionId as the token response.
Changes:
- Generate/store a session id in
HttpContext.Itemsduring successful password/device-grant validation and include it inExtendedUserLoginSuccessEvent. - Update
ExtendedTokenResponseGeneratorto prefer the request-scoped session id (fromHttpContext.Items) when settingValidatedRequest.SessionId. - Extend integration tests to capture raised IdentityServer events and assert the
SessionIdpropagation.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/Indice.Features.Identity.Tests/CustomGrantsIntegrationTests.cs | Captures raised IdentityServer events via a composite sink and asserts ExtendedUserLoginSuccessEvent.SessionId matches token SessionId. |
| src/Indice.Features.Identity.Core/ResponseHandling/ExtendedTokenResponseGenerator.cs | Sets ValidatedRequest.SessionId from request-scoped session id (or generates one). |
| src/Indice.Features.Identity.Core/Grants/ExtendedResourceOwnerPasswordValidator.cs | Generates a request-scoped session id on successful password validation and passes it into the success event. |
| src/Indice.Features.Identity.Core/Grants/DeviceAuthenticationExtensionGrantValidator.cs | Generates a request-scoped session id on successful device-grant validation and passes it into the success event. |
| src/Indice.Features.Identity.Core/Extensions/HttpContextExtensions.cs | Adds ResolveDeviceSessionId helper to read the request-scoped session id from HttpContext.Items. |
| src/Indice.Features.Identity.Core/Constants.cs | Introduces HttpContextItemKeys.DeviceSessionId constant used as the HttpContext.Items key. |
Comments suppressed due to low confidence (1)
src/Indice.Features.Identity.Core/Constants.cs:215
DeviceSessionIdis now used for non-device sign-ins as well (e.g., password grant). Consider renaming this constant (and the correspondingResolveDeviceSessionIdAPI) to something more general likeSignInSessionId/LoginSessionIdto avoid confusion.
public const string DeviceSessionId = "device_session_id";
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…other than device and ropc
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
src/Indice.Features.Identity.Core/Extensions/HttpContextExtensions.cs:37
GetSessionIduses a fully-qualified reference toHttpContextItemKeys.SessionIdwhileGetOrCreateSessionIduses the unqualified name. Keeping this consistent improves readability and avoids redundant qualification.
/// <summary>Tries to resolve the sign-in session id from the current HTTP request.</summary>
public static string? GetSessionId(this HttpContext httpContext) =>
httpContext.Items.TryGetValue(Indice.Features.Identity.Core.HttpContextItemKeys.SessionId, out var value) ? value?.ToString() : null;
src/Indice.Features.Identity.Core/ResponseHandling/ExtendedTokenResponseGenerator.cs:91
ProcessPasswordRequestAsyncnow only assignsValidatedRequest.SessionIdfromHttpContext.Items. If no earlier component populatedHttpContextItemKeys.SessionId(e.g., for custom validators / error paths), the session id can remain null and the issued tokens may miss thesidclaim. Keeping a fallback to generate a new session id preserves the previous behavior while still allowing correlation when the item is present.
protected override async Task<TokenResponse> ProcessPasswordRequestAsync(TokenRequestValidationResult request) {
var httpContext = ServiceProvider.GetRequiredService<IHttpContextAccessor>().HttpContext!;
request.ValidatedRequest.SessionId ??= httpContext.GetSessionId();
var tokenResponse = await base.ProcessPasswordRequestAsync(request);
src/Indice.Features.Identity.Core/ResponseHandling/ExtendedTokenResponseGenerator.cs:127
ProcessExtensionGrantRequestAsyncno longer generates aSessionIdwhenHttpContextItemKeys.SessionIdis absent. Several extension grants (e.g. delegation/totp) don’t populate the HttpContext item, so tokens issued via those grants can end up without asidclaim. Consider preserving the previous fallback behavior to avoid regressions.
protected override async Task<TokenResponse> ProcessExtensionGrantRequestAsync(TokenRequestValidationResult request) {
var httpContext = ServiceProvider.GetRequiredService<IHttpContextAccessor>().HttpContext!;
request.ValidatedRequest.SessionId ??= httpContext.GetSessionId();
var ip = httpContext.GetClientIpAddress();
…persist it in SignInLogs