[server] Only super.users can modify configuration of super.user info. - #4229
[server] Only super.users can modify configuration of super.user info.#4229loserwang1024 wants to merge 1 commit into
Conversation
wuchong
left a comment
wuchong
left a comment
There was a problem hiding this comment.
@loserwang1024 I left some comments.
| conf.setString( | ||
| ConfigOptions.SERVER_SASL_CREDENTIALS.key(), "root:password,guest:passwords"); | ||
| conf.set(ConfigOptions.SUPER_USERS, "User:root"); | ||
| conf.set(ConfigOptions.SUPER_USERS, "User:root;User:guest"); |
There was a problem hiding this comment.
Could we use super instead of guest to represent the super user? The current test code is confusing, particularly when encountering the restriction on modifying the guest password. Additionally, please add comments near the test code to clarify which users are designated as super users and to explain that the CLUSTER ALL permission does not allow modifications to super user accounts.
|
|
||
| /** Returns the merged credentials that belong to a configured super user. */ | ||
| private Map<String, String> superUserCredentials(@Nullable Map<String, String> credentials) { | ||
| Map<String, String> superUserCredentials = new LinkedHashMap<>(); |
There was a problem hiding this comment.
Why do we need LinkedHashMap here? I think we don't need the original order to compare the super user credentials.
| mergePlainCredentials(credentials) | ||
| .forEach( | ||
| (user, password) -> { | ||
| if (isSuperUser(new FlussPrincipal(user, USER_PRINCIPAL_TYPE))) { |
There was a problem hiding this comment.
Why is the User type hard-coded here? According to the description of super.users, the principal_type can be configured to any type, not just User. Shouldn't we simply compare usernames instead? After all, we assume that usernames within the security.sasl.plain.credentials are unique.
| throw new AuthorizationException( | ||
| "Only configured super users may alter credentials of configured super users."); |
There was a problem hiding this comment.
| throw new AuthorizationException( | |
| "Only configured super users may alter credentials of configured super users."); | |
| throw new AuthorizationException( | |
| String.format( | |
| "Principal %s cannot modify credentials belonging to users in 'super.users', the requester must itself be a super user.", | |
| requester)); |
Improve the exception to make it more clear.
2 participants
Purpose
Linked issue: close #xxx
Brief change log
Tests
API and Format
Documentation