fix(sso): harden SSO with role sync, security fixes, design compliance, and test coverage (#564)

Rate-limit OIDC callback route and clear state cookie on all paths.
Validate GitHub API responses and add server-side config validation.
Sync SSO user roles on every login respecting seat limits.
Add LDAP connection timeout, bind DN warning, and multiple entry logging.
Replace Select with Combobox and apply card design tokens in SSOSection.
Expose OIDC scopes field in settings UI.
Fix hardcoded colors to use design system tokens.
Add standard and diagnostic logging throughout SSO flow.
Add tests for role sync, seat limits, LDAP escaping, and config validation.
Remove unused login-form.tsx template.
Update docs and screenshots for SSO feature.

backend/src/__tests__/sso.test.ts

@@ -268,3 +268,191 @@ describe('Database migration - SSO columns', () => {
     expect(byProvider!.username).toBe('sso_migration_test');
   });
 });
+
+describe('SSO Role Sync on Re-Login', () => {
+  beforeAll(async () => {
+    const { LicenseService } = await import('../services/LicenseService');
+    vi.spyOn(LicenseService.getInstance(), 'getSeatLimits').mockReturnValue({ maxAdmins: null, maxViewers: null });
+  });
+
+  afterAll(() => {
+    vi.restoreAllMocks();
+  });
+
+  it('provisionUser promotes user when IdP role changes to admin', async () => {
+    const { SSOService } = await import('../services/SSOService');
+    const sso = SSOService.getInstance();
+
+    // Create a viewer
+    const user1 = sso.provisionUser({
+      authProvider: 'oidc_okta',
+      providerId: 'okta-role-sync-test',
+      preferredUsername: 'rolesync_user',
+      email: 'rolesync@example.com',
+      role: 'viewer',
+    });
+    expect(user1.role).toBe('viewer');
+
+    // Re-login with admin role from IdP
+    const user2 = sso.provisionUser({
+      authProvider: 'oidc_okta',
+      providerId: 'okta-role-sync-test',
+      preferredUsername: 'rolesync_user',
+      email: 'rolesync@example.com',
+      role: 'admin',
+    });
+    expect(user2.id).toBe(user1.id);
+    expect(user2.role).toBe('admin');
+  });
+
+  it('provisionUser demotes user when IdP role changes to viewer', async () => {
+    const { SSOService } = await import('../services/SSOService');
+    const { DatabaseService } = await import('../services/DatabaseService');
+    const sso = SSOService.getInstance();
+    const db = DatabaseService.getInstance();
+
+    // Look up user from previous test (should be admin now)
+    const existing = db.getUserByProviderIdentity('oidc_okta', 'okta-role-sync-test');
+    expect(existing).toBeDefined();
+    expect(existing!.role).toBe('admin');
+
+    // Re-login with viewer role (e.g., removed from admin group)
+    const user = sso.provisionUser({
+      authProvider: 'oidc_okta',
+      providerId: 'okta-role-sync-test',
+      preferredUsername: 'rolesync_user',
+      email: 'rolesync@example.com',
+      role: 'viewer',
+    });
+    expect(user.role).toBe('viewer');
+  });
+});
+
+describe('SSO Seat Limit Enforcement', () => {
+  it('downgrades new admin to viewer when admin seats are full', async () => {
+    const { SSOService } = await import('../services/SSOService');
+    const { LicenseService } = await import('../services/LicenseService');
+
+    // Mock: 1 admin seat max (already used by testadmin)
+    vi.spyOn(LicenseService.getInstance(), 'getSeatLimits').mockReturnValue({ maxAdmins: 1, maxViewers: null });
+
+    const sso = SSOService.getInstance();
+    const user = sso.provisionUser({
+      authProvider: 'oidc_google',
+      providerId: 'seat-limit-admin-test',
+      preferredUsername: 'seatlimit_admin',
+      role: 'admin',
+    });
+
+    // Should be downgraded to viewer since admin seat is taken
+    expect(user.role).toBe('viewer');
+
+    vi.restoreAllMocks();
+  });
+
+  it('throws when all viewer seats are full', async () => {
+    const { SSOService } = await import('../services/SSOService');
+    const { LicenseService } = await import('../services/LicenseService');
+    const { DatabaseService } = await import('../services/DatabaseService');
+    const db = DatabaseService.getInstance();
+
+    // Count existing viewers to set a tight limit
+    const currentViewers = db.getViewerCount();
+    vi.spyOn(LicenseService.getInstance(), 'getSeatLimits').mockReturnValue({ maxAdmins: 1, maxViewers: currentViewers });
+
+    const sso = SSOService.getInstance();
+    expect(() => sso.provisionUser({
+      authProvider: 'oidc_google',
+      providerId: 'seat-limit-viewer-test',
+      preferredUsername: 'seatlimit_viewer',
+      role: 'viewer',
+    })).toThrow('User seat limit reached');
+
+    vi.restoreAllMocks();
+  });
+});
+
+describe('LDAP Filter Escaping', () => {
+  it('escapes special characters in LDAP filters', async () => {
+    const { SSOService } = await import('../services/SSOService');
+    const sso = SSOService.getInstance();
+    // Access private method via bracket notation for testing
+    const escape = (sso as unknown as { escapeLdapFilter: (v: string) => string }).escapeLdapFilter.bind(sso);
+
+    expect(escape('user*(admin)')).toBe('user\\2a\\28admin\\29');
+    expect(escape('test\\value')).toBe('test\\5cvalue');
+    expect(escape('normal')).toBe('normal');
+    expect(escape('null\0byte')).toBe('null\\00byte');
+  });
+});
+
+describe('SSO Config Validation on PUT', () => {
+  // We need an Admiral-licensed admin token for these tests.
+  // Mock getTier/getVariant so requireAdmiral passes.
+  let admiralToken: string;
+
+  beforeAll(async () => {
+    const { LicenseService } = await import('../services/LicenseService');
+    vi.spyOn(LicenseService.getInstance(), 'getTier').mockReturnValue('paid');
+    vi.spyOn(LicenseService.getInstance(), 'getVariant').mockReturnValue('admiral');
+    admiralToken = jwt.sign({ username: 'testadmin', role: 'admin' }, TEST_JWT_SECRET, { expiresIn: '1h' });
+  });
+
+  afterAll(() => {
+    vi.restoreAllMocks();
+  });
+
+  it('rejects enabled LDAP config without Server URL', async () => {
+    const res = await supertest(app)
+      .put('/api/sso/config/ldap')
+      .set('Authorization', `Bearer ${admiralToken}`)
+      .send({ enabled: true, ldapSearchBase: 'ou=users,dc=example' });
+    expect(res.status).toBe(400);
+    expect(res.body.error).toContain('Server URL');
+  });
+
+  it('rejects enabled LDAP config without Search Base', async () => {
+    const res = await supertest(app)
+      .put('/api/sso/config/ldap')
+      .set('Authorization', `Bearer ${admiralToken}`)
+      .send({ enabled: true, ldapUrl: 'ldap://localhost:389' });
+    expect(res.status).toBe(400);
+    expect(res.body.error).toContain('Search Base');
+  });
+
+  it('rejects enabled OIDC config without Client ID', async () => {
+    const res = await supertest(app)
+      .put('/api/sso/config/oidc_google')
+      .set('Authorization', `Bearer ${admiralToken}`)
+      .send({ enabled: true });
+    expect(res.status).toBe(400);
+    expect(res.body.error).toContain('Client ID');
+  });
+
+  it('rejects enabled Okta config without Issuer URL', async () => {
+    const res = await supertest(app)
+      .put('/api/sso/config/oidc_okta')
+      .set('Authorization', `Bearer ${admiralToken}`)
+      .send({ enabled: true, oidcClientId: 'test-client-id' });
+    expect(res.status).toBe(400);
+    expect(res.body.error).toContain('Issuer URL');
+  });
+
+  it('allows saving disabled config without required fields', async () => {
+    const res = await supertest(app)
+      .put('/api/sso/config/ldap')
+      .set('Authorization', `Bearer ${admiralToken}`)
+      .send({ enabled: false });
+    expect(res.status).toBe(200);
+    expect(res.body.success).toBe(true);
+  });
+
+  it('rejects invalid provider name', async () => {
+    const res = await supertest(app)
+      .put('/api/sso/config/invalid_provider')
+      .set('Authorization', `Bearer ${admiralToken}`)
+      .send({ enabled: true });
+    expect(res.status).toBe(400);
+    expect(res.body.error).toContain('Invalid SSO provider');
+  });
+});

backend/src/index.ts

@@ -800,6 +800,7 @@ app.post('/api/auth/sso/ldap', authRateLimiter, async (req: Request, res: Respon
     // Issue JWT (same as local login)
     const settings = DatabaseService.getInstance().getGlobalSettings();
     issueSessionCookie(res, req, user, settings.auth_jwt_secret);
+    console.log(`[SSO] LDAP login successful: ${user.username}`);
     res.json({ success: true, message: 'Login successful' });
   } catch (error: unknown) {
     const msg = error instanceof Error ? error.message : 'LDAP login failed';
@@ -842,7 +843,7 @@ app.get('/api/auth/sso/oidc/:provider/authorize', ssoRateLimiter, async (req: Re
 });
 
 // OIDC: Callback from identity provider
-app.get('/api/auth/sso/oidc/:provider/callback', async (req: Request, res: Response): Promise<void> => {
+app.get('/api/auth/sso/oidc/:provider/callback', ssoRateLimiter, async (req: Request, res: Response): Promise<void> => {
   try {
     const provider = String(req.params.provider);
     const code = String(req.query.code || '');
@@ -862,6 +863,8 @@ app.get('/api/auth/sso/oidc/:provider/callback', async (req: Request, res: Respo
 
     // Read and validate state cookie
     const stateCookie = req.cookies?.sencho_sso_state;
+    // Always clear the one-time state cookie, regardless of outcome
+    res.clearCookie('sencho_sso_state', { httpOnly: true, secure: isSecureRequest(req), sameSite: 'lax' });
     if (!stateCookie) {
       res.redirect('/?sso_error=SSO+session+expired.+Please+try+again.');
       return;
@@ -878,7 +881,7 @@ app.get('/api/auth/sso/oidc/:provider/callback', async (req: Request, res: Respo
     }
 
     if (statePayload.provider !== provider) {
-      res.redirect('/?sso_error=Provider+mismatch');
+      res.redirect(`/?sso_error=${encodeURIComponent(`Provider mismatch: expected ${statePayload.provider}, got ${provider}`)}`);
       return;
     }
 
@@ -892,9 +895,6 @@ app.get('/api/auth/sso/oidc/:provider/callback', async (req: Request, res: Respo
       statePayload.codeVerifier
     );
 
-    // Clear state cookie
-    res.clearCookie('sencho_sso_state', { httpOnly: true, secure: isSecureRequest(req), sameSite: 'lax' });
-
     if (!result.success || !result.user) {
       res.redirect(`/?sso_error=${encodeURIComponent(result.error || 'Authentication failed')}`);
       return;
@@ -912,6 +912,7 @@ app.get('/api/auth/sso/oidc/:provider/callback', async (req: Request, res: Respo
     // Issue JWT + cookie (same as local login)
     const settings = DatabaseService.getInstance().getGlobalSettings();
     issueSessionCookie(res, req, user, settings.auth_jwt_secret);
+    console.log(`[SSO] OIDC login successful: ${user.username} via ${provider}`);
 
     res.redirect('/');
   } catch (error: unknown) {
@@ -4670,7 +4671,25 @@ app.put('/api/sso/config/:provider', (req: Request, res: Response): void => {
       return;
     }
     const config = { ...req.body, provider } as import('./services/SSOService').SSOProviderConfig;
+
+    // Validate required fields when enabling a provider
+    if (config.enabled) {
+      const missing: string[] = [];
+      if (provider === 'ldap') {
+        if (!config.ldapUrl?.trim()) missing.push('Server URL');
+        if (!config.ldapSearchBase?.trim()) missing.push('Search Base');
+      } else {
+        if (!config.oidcClientId?.trim()) missing.push('Client ID');
+        if (provider === 'oidc_okta' && !config.oidcIssuerUrl?.trim()) missing.push('Issuer URL');
+      }
+      if (missing.length > 0) {
+        res.status(400).json({ error: `Missing required fields: ${missing.join(', ')}` });
+        return;
+      }
+    }
+
     SSOService.getInstance().saveProviderConfig(config);
+    console.log(`[SSO] Config updated: ${provider} ${config.enabled ? 'enabled' : 'disabled'}`);
     res.json({ success: true, message: 'SSO configuration saved' });
   } catch (error) {
     console.error('[SSO] Failed to save SSO config:', error);
@@ -4686,7 +4705,9 @@ app.delete('/api/sso/config/:provider', (req: Request, res: Response): void => {
   if (!requireAdmin(req, res)) return;
   if (!requireAdmiral(req, res)) return;
   try {
-    SSOService.getInstance().deleteProviderConfig(String(req.params.provider));
+    const deletedProvider = String(req.params.provider);
+    SSOService.getInstance().deleteProviderConfig(deletedProvider);
+    console.log(`[SSO] Config deleted: ${deletedProvider}`);
     res.json({ success: true, message: 'SSO configuration deleted' });
   } catch (error) {
     console.error('[SSO] Failed to delete SSO config:', error);

backend/src/services/SSOService.ts

@@ -14,6 +14,7 @@ import { DatabaseService, User, AuthProvider } from './DatabaseService';
 import { CryptoService } from './CryptoService';
 import { LicenseService } from './LicenseService';
 import { CacheService } from './CacheService';
+import { isDebugEnabled } from '../utils/debug';
 
 // OIDC discovery metadata changes rarely; caching it eliminates the redundant
 // HTTPS round-trip between getOIDCAuthorizationUrl and handleOIDCCallback in
@@ -221,8 +222,12 @@ export class SSOService {
             return { success: false, error: 'LDAP configuration is incomplete' };
         }
 
+        const debug = isDebugEnabled();
+        if (debug) console.debug('[SSO:debug] LDAP auth attempt', { username, hasBindDn: !!config.ldapBindDn, url: config.ldapUrl });
+
         const client = new LdapClient({
             url: config.ldapUrl,
+            connectTimeout: 10000,
             tlsOptions: {
                 rejectUnauthorized: config.ldapTlsRejectUnauthorized !== false,
             },
@@ -232,6 +237,8 @@ export class SSOService {
             // Step 1: Bind with service account to search for the user
             if (config.ldapBindDn && config.ldapBindPassword) {
                 await client.bind(config.ldapBindDn, config.ldapBindPassword);
+            } else if (config.ldapBindDn && !config.ldapBindPassword) {
+                console.warn('[SSO] LDAP bind DN configured but bind password is empty; anonymous bind will be used');
             }
 
             // Step 2: Search for the user
@@ -246,13 +253,19 @@ export class SSOService {
                 return { success: false, error: 'Invalid credentials' };
             }
 
+            if (searchEntries.length > 1) {
+                console.warn(`[SSO] LDAP search returned ${searchEntries.length} entries for username "${username}"; using first match. Consider narrowing your search filter.`);
+            }
+
             const userEntry = searchEntries[0];
             const userDn = userEntry.dn;
+            if (debug) console.debug('[SSO:debug] LDAP user found', { dn: userDn, hasEmail: !!(userEntry['mail'] || userEntry['email']) });
 
             // Step 3: Bind as the user to verify their password
             await client.unbind();
             const userClient = new LdapClient({
                 url: config.ldapUrl,
+                connectTimeout: 10000,
                 tlsOptions: {
                     rejectUnauthorized: config.ldapTlsRejectUnauthorized !== false,
                 },
@@ -263,11 +276,12 @@ export class SSOService {
             } catch {
                 return { success: false, error: 'Invalid credentials' };
             } finally {
-                try { await userClient.unbind(); } catch { /* ignore */ }
+                try { await userClient.unbind(); } catch (e) { if (debug) console.debug('[SSO:debug] LDAP user unbind error (non-critical):', (e as Error).message); }
             }
 
             // Step 4: Determine role from group membership
             const role = this.resolveRoleFromLdap(userEntry, config);
+            if (debug) console.debug('[SSO:debug] LDAP role resolved', { username, role });
 
             // Extract user info
             const preferredUsername = String(
@@ -289,7 +303,7 @@ export class SSOService {
             console.error('[SSO] LDAP authentication error:', message);
             return { success: false, error: 'LDAP authentication failed. Check server connectivity.' };
         } finally {
-            try { await client.unbind(); } catch { /* ignore */ }
+            try { await client.unbind(); } catch (e) { if (debug) console.debug('[SSO:debug] LDAP service unbind error (non-critical):', (e as Error).message); }
         }
     }
 
@@ -343,6 +357,7 @@ export class SSOService {
         const codeChallenge = await calculatePKCECodeChallenge(codeVerifier);
 
         const scopes = config.oidcScopes || 'openid email profile';
+        if (isDebugEnabled()) console.debug('[SSO:debug] OIDC auth URL', { provider, scopes });
 
         const url = buildAuthorizationUrl(oidcConfig, {
             redirect_uri: callbackUrl,
@@ -407,8 +422,9 @@ export class SSOService {
             }
 
             const email = String(userInfo.email || '');
-            const name = String(userInfo.name || userInfo.preferred_username || userInfo.login || email.split('@')[0] || 'sso_user');
+            const name = String(userInfo.name || userInfo.preferred_username || userInfo.login || email.split('@')[0] || `sso_${sub.substring(0, 8)}`);
             const role = this.resolveRoleFromOidc(userInfo, config);
+            if (isDebugEnabled()) console.debug('[SSO:debug] OIDC userInfo resolved', { provider, sub, email: email || '(none)', name, role });
 
             return {
                 success: true,
@@ -436,6 +452,9 @@ export class SSOService {
             }),
         ]);
 
+        if (!userRes.ok) {
+            throw new Error(`GitHub user API returned ${userRes.status}`);
+        }
         const user = await userRes.json() as Record<string, unknown>;
         let primaryEmail = '';
         try {
@@ -514,13 +533,36 @@ export class SSOService {
         role: 'admin' | 'viewer';
     }): User {
         const db = DatabaseService.getInstance();
+        const debug = isDebugEnabled();
+        if (debug) console.debug('[SSO:debug] provisionUser', { authProvider: params.authProvider, providerId: params.providerId, preferredUsername: params.preferredUsername, role: params.role });
 
         // Check if user already exists by provider identity
         const existing = db.getUserByProviderIdentity(params.authProvider, params.providerId);
         if (existing) {
+            const updates: Partial<{ email: string; role: string }> = {};
+
             // Update email if changed
             if (params.email && params.email !== existing.email) {
-                db.updateUser(existing.id, { email: params.email });
+                updates.email = params.email;
+            }
+
+            // Sync role from identity provider on every login
+            if (params.role !== existing.role) {
+                if (params.role === 'admin') {
+                    const seatLimits = LicenseService.getInstance().getSeatLimits();
+                    if (seatLimits.maxAdmins === null || db.getAdminCount() < seatLimits.maxAdmins) {
+                        updates.role = params.role;
+                    }
+                    // If admin seats full, keep current role rather than upgrading
+                } else {
+                    // Always allow demotion (e.g., removed from admin group)
+                    updates.role = params.role;
+                }
+            }
+
+            if (Object.keys(updates).length > 0) {
+                if (debug) console.debug('[SSO:debug] Updating existing user', { userId: existing.id, username: existing.username, updates });
+                db.updateUser(existing.id, updates);
             }
             return db.getUser(existing.id) || existing;
         }
@@ -529,7 +571,8 @@ export class SSOService {
         let { role } = params;
         const seatLimits = LicenseService.getInstance().getSeatLimits();
         if (role === 'admin' && seatLimits.maxAdmins !== null && db.getAdminCount() >= seatLimits.maxAdmins) {
-            role = 'viewer'; // Downgrade to viewer if admin seats full
+            console.warn(`[SSO] Admin seat limit reached; provisioning ${params.preferredUsername} as viewer instead of admin`);
+            role = 'viewer';
         }
         if (role === 'viewer' && seatLimits.maxViewers !== null && db.getViewerCount() >= seatLimits.maxViewers) {
             throw new Error('User seat limit reached. Contact your administrator to increase your license.');
@@ -561,6 +604,7 @@ export class SSOService {
 
         const user = db.getUser(id);
         if (!user) throw new Error('Failed to create SSO user');
+        if (debug) console.debug('[SSO:debug] New user provisioned', { userId: user.id, username, role });
         return user;
     }
 
@@ -572,6 +616,7 @@ export class SSOService {
             return { success: false, error: 'LDAP not configured' };
         }
 
+        const debug = isDebugEnabled();
         const client = new LdapClient({
             url: config.ldapUrl,
             tlsOptions: { rejectUnauthorized: config.ldapTlsRejectUnauthorized !== false },
@@ -587,7 +632,7 @@ export class SSOService {
             const message = err instanceof Error ? err.message : 'Connection failed';
             return { success: false, error: message };
         } finally {
-            try { await client.unbind(); } catch { /* ignore */ }
+            try { await client.unbind(); } catch (e) { if (debug) console.debug('[SSO:debug] LDAP test unbind error (non-critical):', (e as Error).message); }
         }
     }