aws-amplify / amplify-js

A declarative JavaScript library for application development using cloud services.
https://docs.amplify.aws/lib/q/platform/js
Apache License 2.0
9.42k stars 2.12k forks source link

Federated Sign In (Google) with Cognito is not working on Android and Expo Standalone #4003

Closed oriharel closed 4 years ago

oriharel commented 5 years ago

Describe the bug I'm using withOAuth() HOC. User is directed to the Google (code) flow, comes back to the app. Linking url event is triggered (with code parameter in the url) thus causing a call to Amplify token endpoint (with code_verifier). However, multiple additional Linking url events are fired, causing a throttle of token endpoint calls to trigger (all of them without code_verifier) that ends up with invalid_request and invalid_grant and eventually crashes the app. This happens only on Android (iOS works fine) and in Expo standalone (for some reason, when running the app in the Expo Client, it works fine because the url is stripped from the code this the token endpoint is not called).

To Reproduce Steps to reproduce the behavior: User Expo standalone Android app with Amplify withOAuth HOC to call google.

Expected behavior Avoid invalid calls to the token endpoint.

Smartphone (please complete the following information):

Here is the relevant config of Auth.configure:

const urlOpener = async (url, redirectUrl) => {
    const { type, url: newUrl } = await WebBrowser.openAuthSessionAsync(
        url,
        redirectUrl
    );

    if (Platform.OS === 'ios') {
        if (type === 'success') {
            await WebBrowser.dismissBrowser();
            return Linking.openURL(newUrl);
        }
    }
};
export const oauth = {
    domain: 'some.domain.com',
    scope: [
        'phone',
        'email',
        'openid',
        'profile',
        'aws.cognito.signin.user.admin'
    ],

    redirectSignIn: Linking.makeUrl('/signIn'),
    redirectSignOut: Linking.makeUrl('/signOut'),
    responseType: 'code',

    urlOpener: urlOpener
};
elorzafe commented 5 years ago

@oriharel thanks for your feedback. Can you share your package.json and expo-cli version?

oriharel commented 5 years ago

Sure:

