Conversation
…edge_kinds -> schema_relationship_kinds
# Conflicts: # cmd/api/src/database/migration/migrations/v8.6.0.sql # cmd/api/src/database/upsert_schema_extension_integration_test.go # cmd/api/src/model/graphschema.go # cmd/api/src/services/opengraphschema/extension.go # cmd/api/src/services/opengraphschema/extension_test.go # cmd/api/src/services/opengraphschema/mocks/opengraphschema.go # cmd/api/src/services/opengraphschema/opengraphschema.go
# Conflicts: # cmd/api/src/database/migration/migrations/v8.6.0.sql
# Conflicts: # cmd/api/src/model/graphschema.go
Contributor
📝 WalkthroughWalkthroughAdds a new extension ID URI constant, a DELETE /api/v2/extensions/{extension_id} API route and handler (feature-flag + auth), service and repository DeleteExtension wiring, DB transactional deletion with built-in protection and audit logging, updated mocks/tests, and normalized "id is malformed" error text. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as HTTP Client
participant Handler as DeleteExtension Handler
participant Service as OpenGraphSchemaService
participant Repo as OpenGraphSchemaRepository
participant DB as Database
Client->>Handler: DELETE /api/v2/extensions/{extension_id}
Handler->>Handler: Check feature flag & RequireAuth (admin)
Handler->>Handler: Parse extension_id
Handler->>Service: DeleteExtension(ctx, extensionID)
Service->>Repo: DeleteGraphSchemaExtension(ctx, extensionID)
Repo->>DB: SELECT is_builtin FROM extensions WHERE id=...
alt is_builtin = true
DB-->>Repo: {is_builtin: true}
Repo-->>Service: error (built-in)
Service-->>Handler: wrapped error
Handler-->>Client: HTTP 400/Conflict (error)
else is_builtin = false
DB-->>Repo: {is_builtin: false}
Repo->>DB: DELETE FROM extensions WHERE id=...
DB-->>Repo: rows affected
Repo-->>Service: nil
Service->>Repo: RefreshKinds()
Repo-->>Service: nil
Service-->>Handler: nil
Handler-->>Client: HTTP 204 No Content
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
coderabbitai
Bot
added
api
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@cmd/api/src/api/v2/opengraphschema.go`:
- Around line 305-310: The 500 path in the DeleteExtension handler leaks
internal error details by interpolating err into the response; update the else
branch that calls api.WriteErrorResponse / api.BuildErrorResponse to use the
generic api.ErrorResponseDetailsInternalServerError (the same pattern used in
OpenGraphSchemaIngest) instead of fmt.Sprintf("error deleting graph schema
extension: %v", err), leaving the database.ErrNotFound branch unchanged and
still returning 404 for s.OpenGraphSchemaService.DeleteExtension.
In `@cmd/api/src/services/opengraphschema/extension.go`:
- Around line 162-168: DeleteExtension currently removes the DB entry via
openGraphSchemaRepository.DeleteGraphSchemaExtension but does not refresh the
in-memory kinds map, risking stale schema state; update the method (rename
receiver from o to s) to call the service's kinds refresh routine immediately
after a successful delete (e.g., call s.refreshKinds() or the existing
reload/refresh method that updates the kinds map) and ensure any local variable
declarations are grouped in a var (...) block at the top of the function per
guidelines.
🧹 Nitpick comments (2)
cmd/api/src/database/graphschema.go (1)
208-231: Consider wrapping the SELECT + DELETE in a transaction for atomicity.The existence/built-in check (Line 215) and the actual deletion (Line 227) are two separate queries without transactional protection. A concurrent request could delete the same extension between the two queries, causing the DELETE to return
ErrNotFoundeven though the first check passed. The impact is benign (the caller sees "not found"), but wrapping both in a transaction would make the behavior more predictable and consistent.Also, given that
CreateGraphSchemaExtensionusesAuditableTransactionfor audit logging, consider whether deleting an extension should also be audited—extension deletion is a destructive, admin-only action.cmd/api/src/api/v2/opengraphschema.go (1)
303-304: Inconsistent use ofrequest.Context()vs the localctxvariable.Line 304 calls
request.Context()directly, while every other branch in this function (and thectxdeclaration at line 294) uses the localctx. Usectxhere for consistency.Proposed fix
} else if extID, err := strconv.ParseInt(extensionID, 10, 32); err != nil { - api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusBadRequest, api.ErrorResponseDetailsIDMalformed, request), response) + api.WriteErrorResponse(ctx, api.BuildErrorResponse(http.StatusBadRequest, api.ErrorResponseDetailsIDMalformed, request), response)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@cmd/api/src/api/v2/opengraphschema.go`:
- Around line 305-310: The delete handler currently only checks for
database.ErrNotFound and returns 500 for built-in-extension errors; update the
error branch that handles s.OpenGraphSchemaService.DeleteExtension to also
detect errors.Is(err, model.ErrGraphExtensionBuiltIn) and return a 400 using
api.WriteErrorResponse(api.BuildErrorResponse(http.StatusBadRequest, ...)),
mirroring the upsert handler behavior; ensure you import/reference
model.ErrGraphExtensionBuiltIn (and keep the existing database.ErrNotFound
check) so attempts to delete built-in extensions return a 400 Bad Request
instead of 500.
In `@cmd/api/src/database/graphschema.go`:
- Around line 221-243: The SELECT and DELETE inside the AuditableTransaction
callback are using s.db.WithContext(ctx) and thus bypass the transaction; change
those calls to use the transaction variable (tx) instead — e.g. replace
s.db.WithContext(ctx).Raw(...) with tx.WithContext(ctx).Raw(fmt.Sprintf(`SELECT
is_builtin FROM %s WHERE id = ?`, schemaExtension.TableName()), extensionId) and
replace s.db.WithContext(ctx).Exec(...) with
tx.WithContext(ctx).Exec(fmt.Sprintf(`DELETE FROM %s WHERE id = ?`,
schemaExtension.TableName()), extensionId), keeping the existing CheckError,
ErrNotFound and model.ErrGraphExtensionBuiltIn handling so the SELECT,
is_builtin check and DELETE all participate in AuditableTransaction.
| } | ||
|
|
||
| api.WriteJSONResponse(ctx, ExtensionsResponse{Extensions: extensionsResponse}, http.StatusOK, response) | ||
| api.WriteBasicResponse(ctx, ExtensionsResponse{Extensions: extensionsResponse}, http.StatusOK, response) |
brandonshearin
approved these changes
Feb 10, 2026
Contributor
brandonshearin
left a comment
brandonshearin
left a comment
There was a problem hiding this comment.
nice-- well tested as always.
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
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.
Description
Delete Extension Endpoint
Removal deletes the extension and removes all schema components tied to that extension:
Since we have cascade deletes, this functionality works and it has been validated in the cascade delete integration test
Instead of abstracting that we cannot delete builtin extensions, this was moved back into the database method: https://github.com/SpecterOps/BloodHound/pull/2339/changes#diff-afc4fdb9518a128b497afb174e52ee860b5f75dd2b82d84db6ef76b3e1da701cR215
Motivation and Context
Resolves BED-6898
How Has This Been Tested?
Unit & integration tests
Manual Testing Steps performed:
truecurl --request PUT \ --url http://bhe.localhost/api/v2/extensions \ --header 'authorization: Bearer {{auth-token}}' \ --header 'content-type: application/json' \ --data '{ "schema": { "name": "AzureHound", "display_name": "AzureHound", "version": "1.0.0", "namespace": "AZTEST" }, "node_kinds": [ { "name": "AZTEST_Tenant", "display_name": "Azure Tenant", "description": "An Azure tenant environment", "is_display_kind": true, "icon": "cloud", "color": "0xFF00FF" }, { "name": "AZTEST_Device", "display_name": "Azure Device", "description": "An Azure device", "is_display_kind": true, "icon": "desktop", "color": "0x0000FF" }, { "name": "AZTEST_User", "display_name": "Azure User", "description": "An Azure user account", "is_display_kind": true, "icon": "user", "color": "0x00FF00" }, { "name": "AZTEST_Group", "display_name": "Azure Group", "description": "An Azure security or distribution group", "is_display_kind": true, "icon": "group", "color": "0xFF0000" } ], "relationship_kinds": [ { "name": "AZTEST_MemberOf", "description": "User or computer is a member of a group", "is_traversable": false }, { "name": "AZTEST_HasSession", "description": "User has an active session on a device", "is_traversable": true } ], "environments": [ { "environment_kind": "AZTEST_Tenant", "source_kind": "AZBase", "principal_kinds": [ "AZTEST_User", "AZTEST_Group", "AZTEST_Device" ] } ], "relationship_findings": [ { "name": "AZTEST_GenericAll", "display_name": "Generic All Access (Azure)", "environment_kind": "AZTEST_Tenant", "relationship_kind": "AZTEST_HasSession", "source_kind": "AZBase", "remediation": { "short_description": "Principal has excessive permissions on target", "long_description": "This finding indicates that a principal has been granted GenericAll permissions on a target object, providing complete control including read, write, delete, and permission modification rights.", "short_remediation": "Remove the GenericAll permission from the principal'\''s access control list", "long_remediation": "# Remove Generic All Permissions\n\n## Steps to Remediate:\n\n1. **Identify the affected object**\n2. **Remove excessive permissions**\n3. **Grant minimal required permissions**" } } ] }'curl --request DELETE \ --url http://bhe.localhost/api/v2/extensions/3 \ --header 'authorization: Bearer {{auth-token}}'Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Tests