Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add RemoveApplication method #25078

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

jspenc72
Copy link

@jspenc72 jspenc72 commented Jan 1, 2025

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.
  • Added/updated tests
  • Manual QA for all new/changed functionality

@iansltx
Copy link
Member

iansltx commented Jan 1, 2025

@jspenc72 Thanks for working on this! I just dropped the checklist down to what should be applicable here once this feature is fleshed out, as I don't think you'll need database migrations or BC bcreaking API changes to make this work.

I believe the next step here is to update the uninstall logic to queue up this profile when interacting with an iOS/iPadOS target rather than failing, and then add tests covering the new behavior. Final piece would be to expose this functionality in the web UI.

Get as far as you can; it's a question of when rather than if on picking up where you left off and finishing this feature, but my guess is that this would hit in 4.64.0 depending on what's left to build by the time product works through that UI tweaks need to happen here.

You'll likely have to rebase/merge from main soonish as we have some failing tests on main that went red yesterday that I'll be working on fixing today.

Adding myself as a watcher on this until it's prioritized, so I'll see any pushes/questions as they come up.

@jspenc72 jspenc72 force-pushed the jspenc72/25077-apple-mdm-remove-application branch from 66cac37 to a82a062 Compare January 2, 2025 08:31
Copy link

codecov bot commented Jan 2, 2025

Codecov Report

Attention: Patch coverage is 27.14286% with 51 lines in your changes missing coverage. Please review.

Project coverage is 63.81%. Comparing base (7ac39e2) to head (48ffe26).

Files with missing lines Patch % Lines
server/mdm/apple/commander.go 35.48% 20 Missing ⚠️
ee/server/service/software_installers.go 26.92% 17 Missing and 2 partials ⚠️
...ages/hosts/details/cards/Software/HostSoftware.tsx 0.00% 12 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #25078      +/-   ##
==========================================
+ Coverage   54.30%   63.81%   +9.50%     
==========================================
  Files        1615     1615              
  Lines      153594   153657      +63     
  Branches     4004     3952      -52     
==========================================
+ Hits        83416    98049   +14633     
+ Misses      63296    47803   -15493     
- Partials     6882     7805     +923     
Flag Coverage Δ
backend 64.68% <31.57%> (+10.38%) ⬆️
frontend 53.23% <7.69%> (-0.05%) ⬇️

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

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

@jspenc72
Copy link
Author

jspenc72 commented Jan 2, 2025

Hey FleetDM friends! this pr is working as intended but the action is not yet recorded in the database as it should be.

A little guidance on how to record the uninstall request would be much appreciated.

@iansltx

Rebased as per your suggestion. The latest commit successfully removed the application via apple mdm push notifications.

I manually QA'd this on iOS and iPadOS devices.

Copy link
Member

@getvictor getvictor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution. I made a few comments.

As @iansltx mentioned, we can probably take this over and finish the work sometime in the next couple of months, as this is an important feature for Fleet.

Comment on lines 1078 to 1090
if host.OrbitNodeKey == nil || *host.OrbitNodeKey == "" {
// fleetd is required to install software so if the host is enrolled via plain osquery we return an error
svc.authz.SkipAuthorization(ctx)
return fleet.NewUserMessageError(errors.New("host does not have fleetd installed"), http.StatusUnprocessableEntity)
// fleetd is required to install software so if the host is enrolled via plain osquery we
// return an error
// Handle iOS and iPadOS devices
if !strings.Contains(host.OSVersion, "iPadOS") && !strings.Contains(host.OSVersion, "iOS") {
svc.authz.SkipAuthorization(ctx)
return fleet.NewUserMessageError(errors.New("host does not have fleetd installed"), http.StatusUnprocessableEntity)
} else {
// For iOS and iPadOS devices, we don't need fleetd to install/uninstall software
// We can proceed with the uninstallation
fmt.Printf("iOS and iPadOS devices don't need fleetd to install/uninstall software, proceed to uninstall via Apple MDM")
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use the same validation here as in the InstallSoftwareTitle method:

	platform := host.FleetPlatform()
	mobileAppleDevice := fleet.AppleDevicePlatform(platform) == fleet.IOSPlatform || fleet.AppleDevicePlatform(platform) == fleet.IPadOSPlatform

	if !mobileAppleDevice && (host.OrbitNodeKey == nil || *host.OrbitNodeKey == "") {
		// fleetd is required to install software so if the host is
		// enrolled via plain osquery we return an error
		svc.authz.SkipAuthorization(ctx)
		return fleet.NewUserMessageError(errors.New("Host doesn't have fleetd installed"), http.StatusUnprocessableEntity)
	}

@@ -1091,6 +1100,24 @@ func (svc *Service) UninstallSoftwareTitle(ctx context.Context, hostID uint, sof
if err := svc.authz.Authorize(ctx, &fleet.HostSoftwareInstallerResultAuthz{HostTeamID: host.TeamID}, fleet.ActionWrite); err != nil {
return err
}
// Try Apple MDM uninstallation for iOS and iPadOS devices

if strings.Contains(host.OSVersion, "iPadOS") || strings.Contains(host.OSVersion, "iOS") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this work for macOS as well?

Comment on lines -487 to +488
argErr *fleet.InvalidArgumentError) *fleet.InvalidArgumentError {
argErr *fleet.InvalidArgumentError,
) *fleet.InvalidArgumentError {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any way you can remove these formatting changes? Your IDE should have a setting something like: only format modified lines.

return ctxerr.Wrapf(ctx, err, "sending command to uninstall VPP %s application to host with serial %s", vppApp.BundleIdentifier, host.HardwareSerial)
}
return nil
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When MDM command result comes back, we need to process it, similar to what we do for InstallApplication:

case "InstallApplication":

Update the database

We need to add uninstall column to host_vpp_software_installs and use similar pattern as we do with script-based software installs:

func (ds *Datastore) InsertSoftwareUninstallRequest(ctx context.Context, executionID string, hostID uint, softwareInstallerID uint) error {

Need to update the vppAppJoin method similar to softwareInstallerJoin:

func (ds *Datastore) vppAppJoin(appID fleet.VPPAppID, status fleet.SoftwareInstallerStatus) (string, []interface{}, error) {

Add host activity record

Should be able to use ActivityTypeUninstalledSoftware:

type ActivityTypeUninstalledSoftware struct {

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

Successfully merging this pull request may close these issues.

3 participants