diff --git a/CHANGELOG.md b/CHANGELOG.md index 5848247..0447d4f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,9 @@ tags. A security release driven by a full-project audit. Every entry below closes a finding from that review. +**JDBI hosts pick up one new Flyway migration (V12)** which changes username +uniqueness semantics — read the entry below before upgrading. + ### Security - **Magic-link `SendResult.Sent` no longer carries the login token.** The record @@ -51,6 +54,21 @@ finding from that review. `CeremonyRateLimiter` — including a host's shared Redis implementation — inherits the fix. The SPI documents that the argument arrives folded and MUST be used as given. +- **JDBI username uniqueness is now case-insensitive, matching DynamoDB.** + `DynamoDbUserLookup` keys identity on `lower(username)`, so `Admin` and `admin` + are one account there; `JdbiUserLookup` matched exactly against a plain + `UNIQUE` constraint, so on Postgres they were two. Same library, same SPI, two + identity models — a host assuming the DynamoDB semantics, or migrating between + backends, could end up with look-alike accounts. **Flyway migration V12** + (`PkAuthJdbiSchema.CURRENT_SCHEMA_VERSION` → `12`) adds a unique index on + `lower(username)` and the lookup queries fold case to match. The stored + username keeps its original casing, so display values round-trip unchanged. + + **Upgrade note:** V12 runs a pre-flight check and **refuses to migrate**, naming + the conflicting rows, if the database already contains usernames differing only + by case. Choosing which row is authoritative and what becomes of the other's + credentials is a business decision, not one a schema change should make + silently. Resolve the listed conflicts and re-run. ## [2.2.0] — 2026-06-27 diff --git a/DESIGN.md b/DESIGN.md index b870cf3..ef4c0e7 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -313,7 +313,13 @@ SPIs. `V9__create_refresh_tokens.sql` add the stateful-access-token and refresh-token tables for the 1.1.0 SPIs; `V10__refresh_tokens_amr.sql` adds the `amr` (RFC 8176 authentication-method-reference) column to - `refresh_tokens`. `PkAuthJdbiSchema.CURRENT_SCHEMA_VERSION` is `"10"`. + `refresh_tokens`; `V11__challenges_user_verification.sql` persists the + resolved per-ceremony user-verification requirement; and + `V12__users_username_case_insensitive.sql` makes username uniqueness + case-insensitive (unique index on `lower(username)`) so the JDBI and DynamoDB + backends share one identity model — it refuses to run, naming the offending + rows, if the database already holds usernames differing only by case. + `PkAuthJdbiSchema.CURRENT_SCHEMA_VERSION` is `"12"`. Magic-link tokens are not persisted — the JWT itself is the credential; consumed JTIs live in a `ConsumedJtiStore` (in-memory by default, swap in a shared backend for diff --git a/pk-auth-persistence-jdbi/src/main/java/com/codeheadsystems/pkauth/persistence/jdbi/JdbiUserLookup.java b/pk-auth-persistence-jdbi/src/main/java/com/codeheadsystems/pkauth/persistence/jdbi/JdbiUserLookup.java index 30f95a8..bcfa97b 100644 --- a/pk-auth-persistence-jdbi/src/main/java/com/codeheadsystems/pkauth/persistence/jdbi/JdbiUserLookup.java +++ b/pk-auth-persistence-jdbi/src/main/java/com/codeheadsystems/pkauth/persistence/jdbi/JdbiUserLookup.java @@ -30,7 +30,7 @@ public Optional findHandleByUsername(String username) { () -> jdbi.withHandle( h -> - h.createQuery("SELECT user_handle FROM users WHERE username = :u") + h.createQuery("SELECT user_handle FROM users WHERE lower(username) = lower(:u)") .bind("u", username) .mapTo(byte[].class) .findFirst() @@ -59,12 +59,19 @@ public UserHandle getOrCreateHandle(String username) { () -> jdbi.withHandle( h -> { + // ON CONFLICT infers the V12 expression index on lower(username), so a racing + // insert of any case variant of the same username resolves to the one existing + // row instead of minting a second handle. The no-op DO UPDATE is what makes + // RETURNING yield the winner's handle on the conflict path; the stored username + // keeps its original casing (EXCLUDED.username is only touched to satisfy the + // UPDATE), matching how DynamoDB stores the supplied form and lower-cases only + // the key. byte[] handle = h.createQuery( "INSERT INTO users (user_handle, username, display_name) VALUES" + " (:uh, :u, :dn)" - + " ON CONFLICT (username) DO UPDATE SET username =" - + " EXCLUDED.username" + + " ON CONFLICT (lower(username)) DO UPDATE SET username =" + + " users.username" + " RETURNING user_handle") .bind("uh", candidate.value()) .bind("u", username) diff --git a/pk-auth-persistence-jdbi/src/main/java/com/codeheadsystems/pkauth/persistence/jdbi/PkAuthJdbiSchema.java b/pk-auth-persistence-jdbi/src/main/java/com/codeheadsystems/pkauth/persistence/jdbi/PkAuthJdbiSchema.java index e45e747..cfa7671 100644 --- a/pk-auth-persistence-jdbi/src/main/java/com/codeheadsystems/pkauth/persistence/jdbi/PkAuthJdbiSchema.java +++ b/pk-auth-persistence-jdbi/src/main/java/com/codeheadsystems/pkauth/persistence/jdbi/PkAuthJdbiSchema.java @@ -22,7 +22,7 @@ public final class PkAuthJdbiSchema { * #migrateForDevelopment(DataSource)} pins Flyway's {@code target} to this value so that * unreleased migrations on the classpath are never applied accidentally. */ - public static final String CURRENT_SCHEMA_VERSION = "11"; + public static final String CURRENT_SCHEMA_VERSION = "12"; private PkAuthJdbiSchema() {} diff --git a/pk-auth-persistence-jdbi/src/main/resources/db/migration/V12__users_username_case_insensitive.sql b/pk-auth-persistence-jdbi/src/main/resources/db/migration/V12__users_username_case_insensitive.sql new file mode 100644 index 0000000..b60f466 --- /dev/null +++ b/pk-auth-persistence-jdbi/src/main/resources/db/migration/V12__users_username_case_insensitive.sql @@ -0,0 +1,52 @@ +-- SPDX-License-Identifier: MIT +-- +-- Make username uniqueness case-insensitive, matching DynamoDbUserLookup. +-- +-- Background: DynamoDbUserLookup keys identity on `USERNAME#`, so on that backend +-- "Admin" and "admin" are one account. The JDBI reference implementation matched usernames exactly +-- against a plain `UNIQUE` constraint, so on Postgres they were two. Same library, same SPI, two +-- identity models — a host that assumed the DynamoDB semantics (or migrated between backends) could +-- end up with look-alike accounts. +-- +-- The pre-flight below is the point of this migration being guarded: collapsing the namespace on a +-- database that ALREADY contains case-duplicate usernames is not something a schema change can +-- decide. Which of "Admin" and "admin" is the real account, and what happens to the other one's +-- credentials, is a business call. So we refuse to proceed and name the offending rows rather than +-- failing on an opaque duplicate-key error from the index build, or worse, silently picking one. + +DO $$ +DECLARE + conflicts TEXT; +BEGIN + SELECT string_agg(detail, '; ' ORDER BY detail) + INTO conflicts + FROM ( + SELECT lower(username) || ' -> [' || string_agg(username, ', ' ORDER BY username) || ']' + AS detail + FROM users + GROUP BY lower(username) + HAVING count(*) > 1 + ) AS dupes; + + IF conflicts IS NOT NULL THEN + RAISE EXCEPTION + 'pk-auth V12 cannot make username uniqueness case-insensitive: % existing username(s) ' + 'differ only by case. Conflicting groups: %. ' + 'Resolve these first — decide which row is authoritative, migrate or delete the ' + 'credentials belonging to the others (credentials.user_handle references users.' + 'user_handle), then re-run the migration.', + (SELECT count(*) FROM ( + SELECT 1 FROM users GROUP BY lower(username) HAVING count(*) > 1 + ) AS c), + conflicts; + END IF; +END $$; + +-- Case-insensitive uniqueness. The original `UNIQUE (username)` constraint from V5 is left in +-- place: it is strictly implied by this index (if lower(a) = lower(b) is unique, so is a = b) and +-- dropping it would buy nothing while adding a rewrite step to the upgrade. +-- +-- The stored `username` keeps its original casing so display names round-trip unchanged; only +-- uniqueness and lookup are case-insensitive. This mirrors DynamoDB's UserItem, which stores the +-- username as supplied and lower-cases only the key. +CREATE UNIQUE INDEX users_username_lower_key ON users (lower(username)); diff --git a/pk-auth-persistence-jdbi/src/test/java/com/codeheadsystems/pkauth/persistence/jdbi/JdbiUserLookupIntegrationTest.java b/pk-auth-persistence-jdbi/src/test/java/com/codeheadsystems/pkauth/persistence/jdbi/JdbiUserLookupIntegrationTest.java index b920108..2bed56c 100644 --- a/pk-auth-persistence-jdbi/src/test/java/com/codeheadsystems/pkauth/persistence/jdbi/JdbiUserLookupIntegrationTest.java +++ b/pk-auth-persistence-jdbi/src/test/java/com/codeheadsystems/pkauth/persistence/jdbi/JdbiUserLookupIntegrationTest.java @@ -58,6 +58,37 @@ void findViewByHandleReturnsUsernameForKnownAndEmptyForUnknown() { assertThat(users.findViewByHandle(UserHandle.random())).isEmpty(); } + @Test + void getOrCreateHandleIsCaseInsensitiveAndPreservesOriginalCasing() { + // Matches DynamoDbUserLookup, which keys identity on lower(username). Before V12 the JDBI + // backend matched exactly, so "Alice" and "alice" were two accounts on Postgres and one on + // DynamoDB — the same host code produced different identity models per backend. + UserHandle first = users.getOrCreateHandle("Alice"); + assertThat(users.getOrCreateHandle("alice")).isEqualTo(first); + assertThat(users.getOrCreateHandle("ALICE")).isEqualTo(first); + + // Uniqueness folds, but the stored form keeps the casing it was created with. + assertThat(users.findViewByHandle(first)) + .hasValueSatisfying(v -> assertThat(v.username()).isEqualTo("Alice")); + } + + @Test + void findHandleByUsernameIsCaseInsensitive() { + UserHandle handle = users.getOrCreateHandle("Bob"); + assertThat(users.findHandleByUsername("bob")).hasValue(handle); + assertThat(users.findHandleByUsername("BOB")).hasValue(handle); + assertThat(users.findHandleByUsername("bOb")).hasValue(handle); + } + + @Test + void registerRejectsAUsernameDifferingOnlyByCase() { + users.register("dana", "Dana"); + // The V12 unique index on lower(username) is what enforces this — without it a second row + // would be created and lookups would resolve non-deterministically between the two. + org.assertj.core.api.Assertions.assertThatThrownBy(() -> users.register("DANA", "Impostor")) + .isInstanceOf(com.codeheadsystems.pkauth.spi.PkAuthPersistenceException.class); + } + @Test void registerPersistsUsernameAndDisplayName() { UserHandle handle = users.register("dave", "Dave Display"); diff --git a/pk-auth-persistence-jdbi/src/test/java/com/codeheadsystems/pkauth/persistence/jdbi/V12UsernameCaseMigrationGuardIntegrationTest.java b/pk-auth-persistence-jdbi/src/test/java/com/codeheadsystems/pkauth/persistence/jdbi/V12UsernameCaseMigrationGuardIntegrationTest.java new file mode 100644 index 0000000..78d445f --- /dev/null +++ b/pk-auth-persistence-jdbi/src/test/java/com/codeheadsystems/pkauth/persistence/jdbi/V12UsernameCaseMigrationGuardIntegrationTest.java @@ -0,0 +1,156 @@ +// SPDX-License-Identifier: MIT +package com.codeheadsystems.pkauth.persistence.jdbi; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import com.zaxxer.hikari.HikariConfig; +import com.zaxxer.hikari.HikariDataSource; +import org.flywaydb.core.Flyway; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.condition.DisabledIfEnvironmentVariable; +import org.testcontainers.containers.PostgreSQLContainer; +import org.testcontainers.junit.jupiter.Testcontainers; +import org.testcontainers.utility.DockerImageName; + +/** + * Exercises the pre-flight guard in {@code V12__users_username_case_insensitive.sql} against a real + * Postgres, on a database that is dirty in exactly the way the guard exists to catch. + * + *

