fix: resolve UK and Berlin Group consent access against the PSU that granted it

.github/workflows/test_worktree_build.yml

@@ -18,23 +18,31 @@ jobs:
           ref: ${{ github.event.inputs.branch }}
           fetch-depth: 0
 
-      - name: Set up JDK 11
+      # Same JDK as build_pull_request.yml — the build compiles with -release 25,
+      # so the JDK 11 this job used to request could never get past compilation.
+      - name: Set up JDK
         uses: actions/setup-java@v4
         with:
-          java-version: "11"
-          distribution: "adopt"
+          java-version: "25"
+          distribution: "temurin"
           cache: maven
 
+      # On a branch, not detached: the point of this job is that the stamp names
+      # THIS working copy, and a detached worktree has no branch to compare against.
       - name: Create git worktree
-        run: git worktree add --detach ../obp-api-worktree HEAD
+        run: git worktree add -b worktree-stamp-probe ../obp-api-worktree HEAD
 
       - name: Build from worktree
         working-directory: ../obp-api-worktree
         run: |
           set -o pipefail
           MAVEN_OPTS="-Xmx3G -Xss2m -XX:MaxMetaspaceSize=1G" mvn clean package -DskipTests 2>&1 | tee "$GITHUB_WORKSPACE/worktree-build.log"
 
-      - name: Verify git.properties was generated
+      # A worktree build used to inherit the MAIN checkout's branch and commit
+      # (git-commit-id-maven-plugin redirected the worktree gitdir to the main .git),
+      # so "the fields are non-empty" passed while the values were wrong. Compare
+      # them against the worktree's own HEAD instead.
+      - name: Verify the stamp describes the worktree
         run: |
           PROPS_FILE="../obp-api-worktree/obp-api/target/classes/git.properties"
           if [ ! -f "$PROPS_FILE" ]; then
@@ -44,20 +52,85 @@ jobs:
           echo "Contents of git.properties:"
           cat "$PROPS_FILE"
 
