From 9eb54dc31dc774c755c383e5d715166adb079ef5 Mon Sep 17 00:00:00 2001 From: Ludo Mikula Date: Sun, 20 Sep 2026 18:21:58 +0200 Subject: [PATCH 1/2] fix: email sanitization --- .../lowcoder/domain/user/model/AuthUser.java | 27 +- .../domain/user/service/UserServiceImpl.java | 158 +++- .../org/lowcoder/sdk/exception/BizError.java | 1 + .../org/lowcoder/sdk/util/EmailUtils.java | 84 ++ .../src/main/resources/locale_de.properties | 1 + .../src/main/resources/locale_en.properties | 1 + .../src/main/resources/locale_zh.properties | 1 + .../org/lowcoder/sdk/util/EmailUtilsTest.java | 227 ++++++ .../request/form/FormAuthRequest.java | 9 + .../service/AuthenticationApiServiceImpl.java | 32 +- .../api/usermanagement/UserController.java | 23 +- .../runner/migrations/DatabaseChangelog.java | 11 + .../job/EmailNormalizationBackfill.java | 727 ++++++++++++++++++ .../EmailBackfillEndToEndTest.java | 260 +++++++ .../EmailNormalizationTest.java | 324 ++++++++ .../EmailSanitizationEndpointsTest.java | 365 +++++++++ .../job/EmailNormalizationBackfillTest.java | 503 ++++++++++++ 17 files changed, 2735 insertions(+), 19 deletions(-) create mode 100644 server/api-service/lowcoder-sdk/src/main/java/org/lowcoder/sdk/util/EmailUtils.java create mode 100644 server/api-service/lowcoder-sdk/src/test/java/org/lowcoder/sdk/util/EmailUtilsTest.java create mode 100644 server/api-service/lowcoder-server/src/main/java/org/lowcoder/runner/migrations/job/EmailNormalizationBackfill.java create mode 100644 server/api-service/lowcoder-server/src/test/java/org/lowcoder/api/authentication/EmailBackfillEndToEndTest.java create mode 100644 server/api-service/lowcoder-server/src/test/java/org/lowcoder/api/authentication/EmailNormalizationTest.java create mode 100644 server/api-service/lowcoder-server/src/test/java/org/lowcoder/api/usermanagement/EmailSanitizationEndpointsTest.java create mode 100644 server/api-service/lowcoder-server/src/test/java/org/lowcoder/runner/migrations/job/EmailNormalizationBackfillTest.java diff --git a/server/api-service/lowcoder-domain/src/main/java/org/lowcoder/domain/user/model/AuthUser.java b/server/api-service/lowcoder-domain/src/main/java/org/lowcoder/domain/user/model/AuthUser.java index 8f474da115..460665ed94 100644 --- a/server/api-service/lowcoder-domain/src/main/java/org/lowcoder/domain/user/model/AuthUser.java +++ b/server/api-service/lowcoder-domain/src/main/java/org/lowcoder/domain/user/model/AuthUser.java @@ -4,6 +4,7 @@ import lombok.*; import org.apache.commons.lang3.StringUtils; import org.lowcoder.domain.authentication.context.AuthRequestContext; +import org.lowcoder.sdk.util.EmailUtils; import java.util.Map; import java.util.Optional; @@ -40,13 +41,35 @@ public String getSource() { return getAuthContext().getAuthConfig().getSource(); } + /** + * The connection to persist for this authenticated identity. + * + *

Every path that creates a connection goes through here -- form register, SSO just-in-time + * provisioning and {@code /tp/link} all end at this method -- so it is where connection email values + * are put into canonical form. Normalizing here rather than at each call site is what keeps newly + * written connections consistent without having to find every writer. It is not the only path that + * creates a user: SCIM provisioning builds a {@code User} directly and deliberately attaches no + * connection, so it normalizes its own address + * (see {@code UserController#createSCIMUserAndAddToOrg}). + * + *

{@code email}, when present, is an address, and is normalized. It is frequently absent: several + * providers leave the email claim unset and carry the address in {@code username} instead, so null + * must survive as null rather than becoming {@code ""}. + * + *

{@code rawId} is only an address when the source is {@code EMAIL}; for every other source it is + * the opaque IdP subject and is stored byte-for-byte, because normalizing it would let the subject + * {@code AbC} authenticate as {@code abc}. + */ public Connection toAuthConnection() { return Connection.builder() .authId(getAuthContext().getAuthConfig().getId()) .source(getSource()) .name(getUsername()) - .email(getEmail()) - .rawId(getUid()) + // Null-preserving: an SSO provider that returns no email claim must leave this null, not "". + // An empty string is a value, and it would later look like an address that nothing can + // resolve. + .email(getEmail() == null ? null : EmailUtils.normalize(getEmail())) + .rawId(EmailUtils.normalizeIfEmailSource(getSource(), getUid())) .avatar(getAvatar()) .orgIds(StringUtils.isBlank(getOrgId()) ? Set.of() : Set.of(getOrgId())) .authConnectionAuthToken(Optional.ofNullable(authToken).map(ConnectionAuthToken::of).orElse(null)) diff --git a/server/api-service/lowcoder-domain/src/main/java/org/lowcoder/domain/user/service/UserServiceImpl.java b/server/api-service/lowcoder-domain/src/main/java/org/lowcoder/domain/user/service/UserServiceImpl.java index 1df76dfccf..e5837a943b 100644 --- a/server/api-service/lowcoder-domain/src/main/java/org/lowcoder/domain/user/service/UserServiceImpl.java +++ b/server/api-service/lowcoder-domain/src/main/java/org/lowcoder/domain/user/service/UserServiceImpl.java @@ -33,6 +33,7 @@ import org.lowcoder.sdk.constants.WorkspaceMode; import org.lowcoder.sdk.exception.BizError; import org.lowcoder.sdk.exception.BizException; +import org.lowcoder.sdk.util.EmailUtils; import org.lowcoder.sdk.util.HashUtils; import org.lowcoder.sdk.util.LocaleUtils; import org.springframework.dao.DuplicateKeyException; @@ -45,6 +46,7 @@ import java.security.SecureRandom; import java.time.Instant; +import java.util.Comparator; import java.time.temporal.ChronoUnit; import java.util.*; import java.util.function.Function; @@ -105,19 +107,93 @@ public Mono> getByIds(Collection ids) { ); } + /** + * Resolves a connection by source and subject, tolerating the case in which it was stored. + * + *

Two byte-exact probes rather than one normalized probe. Both are served by the existing + * {@code (connections.source, connections.rawId)} index, and between them they cover the two shapes that + * coexist while data is being cleaned up: rows written before normalization, which hold whatever casing + * the user typed, and rows written after it, which hold the normalized form. + * + *

A single normalized probe would be simpler and is what a fully migrated database wants -- but on an + * un-migrated one it silently stops finding every account whose stored {@code rawId} contains an + * uppercase letter, i.e. it locks those users out. This form has no such failure mode and needs no + * migration. + * + *

Known limitation. The second probe only helps when the STORED value is normalized. An account + * stored as {@code JoHn@DoE.com} is still not found by the input {@code john@doe.com}, because finding it + * would need a case-insensitive query, which MongoDB cannot serve from this index. So this closes + * duplicate creation going forward, and does not retroactively unify addresses that already differ in + * case. Converging those requires the backfill migration. + */ @Override public Mono findBySourceAndId(String source, String sourceUuid) { - return repository.findByConnections_SourceAndConnections_RawId(source, sourceUuid); + // normalizeIfEmailSource, never normalize: for a non-EMAIL source this value is the opaque IdP + // subject and must be matched byte-for-byte. + String normalized = EmailUtils.normalizeIfEmailSource(source, sourceUuid); + Mono asStored = repository.findByConnections_SourceAndConnections_RawId(source, sourceUuid); + if (StringUtils.equals(normalized, sourceUuid)) { + return asStored; + } + return asStored.switchIfEmpty( + repository.findByConnections_SourceAndConnections_RawId(source, normalized)); } public Mono findByName(String rawUuid) { return repository.findByName(rawUuid); } + /** Same two-probe reasoning as {@link #findBySourceAndId}, including the same known limitation. */ @Override public Mono findByEmailDeep(String email) { if(StringUtils.isEmpty(email)) return Mono.empty(); - return repository.findByEmailOrConnections_Email(email, email).next(); + String normalized = EmailUtils.normalize(email); + Mono asStored = probeByEmail(email); + if (StringUtils.equals(normalized, email)) { + return asStored; + } + return asStored.switchIfEmpty(probeByEmail(normalized)); + } + + /** + * Whether password recovery may act on this account. + * + *

Resolving by address rather than by {@code user.name} reaches strictly more rows: any casing, and + * anything matched through {@code connections[].email}. Soft-deleted accounts keep their {@code email} + * -- {@code markAsDeleted} only changes the state, the enabled flag and the connection sources -- so + * without this they become reachable targets for a reset. They cannot log in afterwards, so this is + * tidiness rather than a hole, but widening what an unauthenticated endpoint can mutate is not + * something to do by accident. + */ + private static boolean canRecoverPassword(User user) { + return user.getState() != UserState.DELETED; + } + + /** + * One probe of the email keyspace, resolved deterministically. + * + *

This used to be {@code .next()} on the repository's {@link Flux}, which returns whichever document + * storage happened to yield first. That was tolerable while only SSO linking and SCIM used this lookup, + * and is not now that password recovery does: the backfill deliberately leaves conflicting accounts in + * place, so two accounts matching one address is a state the system is designed to keep, and picking + * between them at random would make the reset target a coin flip. + */ + private Mono probeByEmail(String value) { + return repository.findByEmailOrConnections_Email(value, value) + .collectList() + .flatMap(candidates -> Mono.justOrEmpty(pickDeterministically(candidates, value))); + } + + /** + * An account whose own {@code email} matches wins over one reached only through a connection -- that is + * the account a human means by the address -- and identity breaks any remaining tie. + */ + static User pickDeterministically(List candidates, String value) { + return candidates.stream() + .min(Comparator + .comparingInt((User user) -> StringUtils.equals(user.getEmail(), value) ? 0 : 1) + .thenComparing(user -> StringUtils.defaultString(user.getId()))) + .orElse(null); } @Override @@ -166,7 +242,10 @@ public Mono findByAuthUserRawId(AuthUser authUser) { public Mono createNewUserByAuthUser(AuthUser authUser, boolean isSuperAdmin) { User.UserBuilder userBuilder = User.builder() .name(authUser.getUsername()) - .email(authUser.getEmail()) + // Normalized on write, null-preserving: an SSO provider that returns no email claim must + // leave this null rather than "". user.name is left alone -- it is a display name, and for + // SSO users it is a handle rather than an address. + .email(authUser.getEmail() == null ? null : EmailUtils.normalize(authUser.getEmail())) .state(UserState.ACTIVATED) .superAdmin(isSuperAdmin) .isEnabled(true) @@ -198,21 +277,70 @@ public Mono getUserAvatar(ServerWebExchange exchange, String userId) { .flatMap(user -> assetService.makeImageResponse(exchange, user.getAvatar())); } + /** + * Attaches an email address to an account that authenticated some other way. + * + *

Two guards, both of which the original had no equivalent for. + * + *

Shape. The endpoint takes a bare {@code @RequestParam} and validated nothing, so without + * this an empty string or a display name is stored as an address -- in {@code user.email} AND as a + * connection {@code rawId} the form login then resolves against. Registration validates shape; binding + * must too. + * + *

Ownership. The unique index on {@code (connections.source, connections.rawId)} used to be + * what stopped two accounts holding one address: a second bind of the same value collided and surfaced + * as {@link BizError#ALREADY_BIND}. Normalizing the value is exactly what stops that index from + * catching it -- binding {@code Legacy@Example.COM} now writes {@code legacy@example.com}, a different + * key, so an account already stored under the typed casing no longer collides. Without the probe + * below, adding normalization would therefore have removed a guarantee that existed before it. + * The probe restores it: fed the RAW value, the two-probe lookup reaches both the pre-normalization + * casing and the canonical one. + * + *

What it does not cover, for the same reason no lookup on this branch does: an address + * stored in mixed case is not reachable from lowercase input, so binding {@code legacy@example.com} + * while another account holds {@code Legacy@Example.COM} still writes a second row. That hole is + * unchanged from before this commit -- the byte-exact index never caught that pairing either. Changeset + * {@code 032} converges such rows, so what is left is the owner the backfill deliberately froze as part + * of a reported conflict, or an install where it has not run yet. It is pinned by + * {@code EmailSanitizationEndpointsTest#bindEmailStillMissesALegacyMixedCaseOwnerFromLowercaseInput}. + */ @Override public Mono bindEmail(User user, String email) { + String normalized = EmailUtils.normalize(email); + if (!EmailUtils.looksLikeEmail(normalized)) { + return Mono.error(new BizException(BizError.INVALID_EMAIL_FORMAT, "INVALID_EMAIL_FORMAT")); + } + return findBySourceAndId(AuthSourceConstants.EMAIL, email) + .flatMap(owner -> { + if (StringUtils.equals(owner.getId(), user.getId())) { + // Already this user's address. Writing again would append a second EMAIL connection + // differing from the first only in case, which is the very state this change exists + // to prevent, so the bind is idempotent instead. + return Mono.just(true); + } + return Mono.error( + new BizException(BizError.ALREADY_BIND, "ALREADY_BIND", normalized, "")); + }) + .switchIfEmpty(Mono.defer(() -> doBindEmail(user, normalized))); + } + + /** The write half of {@link #bindEmail}, deferred so it does not run while the ownership probe is still pending. */ + private Mono doBindEmail(User user, String normalized) { Connection connection = Connection.builder() .source(AuthSourceConstants.EMAIL) - .name(email) - .rawId(email) - .email(email) + .name(normalized) + .rawId(normalized) + .email(normalized) .build(); user.getConnections().add(connection); - user.setEmail(email); + user.setEmail(normalized); return repository.save(user) .then(Mono.just(true)) + // Still needed: the probe above is not atomic with the save, so a concurrent bind of the + // same address can still land between the two and be caught only by the index. .onErrorResume(throwable -> { if (throwable instanceof DuplicateKeyException) { - return Mono.error(new BizException(BizError.ALREADY_BIND, "ALREADY_BIND", email, "")); + return Mono.error(new BizException(BizError.ALREADY_BIND, "ALREADY_BIND", normalized, "")); } return Mono.error(throwable); }); @@ -287,7 +415,9 @@ public Mono resetPassword(String userId) { @Override public Mono lostPassword(String userEmail) { - return findByName(userEmail) + return findByEmailDeep(userEmail) + .switchIfEmpty(Mono.defer(() -> findByName(userEmail))) + .filter(UserServiceImpl::canRecoverPassword) .zipWhen(user -> orgMemberService.getCurrentOrgMember(user.getId()) .flatMap(orgMember -> organizationService.getById(orgMember.getOrgId())) .map(organization -> organization.getCommonSettings().getOrDefault(PASSWORD_RESET_EMAIL_TEMPLATE_DEFAULT, PASSWORD_RESET_EMAIL_TEMPLATE_DEFAULT))) @@ -308,9 +438,15 @@ public Mono lostPassword(String userEmail) { @Override public Mono resetLostPassword(String userEmail, String token, String newPassword) { - return findByName(userEmail) + return findByEmailDeep(userEmail) + .switchIfEmpty(Mono.defer(() -> findByName(userEmail))) + .filter(UserServiceImpl::canRecoverPassword) .flatMap(user -> { - if (Instant.now().until(user.getPasswordResetTokenExpiry(), ChronoUnit.MINUTES) <= 0) { + // Null when this account never requested a reset. Previously an NPE -- and a 500 on an + // unauthenticated endpoint -- reachable for any account resolvable by name; resolving by + // address reaches strictly more accounts, so guard it rather than widen the hole. + if (user.getPasswordResetTokenExpiry() == null + || Instant.now().until(user.getPasswordResetTokenExpiry(), ChronoUnit.MINUTES) <= 0) { return ofError(BizError.INVALID_PARAMETER, "TOKEN_EXPIRED"); } diff --git a/server/api-service/lowcoder-sdk/src/main/java/org/lowcoder/sdk/exception/BizError.java b/server/api-service/lowcoder-sdk/src/main/java/org/lowcoder/sdk/exception/BizError.java index 6319237662..d7ecb6f1ff 100644 --- a/server/api-service/lowcoder-sdk/src/main/java/org/lowcoder/sdk/exception/BizError.java +++ b/server/api-service/lowcoder-sdk/src/main/java/org/lowcoder/sdk/exception/BizError.java @@ -104,6 +104,7 @@ public enum BizError { ID_NOT_EXIST(500, 5620), DUPLICATE_AUTH_CONFIG_ADDITION(400, 5621), EMAIL_PROVIDER_DISABLED(403, 5622), + INVALID_EMAIL_FORMAT(400, 5623), // asset related, code range 5700 - 5799 diff --git a/server/api-service/lowcoder-sdk/src/main/java/org/lowcoder/sdk/util/EmailUtils.java b/server/api-service/lowcoder-sdk/src/main/java/org/lowcoder/sdk/util/EmailUtils.java new file mode 100644 index 0000000000..8c907c7054 --- /dev/null +++ b/server/api-service/lowcoder-sdk/src/main/java/org/lowcoder/sdk/util/EmailUtils.java @@ -0,0 +1,84 @@ +package org.lowcoder.sdk.util; + +import java.util.Locale; + +import org.apache.commons.lang3.StringUtils; +import org.lowcoder.sdk.constants.AuthSourceConstants; + +/** + * Canonical form for email addresses, and the shape check that decides whether a value is one. + * + *

Addresses are case-insensitive in practice, so {@code john@doe.com} and {@code JoHn@DoE.com} are one + * identity. Nothing in this codebase enforced that: no normalization anywhere, and no MongoDB collation, so + * the two registered as two separate accounts. + */ +public final class EmailUtils { + + private static final char AT = '@'; + + private EmailUtils() { + } + + /** + * Trim, then lower-case. Null-safe: returns {@code ""} rather than null, so callers can compare without + * guarding. + * + *

{@link Locale#ROOT} is not optional. The no-argument {@code toLowerCase()} uses the default locale, + * and under a Turkish locale it maps {@code I} to the dotless {@code ı} -- so the same address would + * canonicalize differently depending on the JVM's locale, which is exactly the class of bug this method + * exists to remove. + */ + public static String normalize(String value) { + if (value == null) { + return StringUtils.EMPTY; + } + return value.trim().toLowerCase(Locale.ROOT); + } + + /** + * Normalizes only when the value really is an email address. + * + *

A connection's {@code rawId} holds the address for {@code EMAIL} connections and the opaque IdP + * subject for every other source. Those subjects are case-sensitive: normalizing one would let the + * subject {@code AbC} authenticate as {@code abc}. So the source, not the field, decides. + * + *

Unlike {@link #normalize}, this is null-preserving: null in, null out, for every source. It + * is a field sanitizer -- it cleans a value that is being stored or matched, and must not invent a + * value where there was none, because {@code ""} is a value and would later read as an address that + * nothing can resolve. Use {@code normalize} instead when you need a non-null canonical form to + * compare against. + */ + public static String normalizeIfEmailSource(String source, String value) { + if (value == null) { + return null; + } + return AuthSourceConstants.EMAIL.equals(source) ? normalize(value) : value; + } + + /** + * Whether this value is shaped like an email address: exactly one {@code @}, a non-empty local part, a + * non-empty domain part, and no whitespace anywhere. + * + *

Deliberately no dot-in-domain requirement. {@code root@localhost} and {@code user@intranet} + * are real addresses on self-hosted instances, and rejecting them would lock those operators out. + * + *

This is a shape check, not a validity check -- it exists to keep values that are plainly not + * addresses (a GitHub handle, a display name, an empty string) out of fields the login paths treat as + * addresses. It is not an attempt to implement RFC 5322. + */ + public static boolean looksLikeEmail(String value) { + if (StringUtils.isBlank(value)) { + return false; + } + int at = value.indexOf(AT); + // no '@', or an empty local part + if (at <= 0) { + return false; + } + // more than one '@', or an empty domain part + if (at != value.lastIndexOf(AT) || at == value.length() - 1) { + return false; + } + return !StringUtils.containsWhitespace(value); + } +} diff --git a/server/api-service/lowcoder-sdk/src/main/resources/locale_de.properties b/server/api-service/lowcoder-sdk/src/main/resources/locale_de.properties index ed265a0807..931d8cd84b 100644 --- a/server/api-service/lowcoder-sdk/src/main/resources/locale_de.properties +++ b/server/api-service/lowcoder-sdk/src/main/resources/locale_de.properties @@ -100,6 +100,7 @@ CERTIFICATE_EMPTY=Zertifikat ist leer. USER_NOT_EXIST=Benutzer existiert nicht. DUPLICATE_AUTH_CONFIG_ADDITION=Dieser Anbieter wurde bereits zur Organisation hinzugefügt. EMAIL_PROVIDER_DISABLED=E-Mail-Anbieter ist deaktiviert. +INVALID_EMAIL_FORMAT=Bitte geben Sie eine gültige E-Mail-Adresse ein. SLUG_DUPLICATE_ENTRY=Slug existiert bereits. SLUG_INVALID=Slug-Format ist ungültig. FLOW_ERROR=Flow-Fehlermeldung: {0}. diff --git a/server/api-service/lowcoder-sdk/src/main/resources/locale_en.properties b/server/api-service/lowcoder-sdk/src/main/resources/locale_en.properties index f72df80a67..69afb3d0cf 100644 --- a/server/api-service/lowcoder-sdk/src/main/resources/locale_en.properties +++ b/server/api-service/lowcoder-sdk/src/main/resources/locale_en.properties @@ -283,6 +283,7 @@ DISABLE_AUTH_CONFIG_FORBIDDEN=Can not disable current administrator''s last iden USER_NOT_EXIST=User not exist. DUPLICATE_AUTH_CONFIG_ADDITION=Provider auth type already added to organization EMAIL_PROVIDER_DISABLED=Email provider is disabled. +INVALID_EMAIL_FORMAT=Please enter a valid email address. SLUG_DUPLICATE_ENTRY=Slug already exists SLUG_INVALID=Slug format is invalid FLOW_ERROR=Flow error message: {0} diff --git a/server/api-service/lowcoder-sdk/src/main/resources/locale_zh.properties b/server/api-service/lowcoder-sdk/src/main/resources/locale_zh.properties index fbb2598363..4a03fc8b6d 100644 --- a/server/api-service/lowcoder-sdk/src/main/resources/locale_zh.properties +++ b/server/api-service/lowcoder-sdk/src/main/resources/locale_zh.properties @@ -283,6 +283,7 @@ DISABLE_AUTH_CONFIG_FORBIDDEN=无法禁用当前管理员的最后一个身份 USER_NOT_EXIST=用户不存在。 DUPLICATE_AUTH_CONFIG_ADDITION=提供商身份验证类型已添加到组织中 EMAIL_PROVIDER_DISABLED=电子邮件提供商已禁用。 +INVALID_EMAIL_FORMAT=请输入有效的电子邮件地址。 SLUG_DUPLICATE_ENTRY=蛞蝓已经存在 SLUG_INVALID=Slug 格式无效 FLOW_ERROR=流程错误消息：{0} \ No newline at end of file diff --git a/server/api-service/lowcoder-sdk/src/test/java/org/lowcoder/sdk/util/EmailUtilsTest.java b/server/api-service/lowcoder-sdk/src/test/java/org/lowcoder/sdk/util/EmailUtilsTest.java new file mode 100644 index 0000000000..f13679627a --- /dev/null +++ b/server/api-service/lowcoder-sdk/src/test/java/org/lowcoder/sdk/util/EmailUtilsTest.java @@ -0,0 +1,227 @@ +package org.lowcoder.sdk.util; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertTrue; + +import java.util.Locale; + +import org.junit.After; +import org.junit.Test; +import org.lowcoder.sdk.constants.AuthSourceConstants; + +/** + * The form-login and SSO paths resolve an account by address through {@link EmailUtils}, so the exact + * behaviour of these three methods -- including their null and locale edges -- decides who can log in. + * (Password recovery is the exception: {@code UserServiceImpl#lostPassword} still resolves by + * {@code user.name} and does not go through this class.) + * + *

Each assertion prints what it checked, so a failure in CI shows the offending value rather than just a + * line number. + */ +public class EmailUtilsTest { + + private static final String CANONICAL = "john@doe.com"; + private static final String MIXED_CASE = "JoHn@DoE.CoM"; + private static final String GITHUB_SUBJECT = "AbC123"; + + /** Restores whatever locale the JVM started with, so the Turkish test cannot leak into the others. */ + private final Locale originalDefault = Locale.getDefault(); + + @After + public void restoreDefaultLocale() { + Locale.setDefault(originalDefault); + } + + private void checkNormalize(String input, String expected) { + String actual = EmailUtils.normalize(input); + System.out.printf("normalize(%s) -> %s (expected %s)%n", + quote(input), quote(actual), quote(expected)); + assertEquals("normalize(" + quote(input) + ")", expected, actual); + } + + private void checkLooksLikeEmail(String input, boolean expected) { + boolean actual = EmailUtils.looksLikeEmail(input); + System.out.printf("looksLikeEmail(%s) -> %s (expected %s)%n", quote(input), actual, expected); + assertEquals("looksLikeEmail(" + quote(input) + ")", expected, actual); + } + + private static String quote(String value) { + return value == null ? "null" : "\"" + value + "\""; + } + + // ---------------------------------------------------------------- normalize + + @Test + public void normalizeLowerCasesAndTrims() { + checkNormalize(MIXED_CASE, CANONICAL); + checkNormalize(" " + MIXED_CASE + " ", CANONICAL); + checkNormalize("\t" + CANONICAL + "\n", CANONICAL); + checkNormalize(CANONICAL, CANONICAL); + } + + /** + * The whole point of normalizing is that it converges. Applying it twice must not move the value again, + * or a value normalized on write would stop matching the same value normalized on read. + */ + @Test + public void normalizeIsIdempotent() { + String once = EmailUtils.normalize(" " + MIXED_CASE + " "); + String twice = EmailUtils.normalize(once); + System.out.printf("normalize twice: %s -> %s -> %s%n", quote(MIXED_CASE), quote(once), quote(twice)); + assertEquals(once, twice); + } + + /** Null-safe by contract: callers compare the result without guarding, so it must never be null. */ + @Test + public void normalizeReturnsEmptyStringForNullAndBlank() { + checkNormalize(null, ""); + checkNormalize("", ""); + checkNormalize(" ", ""); + } + + /** + * {@code String.trim()} only strips characters at or below U+0020, so interior whitespace survives. That + * is deliberate -- {@link EmailUtils#looksLikeEmail} is what rejects such values; normalize does not + * silently repair them into something that looks valid. + */ + @Test + public void normalizeDoesNotStripInteriorWhitespace() { + checkNormalize(" John Doe@Example.com ", "john doe@example.com"); + assertFalse("a value with interior whitespace must still fail the shape check", + EmailUtils.looksLikeEmail(EmailUtils.normalize(" John Doe@Example.com "))); + } + + /** + * The reason {@code toLowerCase(Locale.ROOT)} is spelled out in the implementation. Under a Turkish + * locale the no-argument {@code toLowerCase()} maps {@code I} to the dotless {@code ı} (U+0131), so + * {@code INFO@X.COM} would canonicalize differently depending on the JVM's locale -- two canonical forms + * for one address, which is exactly the bug this class exists to remove. + */ + @Test + public void normalizeIsIndependentOfTheDefaultLocale() { + Locale.setDefault(new Locale("tr", "TR")); + + String localeSensitive = "INFO@EXAMPLE.COM".toLowerCase(); + String normalized = EmailUtils.normalize("INFO@EXAMPLE.COM"); + System.out.printf("under tr-TR: toLowerCase() -> %s, EmailUtils.normalize() -> %s%n", + quote(localeSensitive), quote(normalized)); + + assertEquals("normalize must use Locale.ROOT", "info@example.com", normalized); + assertFalse("sanity: the default-locale lower-case really does differ under tr-TR, " + + "so this test is actually exercising the difference", + localeSensitive.equals(normalized)); + } + + // -------------------------------------------------- normalizeIfEmailSource + + @Test + public void normalizeIfEmailSourceNormalizesOnlyEmailConnections() { + String normalized = EmailUtils.normalizeIfEmailSource(AuthSourceConstants.EMAIL, " " + MIXED_CASE + " "); + System.out.printf("normalizeIfEmailSource(EMAIL, %s) -> %s%n", quote(MIXED_CASE), quote(normalized)); + assertEquals(CANONICAL, normalized); + } + + /** + * For every non-EMAIL source the value is the IdP's opaque subject, not an address. Lower-casing one + * would let the subject {@code AbC123} authenticate as {@code abc123} -- a different account at the + * provider. + */ + @Test + public void normalizeIfEmailSourceLeavesOpaqueSubjectsByteExact() { + for (String source : new String[] { + AuthSourceConstants.GOOGLE, + AuthSourceConstants.GITHUB, + AuthSourceConstants.ORY, + AuthSourceConstants.KEYCLOAK, + "SOME_FUTURE_PROVIDER", + null }) { + String actual = EmailUtils.normalizeIfEmailSource(source, GITHUB_SUBJECT); + System.out.printf("normalizeIfEmailSource(%s, %s) -> %s%n", + quote(source), quote(GITHUB_SUBJECT), quote(actual)); + assertSame("source " + quote(source) + " must pass the subject through untouched", + GITHUB_SUBJECT, actual); + } + } + + /** + * Null-preserving for every source, unlike {@link EmailUtils#normalize}. A connection whose {@code rawId} + * or {@code email} is absent must stay absent: {@code ""} is a value, and a stored {@code ""} address + * would read as an address that no lookup can ever resolve. + */ + @Test + public void normalizeIfEmailSourcePreservesNull() { + for (String source : new String[] { AuthSourceConstants.EMAIL, AuthSourceConstants.GOOGLE, null }) { + String actual = EmailUtils.normalizeIfEmailSource(source, null); + System.out.printf("normalizeIfEmailSource(%s, null) -> %s%n", quote(source), quote(actual)); + assertNull("null must survive for source " + quote(source), actual); + } + } + + // ---------------------------------------------------------- looksLikeEmail + + @Test + public void looksLikeEmailAcceptsRealAddresses() { + checkLooksLikeEmail(CANONICAL, true); + checkLooksLikeEmail("John.Doe+tag@sub.example.co.uk", true); + checkLooksLikeEmail("a@b", true); + } + + /** + * No dot-in-domain requirement, on purpose. Self-hosted instances really do have accounts at + * {@code localhost} and at single-label intranet hosts, and rejecting those would lock those operators + * out of their own deployment. + */ + @Test + public void looksLikeEmailAcceptsDotlessDomains() { + checkLooksLikeEmail("root@localhost", true); + checkLooksLikeEmail("user@intranet", true); + } + + @Test + public void looksLikeEmailRejectsMissingOrEmptyParts() { + checkLooksLikeEmail(null, false); + checkLooksLikeEmail("", false); + checkLooksLikeEmail(" ", false); + checkLooksLikeEmail("notanemail", false); + checkLooksLikeEmail("@example.com", false); + checkLooksLikeEmail("user@", false); + checkLooksLikeEmail("@", false); + } + + @Test + public void looksLikeEmailRejectsMultipleAtSigns() { + checkLooksLikeEmail("a@b@c.com", false); + checkLooksLikeEmail("user@@example.com", false); + } + + /** + * Whitespace anywhere is a reject, including the leading and trailing kind. Callers must + * {@link EmailUtils#normalize} first -- that ordering is what the production call sites do, and this + * pins it so a future caller that forgets the trim gets a rejection rather than a stored + * {@code " john@doe.com "}. + */ + @Test + public void looksLikeEmailRejectsAnyWhitespaceAndRequiresNormalizeFirst() { + checkLooksLikeEmail("John Doe@example.com", false); + checkLooksLikeEmail("user@exa mple.com", false); + checkLooksLikeEmail("user\t@example.com", false); + checkLooksLikeEmail(" " + CANONICAL + " ", false); + + assertTrue("normalize() first is what makes a padded address acceptable", + EmailUtils.looksLikeEmail(EmailUtils.normalize(" " + CANONICAL + " "))); + } + + /** + * The values that actually reached production as if they were addresses: a display name and an empty + * string, both written by endpoints that validated nothing. + */ + @Test + public void looksLikeEmailRejectsTheValuesThatUsedToBeStoredAsAddresses() { + checkLooksLikeEmail("Iron Man", false); + checkLooksLikeEmail("octocat", false); + checkLooksLikeEmail("", false); + } +} diff --git a/server/api-service/lowcoder-server/src/main/java/org/lowcoder/api/authentication/request/form/FormAuthRequest.java b/server/api-service/lowcoder-server/src/main/java/org/lowcoder/api/authentication/request/form/FormAuthRequest.java index 0aba244914..f9744202a4 100644 --- a/server/api-service/lowcoder-server/src/main/java/org/lowcoder/api/authentication/request/form/FormAuthRequest.java +++ b/server/api-service/lowcoder-server/src/main/java/org/lowcoder/api/authentication/request/form/FormAuthRequest.java @@ -11,6 +11,7 @@ import org.lowcoder.sdk.constants.AuthSourceConstants; import org.lowcoder.sdk.exception.BizError; import org.lowcoder.sdk.exception.BizException; +import org.lowcoder.sdk.util.EmailUtils; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.stereotype.Component; import reactor.core.publisher.Mono; @@ -37,6 +38,14 @@ public Mono auth(AuthRequestContext authRequestContext) { if (AuthSourceConstants.EMAIL.equals(authConfig.getSource()) && authConfig instanceof EmailAuthConfig emailAuthConfig && emailAuthConfig.isEnableRegister()) { + // Shape check before anything is created. A blank loginId would otherwise be + // stored as an address and then resolve on a subsequent login with loginId="". + if (!EmailUtils.looksLikeEmail(EmailUtils.normalize(context.getLoginId()))) { + return ofError(BizError.INVALID_EMAIL_FORMAT, "INVALID_EMAIL_FORMAT"); + } + // Passed AS SUPPLIED on purpose: findBySourceAndId probes the value given and + // then its normalized form, so this now rejects JoHn@DoE.com when + // john@doe.com already exists -- the duplicate that used to get through. return userService.findBySourceAndId(authConfig.getSource(), context.getLoginId()) .flatMap(user -> ofError(BizError.USER_LOGIN_ID_EXIST, "USER_LOGIN_ID_EXIST")); } diff --git a/server/api-service/lowcoder-server/src/main/java/org/lowcoder/api/authentication/service/AuthenticationApiServiceImpl.java b/server/api-service/lowcoder-server/src/main/java/org/lowcoder/api/authentication/service/AuthenticationApiServiceImpl.java index ff15c81879..e351f0b268 100644 --- a/server/api-service/lowcoder-server/src/main/java/org/lowcoder/api/authentication/service/AuthenticationApiServiceImpl.java +++ b/server/api-service/lowcoder-server/src/main/java/org/lowcoder/api/authentication/service/AuthenticationApiServiceImpl.java @@ -41,6 +41,7 @@ import org.lowcoder.sdk.exception.BizException; import org.lowcoder.sdk.models.HasIdAndAuditing; import org.lowcoder.sdk.util.CookieHelper; +import org.lowcoder.sdk.util.EmailUtils; import org.springframework.security.core.context.ReactiveSecurityContextHolder; import org.springframework.stereotype.Service; import org.springframework.web.server.ServerWebExchange; @@ -244,13 +245,38 @@ public void updateConnection(AuthUser authUser, User user) { user.setActiveAuthId(oldConnection.getAuthId()); } + /** + * The connection this authenticated identity corresponds to. + * + *

The subject comparison has to agree with how {@code findBySourceAndId} resolved the user, or the + * lookup finds an account and this then fails to find its connection. That is not hypothetical: the + * lookup now also matches a stored {@code john@doe.com} against the input {@code JoHn@DoE.com}, and a + * byte-exact comparison here would throw on exactly those logins. + * + *

{@code normalizeIfEmailSource} on both sides, so this is case-insensitive for EMAIL connections and + * byte-exact for every other source -- where {@code rawId} is an opaque IdP subject. + * + *

Exact match first, case-insensitive only as a fallback. {@code User.connections} is a + * {@code HashSet}, so its iteration order is not a contract; a single case-insensitive pass would pick + * an arbitrary one of two connections that differ only in case -- a state legacy data can be in. Trying + * the byte-exact match first makes the choice deterministic and gives the connection the caller + * actually authenticated with. + */ @SuppressWarnings("OptionalGetWithoutIsPresent") protected Connection getAuthConnection(AuthUser authUser, User user) { - return user.getConnections() + String subject = EmailUtils.normalizeIfEmailSource(authUser.getSource(), authUser.getUid()); + List sameSource = user.getConnections() .stream() - .filter(connection -> authUser.getSource().equals(connection.getSource()) - && Objects.equals(connection.getRawId(), authUser.getUid())) + .filter(connection -> authUser.getSource().equals(connection.getSource())) + .toList(); + return sameSource.stream() + .filter(connection -> Objects.equals(connection.getRawId(), authUser.getUid())) .findFirst() + .or(() -> sameSource.stream() + .filter(connection -> Objects.equals( + EmailUtils.normalizeIfEmailSource(connection.getSource(), connection.getRawId()), + subject)) + .findFirst()) .get(); } diff --git a/server/api-service/lowcoder-server/src/main/java/org/lowcoder/api/usermanagement/UserController.java b/server/api-service/lowcoder-server/src/main/java/org/lowcoder/api/usermanagement/UserController.java index f6bb280a0c..5e0d3656c0 100644 --- a/server/api-service/lowcoder-server/src/main/java/org/lowcoder/api/usermanagement/UserController.java +++ b/server/api-service/lowcoder-server/src/main/java/org/lowcoder/api/usermanagement/UserController.java @@ -27,6 +27,7 @@ import org.lowcoder.sdk.constants.AuthSourceConstants; import org.lowcoder.sdk.exception.BizError; import org.lowcoder.sdk.exception.BizException; +import org.lowcoder.sdk.util.EmailUtils; import org.springframework.data.domain.Sort; import org.springframework.http.HttpStatus; import org.springframework.http.codec.multipart.Part; @@ -75,12 +76,28 @@ public Mono> createUserAndAddToOrg(@PathVariable String orgId, C @Override public Mono> createSCIMUserAndAddToOrg(@PathVariable String orgId, CreateUserRequest request) { + // The third writer of an address, and the only one that validated nothing. A blank value creates a + // row whose email is "" -- which findByEmailDeep can never resolve, since it short-circuits on + // empty. The lookup below is fed the value AS SUPPLIED rather than the normalized one, so the + // two-probe lookup gets both chances: the typed casing byte-exact, then the canonical form. It + // does NOT find a row stored in some third casing -- e.g. stored "Stored@Example.COM" is not + // reachable from "stored@example.com". Changeset 032 converges such rows, so what is left is one + // the backfill deliberately froze as part of a reported conflict, or an install where 032 has not + // run yet. Only what gets written is normalized. + // + // Authorize first, validate second: a caller who is not an org admin must be turned away by + // checkVisitorAdminRole regardless of what they sent, and must not be able to tell a malformed + // payload from a well-formed one by the error they get back. + String normalizedEmail = EmailUtils.normalize(request.email()); return orgApiService.checkVisitorAdminRole(orgId) .flatMap(__ -> { + if (!EmailUtils.looksLikeEmail(normalizedEmail)) { + return ofError(BizError.INVALID_EMAIL_FORMAT, "INVALID_EMAIL_FORMAT"); + } // For SCIM provisioning: Create a minimal user without auth connection // The auth connection will be added on first SSO login via JIT provisioning // This allows SCIM to pre-provision users while SSO handles authentication - + // Check if user already exists by email return userService.findByEmailDeep(request.email()) .flatMap(existingUser -> { @@ -92,8 +109,8 @@ public Mono> createSCIMUserAndAddToOrg(@PathVariable String orgI // Create new user without auth connection (placeholder for SSO) Mono.defer(() -> { User newUser = User.builder() - .name(request.email()) - .email(request.email()) + .name(normalizedEmail) + .email(normalizedEmail) .isEnabled(true) .build(); newUser.setConnections(new HashSet<>()); diff --git a/server/api-service/lowcoder-server/src/main/java/org/lowcoder/runner/migrations/DatabaseChangelog.java b/server/api-service/lowcoder-server/src/main/java/org/lowcoder/runner/migrations/DatabaseChangelog.java index 02d3fb8712..2379b1b44f 100644 --- a/server/api-service/lowcoder-server/src/main/java/org/lowcoder/runner/migrations/DatabaseChangelog.java +++ b/server/api-service/lowcoder-server/src/main/java/org/lowcoder/runner/migrations/DatabaseChangelog.java @@ -35,6 +35,7 @@ import org.lowcoder.runner.migrations.job.AddPtmFieldsJob; import org.lowcoder.runner.migrations.job.AddSuperAdminUser; import org.lowcoder.runner.migrations.job.CompleteAuthType; +import org.lowcoder.runner.migrations.job.EmailNormalizationBackfill; import org.lowcoder.runner.migrations.job.MigrateAuthConfigJob; import org.lowcoder.sdk.config.CommonConfig; import org.springframework.context.annotation.Profile; @@ -502,6 +503,16 @@ public void deleteOldSuperAdmin(MongockTemplate mongoTemplate, MongoDatabase mon } } + /** + * Backfills existing email addresses to the canonical form every write path has used since the + * sanitization change. Accounts that would collide are left as they are and reported; see + * {@link EmailNormalizationBackfill} and EMAIL-SANITIZATION.md. + */ + @ChangeSet(order = "032", id = "normalize-email-backfill", author = "") + public void normalizeEmailBackfill(MongoDatabase mongoDatabase) { + EmailNormalizationBackfill.run(mongoDatabase); + } + private void addGidField(MongockTemplate mongoTemplate, String collectionName) { // Create a query to match all documents Query query = new Query(); diff --git a/server/api-service/lowcoder-server/src/main/java/org/lowcoder/runner/migrations/job/EmailNormalizationBackfill.java b/server/api-service/lowcoder-server/src/main/java/org/lowcoder/runner/migrations/job/EmailNormalizationBackfill.java new file mode 100644 index 0000000000..084e82cdbe --- /dev/null +++ b/server/api-service/lowcoder-server/src/main/java/org/lowcoder/runner/migrations/job/EmailNormalizationBackfill.java @@ -0,0 +1,727 @@ +package org.lowcoder.runner.migrations.job; + +import java.nio.charset.StandardCharsets; +import java.time.Instant; +import java.util.ArrayList; +import java.util.Date; +import java.util.HashMap; +import java.util.HashSet; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Set; +import java.util.UUID; + +import org.bson.Document; +import org.lowcoder.sdk.constants.AuthSourceConstants; +import org.lowcoder.sdk.util.EmailUtils; +import org.lowcoder.sdk.util.HashUtils; + +import com.mongodb.MongoBulkWriteException; +import com.mongodb.MongoException; +import com.mongodb.bulk.BulkWriteError; +import com.mongodb.bulk.BulkWriteResult; +import com.mongodb.client.MongoCollection; +import com.mongodb.client.MongoCursor; +import com.mongodb.client.MongoDatabase; +import com.mongodb.client.model.BulkWriteOptions; +import com.mongodb.client.model.Filters; +import com.mongodb.client.model.IndexOptions; +import com.mongodb.client.model.Projections; +import com.mongodb.client.model.ReplaceOneModel; +import com.mongodb.client.model.ReplaceOptions; +import com.mongodb.client.model.UpdateOneModel; +import com.mongodb.client.model.WriteModel; + +import lombok.extern.slf4j.Slf4j; + +/** + * Backfills existing email addresses into the canonical form the application has written since the + * sanitization change: trimmed and lower-cased. See {@code EMAIL-SANITIZATION.md}. + * + *

Policy: converge what is unambiguous, freeze and report what is not. Where the survey sees two + * accounts converging on one normalized identity, every member of that group is left byte-for-byte as it + * is and written to {@value #REPORT_COLLECTION} for an operator to resolve. Nothing is merged, deleted or + * disabled. The dual-probe lookups in {@code UserServiceImpl} keep frozen rows reachable by their exact + * stored casing, so freezing costs nobody their login. + * + *

A collision that only appears after the survey is handled differently and more weakly: the + * write is refused by the index, and the document that lost is reported under {@code space: "write"}. The + * account that took the key is not part of that report -- it was not a conflict when it was read, and + * finding it would need a third pass. Such a group is therefore reported by one member, not all of them. + * + *

Nothing in the data it finds may prevent the application from starting. Mongock runs as an + * {@code ApplicationRunner} at boot and records a throwing changeset as FAILED, which by default stops + * startup. Worse, {@code AddSuperAdminRunner} writes the super-admin user from a {@code @PostConstruct} + * that ends in a bare {@code subscribe()}, so it is not ordered against this migration and can land a + * write mid-run on every boot; multi-replica deployments add ordinary login traffic from the other pods. + * The survey therefore cannot be the safety mechanism -- it is the policy enforcer, the one that makes a + * conflict group atomic. Safety comes from tolerating duplicate keys at write time, which is the same + * policy applied a moment later: a write that turns out to collide is not applied, it is reported. + * + *

Only data faults are tolerated. Infrastructure faults -- sockets, auth, write concern, any bulk error + * that is not {@value #DUPLICATE_KEY_ERROR} -- still propagate, because those should be loud. + */ +@Slf4j +public final class EmailNormalizationBackfill { + + public static final String USER_COLLECTION = "user"; + public static final String REPORT_COLLECTION = "emailNormalizationConflict"; + + static final String FIELD_ID = "_id"; + static final String FIELD_EMAIL = "email"; + static final String FIELD_CONNECTIONS = "connections"; + static final String FIELD_SOURCE = "source"; + static final String FIELD_RAW_ID = "rawId"; + + static final String REPORT_SPACE = "space"; + static final String REPORT_VALUE = "normalizedValue"; + static final String REPORT_USER_ID = "userId"; + static final String REPORT_STORED = "storedValues"; + static final String REPORT_PATH = "path"; + static final String REPORT_VALUE_FIELD = "value"; + static final String REPORT_RUN_ID = "runId"; + static final String REPORT_DETECTED_AT = "detectedAt"; + static final String SUMMARY_ID = "summary"; + + /** Two accounts converge on one EMAIL-connection {@code rawId} -- the key the unique index enforces. */ + static final String SPACE_RAW_ID = "connections.rawId"; + /** Two accounts converge in the {@code findByEmailDeep} keyspace -- {@code email} or a connection email. */ + static final String SPACE_EMAIL = "email"; + /** The write collided anyway: someone took the key between the survey and the write. */ + static final String SPACE_WRITE = "write"; + + static final String MONGO_SET = "$set"; + static final int BULK_BATCH_SIZE = 1000; + static final int CURSOR_BATCH_SIZE = 1000; + static final int PROGRESS_LOG_INTERVAL = 50_000; + static final int SAMPLE_LIMIT = 20; + static final int DUPLICATE_KEY_ERROR = 11000; + + private EmailNormalizationBackfill() { + } + + /** Changeset entry point. */ + public static Summary run(MongoDatabase db) { + return run(db, USER_COLLECTION, REPORT_COLLECTION); + } + + /** + * Collection names are parameters so tests can drive this against throwaway collections. The embedded + * mongod is shared across test classes, and the real {@code user} collection carries other suites' + * fixtures. + * + *

The absent-collection guard is load-bearing rather than defensive. {@code @ActiveProfiles} replaces + * the active profile set rather than adding to it, so every test class that names its own profile -- + * {@code emailSanitization}, {@code testGeneric}, {@code testFolder} and the rest -- does NOT have + * {@code test} active, {@code @Profile("!test")} matches, and Mongock runs this changeset at context + * startup against a database those suites have not populated yet. + */ + static Summary run(MongoDatabase db, String usersCollection, String reportCollection) { + if (!collectionExists(db, usersCollection)) { + log.info("032 email backfill: collection {} does not exist; nothing to do.", usersCollection); + return Summary.empty(); + } + MongoCollection users = db.getCollection(usersCollection); + MongoCollection report = db.getCollection(reportCollection); + + Survey survey = survey(users); + Summary summary = backfill(users, report, survey, UUID.randomUUID().toString(), Instant.now()); + ensureLookupIndexes(users); + return summary; + } + + private static boolean collectionExists(MongoDatabase db, String name) { + try (MongoCursor names = db.listCollectionNames().iterator()) { + while (names.hasNext()) { + if (name.equals(names.next())) { + return true; + } + } + } + return false; + } + + // -------------------------------------------------------------------------------- pass 1: survey + + /** + * Finds the normalized values that more than one account would end up owning. + * + *

Grouped in Java with {@link EmailUtils#normalize} -- the very function the backfill applies -- and + * not with a server-side {@code $group} on {@code $toLower}, which neither trims nor handles non-ASCII. + * Under that weaker key {@code " john@doe.com"} forms its own group, passes the gate, and then converges + * during the backfill, surfacing as a bare duplicate-key error: exactly what the gate exists to prevent. + * + *

Every document is scanned, including already-canonical ones. Grouping only the documents that need + * a write would miss the common shape: {@code A="john@doe.com"} needs no write, so {@code B="JoHn@DoE.com"} + * would look like a lone owner and be written straight onto A's key. + * + *

Holds two maps of normalized value to owning id for the length of the pass. This is the only part + * that does not stream; at a million users it is a few hundred MB and well inside the container. + */ + static Survey survey(MongoCollection users) { + Map rawIdOwners = new HashMap<>(); + Map emailOwners = new HashMap<>(); + Set conflictingRawIds = new HashSet<>(); + Set conflictingEmails = new HashSet<>(); + long scanned = 0; + + try (MongoCursor cursor = users.find() + .projection(Projections.include(FIELD_ID, FIELD_EMAIL, FIELD_CONNECTIONS)) + .batchSize(CURSOR_BATCH_SIZE) + .noCursorTimeout(true) + .iterator()) { + while (cursor.hasNext()) { + Document doc = cursor.next(); + scanned++; + Object id = doc.get(FIELD_ID); + + for (String key : emailConnectionRawIdKeys(doc)) { + claim(rawIdOwners, conflictingRawIds, key, id); + } + for (String key : lookupEmailKeys(doc)) { + claim(emailOwners, conflictingEmails, key, id); + } + + if (scanned % PROGRESS_LOG_INTERVAL == 0) { + log.info("032 email backfill survey: scanned {}.", scanned); + } + } + } + log.info("032 email backfill survey: scanned {} documents, found {} conflicting rawId values and " + + "{} conflicting email values.", scanned, conflictingRawIds.size(), conflictingEmails.size()); + return new Survey(conflictingRawIds, conflictingEmails, scanned); + } + + /** + * Records this document as an owner of the key, marking the key conflicting when a different document + * already owns it. Ids are compared as stored objects: {@code ObjectId} has value equality, and + * {@code toString} would merge an {@code ObjectId} with a String of the same hex. + */ + private static void claim(Map owners, Set conflicting, String key, Object id) { + Object previous = owners.putIfAbsent(key, id); + if (previous != null && !Objects.equals(previous, id)) { + conflicting.add(key); + } + } + + /** + * The normalized EMAIL-connection {@code rawId}s this document would own, deduplicated. + * + *

Keyed on {@code source} equal to {@code EMAIL} and nothing else, which is what the unique index is + * keyed on. Two consequences that both matter: + * + *

+ * + *

Values that normalize to empty are excluded here and never written, so they cannot converge onto a + * shared {@code ""} key. + */ + static Set emailConnectionRawIdKeys(Document doc) { + Set keys = new LinkedHashSet<>(); + for (Object element : rawConnections(doc)) { + if (!(element instanceof Document connection) + || !AuthSourceConstants.EMAIL.equals(stringOrNull(connection, FIELD_SOURCE))) { + continue; + } + String rawId = stringOrNull(connection, FIELD_RAW_ID); + if (rawId == null) { + continue; + } + String normalized = EmailUtils.normalize(rawId); + if (!normalized.isEmpty()) { + keys.add(normalized); + } + } + return keys; + } + + /** + * The normalized values this document would own in the {@code findByEmailDeep} keyspace. + * + *

That lookup is {@code {$or: [{email: v}, {"connections.email": v}]}}, so both fields share one + * keyspace and connection emails count for every source, not just EMAIL. + * + *

Gated on {@link EmailUtils#looksLikeEmail}. Changeset {@code 023} ran + * {@code update.set("email", document.getString("name"))} for every user, so on an upgraded install + * {@code user.email} is full of display names, handles, empty strings and nulls. Those are not + * addresses: normalizing {@code "Iron Man"} to {@code "iron man"} is a visible regression in the UI for + * no benefit, and counting duplicates among them would freeze their owners' perfectly unambiguous + * {@code rawId} for reasons that have nothing to do with email case. + */ + static Set lookupEmailKeys(Document doc) { + Set keys = new LinkedHashSet<>(); + addEmailKey(keys, stringOrNull(doc, FIELD_EMAIL)); + for (Object element : rawConnections(doc)) { + if (element instanceof Document connection) { + addEmailKey(keys, stringOrNull(connection, FIELD_EMAIL)); + } + } + return keys; + } + + private static void addEmailKey(Set keys, String value) { + if (value == null) { + return; + } + String normalized = EmailUtils.normalize(value); + if (EmailUtils.looksLikeEmail(normalized)) { + keys.add(normalized); + } + } + + // ---------------------------------------------------------------------- pass 2: backfill + report + + static Summary backfill(MongoCollection users, MongoCollection report, Survey survey, + String runId, Instant runAt) { + List ops = new ArrayList<>(); + List reportRows = new ArrayList<>(); + List samples = new ArrayList<>(); + long scanned = 0; + long modified = 0; + long rawIdFrozen = 0; + long emailFrozen = 0; + long collidedAtWrite = 0; + long notConverged = 0; + + try (MongoCursor cursor = users.find() + .projection(Projections.include(FIELD_ID, FIELD_EMAIL, FIELD_CONNECTIONS)) + .batchSize(CURSOR_BATCH_SIZE) + .noCursorTimeout(true) + .iterator()) { + while (cursor.hasNext()) { + Document doc = cursor.next(); + scanned++; + Object id = doc.get(FIELD_ID); + + boolean freezeRawId = intersects(emailConnectionRawIdKeys(doc), survey.conflictingRawIds()); + boolean freezeEmail = intersects(lookupEmailKeys(doc), survey.conflictingEmails()); + if (freezeRawId) { + rawIdFrozen++; + collectConflictRows(reportRows, samples, doc, id, SPACE_RAW_ID, survey, runId, runAt); + } + if (freezeEmail) { + emailFrozen++; + collectConflictRows(reportRows, samples, doc, id, SPACE_EMAIL, survey, runId, runAt); + } + + UpdateOneModel operation = buildBackfillOperation(doc, freezeRawId, freezeEmail); + if (operation != null) { + ops.add(new Op(id, targetValueOf(doc, freezeRawId), operation)); + modified++; + } + if (ops.size() >= BULK_BATCH_SIZE) { + BulkOutcome outcome = flush(users, ops, reportRows, runId, runAt); + collidedAtWrite += outcome.collided().size(); + notConverged += outcome.notConverged(); + ops.clear(); + } + if (scanned % PROGRESS_LOG_INTERVAL == 0) { + log.info("032 email backfill: scanned {}, modified {}.", scanned, modified); + } + } + } + BulkOutcome last = flush(users, ops, reportRows, runId, runAt); + collidedAtWrite += last.collided().size(); + notConverged += last.notConverged(); + // Counted at queue time, so back out everything that did not actually land: a document whose key + // was taken, and one the compare-and-swap filter rejected. An operator reading "modified" after a + // production identity migration has to be able to trust it. + modified -= collidedAtWrite + notConverged; + + Summary summary = new Summary(scanned, modified, rawIdFrozen, emailFrozen, + survey.conflictingRawIds().size() + survey.conflictingEmails().size(), collidedAtWrite, + notConverged); + // The user data is the valuable part and it is already written. Losing the report is bad, but + // failing the changeset over it is worse: Mongock would record 032 FAILED and stop the application + // from starting, having already normalized the collection. + try { + upsertConflictRows(report, reportRows); + writeSummaryAndSweepStale(report, summary, runId, runAt); + } catch (RuntimeException e) { + log.error("032 email backfill: the data was migrated but the conflict report could not be " + + "written to {}. Re-run the changeset to regenerate it.", + report.getNamespace().getCollectionName(), e); + } + logOutcome(summary, samples, report.getNamespace().getCollectionName()); + return summary; + } + + private static BulkOutcome flush(MongoCollection users, List ops, + List reportRows, String runId, Instant runAt) { + BulkOutcome outcome = executeBulk(users, ops, "032 email backfill"); + for (Op op : outcome.collided()) { + reportRows.add(conflictRow(SPACE_WRITE, op.targetValue(), op.userId(), List.of(), runId, runAt)); + } + return outcome; + } + + /** + * The canonical address this operation is trying to claim, for the report row a late collision + * produces. Empty when the document is only having non-indexed email fields normalized, which cannot + * collide. + */ + private static String targetValueOf(Document doc, boolean freezeRawId) { + if (freezeRawId) { + return ""; + } + for (String key : emailConnectionRawIdKeys(doc)) { + return key; + } + return ""; + } + + private static boolean intersects(Set keys, Set conflicting) { + for (String key : keys) { + if (conflicting.contains(key)) { + return true; + } + } + return false; + } + + /** + * The single update this document needs, or null when nothing changes. + * + *

One operation per document, never split per field. Whole-document atomicity is what makes "frozen + * means untouched" true and keeps the report unambiguous; a half-applied document would be neither + * converged nor pristine, and an operator resolving the conflict could not tell which. + * + *

The filter is a compare-and-swap: every path written is pinned to the value it was read with, and + * a {@code rawId} write additionally pins its {@code source}. This subsumes pinning the array length -- + * if the array shrank, a dotted path matches nothing and the update is simply skipped, so MongoDB never + * pads the array to reach a stale index -- and unlike a length check it also covers reordering and a + * concurrent {@code markAsDeleted} turning {@code EMAIL} into a tombstone source mid-run. + */ + static UpdateOneModel buildBackfillOperation(Document doc, boolean freezeRawId, + boolean freezeEmail) { + Document set = new Document(); + List guards = new ArrayList<>(); + guards.add(Filters.eq(FIELD_ID, doc.get(FIELD_ID))); + + if (!freezeEmail) { + String email = stringOrNull(doc, FIELD_EMAIL); + if (email != null && changesToAnAddress(email)) { + set.append(FIELD_EMAIL, EmailUtils.normalize(email)); + guards.add(Filters.eq(FIELD_EMAIL, email)); + } + } + + List connections = rawConnections(doc); + for (int i = 0; i < connections.size(); i++) { + // Anything that is not a document -- nulls, scalars, nested arrays -- is skipped rather than + // cast. The index is the STORED index because the whole array was projected; projecting + // connections.source/connections.rawId instead would drop non-document elements and shift every + // later position, so a dotted write would land on a foreign IdP subject. + if (!(connections.get(i) instanceof Document connection)) { + continue; + } + String prefix = FIELD_CONNECTIONS + "." + i + "."; + String source = stringOrNull(connection, FIELD_SOURCE); + + if (!freezeRawId && AuthSourceConstants.EMAIL.equals(source)) { + String rawId = stringOrNull(connection, FIELD_RAW_ID); + // No shape gate here, unlike the email fields: rawId under an EMAIL source IS the login + // identifier by definition, so normalizing it always improves reachability. Empty results + // are suppressed so whitespace-only values cannot converge onto a shared "" key. + if (rawId != null && !EmailUtils.normalize(rawId).isEmpty() + && !EmailUtils.normalize(rawId).equals(rawId)) { + set.append(prefix + FIELD_RAW_ID, EmailUtils.normalize(rawId)); + guards.add(Filters.eq(prefix + FIELD_RAW_ID, rawId)); + guards.add(Filters.eq(prefix + FIELD_SOURCE, source)); + } + } + + if (!freezeEmail) { + String email = stringOrNull(connection, FIELD_EMAIL); + if (email != null && changesToAnAddress(email)) { + set.append(prefix + FIELD_EMAIL, EmailUtils.normalize(email)); + guards.add(Filters.eq(prefix + FIELD_EMAIL, email)); + } + } + } + + if (set.isEmpty()) { + return null; + } + return new UpdateOneModel<>(Filters.and(guards), new Document(MONGO_SET, set)); + } + + /** Whether this value is an address AND is not already canonical -- the condition for writing it. */ + private static boolean changesToAnAddress(String value) { + String normalized = EmailUtils.normalize(value); + return EmailUtils.looksLikeEmail(normalized) && !normalized.equals(value); + } + + // ------------------------------------------------------------------------------------- writing + + /** + * Applies the batch and returns the ids that could not be written because their key was already taken. + * + *

Unordered, so one rejected operation does not abandon the rest of the batch. A duplicate key here + * means the survey's picture went stale -- someone registered or bound the address while the migration + * ran -- which is the same condition the survey freezes for, discovered later. So it is reported and + * the run continues. Anything else is an infrastructure fault and propagates. + */ + static BulkOutcome executeBulk(MongoCollection collection, List ops, String what) { + if (ops.isEmpty()) { + return BulkOutcome.empty(); + } + List> models = new ArrayList<>(ops.size()); + for (Op op : ops) { + models.add(op.model()); + } + try { + BulkWriteResult result = collection.bulkWrite(models, new BulkWriteOptions().ordered(false)); + // With w=0 the counts are not zero, getMatchedCount throws outright, so check before reading. + if (!result.wasAcknowledged()) { + log.warn("{}: the connection is configured for unacknowledged writes (w=0), so this batch " + + "cannot be verified. Run the migration against an acknowledged connection.", what); + return BulkOutcome.empty(); + } + long shortfall = ops.size() - result.getMatchedCount(); + if (shortfall > 0) { + // The compare-and-swap filter rejected these: the documents changed under the migration. + // Not fatal and not a conflict -- they are simply not converged, and another run picks + // them up. Failing the changeset here would fail application startup instead. Returned + // rather than only logged, so the summary does not count them as normalized. + log.warn("{}: {} of {} operations matched nothing because the document changed during the " + + "run. Those documents are not normalized; re-run the migration.", what, shortfall, + ops.size()); + } + return new BulkOutcome(List.of(), shortfall); + } catch (MongoBulkWriteException e) { + List collided = new ArrayList<>(); + boolean onlyDuplicates = e.getWriteConcernError() == null; + for (BulkWriteError error : e.getWriteErrors()) { + if (error.getCode() == DUPLICATE_KEY_ERROR) { + collided.add(ops.get(error.getIndex())); + } else { + onlyDuplicates = false; + log.error("{} failed for _id {}: code {} -- {}", what, + ops.get(error.getIndex()).userId(), error.getCode(), error.getMessage()); + } + } + if (!onlyDuplicates) { + if (e.getWriteConcernError() != null) { + log.error("{} write concern error: code {} -- {}", what, + e.getWriteConcernError().getCode(), e.getWriteConcernError().getMessage()); + } + throw e; + } + log.warn("{}: {} documents could not be normalized because another account already holds the " + + "address. They are unchanged and reported.", what, collided.size()); + long applied = e.getWriteResult().wasAcknowledged() + ? e.getWriteResult().getMatchedCount() + : ops.size() - collided.size(); + return new BulkOutcome(collided, Math.max(0, ops.size() - collided.size() - applied)); + } catch (MongoException e) { + // Socket, command and write-concern failures carry no per-operation detail, so name the pass + // rather than letting a bare driver exception surface with no indication of what was running. + log.error("{} failed: {}", what, e.getMessage()); + throw e; + } + } + + // -------------------------------------------------------------------------------------- report + + private static void collectConflictRows(List rows, List samples, Document doc, + Object id, String space, Survey survey, String runId, Instant runAt) { + Set conflicting = SPACE_RAW_ID.equals(space) + ? survey.conflictingRawIds() + : survey.conflictingEmails(); + Set keys = SPACE_RAW_ID.equals(space) + ? emailConnectionRawIdKeys(doc) + : lookupEmailKeys(doc); + + for (String key : keys) { + if (!conflicting.contains(key)) { + continue; + } + rows.add(conflictRow(space, key, id, storedValuesFor(doc, space, key), runId, runAt)); + if (samples.size() < SAMPLE_LIMIT) { + samples.add(String.format("%s=%s held by _id %s", space, key, id)); + } + } + } + + /** The actual stored strings behind a conflicting key, with their dotted paths, for the report. */ + private static List storedValuesFor(Document doc, String space, String key) { + List stored = new ArrayList<>(); + boolean emailSpace = SPACE_EMAIL.equals(space); + + if (emailSpace) { + String email = stringOrNull(doc, FIELD_EMAIL); + if (email != null && key.equals(EmailUtils.normalize(email))) { + stored.add(new Document(REPORT_PATH, FIELD_EMAIL).append(REPORT_VALUE_FIELD, email)); + } + } + List connections = rawConnections(doc); + for (int i = 0; i < connections.size(); i++) { + if (!(connections.get(i) instanceof Document connection)) { + continue; + } + String prefix = FIELD_CONNECTIONS + "." + i + "."; + String field = emailSpace ? FIELD_EMAIL : FIELD_RAW_ID; + if (!emailSpace && !AuthSourceConstants.EMAIL.equals(stringOrNull(connection, FIELD_SOURCE))) { + continue; + } + String value = stringOrNull(connection, field); + if (value != null && key.equals(EmailUtils.normalize(value))) { + stored.add(new Document(REPORT_PATH, prefix + field).append(REPORT_VALUE_FIELD, value)); + } + } + return stored; + } + + /** + * One report row. The {@code _id} is a hash rather than the concatenation because an {@code _id} index + * key is capped at 1024 bytes and an address is not bounded; hashing also makes the row deterministic, + * so a re-run replaces it in place instead of accumulating duplicates. + */ + private static Document conflictRow(String space, String normalizedValue, Object userId, + List storedValues, String runId, Instant runAt) { + String key = space + "" + normalizedValue + "" + userId; + return new Document(FIELD_ID, HashUtils.hash(key.getBytes(StandardCharsets.UTF_8))) + .append(REPORT_SPACE, space) + .append(REPORT_VALUE, normalizedValue) + .append(REPORT_USER_ID, userId) + .append(REPORT_STORED, storedValues) + .append(REPORT_RUN_ID, runId) + .append(REPORT_DETECTED_AT, Date.from(runAt)); + } + + static void upsertConflictRows(MongoCollection report, List rows) { + if (rows.isEmpty()) { + return; + } + List> models = new ArrayList<>(rows.size()); + for (Document row : rows) { + models.add(new ReplaceOneModel<>(Filters.eq(FIELD_ID, row.get(FIELD_ID)), row, + new ReplaceOptions().upsert(true))); + if (models.size() >= BULK_BATCH_SIZE) { + report.bulkWrite(models, new BulkWriteOptions().ordered(false)); + models.clear(); + } + } + if (!models.isEmpty()) { + report.bulkWrite(models, new BulkWriteOptions().ordered(false)); + } + } + + /** + * Writes the one row an operator actually reads, then drops rows from earlier runs. + * + *

The sweep is what stops a resolved conflict haunting the collection forever. It runs only after a + * completed pass, so an aborted run can never delete rows that are still valid. + */ + static void writeSummaryAndSweepStale(MongoCollection report, Summary summary, String runId, + Instant runAt) { + Document doc = new Document(FIELD_ID, SUMMARY_ID) + .append(REPORT_RUN_ID, runId) + .append(REPORT_DETECTED_AT, Date.from(runAt)) + .append("scanned", summary.scanned()) + .append("modified", summary.modified()) + .append("rawIdFrozen", summary.rawIdFrozen()) + .append("emailFrozen", summary.emailFrozen()) + .append("conflictGroups", summary.conflictGroups()) + .append("collidedAtWrite", summary.collidedAtWrite()) + .append("notConverged", summary.notConverged()); + report.replaceOne(Filters.eq(FIELD_ID, SUMMARY_ID), doc, new ReplaceOptions().upsert(true)); + report.deleteMany(Filters.ne(REPORT_RUN_ID, runId)); + } + + private static void logOutcome(Summary summary, List samples, String reportCollection) { + log.info("032 email backfill complete: scanned {}, normalized {}.", summary.scanned(), + summary.modified()); + if (summary.notConverged() > 0) { + log.warn("032 email backfill: {} documents were changed by something else while the migration " + + "ran and were left un-normalized. Re-run the changeset to pick them up.", + summary.notConverged()); + } + if (summary.rawIdFrozen() == 0 && summary.emailFrozen() == 0 && summary.collidedAtWrite() == 0) { + return; + } + log.warn("032 email backfill left {} documents unchanged because their address is shared with " + + "another account ({} on the login identifier, {} in the email lookup, {} discovered " + + "at write time). They still work, by their stored casing. See collection {} -- " + + "its \"{}\" document has the counts. First {} of {}: {}", + summary.rawIdFrozen() + summary.emailFrozen() + summary.collidedAtWrite(), + summary.rawIdFrozen(), summary.emailFrozen(), summary.collidedAtWrite(), reportCollection, + SUMMARY_ID, samples.size(), summary.conflictGroups(), samples); + } + + // ------------------------------------------------------------------------------------- indexes + + /** + * Indexes the {@code findByEmailDeep} keyspace. Both fields, because that lookup is an {@code $or} and + * MongoDB can only serve an {@code $or} from indexes when every branch has one -- indexing {@code email} + * alone would still leave the query a collection scan. + * + *

It was already a scan on the SSO login, invite and SCIM paths; password recovery is about to start + * using it too, and that endpoint is unauthenticated, which makes an unindexed full scan there an + * amplification vector. + * + *

Plain, non-unique, non-sparse. Non-unique is the point: a unique index would hard-fail on exactly + * the conflict groups this migration has deliberately left in place. + */ + static void ensureLookupIndexes(MongoCollection users) { + // Same reasoning as the report: these are a performance improvement, not the migration's purpose. + // An install that already carries an index on the same keys under a different name makes + // createIndex throw, and that must not turn into a failed changeset and a refused startup. + try { + users.createIndex(new Document(FIELD_EMAIL, 1), new IndexOptions().name("email_1")); + users.createIndex(new Document(FIELD_CONNECTIONS + "." + FIELD_EMAIL, 1), + new IndexOptions().name("connections.email_1")); + } catch (RuntimeException e) { + log.warn("032 email backfill: could not create the email lookup indexes. findByEmailDeep will " + + "still work, as a collection scan. Create them by hand: {}", e.getMessage()); + } + } + + // ------------------------------------------------------------------------------------- helpers + + private static List rawConnections(Document doc) { + return doc.get(FIELD_CONNECTIONS) instanceof List list ? list : List.of(); + } + + /** The value at this key when it is a String, else null. Never throws on a wrongly-typed field. */ + private static String stringOrNull(Document doc, String field) { + return doc.get(field) instanceof String value ? value : null; + } + + /** The conflicting normalized values, in each of the two keyspaces that can produce them. */ + record Survey(Set conflictingRawIds, Set conflictingEmails, long scanned) { + } + + /** + * A pending update, the id it targets and the canonical value it is trying to claim, so a bulk error + * can name both the document and the address it lost. + */ + record Op(Object userId, String targetValue, UpdateOneModel model) { + } + + /** What a batch actually did: which operations lost a key, and how many the CAS filter rejected. */ + record BulkOutcome(List collided, long notConverged) { + + static BulkOutcome empty() { + return new BulkOutcome(List.of(), 0); + } + } + + /** The outcome of a run. Public because it is what the public entry point returns. */ + public record Summary(long scanned, long modified, long rawIdFrozen, long emailFrozen, + long conflictGroups, long collidedAtWrite, long notConverged) { + + static Summary empty() { + return new Summary(0, 0, 0, 0, 0, 0, 0); + } + } +} diff --git a/server/api-service/lowcoder-server/src/test/java/org/lowcoder/api/authentication/EmailBackfillEndToEndTest.java b/server/api-service/lowcoder-server/src/test/java/org/lowcoder/api/authentication/EmailBackfillEndToEndTest.java new file mode 100644 index 0000000000..7478cdfd1c --- /dev/null +++ b/server/api-service/lowcoder-server/src/test/java/org/lowcoder/api/authentication/EmailBackfillEndToEndTest.java @@ -0,0 +1,260 @@ +package org.lowcoder.api.authentication; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.lowcoder.sdk.exception.BizError.ALREADY_BIND; + +import java.time.Instant; +import java.time.temporal.ChronoUnit; +import java.util.HashSet; +import java.util.Set; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInstance; +import org.lowcoder.domain.encryption.EncryptionService; +import org.lowcoder.domain.user.model.Connection; +import org.lowcoder.domain.user.model.User; +import org.lowcoder.domain.user.model.UserState; +import org.lowcoder.domain.user.repository.UserRepository; +import org.lowcoder.domain.user.service.UserService; +import org.lowcoder.runner.migrations.job.EmailNormalizationBackfill; +import org.lowcoder.sdk.constants.AuthSourceConstants; +import org.lowcoder.sdk.exception.BizException; +import org.lowcoder.sdk.util.HashUtils; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.data.mongodb.core.MongoTemplate; +import org.springframework.test.context.ActiveProfiles; + +import lombok.extern.slf4j.Slf4j; + +/** + * Proves the backfill actually closes the holes the sanitization branch documented, exercised through the + * real services rather than against the migration's own collections. + * + *

Its own profile, so it gets its own application context and its own embedded mongod. It must not use + * the {@code test} profile: that shares one {@code user} collection with every other {@code test}-profile + * class, and running a real backfill across it would rewrite their fixtures. + * + *

A side effect of naming a profile is that {@code test} is not active, so {@code DatabaseChangelog} + * (which is {@code @Profile("!test")}) runs for real at context startup — including changeset 032 against + * an empty collection, and the changesets that build the unique sparse index this test then relies on. + */ +@SpringBootTest +@ActiveProfiles("emailBackfill") +@TestInstance(TestInstance.Lifecycle.PER_CLASS) +@Slf4j +public class EmailBackfillEndToEndTest { + + private static final String SOURCE = AuthSourceConstants.EMAIL; + private static final String PASSWORD = "lowcoder"; + + @Autowired + private UserService userService; + @Autowired + private UserRepository userRepository; + @Autowired + private EncryptionService encryptionService; + @Autowired + private MongoTemplate blockingMongoTemplate; + + /** Writes a row in the shape a pre-normalization install holds it: whatever casing the user typed. */ + private User seedLegacy(String storedAddress) { + Set connections = new HashSet<>(); + connections.add(Connection.builder() + .source(SOURCE) + .name(storedAddress) + .rawId(storedAddress) + .email(storedAddress) + .build()); + User user = userRepository.save(User.builder() + .name(storedAddress) + .email(storedAddress) + .password(encryptionService.encryptPassword(PASSWORD)) + .state(UserState.ACTIVATED) + .isEnabled(true) + .connections(connections) + .build()) + .block(); + assertNotNull(user); + return user; + } + + private void runBackfill() { + log.info("backfill: {}", EmailNormalizationBackfill.run(blockingMongoTemplate.getDb())); + } + + /** + * The headline fix. Before the backfill this lookup misses — it is the limitation + * {@code EmailNormalizationTest.legacyMixedCaseAccountIsNotReachableByItsLowercaseForm} pins. + */ + @Test + public void aLegacyMixedCaseAccountBecomesReachableByItsLowercaseForm() { + String stored = "Reachable.Legacy@Example.COM"; + String typed = "reachable.legacy@example.com"; + User seeded = seedLegacy(stored); + + assertNull(userService.findBySourceAndId(SOURCE, typed).block(), + "precondition: the lowercase form does not reach it before the backfill"); + + runBackfill(); + + User found = userService.findBySourceAndId(SOURCE, typed).block(); + assertNotNull(found, "after the backfill the canonical form must resolve the account"); + assertEquals(seeded.getId(), found.getId(), "and it must be the same account, not a new one"); + assertEquals(typed, found.getEmail(), "its stored address is now canonical"); + assertTrue(encryptionService.matchPassword(PASSWORD, found.getPassword()), + "the credential must survive the migration untouched"); + } + + /** The other half: the duplicate that the un-migrated row used to let through is now caught. */ + @Test + public void bindEmailFindsALegacyMixedCaseOwnerAfterTheBackfill() { + String stored = "Bindable.Legacy@Example.COM"; + String typed = "bindable.legacy@example.com"; + seedLegacy(stored); + runBackfill(); + + User binder = userRepository.save(User.builder() + .name("sso-only-binder-e2e") + .isEnabled(true) + .connections(new HashSet<>()) + .build()) + .block(); + assertNotNull(binder); + + BizException exception = null; + try { + userService.bindEmail(binder, typed).block(); + } catch (BizException e) { + exception = e; + } + assertNotNull(exception, "binding an address another account holds must now be rejected"); + log.info("bind rejected with {} / {}", exception.getError(), exception.getMessageKey()); + assertEquals(ALREADY_BIND, exception.getError()); + } + + /** + * Password recovery, which this change moved off {@code findByName} and onto the email lookup. Driven + * through {@code resetLostPassword} with the token seeded directly, so the test needs no SMTP. + */ + @Test + public void passwordRecoveryResolvesAnAccountWhateverCaseIsTyped() { + String stored = "Recover.Legacy@Example.COM"; + User seeded = seedLegacy(stored); + + String token = "reset-token-e2e"; + seeded.setPasswordResetToken(HashUtils.hash(token.getBytes())); + seeded.setPasswordResetTokenExpiry(Instant.now().plus(1, ChronoUnit.HOURS)); + userRepository.save(seeded).block(); + + runBackfill(); + + Boolean reset = userService.resetLostPassword("RECOVER.LEGACY@EXAMPLE.COM", token, "new-password") + .block(); + assertEquals(Boolean.TRUE, reset, "recovery must resolve the account regardless of typed case"); + + User after = userRepository.findById(seeded.getId()).block(); + assertNotNull(after); + assertTrue(encryptionService.matchPassword("new-password", after.getPassword()), + "and must actually have changed the password"); + } + + /** The pre-existing NPE this change would otherwise have made far more reachable. */ + @Test + public void passwordResetOnAnAccountThatNeverRequestedOneFailsCleanly() { + seedLegacy("NoToken.Legacy@Example.COM"); + runBackfill(); + + BizException exception = null; + try { + userService.resetLostPassword("notoken.legacy@example.com", "whatever", "new-password").block(); + } catch (BizException e) { + exception = e; + } + assertNotNull(exception, "an account with no reset token must produce a clean error, not an NPE"); + log.info("no-token reset rejected with {} / {}", exception.getError(), exception.getMessageKey()); + assertEquals("TOKEN_EXPIRED", exception.getMessageKey()); + } + + /** + * Resolving recovery by address rather than by {@code user.name} reaches strictly more rows, including + * soft-deleted accounts, which keep their {@code email}. They must not be resettable: a reset cannot + * log anyone in afterwards, but widening what an unauthenticated endpoint can mutate is not something + * to do by accident. + */ + @Test + public void passwordRecoveryIgnoresASoftDeletedAccount() { + String address = "Deleted.Legacy@Example.COM"; + User seeded = seedLegacy(address); + String token = "reset-token-deleted"; + seeded.setPasswordResetToken(HashUtils.hash(token.getBytes())); + seeded.setPasswordResetTokenExpiry(Instant.now().plus(1, ChronoUnit.HOURS)); + seeded.markAsDeleted(); + userRepository.save(seeded).block(); + + runBackfill(); + + Boolean reset = userService.resetLostPassword(address, token, "new-password").block(); + log.info("reset against a soft-deleted account returned {}", reset); + assertNull(reset, "a deleted account must not be a password-reset target"); + } + + /** + * The honest counterpart: a genuine conflict is left in place by design, so the lowercase form still + * does not reach it. This is what the collision policy costs, pinned so it stays visible. + */ + @Test + public void aConflictingPairIsLeftAloneAndStaysUnreachableByItsLowercaseForm() { + String lower = "conflict.pair@example.com"; + String upper = "Conflict.Pair@Example.COM"; + User canonical = seedLegacy(lower); + User mixed = seedLegacy(upper); + + runBackfill(); + + User reloadedMixed = userRepository.findById(mixed.getId()).block(); + assertNotNull(reloadedMixed); + assertEquals(upper, reloadedMixed.getEmail(), + "a conflicting account must be left byte-for-byte as it was"); + + User byLower = userService.findBySourceAndId(SOURCE, lower).block(); + assertNotNull(byLower); + assertEquals(canonical.getId(), byLower.getId(), + "the lowercase form still resolves the account that already held it"); + + User byUpper = userService.findBySourceAndId(SOURCE, upper).block(); + assertNotNull(byUpper, "and the frozen account is still reachable by its own stored casing, " + + "which is what makes freezing safe rather than a lockout"); + assertEquals(mixed.getId(), byUpper.getId()); + } + + /** The conflict report is the whole remediation story, so it has to actually land in the database. */ + @Test + public void aConflictIsRecordedInTheReportCollection() { + seedLegacy("reported.pair@example.com"); + seedLegacy("Reported.Pair@Example.COM"); + + runBackfill(); + + long rows = blockingMongoTemplate.getDb() + .getCollection(EmailNormalizationBackfill.REPORT_COLLECTION) + .countDocuments(com.mongodb.client.model.Filters.eq("normalizedValue", + "reported.pair@example.com")); + log.info("report rows for the conflicting pair: {}", rows); + assertTrue(rows >= 2, "both members of the group must be reported for an operator to resolve"); + } + + /** Re-running the changeset against already-converged data must be a no-op, not a second rewrite. */ + @Test + public void reRunningTheBackfillChangesNothing() { + seedLegacy("Rerun.Legacy@Example.COM"); + runBackfill(); + EmailNormalizationBackfill.Summary second = + EmailNormalizationBackfill.run(blockingMongoTemplate.getDb()); + log.info("second run: {}", second); + assertEquals(0, second.modified(), "a converged database must produce no writes on a re-run"); + } +} diff --git a/server/api-service/lowcoder-server/src/test/java/org/lowcoder/api/authentication/EmailNormalizationTest.java b/server/api-service/lowcoder-server/src/test/java/org/lowcoder/api/authentication/EmailNormalizationTest.java new file mode 100644 index 0000000000..3844cedbb8 --- /dev/null +++ b/server/api-service/lowcoder-server/src/test/java/org/lowcoder/api/authentication/EmailNormalizationTest.java @@ -0,0 +1,324 @@ +package org.lowcoder.api.authentication; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.lowcoder.sdk.exception.BizError.INVALID_EMAIL_FORMAT; +import static org.lowcoder.sdk.exception.BizError.USER_LOGIN_ID_EXIST; + +import java.util.Set; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; +import org.lowcoder.api.authentication.AuthenticationEndpoints.FormLoginRequest; +import org.lowcoder.domain.authentication.AuthenticationService; +import org.lowcoder.domain.authentication.FindAuthConfig; +import org.lowcoder.domain.encryption.EncryptionService; +import org.lowcoder.domain.user.model.Connection; +import org.lowcoder.domain.user.model.User; +import org.lowcoder.domain.user.model.UserState; +import org.lowcoder.domain.user.repository.UserRepository; +import org.lowcoder.sdk.auth.AbstractAuthConfig; +import org.lowcoder.sdk.constants.AuthSourceConstants; +import org.lowcoder.sdk.exception.BizException; +import org.lowcoder.sdk.util.EmailUtils; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.mock.http.server.reactive.MockServerHttpRequest; +import org.springframework.mock.web.server.MockServerWebExchange; +import org.springframework.test.context.ActiveProfiles; + +import lombok.extern.slf4j.Slf4j; + +/** + * End-to-end cover for email sanitization, driven through {@link AuthenticationController#formLogin} -- the + * same entry point the browser hits -- so it exercises the whole chain: the shape check in + * {@code FormAuthRequest}, the two-probe lookups in {@code UserServiceImpl}, the normalization in + * {@code AuthUser#toAuthConnection} and {@code createNewUserByAuthUser}, and the case-insensitive connection + * match in {@code AuthenticationApiServiceImpl#getAuthConnection}. + * + *

The bug being closed: {@code john@doe.com} and {@code JoHn@DoE.com} registered as two separate + * accounts, because nothing normalized and MongoDB has no collation on these fields. + * + *

The constraint being respected: an account stored with mixed case must keep working exactly as before, + * whether or not the backfill (changeset {@code 032}) has reached it -- and it never does here, because + * {@code DatabaseChangelog} is {@code @Profile("!test")}. {@link #legacyMixedCaseAccountStillLogsIn} is the + * regression guard, and {@link #legacyMixedCaseAccountIsNotReachableByItsLowercaseForm} pins the lookup + * limitation that remains for any row the backfill leaves in place. + * + *

Every test uses its own address: this class shares an application context, and therefore one embedded + * MongoDB, with the other {@code @ActiveProfiles("test")} test classes. + */ +@SpringBootTest +@ActiveProfiles("test") +@Slf4j +public class EmailNormalizationTest { + + private static final String SOURCE = AuthSourceConstants.EMAIL; + private static final String PASSWORD = "lowcoder"; + + @Autowired + private AuthenticationController authenticationController; + @Autowired + private UserRepository userRepository; + @Autowired + private EncryptionService encryptionService; + @Autowired + private AuthenticationService authenticationService; + + private String getEmailAuthConfigId() { + return authenticationService.findAuthConfigBySource(null, AuthSourceConstants.EMAIL) + .map(FindAuthConfig::authConfig) + .map(AbstractAuthConfig::getId) + .block(); + } + + /** Drives the real endpoint. {@code register} true is sign-up, false is sign-in. */ + private void formLogin(String loginId, boolean register) { + FormLoginRequest request = new FormLoginRequest(loginId, PASSWORD, register, SOURCE, getEmailAuthConfigId()); + MockServerWebExchange exchange = + MockServerWebExchange.builder(MockServerHttpRequest.post("").build()).build(); + authenticationController.formLogin(request, null, null, exchange).block(); + } + + private BizException formLoginExpectingFailure(String loginId, boolean register) { + try { + formLogin(loginId, register); + } catch (BizException e) { + return e; + } + throw new AssertionError("expected formLogin(\"" + loginId + "\", register=" + register + + ") to fail, but it succeeded"); + } + + private User findByRawId(String rawId) { + return userRepository.findByConnections_SourceAndConnections_RawId(SOURCE, rawId).block(); + } + + private Connection emailConnection(User user) { + return user.getConnections().stream() + .filter(connection -> SOURCE.equals(connection.getSource())) + .findFirst() + .orElseThrow(() -> new AssertionError("user " + user.getId() + " has no EMAIL connection")); + } + + private void logUser(String label, User user) { + if (user == null) { + log.info("{}: ", label); + return; + } + log.info("{}: id={} name={} email={} connections={}", label, user.getId(), user.getName(), + user.getEmail(), + user.getConnections().stream() + .map(c -> c.getSource() + ":" + c.getRawId() + " (email=" + c.getEmail() + ")") + .toList()); + } + + // ------------------------------------------------------------------ writes + + /** + * Registration is the main writer of an address. Whatever case the user typed, one canonical form has to + * land in {@code user.email} and in the connection, or the next login is a coin flip. + */ + @Test + public void registrationStoresTheAddressInCanonicalForm() { + String typed = "MiXeD.Case@Example.COM"; + String canonical = "mixed.case@example.com"; + + formLogin(typed, true); + + User user = findByRawId(canonical); + logUser("registered as " + typed, user); + assertNotNull(user, "the account must be reachable by the canonical rawId " + canonical); + + assertEquals(canonical, user.getEmail(), "user.email must be stored canonical"); + Connection connection = emailConnection(user); + assertEquals(canonical, connection.getRawId(), "connection.rawId must be stored canonical"); + assertEquals(canonical, connection.getEmail(), "connection.email must be stored canonical"); + + assertNull(findByRawId(typed), + "nothing may remain stored under the typed casing " + typed); + + // user.name keeps what the user typed. Pinned so a future change to name handling is a deliberate + // one. It is no longer a lookup key: UserServiceImpl#lostPassword and #resetLostPassword now + // resolve by address via findByEmailDeep, falling back to findByName only when that finds + // nothing, so password recovery is case-insensitive like login. + assertEquals(typed, user.getName(), "user.name is left as typed"); + } + + /** Surrounding whitespace is stripped by the same normalization, not stored into the identifier. */ + @Test + public void registrationTrimsSurroundingWhitespace() { + String typed = " padded@example.com\t"; + String canonical = "padded@example.com"; + + formLogin(typed, true); + + User user = findByRawId(canonical); + logUser("registered as " + EmailUtils.normalize(typed) + " (typed with padding)", user); + assertNotNull(user, "the account must be reachable by the trimmed rawId " + canonical); + assertEquals(canonical, user.getEmail()); + assertEquals(canonical, emailConnection(user).getRawId()); + } + + // ------------------------------------------------------------------- the bug + + /** + * The bug this branch exists to close. Before the fix, the second registration found nothing -- the + * lookup was a byte-exact match on the typed casing -- and happily created a second account for the same + * address. + */ + @Test + public void registrationRejectsADuplicateThatDiffersOnlyInCase() { + formLogin("dup@example.com", true); + + BizException exception = formLoginExpectingFailure("DuP@Example.COM", true); + log.info("second registration rejected with {} / {}", exception.getError(), exception.getMessageKey()); + + assertEquals(USER_LOGIN_ID_EXIST, exception.getError()); + assertEquals("USER_LOGIN_ID_EXIST", exception.getMessageKey()); + + assertNull(findByRawId("DuP@Example.COM"), "no second account may have been created"); + } + + /** + * The other half of the same bug: having registered once, the user must be able to sign in however they + * type it. This is also what proves {@code getAuthConnection} agrees with the lookup -- it runs on the + * login path after the user is resolved, and a byte-exact comparison there would throw on exactly this + * login rather than returning a connection. + */ + @Test + public void loginSucceedsWhenTheTypedCaseDiffersFromTheStoredAddress() { + String canonical = "caselogin@example.com"; + formLogin(canonical, true); + User afterRegister = findByRawId(canonical); + assertNotNull(afterRegister); + String userId = afterRegister.getId(); + + formLogin("CaseLogin@Example.COM", false); + + User afterLogin = findByRawId(canonical); + logUser("after mixed-case login", afterLogin); + assertNotNull(afterLogin, "the mixed-case login must resolve to the existing account"); + assertEquals(userId, afterLogin.getId(), "it must be the SAME account, not a new one"); + assertEquals(UserState.ACTIVATED, afterLogin.getState()); + assertTrue(encryptionService.matchPassword(PASSWORD, afterLogin.getPassword())); + + assertEquals(1, afterLogin.getConnections().size(), + "the login must reuse the existing connection, not append a second one"); + assertEquals(canonical, emailConnection(afterLogin).getRawId(), + "the stored rawId must stay canonical -- the login must not rewrite it to the typed casing"); + assertNull(findByRawId("CaseLogin@Example.COM"), "no second account may have been created"); + } + + // ----------------------------------------------------------- no regression + + /** + * The no-migration guarantee, as a test. An account written before normalization holds whatever case the + * user typed back then; signing in with that same value must still work. A single normalized probe would + * fail here -- it would look up {@code legacy@example.com}, find nothing, and lock the account out. + */ + @Test + public void legacyMixedCaseAccountStillLogsIn() { + String legacy = "Legacy@Example.COM"; + User seeded = userRepository.save(User.builder() + .name(legacy) + .email(legacy) + .password(encryptionService.encryptPassword(PASSWORD)) + .state(UserState.ACTIVATED) + .isEnabled(true) + .connections(Set.of(Connection.builder() + .source(SOURCE) + .name(legacy) + .rawId(legacy) + .email(legacy) + .build())) + .build()) + .block(); + assertNotNull(seeded); + logUser("seeded pre-normalization account", seeded); + + formLogin(legacy, false); + + User afterLogin = findByRawId(legacy); + logUser("after legacy login", afterLogin); + assertNotNull(afterLogin, "a pre-normalization account must still be found by its stored value"); + assertEquals(seeded.getId(), afterLogin.getId(), "it must be the SAME account, not a new one"); + assertEquals(legacy, emailConnection(afterLogin).getRawId(), + "logging in must not rewrite the stored value; only changeset 032 converges it"); + } + + /** + * The known limitation of two byte-exact probes, pinned deliberately. Reaching a stored + * {@code Stored@Example.COM} from the input {@code stored@example.com} needs a case-insensitive query, + * which MongoDB cannot serve from the existing {@code (connections.source, connections.rawId)} index. + * + *

This describes the lookup, and it stays true even though the backfill migration now ships. + * The migration converges stored data; it does not give the query case-insensitivity. So this remains + * the behaviour for every row the backfill does not rewrite -- a member of a reported conflict group, + * a row written by something outside the normalizing paths, or an install where changeset {@code 032} + * has not run yet. + * + *

It therefore keeps its assertions rather than being inverted. The converged case is covered + * separately, end to end, by + * {@code EmailBackfillEndToEndTest#aLegacyMixedCaseAccountBecomesReachableByItsLowercaseForm}; this + * test seeds its own row under the {@code test} profile, where {@code DatabaseChangelog} is disabled, + * so no migration ever touches it. + */ + @Test + public void legacyMixedCaseAccountIsNotReachableByItsLowercaseForm() { + String legacy = "Stored@Example.COM"; + userRepository.save(User.builder() + .name(legacy) + .email(legacy) + .password(encryptionService.encryptPassword(PASSWORD)) + .state(UserState.ACTIVATED) + .isEnabled(true) + .connections(Set.of(Connection.builder() + .source(SOURCE) + .name(legacy) + .rawId(legacy) + .email(legacy) + .build())) + .build()) + .block(); + + User byLowercase = findByRawId("stored@example.com"); + log.info("lookup of the lowercase form of a mixed-case stored account: {}", + byLowercase == null ? "" : byLowercase.getId()); + assertNull(byLowercase, "documents the limitation: converging these needs the backfill migration"); + } + + // --------------------------------------------------------------- shape check + + /** + * Registration used to accept anything, including the empty string, and store it as an address. An + * account whose address is {@code ""} is then resolvable by a subsequent login that also sends + * {@code ""} -- which is why the shape check runs before anything is created. + */ + @ParameterizedTest(name = "registration rejects [{0}]") + @ValueSource(strings = { + "", + " ", + "notanemail", + "@example.com", + "user@", + "a@b@c.com", + "John Doe@example.com", + "Iron Man", + }) + public void registrationRejectsValuesThatAreNotAddresses(String notAnAddress) { + BizException exception = formLoginExpectingFailure(notAnAddress, true); + log.info("registration of [{}] rejected with {} / {}", notAnAddress, exception.getError(), + exception.getMessageKey()); + + assertEquals(INVALID_EMAIL_FORMAT, exception.getError()); + assertEquals("INVALID_EMAIL_FORMAT", exception.getMessageKey()); + + assertNull(findByRawId(notAnAddress), "nothing may have been stored for [" + notAnAddress + "]"); + assertNull(findByRawId(EmailUtils.normalize(notAnAddress)), + "nothing may have been stored for the normalized form of [" + notAnAddress + "]"); + } +} diff --git a/server/api-service/lowcoder-server/src/test/java/org/lowcoder/api/usermanagement/EmailSanitizationEndpointsTest.java b/server/api-service/lowcoder-server/src/test/java/org/lowcoder/api/usermanagement/EmailSanitizationEndpointsTest.java new file mode 100644 index 0000000000..4d62401f94 --- /dev/null +++ b/server/api-service/lowcoder-server/src/test/java/org/lowcoder/api/usermanagement/EmailSanitizationEndpointsTest.java @@ -0,0 +1,365 @@ +package org.lowcoder.api.usermanagement; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.lowcoder.sdk.exception.BizError.ALREADY_BIND; +import static org.lowcoder.sdk.exception.BizError.INVALID_EMAIL_FORMAT; +import static org.lowcoder.sdk.exception.BizError.NOT_AUTHORIZED; + +import java.util.HashSet; + +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInstance; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; +import org.lowcoder.api.common.InitData; +import org.lowcoder.api.common.mockuser.WithMockUser; +import org.lowcoder.api.usermanagement.UserEndpoints.CreateUserRequest; +import org.lowcoder.domain.user.model.Connection; +import org.lowcoder.domain.user.model.User; +import org.lowcoder.domain.user.repository.UserRepository; +import org.lowcoder.domain.user.service.UserService; +import org.lowcoder.sdk.constants.AuthSourceConstants; +import org.lowcoder.sdk.exception.BizException; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.test.context.ActiveProfiles; +import reactor.core.publisher.Mono; +import reactor.test.StepVerifier; + +import lombok.extern.slf4j.Slf4j; + +/** + * Cover for the two address writers that {@link org.lowcoder.api.authentication.EmailNormalizationTest} does + * not reach: SCIM provisioning ({@link UserController#createSCIMUserAndAddToOrg}) and + * {@link UserService#bindEmail}, which attaches an address to an account that signed in via SSO. + * + *

Both wrote whatever they were handed. {@code bindEmail} takes a bare {@code @RequestParam} and wrote it + * to {@code user.email} and to a connection {@code rawId} that the form login then resolves against -- + * so an unvalidated value there is not just untidy data, it is a login identifier. + * + *

Fixtures come from {@code InitData}: {@code user01} is an ADMIN of {@code org01} and {@code user02} is + * an ordinary member, which is what the authorization-ordering test needs. + */ +@SpringBootTest +@ActiveProfiles("emailSanitization") +@TestInstance(TestInstance.Lifecycle.PER_CLASS) +@Slf4j +public class EmailSanitizationEndpointsTest { + + private static final String ORG_ID = WithMockUser.DEFAULT_CURRENT_ORG_ID; + private static final String NON_ADMIN_USER_ID = "user02"; + private static final String PASSWORD = "lowcoder"; + + @Autowired + private UserController userController; + @Autowired + private UserService userService; + @Autowired + private UserRepository userRepository; + @Autowired + private InitData initData; + + @BeforeAll + public void beforeAll() { + initData.init(); + } + + private BizException expectFailure(Mono mono, String what) { + try { + mono.block(); + } catch (BizException e) { + return e; + } + throw new AssertionError("expected " + what + " to fail, but it succeeded"); + } + + private User findByEmail(String email) { + return userRepository.findByEmailOrConnections_Email(email, email).next().block(); + } + + // ------------------------------------------------------- SCIM provisioning + + /** + * A SCIM client that sends a mixed-case address must produce one canonical row, or the next SSO login -- + * which resolves by email -- provisions a second account for the same person. + */ + @Test + @WithMockUser + public void scimProvisioningStoresTheAddressInCanonicalForm() { + String typed = "SCIM.User@Example.COM"; + String canonical = "scim.user@example.com"; + + userController.createSCIMUserAndAddToOrg(ORG_ID, new CreateUserRequest(typed, PASSWORD)).block(); + + User user = findByEmail(canonical); + log.info("SCIM-provisioned [{}] -> id={} name={} email={}", typed, + user == null ? null : user.getId(), + user == null ? null : user.getName(), + user == null ? null : user.getEmail()); + + assertNotNull(user, "the provisioned account must be reachable by the canonical address"); + assertEquals(canonical, user.getEmail(), "user.email must be stored canonical"); + // This endpoint has no display name to work with, so it uses the address for both fields. Both must + // therefore be canonical -- unlike form registration, where name is what the user typed. + assertEquals(canonical, user.getName(), "SCIM uses the address as the name, so it too is canonical"); + + assertNull(findByEmail(typed), "nothing may remain stored under the typed casing"); + } + + /** + * An empty address used to create a row whose {@code email} is {@code ""}. Nothing can ever resolve it: + * {@code findByEmailDeep} short-circuits on an empty value, so the next SCIM call for the same person + * creates yet another row. + */ + @ParameterizedTest(name = "SCIM rejects [{0}]") + @ValueSource(strings = { "", " ", "notanemail", "@example.com", "user@", "a@b@c.com", "Iron Man" }) + @WithMockUser + public void scimProvisioningRejectsValuesThatAreNotAddresses(String notAnAddress) { + BizException exception = expectFailure( + userController.createSCIMUserAndAddToOrg(ORG_ID, new CreateUserRequest(notAnAddress, PASSWORD)), + "SCIM provisioning of [" + notAnAddress + "]"); + log.info("SCIM provisioning of [{}] rejected with {} / {}", notAnAddress, exception.getError(), + exception.getMessageKey()); + + assertEquals(INVALID_EMAIL_FORMAT, exception.getError()); + assertEquals("INVALID_EMAIL_FORMAT", exception.getMessageKey()); + assertNull(findByEmail(notAnAddress), "nothing may have been stored for [" + notAnAddress + "]"); + } + + /** + * Authorization is checked before the payload is. A non-admin must get {@code NOT_AUTHORIZED} whatever + * they send, so that the error they see cannot be used to distinguish a malformed payload from a + * well-formed one. + */ + @Test + @WithMockUser(id = NON_ADMIN_USER_ID) + public void scimProvisioningRejectsANonAdminBeforeLookingAtThePayload() { + BizException exception = expectFailure( + userController.createSCIMUserAndAddToOrg(ORG_ID, new CreateUserRequest("", PASSWORD)), + "SCIM provisioning by a non-admin"); + log.info("SCIM provisioning by non-admin {} rejected with {} / {}", NON_ADMIN_USER_ID, + exception.getError(), exception.getMessageKey()); + + assertEquals(NOT_AUTHORIZED, exception.getError(), + "a non-admin must be turned away by the role check, not by the shape check"); + } + + // -------------------------------------------------------------- bindEmail + + /** + * Binding an address to an existing account writes it in three places at once -- {@code user.email}, the + * new connection's {@code rawId} and its {@code email}. All three must be canonical, because + * {@code rawId} is what the form login matches on. + */ + @Test + public void bindEmailStoresTheAddressInCanonicalForm() { + String typed = " Bound.User@Example.COM "; + String canonical = "bound.user@example.com"; + + User user = userRepository.save(User.builder() + .name("sso-only-user") + .isEnabled(true) + .connections(new HashSet<>()) + .build()) + .block(); + assertNotNull(user); + + StepVerifier.create(userService.bindEmail(user, typed)) + .expectNext(true) + .verifyComplete(); + + User bound = userRepository.findById(user.getId()).block(); + assertNotNull(bound); + log.info("bindEmail([{}]) -> email={} connections={}", typed, bound.getEmail(), + bound.getConnections().stream() + .map(c -> c.getSource() + ":" + c.getRawId() + " (email=" + c.getEmail() + ")") + .toList()); + + assertEquals(canonical, bound.getEmail(), "user.email must be stored canonical"); + + Connection connection = bound.getConnections().stream() + .filter(c -> AuthSourceConstants.EMAIL.equals(c.getSource())) + .findFirst() + .orElseThrow(() -> new AssertionError("bindEmail did not add an EMAIL connection")); + assertEquals(canonical, connection.getRawId(), + "connection.rawId is a login identifier and must be stored canonical"); + assertEquals(canonical, connection.getEmail(), "connection.email must be stored canonical"); + assertEquals(canonical, connection.getName(), "connection.name must be stored canonical"); + + assertTrue(userService.findBySourceAndId(AuthSourceConstants.EMAIL, "BOUND.USER@example.com") + .hasElement().block(), + "the bound address must now resolve however it is typed"); + } + + /** + * The regression that normalizing on its own would introduce. The unique index on + * {@code (connections.source, connections.rawId)} is what used to stop two accounts holding one + * address -- but it is byte-exact, so once the bind writes {@code legacy@example.com} where the + * existing account holds {@code Legacy@Example.COM}, the keys differ and the index waves it through. + * Both accounts would then be reachable by the same address in different casings. + */ + @Test + public void bindEmailRejectsAnAddressAnotherAccountAlreadyHoldsInADifferentCase() { + String legacy = "Taken.Legacy@Example.COM"; + + User owner = userRepository.save(User.builder() + .name(legacy) + .email(legacy) + .isEnabled(true) + .connections(newConnectionSet(legacy)) + .build()) + .block(); + assertNotNull(owner); + + User binder = userRepository.save(User.builder() + .name("sso-only-binder") + .isEnabled(true) + .connections(new HashSet<>()) + .build()) + .block(); + assertNotNull(binder); + + BizException exception = expectFailure(userService.bindEmail(binder, legacy), + "binding an address another account already holds"); + log.info("bindEmail([{}]) by {} rejected with {} / {} (already held by {})", legacy, binder.getId(), + exception.getError(), exception.getMessageKey(), owner.getId()); + + assertEquals(ALREADY_BIND, exception.getError()); + + User unchanged = userRepository.findById(binder.getId()).block(); + assertNotNull(unchanged); + assertNull(unchanged.getEmail(), "the rejected bind must not have written an address"); + assertTrue(unchanged.getConnections().isEmpty(), "the rejected bind must not have added a connection"); + } + + /** + * Re-binding an address the account already holds must not append a second EMAIL connection differing + * from the first only in case -- that is precisely the state this change exists to prevent. + */ + @Test + public void bindEmailIsIdempotentForAnAddressThisAccountAlreadyHolds() { + String legacy = "Own.Legacy@Example.COM"; + + User user = userRepository.save(User.builder() + .name(legacy) + .email(legacy) + .isEnabled(true) + .connections(newConnectionSet(legacy)) + .build()) + .block(); + assertNotNull(user); + + StepVerifier.create(userService.bindEmail(user, legacy)) + .expectNext(true) + .verifyComplete(); + + User after = userRepository.findById(user.getId()).block(); + assertNotNull(after); + log.info("re-bind of [{}] -> email={} connections={}", legacy, after.getEmail(), + after.getConnections().stream().map(c -> c.getSource() + ":" + c.getRawId()).toList()); + + assertEquals(1, after.getConnections().size(), + "re-binding must not append a second EMAIL connection"); + assertEquals(legacy, after.getConnections().iterator().next().getRawId(), + "re-binding must leave the stored value alone; only changeset 032 converges it"); + } + + /** + * The residual hole, pinned deliberately so it is not mistaken for a guarantee the probe gives. + * + *

The ownership probe reaches an owner stored under the typed casing and an owner stored + * canonically. It cannot reach one stored in a third casing, because that needs a + * case-insensitive query and MongoDB will not serve one from the byte-exact + * {@code (connections.source, connections.rawId)} index. + * + *

This is unchanged from before normalization was added: the index is byte-exact, so + * {@code legacy@example.com} and {@code Legacy@Example.COM} were always two distinct keys and this + * bind always succeeded. + * + *

The backfill migration now ships, and this still holds -- it describes the probe, not the + * stored data. Once changeset {@code 032} has converged the owner, the bind is rejected; that is + * covered by {@code EmailBackfillEndToEndTest#bindEmailFindsALegacyMixedCaseOwnerAfterTheBackfill}. + * What remains is the row the backfill deliberately leaves alone -- a member of a reported conflict + * group -- which this test keeps pinned. It seeds its own row under a profile whose migration has + * already run against an empty collection, so nothing converges it. + */ + @Test + public void bindEmailStillMissesALegacyMixedCaseOwnerFromLowercaseInput() { + String storedByOwner = "Missed.Legacy@Example.COM"; + String typedByBinder = "missed.legacy@example.com"; + + User owner = userRepository.save(User.builder() + .name(storedByOwner) + .email(storedByOwner) + .isEnabled(true) + .connections(newConnectionSet(storedByOwner)) + .build()) + .block(); + assertNotNull(owner); + + User binder = userRepository.save(User.builder() + .name("sso-only-binder-lowercase") + .isEnabled(true) + .connections(new HashSet<>()) + .build()) + .block(); + assertNotNull(binder); + + StepVerifier.create(userService.bindEmail(binder, typedByBinder)) + .expectNext(true) + .verifyComplete(); + + User bound = userRepository.findById(binder.getId()).block(); + assertNotNull(bound); + log.info("documented gap: [{}] bound to {} even though {} holds [{}]", typedByBinder, + bound.getId(), owner.getId(), storedByOwner); + + assertEquals(typedByBinder, bound.getEmail(), + "documents the gap: the probe cannot see a third casing, so the bind goes through"); + } + + private static HashSet newConnectionSet(String address) { + HashSet connections = new HashSet<>(); + connections.add(Connection.builder() + .source(AuthSourceConstants.EMAIL) + .name(address) + .rawId(address) + .email(address) + .build()); + return connections; + } + + /** + * The endpoint behind this takes a bare {@code @RequestParam} and validated nothing, so these values + * really could be written as a login identifier. + */ + @ParameterizedTest(name = "bindEmail rejects [{0}]") + @ValueSource(strings = { "", " ", "notanemail", "@example.com", "user@", "a@b@c.com", "Iron Man" }) + public void bindEmailRejectsValuesThatAreNotAddresses(String notAnAddress) { + User user = userRepository.save(User.builder() + .name("bind-reject-" + notAnAddress.hashCode()) + .isEnabled(true) + .connections(new HashSet<>()) + .build()) + .block(); + assertNotNull(user); + + BizException exception = expectFailure(userService.bindEmail(user, notAnAddress), + "bindEmail([" + notAnAddress + "])"); + log.info("bindEmail([{}]) rejected with {} / {}", notAnAddress, exception.getError(), + exception.getMessageKey()); + + assertEquals(INVALID_EMAIL_FORMAT, exception.getError()); + assertEquals("INVALID_EMAIL_FORMAT", exception.getMessageKey()); + + User unchanged = userRepository.findById(user.getId()).block(); + assertNotNull(unchanged); + assertNull(unchanged.getEmail(), "a rejected bind must not have written an address"); + assertTrue(unchanged.getConnections().isEmpty(), + "a rejected bind must not have added a connection"); + } +} diff --git a/server/api-service/lowcoder-server/src/test/java/org/lowcoder/runner/migrations/job/EmailNormalizationBackfillTest.java b/server/api-service/lowcoder-server/src/test/java/org/lowcoder/runner/migrations/job/EmailNormalizationBackfillTest.java new file mode 100644 index 0000000000..635c0c0e66 --- /dev/null +++ b/server/api-service/lowcoder-server/src/test/java/org/lowcoder/runner/migrations/job/EmailNormalizationBackfillTest.java @@ -0,0 +1,503 @@ +package org.lowcoder.runner.migrations.job; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.UUID; + +import org.bson.Document; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInstance; +import org.lowcoder.api.ServerApplication; +import org.lowcoder.runner.migrations.job.EmailNormalizationBackfill.Summary; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.data.mongodb.core.MongoTemplate; +import org.springframework.test.context.ActiveProfiles; + +import com.mongodb.client.MongoCollection; +import com.mongodb.client.MongoDatabase; +import com.mongodb.client.model.Filters; +import com.mongodb.client.model.IndexOptions; + +import lombok.extern.slf4j.Slf4j; + +/** + * Drives {@link EmailNormalizationBackfill} directly against the embedded mongod. + * + *

The changeset itself is never driven through Mongock here. Under {@code @ActiveProfiles("test")}, + * {@code DatabaseChangelog} is {@code @Profile("!test")} and Mongock skips it — which also means the real + * {@code (connections.source, connections.rawId)} unique sparse index does not exist in this context, so + * each test builds it on its own throwaway collection. That is a gift rather than a nuisance: it makes the + * duplicate-key path testable in isolation, which it would not be against the shared {@code user} + * collection. + * + *

Throwaway collections per test, because one embedded mongod is shared across every {@code test}-profile + * class and the real {@code user} collection holds other suites' fixtures. + * + *

{@code classes = ServerApplication.class} is required: {@code @SpringBootTest} only searches upwards + * from the test's own package for a {@code @SpringBootConfiguration}, and {@code org.lowcoder.api} is not an + * ancestor of {@code org.lowcoder.runner.migrations.job}. + */ +@SpringBootTest(classes = ServerApplication.class) +@ActiveProfiles("test") +@TestInstance(TestInstance.Lifecycle.PER_CLASS) +@Slf4j +public class EmailNormalizationBackfillTest { + + private static final String EMAIL = "EMAIL"; + private static final String GOOGLE = "GOOGLE"; + private static final String GENERIC = "GENERIC"; + /** What {@code User.markAsDeleted} leaves behind in {@code connections[].source}. */ + private static final String DELETED_EMAIL_SOURCE = "EMAIL(User deleted at 1700000000)"; + + @Autowired + private MongoTemplate blockingMongoTemplate; + + private MongoDatabase db; + private String usersName; + private String reportName; + private MongoCollection users; + private MongoCollection report; + + @BeforeEach + public void beforeEach() { + db = blockingMongoTemplate.getDb(); + String suffix = UUID.randomUUID().toString().replace("-", ""); + usersName = "testUser_" + suffix; + reportName = "testConflict_" + suffix; + db.createCollection(usersName); + users = db.getCollection(usersName); + report = db.getCollection(reportName); + // The real index from changeset 016, so collisions behave here exactly as they do in production. + users.createIndex(new Document("connections.source", 1).append("connections.rawId", 1), + new IndexOptions().unique(true).sparse(true)); + } + + @AfterEach + public void afterEach() { + users.drop(); + report.drop(); + } + + // ------------------------------------------------------------------------------------- helpers + + private static Document connection(String source, String rawId, String email, String name) { + return new Document("source", source).append("rawId", rawId).append("email", email) + .append("name", name); + } + + private void insert(String id, String email, Object... connections) { + users.insertOne(new Document("_id", id).append("email", email) + .append("connections", Arrays.asList(connections))); + } + + private Summary run() { + return EmailNormalizationBackfill.run(db, usersName, reportName); + } + + private Document reload(String id) { + Document doc = users.find(Filters.eq("_id", id)).first(); + assertNotNull(doc, "user " + id + " vanished"); + return doc; + } + + private Document conn(String id, int index) { + return (Document) ((List) reload(id).get("connections")).get(index); + } + + private long reportRows(String id) { + return report.countDocuments(Filters.eq("userId", id)); + } + + private void logDoc(String label, String id) { + log.info("{}: {}", label, reload(id).toJson()); + } + + // -------------------------------------------------------------------------------- happy path + + @Test + public void convergesAMixedCaseAccount() { + insert("u1", " JoHn@DoE.COM ", + connection(EMAIL, "JoHn@DoE.COM", "JoHn@DoE.COM", "JoHn@DoE.COM"), + connection(GOOGLE, "SuB-XyZ-123", "JoHn@DoE.COM", "John Doe")); + + Summary summary = run(); + logDoc("converged", "u1"); + + assertEquals("john@doe.com", reload("u1").getString("email"), "user.email must be canonical"); + assertEquals("john@doe.com", conn("u1", 0).getString("rawId"), "EMAIL rawId must be canonical"); + assertEquals("john@doe.com", conn("u1", 0).getString("email"), "connection email must be canonical"); + assertEquals("john@doe.com", conn("u1", 1).getString("email"), + "a non-EMAIL connection's email is still an address and is normalized"); + + // The two identity fields the migration must never touch. + assertEquals("SuB-XyZ-123", conn("u1", 1).getString("rawId"), + "an opaque IdP subject must survive byte-for-byte -- lowercasing it would let subject " + + "AbC authenticate as abc"); + assertEquals("JoHn@DoE.COM", conn("u1", 0).getString("name"), + "connection.name is a display field the write path does not normalize either"); + assertEquals(1, summary.modified()); + } + + /** + * {@code connections[].name} is deliberately excluded. {@code AuthUser.toAuthConnection} writes it as + * {@code .name(getUsername())} un-normalized, so every account created since the sanitization change + * already stores a raw-cased name; normalizing it here would fight the application and the divergence + * would reappear on the very next login. + */ + @Test + public void neverTouchesConnectionName() { + insert("u1", "john@doe.com", connection(EMAIL, "john@doe.com", "john@doe.com", "JoHn@DoE.COM")); + run(); + assertEquals("JoHn@DoE.COM", conn("u1", 0).getString("name")); + } + + @Test + public void isIdempotentAndWritesNothingOnASecondRun() { + insert("u1", "JoHn@DoE.COM", connection(EMAIL, "JoHn@DoE.COM", "JoHn@DoE.COM", "n")); + + assertEquals(1, run().modified(), "first run converges the document"); + Summary second = run(); + log.info("second run: {}", second); + assertEquals(0, second.modified(), "a converged document must produce no write at all"); + assertEquals("john@doe.com", reload("u1").getString("email")); + } + + @Test + public void createsNoEmailKeyForADocumentThatHasNone() { + users.insertOne(new Document("_id", "u1") + .append("connections", List.of(connection(EMAIL, "JoHn@DoE.COM", null, "n")))); + run(); + assertTrue(!reload("u1").containsKey("email"), + "a document with no email must not gain one -- normalize() is null-safe and an " + + "unconditional rule would write an explicit null"); + assertEquals("john@doe.com", conn("u1", 0).getString("rawId")); + } + + // ----------------------------------------------------------------------------------- freezing + + @Test + public void leavesBothMembersOfARawIdCollisionByteExact() { + insert("u1", "john@doe.com", connection(EMAIL, "john@doe.com", "john@doe.com", "n")); + insert("u2", "JoHn@DoE.COM", connection(EMAIL, "JoHn@DoE.COM", "JoHn@DoE.COM", "n")); + + Summary summary = run(); + logDoc("collision member 1", "u1"); + logDoc("collision member 2", "u2"); + + assertEquals("john@doe.com", conn("u1", 0).getString("rawId"), "already canonical, unchanged"); + assertEquals("JoHn@DoE.COM", conn("u2", 0).getString("rawId"), + "the conflicting account must be left exactly as it was"); + assertEquals("JoHn@DoE.COM", reload("u2").getString("email"), + "frozen means the whole document is untouched, not partially normalized"); + assertTrue(summary.rawIdFrozen() >= 2, "both members counted as frozen"); + assertTrue(reportRows("u1") > 0 && reportRows("u2") > 0, "both members must be reported"); + } + + /** + * Proves the policy is "every member of a conflicting group", not "first owner wins". A + * {@code putIfAbsent}-style implementation normalizes the first owner straight onto the others' key. + */ + @Test + public void leavesAllThreeMembersOfAGroupUntouched() { + insert("u1", null, connection(EMAIL, "John@Doe.com", null, "n")); + insert("u2", null, connection(EMAIL, "JOHN@DOE.COM", null, "n")); + insert("u3", null, connection(EMAIL, "jOhN@dOe.CoM", null, "n")); + + run(); + + assertEquals("John@Doe.com", conn("u1", 0).getString("rawId")); + assertEquals("JOHN@DOE.COM", conn("u2", 0).getString("rawId")); + assertEquals("jOhN@dOe.CoM", conn("u3", 0).getString("rawId")); + } + + /** + * A tombstone must not block a live account. {@code User.markAsDeleted} mangles the connection source, + * so the deleted row sits under a different index key and is a different group by construction. + */ + @Test + public void doesNotFoldADeletedTombstoneIntoALiveGroup() { + insert("live", null, connection(EMAIL, "JoHn@DoE.COM", null, "n")); + insert("dead", null, connection(DELETED_EMAIL_SOURCE, "john@doe.com", null, "n")); + + run(); + + assertEquals("john@doe.com", conn("live", 0).getString("rawId"), + "the live account must still converge"); + assertEquals("john@doe.com", conn("dead", 0).getString("rawId"), + "the tombstone's rawId is not under an EMAIL source, so it is never rewritten"); + assertEquals(DELETED_EMAIL_SOURCE, conn("dead", 0).getString("source")); + } + + /** + * Many OIDC providers set {@code sub} to the user's address. That subject lives under a different + * {@code source}, so it is a different index key and must not freeze the form account. Grouping on the + * bare value instead of the source-qualified key would freeze most of an SSO-heavy install. + */ + @Test + public void doesNotFoldAnOidcSubjectIntoAnEmailGroup() { + insert("form", null, connection(EMAIL, "JoHn@DoE.COM", null, "n")); + insert("sso", null, connection(GENERIC, "john@doe.com", null, "n")); + + run(); + + assertEquals("john@doe.com", conn("form", 0).getString("rawId"), + "the form account must converge despite an OIDC subject holding the same text"); + assertEquals("john@doe.com", conn("sso", 0).getString("rawId"), "the subject is untouched"); + } + + /** + * The two keyspaces are frozen independently. A shared flag would let changeset 023's pollution of + * {@code user.email} freeze a perfectly unambiguous login identifier. + */ + @Test + public void freezesOnlyTheEmailSideWhenOnlyEmailsCollide() { + insert("u1", "alice@x.com", connection(EMAIL, "alice-one@x.com", "alice@x.com", "n")); + insert("u2", null, connection(EMAIL, "Alice-Two@X.com", "Alice@X.com", "n")); + + run(); + logDoc("email-frozen but rawId-converged", "u2"); + + assertEquals("alice-two@x.com", conn("u2", 0).getString("rawId"), + "the login identifier does not collide, so it must still converge"); + assertEquals("Alice@X.com", conn("u2", 0).getString("email"), + "the connection email does collide, so it must be left alone"); + } + + // ----------------------------------------------------------------------- hostile document shapes + + @Test + public void leavesBlankNormalizingRawIdsAlone() { + insert("u1", null, connection(EMAIL, " ", null, "n")); + insert("u2", null, connection(EMAIL, "\t", null, "n")); + + run(); + + assertEquals(" ", conn("u1", 0).getString("rawId"), + "whitespace-only rawIds must not converge onto a shared empty key"); + assertEquals("\t", conn("u2", 0).getString("rawId")); + } + + @Test + public void leavesNonEmailShapedUserEmailAlone() { + insert("u1", "Iron Man", connection(EMAIL, "ironman@avengers.com", null, "n")); + users.insertOne(new Document("_id", "u2").append("email", 42)); + users.insertOne(new Document("_id", "u3").append("email", List.of("a@b.com"))); + + run(); + + assertEquals("Iron Man", reload("u1").getString("email"), + "changeset 023 wrote display names into email; lowercasing one is a visible UI regression"); + assertEquals(42, reload("u2").get("email"), "a non-string email must not throw or be coerced"); + assertEquals(List.of("a@b.com"), reload("u3").get("email")); + } + + /** + * The highest-severity bug class in this migration: if positions read do not match positions written, a + * dotted update lands on a foreign IdP subject. The whole array is projected precisely so the loop index + * is the stored index. + */ + @Test + public void isSafeOnRaggedConnectionArrays() { + List ragged = new ArrayList<>(); + ragged.add(connection(GOOGLE, "SuB-1", null, "n")); + ragged.add(null); + ragged.add("a bare string"); + ragged.add(42); + ragged.add(new Document("authId", "EMAIL")); + ragged.add(List.of("nested", "array")); + ragged.add(new Document()); + ragged.add(connection(EMAIL, "JoHn@DoE.COM", null, "n")); + users.insertOne(new Document("_id", "u1").append("connections", ragged)); + + run(); + logDoc("ragged", "u1"); + + List after = (List) reload("u1").get("connections"); + assertEquals(8, after.size(), "the array must keep its shape"); + assertEquals("SuB-1", ((Document) after.get(0)).getString("rawId"), "foreign subject intact"); + assertNull(after.get(1)); + assertEquals("a bare string", after.get(2)); + assertEquals(42, after.get(3)); + assertEquals("john@doe.com", ((Document) after.get(7)).getString("rawId"), + "only the genuine EMAIL connection at the stored index is rewritten"); + } + + /** + * One document holding two EMAIL connections differing only in case — a state the old {@code bindEmail} + * could produce. Both converge onto one key; multikey index keys dedupe within a document, so this is + * not a duplicate-key error. Asserted against the real index rather than assumed. + */ + @Test + public void normalizesTwoCaseVariantConnectionsInsideOneDocument() { + insert("u1", null, + connection(EMAIL, "John@Doe.com", null, "n"), + connection(EMAIL, "JOHN@DOE.COM", null, "n")); + + run(); + logDoc("two case-variant connections", "u1"); + + assertEquals("john@doe.com", conn("u1", 0).getString("rawId")); + assertEquals("john@doe.com", conn("u1", 1).getString("rawId")); + assertEquals(2, ((List) reload("u1").get("connections")).size(), + "per-document dedupe means this is one group with one owner, not a conflict"); + assertEquals(0, reportRows("u1"), "a document cannot conflict with itself"); + } + + // -------------------------------------------------------------- the race the survey cannot close + + /** + * The survey is not atomic with the backfill, and on this codebase that window is not theoretical: + * {@code AddSuperAdminRunner} writes the super-admin from a {@code @PostConstruct} ending in a bare + * {@code subscribe()}, so it is unordered against Mongock on every boot, and other replicas keep + * serving registrations throughout. + * + *

Simulated by surveying, then letting someone take the target key, then backfilling. The run must + * absorb the duplicate key and report it — if it threw, Mongock would record the changeset FAILED and + * the application would not start, which on a replicated deployment is a boot loop on exactly the + * installs that most need the migration. + */ + @Test + public void toleratesACollisionThatAppearsAfterTheSurveyAndReportsIt() { + insert("u2", null, connection(EMAIL, "JoHn@DoE.COM", null, "n")); + insert("u3", null, connection(EMAIL, "Other@Example.COM", null, "n")); + + EmailNormalizationBackfill.Survey survey = EmailNormalizationBackfill.survey(users); + assertTrue(survey.conflictingRawIds().isEmpty(), "nothing conflicts at survey time"); + + // Someone registers the canonical address in the window between the two passes. + insert("late", null, connection(EMAIL, "john@doe.com", null, "n")); + + Summary summary = EmailNormalizationBackfill.backfill(users, report, survey, + "run-" + UUID.randomUUID(), java.time.Instant.now()); + log.info("after a late collision: {}", summary); + + assertEquals("JoHn@DoE.COM", conn("u2", 0).getString("rawId"), + "the losing document must be left exactly as it was"); + assertEquals("other@example.com", conn("u3", 0).getString("rawId"), + "an unordered bulk write must still apply every operation that does not collide"); + assertEquals(1, summary.collidedAtWrite()); + assertEquals(1, summary.modified(), "the collided document must not be counted as normalized"); + assertTrue(reportRows("u2") > 0, "the late collision must reach the report"); + } + + /** + * A document the compare-and-swap filter rejects is not normalized, so it must not be counted as + * normalized either. {@code modified} is incremented when the operation is queued, which is before + * anyone knows whether it will land; an operator reading the summary of a production identity + * migration has to be able to trust that number. + */ + @Test + public void doesNotCountADocumentTheCompareAndSwapRejectedAsModified() { + insert("u1", null, connection(EMAIL, "JoHn@DoE.COM", null, "n")); + Document asRead = users.find(Filters.eq("_id", "u1")).first(); + assertNotNull(asRead); + var operation = EmailNormalizationBackfill.buildBackfillOperation(asRead, false, false); + assertNotNull(operation, "this document does need a write"); + + // Someone edits the document between the read and the write. + users.updateOne(Filters.eq("_id", "u1"), + new Document("$set", new Document("connections.0.rawId", "Changed@Example.COM"))); + + EmailNormalizationBackfill.BulkOutcome outcome = EmailNormalizationBackfill.executeBulk(users, + List.of(new EmailNormalizationBackfill.Op("u1", "john@doe.com", operation)), "test"); + log.info("stale write outcome: {}", outcome); + + assertEquals(1, outcome.notConverged(), "the rejected operation must be reported as not converged"); + assertTrue(outcome.collided().isEmpty(), "it is not a collision, just a stale read"); + assertEquals("Changed@Example.COM", conn("u1", 0).getString("rawId"), + "and the concurrent edit must survive -- the migration must not clobber it"); + } + + // ------------------------------------------------------------------------------------- report + + @Test + public void writesASummaryRow() { + insert("u1", null, connection(EMAIL, "John@Doe.com", null, "n")); + insert("u2", null, connection(EMAIL, "JOHN@DOE.COM", null, "n")); + + run(); + + Document summary = report.find(Filters.eq("_id", "summary")).first(); + assertNotNull(summary, "an operator needs one row that states the outcome"); + log.info("summary row: {}", summary.toJson()); + assertEquals(2L, summary.getLong("scanned")); + assertEquals(2L, summary.getLong("rawIdFrozen")); + assertTrue(summary.getLong("conflictGroups") >= 1); + } + + @Test + public void reportIsIdempotentAcrossReruns() { + insert("u1", null, connection(EMAIL, "John@Doe.com", null, "n")); + insert("u2", null, connection(EMAIL, "JOHN@DOE.COM", null, "n")); + + run(); + long afterFirst = report.countDocuments(); + run(); + + assertEquals(afterFirst, report.countDocuments(), + "a deterministic _id must make re-runs replace rows, not accumulate them"); + } + + @Test + public void reportDropsRowsForResolvedConflicts() { + insert("u1", null, connection(EMAIL, "John@Doe.com", null, "n")); + insert("u2", null, connection(EMAIL, "JOHN@DOE.COM", null, "n")); + run(); + assertTrue(reportRows("u2") > 0); + + // The operator resolves it. + users.deleteOne(Filters.eq("_id", "u2")); + run(); + + assertEquals(0, reportRows("u2"), + "a resolved conflict must not haunt the report collection forever"); + assertEquals("john@doe.com", conn("u1", 0).getString("rawId"), + "and the survivor converges on the next run"); + } + + // ------------------------------------------------------------------------- indexes and guards + + @Test + public void ensureLookupIndexesBuildsBothAndIsRerunnable() { + run(); + run(); + + List names = new ArrayList<>(); + users.listIndexes().forEach(index -> names.add(index.getString("name"))); + log.info("indexes: {}", names); + assertTrue(names.contains("email_1"), "findByEmailDeep's email branch must be indexed"); + assertTrue(names.contains("connections.email_1"), + "and its connections.email branch too -- an $or degrades to a collection scan unless " + + "every branch has an index"); + } + + /** + * {@code @ActiveProfiles} replaces the active profiles rather than adding to them, so the eight test + * classes that name their own profile do not have {@code test} active, {@code @Profile("!test")} + * matches, and Mongock really does run this changeset at their context startup — against a database + * their fixtures have not populated yet. + */ + @Test + public void runIsANoOpWhenTheUserCollectionIsAbsent() { + Summary summary = EmailNormalizationBackfill.run(db, "absent_" + UUID.randomUUID(), reportName); + log.info("absent-collection run: {}", summary); + assertEquals(0, summary.scanned()); + assertEquals(0, summary.modified()); + } + + @Test + public void runIsANoOpOnAnEmptyCollection() { + Summary summary = run(); + assertEquals(0, summary.scanned()); + assertEquals(0, summary.modified()); + } +} From 2e9f703d17b9862cd4cd2c550f94de4327168851 Mon Sep 17 00:00:00 2001 From: Ludo Mikula Date: Fri, 2 Oct 2026 20:12:57 +0200 Subject: [PATCH 2/2] new: added tests for email normalization --- .../EmailBackfillEndToEndTest.java | 59 +++++++++++++++++++ .../job/EmailNormalizationBackfillTest.java | 59 +++++++++++++++++++ 2 files changed, 118 insertions(+) diff --git a/server/api-service/lowcoder-server/src/test/java/org/lowcoder/api/authentication/EmailBackfillEndToEndTest.java b/server/api-service/lowcoder-server/src/test/java/org/lowcoder/api/authentication/EmailBackfillEndToEndTest.java index 7478cdfd1c..68ebad965f 100644 --- a/server/api-service/lowcoder-server/src/test/java/org/lowcoder/api/authentication/EmailBackfillEndToEndTest.java +++ b/server/api-service/lowcoder-server/src/test/java/org/lowcoder/api/authentication/EmailBackfillEndToEndTest.java @@ -202,6 +202,65 @@ public void passwordRecoveryIgnoresASoftDeletedAccount() { assertNull(reset, "a deleted account must not be a password-reset target"); } + /** + * An account that belongs to no workspace. It is a reachable state: {@code loginOrRegister} only + * creates a default org when {@code authProperties.getWorkspaceCreation()} is on, so with workspace + * creation disabled every new form registration lands here, as does anyone whose last org was + * deleted or who was removed from it. + * + *

The backfill itself does not care — it never reads org membership. This pins that, and pins what + * {@code lostPassword} does for such an account. + */ + @Test + public void anAccountInNoOrgIsStillConvergedByTheBackfill() { + String stored = "Orgless.Legacy@Example.COM"; + String typed = "orgless.legacy@example.com"; + User seeded = seedLegacy(stored); + + runBackfill(); + + User found = userService.findBySourceAndId(SOURCE, typed).block(); + assertNotNull(found, "org membership is irrelevant to the backfill"); + assertEquals(seeded.getId(), found.getId()); + assertEquals(typed, found.getEmail()); + + // And recovery still resolves it, regardless of case: resetLostPassword does not consult orgs. + User reloaded = userRepository.findById(seeded.getId()).block(); + assertNotNull(reloaded); + String token = "orgless-token"; + reloaded.setPasswordResetToken(HashUtils.hash(token.getBytes())); + reloaded.setPasswordResetTokenExpiry(Instant.now().plus(1, ChronoUnit.HOURS)); + userRepository.save(reloaded).block(); + + assertEquals(Boolean.TRUE, userService.resetLostPassword(typed, token, "new-password").block(), + "resetting must work for an account that belongs to no workspace"); + } + + /** + * ...but requesting the token does not, and fails silently. + * + *

{@code lostPassword} zips the user with their current organization to pick an email template, and + * {@code getCurrentOrgMember} completes empty for an account in no workspace. {@code zipWhen} with an + * empty inner publisher yields empty, so the method stores no token, sends no mail, and reports + * nothing — the caller cannot distinguish it from success. + * + *

Pre-existing and not caused by the email work, but it means password recovery is unusable for + * this class of account regardless of casing, so it is pinned here rather than left as folklore. + */ + @Test + public void requestingAPasswordResetSilentlyDoesNothingForAnAccountInNoOrg() { + String address = "orgless.request@example.com"; + User seeded = seedLegacy(address); + + Boolean requested = userService.lostPassword(address).block(); + log.info("lostPassword for an org-less account returned {}", requested); + + User after = userRepository.findById(seeded.getId()).block(); + assertNotNull(after); + assertNull(after.getPasswordResetToken(), + "no token is stored, and the caller is told nothing -- this is the gap"); + } + /** * The honest counterpart: a genuine conflict is left in place by design, so the lowercase form still * does not reach it. This is what the collision policy costs, pinned so it stays visible. diff --git a/server/api-service/lowcoder-server/src/test/java/org/lowcoder/runner/migrations/job/EmailNormalizationBackfillTest.java b/server/api-service/lowcoder-server/src/test/java/org/lowcoder/runner/migrations/job/EmailNormalizationBackfillTest.java index 635c0c0e66..abc80818e6 100644 --- a/server/api-service/lowcoder-server/src/test/java/org/lowcoder/runner/migrations/job/EmailNormalizationBackfillTest.java +++ b/server/api-service/lowcoder-server/src/test/java/org/lowcoder/runner/migrations/job/EmailNormalizationBackfillTest.java @@ -182,6 +182,65 @@ public void createsNoEmailKeyForADocumentThatHasNone() { assertEquals("john@doe.com", conn("u1", 0).getString("rawId")); } + /** + * A connection carries more than the three fields the migration rewrites — {@code orgIds}, + * {@code authId}, {@code rawUserInfo}, {@code tokens}. Dotted {@code $set} paths are used precisely so + * those survive; a {@code ReplaceOneModel} built from the projected document would silently drop every + * one of them, and an emptied {@code orgIds} would detach the connection from its workspaces. + */ + @Test + public void preservesEveryConnectionFieldItDoesNotRewrite() { + Document connection = connection(EMAIL, "JoHn@DoE.COM", "JoHn@DoE.COM", "John Doe") + .append("authId", "auth-1") + .append("orgIds", List.of("org01", "org02")) + .append("rawUserInfo", new Document("email", "JoHn@DoE.COM").append("sub", "XyZ")) + .append("tokens", List.of("tok-1")); + users.insertOne(new Document("_id", "u1").append("email", "JoHn@DoE.COM") + .append("connections", List.of(connection))); + + run(); + logDoc("after rewrite", "u1"); + + Document after = conn("u1", 0); + assertEquals("john@doe.com", after.getString("rawId"), "precondition: it really was rewritten"); + assertEquals("auth-1", after.getString("authId")); + assertEquals(List.of("org01", "org02"), after.get("orgIds"), + "orgIds must survive -- losing it would detach the connection from its workspaces"); + assertEquals(List.of("tok-1"), after.get("tokens")); + assertEquals("JoHn@DoE.COM", ((Document) after.get("rawUserInfo")).getString("email"), + "rawUserInfo is the verbatim IdP payload and is never rewritten"); + assertEquals("John Doe", after.getString("name")); + } + + /** A connection with no orgIds at all, which is what an account outside every workspace looks like. */ + @Test + public void convergesAnAccountThatBelongsToNoOrg() { + users.insertOne(new Document("_id", "u1").append("email", "Orgless@Example.COM") + .append("connections", List.of( + connection(EMAIL, "Orgless@Example.COM", "Orgless@Example.COM", "n") + .append("orgIds", List.of())))); + + run(); + + assertEquals("orgless@example.com", reload("u1").getString("email")); + assertEquals("orgless@example.com", conn("u1", 0).getString("rawId"), + "org membership is irrelevant to normalization -- nothing in the migration reads it"); + assertEquals(List.of(), conn("u1", 0).get("orgIds")); + } + + /** And a document with no {@code connections} key whatsoever. */ + @Test + public void handlesADocumentWithNoConnectionsAtAll() { + users.insertOne(new Document("_id", "u1").append("email", "Bare@Example.COM")); + + Summary summary = run(); + + assertEquals("bare@example.com", reload("u1").getString("email")); + assertEquals(1, summary.modified()); + assertTrue(!reload("u1").containsKey("connections"), + "the migration must not invent a connections array"); + } + // ----------------------------------------------------------------------------------- freezing @Test