fix: Correcting logic for xblock_handler token expiration (#26224)

* fix: Correcting logic for xblock_handler token expiration

The previous math had a date math error which resulted in token expiring in 0-2 days
instead of 2-4 days

Co-authored-by: Feanil Patel <feanil@edx.org>
Co-authored-by: Tim McCormack <tmccormack@edx.org>

openedx/core/djangoapps/xblock/tests/test_utils.py

@@ -9,6 +9,8 @@ from freezegun import freeze_time
 
 from openedx.core.djangoapps.xblock.utils import (
     get_secure_token_for_xblock_handler,
+    validate_secure_token_for_xblock_handler,
+    _get_secure_token_for_xblock_handler,
     validate_secure_token_for_xblock_handler
 )
 
@@ -35,15 +37,49 @@ REFERENCE_PARAMS = {
         ({}, True),
         # Ensure token still valid in 1 hour
         ({"validation_time_delta_s": 3600}, True),
-        # Ensure token still valid in 1 day, this should be true but isn't due
-        # to the bug that will be fixed in ARCHBOM-1677
-        ({"validation_time_delta_s": 86400}, False),
+        # Ensure token still valid in 1 day
+        ({"validation_time_delta_s": 86400}, True),
         # Ensure token is invalid after 5 days
         ({"validation_time_delta_s": 86400 * 5}, False),
         # Ensure token is not valid 5 days before generation
         # In the case where the validating server is really skewed
         # from the generating server.
         ({"validation_time_delta_s": 86400 * -5}, False),
+        # Setting reference_time to 20 seconds after start of a 2 day time period(UTC)
+        # Demonstrating maximum possible validity period is just below 4 days
+        # This passes because validation time is just below the cutoff point
+        (
+            {"reference_time": datetime.datetime(2021, 1, 27, 0, 0, 20, tzinfo=datetime.timezone.utc),
+             "validation_time_delta_s": (86400 * 4) - 21
+             },
+            True,
+        ),
+        # Setting reference_time to 20 seconds after start of a 2 day time period(UTC)
+        # Demonstrating maximum possible validity period is just below 4 days
+        # This does not pass because validation time is just above the cutoff point
+        (
+            {"reference_time": datetime.datetime(2021, 1, 27, 0, 0, 20, tzinfo=datetime.timezone.utc),
+             "validation_time_delta_s": (86400 * 4) - 19
+             },
+            False,
+        ),
+        # Setting reference_time to 20 seconds before end of a 2 day time period(UTC)
+        # Demonstrating minimum possible validity period is just above 2 days
+        # This passes because validation time is just below the cutoff point
+        (
+            {"reference_time": datetime.datetime(2021, 1, 28, 23, 59, 40, tzinfo=datetime.timezone.utc),
+             "validation_time_delta_s": (86400 * 2) + 19
+             },
+            True,
+        ),
+        # Setting reference_time to 20 seconds before end of a 2 day time period(UTC)
+        # Demonstrating minimum possible validity period is just above 2 days
+        # This fails because validation time is just above the cutoff point
+        (
+            {"reference_time": datetime.datetime(2021, 1, 28, 23, 59, 40, tzinfo=datetime.timezone.utc),
+             "validation_time_delta_s": (86400 * 2) + 21
+             },
+            False),
         # Different user tries to use your token.
         ({"validation_user_id": 54321}, False),
         # Access a different block.
@@ -114,6 +150,18 @@ def test_secure_token(param_delta: dict, expected_validation: bool):
             assert (
                 validate_secure_token_for_xblock_handler(
                     params["validation_user_id"], params["validation_block_key_str"], token
-                )
-                == expected_validation
+                ) ==
+                expected_validation
             )
+
+
+def test_private_get_secure_token_for_xblock_handler():
+    """
+    Confirms function behaviour has not changed and is giving the same token for same inputs
+    If these tokens change, this will invalidate some or all of the tokens handed out to learners
+    """
+    with freeze_time(datetime.datetime(2021, 1, 27, 0, 0, 0, tzinfo=datetime.timezone.utc)):
+        token_now = _get_secure_token_for_xblock_handler("12345", "some_block_key", 0, "some_hashing_key")
+        token_previous = _get_secure_token_for_xblock_handler("12345", "some_block_key", -1, "some_hashing_key")
+        assert token_now == "de8399a51fef6aa7584a"
+        assert token_previous == "f3b5d92b8ca2934009e4"

openedx/core/djangoapps/xblock/utils.py

@@ -97,21 +97,10 @@ def _get_secure_token_for_xblock_handler(user_id, block_key_str, time_idx: int,
     Internal function to extract repeating hashing steps which we
     call multiple times with different time_idx and hashing key.
     """
-    # This was supposed to be an interval boundary number that matches
-    # the current 2 day chunk(midnight UTC of every other day).
-    # Instead it's a number that will always be consistent but
-    # otherwise does not have a semantic meaning.
-    # Right now the math is missing a multiplication by TOKEN_PERIOD after
-    # the math.floor which means the unit is days which is then added to
-    # 0 or -TOKEN_PERIOD seconds.
-    # Right now this is only valid for 0-2 days instead of the intended
-    # 2-4 days because of the units issue above. Correcting the math should
-    # be possible but we are not going to do it in the middle of the other
-    # change we're making. We've made https://openedx.atlassian.net/browse/ARCHBOM-1677
-    # to come back and deal with it.
     TOKEN_PERIOD = 24 * 60 * 60 * 2  # These URLs are valid for 2-4 days
+    # time_token is the number of time periods since unix epoch
     time_token = math.floor(time.time() / TOKEN_PERIOD)
-    time_token += TOKEN_PERIOD * time_idx
+    time_token += time_idx
     check_string = str(time_token) + ':' + str(user_id) + ':' + block_key_str
     secure_key = hmac.new(hashing_key.encode('utf-8'), check_string.encode('utf-8'), hashlib.sha256).hexdigest()
     return secure_key[:20]