-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[SOR] Intersect allowed and authorized types #244967
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
base: main
Are you sure you want to change the base?
[SOR] Intersect allowed and authorized types #244967
Conversation
60c94ce to
3a28421
Compare
⏳ Build in-progress, with failures
Failed CI StepsTest Failures
History
cc @afharo |
| } | ||
| if (authorizationResult?.status === 'partially_authorized') { | ||
| if ( | ||
| authorizationResult?.status === 'fully_authorized' || |
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.
This only ensures the same path is followed for both authorization levels, although, functionally, it achieves the same result if we don't add this clause in the if.
Happy to remove this line if we think that performance might be a problem.
I'd appreciate @azasypkin's thoughts on this.
| // Discard the types that the SO repository doesn't know about (typically hidden objects). | ||
| if (!allowedTypes.includes(objType)) continue; |
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.
Mixing the user's authorization and the client's scope here (which can be odd).
However, typeToNamespacesMap doesn't really imply that it's bound to only one.
| { | ||
| type: [ | ||
| type, | ||
| 'foo', |
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 had to add foo to the requested type so that it is kept in the auth map.
| expect(arrayMapsAreEqual(actualMap, expectedMap)).toBeTruthy(); | ||
| }); | ||
|
|
||
| test(`uses the authorization map when fully authorized`, async () => { |
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.
just repeating the test above, but for fully authorized.
| // Make sure to search through all the hidden types as well. | ||
| includedHiddenTypes: types.filter((t) => t.hidden).map((t) => t.name), |
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 assume that we want to search references to the Data View across all types. Is my assumption correct?
|
Pinging @elastic/kibana-core (Team:Core) |
|
Pinging @elastic/kibana-security (Team:Security) |
|
Pinging @elastic/kibana-data-discovery (Team:DataDiscovery) |
Summary
We identified some scenarios where clients authorized to a partial list of SO types would circumvent the Saved Objects Repository's allowed types (it could list "hidden" SO types).
This PR addresses this issue by unifying the flow for partially and fully authorized clients, and applying the intersection of the allowed and authorized lists.
Checklist
release_note:breakinglabel should be applied in these situations.release_note:*label is applied per the guidelinesbackport:*labels.Identify risks