Fix Google refresh-token checks to prevent event sync drop (#744)#912
Open
vaibhavarora14 wants to merge 6 commits intoleits:masterfrom
Open
Fix Google refresh-token checks to prevent event sync drop (#744)#912vaibhavarora14 wants to merge 6 commits intoleits:masterfrom
vaibhavarora14 wants to merge 6 commits intoleits:masterfrom
Conversation
Use OIDAuthState.refreshToken instead of lastTokenResponse.refreshToken so MeetingBar does not treat valid Google sessions as signed out after token refreshes. Made-with: Cursor
dosubot
Bot
added
the
size:XS
WalkthroughModified Google Calendar sign-in and sign-out authentication logic by introducing a helper method for skip-sign-in conditions, updating token revocation to use Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #912 +/- ##
==========================================
+ Coverage 26.30% 35.88% +9.57%
==========================================
Files 36 48 +12
Lines 5645 5181 -464
Branches 2210 1741 -469
==========================================
+ Hits 1485 1859 +374
+ Misses 4103 3266 -837
+ Partials 57 56 -1 ☔ View full report in Codecov by Sentry. |
Refactor GCEventStore refresh-token checks into testable helpers and add unit tests that verify AppAuth keeps refreshToken in OIDAuthState even when refresh responses omit refresh_token. Made-with: Cursor
dosubot
Bot
added
size:L
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Mark extracted token helper methods as nonisolated for test access and normalize AppAuth parameter dictionaries to Objective-C bridgeable types used by OID response initializers. Made-with: Cursor
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
MeetingBar/Core/EventStores/GCEventStore.swift (1)
223-233:⚠️ Potential issue | 🟠 Major
forcePromptcan be bypassed bysignIn’s early return
ensureSignedIn()computesforceConsenton Line 229, butsignIn(forcePrompt:)still returns early whenauthState?.isAuthorized == true(Line 97). That can skip the intended forced-consent flow for sessions missing a refresh token.Proposed fix
func signIn(forcePrompt: Bool = false) async throws { - // if already authorised, nothing to do - if authState?.isAuthorized == true { return } + // Skip only when session is fully reusable and consent is not being forced. + if !forcePrompt, Self.hasAuthorizedSession(authState) { return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MeetingBar/Core/EventStores/GCEventStore.swift` around lines 223 - 233, ensureSignedIn() computes forceConsent and calls signIn(forcePrompt:), but signIn(forcePrompt:) currently returns early when authState?.isAuthorized == true, allowing the forced-consent flow to be bypassed; update the logic so that signIn(forcePrompt:) honors the forcePrompt flag (or ensureSignedIn() bypasses the early-return) by treating forcePrompt=true as a reason to proceed even if authState?.isAuthorized is true—modify the early-return check in signIn(forcePrompt:) (and/or the hasAuthorizedSession short-circuit in ensureSignedIn()) to require both isAuthorized==true and forcePrompt==false before returning, so forced consent is not skipped for sessions missing a refresh token.
🧹 Nitpick comments (1)
MeetingBarTests/GCEventStoreTests.swift (1)
69-79: Remove redundantaccess_tokenwrite in test helperLine 71 initializes
access_tokenand Line 78 overwrites it immediately. Keeping a single assignment makes intent clearer.Proposed cleanup
private func tokenParameters(accessToken: String, refreshToken: String?) -> [String: NSObject & NSCopying] { var parameters: [String: NSObject & NSCopying] = [ - "access_token": "access-token-1" as NSString, + "access_token": accessToken as NSString, "token_type": "Bearer" as NSString, "expires_in": 3600 as NSNumber ] if let refreshToken { parameters["refresh_token"] = refreshToken as NSString } - parameters["access_token"] = accessToken as NSString return parameters }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MeetingBarTests/GCEventStoreTests.swift` around lines 69 - 79, In tokenParameters(accessToken:refreshToken:) remove the redundant initial "access_token" entry from the parameters dictionary (currently set at line creating parameters) and keep only the final assignment that sets "access_token" to the accessToken argument; ensure the function still adds "token_type" and "expires_in", conditionally sets "refresh_token" when refreshToken is non-nil, and returns the parameters as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@MeetingBar/Core/EventStores/GCEventStore.swift`:
- Around line 223-233: ensureSignedIn() computes forceConsent and calls
signIn(forcePrompt:), but signIn(forcePrompt:) currently returns early when
authState?.isAuthorized == true, allowing the forced-consent flow to be
bypassed; update the logic so that signIn(forcePrompt:) honors the forcePrompt
flag (or ensureSignedIn() bypasses the early-return) by treating
forcePrompt=true as a reason to proceed even if authState?.isAuthorized is
true—modify the early-return check in signIn(forcePrompt:) (and/or the
hasAuthorizedSession short-circuit in ensureSignedIn()) to require both
isAuthorized==true and forcePrompt==false before returning, so forced consent is
not skipped for sessions missing a refresh token.
---
Nitpick comments:
In `@MeetingBarTests/GCEventStoreTests.swift`:
- Around line 69-79: In tokenParameters(accessToken:refreshToken:) remove the
redundant initial "access_token" entry from the parameters dictionary (currently
set at line creating parameters) and keep only the final assignment that sets
"access_token" to the accessToken argument; ensure the function still adds
"token_type" and "expires_in", conditionally sets "refresh_token" when
refreshToken is non-nil, and returns the parameters as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 71e35a77-39d0-4c9c-8924-07fc10c33569
📒 Files selected for processing (2)
MeetingBar/Core/EventStores/GCEventStore.swiftMeetingBarTests/GCEventStoreTests.swift
Add debug-only GCEventStore test hooks and async tests that execute the updated ensureSignedIn/signOut token-check paths without network calls so patch coverage includes the regression fix lines. Made-with: Cursor
| } | ||
|
|
||
| #if DEBUG | ||
| func _test_getAuthState() -> OIDAuthState? { |
| authState | ||
| } | ||
|
|
||
| func _test_setAuthState(_ state: OIDAuthState?) { |
| userEmail = state?.userEmail | ||
| } | ||
|
|
||
| func _test_ensureSignedIn() async throws { |
Move GCEventStore coverage scenarios into an existing test file and reduce GCEventStore token logic changes to directly exercised lines so patch coverage reflects executed code paths. Made-with: Cursor
| try await ensureSignedIn() | ||
| } | ||
|
|
||
| func _test_setSignInTask(_ task: Task<Void, Error>?) { |
Author
|
Hi @leits — quick note on the remaining I investigated this in detail and it appears to be a Codecov aggregation mismatch, not a missing test execution case:
I also restructured tests specifically to avoid new-file patch accounting artifacts (moved GCEventStore coverage tests into an existing test file), but Given this mismatch, this looks like a Codecov-side patch status false negative on this PR context. If helpful, I can open a Codecov support ticket with the API payloads and check-run IDs. |
Prevent signIn from short-circuiting authorized sessions when forced consent is requested and add coverage for the skip-decision logic used by issue leits#744 flow. Made-with: Cursor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
MeetingBarTests/EventManagerTests.swift (1)
203-210: Good coverage of the key fix scenarios.The test correctly validates the three essential cases for the bug fix. Consider adding edge cases for completeness:
💡 Optional: Additional edge case coverage
let authorizedWithoutRefresh = makeAuthState(refreshToken: nil) XCTAssertFalse(GCEventStore.shouldSkipSignIn(forcePrompt: false, state: authorizedWithoutRefresh)) + + // nil state should never skip + XCTAssertFalse(GCEventStore.shouldSkipSignIn(forcePrompt: false, state: nil)) + XCTAssertFalse(GCEventStore.shouldSkipSignIn(forcePrompt: true, state: nil)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MeetingBarTests/EventManagerTests.swift` around lines 203 - 210, Add edge-case assertions to testSignInSkipLogicHonorsForcePrompt to increase coverage: call GCEventStore.shouldSkipSignIn with a nil state and verify expected behavior, and add a case where the state is unauthorized (e.g., makeAuthState with an explicit expired/invalid token if supported) to assert shouldSkipSignIn returns false regardless of forcePrompt; update references inside the test to use makeAuthState and GCEventStore.shouldSkipSignIn to construct these additional scenarios and their expected XCTAssertTrue/False outcomes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@MeetingBarTests/EventManagerTests.swift`:
- Around line 203-210: Add edge-case assertions to
testSignInSkipLogicHonorsForcePrompt to increase coverage: call
GCEventStore.shouldSkipSignIn with a nil state and verify expected behavior, and
add a case where the state is unauthorized (e.g., makeAuthState with an explicit
expired/invalid token if supported) to assert shouldSkipSignIn returns false
regardless of forcePrompt; update references inside the test to use
makeAuthState and GCEventStore.shouldSkipSignIn to construct these additional
scenarios and their expected XCTAssertTrue/False outcomes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cef16747-e45c-44ea-833a-40a4f1bdb6c6
📒 Files selected for processing (2)
MeetingBar/Core/EventStores/GCEventStore.swiftMeetingBarTests/EventManagerTests.swift
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
OIDAuthState.refreshTokeninstead oflastTokenResponse.refreshTokenrefresh_tokenin the latest token responseTest plan
xcodebuild -project \"MeetingBar.xcodeproj\" -scheme \"MeetingBar\" -destination \"platform=macOS\" CODE_SIGNING_ALLOWED=NO CODE_SIGNING_REQUIRED=NO buildCloses #744
Made with Cursor
Summary by CodeRabbit
Bug Fixes
Tests