Apple sign in rownd#2
Conversation
| //Make the request | ||
| let authorizationController = ASAuthorizationController(authorizationRequests: [request]) | ||
|
|
||
| //Assingnig the delegates |
| return (vc?.view.window!)! | ||
| } | ||
|
|
||
| //If authorization is successfull then this method will get triggered |
| func authorizationController(controller: ASAuthorizationController, didCompleteWithError error: Error) { | ||
|
|
||
| //If there is any error will get it here | ||
| logger.trace("apple sign credential error") |
There was a problem hiding this comment.
we can provide a bit better log message here to help with debugging. something like logger.error("An error occurred while signing in with Apple. Error: \String(describing: error))")
There was a problem hiding this comment.
Logger has been updated
| struct TokenRequest: Codable { | ||
| var refreshToken: String | ||
| var refreshToken: String? | ||
| var id_token: String? |
There was a problem hiding this comment.
Use swift convention here idToken and appId and then use the CodingKeys to translate to the proper string format.
|
|
||
| class Auth { | ||
| static func refreshToken(refreshToken: String, withCompletion completion: @escaping (AuthState?) -> Void) -> Void { | ||
| static func refreshToken(refreshToken: String?, id_token: String?, withCompletion completion: @escaping (AuthState?) -> Void) -> Void { |
There was a problem hiding this comment.
Good re-use of this function! Let's rename this to be a bit more generic like func fetchToken()
Then, rather than using two token arguments here, we can overload this function in the following way.
The first function overload would look something like this and handle the refresh token case:
static func fetchToken(refreshToken: String, withCompletion completion: @escaping (AuthState?) -> Void) -> Void {
let tokenRequest = TokenRequest(refreshToken: refreshToken)
return fetchToken(tokenRequest: tokenRequest, withCompletion: completion)
}The second function overload would be something like this, handling the Apple ID token case:
static func fetchToken(idToken: String, withCompletion completion: @escaping (AuthState?) -> Void) -> Void {
let tokenRequest = TokenRequest(idToken: idToken, appId: store.state.appConfig.id)
return fetchToken(tokenRequest: tokenRequest, withCompletion: completion)
}Now, we can pack all of our shared business logic into the final function overload:
static func fetchToken(tokenRequest: TokenRequest, withCompletion completion: @escaping (AuthState?) -> Void) -> Void {
var resource = TokenResource()
resource.headers = [
"Content-Type": "application/json"
]
let request = APIRequest(resource: resource)
let encoder = JSONEncoder()
var body: Data?
do {
body = try encoder.encode(tokenRequest)
} catch {
return completion(nil)
}
request.execute(...)
}So, then the Apple ID code can call the function expecting an ID token, the refresh flow can call this function with that type of token. This makes the internal API contract more explicit and easier to test.
There was a problem hiding this comment.
Wow really like this logic! So cool you can call the same function and get different logic depending on the parameters
|
|
||
| private override init() {} | ||
|
|
||
| public static func requestAppleSignIn() { |
There was a problem hiding this comment.
I think it might be preferable to have a function that looks more like: requestSignIn(with: RowndSignInHint)
Then, the sign-in hint is an enum:
enum RowndSignInHint {
case appleId
}The function body would look something like:
public static func requestSignIn(with: RowndSignInHint) {
switch(with) {
case .appleId:
appleSignUpCoordinator?.didTapButton()
default:
requestSignIn()
}
}If a customer wants the standard sign-in process, they simply call requestSignIn() as before, but if they want to hint that a particular method should be used, they can call requestSignIn(with: .appleId) and Rownd should honor that request if possible.
There may be times when we can't honor it (e.g., if Sign in with Apple isn't enabled within the customer's Rownd app), so a future update might add that logic to fallback to the default in such an event.
mhamann
left a comment
There was a problem hiding this comment.
This looks good! I left a few comments where we can streamline the code a bit.
In this PR or a follow-on, we should update the docs to reflect the new APIs as well as what code a customer might need to put into their app to get Sign in with Apple working.
Would you want me to work on updated the docs this week? Or just wait for Product Meeting? |
| let userAppleSignInData = AppleSignInData(email: email, firstName: fullName?.givenName, lastName: fullName?.familyName) | ||
| let encoder = JSONEncoder() | ||
| if let encoded = try? encoder.encode(userAppleSignInData) { | ||
| let defaults = UserDefaults.standard |
There was a problem hiding this comment.
You can just call Storage.store?.set(encoded, forKey: appleSignInDataKey) to use the SDK's common storage manager (see Models/Storage.swift)
Addresses review feedback from PR #125: - BottomSheetViewController: `isUserDismissalDisabled` now has a `didSet` that updates the live `sheetController` so the lock takes effect on an already-presented sheet. Track the Hub's last-requested dismissibility separately so releasing the lock restores it rather than unconditionally re-enabling (Qodo #1, #2). - Rownd: `releaseForcedConversionLock` now re-invokes `hide()` on the active HubViewProtocol so a post-auth auto-close that lost the race against `UserData.fetch` propagating `authLevel` still closes the sheet. Guarded on the prior locked state so unlocking a non-held lock is a no-op (avoids side effects in tests). - Add `Rownd._bottomSheetIsLocked` internal accessor for tests. - Tests: cover lock engagement on `.instant`, release on `.verified`, and the once-per-session gate after release. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(instant): make forced conversion sheet non-dismissible When `forceInstantUserConversion` is enabled, the sign-in sheet must be non-dismissible until the user actually adds an identifier. Previously the sheet defaulted to dismissible via swipe, tap-outside, and any Hub close message, which left customers with large populations of unconverted instant users. - BottomSheetViewController gains an `isUserDismissalDisabled` flag that disables swipe-to-dismiss and tap-outside-to-dismiss at presentation, and that the Hub's `can_touch_background_to_dismiss` message cannot re-enable. - `hideBottomSheet` and `HubViewController.hide()` honor the same lock, so Hub-dispatched `closeHubViewController`/`signOut` messages cannot close the sheet either. - InstantUsers observes the post-conversion auth-level transition and releases the lock once the user is no longer `.instant`, so the standard post-auth auto-close path still runs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(instant): apply lock live and re-trigger close on release Addresses review feedback from PR #125: - BottomSheetViewController: `isUserDismissalDisabled` now has a `didSet` that updates the live `sheetController` so the lock takes effect on an already-presented sheet. Track the Hub's last-requested dismissibility separately so releasing the lock restores it rather than unconditionally re-enabling (Qodo #1, #2). - Rownd: `releaseForcedConversionLock` now re-invokes `hide()` on the active HubViewProtocol so a post-auth auto-close that lost the race against `UserData.fetch` propagating `authLevel` still closes the sheet. Guarded on the prior locked state so unlocking a non-held lock is a no-op (avoids side effects in tests). - Add `Rownd._bottomSheetIsLocked` internal accessor for tests. - Tests: cover lock engagement on `.instant`, release on `.verified`, and the once-per-session gate after release. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(instant): make forced conversion sheet non-dismissible When `forceInstantUserConversion` is enabled, the sign-in sheet must be non-dismissible until the user actually adds an identifier. Previously the sheet defaulted to dismissible via swipe, tap-outside, and any Hub close message, which left customers with large populations of unconverted instant users. - BottomSheetViewController gains an `isUserDismissalDisabled` flag that disables swipe-to-dismiss and tap-outside-to-dismiss at presentation, and that the Hub's `can_touch_background_to_dismiss` message cannot re-enable. - `hideBottomSheet` and `HubViewController.hide()` honor the same lock, so Hub-dispatched `closeHubViewController`/`signOut` messages cannot close the sheet either. - InstantUsers observes the post-conversion auth-level transition and releases the lock once the user is no longer `.instant`, so the standard post-auth auto-close path still runs. * fix(instant): apply lock live and re-trigger close on release Addresses review feedback from PR #125: - BottomSheetViewController: `isUserDismissalDisabled` now has a `didSet` that updates the live `sheetController` so the lock takes effect on an already-presented sheet. Track the Hub's last-requested dismissibility separately so releasing the lock restores it rather than unconditionally re-enabling (Qodo #1, #2). - Rownd: `releaseForcedConversionLock` now re-invokes `hide()` on the active HubViewProtocol so a post-auth auto-close that lost the race against `UserData.fetch` propagating `authLevel` still closes the sheet. Guarded on the prior locked state so unlocking a non-held lock is a no-op (avoids side effects in tests). - Add `Rownd._bottomSheetIsLocked` internal accessor for tests. - Tests: cover lock engagement on `.instant`, release on `.verified`, and the once-per-session gate after release. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Uh oh!
There was an error while loading. Please reload this page.