Migration#2797
Merged
Merged
Conversation
POST /my/logins/direct (no version prefix) now served by a new code.api.DirectLoginRoutes object wired into Http4sApp.baseServices just before the Lift bridge. The route reuses DirectLogin.validatorFutureWithParams / createTokenCommonPart so the existing /obp/v6.0.0/my/logins/direct handler (Http4s600) and the bare path share the same auth logic. LiftRules.statelessDispatch.append(DirectLogin) is removed from Boot.scala; the allow_direct_login prop gate moved into DirectLoginRoutes (returns HttpRoutes.empty when false). Verified by DirectLoginTest (23 scenarios), DirectLoginV600Test (4), v2_0_0.CreateUserTest (3) — 30/30 pass.
Two Lift RestHelper singletons replaced with native http4s routes:
- GET /alive → code.api.AliveCheckRoutes
Plain Ok("true") with JSON content-type, mirrors the prior
Extraction.decompose(true) output.
- POST /obp_transactions_saver/api/transactions → code.management.ImporterAPIRoutes
Reads `secret` from URL query params, parses body as JSON, sends
the parsed transactions to TransactionInserter LiftActor inside
IO.blocking. Reuses ImporterAPI.whenAddedJson / case classes
verbatim so JSON shape and status codes (400 / 401 / 200 / 500)
match the Lift handler exactly.
Both LiftRules.statelessDispatch.append(...) calls are removed from
Boot.scala; the now-empty enableAPIs def is dropped along with the
code.management.ImporterAPI import.
ImporterTest (8 scenarios) passes; no test exists for /alive.
Scenario "Multiple challenges with maker-checker: different users answer their own challenges" fails ~40% of local runs. Diagnosed root cause: TTL-based proxy-connection propagation (RequestScopeConnection.currentProxy as TransmittableThreadLocal) sometimes fails to reach the read Future's worker thread, so the write-then-read sequence inside one request transaction sees an empty read because RequestAwareConnectionManager falls back to a fresh pool connection without visibility into the proxy's uncommitted writes. Documenting in CLAUDE.md "Other TODOs" with the diagnosis, mechanism, and two possible fix directions for whoever picks it up.
Replace the 10 ResourceDocs140..600 per-version Lift singletons with a
single native http4s service (code.api.util.http4s.Http4sResourceDocs)
wired into Http4sApp.baseServices ahead of the per-version services.
The version-prefix segment (e.g. /obp/v6.0.0/) was always cosmetic for
resource-docs — the response content is determined by the {API_VERSION}
path segment further along. One centralised service handles every prefix
for these routes:
GET /obp/*/resource-docs/{API_VERSION}/obp
GET /obp/*/resource-docs/{API_VERSION}/swagger
GET /obp/v6.0.0/resource-docs/{API_VERSION}/openapi
GET /obp/v6.0.0/resource-docs/{API_VERSION}/openapi.yaml
GET /obp/*/banks/{BANK_ID}/resource-docs/{API_VERSION}/obp
GET /obp/*/message-docs/{CONNECTOR}/swagger2.0
Per-prefix Lift behaviour preserved:
- includeTechnology=true for v6.0.0 prefix (ResourceDocs600 override),
false for all other prefixes — Impl is picked by prefix via
implForPrefix(prefix).
- isV4OrHigher=true for v4.0.0+ prefixes; false for older.
- openapi / openapi.yaml routes guarded to prefix == v6.0.0 (matches
ResourceDocs600's exclusive registration of these handlers in Lift).
- resource_docs_requires_role prop honoured: builds CallContext via
Http4sCallContextBuilder, runs APIUtil.getUserAndSessionContextFuture,
returns 401 if unauthenticated and 403 if missing canReadResourceDoc /
canReadDynamicResourceDocsAtOneBank.
Two private converter helpers in ResourceDocsAPIMethods became
package-visible so the centralised service can reuse them without
duplicating the cache+JSON conversion logic:
convertResourceDocsToOpenAPI31JvalueAndSetCache
convertResourceDocsToSwaggerJvalueAndSetCache
Retired:
- 10 LiftRules.statelessDispatch.append(ResourceDocs140..600) entries
from Boot.scala.
- The openapi.yaml raw `serve { case Req(...) }` block in
ResourceDocs140.scala (~80 lines).
The ResourceDocs140..600 OBPRestHelper objects remain in the codebase
as the source of ImplementationsResourceDocs.getResourceDocsList (used
by the centralised service) but no longer participate in request
dispatch.
Verified by:
- ResourceDocsTest: 63/63 pass.
- SwaggerDocsTest: 10/10 pass.
This also absorbs the v3.1.0 getMessageDocsSwagger leftover that was
previously tracked under "Per-version Lift leftovers".
…adowed v7 handler V7ResourceDocsAggregationTest properties 1 and 2.10 were failing for two distinct reasons: 1. ResourceDocs1_4_0 registered the same (GET, /resource-docs/API_VERSION/obp) ResourceDoc twice (getResourceDocsObp + getResourceDocsObpV400), leaving a stale duplicate after aggregation. Dropped the V400 registration; the handler lazy val is still wired into ResourceDocs400/500/510/600.routes. 2. getAllResourceDocsObpCached kept dynamic-endpoint docs' specifiedUrl via `case Some(_) => it`, so the first request to touch a dynamic doc cemented its URL forever (e.g. /obp/dynamic-endpoint/...) and later v7 requests inherited that stale value. Always recompute specifiedUrl from the current requestedApiVersion; do the same in getResourceDocsObpDynamicCached so the two handlers can't poison each other across requests. Also added requestUrl + specifiedUrl to createLocalisedResourceDocJsonCached's cache key. Defensive — both are rewritten per request and so belong in the key — but not load-bearing for these tests. While auditing, removed the dead Http4s700.getResourceDocsObpV700 handler: in production, Http4sResourceDocs.routes (Http4sApp.baseServices line 109) intercepts every /obp/*/resource-docs/* request before v700Routes (line 113). The handler was only reachable from Http4s700RoutesTest, which drives Http4s700.wrappedRoutesV700Services in isolation; the 9 resource-docs scenarios there tested behavior that never ran in production. Dropped the handler, its ResourceDoc registration, the corresponding role-check special-case in ResourceDocMiddleware, and the 9 isolation-test scenarios. Updated CLAUDE.md count from 111 to 102 scenarios.
`OAuth2Login` extends `RestHelper` but has no `serve` blocks — the file is a Bearer-token validator library (Google / Yahoo / Azure / Keycloak / OBPOIDC / Hydra) consumed by APIUtil.getUserFuture and OBPRestHelper.OAuth2.getUser. Both Lift and http4s endpoints already call it; nothing depends on the Lift HTTP-dispatch surface that RestHelper provides. Removing the mixin lost one inherited implicit (DefaultFormats), used at a single `extract[List[String]]` site for syncing Keycloak roles. Restored it with a local `implicit val formats: Formats = DefaultFormats`. Updated LIFT_HTTP4S_MIGRATION.md's auth-stack table to reflect: - OAuth2 is library-only, not a route-bearing Lift handler (the original "OAuth 2.0 token & callback" description was misleading); - OpenIdConnect is blocked on a portal-session decision — its success path calls AuthUser.logUserIn / S.redirectTo, which mutate Lift SessionVars that the portal reads. No tests cover that path, so a pure http4s rewrite needs either dropped portal-login behaviour or a session shim — both require design before code.
…r mixins Four cleanups in one commit, all small, all unblocked, all already covered by Http4s alternatives or no longer relevant. Auth-stack: GatewayLogin and DAuth follow the same pattern as OAuth2Login (retired in the previous commit) — `extends RestHelper` with no `serve` blocks and no LiftRules registration. The mixin only ever contributed an implicit `DefaultFormats`; restored with an object-level `implicit val formats` at the `.extract[...]` call sites. They remain Bearer-token / JWT-exchange libraries consumed by the auth path. OAuth 1.0a + OpenIdConnect are the only auth handlers with actual route work left (OpenIdConnect blocked on a portal-session decision; OAuth 1.0a needs handler-file location). v1.4.0: `testResourceDoc` (dev-mode-only `GET /dummy` stub) deleted. It returned a dummy APIInfoJSON and existed only to exercise the resource-doc renderer's markdown handling — that's covered by real endpoints today. Removed from `OBPAPI1_4_0.routes` and `Implementations1_4_0`. v3.1.0: `getMessageDocsSwagger` (Lift handler for `GET /message-docs/CONNECTOR/swagger2.0`) deleted. The URL was already served by `Http4sResourceDocs.routes` ahead of any version-specific routes in `Http4sApp.baseServices`; the Lift `lazy val` was shadowed dead code. `GetMessageDocsSwaggerTest` still exercises the path — it just hits the http4s handler now. The test tag, which referenced the deleted `lazy val` via `nameOf(...)`, switched to a string literal. Docs updated to reflect: per-version completeness (v1.4.0 now 0 leftovers, v3.1.0 down to 1), auth-stack table, and the Suggested ordering list. Verified: 130 tests across AuthenticationPropertyTest, AuthenticationRefactorTest, gateWayloginTest, dauthTest, GetMessageDocsSwaggerTest, ResourceDocsTest, SwaggerDocsTest, V7ResourceDocsAggregationTest — all green.
The previous commit (f3f99a1) deleted v3.1.0's `getMessageDocsSwagger` Lift `lazy val` on the grounds that `Http4sResourceDocs.routes` already shadowed its URL in production. That's true, but the deletion has two side effects that crossed the "don't touch tests below v7.0.0" line: 1. `FrozenClassTest` (CI shard 4) detected the missing `lazy val` in `Implementations3_1_0` and reported "v3.1.0 reduced apis: getMessageDocsSwagger" — the frozen-API guard correctly rejects STABLE-version surface reductions. The documented escape hatch is re-running `FrozenClassUtil.main` to refresh the persisted snapshot, which acknowledges the deletion as intentional. 2. `GetMessageDocsSwaggerTest` references the lazy val via `nameOf(Implementations3_1_0.getMessageDocsSwagger)` for its tag, so removing the val required editing a v3.1.0 test file. Both effects are below-v7.0.0 churn that the rest of the migration doesn't take on. Restoring the Lift `lazy val` + ResourceDoc returns v3.1.0 to its pre-commit shape; nothing in production changes because `Http4sResourceDocs.routes` still intercepts the URL ahead of v310Routes. `testResourceDoc` deletion stays — its ResourceDoc was registered behind `if (Props.devMode)`, so the frozen snapshot (captured in test mode) never contained it; `FrozenClassTest` passes without it. Docs updated: CLAUDE.md per-version table puts `getMessageDocsSwagger` back in v3.1.0's row with an explanation of why it's intentionally kept; LIFT_HTTP4S_MIGRATION.md leftovers table likewise. Both endpoints retire together in the bridge-removal PR.
# Conflicts: # obp-api/src/main/scala/code/api/v1_4_0/JSONFactory1_4_0.scala
OAuth 1.0a was removed in 51820c7, but a number of references survived across source code, docs, and config. They are not load-bearing — the auth chain ignores them — but they mislead readers and the OpenAPI spec publishes phantom token-issuance URLs. Source: - AuthHeaderParser.parseOAuthHeader + oAuthParams on ParsedAuthHeader - oAuthParams on CallContext, oAuthToken on CallContextLight - extractOAuthParams in Http4sSupport - APIUtil.hasAnOAuthHeader + its unused import in WriteMetricUtil - AuthInterceptor (gRPC) oAuthParams plumbing + stale comment - OAuthHandshake comment refs in directlogin.scala and AuthUser.scala - Helper.extractOauthToken (zero callers, OAuth 1.0a-specific) - /oauth/authorize from Helper.isValidInternalRedirectUrl whitelist (URL no longer exists since 51820c7) OpenAPI: - OpenAPI31JSONFactory's OAuth2 security scheme was advertising phantom /oauth/authorize and /oauth/token URLs as an oauth2 authorizationCode flow. Replaced with type=http, scheme=bearer, bearerFormat=JWT — accurate for OBP's actual model (Bearer-token validation against external IdPs; no own token issuance). Docs/config: - Glossary.scala consumer_key and Authentication entries: historical OAuth 1.0a mention kept with explicit "no longer supported" note; removed from supported-methods list. - brief_system_documentation.md: removed "OAuth 1.0a (legacy)" from two auth-method lists. - introductory_system_documentation.md: deleted dedicated subsections 7.1.1 and 6.1.1 (full request/response examples), removed inline references in 4 other places, dropped allow_oauth1_login=true sample prop, rebalanced 7.1.x and 6.1.x numbering. - sample.props.template: removed webui_oauth_1_documentation_url prop and its comment (verified zero consumers). - LIFT_HTTP4S_MIGRATION.md: removed stale OAuth row from auth-stack workstream table; generalised stale S.request risk-row advice to apply to any future auth/handshake migration (e.g. OIDC). - Deleted Glossary.scala.bak (246 KB stale backup). Kept on purpose: - code/model/OAuth.scala backs the general Consumer entity, not OAuth 1.0a-specific. - APIUtil.OAuth is misnamed but live test infrastructure (~hundreds of test files use the <@ DirectLogin-signer operator). Renaming is a separate refactor. - Test fixture URLs containing oauth_token/oauth_verifier query params in APIUtilTest, HelperTest, Http4sRequestConversionPropertyTest — these test generic URL/header parsing; the OAuth content is just representative data. mvn compile and mvn test-compile both clean.
…tation
ScalaTest instantiates every suite up-front during discovery, so any
`setPropsValues` call at trait-body or feature-body level (outside a
`scenario {}`) mutates `Props.lockedProviders` before any test executes.
With `forkMode=once` (Jenkins), every subsequent suite's PropsReset
snapshot captures the pollution as its baseline — restored forever by
`afterEach`. Jenkins #7861 saw 105 spurious 404s from a single offending
`setPropsValues("api_enabled_versions" -> "[OBPv4.0.0]")` in
APIUtilHeavyTest cascading through every v6/v7-touching suite.
PR OpenBankProject#2794 fixed the immediate symptom by scoping the offending calls into
`scenario {}` blocks (caller discipline). This commit additionally fixes
the bug class structurally so future regressions of the same shape are
neutralised before they leak across suites.
PropsReset:
- Process-global IdentityHashMap-backed `ownedMaps` set tracks every
`setPropsValues` push across all suite instances.
- `beforeAll` wipes every owned map from Props.lockedProviders before
chaining super, purging any cross-suite contamination from other
suites' construction-time pushes.
- Snapshot captured at first `beforeEach` (after all `beforeAll`-chain
pushes have landed); `afterEach` restores to that snapshot.
- Identity tracking (not structural equality) so two suites pushing the
same content don't erase each other's pushes.
ServerSetup:
- Trait-body `setPropsValues` calls moved into the existing `beforeAll`
override. Required because PropsReset's wipe would otherwise erase
them along with the cross-suite pollution.
Verified: full single-fork run with APIUtilHeavyTest and
ConnectorMethodTest reverted to their pre-PR-OpenBankProject#2794 buggy form passes
2801/2805 (the 4 remaining failures are pre-existing isolation-failing
bugs unrelated to this change).
…r init The previous commit (PropsReset architectural fix) moved ServerSetup's 13 baseline `setPropsValues` calls from trait body to `beforeAll`. That broke the GitHub Actions test run because `val server = TestServer` at trait construction triggers Lift Boot + http4s server startup, which reads Props eagerly: `migration_scripts.execute_all`, `connector`, etc. must be set BEFORE Boot runs, not in `beforeAll` (which fires later). In environments where `test.default.props` already carries these values (the source-controlled file), the missing trait-body pushes are masked. GitHub Actions writes a minimal `test.default.props` from scratch that omits them — so Boot ran without migrations, leaving DB tables missing. This surfaced as 410 `Random.nextInt(bound)` "bound must be positive" failures across ~50 suites whose `randomPrivateAccount` / `randomBank` helpers walked empty result lists. Fix: extract the baseline pushes into `pushBaselineProps()` and call it twice — once at trait body (so Boot sees them) and once in `beforeAll` (so they survive PropsReset's wipe between suites). Verified: full local single-fork 2806/2810 unchanged; API1_2_1Test 323/323 passed when run against the GH-Actions-style minimal props (previously 313/323 failed in shard3 of the same workflow).
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.