From 0ab0609151ea983e1ce9fc312a5dfa8bcca0aa82 Mon Sep 17 00:00:00 2001 From: CrazyFreak <44674613+OffCrazyFreak@users.noreply.github.com> Date: Fri, 7 Aug 2026 13:26:58 +0200 Subject: [PATCH 01/17] docs(agents): Record how to deliver review artifacts Changes: - Require artifacts to be handed back as [name](file:///abs/path) links, and list the forms that were tested and do not work - Add the HTML layout rules: full browser width, a wide Finding column, a narrow Where column, and Now/Expected/Test split into separate blocks Tested five path forms against the terminal and only the Markdown link to a file:// URI is clickable, so the skill now names it as the only option rather than leaving it to judgement. The layout rules each record the way the page had become unreadable, so they are not mistaken for style preferences later. --- .../skills/multi-tool-code-review/SKILL.md | 2 +- .../references/03-triage-doc-format.md | 30 ++++++++++++++++++- 2 files changed, 30 insertions(+), 2 deletions(-) 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: From a2e6d1dd76199862a01e8f184b04396828e33222 Mon Sep 17 00:00:00 2001 From: CrazyFreak <44674613+OffCrazyFreak@users.noreply.github.com> Date: Fri, 7 Aug 2026 13:33:46 +0200 Subject: [PATCH 02/17] refactor(shopping-lists): Share a list by its own id, not a token Changes: - Delete SharedShoppingListController, SharedShoppingListService, the share_token column and findActiveByShareToken - Move the by-id list and item routes onto the optional-bearer chain, matched by method plus a UUID-shaped id so /me and /items stay authenticated - Resolve the caller's access in ShoppingListService and ShoppingListItemService instead of hardcoding OWNER, and port the SHOP versus EDIT field mask across - Answer not-found rather than forbidden for a list the caller may not see - Add NotFoundException and a DataIntegrityViolationException handler The token bought rotation, which protects against a scenario that does not happen: a grocery list is shared once, used for a week and abandoned. Sharing on the list's own id means the URL in the owner's address bar is the one worth sending, which is what people already expect from Google Docs. Notes: The matcher is an allowlist of method plus UUID-shaped id rather than a wildcard minus exceptions. A denylist would silently expose any future literal route such as /api/shopping-lists/archived, since it would match the wildcard and not be listed as an exception. The chain's own rule is permitAll, so the access checks in the services are now an authentication boundary rather than a convenience. Relaxing one removes authentication. This is stated in the SecurityConfig javadoc as well. Adding items stays owner-only through isOwner() rather than canManageShare(), which agrees today, so that a later decision to let a link level manage sharing cannot quietly grant item creation or deletion too. --- .../java/disscount/config/SecurityConfig.java | 45 +++++- .../config/UuidScopedRequestMatcher.java | 47 ++++++ .../exceptions/GlobalExceptionHandler.java | 32 ++++ .../exceptions/NotFoundException.java | 15 ++ .../dao/ShoppingListRepository.java | 13 +- .../shoppingList/domain/ListAccess.java | 9 ++ .../shoppingList/domain/ShoppingList.java | 9 +- .../shoppingList/dto/ShoppingListDto.java | 5 +- .../rest/SharedShoppingListController.java | 80 ---------- .../rest/ShoppingListController.java | 22 +-- .../service/SharedShoppingListService.java | 148 ------------------ .../service/ShoppingListMapper.java | 1 - .../service/ShoppingListService.java | 134 ++++++++++------ .../rest/ShoppingListItemController.java | 18 ++- .../service/ShoppingListItemService.java | 139 +++++++++------- 15 files changed, 346 insertions(+), 371 deletions(-) create mode 100644 backend/src/main/java/disscount/config/UuidScopedRequestMatcher.java create mode 100644 backend/src/main/java/disscount/exceptions/NotFoundException.java delete mode 100644 backend/src/main/java/disscount/shoppingList/rest/SharedShoppingListController.java delete mode 100644 backend/src/main/java/disscount/shoppingList/service/SharedShoppingListService.java diff --git a/backend/src/main/java/disscount/config/SecurityConfig.java b/backend/src/main/java/disscount/config/SecurityConfig.java index 8b42f283..a2cd9b3e 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,20 +72,32 @@ 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 above is {@code permitAll}, so the + * only thing between an anonymous request and a list deletion is the access check in + * {@link disscount.shoppingList.service.ShoppingListService}. Every method reachable from + * this chain resolves the caller through + * {@link disscount.shoppingList.service.ShoppingListAccessService} before it touches + * anything, and relaxing one of those checks removes an authentication boundary rather + * than a convenience. + * + *

The matcher is an allowlist of method plus path plus a UUID-shaped id, so + * {@code /me} and {@code /items} stay on the authenticated chain by virtue of not being + * UUIDs rather than by being listed as exceptions. See {@link UuidScopedRequestMatcher}. */ @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()) @@ -101,6 +115,21 @@ public SecurityFilterChain sharedShoppingListChain( return http.build(); } + /** + * Exactly the six routes an anonymous caller may reach. Listing methods explicitly means + * anything else, including PATCH and OPTIONS, falls 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}"), + 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.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..c5b65f73 --- /dev/null +++ b/backend/src/main/java/disscount/config/UuidScopedRequestMatcher.java @@ -0,0 +1,47 @@ +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. + * + *

This exists so the anonymous-capable chain is an allowlist on both axes. The obvious + * alternative, matching {@code /api/shopping-lists/**} and subtracting the authenticated + * routes, is a denylist nested inside an allowlist: the day somebody adds + * {@code GET /api/shopping-lists/archived} it matches the wildcard, is absent from the + * subtraction, and silently becomes readable without a token. Requiring a UUID excludes + * every such literal automatically, including today's {@code /me} and {@code /items}, and + * anything unlisted falls through to the authenticated chain rather than past it. + * + *

The shape test is stricter than {@link java.util.UUID#fromString}, which accepts + * "1-1-1-1-1". Both forms Spring binds to a UUID are allowed: canonical dashed, and bare + * 32-hex. + */ +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}$" + + "|^[0-9a-fA-F]{32}$"); + + 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..4a00cabc 100644 --- a/backend/src/main/java/disscount/exceptions/GlobalExceptionHandler.java +++ b/backend/src/main/java/disscount/exceptions/GlobalExceptionHandler.java @@ -1,5 +1,6 @@ package disscount.exceptions; +import org.springframework.dao.DataIntegrityViolationException; import org.springframework.http.HttpStatus; import org.springframework.http.ProblemDetail; import org.springframework.http.converter.HttpMessageNotReadableException; @@ -40,11 +41,42 @@ 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()); } + /** + * The race that slips past an application-level uniqueness check: two requests both see a + * free value and both insert. Rendered as a 409 rather than the 500 an unhandled + * constraint violation would produce. + * + *

