Cherry-pick: Adding name to software checksum for mac software #34235
Conversation
ksykulev
changed the title
**Related issue:** Resolves #28788 If some of the following don't apply, delete the relevant line. - [x] 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. - [x] Input data is properly validated, `SELECT *` is avoided, SQL injection is prevented (using placeholders for values in statements) - [x] Added/updated automated tests - [x] Where appropriate, [automated tests simulate multiple hosts and test for host isolation](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/reference/patterns-backend.md#unit-testing) (updates to one hosts's records do not affect another) - [x] QA'd all new/changed functionality manually - [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`). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> * **Bug Fixes** * macOS app checksums now include the app name, improving grouping, deduplication, and preventing mis-linking or duplicate entries when multiple names share a bundle ID. * More stable title handling when bundle IDs are missing, reducing unintended renames and mismatches. * **Tests** * Re-enabled related host-software tests and added a longest-common-prefix test to validate name reconciliation. * **Chores** * Database migration added to recalculate checksums for affected macOS app records. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## rc-patch-fleet-v4.74.1 #34235 +/- ##
=========================================================
Coverage ? 63.04%
=========================================================
Files ? 1335
Lines ? 181119
Branches ? 0
=========================================================
Hits ? 114180
Misses ? 56950
Partials ? 9989
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:
|
| key := titleKey{ | ||
| bundleID: sw.BundleIdentifier, | ||
| source: sw.Source, | ||
| browser: sw.Browser, |
There was a problem hiding this comment.
this is extensionFor in the original patch. Intentional?
There was a problem hiding this comment.
browser was changed to extensionFor in this PR:
41c53860e3
So I need to reverse all those changes for 4.74.0.
Here is how the code looks like in 4.74.0
https://github.com/fleetdm/fleet/blob/rc-minor-fleet-v4.74.0/server/datastore/mysql/software.go#L875-L889
Thanks for calling this out! 👍
|
|
||
| if _, exists := uniqueTitlesToInsert[key]; !exists { | ||
| uniqueTitlesToInsert[key] = title | ||
| } |
There was a problem hiding this comment.
Dropped a change of const numberOfArgsPerSoftwareTitles = 6 below from the original patch
There was a problem hiding this comment.
I don't know what added an additional argument, but I looked in the 4.74.0 code.
https://github.com/fleetdm/fleet/blob/rc-minor-fleet-v4.74.0/server/datastore/mysql/software.go#L926
And it's only 5 args.
titlesValues := strings.TrimSuffix(strings.Repeat("(?,?,?,?,?),", len(uniqueTitlesToInsert)), ",")
👍
| } | ||
| // Log cases where the new software has no last opened timestamp, the current software does, | ||
| // and the software is marked as having a name change. | ||
| if ok && curSw.LastOpenedAt != nil { |
There was a problem hiding this comment.
Dropped && newSw.Source != "apps" here
There was a problem hiding this comment.
Wait i'm a little confused.
4.74.0 https://github.com/fleetdm/fleet/blob/rc-minor-fleet-v4.74.0/server/datastore/mysql/software.go#L1232C4-L1232C40
if ok && curSw.LastOpenedAt != nil {
The change was to get rid of ok and add newSw.Source != "apps".
So the new code looks like
if curSw.LastOpenedAt != nil && newSw.Source != "apps" {
There was a problem hiding this comment.
oops sorry was looking in the wrong direction -- the change is as you said, && newSw.Source != "apps" is added in this cherry pick but missing in the original patch, which sounds like it was intentional
|
Keeping this in our back pocket in case 4.75.0 does not go well. |
2 participants
Back ported: #34097 to be compatible with the 4.70 release.
Original issue #28788