Added support for fetching delegated admin partner information#199
Open
Paradoxis wants to merge 1 commit into
Open
Added support for fetching delegated admin partner information#199Paradoxis wants to merge 1 commit into
Paradoxis wants to merge 1 commit into
Conversation
|
I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
WalkthroughThis PR adds Azure AD partner enumeration support to AzureHound. It introduces a new ChangesPartner Enumeration Feature
Sequence DiagramsequenceDiagram
participant partnersCmd as partners command
participant client as AzureGraphClient
participant output as output channel
participant caller as caller
partnersCmd->>client: ListAzureADPartners(ctx, params)
client->>client: getAzureObjectList goroutine
client->>output: stream Partner items
partnersCmd->>client: GetAzureADTenantInfoById(ctx, tenantId)
client->>output: emit normalized AZTenant
partnersCmd->>client: ListAzureADRoleDefinitions
client->>output: stream role definitions
partnersCmd->>client: ListAzureADRoleAssignments(ctx, filter, expand)
client->>output: stream principal assignments
partnersCmd->>output: close channel on completion
output->>caller: stream AZTenant and AzureWrapper events
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Paradoxis
changed the title
Author
|
I have read the CLA Document and I hereby sign the CLA |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
models/tenant.go (1)
25-25: ⚡ Quick winKeep
externalconsistently present in tenant JSON.On Line 25,
omitemptydropsexternalfor internal tenants (false), creating mixed shapes between internal and delegated tenant objects.♻️ Proposed change
- External bool `json:"external,omitempty"` + External bool `json:"external"`🤖 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 `@models/tenant.go` at line 25, The JSON tag for the Tenant struct's External field (External bool) currently uses `omitempty`, causing the key to be omitted when false; remove `omitempty` from the `json` tag on the External field in the Tenant struct so the `external` key is always emitted (i.e., change `External bool `json:"external,omitempty"` to `External bool `json:"external"`), then run gofmt/build to ensure no other code relies on the missing key.
🤖 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 `@client/client.go`:
- Around line 195-196: The AzureClient interface was expanded (e.g.,
GetAzureADTenantInfoById and other AzureGraphClient-related additions) and
existing mocks no longer implement client.AzureClient; regenerate the mocks so
mocks.MockAzureClient matches the updated interface. Run your mock-generation
tool (e.g., mockgen or the project's generator) targeting the client.AzureClient
interface (including methods like GetAzureADTenantInfoById and any
AzureGraphClient methods) to overwrite/update mocks.MockAzureClient, then run
tests to ensure the mock satisfies the new interface.
In `@cmd/list-partners.go`:
- Around line 87-89: The tenant lookup error handler logs the failure but then
continues and adds a zero-value tenant into partnerTenants; change the error
branch in the loop that calls the external tenant retrieval (the block that logs
using log.Error with "failed to retrieve tenant information for external
partner" and references partner.Ok.CompanyName / partner.Ok.PartnerTenantId) to
return early from that iteration (e.g., skip inserting into partnerTenants) by
continuing the loop after logging. Apply the same fix to the similar
error-handling blocks later in the same loop (the region covering lines ~91-105)
so that when err != nil you do not populate partnerTenants with an empty tenant
entry.
- Around line 212-214: The goroutine currently uses break when
pipeline.SendAny(ctx.Done(), out, AzureWrapper{Kind: kind, Data: data}) returns
false, which only exits the inner role-assignment loop and allows the outer loop
to continue; change this to stop the goroutine immediately (e.g., return from
the goroutine) when SendAny indicates cancellation so no further iterations or
sends occur. Locate the call to pipeline.SendAny and replace the inner-loop-only
break with an immediate function return (or otherwise break out of all loops) to
ensure that on ctx cancellation no more AzureWrapper sends or outer-loop work is
performed. Ensure any necessary deferred cleanup still runs after returning.
---
Nitpick comments:
In `@models/tenant.go`:
- Line 25: The JSON tag for the Tenant struct's External field (External bool)
currently uses `omitempty`, causing the key to be omitted when false; remove
`omitempty` from the `json` tag on the External field in the Tenant struct so
the `external` key is always emitted (i.e., change `External bool
`json:"external,omitempty"` to `External bool `json:"external"`), then run
gofmt/build to ensure no other code relies on the missing key.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f25eb27b-af43-4995-8c4b-2c5d49836a3b
📒 Files selected for processing (10)
client/client.goclient/partners.goclient/role_assignments.goclient/tenants.gocmd/list-azure-ad.gocmd/list-partners.gocmd/list-tenants.gomodels/azure/partner.gomodels/azure/unified_role_assignment.gomodels/tenant.go
Comment on lines
+195
to
196
| GetAzureADTenantInfoById(ctx context.Context, tenantId string) (azure.Tenant, error) | ||
|
|
There was a problem hiding this comment.
Regenerate mocks after expanding AzureGraphClient.
The interface changes at Line 195 and Line 206 currently break type-checking in tests because mocks.MockAzureClient no longer implements client.AzureClient.
Also applies to: 206-206
🤖 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 `@client/client.go` around lines 195 - 196, The AzureClient interface was
expanded (e.g., GetAzureADTenantInfoById and other AzureGraphClient-related
additions) and existing mocks no longer implement client.AzureClient; regenerate
the mocks so mocks.MockAzureClient matches the updated interface. Run your
mock-generation tool (e.g., mockgen or the project's generator) targeting the
client.AzureClient interface (including methods like GetAzureADTenantInfoById
and any AzureGraphClient methods) to overwrite/update mocks.MockAzureClient,
then run tests to ensure the mock satisfies the new interface.
Comment on lines
+87
to
+89
| if err != nil { | ||
| log.Error(err, "failed to retrieve tenant information for external partner", "companyName", partner.Ok.CompanyName, "partnerTenantId", partner.Ok.PartnerTenantId) | ||
| } |
There was a problem hiding this comment.
Skip emission when external tenant lookup fails.
After the error at Line 87, execution continues and emits a zero-value tenant (e.g., Id becomes /tenants/) and inserts an empty-key entry into partnerTenants.
🐛 Proposed fix
externalTenant, err := client.GetAzureADTenantInfoById(ctx, partner.Ok.PartnerTenantId)
if err != nil {
log.Error(err, "failed to retrieve tenant information for external partner", "companyName", partner.Ok.CompanyName, "partnerTenantId", partner.Ok.PartnerTenantId)
+ continue
}Also applies to: 91-105
🤖 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 `@cmd/list-partners.go` around lines 87 - 89, The tenant lookup error handler
logs the failure but then continues and adds a zero-value tenant into
partnerTenants; change the error branch in the loop that calls the external
tenant retrieval (the block that logs using log.Error with "failed to retrieve
tenant information for external partner" and references partner.Ok.CompanyName /
partner.Ok.PartnerTenantId) to return early from that iteration (e.g., skip
inserting into partnerTenants) by continuing the loop after logging. Apply the
same fix to the similar error-handling blocks later in the same loop (the region
covering lines ~91-105) so that when err != nil you do not populate
partnerTenants with an empty tenant entry.
Comment on lines
+212
to
+214
| if ok := pipeline.SendAny(ctx.Done(), out, AzureWrapper{Kind: kind, Data: data}); !ok { | ||
| break | ||
| } |
There was a problem hiding this comment.
Return on canceled output instead of only breaking inner loop.
At Line 212, if pipeline.SendAny fails, break only exits the current role-assignment loop. The goroutine can continue with outer iterations and extra calls despite cancellation.
🐛 Proposed fix
if ok := pipeline.SendAny(ctx.Done(), out, AzureWrapper{Kind: kind, Data: data}); !ok {
- break
+ return
}📝 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.
Suggested change
| if ok := pipeline.SendAny(ctx.Done(), out, AzureWrapper{Kind: kind, Data: data}); !ok { | |
| break | |
| } | |
| if ok := pipeline.SendAny(ctx.Done(), out, AzureWrapper{Kind: kind, Data: data}); !ok { | |
| return | |
| } |
🤖 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 `@cmd/list-partners.go` around lines 212 - 214, The goroutine currently uses
break when pipeline.SendAny(ctx.Done(), out, AzureWrapper{Kind: kind, Data:
data}) returns false, which only exits the inner role-assignment loop and allows
the outer loop to continue; change this to stop the goroutine immediately (e.g.,
return from the goroutine) when SendAny indicates cancellation so no further
iterations or sends occur. Locate the call to pipeline.SendAny and replace the
inner-loop-only break with an immediate function return (or otherwise break out
of all loops) to ensure that on ctx cancellation no more AzureWrapper sends or
outer-loop work is performed. Ensure any necessary deferred cleanup still runs
after returning.
Author
|
recheck |
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.
This PR adds support for collection of tenants, users, groups and service principals created by delegated admin partners (DAP/GDAP) in Entra ID.
I ran into an issue at a client where AzureHound failed to list information linked to role attachments and thought I'd share my solution with you guys.
I'm not sure if this really is the way to go since it's quite hacky, as you can't just easily go and fetch information from other tenants without going through some
$expandhoops (see the big comment in the code itself). This implementation is quite slow and could break on large environments.I was originally planning on adding a
Partnernode but I think it would be better to add a custom relationship (i.e.:AZPartnerOf/AZManagedBy) in Bloodhound itself that shows the tenant has a trust relationship with the other tenants.If you guys have any other suggestions how I could better implement this I'd be happy to change stuff up. Feel free to edit anything if you want
Summary by CodeRabbit
New Features
Updates