From a1d91464ea6066030deb79aaf8b348bf9939fc39 Mon Sep 17 00:00:00 2001 From: Lucas Manuel Rodriguez Date: Thu, 28 May 2026 14:57:27 -0300 Subject: [PATCH] Fix issue with permissions in host activity list for fleet-users (#46362) **Related issue:** Resolves #46009. - [X] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. ## Testing - [X] Added/updated automated tests - [X] QA'd all new/changed functionality manually ## Summary by CodeRabbit * **Bug Fixes** * Resolved an authorization issue preventing users from viewing past host activities on hosts that contained user-initiated operations such as lock, wipe, run script, or install software. [![Review Change Stack](https://storage.googleapis.com/coderabbit_public_assets/review-stack-in-coderabbit-ui.svg)](https://app.coderabbit.ai/change-stack/fleetdm/fleet/pull/46362?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) --- ...09-fix-host-activity-user-enrichment-authz | 1 + cmd/fleet/serve.go | 6 +- server/acl/activityacl/fleet_adapter.go | 26 +++++-- server/datastore/mysql/mysqltest/mysqltest.go | 2 +- server/datastore/mysql/testing_utils_test.go | 2 +- server/service/integration_enterprise_test.go | 72 +++++++++++++++++++ server/service/svctest/server.go | 2 +- server/service/testing_utils_test.go | 2 +- 8 files changed, 101 insertions(+), 12 deletions(-) create mode 100644 changes/46009-fix-host-activity-user-enrichment-authz diff --git a/changes/46009-fix-host-activity-user-enrichment-authz b/changes/46009-fix-host-activity-user-enrichment-authz new file mode 100644 index 0000000000..8e587b9242 --- /dev/null +++ b/changes/46009-fix-host-activity-user-enrichment-authz @@ -0,0 +1 @@ +* Fixed Fleet-scoped users getting a 403 when viewing past activities on a host that has user-initiated activities (e.g. lock/wipe/run script/install software). diff --git a/cmd/fleet/serve.go b/cmd/fleet/serve.go index ad85bf7ad3..2eda3e8b14 100644 --- a/cmd/fleet/serve.go +++ b/cmd/fleet/serve.go @@ -1084,7 +1084,7 @@ func runServeCmd(cmd *cobra.Command, configManager configpkg.Manager, debug, dev logger.InfoContext(ctx, "instance info", "instanceID", instanceID) // Bootstrap activity bounded context (needed for cron schedules and HTTP routes) - activitySvc, activityRoutes := createActivityBoundedContext(svc, dbConns, logger) + activitySvc, activityRoutes := createActivityBoundedContext(svc, ds, dbConns, logger) // Inject the activity bounded context into the main service svc.SetActivityService(activitySvc) @@ -1978,13 +1978,13 @@ func initOrgLogoStore(ctx context.Context, s3Config configpkg.S3Config, logger * return store } -func createActivityBoundedContext(svc fleet.Service, dbConns *common_mysql.DBConnections, logger *slog.Logger) (activity_api.Service, endpointer.HandlerRoutesFunc) { +func createActivityBoundedContext(svc fleet.Service, ds fleet.Datastore, dbConns *common_mysql.DBConnections, logger *slog.Logger) (activity_api.Service, endpointer.HandlerRoutesFunc) { legacyAuthorizer, err := authz.NewAuthorizer() if err != nil { initFatal(err, "initializing activity authorizer") } activityAuthorizer := authz.NewAuthorizerAdapter(legacyAuthorizer) - activityACLAdapter := activityacl.NewFleetServiceAdapter(svc) + activityACLAdapter := activityacl.NewFleetServiceAdapter(svc, ds) activitySvc, activityRoutesFn := activity_bootstrap.New( dbConns, activityAuthorizer, diff --git a/server/acl/activityacl/fleet_adapter.go b/server/acl/activityacl/fleet_adapter.go index ae11a0b96c..4dbe70303b 100644 --- a/server/acl/activityacl/fleet_adapter.go +++ b/server/acl/activityacl/fleet_adapter.go @@ -13,15 +13,29 @@ import ( "github.com/fleetdm/fleet/v4/server/fleet" ) +// UsersByIDsLookup is the minimal interface needed to look up user summaries by ID +// without going through the service layer's authz. The activity bounded context +// performs its own authorization on the host before reaching the user-enrichment +// step, so a second authz check (which would reject team-scoped viewers since +// Service.UsersByIDs authorizes against an empty *fleet.User) is both incorrect +// and the cause of issue #46009 for hosts with user-initiated past activities. +type UsersByIDsLookup interface { + UsersByIDs(ctx context.Context, ids []uint) ([]*fleet.UserSummary, error) +} + // FleetServiceAdapter provides access to Fleet service methods // for data that the activity bounded context doesn't own. type FleetServiceAdapter struct { - svc fleet.ActivityLookupService + svc fleet.ActivityLookupService + usersByIDsDS UsersByIDsLookup } // NewFleetServiceAdapter creates a new adapter for the Fleet service. -func NewFleetServiceAdapter(svc fleet.ActivityLookupService) *FleetServiceAdapter { - return &FleetServiceAdapter{svc: svc} +// usersByIDsDS is a datastore-direct lookup used for activity user enrichment; +// it must bypass service-layer authz because the host authz check has already +// gated access to the activity list. +func NewFleetServiceAdapter(svc fleet.ActivityLookupService, usersByIDsDS UsersByIDsLookup) *FleetServiceAdapter { + return &FleetServiceAdapter{svc: svc, usersByIDsDS: usersByIDsDS} } // Ensure FleetServiceAdapter implements the required interfaces @@ -32,14 +46,16 @@ var ( _ activity.UpcomingActivityActivator = (*FleetServiceAdapter)(nil) ) -// UsersByIDs fetches users by their IDs from the Fleet service. +// UsersByIDs fetches users by their IDs from the datastore directly, +// bypassing service-layer authz. The activity service has already authorized +// the caller against the host before this is called for enrichment. func (a *FleetServiceAdapter) UsersByIDs(ctx context.Context, ids []uint) ([]*activity.User, error) { if len(ids) == 0 { return nil, nil } // Fetch only the requested users by their IDs - users, err := a.svc.UsersByIDs(ctx, ids) + users, err := a.usersByIDsDS.UsersByIDs(ctx, ids) if err != nil { return nil, err } diff --git a/server/datastore/mysql/mysqltest/mysqltest.go b/server/datastore/mysql/mysqltest/mysqltest.go index 2aa08cba45..100f2d12fa 100644 --- a/server/datastore/mysql/mysqltest/mysqltest.go +++ b/server/datastore/mysql/mysqltest/mysqltest.go @@ -903,7 +903,7 @@ func NewTestActivityService(t testing.TB, ds *mysql.Datastore) activity_api.Serv dbConns := TestDBConnections(t, ds) lookupSvc := &testingLookupService{ds: ds} - aclAdapter := activityacl.NewFleetServiceAdapter(lookupSvc) + aclAdapter := activityacl.NewFleetServiceAdapter(lookupSvc, ds) discardLogger := slog.New(slog.DiscardHandler) svc, _ := activity_bootstrap.New(dbConns, &testingAuthorizer{}, aclAdapter, discardLogger) diff --git a/server/datastore/mysql/testing_utils_test.go b/server/datastore/mysql/testing_utils_test.go index 1d805189c7..1e8c1178b2 100644 --- a/server/datastore/mysql/testing_utils_test.go +++ b/server/datastore/mysql/testing_utils_test.go @@ -930,7 +930,7 @@ func NewTestActivityService(t testing.TB, ds *Datastore) activity_api.Service { // Use the real ACL adapter with a testing lookup service lookupSvc := &testingLookupService{ds: ds} - aclAdapter := activityacl.NewFleetServiceAdapter(lookupSvc) + aclAdapter := activityacl.NewFleetServiceAdapter(lookupSvc, ds) // Create service via bootstrap (the public API for creating the bounded context) discardLogger := slog.New(slog.DiscardHandler) diff --git a/server/service/integration_enterprise_test.go b/server/service/integration_enterprise_test.go index 75736d8799..04dff8c210 100644 --- a/server/service/integration_enterprise_test.go +++ b/server/service/integration_enterprise_test.go @@ -43,6 +43,7 @@ import ( "github.com/fleetdm/fleet/v4/pkg/optjson" "github.com/fleetdm/fleet/v4/pkg/scripts" "github.com/fleetdm/fleet/v4/server" + activity_api "github.com/fleetdm/fleet/v4/server/activity/api" apiendpoints "github.com/fleetdm/fleet/v4/server/api_endpoints" "github.com/fleetdm/fleet/v4/server/config" "github.com/fleetdm/fleet/v4/server/contexts/installersize" @@ -31677,3 +31678,74 @@ func (s *integrationEnterpriseTestSuite) TestOrbitEnrollWithEUAToken() { require.Equal(t, idpEmail, dms[0].Email) require.Equal(t, fleet.DeviceMappingMDMIdpAccounts, dms[0].Source) } + +// TestTeamAdminCanReadHostPastActivities is a regression test for issue #46009: +// after the OPA-tag fix, team-scoped users were still getting 403 on +// GET /api/_version_/fleet/hosts/{id}/activities for any host with a +// user-initiated past activity (locked_host being the common reproducer). +// The cause was Service.UsersByIDs gating user enrichment behind an authz +// check on an empty *fleet.User, which no team role can satisfy. The activity +// ACL adapter now bypasses that check by going directly to the datastore. +func (s *integrationEnterpriseTestSuite) TestTeamAdminCanReadHostPastActivities() { + t := s.T() + ctx := context.Background() + + team, err := s.ds.NewTeam(ctx, &fleet.Team{ + Name: t.Name(), + Description: t.Name(), + }) + require.NoError(t, err) + t.Cleanup(func() { + require.NoError(t, s.ds.DeleteTeam(context.Background(), team.ID)) + }) + + teamAdminEmail := t.Name() + "_admin@example.com" + teamAdmin := &fleet.User{ + Name: teamAdminEmail, + Email: teamAdminEmail, + Teams: []fleet.UserTeam{ + {Team: *team, Role: fleet.RoleAdmin}, + }, + } + require.NoError(t, teamAdmin.SetPassword(test.GoodPassword, 10, 10)) + teamAdmin, err = s.ds.NewUser(ctx, teamAdmin) + require.NoError(t, err) + + host, err := s.ds.NewHost(ctx, &fleet.Host{ + DetailUpdatedAt: time.Now(), + LabelUpdatedAt: time.Now(), + PolicyUpdatedAt: time.Now(), + SeenTime: time.Now(), + OsqueryHostID: new(t.Name() + "_host"), + NodeKey: new(t.Name() + "_host"), + UUID: t.Name() + "_host", + Hostname: t.Name() + "_host.local", + Platform: "darwin", + TeamID: &team.ID, + }) + require.NoError(t, err) + + // Insert a host-linked past activity attributed to the team admin. This + // mirrors what happens when the team admin locks one of their hosts — + // the activity row has user_id set, which forces enrichWithUserData to + // call the user lookup. Before the fix, that lookup went through + // Service.UsersByIDs and 403'd the team admin viewing the activity feed. + activitySvc := mysqltest.NewTestActivityService(t, s.ds) + apiUser := &activity_api.User{ID: teamAdmin.ID, Name: teamAdmin.Name, Email: teamAdmin.Email} + require.NoError(t, activitySvc.NewActivity(ctx, apiUser, fleet.ActivityTypeLockedHost{ + HostID: host.ID, + HostDisplayName: host.DisplayName(), + })) + + t.Cleanup(func() { s.token = s.getTestAdminToken() }) + s.token = s.getTestToken(teamAdmin.Email, test.GoodPassword) + + var listResp listActivitiesResponse + s.DoJSON("GET", fmt.Sprintf("/api/latest/fleet/hosts/%d/activities", host.ID), nil, http.StatusOK, &listResp) + require.Len(t, listResp.Activities, 1) + require.Equal(t, fleet.ActivityTypeLockedHost{}.ActivityName(), listResp.Activities[0].Type) + require.NotNil(t, listResp.Activities[0].ActorEmail) + require.Equal(t, teamAdmin.Email, *listResp.Activities[0].ActorEmail) + require.NotNil(t, listResp.Activities[0].ActorFullName) + require.Equal(t, teamAdmin.Name, *listResp.Activities[0].ActorFullName) +} diff --git a/server/service/svctest/server.go b/server/service/svctest/server.go index ebe06d46b2..ae79db9685 100644 --- a/server/service/svctest/server.go +++ b/server/service/svctest/server.go @@ -92,7 +92,7 @@ func RunServerForTestsWithServiceWithDS(t *testing.T, ctx context.Context, ds fl legacyAuthorizer, err := authz.NewAuthorizer() require.NoError(t, err) activityAuthorizer := authz.NewAuthorizerAdapter(legacyAuthorizer) - activityACLAdapter := activityacl.NewFleetServiceAdapter(svc) + activityACLAdapter := activityacl.NewFleetServiceAdapter(svc, ds) activitySvc, activityRoutesFn := activity_bootstrap.New( opts[0].DBConns, activityAuthorizer, diff --git a/server/service/testing_utils_test.go b/server/service/testing_utils_test.go index ec8e7e8021..5cf5a4228b 100644 --- a/server/service/testing_utils_test.go +++ b/server/service/testing_utils_test.go @@ -441,7 +441,7 @@ func RunServerForTestsWithServiceWithDS(t *testing.T, ctx context.Context, ds fl legacyAuthorizer, err := authz.NewAuthorizer() require.NoError(t, err) activityAuthorizer := authz.NewAuthorizerAdapter(legacyAuthorizer) - activityACLAdapter := activityacl.NewFleetServiceAdapter(svc) + activityACLAdapter := activityacl.NewFleetServiceAdapter(svc, ds) activitySvc, activityRoutesFn := activity_bootstrap.New( opts[0].DBConns, activityAuthorizer,