From 12dcaf25eeace192071aa5cd2039af532116a5ca Mon Sep 17 00:00:00 2001 From: Alexis Rodriguez Date: Tue, 8 Sep 2026 07:42:33 -0500 Subject: [PATCH 1/2] Run it the way a deployment runs it, and fix what that found Everything so far had been proved against the demo stack: H2, seeders, one container each. The configuration a real deployment uses - the prod profile on Postgres, Flyway, ddl-auto=validate, no seed data - had only ever been tested per service by Testcontainers. Running the whole thing on it for the first time found that **nobody could sign in at all**: every seeder is @Profile("!prod"), so the user table was empty and there was no way to make an account. The stack was healthy and completely unusable. BootstrapAdmin creates one ADMIN when the table is empty and credentials are configured. Both halves matter: configured but users present does nothing, so a restart cannot resurrect a deleted account or reset a changed password; empty but unconfigured says how to fix it in the log rather than leaving an operator with a bare 401. Nothing ships with a default password. infra/qa/prod-smoke.cjs is that run, kept: sign in, create a tenant and its records, check gear out and back, correct a person, refuse to delete a desk with gear on it, delete it once empty. 11 checks, and it needs no seed data, which is the point. Also in this pass, from the v1 list: - **Locations can be corrected and removed.** PATCH for the label, tag, building and floor; DELETE refused while anything is placed there, which asset-service answers - and it refuses to answer at all if unreachable, because "couldn't verify" must not read as "nothing is there". - **The rate limiter is no longer silent when it fails open.** A counter and two Prometheus alert rules: one for the store being unreachable, one for sustained refusals. The trade is still the right one; it is just visible. - **People and locations paginate.** Both get /paged endpoints in the shape asset-service already uses, and people-service gets /stats so the console stops fetching everyone to count them - nothing at five people, a whole tenant over the wire at five thousand. The desk map deliberately keeps the unpaged read: it groups by building and floor, and a page boundary through the middle of a floor is not a map. Verified: 11/11 prod smoke on Postgres, 57/57 API matrix, 38/38 browser, 109 web unit, all modules green. Co-Authored-By: Claude Opus 5 --- deploy/k8s/base/config.yaml | 5 + infra/RUNBOOK.md | 17 ++ infra/compose/docker-compose.yml | 6 + infra/compose/prometheus/alerts.yml | 30 ++++ infra/compose/prometheus/prometheus.yml | 3 + infra/qa/prod-smoke.cjs | 151 ++++++++++++++++++ .../ratelimit/RateLimitStoreConfig.java | 4 +- .../ratelimit/RedisRateLimitStore.java | 9 +- .../ratelimit/RedisRateLimitStoreTest.java | 5 +- .../authservice/bootstrap/BootstrapAdmin.java | 84 ++++++++++ .../src/main/resources/application.yml | 8 + .../bootstrap/BootstrapAdminTest.java | 66 ++++++++ .../locationservice/client/AssetClient.java | 50 ++++++ .../config/BearerTokenPropagator.java | 37 +++++ .../config/CorrelationIdPropagator.java | 23 +++ .../config/RestClientConfig.java | 30 ++++ .../locationservice/entity/Location.java | 10 ++ .../repository/LocationRepository.java | 6 + .../service/LocationOccupiedException.java | 9 ++ .../service/LocationService.java | 108 ++++++++++++- .../web/GlobalExceptionHandler.java | 7 + .../web/LocationController.java | 34 ++++ .../web/dto/PagedLocations.java | 25 +++ .../web/dto/UpdateLocationRequest.java | 11 ++ .../src/main/resources/application.yml | 4 + .../service/LocationServiceTest.java | 62 +++++++ .../repository/PersonRepository.java | 12 ++ .../peopleservice/service/PersonService.java | 41 +++++ .../peopleservice/web/PersonController.java | 24 +++ .../peopleservice/web/dto/PagedPeople.java | 24 +++ .../peopleservice/web/dto/PeopleStats.java | 17 ++ web/src/app/desks/page.tsx | 2 + web/src/app/people/page.tsx | 49 ++++-- web/src/components/desk-actions.tsx | 62 ++++++- web/src/lib/api.ts | 13 ++ web/src/lib/bff-allow.test.ts | 3 + web/src/lib/bff-allow.ts | 3 + web/src/lib/types.ts | 16 ++ 38 files changed, 1045 insertions(+), 25 deletions(-) create mode 100644 infra/compose/prometheus/alerts.yml create mode 100644 infra/qa/prod-smoke.cjs create mode 100644 services/auth-service/src/main/java/com/assettracker/authservice/bootstrap/BootstrapAdmin.java create mode 100644 services/auth-service/src/test/java/com/assettracker/authservice/bootstrap/BootstrapAdminTest.java create mode 100644 services/location-service/src/main/java/com/assettracker/locationservice/client/AssetClient.java create mode 100644 services/location-service/src/main/java/com/assettracker/locationservice/config/BearerTokenPropagator.java create mode 100644 services/location-service/src/main/java/com/assettracker/locationservice/config/CorrelationIdPropagator.java create mode 100644 services/location-service/src/main/java/com/assettracker/locationservice/config/RestClientConfig.java create mode 100644 services/location-service/src/main/java/com/assettracker/locationservice/service/LocationOccupiedException.java create mode 100644 services/location-service/src/main/java/com/assettracker/locationservice/web/dto/PagedLocations.java create mode 100644 services/location-service/src/main/java/com/assettracker/locationservice/web/dto/UpdateLocationRequest.java create mode 100644 services/people-service/src/main/java/com/assettracker/peopleservice/web/dto/PagedPeople.java create mode 100644 services/people-service/src/main/java/com/assettracker/peopleservice/web/dto/PeopleStats.java diff --git a/deploy/k8s/base/config.yaml b/deploy/k8s/base/config.yaml index 716b4bf..5f3e36c 100644 --- a/deploy/k8s/base/config.yaml +++ b/deploy/k8s/base/config.yaml @@ -47,5 +47,10 @@ stringData: # runs before any person has asked for anything. A cluster that seeds no demo data # can leave these out entirely - blank means the seeding call goes unauthenticated # and is refused, rather than being quietly privileged. + # The first admin of a fresh cluster. The seeders are all @Profile("!prod") and this + # overlay runs prod, so without these the user table is empty and every sign-in is + # refused. Used once, when there are no accounts; change the password after signing in. + SECURITY_BOOTSTRAP_EMAIL: "ops@example.invalid" + SECURITY_BOOTSTRAP_PASSWORD: "change-me-after-first-sign-in" SERVICE_ACCOUNT_EMAIL: "admin@platform.example" SERVICE_ACCOUNT_PASSWORD: "Passw0rd!" diff --git a/infra/RUNBOOK.md b/infra/RUNBOOK.md index 00e3ac8..025902d 100644 --- a/infra/RUNBOOK.md +++ b/infra/RUNBOOK.md @@ -52,6 +52,23 @@ since that exchange also ends in a locally signed token - and two replicas rejec each other's tokens. In the cloud overlay the key comes from the `asset-tracker-jwt` secret, which is what lets auth-service run two pods. +**Running it the way a deployment runs it.** The demo stack is H2 with seeders; the +`prod` profile is Postgres with Flyway and no seed data at all, because every seeder is +`@Profile("!prod")`. Bring that up and prove the journey with: + +```bash +SECURITY_BOOTSTRAP_EMAIL=ops@acme.example SECURITY_BOOTSTRAP_PASSWORD='Boot5trap!' \ + docker compose -f docker-compose.yml -f docker-compose.postgres.yml up -d +node ../qa/prod-smoke.cjs +``` + +Without `SECURITY_BOOTSTRAP_*` a fresh prod database has no accounts and every sign-in +is refused - auth-service says so in the log rather than leaving you with a bare 401. +The account is created once, when the user table is empty; change its password after +the first sign-in. It can act only on the tenants in `SECURITY_BOOTSTRAP_CLIENT_IDS` +(default `1`), and there is no user administration yet, so a tenant created later is +not reachable by that admin - see the gaps list. + **Messaging.** `assignment-service` publishes custody events to the RabbitMQ `asset-tracker.events` exchange; `notification-service` consumes them off a durable queue. Management UI at `http://localhost:15672` (guest / guest). A diff --git a/infra/compose/docker-compose.yml b/infra/compose/docker-compose.yml index 21e76ec..f774281 100644 --- a/infra/compose/docker-compose.yml +++ b/infra/compose/docker-compose.yml @@ -202,6 +202,11 @@ services: # (see secrets/README.md) to keep sessions across restarts, as the cloud # overlay does with the asset-tracker-jwt secret. JWT_PRIVATE_KEY_FILE: ${JWT_PRIVATE_KEY_FILE:-} + # The first account, for a stack that seeds no demo data - the prod profile, where + # every seeder is switched off. Blank here because this stack does seed its own. + SECURITY_BOOTSTRAP_EMAIL: ${SECURITY_BOOTSTRAP_EMAIL:-} + SECURITY_BOOTSTRAP_PASSWORD: ${SECURITY_BOOTSTRAP_PASSWORD:-} + SECURITY_BOOTSTRAP_CLIENT_IDS: ${SECURITY_BOOTSTRAP_CLIENT_IDS:-1} ENTRA_ISSUER_URI: ${ENTRA_ISSUER_URI:-} ENTRA_CLIENT_ID: ${ENTRA_CLIENT_ID:-} ENTRA_DEFAULT_CLIENT_IDS: ${ENTRA_DEFAULT_CLIENT_IDS:-1,2,3} @@ -313,6 +318,7 @@ services: - --storage.tsdb.retention.time=6h volumes: - ./prometheus/prometheus.yml:/etc/prometheus/prometheus.yml:ro + - ./prometheus/alerts.yml:/etc/prometheus/alerts.yml:ro grafana: image: grafana/grafana:11.4.0 diff --git a/infra/compose/prometheus/alerts.yml b/infra/compose/prometheus/alerts.yml new file mode 100644 index 0000000..cccefb2 --- /dev/null +++ b/infra/compose/prometheus/alerts.yml @@ -0,0 +1,30 @@ +# Alert rules for the demo stack. Kept next to the scrape config so what the stack +# watches is reviewable in the same place as what it collects. +groups: + - name: asset-tracker + rules: + # The brake failing open is deliberate - a Redis outage must not take sign-in + # with it - but it means credential stuffing is unthrottled while it lasts, and + # until now the only trace was a log line nobody reads. + - alert: AuthRateLimitStoreUnavailable + expr: increase(assettracker_auth_rate_limit_store_unavailable_total[5m]) > 0 + for: 1m + labels: { severity: warning } + annotations: + summary: "The auth rate limiter cannot reach its store" + description: >- + {{ $value | printf "%.0f" }} sign-in attempts in the last five minutes were + let through without being counted, because Redis was unreachable. Logins keep + working; brute-force protection does not. + + # Sustained refusals are the other half of the same story: either somebody is + # trying passwords, or a client is retrying a broken sign-in in a loop. + - alert: AuthRateLimitRejectingSteadily + expr: rate(assettracker_auth_rate_limited_total[5m]) > 0.2 + for: 10m + labels: { severity: info } + annotations: + summary: "Sign-ins are being rate limited continuously" + description: >- + More than one refusal every five seconds for ten minutes. Worth a look at + which address it is coming from before raising the budget. diff --git a/infra/compose/prometheus/prometheus.yml b/infra/compose/prometheus/prometheus.yml index 9b50602..7b157bc 100644 --- a/infra/compose/prometheus/prometheus.yml +++ b/infra/compose/prometheus/prometheus.yml @@ -2,6 +2,9 @@ global: scrape_interval: 10s evaluation_interval: 10s +rule_files: + - /etc/prometheus/alerts.yml + scrape_configs: - job_name: asset-tracker metrics_path: /actuator/prometheus diff --git a/infra/qa/prod-smoke.cjs b/infra/qa/prod-smoke.cjs new file mode 100644 index 0000000..f454175 --- /dev/null +++ b/infra/qa/prod-smoke.cjs @@ -0,0 +1,151 @@ +#!/usr/bin/env node +/** + * The core journey against a stack running the `prod` profile on PostgreSQL. + * + * The other suites all run against the demo stack, which is H2 with seeders. That leaves the + * configuration a real deployment actually uses - Flyway migrations, ddl-auto=validate, Postgres + * types, no seed data - proven only per service by Testcontainers, and never end to end. The first + * time this ran it found that nobody could sign in at all: every seeder is @Profile("!prod"), so + * the user table was empty and there was no way to make an account. + * + * Start the stack with: + * SECURITY_BOOTSTRAP_EMAIL=ops@acme.example SECURITY_BOOTSTRAP_PASSWORD='Boot5trap!' \ + * docker compose -f docker-compose.yml -f docker-compose.postgres.yml up -d + * + * It creates its own tenant and records, so it needs no seed data - which is the point. + */ +const BASE = process.env.E2E_BASE_URL ?? "http://localhost:8080"; +const EMAIL = process.env.SECURITY_BOOTSTRAP_EMAIL ?? "ops@acme.example"; +const PASSWORD = process.env.SECURITY_BOOTSTRAP_PASSWORD ?? "Boot5trap!"; + +let token = ""; +let passed = 0; +const failures = []; + +function check(name, got, want) { + const ok = got === want; + console.log(` ${ok ? "PASS" : "FAIL"} ${name.padEnd(44)} got=${got} want=${want}`); + ok ? passed++ : failures.push(name); + return ok; +} + +async function call(method, path, body) { + const res = await fetch(`${BASE}${path}`, { + method, + headers: { + ...(token ? { Authorization: `Bearer ${token}` } : {}), + ...(body ? { "Content-Type": "application/json" } : {}), + }, + ...(body ? { body: JSON.stringify(body) } : {}), + }); + const text = await res.text(); + let json = null; + try { + json = text ? JSON.parse(text) : null; + } catch { + /* a non-JSON body is itself the answer */ + } + return { status: res.status, body: json }; +} + +(async () => { + const stamp = Date.now().toString(36).slice(-5); + + console.log("SIGN IN"); + const login = await call("POST", "/api/auth/login", { email: EMAIL, password: PASSWORD }); + if (!check("the bootstrap admin can sign in", login.status, 200)) { + console.error("\nNo way in - is SECURITY_BOOTSTRAP_* set on auth-service?"); + process.exit(1); + } + token = login.body.token; + + console.log("\nA TENANT AND ITS RECORDS"); + // Reused if one is already there, because the bootstrap admin is scoped to the tenants named + // in SECURITY_BOOTSTRAP_CLIENT_IDS and nothing can widen that afterwards - there is no user + // administration yet. A second run that made a second tenant locked itself out of it: every + // call after the create came back 403, which is the system being right and the script being + // wrong. + const existing = await call("GET", "/api/clients"); + let clientId = existing.body?.[0]?.id; + if (clientId === undefined) { + const created = await call("POST", "/api/clients", { + name: `Smoke ${stamp}`, + slug: `smoke-${stamp}`, + }); + check("a client can be created", created.status, 201); + clientId = created.body?.id; + } else { + check("a tenant exists to work in", typeof clientId, "number"); + } + + const person = await call("POST", "/api/people", { + clientId, + fullName: `Smoke Person ${stamp}`, + email: `smoke.${stamp}@example.invalid`, + department: "QA", + }); + check("a person can be created", person.status, 201); + + const desk = await call("POST", "/api/locations", { + clientId, + kind: "DESK", + label: `Smoke Desk ${stamp}`, + qrTag: `SMOKE-${stamp}`, + }); + check("a desk can be created", desk.status, 201); + + const asset = await call("POST", "/api/assets", { + clientId, + assetTag: `SMOKE-A-${stamp}`, + type: "Laptop", + make: "Smoke", + model: "Test", + }); + check("an asset can be created", asset.status, 201); + + console.log("\nCUSTODY"); + const out = await call("POST", "/api/assignments", { + clientId, + assetId: asset.body?.id, + holderType: "PERSON", + holderId: person.body?.id, + }); + check("it can be checked out", out.status, 201); + + const held = await call( + "GET", + `/api/assets?clientId=${clientId}&holderType=PERSON&holderId=${person.body?.id}`, + ); + check("it shows on the person", held.body?.length ?? 0, 1); + + // an idempotent retry of the same intent must replay, not check out twice + const back = await call("POST", `/api/assignments/return?assetId=${asset.body?.id}`); + check("it can be returned", back.status, 200); + + console.log("\nCORRECTIONS"); + const renamed = await call("PATCH", `/api/people/${person.body?.id}`, { + fullName: `Smoke Person ${stamp} (renamed)`, + }); + check("a person can be corrected", renamed.status, 200); + + // A desk with gear on it must refuse to disappear, and that answer comes from asset-service - + // so this also proves one service asking another while carrying the caller's token. + await call("POST", "/api/assignments", { + clientId, + assetId: asset.body?.id, + holderType: "LOCATION", + holderId: desk.body?.id, + }); + const occupied = await call("DELETE", `/api/locations/${desk.body?.id}`); + check("a desk with gear on it is not deleted", occupied.status, 409); + await call("POST", `/api/assignments/return?assetId=${asset.body?.id}`); + + const deleted = await call("DELETE", `/api/locations/${desk.body?.id}`); + check("an empty desk can be deleted", deleted.status, 204); + + console.log(`\n${passed}/${passed + failures.length} passed`); + if (failures.length) { + console.error(`failed: ${failures.join(", ")}`); + process.exit(1); + } +})(); diff --git a/services/api-gateway/src/main/java/com/assettracker/apigateway/ratelimit/RateLimitStoreConfig.java b/services/api-gateway/src/main/java/com/assettracker/apigateway/ratelimit/RateLimitStoreConfig.java index e83cc72..2070ec1 100644 --- a/services/api-gateway/src/main/java/com/assettracker/apigateway/ratelimit/RateLimitStoreConfig.java +++ b/services/api-gateway/src/main/java/com/assettracker/apigateway/ratelimit/RateLimitStoreConfig.java @@ -1,5 +1,6 @@ package com.assettracker.apigateway.ratelimit; +import io.micrometer.core.instrument.MeterRegistry; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.beans.factory.annotation.Value; @@ -25,9 +26,10 @@ public class RateLimitStoreConfig { @ConditionalOnProperty(name = "spring.data.redis.host") public RateLimitStore redisRateLimitStore( StringRedisTemplate redis, + MeterRegistry meters, @Value("${security.rate-limit.key-prefix:asset-tracker:auth-rate:}") String prefix) { log.info("auth rate limit is cluster-wide, counted in Redis under '{}'", prefix); - return new RedisRateLimitStore(redis, prefix); + return new RedisRateLimitStore(redis, prefix, meters); } @Bean diff --git a/services/api-gateway/src/main/java/com/assettracker/apigateway/ratelimit/RedisRateLimitStore.java b/services/api-gateway/src/main/java/com/assettracker/apigateway/ratelimit/RedisRateLimitStore.java index f0c9b36..7097526 100644 --- a/services/api-gateway/src/main/java/com/assettracker/apigateway/ratelimit/RedisRateLimitStore.java +++ b/services/api-gateway/src/main/java/com/assettracker/apigateway/ratelimit/RedisRateLimitStore.java @@ -1,5 +1,7 @@ package com.assettracker.apigateway.ratelimit; +import io.micrometer.core.instrument.Counter; +import io.micrometer.core.instrument.MeterRegistry; import java.util.List; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -37,10 +39,14 @@ public class RedisRateLimitStore implements RateLimitStore { private final StringRedisTemplate redis; private final String prefix; + private final Counter unavailable; - public RedisRateLimitStore(StringRedisTemplate redis, String prefix) { + public RedisRateLimitStore(StringRedisTemplate redis, String prefix, MeterRegistry meters) { this.redis = redis; this.prefix = prefix; + // Counted, not only logged. Failing open is the right trade against an authentication + // outage, but it removes the brake - and a trade nobody can see is one nobody revisits. + this.unavailable = meters.counter("assettracker.auth.rate_limit.store_unavailable"); } @Override @@ -50,6 +56,7 @@ public boolean tryAcquire(String key, int maxPerWindow, long windowMs) { redis.execute(INCREMENT_AND_EXPIRE, List.of(prefix + key), String.valueOf(windowMs)); return hits == null || hits <= maxPerWindow; } catch (DataAccessException unreachable) { + this.unavailable.increment(); log.warn("rate limit store unavailable, allowing the request: {}", unreachable.getMessage()); return true; } diff --git a/services/api-gateway/src/test/java/com/assettracker/apigateway/ratelimit/RedisRateLimitStoreTest.java b/services/api-gateway/src/test/java/com/assettracker/apigateway/ratelimit/RedisRateLimitStoreTest.java index f316625..281fb2d 100644 --- a/services/api-gateway/src/test/java/com/assettracker/apigateway/ratelimit/RedisRateLimitStoreTest.java +++ b/services/api-gateway/src/test/java/com/assettracker/apigateway/ratelimit/RedisRateLimitStoreTest.java @@ -5,6 +5,7 @@ import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.when; +import io.micrometer.core.instrument.simple.SimpleMeterRegistry; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.Mock; @@ -21,7 +22,7 @@ class RedisRateLimitStoreTest { @Test @SuppressWarnings("unchecked") void allowsUntilTheSharedCounterPassesTheBudget() { - RedisRateLimitStore store = new RedisRateLimitStore(redis, "p:"); + RedisRateLimitStore store = new RedisRateLimitStore(redis, "p:", new SimpleMeterRegistry()); when(redis.execute(any(RedisScript.class), any(), anyString())).thenReturn(10L, 11L); // the tenth attempt across the whole cluster is still within a budget of ten @@ -36,7 +37,7 @@ void allowsUntilTheSharedCounterPassesTheBudget() { @Test @SuppressWarnings("unchecked") void anUnreachableStoreLetsTheRequestThrough() { - RedisRateLimitStore store = new RedisRateLimitStore(redis, "p:"); + RedisRateLimitStore store = new RedisRateLimitStore(redis, "p:", new SimpleMeterRegistry()); when(redis.execute(any(RedisScript.class), any(), anyString())) .thenThrow(new RedisConnectionFailureException("down")); diff --git a/services/auth-service/src/main/java/com/assettracker/authservice/bootstrap/BootstrapAdmin.java b/services/auth-service/src/main/java/com/assettracker/authservice/bootstrap/BootstrapAdmin.java new file mode 100644 index 0000000..922beff --- /dev/null +++ b/services/auth-service/src/main/java/com/assettracker/authservice/bootstrap/BootstrapAdmin.java @@ -0,0 +1,84 @@ +package com.assettracker.authservice.bootstrap; + +import com.assettracker.authservice.entity.Role; +import com.assettracker.authservice.entity.User; +import com.assettracker.authservice.repository.UserRepository; +import java.util.Arrays; +import java.util.Set; +import java.util.stream.Collectors; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.springframework.beans.factory.annotation.Value; +import org.springframework.boot.CommandLineRunner; +import org.springframework.security.crypto.password.PasswordEncoder; +import org.springframework.stereotype.Component; +import org.springframework.util.StringUtils; + +/** + * The first account in a real deployment. + * + *

