-
Notifications
You must be signed in to change notification settings - Fork 254
Github issue: #890 Conforming Credential Manager to Sendable #1033
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds Sendable conformance to CredentialsManager and related types to enable Swift 6 compatibility, allowing these types to be safely used within actors and across concurrency boundaries without compiler errors.
- Added
Sendableconformance to key public types includingCredentialsManager,Authentication,CredentialsStorage,Telemetry,BiometricPolicy, andBioAuthentication - Used
@unchecked SendableforBiometricSessionwith proper thread-safe lock-based synchronization - Applied
@preconcurrencyimports for external frameworks that don't yet conform toSendable(JWTDecode, LocalAuthentication)
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Auth0/Telemetry.swift | Added Sendable conformance to the Telemetry struct |
| Auth0/Requestable.swift | Removed unused Combine import |
| Auth0/IDTokenValidator.swift | Added @preconcurrency import for JWTDecode to suppress Sendable warnings |
| Auth0/CredentialsStorage.swift | Added Sendable conformance to the CredentialsStorage protocol |
| Auth0/CredentialsManager.swift | Added Sendable conformance to CredentialsManager and marked BiometricSession as @unchecked Sendable with thread-safe lock usage |
| Auth0/BiometricPolicy.swift | Added Sendable conformance to the BiometricPolicy enum |
| Auth0/BioAuthentication.swift | Added Sendable conformance and @preconcurrency import for LocalAuthentication |
| Auth0/Authentication.swift | Added Sendable conformance to the Authentication protocol |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This comment has been minimized.
This comment has been minimized.
e354332 to
8d9ffa2
Compare
sanchitmehta94
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
App/ViewController.swift
Outdated
| import Auth0 | ||
|
|
||
| // MARK: - Swift 6 Sendability Test: CredentialsManager in Actor | ||
| actor AuthService { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for testing the changes please run the App Target should compile successfully.
I am thinking to add a follow up PR to create a separate target for Swift6Testing
Auth0.xcodeproj/project.pbxproj
Outdated
| PRODUCT_NAME = "$(TARGET_NAME)"; | ||
| PROVISIONING_PROFILE_SPECIFIER = ""; | ||
| SWIFT_VERSION = 5.0; | ||
| SWIFT_VERSION = 6.0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
App Target is moved to use Swift version: 6.0
App/ViewController.swift
Outdated
|
|
||
| func fetchCredentials() async throws -> Credentials { | ||
| // This method can be called across concurrency contexts eg. Actor | ||
| return try await credentialsManager.credentials(withScope: "openid profile email", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#890 (comment)
that is the ask from customer to be able toc all this API within actor when code being compiled with Swift 6.
4a27e2c to
1ae38a2
Compare
| public struct CredentialsManager: Sendable { | ||
|
|
||
| // storage is inherently sendable as it uses Keychain under the hood and is stateless | ||
| private let sendableStorage: SendableBox<CredentialsStorage> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i have used SendableBox instead of conforming to Sendable protocol to keep it backward compatible and let the change go in minor release which is fine as it is sendable by design
|
Claude finished @pmathew92's task —— View job Overall Assessment❌ This PR has critical issues that must be addressed before merging. While the goal of adding Sendable conformance for Swift 6 support is sound, the implementation has several bugs and unsafe patterns that will prevent compilation or cause thread-safety issues. Critical Issues
Additional Concerns
Security & PerformanceNo significant security or performance concerns beyond the thread-safety issues noted above. The lock-based synchronization for |
pmathew92
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
12c0878
7552b9b to
12c0878
Compare
App/ViewController.swift
Outdated
| import Auth0 | ||
|
|
||
| // MARK: - Swift 6 Sendability Test: CredentialsManager in Actor | ||
| actor AuthService { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ideally this code should not impact test cases as they run on test targets rather than App target but somehow this was impacting them would raise a separate PR we do have plan to ass swift 6 test target
📋 Changes
SDK-7259
Swift 6 support · Issue #890 · auth0/Auth0.swift
As per this github issue we need to conform Credential Manager conform to Sendable. so that that can be called from clients within actors when using Swift 6 without any errors.
Swift 6 support · Issue #890 · auth0/Auth0.swift
Even the original ask that came on community page was to conform Credential Manager to Sendable
CredentialsManager.credentials and sendability
The goal of this PR is to ensure that all public interfaces exposed to clients conform to Sendable, so they remain usable both in legacy callback workflows and in modern Swift concurrency contexts.
This also addresses a real issue reported by customers: using CredentialManager inside an actor with the Swift 6 compiler produced errors because CredentialManager did not conform to Sendable. This PR resolves that problem by making the necessary types explicitly Sendable while maintaining full backward compatibility with the current SDK architecture.
Follow up PR: As our customers are trying to access Credential Managers within actors . we would imitate the same behavior in the sample app.
We haven’t migrated the SDK to strict Swift concurrency (e.g., actors, async/await-based APIs). Doing so would be a major undertaking and a breaking change that affects the entire codebase, and we also need to remain backward-compatible with existing callback-based APIs.
📎 References
🎯 Testing
For testing as Auth0.swift is compiled with Swift 5 it would not throw errors for testing we should look if there are are no warning realted to sendability in Credential Manager file.