Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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.
Reviewed commit: 8569a17a23
ℹ️ 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 (user.Status.IsBlocked) | ||
| if (user.Status.IsBlocked || rolesChanged) | ||
| { | ||
| await RevokeAllTokensAsync(user.Id.Value, cancellationToken); |
There was a problem hiding this comment.
When a user with an existing token changes roles, SyncRoles and the other mappings are already tracked in the same scoped IdentityWriteDbContext used by the OpenIddict token manager; revoking the token saves that context before UpdateSecurityStampAsync validates the user. If the request also supplies a duplicate username/email, the later Identity update can return a conflict even though the role membership and mapped fields were already persisted, potentially granting a privileged role despite the failed response. Keep revocation and the user update in a transaction or otherwise prevent token persistence from flushing the pending user graph first.
Useful? React with 👍 / 👎.
| if (await roleManager.FindByNameAsync(name) is not null) | ||
| { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The production chart was checked and configures two replicas in deploy/helm/tactical-heroes-api/values-production.yaml; during the first deployment, both hosted services can observe a missing role here and then race to create it. The losing replica encounters the unique normalized-role-name constraint and fails startup, either from CreateAsync or the explicit exception below. Treat a duplicate created concurrently as success or serialize the seeding operation.
Useful? React with 👍 / 👎.
| if (user.Status.IsBlocked || rolesChanged) | ||
| { | ||
| await RevokeAllTokensAsync(user.Id.Value, cancellationToken); |
There was a problem hiding this comment.
This invalidates tokens only when a user's membership set changes. Once this change allows users to be assigned roles, updating a role's claims/name or deleting it through RolesRepository changes the effective authorization graph without touching rolesChanged, the users' security stamps, or their OpenIddict tokens. Existing access tokens therefore retain removed role permissions until expiration despite introspection, so role updates and deletions must also invalidate tokens for every assigned user.
Useful? React with 👍 / 👎.
|
0 New Issues
0 Fixed Issues
0 Accepted Issues2 participants
Changes
developer,moderator,player). Preserve existing role IDs and claims; assign Developer during public registration.roleIdson user creation/update and return role IDs in user details. On update, omitted/null preserves membership and[]clears it. Reject invalid/unknown IDs before persistence; invalidate sessions when membership changes.Validation
3d92010after restoring the original workflow: tests, architecture, formatting, Helm, and Sonar Quality Gate. Latest CI: https://github.com/tactical-heroes/api/actions/runs/382931472076176235. New-code coverage: 98.90%; duplication: 0.76%; new issues: 0.dotnet test --project tests/Identity/PANiXiDA.TacticalHeroes.Identity.UnitTests/PANiXiDA.TacticalHeroes.Identity.UnitTests.csproj; repeat for Identity.IntegrationTests, Identity.FunctionalTests, and ArchitectureTests.docker build -f src/PANiXiDA.TacticalHeroes.Host/Dockerfile .anddocker build -f tools/PANiXiDA.TacticalHeroes.Ef.Migrator/Dockerfile .with configured NuGet credentials./api/v1/userswith role IDs from/api/v1/roles, read back GET/api/v1/users/{id}, and confirm public registration assigns Developer.Compatibility
roleIdsand a user-detailsroleIdsfield. Role changes invalidate user sessions; existing users are not automatically reassigned. New default roles have no permission/section grants.