-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add BidstreamClient and SharingClient, deprecate UID2Client #63
Add BidstreamClient and SharingClient, deprecate UID2Client #63
Conversation
@@ -44,4 +44,8 @@ public enum DecryptionStatus { | |||
* to decrypt a UID2 token. Ensure the factory class matches the token type you are decrypting. | |||
*/ | |||
INVALID_IDENTITY_SCOPE, | |||
/** | |||
* INVALID_TOKEN_LIFETIME: If token generated timestamp is longer than allow_clock_skew_seconds |
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 comment is not correct, this error should occur when the token's lifetime is greater than is allowed
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.
It's not correct to say "longer than allow_clock_skew_seconds"; that doesn't cause this error.
We can write here: Token has invalid timestamps
.
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.
My bad, that makes sense.
@@ -104,7 +92,12 @@ public void crossPlatformConsistencyCheck_Decrypt() throws Exception { | |||
assertEquals(EXAMPLE_UID, res.getUid()); | |||
} | |||
|
|||
private static void validateAdvertisingToken(String advertisingTokenString, IdentityScope identityScope, IdentityType identityType) { | |||
public static void validateAdvertisingToken(String advertisingTokenString, IdentityScope identityScope, IdentityType identityType, int tokenVersion) { | |||
if (tokenVersion == 2) { |
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 an enum for token versions?
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 created a new class called TokenVersionForTesting for the token version
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 ok why do we do it differently than 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 feel they are the same. in the C#, "TokenVersion" class is also only used for testing. here
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, just a couple of minor comments
assertTrue(response.isSuccess()); | ||
assertEquals(EXAMPLE_UID, response.getUid()); | ||
assertEquals(tokenVersion.ordinal() + 2, response.getAdvertisingTokenVersion()); | ||
assertEquals(tokenVersion.ordinal() + 2, response.getAdvertisingTokenVersion()); |
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.
duplicate assertion
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.
My bad, will remove one.
Introduce two new clients BidStreamClient and SharingClient and deprecate the existing UID2Client.