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

clock drift - not getting an error thrown from endpoint #11675

Closed MatiasFacio-ParkHere closed 3 months ago

MatiasFacio-ParkHere commented 1 year ago

Before opening, please confirm:

JavaScript Framework

React Native

Amplify APIs

Authentication

Amplify Categories

No response

Environment information

``` # Put output below this line System: OS: macOS 13.4.1 CPU: (8) arm64 Apple M1 Memory: 561.05 MB / 16.00 GB Shell: 5.9 - /bin/zsh Binaries: Node: 16.19.1 - ~/.nvm/versions/node/v16.19.1/bin/node Yarn: 1.22.19 - ~/.nvm/versions/node/v16.19.1/bin/yarn npm: 8.19.3 - ~/.nvm/versions/node/v16.19.1/bin/npm Browsers: Brave Browser: 93.1.29.81 Chrome: 114.0.5735.198 Chrome Canary: 117.0.5909.2 Firefox: 112.0.2 Firefox Developer Edition: 104.0 Safari: 16.5.2 npmPackages: @babel/core: ^7.19.3 => 7.21.8 (7.12.13, 7.19.3, 7.20.2, 7.9.0) @expo/webpack-config: ^18.0.1 => 18.0.4 @microsoft/teams-js: ^1.10.0 => 1.13.1 @react-native-async-storage/async-storage: ~1.17.11 => 1.17.12 @react-native-community/masked-view: ^0.1.10 => 0.1.11 @react-native-community/netinfo: 9.3.7 => 9.3.7 @react-navigation/native: ^5.9.0 => 5.9.8 @react-navigation/stack: ^5.13.0 => 5.14.9 @sentry/react-native: 4.13.0 => 4.13.0 @testing-library/jest-dom: ^5.16.5 => 5.16.5 @testing-library/jest-native: ^5.4.2 => 5.4.2 @testing-library/react-native: ^12.1.2 => 12.1.2 @types/iban: ^0.0.32 => 0.0.32 @types/jest: ^26.0.0 => 26.0.24 (29.5.1) @types/lodash.isequal: ^4.5.6 => 4.5.6 @types/react: ~18.0.24 => 18.0.38 (16.14.41, 18.2.6) @types/react-dom: ~18.0.8 => 18.0.11 @types/react-native-modalbox: ^1.4.8 => 1.4.10 @types/styled-components: ^4.4.2 => 4.4.3 HelloWorld: 0.0.1 amazon-cognito-identity-js: ^5.2.4 => 5.2.14 aws-amplify: ^4.3.32 => 4.3.46 country-codes-list: ^1.6.8 => 1.6.10 date-time-format-timezone: ^1.0.22 => 1.0.22 dayjs: ^1.10.7 => 1.11.7 (1.11.5) eslint: ^7.32.0 => 7.32.0 eslint-config-react-native-wcandillon: ^3.7.2 => 3.10.0 expo: ^48.0.0 => 48.0.17 expo-application: ~5.1.1 => 5.1.1 expo-brightness: ~11.2.1 => 11.2.1 expo-checkbox: ~2.3.1 => 2.3.1 expo-constants: ~14.2.1 => 14.2.1 expo-device: ~5.2.1 => 5.2.1 expo-font: ~11.1.1 => 11.1.1 expo-intent-launcher: ^10.5.2 => 10.5.2 expo-keep-awake: ~12.0.1 => 12.0.1 expo-linking: ~4.0.1 => 4.0.1 expo-localization: ~14.1.1 => 14.1.1 expo-splash-screen: ~0.18.2 => 0.18.2 expo-status-bar: ~1.4.4 => 1.4.4 expo-updates: ~0.16.4 => 0.16.4 expo-web-browser: ~12.1.1 => 12.1.1 formik: ^2.2.9 => 2.2.9 i18next: ^19.7.0 => 19.9.2 iban: ^0.0.12 => 0.0.12 jest: ^29.5.0 => 29.5.0 jest-date-mock: ^1.0.8 => 1.0.8 jest-expo: ^48.0.0 => 48.0.2 jest-extended: ^1.1.0 => 1.2.1 jest-junit: ^13.0.0 => 13.2.0 lodash.isequal: ^4.5.0 => 4.5.0 mockdate: ^3.0.5 => 3.0.5 react: 18.2.0 => 18.2.0 react-dom: 18.2.0 => 18.2.0 react-hooks-testing-library: ^0.6.0 => 0.6.0 react-i18next: ^11.18.6 => 11.18.6 react-native: 0.71.8 => 0.71.8 react-native-gesture-handler: ~2.9.0 => 2.9.0 react-native-image-zoom-viewer: 3.0.1 => 3.0.1 react-native-qrcode-svg: ^6.0.6 => undefined (6.2.0, ) react-native-reanimated: ~2.14.4 => 2.14.4 react-native-safe-area-context: 4.5.0 => 4.5.0 react-native-screens: ~3.20.0 => 3.20.0 react-native-svg: 13.4.0 => 13.4.0 react-native-web: ~0.18.7 => 0.18.12 react-native-webview: 11.26.0 => 11.26.0 react-query: ^2.25.2 => 2.26.4 sentry-expo: ~6.1.0 => 6.1.1 styled-components: ^4.4.1 => 4.4.1 styled-components/macro: undefined () styled-components/native: undefined () styled-components/primitives: undefined () typescript: ^4.6.3 => 4.9.5 (5.0.3) yarn-audit-fix: ^9.3.7 => 9.3.10 npmGlobalPackages: corepack: 0.15.1 npm: 8.19.3 yarn: 1.22.19 ```

Describe the bug

When a user tried to login, we used to catch an error that was thrown when the clock was drifted. Did something change and now the error is not being thrown anymore? The issue for me is that users can now login even if in their devices they set a date in the future.

