Assign user a given password and check connection status for a given password#430
Assign user a given password and check connection status for a given password#430piif wants to merge 4 commits into
Conversation
TODO : why PUT or PATCH are refused at runtime but POST works ??
TODO : need more tests
loicgreffier
left a comment
loicgreffier
left a comment
There was a problem hiding this comment.
@piif Here is my review. Overall:
resetPasswordandsetPasswordcould be a single endpointcheckPasswordcould rely on AdminClient
| * @return void | ||
| */ | ||
| @Patch("/{user}/set-password") | ||
| public HttpResponse<KafkaUserResetPassword> setPassword(String namespace, String user, @Body String password) { |
There was a problem hiding this comment.
As suggested in michelin/kafkactl#91, I think we can use a single endpoint for that, following this logic:
- if @Body is not empty, calling
userAsyncExecutor#resetPasswordwith the given username/password - if @Body is empty, generate a random password, then calling
userAsyncExecutor#resetPasswordwith the given username/random password.
➡️ I think userAsyncExecutor#resetPassword and userAsyncExecutor#setPassword can be a single method as well:
- Add a
passwordparameter touserAsyncExecutor#resetPassword - Extract the generation of the random password outside of
userAsyncExecutor#resetPasswordin order to generate it if the @Body is empty
| UserAsyncExecutor userAsyncExecutor = | ||
| applicationContext.getBean(UserAsyncExecutor.class, Qualifiers.byName(ns.getMetadata().getCluster())); | ||
|
|
||
| if (userAsyncExecutor.checkPassword(ns.getSpec().getKafkaUser(), password)) { |
There was a problem hiding this comment.
As suggested in michelin/kafkactl#91, a status that specifies if the connection with the given password ends with SUCCESS or FAIL may be more significant.
| } | ||
|
|
||
| @Override | ||
| public void setPassword(String user, String password) { |
There was a problem hiding this comment.
See suggestions from above:
➡️ I think userAsyncExecutor#resetPassword and userAsyncExecutor#setPassword can be a single method as well:
- Add a password parameter to userAsyncExecutor#resetPassword
- Extract the generation of the random password outside of
userAsyncExecutor#resetPasswordin order to generate it if the @Body is empty
| @Override | ||
| public boolean checkPassword(String user, String password, Properties config) { | ||
| Properties props = new Properties(); | ||
| props.put(ProducerConfig.BOOTSTRAP_SERVERS_CONFIG, config.get("bootstrap.servers")); |
There was a problem hiding this comment.
Using an AdminClient to verify the connexion may be more elegant than instantiating a dedicated producer, let me sleep on that
loicgreffier
changed the title
2 participants
Implements APIs entrypoints to set and check a user password :
PATCH /api/namespaces/[namespace]/users/[user]/set-passwordwith new password as body ; returns same result than reset-password (open to discussion : it may just return a status ?)POST /api/namespaces/[namespace]/users/[user]/check-passwordwith new password as body ; return 200 is password matches current one, 401 else