docs(quick-261008-dts): Modul Domains mit AutoDNS-Anbindung
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
+181
@@ -0,0 +1,181 @@
|
||||
---
|
||||
phase: 261008-dts-modul-domains-autodns-anbindung-kontakte
|
||||
reviewed: 2026-10-08T00:00:00Z
|
||||
depth: quick
|
||||
files_reviewed: 37
|
||||
files_reviewed_list:
|
||||
- apps/api/prisma/migrations/20261008120000_domains_autodns/migration.sql
|
||||
- apps/api/prisma/schema.prisma
|
||||
- apps/api/src/app.module.ts
|
||||
- apps/api/src/domains/autodns-client.ts
|
||||
- apps/api/src/domains/autodns-parse.ts
|
||||
- apps/api/src/domains/domain-name.ts
|
||||
- apps/api/src/domains/domains-cache.ts
|
||||
- apps/api/src/domains/domains-directory.service.ts
|
||||
- apps/api/src/domains/domains-orders.service.ts
|
||||
- apps/api/src/domains/domains-settings.service.ts
|
||||
- apps/api/src/domains/domains.controller.ts
|
||||
- apps/api/src/domains/domains.module.ts
|
||||
- apps/api/src/domains/domains.seed.ts
|
||||
- apps/api/src/domains/domains.types.ts
|
||||
- apps/api/src/domains/dto/domains-contact.dto.ts
|
||||
- apps/api/src/domains/dto/domains-customer.dto.ts
|
||||
- apps/api/src/domains/dto/domains-order.dto.ts
|
||||
- apps/api/src/domains/dto/domains-settings.dto.ts
|
||||
- apps/web/src/app/(portal)/modules/domains/components/ContactForm.tsx
|
||||
- apps/web/src/app/(portal)/modules/domains/components/ContactsTab.tsx
|
||||
- apps/web/src/app/(portal)/modules/domains/components/CustomersTab.tsx
|
||||
- apps/web/src/app/(portal)/modules/domains/components/DomainsTab.tsx
|
||||
- apps/web/src/app/(portal)/modules/domains/components/EnvironmentBadge.tsx
|
||||
- apps/web/src/app/(portal)/modules/domains/components/OrdersTab.tsx
|
||||
- apps/web/src/app/(portal)/modules/domains/components/RegisterTab.tsx
|
||||
- apps/web/src/app/(portal)/modules/domains/components/SettingsTab.tsx
|
||||
- apps/web/src/app/(portal)/modules/domains/layout.tsx
|
||||
- apps/web/src/app/(portal)/modules/domains/page.tsx
|
||||
- apps/web/src/components/domains/group-by-customer.ts
|
||||
- apps/web/src/components/domains/order-status.ts
|
||||
- apps/web/src/lib/domains-api.ts
|
||||
- apps/web/src/lib/module-identity.ts
|
||||
- apps/web/src/lib/module-loader.ts
|
||||
- apps/web/src/lib/stores/nav-store.ts
|
||||
- apps/web/src/components/modules/module-tile.tsx
|
||||
findings:
|
||||
critical: 1
|
||||
warning: 6
|
||||
info: 4
|
||||
total: 11
|
||||
status: issues_found
|
||||
---
|
||||
|
||||
# Quick 261008-dts: Code Review Report
|
||||
|
||||
**Reviewed:** 2026-10-08
|
||||
**Depth:** quick (focus area 1, money safety, traced in full)
|
||||
**Files Reviewed:** 37
|
||||
**Status:** issues_found
|
||||
|
||||
## Summary
|
||||
|
||||
The core money-safety design holds up. The DRAFT -> SUBMITTING claim is an atomic `updateMany` that includes the environment in its `where`, and it happens before any network call. `autodnsRequest` has no retry and there is exactly one `POST /domain`. Timeouts and network errors end in UNKNOWN. The web client has no automatic retry and locks the confirm button after an error. Concurrent confirms, a second tab and a double click cannot produce two POSTs.
|
||||
|
||||
Secrets, tenant isolation, permission levels and route order are clean:
|
||||
- **Secrets:** The password never leaves the settings service. Views only carry `hasPassword`. No log line or error text contains headers or credentials.
|
||||
- **Tenant isolation:** Every Prisma `where` names `tenantId`, RLS is on for all four tables, and the cache key includes tenant, environment and config version.
|
||||
- **Permissions:** Writes are `@ModuleManage`, reads use the class-level `@UseModule`, and there is no `@Roles`.
|
||||
- **Route order:** `orders/refresh` comes before `orders/:id/*`.
|
||||
|
||||
One real hole remains in the "never guess" rule. A non-2xx answer that carries an envelope is treated as "AutoDNS rejected the order", even for 5xx (CR-01). Several smaller gaps around the discard and reconcile path make that hole easier to reach.
|
||||
|
||||
## Fix Status (2026-10-08)
|
||||
|
||||
| ID | Status | Commit | Note |
|
||||
|----|--------|--------|------|
|
||||
| CR-01 | fixed | cc1c83a | 5xx, 408, 425 (with or without envelope) now classify as `http`; only 4xx with envelope, 401/403 and 2xx+ERROR are definitive. Submit: `http`, timeouts, network, tls and 429 all end in UNKNOWN (429 origin is not proven to be AutoDNS pre-processing). Reconcile treats non-`business` failures as inconclusive. Tests: client spec table, order spec for 408/425/429/500/502/503/504 on submit, reconcile and job search. Requires human verification (logic change). |
|
||||
| WR-01 | fixed | cc1c83a | `cancelOrder` re-runs the reconcile at discard time; only an explicit not-found discards. Found -> 409 `orderFound`, inconclusive -> 409 `checkFirst`, other system -> 409 `environmentChanged`. Requires human verification. |
|
||||
| WR-02 | fixed | cc1c83a | Summary carries `version` (draft `updatedAt`); submit DTO requires it, claim `where` includes `updatedAt`, mismatch -> 409 `orderChanged` (German message shown by the register dialog). Requires human verification. |
|
||||
| WR-03 | fixed | cc1c83a | Jobs for the domain without readable date, jobs without readable object and a full result page (10) without match are inconclusive: `lastCheckedAt` stays empty. Note: the `object` filter key is still unverified against the live API. |
|
||||
| WR-04 | fixed | 268d6d5 | `@ValidateIf(v !== undefined)` instead of `@IsOptional()`; context numbers stay nullable. DTO spec added. |
|
||||
| WR-05 | fixed | 9ecf191 | `prices[0]` fallback removed; no verifiable one-year entry -> no price. |
|
||||
| WR-06 | fixed | 9eada2e, e1dd996 | `ModuleGuard` stores `request.moduleAccessLevel`; `?refresh` is ignored unless MANAGE. Refresh buttons hidden for non-managers. |
|
||||
| IN-01 | skipped | - | Out of scope (needs grace-period design for `openKey` on SUCCESS). |
|
||||
| IN-02 | skipped | - | Out of scope (needs a migration with a partial unique index). |
|
||||
| IN-03 | fixed | cc1c83a | Post-POST write wrapped: logs order id and job number, falls back to UNKNOWN (never DRAFT/FAILED), returns an UNKNOWN view instead of 500; if that fails too the row stays SUBMITTING and becomes UNKNOWN after 2 minutes. |
|
||||
| IN-04 | skipped | - | Out of scope (needs a migration). |
|
||||
|
||||
## Critical Issues
|
||||
|
||||
### CR-01: HTTP 5xx with an AutoDNS envelope is classified as definitive rejection, so the order is FAILED and the domain key is freed (double-registration path)
|
||||
|
||||
**File:** `apps/api/src/domains/autodns-client.ts:150-155`, used at `apps/api/src/domains/domains-orders.service.ts:546-556` and `:718`
|
||||
**Issue:** `failureKindForStatus` returns `'business'` for every non-2xx status other than 401, 403 and 429 whenever the body looks like an envelope. The envelope test is loose: `'messages' in parsed` is enough. `autodns-client.spec.ts:187` pins `500 + envelope -> business` as intended.
|
||||
|
||||
`outcomeOfSubmit` maps `business` to `FAILED` with `openKey: null`. A 500, 502, 503, 504 or 408 with a JSON envelope therefore marks the order as not placed. That is the case the class header says must never be guessed. A gateway or backend that times out after the job was created, but still answers with an AutoDNS-style error envelope, will:
|
||||
|
||||
1. save the order as FAILED and free `openKey`, and
|
||||
2. let the user (or a second user) create and submit a new order for the same domain, which is a second `POST /domain` and a second charge.
|
||||
|
||||
The same classification weakens `reconcile`. Line 718 reads `!domain.ok && domain.kind !== 'business'` as "AutoDNS said: domain does not exist". A 500 with an envelope is then taken as "not found" and the order can move toward discardable.
|
||||
|
||||
**Fix:** Treat only unambiguous rejections as FAILED. That means HTTP 4xx other than 408 and 425 with an envelope, plus 2xx with `status.type === 'ERROR'`. Everything with `httpStatus >= 500` becomes UNKNOWN. In `parseAutodnsEnvelope`, return `'http'` for a non-2xx status that is 408, 425 or 5xx, regardless of the envelope:
|
||||
|
||||
```ts
|
||||
function failureKindForStatus(httpStatus: number, hasEnvelope: boolean): AutodnsFailureKind {
|
||||
if (httpStatus === 401) return 'auth';
|
||||
if (httpStatus === 403) return 'forbidden';
|
||||
if (httpStatus === 429) return 'rate-limit';
|
||||
if (httpStatus >= 500 || httpStatus === 408 || httpStatus === 425) return 'http';
|
||||
return hasEnvelope ? 'business' : 'http';
|
||||
}
|
||||
```
|
||||
|
||||
In `outcomeOfSubmit`, also keep `rate-limit` out of the definitive-FAILED group unless it is known to come from AutoDNS. Update `autodns-client.spec.ts:187` (`[500, 'business']` becomes `[500, 'http']`) and add an order-service test: submit answered with `500 + envelope` must end in UNKNOWN with `openKey` kept.
|
||||
|
||||
## Warnings
|
||||
|
||||
### WR-01: "Discard" of an UNKNOWN order relies on a check of arbitrary age
|
||||
|
||||
**File:** `apps/api/src/domains/domains-orders.service.ts:754-767`, `apps/web/src/components/domains/order-status.ts:53-55`
|
||||
**Issue:** After one reconcile that found nothing, `lastCheckedAt` is set. From then on the order is discardable forever. The web client also stops polling it, because `isOpen` is false once `lastCheckedAt` is set. Registrar jobs can be delayed, so a check that was true ten minutes ago may no longer be true. A later discard frees `openKey`, and a re-order then runs a second `POST /domain` while the first can still complete.
|
||||
**Fix:** In `cancelOrder`, require a recent check. For example, reject with `checkFirst` when `lastCheckedAt` is older than 5 minutes. Better, run `reconcile` inside `cancelOrder` and discard only if it still finds nothing.
|
||||
|
||||
### WR-02: Registration is claimed with a payload the user did not confirm (lost update on a shared DRAFT)
|
||||
|
||||
**File:** `apps/api/src/domains/domains-orders.service.ts:386-393` and `:466-488`
|
||||
**Issue:** `createOrder` for an existing DRAFT overwrites `payload` (contacts, name servers) with `updateMany`. `submitOrder` reads the payload first and claims afterwards, with no version check. If manager B re-runs "Show summary" for the same domain after manager A has seen the summary, A confirms and registers with B's owner contact and name servers. The same happens if B's update lands between A's read and A's claim.
|
||||
**Fix:** Return `order.updatedAt` or a payload hash in the summary. The client sends it back on submit, and the claim becomes `where: { id, tenantId, status: 'DRAFT', environment, updatedAt: before.updatedAt }`. Alternatively refuse to overwrite a DRAFT that another user created.
|
||||
|
||||
### WR-03: Job matching in `reconcile` can silently miss an existing job and then allow discard
|
||||
|
||||
**File:** `apps/api/src/domains/domains-orders.service.ts:727-742`, `apps/api/src/domains/autodns-parse.ts:287-306`
|
||||
**Issue:** Candidates need `j.object === row.domainName && j.created && created >= since`. If AutoDNS returns a matching job with no readable `created` (the parser also falls back to `started` and `added`), or with the object in another form, the job is dropped. The result is "nothing found", `lastCheckedAt` is set, and the order becomes discardable although a job exists. The `filters` key `object` is also unverified against the live API.
|
||||
**Fix:** Fail towards caution. If a job for the domain exists but cannot be dated, treat it as a match, or leave `lastCheckedAt` unset and do not make the order discardable.
|
||||
|
||||
### WR-04: `@IsOptional()` lets `null` through, which causes 500 instead of 400
|
||||
|
||||
**File:** `apps/api/src/domains/dto/domains-settings.dto.ts:25-69`, `apps/api/src/domains/domains-settings.service.ts:194-203`
|
||||
**Issue:** `@IsOptional()` skips validation for `null` as well as `undefined`. Sending `{"demoUser": null}`, `{"defaultNameServers": null}` or `{"environment": null}` passes validation. The service only checks `!== undefined` and then calls `.trim()` or `.map()` on `null`, which is a TypeError, or writes `null` into a non-null column. This needs a manager account, but it is a bug and not a validation response.
|
||||
**Fix:** Use `@ValidateIf((_o, v) => v !== undefined)` in place of `@IsOptional()` for these fields. Alternatively check `!= null` in the service.
|
||||
|
||||
### WR-05: Price shown for one year can actually be another period's price
|
||||
|
||||
**File:** `apps/api/src/domains/autodns-parse.ts:195-196`
|
||||
**Issue:** `price = parsePriceEntry(oneYear) ?? parsePriceEntry(prices[0])`. If no entry matches one year, the first entry is used and then displayed as the price for 1 year in the confirmation of a binding order. The fallback could be a multi-year or transfer price.
|
||||
**Fix:** Drop the `prices[0]` fallback and return `null`. The UI already says "price unknown" for `null`.
|
||||
|
||||
### WR-06: Any USE user can force full AutoDNS re-reads with `?refresh=1`
|
||||
|
||||
**File:** `apps/api/src/domains/domains.controller.ts:105-129`, `apps/api/src/domains/domains-directory.service.ts:160-197`
|
||||
**Issue:** `GET contacts?refresh=1` and `GET domains?refresh=1` skip the cache for read-only users. Each call is up to 20 sequential POSTs on the shared 3 requests/second limiter, in addition to any refresh cycle. A read-only user can starve order submission and reconcile, and since the limiter is shared per process, that affects the whole instance.
|
||||
**Fix:** Honour `refresh` only for managers. Add `@ModuleManage` on a separate refresh route, or ignore the flag without MANAGE. A minimum interval per tenant (for example 10 seconds) also helps.
|
||||
|
||||
## Info
|
||||
|
||||
### IN-01: SUCCESS keeps `openKey` for good
|
||||
|
||||
**File:** `apps/api/prisma/migrations/20261008120000_domains_autodns/migration.sql` (header comment), `apps/api/src/domains/domains-orders.service.ts:362`
|
||||
**Issue:** A registered domain that is later deleted or expires can never be ordered again through Tessera in that environment. The user sees `orderOpen` for good.
|
||||
**Fix:** Clear `openKey` on SUCCESS after a grace period (for example 24 hours), or let `createOrder` ignore SUCCESS rows older than that.
|
||||
|
||||
### IN-02: "Own company" is not enforced in the database
|
||||
|
||||
**File:** `apps/api/src/domains/domains-directory.service.ts:286-311`
|
||||
**Issue:** Two concurrent `createCustomer` calls with `isOwnCompany: true` can both succeed, leaving two own companies. The register tab then picks the first one.
|
||||
**Fix:** Add a partial unique index: `CREATE UNIQUE INDEX ... ON "DomainsCustomer"("tenantId") WHERE "isOwnCompany"`.
|
||||
|
||||
### IN-03: Failure of the final status write is not handled
|
||||
|
||||
**File:** `apps/api/src/domains/domains-orders.service.ts:522-533`
|
||||
**Issue:** If the database write after `POST /domain` throws, the user gets a 500 although the order may have been placed. The state is still safe (SUBMITTING becomes UNKNOWN after 2 minutes). The error is neither logged with the order id nor turned into a clear UNKNOWN answer.
|
||||
**Fix:** Wrap the write, log the order id, and return an UNKNOWN view or a dedicated error code. The web client already locks the button.
|
||||
|
||||
### IN-04: Redundant index
|
||||
|
||||
**File:** `apps/api/prisma/migrations/20261008120000_domains_autodns/migration.sql` (`DomainsConfig_tenantId_idx`)
|
||||
**Issue:** `DomainsConfig_tenantId_key` already covers `tenantId`, so the extra index is redundant.
|
||||
**Fix:** Remove `@@index([tenantId])` on `DomainsConfig` in a later migration.
|
||||
|
||||
---
|
||||
|
||||
_Reviewed: 2026-10-08_
|
||||
_Reviewer: Claude (gsd-code-reviewer)_
|
||||
_Depth: quick (money-safety path traced in full)_
|
||||
Reference in New Issue
Block a user