Repository navigation
Apply Trust Email when the IdP e-mail differs only in case - #54837
Conversation
The first-broker-login review profile stores the e-mail address of the brokered user in lower case, which was wrongly recorded as an e-mail change made by the user, so with Verify email enabled the address was created as unverified and the user had to verify it again even though the identity provider is trusted Closes keycloak#54775 Signed-off-by: KayanoLiam <Kayano04@outlook.jp>
1515c23 to
2dbac48
Compare
There was a problem hiding this comment.
@akasakariko, should isCaseOnlyChange lower-case the IdP address and compare it with equals? Java's equalsIgnoreCase also treats some different letters as equal, for example the dotless ı and i, so a user can change the address in the Review Profile form to a different one and still get Trust Email.
The IdP sends probe@gıthub.test. If the user changes it to other@example.test, Keycloak creates the user with emailVerified=false. If the user changes it to probe@github.test, the user is created with probe@github.test and emailVerified=true. With this change reverted, the second address is emailVerified=false as well.
isCaseOnlyChange returns true when oldValue.get(0).equalsIgnoreCase(newValue), and the email branch then returns without setting UPDATE_PROFILE_EMAIL_CHANGED. The user profile lower-cases with KeycloakModelUtils::toLowerCaseSafe, and "gıthub".toLowerCase() stays gıthub, so the two checks disagree. EmailValidationUtil accepts \u0080 to \uFFFF in both parts of the address, so the IdP address passes validation.
How I tested:
- Copied
KcOidcBrokerTrustEmailUppercaseEmailTestwith the hardcodedemailclaim set toprobe@gıthub.test, Trust Email and Verify email on. - Submitted the Review Profile form with three addresses, and printed the created user's email and
emailVerified. - Ran it on this branch, and again with the
IdpReviewProfileAuthenticatorchange reverted.
I ran this with the embedded server only.
| Address submitted | Expected | Actual (this PR) | Actual (change reverted) |
|---|---|---|---|
probe@gıthub.test, unchanged (control) |
verified | verified | verified |
other@example.test (control) |
not verified | not verified | not verified |
probe@github.test |
not verified | verified | not verified |
equalsIgnoreCase treats distinct letters such as the dotless 'ı' and 'i' as equal, so a user could change the brokered e-mail to a different address in the review profile form and still get Trust Email Lower-case the IdP address with the same KeycloakModelUtils.toLowerCaseSafe the user profile applies and compare it with equals, so only the normalization of the address is ignored Signed-off-by: KayanoLiam <Kayano04@outlook.jp>
|
@oleksii-udovychenko thanks, fixed in ead580f
Verified with the embedded server: the second case fails on the previous behaviour and passes with the change |
The unit tests could not resolve surefire-junit4 3.5.6 from Maven Central and the download failed with 403 Forbidden Signed-off-by: KayanoLiam <Kayano04@outlook.jp>
There was a problem hiding this comment.
@akasakariko Thanks for the PR and @oleksii-udovychenko for the suggestion.
The PR looks good to me, I have just one optional improvement, please see below.
The test only checked that the verification page was shown and an e-mail was sent, so it could pass if the submitted address were discarded and the verification went to the address of the identity provider instead Also assert that the submitted address is stored unverified and that the verification e-mail is addressed to it Signed-off-by: KayanoLiam <Kayano04@outlook.jp>
|
Updated in 1c201cd
Verified that the new assertions fail against the previous behaviour (expecting the provider address fails with
|
There was a problem hiding this comment.
@akasakariko Thank you for addressing the comment. LGTM
|
Thanks, @akasakariko. |
Problem
When an identity provider sends an e-mail address with uppercase letters and the first broker login shows the Review Profile form, the user profile lower-cases the address and
IdpReviewProfileAuthenticatorrecords that as an e-mail change made by the user, which setsUPDATE_PROFILE_EMAIL_CHANGEDAbstractIdentityProviderthen skips Trust Email and creates the user withemailVerified=false, so a user of a trusted directory has to verify the address again on the first sign-inFix
A change of the e-mail attribute that is removed by the lower-casing of the user profile is no longer recorded as a change made by the user
The comparison uses the same
KeycloakModelUtils.toLowerCaseSafenormalisation as the user profile, so a different address is still a change even when a case-insensitive comparison such asequalsIgnoreCasetreats the characters as equal, for example the dotlessıandiA change to a different address still requires verification
Tests
KcOidcBrokerTrustEmailUppercaseEmailTestbrokers a user whose address is sent asProbe@gıthub.testwith Trust Email and Verify email enabled and asserts that the sign-in completes and that the created user has a verified addressA second case submits
probe@github.test, which differs from the provider address by the dotlessıagainsti, and asserts that the email verification page is shown, that the submitted address is stored withemailVerified=falseand that the verification e-mail is addressed to itVerified locally
testTrustEmailAppliedWhenEmailDiffersOnlyInCaseandtestChangedEmailRequiresVerificationpass with this change, while the second case fails without itemailVerified=falseplus aVERIFY_EMAILrequired action before the change and completes the sign-in withemailVerified=trueafter it, while a genuine e-mail change still triggers verificationKC_TEST_SERVER=embedded mvnw -pl tests/base -Dtest=KcOidcBrokerTrustEmailUppercaseEmailTest testpassesKC_TEST_SERVER=embedded mvnw -pl tests/base -Dtest=KcOidcBrokerTest testpassesKC_TEST_SERVER=embedded mvnw -pl tests/base -Dtest=KcOidcFirstBrokerLoginDetectExistingUserTest,IdpCreateUserIfUniqueAuthenticatorTest testpassesmvnw -pl services,tests/base spotless:checkpassesFixes #54775