Add token refresh coordinator#4087
Conversation
Generated by 🚫 Danger |
Clang Static Analysis Issues
Generated by 🚫 Danger |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #4087 +/- ##
==========================================
- Coverage 70.73% 68.46% -2.28%
==========================================
Files 246 247 +1
Lines 21541 21611 +70
==========================================
- Hits 15237 14795 -442
- Misses 6304 6816 +512
🚀 New features to boost your workflow:
|
|
||||||||||||||||||
|
||||||||||||||||
| * the callbacks are appended to the waiting list and no new network request is made. | ||
| * When the single in-flight refresh completes, all registered callbacks receive the same result. | ||
| * | ||
| * Completion and error callbacks are dispatched on the main queue. |
There was a problem hiding this comment.
Thread contract mismatch: This says callbacks are dispatched on the main queue, but completeRefreshForKey:credentials:error: calls them directly on the serial queue — no main-queue hop. The old SFOAuthSessionRefresher.completeWithSuccess wrapped callbacks in dispatch_async(dispatch_get_main_queue(), ...), which this PR removes.
Callers updating UI or expecting main-thread delivery will now run on a background thread. Either dispatch in completeRefreshForKey:, or drop this claim from the header and audit call sites.
| }); | ||
| } | ||
| }]; | ||
| if (self.refreshCycleActive) { |
There was a problem hiding this comment.
refreshCycleActive is not reset by cleanup, creating a timing gap. If cleanup runs while a refresh is in-flight, refreshCycleActive stays YES. Any new request enqueued after cleanup but before the coordinator callback resets the flag will hit this guard and silently skip triggering a refresh.
testNewRefreshCyclePossibleAfterCleanup avoids this by manually setting refreshCycleActive = NO rather than going through the real timing path. Consider resetting the flag inside cleanup after clearing activeRequests.
| } | ||
| }); | ||
| if (completionBlock) { | ||
| completionBlock(endpointResponse); |
There was a problem hiding this comment.
Main-queue removal affects all callers of SFSDKOAuth2, not just the coordinator path. The dispatch_async(dispatch_get_main_queue(), ...) wrapper was removed from error-path completions in both accessTokenForApprovalCode: and accessTokenForRefresh:. Any direct caller of these methods that touches UI in the completion block will now run on a background URL session thread.
| XCTAssertNotEqual(account.credentials.accessToken, originalAccessToken, | ||
| "Access token should be rotated after refresh") | ||
| XCTAssertNotNil(account.credentials.refreshToken) | ||
| print("original refresh token: \(originalRefreshToken)") |
There was a problem hiding this comment.
Debug print() statements — looks unintentional.
| let notificationCount = Mutex<Int>(0) | ||
|
|
||
| let observer = NotificationCenter.default.addObserver( | ||
| forName: .init(rawValue: "SFNotificationOAuthUserDidRefreshToken"), |
There was a problem hiding this comment.
Fragile raw notification name string. Observing "SFNotificationOAuthUserDidRefreshToken" instead of kSFNotificationUserDidRefreshToken means if the constant value ever changes this test silently vacuously passes. Use the typed constant.
|
|
||
| #pragma mark - Integration Test (Mock at authClient level) | ||
|
|
||
| - (void)testIntegrationSingleNetworkCallForConcurrentRefreshes { |
There was a problem hiding this comment.
Integration test assertion may be vacuously true. SFOAuthSessionRefresher constructs its own SFOAuthCoordinator internally and calls accessTokenForRefresh: directly — it does not go through SFUserAccountManager.authClient(). With realCoordinator.refresherFactory = nil (real refresher path), mockClient.accessTokenForRefreshCallCount likely stays 0 regardless of how many network calls go out, making the key assertion (== 1) always pass.
Suggest adding XCTAssertGreaterThan(mockClient.accessTokenForRefreshCallCount, 0, @"mock was never called") before the coalescing assertion to confirm the mock is actually hit.
| } | ||
| SFOAuthInfo *authInfo = [[SFOAuthInfo alloc] initWithAuthType:SFOAuthTypeRefresh]; | ||
| [userInfo setValue:authInfo forKey:kSFNotificationUserInfoAuthTypeKey]; | ||
| [[NSNotificationCenter defaultCenter] postNotificationName:kSFNotificationUserDidRefreshToken |
There was a problem hiding this comment.
Notification sender has changed. The old refreshCredentials: in SFUserAccountManager posted kSFNotificationUserDidRefreshToken with object:strongSelf (the account manager). Now this posts with object:self (the refresher). Any observer filtering on notification.object — e.g. addObserver:selector:name:object:accountManager — will silently stop receiving notifications.
2 participants
SFRestAPI,SFIdentityCoordinatorandSFUserAccountManagerSFUserAccountManagerSFOAuthSessionRefresherso that it will be internal only in the future