Apple sign in rownd by mfmurray · Pull Request #2 · rownd/ios

mfmurray · 2022-07-18T21:00:13Z

Added a requestAppleSignIn method in the Rownd class
Added apple sign in flow to the AppleSignUpCoordinator
- Once a user completes apple sign in, the jwtToken gets sent to Rown hub/auth, after that the email, first name, last name gets saved to the Rownd User
Still need to determine a solution for when Rownd might fail getting the access token or fail to update email or names...

mhamann · 2022-07-19T03:31:41Z

+ //Make the request
+ let authorizationController = ASAuthorizationController(authorizationRequests: [request])
+
+ //Assingnig the delegates


typo assigning

mhamann · 2022-07-19T03:38:13Z

+ return (vc?.view.window!)!
+ }
+
+ //If authorization is successfull then this method will get triggered


typo successful

mhamann · 2022-07-19T03:53:21Z

+ func authorizationController(controller: ASAuthorizationController, didCompleteWithError error: Error) {
+
+ //If there is any error will get it here
+ logger.trace("apple sign credential error")


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))")

Logger has been updated

mhamann · 2022-07-19T03:58:05Z

 struct TokenRequest: Codable {
- var refreshToken: String
+ var refreshToken: String?
+ var id_token: String?


Use swift convention here idToken and appId and then use the CodingKeys to translate to the proper string format.

mhamann · 2022-07-19T04:16:24Z


 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 {


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:
staticfunc fetchToken(refreshToken:String, withCompletion completion:@escaping(AuthState?)->Void)->Void{lettokenRequest=TokenRequest(refreshToken: refreshToken)returnfetchToken(tokenRequest: tokenRequest, withCompletion: completion)}
The second function overload would be something like this, handling the Apple ID token case:
staticfunc fetchToken(idToken:String, withCompletion completion:@escaping(AuthState?)->Void)->Void{lettokenRequest=TokenRequest(idToken: idToken, appId: store.state.appConfig.id)returnfetchToken(tokenRequest: tokenRequest, withCompletion: completion)}
Now, we can pack all of our shared business logic into the final function overload:
staticfunc fetchToken(tokenRequest:TokenRequest, withCompletion completion:@escaping(AuthState?)->Void)->Void{varresource=TokenResource() resource.headers =["Content-Type":"application/json"]letrequest=APIRequest(resource: resource)letencoder=JSONEncoder()varbody:Data?do{ body =try encoder.encode(tokenRequest)}catch{returncompletion(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.

Wow really like this logic! So cool you can call the same function and get different logic depending on the parameters

mhamann · 2022-07-19T04:29:28Z


 private override init() {}

+ public static func requestAppleSignIn() {


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:
enumRowndSignInHint{case appleId }
The function body would look something like:
publicstaticfunc 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

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.

…depending on refreshToken or idToken

mfmurray · 2022-07-19T12:33:51Z

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?

mhamann · 2022-07-19T18:24:27Z

+ 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


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>

mfmurray added 3 commits July 18, 2022 16:52

Added a Apple Sign in method to the Rownd class
61c507b

Added a id_token string param to the refreshToken func in Auth
f613feb

Added a Apple Sign Up coordinator to manage the apple sign up flow
a6c3ccb

mfmurray requested a review from mhamann July 18, 2022 21:00

mfmurray added 3 commits July 18, 2022 17:12

Added specific appleSignIn message for WebView
e1f1375

updated naming for apple sign in WebView trigger
048af95

close hub when apple sign in is requested
d372acc

mhamann reviewed Jul 19, 2022
View reviewed changes

mhamann requested changes Jul 19, 2022
View reviewed changes

mfmurray added 3 commits July 19, 2022 08:28

Updated requestSignIn rownd method to use hints for apple request
854dcb8

updated refreshToken to seperate functions with different parameters …
4083fae
…depending on refreshToken or idToken

updated spelling mistakes
112a8fa

mfmurray added 3 commits July 19, 2022 09:21

Added a keychain framework
4ad8e74

Save email, first name, and last name in keychain in case Rownd fails
5b1a89c

replaced storing in keychain with UserDefaults in AppleSignUpCoordinator
29712e6

mhamann reviewed Jul 19, 2022
View reviewed changes

mhamann approved these changes Jul 19, 2022
View reviewed changes

mhamann merged commit df514f6 into mainJul 19, 2022

mhamann deleted the appleSignInRownd branch July 19, 2022 18:57


		private override init() {}

		public static func requestAppleSignIn() {

Uh oh!

Conversation

mfmurray commented Jul 18, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhamannJul 19, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhamann left a comment

Choose a reason for hiding this comment

Uh oh!

mfmurray commented Jul 19, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mfmurray commented Jul 18, 2022•
edited
Loading

mhamannJul 19, 2022•
edited
Loading