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

Modules activities #3057

Merged
merged 21 commits into from
Dec 11, 2023
Merged

Modules activities #3057

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
8bdbbae
Initial version
VeronikaSolovei9 Aug 25, 2023
89509aa
Replaced to generics
VeronikaSolovei9 Aug 25, 2023
03340ba
Minor refactoring
VeronikaSolovei9 Aug 25, 2023
a0c2acb
Working end to end with multiple modules: some restricted and some not
VeronikaSolovei9 Aug 27, 2023
0171679
Refactored duplicated code
VeronikaSolovei9 Aug 28, 2023
e7e03cb
Changed mutation functions to scrubber
VeronikaSolovei9 Aug 30, 2023
97b2639
Remover unnecessary error
VeronikaSolovei9 Aug 30, 2023
d0de9a9
Unit tests
VeronikaSolovei9 Sep 2, 2023
c830408
Imports sort
VeronikaSolovei9 Sep 2, 2023
2d9dfc9
Code clean up
VeronikaSolovei9 Sep 5, 2023
0c4a4db
Merge branch 'master' into modules_activities
VeronikaSolovei9 Sep 5, 2023
a026019
Merge fixes
VeronikaSolovei9 Sep 5, 2023
f2702ae
Merge branch 'master' into modules_activities
VeronikaSolovei9 Sep 12, 2023
90c4f20
Merge branch 'master' into modules_activities
VeronikaSolovei9 Nov 16, 2023
d019263
Merge with master and fixes
VeronikaSolovei9 Nov 17, 2023
30ca359
Merge branch 'master' into modules_activities
VeronikaSolovei9 Nov 28, 2023
e67b1d9
Code clean up
VeronikaSolovei9 Nov 28, 2023
9b71cd2
Interface rename, code clean up
VeronikaSolovei9 Dec 7, 2023
bf08739
Merge branch 'master' into modules_activities
VeronikaSolovei9 Dec 7, 2023
921a52d
Unit tests clean up
VeronikaSolovei9 Dec 8, 2023
fb6be8a
Unit tests clean up
VeronikaSolovei9 Dec 11, 2023
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
2 changes: 2 additions & 0 deletions endpoints/openrtb2/amp_auction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,8 @@ func (deps *endpointDeps) AmpAuction(w http.ResponseWriter, r *http.Request, _ h

activityControl = privacy.NewActivityControl(&account.Privacy)

hookExecutor.SetActivityControl(activityControl)

secGPC := r.Header.Get("Sec-GPC")

auctionRequest := &exchange.AuctionRequest{
Expand Down
2 changes: 2 additions & 0 deletions endpoints/openrtb2/auction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,8 @@ func (deps *endpointDeps) Auction(w http.ResponseWriter, r *http.Request, _ http

activityControl = privacy.NewActivityControl(&account.Privacy)

hookExecutor.SetActivityControl(activityControl)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to call this for other endpoints as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooof, yes, I will add this to amp and video.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added to AMP endpoint only.
In Video endpoint hooks are not configured:
HookExecutor: hookexecution.EmptyHookExecutor{},


ctx := context.Background()

timeout := deps.cfg.AuctionTimeouts.LimitAuctionTimeout(time.Duration(req.TMax) * time.Millisecond)
Expand Down
32 changes: 2 additions & 30 deletions exchange/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"encoding/json"
"errors"
"fmt"
"github.com/prebid/prebid-server/v2/ortb"
"math/rand"
"strings"

Expand All @@ -21,7 +22,6 @@ import (
"github.com/prebid/prebid-server/v2/gdpr"
"github.com/prebid/prebid-server/v2/metrics"
"github.com/prebid/prebid-server/v2/openrtb_ext"
"github.com/prebid/prebid-server/v2/ortb"
"github.com/prebid/prebid-server/v2/privacy"
"github.com/prebid/prebid-server/v2/privacy/ccpa"
"github.com/prebid/prebid-server/v2/privacy/lmt"
Expand Down Expand Up @@ -181,7 +181,7 @@ func (rs *requestSplitter) cleanOpenRTBRequests(ctx context.Context,
// FPD should be applied before policies, otherwise it overrides policies and activities restricted data
applyFPD(auctionReq.FirstPartyData, bidderRequest)

reqWrapper := cloneBidderReq(bidderRequest.BidRequest)
reqWrapper := ortb.CloneBidderReq(bidderRequest.BidRequest)

passIDActivityAllowed := auctionReq.Activities.Allow(privacy.ActivityTransmitUserFPD, scopedName, privacy.NewRequestFromBidRequest(*req))
if !passIDActivityAllowed {
Expand Down Expand Up @@ -238,34 +238,6 @@ func (rs *requestSplitter) cleanOpenRTBRequests(ctx context.Context,
return
}

// cloneBidderReq - clones bidder request and replaces req.User and req.Device with new copies
func cloneBidderReq(req *openrtb2.BidRequest) *openrtb_ext.RequestWrapper {

// bidder request may be modified differently per bidder based on privacy configs
// new request should be created for each bidder request
// pointer fields like User and Device should be cloned and set back to the request copy
var newReq *openrtb2.BidRequest
newReq = ptrutil.Clone(req)

if req.User != nil {
userCopy := ortb.CloneUser(req.User)
newReq.User = userCopy
}

if req.Device != nil {
deviceCopy := ortb.CloneDevice(req.Device)
newReq.Device = deviceCopy
}

if req.Source != nil {
sourceCopy := ortb.CloneSource(req.Source)
newReq.Source = sourceCopy
}

reqWrapper := &openrtb_ext.RequestWrapper{BidRequest: newReq}
return reqWrapper
}

func shouldSetLegacyPrivacy(bidderInfo config.BidderInfos, bidder string) bool {
binfo, defined := bidderInfo[bidder]

Expand Down
12 changes: 7 additions & 5 deletions hooks/hookexecution/context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,17 @@ import (
"github.com/golang/glog"
"github.com/prebid/prebid-server/v2/config"
"github.com/prebid/prebid-server/v2/hooks/hookstage"
"github.com/prebid/prebid-server/v2/privacy"
)

// executionContext holds information passed to module's hook during hook execution.
type executionContext struct {
endpoint string
stage string
accountId string
account *config.Account
moduleContexts *moduleContexts
endpoint string
stage string
accountID string
account *config.Account
moduleContexts *moduleContexts
activityControl privacy.ActivityControl
}

func (ctx executionContext) getModuleContext(moduleName string) hookstage.ModuleInvocationContext {
Expand Down
33 changes: 31 additions & 2 deletions hooks/hookexecution/execution.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,15 @@ package hookexecution
import (
"context"
"fmt"
"github.com/prebid/prebid-server/v2/ortb"
"strings"
"sync"
"time"

"github.com/prebid/prebid-server/v2/hooks"
"github.com/prebid/prebid-server/v2/hooks/hookstage"
"github.com/prebid/prebid-server/v2/metrics"
"github.com/prebid/prebid-server/v2/privacy"
)

type hookResponse[T any] struct {
Expand Down Expand Up @@ -66,10 +68,11 @@ func executeGroup[H any, P any](

for _, hook := range group.Hooks {
mCtx := executionCtx.getModuleContext(hook.Module)
newPayload := handleModuleActivities(hook.Code, executionCtx.activityControl, payload)
wg.Add(1)
go func(hw hooks.HookWrapper[H], moduleCtx hookstage.ModuleInvocationContext) {
defer wg.Done()
executeHook(moduleCtx, hw, payload, hookHandler, group.Timeout, resp, rejected)
executeHook(moduleCtx, hw, newPayload, hookHandler, group.Timeout, resp, rejected)
}(hook, mCtx)
}

Expand Down Expand Up @@ -176,7 +179,7 @@ func handleHookResponse[P any](
metricEngine metrics.MetricsEngine,
) (P, HookOutcome, *RejectError) {
var rejectErr *RejectError
labels := metrics.ModuleLabels{Module: moduleReplacer.Replace(hr.HookID.ModuleCode), Stage: ctx.stage, AccountID: ctx.accountId}
labels := metrics.ModuleLabels{Module: moduleReplacer.Replace(hr.HookID.ModuleCode), Stage: ctx.stage, AccountID: ctx.accountID}
metricEngine.RecordModuleCalled(labels, hr.ExecutionTime)

hookOutcome := HookOutcome{
Expand Down Expand Up @@ -311,3 +314,29 @@ func handleHookMutations[P any](

return payload
}

func handleModuleActivities[P any](hookCode string, activityControl privacy.ActivityControl, payload P) P {
payloadData, ok := any(&payload).(hookstage.RequestUpdater)
if !ok {
return payload
}

scopeGeneral := privacy.Component{Type: privacy.ComponentTypeGeneral, Name: hookCode}
transmitUserFPDActivityAllowed := activityControl.Allow(privacy.ActivityTransmitUserFPD, scopeGeneral, privacy.ActivityRequest{})

if !transmitUserFPDActivityAllowed {
// changes need to be applied to new payload and leave original payload unchanged
bidderReq := payloadData.GetBidderRequestPayload()

bidderReqCopy := ortb.CloneBidderReq(bidderReq.BidRequest)

privacy.ScrubUserFPD(bidderReqCopy)

var newPayload = payload
var np = any(&newPayload).(hookstage.RequestUpdater)
np.SetBidderRequestPayload(bidderReqCopy)
return newPayload
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the point of newPayload? Does it make a shallow copy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At this point we know already that request will be changed for this module (for instance user.ID will be removed).
We create a shallow copy before "Apply" and inside of the "Apply" new User will be created and set back to request so this module will not receive restricted data. We don't do this if activity will not change the request.

      if privacyEnforcement.AnyActivities() {
		// changes need to be applied to new payload and leave original payload unchanged
		bidderReq := payloadData.GetBidderRequestPayload()
		bidderReqCopy := ptrutil.Clone(bidderReq)

		privacyEnforcement.Apply(bidderReqCopy)

		var newPayload = payload
		var np = any(&newPayload).(hookstage.PayloadBidderRequest)
		np.SetBidderRequestPayload(bidderReqCopy)
		return newPayload
	}

Copy link
Collaborator

Choose a reason for hiding this comment

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

So you have a shallow copy of the bidrequest. That shallow copy includes a pointer to the original User object. If you remove the ID from the User object, you will be modifying the original request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What you are saying is correct. In this case User copy will be handled inside of Apply function. It will execute ScrubRequest function and it will make copy of User at the beginning and then set it back to request

var userCopy *openrtb2.User
userCopy = ptrutil.Clone(bidRequest.User)
...
...
bidRequest.User = userCopy
return bidRequest

}

return payload
}
151 changes: 151 additions & 0 deletions hooks/hookexecution/execution_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,151 @@
package hookexecution

import (
"github.com/prebid/openrtb/v19/openrtb2"
"github.com/prebid/prebid-server/v2/config"
"github.com/prebid/prebid-server/v2/hooks/hookstage"
"github.com/prebid/prebid-server/v2/openrtb_ext"
"github.com/prebid/prebid-server/v2/privacy"
"github.com/stretchr/testify/assert"
"testing"
)

func TestHandleModuleActivitiesBidderRequestPayload(t *testing.T) {

testCases := []struct {
description string
hookCode string
privacyConfig *config.AccountPrivacy
inPayloadData hookstage.BidderRequestPayload
expectedPayloadData hookstage.BidderRequestPayload
}{
{
description: "payload should change when userFPD is blocked by activity",
hookCode: "foo",
inPayloadData: hookstage.BidderRequestPayload{
Request: &openrtb_ext.RequestWrapper{BidRequest: &openrtb2.BidRequest{
User: &openrtb2.User{ID: "test_user_id"},
}},
},
privacyConfig: getTransmitUFPDActivityConfig("foo", false),
expectedPayloadData: hookstage.BidderRequestPayload{
Request: &openrtb_ext.RequestWrapper{BidRequest: &openrtb2.BidRequest{
User: &openrtb2.User{ID: ""},
},
}},
},
{
description: "payload should not change when userFPD is not blocked by activity",
hookCode: "foo",
inPayloadData: hookstage.BidderRequestPayload{
Request: &openrtb_ext.RequestWrapper{BidRequest: &openrtb2.BidRequest{
User: &openrtb2.User{ID: "test_user_id"},
}},
},
privacyConfig: getTransmitUFPDActivityConfig("foo", true),
expectedPayloadData: hookstage.BidderRequestPayload{
Request: &openrtb_ext.RequestWrapper{BidRequest: &openrtb2.BidRequest{
User: &openrtb2.User{ID: "test_user_id"},
}},
},
},
}
for _, test := range testCases {
t.Run(test.description, func(t *testing.T) {
//check input payload didn't change
origInPayloadData := test.inPayloadData
activityControl := privacy.NewActivityControl(test.privacyConfig)
newPayload := handleModuleActivities(test.hookCode, activityControl, test.inPayloadData)
assert.Equal(t, test.expectedPayloadData.Request.BidRequest, newPayload.Request.BidRequest)
assert.Equal(t, origInPayloadData, test.inPayloadData)
})
}
}

func TestHandleModuleActivitiesProcessedAuctionRequestPayload(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like processed auction request works because the payload type is able to be casted to the bidder request payload type, is that correct?
Are these the only two payload types we care about because they both deal with a bid request?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, ProcessedAuctionRequestPayload and BidderRequestPayload are only payloads with BidderRequest and they both implement PayloadBidderRequest interface.
This makes them to pass type check here payloadData, ok := any(&payload).(hookstage.PayloadBidderRequest)


testCases := []struct {
description string
hookCode string
privacyConfig *config.AccountPrivacy
inPayloadData hookstage.ProcessedAuctionRequestPayload
expectedPayloadData hookstage.ProcessedAuctionRequestPayload
}{
{
description: "payload should change when userFPD is blocked by activity",
hookCode: "foo",
inPayloadData: hookstage.ProcessedAuctionRequestPayload{
Request: &openrtb_ext.RequestWrapper{BidRequest: &openrtb2.BidRequest{
User: &openrtb2.User{ID: "test_user_id"},
}},
},
privacyConfig: getTransmitUFPDActivityConfig("foo", false),
expectedPayloadData: hookstage.ProcessedAuctionRequestPayload{
Request: &openrtb_ext.RequestWrapper{BidRequest: &openrtb2.BidRequest{
User: &openrtb2.User{ID: ""},
}},
},
},
{
description: "payload should not change when userFPD is not blocked by activity",
hookCode: "foo",
inPayloadData: hookstage.ProcessedAuctionRequestPayload{
Request: &openrtb_ext.RequestWrapper{BidRequest: &openrtb2.BidRequest{
User: &openrtb2.User{ID: "test_user_id"},
}},
},
privacyConfig: getTransmitUFPDActivityConfig("foo", true),
expectedPayloadData: hookstage.ProcessedAuctionRequestPayload{
Request: &openrtb_ext.RequestWrapper{BidRequest: &openrtb2.BidRequest{
User: &openrtb2.User{ID: "test_user_id"},
}},
},
},
}
for _, test := range testCases {
t.Run(test.description, func(t *testing.T) {
//check input payload didn't change
origInPayloadData := test.inPayloadData
activityControl := privacy.NewActivityControl(test.privacyConfig)
newPayload := handleModuleActivities(test.hookCode, activityControl, test.inPayloadData)
assert.Equal(t, test.expectedPayloadData.Request.BidRequest, newPayload.Request.BidRequest)
assert.Equal(t, origInPayloadData, test.inPayloadData)
})
}
}

func TestHandleModuleActivitiesNoBidderRequestPayload(t *testing.T) {

testCases := []struct {
description string
hookCode string
privacyConfig *config.AccountPrivacy
inPayloadData hookstage.RawAuctionRequestPayload
expectedPayloadData hookstage.RawAuctionRequestPayload
}{
{
description: "payload should change when userFPD is blocked by activity",
hookCode: "foo",
inPayloadData: hookstage.RawAuctionRequestPayload{},
privacyConfig: getTransmitUFPDActivityConfig("foo", false),
expectedPayloadData: hookstage.RawAuctionRequestPayload{},
},
{
description: "payload should not change when userFPD is not blocked by activity",
hookCode: "foo",
inPayloadData: hookstage.RawAuctionRequestPayload{},
privacyConfig: getTransmitUFPDActivityConfig("foo", true),
expectedPayloadData: hookstage.RawAuctionRequestPayload{},
},
}
for _, test := range testCases {
t.Run(test.description, func(t *testing.T) {
//check input payload didn't change
origInPayloadData := test.inPayloadData
activityControl := privacy.NewActivityControl(test.privacyConfig)
newPayload := handleModuleActivities(test.hookCode, activityControl, test.inPayloadData)
assert.Equal(t, test.expectedPayloadData, newPayload)
assert.Equal(t, origInPayloadData, test.inPayloadData)
})
}
}
34 changes: 22 additions & 12 deletions hooks/hookexecution/executor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"github.com/prebid/prebid-server/v2/hooks/hookstage"
"github.com/prebid/prebid-server/v2/metrics"
"github.com/prebid/prebid-server/v2/openrtb_ext"
"github.com/prebid/prebid-server/v2/privacy"
)

const (
Expand Down Expand Up @@ -43,17 +44,19 @@ type StageExecutor interface {
type HookStageExecutor interface {
StageExecutor
SetAccount(account *config.Account)
SetActivityControl(activityControl privacy.ActivityControl)
GetOutcomes() []StageOutcome
}

type hookExecutor struct {
account *config.Account
accountID string
endpoint string
planBuilder hooks.ExecutionPlanBuilder
stageOutcomes []StageOutcome
moduleContexts *moduleContexts
metricEngine metrics.MetricsEngine
account *config.Account
accountID string
endpoint string
planBuilder hooks.ExecutionPlanBuilder
stageOutcomes []StageOutcome
moduleContexts *moduleContexts
metricEngine metrics.MetricsEngine
activityControl privacy.ActivityControl
// Mutex needed for BidderRequest and RawBidderResponse Stages as they are run in several goroutines
sync.Mutex
}
Expand All @@ -77,6 +80,10 @@ func (e *hookExecutor) SetAccount(account *config.Account) {
e.accountID = account.ID
}

func (e *hookExecutor) SetActivityControl(activityControl privacy.ActivityControl) {
e.activityControl = activityControl
}

func (e *hookExecutor) GetOutcomes() []StageOutcome {
return e.stageOutcomes
}
Expand Down Expand Up @@ -290,11 +297,12 @@ func (e *hookExecutor) ExecuteAuctionResponseStage(response *openrtb2.BidRespons

func (e *hookExecutor) newContext(stage string) executionContext {
return executionContext{
account: e.account,
accountId: e.accountID,
endpoint: e.endpoint,
moduleContexts: e.moduleContexts,
stage: stage,
account: e.account,
accountID: e.accountID,
endpoint: e.endpoint,
moduleContexts: e.moduleContexts,
stage: stage,
activityControl: e.activityControl,
}
}

Expand All @@ -316,6 +324,8 @@ type EmptyHookExecutor struct{}

func (executor EmptyHookExecutor) SetAccount(_ *config.Account) {}

func (executor EmptyHookExecutor) SetActivityControl(_ privacy.ActivityControl) {}

func (executor EmptyHookExecutor) GetOutcomes() []StageOutcome {
return []StageOutcome{}
}
Expand Down
Loading