Every seeder in this project is {@code @Profile("!prod")}, which is right - nobody wants + * dana.reyes@acme.example in production - but it left the prod profile with no way in at all. The + * stack came up healthy on Postgres, and every sign-in returned 401 because the users table was + * empty. That is not a bug you find by reading; it was found by running the stack on Postgres for + * the first time, which until now nothing had done. + * + *

So: when the table is empty and credentials are configured, one ADMIN is created. Both halves + * matter. Configured but users already present does nothing, so a restart cannot resurrect a + * deleted account or reset a changed password. Empty but unconfigured says loudly in the log how to + * fix it, because the alternative is an operator staring at a 401 with nothing to go on. + * + *

The password is read once, hashed, and never logged. Rotate it after the first sign-in like + * any bootstrap credential; the intended shape for later is a client-credentials or workload + * identity flow, which is tracked with the other secret-handling work. + */ +@Component +public class BootstrapAdmin implements CommandLineRunner { + + private static final Logger log = LoggerFactory.getLogger(BootstrapAdmin.class); + + private final UserRepository repository; + private final PasswordEncoder encoder; + private final String email; + private final String password; + private final String clientIds; + + public BootstrapAdmin( + UserRepository repository, + PasswordEncoder encoder, + @Value("${security.bootstrap.email:}") String email, + @Value("${security.bootstrap.password:}") String password, + @Value("${security.bootstrap.client-ids:1}") String clientIds) { + this.repository = repository; + this.encoder = encoder; + this.email = email; + this.password = password; + this.clientIds = clientIds; + } + + @Override + public void run(String... args) { + if (repository.count() > 0) { + return; + } + if (!StringUtils.hasText(email) || !StringUtils.hasText(password)) { + log.warn( + "no accounts exist and no bootstrap admin is configured - every sign-in will be refused." + + " Set SECURITY_BOOTSTRAP_EMAIL and SECURITY_BOOTSTRAP_PASSWORD, restart, then" + + " change that password."); + return; + } + + repository.save(new User(email, encoder.encode(password), Role.ADMIN, tenants())); + log.info("created the bootstrap admin {} - change its password after signing in", email); + } + + /** The tenants this first admin may act on. One is the common case: the client they create. */ + private Set tenants() { + return Arrays.stream(clientIds.split(",")) + .map(String::trim) + .filter(s -> !s.isEmpty()) + .map(Long::valueOf) + .collect(Collectors.toUnmodifiableSet()); + } +} diff --git a/services/auth-service/src/main/resources/application.yml b/services/auth-service/src/main/resources/application.yml index 4ec2704..5c094d6 100644 --- a/services/auth-service/src/main/resources/application.yml +++ b/services/auth-service/src/main/resources/application.yml @@ -30,6 +30,14 @@ server: port: ${SERVER_PORT:8081} security: + # The first account in a deployment that seeds no demo data. Only used when the + # user table is empty; see BootstrapAdmin. Blank here on purpose - a shipped + # default admin password is worse than no way in, because it is a way in for + # everybody. + bootstrap: + email: ${SECURITY_BOOTSTRAP_EMAIL:} + password: ${SECURITY_BOOTSTRAP_PASSWORD:} + client-ids: ${SECURITY_BOOTSTRAP_CLIENT_IDS:1} jwt: # RS256. The public half is served at /.well-known/jwks.json for the gateway # to fetch, so no shared secret crosses the mesh. diff --git a/services/auth-service/src/test/java/com/assettracker/authservice/bootstrap/BootstrapAdminTest.java b/services/auth-service/src/test/java/com/assettracker/authservice/bootstrap/BootstrapAdminTest.java new file mode 100644 index 0000000..a910290 --- /dev/null +++ b/services/auth-service/src/test/java/com/assettracker/authservice/bootstrap/BootstrapAdminTest.java @@ -0,0 +1,66 @@ +package com.assettracker.authservice.bootstrap; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import com.assettracker.authservice.entity.Role; +import com.assettracker.authservice.entity.User; +import com.assettracker.authservice.repository.UserRepository; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.ArgumentCaptor; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; +import org.springframework.security.crypto.password.NoOpPasswordEncoder; + +/** The account that makes a fresh production deployment reachable at all. */ +@ExtendWith(MockitoExtension.class) +class BootstrapAdminTest { + + @Mock UserRepository repository; + + @SuppressWarnings("deprecation") + private BootstrapAdmin admin(String email, String password, String clientIds) { + return new BootstrapAdmin( + repository, NoOpPasswordEncoder.getInstance(), email, password, clientIds); + } + + @Test + void createsOneAdminWhenThereAreNoUsers() { + when(repository.count()).thenReturn(0L); + + admin("ops@acme.example", "s3cret", "1,2").run(); + + ArgumentCaptor saved = ArgumentCaptor.forClass(User.class); + verify(repository).save(saved.capture()); + assertThat(saved.getValue().getEmail()).isEqualTo("ops@acme.example"); + assertThat(saved.getValue().getRole()).isEqualTo(Role.ADMIN); + assertThat(saved.getValue().getClientIds()).containsExactlyInAnyOrder(1L, 2L); + } + + /** + * The half that keeps it safe: a restart must not resurrect an account somebody deleted, or reset + * a password somebody changed. + */ + @Test + void doesNothingWhenAccountsAlreadyExist() { + when(repository.count()).thenReturn(5L); + + admin("ops@acme.example", "s3cret", "1").run(); + + verify(repository, never()).save(any()); + } + + /** No shipped default: unconfigured means no account, and a warning rather than a guess. */ + @Test + void createsNothingWhenUnconfigured() { + when(repository.count()).thenReturn(0L); + + admin("", "", "1").run(); + + verify(repository, never()).save(any()); + } +} diff --git a/services/location-service/src/main/java/com/assettracker/locationservice/client/AssetClient.java b/services/location-service/src/main/java/com/assettracker/locationservice/client/AssetClient.java new file mode 100644 index 0000000..5a43053 --- /dev/null +++ b/services/location-service/src/main/java/com/assettracker/locationservice/client/AssetClient.java @@ -0,0 +1,50 @@ +package com.assettracker.locationservice.client; + +import java.util.List; +import java.util.Map; +import org.springframework.beans.factory.annotation.Value; +import org.springframework.stereotype.Component; +import org.springframework.web.client.RestClient; +import org.springframework.web.client.RestClient.Builder; + +/** + * Read-only view of asset-service. location-service needs one thing from it: whether anything is + * currently sitting on a location, so a desk cannot be deleted out from under a monitor. + * + *

The same shape as people-service's client, and for the same reason - the service that owns the + * rule has to ask the service that owns the facts. + */ +@Component +public class AssetClient { + + private final RestClient client; + + public AssetClient( + Builder loadBalancedRestClientBuilder, @Value("${downstream.asset-service}") String baseUrl) { + this.client = loadBalancedRestClientBuilder.baseUrl(baseUrl).build(); + } + + /** + * Ids of the assets currently placed at a location. Propagates a failure to reach asset-service + * rather than returning empty - "couldn't verify" must not read as "nothing is there", or an + * outage would start letting deletions through. + */ + @SuppressWarnings("unchecked") + public List assetIdsPlacedAt(Long clientId, Long locationId) { + List> rows = + client + .get() + .uri( + uri -> + uri.path("/assets") + .queryParam("clientId", clientId) + .queryParam("holderType", "LOCATION") + .queryParam("holderId", locationId) + .build()) + .retrieve() + .body(List.class); + return rows == null + ? List.of() + : rows.stream().map(r -> ((Number) r.get("id")).longValue()).toList(); + } +} diff --git a/services/location-service/src/main/java/com/assettracker/locationservice/config/BearerTokenPropagator.java b/services/location-service/src/main/java/com/assettracker/locationservice/config/BearerTokenPropagator.java new file mode 100644 index 0000000..623c363 --- /dev/null +++ b/services/location-service/src/main/java/com/assettracker/locationservice/config/BearerTokenPropagator.java @@ -0,0 +1,37 @@ +package com.assettracker.locationservice.config; + +import java.io.IOException; +import org.springframework.http.HttpHeaders; +import org.springframework.http.HttpRequest; +import org.springframework.http.client.ClientHttpRequestExecution; +import org.springframework.http.client.ClientHttpRequestInterceptor; +import org.springframework.http.client.ClientHttpResponse; +import org.springframework.security.core.context.SecurityContextHolder; +import org.springframework.security.oauth2.server.resource.authentication.JwtAuthenticationToken; + +/** + * Carries the caller's token on to the next service. + * + *

Now that every service validates tokens rather than trusting headers, an internal call has to + * present one - and the right one to present is the token of the person who asked. The alternative, + * a service account with rights of its own, would let this service do things on its own behalf that + * no user asked for, and would flatten the audit trail to "assignment-service did it". + * + *

Relaying the user's token instead means asset-service applies the same tenant scoping and the + * same role rules it would have applied to a direct call, and an expired session stops working + * everywhere at once. + */ +public class BearerTokenPropagator implements ClientHttpRequestInterceptor { + + @Override + public ClientHttpResponse intercept( + HttpRequest request, byte[] body, ClientHttpRequestExecution execution) throws IOException { + if (SecurityContextHolder.getContext().getAuthentication() + instanceof JwtAuthenticationToken authenticated) { + request + .getHeaders() + .set(HttpHeaders.AUTHORIZATION, "Bearer " + authenticated.getToken().getTokenValue()); + } + return execution.execute(request, body); + } +} diff --git a/services/location-service/src/main/java/com/assettracker/locationservice/config/CorrelationIdPropagator.java b/services/location-service/src/main/java/com/assettracker/locationservice/config/CorrelationIdPropagator.java new file mode 100644 index 0000000..c40014f --- /dev/null +++ b/services/location-service/src/main/java/com/assettracker/locationservice/config/CorrelationIdPropagator.java @@ -0,0 +1,23 @@ +package com.assettracker.locationservice.config; + +import com.assettracker.locationservice.web.CorrelationIdFilter; +import java.io.IOException; +import org.slf4j.MDC; +import org.springframework.http.HttpRequest; +import org.springframework.http.client.ClientHttpRequestExecution; +import org.springframework.http.client.ClientHttpRequestInterceptor; +import org.springframework.http.client.ClientHttpResponse; + +/** Copies the current request's correlation id onto outbound calls so a trace spans services. */ +class CorrelationIdPropagator implements ClientHttpRequestInterceptor { + + @Override + public ClientHttpResponse intercept( + HttpRequest request, byte[] body, ClientHttpRequestExecution execution) throws IOException { + String id = MDC.get(CorrelationIdFilter.MDC_KEY); + if (id != null && !request.getHeaders().containsKey(CorrelationIdFilter.HEADER)) { + request.getHeaders().add(CorrelationIdFilter.HEADER, id); + } + return execution.execute(request, body); + } +} diff --git a/services/location-service/src/main/java/com/assettracker/locationservice/config/RestClientConfig.java b/services/location-service/src/main/java/com/assettracker/locationservice/config/RestClientConfig.java new file mode 100644 index 0000000..9f478e7 --- /dev/null +++ b/services/location-service/src/main/java/com/assettracker/locationservice/config/RestClientConfig.java @@ -0,0 +1,30 @@ +package com.assettracker.locationservice.config; + +import java.time.Duration; +import org.springframework.boot.http.client.ClientHttpRequestFactoryBuilder; +import org.springframework.boot.http.client.ClientHttpRequestFactorySettings; +import org.springframework.cloud.client.loadbalancer.LoadBalanced; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; +import org.springframework.web.client.RestClient; + +/** Builds the load-balanced {@link RestClient.Builder} used by {@code AssetClient}. */ +@Configuration +public class RestClientConfig { + + private static final Duration CONNECT_TIMEOUT = Duration.ofSeconds(2); + private static final Duration READ_TIMEOUT = Duration.ofSeconds(3); + + @Bean + @LoadBalanced + public RestClient.Builder loadBalancedRestClientBuilder() { + ClientHttpRequestFactorySettings settings = + ClientHttpRequestFactorySettings.defaults() + .withConnectTimeout(CONNECT_TIMEOUT) + .withReadTimeout(READ_TIMEOUT); + return RestClient.builder() + .requestFactory(ClientHttpRequestFactoryBuilder.detect().build(settings)) + .requestInterceptor(new CorrelationIdPropagator()) + .requestInterceptor(new BearerTokenPropagator()); + } +} diff --git a/services/location-service/src/main/java/com/assettracker/locationservice/entity/Location.java b/services/location-service/src/main/java/com/assettracker/locationservice/entity/Location.java index aa170fd..56e9500 100644 --- a/services/location-service/src/main/java/com/assettracker/locationservice/entity/Location.java +++ b/services/location-service/src/main/java/com/assettracker/locationservice/entity/Location.java @@ -72,6 +72,16 @@ public String getLabel() { return label; } + /** + * Corrects the tag on this location. + * + *

It has to stay unique across the estate - it is what a phone reads off the wall - and the + * check lives in the service, which is the only thing that can see the other rows. + */ + public void setQrTag(String qrTag) { + this.qrTag = qrTag; + } + public void setLabel(String label) { this.label = label; } diff --git a/services/location-service/src/main/java/com/assettracker/locationservice/repository/LocationRepository.java b/services/location-service/src/main/java/com/assettracker/locationservice/repository/LocationRepository.java index c9e466c..4252681 100644 --- a/services/location-service/src/main/java/com/assettracker/locationservice/repository/LocationRepository.java +++ b/services/location-service/src/main/java/com/assettracker/locationservice/repository/LocationRepository.java @@ -4,6 +4,8 @@ import com.assettracker.locationservice.entity.LocationKind; import java.util.List; import java.util.Optional; +import org.springframework.data.domain.Page; +import org.springframework.data.domain.Pageable; import org.springframework.data.jpa.repository.JpaRepository; public interface LocationRepository extends JpaRepository { @@ -12,6 +14,10 @@ public interface LocationRepository extends JpaRepository { List findByClientIdAndKind(Long clientId, LocationKind kind); + Page findByClientId(Long clientId, Pageable pageable); + + Page findByClientIdAndKind(Long clientId, LocationKind kind, Pageable pageable); + Optional findByQrTag(String qrTag); boolean existsByQrTag(String qrTag); diff --git a/services/location-service/src/main/java/com/assettracker/locationservice/service/LocationOccupiedException.java b/services/location-service/src/main/java/com/assettracker/locationservice/service/LocationOccupiedException.java new file mode 100644 index 0000000..7075b05 --- /dev/null +++ b/services/location-service/src/main/java/com/assettracker/locationservice/service/LocationOccupiedException.java @@ -0,0 +1,9 @@ +package com.assettracker.locationservice.service; + +/** Thrown when a location cannot be deleted because equipment is still placed on it. */ +public class LocationOccupiedException extends RuntimeException { + + public LocationOccupiedException(String label, int placed) { + super(label + " still has " + placed + " item" + (placed == 1 ? "" : "s") + " on it"); + } +} diff --git a/services/location-service/src/main/java/com/assettracker/locationservice/service/LocationService.java b/services/location-service/src/main/java/com/assettracker/locationservice/service/LocationService.java index 7e74b07..3efe301 100644 --- a/services/location-service/src/main/java/com/assettracker/locationservice/service/LocationService.java +++ b/services/location-service/src/main/java/com/assettracker/locationservice/service/LocationService.java @@ -1,13 +1,17 @@ package com.assettracker.locationservice.service; import com.assettracker.locationservice.audit.AuditService; +import com.assettracker.locationservice.client.AssetClient; import com.assettracker.locationservice.entity.Location; import com.assettracker.locationservice.entity.LocationKind; import com.assettracker.locationservice.repository.LocationRepository; import com.assettracker.locationservice.web.CallerContext; import com.assettracker.locationservice.web.TenantContext; import com.assettracker.locationservice.web.dto.CreateLocationRequest; +import com.assettracker.locationservice.web.dto.UpdateLocationRequest; import java.util.List; +import org.springframework.data.domain.Page; +import org.springframework.data.domain.Pageable; import org.springframework.stereotype.Service; import org.springframework.transaction.annotation.Transactional; @@ -17,10 +21,13 @@ public class LocationService { private final LocationRepository repository; private final AuditService audit; + private final AssetClient assetClient; - public LocationService(LocationRepository repository, AuditService audit) { + public LocationService( + LocationRepository repository, AuditService audit, AssetClient assetClient) { this.repository = repository; this.audit = audit; + this.assetClient = assetClient; } @Transactional @@ -45,6 +52,97 @@ public Location create(CreateLocationRequest request, String actor) { return saved; } + /** Corrects a location. Null fields are left as they are. */ + @Transactional + public Location update(Long id, UpdateLocationRequest request, String actor) { + CallerContext.requireAssetOperator(); + Location location = getById(id); + TenantContext.requireAllowed(location.getClientId()); + + applyQrTag(location, request.qrTag()); + applyLabel(location, request.label()); + applyPlacement(location, request); + + audit.record( + location.getClientId(), + actor, + "LOCATION_UPDATED", + location.getId(), + "updated " + location.getKind() + " " + location.getLabel(), + null); + return location; + } + + /** + * Deletes a location, refusing while anything is placed on it. + * + *

A hard delete rather than the retirement people get, because the thing this is for is a + * location created by mistake - and a desk carries no history of its own the way a person does. + * What it can carry is equipment, and deleting it out from under a monitor would leave that + * monitor pointing at a location nobody can look up. So asset-service is asked first, and says no + * by refusing to answer if it is unreachable. + * + *

Somebody seated here is not a blocker: a person keeps their own desk id, the desk map stops + * showing them, and nothing is lost. The console warns before it comes to this. + */ + @Transactional + public void delete(Long id, String actor) { + CallerContext.requireAssetOperator(); + Location location = getById(id); + TenantContext.requireAllowed(location.getClientId()); + + List placed = assetClient.assetIdsPlacedAt(location.getClientId(), location.getId()); + if (!placed.isEmpty()) { + throw new LocationOccupiedException(location.getLabel(), placed.size()); + } + + audit.record( + location.getClientId(), + actor, + "LOCATION_DELETED", + location.getId(), + "deleted " + + location.getKind() + + " " + + location.getLabel() + + " (" + + location.getQrTag() + + ")", + null); + repository.delete(location); + } + + /** The tag is what a phone reads off the wall, so it stays unique the way creation demands. */ + private void applyQrTag(Location location, String qrTag) { + if (qrTag == null || qrTag.equals(location.getQrTag())) { + return; + } + if (repository.existsByQrTag(qrTag)) { + throw new QrTagTakenException(qrTag); + } + location.setQrTag(qrTag); + } + + private static void applyLabel(Location location, String label) { + if (label != null && !label.isBlank()) { + location.setLabel(label.trim()); + } + } + + /** Building and floor are the two a caller may legitimately want to empty, so blank clears. */ + private static void applyPlacement(Location location, UpdateLocationRequest request) { + if (request.building() != null) { + location.setBuilding(blankToNull(request.building())); + } + if (request.floor() != null) { + location.setFloor(blankToNull(request.floor())); + } + } + + private static String blankToNull(String value) { + return value.isBlank() ? null : value.trim(); + } + @Transactional(readOnly = true) public List list(Long clientId, LocationKind kind) { return kind == null @@ -52,6 +150,14 @@ public List list(Long clientId, LocationKind kind) { : repository.findByClientIdAndKind(clientId, kind); } + /** Locations, a page at a time. The desk map still uses the unpaged list; see PagedLocations. */ + @Transactional(readOnly = true) + public Page listPage(Long clientId, LocationKind kind, Pageable pageable) { + return kind == null + ? repository.findByClientId(clientId, pageable) + : repository.findByClientIdAndKind(clientId, kind, pageable); + } + @Transactional(readOnly = true) public Location getById(Long id) { return repository diff --git a/services/location-service/src/main/java/com/assettracker/locationservice/web/GlobalExceptionHandler.java b/services/location-service/src/main/java/com/assettracker/locationservice/web/GlobalExceptionHandler.java index e816c53..c752546 100644 --- a/services/location-service/src/main/java/com/assettracker/locationservice/web/GlobalExceptionHandler.java +++ b/services/location-service/src/main/java/com/assettracker/locationservice/web/GlobalExceptionHandler.java @@ -1,6 +1,7 @@ package com.assettracker.locationservice.web; import com.assettracker.locationservice.service.LocationNotFoundException; +import com.assettracker.locationservice.service.LocationOccupiedException; import com.assettracker.locationservice.service.QrTagTakenException; import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; @@ -22,6 +23,12 @@ public ResponseEntity handleConflict(QrTagTakenException ex) { return build(HttpStatus.CONFLICT, "QR_TAG_TAKEN", ex.getMessage()); } + /** Refusing to delete a desk with gear on it is a conflict, not a bad request. */ + @ExceptionHandler(LocationOccupiedException.class) + public ResponseEntity handleOccupied(LocationOccupiedException ex) { + return build(HttpStatus.CONFLICT, "LOCATION_OCCUPIED", ex.getMessage()); + } + @ExceptionHandler(ForbiddenClientException.class) public ResponseEntity handleForbiddenClient(ForbiddenClientException ex) { return build(HttpStatus.FORBIDDEN, "CLIENT_FORBIDDEN", ex.getMessage()); diff --git a/services/location-service/src/main/java/com/assettracker/locationservice/web/LocationController.java b/services/location-service/src/main/java/com/assettracker/locationservice/web/LocationController.java index 125b75e..cf09777 100644 --- a/services/location-service/src/main/java/com/assettracker/locationservice/web/LocationController.java +++ b/services/location-service/src/main/java/com/assettracker/locationservice/web/LocationController.java @@ -4,17 +4,25 @@ import com.assettracker.locationservice.service.LocationService; import com.assettracker.locationservice.web.dto.CreateLocationRequest; import com.assettracker.locationservice.web.dto.LocationResponse; +import com.assettracker.locationservice.web.dto.PagedLocations; +import com.assettracker.locationservice.web.dto.UpdateLocationRequest; import jakarta.validation.Valid; import java.net.URI; import java.util.List; +import org.springframework.data.domain.Pageable; +import org.springframework.data.web.PageableDefault; +import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; +import org.springframework.web.bind.annotation.DeleteMapping; import org.springframework.web.bind.annotation.GetMapping; +import org.springframework.web.bind.annotation.PatchMapping; import org.springframework.web.bind.annotation.PathVariable; import org.springframework.web.bind.annotation.PostMapping; import org.springframework.web.bind.annotation.RequestBody; import org.springframework.web.bind.annotation.RequestHeader; import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RequestParam; +import org.springframework.web.bind.annotation.ResponseStatus; import org.springframework.web.bind.annotation.RestController; /** REST endpoints for locations. Lists are always scoped to a client; writes are audited. */ @@ -22,6 +30,8 @@ @RequestMapping("/locations") public class LocationController { + private static final int DEFAULT_PAGE_SIZE = 50; + private static final String ACTOR = "X-User-Id"; private final LocationService service; @@ -38,12 +48,36 @@ public ResponseEntity create( return ResponseEntity.created(URI.create("/locations/" + body.id())).body(body); } + @PatchMapping("/{id}") + public LocationResponse update( + @PathVariable Long id, + @Valid @RequestBody UpdateLocationRequest request, + @RequestHeader(value = ACTOR, defaultValue = "system") String actor) { + return LocationResponse.from(service.update(id, request, actor)); + } + + @DeleteMapping("/{id}") + @ResponseStatus(HttpStatus.NO_CONTENT) + public void delete( + @PathVariable Long id, @RequestHeader(value = ACTOR, defaultValue = "system") String actor) { + service.delete(id, actor); + } + @GetMapping public List list( @RequestParam Long clientId, @RequestParam(required = false) LocationKind kind) { return service.list(clientId, kind).stream().map(LocationResponse::from).toList(); } + /** Locations, paginated, for callers that should not be handed a whole estate. */ + @GetMapping("/paged") + public PagedLocations listPage( + @RequestParam Long clientId, + @RequestParam(required = false) LocationKind kind, + @PageableDefault(size = DEFAULT_PAGE_SIZE, sort = "label") Pageable pageable) { + return PagedLocations.from(service.listPage(clientId, kind, pageable)); + } + @GetMapping("/{id}") public LocationResponse getById(@PathVariable Long id) { return LocationResponse.from(service.getById(id)); diff --git a/services/location-service/src/main/java/com/assettracker/locationservice/web/dto/PagedLocations.java b/services/location-service/src/main/java/com/assettracker/locationservice/web/dto/PagedLocations.java new file mode 100644 index 0000000..2ab23bb --- /dev/null +++ b/services/location-service/src/main/java/com/assettracker/locationservice/web/dto/PagedLocations.java @@ -0,0 +1,25 @@ +package com.assettracker.locationservice.web.dto; + +import com.assettracker.locationservice.entity.Location; +import java.util.List; +import org.springframework.data.domain.Page; + +/** + * One page of locations, in the shape asset-service and people-service already use. + * + *

The desk map itself still reads the unpaged list on purpose: it groups by building and floor, + * and a page boundary through the middle of a floor is not a map. This exists for the flat lists + * and for any caller that should not be handed a whole estate. + */ +public record PagedLocations( + List items, long total, int page, int size, int totalPages) { + + public static PagedLocations from(Page page) { + return new PagedLocations( + page.getContent().stream().map(LocationResponse::from).toList(), + page.getTotalElements(), + page.getNumber(), + page.getSize(), + page.getTotalPages()); + } +} diff --git a/services/location-service/src/main/java/com/assettracker/locationservice/web/dto/UpdateLocationRequest.java b/services/location-service/src/main/java/com/assettracker/locationservice/web/dto/UpdateLocationRequest.java new file mode 100644 index 0000000..f18b155 --- /dev/null +++ b/services/location-service/src/main/java/com/assettracker/locationservice/web/dto/UpdateLocationRequest.java @@ -0,0 +1,11 @@ +package com.assettracker.locationservice.web.dto; + +/** + * Request body for correcting a location. + * + *

Null means "leave this alone", so fixing a label cannot blank a building. Two fields are + * deliberately absent: the client, because moving a desk between tenants would strand the gear on + * it, and the kind, because a desk that becomes a site is not a correction - it is a different + * thing, and the map groups by it. + */ +public record UpdateLocationRequest(String label, String qrTag, String building, String floor) {} diff --git a/services/location-service/src/main/resources/application.yml b/services/location-service/src/main/resources/application.yml index 6d36c70..506183c 100644 --- a/services/location-service/src/main/resources/application.yml +++ b/services/location-service/src/main/resources/application.yml @@ -44,6 +44,10 @@ eureka: instance: prefer-ip-address: true +downstream: + # Asked whether anything is placed on a location before that location can be deleted. + asset-service: ${ASSET_SERVICE_URL:lb://asset-service} + # The caller's token is validated here, not just at the gateway. Same names the # gateway uses, so one JWKS URI and one issuer are set for the whole stack. security: diff --git a/services/location-service/src/test/java/com/assettracker/locationservice/service/LocationServiceTest.java b/services/location-service/src/test/java/com/assettracker/locationservice/service/LocationServiceTest.java index cc5082f..fb309f5 100644 --- a/services/location-service/src/test/java/com/assettracker/locationservice/service/LocationServiceTest.java +++ b/services/location-service/src/test/java/com/assettracker/locationservice/service/LocationServiceTest.java @@ -3,14 +3,21 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import com.assettracker.locationservice.audit.AuditService; +import com.assettracker.locationservice.client.AssetClient; import com.assettracker.locationservice.entity.Location; import com.assettracker.locationservice.entity.LocationKind; import com.assettracker.locationservice.repository.LocationRepository; import com.assettracker.locationservice.web.CallerContext; import com.assettracker.locationservice.web.dto.CreateLocationRequest; +import com.assettracker.locationservice.web.dto.UpdateLocationRequest; +import java.util.List; import java.util.Optional; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; @@ -37,8 +44,63 @@ void signOut() { @Mock LocationRepository repository; @Mock AuditService audit; + @Mock AssetClient assetClient; + @InjectMocks LocationService service; + @Test + void updateChangesOnlyWhatWasSent() { + Location d = new Location(1L, LocationKind.DESK, "Desk 001", "ACME-D-001"); + d.setBuilding("HQ"); + when(repository.findById(4L)).thenReturn(Optional.of(d)); + + Location updated = + service.update(4L, new UpdateLocationRequest("Desk 1A", null, null, null), "tech@acme"); + + assertThat(updated.getLabel()).isEqualTo("Desk 1A"); + assertThat(updated.getQrTag()).isEqualTo("ACME-D-001"); + assertThat(updated.getBuilding()).isEqualTo("HQ"); + } + + /** The tag is what a phone reads off the wall, so it stays unique the way creation demands. */ + @Test + void updateRefusesATagSomewhereElseHas() { + Location d = new Location(1L, LocationKind.DESK, "Desk 001", "ACME-D-001"); + when(repository.findById(4L)).thenReturn(Optional.of(d)); + when(repository.existsByQrTag("ACME-D-002")).thenReturn(true); + + assertThatThrownBy( + () -> + service.update( + 4L, new UpdateLocationRequest(null, "ACME-D-002", null, null), "tech@acme")) + .isInstanceOf(QrTagTakenException.class); + } + + /** Deleting a desk out from under a monitor would leave the monitor pointing at nothing. */ + @Test + void deleteIsRefusedWhileSomethingIsOnIt() { + Location d = new Location(1L, LocationKind.DESK, "Desk 001", "ACME-D-001"); + when(repository.findById(4L)).thenReturn(Optional.of(d)); + when(assetClient.assetIdsPlacedAt(1L, null)).thenReturn(List.of(7L, 8L)); + + assertThatThrownBy(() -> service.delete(4L, "tech@acme")) + .isInstanceOf(LocationOccupiedException.class); + verify(repository, never()).delete(any()); + } + + @Test + void anEmptyLocationIsDeletedAndAudited() { + Location d = new Location(1L, LocationKind.DESK, "Desk 001", "ACME-D-001"); + when(repository.findById(4L)).thenReturn(Optional.of(d)); + when(assetClient.assetIdsPlacedAt(1L, null)).thenReturn(List.of()); + + service.delete(4L, "tech@acme"); + + verify(repository).delete(d); + verify(audit) + .record(eq(1L), eq("tech@acme"), eq("LOCATION_DELETED"), any(), anyString(), any()); + } + @Test void createRejectsATakenQrTag() { when(repository.existsByQrTag("ACME-D-001")).thenReturn(true); diff --git a/services/people-service/src/main/java/com/assettracker/peopleservice/repository/PersonRepository.java b/services/people-service/src/main/java/com/assettracker/peopleservice/repository/PersonRepository.java index 2eefb5c..629b171 100644 --- a/services/people-service/src/main/java/com/assettracker/peopleservice/repository/PersonRepository.java +++ b/services/people-service/src/main/java/com/assettracker/peopleservice/repository/PersonRepository.java @@ -4,6 +4,8 @@ import com.assettracker.peopleservice.entity.PersonStatus; import java.util.List; import java.util.Optional; +import org.springframework.data.domain.Page; +import org.springframework.data.domain.Pageable; import org.springframework.data.jpa.repository.JpaRepository; public interface PersonRepository extends JpaRepository { @@ -12,7 +14,17 @@ public interface PersonRepository extends JpaRepository { List findByClientIdAndStatus(Long clientId, PersonStatus status); + Page findByClientId(Long clientId, Pageable pageable); + + Page findByClientIdAndStatus(Long clientId, PersonStatus status, Pageable pageable); + Optional findByClientIdAndEmailIgnoreCase(Long clientId, String email); + long countByClientId(Long clientId); + + long countByClientIdAndStatus(Long clientId, PersonStatus status); + + long countByClientIdAndDeskIdIsNotNull(Long clientId); + boolean existsByClientIdAndEmailIgnoreCase(Long clientId, String email); } diff --git a/services/people-service/src/main/java/com/assettracker/peopleservice/service/PersonService.java b/services/people-service/src/main/java/com/assettracker/peopleservice/service/PersonService.java index 61614da..e81cf72 100644 --- a/services/people-service/src/main/java/com/assettracker/peopleservice/service/PersonService.java +++ b/services/people-service/src/main/java/com/assettracker/peopleservice/service/PersonService.java @@ -9,8 +9,12 @@ import com.assettracker.peopleservice.web.CallerContext; import com.assettracker.peopleservice.web.TenantContext; import com.assettracker.peopleservice.web.dto.CreatePersonRequest; +import com.assettracker.peopleservice.web.dto.PeopleStats; import com.assettracker.peopleservice.web.dto.UpdatePersonRequest; import java.util.List; +import org.springframework.data.domain.Page; +import org.springframework.data.domain.PageImpl; +import org.springframework.data.domain.Pageable; import org.springframework.stereotype.Service; import org.springframework.transaction.annotation.Transactional; @@ -104,6 +108,43 @@ public List list(Long clientId, PersonStatus status) { : repository.findByClientIdAndStatus(clientId, status); } + /** + * The directory, a page at a time. Separate from the unpaged {@code list} rather than changing + * its shape - the console's pickers legitimately want every name, and a response that is + * sometimes an array and sometimes an envelope is worse than two honest methods. + * + *

An ordinary employee still sees only themselves, so the page is theirs alone. + */ + @Transactional(readOnly = true) + public Page listPage(Long clientId, PersonStatus status, Pageable pageable) { + TenantContext.requireAllowed(clientId); + if (CallerContext.isSelfServiceUser()) { + Long self = CallerContext.personId(); + List mine = + self == null + ? List.of() + : repository.findById(self).filter(p -> p.getClientId().equals(clientId)).stream() + .toList(); + return new PageImpl<>(mine, pageable, mine.size()); + } + return status == null + ? repository.findByClientId(clientId, pageable) + : repository.findByClientIdAndStatus(clientId, status, pageable); + } + + /** Counts for the summary strip, done in the database rather than by fetching everyone. */ + @Transactional(readOnly = true) + public PeopleStats stats(Long clientId) { + TenantContext.requireAllowed(clientId); + CallerContext.requireStaff(); + return new PeopleStats( + repository.countByClientId(clientId), + repository.countByClientIdAndStatus(clientId, PersonStatus.ACTIVE), + repository.countByClientIdAndStatus(clientId, PersonStatus.OFFBOARDING), + repository.countByClientIdAndStatus(clientId, PersonStatus.DEPARTED), + repository.countByClientIdAndDeskIdIsNotNull(clientId)); + } + @Transactional(readOnly = true) public Person getById(Long id) { Person person = repository.findById(id).orElseThrow(() -> notFound(id)); diff --git a/services/people-service/src/main/java/com/assettracker/peopleservice/web/PersonController.java b/services/people-service/src/main/java/com/assettracker/peopleservice/web/PersonController.java index c6b6096..f01c61d 100644 --- a/services/people-service/src/main/java/com/assettracker/peopleservice/web/PersonController.java +++ b/services/people-service/src/main/java/com/assettracker/peopleservice/web/PersonController.java @@ -4,11 +4,15 @@ import com.assettracker.peopleservice.service.PersonService; import com.assettracker.peopleservice.web.dto.AssignDeskRequest; import com.assettracker.peopleservice.web.dto.CreatePersonRequest; +import com.assettracker.peopleservice.web.dto.PagedPeople; +import com.assettracker.peopleservice.web.dto.PeopleStats; import com.assettracker.peopleservice.web.dto.PersonResponse; import com.assettracker.peopleservice.web.dto.UpdatePersonRequest; import jakarta.validation.Valid; import java.net.URI; import java.util.List; +import org.springframework.data.domain.Pageable; +import org.springframework.data.web.PageableDefault; import org.springframework.http.ResponseEntity; import org.springframework.web.bind.annotation.GetMapping; import org.springframework.web.bind.annotation.PatchMapping; @@ -25,6 +29,8 @@ @RequestMapping("/people") public class PersonController { + private static final int DEFAULT_PAGE_SIZE = 50; + private static final String ACTOR = "X-User-Id"; private final PersonService service; @@ -47,6 +53,24 @@ public List list( return service.list(clientId, status).stream().map(PersonResponse::from).toList(); } + /** + * The directory, paginated. Sized like the catalog's, and clamped by the same property, so a + * caller cannot pull a whole tenant through this route. + */ + @GetMapping("/paged") + public PagedPeople listPage( + @RequestParam Long clientId, + @RequestParam(required = false) PersonStatus status, + @PageableDefault(size = DEFAULT_PAGE_SIZE, sort = "fullName") Pageable pageable) { + return PagedPeople.from(service.listPage(clientId, status, pageable)); + } + + /** Counts for the console's summary strip; see PeopleStats. */ + @GetMapping("/stats") + public PeopleStats stats(@RequestParam Long clientId) { + return service.stats(clientId); + } + @GetMapping("/{id}") public PersonResponse getById(@PathVariable Long id) { return PersonResponse.from(service.getById(id)); diff --git a/services/people-service/src/main/java/com/assettracker/peopleservice/web/dto/PagedPeople.java b/services/people-service/src/main/java/com/assettracker/peopleservice/web/dto/PagedPeople.java new file mode 100644 index 0000000..b7bf739 --- /dev/null +++ b/services/people-service/src/main/java/com/assettracker/peopleservice/web/dto/PagedPeople.java @@ -0,0 +1,24 @@ +package com.assettracker.peopleservice.web.dto; + +import com.assettracker.peopleservice.entity.Person; +import java.util.List; +import org.springframework.data.domain.Page; + +/** + * One page of the directory, plus what a caller needs to render pagination without a second call. + * + *

The same shape asset-service already uses, deliberately: two services answering the same + * question two different ways is a cost the console pays forever. + */ +public record PagedPeople( + List items, long total, int page, int size, int totalPages) { + + public static PagedPeople from(Page page) { + return new PagedPeople( + page.getContent().stream().map(PersonResponse::from).toList(), + page.getTotalElements(), + page.getNumber(), + page.getSize(), + page.getTotalPages()); + } +} diff --git a/services/people-service/src/main/java/com/assettracker/peopleservice/web/dto/PeopleStats.java b/services/people-service/src/main/java/com/assettracker/peopleservice/web/dto/PeopleStats.java new file mode 100644 index 0000000..2cbb75b --- /dev/null +++ b/services/people-service/src/main/java/com/assettracker/peopleservice/web/dto/PeopleStats.java @@ -0,0 +1,17 @@ +package com.assettracker.peopleservice.web.dto; + +/** + * Counts for the directory's summary strip. + * + *

Exists for the same reason asset-service's does: the page used to fetch every person and count + * them in the render, which is fine for five and a whole tenant over the wire for five thousand. + * Counting belongs in the database. + * + * @param total everyone in the tenant + * @param active still here + * @param offboarding leaving, gear not yet all back + * @param departed gone + * @param withDesk how many have a desk, which is what makes the desk map worth opening + */ +public record PeopleStats( + long total, long active, long offboarding, long departed, long withDesk) {} diff --git a/web/src/app/desks/page.tsx b/web/src/app/desks/page.tsx index 661ca45..7a843a2 100644 --- a/web/src/app/desks/page.tsx +++ b/web/src/app/desks/page.tsx @@ -283,6 +283,8 @@ function DeskCard({ inStock={inStock} canSeat={canSeat} canPlace={canPlace} + deskLabel={desk.label} + itemCount={items.length} /> ); diff --git a/web/src/app/people/page.tsx b/web/src/app/people/page.tsx index e836caa..ce61df3 100644 --- a/web/src/app/people/page.tsx +++ b/web/src/app/people/page.tsx @@ -3,30 +3,43 @@ import { redirect } from "next/navigation"; import { getSession } from "@/lib/session"; import { canOperateAssets, isSelfServiceUser } from "@/lib/roles"; import { currentClientId } from "@/lib/client"; -import { listPeople, listLocations } from "@/lib/api"; +import { listPeoplePaged, peopleStats, listLocations } from "@/lib/api"; import { PersonStatusBadge } from "@/components/ui/badge"; import { Button } from "@/components/ui/button"; import { StatStrip } from "@/components/ui/stat"; import { PageHeader, TableCard } from "@/components/ui/page-header"; +import { Pagination } from "@/components/pagination"; export const metadata = { title: "People" }; -export default async function PeoplePage() { +/** Matches the catalog, so the two lists page at the same rate. */ +const PAGE_SIZE = 50; + +export default async function PeoplePage({ + searchParams, +}: { + searchParams: Promise<{ page?: string }>; +}) { const session = await getSession(); if (!session) redirect("/welcome?next=/people"); // staff-only, like every other page that shows the whole tenant: an employee // gets their own gear and the sign-out form, and nothing that lists colleagues if (isSelfServiceUser(session.role)) redirect("/"); - const clientId = await currentClientId(); - const [people, desks] = await Promise.all([ - listPeople(clientId), + const [clientId, sp] = await Promise.all([currentClientId(), searchParams]); + const pageIndex = Math.max(0, Number(sp.page ?? "0") || 0); + + // One page of rows and counts done in the database. This page used to fetch every person + // and count them in the render, which is nothing at five people and the whole tenant over + // the wire at five thousand - the same trade the catalog made when it stopped doing it. + const [page, stats, desks] = await Promise.all([ + listPeoplePaged({ clientId, page: pageIndex, size: PAGE_SIZE }), + peopleStats(clientId), listLocations(clientId, "DESK").catch(() => []), ]); + const people = page.items; const deskLabel = new Map(desks.map((d) => [d.id, d.label])); - const n = (s: string) => people.filter((p) => p.status === s).length; - return (

p.deskId != null).length, - tone: "primary", - }, + { label: "People", value: stats.total }, + { label: "Active", value: stats.active, tone: "success" }, + { label: "Offboarding", value: stats.offboarding, tone: "warn" }, + { label: "Departed", value: stats.departed }, + { label: "With a desk", value: stats.withDesk, tone: "primary" }, ]} /> @@ -92,6 +101,14 @@ export default async function PeoplePage() { ))} + + (next > 0 ? `/people?page=${next}` : "/people")} + />
); } diff --git a/web/src/components/desk-actions.tsx b/web/src/components/desk-actions.tsx index 3691ffd..858edfe 100644 --- a/web/src/components/desk-actions.tsx +++ b/web/src/components/desk-actions.tsx @@ -20,6 +20,8 @@ import type { Asset, Person } from "@/lib/types"; export function DeskActions({ deskId, clientId, + deskLabel, + itemCount, seated, people, inStock, @@ -34,6 +36,10 @@ export function DeskActions({ inStock: Asset[]; canSeat: boolean; canPlace: boolean; + /** What this desk is called, so a correction starts from the current value. */ + deskLabel: string; + /** Whether anything is on the desk - deleting one that is in use is refused by the service. */ + itemCount: number; }) { const router = useRouter(); const [open, setOpen] = useState(false); @@ -41,18 +47,23 @@ export function DeskActions({ const [asset, setAsset] = useState(""); const [busy, setBusy] = useState(false); const [error, setError] = useState(null); + const [name, setName] = useState(deskLabel); if (!canSeat && !canPlace) { return null; } - async function send(path: string, body: unknown) { + async function send(path: string, body: unknown, method = "POST") { setBusy(true); setError(null); const res = await fetch(`/api/bff/${path}`, { - method: "POST", - headers: { "Content-Type": "application/json" }, - body: JSON.stringify(body), + method, + ...(body === undefined + ? {} + : { + headers: { "Content-Type": "application/json" }, + body: JSON.stringify(body), + }), }); setBusy(false); if (!res.ok) { @@ -84,6 +95,16 @@ export function DeskActions({ } } + async function rename() { + await send(`locations/${deskId}`, { label: name.trim() }, "PATCH"); + } + + async function remove() { + // no confirm dialog: an empty desk carries nothing, and the audit trail keeps a + // LOCATION_DELETED row with its name and tag if anyone asks what happened to it + await send(`locations/${deskId}`, undefined, "DELETE"); + } + async function place() { if (!asset) return; const ok = await send("assignments", { @@ -167,6 +188,39 @@ export function DeskActions({ )} + {canPlace && ( +
+ + setName(e.target.value)} + className="h-8 min-w-0 flex-1 rounded-md border border-border bg-background px-2 text-xs outline-none focus-visible:border-primary" + /> + + {/* + Only offered for an empty desk. The service refuses either way - it asks + asset-service what is on it first - but a button that is always refused is + the thing we just spent a change removing from the dashboard. + */} + {itemCount === 0 && ( + + )} +
+ )} + {error && (

{error} diff --git a/web/src/lib/api.ts b/web/src/lib/api.ts index da7f8e2..b146801 100644 --- a/web/src/lib/api.ts +++ b/web/src/lib/api.ts @@ -13,6 +13,8 @@ import type { EventEquipmentItem, EventRequest, PagedAssets, + PagedPeople, + PeopleStats, Person, TypeUsage, } from "@/lib/types"; @@ -163,6 +165,17 @@ export const getEventRequest = (id: string | number) => * that aggregate (dashboard, reports, type counts) or filter narrowly; this is * for the screen that renders a row per asset. */ +export const listPeoplePaged = (params: { + clientId: number; + status?: string; + page?: number; + size?: number; +}) => gateway(`/api/people/paged${qs(params)}`, { auth: true }); + +/** Counts for the directory strip, so the page stops fetching everyone to count them. */ +export const peopleStats = (clientId: number) => + gateway(`/api/people/stats${qs({ clientId })}`, { auth: true }); + export const listAssetsPaged = (params: { clientId: number; type?: string; diff --git a/web/src/lib/bff-allow.test.ts b/web/src/lib/bff-allow.test.ts index cabbebc..9f0154f 100644 --- a/web/src/lib/bff-allow.test.ts +++ b/web/src/lib/bff-allow.test.ts @@ -27,7 +27,10 @@ describe("paths the console actually uses", () => { "assets/types", "assets/types/7?clientId=1", "people", + "people/paged?clientId=1&page=0", + "people/stats?clientId=1", "people/3", + "locations/9", "people/3/offboarding", "people/3/departed", "people/3/desk", diff --git a/web/src/lib/bff-allow.ts b/web/src/lib/bff-allow.ts index 74c748b..4a6fa04 100644 --- a/web/src/lib/bff-allow.ts +++ b/web/src/lib/bff-allow.ts @@ -25,10 +25,13 @@ export const ALLOW: RegExp[] = [ /^assets\/types$/, /^assets\/types\/\d+(\?.*)?$/, /^people$/, + /^people\/(paged|stats)(\?.*)?$/, // the person record itself: read for the detail page, PATCH to correct it /^people\/\d+$/, /^people\/\d+\/(offboarding|departed|desk)$/, /^locations$/, + // one location: PATCH to correct it, DELETE to remove one added by mistake + /^locations\/\d+$/, /^clients$/, ]; diff --git a/web/src/lib/types.ts b/web/src/lib/types.ts index 247862c..74c01eb 100644 --- a/web/src/lib/types.ts +++ b/web/src/lib/types.ts @@ -142,6 +142,22 @@ export interface Assignment { note: string | null; } +export interface PagedPeople { + items: Person[]; + total: number; + page: number; + size: number; + totalPages: number; +} + +export interface PeopleStats { + total: number; + active: number; + offboarding: number; + departed: number; + withDesk: number; +} + export interface OffboardingResult { personId: number; /** Back in stock, and the assignment is closed. */ From 7dea4fb1559666fc3e2e2aae711db70887ac9585 Mon Sep 17 00:00:00 2001 From: Alexis Rodriguez Date: Tue, 8 Sep 2026 08:02:26 -0500 Subject: [PATCH 2/2] Run the browser suite on pull requests too Opening this as a pull request rather than pushing to main turned out to weaken the gate, not strengthen it. `images` - and `e2e`, which needs it - only ran on a push to main or develop, so a pull request was checked by build, web and secret-scan while a direct push additionally stood the whole stack up and drove it through a browser. That is exactly backwards, and this session proves it: the browser suite is what caught the seeder losing its identity when services began validating tokens, and the employee page disagreeing with its own subtitle. Merging on everything except that suite would have let both through. Images from a pull request are tagged by commit SHA like any other, plus a pr-N tag instead of a branch name, so they are identifiable and disposable. Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9c15f25..5e52431 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -78,7 +78,17 @@ jobs: --config=/repo/.gitleaks.toml images: - if: ${{ github.event_name == 'push' && (github.ref_name == 'main' || github.ref_name == 'develop') }} + # Pull requests build and publish too, so the e2e job below can run on them. + # + # It used not to, which made a pull request the *weaker* path: it ran build, web and + # secret-scan, while a push to main additionally stood the whole stack up and drove it. + # The browser suite is what caught the seeder losing its identity and the employee page + # disagreeing with itself - so gating merges on everything except that suite would have + # let both through. Tags are by commit SHA, so a PR's images are identifiable and + # disposable. + if: >- + ${{ github.event_name == 'pull_request' || + (github.event_name == 'push' && (github.ref_name == 'main' || github.ref_name == 'develop')) }} needs: [build, web] runs-on: ubuntu-latest permissions: @@ -129,7 +139,7 @@ jobs: cache-to: type=gha,scope=${{ matrix.service }},mode=max tags: | ghcr.io/${{ env.OWNER_LC }}/asset-tracker-${{ matrix.service }}:${{ github.sha }} - ghcr.io/${{ env.OWNER_LC }}/asset-tracker-${{ matrix.service }}:${{ github.ref_name }} + ghcr.io/${{ env.OWNER_LC }}/asset-tracker-${{ matrix.service }}:${{ github.event_name == 'pull_request' && format('pr-{0}', github.event.number) || github.ref_name }} # Stand the ten images that were just published up as a real stack and drive it # through the gateway. The suite self-skips when no stack is reachable, so this