Skip to content

Commit

Permalink
Fix PR comments.
Browse files Browse the repository at this point in the history
  • Loading branch information
@hoolioh
hoolioh committed Jun 5, 2023
1 parent d898cbd commit 1595f8b
Show file tree
Hide file tree
Showing 9 changed files with 89 additions and 67 deletions.
13 changes: 7 additions & 6 deletions packages/datadog-instrumentations/src/passport-http.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}

Expand All @@ -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)
})
})
15 changes: 8 additions & 7 deletions packages/datadog-instrumentations/src/passport-local.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}

Expand All @@ -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)
})
})
8 changes: 2 additions & 6 deletions packages/datadog-instrumentations/test/passport-http.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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' }
}
)
})
Expand All @@ -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
}
)
})
Expand Down
8 changes: 2 additions & 6 deletions packages/datadog-instrumentations/test/passport-local.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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' }
}
)
})
Expand All @@ -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
}
)
})
Expand Down
4 changes: 2 additions & 2 deletions packages/dd-trace/src/appsec/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand All @@ -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) {
Expand Down
20 changes: 8 additions & 12 deletions packages/dd-trace/src/appsec/passport.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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') {
Expand Down Expand Up @@ -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)) {
Expand All @@ -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)
}
}
Expand Down
5 changes: 3 additions & 2 deletions packages/dd-trace/src/config.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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
}
}
Expand Down
3 changes: 0 additions & 3 deletions packages/dd-trace/test/appsec/index.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down Expand Up @@ -485,8 +484,6 @@ describe('AppSec Index', () => {
expect(passport.passportTrackEvent).to.have.been.calledOnceWithExactly(
credentials,
user,
err,
info,
rootSpan,
config.appsec.eventTracking.mode)
})
Expand Down
80 changes: 57 additions & 23 deletions packages/dd-trace/test/appsec/passport.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,29 +36,29 @@ 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')
})

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')
})

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(
Expand All @@ -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'
Expand All @@ -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'
Expand All @@ -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
Expand All @@ -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: {
Expand All @@ -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: {
Expand All @@ -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
})
Expand All @@ -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
Expand All @@ -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
Expand Down

0 comments on commit 1595f8b

Please sign in to comment.