diff --git a/CHANGELOG.md b/CHANGELOG.md index 287019a4..dc4581b1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,44 @@ +## [5.0.0] - Unreleased + +> **Major release.** Contains breaking changes (Java API, HTTP/response contracts, database schema, required configuration, and bean/auto-configuration structure). Read `MIGRATION.md` ("Migrating to 5.0.x") before upgrading. +> +> **⚠️ ACTION REQUIRED if you run behind a reverse proxy:** password-reset and email-verification links are now built from a configured canonical URL and ignore `X-Forwarded-Host` unless it is explicitly allow-listed. Set `user.security.appUrl` (recommended) or `user.security.trustedHosts`, or your reset/verification links will point at the wrong host. See `MIGRATION.md`. + +### Features +- Added `AppUrlResolver` and `user.security.appUrl` / `user.security.trustedHosts` properties: password-reset and email-verification links are now built from a configured canonical URL, ignoring `X-Forwarded-Host` unless allow-listed (CWE-640). + +### Fixes +- Registration (form and passwordless) and resend-verification endpoints no longer reveal whether an email is registered or already verified (account-enumeration hardening). +- Credential-altering operations (remove password, delete/rename passkey) now require the current password when the account has one, preventing a session-only actor from changing authentication methods. These current-password checks now also participate in brute-force lockout (`LoginAttemptService`): a locked account is rejected, each wrong password counts toward the lockout threshold, and a correct one resets the counter — closing an unlimited-guess gap on the WebAuthn credential-management endpoints. +- `AppUrlResolver` now parses a multi-valued `X-Forwarded-Host` (comma-separated, RFC 7230) by matching its first value against `user.security.trustedHosts`, so legitimate multi-proxy chains are honored instead of silently falling back. `X-Forwarded-Proto` is validated to `http`/`https` (an invalid value from a misconfigured proxy can no longer flow into email links), and attacker-controlled header values are sanitized before logging. +- Registration denials (`RegistrationGuard`) now emit an audit event on both the form and passwordless paths, matching the audit trail already produced for duplicate-email registrations. +- Role/privilege startup setup is now idempotent and safe under concurrent multi-node startup (handles unique-constraint races by re-reading the existing row). +- The library's validation @ControllerAdvice now returns HTTP 400 (not 500) for the class-level @PasswordMatches constraint, surfacing global validation errors. +- The library entry point is now a proper `@AutoConfiguration`. The cross-cutting `@EnableAsync`/`@EnableRetry`/`@EnableScheduling`/`@EnableMethodSecurity` are each now gated behind `user.async.enabled`/`user.retry.enabled`/`user.scheduling.enabled`/`user.method-security.enabled` (all default true), so consumers who manage these themselves can opt out to avoid double-activation. + +### Performance +- `User` → `roles` and `Role` → `privileges` are now LAZY-fetched; the authentication path loads them via `@EntityGraph` in a single query (`UserRepository.findWithRolesByEmail`), removing the previous two-level eager fetch and the associated N+1 problem while keeping authority resolution correct. + +### Breaking Changes +- Role and privilege collections on `User`/`Role` are now LAZY. Consumer code that accesses `user.getRoles()` or `role.getPrivileges()` outside an open transaction/session may now throw `LazyInitializationException`. Load via the authentication path / an `@EntityGraph` query (e.g. `UserRepository.findWithRolesByEmail`), or access the collections within a transaction. See MIGRATION.md. +- Reset/verification email links no longer trust `X-Forwarded-Host` by default. Deployments behind a reverse proxy must set `user.security.appUrl` or `user.security.trustedHosts` (see MIGRATION.md). `UserUtils.getAppUrl(HttpServletRequest)` is deprecated for removal. +- Added a UNIQUE, NOT NULL constraint on the `token` column of `password_reset_token` and `verification_token`. This is a schema/DDL change — see MIGRATION.md. +- Added UNIQUE, NOT NULL constraints on `role.name` and `privilege.name` (schema/DDL change). See MIGRATION.md. +- The registration (`/user/registration`, `/user/registration/passwordless`) and resend-verification endpoints now always return HTTP 200 with a uniform generic body. Previously an existing email returned 409 on both registration paths, and resend returned 409 (already verified) or 500 (unknown email). Clients that branched on those status codes or messages must update — the response is now intentionally uniform. +- `removePassword` and passkey delete/rename now require a `currentPassword` for password-holding accounts; requests omitting it are rejected. Update clients to send the current password. See MIGRATION.md. +- JPA entities (`User`, `PasswordResetToken`, `VerificationToken`, `PasswordHistoryEntry`, `WebAuthnCredential`, `WebAuthnUserEntity`) now use identity-based (id-only) `equals`/`hashCode` instead of Lombok `@Data`'s all-fields equality. Code relying on field-by-field entity equality, or using transient (unsaved, id=null) entities as `Set`/`Map` keys, may behave differently. `toString` no longer includes collections or secrets. See MIGRATION.md. +- Application events (`OnRegistrationCompleteEvent`, `UserPreDeleteEvent`, `ConsentChangedEvent`) no longer carry live JPA `User` entities; they now expose immutable ids/scalars (e.g. `userId`, `userEmail`). Custom listeners calling `event.getUser()` must switch to the new accessors. This prevents detached-entity/`LazyInitializationException` hazards in `@Async` listeners. See MIGRATION.md. +- GlobalValidationExceptionHandler is now scoped to the library's own controllers (assignableTypes) instead of applying application-wide. Consuming apps that relied on the library formatting validation errors for THEIR controllers must provide their own @ControllerAdvice. +- Consolidated the duplicate `com.digitalsanctuary.spring.user.exception` package into `com.digitalsanctuary.spring.user.exceptions` (GlobalValidationExceptionHandler moved). Removed the empty, unused `com.digitalsanctuary.spring.user.api.data` package. Consumers importing `...exception.GlobalValidationExceptionHandler` must update the import to `...exceptions.GlobalValidationExceptionHandler`. +- The library no longer sets `spring.messages.basename` (which previously overrode the consuming app's message bundle); it now registers its own `messages/dsspringusermessages` bundle additively via an EnvironmentPostProcessor. Library bean names are now namespaced (dsUserService, dsUserAPI, dsMailService, etc.); code referencing these beans by their old default names (e.g. @Qualifier("userService")) must update. +- `UserConfiguration` is now an `@AutoConfiguration` instead of a regular `@Configuration`. It loads in the auto-configuration phase (after consumer beans) and remains registered in `META-INF/spring/org.springframework.boot.autoconfigure.AutoConfiguration.imports`. This is transparent for normal usage, but any consumer that imported `UserConfiguration` directly (e.g. via `@Import(UserConfiguration.class)`) or relied on it being picked up by their own application's `@ComponentScan` should remove that — the library configures itself via auto-configuration. The cross-cutting enablers (`@EnableAsync`/`@EnableRetry`/`@EnableScheduling`/`@EnableMethodSecurity`) now live in nested gated configurations toggleable via `user.async.enabled`/`user.retry.enabled`/`user.scheduling.enabled`/`user.method-security.enabled` (all default true, so behavior is unchanged unless explicitly disabled). + +### Dependencies +- Built and tested against Spring Boot 4.1.0 (up from 4.0.6). Starters remain `compileOnly`, so consuming applications still supply their own Spring Boot version; the library is now verified against the 4.1.x line. (dependabot #317) + +### Notes +- Audit-log injection (originally slated here as a JSON-per-line format change) was already resolved in 4.4.0 via field sanitization (CR/LF and `|` stripped). The breaking JSON-per-line conversion was intentionally **not** carried into 5.0.0, as it offered no additional security benefit. + ## [4.4.0] - 2026-06-15 ### Features - Token security hardened: verification and password-reset tokens are now hashed at rest (HMAC-SHA-256 when user.security.tokenHashSecret is set; plain SHA-256 otherwise). Dual-read ensures pre-upgrade plaintext tokens still work until expiry. Only one active token per user is kept; generators delete any previous token before issuing a new one. New properties: diff --git a/MIGRATION.md b/MIGRATION.md index 2d39dec0..1d31897d 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -6,6 +6,19 @@ This guide covers migrating applications using the Spring User Framework between - [Migration Guide](#migration-guide) - [Table of Contents](#table-of-contents) + - [Migrating to 5.0.x](#migrating-to-50x) + - [⚠️ ACTION REQUIRED: Reverse-proxy deployments must configure a canonical app URL](#-action-required-reverse-proxy-deployments-must-configure-a-canonical-app-url) + - [Database schema: unique token constraint](#database-schema-unique-token-constraint) + - [Registration & resend responses are now uniform (anti-enumeration)](#registration--resend-responses-are-now-uniform-anti-enumeration) + - [Re-authentication required for credential changes](#re-authentication-required-for-credential-changes) + - [Database schema: unique role/privilege names](#database-schema-unique-roleprivilege-names) + - [Lazy fetching of roles and privileges](#lazy-fetching-of-roles-and-privileges) + - [Entity equals/hashCode now identity-based](#entity-equalshashcode-now-identity-based) + - [Events carry ids/DTOs instead of entities](#events-carry-idsdtos-instead-of-entities) + - [Validation exception handler is now library-scoped](#validation-exception-handler-is-now-library-scoped) + - [Package consolidation](#package-consolidation) + - [Message bundle no longer overridden; library beans renamed](#message-bundle-no-longer-overridden-library-beans-renamed) + - [Auto-configuration entry point and toggleable cross-cutting features](#auto-configuration-entry-point-and-toggleable-cross-cutting-features) - [Migrating to 4.0.x (Spring Boot 4.0)](#migrating-to-40x-spring-boot-40) - [Prerequisites](#prerequisites) - [Step 1: Update Java Version](#step-1-update-java-version) @@ -29,6 +42,297 @@ This guide covers migrating applications using the Spring User Framework between - [Common Issues](#common-issues) - [Version Compatibility Matrix](#version-compatibility-matrix) +## Migrating to 5.0.x + +This section covers migrating from Spring User Framework 4.4.x to 5.0.x. Version 5.0.0 is a **major release** containing breaking changes. Spring Boot compatibility is unchanged from 4.4.x (Spring Boot 4.0 on Java 21+, and Spring Boot 3.5 on Java 17+); the major-version bump reflects this library's own API/contract changes, not a Spring Boot major change. + +> **Note on versioning:** This library follows Semantic Versioning for its own API. Its major version is deliberately **not** tied to Spring Boot's major version — a single release line supports more than one Spring Boot major. See the Version Compatibility Matrix for supported Spring Boot versions. + +### ⚠️ ACTION REQUIRED: Reverse-proxy deployments must configure a canonical app URL + +Password-reset and email-verification links are now built from a configured canonical base URL instead of the inbound request's `Host` / `X-Forwarded-Host` header (CWE-640, host-header / reset poisoning). By default `X-Forwarded-Host` is **ignored** unless the host is explicitly allow-listed. + +**If your application runs behind a reverse proxy or load balancer, you must take action or your reset/verification links will break:** + +- **Recommended:** set the canonical base URL: + ```properties + user.security.appUrl=https://app.example.com + ``` + When set, `X-Forwarded-Host` is ignored entirely and all email links use this URL. +- **Alternative:** allow-list the trusted forwarded host(s): + ```properties + user.security.trustedHosts=app.example.com,www.example.com + ``` + `X-Forwarded-Host` is then honored only for hosts in this list; all others fall back to the container's own server name. + +Local development with no proxy needs no change. `UserUtils.getAppUrl(HttpServletRequest)` is deprecated in favor of `AppUrlResolver`. + +### Database schema: unique token constraint + +The `token` column on both `password_reset_token` and `verification_token` now carries a **UNIQUE + NOT NULL** constraint. The stored value is always a fixed-length hash (introduced in 4.4.0), so the column length is predictable and the index is safe. + +**Applications using `spring.jpa.hibernate.ddl-auto=update` (or `create`/`create-drop`):** Hibernate will add the unique index automatically on startup — no manual action required. + +**Applications managing schema manually (Flyway, Liquibase, or `ddl-auto=validate`/`none`):** apply the following DDL before upgrading and before starting the application: + +```sql +-- Ensure no existing null or duplicate token values exist first. +-- (4.4.0 already enforces one active token per user, so duplicates are unlikely.) +ALTER TABLE password_reset_token ALTER COLUMN token SET NOT NULL; +ALTER TABLE verification_token ALTER COLUMN token SET NOT NULL; +CREATE UNIQUE INDEX ux_password_reset_token_token ON password_reset_token (token); +CREATE UNIQUE INDEX ux_verification_token_token ON verification_token (token); +``` + +> **Note:** The DDL syntax above is standard SQL (compatible with PostgreSQL and MariaDB/MySQL). For MySQL/MariaDB, `ALTER COLUMN token SET NOT NULL` may need to include the full column definition, e.g. `MODIFY COLUMN token VARCHAR(255) NOT NULL`. + +If your database contains rows with a `null` token value (possible only if tokens were created before 4.4.0 without the hash path), delete or back-fill those rows before applying the NOT NULL constraint. + +### Registration & resend responses are now uniform (anti-enumeration) + +The `/user/registration`, `/user/registration/passwordless`, and `/user/resendRegistrationToken` endpoints now return the **same generic, success-shaped HTTP 200 response** regardless of whether the email is already registered or already verified. This prevents attackers from using these endpoints to enumerate which email addresses have accounts and which are verified (CWE-204). + +**Old behavior:** + +| Endpoint | Case | Old status | Old body message | +|---|---|---|---| +| `/user/registration` | New email | 200 | `Registration Successful!` | +| `/user/registration` | Email already exists | 409 Conflict | `An account already exists for the email address` (code 2) | +| `/user/registration/passwordless` | New email | 200 | `Registration Successful!` | +| `/user/registration/passwordless` | Email already exists | 409 Conflict | `An account already exists for the email address` (code 2) | +| `/user/resendRegistrationToken` | Unverified account | 200 | `Verification Email Resent Successfully!` | +| `/user/resendRegistrationToken` | Already-verified account | 409 Conflict | `Account is already verified.` (code 1) | +| `/user/resendRegistrationToken` | Unknown email | 500 Internal Server Error | `System Error!` (code 2) | + +**New behavior:** + +| Endpoint | All cases | New status | New body message | +|---|---|---|---| +| `/user/registration` | New email **or** already exists | 200 | `If your email address is eligible, you will receive a verification email shortly.` (success, code 0) | +| `/user/registration/passwordless` | New email **or** already exists | 200 | `Registration Successful!` (success, code 0) | +| `/user/resendRegistrationToken` | Unverified, already-verified, **or** unknown email | 200 | `If your account requires verification, a new verification email has been sent.` (success, code 0) | + +Internally the framework still does the correct thing — a brand-new registration creates the account and sends verification, an existing email creates nothing, and resend sends an email only when the account exists and is unverified — and the true outcome is still recorded in the audit log. Only the externally observable response is now uniform. + +**Action required:** Clients (web UIs, mobile apps, integrations) must no longer rely on the `409` status (existing/verified account) or the `500` status (unknown email on resend) to detect account existence or verification state. Branch only on the `success` flag for these three endpoints, and present the generic message to end users. + +### Re-authentication required for credential changes + +Operations that change *how an account authenticates* now require the user to prove knowledge of the current password **when the account has one**. This closes a gap where a session-only actor (e.g. an unattended or hijacked session) could silently alter the account's authentication methods without re-authenticating (CWE-620 / weak re-authentication). + +Affected endpoints (all require `user.webauthn.enabled=true` except where noted): + +| Endpoint | Method | What changed | How to send the current password | +|---|---|---|---| +| `/user/webauthn/password` | `DELETE` | Removing the password (converting to passkey-only) now requires the current password. | JSON body `{"currentPassword": "..."}` | +| `/user/webauthn/credentials/{id}` | `DELETE` | Deleting a passkey requires the current password **when the account has a password**. | JSON body `{"currentPassword": "..."}` | +| `/user/webauthn/credentials/{id}/label` | `PUT` | Renaming a passkey requires the current password **when the account has a password**. The existing body gains a `currentPassword` field. | JSON body `{"label": "...", "currentPassword": "..."}` | + +**Behavior when the account has a password:** +- Missing `currentPassword` → `HTTP 400` with message *"Current password is required to change authentication methods."* — nothing is mutated. +- Incorrect `currentPassword` → `HTTP 400` with message *"Current password is incorrect."* — nothing is mutated. +- Correct `currentPassword` → the operation proceeds as before. + +`/user/updatePassword` is unchanged: it already required and verified `oldPassword`. + +**Action required:** Update any client that calls the three endpoints above so that it collects the user's current password and sends it in the request body. `DELETE /user/webauthn/credentials/{id}` and `DELETE /user/webauthn/password`, which previously had no request body, now accept (and for password-holding accounts require) a JSON body carrying `currentPassword`. Existing IDOR/ownership checks and last-credential lockout protection are unchanged. + +**Passwordless (passkey-only) accounts — residual risk:** For accounts with no password set, there is no current credential to verify, and this library does not yet implement a WebAuthn step-up assertion (a feasible recent-authentication signal does not currently exist in the framework). As a result: +- Deleting or renaming a passkey on a passwordless account remains a session-only operation (last-credential lockout protection and ownership checks still apply). +- Setting an *initial* password via `POST /user/setPassword` on a passwordless account also cannot require a current password (there is none); this endpoint still rejects accounts that already have a password. + +This is a deliberate, documented limitation rather than a half-measure: implementing a true WebAuthn step-up assertion would require significant new challenge/response infrastructure. Consuming applications that need stronger guarantees for passwordless accounts can front these endpoints with their own step-up (e.g. require a fresh passkey assertion) before allowing the call. This will be revisited if/when a step-up mechanism is added to the framework. + +### Database schema: unique role/privilege names + +The `name` column on both the `role` and `privilege` tables now carries a **UNIQUE + NOT NULL** constraint. Role and privilege names were always intended to be unique identifiers (the framework looks them up by name), so this enforces an existing invariant at the schema level. It also makes the startup role/privilege setup safe under concurrent multi-node startup: if two nodes start simultaneously and both try to create the same role/privilege, the unique constraint guarantees only one row is created, and the framework re-reads the winning row instead of failing (first-writer-wins). + +**Applications using `spring.jpa.hibernate.ddl-auto=update` (or `create`/`create-drop`):** Hibernate will add the unique index automatically on startup — no manual action required. + +**Applications managing schema manually (Flyway, Liquibase, or `ddl-auto=validate`/`none`):** **de-duplicate any existing duplicate role/privilege names first**, then apply the following DDL before upgrading and before starting the application: + +```sql +-- 1. Find duplicates before applying the constraint (must return zero rows): +SELECT name, COUNT(*) FROM role GROUP BY name HAVING COUNT(*) > 1; +SELECT name, COUNT(*) FROM privilege GROUP BY name HAVING COUNT(*) > 1; +-- Resolve any duplicates manually (merge/repoint references, delete extras) before continuing. + +-- 2. Apply NOT NULL + UNIQUE: +ALTER TABLE role ALTER COLUMN name SET NOT NULL; +ALTER TABLE privilege ALTER COLUMN name SET NOT NULL; +CREATE UNIQUE INDEX ux_role_name ON role (name); +CREATE UNIQUE INDEX ux_privilege_name ON privilege (name); +``` + +> **Note:** The table names above (`role`, `privilege`) and column name (`name`) are Hibernate's defaults for the `Role` and `Privilege` entities (no `@Table` override). If you have customized Hibernate's physical naming strategy, adjust the identifiers accordingly. The DDL syntax is standard SQL (PostgreSQL / MariaDB / MySQL); for MySQL/MariaDB, `ALTER COLUMN name SET NOT NULL` may need the full column definition, e.g. `MODIFY COLUMN name VARCHAR(255) NOT NULL`. + +### Lazy fetching of roles and privileges + +**What changed:** The `roles` collection on `User` and the `privileges` collection on `Role` were previously `FetchType.EAGER`. They are now `FetchType.LAZY`. The authentication path (`DSUserDetailsService.loadUserByUsername`) now loads the full `User` → `roles` → `privileges` graph via a new `@EntityGraph` repository finder, `UserRepository.findWithRolesByEmail(String email)`, which fetches everything in a **single query**. + +**Why:** The old two-level eager fetch loaded every role and every privilege on *every* `User` load — even for operations that never touch authorities (token lookups, lockout-counter updates, existence checks) — and caused an N+1 query pattern. Making the collections lazy and loading them explicitly only where they are needed removes that overhead while keeping authentication behavior identical. + +**Impact / risk:** Because the collections are now lazy, **any code that accesses `user.getRoles()` (or iterates `role.getPrivileges()`) on a detached entity — i.e. outside an open Hibernate session/transaction — will throw `LazyInitializationException`.** Code that accesses these collections *within* an active transaction (the common case for service methods) is unaffected. The framework's own authentication, OAuth2/OIDC, and GDPR-export paths have been updated to initialize the graph correctly. + +**Remediation patterns for consumers** that traverse roles/privileges on a `User` they obtained outside a transaction: + +- **Load through the authentication path or the entity-graph finder.** Use `UserRepository.findWithRolesByEmail(email)` (it initializes roles and privileges in one query) instead of the plain `findByEmail(email)` when you need authorities. +- **Access the collections inside a transaction.** Annotate the method that reads `user.getRoles()` with `@Transactional` so the persistence session is still open when the lazy collection is first touched. +- **Use a DTO projection.** Map the roles/privileges you need into a DTO while still inside the session, then pass the DTO around the detached boundary. +- **Initialize before detaching.** If you must hand a `User` to detached code, call `Hibernate.initialize(user.getRolesAsSet())` (and the nested privileges) while the session is open. + +The plain `UserRepository.findByEmail(String)` finder is retained unchanged for callers that do not need the authority graph (token lookups, existence checks, lockout counters); it intentionally leaves `roles`/`privileges` uninitialized. + +### Entity equals/hashCode now identity-based + +**What changed:** The JPA entities `User`, `PasswordResetToken`, `VerificationToken`, `PasswordHistoryEntry`, `WebAuthnCredential`, and `WebAuthnUserEntity` previously used Lombok `@Data`, which generates `equals`/`hashCode` over **all** fields. They now use an identity-based implementation that includes only the primary key (`@EqualsAndHashCode(onlyExplicitlyIncluded = true)` with `@EqualsAndHashCode.Include` on the id), matching the pattern already used by `Role` and `Privilege`. Their `toString` also no longer renders collections, associations, passwords, or token/credential secrets. + +Note: `WebAuthnCredential` and `WebAuthnUserEntity` use their **assigned natural String keys** (`credentialId` and `id` respectively, both Base64url-encoded values assigned before persistence) rather than a database-generated surrogate id. Equality is therefore defined by that String key. + +**Why:** All-fields `equals`/`hashCode` on JPA entities is unsafe: it changes as mutable fields change (breaking `Set`/`Map` membership), can force lazy collections to load, and a generated `toString` can trigger `LazyInitializationException` or leak secrets (password hashes, raw/hashed token values, key material) into logs. Identity-based equality is the standard JPA recommendation. + +**Impact / risk:** + +- **Two entities are now equal only when they share a non-null id.** If you compared entities field-by-field (relying on `@Data`'s all-fields equality), that behavior is gone — equality is now purely by primary key. +- **Standard JPA caveat for transient (unsaved) entities:** two newly-constructed entities that have not yet been persisted both have `id == null` and are therefore **not** considered equal to each other (and an unsaved entity is not equal to its persisted counterpart until the id is assigned). Do not use transient, id-less entities as `Set`/`Map` keys and then expect lookups to match after persistence assigns the id. If you need value-equality for unsaved instances, compare the relevant fields explicitly rather than relying on `equals`. +- **`toString` output changed:** collections, associations, and secret fields are excluded. Any code (or log assertion) that depended on those values appearing in `toString()` must be updated. + +No remediation is required for the common cases (comparing managed/persisted entities by identity, or using them in collections after they have ids). This change only affects code that depended on field-by-field entity equality or on the old `toString` format. + +### Events carry ids/DTOs instead of entities + +**What changed:** Three application events no longer carry a live JPA `User` entity. They now expose immutable scalar data (ids/emails) captured at publish time, while the entity is still attached to a persistence context: + +| Event | Old accessor(s) | New accessor(s) | +|---|---|---| +| `OnRegistrationCompleteEvent` | `getUser()` (live `User`) | `getUserId()`, `getUserEmail()`, `isUserEnabled()` — plus the unchanged `getLocale()` / `getAppUrl()` | +| `UserPreDeleteEvent` | `getUser()` (live `User`); `getUserId()` | `getUserId()` (unchanged), `getUserEmail()` (new); `getUser()` removed | +| `ConsentChangedEvent` | `getUser()` (live `User`); `getUserId()` | `getUserId()` (unchanged), `getUserEmail()` (new); `getUser()` removed. The `ConsentRecord` (a plain DTO, not a JPA entity) and `ChangeType` are unchanged | + +The constructors changed accordingly: + +- `OnRegistrationCompleteEvent`: now built from `userId`, `userEmail`, `userEnabled`, `locale`, `appUrl` (the Lombok `@Builder` is retained; the `.user(...)` builder method is gone, replaced by `.userId(...)`, `.userEmail(...)`, `.userEnabled(...)`). +- `UserPreDeleteEvent(Object source, Long userId, String userEmail)` replaces `UserPreDeleteEvent(Object source, User user)`. +- `ConsentChangedEvent(Object source, Long userId, String userEmail, ConsentRecord record, ChangeType changeType)` replaces `ConsentChangedEvent(Object source, User user, ConsentRecord record, ChangeType changeType)`. + +**Why:** These events are (or can be) consumed by `@Async` listeners that run on a different thread from the publisher. Handing a live JPA entity across that boundary is unsafe: the persistence session that loaded it is typically closed by the time the listener runs, so touching a lazy association (or even a basic field on a proxy) throws `LazyInitializationException`, and the entity may be detached or concurrently mutated. The framework's own `UserDeletedEvent`/`UserDisabledEvent` already followed the id-only pattern; this change brings the remaining events in line. + +**Remediation for consumer listeners:** + +- If your listener only needed the id or email, switch from `event.getUser().getId()` / `event.getUser().getEmail()` to `event.getUserId()` / `event.getUserEmail()`. +- If your listener needs the full `User`, **load it by id from `UserRepository` inside your listener's own transaction** (e.g. annotate the listener method `@Transactional`, or call a `@Transactional` service method). Do not retain or pass around the entity beyond that transaction. +- For `OnRegistrationCompleteEvent` specifically, the framework's built-in `RegistrationListener` now passes `event.getUserId()` to `UserEmailService.sendRegistrationVerificationEmail(Long userId, String appUrl)`, which reloads the `User` in its own transaction before creating the verification token and rendering the email. The verification-email content (recipient, token, confirmation URL, template) is unchanged. + +### Validation exception handler is now library-scoped + +**What changed:** `GlobalValidationExceptionHandler` (the `@ControllerAdvice` that formats validation errors into a structured JSON 400 body) is now **scoped to the library's own controllers** via `assignableTypes`: + +```java +@ControllerAdvice(assignableTypes = {UserAPI.class, GdprAPI.class, MfaAPI.class, UserActionController.class, UserPageController.class}) +``` + +Previously it carried a bare `@ControllerAdvice`, which made it apply **application-wide** — including the consuming application's own controllers. + +**Why:** A bare `@ControllerAdvice` from a library is global and silently hijacks the consumer's validation handling: any `MethodArgumentNotValidException` (or, in some cases, `ConstraintViolationException`) thrown by *your* controllers was being caught and reformatted into the library's response shape, overriding whatever error contract your application intended. Scoping the advice to the library's own controllers keeps the library out of your controllers' exception handling. + +`WebAuthnManagementAPI` is intentionally **not** in this list: it has its own dedicated advice (`WebAuthnManagementAPIAdvice`) that already handles validation. Adding it here would create two advices targeting the same controller with overlapping handlers. + +**Also fixed (400 for `@PasswordMatches`):** The handler now collects **global (class-level) binding errors** in addition to field errors. The class-level `@PasswordMatches` constraint on `UserDto` produces a *global* error (not a field error); the previous handler blindly cast every error to `FieldError`, throwing a `ClassCastException` inside the `@ExceptionHandler` and returning an unhelpful HTTP 500. A mismatched password/confirmation on registration now returns a structured **HTTP 400**. A dedicated `@ExceptionHandler(ConstraintViolationException.class)` was also added (for constraints triggered outside method-argument binding, e.g. `@Validated` on method parameters), also returning a structured 400. + +**Remediation:** If your application relied on this library formatting validation errors for **your own** controllers, that no longer happens. Provide your own `@ControllerAdvice` (or `@RestControllerAdvice`) to format `MethodArgumentNotValidException` / `ConstraintViolationException` for your controllers. The library's response shape (for reference) is `{ "success": false, "code": 400, "message": "Validation failed", "errors": { : } }`. + +### Package consolidation + +The duplicate `com.digitalsanctuary.spring.user.exception` package (singular) has been merged into the canonical `com.digitalsanctuary.spring.user.exceptions` package (plural). The only class that moved is `GlobalValidationExceptionHandler`. + +**Impact:** Only affects code that imports `GlobalValidationExceptionHandler` by its fully-qualified name or via an explicit import statement. + +**Remediation:** Update any import referencing the old package: + +```java +// Before (5.0.x prior to this change) +import com.digitalsanctuary.spring.user.exception.GlobalValidationExceptionHandler; + +// After +import com.digitalsanctuary.spring.user.exceptions.GlobalValidationExceptionHandler; +``` + +The empty, unused `com.digitalsanctuary.spring.user.api.data` package has also been removed. This package contained only a placeholder `Response.java` with no content and was not referenced anywhere. No remediation required. + +### Message bundle no longer overridden; library beans renamed + +Two related changes make the library a better citizen inside a consuming application: + +**1. The library no longer overrides your `spring.messages.basename`.** + +Previously the library shipped a hardcoded `spring.messages.basename=messages/messages,messages/dsspringusermessages` default property. Because this was a library default, it OVERRODE the consuming application's own `spring.messages.basename`, clobbering any custom message bundle configuration. + +The library now registers its own bundle (`messages/dsspringusermessages`) **additively** via a Spring Boot `EnvironmentPostProcessor` (`MessageSourceEnvironmentPostProcessor`). It reads your existing `spring.messages.basename` (or Spring Boot's conventional default of `messages` if you have not set one) and appends the library bundle to the end of the list, de-duplicated. The library bundle is placed last so YOUR message keys win on collisions. + +**Impact:** None for most consumers — your message bundle is now preserved automatically. + +**Remediation:** If you had previously worked around the old behavior by manually merging the library basename into your own `spring.messages.basename` (e.g. setting `spring.messages.basename=messages,messages/dsspringusermessages` yourself), you can simplify back to just your own value (e.g. `spring.messages.basename=messages`); the library appends its bundle for you. Leaving the explicit merge in place is harmless — it is de-duplicated. + +**2. Library bean names are now namespaced with a `ds` prefix.** + +High-collision library beans now have explicit, namespaced bean names so they no longer conflict with a consumer bean of the same default name: + +| Class | Old default bean name | New bean name | +|---|---|---| +| `UserService` | `userService` | `dsUserService` | +| `MailService` | `mailService` | `dsMailService` | +| `UserEmailService` | `userEmailService` | `dsUserEmailService` | +| `DSUserDetailsService` | `dSUserDetailsService` | `dsUserDetailsService` | +| `LoginAttemptService` | `loginAttemptService` | `dsLoginAttemptService` | +| `SessionInvalidationService` | `sessionInvalidationService` | `dsSessionInvalidationService` | +| `PasswordPolicyService` | `passwordPolicyService` | `dsPasswordPolicyService` | +| `AuthorityService` | `authorityService` | `dsAuthorityService` | +| `RolePrivilegeSetupService` | `rolePrivilegeSetupService` | `dsRolePrivilegeSetupService` | +| `MailContentBuilder` | `mailContentBuilder` | `dsMailContentBuilder` | +| `UserAPI` | `userAPI` | `dsUserAPI` | +| `GdprAPI` | `gdprAPI` | `dsGdprAPI` | +| `MfaAPI` | `mfaAPI` | `dsMfaAPI` | +| `WebAuthnManagementAPI` | `webAuthnManagementAPI` | `dsWebAuthnManagementAPI` | +| `UserActionController` | `userActionController` | `dsUserActionController` | +| `UserPageController` | `userPageController` | `dsUserPageController` | + +**Impact:** Only affects code that references these beans **by name** rather than by type. By-type injection (the common case) is unaffected. + +**Remediation:** Update any by-name reference — `@Qualifier("userService")`, `@Resource(name = "userService")`, `@DependsOn("userService")`, or `applicationContext.getBean("userService", ...)` — to the new `ds`-prefixed name (e.g. `@Qualifier("dsUserService")`). Injection by type (e.g. `@Autowired UserService userService;`) requires no change. + +### Auto-configuration entry point and toggleable cross-cutting features + +The library's entry point, `UserConfiguration`, is now a proper Spring Boot `@AutoConfiguration` instead of a plain `@Configuration`. It is still registered in `META-INF/spring/org.springframework.boot.autoconfigure.AutoConfiguration.imports`, so it continues to load automatically — but it now runs in the auto-configuration phase (after your application's own beans), which is the correct lifecycle for a library entry point. + +**Impact:** None for normal usage. The library still discovers and registers all of its components, and all existing behavior is preserved. + +**Remediation:** Only required in the uncommon case that you imported the entry point yourself. If you have `@Import(UserConfiguration.class)` anywhere, or you deliberately arranged for your application's own `@ComponentScan` to pick up `com.digitalsanctuary.spring.user`, remove that — the library configures itself via auto-configuration and doing it twice is unnecessary. + +**New opt-out toggles for cross-cutting features.** The library enables four cross-cutting Spring features. Each is now gated behind its own property, all defaulting to `true`, so **no action is needed** to keep current behavior: + +| Property (default `true`) | Enables | +|---|---| +| `user.async.enabled` | `@EnableAsync` | +| `user.retry.enabled` | `@EnableRetry` | +| `user.scheduling.enabled` | `@EnableScheduling` | +| `user.method-security.enabled` | `@EnableMethodSecurity` | + +**Use case:** If your application already enables one of these globally (for example you have your own `@EnableScheduling` or a global `@EnableMethodSecurity` with custom settings), you can disable the library's copy to avoid double-activation conflicts: + +```yaml +user: + scheduling: + enabled: false # you run your own @EnableScheduling + method-security: + enabled: false # you run your own @EnableMethodSecurity +``` + +Leave them unset (or `true`) to keep the library managing these for you, exactly as before. + +> **Note on async/retry interaction:** These toggles are independent. If you disable `user.async.enabled=false` while retry remains enabled (the default), any `@Retryable` methods in the library will still be registered for retry — but if those methods were previously executing on an async thread pool, they will now run synchronously on the caller's thread. In practice the library's `@Retryable` methods are not themselves `@Async`, so the common case is unaffected; however, consumers that wrap library calls in their own async boundaries should verify the combined behavior when selectively disabling these features. + + + ## Migrating to 4.0.x (Spring Boot 4.0) This section covers migrating from Spring User Framework 3.x (Spring Boot 3.x) to 4.x (Spring Boot 4.0). diff --git a/build.gradle b/build.gradle index 03d16d02..e5746881 100644 --- a/build.gradle +++ b/build.gradle @@ -1,5 +1,5 @@ plugins { - id 'org.springframework.boot' version '4.0.6' + id 'org.springframework.boot' version '4.1.0' id 'io.spring.dependency-management' version '1.1.7' id 'com.github.ben-manes.versions' version '0.54.0' id 'java-library' diff --git a/db-scripts/mariadb-schema.sql b/db-scripts/mariadb-schema.sql index 5b8f4be2..21717190 100644 --- a/db-scripts/mariadb-schema.sql +++ b/db-scripts/mariadb-schema.sql @@ -21,9 +21,10 @@ DROP TABLE IF EXISTS `password_reset_token`; CREATE TABLE `password_reset_token` ( `id` BIGINT(20) NOT NULL, `expiry_date` DATETIME(6) DEFAULT NULL, - `token` VARCHAR(255) DEFAULT NULL, + `token` VARCHAR(255) NOT NULL, `user_id` BIGINT(20) NOT NULL, PRIMARY KEY (`id`), + UNIQUE KEY `ux_password_reset_token_token` (`token`), KEY `FKns9q9f0f318uaoxiqn6lka9ux` (`user_id`), CONSTRAINT `FKns9q9f0f318uaoxiqn6lka9ux` FOREIGN KEY (`user_id`) REFERENCES `user_account` (`id`) ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_general_ci; @@ -32,16 +33,18 @@ DROP TABLE IF EXISTS `privilege`; CREATE TABLE `privilege` ( `id` BIGINT(20) NOT NULL, `description` VARCHAR(255) DEFAULT NULL, - `name` VARCHAR(255) DEFAULT NULL, - PRIMARY KEY (`id`) + `name` VARCHAR(255) NOT NULL, + PRIMARY KEY (`id`), + UNIQUE KEY `ux_privilege_name` (`name`) ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_general_ci; DROP TABLE IF EXISTS `role`; CREATE TABLE `role` ( `id` BIGINT(20) NOT NULL, `description` VARCHAR(255) DEFAULT NULL, - `name` VARCHAR(255) DEFAULT NULL, - PRIMARY KEY (`id`) + `name` VARCHAR(255) NOT NULL, + PRIMARY KEY (`id`), + UNIQUE KEY `ux_role_name` (`name`) ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_general_ci; DROP TABLE IF EXISTS `roles_privileges`; @@ -86,9 +89,10 @@ DROP TABLE IF EXISTS `verification_token`; CREATE TABLE `verification_token` ( `id` BIGINT(20) NOT NULL, `expiry_date` DATETIME(6) DEFAULT NULL, - `token` VARCHAR(255) DEFAULT NULL, + `token` VARCHAR(255) NOT NULL, `user_id` BIGINT(20) NOT NULL, PRIMARY KEY (`id`), + UNIQUE KEY `ux_verification_token_token` (`token`), KEY `FK_VERIFY_USER` (`user_id`), CONSTRAINT `FK_VERIFY_USER` FOREIGN KEY (`user_id`) REFERENCES `user_account` (`id`) ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_general_ci; diff --git a/gradle.properties b/gradle.properties index 64bc9e5c..651d7d70 100644 --- a/gradle.properties +++ b/gradle.properties @@ -1,3 +1,3 @@ -version=4.4.1-SNAPSHOT +version=5.0.0-SNAPSHOT mavenCentralPublishing=true mavenCentralAutomaticPublishing=true diff --git a/src/main/java/com/digitalsanctuary/spring/user/MessageSourceEnvironmentPostProcessor.java b/src/main/java/com/digitalsanctuary/spring/user/MessageSourceEnvironmentPostProcessor.java new file mode 100644 index 00000000..ab3d0721 --- /dev/null +++ b/src/main/java/com/digitalsanctuary/spring/user/MessageSourceEnvironmentPostProcessor.java @@ -0,0 +1,72 @@ +package com.digitalsanctuary.spring.user; + +import java.util.LinkedHashSet; +import java.util.Map; +import java.util.Set; +import org.springframework.boot.SpringApplication; +import org.springframework.boot.env.EnvironmentPostProcessor; +import org.springframework.core.Ordered; +import org.springframework.core.env.ConfigurableEnvironment; +import org.springframework.core.env.MapPropertySource; +import org.springframework.util.StringUtils; + +/** + * Registers the library's own message bundle ({@value #LIBRARY_BASENAME}) additively, without overriding the consuming application's + * {@code spring.messages.basename}. + * + *

