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.

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

Implement structured logging using pino #3472

Merged
merged 41 commits into from
Jan 14, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
33f1c73
Install pino
niwsa Dec 31, 2024
f1b098b
Init logger
niwsa Dec 31, 2024
ba69606
Typing tweaks
niwsa Dec 31, 2024
5576767
Use logger instance from pino for logging
niwsa Dec 31, 2024
6caeb57
Pretty print for dev, omit stack trace for prod
niwsa Jan 1, 2025
fbff3c6
Merge branch 'main' into logger-tweaks
niwsa Jan 1, 2025
5976f0d
err object should be first argument
niwsa Jan 1, 2025
fc4bbd8
Use logger in IdP fed path
niwsa Jan 1, 2025
fafbdaf
Switch to logger
niwsa Jan 1, 2025
db34352
Log error for SAML SLO
niwsa Jan 1, 2025
1f0bb18
Indicate SAML Fed error
niwsa Jan 1, 2025
98cf12d
WIP take in logger as part of controller options
niwsa Jan 1, 2025
5d80f22
Ensure logger instance is a singleton and is initialized once at startup
niwsa Jan 2, 2025
0201e12
Rename export
niwsa Jan 2, 2025
9487210
Pass pino logger from outside
niwsa Jan 2, 2025
2a77ace
Set `console` as default logger
niwsa Jan 2, 2025
07d5ac8
Fix build error
niwsa Jan 2, 2025
fcbc802
Hide internal errors
niwsa Jan 2, 2025
82d159f
Use logger from opts
niwsa Jan 6, 2025
12e3a7d
Add `internalError` to `JacksonError`
niwsa Jan 6, 2025
481351c
Merge branch 'main' into logger-tweaks
niwsa Jan 7, 2025
d384682
Pass logger instance around
niwsa Jan 7, 2025
6e41171
Fix tests
niwsa Jan 7, 2025
b78ca27
Hide internal error in sso-handler, fix unit tests
niwsa Jan 9, 2025
a5a61f7
Merge branch 'main' into logger-tweaks
niwsa Jan 9, 2025
5fe773b
Fix lock file
niwsa Jan 9, 2025
8ee3ea9
Fix e2e tests
niwsa Jan 9, 2025
79950ca
e2e fixes
niwsa Jan 9, 2025
ce83ba8
Fix for failing e2e - Wait for added connection to appear
niwsa Jan 10, 2025
92ad34c
Fix expected error message in e2e
niwsa Jan 10, 2025
58784fe
Push pending hiding of internal error
niwsa Jan 10, 2025
54d193c
Add missing logs
niwsa Jan 10, 2025
ba8224e
Tweak error messages
niwsa Jan 10, 2025
8c81fbc
Fix err message casing
niwsa Jan 10, 2025
2519dd6
Merge branch 'main' into logger-tweaks
niwsa Jan 13, 2025
3c7843b
Push new env to example env
niwsa Jan 13, 2025
9a09228
Prepend string to make it more clear
niwsa Jan 13, 2025
e2eb04f
Tweak internal error
niwsa Jan 13, 2025
ea8b126
Tweaks to error messages and some more hiding of internal errors
niwsa Jan 13, 2025
7e2b4f7
Default should be info level
niwsa Jan 13, 2025
8e289ca
Minor tweak pass error object as 2nd argument
niwsa Jan 14, 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
8 changes: 7 additions & 1 deletion .env.example
Original file line number Diff line number Diff line change
Expand Up @@ -117,4 +117,10 @@ SETUP_LINK_EXPIRY_DAYS=3
# SSO_TRACES_DISABLE=true
# SSO_TRACES_REDACT=true
# traces ttl in hours
# SSO_TRACES_TTL=1
# SSO_TRACES_TTL=1

# Logger options
# Log file to write to
LOG_FILE=
# Log levels - "fatal" | "error" | "warn" | "info" (default) | "debug" | "trace"
LOG_LEVEL=
4 changes: 4 additions & 0 deletions e2e/support/fixtures/sso-page.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import { type Page, type Locator, expect } from '@playwright/test';
import { adminPortalSSODefaults } from '@lib/env';