{
  "name": "KahunMobile",
  "version": "1.2.0",
  "private": true,
  "devDependencies": {
    "@babel/core": "^7.2.2",
    "@storybook/addon-actions": "^4.1.13",
    "@storybook/addon-links": "^4.1.13",
    "@storybook/addons": "^4.1.13",
    "@storybook/react-native": "^4.1.13",
    "@types/jest": "23.3.10",
    "@types/react": "^16.5.0",
    "@types/react-native": "^0.57.20",
    "@types/react-native-vector-icons": "4.6.4",
    "@types/react-navigation": "^3.0.1",
    "apollo-cache-inmemory": "^1.6.0",
    "apollo-link-schema": "^1.1.6",
    "babel-core": "^7.0.0-0",
    "babel-eslint": "10.0.1",
    "babel-jest": "^23.6.0",
    "babel-preset-expo": "^5.0.0",
    "babel-runtime": "^6.26.0",
    "detox": "12.3.0",
    "detox-expo-helpers": "https://github.com/kahunmedical/detox-expo-helpers",
    "eslint": "5.11.0",
    "expo-cli": "^2.14.0",
    "expo-detox-hook": "^1.0.10",
    "graphql-code-generator": "^0.16.1",
    "graphql-codegen-introspection": "^0.15.2",
    "graphql-codegen-typescript-client": "0.16.1",
    "graphql-codegen-typescript-common": "0.16.1",
    "graphql-codegen-typescript-graphql-files-modules": "^0.17.0",
    "graphql-codegen-typescript-react-apollo": "0.16.1",
    "graphql-tools": "^4.0.4",
    "husky": "1.2.1",
    "jest-expo": "^33.0.0",
    "prettier": "^1.15.3",
    "prop-types": "^15.7.2",
    "react-dom": "16.5.0",
    "react-native-testing-library": "^1.5.0",
    "react-test-renderer": "^16.8.2",
    "ts-jest": "23.10.5",
    "typescript": "3.2.2",
    "whatwg-fetch": "^3.0.0"
  },
  "main": "node_modules/expo/AppEntry.js",
  "scripts": {
    "start": "expo start",
    "android": "expo start --android",
    "ios": "expo start --ios",
    "test": "node_modules/.bin/jest test/**/*.spec.js",
    "eject": "expo eject",
    "precommit": "yarn test",
    "detox-dev": "detox test -c ios.sim.dev",
    "detox-stag": "detox test -c ios.sim.staging",
    "codegen": "gql-gen --config codegen.yml",
    "storybook": "storybook start",
    "storybookApp": "NODE_ENV=storybook expo start"
  },
  "jest": {
    "preset": "jest-expo",
    "moduleFileExtensions": [
      "ts",
      "tsx",
      "js"
    ],
    "transform": {
      "^.+\\.(js)$": "<rootDir>/node_modules/babel-jest",
      "\\.(ts|tsx)$": "ts-jest"
    },
    "transformIgnorePatterns": [
      "node_modules/(?!(native-base|react-native|expo-font|expo-asset|@unimodules|expo-sensors|react-native-progress|expo-web-browser|expo-app-auth|react-native-animatable|react-native-reanimated|react-native-material-menu|react-native-modal|react-native-swiper|react-native-safe-area-view|react-navigation-material-bottom-tabs|react-native-paper|react-native-action-button|native-base-shoutem-theme|expo-errors|react-native-view-shot|expo-location|react-native-gesture-handler|react-navigation-drawer|react-navigation-stack|react-navigation-tabs|react-native-tab-view|react-native-screens|@react-navigation|expo-constants|expo-core|react-native-iphone-x-helper|expo-react-native-adapter|lottie-react-native|expo|react-native-maps|react-native-svg|react-native-branch|react-native-easy-grid|react-native-drawer|react-native-vector-icons|react-native-keyboard-aware-scroll-view|react-navigation|@expo|react-native-scrollable-tab-view|expo-mixpanel-analytics)/)"
    ],
    "globals": {
      "ts-jest": {
        "tsConfig": "tsconfig.jest.json",
        "diagnostics": false
      }
    },
    "testMatch": [
      "<rootDir>/test/**/*.spec.*"
    ]
  },
  "dependencies": {
    "apollo-boost": "^0.1.28",
    "apollo-client": "^2.6.0",
    "apollo-link": "^1.2.11",
    "apollo-link-context": "^1.0.17",
    "apollo-link-error": "^1.1.10",
    "apollo-link-http": "^1.5.14",
    "aws-amplify": "^1.1.36",
    "aws-amplify-react-native": "^2.1.16",
    "expo": "^33.0.0",
    "expo-constants": "~5.0.1",
    "expo-font": "~5.0.1",
    "expo-mixpanel-analytics": "^0.0.7",
    "expo-web-browser": "~5.0.2",
    "graphql": "^14.1.1",
    "graphql-deduplicator": "^2.0.3",
    "graphql-tag": "^2.10.1",
    "lodash": "4.17.11",
    "lottie-react-native": "~2.6.1",
    "mobx": "5.8.0",
    "mobx-persist": "^0.4.1",
    "mobx-react": "5.4.3",
    "native-base": "^2.12.1",
    "react": "16.8.3",
    "react-apollo": "^2.4.1",
    "react-native": "https://github.com/expo/react-native/archive/sdk-33.0.0.tar.gz",
    "react-native-action-button": "^2.8.5",
    "react-native-gesture-handler": "~1.2.1",
    "react-native-keyboard-aware-scroll-view": "^0.8.0",
    "react-native-material-menu": "^0.4.2",
    "react-native-modal": "^10.0.0",
    "react-native-paper": "^2.15.2",
    "react-native-popup-menu": "^0.15.6",
    "react-native-progress": "^3.6.0",
    "react-native-pure-chart": "^0.0.24",
    "react-native-simple-gauge": "^0.2.2",
    "react-native-svg-charts": "^5.2.0",
    "react-native-swiper": "^1.5.14",
    "react-native-tab-view": "^2.6.2",
    "react-navigation": "^3.11.0",
    "react-navigation-material-bottom-tabs": "^1.0.0",
    "sentry-expo": "^1.13.0",
    "serializr": "^1.5.1",
    "uuid": "^3.3.2"
  },
  "prettier": {
    "useTabs": true,
    "singleQuote": true
  },
  "detox": {
    "test-runner": "jest",
    "configurations": {
      "ios.sim.dev": {
        "binaryPath": "bin/Exponent.app",
        "type": "ios.simulator",
        "name": "iPhone 8"
      },
      "ios.sim.staging": {
        "binaryPath": "binaryApp/kahun-mobile-staging.app",
        "type": "ios.simulator",
        "name": "iPhone 8"
      }
    }
  },
  "rnpm": {
    "assets": [
      "./assets/fonts/"
    ]
  }
}
oriharel commented 5 years ago

