[UID2-3242] Handle lifetime check for v2 tokens differently #36

asloobq · 2024-04-25T18:24:07Z

No description provided.

caroline-ttd · 2024-04-26T00:39:03Z

uid2_client/encryption.py


-    if not _token_has_valid_lifetime(keys, client_type, established, expires, now):
+    if not _token_has_valid_lifetime(keys, client_type, now, expires, now):
        return DecryptedToken(DecryptionStatus.INVALID_TOKEN_LIFETIME, id_str, established, site_id, site_key.site_id,


Probably it would be good to change the established in the error to now

it's correct currently because DecryptedToken has an established field. Recall that DecryptedToken is not just an error, but also shows the fields that were successfully extracted from the token.

caroline-ttd · 2024-04-26T00:52:51Z

uid2_client/uid2_token_generator.py


        master_payload = int.to_bytes(int(params.token_expiry.timestamp()) * 1000, length=8, byteorder='big')  # expiry
-        master_payload += int.to_bytes(generated_at_timestamp, length=8, byteorder='big')  # created
+        master_payload += int.to_bytes(generated_at_timestamp, length=8, byteorder='big')  # generated


Nitpick, do you want to write the same comment as C# SDK here, //identity refreshed, seems to be identical to TokenGenerated in Operator to provide more context.

I think its understood from the # generated comment that its identical to last refreshed/generated in site payload :)

caroline-ttd · 2024-04-26T00:55:14Z

uid2_client/uid2_token_generator.py

    @staticmethod
    def generate_uid_token(id_str, master_key, site_id, site_key, identity_scope, token_version,
-                           created_at=None, expires_at=None):
+                           identity_established=None, token_generated=None, token_expiry=None):


Just curious, why the default value is not datetime.now() but None

@caroline-ttd Default values are only evaluated once.

caroline-ttd · 2024-04-26T01:00:13Z

uid2_client/encryption.py


-    if not _token_has_valid_lifetime(keys, client_type, established, expires, now):
+    if not _token_has_valid_lifetime(keys, client_type, generated, expires, now):
        return DecryptedToken(DecryptionStatus.INVALID_TOKEN_LIFETIME, None, established, site_id, site_key.site_id,


Probably it would be good to change the established in the error to generated

mcollins-ttd · 2024-04-26T01:56:09Z

uid2_client/uid2_token_generator.py

-                 identity_scope=IdentityScope.UID2.value, token_generated_at=dt.datetime.now(tz=timezone.utc)):
-        self.identity_scope = identity_scope
+                 identity_scope=IdentityScope.UID2.value, token_generated=dt.datetime.now(tz=timezone.utc),
+                 identity_established=dt.datetime.now(tz=timezone.utc)):


Shouldn't expiry, token_generated, and identity_established have a default value of None, and we call datetime.now in the function instead?

agree, I assume the reason no test has failed is because we're not relying on these defaults other than in test code

good catch. Fixed it

mcollins-ttd · 2024-04-26T04:44:29Z

uid2_client/uid2_token_generator.py


    @staticmethod
-    def generate_uid2_token_v2(id_str, master_key, site_id, site_key, params = default_params(), version=2):
+    def generate_uid2_token_v2(id_str, master_key, site_id, site_key, params=default_params(), version=2):


default_params() will only be evaluated once, is that going to cause any problems?

got rid of this method

mcollins-ttd · 2024-04-26T04:47:08Z

uid2_client/uid2_token_generator.py

    @staticmethod
    def generate_uid_token(id_str, master_key, site_id, site_key, identity_scope, token_version,
-                           created_at=None, expires_at=None):
+                           identity_established=None, token_generated=None, token_expiry=None):


@caroline-ttd Default values are only evaluated once.

tests/test_bidstream_client.py

jon8787 · 2024-04-26T07:28:39Z

uid2_client/encryption.py


-    if not _token_has_valid_lifetime(keys, client_type, established, expires, now):
+    if not _token_has_valid_lifetime(keys, client_type, now, expires, now):
        return DecryptedToken(DecryptionStatus.INVALID_TOKEN_LIFETIME, id_str, established, site_id, site_key.site_id,


it's correct currently because DecryptedToken has an established field. Recall that DecryptedToken is not just an error, but also shows the fields that were successfully extracted from the token.

jon8787 · 2024-04-26T07:32:13Z

uid2_client/encryption.py


-    if not _token_has_valid_lifetime(keys, client_type, established, expires, now):
+    if not _token_has_valid_lifetime(keys, client_type, generated, expires, now):
        return DecryptedToken(DecryptionStatus.INVALID_TOKEN_LIFETIME, None, established, site_id, site_key.site_id,


jon8787 · 2024-04-26T07:37:32Z

uid2_client/uid2_token_generator.py

-                 identity_scope=IdentityScope.UID2.value, token_generated_at=dt.datetime.now(tz=timezone.utc)):
-        self.identity_scope = identity_scope
+                 identity_scope=IdentityScope.UID2.value, token_generated=dt.datetime.now(tz=timezone.utc),
+                 identity_established=dt.datetime.now(tz=timezone.utc)):


agree, I assume the reason no test has failed is because we're not relying on these defaults other than in test code

mcollins-ttd · 2024-04-28T23:40:50Z

tests/test_bidstream_client.py

-        expires_in_sec = IN_3_DAYS + dt.timedelta(minutes=1)
+    def test_token_lifetime_too_long_for_bidstream_but_remaining_lifetime_allowed(self):  # TokenLifetimeTooLongForBidstreamButRemainingLifetimeAllowed
+        generated = YESTERDAY
+        expires_in_sec = generated + dt.timedelta(days=3) + dt.timedelta(minutes=1)


Can we extract a variable for max bidstream lifetime from here, line 134, and line 33 so we can see the connection

mcollins-ttd · 2024-04-28T23:43:04Z

tests/test_sharing_client.py


+    def test_token_lifetime_too_long_for_sharing_but_remaining_lifetime_allowed(self):  # TokenLifetimeTooLongForSharingButRemainingLifetimeAllowed
+        generated = YESTERDAY
+        expires_in_sec = generated + dt.timedelta(days=31)


Similarly, can we extract a variable for max sharing lifetime in this class

asloobq added 4 commits April 25, 2024 11:07

Handle lifetime check for v2 tokens differently

825dcea

refactor and rename arguments

22c17e8

Add unit tests

aec489d

refactor

c3b930f

asloobq marked this pull request as ready for review April 25, 2024 21:18

caroline-ttd reviewed Apr 26, 2024

View reviewed changes

caroline-ttd approved these changes Apr 26, 2024

View reviewed changes

mcollins-ttd reviewed Apr 26, 2024

View reviewed changes

tests/test_bidstream_client.py Show resolved Hide resolved

jon8787 approved these changes Apr 26, 2024

View reviewed changes

Fix issues due to resolving default arguments

5efe9ad

mcollins-ttd reviewed Apr 28, 2024

View reviewed changes

update tests

ac62ec8

asloobq merged commit 2d7002c into main Apr 29, 2024

asloobq deleted the aaq-UID2-3242-dsp-check-token-lifetime-v2-handling branch April 29, 2024 21:11

[UID2-3242] Handle lifetime check for v2 tokens differently #36

[UID2-3242] Handle lifetime check for v2 tokens differently #36

Uh oh!

Conversation

asloobq commented Apr 25, 2024

Uh oh!

caroline-ttd Apr 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asloobq Apr 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

caroline-ttd Apr 26, 2024 •

edited

Loading

asloobq Apr 26, 2024 •

edited

Loading