-          check_field() {
-            local field=$1
-            local value
-            value=$(grep "^${field}=" "$PROPS_FILE" | cut -d'=' -f2-)
-            if [ -z "$value" ]; then
-              echo "FAIL: $field is empty or missing"
+          read_field() {
+            grep "^${1}=" "$PROPS_FILE" | cut -d'=' -f2- | sed 's/\\//g'
+          }
+
+          expect_field() {
+            local field=$1 expected=$2 actual
+            actual=$(read_field "$field")
+            if [ "$actual" != "$expected" ]; then
+              echo "FAIL: $field is '$actual', expected '$expected'"
               exit 1
             fi
-            echo "OK: $field=$value"
+            echo "OK: $field=$actual"
+          }
+
+          expect_field "git.commit.id" "$(git -C ../obp-api-worktree rev-parse HEAD)"
+          expect_field "git.branch" "worktree-stamp-probe"
+
+          BUILD_TIME=$(read_field "git.build.time")
+          if [ -z "$BUILD_TIME" ]; then
+            echo "FAIL: git.build.time is empty"
+            exit 1
+          fi
+          echo "OK: git.build.time=$BUILD_TIME"
+          echo "FIRST_COMMIT=$(read_field git.commit.id)" >> "$GITHUB_ENV"
+          echo "FIRST_BUILD_TIME=$BUILD_TIME" >> "$GITHUB_ENV"
+
+      # The other half of the bug: the stamp was written once and then reused, so an
+      # incremental rebuild at a new commit shipped the previous commit's values.
+      - name: Rebuild at a new commit without clean and verify the stamp moved
+        working-directory: ../obp-api-worktree
+        run: |
+          set -o pipefail
+          sleep 1
+          git -c user.name=ci -c [email protected] commit --allow-empty -m "stamp probe"
+          MAVEN_OPTS="-Xmx3G -Xss2m -XX:MaxMetaspaceSize=1G" mvn package -DskipTests 2>&1 | tee -a "$GITHUB_WORKSPACE/worktree-build.log"
+
+          PROPS_FILE="obp-api/target/classes/git.properties"
+          read_field() {
+            grep "^${1}=" "$PROPS_FILE" | cut -d'=' -f2- | sed 's/\\//g'
           }
 
-          check_field "git.commit.id"
-          check_field "git.branch"
-          check_field "git.build.time"
+          NEW_COMMIT=$(read_field git.commit.id)
+          NEW_BUILD_TIME=$(read_field git.build.time)
+
+          if [ "$NEW_COMMIT" != "$(git rev-parse HEAD)" ]; then
+            echo "FAIL: after the rebuild git.commit.id is '$NEW_COMMIT', expected '$(git rev-parse HEAD)'"
+            exit 1
+          fi
+          if [ "$NEW_COMMIT" = "$FIRST_COMMIT" ]; then
+            echo "FAIL: git.commit.id did not change across commits (stale stamp)"
+            exit 1
+          fi
+          if [ "$NEW_BUILD_TIME" = "$FIRST_BUILD_TIME" ]; then
+            echo "FAIL: git.build.time did not change across builds (stale stamp)"
+            exit 1
+          fi
+          echo "OK: stamp tracks the rebuild — $NEW_COMMIT at $NEW_BUILD_TIME"
+
+          # The stamp belongs to obp-api alone; a second copy elsewhere on the
+          # classpath makes which one /status reports a coin flip.
+          FOUND=$(find . -name git.properties -not -path '*/target/lib/*' | sort)
+          if [ "$FOUND" != "./obp-api/target/classes/git.properties" ]; then
+            echo "FAIL: expected exactly one git.properties, found:"
+            echo "$FOUND"
+            exit 1
+          fi
+          echo "OK: single git.properties on the classpath"
+
+      - name: Verify the jar ships the stamp
+        working-directory: ../obp-api-worktree
+        run: |
+          unzip -p obp-api/target/obp-api.jar git.properties > /tmp/jar-git.properties
+          cat /tmp/jar-git.properties
+          JAR_COMMIT=$(grep '^git.commit.id=' /tmp/jar-git.properties | cut -d'=' -f2- | sed 's/\\//g')
+          if [ "$JAR_COMMIT" != "$(git rev-parse HEAD)" ]; then
+            echo "FAIL: jar stamp is '$JAR_COMMIT', expected '$(git rev-parse HEAD)'"
+            exit 1
+          fi
+          echo "OK: obp-api.jar carries the current commit"
 
       - name: Upload build log
         if: always()

CLAUDE.md

@@ -3,6 +3,7 @@
 ## Working Style
 - Never blame pre-existing issues or other commits. No excuses, no finger-pointing — diagnose and resolve.
 - Never add `Co-Authored-By` trailers to commit messages.
+- Commit messages, code comments, and PR titles/descriptions: no AI/tool names (Claude, GPT, Copilot, etc.), no AI-typical filler phrasing ("Certainly!", "I'll help you with..."), no emoji, no "AI-generated"/"LLM" labels. Use plain Conventional Commits style (`fix:`, `feat:`, `refactor:`, ...) and set commit author/committer to the actual person directing the work.
 - **Goal is full http4s migration** — eliminate Lift Web and all deprecated libraries entirely. Treat Lift code as temporary scaffolding to be removed, not maintained. When fixing bugs or adding features, always prefer the http4s path.
 - **Versioning is tech-agnostic** — API version numbers reflect API signature changes (new/changed fields, new behaviour), never the underlying framework. A framework migration (Lift → http4s) happens in-place at the existing version; it does not justify a version bump.
 - **`APIMethodsXYZ.scala` (Lift) files are the source of truth for migration.** The commented-out Lift ResourceDocs and endpoints inside each `APIMethodsXYZ.scala` are the canonical reference for what the http4s version should match: URL templates, verb casing, summaries, descriptions, example bodies, error lists, tags. **Do NOT edit these files to make the parity audit pass.** The audit compares http4s against the Lift source-of-truth — when it flags a diff, the fix is to either (a) update http4s to match Lift, or (b) document the difference at the http4s site as a known intentional drift (e.g. a placeholder rename for `ResourceDocMatcher` middleware, or an upstream-driven case-class shape change). Rewriting the Lift comments to match http4s runs the comparison backwards and destroys the historical record. See `scripts/check_lift_http4s_resource_doc_parity.py` for the audit, and `scripts/rehydrate_resource_docs.py` / `scripts/restore_resource_doc_bodies.py` for the canonical Lift → http4s restoration tools.
@@ -265,6 +266,8 @@ Symptoms in tests: a v4-specific assertion fails (e.g. an entitlement should-be-
 
 **`isStatisticallyTooPermissive` is sample-pool-dependent**: a fresh local test DB with a single user trips the ABAC-permissiveness check and causes spurious rejections. Seed enough users in any test exercising ABAC rules — it's a test-data issue, not a regression.
 
+**The build stamp comes from a script, not a Maven plugin**: `git.properties` (what `/status` and the root endpoint's `git_commit` report) is written by `scripts/write_git_properties.sh`, invoked from `obp-api/pom.xml`'s `maven-antrun-plugin` execution `generate-git-properties` at `generate-resources`, straight into `target/classes`. It used to be `git-commit-id-maven-plugin`, which was wrong in two ways: its bundled JGit 6.7 has no `commondir` support, so `GitDirLocator.resolveWorktree()` redirects a linked worktree's gitdir to the *main* checkout's `.git` — every build run from `.claude/worktrees/*` stamped the main checkout's branch and commit — and its `PropertiesFileGenerator` skips rewriting when only `git.build.time` differs, freezing the timestamp. Add stamp fields by editing the script (keep the `git.*` key names; `StatusPage.scala` and `APIUtil.gitCommit` read them by name), and don't reintroduce a per-module generator: exactly one `git.properties` may be on the runtime classpath, otherwise which one is reported is incidental. `.github/workflows/test_worktree_build.yml` guards both failure modes.
+
 ## CI (shard map + run tips)
 
 Perf note: integration tests are DB/HTTP-bound (~0.4 s/test) on both frameworks; the http4s win is the **pure-unit tier** (no running server, ~0.008 s/test). `ResourceDocsTest`/`SwaggerDocsTest` are the slowest per-test cost — they serialize the whole API surface, so cost grows with endpoint count. `Http4sResourceDocs` already caches the serialized output (`Caching.{getDynamic,getStatic,getAll}ResourceDocCache` + `getStaticSwaggerDocCache`, keyed via `APIUtil.createResourceDocCacheKey`), so repeat requests for the same version/params skip re-serialization.

docs/operations/CONSENT_STALE_ACCESS.md

@@ -0,0 +1,177 @@
+# Runbook: "could not revoke ... which consent ... no longer declares"
+
+## The log line
+
+```
+WARN code.api.util.Consent$ -- grantAccessToViews: could not revoke owner on gh.29.uk/1
+from user e326edaf-9783-4fe8-8d87-4a65115defdf, which consent 84d5a583-cad9-4f5c-989b-b874d13b877a
+no longer declares. The access is still held: Failure(access cannot be revoked)
+```
+
+Emitted from [`ConsentUtil.scala:455`](../../obp-api/src/main/scala/code/api/util/ConsentUtil.scala#L455).
+
+## What it means
+
+Every time a consent is used, `grantAccessToViews`
+([`ConsentUtil.scala:419`](../../obp-api/src/main/scala/code/api/util/ConsentUtil.scala#L419))
+reconciles the consent's shadow user against the views the consent's JWT declares: it grants what is
+missing and revokes what is no longer declared.
+
+This line means the revoke half failed. **The access named in the message is still in place**, and
+the request was served anyway.
+
+**It does not self-heal.** Every subsequent use of the consent will retry the same revoke, fail the
+same way, and log the same line. The row stays until someone removes it.
+
+## Why the request still succeeds
+
+This is deliberate, not an oversight. Failing the request would make the consent unusable
+altogether — including every view it still legitimately holds — and leave no way back except
+editing rows by hand.
+
+The two failure directions are not symmetric:
+
+| | effect | who notices |
+|---|---|---|
+| a **grant** fails | the consent is denied access it asked for | the caller, immediately (the request fails) |
+| a **revoke** fails | the consent keeps access it gave up | nobody — until this log line existed |
+
+So the request is served and the discrepancy is recorded. The cost is real: **the un-revoked access
+keeps serving data the consent does not cover.** In the reproduction for this behaviour, a consent
+naming one account returned a *different* account in `/my/accounts`, purely on the strength of the
+stale row. Treat the WARN as live over-exposure, not as a cosmetic inconsistency.
+
+## Why the revoke was refused
+
+`revokeAccess` ([`MapperViews.scala:270`](../../obp-api/src/main/scala/code/views/MapperViews.scala#L270))
+delegates to `canRevokeOwnerAccess`
+([`MapperViews.scala:405`](../../obp-api/src/main/scala/code/views/MapperViews.scala#L405)), which
+refuses in exactly two cases, and **only for the `owner` view**:
+
+1. the user is an account holder on that account (`MapperAccountHolders`), or
+2. that `AccountAccess` row is the **only** `owner` row on the account —
+   `findAllByBankIdAccountIdViewId(...).length > 1` is false
+
+Any other view id returns `true` unconditionally, so a WARN naming a view other than `owner` means
+something outside these two rules failed — read the `Failure(...)` text at the end of the line
+rather than following this runbook.
+
+### Case 2 is a signal about the account, not just the consent
+
+A shadow user is never an account holder, so in practice you are looking at case 2. And case 2 says
+something stronger than "this consent is stuck":
+
+> **Nobody else holds `owner` on that account — not even the PSU.**
+
+A healthy account has the PSU's own `owner` row, which makes the count 2 and lets the revoke
+succeed. If a consent's shadow user is the *sole* `owner` holder, the account has lost its real
+owner access. **Investigate that before deleting anything**, or you will clear the symptom and leave
+an account nobody owns.
+
+## Diagnosis
+
+Substitute the consent id, user id, bank and account from the log line.
+
+```sql
+-- 1. The offending row. Confirm it exists and is the one named.
+SELECT aa.id, aa.bank_id, aa.account_id, aa.view_id, aa.consumer_id, aa.createdat
+FROM accountaccess aa
+JOIN resourceuser ru ON ru.id = aa.user_fk
+WHERE ru.userid_ = '<user id from the log>'
+  AND aa.bank_id = '<bank>' AND aa.account_id = '<account>' AND aa.view_id = 'owner';
+
+-- 2. Who else holds owner on this account? An empty result besides row 1 is case 2,
+--    and is the finding that matters.
+SELECT aa.id, ru.userid_, ru.createdbyconsentid, aa.consumer_id
+FROM accountaccess aa
+JOIN resourceuser ru ON ru.id = aa.user_fk
+WHERE aa.bank_id = '<bank>' AND aa.account_id = '<account>' AND aa.view_id = 'owner';
+
+-- 3. Is this account holder-less too? (the deeper problem, if row 2 came back thin)
+SELECT user_c FROM mapperaccountholders
+WHERE accountbankpermalink = '<bank>' AND accountpermalink = '<account>';
+
+-- 4. State of the consent itself. A REVOKED/EXPIRED consent whose row survived is a
+--    different fault -- see "If the consent is already gone" below.
+SELECT mconsentid, mstatus, mvaliduntil, mlastactiondate
+FROM mappedconsent WHERE mconsentid = '<consent id from the log>';
+
+-- 5. Confirm the user really is this consent's shadow user and nothing else.
+--    createdbyconsentid must equal the consent in the log; a shadow user is 1:1 with its consent.
+SELECT id, userid_, provider_, providerid, createdbyconsentid
+FROM resourceuser WHERE userid_ = '<user id from the log>';
+```
+
+## Resolution
+
+**Preferred — have the PSU re-authorise.** A fresh consent creates a fresh shadow user, so the stale
+row is orphaned rather than reused. This does not remove the row; it stops it being reachable
+through a live consent. Follow with the cleanup below.
+
+**If query 3 showed the account has no holder / no other owner:** fix that first. Restoring the
+PSU's own `owner` access makes the count exceed 1, at which point the *next use of the consent
+revokes the stale row on its own* and the WARN stops without any manual deletion. This is the only
+resolution that lets the code finish its own job — prefer it whenever the account is genuinely
+missing its owner.
+
+**Manual removal**, when the two above do not apply. Take a backup first; there is no undo.
+
+```sql
+-- Verify exactly one row matches BEFORE deleting.
+SELECT count(*) FROM accountaccess aa JOIN resourceuser ru ON ru.id = aa.user_fk
+WHERE ru.userid_ = '<user id>' AND aa.bank_id = '<bank>'
+  AND aa.account_id = '<account>' AND aa.view_id = 'owner';
+
+DELETE FROM accountaccess
+WHERE id IN (
+  SELECT aa.id FROM accountaccess aa JOIN resourceuser ru ON ru.id = aa.user_fk
+  WHERE ru.userid_ = '<user id>' AND aa.bank_id = '<bank>'
+    AND aa.account_id = '<account>' AND aa.view_id = 'owner'
+);
+```
+
+Delete by `id` from a verified `SELECT`. Do not delete by `user_fk` alone: a shadow user legitimately
+holds the rows for every account the consent *does* name, and those are the consent's actual grants.
+
+### If the consent is already revoked
+
+There is a second, more serious line, from the sweep that runs when a consent is revoked:
+
+```
+WARN code.api.util.Consent$ -- revokeConsentAccountAccess: could not revoke owner on gh.29.uk/1
+for revoked consent 84d5a583-…. The access outlives the consent: Failure(access cannot be revoked)
+```
+
+`revokeConsentAccountAccess`
+([`ConsentUtil.scala:920`](../../obp-api/src/main/scala/code/api/util/ConsentUtil.scala#L920)) goes
+through `revokeAccessToViewForUserAndConsumer`, which applies the same `canRevokeOwnerAccess` rule,
+so it can be refused for exactly the reasons above.
+
+**Treat this as higher priority than the `grantAccessToViews` line.** In that case a live consent is
+over-serving, and revoking the consent would still clean up. Here the consent is *already gone* and
+the access it created has outlived it — nothing in the system will come back for that row. Manual
+removal is the only resolution; the "have the PSU re-authorise" option does not apply.
+
+> **Note on the neighbouring info line.** `revokeConsentAccountAccess: dropped N account access rows`
+> counts successful revokes. Before the fix that added the WARN above, it counted *attempts*, so on
+> an affected server it reported rows as dropped that were still present. If you are triaging on a
+> build that predates it, do not take that count as evidence the rows are gone — check the table.
+
+## Verification
+
+```sql
+-- The row is gone.
+SELECT count(*) FROM accountaccess aa JOIN resourceuser ru ON ru.id = aa.user_fk
+WHERE ru.userid_ = '<user id>' AND aa.bank_id = '<bank>' AND aa.account_id = '<account>';
+```
+
+Then use the consent once and confirm no new WARN appears with that consent id, and that the
+account named in the original log line is **absent** from the consent's account listing. The second
+check is the one that matters: it is the over-exposure closing, not merely the log going quiet.
+
+## Monitoring
+
+Alert on the string `could not revoke` from `code.api.util.Consent$`. Every occurrence is a
+consent serving data it does not declare, so this warrants a ticket rather than a dashboard counter.
+Because each use of a stuck consent re-logs it, alert on *distinct* consent ids rather than raw line
+count — one stuck consent under load produces a large number of identical lines.