feat(auth): support built-in OAuth methods#439
Merged
Conversation
yordis
commented
Jul 15, 2026
yordis
commented
Member
- PostgreSQL's method-oriented authentication model gives operators a familiar way to reason about password and OAuth access side by side.
- Modern deployments need external IdP bearer-token authentication without making database auth depend on plugin-only extension points.
- Password authentication should remain available while OAuth is introduced so operators can migrate deliberately.
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
PR SummaryHigh Risk Overview Introduces a built-in OAuth/OIDC provider that validates bearer tokens (issuer, audiences, OIDC metadata) and maps name/role claims. When OAuth-only is configured, user-certificate auth is still wired through the internal password store for cert-based access. The admin UI gains an OAuth browser sign-in path: PKCE code challenge endpoints, authorization-code callback with token exchange, JWT-only cookies ( Configuration, telemetry, and startup validation now key off built-in user store semantics (password or OAuth) rather than a single Reviewed by Cursor Bugbot for commit ffbe9e8. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAuthentication now supports multiple configured methods, composite provider routing, OAuth JWT validation, and an OAuth PKCE browser sign-in flow. Configuration, middleware challenges, telemetry, cookies, UI rendering, startup wiring, documentation, and automated tests were updated. ChangesAuthentication and OAuth integration
Validation and supporting documentation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant SignInPage
participant OAuthBrowserFlowService
participant OAuthProvider
participant UiCredentialCookie
SignInPage->>OAuthBrowserFlowService: request PKCE challenge
OAuthBrowserFlowService-->>SignInPage: challenge and correlation state
SignInPage->>OAuthProvider: browser authorization
OAuthProvider->>OAuthBrowserFlowService: callback with code and state
OAuthBrowserFlowService->>OAuthProvider: exchange authorization code
OAuthBrowserFlowService->>UiCredentialCookie: append OAuth token
OAuthBrowserFlowService-->>SignInPage: redirect to /ui/signin
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/EventStore.ClusterNode/Components/Pages/SignIn.razor (1)
52-88: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winPrevent duplicate error messages when multiple auth methods are supported.
Because
Messageis conditionally rendered inside both theAuthentication.SupportsOAuthBrowserFlowandAuthentication.SupportsBasicblocks, an error (such as invalid credentials or a failed OAuth flow) will be displayed twice on the screen if both methods are enabled.Consider lifting the message display above the individual authentication sections so it is rendered only once.
🎨 Proposed UI fix
else { + `@if` (!string.IsNullOrWhiteSpace(Message)) + { + <div class="mb-6 rounded-2xl border border-red-200 bg-red-50 p-4 text-sm font-bold text-red-800">`@Message`</div> + } + `@if` (Authentication.SupportsOAuthBrowserFlow) { <div class="mb-6 rounded-[1.5rem] border border-es-ink/10 bg-es-cloud/70 p-5"> <h2 class="text-2xl font-black tracking-tight text-es-ink">Continue with OAuth</h2> <p class="mt-2 text-sm leading-6 text-es-muted">Use the configured browser sign-in flow for this node.</p> <button class="mt-4 rounded-2xl bg-es-ink px-5 py-3 text-sm font-black text-white shadow-lg shadow-es-ink/15 transition hover:bg-es-green" type="button" data-ui-oauth-signin data-ui-oauth-return="`@Destination`" data-ui-oauth-type="`@Authentication.Type`" data-ui-oauth-properties="`@Authentication.PropertiesJson`">Continue with OAuth</button> - `@if` (!string.IsNullOrWhiteSpace(Message)) - { - <p class="mt-4 rounded-2xl border border-red-200 bg-red-50 p-4 text-sm font-bold text-red-800">`@Message`</p> - } <p class="mt-4 hidden rounded-2xl border border-red-200 bg-red-50 p-4 text-sm font-bold text-red-800" data-ui-oauth-status></p> </div> } `@if` (Authentication.SupportsBasic) { <EditForm Model="Input" FormName="ui-signin" OnValidSubmit="SignInUser"> <AntiforgeryToken /> <DataAnnotationsValidator /> <div class="grid gap-4"> <label class="text-sm font-bold text-es-muted"> Username <InputText autocomplete="username" autofocus class="mt-2 w-full rounded-2xl border border-es-ink/10 bg-white px-4 py-3 text-es-ink outline-none transition focus:border-es-green focus:ring-4 focus:ring-es-green/15" `@bind-Value`="Input.Username" /> <ValidationMessage For="() => Input.Username" /> </label> <label class="text-sm font-bold text-es-muted"> Password <InputText autocomplete="current-password" type="password" class="mt-2 w-full rounded-2xl border border-es-ink/10 bg-white px-4 py-3 text-es-ink outline-none transition focus:border-es-green focus:ring-4 focus:ring-es-green/15" `@bind-Value`="Input.Password" /> <ValidationMessage For="() => Input.Password" /> </label> </div> - `@if` (!string.IsNullOrWhiteSpace(Message)) - { - <p class="mt-5 rounded-2xl border border-red-200 bg-red-50 p-4 text-sm font-bold text-red-800">`@Message`</p> - } - <div class="mt-6 flex flex-wrap gap-2">🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/EventStore.ClusterNode/Components/Pages/SignIn.razor` around lines 52 - 88, Move the shared Message display out of the Authentication.SupportsOAuthBrowserFlow and Authentication.SupportsBasic sections and render it once above those authentication method blocks. Preserve the existing conditional visibility and styling, while removing both duplicated inline Message elements.
🧹 Nitpick comments (3)
src/EventStore.Core/Authentication/CompositeAuthenticationProvider.cs (2)
34-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrevent potential duplicate keys in aggregated public properties.
If multiple underlying providers return the same property key,
SelectManywill yield duplicates. If downstream consumers load these properties into aDictionary, this will throw anArgumentException. Consider filtering by distinct keys to make the aggregate sequence safer.♻️ Proposed refactor
public override IEnumerable<KeyValuePair<string, string>> GetPublicProperties() => - providers.SelectMany(provider => provider.GetPublicProperties() ?? []); + providers + .SelectMany(provider => provider.GetPublicProperties() ?? []) + .DistinctBy(p => p.Key);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/EventStore.Core/Authentication/CompositeAuthenticationProvider.cs` around lines 34 - 35, Update CompositeAuthenticationProvider.GetPublicProperties to deduplicate aggregated properties by their key after flattening provider results. Preserve each key’s first property value and continue ignoring null property collections, ensuring downstream consumers receive unique keys.
67-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHardcoded scheme selection limits extensibility.
The
SelectSchememethod explicitly binds the composite provider routing toBearer,UserCertificate, andBasic. While this covers the immediate built-in methods, it makes the composite provider rigid and unaware of additional future schemes (such as those introduced by external plugins).Consider mapping the intended scheme dynamically from the
AuthenticationRequestif possible, so that routing adheres better to the Open-Closed Principle for future auth types.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/EventStore.Core/Authentication/CompositeAuthenticationProvider.cs` around lines 67 - 75, Update SelectScheme to derive the authentication scheme from AuthenticationRequest or the existing extensible scheme-resolution mechanism instead of hardcoding Bearer, UserCertificate, and Basic. Preserve the current built-in routing behavior and fallback while allowing externally registered authentication schemes to be selected without modifying this method.src/EventStore.Core.Tests/EventStore.Core.Tests.csproj (1)
29-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider avoiding upward dependencies in test projects.
Adding a project reference from
EventStore.Core.TeststoEventStore.ClusterNodebreaks the dependency direction (Core$\rightarrow$ ClusterNode).If this reference is needed to test
ClusterNodecomponents (such as the newly added browser flow endpoints), consider moving those tests to anEventStore.ClusterNode.Testsproject to preserve clear architectural boundaries.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/EventStore.Core.Tests/EventStore.Core.Tests.csproj` at line 29, The EventStore.Core.Tests project now depends upward on EventStore.ClusterNode. Remove the EventStore.ClusterNode project reference from EventStore.Core.Tests and move the ClusterNode-specific tests, including browser flow endpoint coverage, into an EventStore.ClusterNode.Tests project.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/EventStore.ClusterNode/Components/Services/OAuthBrowserFlowEndpoints.cs`:
- Around line 42-54: Replace the unbounded _pendingChallenges state used by
CreateCodeChallenge with a short-lived encrypted browser cookie containing the
PKCE verifier, and recover and validate it during the OAuth callback. Remove the
per-request PruneExpired iteration and correlationId dictionary dependency,
ensuring cookie settings bind the challenge to the user agent and prevent Login
CSRF. If server-side storage is retained, use a bounded IMemoryCache with
expiration and require an explicit correlation cookie during callback
validation.
In `@src/EventStore.ClusterNode/Components/Services/SecurityBrowserService.cs`:
- Around line 28-32: Complete the OAuth browser-flow property contract in
SecurityBrowserService by requiring response_type and scope alongside the
existing properties before setting SupportsOAuthBrowserFlow. In
src/EventStore.ClusterNode/ui-assets/js/ui-auth.js lines 106-108, update the
flow-start validation to require code_challenge_uri, redirect_uri,
response_type, and scope before proceeding.
In `@src/EventStore.ClusterNode/Program.cs`:
- Line 297: Update the OAuthBrowserFlowService registration in Program so its
HttpClient uses a SocketsHttpHandler configured with an appropriate
PooledConnectionLifetime instead of an unmanaged new HttpClient(). Preserve the
existing OAuth options and TimeProvider.System arguments.
In `@src/EventStore.Core/Authentication/AuthenticationMethodNames.cs`:
- Around line 32-33: Update AuthenticationMethodNames.Normalize to handle a null
method before calling Trim, returning the appropriate normalized fallback
without throwing. Preserve the existing IsLegacyInternal behavior and
normalization for non-null methods.
- Around line 13-19: Update FromOptions to null-coalesce options.Methods to an
empty array before the LINQ Where call, preserving the existing filtering,
normalization, and deduplication behavior when methods are provided.
---
Outside diff comments:
In `@src/EventStore.ClusterNode/Components/Pages/SignIn.razor`:
- Around line 52-88: Move the shared Message display out of the
Authentication.SupportsOAuthBrowserFlow and Authentication.SupportsBasic
sections and render it once above those authentication method blocks. Preserve
the existing conditional visibility and styling, while removing both duplicated
inline Message elements.
---
Nitpick comments:
In `@src/EventStore.Core.Tests/EventStore.Core.Tests.csproj`:
- Line 29: The EventStore.Core.Tests project now depends upward on
EventStore.ClusterNode. Remove the EventStore.ClusterNode project reference from
EventStore.Core.Tests and move the ClusterNode-specific tests, including browser
flow endpoint coverage, into an EventStore.ClusterNode.Tests project.
In `@src/EventStore.Core/Authentication/CompositeAuthenticationProvider.cs`:
- Around line 34-35: Update CompositeAuthenticationProvider.GetPublicProperties
to deduplicate aggregated properties by their key after flattening provider
results. Preserve each key’s first property value and continue ignoring null
property collections, ensuring downstream consumers receive unique keys.
- Around line 67-75: Update SelectScheme to derive the authentication scheme
from AuthenticationRequest or the existing extensible scheme-resolution
mechanism instead of hardcoding Bearer, UserCertificate, and Basic. Preserve the
current built-in routing behavior and fallback while allowing externally
registered authentication schemes to be selected without modifying this method.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6b104ea7-2516-47e1-84e1-ff04932deea7
📒 Files selected for processing (27)
src/Directory.Packages.propssrc/EventStore.ClusterNode/ClusterVNodeHostedService.cssrc/EventStore.ClusterNode/Components/Pages/SignIn.razorsrc/EventStore.ClusterNode/Components/Services/ConfigurationBrowserService.cssrc/EventStore.ClusterNode/Components/Services/OAuthBrowserFlowEndpoints.cssrc/EventStore.ClusterNode/Components/Services/SecurityBrowserService.cssrc/EventStore.ClusterNode/Components/Services/UiCredentialCookie.cssrc/EventStore.ClusterNode/Program.cssrc/EventStore.ClusterNode/ui-assets/js/ui-auth.jssrc/EventStore.Core.Tests/Authentication/CompositeAuthenticationProviderTests.cssrc/EventStore.Core.Tests/Authentication/OAuthAuthenticationProviderTests.cssrc/EventStore.Core.Tests/Authentication/OAuthBrowserFlowServiceTests.cssrc/EventStore.Core.Tests/EventStore.Core.Tests.csprojsrc/EventStore.Core.Tests/Services/Transport/Http/Authentication/authentication_middleware_should.cssrc/EventStore.Core.XUnit.Tests/Authentication/AuthenticationMethodNamesTests.cssrc/EventStore.Core/Authentication/AuthenticationMethodNames.cssrc/EventStore.Core/Authentication/CompositeAuthenticationProvider.cssrc/EventStore.Core/Authentication/CompositeAuthenticationProviderFactory.cssrc/EventStore.Core/Authentication/OAuth/OAuthAuthenticationProvider.cssrc/EventStore.Core/Authentication/OAuth/OAuthAuthenticationProviderFactory.cssrc/EventStore.Core/ClusterVNode.cssrc/EventStore.Core/Configuration/ClusterVNodeOptions.cssrc/EventStore.Core/Configuration/ClusterVNodeOptionsValidator.cssrc/EventStore.Core/EventStore.Core.csprojsrc/EventStore.Core/Services/Transport/Http/AuthenticationMiddleware.cssrc/EventStore.Core/Telemetry/TelemetryService.cssrc/EventStore.Core/Util/Opts.cs
💤 Files with no reviewable changes (1)
- src/EventStore.Core/Util/Opts.cs
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
commented
Jul 15, 2026
yordis
commented
Jul 15, 2026
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/EventStore.Core.Tests/Authentication/CompositeAuthenticationProviderTests.cs (1)
54-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDispose the generated test certificate.
X509Certificate2encapsulates unmanaged resources. To prevent handle leaks and memory exhaustion in the test runner, ensure the certificate is properly disposed after the request is created.♻️ Proposed fix
- var request = HttpAuthenticationRequest.CreateWithValidCertificate(context, "admin", CreateCertificate()); + using var certificate = CreateCertificate(); + var request = HttpAuthenticationRequest.CreateWithValidCertificate(context, "admin", certificate);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/EventStore.Core.Tests/Authentication/CompositeAuthenticationProviderTests.cs` at line 54, Update the test around HttpAuthenticationRequest.CreateWithValidCertificate and CreateCertificate so the generated X509Certificate2 is disposed after the request is created, using scoped disposal while preserving the existing request setup.src/EventStore.Core.Tests/Authentication/OAuthBrowserFlowServiceTests.cs (1)
26-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDispose
OAuthBrowserFlowServiceinstances.The
OAuthBrowserFlowServiceimplementsIDisposable(and manages anHttpClient). To prevent resource exhaustion in test environments, ensure that these instances are cleanly disposed of by making themusingvariables.
src/EventStore.Core.Tests/Authentication/OAuthBrowserFlowServiceTests.cs#L26-L26: change tousing var service = Service(new TokenHandler());src/EventStore.Core.Tests/Authentication/OAuthBrowserFlowServiceTests.cs#L45-L45: change tousing var service = Service(handler);src/EventStore.Core.Tests/Authentication/OAuthBrowserFlowServiceTests.cs#L72-L72: change tousing var service = Service(handler);src/EventStore.Core.Tests/Authentication/OAuthBrowserFlowServiceTests.cs#L93-L93: change tousing var service = Service(handler);src/EventStore.Core.Tests/Authentication/OAuthBrowserFlowServiceTests.cs#L117-L117: change tousing var service = Service(handler);src/EventStore.Core.Tests/Authentication/OAuthBrowserFlowServiceTests.cs#L141-L141: change tousing var service = Service(handler);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/EventStore.Core.Tests/Authentication/OAuthBrowserFlowServiceTests.cs` at line 26, OAuthBrowserFlowService test instances are not being disposed. In src/EventStore.Core.Tests/Authentication/OAuthBrowserFlowServiceTests.cs at lines 26, 45, 72, 93, 117, and 141, declare each Service(...) result as a using variable so every OAuthBrowserFlowService is disposed at the end of its test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@src/EventStore.Core.Tests/Authentication/CompositeAuthenticationProviderTests.cs`:
- Line 54: Update the test around
HttpAuthenticationRequest.CreateWithValidCertificate and CreateCertificate so
the generated X509Certificate2 is disposed after the request is created, using
scoped disposal while preserving the existing request setup.
In `@src/EventStore.Core.Tests/Authentication/OAuthBrowserFlowServiceTests.cs`:
- Line 26: OAuthBrowserFlowService test instances are not being disposed. In
src/EventStore.Core.Tests/Authentication/OAuthBrowserFlowServiceTests.cs at
lines 26, 45, 72, 93, 117, and 141, declare each Service(...) result as a using
variable so every OAuthBrowserFlowService is disposed at the end of its test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dcbda19c-e113-426f-a627-0d31aaf46c7c
📒 Files selected for processing (16)
src/EventStore.ClusterNode/ClusterVNodeHostedService.cssrc/EventStore.ClusterNode/Components/Services/OAuthBrowserFlowEndpoints.cssrc/EventStore.ClusterNode/Components/Services/SecurityBrowserService.cssrc/EventStore.ClusterNode/Program.cssrc/EventStore.ClusterNode/ui-assets/js/ui-auth.jssrc/EventStore.Core.Tests/Authentication/CompositeAuthenticationProviderTests.cssrc/EventStore.Core.Tests/Authentication/OAuthAuthenticationProviderTests.cssrc/EventStore.Core.Tests/Authentication/OAuthBrowserFlowServiceTests.cssrc/EventStore.Core.Tests/Authentication/SecurityBrowserServiceTests.cssrc/EventStore.Core.Tests/EventStore.Core.Tests.csprojsrc/EventStore.Core.XUnit.Tests/Authentication/AuthenticationMethodNamesTests.cssrc/EventStore.Core/Authentication/AuthenticationMethodNames.cssrc/EventStore.Core/Authentication/OAuth/OAuthAuthenticationProvider.cssrc/EventStore.Core/Authentication/UserCertificateAuthenticationProvider.cssrc/EventStore.Core/Authentication/UserCertificateAuthenticationProviderFactory.cssrc/EventStore.Core/Configuration/ClusterVNodeOptions.cs
🚧 Files skipped from review as they are similar to previous changes (8)
- src/EventStore.Core.XUnit.Tests/Authentication/AuthenticationMethodNamesTests.cs
- src/EventStore.ClusterNode/ClusterVNodeHostedService.cs
- src/EventStore.Core/Authentication/AuthenticationMethodNames.cs
- src/EventStore.Core.Tests/EventStore.Core.Tests.csproj
- src/EventStore.ClusterNode/Program.cs
- src/EventStore.Core/Configuration/ClusterVNodeOptions.cs
- src/EventStore.Core/Authentication/OAuth/OAuthAuthenticationProvider.cs
- src/EventStore.ClusterNode/ui-assets/js/ui-auth.js
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/EventStore.Core/Configuration/ClusterVNodeOptionsValidator.cs (1)
153-164: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate default passwords based on password authentication, not the built-in user store.
IncludesBuiltInUserStoreis also true for OAuth-only configurations, so custom default admin/ops passwords are accepted even though password authentication is disabled and those passwords cannot be used. UseIncludesPasswordhere; retainIncludesBuiltInUserStorefor user-management visibility.Proposed fix
- var needsDefaultUsers = AuthenticationMethodNames.IncludesBuiltInUserStore(options.Auth); - if (options.Application.AuthDisabled() || !needsDefaultUsers) + var passwordAuthenticationEnabled = AuthenticationMethodNames.IncludesPassword(options.Auth); + if (options.Application.AuthDisabled() || !passwordAuthenticationEnabled)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/EventStore.Core/Configuration/ClusterVNodeOptionsValidator.cs` around lines 153 - 164, Update the default admin/ops password validation in ClusterVNodeOptionsValidator to determine needsDefaultUsers with AuthenticationMethodNames.IncludesPassword(options.Auth) instead of IncludesBuiltInUserStore. Preserve IncludesBuiltInUserStore for user-management visibility elsewhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/EventStore.Core/Configuration/ClusterVNodeOptionsValidator.cs`:
- Around line 153-164: Update the default admin/ops password validation in
ClusterVNodeOptionsValidator to determine needsDefaultUsers with
AuthenticationMethodNames.IncludesPassword(options.Auth) instead of
IncludesBuiltInUserStore. Preserve IncludesBuiltInUserStore for user-management
visibility elsewhere.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1128c4fa-63ca-470c-89c9-51fae0ec9436
📒 Files selected for processing (11)
src/EventStore.ClusterNode/Components/Services/ConfigurationBrowserService.cssrc/EventStore.ClusterNode/Components/Services/SecurityBrowserService.cssrc/EventStore.ClusterNode/Program.cssrc/EventStore.Core.Tests/Authentication/CompositeAuthenticationProviderTests.cssrc/EventStore.Core.Tests/Authentication/SecurityBrowserServiceTests.cssrc/EventStore.Core.XUnit.Tests/Authentication/AuthenticationMethodNamesTests.cssrc/EventStore.Core.XUnit.Tests/Configuration/ClusterVNodeOptionsValidatorTests.cssrc/EventStore.Core/Authentication/AuthenticationMethodNames.cssrc/EventStore.Core/Authentication/UserCertificateAuthenticationProvider.cssrc/EventStore.Core/ClusterVNode.cssrc/EventStore.Core/Configuration/ClusterVNodeOptionsValidator.cs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/EventStore.Core/Authentication/UserCertificateAuthenticationProvider.cs
- src/EventStore.Core/Authentication/AuthenticationMethodNames.cs
- src/EventStore.ClusterNode/Program.cs
yordis
force-pushed
the
yordis/feat-built-in-oauth-authentication
branch
from
July 15, 2026 10:47
e404de6 to
675962f
Compare
yordis
force-pushed
the
yordis/feat-built-in-oauth-authentication
branch
from
July 15, 2026 10:53
675962f to
0a09f35
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/EventStore.Core.XUnit.Tests/Configuration/ClusterVNodeOptionsTests.cs (1)
289-306: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExpand OAuth browser-sign-in coverage in this test. Add assertions for
AuthorizationEndpoint,TokenEndpoint,ClientId, andScopesso the configuration binding covers the new browser-flow settings too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/EventStore.Core.XUnit.Tests/Configuration/ClusterVNodeOptionsTests.cs` around lines 289 - 306, Extend can_set_oauth_options_from_auth_section by adding in-memory configuration entries for AuthorizationEndpoint, TokenEndpoint, ClientId, and Scopes, then assert the corresponding options.Auth.OAuth properties bind to those values alongside Issuer and Audiences.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/EventStore.Core.XUnit.Tests/Configuration/ClusterVNodeOptionsTests.cs`:
- Around line 289-306: Extend can_set_oauth_options_from_auth_section by adding
in-memory configuration entries for AuthorizationEndpoint, TokenEndpoint,
ClientId, and Scopes, then assert the corresponding options.Auth.OAuth
properties bind to those values alongside Issuer and Audiences.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aeebf333-6cd4-4601-ba8e-74033bb61d34
📒 Files selected for processing (6)
docs/security.mdsrc/EventStore.ClusterNode/Components/Pages/SignIn.razorsrc/EventStore.ClusterNode/Components/Services/OAuthBrowserFlowEndpoints.cssrc/EventStore.Core.Tests/Authentication/OAuthBrowserFlowServiceTests.cssrc/EventStore.Core.XUnit.Tests/Configuration/ClusterVNodeOptionsTests.cssrc/EventStore.Core/Configuration/ClusterVNodeOptions.cs
🚧 Files skipped from review as they are similar to previous changes (4)
- src/EventStore.ClusterNode/Components/Pages/SignIn.razor
- src/EventStore.Core/Configuration/ClusterVNodeOptions.cs
- src/EventStore.Core.Tests/Authentication/OAuthBrowserFlowServiceTests.cs
- src/EventStore.ClusterNode/Components/Services/OAuthBrowserFlowEndpoints.cs
yordis
force-pushed
the
yordis/feat-built-in-oauth-authentication
branch
2 times, most recently
from
July 15, 2026 12:05
c8ac3a5 to
26bfbc7
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f82c4ea. Configure here.
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
force-pushed
the
yordis/feat-built-in-oauth-authentication
branch
from
July 15, 2026 17:29
b0df763 to
ffbe9e8
Compare
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
1 participant
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.