+ * Previously the library shipped a hardcoded {@code spring.messages.basename} default property, which clobbered the consumer's configured (or + * conventional default) message bundle. This post-processor instead reads the existing {@code spring.messages.basename} from the environment (falling + * back to Spring Boot's conventional default of {@code messages} when unset) and appends the library bundle to it, de-duplicated, with the library + * bundle placed LAST so consumer message keys win on collisions. + *

+ */ +public class MessageSourceEnvironmentPostProcessor implements EnvironmentPostProcessor, Ordered { + + /** The {@code spring.messages.basename} property key. */ + public static final String BASENAME_PROPERTY = "spring.messages.basename"; + + /** Spring Boot's conventional default basename used when the consumer has not configured one. */ + public static final String DEFAULT_BASENAME = "messages"; + + /** The library's own message bundle, appended last so consumer keys win on collisions. */ + public static final String LIBRARY_BASENAME = "messages/dsspringusermessages"; + + /** Name of the property source this post-processor contributes. */ + private static final String PROPERTY_SOURCE_NAME = "dsSpringUserMessageBasename"; + + @Override + public void postProcessEnvironment(ConfigurableEnvironment environment, SpringApplication application) { + String existing = environment.getProperty(BASENAME_PROPERTY); + String merged = mergeBasename(existing); + environment.getPropertySources().addFirst(new MapPropertySource(PROPERTY_SOURCE_NAME, Map.of(BASENAME_PROPERTY, merged))); + } + + /** + * Merges the existing basename value (or Spring Boot's default when unset) with the library bundle, preserving order, de-duplicating, and placing the + * library bundle last. + * + * @param existing the current {@code spring.messages.basename} value, or {@code null}/blank if unset + * @return the merged, comma-joined basename list with {@value #LIBRARY_BASENAME} appended last + */ + public static String mergeBasename(String existing) { + Set basenames = new LinkedHashSet<>(); + String base = StringUtils.hasText(existing) ? existing : DEFAULT_BASENAME; + for (String name : base.split(",")) { + String trimmed = name.trim(); + if (StringUtils.hasText(trimmed)) { + basenames.add(trimmed); + } + } + // Ensure the library bundle is last so consumer keys win on collisions. + basenames.remove(LIBRARY_BASENAME); + basenames.add(LIBRARY_BASENAME); + return String.join(",", basenames); + } + + @Override + public int getOrder() { + // Run late so any consumer-supplied basename (from application.properties/yml) is already visible in the environment. + return Ordered.LOWEST_PRECEDENCE; + } +} diff --git a/src/main/java/com/digitalsanctuary/spring/user/UserConfiguration.java b/src/main/java/com/digitalsanctuary/spring/user/UserConfiguration.java index ef65a8b7..4608b4a7 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/UserConfiguration.java +++ b/src/main/java/com/digitalsanctuary/spring/user/UserConfiguration.java @@ -1,6 +1,10 @@ package com.digitalsanctuary.spring.user; +import jakarta.annotation.PostConstruct; +import lombok.extern.slf4j.Slf4j; +import org.springframework.boot.autoconfigure.AutoConfiguration; import org.springframework.boot.autoconfigure.AutoConfigurationExcludeFilter; +import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; import org.springframework.boot.context.TypeExcludeFilter; import org.springframework.context.annotation.ComponentScan; import org.springframework.context.annotation.Configuration; @@ -10,22 +14,27 @@ import org.springframework.scheduling.annotation.EnableAsync; import org.springframework.scheduling.annotation.EnableScheduling; import org.springframework.security.config.annotation.method.configuration.EnableMethodSecurity; -import jakarta.annotation.PostConstruct; -import lombok.extern.slf4j.Slf4j; /** * Main auto-configuration class for the DigitalSanctuary Spring Boot User Framework Library. - * Enables asynchronous processing, retry support, scheduling, method-level security, - * and component scanning for the user framework package. + * Provides component scanning for the user framework package and enables, by default, asynchronous + * processing, retry support, scheduling, and method-level security. + * + *

+ * Each cross-cutting enabler is gated behind its own opt-out property (all default {@code true}, so + * behavior is unchanged unless explicitly disabled). A consuming application that already manages one + * of these concerns globally can disable the library's copy to avoid double-activation conflicts: + *

    + *
  • {@code user.async.enabled} → {@code @EnableAsync}
    • + *
  • {@code user.retry.enabled} → {@code @EnableRetry}
    • + *
  • {@code user.scheduling.enabled} → {@code @EnableScheduling}
    • + *
  • {@code user.method-security.enabled} → {@code @EnableMethodSecurity}
    • + *
* * @see UserAutoConfigurationRegistrar */ @Slf4j -@Configuration -@EnableAsync -@EnableRetry -@EnableScheduling -@EnableMethodSecurity +@AutoConfiguration @ComponentScan(basePackages = "com.digitalsanctuary.spring.user", excludeFilters = {@ComponentScan.Filter(type = FilterType.CUSTOM, classes = TypeExcludeFilter.class), @ComponentScan.Filter(type = FilterType.CUSTOM, classes = AutoConfigurationExcludeFilter.class)}) @@ -42,4 +51,48 @@ public void onStartup() { log.info("DigitalSanctuary SpringBoot User Framework Library loaded."); } + /** + * Enables Spring's asynchronous method execution support for the library. Gated behind + * {@code user.async.enabled} (default {@code true}). Disable if the consuming application already + * enables async processing globally to avoid double-activation. + */ + @Configuration(proxyBeanMethods = false) + @ConditionalOnProperty(name = "user.async.enabled", havingValue = "true", matchIfMissing = true) + @EnableAsync + static class AsyncConfiguration { + } + + /** + * Enables Spring Retry support for the library. Gated behind {@code user.retry.enabled} + * (default {@code true}). Disable if the consuming application already enables retry globally to + * avoid double-activation. + */ + @Configuration(proxyBeanMethods = false) + @ConditionalOnProperty(name = "user.retry.enabled", havingValue = "true", matchIfMissing = true) + @EnableRetry + static class RetryConfiguration { + } + + /** + * Enables Spring's scheduled task execution for the library (used by token purge and similar jobs). + * Gated behind {@code user.scheduling.enabled} (default {@code true}). Disable if the consuming + * application already enables scheduling globally to avoid double-activation. + */ + @Configuration(proxyBeanMethods = false) + @ConditionalOnProperty(name = "user.scheduling.enabled", havingValue = "true", matchIfMissing = true) + @EnableScheduling + static class SchedulingConfiguration { + } + + /** + * Enables Spring Security method-level security for the library. Gated behind + * {@code user.method-security.enabled} (default {@code true}). Disable if the consuming application + * already enables method security globally to avoid double-activation. + */ + @Configuration(proxyBeanMethods = false) + @ConditionalOnProperty(name = "user.method-security.enabled", havingValue = "true", matchIfMissing = true) + @EnableMethodSecurity + static class MethodSecurityConfiguration { + } + } diff --git a/src/main/java/com/digitalsanctuary/spring/user/api/GdprAPI.java b/src/main/java/com/digitalsanctuary/spring/user/api/GdprAPI.java index a2405743..25b7710f 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/api/GdprAPI.java +++ b/src/main/java/com/digitalsanctuary/spring/user/api/GdprAPI.java @@ -52,7 +52,7 @@ */ @Slf4j @RequiredArgsConstructor -@RestController +@RestController("dsGdprAPI") @RequestMapping(path = "/user/gdpr", produces = "application/json") public class GdprAPI { diff --git a/src/main/java/com/digitalsanctuary/spring/user/api/MfaAPI.java b/src/main/java/com/digitalsanctuary/spring/user/api/MfaAPI.java index 8e272827..06578ee0 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/api/MfaAPI.java +++ b/src/main/java/com/digitalsanctuary/spring/user/api/MfaAPI.java @@ -28,7 +28,7 @@ *

*/ @Slf4j -@RestController +@RestController("dsMfaAPI") @RequestMapping(path = "/user/mfa", produces = "application/json") @ConditionalOnProperty(name = "user.mfa.enabled", havingValue = "true", matchIfMissing = false) @RequiredArgsConstructor diff --git a/src/main/java/com/digitalsanctuary/spring/user/api/UserAPI.java b/src/main/java/com/digitalsanctuary/spring/user/api/UserAPI.java index 1dd2dfb4..3ac8bf81 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/api/UserAPI.java +++ b/src/main/java/com/digitalsanctuary/spring/user/api/UserAPI.java @@ -38,6 +38,7 @@ import com.digitalsanctuary.spring.user.service.UserEmailService; import com.digitalsanctuary.spring.user.service.UserService; import com.digitalsanctuary.spring.user.service.WebAuthnCredentialManagementService; +import com.digitalsanctuary.spring.user.util.AppUrlResolver; import com.digitalsanctuary.spring.user.util.JSONResponse; import com.digitalsanctuary.spring.user.util.UserUtils; import jakarta.servlet.ServletException; @@ -59,19 +60,36 @@ */ @Slf4j @RequiredArgsConstructor -@RestController +@RestController("dsUserAPI") @RequestMapping(path = "/user", produces = "application/json") public class UserAPI { /** Error code returned when the {@link RegistrationGuard} denies a registration attempt. */ private static final int ERROR_CODE_REGISTRATION_DENIED = 6; + /** + * Generic, success-shaped message returned by {@code /registration} for every outcome (new account + * created, or email already registered). Keeping the body identical across cases prevents an attacker + * from using the registration endpoint to enumerate which email addresses are already registered. + */ + private static final String REGISTRATION_GENERIC_MESSAGE = + "If your email address is eligible, you will receive a verification email shortly."; + + /** + * Generic, success-shaped message returned by {@code /resendRegistrationToken} for every outcome + * (email unknown, account already verified, or verification email actually resent). Keeping the body + * identical across cases prevents enumeration of which emails exist and which are already verified. + */ + private static final String RESEND_GENERIC_MESSAGE = + "If your account requires verification, a new verification email has been sent."; + private final UserService userService; private final UserEmailService userEmailService; private final MessageSource messages; private final ApplicationEventPublisher eventPublisher; private final PasswordPolicyService passwordPolicyService; private final ObjectProvider webAuthnCredentialManagementServiceProvider; + private final AppUrlResolver appUrlResolver; @Value("${user.security.registrationPendingURI}") private String registrationPendingURI; @@ -119,14 +137,24 @@ public ResponseEntity registerUserAccount(@Valid @RequestBody User String nextURL = registeredUser.isEnabled() ? handleAutoLogin(registeredUser) : registrationPendingURI; - return buildSuccessResponse("Registration Successful!", nextURL); + return buildSuccessResponse(REGISTRATION_GENERIC_MESSAGE, nextURL); } catch (RegistrationDeniedException ex) { log.info("Registration denied for email: {} source: FORM reason: {}", userDto.getEmail(), ex.getReason()); + logAuditEvent("Registration", "Failure", "Registration Denied: " + ex.getReason(), null, request); return buildErrorResponse(ex.getReason(), ERROR_CODE_REGISTRATION_DENIED, HttpStatus.FORBIDDEN); } catch (UserAlreadyExistException ex) { + // Anti-enumeration: the email is already registered, so we create NOTHING and publish no + // registration event, but we return exactly the same generic 200 response a brand-new + // registration would produce. The true reason is recorded server-side via the audit event. + // + // Returning registrationPendingURI here mirrors what a genuine new (unverified) registration + // returns in the default verification-enabled config, making both cases indistinguishable to + // the caller. In verification-disabled / auto-login mode a real new registration additionally + // establishes a session — that is an inherent, accepted difference that cannot be avoided + // without skipping auto-login for legitimate new users. log.warn("User already exists with email: {}", userDto.getEmail()); logAuditEvent("Registration", "Failure", "User Already Exists", null, request); - return buildErrorResponse("An account already exists for the email address", 2, HttpStatus.CONFLICT); + return buildSuccessResponse(REGISTRATION_GENERIC_MESSAGE, registrationPendingURI); } catch (Exception ex) { log.error("Unexpected error during registration.", ex); logAuditEvent("Registration", "Failure", ex.getMessage(), null, request); @@ -146,16 +174,22 @@ public ResponseEntity registerUserAccount(@Valid @RequestBody User @PostMapping("/resendRegistrationToken") public ResponseEntity resendRegistrationToken(@Valid @RequestBody UserDto userDto, HttpServletRequest request) { + // Anti-enumeration: this endpoint ALWAYS returns the same generic 200 response, regardless of + // whether the email is unknown, already verified, or genuinely awaiting verification. Internally + // we only send the verification email when the account exists AND is still unverified. The true + // outcome is recorded server-side via audit/log events so operators retain visibility. User user = userService.findUserByEmail(userDto.getEmail()); - if (user != null) { - if (user.isEnabled()) { - return buildErrorResponse("Account is already verified.", 1, HttpStatus.CONFLICT); - } - userEmailService.sendRegistrationVerificationEmail(user, UserUtils.getAppUrl(request)); + if (user == null) { + log.info("Resend verification requested for unknown email; returning generic response."); + logAuditEvent("Resend Reg Token", "Failure", "Unknown Email", null, request); + } else if (user.isEnabled()) { + log.info("Resend verification requested for already-verified account; returning generic response."); + logAuditEvent("Resend Reg Token", "Failure", "Account Already Verified", user, request); + } else { + userEmailService.sendRegistrationVerificationEmail(user, appUrlResolver.resolveAppUrl(request)); logAuditEvent("Resend Reg Token", "Success", "Verification Email Resent", user, request); - return buildSuccessResponse("Verification Email Resent Successfully!", registrationPendingURI); } - return buildErrorResponse("System Error!", 2, HttpStatus.INTERNAL_SERVER_ERROR); + return buildSuccessResponse(RESEND_GENERIC_MESSAGE, registrationPendingURI); } /** @@ -203,7 +237,7 @@ public ResponseEntity updateUserAccount(@AuthenticationPrincipal D public ResponseEntity resetPassword(@Valid @RequestBody PasswordResetRequestDto passwordResetRequest, HttpServletRequest request) { User user = userService.findUserByEmail(passwordResetRequest.getEmail()); if (user != null) { - userEmailService.sendForgotPasswordVerificationEmail(user, UserUtils.getAppUrl(request)); + userEmailService.sendForgotPasswordVerificationEmail(user, appUrlResolver.resolveAppUrl(request)); logAuditEvent("Reset Password", "Success", "Password reset email sent", user, request); } return buildSuccessResponse("If account exists, password reset email has been sent!", forgotPasswordPendingURI); @@ -427,11 +461,23 @@ public ResponseEntity registerPasswordlessAccount(@Valid @RequestB return buildSuccessResponse("Registration Successful!", nextURL); } catch (RegistrationDeniedException ex) { log.info("Registration denied for email: {} source: PASSWORDLESS reason: {}", dto.getEmail(), ex.getReason()); + logAuditEvent("PasswordlessRegistration", "Failure", "Registration Denied: " + ex.getReason(), null, request); return buildErrorResponse(ex.getReason(), ERROR_CODE_REGISTRATION_DENIED, HttpStatus.FORBIDDEN); } catch (UserAlreadyExistException ex) { + // Anti-enumeration: the email is already registered, so we create NOTHING and publish no + // registration event, but we return exactly the same generic 200 response a brand-new + // passwordless registration would produce. The true reason is recorded server-side via the + // audit event. This mirrors the form-registration path and prevents this endpoint from being + // used to enumerate which email addresses are already registered (previously returned 409). + // + // Returning registrationPendingURI here mirrors what a genuine new (unverified) registration + // returns in the default verification-enabled config, making both cases indistinguishable to + // the caller. In verification-disabled / auto-login mode a real new registration additionally + // establishes a session — that is an inherent, accepted difference that cannot be avoided + // without skipping auto-login for legitimate new users. log.warn("User already exists with email: {}", dto.getEmail()); logAuditEvent("PasswordlessRegistration", "Failure", "User Already Exists", null, request); - return buildErrorResponse("An account already exists for the email address", 2, HttpStatus.CONFLICT); + return buildSuccessResponse("Registration Successful!", registrationPendingURI); } catch (Exception ex) { log.error("Unexpected error during passwordless registration.", ex); logAuditEvent("PasswordlessRegistration", "Failure", ex.getMessage(), null, request); @@ -442,6 +488,20 @@ public ResponseEntity registerPasswordlessAccount(@Valid @RequestB /** * Sets an initial password for a passwordless account. * + *

+ * This endpoint only applies to passwordless (passkey-only) accounts and rejects the request if a password is already + * set (use {@code /user/updatePassword} to change an existing password, which requires the current password). Because + * the account has no current password to verify, this credential-altering operation cannot require re-authentication + * via a current password, and this library does not yet implement a WebAuthn step-up assertion. + *

+ * + *

+ * Residual risk: a session-only actor on a passwordless account could set an initial password. This is + * a known, documented limitation (see MIGRATION.md "Re-authentication required for credential changes"). It is not a + * regression and is bounded: the new password does not displace any existing credential, and consuming applications can + * front this endpoint with their own step-up if required. + *

+ * * @param userDetails the authenticated user details * @param setPasswordDto the set password DTO * @param request the HTTP servlet request @@ -542,8 +602,10 @@ private void logoutUser(HttpServletRequest request) { * @param request the HTTP servlet request */ private void publishRegistrationEvent(User user, HttpServletRequest request) { - String appUrl = UserUtils.getAppUrl(request); - eventPublisher.publishEvent(new OnRegistrationCompleteEvent(user, request.getLocale(), appUrl)); + String appUrl = appUrlResolver.resolveAppUrl(request); + // Capture immutable scalars from the still-attached entity so the @Async listener never touches a detached User. + eventPublisher.publishEvent(OnRegistrationCompleteEvent.builder().userId(user.getId()).userEmail(user.getEmail()) + .userEnabled(user.isEnabled()).locale(request.getLocale()).appUrl(appUrl).build()); } /** diff --git a/src/main/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPI.java b/src/main/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPI.java index 306ff70d..99b4e03a 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPI.java +++ b/src/main/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPI.java @@ -6,6 +6,7 @@ import org.springframework.http.ResponseEntity; import org.springframework.security.core.annotation.AuthenticationPrincipal; import org.springframework.security.core.userdetails.UserDetails; +import org.springframework.validation.annotation.Validated; import org.springframework.web.bind.annotation.DeleteMapping; import org.springframework.web.bind.annotation.GetMapping; import org.springframework.web.bind.annotation.PathVariable; @@ -18,6 +19,7 @@ import com.digitalsanctuary.spring.user.exceptions.WebAuthnException; import com.digitalsanctuary.spring.user.exceptions.WebAuthnUserNotFoundException; import com.digitalsanctuary.spring.user.persistence.model.User; +import com.digitalsanctuary.spring.user.service.LoginAttemptService; import com.digitalsanctuary.spring.user.service.UserService; import com.digitalsanctuary.spring.user.service.WebAuthnCredentialManagementService; import com.digitalsanctuary.spring.user.util.GenericResponse; @@ -28,7 +30,6 @@ import jakarta.validation.constraints.Size; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; -import org.springframework.validation.annotation.Validated; /** * REST API for WebAuthn credential management. @@ -46,10 +47,25 @@ *
  • GET /user/webauthn/has-credentials - Check if user has any passkeys
    • *
  • PUT /user/webauthn/credentials/{id}/label - Rename a passkey
    • *
  • DELETE /user/webauthn/credentials/{id} - Delete a passkey
    • + *
  • DELETE /user/webauthn/password - Remove the account password (passkey-only)
    • * + * + *

    + * Re-authentication for credential-altering operations: Removing the password and deleting/renaming a + * passkey change the account's authentication methods. When the account has a password set, these operations require the + * caller to supply the current password ({@code currentPassword}), which is verified before any mutation. This prevents a + * session-only actor (e.g. via a hijacked or unattended session) from silently altering how the account authenticates. + *

    + * + *

    + * Residual risk (passwordless accounts): For passwordless (passkey-only) accounts there is no current + * password to verify, and this library does not yet implement a WebAuthn step-up assertion. Passkey delete/rename on such + * accounts therefore remain session-only operations. Last-credential protection still prevents lockout, and ownership + * (IDOR) checks remain enforced in the service layer. See MIGRATION.md for details and guidance. + *

    */ @Slf4j -@RestController +@RestController("dsWebAuthnManagementAPI") @RequestMapping("/user/webauthn") @ConditionalOnProperty(name = "user.webauthn.enabled", havingValue = "true", matchIfMissing = false) @RequiredArgsConstructor @@ -59,6 +75,7 @@ public class WebAuthnManagementAPI { private final WebAuthnCredentialManagementService credentialManagementService; private final UserService userService; private final ApplicationEventPublisher eventPublisher; + private final LoginAttemptService loginAttemptService; /** * Get user's registered passkeys. @@ -107,6 +124,7 @@ public ResponseEntity renameCredential(@PathVariable @NotBlank @RequestBody @Valid RenameCredentialRequest request, @AuthenticationPrincipal UserDetails userDetails) { User user = findAuthenticatedUser(userDetails); + requireCurrentPasswordIfSet(user, request.currentPassword()); credentialManagementService.renameCredential(id, request.label(), user); return ResponseEntity.ok(new GenericResponse("Passkey renamed successfully")); } @@ -123,14 +141,24 @@ public ResponseEntity renameCredential(@PathVariable @NotBlank * If this is the user's last passkey and they have no password, the deletion will be blocked with an error message. *

    * + *

    + * Re-authentication: If the account has a password set, the request body must include the current + * {@code currentPassword}, which is verified before the passkey is deleted. This prevents a session-only actor from + * altering the account's authentication methods. For passwordless (passkey-only) accounts there is no current + * credential to verify; see the class-level note on the residual risk for passwordless credential changes. + *

    + * * @param id the credential ID to delete + * @param request the (optional) request body carrying the current password; may be {@code null} for passwordless accounts * @param userDetails the authenticated user details * @return ResponseEntity with success message or error */ @DeleteMapping("/credentials/{id}") public ResponseEntity deleteCredential(@PathVariable @NotBlank @Size(max = 512) String id, + @RequestBody(required = false) CurrentPasswordRequest request, @AuthenticationPrincipal UserDetails userDetails) { User user = findAuthenticatedUser(userDetails); + requireCurrentPasswordIfSet(user, request != null ? request.currentPassword() : null); credentialManagementService.deleteCredential(id, user); return ResponseEntity.ok(new GenericResponse("Passkey deleted successfully")); } @@ -139,23 +167,37 @@ public ResponseEntity deleteCredential(@PathVariable @NotBlank * Remove the user's password, making the account passwordless (passkey-only). * *

    - * Requires the user to have at least one passkey registered. This ensures - * the user can still authenticate after the password is removed. + * Requires the user to have at least one passkey registered. This ensures the user can still authenticate after the + * password is removed. + *

    + * + *

    + * Re-authentication: Because the account by definition has a password (it is being removed), the + * caller must supply {@code currentPassword} in the request body. The body is declared {@code required = false} so + * that a missing body does not produce a generic 415/400 from the message converter; instead, a missing or empty body + * is treated identically to a missing {@code currentPassword} field — both are routed through + * {@link #requireCurrentPasswordIfSet} and result in HTTP 400 with the message + * {@code "Current password is required to change authentication methods."}. A blank or incorrect + * {@code currentPassword} is likewise rejected with a 400 before any mutation occurs. *

    * + * @param body the request body carrying the current password; technically optional at the HTTP layer so that a + * missing body produces a clear 400 rather than a framework-level error, but functionally required * @param userDetails the authenticated user details * @param request the HTTP servlet request * @return ResponseEntity with success message or error */ @DeleteMapping("/password") - public ResponseEntity removePassword(@AuthenticationPrincipal UserDetails userDetails, - HttpServletRequest request) { + public ResponseEntity removePassword(@RequestBody(required = false) CurrentPasswordRequest body, + @AuthenticationPrincipal UserDetails userDetails, HttpServletRequest request) { User user = findAuthenticatedUser(userDetails); if (!userService.hasPassword(user)) { throw new WebAuthnException("User does not have a password to remove"); } + requireCurrentPasswordIfSet(user, body != null ? body.currentPassword() : null); + if (!credentialManagementService.hasCredentials(user)) { throw new WebAuthnException("Cannot remove password. Please register a passkey first."); } @@ -181,11 +223,63 @@ private User findAuthenticatedUser(UserDetails userDetails) throws WebAuthnUserN return user; } + /** + * Requires and verifies the current password for a credential-altering operation when the account has a password set. + * + *

    + * If the account has a password, the supplied {@code currentPassword} must be present and valid (verified via + * {@link UserService#checkIfValidOldPassword(User, String)}); otherwise a {@link WebAuthnException} (HTTP 400) is + * thrown before any mutation, preventing a session-only actor from changing the account's authentication + * methods. If the account is passwordless (passkey-only) there is no current credential to verify, so this check is + * a no-op — see the residual-risk note in MIGRATION.md. + *

    + * + *

    + * Because this verifies the current password, it is an authentication surface and participates in the same + * brute-force lockout as the login path: a locked account is rejected up front, each wrong password is reported to + * {@link LoginAttemptService#loginFailed(String)} (which locks the account once the threshold is reached), and a + * correct password resets the failed-attempt counter via {@link LoginAttemptService#loginSucceeded(String)}. This + * stops a session-holding actor from making unlimited password guesses here. + *

    + * + * @param user the authenticated user + * @param currentPassword the current password supplied by the client (may be {@code null}) + * @throws WebAuthnException if the account is locked, or has a password and the supplied current password is missing or incorrect + */ + private void requireCurrentPasswordIfSet(User user, String currentPassword) { + if (!userService.hasPassword(user)) { + // Passwordless (passkey-only) account: no current credential exists to verify. See MIGRATION.md residual-risk note. + return; + } + if (loginAttemptService.isLocked(user.getEmail())) { + throw new WebAuthnException("Account is locked due to too many failed attempts. Please try again later."); + } + if (currentPassword == null || currentPassword.isBlank()) { + // A missing field is a client error, not a password guess, so it does not count toward lockout. + throw new WebAuthnException("Current password is required to change authentication methods."); + } + if (!userService.checkIfValidOldPassword(user, currentPassword)) { + loginAttemptService.loginFailed(user.getEmail()); + throw new WebAuthnException("Current password is incorrect."); + } + // Successful re-authentication clears the failed-attempt counter, matching login semantics. + loginAttemptService.loginSucceeded(user.getEmail()); + } + /** * Request DTO for renaming credential. * * @param label the new label (must not be blank, max 64 chars) + * @param currentPassword the current account password, required when the account has a password set (re-authentication) + */ + public record RenameCredentialRequest(@NotBlank @Size(max = 64) String label, String currentPassword) { + } + + /** + * Request DTO carrying the current account password for credential-altering operations that re-authenticate the user. + * + * @param currentPassword the current account password, required when the account has a password set */ - public record RenameCredentialRequest(@NotBlank @Size(max = 64) String label) { + public record CurrentPasswordRequest(String currentPassword) { } } diff --git a/src/main/java/com/digitalsanctuary/spring/user/api/data/Response.java b/src/main/java/com/digitalsanctuary/spring/user/api/data/Response.java deleted file mode 100644 index e69de29b..00000000 diff --git a/src/main/java/com/digitalsanctuary/spring/user/audit/AuditMailAutoConfiguration.java b/src/main/java/com/digitalsanctuary/spring/user/audit/AuditMailAutoConfiguration.java index a8002c48..54d63681 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/audit/AuditMailAutoConfiguration.java +++ b/src/main/java/com/digitalsanctuary/spring/user/audit/AuditMailAutoConfiguration.java @@ -87,7 +87,7 @@ public FileAuditLogFlushScheduler fileAuditLogFlushScheduler(FileAuditLogWriter * @param mailContentBuilder the mail content builder * @return the default {@link MailService} */ - @Bean + @Bean("dsMailService") @ConditionalOnMissingBean(MailService.class) public MailService mailService(ObjectProvider mailSenderProvider, MailContentBuilder mailContentBuilder) { return new MailService(mailSenderProvider, mailContentBuilder); diff --git a/src/main/java/com/digitalsanctuary/spring/user/controller/UserActionController.java b/src/main/java/com/digitalsanctuary/spring/user/controller/UserActionController.java index bf69bf56..f595277f 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/controller/UserActionController.java +++ b/src/main/java/com/digitalsanctuary/spring/user/controller/UserActionController.java @@ -36,7 +36,7 @@ */ @Slf4j @RequiredArgsConstructor -@Controller +@Controller("dsUserActionController") @IncludeUserInModel public class UserActionController { private static final String AUTH_MESSAGE_PREFIX = "auth.message."; diff --git a/src/main/java/com/digitalsanctuary/spring/user/controller/UserPageController.java b/src/main/java/com/digitalsanctuary/spring/user/controller/UserPageController.java index e01ec4a8..5072f260 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/controller/UserPageController.java +++ b/src/main/java/com/digitalsanctuary/spring/user/controller/UserPageController.java @@ -26,7 +26,7 @@ */ @Slf4j @RequiredArgsConstructor -@Controller +@Controller("dsUserPageController") @IncludeUserInModel public class UserPageController { diff --git a/src/main/java/com/digitalsanctuary/spring/user/event/ConsentChangedEvent.java b/src/main/java/com/digitalsanctuary/spring/user/event/ConsentChangedEvent.java index 09353a05..0e403ee2 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/event/ConsentChangedEvent.java +++ b/src/main/java/com/digitalsanctuary/spring/user/event/ConsentChangedEvent.java @@ -3,7 +3,6 @@ import org.springframework.context.ApplicationEvent; import com.digitalsanctuary.spring.user.gdpr.ConsentRecord; import com.digitalsanctuary.spring.user.gdpr.ConsentType; -import com.digitalsanctuary.spring.user.persistence.model.User; /** * Event published when a user's consent status changes. @@ -14,6 +13,12 @@ * updating mailing lists, disabling features, or synchronizing with * external consent management systems. * + *

    As of 5.0.0 this event no longer carries a live JPA {@code User} entity. Instead it exposes immutable scalar data + * ({@code userId}, {@code userEmail}) captured at publish time, alongside the {@link ConsentRecord} (a plain DTO, not a + * JPA entity) and the {@link ChangeType}. This prevents detached-entity / {@code LazyInitializationException} hazards + * when the event is consumed across threads. If a listener needs the full {@code User}, it should load it by + * {@code userId} from {@code UserRepository} inside its own transaction. + * * @see ConsentRecord * @see ConsentType * @see com.digitalsanctuary.spring.user.gdpr.ConsentAuditService @@ -38,9 +43,14 @@ public enum ChangeType { } /** - * The user whose consent changed. + * The ID of the user whose consent changed. */ - private final User user; + private final Long userId; + + /** + * The email of the user whose consent changed. + */ + private final String userEmail; /** * The consent record with details of the change. @@ -56,33 +66,35 @@ public enum ChangeType { * Creates a new ConsentChangedEvent. * * @param source the object on which the event initially occurred - * @param user the user whose consent changed + * @param userId the ID of the user whose consent changed + * @param userEmail the email of the user whose consent changed * @param consentRecord the consent record with details * @param changeType whether consent was granted or withdrawn */ - public ConsentChangedEvent(Object source, User user, ConsentRecord consentRecord, ChangeType changeType) { + public ConsentChangedEvent(Object source, Long userId, String userEmail, ConsentRecord consentRecord, ChangeType changeType) { super(source); - this.user = user; + this.userId = userId; + this.userEmail = userEmail; this.consentRecord = consentRecord; this.changeType = changeType; } /** - * Gets the user whose consent changed. + * Gets the ID of the user whose consent changed. * - * @return the user + * @return the user ID */ - public User getUser() { - return user; + public Long getUserId() { + return userId; } /** - * Gets the ID of the user whose consent changed. + * Gets the email of the user whose consent changed. * - * @return the user ID + * @return the user email */ - public Long getUserId() { - return user != null ? user.getId() : null; + public String getUserEmail() { + return userEmail; } /** diff --git a/src/main/java/com/digitalsanctuary/spring/user/event/OnRegistrationCompleteEvent.java b/src/main/java/com/digitalsanctuary/spring/user/event/OnRegistrationCompleteEvent.java index bf6c85a9..4fb14940 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/event/OnRegistrationCompleteEvent.java +++ b/src/main/java/com/digitalsanctuary/spring/user/event/OnRegistrationCompleteEvent.java @@ -2,16 +2,25 @@ import java.util.Locale; import org.springframework.context.ApplicationEvent; -import com.digitalsanctuary.spring.user.persistence.model.User; import lombok.Builder; -import lombok.Data; import lombok.EqualsAndHashCode; +import lombok.Getter; +import lombok.ToString; /** - * The OnRegistrationCompleteEvent class is triggered when a user registers. We are using to send the registration verification email, if enabled, + * The OnRegistrationCompleteEvent class is triggered when a user registers. We are using it to send the registration verification email, if enabled, * asynchronously. You can also listen for this event and perform any other post-registration processing desired. + * + *

    + * As of 5.0.0 this event no longer carries a live JPA {@code User} entity. Instead it exposes immutable scalar data + * ({@code userId}, {@code userEmail}, {@code userEnabled}) captured at publish time, while the entity is still attached + * to a persistence context. This prevents detached-entity / {@code LazyInitializationException} hazards when the event + * is consumed by {@code @Async} listeners on a different thread. If a listener needs the full {@code User}, it should + * load it by {@code userId} from {@code UserRepository} inside its own transaction. + *

    */ -@Data +@Getter +@ToString @EqualsAndHashCode(callSuper = false) public class OnRegistrationCompleteEvent extends ApplicationEvent { @@ -24,20 +33,35 @@ public class OnRegistrationCompleteEvent extends ApplicationEvent { /** The locale. */ private final Locale locale; - /** The user. */ - private final User user; + /** The id of the registered user. */ + private final Long userId; + + /** The email of the registered user (used as the verification-email recipient). */ + private final String userEmail; + + /** + * Whether the registered user is already enabled. First-time OAuth2/OIDC registrations are created enabled (the + * provider has already verified the email), so consumers such as the verification-email listener can skip sending + * a verification email to these users. + */ + private final boolean userEnabled; /** * Instantiates a new on registration complete event. * - * @param user the user + * @param userId the id of the registered user + * @param userEmail the email of the registered user + * @param userEnabled whether the registered user is already enabled * @param locale the locale * @param appUrl the app url */ @Builder - public OnRegistrationCompleteEvent(final User user, final Locale locale, final String appUrl) { - super(user); - this.user = user; + public OnRegistrationCompleteEvent(final Long userId, final String userEmail, final boolean userEnabled, final Locale locale, + final String appUrl) { + super(userId != null ? userId : "OnRegistrationCompleteEvent"); + this.userId = userId; + this.userEmail = userEmail; + this.userEnabled = userEnabled; this.locale = locale; this.appUrl = appUrl; } diff --git a/src/main/java/com/digitalsanctuary/spring/user/event/UserPreDeleteEvent.java b/src/main/java/com/digitalsanctuary/spring/user/event/UserPreDeleteEvent.java index 054febcc..bfb77188 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/event/UserPreDeleteEvent.java +++ b/src/main/java/com/digitalsanctuary/spring/user/event/UserPreDeleteEvent.java @@ -1,7 +1,6 @@ package com.digitalsanctuary.spring.user.event; import org.springframework.context.ApplicationEvent; -import com.digitalsanctuary.spring.user.persistence.model.User; /** * Event published before a user entity is deleted. This event can be used to perform any necessary actions or checks before the deletion occurs. @@ -11,42 +10,56 @@ * cascading deletions. *

    * - * @see User + *

    + * As of 5.0.0 this event no longer carries a live JPA {@code User} entity. Instead it exposes immutable scalar data + * ({@code userId}, {@code userEmail}) captured at publish time. This prevents detached-entity / + * {@code LazyInitializationException} hazards when the event is consumed across threads. If a listener needs the full + * {@code User}, it should load it by {@code userId} from {@code UserRepository} inside its own transaction. + *

    */ public class UserPreDeleteEvent extends ApplicationEvent { + private static final long serialVersionUID = 1L; + + /** + * The ID of the user that is about to be deleted. + */ + private final Long userId; + /** - * The user entity that is about to be deleted. + * The email of the user that is about to be deleted. */ - private final User user; + private final String userEmail; /** - * Create a new UserDeleteEvent. + * Create a new UserPreDeleteEvent. * * @param source The object on which the event initially occurred (never {@code null}) - * @param user The user entity that is about to be deleted (never {@code null}) + * @param userId The ID of the user that is about to be deleted (never {@code null}) + * @param userEmail The email of the user that is about to be deleted */ - public UserPreDeleteEvent(Object source, User user) { + public UserPreDeleteEvent(Object source, Long userId, String userEmail) { super(source); - this.user = user; + this.userId = userId; + this.userEmail = userEmail; } /** - * Get the user entity that is about to be deleted. + * Get the ID of the user that is about to be deleted. * - * @return The user entity (never {@code null}) + * @return The ID of the user (never {@code null}) */ - public User getUser() { - return user; + public Long getUserId() { + return userId; } /** - * Get the ID of the user entity that is about to be deleted. + * Get the email of the user that is about to be deleted. * - * @return The ID of the user entity (never {@code null}) + * @return The email of the user */ - public Long getUserId() { - return user.getId(); + public String getUserEmail() { + return userEmail; } } diff --git a/src/main/java/com/digitalsanctuary/spring/user/exception/GlobalValidationExceptionHandler.java b/src/main/java/com/digitalsanctuary/spring/user/exception/GlobalValidationExceptionHandler.java deleted file mode 100644 index ef11cf44..00000000 --- a/src/main/java/com/digitalsanctuary/spring/user/exception/GlobalValidationExceptionHandler.java +++ /dev/null @@ -1,50 +0,0 @@ -package com.digitalsanctuary.spring.user.exception; - -import java.util.HashMap; -import java.util.Map; - -import org.springframework.http.HttpStatus; -import org.springframework.http.ResponseEntity; -import org.springframework.validation.FieldError; -import org.springframework.web.bind.MethodArgumentNotValidException; -import org.springframework.web.bind.annotation.ControllerAdvice; -import org.springframework.web.bind.annotation.ExceptionHandler; - -import lombok.extern.slf4j.Slf4j; - -/** - * Global exception handler for validation errors across all API endpoints. - * Handles {@link MethodArgumentNotValidException} thrown by {@code @Valid} annotations - * and returns structured error responses with field-level validation details. - */ -@Slf4j -@ControllerAdvice -public class GlobalValidationExceptionHandler { - - /** - * Handles validation errors from @Valid annotations on request bodies. - * - * @param ex the MethodArgumentNotValidException - * @return a ResponseEntity containing validation error details - */ - @ExceptionHandler(MethodArgumentNotValidException.class) - public ResponseEntity> handleValidationExceptions(MethodArgumentNotValidException ex) { - log.warn("Validation error occurred: {}", ex.getMessage()); - - Map response = new HashMap<>(); - Map errors = new HashMap<>(); - - ex.getBindingResult().getAllErrors().forEach((error) -> { - String fieldName = ((FieldError) error).getField(); - String errorMessage = error.getDefaultMessage(); - errors.put(fieldName, errorMessage); - }); - - response.put("success", false); - response.put("code", 400); - response.put("message", "Validation failed"); - response.put("errors", errors); - - return ResponseEntity.status(HttpStatus.BAD_REQUEST).body(response); - } -} \ No newline at end of file diff --git a/src/main/java/com/digitalsanctuary/spring/user/exceptions/GlobalValidationExceptionHandler.java b/src/main/java/com/digitalsanctuary/spring/user/exceptions/GlobalValidationExceptionHandler.java new file mode 100644 index 00000000..44530686 --- /dev/null +++ b/src/main/java/com/digitalsanctuary/spring/user/exceptions/GlobalValidationExceptionHandler.java @@ -0,0 +1,135 @@ +package com.digitalsanctuary.spring.user.exceptions; + +import java.util.HashMap; +import java.util.Map; + +import org.springframework.http.HttpStatus; +import org.springframework.http.ResponseEntity; +import org.springframework.validation.FieldError; +import org.springframework.validation.ObjectError; +import org.springframework.web.bind.MethodArgumentNotValidException; +import org.springframework.web.bind.annotation.ControllerAdvice; +import org.springframework.web.bind.annotation.ExceptionHandler; + +import com.digitalsanctuary.spring.user.api.GdprAPI; +import com.digitalsanctuary.spring.user.api.MfaAPI; +import com.digitalsanctuary.spring.user.api.UserAPI; +import com.digitalsanctuary.spring.user.controller.UserActionController; +import com.digitalsanctuary.spring.user.controller.UserPageController; + +import jakarta.validation.ConstraintViolation; +import jakarta.validation.ConstraintViolationException; +import lombok.extern.slf4j.Slf4j; + +/** + * Validation exception handler for the framework's own controllers. + * + *

    + * This advice is intentionally scoped to the library's controllers via + * {@code assignableTypes}. It does not apply application-wide, so it will never intercept (or reformat) + * validation errors raised by a consuming application's own controllers. Consuming applications that + * want uniform validation error formatting for THEIR controllers must provide their own + * {@code @ControllerAdvice}. + *

    + * + *

    + * {@code WebAuthnManagementAPI} is deliberately excluded: it has its own dedicated advice + * ({@code WebAuthnManagementAPIAdvice}) which already handles {@link MethodArgumentNotValidException} + * and {@link ConstraintViolationException}. Including it here would create two advices targeting the + * same controller with overlapping {@code @ExceptionHandler} types and ambiguous resolution. + *

    + * + *

    + * Handles {@link MethodArgumentNotValidException} (both field-level and class-level/global errors, + * such as the class-level {@code @PasswordMatches} constraint on {@code UserDto}) and + * {@link ConstraintViolationException}, returning a structured HTTP 400 response. + *

    + */ +@Slf4j +@ControllerAdvice(assignableTypes = {UserAPI.class, GdprAPI.class, MfaAPI.class, UserActionController.class, UserPageController.class}) +public class GlobalValidationExceptionHandler { + + /** + * Handles validation errors from {@code @Valid} annotations on request bodies. + * + *

    + * Both field-level errors and global (class-level) errors are collected. Class-level constraints + * such as {@code @PasswordMatches} produce a global error rather than a field error; collecting + * the global errors surfaces them in the response as a structured 400 instead of failing with a + * 500. + *

    + * + * @param ex the MethodArgumentNotValidException + * @return a ResponseEntity containing validation error details + */ + @ExceptionHandler(MethodArgumentNotValidException.class) + public ResponseEntity> handleValidationExceptions(MethodArgumentNotValidException ex) { + log.warn("Validation error occurred: {}", ex.getMessage()); + + Map errors = new HashMap<>(); + + ex.getBindingResult().getFieldErrors().forEach((error) -> { + errors.put(error.getField(), error.getDefaultMessage()); + }); + + ex.getBindingResult().getGlobalErrors().forEach((error) -> { + errors.put(globalErrorKey(error), error.getDefaultMessage()); + }); + + return badRequest(errors); + } + + /** + * Handles {@link ConstraintViolationException} raised outside of method-argument binding, for + * example by {@code @Validated} on controller method parameters. + * + * @param ex the ConstraintViolationException + * @return a ResponseEntity containing validation error details + */ + @ExceptionHandler(ConstraintViolationException.class) + public ResponseEntity> handleConstraintViolationException(ConstraintViolationException ex) { + log.warn("Constraint violation occurred: {}", ex.getMessage()); + + Map errors = new HashMap<>(); + + if (ex.getConstraintViolations() != null) { + for (ConstraintViolation violation : ex.getConstraintViolations()) { + String key = violation.getPropertyPath() != null ? violation.getPropertyPath().toString() : "error"; + errors.put(key, violation.getMessage()); + } + } + + return badRequest(errors); + } + + /** + * Builds the structured 400 response body shared by all validation handlers, preserving the + * response shape (success, code, message, errors). + * + * @param errors the collected validation errors keyed by field/object name + * @return a 400 ResponseEntity with the structured body + */ + private ResponseEntity> badRequest(Map errors) { + Map response = new HashMap<>(); + response.put("success", false); + response.put("code", 400); + response.put("message", "Validation failed"); + response.put("errors", errors); + return ResponseEntity.status(HttpStatus.BAD_REQUEST).body(response); + } + + /** + * Derives a stable key for a global (class-level) error. Falls back to the object name when no + * specific error code is available. + * + * @param error the global ObjectError + * @return a key for the errors map + */ + private String globalErrorKey(ObjectError error) { + if (error instanceof FieldError fieldError) { + return fieldError.getField(); + } + String code = error.getCode(); + return code != null ? code : error.getObjectName(); + } +} diff --git a/src/main/java/com/digitalsanctuary/spring/user/gdpr/ConsentAuditService.java b/src/main/java/com/digitalsanctuary/spring/user/gdpr/ConsentAuditService.java index 81618c36..98b7f247 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/gdpr/ConsentAuditService.java +++ b/src/main/java/com/digitalsanctuary/spring/user/gdpr/ConsentAuditService.java @@ -130,7 +130,7 @@ public ConsentRecord recordConsentGranted(User user, ConsentType consentType, St eventPublisher.publishEvent(auditEvent); // Publish consent changed event - eventPublisher.publishEvent(new ConsentChangedEvent(this, user, record, + eventPublisher.publishEvent(new ConsentChangedEvent(this, user.getId(), user.getEmail(), record, ConsentChangedEvent.ChangeType.GRANTED)); // Log consent type safely - avoid exposing custom type names which could contain PII @@ -210,7 +210,7 @@ public ConsentRecord recordConsentWithdrawn(User user, ConsentType consentType, eventPublisher.publishEvent(auditEvent); // Publish consent changed event - eventPublisher.publishEvent(new ConsentChangedEvent(this, user, record, + eventPublisher.publishEvent(new ConsentChangedEvent(this, user.getId(), user.getEmail(), record, ConsentChangedEvent.ChangeType.WITHDRAWN)); // Log consent type safely - avoid exposing custom type names which could contain PII diff --git a/src/main/java/com/digitalsanctuary/spring/user/gdpr/GdprDeletionService.java b/src/main/java/com/digitalsanctuary/spring/user/gdpr/GdprDeletionService.java index 2ac4b701..775df2f6 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/gdpr/GdprDeletionService.java +++ b/src/main/java/com/digitalsanctuary/spring/user/gdpr/GdprDeletionService.java @@ -173,7 +173,7 @@ protected DeletionResult executeUserDeletion(User user, GdprExportDTO exportedDa // Step 3: Publish UserPreDeleteEvent for additional cleanup log.debug("GdprDeletionService.deleteUser: Publishing UserPreDeleteEvent for user {}", userId); - eventPublisher.publishEvent(new UserPreDeleteEvent(this, user)); + eventPublisher.publishEvent(new UserPreDeleteEvent(this, userId, userEmail)); // Step 4: Delete framework-managed data deleteFrameworkData(user); diff --git a/src/main/java/com/digitalsanctuary/spring/user/gdpr/GdprExportService.java b/src/main/java/com/digitalsanctuary/spring/user/gdpr/GdprExportService.java index 0c89011f..dcc89807 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/gdpr/GdprExportService.java +++ b/src/main/java/com/digitalsanctuary/spring/user/gdpr/GdprExportService.java @@ -18,6 +18,7 @@ import com.digitalsanctuary.spring.user.persistence.model.User; import com.digitalsanctuary.spring.user.persistence.model.VerificationToken; import com.digitalsanctuary.spring.user.persistence.repository.PasswordResetTokenRepository; +import com.digitalsanctuary.spring.user.persistence.repository.UserRepository; import com.digitalsanctuary.spring.user.persistence.repository.VerificationTokenRepository; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; @@ -55,6 +56,7 @@ public class GdprExportService { private final AuditLogQueryService auditLogQueryService; private final VerificationTokenRepository verificationTokenRepository; private final PasswordResetTokenRepository passwordResetTokenRepository; + private final UserRepository userRepository; private final List dataContributors; private final ApplicationEventPublisher eventPublisher; private final ObjectMapper objectMapper; @@ -104,7 +106,11 @@ private GdprExportDTO.ExportMetadata buildMetadata() { * Builds the user data section. */ private GdprExportDTO.UserData buildUserData(User user) { - List roleNames = user.getRoles().stream() + // Roles are now LAZY-fetched and this export runs outside an open persistence session (by design, to avoid + // holding a transaction open during slow I/O). Reload the user through the entity-graph finder so roles are + // initialized in a single query, avoiding a LazyInitializationException on the detached principal. + User userWithRoles = user.getEmail() != null ? userRepository.findWithRolesByEmail(user.getEmail()) : null; + List roleNames = (userWithRoles != null ? userWithRoles.getRoles() : List.of()).stream() .map(Role::getName) .collect(Collectors.toList()); diff --git a/src/main/java/com/digitalsanctuary/spring/user/listener/RegistrationListener.java b/src/main/java/com/digitalsanctuary/spring/user/listener/RegistrationListener.java index 4123941c..5ebf7099 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/listener/RegistrationListener.java +++ b/src/main/java/com/digitalsanctuary/spring/user/listener/RegistrationListener.java @@ -47,9 +47,9 @@ public void onApplicationEvent(final OnRegistrationCompleteEvent event) { // ENABLED). Form registrations that require verification are created DISABLED, so they still receive // the email. This lets OAuth/OIDC services publish OnRegistrationCompleteEvent so consumers can // observe social registrations uniformly, without sending those users a pointless verification email. - if (event.getUser() != null && event.getUser().isEnabled()) { + if (event.isUserEnabled()) { log.debug("RegistrationListener.onApplicationEvent: user {} is already enabled; skipping verification email", - event.getUser().getEmail()); + event.getUserId()); return; } if (sendRegistrationVerificationEmail) { @@ -62,10 +62,16 @@ public void onApplicationEvent(final OnRegistrationCompleteEvent event) { * * Create a Verification token for the user, and send the email out. * + *

    + * The event carries only the user's id (not a live entity), so the email service reloads the {@link com.digitalsanctuary.spring.user.persistence.model.User} + * by id inside its own transaction. This avoids detached-entity / {@code LazyInitializationException} hazards on the + * async listener thread. + *

    + * * @param event the event */ private void sendRegistrationVerificationEmail(final OnRegistrationCompleteEvent event) { - userEmailService.sendRegistrationVerificationEmail(event.getUser(), event.getAppUrl()); + userEmailService.sendRegistrationVerificationEmail(event.getUserId(), event.getAppUrl()); } } diff --git a/src/main/java/com/digitalsanctuary/spring/user/mail/MailContentBuilder.java b/src/main/java/com/digitalsanctuary/spring/user/mail/MailContentBuilder.java index 7d9c85ee..5aa6b25b 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/mail/MailContentBuilder.java +++ b/src/main/java/com/digitalsanctuary/spring/user/mail/MailContentBuilder.java @@ -18,7 +18,7 @@ * * } */ -@Service +@Service("dsMailContentBuilder") public class MailContentBuilder { /** The template engine. */ diff --git a/src/main/java/com/digitalsanctuary/spring/user/persistence/model/PasswordHistoryEntry.java b/src/main/java/com/digitalsanctuary/spring/user/persistence/model/PasswordHistoryEntry.java index 90c8525f..36aee929 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/persistence/model/PasswordHistoryEntry.java +++ b/src/main/java/com/digitalsanctuary/spring/user/persistence/model/PasswordHistoryEntry.java @@ -13,13 +13,19 @@ import jakarta.persistence.ManyToOne; import jakarta.persistence.Table; -import lombok.Data; +import lombok.EqualsAndHashCode; +import lombok.Getter; +import lombok.Setter; +import lombok.ToString; /** * The PasswordHistoryEntry Entity. * Stores password hashes for a user to enforce password history policies. */ -@Data +@Getter +@Setter +@EqualsAndHashCode(onlyExplicitlyIncluded = true) +@ToString @Entity @Table(name = "password_history_entry", indexes = { @Index(name = "idx_user_id", columnList = "user_id"), @@ -30,14 +36,19 @@ public class PasswordHistoryEntry { /** The id. */ @Id @GeneratedValue(strategy = GenerationType.IDENTITY) + // Identity-based equality keys on this id only. Two transient (unsaved, id == null) instances compare equal, + // so never use unsaved entities as Set/Map keys; add them only after they have been persisted. See EntityEqualityTest. + @EqualsAndHashCode.Include private Long id; /** The user associated with this password entry. */ + @ToString.Exclude @ManyToOne(fetch = FetchType.LAZY) @JoinColumn(name = "user_id", nullable = false) private User user; /** The hashed password. */ + @ToString.Exclude @Column(length = 255, nullable = false) private String passwordHash; diff --git a/src/main/java/com/digitalsanctuary/spring/user/persistence/model/PasswordResetToken.java b/src/main/java/com/digitalsanctuary/spring/user/persistence/model/PasswordResetToken.java index e74b158a..4060ce63 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/persistence/model/PasswordResetToken.java +++ b/src/main/java/com/digitalsanctuary/spring/user/persistence/model/PasswordResetToken.java @@ -2,6 +2,7 @@ import java.util.Calendar; import java.util.Date; +import jakarta.persistence.Column; import jakarta.persistence.Entity; import jakarta.persistence.FetchType; import jakarta.persistence.GeneratedValue; @@ -9,12 +10,18 @@ import jakarta.persistence.Id; import jakarta.persistence.JoinColumn; import jakarta.persistence.OneToOne; -import lombok.Data; +import lombok.EqualsAndHashCode; +import lombok.Getter; +import lombok.Setter; +import lombok.ToString; /** * The PasswordResetToken Entity. */ -@Data +@Getter +@Setter +@EqualsAndHashCode(onlyExplicitlyIncluded = true) +@ToString @Entity public class PasswordResetToken { @@ -24,12 +31,18 @@ public class PasswordResetToken { /** The id. */ @Id @GeneratedValue(strategy = GenerationType.AUTO) + // Identity-based equality keys on this id only. Two transient (unsaved, id == null) instances compare equal, + // so never use unsaved entities as Set/Map keys; add them only after they have been persisted. See EntityEqualityTest. + @EqualsAndHashCode.Include private Long id; /** The token. */ + @ToString.Exclude + @Column(name = "token", nullable = false, unique = true) private String token; /** The user. */ + @ToString.Exclude @OneToOne(targetEntity = User.class, fetch = FetchType.EAGER) @JoinColumn(nullable = false, name = "user_id") private User user; diff --git a/src/main/java/com/digitalsanctuary/spring/user/persistence/model/Privilege.java b/src/main/java/com/digitalsanctuary/spring/user/persistence/model/Privilege.java index 9c99ab7e..95dc541e 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/persistence/model/Privilege.java +++ b/src/main/java/com/digitalsanctuary/spring/user/persistence/model/Privilege.java @@ -2,6 +2,7 @@ import java.io.Serializable; import java.util.Collection; +import jakarta.persistence.Column; import jakarta.persistence.Entity; import jakarta.persistence.GeneratedValue; import jakarta.persistence.GenerationType; @@ -32,6 +33,7 @@ public class Privilege implements Serializable { private Long id; /** The name. */ + @Column(unique = true, nullable = false) private String name; /** The description of the role. */ diff --git a/src/main/java/com/digitalsanctuary/spring/user/persistence/model/Role.java b/src/main/java/com/digitalsanctuary/spring/user/persistence/model/Role.java index 516414e0..100bacee 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/persistence/model/Role.java +++ b/src/main/java/com/digitalsanctuary/spring/user/persistence/model/Role.java @@ -4,6 +4,7 @@ import java.util.HashSet; import java.util.Set; import jakarta.persistence.CascadeType; +import jakarta.persistence.Column; import jakarta.persistence.Entity; import jakarta.persistence.FetchType; import jakarta.persistence.GeneratedValue; @@ -43,12 +44,14 @@ public class Role implements Serializable { private Set users = new HashSet<>(); /** The privileges. */ - @ManyToMany(cascade = {CascadeType.PERSIST, CascadeType.MERGE}, fetch = FetchType.EAGER) + @ToString.Exclude + @ManyToMany(cascade = {CascadeType.PERSIST, CascadeType.MERGE}, fetch = FetchType.LAZY) @JoinTable(name = "roles_privileges", joinColumns = @JoinColumn(name = "role_id", referencedColumnName = "id"), inverseJoinColumns = @JoinColumn(name = "privilege_id", referencedColumnName = "id")) private Set privileges = new HashSet<>(); /** The name. */ + @Column(unique = true, nullable = false) private String name; private String description; diff --git a/src/main/java/com/digitalsanctuary/spring/user/persistence/model/User.java b/src/main/java/com/digitalsanctuary/spring/user/persistence/model/User.java index 2f9360c9..0d662efa 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/persistence/model/User.java +++ b/src/main/java/com/digitalsanctuary/spring/user/persistence/model/User.java @@ -10,8 +10,9 @@ import org.springframework.data.annotation.LastModifiedDate; import org.springframework.data.jpa.domain.support.AuditingEntityListener; import jakarta.persistence.*; -import lombok.Data; import lombok.EqualsAndHashCode; +import lombok.Getter; +import lombok.Setter; import lombok.ToString; /** @@ -24,7 +25,10 @@ * distributed sessions must ensure any custom profile or data reachable from the session-stored principal is also {@link Serializable}. *

    */ -@Data +@Getter +@Setter +@EqualsAndHashCode(onlyExplicitlyIncluded = true) +@ToString @Entity @EntityListeners(AuditingEntityListener.class) @Table(name = "user_account") @@ -67,6 +71,9 @@ public enum Provider { @Id @Column(unique = true, nullable = false) @GeneratedValue(strategy = GenerationType.AUTO) + // Identity-based equality keys on this id only. Two transient (unsaved, id == null) instances compare equal, + // so never use unsaved entities as Set/Map keys; add them only after they have been persisted. See EntityEqualityTest. + @EqualsAndHashCode.Include private Long id; /** The first name. */ @@ -110,15 +117,13 @@ public enum Provider { /** The roles - stored as Set to avoid Hibernate immutable collection issues */ @ToString.Exclude - @EqualsAndHashCode.Exclude - @ManyToMany(cascade = {CascadeType.PERSIST, CascadeType.MERGE}, fetch = FetchType.EAGER) + @ManyToMany(cascade = {CascadeType.PERSIST, CascadeType.MERGE}, fetch = FetchType.LAZY) @JoinTable(name = "users_roles", joinColumns = @JoinColumn(name = "user_id", referencedColumnName = "id"), inverseJoinColumns = @JoinColumn(name = "role_id", referencedColumnName = "id")) private Set roles = new HashSet<>(); /** The password history entries. */ @ToString.Exclude - @EqualsAndHashCode.Exclude @OneToMany(mappedBy = "user", cascade = CascadeType.ALL, orphanRemoval = true, fetch = FetchType.LAZY) private List passwordHistoryEntries = new ArrayList<>(); diff --git a/src/main/java/com/digitalsanctuary/spring/user/persistence/model/VerificationToken.java b/src/main/java/com/digitalsanctuary/spring/user/persistence/model/VerificationToken.java index 0aa5f589..134298c4 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/persistence/model/VerificationToken.java +++ b/src/main/java/com/digitalsanctuary/spring/user/persistence/model/VerificationToken.java @@ -2,6 +2,7 @@ import java.util.Calendar; import java.util.Date; +import jakarta.persistence.Column; import jakarta.persistence.Entity; import jakarta.persistence.FetchType; import jakarta.persistence.ForeignKey; @@ -11,12 +12,18 @@ import jakarta.persistence.JoinColumn; import jakarta.persistence.OneToOne; import jakarta.persistence.Transient; -import lombok.Data; +import lombok.EqualsAndHashCode; +import lombok.Getter; +import lombok.Setter; +import lombok.ToString; /** * The VerificationToken Entity. Stores Registration Verification Token data. */ -@Data +@Getter +@Setter +@EqualsAndHashCode(onlyExplicitlyIncluded = true) +@ToString @Entity public class VerificationToken { @@ -26,12 +33,18 @@ public class VerificationToken { /** The id. */ @Id @GeneratedValue(strategy = GenerationType.AUTO) + // Identity-based equality keys on this id only. Two transient (unsaved, id == null) instances compare equal, + // so never use unsaved entities as Set/Map keys; add them only after they have been persisted. See EntityEqualityTest. + @EqualsAndHashCode.Include private Long id; /** The token. */ + @ToString.Exclude + @Column(name = "token", nullable = false, unique = true) private String token; /** The user. */ + @ToString.Exclude @OneToOne(targetEntity = User.class, fetch = FetchType.EAGER) @JoinColumn(nullable = false, name = "user_id", foreignKey = @ForeignKey(name = "FK_VERIFY_USER")) private User user; @@ -45,6 +58,7 @@ public class VerificationToken { * email link can be built) when a service regenerates a token. It is {@code null} on entities * loaded from the database. */ + @ToString.Exclude @Transient private transient String plainToken; diff --git a/src/main/java/com/digitalsanctuary/spring/user/persistence/model/WebAuthnCredential.java b/src/main/java/com/digitalsanctuary/spring/user/persistence/model/WebAuthnCredential.java index 0fe7f724..f1b3b5df 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/persistence/model/WebAuthnCredential.java +++ b/src/main/java/com/digitalsanctuary/spring/user/persistence/model/WebAuthnCredential.java @@ -8,24 +8,32 @@ import jakarta.persistence.ManyToOne; import jakarta.persistence.Table; import java.time.Instant; -import lombok.Data; +import lombok.EqualsAndHashCode; +import lombok.Getter; +import lombok.Setter; +import lombok.ToString; import org.hibernate.Length; /** * JPA entity for the {@code user_credentials} table. Stores WebAuthn credentials (public keys) for passkey * authentication. The {@code credentialId} is stored as a Base64url string matching Spring Security's convention. */ -@Data +@Getter +@Setter +@EqualsAndHashCode(onlyExplicitlyIncluded = true) +@ToString @Entity @Table(name = "user_credentials") public class WebAuthnCredential { /** Credential ID as Base64url string (matches Spring Security's storage convention). */ @Id + @EqualsAndHashCode.Include @Column(name = "credential_id", length = 512) private String credentialId; /** FK to the WebAuthn user entity. */ + @ToString.Exclude @ManyToOne(fetch = FetchType.LAZY) @JoinColumn(name = "user_entity_user_id", nullable = false) private WebAuthnUserEntity userEntity; @@ -39,6 +47,7 @@ public class WebAuthnCredential { * MariaDB DDL failure described in GitHub issue #286. Do not replace with {@code @Lob}, which maps * to {@code OID} on PostgreSQL.

    */ + @ToString.Exclude @Column(name = "public_key", nullable = false, length = Length.LONG32) private byte[] publicKey; @@ -71,6 +80,7 @@ public class WebAuthnCredential { * *

    See {@link #publicKey} for why {@code length = Length.LONG32} is used here.

    */ + @ToString.Exclude @Column(name = "attestation_object", length = Length.LONG32) private byte[] attestationObject; @@ -79,6 +89,7 @@ public class WebAuthnCredential { * *

    See {@link #publicKey} for why {@code length = Length.LONG32} is used here.

    */ + @ToString.Exclude @Column(name = "attestation_client_data_json", length = Length.LONG32) private byte[] attestationClientDataJson; diff --git a/src/main/java/com/digitalsanctuary/spring/user/persistence/model/WebAuthnUserEntity.java b/src/main/java/com/digitalsanctuary/spring/user/persistence/model/WebAuthnUserEntity.java index f42ae716..eaa32568 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/persistence/model/WebAuthnUserEntity.java +++ b/src/main/java/com/digitalsanctuary/spring/user/persistence/model/WebAuthnUserEntity.java @@ -7,19 +7,28 @@ import jakarta.persistence.JoinColumn; import jakarta.persistence.ManyToOne; import jakarta.persistence.Table; -import lombok.Data; +import lombok.EqualsAndHashCode; +import lombok.Getter; +import lombok.Setter; +import lombok.ToString; /** * JPA entity for the {@code user_entities} table. Maps WebAuthn user handles to application users. The {@code id} is * the Base64url-encoded WebAuthn user handle. */ -@Data +@Getter +@Setter +@EqualsAndHashCode(onlyExplicitlyIncluded = true) +@ToString @Entity @Table(name = "user_entities") public class WebAuthnUserEntity { /** Base64url-encoded WebAuthn user handle. */ @Id + // Identity-based equality keys on this id only. Two instances with a null id compare equal, + // so never use unsaved entities as Set/Map keys; add them only after the id is assigned. See EntityEqualityTest. + @EqualsAndHashCode.Include private String id; /** Username (email). */ @@ -31,6 +40,7 @@ public class WebAuthnUserEntity { private String displayName; /** FK to the application user. */ + @ToString.Exclude @ManyToOne(fetch = FetchType.LAZY) @JoinColumn(name = "user_account_id", nullable = false) private User user; diff --git a/src/main/java/com/digitalsanctuary/spring/user/persistence/repository/UserRepository.java b/src/main/java/com/digitalsanctuary/spring/user/persistence/repository/UserRepository.java index 03638a00..eb5ab136 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/persistence/repository/UserRepository.java +++ b/src/main/java/com/digitalsanctuary/spring/user/persistence/repository/UserRepository.java @@ -1,6 +1,7 @@ package com.digitalsanctuary.spring.user.persistence.repository; import java.util.List; +import org.springframework.data.jpa.repository.EntityGraph; import org.springframework.data.jpa.repository.JpaRepository; import org.springframework.data.jpa.repository.Modifying; import org.springframework.data.jpa.repository.Query; @@ -20,6 +21,23 @@ public interface UserRepository extends JpaRepository { */ User findByEmail(String email); + /** + * Find by email, eagerly loading the user's roles and each role's privileges in a single query via an entity graph. + * + *

    This is the finder used on the authentication path (see {@code DSUserDetailsService}). Because {@code User.roles} + * and {@code Role.privileges} are now {@link jakarta.persistence.FetchType#LAZY}, callers that must traverse + * roles/privileges after the persistence session closes (e.g. building Spring Security authorities for a detached + * principal) must load the user through this method. The {@code @EntityGraph} ensures the full + * User → roles → privileges graph is initialized in one round trip, avoiding both the N+1 problem and a + * {@code LazyInitializationException}. The plain {@link #findByEmail(String)} remains for callers (token lookups, + * existence checks, lockout counters) that do not need the authority graph.

    + * + * @param email the email + * @return the user with roles and privileges initialized, or {@code null} if none found + */ + @EntityGraph(attributePaths = {"roles", "roles.privileges"}) + User findWithRolesByEmail(String email); + /** * Atomically increments the failed login attempt counter for the user with the given email. * diff --git a/src/main/java/com/digitalsanctuary/spring/user/roles/RolePrivilegeSetupService.java b/src/main/java/com/digitalsanctuary/spring/user/roles/RolePrivilegeSetupService.java index 951348df..5b7f8d64 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/roles/RolePrivilegeSetupService.java +++ b/src/main/java/com/digitalsanctuary/spring/user/roles/RolePrivilegeSetupService.java @@ -4,34 +4,46 @@ import java.util.List; import java.util.Map; import java.util.Set; +import org.springframework.beans.factory.annotation.Autowired; import org.springframework.context.ApplicationListener; +import org.springframework.context.annotation.Lazy; import org.springframework.context.event.ContextRefreshedEvent; +import org.springframework.dao.DataIntegrityViolationException; import org.springframework.stereotype.Component; +import org.springframework.transaction.annotation.Propagation; +import org.springframework.transaction.annotation.Transactional; import com.digitalsanctuary.spring.user.persistence.model.Privilege; import com.digitalsanctuary.spring.user.persistence.model.Role; import com.digitalsanctuary.spring.user.persistence.repository.PrivilegeRepository; import com.digitalsanctuary.spring.user.persistence.repository.RoleRepository; -import jakarta.transaction.Transactional; -import lombok.Data; -import lombok.RequiredArgsConstructor; +import lombok.Getter; +import lombok.Setter; import lombok.extern.slf4j.Slf4j; /** * Service that initializes roles and privileges from configuration on application startup. *

    - * Listens for {@link ContextRefreshedEvent} and creates or updates roles and privileges - * in the database based on the {@link RolesAndPrivilegesConfig} settings. This ensures - * that the configured role/privilege structure exists before the application handles requests. + * Listens for {@link ContextRefreshedEvent} and creates or updates roles and privileges in the database based on the + * {@link RolesAndPrivilegesConfig} settings. This ensures that the configured role/privilege structure exists before + * the application handles requests. + *

    + *

    + * The setup is idempotent and safe under concurrent multi-node startup. Because {@code role.name} and + * {@code privilege.name} carry a UNIQUE constraint, a second node inserting the same name will fail with a + * {@link DataIntegrityViolationException}. Each insert runs in its own {@link Propagation#REQUIRES_NEW} transaction + * (routed through the Spring proxy via the {@link #self} reference) so that a failed insert rolls back only that inner + * transaction without poisoning the surrounding flow; the catch handler then re-reads the row the winning node created, + * giving first-writer-wins convergence. *

    */ @Slf4j -@Data -@RequiredArgsConstructor -@Component +@Getter +@Component("dsRolePrivilegeSetupService") public class RolePrivilegeSetupService implements ApplicationListener { - /** The already setup flag. */ - private boolean alreadySetup = false; + /** The already setup flag. {@code volatile} so the guard is visible across threads under concurrent context refresh (e.g. parallel test execution). */ + @Setter + private volatile boolean alreadySetup = false; /** The roles and privileges configuration. */ private final RolesAndPrivilegesConfig rolesAndPrivilegesConfig; @@ -42,13 +54,42 @@ public class RolePrivilegeSetupService implements ApplicationListener + * Intentionally not {@code @Transactional}: each {@code getOrCreate} call manages its own + * transaction boundary so that a unique-constraint violation on one insert cannot mark a shared transaction + * rollback-only and poison the subsequent re-read. + *

    * * @param event the context refreshed event */ @Override - @Transactional public void onApplicationEvent(final ContextRefreshedEvent event) { if (alreadySetup) { return; @@ -60,47 +101,150 @@ public void onApplicationEvent(final ContextRefreshedEvent event) { final String roleName = entry.getKey(); final List privileges = entry.getValue(); if (roleName != null && privileges != null) { - final Set privilegeSet = new HashSet<>(); for (String privilegeName : privileges) { - privilegeSet.add(getOrCreatePrivilege(privilegeName)); + if (privilegeName != null && !privilegeName.isBlank()) { + getOrCreatePrivilege(privilegeName); + } else { + log.warn("RolePrivilegeSetupService: skipping null/blank privilege name in role '{}'", roleName); + } } - getOrCreateRole(roleName, privilegeSet); + // Pass only non-null/non-blank privilege names to the role, matching what was persisted above. + Set validPrivileges = privileges.stream() + .filter(p -> p != null && !p.isBlank()) + .collect(java.util.stream.Collectors.toSet()); + getOrCreateRole(roleName, validPrivileges); } } alreadySetup = true; } /** - * Retrieves or creates a privilege by name. + * Retrieves or creates a privilege by name, tolerating concurrent creation by another node. + *

    + * If the privilege does not yet exist it is inserted in its own {@link Propagation#REQUIRES_NEW} transaction. If a + * concurrent node has already inserted a privilege with the same name, the insert fails with a + * {@link DataIntegrityViolationException}; that inner transaction is rolled back and the row created by the winning + * node is re-read and returned. The exception is never propagated to the caller. + *

    * * @param name the name of the privilege - * @return the privilege + * @return the existing or newly created privilege */ - @Transactional - Privilege getOrCreatePrivilege(final String name) { + public Privilege getOrCreatePrivilege(final String name) { Privilege privilege = privilegeRepository.findByName(name); - if (privilege == null) { - privilege = new Privilege(name); - privilege = privilegeRepository.save(privilege); + if (privilege != null) { + return privilege; + } + try { + return self.insertPrivilege(name); + } catch (final DataIntegrityViolationException e) { + log.debug("Privilege '{}' was created concurrently; re-reading existing row.", name); + final Privilege existing = privilegeRepository.findByName(name); + if (existing == null) { + throw e; + } + return existing; + } + } + + /** + * Inserts a new privilege in its own transaction. Must be {@code public} so the Spring proxy can advise it when + * invoked through {@link #self}; a package-private/private target is not overridden by the CGLIB proxy subclass and + * the {@link Propagation#REQUIRES_NEW} boundary would not take effect. + * + * @param name the name of the privilege + * @return the persisted privilege + */ + @Transactional(propagation = Propagation.REQUIRES_NEW) + public Privilege insertPrivilege(final String name) { + return privilegeRepository.saveAndFlush(new Privilege(name)); + } + + /** + * Retrieves or creates a role by name and (re)assigns its privileges, tolerating concurrent creation by another + * node. + *

    + * If the role does not yet exist it is inserted in its own {@link Propagation#REQUIRES_NEW} transaction. If a + * concurrent node has already inserted a role with the same name, the insert fails with a + * {@link DataIntegrityViolationException}; that inner transaction is rolled back and the existing row is re-read, + * has its privileges (re)assigned, and is saved. The exception is never propagated to the caller. + *

    + * + * @param name the name of the role + * @param privilegeNames the names of the privileges associated with the role (resolved to managed entities inside + * the insert/update transaction) + * @return the existing or newly created role + */ + public Role getOrCreateRole(final String name, final Set privilegeNames) { + final Role existing = roleRepository.findByName(name); + if (existing != null) { + return self.updateRolePrivileges(existing.getId(), privilegeNames); + } + try { + return self.insertRole(name, privilegeNames); + } catch (final DataIntegrityViolationException e) { + log.debug("Role '{}' was created concurrently; re-reading existing row.", name); + final Role concurrent = roleRepository.findByName(name); + if (concurrent == null) { + throw e; + } + return self.updateRolePrivileges(concurrent.getId(), privilegeNames); } - return privilege; } /** - * Retrieves or creates a role by name and privileges. + * Inserts a new role with the given privileges in its own transaction. The privileges are resolved to managed + * entities by name within this transaction so the role's cascade does not attempt to persist detached instances. + * Must be {@code public} so the Spring proxy can advise it when invoked through {@link #self} (see + * {@link #insertPrivilege(String)}). * * @param name the name of the role - * @param privileges the set of privileges associated with the role - * @return the role + * @param privilegeNames the names of the privileges to assign + * @return the persisted role */ - @Transactional - Role getOrCreateRole(final String name, final Set privileges) { - Role role = roleRepository.findByName(name); - if (role == null) { - role = new Role(name); + @Transactional(propagation = Propagation.REQUIRES_NEW) + public Role insertRole(final String name, final Set privilegeNames) { + final Role role = new Role(name); + role.setPrivileges(resolvePrivileges(privilegeNames)); + return roleRepository.saveAndFlush(role); + } + + /** + * Re-reads an existing role by id and (re)assigns its privileges in its own transaction. The privileges are + * resolved to managed entities by name within this transaction. Must be {@code public} so the Spring proxy can + * advise it when invoked through {@link #self} (see {@link #insertPrivilege(String)}). + * + * @param roleId the id of the role to update + * @param privilegeNames the names of the privileges to assign + * @return the updated role + */ + @Transactional(propagation = Propagation.REQUIRES_NEW) + public Role updateRolePrivileges(final Long roleId, final Set privilegeNames) { + final Role role = roleRepository.findById(roleId).orElseThrow(); + role.setPrivileges(resolvePrivileges(privilegeNames)); + return roleRepository.saveAndFlush(role); + } + + /** + * Resolves a set of privilege names to managed {@link Privilege} entities loaded in the current transaction. Names + * with no matching row are skipped (they were already created by {@link #getOrCreatePrivilege(String)} earlier in + * the setup flow, so a miss here only happens transiently and is harmless). + * + * @param privilegeNames the privilege names to resolve + * @return the managed privileges + */ + private Set resolvePrivileges(final Set privilegeNames) { + final Set privileges = new HashSet<>(); + for (final String privilegeName : privilegeNames) { + final Privilege privilege = privilegeRepository.findByName(privilegeName); + if (privilege != null) { + privileges.add(privilege); + } else { + log.warn("Configured privilege '{}' was not found and will be absent from the role's privilege set. " + + "This is usually a typo in the user.roles-and-privileges configuration, or a transient miss " + + "during concurrent multi-node startup.", privilegeName); + } } - role.setPrivileges(privileges); - role = roleRepository.save(role); - return role; + return privileges; } } diff --git a/src/main/java/com/digitalsanctuary/spring/user/security/UserSecurityBeansAutoConfiguration.java b/src/main/java/com/digitalsanctuary/spring/user/security/UserSecurityBeansAutoConfiguration.java index 34c506a3..e703a689 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/security/UserSecurityBeansAutoConfiguration.java +++ b/src/main/java/com/digitalsanctuary/spring/user/security/UserSecurityBeansAutoConfiguration.java @@ -1,5 +1,6 @@ package com.digitalsanctuary.spring.user.security; +import java.util.List; import org.springframework.beans.factory.annotation.Value; import org.springframework.boot.autoconfigure.AutoConfiguration; import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; @@ -20,6 +21,7 @@ import org.springframework.security.web.session.HttpSessionEventPublisher; import com.digitalsanctuary.spring.user.UserConfiguration; import com.digitalsanctuary.spring.user.roles.RolesAndPrivilegesConfig; +import com.digitalsanctuary.spring.user.util.AppUrlResolver; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; @@ -166,4 +168,21 @@ public HttpSessionEventPublisher httpSessionEventPublisher() { public AuthenticationEventPublisher authenticationEventPublisher(ApplicationEventPublisher applicationEventPublisher) { return new DefaultAuthenticationEventPublisher(applicationEventPublisher); } + + /** + * Creates the library's default {@link AppUrlResolver}, which builds the base URL for security-sensitive email links (password reset, email + * verification) from the configured canonical URL ({@code user.security.appUrl}) and/or the trusted-host allow-list + * ({@code user.security.trustedHosts}), defending against Host-header / X-Forwarded-Host poisoning (CWE-640). Backs off entirely if the consuming + * application defines its own {@link AppUrlResolver}. + * + * @param appUrl the configured canonical base URL, or {@code null} when unset + * @param trustedHosts the allow-listed forwarded hosts (empty when unset) + * @return the default {@link AppUrlResolver} + */ + @Bean + @ConditionalOnMissingBean(AppUrlResolver.class) + public AppUrlResolver appUrlResolver(@Value("${user.security.appUrl:#{null}}") String appUrl, + @Value("${user.security.trustedHosts:}") List trustedHosts) { + return new AppUrlResolver(appUrl, trustedHosts); + } } diff --git a/src/main/java/com/digitalsanctuary/spring/user/service/AuthorityService.java b/src/main/java/com/digitalsanctuary/spring/user/service/AuthorityService.java index f992f07f..fe9a49e4 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/service/AuthorityService.java +++ b/src/main/java/com/digitalsanctuary/spring/user/service/AuthorityService.java @@ -25,7 +25,7 @@ */ @Slf4j @RequiredArgsConstructor -@Service +@Service("dsAuthorityService") @Transactional public class AuthorityService { diff --git a/src/main/java/com/digitalsanctuary/spring/user/service/DSOAuth2UserService.java b/src/main/java/com/digitalsanctuary/spring/user/service/DSOAuth2UserService.java index 3cd10f5a..176dc64f 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/service/DSOAuth2UserService.java +++ b/src/main/java/com/digitalsanctuary/spring/user/service/DSOAuth2UserService.java @@ -90,7 +90,7 @@ public User handleOAuthLoginSuccess(String registrationId, OAuth2User oAuth2User "Unable to retrieve email address from " + registrationId + ". Please ensure you have granted email permissions."); } log.debug("handleOAuthLoginSuccess: looking up user with email: {}", user.getEmail()); - User existingUser = userRepository.findByEmail(user.getEmail().toLowerCase()); + User existingUser = userRepository.findWithRolesByEmail(user.getEmail().toLowerCase()); log.debug("handleOAuthLoginSuccess: existingUser: {}", existingUser); if (existingUser != null && registrationId != null) { log.debug("handleOAuthLoginSuccess: existingUser.getProvider(): {}", existingUser.getProvider()); @@ -145,7 +145,8 @@ private User registerNewOAuthUser(String registrationId, User user) { // RegistrationListener intentionally skips sending them a verification email; the event still fires. // No HttpServletRequest is available here, so locale defaults and appUrl is null (only the verification // email, which is skipped for enabled users, would have used appUrl). - eventPublisher.publishEvent(new OnRegistrationCompleteEvent(savedUser, Locale.getDefault(), null)); + eventPublisher.publishEvent(OnRegistrationCompleteEvent.builder().userId(savedUser.getId()).userEmail(savedUser.getEmail()) + .userEnabled(savedUser.isEnabled()).locale(Locale.getDefault()).appUrl(null).build()); return savedUser; } diff --git a/src/main/java/com/digitalsanctuary/spring/user/service/DSOidcUserService.java b/src/main/java/com/digitalsanctuary/spring/user/service/DSOidcUserService.java index 676c2e25..b70f97f6 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/service/DSOidcUserService.java +++ b/src/main/java/com/digitalsanctuary/spring/user/service/DSOidcUserService.java @@ -96,7 +96,7 @@ public User handleOidcLoginSuccess(String registrationId, OidcUser oidcUser) { // but we normalize again here defensively in case additional sources are added. String normalizedEmail = user.getEmail().trim().toLowerCase(Locale.ROOT); log.debug("handleOidcLoginSuccess: looking up user with email: {}", normalizedEmail); - User existingUser = userRepository.findByEmail(normalizedEmail); + User existingUser = userRepository.findWithRolesByEmail(normalizedEmail); log.debug("handleOidcLoginSuccess: existingUser: {}", existingUser); if (existingUser != null && registrationId != null) { log.debug("handleOidcLoginSuccess: existingUser.getProvider(): {}", existingUser.getProvider()); @@ -151,7 +151,8 @@ private User registerNewOidcUser(String registrationId, User user) { // RegistrationListener intentionally skips sending them a verification email; the event still fires. // No HttpServletRequest is available here, so locale defaults and appUrl is null (only the verification // email, which is skipped for enabled users, would have used appUrl). - eventPublisher.publishEvent(new OnRegistrationCompleteEvent(savedUser, Locale.getDefault(), null)); + eventPublisher.publishEvent(OnRegistrationCompleteEvent.builder().userId(savedUser.getId()).userEmail(savedUser.getEmail()) + .userEnabled(savedUser.isEnabled()).locale(Locale.getDefault()).appUrl(null).build()); return savedUser; } diff --git a/src/main/java/com/digitalsanctuary/spring/user/service/DSUserDetailsService.java b/src/main/java/com/digitalsanctuary/spring/user/service/DSUserDetailsService.java index 30574462..41d0d8fd 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/service/DSUserDetailsService.java +++ b/src/main/java/com/digitalsanctuary/spring/user/service/DSUserDetailsService.java @@ -20,7 +20,7 @@ */ @Slf4j @RequiredArgsConstructor -@Service +@Service("dsUserDetailsService") @Transactional public class DSUserDetailsService implements UserDetailsService { @@ -42,7 +42,9 @@ public class DSUserDetailsService implements UserDetailsService { @Override public DSUserDetails loadUserByUsername(final String email) throws UsernameNotFoundException { log.debug("DSUserDetailsService.loadUserByUsername: called with username: {}", email); - User dbUser = userRepository.findByEmail(email); + // Use the entity-graph finder so roles and privileges are initialized in a single query before the + // principal is detached, allowing AuthorityService to build authorities without a LazyInitializationException. + User dbUser = userRepository.findWithRolesByEmail(email); if (dbUser == null) { throw new UsernameNotFoundException("No user found with email/username: " + email); } diff --git a/src/main/java/com/digitalsanctuary/spring/user/service/LoginAttemptService.java b/src/main/java/com/digitalsanctuary/spring/user/service/LoginAttemptService.java index abb00212..81c89232 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/service/LoginAttemptService.java +++ b/src/main/java/com/digitalsanctuary/spring/user/service/LoginAttemptService.java @@ -26,7 +26,7 @@ */ @Slf4j @RequiredArgsConstructor -@Service +@Service("dsLoginAttemptService") @Data public class LoginAttemptService { diff --git a/src/main/java/com/digitalsanctuary/spring/user/service/PasswordPolicyService.java b/src/main/java/com/digitalsanctuary/spring/user/service/PasswordPolicyService.java index 3fab1958..a62362ef 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/service/PasswordPolicyService.java +++ b/src/main/java/com/digitalsanctuary/spring/user/service/PasswordPolicyService.java @@ -54,7 +54,7 @@ */ @Slf4j @RequiredArgsConstructor -@Service +@Service("dsPasswordPolicyService") public class PasswordPolicyService { @Value("${user.security.password.enabled}") diff --git a/src/main/java/com/digitalsanctuary/spring/user/service/SessionInvalidationService.java b/src/main/java/com/digitalsanctuary/spring/user/service/SessionInvalidationService.java index a3ae6c32..65bf24f1 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/service/SessionInvalidationService.java +++ b/src/main/java/com/digitalsanctuary/spring/user/service/SessionInvalidationService.java @@ -35,7 +35,7 @@ */ @Slf4j @RequiredArgsConstructor -@Service +@Service("dsSessionInvalidationService") public class SessionInvalidationService { private final SessionRegistry sessionRegistry; diff --git a/src/main/java/com/digitalsanctuary/spring/user/service/UserEmailService.java b/src/main/java/com/digitalsanctuary/spring/user/service/UserEmailService.java index cfe4e488..c08f7fa7 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/service/UserEmailService.java +++ b/src/main/java/com/digitalsanctuary/spring/user/service/UserEmailService.java @@ -22,6 +22,7 @@ import com.digitalsanctuary.spring.user.persistence.model.PasswordResetToken; import com.digitalsanctuary.spring.user.persistence.model.User; import com.digitalsanctuary.spring.user.persistence.repository.PasswordResetTokenRepository; +import com.digitalsanctuary.spring.user.persistence.repository.UserRepository; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; @@ -42,7 +43,7 @@ */ @Slf4j @RequiredArgsConstructor -@Service +@Service("dsUserEmailService") public class UserEmailService { /** The mail service. */ @@ -54,6 +55,9 @@ public class UserEmailService { /** The password token repository. */ private final PasswordResetTokenRepository passwordTokenRepository; + /** The user repository, used to reload a User by id for async-dispatched registration emails. */ + private final UserRepository userRepository; + /** The event publisher. */ private final ApplicationEventPublisher eventPublisher; @@ -135,6 +139,32 @@ public void sendRegistrationVerificationEmail(final User user, final String appU mailService.sendTemplateMessage(user.getEmail(), "Registration Confirmation", variables, "mail/registration-token.html"); } + /** + * Sends the registration verification email for the user with the given id. + * + *

    This overload reloads the {@link User} from the repository inside its own transaction. It is used by the + * {@code @Async} registration listener, which (as of 5.0.0) receives only the user's id rather than a live JPA + * entity, avoiding detached-entity / {@code LazyInitializationException} hazards across threads. If the user no + * longer exists (e.g. deleted before the async dispatch ran), the call is a no-op.

    + * + * @param userId the id of the user to send the verification email to + * @param appUrl the app url (must be a valid HTTP/HTTPS URL) + * @throws IllegalArgumentException if appUrl is null, blank, or uses a dangerous scheme + */ + @Transactional + public void sendRegistrationVerificationEmail(final Long userId, final String appUrl) { + if (userId == null) { + log.warn("UserEmailService.sendRegistrationVerificationEmail: null userId; skipping verification email"); + return; + } + User user = userRepository.findById(userId).orElse(null); + if (user == null) { + log.warn("UserEmailService.sendRegistrationVerificationEmail: user {} no longer exists; skipping verification email", userId); + return; + } + sendRegistrationVerificationEmail(user, appUrl); + } + /** * Creates the email variables. * diff --git a/src/main/java/com/digitalsanctuary/spring/user/service/UserService.java b/src/main/java/com/digitalsanctuary/spring/user/service/UserService.java index f20b8f79..a05451c4 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/service/UserService.java +++ b/src/main/java/com/digitalsanctuary/spring/user/service/UserService.java @@ -157,7 +157,7 @@ * @author Devon Hillard */ @Slf4j -@Service +@Service("dsUserService") @RequiredArgsConstructor @Transactional public class UserService { @@ -231,8 +231,6 @@ public String getValue() { /** The user verification service. */ public final UserVerificationService userVerificationService; - private final AuthorityService authorityService; - /** The user details service. */ private final DSUserDetailsService dsUserDetailsService; @@ -511,7 +509,7 @@ public void deleteOrDisableUser(final User user) { // Publish the UserPreDeleteEvent before deleting the user // This allows any listeners to perform actions before the user is deleted log.debug("Publishing UserPreDeleteEvent"); - eventPublisher.publishEvent(new UserPreDeleteEvent(this, user)); + eventPublisher.publishEvent(new UserPreDeleteEvent(this, userId, userEmail)); // Clean up any Tokens associated with this user final VerificationToken verificationToken = tokenRepository.findByUser(user); @@ -1036,8 +1034,10 @@ public void authWithoutPassword(User user) { return; } - // Generate authorities from user roles and privileges - Collection authorities = authorityService.getAuthoritiesFromUser(user); + // Reuse the authorities already resolved by loadUserByUsername (which loads roles and privileges via the + // entity-graph finder). The incoming `user` may be detached, so deriving authorities from it directly could + // trigger a LazyInitializationException now that roles/privileges are lazily fetched. + Collection authorities = userDetails.getAuthorities(); // Authenticate user authenticateUser(userDetails, authorities); diff --git a/src/main/java/com/digitalsanctuary/spring/user/util/AppUrlResolver.java b/src/main/java/com/digitalsanctuary/spring/user/util/AppUrlResolver.java new file mode 100644 index 00000000..063ad59e --- /dev/null +++ b/src/main/java/com/digitalsanctuary/spring/user/util/AppUrlResolver.java @@ -0,0 +1,148 @@ +package com.digitalsanctuary.spring.user.util; + +import java.util.List; +import jakarta.servlet.http.HttpServletRequest; +import lombok.extern.slf4j.Slf4j; + +/** + * Resolves the application base URL used to build security-sensitive email links (password reset, email verification), defending against Host-header / + * X-Forwarded-Host poisoning (CWE-640). + * + *

    + * Resolution order: + *

      + *
    1. If {@code user.security.appUrl} is configured, always use it (forwarded headers ignored).
      2. + *
    2. Otherwise build from the request, honoring {@code X-Forwarded-*} only when the resolved host is in {@code user.security.trustedHosts}; + * otherwise use the container's own server name (the untrusted forwarded host is ignored and a warning is logged).
      3. + *
    + * + *

    + * The request-derived URL preserves the historical {@code UserUtils.getAppUrl} format with one refinement: the {@code :port} suffix is omitted when the + * port is the default for the scheme (80 for {@code http}, 443 for {@code https}), and the context path is appended only when non-empty. This keeps + * link formatting stable for existing deployments while producing clean canonical URLs. + */ +@Slf4j +public class AppUrlResolver { + + private static final int DEFAULT_HTTP_PORT = 80; + private static final int DEFAULT_HTTPS_PORT = 443; + + private final String configuredAppUrl; + private final List trustedHosts; + + /** + * Creates a resolver. + * + * @param configuredAppUrl the canonical base URL ({@code user.security.appUrl}); blank/null means "not configured" + * @param trustedHosts the allow-listed forwarded hosts ({@code user.security.trustedHosts}); null is treated as empty + */ + public AppUrlResolver(String configuredAppUrl, List trustedHosts) { + String trimmed = (configuredAppUrl == null || configuredAppUrl.isBlank()) ? null : configuredAppUrl.trim(); + // Strip any trailing slash to honour the "no trailing slash" contract on resolveAppUrl's return value. + // Without this, appUrl + "/user/..." produces double slashes when the consumer misconfigures a trailing slash. + this.configuredAppUrl = (trimmed != null && trimmed.endsWith("/")) ? trimmed.replaceAll("/+$", "") : trimmed; + this.trustedHosts = trustedHosts == null ? List.of() : trustedHosts.stream().map(String::trim).toList(); + } + + /** + * Resolves the application base URL for the given request. + * + * @param request the current HTTP request + * @return the resolved base URL (no trailing slash) + */ + public String resolveAppUrl(HttpServletRequest request) { + if (configuredAppUrl != null) { + return configuredAppUrl; + } + + // X-Forwarded-Host may be a comma-separated list when the request traverses multiple proxies + // (RFC 7230); the client-facing host is the first value. Match the allow-list against that value + // only, otherwise legitimate multi-proxy chains (e.g. ALB + nginx) never match and silently fall + // back to the container's server name. + String fwdHost = firstHeaderValue(request.getHeader("X-Forwarded-Host")); + boolean useForwarded = fwdHost != null && !fwdHost.isEmpty() && trustedHosts.contains(stripPort(fwdHost)); + if (fwdHost != null && !fwdHost.isEmpty() && !useForwarded) { + log.warn("AppUrlResolver: ignoring untrusted X-Forwarded-Host '{}' (not in user.security.trustedHosts)", sanitizeForLog(fwdHost)); + } + + String scheme = useForwarded ? forwardedScheme(request) : request.getScheme(); + String host = useForwarded ? stripPort(fwdHost) : request.getServerName(); + int port = useForwarded ? forwardedPort(request, scheme) : request.getServerPort(); + + StringBuilder url = new StringBuilder(); + url.append(scheme).append("://").append(host); + if (!isDefaultPort(scheme, port)) { + url.append(':').append(port); + } + String contextPath = request.getContextPath(); + if (contextPath != null && !contextPath.isEmpty()) { + url.append(contextPath); + } + return url.toString(); + } + + private static int forwardedPort(HttpServletRequest request, String forwardedScheme) { + String portHeader = firstHeaderValue(request.getHeader("X-Forwarded-Port")); + if (portHeader != null && !portHeader.isBlank()) { + try { + return Integer.parseInt(portHeader.trim()); + } catch (NumberFormatException e) { + log.warn("AppUrlResolver: ignoring non-numeric X-Forwarded-Port '{}'", sanitizeForLog(portHeader)); + } + } + // No usable X-Forwarded-Port: derive the port from the forwarded scheme so we don't leak the + // container's internal port (e.g. 8080) into the email link. Default ports are omitted later. + return "https".equalsIgnoreCase(forwardedScheme) ? DEFAULT_HTTPS_PORT : DEFAULT_HTTP_PORT; + } + + /** + * Resolves the forwarded scheme from {@code X-Forwarded-Proto}, accepting only {@code http} or {@code https}. A trusted proxy is expected to send a + * sane value, but a misconfigured or compromised one sending e.g. {@code javascript} must never be allowed to flow into a security-sensitive email + * link, so any unrecognized value falls back to the container's own scheme. + */ + private static String forwardedScheme(HttpServletRequest request) { + String proto = firstHeaderValue(request.getHeader("X-Forwarded-Proto")); + if (proto == null || proto.isEmpty()) { + return request.getScheme(); + } + if ("http".equalsIgnoreCase(proto) || "https".equalsIgnoreCase(proto)) { + return proto; + } + log.warn("AppUrlResolver: ignoring invalid X-Forwarded-Proto '{}', falling back to request scheme", sanitizeForLog(proto)); + return request.getScheme(); + } + + /** + * Returns the first value of a possibly comma-separated forwarded header (RFC 7230), trimmed. Returns {@code null} for a null input. + */ + private static String firstHeaderValue(String headerValue) { + if (headerValue == null) { + return null; + } + int comma = headerValue.indexOf(','); + String first = comma >= 0 ? headerValue.substring(0, comma) : headerValue; + return first.trim(); + } + + /** + * Neutralizes CR/LF/tab in attacker-controlled header values before logging to prevent log-injection / forging. + */ + private static String sanitizeForLog(String value) { + return value == null ? null : value.replaceAll("[\r\n\t]", "_"); + } + + private static boolean isDefaultPort(String scheme, int port) { + return ("http".equalsIgnoreCase(scheme) && port == DEFAULT_HTTP_PORT) + || ("https".equalsIgnoreCase(scheme) && port == DEFAULT_HTTPS_PORT); + } + + private static String stripPort(String host) { + if (host.startsWith("[")) { + // IPv6 literal: [::1] or [::1]:8443 + int bracket = host.indexOf(']'); + return bracket > 0 ? host.substring(0, bracket + 1) : host; + } + int colon = host.lastIndexOf(':'); + return colon > 0 ? host.substring(0, colon) : host; + } +} diff --git a/src/main/java/com/digitalsanctuary/spring/user/util/UserUtils.java b/src/main/java/com/digitalsanctuary/spring/user/util/UserUtils.java index b659b5a9..9c526a9a 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/util/UserUtils.java +++ b/src/main/java/com/digitalsanctuary/spring/user/util/UserUtils.java @@ -50,7 +50,13 @@ public static String getClientIP(HttpServletRequest request) { * * @param request The HttpServletRequest object. * @return The application URL as a String. + * @deprecated This method trusts {@code X-Forwarded-Host} unconditionally, which makes security-sensitive email links (password reset, email + * verification) vulnerable to Host-header / X-Forwarded-Host poisoning (CWE-640). Use + * {@link com.digitalsanctuary.spring.user.util.AppUrlResolver#resolveAppUrl(HttpServletRequest)} instead, which builds the base URL from + * a configured canonical URL ({@code user.security.appUrl}) and/or a trusted-host allow-list ({@code user.security.trustedHosts}). + * Scheduled for removal in a future release. */ + @Deprecated(forRemoval = true) public static String getAppUrl(HttpServletRequest request) { // Check for forwarded protocol String scheme = request.getHeader("X-Forwarded-Proto"); diff --git a/src/main/resources/META-INF/additional-spring-configuration-metadata.json b/src/main/resources/META-INF/additional-spring-configuration-metadata.json index 09914cfa..248d87b7 100644 --- a/src/main/resources/META-INF/additional-spring-configuration-metadata.json +++ b/src/main/resources/META-INF/additional-spring-configuration-metadata.json @@ -36,6 +36,16 @@ "type": "java.lang.String", "description": "Cron expression for token purge schedule" }, + { + "name": "user.security.appUrl", + "type": "java.lang.String", + "description": "Canonical base URL for security email links (password reset, verification). STRONGLY recommended in production to prevent Host-header poisoning (CWE-640). When set, X-Forwarded-Host is ignored." + }, + { + "name": "user.security.trustedHosts", + "type": "java.util.List", + "description": "When user.security.appUrl is not set, X-Forwarded-Host is honored only for hosts in this comma-separated allowlist; otherwise the container's own server name is used." + }, { "name": "user.security.loginActionURI", "type": "java.lang.String", @@ -301,6 +311,30 @@ "name": "user.web.globalUserModelOptIn", "type": "java.lang.Boolean", "description": "Add user to model by default (true) or only with annotation (false)" + }, + { + "name": "user.async.enabled", + "type": "java.lang.Boolean", + "description": "Enable the library's asynchronous method execution support (@EnableAsync). Set to false if the consuming application already enables async processing globally to avoid double-activation.", + "defaultValue": true + }, + { + "name": "user.retry.enabled", + "type": "java.lang.Boolean", + "description": "Enable the library's Spring Retry support (@EnableRetry). Set to false if the consuming application already enables retry globally to avoid double-activation.", + "defaultValue": true + }, + { + "name": "user.scheduling.enabled", + "type": "java.lang.Boolean", + "description": "Enable the library's scheduled task execution (@EnableScheduling), used by token purge and similar jobs. Set to false if the consuming application already enables scheduling globally to avoid double-activation.", + "defaultValue": true + }, + { + "name": "user.method-security.enabled", + "type": "java.lang.Boolean", + "description": "Enable the library's Spring Security method-level security (@EnableMethodSecurity). Set to false if the consuming application already enables method security globally to avoid double-activation.", + "defaultValue": true } ] } \ No newline at end of file diff --git a/src/main/resources/META-INF/spring/org.springframework.boot.env.EnvironmentPostProcessor.imports b/src/main/resources/META-INF/spring/org.springframework.boot.env.EnvironmentPostProcessor.imports new file mode 100644 index 00000000..5b8e3f8d --- /dev/null +++ b/src/main/resources/META-INF/spring/org.springframework.boot.env.EnvironmentPostProcessor.imports @@ -0,0 +1 @@ +com.digitalsanctuary.spring.user.MessageSourceEnvironmentPostProcessor diff --git a/src/main/resources/config/dsspringuserconfig.properties b/src/main/resources/config/dsspringuserconfig.properties index 7582f512..2e5cb603 100644 --- a/src/main/resources/config/dsspringuserconfig.properties +++ b/src/main/resources/config/dsspringuserconfig.properties @@ -7,9 +7,6 @@ spring.data.jpa.repositories.packages=com.digitalsanctuary.spring.user.persisten spring.jpa.entity.packages=com.digitalsanctuary.spring.user.persistence.model -# Spring Configuration Overrides -spring.messages.basename=messages/messages,messages/dsspringusermessages - # DigitalSanctuary Spring User Configuration # User Audit Log Configuration @@ -81,6 +78,11 @@ user.security.bcryptStrength=12 # user.security.tokenHashSecret= # The lifetime, in minutes, of a password reset token before it expires. Default is 1440 (24 hours). user.security.passwordResetTokenValidityMinutes=1440 +# Canonical base URL for security email links (password reset, verification). STRONGLY recommended in production +# to prevent Host-header poisoning (CWE-640). When set, X-Forwarded-Host is ignored. +user.security.appUrl= +# When appUrl is not set, X-Forwarded-Host is honored only for hosts in this comma-separated allowlist. +user.security.trustedHosts= # If true, the test hash time will be logged to the console on startup. This is useful for determining the optimal bcryptStrength value. user.security.testHashTime=true # The default action for all requests. This can be either deny or allow. diff --git a/src/test/java/com/digitalsanctuary/spring/user/MessageSourceEnvironmentPostProcessorTest.java b/src/test/java/com/digitalsanctuary/spring/user/MessageSourceEnvironmentPostProcessorTest.java new file mode 100644 index 00000000..8792a83b --- /dev/null +++ b/src/test/java/com/digitalsanctuary/spring/user/MessageSourceEnvironmentPostProcessorTest.java @@ -0,0 +1,84 @@ +package com.digitalsanctuary.spring.user; + +import static org.assertj.core.api.Assertions.assertThat; +import java.util.Locale; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; +import org.springframework.boot.autoconfigure.AutoConfigurations; +import org.springframework.boot.autoconfigure.context.MessageSourceAutoConfiguration; +import org.springframework.boot.test.context.runner.ApplicationContextRunner; +import org.springframework.context.MessageSource; + +/** + * Verifies that {@link MessageSourceEnvironmentPostProcessor} registers the library's message bundle ADDITIVELY, preserving the consuming application's + * own {@code spring.messages.basename} (or Spring Boot's conventional default) rather than overriding it. + */ +@DisplayName("MessageSourceEnvironmentPostProcessor Tests") +class MessageSourceEnvironmentPostProcessorTest { + + @Nested + @DisplayName("Basename merge logic") + class MergeLogic { + + @Test + @DisplayName("should preserve Spring Boot default and append library bundle when basename is unset") + void shouldPreserveDefaultWhenUnset() { + assertThat(MessageSourceEnvironmentPostProcessor.mergeBasename(null)).isEqualTo("messages,messages/dsspringusermessages"); + assertThat(MessageSourceEnvironmentPostProcessor.mergeBasename("")).isEqualTo("messages,messages/dsspringusermessages"); + assertThat(MessageSourceEnvironmentPostProcessor.mergeBasename(" ")).isEqualTo("messages,messages/dsspringusermessages"); + } + + @Test + @DisplayName("should preserve a single consumer basename and append library bundle last") + void shouldPreserveSingleConsumerBasename() { + assertThat(MessageSourceEnvironmentPostProcessor.mergeBasename("i18n/app")) + .isEqualTo("i18n/app,messages/dsspringusermessages"); + } + + @Test + @DisplayName("should preserve multiple consumer basenames in order with library bundle last") + void shouldPreserveMultipleConsumerBasenames() { + assertThat(MessageSourceEnvironmentPostProcessor.mergeBasename("messages,i18n/labels")) + .isEqualTo("messages,i18n/labels,messages/dsspringusermessages"); + } + + @Test + @DisplayName("should trim whitespace around consumer basenames") + void shouldTrimWhitespace() { + assertThat(MessageSourceEnvironmentPostProcessor.mergeBasename(" messages , i18n/app ")) + .isEqualTo("messages,i18n/app,messages/dsspringusermessages"); + } + + @Test + @DisplayName("should de-duplicate and keep the library bundle last if the consumer already lists it") + void shouldDeduplicateLibraryBundle() { + assertThat(MessageSourceEnvironmentPostProcessor.mergeBasename("messages/dsspringusermessages,messages")) + .isEqualTo("messages,messages/dsspringusermessages"); + } + } + + @Nested + @DisplayName("Additive registration through MessageSource") + class AdditiveRegistration { + + // Boots only MessageSourceAutoConfiguration, then applies the merged basename the post-processor would produce for a consumer who points at + // their own bundle (test-messages/consumer-bundle). Both the consumer key and the library key must resolve. + private final ApplicationContextRunner contextRunner = new ApplicationContextRunner() + .withConfiguration(AutoConfigurations.of(MessageSourceAutoConfiguration.class)) + .withPropertyValues("spring.messages.basename=" + + MessageSourceEnvironmentPostProcessor.mergeBasename("test-messages/consumer-bundle")); + + @Test + @DisplayName("should resolve BOTH a consumer-bundle key AND a library-bundle key") + void shouldResolveConsumerAndLibraryKeys() { + contextRunner.run(context -> { + MessageSource messageSource = context.getBean(MessageSource.class); + assertThat(messageSource.getMessage("consumer.test.key", null, Locale.ENGLISH)) + .as("consumer's own bundle must still resolve").isEqualTo("Consumer bundle value"); + assertThat(messageSource.getMessage("email.forgot-password.prompt", null, Locale.ENGLISH)) + .as("library bundle must also resolve").isEqualTo("Reset your password"); + }); + } + } +} diff --git a/src/test/java/com/digitalsanctuary/spring/user/UserConfigurationToggleTest.java b/src/test/java/com/digitalsanctuary/spring/user/UserConfigurationToggleTest.java new file mode 100644 index 00000000..ca2e2fa3 --- /dev/null +++ b/src/test/java/com/digitalsanctuary/spring/user/UserConfigurationToggleTest.java @@ -0,0 +1,87 @@ +package com.digitalsanctuary.spring.user; + +import static org.assertj.core.api.Assertions.assertThat; + +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.springframework.boot.test.context.runner.ApplicationContextRunner; + +/** + * Verifies that the cross-cutting {@code @Enable*} support enabled by {@link UserConfiguration} is + * individually toggleable and defaults to on (backward compatible). + * + *

    + * The four nested gating configurations are registered directly (rather than the full scanning + * {@link UserConfiguration} entry point) so their {@code @ConditionalOnProperty(matchIfMissing = true)} + * gates can be exercised in isolation. Registering the full {@code UserConfiguration} here would trigger + * its broad {@code @ComponentScan("com.digitalsanctuary.spring.user")}, which in the test classpath also + * picks up unrelated test fixtures and conflicts with other tests' contexts. The gating predicates are + * identical regardless of how the nested config is registered, and the full + * {@code @IntegrationTest}/{@code @SecurityTest} suite remains the oracle for end-to-end wiring with the + * real entry point. + */ +@DisplayName("UserConfiguration Cross-Cutting Toggle Tests") +class UserConfigurationToggleTest { + + private final ApplicationContextRunner runner = new ApplicationContextRunner().withUserConfiguration( + UserConfiguration.AsyncConfiguration.class, UserConfiguration.RetryConfiguration.class, + UserConfiguration.SchedulingConfiguration.class, UserConfiguration.MethodSecurityConfiguration.class); + + @Test + @DisplayName("Defaults (no properties set): all nested cross-cutting configs are active") + void defaultsActivateAllCrossCuttingConfigs() { + runner.run(ctx -> { + assertThat(ctx).hasSingleBean(UserConfiguration.AsyncConfiguration.class); + assertThat(ctx).hasSingleBean(UserConfiguration.RetryConfiguration.class); + assertThat(ctx).hasSingleBean(UserConfiguration.SchedulingConfiguration.class); + assertThat(ctx).hasSingleBean(UserConfiguration.MethodSecurityConfiguration.class); + }); + } + + @Test + @DisplayName("user.method-security.enabled=false: method security nested config does not activate") + void methodSecurityToggleOff() { + runner.withPropertyValues("user.method-security.enabled=false").run(ctx -> { + assertThat(ctx).doesNotHaveBean(UserConfiguration.MethodSecurityConfiguration.class); + // Other cross-cutting configs remain active and independent. + assertThat(ctx).hasSingleBean(UserConfiguration.AsyncConfiguration.class); + assertThat(ctx).hasSingleBean(UserConfiguration.RetryConfiguration.class); + assertThat(ctx).hasSingleBean(UserConfiguration.SchedulingConfiguration.class); + }); + } + + @Test + @DisplayName("user.method-security.enabled=true (explicit): method security nested config activates") + void methodSecurityToggleOnExplicit() { + runner.withPropertyValues("user.method-security.enabled=true") + .run(ctx -> assertThat(ctx).hasSingleBean(UserConfiguration.MethodSecurityConfiguration.class)); + } + + @Test + @DisplayName("user.scheduling.enabled=false: scheduling nested config does not activate") + void schedulingToggleOff() { + runner.withPropertyValues("user.scheduling.enabled=false").run(ctx -> { + assertThat(ctx).doesNotHaveBean(UserConfiguration.SchedulingConfiguration.class); + assertThat(ctx).hasSingleBean(UserConfiguration.AsyncConfiguration.class); + }); + } + + @Test + @DisplayName("user.async.enabled=false: async nested config does not activate") + void asyncToggleOff() { + runner.withPropertyValues("user.async.enabled=false").run(ctx -> { + assertThat(ctx).doesNotHaveBean(UserConfiguration.AsyncConfiguration.class); + assertThat(ctx).hasSingleBean(UserConfiguration.RetryConfiguration.class); + }); + } + + @Test + @DisplayName("user.retry.enabled=false: retry nested config does not activate") + void retryToggleOff() { + runner.withPropertyValues("user.retry.enabled=false").run(ctx -> { + assertThat(ctx).doesNotHaveBean(UserConfiguration.RetryConfiguration.class); + // Other cross-cutting configs remain active and independent. + assertThat(ctx).hasSingleBean(UserConfiguration.AsyncConfiguration.class); + }); + } +} diff --git a/src/test/java/com/digitalsanctuary/spring/user/api/UserAPIRegistrationGuardTest.java b/src/test/java/com/digitalsanctuary/spring/user/api/UserAPIRegistrationGuardTest.java index e26c8938..2fdc003e 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/api/UserAPIRegistrationGuardTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/api/UserAPIRegistrationGuardTest.java @@ -2,6 +2,8 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; @@ -28,11 +30,14 @@ import com.digitalsanctuary.spring.user.dto.PasswordlessRegistrationDto; import com.digitalsanctuary.spring.user.dto.UserDto; +import com.digitalsanctuary.spring.user.event.OnRegistrationCompleteEvent; +import com.digitalsanctuary.spring.user.exceptions.UserAlreadyExistException; import com.digitalsanctuary.spring.user.persistence.model.User; import com.digitalsanctuary.spring.user.registration.RegistrationDeniedException; import com.digitalsanctuary.spring.user.service.PasswordPolicyService; import com.digitalsanctuary.spring.user.service.UserEmailService; import com.digitalsanctuary.spring.user.service.UserService; +import com.digitalsanctuary.spring.user.util.AppUrlResolver; import com.digitalsanctuary.spring.user.service.WebAuthnCredentialManagementService; import com.digitalsanctuary.spring.user.test.builders.UserTestDataBuilder; import com.fasterxml.jackson.databind.ObjectMapper; @@ -67,6 +72,9 @@ class UserAPIRegistrationGuardTest { @Mock private WebAuthnCredentialManagementService webAuthnService; + @Mock + private AppUrlResolver appUrlResolver; + @InjectMocks private UserAPI userAPI; @@ -190,5 +198,33 @@ void shouldAllowPasswordlessRegistrationWhenGuardAllows() throws Exception { .andExpect(status().isOk()) .andExpect(jsonPath("$.success").value(true)); } + + @Test + @DisplayName("Should return uniform success response for existing email (anti-enumeration, no 409)") + void shouldReturnUniformResponseForExistingEmail() throws Exception { + PasswordlessRegistrationDto dto = new PasswordlessRegistrationDto(); + dto.setEmail("existing@example.com"); + dto.setFirstName("Test"); + dto.setLastName("User"); + + when(webAuthnCredentialManagementServiceProvider.getIfAvailable()).thenReturn(webAuthnService); + // The email is already registered: the service signals it via UserAlreadyExistException. + when(userService.registerPasswordlessAccount(any(PasswordlessRegistrationDto.class))) + .thenThrow(new UserAlreadyExistException("User already exists")); + + // Response is indistinguishable from a brand-new passwordless registration: HTTP 200, + // success-shaped body, redirect to the pending URI. No longer a 409 that would enumerate. + mockMvc.perform(post("/user/registration/passwordless") + .contentType(MediaType.APPLICATION_JSON) + .content(objectMapper.writeValueAsString(dto)) + .with(csrf())) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.success").value(true)) + .andExpect(jsonPath("$.code").value(0)) + .andExpect(jsonPath("$.redirectUrl").value("/user/registration-pending.html")); + + // No new account is created: no registration event is published. + verify(eventPublisher, never()).publishEvent(any(OnRegistrationCompleteEvent.class)); + } } } diff --git a/src/test/java/com/digitalsanctuary/spring/user/api/UserAPIUnitTest.java b/src/test/java/com/digitalsanctuary/spring/user/api/UserAPIUnitTest.java index 62be1f0d..420a6411 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/api/UserAPIUnitTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/api/UserAPIUnitTest.java @@ -1,13 +1,22 @@ package com.digitalsanctuary.spring.user.api; +import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.argThat; import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.Mockito.*; -import static org.assertj.core.api.Assertions.assertThat; -import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.*; -import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.*; -import static org.springframework.test.web.servlet.result.MockMvcResultHandlers.print; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; +import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.csrf; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.delete; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +import java.util.Collections; +import java.util.Locale; import com.digitalsanctuary.spring.user.audit.AuditEvent; import com.digitalsanctuary.spring.user.dto.PasswordDto; @@ -22,9 +31,9 @@ import com.digitalsanctuary.spring.user.service.UserEmailService; import com.digitalsanctuary.spring.user.service.UserService; import com.digitalsanctuary.spring.user.test.builders.UserTestDataBuilder; +import com.digitalsanctuary.spring.user.util.AppUrlResolver; import com.digitalsanctuary.spring.user.util.JSONResponse; import com.fasterxml.jackson.databind.ObjectMapper; - import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Nested; @@ -36,22 +45,16 @@ import org.mockito.junit.jupiter.MockitoExtension; import org.springframework.context.ApplicationEventPublisher; import org.springframework.context.MessageSource; +import org.springframework.http.HttpStatus; import org.springframework.http.MediaType; +import org.springframework.security.web.method.annotation.AuthenticationPrincipalArgumentResolver; import org.springframework.test.util.ReflectionTestUtils; import org.springframework.test.web.servlet.MockMvc; -import org.springframework.test.web.servlet.MvcResult; import org.springframework.test.web.servlet.setup.MockMvcBuilders; -import org.springframework.web.method.support.HandlerMethodArgumentResolver; -import org.springframework.security.web.method.annotation.AuthenticationPrincipalArgumentResolver; +import org.springframework.validation.beanvalidation.LocalValidatorFactoryBean; import org.springframework.web.bind.annotation.ExceptionHandler; import org.springframework.web.bind.annotation.RestControllerAdvice; -import org.springframework.http.HttpStatus; -import org.springframework.validation.beanvalidation.LocalValidatorFactoryBean; - -import java.util.Collections; -import java.util.Locale; - -import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.csrf; +import org.springframework.web.method.support.HandlerMethodArgumentResolver; @ExtendWith(MockitoExtension.class) @DisplayName("UserAPI Unit Tests") @@ -91,6 +94,9 @@ public JSONResponse handleSecurityException(SecurityException e) { @Mock private PasswordPolicyService passwordPolicyService; + @Mock + private AppUrlResolver appUrlResolver; + @InjectMocks private UserAPI userAPI; @@ -151,16 +157,16 @@ void registerUserAccount_success_withVerificationEmail() throws Exception { .thenReturn(Collections.emptyList()); // When & Then - MvcResult result = mockMvc.perform(post("/user/registration") + mockMvc.perform(post("/user/registration") .contentType(MediaType.APPLICATION_JSON) .content(objectMapper.writeValueAsString(testUserDto)) .with(csrf())) .andExpect(status().isOk()) .andExpect(jsonPath("$.success").value(true)) .andExpect(jsonPath("$.code").value(0)) - .andExpect(jsonPath("$.messages[0]").value("Registration Successful!")) - .andExpect(jsonPath("$.redirectUrl").value("/user/registration-pending.html")) - .andReturn(); + .andExpect(jsonPath("$.messages[0]") + .value("If your email address is eligible, you will receive a verification email shortly.")) + .andExpect(jsonPath("$.redirectUrl").value("/user/registration-pending.html")); // Verify event publishing verify(eventPublisher, times(2)).publishEvent(any()); @@ -169,7 +175,8 @@ void registerUserAccount_success_withVerificationEmail() throws Exception { ArgumentCaptor registrationCaptor = ArgumentCaptor.forClass(OnRegistrationCompleteEvent.class); verify(eventPublisher).publishEvent(registrationCaptor.capture()); OnRegistrationCompleteEvent registrationEvent = registrationCaptor.getValue(); - assertThat(registrationEvent.getUser()).isEqualTo(newUser); + assertThat(registrationEvent.getUserEmail()).isEqualTo(newUser.getEmail()); + assertThat(registrationEvent.isUserEnabled()).isEqualTo(newUser.isEnabled()); ArgumentCaptor auditCaptor = ArgumentCaptor.forClass(AuditEvent.class); verify(eventPublisher).publishEvent(auditCaptor.capture()); @@ -205,23 +212,29 @@ void registerUserAccount_success_withAutoLogin() throws Exception { } @Test - @DisplayName("POST /user/registration - user already exists") - void registerUserAccount_userAlreadyExists() throws Exception { - // Given + @DisplayName("POST /user/registration - existing email returns the same uniform 200 body as a new registration") + void registerUserAccount_existingEmail_returnsUniformResponse() throws Exception { + // Given - the service signals the email is already registered when(userService.registerNewUserAccount(any(UserDto.class))) .thenThrow(new UserAlreadyExistException("User already exists")); when(passwordPolicyService.validate(any(), anyString(), anyString(), any(Locale.class))) .thenReturn(Collections.emptyList()); - // When & Then + // When & Then - response is indistinguishable from a brand-new registration mockMvc.perform(post("/user/registration") .contentType(MediaType.APPLICATION_JSON) .content(objectMapper.writeValueAsString(testUserDto)) .with(csrf())) - .andExpect(status().isConflict()) - .andExpect(jsonPath("$.success").value(false)) - .andExpect(jsonPath("$.code").value(2)) - .andExpect(jsonPath("$.messages[0]").value("An account already exists for the email address")); + .andExpect(status().isOk()) + .andExpect(jsonPath("$.success").value(true)) + .andExpect(jsonPath("$.code").value(0)) + .andExpect(jsonPath("$.messages[0]") + .value("If your email address is eligible, you will receive a verification email shortly.")) + .andExpect(jsonPath("$.redirectUrl").value("/user/registration-pending.html")); + + // No new account is created: no registration event is published and no auto-login occurs. + verify(eventPublisher, never()).publishEvent(any(OnRegistrationCompleteEvent.class)); + verify(userService, never()).authWithoutPassword(any()); } @Test @@ -286,6 +299,7 @@ void resendRegistrationToken_success() throws Exception { .disabled() .build(); when(userService.findUserByEmail(testUserDto.getEmail())).thenReturn(unverifiedUser); + when(appUrlResolver.resolveAppUrl(any())).thenReturn("http://localhost:8080"); // When & Then mockMvc.perform(post("/user/resendRegistrationToken") @@ -294,42 +308,51 @@ void resendRegistrationToken_success() throws Exception { .with(csrf())) .andExpect(status().isOk()) .andExpect(jsonPath("$.success").value(true)) - .andExpect(jsonPath("$.messages[0]").value("Verification Email Resent Successfully!")); + .andExpect(jsonPath("$.code").value(0)) + .andExpect(jsonPath("$.messages[0]") + .value("If your account requires verification, a new verification email has been sent.")); verify(userEmailService).sendRegistrationVerificationEmail(eq(unverifiedUser), anyString()); } @Test - @DisplayName("POST /user/resendRegistrationToken - account already verified") - void resendRegistrationToken_alreadyVerified() throws Exception { - // Given + @DisplayName("POST /user/resendRegistrationToken - already-verified account returns the same uniform 200 body") + void resendRegistrationToken_alreadyVerified_returnsUniformResponse() throws Exception { + // Given - an existing, already-enabled (verified) account when(userService.findUserByEmail(testUserDto.getEmail())).thenReturn(testUser); // enabled user - // When & Then + // When & Then - same response as the unverified case, and no email is sent mockMvc.perform(post("/user/resendRegistrationToken") .contentType(MediaType.APPLICATION_JSON) .content(objectMapper.writeValueAsString(testUserDto)) .with(csrf())) - .andExpect(status().isConflict()) - .andExpect(jsonPath("$.success").value(false)) - .andExpect(jsonPath("$.code").value(1)) - .andExpect(jsonPath("$.messages[0]").value("Account is already verified.")); + .andExpect(status().isOk()) + .andExpect(jsonPath("$.success").value(true)) + .andExpect(jsonPath("$.code").value(0)) + .andExpect(jsonPath("$.messages[0]") + .value("If your account requires verification, a new verification email has been sent.")); + + verify(userEmailService, never()).sendRegistrationVerificationEmail(any(User.class), anyString()); } @Test - @DisplayName("POST /user/resendRegistrationToken - user not found") - void resendRegistrationToken_userNotFound() throws Exception { - // Given + @DisplayName("POST /user/resendRegistrationToken - unknown email returns the same uniform 200 body (no 500 leak)") + void resendRegistrationToken_unknownEmail_returnsUniformResponse() throws Exception { + // Given - no account exists for the email when(userService.findUserByEmail(testUserDto.getEmail())).thenReturn(null); - // When & Then + // When & Then - same uniform 200 response; nothing leaks existence mockMvc.perform(post("/user/resendRegistrationToken") .contentType(MediaType.APPLICATION_JSON) .content(objectMapper.writeValueAsString(testUserDto)) .with(csrf())) - .andExpect(status().isInternalServerError()) - .andExpect(jsonPath("$.success").value(false)) - .andExpect(jsonPath("$.code").value(2)); + .andExpect(status().isOk()) + .andExpect(jsonPath("$.success").value(true)) + .andExpect(jsonPath("$.code").value(0)) + .andExpect(jsonPath("$.messages[0]") + .value("If your account requires verification, a new verification email has been sent.")); + + verify(userEmailService, never()).sendRegistrationVerificationEmail(any(User.class), anyString()); } } @@ -342,6 +365,7 @@ class PasswordManagementTests { void resetPassword_success() throws Exception { // Given when(userService.findUserByEmail(testUserDto.getEmail())).thenReturn(testUser); + when(appUrlResolver.resolveAppUrl(any())).thenReturn("http://localhost:8080"); // When & Then mockMvc.perform(post("/user/resetPassword") diff --git a/src/test/java/com/digitalsanctuary/spring/user/api/UserApiTest.java b/src/test/java/com/digitalsanctuary/spring/user/api/UserApiTest.java index f269140b..44385ee5 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/api/UserApiTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/api/UserApiTest.java @@ -215,26 +215,32 @@ void shouldRegisterNewUser() throws Exception { .andExpect(status().isOk()) .andExpect(jsonPath("$.success").value(true)) .andExpect(jsonPath("$.code").value(0)) - .andExpect(jsonPath("$.messages[0]").value("Registration Successful!")); + .andExpect(jsonPath("$.messages[0]") + .value("If your email address is eligible, you will receive a verification email shortly.")); assertThat(userService.findUserByEmail(testEmail)).isNotNull(); } @Test - @DisplayName("Should return 409 Conflict when the user already exists") - void shouldRejectExistingUser() throws Exception { + @DisplayName("Should return the same uniform 200 body for an existing email (anti-enumeration) and create no duplicate") + void shouldReturnUniformResponseForExistingUser() throws Exception { // Register once. userService.registerNewUserAccount(baseTestUser); + Long existingId = userService.findUserByEmail(testEmail).getId(); - // Register again with the same email. + // Register again with the same email - response must be indistinguishable from a new registration. mockMvc.perform(post(URL + "/registration") .with(csrf()) .contentType(MediaType.APPLICATION_JSON) .content(json(baseTestUser))) - .andExpect(status().isConflict()) - .andExpect(jsonPath("$.success").value(false)) - .andExpect(jsonPath("$.code").value(2)) - .andExpect(jsonPath("$.messages[0]").value("An account already exists for the email address")); + .andExpect(status().isOk()) + .andExpect(jsonPath("$.success").value(true)) + .andExpect(jsonPath("$.code").value(0)) + .andExpect(jsonPath("$.messages[0]") + .value("If your email address is eligible, you will receive a verification email shortly.")); + + // No duplicate account was created - the existing account is untouched. + assertThat(userService.findUserByEmail(testEmail).getId()).isEqualTo(existingId); } @Test @@ -248,6 +254,32 @@ void shouldRejectInvalidUser() throws Exception { .content(json(new UserDto()))) .andExpect(status().isBadRequest()); } + + @Test + @DisplayName("Should return 400 with a structured error body when password and matchingPassword mismatch (class-level @PasswordMatches)") + void shouldReturnBadRequestWhenPasswordsDoNotMatch() throws Exception { + // The class-level @PasswordMatches constraint produces a GLOBAL (not field) binding error. + // The library's validation advice must surface this as a structured 400 - not a 500. + UserDto mismatched = new UserDto(); + mismatched.setFirstName("Api"); + mismatched.setLastName("Tester"); + mismatched.setEmail(testEmail); + mismatched.setPassword(VALID_PASSWORD); + mismatched.setMatchingPassword(NEW_VALID_PASSWORD); + + mockMvc.perform(post(URL + "/registration") + .with(csrf()) + .contentType(MediaType.APPLICATION_JSON) + .content(json(mismatched))) + .andExpect(status().isBadRequest()) + .andExpect(jsonPath("$.success").value(false)) + .andExpect(jsonPath("$.code").value(400)) + .andExpect(jsonPath("$.message").value("Validation failed")) + .andExpect(jsonPath("$.errors").isNotEmpty()); + + // The mismatched registration must not have created an account. + assertThat(userService.findUserByEmail(testEmail)).isNull(); + } } @Nested diff --git a/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnFeatureEnabledIntegrationTest.java b/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnFeatureEnabledIntegrationTest.java index b1b76908..bfe33ec1 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnFeatureEnabledIntegrationTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnFeatureEnabledIntegrationTest.java @@ -17,6 +17,7 @@ import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.http.MediaType; +import org.springframework.security.crypto.password.PasswordEncoder; import org.springframework.test.context.TestPropertySource; import org.springframework.test.web.servlet.MockMvc; import org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping; @@ -36,6 +37,8 @@ class WebAuthnFeatureEnabledIntegrationTest { private static final String TEST_EMAIL = "webauthn-user@test.com"; + private static final String TEST_PASSWORD = "currentPassword1!"; + @Autowired private MockMvc mockMvc; @@ -54,6 +57,9 @@ class WebAuthnFeatureEnabledIntegrationTest { @Autowired private WebAuthnCredentialManagementService credentialManagementService; + @Autowired + private PasswordEncoder passwordEncoder; + @BeforeEach void setUp() { webAuthnCredentialRepository.deleteAll(); @@ -65,7 +71,7 @@ void setUp() { user.setFirstName("Web"); user.setLastName("Authn"); user.setEnabled(true); - user.setPassword("encoded-password"); + user.setPassword(passwordEncoder.encode(TEST_PASSWORD)); User savedUser = userRepository.saveAndFlush(user); WebAuthnUserEntity userEntity = new WebAuthnUserEntity(); @@ -123,8 +129,56 @@ void shouldReturnValidationResponseForOverlongLabel() throws Exception { @Test @DisplayName("should return business error response when service throws WebAuthnException") void shouldReturnBusinessErrorWhenServiceFails() throws Exception { - mockMvc.perform(delete("/user/webauthn/credentials/does-not-exist").with(user(TEST_EMAIL).roles("USER")).with(csrf())) - .andExpect(status().isBadRequest()) + String payload = "{\"currentPassword\":\"" + TEST_PASSWORD + "\"}"; + mockMvc.perform(delete("/user/webauthn/credentials/does-not-exist").with(user(TEST_EMAIL).roles("USER")).with(csrf()) + .contentType(MediaType.APPLICATION_JSON).content(payload)).andExpect(status().isBadRequest()) .andExpect(jsonPath("$.message").value("Credential not found or access denied")); } + + @Test + @DisplayName("should reject passkey deletion when account has password and current password missing") + void shouldRejectDeleteWhenCurrentPasswordMissingForPasswordAccount() throws Exception { + mockMvc.perform(delete("/user/webauthn/credentials/cred-1").with(user(TEST_EMAIL).roles("USER")).with(csrf())) + .andExpect(status().isBadRequest()) + .andExpect(jsonPath("$.message", containsString("Current password is required"))); + + assertThat(webAuthnCredentialRepository.findByIdWithUser("cred-1")).isPresent(); + } + + @Test + @DisplayName("should reject passkey deletion when account has password and current password incorrect") + void shouldRejectDeleteWhenCurrentPasswordIncorrectForPasswordAccount() throws Exception { + String payload = "{\"currentPassword\":\"wrongPassword!\"}"; + mockMvc.perform(delete("/user/webauthn/credentials/cred-1").with(user(TEST_EMAIL).roles("USER")).with(csrf()) + .contentType(MediaType.APPLICATION_JSON).content(payload)).andExpect(status().isBadRequest()) + .andExpect(jsonPath("$.message", containsString("Current password is incorrect"))); + + assertThat(webAuthnCredentialRepository.findByIdWithUser("cred-1")).isPresent(); + } + + @Test + @DisplayName("should reject passkey rename when account has password and current password missing") + void shouldRejectRenameWhenCurrentPasswordMissingForPasswordAccount() throws Exception { + String payload = "{\"label\":\"New Label\"}"; + mockMvc.perform(put("/user/webauthn/credentials/cred-1/label").with(user(TEST_EMAIL).roles("USER")).with(csrf()) + .contentType(MediaType.APPLICATION_JSON).content(payload)).andExpect(status().isBadRequest()) + .andExpect(jsonPath("$.message", containsString("Current password is required"))); + + assertThat(webAuthnCredentialRepository.findByIdWithUser("cred-1")) + .isPresent() + .hasValueSatisfying(c -> assertThat(c.getLabel()).isEqualTo("My Device")); + } + + @Test + @DisplayName("should reject passkey rename when account has password and current password incorrect") + void shouldRejectRenameWhenCurrentPasswordIncorrectForPasswordAccount() throws Exception { + String payload = "{\"label\":\"New Label\",\"currentPassword\":\"wrongPassword!\"}"; + mockMvc.perform(put("/user/webauthn/credentials/cred-1/label").with(user(TEST_EMAIL).roles("USER")).with(csrf()) + .contentType(MediaType.APPLICATION_JSON).content(payload)).andExpect(status().isBadRequest()) + .andExpect(jsonPath("$.message", containsString("Current password is incorrect"))); + + assertThat(webAuthnCredentialRepository.findByIdWithUser("cred-1")) + .isPresent() + .hasValueSatisfying(c -> assertThat(c.getLabel()).isEqualTo("My Device")); + } } diff --git a/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPITest.java b/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPITest.java index ae1b1dc1..f669cff7 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPITest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/api/WebAuthnManagementAPITest.java @@ -27,6 +27,7 @@ import com.digitalsanctuary.spring.user.exceptions.WebAuthnException; import com.digitalsanctuary.spring.user.exceptions.WebAuthnUserNotFoundException; import com.digitalsanctuary.spring.user.persistence.model.User; +import com.digitalsanctuary.spring.user.service.LoginAttemptService; import com.digitalsanctuary.spring.user.service.UserService; import com.digitalsanctuary.spring.user.service.WebAuthnCredentialManagementService; import com.digitalsanctuary.spring.user.test.annotations.ServiceTest; @@ -48,6 +49,9 @@ class WebAuthnManagementAPITest { @Mock private ApplicationEventPublisher eventPublisher; + @Mock + private LoginAttemptService loginAttemptService; + @Mock private UserDetails userDetails; @@ -161,10 +165,12 @@ void shouldThrowNotFoundWhenUserNotFound() { class RenameCredentialTests { @Test - @DisplayName("should rename credential successfully") - void shouldRenameSuccessfully() { + @DisplayName("should rename credential successfully when account is passwordless") + void shouldRenameSuccessfullyWhenAccountPasswordless() { // Given - WebAuthnManagementAPI.RenameCredentialRequest request = new WebAuthnManagementAPI.RenameCredentialRequest("Work Laptop"); + testUser.setPassword(null); + when(userService.hasPassword(testUser)).thenReturn(false); + WebAuthnManagementAPI.RenameCredentialRequest request = new WebAuthnManagementAPI.RenameCredentialRequest("Work Laptop", null); // When ResponseEntity response = api.renameCredential("cred-1", request, userDetails); @@ -175,11 +181,85 @@ void shouldRenameSuccessfully() { verify(credentialManagementService).renameCredential("cred-1", "Work Laptop", testUser); } + @Test + @DisplayName("should rename credential successfully when correct current password provided") + void shouldRenameSuccessfullyWhenCorrectCurrentPassword() { + // Given + testUser.setPassword("encodedPassword"); + when(userService.hasPassword(testUser)).thenReturn(true); + when(userService.checkIfValidOldPassword(testUser, "currentPass")).thenReturn(true); + WebAuthnManagementAPI.RenameCredentialRequest request = + new WebAuthnManagementAPI.RenameCredentialRequest("Work Laptop", "currentPass"); + + // When + ResponseEntity response = api.renameCredential("cred-1", request, userDetails); + + // Then + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.OK); + verify(credentialManagementService).renameCredential("cred-1", "Work Laptop", testUser); + // Successful re-authentication resets the failed-attempt counter, matching login semantics. + verify(loginAttemptService).loginSucceeded(testUser.getEmail()); + verify(loginAttemptService, never()).loginFailed(testUser.getEmail()); + } + + @Test + @DisplayName("should reject rename and report failed attempt when account is locked") + void shouldRejectRenameWhenAccountLocked() { + // Given + testUser.setPassword("encodedPassword"); + when(userService.hasPassword(testUser)).thenReturn(true); + when(loginAttemptService.isLocked(testUser.getEmail())).thenReturn(true); + WebAuthnManagementAPI.RenameCredentialRequest request = + new WebAuthnManagementAPI.RenameCredentialRequest("Work Laptop", "currentPass"); + + // When & Then - locked accounts are rejected before the password is even checked. + assertThatThrownBy(() -> api.renameCredential("cred-1", request, userDetails)).isInstanceOf(WebAuthnException.class) + .hasMessageContaining("locked"); + verify(credentialManagementService, never()).renameCredential(any(), any(), any()); + verify(userService, never()).checkIfValidOldPassword(any(), any()); + } + + @Test + @DisplayName("should reject rename when account has password and current password missing") + void shouldRejectRenameWhenCurrentPasswordMissing() { + // Given + testUser.setPassword("encodedPassword"); + when(userService.hasPassword(testUser)).thenReturn(true); + WebAuthnManagementAPI.RenameCredentialRequest request = new WebAuthnManagementAPI.RenameCredentialRequest("Work Laptop", null); + + // When & Then + assertThatThrownBy(() -> api.renameCredential("cred-1", request, userDetails)).isInstanceOf(WebAuthnException.class) + .hasMessageContaining("Current password is required"); + verify(credentialManagementService, never()).renameCredential(any(), any(), any()); + // A missing field is a client error, not a password guess, so it must not count toward lockout. + verify(loginAttemptService, never()).loginFailed(any()); + } + + @Test + @DisplayName("should reject rename when account has password and current password incorrect") + void shouldRejectRenameWhenCurrentPasswordIncorrect() { + // Given + testUser.setPassword("encodedPassword"); + when(userService.hasPassword(testUser)).thenReturn(true); + when(userService.checkIfValidOldPassword(testUser, "wrongPass")).thenReturn(false); + WebAuthnManagementAPI.RenameCredentialRequest request = + new WebAuthnManagementAPI.RenameCredentialRequest("Work Laptop", "wrongPass"); + + // When & Then + assertThatThrownBy(() -> api.renameCredential("cred-1", request, userDetails)).isInstanceOf(WebAuthnException.class) + .hasMessageContaining("Current password is incorrect"); + verify(credentialManagementService, never()).renameCredential(any(), any(), any()); + // A wrong password is reported to the lockout service so repeated guesses eventually lock the account. + verify(loginAttemptService).loginFailed(testUser.getEmail()); + } + @Test @DisplayName("should throw when rename fails") void shouldThrowOnFailure() { // Given - WebAuthnManagementAPI.RenameCredentialRequest request = new WebAuthnManagementAPI.RenameCredentialRequest("New Name"); + testUser.setPassword(null); + when(userService.hasPassword(testUser)).thenReturn(false); + WebAuthnManagementAPI.RenameCredentialRequest request = new WebAuthnManagementAPI.RenameCredentialRequest("New Name", null); doThrow(new WebAuthnException("Credential not found or access denied")).when(credentialManagementService) .renameCredential(eq("cred-999"), eq("New Name"), any(User.class)); @@ -192,7 +272,7 @@ void shouldThrowOnFailure() { @DisplayName("should throw not found when user not found") void shouldThrowNotFoundWhenUserNotFound() { // Given - WebAuthnManagementAPI.RenameCredentialRequest request = new WebAuthnManagementAPI.RenameCredentialRequest("New Name"); + WebAuthnManagementAPI.RenameCredentialRequest request = new WebAuthnManagementAPI.RenameCredentialRequest("New Name", null); when(userService.findUserByEmail(testUser.getEmail())).thenReturn(null); // When @@ -207,10 +287,14 @@ void shouldThrowNotFoundWhenUserNotFound() { class DeleteCredentialTests { @Test - @DisplayName("should delete credential successfully") - void shouldDeleteSuccessfully() { + @DisplayName("should delete credential successfully when account is passwordless") + void shouldDeleteSuccessfullyWhenAccountPasswordless() { + // Given + testUser.setPassword(null); + when(userService.hasPassword(testUser)).thenReturn(false); + // When - ResponseEntity response = api.deleteCredential("cred-1", userDetails); + ResponseEntity response = api.deleteCredential("cred-1", null, userDetails); // Then assertThat(response.getStatusCode()).isEqualTo(HttpStatus.OK); @@ -218,15 +302,62 @@ void shouldDeleteSuccessfully() { verify(credentialManagementService).deleteCredential("cred-1", testUser); } + @Test + @DisplayName("should delete credential successfully when correct current password provided") + void shouldDeleteSuccessfullyWhenCorrectCurrentPassword() { + // Given + testUser.setPassword("encodedPassword"); + when(userService.hasPassword(testUser)).thenReturn(true); + when(userService.checkIfValidOldPassword(testUser, "currentPass")).thenReturn(true); + WebAuthnManagementAPI.CurrentPasswordRequest request = new WebAuthnManagementAPI.CurrentPasswordRequest("currentPass"); + + // When + ResponseEntity response = api.deleteCredential("cred-1", request, userDetails); + + // Then + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.OK); + verify(credentialManagementService).deleteCredential("cred-1", testUser); + } + + @Test + @DisplayName("should reject delete when account has password and current password missing") + void shouldRejectDeleteWhenCurrentPasswordMissing() { + // Given + testUser.setPassword("encodedPassword"); + when(userService.hasPassword(testUser)).thenReturn(true); + + // When & Then + assertThatThrownBy(() -> api.deleteCredential("cred-1", null, userDetails)).isInstanceOf(WebAuthnException.class) + .hasMessageContaining("Current password is required"); + verify(credentialManagementService, never()).deleteCredential(any(), any()); + } + + @Test + @DisplayName("should reject delete when account has password and current password incorrect") + void shouldRejectDeleteWhenCurrentPasswordIncorrect() { + // Given + testUser.setPassword("encodedPassword"); + when(userService.hasPassword(testUser)).thenReturn(true); + when(userService.checkIfValidOldPassword(testUser, "wrongPass")).thenReturn(false); + WebAuthnManagementAPI.CurrentPasswordRequest request = new WebAuthnManagementAPI.CurrentPasswordRequest("wrongPass"); + + // When & Then + assertThatThrownBy(() -> api.deleteCredential("cred-1", request, userDetails)).isInstanceOf(WebAuthnException.class) + .hasMessageContaining("Current password is incorrect"); + verify(credentialManagementService, never()).deleteCredential(any(), any()); + } + @Test @DisplayName("should throw when delete fails") void shouldThrowOnFailure() { // Given + testUser.setPassword(null); + when(userService.hasPassword(testUser)).thenReturn(false); doThrow(new WebAuthnException("Cannot delete last passkey")).when(credentialManagementService).deleteCredential(eq("cred-1"), any(User.class)); // When - assertThatThrownBy(() -> api.deleteCredential("cred-1", userDetails)).isInstanceOf(WebAuthnException.class) + assertThatThrownBy(() -> api.deleteCredential("cred-1", null, userDetails)).isInstanceOf(WebAuthnException.class) .hasMessageContaining("Cannot delete last passkey"); } @@ -237,7 +368,7 @@ void shouldThrowNotFoundWhenUserNotFound() { when(userService.findUserByEmail(testUser.getEmail())).thenReturn(null); // When - assertThatThrownBy(() -> api.deleteCredential("cred-1", userDetails)) + assertThatThrownBy(() -> api.deleteCredential("cred-1", null, userDetails)) .isInstanceOf(WebAuthnUserNotFoundException.class).hasMessageContaining("User not found"); verify(credentialManagementService, never()).deleteCredential(any(), any()); } @@ -256,16 +387,18 @@ private HttpServletRequest createMockRequest() { } @Test - @DisplayName("should remove password when user has passkeys") - void shouldRemovePasswordWhenUserHasPasskeys() { + @DisplayName("should remove password when user has passkeys and correct current password") + void shouldRemovePasswordWhenUserHasPasskeysAndCorrectCurrentPassword() { // Given HttpServletRequest mockRequest = createMockRequest(); testUser.setPassword("encodedPassword"); when(userService.hasPassword(testUser)).thenReturn(true); + when(userService.checkIfValidOldPassword(testUser, "currentPass")).thenReturn(true); when(credentialManagementService.hasCredentials(testUser)).thenReturn(true); + WebAuthnManagementAPI.CurrentPasswordRequest request = new WebAuthnManagementAPI.CurrentPasswordRequest("currentPass"); // When - ResponseEntity response = api.removePassword(userDetails, mockRequest); + ResponseEntity response = api.removePassword(request, userDetails, mockRequest); // Then assertThat(response.getStatusCode()).isEqualTo(HttpStatus.OK); @@ -274,6 +407,38 @@ void shouldRemovePasswordWhenUserHasPasskeys() { verify(eventPublisher).publishEvent(any(AuditEvent.class)); } + @Test + @DisplayName("should reject removal when current password missing") + void shouldRejectRemovalWhenCurrentPasswordMissing() { + // Given + HttpServletRequest mockRequest = mock(HttpServletRequest.class); + testUser.setPassword("encodedPassword"); + when(userService.hasPassword(testUser)).thenReturn(true); + + // When & Then + assertThatThrownBy(() -> api.removePassword(null, userDetails, mockRequest)) + .isInstanceOf(WebAuthnException.class) + .hasMessageContaining("Current password is required"); + verify(userService, never()).removeUserPassword(any()); + } + + @Test + @DisplayName("should reject removal when current password incorrect") + void shouldRejectRemovalWhenCurrentPasswordIncorrect() { + // Given + HttpServletRequest mockRequest = mock(HttpServletRequest.class); + testUser.setPassword("encodedPassword"); + when(userService.hasPassword(testUser)).thenReturn(true); + when(userService.checkIfValidOldPassword(testUser, "wrongPass")).thenReturn(false); + WebAuthnManagementAPI.CurrentPasswordRequest request = new WebAuthnManagementAPI.CurrentPasswordRequest("wrongPass"); + + // When & Then + assertThatThrownBy(() -> api.removePassword(request, userDetails, mockRequest)) + .isInstanceOf(WebAuthnException.class) + .hasMessageContaining("Current password is incorrect"); + verify(userService, never()).removeUserPassword(any()); + } + @Test @DisplayName("should reject removal when no passkeys") void shouldRejectRemovalWhenNoPasskeys() { @@ -281,10 +446,12 @@ void shouldRejectRemovalWhenNoPasskeys() { HttpServletRequest mockRequest = mock(HttpServletRequest.class); testUser.setPassword("encodedPassword"); when(userService.hasPassword(testUser)).thenReturn(true); + when(userService.checkIfValidOldPassword(testUser, "currentPass")).thenReturn(true); when(credentialManagementService.hasCredentials(testUser)).thenReturn(false); + WebAuthnManagementAPI.CurrentPasswordRequest request = new WebAuthnManagementAPI.CurrentPasswordRequest("currentPass"); // When & Then - assertThatThrownBy(() -> api.removePassword(userDetails, mockRequest)) + assertThatThrownBy(() -> api.removePassword(request, userDetails, mockRequest)) .isInstanceOf(WebAuthnException.class) .hasMessageContaining("register a passkey first"); verify(userService, never()).removeUserPassword(any()); @@ -297,9 +464,10 @@ void shouldRejectRemovalWhenAlreadyPasswordless() { HttpServletRequest mockRequest = mock(HttpServletRequest.class); testUser.setPassword(null); when(userService.hasPassword(testUser)).thenReturn(false); + WebAuthnManagementAPI.CurrentPasswordRequest request = new WebAuthnManagementAPI.CurrentPasswordRequest("anything"); // When & Then - assertThatThrownBy(() -> api.removePassword(userDetails, mockRequest)) + assertThatThrownBy(() -> api.removePassword(request, userDetails, mockRequest)) .isInstanceOf(WebAuthnException.class) .hasMessageContaining("does not have a password"); verify(userService, never()).removeUserPassword(any()); diff --git a/src/test/java/com/digitalsanctuary/spring/user/architecture/SelfProxiedMethodVisibilityTest.java b/src/test/java/com/digitalsanctuary/spring/user/architecture/SelfProxiedMethodVisibilityTest.java index 14478290..ba7f19a7 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/architecture/SelfProxiedMethodVisibilityTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/architecture/SelfProxiedMethodVisibilityTest.java @@ -10,6 +10,7 @@ import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; import com.digitalsanctuary.spring.user.gdpr.GdprDeletionService; +import com.digitalsanctuary.spring.user.roles.RolePrivilegeSetupService; import com.digitalsanctuary.spring.user.service.UserEmailService; import com.digitalsanctuary.spring.user.service.UserService; @@ -35,9 +36,9 @@ *

    * *

    - * Note: this rule is intentionally scoped to the specific methods invoked via {@code self}. Other package-private - * {@code @Transactional} helpers (e.g. {@code RolePrivilegeSetupService.getOrCreateRole}) are called via {@code this} - * from within an already-transactional method and run in the caller's transaction, so they do not need to be proxied. + * Note: this rule is intentionally scoped to the specific methods invoked via {@code self}. The + * {@code RolePrivilegeSetupService.insert*}/{@code updateRolePrivileges} methods use the same {@code @Lazy self} + * pattern (to obtain a {@code REQUIRES_NEW} boundary per insert during startup), so they are included below. *

    */ @DisplayName("Self-proxied (@Lazy self) transactional methods must be proxyable (public/protected)") @@ -52,7 +53,10 @@ static List selfProxiedMethods() { Arguments.of(UserService.class, "persistChangedPassword"), Arguments.of(UserService.class, "persistInitialPassword"), Arguments.of(GdprDeletionService.class, "executeUserDeletion"), - Arguments.of(UserEmailService.class, "createPasswordResetTokenForUser")); + Arguments.of(UserEmailService.class, "createPasswordResetTokenForUser"), + Arguments.of(RolePrivilegeSetupService.class, "insertPrivilege"), + Arguments.of(RolePrivilegeSetupService.class, "insertRole"), + Arguments.of(RolePrivilegeSetupService.class, "updateRolePrivileges")); } @ParameterizedTest(name = "{0}#{1} is public or protected") diff --git a/src/test/java/com/digitalsanctuary/spring/user/event/ConsentChangedEventTest.java b/src/test/java/com/digitalsanctuary/spring/user/event/ConsentChangedEventTest.java index 3735b696..da8f25b0 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/event/ConsentChangedEventTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/event/ConsentChangedEventTest.java @@ -37,10 +37,11 @@ void eventCreation_storesUserAndConsentRecord() { // When ConsentChangedEvent event = new ConsentChangedEvent( - eventSource, testUser, record, ConsentChangedEvent.ChangeType.GRANTED); + eventSource, testUser.getId(), testUser.getEmail(), record, ConsentChangedEvent.ChangeType.GRANTED); // Then - assertThat(event.getUser()).isEqualTo(testUser); + assertThat(event.getUserId()).isEqualTo(testUser.getId()); + assertThat(event.getUserEmail()).isEqualTo(testUser.getEmail()); assertThat(event.getConsentRecord()).isEqualTo(record); assertThat(event.getSource()).isEqualTo(eventSource); } @@ -55,7 +56,7 @@ void getUserId_returnsUserId() { // When ConsentChangedEvent event = new ConsentChangedEvent( - eventSource, testUser, record, ConsentChangedEvent.ChangeType.GRANTED); + eventSource, testUser.getId(), testUser.getEmail(), record, ConsentChangedEvent.ChangeType.GRANTED); // Then assertThat(event.getUserId()).isEqualTo(1L); @@ -71,7 +72,7 @@ void getConsentType_returnsCorrectType() { // When ConsentChangedEvent event = new ConsentChangedEvent( - eventSource, testUser, record, ConsentChangedEvent.ChangeType.WITHDRAWN); + eventSource, testUser.getId(), testUser.getEmail(), record, ConsentChangedEvent.ChangeType.WITHDRAWN); // Then assertThat(event.getConsentType()).isEqualTo(ConsentType.ANALYTICS); @@ -87,7 +88,7 @@ void isGranted_returnsTrue_forGrantedChangeType() { // When ConsentChangedEvent event = new ConsentChangedEvent( - eventSource, testUser, record, ConsentChangedEvent.ChangeType.GRANTED); + eventSource, testUser.getId(), testUser.getEmail(), record, ConsentChangedEvent.ChangeType.GRANTED); // Then assertThat(event.isGranted()).isTrue(); @@ -104,7 +105,7 @@ void isWithdrawn_returnsTrue_forWithdrawnChangeType() { // When ConsentChangedEvent event = new ConsentChangedEvent( - eventSource, testUser, record, ConsentChangedEvent.ChangeType.WITHDRAWN); + eventSource, testUser.getId(), testUser.getEmail(), record, ConsentChangedEvent.ChangeType.WITHDRAWN); // Then assertThat(event.isWithdrawn()).isTrue(); @@ -121,9 +122,9 @@ void getChangeType_returnsCorrectType() { // When ConsentChangedEvent grantedEvent = new ConsentChangedEvent( - eventSource, testUser, record, ConsentChangedEvent.ChangeType.GRANTED); + eventSource, testUser.getId(), testUser.getEmail(), record, ConsentChangedEvent.ChangeType.GRANTED); ConsentChangedEvent withdrawnEvent = new ConsentChangedEvent( - eventSource, testUser, record, ConsentChangedEvent.ChangeType.WITHDRAWN); + eventSource, testUser.getId(), testUser.getEmail(), record, ConsentChangedEvent.ChangeType.WITHDRAWN); // Then assertThat(grantedEvent.getChangeType()).isEqualTo(ConsentChangedEvent.ChangeType.GRANTED); @@ -141,7 +142,7 @@ void event_timestampIsSet() { // When ConsentChangedEvent event = new ConsentChangedEvent( - eventSource, testUser, record, ConsentChangedEvent.ChangeType.GRANTED); + eventSource, testUser.getId(), testUser.getEmail(), record, ConsentChangedEvent.ChangeType.GRANTED); // Then long afterCreation = System.currentTimeMillis(); diff --git a/src/test/java/com/digitalsanctuary/spring/user/event/OnRegistrationCompleteEventTest.java b/src/test/java/com/digitalsanctuary/spring/user/event/OnRegistrationCompleteEventTest.java index 53854f7d..82fbb5a0 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/event/OnRegistrationCompleteEventTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/event/OnRegistrationCompleteEventTest.java @@ -2,9 +2,6 @@ import static org.assertj.core.api.Assertions.assertThat; -import com.digitalsanctuary.spring.user.persistence.model.User; -import com.digitalsanctuary.spring.user.test.builders.UserTestDataBuilder; - import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; @@ -14,19 +11,15 @@ @DisplayName("OnRegistrationCompleteEvent Tests") class OnRegistrationCompleteEventTest { - private User testUser; + private Long userId; + private String userEmail; private String appUrl; private Locale locale; @BeforeEach void setUp() { - testUser = UserTestDataBuilder.aUser() - .withId(1L) - .withEmail("test@example.com") - .withFirstName("Test") - .withLastName("User") - .enabled() - .build(); + userId = 1L; + userEmail = "test@example.com"; appUrl = "https://example.com"; locale = Locale.ENGLISH; } @@ -36,29 +29,35 @@ void setUp() { void eventCreation_withBuilder() { // When OnRegistrationCompleteEvent event = OnRegistrationCompleteEvent.builder() - .user(testUser) + .userId(userId) + .userEmail(userEmail) + .userEnabled(true) .locale(locale) .appUrl(appUrl) .build(); // Then - assertThat(event.getUser()).isEqualTo(testUser); + assertThat(event.getUserId()).isEqualTo(userId); + assertThat(event.getUserEmail()).isEqualTo(userEmail); + assertThat(event.isUserEnabled()).isTrue(); assertThat(event.getLocale()).isEqualTo(locale); assertThat(event.getAppUrl()).isEqualTo(appUrl); - assertThat(event.getSource()).isEqualTo(testUser); + assertThat(event.getSource()).isEqualTo(userId); } @Test @DisplayName("Event creation with constructor") void eventCreation_withConstructor() { // When - OnRegistrationCompleteEvent event = new OnRegistrationCompleteEvent(testUser, locale, appUrl); + OnRegistrationCompleteEvent event = new OnRegistrationCompleteEvent(userId, userEmail, false, locale, appUrl); // Then - assertThat(event.getUser()).isEqualTo(testUser); + assertThat(event.getUserId()).isEqualTo(userId); + assertThat(event.getUserEmail()).isEqualTo(userEmail); + assertThat(event.isUserEnabled()).isFalse(); assertThat(event.getLocale()).isEqualTo(locale); assertThat(event.getAppUrl()).isEqualTo(appUrl); - assertThat(event.getSource()).isEqualTo(testUser); + assertThat(event.getSource()).isEqualTo(userId); } @Test @@ -66,14 +65,16 @@ void eventCreation_withConstructor() { void event_withNullAppUrl() { // When OnRegistrationCompleteEvent event = OnRegistrationCompleteEvent.builder() - .user(testUser) + .userId(userId) + .userEmail(userEmail) + .userEnabled(false) .locale(locale) .appUrl(null) .build(); // Then assertThat(event.getAppUrl()).isNull(); - assertThat(event.getUser()).isEqualTo(testUser); + assertThat(event.getUserId()).isEqualTo(userId); assertThat(event.getLocale()).isEqualTo(locale); } @@ -85,7 +86,8 @@ void event_withDifferentLocales() { // When OnRegistrationCompleteEvent event = OnRegistrationCompleteEvent.builder() - .user(testUser) + .userId(userId) + .userEmail(userEmail) .locale(frenchLocale) .appUrl(appUrl) .build(); @@ -98,30 +100,28 @@ void event_withDifferentLocales() { @Test @DisplayName("Event equality includes all fields") void eventEquality_includesAllFields() { - // Given - User anotherUser = UserTestDataBuilder.aUser() - .withId(2L) - .withEmail("another@example.com") - .build(); - // When OnRegistrationCompleteEvent event1 = OnRegistrationCompleteEvent.builder() - .user(testUser) + .userId(userId) + .userEmail(userEmail) .locale(locale) .appUrl(appUrl) .build(); OnRegistrationCompleteEvent event2 = OnRegistrationCompleteEvent.builder() - .user(testUser) + .userId(userId) + .userEmail(userEmail) .locale(locale) .appUrl(appUrl) .build(); OnRegistrationCompleteEvent event3 = OnRegistrationCompleteEvent.builder() - .user(testUser) + .userId(userId) + .userEmail(userEmail) .locale(Locale.FRENCH) .appUrl("https://different.com") .build(); OnRegistrationCompleteEvent event4 = OnRegistrationCompleteEvent.builder() - .user(anotherUser) + .userId(2L) + .userEmail("another@example.com") .locale(locale) .appUrl(appUrl) .build(); @@ -137,7 +137,8 @@ void eventEquality_includesAllFields() { void event_toStringIncludesRelevantInfo() { // When OnRegistrationCompleteEvent event = OnRegistrationCompleteEvent.builder() - .user(testUser) + .userId(userId) + .userEmail(userEmail) .locale(locale) .appUrl(appUrl) .build(); @@ -148,4 +149,4 @@ void event_toStringIncludesRelevantInfo() { assertThat(eventString).contains(appUrl); assertThat(eventString).contains("en"); } -} \ No newline at end of file +} diff --git a/src/test/java/com/digitalsanctuary/spring/user/event/UserPreDeleteEventTest.java b/src/test/java/com/digitalsanctuary/spring/user/event/UserPreDeleteEventTest.java index 7812536d..eef26935 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/event/UserPreDeleteEventTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/event/UserPreDeleteEventTest.java @@ -2,9 +2,6 @@ import static org.assertj.core.api.Assertions.assertThat; -import com.digitalsanctuary.spring.user.persistence.model.User; -import com.digitalsanctuary.spring.user.test.builders.UserTestDataBuilder; - import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; @@ -12,29 +9,26 @@ @DisplayName("UserPreDeleteEvent Tests") class UserPreDeleteEventTest { - private User testUser; + private Long userId; + private String userEmail; private Object eventSource; @BeforeEach void setUp() { - testUser = UserTestDataBuilder.aUser() - .withId(1L) - .withEmail("test@example.com") - .withFirstName("Test") - .withLastName("User") - .enabled() - .build(); + userId = 1L; + userEmail = "test@example.com"; eventSource = this; } @Test - @DisplayName("Event creation stores user and source") - void eventCreation_storesUserAndSource() { + @DisplayName("Event creation stores user data and source") + void eventCreation_storesUserDataAndSource() { // When - UserPreDeleteEvent event = new UserPreDeleteEvent(eventSource, testUser); + UserPreDeleteEvent event = new UserPreDeleteEvent(eventSource, userId, userEmail); // Then - assertThat(event.getUser()).isEqualTo(testUser); + assertThat(event.getUserId()).isEqualTo(userId); + assertThat(event.getUserEmail()).isEqualTo(userEmail); assertThat(event.getSource()).isEqualTo(eventSource); } @@ -42,7 +36,7 @@ void eventCreation_storesUserAndSource() { @DisplayName("getUserId returns user's ID") void getUserId_returnsUserId() { // When - UserPreDeleteEvent event = new UserPreDeleteEvent(eventSource, testUser); + UserPreDeleteEvent event = new UserPreDeleteEvent(eventSource, userId, userEmail); // Then assertThat(event.getUserId()).isEqualTo(1L); @@ -56,50 +50,38 @@ void event_withDifferentSources() { Object source2 = "Different Source"; // When - UserPreDeleteEvent event1 = new UserPreDeleteEvent(source1, testUser); - UserPreDeleteEvent event2 = new UserPreDeleteEvent(source2, testUser); + UserPreDeleteEvent event1 = new UserPreDeleteEvent(source1, userId, userEmail); + UserPreDeleteEvent event2 = new UserPreDeleteEvent(source2, userId, userEmail); // Then assertThat(event1.getSource()).isEqualTo(source1); assertThat(event2.getSource()).isEqualTo(source2); - assertThat(event1.getUser()).isEqualTo(event2.getUser()); + assertThat(event1.getUserId()).isEqualTo(event2.getUserId()); } @Test @DisplayName("Event preserves user information") void event_preservesUserInformation() { // When - UserPreDeleteEvent event = new UserPreDeleteEvent(eventSource, testUser); + UserPreDeleteEvent event = new UserPreDeleteEvent(eventSource, userId, userEmail); // Then - User eventUser = event.getUser(); - assertThat(eventUser.getEmail()).isEqualTo("test@example.com"); - assertThat(eventUser.getFirstName()).isEqualTo("Test"); - assertThat(eventUser.getLastName()).isEqualTo("User"); - assertThat(eventUser.isEnabled()).isTrue(); + assertThat(event.getUserId()).isEqualTo(1L); + assertThat(event.getUserEmail()).isEqualTo("test@example.com"); } @Test @DisplayName("Multiple events for different users") void multipleEvents_forDifferentUsers() { - // Given - User user1 = UserTestDataBuilder.aUser() - .withId(1L) - .withEmail("user1@example.com") - .build(); - User user2 = UserTestDataBuilder.aUser() - .withId(2L) - .withEmail("user2@example.com") - .build(); - // When - UserPreDeleteEvent event1 = new UserPreDeleteEvent(eventSource, user1); - UserPreDeleteEvent event2 = new UserPreDeleteEvent(eventSource, user2); + UserPreDeleteEvent event1 = new UserPreDeleteEvent(eventSource, 1L, "user1@example.com"); + UserPreDeleteEvent event2 = new UserPreDeleteEvent(eventSource, 2L, "user2@example.com"); // Then - assertThat(event1.getUser()).isNotEqualTo(event2.getUser()); assertThat(event1.getUserId()).isEqualTo(1L); assertThat(event2.getUserId()).isEqualTo(2L); + assertThat(event1.getUserEmail()).isEqualTo("user1@example.com"); + assertThat(event2.getUserEmail()).isEqualTo("user2@example.com"); } @Test @@ -109,11 +91,11 @@ void event_timestampIsSet() { long beforeCreation = System.currentTimeMillis(); // When - UserPreDeleteEvent event = new UserPreDeleteEvent(eventSource, testUser); + UserPreDeleteEvent event = new UserPreDeleteEvent(eventSource, userId, userEmail); // Then long afterCreation = System.currentTimeMillis(); assertThat(event.getTimestamp()).isGreaterThanOrEqualTo(beforeCreation); assertThat(event.getTimestamp()).isLessThanOrEqualTo(afterCreation); } -} \ No newline at end of file +} diff --git a/src/test/java/com/digitalsanctuary/spring/user/exceptions/GlobalValidationExceptionHandlerScopeTest.java b/src/test/java/com/digitalsanctuary/spring/user/exceptions/GlobalValidationExceptionHandlerScopeTest.java new file mode 100644 index 00000000..74db97c0 --- /dev/null +++ b/src/test/java/com/digitalsanctuary/spring/user/exceptions/GlobalValidationExceptionHandlerScopeTest.java @@ -0,0 +1,56 @@ +package com.digitalsanctuary.spring.user.exceptions; + +import static org.assertj.core.api.Assertions.assertThat; + +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.springframework.web.bind.annotation.ControllerAdvice; + +import com.digitalsanctuary.spring.user.api.GdprAPI; +import com.digitalsanctuary.spring.user.api.MfaAPI; +import com.digitalsanctuary.spring.user.api.UserAPI; +import com.digitalsanctuary.spring.user.api.WebAuthnManagementAPI; +import com.digitalsanctuary.spring.user.controller.UserActionController; +import com.digitalsanctuary.spring.user.controller.UserPageController; + +/** + * Verifies that {@link GlobalValidationExceptionHandler} is scoped to the library's own controllers + * via {@code @ControllerAdvice(assignableTypes = ...)} rather than applying application-wide. + */ +@DisplayName("GlobalValidationExceptionHandler scope") +class GlobalValidationExceptionHandlerScopeTest { + + @Test + @DisplayName("Should be scoped to the library's own controllers via assignableTypes") + void shouldScopeAdviceToLibraryControllers() { + ControllerAdvice annotation = GlobalValidationExceptionHandler.class.getAnnotation(ControllerAdvice.class); + + assertThat(annotation).as("@ControllerAdvice must be present").isNotNull(); + assertThat(annotation.assignableTypes()) + .as("Advice must be scoped to the library's own controllers") + .containsExactlyInAnyOrder(UserAPI.class, GdprAPI.class, MfaAPI.class, UserActionController.class, UserPageController.class); + } + + @Test + @DisplayName("Should NOT target WebAuthnManagementAPI (it has its own dedicated advice)") + void shouldNotTargetWebAuthnManagementApi() { + ControllerAdvice annotation = GlobalValidationExceptionHandler.class.getAnnotation(ControllerAdvice.class); + + assertThat(annotation.assignableTypes()) + .as("WebAuthnManagementAPI is handled by its dedicated advice and must be excluded") + .doesNotContain(WebAuthnManagementAPI.class); + } + + @Test + @DisplayName("Should NOT apply application-wide (no basePackages, annotations, or unrestricted scope)") + void shouldNotApplyApplicationWide() { + ControllerAdvice annotation = GlobalValidationExceptionHandler.class.getAnnotation(ControllerAdvice.class); + + assertThat(annotation.assignableTypes()) + .as("assignableTypes must be set so the advice is not global") + .isNotEmpty(); + assertThat(annotation.basePackages()).as("No basePackages scope").isEmpty(); + assertThat(annotation.basePackageClasses()).as("No basePackageClasses scope").isEmpty(); + assertThat(annotation.annotations()).as("No annotation-based scope").isEmpty(); + } +} diff --git a/src/test/java/com/digitalsanctuary/spring/user/gdpr/ConsentAuditServiceTest.java b/src/test/java/com/digitalsanctuary/spring/user/gdpr/ConsentAuditServiceTest.java index f7eeb9b5..d6708b89 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/gdpr/ConsentAuditServiceTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/gdpr/ConsentAuditServiceTest.java @@ -164,7 +164,8 @@ void publishesConsentChangedEvent() { verify(eventPublisher).publishEvent(eventCaptor.capture()); ConsentChangedEvent capturedEvent = eventCaptor.getValue(); - assertThat(capturedEvent.getUser()).isEqualTo(testUser); + assertThat(capturedEvent.getUserId()).isEqualTo(testUser.getId()); + assertThat(capturedEvent.getUserEmail()).isEqualTo(testUser.getEmail()); assertThat(capturedEvent.getConsentType()).isEqualTo(ConsentType.MARKETING_EMAILS); assertThat(capturedEvent.isGranted()).isTrue(); } diff --git a/src/test/java/com/digitalsanctuary/spring/user/gdpr/GdprDeletionServiceTest.java b/src/test/java/com/digitalsanctuary/spring/user/gdpr/GdprDeletionServiceTest.java index 344c6a47..c4d6599d 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/gdpr/GdprDeletionServiceTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/gdpr/GdprDeletionServiceTest.java @@ -107,7 +107,8 @@ void publishesUserPreDeleteEvent_beforeDeletion() { // Then ArgumentCaptor eventCaptor = ArgumentCaptor.forClass(UserPreDeleteEvent.class); verify(eventPublisher).publishEvent(eventCaptor.capture()); - assertThat(eventCaptor.getValue().getUser()).isEqualTo(testUser); + assertThat(eventCaptor.getValue().getUserId()).isEqualTo(testUser.getId()); + assertThat(eventCaptor.getValue().getUserEmail()).isEqualTo(testUser.getEmail()); } @Test diff --git a/src/test/java/com/digitalsanctuary/spring/user/gdpr/GdprExportServiceTest.java b/src/test/java/com/digitalsanctuary/spring/user/gdpr/GdprExportServiceTest.java index 03cf8162..1a969283 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/gdpr/GdprExportServiceTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/gdpr/GdprExportServiceTest.java @@ -28,6 +28,7 @@ import com.digitalsanctuary.spring.user.persistence.model.User; import com.digitalsanctuary.spring.user.persistence.model.VerificationToken; import com.digitalsanctuary.spring.user.persistence.repository.PasswordResetTokenRepository; +import com.digitalsanctuary.spring.user.persistence.repository.UserRepository; import com.digitalsanctuary.spring.user.persistence.repository.VerificationTokenRepository; import com.digitalsanctuary.spring.user.test.annotations.ServiceTest; import com.digitalsanctuary.spring.user.test.builders.UserTestDataBuilder; @@ -48,6 +49,9 @@ class GdprExportServiceTest { @Mock private PasswordResetTokenRepository passwordResetTokenRepository; + @Mock + private UserRepository userRepository; + @Mock private ApplicationEventPublisher eventPublisher; @@ -102,6 +106,27 @@ void exportsBasicUserData() { assertThat(export.getUserData().isEnabled()).isTrue(); } + @Test + @DisplayName("exports role names by reloading the user through the entity-graph finder") + void exportsRoleNamesViaEntityGraphFinder() { + // Given - roles are LAZY, so the export service reloads the user with roles initialized. + User userWithRoles = UserTestDataBuilder.aVerifiedUser() + .withId(1L) + .withEmail("test@example.com") + .withRole("ROLE_USER") + .build(); + when(userRepository.findWithRolesByEmail("test@example.com")).thenReturn(userWithRoles); + when(gdprConfig.isConsentTracking()).thenReturn(true); + when(auditLogQueryService.findByUser(testUser)).thenReturn(new ArrayList<>()); + when(auditLogQueryService.findByUserAndAction(any(), any())).thenReturn(new ArrayList<>()); + + // When + GdprExportDTO export = gdprExportService.exportUserData(testUser); + + // Then + assertThat(export.getUserData().getRoles()).containsExactly("ROLE_USER"); + } + @Test @DisplayName("includes export metadata") void includesExportMetadata() { diff --git a/src/test/java/com/digitalsanctuary/spring/user/listener/RegistrationListenerTest.java b/src/test/java/com/digitalsanctuary/spring/user/listener/RegistrationListenerTest.java index dbef822e..9cf620bc 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/listener/RegistrationListenerTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/listener/RegistrationListenerTest.java @@ -3,9 +3,7 @@ import static org.mockito.Mockito.*; import com.digitalsanctuary.spring.user.event.OnRegistrationCompleteEvent; -import com.digitalsanctuary.spring.user.persistence.model.User; import com.digitalsanctuary.spring.user.service.UserEmailService; -import com.digitalsanctuary.spring.user.test.builders.UserTestDataBuilder; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.DisplayName; @@ -29,25 +27,27 @@ class RegistrationListenerTest { @InjectMocks private RegistrationListener registrationListener; - private User testUser; + private static final Long USER_ID = 1L; + private static final String USER_EMAIL = "test@example.com"; private String appUrl; private Locale locale; @BeforeEach void setUp() { - // Default the shared fixture to a DISABLED (not-yet-verified) user so the "send verification email" - // tests exercise the email-sending path. The skip-for-enabled-users behavior is covered separately. - testUser = UserTestDataBuilder.aUser() - .withId(1L) - .withEmail("test@example.com") - .withFirstName("Test") - .withLastName("User") - .disabled() - .build(); appUrl = "https://example.com"; locale = Locale.ENGLISH; } + private OnRegistrationCompleteEvent eventFor(Long userId, String userEmail, boolean enabled, String url) { + return OnRegistrationCompleteEvent.builder() + .userId(userId) + .userEmail(userEmail) + .userEnabled(enabled) + .locale(locale) + .appUrl(url) + .build(); + } + @Nested @DisplayName("Registration Event Handling Tests") class RegistrationEventHandlingTests { @@ -55,24 +55,16 @@ class RegistrationEventHandlingTests { @Test @DisplayName("onApplicationEvent - sends verification email when enabled and user is not yet verified") void onApplicationEvent_sendsVerificationEmailWhenEnabled() { - // Given - a DISABLED (not yet verified) user, as produced by the form-registration path when + // Given - a not-yet-verified (disabled) user, as produced by the form-registration path when // email verification is required. ReflectionTestUtils.setField(registrationListener, "sendRegistrationVerificationEmail", true); - User unverifiedUser = UserTestDataBuilder.aUser() - .withEmail("unverified@example.com") - .disabled() - .build(); - OnRegistrationCompleteEvent event = OnRegistrationCompleteEvent.builder() - .user(unverifiedUser) - .locale(locale) - .appUrl(appUrl) - .build(); + OnRegistrationCompleteEvent event = eventFor(2L, "unverified@example.com", false, appUrl); // When registrationListener.onApplicationEvent(event); - // Then - verify(userEmailService).sendRegistrationVerificationEmail(unverifiedUser, appUrl); + // Then - the listener passes the user id; the email service reloads the entity in its own transaction. + verify(userEmailService).sendRegistrationVerificationEmail(2L, appUrl); } @Test @@ -81,21 +73,13 @@ void onApplicationEvent_skipsVerificationEmailForEnabledUser() { // Given - sending is enabled, but the user is already enabled (e.g. a first-time OAuth2/OIDC // registration where the provider has already verified the email). They must NOT receive an email. ReflectionTestUtils.setField(registrationListener, "sendRegistrationVerificationEmail", true); - User enabledUser = UserTestDataBuilder.aUser() - .withEmail("oauth@example.com") - .enabled() - .build(); - OnRegistrationCompleteEvent event = OnRegistrationCompleteEvent.builder() - .user(enabledUser) - .locale(locale) - .appUrl(appUrl) - .build(); + OnRegistrationCompleteEvent event = eventFor(3L, "oauth@example.com", true, appUrl); // When registrationListener.onApplicationEvent(event); // Then - verify(userEmailService, never()).sendRegistrationVerificationEmail(any(), any()); + verify(userEmailService, never()).sendRegistrationVerificationEmail(anyLong(), any()); } @Test @@ -103,35 +87,27 @@ void onApplicationEvent_skipsVerificationEmailForEnabledUser() { void onApplicationEvent_doesNotSendEmailWhenDisabled() { // Given ReflectionTestUtils.setField(registrationListener, "sendRegistrationVerificationEmail", false); - OnRegistrationCompleteEvent event = OnRegistrationCompleteEvent.builder() - .user(testUser) - .locale(locale) - .appUrl(appUrl) - .build(); + OnRegistrationCompleteEvent event = eventFor(USER_ID, USER_EMAIL, false, appUrl); // When registrationListener.onApplicationEvent(event); // Then - verify(userEmailService, never()).sendRegistrationVerificationEmail(any(), any()); + verify(userEmailService, never()).sendRegistrationVerificationEmail(anyLong(), any()); } @Test - @DisplayName("onApplicationEvent - handles null app URL") + @DisplayName("onApplicationEvent - passes null app URL through to email service") void onApplicationEvent_handlesNullAppUrl() { // Given ReflectionTestUtils.setField(registrationListener, "sendRegistrationVerificationEmail", true); - OnRegistrationCompleteEvent event = OnRegistrationCompleteEvent.builder() - .user(testUser) - .locale(locale) - .appUrl(null) - .build(); + OnRegistrationCompleteEvent event = eventFor(USER_ID, USER_EMAIL, false, null); // When registrationListener.onApplicationEvent(event); // Then - verify(userEmailService).sendRegistrationVerificationEmail(testUser, null); + verify(userEmailService).sendRegistrationVerificationEmail(USER_ID, null); } @Test @@ -139,10 +115,11 @@ void onApplicationEvent_handlesNullAppUrl() { void onApplicationEvent_handlesDifferentLocales() { // Given ReflectionTestUtils.setField(registrationListener, "sendRegistrationVerificationEmail", true); - Locale frenchLocale = Locale.FRENCH; OnRegistrationCompleteEvent event = OnRegistrationCompleteEvent.builder() - .user(testUser) - .locale(frenchLocale) + .userId(USER_ID) + .userEmail(USER_EMAIL) + .userEnabled(false) + .locale(Locale.FRENCH) .appUrl(appUrl) .build(); @@ -150,7 +127,7 @@ void onApplicationEvent_handlesDifferentLocales() { registrationListener.onApplicationEvent(event); // Then - verify(userEmailService).sendRegistrationVerificationEmail(testUser, appUrl); + verify(userEmailService).sendRegistrationVerificationEmail(USER_ID, appUrl); } } @@ -163,65 +140,35 @@ class MultipleUserRegistrationTests { void multipleRegistrationEvents_handledIndependently() { // Given ReflectionTestUtils.setField(registrationListener, "sendRegistrationVerificationEmail", true); - User user1 = UserTestDataBuilder.aUser() - .withEmail("user1@example.com") - .build(); - User user2 = UserTestDataBuilder.aUser() - .withEmail("user2@example.com") - .build(); - - OnRegistrationCompleteEvent event1 = OnRegistrationCompleteEvent.builder() - .user(user1) - .locale(locale) - .appUrl(appUrl) - .build(); - OnRegistrationCompleteEvent event2 = OnRegistrationCompleteEvent.builder() - .user(user2) - .locale(locale) - .appUrl(appUrl) - .build(); + OnRegistrationCompleteEvent event1 = eventFor(11L, "user1@example.com", false, appUrl); + OnRegistrationCompleteEvent event2 = eventFor(12L, "user2@example.com", false, appUrl); // When registrationListener.onApplicationEvent(event1); registrationListener.onApplicationEvent(event2); // Then - verify(userEmailService).sendRegistrationVerificationEmail(user1, appUrl); - verify(userEmailService).sendRegistrationVerificationEmail(user2, appUrl); + verify(userEmailService).sendRegistrationVerificationEmail(11L, appUrl); + verify(userEmailService).sendRegistrationVerificationEmail(12L, appUrl); } @Test @DisplayName("Configuration change affects subsequent events") void configurationChange_affectsSubsequentEvents() { // Given - User user1 = UserTestDataBuilder.aUser() - .withEmail("user1@example.com") - .build(); - User user2 = UserTestDataBuilder.aUser() - .withEmail("user2@example.com") - .build(); - - OnRegistrationCompleteEvent event1 = OnRegistrationCompleteEvent.builder() - .user(user1) - .locale(locale) - .appUrl(appUrl) - .build(); - OnRegistrationCompleteEvent event2 = OnRegistrationCompleteEvent.builder() - .user(user2) - .locale(locale) - .appUrl(appUrl) - .build(); + OnRegistrationCompleteEvent event1 = eventFor(11L, "user1@example.com", false, appUrl); + OnRegistrationCompleteEvent event2 = eventFor(12L, "user2@example.com", false, appUrl); // When ReflectionTestUtils.setField(registrationListener, "sendRegistrationVerificationEmail", true); registrationListener.onApplicationEvent(event1); - + ReflectionTestUtils.setField(registrationListener, "sendRegistrationVerificationEmail", false); registrationListener.onApplicationEvent(event2); // Then - verify(userEmailService).sendRegistrationVerificationEmail(user1, appUrl); - verify(userEmailService, never()).sendRegistrationVerificationEmail(user2, appUrl); + verify(userEmailService).sendRegistrationVerificationEmail(11L, appUrl); + verify(userEmailService, never()).sendRegistrationVerificationEmail(12L, appUrl); } } -} \ No newline at end of file +} diff --git a/src/test/java/com/digitalsanctuary/spring/user/listener/WebAuthnPreDeleteEventListenerTest.java b/src/test/java/com/digitalsanctuary/spring/user/listener/WebAuthnPreDeleteEventListenerTest.java index 039999c1..a81afb2f 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/listener/WebAuthnPreDeleteEventListenerTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/listener/WebAuthnPreDeleteEventListenerTest.java @@ -53,7 +53,7 @@ void shouldDeleteWebAuthnDataForUser() { when(userEntityRepository.findByUserId(testUser.getId())).thenReturn(Optional.of(userEntity)); - UserPreDeleteEvent event = new UserPreDeleteEvent(this, testUser); + UserPreDeleteEvent event = new UserPreDeleteEvent(this, testUser.getId(), testUser.getEmail()); // When listener.onUserPreDelete(event); @@ -69,7 +69,7 @@ void shouldDoNothingWhenNoWebAuthnData() { // Given when(userEntityRepository.findByUserId(testUser.getId())).thenReturn(Optional.empty()); - UserPreDeleteEvent event = new UserPreDeleteEvent(this, testUser); + UserPreDeleteEvent event = new UserPreDeleteEvent(this, testUser.getId(), testUser.getEmail()); // When listener.onUserPreDelete(event); diff --git a/src/test/java/com/digitalsanctuary/spring/user/persistence/model/EntityEqualityTest.java b/src/test/java/com/digitalsanctuary/spring/user/persistence/model/EntityEqualityTest.java new file mode 100644 index 00000000..a5ce86e4 --- /dev/null +++ b/src/test/java/com/digitalsanctuary/spring/user/persistence/model/EntityEqualityTest.java @@ -0,0 +1,151 @@ +package com.digitalsanctuary.spring.user.persistence.model; + +import static org.assertj.core.api.Assertions.assertThat; +import org.junit.jupiter.api.Test; + +/** + * Unit tests verifying the identity-based ({@code id}-only) {@code equals}/{@code hashCode} contract and the + * secret-safe {@code toString} on the JPA entities migrated away from Lombok {@code @Data}. + * + *

    Under id-only equality two managed instances are equal only when they share a non-null id; instances with + * different ids (or a null id) are not equal. {@code toString} must never include passwords or token secrets, as + * it is commonly emitted to logs.

    + */ +class EntityEqualityTest { + + @Test + void shouldBeEqualAndShareHashCodeWhenUsersHaveSameId() { + // Given two distinct User instances with the same id but otherwise different field values + User first = new User(); + first.setId(1L); + first.setEmail("first@test.com"); + + User second = new User(); + second.setId(1L); + second.setEmail("second@test.com"); + + // Then they are equal and share a hash code, regardless of differing non-id fields + assertThat(first).isEqualTo(second); + assertThat(first).hasSameHashCodeAs(second); + } + + @Test + void shouldNotBeEqualWhenUsersHaveDifferentIds() { + // Given two User instances with different ids + User first = new User(); + first.setId(1L); + + User second = new User(); + second.setId(2L); + + // Then they are not equal + assertThat(first).isNotEqualTo(second); + } + + @Test + void shouldNotBeEqualWhenOneUserHasNullId() { + // Given two User instances, one with a null id (transient/unsaved) + User saved = new User(); + saved.setId(1L); + + User transientUser = new User(); + + // Then they are not equal under id-only equality + assertThat(saved).isNotEqualTo(transientUser); + } + + @Test + void shouldNotIncludePasswordWhenUserToStringCalled() { + // Given a user carrying a (fake) password hash + User user = new User(); + user.setId(1L); + user.setEmail("user@test.com"); + user.setPassword("SUPERSECRET_HASH"); + + // Then the rendered string excludes the password hash + assertThat(user.toString()).doesNotContain("SUPERSECRET_HASH"); + } + + @Test + void shouldBeEqualAndShareHashCodeWhenVerificationTokensHaveSameId() { + // Given two distinct VerificationToken instances with the same id but different secret values + VerificationToken first = new VerificationToken("token-aaa"); + first.setId(10L); + + VerificationToken second = new VerificationToken("token-bbb"); + second.setId(10L); + + // Then they are equal and share a hash code + assertThat(first).isEqualTo(second); + assertThat(first).hasSameHashCodeAs(second); + } + + @Test + void shouldNotBeEqualWhenVerificationTokensHaveDifferentIds() { + // Given two VerificationToken instances with different ids + VerificationToken first = new VerificationToken("token"); + first.setId(10L); + + VerificationToken second = new VerificationToken("token"); + second.setId(20L); + + // Then they are not equal even with the same token value + assertThat(first).isNotEqualTo(second); + } + + @Test + void shouldNotBeEqualWhenOneVerificationTokenHasNullId() { + // Given two VerificationToken instances, one with a null id + VerificationToken saved = new VerificationToken("token"); + saved.setId(10L); + + VerificationToken transientToken = new VerificationToken("token"); + + // Then they are not equal under id-only equality + assertThat(saved).isNotEqualTo(transientToken); + } + + @Test + void shouldNotIncludeTokenSecretWhenVerificationTokenToStringCalled() { + // Given a verification token holding hashed and raw secret values + VerificationToken token = new VerificationToken("HASHED_TOKEN_SECRET"); + token.setId(10L); + token.setPlainToken("RAW_TOKEN_SECRET"); + + // When rendered to a string + String rendered = token.toString(); + + // Then neither the hashed nor the raw token secret appears + assertThat(rendered).doesNotContain("HASHED_TOKEN_SECRET"); + assertThat(rendered).doesNotContain("RAW_TOKEN_SECRET"); + } + + @Test + void shouldBeEqualAndShareHashCodeWhenWebAuthnCredentialsHaveSameCredentialId() { + // Given two distinct WebAuthnCredential instances with the same natural-key credentialId + WebAuthnCredential first = new WebAuthnCredential(); + first.setCredentialId("AAAA-same-credential-id"); + first.setLabel("My iPhone"); + + WebAuthnCredential second = new WebAuthnCredential(); + second.setCredentialId("AAAA-same-credential-id"); + second.setLabel("Some Other Label"); + + // Then they are equal and share a hash code, regardless of differing non-key fields + assertThat(first).isEqualTo(second); + assertThat(first).hasSameHashCodeAs(second); + } + + @Test + void shouldNotBeEqualWhenWebAuthnCredentialsHaveDifferentCredentialIds() { + // Given two WebAuthnCredential instances with different credentialIds + WebAuthnCredential first = new WebAuthnCredential(); + first.setCredentialId("AAAA-credential-one"); + + WebAuthnCredential second = new WebAuthnCredential(); + second.setCredentialId("BBBB-credential-two"); + + // Then they are not equal + assertThat(first).isNotEqualTo(second); + } +} diff --git a/src/test/java/com/digitalsanctuary/spring/user/persistence/model/RolePrivilegeUniquenessTest.java b/src/test/java/com/digitalsanctuary/spring/user/persistence/model/RolePrivilegeUniquenessTest.java new file mode 100644 index 00000000..ba7b8183 --- /dev/null +++ b/src/test/java/com/digitalsanctuary/spring/user/persistence/model/RolePrivilegeUniquenessTest.java @@ -0,0 +1,49 @@ +package com.digitalsanctuary.spring.user.persistence.model; + +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.jpa.test.autoconfigure.TestEntityManager; +import org.springframework.dao.DataIntegrityViolationException; +import com.digitalsanctuary.spring.user.persistence.repository.PrivilegeRepository; +import com.digitalsanctuary.spring.user.persistence.repository.RoleRepository; +import com.digitalsanctuary.spring.user.test.annotations.DatabaseTest; + +/** + * Database-slice tests verifying that the UNIQUE + NOT NULL constraint on the {@code name} column of {@link Role} and + * {@link Privilege} is enforced by the schema. Two roles (or two privileges) with the same name must not coexist. + */ +@DatabaseTest +class RolePrivilegeUniquenessTest { + + @Autowired + private RoleRepository roleRepository; + + @Autowired + private PrivilegeRepository privilegeRepository; + + @Autowired + private TestEntityManager entityManager; + + @Test + void shouldRejectDuplicateRoleNameWhenFlushed() { + roleRepository.saveAndFlush(new Role("ROLE_DUPLICATE")); + entityManager.clear(); + + Role second = new Role("ROLE_DUPLICATE"); + + assertThatThrownBy(() -> roleRepository.saveAndFlush(second)) + .isInstanceOf(DataIntegrityViolationException.class); + } + + @Test + void shouldRejectDuplicatePrivilegeNameWhenFlushed() { + privilegeRepository.saveAndFlush(new Privilege("DUPLICATE_PRIVILEGE")); + entityManager.clear(); + + Privilege second = new Privilege("DUPLICATE_PRIVILEGE"); + + assertThatThrownBy(() -> privilegeRepository.saveAndFlush(second)) + .isInstanceOf(DataIntegrityViolationException.class); + } +} diff --git a/src/test/java/com/digitalsanctuary/spring/user/persistence/model/TokenUniquenessTest.java b/src/test/java/com/digitalsanctuary/spring/user/persistence/model/TokenUniquenessTest.java new file mode 100644 index 00000000..d338b387 --- /dev/null +++ b/src/test/java/com/digitalsanctuary/spring/user/persistence/model/TokenUniquenessTest.java @@ -0,0 +1,64 @@ +package com.digitalsanctuary.spring.user.persistence.model; + +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.jpa.test.autoconfigure.TestEntityManager; +import org.springframework.dao.DataIntegrityViolationException; +import com.digitalsanctuary.spring.user.persistence.repository.PasswordResetTokenRepository; +import com.digitalsanctuary.spring.user.persistence.repository.VerificationTokenRepository; +import com.digitalsanctuary.spring.user.test.annotations.DatabaseTest; +import com.digitalsanctuary.spring.user.test.builders.UserTestDataBuilder; + +/** + * Database-slice tests verifying that the UNIQUE + NOT NULL constraint on the {@code token} column + * of {@link PasswordResetToken} and {@link VerificationToken} is enforced by the schema. Two tokens + * with the same value must not coexist in either table. + */ +@DatabaseTest +class TokenUniquenessTest { + + @Autowired + private PasswordResetTokenRepository passwordResetTokenRepository; + + @Autowired + private VerificationTokenRepository verificationTokenRepository; + + @Autowired + private TestEntityManager entityManager; + + private User persistUser(String email) { + User user = UserTestDataBuilder.aUser().withId(null).withEmail(email).build(); + return entityManager.persistAndFlush(user); + } + + @Test + void shouldRejectDuplicateTokenValueWhenPasswordResetTokenIsFlushed() { + User user1 = persistUser("prt-unique1@test.com"); + User user2 = persistUser("prt-unique2@test.com"); + String duplicateToken = "duplicate-hash-value-prt"; + + PasswordResetToken first = new PasswordResetToken(duplicateToken, user1); + passwordResetTokenRepository.saveAndFlush(first); + + PasswordResetToken second = new PasswordResetToken(duplicateToken, user2); + + assertThatThrownBy(() -> passwordResetTokenRepository.saveAndFlush(second)) + .isInstanceOf(DataIntegrityViolationException.class); + } + + @Test + void shouldRejectDuplicateTokenValueWhenVerificationTokenIsFlushed() { + User user1 = persistUser("vt-unique1@test.com"); + User user2 = persistUser("vt-unique2@test.com"); + String duplicateToken = "duplicate-hash-value-vt"; + + VerificationToken first = new VerificationToken(duplicateToken, user1); + verificationTokenRepository.saveAndFlush(first); + + VerificationToken second = new VerificationToken(duplicateToken, user2); + + assertThatThrownBy(() -> verificationTokenRepository.saveAndFlush(second)) + .isInstanceOf(DataIntegrityViolationException.class); + } +} diff --git a/src/test/java/com/digitalsanctuary/spring/user/persistence/repository/UserRepositoryEntityGraphTest.java b/src/test/java/com/digitalsanctuary/spring/user/persistence/repository/UserRepositoryEntityGraphTest.java new file mode 100644 index 00000000..35d473d8 --- /dev/null +++ b/src/test/java/com/digitalsanctuary/spring/user/persistence/repository/UserRepositoryEntityGraphTest.java @@ -0,0 +1,141 @@ +package com.digitalsanctuary.spring.user.persistence.repository; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; +import java.util.HashSet; +import java.util.Set; +import org.hibernate.Hibernate; +import org.hibernate.SessionFactory; +import org.hibernate.stat.Statistics; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.jpa.test.autoconfigure.TestEntityManager; +import com.digitalsanctuary.spring.user.persistence.model.Privilege; +import com.digitalsanctuary.spring.user.persistence.model.Role; +import com.digitalsanctuary.spring.user.persistence.model.User; +import com.digitalsanctuary.spring.user.test.annotations.DatabaseTest; +import com.digitalsanctuary.spring.user.test.builders.UserTestDataBuilder; +import jakarta.persistence.EntityManager; + +/** + * Database-slice tests verifying that {@link UserRepository#findWithRolesByEmail(String)} eagerly loads the + * User → roles → privileges graph via {@code @EntityGraph} in a bounded number of queries, while the plain + * {@link UserRepository#findByEmail(String)} leaves the now-LAZY collections uninitialized. This is the regression guard + * for the EAGER → LAZY switch on {@code User.roles} and {@code Role.privileges}. + */ +@DatabaseTest +class UserRepositoryEntityGraphTest { + + @Autowired + private UserRepository userRepository; + + @Autowired + private TestEntityManager entityManager; + + private void persistUserWithRolesAndPrivileges(String email) { + Privilege read = new Privilege("READ_PRIVILEGE"); + Privilege write = new Privilege("WRITE_PRIVILEGE"); + entityManager.persist(read); + entityManager.persist(write); + + Role userRole = new Role("ROLE_USER"); + Set userPrivileges = new HashSet<>(); + userPrivileges.add(read); + userRole.setPrivileges(userPrivileges); + + Role adminRole = new Role("ROLE_ADMIN"); + Set adminPrivileges = new HashSet<>(); + adminPrivileges.add(read); + adminPrivileges.add(write); + adminRole.setPrivileges(adminPrivileges); + + entityManager.persist(userRole); + entityManager.persist(adminRole); + + Set roles = new HashSet<>(); + roles.add(userRole); + roles.add(adminRole); + User user = UserTestDataBuilder.aUser().withId(null).withEmail(email).build(); + user.setRolesAsSet(roles); + entityManager.persist(user); + entityManager.flush(); + entityManager.clear(); + } + + private Statistics statistics() { + EntityManager em = entityManager.getEntityManager(); + Statistics stats = em.getEntityManagerFactory().unwrap(SessionFactory.class).getStatistics(); + stats.setStatisticsEnabled(true); + return stats; + } + + @Test + void shouldInitializeRolesAndPrivilegesWhenLoadedViaEntityGraphFinder() { + persistUserWithRolesAndPrivileges("graph@test.com"); + + User user = userRepository.findWithRolesByEmail("graph@test.com"); + + // Detach so any later access would hit a closed session if the graph were not initialized. + entityManager.getEntityManager().detach(user); + + assertThat(user).isNotNull(); + assertThat(Hibernate.isInitialized(user.getRolesAsSet())).isTrue(); + for (Role role : user.getRolesAsSet()) { + assertThat(Hibernate.isInitialized(role.getPrivileges())).isTrue(); + } + } + + @Test + void shouldAccessRolesAndPrivilegesWithoutLazyInitializationExceptionAfterDetach() { + persistUserWithRolesAndPrivileges("detached@test.com"); + + User user = userRepository.findWithRolesByEmail("detached@test.com"); + entityManager.getEntityManager().detach(user); + + // Traversing the full graph on a detached entity must not throw LazyInitializationException. + assertThatCode(() -> { + for (Role role : user.getRolesAsSet()) { + role.getName(); + for (Privilege privilege : role.getPrivileges()) { + privilege.getName(); + } + } + }).doesNotThrowAnyException(); + + assertThat(user.getRolesAsSet()) + .extracting(Role::getName) + .containsExactlyInAnyOrder("ROLE_USER", "ROLE_ADMIN"); + } + + @Test + void shouldNotInitializeRolesWhenLoadedViaPlainFindByEmail() { + persistUserWithRolesAndPrivileges("lazy@test.com"); + + User user = userRepository.findByEmail("lazy@test.com"); + + // The plain finder must leave roles uninitialized, proving the EAGER -> LAZY switch took effect. + assertThat(Hibernate.isInitialized(user.getRolesAsSet())).isFalse(); + } + + @Test + void shouldLoadRolesAndPrivilegesInBoundedQueryCountViaEntityGraphFinder() { + persistUserWithRolesAndPrivileges("bounded@test.com"); + + Statistics stats = statistics(); + stats.clear(); + + User user = userRepository.findWithRolesByEmail("bounded@test.com"); + // Force full traversal to prove no additional (N+1) queries are issued for privileges. + int privilegeCount = 0; + for (Role role : user.getRolesAsSet()) { + privilegeCount += role.getPrivileges().size(); + } + + assertThat(privilegeCount).isPositive(); + // A single entity-graph fetch should not degenerate into a per-role / per-privilege N+1 explosion. + // Allow a small bound to tolerate join-table fetch strategy differences across Hibernate versions. + assertThat(stats.getPrepareStatementCount()) + .as("EntityGraph finder should load roles + privileges in a bounded number of queries") + .isLessThanOrEqualTo(3); + } +} diff --git a/src/test/java/com/digitalsanctuary/spring/user/roles/RolePrivilegeSetupServiceTest.java b/src/test/java/com/digitalsanctuary/spring/user/roles/RolePrivilegeSetupServiceTest.java index bcc51ae3..41ecf678 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/roles/RolePrivilegeSetupServiceTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/roles/RolePrivilegeSetupServiceTest.java @@ -3,14 +3,18 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; -import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.Mockito.*; +import static org.mockito.Mockito.lenient; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; import java.util.Arrays; import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.Set; import org.junit.jupiter.api.BeforeEach; @@ -22,6 +26,7 @@ import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; import org.springframework.context.event.ContextRefreshedEvent; +import org.springframework.dao.DataIntegrityViolationException; import com.digitalsanctuary.spring.user.persistence.model.Privilege; import com.digitalsanctuary.spring.user.persistence.model.Role; @@ -49,10 +54,14 @@ class RolePrivilegeSetupServiceTest { @BeforeEach void setUp() { rolePrivilegeSetupService = new RolePrivilegeSetupService( - rolesAndPrivilegesConfig, - roleRepository, + rolesAndPrivilegesConfig, + roleRepository, privilegeRepository ); + // In production the REQUIRES_NEW insert methods are routed through the Spring proxy. In this unit test there is + // no proxy, so point the self-reference at the bean itself: a direct call still exercises the catch/re-read + // branch we care about (the transactional propagation is a runtime concern verified by integration tests). + rolePrivilegeSetupService.setSelf(rolePrivilegeSetupService); } @Nested @@ -66,23 +75,22 @@ void shouldSetupRolesAndPrivilegesOnFirstContextRefresh() { Map> rolesAndPrivileges = new HashMap<>(); rolesAndPrivileges.put("ROLE_ADMIN", Arrays.asList("READ_PRIVILEGE", "WRITE_PRIVILEGE", "DELETE_PRIVILEGE")); rolesAndPrivileges.put("ROLE_USER", Arrays.asList("READ_PRIVILEGE")); - + when(rolesAndPrivilegesConfig.getRolesAndPrivileges()).thenReturn(rolesAndPrivileges); when(privilegeRepository.findByName(anyString())).thenReturn(null); - when(privilegeRepository.save(any(Privilege.class))).thenAnswer(invocation -> invocation.getArgument(0)); + when(privilegeRepository.saveAndFlush(any(Privilege.class))).thenAnswer(invocation -> invocation.getArgument(0)); when(roleRepository.findByName(anyString())).thenReturn(null); - when(roleRepository.save(any(Role.class))).thenAnswer(invocation -> invocation.getArgument(0)); + when(roleRepository.saveAndFlush(any(Role.class))).thenAnswer(invocation -> invocation.getArgument(0)); // When rolePrivilegeSetupService.onApplicationEvent(contextRefreshedEvent); // Then - // READ_PRIVILEGE appears twice (for ROLE_ADMIN and ROLE_USER), so 4 total lookups - verify(privilegeRepository, times(4)).findByName(anyString()); - // All 4 privileges will be saved (READ is saved twice because findByName returns null each time) - verify(privilegeRepository, times(4)).save(any(Privilege.class)); + // 4 privilege creations (READ appears for both roles and findByName always returns null, so it is created + // each time it is requested in getOrCreatePrivilege): 3 for ROLE_ADMIN + 1 for ROLE_USER. + verify(privilegeRepository, times(4)).saveAndFlush(any(Privilege.class)); verify(roleRepository, times(2)).findByName(anyString()); - verify(roleRepository, times(2)).save(any(Role.class)); + verify(roleRepository, times(2)).saveAndFlush(any(Role.class)); assertThat(rolePrivilegeSetupService.isAlreadySetup()).isTrue(); } @@ -123,9 +131,11 @@ void shouldSkipNullRoleNamesInConfiguration() { Map> rolesAndPrivileges = new HashMap<>(); rolesAndPrivileges.put(null, Arrays.asList("READ_PRIVILEGE")); rolesAndPrivileges.put("ROLE_USER", Arrays.asList("READ_PRIVILEGE")); - + when(rolesAndPrivilegesConfig.getRolesAndPrivileges()).thenReturn(rolesAndPrivileges); when(privilegeRepository.findByName("READ_PRIVILEGE")).thenReturn(new Privilege("READ_PRIVILEGE")); + when(roleRepository.findByName("ROLE_USER")).thenReturn(null); + when(roleRepository.saveAndFlush(any(Role.class))).thenAnswer(invocation -> invocation.getArgument(0)); // When rolePrivilegeSetupService.onApplicationEvent(contextRefreshedEvent); @@ -142,9 +152,11 @@ void shouldSkipRolesWithNullPrivilegeLists() { Map> rolesAndPrivileges = new HashMap<>(); rolesAndPrivileges.put("ROLE_ADMIN", null); rolesAndPrivileges.put("ROLE_USER", Arrays.asList("READ_PRIVILEGE")); - + when(rolesAndPrivilegesConfig.getRolesAndPrivileges()).thenReturn(rolesAndPrivileges); when(privilegeRepository.findByName("READ_PRIVILEGE")).thenReturn(new Privilege("READ_PRIVILEGE")); + when(roleRepository.findByName("ROLE_USER")).thenReturn(null); + when(roleRepository.saveAndFlush(any(Role.class))).thenAnswer(invocation -> invocation.getArgument(0)); // When rolePrivilegeSetupService.onApplicationEvent(contextRefreshedEvent); @@ -167,7 +179,7 @@ void shouldCreateNewPrivilegeWhenNotExists() { when(privilegeRepository.findByName(privilegeName)).thenReturn(null); Privilege savedPrivilege = new Privilege(privilegeName); savedPrivilege.setId(1L); - when(privilegeRepository.save(any(Privilege.class))).thenReturn(savedPrivilege); + when(privilegeRepository.saveAndFlush(any(Privilege.class))).thenReturn(savedPrivilege); // When Privilege result = rolePrivilegeSetupService.getOrCreatePrivilege(privilegeName); @@ -177,7 +189,7 @@ void shouldCreateNewPrivilegeWhenNotExists() { assertThat(result.getName()).isEqualTo(privilegeName); assertThat(result.getId()).isEqualTo(1L); verify(privilegeRepository).findByName(privilegeName); - verify(privilegeRepository).save(any(Privilege.class)); + verify(privilegeRepository).saveAndFlush(any(Privilege.class)); } @Test @@ -195,31 +207,48 @@ void shouldReturnExistingPrivilegeWhenExists() { // Then assertThat(result).isEqualTo(existingPrivilege); verify(privilegeRepository).findByName(privilegeName); - verify(privilegeRepository, never()).save(any(Privilege.class)); + verify(privilegeRepository, never()).saveAndFlush(any(Privilege.class)); } @Test - @DisplayName("Should handle multiple privileges efficiently") - void shouldHandleMultiplePrivilegesEfficiently() { - // Given - List privilegeNames = Arrays.asList("READ", "WRITE", "DELETE", "UPDATE"); - when(privilegeRepository.findByName(anyString())).thenReturn(null); - when(privilegeRepository.save(any(Privilege.class))).thenAnswer(invocation -> { - Privilege p = invocation.getArgument(0); - p.setId((long) privilegeNames.indexOf(p.getName()) + 1); - return p; - }); + @DisplayName("Should return existing privilege when a concurrent node inserted it (constraint race)") + void shouldReturnExistingPrivilegeWhenConcurrentInsertViolatesConstraint() { + // Given: first findByName misses (privilege not yet visible), insert hits the unique constraint because a + // concurrent node committed it first, then the re-read finds the winning node's row. + String privilegeName = "READ_PRIVILEGE"; + Privilege concurrentPrivilege = new Privilege(privilegeName); + concurrentPrivilege.setId(42L); + when(privilegeRepository.findByName(privilegeName)) + .thenReturn(null) + .thenReturn(concurrentPrivilege); + when(privilegeRepository.saveAndFlush(any(Privilege.class))) + .thenThrow(new DataIntegrityViolationException("unique violation on privilege.name")); // When - Set privileges = new HashSet<>(); - for (String name : privilegeNames) { - privileges.add(rolePrivilegeSetupService.getOrCreatePrivilege(name)); - } + Privilege result = rolePrivilegeSetupService.getOrCreatePrivilege(privilegeName); - // Then - assertThat(privileges).hasSize(4); - verify(privilegeRepository, times(4)).findByName(anyString()); - verify(privilegeRepository, times(4)).save(any(Privilege.class)); + // Then: no exception propagated, the existing row is returned, no duplicate created. + assertThat(result).isSameAs(concurrentPrivilege); + verify(privilegeRepository, times(2)).findByName(privilegeName); + verify(privilegeRepository, times(1)).saveAndFlush(any(Privilege.class)); + } + + @Test + @DisplayName("Should rethrow when insert fails and re-read still finds nothing") + void shouldRethrowWhenInsertFailsAndReReadFindsNothing() { + // Given: a genuine integrity problem (not a name race) — the row is still absent after the failed insert. + String privilegeName = "READ_PRIVILEGE"; + when(privilegeRepository.findByName(privilegeName)).thenReturn(null); + DataIntegrityViolationException failure = new DataIntegrityViolationException("not a name conflict"); + when(privilegeRepository.saveAndFlush(any(Privilege.class))).thenThrow(failure); + + // When / Then + try { + rolePrivilegeSetupService.getOrCreatePrivilege(privilegeName); + org.assertj.core.api.Assertions.failBecauseExceptionWasNotThrown(DataIntegrityViolationException.class); + } catch (DataIntegrityViolationException e) { + assertThat(e).isSameAs(failure); + } } } @@ -232,26 +261,29 @@ class RoleManagementTests { void shouldCreateNewRoleWhenNotExists() { // Given String roleName = "ROLE_ADMIN"; - Set privileges = new HashSet<>(); - privileges.add(new Privilege("READ_PRIVILEGE")); - privileges.add(new Privilege("WRITE_PRIVILEGE")); - + Privilege readPriv = new Privilege("READ_PRIVILEGE"); + readPriv.setId(10L); + Privilege writePriv = new Privilege("WRITE_PRIVILEGE"); + writePriv.setId(11L); + when(privilegeRepository.findByName("READ_PRIVILEGE")).thenReturn(readPriv); + when(privilegeRepository.findByName("WRITE_PRIVILEGE")).thenReturn(writePriv); + when(roleRepository.findByName(roleName)).thenReturn(null); - when(roleRepository.save(any(Role.class))).thenAnswer(invocation -> { + when(roleRepository.saveAndFlush(any(Role.class))).thenAnswer(invocation -> { Role r = invocation.getArgument(0); r.setId(1L); return r; }); // When - Role result = rolePrivilegeSetupService.getOrCreateRole(roleName, privileges); + Role result = rolePrivilegeSetupService.getOrCreateRole(roleName, new HashSet<>(Arrays.asList("READ_PRIVILEGE", "WRITE_PRIVILEGE"))); // Then assertThat(result).isNotNull(); assertThat(result.getName()).isEqualTo(roleName); - assertThat(result.getPrivileges()).isEqualTo(privileges); + assertThat(result.getPrivileges()).containsExactlyInAnyOrder(readPriv, writePriv); verify(roleRepository).findByName(roleName); - verify(roleRepository).save(any(Role.class)); + verify(roleRepository).saveAndFlush(any(Role.class)); } @Test @@ -264,28 +296,28 @@ void shouldUpdateExistingRolePrivileges() { Set oldPrivileges = new HashSet<>(); oldPrivileges.add(new Privilege("OLD_PRIVILEGE")); existingRole.setPrivileges(oldPrivileges); - - Set newPrivileges = new HashSet<>(); + Privilege readPriv = new Privilege("READ_PRIVILEGE"); readPriv.setId(10L); Privilege writePriv = new Privilege("WRITE_PRIVILEGE"); writePriv.setId(11L); - newPrivileges.add(readPriv); - newPrivileges.add(writePriv); - + when(privilegeRepository.findByName("READ_PRIVILEGE")).thenReturn(readPriv); + when(privilegeRepository.findByName("WRITE_PRIVILEGE")).thenReturn(writePriv); + when(roleRepository.findByName(roleName)).thenReturn(existingRole); - when(roleRepository.save(any(Role.class))).thenAnswer(invocation -> invocation.getArgument(0)); + when(roleRepository.findById(2L)).thenReturn(Optional.of(existingRole)); + when(roleRepository.saveAndFlush(any(Role.class))).thenAnswer(invocation -> invocation.getArgument(0)); // When - Role result = rolePrivilegeSetupService.getOrCreateRole(roleName, newPrivileges); + Role result = rolePrivilegeSetupService.getOrCreateRole(roleName, new HashSet<>(Arrays.asList("READ_PRIVILEGE", "WRITE_PRIVILEGE"))); // Then // The role's privileges should be completely replaced with the new set assertThat(result).isSameAs(existingRole); - assertThat(result.getPrivileges()).isEqualTo(newPrivileges); + assertThat(result.getPrivileges()).containsExactlyInAnyOrder(readPriv, writePriv); assertThat(result.getPrivileges()).hasSize(2); verify(roleRepository).findByName(roleName); - verify(roleRepository).save(existingRole); + verify(roleRepository).saveAndFlush(existingRole); } @Test @@ -293,19 +325,47 @@ void shouldUpdateExistingRolePrivileges() { void shouldHandleRoleWithEmptyPrivilegeSet() { // Given String roleName = "ROLE_GUEST"; - Set emptyPrivileges = new HashSet<>(); - + when(roleRepository.findByName(roleName)).thenReturn(null); - when(roleRepository.save(any(Role.class))).thenAnswer(invocation -> invocation.getArgument(0)); + when(roleRepository.saveAndFlush(any(Role.class))).thenAnswer(invocation -> invocation.getArgument(0)); // When - Role result = rolePrivilegeSetupService.getOrCreateRole(roleName, emptyPrivileges); + Role result = rolePrivilegeSetupService.getOrCreateRole(roleName, new HashSet<>()); // Then assertThat(result).isNotNull(); assertThat(result.getName()).isEqualTo(roleName); assertThat(result.getPrivileges()).isEmpty(); - verify(roleRepository).save(any(Role.class)); + verify(roleRepository).saveAndFlush(any(Role.class)); + } + + @Test + @DisplayName("Should return existing role when a concurrent node inserted it (constraint race)") + void shouldReturnExistingRoleWhenConcurrentInsertViolatesConstraint() { + // Given: role not visible on first read, insert hits the unique constraint (concurrent node won), re-read + // finds the winning row, and we (re)assign privileges onto it. + String roleName = "ROLE_ADMIN"; + Privilege readPriv = new Privilege("READ_PRIVILEGE"); + when(privilegeRepository.findByName("READ_PRIVILEGE")).thenReturn(readPriv); + + Role concurrentRole = new Role(roleName); + concurrentRole.setId(99L); + + when(roleRepository.findByName(roleName)) + .thenReturn(null) + .thenReturn(concurrentRole); + when(roleRepository.findById(99L)).thenReturn(Optional.of(concurrentRole)); + when(roleRepository.saveAndFlush(any(Role.class))) + .thenThrow(new DataIntegrityViolationException("unique violation on role.name")) + .thenAnswer(invocation -> invocation.getArgument(0)); + + // When + Role result = rolePrivilegeSetupService.getOrCreateRole(roleName, new HashSet<>(Arrays.asList("READ_PRIVILEGE"))); + + // Then: no exception propagated, the existing row is returned with the desired privileges. + assertThat(result).isSameAs(concurrentRole); + assertThat(result.getPrivileges()).containsExactly(readPriv); + verify(roleRepository, times(2)).findByName(roleName); } } @@ -313,6 +373,39 @@ void shouldHandleRoleWithEmptyPrivilegeSet() { @DisplayName("Integration Tests") class IntegrationTests { + /** + * Wires the mocked repositories to behave like a simple in-memory store: {@code findByName} returns whatever + * was previously {@code saveAndFlush}-ed (assigning an id on first save), and {@code findById} returns the + * stored row. This lets the role insert/update path resolve privileges to "managed" instances by name, mirroring + * production where the privileges were created earlier in the same setup flow. + */ + private void wireInMemoryStore() { + final Map privileges = new HashMap<>(); + final Map roles = new HashMap<>(); + final Map rolesById = new HashMap<>(); + + lenient().when(privilegeRepository.findByName(anyString())).thenAnswer(inv -> privileges.get(inv.getArgument(0))); + lenient().when(privilegeRepository.saveAndFlush(any(Privilege.class))).thenAnswer(inv -> { + Privilege p = inv.getArgument(0); + if (p.getId() == null) { + p.setId((long) (privileges.size() + 1)); + } + privileges.put(p.getName(), p); + return p; + }); + lenient().when(roleRepository.findByName(anyString())).thenAnswer(inv -> roles.get(inv.getArgument(0))); + lenient().when(roleRepository.findById(any())).thenAnswer(inv -> Optional.ofNullable(rolesById.get(inv.getArgument(0)))); + lenient().when(roleRepository.saveAndFlush(any(Role.class))).thenAnswer(inv -> { + Role r = inv.getArgument(0); + if (r.getId() == null) { + r.setId((long) (roles.size() + 1)); + } + roles.put(r.getName(), r); + rolesById.put(r.getId(), r); + return r; + }); + } + @Test @DisplayName("Should handle complex role hierarchy setup") void shouldHandleComplexRoleHierarchySetup() { @@ -322,27 +415,24 @@ void shouldHandleComplexRoleHierarchySetup() { rolesAndPrivileges.put("ROLE_ADMIN", Arrays.asList("READ", "WRITE", "DELETE")); rolesAndPrivileges.put("ROLE_MODERATOR", Arrays.asList("READ", "WRITE")); rolesAndPrivileges.put("ROLE_USER", Arrays.asList("READ")); - + when(rolesAndPrivilegesConfig.getRolesAndPrivileges()).thenReturn(rolesAndPrivileges); - when(privilegeRepository.findByName(anyString())).thenReturn(null); - when(privilegeRepository.save(any(Privilege.class))).thenAnswer(invocation -> invocation.getArgument(0)); - when(roleRepository.findByName(anyString())).thenReturn(null); - when(roleRepository.save(any(Role.class))).thenAnswer(invocation -> invocation.getArgument(0)); + wireInMemoryStore(); // When rolePrivilegeSetupService.onApplicationEvent(contextRefreshedEvent); // Then - // Verify unique privileges are created (7 unique privileges total, but READ appears 4 times) + // 6 unique privileges are persisted exactly once each (READ/WRITE/DELETE are shared across roles). ArgumentCaptor privilegeCaptor = ArgumentCaptor.forClass(Privilege.class); - verify(privilegeRepository, times(9)).save(privilegeCaptor.capture()); // Total privilege saves - - // Verify all roles are created + verify(privilegeRepository, times(6)).saveAndFlush(privilegeCaptor.capture()); + assertThat(privilegeCaptor.getAllValues()).extracting(Privilege::getName) + .containsExactlyInAnyOrder("SUPER_READ", "SUPER_WRITE", "SUPER_DELETE", "READ", "WRITE", "DELETE"); + + // All 4 roles are created with their resolved privileges. ArgumentCaptor roleCaptor = ArgumentCaptor.forClass(Role.class); - verify(roleRepository, times(4)).save(roleCaptor.capture()); - - List savedRoles = roleCaptor.getAllValues(); - assertThat(savedRoles).extracting(Role::getName) + verify(roleRepository, times(4)).saveAndFlush(roleCaptor.capture()); + assertThat(roleCaptor.getAllValues()).extracting(Role::getName) .containsExactlyInAnyOrder("ROLE_SUPER_ADMIN", "ROLE_ADMIN", "ROLE_MODERATOR", "ROLE_USER"); } @@ -353,30 +443,19 @@ void shouldReuseExistingPrivilegesAcrossRoles() { Map> rolesAndPrivileges = new HashMap<>(); rolesAndPrivileges.put("ROLE_ADMIN", Arrays.asList("READ", "WRITE")); rolesAndPrivileges.put("ROLE_USER", Arrays.asList("READ")); - - Privilege readPrivilege = new Privilege("READ"); - readPrivilege.setId(1L); - + when(rolesAndPrivilegesConfig.getRolesAndPrivileges()).thenReturn(rolesAndPrivileges); - // First call returns null (creates), subsequent calls return existing - when(privilegeRepository.findByName("READ")) - .thenReturn(null) - .thenReturn(readPrivilege); - when(privilegeRepository.findByName("WRITE")).thenReturn(null); - when(privilegeRepository.save(any(Privilege.class))).thenAnswer(invocation -> invocation.getArgument(0)); - when(roleRepository.findByName(anyString())).thenReturn(null); - when(roleRepository.save(any(Role.class))).thenAnswer(invocation -> invocation.getArgument(0)); + wireInMemoryStore(); // When rolePrivilegeSetupService.onApplicationEvent(contextRefreshedEvent); // Then - // READ privilege should be looked up twice but only created once - verify(privilegeRepository, times(2)).findByName("READ"); - // WRITE privilege should be created once - verify(privilegeRepository, times(1)).findByName("WRITE"); - // Only 2 privileges should be saved (READ once, WRITE once) - verify(privilegeRepository, times(2)).save(any(Privilege.class)); + // READ and WRITE are each created exactly once; READ is reused for the second role. + ArgumentCaptor privilegeCaptor = ArgumentCaptor.forClass(Privilege.class); + verify(privilegeRepository, times(2)).saveAndFlush(privilegeCaptor.capture()); + assertThat(privilegeCaptor.getAllValues()).extracting(Privilege::getName) + .containsExactlyInAnyOrder("READ", "WRITE"); } @Test @@ -385,41 +464,24 @@ void shouldHandleDatabaseTransactionProperly() { // Given Map> rolesAndPrivileges = new HashMap<>(); rolesAndPrivileges.put("ROLE_TRANSACTIONAL", Arrays.asList("TRANSACT_READ", "TRANSACT_WRITE")); - + when(rolesAndPrivilegesConfig.getRolesAndPrivileges()).thenReturn(rolesAndPrivileges); - when(privilegeRepository.findByName(anyString())).thenReturn(null); - - // Track created privileges to return the same instance when saved - Map createdPrivileges = new HashMap<>(); - when(privilegeRepository.save(any(Privilege.class))).thenAnswer(invocation -> { - Privilege p = invocation.getArgument(0); - p.setId((long) (createdPrivileges.size() + 1)); // Simulate DB generating ID - createdPrivileges.put(p.getName(), p); - return p; - }); - - when(roleRepository.findByName(anyString())).thenReturn(null); - when(roleRepository.save(any(Role.class))).thenAnswer(invocation -> { - Role r = invocation.getArgument(0); - r.setId(1L); - return r; - }); + wireInMemoryStore(); // When rolePrivilegeSetupService.onApplicationEvent(contextRefreshedEvent); // Then ArgumentCaptor roleCaptor = ArgumentCaptor.forClass(Role.class); - verify(roleRepository).save(roleCaptor.capture()); - + verify(roleRepository).saveAndFlush(roleCaptor.capture()); + Role savedRole = roleCaptor.getValue(); // The privileges are stored in a Set, so we should have 2 unique privileges assertThat(savedRole.getPrivileges()).hasSize(2); - // Verify the privileges have their names set correctly assertThat(savedRole.getPrivileges()) .extracting(Privilege::getName) .containsExactlyInAnyOrder("TRANSACT_READ", "TRANSACT_WRITE"); - // Verify the privileges have IDs assigned + // Verify the privileges have IDs assigned (they were resolved as persisted, managed entities) assertThat(savedRole.getPrivileges()).allMatch(p -> p.getId() != null && p.getId() > 0); } @@ -430,27 +492,50 @@ void shouldCompleteSetupWithMixedExistingAndNewEntities() { Map> rolesAndPrivileges = new HashMap<>(); rolesAndPrivileges.put("ROLE_EXISTING", Arrays.asList("EXISTING_PRIV", "NEW_PRIV")); rolesAndPrivileges.put("ROLE_NEW", Arrays.asList("EXISTING_PRIV")); - + Privilege existingPriv = new Privilege("EXISTING_PRIV"); existingPriv.setId(100L); Role existingRole = new Role("ROLE_EXISTING"); existingRole.setId(200L); - + when(rolesAndPrivilegesConfig.getRolesAndPrivileges()).thenReturn(rolesAndPrivileges); - when(privilegeRepository.findByName("EXISTING_PRIV")).thenReturn(existingPriv); - when(privilegeRepository.findByName("NEW_PRIV")).thenReturn(null); - when(privilegeRepository.save(any(Privilege.class))).thenAnswer(invocation -> invocation.getArgument(0)); - when(roleRepository.findByName("ROLE_EXISTING")).thenReturn(existingRole); - when(roleRepository.findByName("ROLE_NEW")).thenReturn(null); - when(roleRepository.save(any(Role.class))).thenAnswer(invocation -> invocation.getArgument(0)); + // Pre-seed an in-memory store with one existing privilege and one existing role. + final Map privileges = new HashMap<>(); + privileges.put("EXISTING_PRIV", existingPriv); + final Map roles = new HashMap<>(); + roles.put("ROLE_EXISTING", existingRole); + final Map rolesById = new HashMap<>(); + rolesById.put(200L, existingRole); + + when(privilegeRepository.findByName(anyString())).thenAnswer(inv -> privileges.get(inv.getArgument(0))); + when(privilegeRepository.saveAndFlush(any(Privilege.class))).thenAnswer(inv -> { + Privilege p = inv.getArgument(0); + if (p.getId() == null) { + p.setId((long) (privileges.size() + 1)); + } + privileges.put(p.getName(), p); + return p; + }); + when(roleRepository.findByName(anyString())).thenAnswer(inv -> roles.get(inv.getArgument(0))); + when(roleRepository.findById(any())).thenAnswer(inv -> Optional.ofNullable(rolesById.get(inv.getArgument(0)))); + when(roleRepository.saveAndFlush(any(Role.class))).thenAnswer(inv -> { + Role r = inv.getArgument(0); + if (r.getId() == null) { + r.setId((long) (roles.size() + 1)); + } + roles.put(r.getName(), r); + rolesById.put(r.getId(), r); + return r; + }); // When rolePrivilegeSetupService.onApplicationEvent(contextRefreshedEvent); // Then - verify(privilegeRepository, times(1)).save(any(Privilege.class)); // Only NEW_PRIV - verify(roleRepository, times(2)).save(any(Role.class)); // Both roles get saved (existing one with updated privileges) + verify(privilegeRepository, times(1)).saveAndFlush(any(Privilege.class)); // Only NEW_PRIV created + verify(roleRepository, times(2)).saveAndFlush(any(Role.class)); // Both roles saved (existing one updated) + verify(roleRepository, times(1)).findById(200L); // existing role re-read via REQUIRES_NEW update path assertThat(rolePrivilegeSetupService.isAlreadySetup()).isTrue(); } } -} \ No newline at end of file +} diff --git a/src/test/java/com/digitalsanctuary/spring/user/service/DSOAuth2UserServiceRegistrationGuardTest.java b/src/test/java/com/digitalsanctuary/spring/user/service/DSOAuth2UserServiceRegistrationGuardTest.java index 90c91401..00ec2ec5 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/service/DSOAuth2UserServiceRegistrationGuardTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/service/DSOAuth2UserServiceRegistrationGuardTest.java @@ -77,7 +77,7 @@ void shouldRejectNewOAuth2UserWhenGuardDenies() { .withLastName("User") .build(); - when(userRepository.findByEmail("new@gmail.com")).thenReturn(null); + when(userRepository.findWithRolesByEmail("new@gmail.com")).thenReturn(null); doThrow(new RegistrationDeniedException("Domain not allowed")) .when(userService).enforceRegistrationGuard(eq("new@gmail.com"), eq(RegistrationSource.OAUTH2), anyString()); @@ -99,7 +99,7 @@ void shouldAllowNewOAuth2UserWhenGuardAllows() { .withLastName("User") .build(); - when(userRepository.findByEmail("allowed@gmail.com")).thenReturn(null); + when(userRepository.findWithRolesByEmail("allowed@gmail.com")).thenReturn(null); doNothing().when(userService) .enforceRegistrationGuard(eq("allowed@gmail.com"), eq(RegistrationSource.OAUTH2), anyString()); when(userRepository.save(any(User.class))).thenAnswer(invocation -> invocation.getArgument(0)); @@ -125,7 +125,7 @@ void shouldNotCallGuardForExistingOAuth2User() { existingUser.setEmail("existing@gmail.com"); existingUser.setProvider(User.Provider.GOOGLE); - when(userRepository.findByEmail("existing@gmail.com")).thenReturn(existingUser); + when(userRepository.findWithRolesByEmail("existing@gmail.com")).thenReturn(existingUser); when(userRepository.save(any(User.class))).thenAnswer(invocation -> invocation.getArgument(0)); User result = service.handleOAuthLoginSuccess("google", googleUser); diff --git a/src/test/java/com/digitalsanctuary/spring/user/service/DSOAuth2UserServiceTest.java b/src/test/java/com/digitalsanctuary/spring/user/service/DSOAuth2UserServiceTest.java index b1598f00..00c0595e 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/service/DSOAuth2UserServiceTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/service/DSOAuth2UserServiceTest.java @@ -90,7 +90,7 @@ void shouldCreateNewUserFromGoogleOAuth2() { .withLastName("Doe") .build(); - when(userRepository.findByEmail("john.doe@gmail.com")).thenReturn(null); + when(userRepository.findWithRolesByEmail("john.doe@gmail.com")).thenReturn(null); when(userRepository.save(any(User.class))).thenAnswer(invocation -> { User savedUser = invocation.getArgument(0); savedUser.setId(123L); @@ -121,8 +121,8 @@ void shouldCreateNewUserFromGoogleOAuth2() { // can observe OAuth2 registrations the same way they observe form registrations. ArgumentCaptor regCaptor = ArgumentCaptor.forClass(OnRegistrationCompleteEvent.class); verify(eventPublisher).publishEvent(regCaptor.capture()); - assertThat(regCaptor.getValue().getUser().getEmail()).isEqualTo("john.doe@gmail.com"); - assertThat(regCaptor.getValue().getUser().isEnabled()).isTrue(); + assertThat(regCaptor.getValue().getUserEmail()).isEqualTo("john.doe@gmail.com"); + assertThat(regCaptor.getValue().isUserEnabled()).isTrue(); } @Test @@ -143,7 +143,7 @@ void shouldUpdateExistingGoogleUser() { existingUser.setProvider(User.Provider.GOOGLE); existingUser.setEnabled(true); - when(userRepository.findByEmail("existing@gmail.com")).thenReturn(existingUser); + when(userRepository.findWithRolesByEmail("existing@gmail.com")).thenReturn(existingUser); when(userRepository.save(any(User.class))).thenAnswer(invocation -> invocation.getArgument(0)); // When @@ -169,7 +169,7 @@ void shouldHandleGoogleUserWithMissingFields() { .withoutAttribute("family_name") .build(); - when(userRepository.findByEmail("nolastname@gmail.com")).thenReturn(null); + when(userRepository.findWithRolesByEmail("nolastname@gmail.com")).thenReturn(null); when(userRepository.save(any(User.class))).thenAnswer(invocation -> invocation.getArgument(0)); // When @@ -191,14 +191,14 @@ void shouldConvertEmailToLowercase() { .withEmail("John.Doe@GMAIL.COM") .build(); - when(userRepository.findByEmail("john.doe@gmail.com")).thenReturn(null); + when(userRepository.findWithRolesByEmail("john.doe@gmail.com")).thenReturn(null); when(userRepository.save(any(User.class))).thenAnswer(invocation -> invocation.getArgument(0)); // When service.handleOAuthLoginSuccess("google", googleUser); // Then - verify(userRepository).findByEmail("john.doe@gmail.com"); // Lowercase lookup + verify(userRepository).findWithRolesByEmail("john.doe@gmail.com"); // Lowercase lookup } } @@ -215,7 +215,7 @@ void shouldAcceptWhenEmailVerifiedBooleanTrue() { .withAttribute("email_verified", Boolean.TRUE) .build(); - when(userRepository.findByEmail("verified@gmail.com")).thenReturn(null); + when(userRepository.findWithRolesByEmail("verified@gmail.com")).thenReturn(null); when(userRepository.save(any(User.class))).thenAnswer(invocation -> invocation.getArgument(0)); // When @@ -235,7 +235,7 @@ void shouldAcceptWhenEmailVerifiedStringTrue() { .withAttribute("email_verified", "true") .build(); - when(userRepository.findByEmail("verified-str@gmail.com")).thenReturn(null); + when(userRepository.findWithRolesByEmail("verified-str@gmail.com")).thenReturn(null); when(userRepository.save(any(User.class))).thenAnswer(invocation -> invocation.getArgument(0)); // When @@ -255,7 +255,7 @@ void shouldAcceptWhenEmailVerifiedAbsent() { .withoutAttribute("email_verified") .build(); - when(userRepository.findByEmail("noclaim@gmail.com")).thenReturn(null); + when(userRepository.findWithRolesByEmail("noclaim@gmail.com")).thenReturn(null); when(userRepository.save(any(User.class))).thenAnswer(invocation -> invocation.getArgument(0)); // When @@ -314,7 +314,7 @@ void shouldCreateNewUserFromFacebookOAuth2() { .withFullName("Jane Marie Smith") .build(); - when(userRepository.findByEmail("jane.smith@facebook.com")).thenReturn(null); + when(userRepository.findWithRolesByEmail("jane.smith@facebook.com")).thenReturn(null); when(userRepository.save(any(User.class))).thenAnswer(invocation -> invocation.getArgument(0)); // When @@ -363,7 +363,7 @@ void shouldHandleFacebookUserWithoutName() { .withoutAttribute("name") .build(); - when(userRepository.findByEmail("noname@facebook.com")).thenReturn(null); + when(userRepository.findWithRolesByEmail("noname@facebook.com")).thenReturn(null); when(userRepository.save(any(User.class))).thenAnswer(invocation -> invocation.getArgument(0)); // When @@ -393,7 +393,7 @@ void shouldRejectGoogleLoginForFacebookUser() { existingUser.setEmail("conflict@example.com"); existingUser.setProvider(User.Provider.FACEBOOK); - when(userRepository.findByEmail("conflict@example.com")).thenReturn(existingUser); + when(userRepository.findWithRolesByEmail("conflict@example.com")).thenReturn(existingUser); // When/Then assertThatThrownBy(() -> service.handleOAuthLoginSuccess("google", googleUser)) @@ -413,7 +413,7 @@ void shouldRejectOAuth2LoginForLocalUser() { existingUser.setEmail("local@example.com"); existingUser.setProvider(User.Provider.LOCAL); - when(userRepository.findByEmail("local@example.com")).thenReturn(existingUser); + when(userRepository.findWithRolesByEmail("local@example.com")).thenReturn(existingUser); // When/Then assertThatThrownBy(() -> service.handleOAuthLoginSuccess("google", googleUser)) @@ -434,7 +434,7 @@ void shouldAllowSameProviderReAuthentication() { existingUser.setProvider(User.Provider.GOOGLE); existingUser.setFirstName("Existing"); - when(userRepository.findByEmail("same@gmail.com")).thenReturn(existingUser); + when(userRepository.findWithRolesByEmail("same@gmail.com")).thenReturn(existingUser); when(userRepository.save(any(User.class))).thenAnswer(invocation -> invocation.getArgument(0)); // When - Should not throw exception @@ -525,7 +525,7 @@ void shouldLoadUserThroughOAuth2RequestFlow() { DSUserDetails mockUserDetails = mock(DSUserDetails.class); when(loginHelperService.userLoginHelper(any(User.class), ArgumentMatchers.>any())).thenReturn(mockUserDetails); - when(userRepository.findByEmail("loadtest@gmail.com")).thenReturn(null); + when(userRepository.findWithRolesByEmail("loadtest@gmail.com")).thenReturn(null); when(userRepository.save(any(User.class))).thenAnswer(invocation -> invocation.getArgument(0)); // When diff --git a/src/test/java/com/digitalsanctuary/spring/user/service/DSOidcUserServiceRegistrationGuardTest.java b/src/test/java/com/digitalsanctuary/spring/user/service/DSOidcUserServiceRegistrationGuardTest.java index e09e334d..04299391 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/service/DSOidcUserServiceRegistrationGuardTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/service/DSOidcUserServiceRegistrationGuardTest.java @@ -77,7 +77,7 @@ void shouldRejectNewOidcUserWhenGuardDenies() { .withFamilyName("User") .build(); - when(userRepository.findByEmail("new@company.com")).thenReturn(null); + when(userRepository.findWithRolesByEmail("new@company.com")).thenReturn(null); doThrow(new RegistrationDeniedException("Organization not whitelisted")) .when(userService).enforceRegistrationGuard(eq("new@company.com"), eq(RegistrationSource.OIDC), anyString()); @@ -99,7 +99,7 @@ void shouldAllowNewOidcUserWhenGuardAllows() { .withFamilyName("User") .build(); - when(userRepository.findByEmail("allowed@company.com")).thenReturn(null); + when(userRepository.findWithRolesByEmail("allowed@company.com")).thenReturn(null); doNothing().when(userService) .enforceRegistrationGuard(eq("allowed@company.com"), eq(RegistrationSource.OIDC), anyString()); when(userRepository.save(any(User.class))).thenAnswer(invocation -> invocation.getArgument(0)); @@ -125,7 +125,7 @@ void shouldNotCallGuardForExistingOidcUser() { existingUser.setEmail("existing@company.com"); existingUser.setProvider(User.Provider.KEYCLOAK); - when(userRepository.findByEmail("existing@company.com")).thenReturn(existingUser); + when(userRepository.findWithRolesByEmail("existing@company.com")).thenReturn(existingUser); when(userRepository.save(any(User.class))).thenAnswer(invocation -> invocation.getArgument(0)); User result = service.handleOidcLoginSuccess("keycloak", keycloakUser); diff --git a/src/test/java/com/digitalsanctuary/spring/user/service/DSOidcUserServiceTest.java b/src/test/java/com/digitalsanctuary/spring/user/service/DSOidcUserServiceTest.java index 1733fa71..af00d0f5 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/service/DSOidcUserServiceTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/service/DSOidcUserServiceTest.java @@ -89,7 +89,7 @@ void shouldCreateNewUserFromKeycloakOidc() { .withPreferredUsername("jdoe") .build(); - when(userRepository.findByEmail("john.doe@company.com")).thenReturn(null); + when(userRepository.findWithRolesByEmail("john.doe@company.com")).thenReturn(null); when(userRepository.save(any(User.class))).thenAnswer(invocation -> { User savedUser = invocation.getArgument(0); savedUser.setId(123L); @@ -116,8 +116,8 @@ void shouldCreateNewUserFromKeycloakOidc() { org.mockito.ArgumentCaptor regCaptor = org.mockito.ArgumentCaptor.forClass(OnRegistrationCompleteEvent.class); verify(eventPublisher).publishEvent(regCaptor.capture()); - assertThat(regCaptor.getValue().getUser().getEmail()).isEqualTo("john.doe@company.com"); - assertThat(regCaptor.getValue().getUser().isEnabled()).isTrue(); + assertThat(regCaptor.getValue().getUserEmail()).isEqualTo("john.doe@company.com"); + assertThat(regCaptor.getValue().isUserEnabled()).isTrue(); } @Test @@ -160,7 +160,7 @@ void shouldUpdateExistingKeycloakUser() { existingUser.setProvider(User.Provider.KEYCLOAK); existingUser.setEnabled(true); - when(userRepository.findByEmail("existing@keycloak.com")).thenReturn(existingUser); + when(userRepository.findWithRolesByEmail("existing@keycloak.com")).thenReturn(existingUser); when(userRepository.save(any(User.class))).thenAnswer(invocation -> invocation.getArgument(0)); // When @@ -184,7 +184,7 @@ void shouldHandleKeycloakUserWithMinimalClaims() { .withoutUserInfoClaim("family_name") .build(); - when(userRepository.findByEmail("minimal@keycloak.com")).thenReturn(null); + when(userRepository.findWithRolesByEmail("minimal@keycloak.com")).thenReturn(null); when(userRepository.save(any(User.class))).thenAnswer(invocation -> invocation.getArgument(0)); // When @@ -233,7 +233,7 @@ void shouldAcceptWhenEmailVerifiedTrue() { .withUserInfoClaim("email_verified", true) .build(); - when(userRepository.findByEmail("verified@keycloak.com")).thenReturn(null); + when(userRepository.findWithRolesByEmail("verified@keycloak.com")).thenReturn(null); when(userRepository.save(any(User.class))).thenAnswer(invocation -> invocation.getArgument(0)); // When @@ -253,7 +253,7 @@ void shouldAcceptWhenEmailVerifiedAbsent() { .withoutUserInfoClaim("email_verified") .build(); - when(userRepository.findByEmail("noclaim@keycloak.com")).thenReturn(null); + when(userRepository.findWithRolesByEmail("noclaim@keycloak.com")).thenReturn(null); when(userRepository.save(any(User.class))).thenAnswer(invocation -> invocation.getArgument(0)); // When @@ -298,7 +298,7 @@ void shouldRejectKeycloakLoginForGoogleUser() { existingUser.setEmail("conflict@example.com"); existingUser.setProvider(User.Provider.GOOGLE); - when(userRepository.findByEmail("conflict@example.com")).thenReturn(existingUser); + when(userRepository.findWithRolesByEmail("conflict@example.com")).thenReturn(existingUser); // When/Then assertThatThrownBy(() -> service.handleOidcLoginSuccess("keycloak", keycloakUser)) @@ -318,7 +318,7 @@ void shouldRejectOidcLoginForLocalUser() { existingUser.setEmail("local@example.com"); existingUser.setProvider(User.Provider.LOCAL); - when(userRepository.findByEmail("local@example.com")).thenReturn(existingUser); + when(userRepository.findWithRolesByEmail("local@example.com")).thenReturn(existingUser); // When/Then assertThatThrownBy(() -> service.handleOidcLoginSuccess("keycloak", keycloakUser)) @@ -339,7 +339,7 @@ void shouldAllowSameProviderReAuthentication() { existingUser.setProvider(User.Provider.KEYCLOAK); existingUser.setFirstName("Existing"); - when(userRepository.findByEmail("same@keycloak.com")).thenReturn(existingUser); + when(userRepository.findWithRolesByEmail("same@keycloak.com")).thenReturn(existingUser); when(userRepository.save(any(User.class))).thenAnswer(invocation -> invocation.getArgument(0)); // When - Should not throw exception @@ -394,7 +394,7 @@ void shouldHandleRepositoryExceptionsDuringSave() { .withEmail("error@keycloak.com") .build(); - when(userRepository.findByEmail("error@keycloak.com")).thenReturn(null); + when(userRepository.findWithRolesByEmail("error@keycloak.com")).thenReturn(null); when(userRepository.save(any(User.class))) .thenThrow(new RuntimeException("Database connection failed")); @@ -438,7 +438,7 @@ void shouldLoadUserThroughOidcRequestFlow() { spyService.defaultOidcUserService = mock(OidcUserService.class); when(spyService.defaultOidcUserService.loadUser(userRequest)).thenReturn(keycloakUser); - when(userRepository.findByEmail("loadtest@keycloak.com")).thenReturn(null); + when(userRepository.findWithRolesByEmail("loadtest@keycloak.com")).thenReturn(null); when(userRepository.save(any(User.class))).thenAnswer(invocation -> { User user = invocation.getArgument(0); user.setId(999L); diff --git a/src/test/java/com/digitalsanctuary/spring/user/service/DSUserDetailsServiceTest.java b/src/test/java/com/digitalsanctuary/spring/user/service/DSUserDetailsServiceTest.java index d1d6f2fd..62c2e6ba 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/service/DSUserDetailsServiceTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/service/DSUserDetailsServiceTest.java @@ -78,7 +78,7 @@ void setUp() { void loadUserByUsername_withValidEmail_returnsUserDetails() { // Given String email = "test@example.com"; - when(userRepository.findByEmail(email)).thenReturn(testUser); + when(userRepository.findWithRolesByEmail(email)).thenReturn(testUser); when(loginHelperService.userLoginHelper(testUser)).thenReturn(mockUserDetails); // When @@ -93,7 +93,7 @@ void loadUserByUsername_withValidEmail_returnsUserDetails() { .containsExactly("ROLE_USER"); // Verify interactions - verify(userRepository).findByEmail(email); + verify(userRepository).findWithRolesByEmail(email); verify(loginHelperService).userLoginHelper(testUser); } @@ -102,7 +102,7 @@ void loadUserByUsername_withValidEmail_returnsUserDetails() { void loadUserByUsername_withNonExistentEmail_throwsException() { // Given String email = "nonexistent@example.com"; - when(userRepository.findByEmail(email)).thenReturn(null); + when(userRepository.findWithRolesByEmail(email)).thenReturn(null); // When & Then assertThatThrownBy(() -> dsUserDetailsService.loadUserByUsername(email)) @@ -117,14 +117,14 @@ void loadUserByUsername_withNonExistentEmail_throwsException() { @DisplayName("Should handle null email") void loadUserByUsername_withNullEmail_throwsException() { // Given - when(userRepository.findByEmail(null)).thenReturn(null); + when(userRepository.findWithRolesByEmail(null)).thenReturn(null); // When & Then assertThatThrownBy(() -> dsUserDetailsService.loadUserByUsername(null)) .isInstanceOf(UsernameNotFoundException.class) .hasMessage("No user found with email/username: null"); - verify(userRepository).findByEmail(null); + verify(userRepository).findWithRolesByEmail(null); verify(loginHelperService, never()).userLoginHelper(any()); } @@ -133,14 +133,14 @@ void loadUserByUsername_withNullEmail_throwsException() { void loadUserByUsername_withEmptyEmail_throwsException() { // Given String emptyEmail = ""; - when(userRepository.findByEmail(emptyEmail)).thenReturn(null); + when(userRepository.findWithRolesByEmail(emptyEmail)).thenReturn(null); // When & Then assertThatThrownBy(() -> dsUserDetailsService.loadUserByUsername(emptyEmail)) .isInstanceOf(UsernameNotFoundException.class) .hasMessage("No user found with email/username: "); - verify(userRepository).findByEmail(emptyEmail); + verify(userRepository).findWithRolesByEmail(emptyEmail); verify(loginHelperService, never()).userLoginHelper(any()); } @@ -153,7 +153,7 @@ void loadUserByUsername_withDisabledUser_returnsUserDetails() { .build(); DSUserDetails disabledUserDetails = new DSUserDetails(disabledUser, Collections.emptyList()); - when(userRepository.findByEmail("disabled@example.com")).thenReturn(disabledUser); + when(userRepository.findWithRolesByEmail("disabled@example.com")).thenReturn(disabledUser); when(loginHelperService.userLoginHelper(disabledUser)).thenReturn(disabledUserDetails); // When @@ -175,7 +175,7 @@ void loadUserByUsername_withLockedUser_returnsUserDetails() { DSUserDetails lockedUserDetails = new DSUserDetails(lockedUser, Arrays.asList(new SimpleGrantedAuthority("ROLE_USER"))); - when(userRepository.findByEmail("locked@example.com")).thenReturn(lockedUser); + when(userRepository.findWithRolesByEmail("locked@example.com")).thenReturn(lockedUser); when(loginHelperService.userLoginHelper(lockedUser)).thenReturn(lockedUserDetails); // When @@ -205,7 +205,7 @@ void loadUserByUsername_withMultipleRoles_returnsUserDetailsWithAllAuthorities() new SimpleGrantedAuthority("ROLE_ADMIN") )); - when(userRepository.findByEmail("multirole@example.com")).thenReturn(multiRoleUser); + when(userRepository.findWithRolesByEmail("multirole@example.com")).thenReturn(multiRoleUser); when(loginHelperService.userLoginHelper(multiRoleUser)).thenReturn(multiRoleUserDetails); // When @@ -229,7 +229,7 @@ void loadUserByUsername_withSpecialCharactersInEmail_returnsUserDetails() { DSUserDetails specialEmailUserDetails = new DSUserDetails(userWithSpecialEmail, Arrays.asList(new SimpleGrantedAuthority("ROLE_USER"))); - when(userRepository.findByEmail(specialEmail)).thenReturn(userWithSpecialEmail); + when(userRepository.findWithRolesByEmail(specialEmail)).thenReturn(userWithSpecialEmail); when(loginHelperService.userLoginHelper(userWithSpecialEmail)).thenReturn(specialEmailUserDetails); // When @@ -257,7 +257,7 @@ void loadUserByUsername_verifiesUserDetailsPropertiesMapping() { DSUserDetails userDetails = new DSUserDetails(user, Arrays.asList(new SimpleGrantedAuthority("ROLE_USER"))); - when(userRepository.findByEmail(email)).thenReturn(user); + when(userRepository.findWithRolesByEmail(email)).thenReturn(user); when(loginHelperService.userLoginHelper(user)).thenReturn(userDetails); // When @@ -284,7 +284,7 @@ void loadUserByUsername_verifiesCorrectUserPassedToLoginHelper() { .withEmail(email) .build(); - when(userRepository.findByEmail(email)).thenReturn(user); + when(userRepository.findWithRolesByEmail(email)).thenReturn(user); when(loginHelperService.userLoginHelper(any())).thenReturn(mockUserDetails); // When diff --git a/src/test/java/com/digitalsanctuary/spring/user/service/TokenHashingSecurityTest.java b/src/test/java/com/digitalsanctuary/spring/user/service/TokenHashingSecurityTest.java index de8d32f6..85c2ac53 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/service/TokenHashingSecurityTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/service/TokenHashingSecurityTest.java @@ -71,6 +71,8 @@ class PasswordResetTokenTests { @Mock private PasswordResetTokenRepository passwordTokenRepository; @Mock + private com.digitalsanctuary.spring.user.persistence.repository.UserRepository userRepository; + @Mock private ApplicationEventPublisher eventPublisher; @Mock private SessionInvalidationService sessionInvalidationService; @@ -80,7 +82,7 @@ class PasswordResetTokenTests { @BeforeEach void initService() { userEmailService = new UserEmailService(mailService, userVerificationService, passwordTokenRepository, - eventPublisher, sessionInvalidationService, tokenHasher); + userRepository, eventPublisher, sessionInvalidationService, tokenHasher); } @Test @@ -133,7 +135,7 @@ class PasswordResetLookupTests { @BeforeEach void initService() { - userService = new UserService(null, null, passwordTokenRepository, null, null, null, null, null, null, null, + userService = new UserService(null, null, passwordTokenRepository, null, null, null, null, null, null, null, null, null, tokenHasher, null); } diff --git a/src/test/java/com/digitalsanctuary/spring/user/service/UserEmailServiceTest.java b/src/test/java/com/digitalsanctuary/spring/user/service/UserEmailServiceTest.java index 987d1e7e..98aaa994 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/service/UserEmailServiceTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/service/UserEmailServiceTest.java @@ -44,6 +44,9 @@ class UserEmailServiceTest { @Mock private PasswordResetTokenRepository passwordTokenRepository; + @Mock + private com.digitalsanctuary.spring.user.persistence.repository.UserRepository userRepository; + @Mock private ApplicationEventPublisher eventPublisher; @@ -62,7 +65,7 @@ class UserEmailServiceTest { @BeforeEach void setUp() { userEmailService = new UserEmailService(mailService, userVerificationService, passwordTokenRepository, - eventPublisher, sessionInvalidationService, tokenHasher); + userRepository, eventPublisher, sessionInvalidationService, tokenHasher); // In production 'self' is the Spring proxy used to apply @Transactional on createPasswordResetTokenForUser. // There is no proxy in a unit test, so point it at the instance itself to exercise the real call path. ReflectionTestUtils.setField(userEmailService, "self", userEmailService); @@ -276,6 +279,54 @@ void sendRegistrationVerificationEmail_rejectsJavascriptUrl() { .hasMessageContaining("Invalid application URL"); } + @Test + @DisplayName("sendRegistrationVerificationEmail - by id reloads user and sends to correct recipient with a token") + void sendRegistrationVerificationEmail_byId_reloadsUserAndSendsEmail() { + // Given - the async listener carries only the user id; the service must reload the entity. + when(userRepository.findById(1L)).thenReturn(java.util.Optional.of(testUser)); + + // When + userEmailService.sendRegistrationVerificationEmail(1L, appUrl); + + // Then - a verification token is created bound to the reloaded user + ArgumentCaptor tokenCaptor = ArgumentCaptor.forClass(String.class); + verify(userVerificationService).createVerificationTokenForUser(eq(testUser), tokenCaptor.capture()); + assertThat(tokenCaptor.getValue()).matches("[A-Za-z0-9_-]{43}"); + + // And the verification email is sent to the reloaded user's address + verify(mailService).sendTemplateMessage( + eq(testUser.getEmail()), + eq("Registration Confirmation"), + any(Map.class), + eq("mail/registration-token.html") + ); + } + + @Test + @DisplayName("sendRegistrationVerificationEmail - by id is a no-op when the user no longer exists") + void sendRegistrationVerificationEmail_byId_noOpWhenUserMissing() { + // Given - the user was deleted before the async dispatch ran + when(userRepository.findById(1L)).thenReturn(java.util.Optional.empty()); + + // When + userEmailService.sendRegistrationVerificationEmail(1L, appUrl); + + // Then - nothing is created or sent + verify(userVerificationService, never()).createVerificationTokenForUser(any(), anyString()); + verify(mailService, never()).sendTemplateMessage(anyString(), anyString(), any(Map.class), anyString()); + } + + @Test + @DisplayName("sendRegistrationVerificationEmail - by id is a no-op for a null id") + void sendRegistrationVerificationEmail_byId_noOpForNullId() { + // When + userEmailService.sendRegistrationVerificationEmail((Long) null, appUrl); + + // Then + verify(userRepository, never()).findById(any()); + verify(mailService, never()).sendTemplateMessage(anyString(), anyString(), any(Map.class), anyString()); + } + @Test @DisplayName("sendRegistrationVerificationEmail - uses different token for each call") void sendRegistrationVerificationEmail_usesDifferentTokenForEachCall() { diff --git a/src/test/java/com/digitalsanctuary/spring/user/service/UserServiceTest.java b/src/test/java/com/digitalsanctuary/spring/user/service/UserServiceTest.java index 8f29e569..a8f93770 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/service/UserServiceTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/service/UserServiceTest.java @@ -96,8 +96,6 @@ public class UserServiceTest { @Mock private ApplicationEventPublisher eventPublisher; @Mock - private AuthorityService authorityService; - @Mock private PasswordHistoryRepository passwordHistoryRepository; @Mock private SessionInvalidationService sessionInvalidationService; @@ -436,8 +434,8 @@ void deleteOrDisableUser_publishesUserPreDeleteEvent() { // Then verify(eventPublisher).publishEvent(eventCaptor.capture()); UserPreDeleteEvent publishedEvent = eventCaptor.getValue(); - assertThat(publishedEvent.getUser()).isEqualTo(testUser); assertThat(publishedEvent.getUserId()).isEqualTo(testUser.getId()); + assertThat(publishedEvent.getUserEmail()).isEqualTo(testUser.getEmail()); } } @@ -647,11 +645,12 @@ class AuthWithoutPasswordTests { @DisplayName("authWithoutPassword - authenticates valid user") void authWithoutPassword_authenticatesValidUser() { // Given - DSUserDetails userDetails = new DSUserDetails(testUser); Collection authorities = Arrays.asList(new SimpleGrantedAuthority("ROLE_USER")); + // Authorities now come from the principal loaded by loadUserByUsername (which resolves roles/privileges + // via the entity-graph finder), not from re-deriving them from the possibly-detached incoming user. + DSUserDetails userDetails = new DSUserDetails(testUser, authorities); when(dsUserDetailsService.loadUserByUsername(testUser.getEmail())).thenReturn(userDetails); - when(authorityService.getAuthoritiesFromUser(testUser)).thenReturn((Collection) authorities); HttpServletRequest mockRequest = mock(HttpServletRequest.class); HttpSession mockSession = mock(HttpSession.class); @@ -692,11 +691,10 @@ void authWithoutPassword_authenticatesValidUser() { @DisplayName("authWithoutPassword - publishes InteractiveAuthenticationSuccessEvent") void shouldPublishInteractiveAuthenticationSuccessEventWhenAuthSucceeds() { // Given - DSUserDetails userDetails = new DSUserDetails(testUser); Collection authorities = Arrays.asList(new SimpleGrantedAuthority("ROLE_USER")); + DSUserDetails userDetails = new DSUserDetails(testUser, authorities); when(dsUserDetailsService.loadUserByUsername(testUser.getEmail())).thenReturn(userDetails); - when(authorityService.getAuthoritiesFromUser(testUser)).thenReturn((Collection) authorities); HttpServletRequest mockRequest = mock(HttpServletRequest.class); HttpSession mockSession = mock(HttpSession.class); @@ -743,7 +741,6 @@ void authWithoutPassword_handlesNullUser() { // Then verify(dsUserDetailsService, never()).loadUserByUsername(any()); - verify(authorityService, never()).getAuthoritiesFromUser(any()); verify(eventPublisher, never()).publishEvent(any()); } @@ -758,7 +755,6 @@ void authWithoutPassword_handlesUserWithNullEmail() { // Then verify(dsUserDetailsService, never()).loadUserByUsername(any()); - verify(authorityService, never()).getAuthoritiesFromUser(any()); verify(eventPublisher, never()).publishEvent(any()); } @@ -773,7 +769,6 @@ void authWithoutPassword_handlesUserNotFound() { userService.authWithoutPassword(testUser); // Then - verify(authorityService, never()).getAuthoritiesFromUser(any()); verify(eventPublisher, never()).publishEvent(any()); } @@ -781,11 +776,10 @@ void authWithoutPassword_handlesUserNotFound() { @DisplayName("authWithoutPassword - handles no request context") void authWithoutPassword_handlesNoRequestContext() { // Given - DSUserDetails userDetails = new DSUserDetails(testUser); Collection authorities = Arrays.asList(new SimpleGrantedAuthority("ROLE_USER")); + DSUserDetails userDetails = new DSUserDetails(testUser, authorities); when(dsUserDetailsService.loadUserByUsername(testUser.getEmail())).thenReturn(userDetails); - when(authorityService.getAuthoritiesFromUser(testUser)).thenReturn((Collection) authorities); try (MockedStatic mockedHolder = mockStatic(RequestContextHolder.class); MockedStatic mockedSecurityHolder = mockStatic( @@ -810,11 +804,10 @@ void shouldRotateSessionIdWhenAuthSucceeds() { // Given a real request/session bound to the RequestContextHolder so that the servlet // changeSessionId() contract is exercised faithfully (MockHttpServletRequest rotates the // underlying MockHttpSession id while preserving attributes). - DSUserDetails userDetails = new DSUserDetails(testUser); Collection authorities = Arrays.asList(new SimpleGrantedAuthority("ROLE_USER")); + DSUserDetails userDetails = new DSUserDetails(testUser, authorities); when(dsUserDetailsService.loadUserByUsername(testUser.getEmail())).thenReturn(userDetails); - when(authorityService.getAuthoritiesFromUser(testUser)).thenReturn((Collection) authorities); MockHttpServletRequest mockRequest = new MockHttpServletRequest(); // Ensure a pre-auth session exists with a fixed id and a pre-existing attribute diff --git a/src/test/java/com/digitalsanctuary/spring/user/service/UserUpdateIntegrationTest.java b/src/test/java/com/digitalsanctuary/spring/user/service/UserUpdateIntegrationTest.java index 8f2f8b23..b4ec607c 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/service/UserUpdateIntegrationTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/service/UserUpdateIntegrationTest.java @@ -51,16 +51,21 @@ class UserUpdateIntegrationTest { @BeforeEach void setUp() { - // Clean up + // Clean up users; do not delete-and-recreate roles. ROLE_USER and ROLE_ADMIN already exist (created at startup + // by RolePrivilegeSetupService) and now carry a UNIQUE constraint on name, so reuse the existing rows via + // findByName-or-create rather than re-inserting duplicates. userRepository.deleteAll(); - roleRepository.deleteAll(); - // Create roles - userRole = new Role("ROLE_USER", "Basic user role"); - userRole = roleRepository.save(userRole); + userRole = getOrCreateRole("ROLE_USER", "Basic user role"); + adminRole = getOrCreateRole("ROLE_ADMIN", "Administrator role"); + } - adminRole = new Role("ROLE_ADMIN", "Administrator role"); - adminRole = roleRepository.save(adminRole); + private Role getOrCreateRole(final String name, final String description) { + final Role existing = roleRepository.findByName(name); + if (existing != null) { + return existing; + } + return roleRepository.save(new Role(name, description)); } @Test diff --git a/src/test/java/com/digitalsanctuary/spring/user/util/AppUrlResolverTest.java b/src/test/java/com/digitalsanctuary/spring/user/util/AppUrlResolverTest.java new file mode 100644 index 00000000..d63bc447 --- /dev/null +++ b/src/test/java/com/digitalsanctuary/spring/user/util/AppUrlResolverTest.java @@ -0,0 +1,157 @@ +package com.digitalsanctuary.spring.user.util; + +import static org.assertj.core.api.Assertions.assertThat; +import java.util.List; +import org.junit.jupiter.api.Test; +import org.springframework.mock.web.MockHttpServletRequest; + +/** + * Unit tests for {@link AppUrlResolver}, verifying defense against Host-header / X-Forwarded-Host + * poisoning (CWE-640). + */ +class AppUrlResolverTest { + + @Test + void stripsTrailingSlashFromConfiguredAppUrl() { + // A consumer misconfiguring a trailing slash must not produce double slashes in email links + // (e.g. appUrl + "/user/registrationConfirm?token=..." → "https://app.example.com//user/...") + assertThat(new AppUrlResolver("https://app.example.com/", List.of()) + .resolveAppUrl(new MockHttpServletRequest())).isEqualTo("https://app.example.com"); + assertThat(new AppUrlResolver("https://app.example.com///", List.of()) + .resolveAppUrl(new MockHttpServletRequest())).isEqualTo("https://app.example.com"); + } + + @Test + void usesConfiguredAppUrlAndIgnoresForwardedHostWhenConfigured() { + AppUrlResolver resolver = new AppUrlResolver("https://app.example.com", List.of()); + MockHttpServletRequest req = new MockHttpServletRequest(); + req.addHeader("X-Forwarded-Host", "evil.com"); + assertThat(resolver.resolveAppUrl(req)).isEqualTo("https://app.example.com"); + } + + @Test + void rejectsForwardedHostNotInAllowlistWhenNoConfiguredUrl() { + AppUrlResolver resolver = new AppUrlResolver(null, List.of("trusted.example.com")); + MockHttpServletRequest req = new MockHttpServletRequest(); + req.setScheme("https"); + req.setServerName("trusted.example.com"); + req.setServerPort(443); + req.addHeader("X-Forwarded-Host", "evil.com"); + assertThat(resolver.resolveAppUrl(req)).contains("trusted.example.com").doesNotContain("evil.com"); + } + + @Test + void honorsForwardedHostOnlyWhenAllowListed() { + AppUrlResolver resolver = new AppUrlResolver(null, List.of("trusted.example.com")); + MockHttpServletRequest req = new MockHttpServletRequest(); + req.setScheme("http"); + req.setServerName("internal"); + req.setServerPort(8080); + req.addHeader("X-Forwarded-Proto", "https"); + req.addHeader("X-Forwarded-Host", "trusted.example.com"); + // A correctly-behaving HTTPS reverse proxy also forwards the public port; with the default + // HTTPS port (443) the resolver omits the redundant ":port" suffix, yielding a clean URL. + req.addHeader("X-Forwarded-Port", "443"); + assertThat(resolver.resolveAppUrl(req)).isEqualTo("https://trusted.example.com"); + } + + @Test + void honorsAllowListedIpv6ForwardedHostWhenForwarded() { + AppUrlResolver resolver = new AppUrlResolver(null, List.of("[::1]")); + MockHttpServletRequest req = new MockHttpServletRequest(); + req.setScheme("http"); + req.setServerName("internal"); + req.setServerPort(8080); + req.addHeader("X-Forwarded-Proto", "https"); + req.addHeader("X-Forwarded-Host", "[::1]:8443"); + // The IPv6 literal (with embedded colons) must be matched against the allow-list correctly, + // so the trusted forwarded host is honored rather than falling back to the container host. + String resolved = resolver.resolveAppUrl(req); + assertThat(resolved).contains("[::1]").doesNotContain("internal"); + } + + @Test + void derivesPortFromForwardedSchemeWhenForwardedPortAbsent() { + AppUrlResolver resolver = new AppUrlResolver(null, List.of("trusted.example.com")); + MockHttpServletRequest req = new MockHttpServletRequest(); + req.setScheme("http"); + req.setServerName("internal"); + req.setServerPort(8080); + req.addHeader("X-Forwarded-Proto", "https"); + req.addHeader("X-Forwarded-Host", "trusted.example.com"); + // No X-Forwarded-Port: the port must be derived from the forwarded https scheme (443) and then + // omitted as the default, NOT taken from the container's internal port (8080). + assertThat(resolver.resolveAppUrl(req)).isEqualTo("https://trusted.example.com"); + } + + @Test + void omitsDefaultPortAndContextPathForNonForwardedRequest() { + AppUrlResolver resolver = new AppUrlResolver(null, List.of()); + MockHttpServletRequest req = new MockHttpServletRequest(); + req.setScheme("https"); + req.setServerName("api.example.com"); + req.setServerPort(443); + req.setContextPath(""); + assertThat(resolver.resolveAppUrl(req)).isEqualTo("https://api.example.com"); + } + + @Test + void includesNonDefaultPortAndContextPathForNonForwardedRequest() { + AppUrlResolver resolver = new AppUrlResolver(null, List.of()); + MockHttpServletRequest req = new MockHttpServletRequest(); + req.setScheme("http"); + req.setServerName("localhost"); + req.setServerPort(8080); + req.setContextPath("/app"); + assertThat(resolver.resolveAppUrl(req)).isEqualTo("http://localhost:8080/app"); + } + + @Test + void matchesFirstValueOfMultiValuedForwardedHostAgainstAllowList() { + AppUrlResolver resolver = new AppUrlResolver(null, List.of("trusted.example.com")); + MockHttpServletRequest req = new MockHttpServletRequest(); + req.setScheme("http"); + req.setServerName("internal"); + req.setServerPort(8080); + req.addHeader("X-Forwarded-Proto", "https"); + // Multi-proxy chain (RFC 7230): X-Forwarded-Host is a comma-separated list whose FIRST value is the + // client-facing host. The allow-list must match that first value, not the whole compound string. + req.addHeader("X-Forwarded-Host", "trusted.example.com, internal.proxy"); + req.addHeader("X-Forwarded-Port", "443"); + assertThat(resolver.resolveAppUrl(req)).isEqualTo("https://trusted.example.com"); + } + + @Test + void ignoresInvalidForwardedProtoAndFallsBackToContainerScheme() { + AppUrlResolver resolver = new AppUrlResolver(null, List.of("trusted.example.com")); + MockHttpServletRequest req = new MockHttpServletRequest(); + req.setScheme("https"); + req.setServerName("internal"); + req.setServerPort(443); + // A trusted but misconfigured/compromised proxy must never inject a non-http(s) scheme into a + // security email link: an invalid X-Forwarded-Proto is ignored and the request scheme is used. + req.addHeader("X-Forwarded-Proto", "javascript"); + req.addHeader("X-Forwarded-Host", "trusted.example.com"); + req.addHeader("X-Forwarded-Port", "443"); + String resolved = resolver.resolveAppUrl(req); + assertThat(resolved).isEqualTo("https://trusted.example.com"); + assertThat(resolved).doesNotContain("javascript"); + } + + @Test + void ignoresUntrustedForwardedHostAndUsesContainerServerName() { + AppUrlResolver resolver = new AppUrlResolver(null, List.of("trusted.example.com")); + MockHttpServletRequest req = new MockHttpServletRequest(); + req.setScheme("http"); + req.setServerName("internal"); + req.setServerPort(8080); + req.addHeader("X-Forwarded-Proto", "https"); + req.addHeader("X-Forwarded-Host", "evil.com"); + req.addHeader("X-Forwarded-Port", "443"); + // Untrusted forwarded host -> forwarded headers are NOT honored; the container's own + // scheme/host/port are used instead. + String resolved = resolver.resolveAppUrl(req); + assertThat(resolved).isEqualTo("http://internal:8080"); + assertThat(resolved).doesNotContain("evil.com"); + } +} diff --git a/src/test/resources/test-messages/consumer-bundle.properties b/src/test/resources/test-messages/consumer-bundle.properties new file mode 100644 index 00000000..80602121 --- /dev/null +++ b/src/test/resources/test-messages/consumer-bundle.properties @@ -0,0 +1 @@ +consumer.test.key=Consumer bundle value