fix(rules): remove id key from uploaded rule

[mwangggg]

Welcome to Cryostat! 👋

Before contributing, make sure you have:

• Read the contributing guidelines
• Linked a relevant issue which this PR resolves
• Linked any other relevant issues, PR's, or documentation, if any
• Resolved all conflicts, if any
• Rebased your branch PR on top of the latest upstream main branch
• Attached at least one of the following labels to the PR: [chore, ci, docs, feat, fix, test]
• Signed all commits using a GPG signature

To recreate commits with GPG signature git fetch upstream && git rebase --force --gpg-sign upstream/main

---

Fixes: cryostatio/cryostat#261

[andrewazores]

I think this should really be done on the server side also, and this is actually a broader change that affects more than just rule definitions.

https://github.com/cryostatio/cryostat3/blob/bd6f61d05436f26fcbf8d3edd27e767be21dc8c9/src/main/java/io/cryostat/discovery/CustomDiscovery.java#L101

Here we have an example of a POST endpoint that expects a form submission (URL-encoded or multipart) for creating a custom target. There is no @RestForm for the entity's id, so there is no way for a client to hit this endpoint and supply an ID that the server will try to (re)use. This is good - the ID should only ever be assigned by the server, or really by the database. In any case the client should not be allowed to specify an ID for a created entity.

https://github.com/cryostatio/cryostat3/blob/bd6f61d05436f26fcbf8d3edd27e767be21dc8c9/src/main/java/io/cryostat/discovery/CustomDiscovery.java#L90

Here we have a problem. This is an equivalent endpoint that expects a JSON payload body. This means the client is free to submit a JSON that includes { "id": 1234 } and the API definition accepts it. This will be deserialized into a Target instance that really has that field with that value set. The server will then try to persist that into the database. This should not be allowed. Either the server should silently scrub and ignore the id field and allow it to be assigned by the database, or the server should refuse the request outright and respond with a 400/409 status before it even gets to the database. IMO the 400/409 response is the better, cleaner way.

---

For user convenience I think it's OK and a good idea to drop id fields on the client UI side like this, since the user probably does not know or care that downloaded rules contain an id. The user just wants to do the workflow we were doing when we discovered this bug - download a rule, then upload it to reuse later. If that fails because of an id field that I don't care about it's annoying, and I have to go edit the JSON file and remove it myself. This way the primary user workflow is nice and smooth and the UI helps the user avoid doing silly things that result in a 400 response.

However, we shouldn't rely on our UI being the only client, so the server does need to be more proactive about checking requests.

[andrewazores]

Looks good. Please double check if there are any other UI workflows that perform a JSON POST like this and might also need to have id fields stripped off.

[mwangggg]

Looks good. Please double check if there are any other UI workflows that perform a JSON POST like this and might also need to have id fields stripped off.

This should be the only user upload where stripping the id applies.