[ECO-5655] Fix server time offset check

[maratal]

Closes #2148 and #2147

Summary by CodeRabbit

• Bug Fixes

  • Simplified server time offset handling in authentication, eliminating an extra server-time fetch and changing default query behavior.

• Tests

  • Updated tests to parameterize and broaden coverage of time-offset scenarios, including out-of-range offsets and authorization path variations.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Removed legacy server-time helpers and associated gating logic from authentication; token creation now uses a single time-offset check. Tests were refactored to a parameterized helper and two wrapper cases; a test exercising queryTime behavior was removed.

Changes

Cohort / File(s)	Summary
Time offset handling simplification Source/ARTAuth.m	Deleted -fetchServerTimeWithCompletion: and -hasTimeOffsetWithValue. Token request creation now checks hasTimeOffset only; removed queryTime reset in storeOptions.
Test refactor & coverage Test/AblyTests/Tests/AuthTests.swift	Reworked test into a parameterized helper and two wrapper tests (offsets 120 and -119). Removed the test that exercised queryTime == true. Updated second authorize call to pass authOptions.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐇 I hopped through clocks and leapt the gate,
Removed the maze that muddled state.
One simple check now keeps time true,
Whether ahead or behind—hooray, woohoo! ⏱️🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'ECO-5655 Fix server time offset check' clearly and specifically summarizes the main change: fixing a bug in server time offset handling logic.
Linked Issues check	✅ Passed	The PR changes directly address all three coding objectives from linked issues #2148 and ECO-5655: removing hasTimeOffsetWithValue method to fix offset recognition, simplifying time-offset handling logic, removing queryTime gating, and introducing parameterized tests for both positive and negative offsets.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to the linked issues: ARTAuth.m modifications address the offset logic bug, and AuthTests.swift changes implement test coverage for both offset scenarios as required.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/2148-server-time-offset

---

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between eafd464 and 1d6bee0.

📒 Files selected for processing (1)

• Test/AblyTests/Tests/AuthTests.swift

🚧 Files skipped from review as they are similar to previous changes (1)

• Test/AblyTests/Tests/AuthTests.swift

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)

• GitHub Check: check (iOS, test_iOS18_4)
• GitHub Check: check (tvOS, test_tvOS18_4)
• GitHub Check: check (macOS, test_macOS)
• GitHub Check: build
• GitHub Check: check
• GitHub Check: check

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

Source/ARTAuth.m (1)

703-707: The simplified condition bypasses the queryTime option, breaking test__119's expectations.

The change from if ([self hasTimeOffsetWithValue] && !replacedOptions.queryTime) to if ([self hasTimeOffset]) removes the gating that previously allowed explicit queryTime=true to force a server query even with a cached offset.

Test test__119__authorize__server_time_offset__should_request_server_time_when_queryTime_is_true_even_if_the_time_offset_is_assigned sets a fake offset, then calls authorize with queryTime=true, expecting the server to be queried and the offset to change. With the current code, the cached offset will be used regardless, causing the test to fail (expecting serverTimeRequestCount == 1 but getting 0).

This is a behavior regression introduced when hasTimeOffsetWithValue was removed.

🧹 Nitpick comments (1)

Test/AblyTests/Tests/AuthTests.swift (1)

3503-3505: Consider clarifying the server-side limitation comment.

The comment "looks like 120 is out of range in server side comparison" could be more specific about what server-side validation or comparison is failing with -120. This would help future maintainers understand whether this is a temporary workaround or an expected constraint.

💡 Suggested comment improvement

-        try _test__115__authorize__server_time_offset__should_obtain_server_time_once_and_persist_the_offset_from_the_local_clock(-119) // looks like 120 is out of range in server side comparison
+        try _test__115__authorize__server_time_offset__should_obtain_server_time_once_and_persist_the_offset_from_the_local_clock(-119) // Using -119 instead of -120 to avoid server-side time offset validation range limit

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 211c51d and eafd464.

📒 Files selected for processing (2)

• Source/ARTAuth.m
• Test/AblyTests/Tests/AuthTests.swift

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-12-10T14:14:47.456Z

Learnt from: lawrence-forooghian
Repo: ably/ably-cocoa PR: 2162
File: Source/ARTJsonLikeEncoder.m:522-525
Timestamp: 2025-12-10T14:14:47.456Z
Learning: In Source/ARTJsonLikeEncoder.m, ensure that artMessage.actionIsInternallySet is only set to YES when the SDK explicitly sets the action field (e.g., in ARTMessage initWithName:data:). Messages decoded from the wire must not have actionIsInternallySet set, even if they contain an action field, because the flag should distinguish SDK-internal actions from wire-provided actions. Apply this behavior broadly to Objective-C files in the Source directory that involve message encoding/decoding, and add tests to verify that decoding does not flip the flag.

Applied to files:

• Source/ARTAuth.m

🧬 Code graph analysis (1)

Test/AblyTests/Tests/AuthTests.swift (2)

Test/AblyTests/Test Utilities/TestUtilities.swift (1)

• commonAppSetup (107-142)

Test/AblyTests/Tests/MessageUpdatesDeletesTests.swift (1)

• rest (41-49)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)

• GitHub Check: check (tvOS, test_tvOS18_4)
• GitHub Check: check (iOS, test_iOS18_4)
• GitHub Check: check (macOS, test_macOS)
• GitHub Check: check
• GitHub Check: build
• GitHub Check: check

🔇 Additional comments (1)

Test/AblyTests/Tests/AuthTests.swift (1)

3442-3497: Good parameterization for server time offset testing.

The refactoring to accept an offset parameter enables testing both positive (server ahead) and negative (server behind) time offset scenarios with the same test logic. This improves test coverage and maintainability.

The change at line 3482 to pass authOptions instead of nil ensures queryTime is available for the second authorize() call, which fixes the issue where options were being cleared between calls.