diff --git a/.claude/skills/multi-tool-code-review/SKILL.md b/.claude/skills/multi-tool-code-review/SKILL.md index 08970392..63322b8f 100644 --- a/.claude/skills/multi-tool-code-review/SKILL.md +++ b/.claude/skills/multi-tool-code-review/SKILL.md @@ -88,7 +88,7 @@ Follow `04-fix-protocol.md`. If the harness supports plan mode, enter it first a ## Conventions (apply throughout) - Ask if you are unsure of anything rather than assuming. Follow the host repo's `AGENTS.md` / `CLAUDE.md` closely. -- **Always hand back full absolute paths, on their own line.** Every artifact you write (the triage doc in each format, and any raw runner output you point at) gets its real path via `realpath`, never a bare filename or a repo-relative fragment buried in a sentence. The user clicks these to open them, and a path that is not absolute is not clickable. Reviews often run from a git worktree while the user sits in the main checkout, so resolve the path instead of assuming a shared working directory, and say which checkout it is in. See "Delivering the doc" in `03-triage-doc-format.md`. +- **Always hand back artifacts as `[filename](file:///absolute/path)` Markdown links, one per line.** Every artifact you write (the triage doc in each format, and any raw runner output you point at) gets its real path via `realpath`, wrapped in a Markdown link with a `file://` target. That is the only form the user can click; a bare absolute path, a bare `file://` URI, and any `vscode://` variant were all tested and none of them work. Reviews often run from a git worktree while the user sits in the main checkout, so resolve the path instead of assuming a shared working directory, and say which checkout it is in. See "Delivering the doc" in `03-triage-doc-format.md`. - **The HTML variant follows the system colour scheme, dark by default.** Base palette dark in `:root`, light via `@media (prefers-color-scheme: light)`, print forced light, every colour a CSS variable. Full rules in `03-triage-doc-format.md`. - No em dashes anywhere (chat, docs, commits, comments). - Do not hardcode any model; ask the user each run and recommend from a fresh online check. diff --git a/.claude/skills/multi-tool-code-review/references/03-triage-doc-format.md b/.claude/skills/multi-tool-code-review/references/03-triage-doc-format.md index beb03fba..e15798b5 100644 --- a/.claude/skills/multi-tool-code-review/references/03-triage-doc-format.md +++ b/.claude/skills/multi-tool-code-review/references/03-triage-doc-format.md @@ -80,6 +80,17 @@ For the HTML variant, build a single self-contained page with the `frontend-desi Self-contained means genuinely self-contained: inline the CSS, use system font stacks, and reference no CDN, webfont, or image. The page is opened over `file://`, often with no network, and anything external renders as a broken document. +### The HTML layout is built for reading a wide table + +The whole point of the HTML variant is that a long table is easier to scan than Markdown, so the layout must give the table room. These rules are not cosmetic preferences; each one fixes a way the page became unreadable in practice: + +- **Use the full browser width. Do not put the page in a centred fixed-width container.** A `max-width` on the wrapper squeezes nine columns into a column of text and forces horizontal scrolling on a screen that had plenty of room. Prose blocks can keep their own reading measure, but the tables get the whole viewport. +- **The Finding column needs roughly three times the width the browser gives it by default.** Left to itself it collapses to one word per line and the table becomes unreadable vertical confetti. Give it a generous `min-width`. +- **The Where column should be about half its natural width.** It holds a `file:line`, which is the least important cell on the row, and letting it sit `nowrap` lets one long path dictate the whole table's geometry. Let it wrap and break on the path separators. +- **Never render Now, Expected and Test as one paragraph.** They are three distinct things and a reader scans for one of them at a time. Split the cell into a separate block per part, each with its own label, so the eye can land on Test without reading Now first. + +Because the Maintainability tail uses a different, shorter column set, key any positional column rules to the main table only (for example by tagging the nine-column tables with a class), or the tail's cells inherit widths meant for columns it does not have. + ### The HTML must follow the system colour scheme Default to **dark**, and let a light system preference override it. Not the other way round: the user's environment is dark nearly all the time, so dark is the right base and the right fallback when the preference is unknown. @@ -132,7 +143,24 @@ PY ## Delivering the doc -Give the user the **full absolute path**, on its own line, for every artifact you wrote. Terminal and desktop chat interfaces turn an absolute path into a clickable link, and clicking is how the user actually opens these. A bare filename, a repo-relative path, or a path in prose is not clickable and forces them to reconstruct it. +Hand back every artifact as a **Markdown link whose target is a `file://` URI**, built from the absolute path. This is the only form that is clickable here, confirmed by testing five variants against this user's terminal on 2026-08-06: + +```markdown +[REVIEW-2026-08-06-DEV-VS-MAIN-BY-AREA.md](file:///home/silver/Desktop/Disscount/reviews/REVIEW-2026-08-06-DEV-VS-MAIN-BY-AREA.md) +``` + +Use the filename as the link text, and put each artifact on its own line. Do not bury a link mid-sentence. + +What does NOT work, so do not fall back to any of it: + +- A bare absolute path (`/home/silver/...`). Claude Code does not wrap paths in OSC 8 escapes, so a bare path is clickable only in terminals that auto-detect paths themselves, and this one does not. +- A bare `file://` URI as plain text. +- A `vscode://file/...` URI in either form. +- A bare filename, a repo-relative path, or a path inside prose. + +Target the **system default handler** via `file://`, not an editor scheme. The two artifacts want different applications (an editor for the Markdown, a browser for the HTML) and `file://` lets the desktop pick correctly for each. + +Note this applies to the artifacts you deliver. Inline `file.ts:42` references to source, which the harness renders as its own clickable reference, keep their existing form. Get the directory right, not just the name. Reviews are frequently run from a **git worktree**, and `reviews/` is typically gitignored, so the file exists only under the worktree it was written in and no git operation will ever move it. Resolve the real path rather than assuming the user shares your working directory: diff --git a/AGENTS.md b/AGENTS.md index 6eb7fdf9..cb0b1d22 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -27,6 +27,8 @@ Ask first: - Widening scope beyond what I asked for. - Any instruction of mine that has two plausible readings. Ask before you edit, do not pick one and start. +**Ask means ask.** Every item above is a question to put to me, not a reason to pick the lesser option and move on. Do not work around a missing endpoint, skip the dependency and hand-roll it, or narrow the task to avoid the question. I say yes far more often than no, so quietly choosing the workaround costs me the better answer and I never learn the choice was there. If you are mid-task and cannot stop, do the parts that do not depend on the answer, then ask before you finish. Never let "I did not want to widen scope" be the reason something shipped worse. + Hand back interactive installers and `init` wizards, except `pnpm dlx shadcn@latest add `, which you may run. Safe without asking, run from `frontend/`: @@ -96,7 +98,7 @@ Conventions: - `I`-prefixed Props interfaces, in the same file as the component. One component per file, default export. - `function name() {}`, not `const name = () => {}`, except for small inline callbacks. - `import { useState } from "react"`, never `React.useState`. -- `components/ui/` is shadcn output, so do not hand-edit it. Our components live in `components/custom/`, grouped by concern. +- `components/ui/` is shadcn output. Editing it is allowed where the primitive is the natural home for the change, such as a prop the component itself should own or a sizing rule our `--spacing` override breaks. Our components live in `components/custom/`, grouped by concern. - Types: API and domain go in `lib/api/schemas/*` as zod `*Dto` / `*Response`; external price API types in `lib/cijene-api/schemas.ts`; shared UI types in `@/typings`; feature-only types stay colocated in `*-types.ts`. - React Query hooks live next to their service in `lib/api//`. Feature composition hooks go in the feature's `hooks/`. - Before generating or redesigning UI, read `frontend/.github/skills/frontend-design/SKILL.md` and follow it. diff --git a/backend/src/main/java/disscount/config/SecurityConfig.java b/backend/src/main/java/disscount/config/SecurityConfig.java index 8b42f283..855deaee 100644 --- a/backend/src/main/java/disscount/config/SecurityConfig.java +++ b/backend/src/main/java/disscount/config/SecurityConfig.java @@ -18,6 +18,8 @@ import org.springframework.security.oauth2.jwt.NimbusJwtDecoder; import org.springframework.security.oauth2.server.resource.web.BearerTokenAuthenticationFilter; import org.springframework.security.web.SecurityFilterChain; +import org.springframework.security.web.util.matcher.OrRequestMatcher; +import org.springframework.security.web.util.matcher.RequestMatcher; @Configuration @EnableWebSecurity @@ -46,7 +48,7 @@ public JwtDecoder jwtDecoder( * {@code @Component} filter meant only for a security chain runs twice: once where it was * placed and once for every request that never reaches that chain. Both of these extend * {@code OncePerRequestFilter}, whose already-filtered attribute makes the second run a - * no-op only when the first one happened, so on any path outside {@code /api/shared/**} + * no-op only when the first one happened, so on any path outside the optional-auth chain * the optional bearer filter would decode the token again after the real chain had * finished with it. These beans turn the servlet registration off and leave the security * chains as the only place either filter runs. @@ -70,37 +72,50 @@ public FilterRegistrationBean optionalBearer } /** - * Shared lists get their own chain because they are the one place where a bearer token is - * optional. Authorization happens on the share token plus ShoppingListAccessService, and - * the caller may legitimately be anonymous, so a token that fails to decode must degrade - * to anonymous rather than 401. permitAll on the main chain cannot express that: its - * bearer filter rejects a stale token before authorization is ever consulted. + * The by-id shopping list routes, which are the one place a bearer token is optional. + * A list is shared by its own id, so the caller may legitimately be anonymous and a token + * that fails to decode must degrade to anonymous rather than 401. permitAll on the main + * chain cannot express that: its bearer filter rejects a stale token before authorization + * is ever consulted. + * + *