export const GENERIC_ERR_STRING = 'Something wrong happened. Please contact your administrator.';

const ADMIN_PORTAL_TENANT = adminPortalSSODefaults.tenant;
export const ADMIN_PORTAL_PRODUCT = adminPortalSSODefaults.product;

Expand Down Expand Up @@ -105,6 +107,8 @@ export class SSOPage {
}
// submit the form
await this.saveConnection.click();
// check if the added connection appears in the connection list
await expect(this.page.getByText(ssoName)).toBeVisible();
this.connections = [...this.connections, ssoName];
}

Expand Down
14 changes: 5 additions & 9 deletions e2e/ui/Enterprise SSO/error.spec.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { test as baseTest, expect, request } from '@playwright/test';
import { ADMIN_PORTAL_PRODUCT, Portal, SSOPage } from 'e2e/support/fixtures';
import { ADMIN_PORTAL_PRODUCT, GENERIC_ERR_STRING, Portal, SSOPage } from 'e2e/support/fixtures';

type MyFixtures = {
ssoPage: SSOPage;
Expand Down Expand Up @@ -72,10 +72,8 @@ test('OAuth2 wrapper + SAML provider + inactive connection', async ({ ssoPage, p
// Wait for browser to redirect to error page
await page.waitForURL((url) => url.origin === baseURL && url.pathname === '/error');
// Assert error text
await expect(
page.getByText('SSO error: SSO connection is deactivated. Please contact your administrator.')
).toBeVisible();
errorMessages.push('SSO connection is deactivated. Please contact your administrator.');
await expect(page.getByText(`SSO error: ${GENERIC_ERR_STRING}`)).toBeVisible();
errorMessages.push('SSO connection is deactivated.');
});

test('OAuth2 wrapper + OIDC provider + wrong redirectUrl', async ({ ssoPage, page, baseURL }, testInfo) => {
Expand Down Expand Up @@ -118,10 +116,8 @@ test('OAuth2 wrapper + OIDC provider + inactive connection', async ({ ssoPage, p
// Wait for browser to redirect to error page
await page.waitForURL((url) => url.origin === baseURL && url.pathname === '/error');
// Assert error text
await expect(
page.getByText('SSO error: SSO connection is deactivated. Please contact your administrator.')
).toBeVisible();
errorMessages.push('SSO connection is deactivated. Please contact your administrator.');
await expect(page.getByText(`SSO error: ${GENERIC_ERR_STRING}`)).toBeVisible();
errorMessages.push('SSO connection is deactivated.');
});

test('SSO Tracer inspect', async ({ page }) => {
Expand Down
17 changes: 7 additions & 10 deletions e2e/ui/Identity Federation/error.spec.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { test as baseTest, expect, request } from '@playwright/test';
import { ADMIN_PORTAL_PRODUCT, Portal, SSOPage } from 'e2e/support/fixtures';
import { ADMIN_PORTAL_PRODUCT, GENERIC_ERR_STRING, Portal, SSOPage } from 'e2e/support/fixtures';
import { IdentityFederationPage } from 'e2e/support/fixtures/identity-federation';

type MyFixtures = {
Expand Down Expand Up @@ -93,7 +93,8 @@ test('SAML Federated app + Wrong ACS url', async ({ baseURL, page, portal, samlF
// Wait for browser to redirect to error page
await page.waitForURL((url) => url.origin === baseURL && url.pathname === '/error');
// Assert error text
await expect(page.getByText("SSO error: Assertion Consumer Service URL doesn't match.")).toBeVisible();
await expect(page.getByText(`SSO error: ${GENERIC_ERR_STRING}`)).toBeVisible();

errorMessages.push("Assertion Consumer Service URL doesn't match.");
await portal.doCredentialsLogin();
await portal.isLoggedIn();
Expand Down Expand Up @@ -130,10 +131,8 @@ test('SAML Federated app + inactive SSO connection', async ({
// Wait for browser to redirect to error page
await page.waitForURL((url) => url.origin === baseURL && url.pathname === '/error');
// Assert error text
await expect(
page.getByText('SSO error: SSO connection is deactivated. Please contact your administrator.')
).toBeVisible();
errorMessages.push('SSO connection is deactivated. Please contact your administrator.');
await expect(page.getByText(`SSO error: ${GENERIC_ERR_STRING}`)).toBeVisible();
errorMessages.push('SSO connection is deactivated.');
await portal.doCredentialsLogin();
await portal.isLoggedIn();

Expand Down Expand Up @@ -205,10 +204,8 @@ test('OIDC Federated app + inactive SSO connection', async ({
// Wait for browser to redirect to error page
await page.waitForURL((url) => url.origin === baseURL && url.pathname === '/error');
// Assert error text
await expect(
page.getByText('SSO error: SSO connection is deactivated. Please contact your administrator.')
).toBeVisible();
errorMessages.push('SSO connection is deactivated. Please contact your administrator.');
await expect(page.getByText(`SSO error: ${GENERIC_ERR_STRING}`)).toBeVisible();
errorMessages.push('SSO connection is deactivated.');
await portal.doCredentialsLogin();
await portal.isLoggedIn();

Expand Down
3 changes: 2 additions & 1 deletion ee/identity-federation/api/oidc/idp-login/[fedAppId].ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { NextApiRequest, NextApiResponse } from 'next';
import jackson from '@lib/jackson';
import { OIDCIdPInitiatedReq } from '@boxyhq/saml-jackson';
import { setErrorCookie } from '@lib/utils';
import { logger } from '@lib/logger';

export default async function handler(req: NextApiRequest, res: NextApiResponse) {
try {
Expand All @@ -28,7 +29,7 @@ export default async function handler(req: NextApiRequest, res: NextApiResponse)
res.redirect(302, redirect_url);
}
} catch (err: any) {
console.error('OIDC IDP initiated login error:', err);
logger.error(err, 'OIDC IdP initiated login error');
const { message, statusCode = 500 } = err;
// set error in cookie redirect to error page
setErrorCookie(res, { message, statusCode }, { path: '/error' });
Expand Down
3 changes: 2 additions & 1 deletion ee/identity-federation/api/sso.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import type { NextApiRequest, NextApiResponse } from 'next';

import jackson from '@lib/jackson';
import { setErrorCookie } from '@lib/utils';
import { logger } from '@lib/logger';

type SAMLRequest = {
SAMLRequest: string;
Expand All @@ -28,7 +29,7 @@ export default async function handler(req: NextApiRequest, res: NextApiResponse)
res.status(405).json({ error: { message: `Method ${method} Not Allowed` } });
}
} catch (err: any) {
console.error('authorize error:', err);
logger.error(err, 'SAML Federation authorize error');
const { message, statusCode = 500 } = err;
setErrorCookie(res, { message, statusCode }, { path: '/error' });
res.redirect(302, '/error');
Expand Down
6 changes: 6 additions & 0 deletions lib/env.ts
Original file line number Diff line number Diff line change
Expand Up @@ -136,8 +136,14 @@ const adminPortalSSODefaults = {
defaultRedirectUrl: `${externalUrl}/admin/auth/idp-login`,
};

const loggerOptions = {
file: process.env.LOG_FILE,
level: process.env.LOG_LEVEL,
};

export { adminPortalSSODefaults };
export { retraced as retracedOptions };
export { terminus as terminusOptions };
export { apiKeys };
export { jacksonOptions };
export { loggerOptions };
12 changes: 11 additions & 1 deletion lib/jackson.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,23 @@ import type { SAMLJackson } from '@boxyhq/saml-jackson';
import jackson from '@boxyhq/saml-jackson';
import { jacksonOptions } from '@lib/env';
import '@lib/metrics';
import { logger } from './logger';

const g = global as any;

const jacksonOptionsWithLogger = {
...jacksonOptions,
logger: {
info: (msg: string, err?: any) => logger.info(err, msg),
error: (msg: string, err?: any) => logger.error(err, msg),
warn: (msg: string, err?: any) => logger.warn(err, msg),
},
};

export default async function init() {
if (!g.jacksonInstance) {
g.jacksonInstance = new Promise((resolve, reject) => {
jackson(jacksonOptions).then(resolve).catch(reject);
jackson(jacksonOptionsWithLogger).then(resolve).catch(reject);
});
}

Expand Down
51 changes: 51 additions & 0 deletions lib/logger.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import pino, { type Logger } from 'pino';
import fs from 'fs';
import { loggerOptions } from '@lib/env';

const isDevelopment = process.env.NODE_ENV !== 'production';
const g = global as any;

// Custom error serializer for production that omits stack traces
const productionErrorSerializer = ({
message,
statusCode = 500,
internalError,
}: Error & { statusCode?: number; internalError?: string }) => {
// stack trace is intentionally omitted
const err: any = { message, statusCode };
if (internalError) {
err.internalError = internalError;
}
return err;
};

export function initLogger(logFile?: string, logLevel?: string): Logger {
if (logFile) {
return pino(fs.createWriteStream(logFile));
}

return pino({
level: logLevel || 'info',
timestamp: () => `,"time":"${new Date().toISOString()}"`,
transport: isDevelopment
? {
target: 'pino-pretty',
options: {
colorize: true,
},
}
: undefined,
serializers: {
err: isDevelopment ? pino.stdSerializers.err : productionErrorSerializer,
},
});
}

function initLoggerFromEnv(): Logger {
if (!g.loggerInstance) {
g.loggerInstance = initLogger(loggerOptions.file, loggerOptions.level);
}
return g.loggerInstance;
}

export const logger = initLoggerFromEnv();
3 changes: 2 additions & 1 deletion lib/nextAuthAdapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,14 @@ import DB from 'npm/src/db/db';
import { jacksonOptions } from './env';
import type { AdapterUser, VerificationToken } from 'next-auth/adapters';
import defaultDb from 'npm/src/db/defaultDb';
import { logger } from './logger';

const g = global as any;

export async function initNextAuthDB(): Promise<Storable> {
if (!g.adminAuthStore) {
const _opts = defaultDb(jacksonOptions);
const db = await DB.new(_opts.db);
const db = await DB.new({ db: _opts.db, logger });
g.adminAuthStore = db.store('admin:auth');
}
return g.adminAuthStore as Storable;
Expand Down
13 changes: 10 additions & 3 deletions npm/src/controller/analytics.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
import { IConnectionAPIController, IDirectorySyncController, Storable } from '../typings';
import {
IConnectionAPIController,
IDirectorySyncController,
JacksonOptionWithRequiredLogger,
Storable,
} from '../typings';
import Mixpanel, { type Event } from 'mixpanel';
import { randomUUID } from 'crypto';

Expand All @@ -11,13 +16,15 @@ export class AnalyticsController {
directorySyncController: IDirectorySyncController;
client: Mixpanel.Mixpanel;
anonymousId: string;
private opts: JacksonOptionWithRequiredLogger;

constructor({ analyticsStore, connectionAPIController, directorySyncController }) {
constructor({ opts, analyticsStore, connectionAPIController, directorySyncController }) {
this.analyticsStore = analyticsStore;
this.client = Mixpanel.init('1028494897a5520b90e7344344060fa7');
this.connectionAPIController = connectionAPIController;
this.directorySyncController = directorySyncController;
this.anonymousId = '';
this.opts = opts;
}

public async init(): Promise<void> {
Expand Down Expand Up @@ -80,7 +87,7 @@ export class AnalyticsController {
this.analyticsStore.put(sentKey, new Date().toISOString());
});
} catch (err) {
console.error('Error sending analytics', err);
this.opts.logger.error('Error sending analytics', err);
}
}
}
6 changes: 5 additions & 1 deletion npm/src/controller/error.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,16 @@ import { ApiError } from '../typings';
export class JacksonError extends Error {
public name: string;
public statusCode: number;
public internalError?: string;

constructor(message: string, statusCode = 500) {
constructor(message: string, statusCode = 500, internalError: string = '') {
super(message);

this.name = this.constructor.name;
this.statusCode = statusCode;
if (internalError) {
this.internalError = internalError;
}

Error.captureStackTrace(this, this.constructor);
}
Expand Down
Loading
Loading