Implement handling of new cookie received from SU platform#722
Implement handling of new cookie received from SU platform#722MattyTheHacker wants to merge 3 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request implements automatic handling of cookie updates from the SU platform. The MSL platform returns updated authentication cookies in HTTP responses that expire approximately every 15 minutes. The PR refactors cookie management to detect and store these updated cookies, and consolidates duplicate HTTP session handling code.
Changes:
- Introduced
fetch_url_content_with_sessionfunction to handle HTTP requests with automatic cookie update detection and storage - Refactored
fetch_community_group_members_listto use the new shared session handling function - Removed duplicate HTTP session code from
CheckSUPlatformAuthorisationBaseCogclass
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| utils/msl/memberships.py | Added fetch_url_content_with_session function to detect and update cookies from server responses; refactored fetch_community_group_members_list to use new function |
| utils/msl/init.py | Exported new fetch_url_content_with_session function |
| cogs/check_su_platform_authorisation.py | Removed duplicate HTTP session handling code; replaced with calls to shared fetch_url_content_with_session function |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| BASE_SU_PLATFORM_WEB_COOKIES: "Final[Mapping[str, str]]" = { | ||
| ".AspNet.SharedCookie": settings["SU_PLATFORM_ACCESS_COOKIE"], | ||
| BASE_SU_PLATFORM_WEB_COOKIES: "Final[MutableMapping[str, str]]" = { |
There was a problem hiding this comment.
The type annotation Final[MutableMapping[str, str]] is contradictory. Final indicates the variable binding cannot be reassigned, but MutableMapping indicates the contents are mutable. While Python will allow this at runtime, it violates the semantic intent of Final and may confuse type checkers. Consider either:
- Using
MutableMapping[str, str]withoutFinal(recommended, since the dictionary contents are mutated) - Using a different pattern that doesn't mix immutability guarantees with mutable collections
| BASE_SU_PLATFORM_WEB_COOKIES: "Final[MutableMapping[str, str]]" = { | |
| BASE_SU_PLATFORM_WEB_COOKIES: "MutableMapping[str, str]" = { |
There was a problem hiding this comment.
I neither know nor care if this matters, the code runs fine - Matt up to you if you want this fixed
There was a problem hiding this comment.
I think the correct annotation here is Mapping[str, str]. Then when you need to update the cookie, assign a new dict to BASE_SU_PLATFORM_WEB_COOKIES rather than just updating the key.
|
|
||
| BASE_SU_PLATFORM_WEB_COOKIES: "Final[Mapping[str, str]]" = { | ||
| ".AspNet.SharedCookie": settings["SU_PLATFORM_ACCESS_COOKIE"], | ||
| BASE_SU_PLATFORM_WEB_COOKIES: "Final[MutableMapping[str, str]]" = { |
There was a problem hiding this comment.
I think the correct annotation here is Mapping[str, str]. Then when you need to update the cookie, assign a new dict to BASE_SU_PLATFORM_WEB_COOKIES rather than just updating the key.
|
|
||
| logger: "Final[Logger]" = logging.getLogger("TeX-Bot") | ||
|
|
||
| SU_PLATFORM_ACCESS_COOKIE: "Final[str]" = settings["SU_PLATFORM_ACCESS_COOKIE"] |
There was a problem hiding this comment.
settings["SU_PLATFORM_ACCESS_COOKIE"] can be used directly, rather than assigning to a variable first.
| http_session.get(url=url, ssl=GLOBAL_SSL_CONTEXT) as http_response, | ||
| ): | ||
| response_html: str = await http_response.text() | ||
| returned_asp_cookie: Morsel | None = http_response.cookies.get(".AspNet.SharedCookie") # type: ignore[type-arg] |
There was a problem hiding this comment.
What is the need to use # type: ignore[type-arg] here?
| response_html: str = await http_response.text() | ||
| returned_asp_cookie: Morsel | None = http_response.cookies.get(".AspNet.SharedCookie") # type: ignore[type-arg] | ||
| if ( | ||
| returned_asp_cookie |
There was a problem hiding this comment.
| returned_asp_cookie | |
| returned_asp_cookie is not None |
| and returned_asp_cookie.value | ||
| != BASE_SU_PLATFORM_WEB_COOKIES[".AspNet.SharedCookie"] |
There was a problem hiding this comment.
| and returned_asp_cookie.value | |
| != BASE_SU_PLATFORM_WEB_COOKIES[".AspNet.SharedCookie"] | |
| and ( | |
| returned_asp_cookie.value | |
| != BASE_SU_PLATFORM_WEB_COOKIES[".AspNet.SharedCookie"] | |
| ) |
| != BASE_SU_PLATFORM_WEB_COOKIES[".AspNet.SharedCookie"] | ||
| ): | ||
| logger.info("SU platform access cookie was updated by the server; updating local.") | ||
| BASE_SU_PLATFORM_WEB_COOKIES[".AspNet.SharedCookie"] = returned_asp_cookie.value |
There was a problem hiding this comment.
| BASE_SU_PLATFORM_WEB_COOKIES[".AspNet.SharedCookie"] = returned_asp_cookie.value | |
| BASE_SU_PLATFORM_WEB_COOKIES = {".AspNet.SharedCookie": returned_asp_cookie.value} |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Note this will need the cookie checking task (or some other query) to run more often than the cookie expires. For MSL this appears to be about 15 minutes