Add a qrcode MFA, webservice, service and encryption for the secret key #19
Conversation
|
Hello,
enfin bref, dans l'ensemble ouvrir un porte dans plume-admin, porte exploitable par un module qui implémente de genre de choses, et à qui on peut donner d'autres authentication factor |
C'est sûrement une bonne idée, on va avoir pas mal de dépendance sinon qui vont s'ajouter puisqu'il va y en avoir pour chacun des MFA
Yes ca complètement => Done dans le dernier commit
Oui, on pourrait faire un MfaService qui a une map d'implem / type, et pouvoir en ajouter dynamiquement ?
|
There was a problem hiding this comment.
Il faudrait faire une lib à part, ça semble trop gros et trop spécifique :
- Ca va trop compliquer la maintenance de plume admin si on ajoute ça
- C'est difficile de l'utiliser hors back-office en l'état, ce qui est dommage
- Par contre, dans plume admin, ça pourrait être bien d'avoir un test d'intégration qui permette de vérifier qu'on ne casse pas plume mfa
Il faudrait documenter le processus global.
Il faudrait annoter les méthodes pour préciser ce qui est nullable et ce qui ne l'est pas.
Pour quelque chose d'aussi sensible, il faudrait ajouter des tests unitaires.
Globalement la qualité de code ne me rassure pas trop, ça sent un peu trop le GPT pas assez nettoyé.
| public String encrypt(String data) throws Exception { | ||
| Cipher cipher = Cipher.getInstance(ALGORITHM); | ||
| byte[] iv = new byte[IV_SIZE]; | ||
| SecureRandom random = new SecureRandom(); |
There was a problem hiding this comment.
SecureRandom est thread safe => autant l'initialiser qu'une seule fois
| @@ -16,13 +19,14 @@ CREATE TABLE `PLM_USER` ( | |||
| `email` varchar(255) NOT NULL, | |||
| `user_name` varchar(255) NOT NULL, | |||
| `password` varchar(255) NOT NULL, | |||
| `mfa_user_handle` BLOB DEFAULT NULL, | |||
There was a problem hiding this comment.
Pourquoi ce n'est pas dans la table PLM_USER_MFA ?
| PublicKeyCredential<AuthenticatorAttestationResponse, ClientRegistrationExtensionOutputs> pkc | ||
| ) { | ||
| AdminMfaBrowser mfa = new AdminMfaBrowser(); | ||
| mfa.setKeyId(result.getKeyId().getId().getBytes()); |
There was a problem hiding this comment.
C'est normal de faire .getBytes() ici ?
| AdminUserMfa userMfa = new AdminUserMfa(); | ||
| userMfa.setIdUser(user.getId()); | ||
| userMfa.setIdMfaBrowser(mfa.getId()); | ||
| userMfa.setType("Browser"); |
There was a problem hiding this comment.
Il faudrait passer par une enum non ?
| adminUserMfaDao.save(userMfa); | ||
| } | ||
|
|
||
| public void updateCredential( |
There was a problem hiding this comment.
Il faudrait document pour expliquer ce qu'on veut faire ici, c'est pas clair du tout pour moi
| // Return the QR code URL to the client | ||
| return new AdminMfaQrcode(qrCodeUrl); | ||
| } catch (Exception e) { | ||
| logger.debug("erreur lors de la génération du QR code", e); |
There was a problem hiding this comment.
On met les commentaires plutôt en anglais
| try { | ||
| PublicKeyCredential<AuthenticatorAttestationResponse, ClientRegistrationExtensionOutputs> pkc = | ||
| PublicKeyCredential.parseRegistrationResponseJson(publicKeyCredentialJson); | ||
| boolean success = mfaService.finishRegistration(authenticatedUser.getUser(), pkc); |
There was a problem hiding this comment.
Si c'est pour utiliser le truc comme ça, autant laisser la méthode lever une exception en cas de souci, le résultat sera correctement loggé et l'erreur interne sera reoturnée, ça évite toute cette logique
| return Response.ok().build(); | ||
| } catch (IOException e) { | ||
| logger.error("publicKeyCredentialJson parsing error", e); | ||
| return Response.serverError().build(); |
There was a problem hiding this comment.
Même chose ici, cette logique est inutile, surtout qu'elle ne suit pas les conventions habituelles pour retourner une erreur INTERNAL_ERROR
| } catch (JsonProcessingException e) { | ||
| logger.debug("erreur lors de la génération du credentialGetOptions", e); | ||
| throw new WsException(WsError.INTERNAL_ERROR); | ||
| } |
| .build(); | ||
|
|
||
| } catch (IOException e) { | ||
| throw new WsException(AdminWsError.WRONG_LOGIN_OR_PASSWORD); |
There was a problem hiding this comment.
Uh ?? S'il y a une erreur IO, ça veut dire que le login ou le password est mauvais ?
Yes c'était la discussion qu'on avait plus haut.
Je ferais la doc quand on aura réfléchi à l'archi :p
Non c'est juste un draft pour vérifier que techniquement ça marche. C'est loin d'être finit, d'où les exceptions qui sont pas vraiment gérer. Y'a plein de refacto à faire que je voulais pas toucher tant qu'on savait pas vraiment comment on voulait pas l'intégrer. Surtout depuis qu'on m'a dit qu'il n'y avait qu'un 2FA (QR Code) de vendu enfait. |
|
Oui c'est clair, on est en phase. Désolé pour le commentaire un peu sec ! |
No description provided.