[DB-1971] Pass claims principal and require leader flag when writing using ISystemClient#5568
Open
timothycoleman wants to merge 2 commits intomasterfrom
Open
[DB-1971] Pass claims principal and require leader flag when writing using ISystemClient#5568timothycoleman wants to merge 2 commits intomasterfrom
timothycoleman wants to merge 2 commits intomasterfrom
Conversation
Contributor
Review Summary by QodoPass claims principal parameter to ISystemClient write operations
WalkthroughsDescription• Add ClaimsPrincipal parameter to IWriteOperations write methods • Pass SystemAccounts.System principal in all write operation calls • Update method signatures across publisher extensions and system client • Remove hardcoded system account dependency from publisher write extensions Diagramflowchart LR
A["IWriteOperations Interface"] -->|"Add principal parameter"| B["WriteEvents Methods"]
B -->|"Updated signatures"| C["PublisherWriteExtensions"]
C -->|"Pass principal"| D["IPublisher.WriteEvents"]
D -->|"Use principal instead of hardcoded"| E["WriteEventsCommand"]
F["Call Sites"] -->|"Pass SystemAccounts.System"| B
File Changes1. src/KurrentDB.Core/Bus/Extensions/PublisherWriteExtensions.cs
|
Contributor
Code Review by Qodo
1. TCP auth silently dropped
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
67
to
71
| expectedVersions: expectedRevisions, | ||
| events: events, | ||
| eventStreamIndexes: eventStreamIndexes, | ||
| user: SystemAccounts.System, | ||
| user: principal, | ||
| cancellationToken: cancellationToken |
Contributor
There was a problem hiding this comment.
1. Tcp auth silently dropped 🐞 Bug ⛨ Security
ClientWriteTcpDispatcher only forwards credentials when the write principal contains a DelegatedClaimsIdentity; other identity types are ignored and the request is sent with TcpFlags.None. With this PR allowing arbitrary ClaimsPrincipal to be passed into ClientMessage.WriteEvents, authenticated writes can become anonymous/AccessDenied when routed through the TCP write dispatcher.
Agent Prompt
### Issue description
`ClientWriteTcpDispatcher.CreateWriteRequestPackage` only extracts credentials from `DelegatedClaimsIdentity`. After this PR, `PublisherWriteExtensions.WriteEvents` can pass any `ClaimsPrincipal`, so authenticated callers whose principal does not include `DelegatedClaimsIdentity` will be forwarded over TCP without credentials (`TcpFlags.None`), leading to unexpected `AccessDenied`/anonymous behavior.
### Issue Context
The PR’s intent is to pass the caller principal on writes. That intent will not hold for TCP-forwarded writes unless the dispatcher can extract credentials from non-delegated identities or the code ensures a delegated identity is always present.
### Fix Focus Areas
- src/KurrentDB.Core/Services/Transport/Tcp/ClientWriteTcpDispatcher.cs[144-171]
- src/KurrentDB.Core/Bus/Extensions/PublisherWriteExtensions.cs[28-72]
- src/KurrentDB.Core/Authentication/DelegatedAuthentication/DelegatedAuthenticationProvider.cs[45-53]
### Implementation notes
Choose one:
1) Update `CreateWriteRequestPackage` to also look for `jwt` / `uid` / `pwd` claims on *any* identity (or on `msg.User.Claims`) rather than only `DelegatedClaimsIdentity`.
2) Alternatively, enforce/ensure that principals passed into these write APIs always include a `DelegatedClaimsIdentity` containing the required tokens (e.g., wrap/add identity when missing), and fail fast if not possible.
3) Or plumb tokens via `ClientMessage.WriteEvents(... tokens: ...)` and use those in TCP packaging.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Pull request overview
This PR updates the internal “system client” write path so event writes can carry an explicit ClaimsPrincipal, and updates current callers to pass SystemAccounts.System explicitly.
Changes:
- Extend
ISystemClient/IWriteOperationsand publisher write extensions to require aClaimsPrincipalfor writes. - Thread the principal through
SystemClient→PublisherWriteExtensions→ClientMessage.WriteEvents. - Update producers, secondary indexing, load testing, and tests to pass
SystemAccounts.Systemwhere they write events.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/KurrentDB.Surge/Producers/SystemProducer.cs | Passes SystemAccounts.System when writing via ISystemClient. |
| src/KurrentDB.SecondaryIndexing/Indexes/User/Management/UserIndexEventStore.cs | Supplies system principal for transactional multi-stream writes. |
| src/KurrentDB.SecondaryIndexing.Tests/Fixtures/SecondaryIndexingFixture.cs | Updates fixture helper writes to include system principal. |
| src/KurrentDB.SecondaryIndexing.LoadTesting/Environments/InMemory/PublisherBasedMessageBatchAppender.cs | Updates load-test publisher writes to include system principal. |
| src/KurrentDB.Core/Bus/Extensions/SystemClient.cs | Changes write operations API to require a ClaimsPrincipal and forwards it to publisher write extensions. |
| src/KurrentDB.Core/Bus/Extensions/PublisherWriteExtensions.cs | Replaces implicit system user with a required ClaimsPrincipal argument and forwards it to ClientMessage.WriteEvents. |
| src/KurrentDB.Core.XUnit.Tests/TransactionLog/MultiStreamWrites/MultiStreamWritesTests.cs | Updates test to provide system principal for a write. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
77
to
82
| public interface IWriteOperations { | ||
| Task<WriteEventsResult> WriteEvents(string stream, Event[] events, long expectedRevision = -2, CancellationToken cancellationToken = default); | ||
| Task<WriteEventsResult> WriteEvents(string stream, Event[] events, | ||
| ClaimsPrincipal principal, | ||
| long expectedRevision = -2, | ||
| CancellationToken cancellationToken = default); | ||
| Task<WriteEventsMultiResult> WriteEvents( |
There was a problem hiding this comment.
This changes the public IWriteOperations API by making ClaimsPrincipal a required parameter for all WriteEvents overloads (the prior overload without a principal is removed). If this interface is consumed outside this repo, it’s a breaking change; consider reintroducing the previous overload(s) (e.g., defaulting to SystemAccounts.System) or providing a backward-compatible extension method, while keeping the new principal-aware overloads for callers that need them.
Comment on lines
28
to
35
| public static async Task<WriteEventsResult> WriteEvents( | ||
| this IPublisher publisher, | ||
| string stream, | ||
| Event[] events, | ||
| ClaimsPrincipal principal, | ||
| long expectedRevision = ExpectedVersion.Any, | ||
| CancellationToken cancellationToken = default | ||
| ) { |
There was a problem hiding this comment.
The PublisherWriteExtensions.WriteEvents overload now requires a ClaimsPrincipal argument and removes the previous call pattern that relied on the implicit SystemAccounts.System user. Since this class is marked [PublicAPI], this is a breaking change for any downstream callers; consider adding an overload matching the previous signature (defaulting to SystemAccounts.System) so existing code can compile unchanged while still allowing principal-aware writes.
timothycoleman
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Allows these to be more flexible, V2 projections will use them.
Note that if requireLeader is false we might forward the write to the leader, but the credentials will only be preserved if the principal is SystemAccounts.System or contains a DelegatedClaimsIdentity (such as when received via an API). see ClientWriteTcpDispatcher.CreateWriteRequestPackage.