Add full name IdP Fleet variable to Apple configuration profiles - #32246
Conversation
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Inactive
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #32246 +/- ##
==========================================
- Coverage 64.04% 64.02% -0.02%
==========================================
Files 1988 1986 -2
Lines 194440 194407 -33
Branches 6514 6514
==========================================
- Hits 124521 124466 -55
- Misses 60221 60230 +9
- Partials 9698 9711 +13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
f547327 to
97d27ed
Compare
GitHub Actions
Inactive
MagnusHJensen
changed the title
GitHub Actions
Error
GitHub Actions
Inactive
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
WalkthroughAdds a new Fleet variable for IdP end-user full name, seeds it in the database, exposes a constant, updates Apple MDM profile preprocessing to validate/expand it and fail when missing, ensures SCIM-driven changes trigger profile resend, and adds tests. Schema seed and migration status are updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Admin
participant Fleet as Fleet Server
participant AppleMDM as Apple MDM Service
participant DS as Datastore
participant Host as macOS Host
Admin->>Fleet: Upload profile with $FLEET_VAR_HOST_END_USER_IDP_FULL_NAME
Fleet->>AppleMDM: Preprocess profile contents
AppleMDM->>DS: Fetch host end-user IdP data (includes IdpFullName)
alt IdpFullName present/non-empty
AppleMDM->>AppleMDM: Substitute fullname in profile
AppleMDM-->>Host: Install profile
else missing fullname
AppleMDM->>DS: Mark profile DeliveryFailed (detail: no IdP fullname)
AppleMDM-->>Admin: Report failed status for host
end
sequenceDiagram
autonumber
participant IdP as Identity Provider
participant SCIM as SCIM Ingest
participant DS as Datastore
participant AppleMDM as Apple MDM Service
participant Hosts as Affected Hosts
IdP-->>SCIM: User fullname updated
SCIM->>DS: Store updated IdpFullName
SCIM->>AppleMDM: triggerResendProfilesUsingVariables([… , HOST_END_USER_IDP_FULL_NAME])
AppleMDM->>Hosts: Re-deliver affected profiles
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNone found. Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/datastore/mysql/scim.go (1)
1170-1182: Resend will not trigger when only full name (given/family name) changes.
triggerResendProfilesForIDPUserChangenow includesFleetVarHostEndUserIDPFullname, butReplaceScimUseronly checksuser_nameanddepartmentdeltas before calling it. If onlygiven_nameand/orfamily_namechange, profiles that use full name won’t be resent.Proposed fix: include
given_nameandfamily_namein the “changed” check.@@ - // load the username and department before updating the user, to check if it changed - old := struct { - UserName string `db:"user_name"` - Department *string `db:"department"` - }{} - err := sqlx.GetContext(ctx, tx, &old, `SELECT user_name, department FROM scim_users WHERE id = ?`, user.ID) + // load fields we track for downstream profile resend triggers + old := struct { + UserName string `db:"user_name"` + Department *string `db:"department"` + GivenName *string `db:"given_name"` + FamilyName *string `db:"family_name"` + }{} + err := sqlx.GetContext(ctx, tx, &old, `SELECT user_name, department, given_name, family_name FROM scim_users WHERE id = ?`, user.ID) @@ - usernameChanged := old.UserName != user.UserName - departmentChanged := !cmp.Equal(old.Department, user.Department) + usernameChanged := old.UserName != user.UserName + departmentChanged := !cmp.Equal(old.Department, user.Department) + givenNameChanged := !cmp.Equal(old.GivenName, user.GivenName) + familyNameChanged := !cmp.Equal(old.FamilyName, user.FamilyName) @@ - // resend profiles that depend on this username if it changed - if usernameChanged || departmentChanged { + // resend profiles that depend on IdP user fields if any changed + if usernameChanged || departmentChanged || givenNameChanged || familyNameChanged { err = triggerResendProfilesForIDPUserChange(ctx, tx, user.ID) if err != nil { return err } }This ensures
$FLEET_VAR_HOST_END_USER_IDP_FULL_NAMEupdates propagate automatically.
🧹 Nitpick comments (6)
changes/308888-add-fullname-idp-fleet-variable (1)
1-1: Name the variable explicitly and mention license + failure behavior.Consider being explicit about the variable and UX, so release notes are self-contained.
Apply this diff:
-* Added IdP fullname attribute as a valid fleet variable for Apple configuration profiles +* Add $FLEET_VAR_HOST_END_USER_IDP_FULL_NAME to Apple configuration profiles (Fleet Premium). + If a host's IdP full name is missing, profile delivery is marked Failed with a clear error message.server/service/apple_mdm_test.go (1)
5077-5163: Add a validation test to assert the new variable is “allowed” in profile parsing.You already test preprocessing; suggest adding one small case to
TestValidateConfigProfileFleetVariablesto ensure$FLEET_VAR_HOST_END_USER_IDP_FULL_NAMEis detected/allowed by validation. This guards against regressions in the allowlist/regex.Example addition inside
TestValidateConfigProfileFleetVariablescases:// Add to cases in TestValidateConfigProfileFleetVariables: { name: "Custom profile with IdP full name var", profile: string(scopedMobileconfigForTest( "FullName Var", "com.example.fullname", nil, "HOST_END_USER_IDP_FULL_NAME", // will be prefixed to $FLEET_VAR_ by helper )), errMsg: "", vars: []string{"HOST_END_USER_IDP_FULL_NAME"}, },I can open a follow-up PR to wire this in if helpful.
server/datastore/mysql/schema.sql (1)
1460-1463: Migration status entry is consistent; consider adding a reversible Down() for the seed (optional).
- AUTO_INCREMENT=414 with the last row (id 413, version 20250825113751) lines up with the new migration.
- Optional: For reversibility, implement a Down_20250825113751 that deletes the inserted fleet_variables row by name. Many seed-like migrations in this repo are no-op on Down, so this is a nice-to-have for symmetry, not a blocker.
If you’d like, I can open a follow-up PR to add a reversible Down(). Would you prefer to keep seed Downs as no-op to match precedent?
server/service/apple_mdm.go (3)
5162-5165: Trim whitespace when substituting full nameIf IdP returns " First Last " or similar, you probably don’t want those spaces persisted into XML. Safe to trim before replacement.
Apply:
- case string(fleet.FleetVarHostEndUserIDPFullname): - rx = fleetVarHostEndUserIDPFullnameRegexp - value = user.IdpFullName + case string(fleet.FleetVarHostEndUserIDPFullname): + rx = fleetVarHostEndUserIDPFullnameRegexp + value = strings.TrimSpace(user.IdpFullName)Also applies to: 5189-5193
5391-5391: User-facing message: “full name” (two words) for consistencyOther messages use natural phrasing (“email”, “groups”, “department”). Recommend “full name” over “fullname” in the error string.
- noFullnameErr := fmt.Sprintf("There is no IdP fullname for this host. Fleet couldn’t populate $FLEET_VAR_%s.", fleet.FleetVarHostEndUserIDPFullname) + noFullnameErr := fmt.Sprintf("There is no IdP full name for this host. Fleet couldn’t populate $FLEET_VAR_%s.", fleet.FleetVarHostEndUserIDPFullname)
5421-5433: Trim whitespace-only “full name” valuesDefensive check: if the IdP supplies only whitespace, treat it as empty so hosts fail with a clear message (aligns with “missing or empty” acceptance).
--- a/server/service/apple_mdm.go +++ b/server/service/apple_mdm.go @@ -5421,7 +5421,7 @@ func someFunction(...) { - if fleetVar == string(fleet.FleetVarHostEndUserIDPFullname) && idpUser.IdpFullName == "" { + if fleetVar == string(fleet.FleetVarHostEndUserIDPFullname) && strings.TrimSpace(idpUser.IdpFullName) == "" { err = ds.UpdateOrDeleteHostMDMAppleProfile(ctx, &fleet.HostMDMAppleProfile{ CommandUUID: target.cmdUUID, HostUUID: hostUUID,Note: the SCIM ingestion’s
ScimUser.DisplayName()returns a non-empty string when eitherGivenNameorFamilyNameis present (so single-word legal names are still allowed) and only returns""when both are absent. Addingstrings.TrimSpace(...)here ensures that purely whitespace values are treated as missing, without affecting valid single-name cases.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
-
changes/308888-add-fullname-idp-fleet-variable(1 hunks) -
server/datastore/mysql/migrations/tables/20250825113751_AddIdpFullnameFleetVariable.go(1 hunks) -
server/datastore/mysql/schema.sql(2 hunks) -
server/datastore/mysql/scim.go(3 hunks) -
server/fleet/mdm.go(1 hunks) -
server/service/apple_mdm.go(7 hunks) -
server/service/apple_mdm_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
When reviewing SQL queries that are added or modified, ensure that appropriate filtering criteria are applied—especially when a query is intended to return data for a specific entity (e.g., a single host). Check for missing WHERE clauses or incorrect filtering that could lead to incorrect or non-deterministic results (e.g., returning the first row instead of the correct one). Flag any queries that may return unintended results due to lack of precise scoping.
Files:
server/fleet/mdm.goserver/datastore/mysql/scim.goserver/datastore/mysql/migrations/tables/20250825113751_AddIdpFullnameFleetVariable.goserver/service/apple_mdm.goserver/service/apple_mdm_test.go
🧠 Learnings (3)
📚 Learning: 2025-07-08T16:13:39.114Z
Learnt from: getvictor
PR: fleetdm/fleet#30589
File: server/datastore/mysql/migrations/tables/20250707095725_HostIdentitySCEPCertificates.go:53-55
Timestamp: 2025-07-08T16:13:39.114Z
Learning: In the Fleet codebase, Down migration functions are intentionally left empty/no-op. The team does not implement rollback functionality for database migrations, so empty Down_* functions in migration files are correct and should not be flagged as issues.
Applied to files:
server/datastore/mysql/migrations/tables/20250825113751_AddIdpFullnameFleetVariable.go
📚 Learning: 2025-08-08T08:32:31.529Z
Learnt from: getvictor
PR: fleetdm/fleet#31695
File: server/datastore/mysql/apple_mdm_test.go:132-132
Timestamp: 2025-08-08T08:32:31.529Z
Learning: Datastore.NewMDMWindowsConfigProfile signature is: NewMDMWindowsConfigProfile(ctx context.Context, cp fleet.MDMWindowsConfigProfile, usesFleetVars []string) (*fleet.MDMWindowsConfigProfile, error). Passing nil for usesFleetVars in tests denotes “no Fleet variables referenced” and is used consistently across the repo.
Applied to files:
server/service/apple_mdm.go
📚 Learning: 2025-08-01T15:08:16.858Z
Learnt from: sgress454
PR: fleetdm/fleet#31508
File: server/datastore/mysql/schema.sql:102-116
Timestamp: 2025-08-01T15:08:16.858Z
Learning: The schema.sql file in server/datastore/mysql/ is auto-generated from migrations for use with tests, so it cannot be manually edited. Any changes must be made through migrations.
Applied to files:
server/datastore/mysql/schema.sql
🔇 Additional comments (9)
server/fleet/mdm.go (1)
49-51: LGTM: new Fleet variable constant is consistent with existing naming.The identifier and value align with existing Host End User IdP variables and match the struct field
Fullnameused elsewhere. No issues.server/datastore/mysql/scim.go (2)
1189-1197: Good: include Full Name in affected vars for user deleted.This correctly ensures profiles using full name are resent (and likely fail gracefully) when the IdP user is removed.
Please confirm there’s no separate email-triggered resend requirement when SCIM emails change (current code doesn’t resend on email-only updates); out of scope for this PR, but worth tracking if expected.
1231-1250: Good: include Full Name in affected vars for user added to host.Covers the “user associated to host” path so profiles using full name are resent.
server/service/apple_mdm_test.go (1)
5077-5163: Nice coverage for full name: First+Last, only first, only last, and missing.The scenarios align with expected substitution and failure behavior, and mirror the style of the existing IdP variable tests.
server/datastore/mysql/schema.sql (1)
326-329: The above script will confirm whether the migration definesUp_<version>andDown_<version>functions matching its filename prefix, and show context to verify naming conventions. Once we have those results, we can finalize whether the schema snapshot truly aligns with the generated migration.server/service/apple_mdm.go (4)
81-81: Add regexp for new IdP full name variable — looks correctPattern matches both
$FLEET_VAR_* and $ {FLEET_VAR_*} forms and uses the new constant. No issues spotted.
4936-4941: Validation phase: treating full name like other IdP fields is fineNo extra preprocessing validation needed here because missing/empty is enforced later during per-host expansion. Good placement.
5448-5450: No-IdP-user fallback detail: coveredNew case funnels to noFullnameErr; behavior matches other IdP fields. Looks good.
84-88: End-to-End Verification Complete: IDP Full Name Fully IntegratedAll required pieces for the new
HostEndUserIDPFullnamevariable are in place and wired end-to-end:
- The
fleet.FleetVarHostEndUserIDPFullnameconstant is referenced throughout the code (e.g. inapple_mdm_test.go), confirming its definition in the fleet package.- The MySQL schema seed (
server/datastore/mysql/schema.sql) and the migration (AddIdpFullnameFleetVariable.go) insertFLEET_VAR_HOST_END_USER_IDP_FULL_NAME.- The SCIM resend logic in
server/datastore/mysql/scim.goincludesfleet.FleetVarHostEndUserIDPFullnamein its trigger lists.server/service/apple_mdm_test.goexercises both success and error scenarios for the full-name variable (Fullname: "Some User"and the “no IdP fullname” error).No further changes are required here.
JordanMontgomery
left a comment
JordanMontgomery
left a comment
There was a problem hiding this comment.
Agree with coderabbit re: migration timestamp otherwise looks good to me
Actually its comment re: resends is good too. I didn't realize we added that functionality but it sounds like it may need to be updated |
GitHub Actions
Inactive
GitHub Actions
Failure
2 participants
fixes: #30888
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing
Added/updated automated tests
QA'd all new/changed functionality manually
Database migrations
Summary by CodeRabbit