From f1ee27a518889d788a7b54609f2e047ed81ffe3b Mon Sep 17 00:00:00 2001 From: Jessica Lawshe Date: Mon, 8 Aug 2022 09:19:14 -0500 Subject: [PATCH] Merge pull request #9074 from overleaf/jel-saml-log-tests [web] Add tests for SAML log GitOrigin-RevId: 7bc5b25461063b32d3471b7f4ab966f2caa4e70c --- .../src/Features/Errors/ErrorController.js | 12 +---- .../src/Features/SamlLog/SamlLogHandler.js | 45 +++++++++++++++++-- services/web/app/src/models/SamlLog.js | 3 ++ .../unit/src/SamlLog/SamlLogHandlerTests.js | 31 +++++++++++-- 4 files changed, 73 insertions(+), 18 deletions(-) diff --git a/services/web/app/src/Features/Errors/ErrorController.js b/services/web/app/src/Features/Errors/ErrorController.js index 48e41253bf..5cb56ee60d 100644 --- a/services/web/app/src/Features/Errors/ErrorController.js +++ b/services/web/app/src/Features/Errors/ErrorController.js @@ -26,17 +26,7 @@ module.exports = ErrorController = { const user = SessionManager.getSessionUser(req.session) // log errors related to SAML flow if (req.session && req.session.saml) { - SamlLogHandler.log(req.session.saml.universityId, req.sessionID, { - error: { - message: error && error.message, - stack: error && error.stack, - }, - body: req.body, - path: req.path, - query: req.query, - saml: req.session.saml, - user_id: user && user._id, - }) + SamlLogHandler.log(req, { error }) } if (error.code === 'EBADCSRFTOKEN') { logger.warn( diff --git a/services/web/app/src/Features/SamlLog/SamlLogHandler.js b/services/web/app/src/Features/SamlLog/SamlLogHandler.js index 8b69f324c0..25c0c5d70e 100644 --- a/services/web/app/src/Features/SamlLog/SamlLogHandler.js +++ b/services/web/app/src/Features/SamlLog/SamlLogHandler.js @@ -1,11 +1,50 @@ const { SamlLog } = require('../../models/SamlLog') +const SessionManager = require('../Authentication/SessionManager') const logger = require('@overleaf/logger') +const { err: errSerializer } = require('@overleaf/logger/serializers') + +function log(req, data, samlAssertion) { + let providerId, sessionId + + data = data || {} -function log(providerId, sessionId, data) { try { const samlLog = new SamlLog() - samlLog.providerId = providerId = (providerId || '').toString() - samlLog.sessionId = sessionId = (sessionId || '').toString().substr(0, 8) + const { path, query } = req + const { saml } = req.session + const userId = SessionManager.getLoggedInUserId(req.session) + + providerId = (req.session.saml?.universityId || '').toString() + sessionId = (req.sessionID || '').toString().substr(0, 8) + + samlLog.providerId = providerId + samlLog.sessionId = sessionId + samlLog.path = path + samlLog.userId = userId + data.query = query + data.samlSession = saml + + if (data.error instanceof Error) { + const errSerialized = errSerializer(data.error) + if (data.error.tryAgain) { + errSerialized.tryAgain = data.error.tryAgain + } + data.error = errSerialized + } + + if (samlAssertion) { + const samlAssertionForLog = { + assertionXml: samlAssertion.getAssertionXml(), + responseXml: samlAssertion.getSamlResponseXml(), + assertionJsonExtended: req.user_info, + } + samlLog.samlAssertion = JSON.stringify(samlAssertionForLog) + } + + if (data.error || samlAssertion) { + data.body = req.body + } + try { samlLog.jsonData = JSON.stringify(data) } catch (err) { diff --git a/services/web/app/src/models/SamlLog.js b/services/web/app/src/models/SamlLog.js index 234a25da8b..dedf076a9d 100644 --- a/services/web/app/src/models/SamlLog.js +++ b/services/web/app/src/models/SamlLog.js @@ -6,8 +6,11 @@ const SamlLogSchema = new Schema( createdAt: { type: Date, default: () => new Date() }, data: { type: Object }, jsonData: { type: String }, + path: { type: String }, providerId: { type: String, default: '' }, + samlAssertion: { type: String }, sessionId: { type: String, default: '' }, + userId: { type: String, default: '' }, }, { collection: 'samlLogs', diff --git a/services/web/test/unit/src/SamlLog/SamlLogHandlerTests.js b/services/web/test/unit/src/SamlLog/SamlLogHandlerTests.js index 75a6933137..cb27f7c350 100644 --- a/services/web/test/unit/src/SamlLog/SamlLogHandlerTests.js +++ b/services/web/test/unit/src/SamlLog/SamlLogHandlerTests.js @@ -31,13 +31,24 @@ describe('SamlLogHandler', function () { describe('with valid data object', function () { beforeEach(function () { - SamlLogHandler.log(providerId, sessionId, data) + SamlLogHandler.log( + { + session: { saml: { universityId: providerId } }, + sessionID: sessionId, + }, + data + ) }) it('should log data', function () { samlLog.providerId.should.equal(providerId) samlLog.sessionId.should.equal(sessionId.substr(0, 8)) - samlLog.jsonData.should.equal('{"foo":true}') + samlLog.jsonData.should.equal( + JSON.stringify({ + foo: true, + samlSession: { universityId: 'provider-id' }, + }) + ) expect(samlLog.data).to.be.undefined samlLog.save.should.have.been.calledOnce }) @@ -48,7 +59,13 @@ describe('SamlLogHandler', function () { const circularRef = {} circularRef.circularRef = circularRef - SamlLogHandler.log(providerId, sessionId, circularRef) + SamlLogHandler.log( + { + session: { saml: { universityId: providerId } }, + sessionID: sessionId, + }, + circularRef + ) }) it('should log without data and log error', function () { @@ -68,7 +85,13 @@ describe('SamlLogHandler', function () { beforeEach(function () { samlLog.save = sinon.stub().yields('error') - SamlLogHandler.log(providerId, sessionId, data) + SamlLogHandler.log( + { + session: { saml: { universityId: providerId } }, + sessionID: sessionId, + }, + data + ) }) it('should log error', function () {