Security: remove put privilege API #32879
Conversation
This commit removes the put privilege API in favor of having a single API to create and update privileges. If we see the need to have an API like this in the future we can always add it back.
jaymode
added
:Security/Authorization
|
Pinging @elastic/es-security |
| "metadata": { | ||
| "key1" : "val1a", | ||
| "key2" : "val2a" | ||
| "app": { |
There was a problem hiding this comment.
Whats the relation between app and p1 and them being later declared again using application: "app" and name: "p1" do these need to be equal?
If we are taking a Map<AppName, Map<PrivilegeName, Privilege>> I think we should omit application and name or we have to take Array<Privilege>
There was a problem hiding this comment.
They are equivalent as far as I know and I agree that it is redundant. @tvernum can you chime in about this aspect?
There was a problem hiding this comment.
They're redundant and optional, but if they exist they need to match the keys from the containing object.
They're allowed for three reasons
- Historical, originally I had the body just being an array, but there are a few problems with array bodies, including that rest tests can't handle it, so I switched to objects, and at Albert's suggestion made the inner fields optional.
- It means that the objects that come from the GET API can be fed back into the PUT/POST without needing to remove fields
- The format for index storage needs the fields and we re-use the parser, so we needed to do something and optional seemed better than rejection.
There was a problem hiding this comment.
Optional makes sense. I updated the rest test to use it both ways
|
This looks good but I'm worried that the semantics aren't quite correct for a PUT (See review comment with link to previous discussion) |
|
@Mpdreamz we discussed this in our team meeting. We came to the conclusion that this will be a post only API since it does not follow the PUT semantics. Given that, we feel the name of the spec is misleading but didn't have a great name for it. This API will create or update a given privilege. Do you have any naming suggestions? IIUC this name might be important for the generation of the client from the spec files so we want to make sure its right for you guys. |
|
@tvernum can you take a look and verify everything looks good other than any potential naming change that we make |
|
@tvernum mind linking to the discussion (do not spot it). I'm actually OK with the
PUT allows for this but typically means it is replacing the resource completely if it exists already. The three linked existing API's do not follow that rule strictly. It depends on what you argue is the resource e.g you do not provide ALL the settings again in the request but does you provide you completely replace. I do think the
Ideally the |
It does return the Map but does repeat the two keys inside the definition |
This reverts commit c3f547b.
|
I chatted with @kobelb and they are currently using the full response. Given the timing, my plan is to leave this as is and for 7.0 we can remove the duplication in the response. |
|
test this please |
This commit removes the put privilege API in favor of having a single API to create and update privileges. If we see the need to have an API like this in the future we can always add it back.
This commit removes the put privilege API in favor of having a single API to create and update privileges. If we see the need to have an API like this in the future we can always add it back.
This commit removes the put privilege API in favor of having a single API to create and update privileges. If we see the need to have an API like this in the future we can always add it back.
* 6.x: (42 commits) [DOCS] Splits the users API documentation into multiple pages (#32825) [DOCS] Splits the token APIs into separate pages (#32865) [DOCS] Creates redirects for role management APIs page Bypassing failing test PainlessDomainSplitIT#testHRDSplit (#32966) TEST: Mute testRetentionPolicyChangeDuringRecovery [DOCS] Fixes more broken links to role management APIs [Docs] Tweaks and fixes to rollup docs [DOCS] Fixes links to role management APIs [ML][TEST] Fix BasicRenormalizationIT after adding multibucket feature [DOCS] Splits the roles API documentation into multiple pages (#32794) [TEST] Run pre 6.4 nodes in non-FIPS JVMs (#32901) Remove assertion in testDocStats on deletedDocs counter (#32914) [ML] fix updating opened jobs scheduled events (#31651) (#32881) Enable FIPS140LicenseBootstrapCheck (#32903) HLRC: Move ML request converters into their own class (#32906) [DOCS] Update getting-started.asciidoc (#29518) Fix allowed value for HighlighterBuilder encoder in javadocs (#32780) [DOCS] Add "remove a tag" script logic as an example (#32556) RFC: Test that example plugins build stand-alone (#32235) Security: remove put privilege API (#32879) ...
This commit removes the put privilege API in favor of having a single API to
create and update privileges. If we see the need to have an API like this in
the future we can always add it back.