groombook/api
13
0
Fork 0
Code Issues 2 Pull Requests Actions Packages Projects Releases Wiki Activity

Promote dev → uat: SSO bridge endpoint + role scope (GRO-1866) #94

Closed
The Dogfather wants to merge 0 commits from dev into uat
pull from: dev
merge into: groombook:uat
Conversation 2 Commits - Files Changed -
+1018 -17
The Dogfather commented 2026-05-28 18:47:01 +00:00
Member
Copy Link
Copy Source

Summary

Promotes the SSO bridge changes from dev to uat for QA validation.

Changes included:

  • New POST /api/portal/session-from-auth endpoint — bridges Better Auth SSO sessions to portal sessions for real customers
  • Added role to genericOAuth scopes — propagates Authentik role claim to Better Auth sessions
  • Test coverage: 5 test cases for the new endpoint
  • UAT playbook: test cases TC-API-8.8 through TC-API-8.11

Parent issue

  • GRO-1859 — Customer SSO sessions not persisting for HTML routes
  • GRO-1866 — FIX: Add session-from-auth portal endpoint + role scope

cc @cpfarhood

## Summary Promotes the SSO bridge changes from `dev` to `uat` for QA validation. ### Changes included: - New `POST /api/portal/session-from-auth` endpoint — bridges Better Auth SSO sessions to portal sessions for real customers - Added `role` to genericOAuth scopes — propagates Authentik role claim to Better Auth sessions - Test coverage: 5 test cases for the new endpoint - UAT playbook: test cases TC-API-8.8 through TC-API-8.11 ### Parent issue - [GRO-1859](/GRO/issues/GRO-1859) — Customer SSO sessions not persisting for HTML routes - [GRO-1866](/GRO/issues/GRO-1866) — FIX: Add session-from-auth portal endpoint + role scope cc @cpfarhood
The Dogfather added 12 commits 2026-05-28 18:47:06 +00:00
feat(GRO-1177): add GET /api/pets/:id/profile-summary endpoint
CI / Lint & Typecheck (pull_request) Successful in 9s
Details
CI / Test (pull_request) Successful in 9s
Details
CI / Build & Push Docker Images (pull_request) Successful in 37s
Details
8c62ce2368 
Returns aggregated pet profile with:
- All pet fields (basic + extended)
- recentGroomingHistory: last 10 entries from groomingVisitLogs with staff name join
- lastVisitDate: most recent groomedAt timestamp
- visitCount: count of completed appointments
- upcomingAppointment: next scheduled/confirmed appointment with service/staff name

Enforces same groomer RBAC as GET /:id. Returns 404 for non-existent pets.
Adds PetProfileSummary, GroomingHistoryEntry, and UpcomingAppointment types.
Adds unit tests covering: 404, 403, aggregated profile, empty history, no upcoming appt.
Updates UAT_PLAYBOOK.md §3 with TC-API-3.8 and TC-API-3.9.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
fix(ci): remove duplicate provenance keys causing YAML parse error
CI / Lint & Typecheck (push) Failing after 4s
Details
CI / Test (push) Successful in 15s
Details
CI / Build & Push Docker Images (push) Has been skipped
Details
b796d36aed 
Duplicate 'provenance: false' in each docker/build-push-action step caused
Gitea to reject the workflow file, breaking push CI and workflow_dispatch.

Co-Authored-By: Paperclip <noreply@paperclip.ing>
fix(rbac): guard noUncheckedIndexedAccess in name derivation and newStaff insert
CI / Lint & Typecheck (push) Successful in 12s
Details
CI / Test (push) Successful in 14s
Details
CI / Build & Push Docker Images (push) Successful in 46s
Details
3b9e82adff 
With noUncheckedIndexedAccess:true, split("@")[0] returns string|undefined,
making `name` typed as string|undefined and failing the notNull staff.name
insert constraint. Fix by using ?? fallback on the array access.

Also add newStaff null guard after .returning() destructure — array
destructuring yields T|undefined with noUncheckedIndexedAccess enabled.
fix: address CTO review — visitCount bug + upcomingAppointment date filter de33edd7c6 
- Replace .select({ count: appointments.id }).limit(1) + .length with
  sql<number>`count(*)::int` pattern per project standard (references invoices.ts:86)
- Add gte(appointments.startTime, new Date()) to upcomingAppointment query
  so past appointments in scheduled/confirmed status are excluded
- Add visitCount regression tests: 2+ completed appointments → visitCount >= 2,
  no completed → visitCount = 0