Expected behavior

An error is thrown if the clock has drifted.

Reproduction steps

1- change date in your device, making it to be in the future (e.g.: 2 weeks). 2- make a call to the api where user logs in inside a try / catch block 3- expect the catch block to be executed as an error should be thrown in when calling the API.

Code Snippet

try {
    const user = await API.get('user', '/me', {});
    return user
  } catch (err: any) {
    const isClockDrift = checkClockDriftError(err);
    if (isClockDrift || isTimeZoneError) {
      showAuthAlert({ code: 'CLOCK_DRIFT_ERROR' });
    } else {
      const isMissingAuthenticationToken =
        checkIfMissingAuthenticationToken(err);
      if (!isMissingAuthenticationToken) {
        showAuthAlert(err);
      }
    }
  }

Log output

``` // Put your logs below this line ```

aws-exports.js

No response

Manual configuration

No response

Additional configuration

No response

Mobile Device

No response

Mobile Operating System

No response

Mobile Browser

No response

Mobile Browser Version

No response

Additional information and screenshots

No response

MatiasFacio-ParkHere commented 1 year ago
try {
    const user = await API.get('user', '/me', {});
    return user
  } catch (err: any) {
    const isClockDrift = checkClockDriftError(err);
    if (isClockDrift || isTimeZoneError) {
      showAuthAlert({ code: 'CLOCK_DRIFT_ERROR' });
    } else {
      const isMissingAuthenticationToken =
        checkIfMissingAuthenticationToken(err);
      if (!isMissingAuthenticationToken) {
        showAuthAlert(err);
      }
    }
  }
}
israx commented 1 year ago

Hey @MatiasFacio-ParkHere . I'm sorry that you are experiencing this issue. What is the amplify version you transitioned from where you were able to capture errors when the clockDrift was modified?

MatiasFacio-ParkHere commented 1 year ago

@israx thank you for getting back. We did not upgrade (I did it only to check if it would work or something). We are using aws-amplify: ^4.3.32 => 4.3.46

MatiasFacio-ParkHere commented 1 year ago

@cwomack a bit more context: We are not using amplify application. so there is no ARN. We are using amplify.js SDK for our react-native custom web-app. Login mechanism into your Amplify application: in the login, we are using amplify js, sign-in function. It is communicating our cognito pool directly and authenticating users.

Framework: We are using react-native expo.dev and amplify js.

israx commented 1 year ago

Hey @MatiasFacio-ParkHere . I just want to confirm, did you use to encounter errors when clockDrift was manipulated ? Or is it something that you'd expect to happen. Otherwise we can treat this issue as a feature request.

MatiasFacio-ParkHere commented 1 year ago

@israx if someone manipulated the clockdrift, aws would throw an error on login that we would catch, check the message, and show an alert to the user. This was and is as of now our workflow. Our users can do reservations, but if for any reason their clock was drifted, they could make reservations in the future which is wrong.

cwomack commented 1 year ago

@MatiasFacio-ParkHere, could you try upgrading to the latest version of amplify to see if the issue persists? It's possible that this behavior is occurring due to differing support and interactions with the aws-sdk in previous, older versions like 4.3.x that you are currently using.

MatiasFacio-ParkHere commented 1 year ago

hey @cwomack, we are using aws-amplify and Expo. I did update this library but the problem persist. I started to think that the problem is not on your end. It works when we run web environment, but it doesn't throw when on iOS or Android: I don't see 403 in this case. I am investigating if the issue might be related to Expo.

cwomack commented 1 year ago

@MatiasFacio-ParkHere, let me know if you find anything related to Expo... but I'd like to be sure we can rule out that it's not on Amplify's side. Are you experiencing it with all Auth API calls? Or or with specific ones (and if so, which)?

MatiasFacio-ParkHere commented 1 year ago

hey @cwomack, I couldn't find anything yet. I don't understand why the 403 error that Amplify throws (silently) will be caught by in the web-app, but not in the iOS/Android devices (always using Expo). What is your opinion, should we relay on Amplify implementation to prevent the user to login in when he had manipulated the device clock, or we should better implement our own mechanism?

cwomack commented 1 year ago

@MatiasFacio-ParkHere, I believe this should now be fixed as of v5.3.10 of Amplify. Can you see if upgrading to the most recent version resolves the problem, please?

MatiasFacio-ParkHere commented 1 year ago

@cwomack I will check and let you know, Thank you!

MatiasFacio-ParkHere commented 1 year ago

@cwomack I upgraded as you suggested and we still have the same issue. As I already mentioned, we use expo. When we provoque a clockdrift, we can only catch the 403 error returned by amplify on the web, but not on the simulator, nor in our test app. This was not the case before: we could always catch 403 error for clock drifting in all platforms.

MatiasFacio-ParkHere commented 1 year ago

@cwomack when I log the response after running Auth.currentSession (aws amplify), the clockDrift value, after having changed it to 1 day in the future, is 86396 which I thought would be milliseconds, but it is actually seconds which represents nearly one day. Do you think that it is possible that there is an issue right there? That somewhere in the code, the expectation is to calculate based on milliseconds but finally it is done by mistake using seconds?

cwomack commented 5 months ago

Possibly related to #9281

ashika112 commented 3 months ago

@MatiasFacio-ParkHere It looks like you are expecting clock drift to throw an error, that would be a feature request for us. Currently, the library looks for clock skews and corrects the skew to make the API call this is expected per aws-sdk-js-v3

Would you want the feature request to be open?

MatiasFacio-ParkHere commented 3 months ago

@ashika112 thank you for your answer! And no, we don't want to request feature.

cwomack commented 3 months ago

@MatiasFacio-ParkHere, thanks for the reply and confirmation. We'll close this out!