From f25b395eca7b063419dfc70913a176958c56bfaa Mon Sep 17 00:00:00 2001 From: Tim McCormack Date: Tue, 31 Aug 2021 16:50:54 +0000 Subject: [PATCH] feat!: Rename CookieNameChange middleware settings (#28584) CookieNameChange allowed the use of the expand-contract pattern for cookies, but the suggested procedure assumed an instantaneous change on the server side. Without that, there would be a brief window of time where servers that had received the newer config would be writing cookies that the servers with old config would not be able to understand. However, the mechanism can be made seamless by using it *twice* in succession, with the first usage in a "no-op" configuration. This allows all the servers to become aware of the new name without using it. The second change flips the configuration and changes the official name of the cookie, and during that window both sets of servers are able to understand both sets of names, even though they're sending a mix of names. This can then be followed by the usual cleanup. --- .../djangoapps/cookie_metadata/middleware.py | 69 ++++++++++++++----- .../tests/test_cookie_name_change.py | 30 ++++---- 2 files changed, 65 insertions(+), 34 deletions(-) diff --git a/openedx/core/djangoapps/cookie_metadata/middleware.py b/openedx/core/djangoapps/cookie_metadata/middleware.py index ecd276d96d..587f7c5455 100644 --- a/openedx/core/djangoapps/cookie_metadata/middleware.py +++ b/openedx/core/djangoapps/cookie_metadata/middleware.py @@ -13,18 +13,49 @@ class CookieNameChange: Changes the names of a cookie in request.COOKIES For this middleware to run: - - set COOKIE_NAME_CHANGE_ACTIVATE_EXPAND_PHASE = True + - set COOKIE_NAME_CHANGE_ACTIVATE = True - COOKIE_NAME_CHANGE_EXPAND_INFO is a dict and has following info: - "old_name": Previous name of cookie - "new_name": New name of cookie + - "current": Cookie name that will be used by relying code + - "alternate": Other cookie name, to be renamed to current name if present - Actions taken by middleware: - - will delete any cookie with "old_name" - - if create a new cookie with "new_name" and set its value to value of "old_name" cookie - + it will not modify cookie with "new_name" if it already exists in request.COOKIES + Actions taken by middleware, during request phase: + - Delete alternate-name cookie from request.COOKIES + - Preserve any cookie with current name, or create one with value of + cookie with alternate name (if alt cookie was present) + + To perform a seamless name change for a cookie, follow this + expand-contract procedure: + + 1. Baseline configuration:: + + SOME_COOKIE_NAME: old + + 2. Enable servers to understand both names by renaming the *new* name + to the *old* (current) name, which should have no immediate effect:: + + COOKIE_NAME_CHANGE_ACTIVATE: True + COOKIE_NAME_CHANGE_EXPAND_INFO: + current: old + alternate: new + SOME_COOKIE_NAME: old + + 3. Change the current name, both in the main cookie setting and in the + name-change dictionary, now that all servers are capable of reading + either name:: + + COOKIE_NAME_CHANGE_ACTIVATE: True + COOKIE_NAME_CHANGE_EXPAND_INFO: + current: new + alternate: old + SOME_COOKIE_NAME: new + + 4. After some time period to allow old cookies to age out, remove the + transition settings:: + + SOME_COOKIE_NAME: new """ - # .. toggle_name: COOKIE_NAME_CHANGE_ACTIVATE_EXPAND_PHASE + # .. toggle_name: COOKIE_NAME_CHANGE_ACTIVATE # .. toggle_implementation: DjangoSetting # .. toggle_default: False # .. toggle_description: Used to enable CookieNameChange middleware which changes a cookie name in request.COOKIES @@ -33,26 +64,26 @@ class CookieNameChange: # .. toggle_creation_date: 2021-08-04 # .. toggle_target_removal_date: 2021-10-01 # .. toggle_tickets: https://openedx.atlassian.net/browse/ARCHBOM-1872 - if getattr(settings, "COOKIE_NAME_CHANGE_ACTIVATE_EXPAND_PHASE", False): - old_cookie_in_request = False + if getattr(settings, "COOKIE_NAME_CHANGE_ACTIVATE", False): + alt_cookie_in_request = False expand_settings = getattr(settings, "COOKIE_NAME_CHANGE_EXPAND_INFO", None) if ( expand_settings is not None and isinstance(expand_settings, dict) - and "new_name" in expand_settings - and "old_name" in expand_settings + and "current" in expand_settings + and "alternate" in expand_settings ): - if expand_settings["old_name"] in request.COOKIES: - old_cookie_in_request = True - old_cookie_value = request.COOKIES[expand_settings["old_name"]] - del request.COOKIES[expand_settings["old_name"]] + if expand_settings["alternate"] in request.COOKIES: + alt_cookie_in_request = True + alt_cookie_value = request.COOKIES[expand_settings["alternate"]] + del request.COOKIES[expand_settings["alternate"]] if ( - expand_settings["new_name"] not in request.COOKIES - and old_cookie_in_request + expand_settings["current"] not in request.COOKIES + and alt_cookie_in_request ): - request.COOKIES[expand_settings["new_name"]] = old_cookie_value + request.COOKIES[expand_settings["current"]] = alt_cookie_value response = self.get_response(request) return response diff --git a/openedx/core/djangoapps/cookie_metadata/tests/test_cookie_name_change.py b/openedx/core/djangoapps/cookie_metadata/tests/test_cookie_name_change.py index 3a4d1ee844..be877a8823 100644 --- a/openedx/core/djangoapps/cookie_metadata/tests/test_cookie_name_change.py +++ b/openedx/core/djangoapps/cookie_metadata/tests/test_cookie_name_change.py @@ -32,8 +32,8 @@ class TestCookieNameChange(TestCase): } self.expand_settings = { - "old_name": self.old_key, - "new_name": "b", + "alternate": self.old_key, + "current": "b", } def test_cookie_swap(self): @@ -44,26 +44,26 @@ class TestCookieNameChange(TestCase): self.mock_request.COOKIES = self.old_dict.copy() with self.settings( - COOKIE_NAME_CHANGE_ACTIVATE_EXPAND_PHASE=True + COOKIE_NAME_CHANGE_ACTIVATE=True ), self.settings(COOKIE_NAME_CHANGE_EXPAND_INFO=self.expand_settings): self.cookie_name_change_middleware(self.mock_request) - assert self.expand_settings["old_name"] not in self.mock_request.COOKIES.keys() - assert self.expand_settings["new_name"] in self.mock_request.COOKIES.keys() - assert self.mock_request.COOKIES[self.expand_settings["new_name"]] == self.old_value + assert self.expand_settings["alternate"] not in self.mock_request.COOKIES.keys() + assert self.expand_settings["current"] in self.mock_request.COOKIES.keys() + assert self.mock_request.COOKIES[self.expand_settings["current"]] == self.old_value test_dict = self.extra_cookies.copy() - test_dict[self.expand_settings['new_name']] = self.old_value + test_dict[self.expand_settings['current']] = self.old_value assert self.mock_request.COOKIES == test_dict # make sure response function is called once self.mock_response.assert_called_once() def test_cookie_no_swap(self): - """Make sure self.cookie_name_change_middleware does not change cookie if new_name cookie is already present""" + """Make sure self.cookie_name_change_middleware does not change cookie if current cookie is already present""" new_value = "." * 13 no_change_cookies = { - self.expand_settings['new_name']: new_value, + self.expand_settings['current']: new_value, "_c_": "." * 13, "a.b": "." * 10, } @@ -73,13 +73,13 @@ class TestCookieNameChange(TestCase): self.mock_request.COOKIES = self.old_dict.copy() with self.settings( - COOKIE_NAME_CHANGE_ACTIVATE_EXPAND_PHASE=True + COOKIE_NAME_CHANGE_ACTIVATE=True ), self.settings(COOKIE_NAME_CHANGE_EXPAND_INFO=self.expand_settings): self.cookie_name_change_middleware(self.mock_request) - assert self.expand_settings["old_name"] not in self.mock_request.COOKIES.keys() - assert self.expand_settings["new_name"] in self.mock_request.COOKIES.keys() - assert self.mock_request.COOKIES[self.expand_settings["new_name"]] == new_value + assert self.expand_settings["alternate"] not in self.mock_request.COOKIES.keys() + assert self.expand_settings["current"] in self.mock_request.COOKIES.keys() + assert self.mock_request.COOKIES[self.expand_settings["current"]] == new_value assert self.mock_request.COOKIES == no_change_cookies # make sure response function is called once @@ -90,7 +90,7 @@ class TestCookieNameChange(TestCase): new_value = "." * 13 no_change_cookies = { - self.expand_settings['new_name']: new_value, + self.expand_settings['current']: new_value, "_c_": "." * 13, "a.b": "." * 10, } @@ -99,7 +99,7 @@ class TestCookieNameChange(TestCase): self.mock_request.COOKIES = self.old_dict.copy() with self.settings( - COOKIE_NAME_CHANGE_ACTIVATE_EXPAND_PHASE=False + COOKIE_NAME_CHANGE_ACTIVATE=False ), self.settings(COOKIE_NAME_CHANGE_EXPAND_INFO=self.expand_settings): self.cookie_name_change_middleware(self.mock_request)