Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[REVIEWABLE] Sign in with Apple on iOS #4034

Merged
merged 16 commits into from
Jun 30, 2020

Commits on Jun 29, 2020

  1. webAuth types: Annotate generateOtp return value.

    Chris Bobbe authored and chrisbobbe committed Jun 29, 2020
    Configuration menu
    Copy the full SHA
    5d82ed0 View commit details
    Browse the repository at this point in the history

Commits on Jun 30, 2020

  1. ZulipButton types: Let text be LocalizableText, not just string.

    Now we can use `values` in the text we pass in. The only thing
    ZulipButton currently does with `text` is pass it to TranslatedText,
    which already accepts LocalizableText.
    chrisbobbe committed Jun 30, 2020
    Configuration menu
    Copy the full SHA
    6907fe7 View commit details
    Browse the repository at this point in the history
  2. AuthScreen: Translate "Log in with..." button for all auth methods.

    Before this, it wouldn't work when `auth.displayName` is something
    other than the values we know about in advance.
    
    So, make it a fixed string with a placeholder and pass the value of
    `auth.displayName` in through `values`.
    chrisbobbe committed Jun 30, 2020
    Configuration menu
    Copy the full SHA
    8db5a4d View commit details
    Browse the repository at this point in the history
  3. button text: Change "Log in with" to "Sign in with" everywhere.

    Apple's Human Interface Guidelines doc on the "Sign in with Apple"
    button [1] (to be added in an upcoming commit) is quite clear that,
    for logging in, the text must say "Sign in with Apple", with that
    exact capitalization and wording.
    
    In order to keep the buttons consistent, change the others to match.
    
    [1]: https://developer.apple.com/design/human-interface-guidelines/sign-in-with-apple/overview/buttons/.
    chrisbobbe committed Jun 30, 2020
    Configuration menu
    Copy the full SHA
    98a626a View commit details
    Browse the repository at this point in the history
  4. jest: Add @unimodules/ to transformModulesWhitelist.

    Empirically (when running Jest), there is some uncompiled JavaScript
    in this directory; we see errors with stack traces that suggest that
    when we use Expo modules, e.g., `expo-apple-authentication`.
    
    Mocking those Expo modules seems to be also necessary, sometimes, as
    we still get errors about things being undefined, after we've
    avoided the syntax errors.
    chrisbobbe committed Jun 30, 2020
    Configuration menu
    Copy the full SHA
    6e0d3bf View commit details
    Browse the repository at this point in the history
  5. iOS deps: Add expo-apple-authentication at latest.

    Following their instructions for a "bare" app (as opposed to
    "managed" by Expo) [1], which include the addition of the "Sign in
    with Apple" entitlement, which we did through the Xcode UI as
    instructed.
    
    We don't have to worry about excluding this package from being
    linked on Android, it seems; its `unimodule.json` specifies
    `"platforms": ["ios"]`.
    
    Empirically (when running Jest), there seems to be uncompiled
    JavaScript in this package, so add an entry in
    `transformModulesWhitelist`. Also mock the module, as we then get
    errors about things being undefined.
    
    [1]: https://github.com/expo/expo/tree/1c5eb9818/packages/expo-apple-authentication
    Chris Bobbe authored and chrisbobbe committed Jun 30, 2020
    Configuration menu
    Copy the full SHA
    c7d982e View commit details
    Browse the repository at this point in the history
  6. apple-auth libdef: Allow onPress to return Promise<void>

    The example in the doc at
    https://docs.expo.io/versions/latest/sdk/apple-authentication/#usage
    has this return type for `onPress`, and we want it too. Omitting
    `Promise<void>` doesn't seem to be well-considered.
    Chris Bobbe authored and chrisbobbe committed Jun 30, 2020
    Configuration menu
    Copy the full SHA
    1c638f4 View commit details
    Browse the repository at this point in the history
  7. expo-apple-auth libdef: Make identityToken, authorizationCode non-opt…

    …ional
    
    The Apple docs are frustratingly vague, but it doesn't seem to be
    the case that "no credential" and "yes credential, but no
    `identityToken`/`authorizationCode`" are separate auth failure modes
    that we have to distinguish.
    
    As I theorize in an Expo issue, these fields being marked optional
    does correspond to the Swift version of an Apple doc [1], where
    they're also marked optional.
    
    We still don't know exactly why that Apple doc has them marked
    optional, but corresponding values in a different protocol, the one
    you use on other platforms (i.e., not the native iOS flow), *are*
    marked optional for a particular reason [2]. In particular, they
    might be missing if you said you didn't want them. But...here, there
    doesn't seem to be a way to say you don't want them. So, they must
    be there.
    
    Also, in the JavaScript layer of `expo-apple-authentication`, an
    error is thrown if `identityToken` or `authorizationCode` is
    missing. (See `signInAsync` in `build/AppleAuthentication.js`.)
    
    [1]: expo/expo#8446 (comment)
    [2]: expo/expo#8446 (comment)
    chrisbobbe committed Jun 30, 2020
    Configuration menu
    Copy the full SHA
    8d056ed View commit details
    Browse the repository at this point in the history
  8. auth ui: Add iOS-compliant "Sign in with Apple" button component.

    In Apple's Human Interface Guidelines doc about the button to be
    used for "Sign in with Apple", they recommend using a standard
    component from the iOS SDK if possible, but that you can use a
    custom button, subject to design constraints, otherwise. That doc is
    https://developer.apple.com/design/human-interface-guidelines/sign-in-with-apple/overview/buttons/.
    
    So, make a component, only to be used on iOS, that will check if the
    standard button is available, and use it, if so. The fallback custom
    button will show on iOS <13.
    
    Since Apple doesn't review our Google Play Store submissions, we
    shouldn't even have to use the custom button on Android; we can just
    use the same ZulipButton as the other social auth buttons use. I
    think it looks better when they all look uniform like this.
    Chris Bobbe authored and chrisbobbe committed Jun 30, 2020
    Configuration menu
    Copy the full SHA
    cc8b569 View commit details
    Browse the repository at this point in the history
  9. webAuth [nfc]: Extract implementation into generateRandomToken func…

    …tion.
    
    In an upcoming commit, we'll need to generate a random token (the
    same implementation is fine), but without the OTP semantics.
    chrisbobbe committed Jun 30, 2020
    Configuration menu
    Copy the full SHA
    6a9db46 View commit details
    Browse the repository at this point in the history
  10. config: Add appOwnDomains.

    These will be useful, in an upcoming commit, to grant the privilege
    of the native "Sign in with Apple" flow (in which sensitive
    `id_token`s will be sent to the server) only to domains that have
    trust with the mobile app.
    
    To do that securely, the base URL we'll be sending requests to
    (basically what the user entered on the first screen) will be
    checked to see if its host matches one of these.
    chrisbobbe committed Jun 30, 2020
    Configuration menu
    Copy the full SHA
    f900521 View commit details
    Browse the repository at this point in the history
  11. auth: Handle "Sign in with Apple".

    From our perspective, this works just like any other social auth in
    many cases. It's set up to conform to our protocol of giving a
    redirect to a URL with the `zulip://` scheme containing the API key.
    
    These cases (where the normal protocol is used) are
    
    - Android
    - iOS before version 13
    - On servers not operated by the same people as the publishers of
      the mobile app (so for the official Zulip app, by Kandra Labs) [1]
    
    In the remaining cases (Kandra-hosted realms on iOS 13+), we'll do
    the native flow. This means we initiate the auth by using an iOS API
    that natively handles querying for, e.g., the user's fingerprint,
    face, or password, and gives us an `id_token`, which we send to the
    server. Currently, we do this by opening the browser and awaiting
    the same `zulip://` redirect, same as in the normal protocol.
    
    As a followup, we may want to tweak this so it's not necessary to
    ever open the browser in the native flow. We could just use `fetch`
    and expect the API key in the response.
    
    [1]: We don't want to send an `id_token` from the native flow to one
         of these realms; see discussion around
         https://chat.zulip.org/#narrow/stream/3-backend/topic/apple.20auth/near/918714.
    Chris Bobbe authored and gnprice committed Jun 30, 2020
    Configuration menu
    Copy the full SHA
    b72e9a3 View commit details
    Browse the repository at this point in the history
  12. Configuration menu
    Copy the full SHA
    c7b37e2 View commit details
    Browse the repository at this point in the history
  13. Configuration menu
    Copy the full SHA
    c19cbe2 View commit details
    Browse the repository at this point in the history
  14. Configuration menu
    Copy the full SHA
    9c13381 View commit details
    Browse the repository at this point in the history
  15. url [nfc]: Factor out helper tryParseUrl.

    This allows callers like `canUseNativeAppleFlow` to use plain
    conditionals instead of try/catch, and use more `const` so that
    the type-checker is better able to see when a variable can no longer
    be a value like `undefined`.
    
    In this case, for example, if instead of this change a `return false`
    is simply added to the `catch` block, then Flow still believes the
    `host !== undefined` check below is necessary (and that without it
    the `host.endsWith(…)` call is ill-typed), because `host` is a
    mutable binding and Flow doesn't see how to prove that it doesn't get
    set back to `undefined`.  With `const` everywhere, the logic to rule
    out such possibilities is more straightforward and the type-checker
    is able to see it.
    gnprice committed Jun 30, 2020
    Configuration menu
    Copy the full SHA
    ba6b6b3 View commit details
    Browse the repository at this point in the history