The field error is attached only when the violated constraint is recognisable, + * because the frontend maps fieldErrors straight onto form fields and naming the wrong + * one is worse than naming none. + */ + @ExceptionHandler(DataIntegrityViolationException.class) + public ProblemDetail handleDataIntegrityViolation(DataIntegrityViolationException ex) { + log.warn("Data integrity violation", ex); + + String cause = ex.getMostSpecificCause().getMessage(); + boolean isUsername = cause != null && cause.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; + } + @ExceptionHandler(MethodArgumentNotValidException.class) public ProblemDetail handleValidationExceptions(MethodArgumentNotValidException ex) { Map fieldErrors = new HashMap<>(); 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/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/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..651f7c7b 100644 --- a/backend/src/main/java/disscount/shoppingList/rest/ShoppingListController.java +++ b/backend/src/main/java/disscount/shoppingList/rest/ShoppingListController.java @@ -41,13 +41,14 @@ 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 = "Update shopping list") @@ -55,16 +56,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 +78,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/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..bf753c1c 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,16 @@ 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 on a chain where a bearer token is optional, so {@code userId} may + * be null and the framework guarantees nothing. Authorization is therefore this class's job: + * each one loads the list, resolves the caller's access through + * {@link ShoppingListAccessService}, and refuses before touching anything. */ @Service @RequiredArgsConstructor @@ -32,19 +36,20 @@ 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(); + // A new list can be born shared, which is what the copy modal's sharing option needs. + applyLinkAccess(shoppingList, request.getLinkAccess()); + shoppingList = shoppingListRepository.save(shoppingList); return shoppingListMapper.toDto(shoppingList, ListAccess.OWNER); } @@ -53,8 +58,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() @@ -62,64 +66,98 @@ public List getUserShoppingLists(UUID ownerId) { .collect(Collectors.toList()); } + /** + * The caller may be anonymous, so this resolves rather than assuming ownership. + * + *

