Skip to content

Commit 1ca5fb8

Browse files
neSpeccneSpecc
committed
Refactor SAML validation logic and add unit tests
Moved SAML audience, recipient, and time condition validation functions from SamlService to a new utils module for better separation of concerns. Added comprehensive unit tests for these utility functions and for SAML service logic. Improved test data isolation by introducing a unique test string generator. Updated existing user and usersFactory tests to use the new generator and ensure test isolation. Also, prevented MongoDB metrics setup in test environments.
1 parent 5892ea0 commit 1ca5fb8

File tree

8 files changed

+385
-115
lines changed

8 files changed

+385
-115
lines changed

‎src/metrics/mongodb.ts‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -260,6 +260,18 @@ function logCommandFailed(event: any): void {
260260
* @param client - MongoDB client to monitor
261261
*/
262262
export function setupMongoMetrics(client: MongoClient): void {
263+
/**
264+
* Skip setup in test environment
265+
* Check NODE_ENV or if running under Jest
266+
*/
267+
if (
268+
process.env.NODE_ENV === 'test' ||
269+
process.env.NODE_ENV === 'e2e' ||
270+
typeof jest !== 'undefined'
271+
) {
272+
return;
273+
}
274+
263275
client.on('commandStarted', (event) => {
264276
storeCommandInfo(event);
265277

‎src/sso/saml/service.ts‎

Lines changed: 1 addition & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { SamlConfig, SamlResponseData } from '../types';
22
import { SamlValidationError, SamlValidationErrorType } from './types';
3+
import { validateAudience, validateRecipient, validateTimeConditions } from './utils';
34

45
/**
56
* Service for SAML SSO operations
@@ -65,51 +66,5 @@ export default class SamlService {
6566
throw new Error('Not implemented');
6667
}
6768

68-
/**
69-
* Validate Audience value
70-
*
71-
* @param audience - audience value from SAML Assertion
72-
* @returns true if audience matches SSO_SP_ENTITY_ID
73-
*/
74-
public validateAudience(audience: string): boolean {
75-
const spEntityId = process.env.SSO_SP_ENTITY_ID;
76-
77-
if (!spEntityId) {
78-
throw new Error('SSO_SP_ENTITY_ID environment variable is not set');
79-
}
80-
81-
return audience === spEntityId;
82-
}
83-
84-
/**
85-
* Validate Recipient value
86-
*
87-
* @param recipient - recipient URL from SAML Assertion
88-
* @param expectedAcsUrl - expected ACS URL
89-
* @returns true if recipient matches expected ACS URL
90-
*/
91-
public validateRecipient(recipient: string, expectedAcsUrl: string): boolean {
92-
return recipient === expectedAcsUrl;
93-
}
94-
95-
/**
96-
* Validate time conditions (NotBefore and NotOnOrAfter)
97-
*
98-
* @param notBefore - NotBefore timestamp
99-
* @param notOnOrAfter - NotOnOrAfter timestamp
100-
* @param clockSkew - allowed clock skew in milliseconds (default: 2 minutes)
101-
* @returns true if assertion is valid at current time
102-
*/
103-
public validateTimeConditions(
104-
notBefore: Date,
105-
notOnOrAfter: Date,
106-
clockSkew: number = 2 * 60 * 1000
107-
): boolean {
108-
const now = Date.now();
109-
const notBeforeTime = notBefore.getTime() - clockSkew;
110-
const notOnOrAfterTime = notOnOrAfter.getTime() + clockSkew;
111-
112-
return now >= notBeforeTime && now < notOnOrAfterTime;
113-
}
11469
}
11570

‎src/sso/saml/utils.ts‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,3 +33,51 @@ export function isValidPemCertificate(cert: string): boolean {
3333
return cert.includes('-----BEGIN CERTIFICATE-----') && cert.includes('-----END CERTIFICATE-----');
3434
}
3535

36+
/**
37+
* Validate Audience value
38+
*
39+
* @param audience - audience value from SAML Assertion
40+
* @returns true if audience matches SSO_SP_ENTITY_ID
41+
* @throws Error if SSO_SP_ENTITY_ID environment variable is not set
42+
*/
43+
export function validateAudience(audience: string): boolean {
44+
const spEntityId = process.env.SSO_SP_ENTITY_ID;
45+
46+
if (!spEntityId) {
47+
throw new Error('SSO_SP_ENTITY_ID environment variable is not set');
48+
}
49+
50+
return audience === spEntityId;
51+
}
52+
53+
/**
54+
* Validate Recipient value
55+
*
56+
* @param recipient - recipient URL from SAML Assertion
57+
* @param expectedAcsUrl - expected ACS URL
58+
* @returns true if recipient matches expected ACS URL
59+
*/
60+
export function validateRecipient(recipient: string, expectedAcsUrl: string): boolean {
61+
return recipient === expectedAcsUrl;
62+
}
63+
64+
/**
65+
* Validate time conditions (NotBefore and NotOnOrAfter)
66+
*
67+
* @param notBefore - NotBefore timestamp
68+
* @param notOnOrAfter - NotOnOrAfter timestamp
69+
* @param clockSkew - allowed clock skew in milliseconds (default: 2 minutes)
70+
* @returns true if assertion is valid at current time
71+
*/
72+
export function validateTimeConditions(
73+
notBefore: Date,
74+
notOnOrAfter: Date,
75+
clockSkew: number = 2 * 60 * 1000
76+
): boolean {
77+
const now = Date.now();
78+
const notBeforeTime = notBefore.getTime() - clockSkew;
79+
const notOnOrAfterTime = notOnOrAfter.getTime() + clockSkew;
80+
81+
return now >= notBeforeTime && now < notOnOrAfterTime;
82+
}
83+

‎test/models/user.test.ts‎

Lines changed: 32 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import UserModel from '../../src/models/user';
33
import UsersFactory from '../../src/models/usersFactory';
44
import * as mongo from '../../src/mongo';
55
import DataLoaders from '../../src/dataLoaders';
6+
import { generateTestString } from '../utils/testData';
67

78
beforeAll(async () => {
89
await mongo.setupConnections();
@@ -11,36 +12,30 @@ beforeAll(async () => {
1112
describe('UserModel SSO identities', () => {
1213
let usersFactory: UsersFactory;
1314
let testUser: UserModel;
14-
const testWorkspaceId = '507f1f77bcf86cd799439011';
15-
const testSamlId = 'test-saml-name-id-123';
16-
const testEmail = '[email protected]';
1715

1816
beforeEach(async () => {
1917
/**
20-
* Create factory instance
18+
* Create factory instance with fresh DataLoaders
2119
*/
2220
usersFactory = new UsersFactory(
2321
mongo.databases.hawk as any,
2422
new DataLoaders(mongo.databases.hawk as any)
2523
);
26-
27-
/**
28-
* Create test user
29-
*/
30-
testUser = await usersFactory.create(testEmail, 'test-password-123');
3124
});
3225

3326
afterEach(async () => {
34-
/**
35-
* Clean up test user
36-
*/
37-
if (testUser && testUser.email) {
27+
if (testUser?.email) {
3828
await usersFactory.deleteByEmail(testUser.email);
3929
}
4030
});
4131

4232
describe('linkSamlIdentity', () => {
4333
it('should link SAML identity to user and update local state', async () => {
34+
const testWorkspaceId = '507f1f77bcf86cd799439011';
35+
const testSamlId = generateTestString('model-link');
36+
const testEmail = generateTestString('[email protected]');
37+
38+
testUser = await usersFactory.create(testEmail, 'test-password-123');
4439
/**
4540
* Initially, user should not have any identities
4641
*/
@@ -63,6 +58,11 @@ describe('UserModel SSO identities', () => {
6358
});
6459

6560
it('should persist SAML identity in database', async () => {
61+
const testWorkspaceId = '507f1f77bcf86cd799439011';
62+
const testSamlId = generateTestString('model-persist');
63+
const testEmail = generateTestString('[email protected]');
64+
65+
testUser = await usersFactory.create(testEmail, 'test-password-123');
6666
/**
6767
* Link SAML identity
6868
*/
@@ -83,13 +83,21 @@ describe('UserModel SSO identities', () => {
8383
});
8484

8585
it('should update existing SAML identity for the same workspace', async () => {
86-
const newSamlId = 'updated-saml-name-id-789';
86+
const testWorkspaceId = '507f1f77bcf86cd799439011';
87+
const testEmail = generateTestString('[email protected]');
88+
testUser = await usersFactory.create(testEmail, 'test-password-123');
89+
90+
/**
91+
* Use unique SAML IDs for this test
92+
*/
93+
const initialSamlId = generateTestString('initial-identity');
94+
const newSamlId = generateTestString('updated-identity');
8795
const newEmail = '[email protected]';
8896

8997
/**
9098
* Link initial identity
9199
*/
92-
await testUser.linkSamlIdentity(testWorkspaceId, testSamlId, testEmail);
100+
await testUser.linkSamlIdentity(testWorkspaceId, initialSamlId, testEmail);
93101

94102
/**
95103
* Update identity for the same workspace
@@ -116,7 +124,10 @@ describe('UserModel SSO identities', () => {
116124
});
117125

118126
describe('getSamlIdentity', () => {
119-
it('should return null when identity does not exist', () => {
127+
it('should return null when identity does not exist', async () => {
128+
const testWorkspaceId = '507f1f77bcf86cd799439011';
129+
const testEmail = generateTestString('[email protected]');
130+
testUser = await usersFactory.create(testEmail, 'test-password-123');
120131
/**
121132
* User without any identities
122133
*/
@@ -125,6 +136,11 @@ describe('UserModel SSO identities', () => {
125136
});
126137

127138
it('should return SAML identity when it exists', async () => {
139+
const testWorkspaceId = '507f1f77bcf86cd799439011';
140+
const testSamlId = generateTestString('model-get');
141+
const testEmail = generateTestString('[email protected]');
142+
143+
testUser = await usersFactory.create(testEmail, 'test-password-123');
128144
/**
129145
* Link SAML identity
130146
*/

0 commit comments

Comments
 (0)