Migrate Identity from string IDs to GUIDs #930
Conversation
Changed all Identity-related entities to use Guid instead of string for primary keys (User ID and Role ID). Changes: - Created ApplicationRole : IdentityRole<Guid> - Updated ApplicationUser to inherit from IdentityUser<Guid> - Updated PhotoBankDbContext to use IdentityDbContext<ApplicationUser, ApplicationRole, Guid> - Changed RoleAccessProfile.RoleId from string to Guid - Changed UserAccessProfile.UserId from string to Guid - Updated ICurrentUser.UserId to Guid - Updated CurrentUser and DummyCurrentUser to use Guid - Modified HttpContextCurrentUserAccessor to parse Guid from claims - Updated TokenService to convert Guid to string for JWT claims - Updated IAdminUserService and AdminUserService method signatures to use Guid - Updated UsersController endpoints to accept Guid parameters - Changed UserDto.Id from string to Guid - Updated AddPhotobankApiExtensions to use ApplicationRole instead of IdentityRole Breaking Changes: - All API endpoints that accept user IDs now require Guid format - Database migration will be required to convert existing string IDs to Guid - JWT tokens will contain Guid string representations in claims Next Steps: - Run database migration: dotnet ef migrations add MigrateToGuid -p PhotoBank.DbContext -s PhotoBank.Api - Update database: dotnet ef database update -p PhotoBank.DbContext -s PhotoBank.Api - Existing user accounts will need to be migrated or recreated
Added useUnionTypes: true to orval config for both frontend and tv targets. This ensures that TypeScript types use union types (e.g., string | null) instead of optional properties, providing better type safety and more explicit nullable handling. Changes: - Added useUnionTypes: true to frontend orval config - Added useUnionTypes: true to tv orval config The existing frontend code already uses optional chaining (?.) and nullish coalescing (??) operators correctly, so no code changes are required to support union types.
Updated the OpenAPI specification to reflect the backend migration
from string to Guid for user identifiers.
Changes:
- UserDto.id: Added format: uuid
- All /admin/users/{id} endpoint parameters: Added format: uuid
- /admin/users/{id}/reset-password parameter: Added format: uuid
- /admin/users/{id}/roles parameter: Added format: uuid
This ensures the frontend API client correctly handles user IDs
as UUID strings, matching the backend Guid type.
Note: In TypeScript/JavaScript, Guid is represented as a string
with UUID format for JSON serialization.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| public class RoleAccessProfile | ||
| { | ||
| public string RoleId { get; set; } = default!; | ||
| public Guid RoleId { get; set; } | ||
| public int ProfileId { get; set; } | ||
| } | ||
|
|
||
| public class UserAccessProfile | ||
| { | ||
| public string UserId { get; set; } = default!; | ||
| public Guid UserId { get; set; } |
There was a problem hiding this comment.
The link entities now expose RoleAccessProfile.RoleId and UserAccessProfile.UserId as Guid, but the rest of the access-profile API still treats these values as strings. AccessProfileService.AssignUserAsync/UnassignUserAsync/AssignRoleAsync and the corresponding controller actions continue to accept string userId/string roleId and compare them with x.UserId == userId or x.RoleId == roleId, which no longer compiles once the properties are Guid. The public routes also still accept {userId} as an arbitrary string. All of these call sites need to switch to Guid (or explicitly convert to string) to keep the project building and to ensure the API matches the new identifier type.
Useful? React with 👍 / 👎.
| public class RoleAccessProfile | ||
| { | ||
| public string RoleId { get; set; } = default!; | ||
| public Guid RoleId { get; set; } | ||
| public int ProfileId { get; set; } | ||
| } | ||
|
|
||
| public class UserAccessProfile | ||
| { | ||
| public string UserId { get; set; } = default!; | ||
| public Guid UserId { get; set; } |
There was a problem hiding this comment.
Changing UserAccessProfile.UserId and RoleAccessProfile.RoleId to Guid requires the access provider to query using GUIDs as well. EffectiveAccessProvider.GetAsync still accepts a string userId and builds queries like roleIds.Contains(rp.RoleId) and _db.UserAccessProfiles.Where(up => up.UserId == userId), where roleIds is a List<string>. After this change the code no longer compiles, because the LINQ expressions compare string to Guid. IEffectiveAccessProvider and its implementation need to accept/parse GUIDs and adjust the queries accordingly.
Useful? React with 👍 / 👎.
| public interface ICurrentUser | ||
| { | ||
| string UserId { get; } | ||
| Guid UserId { get; } |
There was a problem hiding this comment.
The ICurrentUser contract now exposes Guid UserId, yet helper code such as PhotoBank.Services/Internal/CacheKeys.PersonsOf, PathsOf, and StoragesOf still take string userId. Because those helpers are invoked as CacheKeys.Persons(currentUser) etc., the project will not build after this change—Guid can’t be passed to a method expecting string without an explicit conversion. Either revert UserId to string or update the cache key helpers to accept Guid (and call ToString() when building the key).
Useful? React with 👍 / 👎.
Updated access profile assignment endpoints to use Guid instead of string for userId and roleId parameters to match the updated Identity system. Changes: - IAccessProfileService: Changed method signatures to accept Guid parameters - AccessProfileService: Updated implementation to use Guid, converting to string only for cache invalidation - AdminAccessProfilesController: Updated route parameters from string to Guid - EffectiveAccessProvider: Added Guid parsing for role claims and userId to properly query RoleAccessProfile and UserAccessProfile entities This fixes compilation errors where string parameters were being compared with Guid properties in the database entities. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Updated PersonsOf, PathsOf, and StoragesOf methods to accept Guid instead of string to match ICurrentUser.UserId type change from string to Guid. The Guid is automatically converted to string via string interpolation when building the cache key. This fixes compilation errors where currentUser.UserId (now Guid) was being passed to methods expecting string parameters. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var roleIds = principal.Claims | ||
| .Where(c => c.Type == ClaimTypes.Role) | ||
| .Select(c => c.Value) | ||
| .Where(v => Guid.TryParse(v, out _)) | ||
| .Select(v => Guid.Parse(v)) | ||
| .ToList(); | ||
|
|
||
| if (!Guid.TryParse(userId, out var userGuid)) | ||
| { | ||
| var empty = new EffectiveAccess(new HashSet<int>(), new HashSet<int>(), new List<(DateOnly, DateOnly)>(), false, false); | ||
| _cache.Set(key, empty, TimeSpan.FromMinutes(5)); | ||
| return empty; | ||
| } | ||
|
|
||
| var profileIds = await _db.RoleAccessProfiles | ||
| .Where(rp => roleIds.Contains(rp.RoleId)) | ||
| .Select(rp => rp.ProfileId) | ||
| .Concat(_db.UserAccessProfiles.Where(up => up.UserId == userId).Select(up => up.ProfileId)) | ||
| .Concat(_db.UserAccessProfiles.Where(up => up.UserId == userGuid).Select(up => up.ProfileId)) | ||
| .Distinct() |
There was a problem hiding this comment.
The access provider now parses every ClaimTypes.Role value as a Guid before looking up RoleAccessProfiles. However, tokens are still populated with role names (AuthService.CreateTokenAsync builds new Claim(ClaimTypes.Role, role)), so none of those claims will survive the Guid.TryParse filter. After this change any profile assignments granted via roles are silently skipped, leaving users with only their direct assignments and potentially losing expected permissions. Consider emitting role IDs in the token or comparing by role names instead.
Useful? React with 👍 / 👎.
JWT tokens contain role names (e.g., "Admin", "User"), not role IDs. Updated EffectiveAccessProvider to look up role IDs from role names using RoleManager before querying RoleAccessProfiles. Previously, the code attempted to parse role names as Guids, which always failed and caused all role-based profile assignments to be silently skipped. Users would lose expected permissions granted through their roles. Changes: - Inject RoleManager<ApplicationRole> into EffectiveAccessProvider - Extract role names from ClaimTypes.Role claims - Look up corresponding role IDs using RoleManager.FindByNameAsync - Use the resolved role IDs to query RoleAccessProfiles 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| using Microsoft.EntityFrameworkCore.Internal; | ||
| using Models; | ||
|
|
||
| public class PhotoBankDbContext : IdentityDbContext<ApplicationUser>, IDbContextPoolable | ||
| public class PhotoBankDbContext : IdentityDbContext<ApplicationUser, ApplicationRole, Guid>, IDbContextPoolable |
There was a problem hiding this comment.
Switching PhotoBankDbContext to IdentityDbContext<ApplicationUser, ApplicationRole, Guid> means all ASP.NET Identity tables now expect GUID primary keys, but this commit does not update the EF migrations or snapshot (they still describe IdentityRole and related tables with string IDs). Applying this code to an existing database will leave the schema out of sync and cause runtime failures when RoleManager/UserManager try to persist Guid values to text columns. A migration converting the Identity tables and join tables to uuid columns is required.
Useful? React with 👍 / 👎.
Created migrations to update database schema to match the code changes
from IdentityDbContext<ApplicationUser, IdentityRole> (string IDs) to
IdentityDbContext<ApplicationUser, ApplicationRole, Guid> (Guid IDs).
Migrations included:
1. MigrateIdentityToGuid - Converts all ASP.NET Identity tables:
- AspNetUsers.Id: text -> uuid
- AspNetRoles.Id: text -> uuid
- All foreign key columns in AspNetUserRoles, AspNetUserClaims,
AspNetUserLogins, AspNetUserTokens, AspNetRoleClaims
2. MigrateAccessProfileToGuid - Converts AccessControl tables:
- RoleAccessProfiles.RoleId: text -> uuid
- UserAccessProfiles.UserId: text -> uuid
Also added MIGRATION_GUID.md with detailed instructions for:
- Fresh database setup
- Migrating existing data with valid UUID IDs
- Migrating existing data with non-UUID string IDs (requires custom SQL)
- Verification and troubleshooting steps
IMPORTANT: These migrations will fail if existing data has non-UUID
string IDs. Review MIGRATION_GUID.md before applying to production.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| public interface ICurrentUser | ||
| { | ||
| string UserId { get; } | ||
| Guid UserId { get; } | ||
| bool IsAdmin { get; } |
There was a problem hiding this comment.
The interface now exposes Guid UserId, but all of the test helpers that implement ICurrentUser (e.g., NonAdminTestUser in GetPersonsIntegrationTests, the various TestCurrentUser classes in unit tests) still return string UserId. After this change those classes no longer satisfy the interface and the solution fails to compile. Either revert the interface to string or update every implementation and call site to use Guid so the projects build again.
Useful? React with 👍 / 👎.
Updated all test helper implementations of ICurrentUser to use Guid instead of string for the UserId property to match the interface change. Changes: - GetPersonsIntegrationTests.NonAdminTestUser: string userId -> Guid userId - Updated constructor to accept Guid - Updated call site to use Guid.NewGuid() - SearchReferenceDataServiceTests.TestCurrentUser: string userId -> Guid userId - Updated constructor parameter and property - Updated call site to use Guid.NewGuid() instead of "admin"/"user" - PhotoFilterSpecificationTests.TestCurrentUser: string UserId -> Guid UserId - Changed property from "user" to Guid.Empty - PhotoServiceGetPhotoAsyncTests.TestCurrentUser: string UserId -> Guid UserId - Changed property from "user" to Guid.Empty This fixes compilation errors where test implementations no longer satisfied the ICurrentUser interface after UserId was changed to Guid. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // WARNING: This migration converts RoleAccessProfile.RoleId and UserAccessProfile.UserId | ||
| // from string to Guid to match the Identity tables migration. | ||
| // Ensure the main Identity migration has been applied first. | ||
|
|
||
| // Convert RoleAccessProfile.RoleId from text to uuid | ||
| migrationBuilder.AlterColumn<Guid>( | ||
| name: "RoleId", | ||
| table: "RoleAccessProfiles", | ||
| type: "uuid", | ||
| nullable: false, | ||
| oldClrType: typeof(string), | ||
| oldType: "text"); | ||
|
|
||
| // Convert UserAccessProfile.UserId from text to uuid | ||
| migrationBuilder.AlterColumn<Guid>( | ||
| name: "UserId", | ||
| table: "UserAccessProfiles", | ||
| type: "uuid", | ||
| nullable: false, | ||
| oldClrType: typeof(string), |
There was a problem hiding this comment.
The new migration alters RoleAccessProfiles.RoleId and UserAccessProfiles.UserId directly from text to uuid. Prior to this change the access‑profile API stored role names (e.g., "Admin") in RoleAccessProfiles.RoleId—AssignRoleAsync accepted arbitrary strings and EffectiveAccessProvider compared them to ClaimTypes.Role. Casting those values to UUID will fail as soon as the table contains any non‑GUID text, causing the migration to abort on environments that already have role assignments. Consider translating existing rows (e.g., join to AspNetRoles by name and write the corresponding GUID, or rebuild the table) before altering the column type.
Useful? React with 👍 / 👎.
Updated the AccessControl migration to properly handle the conversion of RoleAccessProfiles.RoleId from role names (e.g., "Admin") to role IDs (Guids). The old code stored role NAMES in RoleAccessProfiles.RoleId and compared them directly to ClaimTypes.Role claims. Simply casting these values to uuid would fail on any database with existing role assignments. Migration now: 1. Adds temporary NewRoleId and NewUserId columns (uuid, nullable) 2. Populates NewRoleId by joining with AspNetRoles to look up role names 3. Populates NewUserId by parsing UUIDs or looking up usernames/emails 4. Deletes orphaned rows that cannot be mapped 5. Drops old columns and renames new columns 6. Recreates primary keys with uuid columns This allows the migration to run successfully on databases with: - Role names like "Admin", "User" in RoleAccessProfiles - Usernames or emails in UserAccessProfiles - Valid UUID strings in either table - Mix of the above Updated MIGRATION_GUID.md to document: - The role name to role ID conversion process - Importance of applying Identity migration before AccessControl migration - What happens during the automatic conversion 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Update access profile tests to use GUID user IDs
Identity IDs have been migrated to Guid, yet these integration tests still seed UserAccessProfile with string literals (e.g. "user-unassign") and call the admin endpoints with the same string value in the URL. After the model change the UserId property is Guid and the controller routes bind a Guid, so this code will no longer compile and any request with a non‑GUID path segment will return 400 instead of exercising the real logic. Replace the string literals with generated Guid values and update the expectations so the mocked invalidation receives the matching GUID string.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Updated AdminAccessProfilesControllerTests to use Guid instead of string literals for user IDs in tests that seed UserAccessProfile and call the admin access profile assignment endpoints. Changes: - AssignUser_WhenNewAssignment_InvalidatesEffectiveAccess: - Generate Guid userId instead of using "user-assign" string - Update URL to use Guid value - Update mock verification to expect userId.ToString() - UnassignUser_WhenAssignmentExists_InvalidatesEffectiveAccess: - Seed UserAccessProfile with Guid userId instead of "user-unassign" - Update URL to use Guid value - Update mock verification to expect userId.ToString() - Update_WhenProfileUpdated_InvalidatesAssignedUsers: - Seed UserAccessProfile with Guid userId instead of "user-update" - Update mock verification to expect userId.ToString() - Delete_WhenProfileDeleted_InvalidatesAssignedUsers: - Seed UserAccessProfile with Guid userId instead of "user-delete" - Update mock verification to expect userId.ToString() This fixes compilation errors and ensures tests properly exercise the Guid-based API routes. Previously, string literals would cause 400 Bad Request responses instead of testing the actual logic. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Step 3: Populate NewUserId | ||
| // Try to parse existing UserId as UUID, or look up in AspNetUsers | ||
| migrationBuilder.Sql(@" | ||
| UPDATE ""UserAccessProfiles"" uap | ||
| SET ""NewUserId"" = | ||
| CASE | ||
| -- If UserId is already a valid UUID, use it | ||
| WHEN uap.""UserId"" ~ '^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$' | ||
| THEN uap.""UserId""::uuid | ||
| -- Otherwise look up by username or email | ||
| ELSE ( | ||
| SELECT u.""Id"" | ||
| FROM ""AspNetUsers"" u | ||
| WHERE u.""UserName"" = uap.""UserId"" | ||
| OR u.""Email"" = uap.""UserId"" | ||
| OR u.""Id""::text = uap.""UserId"" | ||
| LIMIT 1 | ||
| ) | ||
| END; | ||
| "); | ||
|
|
||
| // Step 4: Delete any rows that couldn't be mapped (orphaned data) | ||
| migrationBuilder.Sql(@"DELETE FROM ""RoleAccessProfiles"" WHERE ""NewRoleId"" IS NULL;"); | ||
| migrationBuilder.Sql(@"DELETE FROM ""UserAccessProfiles"" WHERE ""NewUserId"" IS NULL;"); |
There was a problem hiding this comment.
The migration populating UserAccessProfiles.NewUserId only treats a user ID as a UUID when it matches the regex [0-9a-f]{8}-… (line 51). Because the pattern is case-sensitive and restricts the letters to a–f, any existing UUID stored with uppercase hex digits (a common output of Guid.ToString().ToUpperInvariant()) will not match. For such rows the CASE expression falls through, the subquery fails to find a user, NewUserId stays NULL, and the subsequent cleanup deletes the mapping (DELETE … WHERE "NewUserId" IS NULL). Running the migration on databases that store uppercase GUIDs therefore drops valid access assignments. The regex should be made case-insensitive or extended to [0-9A-F] so that both representations are preserved.
Useful? React with 👍 / 👎.
Updated the UUID regex pattern in UserAccessProfiles migration to match both uppercase and lowercase hex digits. The original pattern only matched lowercase: [0-9a-f] This caused uppercase UUIDs (e.g., from Guid.ToString().ToUpperInvariant()) to fail the regex check, fall through to the username/email lookup, return NULL, and get deleted as orphaned data. Changed pattern to: [0-9a-fA-F] This now correctly preserves user access profiles with UUIDs in any case: - Lowercase: 550e8400-e29b-41d4-a716-446655440000 - Uppercase: 550E8400-E29B-41D4-A716-446655440000 - Mixed case: 550e8400-E29B-41d4-A716-446655440000 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| public class ApplicationUser : IdentityUser<Guid> | ||
| { | ||
| public long? TelegramUserId { get; set; } | ||
| public TimeSpan? TelegramSendTimeUtc { get; set; } |
There was a problem hiding this comment.
After migrating Identity to GUID keys, ApplicationUser and ApplicationRole now inherit from IdentityUser<Guid>/IdentityRole<Guid> without assigning a value to Id, and the migrations don’t configure a database default. Code paths such as AdminUserService.CreateAsync construct new ApplicationUser and call CreateAsync immediately, so the Id remains Guid.Empty. The first insert may succeed, but every additional user (or role) creation will attempt to reuse the same empty GUID and violate the primary key. Give these entities a constructor that sets Id = Guid.NewGuid() (or configure EF to generate the value) before saving.
Useful? React with 👍 / 👎.
Added parameterless constructors to ApplicationUser and ApplicationRole
that set Id = Guid.NewGuid() to prevent primary key violations when
creating new users or roles.
Problem:
Without explicitly setting the Id, new ApplicationUser and ApplicationRole
instances default to Guid.Empty (00000000-0000-0000-0000-000000000000).
The first insert might succeed, but subsequent inserts fail with primary
key violations because they all attempt to use the same Guid.Empty value.
Solution:
Both entities now have constructors that generate a unique GUID:
- ApplicationUser() sets Id = Guid.NewGuid()
- ApplicationRole() sets Id = Guid.NewGuid()
This ensures every new user or role gets a unique identifier before being
saved to the database, preventing PK constraint violations.
Works seamlessly with object initializers:
```csharp
var user = new ApplicationUser { UserName = "test@example.com" };
// Id is already set to a new Guid by the constructor
```
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (string.IsNullOrWhiteSpace(identifierString)) | ||
| { | ||
| throw new UnauthorizedAccessException("Authenticated user missing identifier claim"); | ||
| } | ||
|
|
||
| if (!Guid.TryParse(identifierString, out var identifier)) | ||
| { | ||
| throw new UnauthorizedAccessException("User identifier is not a valid GUID"); | ||
| } | ||
|
|
||
| var effectiveAccess = await _effectiveAccessProvider | ||
| .GetAsync(identifier, principal, ct) | ||
| .GetAsync(identifierString, principal, ct) | ||
| .ConfigureAwait(false); |
There was a problem hiding this comment.
The effective access cache is keyed with the raw identifierString taken from the claims, but every invalidation path converts the user ID back to a string via Guid.ToString() (e.g., AccessProfileService uses userId.ToString() in Invalidate and InvalidateUsers). Guid.ToString() emits lower‑case hex, so a token that supplies an upper‑case UUID (which is perfectly valid) will populate the cache under an upper‑case key here while subsequent invalidations target a lower‑case key. In that situation, access changes for the user will not be reflected until the cache entry expires. Consider normalizing the GUID casing (e.g., identifier.ToString() or ToLowerInvariant()) before calling _effectiveAccessProvider.GetAsync so cache keys match the invalidation format.
Useful? React with 👍 / 👎.
Fixed a cache key mismatch between cache population and invalidation that prevented access changes from being reflected for users with uppercase UUIDs in their JWT tokens. Problem: 1. JWT tokens can contain UUIDs in any case (uppercase, lowercase, mixed) Example: "550E8400-E29B-41D4-A716-446655440000" 2. HttpContextCurrentUserAccessor passed the raw claim string to EffectiveAccessProvider.GetAsync(), which used it as the cache key: Cache key: "effacc:550E8400-E29B-41D4-A716-446655440000" 3. All invalidation paths (AccessProfileService, etc.) use Guid.ToString() which ALWAYS produces lowercase hex digits: Invalidation targets: "effacc:550e8400-e29b-41d4-a716-446655440000" 4. Keys don't match → cache never invalidated → stale permissions Impact: Users with uppercase UUIDs in tokens would not see access changes (profile assignments, role changes) until the cache entry expired (15 minutes for normal users, 5 minutes for empty access). Solution: After parsing and validating the GUID from claims, normalize it to lowercase by calling identifier.ToString() before passing to GetAsync. This ensures cache keys always match the format used by invalidation. Now: - Cache key: "effacc:550e8400-e29b-41d4-a716-446655440000" (lowercase) - Invalidation: "effacc:550e8400-e29b-41d4-a716-446655440000" (lowercase) - Keys match ✓ 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
PhotoBank/backend/PhotoBank.UnitTests/CurrentUserAccessorTests.cs
Lines 18 to 47 in 113e8e7
Update CurrentUserAccessor tests to compare Guid identifiers
The identity migration changed ICurrentUser.UserId to Guid, but the accessor unit tests still assert and mock with string IDs (for example current.UserId.Should().Be("user1") and IEffectiveAccessProvider.GetAsync("user1", …)). These assertions no longer compile because UserId is now a Guid and GetAsync normalizes to a lowercase GUID string before looking up cache. Adjust the tests to use Guid instances and convert to string only when interacting with IEffectiveAccessProvider so the test suite builds again.
Test fixtures still use string-based IdentityRole services
The production code now registers RoleManager<ApplicationRole> and uses GUID primary keys, but the admin service unit tests still create a RoleManager<IdentityRole> and seed IdentityRole instances. With the new constructor signature AdminUserService(UserManager<ApplicationUser>, RoleManager<ApplicationRole>, …), these tests no longer compile and similar fixtures (e.g., AuthServiceTests, AdminUsersControllerTests) will fail at runtime because the DI container no longer resolves RoleManager<IdentityRole>. Update the tests to use ApplicationRole/RoleManager<ApplicationRole> and IdentityUserRole<Guid> so they match the new identity key type.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Updated all unit tests for HttpContextCurrentUserAccessor to use Guid instead of string literals for user IDs, matching the ICurrentUser.UserId type change from string to Guid. Changes to all tests: 1. Generate Guid userId using Guid.NewGuid() 2. Pass userId.ToString() in JWT claims (NameIdentifier/Sub) 3. Mock IEffectiveAccessProvider.GetAsync to expect userId.ToString() (lowercase normalized string as per HttpContextCurrentUserAccessor) 4. Assert UserId property with Guid values, not strings 5. Anonymous user: Use Guid.Empty instead of BeEmpty() Tests updated: - GetCurrentUserAsync_ShouldPopulateProperties_WhenAuthenticated - GetCurrentUserAsync_ShouldReturnAnonymousUser_WhenUnauthenticated - GetCurrentUserAsync_ShouldFallbackToSubClaim_WhenNameIdentifierMissing - GetCurrentUserAsync_ShouldCacheResultInHttpContextItems - GetCurrentUserAsync_ShouldNotCompleteSynchronously_WhenEffectiveAccessProviderDelays This fixes compilation errors where string literals were used for Guid properties and ensures tests properly verify the GUID normalization behavior (Guid.ToString() produces lowercase). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Updated all test fixtures and the ServerBlazorApp to use ApplicationRole
instead of IdentityRole, matching the production code which now uses
RoleManager<ApplicationRole> with Guid primary keys.
Changes:
- AdminUserServiceTests: Updated CreateRoleManager to return
RoleManager<ApplicationRole> with RoleStore<ApplicationRole, PhotoBankDbContext, Guid>
- AuthServiceTests: Changed test to use ApplicationRole and IdentityUserRole<Guid>
- AdminUsersControllerTests: Updated SeedUsersAsync to use
RoleManager<ApplicationRole> and new ApplicationRole { Name = role }
- ServerBlazorApp/Program.cs: Changed .AddRoles<IdentityRole>() to
.AddRoles<ApplicationRole>()
This ensures all test fixtures compile and run correctly with the new
Guid-based Identity system.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
No description provided.