A list the caller cannot see answers {@link NotFoundException}, never + * {@link ForbiddenException}: a 403 would confirm the list exists, and since the id is + * now the shareable URL, that is exactly what must not leak. + */ @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")); + /** + * Two different checks in one endpoint, deliberately kept apart: renaming needs + * {@code canEditItems}, but changing who can reach the list needs {@code canManageShare}. + * Folding them together would let an EDIT recipient reshare a list they do not own. + */ + public ShoppingListDto updateShoppingList(UUID listId, UUID userId, ShoppingListRequest request) { + ShoppingList list = findVisible(listId, userId); + ListAccess access = accessService.resolve(list, userId); - ShoppingList shoppingList = shoppingListRepository.findActiveByIdAndOwner(listId, owner) - .orElseThrow(() -> new BadRequestException("Shopping list not found")); + if (request.getLinkAccess() != null && !access.canManageShare()) { + throw new ForbiddenException("Only the owner can change who has access"); + } - shoppingList.setTitle(request.getTitle()); - applyLinkAccess(shoppingList, request.getLinkAccess()); + requireWriteUser(userId); + if (!access.canEditItems()) { + throw new ForbiddenException("Insufficient access to this shopping list"); + } - shoppingList = shoppingListRepository.save(shoppingList); - return shoppingListMapper.toDto(shoppingList, ListAccess.OWNER); + list.setTitle(request.getTitle()); + applyLinkAccess(list, request.getLinkAccess()); + + 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. + * Loads a list the caller is allowed to know about, or throws not-found. + * + *

