feat: implement storage of session in database - #52
Conversation
jhbritton-RSK
changed the title
…d also follow same pattern as existig stores from IdS4
3248415 to
2426413
Compare
| <ProjectReference Include="..\..\src\Open.IdentityServer.csproj"/> | ||
| </ItemGroup> | ||
|
|
||
| <ItemGroup> |
There was a problem hiding this comment.
Why is the MockLogger required in this assembly? Does TestLogger.Create<T> not suffice?
There was a problem hiding this comment.
I added this for a mock logger I could validate logs were made. TestLogger is used in may places so wanted avoid touching it. I also shared between two test projects, might be better to spin-up a shared tests project for things like that.
There was a problem hiding this comment.
Missing unit tests for these mappers
There was a problem hiding this comment.
The additional class lacks any unit tests, but can you create one and add a test covering this
| /// <param name="ticketStore">instance of ITicketStore to use</param> | ||
| /// <param name="idsOptions">Open.IdentityServer options</param> | ||
| /// <param name="authOptions">Authentication options</param> | ||
| public class PostConfigureSessionStoreCookieAuthOptions( |
There was a problem hiding this comment.
This feels like something we should have a test covering.
There was a problem hiding this comment.
unit tests on this please
|
|
||
| if (existingSession == null) | ||
| { | ||
| logger.LogInformation("failed renewing '{SessionKey}' session in database, session with key doesn't exists", key); |
There was a problem hiding this comment.
typo: exists shouldn't be plural
There was a problem hiding this comment.
Also this should be log warning not information
|
|
||
| string? subjectId = ticket.Principal.GetSubjectId(); | ||
| string? sessionId = ticket.Properties.GetSessionId(); | ||
| trace?.AddTag(TelemetryConstants.TagConstants.Subject, subjectId); |
There was a problem hiding this comment.
We shouldn't be putting subject and session id in the tags of the telemetry. The success or failure is enough for telemetry.
| public Task RemoveAsync(string key) | ||
| { | ||
| using ITrace? trace = telemetry.Trace(TelemetryConstants.TraceCategories.Stores, this); | ||
| trace?.AddTag(TelemetryConstants.TagConstants.Key, key); |
There was a problem hiding this comment.
Don't put the key in the tags, I can't see the use of this and it will create noise
| /// | ||
| /// </summary> | ||
| /// <param name="seedData"></param> | ||
| public InMemorySessionStore(IDictionary<string, IdentityServerServerSideSessions> seedData): this() |
There was a problem hiding this comment.
Seeding data for sessions? Is this for unit testing?
There was a problem hiding this comment.
I was following original pattern of other stores. But doesn't really make sense for server side sessions other than for testing.
3 participants
Description
Implemented initial storage of session in database following the existing db schema created for compatibility.
Type of change
Does this PR introduce a breaking change?
Testing
Describe how the change has been tested. New code should be covered by appropriate unit and/or integration tests.
LLM Usage
Used for guidance on setting up integration tests, and understanding more about how the sessions work in the existing IdentityServer4 codebase.
Other context
n/a