fix: Expand number of entries included in ConvertAzureRoleEligibilityScheduleInstanceToRel relationships (BED-6391)#2935
Open
dcairnsspecterops wants to merge 4 commits into
Conversation
…duleInstanceToRel to widen eligible roles results list
Contributor
📝 WalkthroughWalkthrough
ChangesAzure Role Eligibility Scope Fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
packages/go/ein/azure.go (1)
1959-1974: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate the non-root scope test to match this widened behavior.
packages/go/ein/azure_test.go:33-61still expects zero relationships forDirectoryScopeId: "abc123", so this change leaves the downstream assertion stale. Please update that case to expect theAZRoleEligibleedge as well.Suggested test update
diff --git a/packages/go/ein/azure_test.go b/packages/go/ein/azure_test.go @@ expectedRels = ein.ConvertAzureRoleEligibilityScheduleInstanceToRel(testData) - require.Len(t, expectedRels, 0) + require.Len(t, expectedRels, 1) + expectedRel = expectedRels[0] + require.Equal(t, expectedRel.Target.Value, strings.ToUpper(fmt.Sprintf("%s@%s", testData.RoleDefinitionId, testData.TenantId))) + require.Equal(t, expectedRel.RelType, azure.AZRoleEligible) + require.Equal(t, expectedRel.Source.Value, strings.ToUpper(testData.PrincipalId))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/go/ein/azure.go` around lines 1959 - 1974, The relationship-building logic in the Azure ingestion path now emits an AZRoleEligible edge for non-root directory scopes, so the existing test expectation is outdated. Update the DirectoryScopeId "abc123" case in azure_test.go to assert the new relationship produced by the code path in the azure.go relationship construction, using the existing NewIngestibleRelationship/IngestibleEndpoint symbols as the reference point.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/go/ein/azure.go`:
- Around line 1959-1974: The relationship-building logic in the Azure ingestion
path now emits an AZRoleEligible edge for non-root directory scopes, so the
existing test expectation is outdated. Update the DirectoryScopeId "abc123" case
in azure_test.go to assert the new relationship produced by the code path in the
azure.go relationship construction, using the existing
NewIngestibleRelationship/IngestibleEndpoint symbols as the reference point.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 97d9b50b-869d-4468-81f1-35ef276840ed
📒 Files selected for processing (1)
packages/go/ein/azure.go
dcairnsspecterops
changed the title
… excluded from relationship processing
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/go/ein/azure_test.go`:
- Around line 63-79: Remove the stale zero-length assertion from
TestNonRootDirectoryScopeIdConvertAzureRoleEligibilityScheduleInstanceToRel so
the test only validates the single relationship returned by
ConvertAzureRoleEligibilityScheduleInstanceToRel for a non-root
DirectoryScopeId; keep the existing require.Len(..., 1) and the checks on
expectedRel.Target, expectedRel.RelType, and expectedRel.Source, and delete the
contradictory require.Len(..., 0) path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 709b21ee-6776-4162-b27f-c054907f2077
📒 Files selected for processing (1)
packages/go/ein/azure_test.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/go/ein/azure_test.go (1)
60-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the emitted relationship fields for the non-root case too.
This now proves non-root scopes are included, but it no longer verifies that the emitted relationship is still the expected
source -> AZRoleEligible -> target. Mirroring the root-case assertions here would keep the widened contract covered.Suggested test tightening
expectedRels = ein.ConvertAzureRoleEligibilityScheduleInstanceToRel(testData) require.Len(t, expectedRels, 1) + expectedRel = expectedRels[0] + require.Equal(t, expectedRel.Target.Value, strings.ToUpper(fmt.Sprintf("%s@%s", testData.RoleDefinitionId, testData.TenantId))) + require.Equal(t, expectedRel.RelType, azure.AZRoleEligible) + require.Equal(t, expectedRel.Source.Value, strings.ToUpper(testData.PrincipalId))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/go/ein/azure_test.go` around lines 60 - 61, The non-root case in ConvertAzureRoleEligibilityScheduleInstanceToRel is only checking the relationship count, so it no longer verifies the emitted source, type, and target fields. Tighten the azure_test.go assertion by mirroring the root-case checks for expectedRels[0] so the test confirms the relationship still resolves to source -> AZRoleEligible -> target while covering non-root scopes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/go/ein/azure_test.go`:
- Around line 60-61: The non-root case in
ConvertAzureRoleEligibilityScheduleInstanceToRel is only checking the
relationship count, so it no longer verifies the emitted source, type, and
target fields. Tighten the azure_test.go assertion by mirroring the root-case
checks for expectedRels[0] so the test confirms the relationship still resolves
to source -> AZRoleEligible -> target while covering non-root scopes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: a58ee2b6-9301-46ca-a6a7-4d226d3df70b
📒 Files selected for processing (1)
packages/go/ein/azure_test.go
dcairnsspecterops
changed the title
cweidenkeller
approved these changes
Jul 1, 2026
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
2 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.
Motivation and Context
A customer noticed that not all eligible user roles are being surfaced in their environment. Seems to be that is because in
ConvertAzureRoleEligibilityScheduleInstanceToRelwe skip the assignment of relationships if theDirectoryScopedis not"/"This PR removes that gate, assigning relationships for all instances, not just those whose
DirectoryScopedis/.Resolves BED-6391
Why is this change required? What problem does it solve?
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.
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
Bug Fixes
Tests