The visibility branch comes first, before any other check, so no later code path can + * answer differently and reveal that an id was real. */ - private void applyLinkAccess(ShoppingList list, LinkAccess requested) { - if (requested == null) { - return; + 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 are capped at VIEW, so every write needs a real account behind it and + * 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")); + } + + /** + * Turning sharing off and back on hands back the same URL, because the URL is the list's + * own id. That is the accepted cost of dropping the rotating token: a link that was once + * shared works again if sharing is re-enabled. + */ + 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); } } From ef396e72cdc62e196b225ed50274f5b4805d0021 Mon Sep 17 00:00:00 2001 From: CrazyFreak <44674613+OffCrazyFreak@users.noreply.github.com> Date: Fri, 7 Aug 2026 13:42:49 +0200 Subject: [PATCH 03/17] refactor(shopping-lists): Serve owners and link visitors from one route Changes: - Delete /s/[token] and read every list through /shopping-lists/[id] - Move the access banner and the unavailable state into the list feature folder - Take the shareToken parameter out of the data, item-mutation and action hooks, the header, the action buttons and the item list - Build the share URL from the list id, and drop the sharedShoppingList query root, its two offline mutation keys and the shared query functions - Gate the share and edit modals on ownership, and the share-link button on the save being in flight - Bump CACHE_BUSTER so a queued write with a retired key is not dropped in silence The detail client used to hardcode isSignedIn and say in a comment that the route was behind the auth gate. It is not any more, so it reads the session and every control gates on the access the server resolved. Notes: The access banner renders an empty element carrying its id rather than null when it has nothing to say. The disabled item controls point at that id with aria-describedby, and a list DTO persisted before myAccess existed replays with it undefined, which makes the controls disabled and the description absent at once. The by-id query inherits the 30 second staleTime and focus refetch the shared query owned. Two people shopping off one list need each other's ticks, and that read is now the only one there is. The lost-access toast moves onto the by-id item writes, which link visitors now use. An owner never reaches its branch, since you cannot lose access to your own list. --- .../components/items/shopping-list-items.tsx | 10 +- .../shopping-list-access-banner.tsx} | 23 ++-- .../shopping-list-action-buttons.tsx | 5 +- .../shopping-list-detail-client.tsx | 54 ++++---- .../[id]/components/shopping-list-header.tsx | 4 - .../components/shopping-list-unavailable.tsx} | 32 +++-- .../[id]/hooks/use-shopping-list-actions.ts | 24 ++-- .../[id]/hooks/use-shopping-list-data.ts | 17 +-- .../hooks/use-shopping-list-item-mutations.ts | 49 +------- .../components/forms/share-list-modal.tsx | 14 ++- .../components/forms/shopping-list-modal.tsx | 8 +- .../hooks/use-share-list-modal.ts | 12 +- .../shared-shopping-list-client.tsx | 116 ------------------ .../app/s/[token]/get-shared-list-preview.ts | 45 ------- frontend/src/app/s/[token]/page.tsx | 46 ------- frontend/src/lib/api/schemas/shopping-list.ts | 1 - frontend/src/lib/api/shopping-lists/hooks.ts | 104 +--------------- frontend/src/lib/api/shopping-lists/keys.ts | 16 ++- .../src/lib/api/shopping-lists/queries.ts | 39 ------ frontend/src/lib/offline/cached-query-keys.ts | 1 - .../src/lib/offline/offline-mutation-keys.ts | 7 +- frontend/src/lib/offline/offline-mutations.ts | 35 ++---- frontend/src/lib/offline/persister.ts | 6 +- frontend/src/lib/offline/purge.ts | 6 +- frontend/src/utils/shopping-list-links.ts | 11 +- 25 files changed, 146 insertions(+), 539 deletions(-) rename frontend/src/app/{s/[token]/components/shared-list-access-banner.tsx => (user)/shopping-lists/[id]/components/shopping-list-access-banner.tsx} (70%) rename frontend/src/app/{s/[token]/components/shared-list-unavailable.tsx => (user)/shopping-lists/[id]/components/shopping-list-unavailable.tsx} (61%) delete mode 100644 frontend/src/app/s/[token]/components/shared-shopping-list-client.tsx delete mode 100644 frontend/src/app/s/[token]/get-shared-list-preview.ts delete mode 100644 frontend/src/app/s/[token]/page.tsx diff --git a/frontend/src/app/(user)/shopping-lists/[id]/components/items/shopping-list-items.tsx b/frontend/src/app/(user)/shopping-lists/[id]/components/items/shopping-list-items.tsx index 485f9408..695b5b95 100644 --- a/frontend/src/app/(user)/shopping-lists/[id]/components/items/shopping-list-items.tsx +++ b/frontend/src/app/(user)/shopping-lists/[id]/components/items/shopping-list-items.tsx @@ -27,8 +27,6 @@ interface IShoppingListItemsProps { cheapestStores: Record; averagePrices: Record; storePrices: Record>; - /** Set when this list was opened through a share link, so writes carry the token. */ - shareToken?: string; } export default function ShoppingListItems({ @@ -36,19 +34,13 @@ export default function ShoppingListItems({ cheapestStores, averagePrices, storePrices, - shareToken, }: IShoppingListItemsProps) { const { canCheck, canEditItems, isOwner } = resolveShoppingListAccess( shoppingList.myAccess, ); const { handleUpdateItem, handleDeleteItem, deletingItemId } = - useShoppingListItemMutations( - shoppingList.id, - averagePrices, - storePrices, - shareToken, - ); + useShoppingListItemMutations(shoppingList.id, averagePrices, storePrices); const [isItemsOpen, setIsItemsOpen] = useState(() => getShoppingListItemsOpen(shoppingList.id), diff --git a/frontend/src/app/s/[token]/components/shared-list-access-banner.tsx b/frontend/src/app/(user)/shopping-lists/[id]/components/shopping-list-access-banner.tsx similarity index 70% rename from frontend/src/app/s/[token]/components/shared-list-access-banner.tsx rename to frontend/src/app/(user)/shopping-lists/[id]/components/shopping-list-access-banner.tsx index 11b7a163..7ac183c7 100644 --- a/frontend/src/app/s/[token]/components/shared-list-access-banner.tsx +++ b/frontend/src/app/(user)/shopping-lists/[id]/components/shopping-list-access-banner.tsx @@ -5,6 +5,11 @@ import type { LinkAccess, ListAccess } from "@/lib/api/schemas/shopping-list"; import { LINK_ACCESS_ICONS } from "@/app/(user)/shopping-lists/utils/link-access-icons"; import { SHARED_ACCESS_BANNER_ID } from "@/app/(user)/shopping-lists/utils/shopping-list-access"; +interface IShoppingListAccessBannerProps { + myAccess: ListAccess | undefined; + isSignedIn: boolean; +} + /** What this visitor may do, in the same terms the owner picked in the share modal. */ const ACCESS_TEXT: Partial> = { VIEW: "Ovaj popis možeš samo pregledavati.", @@ -17,18 +22,20 @@ const ACCESS_TEXT: Partial> = { * controls with no stated reason, which a screen reader renders as "dimmed" and nothing * else. The level is only knowable from myAccess, since linkAccess is nulled for anyone * who is not the owner. + * + *

Renders an empty element rather than null when there is nothing to say, because the + * disabled item controls point at this id with aria-describedby. The owner is the expected + * silent case, but a list DTO persisted to IndexedDB before myAccess existed replays with + * it undefined, which makes the controls disabled and the description absent at once. */ -export default function SharedListAccessBanner({ +export default function ShoppingListAccessBanner({ myAccess, isSignedIn, -}: { - myAccess: ListAccess; - isSignedIn: boolean; -}) { - const text = ACCESS_TEXT[myAccess]; - if (!text) return null; +}: IShoppingListAccessBannerProps) { + const text = myAccess ? ACCESS_TEXT[myAccess] : undefined; + if (!text) return ; - // OWNER never reaches this page, so the remaining levels all exist in LINK_ACCESS_ICONS. + // An owner never reaches the branch above, so the remaining levels all exist in the map. const Icon = LINK_ACCESS_ICONS[myAccess as LinkAccess]; // Where to go for more, rather than leaving a dead end. Anonymous callers are capped at diff --git a/frontend/src/app/(user)/shopping-lists/[id]/components/shopping-list-action-buttons.tsx b/frontend/src/app/(user)/shopping-lists/[id]/components/shopping-list-action-buttons.tsx index 4b095d43..1147c808 100644 --- a/frontend/src/app/(user)/shopping-lists/[id]/components/shopping-list-action-buttons.tsx +++ b/frontend/src/app/(user)/shopping-lists/[id]/components/shopping-list-action-buttons.tsx @@ -9,8 +9,6 @@ interface IShoppingListActionButtonsProps { showEditButton?: boolean; showDeleteButton?: boolean; showShareButton?: boolean; - /** Set when the page was reached through a share link, so share can offer that link. */ - shareToken?: string; /** Off, the row hides below `sm` and the surface has to carry the actions itself, the way the card does with its long press. */ showOnMobile?: boolean; className?: string; @@ -22,7 +20,6 @@ export default function ShoppingListActionButtons({ showEditButton = false, showDeleteButton = false, showShareButton = false, - shareToken, showOnMobile = false, className, }: IShoppingListActionButtonsProps) { @@ -36,7 +33,7 @@ export default function ShoppingListActionButtons({ handleEdit, handleShare, handleCopy, - } = useShoppingListActions(shoppingList, shareToken); + } = useShoppingListActions(shoppingList); const groupProps = { showShareButton, diff --git a/frontend/src/app/(user)/shopping-lists/[id]/components/shopping-list-detail-client.tsx b/frontend/src/app/(user)/shopping-lists/[id]/components/shopping-list-detail-client.tsx index e32c0380..1a3e6007 100644 --- a/frontend/src/app/(user)/shopping-lists/[id]/components/shopping-list-detail-client.tsx +++ b/frontend/src/app/(user)/shopping-lists/[id]/components/shopping-list-detail-client.tsx @@ -1,29 +1,36 @@ "use client"; -import { ArrowLeft } from "lucide-react"; -import { Button } from "@/components/ui/button"; -import Link from "next/link"; import BlockLoadingSpinner from "@/components/custom/common/block-loading-spinner"; import ShoppingListStoreSummary from "@/app/(user)/shopping-lists/[id]/components/stores/shopping-list-stores-list"; import ShoppingListHeader from "@/app/(user)/shopping-lists/[id]/components/shopping-list-header"; import ShoppingListItems from "@/app/(user)/shopping-lists/[id]/components/items/shopping-list-items"; import ShoppingListPriceHistory from "@/app/(user)/shopping-lists/[id]/components/shopping-list-price-history"; import ShoppingListInfoTable from "@/app/(user)/shopping-lists/[id]/components/shopping-list-info-table"; +import ShoppingListAccessBanner from "@/app/(user)/shopping-lists/[id]/components/shopping-list-access-banner"; +import ShoppingListUnavailable from "@/app/(user)/shopping-lists/[id]/components/shopping-list-unavailable"; import LastSyncedLabel from "@/components/custom/offline/last-synced-label"; +import { useUser } from "@/context/user-context"; import { useShoppingListData } from "@/app/(user)/shopping-lists/[id]/hooks/use-shopping-list-data"; interface IShoppingListDetailClientProps { listId: string; } +/** + * One page for the owner and for anyone holding the link. The list id is the shareable + * URL, so this route is reachable signed out and nothing here may assume ownership: every + * control is gated on the access the server resolved into myAccess. + */ export default function ShoppingListDetailClient({ listId, }: IShoppingListDetailClientProps) { - // Use custom hooks for data and mutations + const { isAuthenticated } = useUser(); + const { shoppingList, isLoading, error, + refetch, listUpdatedAt, cheapestStores, averagePrices, @@ -41,30 +48,29 @@ export default function ShoppingListDetailClient({ if (error || !shoppingList) { return ( -

-
-
-

Greška

-

Popis za kupnju nije pronađen ili se dogodila greška.

-
- - -
-
+ void refetch()} + isSignedIn={isAuthenticated} + /> ); } return (
- {/* Header Section */} + {/* Unconditional: it falls silent for an owner on its own, and the disabled item + controls point at its id with aria-describedby, so gating it here would leave + that IDREF dangling for exactly the people who need the explanation. */} + +
- {/* This route is behind the auth gate, so the caller is always signed in. */} - + {listUpdatedAt > 0 && ( - {/* Info Display Section */}
- {/* Shopping List Items Section */}
- {/* Price History Section */}
- {/* Store Summary Section */}
diff --git a/frontend/src/app/(user)/shopping-lists/[id]/components/shopping-list-header.tsx b/frontend/src/app/(user)/shopping-lists/[id]/components/shopping-list-header.tsx index ebd072e8..a8c8489e 100644 --- a/frontend/src/app/(user)/shopping-lists/[id]/components/shopping-list-header.tsx +++ b/frontend/src/app/(user)/shopping-lists/[id]/components/shopping-list-header.tsx @@ -21,14 +21,11 @@ interface IShoppingListHeaderProps { * page and a public one, so the dangerous value must not be the implicit one. */ isSignedIn: boolean; - /** Present when the page was reached through a share link. */ - shareToken?: string; } export default function ShoppingListHeader({ shoppingList, isSignedIn, - shareToken, }: IShoppingListHeaderProps) { // The server resolves this, so it stays right for a link recipient too. Editing and // deleting are owner-only on the backend, so a recipient must not see those controls. @@ -76,7 +73,6 @@ export default function ShoppingListHeader({ showShareButton={true} showEditButton={isOwner} showDeleteButton={isOwner} - shareToken={shareToken} showOnMobile={true} />
diff --git a/frontend/src/app/s/[token]/components/shared-list-unavailable.tsx b/frontend/src/app/(user)/shopping-lists/[id]/components/shopping-list-unavailable.tsx similarity index 61% rename from frontend/src/app/s/[token]/components/shared-list-unavailable.tsx rename to frontend/src/app/(user)/shopping-lists/[id]/components/shopping-list-unavailable.tsx index 115744bf..65d18193 100644 --- a/frontend/src/app/s/[token]/components/shared-list-unavailable.tsx +++ b/frontend/src/app/(user)/shopping-lists/[id]/components/shopping-list-unavailable.tsx @@ -7,9 +7,10 @@ import { Button } from "@/components/ui/button"; import NoResults from "@/components/custom/common/no-results"; import { parseProblem } from "@/lib/api/problem-details"; -interface ISharedListUnavailableProps { +interface IShoppingListUnavailableProps { error: Error | null; onRetry: () => void; + isSignedIn: boolean; } /** @@ -17,28 +18,34 @@ interface ISharedListUnavailableProps { * a retry: telling a shopper on flaky mobile data that the owner revoked their access * sends them off to ask for a new link they do not need. */ -export default function SharedListUnavailable({ +export default function ShoppingListUnavailable({ error, onRetry, -}: ISharedListUnavailableProps) { + isSignedIn, +}: IShoppingListUnavailableProps) { const isDeadLink = parseProblem(error)?.status === 404; if (isDeadLink) { return (
- {/* A revoked token and one that never existed read the same on purpose, so the - wording cannot be used to confirm a list is there. */} + {/* A list that stopped being shared and one that never existed read the same on + purpose, so the wording cannot be used to confirm an id is real. */} } + icon={ +
- {secondary && ( + {/* Not a truthiness check: the prop is a ReactNode, and 0 is a legitimate + price that would otherwise render as nothing at all. */} + {secondary != null && ( <> {/* Margin on both sides: a 24px icon overhangs the 20px line box it sits in, so a divider flush to the tier touches the glyph. */} diff --git a/frontend/src/components/custom/product/product-quick-actions-list.tsx b/frontend/src/components/custom/product/product-quick-actions-list.tsx index a05a655c..4948cef2 100644 --- a/frontend/src/components/custom/product/product-quick-actions-list.tsx +++ b/frontend/src/components/custom/product/product-quick-actions-list.tsx @@ -3,11 +3,13 @@ import { Eye, Image as ImageIcon, ListPlus, Share2 } from "lucide-react"; import QuickActionItem from "@/components/custom/common/quick-action-item"; +import { PRODUCT_ACTION_LABELS } from "@/constants/product-action-labels"; import EyePen from "@/components/custom/icons/eye-pen"; import ListPen from "@/components/custom/icons/list-pen"; import type { ProductResponse } from "@/lib/cijene-api/schemas"; import { watchlistService } from "@/lib/api"; -import { useIsOnPreselectedShoppingList } from "@/lib/api/shopping-lists/use-preselected-list-membership"; +import { useUser } from "@/context/user-context"; +import { useIsOnPreselectedShoppingList } from "@/hooks/use-preselected-list-membership"; import useProductModals from "@/hooks/use-product-modals"; import useProductShare from "@/hooks/use-product-share"; import { productImageSearchUrl } from "@/utils/product-links"; @@ -27,13 +29,16 @@ export default function ProductQuickActionsList({ product, onClose, }: IProductQuickActionsListProps) { + const { user } = useUser(); const { openAddToList, openWatchlist } = useProductModals(product); const share = useProductShare(product); // The same two reads the product row's buttons make, so the sheet and the buttons // behind it never disagree about whether this product is already tracked or listed. + // Both are guarded on a session: /products is public and holding a card opens this + // sheet, so an unguarded read is a 401 plus retries for every signed-out visitor. const { data: currentUserWatchlist = [] } = - watchlistService.useGetCurrentUserWatchlist(); + watchlistService.useGetCurrentUserWatchlist({ enabled: !!user }); const isInWatchlist = currentUserWatchlist.some( (watchlistItem) => watchlistItem.productApiId === product.ean, ); @@ -55,13 +60,21 @@ export default function ProductQuickActionsList({ <> swapToModal(openAddToList)} /> swapToModal(openWatchlist)} /> diff --git a/frontend/src/components/ui/select.tsx b/frontend/src/components/ui/select.tsx index 222bfcea..85f07d21 100644 --- a/frontend/src/components/ui/select.tsx +++ b/frontend/src/components/ui/select.tsx @@ -125,7 +125,11 @@ function SelectItem({ * re-renders the selected item's `ItemText`, so an icon inside it would appear a second * time in the trigger. */ - icon?: React.ElementType; + // Constrained rather than a bare ElementType: the render below hands the component a + // className and aria-hidden, and an unconstrained type promises nothing about either. + icon?: React.ElementType< + Pick, "className" | "aria-hidden"> + >; }) { return ( { - if (!ean) return null; - const drafted = getFormDraft(`add-to-list.${ean}`)?.values.shoppingListId; - - return typeof drafted === "string" ? drafted : null; - }); + // Subscribed, not snapshotted at mount. Product rows stay mounted while the add-to-list + // modal opens over them, so a drafted choice made in that modal has to reach the row + // behind it: without this the icon claims one list while the modal reopens on another, + // which is the exact contradiction this hook exists to prevent. The version counter is + // what changes, so the localStorage read still happens once per change rather than once + // per render. + useSyncExternalStore(subscribeToFormDrafts, getFormDraftsVersion, () => 0); if (!ean) return false; + const drafted = getFormDraft(`add-to-list.${ean}`)?.values.shoppingListId; + const draftedListId = typeof drafted === "string" ? drafted : null; + // The same rule useSelectedShoppingList applies, or the icon would describe a // list the modal is not going to open on: a drafted choice wins while that list // still exists, the newest list is the fallback, and a drafted "new" list has no diff --git a/frontend/src/utils/browser/storage/drafts.ts b/frontend/src/utils/browser/storage/drafts.ts index edc0f3b3..5b8430a0 100644 --- a/frontend/src/utils/browser/storage/drafts.ts +++ b/frontend/src/utils/browser/storage/drafts.ts @@ -3,6 +3,29 @@ import type { IFormDraft } from "@/typings/local-storage"; const DRAFT_TTL_MS = 24 * 60 * 60 * 1000; +// Anything reading a draft outside a form needs to know when one changes, because a draft +// written in a modal decides what a row behind it renders. A counter rather than the value +// itself: useSyncExternalStore needs a stable snapshot, and the drafts blob is rebuilt on +// every write, so returning it directly would loop. +const listeners = new Set<() => void>(); +let version = 0; + +export function subscribeToFormDrafts(onChange: () => void) { + listeners.add(onChange); + return () => { + listeners.delete(onChange); + }; +} + +export function getFormDraftsVersion() { + return version; +} + +function notify() { + version += 1; + listeners.forEach((listener) => listener()); +} + export function getFormDraft(key: string): IFormDraft | null { const draft = getAppStorage().formDrafts?.[key]; if (!draft) return null; @@ -19,6 +42,7 @@ export function setFormDraft(key: string, values: Record) { const drafts = { ...getAppStorage().formDrafts }; drafts[key] = { savedAt: Date.now(), values }; setAppStorage({ formDrafts: drafts }); + notify(); } // For a form whose fields are saved one at a time: dropping the saved field must @@ -39,4 +63,5 @@ export function removeFormDraft(key: string) { delete drafts[key]; setAppStorage({ formDrafts: drafts }); + notify(); } From 3850c973ae67a3b0d2fdcf0ee48df17003d859b5 Mon Sep 17 00:00:00 2001 From: CrazyFreak <44674613+OffCrazyFreak@users.noreply.github.com> Date: Fri, 7 Aug 2026 14:01:16 +0200 Subject: [PATCH 09/17] docs: Record sharing by list id and unique usernames Changes: - Rewrite SHARING.md sections 3 to 10 for the id-as-link model, including the accepted trade that a shared list's id is a capability and is not scrubbed - Update AUTH.md for the new optional-bearer chain and the username rule - Repoint PWA.md at the shopping-list-pages cache bucket - Correct MOBILE-NAV.md: three Radix menus still mount on touch unguarded, and the list card leaves a mouse under 640px without actions - Document per-field draft clearing and the draft subscription - Hyphenate x-axis in LANDING.md Notes: Section 3 stops being "the token, and why it is not the list id" and becomes the record of why that reasoning was reversed, so the argument survives rather than just its conclusion. MOBILE-NAV.md previously said nothing mounts a Radix menu on touch, which was the stated justification for deleting use-tap-to-open. It is false, and it is the kind of claim a reader trusts instead of checking. --- docs/AUTH.md | 31 +++---- docs/LANDING.md | 18 ++--- docs/MOBILE-NAV.md | 4 +- docs/PWA.md | 22 ++--- docs/SHARING.md | 165 +++++++++++++++++--------------------- docs/STATE-PERSISTENCE.md | 6 ++ 6 files changed, 116 insertions(+), 130 deletions(-) diff --git a/docs/AUTH.md b/docs/AUTH.md index f83a6fb7..17698b86 100644 --- a/docs/AUTH.md +++ b/docs/AUTH.md @@ -124,9 +124,10 @@ 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. ## 8. Password reset, set password, and change email @@ -219,16 +220,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 restored | In the entity + local DB | **Apply the unique index on the prod DB manually** (Hibernate `ddl-auto=update` will not add it to a populated table) | +| 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 +293,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 ...`. +- **`app_user.username` is unique again in the entity, but `ddl-auto=update` will not add the index to a populated table.** The production database needs the duplicates resolved and the index created by hand. - **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 +301,7 @@ 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.** +- **Apply the `app_user.username` unique index on the production database**, after de-duplicating existing rows. - **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 `