-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Removes FindQueryHTTP['fields'] option in Saved Objects Management #152306
Conversation
Pinging @elastic/kibana-core (Team:Core) |
@elasticmachine merge upstream |
There was a problem hiding this 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'], |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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') |
There was a problem hiding this comment.
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: [], | ||
// }), |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, thanks!
There was a problem hiding this 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.
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Async chunks
Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
…lastic#152306) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
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
fields
query returns a bad request; external plugins depending on the SOM feature no longer return expected results.For maintainers