Skip to content

Extract authentication business logic and remove deprecated code#6

Merged
nicknisi merged 39 commits intomainfrom
nicknisi/toolkit
Dec 3, 2025
Merged

Extract authentication business logic and remove deprecated code#6
nicknisi merged 39 commits intomainfrom
nicknisi/toolkit

Conversation

@nicknisi
Copy link
Member

@nicknisi nicknisi commented Nov 19, 2025

Summary

Extract authentication business logic into reusable classes and remove deprecated factory pattern. Provides clean API for building framework-specific WorkOS authentication packages.

Key Changes

New API:

  • createAuthService({ sessionStorageFactory }) - Create auth instance
  • AuthService - Main API (withAuth, signOut, getSignInUrl, etc.)
  • Discriminated union for AuthResult - TypeScript-safe auth checks

Removed:

  • createAuthKitFactory() - Deprecated
  • SessionManager - Deprecated
  • ~500 lines of unused code

Internal architecture:

  • AuthKitCore - JWT verification, session encryption, refresh logic
  • AuthOperations - WorkOS API operations
  • Clean separation of concerns

Breaking Changes

1. Factory function renamed:

// Before
import { createAuthKitFactory } from '@workos/authkit-session';
const authKit = createAuthKitFactory({ sessionStorageFactory });

// After
import { createAuthService } from '@workos/authkit-session';
const authService = createAuthService({ sessionStorageFactory });

2. withAuth returns discriminated union:

// Before
const result = await authKit.withAuth(request);
if (result.user) {
  console.log(result.sessionId!); // Needed ! assertion
}

// After
const { auth, refreshedSessionData } = await authService.withAuth(request);
if (!auth.user) {
  return redirect('/login');
}
// TypeScript knows sessionId exists (no ! needed)
console.log(auth.sessionId);    // ✅ string
console.log(auth.accessToken);  // ✅ string

3. AuthResult type:

type AuthResult<T> = 
  | { user: null }
  | { user: User; sessionId: string; accessToken: string; claims: T; ... }

Testing

  • ✅ All 142 tests passing
  • ✅ TanStack Start builds successfully
  • ✅ 80%+ coverage maintained

Documentation

  • Updated README with correct API examples
  • Removed migration guide (no existing users)
  • Clear architecture explanation

…ended

- Emphasize toolkit-first: Core + Operations are primitives
- AuthService is 'one way' not 'the way' to orchestrate
- Update exports: Primitives first, orchestration second (optional)
- Remove prescriptive language ('recommended', 'advanced')
- Acknowledge AuthService is used by TanStack Start as proof it works
- Preserve toolkit philosophy: frameworks choose their orchestration
- Remove 'toolkit' language from all documentation
- Frame AuthService as public API, not 'one option'
- Label Core + Operations as 'internal layers' (advanced use only)
- Update package.json description to be accurate
- Reorder exports: AuthService first, internals last
- Acknowledge what this is: framework-agnostic library with adapter pattern
- Stop pretending Core + Operations are composable primitives
@nicknisi nicknisi changed the title Refactor to toolkit architecture Extract authentication business logic into framework-agnostic library Nov 20, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR successfully extracts WorkOS authentication business logic into a framework-agnostic library, replacing the deprecated factory pattern with a clean service-based architecture. The refactor introduces improved type safety through discriminated unions and establishes clear separation of concerns between public API (AuthService) and internal implementation (AuthKitCore, AuthOperations).

Key changes:

  • Introduced AuthService as the main public API with createAuthService() factory
  • Removed deprecated SessionManager and createAuthKitFactory() (~500 lines)
  • Enhanced type safety with discriminated union AuthResult type (eliminates optional chaining)
  • Updated dependencies (jose 6.1.2, vitest 4.0.10, typescript 5.9.3)

Reviewed Changes

Copilot reviewed 18 out of 19 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/service/factory.ts New factory function with lazy initialization for AuthService creation
src/service/AuthService.ts New main public API coordinating core logic, operations, and storage
src/operations/AuthOperations.ts New operations layer handling WorkOS API calls (signOut, refresh, URLs)
src/core/AuthKitCore.ts New core business logic layer (JWT, encryption, refresh orchestration)
src/core/session/types.ts Enhanced with discriminated union AuthResult type for better type safety
src/core/session/CookieSessionStorage.ts Added secure flag inference and SameSite capitalization for Safari
src/index.ts Reorganized exports with clear public API vs internal layers distinction
package.json Updated dependencies and version to 0.2.0-beta.0
README.md Complete rewrite explaining architecture, storage adapter pattern, and usage
pnpm-lock.yaml Dependency updates reflecting package.json changes
src/core/session/SessionManager.ts Removed deprecated SessionManager class
src/core/createAuthKitFactory.ts Removed deprecated factory pattern
Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