This needs its own container: the shared {@link PostgresFixture} is already migrated to head, + * and the interesting states here are "stopped at V11 with case-duplicate rows" and "stopped at V11 + * and clean". Each test migrates to V11, seeds, then runs V12 on its own database. + */ +@Testcontainers +@DisabledIfEnvironmentVariable(named = "PKAUTH_SKIP_TESTCONTAINERS", matches = "1") +class V12UsernameCaseMigrationGuardIntegrationTest { + + @Test + void migrationRefusesAndNamesTheConflictsWhenCaseDuplicatesExist() { + withPostgres( + ds -> { + migrateTo(ds, "11"); + insertUser(ds, "\\x01", "Admin"); + insertUser(ds, "\\x02", "admin"); + insertUser(ds, "\\x03", "ADMIN"); + insertUser(ds, "\\x04", "unaffected"); + + assertThatThrownBy(() -> migrateTo(ds, "12")) + .hasMessageContaining("cannot make username uniqueness case-insensitive") + // Names the offending group so an operator can act without hunting for it. + .hasMessageContaining("Admin") + .hasMessageContaining("admin") + .hasMessageContaining("ADMIN") + // ...and tells them what resolving it involves. + .hasMessageContaining("credentials"); + + // The guard must fail BEFORE the index exists, so a re-run after cleanup can succeed. + assertThat(indexExists(ds)).isFalse(); + }); + } + + @Test + void migrationSucceedsOnCleanDataAndIsThenEnforcedByTheIndex() { + withPostgres( + ds -> { + migrateTo(ds, "11"); + insertUser(ds, "\\x01", "Alice"); + insertUser(ds, "\\x02", "bob"); + + assertThatCode(() -> migrateTo(ds, "12")).doesNotThrowAnyException(); + assertThat(indexExists(ds)).isTrue(); + + // Post-migration, a case variant of an existing username is refused by the index. + assertThatThrownBy(() -> insertUser(ds, "\\x03", "ALICE")) + .hasMessageContaining("users_username_lower_key"); + }); + } + + @Test + void operatorCanResolveTheConflictAndReRunSuccessfully() { + withPostgres( + ds -> { + migrateTo(ds, "11"); + insertUser(ds, "\\x01", "Admin"); + insertUser(ds, "\\x02", "admin"); + + assertThatThrownBy(() -> migrateTo(ds, "12")).hasMessageContaining("differ only by case"); + + // Operator picks the authoritative row and removes the other — the decision the + // migration deliberately refuses to make for them. + execute(ds, "DELETE FROM users WHERE username = 'admin'"); + + assertThatCode(() -> migrateTo(ds, "12")).doesNotThrowAnyException(); + assertThat(indexExists(ds)).isTrue(); + }); + } + + // -- helpers --------------------------------------------------------------------------------- + + private interface DataSourceConsumer { + void accept(HikariDataSource dataSource) throws Exception; + } + + private static void withPostgres(DataSourceConsumer body) { + try (PostgreSQLContainer container = + new PostgreSQLContainer<>(DockerImageName.parse("postgres:16-alpine")) + .withDatabaseName("pkauth_v12") + .withUsername("pkauth") + .withPassword("pkauth-test")) { + container.start(); + HikariConfig cfg = new HikariConfig(); + cfg.setJdbcUrl(container.getJdbcUrl()); + cfg.setUsername(container.getUsername()); + cfg.setPassword(container.getPassword()); + cfg.setMaximumPoolSize(2); + try (HikariDataSource ds = new HikariDataSource(cfg)) { + body.accept(ds); + } + } catch (Exception e) { + throw new IllegalStateException("V12 guard test failed", e); + } + } + + private static void migrateTo(HikariDataSource ds, String target) { + Flyway.configure() + .dataSource(ds) + .locations("classpath:db/migration") + .target(target) + .load() + .migrate(); + } + + private static void insertUser(HikariDataSource ds, String handleHex, String username) { + execute( + ds, + "INSERT INTO users (user_handle, username, display_name) VALUES ('" + + handleHex + + "'::bytea, '" + + username + + "', '" + + username + + "')"); + } + + private static void execute(HikariDataSource ds, String sql) { + try (var connection = ds.getConnection(); + var statement = connection.createStatement()) { + statement.execute(sql); + } catch (Exception e) { + throw new IllegalStateException(e.getMessage(), e); + } + } + + private static boolean indexExists(HikariDataSource ds) { + try (var connection = ds.getConnection(); + var statement = connection.createStatement(); + var rs = + statement.executeQuery( + "SELECT 1 FROM pg_indexes WHERE indexname = 'users_username_lower_key'")) { + return rs.next(); + } catch (Exception e) { + throw new IllegalStateException(e); + } + } +}