diff --git a/packages/datadog-instrumentations/src/passport-http.js b/packages/datadog-instrumentations/src/passport-http.js index 5fff0efeedc..a1032ca9184 100644 --- a/packages/datadog-instrumentations/src/passport-http.js +++ b/packages/datadog-instrumentations/src/passport-http.js @@ -7,12 +7,13 @@ const passportVerifyChannel = channel('datadog:passport:verify:finish') function wrapVerifiedAndPublish (username, password, verified) { if (passportVerifyChannel.hasSubscribers) { - return shimmer.wrap(verified, (err, user, info) => { - // TODO: check if err and info are really useful + return shimmer.wrap(verified, function (err, user, info) { const credentials = { type: 'http', username: username, password: password } - passportVerifyChannel.publish({ credentials, user, err, info }) - return verified(err, user, info) + passportVerifyChannel.publish({ credentials, user }) + return verified.call(this, err, user, info) }) + } else { + return verified } } @@ -39,8 +40,8 @@ addHook({ if (typeof arguments[0] === 'function') { arguments[0] = wrapVerify(arguments[0], false) } else { - arguments[1] = wrapVerify(arguments[1], arguments[0].passReqToCallback || false) + arguments[1] = wrapVerify(arguments[1], (arguments[0] && arguments[0].passReqToCallback) || false) } - BasicStrategy.apply(this, arguments) + return BasicStrategy.apply(this, arguments) }) }) diff --git a/packages/datadog-instrumentations/src/passport-local.js b/packages/datadog-instrumentations/src/passport-local.js index 873db4ca692..dc67492f2ca 100644 --- a/packages/datadog-instrumentations/src/passport-local.js +++ b/packages/datadog-instrumentations/src/passport-local.js @@ -7,12 +7,13 @@ const passportVerifyChannel = channel('datadog:passport:verify:finish') function wrapVerifiedAndPublish (username, password, verified) { if (passportVerifyChannel.hasSubscribers) { - return shimmer.wrap(verified, (err, user, info) => { - // TODO: check if err and info are really useful - const credentials = { type: 'local', username: username, password: password } - passportVerifyChannel.publish({ credentials, user, err, info }) - return verified(err, user, info) + return shimmer.wrap(verified, function (err, user, info) { + const credentials = { type: 'local', username, password } + passportVerifyChannel.publish({ credentials, user }) + return verified.call(this, err, user, info) }) + } else { + return verified } } @@ -39,8 +40,8 @@ addHook({ if (typeof arguments[0] === 'function') { arguments[0] = wrapVerify(arguments[0], false) } else { - arguments[1] = wrapVerify(arguments[1], arguments[0].passReqToCallback || false) + arguments[1] = wrapVerify(arguments[1], (arguments[0] && arguments[0].passReqToCallback) || false) } - Strategy.apply(this, arguments) + return Strategy.apply(this, arguments) }) }) diff --git a/packages/datadog-instrumentations/test/passport-http.spec.js b/packages/datadog-instrumentations/test/passport-http.spec.js index f2322fd5945..53dcfbe9ecb 100644 --- a/packages/datadog-instrumentations/test/passport-http.spec.js +++ b/packages/datadog-instrumentations/test/passport-http.spec.js @@ -89,9 +89,7 @@ withVersions('passport-http', 'passport-http', version => { expect(subscriberStub).to.be.calledOnceWithExactly( { credentials: { type: 'http', username: 'test', password: '1234' }, - user: { _id: 1, username: 'test', password: '1234', email: 'testuser@ddog.com' }, - err: null, - info: undefined + user: { _id: 1, username: 'test', password: '1234', email: 'testuser@ddog.com' } } ) }) @@ -109,9 +107,7 @@ withVersions('passport-http', 'passport-http', version => { expect(subscriberStub).to.be.calledOnceWithExactly( { credentials: { type: 'http', username: 'test', password: '1' }, - user: false, - err: null, - info: undefined + user: false } ) }) diff --git a/packages/datadog-instrumentations/test/passport-local.spec.js b/packages/datadog-instrumentations/test/passport-local.spec.js index f80db29dd5b..1a5e1e8a6b3 100644 --- a/packages/datadog-instrumentations/test/passport-local.spec.js +++ b/packages/datadog-instrumentations/test/passport-local.spec.js @@ -85,9 +85,7 @@ withVersions('passport-local', 'passport-local', version => { expect(subscriberStub).to.be.calledOnceWithExactly( { credentials: { type: 'local', username: 'test', password: '1234' }, - user: { _id: 1, username: 'test', password: '1234', email: 'testuser@ddog.com' }, - err: null, - info: undefined + user: { _id: 1, username: 'test', password: '1234', email: 'testuser@ddog.com' } } ) }) @@ -100,9 +98,7 @@ withVersions('passport-local', 'passport-local', version => { expect(subscriberStub).to.be.calledOnceWithExactly( { credentials: { type: 'local', username: 'test', password: '1' }, - user: false, - err: null, - info: undefined + user: false } ) }) diff --git a/packages/dd-trace/src/appsec/index.js b/packages/dd-trace/src/appsec/index.js index 787ccdb2e81..974d4840c54 100644 --- a/packages/dd-trace/src/appsec/index.js +++ b/packages/dd-trace/src/appsec/index.js @@ -146,7 +146,7 @@ function onRequestQueryParsed ({ req, res, abortController }) { handleResults(results, req, res, rootSpan, abortController) } -function onPassportVerify ({ credentials, user, err, info }) { +function onPassportVerify ({ credentials, user }) { const store = storage.getStore() const rootSpan = store && store.req && web.root(store.req) @@ -155,7 +155,7 @@ function onPassportVerify ({ credentials, user, err, info }) { return } - passportTrackEvent(credentials, user, err, info, rootSpan, config.appsec.eventTracking.mode) + passportTrackEvent(credentials, user, rootSpan, config.appsec.eventTracking.mode) } function handleResults (actions, req, res, rootSpan, abortController) { diff --git a/packages/dd-trace/src/appsec/passport.js b/packages/dd-trace/src/appsec/passport.js index 654c1ac0e6f..f8a7fbc353c 100644 --- a/packages/dd-trace/src/appsec/passport.js +++ b/packages/dd-trace/src/appsec/passport.js @@ -7,18 +7,14 @@ const { setUserTags } = require('./sdk/set_user') const UUID_PATTERN = '^[0-9A-F]{8}-[0-9A-F]{4}-[1-5][0-9A-F]{3}-[89AB][0-9A-F]{3}-[0-9A-F]{12}$' const regexUsername = new RegExp(UUID_PATTERN, 'i') -const SDK_EVENT_PATTERN = '_dd\\.appsec\\.events\\.[\\W\\w+]+\\.sdk' -const regexSdkEvent = new RegExp(SDK_EVENT_PATTERN, 'i') +const SDK_USER_EVENT_PATTERN = '_dd\\.appsec\\.events\\.users\\.[\\W\\w+]+\\.sdk' +const regexSdkEvent = new RegExp(SDK_USER_EVENT_PATTERN, 'i') function isSdkCalled (tags) { let called = false if (tags && typeof tags === 'object') { - Object.entries(tags).forEach(([key, value]) => { - if (regexSdkEvent.test(key) && value === 'true') { - called = true - } - }) + called = Object.entries(tags).some(([key, value]) => regexSdkEvent.test(key) && value === 'true') } return called @@ -41,8 +37,6 @@ function parseUser (login, passportUser, mode) { user['usr.id'] = passportUser.id } else if (passportUser._id) { user['usr.id'] = passportUser._id - } else { - user['usr.id'] = login } if (mode === 'safe') { @@ -70,7 +64,7 @@ function parseUser (login, passportUser, mode) { return user } -function passportTrackEvent (credentials, passportUser, passportErr, passportInfo, rootSpan, mode) { +function passportTrackEvent (credentials, passportUser, rootSpan, mode) { const tags = rootSpan && rootSpan.context() && rootSpan.context()._tags if (isSdkCalled(tags)) { @@ -86,10 +80,12 @@ function passportTrackEvent (credentials, passportUser, passportErr, passportInf if (passportUser) { // If a passportUser object is published then the login succeded - setUserTags(user['usr.id'], rootSpan) + // TODO : test + setUserTags({ id: user['usr.id'] }, rootSpan) + // Prevent 'usr.id' from being reported again in the metadata + delete user['usr.id'] trackEvent('users.login.success', user, 'passportTrackEvent', rootSpan, mode) } else { - // TODO: handle err and info? trackEvent('users.login.failure', user, 'passportTrackEvent', rootSpan, mode) } } diff --git a/packages/dd-trace/src/config.js b/packages/dd-trace/src/config.js index f6e0f5e4652..358dd4c69c7 100644 --- a/packages/dd-trace/src/config.js +++ b/packages/dd-trace/src/config.js @@ -367,9 +367,10 @@ ken|consumer_?(?:id|key|secret)|sign(?:ed|ature)?|auth(?:entication|orization)?) maybeFile(process.env.DD_APPSEC_HTTP_BLOCKED_TEMPLATE_JSON) ) const DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING = coalesce( + appsec.eventTracking && appsec.eventTracking.mode, process.env.DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING, 'safe' - ) + ).toLowerCase() const remoteConfigOptions = options.remoteConfig || {} const DD_REMOTE_CONFIGURATION_ENABLED = coalesce( @@ -533,7 +534,7 @@ ken|consumer_?(?:id|key|secret)|sign(?:ed|ature)?|auth(?:entication|orization)?) blockedTemplateHtml: DD_APPSEC_HTTP_BLOCKED_TEMPLATE_HTML, blockedTemplateJson: DD_APPSEC_HTTP_BLOCKED_TEMPLATE_JSON, eventTracking: { - enabled: ['extended', 'safe'].some(value => DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING.includes(value)), + enabled: ['extended', 'safe'].some(value => DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING === value), mode: DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING } } diff --git a/packages/dd-trace/test/appsec/index.spec.js b/packages/dd-trace/test/appsec/index.spec.js index ebcddc3fdc2..a40ba2d9d05 100644 --- a/packages/dd-trace/test/appsec/index.spec.js +++ b/packages/dd-trace/test/appsec/index.spec.js @@ -98,7 +98,6 @@ describe('AppSec Index', () => { }) it('should log when enable fails', () => { - // sinon.stub(log, 'error') RuleManager.applyRules.restore() const err = new Error('Invalid Rules') @@ -485,8 +484,6 @@ describe('AppSec Index', () => { expect(passport.passportTrackEvent).to.have.been.calledOnceWithExactly( credentials, user, - err, - info, rootSpan, config.appsec.eventTracking.mode) }) diff --git a/packages/dd-trace/test/appsec/passport.spec.js b/packages/dd-trace/test/appsec/passport.spec.js index cad507def2a..879eff7add8 100644 --- a/packages/dd-trace/test/appsec/passport.spec.js +++ b/packages/dd-trace/test/appsec/passport.spec.js @@ -36,7 +36,7 @@ describe('Passport', () => { describe('passportTrackEvent', () => { it('should call log when credentials is undefined', () => { - passportModule.passportTrackEvent(undefined, undefined, undefined, undefined, undefined, 'safe') + passportModule.passportTrackEvent(undefined, undefined, undefined, 'safe') expect(log.warn).to.have.been.calledOnceWithExactly('No username found in authentication instrumentation') }) @@ -44,7 +44,7 @@ describe('Passport', () => { it('should call log when type is not known', () => { const credentials = { type: 'unknown', username: 'test' } - passportModule.passportTrackEvent(credentials, undefined, undefined, undefined, undefined, 'safe') + passportModule.passportTrackEvent(credentials, undefined, undefined, 'safe') expect(log.warn).to.have.been.calledOnceWithExactly('No username found in authentication instrumentation') }) @@ -52,13 +52,13 @@ describe('Passport', () => { it('should call log when type is known but username not present', () => { const credentials = { type: 'unknown' } - passportModule.passportTrackEvent(credentials, undefined, undefined, undefined, undefined, 'safe') + passportModule.passportTrackEvent(credentials, undefined, undefined, 'safe') expect(log.warn).to.have.been.calledOnceWithExactly('No username found in authentication instrumentation') }) it('should report login failure when passportUser is not present', () => { - passportModule.passportTrackEvent(loginLocal, undefined, undefined, undefined, undefined, 'safe') + passportModule.passportTrackEvent(loginLocal, undefined, undefined, 'safe') expect(setUser.setUserTags).not.to.have.been.called expect(events.trackEvent).to.have.been.calledOnceWithExactly( @@ -71,12 +71,12 @@ describe('Passport', () => { }) it('should report login success when passportUser is present', () => { - passportModule.passportTrackEvent(loginLocal, userUuid, undefined, undefined, rootSpan, 'safe') + passportModule.passportTrackEvent(loginLocal, userUuid, rootSpan, 'safe') - expect(setUser.setUserTags).to.have.been.calledOnceWithExactly(userUuid.id, rootSpan) + expect(setUser.setUserTags).to.have.been.calledOnceWithExactly({ id: userUuid.id }, rootSpan) expect(events.trackEvent).to.have.been.calledOnceWithExactly( 'users.login.success', - { 'usr.id': userUuid.id }, + {}, 'passportTrackEvent', rootSpan, 'safe' @@ -87,12 +87,12 @@ describe('Passport', () => { const user = userUuid user.id = 'publicName' - passportModule.passportTrackEvent(loginLocal, user, undefined, undefined, rootSpan, 'safe') + passportModule.passportTrackEvent(loginLocal, user, rootSpan, 'safe') - expect(setUser.setUserTags).to.have.been.calledOnceWithExactly('', rootSpan) + expect(setUser.setUserTags).to.have.been.calledOnceWithExactly({ id: '' }, rootSpan) expect(events.trackEvent).to.have.been.calledOnceWithExactly( 'users.login.success', - { 'usr.id': '' }, + {}, 'passportTrackEvent', rootSpan, 'safe' @@ -103,12 +103,11 @@ describe('Passport', () => { const user = userUuid user.id = 'publicName' - passportModule.passportTrackEvent(loginLocal, user, undefined, undefined, rootSpan, 'extended') - expect(setUser.setUserTags).to.have.been.calledOnceWithExactly(user.id, rootSpan) + passportModule.passportTrackEvent(loginLocal, user, rootSpan, 'extended') + expect(setUser.setUserTags).to.have.been.calledOnceWithExactly({ id: user.id }, rootSpan) expect(events.trackEvent).to.have.been.calledOnceWithExactly( 'users.login.success', { - 'usr.id': user.id, 'usr.email': user.email, 'usr.username': user.username, 'usr.login': loginLocal.username @@ -119,7 +118,7 @@ describe('Passport', () => { ) }) - it('should not call trackEvent in safe mode if sdk track event functions are already called', () => { + it('should not call trackEvent in safe mode if sdk user event functions are already called', () => { rootSpan.context = () => { return { _tags: { @@ -128,12 +127,12 @@ describe('Passport', () => { } } - passportModule.passportTrackEvent(loginLocal, userUuid, undefined, undefined, rootSpan, 'safe') + passportModule.passportTrackEvent(loginLocal, userUuid, rootSpan, 'safe') expect(setUser.setUserTags).not.to.have.been.called expect(events.trackEvent).not.to.have.been.called }) - it('should not call trackEvent in extended mode if sdk track event functions are already called', () => { + it('should not call trackEvent in extended mode if sdk user event functions are already called', () => { rootSpan.context = () => { return { _tags: { @@ -142,7 +141,44 @@ describe('Passport', () => { } } - passportModule.passportTrackEvent(loginLocal, userUuid, undefined, undefined, rootSpan, 'extended') + passportModule.passportTrackEvent(loginLocal, userUuid, rootSpan, 'extended') + expect(setUser.setUserTags).not.to.have.been.called + expect(events.trackEvent).not.to.have.been.called + }) + + it('should call trackEvent in extended mode if sdk custom event function is already called', () => { + rootSpan.context = () => { + return { + _tags: { + '_dd.appsec.events.custom.event.sdk': 'true' + } + } + } + + passportModule.passportTrackEvent(loginLocal, userUuid, rootSpan, 'extended') + expect(events.trackEvent).to.have.been.calledOnceWithExactly( + 'users.login.success', + { + 'usr.email': userUuid.email, + 'usr.username': userUuid.username, + 'usr.login': loginLocal.username + }, + 'passportTrackEvent', + rootSpan, + 'extended' + ) + }) + + it('should call trackEvent in extended mode if sdk dy called', () => { + rootSpan.context = () => { + return { + _tags: { + '_dd.appsec.events.users.login.success.sdk': 'true' + } + } + } + + passportModule.passportTrackEvent(loginLocal, userUuid, rootSpan, 'extended') expect(setUser.setUserTags).not.to.have.been.called expect(events.trackEvent).not.to.have.been.called }) @@ -156,12 +192,11 @@ describe('Passport', () => { rootSpan.context = () => { return {} } - passportModule.passportTrackEvent(loginLocal, user, undefined, undefined, rootSpan, 'extended') - expect(setUser.setUserTags).to.have.been.calledOnceWithExactly(user._id, rootSpan) + passportModule.passportTrackEvent(loginLocal, user, rootSpan, 'extended') + expect(setUser.setUserTags).to.have.been.calledOnceWithExactly({ id: user._id }, rootSpan) expect(events.trackEvent).to.have.been.calledOnceWithExactly( 'users.login.success', { - 'usr.id': user._id, 'usr.email': user.email, 'usr.username': user.username, 'usr.login': loginLocal.username @@ -180,12 +215,11 @@ describe('Passport', () => { rootSpan.context = () => { return {} } - passportModule.passportTrackEvent(loginLocal, user, undefined, undefined, rootSpan, 'extended') - expect(setUser.setUserTags).to.have.been.calledOnceWithExactly(loginLocal.username, rootSpan) + passportModule.passportTrackEvent(loginLocal, user, rootSpan, 'extended') + expect(setUser.setUserTags).to.have.been.calledOnceWithExactly({ id: loginLocal.username }, rootSpan) expect(events.trackEvent).to.have.been.calledOnceWithExactly( 'users.login.success', { - 'usr.id': loginLocal.username, 'usr.email': user.email, 'usr.username': user.name, 'usr.login': loginLocal.username