Create broker domain verifie authenticator#425
Conversation
|
|
||
| if (StringUtil.isNotBlank(authenticatedUserEmail) | ||
| && StringUtil.isNotBlank(email) | ||
| && !authenticatedUserEmail.equals(email)) { |
There was a problem hiding this comment.
We should probably do some domain normalization here to make sure we're comparing the same thing (lower case, stripping periods).
There was a problem hiding this comment.
It still looks a string equality check for me. Should this authenticator pass if I have the registered email in keycloak as foobar@phasetwo.io but some legacy system stores it as FOOBAR@PHASETWO.IO?
Should we use jakarta.mail.internet.InternetAddress's equals method here? Probably write a utility method into the io.phasetwo.service.util.Emails utility class, like: public static boolean isEmailAddressesEqual(String one, String two)?
There was a problem hiding this comment.
I'm checking the form of the email in the conditions above
| log.warnf( | ||
| "Authenticated user email %s does not have a email {}. Validation not performed", | ||
| authResult.user().getEmail(), authResult.user().getEmail()); |
There was a problem hiding this comment.
This log statement looks wrong. authenticatedUserEmail is blank. {} in the log string is the wrong string replacement type. Should be %s.
vilmosnagy
left a comment
vilmosnagy
left a comment
There was a problem hiding this comment.
Can we write some tests for this authenticator? Either unit tests but I'd prefer the integration tests with cypress.
rtufisi
force-pushed
the
rtuf/create-broker-domain-verifier-authenticator
branch
from
3d97196 to
90c5b03
Compare
Code Coverage
|
3 participants
No description provided.