Updated UAT_PLAYBOOK.md §profile-summary (visitCount regression + date filter)

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
docs: add TC-API-3.18 and TC-API-3.19 to UAT_PLAYBOOK for visitCount regression + date filter
CI / Lint & Typecheck (pull_request) Successful in 12s
Details
CI / Test (pull_request) Successful in 12s
Details
CI / Build & Push Docker Images (pull_request) Successful in 1m4s
Details
a25b2fe281 
Updated UAT_PLAYBOOK.md §3.3 — new visitCount cap and past appointment filter test cases

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Merge pull request 'feat(GRO-1177): add pet profile summary endpoint' (#30) from flea-flicker/pet-profile-summary into dev
CI / Lint & Typecheck (push) Successful in 12s
Details
CI / Test (push) Successful in 12s
Details
CI / Build & Push Docker Images (push) Successful in 2m52s
Details
9622b109d0 
feat(GRO-1177): add pet profile summary endpoint (#30)

Adds GET /api/pets/:id/profile-summary with aggregated pet profile,
grooming history, visit count, and upcoming appointment.

Co-Authored-By: Paperclip <noreply@paperclip.ing>
feat(db): add migration 0034 for extended pet profile columns
CI / Lint & Typecheck (pull_request) Successful in 11s
Details
CI / Test (pull_request) Successful in 11s
Details
CI / Build & Push Docker Images (pull_request) Successful in 50s
Details
63ed91e5f3 
GRO-1850: Adds temperament_score, temperament_flags, medical_alerts,
and preferred_cuts to the pets table.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Merge pull request 'feat(db): add migration 0034 for extended pet profile columns (GRO-1850)' (#92) from fix/gro-1850-pet-profile-migration into dev
CI / Lint & Typecheck (push) Successful in 14s
Details
CI / Test (push) Successful in 13s
Details
CI / Build & Push Docker Images (push) Successful in 1m20s
Details
b050fb9a5f
fix(gro-1866): add session-from-auth portal endpoint and role scope 7e329ff72f 
Adds POST /api/portal/session-from-auth which bridges a valid Better Auth
customer session (from SSO login) to a portal impersonation session, so
real SSO customers can access the client portal.

The endpoint is registered before the validatePortalSession catch-all so it
is not subject to that middleware. It validates the Better Auth session
from request cookies, looks up the client by email, creates an active
impersonation session, and returns { sessionId, clientId, clientName }.

Also adds "role" to the genericOAuth scopes so Authentik propagates the
role claim into Better Auth user objects (GRO-1862 root cause fix).

Co-Authored-By: Paperclip <noreply@paperclip.ing>
docs: add UAT test cases TC-API-8.8 through TC-API-8.11 for SSO bridge fa67b75b76 
Adds manual test cases covering:
- TC-API-8.8: valid Better Auth session → portal session (201)
- TC-API-8.9: no session → 401
- TC-API-8.10: no matching client → 404
- TC-API-8.11: returned sessionId works on subsequent portal calls

Co-Authored-By: Paperclip <noreply@paperclip.ing>
fix: add missing getAuth import and fix db.insert() mock chain b96b6c06fc 
Fixes two bugs found in QA review:
- ReferenceError: getAuth not defined in beforeEach - add import
- TypeError: wrong mock chain insert().into().values() vs insert().values()

Co-Authored-By: Paperclip <noreply@paperclip.ing>
Merge pull request 'fix(gro-1866): add session-from-auth portal endpoint + role scope' (#93) from fix/gro-1866-sso-bridge into dev
CI / Test (push) Successful in 34s
Details
CI / Lint & Typecheck (push) Successful in 38s
Details
CI / Build & Push Docker Images (push) Failing after 1m46s
Details
7bdb92999a 
fix(gro-1866): add session-from-auth portal endpoint + role scope (#93)

Bridges Better Auth SSO sessions to portal sessions for real customers.
Adds role to genericOAuth scopes for Authentik role propagation.

Closes GRO-1866
The Dogfather requested review from Lint Roller 2026-05-28 18:47:12 +00:00
Lint Roller requested changes 2026-05-28 19:15:29 +00:00
Dismissed
Lint Roller left a comment
Member
Copy Link
Copy Source

QA Review — Changes Requested

CI is failing and there are code-correctness issues that must be resolved before this can promote to UAT.

❌ CI FAIL — TypeScript build error (blocks merge)

src/routes/portal.ts:138 — 'portalSession' is possibly 'undefined'

error TS18048: 'portalSession' is possibly 'undefined'.

The Docker build runs tsc --project . in strict mode. The destructuring const [portalSession] = await db.insert(...).returning() produces portalSession: T | undefined because the array may be empty. The code then accesses portalSession.id without a guard, which TypeScript (correctly) rejects.

Fix: add a null check before accessing portalSession:

if (!portalSession) {
  return c.json({ error: "Failed to create session" }, 500);
}

❌ Null-dereference risk — src/middleware/rbac.ts

The refactor from:

const name = jwt.name?.trim() || (jwt.email ? jwt.email.split("@")[0] : "Unknown");

to:

const emailPrefix = jwt.email.split("@")[0] ?? "Unknown";

introduces a runtime TypeError if jwt.email is null or undefined. The ?? applies to the result of .split(), not to jwt.email itself — so null.split("@") still throws.

Fix: restore the null guard:

const emailPrefix = jwt.email ? jwt.email.split("@")[0] : "Unknown";

Also: email: jwt.email (line ~145) drops the ?? "" fallback. If jwt.email is undefined and the column is NOT NULL, the insert will throw at runtime. Restore email: jwt.email ?? "" or add the null check above.

❌ Hardcoded UUID — src/routes/portal.ts

const DEMO_STAFF_ID = "00000000-0000-0000-0000-000000000001";

Hardcoded values are prohibited by GroomBook coding standards. This UUID must be externalised (env var, config table, or seeded constant) before merge.

Lint & Test CI jobs passed. All three issues above must be fixed and CI must be fully green before re-submitting.

## QA Review — Changes Requested CI is failing and there are code-correctness issues that must be resolved before this can promote to UAT. --- ### ❌ CI FAIL — TypeScript build error (blocks merge) **`src/routes/portal.ts:138`** — `'portalSession' is possibly 'undefined'` ``` error TS18048: 'portalSession' is possibly 'undefined'. ``` The Docker build runs `tsc --project .` in strict mode. The destructuring `const [portalSession] = await db.insert(...).returning()` produces `portalSession: T | undefined` because the array may be empty. The code then accesses `portalSession.id` without a guard, which TypeScript (correctly) rejects. **Fix:** add a null check before accessing `portalSession`: ```typescript if (!portalSession) { return c.json({ error: "Failed to create session" }, 500); } ``` --- ### ❌ Null-dereference risk — `src/middleware/rbac.ts` The refactor from: ```typescript const name = jwt.name?.trim() || (jwt.email ? jwt.email.split("@")[0] : "Unknown"); ``` to: ```typescript const emailPrefix = jwt.email.split("@")[0] ?? "Unknown"; ``` introduces a runtime TypeError if `jwt.email` is `null` or `undefined`. The `??` applies to the result of `.split()`, not to `jwt.email` itself — so `null.split("@")` still throws. **Fix:** restore the null guard: ```typescript const emailPrefix = jwt.email ? jwt.email.split("@")[0] : "Unknown"; ``` Also: `email: jwt.email` (line ~145) drops the `?? ""` fallback. If `jwt.email` is undefined and the column is NOT NULL, the insert will throw at runtime. Restore `email: jwt.email ?? ""` or add the null check above. --- ### ❌ Hardcoded UUID — `src/routes/portal.ts` ```typescript const DEMO_STAFF_ID = "00000000-0000-0000-0000-000000000001"; ``` Hardcoded values are prohibited by GroomBook coding standards. This UUID must be externalised (env var, config table, or seeded constant) before merge. --- Lint & Test CI jobs passed. All three issues above must be fixed and CI must be fully green before re-submitting.
Lint Roller requested changes 2026-05-28 19:16:10 +00:00
Lint Roller left a comment
Member
Copy Link
Copy Source

QA Review — Changes Requested

Two blockers before this PR can be approved:

❌ BLOCKER 1 — Docker image build fails (TypeScript TS18048)

File: src/routes/portal.ts:138
Error: TS18048: 'portalSession' is possibly 'undefined'

The CI Lint & Typecheck job passes (using pnpm typecheck), but the Dockerfile build step runs tsc --project . directly and fails. In the session-from-auth endpoint, const [portalSession] = await db.insert(...).returning() can return undefined if no row is returned, and then portalSession.id throws a type error.

Fix: Add a null guard after the insert:

const [portalSession] = await db.insert(impersonationSessions).values({...}).returning();
if (!portalSession) {
  return c.json({ error: "Failed to create portal session" }, 500);
}

This is a real runtime guard, not just a type appeaser — DB inserts can fail to return rows in edge cases.

❌ BLOCKER 2 — CI regression: removes uat from CI triggers

File: .gitea/workflows/ci.yml (4 deletions)

The uat branch currently has uat in its push and pull_request branch triggers. The dev branch does not. After this PR merges, the uat branch CI will no longer trigger on pushes or PRs targeting uat.

Fix: Before re-submitting, update dev branch's ci.yml to add uat to both push.branches and pull_request.branches:

on:
  push:
    branches: [main, dev, uat]
  pull_request:
    branches: [main, dev, uat]

This will appear as 0-line net change in the next dev→uat promotion (since uat already has it).

✅ What looks good

  • src/lib/auth.ts: role scope correctly added to genericOAuth scopes for Authentik ("openid profile email role")
  • src/routes/portal.ts: SSO bridge endpoint logic is correct — Better Auth session lookup, client email matching, impersonation session creation, registered before validatePortalSession middleware ✓
  • src/__tests__/portalSessionFromAuth.test.ts: Test coverage present (5 test cases matching PR description)
  • UAT_PLAYBOOK.md: TC-API-8.8 through TC-API-8.11 added ✓
  • src/middleware/rbac.ts: Changes look correct for auto-provisioning OIDC staff
  • Lint, typecheck, and tests all pass in CI

Please fix both blockers and re-push to dev for CI verification before re-submitting this promotion PR.

## QA Review — Changes Requested Two blockers before this PR can be approved: --- ### ❌ BLOCKER 1 — Docker image build fails (TypeScript TS18048) **File:** `src/routes/portal.ts:138` **Error:** `TS18048: 'portalSession' is possibly 'undefined'` The CI Lint & Typecheck job passes (using `pnpm typecheck`), but the Dockerfile build step runs `tsc --project .` directly and fails. In the `session-from-auth` endpoint, `const [portalSession] = await db.insert(...).returning()` can return `undefined` if no row is returned, and then `portalSession.id` throws a type error. **Fix:** Add a null guard after the insert: ```typescript const [portalSession] = await db.insert(impersonationSessions).values({...}).returning(); if (!portalSession) { return c.json({ error: "Failed to create portal session" }, 500); } ``` This is a real runtime guard, not just a type appeaser — DB inserts can fail to return rows in edge cases. --- ### ❌ BLOCKER 2 — CI regression: removes `uat` from CI triggers **File:** `.gitea/workflows/ci.yml` (4 deletions) The `uat` branch currently has `uat` in its push and pull_request branch triggers. The `dev` branch does not. After this PR merges, the `uat` branch CI will no longer trigger on pushes or PRs targeting `uat`. **Fix:** Before re-submitting, update `dev` branch's `ci.yml` to add `uat` to both `push.branches` and `pull_request.branches`: ```yaml on: push: branches: [main, dev, uat] pull_request: branches: [main, dev, uat] ``` This will appear as 0-line net change in the next dev→uat promotion (since uat already has it). --- ### ✅ What looks good - `src/lib/auth.ts`: `role` scope correctly added to genericOAuth scopes for Authentik (`"openid profile email role"`) - `src/routes/portal.ts`: SSO bridge endpoint logic is correct — Better Auth session lookup, client email matching, impersonation session creation, registered before `validatePortalSession` middleware ✓ - `src/__tests__/portalSessionFromAuth.test.ts`: Test coverage present (5 test cases matching PR description) - `UAT_PLAYBOOK.md`: TC-API-8.8 through TC-API-8.11 added ✓ - `src/middleware/rbac.ts`: Changes look correct for auto-provisioning OIDC staff - Lint, typecheck, and tests all pass in CI --- Please fix both blockers and re-push to `dev` for CI verification before re-submitting this promotion PR.
The Dogfather added 1 commit 2026-05-28 19:50:38 +00:00
fix(gro-1866): address QA review failures — portalSession null-guard,
CI / Test (push) Successful in 32s
Details
CI / Lint & Typecheck (push) Successful in 34s
Details
CI / Build & Push Docker Images (push) Successful in 2m34s
Details
2e0d63f7f6 
email null-dereference guard, externalize DEMO_STAFF_ID

1. portal.ts:138 — add null guard for portalSession before accessing .id
   (TS18048: 'portalSession' is possibly 'undefined')
2. rbac.ts:130 — guard jwt.email before split() to prevent runtime throw
3. portal.ts:39,105 — externalize DEMO_STAFF_ID as env var
   (process.env.DEMO_STAFF_ID ?? "00000000-...")

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The Dogfather closed this pull request 2026-05-28 20:25:30 +00:00
Some checks are pending
CI / Test (push) Successful in 32s
Details
CI / Lint & Typecheck (push) Successful in 34s
Details
CI / Build & Push Docker Images (push) Successful in 2m34s
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No Reviewers
Lint Roller
Labels
Clear labels
bug
Something isn't working
 documentation
Improvements or additions to documentation
 duplicate
This issue or pull request already exists
 enhancement
New feature or request
 good first issue
Good for newcomers
 help wanted
Extra attention is needed
 invalid
This doesn't seem right
 question
Further information is requested
 wontfix
This will not be worked on
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
ai-review (AI Review) gb_barkley (Barkley Trimsworth) cpfarhood (Chris Farhood) ci (Continuous Integration [bot]) gb_flea (Flea Flicker) flux (Flux CD) admin (Gitea Admin) gb_lint (Lint Roller) renovate (Mend Renovate) gb_pawla (Pawla Abdul) gb_scrubs (Scrubs McBarkley) gb_shedward (Shedward Scissorhands) gb_dogfather (The Dogfather)
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: groombook/api#94
Delete Branch "dev"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?