diff --git a/src/index.ts b/src/index.ts index e122787..2f44548 100644 --- a/src/index.ts +++ b/src/index.ts @@ -82,14 +82,23 @@ export async function handleApplication(scope: Scope): Promise { async function updateConfiguration() { const rawOptions = (scope.options.getAll() || {}) as OAuthPluginConfig; - // Expand environment variables in plugin-level options - let debugValue = rawOptions.debug; - if (typeof debugValue === 'string' && debugValue.startsWith('${') && debugValue.endsWith('}')) { - const envVar = debugValue.slice(2, -1); - debugValue = process.env[envVar] || debugValue; + // Expand environment variables in plugin-level options (lazily) + const options: OAuthPluginConfig = { ...rawOptions }; + + // Create lazy getters for all string values that look like env vars + for (const [key, value] of Object.entries(rawOptions)) { + if (typeof value === 'string' && value.startsWith('${') && value.endsWith('}')) { + const envVar = value.slice(2, -1); + Object.defineProperty(options, key, { + get() { + const envValue = process.env[envVar]; + return envValue !== undefined && envValue !== '' ? envValue : value; + }, + enumerable: true, + configurable: true, + }); + } } - - const options = { ...rawOptions, debug: debugValue }; const previousDebugMode = debugMode; // Handle both boolean and string values (from environment variables) debugMode = options.debug === true || options.debug === 'true'; @@ -98,43 +107,22 @@ export async function handleApplication(scope: Scope): Promise { if (isInitialized) { logger?.info?.('OAuth configuration changed, updating providers...'); } else { - logger?.info?.('OAuth plugin loading with options:', JSON.stringify(options, null, 2)); + // Log raw options (don't stringify to avoid triggering lazy getters prematurely) + logger?.info?.('OAuth plugin loading...'); isInitialized = true; } // Re-initialize providers from new configuration providers = initializeProviders(options, logger); - // Update the resource with new providers + // Always register the OAuth resource with lazy initialization support + // Even if no providers are initially configured, they may become available later + scope.resources.set('oauth', createOAuthResource(providers, options, debugMode, hookManager, logger)); + + // Log configured providers if (Object.keys(providers).length === 0) { - // No valid providers configured - scope.resources.set('oauth', { - async get() { - return { - status: 503, - body: { - error: 'No valid OAuth providers configured', - message: 'Please check your OAuth configuration', - example: options.providers - ? 'Check that all required fields are provided' - : { - providers: { - github: { - provider: 'github', - clientId: '${OAUTH_GITHUB_CLIENT_ID}', - clientSecret: '${OAUTH_GITHUB_CLIENT_SECRET}', - }, - }, - }, - }, - }; - }, - }); + logger?.warn?.('No OAuth providers configured yet - will attempt lazy initialization on first request'); } else { - // Register the OAuth resource with configured providers - scope.resources.set('oauth', createOAuthResource(providers, debugMode, hookManager, logger)); - - // Log all configured providers logger?.info?.('OAuth plugin ready:', { providers: Object.entries(providers).map(([name, data]) => ({ name, @@ -160,7 +148,17 @@ export async function handleApplication(scope: Scope): Promise { // Get the provider for this OAuth session const providerName = request.session.oauth.provider; - const providerData = providers[providerName]; + let providerData = providers[providerName]; + + // Try lazy initialization if provider not found (handles race condition with env var loading) + if (!providerData) { + const rawOptions = (scope.options.getAll() || {}) as OAuthPluginConfig; + const { getOrInitializeProvider } = await import('./lib/config.ts'); + const initialized = getOrInitializeProvider(providerName, providers, rawOptions, logger); + if (initialized) { + providerData = initialized; + } + } if (!providerData) { logger?.warn?.(`OAuth provider '${providerName}' not found, logging out user`); diff --git a/src/lib/config.ts b/src/lib/config.ts index cbb2bb4..430c7a9 100644 --- a/src/lib/config.ts +++ b/src/lib/config.ts @@ -6,7 +6,18 @@ import { OAuthProvider } from './OAuthProvider.ts'; import { getProvider } from './providers/index.ts'; -import type { OAuthProviderConfig, OAuthPluginConfig, ProviderRegistry, Logger } from '../types.ts'; +import type { + OAuthProviderConfig, + OAuthPluginConfig, + ProviderRegistry, + ProviderRegistryEntry, + Logger, +} from '../types.ts'; + +/** + * Required fields for OAuth provider configuration + */ +const REQUIRED_PROVIDER_FIELDS = ['clientId', 'clientSecret', 'authorizationUrl', 'tokenUrl', 'userInfoUrl'] as const; /** * Build configuration for a specific provider @@ -18,13 +29,24 @@ export function buildProviderConfig( ): OAuthProviderConfig { const options = providerConfig || {}; - // Expand environment variables in config values + // Create a proxy that lazily expands environment variables when accessed + // This solves race conditions where env vars are loaded after plugin initialization const expandedOptions: Record = {}; for (const [key, value] of Object.entries(options)) { if (typeof value === 'string' && value.startsWith('${') && value.endsWith('}')) { // Extract environment variable name const envVar = value.slice(2, -1); - expandedOptions[key] = process.env[envVar] || value; + + // Use getter for lazy evaluation + Object.defineProperty(expandedOptions, key, { + get() { + const envValue = process.env[envVar]; + // Only use the env value if it's defined and not empty + return envValue !== undefined && envValue !== '' ? envValue : value; + }, + enumerable: true, + configurable: true, + }); } else { expandedOptions[key] = value; } @@ -60,13 +82,18 @@ export function buildProviderConfig( // Provider preset (if available) ...providerPreset, + }; - // Provider-specific options (with expanded env vars) - ...expandedOptions, + // Manually copy expandedOptions to preserve getters (spread operator would invoke them) + for (const key of Object.keys(expandedOptions)) { + const descriptor = Object.getOwnPropertyDescriptor(expandedOptions, key); + if (descriptor) { + Object.defineProperty(config, key, descriptor); + } + } - // Ensure redirect URI includes provider name (override any previous value) - redirectUri, - }; + // Ensure redirect URI includes provider name (override any previous value) + config.redirectUri = redirectUri; // Handle provider-specific configuration if (providerPreset?.configure) { @@ -129,8 +156,12 @@ export function initializeProviders(options: OAuthPluginConfig, logger?: Logger) const config = buildProviderConfig(providerConfig, providerName, pluginDefaults); // Check if this provider is properly configured - const requiredFields = ['clientId', 'clientSecret', 'authorizationUrl', 'tokenUrl', 'userInfoUrl']; - const missingFields = requiredFields.filter((key) => !config[key as keyof OAuthProviderConfig]); + // Note: This triggers lazy env var expansion via getters + const missingFields = REQUIRED_PROVIDER_FIELDS.filter((key) => { + const value = config[key as keyof OAuthProviderConfig]; + // Consider env var placeholders as "missing" if they haven't been loaded yet + return !value || (typeof value === 'string' && value.startsWith('${') && value.endsWith('}')); + }); if (missingFields.length > 0) { logger?.warn?.(`OAuth provider '${providerName}' not configured. Missing: ${missingFields.join(', ')}`); @@ -148,3 +179,50 @@ export function initializeProviders(options: OAuthPluginConfig, logger?: Logger) return providers; } + +/** + * Get or lazily initialize a single provider by name + * This handles race conditions where env vars are loaded after initial plugin initialization + */ +export function getOrInitializeProvider( + providerName: string, + providers: ProviderRegistry, + options: OAuthPluginConfig, + logger?: Logger +): ProviderRegistryEntry | null { + // Check if provider already exists + if (providers[providerName]) { + return providers[providerName]; + } + + // Provider not initialized yet - try to initialize it now + if (!options.providers || !options.providers[providerName]) { + logger?.debug?.(`OAuth provider '${providerName}' not defined in configuration`); + return null; + } + + const pluginDefaults = extractPluginDefaults(options); + const config = buildProviderConfig(options.providers[providerName], providerName, pluginDefaults); + + // Check if this provider is properly configured (with current env vars) + const missingFields = REQUIRED_PROVIDER_FIELDS.filter((key) => { + const value = config[key as keyof OAuthProviderConfig]; + return !value || (typeof value === 'string' && value.startsWith('${') && value.endsWith('}')); + }); + + if (missingFields.length > 0) { + logger?.debug?.(`OAuth provider '${providerName}' cannot be initialized yet. Missing: ${missingFields.join(', ')}`); + return null; + } + + // Initialize the provider + try { + const provider = new OAuthProvider(config, logger); + providers[providerName] = { provider, config }; + logger?.info?.(`OAuth provider '${providerName}' initialized on-demand (${config.provider})`); + return providers[providerName]; + } catch (error) { + logger?.error?.(`Failed to initialize OAuth provider '${providerName}':`, error); + return null; + } +} diff --git a/src/lib/resource.ts b/src/lib/resource.ts index 58a39ae..8e02398 100644 --- a/src/lib/resource.ts +++ b/src/lib/resource.ts @@ -4,9 +4,10 @@ * Harper resource class for handling OAuth REST endpoints */ -import type { Request, RequestTarget, Logger, ProviderRegistry } from '../types.ts'; +import type { Request, RequestTarget, Logger, ProviderRegistry, OAuthPluginConfig } from '../types.ts'; import { handleLogin, handleCallback, handleLogout, handleUserInfo, handleTestPage } from './handlers.ts'; import { validateAndRefreshSession } from './sessionValidator.ts'; +import { getOrInitializeProvider } from './config.ts'; import type { HookManager } from './hookManager.ts'; /** @@ -15,6 +16,7 @@ import type { HookManager } from './hookManager.ts'; */ export function createOAuthResource( providers: ProviderRegistry, + options: OAuthPluginConfig, debugMode: boolean, hookManager: HookManager, logger?: Logger @@ -67,8 +69,16 @@ export function createOAuthResource( }; } - // Check if provider exists - const providerData = providers[providerName]; + // Check if provider exists, or try to initialize it lazily + let providerData = providers[providerName]; + if (!providerData) { + // Try lazy initialization (handles race condition with env var loading) + const initialized = getOrInitializeProvider(providerName, providers, options, logger); + if (initialized) { + providerData = initialized; + } + } + if (!providerData) { return { status: 404, diff --git a/test/config.test.js b/test/config.test.js new file mode 100644 index 0000000..f9068eb --- /dev/null +++ b/test/config.test.js @@ -0,0 +1,306 @@ +/** + * Tests for OAuth configuration and lazy initialization + */ + +import { describe, it, before, after, mock } from 'node:test'; +import assert from 'node:assert'; +import { buildProviderConfig, getOrInitializeProvider, initializeProviders } from '../dist/lib/config.js'; + +describe('Lazy Environment Variable Expansion', () => { + const originalEnv = { ...process.env }; + + after(() => { + // Restore original env vars + process.env = originalEnv; + }); + + it('should expand env vars when accessed, not at config build time', () => { + // Start without env var + delete process.env.TEST_CLIENT_ID; + + const providerConfig = { + provider: 'github', + clientId: '${TEST_CLIENT_ID}', + clientSecret: 'secret123', + }; + + const config = buildProviderConfig(providerConfig, 'github'); + + // At this point, env var isn't set - config should have placeholder + assert.strictEqual(config.clientId, '${TEST_CLIENT_ID}'); + + // Now set the env var + process.env.TEST_CLIENT_ID = 'actual-client-id'; + + // Access the value again - should now return the env var value + assert.strictEqual(config.clientId, 'actual-client-id'); + }); + + it('should return placeholder if env var is empty string', () => { + process.env.TEST_CLIENT_ID = ''; + + const providerConfig = { + clientId: '${TEST_CLIENT_ID}', + }; + + const config = buildProviderConfig(providerConfig, 'github'); + + // Empty string should be treated as "not set" + assert.strictEqual(config.clientId, '${TEST_CLIENT_ID}'); + }); + + it('should handle non-env-var values normally', () => { + const providerConfig = { + clientId: 'hardcoded-id', + scope: 'openid profile', + }; + + const config = buildProviderConfig(providerConfig, 'github'); + + assert.strictEqual(config.clientId, 'hardcoded-id'); + assert.strictEqual(config.scope, 'openid profile'); + }); + + it('should handle multiple env vars in same config', () => { + delete process.env.TEST_CLIENT_ID; + delete process.env.TEST_CLIENT_SECRET; + + const providerConfig = { + clientId: '${TEST_CLIENT_ID}', + clientSecret: '${TEST_CLIENT_SECRET}', + }; + + const config = buildProviderConfig(providerConfig, 'github'); + + // Both should have placeholders + assert.strictEqual(config.clientId, '${TEST_CLIENT_ID}'); + assert.strictEqual(config.clientSecret, '${TEST_CLIENT_SECRET}'); + + // Set one env var + process.env.TEST_CLIENT_ID = 'id-123'; + + // Only the set one should expand + assert.strictEqual(config.clientId, 'id-123'); + assert.strictEqual(config.clientSecret, '${TEST_CLIENT_SECRET}'); + + // Set the other + process.env.TEST_CLIENT_SECRET = 'secret-456'; + + // Both should now expand + assert.strictEqual(config.clientId, 'id-123'); + assert.strictEqual(config.clientSecret, 'secret-456'); + }); +}); + +describe('getOrInitializeProvider', () => { + const originalEnv = { ...process.env }; + let mockLogger; + + before(() => { + mockLogger = { + debug: mock.fn(), + info: mock.fn(), + warn: mock.fn(), + error: mock.fn(), + }; + }); + + after(() => { + process.env = originalEnv; + }); + + it('should return existing provider if already initialized', () => { + const mockProvider = { provider: {}, config: { provider: 'github' } }; + const providers = { + github: mockProvider, + }; + + const options = { + providers: { + github: { + clientId: 'id', + clientSecret: 'secret', + }, + }, + }; + + const result = getOrInitializeProvider('github', providers, options, mockLogger); + + assert.strictEqual(result, mockProvider); + assert.strictEqual(mockLogger.info.mock.calls.length, 0); // Should not log initialization + }); + + it('should return null if provider not defined in config', () => { + const providers = {}; + const options = { + providers: { + github: {}, + }, + }; + + const result = getOrInitializeProvider('gitlab', providers, options, mockLogger); + + assert.strictEqual(result, null); + }); + + it('should return null if required fields still have placeholders', () => { + delete process.env.TEST_CLIENT_ID; + + const providers = {}; + const options = { + providers: { + github: { + provider: 'github', + clientId: '${TEST_CLIENT_ID}', + clientSecret: 'secret', + }, + }, + }; + + const result = getOrInitializeProvider('github', providers, options, mockLogger); + + assert.strictEqual(result, null); + assert.ok(mockLogger.debug.mock.calls.length > 0); // Should log debug message about missing fields + }); + + it('should initialize provider when env vars become available', () => { + // Start without env var + delete process.env.TEST_CLIENT_ID; + delete process.env.TEST_CLIENT_SECRET; + + const providers = {}; + const options = { + providers: { + github: { + provider: 'github', + clientId: '${TEST_CLIENT_ID}', + clientSecret: '${TEST_CLIENT_SECRET}', + }, + }, + }; + + // First attempt - should fail + let result = getOrInitializeProvider('github', providers, options, mockLogger); + assert.strictEqual(result, null); + + // Now set env vars + process.env.TEST_CLIENT_ID = 'test-id'; + process.env.TEST_CLIENT_SECRET = 'test-secret'; + + // Second attempt - should succeed + result = getOrInitializeProvider('github', providers, options, mockLogger); + + assert.ok(result !== null); + assert.ok(result.provider); + assert.strictEqual(result.config.provider, 'github'); + assert.ok(mockLogger.info.mock.calls.length > 0); // Should log initialization + }); + + it('should add initialized provider to registry', () => { + process.env.TEST_CLIENT_ID = 'test-id'; + process.env.TEST_CLIENT_SECRET = 'test-secret'; + + const providers = {}; + const options = { + providers: { + github: { + provider: 'github', + clientId: '${TEST_CLIENT_ID}', + clientSecret: '${TEST_CLIENT_SECRET}', + }, + }, + }; + + const result = getOrInitializeProvider('github', providers, options, mockLogger); + + // Should be added to providers registry + assert.ok(providers.github); + assert.strictEqual(providers.github, result); + }); +}); + +describe('initializeProviders', () => { + const originalEnv = { ...process.env }; + let mockLogger; + + before(() => { + mockLogger = { + debug: mock.fn(), + info: mock.fn(), + warn: mock.fn(), + error: mock.fn(), + }; + }); + + after(() => { + process.env = originalEnv; + }); + + it('should skip providers with missing env vars', () => { + delete process.env.TEST_CLIENT_ID; + + const options = { + providers: { + github: { + provider: 'github', + clientId: '${TEST_CLIENT_ID}', + clientSecret: 'secret', + }, + }, + }; + + const providers = initializeProviders(options, mockLogger); + + assert.strictEqual(Object.keys(providers).length, 0); + assert.ok(mockLogger.warn.mock.calls.length > 0); // Should warn about missing fields + }); + + it('should initialize providers when env vars are available', () => { + process.env.TEST_CLIENT_ID = 'test-id'; + process.env.TEST_CLIENT_SECRET = 'test-secret'; + + const options = { + providers: { + github: { + provider: 'github', + clientId: '${TEST_CLIENT_ID}', + clientSecret: '${TEST_CLIENT_SECRET}', + }, + }, + }; + + const providers = initializeProviders(options, mockLogger); + + assert.strictEqual(Object.keys(providers).length, 1); + assert.ok(providers.github); + assert.strictEqual(providers.github.config.provider, 'github'); + }); + + it('should handle mix of ready and not-ready providers', () => { + process.env.GITHUB_CLIENT_ID = 'github-id'; + process.env.GITHUB_CLIENT_SECRET = 'github-secret'; + delete process.env.GOOGLE_CLIENT_ID; + + const options = { + providers: { + github: { + provider: 'github', + clientId: '${GITHUB_CLIENT_ID}', + clientSecret: '${GITHUB_CLIENT_SECRET}', + }, + google: { + provider: 'google', + clientId: '${GOOGLE_CLIENT_ID}', + clientSecret: 'secret', + }, + }, + }; + + const providers = initializeProviders(options, mockLogger); + + // Only github should be initialized + assert.strictEqual(Object.keys(providers).length, 1); + assert.ok(providers.github); + assert.strictEqual(providers.google, undefined); + }); +}); diff --git a/test/lib/resource.test.js b/test/lib/resource.test.js index 41b72ff..21f10c4 100644 --- a/test/lib/resource.test.js +++ b/test/lib/resource.test.js @@ -56,12 +56,26 @@ describe('OAuth Resource', () => { }; }); + // Mock options object for createOAuthResource + const mockOptions = { + providers: { + github: { + provider: 'github', + clientId: 'github-client', + }, + google: { + provider: 'google', + clientId: 'google-client', + }, + }, + }; + describe('createOAuthResource', () => { describe('Debug Mode OFF (Production)', () => { let resource; beforeEach(() => { - resource = createOAuthResource(mockProviders, false, mockHookManager, mockLogger); + resource = createOAuthResource(mockProviders, mockOptions, false, mockHookManager, mockLogger); }); it('should return 404 for root path', async () => { @@ -133,7 +147,7 @@ describe('OAuth Resource', () => { let resource; beforeEach(() => { - resource = createOAuthResource(mockProviders, true, mockHookManager, mockLogger); + resource = createOAuthResource(mockProviders, mockOptions, true, mockHookManager, mockLogger); }); it('should show provider list at root', async () => { @@ -195,7 +209,7 @@ describe('OAuth Resource', () => { let resource; beforeEach(() => { - resource = createOAuthResource(mockProviders, false, mockHookManager, mockLogger); + resource = createOAuthResource(mockProviders, mockOptions, false, mockHookManager, mockLogger); }); it('should handle string target', async () => { @@ -255,7 +269,7 @@ describe('OAuth Resource', () => { let resource; beforeEach(() => { - resource = createOAuthResource(mockProviders, false, mockHookManager, mockLogger); + resource = createOAuthResource(mockProviders, mockOptions, false, mockHookManager, mockLogger); }); it('should handle logout POST request', async () => { @@ -286,15 +300,15 @@ describe('OAuth Resource', () => { describe('Resource Creation', () => { it('should create resource with providers', () => { - const resource = createOAuthResource(mockProviders, false, mockHookManager, mockLogger); + const resource = createOAuthResource(mockProviders, mockOptions, false, mockHookManager, mockLogger); assert.ok(resource); assert.equal(typeof resource.get, 'function'); assert.equal(typeof resource.post, 'function'); }); it('should create different instances with different configs', () => { - const resource1 = createOAuthResource(mockProviders, false, mockHookManager, mockLogger); - const resource2 = createOAuthResource(mockProviders, true, mockHookManager, mockLogger); + const resource1 = createOAuthResource(mockProviders, mockOptions, false, mockHookManager, mockLogger); + const resource2 = createOAuthResource(mockProviders, mockOptions, true, mockHookManager, mockLogger); // Each resource is a new object assert.notEqual(resource1, resource2); });