refactor: AuthExtensions to Support Bearer Authentication Validation - BED-6900 #2170
Conversation
mykeelium
added
enhancement
WalkthroughThis PR refactors the authentication system by introducing an Changes
Sequence DiagramsequenceDiagram
participant Client
participant Middleware
participant Authenticator as AuthenticatorBase
participant AuthExt as AuthExtensions
participant JWT as JWT Parser
participant DB as Database
Client->>Middleware: HTTP Request with Bearer Token
Middleware->>Authenticator: ValidateBearerToken(ctx, jwtToken)
Authenticator->>AuthExt: ParseClaimsAndVerifySignature(ctx, jwtToken)
AuthExt->>JWT: Parse & Verify Signature (HS256)
JWT-->>AuthExt: RegisteredClaims (ID, Subject, ExpiresAt, etc.)
AuthExt->>DB: Validate claims (expiration, ownership)
DB-->>AuthExt: Session metadata
AuthExt->>AuthExt: InitContextFromClaims(ctx, claims)
AuthExt-->>Authenticator: auth.Context
Authenticator-->>Middleware: auth.Context
Middleware-->>Client: Authorized Request Proceeds
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–60 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
mykeelium
changed the title
| } | ||
| } | ||
|
|
||
| type LoginRequest struct { |
There was a problem hiding this comment.
Moved to model layer due to a circular dependency with api+mock+test
mykeelium
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/api/src/api/auth.go (1)
230-234: Bug:context.Background().Err()always returns nil.When the context is cancelled, returning
context.Background().Err()instead ofctx.Err()will always returnnil, effectively swallowing the cancellation error.Proposed fix
case <-ctx.Done(): - return context.Background().Err() + return ctx.Err() }
🧹 Nitpick comments (2)
cmd/api/src/api/auth_internal_test.go (1)
500-504: Consider usingerrors.Isfor error comparisons.Using
require.Equalfor error comparison works here since you're comparing the exact same error instance, buterrors.Isis the idiomatic approach for error comparison and handles wrapped errors correctly.require.ErrorIs(t, err, database.ErrNotFound)cmd/api/src/api/auth.go (1)
518-535: Review the Owner nil-check logic for bearer token validation.The logic at lines 524-531 uses
authContext.Owner == nilto determine if the token was created by BloodHound (internal session) versus an external provider. While this works given the currentInitContextFromClaimsimplementation returns an empty context, it relies on an implicit contract.Consider adding a comment or using a more explicit mechanism (e.g., a flag in
auth.Context) to make this distinction clearer for future maintainers.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
cmd/api/src/api/auth.go(10 hunks)cmd/api/src/api/auth_internal_test.go(13 hunks)cmd/api/src/api/middleware/auth.go(3 hunks)cmd/api/src/api/mocks/authExtensions.go(1 hunks)cmd/api/src/api/mocks/authenticator.go(2 hunks)cmd/api/src/api/v2/apitest/test.go(2 hunks)cmd/api/src/api/v2/auth/login.go(4 hunks)cmd/api/src/api/v2/auth/login_internal_test.go(2 hunks)cmd/api/src/api/v2/auth/login_test.go(1 hunks)cmd/api/src/api/v2/auth/saml_internal_test.go(2 hunks)cmd/api/src/auth/model.go(0 hunks)cmd/api/src/config/config.go(1 hunks)cmd/api/src/database/auth.go(0 hunks)cmd/api/src/model/auth.go(1 hunks)cmd/api/src/services/entrypoint.go(1 hunks)
💤 Files with no reviewable changes (2)
- cmd/api/src/database/auth.go
- cmd/api/src/auth/model.go
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-07-22T20:30:34.839Z
Learnt from: LawsonWillard
Repo: SpecterOps/BloodHound PR: 1700
File: cmd/api/src/api/v2/saved_queries_test.go:3182-3182
Timestamp: 2025-07-22T20:30:34.839Z
Learning: In Go table-driven tests in cmd/api/src/api/v2/saved_queries_test.go, subtest parallelization with t.Parallel() is acceptable when tests are self-contained, each creating their own mock controller (gomock.NewController(t)) and having isolated mock expectations without shared state between subtests.
Applied to files:
cmd/api/src/api/v2/auth/login_test.gocmd/api/src/api/v2/auth/saml_internal_test.gocmd/api/src/api/auth_internal_test.gocmd/api/src/api/v2/apitest/test.go
📚 Learning: 2025-05-29T18:24:23.227Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1509
File: packages/go/stbernard/command/license/internal/cmd.go:49-51
Timestamp: 2025-05-29T18:24:23.227Z
Learning: In the stbernard package, use slog for all logging prints instead of fmt.Printf/fmt.Fprintf to maintain consistency with the codebase's logging standards.
Applied to files:
cmd/api/src/api/middleware/auth.go
📚 Learning: 2025-06-25T17:52:33.291Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1606
File: cmd/api/src/analysis/azure/post.go:33-35
Timestamp: 2025-06-25T17:52:33.291Z
Learning: In BloodHound Go code, prefer using explicit slog type functions like slog.Any(), slog.String(), slog.Int(), etc. over simple key-value pairs for structured logging. This provides better type safety and makes key-value pairs more visually distinct. For error types, use slog.Any("key", err) or slog.String("key", err.Error()).
Applied to files:
cmd/api/src/api/middleware/auth.go
📚 Learning: 2025-08-28T16:43:43.961Z
Learnt from: mvlipka
Repo: SpecterOps/BloodHound PR: 1784
File: packages/go/openapi/doc/openapi.json:18008-18029
Timestamp: 2025-08-28T16:43:43.961Z
Learning: In SpecterOps/BloodHound, packages/go/openapi/doc/openapi.json is generated from YAML under packages/go/openapi/src/schemas; edits must be made to the YAML and then the spec regenerated.
Applied to files:
cmd/api/src/api/v2/apitest/test.go
🧬 Code graph analysis (11)
cmd/api/src/api/v2/auth/login_test.go (2)
cmd/api/src/model/auth.go (3)
LoginRequest(601-606)LoginDetails(608-611)User(431-451)cmd/api/src/auth/model.go (1)
ProviderTypeSecret(35-35)
cmd/api/src/api/v2/auth/saml_internal_test.go (3)
cmd/api/src/api/auth.go (1)
NewAuthenticator(143-151)cmd/api/src/config/config.go (1)
Configuration(137-170)cmd/api/src/api/mocks/authExtensions.go (1)
NewMockAuthExtensions(51-55)
cmd/api/src/model/auth.go (2)
packages/go/graphschema/azure/azure.go (1)
User(41-41)packages/go/graphschema/ad/ad.go (1)
User(29-29)
cmd/api/src/api/mocks/authenticator.go (1)
cmd/api/src/model/auth.go (2)
LoginRequest(601-606)LoginDetails(608-611)
cmd/api/src/api/mocks/authExtensions.go (1)
cmd/api/src/model/auth.go (1)
AuthToken(147-156)
cmd/api/src/api/middleware/auth.go (4)
cmd/api/src/auth/model.go (1)
Context(160-164)cmd/api/src/api/marshalling.go (1)
WriteErrorResponse(77-85)cmd/api/src/api/error.go (1)
BuildErrorResponse(134-145)cmd/api/src/ctx/ctx.go (1)
Get(75-85)
cmd/api/src/api/auth.go (3)
cmd/api/src/model/auth.go (4)
AuthToken(147-156)UserSession(580-589)AuthSecret(253-262)User(431-451)cmd/api/src/config/config.go (1)
Configuration(137-170)cmd/api/src/database/db.go (2)
Database(71-190)ErrNotFound(41-41)
cmd/api/src/api/auth_internal_test.go (6)
cmd/api/src/model/auth.go (2)
User(431-451)LoginRequest(601-606)cmd/api/src/auth/model.go (1)
Context(160-164)cmd/api/src/api/auth.go (2)
AuthenticatorBase(135-141)ErrInvalidAuth(51-51)cmd/api/src/database/mocks/db.go (1)
NewMockDatabase(55-59)cmd/api/src/api/mocks/authExtensions.go (2)
NewMockAuthExtensions(51-55)MockAuthExtensions(39-43)cmd/api/src/database/db.go (1)
ErrNotFound(41-41)
cmd/api/src/api/v2/apitest/test.go (4)
cmd/api/src/api/v2/auth/auth.go (1)
NewManagementResource(69-81)cmd/api/src/auth/model.go (1)
NewAuthorizer(93-95)cmd/api/src/api/auth.go (1)
NewAuthenticator(143-151)cmd/api/src/api/mocks/authExtensions.go (1)
NewMockAuthExtensions(51-55)
cmd/api/src/services/entrypoint.go (1)
cmd/api/src/api/auth.go (2)
NewAuthenticator(143-151)NewAuthExtensions(81-86)
cmd/api/src/api/v2/auth/login.go (2)
cmd/api/src/model/auth.go (2)
LoginRequest(601-606)LoginResponse(613-617)cmd/api/src/api/marshalling.go (1)
WriteBasicResponse(90-99)
🔇 Additional comments (25)
cmd/api/src/config/config.go (1)
368-370: LGTM!The
GetRootURLHost()method provides a clean utility to format the root URL as a host string with scheme and hostname. The implementation is straightforward and correct.cmd/api/src/api/v2/apitest/test.go (2)
25-25: LGTM!The import alias
apimockshelps clearly distinguish API mocks from database mocks, improving code readability.
46-46: LGTM!The update to use
apimocks.NewMockAuthExtensions(mockCtrl)correctly aligns with the new AuthExtensions interface introduced in this PR.cmd/api/src/api/middleware/auth.go (3)
21-21: LGTM!The addition of
log/slogsupports structured logging in the bearer token validation path.
71-77: LGTM!The updated bearer token validation flow correctly uses the new
ValidateBearerTokenmethod and includes proper error logging with context. The user-facing error message is appropriately generic to avoid leaking implementation details.
205-208: LGTM!Consolidating the constants into a single block improves code organization without changing behavior.
cmd/api/src/api/v2/auth/login_test.go (1)
60-77: LGTM!The migration from
api.LoginRequest/api.LoginDetailstomodel.LoginRequest/model.LoginDetailsis correctly applied throughout the test. The test logic remains intact, and mock expectations properly use the new types.cmd/api/src/services/entrypoint.go (1)
118-118: LGTM!The migration to
api.NewAuthExtensions(cfg, connections.RDMS)correctly implements the new authentication architecture, moving context initialization from the database layer to the API layer for better extensibility.cmd/api/src/api/v2/auth/login.go (2)
32-32: LGTM!The addition of the model package import supports the migration of login-related types to a centralized location.
50-72: LGTM!The migration of login types from
api.LoginRequest/api.LoginResponsetomodel.LoginRequest/model.LoginResponseis applied consistently throughout the login flow. The function logic remains unchanged, with only type references updated.cmd/api/src/api/v2/auth/saml_internal_test.go (2)
30-30: LGTM!The import alias for API mocks improves code clarity when working with multiple mock packages.
57-57: LGTM!The test correctly uses
apimocks.NewMockAuthExtensions(mockCtrl)to align with the new AuthExtensions-based authentication flow.cmd/api/src/model/auth.go (1)
601-617: LGTM!The new login-related types are well-structured and follow Go conventions:
LoginRequestproperly usesomitemptyfor optional fields (Secret, OTP)LoginDetailsserves as an internal type without JSON tagsLoginResponseprovides the public API response structureCentralizing these types in the model package promotes reusability and consistency across authentication flows.
cmd/api/src/api/mocks/authenticator.go (1)
17-18: LGTM - Generated mock correctly reflects interface changes.The MockGen-generated code properly implements the updated
Authenticatorinterface withmodel.LoginRequest/model.LoginDetailstypes and the newValidateBearerTokenmethod.cmd/api/src/api/v2/auth/login_internal_test.go (2)
62-90: LGTM - Consistent type migration to model package.The test correctly updates all
LoginRequestandLoginDetailsreferences from theapipackage to themodelpackage, aligning with the relocated types. Mock expectations are properly configured for the updated interface.
205-212: LGTM - TestLoginSuccess properly updated.The success test case correctly uses
model.LoginRequestas input and expectsmodel.LoginDetailswith the appropriatemodel.AuthSecretstructure in the return value.cmd/api/src/api/mocks/authExtensions.go (1)
17-22: LGTM - New AuthExtensions mock correctly generated.The MockGen-generated
MockAuthExtensionsproperly implements the newAuthExtensionsinterface with all three methods:InitContextFromClaims,InitContextFromToken, andParseClaimsAndVerifySignature.cmd/api/src/api/auth_internal_test.go (3)
65-80: LGTM - setupRequest properly updated.The helper function correctly returns
model.LoginRequestto align with the relocated type.
134-146: LGTM - NewTestAuthenticator uses AuthenticatorBase with authExtensions.The test helper correctly instantiates
AuthenticatorBasewith the newauthExtensionsfield usingapimocks.NewMockAuthExtensions.
446-643: LGTM - Comprehensive test coverage for authExtensions.The new
Test_authExtensionssuite provides good coverage for all three methods of theauthExtensionsimplementation:
InitContextFromClaimsdefault behaviorInitContextFromTokenwith valid/invalid UserID and error scenariosParseClaimsAndVerifySignaturewith valid tokens, expired tokens, and invalid signaturesThe parallel test execution pattern is correctly implemented with each subtest creating its own mock controller. Based on learnings, this is an acceptable pattern for self-contained table-driven tests.
cmd/api/src/api/auth.go (5)
70-86: LGTM - Clean AuthExtensions interface and implementation.The new
AuthExtensionsinterface provides a clear extension point for token/claims handling. The constructor properly initializes the struct with configuration and database dependencies.
88-104: Note:InitContextFromClaimsreturns empty context by design.The
InitContextFromClaimsmethod returns an emptyauth.Contextwhich is used as an extension point. The calling code inValidateBearerToken(lines 524-531) checks fornilOwner to determine if the token was created by BloodHound and falls back to session validation.This design is intentional for AD FS and other external identity provider extensions, where custom implementations can populate the Owner from external claims.
106-122: LGTM - Proper JWT signature verification with error handling.The
ParseClaimsAndVerifySignaturemethod correctly:
- Parses JWT with claims using the configured signing key
- Returns
ErrInvalidAuthfor signature errors- Returns the claims even on validation errors (useful for expired token handling)
537-584: LGTM - ValidateSession correctly handles session validation.The refactored
ValidateSessionmethod properly:
- Parses the claims ID from string to int64
- Retrieves and validates the session from the database
- Handles expired sessions, missing auth secrets, and EULA acceptance
- Applies appropriate permission overrides for expired secrets
503-514: LGTM - JWT claims migration to RegisteredClaims.The session JWT creation correctly uses
jwt.RegisteredClaimswith proper field mapping:
Issuer: Root URL hostID: Session ID as stringSubject: User IDIssuedAt/ExpiresAt: Properjwt.NumericDateformatting
mykeelium
changed the title
Description
The changes in this PR update the Authentication Logic to allow the extension of logic while validating a bearer token in the AuthMiddleware. In addition, some previously extended logic was unified into a struct that is now at the API layer rather than the Database layer.
Motivation and Context
Resolves BED-6900
How Has This Been Tested?
This has been tested by creating an instance and extending this logic here to be able to test the extended functionality in order to authenticate using AD FS.
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.