Deleting no longer used privileges #24873
Conversation
kobelb
added
non-issue
💚 Build Succeeded |
x-pack/plugins/security/server/lib/authorization/register_privileges_with_cluster.js
Show resolved
Hide resolved
💚 Build Succeeded |
|
@legrego I added in a separate log statement and updated the tests, this is good for another review whenever you get the chance. |
| return; | ||
| } | ||
| default: { | ||
| expect(true).toBe(false); |
There was a problem hiding this comment.
nit: test failures here might be hard to debug. Consider throwing an explicit exception about this unhandled scenario.
There was a problem hiding this comment.
If we just threw an explicit exception here, it'd potentially be caught by the code using the callWithRequest and lead to rather difficult to understand behavior. I wanted to fail the test explicitly, and this was the most clear solution that I could come up with, even though it isn't really clear at all. I thought about also doing the equivalent of expect(api).toBeInArray(['shield.getPrivilege', 'shield.postPrivileges', 'shield.deletePrivilege'] but figured that'd be equally confusing, and potentially more-so, so I gave up and did this.
* We can now delete old privileges * Logging message when error deleting specific privilege
* We can now delete old privileges * Logging message when error deleting specific privilege
We've gotten away without needing the ability to delete no longer specified privileges because we've been working with a fixed set of privileges until now. However, with the addition of #20277 we're going to need this functionality if we allow application themselves to drive privileges being defined, and the user later uninstalls that plugin.
Elasticsearch's Privilege API requires us to execute an individual
DELETEfor any privilege that we'd like deleted, as thePOSTAPI only allow bulk "upserts".