fix: address all code review findings
- Extract shared permission logic into boardPermissions.ts utility - Fix DnD drag revert: add dirty flag to prevent overwrite - Wrap OAuth group sync in Prisma transaction (N+1 fix) - Add empty widgetIds validation in widget reorder API - Add invalidateAll() after guest toggle PATCH - Replace console.error with user-visible error banners - Extract WidgetCreationForm component (DraggableSection was 448 lines) - Remove unused boardId prop from DraggableSection - Always include OAuth state parameter + validate in callback - Clean up copyLink timer on component destroy - Add type-specific widget config validation in addWidget action
This commit is contained in:
@@ -25,6 +25,7 @@ import { prisma } from '../../prisma.js';
|
||||
import {
|
||||
invalidateOAuthCache,
|
||||
generateCodeVerifier,
|
||||
generateState,
|
||||
calculateCodeChallenge,
|
||||
generateAuthUrl,
|
||||
handleCallback,
|
||||
@@ -69,6 +70,14 @@ describe('oauthService', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('generateState', () => {
|
||||
it('returns a random state string', () => {
|
||||
const state = generateState();
|
||||
expect(state).toBe('mock-state-123');
|
||||
expect(mockClient.randomState).toHaveBeenCalledOnce();
|
||||
});
|
||||
});
|
||||
|
||||
describe('calculateCodeChallenge', () => {
|
||||
it('returns a PKCE code challenge', async () => {
|
||||
const challenge = await calculateCodeChallenge('my-verifier');
|
||||
@@ -86,7 +95,7 @@ describe('oauthService', () => {
|
||||
new URL('https://auth.example.com/authorize?code_challenge=abc')
|
||||
);
|
||||
|
||||
const url = await generateAuthUrl('https://app.example.com/callback', 'test-challenge');
|
||||
const url = await generateAuthUrl('https://app.example.com/callback', 'test-challenge', 'test-state');
|
||||
|
||||
expect(url).toBe('https://auth.example.com/authorize?code_challenge=abc');
|
||||
expect(mockClient.buildAuthorizationUrl).toHaveBeenCalledWith(
|
||||
@@ -95,7 +104,8 @@ describe('oauthService', () => {
|
||||
redirect_uri: 'https://app.example.com/callback',
|
||||
scope: 'openid profile email',
|
||||
code_challenge: 'test-challenge',
|
||||
code_challenge_method: 'S256'
|
||||
code_challenge_method: 'S256',
|
||||
state: 'test-state'
|
||||
})
|
||||
);
|
||||
});
|
||||
@@ -111,7 +121,7 @@ describe('oauthService', () => {
|
||||
delete process.env.OAUTH_DISCOVERY_URL;
|
||||
|
||||
await expect(
|
||||
generateAuthUrl('https://app.example.com/callback', 'challenge')
|
||||
generateAuthUrl('https://app.example.com/callback', 'challenge', 'state')
|
||||
).rejects.toThrow('OAuth is not configured');
|
||||
|
||||
// Restore
|
||||
@@ -120,25 +130,20 @@ describe('oauthService', () => {
|
||||
process.env.OAUTH_DISCOVERY_URL = origDiscovery;
|
||||
});
|
||||
|
||||
it('adds state when provider does not support PKCE', async () => {
|
||||
it('always includes the state parameter', async () => {
|
||||
setupOAuthSettings();
|
||||
const mockConfig = {
|
||||
serverMetadata: () => ({
|
||||
issuer: 'https://auth.example.com',
|
||||
supportsPKCE: () => false
|
||||
})
|
||||
};
|
||||
const mockConfig = createMockOIDCConfig();
|
||||
mockClient.discovery.mockResolvedValue(mockConfig);
|
||||
mockClient.buildAuthorizationUrl.mockReturnValue(
|
||||
new URL('https://auth.example.com/authorize')
|
||||
);
|
||||
|
||||
await generateAuthUrl('https://app.example.com/callback', 'test-challenge');
|
||||
await generateAuthUrl('https://app.example.com/callback', 'test-challenge', 'custom-state');
|
||||
|
||||
expect(mockClient.buildAuthorizationUrl).toHaveBeenCalledWith(
|
||||
mockConfig,
|
||||
expect.objectContaining({
|
||||
state: 'mock-state-123'
|
||||
state: 'custom-state'
|
||||
})
|
||||
);
|
||||
});
|
||||
@@ -163,8 +168,9 @@ describe('oauthService', () => {
|
||||
});
|
||||
|
||||
const result = await handleCallback(
|
||||
new URL('https://app.example.com/callback?code=abc'),
|
||||
'test-verifier'
|
||||
new URL('https://app.example.com/callback?code=abc&state=test-state'),
|
||||
'test-verifier',
|
||||
'test-state'
|
||||
);
|
||||
|
||||
expect(result).toEqual({
|
||||
@@ -188,8 +194,9 @@ describe('oauthService', () => {
|
||||
|
||||
await expect(
|
||||
handleCallback(
|
||||
new URL('https://app.example.com/callback?code=abc'),
|
||||
'test-verifier'
|
||||
new URL('https://app.example.com/callback?code=abc&state=test-state'),
|
||||
'test-verifier',
|
||||
'test-state'
|
||||
)
|
||||
).rejects.toThrow('subject claim');
|
||||
});
|
||||
@@ -209,8 +216,9 @@ describe('oauthService', () => {
|
||||
|
||||
await expect(
|
||||
handleCallback(
|
||||
new URL('https://app.example.com/callback?code=abc'),
|
||||
'test-verifier'
|
||||
new URL('https://app.example.com/callback?code=abc&state=test-state'),
|
||||
'test-verifier',
|
||||
'test-state'
|
||||
)
|
||||
).rejects.toThrow('email');
|
||||
});
|
||||
|
||||
@@ -96,6 +96,13 @@ export function generateCodeVerifier(): string {
|
||||
return client.randomPKCECodeVerifier();
|
||||
}
|
||||
|
||||
/**
|
||||
* Generates a cryptographically random state parameter.
|
||||
*/
|
||||
export function generateState(): string {
|
||||
return client.randomState();
|
||||
}
|
||||
|
||||
/**
|
||||
* Calculates the PKCE code_challenge from a code_verifier.
|
||||
*/
|
||||
@@ -105,10 +112,12 @@ export async function calculateCodeChallenge(codeVerifier: string): Promise<stri
|
||||
|
||||
/**
|
||||
* Builds the authorization URL to redirect the user to the OIDC provider.
|
||||
* Always includes a state parameter for CSRF protection.
|
||||
*/
|
||||
export async function generateAuthUrl(
|
||||
redirectUri: string,
|
||||
codeChallenge: string
|
||||
codeChallenge: string,
|
||||
state: string
|
||||
): Promise<string> {
|
||||
const config = await getOIDCConfig();
|
||||
|
||||
@@ -116,14 +125,10 @@ export async function generateAuthUrl(
|
||||
redirect_uri: redirectUri,
|
||||
scope: 'openid profile email',
|
||||
code_challenge: codeChallenge,
|
||||
code_challenge_method: 'S256'
|
||||
code_challenge_method: 'S256',
|
||||
state
|
||||
};
|
||||
|
||||
// Add state if the server might not support PKCE
|
||||
if (!config.serverMetadata().supportsPKCE()) {
|
||||
parameters.state = client.randomState();
|
||||
}
|
||||
|
||||
const url = client.buildAuthorizationUrl(config, parameters);
|
||||
return url.href;
|
||||
}
|
||||
@@ -133,12 +138,14 @@ export async function generateAuthUrl(
|
||||
*/
|
||||
export async function handleCallback(
|
||||
callbackUrl: URL,
|
||||
codeVerifier: string
|
||||
codeVerifier: string,
|
||||
expectedState: string
|
||||
): Promise<OAuthUserInfo> {
|
||||
const config = await getOIDCConfig();
|
||||
|
||||
const tokens = await client.authorizationCodeGrant(config, callbackUrl, {
|
||||
pkceCodeVerifier: codeVerifier
|
||||
pkceCodeVerifier: codeVerifier,
|
||||
expectedState
|
||||
});
|
||||
|
||||
// Try to get user info from the userinfo endpoint
|
||||
|
||||
@@ -184,14 +184,16 @@ async function syncOAuthGroups(userId: string, oauthGroupNames: readonly string[
|
||||
return;
|
||||
}
|
||||
|
||||
// Upsert memberships (idempotent — won't fail if already a member)
|
||||
for (const group of matchingGroups) {
|
||||
await prisma.userGroup.upsert({
|
||||
where: {
|
||||
userId_groupId: { userId, groupId: group.id }
|
||||
},
|
||||
update: {},
|
||||
create: { userId, groupId: group.id }
|
||||
});
|
||||
}
|
||||
// Upsert memberships in a single transaction (idempotent — won't fail if already a member)
|
||||
await prisma.$transaction(
|
||||
matchingGroups.map((group) =>
|
||||
prisma.userGroup.upsert({
|
||||
where: {
|
||||
userId_groupId: { userId, groupId: group.id }
|
||||
},
|
||||
update: {},
|
||||
create: { userId, groupId: group.id }
|
||||
})
|
||||
)
|
||||
);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user