Merge the SDAD session into the NextAuth session #199

2026-09-04T12:30:49Z

aaperis commented

2026-09-04 12:30:49 +00:00

(Migrated from github.com)

This PR closes #104 and also #146 as a side effect.

The app maintained two parallel session stores: NextAuth's own JWE cookie (next-auth.session-token) for sign-in state, and a custom SessionManager-backed cookie (sdad-session) for the OAuth access token and the user's Crypt4GH public key. This PR consolidates them into the NextAuth session and removes the custom store.

Everything the server needs — access token, expiresAt, Crypt4GH public key — now lives on the NextAuth JWT, encrypted with NEXTAUTH_SECRET and readable only server-side.

The change adhere to previous principles where the access token never crossed to the browser. The client-facing session object (reachable via useSession() and GET /api/auth/session) exposes only expires and pemChecksum;

Type of change

New feature (non-breaking change which adds functionality)
Refactoring

List of changes made

The JWT contract. types/next-auth.d.ts augments both JWT (with accessToken, refreshToken, expiresAt, publicKey) and Session (with pemChecksum). Note that refreshToken is kept here as placeholder since the implementation for using it is pending (#103).
Server-only helpers. app/lib/serverToken.ts provides:
- getServerToken() - decrypts and returns the JWT, or null.
- getSession() - a SessionData-shaped projection over the JWT ({ token, publicKey }), used by every reader that previously imported from @/app/lib/session.
- updateServerToken(patch) — verifies the current access token against the provider's JWK endpoint, then re-encrypts the cookie with the patch merged in. Refuses to write when the session is missing, has no expiresAt, or is already expired. This is called when the user wants to submit a different c4gh pub key.
- clearServerToken() — deletes the session cookie plus NextAuth's csrf-token and callback-url cookies for a tidy logout.
NextAuth session expiry bound to the access-token TTL. getAuthOptions() overrides jwt.encode to derive maxAge from token.expiresAt, so NextAuth's own exp check, SessionExpiryWatcher, and our helpers all agree on when the session ends.
Access-token verification against JWKS. app/lib/oidc.ts::verifyAccessToken is called on sign-in (extractJWT) and before every patch (updateServerToken). Discovery + JWKS are cached per process. On JOSE errors we clear the cookie and throw SessionInvalidError; on network errors we leave the cookie alone and propagate.
Typed error handling in the Crypt4GH action. Validation errors surface via Crypt4ghValidationError; session-invalid errors surface via SessionInvalidError; anything unexpected is logged server-side and replaced with a generic client message. Prevents future validation-code changes from accidentally leaking sensitive data through the form state.
All callers are migrated to getSession() or getServerToken(). All server components (app/(pages)/*, app/page.tsx, app/layout.tsx), route handlers (app/api/files/[fileId]/route.ts, app/api/datasets/[datasetId]/download.tar/route.ts), and server actions (app/actions/crypt4ghKey.ts, app/actions/logout.ts) now import from @/app/lib/serverToken.
Custom session code removed. app/lib/SessionManager.ts, app/lib/session.ts, and their tests are gone.
Config cleanup. sessionSecretPath is removed from the config schema.

Testing

A rebuild is needed due to new dependencies
Requires new configuration settings
Instructions on how to test

Manual testing:

Sign in with LS AAI.
Inspect cookies in DevTools: next-auth.session-token present on the app origin, sdad-session absent.
Submit a a Crypt4GH public key on /userinfo.
Download a single file: /api/files/… works.
Log out. NextAuth cookies are gone; sdad-session is absent throughout.

Further comments

Breaking config change.
sessionSecretPath is no longer a valid key in sdad-config.json - remove it before deploying. The secret file it pointed at is no longer needed and can be deleted from the deployment.

Definition of Done checklist

I have made an effort making the commit history understandable
I have performed a self-review of my own code and commented any hard-to-understand areas
Existing tests are passing and I wrote unit tests for new functionality
My changes generate no new warnings
All Acceptance Criteria (AC) Fulfilled and checked in the Related issue
Documentation Updated (if applicable):
- Relevant documentation is current.

## Related issue(s) and PR(s) This PR closes #104 and also #146 as a side effect. The app maintained two parallel session stores: NextAuth's own JWE cookie (next-auth.session-token) for sign-in state, and a custom SessionManager-backed cookie (sdad-session) for the OAuth access token and the user's Crypt4GH public key. This PR consolidates them into the NextAuth session and removes the custom store. Everything the server needs — access token, expiresAt, Crypt4GH public key — now lives on the NextAuth JWT, encrypted with NEXTAUTH_SECRET and readable only server-side. The change adhere to previous principles where the access token never crossed to the browser. The client-facing session object (reachable via useSession() and GET /api/auth/session) exposes only `expires` and `pemChecksum`; ## Type of change - [x] New feature (non-breaking change which adds functionality) - [x] Refactoring ## List of changes made - The JWT contract. `types/next-auth.d.ts` augments both JWT (with accessToken, refreshToken, expiresAt, publicKey) and Session (with pemChecksum). Note that refreshToken is kept here as placeholder since the implementation for using it is pending (#103). - Server-only helpers. `app/lib/serverToken.ts` provides: - `getServerToken()` - decrypts and returns the JWT, or null. - `getSession()` - a SessionData-shaped projection over the JWT ({ token, publicKey }), used by every reader that previously imported from `@/app/lib/session`. - `updateServerToken(patch)` — verifies the current access token against the provider's JWK endpoint, then re-encrypts the cookie with the patch merged in. Refuses to write when the session is missing, has no expiresAt, or is already expired. This is called when the user wants to submit a different c4gh pub key. - `clearServerToken()` — deletes the session cookie plus NextAuth's csrf-token and callback-url cookies for a tidy logout. - NextAuth session expiry bound to the access-token TTL. getAuthOptions() overrides jwt.encode to derive maxAge from token.expiresAt, so NextAuth's own exp check, SessionExpiryWatcher, and our helpers all agree on when the session ends. - Access-token verification against JWKS. app/lib/oidc.ts::verifyAccessToken is called on sign-in (extractJWT) and before every patch (updateServerToken). Discovery + JWKS are cached per process. On JOSE errors we clear the cookie and throw SessionInvalidError; on network errors we leave the cookie alone and propagate. - Typed error handling in the Crypt4GH action. Validation errors surface via Crypt4ghValidationError; session-invalid errors surface via SessionInvalidError; anything unexpected is logged server-side and replaced with a generic client message. Prevents future validation-code changes from accidentally leaking sensitive data through the form state. - All callers are migrated to getSession() or getServerToken(). All server components (app/(pages)/*, app/page.tsx, app/layout.tsx), route handlers (app/api/files/[fileId]/route.ts, app/api/datasets/[datasetId]/download.tar/route.ts), and server actions (app/actions/crypt4ghKey.ts, app/actions/logout.ts) now import from @/app/lib/serverToken. - Custom session code removed. app/lib/SessionManager.ts, app/lib/session.ts, and their tests are gone. - Config cleanup. sessionSecretPath is removed from the config schema. ## Testing - A rebuild is needed due to new dependencies - Requires new configuration settings - Instructions on how to test Manual testing: - Sign in with LS AAI. - Inspect cookies in DevTools: next-auth.session-token present on the app origin, sdad-session absent. - Submit a a Crypt4GH public key on /userinfo. - Download a single file: /api/files/… works. - Log out. NextAuth cookies are gone; sdad-session is absent throughout. ## Further comments **Breaking config change**. `sessionSecretPath` is no longer a valid key in sdad-config.json - remove it before deploying. The secret file it pointed at is no longer needed and can be deleted from the deployment. ## Definition of Done checklist - [x] I have made an effort making the commit history understandable - [x] I have performed a self-review of my own code and commented any hard-to-understand areas - [x] Existing tests are passing and I wrote unit tests for new functionality - [x] My changes generate no new warnings - [x] All Acceptance Criteria (AC) Fulfilled and checked in the Related issue - [x] **Documentation Updated (if applicable):** - Relevant documentation is current.

pdmnyberg commented

2026-09-04 13:54:06 +00:00

(Migrated from github.com)

I have had a look at most of the new code and I think that this is a clear improvement in many ways. I have tested the functionality and it seems to work as expected. I will take a more detailed look at the tests before approving.

👍 1

pdmnyberg (Migrated from github.com) reviewed

2026-09-04 14:23:26 +00:00

frontend/src/app/lib/serverToken.test.ts

					
				@ -0,0 +1,400 @@

				import { beforeEach, describe, expect, it, vi } from "vitest";

I feel like the name of this test hides what is actually being tester here. We are making sure it uses __Secure- when the auth url is https right? Does this always mean a production setup? I don't think this needs to change but it made me have a second look.

I feel like the name of this test hides what is actually being tester here. We are making sure it uses `__Secure-` when the auth url is https right? Does this always mean a production setup? I don't think this needs to change but it made me have a second look.

aaperis (Migrated from github.com) reviewed

2026-09-04 14:28:12 +00:00

frontend/src/app/lib/serverToken.test.ts

					
				@ -0,0 +1,400 @@

				import { beforeEach, describe, expect, it, vi } from "vitest";

Yes, great catch. I just noticed that the production image does not work as expected and I suspect I missed something related to https and cookie settings. I am looking at it now. I am setting the PR to draft until this is fixed.

aaperis (Migrated from github.com) reviewed

2026-09-04 14:40:34 +00:00

frontend/src/app/lib/serverToken.test.ts

					
				@ -0,0 +1,400 @@

				import { beforeEach, describe, expect, it, vi } from "vitest";

this is probably unrelated to what I said and may be due to changed docker networking internals. I will keep looking.

EDIT: False alarm. It was due to docker compose internals (network configuration) after all.

this is probably unrelated to what I said and may be due to changed docker networking internals. I will keep looking. EDIT: False alarm. It was due to docker compose internals (network configuration) after all.

aaperis (Migrated from github.com) reviewed

2026-09-07 12:07:12 +00:00

frontend/src/app/lib/serverToken.test.ts

					
				@ -0,0 +1,400 @@

				import { beforeEach, describe, expect, it, vi } from "vitest";

Back to your comment, you're right the title says production but what the test actually looks for is https. It is just that we assume that production is always https. Made that explicit now.

Back to your comment, you're right the title says production but what the test actually looks for is `https`. It is just that we assume that production is always https. Made that explicit now.

pdmnyberg (Migrated from github.com) reviewed

2026-09-08 08:54:35 +00:00

frontend/src/app/actions/crypt4ghKey.test.ts

This "expect" statement kind of makes the following "expect" statement redundant. Is one of them more important than the other? Do we really need both?

pdmnyberg (Migrated from github.com) reviewed

2026-09-08 09:31:14 +00:00

frontend/src/app/lib/serverToken.test.ts

					
				@ -0,0 +1,400 @@

				import { beforeEach, describe, expect, it, vi } from "vitest";

My comment applies to a number of other tests too that expects to be using __Secure- when encountering https. Most of the cases seem to also refer to this as production. So in order to be consistent I think the adjustment needs to be applied to the other tests too.

My comment applies to a number of other tests too that expects to be using `__Secure-` when encountering https. Most of the cases seem to also refer to this as production. So in order to be consistent I think the adjustment needs to be applied to the other tests too.

pdmnyberg commented

2026-09-08 09:32:48 +00:00

(Migrated from github.com)

@aaperis I think this looks good. I haven't tested this again since I expect there not to be any significant changes since last time I looked at it. Please have a look at my follow up for the adjust to the test and my new question regarding the crypt4ghKey test. Other than that I am ready to approve this.

Great work!

@aaperis I think this looks good. I haven't tested this again since I expect there not to be any significant changes since last time I looked at it. Please have a look at my follow up for the adjust to the test and my new question regarding the crypt4ghKey test. Other than that I am ready to approve this. Great work!

👍 1

aaperis (Migrated from github.com) reviewed

2026-09-08 12:15:54 +00:00

frontend/src/app/lib/serverToken.test.ts

					
				@ -0,0 +1,400 @@

				import { beforeEach, describe, expect, it, vi } from "vitest";

Good point, now I hope I fixed this in all places needed.

aaperis (Migrated from github.com) reviewed

2026-09-08 12:42:46 +00:00

frontend/src/app/actions/crypt4ghKey.test.ts

Yes, actually the 2nd expect statement is a guard for the EqualTo of the first expect but this is probably too paranoid. I'll take it away.

Yes, actually the 2nd `expect` statement is a guard for the `EqualTo` of the first `expect` but this is probably too paranoid. I'll take it away.

aaperis commented

2026-09-08 12:46:12 +00:00

(Migrated from github.com)

@aaperis I think this looks good. I haven't tested this again since I expect there not to be any significant changes since last time I looked at it. Please have a look at my follow up for the adjust to the test and my new question regarding the crypt4ghKey test. Other than that I am ready to approve this.

Great work!

Thanks @pdmnyberg for the review and nice comments! I think I have made the changes needed. Please have another look.

> @aaperis I think this looks good. I haven't tested this again since I expect there not to be any significant changes since last time I looked at it. Please have a look at my follow up for the adjust to the test and my new question regarding the crypt4ghKey test. Other than that I am ready to approve this. > > Great work! Thanks @pdmnyberg for the review and nice comments! I think I have made the changes needed. Please have another look.

pdmnyberg (Migrated from github.com) reviewed

2026-09-08 12:55:30 +00:00

frontend/src/app/actions/crypt4ghKey.test.ts

If you wanted too keep both I guess you could also loosen the first expect by only checking expect(data.errors).toEqual...

That way you could test the error while leaving the object open for explicit checks for SECRET_INTERNAL_DETAIL. But the current implementation will make the test fail either way if we have some data leaks.

Anyways. This looks good now.

If you wanted too keep both I guess you could also loosen the first expect by only checking `expect(data.errors).toEqual...` That way you could test the error while leaving the object open for explicit checks for `SECRET_INTERNAL_DETAIL`. But the current implementation will make the test fail either way if we have some data leaks. Anyways. This looks good now.