updated default profile, added endpoint for seeing what default is applied - #44236
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
GitHub Actions
Inactive
| if user != nil && user.HasAnyTeamRole() { | ||
| // Check each team role permission, since if the user has just one team level permission, they can use this endpoint | ||
| var authorized bool | ||
| var authErr error | ||
| for _, tm := range user.Teams { | ||
| err := svc.authz.Authorize(ctx, &fleet.MDMAppleSetupAssistant{TeamID: &tm.ID}, fleet.ActionRead) | ||
| if err == nil { | ||
| // Early skip, since we only need one team with permission | ||
| authorized = true | ||
| break | ||
| } | ||
|
|
||
| authErr = err | ||
| } | ||
|
|
||
| if !authorized { | ||
| return godep.Profile{}, nil, authErr | ||
| } |
There was a problem hiding this comment.
I don't think I've seen this pattern elsewhere in the codebase.
Let me know if it exists and it doesn't align, I was thinking of a helper, but ended up just doing it inline for now, lmk. if we should put it in a helper.
There was a problem hiding this comment.
Might be a good one for backend sync? I agree not sure if I've seen this pattern elsewhere either
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a GET endpoint 🚥 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 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}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
server/service/integration_mdm_dep_test.go (1)
3447-3462: Use test-unique teams/users to avoid cross-test collisions.
ensureTeamExistscan return pre-existing teams, but cleanup always deletes them. Combined with static team names/emails, this can create flaky or state-coupled integration runs.♻️ Suggested hardening
- ensureTeamExists := func(teamName string) *fleet.Team { - team, err := s.ds.TeamByName(t.Context(), teamName) - if err == nil { - require.NoError(t, err) - return team - } - - team, err = s.ds.NewTeam(t.Context(), &fleet.Team{Name: teamName}) - require.NoError(t, err) - return team - } - extraTeamName := "extra-team" - extraTeam := ensureTeamExists(extraTeamName) - defaultDEPTeamName := "default-dep-profile" - defaultDEPTeam := ensureTeamExists(defaultDEPTeamName) + extraTeamName := fmt.Sprintf("%s-extra-%s", t.Name(), uuid.NewString()) + extraTeam, err := s.ds.NewTeam(t.Context(), &fleet.Team{Name: extraTeamName}) + require.NoError(t, err) + defaultDEPTeamName := fmt.Sprintf("%s-default-dep-%s", t.Name(), uuid.NewString()) + defaultDEPTeam, err := s.ds.NewTeam(t.Context(), &fleet.Team{Name: defaultDEPTeamName}) + require.NoError(t, err) @@ - email := "team_observer@example.com" + email := fmt.Sprintf("team_observer_%s@example.com", uuid.NewString()) @@ - email := "team_maintainer@example.com" + email := fmt.Sprintf("team_maintainer_%s@example.com", uuid.NewString())Also applies to: 3464-3466, 3486-3488, 3510-3513
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/integration_mdm_dep_test.go` around lines 3447 - 3462, The helper ensureTeamExists currently looks up and may return pre-existing teams (used for extraTeam/defaultDEPTeam with static names extra-team/default-dep-profile), which combined with cleanup that deletes by name can cause cross-test collisions; modify the test to generate and use test-unique team names (e.g., append t.Name(), a timestamp, or a random suffix) when setting extraTeamName and defaultDEPTeamName and/or change ensureTeamExists to always create a new team using a unique name rather than returning an existing one; also apply the same uniqueness strategy to any static user emails referenced at 3464-3466, 3486-3488, 3510-3513 so cleanup only touches entities created by this test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@changes/40905-update-default-automatic-enrollment-profile`:
- Line 1: The PR description and any user-facing text currently state users can
"download" the default automatic enrollment profile; change wording to "view" or
"retrieve" to accurately reflect that the change exposes the applied profile via
a GET endpoint (e.g., the new GET default-profile endpoint), updating the PR
title/description and any README/UX strings that mention "download" to "view" or
"retrieve" the default automatic enrollment profile so we don't overpromise
behavior.
In `@ee/server/service/mdm.go`:
- Around line 786-793: The code assumes profile.DEPProfile is non-nil before
calling json.Unmarshal(*profile.DEPProfile, &godepProfile), which will panic if
DEPProfile is nil; fix by adding a guard that checks if profile.DEPProfile ==
nil and return an appropriate error (e.g., ctxerr.New or ctxerr.Wrap) before
attempting to dereference it, preserving the existing return types and error
semantics around godep.Profile, the variable godepProfile, and the
json.Unmarshal call.
In `@server/service/apple_mdm.go`:
- Around line 3388-3401: The response struct
getDefaultMDMAppleSetupAssistantProfileResponse currently serializes the
timestamp as "uploaded_at"; change the json struct tag on the UploadedAt field
to "updated_at" so API consumers receive "updated_at" while leaving the db tag
alone if needed (i.e., make UploadedAt `json:"updated_at" db:"uploaded_at"`); no
other behavioral changes to Error() or
getDefaultMDMAppleSetupAssistantProfileEndpoint are required.
---
Nitpick comments:
In `@server/service/integration_mdm_dep_test.go`:
- Around line 3447-3462: The helper ensureTeamExists currently looks up and may
return pre-existing teams (used for extraTeam/defaultDEPTeam with static names
extra-team/default-dep-profile), which combined with cleanup that deletes by
name can cause cross-test collisions; modify the test to generate and use
test-unique team names (e.g., append t.Name(), a timestamp, or a random suffix)
when setting extraTeamName and defaultDEPTeamName and/or change ensureTeamExists
to always create a new team using a unique name rather than returning an
existing one; also apply the same uniqueness strategy to any static user emails
referenced at 3464-3466, 3486-3488, 3510-3513 so cleanup only touches entities
created by this test.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 51033d79-7f0b-48b2-b24e-0718d9ffe04e
📒 Files selected for processing (11)
changes/40905-update-default-automatic-enrollment-profileee/server/service/mdm.goserver/api_endpoints/api_endpoints.ymlserver/fleet/service.goserver/mdm/apple/apple_mdm.goserver/mdm/apple/apple_mdm_test.goserver/mock/service/service_mock.goserver/service/apple_mdm.goserver/service/apple_mdm_test.goserver/service/handler.goserver/service/integration_mdm_dep_test.go
There was a problem hiding this comment.
Pull request overview
Updates Fleet’s Apple Automatic Enrollment (DEP) defaults and introduces a new REST endpoint to retrieve the effective default automatic enrollment profile (either stored in DB or the in-code default for fresh installs), with accompanying test coverage.
Changes:
- Updated the in-code default DEP profile and exposed it via an exported
DEPService.GetDefaultProfile. - Added
GET /api/v1/fleet/enrollment_profiles/automatic/defaultto return the default automatic enrollment profile plus its timestamp (or null when using in-code defaults). - Added unit/integration tests and registered the endpoint in the API endpoints list.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
server/mdm/apple/apple_mdm.go |
Updates and exports the default DEP profile generator used for “default automatic enrollment profile” behavior. |
server/mdm/apple/apple_mdm_test.go |
Updates test to use the exported GetDefaultProfile API. |
server/service/handler.go |
Wires the new default-profile endpoint into the authenticated Apple MDM route group. |
server/service/apple_mdm.go |
Adds endpoint/response plumbing and the OSS (missing license) stub service method. |
ee/server/service/mdm.go |
Implements the premium service behavior for returning either the stored default profile or the in-code default. |
server/service/integration_mdm_dep_test.go |
Adds integration coverage for no-row/row-exists cases and authorization expectations. |
server/service/apple_mdm_test.go |
Adds free-license gating unit test for the new service method. |
server/fleet/service.go |
Extends the service interface with GetDefaultMDMAppleSetupAssistantProfile. |
server/mock/service/service_mock.go |
Updates the service mock to support the new method. |
server/api_endpoints/api_endpoints.yml |
Registers the new API endpoint for endpoint listing/docs tooling. |
changes/40905-update-default-automatic-enrollment-profile |
Adds user-visible changelog entry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #44236 +/- ##
==========================================
+ Coverage 66.72% 66.78% +0.06%
==========================================
Files 2627 2627
Lines 211185 211224 +39
Branches 9422 9422
==========================================
+ Hits 140908 141070 +162
+ Misses 57486 57327 -159
- Partials 12791 12827 +36
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:
|
GitHub Actions
Inactive
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ee/server/service/mdm.go (1)
781-784: Tighten the "can never be null" assumption.
*svc.depService.GetDefaultProfile()dereferences the returned pointer; the comment asserts non-nil but there's no guard. SinceGetDefaultProfileis now an exported method onDEPService, a future change to its body (or a misconfigureddepServiceinjection in tests) would turn this into a panic on the request path. A cheap defensive check or returning a value (not pointer) fromGetDefaultProfilewould remove the foot-gun:🛡️ Optional defensive guard
if fleet.IsNotFound(err) { - // This can never be null - return *svc.depService.GetDefaultProfile(), nil, nil + def := svc.depService.GetDefaultProfile() + if def == nil { + return godep.Profile{}, nil, ctxerr.New(ctx, "default DEP profile is unexpectedly nil") + } + return *def, nil, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/server/service/mdm.go` around lines 781 - 784, The code dereferences the pointer returned by svc.depService.GetDefaultProfile() without checking for nil which can panic; add a defensive nil check: call prof := svc.depService.GetDefaultProfile(); if prof == nil { return dep.Profile{}, nil, nil } (or return an explicit error) instead of directly returning *svc.depService.GetDefaultProfile(); alternatively convert DEPService.GetDefaultProfile to return a non-pointer value to eliminate the need to dereference. Ensure you update any callers/tests to match the chosen approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ee/server/service/mdm.go`:
- Around line 781-784: The code dereferences the pointer returned by
svc.depService.GetDefaultProfile() without checking for nil which can panic; add
a defensive nil check: call prof := svc.depService.GetDefaultProfile(); if prof
== nil { return dep.Profile{}, nil, nil } (or return an explicit error) instead
of directly returning *svc.depService.GetDefaultProfile(); alternatively convert
DEPService.GetDefaultProfile to return a non-pointer value to eliminate the need
to dereference. Ensure you update any callers/tests to match the chosen
approach.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ffb84406-4633-479c-a6b0-dd9bb7673a13
📒 Files selected for processing (4)
ee/server/service/mdm.goserver/fleet/service.goserver/service/apple_mdm.goserver/service/integration_mdm_dep_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- server/fleet/service.go
- server/service/integration_mdm_dep_test.go
GitHub Actions
Inactive
GitHub Actions
Error
GitHub Actions
Inactive
There was a problem hiding this comment.
🧹 Nitpick comments (2)
server/service/integration_mdm_dep_test.go (2)
3447-3457: SimplifyensureTeamExistsand stop swallowing non-"not found" errors.The
require.NoError(t, err)inside theif err == nilbranch is dead —erris already known to benilthere. More importantly, the current shape treats anyTeamByNameerror (e.g. a real DB error) as "team is missing" and falls through toNewTeam, which can mask bugs and produce confusing duplicate-key failures on the create path. Prefer narrowing onfleet.IsNotFound(err).♻️ Proposed refactor
ensureTeamExists := func(teamName string) *fleet.Team { team, err := s.ds.TeamByName(t.Context(), teamName) - if err == nil { - require.NoError(t, err) - return team - } + if err == nil { + return team + } + require.True(t, fleet.IsNotFound(err), "unexpected error from TeamByName: %v", err) team, err = s.ds.NewTeam(t.Context(), &fleet.Team{Name: teamName}) require.NoError(t, err) return team }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/integration_mdm_dep_test.go` around lines 3447 - 3457, The helper ensureTeamExists currently treats any error from s.ds.TeamByName as "not found" and proceeds to create a team, masking real DB errors and containing a dead require.NoError; change it to first check if err == nil and return team, then if fleet.IsNotFound(err) call s.ds.NewTeam and return that, otherwise call require.NoError(t, err) (or require.FailNow with the error) to fail the test on unexpected errors; update references to TeamByName, NewTeam, ensureTeamExists and use fleet.IsNotFound(err) to distinguish missing-team vs real errors.
3473-3498: Nit: inconsistent zero-*boolconstruction.Line 3473 uses
new(false)(Go 1.26 expression form, evaluates to*bool→ false) while line 3498 usesnew(bool)(zero value, also false) for the sameAdminForcedPasswordResetfield. Both are valid but mixing them in adjacent code is needlessly confusing — pick one (the more common idiom in this repo isptr.Bool(false)ornew(bool)).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/integration_mdm_dep_test.go` around lines 3473 - 3498, The AdminForcedPasswordReset field is using inconsistent pointer-boolean construction (new(false) vs new(bool)); update the occurrence inside the createUserRequest / UserPayload block to match the repo idiom (use new(bool) or ptr.Bool(false)) so both instances are the same; modify the new(false) at the AdminForcedPasswordReset assignment to new(bool) (or ptr.Bool(false)) to be consistent with the other occurrence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/service/integration_mdm_dep_test.go`:
- Around line 3447-3457: The helper ensureTeamExists currently treats any error
from s.ds.TeamByName as "not found" and proceeds to create a team, masking real
DB errors and containing a dead require.NoError; change it to first check if err
== nil and return team, then if fleet.IsNotFound(err) call s.ds.NewTeam and
return that, otherwise call require.NoError(t, err) (or require.FailNow with the
error) to fail the test on unexpected errors; update references to TeamByName,
NewTeam, ensureTeamExists and use fleet.IsNotFound(err) to distinguish
missing-team vs real errors.
- Around line 3473-3498: The AdminForcedPasswordReset field is using
inconsistent pointer-boolean construction (new(false) vs new(bool)); update the
occurrence inside the createUserRequest / UserPayload block to match the repo
idiom (use new(bool) or ptr.Bool(false)) so both instances are the same; modify
the new(false) at the AdminForcedPasswordReset assignment to new(bool) (or
ptr.Bool(false)) to be consistent with the other occurrence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ddff5690-4748-4fd5-a777-880ca32684a7
📒 Files selected for processing (2)
server/service/apple_mdm.goserver/service/integration_mdm_dep_test.go
JordanMontgomery
left a comment
JordanMontgomery
left a comment
There was a problem hiding this comment.
One minor comment but good to merge I think
| if user != nil && user.HasAnyTeamRole() { | ||
| // Check each team role permission, since if the user has just one team level permission, they can use this endpoint | ||
| var authorized bool | ||
| var authErr error | ||
| for _, tm := range user.Teams { | ||
| err := svc.authz.Authorize(ctx, &fleet.MDMAppleSetupAssistant{TeamID: &tm.ID}, fleet.ActionRead) | ||
| if err == nil { | ||
| // Early skip, since we only need one team with permission | ||
| authorized = true | ||
| break | ||
| } | ||
|
|
||
| authErr = err | ||
| } | ||
|
|
||
| if !authorized { | ||
| return godep.Profile{}, nil, authErr | ||
| } |
There was a problem hiding this comment.
Might be a good one for backend sync? I agree not sure if I've seen this pattern elsewhere either
### Needs #44236 <!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves #43790 <img width="1109" height="511" alt="image" src="https://github.com/user-attachments/assets/256560ee-0d70-4fff-b553-37e46224a54a" /> # 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](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. Added in backend PR - [x] Input data is properly validated, `SELECT *` is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters. - [x] Timeouts are implemented and retries are limited to avoid infinite loops - [x] If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes ## Testing - [ ] Added/updated automated tests - [x] QA'd all new/changed functionality manually <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Setup Assistant now fetches and shows a default Apple enrollment profile when a team profile is missing, including its loading state before showing the uploader. * **User-facing behavior** * Default profile can be viewed and downloaded immediately; download uses a fixed filename and formatted JSON. * **Documentation** * Added a "Learn more" link to the Setup Assistant section. * **Style** * Default profile card uses a distinct background, smaller description text, and hides the delete action. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Related issue: Resolves #43789
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/oree/fleetd-chrome/changes.See Changes files for more information.
Input data is properly validated,
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.Timeouts are implemented and retries are limited to avoid infinite loops
If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes
Testing
Summary by CodeRabbit
New Features
Access
Tests