Repository navigation
Synchronize account and user token lifetimes - #2627
Conversation
7bfd49d to
1cf38ce
Compare
cd9081e to
e31f3c9
Compare
e31f3c9 to
605b43d
Compare
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: 605b43dbf4
ℹ️ 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".
605b43d to
0f3cc3a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f3cc3a1b1
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d643dffa5
ℹ️ 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".
| /// This value does not describe the password hashing format. | ||
| /// </summary> | ||
| public ISet<string> OrganizationIds { get; init; } = new HashSet<string>(); | ||
| public string? AuthenticationVersion { get; set; } |
There was a problem hiding this comment.
When token cleanup fails—the case this generation is intended to protect—an already-authenticated request can retain the pre-rotation User and later overwrite this property through an unrelated whole-document save. For example, OrganizationHandler.AddModelAsync captures the request user before awaiting organization creation and then calls SaveAsync(user); if a password change completes between those operations, that save restores the old/null generation and makes the surviving old tokens valid again. Persist the generation separately, use partial updates for unrelated user mutations, or enforce optimistic concurrency so stale user documents cannot undo revocation.
AGENTS.md reference: AGENTS.md:L79-L82
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aec2c07fb6
ℹ️ 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.HasValidAuthenticationVersion(tokenRecord.AuthenticationVersion)) | ||
| return AuthenticateResult.Fail("Token is not valid"); |
There was a problem hiding this comment.
When token cleanup fails and an authentication request began an uncached user read before a password change, that request can publish the old user snapshot to the cache after SaveAsync has rotated the generation and invalidated the cache. This check then accepts the surviving old token on subsequent requests until the stale cache entry expires or is invalidated again, defeating the revocation fallback; validate the generation against a current read or make cache publication/invalidation ordering safe.
Useful? React with 👍 / 👎.
956f021 to
9593b05
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 956f021aab
ℹ️ 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 destination = await userRepository.GetByIdAsync(destinationUserId); | ||
| if (destination is not null && destination.OAuthAccounts.Any(existing => String.Equals(existing.Provider, account.Provider, StringComparison.Ordinal) | ||
| && String.Equals(existing.ProviderUserId, account.ProviderUserId, StringComparison.Ordinal))) | ||
| return; |
There was a problem hiding this comment.
When another signed-in account links the same provider identity after it is removed from the previous owner but before the destination save fails, this check examines only the intended destination. The rollback can therefore restore the identity to currentOwner even though the third account now owns the same provider/provider-user-id pair, leaving duplicates that GetUserByOAuthProviderAsync resolves via an arbitrary FirstOrDefault; verify that no user currently owns the identity before restoring it.
AGENTS.md reference: AGENTS.md:L81-L81
Useful? React with 👍 / 👎.
|
|
||
| try | ||
| { | ||
| await userRepository.PatchAsync(user.Id, new ActionPatch<User>(current => current.OrganizationIds.Add(organization.Id)), o => o.Cache()); |
There was a problem hiding this comment.
If the authenticated user is deleted after the organization is added but before this patch executes, PatchAsync can return false rather than throw. The handler then skips compensation, updates only the stale request object, publishes UserMembershipChanged, and returns the newly created organization even though no persisted user belongs to it; treat a false result as failure and remove the organization just as the exception path does.
AGENTS.md reference: AGENTS.md:L81-L81
Useful? React with 👍 / 👎.
9593b05 to
2ef6152
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ef6152178
ℹ️ 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.OrganizationIds.Contains(organization.Id)) | ||
| { | ||
| await userRepository.PatchAsync(user.Id, new ActionPatch<User>(current => current.OrganizationIds.Add(organization.Id)), o => o.Cache()); |
There was a problem hiding this comment.
If the user is deleted after GetByEmailAddressAsync but before this patch, PatchAsync returns false; the handler nevertheless mutates the stale object, publishes UserMembershipChanged, sends the organization-added email, and returns success even though no persisted user joined. Check the result and treat a missing destination as failure or create an invite instead.
AGENTS.md reference: AGENTS.md:L81-L81
Useful? React with 👍 / 👎.
| namespace Exceptionless.Core.Models; | ||
|
|
||
| public record User : IIdentity, IHaveDates, IValidatableObject | ||
| public record User : IIdentity, IHaveDates, IVersioned, IValidatableObject |
There was a problem hiding this comment.
Making User versioned causes concurrent whole-user saves to surface as HTTP 409, but the OpenAPI changes cover only auth routes. For example, the organization membership tests now expect a conflict from POST /organizations/{id}/users/{email}, while OrganizationEndpoints.cs and the snapshot still omit that response; avatar, email-address, and verification saves have the same undocumented outcome. Add 409 metadata and regenerate the snapshot for every newly affected endpoint.
AGENTS.md reference: AGENTS.md:L74-L74
Useful? React with 👍 / 👎.
4222af7 to
a178e7b
Compare
|
1 participant
Rotate authentication generations after password changes/resets and external-login removal/transfer, and reject user and OAuth tokens from an older generation. Authentication reads remain cached; existing users and tokens with matching null generations remain supported.
Scope: session lifecycle only. User repository concurrency, serializer changes and transfer compensation are in draft #2631.
Validation: the published revision builds with zero warnings/errors; all 71 focused tests pass. Complete CI is green, including API/OpenAPI, all six browser shards, client checks and Docker builds.
Remaining limitations: stale whole-user writes are addressed separately in #2631, and late cache publication is tracked in Foundatio.Repositories #323. This PR improves token revocation but does not guarantee immediate revocation through stale cached snapshots.
Verification and implementation details
Coverage includes legacy null generations, stale user/OAuth token rejection, password-change cleanup failures and external-login transfers using validated fixtures. The remaining production diff is 86 added and 14 removed lines across eight files; the rest is tests and the API snapshot. Local integration startup was previously blocked by Aspire's EnsureKubernetesAsync; hosted CI provides endpoint and OpenAPI verification. Password-change/reset and provider-transfer browser journeys have not been manually dogfooded on this revision.