Nothing here authorizes anything. The rule is {@code permitAll}, so the + * access checks in {@link disscount.shoppingList.service.ShoppingListService} are an + * authentication boundary: relaxing one removes authentication, not a convenience. */ @Bean @Order(1) - public SecurityFilterChain sharedShoppingListChain( + public SecurityFilterChain optionalAuthShoppingListChain( HttpSecurity http, OptionalBearerAuthenticationFilter optionalBearerAuthenticationFilter ) throws Exception { http - .securityMatcher("/api/shared/**") + .securityMatcher(shoppingListByIdMatcher()) .csrf(csrf -> csrf.disable()) .sessionManagement(session -> session.sessionCreationPolicy(SessionCreationPolicy.STATELESS)) .authorizeHttpRequests(authz -> authz.anyRequest().permitAll()) - // Both are anchored on the slot where a bearer token is normally decoded, which - // is where these two belong and which leaves them 98 places of headroom before - // the next registered filter. addFilterAfter is order + 1, so this is bearer at - // +1 and provisioning at +2: a strict sequence, which provisioning needs because - // it acts on the authentication the bearer filter produced. Anchoring either one - // on AnonymousAuthenticationFilter instead lands exactly on top of it, since a - // before is order - 1 and the following after adds the 1 straight back, and the - // resulting tie is broken only by the sort happening to be stable. + // Anchored on the bearer slot so provisioning lands strictly after it. Anchoring + // on AnonymousAuthenticationFilter ties with it, since before is -1 and the + // following after adds the 1 straight back. .addFilterAfter(optionalBearerAuthenticationFilter, BearerTokenAuthenticationFilter.class) .addFilterAfter(userProvisioningFilter, OptionalBearerAuthenticationFilter.class); return http.build(); } + /** Unlisted methods fall through to the authenticated chain, so the default is deny. */ + private static RequestMatcher shoppingListByIdMatcher() { + return new OrRequestMatcher( + new UuidScopedRequestMatcher(HttpMethod.GET, "/api/shopping-lists/{id}"), + // Link unfurlers probe with HEAD, which the matcher compares exactly. + new UuidScopedRequestMatcher(HttpMethod.HEAD, "/api/shopping-lists/{id}"), + new UuidScopedRequestMatcher(HttpMethod.PUT, "/api/shopping-lists/{id}"), + new UuidScopedRequestMatcher(HttpMethod.DELETE, "/api/shopping-lists/{id}"), + new UuidScopedRequestMatcher(HttpMethod.POST, "/api/shopping-lists/{id}/items"), + new UuidScopedRequestMatcher(HttpMethod.POST, "/api/shopping-lists/{id}/copy"), + new UuidScopedRequestMatcher(HttpMethod.PUT, "/api/shopping-lists/{id}/items/{itemId}"), + new UuidScopedRequestMatcher(HttpMethod.DELETE, "/api/shopping-lists/{id}/items/{itemId}")); + } + @Bean @Order(2) public SecurityFilterChain filterChain(HttpSecurity http) throws Exception { diff --git a/backend/src/main/java/disscount/config/UuidScopedRequestMatcher.java b/backend/src/main/java/disscount/config/UuidScopedRequestMatcher.java new file mode 100644 index 00000000..505aacf6 --- /dev/null +++ b/backend/src/main/java/disscount/config/UuidScopedRequestMatcher.java @@ -0,0 +1,41 @@ +package disscount.config; + +import jakarta.servlet.http.HttpServletRequest; +import org.springframework.http.HttpMethod; +import org.springframework.security.web.util.matcher.AntPathRequestMatcher; +import org.springframework.security.web.util.matcher.RequestMatcher; + +import java.util.regex.Pattern; + +/** + * Matches one method and path only when the {@code id} segment is UUID-shaped. + * + *

An allowlist on both axes. A wildcard minus the authenticated routes would be a + * denylist inside an allowlist, silently exposing the next literal route somebody adds; + * requiring a UUID excludes {@code /me}, {@code /items} and any future literal for free. + * + *

Canonical dashed form only. Bare 32-hex is excluded because Spring's + * StringToUUIDConverter rejects it, so it would only buy an anonymous caller a logged 500. + */ +final class UuidScopedRequestMatcher implements RequestMatcher { + + private static final Pattern UUID_SHAPE = Pattern.compile( + "^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$"); + + private final AntPathRequestMatcher delegate; + + UuidScopedRequestMatcher(HttpMethod method, String pattern) { + this.delegate = new AntPathRequestMatcher(pattern, method.name()); + } + + @Override + public boolean matches(HttpServletRequest request) { + MatchResult result = delegate.matcher(request); + if (!result.isMatch()) { + return false; + } + + String id = result.getVariables().get("id"); + return id != null && UUID_SHAPE.matcher(id).matches(); + } +} diff --git a/backend/src/main/java/disscount/exceptions/GlobalExceptionHandler.java b/backend/src/main/java/disscount/exceptions/GlobalExceptionHandler.java index 2fffec16..55adb8d4 100644 --- a/backend/src/main/java/disscount/exceptions/GlobalExceptionHandler.java +++ b/backend/src/main/java/disscount/exceptions/GlobalExceptionHandler.java @@ -1,5 +1,8 @@ package disscount.exceptions; +import org.hibernate.exception.ConstraintViolationException; +import org.springframework.dao.DataIntegrityViolationException; +import org.springframework.dao.DuplicateKeyException; import org.springframework.http.HttpStatus; import org.springframework.http.ProblemDetail; import org.springframework.http.converter.HttpMessageNotReadableException; @@ -7,10 +10,12 @@ import org.springframework.web.bind.MethodArgumentNotValidException; import org.springframework.web.bind.annotation.ExceptionHandler; import org.springframework.web.bind.annotation.RestControllerAdvice; +import org.springframework.web.method.annotation.MethodArgumentTypeMismatchException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import java.net.URI; +import java.sql.SQLException; import java.time.Instant; import java.util.HashMap; import java.util.Map; @@ -40,11 +45,68 @@ public ProblemDetail handleForbiddenException(ForbiddenException ex) { return problem(HttpStatus.FORBIDDEN, "forbidden", "Zabranjeno", ex.getMessage()); } + @ExceptionHandler(NotFoundException.class) + public ProblemDetail handleNotFoundException(NotFoundException ex) { + return problem(HttpStatus.NOT_FOUND, "not-found", "Nije pronađeno", ex.getMessage()); + } + @ExceptionHandler(ConflictException.class) public ProblemDetail handleConflictException(ConflictException ex) { return problem(HttpStatus.CONFLICT, "conflict", "Sukob", ex.getMessage()); } + /** + * Hibernate reports a unique-index violation as DataIntegrityViolationException, not + * DuplicateKeyException, so both are caught and narrowed here. Not-null, check and + * foreign-key violations are bugs rather than conflicts and rethrow to the 500. + * Nothing from the driver message is logged: PostgreSQL puts the colliding value in it. + */ + @ExceptionHandler(DataIntegrityViolationException.class) + public ProblemDetail handleDuplicateKey(DataIntegrityViolationException ex) { + // Rethrowing would bypass this advice entirely and surface a container 500, so the + // non-conflict case is answered here instead. + if (!(ex instanceof DuplicateKeyException) && !isUniqueViolation(ex)) { + return handleGenericException(ex); + } + + String constraint = constraintNameOf(ex); + log.warn("Duplicate key violation on constraint: {}", constraint); + + boolean isUsername = constraint.toLowerCase().contains("username"); + + ProblemDetail detail = problem(HttpStatus.CONFLICT, "conflict", "Sukob", + isUsername ? "Korisničko ime je već zauzeto." : "Vrijednost je već zauzeta."); + + if (isUsername) { + detail.setProperty("fieldErrors", Map.of("username", "Korisničko ime je već zauzeto.")); + } + + return detail; + } + + /** SQLState 23505 is unique_violation in both PostgreSQL and the H2 the tests run on. */ + private static boolean isUniqueViolation(Throwable ex) { + for (Throwable cause = ex; cause != null; cause = cause.getCause()) { + if (cause instanceof SQLException sql && "23505".equals(sql.getSQLState())) { + return true; + } + if (cause.getCause() == cause) { + break; + } + } + return false; + } + + /** The constraint name only; the message around it carries the colliding value. */ + private static String constraintNameOf(DataIntegrityViolationException ex) { + Throwable cause = ex.getCause(); + if (cause instanceof ConstraintViolationException violation + && violation.getConstraintName() != null) { + return violation.getConstraintName(); + } + return ex.getClass().getSimpleName(); + } + @ExceptionHandler(MethodArgumentNotValidException.class) public ProblemDetail handleValidationExceptions(MethodArgumentNotValidException ex) { Map fieldErrors = new HashMap<>(); @@ -61,6 +123,13 @@ public ProblemDetail handleValidationExceptions(MethodArgumentNotValidException return problemDetail; } + /** Without this a malformed UUID is a logged 500, free for anyone to trigger. */ + @ExceptionHandler(MethodArgumentTypeMismatchException.class) + public ProblemDetail handleTypeMismatch(MethodArgumentTypeMismatchException ex) { + return problem(HttpStatus.BAD_REQUEST, "bad-request", "Neispravan zahtjev", + "Neispravan format parametra: " + ex.getName()); + } + @ExceptionHandler(HttpMessageNotReadableException.class) public ProblemDetail handleNotReadable(HttpMessageNotReadableException ex) { return problem(HttpStatus.BAD_REQUEST, "malformed-request", "Neispravan zahtjev", diff --git a/backend/src/main/java/disscount/exceptions/NotFoundException.java b/backend/src/main/java/disscount/exceptions/NotFoundException.java new file mode 100644 index 00000000..20f787d6 --- /dev/null +++ b/backend/src/main/java/disscount/exceptions/NotFoundException.java @@ -0,0 +1,15 @@ +package disscount.exceptions; + +/** + * The resource does not exist, or the caller has no business knowing that it does. + * + *

Deliberately used in place of {@link ForbiddenException} where a 403 would confirm that + * something exists: a private shopping list answers the same way as an id that was never + * real, so holding an id tells a stranger nothing. + */ +public class NotFoundException extends RuntimeException { + + public NotFoundException(String message) { + super(message); + } +} diff --git a/backend/src/main/java/disscount/shoppingList/dao/ShoppingListRepository.java b/backend/src/main/java/disscount/shoppingList/dao/ShoppingListRepository.java index c2b0fa37..f07344a7 100644 --- a/backend/src/main/java/disscount/shoppingList/dao/ShoppingListRepository.java +++ b/backend/src/main/java/disscount/shoppingList/dao/ShoppingListRepository.java @@ -31,12 +31,13 @@ public interface ShoppingListRepository extends JpaRepository findActiveByOwner(User owner); + /** + * The only way a by-id request loads a list, deliberately. There is no + * "...AndOwner" variant, because that was the old authorization mechanism and a + * future method reaching for it would silently get owner-only semantics back plus + * the 400-not-404 answer that tells a stranger the id was real. Authorization now + * belongs to ShoppingListService.findVisible, which is this method's one caller. + */ @Query("SELECT sl FROM ShoppingList sl WHERE sl.id = :id AND sl.deletedAt IS NULL") Optional findActiveById(UUID id); - - @Query("SELECT sl FROM ShoppingList sl WHERE sl.id = :id AND sl.owner = :owner AND sl.deletedAt IS NULL") - Optional findActiveByIdAndOwner(UUID id, User owner); - - @Query("SELECT sl FROM ShoppingList sl WHERE sl.shareToken = :shareToken AND sl.deletedAt IS NULL") - Optional findActiveByShareToken(UUID shareToken); } diff --git a/backend/src/main/java/disscount/shoppingList/domain/ListAccess.java b/backend/src/main/java/disscount/shoppingList/domain/ListAccess.java index 91095b9a..270fc2bd 100644 --- a/backend/src/main/java/disscount/shoppingList/domain/ListAccess.java +++ b/backend/src/main/java/disscount/shoppingList/domain/ListAccess.java @@ -32,4 +32,13 @@ public boolean canEditItems() { public boolean canManageShare() { return this == OWNER; } + + /** + * Kept separate from {@link #canManageShare()} even though the two agree today. Deleting + * a list and adding items to it are ownership questions, not sharing ones, so a future + * decision to let some link level manage sharing must not quietly grant either. + */ + public boolean isOwner() { + return this == OWNER; + } } diff --git a/backend/src/main/java/disscount/shoppingList/domain/ShoppingList.java b/backend/src/main/java/disscount/shoppingList/domain/ShoppingList.java index 0fb943bb..62f2076b 100644 --- a/backend/src/main/java/disscount/shoppingList/domain/ShoppingList.java +++ b/backend/src/main/java/disscount/shoppingList/domain/ShoppingList.java @@ -36,15 +36,14 @@ public class ShoppingList { // Nullable because ddl-auto=update cannot add a NOT NULL column to a populated table. // Read it through resolvedLinkAccess(), never directly. + // + // This is the whole of sharing: the list's own id is the shareable URL, and this says + // what holding that URL grants. There is no separate token, so turning sharing off and + // on again hands back the same URL, which is the Google Docs behaviour and is intended. @Enumerated(EnumType.STRING) @Column(name = "link_access", length = 16) private ListAccess linkAccess; - // Deliberately not the list id: a token can be rotated, so turning sharing off and on - // again actually revokes instead of handing the same URL back to everyone who kept it. - @Column(name = "share_token", unique = true) - private UUID shareToken; - @Column(name = "updated_at", nullable = false) private LocalDateTime updatedAt; diff --git a/backend/src/main/java/disscount/shoppingList/dto/ShoppingListCopyRequest.java b/backend/src/main/java/disscount/shoppingList/dto/ShoppingListCopyRequest.java new file mode 100644 index 00000000..618c48a8 --- /dev/null +++ b/backend/src/main/java/disscount/shoppingList/dto/ShoppingListCopyRequest.java @@ -0,0 +1,36 @@ +package disscount.shoppingList.dto; + +import jakarta.validation.constraints.NotBlank; +import lombok.Data; + +/** + * What a copy carries. The client used to answer this by creating a list and then firing + * one add per item, which is not a transaction: a failed item left a half-populated list + * behind that no retry could tidy up. + * + *

Booleans rather than a level enum, because the three are independent: a copy can take + * the products without the shopping progress, or the sharing without the products. + */ +@Data +public class ShoppingListCopyRequest { + + @NotBlank(message = "Title is required") + private String title; + + /** Without this the copy is just the title, which is a legitimate thing to want. */ + private boolean includeItems; + + /** + * What was ticked, the shop it was ticked at, and the prices captured at that moment. + * One flag for all four: a tick without its price reads as a bargain nobody recorded. + * Ignored when {@code includeItems} is false, since there is nothing to carry it on. + */ + private boolean includeProgress; + + /** + * Carries the source list's link access onto the copy. Owner-only, enforced server + * side: without that check a recipient could copy a list they were merely shown and + * hand the owner's people a link at a level the owner never chose to give. + */ + private boolean includeSharing; +} diff --git a/backend/src/main/java/disscount/shoppingList/dto/ShoppingListDto.java b/backend/src/main/java/disscount/shoppingList/dto/ShoppingListDto.java index 61da5940..dbcef7ce 100644 --- a/backend/src/main/java/disscount/shoppingList/dto/ShoppingListDto.java +++ b/backend/src/main/java/disscount/shoppingList/dto/ShoppingListDto.java @@ -18,10 +18,9 @@ public class ShoppingListDto { private UUID ownerId; private String title; - // Both owner-only: a link visitor handed the token could reshare the list at a level - // its owner never granted. + // Owner-only: a link visitor who could read this would learn the list is shared more + // widely than their own access shows, and the share control keys off it. private ListAccess linkAccess; - private UUID shareToken; /** The caller's resolved access, echoed back so the frontend never re-derives the rule. */ private ListAccess myAccess; diff --git a/backend/src/main/java/disscount/shoppingList/dto/ShoppingListRequest.java b/backend/src/main/java/disscount/shoppingList/dto/ShoppingListRequest.java index eff3bbbf..92d73095 100644 --- a/backend/src/main/java/disscount/shoppingList/dto/ShoppingListRequest.java +++ b/backend/src/main/java/disscount/shoppingList/dto/ShoppingListRequest.java @@ -11,8 +11,7 @@ public class ShoppingListRequest { @NotBlank(message = "Title is required") private String title; - // Owner-only, and ignored on create: sharing is turned on from an existing list, - // because there is no id to bind a token to until the list has been saved. - // LinkAccess rather than ListAccess, so OWNER cannot be sent at all. + // Owner-only. Honoured on create, so a copy can be born shared, and the caller of a + // create is by definition the new list's owner. private LinkAccess linkAccess; } diff --git a/backend/src/main/java/disscount/shoppingList/rest/SharedShoppingListController.java b/backend/src/main/java/disscount/shoppingList/rest/SharedShoppingListController.java deleted file mode 100644 index b5d683db..00000000 --- a/backend/src/main/java/disscount/shoppingList/rest/SharedShoppingListController.java +++ /dev/null @@ -1,80 +0,0 @@ -package disscount.shoppingList.rest; - -import io.swagger.v3.oas.annotations.Operation; -import io.swagger.v3.oas.annotations.tags.Tag; -import jakarta.validation.Valid; -import lombok.RequiredArgsConstructor; -import org.springframework.http.ResponseEntity; -import org.springframework.web.bind.annotation.*; - -import disscount.shoppingList.dto.ShoppingListDto; -import disscount.shoppingList.dto.ShoppingListRequest; -import disscount.shoppingList.service.SharedShoppingListService; -import disscount.shoppingListItem.dto.ShoppingListItemDto; -import disscount.shoppingListItem.dto.ShoppingListItemRequest; -import disscount.util.SecurityUtils; - -import java.util.UUID; - -/** - * Public entry point for shared lists. Permit-all at the filter chain; the real check is the - * token plus {@link disscount.shoppingList.service.ShoppingListAccessService}. - * - *

Callers may be anonymous, so every method reads the user through - * {@code getCurrentUserIdOptional()} and never {@code getCurrentUserId()}, which throws. - */ -@RestController -@RequestMapping("/api/shared") -@RequiredArgsConstructor -@Tag(name = "Shared Shopping Lists", description = "Shopping lists reachable by share token") -public class SharedShoppingListController { - - private final SharedShoppingListService sharedShoppingListService; - - @Operation(summary = "Get a shared shopping list by its share token") - @GetMapping("/{token}") - public ResponseEntity getSharedShoppingList(@PathVariable String token) { - UUID userId = currentUserId(); - return sharedShoppingListService.getByToken(token, userId) - .map(ResponseEntity::ok) - .orElse(ResponseEntity.notFound().build()); - } - - @Operation(summary = "Rename a shared shopping list") - @PutMapping("/{token}") - public ResponseEntity updateSharedShoppingList( - @PathVariable String token, - @Valid @RequestBody ShoppingListRequest request) { - UUID userId = currentUserId(); - return sharedShoppingListService.updateTitle(token, userId, request) - .map(ResponseEntity::ok) - .orElse(ResponseEntity.notFound().build()); - } - - @Operation(summary = "Update an item on a shared shopping list") - @PutMapping("/{token}/items/{itemId}") - public ResponseEntity updateSharedItem( - @PathVariable String token, - @PathVariable UUID itemId, - @Valid @RequestBody ShoppingListItemRequest request) { - UUID userId = currentUserId(); - return sharedShoppingListService.updateItem(token, itemId, userId, request) - .map(ResponseEntity::ok) - .orElse(ResponseEntity.notFound().build()); - } - - @Operation(summary = "Delete an item from a shared shopping list") - @DeleteMapping("/{token}/items/{itemId}") - public ResponseEntity deleteSharedItem( - @PathVariable String token, - @PathVariable UUID itemId) { - UUID userId = currentUserId(); - return sharedShoppingListService.deleteItem(token, itemId, userId) - ? ResponseEntity.noContent().build() - : ResponseEntity.notFound().build(); - } - - private UUID currentUserId() { - return SecurityUtils.getCurrentUserIdOptional().orElse(null); - } -} diff --git a/backend/src/main/java/disscount/shoppingList/rest/ShoppingListController.java b/backend/src/main/java/disscount/shoppingList/rest/ShoppingListController.java index 07921ab2..83d1ff61 100644 --- a/backend/src/main/java/disscount/shoppingList/rest/ShoppingListController.java +++ b/backend/src/main/java/disscount/shoppingList/rest/ShoppingListController.java @@ -7,8 +7,10 @@ import org.springframework.http.ResponseEntity; import org.springframework.web.bind.annotation.*; +import disscount.shoppingList.dto.ShoppingListCopyRequest; import disscount.shoppingList.dto.ShoppingListDto; import disscount.shoppingList.dto.ShoppingListRequest; +import disscount.shoppingList.service.ShoppingListCopyService; import disscount.shoppingList.service.ShoppingListService; import disscount.shoppingListItem.dto.ShoppingListItemDto; import disscount.util.SecurityUtils; @@ -24,6 +26,7 @@ public class ShoppingListController { private final ShoppingListService shoppingListService; + private final ShoppingListCopyService shoppingListCopyService; @Operation(summary = "Create a new shopping list") @PostMapping @@ -41,13 +44,23 @@ public ResponseEntity> getCurrentUserShoppingLists() { return ResponseEntity.ok(lists); } - @Operation(summary = "Get shopping list by ID") + // The three by-id endpoints run on the chain where a bearer token is optional, so the + // caller may be anonymous and currentUserId() may be null. Authorization is entirely + // ShoppingListService's job; nothing here may assume an owner. + + @Operation(summary = "Get shopping list by ID, for its owner or anyone holding a share link") @GetMapping("/{id}") public ResponseEntity getShoppingListById(@PathVariable UUID id) { - UUID ownerId = SecurityUtils.getCurrentUserId(); - return shoppingListService.getShoppingListById(id, ownerId) - .map(ResponseEntity::ok) - .orElse(ResponseEntity.notFound().build()); + return ResponseEntity.ok(shoppingListService.getShoppingListById(id, currentUserId())); + } + + @Operation(summary = "Copy a shopping list into a new list the caller owns") + @PostMapping("/{id}/copy") + public ResponseEntity copyShoppingList( + @PathVariable UUID id, + @Valid @RequestBody ShoppingListCopyRequest request) { + ShoppingListDto copy = shoppingListCopyService.copy(id, currentUserId(), request); + return ResponseEntity.ok(copy); } @Operation(summary = "Update shopping list") @@ -55,16 +68,14 @@ public ResponseEntity getShoppingListById(@PathVariable UUID id public ResponseEntity updateShoppingList( @PathVariable UUID id, @Valid @RequestBody ShoppingListRequest request) { - UUID ownerId = SecurityUtils.getCurrentUserId(); - ShoppingListDto updated = shoppingListService.updateShoppingList(id, ownerId, request); + ShoppingListDto updated = shoppingListService.updateShoppingList(id, currentUserId(), request); return ResponseEntity.ok(updated); } @Operation(summary = "Delete shopping list") @DeleteMapping("/{id}") public ResponseEntity deleteShoppingList(@PathVariable UUID id) { - UUID ownerId = SecurityUtils.getCurrentUserId(); - shoppingListService.deleteShoppingList(id, ownerId); + shoppingListService.deleteShoppingList(id, currentUserId()); return ResponseEntity.noContent().build(); } @@ -79,4 +90,9 @@ public ResponseEntity> getAllUserShoppingListItems() { .collect(Collectors.toList()); return ResponseEntity.ok(items); } + + /** Null for an anonymous caller on the by-id routes, which is a legitimate state there. */ + private UUID currentUserId() { + return SecurityUtils.getCurrentUserIdOptional().orElse(null); + } } diff --git a/backend/src/main/java/disscount/shoppingList/service/SharedShoppingListService.java b/backend/src/main/java/disscount/shoppingList/service/SharedShoppingListService.java deleted file mode 100644 index e55001b9..00000000 --- a/backend/src/main/java/disscount/shoppingList/service/SharedShoppingListService.java +++ /dev/null @@ -1,148 +0,0 @@ -package disscount.shoppingList.service; - -import lombok.RequiredArgsConstructor; -import org.springframework.stereotype.Service; -import org.springframework.transaction.annotation.Transactional; - -import disscount.exceptions.ForbiddenException; -import disscount.exceptions.UnauthorizedException; -import disscount.shoppingList.dao.ShoppingListRepository; -import disscount.shoppingList.domain.ListAccess; -import disscount.shoppingList.domain.ShoppingList; -import disscount.shoppingList.dto.ShoppingListDto; -import disscount.shoppingList.dto.ShoppingListRequest; -import disscount.shoppingListItem.dao.ShoppingListItemRepository; -import disscount.shoppingListItem.domain.ShoppingListItem; -import disscount.shoppingListItem.dto.ShoppingListItemDto; -import disscount.shoppingListItem.dto.ShoppingListItemRequest; -import disscount.user.dao.UserRepository; -import disscount.user.domain.User; -import disscount.util.Timestamps; - -import java.util.Optional; -import java.util.UUID; -import java.util.function.Predicate; - -/** - * Everything reachable through a share token. The token is the capability, so it travels on - * every call here and knowing a list's id is never enough on its own. - * - *

An unresolvable token yields an empty Optional, which the controller turns into a 404 - * rather than a 403, so the endpoint never confirms that a token once existed. - */ -@Service -@RequiredArgsConstructor -@Transactional -public class SharedShoppingListService { - - private final ShoppingListRepository shoppingListRepository; - private final ShoppingListItemRepository shoppingListItemRepository; - private final UserRepository userRepository; - private final ShoppingListAccessService accessService; - private final ShoppingListMapper shoppingListMapper; - - @Transactional(readOnly = true) - public Optional getByToken(String token, UUID userId) { - return findShared(token) - .map(list -> shoppingListMapper.toDto(list, accessService.resolve(list, userId))); - } - - public Optional updateTitle(String token, UUID userId, ShoppingListRequest request) { - return findShared(token).map(list -> { - ListAccess access = requireAccess(list, userId, ListAccess::canEditItems); - - list.setTitle(request.getTitle()); - return shoppingListMapper.toDto(shoppingListRepository.save(list), access); - }); - } - - public Optional updateItem( - String token, UUID itemId, UUID userId, ShoppingListItemRequest request) { - return findShared(token).flatMap(list -> { - ListAccess access = requireAccess(list, userId, ListAccess::canCheck); - User actor = requireUser(userId); - - return shoppingListItemRepository.findActiveByIdAndShoppingList(itemId, list).map(item -> { - applyItemUpdate(item, request, access); - item.setUpdatedAt(Timestamps.nowUtc()); - item.setUpdatedByUser(actor); - - ShoppingListItem saved = shoppingListItemRepository.save(item); - touchList(list); - return shoppingListMapper.toItemDto(saved, access); - }); - }); - } - - public boolean deleteItem(String token, UUID itemId, UUID userId) { - return findShared(token).map(list -> { - requireAccess(list, userId, ListAccess::canEditItems); - - return shoppingListItemRepository.findActiveByIdAndShoppingList(itemId, list).map(item -> { - item.setDeletedAt(Timestamps.nowUtc()); - shoppingListItemRepository.save(item); - touchList(list); - return true; - }).orElse(false); - }).orElse(false); - } - - /** - * A list is reachable by token only while it is actually shared. The token is nulled - * whenever link access goes back to NONE, so this is belt and braces. - */ - private Optional findShared(String token) { - UUID parsed; - try { - parsed = UUID.fromString(token); - } catch (IllegalArgumentException ex) { - // A malformed token is indistinguishable from an unknown one, by design. - return Optional.empty(); - } - - return shoppingListRepository.findActiveByShareToken(parsed) - .filter(list -> list.resolvedLinkAccess() != ListAccess.NONE); - } - - private ListAccess requireAccess(ShoppingList list, UUID userId, Predicate allowed) { - ListAccess access = accessService.resolve(list, userId); - if (!allowed.test(access)) { - throw new ForbiddenException("Insufficient access to this shopping list"); - } - return access; - } - - private User requireUser(UUID userId) { - return userRepository.findById(userId) - .orElseThrow(() -> new UnauthorizedException("User not found")); - } - - /** - * SHOP is the in-the-shop level: what got ticked, which store it is coming from, and the - * prices captured at that moment. Everything structural is left as the server has it, so a - * SHOP-level caller cannot rename or resize an item by sending a fuller payload. - */ - private void applyItemUpdate(ShoppingListItem item, ShoppingListItemRequest request, ListAccess access) { - item.setIsChecked(request.getIsChecked() != null ? request.getIsChecked() : false); - item.setChainCode(request.getChainCode()); - item.setAvgPrice(request.getAvgPrice()); - item.setStorePrice(request.getStorePrice()); - - if (!access.canEditItems()) { - return; - } - - item.setEan(request.getEan()); - item.setBrand(request.getBrand()); - item.setName(request.getName()); - item.setQuantity(request.getQuantity()); - item.setUnit(request.getUnit()); - item.setAmount(request.getAmount() != null ? request.getAmount() : 1); - } - - /** Item activity reorders the owner's list index, which sorts by updatedAt. */ - private void touchList(ShoppingList list) { - list.setUpdatedAt(Timestamps.nowUtc()); - shoppingListRepository.save(list); - } -} diff --git a/backend/src/main/java/disscount/shoppingList/service/ShoppingListCopyService.java b/backend/src/main/java/disscount/shoppingList/service/ShoppingListCopyService.java new file mode 100644 index 00000000..6b63d382 --- /dev/null +++ b/backend/src/main/java/disscount/shoppingList/service/ShoppingListCopyService.java @@ -0,0 +1,86 @@ +package disscount.shoppingList.service; + +import lombok.RequiredArgsConstructor; +import org.springframework.stereotype.Service; +import org.springframework.transaction.annotation.Transactional; + +import disscount.exceptions.ForbiddenException; +import disscount.shoppingList.dao.ShoppingListRepository; +import disscount.shoppingList.domain.ListAccess; +import disscount.shoppingList.domain.ShoppingList; +import disscount.shoppingList.dto.ShoppingListCopyRequest; +import disscount.shoppingList.dto.ShoppingListDto; +import disscount.shoppingListItem.domain.ShoppingListItem; +import disscount.user.domain.User; + +import java.util.UUID; + +/** + * Copying a list into a new one the caller owns. + * + *

Its own service because it is the one write that reads a list belonging to somebody + * else, and because it has to be a single transaction. It used to be a create followed by + * one add per item from the client, which are separate transactions: a failure partway + * left a half-populated copy behind that no retry could tidy up, and pressing the button + * again simply made another one. + */ +@Service +@RequiredArgsConstructor +@Transactional +public class ShoppingListCopyService { + + private final ShoppingListRepository shoppingListRepository; + private final ShoppingListAccessService accessService; + private final ShoppingListService shoppingListService; + private final ShoppingListMapper shoppingListMapper; + + public ShoppingListDto copy(UUID listId, UUID userId, ShoppingListCopyRequest request) { + ShoppingList source = shoppingListService.findVisible(listId, userId); + User owner = shoppingListService.requireWriteUser(userId); + + // Only an owner may carry sharing across. A recipient could otherwise copy a list + // they were merely shown and hand the owner's people a link at a level the owner + // never chose. Everything else about the copy is the caller's own, so it needs no + // further check: they are the new list's owner by construction. + if (request.isIncludeSharing() && !accessService.resolve(source, userId).isOwner()) { + throw new ForbiddenException("Only the owner can copy the sharing settings"); + } + + ShoppingList copy = ShoppingList.builder() + .owner(owner) + .title(request.getTitle()) + .linkAccess(request.isIncludeSharing() ? source.getLinkAccess() : null) + .build(); + + if (request.isIncludeItems()) { + source.getItems().stream() + .filter(item -> item.getDeletedAt() == null) + .map(item -> copyItem(item, copy, owner, request.isIncludeProgress())) + .forEach(copy.getItems()::add); + } + + // Cascades to the items, so the list and everything on it commit as one. + return shoppingListMapper.toDto(shoppingListRepository.save(copy), ListAccess.OWNER); + } + + private ShoppingListItem copyItem( + ShoppingListItem source, ShoppingList target, User actor, boolean includeProgress) { + return ShoppingListItem.builder() + .shoppingList(target) + .ean(source.getEan()) + .brand(source.getBrand()) + .name(source.getName()) + .quantity(source.getQuantity()) + .unit(source.getUnit()) + .amount(source.getAmount()) + // All four move together or none do. Progress is what was ticked, where, + // and for how much, and a tick without its price is a claim with no + // evidence behind it. + .isChecked(includeProgress ? source.getIsChecked() : false) + .chainCode(includeProgress ? source.getChainCode() : null) + .avgPrice(includeProgress ? source.getAvgPrice() : null) + .storePrice(includeProgress ? source.getStorePrice() : null) + .updatedByUser(actor) + .build(); + } +} diff --git a/backend/src/main/java/disscount/shoppingList/service/ShoppingListMapper.java b/backend/src/main/java/disscount/shoppingList/service/ShoppingListMapper.java index 656ef09a..da91f36f 100644 --- a/backend/src/main/java/disscount/shoppingList/service/ShoppingListMapper.java +++ b/backend/src/main/java/disscount/shoppingList/service/ShoppingListMapper.java @@ -35,7 +35,6 @@ public ShoppingListDto toDto(ShoppingList list, ListAccess access) { .ownerId(isOwner ? list.getOwner().getId() : null) .title(list.getTitle()) .linkAccess(isOwner ? list.resolvedLinkAccess() : null) - .shareToken(isOwner ? list.getShareToken() : null) .myAccess(access) .updatedAt(list.getUpdatedAt()) .createdAt(list.getCreatedAt()) diff --git a/backend/src/main/java/disscount/shoppingList/service/ShoppingListService.java b/backend/src/main/java/disscount/shoppingList/service/ShoppingListService.java index d73c8685..5432b01f 100644 --- a/backend/src/main/java/disscount/shoppingList/service/ShoppingListService.java +++ b/backend/src/main/java/disscount/shoppingList/service/ShoppingListService.java @@ -4,7 +4,8 @@ import org.springframework.stereotype.Service; import org.springframework.transaction.annotation.Transactional; -import disscount.exceptions.BadRequestException; +import disscount.exceptions.ForbiddenException; +import disscount.exceptions.NotFoundException; import disscount.exceptions.UnauthorizedException; import disscount.shoppingList.dao.ShoppingListRepository; import disscount.shoppingList.domain.LinkAccess; @@ -17,13 +18,15 @@ import disscount.util.Timestamps; import java.util.List; -import java.util.Optional; import java.util.UUID; import java.util.stream.Collectors; /** - * The owner's view of their own lists. Everything here is owner-only; access granted by a - * share link runs through {@link SharedShoppingListService} instead. + * Every path to a shopping list, for owners and link visitors alike. + * + *

The by-id methods run where a bearer token is optional, so {@code userId} may be null + * and the framework guarantees nothing: authorization is this class's job. See + * {@code docs/SHARING.md} §4. */ @Service @RequiredArgsConstructor @@ -32,19 +35,19 @@ public class ShoppingListService { private final ShoppingListRepository shoppingListRepository; private final UserRepository userRepository; + private final ShoppingListAccessService accessService; private final ShoppingListMapper shoppingListMapper; public ShoppingListDto createShoppingList(UUID ownerId, ShoppingListRequest request) { - User owner = userRepository.findById(ownerId) - .orElseThrow(() -> new UnauthorizedException("User not found")); + User owner = requireUser(ownerId); - // New lists are always private, which also makes "copy list" private by construction. - // Sharing needs a persisted id to bind a token to, so it is turned on afterwards. ShoppingList shoppingList = ShoppingList.builder() .owner(owner) .title(request.getTitle()) .build(); + applyLinkAccess(shoppingList, request.getLinkAccess()); + shoppingList = shoppingListRepository.save(shoppingList); return shoppingListMapper.toDto(shoppingList, ListAccess.OWNER); } @@ -53,8 +56,7 @@ public ShoppingListDto createShoppingList(UUID ownerId, ShoppingListRequest requ // flush at commit for a query that never writes. @Transactional(readOnly = true) public List getUserShoppingLists(UUID ownerId) { - User owner = userRepository.findById(ownerId) - .orElseThrow(() -> new UnauthorizedException("User not found")); + User owner = requireUser(ownerId); return shoppingListRepository.findActiveByOwner(owner) .stream() @@ -63,63 +65,74 @@ public List getUserShoppingLists(UUID ownerId) { } @Transactional(readOnly = true) - public Optional getShoppingListById(UUID listId, UUID ownerId) { - User owner = userRepository.findById(ownerId) - .orElseThrow(() -> new UnauthorizedException("User not found")); - - return shoppingListRepository.findActiveByIdAndOwner(listId, owner) - .map(list -> shoppingListMapper.toDto(list, ListAccess.OWNER)); + public ShoppingListDto getShoppingListById(UUID listId, UUID userId) { + ShoppingList list = findVisible(listId, userId); + return shoppingListMapper.toDto(list, accessService.resolve(list, userId)); } - public ShoppingListDto updateShoppingList(UUID listId, UUID ownerId, ShoppingListRequest request) { - User owner = userRepository.findById(ownerId) - .orElseThrow(() -> new UnauthorizedException("User not found")); + /** Renaming and resharing are separate rights; folding them lets EDIT reshare. */ + public ShoppingListDto updateShoppingList(UUID listId, UUID userId, ShoppingListRequest request) { + ShoppingList list = findVisible(listId, userId); + ListAccess access = accessService.resolve(list, userId); + + if (request.getLinkAccess() != null && !access.canManageShare()) { + throw new ForbiddenException("Only the owner can change who has access"); + } - ShoppingList shoppingList = shoppingListRepository.findActiveByIdAndOwner(listId, owner) - .orElseThrow(() -> new BadRequestException("Shopping list not found")); + requireWriteUser(userId); + if (!access.canEditItems()) { + throw new ForbiddenException("Insufficient access to this shopping list"); + } - shoppingList.setTitle(request.getTitle()); - applyLinkAccess(shoppingList, request.getLinkAccess()); + list.setTitle(request.getTitle()); + applyLinkAccess(list, request.getLinkAccess()); - shoppingList = shoppingListRepository.save(shoppingList); - return shoppingListMapper.toDto(shoppingList, ListAccess.OWNER); + return shoppingListMapper.toDto(shoppingListRepository.save(list), access); } - public void deleteShoppingList(UUID listId, UUID ownerId) { - User owner = userRepository.findById(ownerId) - .orElseThrow(() -> new UnauthorizedException("User not found")); + public void deleteShoppingList(UUID listId, UUID userId) { + ShoppingList list = findVisible(listId, userId); - ShoppingList shoppingList = shoppingListRepository.findActiveByIdAndOwner(listId, owner) - .orElseThrow(() -> new BadRequestException("Shopping list not found")); + if (!accessService.resolve(list, userId).isOwner()) { + throw new ForbiddenException("Only the owner can delete this shopping list"); + } - shoppingList.setDeletedAt(Timestamps.nowUtc()); - shoppingListRepository.save(shoppingList); + list.setDeletedAt(Timestamps.nowUtc()); + shoppingListRepository.save(list); } - /** - * Re-enabling a link mints a fresh token, so turning sharing off and on again is a real - * revoke. Merely changing the level leaves the token alone, since the people already - * holding the link are meant to keep working at the new level. - */ - private void applyLinkAccess(ShoppingList list, LinkAccess requested) { - if (requested == null) { - return; + /** Not-found rather than forbidden, so a 403 cannot confirm an id is real. */ + public ShoppingList findVisible(UUID listId, UUID userId) { + ShoppingList list = shoppingListRepository.findActiveById(listId) + .orElseThrow(() -> new NotFoundException("Shopping list not found")); + + if (!accessService.resolve(list, userId).canView()) { + throw new NotFoundException("Shopping list not found"); } - ListAccess next = requested.toListAccess(); - if (next == list.resolvedLinkAccess()) { - return; + return list; + } + + /** Anonymous callers cap at VIEW, so every write stays attributable. */ + public User requireWriteUser(UUID userId) { + if (userId == null) { + throw new UnauthorizedException("Sign in to change this shopping list"); } + return requireUser(userId); + } - if (next == ListAccess.NONE) { - list.setLinkAccess(null); - list.setShareToken(null); + private User requireUser(UUID userId) { + return userRepository.findById(userId) + .orElseThrow(() -> new UnauthorizedException("User not found")); + } + + /** Off then on hands back the same URL, since the URL is the id. Intended. */ + private void applyLinkAccess(ShoppingList list, LinkAccess requested) { + if (requested == null) { return; } - list.setLinkAccess(next); - if (list.getShareToken() == null) { - list.setShareToken(UUID.randomUUID()); - } + ListAccess next = requested.toListAccess(); + list.setLinkAccess(next == ListAccess.NONE ? null : next); } } diff --git a/backend/src/main/java/disscount/shoppingListItem/rest/ShoppingListItemController.java b/backend/src/main/java/disscount/shoppingListItem/rest/ShoppingListItemController.java index dd940788..c8ffadd7 100644 --- a/backend/src/main/java/disscount/shoppingListItem/rest/ShoppingListItemController.java +++ b/backend/src/main/java/disscount/shoppingListItem/rest/ShoppingListItemController.java @@ -22,13 +22,16 @@ public class ShoppingListItemController { private final ShoppingListItemService shoppingListItemService; + // These run on the chain where a bearer token is optional, so the caller may be + // anonymous and currentUserId() may be null. ShoppingListItemService resolves the + // caller's access on every one of them; nothing here may assume an owner. + @Operation(summary = "Add item to shopping list") @PostMapping public ResponseEntity addItemToShoppingList( @PathVariable UUID listId, @Valid @RequestBody ShoppingListItemRequest request) { - UUID ownerId = SecurityUtils.getCurrentUserId(); - ShoppingListItemDto created = shoppingListItemService.addItemToShoppingList(listId, ownerId, request); + ShoppingListItemDto created = shoppingListItemService.addItemToShoppingList(listId, currentUserId(), request); return ResponseEntity.ok(created); } @@ -38,8 +41,7 @@ public ResponseEntity updateShoppingListItem( @PathVariable UUID listId, @PathVariable UUID itemId, @Valid @RequestBody ShoppingListItemRequest request) { - UUID ownerId = SecurityUtils.getCurrentUserId(); - ShoppingListItemDto updated = shoppingListItemService.updateShoppingListItem(listId, itemId, ownerId, request); + ShoppingListItemDto updated = shoppingListItemService.updateShoppingListItem(listId, itemId, currentUserId(), request); return ResponseEntity.ok(updated); } @@ -48,8 +50,12 @@ public ResponseEntity updateShoppingListItem( public ResponseEntity deleteShoppingListItem( @PathVariable UUID listId, @PathVariable UUID itemId) { - UUID ownerId = SecurityUtils.getCurrentUserId(); - shoppingListItemService.deleteShoppingListItem(listId, itemId, ownerId); + shoppingListItemService.deleteShoppingListItem(listId, itemId, currentUserId()); return ResponseEntity.noContent().build(); } + + /** Null for an anonymous caller, which is a legitimate state on these routes. */ + private UUID currentUserId() { + return SecurityUtils.getCurrentUserIdOptional().orElse(null); + } } diff --git a/backend/src/main/java/disscount/shoppingListItem/service/ShoppingListItemService.java b/backend/src/main/java/disscount/shoppingListItem/service/ShoppingListItemService.java index e5051e19..1af5d222 100644 --- a/backend/src/main/java/disscount/shoppingListItem/service/ShoppingListItemService.java +++ b/backend/src/main/java/disscount/shoppingListItem/service/ShoppingListItemService.java @@ -4,12 +4,15 @@ import org.springframework.stereotype.Service; import org.springframework.transaction.annotation.Transactional; -import disscount.exceptions.BadRequestException; +import disscount.exceptions.ForbiddenException; +import disscount.exceptions.NotFoundException; import disscount.exceptions.UnauthorizedException; import disscount.shoppingList.dao.ShoppingListRepository; import disscount.shoppingList.domain.ListAccess; import disscount.shoppingList.domain.ShoppingList; +import disscount.shoppingList.service.ShoppingListAccessService; import disscount.shoppingList.service.ShoppingListMapper; +import disscount.shoppingList.service.ShoppingListService; import disscount.shoppingListItem.dao.ShoppingListItemRepository; import disscount.shoppingListItem.domain.ShoppingListItem; import disscount.shoppingListItem.dto.ShoppingListItemDto; @@ -24,9 +27,12 @@ import java.util.stream.Collectors; /** - * The owner's own items. Writes granted by a share link go through - * {@link disscount.shoppingList.service.SharedShoppingListService} instead, so that the token - * has to travel with the request. + * Items on a list, for owners and link visitors alike. + * + *

Like {@link ShoppingListService}, these run where a bearer token is optional, so each + * method resolves the caller's access rather than assuming an owner. The list is loaded + * through {@code findVisible}, which answers not-found for anything the caller may not see, + * so an item endpoint cannot be used to probe for list ids either. */ @Service @RequiredArgsConstructor @@ -36,29 +42,33 @@ public class ShoppingListItemService { private final ShoppingListItemRepository shoppingListItemRepository; private final ShoppingListRepository shoppingListRepository; private final UserRepository userRepository; + private final ShoppingListAccessService accessService; + private final ShoppingListService shoppingListService; private final ShoppingListMapper shoppingListMapper; - public ShoppingListItemDto addItemToShoppingList(UUID shoppingListId, UUID ownerId, ShoppingListItemRequest request) { - User owner = userRepository.findById(ownerId) - .orElseThrow(() -> new UnauthorizedException("User not found")); + /** + * Owner only. No link level grants item creation: an unbounded right to add to someone + * else's list waits for per-person revocation, which is version 2's job. + */ + public ShoppingListItemDto addItemToShoppingList(UUID shoppingListId, UUID userId, ShoppingListItemRequest request) { + ShoppingList shoppingList = shoppingListService.findVisible(shoppingListId, userId); + User actor = shoppingListService.requireWriteUser(userId); - ShoppingList shoppingList = shoppingListRepository.findActiveByIdAndOwner(shoppingListId, owner) - .orElseThrow(() -> new BadRequestException("Shopping list not found or access denied")); + if (!accessService.resolve(shoppingList, userId).isOwner()) { + throw new ForbiddenException("Only the owner can add items to this shopping list"); + } - // Check if item with same name already exists in the shopping list Optional existingItem = shoppingListItemRepository .findActiveByShoppingListAndName(shoppingList, request.getName()); ShoppingListItem item; if (existingItem.isPresent()) { - // Item exists, increase the amount item = existingItem.get(); int requestedAmount = request.getAmount() != null ? request.getAmount() : 1; // Cap the merged total at the same limit the request DTO enforces (@Max) int newAmount = Math.min(item.getAmount() + requestedAmount, 999); item.setAmount(newAmount); - // Update other fields with new values if provided if (request.getEan() != null) item.setEan(request.getEan()); if (request.getBrand() != null) item.setBrand(request.getBrand()); if (request.getQuantity() != null) item.setQuantity(request.getQuantity()); @@ -67,11 +77,9 @@ public ShoppingListItemDto addItemToShoppingList(UUID shoppingListId, UUID owner if (request.getAvgPrice() != null) item.setAvgPrice(request.getAvgPrice()); if (request.getStorePrice() != null) item.setStorePrice(request.getStorePrice()); - // Update tracking fields item.setUpdatedAt(Timestamps.nowUtc()); - item.setUpdatedByUser(owner); + item.setUpdatedByUser(actor); } else { - // Create new item item = ShoppingListItem.builder() .shoppingList(shoppingList) .ean(request.getEan()) @@ -84,62 +92,45 @@ public ShoppingListItemDto addItemToShoppingList(UUID shoppingListId, UUID owner .chainCode(request.getChainCode()) .avgPrice(request.getAvgPrice()) .storePrice(request.getStorePrice()) - .updatedByUser(owner) + .updatedByUser(actor) .build(); } item = shoppingListItemRepository.save(item); - - // Update the shopping list's updatedAt timestamp - shoppingList.setUpdatedAt(Timestamps.nowUtc()); - shoppingListRepository.save(shoppingList); + touchList(shoppingList); return shoppingListMapper.toItemDto(item, ListAccess.OWNER); } - public ShoppingListItemDto updateShoppingListItem(UUID listId, UUID itemId, UUID ownerId, ShoppingListItemRequest request) { - User owner = userRepository.findById(ownerId) - .orElseThrow(() -> new UnauthorizedException("User not found")); + /** SHOP is enough: ticking off and choosing a store is the in-the-shop level. */ + public ShoppingListItemDto updateShoppingListItem( + UUID listId, UUID itemId, UUID userId, ShoppingListItemRequest request) { + ShoppingList list = shoppingListService.findVisible(listId, userId); + User actor = shoppingListService.requireWriteUser(userId); + ListAccess access = requireAccess(list, userId, ListAccess::canCheck); - ShoppingListItem item = findOwnedItem(listId, itemId, owner); + ShoppingListItem item = findItem(list, itemId); - // Update fields - item.setEan(request.getEan()); - item.setBrand(request.getBrand()); - item.setName(request.getName()); - item.setQuantity(request.getQuantity()); - item.setUnit(request.getUnit()); - item.setAmount(request.getAmount() != null ? request.getAmount() : 1); - item.setIsChecked(request.getIsChecked() != null ? request.getIsChecked() : false); - item.setChainCode(request.getChainCode()); - item.setAvgPrice(request.getAvgPrice()); - item.setStorePrice(request.getStorePrice()); - - // Update tracking fields + applyItemUpdate(item, request, access); item.setUpdatedAt(Timestamps.nowUtc()); - item.setUpdatedByUser(owner); + item.setUpdatedByUser(actor); item = shoppingListItemRepository.save(item); + touchList(list); - // Update the shopping list's updatedAt timestamp - item.getShoppingList().setUpdatedAt(Timestamps.nowUtc()); - shoppingListRepository.save(item.getShoppingList()); - - return shoppingListMapper.toItemDto(item, ListAccess.OWNER); + return shoppingListMapper.toItemDto(item, access); } - public void deleteShoppingListItem(UUID listId, UUID itemId, UUID ownerId) { - User owner = userRepository.findById(ownerId) - .orElseThrow(() -> new UnauthorizedException("User not found")); + public void deleteShoppingListItem(UUID listId, UUID itemId, UUID userId) { + ShoppingList list = shoppingListService.findVisible(listId, userId); + shoppingListService.requireWriteUser(userId); + requireAccess(list, userId, ListAccess::canEditItems); - ShoppingListItem item = findOwnedItem(listId, itemId, owner); + ShoppingListItem item = findItem(list, itemId); item.setDeletedAt(Timestamps.nowUtc()); shoppingListItemRepository.save(item); - - // Update the shopping list's updatedAt timestamp - item.getShoppingList().setUpdatedAt(Timestamps.nowUtc()); - shoppingListRepository.save(item.getShoppingList()); + touchList(list); } public List getUserShoppingListItems(UUID ownerId) { @@ -152,12 +143,46 @@ public List getUserShoppingListItems(UUID ownerId) { .collect(Collectors.toList()); } - /** The item has to belong both to the list in the path and to the caller. */ - private ShoppingListItem findOwnedItem(UUID listId, UUID itemId, User owner) { - ShoppingList shoppingList = shoppingListRepository.findActiveByIdAndOwner(listId, owner) - .orElseThrow(() -> new BadRequestException("Shopping list not found or access denied")); + private ShoppingListItem findItem(ShoppingList list, UUID itemId) { + return shoppingListItemRepository.findActiveByIdAndShoppingList(itemId, list) + .orElseThrow(() -> new NotFoundException("Shopping list item not found")); + } + + private ListAccess requireAccess( + ShoppingList list, UUID userId, java.util.function.Predicate allowed) { + ListAccess access = accessService.resolve(list, userId); + if (!allowed.test(access)) { + throw new ForbiddenException("Insufficient access to this shopping list"); + } + return access; + } + + /** + * SHOP is the in-the-shop level: what got ticked, which store it is coming from, and the + * prices captured at that moment. Everything structural is left as the server has it, so a + * SHOP-level caller cannot rename or resize an item by sending a fuller payload. + */ + private void applyItemUpdate(ShoppingListItem item, ShoppingListItemRequest request, ListAccess access) { + item.setIsChecked(request.getIsChecked() != null ? request.getIsChecked() : false); + item.setChainCode(request.getChainCode()); + item.setAvgPrice(request.getAvgPrice()); + item.setStorePrice(request.getStorePrice()); + + if (!access.canEditItems()) { + return; + } + + item.setEan(request.getEan()); + item.setBrand(request.getBrand()); + item.setName(request.getName()); + item.setQuantity(request.getQuantity()); + item.setUnit(request.getUnit()); + item.setAmount(request.getAmount() != null ? request.getAmount() : 1); + } - return shoppingListItemRepository.findActiveByIdAndShoppingList(itemId, shoppingList) - .orElseThrow(() -> new BadRequestException("Shopping list item not found or access denied")); + /** Item activity reorders the owner's list index, which sorts by updatedAt. */ + private void touchList(ShoppingList list) { + list.setUpdatedAt(Timestamps.nowUtc()); + shoppingListRepository.save(list); } } diff --git a/backend/src/main/java/disscount/user/dao/UserRepository.java b/backend/src/main/java/disscount/user/dao/UserRepository.java index 9f618670..87c2f9ec 100644 --- a/backend/src/main/java/disscount/user/dao/UserRepository.java +++ b/backend/src/main/java/disscount/user/dao/UserRepository.java @@ -12,4 +12,11 @@ public interface UserRepository extends JpaRepository { List findByDeletedAtIsNullOrderByCreatedAtAsc(); + + /** + * Spans soft-deleted rows too, matching the unique index, which does not care that an + * account is gone. In practice they never collide: deleteAccount nulls the username, + * which is what frees the name for somebody else. + */ + boolean existsByUsername(String username); } diff --git a/backend/src/main/java/disscount/user/domain/User.java b/backend/src/main/java/disscount/user/domain/User.java index 2316014e..22bf3d37 100644 --- a/backend/src/main/java/disscount/user/domain/User.java +++ b/backend/src/main/java/disscount/user/domain/User.java @@ -27,7 +27,11 @@ public class User { @Id private UUID id; - @Column(nullable = true) + // Unique, so version 2 can invite somebody by name and mean one person. Nullable + // rather than required, because deleteAccount nulls it to free the name, and Postgres + // lets a unique index hold as many nulls as it likes. Presence is the frontend's job: + // the settings form will not save without one. + @Column(nullable = true, unique = true) private String username; // Preference toggles are stored as timestamps instead of booleans: null = off, diff --git a/backend/src/main/java/disscount/user/service/UserProfileCreator.java b/backend/src/main/java/disscount/user/service/UserProfileCreator.java new file mode 100644 index 00000000..70d3dc8a --- /dev/null +++ b/backend/src/main/java/disscount/user/service/UserProfileCreator.java @@ -0,0 +1,27 @@ +package disscount.user.service; + +import lombok.RequiredArgsConstructor; +import org.springframework.stereotype.Service; +import org.springframework.transaction.annotation.Propagation; +import org.springframework.transaction.annotation.Transactional; + +import disscount.user.dao.UserRepository; +import disscount.user.domain.User; + +/** + * The one insert that provisions a profile, in a transaction of its own. + * + *

Its own bean because REQUIRES_NEW only takes effect through the proxy, and because a + * failed insert has to leave the caller's transaction usable. Reasoning in `docs/AUTH.md`. + */ +@Service +@RequiredArgsConstructor +public class UserProfileCreator { + + private final UserRepository userRepository; + + @Transactional(propagation = Propagation.REQUIRES_NEW) + public void create(User user) { + userRepository.saveAndFlush(user); + } +} diff --git a/backend/src/main/java/disscount/user/service/UserService.java b/backend/src/main/java/disscount/user/service/UserService.java index f48237f6..90291190 100644 --- a/backend/src/main/java/disscount/user/service/UserService.java +++ b/backend/src/main/java/disscount/user/service/UserService.java @@ -5,6 +5,7 @@ import org.springframework.transaction.annotation.Transactional; import disscount.exceptions.BadRequestException; +import disscount.exceptions.ConflictException; import disscount.user.dao.AuthIdentityDao; import disscount.user.dao.UserRepository; import disscount.user.domain.User; @@ -39,6 +40,7 @@ public class UserService { private final UserRepository userRepository; private final AuthIdentityDao authIdentityDao; + private final UserProfileCreator profileCreator; // Compared against better-auth's UTC session timestamps, so the JVM zone must not leak in. private static LocalDateTime nowUtc() { @@ -90,20 +92,25 @@ public void ensureActiveProfile(UUID id, String email, String name, String image String username = seedUsername(name, email); // Every switch starts ON; the stamped timestamp is what the settings form reads back. LocalDateTime now = nowUtc(); + User.UserBuilder profile = User.builder() + .id(id) + .image(image) + .accountType(accountType) + .notificationsPushEnabledAt(now) + .notificationsEmailEnabledAt(now) + .newsletterEnabledAt(now) + .feedbackContactEnabledAt(now) + .lastActiveAt(now); + try { - userRepository.save(User.builder() - .id(id) - .username(username) - .image(image) - .accountType(accountType) - .notificationsPushEnabledAt(now) - .notificationsEmailEnabledAt(now) - .newsletterEnabledAt(now) - .feedbackContactEnabledAt(now) - .lastActiveAt(now) - .build()); - } catch (DataIntegrityViolationException ignored) { - // Concurrent first-login race: the other request won - profile already exists + profileCreator.create(profile.username(username).build()); + } catch (DataIntegrityViolationException collision) { + // A concurrent first login, or two accounts seeding one username. The insert + // ran in its own transaction, so this one is still usable and can retry. + if (userRepository.existsById(id)) return; + + // Nameless rather than fail: the settings form asks for one immediately. + profileCreator.create(profile.username(null).build()); } } } @@ -114,17 +121,34 @@ private boolean isActivityStampStale(LocalDateTime lastActiveAt) { } /** - * Seeds a username for a brand-new profile from the provider display name, - * falling back to the email local-part when the name is missing. - * Usernames are not unique, so no de-duplication is needed. + * Seeds a username for a brand-new profile from the provider display name, falling back + * to the email local-part when the name is missing. + * + *

Suffixes on a collision rather than refusing: nobody is present to choose on a + * first login. The user-facing edit refuses instead. */ private String seedUsername(String name, String email) { - if (name != null && !name.isBlank()) { - return name.trim(); + String base = (name != null && !name.isBlank()) + ? name.trim() + : email.split("@")[0]; + + if (base.isBlank()) { + return null; } - String localPart = email.split("@")[0]; - return localPart.isBlank() ? null : localPart; + if (!userRepository.existsByUsername(base)) { + return base; + } + + // Bounded: past the cap a null name beats a slow loop, and the form will ask. + for (int suffix = 1; suffix <= 100; suffix++) { + String candidate = base + suffix; + if (!userRepository.existsByUsername(candidate)) { + return candidate; + } + } + + return null; } public UserDto updateProfile(UUID userId, UserRequest request) { @@ -134,6 +158,11 @@ public UserDto updateProfile(UUID userId, UserRequest request) { String username = request.getUsername(); if (username != null && !username.equals(user.getUsername())) { + // Checked here so the answer carries fieldErrors; the unique index catches the + // race and renders the same 409. + if (userRepository.existsByUsername(username)) { + throw new ConflictException("Korisničko ime je već zauzeto."); + } user.setUsername(username); } diff --git a/docs/AUTH.md b/docs/AUTH.md index f83a6fb7..035a7b0d 100644 --- a/docs/AUTH.md +++ b/docs/AUTH.md @@ -124,9 +124,12 @@ Mapped codes: `email_not_found`, `email_doesn't_match`, `account_already_linked_ - A `NimbusJwtDecoder` is built with the JWKS URI and pinned to `ES256`, and it validates the token issuer (`better.auth.issuer`). - The session policy is `STATELESS` (no server session; the JWT is the whole story) and CSRF is disabled (there is no cookie-based auth to protect). -- Public endpoints: `/actuator/health`, Swagger, the OpenAPI docs, `POST /api/contact`, and `/api/shared/**`. Everything else requires a valid token. -- `/api/shared/**` (shopping list sharing) has its own `@Order(1)` filter chain. It is the one place where a bearer token is **optional**: the caller may legitimately be anonymous, and authorization is done in application code against the list's `share_token` and `link_access` via `SharedShoppingListService` / `ShoppingListAccessService`. `permitAll` on the main chain would not be enough, because `BearerTokenAuthenticationFilter` answers 401 for an expired or malformed token before authorization is consulted, so a stale cached token would lock a visitor out of a link that works. An `OptionalBearerAuthenticationFilter` decodes the token when it can and falls through to anonymous when it cannot. -- A `UserProvisioningFilter` runs after the bearer-token filter. On the first authenticated request it lazily upserts the `app_user` profile row (same UUID as the Better Auth user) via `UserService.ensureActiveProfile`, seeding the username from the provider name (falling back to the email local-part). `app_user.username` is deliberately **not unique**; only email is. +- Public endpoints: `/actuator/health`, Swagger, the OpenAPI docs, `POST /api/contact`, and the by-id shopping list routes. Everything else requires a valid token. +- The by-id shopping list routes have their own `@Order(1)` filter chain. They are the one place where a bearer token is **optional**: a list is shared at its own URL, so the caller may legitimately be anonymous, and authorization is done in application code against `link_access` via `ShoppingListAccessService`. `permitAll` on the main chain would not be enough, because `BearerTokenAuthenticationFilter` answers 401 for an expired or malformed token before authorization is consulted, so a stale cached token would lock a visitor out of a link that works. An `OptionalBearerAuthenticationFilter` decodes the token when it can and falls through to anonymous when it cannot. +- **That chain's own rule is `permitAll`, so the service-layer checks are the authentication boundary.** `UuidScopedRequestMatcher` keeps the chain to six routes, matched by method plus a UUID-shaped id: `/api/shopping-lists/me` and `/api/shopping-lists/items` stay authenticated because they are not UUIDs, not because they are listed as exceptions. See `docs/SHARING.md` §4. +- A `UserProvisioningFilter` runs after the bearer-token filter. On the first authenticated request it lazily upserts the `app_user` profile row (same UUID as the Better Auth user) via `UserService.ensureActiveProfile`, seeding the username from the provider name (falling back to the email local-part, and suffixing a digit on a collision). `app_user.username` is **unique** as of August 2026, so version 2 can invite somebody by name and mean one person. It stays nullable, because deleting an account nulls it to free the name and Postgres lets a unique index hold any number of nulls. Presence is the frontend's rule. + - The seeding insert goes through `UserProfileCreator`, a separate bean, so `REQUIRES_NEW` takes effect through the proxy. A unique violation aborts the database transaction, and retrying inside the same one could never commit, so the insert needs a transaction the caller can outlive. On a collision the retry first re-reads by id, since the usual cause is a concurrent first login that already created the row. + - A collision that reaches the database surfaces as `DataIntegrityViolationException`, not `DuplicateKeyException`: Hibernate only raises the latter for a session-level id clash. `GlobalExceptionHandler` therefore catches the general type and narrows on SQLState `23505`, which is `unique_violation` in both PostgreSQL and the H2 the tests run on. Anything else falls through to the 500. ## 8. Password reset, set password, and change email @@ -219,16 +222,16 @@ Backend (`backend/.env`, referenced in `application.properties`): ## 10. Automatic vs manual -| Concern | Automatic | Needs manual work | -| ------------------------------------------------------------------ | ---------------------------------------------------- | ----------------------------------------------------------------------------------------------- | -| Auth schema (`user`, `session`, `account`, `verification`, `jwks`) | Managed by Drizzle | Regenerate/migrate when Better Auth config that changes tables is added | -| JWT signing keys | Better Auth generates and rotates into `jwks` | None | -| OAuth email verified for linking | `databaseHooks.account.create.after` | None | -| Email verification on signup | Sent automatically (`sendOnSignUp`) | None | -| Sending real email | Resend, once `RESEND_API_KEY` + `EMAIL_FROM` are set | Verify the sending domain in Resend (done for `disscount.me`) | -| Facebook availability | Gated by a constant | Flip `FACEBOOK_COMING_SOON` after Meta verification | -| `app_user.username` uniqueness dropped | In the entity + local DB | **Apply the constraint drop on the prod DB manually** (Hibernate `ddl-auto=update` never drops) | -| New auth config taking effect | Not automatic in dev | **Restart the dev server after editing `lib/auth/index.ts`** (see gotchas) | +| Concern | Automatic | Needs manual work | +| ------------------------------------------------------------------ | ---------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------- | +| Auth schema (`user`, `session`, `account`, `verification`, `jwks`) | Managed by Drizzle | Regenerate/migrate when Better Auth config that changes tables is added | +| JWT signing keys | Better Auth generates and rotates into `jwks` | None | +| OAuth email verified for linking | `databaseHooks.account.create.after` | None | +| Email verification on signup | Sent automatically (`sendOnSignUp`) | None | +| Sending real email | Resend, once `RESEND_API_KEY` + `EMAIL_FROM` are set | Verify the sending domain in Resend (done for `disscount.me`) | +| Facebook availability | Gated by a constant | Flip `FACEBOOK_COMING_SOON` after Meta verification | +| `app_user.username` uniqueness | Entity + all databases | Applied by hand, because `ddl-auto=update` will not add a unique index to a populated table and logs the failure rather than refusing to start | +| New auth config taking effect | Not automatic in dev | **Restart the dev server after editing `lib/auth/index.ts`** (see gotchas) | ## 11. Key files @@ -292,7 +295,7 @@ Backend: Spring Boot `3.1.0` on Java `21`, using `spring-boot-starter-oauth2-res - **Email hooks are fire-and-forget (`void`) on purpose.** Awaiting them would make the response time reveal whether an email exists (a timing oracle). Each has a non-PII `.catch` so a failed send is logged, not lost. - **Keep the reset `redirectTo` query-free.** Better Auth appends `?token=...` to `redirectTo`, so it must not already contain a query. This is why the reset link points at `/reset-password` (which then forwards to `/?modal=reset-password&token=...`) rather than at `/?modal=reset-password` directly, which would produce a broken double-`?` URL. The verification and change-email `callbackURL`s are fine with a query because Better Auth passes them through its redirect handling, not naive string concatenation. - **The reset modal strips its token from the URL.** `ResetPasswordModal` captures the token on first open and `replaceState`s it out of the address bar, so it does not sit in browser history or leak via Referer once the homepage loads its usual content. -- **`app_user.username` uniqueness was dropped in the entity and the local DB, but `ddl-auto=update` never drops constraints.** The production database still needs a manual `ALTER TABLE app_user DROP CONSTRAINT ...`. +- **`ddl-auto=update` will not add a unique index to a populated table**, and it logs the failure rather than refusing to start, so the app runs happily without the constraint it thinks it has. `app_user.username`'s index was created by hand after de-duplicating. Any future uniqueness needs the same treatment, and needs verifying afterwards rather than assuming. - **The stack stores naive timestamps, so a zone has to be chosen explicitly on both sides.** `app_user` and Better Auth's drizzle `session` / `user` tables are all `timestamp` without a time zone, and Drizzle writes UTC. Spring must match: `UserService.nowUtc()` stamps in UTC rather than the JVM default zone, otherwise the same column carries a different clock base per environment and comparisons against `MAX(session.updated_at)` silently favour whichever side is ahead. - **`UserDto.lastLoginAt` and `lastActiveAt` are `Instant`, not `LocalDateTime`, and that is deliberate.** Jackson serialises a `LocalDateTime` with no offset, and `new Date("2026-07-24T13:05:11.123")` in the browser reads that as **local** time. The admin dashboard does date maths on both fields, so an offset-less value made a just-signed-in user render as "prije 2 sata" and shifted the WAU/MAU window by each admin's UTC offset. Any future field the frontend compares against `Date.now()` needs the same treatment. - **`lastLoginAt` is derived, not stored.** It comes from `MAX(session.created_at)` in `AuthIdentityDao`, and only on the admin list endpoint. Signing out deletes those rows, so a long-standing user can read as "never logged in" (tracked in #117). `lastActiveAt` is the stamped column and does survive. @@ -300,7 +303,6 @@ Backend: Spring Boot `3.1.0` on Java `21`, using `spring-boot-starter-oauth2-res ## 14. Future improvements and TODOs - **Enable Facebook (re-audit the linking hook first):** complete Meta Business Verification (needs a registered entity, for example a Croatian obrt), then set `FACEBOOK_COMING_SOON = false`. Before flipping it, revisit `databaseHooks.account.create.after`: it marks OAuth emails verified, which is how Facebook (no verified-email claim) links without the deprecated `requireLocalEmailVerified` flag. Facebook does not guarantee a verified email and phone-only users have none, so a verified-provider-email gate is not viable; the better-auth core auto-link CVE is already patched in `1.6.14`, and the flow stays gated behind `FACEBOOK_COMING_SOON` until then. -- **Apply the `app_user.username` constraint drop on the production database.** - **Dedicated register/set-password email copy:** the "set your password" case currently reuses reset wording, which reads slightly oddly for a first-time set. - **Deliverability hardening before any marketing mail:** SPF/DKIM/DMARC, a dedicated sending subdomain, and one-click unsubscribe. Templates are Croatian only today (no i18n). - **Harden the logged-out change-email confirmation edge:** the `user.update.before` guard relies on an active session at confirmation time; a logged-out click skips that layer (the request-time POST guard and UI gate still cover the normal path). diff --git a/docs/LANDING.md b/docs/LANDING.md index 8f96885f..6b113def 100644 --- a/docs/LANDING.md +++ b/docs/LANDING.md @@ -141,15 +141,15 @@ Because `faqItems` feeds both the visible accordion and the structured data, the The landing is the app's most SEO-sensitive surface, so several layers work together. -| Layer | Where | Notes | -| ------------------------ | -------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------- | -| Page title + description | `page.tsx` `metadata` | Title fills the `Disscount - %s` template from the layout; Croatian description | -| Site-wide metadata | `app/layout.tsx` | `openGraph` (`hr_HR`), `twitter` (`summary_large_image`), keywords, robots index/follow | -| Structured data | `components/json-ld.tsx` | One `