Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removes FindQueryHTTP['fields'] option in Saved Objects Management #152306

Conversation

TinaHeiligers
Copy link
Contributor

fix #151427

The saved object's management (SOM) plugin allowed specifying saved object attributes by optionally accepting a fields option in _find calls.

This means that we could potentially leak the internal structure of saved objects in the browser.

Note: The SOM api's were not intended to be public, and this change is therefore not considered a breaking API change.

Checklist

Risk Matrix

Risk Probability Severity Mitigation/Notes
Code explicitly using the SOM 'find' API with a fields query returns a bad request; external plugins depending on the SOM feature no longer return expected results. Low Low The APIs aren't explicitly documented and any inference that they are public is unintentional. We will need to handle issues on an individual basis for 3rd party plugins.

For maintainers

  • This was checked for breaking API changes and was labeled appropriately The SOM api's were not intended to be public, and this change is therefore not considered a breaking API change.

@TinaHeiligers TinaHeiligers added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting Feature:Saved Objects Management v8.8.0 labels Feb 28, 2023
@TinaHeiligers TinaHeiligers marked this pull request as ready for review February 28, 2023 02:09
@TinaHeiligers TinaHeiligers requested a review from a team as a code owner February 28, 2023 02:09
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@TinaHeiligers
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor Author

@TinaHeiligers TinaHeiligers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self review

@@ -236,7 +236,6 @@ export class SavedObjectsTable extends Component<SavedObjectsTableProps, SavedOb
search: queryText ? `${queryText}*` : undefined,
perPage,
page: page + 1,
fields: ['id'],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than trying hard coding the option, I've removed it instead.

@@ -90,9 +89,6 @@ export const registerFindRoute = (
saved_objects: savedObjects.map((so) => {
const obj = injectMetaAttributes(so, managementService);
const result = { ...obj, attributes: {} as Record<string, unknown> };
for (const field of includedFields) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removes handling the 'fields' option in the query since we don't want to explicitly expose attributes in the response

@@ -37,7 +37,7 @@ export default function ({ getService }: FtrProviderContext) {

it('should return 200 with individual responses', async () =>
await supertest
.get('/api/kibana/management/saved_objects/_find?type=visualization&fields=title')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adapts tests

}),
// fields: schema.oneOf([schema.string(), schema.arrayOf(schema.string())], {
// defaultValue: [],
// }),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think you want to remove the fields schema rather than comment it out

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks!

Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @TinaHeiligers ! Pending @Bamieh 's comment, this LGTM. I tested locally and worked as expected.

@TinaHeiligers
Copy link
Contributor Author

@elasticmachine merge upstream

@TinaHeiligers TinaHeiligers enabled auto-merge (squash) February 28, 2023 16:35
@TinaHeiligers TinaHeiligers merged commit 5274175 into elastic:main Feb 28, 2023
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
savedObjectsManagement 87.6KB 87.6KB -14.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
securitySolution 428 430 +2

Total ESLint disabled count

id before after diff
securitySolution 506 508 +2

History

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

@TinaHeiligers TinaHeiligers deleted the kbn-151427-findqueryhttp-type-remove-fields branch February 28, 2023 18:21
bmorelli25 pushed a commit to bmorelli25/kibana that referenced this pull request Mar 10, 2023
…lastic#152306)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Saved Objects Management release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Saved Objects Management] Consider removing FindQueryHTTP['fields']
6 participants