-
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
UID2-3243 token lifetime check fix for token v2 #66
UID2-3243 token lifetime check fix for token v2 #66
Conversation
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.
some minor comments in tests. Otherwise LGTM
callAndVerifyRefreshJson(identityScope); | ||
public void smokeTestForBidstream(IdentityScope identityScope, TokenVersionForTesting tokenVersion) throws Exception { | ||
Instant now = Instant.now(); | ||
String advertisingToken = AdvertisingTokenBuilder.builder().withScope(identityScope).withVersion(tokenVersion).withEstablished(now.minus(120, ChronoUnit.DAYS)).withGenerated(now.plus(-1, ChronoUnit.DAYS)).withExpiry(now.plus(2, ChronoUnit.DAYS)).build(); |
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.
nitpick: better to say now.minus(1, ChronoUnit.DAYS)
instead of now.plus(-1, ChronoUnit.DAYS)
callAndVerifyRefreshJson(identityScope); | ||
public void smokeTestForBidstream(IdentityScope identityScope, TokenVersionForTesting tokenVersion) throws Exception { | ||
Instant now = Instant.now(); | ||
String advertisingToken = AdvertisingTokenBuilder.builder().withScope(identityScope).withVersion(tokenVersion).withEstablished(now.minus(120, ChronoUnit.DAYS)).withGenerated(now.plus(-1, ChronoUnit.DAYS)).withExpiry(now.plus(2, ChronoUnit.DAYS)).build(); |
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.
120 days in the past is unrealistic for bidstream tokens. Probably 3 days in the past is normal
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 true, we actually get tokens that are generated months ago and are refreshed regularly. That's why we're making this PR. Feel free to add that as a comment if it's not clear though.
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.
that's right. I mixed up established
and generated
Token lifetime check fix for token v2.
Did Unit tests & integration tests locally.