-
Notifications
You must be signed in to change notification settings - Fork 862
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
Implement Verifiable Advertiser Conversions for Brave Ads #7524
Conversation
@moritzhaller please open a security review for this when it's ready, thx |
04daa77
to
354fb3f
Compare
e5fd0db
to
81c95c6
Compare
4e2dc75
to
8142fc5
Compare
663232c
to
43e47e6
Compare
vendor/bat-native-ads/src/bat/ads/internal/security/conversions/conversions_util.cc
Outdated
Show resolved
Hide resolved
vendor/bat-native-ads/src/bat/ads/internal/security/conversions/conversions_util.cc
Outdated
Show resolved
Hide resolved
vendor/bat-native-ads/src/bat/ads/internal/security/conversions/conversions_util.cc
Outdated
Show resolved
Hide resolved
vendor/bat-native-ads/src/bat/ads/internal/security/conversions/conversions_util.cc
Outdated
Show resolved
Hide resolved
vendor/bat-native-ads/src/bat/ads/internal/security/conversions/conversions_util.cc
Outdated
Show resolved
Hide resolved
...-native-ads/src/bat/ads/internal/security/conversions/verifiable_conversion_envelope_info.cc
Show resolved
Hide resolved
vendor/bat-native-ads/src/bat/ads/internal/security/crypto_util_unittest.cc
Show resolved
Hide resolved
03058a4
to
04f8a48
Compare
@moritzhaller A note on seeds in this library: Technically TweetNaCL allows you to "pre-seed" if you want; however, I want to avoid this as much as possible to prevent implementors downstream from incorrectly seeding (just let the library call Random). |
@orspetol totally agree for future implementations, and as discussed we need to keep the seed in the utils for now to ensure key parity between ads and rewards. |
256f2f7
to
cb0f937
Compare
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.
LGTM++
vendor/bat-native-ads/src/bat/ads/internal/account/confirmations/confirmations.cc
Show resolved
Hide resolved
vendor/bat-native-ads/src/bat/ads/internal/security/conversions/conversions_util_unittest.cc
Outdated
Show resolved
Hide resolved
...bat-native-ads/src/bat/ads/internal/tokens/redeem_unblinded_token/create_confirmation_util.h
Outdated
Show resolved
Hide resolved
cb0f937
to
c6a4b84
Compare
c6a4b84
to
bc2732b
Compare
a751c1b
to
be0ef59
Compare
abdbabf
to
660a65a
Compare
e278260
to
f81def3
Compare
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.
iOS lgtm
378fc28
to
d4d822f
Compare
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.
LGTM
175f4fc
to
adcdfe4
Compare
adcdfe4
to
0e2b4b1
Compare
CI failed due to known audit deps |
Resolves brave/brave-browser#13368
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
,npm run lint
,npm run gn_check
,npm run tslint
git rebase master
(if needed).Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on.
Test Plan:
start browser on fresh profile with
npm start -- --enable-logging=stderr --vmodule="*/bat-native-ads/*"=6,"*/brave_ads/*"=6,"*/brave_user_model/*"=6,"*/bat_ads/*"=6 --brave-ads-staging --rewards=staging=true --brave-ads-debug
run test site with conversion tag (e.g.
<meta name="ad-conversion-id" content="smartbrownfoxes42">
) onlocalhost:8000
add test "untargeted" ad with conversion ID and test key to catalog and inject via proxy, e.g.
trigger add of segment "untargeted"
navigate to
localhost:8000
and verify that a conversion was triggered with logs