-
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 13 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 |
---|---|---|
|
@@ -103,6 +103,11 @@ func GetAccount(ctx context.Context, cfg *config.Configuration, fetcher stored_r | |
return nil, errs | ||
} | ||
|
||
errs = account.Privacy.IPMasking.Validate(errs) | ||
if len(errs) > 0 { | ||
return nil, errs | ||
} | ||
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 |
---|---|---|
|
@@ -21,6 +21,11 @@ const ( | |
ChannelWeb ChannelType = "web" | ||
) | ||
|
||
const ( | ||
IPv4BitSize = 32 | ||
IPv6BitSize = 128 | ||
) | ||
SyntaxNode marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// Account represents a publisher account configuration | ||
type Account struct { | ||
ID string `mapstructure:"id" json:"id"` | ||
|
@@ -40,7 +45,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 +300,36 @@ func (a *AccountChannel) IsSet() bool { | |
} | ||
|
||
type AccountPrivacy struct { | ||
AllowActivities AllowActivities `mapstructure:"allowactivities" json:"allowactivities"` | ||
AllowActivities *AllowActivities `mapstructure:"allowactivities" json:"allowactivities"` | ||
IPMasking IPMasking `mapstructure:"ip_masking" json:"ip_masking"` | ||
} | ||
|
||
type IPMasking struct { | ||
IPv6 IPMasks `mapstructure:"ipv6" json:"ipv6"` | ||
IPv4 IPMasks `mapstructure:"ipv4" json:"ipv4"` | ||
} | ||
|
||
type IPMasks struct { | ||
LeftMaskBits int `mapstructure:"left_mask_bits" json:"left_mask_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 (ipMasking *IPMasking) Validate(errs []error) []error { | ||
if e := ipMasking.IPv6.validate(IPv6BitSize, "ipv6"); e != nil { | ||
errs = append(errs, e...) | ||
} | ||
if e := ipMasking.IPv4.validate(IPv4BitSize, "ipv4"); e != nil { | ||
errs = append(errs, e...) | ||
} | ||
return errs | ||
} | ||
|
||
func (ipMasks *IPMasks) validate(maxBits int, ipVersion string) []error { | ||
var errs []error | ||
|
||
if ipMasks.LeftMaskBits > maxBits || ipMasks.LeftMaskBits < 0 { | ||
err := fmt.Errorf("left mask bits cannot exceed %d in %s address, or be less than 0", maxBits, ipVersion) | ||
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.
Is there a reasonable way to add a test that throws this error? Or is it challenging with this Validate function?
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.
Yes, added!