fix(API, ETAC): Added ETAC to entity APIs BED-7403#2382
Open
fix(API, ETAC): Added ETAC to entity APIs BED-7403#2382
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR integrates ETAC (Environment-Targeted Access Control) into the v2 API handlers by adding authentication and authorization checks to multiple AD and Azure entity endpoints. It introduces a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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: 4
🧹 Nitpick comments (1)
cmd/api/src/api/v2/ad_entity.go (1)
81-85: Consider: Duplicate node fetch when access is granted.When
hasAccessis true,CheckUserHasAccessToNodeById(line 81) fetches the node to extract DomainSID, then line 85 fetches the same node again viaGetEntityByObjectId. This could be optimized by having the access check return the node when found, avoiding a second query.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/src/api/v2/ad_entity.go` around lines 81 - 85, Change the access-check to return the fetched node so we don't query twice: update CheckUserHasAccessToNodeById (or add a new CheckUserHasAccessToNodeByIdWithNode) to return (bool, *Entity, error) instead of just (bool, error), then in ad_entity.go capture the returned node when hasAccess is true and reuse it instead of calling s.GraphQuery.GetEntityByObjectId again; ensure you update all callers of CheckUserHasAccessToNodeById to handle the new signature (or add an overload wrapper) and adjust error handling to use the returned node where the code currently expects node from GetEntityByObjectId.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/api/src/api/v2/ad_related_entity.go`:
- Around line 43-55: The access check is using the generic ad.Entity (kind
"Base") which causes node lookup to fail and silently grants access; replace
ad.Entity with the specific entity kind resolved by BuildEntityQueryParams (the
returned params' entity type — e.g., params.EntityType or the variable named
entityType in other handlers) when calling CheckUserHasAccessToNodeById so the
check uses the concrete AD kind (match how handleAdEntityInfoQuery passes
entityType) and prevents the unintended access grant.
In `@cmd/api/src/api/v2/azure.go`:
- Around line 417-433: CheckUserHasAccessToNodeById is using ad.DomainSID for
environment identifiers which fails for Azure entities that use azure.TenantID;
update CheckUserHasAccessToNodeById to detect Azure vs AD (or fall back) when
resolving the node's environment identifier and use azure.TenantID for Azure
entities (or use TenantID if present) and ad.DomainSID for AD entities so ETAC
lookups succeed; ensure the change is used when called from GetAZRelatedEntities
/ azure.go (graph.StringKind(entityType) path) and preserve existing behavior
for AD entities and error handling.
In `@cmd/api/src/api/v2/etac.go`:
- Around line 68-83: The current empty branch after calling
graphQuery.GetEntityByObjectId in CheckUserHasAccessToNodeById silently grants
access on errors or missing nodes; replace that empty if-block with explicit
failure handling: if err != nil return false, err and if node == nil return
false, nil (or return a descriptive error if you prefer), then continue to
extract domainSid via node.Properties.Get(ad.DomainSID.String()).String() and
call CheckUserAccessToEnvironments as before; make sure to keep
dogTagsService.GetFlagAsBool(dogtags.ETAC_ENABLED) gating unchanged.
- Around line 73-74: In CheckUserHasAccessToNodeById, the current logic only
reads DomainSID via node.Properties.Get(ad.DomainSID.String()) and returns an
error if that lookup fails; change it to first attempt DomainSID and if that
value is absent or empty, fall back to TenantID
(node.Properties.Get(ad.TenantID.String())), mirroring filterETACGraph behavior;
only return an error if both property reads fail or both values are empty, and
ensure the subsequent ETAC checks use whichever ID was successfully retrieved.
---
Nitpick comments:
In `@cmd/api/src/api/v2/ad_entity.go`:
- Around line 81-85: Change the access-check to return the fetched node so we
don't query twice: update CheckUserHasAccessToNodeById (or add a new
CheckUserHasAccessToNodeByIdWithNode) to return (bool, *Entity, error) instead
of just (bool, error), then in ad_entity.go capture the returned node when
hasAccess is true and reuse it instead of calling
s.GraphQuery.GetEntityByObjectId again; ensure you update all callers of
CheckUserHasAccessToNodeById to handle the new signature (or add an overload
wrapper) and adjust error handling to use the returned node where the code
currently expects node from GetEntityByObjectId.
Comment on lines
43
to
55
| user, isUser := auth.GetUserFromAuthCtx(bhCtx.FromRequest(request).AuthCtx) | ||
| if !isUser { | ||
| slog.Error("Unable to get user from auth context") | ||
| api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusInternalServerError, api.ErrorResponseDetailsInternalServerError, request), response) | ||
| return | ||
| } | ||
|
|
||
| if params, err := queries.BuildEntityQueryParams(request, queryName, pathDelegate, listDelegate); err != nil { | ||
| api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusBadRequest, fmt.Sprintf(api.FmtErrorResponseDetailsBadQueryParameters, err), request), response) | ||
| } else if hasAccess, err := CheckUserHasAccessToNodeById(request.Context(), s.DB, s.GraphQuery, s.DogTags, user, params.ObjectID, ad.Entity); err != nil { | ||
| api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusInternalServerError, api.ErrorResponseDetailsInternalServerError, request), response) | ||
| } else if !hasAccess { | ||
| api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusForbidden, api.ErrorResponseDetailsForbidden, request), response) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check GetEntityByObjectId implementation
echo "=== GetEntityByObjectId implementation ==="
rg -n -A 20 'func.*GetEntityByObjectId' cmd/api/src/ --type=go | head -100
echo ""
echo "=== ad.Entity definition ==="
fd -e go . packages/go/graphschema/ad/ | xargs rg -n 'Entity\s*=' | grep -v '//' | head -20
echo ""
echo "=== Check ad package exports ==="
cat packages/go/graphschema/ad/*.go | grep -n 'Entity\|const\|var' | head -30Repository: SpecterOps/BloodHound
Length of output: 8118
🏁 Script executed:
#!/bin/bash
# Check how ad_entity.go handles the access check
echo "=== ad_entity.go access check pattern ==="
rg -n -B5 -A10 'CheckUserHasAccessToNodeById' cmd/api/src/api/v2/ad_entity.go
echo ""
echo "=== ad_related_entity.go access check pattern ==="
rg -n -B5 -A10 'CheckUserHasAccessToNodeById' cmd/api/src/api/v2/ad_related_entity.go
echo ""
echo "=== Check what GetEntityByObjectId returns ==="
rg -n -B2 -A15 'func.*GetEntityByObjectId' cmd/api/src/ --type=goRepository: SpecterOps/BloodHound
Length of output: 7064
🏁 Script executed:
#!/bin/bash
# Look at the structure of ad package to understand Entity
echo "=== ad package contents ==="
ls -la packages/go/graphschema/ad/
echo ""
echo "=== Check ad.go for Entity definition ==="
head -50 packages/go/graphschema/ad/ad.go | cat -n
echo ""
echo "=== Search for Kind type definition ==="
rg -n 'type Kind' packages/go/graphschema/Repository: SpecterOps/BloodHound
Length of output: 3117
🏁 Script executed:
#!/bin/bash
# Check CheckUserHasAccessToNodeById implementation
echo "=== CheckUserHasAccessToNodeById implementation ==="
rg -n -A 40 'func.*CheckUserHasAccessToNodeById' cmd/api/src/api/v2/ --type=go
echo ""
echo "=== Check if "Base" is ever used as a node kind in the database ==="
rg -n '"Base"' cmd/api/src/ --type=go | head -30Repository: SpecterOps/BloodHound
Length of output: 6112
🏁 Script executed:
#!/bin/bash
# Look at how the function determines access
echo "=== Find where the node lookup happens in access check ==="
rg -n -B5 -A30 'func.*CheckUserHasAccessToNodeById' cmd/api/src/api/v2/helpers.go --type=go
echo ""
echo "=== Check what AllKindsAD contains and if Base/Entity is separate ==="
rg -n 'AllKindsAD' packages/go/graphschema/ad/ -A 2Repository: SpecterOps/BloodHound
Length of output: 194
🏁 Script executed:
#!/bin/bash
# Look at the full etac.go file to understand the flow better
echo "=== Full CheckUserHasAccessToNodeById context ==="
sed -n '69,83p' cmd/api/src/api/v2/etac.go | cat -n
echo ""
echo "=== Check what happens when node is nil - the empty else-if ==="
sed -n '69,83p' cmd/api/src/api/v2/etac.go
echo ""
echo "=== Verify if "Base" nodes actually exist in the schema ==="
rg -n 'Kind.*Base|StringKind.*Base' packages/go/graphschema/ --type=go | head -20Repository: SpecterOps/BloodHound
Length of output: 1942
🏁 Script executed:
#!/bin/bash
# Check how ad_entity.go determines entityType before calling the access check
echo "=== How ad_entity.go gets entityType ==="
rg -n -B15 'CheckUserHasAccessToNodeById' cmd/api/src/api/v2/ad_entity.go | head -40
echo ""
echo "=== Check what GetADEntityQueryResult does ==="
rg -n -A5 'func.*GetADEntityQueryResult' cmd/api/src/queries/ --type=go | head -30Repository: SpecterOps/BloodHound
Length of output: 3061
🏁 Script executed:
#!/bin/bash
# Check if "Base" nodes are ever created in actual data
echo "=== Search for node creation with kind Base ==="
rg -n 'NewNode.*Base|Kind.*Base' cmd/api/src/ingest cmd/api/src/models --type=go | head -20
echo ""
echo "=== Check ad_related_entity handler to see what kind it receives ==="
rg -n -B30 'handleAdRelatedEntityQuery' cmd/api/src/api/v2/ad_related_entity.go | head -60
echo ""
echo "=== Verify the signature and parameters ==="
rg -n 'func.*handleAdRelatedEntityQuery|func.*handleAdEntityInfoQuery' cmd/api/src/api/v2/ --type=goRepository: SpecterOps/BloodHound
Length of output: 4649
🏁 Script executed:
#!/bin/bash
# Let me check the exact line where the empty block is
echo "=== Exact etac.go lines 72-79 ==="
sed -n '72,79p' cmd/api/src/api/v2/etac.go | cat -n
echo ""
echo "=== What happens if we trace through: node is nil when using ad.Entity ==="
echo "Line 72: if err != nil || node == nil {"
echo "Line 73: } <-- EMPTY BLOCK, no action taken"
echo "Line 74: } else if ... <-- This is not reached because there's nothing in the block above"
echo "Line 82: return true, nil <-- Falls through here, GRANTS ACCESS"
echo ""
echo "=== Check if there's documentation about what ad.Entity represents ==="
rg -n -B5 'Entity.*=.*Base' packages/go/graphschema/ad/ad.goRepository: SpecterOps/BloodHound
Length of output: 988
Use the specific entity kind for the access check instead of ad.Entity.
The access check on line 52 uses ad.Entity (which represents kind "Base"), but GetEntityByObjectId will never find a node with that kind since all AD nodes have specific kinds (User, Computer, Group, etc.). When the node lookup fails and returns nil, the empty if block in CheckUserHasAccessToNodeById (line 72 in etac.go) takes no action, and the function falls through to return true, nil, silently granting access. This bypasses ETAC enforcement entirely.
Use the specific entity type from BuildEntityQueryParams, similar to how handleAdEntityInfoQuery in ad_entity.go passes the resolved entityType parameter to the access check.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/api/src/api/v2/ad_related_entity.go` around lines 43 - 55, The access
check is using the generic ad.Entity (kind "Base") which causes node lookup to
fail and silently grants access; replace ad.Entity with the specific entity kind
resolved by BuildEntityQueryParams (the returned params' entity type — e.g.,
params.EntityType or the variable named entityType in other handlers) when
calling CheckUserHasAccessToNodeById so the check uses the concrete AD kind
(match how handleAdEntityInfoQuery passes entityType) and prevents the
unintended access grant.
Contributor
Author
There was a problem hiding this comment.
I'm not sure if this is critical
My understanding is kinds checks against a list of kinds, which sort of acts as an inheritance, IE:
Any node that is part of an AD environment will also have an Entity kind as well as their specific kind
Any node that is part of an AZ environment will also have an AZBase kind as well as their specific kind
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== Check how AD nodes are created and if they have multiple kinds ==="
rg -n -A10 'func.*NewNode|node\.AddKinds|SetKinds' packages/go/graphschema/ad/ --type=go | head -80
echo ""
echo "=== Check graph.Kinds implementation and if it supports multiple kinds ==="
rg -n -B3 -A10 'type.*Kinds|func.*Kinds' packages/go/dawgs/graph/ --type=go | head -100
echo ""
echo "=== Look for how Entity/Base kind is used in node creation ==="
rg -n 'Entity.*User|Computer.*Entity|AddKinds.*Entity' cmd/api/src/ packages/go/ --type=go | head -50
echo ""
echo "=== Check GetEntityByObjectId to see if it matches multiple kinds ==="
rg -n -B5 -A20 'GetEntityByObjectId.*func|\.GetEntityByObjectId' cmd/api/src/queries/ --type=go | head -80
Length of output: 12284
🏁 Script executed:
#!/bin/bash
echo "=== Check KindIn implementation to understand if it matches ANY kind ==="
rg -n -B5 -A15 'func.*KindIn' packages/go/dawgs/ --type=go | head -100
echo ""
echo "=== Look for more examples of nodes with multiple kinds in tests ==="
rg -n 'NewNode.*Entity.*User|NewNode.*Entity.*Computer|NewNode.*Entity.*Group' cmd/api/src/test/ --type=go | head -30
echo ""
echo "=== Check if there's documentation about the kind inheritance pattern ==="
rg -n -B3 -A3 'base kind|inheritance.*kind|Entity.*base' cmd/api/src/ packages/go/ --type=go | head -40
Length of output: 4409
@mvlipka You're absolutely correct, and I apologize for the confusion. Thank you for clarifying!
The evidence confirms your understanding:
-
Nodes have multiple kinds: Test code shows nodes are created with both base and specific kinds:
NewNode(properties, ad.Entity, ad.User) -
Inheritance pattern: Comments in the codebase explicitly state that "ad.Base and az.Base are used for *hound collections" and note that "generic entities do not have a predefined base kind (unlike AZ or AD)"
-
KindIn matching: The
GetEntityByObjectIdfunction usesquery.KindIn(query.Node(), kinds...)which matches if the node has ANY of the specified kinds -
Working code:
GetADEntityQueryResult(line 692) already successfully usesGetEntityByObjectId(ctx, params.ObjectID, ad.Entity)to retrieve nodes
So using ad.Entity in the access check is correct - it will match any AD node regardless of its specific type (User, Computer, Group, etc.) because all AD nodes include the base Entity kind. The access check is working as intended.
I was wrong about this being a security issue. The code is functioning correctly, and this is not critical.
✏️ Learnings added
Learnt from: mvlipka
Repo: SpecterOps/BloodHound PR: 2382
File: cmd/api/src/api/v2/ad_related_entity.go:44-57
Timestamp: 2026-02-23T17:06:23.991Z
Learning: In BloodHound's graph schema, nodes use a multi-kind inheritance pattern. AD nodes have both a base kind (ad.Entity) and specific kinds (ad.User, ad.Computer, ad.Group, etc.). Similarly, Azure nodes have az.Entity as a base kind. The query.KindIn function matches if a node has ANY of the specified kinds, so querying by ad.Entity will correctly match all AD nodes regardless of their specific type. This is the intended design pattern used throughout the codebase.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: ktstrader
Repo: SpecterOps/BloodHound PR: 2020
File: cmd/api/src/queries/graph.go:198-203
Timestamp: 2025-11-05T21:13:08.542Z
Learning: In cmd/api/src/queries/graph.go, when ETAC filtering is enabled in GetAssetGroupComboNode, an empty etacAllowedList (when user.AllEnvironments is false and user.EnvironmentTargetedAccessControl is empty) is intentional and should not be guarded against. The empty list will cause query.In() to filter out all nodes, which is the correct security behavior—users with no environment assignments should see no results.
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 2105
File: cmd/api/src/database/migration/migrations/v8.5.0.sql:139-143
Timestamp: 2025-12-10T20:16:54.652Z
Learning: In the BloodHound schema_environments_principal_kinds table (cmd/api/src/database/migration/migrations/v8.5.0.sql), the PRIMARY KEY(principal_kind) design is intentional and enforces that each principal kind is globally unique and exclusively owned by a single environment. This creates a one-to-many relationship from environments to principal kinds where an environment can have multiple principal kinds, but a principal kind can never be shared between environments.
Comment on lines
417
to
433
| user, isUser := auth.GetUserFromAuthCtx(bhCtx.FromRequest(request).AuthCtx) | ||
| if !isUser { | ||
| slog.Error("Unable to get user from auth context") | ||
| api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusInternalServerError, api.ErrorResponseDetailsInternalServerError, request), response) | ||
| return | ||
| } | ||
|
|
||
| if objectID := queryVars.Get(objectIDQueryParameterName); objectID == "" { | ||
| api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusBadRequest, fmt.Sprintf("query parameter %s is required", objectIDQueryParameterName), request), response) | ||
| } else if relatedEntityTypeStr := queryVars.Get(relatedEntityTypeQueryParameterName); relatedEntityTypeStr != "" { | ||
| s.GetAZRelatedEntities(request.Context(), response, request, objectID) | ||
| } else if includeCounts, err := api.ParseOptionalBool(queryVars.Get(api.QueryParameterIncludeCounts), true); err != nil { | ||
| api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusBadRequest, api.ErrorResponseDetailsBadQueryParameterFilters, request), response) | ||
| } else if hasAccess, err := CheckUserHasAccessToNodeById(request.Context(), s.DB, s.GraphQuery, s.DogTags, user, objectID, graph.StringKind(entityType)); err != nil { | ||
| api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusInternalServerError, api.ErrorResponseDetailsInternalServerError, request), response) | ||
| } else if !hasAccess { | ||
| api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusForbidden, api.ErrorResponseDetailsForbidden, request), response) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check Azure entity properties and DomainSID/tenantid usage
echo "=== Searching for tenantid/TenantID usage in codebase ==="
rg -n 'tenantid|TenantID|TenantId' --type=go -C 2 | head -100
echo "=== Searching for DomainSID usage in ETAC-related code ==="
rg -n 'DomainSID' --type=go -C 2 | head -100
echo "=== Check etac.go implementation around the property access ==="
fd 'etac.go' --type=fRepository: SpecterOps/BloodHound
Length of output: 13282
🏁 Script executed:
#!/bin/bash
# Get etac.go file content to check line 72-73
fd 'etac.go' --type=f -x cat -n {} | head -100Repository: SpecterOps/BloodHound
Length of output: 4089
🏁 Script executed:
#!/bin/bash
# Check CheckUserHasAccessToNodeById function signature and implementation
rg -n 'CheckUserHasAccessToNodeById' --type=go -A 15 | head -150Repository: SpecterOps/BloodHound
Length of output: 7045
🏁 Script executed:
#!/bin/bash
# Search for Azure-specific node type checking or handling
rg -n 'StringKind.*azure|azure.*StringKind' --type=go -i -C 2Repository: SpecterOps/BloodHound
Length of output: 47
Azure entities cannot be properly ETAC-filtered with the current implementation.
The CheckUserHasAccessToNodeById function retrieves environment identifiers using ad.DomainSID, which is specific to Active Directory entities. Azure entities use azure.TenantID instead (as seen in packages/go/ein/azure.go and graphschema/schema.go). When this function is called with an Azure entity at line 430 of azure.go, the property lookup fails and returns an error, causing the access check to deny access rather than properly validating ETAC permissions. This prevents Azure entities from being fetched even for users with valid ETAC assignments. Update CheckUserHasAccessToNodeById to handle both AD and Azure entity types by checking the appropriate environment property for each.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/api/src/api/v2/azure.go` around lines 417 - 433,
CheckUserHasAccessToNodeById is using ad.DomainSID for environment identifiers
which fails for Azure entities that use azure.TenantID; update
CheckUserHasAccessToNodeById to detect Azure vs AD (or fall back) when resolving
the node's environment identifier and use azure.TenantID for Azure entities (or
use TenantID if present) and ad.DomainSID for AD entities so ETAC lookups
succeed; ensure the change is used when called from GetAZRelatedEntities /
azure.go (graph.StringKind(entityType) path) and preserve existing behavior for
AD entities and error handling.
Contributor
Author
There was a problem hiding this comment.
This doesn't seem right
The code in CheckUserHasAccessToNodeById checks if the node is type of azure.Entity before checking for a TenantID property
Similarly for if the node is type ad.Entity, only then does it check for a DomainSid property
The result of either of those functions gets set to environmentID, if environmentID is empty then we throw an error
The errors you're mentioning only propagate if the property to the corresponding entity type does not exist, which in this case we'd want it to fail since an Azure node should always have a tenantid and an AD node should always have a domainsid
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Get the full implementation of CheckUserHasAccessToNodeById to verify the logic
echo "=== Full CheckUserHasAccessToNodeById implementation ==="
rg -n 'func CheckUserHasAccessToNodeById' --type=go -A 50
echo -e "\n=== Check if there's type checking for azure.Entity and ad.Entity ==="
rg -n 'azure\.Entity|ad\.Entity' --type=go cmd/api/src/api/v2/etac.go -C 5
Length of output: 4008
@mvlipka You're absolutely right, and I apologize for the incorrect analysis.
Looking at the actual implementation in cmd/api/src/api/v2/etac.go (lines 78-89), the function does exactly what you described:
- Line 78 checks if the node is an
azure.Entitytype before attempting to retrieveazure.TenantID - Line 84 checks if the node is an
ad.Entitytype before attempting to retrievead.DomainSID
The logic properly distinguishes between Azure and AD entities and uses the appropriate property for each. The errors would only occur if an Azure node is missing its tenantid or an AD node is missing its domainsid, which is the correct failure behavior as you noted.
My initial concern about the function using ad.DomainSID for Azure entities was completely unfounded. Thank you for the correction!
🐰💭
Taking notes more carefully...
🧠 Learnings used
Learnt from: ktstrader
Repo: SpecterOps/BloodHound PR: 2020
File: cmd/api/src/queries/graph.go:198-203
Timestamp: 2025-11-05T21:13:08.542Z
Learning: In cmd/api/src/queries/graph.go, when ETAC filtering is enabled in GetAssetGroupComboNode, an empty etacAllowedList (when user.AllEnvironments is false and user.EnvironmentTargetedAccessControl is empty) is intentional and should not be guarded against. The empty list will cause query.In() to filter out all nodes, which is the correct security behavior—users with no environment assignments should see no results.
Learnt from: mvlipka
Repo: SpecterOps/BloodHound PR: 1784
File: cmd/api/src/api/v2/auth/auth.go:559-573
Timestamp: 2025-09-02T16:46:30.895Z
Learning: In the BloodHound codebase, when updating user ETAC (Environment Access Control) lists in the UpdateUser function, the approach is to let GORM handle the creation/persistence of the environment access records through model associations rather than using explicit database helper methods like UpdateEnvironmentListForUser.
Learnt from: mvlipka
Repo: SpecterOps/BloodHound PR: 2382
File: cmd/api/src/api/v2/ad_related_entity.go:44-57
Timestamp: 2026-02-23T17:06:23.991Z
Learning: In BloodHound, AD nodes are represented by a base kind (ad.Entity) plus specific kinds (ad.User, ad.Computer, ad.Group, etc.), and Azure nodes by az.Entity. The query.KindIn function matches if a node has any of the specified kinds, so using the base kind (ad.Entity) will correctly include all AD nodes regardless of their specific subtype. During reviews, ensure code that filters or queries graph nodes via KindIn uses the base kind to capture all inherited subkinds for AD nodes, and similarly recognizes base kinds for Azure nodes. This design pattern should be consistently respected across files that model or query graph schema kinds.
Learnt from: mvlipka
Repo: SpecterOps/BloodHound PR: 1784
File: cmd/api/src/api/v2/auth/auth.go:377-383
Timestamp: 2025-09-09T19:19:06.998Z
Learning: In the BloodHound codebase, during user creation with ETAC (Environment Access Control), GORM automatically handles the UserID foreign key association when persisting the User model with its EnvironmentAccessControl association, eliminating the need to manually set UserID in the EnvironmentAccess records before persistence.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
cmd/api/src/api/v2/ad_related_entity.go (1)
51-54:⚠️ Potential issue | 🟠 MajorConfirm the ETAC kind used here (ad.Entity may be too generic).
If AD nodes don’t includead.Entityin theirKinds,GetEntityByObjectIdwill miss and ETAC will deny legitimate access. Prefer the concrete kind fromparams(if available), or verify thatad.Entityis always present.#!/bin/bash # Verify ad.Entity kind usage and GetEntityByObjectId kind filtering rg -n 'Entity\s*=\s*graph\.StringKind' packages/go/graphschema/ad -g'*.go' rg -n -C3 'func .*GetEntityByObjectId' cmd/api/src/queries -g'*.go' rg -n 'ad\.Entity' cmd/api/src/ingest cmd/api/src/queries packages/go/graphschema/ad -g'*.go'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/src/api/v2/ad_related_entity.go` around lines 51 - 54, The access-check is using the generic ad.Entity kind which can cause GetEntityByObjectId to miss AD nodes; update the check in the flow that calls CheckUserHasAccessToNodeById (after queries.BuildEntityQueryParams returns) to use the concrete kind from the built params (e.g., params.Kind or params.EntityKind) instead of ad.Entity when available, falling back to ad.Entity only if params does not supply a kind; ensure references include BuildEntityQueryParams, params.ObjectID, CheckUserHasAccessToNodeById and GetEntityByObjectId so the change adjusts the kind used for ETAC.cmd/api/src/api/v2/etac.go (1)
71-75:⚠️ Potential issue | 🟠 MajorAvoid 500s for missing nodes when ETAC is enabled.
GetEntityByObjectIdcan returngraph.ErrNoResultsFound; bubbling that up as an error causes callers to respond 500 instead of the expected 404. Consider treating “not found” as a non-error (or surfacing a sentinel) so handlers can return 404.🛠️ Suggested fix
- node, err := graphQuery.GetEntityByObjectId(ctx, objectId, kind) - if err != nil || node == nil { - return false, err - } + node, err := graphQuery.GetEntityByObjectId(ctx, objectId, kind) + if err != nil { + if graph.IsErrNotFound(err) { + return true, nil + } + return false, err + } else if node == nil { + return true, nil + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/src/api/v2/etac.go` around lines 71 - 75, When ETAC filtering is enabled in ShouldFilterForETAC, do not propagate graph.ErrNoResultsFound as an error; instead detect errors.Is(err, graph.ErrNoResultsFound) (or compare to graph.ErrNoResultsFound) and treat it as “not found” (return a non-error sentinel so the HTTP handler can produce 404). Update the block that calls graphQuery.GetEntityByObjectId in cmd/api/src/api/v2/etac.go to: if err != nil { if errors.Is(err, graph.ErrNoResultsFound) { return false, nil } return false, err } and likewise treat a nil node as not found (return false, nil), referencing ShouldFilterForETAC and GetEntityByObjectId to locate the change.
🧹 Nitpick comments (1)
cmd/api/src/api/v2/azure.go (1)
411-424: Hoistuser/isUserinto the existing var block.
This keeps the function aligned with the project’s variable-initialization guideline.♻️ Suggested refactor
var ( requestVars = mux.Vars(request) queryVars = request.URL.Query() entityType = requestVars[entityTypePathParameterName] + user, isUser = auth.GetUserFromAuthCtx(bhCtx.FromRequest(request).AuthCtx) ) - user, isUser := auth.GetUserFromAuthCtx(bhCtx.FromRequest(request).AuthCtx) if !isUser {As per coding guidelines “Group variable initializations in a
var ( ... )block and hoist them to the top of the function when possible”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/src/api/v2/azure.go` around lines 411 - 424, In GetAZEntity, hoist the auth lookup into the existing top var block: add user, isUser := auth.GetUserFromAuthCtx(bhCtx.FromRequest(request).AuthCtx) (or the equivalent names user and isUser) inside the var (...) group alongside requestVars, queryVars, and entityType so all local initializations are grouped; then remove the separate assignment below and keep the subsequent isUser check and error handling as-is. This affects the GetAZEntity function and references auth.GetUserFromAuthCtx, bhCtx.FromRequest, and the user/isUser symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/api/src/api/v2/azure.go`:
- Around line 431-432: Before calling CheckUserHasAccessToNodeById, map the
incoming API entityType (values like "users","groups","tenants") to the correct
Azure graph kind constants (e.g., azure.AZUser, azure.AZGroup, azure.AZTenant)
and pass that mapped kind into graph.StringKind instead of the raw entityType;
update the code around the ETAC check that calls CheckUserHasAccessToNodeById
(and references entityType and graph.StringKind) to compute a variable like
mappedKind := graph.StringKind(mappedAzureKind) via a small switch or map and
use mappedKind in the access check so the function looks up nodes by the correct
Azure kind.
---
Duplicate comments:
In `@cmd/api/src/api/v2/ad_related_entity.go`:
- Around line 51-54: The access-check is using the generic ad.Entity kind which
can cause GetEntityByObjectId to miss AD nodes; update the check in the flow
that calls CheckUserHasAccessToNodeById (after queries.BuildEntityQueryParams
returns) to use the concrete kind from the built params (e.g., params.Kind or
params.EntityKind) instead of ad.Entity when available, falling back to
ad.Entity only if params does not supply a kind; ensure references include
BuildEntityQueryParams, params.ObjectID, CheckUserHasAccessToNodeById and
GetEntityByObjectId so the change adjusts the kind used for ETAC.
In `@cmd/api/src/api/v2/etac.go`:
- Around line 71-75: When ETAC filtering is enabled in ShouldFilterForETAC, do
not propagate graph.ErrNoResultsFound as an error; instead detect errors.Is(err,
graph.ErrNoResultsFound) (or compare to graph.ErrNoResultsFound) and treat it as
“not found” (return a non-error sentinel so the HTTP handler can produce 404).
Update the block that calls graphQuery.GetEntityByObjectId in
cmd/api/src/api/v2/etac.go to: if err != nil { if errors.Is(err,
graph.ErrNoResultsFound) { return false, nil } return false, err } and likewise
treat a nil node as not found (return false, nil), referencing
ShouldFilterForETAC and GetEntityByObjectId to locate the change.
---
Nitpick comments:
In `@cmd/api/src/api/v2/azure.go`:
- Around line 411-424: In GetAZEntity, hoist the auth lookup into the existing
top var block: add user, isUser :=
auth.GetUserFromAuthCtx(bhCtx.FromRequest(request).AuthCtx) (or the equivalent
names user and isUser) inside the var (...) group alongside requestVars,
queryVars, and entityType so all local initializations are grouped; then remove
the separate assignment below and keep the subsequent isUser check and error
handling as-is. This affects the GetAZEntity function and references
auth.GetUserFromAuthCtx, bhCtx.FromRequest, and the user/isUser symbols.
ℹ️ Review info
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cmd/api/src/api/v2/ad_entity.gocmd/api/src/api/v2/ad_entity_test.gocmd/api/src/api/v2/ad_related_entity.gocmd/api/src/api/v2/ad_related_entity_test.gocmd/api/src/api/v2/azure.gocmd/api/src/api/v2/azure_test.gocmd/api/src/api/v2/etac.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmd/api/src/api/v2/ad_entity_test.go (2)
1718-1733:⚠️ Potential issue | 🟡 MinorPre-existing
buildRequestfunctions for RootCA don't setMethod.All other table-driven test functions in this file (Base, Container, AIACA, EnterpriseCA, NTAuthStore, CertTemplate, IssuancePolicy) consistently set
Method: http.MethodGetin everybuildRequest, but the five pre-existing RootCA test cases omit it. The rewritten loop at line 1975 now uses.Methods(requestWithCtx.Method), which will receive an empty string for these cases.🔧 Add `Method: http.MethodGet` to all pre-existing RootCA test cases
Example for the first case (apply similarly to the others at lines 1737, 1756, 1775, 1796):
buildRequest: func() *http.Request { return &http.Request{ URL: &url.URL{ RawQuery: "counts=`", Path: "/api/v2/rootcas/id", }, + Method: http.MethodGet, } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/src/api/v2/ad_entity_test.go` around lines 1718 - 1733, Several RootCA table-driven test cases in ad_entity_test.go have buildRequest functions that omit the HTTP method, causing requestWithCtx.Method to be empty when routed via .Methods(requestWithCtx.Method); update each pre-existing RootCA buildRequest (the ones around the first case and those at the other listed cases) to include Method: http.MethodGet so the router receives a valid method; locate the buildRequest closures referenced in the RootCA test table and add Method: http.MethodGet to the returned *http.Request in each case.
46-153:⚠️ Potential issue | 🟡 MinorAdd ETAC-specific test cases to harness-based handlers.
The harness-based tests (Computer, Domain, GPO, OU, User, Group) call
handleAdEntityInfoQuery, which invokesCheckUserHasAccessToNodeByIdfor ETAC validation. However, these tests provide empty auth context and disabled ETAC overrides, exercising only the "ETAC disabled" path. The table-driven tests (Base, Container, AIACA, RootCA, EnterpriseCA, NTAuthStore, CertTemplate, IssuancePolicy) explicitly cover three ETAC scenarios: AllEnvironments access, Specific Environment access, and Forbidden cases. Add similar scenarios to the harness-based tests to achieve parity in ETAC coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/src/api/v2/ad_entity_test.go` around lines 46 - 153, The harness-based test TestResources_GetComputerEntityInfo currently only exercises the ETAC-disabled path; add ETAC-specific cases mirroring the table-driven tests by creating three new apitest.Case entries in TestResources_GetComputerEntityInfo that set different auth contexts and permission overrides and assert the expected responses. Specifically, add cases for "ETACAllEnvironments" (AuthCtx.PermissionOverrides granting AllEnvironments access -> expect 200), "ETACSpecificEnvironment" (granting access to a specific environment present on the node -> expect 200), and "ETACForbidden" (granting no matching environment -> expect 403), ensuring each case still exercises Resources.GetComputerEntityInfo which calls handleAdEntityInfoQuery and CheckUserHasAccessToNodeById, and adjust mockGraph expectations (GetEntityByObjectId / GetEntityCountResults) to return a node with environment metadata when needed for the SpecificEnvironment case.
🧹 Nitpick comments (4)
cmd/api/src/api/v2/ad_related_entity.go (1)
44-47: Hoist auth user variables into a var block.
This keeps initialization grouped at the top of the function per project style.As per coding guidelines: Group variable initializations in a `var ( ... )` block and hoist them to the top of the function when possible.♻️ Suggested refactor
- user, isUser := auth.GetUserFromAuthCtx(bhCtx.FromRequest(request).AuthCtx) + var ( + user model.User + isUser bool + ) + user, isUser = auth.GetUserFromAuthCtx(bhCtx.FromRequest(request).AuthCtx)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/src/api/v2/ad_related_entity.go` around lines 44 - 47, Hoist the auth user vars into a var block at the top of the function: declare a var (user auth.UserType?; isUser bool) or appropriate types, then assign them using user, isUser = auth.GetUserFromAuthCtx(bhCtx.FromRequest(request).AuthCtx) where the current call appears; update the existing error branch to reference the hoisted variables (user, isUser) and remove the inline declaration. Ensure the var block is placed before any logic in the function so variable initialization is grouped per project style.cmd/api/src/api/v2/ad_entity.go (1)
71-74: Hoist auth user variables into a var block.As per coding guidelines: Group variable initializations in a `var ( ... )` block and hoist them to the top of the function when possible.♻️ Suggested refactor
- user, isUser := auth.GetUserFromAuthCtx(bhCtx.FromRequest(request).AuthCtx) + var ( + user model.User + isUser bool + ) + user, isUser = auth.GetUserFromAuthCtx(bhCtx.FromRequest(request).AuthCtx)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/src/api/v2/ad_entity.go` around lines 71 - 74, The user/auth extraction variables (user, isUser) are declared inline; hoist them into a var block near the top of the containing function and group related initializations per the coding guideline: replace the inline "user, isUser := auth.GetUserFromAuthCtx(bhCtx.FromRequest(request).AuthCtx)" with a var block that declares "var user *<type>" and "var isUser bool" (use the actual user type), then assign to them with "user, isUser = auth.GetUserFromAuthCtx(bhCtx.FromRequest(request).AuthCtx)" before the existing nil-check and the api.WriteErrorResponse / api.BuildErrorResponse call so the variables are grouped and initialize location is unchanged.cmd/api/src/api/v2/azure.go (1)
413-423: Hoist auth user variables into the existing var block.As per coding guidelines: Group variable initializations in a `var ( ... )` block and hoist them to the top of the function when possible.♻️ Suggested refactor
var ( requestVars = mux.Vars(request) queryVars = request.URL.Query() entityType = requestVars[entityTypePathParameterName] + user model.User + isUser bool ) -user, isUser := auth.GetUserFromAuthCtx(bhCtx.FromRequest(request).AuthCtx) +user, isUser = auth.GetUserFromAuthCtx(bhCtx.FromRequest(request).AuthCtx)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/src/api/v2/azure.go` around lines 413 - 423, Hoist the auth user extraction into the existing var block by adding user and isUser initializers alongside requestVars, queryVars and entityType (call auth.GetUserFromAuthCtx(bhCtx.FromRequest(request).AuthCtx) inside the same var ( ... ) block); keep the subsequent isUser check and error handling logic (slog.Error and api.WriteErrorResponse) unchanged so only the variable declarations are moved, and reference the same symbols (requestVars, queryVars, entityType, auth.GetUserFromAuthCtx, bhCtx.FromRequest, user, isUser) when locating where to apply the change.cmd/api/src/api/v2/ad_entity_test.go (1)
928-1055: Consider extracting shared ETAC test scaffolding to reduce duplication.The three ETAC test scenarios (AllEnvironments, Specific Environment, Forbidden) and the test loop (bheCtx construction, resource wiring, router registration, assertions) are nearly identical across all eight table-driven test functions, differing only in the entity kind and URL path. This is ~130+ lines duplicated eight times.
A shared helper function parameterized by entity kind, URL pattern, and handler could generate the ETAC-specific test cases and/or the common loop logic, significantly reducing the maintenance surface. Understandably this could be deferred.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/src/api/v2/ad_entity_test.go` around lines 928 - 1055, Extract the repeated ETAC table-driven test scaffolding in ad_entity_test.go into a single helper that builds and runs the three ETAC scenarios (AllEnvironments, Specific Environment, Forbidden) parameterized by entity kind, URL path and the handler under test; move the duplicated setup logic that constructs bheCtx, wires resources, registers the router, and asserts response into this helper and invoke it from each test, reusing the same mock expectations (mock.mockGraphQuery.GetEntityByObjectId, mock.mockGraphQuery.GetEntityCountResults, mock.mockDatabase.GetEnvironmentTargetedAccessControlForUser), dogtags.ETAC_ENABLED overrides and model.User variants to reduce duplication. Ensure the helper accepts the unique symbols (entity kind like ad.Entity, the request URL/RawQuery, and the handler function) so existing table entries can be replaced with concise calls to the helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/api/src/api/v2/etac.go`:
- Around line 71-75: When calling graphQuery.GetEntityByObjectId inside
ShouldFilterForETAC handling, detect and translate a "not found" result instead
of returning it as a generic error; specifically, check the error against the
graph-query not-found sentinel or helper (e.g., errors.Is(err,
graphQuery.ErrNotFound) or graphQuery.IsNotFound(err)) and for that case return
a non-500 response (either return false, nil to treat as “no access” or
wrap/convert to a typed NotFound error that handlers will map to 404). Update
the GetEntityByObjectId error handling branch around node, err so not-found is
handled explicitly and other errors are still returned unchanged.
---
Outside diff comments:
In `@cmd/api/src/api/v2/ad_entity_test.go`:
- Around line 1718-1733: Several RootCA table-driven test cases in
ad_entity_test.go have buildRequest functions that omit the HTTP method, causing
requestWithCtx.Method to be empty when routed via
.Methods(requestWithCtx.Method); update each pre-existing RootCA buildRequest
(the ones around the first case and those at the other listed cases) to include
Method: http.MethodGet so the router receives a valid method; locate the
buildRequest closures referenced in the RootCA test table and add Method:
http.MethodGet to the returned *http.Request in each case.
- Around line 46-153: The harness-based test TestResources_GetComputerEntityInfo
currently only exercises the ETAC-disabled path; add ETAC-specific cases
mirroring the table-driven tests by creating three new apitest.Case entries in
TestResources_GetComputerEntityInfo that set different auth contexts and
permission overrides and assert the expected responses. Specifically, add cases
for "ETACAllEnvironments" (AuthCtx.PermissionOverrides granting AllEnvironments
access -> expect 200), "ETACSpecificEnvironment" (granting access to a specific
environment present on the node -> expect 200), and "ETACForbidden" (granting no
matching environment -> expect 403), ensuring each case still exercises
Resources.GetComputerEntityInfo which calls handleAdEntityInfoQuery and
CheckUserHasAccessToNodeById, and adjust mockGraph expectations
(GetEntityByObjectId / GetEntityCountResults) to return a node with environment
metadata when needed for the SpecificEnvironment case.
---
Nitpick comments:
In `@cmd/api/src/api/v2/ad_entity_test.go`:
- Around line 928-1055: Extract the repeated ETAC table-driven test scaffolding
in ad_entity_test.go into a single helper that builds and runs the three ETAC
scenarios (AllEnvironments, Specific Environment, Forbidden) parameterized by
entity kind, URL path and the handler under test; move the duplicated setup
logic that constructs bheCtx, wires resources, registers the router, and asserts
response into this helper and invoke it from each test, reusing the same mock
expectations (mock.mockGraphQuery.GetEntityByObjectId,
mock.mockGraphQuery.GetEntityCountResults,
mock.mockDatabase.GetEnvironmentTargetedAccessControlForUser),
dogtags.ETAC_ENABLED overrides and model.User variants to reduce duplication.
Ensure the helper accepts the unique symbols (entity kind like ad.Entity, the
request URL/RawQuery, and the handler function) so existing table entries can be
replaced with concise calls to the helper.
In `@cmd/api/src/api/v2/ad_entity.go`:
- Around line 71-74: The user/auth extraction variables (user, isUser) are
declared inline; hoist them into a var block near the top of the containing
function and group related initializations per the coding guideline: replace the
inline "user, isUser :=
auth.GetUserFromAuthCtx(bhCtx.FromRequest(request).AuthCtx)" with a var block
that declares "var user *<type>" and "var isUser bool" (use the actual user
type), then assign to them with "user, isUser =
auth.GetUserFromAuthCtx(bhCtx.FromRequest(request).AuthCtx)" before the existing
nil-check and the api.WriteErrorResponse / api.BuildErrorResponse call so the
variables are grouped and initialize location is unchanged.
In `@cmd/api/src/api/v2/ad_related_entity.go`:
- Around line 44-47: Hoist the auth user vars into a var block at the top of the
function: declare a var (user auth.UserType?; isUser bool) or appropriate types,
then assign them using user, isUser =
auth.GetUserFromAuthCtx(bhCtx.FromRequest(request).AuthCtx) where the current
call appears; update the existing error branch to reference the hoisted
variables (user, isUser) and remove the inline declaration. Ensure the var block
is placed before any logic in the function so variable initialization is grouped
per project style.
In `@cmd/api/src/api/v2/azure.go`:
- Around line 413-423: Hoist the auth user extraction into the existing var
block by adding user and isUser initializers alongside requestVars, queryVars
and entityType (call auth.GetUserFromAuthCtx(bhCtx.FromRequest(request).AuthCtx)
inside the same var ( ... ) block); keep the subsequent isUser check and error
handling logic (slog.Error and api.WriteErrorResponse) unchanged so only the
variable declarations are moved, and reference the same symbols (requestVars,
queryVars, entityType, auth.GetUserFromAuthCtx, bhCtx.FromRequest, user, isUser)
when locating where to apply the change.
ℹ️ Review info
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cmd/api/src/api/v2/ad_entity.gocmd/api/src/api/v2/ad_entity_test.gocmd/api/src/api/v2/ad_related_entity.gocmd/api/src/api/v2/ad_related_entity_test.gocmd/api/src/api/v2/azure.gocmd/api/src/api/v2/azure_test.gocmd/api/src/api/v2/etac.go
Comment on lines
+71
to
+75
| if ShouldFilterForETAC(dogTagsService, user) { | ||
| node, err := graphQuery.GetEntityByObjectId(ctx, objectId, kind) | ||
| if err != nil || node == nil { | ||
| return false, err | ||
| } |
Contributor
There was a problem hiding this comment.
Handle not-found node lookups to avoid 500s.
If GetEntityByObjectId returns a not-found error, callers currently treat that as a 500. Consider translating not-found into a non-error (403) or a typed not-found error that handlers map to 404.
🔧 Suggested fix (convert not-found to a non-error)
- node, err := graphQuery.GetEntityByObjectId(ctx, objectId, kind)
- if err != nil || node == nil {
- return false, err
- }
+ node, err := graphQuery.GetEntityByObjectId(ctx, objectId, kind)
+ if err != nil {
+ if graph.IsErrNotFound(err) {
+ return false, nil
+ }
+ return false, err
+ }
+ if node == nil {
+ return false, nil
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/api/src/api/v2/etac.go` around lines 71 - 75, When calling
graphQuery.GetEntityByObjectId inside ShouldFilterForETAC handling, detect and
translate a "not found" result instead of returning it as a generic error;
specifically, check the error against the graph-query not-found sentinel or
helper (e.g., errors.Is(err, graphQuery.ErrNotFound) or
graphQuery.IsNotFound(err)) and for that case return a non-500 response (either
return false, nil to treat as “no access” or wrap/convert to a typed NotFound
error that handlers will map to 404). Update the GetEntityByObjectId error
handling branch around node, err so not-found is handled explicitly and other
errors are still returned unchanged.
Contributor
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)
cmd/api/src/api/v2/azure_test.go (1)
1178-1192:⚠️ Potential issue | 🟡 MinorRename this test case to match the asserted status code.
Line 1178 says “Internal Server Error”, but Line 1190 asserts
http.StatusBadRequest(400). The mismatch makes failure triage confusing.✏️ Suggested rename
- name: "Error: GetAZEntityInformation unknown azure entity - Internal Server Error", + name: "Error: GetAZEntityInformation unknown azure entity - Bad Request",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/src/api/v2/azure_test.go` around lines 1178 - 1192, The test case name string in the table-driven test ("Error: GetAZEntityInformation unknown azure entity - Internal Server Error") is inconsistent with the asserted status code (expected.responseCode/http.StatusBadRequest); update the name field to reflect a 400 Bad Request (e.g., mention "Bad Request" or "400") so the test description matches the assertion—locate and change the name value in the test entry alongside the existing buildRequest, setupMocks, and expected blocks.
♻️ Duplicate comments (1)
cmd/api/src/api/v2/etac.go (1)
72-75:⚠️ Potential issue | 🟠 MajorHandle not-found separately in ETAC pre-check to avoid 500s.
Line 73 currently returns
false, errfor all lookup errors. When that error is “not found”, callers convert it to a 500. This causes missing entities to fail as internal errors whenever ETAC filtering is active.💡 Proposed fix
node, err := graphQuery.GetEntityByObjectId(ctx, objectId, kind) - if err != nil || node == nil { - return false, err - } + if err != nil { + if graph.IsErrNotFound(err) { + return false, nil + } + return false, err + } + if node == nil { + return false, nil + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/src/api/v2/etac.go` around lines 72 - 75, In the ETAC pre-check where you call graphQuery.GetEntityByObjectId (the node, err := graphQuery.GetEntityByObjectId(ctx, objectId, kind) block), treat "not found" separately: if err != nil detect the not-found case (e.g. errors.Is(err, graphQuery.ErrNotFound) or graphQuery.IsNotFound(err)) and return false, nil so callers don't convert it to a 500; for any other error return false, err; also if node == nil treat as not-found and return false, nil. Ensure you import errors if using errors.Is.
🧹 Nitpick comments (1)
cmd/api/src/api/v2/azure_test.go (1)
292-367: Add ETAC coverage for users with no environment assignments.The new ETAC tests cover all-env and specific-env allow/deny, but they miss the
AllEnvironments=false+ empty assignment case. That edge case is security-relevant and should be pinned with an explicit test in both suites.Based on learnings: when ETAC is enabled and a user has no assigned environments, an empty ETAC list is intentional and should result in no access.
Also applies to: 1217-1292
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/src/api/v2/azure_test.go` around lines 292 - 367, Add explicit ETAC test cases that cover the edge where user.AllEnvironments is false and user.EnvironmentTargetedAccessControl is an empty slice: create a test similar to the existing "Success: ETAC enabled For Specific Environment" and "Success: ETAC enabled AllEnvironments" cases but set AllEnvironments:false and EnvironmentTargetedAccessControl:[]model.EnvironmentTargetedAccessControl{} and keep dogtags.TestOverrides { Bools: { dogtags.ETAC_ENABLED: true } }; in setupMocks ensure GraphDB/GraphQuery/Database mocks behave as for the other ETAC tests (e.g., mock.mockGraphDB.EXPECT().ReadTransaction(...).Return(nil) and, if needed, return the role node via mock.mockGraphQuery.EXPECT().GetEntityByObjectId(...)) and assert the response is a denied/no-access result (responseCode http.StatusOK with empty data as the other ETAC deny cases expect). Apply the same addition to both test suites noted around the other block (also in the region referenced 1217-1292) so the empty-assignment edge case is covered in all relevant tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@cmd/api/src/api/v2/azure_test.go`:
- Around line 1178-1192: The test case name string in the table-driven test
("Error: GetAZEntityInformation unknown azure entity - Internal Server Error")
is inconsistent with the asserted status code
(expected.responseCode/http.StatusBadRequest); update the name field to reflect
a 400 Bad Request (e.g., mention "Bad Request" or "400") so the test description
matches the assertion—locate and change the name value in the test entry
alongside the existing buildRequest, setupMocks, and expected blocks.
---
Duplicate comments:
In `@cmd/api/src/api/v2/etac.go`:
- Around line 72-75: In the ETAC pre-check where you call
graphQuery.GetEntityByObjectId (the node, err :=
graphQuery.GetEntityByObjectId(ctx, objectId, kind) block), treat "not found"
separately: if err != nil detect the not-found case (e.g. errors.Is(err,
graphQuery.ErrNotFound) or graphQuery.IsNotFound(err)) and return false, nil so
callers don't convert it to a 500; for any other error return false, err; also
if node == nil treat as not-found and return false, nil. Ensure you import
errors if using errors.Is.
---
Nitpick comments:
In `@cmd/api/src/api/v2/azure_test.go`:
- Around line 292-367: Add explicit ETAC test cases that cover the edge where
user.AllEnvironments is false and user.EnvironmentTargetedAccessControl is an
empty slice: create a test similar to the existing "Success: ETAC enabled For
Specific Environment" and "Success: ETAC enabled AllEnvironments" cases but set
AllEnvironments:false and
EnvironmentTargetedAccessControl:[]model.EnvironmentTargetedAccessControl{} and
keep dogtags.TestOverrides { Bools: { dogtags.ETAC_ENABLED: true } }; in
setupMocks ensure GraphDB/GraphQuery/Database mocks behave as for the other ETAC
tests (e.g., mock.mockGraphDB.EXPECT().ReadTransaction(...).Return(nil) and, if
needed, return the role node via
mock.mockGraphQuery.EXPECT().GetEntityByObjectId(...)) and assert the response
is a denied/no-access result (responseCode http.StatusOK with empty data as the
other ETAC deny cases expect). Apply the same addition to both test suites noted
around the other block (also in the region referenced 1217-1292) so the
empty-assignment edge case is covered in all relevant tests.
ℹ️ Review info
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cmd/api/src/api/v2/ad_entity.gocmd/api/src/api/v2/ad_entity_test.gocmd/api/src/api/v2/ad_related_entity.gocmd/api/src/api/v2/ad_related_entity_test.gocmd/api/src/api/v2/azure.gocmd/api/src/api/v2/azure_test.gocmd/api/src/api/v2/etac.go
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.
Description
This adds ETAC protection to the remaining entity APIs
Motivation and Context
Resolves: BED-7403
How Has This Been Tested?
Check ticket for testing details
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes