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 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. */