Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refacto(auth): improve type + remove complexity #9949

Merged
merged 2 commits into from
Jan 31, 2025
Merged

refacto(auth): improve type + remove complexity #9949

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
85579d3
feat(auth/sso): Add support for forceSubdomainUrl functionality
AMoreaux Jan 31, 2025
5b9c9c5
feat(security): add support for Microsoft SSO icon detection
AMoreaux Jan 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion ...ges/twenty-front/src/modules/settings/security/utils/guessSSOIdentityProviderIconByUrl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
/* @license Enterprise */

import { IconComponent, IconGoogle, IconKey } from 'twenty-ui';
import {
IconComponent,
IconGoogle,
IconKey,
IconMicrosoftOutlook,
} from 'twenty-ui';

export const guessSSOIdentityProviderIconByUrl = (
url: string,
Expand All @@ -9,5 +14,9 @@ export const guessSSOIdentityProviderIconByUrl = (
return IconGoogle;
}

if (url.includes('microsoft')) {
return IconMicrosoftOutlook;
}

return IconKey;
};
3 changes: 1 addition & 2 deletions packages/twenty-server/src/engine/core-modules/auth/controllers/google-auth.controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ export class GoogleAuthController {
email,
picture,
workspaceInviteHash,
workspacePersonalInviteToken,
workspaceId,
billingCheckoutSessionState,
} = req.user;
Expand All @@ -65,7 +64,7 @@ export class GoogleAuthController {

try {
const invitation =
currentWorkspace && workspacePersonalInviteToken && email
currentWorkspace && email
? await this.authService.findInvitationForSignInUp({
currentWorkspace,
email,
Expand Down
3 changes: 1 addition & 2 deletions packages/twenty-server/src/engine/core-modules/auth/controllers/microsoft-auth.controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ export class MicrosoftAuthController {
email,
picture,
workspaceInviteHash,
workspacePersonalInviteToken,
workspaceId,
billingCheckoutSessionState,
} = req.user;
Expand All @@ -66,7 +65,7 @@ export class MicrosoftAuthController {

try {
const invitation =
currentWorkspace && workspacePersonalInviteToken && email
currentWorkspace && email
? await this.authService.findInvitationForSignInUp({
currentWorkspace,
email,
Expand Down
16 changes: 9 additions & 7 deletions packages/twenty-server/src/engine/core-modules/auth/controllers/sso-auth.controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ import { User } from 'src/engine/core-modules/user/user.entity';
import { AuthOAuthExceptionFilter } from 'src/engine/core-modules/auth/filters/auth-oauth-exception.filter';
import { EnvironmentService } from 'src/engine/core-modules/environment/environment.service';
import { GuardRedirectService } from 'src/engine/core-modules/guard-redirect/services/guard-redirect.service';
import { SAMLRequest } from 'src/engine/core-modules/auth/strategies/saml.auth.strategy';
import { OIDCRequest } from 'src/engine/core-modules/auth/strategies/oidc.auth.strategy';

@Controller('auth')
export class SSOAuthController {
Expand Down Expand Up @@ -85,14 +87,14 @@ export class SSOAuthController {
@Get('oidc/callback')
@UseGuards(EnterpriseFeaturesEnabledGuard, OIDCAuthGuard)
@UseFilters(AuthOAuthExceptionFilter)
async oidcAuthCallback(@Req() req: any, @Res() res: Response) {
async oidcAuthCallback(@Req() req: OIDCRequest, @Res() res: Response) {
return await this.authCallback(req, res);
}

@Post('saml/callback/:identityProviderId')
@UseGuards(EnterpriseFeaturesEnabledGuard, SAMLAuthGuard)
@UseFilters(AuthOAuthExceptionFilter)
async samlAuthCallback(@Req() req: any, @Res() res: Response) {
async samlAuthCallback(@Req() req: SAMLRequest, @Res() res: Response) {
try {
return await this.authCallback(req, res);
} catch (err) {
Expand All @@ -103,10 +105,10 @@ export class SSOAuthController {
}
}

private async authCallback({ user }: any, res: Response) {
private async authCallback(req: OIDCRequest | SAMLRequest, res: Response) {
const workspaceIdentityProvider =
await this.findWorkspaceIdentityProviderByIdentityProviderId(
user.identityProviderId,
req.user.identityProviderId,
);

try {
Expand All @@ -117,15 +119,15 @@ export class SSOAuthController {
);
}

if (!user.user.email) {
if (!req.user.email) {
throw new AuthException(
'Email not found from identity provider.',
AuthExceptionCode.OAUTH_ACCESS_DENIED,
);
}

const { loginToken, identityProvider } = await this.generateLoginToken(
user.user,
req.user,
workspaceIdentityProvider,
);

Expand Down Expand Up @@ -157,7 +159,7 @@ export class SSOAuthController {
}

private async generateLoginToken(
payload: { email: string } & Record<string, string>,
payload: { email: string },
identityProvider: WorkspaceSSOIdentityProvider,
Comment on lines +162 to 163
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: The payload type is too restrictive - OIDC and SAML responses typically include firstName and lastName which are used in formatUserDataPayload()

) {
if (!identityProvider) {
Expand Down
26 changes: 2 additions & 24 deletions packages/twenty-server/src/engine/core-modules/auth/guards/google-oauth.guard.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { AuthGuard } from '@nestjs/passport';
import { InjectRepository } from '@nestjs/typeorm';

import { Repository } from 'typeorm';
import { Request } from 'express';

import {
AuthException,
Expand All @@ -26,7 +27,7 @@ export class GoogleOauthGuard extends AuthGuard('google') {
}

async canActivate(context: ExecutionContext) {
const request = context.switchToHttp().getRequest();
const request = context.switchToHttp().getRequest<Request>();
let workspace: Workspace | null = null;

try {
Expand All @@ -40,36 +41,13 @@ export class GoogleOauthGuard extends AuthGuard('google') {
});
}

const workspaceInviteHash = request.query.inviteHash;
const workspacePersonalInviteToken = request.query.inviteToken;

if (request.query.error === 'access_denied') {
throw new AuthException(
'Google OAuth access denied',
AuthExceptionCode.OAUTH_ACCESS_DENIED,
);
}

if (workspaceInviteHash && typeof workspaceInviteHash === 'string') {
request.params.workspaceInviteHash = workspaceInviteHash;
}

if (
workspacePersonalInviteToken &&
typeof workspacePersonalInviteToken === 'string'
) {
request.params.workspacePersonalInviteToken =
workspacePersonalInviteToken;
}

if (
request.query.billingCheckoutSessionState &&
typeof request.query.billingCheckoutSessionState === 'string'
) {
request.params.billingCheckoutSessionState =
request.query.billingCheckoutSessionState;
}

return (await super.canActivate(context)) as boolean;
} catch (err) {
this.guardRedirectService.dispatchErrorFromGuard(
Expand Down
23 changes: 0 additions & 23 deletions packages/twenty-server/src/engine/core-modules/auth/guards/microsoft-oauth.guard.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,29 +36,6 @@ export class MicrosoftOAuthGuard extends AuthGuard('microsoft') {
});
}

const workspaceInviteHash = request.query.inviteHash;
const workspacePersonalInviteToken = request.query.inviteToken;

if (workspaceInviteHash && typeof workspaceInviteHash === 'string') {
request.params.workspaceInviteHash = workspaceInviteHash;
}

if (
workspacePersonalInviteToken &&
typeof workspacePersonalInviteToken === 'string'
) {
request.params.workspacePersonalInviteToken =
workspacePersonalInviteToken;
}

if (
request.query.billingCheckoutSessionState &&
typeof request.query.billingCheckoutSessionState === 'string'
) {
request.params.billingCheckoutSessionState =
request.query.billingCheckoutSessionState;
}

return (await super.canActivate(context)) as boolean;
} catch (err) {
this.guardRedirectService.dispatchErrorFromGuard(
Expand Down
18 changes: 4 additions & 14 deletions packages/twenty-server/src/engine/core-modules/auth/strategies/google.auth.strategy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,24 +34,14 @@ export class GoogleStrategy extends PassportStrategy(Strategy, 'google') {
});
}

authenticate(req: any, options: any) {
authenticate(req: Request, options: any) {
options = {
...options,
state: JSON.stringify({
workspaceInviteHash: req.params.workspaceInviteHash,
workspaceInviteHash: req.query.workspaceInviteHash,
workspaceId: req.params.workspaceId,
...(req.params.billingCheckoutSessionState
? {
billingCheckoutSessionState:
req.params.billingCheckoutSessionState,
}
: {}),
...(req.params.workspacePersonalInviteToken
? {
workspacePersonalInviteToken:
req.params.workspacePersonalInviteToken,
}
: {}),
billingCheckoutSessionState: req.query.billingCheckoutSessionState,
workspacePersonalInviteToken: req.query.workspacePersonalInviteToken,
}),
};

Expand Down
18 changes: 4 additions & 14 deletions packages/twenty-server/src/engine/core-modules/auth/strategies/microsoft.auth.strategy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,24 +38,14 @@ export class MicrosoftStrategy extends PassportStrategy(Strategy, 'microsoft') {
});
}

authenticate(req: any, options: any) {
authenticate(req: Request, options: any) {
options = {
...options,
state: JSON.stringify({
workspaceInviteHash: req.params.workspaceInviteHash,
workspaceInviteHash: req.query.workspaceInviteHash,
workspaceId: req.params.workspaceId,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: workspaceId is still being read from req.params while other parameters were moved to req.query. This inconsistency could cause issues with workspace ID retrieval.

...(req.params.billingCheckoutSessionState
? {
billingCheckoutSessionState:
req.params.billingCheckoutSessionState,
}
: {}),
...(req.params.workspacePersonalInviteToken
? {
workspacePersonalInviteToken:
req.params.workspacePersonalInviteToken,
}
: {}),
billingCheckoutSessionState: req.query.billingCheckoutSessionState,
workspacePersonalInviteToken: req.query.workspacePersonalInviteToken,
}),
};

Expand Down
74 changes: 51 additions & 23 deletions packages/twenty-server/src/engine/core-modules/auth/strategies/oidc.auth.strategy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,26 @@
import { Injectable } from '@nestjs/common';
import { PassportStrategy } from '@nestjs/passport';

import { isEmail } from 'class-validator';
import { Request } from 'express';
import { Strategy, StrategyOptions, TokenSet } from 'openid-client';

import {
Strategy,
StrategyOptions,
StrategyVerifyCallbackReq,
} from 'openid-client';
AuthException,
AuthExceptionCode,
} from 'src/engine/core-modules/auth/auth.exception';

export type OIDCRequest = Omit<
Request,
'user' | 'workspace' | 'workspaceMetadataVersion'
> & {
user: {
identityProviderId: string;
email: string;
firstName?: string | null;
lastName?: string | null;
};
};

@Injectable()
export class OIDCAuthStrategy extends PassportStrategy(
Expand All @@ -30,7 +45,7 @@ export class OIDCAuthStrategy extends PassportStrategy(
});
}

async authenticate(req: any, options: any) {
async authenticate(req: Request, options: any) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: options parameter should be properly typed instead of 'any'

return super.authenticate(req, {
...options,
state: JSON.stringify({
Expand All @@ -39,37 +54,50 @@ export class OIDCAuthStrategy extends PassportStrategy(
});
}

validate: StrategyVerifyCallbackReq<{
private extractState(req: Request): {
identityProviderId: string;
user: {
email?: string;
firstName?: string | null;
lastName?: string | null;
};
}> = async (req, tokenset, done) => {
} {
try {
const state = JSON.parse(
'query' in req &&
req.query &&
typeof req.query === 'object' &&
'state' in req.query &&
req.query.state &&
typeof req.query.state === 'string'
req.query.state && typeof req.query.state === 'string'
? req.query.state
: '{}',
);

if (!state.identityProviderId) {
throw new Error();
}

return {
identityProviderId: state.identityProviderId,
};
} catch (err) {
throw new AuthException('Invalid state', AuthExceptionCode.INVALID_INPUT);
}
}

async validate(
req: Request,
tokenset: TokenSet,
done: (err: any, user?: OIDCRequest['user']) => void,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: err parameter should be properly typed instead of 'any'

) {
try {
const state = this.extractState(req);

const userinfo = await this.client.userinfo(tokenset);

const user = {
if (!userinfo.email || !isEmail(userinfo.email)) {
return done(new Error('Invalid email'));
}
Comment on lines +89 to +91
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: error message should use AuthException with proper error code for consistency with extractState method

Suggested change
if (!userinfo.email || !isEmail(userinfo.email)) {
return done(new Error('Invalid email'));
}
if (!userinfo.email || !isEmail(userinfo.email)) {
return done(new AuthException('Invalid email', AuthExceptionCode.INVALID_INPUT));
}


done(null, {
email: userinfo.email,
identityProviderId: state.identityProviderId,
...(userinfo.given_name ? { firstName: userinfo.given_name } : {}),
...(userinfo.family_name ? { lastName: userinfo.family_name } : {}),
};

done(null, { user, identityProviderId: state.identityProviderId });
});
} catch (err) {
done(err);
}
};
}
}
Loading
Loading