Attribute ai_tools Windows rows from ProfileList, not the directory owner

[juan-fdz-hawa]

Resolves #51061

A profile directory's security-descriptor OWNER is an ACL field naming whoever administers the object, not the profile's user — on an administrator's profile it is commonly BUILTIN\Administrators. That put the group SID S-1-5-32-544 in uid and left username blank on every user-scoped row.

Enumerate HKLM...\ProfileList instead, which is where Windows records which account a profile belongs to, so a home arrives with its SID already attached. The key is admin-only-writable, preserving the anti-spoofing property the owner read was chosen for. The owner read stays as a fallback for homes the key does not record, now gated on the SID naming a user account so a group or well-known SID reports empty rather than a misleading value.

Checklist for submitter

If some of the following don't apply, delete the relevant line.

• Changes file added for user-visible changes in changes/, orbit/changes/ or ee/fleetd-chrome/changes.
  See Changes files for more information.

Testing

• Added/updated automated tests
• QA'd all new/changed functionality manually

Summary by CodeRabbit

• Bug Fixes
  • Improved home-directory discovery and deduplication across supported platforms.
  • Improved ownership detection on Unix systems, including graceful handling of unresolved accounts.
  • On Windows, profile directories are now validated more reliably, including path, account type, drive, and on-disk ownership checks.
  • Prevented invalid, redirected, reparse-point, or non-user profile paths from being incorrectly reported as home directories.
• Tests
  • Added comprehensive coverage for Windows profile discovery and ownership scenarios.

[codecov]

Codecov Report

❌ Patch coverage is 18.18182% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.65%. Comparing base (4fa2352) to head (4e346da).
⚠️ Report is 12 commits behind head on main.

Files with missing lines	Patch %	Lines
orbit/pkg/table/ai_tools/internal/homes/homes.go	0.00%	24 Missing ⚠️
...it/pkg/table/ai_tools/internal/homes/owner_unix.go	66.66%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #51083      +/-   ##
==========================================
+ Coverage   68.56%   68.65%   +0.08%     
==========================================
  Files        3993     3994       +1     
  Lines      257379   257558     +179     
  Branches    13684    13684              
==========================================
+ Hits       176484   176821     +337     
+ Misses      65189    65018     -171     
- Partials    15706    15719      +13     

Flag	Coverage Δ
backend	69.74% <18.18%> (+0.09%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[coderabbitai]

Walkthrough

The homes package now centralizes path validation and deduplication. Platform-recorded homes are added before scanned directories. Unix ownership returns UID and best-effort username values. Windows discovery reads ProfileList entries, filters user SIDs and local paths, expands and canonicalizes paths, and verifies on-disk directories. Windows tests cover ownership attribution, profile validation, SID filtering, redirected profiles, unresolved accounts, and verification order.

Possibly related PRs

• fleetdm/fleet#49243 — Introduced the homes package whose discovery and ownership helpers this change revises.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly describes the primary Windows ProfileList attribution change.
Description check	✅ Passed	The description identifies issue #51061, explains the fix, and records automated testing and the changes file.
Linked Issues check	✅ Passed	The changes satisfy issue #51061 by using ProfileList attribution, filtering non-user SIDs, retaining a fallback, and adding Windows tests.
Out of Scope Changes check	✅ Passed	The production and test changes are directly related to Windows home ownership attribution and issue #51061.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch 51061-ai_tools-on-windows-blank-name-reported

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

🧹 Nitpick comments (2)

orbit/pkg/table/ai_tools/internal/homes/homes_windows_test.go (1)

204-219: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Optional: cover the lowercase drive letter branch.

isLocalDriveAbs accepts both 'A'..'Z' and 'a'..'z'. The rejection cases here and the accepted paths in TestProfileHomes all use an uppercase drive letter, so the lowercase half of that condition is never exercised. A single accepted entry with c:\Users\xpkoa would pin the branch.

💚 Proposed addition

 func TestProfileHomesRejectsNonLocalPaths(t *testing.T) {
 	lookup := accountLookup(map[string]string{sidLocalUser: "xpkoa"})
 
+	// A lowercase drive letter is a valid local path.
+	checkProfileHomes(t, []profileEntry{{SID: sidLocalUser, Path: `c:\Users\xpkoa`}},
+		lookup, diskAlwaysConsistent, []Home{{UID: sidLocalUser, Username: "xpkoa", Dir: `c:\Users\xpkoa`}})
+
 	for _, path := range []string{

🤖 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 `@orbit/pkg/table/ai_tools/internal/homes/homes_windows_test.go` around lines
204 - 219, Add a lowercase-drive accepted case to TestProfileHomes, using
c:\Users\xpkoa for the local user entry, so the lowercase branch of
isLocalDriveAbs is exercised while preserving the existing assertions and test
structure.

orbit/pkg/table/ai_tools/internal/homes/owner_windows.go (1)

103-119: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Optional: express userAccountForSID through resolveUserAccount.

Both functions run the same two steps: the isNonUserSID value gate, then LookupAccount("") with a SidTypeUser check. The only difference is the treatment of a failed lookup, and userAccountForSID maps that to false, which equals res != resolvedUser. One implementation removes the risk that the two gates drift apart later.

♻️ Proposed consolidation

 func userAccountForSID(sid *windows.SID) (string, bool) {
-	if isNonUserSID(sid.String()) {
-		return "", false
-	}
-	account, _, accType, err := sid.LookupAccount("")
-	if err != nil || accType != windows.SidTypeUser {
-		return "", false
-	}
-	return account, true
+	account, res := resolveUserAccount(sid.String())
+	return account, res == resolvedUser
 }

🤖 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 `@orbit/pkg/table/ai_tools/internal/homes/owner_windows.go` around lines 103 -
119, Optionally consolidate userAccountForSID with resolveUserAccount by reusing
resolveUserAccount’s non-user SID gate and account lookup/type validation,
mapping unresolved results to an empty account and false. Preserve
userAccountForSID’s bare account-name return contract and avoid duplicating the
validation logic.

🤖 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 `@orbit/pkg/table/ai_tools/internal/homes/homes_windows_test.go`:
- Around line 204-219: Add a lowercase-drive accepted case to TestProfileHomes,
using c:\Users\xpkoa for the local user entry, so the lowercase branch of
isLocalDriveAbs is exercised while preserving the existing assertions and test
structure.

In `@orbit/pkg/table/ai_tools/internal/homes/owner_windows.go`:
- Around line 103-119: Optionally consolidate userAccountForSID with
resolveUserAccount by reusing resolveUserAccount’s non-user SID gate and account
lookup/type validation, mapping unresolved results to an empty account and
false. Preserve userAccountForSID’s bare account-name return contract and avoid
duplicating the validation logic.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: ddf86a4f-e3dc-4594-95f7-84980adc7a5d

📥 Commits

Reviewing files that changed from the base of the PR and between 308d9ed and 4e346da.

⛔ Files ignored due to path filters (1)

• orbit/pkg/table/ai_tools/README.md is excluded by !**/*.md

📒 Files selected for processing (6)

• orbit/pkg/table/ai_tools/internal/homes/homes.go
• orbit/pkg/table/ai_tools/internal/homes/homes_windows_test.go
• orbit/pkg/table/ai_tools/internal/homes/owner_other.go
• orbit/pkg/table/ai_tools/internal/homes/owner_unix.go
• orbit/pkg/table/ai_tools/internal/homes/owner_unix_test.go
• orbit/pkg/table/ai_tools/internal/homes/owner_windows.go