[Security Solution] Fix Policy-License-Watcher payload

[pzl]

Summary

License-watcher, Policy-changer service didn't adhere to the update package policy payload, which lacks an id. This strips that out. Critical bugfix for 7.11

[elasticmachine]

Pinging @elastic/endpoint-app-team (Feature:Endpoint)

[paul-tavares]

I don't think this is going to do it. The type for UpdatePackagePolicy (Fleet) does not have several of the properties defined in the PackagePolicy which we get from the list API (I'm surprised its taking in the revision - wondering if that is doing anything at all when the update occurs)

My suggestion would be to do something similar to what we do in

kibana/x-pack/plugins/security_solution/public/management/pages/policy/store/policy_details/selectors.ts

Line 33 in 80ca5a5

const { id, revision, created_by, created_at, updated_by, updated_at, ...newPolicy } = policy;

Perhaps the above method I reference should be elevated to security_solution/common/ and then re-used in both frontend and server?

_(ps. I realize now that even the method I referenced can still "leak" properties)

[pzl]

so, yes, you're right in that there are extra properties not being stripped in order to match UpdatePackagePolicy perfectly. I don't know why typescript isn't complaining more about this.

that being said, only id is getting rejected during save. The rest are ignored.

as for destructuring further, we'd be starting a sisyphus task. Any time a field might be added to PackagePolicy, and not explicitly added to UpdatePackagePolicy, it would need to be destructured here too.

We can aid ourselves a little by combining the two places it may happen as you mention.

But I would say no, no need here. My reasoning being:

1. This does fix the bug behavior. Everything succeeds here
2. So doing more destructuring and cleanup would be a refactor / improvement. But not a very clean or future-proof one. This can be cleaned up, but that doesn't seem any more robust. We should actually do that robustly (perhaps explicitly creating a new UpdatePolicyPackage object, and assigning the properties explicitly. So the default is to drop extra props))

[paul-tavares]

😄
Ok. Maybe you can get a 👍 from Fleet then? Maybe @jfsiii or @neptunian or @nchaulet ?

(a bit more - some of which I mentioned in our slack conversation)

The problem I see (and I will likely suggest changes to Fleet services if possible) is that for fleet, this is not an issue because they use the service from the API and have Schema’s in place to validate the input - so no extra properties are going to make it in. We however are using it via their service which means we have to ensure the data is valid. We're fortunate in this case in that the update service overrides the extra properties we're passing in.

Typescript types by default are not "exact" but rather if an object with x amount of properties has enough of them to full-fill the targe Type, then typescript is happy, which is the case here. Really (IMO) the types for the input arguments to the service need to be defined as Exact<> (note: this is not supported in Typescript today, but a proposal). There are ways to do this in Typescript today until the utility type lands (see here)

[kevinlog]

I think @pzl has a good point in that we'd have to maintain destructuring - however the one part that scares me is that it doesn't look like created_at or created_by are overridden, so we'd presumably be overwriting that.

the code I'm looking at:

kibana/x-pack/plugins/fleet/server/services/package_policy.ts

Lines 298 to 304 in 4e4e550

	{
	...restOfPackagePolicy,
	inputs,
	revision: oldPackagePolicy.revision + 1,
	updated_at: new Date().toISOString(),
	updated_by: options?.user?.username ?? 'system',
	},

Agreed with Dan's point that the types should be stricter and we should just create a new UpdatePackagePolicy object that enforces that we're only passing the expected fields

[kevinlog]

I verified this locally and the update will now occur:

The bug looks like it's fixed!

[kevinlog]

@pzl
I'm OK with this as long as you're confident that there isn't any surface area to update fields that we don't want to (created_*). Destructuring seems like a quick way to go about this, although you have valid points that it doesn't scale well.

Could we make the change @paul-tavares suggests and then open another ticket to make the improvements on type checking, etc?

[nchaulet]

we should probably add the Exact<UpdatePackagePolicy> on the fleet side in a follow up PR.

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 1af2466

Metrics [docs]

Distributable file count

id	before	after	diff
default	47283	48043	+760

History

• 💚 Build #94992 succeeded 4e4cd47

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream