Make Authentication/Authorization Stacks Shallower/Simpler #75662
Conversation
Same as elastic#75252 pretty much just continuing to make this logic a little simpler for easier profiling and (very) maybe performance through saving some allocations/indirection.
original-brownbear
added
>refactoring
:Security/Authorization
|
Pinging @elastic/es-security (Team:Security) |
There was a problem hiding this comment.
LGTM with just one question:
This PR simplifies call stacks but introduces some more try/catch blocks, e.g. authorizeIndexActionName now throws the error object instead of just instantiating it. Unwinding the callstack when catch fires could be expensive. Is it a concern? Or it is ok because the happy path (successful authentication and authorization) is not affected?
| // we assign the listener call to an action to avoid calling the listener within a try block and auditing the wrong thing when | ||
| // an exception bubbles up even after successful authentication |
There was a problem hiding this comment.
Don't quite get this existing comment. But I suspect the original concern may not exist any more.
| // we assign the listener call to an action to avoid calling the listener within a try block and auditing the wrong thing | ||
| // when an exception bubbles up even after successful authentication | ||
| action.run(); |
There was a problem hiding this comment.
Ah ok the same comment makes more sense here (because it has a try/catch block). As I suspected, it is no longer applicable even before this PR.
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/RBACEngine.java
Outdated
Show resolved
Hide resolved
|
Thanks Yang!
I think it's probably not relevant. For one, as you point out this is just the unhappy path. But also the catch is always really close to the |
…5662) Same as elastic#75252 pretty much just continuing to make this logic a little simpler for easier profiling and (very) maybe performance through saving some allocations/indirection.
…5662) Same as elastic#75252 pretty much just continuing to make this logic a little simpler for easier profiling and (very) maybe performance through saving some allocations/indirection.
Same as #75252 pretty much just continuing to make this logic a little
simpler for easier profiling and (very) maybe performance through
saving some allocations/indirection.