I've opened a PR #4005

oriharel commented 5 years ago

So this issue is actually not an Amplify issue, but an expo one. For some reason, expo standalone apps fires multiple events on deep linking on Android. There is an issue on this matter here. Perhaps @brentvatne can take a look. However, Amplify can provide a hook to workaround this as suggested in the PR (#4005).

Thanks.

schellack commented 4 years ago

I have this issue as well on standalone Android builds, generated using Expo. I can see the error when connecting to a debugger (shows up a couple times before the app crashes):

2019-09-18 18:14:56.861 17177-17500/? E/ReactNativeJS: '[ERROR] 14:56.849 OAuth - Error handling auth response.', { [Error: invalid_grant] line: 978, column: 4017, sourceURL: '/data/user/0/redactedappslug/files/34.0.0/cached-bundle-experience-%40redacted%2Fappnameredacted-1424113275-34.0.0' }

oriharel commented 4 years ago

@schellack Yes, this is caused by multiple calls to the TOKEN ENDPOINT of Cognito. theis endpoint can be called only once (no matter if fails or succeeded) and consecutive calls result in an invalid grant (which means that the code used in the request is no longer valid)

schellack commented 4 years ago

I can see that's what's happening. Do you know of a workaround @oriharel?

oriharel commented 4 years ago

I ended up overriding Linking.addEventListener function and wrapped the handler passed as a property. in the override I ignore multiple calls with the same 'url' value. Hopefully we'll have amplify provide the urlListener as a configuration parameter so we won't have to do this ugly workaround.

loganwedwards commented 4 years ago

@oriharel I have been following all matter of oauth flow-related issues (FB, specifically) with an ejected Expo 33 app and just stumbled upon this issue. I believe I am experiencing the exact same root problem on iOS. I went as far as attempting to manually monkey patch it without success and your linked PR looks like a nice improvement. When you specifically mention, you overriding Linking.addEventListener, would you mind sharing any specific details? Did you override the function globally?

loganwedwards commented 4 years ago

@oriharel @schellack I dug into this a bit more and discovered the issues is actually partly my fault in how I installed the project or an architectural one on the Amplify side. I disccovered that the urlListener is in fact executing the code flow twice, but it was only happening once in @aws-amplify/auth. Come to find out, there is a separate node_modules directory under aws-amplify (seems to be required no matter what by aws-amplify-react-native and another under @aws-amplify/api. The extra urlListeners are actually exectuting the code flow as well. I nuked the node modules directory since amplify is correct using the require('@aws-amplify/auth'), so it does end up resolving to the correct issues.

Related, this also fixed my problem where Amplify complained that .configure was not called (or not called correctly), which now makes sense since there were multiple copies of the Auth service installed. I tried yarn, which may have cleared all of this up, but it introduced other issues in my detached Expo project, so I reverted to using npm and manually deleting the requisite node_modules.

Here is my (relevant) bits of the package.json:

"dependencies": {
    "@aws-amplify/api": "^1.0.42", // manually deleted these node_modules
    "@aws-amplify/auth": "1.2.31",
    "@aws-amplify/core": "^1.1.0",
    "@aws-amplify/storage": "^1.1.0",
    "aws-amplify": "^1.1.36", // manually deleted these node_modules
    "aws-amplify-react-native": "2.1.16",
    "aws-appsync": "1.8.1",
    "aws-appsync-react": "1.2.9",
   ...
}

It seems there may be a conflict with using both the modularized packages @aws-amplify and the original package aws-amplify. I hope this helps as I have been struggling with this issue for months.

Any suggestions from @manueliglesias or other members of the Amplify team on a non-hacked resolution?

schellack commented 4 years ago

Interesting. Any particular reason why you are using the scoped package imports (e.g., @aws-amplify/auth) and also importing the full aws-amplify library?

I will take another look at my project to see if that's at all related to the issue I see. Personally I only use the scoped imports.

    "@aws-amplify/analytics": "~1.2.25",
    "@aws-amplify/auth": "~1.3.3",
    "@aws-amplify/core": "~1.1.2",
loganwedwards commented 4 years ago

It's been a while, but I think I was trying to trim my deps down (I only need API, Auth, Core) instead of everything (don't use Analytics, MQTT, Chatbot, etc...). The idea was to get rid of just aws-amplify, but it seems it's required by the Amplify RN package.

@schellack Are you using React Native?

schellack commented 4 years ago

For what it's worth, I don't see multiple code flow events. I track every auth event published to the Hub on the 'auth' channel and only see a single codeFlow event.

@loganwedwards if you are still seeing multiple codeFlow events, then you should probably look at only using the scoped packages (and removing aws-amplify) or only use aws-amplify (and removing scoped packages).

My app is all React Native, but I do not import the Amplify React Native package. Instead I directly use a modified version of withOAuth. Since I'm using the Amplify hosted UI, there was no need for me to use any of the other stuff in the package, as all the rest of it is UI-related.

The original behavior described above by @oriharel still occurs for me when running on actual Android hardware (not an emulator).

Here are the steps that occur:

  1. Import @aws-amplify/auth (elsewhere I already imported the Hub from @aws-amplify/core and subscribed to its auth events).
  2. Configure amplify auth: Auth.configure({...
  3. Call custom provider signin (in my case; in oriharel's case he was using the Google provider, I believe)
  4. Amplify calls the configured urlOpener to prompt the user to sign in. In step 1, above, I configured the urlOpener to use await WebBrowser.openAuthSessionAsync(url, redirectUrl), where WebBrowser is imported from expo-web-browser. This is per the Amplify instructions for Expo users.
  5. The Android device opens a Chrome Custom Tab and takes the user the the auth URL.
  6. The user signs in and is redirected back to the app (via the redirectUrl parameter that was passed to WebBrowser.openAuthSessionAsync in step 4).
  7. Expo returns the dismiss result type as part of the result returned from WebBrowser.openAuthSessionAsync.
  8. Meanwhile Amplify is parsing things (I see the parsingCallbackUrl event published on the Hub).
  9. WebBrowser.openAuthSessionAsync then inexplicably returns again. This is not visible on the screen. The urlOpener I configured was not called again (I check for that), but rather WebBrowser is apparently acting up (the underlying problem here).

The above pull request is a workaround to allow us to handle this before the url is called and stop the extra callback calls that otherwise happen in the urlListener.

loganwedwards commented 4 years ago

It seems the actual errors/flows are subtly different in our use cases, but I am sure the steps you outlined above will help other Android users.

sammartinez commented 4 years ago

The above Pull Request that is merged in seems to resolve this issue. @oriharel Can you confirm this?

stale[bot] commented 4 years ago

This issue has been automatically closed because of inactivity. Please open a new issue if are still encountering problems.

github-actions[bot] commented 3 years ago

This issue has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels or Discussions for those types of questions.