Skip to content

Commit

Permalink
Added Android activity and better handling of deleted users. (#26640)
Browse files Browse the repository at this point in the history
For #26218

- Added `users_deleted` table to track user actions if the user was
actually deleted.
- Added enable/disable Android MDM activities

Note: I could not auto-generate fleet.Service mock because it has issues
with methods that don't return anything. I ended up using testify mock
instead.

# Checklist for submitter

- [x] If database migrations are included, checked table schema to
confirm autoupdate
- For database migrations:
- [x] Checked schema for all modified table for columns that will
auto-update timestamps during migration.
- [x] Confirmed that updating the timestamps is acceptable, and will not
cause unwanted side effects.
- [x] Ensured the correct collation is explicitly set for character
columns (`COLLATE utf8mb4_unicode_ci`).
- [x] Added/updated automated tests
- [x] Manual QA for all new/changed functionality
  • Loading branch information
getvictor authored Feb 27, 2025
1 parent a4a2182 commit 67b7276
Show file tree
Hide file tree
Showing 24 changed files with 255 additions and 39 deletions.
1 change: 1 addition & 0 deletions cmd/fleet/serve.go
Original file line number Diff line number Diff line change
Expand Up @@ -750,6 +750,7 @@ the way that the Fleet server works.
ctx,
logger,
ds,
svc,
)
if err != nil {
initFatal(err, "initializing android service")
Expand Down
12 changes: 12 additions & 0 deletions docs/Contributing/Audit-logs.md
Original file line number Diff line number Diff line change
Expand Up @@ -906,6 +906,18 @@ Generated when a user turns off MDM features for all Windows hosts.

This activity does not contain any detail fields.

## enabled_android_mdm

Generated when a user turns on MDM features for all Android hosts.

This activity does not contain any detail fields.

## disabled_android_mdm

Generated when a user turns off MDM features for all Android hosts.

This activity does not contain any detail fields.

## enabled_windows_mdm_migration

Generated when a user enables automatic MDM migration for Windows hosts, if Windows MDM is turned on.
Expand Down
2 changes: 1 addition & 1 deletion server/datastore/mysql/activities.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ func (ds *Datastore) NewActivity(
// To support creating activities with users that were deleted. This can happen
// for automatically installed software which uses the author of the upload as the author of
// the installation.
if user.ID != 0 {
if user.ID != 0 && !user.Deleted {
userID = &user.ID
}
userName = &user.Name
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package tables

import (
"database/sql"
"fmt"
)

func init() {
MigrationClient.AddMigration(Up_20250226153445, Down_20250226153445)
}

func Up_20250226153445(tx *sql.Tx) error {
_, err := tx.Exec(`
CREATE TABLE IF NOT EXISTS users_deleted (
-- matches users.id, which is an auto-incrementing primary key
id int unsigned NOT NULL,
name varchar(255) COLLATE utf8mb4_unicode_ci NOT NULL DEFAULT '',
email varchar(255) COLLATE utf8mb4_unicode_ci NOT NULL,
created_at DATETIME(6) NULL DEFAULT NOW(6),
updated_at DATETIME(6) NULL DEFAULT NOW(6) ON UPDATE NOW(6),
PRIMARY KEY (id)
)`)
if err != nil {
return fmt.Errorf("create users_deleted table: %w", err)
}

_, err = tx.Exec(`
ALTER TABLE android_enterprises
-- user that created the enterprise
ADD COLUMN user_id int unsigned NOT NULL DEFAULT 0`)
if err != nil {
return fmt.Errorf("add user_id to android_enterprise table: %w", err)
}

return nil
}

func Down_20250226153445(_ *sql.Tx) error {
return nil
}
16 changes: 14 additions & 2 deletions server/datastore/mysql/schema.sql

Large diffs are not rendered by default.

38 changes: 38 additions & 0 deletions server/datastore/mysql/users.go
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,30 @@ func (ds *Datastore) UserByID(ctx context.Context, id uint) (*fleet.User, error)
return ds.findUser(ctx, "id", id)
}

func (ds *Datastore) UserOrDeletedUserByID(ctx context.Context, id uint) (*fleet.User, error) {
user, err := ds.findUser(ctx, "id", id)
switch {
case fleet.IsNotFound(err):
return ds.deletedUserByID(ctx, id)
case err != nil:
return nil, ctxerr.Wrap(ctx, err, "find user")
}
return user, nil
}

func (ds *Datastore) deletedUserByID(ctx context.Context, id uint) (*fleet.User, error) {
stmt := `SELECT id, name, email, 1 as deleted FROM users_deleted WHERE id = ?`
var user fleet.User
err := sqlx.GetContext(ctx, ds.reader(ctx), &user, stmt, id)
switch {
case errors.Is(err, sql.ErrNoRows):
return nil, ctxerr.Wrap(ctx, notFound("deleted user").WithID(id))
case err != nil:
return nil, ctxerr.Wrap(ctx, err, "selecting deleted user")
}
return &user, nil
}

func (ds *Datastore) SaveUser(ctx context.Context, user *fleet.User) error {
return ds.withTx(ctx, func(tx sqlx.ExtContext) error {
return saveUserDB(ctx, tx, user)
Expand Down Expand Up @@ -308,6 +332,20 @@ func saveTeamsForUserDB(ctx context.Context, tx sqlx.ExtContext, user *fleet.Use

// DeleteUser deletes the associated user
func (ds *Datastore) DeleteUser(ctx context.Context, id uint) error {
// Transfer user data to deleted_users table for audit/activity purposes
stmt := `
INSERT INTO users_deleted (id, name, email)
SELECT u.id, u.name, u.email
FROM users AS u
WHERE u.id = ?
ON DUPLICATE KEY UPDATE
name = u.name,
email = u.email`
_, err := ds.writer(ctx).ExecContext(ctx, stmt, id)
if err != nil {
return ctxerr.Wrap(ctx, err, "populate users_deleted entry")
}

return ds.deleteEntity(ctx, usersTable, id)
}

Expand Down
20 changes: 20 additions & 0 deletions server/datastore/mysql/users_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ func TestUsers(t *testing.T) {
}{
{"Create", testUsersCreate},
{"ByID", testUsersByID},
{"Delete", testUsersDelete},
{"Save", testUsersSave},
{"Has", testUsersHas},
{"List", testUsersList},
Expand Down Expand Up @@ -134,6 +135,25 @@ func createTestUsers(t *testing.T, ds fleet.Datastore) []*fleet.User {
return users
}

func testUsersDelete(t *testing.T, ds *Datastore) {
_, err := ds.UserOrDeletedUserByID(context.Background(), 999999)
var nfe fleet.NotFoundError
assert.ErrorAs(t, err, &nfe)

users := createTestUsers(t, ds)
for _, tt := range users {
err := ds.DeleteUser(context.Background(), tt.ID)
assert.Nil(t, err)
_, err = ds.UserByID(context.Background(), tt.ID)
var nfe fleet.NotFoundError
assert.ErrorAs(t, err, &nfe)
returned, err := ds.UserOrDeletedUserByID(context.Background(), tt.ID)
require.NoError(t, err)
assert.Equal(t, tt.ID, returned.ID)
assert.True(t, returned.Deleted)
}
}

func testUsersSave(t *testing.T, ds *Datastore) {
users := createTestUsers(t, ds)
testUserGlobalRole(t, ds, users)
Expand Down
4 changes: 2 additions & 2 deletions server/docs/patterns.md
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,8 @@ MySQL. [Backend sync where discussed](https://us-65885.app.gong.io/call?id=80410
### Data retention

Sometimes we need data from rows that have been deleted from DB. For example, the activity feed may be retained forever, and it needs user info (or host info) that may not exist anymore.
Going forward, we need to keep this data in a dedicated table(s). A reference unmerged PR is [here](https://github.com/fleetdm/fleet/pull/17472/files#diff-57a635e42320a87dd15a3ae03d66834f2cbc4fcdb5f3ebb7075d966b96f760afR16).
The `id` may be the same as that of the original table. For example, if the `user` row is deleted, a new entry with the same `user.id` can be added to `user_persistent_info`.
We need to keep this data in dedicated table(s). When a `user` row is deleted, a new entry with the same `user.id` is added to `users_deleted` table. The user info can be retrieved using
`ds.UserOrDeletedUserByID` method.

### Re-usable transactionable functions

Expand Down
16 changes: 16 additions & 0 deletions server/fleet/activities.go
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,8 @@ var ActivityDetailsList = []ActivityDetails{

ActivityTypeEnabledWindowsMDM{},
ActivityTypeDisabledWindowsMDM{},
ActivityTypeEnabledAndroidMDM{},
ActivityTypeDisabledAndroidMDM{},
ActivityTypeEnabledWindowsMDMMigration{},
ActivityTypeDisabledWindowsMDMMigration{},

Expand Down Expand Up @@ -2190,3 +2192,17 @@ func (a ActivityEditedNDESSCEPProxy) ActivityName() string {
func (a ActivityEditedNDESSCEPProxy) Documentation() (activity string, details string, detailsExample string) {
return "Generated when NDES SCEP proxy configuration is edited in Fleet.", `This activity does not contain any detail fields.`, ``
}

type ActivityTypeEnabledAndroidMDM struct{}

func (a ActivityTypeEnabledAndroidMDM) ActivityName() string { return "enabled_android_mdm" }
func (a ActivityTypeEnabledAndroidMDM) Documentation() (activity string, details string, detailsExample string) {
return "Generated when a user turns on MDM features for all Android hosts.", `This activity does not contain any detail fields.`, ``
}

type ActivityTypeDisabledAndroidMDM struct{}

func (a ActivityTypeDisabledAndroidMDM) ActivityName() string { return "disabled_android_mdm" }
func (a ActivityTypeDisabledAndroidMDM) Documentation() (activity string, details string, detailsExample string) {
return "Generated when a user turns off MDM features for all Android hosts.", `This activity does not contain any detail fields.`, ``
}
1 change: 1 addition & 0 deletions server/fleet/datastore.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ type Datastore interface {
ListUsers(ctx context.Context, opt UserListOptions) ([]*User, error)
UserByEmail(ctx context.Context, email string) (*User, error)
UserByID(ctx context.Context, id uint) (*User, error)
UserOrDeletedUserByID(ctx context.Context, id uint) (*User, error)
SaveUser(ctx context.Context, user *User) error
SaveUsers(ctx context.Context, users []*User) error
// DeleteUser permanently deletes the user identified by the provided ID.
Expand Down
1 change: 1 addition & 0 deletions server/fleet/users.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ type User struct {
Teams []UserTeam `json:"teams"`

Settings *UserSettings `json:"settings,omitempty"`
Deleted bool `json:"-" db:"deleted"`
}

type UserSettings struct {
Expand Down
1 change: 1 addition & 0 deletions server/mdm/android/android.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ type EnterpriseDetails struct {
SignupName string `db:"signup_name"`
SignupToken string `db:"signup_token"`
TopicID string `db:"pubsub_topic_id"`
UserID uint `db:"user_id"`
}

type EnrollmentToken struct {
Expand Down
2 changes: 1 addition & 1 deletion server/mdm/android/datastore.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import (
)

type Datastore interface {
CreateEnterprise(ctx context.Context) (uint, error)
CreateEnterprise(ctx context.Context, userID uint) (uint, error)
GetEnterpriseByID(ctx context.Context, ID uint) (*EnterpriseDetails, error)
GetEnterprise(ctx context.Context) (*Enterprise, error)
UpdateEnterprise(ctx context.Context, enterprise *EnterpriseDetails) error
Expand Down
6 changes: 3 additions & 3 deletions server/mdm/android/mock/datastore.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import (

var _ android.Datastore = (*Datastore)(nil)

type CreateEnterpriseFunc func(ctx context.Context) (uint, error)
type CreateEnterpriseFunc func(ctx context.Context, userID uint) (uint, error)

type GetEnterpriseByIDFunc func(ctx context.Context, ID uint) (*android.EnterpriseDetails, error)

Expand Down Expand Up @@ -56,11 +56,11 @@ type Datastore struct {
mu sync.Mutex
}

func (ds *Datastore) CreateEnterprise(ctx context.Context) (uint, error) {
func (ds *Datastore) CreateEnterprise(ctx context.Context, userID uint) (uint, error) {
ds.mu.Lock()
ds.CreateEnterpriseFuncInvoked = true
ds.mu.Unlock()
return ds.CreateEnterpriseFunc(ctx)
return ds.CreateEnterpriseFunc(ctx, userID)
}

func (ds *Datastore) GetEnterpriseByID(ctx context.Context, ID uint) (*android.EnterpriseDetails, error) {
Expand Down
2 changes: 1 addition & 1 deletion server/mdm/android/mock/datastore_setup.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import (
)

func (s *Datastore) InitCommonMocks() {
s.CreateEnterpriseFunc = func(ctx context.Context) (uint, error) {
s.CreateEnterpriseFunc = func(ctx context.Context, _ uint) (uint, error) {
return 1, nil
}
s.UpdateEnterpriseFunc = func(ctx context.Context, enterprise *android.EnterpriseDetails) error {
Expand Down
9 changes: 5 additions & 4 deletions server/mdm/android/mysql/enterprises.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,10 @@ import (
"github.com/jmoiron/sqlx"
)

func (ds *Datastore) CreateEnterprise(ctx context.Context) (uint, error) {
stmt := `INSERT INTO android_enterprises (signup_name) VALUES ('')`
res, err := ds.Writer(ctx).ExecContext(ctx, stmt)
func (ds *Datastore) CreateEnterprise(ctx context.Context, userID uint) (uint, error) {
// android_enterprises user_id is only set when the row is created
stmt := `INSERT INTO android_enterprises (signup_name, user_id) VALUES ('', ?)`
res, err := ds.Writer(ctx).ExecContext(ctx, stmt, userID)
if err != nil {
return 0, ctxerr.Wrap(ctx, err, "inserting enterprise")
}
Expand All @@ -22,7 +23,7 @@ func (ds *Datastore) CreateEnterprise(ctx context.Context) (uint, error) {
}

func (ds *Datastore) GetEnterpriseByID(ctx context.Context, id uint) (*android.EnterpriseDetails, error) {
stmt := `SELECT id, signup_name, enterprise_id, pubsub_topic_id, signup_token FROM android_enterprises WHERE id = ?`
stmt := `SELECT id, signup_name, enterprise_id, pubsub_topic_id, signup_token, user_id FROM android_enterprises WHERE id = ?`
var enterprise android.EnterpriseDetails
err := sqlx.GetContext(ctx, ds.reader(ctx), &enterprise, stmt, id)
switch {
Expand Down
14 changes: 10 additions & 4 deletions server/mdm/android/mysql/enterprises_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,15 @@ func testCreateGetEnterprise(t *testing.T, ds *Datastore) {
_, err := ds.GetEnterpriseByID(testCtx(), 9999)
assert.True(t, fleet.IsNotFound(err))

id, err := ds.CreateEnterprise(testCtx())
const userID = uint(10)
id, err := ds.CreateEnterprise(testCtx(), userID)
require.NoError(t, err)
assert.NotZero(t, id)

result, err := ds.GetEnterpriseByID(testCtx(), id)
require.NoError(t, err)
assert.Equal(t, android.Enterprise{ID: id}, result.Enterprise)
assert.Equal(t, userID, result.UserID)
}

func testUpdateEnterprise(t *testing.T, ds *Datastore) {
Expand All @@ -57,14 +59,16 @@ func testUpdateEnterprise(t *testing.T, ds *Datastore) {
err := ds.UpdateEnterprise(testCtx(), enterprise)
assert.Error(t, err)

id, err := ds.CreateEnterprise(testCtx())
const userID = uint(10)
id, err := ds.CreateEnterprise(testCtx(), userID)
require.NoError(t, err)
assert.NotZero(t, id)

enterprise.ID = id
err = ds.UpdateEnterprise(testCtx(), enterprise)
require.NoError(t, err)

enterprise.UserID = userID
resultEnriched, err := ds.GetEnterpriseByID(testCtx(), enterprise.ID)
require.NoError(t, err)
assert.Equal(t, enterprise, resultEnriched)
Expand All @@ -86,7 +90,7 @@ func testDeleteEnterprises(t *testing.T, ds *Datastore) {
assert.Equal(t, enterprise, result)

// Create enteprise without enterprise_id
id, err := ds.CreateEnterprise(testCtx())
id, err := ds.CreateEnterprise(testCtx(), 10)
require.NoError(t, err)
assert.NotZero(t, id)

Expand Down Expand Up @@ -123,11 +127,13 @@ func createEnterprise(t *testing.T, ds *Datastore) *android.EnterpriseDetails {
},
SignupName: "signupUrls/C97372c91c6a85139",
}
id, err := ds.CreateEnterprise(testCtx())
const userID = uint(10)
id, err := ds.CreateEnterprise(testCtx(), userID)
require.NoError(t, err)
assert.NotZero(t, id)

enterprise.ID = id
enterprise.UserID = userID
err = ds.UpdateEnterprise(testCtx(), enterprise)
require.NoError(t, err)
return enterprise
Expand Down
1 change: 1 addition & 0 deletions server/mdm/android/mysql/schema.sql
Original file line number Diff line number Diff line change
Expand Up @@ -24,5 +24,6 @@ CREATE TABLE `android_enterprises` (
`updated_at` datetime(6) DEFAULT CURRENT_TIMESTAMP(6) ON UPDATE CURRENT_TIMESTAMP(6),
`signup_token` varchar(63) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NOT NULL DEFAULT '',
`pubsub_topic_id` varchar(63) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NOT NULL DEFAULT '',
`user_id` int unsigned NOT NULL DEFAULT '0',
PRIMARY KEY (`id`)
) /*!50100 TABLESPACE `innodb_system` */ ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci;
Loading

0 comments on commit 67b7276

Please sign in to comment.