feat: generic oauth plugin #13
Conversation
WalkthroughAdds a Generic OAuth plugin and client APIs, new DTOs and network routes for OAuth2 flows, exposes OAuthHandler methods, registers a new package library target, and provides a SwiftUI example view wired into the app routing to demonstrate OAuth2 sign-in and logout. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GenericOAuthView
participant BetterAuthClient
participant GenericOAuthPlugin
participant OAuthHandler
participant Server
participant ASWebAuth as ASWebAuthenticationSession
User->>GenericOAuthView: enters providerId & taps Sign In
GenericOAuthView->>BetterAuthClient: signIn.genericOAuth.oauth2(request)
BetterAuthClient->>GenericOAuthPlugin: perform sign-in request
GenericOAuthPlugin->>Server: POST /sign-in/oauth2 (providerId)
Server-->>GenericOAuthPlugin: { url?, redirect }
alt redirect == true
GenericOAuthPlugin->>GenericOAuthPlugin: makeAuthorizationProxyURL(authURL)
GenericOAuthPlugin->>OAuthHandler: authenticate(authURL, callbackScheme)
OAuthHandler->>ASWebAuth: start web auth session
ASWebAuth->>User: show web consent
User-->>ASWebAuth: authorize & redirect
ASWebAuth-->>OAuthHandler: return callback URL
OAuthHandler-->>GenericOAuthPlugin: deliver auth result
end
GenericOAuthPlugin->>BetterAuthClient: store session cookie
BetterAuthClient-->>GenericOAuthView: return signed-in user/session
GenericOAuthView->>User: display profile or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Core/Session/OAuthHandler.swift (1)
39-70: Handle failedASWebAuthenticationSession.start()to avoid hanging awaits.
ASWebAuthenticationSession.start()returnsfalsewhen the session can’t be presented (for example, restrictions, misconfigured scheme, or missing presentation anchor). In that case our continuation never resumes, soauthenticatesuspends forever and the new public API surfaces a hang to callers. Please guard thestart()result and resume with an error when it’sfalse.(learn.microsoft.com)Suggested fix:
webAuthSession?.presentationContextProvider = self webAuthSession?.prefersEphemeralWebBrowserSession = false - webAuthSession?.start() + guard webAuthSession?.start() == true else { + webAuthSession = nil + completion = nil + continuation.resume( + throwing: BetterAuthSwiftError(message: "Failed to start web auth session") + ) + return + }
🧹 Nitpick comments (4)
Example/BetterAuthSwiftExample/Plugins/GenericOAuthView.swift (2)
50-60: Consider handling signOut errors.The
try?operator silently suppresses sign-out errors, leaving users unaware if logout fails. Consider capturing and displaying the error.Apply this diff:
Button { - Task { try? await client.signOut() } + Task { + do { + try await client.signOut() + } catch { + errorMessage = "Failed to sign out: \(error.localizedDescription)" + } + } } label: {
137-142: Consider making callback URL configurable.The callback URL is hardcoded. While acceptable for example code, consider extracting it to a configuration constant or environment variable for better maintainability.
Example:
private let callbackURL = "betterauthswiftexample://" // Then in the function: _ = try await client.signIn.oauth2( with: .init( providerId: providerId, callbackURL: callbackURL ) )Sources/Plugins/GenericOAuth/Models/Payloads.swift (1)
4-33: Add documentation for public API.The
GenericOAuthSignInRequeststruct is part of the public API but lacks documentation. Add doc comments explaining the purpose of each property, especially the conditional requirements (e.g.,callbackURLis required when redirect is enabled).Example:
/// Request payload for Generic OAuth2 sign-in. public struct GenericOAuthSignInRequest: Codable, Sendable { /// The OAuth provider identifier (e.g., "google", "github"). public let providerId: String /// Callback URL for OAuth redirect. Required when redirect flow is used. public let callbackURL: String? /// URL to redirect to on authentication error. public let errorCallbackURL: String? // ... document remaining properties }Sources/Plugins/GenericOAuth/GenericOAuthPlugin.swift (1)
26-42: Enhance API documentation.The method includes basic error documentation but lacks details about the parameters, return value structure, and usage example.
Apply this diff to improve documentation:
- /// Make a request to **/oauth2/link**. - /// - Parameter body: ``GenericOAuthLinkRequest`` - /// - Returns: ``GenericOAuthLink`` - /// - Throws: ``/BetterAuth/BetterAuthApiError`` - ``/BetterAuth/BetterAuthSwiftError`` + /// Link an OAuth2 provider to the current user's account. + /// + /// This method initiates the OAuth2 account linking flow by making a request to the + /// **/oauth2/link** endpoint. + /// + /// - Parameter body: The link request containing provider ID, callback URL, and optional scopes. + /// - Returns: An `APIResource` containing the OAuth URL and redirect flag. + /// - Throws: + /// - `BetterAuthApiError` if the API returns an error response + /// - `BetterAuthSwiftError` if the client has been deallocated public func link(with body: GenericOAuthLinkRequest) async throws
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
Example/BetterAuthSwiftExample/ContentView.swift(5 hunks)Example/BetterAuthSwiftExample/Plugins/GenericOAuthView.swift(1 hunks)Package.swift(2 hunks)Sources/Core/Session/OAuthHandler.swift(4 hunks)Sources/Plugins/GenericOAuth/BetterAuthGenericOAuth.swift(1 hunks)Sources/Plugins/GenericOAuth/GenericOAuthPlugin.swift(1 hunks)Sources/Plugins/GenericOAuth/Models/Payloads.swift(1 hunks)Sources/Plugins/GenericOAuth/Network/Route.swift(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
Sources/Plugins/GenericOAuth/BetterAuthGenericOAuth.swift (3)
Sources/Core/Plugins/Events.swift (1)
emittingSignal(46-55)Sources/Plugins/GenericOAuth/GenericOAuthPlugin.swift (1)
makeAuthorizationProxyURL(44-66)Sources/Core/Session/OAuthHandler.swift (1)
extractScheme(10-22)
Example/BetterAuthSwiftExample/Plugins/GenericOAuthView.swift (3)
Sources/Core/BetterAuth.swift (1)
signOut(78-85)Example/BetterAuthSwiftExample/Plugins/PasskeyView.swift (1)
signIn(130-142)Sources/Plugins/GenericOAuth/BetterAuthGenericOAuth.swift (1)
oauth2(14-56)
🔇 Additional comments (5)
Sources/Plugins/GenericOAuth/Models/Payloads.swift (2)
35-43: Verify the intentional difference in URL optionality.The
urlproperty is optional inGenericOAuthSignInResponsebut required inGenericOAuthLinkResponse. Confirm this difference is intentional and reflects the actual API contract, as it could indicate an inconsistency in the backend responses.If the responses should be consistent, consider making both optional or both required based on the API specification.
45-62: Good API design for required callback.The
GenericOAuthLinkRequestcorrectly makescallbackURLrequired (non-optional) whileGenericOAuthSignInRequesthas it optional. This distinction makes sense as account linking always requires a callback, whereas sign-in may support non-redirect flows.Sources/Plugins/GenericOAuth/GenericOAuthPlugin.swift (2)
4-12: LGTM: Clean plugin factory implementation.The plugin factory follows the standard pattern with proper public access control and initialization.
14-14: @mainactor usage is correct and follows the established architectural pattern.The
@MainActorannotation on theGenericOAuthclass is required—thePluggableprotocol itself is marked@MainActor, and all plugin implementations in the codebase consistently use this annotation (Username, TwoFactor, PhoneNumber, EmailOTP, Passkey, MagicLink, Anonymous). This is an intentional architectural pattern applied throughout the SDK (core classes likeBetterAuthClient,SessionStore, andOAuthHandleralso use it). Async network calls execute off the main thread; only the coordination layer runs on the main thread for thread safety and consistency.Example/BetterAuthSwiftExample/Plugins/GenericOAuthView.swift (1)
40-43: No action required. The User model defines bothnameas non-optionalStringproperties, making the access pattern in lines 40–43 safe from nil-related crashes.Likely an incorrect or invalid review comment.
| import BetterAuth | ||
| import BetterAuthGenericOAuth | ||
|
|
||
| struct genericOAuthView: View { |
There was a problem hiding this comment.
Fix type naming convention.
The struct name genericOAuthView violates Swift naming conventions. Type names should use PascalCase.
Apply this diff:
-struct genericOAuthView: View {
+struct GenericOAuthView: View {Note: The filename should also be updated from genericOAuthView.swift to GenericOAuthView.swift to match.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| struct genericOAuthView: View { | |
| struct GenericOAuthView: View { |
🤖 Prompt for AI Agents
In Example/BetterAuthSwiftExample/Plugins/GenericOAuthView.swift around line 12,
the struct name `genericOAuthView` violates Swift PascalCase type naming; rename
the struct to `GenericOAuthView` and update all references/usages accordingly,
and also rename the source file from genericOAuthView.swift to
GenericOAuthView.swift to match the new type name.
| var components = URLComponents( | ||
| url: client.baseUrl, | ||
| resolvingAgainstBaseURL: false | ||
| ) | ||
| components?.path.append(BetterAuthRoute.expoAuthorizationProxy.path) | ||
| components?.queryItems = [ | ||
| URLQueryItem( | ||
| name: "authorizationURL", | ||
| value: authorizationURL | ||
| ) | ||
| ] |
There was a problem hiding this comment.
Potential path construction issue with trailing slashes.
Using path.append() may not correctly handle cases where client.baseUrl.path or expoAuthorizationProxy.path contain leading/trailing slashes, potentially resulting in malformed URLs (e.g., double slashes or missing separators).
Apply this diff for more robust path handling:
var components = URLComponents(
url: client.baseUrl,
resolvingAgainstBaseURL: false
)
- components?.path.append(BetterAuthRoute.expoAuthorizationProxy.path)
+ if let path = components?.path {
+ let separator = path.hasSuffix("/") ? "" : "/"
+ let routePath = BetterAuthRoute.expoAuthorizationProxy.path.hasPrefix("/")
+ ? String(BetterAuthRoute.expoAuthorizationProxy.path.dropFirst())
+ : BetterAuthRoute.expoAuthorizationProxy.path
+ components?.path = path + separator + routePath
+ }
components?.queryItems = [Alternatively, consider using URL(fileURLWithPath:relativeTo:) or appendingPathComponent() if baseUrl supports it.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var components = URLComponents( | |
| url: client.baseUrl, | |
| resolvingAgainstBaseURL: false | |
| ) | |
| components?.path.append(BetterAuthRoute.expoAuthorizationProxy.path) | |
| components?.queryItems = [ | |
| URLQueryItem( | |
| name: "authorizationURL", | |
| value: authorizationURL | |
| ) | |
| ] | |
| var components = URLComponents( | |
| url: client.baseUrl, | |
| resolvingAgainstBaseURL: false | |
| ) | |
| if let path = components?.path { | |
| let separator = path.hasSuffix("/") ? "" : "/" | |
| let routePath = BetterAuthRoute.expoAuthorizationProxy.path.hasPrefix("/") | |
| ? String(BetterAuthRoute.expoAuthorizationProxy.path.dropFirst()) | |
| : BetterAuthRoute.expoAuthorizationProxy.path | |
| components?.path = path + separator + routePath | |
| } | |
| components?.queryItems = [ | |
| URLQueryItem( | |
| name: "authorizationURL", | |
| value: authorizationURL | |
| ) | |
| ] |
🤖 Prompt for AI Agents
In Sources/Plugins/GenericOAuth/GenericOAuthPlugin.swift around lines 49 to 59,
the current use of components?.path.append(...) can produce malformed paths when
client.baseUrl.path or BetterAuthRoute.expoAuthorizationProxy.path contain
leading/trailing slashes; replace the append with a robust join: construct the
final path by normalizing both segments (trim trailing slash from base path and
leading slash from the route) and then concatenate with a single "/" between
them, or better, build the URL via client.baseUrl.appendingPathComponent(...) or
URL(fileURLWithPath:relativeTo:) so Apple's path-handling API manages
separators; ensure components?.path is set to the resulting normalized path and
not appended naively.
ouwargui
left a comment
ouwargui
left a comment
There was a problem hiding this comment.
Hey, great job. Specially considering that I haven't setup any contribution guide yet.
I left a couple comments and I still need to manually test it before approving it, but lgtm so far.
Co-authored-by: Guilherme D'Alessandro <guikemon2@gmail.com>
Co-authored-by: Guilherme D'Alessandro <guikemon2@gmail.com>
|
Hey, thanks so much! I needed it, so I didn’t want to wait :) |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Core/Session/OAuthHandler.swift (1)
24-71: Changeauthenticatemethod frompublictopackage funcfor architectural consistency.The method is called from both Sources/Core/BetterAuth.swift and Sources/Plugins/GenericOAuth/BetterAuthGenericOAuth.swift, both within the same Swift package structure. Since OAuthHandler is marked
package class, theauthenticatemethod should also usepackage funcrather thanpublic functo maintain consistent visibility boundaries. This aligns with the architecture where plugins access Core functionality through the same-packagepackagevisibility level.
♻️ Duplicate comments (1)
Example/BetterAuthSwiftExample/Plugins/GenericOAuthView.swift (1)
2-2: Update filename comment to match Swift naming conventions.The comment still references
genericOAuthView.swift, but the struct has been correctly renamed toGenericOAuthView. Update the filename comment to match the expected filenameGenericOAuthView.swift.Apply this diff:
-// genericOAuthView.swift +// GenericOAuthView.swift
🧹 Nitpick comments (3)
Example/BetterAuthSwiftExample/Plugins/GenericOAuthView.swift (3)
25-25: Consider making the nil-coalescing more explicit.The expression
URL(string: user.image ?? "")works but relies onURL(string: "")returning nil. For clarity, consider using optional chaining directly onuser.image.Apply this diff:
- AsyncImage(url: URL(string: user.image ?? "")) { image in + AsyncImage(url: user.image.flatMap { URL(string: $0) }) { image in
50-60: Consider adding error feedback for sign-out failures.The
try?operator silently ignores sign-out errors, which could leave users confused if the operation fails. Consider capturing and displaying errors similar to the sign-in flow.Apply this diff:
Button { - Task { try? await client.signOut() } + Task { + do { + try await client.signOut() + } catch { + errorMessage = "Failed to sign out: \(error.localizedDescription)" + } + } } label: {
132-148: Consider extracting the hardcoded callback URL.The callback URL
"betterauthswiftexample://"is hardcoded in the sign-in logic. For better maintainability, consider extracting this as a constant at the file or struct level, especially if it needs to be kept in sync with URL scheme configurations.Apply this diff:
struct GenericOAuthView: View { @EnvironmentObject var client: BetterAuthClient + private let callbackURL = "betterauthswiftexample://" @State private var providerId = "" @State private var isSigningIn = falseThen update the function:
_ = try await client.signIn.oauth2( with: .init( providerId: providerId, - callbackURL: "betterauthswiftexample://" + callbackURL: callbackURL ) )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Example/BetterAuthSwiftExample/Plugins/GenericOAuthView.swift(1 hunks)Sources/Core/Session/OAuthHandler.swift(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Example/BetterAuthSwiftExample/Plugins/GenericOAuthView.swift (2)
Sources/Core/BetterAuth.swift (1)
signOut(78-85)Sources/Plugins/GenericOAuth/BetterAuthGenericOAuth.swift (1)
oauth2(14-56)
🔇 Additional comments (5)
Example/BetterAuthSwiftExample/Plugins/GenericOAuthView.swift (2)
85-109: LGTM!The OAuth2 form implementation is well-structured with appropriate TextField configuration (disabled autocapitalization and autocorrection for provider ID), correct button disabling logic, and clear error messaging.
19-155: LGTM!The conditional compilation for platform-specific behavior is well-implemented, providing a clear unsupported message for watchOS while maintaining full functionality on other platforms.
Sources/Core/Session/OAuthHandler.swift (3)
6-6: Past review feedback applied; consider access level consistency.The
packageaccess level has been applied as suggested in the previous review. However, ensure thatpackageis the intended visibility level given that several methods within this class are markedpublic(lines 10, 24, 89), which creates an access level mismatch. If external modules need to use this class, consider making itpublicto match the method visibility.Based on learnings.
89-102: Protocol conformance may requirepublicvisibility.The
presentationAnchormethod implements theASWebAuthenticationPresentationContextProvidingprotocol requirement. Since the protocol is public (from Apple's framework), the implementing method may need to remainpubliceven if the class ispackage-visible. However, verify this is the intended behavior.
10-22: ****The review comment's concern is based on a misunderstanding of Swift's
packageaccess level. TheOAuthHandlerclass is markedpackage, and in Swift 5.9+, thepackagemodifier grants access to all targets within the same Swift package.The Package.swift file confirms that both call sites—
Sources/Core/BetterAuth.swiftandSources/Plugins/GenericOAuth/BetterAuthGenericOAuth.swift—belong to different targets (BetterAuthandBetterAuthGenericOAuth) within the sameBetterAuthSwiftpackage. Sincepackageis package-scoped (not target-scoped), thepackage class OAuthHandleris accessible from both targets. Thepublic func extractSchemeis the correct access level for a method intended to be part of the class's public API within the package.There is no access level inconsistency—the current implementation is correct.
Likely an incorrect or invalid review comment.
|
@newtondotcom hey, sorry for taking too long to review - i haven't had time to test this yet, could you provide a video of the feature working? in this video it would be nice to go through:
let me know if you can do it, otherwise it'll take a little longer for me to be able to test it and approve |
|
hey @ouwargui I have been kind of busy lately, I will when I have time ! |
|
would love to try this! |
3 participants
Summary by CodeRabbit