fix(auth): add checks against random logouts #4099
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4099 +/- ##
==========================================
- Coverage 66.84% 66.66% -0.18%
==========================================
Files 1129 1129
Lines 42540 42543 +3
==========================================
- Hits 28435 28363 -72
- Misses 14105 14180 +75
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
...Auth/Sources/AWSCognitoAuthPlugin/Actions/FetchAuthorizationSession/InformSessionError.swift
Show resolved
Hide resolved
...gins/Auth/Sources/AWSCognitoAuthPlugin/CredentialStorage/AWSCognitoAuthCredentialStore.swift
Outdated
Show resolved
Hide resolved
...uthPluginUnitTests/TaskTests/ClientBehaviorTests/AuthenticationProviderDeleteUserTests.swift
Show resolved
Hide resolved
| // During iOS prewarming or before first device unlock, keychain operations will fail | ||
| // because protected data is not available. Check proactively before any keychain/UserDefaults access. | ||
| #if canImport(UIKit) && !os(watchOS) | ||
| guard UIApplication.shared.isProtectedDataAvailable else { |
There was a problem hiding this comment.
@thisisabhash @harsh62 without this check will the keychain clear if the app is prewarmed?
There was a problem hiding this comment.
Yes. it would..
The reason we reverted that change because Xcode complained for accessing UIApplication outside of MainActor context.
We would have to rethink how to reintroduce that check in a safe way.
There was a problem hiding this comment.
So does this PR fix any of the keychain clearing logic? I think that might be causing the random logouts
There was a problem hiding this comment.
It doesn't fix the keychain clearing logic, but the PR introduces a very subtle fix of incorrectly signally sessionExpired that could be inadvertently resulting app developers logging out their users incorrectly.
Meanwhile, we are reconsidering the keychain clearing logic internally and will make changes accordingly.
There was a problem hiding this comment.
Ok so with this PR the keychain shouldn't be cleared if the app is prewarming and the user is logged in?
When was sessionExpired being incorrectly signaled?
There was a problem hiding this comment.
Ok so with this PR the keychain shouldn't be cleared if the app is prewarming and the user is logged in?
It still can, the fix was not applied.
When was sessionExpired being incorrectly signaled?
If an expired idToken was passed to Identity Pools during fetching an authenticated identity id and temporary credentials.
There was a problem hiding this comment.
Thanks for the info!
Do you have an ETA on when the prewarming keychain clearing logic will be fixed? This is effecting a lot of our users unfortunately.
There was a problem hiding this comment.
I aiming to have something ready to be released next week.
3 participants
This pull request introduces several improvements and fixes to the authentication logic in the AWS Cognito Auth Plugin. The most significant changes include making token expiry checks more robust, ensuring keychain operations are only attempted when protected data is available, and simplifying error handling for session errors. Additionally, test mocks are enhanced to better simulate authentication flows.
Issue #
Description
General Checklist
Given When Theninline code documentation and are named accordinglytestThing_condition_expectation()By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.