feat(infra): authentication through github is here. Need to be tested… - #133
Merged
yaylymov merged 23 commits intoJul 17, 2026
Merged
Conversation
… and checked before merging
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds GitHub OAuth authentication across the orchestrator and client, persists GitHub users with provisioned accounts, sends bearer tokens to protected APIs, changes frontend image packaging, wires PostgreSQL and OAuth configuration, and adds Windows development and demo-data tooling. ChangesGitHub OAuth authentication
Runtime and development tooling
GenAI payload defaults
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant ClientApp
participant AuthController
participant GitHub
participant UserRepository
Browser->>ClientApp: Click GitHub sign-in
ClientApp->>AuthController: Request authorization URL
AuthController-->>ClientApp: Return authorization URL and state
ClientApp->>GitHub: Authorize application
GitHub-->>ClientApp: Return code and state
ClientApp->>AuthController: Submit callback
AuthController->>GitHub: Exchange code and fetch profile
AuthController->>UserRepository: Upsert user and provision account
AuthController-->>ClientApp: Return session token and user
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ion-user-registration
…ion-user-registration
yaylymov
marked this pull request as ready for review
July 16, 2026 21:18
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (1)
scripts/seed-demo-data.sql (1)
14-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate stale comment regarding idempotency mechanism.
The comment indicates that idempotency relies on
ON CONFLICT (id) DO UPDATE, but the script successfully achieves this by explicitly issuingDELETE FROMfollowed byINSERT.💡 Proposed update to documentation
-- Fixed IDs for the seeded connections. Keeping them stable makes the script idempotent --- (ON CONFLICT (id) DO UPDATE) and lets us wipe just our seeded transactions. +-- (by explicitly wiping and recreating the rows) and lets us wipe just our seeded transactions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/seed-demo-data.sql` around lines 14 - 15, Update the comment above the seeded connection IDs to describe the actual idempotency mechanism: the script explicitly deletes existing seeded rows before inserting them. Remove the inaccurate reference to ON CONFLICT (id) DO UPDATE while retaining the explanation that stable IDs enable targeted cleanup of seeded transactions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/src/api.ts`:
- Around line 94-137: Remove bearer-token persistence from localStorage in
getAuthToken, saveAuth, and clearAuth, and rely on a server-set Secure,
HttpOnly, SameSite cookie for authentication instead. Update authHeaders and API
request handling to use cookie credentials without emitting an Authorization
bearer token, and add CSRF protection for state-changing requests using the
project’s existing conventions.
- Around line 167-174: Update signOut to capture authHeaders() first, call
clearAuth() synchronously before awaiting the logout request, then send the
captured headers for best-effort server invalidation. Preserve the existing
error suppression and avoid relying on current authentication state after the
request begins.
In `@client/src/App.tsx`:
- Around line 1224-1271: Prevent the mount-time revalidation effect from running
while the current URL is the GitHub OAuth callback, so fetchCurrentUser cannot
race with completeGithubLogin. Update the early guard in the revalidation
useEffect, while preserving normal stored-token validation and the existing
callback flow.
In `@docker-compose.dev.yml`:
- Around line 110-115: Update the orchestrator service’s depends_on
configuration to map syntax and set the postgres dependency condition to
service_healthy, while preserving the existing dependencies and their startup
behavior.
In
`@server/orchestrator-service/src/main/java/com/team/bank/orchestrator/AuthController.java`:
- Around line 315-322: Update pruneStates() so exceeding MAX_PENDING_STATES
evicts only expired or excess individual entries rather than clearing
pendingStates globally. Preserve active OAuth states, and add rate limiting to
the unauthenticated login flow to prevent repeated requests from forcing
unbounded state creation.
- Around line 64-65: Update the session handling around the sessions map and its
token creation and lookup paths to store issuance/expiry metadata, enforce a
fixed TTL by rejecting and removing expired tokens, and enforce a maximum
session capacity by evicting or rejecting entries when full. Prefer an existing
shared session-store abstraction if available, while preserving
authenticated-user and logout behavior.
- Around line 62-63: Bind each OAuth state to the initiating browser in the
authorization flow around pendingStates and the callback handling at lines 81-95
and 108-112. Set a Secure, HttpOnly, SameSite cookie containing the issued state
when login begins, then require and strictly compare that cookie with the
returned callback state before exchanging the code; reject mismatches and clear
the cookie after consumption.
- Around line 146-152: Update the sign-in log in AuthController to remove direct
user profile fields, including firstName, lastName, and email, from the INFO
message and its arguments. Retain only the non-PII identifiers or event
information needed to record the successful sign-in.
- Around line 178-185: Update DashboardController and BankingController to call
AuthController.lookupSession for every user-scoped dashboard, banking, sync, and
banking/chat proxy route, requiring a valid signed-in session before processing
requests. Enforce that each path or body accountId matches the authenticated
AppUser.accountId, rejecting mismatches; preserve only explicitly designated
demo/read-only exceptions.
In
`@server/orchestrator-service/src/main/java/com/team/bank/orchestrator/UserRepository.java`:
- Around line 122-172: Restrict seedStarterData so it never runs for Live
accounts; only invoke it for the shared demo account or another explicitly
identified demo context. Preserve the existing synthetic connection, balance,
and transaction inserts for that demo path, and ensure first-time Live accounts
retain the intended empty state.
---
Nitpick comments:
In `@scripts/seed-demo-data.sql`:
- Around line 14-15: Update the comment above the seeded connection IDs to
describe the actual idempotency mechanism: the script explicitly deletes
existing seeded rows before inserting them. Remove the inaccurate reference to
ON CONFLICT (id) DO UPDATE while retaining the explanation that stable IDs
enable targeted cleanup of seeded transactions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9431d146-8cca-4886-932e-6e80d52fcde2
📒 Files selected for processing (20)
client/.dockerignoreclient/Dockerfileclient/src/App.test.tsxclient/src/App.tsxclient/src/api.tsclient/src/styles/app.cssdocker-compose.dev.ymldocker-compose.ymlgenai/main.pyinfra/docker/init.sqlinfra/docker/migrate-users.sqlscripts/dev-down.ps1scripts/dev-up.ps1scripts/seed-demo-data.sqlserver/gradle/libs.versions.tomlserver/orchestrator-service/build.gradle.ktsserver/orchestrator-service/src/main/java/com/team/bank/orchestrator/AuthController.javaserver/orchestrator-service/src/main/java/com/team/bank/orchestrator/AuthModels.javaserver/orchestrator-service/src/main/java/com/team/bank/orchestrator/UserRepository.javaserver/orchestrator-service/src/main/resources/application.yml
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a GitHub OAuth-based sign-in flow with persisted per-user accounts (plus first-login provisioning/seed data), updates the SPA to use authenticated sessions and per-user accountId in Live mode, and adjusts dev/CI tooling to support the new build + seed workflows.
Changes:
- Introduces orchestrator GitHub OAuth endpoints and a JDBC-backed
userspersistence layer with per-user account provisioning. - Updates the client to sign in via GitHub, store a bearer token + user in
localStorage, and sendAuthorization: Beareron/api/*calls. - Adds demo data seeding and Windows PowerShell dev scripts; updates Dockerfiles/CI workflows to build
client/diston the host.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| server/orchestrator-service/src/main/resources/application.yml | Adds datasource + GitHub OAuth config wiring. |
| server/orchestrator-service/src/main/java/com/team/bank/orchestrator/UserRepository.java | New JDBC repository to upsert users and provision per-user aggregate accounts + starter data. |
| server/orchestrator-service/src/main/java/com/team/bank/orchestrator/AuthModels.java | New auth-related DTOs/records for the OAuth flow. |
| server/orchestrator-service/src/main/java/com/team/bank/orchestrator/AuthController.java | New GitHub OAuth login/callback/me/logout endpoints with in-memory sessions/state. |
| server/orchestrator-service/build.gradle.kts | Adds JDBC starter + Postgres driver. |
| server/gradle/libs.versions.toml | Adds version catalog entry for Spring JDBC starter. |
| scripts/seed-demo-data.sql | Adds demo aggregate seed dataset for dev/public demo usage. |
| scripts/dev-up.ps1 | Adds PowerShell dev stack bring-up script with optional sequential build + seeding. |
| scripts/dev-down.ps1 | Adds PowerShell dev stack tear-down script. |
| infra/docker/migrate-users.sql | Adds idempotent migration for the new users table. |
| infra/docker/init.sql | Adds users table creation to DB init. |
| genai/main.py | Makes some AccountSummary fields optional-by-default to avoid 422s. |
| docker-compose.yml | Wires orchestrator datasource + GitHub OAuth env vars and DB dependency. |
| docker-compose.dev.yml | Wires orchestrator datasource + GitHub OAuth env vars for dev compose. |
| client/src/styles/app.css | Adds avatar styling in header status bar. |
| client/src/App.tsx | Replaces admin/admin login with GitHub OAuth, stores session, and uses per-user accountId for Live mode. |
| client/src/App.test.tsx | Updates tests for GitHub sign-in button and per-user accountId flow. |
| client/src/api.ts | Adds auth helpers + GitHub auth API calls; attaches Authorization headers to API calls. |
| client/src/api.test.ts | Updates fetch expectations to allow options bags for auth headers. |
| client/Dockerfile | Switches to serving pre-built dist/ via nginx only. |
| client/.dockerignore | Narrows client build context to only required runtime assets. |
| .github/workflows/docker.yaml | Builds client bundle on the runner before building/pushing client image. |
| .github/workflows/cd.yml | Builds client bundle on the runner before building/pushing client image. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Terraform plan
|
Terraform plan
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/src/api.test.ts`:
- Around line 74-78: Update both authenticated request assertions in
client/src/api.test.ts:74-78 and client/src/api.test.ts:97-97 to match the
expected headers object containing the bearer authorization value instead of
accepting any options object. Apply this to the dashboard request assertion and
the bank request assertion, preserving their existing URL checks.
In `@infra/ansible/playbook.yml`:
- Around line 189-202: Add a task immediately after the “Build client bundle”
task to recursively restore ownership of client/dist and client/node_modules to
the deployment user and group used by the playbook. Use the existing app_dir and
ansible_user-related variables rather than hardcoding values, ensuring the
subsequent synchronize task can delete or replace generated files without
permission errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e60e34b1-e21b-454e-a328-e4c292e1f6fe
📒 Files selected for processing (14)
.github/workflows/cd.yml.github/workflows/docker.yaml.github/workflows/infra-deploy.ymlclient/src/App.test.tsxclient/src/api.test.tsclient/src/api.tsinfra/ansible/group_vars/all.ymlinfra/ansible/playbook.ymlinfra/helm/banking-app/templates/orchestratorService-deployment.yamlinfra/helm/banking-app/values.yamlscripts/seed-demo-data.sqlserver/orchestrator-service/src/main/java/com/team/bank/orchestrator/AuthController.javaserver/orchestrator-service/src/main/java/com/team/bank/orchestrator/DashboardModels.javaserver/orchestrator-service/src/main/java/com/team/bank/orchestrator/UserRepository.java
🚧 Files skipped from review as they are similar to previous changes (5)
- client/src/App.test.tsx
- server/orchestrator-service/src/main/java/com/team/bank/orchestrator/UserRepository.java
- client/src/api.ts
- server/orchestrator-service/src/main/java/com/team/bank/orchestrator/AuthController.java
- scripts/seed-demo-data.sql
Terraform plan
|
Terraform plan
|
Terraform plan
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@infra/ansible/playbook.yml`:
- Around line 231-264: Update the “Wait for postgres to accept connections”,
“Apply multibank schema migration”, and “Apply users schema migration” tasks to
use the Postgres container’s populated environment variables for the username
and database instead of hardcoded bank and bankdb values. Execute the readiness
check and psql commands through sh -c inside the database container so
POSTGRES_USER and the corresponding database configuration are resolved there,
while preserving the existing retry and migration behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c475bd60-59f3-4885-8595-c90364f1edec
📒 Files selected for processing (2)
client/src/App.test.tsxinfra/ansible/playbook.yml
…ion-user-registration
Terraform plan
|
Terraform plan
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/orchestrator-service/src/main/java/com/team/bank/orchestrator/UserRepository.java (1)
89-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the
upsertJavadoc to match the empty-account behavior.Lines 27-28 still claim that a starter transaction set is provisioned. Remove that phrase so the documented contract matches this change and does not imply synthetic data belongs in Live accounts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/orchestrator-service/src/main/java/com/team/bank/orchestrator/UserRepository.java` around lines 89 - 92, Update the Javadoc for UserRepository.upsert to remove the claim that a starter transaction set is provisioned, documenting only the empty-account creation and linking behavior for live accounts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@infra/helm/banking-app/templates/init-sql-configmap.yaml`:
- Around line 76-78: Ensure Helm upgrades execute the users migration for
existing PVCs by adding or updating a Helm hook/job that runs
infra/docker/migrate-users.sql, rather than relying solely on the
init-sql-configmap.yaml initialization script. Keep the migration idempotent and
ensure it runs during upgrade without disrupting fresh database initialization.
---
Nitpick comments:
In
`@server/orchestrator-service/src/main/java/com/team/bank/orchestrator/UserRepository.java`:
- Around line 89-92: Update the Javadoc for UserRepository.upsert to remove the
claim that a starter transaction set is provisioned, documenting only the
empty-account creation and linking behavior for live accounts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c5533582-4be3-4f56-8f93-031287341229
📒 Files selected for processing (9)
client/src/App.test.tsxclient/src/App.tsxclient/src/api.test.tsclient/src/api.tsclient/src/styles/app.cssdocker-compose.ymlinfra/helm/banking-app/templates/init-sql-configmap.yamlserver/orchestrator-service/src/main/java/com/team/bank/orchestrator/DashboardModels.javaserver/orchestrator-service/src/main/java/com/team/bank/orchestrator/UserRepository.java
💤 Files with no reviewable changes (1)
- client/src/api.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- client/src/styles/app.css
- server/orchestrator-service/src/main/java/com/team/bank/orchestrator/DashboardModels.java
- docker-compose.yml
- client/src/api.ts
- client/src/App.test.tsx
- client/src/App.tsx
Terraform plan
|
Terraform plan
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/orchestrator-service/src/main/java/com/team/bank/orchestrator/DashboardController.java (1)
52-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant
WebClientclone.The
webClient.mutate().build()call creates an exact copy of the injectedWebClientwithout applying any configuration changes. SinceWebClientinstances are immutable and thread-safe, you can safely assign the injected instance directly.(Note: If the intent was to forward the incoming
Authorizationheader to downstream services, an exchange filter should be added here, or the header must be explicitly passed in eachretrieve()call.)♻️ Proposed refactor
public DashboardController(WebClient webClient, AuthController authController) { - this.webClient = webClient.mutate().build(); + this.webClient = webClient; this.authController = authController; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/orchestrator-service/src/main/java/com/team/bank/orchestrator/DashboardController.java` around lines 52 - 54, Update the DashboardController constructor to assign the injected WebClient directly to the webClient field instead of calling mutate().build(). Keep the existing authController assignment and downstream request behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@server/orchestrator-service/src/main/java/com/team/bank/orchestrator/DashboardController.java`:
- Around line 52-54: Update the DashboardController constructor to assign the
injected WebClient directly to the webClient field instead of calling
mutate().build(). Keep the existing authController assignment and downstream
request behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 70e9049d-590f-4a71-b193-c3f8c5d918ef
📒 Files selected for processing (1)
server/orchestrator-service/src/main/java/com/team/bank/orchestrator/DashboardController.java
Terraform plan
|
Terraform plan
|
azzabaatout
reviewed
Jul 17, 2026
Collaborator
There was a problem hiding this comment.
there are 2 ways of doing this, either hard code like you did, or we could have put the data in a json file and uploaded it to enablebanking in the mock asps profile. both are okay, even though, this variant is a lot of overhead
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Terraform plan
|
azzabaatout
approved these changes
Jul 17, 2026
azzabaatout
left a comment
azzabaatout
left a comment
Collaborator
There was a problem hiding this comment.
i left some comments, some are observations - there's however one with the secrets that i think you should have a look on @yaylymov
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Terraform plan
|
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Terraform plan
|
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
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.
GitHub OAuth sign-in + per-user accounts
Replaces the hardcoded
admin/adminlogin with a real GitHub OAuth flow, persists users in a newuserstable, and gives each signed-in user their own aggregate account with seeded starter transactions on first login.Backend (
orchestrator-service)AuthController:login,callback,me,logout. In-memory session map (no JWT infra); state param TTL-bounded to prevent CSRF/replay.UserRepository(JdbcClient, no JPA). On first sign-in: upserts the user, creates theiraccountsrow, linksusers.account_id, seeds 1 bank + ~15 transactions. Idempotent on later logins.spring-boot-starter-jdbc+postgresql; datasource wired viaSPRING_DATASOURCE_*.GITHUB_CLIENT_ID,GITHUB_CLIENT_SECRET,GITHUB_REDIRECT_URI(defaulthttps://localhost/login/oauth2/code/github).genai/main.pyfix —AccountSummary.totalCreditLimit/utilizationRatemade optional so/summarizeand/chatstop 422-ing.Frontend (
client)localStorage; every/api/*call sendsAuthorization: Bearer; SPA revalidates via/api/auth/meon mount.accountId. Demo mode stays on the shared1111…aggregate.testUser.accountId).Database
userstable + migrationinfra/docker/migrate-users.sqlfor existing volumes.scripts/seed-demo-data.sql: 4 banks, ~€16k, 48 transactions over 6 months on the demo account. Idempotent, doesn't touch the Live aggregate.Dev workflow
.shscripts:scripts/dev-up.ps1/scripts/dev-down.ps1.-Sequentialfor small Docker Desktop VMs,-SeedDemoto apply the demo seed.client/Dockerfilenow just serves a pre-builtdist/; the npm build runs on the host (documented in the Dockerfile — Cloudflare rejectsregistry.npmjs.orgfrom containers inside Docker Desktop's WSL 2 VM).How to test
https://localhost/login/oauth2/code/github..\scripts\dev-up.ps1 -Sequential -SeedDemohttps://localhost/, click Sign in with GitHub.bankdbvolume predates this PR, apply the migrations once (seeinfra/docker/migrate-users.sql+infra/docker/migrate-multibank.sql).Summary by CodeRabbit