MBL-1159: Exchange temporary token for OAuth token and login #1950

amy-at-kickstarter · 2024-02-20T16:25:10Z

📲 What

This implements the MVP of login with OAuth.

It works like this:

Open a web authentication session
If it completes successful, the website redirects to a URL which includes a tempoary authorization code
We exchange that code and a secret for the real OAuth token
We use that OAuth token to fetch the user's information
...and then save that to the app environment login.

🤔 Why

We are porting the app to use OAuth via a web authentication session to improve security.

👀 See

This snazzy demo!

better.mp4

amy-at-kickstarter · 2024-02-20T16:46:11Z

Kickstarter-iOS/Features/LoginTout/Controller/LoginToutViewController.swift

+    return OAuth.createAuthorizationSession { [weak self] result in
+      switch result {
+      case .loggedIn:
+        self?.viewModel.inputs.environmentLoggedIn()


I went back-and-forth on where to put which pieces of logic. I settled on having the actual login call live in OAuth.swift, but the view controller is responsible for updating the UI when the login is complete.

amy-at-kickstarter · 2024-02-20T16:48:28Z

KsApi/ServiceType.swift

@@ -284,6 +284,9 @@ public protocol ServiceType {
  /// Fetch the logged-in user's data.
  func fetchUserSelf() -> SignalProducer<User, ErrorEnvelope>

+  /// Fetch the logged-in user's data.
+  func fetchUserSelf_combine(withToken: String) -> AnyPublisher<User, ErrorEnvelope>


This will make more sense in context - AppEnvironment.login takes a token and a user. Our new endpoint returns the new oauth token but not the user; so I needed a way to fetch the current user manually, without actually being logged in. That resulted in this "fetch me the user, given this token" endpoint.

I've discussed this with @santiDotIO and @Arkariang , hopefully we'll be able to get the User in the original exchange call, and then I'll clean this up.

amy-at-kickstarter · 2024-02-20T16:48:51Z

KsApi/lib/Route.swift

@@ -243,6 +248,9 @@ internal enum Route {
      case .userSelf:
        return (.GET, "/v1/users/self", [:], nil)

+      case let .userSelfWithToken(token):
+        return (.GET, "/v1/users/self", ["oauth_token": token], nil)


Parameters passed in here will override the defaults, which in this case will be missing the oauth_token.

amy-at-kickstarter · 2024-02-20T16:49:40Z

Library/OAuth.swift

+            authenticationError.code == .canceledLogin {
+            onComplete(.cancelled)
+          } else {
+            onComplete(.failure(errorMessage: Strings.Something_went_wrong_please_try_again()))


These are all errors that aren't user-solvable, so giving them a generic error message.

amy-at-kickstarter · 2024-02-20T16:54:28Z

KsApi/MockService.swift

+    func exchangeTokenForOAuthToken(params _: OAuthTokenExchangeParams)
+      -> AnyPublisher<OAuthTokenExchangeResponse, ErrorEnvelope> {
+      // TODO: Implement for testing.
+      Empty(completeImmediately: false).eraseToAnyPublisher()


I need to come back and evaluate what of this flow I can test. It's all fairly tightly integrated with Apple's authentication controller. I could add tests just for these new endpoints, but since network tests go against these mocks anyways, it doesn't seem super valuable to me...

@ifosli @scottkicks I'm open to suggestions for what seems worthwhile to test here.

amy-at-kickstarter · 2024-02-20T17:22:17Z

Converting to draft while I write some tests.

amy-at-kickstarter · 2024-02-20T19:02:30Z

Library/OAuth.swift

@@ -55,4 +65,67 @@ public struct OAuth {
      return nil
    }
  }
+
+  internal static func handleRedirect(


Pulled this out into its own method to make it testable, without needing to deal with ASWebAuthenticationSession in the tests. It's not the prettiest interface, but gets the job done.

amy-at-kickstarter · 2024-02-20T19:04:25Z

Library/OAuth.swift

+    guard error == nil else {
+      if let authenticationError = error as? ASWebAuthenticationSessionError,
+        authenticationError.code == .canceledLogin {
+        DispatchQueue.main.async {


These dispatches are here to prevent us from releasing Zalgo - they ensure that the completion block is only ever called asynchronously, instead of a mix of synchronously and asynchronously.

amy-at-kickstarter · 2024-02-20T19:07:28Z

Kickstarter-iOS/Features/LoginTout/Controller/LoginToutViewController.swift

+          message: errorMessage,
+          preferredStyle: .alert
+        )
+        alert.addAction(UIAlertAction(title: Strings.login_errors_button_ok(), style: .cancel))


Interestingly these strings existed, but were unused 🤷‍♀️

amy-at-kickstarter · 2024-02-20T19:18:48Z

Library/OAuthTests.swift

+@testable import Library
+import XCTest
+
+final class OAuthTests: XCTestCase {


These tests are all a little tautological (they really test the specific internal implementation), but I feel like I'd rather have them than not.

ifosli · 2024-02-21T17:54:33Z

Library/OAuth.swift

+      }
+      .sink { result in
+        if case let .failure(error) = result {
+          let message = error.errorMessages.first ?? Strings.login_errors_unable_to_log_in()


nit: Will the error message here be localized? If not, we may want to just keep the "unable to log in" error, even though it may be less helpful/more generic

Good question. I assumed that server error messages are localized, but I've never actually checked - is that assumption incorrect?

…and to fetch current user with a given token

amy-at-kickstarter force-pushed the feat/adyer/MBL-1159/exchange-for-token branch from c60f91a to a66eb25 Compare February 20, 2024 16:43

amy-at-kickstarter commented Feb 20, 2024

View reviewed changes

amy-at-kickstarter requested review from ifosli, Arkariang and santiDotIO February 20, 2024 16:54

amy-at-kickstarter self-assigned this Feb 20, 2024

amy-at-kickstarter marked this pull request as ready for review February 20, 2024 16:55

amy-at-kickstarter marked this pull request as draft February 20, 2024 17:22

amy-at-kickstarter commented Feb 20, 2024

View reviewed changes

amy-at-kickstarter marked this pull request as ready for review February 20, 2024 19:05

amy-at-kickstarter commented Feb 20, 2024

View reviewed changes

amy-at-kickstarter force-pushed the feat/adyer/MBL-1159/exchange-for-token branch 2 times, most recently from c1bf6fa to 6baad91 Compare February 20, 2024 19:15

amy-at-kickstarter commented Feb 20, 2024

View reviewed changes

ifosli approved these changes Feb 21, 2024

View reviewed changes

amy-at-kickstarter added 4 commits February 21, 2024 14:22

MBL-1159: Add endpoints to exchange temporary token for OAuth token, …

5f79436

…and to fetch current user with a given token

MBL-1159: Exchange temporary token for OAuth token and login

d4cff73

Add OAuthTests and make OAuth more testable

bee4e1d

Add two more tests to OAuthTests

0917b15

amy-at-kickstarter force-pushed the feat/adyer/MBL-1159/exchange-for-token branch from 9aa43d3 to 0917b15 Compare February 21, 2024 19:22

amy-at-kickstarter merged commit 03df52c into main Feb 21, 2024
5 checks passed

amy-at-kickstarter deleted the feat/adyer/MBL-1159/exchange-for-token branch February 21, 2024 21:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MBL-1159: Exchange temporary token for OAuth token and login #1950

MBL-1159: Exchange temporary token for OAuth token and login #1950

amy-at-kickstarter commented Feb 20, 2024 •

edited

Loading

amy-at-kickstarter Feb 20, 2024

amy-at-kickstarter Feb 20, 2024

amy-at-kickstarter Feb 20, 2024

amy-at-kickstarter Feb 20, 2024

amy-at-kickstarter Feb 20, 2024

amy-at-kickstarter Feb 20, 2024

amy-at-kickstarter commented Feb 20, 2024

amy-at-kickstarter Feb 20, 2024 •

edited

Loading

amy-at-kickstarter Feb 20, 2024 •

edited

Loading

amy-at-kickstarter Feb 20, 2024

amy-at-kickstarter Feb 20, 2024

ifosli Feb 21, 2024

amy-at-kickstarter Feb 21, 2024

MBL-1159: Exchange temporary token for OAuth token and login #1950

MBL-1159: Exchange temporary token for OAuth token and login #1950

Conversation

amy-at-kickstarter commented Feb 20, 2024 • edited Loading

📲 What

🤔 Why

👀 See

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amy-at-kickstarter commented Feb 20, 2024

amy-at-kickstarter Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

amy-at-kickstarter Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amy-at-kickstarter commented Feb 20, 2024 •

edited

Loading

amy-at-kickstarter Feb 20, 2024 •

edited

Loading

amy-at-kickstarter Feb 20, 2024 •

edited

Loading