-
Notifications
You must be signed in to change notification settings - Fork 3
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
Syw UID2-3679 cpp sdk base64 v4 token #26
Conversation
sunnywu
commented
Aug 21, 2024
- Able to handle Base64 (non url friendly) encoded v4 ad token decryption and added a unit test
- I use Clion/Docker/Mac OS setup so added some readme entries how people can replicate such setup easily
@@ -45,7 +45,8 @@ DecryptionResult DecryptToken(const std::string& token, const KeyContainer& keys | |||
} | |||
|
|||
const std::string headerStr = token.substr(0, 4); | |||
const bool isBase64UrlEncoding = std::any_of(headerStr.begin(), headerStr.end(), [](char c) { return c == '-' || c == '_'; }); | |||
// check the whole ad token string instead of the headerStr to make sure |
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.
nit - headerStr
should be moved to where it's first used (above line 54)
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.
fixed
uid2::UID2Base64UrlCoder::Decode(token, encryptedId); | ||
if (isBase64UrlEncoding) { | ||
// same as V3 but use Base64URL encoding | ||
uid2::UID2Base64UrlCoder::Decode(token, encryptedId); |
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.
This is different from the Java SDK which converts the token to to base64 and then calls decryptV3.
Is that intentional?
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.
not intentional but i just chose the easiest way - don't necessarily see the need to clone the exact same logic as long as we have unit tests to cover it?
test/encryption_tests_v4.cpp
Outdated
{ | ||
UID2Client client("ep", "ak", CLIENT_SECRET, IdentityScope::UID2); | ||
client.RefreshJson(KeySetToJson({MASTER_KEY, SITE_KEY})); | ||
auto advertisingToken = GenerateUid2TokenV4AndValidate(EXAMPLE_UID, MASTER_KEY, SITE_ID, SITE_KEY, EncryptTokenParams()); |
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.
can we use do/while so we only have 1 GenerateUid2TokenV4AndValidate
like in C# SDK?
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.
no i tried that initially
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.
what was the issue / error?
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.
hmm i changed to do the base64 char check inline within while condition and now it works
EXPECT_TRUE(isBase64UrlEncoding); | ||
|
||
std::vector<std::uint8_t> adTokenBytes; | ||
uid2::UID2Base64UrlCoder::Decode(advertisingToken, adTokenBytes); |
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 any of the below code duplicated from the other tests?
Worth making a 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.
done some refactoring to have DecryptAndAssertSuccess method like other SDK
@@ -116,15 +117,46 @@ std::string GenerateUid2TokenV4AndValidate( | |||
return advertisingToken; | |||
} | |||
|
|||
void DecryptAndAssertSuccess(UID2Client& client, const std::string& advertisingTokenString, Timestamp timestamp = Timestamp::Now()) |
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.
does const UID2Client&
work?
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.
tried but no
test/encryption_tests_v4.cpp
Outdated
advertisingToken = GenerateUid2TokenV4AndValidate(EXAMPLE_UID, MASTER_KEY, SITE_ID, SITE_KEY, EncryptTokenParams()); | ||
} while (!std::any_of(advertisingToken.begin(), advertisingToken.end(), [](char c) { return c == '-' || c == '_'; })); | ||
|
||
const bool isBase64UrlEncoding = std::any_of(advertisingToken.begin(), advertisingToken.end(), [](char c) { return c == '-' || c == '_'; }); |
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 think we don't need this anymore because of the while condition