feat: first implementation of identity server#15
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements the initial version of an identity server along with updates to user management endpoints and supporting infrastructure. Key changes include:
- Refactored authentication logic in endpoints to use ClaimsPrincipal and a new IAuthService implementation.
- Updates to project references and package versions, along with the removal of the JwtConfiguration file.
- Enhancements in the Aggnostic layer for EF Core integration with migration and identity support.
Reviewed Changes
Copilot reviewed 62 out of 63 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| UpdateUserDetailsEndpoint.cs | Replaced header-based auth with ClaimsPrincipal usage and updated error messages. |
| Stickerlandia.UserManagement.AspNet.csproj | Updated package references and project references, notably downgrading Microsoft.EntityFrameworkCore.Design. |
| Program.cs | Added configuration for environment variables, logging, driven adapter selection, and refactored startup flow. |
| LoginEndpoint.cs & GetUserDetailsEndpoint.cs | Refactored endpoints to use new authentication approaches. |
| Aggnostic/* | Introduced new repository methods, updated Identity models, and added MicrosoftIdentityAuthService along with migration and hosted service changes. |
| infra/main.tf | Removed hardcoded auth settings from app settings. |
| Stickerlandia.UserManagement.sln | Replaced the SharedSetup project with the Auth project. |
Files not reviewed (1)
- user-management/src/Stickerlandia.UserManagement.Agnostic/Migrations/20250502111315_InitialCreate.Designer.cs: Language not supported
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
| /// <summary> | ||
| /// DynamoDB implementation of IRoleStore for ASP.NET Core Identity | ||
| /// </summary> | ||
| public class DynamoDbRoleStore : |
There was a problem hiding this comment.
🔵 Code Quality Violation
class with a Dispose() method must implement IDisposable (...read more)
IDisposable provides an interface for the cleanup of unmanaged resources through the function void Dispose().
To avoid confusion, this rule ensures that any class that exposes a public void Dispose() function must implement IDisposable.
Learn More
There was a problem hiding this comment.
This seems like a reasonable demand!
| /// <summary> | ||
| /// DynamoDB implementation of IUserStore for ASP.NET Core Identity | ||
| /// </summary> | ||
| public class DynamoDbUserStore : |
There was a problem hiding this comment.
🔵 Code Quality Violation
class with a Dispose() method must implement IDisposable (...read more)
IDisposable provides an interface for the cleanup of unmanaged resources through the function void Dispose().
To avoid confusion, this rule ensures that any class that exposes a public void Dispose() function must implement IDisposable.
Learn More
…ations/20250527125041_InitialCreate.cs Co-authored-by: datadog-datadog-prod-us1[bot] <88084959+datadog-datadog-prod-us1[bot]@users.noreply.github.com>
…ations/20250527125041_InitialCreate.cs Co-authored-by: datadog-datadog-prod-us1[bot] <88084959+datadog-datadog-prod-us1[bot]@users.noreply.github.com>
| return new ValueTask<ImmutableDictionary<CultureInfo, string>>(cultureDisplayNames.ToImmutable()); | ||
| } | ||
| } | ||
| catch (JsonException) |
There was a problem hiding this comment.
🟠 Code Quality Violation
Catch clause is empty (...read more)
Exceptions must be appropriately handled and have code to recover from the exceptions. If no recovery is added, the code should at least log the error.
scottgerring
left a comment
scottgerring
left a comment
There was a problem hiding this comment.
Nice! Chunky PR is chunky.
Some observations and a couple things I think we may wanna change. Also can you check that this still works in the docker-compose environment before we merge? There are test URLs in the compose file itself.
There's also an upper limit to how big a PR can get before you can't really sensibly review it anymore. I too am 100% guilty of this. I think we should operate like this:
- We're in "knock out the boundaries" mode, so we accept big PRs and high-level scrutiny in exchange for velocity
- Once we've got a service into a reasonable state, we start to do narrow, focussed PRs, and potentially carefully crafted commits (refactor/impl/cleanup), and stacked PRs where appropriate
What do you think?
|
|
||
| b.HasIndex("NormalizedName") | ||
| .IsUnique() | ||
| .HasDatabaseName("RoleNameIndex"); |
There was a problem hiding this comment.
Yep, I agree with your comments on smaller, focused PR's. I'm guilty of it mostly because I get carried away and keep adding stuff. But going forward let's keep it small and focused.
To answer your question, this is the auto-generated EntityFramework migration code. That looks like it's a unique index on the internal database table for roles, to make sure you can't have two roles with the same name.
There was a problem hiding this comment.
i think while we smash out the highlevel structure we can still play it fast and loose, but we should actively shift mental models at some point!
| { | ||
| public string FirstName { get; set; } = string.Empty; | ||
| public string LastName { get; set; } = string.Empty; | ||
| public int ClaimedStickerCount { get; set; } |
There was a problem hiding this comment.
I don't think this belongs here? You should be asking the award service
There was a problem hiding this comment.
Interesting point of discussion. Now with my EDA hat on I'd try to avoid needing to make that synchronous call by keeping a local 'cache' of the current # of stickers. Happy to drop that column if you think that's more sensible though.
To add to that, I was also thinking it gives us a reason for the user service to respond to the sticker assigned event.....
| entity.Property(e => e.Id).HasColumnName("id"); | ||
| entity.Property(e => e.FirstName).HasColumnName("first_name"); | ||
| entity.Property(e => e.LastName).HasColumnName("last_name"); | ||
| entity.Property(e => e.ClaimedStickerCount).HasColumnName("claimed_sticker_count"); |
There was a problem hiding this comment.
per above - don't think this should be here
There was a problem hiding this comment.
Pull Request Overview
This PR introduces the first implementation of the identity server, including API project renames, Docker support, EF Core identity integration, and a new messaging worker.
- Add Dockerfiles for x86-64 and ARM64 builds and update docker-compose service definitions
- Rename namespaces in the API project and consolidate
ApiResponselogic - Introduce EF Core
UserManagementDbContext, identity models,PostgresUserRepository, OpenIddict setup, and a Kafka-based messaging worker
Reviewed Changes
Copilot reviewed 113 out of 114 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/Stickerlandia.UserManagement.Api/Dockerfile{-x86} | Added multi-arch Dockerfiles for the API |
| src/Stickerlandia.UserManagement.Api/ApiResponse.cs | Simplified response-writing logic and updated namespace |
| src/Stickerlandia.UserManagement.Agnostic/ServiceExtensions.cs | Moved Kafka and Postgres auth service registrations |
| src/Stickerlandia.UserManagement.Agnostic/PostgresUserRepository.cs | Refactored repository to use UserManager and DI |
| src/Stickerlandia.UserManagement.Agnostic/KafakStickerClaimedWorker.cs | Added Kafka message processor with DLQ handling |
| docker-compose.yml | Updated service definitions to use the new API and worker |
| Stickerlandia.UserManagement.Agnostic/Stickerlandia.UserManagement.Agnostic.csproj | Updated NuGet references for EF Core and OpenIddict |
Files not reviewed (1)
- user-management/src/Stickerlandia.UserManagement.Agnostic/Migrations/20250502111315_InitialCreate.Designer.cs: Language not supported
Comments suppressed due to low confidence (5)
user-management/src/Stickerlandia.UserManagement.Agnostic/KafakStickerClaimedWorker.cs:18
- [nitpick] The class name misspells "Kafka" as "Kafak". Rename to
KafkaStickerClaimedWorkerfor clarity and consistency.
public class KafakStickerClaimedWorker
user-management/src/Stickerlandia.UserManagement.Agnostic/MicrsoftIdentityAuthService.cs:18
- The filename
MicrsoftIdentityAuthService.csis missing an 'o' in Microsoft. Rename the file toMicrosoftIdentityAuthService.csto match the class name.
public class MicrosoftIdentityAuthService(
user-management/src/Stickerlandia.UserManagement.Api/ApiResponse.cs:46
- The WriteResponse overload ignores the passed-in statusCode parameter and always uses the existing
StatusCodeproperty. Consider assigningStatusCode = statusCode;before creating the response so the correct code is returned.
activeSpan.SetTag("http.status_code", (int)StatusCode);
user-management/src/Stickerlandia.UserManagement.Agnostic/KafakStickerClaimedWorker.cs:58
- The
producerConfigparameter is not stored in a field; the class defines_producerConfigbut referencesproducerConfig, which will not compile. Update the reference to_producerConfigor assign the constructor parameter to a field.
using var producer = new ProducerBuilder<string, string>(producerConfig).Build();
user-management/src/Stickerlandia.UserManagement.Agnostic/Stickerlandia.UserManagement.Agnostic.csproj:20
- There are duplicate
PackageReferenceentries forMicrosoft.AspNetCore.Identity.EntityFrameworkCore. Remove the redundant reference to avoid confusion.
<PackageReference Include="Microsoft.AspNetCore.Identity.EntityFrameworkCore" Version="8.0.16" />
There was a problem hiding this comment.
Pull Request Overview
The PR introduces the first implementation of an identity server for user management, wiring up OpenIddict, EF Core with PostgreSQL, and Docker/CI support.
- Multi-arch Dockerfiles and Docker Compose updates for API, worker, and migration services
- New
Agnosticproject with IdentityDbContext, repository, entities, migrations, and auth service - Refactored DI (
ServiceExtensions), API response tracing, and updated solution/project references
Reviewed Changes
Copilot reviewed 113 out of 114 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| user-management/src/Stickerlandia.UserManagement.Api/Dockerfile-x86 | Add multi-stage .NET 8.0 Docker build for linux-x64 |
| user-management/src/Stickerlandia.UserManagement.Api/Dockerfile | Add multi-stage .NET 8.0 Docker build for linux-arm64 |
| user-management/src/Stickerlandia.UserManagement.Api/Configurations/DocumentationConfig.cs | Fix namespace from AspNet to Api |
| user-management/src/Stickerlandia.UserManagement.Api/ApiResponse.cs | Refactor response serialization and tracing tag usage |
| user-management/src/Stickerlandia.UserManagement.Agnostic/UserManagementDbContext.cs | Introduce IdentityDbContext with OpenIddict |
| user-management/src/Stickerlandia.UserManagement.Agnostic/Stickerlandia.UserManagement.Agnostic.csproj | Update EF, Identity, OpenIddict package references |
| user-management/src/Stickerlandia.UserManagement.Agnostic/ServiceExtensions.cs | Split DI into AddKafkaMessaging and AddPostgresAuthServices |
| user-management/src/Stickerlandia.UserManagement.Agnostic/PostgresUserRepository.cs | Migrate to primary‐constructor repository using UserManager |
| user-management/src/Stickerlandia.UserManagement.Agnostic/PostgresUserAccount.cs | Define IdentityUser-based account entity |
| user-management/src/Stickerlandia.UserManagement.Agnostic/PostgresOutboxItem.cs | Define outbox item entity |
| user-management/src/Stickerlandia.UserManagement.Agnostic/Migrations/20250527125041_InitialCreate.cs | Scaffold initial schema including Identity & OpenIddict |
| user-management/src/Stickerlandia.UserManagement.Agnostic/Migrations/20250502111315_InitialCreate.cs | Remove legacy simple migration |
| user-management/src/Stickerlandia.UserManagement.Agnostic/MicrsoftIdentityAuthService.cs | Implement OpenIddict auth service |
| user-management/src/Stickerlandia.UserManagement.Agnostic/KafakStickerClaimedWorker.cs | Implement Kafka consumer with DLQ support |
| user-management/src/Stickerlandia.UserManagement.Agnostic/DbContextFactory.cs | Remove direct repository registration |
| user-management/infra/main.tf | Remove hard-coded Auth__ settings from app_settings |
| user-management/Stickerlandia.UserManagement.sln | Update solution to reference new projects (Api, Auth, Worker, etc.) |
| docker-compose.yml | Point to new Dockerfiles, add worker & migrations services, env updates |
| .github/workflows/user-management-test.yml | Update matrix, .NET versions, and CI steps |
Files not reviewed (1)
- user-management/src/Stickerlandia.UserManagement.Agnostic/Migrations/20250502111315_InitialCreate.Designer.cs: Language not supported
Comments suppressed due to low confidence (3)
user-management/src/Stickerlandia.UserManagement.Agnostic/KafakStickerClaimedWorker.cs:18
- [nitpick] The class name
KafakStickerClaimedWorkerappears to be a typo. Consider renaming it (and the filename) toKafkaStickerClaimedWorkerfor consistency and clarity.
public class KafakStickerClaimedWorker(
user-management/src/Stickerlandia.UserManagement.Agnostic/MicrsoftIdentityAuthService.cs:1
- The filename
MicrsoftIdentityAuthService.csis missing an 'o' in 'Microsoft', which could lead to confusion—rename toMicrosoftIdentityAuthService.csto match the class it contains.
// Unless explicitly stated otherwise all files in this repository are licensed under the Apache License Version 2.0.
user-management/src/Stickerlandia.UserManagement.Agnostic/DbContextFactory.cs:20
- You appear to have removed the registration of
PostgresUserRepositoryfor bothIUsersandIOutbox, which will break DI resolution—re-add theseAddScopedcalls or confirm alternative registration.
services.AddScoped<IUsers, PostgresUserRepository>();
| internal async Task<HttpResponseData> WriteResponse(HttpRequestData req) | ||
| { | ||
| var response = req.CreateResponse(StatusCode); | ||
| await response.WriteAsJsonAsync(this); | ||
| return response; | ||
| } | ||
|
|
||
| internal async Task<HttpResponseData> WriteResponse(HttpRequestData req, HttpStatusCode statusCode) | ||
| { | ||
| var activeSpan = Tracer.Instance.ActiveScope?.Span; | ||
|
|
||
| if (activeSpan != null) | ||
| { | ||
| activeSpan.SetTag("http.status_code", (int)statusCode); | ||
| activeSpan.SetTag("http.status_code", (int)StatusCode); | ||
| activeSpan.SetTag("http.method", req.Method); | ||
| } | ||
|
|
||
| StatusCode = statusCode; | ||
| return await WriteResponse(req); | ||
| var response = req.CreateResponse(StatusCode); |
There was a problem hiding this comment.
In the overload WriteResponse(HttpRequestData req, HttpStatusCode statusCode), you should use the statusCode parameter for tracing ((int)statusCode) rather than the instance StatusCode property, which may not reflect the intended code.
| internal async Task<HttpResponseData> WriteResponse(HttpRequestData req) | ||
| { | ||
| var response = req.CreateResponse(StatusCode); | ||
| await response.WriteAsJsonAsync(this); | ||
| return response; | ||
| } | ||
|
|
||
| internal async Task<HttpResponseData> WriteResponse(HttpRequestData req, HttpStatusCode statusCode) | ||
| { | ||
| var activeSpan = Tracer.Instance.ActiveScope?.Span; | ||
|
|
||
| if (activeSpan != null) | ||
| { | ||
| activeSpan.SetTag("http.status_code", (int)statusCode); | ||
| activeSpan.SetTag("http.status_code", (int)StatusCode); | ||
| activeSpan.SetTag("http.method", req.Method); | ||
| } | ||
|
|
||
| StatusCode = statusCode; | ||
| return await WriteResponse(req); | ||
| var response = req.CreateResponse(StatusCode); |
There was a problem hiding this comment.
When creating the HTTP response in the WriteResponse overload, pass the provided statusCode parameter to CreateResponse instead of the old StatusCode property to ensure the correct status is returned.
| <PackageReference Include="CloudNative.CloudEvents" Version="2.8.0" /> | ||
| <PackageReference Include="CloudNative.CloudEvents.SystemTextJson" Version="2.8.0" /> | ||
| <PackageReference Include="Confluent.Kafka" Version="2.10.0" /> | ||
| <PackageReference Include="Microsoft.AspNetCore.Identity.EntityFrameworkCore" Version="8.0.16" /> |
There was a problem hiding this comment.
The Microsoft.AspNetCore.Identity.EntityFrameworkCore package is referenced twice—remove the duplicate entry to avoid unnecessary restoration and possible confusion.
No description provided.