feat: browser auth bridge impl#31
Conversation
| url: string, | ||
| redirectUri: string | ||
| ): Promise<BrowserSessionResult> { | ||
| console.log('[openAuthSession] called') |
There was a problem hiding this comment.
should the console.logs in this function be gated behind a debug flag or something?
There was a problem hiding this comment.
These are just for debugging/development. They will be removed before beta release
There was a problem hiding this comment.
(this is just a merge into the feature branch - to break up what otherwise would be a 300+ file PR 😅 )
| getDefault (): Promise<Credential | null>; | ||
| setDefault (cred: Credential | null): Promise<void>; | ||
| get tokenStorage (): TokenStorage; | ||
| tokenStorage: TokenStorage; |
There was a problem hiding this comment.
I see this is needed so RN can install its own storage at import time, but that be limited to a one time operation? this allows mutability even after initialization, right
There was a problem hiding this comment.
Correct. This is intentional. JavaScript doesn't have a compiler, therefore any changes to configurations and/or defaults will be performed at runtime
| console.log('here 3'); | ||
| console.log(err, (err as Error)?.stack); |
There was a problem hiding this comment.
FYI some additional debugging logs
| // Check if it's a cancellation or a real error | ||
| // ASWebAuthenticationSessionError code 1 = user cancelled | ||
| let nsError = error as NSError | ||
| if nsError.code == 1 { |
There was a problem hiding this comment.
You should probably compare to ASWebAuthenticationSessionErrorCodeCanceledLogin instead of the underlying enum value 1.
| // Set presentation context provider for iOS 13+ | ||
| if #available(iOS 13.0, *) { | ||
| authSession.presentationContextProvider = self | ||
| authSession.prefersEphemeralWebBrowserSession = false |
There was a problem hiding this comment.
We should provide a mechanism for a developer to indicate that they want ephemeral sessions.
| guard !completed else { return } | ||
| completed = true | ||
| reject("browser_session_error", "Failed to start authentication session", nil) | ||
| self.authSession = nil |
There was a problem hiding this comment.
You set authSession to nil here, but when the callback is invoked above (either through an error or success response) we don't set it to nil. Depending on how this bridge plugin's lifecycle is managed by ReactNative it may leak.
| // Keep a reference to prevent deallocation | ||
| self.authSession = authSession |
There was a problem hiding this comment.
We're assigning the new authSession, but aren't checking to see if there's already one presented. If this value is not nil, we should cancel the previous one.
| override init() { | ||
| super.init() | ||
| print("✅ TokenStorageBridge initialized!") | ||
| // Debug: Print all methods | ||
| let methodCount = UnsafeMutablePointer<UInt32>.allocate(capacity: 1) | ||
| let methods = class_copyMethodList(type(of: self), methodCount) | ||
| print("📋 TokenStorageBridge methods:") | ||
| for i in 0..<Int(methodCount.pointee) { | ||
| if let method = methods?[i] { | ||
| print(" - \(NSStringFromSelector(method_getName(method)))") | ||
| } | ||
| } | ||
| free(methods) | ||
| } |
There was a problem hiding this comment.
The indentation here is weird. Plus we probably don't want to include these debugging print statements long-term.
| name: "RNTokenStorageBridge", | ||
| targets: ["RNTokenStorageBridge"] |
There was a problem hiding this comment.
It looks like you only have a token storage bridge target, but aren't including the session bridge? Is this for a reason? Typically you'd probably just want to have a single library target for the overall collection of bridges
| name: "RNTokenStorageBridge", | ||
| path: "Sources/RNTokenStorageBridge", | ||
| exclude: ["TokenStorageBridge.swift", "TokenStorageBridge.m", "TokenStorageBridge.h"], | ||
| publicHeadersPath: "." |
There was a problem hiding this comment.
This format is strange. You shouldn't need to specify the path here since it'll assume the subdirectory of the same name as the target within the Sources directory. Also is there a reason why you're excluding all the source files?
| path: "Tests/RNTokenStorageBridgeTests", | ||
| exclude: ["TokenStorageBridgeTests.swift"] |
There was a problem hiding this comment.
You can probably delete these lines since it'll automatically pick up all test sources within the proper directory within Tests. Also, it looks like you're skipping the test.
| value: String, | ||
| accessibility: CFString | ||
| ) throws { | ||
| let data = value.data(using: .utf8)! |
There was a problem hiding this comment.
Force unwrapping this isn't a good idea; this SHOULD technically work, but if there's an edge-case or invalid data that is passed, this will crash the app. Instead we should ensure the value is correct, or throw an error
| let data = value.data(using: .utf8)! | |
| guard let data = value.data(using: .utf8) else { | |
| throw SomeError.blah | |
| } |
| // Loop to delete all items matching the query (defensive against duplicates) | ||
| while true { | ||
| let status = SecItemDelete(query as CFDictionary) | ||
| if status == errSecItemNotFound { | ||
| break | ||
| } else if status != errSecSuccess { | ||
| throw NSError(domain: NSOSStatusErrorDomain, code: Int(status)) | ||
| } | ||
| // If successful, loop again to ensure all items are deleted | ||
| } |
There was a problem hiding this comment.
We don't need to loop to perform a deletion, since SecItemDelete will delete all items matching the query, as long as kSecMatchLimit isn't specified.
| // SecItemDelete only deletes one item per call, so loop until empty | ||
| while true { | ||
| let status = SecItemDelete(query as CFDictionary) | ||
| if status == errSecItemNotFound { | ||
| // No more items to delete | ||
| break | ||
| } else if status != errSecSuccess { | ||
| throw NSError(domain: NSOSStatusErrorDomain, code: Int(status)) | ||
| } | ||
| // If successful, loop again to delete next item | ||
| } |
There was a problem hiding this comment.
This doesn't need to loop either, as anything that matches the query will be deleted
3 participants
No description provided.