feat(ext): support authenticated principals and config - #657
feat(ext): support authenticated principals and config#657SantiagoDePolonia wants to merge 11 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThe change adds opaque extension configuration, one-time setup, pluggable request authenticators, authentication context propagation, audit persistence, credential URL redaction, and dashboard support for external authentication. ChangesExtension configuration and setup
Authenticator API and wiring
Request authentication
Principal ID audit persistence
Credential redaction and dashboard metadata
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Request
participant AuthMiddlewareWithRequestAuthenticators
participant RequestAuthenticator
participant AuthenticationContext
participant AuthenticationEventRecorder
participant AuditLogStore
Request->>AuthMiddlewareWithRequestAuthenticators: Request without explicit credentials
AuthMiddlewareWithRequestAuthenticators->>RequestAuthenticator: AuthenticateRequest(request)
RequestAuthenticator-->>AuthMiddlewareWithRequestAuthenticators: Authentication
AuthMiddlewareWithRequestAuthenticators->>AuthenticationContext: WithAuthentication(authentication)
AuthenticationContext->>AuthenticationEventRecorder: Authentication event metadata
AuthenticationEventRecorder->>AuditLogStore: Durable audit entry
AuditLogStore-->>Request: Authenticated request continues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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 `@config/config.example.yaml`:
- Around line 540-545: Update the commented extensions configuration example to
explicitly state that extensions is absent by default and operators should add
an extension section only when required by their custom distribution, while
preserving the existing explanation of extension-owned validation.
In `@internal/auditlog/enrich.go`:
- Line 308: Make authentication-method handling provider-neutral: in
internal/auditlog/enrich.go lines 308-308, accept and validate opaque extension
methods without enumerating the SSO-specific value; in
internal/auditlog/auditlog.go lines 39-39, remove the SSO-specific Core constant
or replace it with a generic extension-auth method, ensuring other extension
providers remain supported.
In `@internal/auditlog/middleware.go`:
- Around line 211-213: Update applyAuthentication to trim
authentication.PrincipalID and assign it to entry.PrincipalID only when the
trimmed value is non-empty, preserving any previously populated ID; add a
regression test covering an existing ID with an empty or whitespace-only context
principal.
In `@internal/auditlog/store_sql_test.go`:
- Around line 62-78: Update the round-trip test around the direct audit_logs
query to call SQLReader.GetLogs and inspect the returned log entry, asserting
PrincipalID along with the existing relevant fields. Retain only any direct
database checks needed for writer persistence, ensuring the test exercises
scanSQLLogEntry’s positional column contract.
In `@internal/server/auth_test.go`:
- Around line 264-316: Extend the auth middleware tests around
AuthMiddlewareWithRequestAuthenticators with table-driven cases covering
authenticator errors, nil results, multiple authenticators, empty PrincipalID,
invalid UserPath, and an extension identity lacking DashboardAccess on an admin
route. Assert the expected status and response behavior for each case, including
that error responses do not expose extension error text, while preserving the
existing success and explicit master-key precedence coverage.
In `@internal/server/auth.go`:
- Around line 146-148: Update AdminAccessMiddleware’s dashboard-access denial
response for extension identities to use an identity-neutral message such as
“identity does not have dashboard access” instead of referring specifically to
an API key, while preserving the existing authorization behavior.
- Around line 117-142: The applyExtensionAuthResult function currently
normalizes identity fields only locally while storing the original result in
context. Trim result.PrincipalID and copy result before setting its PrincipalID
and normalized UserPath, then pass that copy to ext.WithAuthentication; use the
normalized principal value for the corresponding audit enrichment as well.
- Around line 98-100: The AuthenticateRequest error path in the request handler
exposes raw extension errors through authenticationErrorWithAudit; replace
authFailureMessage(err) with the fixed public message "authentication failed"
and retain only a sanitized error code in the audit data. Ensure API keys,
authorization headers, secrets, and upstream diagnostics are not returned or
logged.
In `@internal/server/http.go`:
- Around line 353-355: Update the authSkipPaths construction in
internal/server/http.go so "/admin/*" is added only when cfg.MasterKey is empty
and no RequestAuthenticators are configured; keep extension-authenticated admin
requests flowing through AuthMiddlewareWithRequestAuthenticators. Add an
integration test covering an extension identity with DashboardAccess: false and
verify admin REST access is denied unless a master key or authorized managed key
is present.
🪄 Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: 55e5e61f-9cce-4463-8170-7fed2b9b60d7
📒 Files selected for processing (23)
config/config.example.yamlconfig/config.goconfig/config_test.goext/auth.goext/registry.goext/registry_test.gointernal/app/app.gointernal/app/app_test.gointernal/auditlog/auditlog.gointernal/auditlog/enrich.gointernal/auditlog/middleware.gointernal/auditlog/middleware_test.gointernal/auditlog/reader_mongodb.gointernal/auditlog/reader_sql.gointernal/auditlog/store_mongodb.gointernal/auditlog/store_sql.gointernal/auditlog/store_sql_test.gointernal/auditlog/stream_wrapper.gointernal/server/auth.gointernal/server/auth_test.gointernal/server/http.gorun/run.gorun/run_test.go
Confidence Score: 3/5Not safe to merge until explicit credential authentication removes any pre-existing extension identity from the request context. A reproduced security-sensitive authorization-boundary failure remains in explicit credential handling. Files Needing Attention: internal/server/auth.go
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Addressed the review audit in c95ccaf:
Validation: go test ./...; go test -race ./config ./ext ./internal/auditlog ./internal/server; all repository pre-commit hooks passed. The docstring warning was reviewed separately: all new exported APIs and the non-obvious normalization helper are documented; adding comments to every private test/helper solely to satisfy the heuristic would add noise. |
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)
internal/server/auth.go (1)
41-41: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle nil request authenticators consistently.
A slice containing only nil authenticators has a non-zero length. The request loop skips those values. The current checks then register authentication, disable admin recovery, and reject requests with missing credentials although no authenticator can authenticate them.
internal/server/auth.go#L41-L41: determine whether at least one non-nil request authenticator exists before disabling the no-authentication path.internal/server/http.go#L247-L247: use the same non-nil check before disabling the admin recovery bypass.internal/server/http.go#L353-L355: use the same non-nil check before registering authentication middleware.Proposed fix
+func hasRequestAuthenticator(authenticators []ext.RequestAuthenticator) bool { + for _, authenticator := range authenticators { + if authenticator != nil { + return true + } + } + return false +}Replace each
len(...RequestAuthenticators) == 0or> 0presence check with this helper. Add a regression test for[]ext.RequestAuthenticator{nil}.🤖 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 `@internal/server/auth.go` at line 41, Replace request-authenticator length checks with a shared non-nil presence check so slices containing only nil authenticators are treated as having no usable authenticator. Update the checks at internal/server/auth.go:41, internal/server/http.go:247, and internal/server/http.go:353-355, preserving the existing no-authentication, admin-recovery, and middleware behavior for genuinely usable authenticators. Add a regression test covering []ext.RequestAuthenticator{nil}.
🤖 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 `@internal/server/auth.go`:
- Line 41: Replace request-authenticator length checks with a shared non-nil
presence check so slices containing only nil authenticators are treated as
having no usable authenticator. Update the checks at internal/server/auth.go:41,
internal/server/http.go:247, and internal/server/http.go:353-355, preserving the
existing no-authentication, admin-recovery, and middleware behavior for
genuinely usable authenticators. Add a regression test covering
[]ext.RequestAuthenticator{nil}.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 01da5c17-f22e-4d26-ad46-599118bbfc3b
📒 Files selected for processing (12)
config/config.example.yamlext/auth.gointernal/auditlog/auditlog.gointernal/auditlog/enrich.gointernal/auditlog/middleware.gointernal/auditlog/middleware_auth_method_test.gointernal/auditlog/middleware_test.gointernal/auditlog/store_sql_test.gointernal/server/admin_access_test.gointernal/server/auth.gointernal/server/auth_test.gointernal/server/http.go
|
Incremental CodeRabbit finding addressed in 63a59af: request-authenticator presence now ignores both nil interfaces and typed-nil implementations consistently in the no-auth path, admin recovery decision, middleware registration, and request loop. Added middleware and full admin-server regression tests for both nil forms. Pro remains green without a local workspace against the refreshed Core pin. |
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
Codecov patch-coverage feedback addressed in 55f9a3e with behavioral tests for the meaningful uncovered paths: public authentication context round trips and defensive reads, principal-ID enrichment, and absent extension configuration. Local package coverage moved from 0% to 100% for ext/auth.go, from 0% to 100% for EnrichEntryWithPrincipalID, and DecodeExtension from 78.6% to 92.9%. Full tests and every pre-commit hook pass. The separate Codecov App installation warning is repository/organization configuration rather than a code change. |
|
The completed upload is now reflected in the Codecov API: patch coverage is 91.43% (160 hits, 11 misses, 4 partials), up from 78.86%. The older bot comment remains stale because this repository is using tokenless uploads without the Codecov App; its own warning notes that comments may not be reliably updated. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/dashboard/src/lib/api/client.js (1)
31-39: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not apply metadata from stale responses.
Line 34 updates external authentication state before the generation check. If the API key changes while a request is in flight, an older response can replace the current login URL, logout URL, or displayed user. Compute
stalebefore observing the response and skip observation when it is stale.Proposed fix
async function request(path, options, { label = path, parse = true } = {}) { const generation = auth.generation; const res = await apiFetch(path, options); - auth.observeResponse(res); + const stale = generation < auth.generation; + if (!stale) { + auth.observeResponse(res); + } if (res.status === 401) { auth.handleUnauthorized(generation); return { ok: false, stale: generation < auth.generation, status: 401, data: null, res }; } - const stale = generation < auth.generation;🤖 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 `@web/dashboard/src/lib/api/client.js` around lines 31 - 39, Update request so it computes the stale flag immediately after apiFetch and before auth.observeResponse. Only call auth.observeResponse(res) when the response is not stale, while preserving the existing 401 handling and stale response metadata.
🤖 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 `@internal/auditlog/authentication_events.go`:
- Around line 67-75: Sanitize the path before storing it in the LogEntry
construction by applying core.RedactSensitiveURLQuery to the trimmed event.Path
value. Update the relevant authentication event test to cover a callback URI
containing sensitive query parameters such as code, state, or id_token and
verify the stored Path contains redacted values.
In `@web/dashboard/src/lib/components/organisms/Sidebar.svelte`:
- Around line 111-126: Update the conditional in the Sidebar external-auth
section to render whenever auth.externalLogoutURL is available, and make the
external-auth-user row conditional on auth.externalUser. Keep the logout link
and its gomodelPath(auth.externalLogoutURL) behavior unchanged.
---
Outside diff comments:
In `@web/dashboard/src/lib/api/client.js`:
- Around line 31-39: Update request so it computes the stale flag immediately
after apiFetch and before auth.observeResponse. Only call
auth.observeResponse(res) when the response is not stale, while preserving the
existing 401 handling and stale response metadata.
🪄 Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: 4c99fb0b-54b0-4aad-a0e6-17f34d23f9ce
⛔ Files ignored due to path filters (5)
internal/admin/dashboard/static/dist/assets/index-BFUCi3g4.jsis excluded by!**/dist/**internal/admin/dashboard/static/dist/assets/index-CT5lg2co.cssis excluded by!**/dist/**internal/admin/dashboard/static/dist/assets/index-Cz8GQupq.cssis excluded by!**/dist/**internal/admin/dashboard/static/dist/assets/index-nQCIlv1m.jsis excluded by!**/dist/**internal/admin/dashboard/static/dist/index.htmlis excluded by!**/dist/**
📒 Files selected for processing (18)
ext/auth.gointernal/app/app.gointernal/app/app_test.gointernal/auditlog/auditlog.gointernal/auditlog/auditlog_test.gointernal/auditlog/authentication_events.gointernal/auditlog/authentication_events_test.gointernal/auditlog/stream_wrapper.gointernal/core/url_redaction.gointernal/server/http.gointernal/server/request_log.gointernal/server/request_log_test.goweb/dashboard/src/lib/api/client.jsweb/dashboard/src/lib/components/organisms/AuthDialog.svelteweb/dashboard/src/lib/components/organisms/Sidebar.svelteweb/dashboard/src/lib/stores/auth.svelte.jsweb/dashboard/src/lib/stores/external-auth.jsweb/dashboard/tests/external-auth.test.js
…rincipals # Conflicts: # internal/admin/dashboard/static/dist/assets/index-_N2yDOD9.css # internal/admin/dashboard/static/dist/index.html
|
Merged latest main in d3cc163 and addressed all findings from the latest review. In addition to the two inline fixes, stale API responses can no longer overwrite external login, logout, or user metadata from a newer authentication generation. Rebuilt the embedded dashboard bundle. Validation: full Go suite, 474 dashboard tests, Svelte check, production dashboard build, repository pre-commit race suite, performance guard, fix-check, and lint. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Final SSO hardening update:
Validation: full |
|
Post-review verification: reran the deployed Cognito browser regression against the final Core head |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@internal/server/auth.go`:
- Around line 66-71: Update the explicit-credential branch in
AuthMiddlewareWithRequestAuthenticators to reset
ext.AuthenticationFromContext(c.Request().Context()) to a cleared identity, and
clear or override the inherited core.UserPathFromContext context value before
master-key or managed-key validation continues downstream. Keep the existing
setAuthenticationUserHeader call while ensuring all inherited authentication
context is removed.
In `@web/dashboard/src/lib/stores/external-auth.js`:
- Around line 18-21: Update the login URL construction around returnTo so any
fragment in loginURL is separated before adding the return_to query parameter,
then reattach the unchanged fragment after the query string. Preserve existing
query-separator and encoding behavior, and add a regression test covering
/g/sso/login#fragment.
🪄 Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: 114ee09a-5c80-4d23-ab92-135389e6c452
⛔ Files ignored due to path filters (3)
internal/admin/dashboard/static/dist/assets/index-DM1VKejK.cssis excluded by!**/dist/**internal/admin/dashboard/static/dist/assets/index-nqGolcSX.jsis excluded by!**/dist/**internal/admin/dashboard/static/dist/index.htmlis excluded by!**/dist/**
📒 Files selected for processing (10)
internal/auditlog/authentication_events.gointernal/auditlog/authentication_events_test.gointernal/server/auth.gointernal/server/auth_test.goweb/dashboard/src/lib/api/client.jsweb/dashboard/src/lib/components/organisms/AuthDialog.svelteweb/dashboard/src/lib/components/organisms/Sidebar.svelteweb/dashboard/src/lib/stores/auth.svelte.jsweb/dashboard/src/lib/stores/external-auth.jsweb/dashboard/tests/external-auth.test.js
|
Review follow-up in 2e82412: recorder rebinding now occurs only after the replacement App has completed every fallible construction step; a late-failure lifecycle regression proves the old recorder is unchanged. Explicit credentials now mask both inherited extension authentication and its effective user-path override before Core validates the selected credential. External login deep links insert return_to before URL fragments, with query/fragment regression coverage. The dashboard production bundle was rebuilt. Validation: go test ./..., focused race tests, golangci-lint, 476 dashboard tests, Svelte checks, production build, and all pre-commit hooks. |
Summary
SSO and OIDC remain entirely in Pro; Core only provides protocol-neutral extension seams, UI discovery, durable audit integration, and generic sensitive-query redaction.
Validation
Summary by CodeRabbit