Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: removed the possibility of concurrent webauth transactions to handle continuation misuse #848

Merged
merged 12 commits into from
May 8, 2024

Conversation

desusai7
Copy link
Contributor

@desusai7 desusai7 commented May 2, 2024

📋 Changes

  • Updated WebAuth to cancel any newly triggered WebAuth transactions when there exists an active WebAuth transaction already i.e ensuring that only single WebAuth transaction is active at any point of time.
  • The above change would fix the issues in some special cases of race condition where continuations are misused by resuming twice.
  • Updated unit tests for login & logout in WebAuthSpec to clear TransactionStore before each run to start clean and not get failed by the un-cleared transaction in memory by previous test runs.

📎 References

auth0/Auth0.swift#827

Checklist

  • All new/changed/fixed functionality is covered by tests (or N/A)
  • I have added documentation for all new/changed functionality (or N/A)

🎯 Testing

Tested by trying to start the below defined two flows at exact same time via both callbacks & async/await approach and found that the code is working as per expectations by cancelling the newly triggering web auth flow and ensuring the existing web auth flow executes completely.

  1. WebAuth flow with google-oauth2 connection using ASWebAuthenticationSession
  2. WebAuth flow with Universal Login using SafariViewController

@desusai7 desusai7 self-assigned this May 2, 2024
@desusai7 desusai7 linked an issue May 2, 2024 that may be closed by this pull request
6 tasks
@desusai7 desusai7 added the review:tiny Tiny review label May 8, 2024
@desusai7 desusai7 marked this pull request as ready for review May 8, 2024 09:42
@desusai7 desusai7 requested a review from a team as a code owner May 8, 2024 09:42
@@ -79,6 +80,7 @@ extension WebAuthError {
switch self.code {
case .noBundleIdentifier: return "Unable to retrieve the bundle identifier from Bundle.main.bundleIdentifier,"
+ " or it could not be used to build a valid URL."
case .transactionActiveAlready: return "Failed to start this transaction, as there is an active transaction at the moment"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
case .transactionActiveAlready: return "Failed to start this transaction, as there is an active transaction at the moment"
case .transactionActiveAlready: return "Failed to start this transaction, as there is an active transaction at the moment."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the text description for the WebAuthError .transactionActiveAlready as per description

@@ -6,6 +6,7 @@ public struct WebAuthError: Auth0Error {

enum Code: Equatable {
case noBundleIdentifier
case transactionActiveAlready
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are tests for WebAuthError, those should be updated as well: https://github.com/auth0/Auth0.swift/blob/master/Auth0Tests/WebAuthErrorSpec.swift

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing it out, included tests for it now.

@desusai7 desusai7 merged commit 1aa48da into master May 8, 2024
9 checks passed
@desusai7 desusai7 deleted the fix/concurrent_web_auth_transactions branch May 8, 2024 10:34
@desusai7 desusai7 mentioned this pull request May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review:tiny Tiny review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Auth0 crashes due to swift task continuation misuse
2 participants