fix(oauth): treat naive datetime values as UTC when converting to timestamps - #1970
Conversation
|
Thanks for the contribution! Before we can merge this, we need @kwy404 to sign the Salesforce Inc. Contributor License Agreement. |
AmyScript
left a comment
There was a problem hiding this comment.
Nice catch, normalizing to UTC with .replace(tzinfo=timezone.utc) is exactly right. A couple of small things:
On the test (test_internals.py)
One thing to flag: this case expects the same value as the existing tz-aware one, so if CI runs in UTC it'll pass even without the fix. Could we force a non-UTC timezone so it fails on main? Something like:
@mock.patch.dict(os.environ, {"TZ": "America/New_York"})
def test_naive_datetime_is_treated_as_utc(self):
time.tzset() # POSIX only
try:
assert _timestamp_to_type(datetime(2023, 11, 28, 22, 9, 7), int) == 1701209347
finally:
del os.environ["TZ"]
time.tzset()On the string branch (internals.py)
Quick question: does the isinstance(ts, str) branch just below have the same issue? If it parses to a naive datetime and calls .timestamp(), it'd hit the same local-time bug. Might be worth normalizing it the same way for consistency.
Nit: could be nice to mention this in the changelog, since it slightly changes behavior for anyone who was relying on naive = local time.
|
Thanks for the review!
|
|
This looks good, thank you! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1970 +/- ##
==========================================
- Coverage 84.28% 84.26% -0.03%
==========================================
Files 118 118
Lines 13677 13679 +2
==========================================
- Hits 11528 11526 -2
- Misses 2149 2153 +4 ☔ View full report in Codecov by Harness. |
|
|
||
| elif isinstance(ts, datetime): | ||
| if ts.tzinfo is None: | ||
| # naive datetime values (e.g., loaded from a database) are stored in UTC |
There was a problem hiding this comment.
The code is self explanatory 🙏 I don't think we need this comment
Once removed we should be good to merge 🚀
| # naive datetime values (e.g., loaded from a database) are stored in UTC |
Summary
SQL-based installation stores save expiration and install times as naive UTC datetimes, but
_timestamp_to_typeconverted naive datetimes withdatetime.timestamp(), which assumes local time. On servers not running in UTC this shiftedbot_token_expires_atanduser_token_expires_atby the local offset, so token rotation could skip an already expired token. Naive datetimes are now treated as UTC, matching how naive ISO strings are already handled.Fixes #1731
Testing
Added a naive datetime case to
test_timestamp_to_typeintests/slack_sdk/oauth/installation_store/test_internals.py; it fails before the change on a machine outside UTC and passes after. The oauth test suites (sync and async),ruff format --check,ruff checkandscripts/run_mypy.shwere run locally.Category
/docs(Documents)/tutorial(PythOnBoardingBot tutorial)tests/integration_tests(Automated tests for this library)Requirements
python3 -m venv .venv && source .venv/bin/activate && ./scripts/run_validation.shafter making the changes.