-
Notifications
You must be signed in to change notification settings - Fork 750
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
IP scrubber #2990
IP scrubber #2990
Changes from 19 commits
73cc1fe
449f53b
0f3dd97
8a0179b
6e71825
56ecfa5
89cc995
f3aef01
3d47811
7a030e3
5020713
9e19dd8
0f3c0ee
c48c25a
0858cf3
a6c4e18
82214f0
cd709cc
1332bd0
83fbd9d
a40290f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,14 +4,14 @@ import ( | |
"context" | ||
"encoding/json" | ||
"fmt" | ||
|
||
"github.com/buger/jsonparser" | ||
"github.com/prebid/go-gdpr/consentconstants" | ||
"github.com/prebid/prebid-server/config" | ||
"github.com/prebid/prebid-server/errortypes" | ||
"github.com/prebid/prebid-server/metrics" | ||
"github.com/prebid/prebid-server/openrtb_ext" | ||
"github.com/prebid/prebid-server/stored_requests" | ||
"github.com/prebid/prebid-server/util/iputil" | ||
jsonpatch "gopkg.in/evanphx/json-patch.v4" | ||
) | ||
|
||
|
@@ -103,6 +103,20 @@ func GetAccount(ctx context.Context, cfg *config.Configuration, fetcher stored_r | |
return nil, errs | ||
} | ||
|
||
var ipv6Errs []error | ||
ipv6Errs = account.Privacy.IPv6Config.Validate(ipv6Errs) | ||
if len(ipv6Errs) > 0 { | ||
account.Privacy.IPv6Config.AnonKeepBits = iputil.IPv6BitSize | ||
// record invalid ipv6 metric | ||
} | ||
SyntaxNode marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
var ipv4Errs []error | ||
ipv4Errs = account.Privacy.IPv4Config.Validate(ipv4Errs) | ||
if len(ipv4Errs) > 0 { | ||
account.Privacy.IPv4Config.AnonKeepBits = iputil.IPv4BitSize | ||
// record invalid ipv4 metric | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same point about removing metric related comment |
||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpick: Could simplify the syntax...
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These lines show as having test coverage, but there doesn't seem to an assertion on setting their default values. I made a type locally where There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this is tested in a general GetAccount test.
For the simplified syntax I added new variables for errors to show explicitly we don't want to add IP config validation errors to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And also tested this update end to end including test with publisher account id. |
||
|
||
// set the value of events.enabled field based on deprecated events_enabled field and ensure backward compatibility | ||
deprecateEventsEnabledField(account) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ import ( | |
|
||
"github.com/prebid/go-gdpr/consentconstants" | ||
"github.com/prebid/prebid-server/openrtb_ext" | ||
"github.com/prebid/prebid-server/util/iputil" | ||
) | ||
|
||
// ChannelType enumerates the values of integrations Prebid Server can configure for an account | ||
|
@@ -40,7 +41,7 @@ type Account struct { | |
Validations Validations `mapstructure:"validations" json:"validations"` | ||
DefaultBidLimit int `mapstructure:"default_bid_limit" json:"default_bid_limit"` | ||
BidAdjustments *openrtb_ext.ExtRequestPrebidBidAdjustments `mapstructure:"bidadjustments" json:"bidadjustments"` | ||
Privacy *AccountPrivacy `mapstructure:"privacy" json:"privacy"` | ||
Privacy AccountPrivacy `mapstructure:"privacy" json:"privacy"` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For my understanding, why is this converted to remove its pointer? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I changed it to non-pointer because we no have a new type AccountPrivacy struct {
AllowActivities *AllowActivities `mapstructure:"allowactivities" json:"allowactivities"`
IPMasking IPMasking `mapstructure:"ip_masking" json:"ip_masking"`
} We need to have IPMasking to be always present and to be populated with default values from configs. This is why config.config.go has these lines: v.SetDefault("account_defaults.privacy.ip_masking.ipv6.left_mask_bits", 56)
v.SetDefault("account_defaults.privacy.ip_masking.ipv4.left_mask_bits", 24) |
||
} | ||
|
||
// CookieSync represents the account-level defaults for the cookie sync endpoint. | ||
|
@@ -295,5 +296,31 @@ func (a *AccountChannel) IsSet() bool { | |
} | ||
|
||
type AccountPrivacy struct { | ||
AllowActivities AllowActivities `mapstructure:"allowactivities" json:"allowactivities"` | ||
AllowActivities *AllowActivities `mapstructure:"allowactivities" json:"allowactivities"` | ||
IPv6Config IPv6 `mapstructure:"ipv6" json:"ipv6"` | ||
IPv4Config IPv4 `mapstructure:"ipv4" json:"ipv4"` | ||
} | ||
|
||
type IPv6 struct { | ||
AnonKeepBits int `mapstructure:"anon-keep-bits" json:"anon-keep-bits"` | ||
} | ||
|
||
type IPv4 struct { | ||
AnonKeepBits int `mapstructure:"anon-keep-bits" json:"anon-keep-bits"` | ||
} | ||
SyntaxNode marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpick: Let's use underscores to be consistent with the rest of this object: |
||
|
||
func (ip *IPv6) Validate(errs []error) []error { | ||
if ip.AnonKeepBits > iputil.IPv6BitSize || ip.AnonKeepBits < 0 { | ||
err := fmt.Errorf("bits cannot exceed %d in ipv6 address, or be less than 0", iputil.IPv6BitSize) | ||
errs = append(errs, err) | ||
} | ||
return errs | ||
} | ||
|
||
func (ip *IPv4) Validate(errs []error) []error { | ||
if ip.AnonKeepBits > iputil.IPv4BitSize || ip.AnonKeepBits < 0 { | ||
err := fmt.Errorf("bits cannot exceed %d in ipv4 address, or be less than 0", iputil.IPv4BitSize) | ||
errs = append(errs, err) | ||
} | ||
return errs | ||
} | ||
SyntaxNode marked this conversation as resolved.
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you commented asking if this can be done in a separate PR, if so can this comment be removed?