The 'toolkit' framing was fine. Overthinking the semantics added no value.
Keep the good parts from original update: philosophy, architecture, examples.
@nicknisi nicknisi changed the title Extract authentication business logic into framework-agnostic library Extract authentication business logic and remove deprecated code Nov 20, 2025
- replace vague "toolkit primitives" with "framework-agnostic authentication service"
- update "toolkit API" to "public API" for clarity
- add missing comment to empty catch block in token validation
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 18 out of 19 changed files in this pull request and generated 5 comments.

Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@nicknisi
Copy link
Member Author

@greptileai

@nicknisi
Copy link
Member Author

@greptileai

@greptile-apps
Copy link

greptile-apps bot commented Nov 24, 2025

Greptile Overview

Greptile Summary

This PR successfully extracts authentication business logic into a cleaner, more maintainable architecture with three distinct layers: AuthKitCore (pure business logic), AuthOperations (WorkOS API calls), and AuthService (framework orchestration). The refactoring removes ~500 lines of deprecated code (SessionManager, createAuthKitFactory) and introduces a discriminated union AuthResult type for improved TypeScript safety.

Key improvements:

  • Clean separation of concerns with clear responsibility boundaries
  • Lazy initialization pattern allows config to be set after instantiation
  • Type-safe authentication checks via discriminated unions eliminate need for null assertions
  • All 142 tests passing with 80%+ coverage maintained
  • Comprehensive documentation updates with accurate examples

Breaking changes:

  • Factory function renamed: createAuthKitFactorycreateAuthService
  • withAuth() now returns discriminated union instead of nullable fields
  • No existing users affected (prerelease software)

Security posture:

  • No sensitive data logging detected
  • Secure cookie defaults maintained (HttpOnly, Secure, SameSite=lax)
  • Session encryption and JWT verification properly implemented
  • Error handling swallows exceptions and returns safe defaults

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk - well-architected refactoring with comprehensive test coverage
  • Score reflects excellent code organization, thorough testing (142 tests passing), clean separation of concerns, proper security practices, and clear documentation. The refactoring simplifies the codebase while maintaining all functionality. No security issues or logical errors detected.
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
src/core/AuthKitCore.ts 5/5 New pure business logic layer for JWT verification, session encryption, and token refresh - well-structured with clear separation of concerns
src/operations/AuthOperations.ts 5/5 New high-level operations layer orchestrating WorkOS API calls for sign out, org switching, and authorization URLs
src/service/AuthService.ts 5/5 Main framework-agnostic authentication service with lazy initialization - coordinates core logic, operations, and storage
src/service/factory.ts 5/5 Factory function for creating AuthService with lazy initialization support and sensible defaults
src/index.ts 5/5 Updated exports reflecting new API - removed deprecated factory, added new createAuthService and core layers
src/core/session/types.ts 5/5 Updated type definitions with new discriminated union AuthResult for type-safe authentication checks

Sequence Diagram

sequenceDiagram
    participant App as Framework App
    participant AS as AuthService
    participant Storage as SessionStorage
    participant Core as AuthKitCore
    participant Ops as AuthOperations
    participant WorkOS as WorkOS API

    Note over App,WorkOS: Authentication Flow (withAuth)
    App->>AS: withAuth(request)
    AS->>Storage: getSession(request)
    Storage-->>AS: encryptedSession
    AS->>Core: decryptSession(encryptedSession)
    Core-->>AS: session (tokens + user)
    AS->>Core: validateAndRefresh(session)
    Core->>Core: verifyToken(accessToken)
    Core->>Core: isTokenExpiring(accessToken)
    alt Token valid and not expiring
        Core-->>AS: valid session, no refresh
    else Token invalid or expiring
        Core->>WorkOS: authenticateWithRefreshToken()
        WorkOS-->>Core: new tokens and user
        Core->>Core: encryptSession(newSession)
        Core-->>AS: refreshed session data
    end
    AS-->>App: AuthResult with optional refresh

    Note over App,WorkOS: OAuth Callback Flow
    App->>AS: handleCallback(request, response, options)
    AS->>WorkOS: authenticateWithCode(authCode)
    WorkOS-->>AS: tokens and user data
    AS->>Core: encryptSession(session)
    Core-->>AS: encryptedSession
    AS->>Storage: saveSession(response, encryptedSession)
    Storage-->>AS: updated response with cookie
    AS-->>App: response with return path

    Note over App,WorkOS: Sign Out Flow
    App->>AS: signOut(sessionId, options)
    AS->>Ops: signOut(sessionId, options)
    Ops->>WorkOS: getLogoutUrl(sessionId)
    WorkOS-->>Ops: logoutUrl
    Ops->>Ops: buildClearCookieHeader()
    Ops-->>AS: logout URL and clear cookie
    AS-->>App: logout data for redirect
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

18 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

Copy link

@cmatheson cmatheson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm still making my way through this, but posting my current set of comments now

const { accessToken } = session;
const isValid = await this.verifyToken(accessToken);
const isExpiring = this.isTokenExpiring(accessToken);
if (isValid && !isExpiring) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the point of this !isExpiring check?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proactive refresh. If the token expires within the buffer window, refresh it preemptively rather than waiting for it to actually expire.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think for server-side proactively refreshing is all downside? (you just refresh more often which is worse).

(we do want to proactively refresh for front-end consumers but i don't think this is used for that?).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you're right. I'll clean this up.


// Build cookie clear header
const cookieName = this.config.getValue('cookieName');
const clearCookieHeader = `${cookieName}=; Path=/; Max-Age=0; HttpOnly; Secure; SameSite=lax`;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not a blocker but i think for a general-purpose library, operating on cookie headers seems like the wrong level of abstraction (like suppose a framework exists that has a proper session abstraction, we wouldn't want to be setting/clearing cookies directly).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. I refactored AuthOperations to just getLogoutUrl which will just return the logout url. AuthService#signOut will now do the orchestration.

const clearResult = await this.storage.clearSession();
return { logoutUrl, ...clearResult };

That way, it leave it to the framework implementation to handle it however they need to.

if (!orgId) {
// Extract org from current token (decodeJwt works even on expired tokens)
try {
const claims = this.core.parseTokenClaims<BaseTokenClaims>(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems like this is basically just 100% duplication of the core logic? why do we need this function?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Refactored refreshSession() to delegate to validateAndRefresh(session, { force: true, organizationId }).

loginHint: options.loginHint,
prompt: options.prompt,
clientId: this.config.getValue('clientId'),
state: options.returnPathname
Copy link

@cmatheson cmatheson Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is probably a weakness of the existing apis, but we're stopping our customers from other meaningful uses of state when we set it to returnPathname like this.

also this would be a good place for us to be preventing login-csrf (this sdk should be generating a nonce and passing it in here and storing in a cookie)

(login-csrf prevention could be a separate PR, but we should fix the issue of customers not being able to set state now).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I matched how we handle state in authkit-nextjs.

  • Custom state passed as-is when no returnPathname
  • Combined as {internal}.{userState} when both provided
  • URL-safe base64 for internal state
  • handleCallback returns the custom state to caller

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll handle the login-csrf issue in a separate PR.

/**
* Sign out operation - delegates to AuthOperations.
*/
async signOut(sessionId: string, options?: { returnTo?: string }) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not clear to me that the different layers are all providing value. what's the point of all these functions that just call (an equally simple) implementation in the previous layer? could we just re-export? do we need AuthOperations?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The layers provide composition flexibility. My idea is that

  • AuthKitCore handles pure token logic (validation, refresh, encryption).
  • AuthOperations handles WorkOS API coordination (URLs, logout).
  • AuthService adds storage orchestration for frameworks without session primitives.

// Authenticate with WorkOS using the OAuth code
const authResponse = await client.userManagement.authenticateWithCode({
code: options.code,
clientId,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(for login-csrf prevention, this is where we would pull our nonce out of a cookie (or the session) and ensure that it matches the state param coming on the request.)

…rationProvider

AuthService lazy getters resolve config when first accessed, preserving
configure() flexibility while simplifying internal dependencies.
AuthService now takes resolved config, client, and encryption directly.
Factory returns a lazy proxy that defers AuthService creation until first use.
This keeps the same external API while simplifying internal dependencies.
- Add force/organizationId options to validateAndRefresh for org switching
- Simplify refreshSession to call validateAndRefresh({ force: true })
- Rename signOut to getLogoutUrl (just returns URL)
- Move session clearing orchestration to AuthService.signOut
- Add `state` option to GetAuthorizationUrlOptions
- Use URL-safe base64 for internal state (returnPathname)
- Combine as `internal.userState` when both provided
- Pass custom state as-is when no returnPathname (matches authkit-nextjs)
- Parse and return custom state in handleCallback result
Updated module docs to reflect that session encryption should be
handled by framework adapters, with authkit-session providing a
fallback. Exported sessionEncryption for frameworks to use.
Server-side requests are discrete - no benefit to refreshing valid
tokens "just in case". Only refresh when token is actually invalid
or force is true. Matches authkit-nextjs server behavior.

isTokenExpiring() kept for client-side frameworks that want proactive refresh.
@nicknisi nicknisi merged commit 9be02e9 into main Dec 3, 2025
4 checks passed
@nicknisi nicknisi mentioned this pull request Dec 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants