Skip to content

test(analysis): guard service principal app id resolution against regressions - #3309

Open
sanjio3 wants to merge 1 commit into
SpecterOps:mainfrom
sanjio3:fix/1846-service-principal-appid
Open

sanjio3 wants to merge 1 commit into
SpecterOps:mainfrom
sanjio3:fix/1846-service-principal-appid

Conversation

@sanjio3

@sanjio3 sanjio3 commented Sep 13, 2026

Copy link
Copy Markdown

Closes #1846

Summary

The original report was that the Service Principal panel showed the Object ID where the App ID should be. While investigating, the production query already resolves via FetchStartNodes on the current main, so the underlying bug is fixed upstream. This PR adds regression tests to lock in the correct behavior and prevent future regressions.

Changes

  • Add 133 lines of regression tests guarding the Service Principal App ID resolution

Test plan

  • go test ./packages/go/analysis/ad/ -run TestServicePrincipal passes

Summary by CodeRabbit

  • Tests
    • Added coverage for Azure service principal analysis.
    • Verified that linked application nodes are returned correctly through service principal relationships.
    • Verified that application IDs are read from the linked application’s object ID.

…pecterOps#1846)

Signed-off-by: shaoyihan@zerozen.cn <shaoyihan@zerozen.cn>
@github-actions

github-actions Bot commented Sep 13, 2026

Copy link
Copy Markdown

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@coderabbitai

coderabbitai Bot commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Advanced

Run ID: d18582bd-7430-4498-9fe8-3650adfd5900

📥 Commits

Reviewing files that changed from the base of the PR and between a0076b5 and 3ba994c.

📒 Files selected for processing (1)
  • packages/go/analysis/azure/service_principal_test.go

Included review availability: Your plan provides up to 8 included reviews per hour; 4 remain after this review.


📝 Walkthrough

Walkthrough

The change adds two Azure analysis tests. The tests verify that service principal application lookup returns the linked application node and reads its ObjectID as the application ID.

Changes

Azure service principal analysis

Layer / File(s) Summary
Application lookup regression coverage
packages/go/analysis/azure/service_principal_test.go
Adds mocked graph transaction tests for FetchServicePrincipalApplications and getServicePrincipalAppID. The tests verify application-node traversal and retrieval of the application's ObjectID.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Other · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 3ba99

The change adds focused regression coverage for the Service Principal App ID behavior with no actionable merge risk identified.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the primary change: regression tests for Service Principal App ID resolution.
Description check ✅ Passed The description explains the issue, the regression-test scope, the linked issue, and the test command. It does not include the template's explicit types and checklist sections, but the required change…
Linked Issues check ✅ Passed The PR meets the coding objective in issue #1846. The reviewed production code reads the service principal App ID from the linked application node's common.ObjectID. The PR adds `TestFetchServicePri…
Out of Scope Changes check ✅ Passed The whole PR adds only packages/go/analysis/azure/service_principal_test.go. Both tests directly protect the App ID resolution behavior required by issue #1846. No unrelated production, API, or docu…
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@sanjio3

sanjio3 commented Sep 13, 2026

Copy link
Copy Markdown
Author

I have read the CLA Document and I hereby sign the CLA

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: Service Principal App ID node property displaying Service Principal Object ID instead of App ID

1 participant