Register event listeners for default system access control#11632
Register event listeners for default system access control#11632kokosing merged 2 commits intotrinodb:masterfrom gaurav8297:guarav8297/default_system_access_control
Conversation
ksobolew
left a comment
ksobolew
left a comment
There was a problem hiding this comment.
Right now if you extend and set default access control to something else using an optional binder, then there will be a problem because event listeners won't get registered.
I'm not sure how the optional binders are related?
On that note, an explanation of the issue in the commit message would be appreciated :)
| @VisibleForTesting | ||
| protected void setSystemAccessControl(String name, Map<String, String> properties) | ||
| public void loadSystemAccessControl(String name, Map<String, String> properties) |
There was a problem hiding this comment.
If it's public, does the @VisibleForTesting still make sense?
There was a problem hiding this comment.
I think so. It's being used only in tests other than this class itself
| systemAccessControl.getEventListeners() | ||
| .forEach(eventListenerManager::addEventListener); |
There was a problem hiding this comment.
I would move it to the no-argument loadSystemAccessControl(), mostly because this is also where the even listeners are registered for the non-default access controls. This ways it's clearer why this has to be done.
But then you'll have to change TestingAccessControlManager to call loadSystemAccessControl() instead (and orchestrate all the configuration set-up I presume). Do you think this is feasible?
There was a problem hiding this comment.
But then we would need to move out factory.create(..) from setSystemAccessControl(String name, Map<String, String> properties) to loadSystemAccessControl() because without initializing systemAccessControl, we can't register event listeners.
Also in a way setSystemAccessControl(String name...) is loading system access control from the name so I think it's fine
There was a problem hiding this comment.
OK then. This was my thought from looking around the code, as I wanted to understand the context of this change.
Yes, It's not related. I'm just pointing out that is one of the ways someone could extend. |
|
@mosabua No release notes are needed here. |
|
Merged, thanks! |
3 participants
Description
It's a fix
Its part of the core engine security
Right now if you extend and set default access control to something else using an optional binder, then there will be a problem because event listeners won't get registered.
Related issues, pull requests, and links
Documentation
( ) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.
Release notes
( ) No release notes entries required.
( ) Release notes entries required with the following suggested text: