feat: BED-6601 Add exposure values to Domain Selectors #1983
Conversation
sirisjo
added
enhancement
WalkthroughAdds enterprise-facing exposure data to the DomainSelector model: new exported types Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as "API / Handler"
participant Model as "DomainSelector Model"
participant OpenAPI as "OpenAPI Schema"
Client->>API: Request domain selector (read)
API->>Model: Load DomainSelector (includes exposures, impactValue)
Note right of Model #D6EAF8: New Exposures slice and Exposure items
Model-->>API: DomainSelector with Exposures[]
API->>Client: JSON response (includes exposures, impactValue)
API->>OpenAPI: Serve updated schema (exposures, impactValue)
OpenAPI-->>Client: Schema documents enterprise-only fields
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
![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
🧹 Nitpick comments (1)
packages/go/openapi/src/schemas/model.domain-selector.yaml (1)
35-46: Naming inconsistency confirmed; schema reference verified.The referenced schema
model.asset-group-tag.yamlexists. However, the naming inconsistency is real:impactValueuses camelCase (line 31) while the nested properties use snake_case (exposure_percent,asset_group_tag). Ensure this naming variance is intentional or aligns with project conventions.Optional improvement: Extract the inline
itemsschema (lines 39-46) to a separate file for reusability:exposures: type: array readOnly: true description: "Enterprise Only" items: $ref: './model.exposure.yaml'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/api/src/model/search.go(1 hunks)packages/go/openapi/doc/openapi.json(2 hunks)packages/go/openapi/src/schemas/model.domain-selector.yaml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/api/src/model/search.go (1)
packages/go/graphschema/common/common.go (3)
Name(53-53)ObjectID(52-52)Collected(57-57)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-analysis
- GitHub Check: run-tests
- GitHub Check: run-tests
- GitHub Check: build-ui
🔇 Additional comments (3)
cmd/api/src/model/search.go (2)
43-43: LGTM: Exposures field addition is backward compatible.The
omitemptytag ensures this change is non-breaking for existing clients.
48-53: Clarify JSON naming convention—inconsistency exists but broader codebase uses snake_case.The JSON tags for
Exposurefields use snake_case (exposure_percent,asset_group_tag), whileDomainSelector.ImpactValueuses camelCase. However, sampling the broader model package reveals that snake_case is used consistently elsewhere (e.g.,OIDCProvider.client_id,SSOProvider.oidc_provider), suggesting this may be an intentional pattern.
AssetGroupTagtype is confirmed to exist incmd/api/src/model/assetgrouptags.go:93- Verify whether the intended convention is snake_case (matching broader model patterns) or if
DomainSelectorshould be updated for consistency- Clarify whether
ExposurePercentshould have validation constraintspackages/go/openapi/src/schemas/model.domain-selector.yaml (1)
31-34: LGTM: impactValue field properly documented.The field is correctly marked as
readOnlyand documented as enterprise-only.
| "null.boolean": { | ||
| "type": "object", | ||
| "properties": { | ||
| "bool": { | ||
| "type": "boolean" | ||
| }, | ||
| "valid": { | ||
| "description": "Valid is true if `boolean` is not `null`", | ||
| "type": "boolean" | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
Was just looking at the PR because I'm trying to dive into the BE, but this jumped up to me so wanted to just confirm if it was intended or not.
There was a problem hiding this comment.
Good eye! This file is auto-generated, I'm not sure why it decided to move these things up in the file, but it looks like that's what it did.
There was a problem hiding this comment.
Sounds good, TIL! Thank you for the explanation 😊
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/api/src/model/search.go(1 hunks)packages/go/openapi/doc/openapi.json(2 hunks)packages/go/openapi/src/schemas/model.domain-selector.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/go/openapi/src/schemas/model.domain-selector.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: run-tests
- GitHub Check: run-tests
- GitHub Check: run-analysis
- GitHub Check: build-ui
| Type string `json:"type"` | ||
| Name string `json:"name"` | ||
| ObjectID string `json:"id"` | ||
| Collected bool `json:"collected"` | ||
| ImpactValue *int `json:"impactValue,omitempty"` | ||
| Exposures Exposures `json:"exposures,omitempty"` | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add documentation for the new enterprise-only fields.
The new ImpactValue and Exposures fields lack godoc comments. Since these are public fields on a public type and are marked as "enterprise only" in the PR description, documentation should clarify:
- Their purpose and usage
- That they are enterprise-only features
- Valid ranges/constraints (e.g., for ImpactValue)
Additionally, there's no visible enforcement mechanism in the code to ensure these fields are only populated in enterprise contexts. Consider documenting where this enforcement occurs or adding validation.
Apply this diff to add documentation:
+// DomainSelector represents a domain or tenant entity with optional enterprise features.
type DomainSelector struct {
Type string `json:"type"`
Name string `json:"name"`
ObjectID string `json:"id"`
Collected bool `json:"collected"`
+ // ImpactValue represents the impact value for this domain (Enterprise only).
ImpactValue *int `json:"impactValue,omitempty"`
+ // Exposures contains per-zone exposure information (Enterprise only).
Exposures Exposures `json:"exposures,omitempty"`
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Type string `json:"type"` | |
| Name string `json:"name"` | |
| ObjectID string `json:"id"` | |
| Collected bool `json:"collected"` | |
| ImpactValue *int `json:"impactValue,omitempty"` | |
| Exposures Exposures `json:"exposures,omitempty"` | |
| } | |
| // DomainSelector represents a domain or tenant entity with optional enterprise features. | |
| type DomainSelector struct { | |
| Type string `json:"type"` | |
| Name string `json:"name"` | |
| ObjectID string `json:"id"` | |
| Collected bool `json:"collected"` | |
| // ImpactValue represents the impact value for this domain (Enterprise only). | |
| ImpactValue *int `json:"impactValue,omitempty"` | |
| // Exposures contains per-zone exposure information (Enterprise only). | |
| Exposures Exposures `json:"exposures,omitempty"` | |
| } |
🤖 Prompt for AI Agents
In cmd/api/src/model/search.go around lines 38 to 44, the public fields
ImpactValue and Exposures are missing godoc and enterprise-only guidance; add
concise godoc comments above each field describing their purpose/usage, that
they are enterprise-only features, any constraints (e.g., ImpactValue valid
range like 0–100 or nullable semantics) and examples of when Exposures is
populated; also reference where enterprise enforcement/validation occurs (point
to the package/function that gates population) or add/annotate a validation
check to ensure these fields are only set in enterprise contexts.
| type Exposure struct { | ||
| ExposurePercent int `json:"exposure_percent"` | ||
| AssetGroupTag AssetGroupTag `json:"asset_group_tag"` | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Add documentation and consider validation for ExposurePercent.
The Exposure type has several concerns:
-
Missing documentation: This public type lacks godoc comments explaining its purpose and usage.
-
ExposurePercent validation: The field is an
intwith no apparent validation. Percentages typically range from 0-100. Consider:- Adding validation to ensure values are within valid range
- Using a pointer
*intif you need to distinguish between 0% exposure and unset/unknown
-
JSON naming inconsistency: This struct uses
snake_casefor JSON tags (exposure_percent,asset_group_tag), whileDomainSelectorusescamelCase(impactValue). Consider standardizing tocamelCasefor consistency.
Apply this diff to add documentation:
+// Exposure represents exposure information for a specific asset group zone (Enterprise only).
type Exposure struct {
+ // ExposurePercent is the exposure percentage (0-100).
ExposurePercent int `json:"exposure_percent"`
+ // AssetGroupTag identifies the asset group zone.
AssetGroupTag AssetGroupTag `json:"asset_group_tag"`
}Run the following script to verify the AssetGroupTag type exists and check for any existing validation:
🏁 Script executed:
#!/bin/bash
# Verify AssetGroupTag type exists and check for validation
# Find AssetGroupTag type definition
echo "=== Searching for AssetGroupTag type definition ==="
ast-grep --pattern 'type AssetGroupTag $_'
# Search for any validation on ExposurePercent
echo -e "\n=== Searching for ExposurePercent validation ==="
rg -nP --type=go '\bExposurePercent\b' -A5 -B5Length of output: 2200
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Verify DomainSelector JSON naming pattern and check model.go structure
echo "=== Searching for DomainSelector type definition ==="
rg -nP --type=go 'type DomainSelector struct' -A20
echo -e "\n=== Check for other struct naming patterns in search.go ==="
rg -nP --type=go 'type \w+ struct' cmd/api/src/model/search.go -A8Length of output: 1938
Add documentation and consider validation for ExposurePercent.
The Exposure type has the following concerns:
-
Missing documentation: This public type lacks godoc comments explaining its purpose and usage.
-
ExposurePercent validation: The field is an
intwith no apparent validation. Percentages typically range from 0-100. Consider:- Adding validation to ensure values are within valid range
- Using a pointer
*intif you need to distinguish between 0% exposure and unset/unknown
Apply this diff to add documentation:
+// Exposure represents exposure information for a specific asset group zone (Enterprise only).
type Exposure struct {
+ // ExposurePercent is the exposure percentage (0-100).
ExposurePercent int `json:"exposure_percent"`
+ // AssetGroupTag identifies the asset group zone.
AssetGroupTag AssetGroupTag `json:"asset_group_tag"`
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type Exposure struct { | |
| ExposurePercent int `json:"exposure_percent"` | |
| AssetGroupTag AssetGroupTag `json:"asset_group_tag"` | |
| } | |
| // Exposure represents exposure information for a specific asset group zone (Enterprise only). | |
| type Exposure struct { | |
| // ExposurePercent is the exposure percentage (0-100). | |
| ExposurePercent int `json:"exposure_percent"` | |
| // AssetGroupTag identifies the asset group zone. | |
| AssetGroupTag AssetGroupTag `json:"asset_group_tag"` | |
| } |
🤖 Prompt for AI Agents
In cmd/api/src/model/search.go around lines 48 to 51, add a godoc comment for
the Exposure type and its fields explaining its purpose and semantics, and
enforce/clarify valid values for ExposurePercent: either change ExposurePercent
to *int if you must distinguish unset vs 0, or keep it as int and add a
Validate() method (or validation in existing validation flow) that returns an
error if ExposurePercent is outside 0–100; ensure the godoc notes the chosen
representation (pointer vs value) and that valid range is 0–100 so callers know
to call/expect validation.
| AssetGroupTag AssetGroupTag `json:"asset_group_tag"` | ||
| } | ||
|
|
||
| type Exposures []Exposure |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add documentation for the Exposures type.
This public type alias lacks a godoc comment.
Apply this diff:
+// Exposures is a collection of exposure information across multiple asset group zones (Enterprise only).
type Exposures []Exposure📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type Exposures []Exposure | |
| // Exposures is a collection of exposure information across multiple asset group zones (Enterprise only). | |
| type Exposures []Exposure |
🤖 Prompt for AI Agents
In cmd/api/src/model/search.go around line 53, the exported type alias
`Exposures` lacks a godoc comment; add a concise godoc comment immediately above
the `type Exposures []Exposure` line that explains what Exposures represents
(e.g., a slice of Exposure objects used to represent multiple exposure
records/results) so the public type is documented for GoDoc.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/api/src/model/assetgrouptags.go (1)
186-188: Implementation is correct; consider adding documentation and tests.The logic correctly identifies tier-zero tags by comparing the position to
AssetGroupTierZeroPosition. The use ofValueOrZero()properly handles the nullablePositionfield.However, consider the following improvements:
- Add godoc comment: Public methods should document their purpose and behavior.
- Add unit tests: The PR states "Nothing to test here," but this new method has testable logic (position = 1, position = null, position ≠ 1). Simple unit tests would prevent future regressions.
Example documentation:
+// IsTierZero returns true if this asset group tag represents a Tier Zero asset. +// It checks whether the Position field equals AssetGroupTierZeroPosition (1). func (s AssetGroupTag) IsTierZero() bool { return s.Position.ValueOrZero() == AssetGroupTierZeroPosition }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/api/src/model/assetgrouptags.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/api/src/model/assetgrouptags.go (2)
packages/javascript/js-client-library/src/types.ts (1)
AssetGroupTag(99-110)packages/go/analysis/tiering/tiering.go (1)
IsTierZero(37-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: build-ui
- GitHub Check: run-tests
- GitHub Check: run-analysis
- GitHub Check: run-tests
Description
This change enables exposure values per zone in Enterprise.
Motivation and Context
Resolves BED-6601
Because the domain selectors model and the openAPI schema are part of the foss model, we need to update them here in order to enable exposure values per zone in enterprise. The new fields have been labeled "enterprise only" (since this is a limitation of the OpenAPI schema, we cannot actually mark them as enterprise only).
How Has This Been Tested?
Please describe in detail how you tested your changes.
Include details of your testing environment, and the tests you ran to
see how your change affects other areas of the code, etc.
Nothing to test here.
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit