Skip to content

Commit fb8c386

Browse files
HuanCheng65HuanCheng65
committed
fix(tests): resolve OAuth test failures and type errors
- Fix cookie handling in e2e tests with proper array type checking - Update OAuth service test mocks to use correct environment variable names - Fix nullable field handling in OAuthUserInfo type definitions - Update test expectations to match actual OAuth service method signatures - Resolve private method access issues in users service tests
1 parent 2a68784 commit fb8c386

File tree

3 files changed

+121
-82
lines changed

3 files changed

+121
-82
lines changed

‎src/auth/oauth/oauth.service.spec.ts‎

Lines changed: 71 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -5,16 +5,16 @@
55
* Claude Assistant
66
*/
77

8-
import { Test, TestingModule } from '@nestjs/testing';
98
import { ConfigService } from '@nestjs/config';
9+
import { Test, TestingModule } from '@nestjs/testing';
10+
import * as fs from 'fs';
11+
import * as path from 'path';
1012
import { OAuthService } from './oauth.service';
1113
import {
1214
OAuthProvider,
1315
OAuthProviderConfig,
1416
OAuthUserInfo,
1517
} from './oauth.types';
16-
import * as fs from 'fs';
17-
import * as path from 'path';
1818

1919
// Mock provider for testing
2020
class MockOAuthProvider implements OAuthProvider {
@@ -72,17 +72,17 @@ describe('OAuthService', () => {
7272
useValue: {
7373
get: jest.fn((key: string) => {
7474
switch (key) {
75-
case 'oauth.enabledProviders':
76-
return ['test'];
77-
case 'oauth.pluginPaths':
78-
return ['./test-plugins'];
79-
case 'oauth.allowNpmLoading':
75+
case 'OAUTH_ENABLED_PROVIDERS':
76+
return 'test';
77+
case 'OAUTH_PLUGIN_PATHS':
78+
return './test-plugins';
79+
case 'OAUTH_ALLOW_NPM_LOADING':
8080
return false;
81-
case 'oauth.test.clientId':
81+
case 'OAUTH_TEST_CLIENT_ID':
8282
return 'test-client-id';
83-
case 'oauth.test.clientSecret':
83+
case 'OAUTH_TEST_CLIENT_SECRET':
8484
return 'test-client-secret';
85-
case 'oauth.test.redirectUrl':
85+
case 'OAUTH_TEST_REDIRECT_URL':
8686
return 'http://localhost:3000/callback';
8787
default:
8888
return undefined;
@@ -104,30 +104,32 @@ describe('OAuthService', () => {
104104
describe('initialize', () => {
105105
it('should initialize without providers when none are enabled', async () => {
106106
jest.spyOn(configService, 'get').mockImplementation((key: string) => {
107-
if (key === 'oauth.enabledProviders') return [];
107+
if (key === 'OAUTH_ENABLED_PROVIDERS') return '';
108108
return undefined;
109109
});
110110

111111
await service.initialize();
112-
expect(service.getAvailableProviders()).toEqual([]);
112+
expect(
113+
(await service.getAllProviders()).map((p) => p.getConfig().id),
114+
).toEqual([]);
113115
});
114116

115117
it('should skip provider loading when config is missing', async () => {
116118
jest.spyOn(configService, 'get').mockImplementation((key: string) => {
117-
if (key === 'oauth.enabledProviders') return ['test'];
118-
if (key === 'oauth.test.clientId') return undefined;
119+
if (key === 'OAUTH_ENABLED_PROVIDERS') return 'test';
120+
if (key === 'OAUTH_TEST_CLIENT_ID') return undefined;
119121
return undefined;
120122
});
121123

122124
const consoleSpy = jest.spyOn(console, 'warn').mockImplementation();
123125
await service.initialize();
124126

125127
expect(consoleSpy).toHaveBeenCalledWith(
126-
expect.stringContaining(
127-
'OAuth provider test configuration is incomplete',
128-
),
128+
expect.stringContaining('Missing configuration for OAuth provider'),
129129
);
130-
expect(service.getAvailableProviders()).toEqual([]);
130+
expect(
131+
(await service.getAllProviders()).map((p) => p.getConfig().id),
132+
).toEqual([]);
131133
});
132134

133135
it('should load provider from plugin file', async () => {
@@ -136,6 +138,8 @@ describe('OAuthService', () => {
136138

137139
const mockCreateProvider = jest.fn().mockReturnValue(
138140
new MockOAuthProvider({
141+
id: 'test',
142+
name: 'Test Provider',
139143
clientId: 'test-client-id',
140144
clientSecret: 'test-client-secret',
141145
redirectUrl: 'http://localhost:3000/callback',
@@ -155,11 +159,18 @@ describe('OAuthService', () => {
155159

156160
await service.initialize();
157161

158-
expect(service.getAvailableProviders()).toContain('test');
162+
expect(
163+
(await service.getAllProviders()).map((p) => p.getConfig().id),
164+
).toContain('test');
159165
expect(mockCreateProvider).toHaveBeenCalledWith({
166+
id: 'test',
167+
name: 'test',
160168
clientId: 'test-client-id',
161169
clientSecret: 'test-client-secret',
170+
authorizationUrl: '',
171+
tokenUrl: '',
162172
redirectUrl: 'http://localhost:3000/callback',
173+
scope: [],
163174
});
164175
});
165176

@@ -181,12 +192,14 @@ describe('OAuthService', () => {
181192
expect(consoleSpy).toHaveBeenCalledWith(
182193
expect.stringContaining('Failed to load OAuth provider test'),
183194
);
184-
expect(service.getAvailableProviders()).toEqual([]);
195+
expect(
196+
(await service.getAllProviders()).map((p) => p.getConfig().id),
197+
).toEqual([]);
185198
});
186199

187200
it('should validate provider ID format', async () => {
188201
jest.spyOn(configService, 'get').mockImplementation((key: string) => {
189-
if (key === 'oauth.enabledProviders') return ['invalid-provider!'];
202+
if (key === 'OAUTH_ENABLED_PROVIDERS') return 'invalid-provider!';
190203
return undefined;
191204
});
192205

@@ -196,7 +209,9 @@ describe('OAuthService', () => {
196209
expect(consoleSpy).toHaveBeenCalledWith(
197210
expect.stringContaining('Invalid provider ID: invalid-provider!'),
198211
);
199-
expect(service.getAvailableProviders()).toEqual([]);
212+
expect(
213+
(await service.getAllProviders()).map((p) => p.getConfig().id),
214+
).toEqual([]);
200215
});
201216
});
202217

@@ -207,6 +222,8 @@ describe('OAuthService', () => {
207222

208223
const mockCreateProvider = jest.fn().mockReturnValue(
209224
new MockOAuthProvider({
225+
id: 'test',
226+
name: 'Test Provider',
210227
clientId: 'test-client-id',
211228
clientSecret: 'test-client-secret',
212229
redirectUrl: 'http://localhost:3000/callback',
@@ -227,15 +244,14 @@ describe('OAuthService', () => {
227244
await service.initialize();
228245
});
229246

230-
it('should return provider when it exists', () => {
231-
const provider = service.getProvider('test');
247+
it('should return provider when it exists', async () => {
248+
const provider = await service.getProvider('test');
232249
expect(provider).toBeInstanceOf(MockOAuthProvider);
233250
});
234251

235-
it('should throw error when provider does not exist', () => {
236-
expect(() => service.getProvider('nonexistent')).toThrow(
237-
'OAuth provider not found: nonexistent',
238-
);
252+
it('should return undefined when provider does not exist', async () => {
253+
const provider = await service.getProvider('nonexistent');
254+
expect(provider).toBeUndefined();
239255
});
240256
});
241257

@@ -246,6 +262,8 @@ describe('OAuthService', () => {
246262

247263
const mockCreateProvider = jest.fn().mockReturnValue(
248264
new MockOAuthProvider({
265+
id: 'test',
266+
name: 'Test Provider',
249267
clientId: 'test-client-id',
250268
clientSecret: 'test-client-secret',
251269
redirectUrl: 'http://localhost:3000/callback',
@@ -266,17 +284,17 @@ describe('OAuthService', () => {
266284
await service.initialize();
267285
});
268286

269-
it('should generate authorization URL', () => {
270-
const url = service.generateAuthorizationUrl('test', 'state123');
287+
it('should generate authorization URL', async () => {
288+
const url = await service.generateAuthorizationUrl('test', 'state123');
271289
expect(url).toContain('https://test.com/oauth/authorize');
272290
expect(url).toContain('client_id=test-client-id');
273291
expect(url).toContain('state=state123');
274292
});
275293

276-
it('should throw error for nonexistent provider', () => {
277-
expect(() => service.generateAuthorizationUrl('nonexistent')).toThrow(
278-
'OAuth provider not found: nonexistent',
279-
);
294+
it('should throw error for nonexistent provider', async () => {
295+
await expect(
296+
service.generateAuthorizationUrl('nonexistent'),
297+
).rejects.toThrow("OAuth provider 'nonexistent' not found");
280298
});
281299
});
282300

@@ -287,6 +305,8 @@ describe('OAuthService', () => {
287305

288306
const mockCreateProvider = jest.fn().mockReturnValue(
289307
new MockOAuthProvider({
308+
id: 'test',
309+
name: 'Test Provider',
290310
clientId: 'test-client-id',
291311
clientSecret: 'test-client-secret',
292312
redirectUrl: 'http://localhost:3000/callback',
@@ -308,14 +328,8 @@ describe('OAuthService', () => {
308328
});
309329

310330
it('should handle callback successfully', async () => {
311-
const userInfo = await service.handleCallback('test', 'valid_code');
312-
expect(userInfo).toEqual({
313-
id: '12345',
314-
email: 'test@example.com',
315-
name: 'Test User',
316-
username: 'testuser',
317-
preferredUsername: 'testuser',
318-
});
331+
const accessToken = await service.handleCallback('test', 'valid_code');
332+
expect(accessToken).toBe('mock_access_token');
319333
});
320334

321335
it('should throw error for invalid code', async () => {
@@ -327,19 +341,21 @@ describe('OAuthService', () => {
327341
it('should throw error for nonexistent provider', async () => {
328342
await expect(
329343
service.handleCallback('nonexistent', 'code'),
330-
).rejects.toThrow('OAuth provider not found: nonexistent');
344+
).rejects.toThrow("OAuth provider 'nonexistent' not found");
331345
});
332346
});
333347

334-
describe('getAvailableProviders', () => {
348+
describe('getAllProviders', () => {
335349
it('should return empty array when no providers are loaded', async () => {
336350
jest.spyOn(configService, 'get').mockImplementation((key: string) => {
337-
if (key === 'oauth.enabledProviders') return [];
351+
if (key === 'OAUTH_ENABLED_PROVIDERS') return '';
338352
return undefined;
339353
});
340354

341355
await service.initialize();
342-
expect(service.getAvailableProviders()).toEqual([]);
356+
expect(
357+
(await service.getAllProviders()).map((p) => p.getConfig().id),
358+
).toEqual([]);
343359
});
344360

345361
it('should return provider IDs when providers are loaded', async () => {
@@ -348,6 +364,8 @@ describe('OAuthService', () => {
348364

349365
const mockCreateProvider = jest.fn().mockReturnValue(
350366
new MockOAuthProvider({
367+
id: 'test',
368+
name: 'Test Provider',
351369
clientId: 'test-client-id',
352370
clientSecret: 'test-client-secret',
353371
redirectUrl: 'http://localhost:3000/callback',
@@ -366,15 +384,17 @@ describe('OAuthService', () => {
366384
);
367385

368386
await service.initialize();
369-
expect(service.getAvailableProviders()).toEqual(['test']);
387+
expect(
388+
(await service.getAllProviders()).map((p) => p.getConfig().id),
389+
).toEqual(['test']);
370390
});
371391
});
372392

373393
describe('security validations', () => {
374394
it('should prevent path traversal in plugin loading', async () => {
375395
jest.spyOn(configService, 'get').mockImplementation((key: string) => {
376-
if (key === 'oauth.enabledProviders') return ['../../../malicious'];
377-
if (key === 'oauth.pluginPaths') return ['./plugins'];
396+
if (key === 'OAUTH_ENABLED_PROVIDERS') return '../../../malicious';
397+
if (key === 'OAUTH_PLUGIN_PATHS') return './plugins';
378398
return undefined;
379399
});
380400

@@ -388,7 +408,7 @@ describe('OAuthService', () => {
388408

389409
it('should validate provider ID contains only safe characters', async () => {
390410
jest.spyOn(configService, 'get').mockImplementation((key: string) => {
391-
if (key === 'oauth.enabledProviders') return ['test<script>'];
411+
if (key === 'OAUTH_ENABLED_PROVIDERS') return 'test<script>';
392412
return undefined;
393413
});
394414

‎src/users/users.service.spec.ts‎

Lines changed: 19 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -5,22 +5,22 @@
55
* Claude Assistant
66
*/
77

8-
import { Test, TestingModule } from '@nestjs/testing';
9-
import { ConfigService } from '@nestjs/config';
108
import { RedisService } from '@liaoliaots/nestjs-redis';
11-
import { UsersService } from './users.service';
12-
import { PrismaService } from '../common/prisma/prisma.service';
9+
import { ConfigService } from '@nestjs/config';
10+
import { Test, TestingModule } from '@nestjs/testing';
1311
import { AuthService } from '../auth/auth.service';
12+
import { OAuthUserInfo } from '../auth/oauth/oauth.types';
1413
import { SessionService } from '../auth/session.service';
15-
import { EmailService } from '../email/email.service';
1614
import { AvatarsService } from '../avatars/avatars.service';
17-
import { UsersRegisterRequestService } from './users-register-request.service';
18-
import { UsersPermissionService } from './users-permission.service';
15+
import { PrismaService } from '../common/prisma/prisma.service';
16+
import { EmailService } from '../email/email.service';
1917
import { RolePermissionService } from './role-permission.service';
20-
import { UserChallengeRepository } from './user-challenge.repository';
21-
import { TOTPService } from './totp.service';
2218
import { SrpService } from './srp.service';
23-
import { OAuthUserInfo } from '../auth/oauth/oauth.types';
19+
import { TOTPService } from './totp.service';
20+
import { UserChallengeRepository } from './user-challenge.repository';
21+
import { UsersPermissionService } from './users-permission.service';
22+
import { UsersRegisterRequestService } from './users-register-request.service';
23+
import { UsersService } from './users.service';
2424

2525
describe('UsersService - OAuth', () => {
2626
let service: UsersService;
@@ -129,11 +129,15 @@ describe('UsersService - OAuth', () => {
129129
email: 'test@ruc.edu.cn',
130130
} as any);
131131

132-
// Mock createSession method
133-
jest.spyOn(service, 'createSession').mockResolvedValue('session-token');
132+
// Mock createSession method (it's private, so we need to mock the sessionService.createSession instead)
133+
jest
134+
.spyOn(mockSessionService, 'createSession')
135+
.mockResolvedValue('session-token');
134136

135-
// Mock createDefaultProfileForUser method
136-
jest.spyOn(service, 'createDefaultProfileForUser').mockResolvedValue();
137+
// Mock createDefaultProfileForUser method (private method, mock via prisma)
138+
jest
139+
.spyOn(service as any, 'createDefaultProfileForUser')
140+
.mockResolvedValue(undefined);
137141
});
138142

139143
it('should login existing user with OAuth connection', async () => {
@@ -200,7 +204,7 @@ describe('UsersService - OAuth', () => {
200204
'test-agent',
201205
);
202206

203-
expect(service.createDefaultProfileForUser).toHaveBeenCalledWith(1);
207+
expect(service['createDefaultProfileForUser']).toHaveBeenCalledWith(1);
204208
});
205209

206210
it('should bind OAuth to existing user by email', async () => {
@@ -324,7 +328,6 @@ describe('UsersService - OAuth', () => {
324328
it('should handle OAuth user without email', async () => {
325329
const userInfoWithoutEmail: OAuthUserInfo = {
326330
id: '12345',
327-
email: null,
328331
name: 'Test User',
329332
username: 'testuser',
330333
preferredUsername: 'testuser',
@@ -420,7 +423,6 @@ describe('UsersService - OAuth', () => {
420423
const userInfoWithoutName: OAuthUserInfo = {
421424
id: '12345',
422425
email: 'test@ruc.edu.cn',
423-
name: null,
424426
username: 'testuser',
425427
preferredUsername: 'testuser',
426428
};

0 commit comments

Comments
 (0)